Re: [PATCH/RFC 3/3] stash: pass the pathspec argument to git reset

On 03/20, Jeff King wrote:

On Sun, Mar 19, 2017 at 08:23:51PM +0000, Thomas Gummerer wrote:

quoted

For "git stash -p --no-keep-index", the pathspec argument is currently
not passed to "git reset".  This means that changed that are staged but
that are excluded from the pathspec still get unstaged by git stash -p.

Yeah, I noticed this while playing around with patch 2. This seems
like an improvement to me. Unlike the other patches (which are just
tweaking quietness), I think this one really needs a test.

Also, s/changed/changes/ above.

Thanks.  Will add a test in the re-roll.

quoted

---
So this make things a bit inconsistent in for example the following
situation:

    $ git status -s
     M git-stash.sh
    M  read-cache.c

where using git stash -p --no-keep-index, when only changes in
git-stash.sh are added to the stash would reset read-cache.c, but with
git stash -p --no-keep-index -- git-stash.sh, read-cache.c would not
be reset.

I think it's OK. You can't select (or not select) changes from the index
anyway. TBH, I kind of wonder if "git stash -p --no-keep-index" makes
any sense at all.

Yeah I don't have any use case for it, but maybe someone does, so I
think fixing it this way now is the best way forward.

quoted

However it does bring things more in line with git stash --
<pathspec>, so I think this is an improvement.

I did notice one other case while looking at this that I think is much
more serious. The "read-tree" call in the non-patch-mode case doesn't
use a pathspec either. So if we have our same setup where "one" and
"two" have unstaged changes and we do:

  git stash push -k one

Then we stash "one", but the change in "two" is wiped out completely!

Good catch, that's definitely another bug :/ 

I don't think read-tree takes pathspecs, so it would have to drop the
"-u" and replace it with checkout-index. I'm confused about why we would
need it in the first place, though. We should have already dealt with
the working tree files in the earlier block, so there should be nothing
to checkout.

The "-u" goes all the way back to 7bedebcaa (stash: introduce 'stash
save --keep-index' option, 2008-06-27). I wonder if it has always been
unnecessary, but we never noticed because it's just a noop.

I do think the "-u" is necessary.  Say we have a repository with
changes in 'foo' and 'bar', where the changes in bar are added to the
index.

Then 'git stash -k' would wipe out the changes in both 'foo' and 'bar'
through 'git reset --hard', but we do want the changes in 'bar' to
still exist on disk, which they wouldn't without the "-u".

But I'll replace the "-u" with checkout-index, so we can respect the
pathspec.

Thanks!

-Peff