git-core: conflictstyle=diff3 doesn't actually use diff3 compatible format

Hi gitsters,

Stefan Monnier wrote [1]:

No opinion on this myself. I’d be interested to hear from xdiff people whether it should be easy to add the ancestor name to the output.

See http://thread.gmane.org/gmane.comp.version-control.git/136500

Short answer: the usual method is to report bugs to the list, preferably with a patch for t/ or even better, a fix. ;-)

Jonathan

[1] http://bugs.debian.org/572720

I don't think there was any xdiff people involved in this area.

I suspect that our tools actually rely on the common ancestor markers not having any extra cruft after them, so it would be introducing a bug if you change this output without changing the places that read them (I know about "rerere", but there may be others).

I guess the relevant xdiff person was you. ;-)

Thank you for the quick response. On the xdiff level, it looks like all that is needed is to pass the ancestor label as a member of struct s_xmparam, and then fill_conflict_hunk() could respect that. Not complicated at all.

For merge_trees() users, the ancestor label could be passed with branch1 and branch2 in struct merge_options.

That leaves the question of merge_recursive(). With merge_recursive(), there is more than one ancestor, so it is not completely clear what the diff3 merge should do. Currently it writes something like this:

<<<<<<< HEAD
Conflict resolution is hard;
let's go shopping.
|||||||
<<<<<<< Temporary merge branch 1
Who knows whose this is?
|||||||
Ancient history.
=======
Another intermediate result.

=======
Git makes conflict resolution easy.

which is hard to read [1]. Probably it would be better to use a consolidated common ancestor, by cocatenating the internal common ancestors; in this simple case, that would look like this:

<<<<<<< HEAD
Conflict resolution is hard;
let's go shopping.
||||||||
Ancient history.
========
Git makes conflict resolution easy.

What should be the label of this possibly fictional merge base?

Jonathan

[1] For people and for rerere. See http://bugs.debian.org/569645

is_cmarker() is called with want_sp = 0 for '=======' and '|||||||', so
the current code accepts both "||||||| rubbish\n" and "|||||||\n".

Also, rerere sanitizes a diff3 style conflict into a merge style conflict before writing a preimage to rr-cache for later comparison [*1*], so it probably is Ok even if we changed it in a way that it no longer reads the current output without " rubbish" after the marker. It appears that it is already prepared to take either form, so we are probably safe here.

But I didn't check subcommands other than rerere that may read and act on the conflict markers.

If we were to change the output, git needs to be prepared to see both the output before and after the change and behave sensibly; it is not limited to rerere.

For the reverse combined diff format, rerere probably needs to be taught how to convert it as merge style conflict before computing the conflict identifier and write it out in the preimage file, I think.

[Footnote]

*1* See 387c9d4 (rerere: understand "diff3 -m" style conflicts with the original, 2008-08-29).

I changed my mind; it looked like a useful and fun thing to do, so here it is.

The bad news is that sometimes the labels for the ancestor are kind of pointless, as in “merged common ancestors” for merge-recursive. I am not planning to improve this soon. I’d be thrilled if someone changes it to something like “merge of ac76d, 8d7ca9, and 81873”.

This patch series adds a label for the common ancestor to various places git outputs conflict hunks:

<<<<<<< ours
Text from the current branch
||||||| original
Text from the merge base
=======
Text from the remote branch

I am hoping this output will be more self explanatory, especially in cases where it is not completely obvious to a naïve user what the common ancestor would be, such as cherry-pick.

This passes all tests here and I tried to find any untested in-tree consumers of conflict hunks that would be affected to make sure it would be safe. I would like to see it get some testing on other machines, especially by people using merge tools and other programs that parse conflicts.

