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

On Mon, Mar 15, 2021 at 12:32 PM ZheNing Hu [off-list ref] wrote:

Christian Couder [off-list ref] 于2021年3月15日周一 下午6:14写道：

quoted

On Mon, Mar 15, 2021 at 10:08 AM ZheNing Hu via GitGitGadget
[off-list ref] wrote:

quoted

      + 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
[off-list ref]"

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 [off-list ref]"

- 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
[off-list ref]"

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.