Junio C Hamano <gitster@xxxxxxxxx> 于2021年4月21日周三 上午8:09写道：
>
> ZheNing Hu <adlternative@xxxxxxxxx> 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
