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

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

[Uwe Kleine-König]

Hello,

after these commands:

	$ git init
	$ mkdir subdir
	$ echo a > subdir/a
	$ git add subdir/a
	$ git commit -m a
	$ echo more a >> subdir/a
	$ echo b > subdir/b
	$ git add subdir/*
	$ git commit -m b
	$ git checkout HEAD^ -- subdir

I'd expect that subdir/b is removed from the index as this file didn't
exist in HEAD^ but git-status only reports:

	# On branch master
	# Changes to be committed:
	#   (use "git reset HEAD <file>..." to unstage)
	#
	#	modified:   subdir/a
	#

Is this intended?

(I'm using git 1.8.4.rc3 as shipped by Debian (package version
1:1.8.4~rc3-1).)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Junio C Hamano]

Uwe Kleine-König  <u.kleine-koenig@pengutronix.de> writes:

> after these commands:
>
> 	$ git init
> 	$ mkdir subdir
> 	$ echo a > subdir/a
> 	$ git add subdir/a
> 	$ git commit -m a
> 	$ echo more a >> subdir/a
> 	$ echo b > subdir/b
> 	$ git add subdir/*
> 	$ git commit -m b
> 	$ git checkout HEAD^ -- subdir
>
> I'd expect that subdir/b is removed from the index as this file didn't
> exist in HEAD^ but git-status only reports:
>
> 	# On branch master
> 	# Changes to be committed:
> 	#   (use "git reset HEAD <file>..." to unstage)
> 	#
> 	#	modified:   subdir/a
> 	#
>
> Is this intended?
>
> (I'm using git 1.8.4.rc3 as shipped by Debian (package version
> 1:1.8.4~rc3-1).)

The intended behaviour of "git checkout tree-ish -- pathspec" has
always been "grab paths that match pathspec from tree-ish, add them
to the index and check them out to the working tree".  If you have
subdir/b in the index and the working tree, it will be kept when the
paths that match "subdir" pathspec is grabbed out of the tree-ish
HEAD^ (namely, subdir/a) is added to the index and to the working
tree.

I could argue that the above intended behaviour is suboptimal and it
should have been "the resulting paths in the index and the work tree
that match the given pathspec must be identical to that of the
tree-ish".  In the resulting index or working tree, paths that match
"subdir" pathspec in the above result is subdir/a and subdir/b, and
that is different from what exists in the given tree-ish (which has
only subdir/a and not subdir/b), and under that modified definition,
what the current one does is not correct.

I vaguely recall arguing for the updated behaviour described in the
above paragraph, and I even might have started working on it, but I
do not think we changed this behaviour recently, unfortunately.

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

[Jeff King]

On Tue, Sep 17, 2013 at 12:58:07PM -0700, Junio C Hamano wrote:

> I could argue that the above intended behaviour is suboptimal and it
> should have been "the resulting paths in the index and the work tree
> that match the given pathspec must be identical to that of the
> tree-ish".  In the resulting index or working tree, paths that match
> "subdir" pathspec in the above result is subdir/a and subdir/b, and
> that is different from what exists in the given tree-ish (which has
> only subdir/a and not subdir/b), and under that modified definition,
> what the current one does is not correct.

Our emails just crossed, but I basically ended up saying a similar
thing.  Could we simply replace the "update_some" in builtin/checkout.c
with a two-way merge via unpack-trees?

> I vaguely recall arguing for the updated behaviour described in the
> above paragraph, and I even might have started working on it, but I
> do not think we changed this behaviour recently, unfortunately.

Yes, I did some digging and I think it has always been this way, even
before git-checkout was a builtin.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Sep 17, 2013 at 12:58:07PM -0700, Junio C Hamano wrote:
>
>> I could argue that the above intended behaviour is suboptimal and it
>> should have been "the resulting paths in the index and the work tree
>> that match the given pathspec must be identical to that of the
>> tree-ish".  In the resulting index or working tree, paths that match
>> "subdir" pathspec in the above result is subdir/a and subdir/b, and
>> that is different from what exists in the given tree-ish (which has
>> only subdir/a and not subdir/b), and under that modified definition,
>> what the current one does is not correct.
>
> Our emails just crossed, but I basically ended up saying a similar
> thing.  Could we simply replace the "update_some" in builtin/checkout.c
> with a two-way merge via unpack-trees?

Would it work to resolve a conflicted index by checking out from a
known tree?

>> I vaguely recall arguing for the updated behaviour described in the
>> above paragraph, and I even might have started working on it, but I
>> do not think we changed this behaviour recently, unfortunately.
>
> Yes, I did some digging and I think it has always been this way, even
> before git-checkout was a builtin.
>
> -Peff

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

[Jeff King]

On Tue, Sep 17, 2013 at 01:27:08PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Tue, Sep 17, 2013 at 12:58:07PM -0700, Junio C Hamano wrote:
> >
> >> I could argue that the above intended behaviour is suboptimal and it
> >> should have been "the resulting paths in the index and the work tree
> >> that match the given pathspec must be identical to that of the
> >> tree-ish".  In the resulting index or working tree, paths that match
> >> "subdir" pathspec in the above result is subdir/a and subdir/b, and
> >> that is different from what exists in the given tree-ish (which has
> >> only subdir/a and not subdir/b), and under that modified definition,
> >> what the current one does is not correct.
> >
> > Our emails just crossed, but I basically ended up saying a similar
> > thing.  Could we simply replace the "update_some" in builtin/checkout.c
> > with a two-way merge via unpack-trees?
> 
> Would it work to resolve a conflicted index by checking out from a
> known tree?

Hrm. Probably not. It is almost a one-way merge going to the named tree
(but limited by the pathspec), except that I think the current
git-checkout code may provide some safety checks related to where we are
coming from (e.g., do we unconditionally overwrite entries that are not
uptodate?).

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Sep 17, 2013 at 01:27:08PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > On Tue, Sep 17, 2013 at 12:58:07PM -0700, Junio C Hamano wrote:
>> >
>> >> I could argue that the above intended behaviour is suboptimal and it
>> >> should have been "the resulting paths in the index and the work tree
>> >> that match the given pathspec must be identical to that of the
>> >> tree-ish".  In the resulting index or working tree, paths that match
>> >> "subdir" pathspec in the above result is subdir/a and subdir/b, and
>> >> that is different from what exists in the given tree-ish (which has
>> >> only subdir/a and not subdir/b), and under that modified definition,
>> >> what the current one does is not correct.
>> >
>> > Our emails just crossed, but I basically ended up saying a similar
>> > thing.  Could we simply replace the "update_some" in builtin/checkout.c
>> > with a two-way merge via unpack-trees?
>> 
>> Would it work to resolve a conflicted index by checking out from a
>> known tree?
>
> Hrm. Probably not. It is almost a one-way merge going to the named tree
> (but limited by the pathspec), except that I think the current
> git-checkout code may provide some safety checks related to where we are
> coming from (e.g., do we unconditionally overwrite entries that are not
> uptodate?).

I think we do unconditionally overwrite and that has been very much
on purpose.

"git checkout tree-ish -- file.txt" has always been about replacing
whatever cruft is in paths in the worktree that match pathspec, just
like "cat content-created-elsewhere >file.txt" is.  "Oops, you have
a local change that do not match index" is the last thing we want to
say, because getting rid of that local change is the primary reason
why "checkout tree-ish -- file.txt" exists.

Taking the state of a subdirectory as a whole as "content", the
change we are discussing will make it more like "rm -fr dir && tar
xf some-content dir" to replace the directory wholesale, which I
personally think is a good thing in the longer term.

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

[Jeff King]

On Tue, Sep 17, 2013 at 01:40:17PM -0700, Junio C Hamano wrote:

> > Hrm. Probably not. It is almost a one-way merge going to the named tree
> > (but limited by the pathspec), except that I think the current
> > git-checkout code may provide some safety checks related to where we are
> > coming from (e.g., do we unconditionally overwrite entries that are not
> > uptodate?).
> 
> I think we do unconditionally overwrite and that has been very much
> on purpose.

I thought so, too, but I was thrown off by the code in checkout_paths()
that warns/aborts if there are unmerged entries. But it looks like we
will have thrown out those entries already during the read_tree_some
call, which adds the new entries using OK_TO_REPLACE.

> "git checkout tree-ish -- file.txt" has always been about replacing
> whatever cruft is in paths in the worktree that match pathspec, just
> like "cat content-created-elsewhere >file.txt" is.  "Oops, you have
> a local change that do not match index" is the last thing we want to
> say, because getting rid of that local change is the primary reason
> why "checkout tree-ish -- file.txt" exists.
> 
> Taking the state of a subdirectory as a whole as "content", the
> change we are discussing will make it more like "rm -fr dir && tar
> xf some-content dir" to replace the directory wholesale, which I
> personally think is a good thing in the longer term.

Yeah, that makes sense. What about untracked files?

Right now we overwrite them if the tree-ish has an entry at the same
path; that is a bit more dangerous than the rest of git, but does match
the "ignore local modifications" rule. I assume if we handled deletions,
though, that we would simply leave them be.

So given that, is it fair to say that a one-way "go here" merge, limited
by pathspec, is the closest equivalent?

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Sep 17, 2013 at 01:40:17PM -0700, Junio C Hamano wrote:
>
>> Taking the state of a subdirectory as a whole as "content", the
>> change we are discussing will make it more like "rm -fr dir && tar
>> xf some-content dir" to replace the directory wholesale, which I
>> personally think is a good thing in the longer term.
>
> Yeah, that makes sense. What about untracked files?

Obviously we cannot literally do "rm -fr dir && tar x", but I agree
that if tree-ish has a path that is not tracked in the current
index, the path should be overwritten and made identical to what is
in the tree-ish.

> Right now we overwrite them if the tree-ish has an entry at the same
> path; that is a bit more dangerous than the rest of git, but does match
> the "ignore local modifications" rule. I assume if we handled deletions,
> though, that we would simply leave them be.
>
> So given that, is it fair to say that a one-way "go here" merge, limited
> by pathspec, is the closest equivalent?

Sorry, but it is unclear to me what you mean by one-way "go here"
merge.  Do you mean oneway_merge() in unpack-trees.c?

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

[Jeff King]

On Tue, Sep 17, 2013 at 03:00:41PM -0700, Junio C Hamano wrote:

> > So given that, is it fair to say that a one-way "go here" merge, limited
> > by pathspec, is the closest equivalent?
> 
> Sorry, but it is unclear to me what you mean by one-way "go here"
> merge.  Do you mean oneway_merge() in unpack-trees.c?

Yes, that is what I meant.

In my mind, oneway_merge is "go here unconditionally", twoway_merge is
"go from A to B, respecting changes already made from A", and
threeway_merge is a "real" merge. But I am not sure if that is accurate
or just my confused mental model of unpack-trees. :)

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Sep 17, 2013 at 03:00:41PM -0700, Junio C Hamano wrote:
>
>> > So given that, is it fair to say that a one-way "go here" merge, limited
>> > by pathspec, is the closest equivalent?
>> 
>> Sorry, but it is unclear to me what you mean by one-way "go here"
>> merge.  Do you mean oneway_merge() in unpack-trees.c?
>
> Yes, that is what I meant.

Yeah, then I agree that "git checkout HEAD^ -- subdir" should be a
one-way "go HEAD^" merge limited only to the paths that match
subdir/.

If implemented in a straight-forward way, I suspect that we may end
up not removing subdir/b in Uwe's sample transcript. I am not sure
if that is a good thing or not, though.  If the index originally
tracked and then "going to" tree does not, I think removing it would
match "ignore local modifications" rule, as subdir/a that is tracked
in the index and also in "going to" tree does get overwritten to
match the state recorded in the tree.

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

[Jeff King]

On Tue, Sep 17, 2013 at 03:14:39PM -0700, Junio C Hamano wrote:

> Yeah, then I agree that "git checkout HEAD^ -- subdir" should be a
> one-way "go HEAD^" merge limited only to the paths that match
> subdir/.
> 
> If implemented in a straight-forward way, I suspect that we may end
> up not removing subdir/b in Uwe's sample transcript. I am not sure
> if that is a good thing or not, though.  If the index originally
> tracked and then "going to" tree does not, I think removing it would
> match "ignore local modifications" rule, as subdir/a that is tracked
> in the index and also in "going to" tree does get overwritten to
> match the state recorded in the tree.

I had assumed the goal was to subdir/b (by the reasoning above, and the
"rm -rf && tar" analogy you gave earlier).

I tried a very hacky attempt at shoving unpack-trees into the right spot
in checkout_paths. But its pathspec handling from unpack_trees is not
quite what we want.

In Uwe's case, it did delete subdir/b and overwrite subdir/a, which I'd
expect. But if there was an additional file outside of subdir, it got
deleted, too.  The problem is that I was giving the regular index to
unpack_trees as the destination; so it wrote out only the bits of the
index under the pathspec subdir/, and omitted the rest entirely. I had
hoped instead it would leave those parts untouched from the source.

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

At this point, I'm giving up for now. I was hoping a naive application
of unpack-trees would just work (and reduce the code size to boot), but
it is obviously a bit more complicated than that. I still think it's a
good idea for checkout to behave as Uwe described, but I don't think
it's that high a priority, and I have other stuff I'd prefer to work on
at the moment.

-Peff

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

[Junio C Hamano]

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

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

[Jeff King]

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

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

> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

True.  The unpack-trees API knows how to walk the index and trees in
parallel, and I tend to agree that it may be a more suitable vehicle
for this purpose.

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

[Jeff King]

On Tue, Sep 17, 2013 at 09:06:59PM +0200, Uwe Kleine-König wrote:

> 	$ git checkout HEAD^ -- subdir
> 
> I'd expect that subdir/b is removed from the index as this file didn't
> exist in HEAD^ but git-status only reports:

I'm not sure if this is intentional or not. The definition of "git
checkout $tree $path" given in commit 0a1283b says:

    Checking paths out of a tree is (currently) defined to do:

     - Grab the paths from the named tree that match the given pathspec,
       and add them to the index;

     - Check out the contents from the index for paths that match the
       pathspec to the working tree; and while at it

     - If the given pathspec did not match anything, suspect a typo from the
       command line and error out without updating the index nor the working
       tree.

So we are applying the pathspec to the named tree, and pulling anything
that matches into the index. Which by definition cannot involve a
deletion, because there is no comparison happening (so we either have
it, or we do not). Whereas what you are expecting is to compare the tree
and the index, limited by the pathspec, and pull any changes from the
tree into the index.

I tend to agree that the latter is more like how other git commands
behave (e.g., if you tried to use "read-tree" to emulate what
git-checkout was doing, I think you would end up with a two-way merge).
But there may be implications I haven't thought of.

Note also that "git checkout -p" does present "subdir/b" as a deletion.
It works by doing a pathspec-limited diff between the two endpoints,
which will notice deletions. So there is some inconsistency there with
the baseline form.

-Peff