# [PATCH v3] stash: handle specifying stashes with spaces 6 messages from 2014-01-07 to 2014-01-09. Participants: Øystein Walle, Junio C Hamano, Thomas Rast. Thread: https://gitlist.dev/t/35622 ## Øystein Walle, 2014-01-07 08:22 Subject: [PATCH v3] stash: handle specifying stashes with spaces Message-ID: <1389082935-16159-1-git-send-email-oystwa@gmail.com> URL: https://gitlist.dev/e/1389082935-16159-1-git-send-email-oystwa%40gmail.com ``` When trying to pop/apply a stash specified with an argument containing spaces git-stash will throw an error: $ git stash pop 'stash@{two hours ago}' Too many revisions specified: stash@{two hours ago} This happens because word splitting is used to count non-option arguments. Make use of rev-parse's --sq option to quote the arguments for us to ensure a correct count. Add quotes where necessary. Also add a test that verifies correct behaviour. Helped-by: Thomas Rast Signed-off-by: Øystein Walle --- v3 uses the same eval/--sq technique as v2, suggested by Thomas Rast. This is basically a resend except that I added a missing '&&' in the test that Eric Sunshine noticed when reading v1. git-stash.sh | 14 +++++++------- t/t3903-stash.sh | 11 +++++++++++ 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/git-stash.sh b/git-stash.sh index 1e541a2..f0a94ab 100755 --- a/git-stash.sh +++ b/git-stash.sh @@ -358,7 +358,7 @@ parse_flags_and_rev() i_tree= u_tree= - REV=$(git rev-parse --no-flags --symbolic "$@") || exit 1 + REV=$(git rev-parse --no-flags --symbolic --sq "$@") || exit 1 FLAGS= for opt @@ -376,7 +376,7 @@ parse_flags_and_rev() esac done - set -- $REV + eval set -- $REV case $# in 0) @@ -391,13 +391,13 @@ parse_flags_and_rev() ;; esac - REV=$(git rev-parse --quiet --symbolic --verify $1 2>/dev/null) || { + REV=$(git rev-parse --quiet --symbolic --verify "$1" 2>/dev/null) || { reference="$1" die "$(eval_gettext "\$reference is not valid reference")" } - i_commit=$(git rev-parse --quiet --verify $REV^2 2>/dev/null) && - set -- $(git rev-parse $REV $REV^1 $REV: $REV^1: $REV^2: 2>/dev/null) && + i_commit=$(git rev-parse --quiet --verify "$REV^2" 2>/dev/null) && + set -- $(git rev-parse "$REV" "$REV^1" "$REV:" "$REV^1:" "$REV^2:" 2>/dev/null) && s=$1 && w_commit=$1 && b_commit=$2 && @@ -408,8 +408,8 @@ parse_flags_and_rev() test "$ref_stash" = "$(git rev-parse --symbolic-full-name "${REV%@*}")" && IS_STASH_REF=t - u_commit=$(git rev-parse --quiet --verify $REV^3 2>/dev/null) && - u_tree=$(git rev-parse $REV^3: 2>/dev/null) + u_commit=$(git rev-parse --quiet --verify "$REV^3" 2>/dev/null) && + u_tree=$(git rev-parse "$REV^3:" 2>/dev/null) } is_stash_like() diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index debda7a..7eb011c 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -673,4 +673,15 @@ test_expect_success 'store updates stash ref and reflog' ' grep quux bazzy ' +test_expect_success 'handle stash specification with spaces' ' + git stash clear && + echo pig > file && + git stash && + test_tick && + echo cow > file && + git stash && + git stash apply "stash@{Thu Apr 7 15:17:13 2005 -0700}" && + grep pig file +' + test_done -- 1.8.5 ``` ## Junio C Hamano, 2014-01-07 18:42 Subject: Re: [PATCH v3] stash: handle specifying stashes with spaces Message-ID: URL: https://gitlist.dev/e/xmqq7gabeiqo.fsf%40gitster.dls.corp.google.com In-Reply-To: <1389082935-16159-1-git-send-email-oystwa@gmail.com> ``` Øystein Walle writes: > When trying to pop/apply a stash specified with an argument containing > spaces git-stash will throw an error: > > $ git stash pop 'stash@{two hours ago}' > Too many revisions specified: stash@{two hours ago} > > This happens because word splitting is used to count non-option > arguments. Make use of rev-parse's --sq option to quote the arguments > for us to ensure a correct count. Add quotes where necessary. > > Also add a test that verifies correct behaviour. > > Helped-by: Thomas Rast > Signed-off-by: Øystein Walle > --- > v3 uses the same eval/--sq technique as v2, suggested by Thomas Rast. > This is basically a resend except that I added a missing '&&' in the > test that Eric Sunshine noticed when reading v1. The change looks good. An unrelated tangent, but here is a tip-of-the-day for the approximate parser. You could have just said git stash pop stash@{2.hours} and it would have been interpreted just the same ;-) > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > index debda7a..7eb011c 100755 > --- a/t/t3903-stash.sh > +++ b/t/t3903-stash.sh > @@ -673,4 +673,15 @@ test_expect_success 'store updates stash ref and reflog' ' > grep quux bazzy > ' > > +test_expect_success 'handle stash specification with spaces' ' > + git stash clear && > + echo pig > file && Style: no SP between redirection operator and its target, i.e. echo pig >file && > + git stash && > + test_tick && > + echo cow > file && > + git stash && > + git stash apply "stash@{Thu Apr 7 15:17:13 2005 -0700}" && This is brittle. If new tests are added before this, the test_tick will give you different timestamp and this test will start failing. Perhaps grab the timestamp out of the stash that was created, e.g. ... test_tick && git stash && stamp=$(git log -g --format="%cd" -1 refs/stash) && test_tick && echo cow >file && git stash && git stash apply "stash@{$stamp}" && or something? > + grep pig file > +' > + > test_done ``` ## Thomas Rast, 2014-01-07 22:17 Subject: Re: [PATCH v3] stash: handle specifying stashes with spaces Message-ID: <87ob3nphc5.fsf@thomasrast.ch> URL: https://gitlist.dev/e/87ob3nphc5.fsf%40thomasrast.ch In-Reply-To: ``` Junio C Hamano writes: > Øystein Walle writes: > >> + git stash && >> + test_tick && >> + echo cow > file && >> + git stash && >> + git stash apply "stash@{Thu Apr 7 15:17:13 2005 -0700}" && > > This is brittle. If new tests are added before this, the test_tick > will give you different timestamp and this test will start failing. > > Perhaps grab the timestamp out of the stash that was created [...] Hmm, now that I stare at this, why not simply use something like git stash apply "stash@{ 0 }" It seems to refer to the same as stash@{0} as one would expect, while still triggering the bug with unpatched git-stash. -- Thomas Rast tr@thomasrast.ch ``` ## Junio C Hamano, 2014-01-07 22:29 Subject: Re: [PATCH v3] stash: handle specifying stashes with spaces Message-ID: URL: https://gitlist.dev/e/xmqqiotvbf4g.fsf%40gitster.dls.corp.google.com In-Reply-To: <87ob3nphc5.fsf@thomasrast.ch> ``` Thomas Rast writes: > Junio C Hamano writes: > >> Øystein Walle writes: >> >>> + git stash && >>> + test_tick && >>> + echo cow > file && >>> + git stash && >>> + git stash apply "stash@{Thu Apr 7 15:17:13 2005 -0700}" && >> >> This is brittle. If new tests are added before this, the test_tick >> will give you different timestamp and this test will start failing. >> >> Perhaps grab the timestamp out of the stash that was created [...] > > Hmm, now that I stare at this, why not simply use something like > > git stash apply "stash@{ 0 }" > > It seems to refer to the same as stash@{0} as one would expect, while > still triggering the bug with unpatched git-stash. Yeah, that is fine as well. For the record, here is what I tentatively queued. -- >8 -- From: Øystein Walle Date: Tue, 7 Jan 2014 09:22:15 +0100 Subject: [PATCH] stash: handle specifying stashes with $IFS When trying to pop/apply a stash specified with an argument containing IFS whitespace, git-stash will throw an error: $ git stash pop 'stash@{two hours ago}' Too many revisions specified: stash@{two hours ago} This happens because word splitting is used to count non-option arguments. Make use of rev-parse's --sq option to quote the arguments for us to ensure a correct count. Add quotes where necessary. Also add a test that verifies correct behaviour. Helped-by: Thomas Rast Signed-off-by: Øystein Walle Signed-off-by: Junio C Hamano --- git-stash.sh | 14 +++++++------- t/t3903-stash.sh | 12 ++++++++++++ 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/git-stash.sh b/git-stash.sh index 1e541a2..f0a94ab 100755 --- a/git-stash.sh +++ b/git-stash.sh @@ -358,7 +358,7 @@ parse_flags_and_rev() i_tree= u_tree= - REV=$(git rev-parse --no-flags --symbolic "$@") || exit 1 + REV=$(git rev-parse --no-flags --symbolic --sq "$@") || exit 1 FLAGS= for opt @@ -376,7 +376,7 @@ parse_flags_and_rev() esac done - set -- $REV + eval set -- $REV case $# in 0) @@ -391,13 +391,13 @@ parse_flags_and_rev() ;; esac - REV=$(git rev-parse --quiet --symbolic --verify $1 2>/dev/null) || { + REV=$(git rev-parse --quiet --symbolic --verify "$1" 2>/dev/null) || { reference="$1" die "$(eval_gettext "\$reference is not valid reference")" } - i_commit=$(git rev-parse --quiet --verify $REV^2 2>/dev/null) && - set -- $(git rev-parse $REV $REV^1 $REV: $REV^1: $REV^2: 2>/dev/null) && + i_commit=$(git rev-parse --quiet --verify "$REV^2" 2>/dev/null) && + set -- $(git rev-parse "$REV" "$REV^1" "$REV:" "$REV^1:" "$REV^2:" 2>/dev/null) && s=$1 && w_commit=$1 && b_commit=$2 && @@ -408,8 +408,8 @@ parse_flags_and_rev() test "$ref_stash" = "$(git rev-parse --symbolic-full-name "${REV%@*}")" && IS_STASH_REF=t - u_commit=$(git rev-parse --quiet --verify $REV^3 2>/dev/null) && - u_tree=$(git rev-parse $REV^3: 2>/dev/null) + u_commit=$(git rev-parse --quiet --verify "$REV^3" 2>/dev/null) && + u_tree=$(git rev-parse "$REV^3:" 2>/dev/null) } is_stash_like() diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index debda7a..5b79b21 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -673,4 +673,16 @@ test_expect_success 'store updates stash ref and reflog' ' grep quux bazzy ' +test_expect_success 'handle stash specification with spaces' ' + git stash clear && + echo pig >file && + git stash && + stamp=$(git log -g --format="%cd" -1 refs/stash) && + test_tick && + echo cow >file && + git stash && + git stash apply "stash@{$stamp}" && + grep pig file +' + test_done -- 1.8.5.2-419-g5ca323a ``` ## Øystein Walle, 2014-01-08 00:03 Subject: Re: [PATCH v3] stash: handle specifying stashes with spaces Message-ID: URL: https://gitlist.dev/e/loom.20140108T003945-67%40post.gmane.org In-Reply-To: ``` Junio C Hamano pobox.com> writes: > > Thomas Rast thomasrast.ch> writes: > > > Junio C Hamano pobox.com> writes: > > > >> > >> This is brittle. If new tests are added before this, the test_tick > >> will give you different timestamp and this test will start failing. > >> > >> Perhaps grab the timestamp out of the stash that was created [...] > > > > Hmm, now that I stare at this, why not simply use something like > > > > git stash apply "stash { 0 }" > > > > It seems to refer to the same as stash {0} as one would expect, while > > still triggering the bug with unpatched git-stash. > > Yeah, that is fine as well. For the record, here is what I > tentatively queued. > I completely agree that it's brittle. I mentioned it when I submitted v1 but at the time it didn't occur to me that new tests might be added before it. And of course I agree with your proposed changes to the test. I must say I like Thomas' solution because of its simplicity and the whole test could be made much shorter: just create stash and try to pop it. But it's seems the spaces trigger some other way of interpreting the selector. In my git.git, git rev-parse HEAD{0} gives me the same result as HEAD@{ 0 } but HEAD@{1} and HEAD@{ 1 } are different. Is this intentional? If not, can this impact the reliability of the test if we use HEAD@{ 0 } ? Thanks for queuing! Regards, Øsse ``` ## Junio C Hamano, 2014-01-09 18:42 Subject: Re: [PATCH v3] stash: handle specifying stashes with spaces Message-ID: URL: https://gitlist.dev/e/xmqq1u0hatf9.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Øystein Walle writes: > But it's seems the spaces trigger some other way of interpreting the > selector. In my git.git, git rev-parse HEAD{0} gives me the same result > as HEAD@{ 0 } but HEAD@{1} and HEAD@{ 1 } are different. The integer to specify the nth reflog entry (or nth prior checkout) are never spelled with any SP stuffing. HEAD@{1} is the prior state, HEAD@{-1} is the previous branch; HEAD@{ 1 } nor HEAD@{ -1 } do not mean these things. Any string inside @{...} that is _not_ a valid nth reflog entry specifier is interpreted as a human-readable timestamp via the approxidate logic (and used only when it makes sense). " 1" happens to mean "the first day of the month". ```