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

[Junio C Hamano <gitster@pobox.com>]

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

> +--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).

> 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.

> +	}
> +

> 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.

> +	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.

> +'
> +
> +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.