threads / patch / 49554

patch, 4 partsMore merge cleanups

Subject: [PATCH 0/4] More merge cleanups

## tl;dr

15 messages between Oct 12, 2018 and Oct 18, 2018. Diffs are folded; open one to read it.

replies: 14people: 2as markdown or json

Elijah Newren· Oct 12, 2018, 21:25 UTC · lore

This series builds on en/merge-cleanup. Technically, this could be broken into three separate topics with only one of them depending on en/merge-cleanup, but I have a subsequent series that depends on both en/merge-cleanup and the fixes here, so I'm submitting these four patches as a set.

Elijah Newren (4):
  t6036: add testcase where virtual merge base contains nested conflicts
  merge-recursive: increase marker length with depth of recursion

    Improving diff3 conflict markers in the face of arbitrarily deeply
    nested conflicts

  merge-recursive: improve auto-merging messages with path collisions

    Simple attempt at wording improvement

  merge-recursive: Avoid showing conflicts with merge branch before HEAD

    Conflict markers are expected to be shown in a certain order

 ll-merge.c                        |   4 +-
 ll-merge.h                        |   1 +
 merge-recursive.c                 |  59 +++++++++--
 t/t6036-recursive-corner-cases.sh | 159 +++++++++++++++++++++++++++++-
 4 files changed, 208 insertions(+), 15 deletions(-)

-- 
2.19.0.235.g7c386e1068

Elijah Newren· Oct 12, 2018, 21:25 UTC · re: Elijah Newren · lore

[PATCH 3/4] merge-recursive: improve auto-merging messages with path collisions

Each individual file involved in a rename could have also been modified on both sides of history, meaning it may need to have content merges. If two such files are renamed into the same location, then on top of the two natural auto-merging messages we also have to two-way merge the result, giving us messages that look like

  Auto-merging somefile.c (was somecase.c)
  Auto-merging somefile.c (was somefolder.c)
  Auto-merging somefile.c

However, despite the fact that I was the one who put the "(was %s)" portions into the messages (and just a few months ago), I was still initially confused when running into a rename/rename(2to1) case and wondered if somefile.c had been merged three times. Update this to instead be:

  Auto-merging version of somefile.c from somecase.c
  Auto-merging version of somefile.c from someportfolio.c
  Auto-merging somefile.c

This is an admittedly long set of messages for a single path, but you only get all three messages when dealing with the rare case of a rename/rename(2to1) conflict where both sides of both original files were also modified, in conflicting ways.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Show changes to merge-recursive.c +2 −2

