Re: [PATCH v2] pull: merge into unborn by fast-forwarding from empty tree

From: Jeff King <peff@peff.net>
Date: Jun 20, 2013, 13:47 UTC
Message-ID: <20130620134726.GB18200@sigill.intra.peff.net>
In-Reply-To: <87mwqk6h5d.fsf@linux-k42r.v.cablecom.net>

On Thu, Jun 20, 2013 at 03:33:34PM +0200, Thomas Rast wrote:

Show 11 quoted lines

> >> I naively would have expected this to leave us in a conflicted state
> >> over "file".  But I guess read-tree just rejects it, because we are not
> >> doing a real three-way merge.  I'm not sure it is that big a deal; this
> >> is more about safety than about creating a conflicted/resolvable state.
> >
> > Note that the test_must_fail essentially tests that the merge is rejected.
> 
> Bah, no it doesn't, a conflicting merge also returns a nonzero status.
> Sigh.
> 
> If you meant we should actually conflict,

Yes, that's what I meant.

> I'm not sure what options there would be other than actually calling a
> merge driver.  And we could actually do this like so (it'll obviously
> break the tests):

I'd rather not invoke a merge driver directly if we can avoid it. I think you could get rid of this special code-path entirely if you just lied to the "git-merge" and said "the ancestor and current tree are fake commits with an empty tree", and then followed the usual path. But that lying through git-merge is ugly and complicated (and is more or less what you're doing with the merge-recursive patch here).

Show 10 quoted lines

> diff --git i/git-pull.sh w/git-pull.sh
> index 1f84383..b3d36a8 100755
> --- i/git-pull.sh
> +++ w/git-pull.sh
> @@ -276,7 +276,7 @@ then
>  	# lose index/worktree changes that the user already made on
>  	# the unborn branch.
>  	empty_tree=4b825dc642cb6eb9a060e54bf8d69288fbee4904
> -	git read-tree -m -u $empty_tree HEAD || exit 1
> +	git merge-recursive $empty_tree -- $(git write-tree) HEAD || exit 1

I don't think there is any advantage to using merge-recursive over read-tree here, in the sense that there cannot be any interesting content-level merging going on (our ancestor is the empty tree, so we know that differing content cannot be resolved).

So I think you could just use read-tree with a 3-way merge, but I cannot seem to provoke it to leave a conflict. Hrm.

I also noticed that the procedure in this code path is:

  1. Update HEAD with the fetched ref.

  2. Checkout the contents with read-tree.

I wonder if it would make sense to update HEAD only _after_ we had resolved successfully. As it is now, you are left in a weird state where pull has reported failure, but we actually update the HEAD (and "git status" afterwards reflects that you are building on top of the pulled HEAD).

-Peff