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

From: Brandon Casey <drafnel@gmail.com>
Date: Apr 16, 2013, 18:57 UTC
Message-ID: <CA+sFfMfkoBcoPvJSYBGUe46EbMfiC7q6tVZs6db2NHrEb2gb7Q@mail.gmail.com>
In-Reply-To: <1366135765-18437-1-git-send-email-artagnon@gmail.com>

On Tue, Apr 16, 2013 at 11:09 AM, Ramkumar Ramachandra <artagnon@gmail.com> 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.

Show 12 quoted lines

> 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 <artagnon@gmail.com>
> ---
>  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?

Show 16 quoted lines

>  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" &&

            # Save another stash here

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

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

Show 7 quoted lines

> +       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