Re: [PATCH v11 0/2] [GSOC] trailer: add new .cmd config option

[ZheNing Hu <adlternative@gmail.com>]

Junio C Hamano <gitster@pobox.com> 于2021年4月21日周三 上午8:09写道：
>
> ZheNing Hu <adlternative@gmail.com> writes:
>
> > OK, I understand, then I can wait for a while until `trailer_cmd` merge
> > to master.
> >
> >> But let's see what's new in this iteration.
> >>
> >>
> >> >       +#!/bin/sh
> >> >      -+test -n "$1" && git shortlog -s --author="$1" HEAD || true
> >> >      ++if test "$#" != 1
> >> >      ++then
> >> >      ++       exit 1
> >> >      ++else
> >> >      ++       test -n "$1" && git shortlog -s --author="$1" HEAD || true
> >> >      ++fi
> >>
> >> I find this dubious.  Why not
> >>
> >>         if test "$#" != 1 || test -z "$1"
> >>         then
> >>                 exit 1
> >>         else
> >>                 git shortlog -s --author="$1" HEAD
> >>         fi
> >>
> >> That is, if you happened to give an empty string, your version gives
> >> "" to <value> and returns success, letting a trailer "cnt:" with
> >> empty value.  Is that what we really want?
> >
> > If it's the user use `--trailer="cnt:"` instread of command implict running,
> > I think keep it is right.
>
> No, if you give an empty string, you'd end up running "shortlog"
> with --author="" and give whatever random number it comes up with,
> which I do not think is what you would want.
>
> That is why --trailer=cnt: without name to match --author can be
> rejected with "exit 1" to demonstrate the feature.  The .cmd can
> squelch not just the "unasked for extra invocation", but invocation
> from the command line whose <value> was bogus, unlike the .runmode
> feature we've seen proposed earlier.
>

I admit that your idea makes sense, but we actually have another requirement:
Construct a trailer with an empty value.

The Commit-Count example above may not be good, Let’s take a look at the
Signed-off-by.

e.g. `--trailer="sign:"`, we expect to output a "Signed-off-by: ",
then we can fill it
with the "name <email>" pair we want, this is when we shouldn't return non-zero
and suppress its output.

> >> >      +        if (capture_command(&cp, &buf, 1024)) {
> >> >      +-               error(_("running trailer command '%s' failed"), cmd.buf);
> >> >      +                strbuf_release(&buf);
> >> >      +-               result = xstrdup("");
> >> >      ++               if (!conf->cmd || arg) {
> >> >      ++                       error(_("running trailer command '%s' failed"), cmd.buf);
> >>
> >> I am not sure about this part.  If .cmd (the new style) exits with a
> >> non-zero status for user-supplied --trailer=<token>:<value> (because
> >> it did not like the <value>), is that "running failed"?  The script
> >> is expected to express yes/no with its exit status, so I would say it
> >> is not failing, but successfully expressed its displeasure and vetoed
> >> the particular trailer from getting added.  IOW, "|| arg" part in
> >> the condition feels iffy to me.
> >
> > Well, you mean we can take advantage of non-zero exits instead of
> > just removing implicitly executed content. I argee with you, this
> > place is worth change.
>
> Yup, that is what I meant.
>
> In any case, let's see how well the base topic fares.
>

Yes, I don't know if Christian agrees with temporary situation.

> Thanks.

Thanks.
--
ZheNing Hu