Junio C Hamano <gitster@xxxxxxxxx> writes:
> "Linus Arver via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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.
