Re: [PATCH v3 13/13] trailer doc: <token> is a <key> or <keyAlias>, not both

[Linus Arver <linusa@google.com>]

Jonathan Tan <jonathantanmy@google.com> writes:

> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> @@ -248,34 +258,40 @@ With `add`, a new trailer will be added.
>>  +
>>  With `doNothing`, nothing will be done.
>>  
>> -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.
>> +trailer.<keyAlias>.key::
>> +	Defines a <keyAlias> for the <key>. The <keyAlias> must be a
>> +	prefix (case does not matter) of the <key>. For example, in `git
>> +	config trailer.ack.key "Acked-by"` the "Acked-by" is the <key> and
>> +	the "ack" is the <keyAlias>. This configuration allows the shorter
>> +	`--trailer "ack:..."` invocation on the command line using the "ack"
>> +	<keyAlias> instead of the longer `--trailer "Acked-by:..."`.
>> ++
>> +At the end of the <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.
>
> I think all the other patches will be a great help to the user, but I'm
> on the fence about this one.

Ack. I'm OK with dropping this patch (I assume I don't need to reroll
this series for that?).

> Someone who knows these trailer components
> by their old names might be confused upon seeing tne new ones, so I'm
> inclined to minimize such changes.

True, I had some inclination to do the same kind of change but for the
trailer.c code also (that code uses the "token" language also,
furthering the ambiguity, sadly) but wanted to just do the user-facing
part first because I thought the users shouldn't need to know about the
internals anyway.

If I did revise this patch to include the same changes in trailer.c and
not just the docs, would you be more willing to support the (new) patch?
I'm mainly curious about what will make you more comfortable to accept
this change.

> I do think that the new names make
> more sense, though.

Thanks!

> The documentation doesn't seem to say what happens when trailer.ack.cmd
> and trailer.Acked-by.cmd (replace "cmd" with whatever) are defined, but
> that was true previously too (and knowing this does not really enable
> the user to be able to do something they previously couldn't, so this
> is fine).

Ah interesting point. I'll try to remember this when I revisit this
patch again in the (near?) future.