{"thread":{"id":"40285","subject":"[PATCH] tag: support mixing --sort= and -n","startedAt":"2015-09-05T17:52:02Z","lastAt":"2015-09-06T06:25:05Z","messageCount":5,"participants":["Rudy Matela","Jacob Keller","Karthik Nayak","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"269501","messageId":"20150905175202.GC7050@zero.home","threadId":"40285","inReplyTo":null,"subject":"[PATCH] tag: support mixing --sort= and -n","fromName":"Rudy Matela","fromEmail":"rudy@matela.com.br","sentAt":"2015-09-05T17:52:02Z","receivedAt":"2015-09-05T17:52:02Z","isPatch":true,"sender":{"key":"rudy@matela.com.br","avatar":null},"body":"\nAllow -n and --sort=version:refname to be used together\ninstead of failing with:\n\n fatal: --sort and -n are incompatible\n\nSigned-off-by: Rudy Matela \n---\n builtin/tag.c | 64 ++++++++++++++++++++++++++++++++---------------------------\n 1 file changed, 35 insertions(+), 29 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex cccca99..cdcb373 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -176,13 +176,19 @@ static enum contains_result contains(struct commit *candidate,\n \treturn contains_test(candidate, want);\n }\n \n-static void show_tag_lines(const struct object_id *oid, int lines)\n+static char *get_tag_lines(const struct object_id *oid, int lines)\n {\n \tint i;\n \tunsigned long size;\n \tenum object_type type;\n \tchar *buf, *sp, *eol;\n \tsize_t len;\n+\tstruct strbuf sb;\n+\n+\tif (!lines)\n+\t\treturn NULL;\n+\n+\tstrbuf_init(&sb,0);\n \n \tbuf = read_sha1_file(oid->hash, &type, &size);\n \tif (!buf)\n@@ -203,20 +209,21 @@ static void show_tag_lines(const struct object_id *oid, int lines)\n \t\tsize = parse_signature(buf, size);\n \tfor (i = 0, sp += 2; i < lines && sp < buf + size; i++) {\n \t\tif (i)\n-\t\t\tprintf(\"\\n \");\n+\t\t\tstrbuf_addstr(&sb,\"\\n \");\n \t\teol = memchr(sp, '\\n', size - (sp - buf));\n \t\tlen = eol ? eol - sp : size - (sp - buf);\n-\t\tfwrite(sp, len, 1, stdout);\n+\t\tstrbuf_add(&sb, sp, len);\n \t\tif (!eol)\n \t\t\tbreak;\n \t\tsp = eol + 1;\n \t}\n free_return:\n \tfree(buf);\n+\treturn strbuf_detach(&sb, NULL);\n }\n \n-static int show_reference(const char *refname, const struct object_id *oid,\n-\t\t\t int flag, void *cb_data)\n+static int get_reference_and_tag_lines(const char *refname, const struct object_id *oid,\n+\t\t\t\t int flag, void *cb_data)\n {\n \tstruct tag_filter *filter = cb_data;\n \n@@ -234,16 +241,8 @@ static int show_reference(const char *refname, const struct object_id *oid,\n \t\tif (points_at.nr && !match_points_at(refname, oid->hash))\n \t\t\treturn 0;\n \n-\t\tif (!filter->lines) {\n-\t\t\tif (filter->sort)\n-\t\t\t\tstring_list_append(&filter->tags, refname);\n-\t\t\telse\n-\t\t\t\tprintf(\"%s\\n\", refname);\n-\t\t\treturn 0;\n-\t\t}\n-\t\tprintf(\"%-15s \", refname);\n-\t\tshow_tag_lines(oid, filter->lines);\n-\t\tputchar('\\n');\n+\t\tstring_list_append(&filter->tags, refname)->util =\n+\t\t\tget_tag_lines(oid, filter->lines);\n \t}\n \n \treturn 0;\n@@ -260,6 +259,7 @@ static int list_tags(const char **patterns, int lines,\n \t\t struct commit_list *with_commit, int sort)\n {\n \tstruct tag_filter filter;\n+\tint i;\n \n \tfilter.patterns = patterns;\n \tfilter.lines = lines;\n@@ -268,20 +268,28 @@ static int list_tags(const char **patterns, int lines,\n \tmemset(&filter.tags, 0, sizeof(filter.tags));\n \tfilter.tags.strdup_strings = 1;\n \n-\tfor_each_tag_ref(show_reference, (void *)&filter);\n-\tif (sort) {\n-\t\tint i;\n-\t\tif ((sort & SORT_MASK) == VERCMP_SORT)\n-\t\t\tqsort(filter.tags.items, filter.tags.nr,\n-\t\t\t sizeof(struct string_list_item), sort_by_version);\n-\t\tif (sort & REVERSE_SORT)\n-\t\t\tfor (i = filter.tags.nr - 1; i >= 0; i--)\n+\tfor_each_tag_ref(get_reference_and_tag_lines, (void *)&filter);\n+\tif ((sort & SORT_MASK) == VERCMP_SORT)\n+\t\tqsort(filter.tags.items, filter.tags.nr,\n+\t\t sizeof(struct string_list_item), sort_by_version);\n+\tif (sort & REVERSE_SORT)\n+\t\tfor (i = filter.tags.nr - 1; i >= 0; i--)\n+\t\t\tif (lines)\n+\t\t\t\tprintf(\"%-15s %s\\n\",\n+\t\t\t\t\tfilter.tags.items[i].string,\n+\t\t\t\t\t(char*)filter.tags.items[i].util);\n+\t\t\telse\n \t\t\t\tprintf(\"%s\\n\", filter.tags.items[i].string);\n-\t\telse\n-\t\t\tfor (i = 0; i < filter.tags.nr; i++)\n+\telse\n+\t\tfor (i = 0; i < filter.tags.nr; i++)\n+\t\t\tif (lines)\n+\t\t\t\tprintf(\"%-15s %s\\n\",\n+\t\t\t\t\tfilter.tags.items[i].string,\n+\t\t\t\t\t(char*)filter.tags.items[i].util);\n+\t\t\telse\n \t\t\t\tprintf(\"%s\\n\", filter.tags.items[i].string);\n-\t\tstring_list_clear(&filter.tags, 0);\n-\t}\n+\tstring_list_clear(&filter.tags, 1);\n+\n \treturn 0;\n }\n \n@@ -665,8 +673,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\t\tcopts.padding = 2;\n \t\t\trun_column_filter(colopts, &copts);\n \t\t}\n-\t\tif (lines != -1 && tag_sort)\n-\t\t\tdie(_(\"--sort and -n are incompatible\"));\n \t\tret = list_tags(argv, lines == -1 ? 0 : lines, with_commit, tag_sort);\n \t\tif (column_active(colopts))\n \t\t\tstop_column_filter();\n-- \n2.5.0\n"},{"id":"269521","messageId":"CA+P7+xo=4mC0fKkbtL+fkMXGxdUNcdUGUYHdaJ58MF9jVUUziw@mail.gmail.com","threadId":"40285","inReplyTo":"20150905175202.GC7050@zero.home","subject":"Re: [PATCH] tag: support mixing --sort= and -n","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-09-05T22:25:36Z","receivedAt":"2015-09-05T22:25:36Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Sat, Sep 5, 2015 at 10:52 AM, Rudy Matela wrote:\n>\n> Allow -n and --sort=version:refname to be used together\n> instead of failing with:\n>\n> fatal: --sort and -n are incompatible\n>\n> Signed-off-by: Rudy Matela \n\nNice! I've been wondering about this one for a while. Especially since\nimplementing tag.sort configuration which made -n not work at all.\n\nNote that it may be worth rebasing this on top of Karthik's part tag\nto use ref-filter series, since I think there will be plenty of merge\nconflicts there...\n\nI also suggest adding some tests for this, as sort and -n didn't even\nhave a test before, but now we could add a test that shows it works.\n\n> ---\n> builtin/tag.c | 64 ++++++++++++++++++++++++++++++++---------------------------\n> 1 file changed, 35 insertions(+), 29 deletions(-)\n>\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index cccca99..cdcb373 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -176,13 +176,19 @@ static enum contains_result contains(struct commit *candidate,\n> return contains_test(candidate, want);\n> }\n>\n> -static void show_tag_lines(const struct object_id *oid, int lines)\n> +static char *get_tag_lines(const struct object_id *oid, int lines)\n> {\n> int i;\n> unsigned long size;\n> enum object_type type;\n> char *buf, *sp, *eol;\n> size_t len;\n> + struct strbuf sb;\n> +\n> + if (!lines)\n> + return NULL;\n> +\n> + strbuf_init(&sb,0);\n>\n> buf = read_sha1_file(oid->hash, &type, &size);\n> if (!buf)\n> @@ -203,20 +209,21 @@ static void show_tag_lines(const struct object_id *oid, int lines)\n> size = parse_signature(buf, size);\n> for (i = 0, sp += 2; i < lines && sp < buf + size; i++) {\n> if (i)\n> - printf(\"\\n \");\n> + strbuf_addstr(&sb,\"\\n \");\n> eol = memchr(sp, '\\n', size - (sp - buf));\n> len = eol ? eol - sp : size - (sp - buf);\n> - fwrite(sp, len, 1, stdout);\n> + strbuf_add(&sb, sp, len);\n> if (!eol)\n> break;\n> sp = eol + 1;\n> }\n> free_return:\n> free(buf);\n> + return strbuf_detach(&sb, NULL);\n> }\n>\n> -static int show_reference(const char *refname, const struct object_id *oid,\n> - int flag, void *cb_data)\n> +static int get_reference_and_tag_lines(const char *refname, const struct object_id *oid,\n> + int flag, void *cb_data)\n> {\n> struct tag_filter *filter = cb_data;\n>\n> @@ -234,16 +241,8 @@ static int show_reference(const char *refname, const struct object_id *oid,\n> if (points_at.nr && !match_points_at(refname, oid->hash))\n> return 0;\n>\n> - if (!filter->lines) {\n> - if (filter->sort)\n> - string_list_append(&filter->tags, refname);\n> - else\n> - printf(\"%s\\n\", refname);\n> - return 0;\n> - }\n> - printf(\"%-15s \", refname);\n> - show_tag_lines(oid, filter->lines);\n> - putchar('\\n');\n> + string_list_append(&filter->tags, refname)->util =\n> + get_tag_lines(oid, filter->lines);\n> }\n>\n> return 0;\n> @@ -260,6 +259,7 @@ static int list_tags(const char **patterns, int lines,\n> struct commit_list *with_commit, int sort)\n> {\n> struct tag_filter filter;\n> + int i;\n>\n> filter.patterns = patterns;\n> filter.lines = lines;\n> @@ -268,20 +268,28 @@ static int list_tags(const char **patterns, int lines,\n> memset(&filter.tags, 0, sizeof(filter.tags));\n> filter.tags.strdup_strings = 1;\n>\n> - for_each_tag_ref(show_reference, (void *)&filter);\n> - if (sort) {\n> - int i;\n> - if ((sort & SORT_MASK) == VERCMP_SORT)\n> - qsort(filter.tags.items, filter.tags.nr,\n> - sizeof(struct string_list_item), sort_by_version);\n> - if (sort & REVERSE_SORT)\n> - for (i = filter.tags.nr - 1; i >= 0; i--)\n> + for_each_tag_ref(get_reference_and_tag_lines, (void *)&filter);\n> + if ((sort & SORT_MASK) == VERCMP_SORT)\n> + qsort(filter.tags.items, filter.tags.nr,\n> + sizeof(struct string_list_item), sort_by_version);\n\nNice. So we store the items and sort by the lines.\n\n> + if (sort & REVERSE_SORT)\n> + for (i = filter.tags.nr - 1; i >= 0; i--)\n> + if (lines)\n> + printf(\"%-15s %s\\n\",\n> + filter.tags.items[i].string,\n> + (char*)filter.tags.items[i].util);\n> + else\n> printf(\"%s\\n\", filter.tags.items[i].string);\n> - else\n> - for (i = 0; i < filter.tags.nr; i++)\n> + else\n> + for (i = 0; i < filter.tags.nr; i++)\n> + if (lines)\n> + printf(\"%-15s %s\\n\",\n> + filter.tags.items[i].string,\n> + (char*)filter.tags.items[i].util);\n\nI see we print them here (or above depending on whether we reverse sort or not..\n\nNice! I would maybe suggest if we can rename util to something else so\nit is more clear? Maybe I am not understanding why it has to be named\nsuch.\n\n> + else\n> printf(\"%s\\n\", filter.tags.items[i].string);\n> - string_list_clear(&filter.tags, 0);\n> - }\n> + string_list_clear(&filter.tags, 1);\n> +\n> return 0;\n> }\n>\n> @@ -665,8 +673,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n> copts.padding = 2;\n> run_column_filter(colopts, &copts);\n> }\n> - if (lines != -1 && tag_sort)\n> - die(_(\"--sort and -n are incompatible\"));\n> ret = list_tags(argv, lines == -1 ? 0 : lines, with_commit, tag_sort);\n> if (column_active(colopts))\n> stop_column_filter();\n> --\n> 2.5.0\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"269522","messageId":"CAOLa=ZTiG8aMdURDYTHALGfhkg_YWe6bXEmPUySgGBrrQXD+Nw@mail.gmail.com","threadId":"40285","inReplyTo":"CA+P7+xo=4mC0fKkbtL+fkMXGxdUNcdUGUYHdaJ58MF9jVUUziw@mail.gmail.com","subject":"Re: [PATCH] tag: support mixing --sort= and -n","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-09-06T03:52:36Z","receivedAt":"2015-09-06T03:52:36Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sun, Sep 6, 2015 at 3:55 AM, Jacob Keller wrote:\n> On Sat, Sep 5, 2015 at 10:52 AM, Rudy Matela wrote:\n>>\n>> Allow -n and --sort=version:refname to be used together\n>> instead of failing with:\n>>\n>> fatal: --sort and -n are incompatible\n>>\n>> Signed-off-by: Rudy Matela \n>\n> Nice! I've been wondering about this one for a while. Especially since\n> implementing tag.sort configuration which made -n not work at all.\n>\n> Note that it may be worth rebasing this on top of Karthik's part tag\n> to use ref-filter series, since I think there will be plenty of merge\n> conflicts there...\n>\n\nIts already resolved in my series. We use ref-filter's sorting APIs\nthere :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"269523","messageId":"xmqqoahgb1r9.fsf@gitster.mtv.corp.google.com","threadId":"40285","inReplyTo":"CAOLa=ZTiG8aMdURDYTHALGfhkg_YWe6bXEmPUySgGBrrQXD+Nw@mail.gmail.com","subject":"Re: [PATCH] tag: support mixing --sort= and -n","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-06T04:38:50Z","receivedAt":"2015-09-06T04:38:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> On Sun, Sep 6, 2015 at 3:55 AM, Jacob Keller wrote:\n>> On Sat, Sep 5, 2015 at 10:52 AM, Rudy Matela wrote:\n>>>\n>>> Allow -n and --sort=version:refname to be used together\n>>> instead of failing with:\n>>>\n>>> fatal: --sort and -n are incompatible\n>>>\n>>> Signed-off-by: Rudy Matela \n>>\n>> Nice! I've been wondering about this one for a while. Especially since\n>> implementing tag.sort configuration which made -n not work at all.\n>>\n>> Note that it may be worth rebasing this on top of Karthik's part tag\n>> to use ref-filter series, since I think there will be plenty of merge\n>> conflicts there...\n>\n> Its already resolved in my series. We use ref-filter's sorting APIs\n> there :)\n\nI guess Jacob needs to rebase that tag.sort thing on top of yours,\nthen ;-)?\n"},{"id":"269525","messageId":"CA+P7+xqTT2soejXEmpaqfLTF3twWERTLZY6OpW2KdJHS3jxKyQ@mail.gmail.com","threadId":"40285","inReplyTo":"xmqqoahgb1r9.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] tag: support mixing --sort= and -n","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-09-06T06:25:05Z","receivedAt":"2015-09-06T06:25:05Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Sat, Sep 5, 2015 at 9:38 PM, Junio C Hamano wrote:\n> Karthik Nayak writes:\n>\n>> On Sun, Sep 6, 2015 at 3:55 AM, Jacob Keller wrote:\n>>> On Sat, Sep 5, 2015 at 10:52 AM, Rudy Matela wrote:\n>>>>\n>>>> Allow -n and --sort=version:refname to be used together\n>>>> instead of failing with:\n>>>>\n>>>> fatal: --sort and -n are incompatible\n>>>>\n>>>> Signed-off-by: Rudy Matela \n>>>\n>>> Nice! I've been wondering about this one for a while. Especially since\n>>> implementing tag.sort configuration which made -n not work at all.\n>>>\n>>> Note that it may be worth rebasing this on top of Karthik's part tag\n>>> to use ref-filter series, since I think there will be plenty of merge\n>>> conflicts there...\n>>\n>> Its already resolved in my series. We use ref-filter's sorting APIs\n>> there :)\n>\n> I guess Jacob needs to rebase that tag.sort thing on top of yours,\n> then ;-)?\n\nYa.. I wasn't sure if this was resolved, and I ended up deciding to\nwait until after the tag re-write landed to look at that again.\n\nRegards,\nJake\n"}]}