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

Commit Message

Linus Arver Sept. 7, 2023, 10:20 p.m. UTC

From: Linus Arver <[e-mail]>

The `--trailer` option takes a "<token>=<value>" argument, for example

    --trailer "Acked-by=Bob"

And in this exampple it is understood that "Acked-by" is the <token>.
However, the user can use a shorter "ack" string by defining
configuration like

    git config trailer.ack.key "Acked-by"

However, in the docs we define the above configuration as

    trailer.<token>.key

so the <token> can mean either the longer "Acked-by" or the shorter
"ack".

Separate the two meanings of <token> into <key> and <keyAlias>, and
update the configuration syntax to say "trailer.<keyAlias>.key".

Signed-off-by: Linus Arver <[e-mail]>
---
 Documentation/git-interpret-trailers.txt | 136 +++++++++++++----------
 builtin/interpret-trailers.c             |   2 +-
 2 files changed, 77 insertions(+), 61 deletions(-)

Comments

Jonathan Tan Sept. 19, 2023, 10:59 p.m. UTC | #1

"Linus Arver via GitGitGadget" <[e-mail]> 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. 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. I do think that the new names make
more sense, though.

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

Linus Arver Sept. 20, 2023, 6:48 a.m. UTC | #2

Jonathan Tan <[e-mail]> writes:

> "Linus Arver via GitGitGadget" <[e-mail]> 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.

Junio C Hamano Sept. 20, 2023, 3:01 p.m. UTC | #3

Jonathan Tan <[e-mail]> writes:

> "Linus Arver via GitGitGadget" <[e-mail]> 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. 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. I do think that the new names make
> more sense, though.

As long as the new names describe the world order better than the
old description, I do not mind rephrasing the documentation, and you
seem to find the more descriptive <keyAlias> easier to understand
compared to the non-descriptive <token>.  Adding a concrete example
(ack vs acked-by) is also a good change.

Linus Arver Sept. 22, 2023, 6:26 p.m. UTC | #4

Junio C Hamano <[e-mail]> writes:

> Jonathan Tan <[e-mail]> writes:
>
>> "Linus Arver via GitGitGadget" <[e-mail]> 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. 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. I do think that the new names make
>> more sense, though.
>
> As long as the new names describe the world order better than the
> old description, I do not mind rephrasing the documentation, and you
> seem to find the more descriptive <keyAlias> easier to understand
> compared to the non-descriptive <token>.  Adding a concrete example
> (ack vs acked-by) is also a good change.

SGTM (i.e., I am OK to keep this patch as is). So I will not re-roll.

In the future I will be cleaning up more of the trailer system to make
use of the disinction between the key vs keyAlias where appropriate.

Thanks!

Teng Long Nov. 10, 2023, 6:33 a.m. UTC | #5

Linus Arver <[e-mail]> writes:

> diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
> index 25433e1a1ff..418265f044d 100644
> --- a/Documentation/git-interpret-trailers.txt
> +++ b/Documentation/git-interpret-trailers.txt
> @@ -9,7 +9,7 @@ SYNOPSIS
>  --------
>  [verse]
>  'git interpret-trailers' [--in-place] [--trim-empty]
> -			[(--trailer <token>[(=|:)<value>])...]
> +			[(--trailer (<key>|<keyAlias>)[(=|:)<value>])...]

Maybe use 'key-alias' instead of 'keyAlias'? After checking the naming in some
other documents, it seems that they are basically in the former format, not
camel case format.

Thanks.

Patch

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 25433e1a1ff..418265f044d 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -9,7 +9,7 @@  SYNOPSIS
 --------
 [verse]
 'git interpret-trailers' [--in-place] [--trim-empty]
-			[(--trailer <token>[(=|:)<value>])...]
+			[(--trailer (<key>|<keyAlias>)[(=|:)<value>])...]
 			[--parse] [<file>...]
 
 DESCRIPTION
