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

[Linus Arver <linusa@google.com>]

Junio C Hamano <gitster@pobox.com> writes:

> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> @@ -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.