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

Jonathan Nieder [off-list ref] writes:

quoted

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

Bert Wesarg [off-list ref] 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).

Junio C Hamano wrote:

Jonathan Nieder [off-list ref] writes:

quoted

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

On Fri, Mar 5, 2010 at 23:31, Junio C Hamano [off-list ref] wrote:

Jonathan Nieder [off-list ref] writes:

quoted

quoted

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