{"thread":{"id":"40009","subject":"[PATCH v9 0/11] Port tag.c over to use ref-filter APIs","startedAt":"2015-08-04T12:39:52Z","lastAt":"2015-08-07T17:50:30Z","messageCount":21,"participants":["Karthik Nayak","Eric Sunshine"],"isPatch":true,"patchVersion":9,"patchTotal":11},"messages":[{"id":"267396","messageId":"CAOLa=ZSBMk9y1VGTVKSVsGMdYuPtjhPADciVUaEVwESRdSvWZg@mail.gmail.com","threadId":"40009","inReplyTo":null,"subject":"[PATCH v9 0/11] Port tag.c over to use ref-filter APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-04T12:39:52Z","receivedAt":"2015-08-04T12:39:52Z","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. This patch series is continued from: Git (next)\nhttps://github.com/git/git/commit/bf5418f49ff0cebc6e5ce04ad1417e1a47c81b61\n\nThis series consists of porting tag.c over to using the ref-filter APIs\n\nVersion 8 can be found here:\nhttp://thread.gmane.org/gmane.comp.version-control.git/274990\n\nChanges:\n* align now excepts strings till a %(end) is met.\n* implement the current strbuf within ref_formatting_change\n* change the strbuf name from format to final.\n\nInterdiff:\n\ndiff --git a/Documentation/git-for-each-ref.txt\nb/Documentation/git-for-each-ref.txt\nindex e89b9b0..6b6eb93 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -128,11 +128,12 @@ color::\n are described in `color.branch.*`.\n\n align::\n- Align succeeding atoms to the right, left or middle. Followed\n- by `:,`, where the `` is either\n- left, right or middle and `` is the total\n- length of the padding to be performed. If the atom length is\n- more than the padding length then no padding is performed.\n+ Align any string with or without %(atom) before the %(end)\n+ atom to the right, left or middle. Followed by\n+ `:,`, where the `` is either left,\n+ right or middle and `` is the total length of\n+ the padding to be performed. If the string length is more than\n+ the padding length then no padding is performed.\n\n In addition to the above, for commit and tag objects, the header\n field names (`tree`, `parent`, `object`, `type`, and `tag`) can\ndiff --git a/ref-filter.c b/ref-filter.c\nindex afeab37..de84dd4 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -57,6 +57,7 @@ static struct {\n { \"HEAD\" },\n { \"color\" },\n { \"align\" },\n+ { \"end\" },\n };\n\n /*\n@@ -708,6 +709,11 @@ static void populate_value(struct ref_array_item *ref)\n if (strtoul_ui(valp, 10, &align->align_value))\n die(_(\"align: positive value expected\"));\n v->align = align;\n+ v->modifier_atom = 1;\n+ continue;\n+ } else if (starts_with(name, \"end\")) {\n+ v->end = 1;\n+ v->modifier_atom = 1;\n continue;\n } else\n continue;\n@@ -1247,24 +1253,23 @@ void ref_array_sort(struct ref_sorting\n*sorting, struct ref_array *array)\n qsort(array->items, array->nr, sizeof(struct ref_array_item\n*), compare_refs);\n }\n\n-static void print_value(struct atom_value *v, struct\nref_formatting_state *state,\n- struct strbuf *output)\n+static void print_value(struct atom_value *v, struct\nref_formatting_state *state)\n {\n switch (state->quote_style) {\n case QUOTE_NONE:\n- strbuf_addstr(output, v->s);\n+ strbuf_addstr(state->output, v->s);\n break;\n case QUOTE_SHELL:\n- sq_quote_buf(output, v->s);\n+ sq_quote_buf(state->output, v->s);\n break;\n case QUOTE_PERL:\n- perl_quote_buf(output, v->s);\n+ perl_quote_buf(state->output, v->s);\n break;\n case QUOTE_PYTHON:\n- python_quote_buf(output, v->s);\n+ python_quote_buf(state->output, v->s);\n break;\n case QUOTE_TCL:\n- tcl_quote_buf(output, v->s);\n+ tcl_quote_buf(state->output, v->s);\n break;\n }\n }\n@@ -1287,7 +1292,7 @@ static int hex2(const char *cp)\n return -1;\n }\n\n-static void emit(const char *cp, const char *ep, struct strbuf *output)\n+static void emit(const char *cp, const char *ep, struct\nref_formatting_state *state)\n {\n while (*cp && (!ep || cp < ep)) {\n if (*cp == '%') {\n@@ -1296,13 +1301,13 @@ static void emit(const char *cp, const char\n*ep, struct strbuf *output)\n else {\n int ch = hex2(cp + 1);\n if (0 <= ch) {\n- strbuf_addch(output, ch);\n+ strbuf_addch(state->output, ch);\n cp += 3;\n continue;\n }\n }\n }\n- strbuf_addch(output, *cp);\n+ strbuf_addch(state->output, *cp);\n cp++;\n }\n }\n@@ -1313,12 +1318,14 @@ static void process_formatting_state(struct\natom_value *atomv, struct ref_format\n state->align = atomv->align;\n atomv->align = NULL;\n }\n+ if (atomv->end)\n+ state->end = 1;\n }\n\n-static void apply_formatting_state(struct ref_formatting_state\n*state, struct strbuf *value,\n- struct strbuf *format)\n+static void apply_formatting_state(struct ref_formatting_state\n*state, struct strbuf *final)\n {\n- if (state->align) {\n+ if (state->align && state->end) {\n+ struct strbuf *value = state->output;\n int len = 0, buf_len = value->len;\n struct align *align = state->align;\n\n@@ -1331,26 +1338,26 @@ static void apply_formatting_state(struct\nref_formatting_state *state, struct st\n\n if (align->align_value < buf_len) {\n state->align = NULL;\n- strbuf_addbuf(format, value);\n+ strbuf_addbuf(final, value);\n strbuf_release(value);\n return;\n }\n\n if (align->align_type == ALIGN_LEFT)\n- strbuf_addf(format, \"%-*s\", len +\nalign->align_value, value->buf);\n+ strbuf_addf(final, \"%-*s\", len +\nalign->align_value, value->buf);\n else if (align->align_type == ALIGN_MIDDLE) {\n int right = (align->align_value - buf_len)/2;\n- strbuf_addf(format, \"%*s%-*s\",\nalign->align_value - right + len,\n+ strbuf_addf(final, \"%*s%-*s\",\nalign->align_value - right + len,\n value->buf, right, \"\");\n } else if (align->align_type == ALIGN_RIGHT)\n- strbuf_addf(format, \"%*s\", align->align_value,\nvalue->buf);\n+ strbuf_addf(final, \"%*s\", align->align_value,\nvalue->buf);\n strbuf_release(value);\n state->align = NULL;\n return;\n- }\n- strbuf_addbuf(format, value);\n- strbuf_release(value);\n-\n+ } else if (state->align)\n+ return;\n+ strbuf_addbuf(final, state->output);\n+ strbuf_release(state->output);\n }\n\n /*\n@@ -1407,27 +1414,34 @@ void show_ref_array_item(struct ref_array_item\n*info, const char *format,\n\n memset(&state, 0, sizeof(state));\n state.quote_style = quote_style;\n+ state.output = &value;\n\n for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {\n struct atom_value *atomv = NULL;\n\n ep = strchr(sp, ')');\n if (cp < sp) {\n- emit(cp, sp, &value);\n- apply_formatting_state(&state, &value, &final_buf);\n+ emit(cp, sp, &state);\n+ apply_formatting_state(&state, &final_buf);\n }\n get_ref_atom_value(info, parse_ref_filter_atom(sp + 2,\nep), &atomv);\n- if (atomv->align)\n+ if (atomv->modifier_atom)\n process_formatting_state(atomv, &state);\n else {\n- print_value(atomv, &state, &value);\n- apply_formatting_state(&state, &value, &final_buf);\n+ print_value(atomv, &state);\n+ apply_formatting_state(&state, &final_buf);\n+ }\n+ if (atomv->end) {\n+ print_value(atomv, &state);\n+ apply_formatting_state(&state, &final_buf);\n }\n }\n+ if (state.align)\n+ die(_(\"format: align used without an `end` atom\"));\n if (*cp) {\n sp = cp + strlen(cp);\n- emit(cp, sp, &value);\n- apply_formatting_state(&state, &value, &final_buf);\n+ emit(cp, sp, &state);\n+ apply_formatting_state(&state, &final_buf);\n }\n if (need_color_reset_at_eol) {\n struct atom_value resetv;\n@@ -1436,8 +1450,8 @@ void show_ref_array_item(struct ref_array_item\n*info, const char *format,\n if (color_parse(\"reset\", color) < 0)\n die(\"BUG: couldn't parse 'reset' as a color\");\n resetv.s = color;\n- print_value(&resetv, &state, &value);\n- apply_formatting_state(&state, &value, &final_buf);\n+ print_value(&resetv, &state);\n+ apply_formatting_state(&state, &final_buf);\n }\n\n for (i = 0; i < final_buf.len; i++)\ndiff --git a/ref-filter.h b/ref-filter.h\nindex b3b9cd8..5be3e35 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -23,8 +23,9 @@\n\n struct ref_formatting_state {\n int quote_style;\n+ struct strbuf *output;\n struct align *align;\n- struct strbuf *sb;\n+ unsigned int end : 1;\n };\nstruct align {\n@@ -36,6 +37,8 @@ struct atom_value {\n const char *s;\n struct align *align;\n unsigned long ul; /* used for sorting when not FIELD_STR */\n+ unsigned int modifier_atom : 1,\n+ end : 1;\n };\n\n struct ref_sorting {\n\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 7332bea..272a7e4 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -83,81 +83,49 @@ test_expect_success 'filtering with --contains' '\n\n test_expect_success 'left alignment' '\n cat >expect <<-\\EOF &&\n- refs/heads/master |refs/heads/master\n- refs/heads/side |refs/heads/side\n- refs/odd/spot |refs/odd/spot\n- refs/tags/double-tag|refs/tags/double-tag\n- refs/tags/four |refs/tags/four\n- refs/tags/one |refs/tags/one\n- refs/tags/signed-tag|refs/tags/signed-tag\n- refs/tags/three |refs/tags/three\n- refs/tags/two |refs/tags/two\n+ refname is refs/heads/master |refs/heads/master\n+ refname is refs/heads/side |refs/heads/side\n+ refname is refs/odd/spot |refs/odd/spot\n+ refname is refs/tags/double-tag|refs/tags/double-tag\n+ refname is refs/tags/four |refs/tags/four\n+ refname is refs/tags/one |refs/tags/one\n+ refname is refs/tags/signed-tag|refs/tags/signed-tag\n+ refname is refs/tags/three |refs/tags/three\n+ refname is refs/tags/two |refs/tags/two\n EOF\n- git for-each-ref\n--format=\"%(align:left,20)%(refname)|%(refname)\" >actual &&\n+ git for-each-ref --format=\"%(align:left,30)refname is\n%(refname)%(end)|%(refname)\" >actual &&\n test_cmp expect actual\n '\n\n test_expect_success 'middle alignment' '\n\n cat >expect <<-\\EOF &&\n- | refs/heads/master |refs/heads/master\n- | refs/heads/side |refs/heads/side\n- | refs/odd/spot |refs/odd/spot\n- |refs/tags/double-tag|refs/tags/double-tag\n- | refs/tags/four |refs/tags/four\n- | refs/tags/one |refs/tags/one\n- |refs/tags/signed-tag|refs/tags/signed-tag\n- | refs/tags/three |refs/tags/three\n- | refs/tags/two |refs/tags/two\n+ | refname is refs/heads/master |refs/heads/master\n+ | refname is refs/heads/side |refs/heads/side\n+ | refname is refs/odd/spot |refs/odd/spot\n+ |refname is refs/tags/double-tag|refs/tags/double-tag\n+ | refname is refs/tags/four |refs/tags/four\n+ | refname is refs/tags/one |refs/tags/one\n+ |refname is refs/tags/signed-tag|refs/tags/signed-tag\n+ | refname is refs/tags/three |refs/tags/three\n+ | refname is refs/tags/two |refs/tags/two\n EOF\n- git for-each-ref\n--format=\"|%(align:middle,20)%(refname)|%(refname)\" >actual &&\n+ git for-each-ref --format=\"|%(align:middle,30)refname is\n%(refname)%(end)|%(refname)\" >actual &&\n test_cmp expect actual\n '\n\n test_expect_success 'right alignment' '\n cat >expect <<-\\EOF &&\n- | refs/heads/master|refs/heads/master\n- | refs/heads/side|refs/heads/side\n- | refs/odd/spot|refs/odd/spot\n- |refs/tags/double-tag|refs/tags/double-tag\n- | refs/tags/four|refs/tags/four\n- | refs/tags/one|refs/tags/one\n- |refs/tags/signed-tag|refs/tags/signed-tag\n- | refs/tags/three|refs/tags/three\n- | refs/tags/two|refs/tags/two\n+ | refname is refs/heads/master|refs/heads/master\n+ | refname is refs/heads/side|refs/heads/side\n+ | refname is refs/odd/spot|refs/odd/spot\n+ |refname is refs/tags/double-tag|refs/tags/double-tag\n+ | refname is refs/tags/four|refs/tags/four\n+ | refname is refs/tags/one|refs/tags/one\n+ |refname is refs/tags/signed-tag|refs/tags/signed-tag\n+ | refname is refs/tags/three|refs/tags/three\n+ | refname is refs/tags/two|refs/tags/two\n EOF\n- git for-each-ref\n--format=\"|%(align:right,20)%(refname)|%(refname)\" >actual &&\n- test_cmp expect actual\n-'\n-\n-test_expect_success 'alignment value lesser than atom value' '\n- cat >expect <<-\\EOF &&\n- |refs/heads/master|\n- |refs/heads/side|\n- | refs/odd/spot|\n- |refs/tags/double-tag|\n- | refs/tags/four|\n- | refs/tags/one|\n- |refs/tags/signed-tag|\n- |refs/tags/three|\n- | refs/tags/two|\n- EOF\n- git for-each-ref --format=\"|%(align:right,15)%(refname)|\" >actual &&\n- test_cmp expect actual\n-'\n-\n-test_expect_success 'non atom alignment' '\n- cat >expect <<-\\EOF &&\n- | |master | refs/heads/master| refs/heads/master\n- | |side | refs/heads/side| refs/heads/side\n- | |odd/spot | refs/odd/spot| refs/odd/spot\n- | |double-tag | refs/tags/double-tag| refs/tags/double-tag\n- | |four | refs/tags/four| refs/tags/four\n- | |one | refs/tags/one| refs/tags/one\n- | |signed-tag | refs/tags/signed-tag| refs/tags/signed-tag\n- | |three | refs/tags/three| refs/tags/three\n- | |two | refs/tags/two| refs/tags/two\n- EOF\n- git for-each-ref\n--format=\"|%(align:right,5)|%(refname:short)%(align:middle,5)|%(refname)%(align:left,3)|%(refname)\"\n>actual &&\n+ git for-each-ref --format=\"|%(align:right,30)refname is\n%(refname)%(end)|%(refname)\" >actual &&\n test_cmp expect actual\n '\n\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267397","messageId":"1438692188-14367-1-git-send-email-Karthik.188@gmail.com","threadId":"40009","inReplyTo":"CAOLa=ZSBMk9y1VGTVKSVsGMdYuPtjhPADciVUaEVwESRdSvWZg@mail.gmail.com","subject":"[PATCH v9 01/11] ref-filter: print output to strbuf for formatting","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-04T12:42:58Z","receivedAt":"2015-08-04T12:42:58Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Introduce a strbuf `output` which will act as a substitute rather than\nprinting directly to stdout. This will be used for formatting\neventually.\n\nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 36 ++++++++++++++++++------------------\n 1 file changed, 18 insertions(+), 18 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 46963a5..91482c9 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1190,30 +1190,25 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)\n \tqsort(array->items, array->nr, sizeof(struct ref_array_item *), compare_refs);\n }\n \n-static void print_value(struct atom_value *v, int quote_style)\n+static void print_value(struct atom_value *v, int quote_style, struct strbuf *output)\n {\n-\tstruct strbuf sb = STRBUF_INIT;\n \tswitch (quote_style) {\n \tcase QUOTE_NONE:\n-\t\tfputs(v->s, stdout);\n+\t\tstrbuf_addstr(output, v->s);\n \t\tbreak;\n \tcase QUOTE_SHELL:\n-\t\tsq_quote_buf(&sb, v->s);\n+\t\tsq_quote_buf(output, v->s);\n \t\tbreak;\n \tcase QUOTE_PERL:\n-\t\tperl_quote_buf(&sb, v->s);\n+\t\tperl_quote_buf(output, v->s);\n \t\tbreak;\n \tcase QUOTE_PYTHON:\n-\t\tpython_quote_buf(&sb, v->s);\n+\t\tpython_quote_buf(output, v->s);\n \t\tbreak;\n \tcase QUOTE_TCL:\n-\t\ttcl_quote_buf(&sb, v->s);\n+\t\ttcl_quote_buf(output, v->s);\n \t\tbreak;\n \t}\n-\tif (quote_style != QUOTE_NONE) {\n-\t\tfputs(sb.buf, stdout);\n-\t\tstrbuf_release(&sb);\n-\t}\n }\n \n static int hex1(char ch)\n@@ -1234,7 +1229,7 @@ static int hex2(const char *cp)\n \t\treturn -1;\n }\n \n-static void emit(const char *cp, const char *ep)\n+static void emit(const char *cp, const char *ep, struct strbuf *output)\n {\n \twhile (*cp && (!ep || cp < ep)) {\n \t\tif (*cp == '%') {\n@@ -1243,13 +1238,13 @@ static void emit(const char *cp, const char *ep)\n \t\t\telse {\n \t\t\t\tint ch = hex2(cp + 1);\n \t\t\t\tif (0 <= ch) {\n-\t\t\t\t\tputchar(ch);\n+\t\t\t\t\tstrbuf_addch(output, ch);\n \t\t\t\t\tcp += 3;\n \t\t\t\t\tcontinue;\n \t\t\t\t}\n \t\t\t}\n \t\t}\n-\t\tputchar(*cp);\n+\t\tstrbuf_addch(output, *cp);\n \t\tcp++;\n \t}\n }\n@@ -1257,19 +1252,21 @@ static void emit(const char *cp, const char *ep)\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+\tstruct strbuf output = STRBUF_INIT;\n+\tint i;\n \n \tfor (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {\n \t\tstruct atom_value *atomv;\n \n \t\tep = strchr(sp, ')');\n \t\tif (cp < sp)\n-\t\t\temit(cp, sp);\n+\t\t\temit(cp, sp, &output);\n \t\tget_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);\n-\t\tprint_value(atomv, quote_style);\n+\t\tprint_value(atomv, quote_style, &output);\n \t}\n \tif (*cp) {\n \t\tsp = cp + strlen(cp);\n-\t\temit(cp, sp);\n+\t\temit(cp, sp, &output);\n \t}\n \tif (need_color_reset_at_eol) {\n \t\tstruct atom_value resetv;\n@@ -1278,9 +1275,12 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n \t\tif (color_parse(\"reset\", color) < 0)\n \t\t\tdie(\"BUG: couldn't parse 'reset' as a color\");\n \t\tresetv.s = color;\n-\t\tprint_value(&resetv, quote_style);\n+\t\tprint_value(&resetv, quote_style, &output);\n \t}\n+\tfor (i = 0; i < output.len; i++)\n+\t\tprintf(\"%c\", output.buf[i]);\n \tputchar('\\n');\n+\tstrbuf_release(&output);\n }\n \n /* If no sorting option is given, use refname to sort as default */\n-- \n2.5.0\n"},{"id":"267398","messageId":"1438692188-14367-2-git-send-email-Karthik.188@gmail.com","threadId":"40009","inReplyTo":"1438692188-14367-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v9 02/11] ref-filter: introduce ref_formatting_state","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-04T12:42:59Z","receivedAt":"2015-08-04T12:42:59Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Introduce a ref_formatting_state which will eventually hold the values\nof modifier atoms. Implement this within ref-filter.\n\nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 64 +++++++++++++++++++++++++++++++++++++++++-------------------\n ref-filter.h | 5 +++++\n 2 files changed, 49 insertions(+), 20 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 91482c9..2c074a1 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1190,23 +1190,23 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)\n \tqsort(array->items, array->nr, sizeof(struct ref_array_item *), compare_refs);\n }\n \n-static void print_value(struct atom_value *v, int quote_style, struct strbuf *output)\n+static void print_value(struct atom_value *v, struct ref_formatting_state *state)\n {\n-\tswitch (quote_style) {\n+\tswitch (state->quote_style) {\n \tcase QUOTE_NONE:\n-\t\tstrbuf_addstr(output, v->s);\n+\t\tstrbuf_addstr(state->output, v->s);\n \t\tbreak;\n \tcase QUOTE_SHELL:\n-\t\tsq_quote_buf(output, v->s);\n+\t\tsq_quote_buf(state->output, v->s);\n \t\tbreak;\n \tcase QUOTE_PERL:\n-\t\tperl_quote_buf(output, v->s);\n+\t\tperl_quote_buf(state->output, v->s);\n \t\tbreak;\n \tcase QUOTE_PYTHON:\n-\t\tpython_quote_buf(output, v->s);\n+\t\tpython_quote_buf(state->output, v->s);\n \t\tbreak;\n \tcase QUOTE_TCL:\n-\t\ttcl_quote_buf(output, v->s);\n+\t\ttcl_quote_buf(state->output, v->s);\n \t\tbreak;\n \t}\n }\n@@ -1229,7 +1229,7 @@ static int hex2(const char *cp)\n \t\treturn -1;\n }\n \n-static void emit(const char *cp, const char *ep, struct strbuf *output)\n+static void emit(const char *cp, const char *ep, struct ref_formatting_state *state)\n {\n \twhile (*cp && (!ep || cp < ep)) {\n \t\tif (*cp == '%') {\n@@ -1238,35 +1238,58 @@ static void emit(const char *cp, const char *ep, struct strbuf *output)\n \t\t\telse {\n \t\t\t\tint ch = hex2(cp + 1);\n \t\t\t\tif (0 <= ch) {\n-\t\t\t\t\tstrbuf_addch(output, ch);\n+\t\t\t\t\tstrbuf_addch(state->output, ch);\n \t\t\t\t\tcp += 3;\n \t\t\t\t\tcontinue;\n \t\t\t\t}\n \t\t\t}\n \t\t}\n-\t\tstrbuf_addch(output, *cp);\n+\t\tstrbuf_addch(state->output, *cp);\n \t\tcp++;\n \t}\n }\n \n+static void process_formatting_state(struct atom_value *atomv, struct ref_formatting_state *state)\n+{\n+\t/* Based on the atomv values, the formatting state is set */\n+}\n+\n+static void apply_formatting_state(struct ref_formatting_state *state, struct strbuf *final)\n+{\n+\t/* More formatting options to be evetually added */\n+\tstrbuf_addbuf(final, state->output);\n+\tstrbuf_release(state->output);\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-\tstruct strbuf output = STRBUF_INIT;\n+\tstruct strbuf value = STRBUF_INIT;\n+\tstruct strbuf final_buf = STRBUF_INIT;\n+\tstruct ref_formatting_state state;\n \tint i;\n \n+\tmemset(&state, 0, sizeof(state));\n+\tstate.quote_style = quote_style;\n+\tstate.output = &value;\n+\n \tfor (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {\n-\t\tstruct atom_value *atomv;\n+\t\tstruct atom_value *atomv = NULL;\n \n \t\tep = strchr(sp, ')');\n-\t\tif (cp < sp)\n-\t\t\temit(cp, sp, &output);\n+\t\tif (cp < sp) {\n+\t\t\temit(cp, sp, &state);\n+\t\t\tapply_formatting_state(&state, &final_buf);\n+\t\t}\n \t\tget_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);\n-\t\tprint_value(atomv, quote_style, &output);\n+\t\tprocess_formatting_state(atomv, &state);\n+\t\tprint_value(atomv, &state);\n+\t\tapply_formatting_state(&state, &final_buf);\n \t}\n \tif (*cp) {\n \t\tsp = cp + strlen(cp);\n-\t\temit(cp, sp, &output);\n+\t\temit(cp, sp, &state);\n+\t\tapply_formatting_state(&state, &final_buf);\n \t}\n \tif (need_color_reset_at_eol) {\n \t\tstruct atom_value resetv;\n@@ -1275,12 +1298,13 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n \t\tif (color_parse(\"reset\", color) < 0)\n \t\t\tdie(\"BUG: couldn't parse 'reset' as a color\");\n \t\tresetv.s = color;\n-\t\tprint_value(&resetv, quote_style, &output);\n+\t\tprint_value(&resetv, &state);\n+\t\tapply_formatting_state(&state, &final_buf);\n \t}\n-\tfor (i = 0; i < output.len; i++)\n-\t\tprintf(\"%c\", output.buf[i]);\n+\tfor (i = 0; i < final_buf.len; i++)\n+\t\tprintf(\"%c\", final_buf.buf[i]);\n \tputchar('\\n');\n-\tstrbuf_release(&output);\n+\tstrbuf_release(&final_buf);\n }\n \n /* If no sorting option is given, use refname to sort as default */\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 6bf27d8..9e6c2d4 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -16,6 +16,11 @@\n #define FILTER_REFS_INCLUDE_BROKEN 0x1\n #define FILTER_REFS_ALL 0x2\n \n+struct ref_formatting_state {\n+\tint quote_style;\n+\tstruct strbuf *output;\n+};\n+\n struct atom_value {\n \tconst char *s;\n \tunsigned long ul; /* used for sorting when not FIELD_STR */\n-- \n2.5.0\n"},{"id":"267399","messageId":"1438692188-14367-3-git-send-email-Karthik.188@gmail.com","threadId":"40009","inReplyTo":"1438692188-14367-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v9 03/11] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-04T12:43:00Z","receivedAt":"2015-08-04T12:43:00Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Implement an `align` atom which will act as a modifier atom and align\nany string with or without an %(atom) appearing before a %(end) atom\nto the right, left or middle.\n\nIt is followed by `:,`, where the `` is\neither left, right or middle and `` is the total length\nof the padding to be performed. If the atom length is more than the\npadding length then no padding is performed. e.g. to pad a succeeding\natom to the middle with a total padding size of 40 we can do a\n--format=\"%(align:middle,40)..\"\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 | 8 ++++\n ref-filter.c | 84 +++++++++++++++++++++++++++++++++++---\n ref-filter.h | 14 +++++++\n t/t6302-for-each-ref-filter.sh | 48 ++++++++++++++++++++++\n 4 files changed, 148 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex e49d578..d865f98 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -127,6 +127,14 @@ color::\n \tChange output color. Followed by `:`, where names\n \tare described in `color.branch.*`.\n \n+align::\n+\tAlign any string with or without %(atom) before the %(end)\n+\tatom to the right, left or middle. Followed by\n+\t`:,`, where the `` is either left,\n+\tright or middle and `` is the total length of\n+\tthe padding to be performed. If the string length is more than\n+\tthe padding length then no padding is performed.\n+\n In addition to the above, for commit and tag objects, the header\n field names (`tree`, `parent`, `object`, `type`, and `tag`) can\n be used to specify the value in the header field.\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 2c074a1..d123299 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -10,6 +10,7 @@\n #include \"quote.h\"\n #include \"ref-filter.h\"\n #include \"revision.h\"\n+#include \"utf8.h\"\n \n typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n \n@@ -53,6 +54,8 @@ static struct {\n \t{ \"flag\" },\n \t{ \"HEAD\" },\n \t{ \"color\" },\n+\t{ \"align\" },\n+\t{ \"end\" },\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,29 @@ 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+\t\t\tstruct align *align = xmalloc(sizeof(struct align));\n+\n+\t\t\tskip_prefix(name, \"align:\", &valp);\n+\n+\t\t\tif (skip_prefix(valp, \"left,\", &valp))\n+\t\t\t\talign->align_type = ALIGN_LEFT;\n+\t\t\telse if (skip_prefix(valp, \"right,\", &valp))\n+\t\t\t\talign->align_type = ALIGN_RIGHT;\n+\t\t\telse if (skip_prefix(valp, \"middle,\", &valp))\n+\t\t\t\talign->align_type = ALIGN_MIDDLE;\n+\t\t\telse\n+\t\t\t\tdie(_(\"align: improper format\"));\n+\t\t\tif (strtoul_ui(valp, 10, &align->align_value))\n+\t\t\t\tdie(_(\"align: positive value expected\"));\n+\t\t\tv->align = align;\n+\t\t\tv->modifier_atom = 1;\n+\t\t\tcontinue;\n+\t\t} else if (starts_with(name, \"end\")) {\n+\t\t\tv->end = 1;\n+\t\t\tv->modifier_atom = 1;\n+\t\t\tcontinue;\n \t\t} else\n \t\t\tcontinue;\n \n@@ -1251,12 +1277,48 @@ static void emit(const char *cp, const char *ep, struct ref_formatting_state *st\n \n static void process_formatting_state(struct atom_value *atomv, struct ref_formatting_state *state)\n {\n-\t/* Based on the atomv values, the formatting state is set */\n+\tif (atomv->align) {\n+\t\tstate->align = atomv->align;\n+\t\tatomv->align = NULL;\n+\t}\n+\tif (atomv->end)\n+\t\tstate->end = 1;\n }\n \n static void apply_formatting_state(struct ref_formatting_state *state, struct strbuf *final)\n {\n-\t/* More formatting options to be evetually added */\n+\tif (state->align && state->end) {\n+\t\tstruct strbuf *value = state->output;\n+\t\tint len = 0, buf_len = value->len;\n+\t\tstruct align *align = state->align;\n+\n+\t\tif (!value->buf)\n+\t\t\treturn;\n+\t\tif (!is_utf8(value->buf)) {\n+\t\t\tlen = value->len - utf8_strwidth(value->buf);\n+\t\t\tbuf_len -= len;\n+\t\t}\n+\n+\t\tif (align->align_value < buf_len) {\n+\t\t\tstate->align = NULL;\n+\t\t\tstrbuf_addbuf(final, value);\n+\t\t\tstrbuf_release(value);\n+\t\t\treturn;\n+\t\t}\n+\n+\t\tif (align->align_type == ALIGN_LEFT)\n+\t\t\tstrbuf_addf(final, \"%-*s\", len + align->align_value, value->buf);\n+\t\telse if (align->align_type == ALIGN_MIDDLE) {\n+\t\t\tint right = (align->align_value - buf_len)/2;\n+\t\t\tstrbuf_addf(final, \"%*s%-*s\", align->align_value - right + len,\n+\t\t\t\t value->buf, right, \"\");\n+\t\t} else if (align->align_type == ALIGN_RIGHT)\n+\t\t\tstrbuf_addf(final, \"%*s\", align->align_value, value->buf);\n+\t\tstrbuf_release(value);\n+\t\tstate->align = NULL;\n+\t\treturn;\n+\t} else if (state->align)\n+\t\treturn;\n \tstrbuf_addbuf(final, state->output);\n \tstrbuf_release(state->output);\n }\n@@ -1282,10 +1344,19 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n \t\t\tapply_formatting_state(&state, &final_buf);\n \t\t}\n \t\tget_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);\n-\t\tprocess_formatting_state(atomv, &state);\n-\t\tprint_value(atomv, &state);\n-\t\tapply_formatting_state(&state, &final_buf);\n+\t\tif (atomv->modifier_atom)\n+\t\t\tprocess_formatting_state(atomv, &state);\n+\t\telse {\n+\t\t\tprint_value(atomv, &state);\n+\t\t\tapply_formatting_state(&state, &final_buf);\n+\t\t}\n+\t\tif (atomv->end) {\n+\t\t\tprint_value(atomv, &state);\n+\t\t\tapply_formatting_state(&state, &final_buf);\n+\t\t}\n \t}\n+\tif (state.align)\n+\t\tdie(_(\"format: align used without an `end` atom\"));\n \tif (*cp) {\n \t\tsp = cp + strlen(cp);\n \t\temit(cp, sp, &state);\n@@ -1301,6 +1372,7 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n \t\tprint_value(&resetv, &state);\n \t\tapply_formatting_state(&state, &final_buf);\n \t}\n+\n \tfor (i = 0; i < final_buf.len; i++)\n \t\tprintf(\"%c\", final_buf.buf[i]);\n \tputchar('\\n');\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 9e6c2d4..81de1e2 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -16,14 +16,28 @@\n #define FILTER_REFS_INCLUDE_BROKEN 0x1\n #define FILTER_REFS_ALL 0x2\n \n+#define ALIGN_LEFT 0x01\n+#define ALIGN_RIGHT 0x02\n+#define ALIGN_MIDDLE 0x04\n+\n struct ref_formatting_state {\n \tint quote_style;\n \tstruct strbuf *output;\n+\tstruct align *align;\n+\tunsigned int end : 1;\n+};\n+\n+struct align {\n+\tunsigned int align_type,\n+\t\talign_value;\n };\n \n struct atom_value {\n \tconst char *s;\n+\tstruct align *align;\n \tunsigned long ul; /* used for sorting when not FIELD_STR */\n+\tunsigned int modifier_atom : 1,\n+\t\tend : 1;\n };\n \n struct ref_sorting {\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 505a360..76041a2 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -81,4 +81,52 @@ test_expect_success 'filtering with --contains' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'left alignment' '\n+\tcat >expect <<-\\EOF &&\n+\trefname is refs/heads/master |refs/heads/master\n+\trefname is refs/heads/side |refs/heads/side\n+\trefname is refs/odd/spot |refs/odd/spot\n+\trefname is refs/tags/double-tag|refs/tags/double-tag\n+\trefname is refs/tags/four |refs/tags/four\n+\trefname is refs/tags/one |refs/tags/one\n+\trefname is refs/tags/signed-tag|refs/tags/signed-tag\n+\trefname is refs/tags/three |refs/tags/three\n+\trefname is refs/tags/two |refs/tags/two\n+\tEOF\n+\tgit for-each-ref --format=\"%(align:left,30)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'middle alignment' '\n+\tcat >expect <<-\\EOF &&\n+\t| refname is refs/heads/master |refs/heads/master\n+\t| refname is refs/heads/side |refs/heads/side\n+\t| refname is refs/odd/spot |refs/odd/spot\n+\t|refname is refs/tags/double-tag|refs/tags/double-tag\n+\t| refname is refs/tags/four |refs/tags/four\n+\t| refname is refs/tags/one |refs/tags/one\n+\t|refname is refs/tags/signed-tag|refs/tags/signed-tag\n+\t| refname is refs/tags/three |refs/tags/three\n+\t| refname is refs/tags/two |refs/tags/two\n+\tEOF\n+\tgit for-each-ref --format=\"|%(align:middle,30)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'right alignment' '\n+\tcat >expect <<-\\EOF &&\n+\t| refname is refs/heads/master|refs/heads/master\n+\t| refname is refs/heads/side|refs/heads/side\n+\t| refname is refs/odd/spot|refs/odd/spot\n+\t|refname is refs/tags/double-tag|refs/tags/double-tag\n+\t| refname is refs/tags/four|refs/tags/four\n+\t| refname is refs/tags/one|refs/tags/one\n+\t|refname is refs/tags/signed-tag|refs/tags/signed-tag\n+\t| refname is refs/tags/three|refs/tags/three\n+\t| refname is refs/tags/two|refs/tags/two\n+\tEOF\n+\tgit for-each-ref --format=\"|%(align:right,30)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.5.0\n"},{"id":"267400","messageId":"1438692188-14367-4-git-send-email-Karthik.188@gmail.com","threadId":"40009","inReplyTo":"1438692188-14367-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v9 04/11] ref-filter: add option to filter only tags","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-04T12:43:01Z","receivedAt":"2015-08-04T12:43:01Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"From: Karthik Nayak \n\nAdd a functions called 'for_each_tag_ref_fullpath()' to refs.{c,h}\nwhich iterates through each tag ref without trimming the path.\n\nAdd an option in 'filter_refs()' to use 'for_each_tag_ref_fullpath()'\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 refs.c | 5 +++++\n refs.h | 1 +\n 4 files changed, 9 insertions(+)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex d123299..7c9c51d 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1161,6 +1161,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_fullpath(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 81de1e2..3412dce 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 #define ALIGN_LEFT 0x01\n #define ALIGN_RIGHT 0x02\ndiff --git a/refs.c b/refs.c\nindex 2db2975..0103a88 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2114,6 +2114,11 @@ int for_each_tag_ref(each_ref_fn fn, void *cb_data)\n \treturn for_each_ref_in(\"refs/tags/\", fn, cb_data);\n }\n \n+int for_each_tag_ref_fullpath(each_ref_fn fn, void *cb_data)\n+{\n+\treturn do_for_each_ref(&ref_cache, \"refs/tags/\", fn, 0, 0, cb_data);\n+}\n+\n int for_each_tag_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)\n {\n \treturn for_each_ref_in_submodule(submodule, \"refs/tags/\", fn, cb_data);\ndiff --git a/refs.h b/refs.h\nindex 6a3fa6d..0956255 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -174,6 +174,7 @@ extern int head_ref(each_ref_fn fn, void *cb_data);\n extern int for_each_ref(each_ref_fn fn, void *cb_data);\n extern int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data);\n extern int for_each_tag_ref(each_ref_fn fn, void *cb_data);\n+extern int for_each_tag_ref_fullpath(each_ref_fn fn, void *cb_data);\n extern int for_each_branch_ref(each_ref_fn fn, void *cb_data);\n extern int for_each_remote_ref(each_ref_fn fn, void *cb_data);\n extern int for_each_replace_ref(each_ref_fn fn, void *cb_data);\n-- \n2.5.0\n"},{"id":"267402","messageId":"1438692188-14367-5-git-send-email-Karthik.188@gmail.com","threadId":"40009","inReplyTo":"1438692188-14367-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v9 05/11] ref-filter: support printing N lines from tag annotation","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-04T12:43:02Z","receivedAt":"2015-08-04T12:43:02Z","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 | 54 ++++++++++++++++++++++++++++++++++++++++++++++++--\n ref-filter.h | 9 +++++++--\n 4 files changed, 64 insertions(+), 5 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 471d6b1..0fc7557 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 7c9c51d..1609b01 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1325,7 +1325,51 @@ static void apply_formatting_state(struct ref_formatting_state *state, struct st\n \tstrbuf_release(state->output);\n }\n \n-void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)\n+/*\n+ * If 'lines' is greater than 0, print that many lines from the given\n+ * object_id 'oid'.\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+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 \tstruct strbuf value = STRBUF_INIT;\n@@ -1377,8 +1421,14 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n \n \tfor (i = 0; i < final_buf.len; i++)\n \t\tprintf(\"%c\", final_buf.buf[i]);\n-\tputchar('\\n');\n \tstrbuf_release(&final_buf);\n+\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 \n /* If no sorting option is given, use refname to sort as default */\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 3412dce..2f571ed 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -74,6 +74,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@@ -105,8 +106,12 @@ 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+ * print that many lines of the the given ref.\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 /* 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.5.0\n"},{"id":"267401","messageId":"1438692188-14367-6-git-send-email-Karthik.188@gmail.com","threadId":"40009","inReplyTo":"1438692188-14367-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v9 06/11] ref-filter: add support to sort by version","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-04T12:43:03Z","receivedAt":"2015-08-04T12:43:03Z","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 'versioncmp()'\nfunction 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 | 3 +++\n ref-filter.c | 15 ++++++++++-----\n ref-filter.h | 3 ++-\n t/t6302-for-each-ref-filter.sh | 36 ++++++++++++++++++++++++++++++++++++\n 4 files changed, 51 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex d865f98..6b6eb93 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -153,6 +153,9 @@ 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 \n+There is also an option to sort by versions, this can be done by using\n+the fieldname `version:refname` or its alias `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\n returns an empty string instead.\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 1609b01..d244a81 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -11,6 +11,8 @@\n #include \"ref-filter.h\"\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@@ -1181,19 +1183,19 @@ 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+\tif (s->version)\n+\t\tcmp = versioncmp(va->s, vb->s);\n+\telse if (cmp_type == FIELD_STR)\n \t\tcmp = strcmp(va->s, vb->s);\n-\t\tbreak;\n-\tdefault:\n+\telse {\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 \t}\n+\n \treturn (s->reverse) ? -cmp : cmp;\n }\n \n@@ -1460,6 +1462,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 2f571ed..449b0d9 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -44,7 +44,8 @@ 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,\n+\t\tversion : 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 76041a2..272a7e4 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -129,4 +129,40 @@ test_expect_success 'right alignment' '\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 for-each-ref --sort=version:refname --format=\"%(refname:short)\" refs/tags/ | 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 'version sort (shortened)' '\n+\tgit for-each-ref --sort=v:refname --format=\"%(refname:short)\" refs/tags/ | 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 for-each-ref --sort=-version:refname --format=\"%(refname:short)\" refs/tags/ | 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.5.0\n"},{"id":"267405","messageId":"1438692188-14367-7-git-send-email-Karthik.188@gmail.com","threadId":"40009","inReplyTo":"1438692188-14367-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v9 07/11] ref-filter: add option to match literal pattern","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-04T12:43:04Z","receivedAt":"2015-08-04T12:43:04Z","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 | 39 ++++++++++++++++++++++++++++++++++++---\n ref-filter.h | 3 ++-\n 3 files changed, 39 insertions(+), 4 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 d244a81..de84dd4 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -956,9 +956,32 @@ 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/mas\") or a wildcard (e.g. the same ref\n+ * matches \"refs/heads/mas*\", too).\n+ */\n+static int match_pattern(const char **patterns, const char *refname)\n+{\n+\t/*\n+\t * When no '--format' option is given we need to skip the prefix\n+\t * for matching refs of tags and branches.\n+\t */\n+\t(void)(skip_prefix(refname, \"refs/tags/\", &refname) ||\n+\t skip_prefix(refname, \"refs/heads/\", &refname) ||\n+\t skip_prefix(refname, \"refs/remotes/\", &refname));\n+\n+\tfor (; *patterns; patterns++) {\n+\t\tif (!wildmatch(*patterns, refname, 0, NULL))\n+\t\t\treturn 1;\n+\t}\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 a pattern \"refs/heads/\" but not \"refs/heads/m\") or a\n+ * wildcard (e.g. the same ref matches \"refs/heads/m*\", too).\n */\n static int match_name_as_path(const char **pattern, const char *refname)\n {\n@@ -979,6 +1002,16 @@ static int match_name_as_path(const char **pattern, const char *refname)\n \treturn 0;\n }\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+\tif (!*filter->name_patterns)\n+\t\treturn 1; /* No pattern always matches */\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@@ -1047,7 +1080,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 449b0d9..5be3e35 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -74,7 +74,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.5.0\n"},{"id":"267406","messageId":"1438692188-14367-8-git-send-email-Karthik.188@gmail.com","threadId":"40009","inReplyTo":"1438692188-14367-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v9 08/11] tag.c: use 'ref-filter' data structures","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-04T12:43:05Z","receivedAt":"2015-08-04T12:43:05Z","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 0fc7557..e96bae2 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,17 +579,17 @@ 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 create_reflog = 0;\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@@ -606,14 +611,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@@ -622,6 +627,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@@ -638,7 +645,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@@ -651,18 +658,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.5.0\n"},{"id":"267407","messageId":"1438692188-14367-9-git-send-email-Karthik.188@gmail.com","threadId":"40009","inReplyTo":"1438692188-14367-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v9 09/11] tag.c: use 'ref-filter' APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-04T12:43:06Z","receivedAt":"2015-08-04T12:43:06Z","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 | 342 ++++++----------------------------------------\n t/t7004-tag.sh | 8 +-\n 3 files changed, 50 insertions(+), 316 deletions(-)\n\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 84f6496..3ac4a96 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] [--create-reflog] [...]\n+\t[--column[=] | --no-column] [--create-reflog] [--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 e96bae2..829af6f 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -28,278 +28,32 @@ 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:left,16)%(refname:short)\";\n+\telse\n+\t\tformat = \"%(refname:short)\";\n+\n+\tverify_ref_format(format);\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 +120,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+\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 +147,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 +310,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@@ -587,6 +326,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@@ -613,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@@ -624,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@@ -650,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@@ -658,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 d31788c..1f066aa 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1462,13 +1462,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.5.0\n"},{"id":"267403","messageId":"1438692188-14367-10-git-send-email-Karthik.188@gmail.com","threadId":"40009","inReplyTo":"1438692188-14367-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v9 10/11] tag.c: implement '--format' option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-04T12:43:07Z","receivedAt":"2015-08-04T12:43:07Z","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 | 15 ++++++++++++++-\n builtin/tag.c | 11 +++++++----\n t/t7004-tag.sh | 16 ++++++++++++++++\n 3 files changed, 37 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 3ac4a96..75703c5 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] [--create-reflog] [--sort=] [...]\n+\t[--column[=] | --no-column] [--create-reflog] [--sort=]\n+\t[--format=] [...]\n 'git tag' -v ...\n \n DESCRIPTION\n@@ -158,6 +159,18 @@ 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 object\n+\tpointed at by a ref being shown. If `fieldname` is prefixed\n+\twith an asterisk (`*`) and the ref points at a tag object, the\n+\tvalue for the field in the object tag refers is used. When\n+\tunspecified, defaults to `%(refname:short)`. It also\n+\tinterpolates `%%` to `%`, and `%xx` where `xx` are hex digits\n+\tinterpolates to character with hex code `xx`; for example\n+\t`%00` interpolates to `\\0` (NUL), `%09` to `\\t` (TAB) and\n+\t`%0a` to `\\n` (LF). The fields are same as those in `git\n+\tfor-each-ref`.\n+\n \n CONFIGURATION\n -------------\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 829af6f..13c9579 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:left,16)%(refname:short)\";\n-\telse\n+\telse if (!format)\n \t\tformat = \"%(refname:short)\";\n \n \tverify_ref_format(format);\n@@ -327,6 +326,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 1f066aa..1809011 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1519,4 +1519,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+\trefname : refs/tags/foo1.10\n+\trefname : refs/tags/foo1.3\n+\trefname : refs/tags/foo1.6\n+\trefname : refs/tags/foo1.6-rc1\n+\trefname : refs/tags/foo1.6-rc2\n+\tEOF\n+\tgit tag -l --format=\"refname : %(refname)\" \"foo*\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.5.0\n"},{"id":"267404","messageId":"1438692188-14367-11-git-send-email-Karthik.188@gmail.com","threadId":"40009","inReplyTo":"1438692188-14367-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v9 11/11] tag.c: implement '--merged' and '--no-merged' options","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-04T12:43:08Z","receivedAt":"2015-08-04T12:43:08Z","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 | 7 ++++++-\n builtin/tag.c | 6 +++++-\n t/t7004-tag.sh | 27 +++++++++++++++++++++++++++\n 3 files changed, 38 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 75703c5..c2785d9 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] [--create-reflog] [--sort=]\n-\t[--format=] [...]\n+\t[--format=] [--[no-]merged []] [...]\n 'git tag' -v ...\n \n DESCRIPTION\n@@ -171,6 +171,11 @@ This option is only applicable when listing tags without annotation lines.\n \t`%0a` to `\\n` (LF). The fields are same as those in `git\n \tfor-each-ref`.\n \n+--[no-]merged []::\n+\tOnly list tags whose tips are reachable, or not reachable\n+\tif '--no-merged' is used, from the specified commit ('HEAD'\n+\tif not specified).\n+\n \n CONFIGURATION\n -------------\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 13c9579..529b29f 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[--[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 1809011..5b73539 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1535,4 +1535,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.5.0\n"},{"id":"267593","messageId":"CAPig+cT6uu3DqUB+5gR35JS2eigPBizo_Y48rAzFYkbyCm+=OQ@mail.gmail.com","threadId":"40009","inReplyTo":"1438692188-14367-1-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v9 01/11] ref-filter: print output to strbuf for formatting","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-06T22:21:22Z","receivedAt":"2015-08-06T22:21:22Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 4, 2015 at 8:42 AM, Karthik Nayak wrote:\n> Introduce a strbuf `output` which will act as a substitute rather than\n> printing directly to stdout. This will be used for formatting\n> eventually.\n>\n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 46963a5..91482c9 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -1278,9 +1275,12 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n> if (color_parse(\"reset\", color) < 0)\n> die(\"BUG: couldn't parse 'reset' as a color\");\n> resetv.s = color;\n> - print_value(&resetv, quote_style);\n> + print_value(&resetv, quote_style, &output);\n> }\n> + for (i = 0; i < output.len; i++)\n> + printf(\"%c\", output.buf[i]);\n\nEverything up to this point seems straightforward, however, it's not\nclear why you need to emit 'output' one character at a time. Is it\nbecause it might contain a NUL '\\0' character and therefore you can't\nuse the simpler printf(\"%s\", output.buf)?\n\nIf that's the case, then why not just use fwrite() to emit it all at once?\n\n fwrite(output.buf, output.len, 1, stdout);\n\n> putchar('\\n');\n> + strbuf_release(&output);\n> }\n>\n> /* If no sorting option is given, use refname to sort as default */\n> --\n> 2.5.0\n"},{"id":"267597","messageId":"CAPig+cQftyjKFi0Qkg_ZVEJ9A+zGSAmFtHwQ-8hCnf8xtU_PEA@mail.gmail.com","threadId":"40009","inReplyTo":"1438692188-14367-2-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v9 02/11] ref-filter: introduce ref_formatting_state","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-07T00:19:00Z","receivedAt":"2015-08-07T00:19:00Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 4, 2015 at 8:42 AM, Karthik Nayak wrote:\n> Introduce a ref_formatting_state which will eventually hold the values\n> of modifier atoms. Implement this within ref-filter.\n>\n> Signed-off-by: Karthik Nayak \n> ---\n> +static void apply_formatting_state(struct ref_formatting_state *state, struct strbuf *final)\n> +{\n> + /* More formatting options to be evetually added */\n> + strbuf_addbuf(final, state->output);\n> + strbuf_release(state->output);\n\nI guess the idea here is that you intend state->output to be re-used\nand it is convenient to \"clear\" it here rather than making that the\nresponsibility of each caller. For re-use, it is more typical to use\nstrbuf_reset() than strbuf_release() (though Junio may disagree[1]).\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/273094\n\n> +}\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> - struct strbuf output = STRBUF_INIT;\n> + struct strbuf value = STRBUF_INIT;\n> + struct strbuf final_buf = STRBUF_INIT;\n> + struct ref_formatting_state state;\n> int i;\n>\n> + memset(&state, 0, sizeof(state));\n> + state.quote_style = quote_style;\n> + state.output = &value;\n\nIt feels strange to assign a local variable reference to state.output,\nand there's no obvious reason why you should need to do so. I would\nhave instead expected ref_format_state to be declared as:\n\n struct ref_formatting_state {\n int quote_style;\n struct strbuf output;\n };\n\nand initialized as so:\n\n memset(&state, 0, sizeof(state));\n state.quote_style = quote_style;\n strbuf_init(&state.output, 0);\n\n(In fact, the memset() isn't even necessary here since you're\ninitializing all fields explicitly, though perhaps you want the\nmemset() because a future patch adds more fields which are not\ninitialized explicitly?)\n\nThis still allows re-use via strbuf_reset() mentioned above.\n\nAnd, of course, you'd want to strbuf_release() it at the end of this\nfunction where you're already releasing final_buf.\n\n> for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {\n> - struct atom_value *atomv;\n> + struct atom_value *atomv = NULL;\n\nWhat is this change about?\n\n> ep = strchr(sp, ')');\n> - if (cp < sp)\n> - emit(cp, sp, &output);\n> + if (cp < sp) {\n> + emit(cp, sp, &state);\n> + apply_formatting_state(&state, &final_buf);\n> + }\n> get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);\n> - print_value(atomv, quote_style, &output);\n> + process_formatting_state(atomv, &state);\n> + print_value(atomv, &state);\n> + apply_formatting_state(&state, &final_buf);\n> }\n> if (*cp) {\n> sp = cp + strlen(cp);\n> - emit(cp, sp, &output);\n> + emit(cp, sp, &state);\n> + apply_formatting_state(&state, &final_buf);\n\nI'm getting the feeling that these functions\n(process_formatting_state, print_value, emit, apply_formatting_state)\nare becoming misnamed (again) with the latest structural changes (but\nperhaps I haven't read far enough into the series yet?).\n\nprocess_formatting_state() is rather generic.\n\nprint_value() and emit() both imply outputting something, but neither\ndoes so anymore.\n\napply_formatting_state() seems to be more about finalizing the\nalready-formatted output.\n"},{"id":"267599","messageId":"CAOLa=ZT541+4say3jrqJVK7QgFFwYg3jVJ00nBtGAVzwMTU3CA@mail.gmail.com","threadId":"40009","inReplyTo":"CAPig+cT6uu3DqUB+5gR35JS2eigPBizo_Y48rAzFYkbyCm+=OQ@mail.gmail.com","subject":"Re: [PATCH v9 01/11] ref-filter: print output to strbuf for formatting","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-07T03:24:51Z","receivedAt":"2015-08-07T03:24:51Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Fri, Aug 7, 2015 at 3:51 AM, Eric Sunshine wrote:\n> On Tue, Aug 4, 2015 at 8:42 AM, Karthik Nayak wrote:\n>> Introduce a strbuf `output` which will act as a substitute rather than\n>> printing directly to stdout. This will be used for formatting\n>> eventually.\n>>\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> index 46963a5..91482c9 100644\n>> --- a/ref-filter.c\n>> +++ b/ref-filter.c\n>> @@ -1278,9 +1275,12 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n>> if (color_parse(\"reset\", color) < 0)\n>> die(\"BUG: couldn't parse 'reset' as a color\");\n>> resetv.s = color;\n>> - print_value(&resetv, quote_style);\n>> + print_value(&resetv, quote_style, &output);\n>> }\n>> + for (i = 0; i < output.len; i++)\n>> + printf(\"%c\", output.buf[i]);\n>\n> Everything up to this point seems straightforward, however, it's not\n> clear why you need to emit 'output' one character at a time. Is it\n> because it might contain a NUL '\\0' character and therefore you can't\n> use the simpler printf(\"%s\", output.buf)?\n>\n> If that's the case, then why not just use fwrite() to emit it all at once?\n>\n> fwrite(output.buf, output.len, 1, stdout);\n>\n\nIt was to avoid the printing to stop at '\\0' as you mentioned.\nI've never come across such a situation before, so I looked for\nsimilar implementations online, and found the individual character printing.\nThanks `fwrite` seems neater.\n\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267601","messageId":"CAPig+cTBnBXeNjR53wDUYf6Ewu5t5nZqFB8P-Q8QyE1p98==uA@mail.gmail.com","threadId":"40009","inReplyTo":"1438692188-14367-2-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v9 02/11] ref-filter: introduce ref_formatting_state","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-07T03:36:14Z","receivedAt":"2015-08-07T03:36:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 4, 2015 at 8:42 AM, Karthik Nayak wrote:\n> Introduce a ref_formatting_state which will eventually hold the values\n> of modifier atoms. Implement this within ref-filter.\n>\n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 91482c9..2c074a1 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -1238,35 +1238,58 @@ static void emit(const char *cp, const char *ep, struct strbuf *output)\n> +static void apply_formatting_state(struct ref_formatting_state *state, struct strbuf *final)\n> +{\n> + /* More formatting options to be evetually added */\n\nI forgot to mention this in my earlier review of this patch:\n\ns/evetually/eventually/\n\n> + strbuf_addbuf(final, state->output);\n> + strbuf_release(state->output);\n> +}\n"},{"id":"267602","messageId":"CAOLa=ZSp3aL0Z5YP5xmzdW7H92yU3EA+MJjLYA29QyoZTD5RiA@mail.gmail.com","threadId":"40009","inReplyTo":"CAPig+cQftyjKFi0Qkg_ZVEJ9A+zGSAmFtHwQ-8hCnf8xtU_PEA@mail.gmail.com","subject":"Re: [PATCH v9 02/11] ref-filter: introduce ref_formatting_state","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-07T03:53:04Z","receivedAt":"2015-08-07T03:53:04Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Fri, Aug 7, 2015 at 5:49 AM, Eric Sunshine wrote:\n> On Tue, Aug 4, 2015 at 8:42 AM, Karthik Nayak wrote:\n>> Introduce a ref_formatting_state which will eventually hold the values\n>> of modifier atoms. Implement this within ref-filter.\n>>\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> +static void apply_formatting_state(struct ref_formatting_state *state, struct strbuf *final)\n>> +{\n>> + /* More formatting options to be evetually added */\n>> + strbuf_addbuf(final, state->output);\n>> + strbuf_release(state->output);\n>\n> I guess the idea here is that you intend state->output to be re-used\n> and it is convenient to \"clear\" it here rather than making that the\n> responsibility of each caller. For re-use, it is more typical to use\n> strbuf_reset() than strbuf_release() (though Junio may disagree[1]).\n>\n> [1]: http://article.gmane.org/gmane.comp.version-control.git/273094\n>\n\nit seems like a smarter way to around this without much overhead But it\nwas more of to release it as its no longer required unless another modifier atom\nis encountered. Is it worth keeping hoping for another modifier atom eventually,\nand release it at the end like you suggested below?\n\n>> +}\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>> - struct strbuf output = STRBUF_INIT;\n>> + struct strbuf value = STRBUF_INIT;\n>> + struct strbuf final_buf = STRBUF_INIT;\n>> + struct ref_formatting_state state;\n>> int i;\n>>\n>> + memset(&state, 0, sizeof(state));\n>> + state.quote_style = quote_style;\n>> + state.output = &value;\n>\n> It feels strange to assign a local variable reference to state.output,\n> and there's no obvious reason why you should need to do so. I would\n> have instead expected ref_format_state to be declared as:\n>\n> struct ref_formatting_state {\n> int quote_style;\n> struct strbuf output;\n> };\n>\n> and initialized as so:\n>\n> memset(&state, 0, sizeof(state));\n> state.quote_style = quote_style;\n> strbuf_init(&state.output, 0);\n>\n\nThis looks neater, thanks. It'll go along with the previous patch.\n\n> (In fact, the memset() isn't even necessary here since you're\n> initializing all fields explicitly, though perhaps you want the\n> memset() because a future patch adds more fields which are not\n> initialized explicitly?)\n>\n\nYea the memset is needed for bit fields evnetually added in the future.\n\n> This still allows re-use via strbuf_reset() mentioned above.\n>\n> And, of course, you'd want to strbuf_release() it at the end of this\n> function where you're already releasing final_buf.\n>\n\nAddressed this above.\n\n>> for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {\n>> - struct atom_value *atomv;\n>> + struct atom_value *atomv = NULL;\n>\n> What is this change about?\n>\n\nTo remove the warning about atomv being unassigned before usage.\n\n>> ep = strchr(sp, ')');\n>> - if (cp < sp)\n>> - emit(cp, sp, &output);\n>> + if (cp < sp) {\n>> + emit(cp, sp, &state);\n>> + apply_formatting_state(&state, &final_buf);\n>> + }\n>> get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);\n>> - print_value(atomv, quote_style, &output);\n>> + process_formatting_state(atomv, &state);\n>> + print_value(atomv, &state);\n>> + apply_formatting_state(&state, &final_buf);\n>> }\n>> if (*cp) {\n>> sp = cp + strlen(cp);\n>> - emit(cp, sp, &output);\n>> + emit(cp, sp, &state);\n>> + apply_formatting_state(&state, &final_buf);\n>\n> I'm getting the feeling that these functions\n> (process_formatting_state, print_value, emit, apply_formatting_state)\n> are becoming misnamed (again) with the latest structural changes (but\n> perhaps I haven't read far enough into the series yet?).\n>\n> process_formatting_state() is rather generic.\n>\n\nperhaps set_formatting_state()?\n\n> print_value() and emit() both imply outputting something, but neither\n> does so anymore.\n>\n\nI think I'll append a \"to_state\" to each of them.\n\n> apply_formatting_state() seems to be more about finalizing the\n> already-formatted output.\n\nperform_state_formatting()? perhaps.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267604","messageId":"CAPig+cTt__tphEqFuyeOiTadOL9cAi51RLd3z6rr3nM-8Qp6Aw@mail.gmail.com","threadId":"40009","inReplyTo":"CAOLa=ZSp3aL0Z5YP5xmzdW7H92yU3EA+MJjLYA29QyoZTD5RiA@mail.gmail.com","subject":"Re: [PATCH v9 02/11] ref-filter: introduce ref_formatting_state","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-07T04:43:02Z","receivedAt":"2015-08-07T04:43:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 6, 2015 at 11:53 PM, Karthik Nayak wrote:\n> On Fri, Aug 7, 2015 at 5:49 AM, Eric Sunshine wrote:\n>> On Tue, Aug 4, 2015 at 8:42 AM, Karthik Nayak wrote:\n>>> +static void apply_formatting_state(struct ref_formatting_state *state, struct strbuf *final)\n>>> +{\n>>> + /* More formatting options to be evetually added */\n>>> + strbuf_addbuf(final, state->output);\n>>> + strbuf_release(state->output);\n>>\n>> I guess the idea here is that you intend state->output to be re-used\n>> and it is convenient to \"clear\" it here rather than making that the\n>> responsibility of each caller. For re-use, it is more typical to use\n>> strbuf_reset() than strbuf_release() (though Junio may disagree[1]).\n>\n> it seems like a smarter way to around this without much overhead But it\n> was more of to release it as its no longer required unless another modifier atom\n> is encountered. Is it worth keeping hoping for another modifier atom eventually,\n> and release it at the end like you suggested below?\n\nIf I understand your question correctly, it sounds like you're asking\nabout a memory micro-optimization. From an architectural standpoint,\nit's cleaner for the entity which allocates a resource to also release\nit. In this case, show_ref_array_item() allocates the strbuf, thus it\nshould be the one to release it.\n\nAnd, although we shouldn't be worrying about micro-optimizations at\nthis point, if it were to be an issue, resetting the strbuf via\nstrbuf_reset(), which doesn't involve slow memory\ndeallocation/reallocation, is likely to be a winner over repeated\nstrbuf_release().\n\n>>> + memset(&state, 0, sizeof(state));\n>>> + state.quote_style = quote_style;\n>>> + state.output = &value;\n>>\n>> It feels strange to assign a local variable reference to state.output,\n>> and there's no obvious reason why you should need to do so. I would\n>> have instead expected ref_format_state to be declared as:\n>>\n>> struct ref_formatting_state {\n>> int quote_style;\n>> struct strbuf output;\n>> };\n>>\n>> and initialized as so:\n>>\n>> memset(&state, 0, sizeof(state));\n>> state.quote_style = quote_style;\n>> strbuf_init(&state.output, 0);\n>\n> This looks neater, thanks. It'll go along with the previous patch.\n>\n>> (In fact, the memset() isn't even necessary here since you're\n>> initializing all fields explicitly, though perhaps you want the\n>> memset() because a future patch adds more fields which are not\n>> initialized explicitly?)\n>\n> Yea the memset is needed for bit fields evnetually added in the future.\n\nPerhaps move the memset() to the first patch which actually requires\nit, where it won't be (effectively) dead code, as it becomes here once\nyou make the above change.\n\n>>> for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {\n>>> - struct atom_value *atomv;\n>>> + struct atom_value *atomv = NULL;\n>>\n>> What is this change about?\n>\n> To remove the warning about atomv being unassigned before usage.\n\nHmm, where were you seeing that warning? The first use of 'atomv'\nfollowing its declaration is in the get_ref_atom_value() below, and\n(as far as the compiler knows) that should be setting its value.\n\n>>> ep = strchr(sp, ')');\n>>> - if (cp < sp)\n>>> - emit(cp, sp, &output);\n>>> + if (cp < sp) {\n>>> + emit(cp, sp, &state);\n>>> + apply_formatting_state(&state, &final_buf);\n>>> + }\n>>> get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);\n>>> - print_value(atomv, quote_style, &output);\n>>> + process_formatting_state(atomv, &state);\n>>> + print_value(atomv, &state);\n>>> + apply_formatting_state(&state, &final_buf);\n>>> }\n>>> if (*cp) {\n>>> sp = cp + strlen(cp);\n>>> - emit(cp, sp, &output);\n>>> + emit(cp, sp, &state);\n>>> + apply_formatting_state(&state, &final_buf);\n>>\n>> I'm getting the feeling that these functions\n>> (process_formatting_state, print_value, emit, apply_formatting_state)\n>> are becoming misnamed (again) with the latest structural changes (but\n>> perhaps I haven't read far enough into the series yet?).\n>>\n>> process_formatting_state() is rather generic.\n>\n> perhaps set_formatting_state()?\n\nI don't know. I don't have a proper high-level overview of the\nfunctionality yet to say if that is a good name or not (which is one\nreason I didn't suggest an alternative).\n\n>> print_value() and emit() both imply outputting something, but neither\n>> does so anymore.\n>\n> I think I'll append a \"to_state\" to each of them.\n\nMeh. print_value() might be better named format_value(). emit() might\nbecome append_literal() or append_non_atom() or something.\n\n>> apply_formatting_state() seems to be more about finalizing the\n>> already-formatted output.\n>\n> perform_state_formatting()? perhaps.\n\nDunno.\n"},{"id":"267610","messageId":"CAOLa=ZSRFcfQqjphdSYkWDry8QibKL78Ev=XnfPRxc3PJ17Dog@mail.gmail.com","threadId":"40009","inReplyTo":"CAPig+cTt__tphEqFuyeOiTadOL9cAi51RLd3z6rr3nM-8Qp6Aw@mail.gmail.com","subject":"Re: [PATCH v9 02/11] ref-filter: introduce ref_formatting_state","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-07T11:37:57Z","receivedAt":"2015-08-07T11:37:57Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Fri, Aug 7, 2015 at 10:13 AM, Eric Sunshine wrote:\n> On Thu, Aug 6, 2015 at 11:53 PM, Karthik Nayak wrote:\n>> On Fri, Aug 7, 2015 at 5:49 AM, Eric Sunshine wrote:\n>>> On Tue, Aug 4, 2015 at 8:42 AM, Karthik Nayak wrote:\n>>>> +static void apply_formatting_state(struct ref_formatting_state *state, struct strbuf *final)\n>>>> +{\n>>>> + /* More formatting options to be evetually added */\n>>>> + strbuf_addbuf(final, state->output);\n>>>> + strbuf_release(state->output);\n>>>\n>>> I guess the idea here is that you intend state->output to be re-used\n>>> and it is convenient to \"clear\" it here rather than making that the\n>>> responsibility of each caller. For re-use, it is more typical to use\n>>> strbuf_reset() than strbuf_release() (though Junio may disagree[1]).\n>>\n>> it seems like a smarter way to around this without much overhead But it\n>> was more of to release it as its no longer required unless another modifier atom\n>> is encountered. Is it worth keeping hoping for another modifier atom eventually,\n>> and release it at the end like you suggested below?\n>\n> If I understand your question correctly, it sounds like you're asking\n> about a memory micro-optimization. From an architectural standpoint,\n> it's cleaner for the entity which allocates a resource to also release\n> it. In this case, show_ref_array_item() allocates the strbuf, thus it\n> should be the one to release it.\n>\n> And, although we shouldn't be worrying about micro-optimizations at\n> this point, if it were to be an issue, resetting the strbuf via\n> strbuf_reset(), which doesn't involve slow memory\n> deallocation/reallocation, is likely to be a winner over repeated\n> strbuf_release().\n>\n\nExactly what I was asking, thanks for explaining :D\n\n>>>> + memset(&state, 0, sizeof(state));\n>>>> + state.quote_style = quote_style;\n>>>> + state.output = &value;\n>>>\n>>> It feels strange to assign a local variable reference to state.output,\n>>> and there's no obvious reason why you should need to do so. I would\n>>> have instead expected ref_format_state to be declared as:\n>>>\n>>> struct ref_formatting_state {\n>>> int quote_style;\n>>> struct strbuf output;\n>>> };\n>>>\n>>> and initialized as so:\n>>>\n>>> memset(&state, 0, sizeof(state));\n>>> state.quote_style = quote_style;\n>>> strbuf_init(&state.output, 0);\n>>\n>> This looks neater, thanks. It'll go along with the previous patch.\n>>\n>>> (In fact, the memset() isn't even necessary here since you're\n>>> initializing all fields explicitly, though perhaps you want the\n>>> memset() because a future patch adds more fields which are not\n>>> initialized explicitly?)\n>>\n>> Yea the memset is needed for bit fields evnetually added in the future.\n>\n> Perhaps move the memset() to the first patch which actually requires\n> it, where it won't be (effectively) dead code, as it becomes here once\n> you make the above change.\n>\n\nBut why would I need it there, we need to only memset() the ref_formatting_state\nwhich is introduced here. Also here it helps in setting the strbuf\nwithin ref_formatting_state\nto {0, 0, 0}.\n\n>>>> for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {\n>>>> - struct atom_value *atomv;\n>>>> + struct atom_value *atomv = NULL;\n>>>\n>>> What is this change about?\n>>\n>> To remove the warning about atomv being unassigned before usage.\n>\n> Hmm, where were you seeing that warning? The first use of 'atomv'\n> following its declaration is in the get_ref_atom_value() below, and\n> (as far as the compiler knows) that should be setting its value.\n>\n\nI'll check this out.\n\n>>>> ep = strchr(sp, ')');\n>>>> - if (cp < sp)\n>>>> - emit(cp, sp, &output);\n>>>> + if (cp < sp) {\n>>>> + emit(cp, sp, &state);\n>>>> + apply_formatting_state(&state, &final_buf);\n>>>> + }\n>>>> get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);\n>>>> - print_value(atomv, quote_style, &output);\n>>>> + process_formatting_state(atomv, &state);\n>>>> + print_value(atomv, &state);\n>>>> + apply_formatting_state(&state, &final_buf);\n>>>> }\n>>>> if (*cp) {\n>>>> sp = cp + strlen(cp);\n>>>> - emit(cp, sp, &output);\n>>>> + emit(cp, sp, &state);\n>>>> + apply_formatting_state(&state, &final_buf);\n>>>\n>>> I'm getting the feeling that these functions\n>>> (process_formatting_state, print_value, emit, apply_formatting_state)\n>>> are becoming misnamed (again) with the latest structural changes (but\n>>> perhaps I haven't read far enough into the series yet?).\n>>>\n>>> process_formatting_state() is rather generic.\n>>\n>> perhaps set_formatting_state()?\n>\n> I don't know. I don't have a proper high-level overview of the\n> functionality yet to say if that is a good name or not (which is one\n> reason I didn't suggest an alternative).\n>\n\nAh! okay.\n\n>>> print_value() and emit() both imply outputting something, but neither\n>>> does so anymore.\n>>\n>> I think I'll append a \"to_state\" to each of them.\n>\n> Meh. print_value() might be better named format_value(). emit() might\n> become append_literal() or append_non_atom() or something.\n>\n\nIll think about this, thanks :)\n\n>>> apply_formatting_state() seems to be more about finalizing the\n>>> already-formatted output.\n>>\n>> perform_state_formatting()? perhaps.\n>\n> Dunno.\n\nThat's okay, I'll think about it.\n\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267621","messageId":"CAPig+cSHfQ+Dan1Ps_C0hnCxAz0ZXRu9-gOtDWp9auLvkobEWg@mail.gmail.com","threadId":"40009","inReplyTo":"CAOLa=ZSRFcfQqjphdSYkWDry8QibKL78Ev=XnfPRxc3PJ17Dog@mail.gmail.com","subject":"Re: [PATCH v9 02/11] ref-filter: introduce ref_formatting_state","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-07T17:30:48Z","receivedAt":"2015-08-07T17:30:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Aug 7, 2015 at 7:37 AM, Karthik Nayak wrote:\n> On Fri, Aug 7, 2015 at 10:13 AM, Eric Sunshine wrote:\n>> On Thu, Aug 6, 2015 at 11:53 PM, Karthik Nayak wrote:\n>>> On Fri, Aug 7, 2015 at 5:49 AM, Eric Sunshine wrote:\n>>>> It feels strange to assign a local variable reference to state.output,\n>>>> and there's no obvious reason why you should need to do so. I would\n>>>> have instead expected ref_format_state to be declared as:\n>>>>\n>>>> struct ref_formatting_state {\n>>>> int quote_style;\n>>>> struct strbuf output;\n>>>> };\n>>>>\n>>>> and initialized as so:\n>>>>\n>>>> memset(&state, 0, sizeof(state));\n>>>> state.quote_style = quote_style;\n>>>> strbuf_init(&state.output, 0);\n>>>>\n>>>> (In fact, the memset() isn't even necessary here since you're\n>>>> initializing all fields explicitly, though perhaps you want the\n>>>> memset() because a future patch adds more fields which are not\n>>>> initialized explicitly?)\n>>>\n>>> Yea the memset is needed for bit fields evnetually added in the future.\n>>\n>> Perhaps move the memset() to the first patch which actually requires\n>> it, where it won't be (effectively) dead code, as it becomes here once\n>> you make the above change.\n>\n> But why would I need it there, we need to only memset() the ref_formatting_state\n> which is introduced here. Also here it helps in setting the strbuf\n> within ref_formatting_state to {0, 0, 0}.\n\nIf you declare ref_formatting_state as shown above, and then\ninitialize it like so:\n\n state.quote_style = quote_style;\n strbuf_init(&state.output, 0);\n\nthen (as of this patch) the structure is fully initialized because\nyou've initialized each member individually. Adding a memset() above\nthose two lines would be do-nothing -- it would be wasted code -- and\nwould likely confuse someone reading the code, specifically because\nthe code is do-nothing and has no value (in this patch). Making each\npatch understandable is one of your goals when organizing the patch\nseries; if a patch confuses a reader (say, by doing unnecessary work),\nthen it isn't satisfying that goal.\n\nAs for the strbuf member, it's initialized explicitly via\nstrbuf_init(), so there's no value in having memset() first initialize\nit to {0, 0, 0}. Again, that's wasted code.\n\nIn a later patch, when you add another ref_formatting_state member or\ntwo, then you will need to initialize those members too. That\ninitialization may be in the form of explicit assignment to each\nmember, or it may be the memset() sledgehammer approach, but the\ninitialization for those members should be added in the patch which\nintroduces them.\n\nIt's true that the end result is the same. By the end of the series,\nyou'll have memset() above the 'state.quote' and 'state.output'\ninitialization lines to ensure that your various boolean fields and\nwhatnot are initialized to zero, but each patch should be\nself-contained and make sense on its own, doing just the work that it\nneeds to do, and not doing unrelated work. For this patch, the\nmemset() is unrelated work. For the later patch in which you add more\nfields to ref_formatting_state(), the memset() is work necessary to\nsatisfy that patch's objective, thus belongs there.\n"},{"id":"267622","messageId":"CAOLa=ZTi0BfP1kkK+Ty49Z7kYoh8k0TjrfCfpapBWAot1kMVoQ@mail.gmail.com","threadId":"40009","inReplyTo":"CAPig+cSHfQ+Dan1Ps_C0hnCxAz0ZXRu9-gOtDWp9auLvkobEWg@mail.gmail.com","subject":"Re: [PATCH v9 02/11] ref-filter: introduce ref_formatting_state","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-07T17:50:30Z","receivedAt":"2015-08-07T17:50:30Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Fri, Aug 7, 2015 at 11:00 PM, Eric Sunshine wrote:\n> On Fri, Aug 7, 2015 at 7:37 AM, Karthik Nayak wrote:\n>> On Fri, Aug 7, 2015 at 10:13 AM, Eric Sunshine wrote:\n>>> On Thu, Aug 6, 2015 at 11:53 PM, Karthik Nayak wrote:\n>>>> On Fri, Aug 7, 2015 at 5:49 AM, Eric Sunshine wrote:\n>>>>> It feels strange to assign a local variable reference to state.output,\n>>>>> and there's no obvious reason why you should need to do so. I would\n>>>>> have instead expected ref_format_state to be declared as:\n>>>>>\n>>>>> struct ref_formatting_state {\n>>>>> int quote_style;\n>>>>> struct strbuf output;\n>>>>> };\n>>>>>\n>>>>> and initialized as so:\n>>>>>\n>>>>> memset(&state, 0, sizeof(state));\n>>>>> state.quote_style = quote_style;\n>>>>> strbuf_init(&state.output, 0);\n>>>>>\n>>>>> (In fact, the memset() isn't even necessary here since you're\n>>>>> initializing all fields explicitly, though perhaps you want the\n>>>>> memset() because a future patch adds more fields which are not\n>>>>> initialized explicitly?)\n>>>>\n>>>> Yea the memset is needed for bit fields evnetually added in the future.\n>>>\n>>> Perhaps move the memset() to the first patch which actually requires\n>>> it, where it won't be (effectively) dead code, as it becomes here once\n>>> you make the above change.\n>>\n>> But why would I need it there, we need to only memset() the ref_formatting_state\n>> which is introduced here. Also here it helps in setting the strbuf\n>> within ref_formatting_state to {0, 0, 0}.\n>\n> If you declare ref_formatting_state as shown above, and then\n> initialize it like so:\n>\n> state.quote_style = quote_style;\n> strbuf_init(&state.output, 0);\n>\n> then (as of this patch) the structure is fully initialized because\n> you've initialized each member individually. Adding a memset() above\n> those two lines would be do-nothing -- it would be wasted code -- and\n> would likely confuse someone reading the code, specifically because\n> the code is do-nothing and has no value (in this patch). Making each\n> patch understandable is one of your goals when organizing the patch\n> series; if a patch confuses a reader (say, by doing unnecessary work),\n> then it isn't satisfying that goal.\n>\n> As for the strbuf member, it's initialized explicitly via\n> strbuf_init(), so there's no value in having memset() first initialize\n> it to {0, 0, 0}. Again, that's wasted code.\n\nYes, what you're saying is true, if we replace memset() with\n\nstate.quote_style = quote_style;\nstrbuf_init(&state.output, 0);\n\nThat does replace the need for memset and make the patch\nmore self-explanatory.\n\n>\n> In a later patch, when you add another ref_formatting_state member or\n> two, then you will need to initialize those members too. That\n> initialization may be in the form of explicit assignment to each\n> member, or it may be the memset() sledgehammer approach, but the\n> initialization for those members should be added in the patch which\n> introduces them.\n>\n> It's true that the end result is the same. By the end of the series,\n> you'll have memset() above the 'state.quote' and 'state.output'\n> initialization lines to ensure that your various boolean fields and\n> whatnot are initialized to zero, but each patch should be\n> self-contained and make sense on its own, doing just the work that it\n> needs to do, and not doing unrelated work. For this patch, the\n> memset() is unrelated work. For the later patch in which you add more\n> fields to ref_formatting_state(), the memset() is work necessary to\n> satisfy that patch's objective, thus belongs there.\n\nI understand what you're saying, it makes sense to have individual patches\nonly bring in changes related to the patch. This makes a lot of sense.\n\nI will change it up. Thanks :)\n\n-- \nRegards,\nKarthik Nayak\n"}]}