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

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

On Wed, May 8, 2013 at 4:51 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> 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.
>
> What I meant was the "<nothing>@{-" check, which happens only at==0.
>
>         if (!memcmp(str, "@{-", 3) && len > 3)
>                 nth_prior = 1;
>         else
>                 for (at = len - 4; at; at--) {
>                         ... look for and break at the first "@{" ...
>                 }
>
> or something.

That's doable, but would screw up the next patch.

>>> 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:
>
> Yes, the reason I did not immediately think of that is because I
> knew @{-N} was expensive (need to read reflog backwards) and didn't
> think anybody would redo the code to deliberately do that twice ;-)

But that's what the commit message said.

>>> 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.
>
> Yes, didn't I say "the code before the change" above?
>
> These two correctness issues look more important issues to me, with
> or without the restructure patch (in other words, they are
> independent).

Right, if you are interested in correctness, you might want to try
@{-1}{0}; it resolves to @{-1} currently, and it fails correctly with
my patch.

Cheers.

-- 
Felipe Contreras