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

Message ID 20181012212551.7689-3-newren@gmail.com
State New
Headers show
Series
  • More merge cleanups
Related show

Commit Message

Elijah Newren Oct. 12, 2018, 9:25 p.m. UTC
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(-)

Comments

Junio C Hamano Oct. 15, 2018, 5:12 a.m. UTC | #1
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.
Elijah Newren Oct. 15, 2018, 3:02 p.m. UTC | #2
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?
Junio C Hamano Oct. 16, 2018, 2:16 a.m. UTC | #3
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, 6 p.m. UTC | #4
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
diff mbox series

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