[v9,1/2,GSOC] docs: correct descript of trailer.<token>.command

Commit Message

ZheNing Hu April 12, 2021, 4:39 p.m. UTC

From: ZheNing Hu <[e-mail]>

In the original documentation of `trailer.<token>.command`,
some descriptions are easily misunderstood. So let's modify
it to increase its readability.

In addition, clarify that `$ARG` in command can only be
replaced once.

Signed-off-by: ZheNing Hu <[e-mail]>
---
 Documentation/git-interpret-trailers.txt | 37 ++++++++++++++----------
 1 file changed, 21 insertions(+), 16 deletions(-)

Comments

Junio C Hamano April 12, 2021, 8:42 p.m. UTC | #1

"ZheNing Hu via GitGitGadget" <[e-mail]> writes:

>  trailer.<token>.command::
> +	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
> +	each time a '--trailer <token>=<value>' argument to modify the <value> of
> +	the trailer that this option would produce.
>  +
> -When this option is specified, the behavior is as if a special
> +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
> +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.

So, with

	[trailer "foo"] command = "echo $ARG"

in your .git/config

    $ git interpret-trailers </dev/null

gives 'foo:'.

> +If some '--trailer <token>=<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, if any, will be used to replace the first
> +occurrence of substring `$ARG` in the command. This way the
> +command can produce a <value> computed from the <value> passed
> +in the '--trailer <token>=<value>' argument.
>  +
> +For consistency, the first occurrence of substring `$ARG` is
> +also replaced, this time with the empty string, in the command
> +when the command is first called to add a trailer with the
> +specified <token>.

And then

    $ git interpret-trailers --trailer=foo:F </dev/null

would give you

    foo:
    foo: F

The above is quite an easy to read explanation of the behaviour.

I somehow wonder if this "run with empty even without anything on
the command line" a misfeature, and I'd prefer to iron it out before
we add .cmd, because we may not want to inherit it to the new .cmd,
just like we avoided '$ARG' that does not properly quote and
replaces only once from getting inherited by .cmd mechanism.

The reason why I suspect this may be a misfeature is because I do
not see any way to avoid 'foo' trailer once trailer.foo.command is
set.  Which means I cannot use this mechanism to emulate "commit -s",
which would hopefully be something like

	[trailer "signoff"]
		command = "git var GIT_COMMITTER_IDENT | sed -e 's/>.*/>/"
		ifexists = addIfDifferentNeighbor

And trailer.signoff.ifmissing=donothing would not help in this case,
either, I am afraid, as that would not just suppress the automatic
empty one, which is fairly useless, but also suppresses the one that
is made in response to the option from the command line.

Christian?  What's your thought on this?

I can understand that it sometimes may be useful to unconditionally
be able to add a trailer without doing anything from the command
line, but it feels fairly useless that an empty one is automatically
added, that the only way to hide that empty one from the end result
is to use ifexists=replace, and that there is no apparent way to remove
the empty one.

The --trim-empty option is a bit too crude a band-aid to use, as the
existing log message may have an unrelated trailer for which it is
perfectly valid not to have any value.

The fix we'd do when introducing .cmd should also get rid of this
initial "run with an empty even when not asked"?

Or if the execution without any input from the command line were
truly useful sometimes, a configuration variable that disables this
behaviour, i.e.

	[trailer "signoff"]
		command = "git who"
		ifexists = addIfDifferentNeighbor
                implicitExecution = false		

so that

	git commit --trailer=signoff:Couder --trailer=signoff:gitster@

would give us two sign-offs without the empty one, perhaps?


In any case, the documentation update in this step looks reasonably
well written.

Thanks.

Christian Couder April 16, 2021, 12:03 p.m. UTC | #2

Sorry for the late answer.

On Mon, Apr 12, 2021 at 10:42 PM Junio C Hamano <[e-mail]> wrote:
>
> "ZheNing Hu via GitGitGadget" <[e-mail]> writes:
>
> >  trailer.<token>.command::
> > +     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
> > +     each time a '--trailer <token>=<value>' argument to modify the <value> of
> > +     the trailer that this option would produce.
> >  +
> > -When this option is specified, the behavior is as if a special
> > +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
> > +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.
>
> So, with
>
>         [trailer "foo"] command = "echo $ARG"
>
> in your .git/config
>
>     $ git interpret-trailers </dev/null
>
> gives 'foo:'.

