From: Linus Arver Date: Mon, 07 Aug 2023 05:28:43 GMT Subject: Re: [PATCH 3/5] trailer: add tests to check defaulting behavior with --no-* flags Message-ID: In-Reply-To: Junio C Hamano writes: > "Linus Arver via GitGitGadget" 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 "" 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 . However, it defines the relevant configuration section like this: trailer..key:: This `key` will be used instead of 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 and the default separator when adding the trailer. So if I configure this like git config trailer.ack.key "Acked-by" && the is both the longer-form "Acked-by:" (per the meaning so far in the doc) but also the shorter string "ack" per the "trailer..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 [(=|:)])...] [--parse] [...] because the 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 "" with "" while leaving the "trailer..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.