[PATCH 1/2] xdl_fill_merge_buffer(): separate out a too deeply nested function

[PATCH 1/2] xdl_fill_merge_buffer(): separate out a too deeply nested function

[Junio C Hamano]

This simply moves code around to make a separate function that prepares
a single conflicted hunk with markers into the buffer.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * I am sending this out because I need an extra set of eyeballs and a bit
   of insight into this code from you for the second part, which is still
   an early RFC/WIP, and it depends on this refactoring.

 xdiff/xmerge.c |  121 ++++++++++++++++++++++++++++++++-----------------------
 1 files changed, 70 insertions(+), 51 deletions(-)

diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
index 82b3573..6ffaa4f 100644
--- a/xdiff/xmerge.c
+++ b/xdiff/xmerge.c
@@ -113,65 +113,84 @@ static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)
 	return size;
 }
 
-static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1,
-		xdfenv_t *xe2, const char *name2, xdmerge_t *m, char *dest)
+static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
+			      xdfenv_t *xe2, const char *name2,
+			      int size, int i,
+			      xdmerge_t *m, char *dest)
 {
 	const int marker_size = 7;
 	int marker1_size = (name1 ? strlen(name1) + 1 : 0);
 	int marker2_size = (name2 ? strlen(name2) + 1 : 0);
-	int conflict_marker_size = 3 * (marker_size + 1)
-		+ marker1_size + marker2_size;
-	int size, i1, j;
-
-	for (size = i1 = 0; m; m = m->next) {
-		if (m->mode == 0) {
-			size += xdl_recs_copy(xe1, i1, m->i1 - i1, 0,
-					dest ? dest + size : NULL);
-			if (dest) {
-				for (j = 0; j < marker_size; j++)
-					dest[size++] = '<';
-				if (marker1_size) {
-					dest[size] = ' ';
-					memcpy(dest + size + 1, name1,
-							marker1_size - 1);
-					size += marker1_size;
-				}
-				dest[size++] = '\n';
-			} else
-				size += conflict_marker_size;
-			size += xdl_recs_copy(xe1, m->i1, m->chg1, 1,
-					dest ? dest + size : NULL);
-			if (dest) {
-				for (j = 0; j < marker_size; j++)
-					dest[size++] = '=';
-				dest[size++] = '\n';
-			}
-			size += xdl_recs_copy(xe2, m->i2, m->chg2, 1,
-					dest ? dest + size : NULL);
-			if (dest) {
-				for (j = 0; j < marker_size; j++)
-					dest[size++] = '>';
-				if (marker2_size) {
-					dest[size] = ' ';
-					memcpy(dest + size + 1, name2,
-							marker2_size - 1);
-					size += marker2_size;
-				}
-				dest[size++] = '\n';
-			}
-		} else if (m->mode == 1)
-			size += xdl_recs_copy(xe1, i1, m->i1 + m->chg1 - i1, 0,
-					dest ? dest + size : NULL);
+	int j;
+
+	/* Before conflicting part */
+	size += xdl_recs_copy(xe1, i, m->i1 - i, 0,
+			      dest ? dest + size : NULL);
+
+	if (!dest) {
+		size += marker_size + 1 + marker1_size;
+	} else {
+		for (j = 0; j < marker_size; j++)
+			dest[size++] = '<';
+		if (marker1_size) {
+			dest[size] = ' ';
+			memcpy(dest + size + 1, name1, marker1_size - 1);
+			size += marker1_size;
+		}
+		dest[size++] = '\n';
+	}
+
+	/* Postimage from side #1 */
+	size += xdl_recs_copy(xe1, m->i1, m->chg1, 1,
+			      dest ? dest + size : NULL);
+	if (!dest) {
+		size += marker_size + 1;
+	} else {
+		for (j = 0; j < marker_size; j++)
+			dest[size++] = '=';
+		dest[size++] = '\n';
+	}
+
+	/* Postimage from side #2 */
+	size += xdl_recs_copy(xe2, m->i2, m->chg2, 1,
+			      dest ? dest + size : NULL);
+	if (!dest) {
+		size += marker_size + 1 + marker2_size;
+	} else {
+		for (j = 0; j < marker_size; j++)
+			dest[size++] = '>';
+		if (marker2_size) {
+			dest[size] = ' ';
+			memcpy(dest + size + 1, name2, marker2_size - 1);
+			size += marker2_size;
+		}
+		dest[size++] = '\n';
+	}
+	return size;
+}
+
+static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1,
+		xdfenv_t *xe2, const char *name2, xdmerge_t *m, char *dest)
+{
+	int size, i;
+
+	for (size = i = 0; m; m = m->next) {
+		if (m->mode == 0)
+			size = fill_conflict_hunk(xe1, name1, xe2, name2,
+						  size, i, m, dest);
+		else if (m->mode == 1)
+			size += xdl_recs_copy(xe1, i, m->i1 + m->chg1 - i, 0,
+					      dest ? dest + size : NULL);
 		else if (m->mode == 2)
-			size += xdl_recs_copy(xe2, m->i2 - m->i1 + i1,
-					m->i1 + m->chg2 - i1, 0,
-					dest ? dest + size : NULL);
+			size += xdl_recs_copy(xe2, m->i2 - m->i1 + i,
+					      m->i1 + m->chg2 - i, 0,
+					      dest ? dest + size : NULL);
 		else
 			continue;
-		i1 = m->i1 + m->chg1;
+		i = m->i1 + m->chg1;
 	}
-	size += xdl_recs_copy(xe1, i1, xe1->xdf2.nrec - i1, 0,
-			dest ? dest + size : NULL);
+	size += xdl_recs_copy(xe1, i, xe1->xdf2.nrec - i, 0,
+			      dest ? dest + size : NULL);
 	return size;
 }
 
-- 
1.6.0.1.200.ge682f

[PATCH 2/2 - RFH/WIP] xdiff-merge: optionally show conflicts in "diff3 -m" style

[Junio C Hamano]

When showing conflicting merges, we traditionally followed RCS's merge
output format.  The output shows:

 <<<<<<<
 postimage from one side;
 =======
 postimage of the other side; and
 >>>>>>>

Some poeple find it easier to be able to understand what is going on when
they can view the common ancestor's version, which is used by "diff3 -m",
which shows:

 <<<<<<<
 postimage from one side;
 |||||||
 shared preimage;
 =======
 postimage of the other side; and
 >>>>>>>