@@ -53,22 +53,32 @@  are applied to each input and the way any existing trailer in
 the input is changed. They also make it possible to
 automatically add some trailers.
 
-By default, a '<token>=<value>' or '<token>:<value>' argument given
+By default, a '<key>=<value>' or '<key>:<value>' argument given
 using `--trailer` will be appended after the existing trailers only if
-the last trailer has a different (<token>, <value>) pair (or if there
-is no existing trailer). The <token> and <value> parts will be trimmed
+the last trailer has a different (<key>, <value>) pair (or if there
+is no existing trailer). The <key> and <value> parts will be trimmed
 to remove starting and trailing whitespace, and the resulting trimmed
-<token> and <value> will appear in the output like this:
+<key> and <value> will appear in the output like this:
 
 ------------------------------------------------
-token: value
+key: value
 ------------------------------------------------
 
-This means that the trimmed <token> and <value> will be separated by
-`': '` (one colon followed by one space). For convenience, the <token> can be a
-shortened string key (e.g., "sign") instead of the full string which should
-appear before the separator on the output (e.g., "Signed-off-by"). This can be
-configured using the 'trailer.<token>.key' configuration variable.
+This means that the trimmed <key> and <value> will be separated by
+`': '` (one colon followed by one space).
+
+For convenience, a <keyAlias> can be configured to make using `--trailer`
+shorter to type on the command line. This can be configured using the
+'trailer.<keyAlias>.key' configuration variable. The <keyAlias> must be a prefix
+of the full <key> string, although case sensitivity does not matter. For
+example, if you have
+
+------------------------------------------------
+trailer.sign.key "Signed-off-by: "
+------------------------------------------------
+
+in your configuration, you only need to specify `--trailer="sign: foo"`
+on the command line instead of `--trailer="Signed-off-by: foo"`.
 
 By default the new trailer will appear at the end of all the existing
 trailers. If there is no existing trailer, the new trailer will appear
@@ -85,14 +95,14 @@  non-whitespace lines before a line that starts with '---' (followed by a
 space or the end of the line).
 
 When reading trailers, there can be no whitespace before or inside the
-<token>, but any number of regular space and tab characters are allowed
-between the <token> and the separator. There can be whitespaces before,
+<key>, but any number of regular space and tab characters are allowed
+between the <key> and the separator. There can be whitespaces before,
 inside or after the <value>. The <value> may be split over multiple lines
 with each subsequent line starting with at least one whitespace, like
 the "folding" in RFC 822. Example:
 
 ------------------------------------------------
-token: This is a very long value, with spaces and
+key: This is a very long value, with spaces and
   newlines in it.
 ------------------------------------------------
 
@@ -109,8 +119,8 @@  OPTIONS
 	the whole trailer will be removed from the output.
 	This applies to existing trailers as well as new trailers.
 
---trailer <token>[(=|:)<value>]::
-	Specify a (<token>, <value>) pair that should be applied as a
+--trailer <key>[(=|:)<value>]::
+	Specify a (<key>, <value>) pair that should be applied as a
 	trailer to the inputs. See the description of this
 	command.
 
@@ -118,7 +128,7 @@  OPTIONS
 --no-where::
 	Specify where all new trailers will be added.  A setting
 	provided with '--where' overrides the `trailer.where` and any
-	applicable `trailer.<token>.where` configuration variables
+	applicable `trailer.<keyAlias>.where` configuration variables
 	and applies to all '--trailer' options until the next occurrence of
 	'--where' or '--no-where'. Upon encountering '--no-where', clear the
 	effect of any previous use of '--where', such that the relevant configuration
@@ -128,9 +138,9 @@  OPTIONS
 --if-exists <action>::
 --no-if-exists::
 	Specify what action will be performed when there is already at
-	least one trailer with the same <token> in the input.  A setting
+	least one trailer with the same <key> in the input.  A setting
 	provided with '--if-exists' overrides the `trailer.ifExists` and any
