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

[Felipe Contreras <felipe.contreras@gmail.com>]

On Wed, May 8, 2013 at 1:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> 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/;
>
>> @@ -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.

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