Thanks for that link.
I did spend a few hours on this topic earlier today, and got very
confused trying to figure out what the deletion behavior _should_ be,
and whether I was breaking it. For some reason I had zero recollection
of a conversation from last year that I was obviously a major part of. I
think I am getting old. :)
The end of that thread concludes that a diff-based approach is not going
to work, because we need to update the working tree even for files not
mentioned by the diff. I do not think that is a show-stopper, though.
It just means that we need to load the new index as one step (done now
with read_tree_recursive, but ideally using diff), and then walk over
the whole resulting index applying our pathspec again (instead of
relying on CE_UPDATE flags).
This turns out not to be a big deal, because the existing code is
already doing most of that second pathspec application anyway. It does
it because read_tree_recursive is not smart enough to update the "seen"
bits for the pathspec. But now we would have another reason to do it
this way. :)
So just to be clear, the behavior we want is that:
echo foo >some-new-path
git add some-new-path
git checkout HEAD -- .
will delete some-new-path (whereas the current code turns it into an
untracked file). What should:
git checkout HEAD -- some-new-path
do in that case? With the current code, it actually barfs, complaining
that nothing matched some-new-path (because it is not part of HEAD, and
therefore we don't consider it at all), and aborts the whole operation.
I think we would want to delete some-new-path in that case, too.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 23:02:52
On Sat, Nov 08, 2014 at 03:30:40AM -0500, Jeff King wrote:
So just to be clear, the behavior we want is that:
echo foo >some-new-path
git add some-new-path
git checkout HEAD -- .
will delete some-new-path (whereas the current code turns it into an
untracked file). What should:
git checkout HEAD -- some-new-path
do in that case? With the current code, it actually barfs, complaining
that nothing matched some-new-path (because it is not part of HEAD, and
therefore we don't consider it at all), and aborts the whole operation.
I think we would want to delete some-new-path in that case, too.
Also, t2022.3 has me very confused.
It is explicitly checking that if we have "subdir/foo" unmerged in the
index, and we "git checkout $tree -- subdir", and $tree does not mention
"foo", that we _leave_ foo in place.
That seems very counter-intuitive to me. If you asked to make "subdir"
look like $tree, then we should clobber it. That change comes from
e721c15 (checkout: avoid unnecessary match_pathspec calls, 2013-03-27),
where it is mentioned as a _bugfix_. That in turn references 0a1283b
(checkout $tree $path: do not clobber local changes in $path not in
$tree, 2011-09-30), which explicitly goes against the goal we are
talking about here. It is not "make my index and working tree look like
$tree" at all.
So now I'm doubly confused about what we want to do.
If we want to retain that behavior, I think we can still cover these
cases by marking items missing from $tree as "to remove" during the
diff/"update the index" phase, and then being more gentle with "to
remove" files (e.g., not clobbering changed worktree files unless "-f"
is given).
I am not sure that provides a sane user experience, though. Why is it OK
to clobber local changes to a file if we are replacing it with other
content, but _not_ if we are replacing it with nothing? Either the
content we are losing is valuable or not, but it has nothing to do with
what we are replacing. And Junio argued in the thread linked elsewhere
that the point of "git checkout $tree -- $path" is to clobber what is in
$path, which I would agree with.
I think the argument made in 0a1283b is that "git checkout $tree $path"
is not "make $path like $tree", but rather "pick bits of $path out of
$tree". Which would mean this whole deletion thing we are talking about
is completely contrary to that.
So which is it?
-Peff
From: Martin von Zweigbergk <hidden> Date: 2016-06-15 23:02:53
First of all, thanks again for spending time on this.
On Sat, Nov 8, 2014 at 12:30 AM, Jeff King [off-list ref] wrote:
On Fri, Nov 07, 2014 at 11:35:59PM -0800, Junio C Hamano wrote:
So just to be clear, the behavior we want is that:
echo foo >some-new-path
git add some-new-path
git checkout HEAD -- .
will delete some-new-path (whereas the current code turns it into an
untracked file).
Yes, I think that's what I would expect.
What should:
git checkout HEAD -- some-new-path
do in that case? With the current code, it actually barfs, complaining
that nothing matched some-new-path (because it is not part of HEAD, and
therefore we don't consider it at all), and aborts the whole operation.
I think we would want to delete some-new-path in that case, too.
I don't think we'd want it to be deleted. I would view 'git reset
--hard' as the role model here, and that command (without paths) would
not remove the file. And applying it to a path should not change the
behavior, just restrict it to the paths, right?
From: Jeff King <hidden> Date: 2016-06-15 23:02:53
On Sat, Nov 08, 2014 at 08:19:21AM -0800, Martin von Zweigbergk wrote:
quoted
What should:
git checkout HEAD -- some-new-path
do in that case? With the current code, it actually barfs, complaining
that nothing matched some-new-path (because it is not part of HEAD, and
therefore we don't consider it at all), and aborts the whole operation.
I think we would want to delete some-new-path in that case, too.
I don't think we'd want it to be deleted. I would view 'git reset
--hard' as the role model here, and that command (without paths) would
not remove the file. And applying it to a path should not change the
behavior, just restrict it to the paths, right?
Are you sure about "git reset" here? If I do:
git init
echo content >file && git add file && git commit -m base
echo modified >file
echo new >some-new-path
git add file some-new-path
git reset --hard
then we delete some-new-path (it is not untracked, because the index
knows about it). That makes sense to me. I.e., we treat it with the same
"preciousness" whether it is named explicitly or not.
-Peff