-	applicable `trailer.<token>.ifExists` configuration variables
+	applicable `trailer.<keyAlias>.ifExists` configuration variables
 	and applies to all '--trailer' options until the next occurrence of
 	'--if-exists' or '--no-if-exists'. Upon encountering '--no-if-exists, clear the
 	effect of any previous use of '--if-exists, such that the relevant configuration
@@ -140,9 +150,9 @@  OPTIONS
 --if-missing <action>::
 --no-if-missing::
 	Specify what action will be performed when there is no other
-	trailer with the same <token> in the input.  A setting
+	trailer with the same <key> in the input.  A setting
 	provided with '--if-missing' overrides the `trailer.ifMissing` and any
-	applicable `trailer.<token>.ifMissing` configuration variables
+	applicable `trailer.<keyAlias>.ifMissing` configuration variables
 	and applies to all '--trailer' options until the next occurrence of
 	'--if-missing' or '--no-if-missing'. Upon encountering '--no-if-missing,
 	clear the effect of any previous use of '--if-missing, such that the relevant
@@ -187,11 +197,11 @@  used when another separator is not specified in the config for this
 trailer.
 +
 For example, if the value for this option is "%=$", then only lines
-using the format '<token><sep><value>' with <sep> containing '%', '='
+using the format '<key><sep><value>' with <sep> containing '%', '='
 or '$' and then spaces will be considered trailers. And '%' will be
 the default separator used, so by default trailers will appear like:
-'<token>% <value>' (one percent sign and one space will appear between
-the token and the value).
+'<key>% <value>' (one percent sign and one space will appear between
+the key and the value).
 
 trailer.where::
 	This option tells where a new trailer will be added.
@@ -205,41 +215,41 @@  If it is `start`, then each new trailer will appear at the start,
 instead of the end, of the existing trailers.
 +
 If it is `after`, then each new trailer will appear just after the
-last trailer with the same <token>.
+last trailer with the same <key>.
 +
 If it is `before`, then each new trailer will appear just before the
-first trailer with the same <token>.
+first trailer with the same <key>.
 
 trailer.ifexists::
 	This option makes it possible to choose what action will be
 	performed when there is already at least one trailer with the
-	same <token> in the input.
+	same <key> in the input.
 +
 The valid values for this option are: `addIfDifferentNeighbor` (this
 is the default), `addIfDifferent`, `add`, `replace` or `doNothing`.
 +
 With `addIfDifferentNeighbor`, a new trailer will be added only if no
-trailer with the same (<token>, <value>) pair is above or below the line
+trailer with the same (<key>, <value>) pair is above or below the line
 where the new trailer will be added.
 +
 With `addIfDifferent`, a new trailer will be added only if no trailer
-with the same (<token>, <value>) pair is already in the input.
+with the same (<key>, <value>) pair is already in the input.
 +
 With `add`, a new trailer will be added, even if some trailers with
-the same (<token>, <value>) pair are already in the input.
+the same (<key>, <value>) pair are already in the input.
 +
-With `replace`, an existing trailer with the same <token> will be
+With `replace`, an existing trailer with the same <key> will be
 deleted and the new trailer will be added. The deleted trailer will be
-the closest one (with the same <token>) to the place where the new one
+the closest one (with the same <key>) to the place where the new one
 will be added.
 +
 With `doNothing`, nothing will be done; that is no new trailer will be
-added if there is already one with the same <token> in the input.
+added if there is already one with the same <key> in the input.
 
 trailer.ifmissing::
 	This option makes it possible to choose what action will be
 	performed when there is not yet any trailer with the same
-	<token> in the input.
+	<key> in the input.
 +
 The valid values for this option are: `add` (this is the default) and
 `doNothing`.
@@ -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.
 +
 If there is a separator in the key, then it overrides the default
 separator when adding the trailer.
 
