Re: [PATCH v8 08/11] merge: cleanup messages like commit

[Denton Liu <liu.denton@gmail.com>]

Hi Phillip,

On Tue, Mar 19, 2019 at 11:13:37AM +0000, Phillip Wood wrote:
> Hi Denton
> 
> On 17/03/2019 10:16, Denton Liu wrote:
> > This change allows git-merge messages to be cleaned up with the
> > commit.cleanup configuration or --cleanup option, just like how
> > git-commit does it.
> > 
> > We also give git-pull the passthrough option of --cleanup so that it can
> > also take advantage of this change.
> > 
> > Finally, add testing to ensure that messages are properly cleaned up.
> > Note that some newlines that were added to the commit message were
> > removed so that if a file were read via -F, it would be copied
> > faithfully.
> > 
> > Reviewed-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> I'd echo Eric's comments about Reviewed-by tags.

Will do.

> 
> > Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
> > Signed-off-by: Denton Liu <liu.denton@gmail.com>
> > ---
> > 
> > Phillip Wood wrote:
> > > cleanup needs to take an argument so PARSE_OPT_NOARG does not look
> > > right. Also I think it would be bettor from the user's point of view if
> > > the value of the argument was checked by pull before it does any work
> > > rather otherwise if they pass in invalid value pull mostly runs and then
> > > merge errors out at the end.
> > 
> > I opted not to do a check on the validity of the value of the
> > --cleanup-mode argument because the strategy options that existed before
> > also didn't verify the validity of their values. In the future, it might
> > be a good idea to check the values of both cleanup-mode and the
> > strategy options but for now, I think we can leave it as it is.
> 
> With --strategy-option the valid values depend on the --strategy option
> which may invoke an external command so in general there is no way to check
> the values are valid (it could do for the strategies we know about). With
> --cleanup we know the valid values and have a function that can check for
> them so I think it would be worth doing.

Makes sense, I'll do it (and throw in a test case for free ;) )

> 
> >   Documentation/merge-options.txt |  5 +++
> >   builtin/merge.c                 | 31 +++++++++++----
> >   builtin/pull.c                  |  6 +++
> >   t/t7604-merge-custom-message.sh | 67 +++++++++++++++++++++++++++++++++
> >   wt-status.c                     | 12 ++++--
> >   wt-status.h                     |  1 +
> >   6 files changed, 112 insertions(+), 10 deletions(-)
> > 
> > diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
> > index 92a7d936c1..646100ea9a 100644
> > --- a/Documentation/merge-options.txt
> > +++ b/Documentation/merge-options.txt
> > @@ -32,6 +32,11 @@ they run `git merge`. To make it easier to adjust such scripts to the
> >   updated behaviour, the environment variable `GIT_MERGE_AUTOEDIT` can be
> >   set to `no` at the beginning of them.
> > +--cleanup=<mode>::
> > +	This option determines how the merge message will be cleaned up
> > +	before commiting or being passed on. See linkgit:git-commit[1] for more
> 
> As I commented before I don't understand 'passed on'

I changed it in git-revert.txt and git-cherry-pick.txt but I forgot to
change it here. What do you think about this:

	--cleanup=<mode>::
		This option determines how the merge message will be cleaned up
		before commiting or being passed on to the commit machinery. See
		linkgit:git-commit[1] for more details.