Thoughts?
Jonathan Nieder (10):
xdl_merge(): add optional ancestor label to diff3-style output merge-file --diff3: add a label for ancestor ll_merge(): add ancestor label parameter for diff3-style output checkout --conflict=diff3: add a label for ancestor merge_file(): add comment explaining behavior wrt conflict style merge_trees(): add ancestor label parameter for diff3-style output
tests: document format of conflicts from checkout -m checkout -m --conflict=diff3: add a label for ancestor
cherry-pick: add a label for ancestor
merge-recursive: add a label for ancestor

 builtin/checkout.c    |    3 +-
 builtin/merge-file.c  |    1 +
 builtin/revert.c      |    1 +
 ll-merge.c            |   20 ++++++++------
 ll-merge.h            |    2 +-
 merge-file.c          |    8 +++++-
 merge-recursive.c     |   12 ++++++--
 merge-recursive.h     |    1 +
 rerere.c              |    4 +-
 t/t6023-merge-file.sh |    2 +-
 t/t7201-co.sh         |   69 ++++++++++++++++++++++++++++++++++++++++++++----
 xdiff/xdiff.h         |    1 +
 xdiff/xmerge.c        |   16 ++++++++++-
 13 files changed, 114 insertions(+), 26 deletions(-)

[1] http://bugs.debian.org/572720

The ‘git checkout --conflict=diff3’ command can be used to present conflicts hunks including text from the common ancestor:

	<<<<<<< ours
	ourside
	|||||||
	original
	=======
	theirside
	>>>>>>> theirs

The added information is helpful for resolving merges by hand, and merge tools can usually grok it because it is very similar to the output from diff3 -m.

A subtle change can help more tools to understand the output. ‘diff3’ includes the name of the merge base on the ||||||| line of the output, and some tools misparse the conflict hunks without it. Add a new xmp->ancestor parameter to xdl_merge() for use with conflict style XDL_MERGE_DIFF3 as a label on the ||||||| line for any conflict hunks.

If xmp->ancestor is NULL, the output format is unchanged. ‘git checkout’ and other users within git all use NULL for this parameter, so this change only provides unexposed plumbing for a fix: it does not affect the outward behavior of git in any way.

---
 xdiff/xdiff.h  |    1 +
 xdiff/xmerge.c |   16 ++++++++++++++--
 2 files changed, 15 insertions(+), 2 deletions(-)

index 3f6229e..abaf960 100644
@@ -117,6 +117,7 @@ int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
 typedef struct s_xmparam {
 	xpparam_t xpp;
 	int marker_size;
+	const char *ancestor;
 } xmparam_t;
 
 #define DEFAULT_CONFLICT_MARKER_SIZE 7

index 8cbe45e..3a0ae14 100644
@@ -144,11 +144,13 @@ static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)
 
 static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 			      xdfenv_t *xe2, const char *name2,
