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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: May 2, 2013, 19:10 UTC
Message-ID: <CAMP44s3UTwgcxQ0Y89UtjBAw46ps_2EOGqZ=OG499G_uYL0DCA@mail.gmail.com>
In-Reply-To: <CALkWK0k4YisL0Nc6906FRsx7556UVqy_DD9Wo=1j4OWMkwqK-A@mail.gmail.com>

On Thu, May 2, 2013 at 1:55 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

Show 10 quoted lines

> Felipe Contreras wrote:
>> [...]
>
> Okay, you used nth_prior in this one.
>
>> 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.

Show 12 quoted lines

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

Show 29 quoted lines

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

Show 8 quoted lines

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