"git stash -- path" reports wrong unstaged changes
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.
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
---
git-stash.sh +6 -1
@@ -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
Quoted 17 lines · Jeff King
I used "git stash -- path" for the first time today and happened to notice an oddity. If I run:
git init -q repo
cd repofor i in one two; do
echo content >$i
git add $i
done
git commit -qm basefor i in one two; do
echo change >$i
done
git stash -- one
Quoted 8 lines
it says:
Saved working directory and index state WIP on master: 20cfadf base Unstaged changes after reset:
M one
M twoEven though "one" no longer has unstaged changes.
Yeah, this is clearly not right. Thanks for catching this before it got into any release.
Quoted 28 lines
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.
(quoted diff) +0
-2-Peff ---git-stash.sh +1
-1@@ -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-stash.sh push_stash () { · line 313
Quoted 10 lines
git-stash.sh +5
-0git 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 :)
This series implements my proposal for fixing bug in git stash -- <pathspec>, plus the two other things I noticed while looking at the code.
Quoted 6 lines
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.
Quoted 5 lines
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(-)
Signature · 1 line
2.12.0.483.gad4152297
PATCH 1/3 stash: show less information for stash push -- <pathspec>
When using git stash push -- <pathspec> in the following sequence:
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 shows:
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.
It really is enough for the user to know that the stash is created, without bothering them with the internal details of what's happening. Always pass the -q flag to git clean and git reset in the pathspec case, to avoid unnecessary and potentially confusing output.
--- git-stash.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
git-stash.sh +2 -2
@@ -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
Signature · 1 line
2.12.0.483.gad4152297
Re: [PATCH/RFC 1/3] stash: show less information for stash push -- <pathspec>
Quoted 43 lines · Thomas Gummerer
When using git stash push -- <pathspec> in the following sequence:
git init -q repo
cd repofor i in one two; do
echo content >$i
git add $i
done
git commit -qm basefor i in one two; do
echo change >$i
done
git stash -- oneit shows:
Saved working directory and index state WIP on master: 20cfadf base Unstaged changes after reset:
M one
M twoEven though "one" no longer has unstaged changes.
It really is enough for the user to know that the stash is created, without bothering them with the internal details of what's happening. Always pass the -q flag to git clean and git reset in the pathspec case, to avoid unnecessary and potentially confusing output.
Reported-by Jeff King <[email protected]>Signed-off-by Thomas Gummerer <[email protected]>--- git-stash.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)git-stash.sh +0
-0@@ -299,10 +299,10 @@ push_stash () { then if test $# != 0
git-stash.sh push_stash () { · line 308
Quoted 10 lines
git-stash.sh +2
-2then - 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).
Re: [PATCH/RFC 1/3] stash: show less information for stash push -- <pathspec>
Quoted 23 lines · Junio C Hamano
Quoted 7 lines
git-stash.sh +0
-0@@ -299,10 +299,10 @@ push_stash () { then if test $# != 0git-stash.sh push_stash () { · line 308
Quoted 10 lines
git-stash.sh +2
-2then - 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} fiYup, 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).
Quoted 4 lines
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).
-Peff
Re: [PATCH/RFC 1/3] stash: show less information for stash push -- <pathspec>
Quoted 30 lines · Jeff King
Quoted 23 lines · Junio C Hamano
Quoted 7 lines
git-stash.sh +0
-0@@ -299,10 +299,10 @@ push_stash () { then if test $# != 0git-stash.sh push_stash () { · line 308
Quoted 10 lines
git-stash.sh +2
-2then - 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} fiYup, 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).
Quoted 4 lines
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).
Quoted 6 lines
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.
Re: [PATCH/RFC 1/3] stash: show less information for stash push -- <pathspec>
Quoted 39 lines · Junio C Hamano
Quoted 30 lines · Jeff King
Quoted 23 lines · Junio C Hamano
Quoted 7 lines
git-stash.sh +0
-0@@ -299,10 +299,10 @@ push_stash () { then if test $# != 0git-stash.sh push_stash () { · line 308
Quoted 10 lines
git-stash.sh +2
-2then - 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} fiYup, 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).
Quoted 4 lines
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).
Quoted 6 lines
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).
Quoted 8 lines
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.
Re: [PATCH/RFC 1/3] stash: show less information for stash push -- <pathspec>
In reply to Thomas GummererQuoted 23 lines · Thomas Gummerer
When using git stash push -- <pathspec> in the following sequence:
git init -q repo
cd repofor i in one two; do
echo content >$i
git add $i
done
git commit -qm basefor i in one two; do
echo change >$i
done
git stash -- oneit shows:
Saved working directory and index state WIP on master: 20cfadf base Unstaged changes after reset:
M one
M two
Quoted 6 lines
Even though "one" no longer has unstaged changes.
It really is enough for the user to know that the stash is created, without bothering them with the internal details of what's happening. Always pass the -q flag to git clean and git reset in the pathspec case, to avoid unnecessary and potentially confusing output.
Yeah, on further reflection, this is definitely the right thing to do.
-Peff
PATCH 2/3 stash: make push -p -q --no-keep-index quiet
In reply to Thomas GummererCurrently 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.
--- git-stash.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
git-stash.sh +1 -1
@@ -322,7 +322,7 @@ push_stash () { if test "$keep_index" != "t" then - git reset + git reset ${GIT_QUIET:+-q} fi fi }
Signature · 1 line
2.12.0.483.gad4152297
Re: [PATCH/RFC 2/3] stash: make push -p -q --no-keep-index quiet
Quoted 9 lines · Thomas Gummerer
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
PATCH 3/3 stash: pass the pathspec argument to git reset
In reply to Thomas GummererFor "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.
Make sure that doesn't happen by passing the pathspec argument to the git reset in question, bringing the behaviour in line with "git stash -- <pathspec>".
---
So this make things a bit inconsistent in for example the following situation:
$ git status -s
M git-stash.sh
M read-cache.cwhere 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.
However it does bring things more in line with git stash -- <pathspec>, so I think this is an improvement.
git-stash.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
git-stash.sh +1 -1
@@ -322,7 +322,7 @@ push_stash () { if test "$keep_index" != "t" then - git reset ${GIT_QUIET:+-q} + git reset ${GIT_QUIET:+-q} -- "$@" fi fi }
Signature · 1 line
2.12.0.483.gad4152297
Re: [PATCH/RFC 3/3] stash: pass the pathspec argument to git reset
Quoted 3 lines · Thomas Gummerer
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.
Quoted 4 lines
---
So this make things a bit inconsistent in for example the following situation:
Quoted 8 lines
$ git status -s M git-stash.sh M read-cache.cwhere using git stash -p --no-keep-index, when only changes in
git-stash.share added to the stash would resetread-cache.c, but with git stash -p --no-keep-index --git-stash.sh,read-cache.cwould 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.
Quoted 2 lines
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
Re: [PATCH/RFC 3/3] stash: pass the pathspec argument to git reset
Quoted 11 lines · Jeff King
Quoted 3 lines · Thomas Gummerer
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.
Quoted 8 lines
Quoted 8 lines
---
So this make things a bit inconsistent in for example the following situation:$ git status -s M git-stash.sh M read-cache.c
Quoted 8 lines
Quoted 4 lines
where using git stash -p --no-keep-index, when only changes in
git-stash.share added to the stash would resetread-cache.c, but with git stash -p --no-keep-index --git-stash.sh,read-cache.cwould 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.
Quoted 11 lines
Quoted 2 lines
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 :/
Quoted 9 lines
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!
Quoted 1 line
-Peff
Re: [PATCH/RFC 3/3] stash: pass the pathspec argument to git reset
Quoted 10 lines · Thomas Gummerer
Quoted 9 lines
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.
Quoted 7 lines
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.
Quoted 2 lines
But I'll replace the "-u" with checkout-index, so we can respect the pathspec.
That makes sense.
-Peff
Thanks Junio and Peff for comments and suggestions on the previous round.
Changes since v1:
- 1/3 now contains the changes that were previously split up into 1/3 and 2/3, and also makes git reset --hard in the simple 'git stash' case quiet unconditionally.
- 2/3 is what was 3/3 in the previous round, and now includes a test for the expected behaviour.
- 3/3 is the fix for the bug Peff noticed for git stash -k with changes that are not yet in the index.
Thomas Gummerer (3):
stash: don't show internal implementation details
stash: pass the pathspec argument to git reset
stash: keep untracked files intact in stash -k
git-stash.sh | 12 +++++++----- t/t3903-stash.sh | 16 +++++++++++++++- t/t3904-stash-patch.sh | 8 ++++++++ 3 files changed, 30 insertions(+), 6 deletions(-)
Signature · 1 line
2.12.0.401.g98d3b1bb99.dirty
PATCH v2 1/3 stash: don't show internal implementation details
git stash push uses other git commands internally. Currently it only passes the -q flag to those if the -q flag is passed to git stash. when using 'git stash push -p -q --no-keep-index', it doesn't even pass the flag on to the internal reset at all.
It really is enough for the user to know that the stash is created, without bothering them with the internal details of what's happening. Always pass the -q flag to the internal git clean and git reset commands, to avoid unnecessary and potentially confusing output.
--- git-stash.sh | 8 ++++---- t/t3903-stash.sh | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-)
git-stash.sh +4 -4
@@ -299,12 +299,12 @@ 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} + git reset --hard -q fi test "$untracked" = "all" && CLEAN_X_OPTION=-x || CLEAN_X_OPTION= if test -n "$untracked"
@@ -322,7 +322,7 @@ push_stash () { if test "$keep_index" != "t" then - git reset + git reset -q fi fi }
t/t3903-stash.sh +1 -1
@@ -663,7 +663,7 @@ test_expect_success 'stash apply shows status same as git status (relative to cu sane_unset GIT_MERGE_VERBOSITY && git stash apply ) | - sed -e 1,2d >actual && # drop "Saved..." and "HEAD is now..." + sed -e 1,1d >actual && # drop "Saved..." test_i18ncmp expect actual '
Signature · 1 line
2.12.0.401.g98d3b1bb99.dirty
Re: [PATCH v2 1/3] stash: don't show internal implementation details
Quoted 5 lines · Thomas Gummerer
git stash push uses other git commands internally. Currently it only passes the -q flag to those if the -q flag is passed to git stash. when using 'git stash push -p -q --no-keep-index', it doesn't even pass the flag on to the internal reset at all.
Quoted 7 lines
It really is enough for the user to know that the stash is created, without bothering them with the internal details of what's happening. Always pass the -q flag to the internal git clean and git reset commands, to avoid unnecessary and potentially confusing output.
Reported-by Jeff King <[email protected]>Signed-off-by Thomas Gummerer <[email protected]>
I think combining these is fine. The incorrect output with pathspecs isn't mentioned anymore, but I think that's OK.
Quoted 21 lines
git-stash.sh +3
-3@@ -299,12 +299,12 @@ 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} + git reset --hard -q fi test "$untracked" = "all" && CLEAN_X_OPTION=-x || CLEAN_X_OPTION= if test -n "$untracked"
git-stash.sh push_stash () { · line 328
Quoted 8 lines
git-stash.sh +1
-1@@ -322,7 +322,7 @@ push_stash () { if test "$keep_index" != "t" then - git reset + git reset -q fi fi }
These all look fine.
t/t3903-stash.sh test_expect_success 'stash apply shows status same as git status (relative to cu · line 668
Quoted 12 lines
t/t3903-stash.sh +1
-1@@ -663,7 +663,7 @@ test_expect_success 'stash apply shows status same as git status (relative to cu sane_unset GIT_MERGE_VERBOSITY && git stash apply ) | - sed -e 1,2d >actual && # drop "Saved..." and "HEAD is now..." + sed -e 1,1d >actual && # drop "Saved..." test_i18ncmp expect actual '
This too, though I think "1d" would be the more usual way to say it.
-peff
Re: [PATCH v2 1/3] stash: don't show internal implementation details
Quoted 12 lines · Jeff King
Quoted 10 lines · Thomas Gummerer
git stash push uses other git commands internally. Currently it only passes the -q flag to those if the -q flag is passed to git stash. when using 'git stash push -p -q --no-keep-index', it doesn't even pass the flag on to the internal reset at all.
It really is enough for the user to know that the stash is created, without bothering them with the internal details of what's happening. Always pass the -q flag to the internal git clean and git reset commands, to avoid unnecessary and potentially confusing output.
Quoted 5 lines
Quoted 2 lines
Reported-by Jeff King <[email protected]>Signed-off-by Thomas Gummerer <[email protected]>I think combining these is fine. The incorrect output with pathspecs isn't mentioned anymore, but I think that's OK.
Yeah, they felt so similar now that splitting this up in different patches would be a bit too noisy.
Quoted 16 lines
[...]
t/t3903-stash.sh test_expect_success 'stash apply shows status same as git status (relative to cu · line 668
Quoted 12 lines
t/t3903-stash.sh +1
-1@@ -663,7 +663,7 @@ test_expect_success 'stash apply shows status same as git status (relative to cu sane_unset GIT_MERGE_VERBOSITY && git stash apply ) | - sed -e 1,2d >actual && # drop "Saved..." and "HEAD is now..." + sed -e 1,1d >actual && # drop "Saved..." test_i18ncmp expect actual 'This too, though I think "1d" would be the more usual way to say it.
Right thanks, I'll keep that in mind for another time. (I guess just changing this doesn't warrant a re-roll?)
Quoted 1 line
-peff
Re: [PATCH v2 1/3] stash: don't show internal implementation details
Quoted 9 lines · Thomas Gummerer
Quoted 6 lines
t/t3903-stash.sh test_expect_success 'stash apply shows status same as git status (relative to cu · line 668
Quoted 4 lines
t/t3903-stash.sh +1
-1- sed -e 1,2d >actual && # drop "Saved..." and "HEAD is now..." + sed -e 1,1d >actual && # drop "Saved..." test_i18ncmp expect actual 'This too, though I think "1d" would be the more usual way to say it.
Right thanks, I'll keep that in mind for another time. (I guess just changing this doesn't warrant a re-roll?)
Nah, I don't think it's important enough (and I give even odds that Junio may just tweak it while applying).
-Peff
PATCH v2 2/3 stash: pass the pathspec argument to git reset
In reply to Thomas GummererFor "git stash -p --no-keep-index", the pathspec argument is currently not passed to "git reset". This means that changes that are staged but that are excluded from the pathspec still get unstaged by git stash -p.
Make sure that doesn't happen by passing the pathspec argument to the git reset in question, bringing the behaviour in line with "git stash -- <pathspec>".
--- git-stash.sh | 2 +- t/t3904-stash-patch.sh | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-)
git-stash.sh +1 -1
@@ -322,7 +322,7 @@ push_stash () { if test "$keep_index" != "t" then - git reset -q + git reset -q -- "$@" fi fi }
t/t3904-stash-patch.sh +8 -0
@@ -77,6 +77,14 @@ test_expect_success 'git stash --no-keep-index -p' ' verify_state dir/foo work index ' +test_expect_success 'stash -p --no-keep-index -- <pathspec> does not unstage other files' ' + set_state HEAD HEADfile_work HEADfile_index && + set_state dir/foo work index && + echo y | git stash push -p --no-keep-index -- HEAD && + verify_state HEAD committed committed && + verify_state dir/foo work index +' + test_expect_success 'none of this moved HEAD' ' verify_saved_head '
Signature · 1 line
2.12.0.401.g98d3b1bb99.dirty
Re: [PATCH v2 2/3] stash: pass the pathspec argument to git reset
Quoted 8 lines · Thomas Gummerer
For "git stash -p --no-keep-index", the pathspec argument is currently not passed to "git reset". This means that changes that are staged but that are excluded from the pathspec still get unstaged by git stash -p.
Make sure that doesn't happen by passing the pathspec argument to the git reset in question, bringing the behaviour in line with "git stash -- <pathspec>".
Quoted 5 lines
Signed-off-by Thomas Gummerer <[email protected]>--- git-stash.sh | 2 +- t/t3904-stash-patch.sh | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-)
This one looks good to me.
-Peff
PATCH v2 3/3 stash: keep untracked files intact in stash -k
In reply to Thomas GummererCurrently when there are untracked changes in a file "one" and in a file "two" in the repository and the user uses:
git stash push -k one
all changes in "two" are wiped out completely. That is clearly not the intended result. Make sure that only the files given in the pathspec are changed when git stash push -k <pathspec> is used.
--- git-stash.sh | 4 +++- t/t3903-stash.sh | 14 ++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-)
git-stash.sh +3 -1
@@ -314,7 +314,9 @@ push_stash () { if test "$keep_index" = "t" && test -n "$i_tree" then - git read-tree --reset -u $i_tree + git read-tree --reset $i_tree + git ls-files -z --modified -- "$@" | + git checkout-index -z --force --stdin fi else git apply -R < "$TMP-patch" ||
t/t3903-stash.sh +14 -0
@@ -907,4 +907,18 @@ test_expect_success 'stash without verb with pathspec' ' test_path_is_file bar ' +test_expect_success 'stash -k -- <pathspec> leaves unstaged files intact' ' + git reset && + >foo && + >bar && + git add foo bar && + git commit -m "test" && + echo "foo" >foo && + echo "bar" >bar && + git stash -k -- foo && + test "",bar = $(cat foo),$(cat bar) && + git stash pop && + test foo,bar = $(cat foo),$(cat bar) +' + test_done
Signature · 1 line
2.12.0.401.g98d3b1bb99.dirty
Re: [PATCH v2 3/3] stash: keep untracked files intact in stash -k
Quoted 8 lines · Thomas Gummerer
Currently when there are untracked changes in a file "one" and in a file "two" in the repository and the user uses:
git stash push -k one
all changes in "two" are wiped out completely. That is clearly not the intended result. Make sure that only the files given in the pathspec are changed when git stash push -k <pathspec> is used.
Good description.
git-stash.sh push_stash () { · line 320
Quoted 13 lines
git-stash.sh +3
-1@@ -314,7 +314,9 @@ push_stash () { if test "$keep_index" = "t" && test -n "$i_tree" then - git read-tree --reset -u $i_tree + git read-tree --reset $i_tree + git ls-files -z --modified -- "$@" | + git checkout-index -z --force --stdin fi
I briefly wondered if this needed "-q" to match the earlier commit, but "checkout-index" isn't really chatty, so I don't think so (and the earlier checkout-index doesn't have it either).
I also wondered if this could be done in a single command as:
git reset -q --hard $i_tree -- "$@"
But "git reset" can't handle pathspecs with "--hard" (which is why the similar case a few lines above uses the same commands).
So this looks good to me.
git-stash.sh push_stash () {
Quoted 13 lines
git-stash.sh +13
-0+test_expect_success 'stash -k -- <pathspec> leaves unstaged files intact' ' + git reset && + >foo && + >bar && + git add foo bar && + git commit -m "test" && + echo "foo" >foo && + echo "bar" >bar && + git stash -k -- foo && + test "",bar = $(cat foo),$(cat bar) && + git stash pop && + test foo,bar = $(cat foo),$(cat bar) +'
I always get nervous when I see test arguments without quotes, but I think this is fine (and I couldn't see a shorter way of doing it with test_cmp).
-Peff
Re: [PATCH v2 3/3] stash: keep untracked files intact in stash -k
Quoted 5 lines · Jeff King
Quoted 3 lines · Thomas Gummerer
Currently when there are untracked changes in a file "one" and in a file "two" in the repository and the user uses:
Quoted 7 lines
Quoted 5 lines
git stash push -k one
all changes in "two" are wiped out completely. That is clearly not the intended result. Make sure that only the files given in the pathspec are changed when git stash push -k <pathspec> is used.
Good description.
I basically just tweaked your report here :) thanks for that!
Quoted 6 lines
Quoted 6 lines
git-stash.sh +0
-0@@ -314,7 +314,9 @@ push_stash () {
Quoted 11 lines
git-stash.sh push_stash () { · line 320
Quoted 7 lines
git-stash.sh +3
-1if test "$keep_index" = "t" && test -n "$i_tree" then - git read-tree --reset -u $i_tree + git read-tree --reset $i_tree + git ls-files -z --modified -- "$@" | + git checkout-index -z --force --stdin fiI briefly wondered if this needed "-q" to match the earlier commit, but "checkout-index" isn't really chatty, so I don't think so (and the earlier checkout-index doesn't have it either).
Yep, I had the same thoughts here.
Quoted 6 lines
I also wondered if this could be done in a single command as:
git reset -q --hard $i_tree -- "$@"But "git reset" can't handle pathspecs with "--hard" (which is why the similar case a few lines above uses the same commands).
Yeah unfortunately, that would have made the previous patch series quite a bit easier :) But here it wouldn't help anyway, as git reset without pathspecs can't handle a tree-ish either, right? And we also want to handle the case where no pathspecs are given to git stash.
Quoted 12 lines
So this looks good to me.
Quoted 9 lines
git-stash.sh +9
-0+test_expect_success 'stash -k -- <pathspec> leaves unstaged files intact' ' + git reset && + >foo && + >bar && + git add foo bar && + git commit -m "test" && + echo "foo" >foo && + echo "bar" >bar && + git stash -k -- foo &&
Quoted 8 lines
git-stash.sh
Quoted 4 lines
git-stash.sh +4
-0+ test "",bar = $(cat foo),$(cat bar) && + git stash pop && + test foo,bar = $(cat foo),$(cat bar) +'I always get nervous when I see test arguments without quotes, but I think this is fine (and I couldn't see a shorter way of doing it with
test_cmp).
Hmm yeah I basically copied this from a different test and tweaked it a bit to fit here.
Quoted 1 line
-Peff
Quoted 11 lines · Thomas Gummerer
git-stash.sh push_stash () { · line 313
Quoted 8 lines
git-stash.sh +5
-0+ 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} fiThis would mean the user gets something like in your case above:
Quoted 7 lines
Unstaged changes after reset:
M twoAs 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".
Quoted 6 lines
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