This is an initial step to bring that as an optional feature to git.
Only "git merge-file" has been converted, with "--diff3" option.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This is RFH/WIP partly because I tested it only on simplest cases, but
   more importantly I suspect this should be orthogonal to the refinement
   level parameter.

   The idea is to keep (just like <i1,chg1>, <i2,chg2> you use to point at
   the postimage of both sides) <i0,chg0> in xdmerge_t that point at the
   corresponding part of the shared preimage.  I think I got the initial
   computation of <i0,chg0>, coalescing done in xdl_append_merge(), and
   use of that in xdl_fill_merge_buffer() right, but I couldn't figure out
   how to adjust them inside the merge refinement logic.

   Of course the eventual goal is to add this to merge-file.c, give a new
   configuration variable to allow people to use this, while teaching
   rerere to ignore the new '|||||||' + shared preimage section, so that
   existing rerere information can be reused for new conflicts in
   diff3 style output.

 builtin-merge-file.c |    6 ++-
 xdiff/xdiff.h        |    5 +-
 xdiff/xmerge.c       |  145 ++++++++++++++++++++++++++++++++++++++------------
 3 files changed, 119 insertions(+), 37 deletions(-)

diff --git a/builtin-merge-file.c b/builtin-merge-file.c
index 3605960..2f69aa1 100644
--- a/builtin-merge-file.c
+++ b/builtin-merge-file.c
@@ -13,6 +13,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 	mmbuffer_t result = {NULL, 0};
 	xpparam_t xpp = {XDF_NEED_MINIMAL};
 	int ret = 0, i = 0, to_stdout = 0;
+	int level = XDL_MERGE_ZEALOUS_ALNUM;
 
 	while (argc > 4) {
 		if (!strcmp(argv[1], "-L") && i < 3) {
@@ -25,6 +26,9 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 		else if (!strcmp(argv[1], "-q") ||
 				!strcmp(argv[1], "--quiet"))
 			freopen("/dev/null", "w", stderr);
+		else if (!strcmp(argv[1], "-3") ||
+			 !strcmp(argv[1], "--diff3"))
+			level = XDL_MERGE_DIFF3;
 		else
 			usage(merge_file_usage);
 		argc--;
@@ -46,7 +50,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 	}
 
 	ret = xdl_merge(mmfs + 1, mmfs + 0, names[0], mmfs + 2, names[2],
-			&xpp, XDL_MERGE_ZEALOUS_ALNUM, &result);
+			&xpp, level, &result);
 
 	for (i = 0; i < 3; i++)
 		free(mmfs[i].ptr);
diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index 413082e..a7b6e08 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -52,8 +52,9 @@ extern "C" {
 
 #define XDL_MERGE_MINIMAL 0
 #define XDL_MERGE_EAGER 1
-#define XDL_MERGE_ZEALOUS 2
-#define XDL_MERGE_ZEALOUS_ALNUM 3
+#define XDL_MERGE_DIFF3 2
+#define XDL_MERGE_ZEALOUS 10
+#define XDL_MERGE_ZEALOUS_ALNUM 11
 
 typedef struct s_mmfile {
 	char *ptr;
diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
index 6ffaa4f..ac6cc40 100644
--- a/xdiff/xmerge.c
+++ b/xdiff/xmerge.c
@@ -30,17 +30,32 @@ typedef struct s_xdmerge {
 	 * 2 = no conflict, take second.
 	 */
 	int mode;
+	/*
+	 * These point at the respective postimages.  E.g. <i1,chg1> is
+	 * how side #1 wants to change the common ancestor; if there is no
+	 * overlap, lines before i1 in the postimage of side #1 appear
+	 * in the merge result as a region touched by neither side.
+	 */
 	long i1, i2;
 	long chg1, chg2;
+	/*
+	 * These point at the preimage; of course there is just one
+	 * preimage, that is from the shared common ancestor.
+	 */
+	long i0;
+	long chg0;
 } xdmerge_t;
 
 static int xdl_append_merge(xdmerge_t **merge, int mode,
-		long i1, long chg1, long i2, long chg2)
+			    long i0, long chg0,
+			    long i1, long chg1,
+			    long i2, long chg2)
 {
 	xdmerge_t *m = *merge;
 	if (m && (i1 <= m->i1 + m->chg1 || i2 <= m->i2 + m->chg2)) {
 		if (mode != m->mode)
 			m->mode = 0;
+		m->chg0 = i0 + chg0 - m->i0;
 		m->chg1 = i1 + chg1 - m->i1;
 		m->chg2 = i2 + chg2 - m->i2;
 	} else {
@@ -49,6 +64,8 @@ static int xdl_append_merge(xdmerge_t **merge, int mode,
 			return -1;
 		m->next = NULL;
 		m->mode = mode;
+		m->i0 = i0;
+		m->chg0 = chg0;
 		m->i1 = i1;
 		m->chg1 = chg1;
 		m->i2 = i2;
@@ -91,11 +108,13 @@ static int xdl_merge_cmp_lines(xdfenv_t *xe1, int i1, xdfenv_t *xe2, int i2,
 	return 0;
 }
 
-static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)
+static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, int i, int count, int add_nl, char *dest)
 {
-	xrecord_t **recs = xe->xdf2.recs + i;
+	xrecord_t **recs;
 	int size = 0;
 
+	recs = (use_orig ? xe->xdf1.recs : xe->xdf2.recs) + i;
+
 	if (count < 1)
 		return 0;
 
@@ -113,9 +132,19 @@ static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)
 	return size;
 }
 
+static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)
+{
+	return xdl_recs_copy_0(0, xe, i, count, add_nl, dest);
+}
+
+static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)
+{
+	return xdl_recs_copy_0(1, xe, i, count, add_nl, dest);
+}
+
 static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 			      xdfenv_t *xe2, const char *name2,
