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

From: Christian Couder <christian.couder@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: ZheNing Hu <adlternative@gmail.com>,
	ZheNing Hu via GitGitGadget <gitgitgadget@gmail.com>,
	git <git@vger.kernel.org>
Subject: Re: [PATCH v9 2/2] [GSOC] trailer: add new .cmd config option
Date: Fri, 16 Apr 2021 14:54:58 +0200	[thread overview]
Message-ID: <CAP8UFD2J8jz2Ku4ur_L+36b9hKB5drrexKNHYF6dM9G9BKhFug@mail.gmail.com> (raw)
In-Reply-To: <xmqqv98oy5bh.fsf@gitster.g>

On Wed, Apr 14, 2021 at 10:33 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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,

Well the `git config trailer.sign.command 'echo "$(git config
user.name) <$(git config user.email)>"'` has been documented in the
EXAMPLES section of the `git interpret-trailers` doc since when
".command" was implemented, and I believe that reviewers thought that
it was a good feature to have then.

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

Such use cases and some of this were discussed when `git
interpret-trailers` was initially implemented.

For example in https://lore.kernel.org/git/CAP8UFD2oXpW9QEkSh+vpNGRAxRFp0zJF39ZZ8sUZLTcKB9mHWQ@mail.gmail.com/
I suggested the following:

         [trailer "m-o-b"]
                 key = "Made-on-branch: "
                 command = "git name-rev --name-only HEAD"

when someone wanted a way to always add "a trailer for the branch that
the commit was checked in to at the time"

In https://lore.kernel.org/git/534414FB.6040604@alum.mit.edu/ Michael
Haggerty suggested:

"Maybe there should be a trailer.<token>.trimEmpty config option."

I haven't fully checked the discussions, so there might be other examples.