git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Eric Sunshine <sunshine@sunshineco.com>
Date
Jul 20, 2015, 01:39 UTC
Message-ID
<CAPig+cRkwy_Dq3oYgHdNH6naaUDHZ_6DPopec1EG3aNwz56rzA@mail.gmail.com>
In-Reply-To
<1437246749-14423-5-git-send-email-Karthik.188@gmail.com>
On Sat, Jul 18, 2015 at 3:12 PM, 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
> 'version_cmp()' function as the comparing function for qsort.
To agree with the actual code: s/version_cmp/versioncmp/
Show 17 quoted lines
> 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 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.

Show 38 quoted lines
>  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.)
Show 13 quoted lines
>         }
>         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.

Show 23 quoted lines
>  };
>
>  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?

Show 11 quoted lines
> +       cat >expect <<-\EOF &&
> +       foo1.10
> +       foo1.6
> +       foo1.3
> +       EOF
> +       test_cmp expect actual
> +'
> +
>  test_done
> --
> 2.4.6
Previous: Karthik NayakNext: Karthik Nayak
Message 21 of 37 in “Port tag.c to use ref-filter APIs”
  1. 0/9 Port tag.c to use ref-filter APIsKarthik Nayak, Jul 18, 2015
  2. 1/9 ref-filter: add option to align atoms to the leftKarthik Nayak, Jul 18, 2015
  3. Eric SunshineJul 19, 2015
  4. Karthik NayakJul 20, 2015
  5. Junio C HamanoJul 20, 2015
  6. Eric SunshineJul 20, 2015
  7. Junio C HamanoJul 20, 2015
  8. Karthik NayakJul 20, 2015
  9. 1/9 ref-filter: add option to align atoms to the leftKarthik Nayak, Jul 20, 2015
  10. Eric SunshineJul 20, 2015
  11. Karthik NayakJul 20, 2015
  12. Junio C HamanoJul 20, 2015
  13. Karthik NayakJul 21, 2015
  14. Matthieu MoyJul 22, 2015
  15. Karthik NayakJul 23, 2015
  16. 2/9 ref-filter: add option to filter only tagsKarthik Nayak, Jul 18, 2015
  17. 3/9 ref-filter: support printing N lines from tag annotationKarthik Nayak, Jul 18, 2015
  18. Eric SunshineJul 20, 2015
  19. Karthik NayakJul 20, 2015
  20. 4/9 ref-filter: add support to sort by versionKarthik Nayak, Jul 18, 2015
  21. Eric SunshineJul 20, 2015
  22. Karthik NayakJul 20, 2015
  23. 5/9 ref-filter: add option to match literal patternKarthik Nayak, Jul 18, 2015
  24. Eric SunshineJul 20, 2015
  25. Christian CouderJul 20, 2015
  26. Eric SunshineJul 20, 2015
  27. Karthik NayakJul 21, 2015
  28. Matthieu MoyJul 22, 2015
  29. 6/9 tag.c: use 'ref-filter' data structuresKarthik Nayak, Jul 18, 2015
  30. 7/9 tag.c: use 'ref-filter' APIsKarthik Nayak, Jul 18, 2015
  31. 8/9 tag.c: implement '--format' optionKarthik Nayak, Jul 18, 2015
  32. Matthieu MoyJul 22, 2015
  33. 9/9 tag.c: implement '--merged' and '--no-merged' optionsKarthik Nayak, Jul 18, 2015
  34. Christian CouderJul 19, 2015
  35. Karthik NayakJul 21, 2015
  36. Matthieu MoyJul 22, 2015
  37. Karthik NayakJul 23, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.