Re: [PATCH v2 10/11] sha1_name: reorganize get_sha1_basic()
On Wed, May 8, 2013 at 4:51 PM, Junio C Hamano wrote:
> Felipe Contreras writes:
>
>> On Wed, May 8, 2013 at 1:18 PM, Junio C Hamano wrote:
>>> Felipe Contreras writes:
>>>
Through the years the functionality to handle @{-N} and @{u} has moved
around the code, and as a result, code that once made sense, doesn't any
more.
There is no need to call this function recursively with the branch of
@{-N} substituted because dwim_{ref,log} already replaces it.
However, there's one corner-case where @{-N} resolves to a detached
HEAD, in which case we wouldn't get any ref back.
So we parse the nth-prior manually, and deal with it depending on
weather it's a SHA-1, or a ref.
...
>>>
>>> s/weather/whether/;
>>>
@@ -447,6 +448,10 @@ static int get_sha1_basic(const char *str, int len,
unsigned char *sha1)
if (len && str[len-1] == '}') {
for (at = len-4; at >= 0; at--) {
if (str[at] == '@' && str[at+1] == '{') {
+ if (at == 0 && str[2] == '-') {
+ nth_prior = 1;
+ continue;
+ }
>>>
>>> Does this have to be inside the loop?
>>
>> Yes, the whole purpose is to avoid reflog_len to be set.
>
> What I meant was the "@{-" check, which happens only at==0.
>
> if (!memcmp(str, "@{-", 3) && len > 3)
> nth_prior = 1;
> else
> for (at = len - 4; at; at--) {
> ... look for and break at the first "@{" ...
> }
>
> or something.
That's doable, but would screw up the next patch.
>>> Ahh, OK, the new code will now let dwim_ref/log to process @{-N}
>>> again (the log message hints this but it wasn't all that clear),
>>
>> I thought it was clear we would let dwim_{ref,log} do the job:
>
> Yes, the reason I did not immediately think of that is because I
> knew @{-N} was expensive (need to read reflog backwards) and didn't
> think anybody would redo the code to deliberately do that twice ;-)
But that's what the commit message said.
>>> Also, a few points this patch highlights in the code before the
>>> change:
>>>
>>> - If we were on a branch with 40-hex name at nth prior checkout,
>>>would we mistake it as being detached at the commit?
>>>
>>> - If we were on a branch 'foo' at nth prior checkout, would our
>>>previous get_sha1_1() have made us mistake it as referring to a
>>>tag 'foo' with the same name if it exists?
>>
>> I don't know, but I suspect there's no change after this patch.
>
> Yes, didn't I say "the code before the change" above?
>
> These two correctness issues look more important issues to me, with
> or without the restructure patch (in other words, they are
> independent).
Right, if you are interested in correctness, you might want to try
@{-1}{0}; it resolves to @{-1} currently, and it fails correctly with
my patch.
Cheers.
--
Felipe Contreras
--
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 v2 10/11] sha1_name: reorganize get_sha1_basic()
Felipe Contreras writes:
> On Wed, May 8, 2013 at 1:18 PM, Junio C Hamano wrote:
>> Felipe Contreras writes:
>>
>>> Through the years the functionality to handle @{-N} and @{u} has moved
>>> around the code, and as a result, code that once made sense, doesn't any
>>> more.
>>>
>>> There is no need to call this function recursively with the branch of
>>> @{-N} substituted because dwim_{ref,log} already replaces it.
>>>
>>> However, there's one corner-case where @{-N} resolves to a detached
>>> HEAD, in which case we wouldn't get any ref back.
>>>
>>> So we parse the nth-prior manually, and deal with it depending on
>>> weather it's a SHA-1, or a ref.
>>> ...
>>
>> s/weather/whether/;
>>
>>> @@ -447,6 +448,10 @@ static int get_sha1_basic(const char *str, int len,
>>> unsigned char *sha1)
>>> if (len && str[len-1] == '}') {
>>> for (at = len-4; at >= 0; at--) {
>>> if (str[at] == '@' && str[at+1] == '{') {
>>> + if (at == 0 && str[2] == '-') {
>>> + nth_prior = 1;
>>> + continue;
>>> + }
>>
>> Does this have to be inside the loop?
>
> Yes, the whole purpose is to avoid reflog_len to be set.
What I meant was the "@{-" check, which happens only at==0.
if (!memcmp(str, "@{-", 3) && len > 3)
nth_prior = 1;
else
for (at = len - 4; at; at--) {
... look for and break at the first "@{" ...
}
or something.
>> Ahh, OK, the new code will now let dwim_ref/log to process @{-N}
>> again (the log message hints this but it wasn't all that clear),
>
> I thought it was clear we would let dwim_{ref,log} do the job:
Yes, the reason I did not immediately think of that is because I
knew @{-N} was expensive (need to read reflog backwards) and didn't
think anybody would redo the code to deliberately do that twice ;-)
>> Also, a few points this patch highlights in the code before the
>> change:
>>
>> - If we were on a branch with 40-hex name at nth prior checkout,
>>would we mistake it as being detached at the commit?
>>
>> - If we were on a branch 'foo' at nth prior checkout, would our
>>previous get_sha1_1() have made us mistake it as referring to a
>>tag 'foo' with the same name if it exists?
>
> I don't know, but I suspect there's no change after this patch.
Yes, didn't I say "the code before the change" above?
These two correctness issues look more important issues to me, with
or without the restructure patch (in other words, they are
independent).
--
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 v2 10/11] sha1_name: reorganize get_sha1_basic()
On Wed, May 8, 2013 at 1:18 PM, Junio C Hamano wrote:
> Felipe Contreras writes:
>
>> Through the years the functionality to handle @{-N} and @{u} has moved
>> around the code, and as a result, code that once made sense, doesn't any
>> more.
>>
>> There is no need to call this function recursively with the branch of
>> @{-N} substituted because dwim_{ref,log} already replaces it.
>>
>> However, there's one corner-case where @{-N} resolves to a detached
>> HEAD, in which case we wouldn't get any ref back.
>>
>> So we parse the nth-prior manually, and deal with it depending on
>> weather it's a SHA-1, or a ref.
>> ...
>
> s/weather/whether/;
>
>> @@ -447,6 +448,10 @@ static int get_sha1_basic(const char *str, int len,
>> unsigned char *sha1)
>> if (len && str[len-1] == '}') {
>> for (at = len-4; at >= 0; at--) {
>> if (str[at] == '@' && str[at+1] == '{') {
>> + if (at == 0 && str[2] == '-') {
>> + nth_prior = 1;
>> + continue;
>> + }
>
> Does this have to be inside the loop?
Yes, the whole purpose is to avoid reflog_len to be set.
>> @@ -460,19 +465,22 @@ static int get_sha1_basic(const char *str, int len,
>> unsigned char *sha1)
>> if (len && ambiguous_path(str, len))
>> return -1;
>>
>> - if (!len && reflog_len) {
>> + if (nth_prior) {
>> struct strbuf buf = STRBUF_INIT;
>> - int ret;
>> - /* try the @{-N} syntax for n-th checkout */
>> - ret = interpret_branch_name(str, &buf);
>> - if (ret > 0)
>> - /* substitute this branch name and restart */
>> - return get_sha1_1(buf.buf, buf.len, sha1, 0);
>> - else if (ret == 0)
>> - return -1;
>> + int detached;
>> +
>> + if (interpret_nth_prior_checkout(str, &buf) > 0) {
>> + detached = (buf.len == 40 && !get_sha1_hex(buf.buf,
>> sha1));
>> + strbuf_release(&buf);
>> + if (detached)
>> + return 0;
>> + }
>> + }
>
> Earlier, if @{-N} resolved to a detached head, we just fed it to
> get_sha1_1(). If it resolved to a concrete refname, we also fed it
> to get_sha1_1(). We ended up calling ourselves again and did the
> right thing either way.
>
> The new code bypasses the recursive call when we get a detached head
> back, because we know that calling get_sha1_1() with the 40-hex will
> eventually take us back to this codepath, and immediately return
> when it sees get_sha1_hex() succeeds.
>
> What happens when str @{-N} leaves a concrete refname in buf.buf?
> The branch name is lost with strbuf_release(), and then where do we
> go from here? Continuing down from here would run dwim_ref/log on
> str which is still @{-N}, no?
>
> Ahh, OK, the new code will now let dwim_ref/log to process @{-N}
> again (the log message hints this but it wasn't all that clear),
I thought it was clear we would let dwim_{ref,log} do the job:
---
There is no need to call this function recursively with the branch of
@{-N} substituted because dwim_{ref,log} already replaces it.
---
> That is somewhat contrived, and I am not so sure if that is a good
> reorganization.
But much less contrived than before, because the code that deals with
@{-N} is in one place, instead of sprinkled all over as many
corner-cases, and there's no recursion.
> Also, a few points this patch highlights in the code before the
> change:
>
> - If we were on a branch with 40-hex name at nth prior checkout,
>would we mistake it as being detached at the commit?
>
> - If we were on a branch 'foo' at nth prior checkout, would our
>previous get_sha1_1() have made us mistake it as referring to a
>tag 'foo' with the same name if it exists?
I don't know, but I suspect there's no change after this patch.
--
Felipe Contreras
--
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 v2 10/11] sha1_name: reorganize get_sha1_basic()
Junio C Hamano writes:
> Also, a few points this patch highlights in the code before the
> change:
>
> - If we were on a branch with 40-hex name at nth prior checkout,
>would we mistake it as being detached at the commit?
>
> - If we were on a branch 'foo' at nth prior checkout, would our
>previous get_sha1_1() have made us mistake it as referring to a
>tag 'foo' with the same name if it exists?
>
> The former needs a fix to either writing of reflog or reading by
> interpret_nth_prior_checkout() so that we can tell these cases apart
> more reliably. Then the latter can be solved by splicing
> refs/heads/ in front of the branch name before recursively calling
> get_sha1_1() in the original code (and similar fix can be
> forward-ported to this patch).
>
> Incidentally, I think if we prefix refs/heads/ in front and feed
> that to dwim_ref/log, we would also avoid running @{-N} twice (which
> I suspect might be more expensive than a simple recursion, as it
> needs to go through the reflog file), which may be a nice side
> effect of such a fix we would get for free.
Here is the first step (i.e. more reliable interpret_nth_prior).
I looked at all the existing callers of interpret_branch_name() and
it appears to me that most of them currently assume they are getting
a branch name, because they want to work on a ref, and some of them
do not care, because they are working on a revision. For the
former, they can (and should) error out instead of relying on not
having refs/heads/$detached_SHA1 that will prevent them from working
on a ref which is what they currently do, if they had the "detached"
information. For the latter, if we give "detached" information,
they can either prefix "refs/heads/" (if the result is "not
detached") to call resolve_ref(), or call get_sha1_hex (if the
result is "detached"), which would be the solution for the second
issue I noticed in the message I am replying to.
The next step on top of this patch may be to expose the "detached"
bit up in the API chain to let callers of interpret_branch_name() to
know about the distinction.
sha1_name.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/sha1_name.c b/sha1_name.c
index 3820f28..3dd6667 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -862,6 +862,7 @@ static int get_sha1_oneline(const char *prefix, unsigned
char *sha1,
struct grab_nth_branch_switch_cbdata {
int remaining;
struct strbuf buf;
+ int detached;
};
static int grab_nth_branch_switch(unsigned char *osha1, unsigned char *nsha1,
@@ -880,9 +881,14 @@ static int grab_nth_branch_switch(unsigned char *osha1,
unsigned char *nsha1,
if (!match || !target)
return 0;
if (--(cb->remaining) == 0) {
+ unsigned char sha1[20];
+
len = target - match;
strbuf_reset(&cb->buf);
strbuf_add(&cb->buf, match, len);
+ cb->detached = (len == 40 &&
+ !get_sha1_hex(match, sha1) &&
+ !hashcmp(osha1, sha1));
return 1; /* we are done */
}
return 0;
@@ -892,7 +898,7 @@ static int grab_nth_branch_switch(unsigned char *osha1,
unsigned char *nsha1,
* Parse @{-N} syntax, return the number of characters parsed
* if successful; otherwise signal an error with negative value.
*/
-static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf)
+static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf,
int *detached)
{
long nth;
int retval;
@@ -917,6 +923,8 @@ static int interpret_nth_prior_checkout(const char *name,
struct strbuf *buf)
if (0 < for_each_reflog_ent_reverse("HEAD", grab_nth_branch_switch,
&cb)) {
strbuf_reset(buf);
strbuf_add(buf, cb.buf.buf, cb.buf.len);
+ if (detached)
+ *detached = cb.detached;
retval = brace - name + 1;
}
@@ -992,7 +1000,7 @@ int interpret_branch_name(const char *name, struct strbuf
*buf)
char *cp;
struct branch *upstream;
int namelen = strlen(name);
- int len = interpret_nth_prior_checkout(name, buf);
+ int len = interpret_nth_prior_checkout(name, buf, NULL);
int tmp_len;
if (!len)
--
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 v2 10/11] sha1_name: reorganize get_sha1_basic()
Felipe Contreras writes:
> Through the years the functionality to handle @{-N} and @{u} has moved
> around the code, and as a result, code that once made sense, doesn't any
> more.
>
> There is no need to call this function recursively with the branch of
> @{-N} substituted because dwim_{ref,log} already replaces it.
>
> However, there's one corner-case where @{-N} resolves to a detached
> HEAD, in which case we wouldn't get any ref back.
>
> So we parse the nth-prior manually, and deal with it depending on
> weather it's a SHA-1, or a ref.
> ...
s/weather/whether/;
> @@ -447,6 +448,10 @@ static int get_sha1_basic(const char *str, int len,
> unsigned char *sha1)
> if (len && str[len-1] == '}') {
> for (at = len-4; at >= 0; at--) {
> if (str[at] == '@' && str[at+1] == '{') {
> + if (at == 0 && str[2] == '-') {
> + nth_prior = 1;
> + continue;
> + }
Does this have to be inside the loop?
> @@ -460,19 +465,22 @@ static int get_sha1_basic(const char *str, int len,
> unsigned char *sha1)
> if (len && ambiguous_path(str, len))
> return -1;
>
> - if (!len && reflog_len) {
> + if (nth_prior) {
> struct strbuf buf = STRBUF_INIT;
> - int ret;
> - /* try the @{-N} syntax for n-th checkout */
> - ret = interpret_branch_name(str, &buf);
> - if (ret > 0)
> - /* substitute this branch name and restart */
> - return get_sha1_1(buf.buf, buf.len, sha1, 0);
> - else if (ret == 0)
> - return -1;
> + int detached;
> +
> + if (interpret_nth_prior_checkout(str, &buf) > 0) {
> + detached = (buf.len == 40 && !get_sha1_hex(buf.buf,
> sha1));
> + strbuf_release(&buf);
> + if (detached)
> + return 0;
> + }
> + }
Earlier, if @{-N} resolved to a detached head, we just fed it to
get_sha1_1(). If it resolved to a concrete refname, we also fed it
to get_sha1_1(). We ended up calling ourselves again and did the
right thing either way.
The new code bypasses the recursive call when we get a detached head
back, because we know that calling get_sha1_1() with the 40-hex will
eventually take us back to this codepath, and immediately return
when it sees get_sha1_hex() succeeds.
What happens when str @{-N} leaves a concrete refname in buf.buf?
The branch name is lost with strbuf_release(), and then where do we
go from here? Continuing down from here would run dwim_ref/log on
str which is still @{-N}, no?
Ahh, OK, the new code will now let dwim_ref/log to process @{-N}
again (the log message hints this but it wasn't all that clear), so
even though we already learned the branch name in buf here and
discard it, we will eventually discover the same information later.
That is somewhat contrived, and I am not so sure if that is a good
reorganization.
Also, a few points this patch highlights in the code before the
change:
- If we were on a branch with 40-hex name at nth prior checkout,
would we mistake it as being detached at the commit?
- If we were on a branch 'foo' at nth prior checkout, would our
previous get_sha1_1() have made us mistake it as referring to a
tag 'foo' with the same name if it exists?
The former needs a fix to either writing of reflog or reading by
interpret_nth_prior_checkout() so that we can tell these cases apart
more reliably. Then the latter can be solved by splicing
refs/heads/ in front of the branch name before recursively calling
get_sha1_1() in the original code (and similar fix can be
forward-ported to this patch).
Incidentally, I think if we prefix refs/heads/ in front and feed
that to dwim_ref/log, we would also avoid running @{-N} twice (which
I suspect might be more expensive than a simple recursion, as it
needs to go through the reflog file), which may be a nice side
effect of such a fix we would get for free.
> +
> + if (!len && reflog_len)
> /* allow "@{...}" to mean the current branch reflog */
> refs_found = dwim_ref("HEAD", 4, sha1, &real_ref);
> - } else if (reflog_len)
> + else if (reflog_len)
> refs_found = dwim_log(str, len, sha1, &real_ref);
> else
> refs_found = dwim_ref(str, len, sha1, &real_ref);
--
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 v2 10/11] sha1_name: reorganize get_sha1_basic()
On Wed, May 8, 2013 at 2:39 AM, Ramkumar Ramachandra wrote: > Felipe Contreras wrote: >> --- >> sha1_name.c | 30 +++--- >> 1 file changed, 19 insertions(+), 11 deletions(-) > > How has this changed since my eyeballing of the previous version? An > inter-diff would be nice: having spent a significant amount of time > looking at this area, I can confirm that the patch is Correct. You mean this compared to v4? It hasn't changed. -- Felipe Contreras -- 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 v2 10/11] sha1_name: reorganize get_sha1_basic()
Felipe Contreras wrote: > --- > sha1_name.c | 30 +++--- > 1 file changed, 19 insertions(+), 11 deletions(-) How has this changed since my eyeballing of the previous version? An inter-diff would be nice: having spent a significant amount of time looking at this area, I can confirm that the patch is Correct. -- 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
[PATCH v2 10/11] sha1_name: reorganize get_sha1_basic()
Through the years the functionality to handle @{-N} and @{u} has moved
around the code, and as a result, code that once made sense, doesn't any
more.
There is no need to call this function recursively with the branch of
@{-N} substituted because dwim_{ref,log} already replaces it.
However, there's one corner-case where @{-N} resolves to a detached
HEAD, in which case we wouldn't get any ref back.
So we parse the nth-prior manually, and deal with it depending on
weather it's a SHA-1, or a ref.
Signed-off-by: Felipe Contreras
---
sha1_name.c | 30 +++---
1 file changed, 19 insertions(+), 11 deletions(-)
diff --git a/sha1_name.c b/sha1_name.c
index 0acc6e0..c512c69 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -431,13 +431,14 @@ static inline int upstream_mark(const char *string, int
len)
}
static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned
lookup_flags);
+static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf);
static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
{
static const char *warn_msg = "refname '%.*s' is ambiguous.";
char *real_ref = NULL;
int refs_found = 0;
- int at, reflog_len;
+ int at, reflog_len, nth_prior = 0;
if (len == 40 && !get_sha1_hex(str, sha1))
return 0;
@@ -447,6 +448,10 @@ static int get_sha1_basic(const char *str, int len,
unsigned char *sha1)
if (len && str[len-1] == '}') {
for (at = len-4; at >= 0; at--) {
if (str[at] == '@' && str[at+1] == '{') {
+ if (at == 0 && str[2] == '-') {
+ nth_prior = 1;
+ continue;
+ }
if (!upstream_mark(str + at, len - at)) {
reflog_len = (len-1) - (at+2);
len = at;
@@ -460,19 +465,22 @@ static int get_sha1_basic(const char *str, int len,
unsigned char *sha1)
if (len && ambiguous_path(str, len))
return -1;
- if (!len && reflog_len) {
+ if (nth_prior) {
struct strbuf buf = STRBUF_INIT;
- int ret;
- /* try the @{-N} syntax for n-th checkout */
- ret = interpret_branch_name(str, &buf);
- if (ret > 0)
- /* substitute this branch name and restart */
- return get_sha1_1(buf.buf, buf.len, sha1, 0);
- else if (ret == 0)
- return -1;
+ int detached;
+
+ if (interpret_nth_prior_checkout(str, &buf) > 0) {
+ detached = (buf.len == 40 && !get_sha1_hex(buf.buf,
sha1));
+ strbuf_release(&buf);
+ if (detached)
+ return 0;
+ }
+ }
+
+ if (!len && reflog_len)
/* allow "@{...}" to mean the current branch reflog */
refs_found = dwim_ref("HEAD", 4, sha1, &real_ref);
- } else if (reflog_len)
+ else if (reflog_len)
refs_found = dwim_log(str, len, sha1, &real_ref);
else
refs_found = dwim_ref(str, len, sha1, &real_ref);
--
1.8.3.rc0.401.g45bba44
--
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
