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

From: Linus Arver <linusa@google.com>
Date: Aug 7, 2023, 05:28 UTC
Message-ID: <owlyfs4vbeus.fsf@fine.c.googlers.com>
In-Reply-To: <xmqqjzu7irhw.fsf@gitster.g>

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

Show 9 quoted lines

> "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.