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

Jonathan Nieder [off-list ref] 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 <redacted>
Signed-off-by: Jonathan Nieder <redacted>

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

Junio C Hamano wrote:

Jonathan Nieder [off-list ref] writes:

quoted

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.

quoted

Reported-by: Stefan Monnier <redacted>
Signed-off-by: Jonathan Nieder <redacted>

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

Makes sense, thanks.

quoted

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