Re: [PATCH v3 4/9] ref-filter: add support to sort by version

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

On Mon, Jul 20, 2015 at 7:09 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> To agree with the actual code: s/version_cmp/versioncmp/
>

Yeah! will change.

>
> Assuming I'm a reader without prior knowledge, the first question
> which pops into my mind is "what's the difference between
> 'version:refname' and 'v:refname'?" Is one just shorthand for the
> other, or is there some subtle difference in behavior, or what? The
> documentation should explain this better.
>

Will include more explanation.

> Also, why are there parentheses around 'version:refname' or
> 'v:refname'? And, you should use backticks rather than apostrophes, as
> is done with the other field names.
>

Wanted to show that they are the same command. Seems confusing now that
you mentioned, will change it.

>>  In any case, a field name that refers to a field inapplicable to
>>  the object referred by the ref does not cause an error.  It
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 82731ac..85c561e 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1169,18 +1170,22 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
>>
>>         get_ref_atom_value(a, s->atom, &va);
>>         get_ref_atom_value(b, s->atom, &vb);
>> -       switch (cmp_type) {
>> -       case 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;
>> +       if (s->version)
>> +               cmp = versioncmp(va->s, vb->s);
>> +       else {
>> +               switch (cmp_type) {
>> +               case 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;
>> +               }
>
> The logic might be slightly easier to follow, and give a much less
> noisy diff if you rewrite it like this instead:
>
>     if (s->version)
>         cmp = versioncmp(va->s, vb->s);
>     else if (cmp_type == FIELD_STR)
>         cmp = strcmp(va->s, vb->s);
>     else {
>         if (va->ul < vb->ul)
>             cmp = -1;
>         else if (va->ul == vb->ul)
>             cmp = 0;
>         else
>             cmp = 1;
>     }
>
> Or, if you don't mind a noisy diff, you can outdent the other branches, as well:
>
>     if (s->version)
>        cmp = versioncmp(va->s, vb->s);
>     else if (cmp_type == FIELD_STR)
>        cmp = strcmp(va->s, vb->s);
>     else if (va->ul < vb->ul)
>        cmp = -1;
>     else if (va->ul == vb->ul)
>        cmp = 0;
>     else
>        cmp = 1;
>
> (I rather prefer the latter, despite the noisy diff.)
>

Err! just didn't want to existing code. Your latter code usage seems
easier and simpler to follow. Will use that thanks :D

>>         }
>>         return (s->reverse) ? -cmp : cmp;
>>  }
>> diff --git a/ref-filter.h b/ref-filter.h
>> index 7dfdea0..6f1646b 100644
>> --- a/ref-filter.h
>> +++ b/ref-filter.h
>> @@ -25,7 +25,7 @@ struct atom_value {
>>  struct ref_sorting {
>>         struct ref_sorting *next;
>>         int atom; /* index into used_atom array (internal) */
>> -       unsigned reverse : 1;
>> +       unsigned reverse : 1, version : 1;
>
> This is difficult to read. Style elsewhere (if I'm not mistaken) is to
> place the declaration on a line by itself.
>

Yes just checked, thats the style. Thank you.

>>  };
>>
>>  struct ref_array_item {
>> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
>> index 505a360..c31fd2f 100755
>> --- a/t/t6302-for-each-ref-filter.sh
>> +++ b/t/t6302-for-each-ref-filter.sh
>> @@ -81,4 +81,30 @@ test_expect_success 'filtering with --contains' '
>>         test_cmp expect actual
>>  '
>>
>> +test_expect_success 'version sort' '
>> +       git tag -l --sort=version:refname | grep "foo" >actual &&
>> +       cat >expect <<-\EOF &&
>> +       foo1.3
>> +       foo1.6
>> +       foo1.10
>> +       EOF
>> +       test_cmp expect actual
>> +'
>> +
>> +test_expect_success 'reverse version sort' '
>> +       git tag -l --sort=-version:refname | grep "foo" >actual &&
>
> Maybe use 'v:refname' in one of these tests in order to exercise that
> alias as well?
>

The idea was to only include a minimal test as t7004 has tests for the same,
but I guess a minimal test for 'v:refname' is also required.

-- 
Regards,
Karthik Nayak