Re: [PATCH 6/8] apply: fall back on three-way merge

From: Junio C Hamano <gitster@pobox.com>
Date: May 10, 2012, 21:06 UTC
Message-ID: <7vaa1fn3u8.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20120510203113.GB18276@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 11 quoted lines

> On Wed, May 09, 2012 at 11:02:23PM -0700, Junio C Hamano wrote:
> ...
>> +	/* No point falling back to 3-way merge in these cases */
>> +	if (patch->is_binary || patch->is_new || patch->is_delete ||
>> +	    S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode))
>> +		return -1;
>
> Is it true that there is no point in doing a 3-way fallback when
> patch->is_binary? What if the user has a custom merge driver?
> ...
> It seems like we should just keep the logic here as stupid as possible,

Actually, the motivation behind that "No point" is to keep the logic as stupid as possible.

We had a history of setting up logic that covers cases far wider than we originally designed the behaviours for, leaving the corner case behaviour of the code "undefined", without saying exactly what is and what is not the defined use case, which lead users to try unexpected (possibly but not necessarily stupid) things and getting a random "you get whatever the code happens to do" results. The above is an attempt to limit the scope of the initial implementation to a narrow, well defined, and commonly occurring subset of the problem space.

I can buy that lifting "must not be binary" would be very cheap, but adding support for new/delete case would mean the index-stuffing part needs to gain more code, so removing the "not new, not delete" conditionals actually goes against keeping the logic as stupid as possible.