+			      const char *name3,
 			      int size, int i, int style,
 			      xdmerge_t *m, char *dest, int marker_size)
 {
 	int marker1_size = (name1 ? strlen(name1) + 1 : 0);
 	int marker2_size = (name2 ? strlen(name2) + 1 : 0);
+	int marker3_size = (name3 ? strlen(name3) + 1 : 0);
 	int j;
 
 	if (marker_size <= 0)

@@ -178,10 +180,15 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 	if (style == XDL_MERGE_DIFF3) {
 		/* Shared preimage */
 		if (!dest) {
-			size += marker_size + 1;
+			size += marker_size + 1 + marker3_size;
 		} else {
 			for (j = 0; j < marker_size; j++)
 				dest[size++] = '|';
+			if (marker3_size) {
+				dest[size] = ' ';
+				memcpy(dest + size + 1, name3, marker3_size - 1);
+				size += marker3_size;
+			}
 			dest[size++] = '\n';
 		}
 		size += xdl_orig_copy(xe1, m->i0, m->chg0, 1,

@@ -216,6 +223,7 @@ 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,
+				 const char *ancestor_name,
 				 int favor,
 				 xdmerge_t *m, char *dest, int style,
 				 int marker_size)

@@ -228,6 +236,7 @@ static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1,
 
 		if (m->mode == 0)
 			size = fill_conflict_hunk(xe1, name1, xe2, name2,
+						  ancestor_name,
 						  size, i, style, m, dest,
 						  marker_size);
 		else if (m->mode == 1)

@@ -397,6 +406,7 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1,
 		int flags, xmparam_t const *xmp, mmbuffer_t *result) {
 	xdmerge_t *changes, *c;
 	xpparam_t const *xpp = &xmp->xpp;
+	const char * const ancestor_name = xmp->ancestor;
 	int i0, i1, i2, chg0, chg1, chg2;
 	int level = flags & XDL_MERGE_LEVEL_MASK;
 	int style = flags & XDL_MERGE_STYLE_MASK;

@@ -534,6 +544,7 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1,
 	if (result) {
 		int marker_size = xmp->marker_size;
 		int size = xdl_fill_merge_buffer(xe1, name1, xe2, name2,
+						 ancestor_name,
 						 favor, changes, NULL, style,
 						 marker_size);
 		result->ptr = xdl_malloc(size);

@@ -542,7 +553,8 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, const char *name1,
 			return -1;
 		}
 		result->size = size;
-		xdl_fill_merge_buffer(xe1, name1, xe2, name2, favor, changes,
+		xdl_fill_merge_buffer(xe1, name1, xe2, name2,
+				      ancestor_name, favor, changes,
 				      result->ptr, style, marker_size);
 	}
 	return xdl_cleanup_merge(changes);

1.7.0

I think this patch itself makes sense, but if you were to add one name to xmparam structure, wouldn't it make sense to store all three names in there?

Good idea. Will do.

Ciao,
Jonathan

That would also mean that existing name1 and name2 arguments many functions (starting from xdl_merge()) have will go.

git merge-file --diff3 can be used to present conflicts hunks including text from the common ancestor.

The added information can be very helpful for resolving a merge by hand, and merge tools can usually grok it because it looks like output from diff3 -m. Unfortunately, some cannot reportedly, because ‘diff3’ includes a label for the merge base on the ||||||| line and some tools cannot parse conflict hunks without such a label. So write the base-name as passed in a -L option (or the name of the “ancestor” file by default) on that line.

git rerere will not have trouble parsing this output, since instead of looking for a newline, it looks for whitespace after the ||||||| marker. No other code in git tries to parse conflict hunks.

---
This requires a slight change when merging with next. A test was added to t6023-merge-file.sh for the longer conflict markers; the corresponding line in that new test needs to have new5.txt added as well.

 builtin/merge-file.c  |    1 +
 t/t6023-merge-file.sh |    2 +-
 2 files changed, 2 insertions(+), 1 deletions(-)

index 1e70073..0ff8b03 100644
@@ -72,6 +72,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 					argv[i]);
 	}
 
+	xmp.ancestor = names[1];
 	ret = xdl_merge(mmfs + 1, mmfs + 0, names[0], mmfs + 2, names[2],
 			&xmp, XDL_MERGE_FLAGS(level, style, favor), &result);
 

index d605024..16c3c69 100755
@@ -181,7 +181,7 @@ et nihil mihi deerit;
 
 In loco pascuae ibi me collocavit;
 super aquam refectionis educavit me.
-|||||||
+||||||| new5.txt
 et nihil mihi deerit.
 In loco pascuae ibi me collocavit,
 super aquam refectionis educavit me;

1.7.0

Various commands using the ll_merge() function present conflict hunks in output something like what ‘diff3 -m’ produces if the merge.conflictstyle configuration option is set to diff3.

Unfortunately, the format is not fully compatible yet. ‘diff3’ includes the name of the merge base on the ||||||| line of the output, and some tools are reported to misparse the conflict hunks without it.

Add a new ancestor_label parameter to ll_merge() to give callers the power to rectify this situation. Let all callers pass NULL as that parameter for now to avoid distracting changes in behavior when testing this interface change.

---
A change to unexposed plumbing only.

 builtin/checkout.c |    2 +-
 ll-merge.c         |   20 +++++++++++---------
 ll-merge.h         |    2 +-
 merge-file.c       |    2 +-
 merge-recursive.c  |    2 +-
 rerere.c           |    4 ++--
 6 files changed, 17 insertions(+), 15 deletions(-)

index acefaaf..d67f809 100644
@@ -149,7 +149,7 @@ static int checkout_merged(int pos, struct checkout *state)
 	read_mmblob(&ours, active_cache[pos+1]->sha1);
 	read_mmblob(&theirs, active_cache[pos+2]->sha1);
 
