{"thread":{"id":"45407","subject":"[BUG] \"git stash -- path\" reports wrong unstaged changes","startedAt":"2017-03-17T15:00:37Z","lastAt":"2017-03-22T21:46:22Z","messageCount":26,"participants":["Jeff King","Thomas Gummerer","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"314338","messageId":"20170317145039.dmcb3qyqbzfvtmgz@sigill.intra.peff.net","threadId":"45407","inReplyTo":null,"subject":"[BUG] \"git stash -- path\" reports wrong unstaged changes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-17T14:50:39Z","receivedAt":"2017-03-17T15:00:37Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I used \"git stash -- path\" for the first time today and happened to\nnotice an oddity. If I run:\n\n\tgit init -q repo\n\tcd repo\n\t\n\tfor i in one two; do\n\t\techo content >$i\n\t\tgit add $i\n\tdone\n\tgit commit -qm base\n\t\n\tfor i in one two; do\n\t\techo change >$i\n\tdone\n\tgit stash -- one\n\nit says:\n\n  Saved working directory and index state WIP on master: 20cfadf base\n  Unstaged changes after reset:\n  M\tone\n  M\ttwo\n\nEven though \"one\" no longer has unstaged changes.\n\nIf I run with GIT_TRACE=1, that message is generated by:\n\n  git reset -- one\n\nwhich makes sense. At that stage we've just reset the index, but the\nworking tree file still has modifications. In the non-pathspec case we\nrun \"git reset --hard\", which takes care of the index and the working\ntree.\n\nIt's really \"checkout-index\" that cleans the working tree, but it\ndoesn't have porcelain finery like an \"Unstaged changes\" message. I\nthink the patch below would fix it, but I wonder if we can do it in a\nway that doesn't involve calling diff-files twice.\n\n-Peff\n\n---\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 9c70662cc..9a4bb503a 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -299,10 +299,15 @@ push_stash () {\n \tthen\n \t\tif test $# != 0\n \t\tthen\n-\t\t\tgit reset ${GIT_QUIET:+-q} -- \"$@\"\n+\t\t\tgit reset -q -- \"$@\"\n \t\t\tgit ls-files -z --modified -- \"$@\" |\n \t\t\tgit checkout-index -z --force --stdin\n \t\t\tgit clean --force ${GIT_QUIET:+-q} -d -- \"$@\"\n+\t\t\tif test -z \"$GIT_QUIET\" && ! git diff-files --quiet\n+\t\t\tthen\n+\t\t\t\tsay \"$(gettext \"Unstaged changes after reset:\")\"\n+\t\t\t\tgit diff-files --name-status\n+\t\t\tfi\n \t\telse\n \t\t\tgit reset --hard ${GIT_QUIET:+-q}\n \t\tfi\n"},{"id":"314524","messageId":"20170318183658.GC27158@hank","threadId":"45407","inReplyTo":"20170317145039.dmcb3qyqbzfvtmgz@sigill.intra.peff.net","subject":"Re: [BUG] \"git stash -- path\" reports wrong unstaged changes","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2017-03-18T18:36:58Z","receivedAt":"2017-03-18T18:43:30Z","isPatch":false,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 03/17, Jeff King wrote:\n> I used \"git stash -- path\" for the first time today and happened to\n> notice an oddity. If I run:\n> \n> \tgit init -q repo\n> \tcd repo\n> \t\n> \tfor i in one two; do\n> \t\techo content >$i\n> \t\tgit add $i\n> \tdone\n> \tgit commit -qm base\n> \t\n> \tfor i in one two; do\n> \t\techo change >$i\n> \tdone\n> \tgit stash -- one\n> \n> it says:\n> \n>   Saved working directory and index state WIP on master: 20cfadf base\n>   Unstaged changes after reset:\n>   M\tone\n>   M\ttwo\n> \n> Even though \"one\" no longer has unstaged changes.\n\nYeah, this is clearly not right.  Thanks for catching this before it\ngot into any release.\n\n> If I run with GIT_TRACE=1, that message is generated by:\n> \n>   git reset -- one\n> \n> which makes sense. At that stage we've just reset the index, but the\n> working tree file still has modifications. In the non-pathspec case we\n> run \"git reset --hard\", which takes care of the index and the working\n> tree.\n> \n> It's really \"checkout-index\" that cleans the working tree, but it\n> doesn't have porcelain finery like an \"Unstaged changes\" message. I\n> think the patch below would fix it, but I wonder if we can do it in a\n> way that doesn't involve calling diff-files twice.\n> \n> -Peff\n> \n> ---\n> diff --git a/git-stash.sh b/git-stash.sh\n> index 9c70662cc..9a4bb503a 100755\n> --- a/git-stash.sh\n> +++ b/git-stash.sh\n> @@ -299,10 +299,15 @@ push_stash () {\n>  \tthen\n>  \t\tif test $# != 0\n>  \t\tthen\n> -\t\t\tgit reset ${GIT_QUIET:+-q} -- \"$@\"\n> +\t\t\tgit reset -q -- \"$@\"\n>  \t\t\tgit ls-files -z --modified -- \"$@\" |\n>  \t\t\tgit checkout-index -z --force --stdin\n>  \t\t\tgit clean --force ${GIT_QUIET:+-q} -d -- \"$@\"\n> +\t\t\tif test -z \"$GIT_QUIET\" && ! git diff-files --quiet\n> +\t\t\tthen\n> +\t\t\t\tsay \"$(gettext \"Unstaged changes after reset:\")\"\n> +\t\t\t\tgit diff-files --name-status\n> +\t\t\tfi\n>  \t\telse\n>  \t\t\tgit reset --hard ${GIT_QUIET:+-q}\n>  \t\tfi\n\nThis would mean the user gets something like in your case above:\n\n    Unstaged changes after reset:\n     M\ttwo\n\nAs a user that doesn't know the internal implementation of push_stash,\nthis would make me wonder why git stash would mention a file that is\nnot provided as pathspec, but not the one that was provided in the\npathspec argument.\n\nI think one option would be to to just keep quiet about the exact\nchanges that git stash push makes, similar to what we do in the\n--include-untracked and in the -p case.  The other option would be to\nfind the files that are affected and print them, but that would\nprobably be a bit too noisy especially in cases such as\ngit stash push -- docs/*.\n\nAlso from reading the code in the -p case, when --keep-index is given,\nthe git reset there doesn't respect $GIT_QUIET at all, and also\ndoesn't respect the pathspec argument, which seems like another bug.\nI can submit a patch series for those, but I won't get to it before\ntomorrow :)\n"},{"id":"314598","messageId":"20170319202351.8825-2-t.gummerer@gmail.com","threadId":"45407","inReplyTo":"20170319202351.8825-1-t.gummerer@gmail.com","subject":"[PATCH/RFC 1/3] stash: show less information for stash push -- <pathspec>","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2017-03-19T20:23:49Z","receivedAt":"2017-03-19T20:24:13Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"When using git stash push -- <pathspec> in the following sequence:\n\n       git init -q repo\n       cd repo\n\n       for i in one two; do\n               echo content >$i\n               git add $i\n       done\n       git commit -qm base\n\n       for i in one two; do\n               echo change >$i\n       done\n       git stash -- one\n\nit shows:\n\n   Saved working directory and index state WIP on master: 20cfadf base\n   Unstaged changes after reset:\n    M   one\n    M   two\n\nEven though \"one\" no longer has unstaged changes.\n\nIt really is enough for the user to know that the stash is created,\nwithout bothering them with the internal details of what's happening.\nAlways pass the -q flag to git clean and git reset in the pathspec case,\nto avoid unnecessary and potentially confusing output.\n\nReported-by: Jeff King <peff@peff.net>\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n git-stash.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 9c70662cc8..59f055e27b 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -299,10 +299,10 @@ push_stash () {\n \tthen\n \t\tif test $# != 0\n \t\tthen\n-\t\t\tgit reset ${GIT_QUIET:+-q} -- \"$@\"\n+\t\t\tgit reset -q -- \"$@\"\n \t\t\tgit ls-files -z --modified -- \"$@\" |\n \t\t\tgit checkout-index -z --force --stdin\n-\t\t\tgit clean --force ${GIT_QUIET:+-q} -d -- \"$@\"\n+\t\t\tgit clean --force -q -d -- \"$@\"\n \t\telse\n \t\t\tgit reset --hard ${GIT_QUIET:+-q}\n \t\tfi\n-- \n2.12.0.483.gad4152297\n\n"},{"id":"314599","messageId":"20170319202351.8825-1-t.gummerer@gmail.com","threadId":"45407","inReplyTo":"20170318183658.GC27158@hank","subject":"Re: [BUG] \"git stash -- path\" reports wrong unstaged changes","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2017-03-19T20:23:48Z","receivedAt":"2017-03-19T20:24:15Z","isPatch":false,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"This series implements my proposal for fixing bug in git stash --\n<pathspec>, plus the two other things I noticed while looking at the\ncode.\n\n> I think one option would be to to just keep quiet about the exact\n> changes that git stash push makes, similar to what we do in the\n> --include-untracked and in the -p case.  The other option would be to\n> find the files that are affected and print them, but that would\n> probably be a bit too noisy especially in cases such as\n> git stash push -- docs/*.\n\n1/3 implements just this.  This may deserve some more discussion on\nwhat should actually be done.\n\n> Also from reading the code in the -p case, when --keep-index is given,\n> the git reset there doesn't respect $GIT_QUIET at all, and also\n> doesn't respect the pathspec argument, which seems like another bug.\n> I can submit a patch series for those, but I won't get to it before\n> tomorrow :)\n\n2/3 and 3/3 have the fixes for the above.  No tests yet, as I'm not\n100% sure 3/3 is doing the right thing, though I think it makes the\noverall experience better.\n\nThomas Gummerer (3):\n  stash: show less information for stash push -- <pathspec>\n  stash: make push -p -q --no-keep-index quiet\n  stash: pass the pathspec argument to git reset\n\n git-stash.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\n-- \n2.12.0.483.gad4152297\n\n"},{"id":"314601","messageId":"20170319202351.8825-4-t.gummerer@gmail.com","threadId":"45407","inReplyTo":"20170319202351.8825-1-t.gummerer@gmail.com","subject":"[PATCH/RFC 3/3] stash: pass the pathspec argument to git reset","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2017-03-19T20:23:51Z","receivedAt":"2017-03-19T20:24:23Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"For \"git stash -p --no-keep-index\", the pathspec argument is currently\nnot passed to \"git reset\".  This means that changed that are staged but\nthat are excluded from the pathspec still get unstaged by git stash -p.\n\nMake sure that doesn't happen by passing the pathspec argument to the\ngit reset in question, bringing the behaviour in line with \"git stash --\n<pathspec>\".\n\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\nSo this make things a bit inconsistent in for example the following\nsituation:\n\n    $ git status -s\n     M git-stash.sh\n    M  read-cache.c\n\nwhere using git stash -p --no-keep-index, when only changes in\ngit-stash.sh are added to the stash would reset read-cache.c, but with\ngit stash -p --no-keep-index -- git-stash.sh, read-cache.c would not\nbe reset.  \n\nHowever it does bring things more in line with git stash --\n<pathspec>, so I think this is an improvement.\n\n git-stash.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex c58f98f1d6..2d5c9a609c 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -322,7 +322,7 @@ push_stash () {\n \n \t\tif test \"$keep_index\" != \"t\"\n \t\tthen\n-\t\t\tgit reset ${GIT_QUIET:+-q}\n+\t\t\tgit reset ${GIT_QUIET:+-q} -- \"$@\"\n \t\tfi\n \tfi\n }\n-- \n2.12.0.483.gad4152297\n\n"},{"id":"314602","messageId":"20170319202351.8825-3-t.gummerer@gmail.com","threadId":"45407","inReplyTo":"20170319202351.8825-1-t.gummerer@gmail.com","subject":"[PATCH/RFC 2/3] stash: make push -p -q --no-keep-index quiet","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2017-03-19T20:23:50Z","receivedAt":"2017-03-19T20:24:25Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Currently when using \"git stash push -p -q --no-keep-index\", the -q flag\nis not passed to the git reset which is executed when --no-keep-index is\nalso passed in.  This means that git stash is somewhat verbose in this\nmode even when the -q flag is passed in.  This was always the case since\n\"git stash save -p\" was introduced in dda1f2a5 (\"Implement 'git stash\nsave --patch'\", 2009-08-13).\n\nProperly pass the -q flag on to git reset, to make \"git stash push -p\n-q\" as quiet as it should be.\n\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n git-stash.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 59f055e27b..c58f98f1d6 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -322,7 +322,7 @@ push_stash () {\n \n \t\tif test \"$keep_index\" != \"t\"\n \t\tthen\n-\t\t\tgit reset\n+\t\t\tgit reset ${GIT_QUIET:+-q}\n \t\tfi\n \tfi\n }\n-- \n2.12.0.483.gad4152297\n\n"},{"id":"314693","messageId":"xmqqk27jx2ej.fsf@gitster.mtv.corp.google.com","threadId":"45407","inReplyTo":"20170319202351.8825-2-t.gummerer@gmail.com","subject":"Re: [PATCH/RFC 1/3] stash: show less information for stash push -- <pathspec>","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-20T17:51:16Z","receivedAt":"2017-03-20T17:51:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer <t.gummerer@gmail.com> writes:\n\n> When using git stash push -- <pathspec> in the following sequence:\n>\n>        git init -q repo\n>        cd repo\n>\n>        for i in one two; do\n>                echo content >$i\n>                git add $i\n>        done\n>        git commit -qm base\n>\n>        for i in one two; do\n>                echo change >$i\n>        done\n>        git stash -- one\n>\n> it shows:\n>\n>    Saved working directory and index state WIP on master: 20cfadf base\n>    Unstaged changes after reset:\n>     M   one\n>     M   two\n>\n> Even though \"one\" no longer has unstaged changes.\n>\n> It really is enough for the user to know that the stash is created,\n> without bothering them with the internal details of what's happening.\n> Always pass the -q flag to git clean and git reset in the pathspec case,\n> to avoid unnecessary and potentially confusing output.\n>\n> Reported-by: Jeff King <peff@peff.net>\n> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n> ---\n>  git-stash.sh | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/git-stash.sh b/git-stash.sh\n> index 9c70662cc8..59f055e27b 100755\n> --- a/git-stash.sh\n> +++ b/git-stash.sh\n> @@ -299,10 +299,10 @@ push_stash () {\n>  \tthen\n>  \t\tif test $# != 0\n>  \t\tthen\n> -\t\t\tgit reset ${GIT_QUIET:+-q} -- \"$@\"\n> +\t\t\tgit reset -q -- \"$@\"\n>  \t\t\tgit ls-files -z --modified -- \"$@\" |\n>  \t\t\tgit checkout-index -z --force --stdin\n> -\t\t\tgit clean --force ${GIT_QUIET:+-q} -d -- \"$@\"\n> +\t\t\tgit clean --force -q -d -- \"$@\"\n>  \t\telse\n>  \t\t\tgit reset --hard ${GIT_QUIET:+-q}\n>  \t\tfi\n\nYup, we only said \"HEAD is now at ...\" in the non-pathspec case (and\nwe of course still do).  We didn't report changes to which paths\nhave been reverted in (or which new paths are removed from) the\nworking tree to the original state (and we of course still don't).\n\nThe messages from reset and clean that reports these probably do not\nneed to be shown by default to the users (they can always check with\n\"git stash show\" when they are curious or when they want to double\ncheck).\n\n\n\n\n"},{"id":"314704","messageId":"20170320184156.sji45up2aba7z3ws@sigill.intra.peff.net","threadId":"45407","inReplyTo":"20170318183658.GC27158@hank","subject":"Re: [BUG] \"git stash -- path\" reports wrong unstaged changes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-20T18:41:56Z","receivedAt":"2017-03-20T18:42:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 18, 2017 at 06:36:58PM +0000, Thomas Gummerer wrote:\n\n> > +\t\t\tif test -z \"$GIT_QUIET\" && ! git diff-files --quiet\n> > +\t\t\tthen\n> > +\t\t\t\tsay \"$(gettext \"Unstaged changes after reset:\")\"\n> > +\t\t\t\tgit diff-files --name-status\n> > +\t\t\tfi\n> >  \t\telse\n> >  \t\t\tgit reset --hard ${GIT_QUIET:+-q}\n> >  \t\tfi\n> \n> This would mean the user gets something like in your case above:\n> \n>     Unstaged changes after reset:\n>      M\ttwo\n> \n> As a user that doesn't know the internal implementation of push_stash,\n> this would make me wonder why git stash would mention a file that is\n> not provided as pathspec, but not the one that was provided in the\n> pathspec argument.\n\nThat's a good point. I was going for consistency with the non-pathspec\ncase, but of course it wouldn't mention any files in the first place,\nbecause it's just done a complete \"git reset --hard\".\n\n> I think one option would be to to just keep quiet about the exact\n> changes that git stash push makes, similar to what we do in the\n> --include-untracked and in the -p case.  The other option would be to\n> find the files that are affected and print them, but that would\n> probably be a bit too noisy especially in cases such as\n> git stash push -- docs/*.\n\nYeah, that's the right thing. I was just trying to be too clever above.\n\n-Peff\n"},{"id":"314705","messageId":"20170320184250.rkpgx4sl6zzqzelw@sigill.intra.peff.net","threadId":"45407","inReplyTo":"20170319202351.8825-2-t.gummerer@gmail.com","subject":"Re: [PATCH/RFC 1/3] stash: show less information for stash push -- <pathspec>","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-20T18:42:51Z","receivedAt":"2017-03-20T18:43:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 19, 2017 at 08:23:49PM +0000, Thomas Gummerer wrote:\n\n> When using git stash push -- <pathspec> in the following sequence:\n> \n>        git init -q repo\n>        cd repo\n> \n>        for i in one two; do\n>                echo content >$i\n>                git add $i\n>        done\n>        git commit -qm base\n> \n>        for i in one two; do\n>                echo change >$i\n>        done\n>        git stash -- one\n> \n> it shows:\n> \n>    Saved working directory and index state WIP on master: 20cfadf base\n>    Unstaged changes after reset:\n>     M   one\n>     M   two\n> \n> Even though \"one\" no longer has unstaged changes.\n> \n> It really is enough for the user to know that the stash is created,\n> without bothering them with the internal details of what's happening.\n> Always pass the -q flag to git clean and git reset in the pathspec case,\n> to avoid unnecessary and potentially confusing output.\n\nYeah, on further reflection, this is definitely the right thing to do.\n\n-Peff\n"},{"id":"314707","messageId":"20170320184855.x7m2gxwdqdt3lnet@sigill.intra.peff.net","threadId":"45407","inReplyTo":"xmqqk27jx2ej.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH/RFC 1/3] stash: show less information for stash push -- <pathspec>","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-20T18:48:55Z","receivedAt":"2017-03-20T18:49:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 20, 2017 at 10:51:16AM -0700, Junio C Hamano wrote:\n\n> > diff --git a/git-stash.sh b/git-stash.sh\n> > index 9c70662cc8..59f055e27b 100755\n> > --- a/git-stash.sh\n> > +++ b/git-stash.sh\n> > @@ -299,10 +299,10 @@ push_stash () {\n> >  \tthen\n> >  \t\tif test $# != 0\n> >  \t\tthen\n> > -\t\t\tgit reset ${GIT_QUIET:+-q} -- \"$@\"\n> > +\t\t\tgit reset -q -- \"$@\"\n> >  \t\t\tgit ls-files -z --modified -- \"$@\" |\n> >  \t\t\tgit checkout-index -z --force --stdin\n> > -\t\t\tgit clean --force ${GIT_QUIET:+-q} -d -- \"$@\"\n> > +\t\t\tgit clean --force -q -d -- \"$@\"\n> >  \t\telse\n> >  \t\t\tgit reset --hard ${GIT_QUIET:+-q}\n> >  \t\tfi\n> \n> Yup, we only said \"HEAD is now at ...\" in the non-pathspec case (and\n> we of course still do).  We didn't report changes to which paths\n> have been reverted in (or which new paths are removed from) the\n> working tree to the original state (and we of course still don't).\n> \n> The messages from reset and clean that reports these probably do not\n> need to be shown by default to the users (they can always check with\n> \"git stash show\" when they are curious or when they want to double\n> check).\n\nI'm not sure if you are arguing here that the non-pathspec case should\nmove to an unconditional \"-q\", too, to suppress the \"HEAD is now at\"\nmessage.  But I think that is a good suggestion. It would make the two\ncases consistent, and it is not really adding anything of value (it is\nalways just HEAD, and if you do not provide a custom message, the\nshort-sha1 and subject are already in the \"Saved...\" line above).\n\n-Peff\n"},{"id":"314709","messageId":"20170320185518.nspfukazbgmhzybd@sigill.intra.peff.net","threadId":"45407","inReplyTo":"20170319202351.8825-3-t.gummerer@gmail.com","subject":"Re: [PATCH/RFC 2/3] stash: make push -p -q --no-keep-index quiet","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-20T18:55:18Z","receivedAt":"2017-03-20T18:55:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 19, 2017 at 08:23:50PM +0000, Thomas Gummerer wrote:\n\n> Currently when using \"git stash push -p -q --no-keep-index\", the -q flag\n> is not passed to the git reset which is executed when --no-keep-index is\n> also passed in.  This means that git stash is somewhat verbose in this\n> mode even when the -q flag is passed in.  This was always the case since\n> \"git stash save -p\" was introduced in dda1f2a5 (\"Implement 'git stash\n> save --patch'\", 2009-08-13).\n> \n> Properly pass the -q flag on to git reset, to make \"git stash push -p\n> -q\" as quiet as it should be.\n\nYeah, this is an obvious bug-fix. Though given my earlier response to\nJunio, I wonder if we should just be doing \"reset -q\" unconditionally\nfor most of these calls.\n\nI guess this one does say more than just \"HEAD is at...\"; it mentions\nthe files that you _didn't_ pick. Though that now makes it inconsistent\nwith the pathspec case, especially if you do:\n\n  git stash push -p --no-keep-index -- one\n\nwhich will mention \"two\" as remaining with unstaged changes.\n\n-Peff\n"},{"id":"314711","messageId":"20170320190843.aw3fsenjvcek7a44@sigill.intra.peff.net","threadId":"45407","inReplyTo":"20170319202351.8825-4-t.gummerer@gmail.com","subject":"Re: [PATCH/RFC 3/3] stash: pass the pathspec argument to git reset","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-20T19:08:43Z","receivedAt":"2017-03-20T19:09:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 19, 2017 at 08:23:51PM +0000, Thomas Gummerer wrote:\n\n> For \"git stash -p --no-keep-index\", the pathspec argument is currently\n> not passed to \"git reset\".  This means that changed that are staged but\n> that are excluded from the pathspec still get unstaged by git stash -p.\n\nYeah, I noticed this while playing around with patch 2. This seems\nlike an improvement to me. Unlike the other patches (which are just\ntweaking quietness), I think this one really needs a test.\n\nAlso, s/changed/changes/ above.\n\n> ---\n> So this make things a bit inconsistent in for example the following\n> situation:\n> \n>     $ git status -s\n>      M git-stash.sh\n>     M  read-cache.c\n> \n> where using git stash -p --no-keep-index, when only changes in\n> git-stash.sh are added to the stash would reset read-cache.c, but with\n> git stash -p --no-keep-index -- git-stash.sh, read-cache.c would not\n> be reset.\n\nI think it's OK. You can't select (or not select) changes from the index\nanyway. TBH, I kind of wonder if \"git stash -p --no-keep-index\" makes\nany sense at all.\n\n> However it does bring things more in line with git stash --\n> <pathspec>, so I think this is an improvement.\n\nI did notice one other case while looking at this that I think is much\nmore serious. The \"read-tree\" call in the non-patch-mode case doesn't\nuse a pathspec either. So if we have our same setup where \"one\" and\n\"two\" have unstaged changes and we do:\n\n  git stash push -k one\n\nThen we stash \"one\", but the change in \"two\" is wiped out completely!\n\nI don't think read-tree takes pathspecs, so it would have to drop the\n\"-u\" and replace it with checkout-index. I'm confused about why we would\nneed it in the first place, though. We should have already dealt with\nthe working tree files in the earlier block, so there should be nothing\nto checkout.\n\nThe \"-u\" goes all the way back to 7bedebcaa (stash: introduce 'stash\nsave --keep-index' option, 2008-06-27). I wonder if it has always been\nunnecessary, but we never noticed because it's just a noop.\n\n-Peff\n"},{"id":"314721","messageId":"xmqqtw6nvip1.fsf@gitster.mtv.corp.google.com","threadId":"45407","inReplyTo":"20170320184855.x7m2gxwdqdt3lnet@sigill.intra.peff.net","subject":"Re: [PATCH/RFC 1/3] stash: show less information for stash push -- <pathspec>","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-20T19:42:18Z","receivedAt":"2017-03-20T20:03:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, Mar 20, 2017 at 10:51:16AM -0700, Junio C Hamano wrote:\n>\n>> > diff --git a/git-stash.sh b/git-stash.sh\n>> > index 9c70662cc8..59f055e27b 100755\n>> > --- a/git-stash.sh\n>> > +++ b/git-stash.sh\n>> > @@ -299,10 +299,10 @@ push_stash () {\n>> >  \tthen\n>> >  \t\tif test $# != 0\n>> >  \t\tthen\n>> > -\t\t\tgit reset ${GIT_QUIET:+-q} -- \"$@\"\n>> > +\t\t\tgit reset -q -- \"$@\"\n>> >  \t\t\tgit ls-files -z --modified -- \"$@\" |\n>> >  \t\t\tgit checkout-index -z --force --stdin\n>> > -\t\t\tgit clean --force ${GIT_QUIET:+-q} -d -- \"$@\"\n>> > +\t\t\tgit clean --force -q -d -- \"$@\"\n>> >  \t\telse\n>> >  \t\t\tgit reset --hard ${GIT_QUIET:+-q}\n>> >  \t\tfi\n>> \n>> Yup, we only said \"HEAD is now at ...\" in the non-pathspec case (and\n>> we of course still do).  We didn't report changes to which paths\n>> have been reverted in (or which new paths are removed from) the\n>> working tree to the original state (and we of course still don't).\n>> \n>> The messages from reset and clean that reports these probably do not\n>> need to be shown by default to the users (they can always check with\n>> \"git stash show\" when they are curious or when they want to double\n>> check).\n>\n> I'm not sure if you are arguing here that the non-pathspec case should\n> move to an unconditional \"-q\", too, to suppress the \"HEAD is now at\"\n> message.  But I think that is a good suggestion. It would make the two\n> cases consistent, and it is not really adding anything of value (it is\n> always just HEAD, and if you do not provide a custom message, the\n> short-sha1 and subject are already in the \"Saved...\" line above).\n\nI wasn't suggesting it (I was just saying that these extra messages\nare not something we found necessary for consistency with the\noriginal codepath when we added the pathspec support).  I wasn't\neven thinking about what the original codepath did, i.e. when the\ncommand is run without pathspec.\n\nI too suspect that most of the ${GIT_QUIET:+-q} can just become an\nunconditional -q as you do.\n\n\n"},{"id":"314865","messageId":"20170321204842.GD27158@hank","threadId":"45407","inReplyTo":"xmqqtw6nvip1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH/RFC 1/3] stash: show less information for stash push -- <pathspec>","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2017-03-21T20:48:42Z","receivedAt":"2017-03-21T20:48:35Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 03/20, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n> \n> > On Mon, Mar 20, 2017 at 10:51:16AM -0700, Junio C Hamano wrote:\n> >\n> >> > diff --git a/git-stash.sh b/git-stash.sh\n> >> > index 9c70662cc8..59f055e27b 100755\n> >> > --- a/git-stash.sh\n> >> > +++ b/git-stash.sh\n> >> > @@ -299,10 +299,10 @@ push_stash () {\n> >> >  \tthen\n> >> >  \t\tif test $# != 0\n> >> >  \t\tthen\n> >> > -\t\t\tgit reset ${GIT_QUIET:+-q} -- \"$@\"\n> >> > +\t\t\tgit reset -q -- \"$@\"\n> >> >  \t\t\tgit ls-files -z --modified -- \"$@\" |\n> >> >  \t\t\tgit checkout-index -z --force --stdin\n> >> > -\t\t\tgit clean --force ${GIT_QUIET:+-q} -d -- \"$@\"\n> >> > +\t\t\tgit clean --force -q -d -- \"$@\"\n> >> >  \t\telse\n> >> >  \t\t\tgit reset --hard ${GIT_QUIET:+-q}\n> >> >  \t\tfi\n> >> \n> >> Yup, we only said \"HEAD is now at ...\" in the non-pathspec case (and\n> >> we of course still do).  We didn't report changes to which paths\n> >> have been reverted in (or which new paths are removed from) the\n> >> working tree to the original state (and we of course still don't).\n> >> \n> >> The messages from reset and clean that reports these probably do not\n> >> need to be shown by default to the users (they can always check with\n> >> \"git stash show\" when they are curious or when they want to double\n> >> check).\n> >\n> > I'm not sure if you are arguing here that the non-pathspec case should\n> > move to an unconditional \"-q\", too, to suppress the \"HEAD is now at\"\n> > message.  But I think that is a good suggestion. It would make the two\n> > cases consistent, and it is not really adding anything of value (it is\n> > always just HEAD, and if you do not provide a custom message, the\n> > short-sha1 and subject are already in the \"Saved...\" line above).\n> \n> I wasn't suggesting it (I was just saying that these extra messages\n> are not something we found necessary for consistency with the\n> original codepath when we added the pathspec support).  I wasn't\n> even thinking about what the original codepath did, i.e. when the\n> command is run without pathspec.\n> \n> I too suspect that most of the ${GIT_QUIET:+-q} can just become an\n> unconditional -q as you do.\n\nThanks both, I do agree that passing -q unconditionally is probably\nthe right thing to do.  Will do that, and also pass -q unconditionally\nto the git reset I addressed in 2/3 here in the re-roll.\n"},{"id":"314867","messageId":"20170321211424.GE27158@hank","threadId":"45407","inReplyTo":"20170320190843.aw3fsenjvcek7a44@sigill.intra.peff.net","subject":"Re: [PATCH/RFC 3/3] stash: pass the pathspec argument to git reset","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2017-03-21T21:14:24Z","receivedAt":"2017-03-21T21:14:18Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 03/20, Jeff King wrote:\n> On Sun, Mar 19, 2017 at 08:23:51PM +0000, Thomas Gummerer wrote:\n> \n> > For \"git stash -p --no-keep-index\", the pathspec argument is currently\n> > not passed to \"git reset\".  This means that changed that are staged but\n> > that are excluded from the pathspec still get unstaged by git stash -p.\n> \n> Yeah, I noticed this while playing around with patch 2. This seems\n> like an improvement to me. Unlike the other patches (which are just\n> tweaking quietness), I think this one really needs a test.\n>\n> Also, s/changed/changes/ above.\n\nThanks.  Will add a test in the re-roll.\n\n> > ---\n> > So this make things a bit inconsistent in for example the following\n> > situation:\n> > \n> >     $ git status -s\n> >      M git-stash.sh\n> >     M  read-cache.c\n> > \n> > where using git stash -p --no-keep-index, when only changes in\n> > git-stash.sh are added to the stash would reset read-cache.c, but with\n> > git stash -p --no-keep-index -- git-stash.sh, read-cache.c would not\n> > be reset.\n> \n> I think it's OK. You can't select (or not select) changes from the index\n> anyway. TBH, I kind of wonder if \"git stash -p --no-keep-index\" makes\n> any sense at all.\n\nYeah I don't have any use case for it, but maybe someone does, so I\nthink fixing it this way now is the best way forward.\n\n> > However it does bring things more in line with git stash --\n> > <pathspec>, so I think this is an improvement.\n> \n> I did notice one other case while looking at this that I think is much\n> more serious. The \"read-tree\" call in the non-patch-mode case doesn't\n> use a pathspec either. So if we have our same setup where \"one\" and\n> \"two\" have unstaged changes and we do:\n> \n>   git stash push -k one\n> \n> Then we stash \"one\", but the change in \"two\" is wiped out completely!\n\nGood catch, that's definitely another bug :/ \n\n> I don't think read-tree takes pathspecs, so it would have to drop the\n> \"-u\" and replace it with checkout-index. I'm confused about why we would\n> need it in the first place, though. We should have already dealt with\n> the working tree files in the earlier block, so there should be nothing\n> to checkout.\n> \n> The \"-u\" goes all the way back to 7bedebcaa (stash: introduce 'stash\n> save --keep-index' option, 2008-06-27). I wonder if it has always been\n> unnecessary, but we never noticed because it's just a noop.\n\nI do think the \"-u\" is necessary.  Say we have a repository with\nchanges in 'foo' and 'bar', where the changes in bar are added to the\nindex.\n\nThen 'git stash -k' would wipe out the changes in both 'foo' and 'bar'\nthrough 'git reset --hard', but we do want the changes in 'bar' to\nstill exist on disk, which they wouldn't without the \"-u\".\n\nBut I'll replace the \"-u\" with checkout-index, so we can respect the\npathspec.\n\nThanks!\n\n> -Peff\n"},{"id":"314871","messageId":"20170321221219.28041-1-t.gummerer@gmail.com","threadId":"45407","inReplyTo":"20170319202351.8825-1-t.gummerer@gmail.com","subject":"[PATCH v2 0/3] Re: [BUG] \"git stash -- path\" reports wrong unstaged changes","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2017-03-21T22:12:16Z","receivedAt":"2017-03-21T22:12:25Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Thanks Junio and Peff for comments and suggestions on the previous\nround.\n\nChanges since v1:\n- 1/3 now contains the changes that were previously split up into 1/3\n  and 2/3, and also makes git reset --hard in the simple 'git stash'\n  case quiet unconditionally.\n- 2/3 is what was 3/3 in the previous round, and now includes a test\n  for the expected behaviour.\n- 3/3 is the fix for the bug Peff noticed for git stash -k with\n  changes that are not yet in the index.\n\nThomas Gummerer (3):\n  stash: don't show internal implementation details\n  stash: pass the pathspec argument to git reset\n  stash: keep untracked files intact in stash -k\n\n git-stash.sh           | 12 +++++++-----\n t/t3903-stash.sh       | 16 +++++++++++++++-\n t/t3904-stash-patch.sh |  8 ++++++++\n 3 files changed, 30 insertions(+), 6 deletions(-)\n\n-- \n2.12.0.401.g98d3b1bb99.dirty\n\n"},{"id":"314872","messageId":"20170321221219.28041-3-t.gummerer@gmail.com","threadId":"45407","inReplyTo":"20170321221219.28041-1-t.gummerer@gmail.com","subject":"[PATCH v2 2/3] stash: pass the pathspec argument to git reset","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2017-03-21T22:12:18Z","receivedAt":"2017-03-21T22:12:27Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"For \"git stash -p --no-keep-index\", the pathspec argument is currently\nnot passed to \"git reset\".  This means that changes that are staged but\nthat are excluded from the pathspec still get unstaged by git stash -p.\n\nMake sure that doesn't happen by passing the pathspec argument to the\ngit reset in question, bringing the behaviour in line with \"git stash --\n<pathspec>\".\n\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n git-stash.sh           | 2 +-\n t/t3904-stash-patch.sh | 8 ++++++++\n 2 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex ba86d84321..13711764a9 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -322,7 +322,7 @@ push_stash () {\n \n \t\tif test \"$keep_index\" != \"t\"\n \t\tthen\n-\t\t\tgit reset -q\n+\t\t\tgit reset -q -- \"$@\"\n \t\tfi\n \tfi\n }\ndiff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh\nindex 38e730090f..83744f8c93 100755\n--- a/t/t3904-stash-patch.sh\n+++ b/t/t3904-stash-patch.sh\n@@ -77,6 +77,14 @@ test_expect_success 'git stash --no-keep-index -p' '\n \tverify_state dir/foo work index\n '\n \n+test_expect_success 'stash -p --no-keep-index -- <pathspec> does not unstage other files' '\n+\tset_state HEAD HEADfile_work HEADfile_index &&\n+\tset_state dir/foo work index &&\n+\techo y | git stash push -p --no-keep-index -- HEAD &&\n+\tverify_state HEAD committed committed &&\n+\tverify_state dir/foo work index\n+'\n+\n test_expect_success 'none of this moved HEAD' '\n \tverify_saved_head\n '\n-- \n2.12.0.401.g98d3b1bb99.dirty\n\n"},{"id":"314873","messageId":"20170321221219.28041-2-t.gummerer@gmail.com","threadId":"45407","inReplyTo":"20170321221219.28041-1-t.gummerer@gmail.com","subject":"[PATCH v2 1/3] stash: don't show internal implementation details","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2017-03-21T22:12:17Z","receivedAt":"2017-03-21T22:12:28Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"git stash push uses other git commands internally.  Currently it only\npasses the -q flag to those if the -q flag is passed to git stash.  when\nusing 'git stash push -p -q --no-keep-index', it doesn't even pass the\nflag on to the internal reset at all.\n\nIt really is enough for the user to know that the stash is created,\nwithout bothering them with the internal details of what's happening.\nAlways pass the -q flag to the internal git clean and git reset\ncommands, to avoid unnecessary and potentially confusing output.\n\nReported-by: Jeff King <peff@peff.net>\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n git-stash.sh     | 8 ++++----\n t/t3903-stash.sh | 2 +-\n 2 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 9c70662cc8..ba86d84321 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -299,12 +299,12 @@ push_stash () {\n \tthen\n \t\tif test $# != 0\n \t\tthen\n-\t\t\tgit reset ${GIT_QUIET:+-q} -- \"$@\"\n+\t\t\tgit reset -q -- \"$@\"\n \t\t\tgit ls-files -z --modified -- \"$@\" |\n \t\t\tgit checkout-index -z --force --stdin\n-\t\t\tgit clean --force ${GIT_QUIET:+-q} -d -- \"$@\"\n+\t\t\tgit clean --force -q -d -- \"$@\"\n \t\telse\n-\t\t\tgit reset --hard ${GIT_QUIET:+-q}\n+\t\t\tgit reset --hard -q\n \t\tfi\n \t\ttest \"$untracked\" = \"all\" && CLEAN_X_OPTION=-x || CLEAN_X_OPTION=\n \t\tif test -n \"$untracked\"\n@@ -322,7 +322,7 @@ push_stash () {\n \n \t\tif test \"$keep_index\" != \"t\"\n \t\tthen\n-\t\t\tgit reset\n+\t\t\tgit reset -q\n \t\tfi\n \tfi\n }\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 89877e4b52..ea8e5c7818 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -663,7 +663,7 @@ test_expect_success 'stash apply shows status same as git status (relative to cu\n \t\tsane_unset GIT_MERGE_VERBOSITY &&\n \t\tgit stash apply\n \t) |\n-\tsed -e 1,2d >actual && # drop \"Saved...\" and \"HEAD is now...\"\n+\tsed -e 1,1d >actual && # drop \"Saved...\"\n \ttest_i18ncmp expect actual\n '\n \n-- \n2.12.0.401.g98d3b1bb99.dirty\n\n"},{"id":"314874","messageId":"20170321221219.28041-4-t.gummerer@gmail.com","threadId":"45407","inReplyTo":"20170321221219.28041-1-t.gummerer@gmail.com","subject":"[PATCH v2 3/3] stash: keep untracked files intact in stash -k","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2017-03-21T22:12:19Z","receivedAt":"2017-03-21T22:12:30Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Currently when there are untracked changes in a file \"one\" and in a file\n\"two\" in the repository and the user uses:\n\n    git stash push -k one\n\nall changes in \"two\" are wiped out completely.  That is clearly not the\nintended result.  Make sure that only the files given in the pathspec\nare changed when git stash push -k <pathspec> is used.\n\nReported-by: Jeff King <peff@peff.net>\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n git-stash.sh     |  4 +++-\n t/t3903-stash.sh | 14 ++++++++++++++\n 2 files changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 13711764a9..2fb651b2b8 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -314,7 +314,9 @@ push_stash () {\n \n \t\tif test \"$keep_index\" = \"t\" && test -n \"$i_tree\"\n \t\tthen\n-\t\t\tgit read-tree --reset -u $i_tree\n+\t\t\tgit read-tree --reset $i_tree\n+\t\t\tgit ls-files -z --modified -- \"$@\" |\n+\t\t\tgit checkout-index -z --force --stdin\n \t\tfi\n \telse\n \t\tgit apply -R < \"$TMP-patch\" ||\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex ea8e5c7818..6f6e31cc6d 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -907,4 +907,18 @@ test_expect_success 'stash without verb with pathspec' '\n \ttest_path_is_file bar\n '\n \n+test_expect_success 'stash -k -- <pathspec> leaves unstaged files intact' '\n+\tgit reset &&\n+\t>foo &&\n+\t>bar &&\n+\tgit add foo bar &&\n+\tgit commit -m \"test\" &&\n+\techo \"foo\" >foo &&\n+\techo \"bar\" >bar &&\n+\tgit stash -k -- foo &&\n+\ttest \"\",bar = $(cat foo),$(cat bar) &&\n+\tgit stash pop &&\n+\ttest foo,bar = $(cat foo),$(cat bar)\n+'\n+\n test_done\n-- \n2.12.0.401.g98d3b1bb99.dirty\n\n"},{"id":"314875","messageId":"20170321221252.eslboc7kcldg6o43@sigill.intra.peff.net","threadId":"45407","inReplyTo":"20170321211424.GE27158@hank","subject":"Re: [PATCH/RFC 3/3] stash: pass the pathspec argument to git reset","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-21T22:12:52Z","receivedAt":"2017-03-21T22:13:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 21, 2017 at 09:14:24PM +0000, Thomas Gummerer wrote:\n\n> > I don't think read-tree takes pathspecs, so it would have to drop the\n> > \"-u\" and replace it with checkout-index. I'm confused about why we would\n> > need it in the first place, though. We should have already dealt with\n> > the working tree files in the earlier block, so there should be nothing\n> > to checkout.\n> > \n> > The \"-u\" goes all the way back to 7bedebcaa (stash: introduce 'stash\n> > save --keep-index' option, 2008-06-27). I wonder if it has always been\n> > unnecessary, but we never noticed because it's just a noop.\n> \n> I do think the \"-u\" is necessary.  Say we have a repository with\n> changes in 'foo' and 'bar', where the changes in bar are added to the\n> index.\n> \n> Then 'git stash -k' would wipe out the changes in both 'foo' and 'bar'\n> through 'git reset --hard', but we do want the changes in 'bar' to\n> still exist on disk, which they wouldn't without the \"-u\".\n\nYeah, you're right. It makes me wonder if the \"-k\" case should be\nskipping the \"reset --hard\". But as this series has shown, this\nprocedure is sufficiently tricky that it may be a good idea to make the\nminimal change.\n\n> But I'll replace the \"-u\" with checkout-index, so we can respect the\n> pathspec.\n\nThat makes sense.\n\n-Peff\n"},{"id":"314876","messageId":"20170321221445.3jpaoce7qbshtdoq@sigill.intra.peff.net","threadId":"45407","inReplyTo":"20170321221219.28041-2-t.gummerer@gmail.com","subject":"Re: [PATCH v2 1/3] stash: don't show internal implementation details","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-21T22:14:46Z","receivedAt":"2017-03-21T22:14:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 21, 2017 at 10:12:17PM +0000, Thomas Gummerer wrote:\n\n> git stash push uses other git commands internally.  Currently it only\n> passes the -q flag to those if the -q flag is passed to git stash.  when\n> using 'git stash push -p -q --no-keep-index', it doesn't even pass the\n> flag on to the internal reset at all.\n> \n> It really is enough for the user to know that the stash is created,\n> without bothering them with the internal details of what's happening.\n> Always pass the -q flag to the internal git clean and git reset\n> commands, to avoid unnecessary and potentially confusing output.\n> \n> Reported-by: Jeff King <peff@peff.net>\n> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n\nI think combining these is fine. The incorrect output with pathspecs\nisn't mentioned anymore, but I think that's OK.\n\n> diff --git a/git-stash.sh b/git-stash.sh\n> index 9c70662cc8..ba86d84321 100755\n> --- a/git-stash.sh\n> +++ b/git-stash.sh\n> @@ -299,12 +299,12 @@ push_stash () {\n>  \tthen\n>  \t\tif test $# != 0\n>  \t\tthen\n> -\t\t\tgit reset ${GIT_QUIET:+-q} -- \"$@\"\n> +\t\t\tgit reset -q -- \"$@\"\n>  \t\t\tgit ls-files -z --modified -- \"$@\" |\n>  \t\t\tgit checkout-index -z --force --stdin\n> -\t\t\tgit clean --force ${GIT_QUIET:+-q} -d -- \"$@\"\n> +\t\t\tgit clean --force -q -d -- \"$@\"\n>  \t\telse\n> -\t\t\tgit reset --hard ${GIT_QUIET:+-q}\n> +\t\t\tgit reset --hard -q\n>  \t\tfi\n>  \t\ttest \"$untracked\" = \"all\" && CLEAN_X_OPTION=-x || CLEAN_X_OPTION=\n>  \t\tif test -n \"$untracked\"\n> @@ -322,7 +322,7 @@ push_stash () {\n>  \n>  \t\tif test \"$keep_index\" != \"t\"\n>  \t\tthen\n> -\t\t\tgit reset\n> +\t\t\tgit reset -q\n>  \t\tfi\n>  \tfi\n>  }\n\nThese all look fine.\n\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index 89877e4b52..ea8e5c7818 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -663,7 +663,7 @@ test_expect_success 'stash apply shows status same as git status (relative to cu\n>  \t\tsane_unset GIT_MERGE_VERBOSITY &&\n>  \t\tgit stash apply\n>  \t) |\n> -\tsed -e 1,2d >actual && # drop \"Saved...\" and \"HEAD is now...\"\n> +\tsed -e 1,1d >actual && # drop \"Saved...\"\n>  \ttest_i18ncmp expect actual\n>  '\n\nThis too, though I think \"1d\" would be the more usual way to say it.\n\n-peff\n"},{"id":"314877","messageId":"20170321221959.dtkz2i33jyojwzca@sigill.intra.peff.net","threadId":"45407","inReplyTo":"20170321221219.28041-3-t.gummerer@gmail.com","subject":"Re: [PATCH v2 2/3] stash: pass the pathspec argument to git reset","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-21T22:19:59Z","receivedAt":"2017-03-21T22:20:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 21, 2017 at 10:12:18PM +0000, Thomas Gummerer wrote:\n\n> For \"git stash -p --no-keep-index\", the pathspec argument is currently\n> not passed to \"git reset\".  This means that changes that are staged but\n> that are excluded from the pathspec still get unstaged by git stash -p.\n> \n> Make sure that doesn't happen by passing the pathspec argument to the\n> git reset in question, bringing the behaviour in line with \"git stash --\n> <pathspec>\".\n> \n> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n> ---\n>  git-stash.sh           | 2 +-\n>  t/t3904-stash-patch.sh | 8 ++++++++\n>  2 files changed, 9 insertions(+), 1 deletion(-)\n\nThis one looks good to me.\n\n-Peff\n"},{"id":"314880","messageId":"20170321223809.c7wik5lfjylno6wn@sigill.intra.peff.net","threadId":"45407","inReplyTo":"20170321221219.28041-4-t.gummerer@gmail.com","subject":"Re: [PATCH v2 3/3] stash: keep untracked files intact in stash -k","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-21T22:38:09Z","receivedAt":"2017-03-21T22:38:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 21, 2017 at 10:12:19PM +0000, Thomas Gummerer wrote:\n\n> Currently when there are untracked changes in a file \"one\" and in a file\n> \"two\" in the repository and the user uses:\n> \n>     git stash push -k one\n> \n> all changes in \"two\" are wiped out completely.  That is clearly not the\n> intended result.  Make sure that only the files given in the pathspec\n> are changed when git stash push -k <pathspec> is used.\n\nGood description.\n\n> diff --git a/git-stash.sh b/git-stash.sh\n> index 13711764a9..2fb651b2b8 100755\n> --- a/git-stash.sh\n> +++ b/git-stash.sh\n> @@ -314,7 +314,9 @@ push_stash () {\n>  \n>  \t\tif test \"$keep_index\" = \"t\" && test -n \"$i_tree\"\n>  \t\tthen\n> -\t\t\tgit read-tree --reset -u $i_tree\n> +\t\t\tgit read-tree --reset $i_tree\n> +\t\t\tgit ls-files -z --modified -- \"$@\" |\n> +\t\t\tgit checkout-index -z --force --stdin\n>  \t\tfi\n\nI briefly wondered if this needed \"-q\" to match the earlier commit, but\n\"checkout-index\" isn't really chatty, so I don't think so (and the\nearlier checkout-index doesn't have it either).\n\nI also wondered if this could be done in a single command as:\n\n  git reset -q --hard $i_tree -- \"$@\"\n\nBut \"git reset\" can't handle pathspecs with \"--hard\" (which is why the\nsimilar case a few lines above uses the same commands).\n\nSo this looks good to me.\n\n> +test_expect_success 'stash -k -- <pathspec> leaves unstaged files intact' '\n> +\tgit reset &&\n> +\t>foo &&\n> +\t>bar &&\n> +\tgit add foo bar &&\n> +\tgit commit -m \"test\" &&\n> +\techo \"foo\" >foo &&\n> +\techo \"bar\" >bar &&\n> +\tgit stash -k -- foo &&\n> +\ttest \"\",bar = $(cat foo),$(cat bar) &&\n> +\tgit stash pop &&\n> +\ttest foo,bar = $(cat foo),$(cat bar)\n> +'\n\nI always get nervous when I see test arguments without quotes, but I\nthink this is fine (and I couldn't see a shorter way of doing it with\ntest_cmp).\n\n-Peff\n"},{"id":"314977","messageId":"20170322213347.GF27158@hank","threadId":"45407","inReplyTo":"20170321221445.3jpaoce7qbshtdoq@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/3] stash: don't show internal implementation details","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2017-03-22T21:33:47Z","receivedAt":"2017-03-22T21:34:49Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 03/21, Jeff King wrote:\n> On Tue, Mar 21, 2017 at 10:12:17PM +0000, Thomas Gummerer wrote:\n> \n> > git stash push uses other git commands internally.  Currently it only\n> > passes the -q flag to those if the -q flag is passed to git stash.  when\n> > using 'git stash push -p -q --no-keep-index', it doesn't even pass the\n> > flag on to the internal reset at all.\n> > \n> > It really is enough for the user to know that the stash is created,\n> > without bothering them with the internal details of what's happening.\n> > Always pass the -q flag to the internal git clean and git reset\n> > commands, to avoid unnecessary and potentially confusing output.\n> > \n> > Reported-by: Jeff King <peff@peff.net>\n> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n> \n> I think combining these is fine. The incorrect output with pathspecs\n> isn't mentioned anymore, but I think that's OK.\n\nYeah, they felt so similar now that splitting this up in different\npatches would be a bit too noisy.\n\n> [...]\n> \n> > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> > index 89877e4b52..ea8e5c7818 100755\n> > --- a/t/t3903-stash.sh\n> > +++ b/t/t3903-stash.sh\n> > @@ -663,7 +663,7 @@ test_expect_success 'stash apply shows status same as git status (relative to cu\n> >  \t\tsane_unset GIT_MERGE_VERBOSITY &&\n> >  \t\tgit stash apply\n> >  \t) |\n> > -\tsed -e 1,2d >actual && # drop \"Saved...\" and \"HEAD is now...\"\n> > +\tsed -e 1,1d >actual && # drop \"Saved...\"\n> >  \ttest_i18ncmp expect actual\n> >  '\n> \n> This too, though I think \"1d\" would be the more usual way to say it.\n\nRight thanks, I'll keep that in mind for another time. (I guess just\nchanging this doesn't warrant a re-roll?)\n\n> -peff\n"},{"id":"314980","messageId":"20170322214139.3nqxdzfxar5nqawo@sigill.intra.peff.net","threadId":"45407","inReplyTo":"20170322213347.GF27158@hank","subject":"Re: [PATCH v2 1/3] stash: don't show internal implementation details","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-22T21:41:39Z","receivedAt":"2017-03-22T21:41:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 22, 2017 at 09:33:47PM +0000, Thomas Gummerer wrote:\n\n> > > -\tsed -e 1,2d >actual && # drop \"Saved...\" and \"HEAD is now...\"\n> > > +\tsed -e 1,1d >actual && # drop \"Saved...\"\n> > >  \ttest_i18ncmp expect actual\n> > >  '\n> > \n> > This too, though I think \"1d\" would be the more usual way to say it.\n> \n> Right thanks, I'll keep that in mind for another time. (I guess just\n> changing this doesn't warrant a re-roll?)\n\nNah, I don't think it's important enough (and I give even odds that\nJunio may just tweak it while applying).\n\n-Peff\n"},{"id":"314982","messageId":"20170322214634.GG27158@hank","threadId":"45407","inReplyTo":"20170321223809.c7wik5lfjylno6wn@sigill.intra.peff.net","subject":"Re: [PATCH v2 3/3] stash: keep untracked files intact in stash -k","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2017-03-22T21:46:34Z","receivedAt":"2017-03-22T21:46:22Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 03/21, Jeff King wrote:\n> On Tue, Mar 21, 2017 at 10:12:19PM +0000, Thomas Gummerer wrote:\n> \n> > Currently when there are untracked changes in a file \"one\" and in a file\n> > \"two\" in the repository and the user uses:\n> > \n> >     git stash push -k one\n> > \n> > all changes in \"two\" are wiped out completely.  That is clearly not the\n> > intended result.  Make sure that only the files given in the pathspec\n> > are changed when git stash push -k <pathspec> is used.\n> \n> Good description.\n\nI basically just tweaked your report here :)  thanks for that!\n\n> > diff --git a/git-stash.sh b/git-stash.sh\n> > index 13711764a9..2fb651b2b8 100755\n> > --- a/git-stash.sh\n> > +++ b/git-stash.sh\n> > @@ -314,7 +314,9 @@ push_stash () {\n> >  \n> >  \t\tif test \"$keep_index\" = \"t\" && test -n \"$i_tree\"\n> >  \t\tthen\n> > -\t\t\tgit read-tree --reset -u $i_tree\n> > +\t\t\tgit read-tree --reset $i_tree\n> > +\t\t\tgit ls-files -z --modified -- \"$@\" |\n> > +\t\t\tgit checkout-index -z --force --stdin\n> >  \t\tfi\n> \n> I briefly wondered if this needed \"-q\" to match the earlier commit, but\n> \"checkout-index\" isn't really chatty, so I don't think so (and the\n> earlier checkout-index doesn't have it either).\n\nYep, I had the same thoughts here.\n\n> I also wondered if this could be done in a single command as:\n> \n>   git reset -q --hard $i_tree -- \"$@\"\n> \n> But \"git reset\" can't handle pathspecs with \"--hard\" (which is why the\n> similar case a few lines above uses the same commands).\n\nYeah unfortunately, that would have made the previous patch series\nquite a bit easier :)  But here it wouldn't help anyway, as git reset\nwithout pathspecs can't handle a tree-ish either, right?  And we also\nwant to handle the case where no pathspecs are given to git stash.\n\n> \n> So this looks good to me.\n> \n> > +test_expect_success 'stash -k -- <pathspec> leaves unstaged files intact' '\n> > +\tgit reset &&\n> > +\t>foo &&\n> > +\t>bar &&\n> > +\tgit add foo bar &&\n> > +\tgit commit -m \"test\" &&\n> > +\techo \"foo\" >foo &&\n> > +\techo \"bar\" >bar &&\n> > +\tgit stash -k -- foo &&\n> > +\ttest \"\",bar = $(cat foo),$(cat bar) &&\n> > +\tgit stash pop &&\n> > +\ttest foo,bar = $(cat foo),$(cat bar)\n> > +'\n> \n> I always get nervous when I see test arguments without quotes, but I\n> think this is fine (and I couldn't see a shorter way of doing it with\n> test_cmp).\n\nHmm yeah I basically copied this from a different test and tweaked it\na bit to fit here.\n\n> -Peff\n"}]}