Re: two-way merge corner case bug

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 1, 2013, 16:57 UTC
Message-ID: <7va9qngisg.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20130301092201.GA17254@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 5 quoted lines

> 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?

An initial checkout is *supposed* to happen in an empty working tree, so if we code it not to overwrite an existing path in the working tree, the user cannot lose possibly precious contents with an mistaken initial checkout (they will instead appear as modified relative to the index), while in the normal case we will write out the contents from the HEAD through the index. We could attempt "we do not have to if the user behaves, but with this we could help misbehaving users" if we used twoway merge for an initial checkout.

Having said that, I notice that in the normal codepath (e.g. "git clone" without the "--no-checkout" option) we no longer use twoway merge for the initial checkout. Back when "git clone" was a scripted Porcelain, I think we used to do a twoway read-tree. It may be that we broke it when "clone" was rewritten in C, but the breakage is to the "we do not have to..." thing, so it may not be a big deal.

The only case that matters in today's code is "git checkout" (without any option or argument) immediately after "git clone -n", I think. The special casing for this initial checkout in twoway merge is needed because we go from HEAD to HEAD in that case, and we do not want to keep the artificial local removals from the index; we start from not even having the $GIT_INDEX_FILE, so without the special case all paths appear to have been "rm --cached", which is usually not what the user would want to see ;-)

> ... 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).

I've checked all in-tree uses of "read-tree --reset -u".

Nobody seems to use that combination, either from scripts or from C (i.e. when opts.update==1 and opts.merge==1, opts.reset is not set) with a twoway merge, other than "git am --abort/--skip".