Re: [PATCH v3 4/9] ref-filter: add support to sort by version
flat view
From: Eric Sunshine <hidden>
Date: 2016-06-15 23:05:49
On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak [off-list ref] wrote:
Add support to sort by version using the "v:refname" and "version:refname" option. This is achieved by using the 'version_cmp()' function as the comparing function for qsort.
To agree with the actual code: s/version_cmp/versioncmp/
quoted hunk ↗ jump to hunk
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 <redacted> ---diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index e49d578..cc91275 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt@@ -144,6 +144,8 @@ blank line. Finally, the optional GPG signature is `contents:signature`. 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 is done using the +field names ('version:refname' or 'v:refname').
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. 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.
quoted hunk ↗ jump to hunk
In any case, a field name that refers to a field inapplicable to the object referred by the ref does not cause an error. Itdiff --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.)
quoted hunk ↗ jump to hunk
} 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.
quoted hunk ↗ jump to hunk
}; 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?
+ cat >expect <<-\EOF && + foo1.10 + foo1.6 + foo1.3 + EOF + test_cmp expect actual +' + test_done -- 2.4.6