threads / patch / 35622

v3stash: handle specifying stashes with spaces

Subject: [PATCH v3] stash: handle specifying stashes with spaces

## tl;dr

6 messages between Jan 7, 2014 and Jan 9, 2014. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Øystein Walle· Jan 7, 2014, 08:22 UTC · lore

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 <tr@thomasrast.ch>
Signed-off-by: Øystein Walle <oystwa@gmail.com>
---
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(-)
Show changes to 2 files +18 −7

git-stash.sh, t/t3903-stash.sh

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· Jan 7, 2014, 18:42 UTC · re: Øystein Walle · lore

Re: [PATCH v3] stash: handle specifying stashes with spaces

Øystein Walle <oystwa@gmail.com> writes:
Show 18 quoted lines
> 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 <tr@thomasrast.ch>
> Signed-off-by: Øystein Walle <oystwa@gmail.com>
> ---
> 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 ;-)
Show 11 quoted lines
> 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 &&
Show 5 quoted lines
> +	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· Jan 7, 2014, 22:17 UTC · re: Junio C Hamano · lore

Re: [PATCH v3] stash: handle specifying stashes with spaces

Junio C Hamano <gitster@pobox.com> writes:
Show 12 quoted lines
> Øystein Walle <oystwa@gmail.com> 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· Jan 7, 2014, 22:29 UTC · re: Thomas Rast · lore

Re: [PATCH v3] stash: handle specifying stashes with spaces

Thomas Rast <tr@thomasrast.ch> writes:
Show 21 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Øystein Walle <oystwa@gmail.com> 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 <oystwa@gmail.com>
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 <tr@thomasrast.ch>
Signed-off-by: Øystein Walle <oystwa@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-stash.sh     | 14 +++++++-------
 t/t3903-stash.sh | 12 ++++++++++++
 2 files changed, 19 insertions(+), 7 deletions(-)
Show changes to 2 files +19 −7

git-stash.sh, t/t3903-stash.sh

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· Jan 8, 2014, 00:03 UTC · re: Junio C Hamano · lore

Re: [PATCH v3] stash: handle specifying stashes with spaces

Junio C Hamano <gitster <at> pobox.com> writes:
Show 21 quoted lines
> 
> Thomas Rast <tr <at> thomasrast.ch> writes:
> 
> > Junio C Hamano <gitster <at> 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 <at> { 0 }"
> >
> > It seems to refer to the same as stash <at> {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· Jan 9, 2014, 18:42 UTC · re: Øystein Walle · lore

Re: [PATCH v3] stash: handle specifying stashes with spaces

Øystein Walle <oystwa@gmail.com> 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".

← back to recent threads