[PATCH v2] sha1_name: reorganize get_sha1_basic()

[PATCH v2] sha1_name: reorganize get_sha1_basic()

[Felipe Contreras]

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 <felipe.contreras@gmail.com>
---
 sha1_name.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 3820f28..6428001 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-2; 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,20 +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+at, &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

Re: [PATCH v2] sha1_name: reorganize get_sha1_basic()

[Felipe Contreras]

On Thu, May 2, 2013 at 12:48 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> 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.

Forgot again: Inspired by a patch from Ramkumar Ramachandra.

-- 
Felipe Contreras

Re: [PATCH v2] sha1_name: reorganize get_sha1_basic()

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> [...]

Okay, you used nth_prior in this one.

> There is no need to call this function recursively with the branch of
> @{-N} substituted because dwim_{ref,log} already replaces it.

I figured that the recursion is because dwim_{ref,log} didn't exist
when this was written.

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

Right.  _This_ is the special case, which the old logic didn't quite
convey.  The end-user version of this is: 'git checkout -' won't bring
you back to the branch if you said git checkout HEAD~1 earlier.

> diff --git a/sha1_name.c b/sha1_name.c
> index 3820f28..6428001 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);

It didn't strike me to use interpret_nth_prior_checkout() directly.  I
was still stuck at interpret_branch_name() returning a positive value.

