[Resend PATCH] t3903 (stash): add failing test for ref of form ^{/message}

While a 'git stash show stash^{/quuxery}' works just fine, a 'git
stash pop stash^{/quuxery}' complains with: 'stash^{/quuxery} is not a
stash reference'.  This confusing behavior arises from the differences
in logic that 'show' and 'pop' internally employ to validate the
specified ref.  Document this bug by adding a failing testcase for it.

Signed-off-by: Ramkumar Ramachandra <redacted>
---
 So sorry about misspelling Junio's address in my previous email.
 Please respond to this one instead.

 So if you look at git-stash.sh:377, you'll notice that it's doing a
 the shell substitution "${REV%@*}" to figure out whether the stash
 ref is a valid ref.  This hacky myopic design has to be done away
 with immediately, and we should really compare the SHA-1 hex of the
 specified ref with those in the stash reflog.

 The only reason I haven't written a fix yet is because I'm not sure
 why you need this convoluted IS_STASH_LIKE and IS_STASH_REF logic in
 the first place.  Can someone enlighten me as to what is going on?

 t/t3903-stash.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 5dfbda7..04ba983 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh

@@ -583,6 +583,15 @@ test_expect_success 'invalid ref of the form stash@{n}, n >= N' '
 	git stash drop
 '
 
+test_expect_failure 'valid ref of the form stash^{/message}' '
+	git stash clear &&
+	echo bar > file &&
+	git add file &&
+	git stash save "quuxery" &&
+	git stash show stash^{/quuxery} &&
+	git stash pop stash^{/quuxery}
+'
+
 test_expect_success 'stash branch should not drop the stash if the branch exists' '
 	git stash clear &&
 	echo foo >file &&

-- 
1.8.2.1.390.g924f6c3.dirty

On Tue, Apr 16, 2013 at 11:09 AM, Ramkumar Ramachandra
[off-list ref] wrote:

While a 'git stash show stash^{/quuxery}' works just fine, a 'git
stash pop stash^{/quuxery}' complains with: 'stash^{/quuxery} is not a
stash reference'.

I don't think it is appropriate to use the ^{/<text>} notation with stashes.

The stash is implemented using the reflog.  The ^{/<text>} notation
searches the commit history, not the reflog.  So I think it will be
able to match the first entry in your stash stack, but not any of the
other ones.

Try inserting another stash (see below) on top of the one that
contains the string "quuxery" and I think you'll find that your 'git
stash show stash^{/quuxery}' no longer works.

An extension to the reflog dwimery that implements @{/<text>} could be
interesting though.

This confusing behavior arises from the differences
in logic that 'show' and 'pop' internally employ to validate the
specified ref.  Document this bug by adding a failing testcase for it.

Signed-off-by: Ramkumar Ramachandra <redacted>
---
 So sorry about misspelling Junio's address in my previous email.
 Please respond to this one instead.

 So if you look at git-stash.sh:377, you'll notice that it's doing a
 the shell substitution "${REV%@*}" to figure out whether the stash
 ref is a valid ref.

This hacky myopic design has to be done away
 with immediately, and we should really compare the SHA-1 hex of the
 specified ref with those in the stash reflog.

Just a bit of advice, maybe you should think about softening your tone
a bit hmm?  I find this last sentence to be somewhat repelling and
tend to refrain from responding to such.

 The only reason I haven't written a fix yet is because I'm not sure
 why you need this convoluted IS_STASH_LIKE and IS_STASH_REF logic in
 the first place.  Can someone enlighten me as to what is going on?

            # Save another stash here

            echo bash >file
            git add file
            git stash save "something"

            # Now git stash show stash^{/quuxery} no longer works.

+       git stash show stash^{/quuxery} &&
+       git stash pop stash^{/quuxery}
+'
+
 test_expect_success 'stash branch should not drop the stash if the branch exists' '
        git stash clear &&
        echo foo >file &&

-Brandon

Brandon Casey wrote:

            # Save another stash here

            echo bash >file
            git add file
            git stash save "something"

            # Now git stash show stash^{/quuxery} no longer works.

Ah, yes.  My stupidity.  Why was I expecting ^{/quuxery} to dig
through the reflog?

An extension to the reflog dwimery that implements @{/<text>} could be
interesting though.

Yeah, this sounds interesting.

My initial itch that led up to this: I wanted a way to stash something
away and recover it at a later time predictably for rebase.autostash
(there might have been other stash invocations in between).
Originally, I thought I'd need a refs/stashes/* or something of the
sort to solve this problem, but git-stash.sh hard-codes refs/stash
everywhere (and so do other things like reflog).  So, I was thinking
about retrieving it based on commit message, but the solution is still
short of ideal.  What are your thoughts on my original refs/stashes/*
idea?

On Tue, Apr 16, 2013 at 1:11 PM, Ramkumar Ramachandra
[off-list ref] wrote:

Brandon Casey wrote:

quoted

            # Save another stash here

            echo bash >file
            git add file
            git stash save "something"

            # Now git stash show stash^{/quuxery} no longer works.

Ah, yes.  My stupidity.  Why was I expecting ^{/quuxery} to dig
through the reflog?

quoted

An extension to the reflog dwimery that implements @{/<text>} could be
interesting though.

Yeah, this sounds interesting.

My initial itch that led up to this: I wanted a way to stash something
away and recover it at a later time predictably for rebase.autostash
(there might have been other stash invocations in between).
Originally, I thought I'd need a refs/stashes/* or something of the
sort to solve this problem, but git-stash.sh hard-codes refs/stash
everywhere (and so do other things like reflog).  So, I was thinking
about retrieving it based on commit message, but the solution is still
short of ideal.  What are your thoughts on my original refs/stashes/*
idea?

You can create a stash without modifying the refs/stash reflog using
'sha1=`git stash create`' and then later apply it using 'git stash
apply --index $sha1'.  You'll have to reset the work directory
yourself though since 'git stash create' does not do so.  The stash
created this way is just a dangling commit so it will have a lifetime
according to the gc.pruneexpire (default 2 weeks currently).

-Brandon

So, I read through git-stash.sh a little more, and found the following:

1. Any stash that can be shown can be applied, but not necessarily
popped or dropped (as the documentation indicates).  The reason for
this is simple: a pop/drop attempts to clear the entry in the stash
reflog as well, but all stashes need to have a corresponding reflog
entry (for instance, those created with 'stash create').

2. IS_STASH_LIKE is a misnomer: all it checks is that the given <rev>
is a merge commit.  As a result, you can 'stash show' and 'stash
apply' any merge commit.  Should we attempt to tighten this somehow,
or are we okay with the stash being just another merge commit?  Check
for a special commit message perhaps?

Brandon Casey wrote:

You can create a stash without modifying the refs/stash reflog using
'sha1=`git stash create`' and then later apply it using 'git stash
apply --index $sha1'.  You'll have to reset the work directory
yourself though since 'git stash create' does not do so.  The stash
created this way is just a dangling commit so it will have a lifetime
according to the gc.pruneexpire (default 2 weeks currently).

Thanks, but I was worried more about reachability of the commit: if I
create a ref to it in refs/stashes/* like I suggested, it wouldn't
expire until that ref was gone.  Then again, I suppose a ref is
unnecessary for a temporary stash.  Yeah, I can store the SHA-1 hex of
the dangling commit in my caller's $state_dir, and apply it from there
later.