[PATCH] stash: handle specifying stashes with spaces

[PATCH] stash: handle specifying stashes with spaces

[Øystein Walle]

When trying to pop/apply a stash specified with an argument containing
spaces the user will see 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. Instead shift the positional arguments as the options are
processed; the number of arguments left is then the number we're after.
Add quotes where necessary.

Also add a test that verifies correct behaviour.

Signed-off-by: Øystein Walle <oystwa@gmail.com>
---
This is perhaps an esoteric use case but it's still worth fixing in my opinion.
It also saves a fork/exec so I see it as a win-win :)

Comments welcome, of course. Especially on the test. I couldn't use a relative
date spec since the commits and stashes are made with bogus timestamps and the
spec is compared to the local time. It looks a bit icky the way it's written
now.

 git-stash.sh     | 20 ++++++++++----------
 t/t3903-stash.sh | 11 +++++++++++
 2 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index 1e541a2..0a48d42 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -358,26 +358,25 @@ parse_flags_and_rev()
 	i_tree=
 	u_tree=
 
-	REV=$(git rev-parse --no-flags --symbolic "$@") || exit 1
-
 	FLAGS=
 	for opt
 	do
 		case "$opt" in
 			-q|--quiet)
 				GIT_QUIET=-t
+				shift
 			;;
 			--index)
 				INDEX_OPTION=--index
+				shift
 			;;
 			-*)
 				FLAGS="${FLAGS}${FLAGS:+ }$opt"
+				shift
 			;;
 		esac
 	done
 
-	set -- $REV
-
 	case $# in
 		0)
 			have_stash || die "$(gettext "No stash found.")"
@@ -387,17 +386,18 @@ parse_flags_and_rev()
 			:
 		;;
 		*)
-			die "$(eval_gettext "Too many revisions specified: \$REV")"
+			refs="$*"
+			die "$(eval_gettext "Too many revisions specified: \$refs")"
 		;;
 	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..0568ec5 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.1.g359345f

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

[Thomas Rast]

Øystein Walle <oystwa@gmail.com> writes:

> When trying to pop/apply a stash specified with an argument containing
> spaces the user will see 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. Instead shift the positional arguments as the options are
> processed; the number of arguments left is then the number we're after.
[...]
>  	for opt
>  	do
>  		case "$opt" in
>  			-q|--quiet)
>  				GIT_QUIET=-t
> +				shift
>  			;;
>  			--index)
>  				INDEX_OPTION=--index
> +				shift
>  			;;
>  			-*)
>  				FLAGS="${FLAGS}${FLAGS:+ }$opt"
> +				shift
>  			;;
>  		esac
>  	done
>  
> -	set -- $REV
> -

But this isn't correct any more, is it?  You unconditionally shift off
arguments when you see something of the form -*, even if what you shift
is not what you're currently looking at.

For example, without this patch:

  $ g stash apply stash@{0} --index
  On branch next
  Your branch is ahead of 'origin/next' by 41 commits.
    (use "git push" to publish your local commits)
  [blah blah]

but with this patch:

  $ g stash apply stash@{0} --index
  --index is not valid reference


Granted, git-stash is extremely inconsistent in its handling of options.
For example, 'git stash save foo -k' does _not_ treat -k as an option.
If you set out to unify this (not just randomly (un)break one
subroutine) I'd be all for it.

-- 
Thomas Rast
tr@thomasrast.ch

[PATCH v2] stash: handle specifying stashes with spaces

[Øystein Walle]

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>
---
Many thanks to Thomas Rast for helping me with this approach.

 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..0568ec5 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 mook > file &&
+	git stash &&
+	test_tick &&
+	echo kleb > file &&
+	git stash &&
+	git stash apply "stash@{Thu Apr 7 15:17:13 2005 -0700}" &&
+	grep mook file
+'
+
 test_done
-- 
1.8.5.rc0.23.gaa27064.dirty

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

[Thomas Rast]

Thanks for looking into this!

Øystein Walle <oystwa@gmail.com> writes:

> -	REV=$(git rev-parse --quiet --symbolic --verify $1 2>/dev/null) || {
> +	REV=$(git rev-parse --quiet --symbolic --verify "$1" 2>/dev/null) || {
>  		reference="$1"

It's somewhat ironic that the one place where the original did use
quoting is where it's actually not required.

> -	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) &&

Thanks for being careful here.

I wonder what we would lose by dropping the --symbolic in the line I
quoted above (which is the second parsing pass), so that it resolves to
a SHA1.  We would gain some robustness, as I'm not sure "$REV:" works
correctly in the face of weird revision expressions like ":/foo".

-- 
Thomas Rast
tr@thomasrast.ch

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

[Øystein Walle]

Thomas Rast <tr <at> thomasrast.ch> writes:

> I wonder what we would lose by dropping the --symbolic in the line I
> quoted above (which is the second parsing pass), so that it resolves
> to a SHA1.  We would gain some robustness, as I'm not sure "$REV:"
> works correctly in the face of weird revision expressions like
> ":/foo".

If I drop --symbolic then all hell breaks loose. Removing it very
naively led to 29 failed tests. I managed to get that down quite a bit
though.

After the function ends $REV is supposed to expand to a symbolic
reference, and the test for whether the given argument is a valid stash
reference is to see if running `git rev-parse --symbolic-full-name
"${REV%@*}"` expands to 'refs/stash' or not. So we have to do both with
and without --symbolic and keep both around. For example `git stash
drop` had problems because git-reflog doesn't let you remove entries in
the log by SHA1:


    $ git reflog delete --updateref --rewrite $(git rev-parse stash@{0})
    error: Not a reflog: 418af27beea220ad8a2fd3b8286959b1ec9c8852

I think a not entirely accurate but succinct way of putting it is that
if foo is a valid ref or valid entry in the reflog then

    git rev-parse --symbolic $(git rev-parse foo) 

does *not* output 'foo' but the SHA1 of 'foo'. So we cannot simply
convert everything to hashes and proceed from there.

Øsse

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

[Eric Sunshine]

On Fri, Nov 29, 2013 at 2:22 PM, Øystein Walle <oystwa@gmail.com> wrote:
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index debda7a..0568ec5 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

Do you want && at the end of this line?

> +       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.1.g359345f