Re: [BUG?] git checkout $commit -- somedir doesn't drop files

From: Jeff King <peff@peff.net>
Date: Sep 19, 2013, 22:13 UTC
Message-ID: <20130919221323.GA18085@sigill.intra.peff.net>
In-Reply-To: <xmqqbo3ou1ue.fsf@gitster.dls.corp.google.com>

On Thu, Sep 19, 2013 at 11:02:17AM -0700, Junio C Hamano wrote:

Show 10 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > An alternative would be to write the new entries to a temporary index
> > in memory. And then you can throw away the entries in the current index
> > that match the pathspec, and add in the entries from the temporary
> > index. I was just hoping that unpack-trees would do all of that for me.
> > :)
> 
> Isn't a "go there" one-way merge with pathspecs very similar to what
> happens in "reset -- pathspec" except for the working tree part?

I thought so at first, but now I'm not so sure...

Show 5 quoted lines

> suspect that it may be sufficient to mimic the read_from_tree() and
> adapt update_index_from_diff() callback in builtin/reset.c to also
> update the working tree (and we can do so unconditionally without
> checking if we have any local modification in this case, which
> simplifies things a lot), but I may be missing something.

It almost works to simply update the index as "reset" does via diff_cache, marking each updated entry with CE_UPDATE, and then let the rest of checkout_paths proceed, which handles updating the working tree based on the new index.

But I found two problems:

  1. The diff never feeds us entries that are unchanged, so we never
     mark them for update. But that interferes with later code to check
     whether our pathspecs matched anything (so we complain on "git
     reset --hard; git checkout HEAD foo" would barf on the checkout,
     since we do not need to touch foo).

     But I think that points to a larger problem, which is that we do
     not want to just look at the entries that are different between the
     tree and the index. We also need to care about the entries that are
     different in the working tree and index, because those need written
     out, too.

  2. The code in checkout_paths cannot handle the deletion for us,
     because it doesn't even know about the path any more (we removed it
     during the index diff). I think we could get around this by leaving
     an entry with the CE_WT_REMOVE flag set. But it looks like there is
     a bit more magic to removing a path than just remove_or_warn().
     unpack-trees has unlink_entry, which queues up leading directories
     for removal.

I think (2) is a matter of refactoring (but again, if we could convince unpack-trees to do this for us, that might be the nicest way to reuse the code). But (1) points to a larger problem in thinking about this as a "diff"; it is really about re-loading bits of the index, and then checking it out into the working tree.

-Peff