-	status = ll_merge(&result_buf, path, &ancestor,
+	status = ll_merge(&result_buf, path, &ancestor, NULL,
 			  &ours, "ours", &theirs, "theirs", 0);
 	free(ancestor.ptr);
 	free(ours.ptr);

index 4c7f11b..68fa1e6 100644
@@ -15,7 +15,7 @@ struct ll_merge_driver;
 typedef int (*ll_merge_fn)(const struct ll_merge_driver *,
 			   mmbuffer_t *result,
 			   const char *path,
-			   mmfile_t *orig,
+			   mmfile_t *orig, const char *orig_name,
 			   mmfile_t *src1, const char *name1,
 			   mmfile_t *src2, const char *name2,
 			   int flag,

@@ -36,7 +36,7 @@ struct ll_merge_driver {
 static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
 			   mmbuffer_t *result,
 			   const char *path_unused,
-			   mmfile_t *orig,
+			   mmfile_t *orig, const char *orig_name,
 			   mmfile_t *src1, const char *name1,
 			   mmfile_t *src2, const char *name2,
 			   int flag, int marker_size)

@@ -57,7 +57,7 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
 static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
 			mmbuffer_t *result,
 			const char *path,
-			mmfile_t *orig,
+			mmfile_t *orig, const char *orig_name,
 			mmfile_t *src1, const char *name1,
 			mmfile_t *src2, const char *name2,
 			int flag, int marker_size)

@@ -73,12 +73,14 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
 			path, name1, name2);
 		return ll_binary_merge(drv_unused, result,
 				       path,
-				       orig, src1, name1,
+				       orig, orig_name,
+				       src1, name1,
 				       src2, name2,
 				       flag, marker_size);
 	}
 
 	memset(&xmp, 0, sizeof(xmp));
+	xmp.ancestor = orig_name;
 	if (git_xmerge_style >= 0)
 		style = git_xmerge_style;
 	if (marker_size > 0)

@@ -93,7 +95,7 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
 static int ll_union_merge(const struct ll_merge_driver *drv_unused,
 			  mmbuffer_t *result,
 			  const char *path_unused,
-			  mmfile_t *orig,
+			  mmfile_t *orig, const char *orig_name,
 			  mmfile_t *src1, const char *name1,
 			  mmfile_t *src2, const char *name2,
 			  int flag, int marker_size)

