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

From: ZheNing Hu <adlternative@gmail.com>
Date: Apr 15, 2021, 15:32 UTC
Message-ID: <CAOLTT8TVAvk_Hdcd27jHz5wWOR6xEMuCeQvDfu48qVDFPN666A@mail.gmail.com>
In-Reply-To: <xmqqv98oy5bh.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> 于2021年4月15日周四 上午4:33写道：

Show 52 quoted lines

>
> ZheNing Hu <adlternative@gmail.com> writes:
>
> >> So I am waiting to hear why it is not a misfeature.  If it is not,
> >> then surely I am fine to keep it for now and add a workaround later,
> >> but until that happens, I do not think "commit --trailer" can be
> >> used as a way to allow end-users emulate "-s" for their favorite
> >> trailer like helped-by, acked-by, etc.
> >>
> >
> > If it is really necessary to solve this "empty execution" in .cmd,
>
> > Maybe we need to consider two points:
> > * Do we need a new config flag as you said `[implicitExecution = false]`
> > or just drop it? Has anyone been relying on the "empty execution" of
> > .command before? This may be worthy of concern.
>
> Yes, if it is useful sometimes to run the .command or .cmd with
> empty <value> even when nobody asks for it on the command line with
> a "--trailer=<key>:<value>" option, then I agree that the users
> should be able to choose between running and not running [*].
>
> > *  Do we need `trailer.<token>.runMode` as Christan said before?
> > I rejected his this suggestion before, and now I regret it a bit.
>
> So far, I haven't heard anything that indicates it is a useful
> behaviour for .command, so my preference is to get rid of the
> behaviour when we introduce .cmd to deprecate .command; yes, until
> we get rid of .command, the behaviour between the two would be
> inconsistent but that is unavoidable when making a bugfix that is
> backward incompatible.
>
> When (and only when) anybody finds a credible use case, we can add a
> mechanism to optionally turn it on (e.g. .runMode).
>
> That is my thinking right at this moment, but that's of course
> subject to change when a use case that would be helped by having
> this extra execution.
>
>
> [Footnote]
>
> * Right now, all I know is that not being able to turn it off makes
>   it impossible to use "git commit --trailer" as a more general
>   substitute for "git commit --signoff" without breaking other
>   trailers (e.g. --trim-empty may get rid of the result of the extra
>   execution, but would remove other trailers that can be
>   legitimately empty).  And making it on by default with
>   configuration would mean that even though we designed .cmd as a
>   better version of the .command feature with its misdesign
>   corrected, we'd inherit one misdesign from it, which defeats one
>   third of the point of introducing the .cmd in the first place ;-)

Perhaps such a modification can meet our temporary needs!

@@ -721,9 +738,10 @@ static void process_command_line_args(struct
list_head *arg_head,
        char *cl_separators = xstrfmt("=%s", separators);

        /* Add an arg item for each configured trailer with a command */
        list_for_each(pos, &conf_head) {
                item = list_entry(pos, struct arg_item, list);
-               if (item->conf.cmd || item->conf.command)
+               if (item->conf.command)

I'm so busy today that I probably haven't had time to put this into a
patch today,
If this solution is reasonable, I will send a new version of the patch tomorrow.
As you said, first solve the misfeature, and we can add [trailer.runMode] later.

Thanks, Junio!
--
ZheNing Hu