>  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-2; at >= 0; at--) {
>                         if (str[at] == '@' && str[at+1] == '{') {
> +                               if (at == 0 && str[2] == '-') {
> +                                       nth_prior = 1;
> +                                       continue;
> +                               }

Looking at this closely once again.
You've already hit the beginning.  What are you continuing?  Take the
example of a compound expression with @{-

@{-1}@{0}
     ^ at is here
     "@{-" is not matched

@{-1}@{0}
^ at is here
"@{-" is matched
What's to continue? at is already at 0

On another note, I think you've fixed a bug: @{-1}{0} was parsing to
the same value as @{-1}@{0} before your patch.

> @@ -460,20 +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) {

nth_prior makes this much cleaner overall.

>                 struct strbuf buf = STRBUF_INIT;
> -               int ret;
> -               /* try the @{-N} syntax for n-th checkout */
> -               ret = interpret_branch_name(str+at, &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;

Neat.  I'd set reflog_len to zero and made sure that the last part of
the function wouldn't be executed.  How did you get away without
setting refs_found to 1 though?

>                 }
> +       }
> +
> +       if (!len && reflog_len)
>                 /* allow "@{...}" to mean the current branch reflog */
>                 refs_found = dwim_ref("HEAD", 4, sha1, &real_ref);

I got this part wrong too: I said dwim_log() instead of dwim_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
>

Re: [PATCH v2] sha1_name: reorganize get_sha1_basic()

[Felipe Contreras]

On Thu, May 2, 2013 at 1:55 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> Felipe Contreras wrote:
>> [...]
>
> Okay, you used nth_prior in this one.
>
>> There is no need to call this function recursively with the branch of
>> @{-N} substituted because dwim_{ref,log} already replaces it.
>
> I figured that the recursion is because dwim_{ref,log} didn't exist
> when this was written.

They did, but they were not substituting the branches.

>> @@ -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-2; at >= 0; at--) {
>>                         if (str[at] == '@' && str[at+1] == '{') {
>> +                               if (at == 0 && str[2] == '-') {
>> +                                       nth_prior = 1;
>> +                                       continue;
>> +                               }
>
> Looking at this closely once again.
> You've already hit the beginning.  What are you continuing?  Take the
> example of a compound expression with @{-

Yeah, we could break, but I would prefer the break to happen naturally
when in the for loop check.

> On another note, I think you've fixed a bug: @{-1}{0} was parsing to
> the same value as @{-1}@{0} before your patch.

Yeap.

>> @@ -460,20 +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) {
>
> nth_prior makes this much cleaner overall.
>
>>                 struct strbuf buf = STRBUF_INIT;
>> -               int ret;
>> -               /* try the @{-N} syntax for n-th checkout */
>> -               ret = interpret_branch_name(str+at, &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;
>
> Neat.  I'd set reflog_len to zero and made sure that the last part of
> the function wouldn't be executed.  How did you get away without
> setting refs_found to 1 though?

The rest of the code is not executed, there's no need if @{-N}
evaluates to a SHA-1. There's no ref to dwim, and there's no reflog
anyway. We just fetch the SHA-1 and return.

>>                 }
>> +       }
>> +
>> +       if (!len && reflog_len)
>>                 /* allow "@{...}" to mean the current branch reflog */
>>                 refs_found = dwim_ref("HEAD", 4, sha1, &real_ref);
>
> I got this part wrong too: I said dwim_log() instead of dwim_ref().

Fortunately we are not changing the code this time, which is the best
way to make sure that the behavior doesn't change.

It took me a long time to play with alternatives and find a clean
solution with minimal changes that is easy to understand, but I think
this code does the trick.

Cheers.

-- 
Felipe Contreras

Re: [PATCH v2] sha1_name: reorganize get_sha1_basic()

[Ramkumar Ramachandra]

Felipe Contreras wrote:
>> Looking at this closely once again.
>> You've already hit the beginning.  What are you continuing?  Take the
>> example of a compound expression with @{-
>
> Yeah, we could break, but I would prefer the break to happen naturally
> when in the for loop check.

This is followed by a condition on upstream_mark: just change that
from if/else if/ and there's a break; at the end anyway.

The continue is misleading and should be removed.

>> On another note, I think you've fixed a bug: @{-1}{0} was parsing to
>> the same value as @{-1}@{0} before your patch.
>
> Yeap.

Write a note about it in the commit message atleast?  I found it to be
a very non-trivial conclusion.

>>> +               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;
>>
>> Neat.  I'd set reflog_len to zero and made sure that the last part of
>> the function wouldn't be executed.  How did you get away without
>> setting refs_found to 1 though?
>
> The rest of the code is not executed, there's no need if @{-N}
> evaluates to a SHA-1. There's no ref to dwim, and there's no reflog
> anyway. We just fetch the SHA-1 and return.

Obviously the return 0 breaks out of the function.  I meant what
happens if it's not detached.  I'll answer it myself: you have a
string that's either not 40-characters, or doesn't resolve to a valid
object.  You haven't found anything yet.  Now, you'll be going down
the reflog_len codepath and calling dwim_log() to set the refs_found.

Re: [PATCH v2] sha1_name: reorganize get_sha1_basic()

[Ramkumar Ramachandra]

A small suggestion.  Squash this in if you like; optionally submit it
as a separate part.

diff --git a/sha1_name.c b/sha1_name.c
index 6428001..109ab41 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -448,11 +448,12 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
        if (len && str[len-1] == '}') {
                for (at = len-2; 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)) {
+                               if (str[at+2] == '-') {
+                                       if (at != 0)
+                                               return -1;
+                                       else
+                                               nth_prior = 1;
+                               } else if (!upstream_mark(str + at, len - at)) {
                                        reflog_len = (len-1) - (at+2);
                                        len = at;
                                }
@@ -497,10 +498,6 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
                unsigned long co_time;
                int co_tz, co_cnt;
 
-               /* a @{-N} placed anywhere except the start is an error */
-               if (str[at+2] == '-')
-                       return -1;
-
                /* Is it asking for N-th entry, or approxidate? */
                for (i = nth = 0; 0 <= nth && i < reflog_len; i++) {
                        char ch = str[at+2+i];

Re: [PATCH v2] sha1_name: reorganize get_sha1_basic()

[Felipe Contreras]

On Thu, May 2, 2013 at 2:47 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> A small suggestion.  Squash this in if you like; optionally submit it
> as a separate part.
>
> diff --git a/sha1_name.c b/sha1_name.c
> index 6428001..109ab41 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -448,11 +448,12 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
>         if (len && str[len-1] == '}') {
>                 for (at = len-2; 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)) {
> +                               if (str[at+2] == '-') {
> +                                       if (at != 0)
> +                                               return -1;
> +                                       else
> +                                               nth_prior = 1;
> +                               } else if (!upstream_mark(str + at, len - at)) {

Generally I don't like this style, it's prone to multiple indentation levels.

                        if (str[at] == '@' && str[at+1] == '{') {
-                               if (at == 0 && str[2] == '-') {
+                               if (str[at+2] == '-') {
+                                       if (at != 0)
+                                               /* @{-N} not at start */
+                                               return -1;
                                        nth_prior = 1;
                                        continue;
                                }

Otherwise makes sense, but I would do it as a separate patch.

-- 
Felipe Contreras