On Mon, Jul 20, 2015 at 7:09 AM, Eric Sunshine <sunsh...@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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
