Re: two-way merge corner case bug

From: Jeff King <peff@peff.net>
Date: Mar 1, 2013, 09:22 UTC
Message-ID: <20130301092201.GA17254@sigill.intra.peff.net>
In-Reply-To: <7vwqtulplp.fsf@alter.siamese.dyndns.org>

On Tue, Feb 26, 2013 at 01:41:54PM -0800, Junio C Hamano wrote:

Show 9 quoted lines

> > Isn't this a repeat of:
> >
> >   http://thread.gmane.org/gmane.comp.version-control.git/202440/focus=212316
> [...]
> 
> Yeah, I think the patch in your message is a good starting point to
> solve a half of the issue (i.e. unmerged current one should be
> replaced with the version from the "going to" tree).  Attached is
> with a slight update.

Thanks for picking this up. I'm not sure, though, about the trigger here:

Show 8 quoted lines

> +		if (current->ce_flags & CE_CONFLICTED) {
> +			if (same(oldtree, newtree) || o->reset) {
> +				if (!newtree)
> +					return deleted_entry(current, current, o);
> +				else
> +					return merged_entry(newtree, current, o);
> +			}
> +			return o->gently ? -1 : reject_merge(current, o);

We have a conflicted state in the index for a path. If o->reset is true, then we want to throw it out in favor of where we are moving to. So that part makes sense.

But if we are not in "--reset", and we see that oldtree and newtree are the same, do we still want to move to newtree? It seems like we are throwing away the conflicted state that the user has. Usually this isn't a big deal, but with "-u" it could mean losing any in-progress resolution in the working tree.

I think this generally shouldn't happen, as "-m" does not operate when the index contains unmerged entries. So it would really just be a safety valve for somebody calling into unpack_trees without having done the index check. IOW, should the same(newtree, oldtree) call be cut there, and we trigger this just on reset? Any other case would be rejected (which _shouldn't_ happen, but this keeps us conservative, and if we find a case where it does happen, we can figure out the semantics then).

Show 9 quoted lines

> The other half that is not solved by this patch is that we will lose
> Z because merge-recursive removed it when it renamed it and left
> three stages for A in the index, and to "read-tree --reset -u HEAD
> ORIG_HEAD", it looks as if you removed Z yourself while on HEAD and
> want to carry your local change forward to ORIG_HEAD.
>
> I think this too needs to be fixed.  We are saying "-u --reset" to
> mean "I want to go there no matter what", not "-u -m" that means "I
> am carrying my changes forward while checking out another branch".

Yeah, hmm. Since we cannot distinguish between such a staged removal and what merge-recursive deposits in the index, we have to err on on side (either losing a staged removal, or failing to move to the new state). What we really want is to detect that Z's removal is related to the conflicted path A, but we can't do so; that information was never put into the index in the first place.

We could do this:

diff --git a/unpack-trees.c b/unpack-trees.c
index 3d17108..1e84131 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1788,7 +1788,7 @@ int twoway_merge(struct cache_entry **src, struct unpack_trees_options *o)
 		}
 	}
 	else if (newtree) {
-		if (oldtree && !o->initial_checkout) {
+		if (oldtree && !o->initial_checkout && !o->reset) {
 			/*
 			 * deletion of the path was staged;
 			 */

which solves the case you presented. My worry would be that somebody is
using "--reset" but expecting the removal to be carried through
(technically, "--reset" is documented as "-m but discard unmerged
entries", but we are not really treating it that way here). The test
suite passes, but we may not be covering some oddball case.

-Peff

PS I wonder if the "initial_checkout" hack can just go away under this
   new rule. Although we don't seem to use "o->reset" for the initial
   checkout. Which seems kind of weird. I would think the initial
   checkout would actually just be a oneway merge. Is the point to try
   to leave any existing working tree contents untouched or something?