Re: [PATCH v2] stash: handle specifying stashes with spaces
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
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
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