Re: [PATCH v6] [GSOC] trailer: add new trailer.<token>.cmd config option

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

On Fri, Apr 2, 2021 at 10:49 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:

> > +trailer.<token>.cmd::
> > +     The command specified by this configuration variable is run
> > +     with a single parameter, which is the <value> part of an

I prefer "argument" over "parameter". In the next paragraph you use
"argument" (in "as if a special '<token>=<value>' argument"), so it
would be more consistent and easier to read if "argument" was used
here too.

> > +     existing trailer with the same <token>.

It would be better to say something like "which is the <value> part of
a `--trailer <token>=<value>` on the command line". (Yeah, the
existing doc might be bad about this.)

Also it might be better to explicitly say that the command is run once
for each `--trailer <token>=<value>` on the command line with the same
<token>.

> > The output from the
> > +     command is then used as the value for the <token> in the
> > +     resulting trailer.
> > ++
> > +When this option is specified, If there is no trailer with same <token>,

As above, it's better to be more explicit and say something like "if
there is no `--trailer <token>=<value>` with the same <token> on the
command line". That's because there could already be such trailers in
the input file, but those don't trigger the command.

Anyway see below as you might need to change the whole paragraph anyway.

> s/If/if/ (downcase).
>
> > +the behavior is as if a special '<token>=<value>' argument were added at
> > +the beginning of the command, <value> will be passed to the user's
> > +command as an empty value.

The problem with this is that people could understand that the command
will run with an empty argument only if there is no trailer with the
same <token>. The current situation though is that the command will
run once with an empty argument whether there are `--trailer
<token>=<value>` options with the same <token> passed on the command
line or not.

> Do the two occurrences of the word "command" in the sentence refer
> to different things?  I do not think this is an existing problem
> inherited from the original, but as we are trying to improve the
> description, I wonder if we can clarify them a bit.
>
>         ... as if a '<token>=<value>' argument were added at the
>         beginning of the "git interpret-trailers" command, the
>         command specified by this configuration variable will be
>         called with an empty string as the argument.
>
> is my attempt,

It looks better to me.

> but I am not still sure what that "as if" part is
> trying to say.  Does it mean with
>
>         [trailer "Foo"] cmd = foo-cmd
>
> and the 'input-file' does not have "Foo: <some existing value>"
> trailer in it, the command "git interpret-trailers input-file"
> would behave as if this command was run
>
>         $ Foo= git interpret-trailers input-file

I would say it would behave as if:

$ git interpret-trailers --trailer Foo= input-file

> (as there is no <value>, I am not sure what <value> is used when
> <token>=<value> is prefixed to the command)?

Nothing is done when such things are prefixed to the command.

> Puzzled and confused utterly am I...  Help, Christian?

I hope the above helps.

> >  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
>
> This talks about 'trailer.<token>.command'.  Should this be changed
> to '.cmd'?

I think so.

> Or does everything after "When this option is specified, if there is
> no trailer with ..." apply to both the old .command and new .cmd?
> If so, that was not clear at all---we'd need to clarify this part.

Yeah, in the doc about ".command" I think we should say that it
behaves in a similar way as ".cmd" except for the $ARG vs $1 issue.

> > -these arguments, if any, will be used to replace the `$ARG` string in
> > -the command.
> > +these arguments, if any, will be passed to the command as first parameter.

s/as first parameter/as its first argument/