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

Rafael Silva [off-list ref] 于2021年3月14日周日 下午9:46写道：

"ZheNing Hu via GitGitGadget" [off-list ref] writes:

quoted

diff --git a/builtin/commit.c b/builtin/commit.c
index 739110c5a7f6..abbd136b27f0 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -113,6 +113,8 @@ static int config_commit_verbose = -1; /* unspecified */
 static int no_post_rewrite, allow_empty_message, pathspec_file_nul;
 static char *untracked_files_arg, *force_date, *ignore_submodule_arg, *ignored_arg;
 static char *sign_commit, *pathspec_from_file;
+struct child_process run_trailer = CHILD_PROCESS_INIT;
+static const char *trailer;

 /*
  * The default commit message cleanup mode will remove the lines

@@ -131,6 +133,17 @@ static struct strbuf message = STRBUF_INIT;

 static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;

+static int opt_pass_trailer(const struct option *opt, const char *arg, int unset)
+{
+     if (unset) {
+             strvec_clear(&run_trailer.args);
+             return -1;
+     }
+     run_trailer.git_cmd = 1;
+     strvec_pushl(&run_trailer.args, "--trailer", arg, NULL);
+     return 0;
+}
+
 static int opt_parse_porcelain(const struct option *opt, const char *arg, int unset)
 {
      enum wt_status_format *value = (enum wt_status_format *)opt->value;

@@ -958,6 +971,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,

      fclose(s->fp);

+     run_command(&run_trailer);
+

There is slight problem with running the command unconditionally.
If no --trailer is passed, then the opt_pass_trailer() backend
is never called, which consequently will not set the trailer
command ".git_cmd" option to 1.

This will lead the run_command() API to not interpret the command as git
internal, and attempt to launch as an usual command "interpret-trailers"
that will likely not exist or launch an unwanted command that is not
part of the GIT suite.

This can be seen by running `git commit` without any options:

     $ ./bin-wrappers/git -C /tmp/test commit
     error: cannot run interpret-trailers: No such file or directory
     ...

The `.git_cmd` should be set to true before running the command.

(the above output is from a built version with v2.31.0-rc2 + this patch
 for confirmation).


Furthermore, in my opinion, we shouldn't even bother to run the command
if no --trailer is passed, otherwise, we always be paying the cost of
launching an OS process regardless if the user doesn't want to add
trailers in theirs projects.

With that said and based on this current implementation, maybe an
improved version will look like:

        if (run_trailer.args.nr) {
                run_trailer.git_cmd = 1;
                run_command(&run_trailer);
        }

Naturally the `git_cmd = 1` will be removed from opt_pass_trailer()
function as it won't be necessary. As minor bonus, we don't end up
setting the value for every new --trailer :).

Thank you, I didn't notice the problem before :).

quoted

      /*
       * Reject an attempt to record a non-merge empty commit without
       * explicit --allow-empty. In the cherry-pick case, it may be

@@ -1507,6 +1522,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(0, "trailer", &trailer, N_("trailer"), N_("trailer(s) to add"), opt_pass_trailer),
              OPT_BOOL('s', "signoff", &signoff, N_("add a Signed-off-by trailer")),
              OPT_FILENAME('t', "template", &template_file, N_("use specified template file")),
              OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")),

@@ -1577,6 +1593,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
                      die(_("could not parse HEAD commit"));
      }
      verbose = -1; /* unspecified */
+     strvec_pushl(&run_trailer.args, "interpret-trailers",
+             "--in-place", "--where=end", git_path_commit_editmsg(), NULL);

Style: The "--in-place" part should be aligned with the parentheses
much the like the following line with the "argc = parse_and_validate_options....".

Well, the CodingGuidelines say it's both vaild. But you are right,
try to keep the same format as nearby code will be better.

For example:

        strvec_pushl(&run_trailer.args, "interpret-trailers",
                     "--in-place", "--where=end", .....

quoted

      argc = parse_and_validate_options(argc, argv, builtin_commit_options,
                                        builtin_commit_usage,
                                        prefix, current_head, &s);

diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
index 6396897cc818..4b9ac4587d17 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" &&

Perhaps here, the --trailer lines and "-m hello" option should be
indent in order to make it clear that these option are part of the
"git commit" from the above line, something like this:

        git commit -s --trailer "Signed-off-by:C O Mitter1 [off-list ref]" \
                --trailer "Helped-by:C O Mitter2 [off-list ref]"  \
                --trailer "Reported-by:C O Mitter3 [off-list ref]" \
                --trailer "Mentored-by:C O Mitter4 [off-list ref]" \
                -m "hello" &&

quoted

+     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
+'
+
 test_expect_success 'multiple -m' '

Hope these comments are useful.
--
Thanks
Rafael

 I am gratitude to you for helping me.
--
Thanks
ZheNing Hu