Re: [PATCH v3 0/5] stash: support pathspec argument

From: Jeff King <peff@peff.net>
Date: Feb 6, 2017, 16:14 UTC
Message-ID: <20170206161432.zvpsqegjspaa2l5l@sigill.intra.peff.net>
In-Reply-To: <20170205202642.14216-1-t.gummerer@gmail.com>

On Sun, Feb 05, 2017 at 08:26:37PM +0000, Thomas Gummerer wrote:

Show 11 quoted lines

> Thanks Junio for the review in the previous round.
> 
> Changes since v2:
> 
> - $IFS should now be supported by using "$@" everywhere instead of using
>   a $files variable.
> - Added a new patch showing the old behaviour of git stash create is
>   preserved.
> - Rephrased the documentation
> - Simplified the option parsing in save_stash, by leaving the
>   actual parsing to push_stash instead.

Overall, I like the direction this is heading. I raised a few issues, the most major of which is whether we want to allow the minor regression in "git stash create -m foo".

This also makes "git stash push -p <pathspec...>" work, which is good. I don't think "git stash -p <pathspec...>" does, though. I _think_ it would be trivial to do on top, since we already consider that case an error. That's somewhat outside the scope of your series, so I won't complain (too much :) ) if you don't dig into it, but it might just be trivial on top.

A few other random bits I noticed while playing with the new code:

  $ git init
  $ echo content >file && git add file && git commit -m file
  $ echo change >file

  $ git stash push -p not-file
  No changes.
  No changes selected

Probably only one of those is necessary. :)

Let's keep trying a few things:

  $ git stash push not-file
  Saved working directory and index state WIP on master: 5d5f951 file
  Unstaged changes after reset:
  M	file
  M	file

The unstaged change is mentioned twice, which is weird. But weirder still is that we created a stash at all, as it's empty. Why didn't we hit the "no changes selected" path?

And one more:

  $ echo foo >untracked
  $ git stash push untracked
  Saved working directory and index state WIP on master: 5d5f951 file
  Unstaged changes after reset:
  M	file
  M	file
  Removing untracked

We removed the untracked file, even though it wasn't actually stashed! I thought at first this was because it was mentioned as a pathspec, but it seems to happen even with a different name:

  $ echo foo >untracked
  $ git stash push does-not-exist
  ...
  Removing untracked

That seems dangerous.

-Peff