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

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

[Jonathan Nieder]

Hi gitsters,

Stefan Monnier wrote [1]:

> I can't live without conflictstyle=diff3m and I'm very happy it exists.
> But it has a little problem: it uses "|||||||\n" as a separator for the
> ancestor version of the text, whereas diff3 uses "||||||| <ancestorname>\n".
> The difference is harmless for a human (tho the <ancestorname> can sometimes
> be useful, assuming it's meaningful), but it makes some tools fail to
> recognize the conflict markers properly.
> So please add a " BASE" or " ANCESTOR" after the "|||||||".

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.

>
> 	Stefan
>
> PS: I intended to report this bug directly to the Git bug-tracker, but I was
> simply unable to find it.  I know I'm sometimes dense, but I did make an
> honest effort to look for it.

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

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>> I can't live without conflictstyle=diff3m and I'm very happy it exists.
>> But it has a little problem: it uses "|||||||\n" as a separator for the
>> ancestor version of the text, whereas diff3 uses "||||||| <ancestorname>\n".
>> The difference is harmless for a human (tho the <ancestorname> can sometimes
>> be useful, assuming it's meaningful), but it makes some tools fail to
>> recognize the conflict markers properly.
>> So please add a " BASE" or " ANCESTOR" after the "|||||||".
>
> 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.

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

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

[Bert Wesarg]

On Fri, Mar 5, 2010 at 23:31, Junio C Hamano <gitster@pobox.com> wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>>> I can't live without conflictstyle=diff3m and I'm very happy it exists.
>>> But it has a little problem: it uses "|||||||\n" as a separator for the
>>> ancestor version of the text, whereas diff3 uses "||||||| <ancestorname>\n".
>>> The difference is harmless for a human (tho the <ancestorname> can sometimes
>>> be useful, assuming it's meaningful), but it makes some tools fail to
>>> recognize the conflict markers properly.
>>> So please add a " BASE" or " ANCESTOR" after the "|||||||".
>>
>> 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.
>
> 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).

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

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

[Junio C Hamano]

Bert Wesarg <bert.wesarg@googlemail.com> writes:

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

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.

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

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

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

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> I’d be interested to hear from xdiff people
>> whether it should be easy to add the ancestor name to the output.
>
> 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.
 >>>>>>> Temporary merge branch 2
 =======
 Git makes conflict resolution easy.
 >>>>>>> topic

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

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

Jonathan

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

[PATCH/RFC 0/10] Add label for common ancestor to conflictstyle=diff3

[Jonathan Nieder]

Jonathan Nieder wrote:
> Stefan Monnier wrote [1]:

>> I can't live without conflictstyle=diff3m and I'm very happy it exists.
>> But it has a little problem: it uses "|||||||\n" as a separator for the
>> ancestor version of the text, whereas diff3 uses "||||||| <ancestorname>\n".
>> The difference is harmless for a human (tho the <ancestorname> can sometimes
>> be useful, assuming it's meaningful), but it makes some tools fail to
>> recognize the conflict markers properly.
>> So please add a " BASE" or " ANCESTOR" after the "|||||||".
>
> No opinion on this myself.

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

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

[PATCH 01/10] xdl_merge(): add optional ancestor label to diff3-style output

[Jonathan Nieder]

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.

Reported-by: Stefan Monnier <monnier@iro.umontreal.ca>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 xdiff/xdiff.h  |    1 +
 xdiff/xmerge.c |   16 ++++++++++++++--
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index 3f6229e..abaf960 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -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
diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
index 8cbe45e..3a0ae14 100644
--- a/xdiff/xmerge.c
+++ b/xdiff/xmerge.c
@@ -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

[PATCH 02/10] merge-file --diff3: add a label for ancestor

[Jonathan Nieder]

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.

Reported-by: Stefan Monnier <monnier@iro.umontreal.ca>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
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(-)

diff --git a/builtin/merge-file.c b/builtin/merge-file.c
index 1e70073..0ff8b03 100644
--- a/builtin/merge-file.c
+++ b/builtin/merge-file.c
@@ -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);
 
diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh
index d605024..16c3c69 100755
--- a/t/t6023-merge-file.sh
+++ b/t/t6023-merge-file.sh
@@ -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

[PATCH 03/10] ll_merge(): add ancestor label parameter for diff3-style output

[Jonathan Nieder]

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.

Reported-by: Stefan Monnier <monnier@iro.umontreal.ca>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
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(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index acefaaf..d67f809 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -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);
diff --git a/ll-merge.c b/ll-merge.c
index 4c7f11b..68fa1e6 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -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);
 }
