[PATCH 0/4] More merge cleanups

[PATCH 0/4] More merge cleanups

[Elijah Newren]

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

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

[Elijah Newren]

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

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

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

[Elijah Newren]

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(-)

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

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

[Elijah Newren]

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(-)

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

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

[Elijah Newren]

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(-)

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

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

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

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

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

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

> @@ -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;
> +	}

;-)

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

[Elijah Newren]

On Sun, Oct 14, 2018 at 10:12 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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?

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

[Junio C Hamano]

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.

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

[Elijah Newren]

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

[PATCH v2 0/2] More merge cleanups

[Elijah Newren]

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

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

[Elijah Newren]

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(-)

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

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

[Elijah Newren]

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(-)

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

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

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> @@ -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(-)

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;