{"thread":{"id":"35622","subject":"[PATCH v3] stash: handle specifying stashes with spaces","startedAt":"2014-01-07T08:22:15Z","lastAt":"2014-01-09T18:42:18Z","messageCount":6,"participants":["Øystein Walle","Junio C Hamano","Thomas Rast"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"232801","messageId":"1389082935-16159-1-git-send-email-oystwa@gmail.com","threadId":"35622","inReplyTo":null,"subject":"[PATCH v3] stash: handle specifying stashes with spaces","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2014-01-07T08:22:15Z","receivedAt":"2014-01-07T08:22:15Z","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---\nv3 uses the same eval/--sq technique as v2, suggested by Thomas Rast.\nThis is basically a resend except that I added a missing '&&' in the\ntest that Eric Sunshine noticed when reading v1.\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..7eb011c 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\n"},{"id":"232826","messageId":"xmqq7gabeiqo.fsf@gitster.dls.corp.google.com","threadId":"35622","inReplyTo":"1389082935-16159-1-git-send-email-oystwa@gmail.com","subject":"Re: [PATCH v3] stash: handle specifying stashes with spaces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-07T18:42:39Z","receivedAt":"2014-01-07T18:42:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?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 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>\n> This happens because word splitting is used to count non-option\n> arguments. Make use of rev-parse's --sq option to quote the arguments\n> for us to ensure a correct count. Add quotes where necessary.\n>\n> Also add a test that verifies correct behaviour.\n>\n> Helped-by: Thomas Rast <tr@thomasrast.ch>\n> Signed-off-by: Øystein Walle <oystwa@gmail.com>\n> ---\n> v3 uses the same eval/--sq technique as v2, suggested by Thomas Rast.\n> This is basically a resend except that I added a missing '&&' in the\n> test that Eric Sunshine noticed when reading v1.\n\nThe change looks good.\n\nAn unrelated tangent, but here is a tip-of-the-day for the\napproximate parser.  You could have just said\n\n\tgit stash pop stash@{2.hours}\n\nand it would have been interpreted just the same ;-)\n\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index debda7a..7eb011c 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\nStyle: no SP between redirection operator and its target, i.e.\n\n\techo pig >file &&\n\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\nThis is brittle.  If new tests are added before this, the test_tick\nwill give you different timestamp and this test will start failing.\n\nPerhaps grab the timestamp out of the stash that was created, e.g.\n\n\t...\n        test_tick &&\n        git stash &&\n        stamp=$(git log -g --format=\"%cd\" -1 refs/stash) &&\n\ttest_tick &&\n        echo cow >file &&\n        git stash &&\n        git stash apply \"stash@{$stamp}\" &&\n\nor something?\n\n> +\tgrep pig file\n> +'\n> +\n>  test_done\n"},{"id":"232874","messageId":"87ob3nphc5.fsf@thomasrast.ch","threadId":"35622","inReplyTo":"xmqq7gabeiqo.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3] stash: handle specifying stashes with spaces","fromName":"Thomas Rast","fromEmail":"tr@thomasrast.ch","sentAt":"2014-01-07T22:17:30Z","receivedAt":"2014-01-07T22:17:30Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Øystein Walle <oystwa@gmail.com> writes:\n>\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>\n> This is brittle.  If new tests are added before this, the test_tick\n> will give you different timestamp and this test will start failing.\n>\n> Perhaps grab the timestamp out of the stash that was created [...]\n\nHmm, now that I stare at this, why not simply use something like\n\n  git stash apply \"stash@{ 0 }\"\n\nIt seems to refer to the same as stash@{0} as one would expect, while\nstill triggering the bug with unpatched git-stash.\n\n-- \nThomas Rast\ntr@thomasrast.ch\n"},{"id":"232876","messageId":"xmqqiotvbf4g.fsf@gitster.dls.corp.google.com","threadId":"35622","inReplyTo":"87ob3nphc5.fsf@thomasrast.ch","subject":"Re: [PATCH v3] stash: handle specifying stashes with spaces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-07T22:29:03Z","receivedAt":"2014-01-07T22:29:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <tr@thomasrast.ch> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Øystein Walle <oystwa@gmail.com> writes:\n>>\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>>\n>> This is brittle.  If new tests are added before this, the test_tick\n>> will give you different timestamp and this test will start failing.\n>>\n>> Perhaps grab the timestamp out of the stash that was created [...]\n>\n> Hmm, now that I stare at this, why not simply use something like\n>\n>   git stash apply \"stash@{ 0 }\"\n>\n> It seems to refer to the same as stash@{0} as one would expect, while\n> still triggering the bug with unpatched git-stash.\n\nYeah, that is fine as well.  For the record, here is what I\ntentatively queued.\n\n-- >8 --\nFrom: Øystein Walle <oystwa@gmail.com>\nDate: Tue, 7 Jan 2014 09:22:15 +0100\nSubject: [PATCH] stash: handle specifying stashes with $IFS\n\nWhen trying to pop/apply a stash specified with an argument\ncontaining IFS whitespace, 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>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n git-stash.sh     | 14 +++++++-------\n t/t3903-stash.sh | 12 ++++++++++++\n 2 files changed, 19 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..5b79b21 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -673,4 +673,16 @@ 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+\tstamp=$(git log -g --format=\"%cd\" -1 refs/stash) &&\n+\ttest_tick &&\n+\techo cow >file &&\n+\tgit stash &&\n+\tgit stash apply \"stash@{$stamp}\" &&\n+\tgrep pig file\n+'\n+\n test_done\n-- \n1.8.5.2-419-g5ca323a\n"},{"id":"232888","messageId":"loom.20140108T003945-67@post.gmane.org","threadId":"35622","inReplyTo":"xmqqiotvbf4g.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3] stash: handle specifying stashes with spaces","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2014-01-08T00:03:54Z","receivedAt":"2014-01-08T00:03:54Z","isPatch":true,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"Junio C Hamano <gitster <at> pobox.com> writes:\n\n> \n> Thomas Rast <tr <at> thomasrast.ch> writes:\n> \n> > Junio C Hamano <gitster <at> pobox.com> writes:\n> >\n> >>\n> >> This is brittle.  If new tests are added before this, the test_tick\n> >> will give you different timestamp and this test will start failing.\n> >>\n> >> Perhaps grab the timestamp out of the stash that was created [...]\n> >\n> > Hmm, now that I stare at this, why not simply use something like\n> >\n> >   git stash apply \"stash <at> { 0 }\"\n> >\n> > It seems to refer to the same as stash <at> {0} as one would expect, while\n> > still triggering the bug with unpatched git-stash.\n> \n> Yeah, that is fine as well.  For the record, here is what I\n> tentatively queued.\n> \n\nI completely agree that it's brittle. I mentioned it when I submitted v1\nbut at the time it didn't occur to me that new tests might be added\nbefore it. And of course I agree with your proposed changes to the test.\nI must say I like Thomas' solution because of its simplicity and the\nwhole test could be made much shorter: just create stash and try to pop\nit.\n\nBut it's seems the spaces trigger some other way of interpreting the\nselector. In my git.git, git rev-parse HEAD{0} gives me the same result\nas HEAD@{ 0 } but HEAD@{1} and HEAD@{ 1 } are different. Is this\nintentional? If not, can this impact the reliability of the test if we\nuse HEAD@{ 0 } ?\n\nThanks for queuing!\n\nRegards,\nØsse\n"},{"id":"232959","messageId":"xmqq1u0hatf9.fsf@gitster.dls.corp.google.com","threadId":"35622","inReplyTo":"loom.20140108T003945-67@post.gmane.org","subject":"Re: [PATCH v3] stash: handle specifying stashes with spaces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-09T18:42:18Z","receivedAt":"2014-01-09T18:42:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Øystein Walle <oystwa@gmail.com> writes:\n\n> But it's seems the spaces trigger some other way of interpreting the\n> selector. In my git.git, git rev-parse HEAD{0} gives me the same result\n> as HEAD@{ 0 } but HEAD@{1} and HEAD@{ 1 } are different.\n\nThe integer to specify the nth reflog entry (or nth prior checkout)\nare never spelled with any SP stuffing. HEAD@{1} is the prior state,\nHEAD@{-1} is the previous branch; HEAD@{ 1 } nor HEAD@{ -1 } do not\nmean these things.\n\nAny string inside @{...} that is _not_ a valid nth reflog entry\nspecifier is interpreted as a human-readable timestamp via the\napproxidate logic (and used only when it makes sense).  \" 1\" happens\nto mean \"the first day of the month\".\n"}]}