diff --git a/ll-merge.h b/ll-merge.h
index 5788922..57754cc 100644
--- a/ll-merge.h
+++ b/ll-merge.h
@@ -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);
diff --git a/merge-file.c b/merge-file.c
index fd34d76..07cc0c9 100644
--- a/merge-file.c
+++ b/merge-file.c
@@ -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;
diff --git a/merge-recursive.c b/merge-recursive.c
index 195ebf9..3b2cc9d 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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));
 
diff --git a/rerere.c b/rerere.c
index a59f74f..f221bed 100644
--- a/rerere.c
+++ b/rerere.c
@@ -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

[PATCH 04/10] checkout --conflict=diff3: add a label for ancestor

[Jonathan Nieder]

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.

Reported-by: Stefan Monnier <monnier@iro.umontreal.ca>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
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(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index d67f809..c60c3e0 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -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);
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index d20ed61..ebfa5ca 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -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

[PATCH 05/10] merge_file(): add comment explaining behavior wrt conflict style

[Jonathan Nieder]

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.

Signed-off-by: Jonathan Nieder <jrnieder@mgila.com>
---
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(-)

diff --git a/merge-file.c b/merge-file.c
index 07cc0c9..c336c93 100644
--- a/merge-file.c
+++ b/merge-file.c
@@ -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

[PATCH 06/10] merge_trees(): add ancestor label parameter for diff3-style output

[Jonathan Nieder]

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.

Reported-by: Stefan Monnier <monnier@iro.umontreal.ca>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Another no-op.

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

diff --git a/merge-recursive.c b/merge-recursive.c
index 3b2cc9d..017cafd 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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));
 
diff --git a/merge-recursive.h b/merge-recursive.h
index be8410a..d1192f5 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -4,6 +4,7 @@
 #include "string-list.h"
 
 struct merge_options {
+	const char *ancestor;
 	const char *branch1;
 	const char *branch2;
 	enum {
-- 
1.7.0

[PATCH 07/10] tests: document format of conflicts from checkout -m

[Jonathan Nieder]

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

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t7201-co.sh |   65 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 61 insertions(+), 4 deletions(-)

diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index ebfa5ca..970a460 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -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

[PATCH 08/10] checkout -m --conflict=diff3: add a label for ancestor

[Jonathan Nieder]

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.

Reported-by: Stefan Monnier <monnier@iro.umontreal.ca>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/checkout.c |    1 +
 t/t7201-co.sh      |    2 +-
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index c60c3e0..ebc5891 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -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,
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index 970a460..ae14b19 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -209,7 +209,7 @@ test_expect_success 'checkout --merge --conflict=diff3 <branch>' '
 	a
 	c
 	e
-	|||||||
+	||||||| master
 	a
 	b
 	c
-- 
1.7.0

[PATCH 09/10] cherry-pick: add a label for ancestor

[Jonathan Nieder]

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.

Reported-by: Stefan Monnier <monnier@iro.umontreal.ca>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
In the revert case, this gives

 <<<<<<< HEAD
 foo
 ||||||| parent
 bar
 =======
 baz
 >>>>>>> ab78c93 "Make some change we would like to revert"

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

diff --git a/builtin/revert.c b/builtin/revert.c
index eff5268..a61dd9a 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -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

[PATCH 10/10] merge-recursive: add a label for ancestor

[Jonathan Nieder]

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.

Reported-by: Stefan Monnier <monnier@iro.umontreal.ca>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
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(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 017cafd..917397c 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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

Re: [PATCH 01/10] xdl_merge(): add optional ancestor label to diff3-style output

[Junio C Hamano]

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?

Re: [PATCH 04/10] checkout --conflict=diff3: add a label for ancestor

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

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

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.

> Reported-by: Stefan Monnier <monnier@iro.umontreal.ca>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

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

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

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

Re: [PATCH 10/10] merge-recursive: add a label for ancestor

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

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

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

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

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.

Re: [PATCH 04/10] checkout --conflict=diff3: add a label for ancestor

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> git rerere will not have trouble parsing this output, since instead of
>> looking for a newline, it looks for whitespace after the |||||||
>> marker.
>
> 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.

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.

>> Reported-by: Stefan Monnier <monnier@iro.umontreal.ca>
>> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
>
> This "Reported" feels very odd for a feature enhancement ("Requested"
> would be more appropriate) not a bugfix.

Makes sense, thanks.

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

Sound good.  “base” it is, then.

Thanks,
Jonathan

Re: [PATCH 01/10] xdl_merge(): add optional ancestor label to diff3-style output

[Jonathan Nieder]

Junio C Hamano wrote:

> 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

Re: [PATCH 01/10] xdl_merge(): add optional ancestor label to diff3-style output

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Junio C Hamano wrote:
>
>> 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.

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

Re: [PATCH 10/10] merge-recursive: add a label for ancestor

[Stefan Monnier]

> I would be interested to know which merge tool chokes on |||||||. ;)

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.