Re: [PATCH v8 1/2] [GSOC] commit: add --trailer option

Ævar Arnfjörð Bjarmason [off-list ref] 于2021年3月16日周二 下午8:52写道：

quoted

+             if (run_command(&run_trailer))
+                     strvec_clear(&run_trailer.args);

This is git-commit, shouldn't we die() here instead of ignoring errors
in sub-processes?

After thinking about it carefully, your opinion is more
reasonable, because if the user uses the wrong `--trailer`
and does not get the information he needs, I think he will
have to use `--amend` to modify, and `die()` can exit
this commit directly.

quoted

+             strvec_clear(&trailer_args);
+     }
+
      /*
       * Reject an attempt to record a non-merge empty commit without
       * explicit --allow-empty. In the cherry-pick case, it may be

@@ -1507,6 +1529,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
              OPT_STRING(0, "fixup", &fixup_message, N_("commit"), N_("use autosquash formatted message to fixup specified commit")),
              OPT_STRING(0, "squash", &squash_message, N_("commit"), N_("use autosquash formatted message to squash specified commit")),
              OPT_BOOL(0, "reset-author", &renew_authorship, N_("the commit is authored by me now (used with -C/-c/--amend)")),
+             OPT_CALLBACK_F(0, "trailer", NULL, N_("trailer"), N_("trailer(s) to add"), PARSE_OPT_NONEG, opt_pass_trailer),
              OPT_BOOL('s', "signoff", &signoff, N_("add a Signed-off-by trailer")),

Not required for this change, but perhaps a change here to N_() (if we
can get it to fit) + doc update saying that we prefer
--trailer="Signed-Off-By: to --signoff"? More on that later.

quoted

              OPT_FILENAME('t', "template", &template_file, N_("use specified template file")),
              OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")),

diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
index 6396897cc818..0acf23799931 100755
--- a/t/t7502-commit-porcelain.sh
+++ b/t/t7502-commit-porcelain.sh

@@ -154,6 +154,26 @@ test_expect_success 'sign off' '

 '

+test_expect_success 'trailer' '
+     >file1 &&
+     git add file1 &&
+     git commit -s --trailer "Signed-off-by:C O Mitter1 <committer1@example.com>" \
+             --trailer "Helped-by:C O Mitter2 <committer2@example.com>"  \
+             --trailer "Reported-by:C O Mitter3 <committer3@example.com>" \
+             --trailer "Mentored-by:C O Mitter4 <committer4@example.com>" \
+             -m "hello" &&
+     git cat-file commit HEAD >commit.msg &&
+     sed -e "1,7d" commit.msg >actual &&
+     cat >expected <<-\EOF &&
+     Signed-off-by: C O Mitter <committer@example.com>
+     Signed-off-by: C O Mitter1 <committer1@example.com>
+     Helped-by: C O Mitter2 <committer2@example.com>
+     Reported-by: C O Mitter3 <committer3@example.com>
+     Mentored-by: C O Mitter4 <committer4@example.com>
+     EOF
+     test_cmp expected actual
+'
+

How does this interact with cases where the user has configured
"trailer.separators" to have a value that doesn't contain ":"?  I
haven't tested, but my reading of git-interpret-trailers(1) is that if
you supplied "=" instead that case would just work:

    By default only : is recognized as a trailer separator, except that
    = is always accepted on the command line for compatibility with
    other git commands.

But interpret_trailers interface allow us use "=" instead of other separators.

I did a simple test and modified the configuration "trailer.separators"
and it still works. Now things are good here:

$ git -c trailer.separators="@" commit --trailer="Signed-off-by=C O <email>"

or

$ git -c trailer.separators="@" commit --trailer="Signed-off-by@C O <email>"

Both can work normally,

--trailer="Signed-off-by@ C O <email>"

will output in the commit message.

I don't know if that does the right thing in the presence of
--if-exists=add.

Yesterday, Christian Couder and I had already discussed this issue:
Your idea is correct, I should not add "--if-exists = add",  this will destroy
the user's rights to configure by using `git -c trailer.if-exist`.

So it would be good to update these tests so you test:

 * For the --if-exists=add case at all, there's no tests for it
   now. I.e. add some trailers manually to the commit (via -F or
   whatever) and then see if they get added to, replacet etc.

 * Ditto but for the user having configured trailer.separators (see the
   test_config helper for how to set config in a test). I.e. if it's "="
   does adding trailers work, how about if it's "=" on the CLI but the
   config/commit message has ";" instead of ":" or something?

As mentioned above, it works normally.

 * Hrm, actually I think tweaking "-c trailer.ifexists" won't work at
   all, since the CLI switch would override it. I honestly don't know,
   but why not not supply it and keep the addIfDifferentNeighbor
   default?

   If it's essential that seems like a good test / documentation
   addition...

 * For the above -c ... case I can't think of a good way to deal with it
   that doesn't involve pulling in git_trailer_config() into
   git_commit_config(), but perhaps the least nasty way is to just set a
   flag in git_commit_config() if we see a "trailer.ifexists" flag, and
   if so don't provide "--if-exists=add", if there's no config (this
   will include "git -c ... commit" we set provide "--if-exists=add" )
   or as noted above, maybe we can skip the whole thing and use the
   addIfDifferentNeighbor default.

Has been restored to the default settings.

And, not needed for this patch but worth thinking about:

 * We pass through --trailer to git-interpret-trailers, what should we
   do about the other options? Should git-commit eventually support
   --trailer-where and pass it along as --where to
   git-interpret-trailers, or is "git -c trailer.where=... commit" good
   enough?

Logically speaking, `interpret_trailers` should be dedicated to `commit`
or other sub-commands that require trailers.

But I think that in the later stage, the parse_options of the `cmd_commit`
can keep the unrecognized options, and then these choices can be directly
passed to the `interpret_trailers` backend.

 * It would be good to test for and document if that "-c trailer.*"
   trick works (no reason it shouldn't). I.e. to add something like this
   after what you have (along with tests, and check if it's even true):

I haven't tested them for the time being, but I will do it.

       Only the `--trailer` argument to
       linkgit:git-interpret-trailers[1] is supported. Other
       pass-through switches may be added in the future, but currently
       you'll need to pass arguments to
       linkgit:git-interpret-trailers[1] along as config, e.g. `git -c
       trailer.where=start commit [...] --trailer=[...]`.

I think this is worth writing in the documentation.

 * We have a longer-term goal of having the .mailmap apply to trailers,
   it would be nice if git-interpret-trailers had some fuzzy-matching to
   check if the RHS of a trailer is a name/E-Mail pair, and if so did
   stricter validation on it with the ident functions we use for fsck
   etc. (that's copied & subtly different in several different places in
   the codebase, unfortunately[1]).

I may not know much about fuzzy-matching, which may be worth studying later.

More thoughts:

 * Having written all the above I checked how --signoff is implemented.

   It seems to me to be a good idea to (at least for testing) convert
   the --signoff trailer to your implementation. We have plenty of tests
   for it, does migrating it over pass or fail those?

I don’t know how to migrating yet, it may take a long time.
Even I think I can leave it as #leftoverbit later.

 * I also agree with Junio that we shouldn't have a --fixed-by or
   whatever and wouldn't add --signoff today, but it seems very useful
   to me to have a shortcut like:

       --trailer "Signed-off-by"

   I.e. omitting the value, or:

      --trailer "Signed-off-by="

   Or some other thing we deem sufficiently useful/sane
   syntax/unambiguous.n

   Then the value would be provided by fmt_name(WANT_COMMITTER_IDENT)
   just as we do in append_signoff() now. I think a *very common* case
   for this would be something like:

       git commit --amend -v --trailer "Reviewed-by"

   And it would be useful to help that along and not have to do:

       git commit --amend -v --trailer "Reviewed-by=$(git config user.name) <$(git config user.email)>"

   Or worse yet, manually typo your name/e-mail address, as I'm sure I
   and many others will inevitably do when using this option...

I think this idea is very good and easy to implement.
We only need to do a simple string match when we get the "trailer" string,
If it can be completed, it can indeed bring great convenience to users.

1. https://lore.kernel.org/git/87bld8ov9q.fsf@evledraar.gmail.com/ (local)

Thanks, Ævar Arnfjörð Bjarmason!

--
ZheNing Hu