threads / patch / 35438

patchstash: handle specifying stashes with spaces

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

## tl;dr

6 messages between Nov 29, 2013 and Dec 1, 2013. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Øystein Walle· Nov 29, 2013, 19:22 UTC · lore

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(-)
Show changes to 2 files +21 −10

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

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
Thomas Rast· Nov 29, 2013, 19:54 UTC · re: Øystein Walle · lore

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

Øystein Walle <oystwa@gmail.com> writes:
Show 9 quoted lines
> 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.
[...]
Show 20 quoted lines
>  	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
Øystein Walle· Nov 29, 2013, 21:00 UTC · re: Thomas Rast · lore

[PATCH v2] stash: handle specifying stashes with spaces

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(-)
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..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
Thomas Rast· Nov 29, 2013, 21:33 UTC · re: Øystein Walle · lore

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

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
Øystein Walle· Nov 30, 2013, 15:29 UTC · re: Thomas Rast · lore

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

Thomas Rast <tr <at> thomasrast.ch> writes:
Show 5 quoted lines
> 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
Eric Sunshine· Dec 1, 2013, 00:41 UTC · re: Øystein Walle · lore

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

On Fri, Nov 29, 2013 at 2:22 PM, Øystein Walle <oystwa@gmail.com> wrote:
Show 10 quoted lines
> 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?
Show 12 quoted lines
> +       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

← back to recent threads