-			      int size, int i,
+			      int size, int i, int level,
 			      xdmerge_t *m, char *dest)
 {
 	const int marker_size = 7;
@@ -143,6 +172,20 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 	/* Postimage from side #1 */
 	size += xdl_recs_copy(xe1, m->i1, m->chg1, 1,
 			      dest ? dest + size : NULL);
+
+	if (level == XDL_MERGE_DIFF3) {
+		/* Shared preimage */
+		if (!dest) {
+			size += marker_size + 1;
+		} else {
+			for (j = 0; j < marker_size; j++)
+				dest[size++] = '|';
+			dest[size++] = '\n';
+		}
+		size += xdl_orig_copy(xe1, m->i0, m->chg0, 1,
+				      dest ? dest + size : NULL);
+	}
+
 	if (!dest) {
 		size += marker_size + 1;
 	} else {
@@ -170,14 +213,15 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 }
 
 static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1,
-		xdfenv_t *xe2, const char *name2, xdmerge_t *m, char *dest)
+				 xdfenv_t *xe2, const char *name2,
+				 xdmerge_t *m, char *dest, int level)
 {
 	int size, i;
 
 	for (size = i = 0; m; m = m->next) {
 		if (m->mode == 0)
 			size = fill_conflict_hunk(xe1, name1, xe2, name2,
-						  size, i, m, dest);
+						  size, i, level, m, dest);
 		else if (m->mode == 1)
 			size += xdl_recs_copy(xe1, i, m->i1 + m->chg1 - i, 0,
 					      dest ? dest + size : NULL);
@@ -332,19 +376,31 @@ static int xdl_simplify_non_conflicts(xdfenv_t *xe1, xdmerge_t *m,
 }
 
 /*
- * level == 0: mark all overlapping changes as conflict
- * level == 1: mark overlapping changes as conflict only if not identical
- * level == 2: analyze non-identical changes for minimal conflict set
- * level == 3: analyze non-identical changes for minimal conflict set, but
- *             treat hunks not containing any letter or number as conflicting
+ * "Level" parameter can be:
+ *
+ * (MINIMAL):
+ * Mark all overlapping changes as conflict
+ *
+ * (EAGER):
+ * Mark overlapping changes as conflict only if not identical
+ *
+ * (DIFF3):
+ * Same as EAGER but show the shared preimage in the output as well
+ *
+ * (ZEALOUS):
+ * Analyze non-identical changes for minimal conflict set
+ *
+ * (ZEALOUS_ALNUM):
+ * Analyze non-identical changes for minimal conflict set, but
+ * treat hunks not containing any letter or number as conflicting
  *
  * returns < 0 on error, == 0 for no conflicts, else number of conflicts
  */
 static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1,
-		xdfenv_t *xe2, xdchange_t *xscr2, const char *name2,
-		int level, xpparam_t const *xpp, mmbuffer_t *result) {
+			xdfenv_t *xe2, xdchange_t *xscr2, const char *name2,
+			int level, xpparam_t const *xpp, mmbuffer_t *result) {
 	xdmerge_t *changes, *c;
-	int i1, i2, chg1, chg2;
+	int i0, i1, i2, chg0, chg1, chg2;
 
 	c = changes = NULL;
 
@@ -352,11 +408,14 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1,
 		if (!changes)
 			changes = c;
 		if (xscr1->i1 + xscr1->chg1 < xscr2->i1) {
+			i0 = xscr1->i1;
 			i1 = xscr1->i2;
 			i2 = xscr2->i2 - xscr2->i1 + xscr1->i1;
+			chg0 = xscr1->chg1;
 			chg1 = xscr1->chg2;
 			chg2 = xscr1->chg1;
-			if (xdl_append_merge(&c, 1, i1, chg1, i2, chg2)) {
+			if (xdl_append_merge(&c, 1,
+					     i0, chg0, i1, chg1, i2, chg2)) {
 				xdl_cleanup_merge(changes);
 				return -1;
 			}
@@ -364,40 +423,50 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1,
 			continue;
 		}
 		if (xscr2->i1 + xscr2->chg1 < xscr1->i1) {
+			i0 = xscr2->i1;
 			i1 = xscr1->i2 - xscr1->i1 + xscr2->i1;
 			i2 = xscr2->i2;
+			chg0 = xscr2->chg1;
 			chg1 = xscr2->chg1;
 			chg2 = xscr2->chg2;
-			if (xdl_append_merge(&c, 2, i1, chg1, i2, chg2)) {
+			if (xdl_append_merge(&c, 2,
+					     i0, chg0, i1, chg1, i2, chg2)) {
 				xdl_cleanup_merge(changes);
 				return -1;
 			}
 			xscr2 = xscr2->next;
 			continue;
 		}
-		if (level < 1 || xscr1->i1 != xscr2->i1 ||
-				xscr1->chg1 != xscr2->chg1 ||
-				xscr1->chg2 != xscr2->chg2 ||
-				xdl_merge_cmp_lines(xe1, xscr1->i2,
+		if (level == XDL_MERGE_MINIMAL ||
+		    (xscr1->i1 != xscr2->i1 ||
+		     xscr1->chg1 != xscr2->chg1 ||
+		     xscr1->chg2 != xscr2->chg2) ||
+		    xdl_merge_cmp_lines(xe1, xscr1->i2,
 					xe2, xscr2->i2,
 					xscr1->chg2, xpp->flags)) {
 			/* conflict */
 			int off = xscr1->i1 - xscr2->i1;
 			int ffo = off + xscr1->chg1 - xscr2->chg1;
 
+			i0 = xscr1->i1;
 			i1 = xscr1->i2;
 			i2 = xscr2->i2;
-			if (off > 0)
+			if (off > 0) {
+				i0 -= off;
 				i1 -= off;
+			}
 			else
 				i2 += off;
+			chg0 = xscr1->i1 + xscr1->chg1 - i0;
 			chg1 = xscr1->i2 + xscr1->chg2 - i1;
 			chg2 = xscr2->i2 + xscr2->chg2 - i2;
-			if (ffo > 0)
-				chg2 += ffo;
-			else
+			if (ffo < 0) {
+				chg0 -= ffo;
 				chg1 -= ffo;
-			if (xdl_append_merge(&c, 0, i1, chg1, i2, chg2)) {
+			} else
+				chg2 += ffo;
+			if (xdl_append_merge(&c, 0,
+					     i0, chg0, i1, chg1, i2, chg2)) {
 				xdl_cleanup_merge(changes);
 				return -1;
 			}
@@ -414,11 +483,14 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1,
 	while (xscr1) {
 		if (!changes)
 			changes = c;
+		i0 = xscr1->i1;
 		i1 = xscr1->i2;
 		i2 = xscr1->i1 + xe2->xdf2.nrec - xe2->xdf1.nrec;
+		chg0 = xscr1->chg1;
 		chg1 = xscr1->chg2;
 		chg2 = xscr1->chg1;
-		if (xdl_append_merge(&c, 1, i1, chg1, i2, chg2)) {
+		if (xdl_append_merge(&c, 1,
+				     i0, chg0, i1, chg1, i2, chg2)) {
 			xdl_cleanup_merge(changes);
 			return -1;
 		}
@@ -427,11 +499,14 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1,
 	while (xscr2) {
 		if (!changes)
 			changes = c;
+		i0 = xscr2->i1;
 		i1 = xscr2->i1 + xe1->xdf2.nrec - xe1->xdf1.nrec;
 		i2 = xscr2->i2;
+		chg0 = xscr2->chg1;
 		chg1 = xscr2->chg1;
 		chg2 = xscr2->chg2;
-		if (xdl_append_merge(&c, 2, i1, chg1, i2, chg2)) {
+		if (xdl_append_merge(&c, 2,
+				     i0, chg0, i1, chg1, i2, chg2)) {
 			xdl_cleanup_merge(changes);
 			return -1;
 		}
@@ -440,16 +515,17 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1,
 	if (!changes)
 		changes = c;
 	/* refine conflicts */
-	if (level > 1 &&
+	if (XDL_MERGE_ZEALOUS <= level &&
 	    (xdl_refine_conflicts(xe1, xe2, changes, xpp) < 0 ||
-	     xdl_simplify_non_conflicts(xe1, changes, level > 2) < 0)) {
+	     xdl_simplify_non_conflicts(xe1, changes,
+					level > XDL_MERGE_ZEALOUS) < 0)) {
 		xdl_cleanup_merge(changes);
 		return -1;
 	}
 	/* output */
 	if (result) {
 		int size = xdl_fill_merge_buffer(xe1, name1, xe2, name2,
-			changes, NULL);
+						 changes, NULL, level);
 		result->ptr = xdl_malloc(size);
 		if (!result->ptr) {
 			xdl_cleanup_merge(changes);
@@ -457,14 +533,15 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1,
 		}
 		result->size = size;
 		xdl_fill_merge_buffer(xe1, name1, xe2, name2, changes,
-				result->ptr);
+				      result->ptr, level);
 	}
 	return xdl_cleanup_merge(changes);
 }
 
-int xdl_merge(mmfile_t *orig, mmfile_t *mf1, const char *name1,
-		mmfile_t *mf2, const char *name2,
-		xpparam_t const *xpp, int level, mmbuffer_t *result) {
+int xdl_merge(mmfile_t *orig,
+	      mmfile_t *mf1, const char *name1,
+	      mmfile_t *mf2, const char *name2,
+	      xpparam_t const *xpp, int level, mmbuffer_t *result) {
 	xdchange_t *xscr1, *xscr2;
 	xdfenv_t xe1, xe2;
 	int status;
-- 
1.6.0.1.200.ge682f

Re: [PATCH 2/2 - RFH/WIP] xdiff-merge: optionally show conflicts in "diff3 -m" style

[Linus Torvalds]

On Thu, 28 Aug 2008, Junio C Hamano wrote:
> 
> Some poeple find it easier to be able to understand what is going on when
> they can view the common ancestor's version, which is used by "diff3 -m",
> which shows:
> 
>  <<<<<<<
>  postimage from one side;
>  |||||||
>  shared preimage;
>  =======
>  postimage of the other side; and
>  >>>>>>>
> 
> This is an initial step to bring that as an optional feature to git.
> Only "git merge-file" has been converted, with "--diff3" option.

If you have the common ancestor, why would you ever want this format, and 
not a nice conflict entry in the index?

Anyway, that's irrelevant for my real question, which is:

>  /*
> - * level == 0: mark all overlapping changes as conflict
> - * level == 1: mark overlapping changes as conflict only if not identical
> - * level == 2: analyze non-identical changes for minimal conflict set
> - * level == 3: analyze non-identical changes for minimal conflict set, but
> - *             treat hunks not containing any letter or number as conflicting
> + * "Level" parameter can be:
> + *
> + * (MINIMAL):
> + * Mark all overlapping changes as conflict
> + *
> + * (EAGER):
> + * Mark overlapping changes as conflict only if not identical
> + *
> + * (DIFF3):
> + * Same as EAGER but show the shared preimage in the output as well
> + *
> + * (ZEALOUS):
> + * Analyze non-identical changes for minimal conflict set
> + *
> + * (ZEALOUS_ALNUM):
> + * Analyze non-identical changes for minimal conflict set, but
> + * treat hunks not containing any letter or number as conflicting
>   *

Wouldn't it be much nicer to make this a bitflag, than an enumeration of 
different models?

In particular, why would it be wrong to want to be ZEALOUS (possibly 
_ALNUM) and still want DIFF3 output?

IOW, I don't think these are at all disjoint sets. In fact, I think DIFF3 
sounds not at all like a "level" to me, but like an output format thing, 
so it's in a totally different "address space".

Hmm?

		Linus

Re: [PATCH 2/2 - RFH/WIP] xdiff-merge: optionally show conflicts in "diff3 -m" style

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Thu, 28 Aug 2008, Junio C Hamano wrote:
>> 
>> Some poeple find it easier to be able to understand what is going on when
>> they can view the common ancestor's version, which is used by "diff3 -m",
>> which shows:
>> 
>>  <<<<<<<
>>  postimage from one side;
>>  |||||||
>>  shared preimage;
>>  =======
>>  postimage of the other side; and
>>  >>>>>>>
>> 
>> This is an initial step to bring that as an optional feature to git.
>> Only "git merge-file" has been converted, with "--diff3" option.
>
> If you have the common ancestor, why would you ever want this format, and 
> not a nice conflict entry in the index?

We already have that, don't we?  You can think of it as how to present
that information without resorting to "diff :1:path :2:path".

> Anyway, that's irrelevant for my real question, which is:

... the same thing as I already said after three-dashes, hence RFH/WIP.

Re: [PATCH 2/2 - RFH/WIP] xdiff-merge: optionally show conflicts in "diff3 -m" style

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> Anyway, that's irrelevant for my real question, which is:
> ...
> Wouldn't it be much nicer to make this a bitflag, than an enumeration of 
> different models?
>
> In particular, why would it be wrong to want to be ZEALOUS (possibly 
> _ALNUM) and still want DIFF3 output?
>
> IOW, I don't think these are at all disjoint sets. In fact, I think DIFF3 
> sounds not at all like a "level" to me, but like an output format thing, 
> so it's in a totally different "address space".

Ok, here is a replacement for [2/2].

This still forces XDL_MERGE_EAGER when "git merge-file --diff3" is asked
for, because higher level "merge refinement" routines do not know anything
about the necessary adjustment of the preimage pointers.

I threw away some clean-ups to xdiff/xmerge.c to use symbolic constants
when deciding what merge refinement codepaths are taken, to make it 
easier to review (the clean-up and style fixes should be done
independently, not as part of this patch).

 builtin-merge-file.c  |   10 ++++-
 xdiff/xdiff.h         |    6 +++
 xdiff/xmerge.c        |  103 ++++++++++++++++++++++++++++++++++++++++---------
 t/t6023-merge-file.sh |   37 +++++++++++++++++
 4 files changed, 135 insertions(+), 21 deletions(-)

diff --git c/builtin-merge-file.c w/builtin-merge-file.c
index 3605960..5b4f020 100644
--- c/builtin-merge-file.c
+++ w/builtin-merge-file.c
@@ -4,7 +4,7 @@
 #include "xdiff-interface.h"
 
 static const char merge_file_usage[] =
-"git merge-file [-p | --stdout] [-q | --quiet] [-L name1 [-L orig [-L name2]]] file1 orig_file file2";
+"git merge-file [-p | --stdout] [--diff3] [-q | --quiet] [-L name1 [-L orig [-L name2]]] file1 orig_file file2";
 
 int cmd_merge_file(int argc, const char **argv, const char *prefix)
 {
@@ -13,6 +13,8 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 	mmbuffer_t result = {NULL, 0};
 	xpparam_t xpp = {XDF_NEED_MINIMAL};
 	int ret = 0, i = 0, to_stdout = 0;
+	int merge_level = XDL_MERGE_ZEALOUS_ALNUM;
+	int merge_style = 0;
 
 	while (argc > 4) {
 		if (!strcmp(argv[1], "-L") && i < 3) {
@@ -25,6 +27,10 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 		else if (!strcmp(argv[1], "-q") ||
 				!strcmp(argv[1], "--quiet"))
 			freopen("/dev/null", "w", stderr);
+		else if (!strcmp(argv[1], "--diff3")) {
+			merge_style = XDL_MERGE_DIFF3;
+			merge_level = XDL_MERGE_EAGER;
+		}
 		else
 			usage(merge_file_usage);
 		argc--;
@@ -46,7 +52,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 	}
 
 	ret = xdl_merge(mmfs + 1, mmfs + 0, names[0], mmfs + 2, names[2],
-			&xpp, XDL_MERGE_ZEALOUS_ALNUM, &result);
+			&xpp, merge_level | merge_style, &result);
 
 	for (i = 0; i < 3; i++)
 		free(mmfs[i].ptr);
diff --git c/xdiff/xdiff.h w/xdiff/xdiff.h
index 413082e..deebe02 100644
--- c/xdiff/xdiff.h
+++ w/xdiff/xdiff.h
@@ -50,10 +50,16 @@ extern "C" {
 #define XDL_BDOP_CPY 2
 #define XDL_BDOP_INSB 3
 
+/* merge simplification levels */
 #define XDL_MERGE_MINIMAL 0
 #define XDL_MERGE_EAGER 1
 #define XDL_MERGE_ZEALOUS 2
 #define XDL_MERGE_ZEALOUS_ALNUM 3
+#define XDL_MERGE_LEVEL_MASK 0x0f
+
+/* merge output styles */
+#define XDL_MERGE_DIFF3 0x8000
+#define XDL_MERGE_STYLE_MASK 0x8000
 
 typedef struct s_mmfile {
 	char *ptr;
diff --git c/xdiff/xmerge.c w/xdiff/xmerge.c
index 6ffaa4f..29cdbea 100644
--- c/xdiff/xmerge.c
+++ w/xdiff/xmerge.c
@@ -30,17 +30,32 @@ typedef struct s_xdmerge {
 	 * 2 = no conflict, take second.
 	 */
 	int mode;
+	/*
+	 * These point at the respective postimages.  E.g. <i1,chg1> is
+	 * how side #1 wants to change the common ancestor; if there is no
+	 * overlap, lines before i1 in the postimage of side #1 appear
+	 * in the merge result as a region touched by neither side.
+	 */
 	long i1, i2;
 	long chg1, chg2;
+	/*
+	 * These point at the preimage; of course there is just one
+	 * preimage, that is from the shared common ancestor.
+	 */
+	long i0;
+	long chg0;
 } xdmerge_t;
 
 static int xdl_append_merge(xdmerge_t **merge, int mode,
-		long i1, long chg1, long i2, long chg2)
+			    long i0, long chg0,
+			    long i1, long chg1,
+			    long i2, long chg2)
 {
 	xdmerge_t *m = *merge;
 	if (m && (i1 <= m->i1 + m->chg1 || i2 <= m->i2 + m->chg2)) {
 		if (mode != m->mode)
 			m->mode = 0;
+		m->chg0 = i0 + chg0 - m->i0;
 		m->chg1 = i1 + chg1 - m->i1;
 		m->chg2 = i2 + chg2 - m->i2;
 	} else {
@@ -49,6 +64,8 @@ static int xdl_append_merge(xdmerge_t **merge, int mode,
 			return -1;
 		m->next = NULL;
 		m->mode = mode;
+		m->i0 = i0;
+		m->chg0 = chg0;
 		m->i1 = i1;
 		m->chg1 = chg1;
 		m->i2 = i2;
@@ -91,11 +108,13 @@ static int xdl_merge_cmp_lines(xdfenv_t *xe1, int i1, xdfenv_t *xe2, int i2,
 	return 0;
 }
 
-static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)
+static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, int i, int count, int add_nl, char *dest)
 {
-	xrecord_t **recs = xe->xdf2.recs + i;
+	xrecord_t **recs;
 	int size = 0;
 
+	recs = (use_orig ? xe->xdf1.recs : xe->xdf2.recs) + i;
+
 	if (count < 1)
 		return 0;
 
@@ -113,9 +132,19 @@ static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)
 	return size;
 }
 
+static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)
+{
+	return xdl_recs_copy_0(0, xe, i, count, add_nl, dest);
+}
+
+static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)
+{
+	return xdl_recs_copy_0(1, xe, i, count, add_nl, dest);
+}
+
 static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 			      xdfenv_t *xe2, const char *name2,
-			      int size, int i,
+			      int size, int i, int style,
 			      xdmerge_t *m, char *dest)
 {
 	const int marker_size = 7;
@@ -143,6 +172,20 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 	/* Postimage from side #1 */
 	size += xdl_recs_copy(xe1, m->i1, m->chg1, 1,
 			      dest ? dest + size : NULL);
+
+	if (style == XDL_MERGE_DIFF3) {
+		/* Shared preimage */
+		if (!dest) {
+			size += marker_size + 1;
+		} else {
+			for (j = 0; j < marker_size; j++)
+				dest[size++] = '|';
+			dest[size++] = '\n';
+		}
+		size += xdl_orig_copy(xe1, m->i0, m->chg0, 1,
+				      dest ? dest + size : NULL);
+	}
+
 	if (!dest) {
 		size += marker_size + 1;
 	} else {
@@ -170,14 +213,15 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 }
 
 static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1,
-		xdfenv_t *xe2, const char *name2, xdmerge_t *m, char *dest)
+				 xdfenv_t *xe2, const char *name2,
+				 xdmerge_t *m, char *dest, int style)
 {
 	int size, i;
 
 	for (size = i = 0; m; m = m->next) {
 		if (m->mode == 0)
 			size = fill_conflict_hunk(xe1, name1, xe2, name2,
-						  size, i, m, dest);
+						  size, i, style, m, dest);
 		else if (m->mode == 1)
 			size += xdl_recs_copy(xe1, i, m->i1 + m->chg1 - i, 0,
 					      dest ? dest + size : NULL);
@@ -342,9 +386,11 @@ static int xdl_simplify_non_conflicts(xdfenv_t *xe1, xdmerge_t *m,
  */
 static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1,
 		xdfenv_t *xe2, xdchange_t *xscr2, const char *name2,
-		int level, xpparam_t const *xpp, mmbuffer_t *result) {
+		int flags, xpparam_t const *xpp, mmbuffer_t *result) {
 	xdmerge_t *changes, *c;
-	int i1, i2, chg1, chg2;
+	int i0, i1, i2, chg0, chg1, chg2;
+	int level = flags & XDL_MERGE_LEVEL_MASK;
+	int style = flags & XDL_MERGE_STYLE_MASK;
 
 	c = changes = NULL;
 
@@ -352,11 +398,14 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1,
 		if (!changes)
 			changes = c;
 		if (xscr1->i1 + xscr1->chg1 < xscr2->i1) {
+			i0 = xscr1->i1;
 			i1 = xscr1->i2;
 			i2 = xscr2->i2 - xscr2->i1 + xscr1->i1;
+			chg0 = xscr1->chg1;
 			chg1 = xscr1->chg2;
 			chg2 = xscr1->chg1;
-			if (xdl_append_merge(&c, 1, i1, chg1, i2, chg2)) {
+			if (xdl_append_merge(&c, 1,
+					     i0, chg0, i1, chg1, i2, chg2)) {
 				xdl_cleanup_merge(changes);
 				return -1;
 			}
@@ -364,11 +413,14 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1,
 			continue;
 		}
 		if (xscr2->i1 + xscr2->chg1 < xscr1->i1) {
+			i0 = xscr2->i1;
 			i1 = xscr1->i2 - xscr1->i1 + xscr2->i1;
 			i2 = xscr2->i2;
+			chg0 = xscr2->chg1;
 			chg1 = xscr2->chg1;
 			chg2 = xscr2->chg2;
-			if (xdl_append_merge(&c, 2, i1, chg1, i2, chg2)) {
+			if (xdl_append_merge(&c, 2,
+					     i0, chg0, i1, chg1, i2, chg2)) {
 				xdl_cleanup_merge(changes);
 				return -1;
 			}
@@ -385,19 +437,26 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1,
 			int off = xscr1->i1 - xscr2->i1;
 			int ffo = off + xscr1->chg1 - xscr2->chg1;
 
+			i0 = xscr1->i1;
 			i1 = xscr1->i2;
 			i2 = xscr2->i2;
-			if (off > 0)
+			if (off > 0) {
+				i0 -= off;
 				i1 -= off;
+			}
 			else
 				i2 += off;
+			chg0 = xscr1->i1 + xscr1->chg1 - i0;
 			chg1 = xscr1->i2 + xscr1->chg2 - i1;
 			chg2 = xscr2->i2 + xscr2->chg2 - i2;
 			if (ffo > 0)
 				chg2 += ffo;
-			else
+			else {
+				chg0 -= ffo;
 				chg1 -= ffo;
-			if (xdl_append_merge(&c, 0, i1, chg1, i2, chg2)) {
+			}
+			if (xdl_append_merge(&c, 0,
+					     i0, chg0, i1, chg1, i2, chg2)) {
 				xdl_cleanup_merge(changes);
 				return -1;
 			}
@@ -414,11 +473,14 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1,
 	while (xscr1) {
 		if (!changes)
 			changes = c;
+		i0 = xscr1->i1;
 		i1 = xscr1->i2;
 		i2 = xscr1->i1 + xe2->xdf2.nrec - xe2->xdf1.nrec;
+		chg0 = xscr1->chg1;
 		chg1 = xscr1->chg2;
 		chg2 = xscr1->chg1;
-		if (xdl_append_merge(&c, 1, i1, chg1, i2, chg2)) {
+		if (xdl_append_merge(&c, 1,
+				     i0, chg0, i1, chg1, i2, chg2)) {
 			xdl_cleanup_merge(changes);
 			return -1;
 		}
@@ -427,11 +489,14 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1,
 	while (xscr2) {
 		if (!changes)
 			changes = c;
+		i0 = xscr2->i1;
 		i1 = xscr2->i1 + xe1->xdf2.nrec - xe1->xdf1.nrec;
 		i2 = xscr2->i2;
+		chg0 = xscr2->chg1;
 		chg1 = xscr2->chg1;
 		chg2 = xscr2->chg2;
-		if (xdl_append_merge(&c, 2, i1, chg1, i2, chg2)) {
+		if (xdl_append_merge(&c, 2,
+				     i0, chg0, i1, chg1, i2, chg2)) {
 			xdl_cleanup_merge(changes);
 			return -1;
 		}
@@ -449,7 +514,7 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1,
 	/* output */
 	if (result) {
 		int size = xdl_fill_merge_buffer(xe1, name1, xe2, name2,
-			changes, NULL);
+			changes, NULL, style);
 		result->ptr = xdl_malloc(size);
 		if (!result->ptr) {
 			xdl_cleanup_merge(changes);
@@ -457,14 +522,14 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1,
 		}
 		result->size = size;
 		xdl_fill_merge_buffer(xe1, name1, xe2, name2, changes,
-				result->ptr);
+				      result->ptr, style);
 	}
 	return xdl_cleanup_merge(changes);
 }
 
 int xdl_merge(mmfile_t *orig, mmfile_t *mf1, const char *name1,
 		mmfile_t *mf2, const char *name2,
-		xpparam_t const *xpp, int level, mmbuffer_t *result) {
+		xpparam_t const *xpp, int flags, mmbuffer_t *result) {
 	xdchange_t *xscr1, *xscr2;
 	xdfenv_t xe1, xe2;
 	int status;
@@ -501,7 +566,7 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, const char *name1,
 		} else {
 			status = xdl_do_merge(&xe1, xscr1, name1,
 					      &xe2, xscr2, name2,
-					      level, xpp, result);
+					      flags, xpp, result);
 		}
 		xdl_free_script(xscr1);
 		xdl_free_script(xscr2);


diff --git c/t/t6023-merge-file.sh w/t/t6023-merge-file.sh
index 42620e0..f3484e3 100755
--- c/t/t6023-merge-file.sh
+++ w/t/t6023-merge-file.sh
@@ -161,4 +161,41 @@ test_expect_success 'ZEALOUS_ALNUM' '
 
 '
 
+cat >expect <<\EOF
+Dominus regit me,
+<<<<<<< new8.txt
+et nihil mihi deerit;
+
+
+
+
+In loco pascuae ibi me collocavit;
+super aquam refectionis educavit me.
+|||||||
+et nihil mihi deerit.
+In loco pascuae ibi me collocavit,
+super aquam refectionis educavit me;
+=======
+et nihil mihi deerit,
+
+
+
+
+In loco pascuae ibi me collocavit --
+super aquam refectionis educavit me,
+>>>>>>> new9.txt
+animam meam convertit,
+deduxit me super semitas jusitiae,
+propter nomen suum.
+Nam et si ambulavero in medio umbrae mortis,
+non timebo mala, quoniam TU mecum es:
+virga tua et baculus tuus ipsa me consolata sunt.
+EOF
+
+test_expect_success '"diff3 -m" style output' '
+	test_must_fail git merge-file -p --diff3 \
+		new8.txt new5.txt new9.txt >actual &&
+	test_cmp expect actual
+'
+
 test_done

Re: [PATCH 2/2 - RFH/WIP] xdiff-merge: optionally show conflicts in "diff3 -m" style

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Thu, 28 Aug 2008, Junio C Hamano wrote:
>> 
>> Some poeple find it easier to be able to understand what is going on when
>> they can view the common ancestor's version, which is used by "diff3 -m",
>> which shows:
>> 
>>  <<<<<<<
>>  postimage from one side;
>>  |||||||
>>  shared preimage;
>>  =======
>>  postimage of the other side; and
>>  >>>>>>>
> ...
> Wouldn't it be much nicer to make this a bitflag, than an enumeration of 
> different models?
>
> In particular, why would it be wrong to want to be ZEALOUS (possibly 
> _ALNUM) and still want DIFF3 output?
>
> IOW, I don't think these are at all disjoint sets. In fact, I think DIFF3 
> sounds not at all like a "level" to me, but like an output format thing, 
> so it's in a totally different "address space".
>
> Hmm?

I've thought about this a bit more, and I think "diff3 -m" style output
would not make sense for anything more aggressive than XDL_MERGE_EAGER,
because of the way how the merge reduction works.

Suppose a common ancestor (shared preimage) is modified to postimage #1
and #2 (each letter represents one line):

                     #####
    postimage#1: 1234ABCDE789
                    |    /
                    |   /
    preimage:    123456789
                    |   \
    postimage#2: 1234AXYE789
                     ####

XDL_MERGE_MINIMAL and XDL_MERGE_EAGER would:

 (1) find the s/56/ABCDE/ done on one side and s/56/AXYE/ done on the
     other side,

 (2) notice that they touch an overlapping area, and

 (3) mark it as a conflict, "ABCDE vs AXYE".

The difference between the two algorithm is that EAGER drops the hunk if
the postimages match (i.e. both sides modified the same way), while
MINIMAL keeps it.  There is no other operation performed to the hunk.  As
the result, lines marked with "#" in the above picure will be in the RCS
merge style output.  We can sanely say that the part from the preimage
that corresponds to these conflicting changes is "567", which is what
"diff3 -m" style output adds to it.

Now, XDL_MERGE_ZEALOUS postprocesses output XDL_MERGE_EAGER would have
produced, by looking at the differences between the changes two postimages
made.  It notices that both sides start their new contents with "A", and
excludes it from the output (it also excludes "E" for the same reason).
The conflict that used to be "ABCDE vs AXYE" is now "BCD vs XY".

There could even be matching part between two postimages in the middle.
Instead of one side rewriting the shared "56" to "ABCDE" and the other
side to "AXYE", imagine the case where the postimages are "ABCDE" and
"AXCYE", in which case instead of having one conflicted hunk "BCD vs XY",
you would have two conflicting hunks "B vs X" and "D vs Y".

In either case, once you reduce "ABCDE vs AXYE" to "BCD vs XY" (or "ABCDE
vs AXCYE" to "B vs X" and "D vs Y"), there is no part from the preimage
that corresponds to the conflicting change made in both postimages
anymore.  In other words, conflict reduced by ZEALOUS algorithm cannot be
expressed in "diff3 -m" style.

You can still have "diff3 -m" style with MINIMAL or EAGER, but I think MINIMAL
is pointless and using "diff3 -m" style with it is even more so.

Re: [PATCH 2/2 - RFH/WIP] xdiff-merge: optionally show conflicts in "diff3 -m" style

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
>> On Thu, 28 Aug 2008, Junio C Hamano wrote:
>>> 
>>> Some poeple find it easier to be able to understand what is going on when
>>> they can view the common ancestor's version, which is used by "diff3 -m",
>>> which shows:
>>> 
>>>  <<<<<<<
>>>  postimage from one side;
>>>  |||||||
>>>  shared preimage;
>>>  =======
>>>  postimage of the other side; and
>>>  >>>>>>>
>>> 
>>> This is an initial step to bring that as an optional feature to git.
>>> Only "git merge-file" has been converted, with "--diff3" option.
>>
>> If you have the common ancestor, why would you ever want this format, and 
>> not a nice conflict entry in the index?
>
> We already have that, don't we?  You can think of it as how to present
> that information without resorting to "diff :1:path :2:path".

I've been using this for my own work, and here is a typical example.  This
happened while rebuilding 'pu', merging jc/merge-whitespace topic on top
of what contains jc/better-conflict-resolution topic.

    In case if anybody is interested in reproducing this to follow the
    discussion of improving the support of conflicted merge resolution,
    here is how to reproduce this:

        $ git checkout 7ead6a7^
        $ git merge ^2

In builtin-merge-file.c, there appears:

	ret = xdl_merge(mmfs + 1, mmfs + 0, names[0], mmfs + 2, names[2],
<<<<<<< HEAD:builtin-merge-file.c
			&xpp, merge_level | merge_style, &result);
|||||||
			&xpp, XDL_MERGE_ZEALOUS_ALNUM, &result);
=======
			&xpp, XDL_MERGE_ZEALOUS_ALNUM, ws_rule, &result);
>>>>>>> jc/merge-whitespace:builtin-merge-file.c

	for (i = 0; i < 3; i++)

In this case, both branches are my code, so I do not need the additional
hint to see that one branch added ws_rule parameter while the other one
generalized the "merge simplify level" parameter and is now passing a
bitwise OR of two variables.  But if somebody else is doing the merge, it
may not be so obvious how the resolution should be.

On the other hand, this appears in ll-merge.c:

	xpparam_t xpp;
<<<<<<< HEAD:ll-merge.c
	int style = 0;
|||||||
=======
	unsigned ws_rule = 0;
>>>>>>> jc/merge-whitespace:ll-merge.c

This is obviously useless; perhaps we can drop the "original" hunk when it
is empty to save space?

Another thing that helped me figure out what was going on was when merging
mv/merge-recursive topic on top of next.

    In case if anybody is interested in reproducing this to follow the
    discussion of improving the support of conflicted merge resolution,
    here is how to reproduce this:

        $ git checkout ebe0e3b^
        $ git merge ebe0e3b^2

This merge involves removal of quite a lot of lines to a new file
merge-recursive.c from existing builtin-merge-recursive.c; the default
conflict simplification (XDL_MERGE_ZEALOUS) works _extremely_ well for
this merge, in that it shows this (I'll refer to this result as *1* in a
later paragraph):

    <<<<<<<
    static struct commit *get_ref(const char *ref)
    {
    ...
    }

    static int merge_config(const char *var, const char *value, void *cb)
    {
    ...
    }

    =======
    >>>>>>>

It just shows we have _some_ changes to these two functions since the
other side removed them from this file, but it does not say what the
change is.  However, with the "common ancestor" version, here is what I
get (you need to use 'next' and "merge.conflictstyle" configuration set to
"diff3" to reproduce this):

    <<<<<<<
    static struct commit *get_ref(const char *ref)
    {
    ...
    }

    static int merge_config(const char *var, const char *value, void *cb)
    {
    ...
            return git_xmerge_config(var, value, cb);
    }

    |||||||
    static struct commit *get_ref(const char *ref)
    {
    ...
    }

    static int merge_config(const char *var, const char *value, void *cb)
    {
    ...
            return git_default_config(var, value, cb);
    }

    =======
    >>>>>>> theirs

By comparing the first and second part, we can tell that the call to
git_default_config() was replaced to a call to git_xmerge_config().  And
this is a crucial clue to me to make an corresponding update to the
function that now lives in merge-file.c.

On average, I am finding that "diff3 -m" format more irritating than
useful.  However, on occasions like this, I am finding it quite useful.

My observation so far suggests that it would be best for me to leave the
configuration "merge.conflictstyle" to the default "merge", and instead
give an option to allow me to tell "git checkout -m -- $path" (which is
also a new feature; it overwrites the $path by the result of a fresh merge
to reproduce the conflicted state in the working tree, using the three
stages recorded in the index) to use "diff3 -m" style, when I want to.

That means the command sequence would look like:

    $ git merge mv/merge-recursive ;# git merge ebe0e3b^2
    .. oops, heavily conflicted merge and I see *1*, which is not
    .. very useful.  Let's see what is going on.
    .. Note that this --merge=diff3 option does not exist yet.
    $ git checkout --merge=diff3 -- builtin-merge-recursive.c
    $ emacs builtin-merge-recursive.c
    .. split the buffer into two, move the cursor to the line after <<<
    .. in one window while moving the cursor to the line after ||| line
    .. in the other one, and say "M-x compare-windows".  Aha, there is
    .. a change there.

Having said all that, I now realize that the way I described in my
response to you is just as easy to view this particular case:

    $ git merge mv/merge-recursive ;# git merge ebe0e3b^2
    .. oops, heavily conflicted merge and I see *1*, which is not
    .. very useful.  Let's see what is going on.
    .. Note that this --merge=diff3 option does not exist yet.
    $ git diff :{1,2}:builtin-merge-recursive.c ;# shell expands {1,2} ;-)

which exactly shows what one side changed -- instead of calling
git_default_config(), the code calls git_xmerge_config().

$ git diff :{1,2}:builtin-merge-recursive.c
diff --git a/:1:builtin-merge-recursive.c b/:2:builtin-merge-recursive.c
index dfb363e..f3b6ede 100644
--- a/:1:builtin-merge-recursive.c
+++ b/:2:builtin-merge-recursive.c
@@ -1348,7 +1348,7 @@ static int merge_config(const char *var, const char *value, void *cb)
                merge_rename_limit = git_config_int(var, value);
                return 0;
        }
-       return git_default_config(var, value, cb);
+       return git_xmerge_config(var, value, cb);
 }

 int cmd_merge_recursive(int argc, const char **argv, const char *prefix)

Re: [PATCH 2/2 - RFH/WIP] xdiff-merge: optionally show conflicts in "diff3 -m" style

[Linus Torvalds]

On Sun, 31 Aug 2008, Junio C Hamano wrote:
> 
> On average, I am finding that "diff3 -m" format more irritating than
> useful.  However, on occasions like this, I am finding it quite useful.

I have to say that the best feature of the standard merge is the zealous 
version, which apparently doesn't work well with the "diff3 -m" format.

I do agree that what is often really nasty with the normal merge conflict 
info is when you actually need the original thing to understand why some 
hunk was left, and yes, a common case of that is that one side simply 
removed a function, and the other side had modified it. At that point the 
resulting conflict is often hard to understand without seeing the 
original, exactly because you don't see any actual "conflict", you only 
see one side (the other side being empty).

But what really saves that situation is the combination of

 - 'gitk --merge <file>'
 - 'git diff' showing the multi-way merge

and I find myself really _hating_ doing rebases because the merge helpers 
are so totally useless (ie "gitk --merge" at least didn't use to work 
across a rebase conflict because MERGE_HEAD isn't set)

But the biggest problem, and the reason I _really_ detest the diff3 
format, is that small merges are fairly often pretty easy to see anyway. 
If the conflict markers all fit in one screenful, it's generally fairly 
easy to see why something conflicted, because you can visually compare 
things.

But the complicated cases are when there are bigger changes, and the 
conflict is over many many lines of code, and it's really hard to visually 
see what changed. And the diff3 format makes this worse - it not only 
makes the conflict 50% bigger to begin with, it moves the two conflicting 
versions away from each other, making that visual comparison much harder.

Now, there are tools to help with that. I think various of the graphical 
merge tools understand the diff3 format, and then ti can really help. But 
I think it hurts for a lot of the _common_ cases.

> My observation so far suggests that it would be best for me to leave the
> configuration "merge.conflictstyle" to the default "merge", and instead
> give an option to allow me to tell "git checkout -m -- $path" (which is
> also a new feature; it overwrites the $path by the result of a fresh merge
> to reproduce the conflicted state in the working tree, using the three
> stages recorded in the index) to use "diff3 -m" style, when I want to.

Now *this* I think is a great idea! 

The reason I think it's a great idea is that it solves so many _different_ 
issues (which is the mark of a really good solution):

 - it fixes my problem with diff3 output: the fact that it's more annoying 
   by _default_ than it is occasionally useful.

   If the default isn't to do it - since by default it often hurts - but 
   you have the option to do it when there is something confusing going on 
   (like the "one side disappeared, why did it conflict?" case), then you 
   have the best of both worlds - a good default with a way to dig deeper 
   when you need to.

 - it fixes another totally unrelated problem: incorrect merge 
   resolutions.

   Again, I find this to be fairly rare, but what git is good at is to 
   make incremental resolutions for merge problems - you can resolve the 
   merge in the work tree, then compile and test the result before 
   actually committing it, and "git diff" always gives relevant and 
   interesting output for the merge.

   And _occasionally_ the resolve looks obvious, but then when you compile 
   things you notice that it doesn't work because (for example) you 
   resolved it by removing one side (exactly because the other side was a 
   removal), and it turned out that the conflict was adding a function 
   that you hadn't realized was new, and was needed.

   And while "git diff" is fine, and you can cut-and-paste things and try 
   to re-resolve it that way, I have occasionally decided to just do a 
   "git reset" and re-do the whole merge.

   But your idea allows us to just re-do the merge for a single file.

So I think we do quite well already, but your solution really does sound 
like a good and useful addition to the toolbox.

		Linus

Re: [PATCH 2/2 - RFH/WIP] xdiff-merge: optionally show conflicts in "diff3 -m" style

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Sun, 31 Aug 2008, Junio C Hamano wrote:
>> ...
>> My observation so far suggests that it would be best for me to leave the
>> configuration "merge.conflictstyle" to the default "merge", and instead
>> give an option to allow me to tell "git checkout -m -- $path" (which is
>> also a new feature; it overwrites the $path by the result of a fresh merge
>> to reproduce the conflicted state in the working tree, using the three
>> stages recorded in the index) to use "diff3 -m" style, when I want to.
>
> Now *this* I think is a great idea! 
>
> The reason I think it's a great idea is that it solves so many _different_ 
> issues (which is the mark of a really good solution):
> ...
>  - it fixes another totally unrelated problem: incorrect merge 
>    resolutions.

I do not know if rerere often kicks in in your workflow, but occasionally
I notice that I have a faulty merge recorded by it, which automatically is
applied again when reproducing a merge.  And it makes me deeply regret
that I invented the rerere mechanism.

The best solution for this issue I found so far is embarrasingly clumsy.
Run "ls -tl .git/rr-cache" to find the newest one with "thisimage", make
sure it is the right one, remove the directory and redo the merge.

Actually, the feature of "git checkout -m -- $path" to reproduce the merge
for the named path was conceived to address this "I cannot easily correct
the faulty resolution that was re-applied" issue.