[+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
