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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 19, 2021, 17:48 UTC
Message-ID: <xmqq4kh7nifp.fsf@gitster.g>
In-Reply-To: <pull.901.v11.git.1616155517590.gitgitgadget@gmail.com>

"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 6 quoted lines

> +--trailer <token>[(=|:)<value>]::
> +	Specify a (<token>, <value>) pair that should be applied as a
> +	trailer. (e.g. `git commit --trailer "Signed-off-by:C O Mitter \
> +	<committer@example.com>" --trailer "Helped-by:C O Mitter \
> +	<committer@example.com>"` will add the "Signed-off-by" trailer
> +	and the "Helped-by" trailer in the commit message.)

s/in the commit message/to the commit message/ probably.

> +	Use `git -c trailer.* commit --trailer` to make the appropriate
> +	configuration. See linkgit:git-interpret-trailers[1] for details.

I doubt this is a good advice for a few reasons.

 (1) The "git -c var=val" is meant to be used as a single-shot
     oddball configuration.  If the user will be working on the
     project long enough to be worth using the --trailer option
     (otherwise a single-shot drive-by patch can just add these
     trailers while editing the log message in the editor), the user
     would not want to use "git -c var=val" mechanism to use
     different configuration every time the --trailer option is
     used.

 (2) The "appropriate configuration" is too vague and does not give
     enough incentive to the reader to go look in the other manual
     page.  At least there should be a cursory mention of what kind
     of things are possible by the configuration.

Prehaps

    The `trailer.*` configuration variables (see linkgit:...) can be
    used to define if a duplicated trailer is omitted, where in the
    run of trailers each trailer would appear, and other details.

or something along the line (Christian would be a better person to suggest what good examples are than I am, though).

Show 41 quoted lines

> diff --git a/builtin/commit.c b/builtin/commit.c
> index 739110c5a7f6..4b06672bd07d 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -113,6 +113,7 @@ 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;
> +static struct strvec trailer_args = STRVEC_INIT;
>  
>  /*
>   * The default commit message cleanup mode will remove the lines
> @@ -131,6 +132,14 @@ 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)
> +{
> +	BUG_ON_OPT_NEG(unset);
> +
> +	strvec_pushl(&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 +967,18 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  
>  	fclose(s->fp);
>  
> +	if (trailer_args.nr) {
> +		struct child_process run_trailer = CHILD_PROCESS_INIT;
> +
> +		strvec_pushl(&run_trailer.args, "interpret-trailers",
> +			     "--in-place", git_path_commit_editmsg(), NULL);
> +		strvec_pushv(&run_trailer.args, trailer_args.v);
> +		run_trailer.git_cmd = 1;
> +		if (run_command(&run_trailer))
> +			die(_("unable to pass trailers to --trailers"));
> +		strvec_clear(&trailer_args);

OK. run_command() cleans the run_trailer.args when it returns, so we only need to clear our own array here.

> +	}
> +

Show 26 quoted lines

> diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
> index 6396897cc818..024cf3c81b18 100755
> --- a/t/t7502-commit-porcelain.sh
> +++ b/t/t7502-commit-porcelain.sh
> @@ -154,6 +154,341 @@ test_expect_success 'sign off' '
>  
>  '
>  
> +test_expect_success 'commit --trailer without -c' '
> +	echo "fun" >>file &&
> +	git add file &&
> +	cat >expected <<-\EOF &&
> +
> +	Signed-off-by: C O Mitter <committer@example.com>
> +	Signed-off-by: C1 E1
> +	Helped-by: C2 E2
> +	Reported-by: C3 E3
> +	Mentored-by: C4 E4
> +	EOF
> +	git commit -s --trailer "Signed-off-by:C1 E1 " \
> +		--trailer "Helped-by:C2 E2 " \
> +		--trailer "Reported-by:C3 E3" \
> +		--trailer "Mentored-by:C4 E4" \
> +		-m "hello" &&
> +	git cat-file commit HEAD >commit.msg &&
> +	sed -e "1,6d" commit.msg >actual &&

This 1,6d depends on the exact line organization of the underlying object detail, which is not good. You'd want to grab the run of the consecutive non-empty lines at the end, so that commit object headers and the log message "fun" can change over time by changes to Git and changes to this test, without breaking this test.

Show 21 quoted lines

> +	test_cmp expected actual
> +'
> +
> +test_expect_success 'commit --trailer with -c and "replace" as ifexists' '
> +	echo "fun" >>file1 &&
> +	git add file1 &&
> +	cat >expected <<-\EOF &&
> +
> +	Signed-off-by: C O Mitter <committer@example.com>
> +	Signed-off-by: C1 E1
> +	Reported-by: C3 E3
> +	Mentored-by: C4 E4
> +	Helped-by: C3 E3
> +	EOF
> +	git -c trailer.ifexists="replace" \
> +		commit --trailer "Mentored-by: C4 E4" \
> +		 --trailer "Helped-by: C3 E3" \
> +		--amend &&
> +	git cat-file commit HEAD >commit.msg &&
> +	sed -e "1,6d" commit.msg >actual &&
> +	test_cmp expected actual

The same comment applies, and also by using "--amend", this relies on the outcome of the previous test, which is not great.

Show 19 quoted lines

> +'
> +
> +test_expect_success 'commit --trailer with -c and "add" as ifexists' '
> +	echo "fun" >>file1 &&
> +	git add file1 &&
> +	cat >expected <<-\EOF &&
> +
> +	Signed-off-by: C O Mitter <committer@example.com>
> +	Signed-off-by: C1 E1
> +	Reported-by: C3 E3
> +	Mentored-by: C4 E4
> +	Helped-by: C3 E3
> +	Helped-by: C3 E3
> +	Helped-by: C3 E3
> +	EOF
> +	git -c trailer.ifexists="add" \
> +		commit --trailer "Helped-by: C3 E3" \
> +		--trailer "Helped-by: C3 E3" \
> +		--amend &&

And it makes things worse by keep amending.

At least, establish a baseline commit that has known set of trailers, tag it, and reset the HEAD to that commit at the beginning of each test that tries to amend an existing commit. That way, the correctness of each individual test would depend only on the test that creates the baseline commit and tags it.