Improving diff3 conflict quality -- reducing the prevalence of nested conflicts with recursive merges

builtin/merge-file.c

@@ -73,6 +73,8 @@ int cmd_merge_file(int argc,
 		OPT_SET_INT(0, "diff3", &xmp.style, N_("use a diff3 based merge"), XDL_MERGE_DIFF3),
 		OPT_SET_INT(0, "zdiff3", &xmp.style, N_("use a zealous diff3 based merge"),
 				XDL_MERGE_ZEALOUS_DIFF3),
+		OPT_SET_INT(0, "base", &xmp.favor, N_("for conflicts, use base version"),
+			    XDL_MERGE_FAVOR_BASE),
 		OPT_SET_INT(0, "ours", &xmp.favor, N_("for conflicts, use our version"),
 			    XDL_MERGE_FAVOR_OURS),
 		OPT_SET_INT(0, "theirs", &xmp.favor, N_("for conflicts, use their version"),

merge-ll.h

@@ -61,9 +61,9 @@ struct ll_merge_options {
 	 * Resolve local conflicts automatically in favor of one side or the other
 	 * (as in 'git merge-file' `--ours`/`--theirs`/`--union`).  Can be `0`,
 	 * `XDL_MERGE_FAVOR_OURS`, `XDL_MERGE_FAVOR_THEIRS`,
-	 * or `XDL_MERGE_FAVOR_UNION`.
+	 * or `XDL_MERGE_FAVOR_UNION`, `XDL_MERGE_FAVOR_BASE`.
 	 */
-	unsigned variant : 2;
+	unsigned variant : 3;
 
 	/**
 	 * Resmudge and clean the "base", "theirs" and "ours" files before merging.

merge-ort.c

@@ -401,6 +401,9 @@ struct merge_options_internal {
 	/* call_depth: recursion level counter for merging merge bases */
 	int call_depth;
 
+	/* vmb_favor: preferred resolution variant for virtual merge base */
+	unsigned vmb_favor;
+
 	/* field that holds submodule conflict information */
 	struct string_list conflicted_submodules;
 };
@@ -2070,7 +2073,7 @@ static int merge_3way(struct merge_options *opt,
 
 	if (opt->priv->call_depth) {
 		ll_opts.virtual_ancestor = 1;
-		ll_opts.variant = 0;
+		ll_opts.variant = opt->priv->vmb_favor;
 	} else {
 		switch (opt->recursive_variant) {
 		case MERGE_VARIANT_OURS:
@@ -5186,6 +5189,17 @@ static void merge_ort_internal(struct merge_options *opt,
 		ancestor_name = "empty tree";
 	} else if (merge_bases) {
 		ancestor_name = "merged common ancestors";
+		/*
+		 * If there were more than two virtual merge bases, we just
+		 * fall back to our virtual merge base having conflict markers
+		 * between versions.  But if there are only two, then we can
+		 * resolve conflicts by taking the version from the merge
+		 * base of our merge bases.
+		 */
+		if (merge_bases->next)
+			opt->priv->vmb_favor = 0;
+		else
+			opt->priv->vmb_favor = XDL_MERGE_FAVOR_BASE;
 	} else {
 		strbuf_add_unique_abbrev(&merge_base_abbrev,
 					 &merged_merge_bases->object.oid,

t/t6416-recursive-corner-cases.sh

@@ -213,13 +213,12 @@ test_expect_success 'git detects differently handled merges conflict' '
 		git cat-file -p C:new_a >ours &&
 		git cat-file -p C:a >theirs &&
 		>empty &&
-		test_must_fail git merge-file \
+		git merge-file --base \
 			-L "Temporary merge branch 1" \
 			-L "" \
 			-L "Temporary merge branch 2" \
 			ours empty theirs &&
-		sed -e "s/^\([<=>]\)/\1\1\1/" ours >ours-tweaked &&
-		git hash-object ours-tweaked >expect &&
+		git hash-object ours >expect &&
 		git rev-parse >>expect      \
 				  D:new_a  E:new_a &&
 		git rev-parse   >actual     \
@@ -275,13 +274,12 @@ test_expect_success 'git detects differently handled merges conflict, swapped' '
 		git cat-file -p C:a >ours &&
 		git cat-file -p C:new_a >theirs &&
 		>empty &&
-		test_must_fail git merge-file \
+		git merge-file --base \
 			-L "Temporary merge branch 1" \
 			-L "" \
 			-L "Temporary merge branch 2" \
 			ours empty theirs &&
-		sed -e "s/^\([<=>]\)/\1\1\1/" ours >ours-tweaked &&
-		git hash-object ours-tweaked >expect &&
+		git hash-object ours >expect &&
 		git rev-parse >>expect      \
 				  D:new_a  E:new_a &&
 		git rev-parse   >actual     \
@@ -1564,11 +1562,13 @@ test_expect_failure 'check conflicting modes for regular file' '
 #     - R2 renames 'a' to 'm'
 #
 #   In the end, in file 'm' we have four different conflicting files (from
-#   two versions of 'b' and two of 'a').  In addition, if
-#   merge.conflictstyle is diff3, then the base version also has
-#   conflict markers of its own, leading to a total of three levels of
-#   conflict markers.  This is a pretty weird corner case, but we just want
-#   to ensure that we handle it as well as practical.
+#   two versions of 'b' and two of 'a').  In addition, if we didn't use
+#   XDL_MERGE_FAVOR_BASE and merge.conflictstyle is diff3, then the base
+#   version would also have conflict markers of its own, leading to a total of
+#   three levels of conflict markers.  However, we do use XDL_MERGE_FAVOR_BASE
+#   for recursive merges, so this should keep conflict nestings to two.  This
+#   is a pretty weird corner case, but we just want to ensure that we handle
+#   it as well as practical.
 
 test_expect_success 'setup nested conflicts' '
 	git init nested_conflicts &&
@@ -1646,7 +1646,7 @@ test_expect_success 'setup nested conflicts' '
 	)
 '
 
-test_expect_success 'check nested conflicts' '
+test_expect_success 'check nested merges without nested conflicts' '
 	(
 		cd nested_conflicts &&
 
@@ -1670,26 +1670,28 @@ test_expect_success 'check nested conflicts' '
 		git cat-file -p main:a >base &&
 		git cat-file -p L1:a >ours &&
 		git cat-file -p R1:a >theirs &&
-		test_must_fail git merge-file --diff3 \
+		git merge-file --base \
 			-L "Temporary merge branch 1" \
 			-L "$MAIN"  \
 			-L "Temporary merge branch 2" \
 			ours  \
 			base  \
 			theirs &&
-		sed -e "s/^\([<|=>]\)/\1\1/" ours >vmb_a &&
+		mv ours vmb_a &&
+		test_cmp base vmb_a &&
 
 		git cat-file -p main:b >base &&
 		git cat-file -p L1:b >ours &&
 		git cat-file -p R1:b >theirs &&
-		test_must_fail git merge-file --diff3 \
+		git merge-file --base \
 			-L "Temporary merge branch 1" \
 			-L "$MAIN"  \
 			-L "Temporary merge branch 2" \
 			ours  \
 			base  \
 			theirs &&
-		sed -e "s/^\([<|=>]\)/\1\1/" ours >vmb_b &&
+		mv ours vmb_b &&
+		test_cmp base vmb_b &&
 
 		# Compare :2:m to expected values
 		git cat-file -p L2:m >ours &&
@@ -1749,14 +1751,15 @@ test_expect_success 'check nested conflicts' '
 #   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.
+#   By construction, X1 has two handle conflicting versions.   X2 is both
+#   based on a merge base that had to ahndle conflicting versions, and is
+#   trying to resolve conflicting versions between L2 and R2.
 #
-#   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?
+#   So, merging R3 into L3 using merge.conflictstyle=3 has a merge base where
+#   conflicts had to dealth with multiple levels back.  Do we get a reasonable
+#   diff3 output for it?
 
-test_expect_success 'setup virtual merge base with nested conflicts' '
+test_expect_success 'setup virtual merge base where multiple levels back had conflicts' '
 	git init virtual_merge_base_has_nested_conflicts &&
 	(
 		cd virtual_merge_base_has_nested_conflicts &&
@@ -1815,7 +1818,7 @@ test_expect_success 'setup virtual merge base with nested conflicts' '
 	)
 '
 
-test_expect_success 'check virtual merge base with nested conflicts' '
+test_expect_success 'check virtual merge base where multiple levels back had conflicts' '
 	(
 		cd virtual_merge_base_has_nested_conflicts &&
 
@@ -1839,34 +1842,30 @@ test_expect_success 'check virtual merge base with nested conflicts' '
 		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.
+		# Imitate X1 merge base, Put result into vmb.
 		git cat-file -p main: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 \
+		git merge-file --base \
 			-L "Temporary merge branch 1" \
 			-L "$MAIN"  \
 			-L "Temporary merge branch 2" \
 			merged-once \
 			base        \
 			right       &&
-		sed -e "s/^\([<|=>]\)/\1\1\1/" merged-once >vmb &&
+		mv 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.
+		# Imitate X2 merge base, overwriting vmb.
 		cp left merged-twice &&
-		test_must_fail git merge-file --diff3 \
+		git merge-file --base \
 			-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 &&
+		mv merged-twice vmb &&
 
 		# Compare :1:content to expected value
 		git cat-file -p :1:content >actual &&

xdiff/xdiff.h

@@ -63,6 +63,7 @@ extern "C" {
 #define XDL_MERGE_FAVOR_OURS 1
 #define XDL_MERGE_FAVOR_THEIRS 2
 #define XDL_MERGE_FAVOR_UNION 3
+#define XDL_MERGE_FAVOR_BASE 4
 
 /* merge output styles */
 #define XDL_MERGE_DIFF3 1

xdiff/xmerge.c

@@ -26,9 +26,10 @@ typedef struct s_xdmerge {
 	struct s_xdmerge *next;
 	/*
 	 * 0 = conflict,
-	 * 1 = no conflict, take first,
+	 * 1 = no conflict, take first.
 	 * 2 = no conflict, take second.
-	 * 3 = no conflict, take both.
+	 * 3 = no conflict, take both first & second.
+	 * 4 = no conflict, take base.
 	 */
 	int mode;
 	/*
@@ -313,6 +314,13 @@ static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1,
 			if (m->mode & 2)
 				size += xdl_recs_copy(xe2, m->i2, m->chg2, 0, 0,
 						      dest ? dest + size : NULL);
+		} else if (m->mode == XDL_MERGE_FAVOR_BASE) {
+			/* Before conflicting part */
+			size += xdl_recs_copy(xe1, i, m->i1 - i, 0, 0,
+					      dest ? dest + size : NULL);
+			/* Image from merge base */
+			size += xdl_orig_copy(xe1, m->i0, m->chg0, 0, 0,
+					      dest ? dest + size : NULL);
 		} else
 			continue;
 		i = m->i1 + m->chg1;