> 
> > +	details.
> > +
> >   --ff::
> >   	When the merge resolves as a fast-forward, only update the branch
> >   	pointer, without creating a merge commit.  This is the default
> > diff --git a/builtin/merge.c b/builtin/merge.c
> > index 5ce8946d39..7be03a2610 100644
> > --- a/builtin/merge.c
> > +++ b/builtin/merge.c
> > @@ -38,6 +38,7 @@
> >   #include "tag.h"
> >   #include "alias.h"
> >   #include "commit-reach.h"
> > +#include "wt-status.h"
> >   #define DEFAULT_TWOHEAD (1<<0)
> >   #define DEFAULT_OCTOPUS (1<<1)
> > @@ -98,6 +99,9 @@ enum ff_type {
> >   static enum ff_type fast_forward = FF_ALLOW;
> > +static const char *cleanup_arg;
> > +static enum commit_msg_cleanup_mode cleanup_mode;
> > +
> >   static int option_parse_message(const struct option *opt,
> >   				const char *arg, int unset)
> >   {
> > @@ -249,6 +253,7 @@ static struct option builtin_merge_options[] = {
> >   		N_("perform a commit if the merge succeeds (default)")),
> >   	OPT_BOOL('e', "edit", &option_edit,
> >   		N_("edit message before committing")),
> > +	OPT_CLEANUP(&cleanup_arg),
> >   	OPT_SET_INT(0, "ff", &fast_forward, N_("allow fast-forward (default)"), FF_ALLOW),
> >   	OPT_SET_INT_F(0, "ff-only", &fast_forward,
> >   		      N_("abort if fast-forward is not possible"),
> > @@ -612,6 +617,8 @@ static int git_merge_config(const char *k, const char *v, void *cb)
> >   		return git_config_string(&pull_twohead, k, v);
> >   	else if (!strcmp(k, "pull.octopus"))
> >   		return git_config_string(&pull_octopus, k, v);
> > +	else if (!strcmp(k, "commit.cleanup"))
> > +		return git_config_string(&cleanup_arg, k, v);
> >   	else if (!strcmp(k, "merge.renormalize"))
> >   		option_renormalize = git_config_bool(k, v);
> >   	else if (!strcmp(k, "merge.ff")) {
> > @@ -797,23 +804,32 @@ static void abort_commit(struct commit_list *remoteheads, const char *err_msg)
> >   	exit(1);
> >   }
> > +static const char comment_line_explanation[] =
> > +N_("Lines starting with '%c' will be ignored.\n");
> > +
> >   static const char merge_editor_comment[] =
> >   N_("Please enter a commit message to explain why this merge is necessary,\n"
> >      "especially if it merges an updated upstream into a topic branch.\n"
> >      "\n"
> > -   "Lines starting with '%c' will be ignored, and an empty message aborts\n"
> > -   "the commit.\n");
> > +   "An empty message aborts the commit.\n");
> >   static void write_merge_heads(struct commit_list *);
> >   static void prepare_to_commit(struct commit_list *remoteheads)
> >   {
> >   	struct strbuf msg = STRBUF_INIT;
> >   	strbuf_addbuf(&msg, &merge_msg);
> > -	strbuf_addch(&msg, '\n');
> >   	if (squash)
> >   		BUG("the control must not reach here under --squash");
> > -	if (0 < option_edit)
> > -		strbuf_commented_addf(&msg, _(merge_editor_comment), comment_line_char);
> > +	if (0 < option_edit) {
> > +		strbuf_addch(&msg, '\n');
> > +		if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS)
> > +			wt_status_append_cut_line(&msg);
> > +		else
> > +			strbuf_commented_addf(&msg, _(comment_line_explanation), comment_line_char);
> > +
> > +		strbuf_commented_addf(&msg, "\n");
> > +		strbuf_commented_addf(&msg, _(merge_editor_comment));
> > +	}
> 
> This still changes the wording of the message cf https://public-inbox.org/git/cover.1552275703.git.liu.denton@gmail.com/T/#m09cb1a05eb3bffb47ee9f25572904a7279efa362

Will do.

Thanks again for reviewing carefully!

> 
> Best Wishes
> 
> Phillip
> 
> >   	if (signoff)
> >   		append_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);
> >   	write_merge_heads(remoteheads);
> > @@ -832,7 +848,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
> >   		abort_commit(remoteheads, NULL);
> >   	read_merge_msg(&msg);
> > -	strbuf_stripspace(&msg, 0 < option_edit);
> > +	cleanup_message(&msg, cleanup_mode, 0);
> >   	if (!msg.len)
> >   		abort_commit(remoteheads, _("Empty commit message."));
> >   	strbuf_release(&merge_msg);
> > @@ -880,7 +896,6 @@ static int finish_automerge(struct commit *head,
> >   	parents = remoteheads;
> >   	if (!head_subsumed || fast_forward == FF_NO)
> >   		commit_list_insert(head, &parents);
> > -	strbuf_addch(&merge_msg, '\n');
> >   	prepare_to_commit(remoteheads);
> >   	if (commit_tree(merge_msg.buf, merge_msg.len, result_tree, parents,
> >   			&result_commit, NULL, sign_commit))
> > @@ -1389,6 +1404,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
> >   	if (option_edit < 0)
> >   		option_edit = default_edit_option();
> > +	cleanup_mode = get_cleanup_mode(cleanup_arg, 0 < option_edit, 1);
> > +
> >   	if (!use_strategies) {
> >   		if (!remoteheads)
> >   			; /* already up-to-date */
> > diff --git a/builtin/pull.c b/builtin/pull.c
> > index 33db889955..292c1dac27 100644
> > --- a/builtin/pull.c
> > +++ b/builtin/pull.c
> > @@ -101,6 +101,7 @@ static char *opt_signoff;
> >   static char *opt_squash;
> >   static char *opt_commit;
> >   static char *opt_edit;
> > +static char *opt_cleanup;
> >   static char *opt_ff;
> >   static char *opt_verify_signatures;
> >   static int opt_autostash = -1;
> > @@ -168,6 +169,9 @@ static struct option pull_options[] = {
> >   	OPT_PASSTHRU(0, "edit", &opt_edit, NULL,
> >   		N_("edit message before committing"),
> >   		PARSE_OPT_NOARG),
> > +	OPT_PASSTHRU(0, "cleanup", &opt_cleanup, NULL,
> > +		N_("how to strip spaces and #comments from message"),
> > +		0),
> >   	OPT_PASSTHRU(0, "ff", &opt_ff, NULL,
> >   		N_("allow fast-forward"),
> >   		PARSE_OPT_NOARG),
> > @@ -644,6 +648,8 @@ static int run_merge(void)
> >   		argv_array_push(&args, opt_commit);
> >   	if (opt_edit)
> >   		argv_array_push(&args, opt_edit);
> > +	if (opt_cleanup)
> > +		argv_array_push(&args, opt_cleanup);
> >   	if (opt_ff)
> >   		argv_array_push(&args, opt_ff);
> >   	if (opt_verify_signatures)
> > diff --git a/t/t7604-merge-custom-message.sh b/t/t7604-merge-custom-message.sh
> > index b045fdb413..c9685a318d 100755
> > --- a/t/t7604-merge-custom-message.sh
> > +++ b/t/t7604-merge-custom-message.sh
> > @@ -51,4 +51,71 @@ test_expect_success 'merge --log appends to custom message' '
> >   	test_cmp exp.log actual
> >   '
> > +mesg_with_comment_and_newlines='
> > +# text
> > +
> > +'
> > +
> > +test_expect_success 'prepare file with comment line and trailing newlines'  '
> > +	printf "%s" "$mesg_with_comment_and_newlines" >expect
> > +'
> > +
> > +test_expect_success 'cleanup commit messages (verbatim option)' '
> > +	git reset --hard c1 &&
> > +	git merge --cleanup=verbatim -F expect c2 &&
> > +	git cat-file commit HEAD >actual &&
> > +	sed -e "1,/^$/d" <actual >tmp &&
> > +	mv tmp actual &&
> > +	test_cmp expect actual
> > +'
> > +
> > +test_expect_success 'cleanup commit messages (whitespace option)' '
> > +	git reset --hard c1 &&
> > +	test_write_lines "" "# text" "" >text &&
> > +	echo "# text" >expect &&
> > +	git merge --cleanup=whitespace -F text c2 &&
> > +	git cat-file commit HEAD >actual &&
> > +	sed -e "1,/^$/d" <actual >tmp &&
> > +	mv tmp actual &&
> > +	test_cmp expect actual
> > +'
> > +
> > +test_expect_success 'cleanup merge messages (scissors option)' '
> > +	git reset --hard c1 &&
> > +	cat >text <<-\EOF &&
> > +
> > +	# to be kept
> > +
> > +	  # ------------------------ >8 ------------------------
> > +	# to be kept, too
> > +	# ------------------------ >8 ------------------------
> > +	to be removed
> > +	# ------------------------ >8 ------------------------
> > +	to be removed, too
> > +	EOF
> > +
> > +	cat >expect <<-\EOF &&
> > +	# to be kept
> > +
> > +	  # ------------------------ >8 ------------------------
> > +	# to be kept, too
> > +	EOF
> > +	git merge --cleanup=scissors -e -F text c2 &&
> > +	git cat-file commit HEAD >actual &&
> > +	sed -e "1,/^$/d" <actual >tmp &&
> > +	mv tmp actual &&
> > +	test_cmp expect actual
> > +'
> > +
> > +test_expect_success 'cleanup commit messages (strip option)' '
> > +	git reset --hard c1 &&
> > +	test_write_lines "" "# text" "sample" "" >text &&
> > +	echo sample >expect &&
> > +	git merge --cleanup=strip -F text c2 &&
> > +	git cat-file commit HEAD >actual &&
> > +	sed -e "1,/^$/d" <actual >tmp &&
> > +	mv tmp actual &&
> > +	test_cmp expect actual
> > +'
> > +
> >   test_done
> > diff --git a/wt-status.c b/wt-status.c
> > index 445a36204a..b81fcd428d 100644
> > --- a/wt-status.c
> > +++ b/wt-status.c
> > @@ -1006,13 +1006,19 @@ size_t wt_status_locate_end(const char *s, size_t len)
> >   	return len;
> >   }
> > -void wt_status_add_cut_line(FILE *fp)
> > +void wt_status_append_cut_line(struct strbuf *buf)
> >   {
> >   	const char *explanation = _("Do not modify or remove the line above.\nEverything below it will be ignored.");
> > +
> > +	strbuf_commented_addf(buf, "%s", cut_line);
> > +	strbuf_add_commented_lines(buf, explanation, strlen(explanation));
> > +}
> > +
> > +void wt_status_add_cut_line(FILE *fp)
> > +{
> >   	struct strbuf buf = STRBUF_INIT;
> > -	fprintf(fp, "%c %s", comment_line_char, cut_line);
> > -	strbuf_add_commented_lines(&buf, explanation, strlen(explanation));
> > +	wt_status_append_cut_line(&buf);
> >   	fputs(buf.buf, fp);
> >   	strbuf_release(&buf);
> >   }
> > diff --git a/wt-status.h b/wt-status.h
> > index 3a95975032..64f1ddc9fd 100644
> > --- a/wt-status.h
> > +++ b/wt-status.h
> > @@ -129,6 +129,7 @@ struct wt_status {
> >   };
> >   size_t wt_status_locate_end(const char *s, size_t len);
> > +void wt_status_append_cut_line(struct strbuf *buf);
> >   void wt_status_add_cut_line(FILE *fp);
> >   void wt_status_prepare(struct repository *r, struct wt_status *s);
> >   void wt_status_print(struct wt_status *s);
> >