diff --git a/merge-recursive.c b/merge-recursive.c
index 2452788d28..33cd9ee81f 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1679,8 +1679,8 @@ static int handle_rename_rename_2to1(struct merge_options *o,
 	remove_file(o, 1, a->path, o->call_depth || would_lose_untracked(a->path));
 	remove_file(o, 1, b->path, o->call_depth || would_lose_untracked(b->path));
 
-	path_side_1_desc = xstrfmt("%s (was %s)", path, a->path);
-	path_side_2_desc = xstrfmt("%s (was %s)", path, b->path);
+	path_side_1_desc = xstrfmt("version of %s from %s", path, a->path);
+	path_side_2_desc = xstrfmt("version of %s from %s", path, b->path);
 	if (merge_mode_and_contents(o, a, c1, &ci->ren1_other, path_side_1_desc,
 				    o->branch1, o->branch2,
 				    o->call_depth * 2, &mfi_c1) ||

-- 
2.19.0.235.g7c386e1068

Junio C Hamano· Oct 15, 2018, 05:18 UTC · re: Elijah Newren · lore

Re: [PATCH 3/4] merge-recursive: improve auto-merging messages with path collisions

Elijah Newren <newren@gmail.com> writes:

Show 24 quoted lines

> Each individual file involved in a rename could have also been modified
> on both sides of history, meaning it may need to have content merges.
> If two such files are renamed into the same location, then on top of the
> two natural auto-merging messages we also have to two-way merge the
> result, giving us messages that look like
>
>   Auto-merging somefile.c (was somecase.c)
>   Auto-merging somefile.c (was somefolder.c)
>   Auto-merging somefile.c
>
> However, despite the fact that I was the one who put the "(was %s)"
> portions into the messages (and just a few months ago), I was still
> initially confused when running into a rename/rename(2to1) case and
> wondered if somefile.c had been merged three times.  Update this to
> instead be:
>
>   Auto-merging version of somefile.c from somecase.c
>   Auto-merging version of somefile.c from someportfolio.c
>   Auto-merging somefile.c
>
> This is an admittedly long set of messages for a single path, but you
> only get all three messages when dealing with the rare case of a
> rename/rename(2to1) conflict where both sides of both original files
> were also modified, in conflicting ways.

Yeah, that does look mouthful, but definitely is more understandable.

Elijah Newren· Oct 12, 2018, 21:25 UTC · re: Elijah Newren · lore

[PATCH 4/4] merge-recursive: Avoid showing conflicts with merge branch before HEAD

We want to load unmerged entries from HEAD into the index at stage 2 and from MERGE_HEAD into stage 3. Similarly, folks expect merge conflicts to look like

<<<<<<<< HEAD content from our side ======== content from their side

>>>>>>>> MERGE_HEAD

not

<<<<<<<< MERGE_HEAD content from their side ======== content from our side

>>>>>>>> HEAD

The correct order usually comes naturally and for free, but with renames we often have data in the form {rename_branch, other_branch}, and working relative to the rename first (e.g. for rename/add) is more convenient elsewhere in the code. Address the slight impedance mismatch by having some functions re-call themselves with flipped arguments when the branch order is reversed.

Note that setup_rename_conflict_info() has one asymmetry in it, in setting dst_entry1->processed=0 but not doing similarly for dst_entry2->processed. When dealing with rename/rename and similar conflicts, we do not want the processing to happen twice, so the desire to only set one of the entries to unprocessed is intentional. So, while this change modifies which branch's entry will be marked as unprocessed, that dovetails nicely with putting HEAD first so that we get the index stage entries and conflict markers in the right order.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c                 | 33 ++++++++++++++++++++++++++++++-
 t/t6036-recursive-corner-cases.sh |  8 ++++----
 2 files changed, 36 insertions(+), 5 deletions(-)

Show changes to 2 files +36 −5

merge-recursive.c, t/t6036-recursive-corner-cases.sh

diff --git a/merge-recursive.c b/merge-recursive.c
index 33cd9ee81f..f795c92a69 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -228,7 +228,26 @@ static inline void setup_rename_conflict_info(enum rename_type rename_type,
 					      struct stage_data *src_entry1,
 					      struct stage_data *src_entry2)
 {
-	struct rename_conflict_info *ci = xcalloc(1, sizeof(struct rename_conflict_info));
+	struct rename_conflict_info *ci;
+
+	/*
+	 * When we have two renames involved, it's easiest to get the
+	 * correct things into stage 2 and 3, and to make sure that the
+	 * content merge puts HEAD before the other branch if we just
+	 * ensure that branch1 == o->branch1.  So, simply flip arguments
+	 * around if we don't have that.
+	 */
+	if (dst_entry2 && branch1 != o->branch1) {
+		setup_rename_conflict_info(rename_type,
+					   pair2,      pair1,
+					   branch2,    branch1,
+					   dst_entry2, dst_entry1,
+					   o,
+					   src_entry2, src_entry1);
+		return;
+	}
+
+	ci = xcalloc(1, sizeof(struct rename_conflict_info));
 	ci->rename_type = rename_type;
 	ci->pair1 = pair1;
 	ci->branch1 = branch1;
@@ -1286,6 +1305,18 @@ static int merge_mode_and_contents(struct merge_options *o,
 				   const int extra_marker_size,
 				   struct merge_file_info *result)
 {
+	if (o->branch1 != branch1) {
+		/*
+		 * It's weird getting a reverse merge with HEAD on the bottom
+		 * side of the conflict markers and the other branch on the
+		 * top.  Fix that.
+		 */
+		return merge_mode_and_contents(o, one, b, a,
+					       filename,
+					       branch2, branch1,
+					       extra_marker_size, result);
+	}
+
 	result->merge = 0;
 	result->clean = 1;
 
diff --git a/t/t6036-recursive-corner-cases.sh b/t/t6036-recursive-corner-cases.sh
index 21954db624..276b4e8792 100755
--- a/t/t6036-recursive-corner-cases.sh
+++ b/t/t6036-recursive-corner-cases.sh
@@ -230,13 +230,13 @@ test_expect_success 'git detects differently handled merges conflict' '
 			:2:new_a :3:new_a &&
 		test_cmp expect actual &&
 
-		git cat-file -p B:new_a >ours &&
-		git cat-file -p C:new_a >theirs &&
+		git cat-file -p C:new_a >ours &&
+		git cat-file -p B:new_a >theirs &&
 		>empty &&
 		test_must_fail git merge-file \
-			-L "Temporary merge branch 2" \
-			-L "" \
 			-L "Temporary merge branch 1" \
+			-L "" \
+			-L "Temporary merge branch 2" \
 			ours empty theirs &&
 		sed -e "s/^\([<=>]\)/\1\1\1/" ours >expect &&
 		git cat-file -p :1:new_a >actual &&

-- 
2.19.0.235.g7c386e1068

Junio C Hamano· Oct 15, 2018, 05:23 UTC · re: Elijah Newren · lore

Re: [PATCH 4/4] merge-recursive: Avoid showing conflicts with merge branch before HEAD

Elijah Newren <newren@gmail.com> writes:

Show 6 quoted lines

> The correct order usually comes naturally and for free, but with renames
> we often have data in the form {rename_branch, other_branch}, and
> working relative to the rename first (e.g. for rename/add) is more
> convenient elsewhere in the code.  Address the slight impedance
> mismatch by having some functions re-call themselves with flipped
> arguments when the branch order is reversed.

I've never noticed or felt disturbed myself, but thanks for this level of attention to the detail.

Show 23 quoted lines

> @@ -228,7 +228,26 @@ static inline void setup_rename_conflict_info(enum rename_type rename_type,
>  					      struct stage_data *src_entry1,
>  					      struct stage_data *src_entry2)
>  {
> -	struct rename_conflict_info *ci = xcalloc(1, sizeof(struct rename_conflict_info));
> +	struct rename_conflict_info *ci;
> +
> +	/*
> +	 * When we have two renames involved, it's easiest to get the
> +	 * correct things into stage 2 and 3, and to make sure that the
> +	 * content merge puts HEAD before the other branch if we just
> +	 * ensure that branch1 == o->branch1.  So, simply flip arguments
> +	 * around if we don't have that.
> +	 */
> +	if (dst_entry2 && branch1 != o->branch1) {
> +		setup_rename_conflict_info(rename_type,
> +					   pair2,      pair1,
> +					   branch2,    branch1,
> +					   dst_entry2, dst_entry1,
> +					   o,
> +					   src_entry2, src_entry1);
> +		return;
> +	}

;-)

Elijah Newren· Oct 12, 2018, 21:25 UTC · re: Elijah Newren · lore

[PATCH 1/4] t6036: add testcase where virtual merge base contains nested conflicts

When merges involve content conflicts, merge.conflictstyle=diff3 can be used to also show the content for the file from the base version. If there was more than one merge base, then the "base version" is determined by merging the merge bases to create a virtual merge base. This means that the "base version" of the file can itself already have conflict markers. In the past, this could have created a situation where users have difficulty distinguishing between the inner and outer conflict markers. However, commit d694a17986a2 ("ll-merge: use a longer conflict marker for internal merge", 2016-04-14) addressed this issue by making conflict markers two characters longer for a virtual merge base.

Unfortunately, this solution was incomplete. The recursive merge algorithm is, as you'd expect from it's name, recursive in nature. In particular, this means that the virtual merge base itself could have had a virtual merge base of its own that contained conflict markers. In other words, we can have three (or more) levels of conflict markers when using merge.conflictstyle=diff3. Thus, what we need is for conflict marker length to increase with the depth of recursion. Add a testcase demonstrating the problem and testing for marker length increasing with increasing recursion depth.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t6036-recursive-corner-cases.sh | 151 ++++++++++++++++++++++++++++++
 1 file changed, 151 insertions(+)

Show changes to t/t6036-recursive-corner-cases.sh +151 −0

diff --git a/t/t6036-recursive-corner-cases.sh b/t/t6036-recursive-corner-cases.sh
index 59e52c5a09..1d1135082c 100755
--- a/t/t6036-recursive-corner-cases.sh
+++ b/t/t6036-recursive-corner-cases.sh
@@ -1402,4 +1402,155 @@ test_expect_failure 'check conflicting modes for regular file' '
 	)
 '
 
+# Setup:
+#          L1---L2---L3
+#         /  \ /  \ /  \
+#   master    X1   X2   ?
+#         \  / \  / \  /
+#          R1---R2---R3
+#
+# Where:
+#   master has one file named 'content'
+#   branches L1 and R1 both modify each of the two files in conflicting ways
+#
+#   L<n> (n>1) is a merge of R<n-1> into L<n-1>
+#   R<n> (n>1) is a merge of L<n-1> into R<n-1>
+#   L<n> and R<n> resolve the conflicts differently.
+#
+#   X<n> is an auto-generated merge-base used when merging L<n+1> and R<n+1>.
+#   By construction, X1 has conflict markers due to conflicting versions.
+#   X2, due to using merge.conflictstyle=3, has nested conflict markers.
+#
+#   So, merging R3 into L3 using merge.conflictstyle=3 should show the
+#   nested conflict markers from X2 in the base version -- that means we
+#   have three levels of conflict markers.  Can we distinguish all three?
+
+test_expect_success "setup virtual merge base with nested conflicts" '
+	test_create_repo virtual_merge_base_has_nested_conflicts &&
+	(
+		cd virtual_merge_base_has_nested_conflicts &&
+
+		# Create some related files now
+		for i in $(test_seq 1 10)
+		do
+			echo Random base content line $i
+		done >content &&
+
+		# Setup original commit
+		git add content &&
+		test_tick && git commit -m initial &&
+
+		git branch L &&
+		git branch R &&
+
+		# Create L1
+		git checkout L &&
+		echo left >>content &&
+		git add content &&
+		test_tick && git commit -m "version L1 of content" &&
+		git tag L1 &&
+
+		# Create R1
+		git checkout R &&
+		echo right >>content &&
+		git add content &&
+		test_tick && git commit -m "verson R1 of content" &&
+		git tag R1 &&
+
+		# Create L2
+		git checkout L &&
+		test_must_fail git -c merge.conflictstyle=diff3 merge R1 &&
+		git checkout L1 content &&
+		test_tick && git commit -m "version L2 of content" &&
+		git tag L2 &&
+
+		# Create R2
+		git checkout R &&
+		test_must_fail git -c merge.conflictstyle=diff3 merge L1 &&
+		git checkout R1 content &&
+		test_tick && git commit -m "version R2 of content" &&
+		git tag R2 &&
+
+		# Create L3
+		git checkout L &&
+		test_must_fail git -c merge.conflictstyle=diff3 merge R2 &&
+		git checkout L1 content &&
+		test_tick && git commit -m "version L3 of content" &&
+		git tag L3 &&
+
+		# Create R3
+		git checkout R &&
+		test_must_fail git -c merge.conflictstyle=diff3 merge L2 &&
+		git checkout R1 content &&
+		test_tick && git commit -m "version R3 of content" &&
+		git tag R3
+	)
+'
+
+test_expect_failure "check virtual merge base with nested conflicts" '
+	(
+		cd virtual_merge_base_has_nested_conflicts &&
+
+		git checkout L3^0 &&
+
+		# Merge must fail; there is a conflict
+		test_must_fail git -c merge.conflictstyle=diff3 merge -s recursive R3^0 &&
+
+		# Make sure the index has the right number of entries
+		git ls-files -s >out &&
+		test_line_count = 3 out &&
+		git ls-files -u >out &&
+		test_line_count = 3 out &&
+		# Ensure we have the correct number of untracked files
+		git ls-files -o >out &&
+		test_line_count = 1 out &&
+
+		# Compare :[23]:content to expected values
+		git rev-parse L1:content R1:content >expect &&
+		git rev-parse :2:content :3:content >actual &&
+		test_cmp expect actual &&
+
+		# Imitate X1 merge base, except without long enough conflict
+		# markers because a subsequent sed will modify them.  Put
+		# result into vmb.
+		git cat-file -p master:content >base &&
+		git cat-file -p L:content >left &&
+		git cat-file -p R:content >right &&
+		cp left merged-once &&
+		test_must_fail git merge-file --diff3 \
+			-L "Temporary merge branch 1" \
+			-L "merged common ancestors"  \
+			-L "Temporary merge branch 2" \
+			merged-once \
+			base        \
+			right       &&
+		sed -e "s/^\([<|=>]\)/\1\1\1/" merged-once >vmb &&
+
+		# Imitate X2 merge base, overwriting vmb.  Note that we
+		# extend both sets of conflict markers to make them longer
+		# with the sed command.
+		cp left merged-twice &&
+		test_must_fail git merge-file --diff3 \
+			-L "Temporary merge branch 1" \
+			-L "merged common ancestors"  \
+			-L "Temporary merge branch 2" \
+			merged-twice \
+			vmb          \
+			right        &&
+		sed -e "s/^\([<|=>]\)/\1\1\1/" merged-twice >vmb &&
+
+		# Compare :1:content to expected value
+		git cat-file -p :1:content >actual &&
+		test_cmp vmb actual &&
+
+		# Determine expected content in final outer merge, compare to
+		# what the merge generated.
+		cp -f left expect &&
+		test_must_fail git merge-file --diff3                      \
+			-L "HEAD"  -L "merged common ancestors"  -L "R3^0" \
+			expect     vmb                           right     &&
+		test_cmp expect content
+	)
+'
+
 test_done

-- 
2.19.0.235.g7c386e1068

Elijah Newren· Oct 12, 2018, 21:25 UTC · re: Elijah Newren · lore

[PATCH 2/4] merge-recursive: increase marker length with depth of recursion

When using merge.conflictstyle=diff3 to have the "base version" be shown in conflicts, there is the possibility that the base version itself has conflicts in it. This comes about when there are more than one merge base, and the merging of those merge bases produces a conflict. Since this process applies recursively, it is possible to have conflict markers nested at an arbitrary depth; to be able to differentiate the conflict markers from different nestings, we make them all of different lengths.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 ll-merge.c                        |  4 +++-
 ll-merge.h                        |  1 +
 merge-recursive.c                 | 22 +++++++++++++++-------
 t/t6036-recursive-corner-cases.sh |  2 +-
 4 files changed, 20 insertions(+), 9 deletions(-)

Show changes to 4 files +20 −9

ll-merge.c, ll-merge.h, merge-recursive.c, t/t6036-recursive-corner-cases.sh

diff --git a/ll-merge.c b/ll-merge.c
index 0e2800f7bb..aabc1b5c2e 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -384,7 +384,9 @@ int ll_merge(mmbuffer_t *result_buf,
 	if (opts->virtual_ancestor) {
 		if (driver->recursive)
 			driver = find_ll_merge_driver(driver->recursive);
-		marker_size += 2;
+	}
+	if (opts->extra_marker_size) {
+		marker_size += opts->extra_marker_size;
 	}
 	return driver->fn(driver, result_buf, path, ancestor, ancestor_label,
 			  ours, our_label, theirs, their_label,
diff --git a/ll-merge.h b/ll-merge.h
index b72b19921e..5b4e158502 100644
--- a/ll-merge.h
+++ b/ll-merge.h
@@ -11,6 +11,7 @@ struct ll_merge_options {
 	unsigned virtual_ancestor : 1;
 	unsigned variant : 2;	/* favor ours, favor theirs, or union merge */
 	unsigned renormalize : 1;
+	unsigned extra_marker_size;
 	long xdl_opts;
 };
 
diff --git a/merge-recursive.c b/merge-recursive.c
index 5206d6cfb6..2452788d28 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1039,7 +1039,8 @@ static int merge_3way(struct merge_options *o,
 		      const struct diff_filespec *a,
 		      const struct diff_filespec *b,
 		      const char *branch1,
-		      const char *branch2)
+		      const char *branch2,
+		      const int extra_marker_size)
 {
 	mmfile_t orig, src1, src2;
 	struct ll_merge_options ll_opts = {0};
@@ -1047,6 +1048,7 @@ static int merge_3way(struct merge_options *o,
 	int merge_status;
 
 	ll_opts.renormalize = o->renormalize;
+	ll_opts.extra_marker_size = extra_marker_size;
 	ll_opts.xdl_opts = o->xdl_opts;
 
 	if (o->call_depth) {
@@ -1281,6 +1283,7 @@ static int merge_mode_and_contents(struct merge_options *o,
 				   const char *filename,
 				   const char *branch1,
 				   const char *branch2,
+				   const int extra_marker_size,
 				   struct merge_file_info *result)
 {
 	result->merge = 0;
@@ -1321,7 +1324,8 @@ static int merge_mode_and_contents(struct merge_options *o,
 			int ret = 0, merge_status;
 
 			merge_status = merge_3way(o, &result_buf, one, a, b,
-						  branch1, branch2);
+						  branch1, branch2,
+						  extra_marker_size);
 
 			if ((merge_status < 0) || !result_buf.ptr)
 				ret = err(o, _("Failed to execute internal merge"));
@@ -1610,7 +1614,8 @@ static int handle_rename_rename_1to2(struct merge_options *o,
 		struct diff_filespec other;
 		struct diff_filespec *add;
 		if (merge_mode_and_contents(o, one, a, b, one->path,
-					    ci->branch1, ci->branch2, &mfi))
+					    ci->branch1, ci->branch2,
+					    o->call_depth * 2, &mfi))
 			return -1;
 
 		/*
@@ -1677,9 +1682,11 @@ static int handle_rename_rename_2to1(struct merge_options *o,
 	path_side_1_desc = xstrfmt("%s (was %s)", path, a->path);
 	path_side_2_desc = xstrfmt("%s (was %s)", path, b->path);
 	if (merge_mode_and_contents(o, a, c1, &ci->ren1_other, path_side_1_desc,
-				    o->branch1, o->branch2, &mfi_c1) ||
+				    o->branch1, o->branch2,
+				    o->call_depth * 2, &mfi_c1) ||
 	    merge_mode_and_contents(o, b, &ci->ren2_other, c2, path_side_2_desc,
-				    o->branch1, o->branch2, &mfi_c2))
+				    o->branch1, o->branch2,
+				    o->call_depth * 2, &mfi_c2))
 		return -1;
 	free(path_side_1_desc);
 	free(path_side_2_desc);
@@ -2725,7 +2732,7 @@ static int process_renames(struct merge_options *o,
 
 					if (merge_mode_and_contents(o, &one, &a, &b, ren1_dst,
 								    branch1, branch2,
-								    &mfi)) {
+								    o->call_depth * 2, &mfi)) {
 						clean_merge = -1;
 						goto cleanup_and_return;
 					}
@@ -3022,7 +3029,8 @@ static int handle_content_merge(struct merge_options *o,
 			df_conflict_remains = 1;
 	}
 	if (merge_mode_and_contents(o, &one, &a, &b, path,
-				    o->branch1, o->branch2, &mfi))
+				    o->branch1, o->branch2,
+				    o->call_depth * 2, &mfi))
 		return -1;
 
 	/*
diff --git a/t/t6036-recursive-corner-cases.sh b/t/t6036-recursive-corner-cases.sh
index 1d1135082c..21954db624 100755
--- a/t/t6036-recursive-corner-cases.sh
+++ b/t/t6036-recursive-corner-cases.sh
@@ -1487,7 +1487,7 @@ test_expect_success "setup virtual merge base with nested conflicts" '
 	)
 '
 
-test_expect_failure "check virtual merge base with nested conflicts" '
+test_expect_success "check virtual merge base with nested conflicts" '
 	(
 		cd virtual_merge_base_has_nested_conflicts &&

-- 
2.19.0.235.g7c386e1068

Junio C Hamano· Oct 15, 2018, 05:12 UTC · re: Elijah Newren · lore

Re: [PATCH 2/4] merge-recursive: increase marker length with depth of recursion

Elijah Newren <newren@gmail.com> writes:

Show 8 quoted lines

> When using merge.conflictstyle=diff3 to have the "base version" be shown
> in conflicts, there is the possibility that the base version itself has
> conflicts in it.  This comes about when there are more than one merge
> base, and the merging of those merge bases produces a conflict.
> Since this process applies recursively, it is possible to have conflict
> markers nested at an arbitrary depth; to be able to differentiate the
> conflict markers from different nestings, we make them all of different
> lengths.

I know it is possible that the common ancestor part that is enclosed by the outermost makers can have arbitrary conflicts, and they can be even recursive conflicts. But I fail to see why it is a problem. Perhaps that is because I am not particularly good at resolving merge conflicts, but as long as the common part of the outermost merge is identifyable, would that really matter? What I would do while looking at common ancestor part with conflicts (not even a recursive one) is just to ignore it, so...

Not that I strongly oppose to incrementing the marker length at every level. I do not think it would hurt, but I just do not see how it would help.

Elijah Newren· Oct 15, 2018, 15:02 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/4] merge-recursive: increase marker length with depth of recursion

On Sun, Oct 14, 2018 at 10:12 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 24 quoted lines

>
> Elijah Newren <newren@gmail.com> writes:
>
> > When using merge.conflictstyle=diff3 to have the "base version" be shown
> > in conflicts, there is the possibility that the base version itself has
> > conflicts in it.  This comes about when there are more than one merge
> > base, and the merging of those merge bases produces a conflict.
> > Since this process applies recursively, it is possible to have conflict
> > markers nested at an arbitrary depth; to be able to differentiate the
> > conflict markers from different nestings, we make them all of different
> > lengths.
>
> I know it is possible that the common ancestor part that is enclosed
> by the outermost makers can have arbitrary conflicts, and they can
> be even recursive conflicts.  But I fail to see why it is a problem.
> Perhaps that is because I am not particularly good at resolving
> merge conflicts, but as long as the common part of the outermost
> merge is identifyable, would that really matter?  What I would do
> while looking at common ancestor part with conflicts (not even a
> recursive one) is just to ignore it, so...
>
> Not that I strongly oppose to incrementing the marker length at
> every level.  I do not think it would hurt, but I just do not see
> how it would help.

Fair enough. The real motivation for these changes was the modification to rename/rename(2to1) conflicts (and rename/add conflicts) to behave like add/add conflicts -- that change means we can have nested conflict markers even without invoking the recursive part of the recursive machinery. So, I needed a way to increase the length of the conflict markers besides just checking opts->virtual_ancestor. Just using a fixed extra indent seemed problematic, because if I also had to worry about even one virtual_ancestor, then I was already dealing with the possibility of triply nested conflict markers and only one of them from a virtual merge base. See t6036 in https://public-inbox.org/git/20181014020537.17991-3-newren@gmail.com/.

However, that series was long enough, so to try to simplify review I split as much as I could out of it. That resulted, among other things, in me submitting this marker nesting change as part of this series using a more limited rationale.

Would you like me to edit the commit message to include this more difficult case?

Junio C Hamano· Oct 16, 2018, 02:16 UTC · re: Elijah Newren · lore

Re: [PATCH 2/4] merge-recursive: increase marker length with depth of recursion

Elijah Newren <newren@gmail.com> writes:

> Would you like me to edit the commit message to include this more
> difficult case?

Neither. If the "marker length" change is required in a separate series that will build on top of the current 4-patch series, I think dropping this step from the current series and make it a part of the series that deals with rename/rename would make more sense.

Elijah Newren· Oct 16, 2018, 18:00 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/4] merge-recursive: increase marker length with depth of recursion

On Mon, Oct 15, 2018 at 7:17 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 10 quoted lines

>
> Elijah Newren <newren@gmail.com> writes:
>
> > Would you like me to edit the commit message to include this more
> > difficult case?
>
> Neither.  If the "marker length" change is required in a separate
> series that will build on top of the current 4-patch series, I think
> dropping this step from the current series and make it a part of the
> series that deals with rename/rename would make more sense.

Okay, I'll resubmit this series without this patch or associated testcase, and include those in the later file collision series.

Elijah Newren· Oct 16, 2018, 20:19 UTC · re: Elijah Newren · lore

[PATCH v2 0/2] More merge cleanups

This series adds a few more cleanups on top of en/merge-cleanup.
Changes since v1:
  - Removed two patches that will instead be included in a follow-on
    series, as suggested by Junio.
  - Incorporated commit message cleanups (capitalization and indents)
    made by Junio to the previous round.

Elijah Newren (2):
  merge-recursive: improve auto-merging messages with path collisions
  merge-recursive: avoid showing conflicts with merge branch before HEAD

 merge-recursive.c                 | 37 ++++++++++++++++++++++++++++---
 t/t6036-recursive-corner-cases.sh |  8 +++----
 2 files changed, 38 insertions(+), 7 deletions(-)

-- 
2.19.1.280.g0c175526bf

Elijah Newren· Oct 16, 2018, 20:19 UTC · re: Elijah Newren · lore

[PATCH v2 1/2] merge-recursive: improve auto-merging messages with path collisions

  Auto-merging somefile.c (was somecase.c)
  Auto-merging somefile.c (was somefolder.c)
  Auto-merging somefile.c

  Auto-merging version of somefile.c from somecase.c
  Auto-merging version of somefile.c from someportfolio.c
  Auto-merging somefile.c

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Show changes to merge-recursive.c +2 −2

diff --git a/merge-recursive.c b/merge-recursive.c
index 5206d6cfb6..8a47e54e2f 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1674,8 +1674,8 @@ static int handle_rename_rename_2to1(struct merge_options *o,
 	remove_file(o, 1, a->path, o->call_depth || would_lose_untracked(a->path));
 	remove_file(o, 1, b->path, o->call_depth || would_lose_untracked(b->path));
 
-	path_side_1_desc = xstrfmt("%s (was %s)", path, a->path);
-	path_side_2_desc = xstrfmt("%s (was %s)", path, b->path);
+	path_side_1_desc = xstrfmt("version of %s from %s", path, a->path);
+	path_side_2_desc = xstrfmt("version of %s from %s", path, b->path);
 	if (merge_mode_and_contents(o, a, c1, &ci->ren1_other, path_side_1_desc,
 				    o->branch1, o->branch2, &mfi_c1) ||
 	    merge_mode_and_contents(o, b, &ci->ren2_other, c2, path_side_2_desc,

-- 
2.19.1.280.g0c175526bf

Elijah Newren· Oct 16, 2018, 20:19 UTC · re: Elijah Newren · lore

[PATCH v2 2/2] merge-recursive: avoid showing conflicts with merge branch before HEAD

We want to load unmerged entries from HEAD into the index at stage 2 and from MERGE_HEAD into stage 3. Similarly, folks expect merge conflicts to look like

    <<<<<<<< HEAD
    content from our side
    ========
    content from their side
    >>>>>>>> MERGE_HEAD

not

    <<<<<<<< MERGE_HEAD
    content from their side
    ========
    content from our side
    >>>>>>>> HEAD

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c                 | 33 ++++++++++++++++++++++++++++++-
 t/t6036-recursive-corner-cases.sh |  8 ++++----
 2 files changed, 36 insertions(+), 5 deletions(-)

Show changes to 2 files +36 −5

merge-recursive.c, t/t6036-recursive-corner-cases.sh

diff --git a/merge-recursive.c b/merge-recursive.c
index 8a47e54e2f..16980db7f9 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -228,7 +228,26 @@ static inline void setup_rename_conflict_info(enum rename_type rename_type,
 					      struct stage_data *src_entry1,
 					      struct stage_data *src_entry2)
 {
-	struct rename_conflict_info *ci = xcalloc(1, sizeof(struct rename_conflict_info));
+	struct rename_conflict_info *ci;
+
+	/*
+	 * When we have two renames involved, it's easiest to get the
+	 * correct things into stage 2 and 3, and to make sure that the
+	 * content merge puts HEAD before the other branch if we just
+	 * ensure that branch1 == o->branch1.  So, simply flip arguments
+	 * around if we don't have that.
+	 */
+	if (dst_entry2 && branch1 != o->branch1) {
+		setup_rename_conflict_info(rename_type,
+					   pair2,      pair1,
+					   branch2,    branch1,
+					   dst_entry2, dst_entry1,
+					   o,
+					   src_entry2, src_entry1);
+		return;
+	}
+
+	ci = xcalloc(1, sizeof(struct rename_conflict_info));
 	ci->rename_type = rename_type;
 	ci->pair1 = pair1;
 	ci->branch1 = branch1;
@@ -1283,6 +1302,18 @@ static int merge_mode_and_contents(struct merge_options *o,
 				   const char *branch2,
 				   struct merge_file_info *result)
 {
+	if (o->branch1 != branch1) {
+		/*
+		 * It's weird getting a reverse merge with HEAD on the bottom
+		 * side of the conflict markers and the other branch on the
+		 * top.  Fix that.
+		 */
+		return merge_mode_and_contents(o, one, b, a,
+					       filename,
+					       branch2, branch1,
+					       extra_marker_size, result);
+	}
+
 	result->merge = 0;
 	result->clean = 1;
 
diff --git a/t/t6036-recursive-corner-cases.sh b/t/t6036-recursive-corner-cases.sh
index 59e52c5a09..e1cef58f2a 100755
--- a/t/t6036-recursive-corner-cases.sh
+++ b/t/t6036-recursive-corner-cases.sh
@@ -230,13 +230,13 @@ test_expect_success 'git detects differently handled merges conflict' '
 			:2:new_a :3:new_a &&
 		test_cmp expect actual &&
 
-		git cat-file -p B:new_a >ours &&
-		git cat-file -p C:new_a >theirs &&
+		git cat-file -p C:new_a >ours &&
+		git cat-file -p B:new_a >theirs &&
 		>empty &&
 		test_must_fail git merge-file \
-			-L "Temporary merge branch 2" \
-			-L "" \
 			-L "Temporary merge branch 1" \
+			-L "" \
+			-L "Temporary merge branch 2" \
 			ours empty theirs &&
 		sed -e "s/^\([<=>]\)/\1\1\1/" ours >expect &&
 		git cat-file -p :1:new_a >actual &&

-- 
2.19.1.280.g0c175526bf

Junio C Hamano· Oct 18, 2018, 06:09 UTC · re: Elijah Newren · lore

Re: [PATCH v2 2/2] merge-recursive: avoid showing conflicts with merge branch before HEAD

Elijah Newren <newren@gmail.com> writes:

Show 15 quoted lines

> @@ -1283,6 +1302,18 @@ static int merge_mode_and_contents(struct merge_options *o,
>  				   const char *branch2,
>  				   struct merge_file_info *result)
>  {
> +	if (o->branch1 != branch1) {
> +		/*
> +		 * It's weird getting a reverse merge with HEAD on the bottom
> +		 * side of the conflict markers and the other branch on the
> +		 * top.  Fix that.
> +		 */
> +		return merge_mode_and_contents(o, one, b, a,
> +					       filename,
> +					       branch2, branch1,
> +					       extra_marker_size, result);
> +	}

Will queue with the following squashed in.

Thanks.

 merge-recursive.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Show changes to merge-recursive.c +1 −2

diff --git a/merge-recursive.c b/merge-recursive.c
index 16980db7f9..73b5710386 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1310,8 +1310,7 @@ static int merge_mode_and_contents(struct merge_options *o,
 		 */
 		return merge_mode_and_contents(o, one, b, a,
 					       filename,
-					       branch2, branch1,
-					       extra_marker_size, result);
+					       branch2, branch1, result);
 	}
 
 	result->merge = 0;