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

From: ZheNing Hu <adlternative@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: ZheNing Hu via GitGitGadget <gitgitgadget@gmail.com>,
	Git List <git@vger.kernel.org>,
	Christian Couder <christian.couder@gmail.com>
Subject: Re: [PATCH v6] [GSOC] trailer: add new trailer.<token>.cmd config option
Date: Sat, 3 Apr 2021 13:08:05 +0800	[thread overview]
Message-ID: <CAOLTT8Tq+-tRzgG1AFD2cE2Vp9z3E94eW1eyN2BxKKg17FzTng@mail.gmail.com> (raw)
In-Reply-To: <xmqqim544dl4.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> 于2021年4月3日周六 上午4:49写道：

> > +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.
>
> 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, 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
>
> (as there is no <value>, I am not sure what <value> is used when
> <token>=<value> is prefixed to the command)?
>
> Puzzled and confused utterly am I...  Help, Christian?
>

If we don’t want a input-file without any trailers to execute this command,
 I’m thinking that we maybe need "trailer.<token>.ifmissing=doNothing"
or something else, otherwise execute this 'alwaysRunCmd' for create a
empty trailer like "Signed-off-by" and users can use it to add content.

> >  +
> >  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'?
>
> 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.
>

Because ".command" will be eliminated, may be only leave those
description Information to ".cmd" is better.

> > -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.
> >
> >  EXAMPLES
> >  --------
> > @@ -333,6 +346,55 @@ subject
> >  Fix #42
> >  ------------
> >
> > +* Configure a 'see' trailer with a cmd use a global script `git-one`
> > +  to show the subject of a commit that is related, and show how it works:
> > ++
> > +------------
> > +$ cat ~/bin/git-one
> > +#!/bin/sh
> > +git show -s --pretty=reference "$1"
> > +$ git config trailer.see.key "See-also: "
> > +$ git config trailer.see.ifExists "replace"
> > +$ git config trailer.see.ifMissing "doNothing"
> > +$ git config trailer.see.cmd "~/bin/git-one"
> > +$ git interpret-trailers <<EOF
> > +> subject
> > +>
> > +> message
> > +>
> > +> see: HEAD~2
> > +> EOF
> > +subject
> > +
> > +message
> > +
> > +See-also: fe3187e (subject of related commit, 2021-4-2)
> > +------------
> > +
> > +* Configure a 'who' trailer with a cmd use a global script `git-who`
> > +  to find the recent matching "author <mail>" pair in git log and
> > +  show how it works:
> > ++
> > +------------
> > +$ cat ~/bin/git-who
> > + #!/bin/sh
> > +    git log -1 --format="%an <%ae>" --author="$1"
>
> Unusual indentation here.  But more importantly, I am not sure if
> having both 'see' and 'help' examples is worth it---they are similar
> enough that the second one does not teach anything new to those who
> studied the first one already, aren't they?
>

Ok, I will think about other examples.

> > +$ git config trailer.help.key "Helped-by: "
> > +$ git config trailer.help.ifExists "replace"
> > +$ git config trailer.help.cmd "~/bin/git-who"
> > +$ git interpret-trailers --trailer="help:gitster@" <<EOF
> > +> subject
> > +>
> > +> message
> > +>
> > +> EOF
> > +subject
> > +
> > +message
> > +
> > +Helped-by: Junio C Hamano <gitster@pobox.com>
> > +------------
> > +
> >  * Configure a 'see' trailer with a command to show the subject of a
> >    commit that is related, and show how it works:
> >  +
>
>
>
> > diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
> > index 6602790b5f4c..923923e57573 100755
> > --- a/t/t7513-interpret-trailers.sh
> > +++ b/t/t7513-interpret-trailers.sh
> > @@ -51,6 +51,77 @@ test_expect_success 'setup' '
> >       EOF
> >  '
> >
> > +test_expect_success 'with cmd' '
> > +     test_when_finished "git config --unset trailer.bug.key && \
> > +                         git config --unset trailer.bug.ifExists && \
> > +                         git config --unset trailer.bug.cmd" &&
>
> It is unwise to use && between these three "git config" invocations,
> I suspect.  "git config --unset" exits with non-zero status when you
> attempt to remove with an non-existent key, but you would remove the
> .ifExists and .cmd even if .key is not defined.  Perhaps
>
>         test_when_finished "git config --unset-all trailer.bug.key
>                             git config --unset-all trailer.bug.ifExists
>                             git config --unset-all trailer.bug.cmd" &&
>
> would be more sensible.  Or if we just want to remove everything
> under the trailer.bug.* section, this might be even better:
>
>         test_when_finished "git config --remove-section trailer.bug" &&
>
> as we can add new trailer.bug.* to the system and use them in this
> test, but removing the entire section would still be a good way to
> clean after ourselves.
>

Good idea, This removing is also more efficient.

> > diff --git a/trailer.c b/trailer.c
> > index be4e9726421c..6aeff6a1bd33 100644
> > --- a/trailer.c
> > +++ b/trailer.c
> > ...
> > +static char *apply_command(struct conf_info *conf, const char *arg)
> >  {
> >       struct strbuf cmd = STRBUF_INIT;
> >       struct strbuf buf = STRBUF_INIT;
> >       struct child_process cp = CHILD_PROCESS_INIT;
> >       char *result;
> >
> > -     strbuf_addstr(&cmd, command);
> > -     if (arg)
> > -             strbuf_replace(&cmd, TRAILER_ARG_STRING, arg);
> > -
> > -     strvec_push(&cp.args, cmd.buf);
> > +     if (conf->cmd) {
> > +             // cp.shell_no_implicit_args = 1;
>
> Do not add new code that is commented out.  Besides we do not use // comment.
>
> > +             strbuf_addstr(&cmd, conf->cmd);
> > +             strvec_push(&cp.args, cmd.buf);
> > +             if (arg)
> > +                     strvec_push(&cp.args, arg);
>
> Thanks.

Thanks.