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

From: Jeff King <peff@peff.net>
Date: Mar 20, 2017, 19:08 UTC
Message-ID: <20170320190843.aw3fsenjvcek7a44@sigill.intra.peff.net>
In-Reply-To: <20170319202351.8825-4-t.gummerer@gmail.com>

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

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

Show 12 quoted lines

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

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

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.

-Peff