From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 21 Apr 2021 00:09:09 GMT
Subject: Re: [PATCH v11 0/2] [GSOC] trailer: add new .cmd config option
Message-ID: <xmqqfszk1ot6.fsf@gitster.g>
In-Reply-To: <CAOLTT8RKCV+Kpya-_AVjuVGWzs1WtGS8n_+sD0FVzwEpeXGwCw@mail.gmail.com>

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.

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