Re: [PATCH 2/5] Treat D/F conflict entry more carefully in unpack-trees.c::threeway_merge()

From: Daniel Barkalow <barkalow@iabervon.org>
Date: Apr 7, 2007, 19:08 UTC
Message-ID: <Pine.LNX.4.64.0704071456470.27922@iabervon.org>
In-Reply-To: <7vabxkcknp.fsf@assigned-by-dhcp.cox.net>

On Sat, 7 Apr 2007, Junio C Hamano wrote:

Show 10 quoted lines

> This fixes three buglets in threeway_merge() regarding D/F
> conflict entries.
> 
> * After finishing with path D and handling path D/F, some stages
>   have D/F conflict entry which are obviously non-NULL.  For the
>   purpose of determining if the path D/F is missing in the
>   ancestor, they should not be taken into account.
> 
> * D/F conflict entry is a phony entry and does not record the
>   path being processed, so do not pick up the name from there.

This bit is unnecessary, because the first bit means we treat D/F conflict as missing in that conditional, and don't count it as an entry at all, let alone one with a useful name.

Show 6 quoted lines

> * D/F conflict entry is a marker to say "this stage does _not_
>   have the path", so do not send them to keep_entry().
> 
> There might be more glitches, but I am slowly digging this mess
> through, which unfortunately was made even more work since
> merge-recursive is a built-in now.

Looks good, although it might be wise to add an "exists" function that returns false for df_conflict_entry and for NULL, to make the tests clearer, and to get a comment to point out the meaning of df_conflict_entry. Also, I think df_conflict_entry can be static bss in unpack-trees and not accessed through o->df_conflict_entry, since it's always the same value (being now universally initialized to a static pointer to heap...).

	-Daniel
*This .sig left intentionally blank*