Christian Couder <christian.couder@xxxxxxxxx> 于2021年3月28日周日 上午3:53写道：
>
> On Sat, Mar 27, 2021 at 7:04 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
> >
> > "ZheNing Hu via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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.
>
This shows that .command itself has the characteristic of alwaysRun:
even if <token> <value> is not specified, the shell in .command will be
executed at least once, $ARG is empty by default. This is why I asked
`log --author=$ARG -1` will show the last commit identity when `--trailer`
is not used.
> > 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.
>
Yes, $ARG or $1 are always exist because of:
arg = xstrdup("");
so I think maybe we don't even need this judge in `apply_command`?
+ if (arg)
+ strvec_push(&cp.args, arg);
> 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.
>
In fact, I would prefer this design, because if I don’t add any trailers,
the trailer.<token>.command I set will be executed, which may be very
distressing sometimes, and `alwayRunCmd` is the user I hope that "trailers"
can be added automatically, and other trailers.<token>.command will not be
executed automatically. This allows the user to reasonably configure the
commands that need to be executed. This must be a very comfortable thing.
But as you said, to disable the automatic addition in the original .command
and use the new .alwaysRunCmd, I’m afraid there are a lot of things to consider.
Perhaps future series of patches can be considered to do it.
> 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.
>
It looks very practical indeed.
> > 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'.
Sure. I agree that more discussion is needed.
I think if the documents that once belonged to .command are copied to .cmd,
will the readers be too burdensome to read them? Will it be better to migrate
its documentation until we completely delete .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@xxxxxxxxxxx>' --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.
Already noticed that.
>
> > > @@ -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?
So the correct solution should be to keep the original .command Examples,
and then give the .cmd examples again.
> >
> > 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.
OK...I will modify it.
>
> > 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.
Yes, `--pretty=reference` is similar to `--format="%h(%s)"` and provides better
readability.
Thanks,Junio and Christian!
--
ZheNing Hu