Yeah:

------
$ git -c trailer.foo.command='echo $ARG' interpret-trailers </dev/null

foo:
------

> > +If some '--trailer <token>=<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, if any, will be used to replace the first
> > +occurrence of substring `$ARG` in the command. This way the
> > +command can produce a <value> computed from the <value> passed
> > +in the '--trailer <token>=<value>' argument.
> >  +
> > +For consistency, the first occurrence of substring `$ARG` is
> > +also replaced, this time with the empty string, in the command
> > +when the command is first called to add a trailer with the
> > +specified <token>.
>
> And then
>
>     $ git interpret-trailers --trailer=foo:F </dev/null
>
> would give you
>
>     foo:
>     foo: F

Yeah:

------
git -c trailer.foo.command='echo $ARG' interpret-trailers
--trailer=foo:F </dev/null

foo:
foo: F
------

> The above is quite an easy to read explanation of the behaviour.
>
> I somehow wonder if this "run with empty even without anything on
> the command line" a misfeature, and I'd prefer to iron it out before
> we add .cmd, because we may not want to inherit it to the new .cmd,
> just like we avoided '$ARG' that does not properly quote and
> replaces only once from getting inherited by .cmd mechanism.

I don't think it's a misfeature. I think it can be very useful in some
cases like with:

$ git config trailer.sign.command 'echo "$(git config user.name)
<$(git config user.email)>"'

My opinion is that we should have added a `trailer.<token>.runMode`
config option along with `trailer.<token>.command`. This was not
discussed unfortunately when ".command" was implemented, but it seems
to be a good idea now.

> The reason why I suspect this may be a misfeature is because I do
> not see any way to avoid 'foo' trailer once trailer.foo.command is
> set.

It can be avoided when the --trim-empty CLI option can be used. A hook
to remove empty trailers (which might call `git interpret-trailers
--trim-empty` itself) could also be used when --trim-empty cannot be
used directly.

> Which means I cannot use this mechanism to emulate "commit -s",
> which would hopefully be something like
>
>         [trailer "signoff"]
>                 command = "git var GIT_COMMITTER_IDENT | sed -e 's/>.*/>/"
>                 ifexists = addIfDifferentNeighbor
>
> And trailer.signoff.ifmissing=donothing would not help in this case,
> either, I am afraid, as that would not just suppress the automatic
> empty one, which is fairly useless, but also suppresses the one that
> is made in response to the option from the command line.

I agree that the current mechanism cannot easily emulate "commit -s".

> Christian?  What's your thought on this?
>
> I can understand that it sometimes may be useful to unconditionally
> be able to add a trailer without doing anything from the command
> line, but it feels fairly useless that an empty one is automatically
> added, that the only way to hide that empty one from the end result
> is to use ifexists=replace, and that there is no apparent way to remove
> the empty one.
>
> The --trim-empty option is a bit too crude a band-aid to use, as the
> existing log message may have an unrelated trailer for which it is
> perfectly valid not to have any value.

I agree that --trim-empty is not usable sometimes. Another idea would
be to add 'trailer.<token>.trimEmpty' to be able to do things like:

------
git -c trailer.foo.trimEmpty=true -c trailer.foo.command='echo $ARG'
interpret-trailers --trailer=foo:F </dev/null

foo: F
------

It could also be used along with `git commit --trailer=foo:F`.

> The fix we'd do when introducing .cmd should also get rid of this
> initial "run with an empty even when not asked"?

I don't think it's a good idea to make ".cmd" behave in a different
way as ".command" regarding this. Yes, there could be a short term
gain because people could use ".cmd" when they don't want the initial
"run with an empty value", but, as some people might still want this
initial run, long term it looks like a burden that might:

  - make it more difficult to understand how ".cmd" and ".command" both work
  - make it more difficult to migrate from ".command" to ".cmd"
  - prevent us from deprecating ".command" in favor of ".cmd"

