On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak <karthik.188@xxxxxxxxx> 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/
> 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@xxxxxxxxx>
> ---
> 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.
> 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.)
> }
> 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.
> };
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html