Re: [PATCH v3] [GSOC] commit: add --trailer option
- From
Rafael Silva <rafaeloliveira.cs@gmail.com>
- Date
- Mar 14, 2021, 13:10 UTC
- Message-ID
- <87czw1yj6q.fsf@gmail.com>
- In-Reply-To
- <pull.901.v3.git.1615726978059.gitgitgadget@gmail.com>
"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 37 quoted lines
> 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 :).
Show 17 quoted lines
> /*
> * 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....".
For example:
strvec_pushl(&run_trailer.args, "interpret-trailers", "--in-place", "--where=end", .....
Show 19 quoted lines
> 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" &&
Show 13 quoted lines
> + 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