Rafael Silva <rafaeloliveira.cs@xxxxxxxxx> 于2021年3月14日周日 下午9:46写道：
>
>
> "ZheNing Hu via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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@xxxxxxxxxxx>" \
> > + --trailer "Helped-by:C O Mitter2 <committer2@xxxxxxxxxxx>" \
> > + --trailer "Reported-by:C O Mitter3 <committer3@xxxxxxxxxxx>" \
> > + --trailer "Mentored-by:C O Mitter4 <committer4@xxxxxxxxxxx>" \
> > + -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@xxxxxxxxxxx>" \
> --trailer "Helped-by:C O Mitter2 <committer2@xxxxxxxxxxx>" \
> --trailer "Reported-by:C O Mitter3 <committer3@xxxxxxxxxxx>" \
> --trailer "Mentored-by:C O Mitter4 <committer4@xxxxxxxxxxx>" \
> -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@xxxxxxxxxxx>
> > + Signed-off-by: C O Mitter1 <committer1@xxxxxxxxxxx>
> > + Helped-by: C O Mitter2 <committer2@xxxxxxxxxxx>
> > + Reported-by: C O Mitter3 <committer3@xxxxxxxxxxx>
> > + Mentored-by: C O Mitter4 <committer4@xxxxxxxxxxx>
> > + 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
