{"thread":{"id":"39886","subject":"[PATCH v3 0/9] Port tag.c to use ref-filter APIs","startedAt":"2015-07-18T19:12:20Z","lastAt":"2015-07-23T16:20:52Z","messageCount":37,"participants":["Karthik Nayak","Christian Couder","Eric Sunshine","Junio C Hamano","Matthieu Moy"],"isPatch":true,"patchVersion":3,"patchTotal":9},"messages":[{"id":"266333","messageId":"1437246749-14423-1-git-send-email-Karthik.188@gmail.com","threadId":"39886","inReplyTo":null,"subject":"[PATCH v3 0/9] Port tag.c to use ref-filter APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-18T19:12:20Z","receivedAt":"2015-07-18T19:12:20Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"This is part of my GSoC project to unify git tag -l, git branch -l,\ngit for-each-ref.\nThis patch series is continued from: Git (next)\nhttps://github.com/git/git/commit/bf5418f49ff0cebc6e5ce04ad1417e1a47c81b61\n\nVersion 2 can be found here:\nhttp://article.gmane.org/gmane.comp.version-control.git/273569\n\nChanges in this version:\n* Grammatical errors were corrected.\n* Added missing documentation and tests for \"%(version:refname)\"\n* Made the alignment option an atom as per Duy's Suggestions\n (http://article.gmane.org/gmane.comp.version-control.git/273569)\n\nDiffStat:\nDocumentation/git-for-each-ref.txt | 2 +\nDocumentation/git-tag.txt | 38 ++++++++---\nbuiltin/for-each-ref.c | 3 +-\nbuiltin/tag.c | 371 +++++++++++++++++++----------------------------------------------------------------------------------------\nref-filter.c | 148 ++++++++++++++++++++++++++++++++++++++-----\nref-filter.h | 15 +++--\nt/t6302-for-each-ref-filter.sh | 26 ++++++++\nt/t7004-tag.sh | 51 ++++++++++++---\n\n\n[PATCH v3 1/9] ref-filter: add option to align atoms to the left\n[PATCH v3 2/9] ref-filter: add option to filter only tags\n[PATCH v3 3/9] ref-filter: support printing N lines from tag\n[PATCH v3 4/9] ref-filter: add support to sort by version\n[PATCH v3 5/9] ref-filter: add option to match literal pattern\n[PATCH v3 6/9] tag.c: use 'ref-filter' data structures\n[PATCH v3 7/9] tag.c: use 'ref-filter' APIs\n[PATCH v3 8/9] tag.c: implement '--format' option\n[PATCH v3 9/9] tag.c: implement '--merged' and '--no-merged' options\n"},{"id":"266334","messageId":"1437246749-14423-2-git-send-email-Karthik.188@gmail.com","threadId":"39886","inReplyTo":"1437246749-14423-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v3 1/9] ref-filter: add option to align atoms to the left","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-18T19:12:21Z","receivedAt":"2015-07-18T19:12:21Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"From: Karthik Nayak \n\nAdd a new atom \"align\" and support %(align:X) where X is a number.\nThis will align the preceeding atom value to the left followed by\nspaces for a total length of X characters. If X is less than the item\nsize, the entire atom value is printed.\n\nHelped-by: Duy Nguyen \nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 39 +++++++++++++++++++++++++++++++++++++--\n ref-filter.h | 1 +\n 2 files changed, 38 insertions(+), 2 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 7561727..b81a08d 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -10,6 +10,8 @@\n #include \"quote.h\"\n #include \"ref-filter.h\"\n #include \"revision.h\"\n+#include \"utf8.h\"\n+#include \"git-compat-util.h\"\n \n typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n \n@@ -53,6 +55,7 @@ static struct {\n \t{ \"flag\" },\n \t{ \"HEAD\" },\n \t{ \"color\" },\n+\t{ \"align\" },\n };\n \n /*\n@@ -620,7 +623,7 @@ static void populate_value(struct ref_array_item *ref)\n \t\tconst char *name = used_atom[i];\n \t\tstruct atom_value *v = &ref->value[i];\n \t\tint deref = 0;\n-\t\tconst char *refname;\n+\t\tconst char *refname = NULL;\n \t\tconst char *formatp;\n \t\tstruct branch *branch = NULL;\n \n@@ -687,6 +690,17 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\telse\n \t\t\t\tv->s = \" \";\n \t\t\tcontinue;\n+\t\t} else if (starts_with(name, \"align:\")) {\n+\t\t\tconst char *valp = NULL;\n+\n+\t\t\tskip_prefix(name, \"align:\", &valp);\n+\t\t\tif (!valp[0])\n+\t\t\t\tdie(_(\"No value given with 'align='\"));\n+\t\t\tstrtoul_ui(valp, 10, &ref->align_value);\n+\t\t\tif (ref->align_value < 1)\n+\t\t\t\tdie(_(\"Value should be greater than zero\"));\n+\t\t\tv->s = \"\";\n+\t\t\tcontinue;\n \t\t} else\n \t\t\tcontinue;\n \n@@ -1254,17 +1268,38 @@ static void emit(const char *cp, const char *ep)\n \t}\n }\n \n+static void assign_formating(struct ref_array_item *ref, int parsed_atom, struct atom_value *v)\n+{\n+\tif (v->s[0] && ref->align_value) {\n+\t\tunsigned int len = 0;\n+\n+\t\tlen = utf8_strwidth(v->s);\n+\t\tif (ref->align_value > len) {\n+\t\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\t\tstrbuf_addstr(&buf, v->s);\n+\t\t\tif (!v->s[0])\n+\t\t\t\tfree((char *)v->s);\n+\t\t\tstrbuf_addchars(&buf, ' ', ref->align_value - len);\n+\t\t\tv->s = strbuf_detach(&buf, NULL);\n+\t\t}\n+\t\tref->align_value = 0;\n+\t}\n+}\n+\n void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)\n {\n \tconst char *cp, *sp, *ep;\n \n \tfor (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {\n \t\tstruct atom_value *atomv;\n+\t\tint parsed_atom;\n \n \t\tep = strchr(sp, ')');\n \t\tif (cp < sp)\n \t\t\temit(cp, sp);\n-\t\tget_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);\n+\t\tparsed_atom = parse_ref_filter_atom(sp + 2, ep);\n+\t\tget_ref_atom_value(info, parsed_atom, &atomv);\n+\t\tassign_formating(info, parsed_atom, atomv);\n \t\tprint_value(atomv, quote_style);\n \t}\n \tif (*cp) {\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 6bf27d8..12ffbc5 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -30,6 +30,7 @@ struct ref_sorting {\n struct ref_array_item {\n \tunsigned char objectname[20];\n \tint flag;\n+\tunsigned int align_value;\n \tconst char *symref;\n \tstruct commit *commit;\n \tstruct atom_value *value;\n-- \n2.4.6\n"},{"id":"266335","messageId":"1437246749-14423-3-git-send-email-Karthik.188@gmail.com","threadId":"39886","inReplyTo":"1437246749-14423-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v3 2/9] ref-filter: add option to filter only tags","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-18T19:12:22Z","receivedAt":"2015-07-18T19:12:22Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"From: Karthik Nayak \n\nAdd an option in 'filter_refs()' to use 'for_each_tag_ref()'\nand filter refs. This type checking is done by adding a\n'FILTER_REFS_TAGS' in 'ref-filter.h'\n\nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 2 ++\n ref-filter.h | 1 +\n 2 files changed, 3 insertions(+)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex b81a08d..771c48d 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1149,6 +1149,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int\n \t\tret = for_each_rawref(ref_filter_handler, &ref_cbdata);\n \telse if (type & FILTER_REFS_ALL)\n \t\tret = for_each_ref(ref_filter_handler, &ref_cbdata);\n+\telse if (type & FILTER_REFS_TAGS)\n+\t\tret = for_each_tag_ref(ref_filter_handler, &ref_cbdata);\n \telse if (type)\n \t\tdie(\"filter_refs: invalid type\");\n \ndiff --git a/ref-filter.h b/ref-filter.h\nindex 12ffbc5..c18781b 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -15,6 +15,7 @@\n \n #define FILTER_REFS_INCLUDE_BROKEN 0x1\n #define FILTER_REFS_ALL 0x2\n+#define FILTER_REFS_TAGS 0x4\n \n struct atom_value {\n \tconst char *s;\n-- \n2.4.6\n"},{"id":"266336","messageId":"1437246749-14423-4-git-send-email-Karthik.188@gmail.com","threadId":"39886","inReplyTo":"1437246749-14423-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v3 3/9] ref-filter: support printing N lines from tag annotation","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-18T19:12:23Z","receivedAt":"2015-07-18T19:12:23Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"From: Karthik Nayak \n\nIn 'tag.c' we can print N lines from the annotation of the tag using\nthe '-n' option. Copy code from 'tag.c' to 'ref-filter' and\nmodify 'ref-filter' to support printing of N lines from the annotation\nof tags.\n\nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n builtin/for-each-ref.c | 2 +-\n builtin/tag.c | 4 ++++\n ref-filter.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++-\n ref-filter.h | 8 ++++++--\n 4 files changed, 58 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 40f343b..e4a4f8a 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -74,7 +74,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \tif (!maxcount || array.nr < maxcount)\n \t\tmaxcount = array.nr;\n \tfor (i = 0; i < maxcount; i++)\n-\t\tshow_ref_array_item(array.items[i], format, quote_style);\n+\t\tshow_ref_array_item(array.items[i], format, quote_style, 0);\n \tref_array_clear(&array);\n \treturn 0;\n }\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 071d001..10b11ce 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -185,6 +185,10 @@ static enum contains_result contains(struct commit *candidate,\n \treturn contains_test(candidate, want);\n }\n \n+/*\n+ * Currently duplicated in ref-filter, will eventually be removed as\n+ * we port tag.c to use ref-filter APIs.\n+ */\n static void show_tag_lines(const struct object_id *oid, int lines)\n {\n \tint i;\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 771c48d..82731ac 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1288,7 +1288,48 @@ static void assign_formating(struct ref_array_item *ref, int parsed_atom, struct\n \t}\n }\n \n-void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)\n+/* Print 'lines' no of lines of a given oid */\n+static void show_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+\n+\tbuf = read_sha1_file(oid->hash, &type, &size);\n+\tif (!buf)\n+\t\tdie_errno(\"unable to read object %s\", oid_to_hex(oid));\n+\tif (type != OBJ_COMMIT && type != OBJ_TAG)\n+\t\tgoto free_return;\n+\tif (!size)\n+\t\tdie(\"an empty %s object %s?\",\n+\t\t typename(type), oid_to_hex(oid));\n+\n+\t/* skip header */\n+\tsp = strstr(buf, \"\\n\\n\");\n+\tif (!sp)\n+\t\tgoto free_return;\n+\n+\t/* only take up to \"lines\" lines, and strip the signature from a tag */\n+\tif (type == OBJ_TAG)\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\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\tif (!eol)\n+\t\t\tbreak;\n+\t\tsp = eol + 1;\n+\t}\n+free_return:\n+\tfree(buf);\n+}\n+\n+void show_ref_array_item(struct ref_array_item *info, const char *format,\n+\t\t\t int quote_style, unsigned int lines)\n {\n \tconst char *cp, *sp, *ep;\n \n@@ -1317,6 +1358,11 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n \t\tresetv.s = color;\n \t\tprint_value(&resetv, quote_style);\n \t}\n+\tif (lines > 0) {\n+\t\tstruct object_id oid;\n+\t\thashcpy(oid.hash, info->objectname);\n+\t\tshow_tag_lines(&oid, lines);\n+\t}\n \tputchar('\\n');\n }\n \ndiff --git a/ref-filter.h b/ref-filter.h\nindex c18781b..7dfdea0 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -56,6 +56,7 @@ struct ref_filter {\n \tstruct commit *merge_commit;\n \n \tunsigned int with_commit_tag_algo : 1;\n+\tunsigned int lines;\n };\n \n struct ref_filter_cbdata {\n@@ -87,8 +88,11 @@ int parse_ref_filter_atom(const char *atom, const char *ep);\n int verify_ref_format(const char *format);\n /* Sort the given ref_array as per the ref_sorting provided */\n void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);\n-/* Print the ref using the given format and quote_style */\n-void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style);\n+/*\n+ * Print the ref using the given format and quote_style. If lines > 0,\n+ * prints the \"lines\" no of lines of the objected pointed to.\n+ */\n+void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style, unsigned int lines);\n /* Callback function for parsing the sort option */\n int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset);\n /* Default sort option based on refname */\n-- \n2.4.6\n"},{"id":"266337","messageId":"1437246749-14423-5-git-send-email-Karthik.188@gmail.com","threadId":"39886","inReplyTo":"1437246749-14423-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v3 4/9] ref-filter: add support to sort by version","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-18T19:12:24Z","receivedAt":"2015-07-18T19:12:24Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"From: Karthik Nayak \n\nAdd support to sort by version using the \"v:refname\" and\n\"version:refname\" option. This is achieved by using the\n'version_cmp()' function as the comparing function for qsort.\n\nThis option is included to support sorting by versions in `git tag -l`\nwhich will eventaully be ported to use ref-filter APIs.\n\nAdd documentation and tests for the same.\n\nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n Documentation/git-for-each-ref.txt | 2 ++\n ref-filter.c | 32 ++++++++++++++++++++------------\n ref-filter.h | 2 +-\n t/t6302-for-each-ref-filter.sh | 26 ++++++++++++++++++++++++++\n 4 files changed, 49 insertions(+), 13 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex e49d578..cc91275 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -144,6 +144,8 @@ blank line. Finally, the optional GPG signature is `contents:signature`.\n For sorting purposes, fields with numeric values sort in numeric\n order (`objectsize`, `authordate`, `committerdate`, `taggerdate`).\n All other fields are used to sort in their byte-value order.\n+There is also an option to sort by versions, this is done using the\n+field names ('version:refname' or 'v:refname').\n \n In any case, a field name that refers to a field inapplicable to\n the object referred by the ref does not cause an error. It\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 82731ac..85c561e 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -12,6 +12,7 @@\n #include \"revision.h\"\n #include \"utf8.h\"\n #include \"git-compat-util.h\"\n+#include \"version.h\"\n \n typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n \n@@ -1169,18 +1170,22 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n \n \tget_ref_atom_value(a, s->atom, &va);\n \tget_ref_atom_value(b, s->atom, &vb);\n-\tswitch (cmp_type) {\n-\tcase FIELD_STR:\n-\t\tcmp = strcmp(va->s, vb->s);\n-\t\tbreak;\n-\tdefault:\n-\t\tif (va->ul < vb->ul)\n-\t\t\tcmp = -1;\n-\t\telse if (va->ul == vb->ul)\n-\t\t\tcmp = 0;\n-\t\telse\n-\t\t\tcmp = 1;\n-\t\tbreak;\n+\tif (s->version)\n+\t\tcmp = versioncmp(va->s, vb->s);\n+\telse {\n+\t\tswitch (cmp_type) {\n+\t\tcase FIELD_STR:\n+\t\t\tcmp = strcmp(va->s, vb->s);\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tif (va->ul < vb->ul)\n+\t\t\t\tcmp = -1;\n+\t\t\telse if (va->ul == vb->ul)\n+\t\t\t\tcmp = 0;\n+\t\t\telse\n+\t\t\t\tcmp = 1;\n+\t\t\tbreak;\n+\t\t}\n \t}\n \treturn (s->reverse) ? -cmp : cmp;\n }\n@@ -1395,6 +1400,9 @@ int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset)\n \t\ts->reverse = 1;\n \t\targ++;\n \t}\n+\tif (skip_prefix(arg, \"version:\", &arg) ||\n+\t skip_prefix(arg, \"v:\", &arg))\n+\t\ts->version = 1;\n \tlen = strlen(arg);\n \ts->atom = parse_ref_filter_atom(arg, arg+len);\n \treturn 0;\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 7dfdea0..6f1646b 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -25,7 +25,7 @@ struct atom_value {\n struct ref_sorting {\n \tstruct ref_sorting *next;\n \tint atom; /* index into used_atom array (internal) */\n-\tunsigned reverse : 1;\n+\tunsigned reverse : 1, version : 1;\n };\n \n struct ref_array_item {\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 505a360..c31fd2f 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -81,4 +81,30 @@ test_expect_success 'filtering with --contains' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'setup for version sort' '\n+\ttest_commit foo1.3 &&\n+\ttest_commit foo1.6 &&\n+\ttest_commit foo1.10\n+'\n+\n+test_expect_success 'version sort' '\n+\tgit tag -l --sort=version:refname | grep \"foo\" >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfoo1.3\n+\tfoo1.6\n+\tfoo1.10\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'reverse version sort' '\n+\tgit tag -l --sort=-version:refname | grep \"foo\" >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfoo1.10\n+\tfoo1.6\n+\tfoo1.3\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.4.6\n"},{"id":"266338","messageId":"1437246749-14423-6-git-send-email-Karthik.188@gmail.com","threadId":"39886","inReplyTo":"1437246749-14423-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v3 5/9] ref-filter: add option to match literal pattern","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-18T19:12:25Z","receivedAt":"2015-07-18T19:12:25Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"From: Karthik Nayak \n\nSince 'ref-filter' only has an option to match path names add an\noption for plain fnmatch pattern-matching.\n\nThis is to support the pattern matching options which are used in `git\ntag -l` and `git branch -l` where we can match patterns like `git tag\n-l foo*` which would match all tags which has a \"foo*\" pattern.\n\nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n builtin/for-each-ref.c | 1 +\n ref-filter.c | 27 +++++++++++++++++++++++++--\n ref-filter.h | 3 ++-\n 3 files changed, 28 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex e4a4f8a..3ad6a64 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -68,6 +68,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \tgit_config(git_default_config, NULL);\n \n \tfilter.name_patterns = argv;\n+\tfilter.match_as_path = 1;\n \tfilter_refs(&array, &filter, FILTER_REFS_ALL | FILTER_REFS_INCLUDE_BROKEN);\n \tref_array_sort(sorting, &array);\n \ndiff --git a/ref-filter.c b/ref-filter.c\nindex 85c561e..7ff3ded 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -943,9 +943,23 @@ static int commit_contains(struct ref_filter *filter, struct commit *commit)\n \n /*\n * Return 1 if the refname matches one of the patterns, otherwise 0.\n+ * A pattern can be a literal prefix (e.g. a refname \"refs/heads/master\"\n+ * matches a pattern \"refs/heads/m\") or a wildcard (e.g. the same ref\n+ * matches \"refs/heads/m*\", too).\n+ */\n+static int match_pattern(const char **patterns, const char *refname)\n+{\n+\tfor (; *patterns; patterns++)\n+\t\tif (!wildmatch(*patterns, refname, 0, NULL))\n+\t\t\treturn 1;\n+\treturn 0;\n+}\n+\n+/*\n+ * Return 1 if the refname matches one of the patterns, otherwise 0.\n * A pattern can be path prefix (e.g. a refname \"refs/heads/master\"\n * matches a pattern \"refs/heads/\") or a wildcard (e.g. the same ref\n- * matches \"refs/heads/m*\",too).\n+ * matches \"refs/heads/m*\", too).\n */\n static int match_name_as_path(const char **pattern, const char *refname)\n {\n@@ -966,6 +980,15 @@ static int match_name_as_path(const char **pattern, const char *refname)\n \treturn 0;\n }\n \n+static int filter_pattern_match(struct ref_filter *filter, const char *refname)\n+{\n+\tif (!*filter->name_patterns)\n+\t\treturn 1;\n+\tif (filter->match_as_path)\n+\t\treturn match_name_as_path(filter->name_patterns, refname);\n+\treturn match_pattern(filter->name_patterns, refname);\n+}\n+\n /*\n * Given a ref (sha1, refname), check if the ref belongs to the array\n * of sha1s. If the given ref is a tag, check if the given tag points\n@@ -1034,7 +1057,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n \t\treturn 0;\n \t}\n \n-\tif (*filter->name_patterns && !match_name_as_path(filter->name_patterns, refname))\n+\tif (!filter_pattern_match(filter, refname))\n \t\treturn 0;\n \n \tif (filter->points_at.nr && !match_points_at(&filter->points_at, oid->hash, refname))\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 6f1646b..687a077 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -55,7 +55,8 @@ struct ref_filter {\n \t} merge;\n \tstruct commit *merge_commit;\n \n-\tunsigned int with_commit_tag_algo : 1;\n+\tunsigned int with_commit_tag_algo : 1,\n+\t\tmatch_as_path : 1;\n \tunsigned int lines;\n };\n \n-- \n2.4.6\n"},{"id":"266339","messageId":"1437246749-14423-7-git-send-email-Karthik.188@gmail.com","threadId":"39886","inReplyTo":"1437246749-14423-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v3 6/9] tag.c: use 'ref-filter' data structures","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-18T19:12:26Z","receivedAt":"2015-07-18T19:12:26Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"From: Karthik Nayak \n\nMake 'tag.c' use 'ref-filter' data structures and make changes to\nsupport the new data structures. This is a part of the process\nof porting 'tag.c' to use 'ref-filter' APIs.\n\nThis is a temporary step before porting 'tag.c' to use 'ref-filter'\ncompletely. As this is a temporary step, most of the code\nintroduced here will be removed when 'tag.c' is ported over to use\n'ref-filter' APIs\n\nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n builtin/tag.c | 106 +++++++++++++++++++++++++++++++---------------------------\n 1 file changed, 57 insertions(+), 49 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 10b11ce..dadc686 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -17,6 +17,7 @@\n #include \"gpg-interface.h\"\n #include \"sha1-array.h\"\n #include \"column.h\"\n+#include \"ref-filter.h\"\n \n static const char * const git_tag_usage[] = {\n \tN_(\"git tag [-a | -s | -u ] [-f] [-m | -F ] []\"),\n@@ -34,15 +35,6 @@ static const char * const git_tag_usage[] = {\n \n static int tag_sort;\n \n-struct tag_filter {\n-\tconst char **patterns;\n-\tint lines;\n-\tint sort;\n-\tstruct string_list tags;\n-\tstruct commit_list *with_commit;\n-};\n-\n-static struct sha1_array points_at;\n static unsigned int colopts;\n \n static int match_pattern(const char **patterns, const char *ref)\n@@ -61,19 +53,20 @@ static int match_pattern(const char **patterns, const char *ref)\n * removed as we port tag.c to use the ref-filter APIs.\n */\n static const unsigned char *match_points_at(const char *refname,\n-\t\t\t\t\t const unsigned char *sha1)\n+\t\t\t\t\t const unsigned char *sha1,\n+\t\t\t\t\t struct sha1_array *points_at)\n {\n \tconst unsigned char *tagged_sha1 = NULL;\n \tstruct object *obj;\n \n-\tif (sha1_array_lookup(&points_at, sha1) >= 0)\n+\tif (sha1_array_lookup(points_at, sha1) >= 0)\n \t\treturn sha1;\n \tobj = parse_object(sha1);\n \tif (!obj)\n \t\tdie(_(\"malformed object at '%s'\"), refname);\n \tif (obj->type == OBJ_TAG)\n \t\ttagged_sha1 = ((struct tag *)obj)->tagged->sha1;\n-\tif (tagged_sha1 && sha1_array_lookup(&points_at, tagged_sha1) >= 0)\n+\tif (tagged_sha1 && sha1_array_lookup(points_at, tagged_sha1) >= 0)\n \t\treturn tagged_sha1;\n \treturn NULL;\n }\n@@ -228,12 +221,24 @@ free_return:\n \tfree(buf);\n }\n \n+static void ref_array_append(struct ref_array *array, const char *refname)\n+{\n+\tsize_t len = strlen(refname);\n+\tstruct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);\n+\tmemcpy(ref->refname, refname, len);\n+\tref->refname[len] = '\\0';\n+\tREALLOC_ARRAY(array->items, array->nr + 1);\n+\tarray->items[array->nr++] = ref;\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 {\n-\tstruct tag_filter *filter = cb_data;\n+\tstruct ref_filter_cbdata *data = cb_data;\n+\tstruct ref_array *array = data->array;\n+\tstruct ref_filter *filter = data->filter;\n \n-\tif (match_pattern(filter->patterns, refname)) {\n+\tif (match_pattern(filter->name_patterns, refname)) {\n \t\tif (filter->with_commit) {\n \t\t\tstruct commit *commit;\n \n@@ -244,12 +249,12 @@ static int show_reference(const char *refname, const struct object_id *oid,\n \t\t\t\treturn 0;\n \t\t}\n \n-\t\tif (points_at.nr && !match_points_at(refname, oid->hash))\n+\t\tif (filter->points_at.nr && !match_points_at(refname, oid->hash, &filter->points_at))\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\tif (tag_sort)\n+\t\t\t\tref_array_append(array, refname);\n \t\t\telse\n \t\t\t\tprintf(\"%s\\n\", refname);\n \t\t\treturn 0;\n@@ -264,36 +269,36 @@ static int show_reference(const char *refname, const struct object_id *oid,\n \n static int sort_by_version(const void *a_, const void *b_)\n {\n-\tconst struct string_list_item *a = a_;\n-\tconst struct string_list_item *b = b_;\n-\treturn versioncmp(a->string, b->string);\n+\tconst struct ref_array_item *a = *((struct ref_array_item **)a_);\n+\tconst struct ref_array_item *b = *((struct ref_array_item **)b_);\n+\treturn versioncmp(a->refname, b->refname);\n }\n \n-static int list_tags(const char **patterns, int lines,\n-\t\t struct commit_list *with_commit, int sort)\n+static int list_tags(struct ref_filter *filter, int sort)\n {\n-\tstruct tag_filter filter;\n+\tstruct ref_array array;\n+\tstruct ref_filter_cbdata data;\n+\n+\tmemset(&array, 0, sizeof(array));\n+\tdata.array = &array;\n+\tdata.filter = filter;\n \n-\tfilter.patterns = patterns;\n-\tfilter.lines = lines;\n-\tfilter.sort = sort;\n-\tfilter.with_commit = with_commit;\n-\tmemset(&filter.tags, 0, sizeof(filter.tags));\n-\tfilter.tags.strdup_strings = 1;\n+\tif (filter->lines == -1)\n+\t\tfilter->lines = 0;\n \n-\tfor_each_tag_ref(show_reference, (void *)&filter);\n+\tfor_each_tag_ref(show_reference, &data);\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\t\tqsort(array.items, array.nr,\n+\t\t\t sizeof(struct ref_array_item *), sort_by_version);\n \t\tif (sort & REVERSE_SORT)\n-\t\t\tfor (i = filter.tags.nr - 1; i >= 0; i--)\n-\t\t\t\tprintf(\"%s\\n\", filter.tags.items[i].string);\n+\t\t\tfor (i = array.nr - 1; i >= 0; i--)\n+\t\t\t\tprintf(\"%s\\n\", array.items[i]->refname);\n \t\telse\n-\t\t\tfor (i = 0; i < filter.tags.nr; i++)\n-\t\t\t\tprintf(\"%s\\n\", filter.tags.items[i].string);\n-\t\tstring_list_clear(&filter.tags, 0);\n+\t\t\tfor (i = 0; i < array.nr; i++)\n+\t\t\t\tprintf(\"%s\\n\", array.items[i]->refname);\n+\t\tref_array_clear(&array);\n \t}\n \treturn 0;\n }\n@@ -574,16 +579,16 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tconst char *object_ref, *tag;\n \tstruct create_tag_options opt;\n \tchar *cleanup_arg = NULL;\n-\tint annotate = 0, force = 0, lines = -1;\n+\tint annotate = 0, force = 0;\n \tint cmdmode = 0;\n \tconst char *msgfile = NULL, *keyid = NULL;\n \tstruct msg_arg msg = { 0, STRBUF_INIT };\n-\tstruct commit_list *with_commit = NULL;\n \tstruct ref_transaction *transaction;\n \tstruct strbuf err = STRBUF_INIT;\n+\tstruct ref_filter filter;\n \tstruct option options[] = {\n \t\tOPT_CMDMODE('l', \"list\", &cmdmode, N_(\"list tag names\"), 'l'),\n-\t\t{ OPTION_INTEGER, 'n', NULL, &lines, N_(\"n\"),\n+\t\t{ OPTION_INTEGER, 'n', NULL, &filter.lines, N_(\"n\"),\n \t\t\t\tN_(\"print lines of each tag message\"),\n \t\t\t\tPARSE_OPT_OPTARG, NULL, 1 },\n \t\tOPT_CMDMODE('d', \"delete\", &cmdmode, N_(\"delete tags\"), 'd'),\n@@ -604,14 +609,14 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \n \t\tOPT_GROUP(N_(\"Tag listing options\")),\n \t\tOPT_COLUMN(0, \"column\", &colopts, N_(\"show tag list in columns\")),\n-\t\tOPT_CONTAINS(&with_commit, N_(\"print only tags that contain the commit\")),\n-\t\tOPT_WITH(&with_commit, N_(\"print only tags that contain the commit\")),\n+\t\tOPT_CONTAINS(&filter.with_commit, N_(\"print only tags that contain the commit\")),\n+\t\tOPT_WITH(&filter.with_commit, N_(\"print only tags that contain the commit\")),\n \t\t{\n \t\t\tOPTION_CALLBACK, 0, \"sort\", &tag_sort, N_(\"type\"), N_(\"sort tags\"),\n \t\t\tPARSE_OPT_NONEG, parse_opt_sort\n \t\t},\n \t\t{\n-\t\t\tOPTION_CALLBACK, 0, \"points-at\", &points_at, N_(\"object\"),\n+\t\t\tOPTION_CALLBACK, 0, \"points-at\", &filter.points_at, N_(\"object\"),\n \t\t\tN_(\"print only tags of the object\"), 0, parse_opt_object_name\n \t\t},\n \t\tOPT_END()\n@@ -620,6 +625,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tgit_config(git_tag_config, NULL);\n \n \tmemset(&opt, 0, sizeof(opt));\n+\tmemset(&filter, 0, sizeof(filter));\n+\tfilter.lines = -1;\n \n \targc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);\n \n@@ -636,7 +643,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(git_tag_usage, options);\n \n \tfinalize_colopts(&colopts, -1);\n-\tif (cmdmode == 'l' && lines != -1) {\n+\tif (cmdmode == 'l' && filter.lines != -1) {\n \t\tif (explicitly_enable_column(colopts))\n \t\t\tdie(_(\"--column and -n are incompatible\"));\n \t\tcolopts = 0;\n@@ -649,18 +656,19 @@ 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\tif (filter.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\tfilter.name_patterns = argv;\n+\t\tret = list_tags(&filter, tag_sort);\n \t\tif (column_active(colopts))\n \t\t\tstop_column_filter();\n \t\treturn ret;\n \t}\n-\tif (lines != -1)\n+\tif (filter.lines != -1)\n \t\tdie(_(\"-n option is only allowed with -l.\"));\n-\tif (with_commit)\n+\tif (filter.with_commit)\n \t\tdie(_(\"--contains option is only allowed with -l.\"));\n-\tif (points_at.nr)\n+\tif (filter.points_at.nr)\n \t\tdie(_(\"--points-at option is only allowed with -l.\"));\n \tif (cmdmode == 'd')\n \t\treturn for_each_tag_name(argv, delete_tag);\n-- \n2.4.6\n"},{"id":"266346","messageId":"1437256837-13378-1-git-send-email-Karthik.188@gmail.com","threadId":"39886","inReplyTo":"1437246749-14423-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v3 7/9] tag.c: use 'ref-filter' APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-18T22:00:35Z","receivedAt":"2015-07-18T22:00:35Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"From: Karthik Nayak \n\nMake 'tag.c' use 'ref-filter' APIs for iterating through refs sorting\nand printing of refs. This removes most of the code used in 'tag.c'\nreplacing it with calls to the 'ref-filter' library.\n\nMake 'tag.c' use the 'filter_refs()' function provided by 'ref-filter'\nto filter out tags based on the options set.\n\nFor printing tags we use 'show_ref_array_item()' function provided by\n'ref-filter'.\n\nWe improve the sorting option provided by 'tag.c' by using the sorting\noptions provided by 'ref-filter'. This causes the test 'invalid sort\nparameter on command line' in t7004 to fail, as 'ref-filter' throws an\nerror for all sorting fields which are incorrect. The test is changed\nto reflect the same.\n\nModify documentation for the same.\n\nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n Documentation/git-tag.txt | 16 ++-\n builtin/tag.c | 346 ++++++----------------------------------------\n t/t7004-tag.sh | 8 +-\n 3 files changed, 53 insertions(+), 317 deletions(-)\n\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 4b04c2b..02fb363 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -13,7 +13,7 @@ SYNOPSIS\n \t [ | ]\n 'git tag' -d ...\n 'git tag' [-n[]] -l [--contains ] [--points-at ]\n-\t[--column[=] | --no-column] [...]\n+\t[--column[=] | --no-column] [--sort=] [...]\n 'git tag' -v ...\n \n DESCRIPTION\n@@ -94,14 +94,16 @@ OPTIONS\n \tusing fnmatch(3)). Multiple patterns may be given; if any of\n \tthem matches, the tag is shown.\n \n---sort=::\n-\tSort in a specific order. Supported type is \"refname\"\n-\t(lexicographic order), \"version:refname\" or \"v:refname\" (tag\n+--sort=::\n+\tSort based on the key given. Prefix `-` to sort in\n+\tdescending order of the value. You may use the --sort= option\n+\tmultiple times, in which case the last key becomes the primary\n+\tkey. Also supports \"version:refname\" or \"v:refname\" (tag\n \tnames are treated as versions). The \"version:refname\" sort\n \torder can also be affected by the\n-\t\"versionsort.prereleaseSuffix\" configuration variable. Prepend\n-\t\"-\" to reverse sort order. When this option is not given, the\n-\tsort order defaults to the value configured for the 'tag.sort'\n+\t\"versionsort.prereleaseSuffix\" configuration variable.\n+\tThe keys supported are the same as those in `git for-each-ref`.\n+\tSort order defaults to the value configured for the 'tag.sort'\n \tvariable if it exists, or lexicographic order otherwise. See\n \tlinkgit:git-config[1].\n \ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex dadc686..52a179e 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -28,278 +28,34 @@ static const char * const git_tag_usage[] = {\n \tNULL\n };\n \n-#define STRCMP_SORT 0\t/* must be zero */\n-#define VERCMP_SORT 1\n-#define SORT_MASK 0x7fff\n-#define REVERSE_SORT 0x8000\n-\n-static int tag_sort;\n-\n static unsigned int colopts;\n \n-static int match_pattern(const char **patterns, const char *ref)\n-{\n-\t/* no pattern means match everything */\n-\tif (!*patterns)\n-\t\treturn 1;\n-\tfor (; *patterns; patterns++)\n-\t\tif (!wildmatch(*patterns, ref, 0, NULL))\n-\t\t\treturn 1;\n-\treturn 0;\n-}\n-\n-/*\n- * This is currently duplicated in ref-filter.c, and will eventually be\n- * removed as we port tag.c to use the ref-filter APIs.\n- */\n-static const unsigned char *match_points_at(const char *refname,\n-\t\t\t\t\t const unsigned char *sha1,\n-\t\t\t\t\t struct sha1_array *points_at)\n-{\n-\tconst unsigned char *tagged_sha1 = NULL;\n-\tstruct object *obj;\n-\n-\tif (sha1_array_lookup(points_at, sha1) >= 0)\n-\t\treturn sha1;\n-\tobj = parse_object(sha1);\n-\tif (!obj)\n-\t\tdie(_(\"malformed object at '%s'\"), refname);\n-\tif (obj->type == OBJ_TAG)\n-\t\ttagged_sha1 = ((struct tag *)obj)->tagged->sha1;\n-\tif (tagged_sha1 && sha1_array_lookup(points_at, tagged_sha1) >= 0)\n-\t\treturn tagged_sha1;\n-\treturn NULL;\n-}\n-\n-static int in_commit_list(const struct commit_list *want, struct commit *c)\n-{\n-\tfor (; want; want = want->next)\n-\t\tif (!hashcmp(want->item->object.sha1, c->object.sha1))\n-\t\t\treturn 1;\n-\treturn 0;\n-}\n-\n-/*\n- * The entire code segment for supporting the --contains option has been\n- * copied over to ref-filter.{c,h}. This will be deleted evetually when\n- * we port tag.c to use ref-filter APIs.\n- */\n-enum contains_result {\n-\tCONTAINS_UNKNOWN = -1,\n-\tCONTAINS_NO = 0,\n-\tCONTAINS_YES = 1\n-};\n-\n-/*\n- * Test whether the candidate or one of its parents is contained in the list.\n- * Do not recurse to find out, though, but return -1 if inconclusive.\n- */\n-static enum contains_result contains_test(struct commit *candidate,\n-\t\t\t const struct commit_list *want)\n-{\n-\t/* was it previously marked as containing a want commit? */\n-\tif (candidate->object.flags & TMP_MARK)\n-\t\treturn 1;\n-\t/* or marked as not possibly containing a want commit? */\n-\tif (candidate->object.flags & UNINTERESTING)\n-\t\treturn 0;\n-\t/* or are we it? */\n-\tif (in_commit_list(want, candidate)) {\n-\t\tcandidate->object.flags |= TMP_MARK;\n-\t\treturn 1;\n-\t}\n-\n-\tif (parse_commit(candidate) < 0)\n-\t\treturn 0;\n-\n-\treturn -1;\n-}\n-\n-/*\n- * Mimicking the real stack, this stack lives on the heap, avoiding stack\n- * overflows.\n- *\n- * At each recursion step, the stack items points to the commits whose\n- * ancestors are to be inspected.\n- */\n-struct stack {\n-\tint nr, alloc;\n-\tstruct stack_entry {\n-\t\tstruct commit *commit;\n-\t\tstruct commit_list *parents;\n-\t} *stack;\n-};\n-\n-static void push_to_stack(struct commit *candidate, struct stack *stack)\n-{\n-\tint index = stack->nr++;\n-\tALLOC_GROW(stack->stack, stack->nr, stack->alloc);\n-\tstack->stack[index].commit = candidate;\n-\tstack->stack[index].parents = candidate->parents;\n-}\n-\n-static enum contains_result contains(struct commit *candidate,\n-\t\tconst struct commit_list *want)\n-{\n-\tstruct stack stack = { 0, 0, NULL };\n-\tint result = contains_test(candidate, want);\n-\n-\tif (result != CONTAINS_UNKNOWN)\n-\t\treturn result;\n-\n-\tpush_to_stack(candidate, &stack);\n-\twhile (stack.nr) {\n-\t\tstruct stack_entry *entry = &stack.stack[stack.nr - 1];\n-\t\tstruct commit *commit = entry->commit;\n-\t\tstruct commit_list *parents = entry->parents;\n-\n-\t\tif (!parents) {\n-\t\t\tcommit->object.flags |= UNINTERESTING;\n-\t\t\tstack.nr--;\n-\t\t}\n-\t\t/*\n-\t\t * If we just popped the stack, parents->item has been marked,\n-\t\t * therefore contains_test will return a meaningful 0 or 1.\n-\t\t */\n-\t\telse switch (contains_test(parents->item, want)) {\n-\t\tcase CONTAINS_YES:\n-\t\t\tcommit->object.flags |= TMP_MARK;\n-\t\t\tstack.nr--;\n-\t\t\tbreak;\n-\t\tcase CONTAINS_NO:\n-\t\t\tentry->parents = parents->next;\n-\t\t\tbreak;\n-\t\tcase CONTAINS_UNKNOWN:\n-\t\t\tpush_to_stack(parents->item, &stack);\n-\t\t\tbreak;\n-\t\t}\n-\t}\n-\tfree(stack.stack);\n-\treturn contains_test(candidate, want);\n-}\n-\n-/*\n- * Currently duplicated in ref-filter, will eventually be removed as\n- * we port tag.c to use ref-filter APIs.\n- */\n-static void show_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-\n-\tbuf = read_sha1_file(oid->hash, &type, &size);\n-\tif (!buf)\n-\t\tdie_errno(\"unable to read object %s\", oid_to_hex(oid));\n-\tif (type != OBJ_COMMIT && type != OBJ_TAG)\n-\t\tgoto free_return;\n-\tif (!size)\n-\t\tdie(\"an empty %s object %s?\",\n-\t\t typename(type), oid_to_hex(oid));\n-\n-\t/* skip header */\n-\tsp = strstr(buf, \"\\n\\n\");\n-\tif (!sp)\n-\t\tgoto free_return;\n-\n-\t/* only take up to \"lines\" lines, and strip the signature from a tag */\n-\tif (type == OBJ_TAG)\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\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\tif (!eol)\n-\t\t\tbreak;\n-\t\tsp = eol + 1;\n-\t}\n-free_return:\n-\tfree(buf);\n-}\n-\n-static void ref_array_append(struct ref_array *array, const char *refname)\n-{\n-\tsize_t len = strlen(refname);\n-\tstruct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);\n-\tmemcpy(ref->refname, refname, len);\n-\tref->refname[len] = '\\0';\n-\tREALLOC_ARRAY(array->items, array->nr + 1);\n-\tarray->items[array->nr++] = ref;\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-{\n-\tstruct ref_filter_cbdata *data = cb_data;\n-\tstruct ref_array *array = data->array;\n-\tstruct ref_filter *filter = data->filter;\n-\n-\tif (match_pattern(filter->name_patterns, refname)) {\n-\t\tif (filter->with_commit) {\n-\t\t\tstruct commit *commit;\n-\n-\t\t\tcommit = lookup_commit_reference_gently(oid->hash, 1);\n-\t\t\tif (!commit)\n-\t\t\t\treturn 0;\n-\t\t\tif (!contains(commit, filter->with_commit))\n-\t\t\t\treturn 0;\n-\t\t}\n-\n-\t\tif (filter->points_at.nr && !match_points_at(refname, oid->hash, &filter->points_at))\n-\t\t\treturn 0;\n-\n-\t\tif (!filter->lines) {\n-\t\t\tif (tag_sort)\n-\t\t\t\tref_array_append(array, 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}\n-\n-\treturn 0;\n-}\n-\n-static int sort_by_version(const void *a_, const void *b_)\n-{\n-\tconst struct ref_array_item *a = *((struct ref_array_item **)a_);\n-\tconst struct ref_array_item *b = *((struct ref_array_item **)b_);\n-\treturn versioncmp(a->refname, b->refname);\n-}\n-\n-static int list_tags(struct ref_filter *filter, int sort)\n+static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting)\n {\n \tstruct ref_array array;\n-\tstruct ref_filter_cbdata data;\n+\tchar *format;\n+\tint i;\n \n \tmemset(&array, 0, sizeof(array));\n-\tdata.array = &array;\n-\tdata.filter = filter;\n \n \tif (filter->lines == -1)\n \t\tfilter->lines = 0;\n \n-\tfor_each_tag_ref(show_reference, &data);\n-\tif (sort) {\n-\t\tint i;\n-\t\tif ((sort & SORT_MASK) == VERCMP_SORT)\n-\t\t\tqsort(array.items, array.nr,\n-\t\t\t sizeof(struct ref_array_item *), sort_by_version);\n-\t\tif (sort & REVERSE_SORT)\n-\t\t\tfor (i = array.nr - 1; i >= 0; i--)\n-\t\t\t\tprintf(\"%s\\n\", array.items[i]->refname);\n-\t\telse\n-\t\t\tfor (i = 0; i < array.nr; i++)\n-\t\t\t\tprintf(\"%s\\n\", array.items[i]->refname);\n-\t\tref_array_clear(&array);\n-\t}\n+\tif (filter->lines)\n+\t\tformat = \"%(align:16)%(refname:short)\";\n+\telse\n+\t\tformat = \"%(refname:short)\";\n+\n+\tverify_ref_format(format);\n+\tif (!sorting)\n+\t\tsorting = ref_default_sorting();\n+\tfilter_refs(&array, filter, FILTER_REFS_TAGS);\n+\tref_array_sort(sorting, &array);\n+\n+\tfor (i = 0; i < array.nr; i++)\n+\t\tshow_ref_array_item(array.items[i], format, QUOTE_NONE, filter->lines);\n+\tref_array_clear(&array);\n+\n \treturn 0;\n }\n \n@@ -366,35 +122,26 @@ static const char tag_template_nocleanup[] =\n \t\"Lines starting with '%c' will be kept; you may remove them\"\n \t\" yourself if you want to.\\n\");\n \n-/*\n- * Parse a sort string, and return 0 if parsed successfully. Will return\n- * non-zero when the sort string does not parse into a known type. If var is\n- * given, the error message becomes a warning and includes information about\n- * the configuration value.\n- */\n-static int parse_sort_string(const char *var, const char *arg, int *sort)\n+/* Parse arg given and add it the ref_sorting array */\n+static int parse_sorting_string(const char *arg, struct ref_sorting **sorting_tail)\n {\n-\tint type = 0, flags = 0;\n-\n-\tif (skip_prefix(arg, \"-\", &arg))\n-\t\tflags |= REVERSE_SORT;\n+\tstruct ref_sorting *s;\n+\tint len;\n \n-\tif (skip_prefix(arg, \"version:\", &arg) || skip_prefix(arg, \"v:\", &arg))\n-\t\ttype = VERCMP_SORT;\n-\telse\n-\t\ttype = STRCMP_SORT;\n+\ts = xcalloc(1, sizeof(*s));\n+\ts->next = *sorting_tail;\n+\t*sorting_tail = s;\n \n-\tif (strcmp(arg, \"refname\")) {\n-\t\tif (!var)\n-\t\t\treturn error(_(\"unsupported sort specification '%s'\"), arg);\n-\t\telse {\n-\t\t\twarning(_(\"unsupported sort specification '%s' in variable '%s'\"),\n-\t\t\t\tvar, arg);\n-\t\t\treturn -1;\n-\t\t}\n-\t}\n+\tif (*arg == '-') {\n+\t\ts->reverse = 1;\n+\t\targ++;\n+ \t}\n+\tif (skip_prefix(arg, \"version:\", &arg) ||\n+\t skip_prefix(arg, \"v:\", &arg))\n+\t\ts->version = 1;\n \n-\t*sort = (type | flags);\n+\tlen = strlen(arg);\n+\ts->atom = parse_ref_filter_atom(arg, arg+len);\n \n \treturn 0;\n }\n@@ -402,11 +149,12 @@ static int parse_sort_string(const char *var, const char *arg, int *sort)\n static int git_tag_config(const char *var, const char *value, void *cb)\n {\n \tint status;\n+\tstruct ref_sorting **sorting_tail = (struct ref_sorting **)cb;\n \n \tif (!strcmp(var, \"tag.sort\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n-\t\tparse_sort_string(var, value, &tag_sort);\n+\t\tparse_sorting_string(value, sorting_tail);\n \t\treturn 0;\n \t}\n \n@@ -564,13 +312,6 @@ static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)\n \treturn check_refname_format(sb->buf, 0);\n }\n \n-static int parse_opt_sort(const struct option *opt, const char *arg, int unset)\n-{\n-\tint *sort = opt->value;\n-\n-\treturn parse_sort_string(NULL, arg, sort);\n-}\n-\n int cmd_tag(int argc, const char **argv, const char *prefix)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -586,6 +327,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tstruct ref_transaction *transaction;\n \tstruct strbuf err = STRBUF_INIT;\n \tstruct ref_filter filter;\n+\tstatic struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;\n \tstruct option options[] = {\n \t\tOPT_CMDMODE('l', \"list\", &cmdmode, N_(\"list tag names\"), 'l'),\n \t\t{ OPTION_INTEGER, 'n', NULL, &filter.lines, N_(\"n\"),\n@@ -611,10 +353,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tOPT_COLUMN(0, \"column\", &colopts, N_(\"show tag list in columns\")),\n \t\tOPT_CONTAINS(&filter.with_commit, N_(\"print only tags that contain the commit\")),\n \t\tOPT_WITH(&filter.with_commit, N_(\"print only tags that contain the commit\")),\n-\t\t{\n-\t\t\tOPTION_CALLBACK, 0, \"sort\", &tag_sort, N_(\"type\"), N_(\"sort tags\"),\n-\t\t\tPARSE_OPT_NONEG, parse_opt_sort\n-\t\t},\n+\t\tOPT_CALLBACK(0 , \"sort\", sorting_tail, N_(\"key\"),\n+\t\t\t N_(\"field name to sort on\"), &parse_opt_ref_sorting),\n \t\t{\n \t\t\tOPTION_CALLBACK, 0, \"points-at\", &filter.points_at, N_(\"object\"),\n \t\t\tN_(\"print only tags of the object\"), 0, parse_opt_object_name\n@@ -622,7 +362,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n-\tgit_config(git_tag_config, NULL);\n+\tgit_config(git_tag_config, sorting_tail);\n \n \tmemset(&opt, 0, sizeof(opt));\n \tmemset(&filter, 0, sizeof(filter));\n@@ -648,6 +388,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"--column and -n are incompatible\"));\n \t\tcolopts = 0;\n \t}\n+\tif (!sorting)\n+\t\tsorting = ref_default_sorting();\n \tif (cmdmode == 'l') {\n \t\tint ret;\n \t\tif (column_active(colopts)) {\n@@ -656,10 +398,8 @@ 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 (filter.lines != -1 && tag_sort)\n-\t\t\tdie(_(\"--sort and -n are incompatible\"));\n \t\tfilter.name_patterns = argv;\n-\t\tret = list_tags(&filter, tag_sort);\n+\t\tret = list_tags(&filter, sorting);\n \t\tif (column_active(colopts))\n \t\t\tstop_column_filter();\n \t\treturn ret;\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex d1ff5c9..51a233f 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1450,13 +1450,7 @@ test_expect_success 'invalid sort parameter on command line' '\n \n test_expect_success 'invalid sort parameter in configuratoin' '\n \tgit config tag.sort \"v:notvalid\" &&\n-\tgit tag -l \"foo*\" >actual &&\n-\tcat >expect <<-\\EOF &&\n-\tfoo1.10\n-\tfoo1.3\n-\tfoo1.6\n-\tEOF\n-\ttest_cmp expect actual\n+\ttest_must_fail git tag -l \"foo*\" >actual\n '\n \n test_expect_success 'version sort with prerelease reordering' '\n-- \n2.4.6\n"},{"id":"266347","messageId":"1437256837-13378-2-git-send-email-Karthik.188@gmail.com","threadId":"39886","inReplyTo":"1437256837-13378-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v3 8/9] tag.c: implement '--format' option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-18T22:00:36Z","receivedAt":"2015-07-18T22:00:36Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"From: Karthik Nayak \n\nImplement the '--format' option provided by 'ref-filter'.\nThis lets the user list tags as per desired format similar\nto the implementation in 'git for-each-ref'.\n\nAdd tests and documentation for the same.\n\nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n Documentation/git-tag.txt | 16 +++++++++++++++-\n builtin/tag.c | 11 +++++++----\n t/t7004-tag.sh | 16 ++++++++++++++++\n 3 files changed, 38 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 02fb363..16e396c 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -13,7 +13,8 @@ SYNOPSIS\n \t [ | ]\n 'git tag' -d ...\n 'git tag' [-n[]] -l [--contains ] [--points-at ]\n-\t[--column[=] | --no-column] [--sort=] [...]\n+\t[--column[=] | --no-column] [--sort=] [--format=]\n+\t[...]\n 'git tag' -v ...\n \n DESCRIPTION\n@@ -155,6 +156,19 @@ This option is only applicable when listing tags without annotation lines.\n \tThe object that the new tag will refer to, usually a commit.\n \tDefaults to HEAD.\n \n+::\n+\tA string that interpolates `%(fieldname)` from the\n+\tobject pointed at by a ref being shown. If `fieldname`\n+\tis prefixed with an asterisk (`*`) and the ref points\n+\tat a tag object, the value for the field in the object\n+\ttag refers is used. When unspecified, defaults to\n+\t`%(objectname) SPC %(objecttype) TAB %(refname)`.\n+\tIt also interpolates `%%` to `%`, and `%xx` where `xx`\n+\tare hex digits interpolates to character with hex code\n+\t`xx`; for example `%00` interpolates to `\\0` (NUL),\n+\t`%09` to `\\t` (TAB) and `%0a` to `\\n` (LF).\n+\tThe fields are same as those in `git for-each-ref`.\n+\n \n CONFIGURATION\n -------------\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 52a179e..cae113b 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -30,10 +30,9 @@ static const char * const git_tag_usage[] = {\n \n static unsigned int colopts;\n \n-static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting)\n+static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)\n {\n \tstruct ref_array array;\n-\tchar *format;\n \tint i;\n \n \tmemset(&array, 0, sizeof(array));\n@@ -43,7 +42,7 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting)\n \n \tif (filter->lines)\n \t\tformat = \"%(align:16)%(refname:short)\";\n-\telse\n+\telse if (!format)\n \t\tformat = \"%(refname:short)\";\n \n \tverify_ref_format(format);\n@@ -328,6 +327,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tstruct strbuf err = STRBUF_INIT;\n \tstruct ref_filter filter;\n \tstatic struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;\n+\tconst char *format = NULL;\n \tstruct option options[] = {\n \t\tOPT_CMDMODE('l', \"list\", &cmdmode, N_(\"list tag names\"), 'l'),\n \t\t{ OPTION_INTEGER, 'n', NULL, &filter.lines, N_(\"n\"),\n@@ -359,6 +359,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\t\tOPTION_CALLBACK, 0, \"points-at\", &filter.points_at, N_(\"object\"),\n \t\t\tN_(\"print only tags of the object\"), 0, parse_opt_object_name\n \t\t},\n+\t\tOPT_STRING( 0 , \"format\", &format, N_(\"format\"), N_(\"format to use for the output\")),\n \t\tOPT_END()\n \t};\n \n@@ -398,8 +399,10 @@ 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 (format && (filter.lines != -1))\n+\t\t\tdie(_(\"--format and -n are incompatible\"));\n \t\tfilter.name_patterns = argv;\n-\t\tret = list_tags(&filter, sorting);\n+\t\tret = list_tags(&filter, sorting, format);\n \t\tif (column_active(colopts))\n \t\t\tstop_column_filter();\n \t\treturn ret;\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 51a233f..e8cebb6 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1507,4 +1507,20 @@ EOF\"\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--format cannot be used with -n' '\n+\ttest_must_fail git tag -l -n4 --format=\"%(refname)\"\n+'\n+\n+test_expect_success '--format should list tags as per format given' '\n+\tcat >expect <<-\\EOF &&\n+\tfoo1.10\n+\tfoo1.3\n+\tfoo1.6\n+\tfoo1.6-rc1\n+\tfoo1.6-rc2\n+\tEOF\n+\tgit tag -l --format=\"%(refname)\" \"foo*\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.4.6\n"},{"id":"266348","messageId":"1437256837-13378-3-git-send-email-Karthik.188@gmail.com","threadId":"39886","inReplyTo":"1437256837-13378-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v3 9/9] tag.c: implement '--merged' and '--no-merged' options","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-18T22:00:37Z","receivedAt":"2015-07-18T22:00:37Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"From: Karthik Nayak \n\nUsing 'ref-filter' APIs implement the '--merged' and '--no-merged'\noptions into 'tag.c'. The '--merged' option lets the user to only\nlist tags merged into the named commit. The '--no-merged' option\nlets the user to only list tags not merged into the named commit.\nIf no object is provided it assumes HEAD as the object.\n\nAdd documentation and tests for the same.\n\nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n Documentation/git-tag.txt | 10 +++++++++-\n builtin/tag.c | 6 +++++-\n t/t7004-tag.sh | 27 +++++++++++++++++++++++++++\n 3 files changed, 41 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 16e396c..74ed157 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -14,7 +14,7 @@ SYNOPSIS\n 'git tag' -d ...\n 'git tag' [-n[]] -l [--contains ] [--points-at ]\n \t[--column[=] | --no-column] [--sort=] [--format=]\n-\t[...]\n+\t[(--merged | --no-merged) []] [...]\n 'git tag' -v ...\n \n DESCRIPTION\n@@ -169,6 +169,14 @@ This option is only applicable when listing tags without annotation lines.\n \t`%09` to `\\t` (TAB) and `%0a` to `\\n` (LF).\n \tThe fields are same as those in `git for-each-ref`.\n \n+--merged []::\n+\tOnly list tags whose tips are reachable from the\n+\tspecified commit (HEAD if not specified).\n+\n+--no-merged []::\n+\tOnly list tags whose tips are not reachable from the\n+\tspecified commit (HEAD if not specified).\n+\n \n CONFIGURATION\n -------------\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex cae113b..0fa1d31 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -23,7 +23,7 @@ static const char * const git_tag_usage[] = {\n \tN_(\"git tag [-a | -s | -u ] [-f] [-m | -F ] []\"),\n \tN_(\"git tag -d ...\"),\n \tN_(\"git tag -l [-n[]] [--contains ] [--points-at ]\"\n-\t\t\"\\n\\t\\t[...]\"),\n+\t\t\"\\n\\t\\t[--merged []] [--no-merged []] [...]\"),\n \tN_(\"git tag -v ...\"),\n \tNULL\n };\n@@ -353,6 +353,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tOPT_COLUMN(0, \"column\", &colopts, N_(\"show tag list in columns\")),\n \t\tOPT_CONTAINS(&filter.with_commit, N_(\"print only tags that contain the commit\")),\n \t\tOPT_WITH(&filter.with_commit, N_(\"print only tags that contain the commit\")),\n+\t\tOPT_MERGED(&filter, N_(\"print only tags that are merged\")),\n+\t\tOPT_NO_MERGED(&filter, N_(\"print only tags that are not merged\")),\n \t\tOPT_CALLBACK(0 , \"sort\", sorting_tail, N_(\"key\"),\n \t\t\t N_(\"field name to sort on\"), &parse_opt_ref_sorting),\n \t\t{\n@@ -413,6 +415,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"--contains option is only allowed with -l.\"));\n \tif (filter.points_at.nr)\n \t\tdie(_(\"--points-at option is only allowed with -l.\"));\n+\tif (filter.merge_commit)\n+\t\tdie(_(\"--merged and --no-merged option are only allowed with -l\"));\n \tif (cmdmode == 'd')\n \t\treturn for_each_tag_name(argv, delete_tag);\n \tif (cmdmode == 'v')\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex e8cebb6..873aad3 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1523,4 +1523,31 @@ test_expect_success '--format should list tags as per format given' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'setup --merged test tags' '\n+\tgit tag mergetest-1 HEAD~2 &&\n+\tgit tag mergetest-2 HEAD~1 &&\n+\tgit tag mergetest-3 HEAD\n+'\n+\n+test_expect_success '--merged cannot be used in non-list mode' '\n+\ttest_must_fail git tag --merged=mergetest-2 foo\n+'\n+\n+test_expect_success '--merged shows merged tags' '\n+\tcat >expect <<-\\EOF &&\n+\tmergetest-1\n+\tmergetest-2\n+\tEOF\n+\tgit tag -l --merged=mergetest-2 mergetest-* >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--no-merged show unmerged tags' '\n+\tcat >expect <<-\\EOF &&\n+\tmergetest-3\n+\tEOF\n+\tgit tag -l --no-merged=mergetest-2 mergetest-* >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.4.6\n"},{"id":"266440","messageId":"CAP8UFD0EXgt7ijdv2cY-HGuqcwjz=Z3-8KV7Xqk=-EBRp-QrGQ@mail.gmail.com","threadId":"39886","inReplyTo":"1437256837-13378-3-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v3 9/9] tag.c: implement '--merged' and '--no-merged' options","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-07-19T19:20:07Z","receivedAt":"2015-07-19T19:20:07Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, Jul 19, 2015 at 12:00 AM, Karthik Nayak wrote:\n> From: Karthik Nayak \n>\n> Using 'ref-filter' APIs implement the '--merged' and '--no-merged'\n> options into 'tag.c'. The '--merged' option lets the user to only\n> list tags merged into the named commit. The '--no-merged' option\n> lets the user to only list tags not merged into the named commit.\n> If no object is provided it assumes HEAD as the object.\n>\n> Add documentation and tests for the same.\n>\n> Mentored-by: Christian Couder \n> Mentored-by: Matthieu Moy \n> Signed-off-by: Karthik Nayak \n> ---\n> Documentation/git-tag.txt | 10 +++++++++-\n> builtin/tag.c | 6 +++++-\n> t/t7004-tag.sh | 27 +++++++++++++++++++++++++++\n> 3 files changed, 41 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\n> index 16e396c..74ed157 100644\n> --- a/Documentation/git-tag.txt\n> +++ b/Documentation/git-tag.txt\n> @@ -14,7 +14,7 @@ SYNOPSIS\n> 'git tag' -d ...\n> 'git tag' [-n[]] -l [--contains ] [--points-at ]\n> [--column[=] | --no-column] [--sort=] [--format=]\n> - [...]\n> + [(--merged | --no-merged) []] [...]\n\nMaybe [--[no-]merged []] instead of [(--merged | --no-merged)\n[]].\n\n> 'git tag' -v ...\n>\n> DESCRIPTION\n> @@ -169,6 +169,14 @@ This option is only applicable when listing tags without annotation lines.\n> `%09` to `\\t` (TAB) and `%0a` to `\\n` (LF).\n> The fields are same as those in `git for-each-ref`.\n>\n> +--merged []::\n> + Only list tags whose tips are reachable from the\n> + specified commit (HEAD if not specified).\n> +\n> +--no-merged []::\n> + Only list tags whose tips are not reachable from the\n> + specified commit (HEAD if not specified).\n\nHere also you could write something like:\n\n+--[no-]merged []::\n+ Only list tags whose tips are reachable, or not reachable\n+ if --no-merged is used, from the specified commit\n+ (HEAD if not specified).\n\n>\n> CONFIGURATION\n> -------------\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index cae113b..0fa1d31 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -23,7 +23,7 @@ static const char * const git_tag_usage[] = {\n> N_(\"git tag [-a | -s | -u ] [-f] [-m | -F ] []\"),\n> N_(\"git tag -d ...\"),\n> N_(\"git tag -l [-n[]] [--contains ] [--points-at ]\"\n> - \"\\n\\t\\t[...]\"),\n> + \"\\n\\t\\t[--merged []] [--no-merged []] [...]\"),\n\n[--[no-]merged []] here too.\n\nThanks,\nChristian.\n"},{"id":"266464","messageId":"CAPig+cRXsmi=UxRr-3rnt919d86jD6uMuTqdDxCComYLXk1TYw@mail.gmail.com","threadId":"39886","inReplyTo":"1437246749-14423-2-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v3 1/9] ref-filter: add option to align atoms to the left","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-07-19T23:49:36Z","receivedAt":"2015-07-19T23:49:36Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak wrote:\n> Add a new atom \"align\" and support %(align:X) where X is a number.\n> This will align the preceeding atom value to the left followed by\n> spaces for a total length of X characters. If X is less than the item\n> size, the entire atom value is printed.\n>\n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 7561727..b81a08d 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -53,6 +55,7 @@ static struct {\n> { \"flag\" },\n> { \"HEAD\" },\n> { \"color\" },\n> + { \"align\" },\n\nNot a new issue, but some compilers (Solaris?) complain about the\ntrailing comma.\n\n> };\n>\n> /*\n> @@ -687,6 +690,17 @@ static void populate_value(struct ref_array_item *ref)\n> else\n> v->s = \" \";\n> continue;\n> + } else if (starts_with(name, \"align:\")) {\n> + const char *valp = NULL;\n> +\n> + skip_prefix(name, \"align:\", &valp);\n> + if (!valp[0])\n> + die(_(\"No value given with 'align='\"));\n\nThe parser expects \"align:\", but the error message talks about\n\"align=\". Also, current trend is to drop capitalization from the error\nmessage.\n\n> + strtoul_ui(valp, 10, &ref->align_value);\n> + if (ref->align_value < 1)\n> + die(_(\"Value should be greater than zero\"));\n\nDrop capitalization. Also, the user seeing this message won't\nnecessarily know to which value this refers. Better would be to\nprovide context (\"'align:' value should be...\"), and even better would\nbe to show the actual value at fault:\n\n die(_(\"value should be greater than zero: align:%u\\n\",\n ref_align_value);\n\nor something.\n\n> + v->s = \"\";\n> + continue;\n> } else\n> continue;\n>\n> @@ -1254,17 +1268,38 @@ static void emit(const char *cp, const char *ep)\n> }\n> }\n>\n> +static void assign_formating(struct ref_array_item *ref, int parsed_atom, struct atom_value *v)\n> +{\n> + if (v->s[0] && ref->align_value) {\n\nMental note: v->s[0] is not NUL ('\\0').\n\nAlso, in this code base, this is typically written *v->s rather than v->s[0].\n\n> + unsigned int len = 0;\n> + len = utf8_strwidth(v->s);\n\nYou initialize 'len' to 0 but then immediately re-assign it.\n\n> + if (ref->align_value > len) {\n> + struct strbuf buf = STRBUF_INIT;\n> + strbuf_addstr(&buf, v->s);\n> + if (!v->s[0])\n> + free((char *)v->s);\n\nWe know from the \"mental note\" above that v->s[0] is not NUL ('\\0'),\nso this 'if' statement can never be true, thus is dead code.\n\n> + strbuf_addchars(&buf, ' ', ref->align_value - len);\n> + v->s = strbuf_detach(&buf, NULL);\n> + }\n> + ref->align_value = 0;\n> + }\n> +}\n"},{"id":"266465","messageId":"CAPig+cSXuJ7ohR=afzRbmfLtEuzhGYYTznc2J_qxwgBKwyYu9Q@mail.gmail.com","threadId":"39886","inReplyTo":"1437246749-14423-4-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v3 3/9] ref-filter: support printing N lines from tag annotation","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-07-20T00:02:54Z","receivedAt":"2015-07-20T00:02:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak wrote:\n> In 'tag.c' we can print N lines from the annotation of the tag using\n> the '-n' option. Copy code from 'tag.c' to 'ref-filter' and\n> modify 'ref-filter' to support printing of N lines from the annotation\n> of tags.\n>\n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 771c48d..82731ac 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -1288,7 +1288,48 @@ static void assign_formating(struct ref_array_item *ref, int parsed_atom, struct\n> }\n> }\n>\n> -void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)\n> +/* Print 'lines' no of lines of a given oid */\n\nThis is difficult to read and understand. I presume you meant \"no.\" as\nshorthand for \"number\" but dropped the period. Even with the period,\nit's still hard to read. Perhaps rewrite it as:\n\n If 'lines' is greater than 0, print that may lines of...\n\nor something.\n\n> diff --git a/ref-filter.h b/ref-filter.h\n> index c18781b..7dfdea0 100644\n> --- a/ref-filter.h\n> +++ b/ref-filter.h\n> @@ -87,8 +88,11 @@ int parse_ref_filter_atom(const char *atom, const char *ep);\n> int verify_ref_format(const char *format);\n> /* Sort the given ref_array as per the ref_sorting provided */\n> void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);\n> -/* Print the ref using the given format and quote_style */\n> -void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style);\n> +/*\n> + * Print the ref using the given format and quote_style. If lines > 0,\n> + * prints the \"lines\" no of lines of the objected pointed to.\n> + */\n\nSame problem. This literal \"no\" is quite confusing. Perhaps rewrite as above:\n\n If lines > 0, print that many lines of...\n\n> +void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style, unsigned int lines);\n\nref-filter.h seems to be setting a precedent for overlong lines.\nWrapping this manually could improve readability.\n\n> /* Callback function for parsing the sort option */\n> int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset);\n> /* Default sort option based on refname */\n> --\n> 2.4.6\n"},{"id":"266468","messageId":"CAPig+cRkwy_Dq3oYgHdNH6naaUDHZ_6DPopec1EG3aNwz56rzA@mail.gmail.com","threadId":"39886","inReplyTo":"1437246749-14423-5-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v3 4/9] ref-filter: add support to sort by version","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-07-20T01:39:31Z","receivedAt":"2015-07-20T01:39:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak wrote:\n> Add support to sort by version using the \"v:refname\" and\n> \"version:refname\" option. This is achieved by using the\n> 'version_cmp()' function as the comparing function for qsort.\n\nTo agree with the actual code: s/version_cmp/versioncmp/\n\n> This option is included to support sorting by versions in `git tag -l`\n> which will eventaully be ported to use ref-filter APIs.\n>\n> Add documentation and tests for the same.\n>\n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\n> index e49d578..cc91275 100644\n> --- a/Documentation/git-for-each-ref.txt\n> +++ b/Documentation/git-for-each-ref.txt\n> @@ -144,6 +144,8 @@ blank line. Finally, the optional GPG signature is `contents:signature`.\n> For sorting purposes, fields with numeric values sort in numeric\n> order (`objectsize`, `authordate`, `committerdate`, `taggerdate`).\n> All other fields are used to sort in their byte-value order.\n> +There is also an option to sort by versions, this is done using the\n> +field names ('version:refname' or 'v:refname').\n\nAssuming I'm a reader without prior knowledge, the first question\nwhich pops into my mind is \"what's the difference between\n'version:refname' and 'v:refname'?\" Is one just shorthand for the\nother, or is there some subtle difference in behavior, or what? The\ndocumentation should explain this better.\n\nAlso, why are there parentheses around 'version:refname' or\n'v:refname'? And, you should use backticks rather than apostrophes, as\nis done with the other field names.\n\n> In any case, a field name that refers to a field inapplicable to\n> the object referred by the ref does not cause an error. It\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 82731ac..85c561e 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -1169,18 +1170,22 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n>\n> get_ref_atom_value(a, s->atom, &va);\n> get_ref_atom_value(b, s->atom, &vb);\n> - switch (cmp_type) {\n> - case FIELD_STR:\n> - cmp = strcmp(va->s, vb->s);\n> - break;\n> - default:\n> - if (va->ul < vb->ul)\n> - cmp = -1;\n> - else if (va->ul == vb->ul)\n> - cmp = 0;\n> - else\n> - cmp = 1;\n> - break;\n> + if (s->version)\n> + cmp = versioncmp(va->s, vb->s);\n> + else {\n> + switch (cmp_type) {\n> + case FIELD_STR:\n> + cmp = strcmp(va->s, vb->s);\n> + break;\n> + default:\n> + if (va->ul < vb->ul)\n> + cmp = -1;\n> + else if (va->ul == vb->ul)\n> + cmp = 0;\n> + else\n> + cmp = 1;\n> + break;\n> + }\n\nThe logic might be slightly easier to follow, and give a much less\nnoisy diff if you rewrite it like this instead:\n\n if (s->version)\n cmp = versioncmp(va->s, vb->s);\n else if (cmp_type == FIELD_STR)\n cmp = strcmp(va->s, vb->s);\n else {\n if (va->ul < vb->ul)\n cmp = -1;\n else if (va->ul == vb->ul)\n cmp = 0;\n else\n cmp = 1;\n }\n\nOr, if you don't mind a noisy diff, you can outdent the other branches, as well:\n\n if (s->version)\n cmp = versioncmp(va->s, vb->s);\n else if (cmp_type == FIELD_STR)\n cmp = strcmp(va->s, vb->s);\n else if (va->ul < vb->ul)\n cmp = -1;\n else if (va->ul == vb->ul)\n cmp = 0;\n else\n cmp = 1;\n\n(I rather prefer the latter, despite the noisy diff.)\n\n> }\n> return (s->reverse) ? -cmp : cmp;\n> }\n> diff --git a/ref-filter.h b/ref-filter.h\n> index 7dfdea0..6f1646b 100644\n> --- a/ref-filter.h\n> +++ b/ref-filter.h\n> @@ -25,7 +25,7 @@ struct atom_value {\n> struct ref_sorting {\n> struct ref_sorting *next;\n> int atom; /* index into used_atom array (internal) */\n> - unsigned reverse : 1;\n> + unsigned reverse : 1, version : 1;\n\nThis is difficult to read. Style elsewhere (if I'm not mistaken) is to\nplace the declaration on a line by itself.\n\n> };\n>\n> struct ref_array_item {\n> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\n> index 505a360..c31fd2f 100755\n> --- a/t/t6302-for-each-ref-filter.sh\n> +++ b/t/t6302-for-each-ref-filter.sh\n> @@ -81,4 +81,30 @@ test_expect_success 'filtering with --contains' '\n> test_cmp expect actual\n> '\n>\n> +test_expect_success 'version sort' '\n> + git tag -l --sort=version:refname | grep \"foo\" >actual &&\n> + cat >expect <<-\\EOF &&\n> + foo1.3\n> + foo1.6\n> + foo1.10\n> + EOF\n> + test_cmp expect actual\n> +'\n> +\n> +test_expect_success 'reverse version sort' '\n> + git tag -l --sort=-version:refname | grep \"foo\" >actual &&\n\nMaybe use 'v:refname' in one of these tests in order to exercise that\nalias as well?\n\n> + cat >expect <<-\\EOF &&\n> + foo1.10\n> + foo1.6\n> + foo1.3\n> + EOF\n> + test_cmp expect actual\n> +'\n> +\n> test_done\n> --\n> 2.4.6\n"},{"id":"266481","messageId":"CAPig+cSTQgaL-nYfOCWdTaCEpM_23E5TZOPQpnWXEZ8YCcvbZg@mail.gmail.com","threadId":"39886","inReplyTo":"1437246749-14423-6-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v3 5/9] ref-filter: add option to match literal pattern","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-07-20T06:24:54Z","receivedAt":"2015-07-20T06:24:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak wrote:\n> Since 'ref-filter' only has an option to match path names add an\n> option for plain fnmatch pattern-matching.\n>\n> This is to support the pattern matching options which are used in `git\n> tag -l` and `git branch -l` where we can match patterns like `git tag\n> -l foo*` which would match all tags which has a \"foo*\" pattern.\n>\n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 85c561e..7ff3ded 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -966,6 +980,15 @@ static int match_name_as_path(const char **pattern, const char *refname)\n> return 0;\n> }\n>\n> +static int filter_pattern_match(struct ref_filter *filter, const char *refname)\n> +{\n> + if (!*filter->name_patterns)\n> + return 1;\n> + if (filter->match_as_path)\n> + return match_name_as_path(filter->name_patterns, refname);\n> + return match_pattern(filter->name_patterns, refname);\n> +}\n> +\n> /*\n> * Given a ref (sha1, refname), check if the ref belongs to the array\n> * of sha1s. If the given ref is a tag, check if the given tag points\n> @@ -1034,7 +1057,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n> return 0;\n> }\n>\n> - if (*filter->name_patterns && !match_name_as_path(filter->name_patterns, refname))\n> + if (!filter_pattern_match(filter, refname))\n> return 0;\n\nI find it much more difficult to grok the new logic due to\n'*filter->name_patterns' having moved into the called function and its\nnegation inside the function returning 1 which is then negated (again)\nupon return here. This sort of twisty logic places a higher cognitive\nload on the reader. Retaining the original logic makes the code far\nsimpler to understand:\n\n if (*filter->name_patterns &&\n !filter_pattern_match(filter, refname))\n return 0;\n\nalthough it's a bit less nicely encapsulated, so I dunno...\n\n> if (filter->points_at.nr && !match_points_at(&filter->points_at, oid->hash, refname))\n"},{"id":"266485","messageId":"CAP8UFD0kaqSUGMbWrim+59H8CweO-MKSyeQr+3q2cYdJ-+D-Mw@mail.gmail.com","threadId":"39886","inReplyTo":"CAPig+cSTQgaL-nYfOCWdTaCEpM_23E5TZOPQpnWXEZ8YCcvbZg@mail.gmail.com","subject":"Re: [PATCH v3 5/9] ref-filter: add option to match literal pattern","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-07-20T08:01:11Z","receivedAt":"2015-07-20T08:01:11Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Jul 20, 2015 at 8:24 AM, Eric Sunshine wrote:\n> On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak wrote:\n>> Since 'ref-filter' only has an option to match path names add an\n>> option for plain fnmatch pattern-matching.\n>>\n>> This is to support the pattern matching options which are used in `git\n>> tag -l` and `git branch -l` where we can match patterns like `git tag\n>> -l foo*` which would match all tags which has a \"foo*\" pattern.\n>>\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> index 85c561e..7ff3ded 100644\n>> --- a/ref-filter.c\n>> +++ b/ref-filter.c\n>> @@ -966,6 +980,15 @@ static int match_name_as_path(const char **pattern, const char *refname)\n>> return 0;\n>> }\n>>\n>> +static int filter_pattern_match(struct ref_filter *filter, const char *refname)\n>> +{\n>> + if (!*filter->name_patterns)\n>> + return 1;\n>> + if (filter->match_as_path)\n>> + return match_name_as_path(filter->name_patterns, refname);\n>> + return match_pattern(filter->name_patterns, refname);\n>> +}\n>> +\n>> /*\n>> * Given a ref (sha1, refname), check if the ref belongs to the array\n>> * of sha1s. If the given ref is a tag, check if the given tag points\n>> @@ -1034,7 +1057,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n>> return 0;\n>> }\n>>\n>> - if (*filter->name_patterns && !match_name_as_path(filter->name_patterns, refname))\n>> + if (!filter_pattern_match(filter, refname))\n>> return 0;\n>\n> I find it much more difficult to grok the new logic due to\n> '*filter->name_patterns' having moved into the called function and its\n> negation inside the function returning 1 which is then negated (again)\n> upon return here. This sort of twisty logic places a higher cognitive\n> load on the reader. Retaining the original logic makes the code far\n> simpler to understand:\n>\n> if (*filter->name_patterns &&\n> !filter_pattern_match(filter, refname))\n> return 0;\n>\n> although it's a bit less nicely encapsulated, so I dunno...\n\nI think a comment before filter_pattern_match() and perhaps also one\ninside it might help. For example something like:\n\n/* Return 1 if the refname matches one of the patterns, otherwise 0. */\nstatic int filter_pattern_match(struct ref_filter *filter, const char *refname)\n{\n if (!*filter->name_patterns)\n return 1; /* No pattern always matches */\n if (filter->match_as_path)\n return match_name_as_path(filter->name_patterns, refname);\n return match_pattern(filter->name_patterns, refname);\n}\n"},{"id":"266488","messageId":"CAOLa=ZRp0WXKj7au_eDk=80ivsgsNWEyO9JBy_rwzOXmnQwS_A@mail.gmail.com","threadId":"39886","inReplyTo":"CAPig+cRXsmi=UxRr-3rnt919d86jD6uMuTqdDxCComYLXk1TYw@mail.gmail.com","subject":"Re: [PATCH v3 1/9] ref-filter: add option to align atoms to the left","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-20T15:06:44Z","receivedAt":"2015-07-20T15:06:44Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Mon, Jul 20, 2015 at 5:19 AM, Eric Sunshine wrote:\n> On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak wrote:\n>> Add a new atom \"align\" and support %(align:X) where X is a number.\n>> This will align the preceeding atom value to the left followed by\n>> spaces for a total length of X characters. If X is less than the item\n>> size, the entire atom value is printed.\n>>\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> index 7561727..b81a08d 100644\n>> --- a/ref-filter.c\n>> +++ b/ref-filter.c\n>> @@ -53,6 +55,7 @@ static struct {\n>> { \"flag\" },\n>> { \"HEAD\" },\n>> { \"color\" },\n>> + { \"align\" },\n>\n> Not a new issue, but some compilers (Solaris?) complain about the\n> trailing comma.\n>\n\nOk will check.\n\n>> };\n>>\n>> /*\n>> @@ -687,6 +690,17 @@ static void populate_value(struct ref_array_item *ref)\n>> else\n>> v->s = \" \";\n>> continue;\n>> + } else if (starts_with(name, \"align:\")) {\n>> + const char *valp = NULL;\n>> +\n>> + skip_prefix(name, \"align:\", &valp);\n>> + if (!valp[0])\n>> + die(_(\"No value given with 'align='\"));\n>\n> The parser expects \"align:\", but the error message talks about\n> \"align=\". Also, current trend is to drop capitalization from the error\n> message.\n>\n\nThanks will change.\n\n>> + strtoul_ui(valp, 10, &ref->align_value);\n>> + if (ref->align_value < 1)\n>> + die(_(\"Value should be greater than zero\"));\n>\n> Drop capitalization. Also, the user seeing this message won't\n> necessarily know to which value this refers. Better would be to\n> provide context (\"'align:' value should be...\"), and even better would\n> be to show the actual value at fault:\n>\n> die(_(\"value should be greater than zero: align:%u\\n\",\n> ref_align_value);\n>\n> or something.\n\nMakes sense, thanks :)\n\n>\n>> + v->s = \"\";\n>> + continue;\n>> } else\n>> continue;\n>>\n>> @@ -1254,17 +1268,38 @@ static void emit(const char *cp, const char *ep)\n>> }\n>> }\n>>\n>> +static void assign_formating(struct ref_array_item *ref, int parsed_atom, struct atom_value *v)\n>> +{\n>> + if (v->s[0] && ref->align_value) {\n>\n> Mental note: v->s[0] is not NUL ('\\0').\n>\n> Also, in this code base, this is typically written *v->s rather than v->s[0].\n>\n\nMy bad, got confused with that :)\n\n>> + unsigned int len = 0;\n>> + len = utf8_strwidth(v->s);\n>\n> You initialize 'len' to 0 but then immediately re-assign it.\n\nWill change.\n\n>\n>> + if (ref->align_value > len) {\n>> + struct strbuf buf = STRBUF_INIT;\n>> + strbuf_addstr(&buf, v->s);\n>> + if (!v->s[0])\n>> + free((char *)v->s);\n>\n> We know from the \"mental note\" above that v->s[0] is not NUL ('\\0'),\n> so this 'if' statement can never be true, thus is dead code.\n\nYes, my bad. Will change.\n\n>\n>> + strbuf_addchars(&buf, ' ', ref->align_value - len);\n>> + v->s = strbuf_detach(&buf, NULL);\n>> + }\n>> + ref->align_value = 0;\n>> + }\n>> +}\n\nThanks.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"266489","messageId":"CAOLa=ZQBve8Uy+O9mKttTxUBonGGocC=kXS7s8BCf=FaF9sOug@mail.gmail.com","threadId":"39886","inReplyTo":"CAPig+cSXuJ7ohR=afzRbmfLtEuzhGYYTznc2J_qxwgBKwyYu9Q@mail.gmail.com","subject":"Re: [PATCH v3 3/9] ref-filter: support printing N lines from tag annotation","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-20T15:17:24Z","receivedAt":"2015-07-20T15:17:24Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Mon, Jul 20, 2015 at 5:32 AM, Eric Sunshine wrote:\n> On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak wrote:\n>> In 'tag.c' we can print N lines from the annotation of the tag using\n>> the '-n' option. Copy code from 'tag.c' to 'ref-filter' and\n>> modify 'ref-filter' to support printing of N lines from the annotation\n>> of tags.\n>>\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> index 771c48d..82731ac 100644\n>> --- a/ref-filter.c\n>> +++ b/ref-filter.c\n>> @@ -1288,7 +1288,48 @@ static void assign_formating(struct ref_array_item *ref, int parsed_atom, struct\n>> }\n>> }\n>>\n>> -void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)\n>> +/* Print 'lines' no of lines of a given oid */\n>\n> This is difficult to read and understand. I presume you meant \"no.\" as\n> shorthand for \"number\" but dropped the period. Even with the period,\n> it's still hard to read. Perhaps rewrite it as:\n>\n> If 'lines' is greater than 0, print that may lines of...\n>\n> or something.\n>\n\nAh okay :) Will change it to a better explanation.\n\n>> diff --git a/ref-filter.h b/ref-filter.h\n>> index c18781b..7dfdea0 100644\n>> --- a/ref-filter.h\n>> +++ b/ref-filter.h\n>> @@ -87,8 +88,11 @@ int parse_ref_filter_atom(const char *atom, const char *ep);\n>> int verify_ref_format(const char *format);\n>> /* Sort the given ref_array as per the ref_sorting provided */\n>> void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);\n>> -/* Print the ref using the given format and quote_style */\n>> -void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style);\n>> +/*\n>> + * Print the ref using the given format and quote_style. If lines > 0,\n>> + * prints the \"lines\" no of lines of the objected pointed to.\n>> + */\n>\n> Same problem. This literal \"no\" is quite confusing. Perhaps rewrite as above:\n>\n> If lines > 0, print that many lines of...\n\nWill change this too.\n\n>\n>> +void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style, unsigned int lines);\n>\n> ref-filter.h seems to be setting a precedent for overlong lines.\n> Wrapping this manually could improve readability.\n>\n\nDefinitely!\n\nThanks for all the suggestions.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"266493","messageId":"xmqq8uaazu3v.fsf@gitster.dls.corp.google.com","threadId":"39886","inReplyTo":"CAPig+cRXsmi=UxRr-3rnt919d86jD6uMuTqdDxCComYLXk1TYw@mail.gmail.com","subject":"Re: [PATCH v3 1/9] ref-filter: add option to align atoms to the left","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-20T16:12:04Z","receivedAt":"2015-07-20T16:12:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak wrote:\n>> Add a new atom \"align\" and support %(align:X) where X is a number.\n>> This will align the preceeding atom value to the left followed by\n>> spaces for a total length of X characters. If X is less than the item\n>> size, the entire atom value is printed.\n>>\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> index 7561727..b81a08d 100644\n>> --- a/ref-filter.c\n>> +++ b/ref-filter.c\n>> @@ -53,6 +55,7 @@ static struct {\n>> { \"flag\" },\n>> { \"HEAD\" },\n>> { \"color\" },\n>> + { \"align\" },\n>\n> Not a new issue, but some compilers (Solaris?) complain about the\n> trailing comma.\n\nHmm, are you sure? I thought we avoid trailing comma for enum\ndefinitions, but not a list of values like this.\n"},{"id":"266494","messageId":"CAOLa=ZR8d37s4rHuDuASzb_YEs-UBRN0AiY2scEJQe+jFdGxEA@mail.gmail.com","threadId":"39886","inReplyTo":"CAPig+cRkwy_Dq3oYgHdNH6naaUDHZ_6DPopec1EG3aNwz56rzA@mail.gmail.com","subject":"Re: [PATCH v3 4/9] ref-filter: add support to sort by version","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-20T16:34:57Z","receivedAt":"2015-07-20T16:34:57Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Mon, Jul 20, 2015 at 7:09 AM, Eric Sunshine wrote:\n>\n> To agree with the actual code: s/version_cmp/versioncmp/\n>\n\nYeah! will change.\n\n>\n> Assuming I'm a reader without prior knowledge, the first question\n> which pops into my mind is \"what's the difference between\n> 'version:refname' and 'v:refname'?\" Is one just shorthand for the\n> other, or is there some subtle difference in behavior, or what? The\n> documentation should explain this better.\n>\n\nWill include more explanation.\n\n> Also, why are there parentheses around 'version:refname' or\n> 'v:refname'? And, you should use backticks rather than apostrophes, as\n> is done with the other field names.\n>\n\nWanted to show that they are the same command. Seems confusing now that\nyou mentioned, will change it.\n\n>> In any case, a field name that refers to a field inapplicable to\n>> the object referred by the ref does not cause an error. It\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> index 82731ac..85c561e 100644\n>> --- a/ref-filter.c\n>> +++ b/ref-filter.c\n>> @@ -1169,18 +1170,22 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n>>\n>> get_ref_atom_value(a, s->atom, &va);\n>> get_ref_atom_value(b, s->atom, &vb);\n>> - switch (cmp_type) {\n>> - case FIELD_STR:\n>> - cmp = strcmp(va->s, vb->s);\n>> - break;\n>> - default:\n>> - if (va->ul < vb->ul)\n>> - cmp = -1;\n>> - else if (va->ul == vb->ul)\n>> - cmp = 0;\n>> - else\n>> - cmp = 1;\n>> - break;\n>> + if (s->version)\n>> + cmp = versioncmp(va->s, vb->s);\n>> + else {\n>> + switch (cmp_type) {\n>> + case FIELD_STR:\n>> + cmp = strcmp(va->s, vb->s);\n>> + break;\n>> + default:\n>> + if (va->ul < vb->ul)\n>> + cmp = -1;\n>> + else if (va->ul == vb->ul)\n>> + cmp = 0;\n>> + else\n>> + cmp = 1;\n>> + break;\n>> + }\n>\n> The logic might be slightly easier to follow, and give a much less\n> noisy diff if you rewrite it like this instead:\n>\n> if (s->version)\n> cmp = versioncmp(va->s, vb->s);\n> else if (cmp_type == FIELD_STR)\n> cmp = strcmp(va->s, vb->s);\n> else {\n> if (va->ul < vb->ul)\n> cmp = -1;\n> else if (va->ul == vb->ul)\n> cmp = 0;\n> else\n> cmp = 1;\n> }\n>\n> Or, if you don't mind a noisy diff, you can outdent the other branches, as well:\n>\n> if (s->version)\n> cmp = versioncmp(va->s, vb->s);\n> else if (cmp_type == FIELD_STR)\n> cmp = strcmp(va->s, vb->s);\n> else if (va->ul < vb->ul)\n> cmp = -1;\n> else if (va->ul == vb->ul)\n> cmp = 0;\n> else\n> cmp = 1;\n>\n> (I rather prefer the latter, despite the noisy diff.)\n>\n\nErr! just didn't want to existing code. Your latter code usage seems\neasier and simpler to follow. Will use that thanks :D\n\n>> }\n>> return (s->reverse) ? -cmp : cmp;\n>> }\n>> diff --git a/ref-filter.h b/ref-filter.h\n>> index 7dfdea0..6f1646b 100644\n>> --- a/ref-filter.h\n>> +++ b/ref-filter.h\n>> @@ -25,7 +25,7 @@ struct atom_value {\n>> struct ref_sorting {\n>> struct ref_sorting *next;\n>> int atom; /* index into used_atom array (internal) */\n>> - unsigned reverse : 1;\n>> + unsigned reverse : 1, version : 1;\n>\n> This is difficult to read. Style elsewhere (if I'm not mistaken) is to\n> place the declaration on a line by itself.\n>\n\nYes just checked, thats the style. Thank you.\n\n>> };\n>>\n>> struct ref_array_item {\n>> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\n>> index 505a360..c31fd2f 100755\n>> --- a/t/t6302-for-each-ref-filter.sh\n>> +++ b/t/t6302-for-each-ref-filter.sh\n>> @@ -81,4 +81,30 @@ test_expect_success 'filtering with --contains' '\n>> test_cmp expect actual\n>> '\n>>\n>> +test_expect_success 'version sort' '\n>> + git tag -l --sort=version:refname | grep \"foo\" >actual &&\n>> + cat >expect <<-\\EOF &&\n>> + foo1.3\n>> + foo1.6\n>> + foo1.10\n>> + EOF\n>> + test_cmp expect actual\n>> +'\n>> +\n>> +test_expect_success 'reverse version sort' '\n>> + git tag -l --sort=-version:refname | grep \"foo\" >actual &&\n>\n> Maybe use 'v:refname' in one of these tests in order to exercise that\n> alias as well?\n>\n\nThe idea was to only include a minimal test as t7004 has tests for the same,\nbut I guess a minimal test for 'v:refname' is also required.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"266495","messageId":"xmqq1tg2zsxr.fsf@gitster.dls.corp.google.com","threadId":"39886","inReplyTo":"1437246749-14423-2-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v3 1/9] ref-filter: add option to align atoms to the left","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-20T16:37:20Z","receivedAt":"2015-07-20T16:37:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> @@ -687,6 +690,17 @@ static void populate_value(struct ref_array_item *ref)\n> \t\t\telse\n> \t\t\t\tv->s = \" \";\n> \t\t\tcontinue;\n> +\t\t} else if (starts_with(name, \"align:\")) {\n> +\t\t\tconst char *valp = NULL;\n> +\n> +\t\t\tskip_prefix(name, \"align:\", &valp);\n> +\t\t\tif (!valp[0])\n> +\t\t\t\tdie(_(\"No value given with 'align='\"));\n> +\t\t\tstrtoul_ui(valp, 10, &ref->align_value);\n> +\t\t\tif (ref->align_value < 1)\n> +\t\t\t\tdie(_(\"Value should be greater than zero\"));\n> +\t\t\tv->s = \"\";\n> +\t\t\tcontinue;\n> \t\t} else\n> \t\t\tcontinue;\n> \n> @@ -1254,17 +1268,38 @@ static void emit(const char *cp, const char *ep)\n> \t}\n> }\n> \n> +static void assign_formating(struct ref_array_item *ref, int parsed_atom, struct atom_value *v)\n> +{\n> +\tif (v->s[0] && ref->align_value) {\n> +\t\tunsigned int len = 0;\n> +\n> +\t\tlen = utf8_strwidth(v->s);\n> +\t\tif (ref->align_value > len) {\n> +\t\t\tstruct strbuf buf = STRBUF_INIT;\n> +\t\t\tstrbuf_addstr(&buf, v->s);\n> +\t\t\tif (!v->s[0])\n> +\t\t\t\tfree((char *)v->s);\n> +\t\t\tstrbuf_addchars(&buf, ' ', ref->align_value - len);\n> +\t\t\tv->s = strbuf_detach(&buf, NULL);\n> +\t\t}\n> +\t\tref->align_value = 0;\n> +\t}\n> +}\n\nWhat is your plan for this function? Is it envisioned that this\nwill gain more variations of formatting options over time?\nOtherwise it seems misnamed (it is not \"assign formatting\" but\nmerely \"pad to the right\").\n\nI also doubt that the logic is sane. More specifically, what does\nthat \"if (v->s[0])\" buy you?\n\nYour caller is iterating over the elements in a format string,\ne.g. 'A %(align:20)%(foo) B %(bar) C', and its caller is iterating\nover a list of refs, e.g. 'maint', 'master' branches. With that\nformat string, as long as %(foo) does not expand to something that\nexceeds 20 display places or so, I'd expect literal 'B' for all refs\nto align, but I do not think this code gives me that; what happens\nif '%(foo)' happens to be an empty string for 'maint' but is a\nstring, say 'x', for 'master'?\n"},{"id":"266496","messageId":"CAOLa=ZT7szz=cwzEmVDPKucRbh2o_8mBhUY22=R8qCU34QPwGg@mail.gmail.com","threadId":"39886","inReplyTo":"xmqq1tg2zsxr.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 1/9] ref-filter: add option to align atoms to the left","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-20T17:04:27Z","receivedAt":"2015-07-20T17:04:27Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":">\n> What is your plan for this function? Is it envisioned that this\n> will gain more variations of formatting options over time?\n> Otherwise it seems misnamed (it is not \"assign formatting\" but\n> merely \"pad to the right\").\n>\n\nI wanna include an 'ifexists' atom in the future where an user can\nspecify a string format to be printed only if the preceding atom holds\na value.\ne.g. '%(ifexists:refname is %s)%(refname)'\nSo I see that going in here.\n\n> I also doubt that the logic is sane. More specifically, what does\n> that \"if (v->s[0])\" buy you?\n\nYeah, that's wrong, like Eric pointed out. Should have been\n\"if (v->s[0] == '\\0')\"\n\n>\n> Your caller is iterating over the elements in a format string,\n> e.g. 'A %(align:20)%(foo) B %(bar) C', and its caller is iterating\n> over a list of refs, e.g. 'maint', 'master' branches. With that\n> format string, as long as %(foo) does not expand to something that\n> exceeds 20 display places or so, I'd expect literal 'B' for all refs\n> to align, but I do not think this code gives me that; what happens\n> if '%(foo)' happens to be an empty string for 'maint' but is a\n> string, say 'x', for 'master'?\n\nOh yeah! That doesn't work, I have a fix in mind I'll reply to this\nmail with a fix.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"266497","messageId":"1437412947-17555-1-git-send-email-Karthik.188@gmail.com","threadId":"39886","inReplyTo":"CAOLa=ZT7szz=cwzEmVDPKucRbh2o_8mBhUY22=R8qCU34QPwGg@mail.gmail.com","subject":"[PATCH v3 1/9] ref-filter: add option to align atoms to the left","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-20T17:22:27Z","receivedAt":"2015-07-20T17:22:27Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"From: Karthik Nayak \n\nAdd a new atom \"align\" and support %(align:X) where X is a number.\nThis will align the preceeding atom value to the left followed by\nspaces for a total length of X characters. If X is less than the item\nsize, the entire atom value is printed.\n\nHelped-by: Duy Nguyen \nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 41 +++++++++++++++++++++++++++++++++++++++--\n ref-filter.h | 1 +\n 2 files changed, 40 insertions(+), 2 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 7561727..93f59aa 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -10,6 +10,8 @@\n #include \"quote.h\"\n #include \"ref-filter.h\"\n #include \"revision.h\"\n+#include \"utf8.h\"\n+#include \"git-compat-util.h\"\n \n typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n \n@@ -53,6 +55,7 @@ static struct {\n \t{ \"flag\" },\n \t{ \"HEAD\" },\n \t{ \"color\" },\n+\t{ \"align\" },\n };\n \n /*\n@@ -620,7 +623,7 @@ static void populate_value(struct ref_array_item *ref)\n \t\tconst char *name = used_atom[i];\n \t\tstruct atom_value *v = &ref->value[i];\n \t\tint deref = 0;\n-\t\tconst char *refname;\n+\t\tconst char *refname = NULL;\n \t\tconst char *formatp;\n \t\tstruct branch *branch = NULL;\n \n@@ -687,6 +690,17 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\telse\n \t\t\t\tv->s = \" \";\n \t\t\tcontinue;\n+\t\t} else if (starts_with(name, \"align:\")) {\n+\t\t\tconst char *valp = NULL;\n+\n+\t\t\tskip_prefix(name, \"align:\", &valp);\n+\t\t\tif (!valp[0])\n+\t\t\t\tdie(_(\"no value given with 'align:'\"));\n+\t\t\tstrtoul_ui(valp, 10, &ref->align_value);\n+\t\t\tif (ref->align_value < 1)\n+\t\t\t\tdie(_(\"value should be greater than zero: align:%u\"), ref->align_value);\n+\t\t\tv->s = \"\";\n+\t\t\tcontinue;\n \t\t} else\n \t\t\tcontinue;\n \n@@ -1254,17 +1268,40 @@ static void emit(const char *cp, const char *ep)\n \t}\n }\n \n+static void assign_formating(struct ref_array_item *ref, int parsed_atom, struct atom_value *v)\n+{\n+\tif (ref->align_value && !starts_with(used_atom[parsed_atom], \"align\")) {\n+\t\tunsigned int len = 0;\n+\n+\t\tif (*v->s)\n+\t\t\tlen = utf8_strwidth(v->s);\n+\t\tif (ref->align_value > len) {\n+\t\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\t\tif (*v->s)\n+\t\t\t\tstrbuf_addstr(&buf, v->s);\n+\t\t\tif (*v->s && v->s[0] == '\\0')\n+\t\t\t\tfree((char *)v->s);\n+\t\t\tstrbuf_addchars(&buf, ' ', ref->align_value - len);\n+\t\t\tv->s = strbuf_detach(&buf, NULL);\n+\t\t}\n+\t\tref->align_value = 0;\n+\t}\n+}\n+\n void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)\n {\n \tconst char *cp, *sp, *ep;\n \n \tfor (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {\n \t\tstruct atom_value *atomv;\n+\t\tint parsed_atom;\n \n \t\tep = strchr(sp, ')');\n \t\tif (cp < sp)\n \t\t\temit(cp, sp);\n-\t\tget_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);\n+\t\tparsed_atom = parse_ref_filter_atom(sp + 2, ep);\n+\t\tget_ref_atom_value(info, parsed_atom, &atomv);\n+\t\tassign_formating(info, parsed_atom, atomv);\n \t\tprint_value(atomv, quote_style);\n \t}\n \tif (*cp) {\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 6bf27d8..12ffbc5 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -30,6 +30,7 @@ struct ref_sorting {\n struct ref_array_item {\n \tunsigned char objectname[20];\n \tint flag;\n+\tunsigned int align_value;\n \tconst char *symref;\n \tstruct commit *commit;\n \tstruct atom_value *value;\n-- \n2.4.6\n"},{"id":"266498","messageId":"xmqqsi8iybxr.fsf@gitster.dls.corp.google.com","threadId":"39886","inReplyTo":"xmqq1tg2zsxr.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 1/9] ref-filter: add option to align atoms to the left","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-20T17:29:52Z","receivedAt":"2015-07-20T17:29:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Your caller is iterating over the elements in a format string,\n> e.g. 'A %(align:20)%(foo) B %(bar) C', and its caller is iterating\n> over a list of refs, e.g. 'maint', 'master' branches. With that\n> format string, as long as %(foo) does not expand to something that\n> exceeds 20 display places or so, I'd expect literal 'B' for all refs\n> to align, but I do not think this code gives me that; what happens\n> if '%(foo)' happens to be an empty string for 'maint' but is a\n> string, say 'x', for 'master'?\n\nHaving looked at the caller once again, I have to say that the\ninterface to this function is poorly designed. 'info' might have\nbeen a convenient place to keep the \"formatting state\" during this\nloop (e.g. \"was the previous atom tell us to format this atom in a\nspecial way and if so how?\"), but that state does not belong to the\n'info' thing we are getting from our caller. It is something we'd\nwant to clear before we come into the for() loop, and mutate and\nutilize while in the loop. For example, if the caller ever wants\nto show the same ref twice by calling this function with the same\nref twice, and if the format string ended with %(align:N), you do\nnot want that leftover state to right-pad the first atom in the\nsecond invocation.\n\nImagine that in the future you might want to affect how things are\nformatted based on how much we have already output for the ref so\nfar (e.g. limiting the total line length). Where would you implement\nsuch a feature and hook it in this loop?\n\nI'd imagine that a sensible way to organize and structure the\ncodeflow to support this \"align\" and related enhancement we may want\nto have in the future cleanly would be to teach \"print_value\" about\nthe \"formatting state\" and share it with this loop. Roughly...\n\n> void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)\n> {\n> \tconst char *cp, *sp, *ep;\n> \n\nInsert something like this here:\n\n\tstruct ref_formatting_state state;\n\n\tmemset(&state, 0, sizeof(state));\n state.quote_style = quote_style;\n\n> \tfor (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {\n> \t\tstruct atom_value *atomv;\n> +\t\tint parsed_atom;\n> \n> \t\tep = strchr(sp, ')');\n> \t\tif (cp < sp)\n> \t\t\temit(cp, sp);\n> -\t\tget_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);\n> +\t\tparsed_atom = parse_ref_filter_atom(sp + 2, ep);\n> +\t\tget_ref_atom_value(info, parsed_atom, &atomv);\n> +\t\tassign_formating(info, parsed_atom, atomv);\n> \t\tprint_value(atomv, quote_style);\n\nand replace all of the above with something like this (a separate\nvariable parsed_atom may not be necessary):\n\n\t\tget_ref_atom_value(&state, info,\n\t\t\t\tparse_ref_filter_atom(sp + 2, ep), &atomv);\n\t\tprint_value(&state, atomv);\n\nThings like %(align:20) are not really atoms in the sense that they\nare not used as placeholders for attributes that refs being printed\nhave, but they are there solely in order to affect the \"formating\nstate\". Introduce a new field \"struct atom_value.pseudo_atom\" to\ntell print_value() that fact from get_ref_atom_value(), e.g.\n\n\tstatic void print_value(struct ref_formatting_state *state,\n \t\t\tstruct atom_value *v)\n\t{\n\t\tstruct strbuf value = STRBUF_INIT;\n\t\tstruct strbuf formatted = STRBUF_INIT;\n\n \tif (v->pseudo_atom)\n \treturn;\n\t\tif (state->pad_to_right) {\n \tstrbuf_addf(&value, \"%.*s\", state->pad_to_right, v->s);\n\t\t\tstate->pad_to_right = 0;\n\t\t}\n\t\tswitch (state->quote_style) { \n\t\tcase QUOTE_SHELL:\n \tsq_quote_buf(&formatted, value.buf);\n break;\n ...\n\t\t}\n fputs(formatted.buf, stdout);\n strbuf_release(&value);\n strbuf_release(&formatted);\n\t}\n\nor something like that. As this print_value() knows everything that\nhappens to a single output line during that loop and is allowed to\nkeep track of what it sees in 'state', this would give a natural and\ncodeflow to add 'limit the total line length' and things like that\nif desired.\n\nWe may want to further clean up to update %(color) thing to clarify\nthat it is a pseudo atom. I suspect %(align:20)%(color:blue) would\ndo a wrong thing with the current code, and it would be a reasonable\nthing to allow both of these interchangeably:\n\n %(align:20)%(color:blue)%(refname:short)%(color:reset)\n %(color:blue)%(align:20)%(refname:short)%(color:reset)\n\nand implementation of that would become more obvious once you have a\nmore explicit \"formatting state\" that is known to and shared among\nget_value(), the for() loop that walks the format string, and\nprint_value().\n"},{"id":"266500","messageId":"CAPig+cT-_Ha=zF1E832cjN-Yhqgs1W73+A2+O9MxgeJ+VJNzbQ@mail.gmail.com","threadId":"39886","inReplyTo":"xmqq8uaazu3v.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 1/9] ref-filter: add option to align atoms to the left","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-07-20T17:47:44Z","receivedAt":"2015-07-20T17:47:44Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jul 20, 2015 at 12:12 PM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>\n>> On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak wrote:\n>>> Add a new atom \"align\" and support %(align:X) where X is a number.\n>>> This will align the preceeding atom value to the left followed by\n>>> spaces for a total length of X characters. If X is less than the item\n>>> size, the entire atom value is printed.\n>>>\n>>> Signed-off-by: Karthik Nayak \n>>> ---\n>>> diff --git a/ref-filter.c b/ref-filter.c\n>>> index 7561727..b81a08d 100644\n>>> --- a/ref-filter.c\n>>> +++ b/ref-filter.c\n>>> @@ -53,6 +55,7 @@ static struct {\n>>> { \"flag\" },\n>>> { \"HEAD\" },\n>>> { \"color\" },\n>>> + { \"align\" },\n>>\n>> Not a new issue, but some compilers (Solaris?) complain about the\n>> trailing comma.\n>\n> Hmm, are you sure? I thought we avoid trailing comma for enum\n> definitions, but not a list of values like this.\n\nIt's been years since I encountered such a compiler, so it's possible\nthat my brain is conflating different cases of trailing commas...\n"},{"id":"266501","messageId":"CAPig+cQ1uAHxMRuiBja2ap-nVCU2dUMLcN40Xb=HnT_kN_MW_A@mail.gmail.com","threadId":"39886","inReplyTo":"1437412947-17555-1-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v3 1/9] ref-filter: add option to align atoms to the left","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-07-20T18:01:15Z","receivedAt":"2015-07-20T18:01:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jul 20, 2015 at 1:22 PM, Karthik Nayak wrote:\n> Add a new atom \"align\" and support %(align:X) where X is a number.\n> This will align the preceeding atom value to the left followed by\n> spaces for a total length of X characters. If X is less than the item\n> size, the entire atom value is printed.\n>\n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 7561727..93f59aa 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -687,6 +690,17 @@ static void populate_value(struct ref_array_item *ref)\n> else\n> v->s = \" \";\n> continue;\n> + } else if (starts_with(name, \"align:\")) {\n> + const char *valp = NULL;\n> +\n> + skip_prefix(name, \"align:\", &valp);\n> + if (!valp[0])\n> + die(_(\"no value given with 'align:'\"));\n> + strtoul_ui(valp, 10, &ref->align_value);\n> + if (ref->align_value < 1)\n> + die(_(\"value should be greater than zero: align:%u\"), ref->align_value);\n> + v->s = \"\";\n\nMental note: v->s points at literal zero-length string (\"\").\n\n> + continue;\n> } else\n> continue;\n>\n> @@ -1254,17 +1268,40 @@ static void emit(const char *cp, const char *ep)\n> }\n> }\n>\n> +static void assign_formating(struct ref_array_item *ref, int parsed_atom, struct atom_value *v)\n> +{\n> + if (ref->align_value && !starts_with(used_atom[parsed_atom], \"align\")) {\n> + unsigned int len = 0;\n> +\n> + if (*v->s)\n> + len = utf8_strwidth(v->s);\n> + if (ref->align_value > len) {\n> + struct strbuf buf = STRBUF_INIT;\n> + if (*v->s)\n> + strbuf_addstr(&buf, v->s);\n> + if (*v->s && v->s[0] == '\\0')\n> + free((char *)v->s);\n\nIs the \"v->s[0] == '\\0'\" checking for the same literal zero-length\nstring assigned above? If so, attempting to free() that string doesn't\nmake sense, since it's not heap-allocated. Maybe you meant != '\\0'?\n\nOverall, this code is getting rather complex and difficult to follow\n(especially with all the 'v->s' checks thrown in). Junio's proposed\n'pseudo_atom' and 'ref_formatting_state' would go a long way toward\nsimplifying it.\n\n> + strbuf_addchars(&buf, ' ', ref->align_value - len);\n> + v->s = strbuf_detach(&buf, NULL);\n> + }\n> + ref->align_value = 0;\n> + }\n> +}\n"},{"id":"266504","messageId":"CAPig+cSu7-wegGd90WHeppt8inHSBxrakzuJSvcRV7OocJHAgg@mail.gmail.com","threadId":"39886","inReplyTo":"CAP8UFD0kaqSUGMbWrim+59H8CweO-MKSyeQr+3q2cYdJ-+D-Mw@mail.gmail.com","subject":"Re: [PATCH v3 5/9] ref-filter: add option to match literal pattern","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-07-20T18:12:11Z","receivedAt":"2015-07-20T18:12:11Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jul 20, 2015 at 4:01 AM, Christian Couder\n wrote:\n> On Mon, Jul 20, 2015 at 8:24 AM, Eric Sunshine wrote:\n>> On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak wrote:\n>>> +static int filter_pattern_match(struct ref_filter *filter, const char *refname)\n>>> +{\n>>> + if (!*filter->name_patterns)\n>>> + return 1;\n>>> + if (filter->match_as_path)\n>>> + return match_name_as_path(filter->name_patterns, refname);\n>>> + return match_pattern(filter->name_patterns, refname);\n>>> +}\n>>> @@ -1034,7 +1057,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n>>> - if (*filter->name_patterns && !match_name_as_path(filter->name_patterns, refname))\n>>> + if (!filter_pattern_match(filter, refname))\n>>> return 0;\n>>\n>> I find it much more difficult to grok the new logic due to\n>> '*filter->name_patterns' having moved into the called function and its\n>> negation inside the function returning 1 which is then negated (again)\n>> upon return here. This sort of twisty logic places a higher cognitive\n>> load on the reader. Retaining the original logic makes the code far\n>> simpler to understand:\n>>\n>> if (*filter->name_patterns &&\n>> !filter_pattern_match(filter, refname))\n>> return 0;\n>>\n>> although it's a bit less nicely encapsulated, so I dunno...\n>\n> I think a comment before filter_pattern_match() and perhaps also one\n> inside it might help. For example something like:\n>\n> /* Return 1 if the refname matches one of the patterns, otherwise 0. */\n> static int filter_pattern_match(struct ref_filter *filter, const char *refname)\n> {\n> if (!*filter->name_patterns)\n> return 1; /* No pattern always matches */\n> if (filter->match_as_path)\n> return match_name_as_path(filter->name_patterns, refname);\n> return match_pattern(filter->name_patterns, refname);\n> }\n\nYes, the comments do improve the situation and may be a reasonable compromise...\n"},{"id":"266505","messageId":"CAOLa=ZTJ3syFEMjdwumZEP+XXzVK9qWz2S=7QsK_N1ev-WULtw@mail.gmail.com","threadId":"39886","inReplyTo":"CAPig+cQ1uAHxMRuiBja2ap-nVCU2dUMLcN40Xb=HnT_kN_MW_A@mail.gmail.com","subject":"Re: [PATCH v3 1/9] ref-filter: add option to align atoms to the left","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-20T18:23:11Z","receivedAt":"2015-07-20T18:23:11Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Mon, Jul 20, 2015 at 11:31 PM, Eric Sunshine wrote:\n> On Mon, Jul 20, 2015 at 1:22 PM, Karthik Nayak wrote:\n>> Add a new atom \"align\" and support %(align:X) where X is a number.\n>> This will align the preceeding atom value to the left followed by\n>> spaces for a total length of X characters. If X is less than the item\n>> size, the entire atom value is printed.\n>>\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> index 7561727..93f59aa 100644\n>> --- a/ref-filter.c\n>> +++ b/ref-filter.c\n>> @@ -687,6 +690,17 @@ static void populate_value(struct ref_array_item *ref)\n>> else\n>> v->s = \" \";\n>> continue;\n>> + } else if (starts_with(name, \"align:\")) {\n>> + const char *valp = NULL;\n>> +\n>> + skip_prefix(name, \"align:\", &valp);\n>> + if (!valp[0])\n>> + die(_(\"no value given with 'align:'\"));\n>> + strtoul_ui(valp, 10, &ref->align_value);\n>> + if (ref->align_value < 1)\n>> + die(_(\"value should be greater than zero: align:%u\"), ref->align_value);\n>> + v->s = \"\";\n>\n> Mental note: v->s points at literal zero-length string (\"\").\n>\n>> + continue;\n>> } else\n>> continue;\n>>\n>> @@ -1254,17 +1268,40 @@ static void emit(const char *cp, const char *ep)\n>> }\n>> }\n>>\n>> +static void assign_formating(struct ref_array_item *ref, int parsed_atom, struct atom_value *v)\n>> +{\n>> + if (ref->align_value && !starts_with(used_atom[parsed_atom], \"align\")) {\n>> + unsigned int len = 0;\n>> +\n>> + if (*v->s)\n>> + len = utf8_strwidth(v->s);\n>> + if (ref->align_value > len) {\n>> + struct strbuf buf = STRBUF_INIT;\n>> + if (*v->s)\n>> + strbuf_addstr(&buf, v->s);\n>> + if (*v->s && v->s[0] == '\\0')\n>> + free((char *)v->s);\n>\n> Is the \"v->s[0] == '\\0'\" checking for the same literal zero-length\n> string assigned above? If so, attempting to free() that string doesn't\n> make sense, since it's not heap-allocated. Maybe you meant != '\\0'?\n>\n> Overall, this code is getting rather complex and difficult to follow\n> (especially with all the 'v->s' checks thrown in). Junio's proposed\n> 'pseudo_atom' and 'ref_formatting_state' would go a long way toward\n> simplifying it.\n>\n\nYou're right, thats what I meant.\nHaving a look at Junio's proposed idea.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"266554","messageId":"CAOLa=ZRc2PZ9XqN5WfjN+UswHy3c+MSTZKfROqYk9eRJR0EjcA@mail.gmail.com","threadId":"39886","inReplyTo":"xmqqsi8iybxr.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 1/9] ref-filter: add option to align atoms to the left","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-21T18:00:44Z","receivedAt":"2015-07-21T18:00:44Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Mon, Jul 20, 2015 at 10:59 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Your caller is iterating over the elements in a format string,\n>> e.g. 'A %(align:20)%(foo) B %(bar) C', and its caller is iterating\n>> over a list of refs, e.g. 'maint', 'master' branches. With that\n>> format string, as long as %(foo) does not expand to something that\n>> exceeds 20 display places or so, I'd expect literal 'B' for all refs\n>> to align, but I do not think this code gives me that; what happens\n>> if '%(foo)' happens to be an empty string for 'maint' but is a\n>> string, say 'x', for 'master'?\n>\n> Having looked at the caller once again, I have to say that the\n> interface to this function is poorly designed. 'info' might have\n> been a convenient place to keep the \"formatting state\" during this\n> loop (e.g. \"was the previous atom tell us to format this atom in a\n> special way and if so how?\"), but that state does not belong to the\n> 'info' thing we are getting from our caller. It is something we'd\n> want to clear before we come into the for() loop, and mutate and\n> utilize while in the loop. For example, if the caller ever wants\n> to show the same ref twice by calling this function with the same\n> ref twice, and if the format string ended with %(align:N), you do\n> not want that leftover state to right-pad the first atom in the\n> second invocation.\n\nYou do have a point, my current implementation won't work with the\nscenario you just mentioned.\n\n>\n> Imagine that in the future you might want to affect how things are\n> formatted based on how much we have already output for the ref so\n> far (e.g. limiting the total line length). Where would you implement\n> such a feature and hook it in this loop?\n>\n> I'd imagine that a sensible way to organize and structure the\n> codeflow to support this \"align\" and related enhancement we may want\n> to have in the future cleanly would be to teach \"print_value\" about\n> the \"formatting state\" and share it with this loop. Roughly...\n>\n>> void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)\n>> {\n>> const char *cp, *sp, *ep;\n>>\n>\n> Insert something like this here:\n>\n> struct ref_formatting_state state;\n>\n> memset(&state, 0, sizeof(state));\n> state.quote_style = quote_style;\n>\n>> for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {\n>> struct atom_value *atomv;\n>> + int parsed_atom;\n>>\n>> ep = strchr(sp, ')');\n>> if (cp < sp)\n>> emit(cp, sp);\n>> - get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);\n>> + parsed_atom = parse_ref_filter_atom(sp + 2, ep);\n>> + get_ref_atom_value(info, parsed_atom, &atomv);\n>> + assign_formating(info, parsed_atom, atomv);\n>> print_value(atomv, quote_style);\n>\n> and replace all of the above with something like this (a separate\n> variable parsed_atom may not be necessary):\n>\n> get_ref_atom_value(&state, info,\n> parse_ref_filter_atom(sp + 2, ep), &atomv);\n> print_value(&state, atomv);\n>\n> Things like %(align:20) are not really atoms in the sense that they\n> are not used as placeholders for attributes that refs being printed\n> have, but they are there solely in order to affect the \"formating\n> state\". Introduce a new field \"struct atom_value.pseudo_atom\" to\n> tell print_value() that fact from get_ref_atom_value(), e.g.\n>\n> static void print_value(struct ref_formatting_state *state,\n> struct atom_value *v)\n> {\n> struct strbuf value = STRBUF_INIT;\n> struct strbuf formatted = STRBUF_INIT;\n>\n> if (v->pseudo_atom)\n> return;\n> if (state->pad_to_right) {\n> strbuf_addf(&value, \"%.*s\", state->pad_to_right, v->s);\n> state->pad_to_right = 0;\n> }\n> switch (state->quote_style) {\n> case QUOTE_SHELL:\n> sq_quote_buf(&formatted, value.buf);\n> break;\n> ...\n> }\n> fputs(formatted.buf, stdout);\n> strbuf_release(&value);\n> strbuf_release(&formatted);\n> }\n>\n> or something like that. As this print_value() knows everything that\n> happens to a single output line during that loop and is allowed to\n> keep track of what it sees in 'state', this would give a natural and\n> codeflow to add 'limit the total line length' and things like that\n> if desired.\n\nThis implementation looks good. Will include this, thanks a bunch.\n\n>\n> We may want to further clean up to update %(color) thing to clarify\n> that it is a pseudo atom. I suspect %(align:20)%(color:blue) would\n> do a wrong thing with the current code, and it would be a reasonable\n> thing to allow both of these interchangeably:\n>\n> %(align:20)%(color:blue)%(refname:short)%(color:reset)\n> %(color:blue)%(align:20)%(refname:short)%(color:reset)\n>\n> and implementation of that would become more obvious once you have a\n> more explicit \"formatting state\" that is known to and shared among\n> get_value(), the for() loop that walks the format string, and\n> print_value().\n\nYup! ill put in a fix for %(color:) with this patch.\n\nThanks a lot.\n\n\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"266564","messageId":"CAOLa=ZTaNcaiVt02BE4JukSj-ADeoQByWtH1jBvn5XUrVUztEQ@mail.gmail.com","threadId":"39886","inReplyTo":"CAPig+cSu7-wegGd90WHeppt8inHSBxrakzuJSvcRV7OocJHAgg@mail.gmail.com","subject":"Re: [PATCH v3 5/9] ref-filter: add option to match literal pattern","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-21T19:27:21Z","receivedAt":"2015-07-21T19:27:21Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Mon, Jul 20, 2015 at 11:42 PM, Eric Sunshine wrote:\n> On Mon, Jul 20, 2015 at 4:01 AM, Christian Couder\n> wrote:\n>> On Mon, Jul 20, 2015 at 8:24 AM, Eric Sunshine wrote:\n>>> On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak wrote:\n>>>> +static int filter_pattern_match(struct ref_filter *filter, const char *refname)\n>>>> +{\n>>>> + if (!*filter->name_patterns)\n>>>> + return 1;\n>>>> + if (filter->match_as_path)\n>>>> + return match_name_as_path(filter->name_patterns, refname);\n>>>> + return match_pattern(filter->name_patterns, refname);\n>>>> +}\n>>>> @@ -1034,7 +1057,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n>>>> - if (*filter->name_patterns && !match_name_as_path(filter->name_patterns, refname))\n>>>> + if (!filter_pattern_match(filter, refname))\n>>>> return 0;\n>>>\n>>> I find it much more difficult to grok the new logic due to\n>>> '*filter->name_patterns' having moved into the called function and its\n>>> negation inside the function returning 1 which is then negated (again)\n>>> upon return here. This sort of twisty logic places a higher cognitive\n>>> load on the reader. Retaining the original logic makes the code far\n>>> simpler to understand:\n>>>\n>>> if (*filter->name_patterns &&\n>>> !filter_pattern_match(filter, refname))\n>>> return 0;\n>>>\n>>> although it's a bit less nicely encapsulated, so I dunno...\n>>\n>> I think a comment before filter_pattern_match() and perhaps also one\n>> inside it might help. For example something like:\n>>\n>> /* Return 1 if the refname matches one of the patterns, otherwise 0. */\n>> static int filter_pattern_match(struct ref_filter *filter, const char *refname)\n>> {\n>> if (!*filter->name_patterns)\n>> return 1; /* No pattern always matches */\n>> if (filter->match_as_path)\n>> return match_name_as_path(filter->name_patterns, refname);\n>> return match_pattern(filter->name_patterns, refname);\n>> }\n>\n> Yes, the comments do improve the situation and may be a reasonable compromise...\n\nYes, these comments would help, thanks :D\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"266565","messageId":"CAOLa=ZQ9PHfppqbMzrhfTKQws-Goz7SGPSE2S-WNwMWxUC0fwQ@mail.gmail.com","threadId":"39886","inReplyTo":"CAP8UFD0EXgt7ijdv2cY-HGuqcwjz=Z3-8KV7Xqk=-EBRp-QrGQ@mail.gmail.com","subject":"Re: [PATCH v3 9/9] tag.c: implement '--merged' and '--no-merged' options","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-21T19:28:53Z","receivedAt":"2015-07-21T19:28:53Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Mon, Jul 20, 2015 at 12:50 AM, Christian Couder\n wrote:\n> On Sun, Jul 19, 2015 at 12:00 AM, Karthik Nayak wrote:\n>> From: Karthik Nayak \n>>\n>> Using 'ref-filter' APIs implement the '--merged' and '--no-merged'\n>> options into 'tag.c'. The '--merged' option lets the user to only\n>> list tags merged into the named commit. The '--no-merged' option\n>> lets the user to only list tags not merged into the named commit.\n>> If no object is provided it assumes HEAD as the object.\n>>\n>> Add documentation and tests for the same.\n>>\n>> Mentored-by: Christian Couder \n>> Mentored-by: Matthieu Moy \n>> Signed-off-by: Karthik Nayak \n>> ---\n>> Documentation/git-tag.txt | 10 +++++++++-\n>> builtin/tag.c | 6 +++++-\n>> t/t7004-tag.sh | 27 +++++++++++++++++++++++++++\n>> 3 files changed, 41 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\n>> index 16e396c..74ed157 100644\n>> --- a/Documentation/git-tag.txt\n>> +++ b/Documentation/git-tag.txt\n>> @@ -14,7 +14,7 @@ SYNOPSIS\n>> 'git tag' -d ...\n>> 'git tag' [-n[]] -l [--contains ] [--points-at ]\n>> [--column[=] | --no-column] [--sort=] [--format=]\n>> - [...]\n>> + [(--merged | --no-merged) []] [...]\n>\n> Maybe [--[no-]merged []] instead of [(--merged | --no-merged)\n> []].\n>\n\nLooks better. will use.\n\n>> 'git tag' -v ...\n>>\n>> DESCRIPTION\n>> @@ -169,6 +169,14 @@ This option is only applicable when listing tags without annotation lines.\n>> `%09` to `\\t` (TAB) and `%0a` to `\\n` (LF).\n>> The fields are same as those in `git for-each-ref`.\n>>\n>> +--merged []::\n>> + Only list tags whose tips are reachable from the\n>> + specified commit (HEAD if not specified).\n>> +\n>> +--no-merged []::\n>> + Only list tags whose tips are not reachable from the\n>> + specified commit (HEAD if not specified).\n>\n> Here also you could write something like:\n>\n> +--[no-]merged []::\n> + Only list tags whose tips are reachable, or not reachable\n> + if --no-merged is used, from the specified commit\n> + (HEAD if not specified).\n>\n>>\n>> CONFIGURATION\n>> -------------\n>> diff --git a/builtin/tag.c b/builtin/tag.c\n>> index cae113b..0fa1d31 100644\n>> --- a/builtin/tag.c\n>> +++ b/builtin/tag.c\n>> @@ -23,7 +23,7 @@ static const char * const git_tag_usage[] = {\n>> N_(\"git tag [-a | -s | -u ] [-f] [-m | -F ] []\"),\n>> N_(\"git tag -d ...\"),\n>> N_(\"git tag -l [-n[]] [--contains ] [--points-at ]\"\n>> - \"\\n\\t\\t[...]\"),\n>> + \"\\n\\t\\t[--merged []] [--no-merged []] [...]\"),\n>\n> [--[no-]merged []] here too.\n>\n> Thanks,\n> Christian.\n\nHere too, thanks :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"266616","messageId":"vpqwpxsnibr.fsf@anie.imag.fr","threadId":"39886","inReplyTo":"1437246749-14423-2-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v3 1/9] ref-filter: add option to align atoms to the left","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-07-22T18:44:08Z","receivedAt":"2015-07-22T18:44:08Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Karthik Nayak writes:\n\n> +\t\t\tstrtoul_ui(valp, 10, &ref->align_value);\n> +\t\t\tif (ref->align_value < 1)\n> +\t\t\t\tdie(_(\"Value should be greater than zero\"));\n\nYou're not checking the return value of strtoul_ui, which returns -1\nbefore assigning align_value if the value can't be parsed. As a result,\nyou're testing an undefined value in the 'if' statement in this case.\n\nYou should test the return value and issue a distinct error message in\nthis case like\n\nif (strtoul_ui(valp, 10, &ref->align_value))\n\tdie(_(\"positive integer expected after ':' in align:%u\\n\",\n\t ref_align_value));\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"266621","messageId":"vpqmvyongnf.fsf@anie.imag.fr","threadId":"39886","inReplyTo":"1437246749-14423-6-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v3 5/9] ref-filter: add option to match literal pattern","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-07-22T19:20:20Z","receivedAt":"2015-07-22T19:20:20Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Karthik Nayak writes:\n\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -943,9 +943,23 @@ static int commit_contains(struct ref_filter *filter, struct commit *commit)\n> \n> /*\n> * Return 1 if the refname matches one of the patterns, otherwise 0.\n> + * A pattern can be a literal prefix (e.g. a refname \"refs/heads/master\"\n> + * matches a pattern \"refs/heads/m\") or a wildcard (e.g. the same ref\n> + * matches \"refs/heads/m*\", too).\n> + */\n> +static int match_pattern(const char **patterns, const char *refname)\n> +{\n> +\tfor (; *patterns; patterns++)\n> +\t\tif (!wildmatch(*patterns, refname, 0, NULL))\n> +\t\t\treturn 1;\n> +\treturn 0;\n> +}\n> +\n> +/*\n> + * Return 1 if the refname matches one of the patterns, otherwise 0.\n> * A pattern can be path prefix (e.g. a refname \"refs/heads/master\"\n> * matches a pattern \"refs/heads/\") or a wildcard (e.g. the same ref\n\nWhile you're there, why not say explicitly\n\n * A pattern can be path prefix (e.g. a refname \"refs/heads/master\"\n * matches a pattern \"refs/heads/\" but not \"refs/heads/m\")\n ^^^^^^^^^^^^^^^^^^^^^^\n ...\n\n(I find it frustrating when the docstrings for two function look\nidentical and I have to find out the 1-character difference to\nunderstand ...)\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"266625","messageId":"vpq7fpsnft1.fsf@anie.imag.fr","threadId":"39886","inReplyTo":"1437256837-13378-2-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v3 8/9] tag.c: implement '--format' option","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-07-22T19:38:34Z","receivedAt":"2015-07-22T19:38:34Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Karthik Nayak writes:\n\n> --- a/Documentation/git-tag.txt\n> +++ b/Documentation/git-tag.txt\n> @@ -13,7 +13,8 @@ SYNOPSIS\n> \t [ | ]\n> 'git tag' -d ...\n> 'git tag' [-n[]] -l [--contains ] [--points-at ]\n> -\t[--column[=] | --no-column] [--sort=] [...]\n> +\t[--column[=] | --no-column] [--sort=] [--format=]\n> +\t[...]\n> 'git tag' -v ...\n> \n> DESCRIPTION\n> @@ -155,6 +156,19 @@ This option is only applicable when listing tags without annotation lines.\n> \tThe object that the new tag will refer to, usually a commit.\n> \tDefaults to HEAD.\n> \n> +::\n> +\tA string that interpolates `%(fieldname)` from the\n> +\tobject pointed at by a ref being shown. If `fieldname`\n> +\tis prefixed with an asterisk (`*`) and the ref points\n> +\tat a tag object, the value for the field in the object\n> +\ttag refers is used. When unspecified, defaults to\n> +\t`%(objectname) SPC %(objecttype) TAB %(refname)`.\n\nI think this last sentence is taken from for-each-ref where it is true,\nbut for 'git tag', the default is just %(refname:short), as written\nhere:\n\n> -\telse\n> +\telse if (!format)\n> \t\tformat = \"%(refname:short)\";\n\nright?\n\n> --- a/t/t7004-tag.sh\n> +++ b/t/t7004-tag.sh\n> @@ -1507,4 +1507,20 @@ EOF\"\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success '--format cannot be used with -n' '\n> +\ttest_must_fail git tag -l -n4 --format=\"%(refname)\"\n> +'\n> +\n> +test_expect_success '--format should list tags as per format given' '\n> +\tcat >expect <<-\\EOF &&\n> +\tfoo1.10\n> +\tfoo1.3\n> +\tfoo1.6\n> +\tfoo1.6-rc1\n> +\tfoo1.6-rc2\n> +\tEOF\n> +\tgit tag -l --format=\"%(refname)\" \"foo*\" >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nThis tests the pattern argument, but the the test still passes if I\nremove the --format option, so it does not test what it claims.\n\nAlso, why does \"git tag\"'s %(refname) behave like \"git for-each-ref\"'s\n%(refname:short)? I find it very confusing as I think --format's\nargument should be interpreted the same way for all ref-listing\ncommands. Actually I didn't find a way to have \"git tag\" display the\nfull refname other than with --format \"refs/tags/%(refname)\".\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"266626","messageId":"vpqy4i8m14v.fsf@anie.imag.fr","threadId":"39886","inReplyTo":"1437256837-13378-3-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v3 9/9] tag.c: implement '--merged' and '--no-merged' options","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-07-22T19:40:48Z","receivedAt":"2015-07-22T19:40:48Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Karthik Nayak writes:\n\n> +--merged []::\n> +\tOnly list tags whose tips are reachable from the\n> +\tspecified commit (HEAD if not specified).\n> +\n> +--no-merged []::\n> +\tOnly list tags whose tips are not reachable from the\n> +\tspecified commit (HEAD if not specified).\n\nYou may want to spell it `HEAD` (with backticks).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"266659","messageId":"CAOLa=ZRZeTqhbFB4JASac+QOFsJEvqWAsGdpx2hbd-myPd1a9g@mail.gmail.com","threadId":"39886","inReplyTo":"vpqwpxsnibr.fsf@anie.imag.fr","subject":"Re: [PATCH v3 1/9] ref-filter: add option to align atoms to the left","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-23T14:32:10Z","receivedAt":"2015-07-23T14:32:10Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Thu, Jul 23, 2015 at 12:14 AM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> + strtoul_ui(valp, 10, &ref->align_value);\n>> + if (ref->align_value < 1)\n>> + die(_(\"Value should be greater than zero\"));\n>\n> You're not checking the return value of strtoul_ui, which returns -1\n> before assigning align_value if the value can't be parsed. As a result,\n> you're testing an undefined value in the 'if' statement in this case.\n>\n> You should test the return value and issue a distinct error message in\n> this case like\n>\n> if (strtoul_ui(valp, 10, &ref->align_value))\n> die(_(\"positive integer expected after ':' in align:%u\\n\",\n> ref_align_value));\n>\n> --\n> Matthieu Moy\n> http://www-verimag.imag.fr/~moy/\n\nMakes sense, thanks :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"266664","messageId":"CAOLa=ZS5=S_vyCKOFTQbb+0-9WYnwrOyU917GLjfHEr-GYwybw@mail.gmail.com","threadId":"39886","inReplyTo":"vpqy4i8m14v.fsf@anie.imag.fr","subject":"Re: [PATCH v3 9/9] tag.c: implement '--merged' and '--no-merged' options","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-23T16:20:52Z","receivedAt":"2015-07-23T16:20:52Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Thu, Jul 23, 2015 at 1:10 AM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> +--merged []::\n>> + Only list tags whose tips are reachable from the\n>> + specified commit (HEAD if not specified).\n>> +\n>> +--no-merged []::\n>> + Only list tags whose tips are not reachable from the\n>> + specified commit (HEAD if not specified).\n>\n> You may want to spell it `HEAD` (with backticks).\n>\n\nWill do!\n\n-- \nRegards,\nKarthik Nayak\n"}]}