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

On Thu, May 2, 2013 at 1:55 PM, Ramkumar Ramachandra [off-list ref] wrote:

Felipe Contreras wrote:

quoted

[...]

Okay, you used nth_prior in this one.

quoted

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.

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

quoted

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

quoted

                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.

quoted

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