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

[Thomas Gummerer <t.gummerer@gmail.com>]

[+cc Matthieu Moy as author of a patch mentioned below]

On 02/06, Jeff King wrote:
> 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.

Hmm good idea, I think it would indeed be nice to add that.  Theres a
few things to consider though.  First, we need to switch git stash
without arguments over to use git stash push internally.  This does
introduce a slight regression as we currently allow git stash save --
-fmessage, (only messages starting with - are allowed).  I think that
regression would be acceptable however.

The other question is when we should allow the pathspec argument.
3c2eb80f, ("stash: simplify defaulting to "save" and reject unknown
options") made sure that all option arguments are treated the same.  I
think we could use the usual disambiguation of -- to allow pathspecs
after that, so stash -p wouldn't be treated specially, otherwise the
rules could get a bit confusing again.

The other question is how we would deal with the -m flag for
specifying a stash message.  I think we could special case it, but
that would allow users to do things such as git stash -m apply, which
would make the interface a bit more error prone.  So I'm leaning
towards disallowing that for now.

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

Ouch, yeah this is clearly not good.  I'll fix this, and add tests for
these cases.

> -Peff