Re: [PATCH v4 05/10] ref-filter: add support to sort by version

[Karthik Nayak <karthik.188@gmail.com>]

On Sun, Jul 26, 2015 at 4:10 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> @@ -1180,19 +1181,17 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
>>
>>       get_ref_atom_value(&state, a, s->atom, &va);
>>       get_ref_atom_value(&state, b, s->atom, &vb);
>> -     switch (cmp_type) {
>> -     case FIELD_STR:
>> +     if (s->version)
>> +             cmp = versioncmp(va->s, vb->s);
>> +     else if (cmp_type == FIELD_STR)
>>               cmp = strcmp(va->s, vb->s);
>> -             break;
>> -     default:
>> -             if (va->ul < vb->ul)
>> -                     cmp = -1;
>> -             else if (va->ul == vb->ul)
>> -                     cmp = 0;
>> -             else
>> -                     cmp = 1;
>> -             break;
>> -     }
>> +     else if (va->ul < vb->ul)
>> +             cmp = -1;
>> +     else if (va->ul == vb->ul)
>> +             cmp = 0;
>> +     else
>> +             cmp = 1;
>> +
>
> So there are generally three kinds of comparison possible:
>
>     - if it is to be compared as versions, do versioncmp
>     - if it is to be compared as strings, do strcmp
>     - if it is to be compared as numbers, do <=> but because
>       we are writing in C, not in Perl, do so as if/else/else
>
> Having understood that, the above is not really easy to read and
> extend.  We should structure the above more like this:
>
>         if (s->version)
>                 ... versioncmp
>         else if (... FIELD_STR)
>                 ... strcmp
>         else {
>                 if (a < b)
>                         ...
>                 else if (a == b)
>                         ...
>                 else
>                         ...
>         }
>
> so that it would be obvious how this code need to be updated
> when we need to add yet another kind of comparison.
>

I find the current version more pleasing to read, The way you've explained
it though, it seems that its better to structure it the way you've
mentioned as this
actually shows the code flow of the three kinds of comparison possible.

> Without looking at the callers, s->version looks like a misdesign
> that should be updated to use the same cmp_type mechanism?  That
> would lead to even more obvious construct that is easy to enhance,
> i.e.
>
>         switch (cmp_type) {
>         case CMP_VERSION:
>                 ...
>         case CMP_STRING:
>                 ...
>         case CMP_NUMBER:
>                 ...
>         }
>
> I dunno.
>
> Other than that (and the structure of that "format-state" stuff we
> discussed separately), the series was a pleasant read.
>
> Thanks.

That was the previous design, but Duy asked me to do this so
that we could support all atoms. And I agree with him on this.

http://article.gmane.org/gmane.comp.version-control.git/273888

-- 
Regards,
Karthik Nayak