Re: [Resend PATCH] t3903 (stash): add failing test for ref of form ^{/message}
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. -- 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: [Resend PATCH] t3903 (stash): add failing test for ref of form ^{/message}
Ramkumar Ramachandra artag...@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. -- 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
[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 artag...@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
--
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: [Resend PATCH] t3903 (stash): add failing test for ref of form ^{/message}
Ramkumar Ramachandra artag...@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 artag...@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.
--
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: [Resend PATCH] t3903 (stash): add failing test for ref of form ^{/message}
On Tue, Apr 16, 2013 at 11:09 AM, Ramkumar Ramachandra
artag...@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 artag...@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
--
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: [Resend PATCH] t3903 (stash): add failing test for ref of form ^{/message}
Brandon Casey draf...@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.
--
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: [Resend PATCH] t3903 (stash): add failing test for ref of form ^{/message}
On Tue, Apr 16, 2013 at 12:11 PM, Junio C Hamano gits...@pobox.com wrote:
Brandon Casey draf...@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
--
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: [Resend PATCH] t3903 (stash): add failing test for ref of form ^{/message}
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. -- 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: [Resend PATCH] t3903 (stash): add failing test for ref of form ^{/message}
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?
--
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: [Resend PATCH] t3903 (stash): add failing test for ref of form ^{/message}
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
--
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: [Resend PATCH] t3903 (stash): add failing test for ref of form ^{/message}
Ramkumar Ramachandra artag...@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.
--
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: [Resend PATCH] t3903 (stash): add failing test for ref of form ^{/message}
On Tue, Apr 16, 2013 at 1:11 PM, Ramkumar Ramachandra
artag...@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
--
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
