Re: [PATCH v3] stash: handle specifying stashes with spaces
Øystein Walle oys...@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.
--
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 v3] stash: handle specifying stashes with spaces
Øystein Walle oys...@gmail.com writes:
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
---
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 ;-)
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
+ 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
--
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 v3] stash: handle specifying stashes with spaces
Junio C Hamano gits...@pobox.com writes:
Øystein Walle oys...@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
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
Re: [PATCH v3] stash: handle specifying stashes with spaces
Thomas Rast t...@thomasrast.ch writes:
Junio C Hamano gits...@pobox.com writes:
Øystein Walle oys...@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 oys...@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 t...@thomasrast.ch
Signed-off-by: Øystein Walle oys...@gmail.com
Signed-off-by: Junio C Hamano gits...@pobox.com
---
git-stash.sh | 14 +++---
t/t3903-stash.sh | 12
2 files changed, 19 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..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
--
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 v3] stash: handle specifying stashes with spaces
Junio C Hamano gitster at pobox.com writes:
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
--
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
