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

From: Kevin Bracey <kevin@bracey.fi>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, David Aguilar <davvid@gmail.com>,
	Ciaran Jessup <ciaranj@gmail.com>,
	Scott Chacon <schacon@gmail.com>,
	Alex Riesen <raa.lkml@gmail.com>
Subject: Re: [PATCH v3 3/3] git-merge-one-file: revise merge error reporting
Date: Thu, 14 Mar 2013 08:27:33 +0200	[thread overview]
Message-ID: <51416DD5.2030805@bracey.fi> (raw)
In-Reply-To: <7vehfj2neh.fsf@alter.siamese.dyndns.org>

On 13/03/2013 19:57, Junio C Hamano wrote:
> 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