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

[ZheNing Hu <adlternative@gmail.com>]

Rafael Silva <rafaeloliveira.cs@gmail.com> 于2021年3月14日周日 下午9:46写道：
>
>
> "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > 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 :).

>
> >       /*
> >        * 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", .....
>
> >       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 <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
> > +'
> > +
> >  test_expect_success 'multiple -m' '
>
>
> Hope these comments are useful.
> --
> Thanks
> Rafael

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