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

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

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