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

Felipe Contreras wrote:

quoted

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.

quoted

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.

quoted

quoted

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