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.
>> > + 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.
Thanks.
