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

2013-11-30 Thread Ø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




--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH v2] stash: handle specifying stashes with spaces

2013-11-29 Thread Ø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 t...@thomasrast.ch
Signed-off-by: Øystein Walle oys...@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

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2013-11-29 Thread Thomas Rast
Thanks for looking into this!

Øystein Walle oys...@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
t...@thomasrast.ch
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html