Re: [PATCH 3/5] trailer: add tests to check defaulting behavior with --no-* flags

Junio C Hamano [off-list ref] writes:

"Linus Arver via GitGitGadget" [off-list ref] writes:

quoted

@@ -114,8 +114,10 @@ OPTIONS
 	Specify where all new trailers will be added.  A setting
 	provided with '--where' overrides all configuration variables

Obviously this is not a new issue, but "all configuration variables"
is misleading (the same comment applies to the description of the
"--[no-]if-exists" and the "--[no-]if-missing" options).

Agreed.

If I am reading the code correctly, --where=value overrides the
trailer.where variable and nothing else, and --no-where stops the
overriding of the trailer.where variable.  Ditto for the other two
with their relevant configuration variables.

That is also my understanding. Will update to remove the "all" wording.

On a separate note, I've realized there are more fixes to be done in
this area (as I get more familiar with the codebase). For example, we
have the following language in builtin/interpret-trailers.c inside
cmd_interpret_trailers():

    OPT_BOOL(0, "only-input", &opts.only_input, N_("do not apply config rules")),

which should be fixed in similar style to what you suggested above,
probably with:

    OPT_BOOL(0, "only-input", &opts.only_input, N_("do not apply trailer.* configuration variables")),

When I reroll, I will include these additional fixes so expect the patch
series to grow (probably ~12 patches instead of the ~5).

One more thing. I think the documentation
(Documentation/git-interpret-trailers.txt) uses the word "<token>" in
two different ways. For example, if we have in the input

    subject line

    body text

    Acked-by: Foo

the docs treat the word "Acked-by:" as the <token>. However, it defines
the relevant configuration section like this:

    trailer.<token>.key::
            This `key` will be used instead of <token> in the trailer. At
            the end of this key, a separator can appear and then some
            space characters. By default the only valid separator is ':',
            but this can be changed using the `trailer.separators` config
            variable.
    +
    If there is a separator, then the key will be used instead of both the
    <token> and the default separator when adding the trailer.

So if I configure this like

   git config trailer.ack.key "Acked-by" &&

the <token> is both the longer-form "Acked-by:" (per the meaning so far
in the doc) but also the shorter string "ack" per the
"trailer.<token>.key" configuration section syntax. This secondary
meaning is repeated again in the very start of the doc when we define
the --trailer option syntax as

    SYNOPSIS
    --------
    [verse]
    'git interpret-trailers' [--in-place] [--trim-empty]
                [(--trailer <token>[(=|:)<value>])...]
                [--parse] [<file>...]

because the <token> here could be (using the example above) either
"Acked-by" (as in "--trailer=Acked-by:...") if we did not configure
"trailer.ack.key", or just "ack" (as in "--trailer=ack:...") if we did
configure it. These two scenarios would give identical "Acked-by: ..."
output.

This is confusing and I don't like how we overload this "token" word
(not to mention we already have the word "key" which we don't really use
much in the docs).

I am inclined to replace most uses of the word "<token>" with "<key>"
while leaving the "trailer.<token>.key" configuration syntax intact.
This will result in a large diff but I think the removal of the double
meaning is worth it, and will include this fix also in the next reroll.

The main reason I bring this up is because this means also having to
update our funciton names like "token_len_without_separator" in
trailer.c, to be "key_len_without_separator" if we want the nomenclature
in the trailer.c internals to be consistent with the (updated)
user-facing docs. I am not sure whether we want to do this as part of
the same reroll, or if we should leave it as #leftoverbits for a future
series.