Re: [PATCH v4] [GSOC]trailer: pass arg as positional parameter

[Christian Couder <christian.couder@gmail.com>]

On Sat, Mar 27, 2021 at 7:04 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > @@ -252,6 +252,16 @@ 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.
> >
> > +trailer.<token>.cmd::
> > +     The command specified by this configuration variable is run
> > +     with a single parameter, which is the <value> part of an
> > +     existing trailer with the same <token>.  The output from the
> > +     command is then used as the value for the <token> in the
> > +     resulting trailer.
> > +     The command is expected to replace `trailer.<token>.cmd`.

s/trailer.<token>.cmd/trailer.<token>.command/

> > +     When both .cmd and .command are given for the same <token>,
> > +        .cmd is used and .command is ignored.
>
> Christian, because ".cmd" is trying to eventually replace it, I find
> it a bit disturbing that the description we give here looks a lot
> smaller compared to the one for ".command".  I am afraid that we may
> have failed to reproduce something important from the description of
> the ".command" for the above; care to rend a hand or two here to
> complete the description?

Yeah, sure. I just saw that you already asked about this in this
thread. Sorry for not answering earlier.

> As I cannot grok what the description for ".command" is trying to
> say, especially around this part:
>
>     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 ...

This is because when a number of trailers are passed on the command
line, and some other trailers are in the input file, the order in
which the different trailers are processed and their priorities can be
important. So by saying the above, people can get an idea about at
which point and with which priority a trailer coming from such a
config option will be processed.

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

Yeah, this means that when a 'trailer.foo.command' is configured, it
is always executed at least once. The first time it is executed, it is
passed nothing ($ARG is replaced with the empty string). Then for each
'foo=<value>' argument passed on the command line, it is executed once
more with $ARG replaced by <value>.

The reason it is always executed first with $ARG replaced with the
empty string is that this way it makes it possible to set up commands
that will always be executed when `git interpret-trailers` is run.
This makes it possible to automatically add some trailers if they are
missing for example.

Another way to do it would be to have another config option called
`trailer.<token>.alwaysRunCmd` to tell if the cmd specified by
`trailer.<token>.cmd` should be run even if no '<token>=<value>'
argument is passed on the command line. As we are introducing
`trailer.<token>.cmd`, it's a good time to wonder if this would be a
better design. But this issue is quite complex, because of the fact
that 'trailer.<token>.ifMissing' and 'trailer.<token>.ifExists' also
take a part in deciding if the command will be run.

This mechanism is the reason why a trick, when setting up a
'trailer.foo.command' trailer, is to also set 'trailer.foo.ifexists'
to "replace", so that the first time the command is run (with $ARG
replaced with the empty string) it will add a foo trailer with a
default value, and if it is run another time, because a 'foo=bar'
argument is passed on the command line, then the trailer with the
default value will be replaced by the value computed from running the
command again with $ARG replaced with "bar".

Another trick is to have the command output nothing when $ARG is the
empty string along with using --trim-empty. This way the command will
create an empty trailer, when it is run the first time, and if it's
not another time, then this empty trailer will be removed because of
--trim-empty.

> I cannot quite judge if what we came up with in the above
> description is sufficient.

I don't think it's sufficient. I think that, while we are at it, a bit
more thinking/discussion is required to make sure we want to keep the
same design as 'trailer.<token>.command'.

> > -* Configure a 'sign' trailer with a command to automatically add a
> > +* Configure a 'sign' trailer with a cmd to automatically add a
> >    'Signed-off-by: ' with the author information only if there is no
> >    'Signed-off-by: ' already, and show how it works:
> >  +
> > @@ -309,7 +319,7 @@ $ git interpret-trailers --trailer 'Cc: Alice <alice@example.com>' --trailer 'Re
> >  $ git config trailer.sign.key "Signed-off-by: "
> >  $ git config trailer.sign.ifmissing add
> >  $ git config trailer.sign.ifexists doNothing
> > -$ git config trailer.sign.command 'echo "$(git config user.name) <$(git config user.email)>"'
> > +$ git config trailer.sign.cmd 'echo "$(git config user.name) <$(git config user.email)>"'
> >  $ git interpret-trailers <<EOF
> >  > EOF
>
> This change would definitely be needed when the support for
> ".command" is removed after deprecation period.  As it does not take
> any argument, .cmd and .command should behave identically, so making
> this change now, without waiting, may make sense.

By the way the above example is an example of why we might want any
configured command to be executed at least once, even when no
corresponding '<token>=<value>' argument is passed on the command
line.

> > @@ -333,14 +343,14 @@ subject
> >  Fix #42
> >  ------------
> >
> > -* Configure a 'see' trailer with a command to show the subject of a
> > +* Configure a 'see' trailer with a cmd to show the subject of a
> >    commit that is related, and show how it works:
> >  +
> >  ------------
> >  $ git config trailer.see.key "See-also: "
> >  $ git config trailer.see.ifExists "replace"
> >  $ git config trailer.see.ifMissing "doNothing"
> > -$ git config trailer.see.command "git log -1 --oneline --format=\"%h (%s)\" --abbrev-commit --abbrev=14 \$ARG"
> > +$ git config trailer.see.cmd "test -n \"\$1\" && git log -1 --oneline --format=\"%h (%s)\" --abbrev-commit --abbrev=14 \"\$1\"|| true "
> >  $ git interpret-trailers <<EOF
> >  > subject
>
> This, too, but until ".command" is removed, wouldn't it be better
> for readers to keep both variants, as the distinction between $ARG
> and $1 needs to be illustrated?
>
> Besides, the examples given here are not equivalent.  The original
> assumes that ARG is there, or it is OK to default to HEAD; the new
> one gives no output when $ARG/$1 is not supplied.

Yeah, I agree they are not equivalent.

> It would confuse
> readers to give two too-similar-but-subtly-different examles, as
> they will be forced to wonder if the difference is something needed
> to transition from .command to .cmd (and I am guessing that it is
> not).

I agree.

> Rewriting both to use "--pretty=reference" may be worth doing.  As
> can be seen in these examples:
>
> git show -s --pretty=reference \$1
> git log -1 --oneline --format=\"%h (%s)\" --abbrev-commit --abbrev=14 \$1
>
> that it makes the result much easier to read.

Yeah, thanks for the good suggestion.