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

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

On Thu, Jul 30, 2015 at 1:04 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tuesday, July 28, 2015, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Add support to sort by version using the "v:refname" and
>> "version:refname" option. This is achieved by using the 'versioncmp()'
>> function as the comparing function for qsort.
>>
>> This option is included to support sorting by versions in `git tag -l`
>> which will eventaully be ported to use ref-filter APIs.
>>
>> Add documentation and tests for the same.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>> index 45dd7f8..2b60aee 100644
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -151,6 +151,9 @@ For sorting purposes, fields with numeric values sort in numeric
>>  order (`objectsize`, `authordate`, `committerdate`, `taggerdate`).
>>  All other fields are used to sort in their byte-value order.
>>
>> +There is also an option to sort by versions, this can be done by using
>> +the fieldname `version:refname` or in short `v:refname`.
>
> Nit: "or in short" reads a bit oddly. Perhaps:
>
>     ...`version:refname` or its alias `v:refname`.
>
> or
>
>     ...`version:refname` or the short-form `v:refname`.
>
> (I rather prefer the "alias" alternative.)
>

Thanks! will change.

>>  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
>>  returns an empty string instead.
>> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
>> index 19ac480..68688a9 100755
>> --- a/t/t6302-for-each-ref-filter.sh
>> +++ b/t/t6302-for-each-ref-filter.sh
>> @@ -97,4 +97,40 @@ test_expect_success 'padding to the right using `padright`' '
>> +test_expect_success 'version sort' '
>> +       git for-each-ref --sort=version:refname --format="%(refname:short)" refs/tags/ | grep "foo" >actual &&
>> +       cat >expect <<-\EOF &&
>> +       foo1.3
>> +       foo1.6
>> +       foo1.10
>> +       EOF
>> +       test_cmp expect actual
>> +'
>> +
>> +test_expect_success 'version sort (shortened)' '
>> +       git for-each-ref --sort=v:refname --format="%(refname:short)" refs/tags/ | grep "foo" >actual &&
>> +       cat >expect <<-\EOF &&
>> +       foo1.3
>> +       foo1.6
>> +       foo1.10
>> +       EOF
>> +       test_cmp expect actual
>
> Nit: In the earlier review when I suggested using "v:refname" for one
> of the tests in order to exercise it (in addition to
> "version:refname"), I didn't mean that you had to add another
> (identical) test but rather that you could have one of the existing
> tests use "v:refname". (Not a big deal. You can leave this as is if
> you like. I just wanted to clarify.)
>

Ah! Thats ok, this seems to explain the commit entirely showing that
"v:refname" and "version:refname" are the same thing.

-- 
Regards,
Karthik Nayak