diff mbox series

[v3,1/1] merge: add scissors line on merge conflict

Message ID 5c1aa82794da23077ef4ec4e836ee221c64162ef.1542496915.git.liu.denton@gmail.com (mailing list archive)
State New, archived
Headers show
Series Fix scissors bug during merge conflict | expand

Commit Message

Denton Liu Nov. 17, 2018, 11:32 p.m. UTC
This fixes a bug where the scissors line is placed after the Conflicts:
section, in the case where a merge conflict occurs and
merge.cleanup = scissors.

Next, if commit.cleanup = scissors is specified, don't produce a
scissors line in commit if one already exists in the MERGE_MSG file.

Finally, we give pull the passthrough option of --cleanup so that it
can also take advantage of this change.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/merge-options.txt |  6 +++++
 builtin/commit.c                | 20 ++++++++++----
 builtin/merge.c                 | 16 +++++++++++
 builtin/pull.c                  |  6 +++++
 t/t7600-merge.sh                | 48 +++++++++++++++++++++++++++++++++
 5 files changed, 91 insertions(+), 5 deletions(-)

Comments

SZEDER Gábor Nov. 18, 2018, 2:18 p.m. UTC | #1
On Sat, Nov 17, 2018 at 06:32:33PM -0500, Denton Liu wrote:
> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
> index 106148254d..0d3db34f08 100755
> --- a/t/t7600-merge.sh
> +++ b/t/t7600-merge.sh
> @@ -247,6 +247,54 @@ test_expect_success 'merge --squash c3 with c7' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'merge c3 with c7 with commit.cleanup = scissors' '
> +	git config commit.cleanup scissors &&
> +	git reset --hard c3 &&
> +	test_must_fail git merge c7 &&
> +	cat result.9z >file &&
> +	git commit --no-edit -a &&
> +
> +	{
> +		cat <<-EOF
> +		Merge tag '"'"'c7'"'"'
> +
> +		# ------------------------ >8 ------------------------
> +		# Do not modify or remove the line above.
> +		# Everything below it will be ignored.

Note that these two lines of advice text are translated; see the
consequences below.

> +		#
> +		# Conflicts:
> +		#	file
> +		EOF
> +	} >expect &&

The {...} block is unnecessary, because there is only a single command
in there.

> +	git cat-file commit HEAD | sed -e '1,/^$/d' >actual &&

Please don't run git commands upstream of a pipe, because the pipe
hides their exit code.  Furthermore, put the sed script inside double
quotes, because the whole test is already in a single-quoted block.

I presume you wrote the test this way because you simply followed suit
of the previous test 'merge --squash c3 with c7', which did all the
same.  Bonus points for a preparatory patch that cleans up the
previous test ;)

> +	test_cmp expect actual

But most importantly, here 'test_cmp' compares translated advice text
as well, which fails in the GETTEXT_POISON build.  Use 'test_i18ncmp'
instead.

> +'
> +
> +test_expect_success 'merge c3 with c7 with --squash commit.cleanup = scissors' '
> +	git config commit.cleanup scissors &&
> +	git reset --hard c3 &&
> +	test_must_fail git merge --squash c7 &&
> +	cat result.9z >file &&
> +	git commit --no-edit -a &&
> +
> +	{
> +		cat <<-EOF
> +		Squashed commit of the following:
> +
> +		$(git show -s c7)
> +
> +		# ------------------------ >8 ------------------------
> +		# Do not modify or remove the line above.
> +		# Everything below it will be ignored.
> +		#
> +		# Conflicts:
> +		#	file
> +		EOF
> +	} >expect &&
> +	git cat-file commit HEAD | sed -e '1,/^$/d' >actual &&
> +	test_cmp expect actual

Likewise.

> +'
> +
>  test_debug 'git log --graph --decorate --oneline --all'
>  
>  test_expect_success 'merge c1 with c2 and c3' '
> -- 
> 2.19.1
>
diff mbox series

Patch

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 63a3fc0954..115e0ca6f0 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -27,6 +27,12 @@  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 being passed on. Specifically, if the '<mode>' is given a
+	value of `scissors`, scissors will be prepended to the message in
+	the case of a merge conflict. See also linkgit:git-commit[1].
+
 --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/commit.c b/builtin/commit.c
