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

[Jeff King <peff@peff.net>]

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

> 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