-trailer.<token>.where::
+trailer.<keyAlias>.where::
 	This option takes the same values as the 'trailer.where'
 	configuration variable and it overrides what is specified by
-	that option for trailers with the specified <token>.
+	that option for trailers with the specified <keyAlias>.
 
-trailer.<token>.ifexists::
+trailer.<keyAlias>.ifexists::
 	This option takes the same values as the 'trailer.ifexists'
 	configuration variable and it overrides what is specified by
-	that option for trailers with the specified <token>.
+	that option for trailers with the specified <keyAlias>.
 
-trailer.<token>.ifmissing::
+trailer.<keyAlias>.ifmissing::
 	This option takes the same values as the 'trailer.ifmissing'
 	configuration variable and it overrides what is specified by
-	that option for trailers with the specified <token>.
+	that option for trailers with the specified <keyAlias>.
 
-trailer.<token>.command::
-	Deprecated in favor of 'trailer.<token>.cmd'.
-	This option behaves in the same way as 'trailer.<token>.cmd', except
+trailer.<keyAlias>.command::
+	Deprecated in favor of 'trailer.<keyAlias>.cmd'.
+	This option behaves in the same way as 'trailer.<keyAlias>.cmd', except
 	that it doesn't pass anything as argument to the specified command.
 	Instead the first occurrence of substring $ARG is replaced by the
 	<value> that would be passed as argument.
@@ -283,29 +299,29 @@  trailer.<token>.command::
 Note that $ARG in the user's command is
 only replaced once and that the original way of replacing $ARG is not safe.
 +
-When both 'trailer.<token>.cmd' and 'trailer.<token>.command' are given
-for the same <token>, 'trailer.<token>.cmd' is used and
-'trailer.<token>.command' is ignored.
+When both 'trailer.<keyAlias>.cmd' and 'trailer.<keyAlias>.command' are given
+for the same <keyAlias>, 'trailer.<keyAlias>.cmd' is used and
+'trailer.<keyAlias>.command' is ignored.
 
-trailer.<token>.cmd::
+trailer.<keyAlias>.cmd::
 	This option can be used to specify a shell command that will be called
-	once to automatically add a trailer with the specified <token>, and then
-	called each time a '--trailer <token>=<value>' argument is specified to
+	once to automatically add a trailer with the specified <keyAlias>, and then
+	called each time a '--trailer <keyAlias>=<value>' argument is specified to
 	modify the <value> of the trailer that this option would produce.
 +
 When the specified command is first called to add a trailer
-with the specified <token>, the behavior is as if a special
-'--trailer <token>=<value>' argument was added at the beginning
+with the specified <keyAlias>, the behavior is as if a special
+'--trailer <keyAlias>=<value>' argument was added at the beginning
 of the "git interpret-trailers" command, where <value>
 is taken to be the standard output of the command with any
 leading and trailing whitespace trimmed off.
 +
-If some '--trailer <token>=<value>' arguments are also passed
+If some '--trailer <keyAlias>=<value>' arguments are also passed
 on the command line, the command is called again once for each
-of these arguments with the same <token>. And the <value> part
+of these arguments with the same <keyAlias>. And the <value> part
 of these arguments, if any, will be passed to the command as its
 first argument. This way the command can produce a <value> computed
-from the <value> passed in the '--trailer <token>=<value>' argument.
+from the <value> passed in the '--trailer <keyAlias>=<value>' argument.
 
 EXAMPLES
 --------
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 832f86a770a..d2d78fd961f 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -14,7 +14,7 @@ 
 
 static const char * const git_interpret_trailers_usage[] = {
 	N_("git interpret-trailers [--in-place] [--trim-empty]\n"
-	   "                       [(--trailer <token>[(=|:)<value>])...]\n"
+	   "                       [(--trailer (<key>|<keyAlias>)[(=|:)<value>])...]\n"
 	   "                       [--parse] [<file>...]"),
 	NULL
 };