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

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

[Ramkumar Ramachandra]

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

 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

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> 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 <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.
>
>  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?

I think they were an attempt to catch command line argument errors
early to be helpful to the end users.

See ef763129d105 (detached-stash: introduce parse_flags_and_revs
function, 2010-08-21).  As the advertised and originally intended
use for stash was to name them with "stash@{number}", chomping at
the first at-sign to make sure it names refs/stash does not sound
too bad a check.

I do not think anybody considered the approach to look at the commit
object name and making sure it appears in the reflog that implements
the stash. It sounds like a more robust check if done right.

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

[Brandon Casey]

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.

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

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

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

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

[Junio C Hamano]

Brandon Casey <drafnel@gmail.com> writes:

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

Good point, together with...

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

"log -g --grep=<text>" gives you a way to eyeball, but with
@{/<text>} you _might_ have a good way to name the revision.

I am not however so sure if it is useful outside the context of the
stash, because the ones you would want to recover from a normal
reflog is most likely the older version of what you already amended,
so the latest hit will likely be the post-amend version, not the one
closer to the original.  You would end up eyeballing the output of
"log --oneline -g -grep=<text>" and cutting from it.

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

Oh, so it wasn't just me.  I was about to say something similar,
along the lines of "people whom you just called myopic, because you
did not understand the rationale behind their past work, are less
likely to be inclined to help you. you would have more luck if you
ask them nicely.", but I've long given up on helping him be a better
community member and deleted that part.

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

[Brandon Casey]

On Tue, Apr 16, 2013 at 12:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Brandon Casey <drafnel@gmail.com> writes:
>
>> 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.
>
> Good point, together with...
>
>> An extension to the reflog dwimery that implements @{/<text>} could be
>> interesting though.
>
> "log -g --grep=<text>" gives you a way to eyeball, but with
> @{/<text>} you _might_ have a good way to name the revision.
>
> I am not however so sure if it is useful outside the context of the
> stash, because the ones you would want to recover from a normal
> reflog is most likely the older version of what you already amended,
> so the latest hit will likely be the post-amend version, not the one
> closer to the original.  You would end up eyeballing the output of
> "log --oneline -g -grep=<text>" and cutting from it.

Yeah, I think that's true.  I can't think of a reason, at the moment,
where it would be useful outside of with 'git stash'.  I mainly wanted
to spell out "@{/<text>}" so that the mental link could be made back
to the code in git-stash that removes the "@*" suffix.

-Brandon

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

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Brandon Casey writes:
>> 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.
>
> Oh, so it wasn't just me.  I was about to say something similar,
> along the lines of "people whom you just called myopic, because you
> did not understand the rationale behind their past work, are less
> likely to be inclined to help you. you would have more luck if you
> ask them nicely.", but I've long given up on helping him be a better
> community member and deleted that part.

I'm truly sorry.

I've only had a cursory look at git-stash.sh, and was merely saying
what came to my mind first after a cursory glance: it wasn't an
opinion of any sort.  A lot of things I say are stupid in retrospect:
I'm not ashamed to admit it; I'm an inexperienced kid, and I make lots
of mistakes.  And please don't interpret my comments as attacking the
people who wrote the code: in a community like ours, I don't believe
in associating blame to any one person; I believe that all of us are
equally responsible for all parts of the code as a collective; if
something doesn't match what I expect, why didn't I participate in the
discussion of the patch that led up to it?

I complain very loudly about little things that annoy me, and I think
this is a good attribute.  People who are generally happy with the
current state of affairs cannot make a big difference.  This does not
mean that I go on a stubborn rampage breaking backward compatibility
everywhere, but rather that I raise the kind of questions that other
people normally don't.

I do not blame people for who they are: they are just a product of
their histories; a sum of absorbed influences.  It is, therefore,
irrational to be rude to someone.  If someone is not behaving as I
expect them to, I send them a polite off-list email pointing out what
I think their negative attributes  are, and attempt to nudge them in
the desired direction.

I'll try to be a better community member in the future.

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

[Ramkumar Ramachandra]

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?

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

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> I do not think anybody considered the approach to look at the commit
> object name and making sure it appears in the reflog that implements
> the stash. It sounds like a more robust check if done right.

Actually, if you think about it, there is really only one way to
specify revisions in the stash's reflog: stash@*.  Since these
commits don't have to be reachable from any refs in the general case,
all the other revision syntaxes are useless.  So, I would argue that
"${REV%@*}" is sufficient and correct*: anything beyond it is an
unnecessary overhead.

However, the initial bug is still valid:  show should not show
something that pop cannot operate on.

* although I'd have been more comfortable if we had a neater way to specify that

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> I do not think anybody considered the approach to look at the commit
>> object name and making sure it appears in the reflog that implements
>> the stash. It sounds like a more robust check if done right.
>
> Actually, if you think about it, there is really only one way to
> specify revisions in the stash's reflog: stash@*.
> ...
> * although I'd have been more comfortable if we had a neater way to specify that

Yup.  "git stash pop +4", without a must-be-it token "stash" or
mandatory "@{}" frill would have been much nicer.

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

[Brandon Casey]

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

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

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

[Ramkumar Ramachandra]

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.

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> 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').

Correct.

To show or apply, you only need a product of "stash create" that may
not be linked anywhere in the refs or reflogs.  But you can only pop
or drop something on the stash reflog.

> 2. IS_STASH_LIKE is a misnomer: all it checks is that the given <rev>
> is a merge commit.

The purpose of the logic is to reject a mistake to name stuff that
is clearly not a product of "stash create".  The name is just being
humble by not claiming "I know this is definitely a product of
stash-create" but merely hinting "This smells like such an object";
I am not sure if that is a "misnomer".

You are free to try to think of a way to tighten the implemention to
reject a random two-or-three parent merge commit that is not a
product of "stash create".  People already have looked at this code
since it was written, and didn't find a reasonable way to tighten it
without triggering false negatives, so I wouldn't be surprised if
anybody tried it again today and failed.