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

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: May 2, 2013, 19:35 UTC
Message-ID: <CALkWK0=1qxWWRT9ic_4uVg_ZF_5T0_ogpWwTHotqxWqsi9GOnA@mail.gmail.com>
In-Reply-To: <CAMP44s3UTwgcxQ0Y89UtjBAw46ps_2EOGqZ=OG499G_uYL0DCA@mail.gmail.com>

Felipe Contreras wrote:

Show 6 quoted lines

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

Show 13 quoted lines

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