Re: [PATCH v7] [GSOC] commit: add --trailer option

[Christian Couder <christian.couder@gmail.com>]

On Mon, Mar 15, 2021 at 12:32 PM ZheNing Hu <adlternative@gmail.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> 于2021年3月15日周一 下午6:14写道：
> >
> > On Mon, Mar 15, 2021 at 10:08 AM ZheNing Hu via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >
> > >       + if (trailer_args.nr) {
> > >      -+         static struct child_process run_trailer = CHILD_PROCESS_INIT;
> > >      ++         struct child_process run_trailer = CHILD_PROCESS_INIT;
> > >       +
> > >       +         strvec_pushl(&run_trailer.args, "interpret-trailers",
> > >       +                      "--in-place", "--where=end", git_path_commit_editmsg(), NULL);
> >
> > Actually I don't think "--where=end" should be used here. "end" is the
> > default for the "trailer.where" config variable, so by default if
> > nothing has been configured, it will work as if "--where=end" was
> > passed above.
> >
> > If a user has configured "trailer.where" or trailer.<token>.where,
> > then this should be respected. And users should be able to override
> > such config variable using for example:
> >
> > git -c trailer.where=start commit --trailer "Signed-off-by:C O Mitter
> > <committer@example.com>"
>
> Thanks for reminding, generally speaking, we will put the trailer at the
> end of the commit messages.Take trailers in start, this should be
> something I haven't considered.

In general what I want to say is that `git interpret-trailers` should
be considered to have sensible defaults, that can possibly be
overridden using a number of config variables (or the git -c ...
mechanism) which is a good thing. If something in it doesn't work
well, it's possible to improve it of course. Otherwise it's better to
just fully take advantage of it.

> I notice another question:
> if we commit this again with same trailer (even same email or same commiter)
> `--trailer` will not work again, because in `interpret_trailers`,
> "if-exists" default
> set to "addIfDifferentNeighbor", I addvice enforce use "if-exists="add".

I don't agree with using "--if-exists=add". I think the default to not
add a trailer line if it would be just above or below the same line is
better, as doing that wouldn't add much information. It's better to
encourage people to use trailers in a meaningful way.

And again if we use "--if-exists=add", then people who would want to
take advantage of `git interpret-trailers` to customize what happens
when the trailer already exists would not be able to do it.

For example if we don't use "--if-exists=add", then:

- people who want to customize what happens when the trailer already
exists can do it with for example:

git -c trailer.ifexists=addIfDifferent commit --trailer
"Signed-off-by:C O Mitter <committer@example.com>"

- which means that people who want the "--if-exists=add" behavior can
still have it with:

git -c trailer.ifexists=add commit --trailer "Signed-off-by:C O Mitter
<committer@example.com>"

While if we use "--if-exists=add", then using `git -c
trailer.ifexists=... commit ...` will not customize anything as the
"--if-exists=add" command line option will override any config
customization.