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

[Rafael Silva <rafaeloliveira.cs@gmail.com>]

"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 :).

>  	/*
>  	 * 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", .....

>  	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