{"thread":{"id":"35438","subject":"[PATCH] stash: handle specifying stashes with spaces","startedAt":"2013-11-29T19:22:59Z","lastAt":"2013-12-01T00:41:56Z","messageCount":6,"participants":["Øystein Walle","Thomas Rast","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"231277","messageId":"1385752979-28162-1-git-send-email-oystwa@gmail.com","threadId":"35438","inReplyTo":null,"subject":"[PATCH] stash: handle specifying stashes with spaces","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2013-11-29T19:22:59Z","receivedAt":"2013-11-29T19:22:59Z","isPatch":true,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"When trying to pop/apply a stash specified with an argument containing\nspaces the user will see an error:\n\n    $ git stash pop 'stash@{two hours ago}'\n    Too many revisions specified: stash@{two hours ago}\n\nThis happens because word splitting is used to count non-option\narguments. Instead shift the positional arguments as the options are\nprocessed; the number of arguments left is then the number we're after.\nAdd quotes where necessary.\n\nAlso add a test that verifies correct behaviour.\n\nSigned-off-by: Øystein Walle <oystwa@gmail.com>\n---\nThis is perhaps an esoteric use case but it's still worth fixing in my opinion.\nIt also saves a fork/exec so I see it as a win-win :)\n\nComments welcome, of course. Especially on the test. I couldn't use a relative\ndate spec since the commits and stashes are made with bogus timestamps and the\nspec is compared to the local time. It looks a bit icky the way it's written\nnow.\n\n git-stash.sh     | 20 ++++++++++----------\n t/t3903-stash.sh | 11 +++++++++++\n 2 files changed, 21 insertions(+), 10 deletions(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 1e541a2..0a48d42 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -358,26 +358,25 @@ parse_flags_and_rev()\n \ti_tree=\n \tu_tree=\n \n-\tREV=$(git rev-parse --no-flags --symbolic \"$@\") || exit 1\n-\n \tFLAGS=\n \tfor opt\n \tdo\n \t\tcase \"$opt\" in\n \t\t\t-q|--quiet)\n \t\t\t\tGIT_QUIET=-t\n+\t\t\t\tshift\n \t\t\t;;\n \t\t\t--index)\n \t\t\t\tINDEX_OPTION=--index\n+\t\t\t\tshift\n \t\t\t;;\n \t\t\t-*)\n \t\t\t\tFLAGS=\"${FLAGS}${FLAGS:+ }$opt\"\n+\t\t\t\tshift\n \t\t\t;;\n \t\tesac\n \tdone\n \n-\tset -- $REV\n-\n \tcase $# in\n \t\t0)\n \t\t\thave_stash || die \"$(gettext \"No stash found.\")\"\n@@ -387,17 +386,18 @@ parse_flags_and_rev()\n \t\t\t:\n \t\t;;\n \t\t*)\n-\t\t\tdie \"$(eval_gettext \"Too many revisions specified: \\$REV\")\"\n+\t\t\trefs=\"$*\"\n+\t\t\tdie \"$(eval_gettext \"Too many revisions specified: \\$refs\")\"\n \t\t;;\n \tesac\n \n-\tREV=$(git rev-parse --quiet --symbolic --verify $1 2>/dev/null) || {\n+\tREV=$(git rev-parse --quiet --symbolic --verify \"$1\" 2>/dev/null) || {\n \t\treference=\"$1\"\n \t\tdie \"$(eval_gettext \"\\$reference is not valid reference\")\"\n \t}\n \n-\ti_commit=$(git rev-parse --quiet --verify $REV^2 2>/dev/null) &&\n-\tset -- $(git rev-parse $REV $REV^1 $REV: $REV^1: $REV^2: 2>/dev/null) &&\n+\ti_commit=$(git rev-parse --quiet --verify \"$REV^2\" 2>/dev/null) &&\n+\tset -- $(git rev-parse \"$REV\" \"$REV^1\" \"$REV:\" \"$REV^1:\" \"$REV^2:\" 2>/dev/null) &&\n \ts=$1 &&\n \tw_commit=$1 &&\n \tb_commit=$2 &&\n@@ -408,8 +408,8 @@ parse_flags_and_rev()\n \ttest \"$ref_stash\" = \"$(git rev-parse --symbolic-full-name \"${REV%@*}\")\" &&\n \tIS_STASH_REF=t\n \n-\tu_commit=$(git rev-parse --quiet --verify $REV^3 2>/dev/null) &&\n-\tu_tree=$(git rev-parse $REV^3: 2>/dev/null)\n+\tu_commit=$(git rev-parse --quiet --verify \"$REV^3\" 2>/dev/null) &&\n+\tu_tree=$(git rev-parse \"$REV^3:\" 2>/dev/null)\n }\n \n is_stash_like()\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex debda7a..0568ec5 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -673,4 +673,15 @@ test_expect_success 'store updates stash ref and reflog' '\n \tgrep quux bazzy\n '\n \n+test_expect_success 'handle stash specification with spaces' '\n+\tgit stash clear\n+\techo pig > file &&\n+\tgit stash &&\n+\ttest_tick &&\n+\techo cow > file &&\n+\tgit stash &&\n+\tgit stash apply \"stash@{Thu Apr 7 15:17:13 2005 -0700}\" &&\n+\tgrep pig file\n+'\n+\n test_done\n-- \n1.8.5.1.g359345f\n"},{"id":"231279","messageId":"87k3frdlwc.fsf@thomasrast.ch","threadId":"35438","inReplyTo":"1385752979-28162-1-git-send-email-oystwa@gmail.com","subject":"Re: [PATCH] stash: handle specifying stashes with spaces","fromName":"Thomas Rast","fromEmail":"tr@thomasrast.ch","sentAt":"2013-11-29T19:54:11Z","receivedAt":"2013-11-29T19:54:11Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Øystein Walle <oystwa@gmail.com> writes:\n\n> When trying to pop/apply a stash specified with an argument containing\n> spaces the user will see an error:\n>\n>     $ git stash pop 'stash@{two hours ago}'\n>     Too many revisions specified: stash@{two hours ago}\n>\n> This happens because word splitting is used to count non-option\n> arguments. Instead shift the positional arguments as the options are\n> processed; the number of arguments left is then the number we're after.\n[...]\n>  \tfor opt\n>  \tdo\n>  \t\tcase \"$opt\" in\n>  \t\t\t-q|--quiet)\n>  \t\t\t\tGIT_QUIET=-t\n> +\t\t\t\tshift\n>  \t\t\t;;\n>  \t\t\t--index)\n>  \t\t\t\tINDEX_OPTION=--index\n> +\t\t\t\tshift\n>  \t\t\t;;\n>  \t\t\t-*)\n>  \t\t\t\tFLAGS=\"${FLAGS}${FLAGS:+ }$opt\"\n> +\t\t\t\tshift\n>  \t\t\t;;\n>  \t\tesac\n>  \tdone\n>  \n> -\tset -- $REV\n> -\n\nBut this isn't correct any more, is it?  You unconditionally shift off\narguments when you see something of the form -*, even if what you shift\nis not what you're currently looking at.\n\nFor example, without this patch:\n\n  $ g stash apply stash@{0} --index\n  On branch next\n  Your branch is ahead of 'origin/next' by 41 commits.\n    (use \"git push\" to publish your local commits)\n  [blah blah]\n\nbut with this patch:\n\n  $ g stash apply stash@{0} --index\n  --index is not valid reference\n\n\nGranted, git-stash is extremely inconsistent in its handling of options.\nFor example, 'git stash save foo -k' does _not_ treat -k as an option.\nIf you set out to unify this (not just randomly (un)break one\nsubroutine) I'd be all for it.\n\n-- \nThomas Rast\ntr@thomasrast.ch\n"},{"id":"231280","messageId":"1385758845-8627-1-git-send-email-oystwa@gmail.com","threadId":"35438","inReplyTo":"87k3frdlwc.fsf@thomasrast.ch","subject":"[PATCH v2] stash: handle specifying stashes with spaces","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2013-11-29T21:00:45Z","receivedAt":"2013-11-29T21:00:45Z","isPatch":true,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"When trying to pop/apply a stash specified with an argument containing\nspaces git-stash will throw an error:\n\n    $ git stash pop 'stash@{two hours ago}'\n    Too many revisions specified: stash@{two hours ago}\n\nThis happens because word splitting is used to count non-option\narguments. Make use of rev-parse's --sq option to quote the arguments\nfor us to ensure a correct count. Add quotes where necessary.\n\nAlso add a test that verifies correct behaviour.\n\nHelped-by: Thomas Rast <tr@thomasrast.ch>\nSigned-off-by: Øystein Walle <oystwa@gmail.com>\n---\nMany thanks to Thomas Rast for helping me with this approach.\n\n git-stash.sh     | 14 +++++++-------\n t/t3903-stash.sh | 11 +++++++++++\n 2 files changed, 18 insertions(+), 7 deletions(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 1e541a2..f0a94ab 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -358,7 +358,7 @@ parse_flags_and_rev()\n \ti_tree=\n \tu_tree=\n \n-\tREV=$(git rev-parse --no-flags --symbolic \"$@\") || exit 1\n+\tREV=$(git rev-parse --no-flags --symbolic --sq \"$@\") || exit 1\n \n \tFLAGS=\n \tfor opt\n@@ -376,7 +376,7 @@ parse_flags_and_rev()\n \t\tesac\n \tdone\n \n-\tset -- $REV\n+\teval set -- $REV\n \n \tcase $# in\n \t\t0)\n@@ -391,13 +391,13 @@ parse_flags_and_rev()\n \t\t;;\n \tesac\n \n-\tREV=$(git rev-parse --quiet --symbolic --verify $1 2>/dev/null) || {\n+\tREV=$(git rev-parse --quiet --symbolic --verify \"$1\" 2>/dev/null) || {\n \t\treference=\"$1\"\n \t\tdie \"$(eval_gettext \"\\$reference is not valid reference\")\"\n \t}\n \n-\ti_commit=$(git rev-parse --quiet --verify $REV^2 2>/dev/null) &&\n-\tset -- $(git rev-parse $REV $REV^1 $REV: $REV^1: $REV^2: 2>/dev/null) &&\n+\ti_commit=$(git rev-parse --quiet --verify \"$REV^2\" 2>/dev/null) &&\n+\tset -- $(git rev-parse \"$REV\" \"$REV^1\" \"$REV:\" \"$REV^1:\" \"$REV^2:\" 2>/dev/null) &&\n \ts=$1 &&\n \tw_commit=$1 &&\n \tb_commit=$2 &&\n@@ -408,8 +408,8 @@ parse_flags_and_rev()\n \ttest \"$ref_stash\" = \"$(git rev-parse --symbolic-full-name \"${REV%@*}\")\" &&\n \tIS_STASH_REF=t\n \n-\tu_commit=$(git rev-parse --quiet --verify $REV^3 2>/dev/null) &&\n-\tu_tree=$(git rev-parse $REV^3: 2>/dev/null)\n+\tu_commit=$(git rev-parse --quiet --verify \"$REV^3\" 2>/dev/null) &&\n+\tu_tree=$(git rev-parse \"$REV^3:\" 2>/dev/null)\n }\n \n is_stash_like()\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex debda7a..0568ec5 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -673,4 +673,15 @@ test_expect_success 'store updates stash ref and reflog' '\n \tgrep quux bazzy\n '\n \n+test_expect_success 'handle stash specification with spaces' '\n+\tgit stash clear\n+\techo mook > file &&\n+\tgit stash &&\n+\ttest_tick &&\n+\techo kleb > file &&\n+\tgit stash &&\n+\tgit stash apply \"stash@{Thu Apr 7 15:17:13 2005 -0700}\" &&\n+\tgrep mook file\n+'\n+\n test_done\n-- \n1.8.5.rc0.23.gaa27064.dirty\n"},{"id":"231282","messageId":"878uw6dha7.fsf@thomasrast.ch","threadId":"35438","inReplyTo":"1385758845-8627-1-git-send-email-oystwa@gmail.com","subject":"Re: [PATCH v2] stash: handle specifying stashes with spaces","fromName":"Thomas Rast","fromEmail":"tr@thomasrast.ch","sentAt":"2013-11-29T21:33:52Z","receivedAt":"2013-11-29T21:33:52Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Thanks for looking into this!\n\nØystein Walle <oystwa@gmail.com> writes:\n\n> -\tREV=$(git rev-parse --quiet --symbolic --verify $1 2>/dev/null) || {\n> +\tREV=$(git rev-parse --quiet --symbolic --verify \"$1\" 2>/dev/null) || {\n>  \t\treference=\"$1\"\n\nIt's somewhat ironic that the one place where the original did use\nquoting is where it's actually not required.\n\n> -\ti_commit=$(git rev-parse --quiet --verify $REV^2 2>/dev/null) &&\n> -\tset -- $(git rev-parse $REV $REV^1 $REV: $REV^1: $REV^2: 2>/dev/null) &&\n> +\ti_commit=$(git rev-parse --quiet --verify \"$REV^2\" 2>/dev/null) &&\n> +\tset -- $(git rev-parse \"$REV\" \"$REV^1\" \"$REV:\" \"$REV^1:\" \"$REV^2:\" 2>/dev/null) &&\n\nThanks for being careful here.\n\nI wonder what we would lose by dropping the --symbolic in the line I\nquoted above (which is the second parsing pass), so that it resolves to\na SHA1.  We would gain some robustness, as I'm not sure \"$REV:\" works\ncorrectly in the face of weird revision expressions like \":/foo\".\n\n-- \nThomas Rast\ntr@thomasrast.ch\n"},{"id":"231309","messageId":"loom.20131130T153012-412@post.gmane.org","threadId":"35438","inReplyTo":"878uw6dha7.fsf@thomasrast.ch","subject":"Re: [PATCH v2] stash: handle specifying stashes with spaces","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2013-11-30T15:29:57Z","receivedAt":"2013-11-30T15:29:57Z","isPatch":true,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"Thomas Rast <tr <at> thomasrast.ch> writes:\n\n> I wonder what we would lose by dropping the --symbolic in the line I\n> quoted above (which is the second parsing pass), so that it resolves\n> to a SHA1.  We would gain some robustness, as I'm not sure \"$REV:\"\n> works correctly in the face of weird revision expressions like\n> \":/foo\".\n\nIf I drop --symbolic then all hell breaks loose. Removing it very\nnaively led to 29 failed tests. I managed to get that down quite a bit\nthough.\n\nAfter the function ends $REV is supposed to expand to a symbolic\nreference, and the test for whether the given argument is a valid stash\nreference is to see if running `git rev-parse --symbolic-full-name\n\"${REV%@*}\"` expands to 'refs/stash' or not. So we have to do both with\nand without --symbolic and keep both around. For example `git stash\ndrop` had problems because git-reflog doesn't let you remove entries in\nthe log by SHA1:\n\n\n    $ git reflog delete --updateref --rewrite $(git rev-parse stash@{0})\n    error: Not a reflog: 418af27beea220ad8a2fd3b8286959b1ec9c8852\n\nI think a not entirely accurate but succinct way of putting it is that\nif foo is a valid ref or valid entry in the reflog then\n\n    git rev-parse --symbolic $(git rev-parse foo) \n\ndoes *not* output 'foo' but the SHA1 of 'foo'. So we cannot simply\nconvert everything to hashes and proceed from there.\n\nØsse\n"},{"id":"231318","messageId":"CAPig+cTRhb9vwMSsw9n6JF-00rFnToZtSuGnWAwNMaUcvvg5KQ@mail.gmail.com","threadId":"35438","inReplyTo":"1385752979-28162-1-git-send-email-oystwa@gmail.com","subject":"Re: [PATCH] stash: handle specifying stashes with spaces","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-12-01T00:41:56Z","receivedAt":"2013-12-01T00:41:56Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Nov 29, 2013 at 2:22 PM, Øystein Walle <oystwa@gmail.com> wrote:\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index debda7a..0568ec5 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -673,4 +673,15 @@ test_expect_success 'store updates stash ref and reflog' '\n>         grep quux bazzy\n>  '\n>\n> +test_expect_success 'handle stash specification with spaces' '\n> +       git stash clear\n\nDo you want && at the end of this line?\n\n> +       echo pig > file &&\n> +       git stash &&\n> +       test_tick &&\n> +       echo cow > file &&\n> +       git stash &&\n> +       git stash apply \"stash@{Thu Apr 7 15:17:13 2005 -0700}\" &&\n> +       grep pig file\n> +'\n> +\n>  test_done\n> --\n> 1.8.5.1.g359345f\n"}]}