index 0d9828e29e..7902645bc9 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -659,6 +659,7 @@  static int prepare_to_commit(const char *index_file, const char *prefix,
 	const char *hook_arg2 = NULL;
 	int clean_message_contents = (cleanup_mode != COMMIT_MSG_CLEANUP_NONE);
 	int old_display_comment_prefix;
+	int merge_contains_scissors = 0;
 
 	/* This checks and barfs if author is badly specified */
 	determine_author_info(author_ident);
@@ -719,6 +720,8 @@  static int prepare_to_commit(const char *index_file, const char *prefix,
 			strbuf_addbuf(&sb, &message);
 		hook_arg1 = "message";
 	} else if (!stat(git_path_merge_msg(the_repository), &statbuf)) {
+		size_t merge_msg_start;
+
 		/*
 		 * prepend SQUASH_MSG here if it exists and a
 		 * "merge --squash" was originally performed
@@ -729,8 +732,14 @@  static int prepare_to_commit(const char *index_file, const char *prefix,
 			hook_arg1 = "squash";
 		} else
 			hook_arg1 = "merge";
+
+		merge_msg_start = sb.len;
 		if (strbuf_read_file(&sb, git_path_merge_msg(the_repository), 0) < 0)
 			die_errno(_("could not read MERGE_MSG"));
+
+		if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
+		    wt_status_locate_end(sb.buf + merge_msg_start, sb.len - merge_msg_start) < sb.len - merge_msg_start)
+			merge_contains_scissors = 1;
 	} else if (!stat(git_path_squash_msg(the_repository), &statbuf)) {
 		if (strbuf_read_file(&sb, git_path_squash_msg(the_repository), 0) < 0)
 			die_errno(_("could not read SQUASH_MSG"));
@@ -798,7 +807,8 @@  static int prepare_to_commit(const char *index_file, const char *prefix,
 		struct ident_split ci, ai;
 
 		if (whence != FROM_COMMIT) {
-			if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS)
+			if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
+				!merge_contains_scissors)
 				wt_status_add_cut_line(s->fp);
 			status_printf_ln(s, GIT_COLOR_NORMAL,
 			    whence == FROM_MERGE
@@ -823,10 +833,10 @@  static int prepare_to_commit(const char *index_file, const char *prefix,
 				_("Please enter the commit message for your changes."
 				  " Lines starting\nwith '%c' will be ignored, and an empty"
 				  " message aborts the commit.\n"), comment_line_char);
-		else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
-			 whence == FROM_COMMIT)
-			wt_status_add_cut_line(s->fp);
-		else /* COMMIT_MSG_CLEANUP_SPACE, that is. */
+		else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {
+			if (whence == FROM_COMMIT && !merge_contains_scissors)
+				wt_status_add_cut_line(s->fp);
+		} else /* COMMIT_MSG_CLEANUP_SPACE, that is. */
 			status_printf(s, GIT_COLOR_NORMAL,
 				_("Please enter the commit message for your changes."
 				  " Lines starting\n"
diff --git a/builtin/merge.c b/builtin/merge.c
index 8f4a5065c2..23a6e6bb93 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -36,6 +36,7 @@ 
 #include "packfile.h"
 #include "tag.h"
 #include "alias.h"
+#include "wt-status.h"
 
 #define DEFAULT_TWOHEAD (1<<0)
 #define DEFAULT_OCTOPUS (1<<1)
@@ -96,6 +97,9 @@  enum ff_type {
 
 static enum ff_type fast_forward = FF_ALLOW;
 
+static const char *cleanup_arg;
+static int put_scissors;
+
 static int option_parse_message(const struct option *opt,
 				const char *arg, int unset)
 {
@@ -243,6 +247,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_STRING(0, "cleanup", &cleanup_arg, N_("default"), N_("how to strip spaces and #comments from message")),
 	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"),
@@ -606,6 +611,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")) {
@@ -894,6 +901,13 @@  static int suggest_conflicts(void)
 	filename = git_path_merge_msg(the_repository);
 	fp = xfopen(filename, "a");
 
+	if (put_scissors) {
+	    fputc('\n', fp);
+	    wt_status_add_cut_line(fp);
+	    /* comments out the newline from append_conflicts_hint */
+	    fputc(comment_line_char, fp);
+	}
+
 	append_conflicts_hint(&msgbuf);
 	fputs(msgbuf.buf, fp);
 	strbuf_release(&msgbuf);
@@ -1402,6 +1416,8 @@  int cmd_merge(int argc, const char **argv, const char *prefix)
 	if (option_edit < 0)
 		option_edit = default_edit_option();
 
+	put_scissors = cleanup_arg && !strcmp(cleanup_arg, "scissors");
+
 	if (!use_strategies) {
 		if (!remoteheads)
 			; /* already up-to-date */
diff --git a/builtin/pull.c b/builtin/pull.c
index 681c127a07..88245bce0e 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -95,6 +95,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;
@@ -162,6 +163,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"),
+		PARSE_OPT_NOARG),
 	OPT_PASSTHRU(0, "ff", &opt_ff, NULL,
 		N_("allow fast-forward"),
 		PARSE_OPT_NOARG),
@@ -625,6 +629,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/t7600-merge.sh b/t/t7600-merge.sh
index 106148254d..0d3db34f08 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -247,6 +247,54 @@  test_expect_success 'merge --squash c3 with c7' '
 	test_cmp expect actual
 '
 
+test_expect_success 'merge c3 with c7 with commit.cleanup = scissors' '
+	git config commit.cleanup scissors &&
+	git reset --hard c3 &&
+	test_must_fail git merge c7 &&
+	cat result.9z >file &&
+	git commit --no-edit -a &&
+
+	{
+		cat <<-EOF
+		Merge tag '"'"'c7'"'"'
+
+		# ------------------------ >8 ------------------------
+		# Do not modify or remove the line above.
+		# Everything below it will be ignored.
+		#
+		# Conflicts:
+		#	file
+		EOF
+	} >expect &&
+	git cat-file commit HEAD | sed -e '1,/^$/d' >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'merge c3 with c7 with --squash commit.cleanup = scissors' '
+	git config commit.cleanup scissors &&
+	git reset --hard c3 &&
+	test_must_fail git merge --squash c7 &&
+	cat result.9z >file &&
+	git commit --no-edit -a &&
+
+	{
+		cat <<-EOF
+		Squashed commit of the following:
+
+		$(git show -s c7)
+
+		# ------------------------ >8 ------------------------
+		# Do not modify or remove the line above.
+		# Everything below it will be ignored.
+		#
+		# Conflicts:
+		#	file
+		EOF
+	} >expect &&
+	git cat-file commit HEAD | sed -e '1,/^$/d' >actual &&
+	test_cmp expect actual
+'
+
 test_debug 'git log --graph --decorate --oneline --all'
 
 test_expect_success 'merge c1 with c2 and c3' '