*Re: [BUG] "git stash -- path" reports wrong unstaged changes
2017-03-17 14:50 [BUG] "git stash -- path" reports wrong unstaged changes Jeff King
@ 2017-03-18 18:36 ` Thomas Gummerer
2017-03-19 20:23 ` Thomas Gummerer
2017-03-20 18:41 ` [BUG] "git stash -- path" reports wrong unstaged changes Jeff King
0 siblings, 2 replies; 26+ messages in thread
From: Thomas Gummerer @ 2017-03-18 18:36 UTC (permalink / raw)
To: Jeff King; +Cc: git
On 03/17, Jeff King wrote:
> I used "git stash -- path" for the first time today and happened to
> notice an oddity. If I run:
>
> git init -q repo
> cd repo
>
> for i in one two; do
> echo content >$i
> git add $i
> done
> git commit -qm base
>
> for i in one two; do
> echo change >$i
> done
> git stash -- one
>
> it says:
>
> Saved working directory and index state WIP on master: 20cfadf base
> Unstaged changes after reset:
> M one
> M two
>
> Even though "one" no longer has unstaged changes.
Yeah, this is clearly not right. Thanks for catching this before it
got into any release.
> If I run with GIT_TRACE=1, that message is generated by:
>
> git reset -- one
>
> which makes sense. At that stage we've just reset the index, but the
> working tree file still has modifications. In the non-pathspec case we
> run "git reset --hard", which takes care of the index and the working
> tree.
>
> It's really "checkout-index" that cleans the working tree, but it
> doesn't have porcelain finery like an "Unstaged changes" message. I
> think the patch below would fix it, but I wonder if we can do it in a
> way that doesn't involve calling diff-files twice.
>
> -Peff
>
> ---
> diff --git a/git-stash.sh b/git-stash.sh
> index 9c70662cc..9a4bb503a 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -299,10 +299,15 @@ push_stash () {
> then
> if test $# != 0
> then
> - git reset ${GIT_QUIET:+-q} -- "$@"
> + git reset -q -- "$@"
> git ls-files -z --modified -- "$@" |
> git checkout-index -z --force --stdin
> git clean --force ${GIT_QUIET:+-q} -d -- "$@"
> + if test -z "$GIT_QUIET" && ! git diff-files --quiet
> + then
> + say "$(gettext "Unstaged changes after reset:")"
> + git diff-files --name-status
> + fi
> else
> git reset --hard ${GIT_QUIET:+-q}
> fi
This would mean the user gets something like in your case above:
Unstaged changes after reset:
M two
As a user that doesn't know the internal implementation of push_stash,
this would make me wonder why git stash would mention a file that is
not provided as pathspec, but not the one that was provided in the
pathspec argument.
I think one option would be to to just keep quiet about the exact
changes that git stash push makes, similar to what we do in the
--include-untracked and in the -p case. The other option would be to
find the files that are affected and print them, but that would
probably be a bit too noisy especially in cases such as
git stash push -- docs/*.
Also from reading the code in the -p case, when --keep-index is given,
the git reset there doesn't respect $GIT_QUIET at all, and also
doesn't respect the pathspec argument, which seems like another bug.
I can submit a patch series for those, but I won't get to it before
tomorrow :)
^permalinkrawreply [flat|threaded] 26+ messages in thread

*Re: [BUG] "git stash -- path" reports wrong unstaged changes
2017-03-18 18:36 ` Thomas Gummerer@ 2017-03-19 20:23 ` Thomas Gummerer
2017-03-19 20:23 ` [PATCH/RFC 1/3] stash: show less information for stash push -- <pathspec> Thomas Gummerer
` (3 more replies)
2017-03-20 18:41 ` [BUG] "git stash -- path" reports wrong unstaged changes Jeff King
1 sibling, 4 replies; 26+ messages in thread
From: Thomas Gummerer @ 2017-03-19 20:23 UTC (permalink / raw)
To: Jeff King; +Cc: git, Thomas Gummerer
This series implements my proposal for fixing bug in git stash --
<pathspec>, plus the two other things I noticed while looking at the
code.
> I think one option would be to to just keep quiet about the exact
> changes that git stash push makes, similar to what we do in the
> --include-untracked and in the -p case. The other option would be to
> find the files that are affected and print them, but that would
> probably be a bit too noisy especially in cases such as
> git stash push -- docs/*.
1/3 implements just this. This may deserve some more discussion on
what should actually be done.
> Also from reading the code in the -p case, when --keep-index is given,
> the git reset there doesn't respect $GIT_QUIET at all, and also
> doesn't respect the pathspec argument, which seems like another bug.
> I can submit a patch series for those, but I won't get to it before
> tomorrow :)
2/3 and 3/3 have the fixes for the above. No tests yet, as I'm not
100% sure 3/3 is doing the right thing, though I think it makes the
overall experience better.
Thomas Gummerer (3):
stash: show less information for stash push -- <pathspec>
stash: make push -p -q --no-keep-index quiet
stash: pass the pathspec argument to git reset
git-stash.sh | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
--
2.12.0.483.gad4152297
^permalinkrawreply [flat|threaded] 26+ messages in thread

*Re: [BUG] "git stash -- path" reports wrong unstaged changes
2017-03-18 18:36 ` Thomas Gummerer
2017-03-19 20:23 ` Thomas Gummerer@ 2017-03-20 18:41 ` Jeff King1 sibling, 0 replies; 26+ messages in thread
From: Jeff King @ 2017-03-20 18:41 UTC (permalink / raw)
To: Thomas Gummerer; +Cc: git
On Sat, Mar 18, 2017 at 06:36:58PM +0000, Thomas Gummerer wrote:
> > + if test -z "$GIT_QUIET" && ! git diff-files --quiet
> > + then
> > + say "$(gettext "Unstaged changes after reset:")"
> > + git diff-files --name-status
> > + fi
> > else
> > git reset --hard ${GIT_QUIET:+-q}
> > fi
>
> This would mean the user gets something like in your case above:
>
> Unstaged changes after reset:
> M two
>
> As a user that doesn't know the internal implementation of push_stash,
> this would make me wonder why git stash would mention a file that is
> not provided as pathspec, but not the one that was provided in the
> pathspec argument.
That's a good point. I was going for consistency with the non-pathspec
case, but of course it wouldn't mention any files in the first place,
because it's just done a complete "git reset --hard".
> I think one option would be to to just keep quiet about the exact
> changes that git stash push makes, similar to what we do in the
> --include-untracked and in the -p case. The other option would be to
> find the files that are affected and print them, but that would
> probably be a bit too noisy especially in cases such as
> git stash push -- docs/*.
Yeah, that's the right thing. I was just trying to be too clever above.
-Peff
^permalinkrawreply [flat|threaded] 26+ messages in thread

*Re: [PATCH/RFC 2/3] stash: make push -p -q --no-keep-index quiet
2017-03-19 20:23 ` [PATCH/RFC 2/3] stash: make push -p -q --no-keep-index quiet Thomas Gummerer
@ 2017-03-20 18:55 ` Jeff King0 siblings, 0 replies; 26+ messages in thread
From: Jeff King @ 2017-03-20 18:55 UTC (permalink / raw)
To: Thomas Gummerer; +Cc: git
On Sun, Mar 19, 2017 at 08:23:50PM +0000, Thomas Gummerer wrote:
> Currently when using "git stash push -p -q --no-keep-index", the -q flag
> is not passed to the git reset which is executed when --no-keep-index is
> also passed in. This means that git stash is somewhat verbose in this
> mode even when the -q flag is passed in. This was always the case since
> "git stash save -p" was introduced in dda1f2a5 ("Implement 'git stash
> save --patch'", 2009-08-13).
>
> Properly pass the -q flag on to git reset, to make "git stash push -p
> -q" as quiet as it should be.
Yeah, this is an obvious bug-fix. Though given my earlier response to
Junio, I wonder if we should just be doing "reset -q" unconditionally
for most of these calls.
I guess this one does say more than just "HEAD is at..."; it mentions
the files that you _didn't_ pick. Though that now makes it inconsistent
with the pathspec case, especially if you do:
git stash push -p --no-keep-index -- one
which will mention "two" as remaining with unstaged changes.
-Peff
^permalinkrawreply [flat|threaded] 26+ messages in thread

*Re: [PATCH/RFC 3/3] stash: pass the pathspec argument to git reset
2017-03-19 20:23 ` [PATCH/RFC 3/3] stash: pass the pathspec argument to git reset Thomas Gummerer
@ 2017-03-20 19:08 ` Jeff King
2017-03-21 21:14 ` Thomas Gummerer0 siblings, 1 reply; 26+ messages in thread
From: Jeff King @ 2017-03-20 19:08 UTC (permalink / raw)
To: Thomas Gummerer; +Cc: git
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.
> ---
> 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
^permalinkrawreply [flat|threaded] 26+ messages in thread

*Re: [PATCH/RFC 1/3] stash: show less information for stash push -- <pathspec>
2017-03-20 18:48 ` Jeff King@ 2017-03-20 19:42 ` Junio C Hamano
2017-03-21 20:48 ` Thomas Gummerer0 siblings, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2017-03-20 19:42 UTC (permalink / raw)
To: Jeff King; +Cc: Thomas Gummerer, git
Jeff King <peff@peff.net> writes:
> On Mon, Mar 20, 2017 at 10:51:16AM -0700, Junio C Hamano wrote:
>
>> > diff --git a/git-stash.sh b/git-stash.sh
>> > index 9c70662cc8..59f055e27b 100755
>> > --- a/git-stash.sh
>> > +++ b/git-stash.sh
>> > @@ -299,10 +299,10 @@ push_stash () {
>> > then
>> > if test $# != 0
>> > then
>> > - git reset ${GIT_QUIET:+-q} -- "$@"
>> > + git reset -q -- "$@"
>> > git ls-files -z --modified -- "$@" |
>> > git checkout-index -z --force --stdin
>> > - git clean --force ${GIT_QUIET:+-q} -d -- "$@"
>> > + git clean --force -q -d -- "$@"
>> > else
>> > git reset --hard ${GIT_QUIET:+-q}
>> > fi
>>
>> Yup, we only said "HEAD is now at ..." in the non-pathspec case (and
>> we of course still do). We didn't report changes to which paths
>> have been reverted in (or which new paths are removed from) the
>> working tree to the original state (and we of course still don't).
>>
>> The messages from reset and clean that reports these probably do not
>> need to be shown by default to the users (they can always check with
>> "git stash show" when they are curious or when they want to double
>> check).
>
> I'm not sure if you are arguing here that the non-pathspec case should
> move to an unconditional "-q", too, to suppress the "HEAD is now at"
> message. But I think that is a good suggestion. It would make the two
> cases consistent, and it is not really adding anything of value (it is
> always just HEAD, and if you do not provide a custom message, the
> short-sha1 and subject are already in the "Saved..." line above).
I wasn't suggesting it (I was just saying that these extra messages
are not something we found necessary for consistency with the
original codepath when we added the pathspec support). I wasn't
even thinking about what the original codepath did, i.e. when the
command is run without pathspec.
I too suspect that most of the ${GIT_QUIET:+-q} can just become an
unconditional -q as you do.
^permalinkrawreply [flat|threaded] 26+ messages in thread

*Re: [PATCH/RFC 1/3] stash: show less information for stash push -- <pathspec>
2017-03-20 19:42 ` Junio C Hamano@ 2017-03-21 20:48 ` Thomas Gummerer0 siblings, 0 replies; 26+ messages in thread
From: Thomas Gummerer @ 2017-03-21 20:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
On 03/20, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > On Mon, Mar 20, 2017 at 10:51:16AM -0700, Junio C Hamano wrote:
> >
> >> > diff --git a/git-stash.sh b/git-stash.sh
> >> > index 9c70662cc8..59f055e27b 100755
> >> > --- a/git-stash.sh
> >> > +++ b/git-stash.sh
> >> > @@ -299,10 +299,10 @@ push_stash () {
> >> > then
> >> > if test $# != 0
> >> > then
> >> > - git reset ${GIT_QUIET:+-q} -- "$@"
> >> > + git reset -q -- "$@"
> >> > git ls-files -z --modified -- "$@" |
> >> > git checkout-index -z --force --stdin
> >> > - git clean --force ${GIT_QUIET:+-q} -d -- "$@"
> >> > + git clean --force -q -d -- "$@"
> >> > else
> >> > git reset --hard ${GIT_QUIET:+-q}
> >> > fi
> >>
> >> Yup, we only said "HEAD is now at ..." in the non-pathspec case (and
> >> we of course still do). We didn't report changes to which paths
> >> have been reverted in (or which new paths are removed from) the
> >> working tree to the original state (and we of course still don't).
> >>
> >> The messages from reset and clean that reports these probably do not
> >> need to be shown by default to the users (they can always check with
> >> "git stash show" when they are curious or when they want to double
> >> check).
> >
> > I'm not sure if you are arguing here that the non-pathspec case should
> > move to an unconditional "-q", too, to suppress the "HEAD is now at"
> > message. But I think that is a good suggestion. It would make the two
> > cases consistent, and it is not really adding anything of value (it is
> > always just HEAD, and if you do not provide a custom message, the
> > short-sha1 and subject are already in the "Saved..." line above).
>
> I wasn't suggesting it (I was just saying that these extra messages
> are not something we found necessary for consistency with the
> original codepath when we added the pathspec support). I wasn't
> even thinking about what the original codepath did, i.e. when the
> command is run without pathspec.
>
> I too suspect that most of the ${GIT_QUIET:+-q} can just become an
> unconditional -q as you do.
Thanks both, I do agree that passing -q unconditionally is probably
the right thing to do. Will do that, and also pass -q unconditionally
to the git reset I addressed in 2/3 here in the re-roll.
^permalinkrawreply [flat|threaded] 26+ messages in thread

*Re: [PATCH/RFC 3/3] stash: pass the pathspec argument to git reset
2017-03-20 19:08 ` Jeff King@ 2017-03-21 21:14 ` Thomas Gummerer
2017-03-21 22:12 ` Jeff King0 siblings, 1 reply; 26+ messages in thread
From: Thomas Gummerer @ 2017-03-21 21:14 UTC (permalink / raw)
To: Jeff King; +Cc: git
On 03/20, Jeff King wrote:
> 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.
Thanks. Will add a test in the re-roll.
> > ---
> > 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.
Yeah I don't have any use case for it, but maybe someone does, so I
think fixing it this way now is the best way forward.
> > 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!
Good catch, that's definitely another bug :/
> 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.
I do think the "-u" is necessary. Say we have a repository with
changes in 'foo' and 'bar', where the changes in bar are added to the
index.
Then 'git stash -k' would wipe out the changes in both 'foo' and 'bar'
through 'git reset --hard', but we do want the changes in 'bar' to
still exist on disk, which they wouldn't without the "-u".
But I'll replace the "-u" with checkout-index, so we can respect the
pathspec.
Thanks!
> -Peff
^permalinkrawreply [flat|threaded] 26+ messages in thread

*Re: [PATCH/RFC 3/3] stash: pass the pathspec argument to git reset
2017-03-21 21:14 ` Thomas Gummerer@ 2017-03-21 22:12 ` Jeff King0 siblings, 0 replies; 26+ messages in thread
From: Jeff King @ 2017-03-21 22:12 UTC (permalink / raw)
To: Thomas Gummerer; +Cc: git
On Tue, Mar 21, 2017 at 09:14:24PM +0000, Thomas Gummerer wrote:
> > 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.
>
> I do think the "-u" is necessary. Say we have a repository with
> changes in 'foo' and 'bar', where the changes in bar are added to the
> index.
>
> Then 'git stash -k' would wipe out the changes in both 'foo' and 'bar'
> through 'git reset --hard', but we do want the changes in 'bar' to
> still exist on disk, which they wouldn't without the "-u".
Yeah, you're right. It makes me wonder if the "-k" case should be
skipping the "reset --hard". But as this series has shown, this
procedure is sufficiently tricky that it may be a good idea to make the
minimal change.
> But I'll replace the "-u" with checkout-index, so we can respect the
> pathspec.
That makes sense.
-Peff
^permalinkrawreply [flat|threaded] 26+ messages in thread