> Or if the execution without any input from the command line were
> truly useful sometimes, a configuration variable that disables this
> behaviour, i.e.
>
>         [trailer "signoff"]
>                 command = "git who"
>                 ifexists = addIfDifferentNeighbor
>                 implicitExecution = false
>
> so that
>
>         git commit --trailer=signoff:Couder --trailer=signoff:gitster@
>
> would give us two sign-offs without the empty one, perhaps?

Yeah, that's basically the same idea that we previously discussed with
'tailer.<token>.runMode' (or 'trailer.<token>.alwaysRunCmd') in:

https://lore.kernel.org/git/CAP8UFD0OMJfkuX_JoDros7h0B20D8sm0ZbtkVpL3dCYRV_M=[e-mail]/

> In any case, the documentation update in this step looks reasonably
> well written.

Thanks!

Junio C Hamano April 17, 2021, 1:54 a.m. UTC | #3

Christian Couder <[e-mail]> writes:

> $ git config trailer.sign.command 'echo "$(git config user.name)
> <$(git config user.email)>"'
>
> My opinion is that we should have added a `trailer.<token>.runMode`
> config option along with `trailer.<token>.command`. This was not
> discussed unfortunately when ".command" was implemented, but it seems
> to be a good idea now.

Yes, without a knob to diable/enable, the "feature" is pretty much
useless.  

> It can be avoided when the --trim-empty CLI option can be used. A hook
> to remove empty trailers (which might call `git interpret-trailers
> --trim-empty` itself) could also be used when --trim-empty cannot be
> used directly.

And as you know, --trim-empty that applies to all trailer keys would
destroy other trailers and trailer.<token>.trimEmpty, even if it
were available, would not work at all to remove it for the case you
cited above, to add "user.name <user.email>", which is not an empty
string.

> I agree that the current mechanism cannot easily emulate "commit -s".
> ...
> I agree that --trim-empty is not usable sometimes. Another idea would
> be to add 'trailer.<token>.trimEmpty' to be able to do things like:

"easily"?  "sometimes"?

"--trim-empty" is unusable, trailer.<token>.trimEmpty would not work
even if it were added because the reason why this is broken is not
because it gives an empty value, but because it runs even when it is
not asked and there is no way to turn it off.

This shows that even the only plausibly-useful use case we've seen
so far is not very well supported, and necessary options/knobs to
make it usable have not been invented and implemented.

It is time for us to admit that this is a misfeature that is not
well thought out.  Recognising that .command is broken is the first
step to remedy it with a better alternative in .cmd; otherwise we
would inherit the same breakage in the replacement.

The only reasonable way out I would think of is to hide the
unconditional execution behind trailer.<token>.runMode, and make the
default for runMode to "do not run when --trailer=<token>[:<value>]
is not asked from the command line" for .cmd; it is OK for .command
not to honor the knob, as we will get rid of it once we see .cmd
works well.

Thanks.

Patch

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 96ec6499f001..6f2a7a130464 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -232,25 +232,30 @@  trailer.<token>.ifmissing::
 	that option for trailers with the specified <token>.
 
 trailer.<token>.command::
-	This option can be used to specify a shell command that will
-	be called to automatically add or modify a trailer with the
-	specified <token>.
+	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
+	each time a '--trailer <token>=<value>' argument to modify the <value> of
+	the trailer that this option would produce.
 +
-When this option is specified, the behavior is as if a special
-'<token>=<value>' argument were added at the beginning of the command
-line, where <value> is taken to be the standard output of the
-specified command with any leading and trailing whitespace trimmed
-off.
+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
+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 the command contains the `$ARG` string, this string will be
-replaced with the <value> part of an existing trailer with the same
-<token>, if any, before the command is launched.
+If some '--trailer <token>=<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, if any, will be used to replace the first
+occurrence of substring `$ARG` in the command. This way the
+command can produce a <value> computed from the <value> passed
+in the '--trailer <token>=<value>' argument.
 +
-If some '<token>=<value>' arguments are also passed on the command
-line, when a 'trailer.<token>.command' is configured, the command will
-also be executed for each of these arguments. And the <value> part of
-these arguments, if any, will be used to replace the `$ARG` string in
-the command.
+For consistency, the first occurrence of substring `$ARG` is
+also replaced, this time with the empty string, in the command
+when the command is first called to add a trailer with the
+specified <token>.
 
 EXAMPLES
 --------