@@ -106,7 +108,7 @@ static int ll_union_merge(const struct ll_merge_driver *drv_unused,
 	saved_style = git_xmerge_style;
 	git_xmerge_style = 0;
 	status = ll_xdl_merge(drv_unused, result, path_unused,
-			      orig, src1, NULL, src2, NULL,
+			      orig, orig_name, src1, NULL, src2, NULL,
 			      flag, marker_size);
 	git_xmerge_style = saved_style;
 	if (status <= 0)

@@ -165,7 +167,7 @@ static void create_temp(mmfile_t *src, char *path)
 static int ll_ext_merge(const struct ll_merge_driver *fn,
 			mmbuffer_t *result,
 			const char *path,
-			mmfile_t *orig,
+			mmfile_t *orig, const char *orig_name,
 			mmfile_t *src1, const char *name1,
 			mmfile_t *src2, const char *name2,
 			int flag, int marker_size)

@@ -356,7 +358,7 @@ static int git_path_check_merge(const char *path, struct git_attr_check check[2]
 
 int ll_merge(mmbuffer_t *result_buf,
 	     const char *path,
-	     mmfile_t *ancestor,
+	     mmfile_t *ancestor, const char *ancestor_label,
 	     mmfile_t *ours, const char *our_label,
 	     mmfile_t *theirs, const char *their_label,
 	     int flag)

@@ -378,7 +380,7 @@ int ll_merge(mmbuffer_t *result_buf,
 	driver = find_ll_merge_driver(ll_driver_name);
 	if (virtual_ancestor && driver->recursive)
 		driver = find_ll_merge_driver(driver->recursive);
-	return driver->fn(driver, result_buf, path, ancestor,
+	return driver->fn(driver, result_buf, path, ancestor, ancestor_label,
 			  ours, our_label, theirs, their_label,
 			  flag, marker_size);
 }

index 5788922..57754cc 100644
@@ -7,7 +7,7 @@
 
 int ll_merge(mmbuffer_t *result_buf,
 	     const char *path,
-	     mmfile_t *ancestor,
+	     mmfile_t *ancestor, const char *ancestor_label,
 	     mmfile_t *ours, const char *our_label,
 	     mmfile_t *theirs, const char *their_label,
 	     int flag);

index fd34d76..07cc0c9 100644
@@ -30,7 +30,7 @@ static void *three_way_filemerge(const char *path, mmfile_t *base, mmfile_t *our
 	int merge_status;
 	mmbuffer_t res;
 
-	merge_status = ll_merge(&res, path, base,
+	merge_status = ll_merge(&res, path, base, NULL,
 				our, ".our", their, ".their", 0);
 	if (merge_status < 0)
 		return NULL;

index 195ebf9..3b2cc9d 100644
@@ -640,7 +640,7 @@ static int merge_3way(struct merge_options *o,
 	read_mmblob(&src1, a->sha1);
 	read_mmblob(&src2, b->sha1);
 
-	merge_status = ll_merge(result_buf, a->path, &orig,
+	merge_status = ll_merge(result_buf, a->path, &orig, NULL,
 				&src1, name1, &src2, name2,
 				(!!o->call_depth) | (favor << 1));
 

index a59f74f..f221bed 100644
@@ -319,7 +319,7 @@ static int handle_cache(const char *path, unsigned char *sha1, const char *outpu
 		if (!mmfile[i].ptr && !mmfile[i].size)
 			mmfile[i].ptr = xstrdup("");
 	}
-	ll_merge(&result, path, &mmfile[0],
+	ll_merge(&result, path, &mmfile[0], NULL,
 		 &mmfile[1], "ours",
 		 &mmfile[2], "theirs", 0);
 	for (i = 0; i < 3; i++)

@@ -376,7 +376,7 @@ static int merge(const char *name, const char *path)
 		ret = 1;
 		goto out;
 	}
-	ret = ll_merge(&result, path, &base, &cur, "", &other, "", 0);
+	ret = ll_merge(&result, path, &base, NULL, &cur, "", &other, "", 0);
 	if (!ret) {
 		FILE *f = fopen(path, "w");
 		if (!f)

1.7.0

git checkout --conflict=diff3 can be used to present conflicts hunks including text from the common ancestor:

	<<<<<<< ours
	ourside
	|||||||
	original
	=======
	theirside
	>>>>>>> theirs

The added information can be very helpful for resolving a merge by hand, and merge tools can usually understand it without trouble because it looks like output from ‘diff3 -m’.

Unfortunately, not all can: ‘diff3’ includes a label for the merge base on the ||||||| line and it seems some tools cannot parse conflict hunks without such a label. Humans could use help in interpreting the output, too. So mark the start of the text from the common ancestor with the label “||||||| original”.

git rerere will not have trouble parsing this output, since instead of looking for a newline, it looks for whitespace after the ||||||| marker. No other code in git tries to parse conflict hunks.

---
Since “original” a good name for the common ancestor? I also considered “base” and “ancestor”; the latter is too jargon-y for my taste, but “base” seems all right.

 builtin/checkout.c |    2 +-
 t/t7201-co.sh      |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

index d67f809..c60c3e0 100644
@@ -149,7 +149,7 @@ static int checkout_merged(int pos, struct checkout *state)
 	read_mmblob(&ours, active_cache[pos+1]->sha1);
 	read_mmblob(&theirs, active_cache[pos+2]->sha1);
 
-	status = ll_merge(&result_buf, path, &ancestor, NULL,
+	status = ll_merge(&result_buf, path, &ancestor, "original",
 			  &ours, "ours", &theirs, "theirs", 0);
 	free(ancestor.ptr);
 	free(ours.ptr);

index d20ed61..ebfa5ca 100755
@@ -481,7 +481,7 @@ test_expect_success 'checkout with --merge, in diff3 -m style' '
 	(
 		echo "<<<<<<< ours"
 		echo ourside
-		echo "|||||||"
+		echo "||||||| original"
 		echo original
 		echo "======="
 		echo theirside

@@ -525,7 +525,7 @@ test_expect_success 'checkout --conflict=diff3' '
 	(
 		echo "<<<<<<< ours"
 		echo ourside
-		echo "|||||||"
+		echo "||||||| original"
 		echo original
 		echo "======="
 		echo theirside

1.7.0

Missing:

"... and adding the extra label will not affect the computed the conflict identifier, so existing rerere database will not be invalidated with this change either".

I didn't verified the above claim, but if it does not hold true, then we need to think the transition strategy. I don't expect a problem, though.

This "Reported" feels very odd for a feature enhancement ("Requested" would be more appropriate) not a bugfix.

Yeah, base sounds good. Even though at the lowest level, a merge is a merge between two equals, people tend to think of the contents of their own side "original" (vs the merge outcome "result").

It holds. handle_path() in rerere.c actually recreates the conflict hunk after parsing it:

	rerere_io_putconflict('<', marker_size, io);
	rerere_io_putmem(one.buf, one.len, io);
	rerere_io_putconflict('=', marker_size, io);
	rerere_io_putmem(two.buf, two.len, io);
	rerere_io_putconflict('>', marker_size, io);

This is a piece of defensive programing by Dscho that has lasted since rerere was first made builtin. It shelters rerere from changes in the format of the conflict markers.

Makes sense, thanks.

Sound good. “base” it is, then.

Thanks,
Jonathan

The merge_file() function is a helper for ‘git read-tree’, which does not respect the merge.conflictstyle option, so there is no need to worry about its ancestor_name parameter for ll_merge(). Add a comment to this effect.

---
There are more non-changes (e.g., in builtin/merge-tree.c), but none of them seemed nonobvious like this one.

 merge-file.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

index 07cc0c9..c336c93 100644
@@ -30,6 +30,12 @@ static void *three_way_filemerge(const char *path, mmfile_t *base, mmfile_t *our
 	int merge_status;
 	mmbuffer_t res;
 
+	/*
+	 * This function is only used by cmd_merge_tree, which
+	 * does not respect the merge.conflictstyle option.
+	 * There is no need to worry about a label for the
+	 * common ancestor.
+	 */
 	merge_status = ll_merge(&res, path, base, NULL,
 				our, ".our", their, ".their", 0);
 	if (merge_status < 0)

1.7.0

Commands using the merge_trees() machinery will present conflict hunks in output something like what ‘diff3 -m’ produces if the merge.conflictstyle configuration option is set to diff3.

Unfortunately, the format is not fully compatible yet. ‘diff3’ includes the name of the merge base on the ||||||| line of the output, and tools can misparse the conflict hunks without it. Add a new o->ancestor parameter to merge_trees() for use as a label for the ancestor in conflict hunks to give callers the power to rectify this situation.

If o->ancestor is NULL, the output format is as before. All callers pass NULL, so this patch does not affect the outward behavior of git.

If o->ancestor is non-NULL and both branches renamed the base file to the same name, that name is included in the conflict hunk labels. Even if o->ancestor is NULL I think this would be a good change, but this patch only does it in the non-NULL case to ensure the output format does not change where it might matter.

---
Another no-op.

 merge-recursive.c |   11 ++++++++---
 merge-recursive.h |    1 +
 2 files changed, 9 insertions(+), 3 deletions(-)

index 3b2cc9d..017cafd 100644
@@ -608,7 +608,7 @@ static int merge_3way(struct merge_options *o,
 		      const char *branch2)
 {
 	mmfile_t orig, src1, src2;
-	char *name1, *name2;
+	char *base_name, *name1, *name2;
 	int merge_status;
 	int favor;
 

@@ -628,10 +628,15 @@ static int merge_3way(struct merge_options *o,
 		}
 	}
 
-	if (strcmp(a->path, b->path)) {
+	if (strcmp(a->path, b->path) ||
+	    (o->ancestor != NULL && strcmp(a->path, one->path) != 0)) {
+		base_name = o->ancestor == NULL ? NULL :
+			xstrdup(mkpath("%s:%s", o->ancestor, one->path));
 		name1 = xstrdup(mkpath("%s:%s", branch1, a->path));
 		name2 = xstrdup(mkpath("%s:%s", branch2, b->path));
 	} else {
+		base_name = o->ancestor == NULL ? NULL :
+			xstrdup(mkpath("%s", o->ancestor));
 		name1 = xstrdup(mkpath("%s", branch1));
 		name2 = xstrdup(mkpath("%s", branch2));
 	}

@@ -640,7 +645,7 @@ static int merge_3way(struct merge_options *o,
 	read_mmblob(&src1, a->sha1);
 	read_mmblob(&src2, b->sha1);
 
-	merge_status = ll_merge(result_buf, a->path, &orig, NULL,
+	merge_status = ll_merge(result_buf, a->path, &orig, base_name,
 				&src1, name1, &src2, name2,
 				(!!o->call_depth) | (favor << 1));
 

index be8410a..d1192f5 100644
@@ -4,6 +4,7 @@
 #include "string-list.h"
 
 struct merge_options {
+	const char *ancestor;
 	const char *branch1;
 	const char *branch2;
 	enum {

1.7.0

We are about to change the format of the conflict hunks that ‘checkout --merge’ writes. Add tests checking the current behavior first.

---
 t/t7201-co.sh |   65 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 61 insertions(+), 4 deletions(-)

index ebfa5ca..970a460 100755
@@ -11,10 +11,12 @@ Test switching across them.
   ! [master] Initial A one, A two
    * [renamer] Renamer R one->uno, M two
     ! [side] Side M one, D two, A three
-  ---
-    + [side] Side M one, D two, A three
-   *  [renamer] Renamer R one->uno, M two
-  +*+ [master] Initial A one, A two
+     ! [simple] Simple D one, M two
+  ----
+     + [simple] Simple D one, M two
+    +  [side] Side M one, D two, A three
+   *   [renamer] Renamer R one->uno, M two
+  +*++ [master] Initial A one, A two
 
 '
 

@@ -52,6 +54,11 @@ test_expect_success setup '
 	git update-index --add --remove one two three &&
 	git commit -m "Side M one, D two, A three" &&
 
+	git checkout -b simple master &&
+	rm -f one &&
+	fill a c e > two &&
+	git commit -a -m "Simple D one, M two" &&
+
 	git checkout master
 '
 

@@ -166,6 +173,56 @@ test_expect_success 'checkout -m with merge conflict' '
 	! test -s current
 '
 
+test_expect_success 'format of merge conflict from checkout -m' '
+
+	git checkout -f master && git clean -f &&
+
+	fill b d > two &&
+	git checkout -m simple &&
+
+	git ls-files >current &&
+	fill same two two two >expect &&
+	test_cmp current expect &&
+
+	cat <<-EOF >expect &&
+	<<<<<<< simple
+	a
+	c
+	e
+	=======
+	b
+	d
+	>>>>>>> local
+	EOF
+	test_cmp two expect
+'
+
+test_expect_success 'checkout --merge --conflict=diff3 <branch>' '
+
+	git checkout -f master && git reset --hard && git clean -f &&
+
+	fill b d > two &&
+	git checkout --merge --conflict=diff3 simple &&
+
+	cat <<-EOF >expect &&
+	<<<<<<< simple
+	a
+	c
+	e
+	|||||||
+	a
+	b
+	c
+	d
+	e
+	=======
+	b
+	d
+	>>>>>>> local
+	EOF
+	test_cmp two expect
+'
+
 test_expect_success 'checkout to detach HEAD (with advice declined)' '
 
 	git config advice.detachedHead false &&

1.7.0

git checkout --merge --conflict=diff3 can be used to present conflicts hunks including text from the common ancestor.

The added information can be very helpful for resolving a merge by hand, and many merge tools can usually grok it because it is very similar to the output from ‘diff3 -m’.

Unfortunately, some tools reportedly cannot parse the conflict hunks because of a small difference from diff3: diff3 -m includes a label for the merge base on the ||||||| line. Humans can benefit from a cue when learning to interpreting the format, too. So mark the start of the text from the old branch with a label based on the branch’s name.

git rerere will not have trouble parsing this output, since instead of looking for a newline, it looks for whitespace after the ||||||| marker. No other code in git tries to parse conflict hunks.

---
 builtin/checkout.c |    1 +
 t/t7201-co.sh      |    2 +-
 2 files changed, 2 insertions(+), 1 deletions(-)

index c60c3e0..ebc5891 100644
@@ -439,6 +439,7 @@ static int merge_working_tree(struct checkout_opts *opts,
 			ret = reset_tree(new->commit->tree, opts, 1);
 			if (ret)
 				return ret;
+			o.ancestor = old->name;
 			o.branch1 = new->name;
 			o.branch2 = "local";
 			merge_trees(&o, new->commit->tree, work,

index 970a460..ae14b19 100755
@@ -209,7 +209,7 @@ test_expect_success 'checkout --merge --conflict=diff3 <branch>' '
 	a
 	c
 	e
-	|||||||
+	||||||| master
 	a
 	b
 	c

1.7.0

git cherry-pick and git revert will present conflict hunks in output something like what ‘diff3 -m’ produces if the merge.conflictstyle configuration option is set to diff3.

Unfortunately, some tools reportedly cannot parse the conflict hunks because of a small difference from diff3: diff3 -m includes a label for the merge base on the ||||||| line. Humans parsing the conflict hunks by hand might want to know what the common ancestor represents, too. So mark the start of the text from the parent of the commit being cherrypicked (resp. from the commit being reverted) with the label "parent".

It would be nicer to use a more informative label, and that might happen in the future.

git rerere will not have trouble parsing this output, since instead of looking for a newline, it looks for whitespace after the ||||||| marker. No other code in git tries to parse conflict hunks.

---
In the revert case, this gives

<<<<<<< HEAD
foo
||||||| parent
bar
=======
baz

This is a bit misleading, since actually “baz” is from the commit to revert’s parent and “bar” is from the commit to revert.

Probably the labels should be switched. I didn’t do this because it is late and it would have been a larger change from the current behavior.

It also would be nice to suppress the ancestor and do a two-way merge when cherry-picking a root commit. I haven’t yet looked into how hard this would be.

 builtin/revert.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

index eff5268..a61dd9a 100644
@@ -372,6 +372,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 
 	read_cache();
 	init_merge_options(&o);
+	o.ancestor = base ? "parent" : "(empty tree)";
 	o.branch1 = "HEAD";
 	o.branch2 = oneline;
 

1.7.0

git merge-recursive (and hence git merge) will present conflict hunks in output something like what ‘diff3 -m’ produces if the merge.conflictstyle configuration option is set to diff3.

Unfortunately, some tools reportedly cannot parse the conflict hunks because of a small difference from diff3: diff3 -m includes a label for the merge base on the ||||||| line. Humans unfamiliar with the format would do a better job with a label, too. Mark the start of the text from the merge bases with the heading "||||||| merged common ancestors".

It would be nicer to use a more informative label, and that might happen in the future.

git rerere does not have trouble parsing this output, since instead of looking for a newline, it looks for whitespace after the ||||||| marker. No other code in git tries to parse conflict hunks.

---
I would be interested to know which merge tool chokes on |||||||. ;) As it is, since I haven’t experienced the mechanical misbehavior, I am targetting humans.

That’s the end of the series. Thanks for reading.

Good night,
Jonathan

 merge-recursive.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

index 017cafd..917397c 100644
@@ -1347,6 +1347,7 @@ int merge_recursive(struct merge_options *o,
 	if (!o->call_depth)
 		read_cache();
 
+	o->ancestor = "merged common ancestors";
 	clean = merge_trees(o, h1->tree, h2->tree, merged_common_ancestors->tree,
 			    &mrtree);
 

1.7.0

Even if there weren't any broken tools, I think targetting humans is a good thing to do, as long as we don't break ourselves (including old rerere database).

Thanks for writing. I think this is generally a good thing to have, and from my cursory look I didn't see anything glaringly wrong except for the one I already commented on the first patch.

In my case, it was smerge-mode.el (a minor mode included in Emacs since Emacs-22, which highlights such conflicts and tries to help you resolve them).

Stefan

PS: As you may have guessed, I'm the primary author of this Emacs package.

rerere needs an isspace() after the specified marker length. So I assume it could live with extra cruft after the | marker and a space.

BTW: Am I right, that rerere would need to handle my new conflict style too?

Bert