Re: [PATCH v2 10/11] sha1_name: reorganize get_sha1_basic()

On Wed, May 8, 2013 at 1:18 PM, Junio C Hamano [off-list ref] wrote:

Felipe Contreras [off-list ref] writes:

quoted

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/;

quoted

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

quoted

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