Re: [PATCH] tag: support mixing --sort=<spec> and -n

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Sep 5, 2015 at 10:52 AM, Rudy Matela <rudy@xxxxxxxxxxxxx> wrote:
>
> Allow -n and --sort=version:refname to be used together
> instead of failing with:
>
>   fatal: --sort and -n are incompatible
>
> Signed-off-by: Rudy Matela <rudy@xxxxxxxxxxxxx>

Nice! I've been wondering about this one for a while. Especially since
implementing tag.sort configuration which made -n not work at all.

Note that it may be worth rebasing this on top of Karthik's part tag
to use ref-filter series, since I think there will be plenty of merge
conflicts there...

I also suggest adding some tests for this, as sort and -n didn't even
have a test before, but now we could add a test that shows it works.

> ---
>  builtin/tag.c | 64 ++++++++++++++++++++++++++++++++---------------------------
>  1 file changed, 35 insertions(+), 29 deletions(-)
>
> diff --git a/builtin/tag.c b/builtin/tag.c
> index cccca99..cdcb373 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -176,13 +176,19 @@ static enum contains_result contains(struct commit *candidate,
>         return contains_test(candidate, want);
>  }
>
> -static void show_tag_lines(const struct object_id *oid, int lines)
> +static char *get_tag_lines(const struct object_id *oid, int lines)
>  {
>         int i;
>         unsigned long size;
>         enum object_type type;
>         char *buf, *sp, *eol;
>         size_t len;
> +       struct strbuf sb;
> +
> +       if (!lines)
> +               return NULL;
> +
> +       strbuf_init(&sb,0);
>
>         buf = read_sha1_file(oid->hash, &type, &size);
>         if (!buf)
> @@ -203,20 +209,21 @@ static void show_tag_lines(const struct object_id *oid, int lines)
>                 size = parse_signature(buf, size);
>         for (i = 0, sp += 2; i < lines && sp < buf + size; i++) {
>                 if (i)
> -                       printf("\n    ");
> +                       strbuf_addstr(&sb,"\n    ");
>                 eol = memchr(sp, '\n', size - (sp - buf));
>                 len = eol ? eol - sp : size - (sp - buf);
> -               fwrite(sp, len, 1, stdout);
> +               strbuf_add(&sb, sp, len);
>                 if (!eol)
>                         break;
>                 sp = eol + 1;
>         }
>  free_return:
>         free(buf);
> +       return strbuf_detach(&sb, NULL);
>  }
>
> -static int show_reference(const char *refname, const struct object_id *oid,
> -                         int flag, void *cb_data)
> +static int get_reference_and_tag_lines(const char *refname, const struct object_id *oid,
> +                                      int flag, void *cb_data)
>  {
>         struct tag_filter *filter = cb_data;
>
> @@ -234,16 +241,8 @@ static int show_reference(const char *refname, const struct object_id *oid,
>                 if (points_at.nr && !match_points_at(refname, oid->hash))
>                         return 0;
>
> -               if (!filter->lines) {
> -                       if (filter->sort)
> -                               string_list_append(&filter->tags, refname);
> -                       else
> -                               printf("%s\n", refname);
> -                       return 0;
> -               }
> -               printf("%-15s ", refname);
> -               show_tag_lines(oid, filter->lines);
> -               putchar('\n');
> +               string_list_append(&filter->tags, refname)->util =
> +                       get_tag_lines(oid, filter->lines);
>         }
>
>         return 0;
> @@ -260,6 +259,7 @@ static int list_tags(const char **patterns, int lines,
>                      struct commit_list *with_commit, int sort)
>  {
>         struct tag_filter filter;
> +       int i;
>
>         filter.patterns = patterns;
>         filter.lines = lines;
> @@ -268,20 +268,28 @@ static int list_tags(const char **patterns, int lines,
>         memset(&filter.tags, 0, sizeof(filter.tags));
>         filter.tags.strdup_strings = 1;
>
> -       for_each_tag_ref(show_reference, (void *)&filter);
> -       if (sort) {
> -               int i;
> -               if ((sort & SORT_MASK) == VERCMP_SORT)
> -                       qsort(filter.tags.items, filter.tags.nr,
> -                             sizeof(struct string_list_item), sort_by_version);
> -               if (sort & REVERSE_SORT)
> -                       for (i = filter.tags.nr - 1; i >= 0; i--)
> +       for_each_tag_ref(get_reference_and_tag_lines, (void *)&filter);
> +       if ((sort & SORT_MASK) == VERCMP_SORT)
> +               qsort(filter.tags.items, filter.tags.nr,
> +                     sizeof(struct string_list_item), sort_by_version);

Nice. So we store the items and sort  by the lines.

> +       if (sort & REVERSE_SORT)
> +               for (i = filter.tags.nr - 1; i >= 0; i--)
> +                       if (lines)
> +                               printf("%-15s %s\n",
> +                                       filter.tags.items[i].string,
> +                                       (char*)filter.tags.items[i].util);
> +                       else
>                                 printf("%s\n", filter.tags.items[i].string);
> -               else
> -                       for (i = 0; i < filter.tags.nr; i++)
> +       else
> +               for (i = 0; i < filter.tags.nr; i++)
> +                       if (lines)
> +                               printf("%-15s %s\n",
> +                                       filter.tags.items[i].string,
> +                                       (char*)filter.tags.items[i].util);

I see we print them here (or above depending on whether we reverse sort or not..

Nice! I would maybe suggest if we can rename util to something else so
it is more clear? Maybe I am not understanding why it has to be named
such.

> +                       else
>                                 printf("%s\n", filter.tags.items[i].string);
> -               string_list_clear(&filter.tags, 0);
> -       }
> +       string_list_clear(&filter.tags, 1);
> +
>         return 0;
>  }
>
> @@ -665,8 +673,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>                         copts.padding = 2;
>                         run_column_filter(colopts, &copts);
>                 }
> -               if (lines != -1 && tag_sort)
> -                       die(_("--sort and -n are incompatible"));
>                 ret = list_tags(argv, lines == -1 ? 0 : lines, with_commit, tag_sort);
>                 if (column_active(colopts))
>                         stop_column_filter();
> --
> 2.5.0
>
> --
> 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
--
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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]