Re: [PATCH v3 3/3] git-merge-one-file: revise merge error reporting

From: Kevin Bracey <kevin@bracey.fi>
Date: Mar 14, 2013, 06:27 UTC
Message-ID: <51416DD5.2030805@bracey.fi>
In-Reply-To: <7vehfj2neh.fsf@alter.siamese.dyndns.org>

On 13/03/2013 19:57, Junio C Hamano wrote:

Show 17 quoted lines

> Kevin Bracey <kevin@bracey.fi> writes:
>
>> -		echo "Added $4 in both, but differently."
>> +		echo "ERROR: Added $4 in both, but differently."
>> +		ret=1
> The problem you identified may be worth fixing, but I do not think
> this change is correct.
>
> This message is at the same severity level as the message on the
> other arm of this case that says "Auto-merging $4".  In that other
> case arm, we are attempting a true three-way merge, and in this case
> arm, we are attempting a similar three-way merge using your "virtual
> base".
>
> Neither has found any error in this case arm yet.  The messages are
> both "informational", not an error.  I do not think you would want
> to set ret=1 until you see content conflict.

I disagree here. At the minute, it does set ret to 1 (but further down the code - bringing it up here next to the "ERROR" print clarifies that), and will report the merge as failed, conflict in the 3-way merge or not. Which I think is correct.

We have to stop for user inspection here. We do have a fake base; we can't trust the 3-way merge with it.

The virtual 3-way merge will take ABCDE and ABDE and produce ABCDE without conflict. That's flat wrong if the real base they failed to tell Git about was ABCDE.

Despite being useful, I'm still slightly uncomfortable that it can produce something without any conflict markers. The user really needs to look at properly.

(And one interesting related glitch, or at least thing that puzzled me when it happened. This is from memory, so may be slightly mistaken, but what seemed to happen was that if you have rerere enabled, then mergetool tends to say "nothing to merge", because it relies on "rerere remaining", which relies on conflict markers. I think you could still force a mergetool up by specifying the specific file though.)

Maybe the virtual base itself should be different. Maybe it should put a ??????? marker in place of every unique line. So you get:

Left ABCEFGH Right XABCDEFJH -> Merge result <|X>ABC<|D>EF<G|J>H VBase ?ABC?EF??H

That actually feels like it may be the correct answer here. And it's effectively what P4Merge does in its "2-way" mode I failed to invoke. (At least for the result view).

Kevin