{"thread":{"id":"40155","subject":"[PATCH v13 00/12] port tag.c to use ref-filter APIs","startedAt":"2015-08-22T03:39:36Z","lastAt":"2015-08-27T18:00:41Z","messageCount":60,"participants":["Karthik Nayak","Matthieu Moy","Junio C Hamano","Michael Haggerty"],"isPatch":true,"patchVersion":13,"patchTotal":12},"messages":[{"id":"268485","messageId":"1440214788-1309-1-git-send-email-Karthik.188@gmail.com","threadId":"40155","inReplyTo":null,"subject":"[PATCH v13 00/12] port tag.c to use ref-filter APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-22T03:39:36Z","receivedAt":"2015-08-22T03:39:36Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Part of my GSoC project to port tag.c to use ref-filter APIs. This is\na follow up to porting for-each-ref to use ref-filter APIs.\n\nVersion 12 can be found here:\nthread.gmane.org/gmane.comp.version-control.git/276133\n\nChanges since v12:\n* %(align)...%(end) now quote formats everything in between the atoms\neven if they are string literals. \n* Changet the structure of ref_formatting_state to hold ref_formatting_stack\nas the stack and quote_value, this ensures that we do not need to copy the \nquote_value to each element of the stack.\n* While checking for %(align) atom, use strbuf_split_str().\n* In tag.c support usage of --format with -n.\n\nKarthik Nayak (12):\n ref-filter: move `struct atom_value` to ref-filter.c\n ref-filter: introduce ref_formatting_state and ref_formatting_stack\n utf8: add function to align a string into given strbuf\n ref-filter: implement an `align` atom\n ref-filter: add option to filter out tags, branches and remotes\n ref-filter: support printing N lines from tag annotation\n ref-filter: add support to sort by version\n ref-filter: add option to match literal pattern\n tag.c: use 'ref-filter' data structures\n tag.c: use 'ref-filter' APIs\n tag.c: implement '--format' option\n tag.c: implement '--merged' and '--no-merged' options\n\n Documentation/git-for-each-ref.txt | 12 ++\n Documentation/git-tag.txt | 27 ++-\n builtin/for-each-ref.c | 3 +-\n builtin/tag.c | 365 +++++++------------------------------\n ref-filter.c | 350 +++++++++++++++++++++++++++++++----\n ref-filter.h | 32 +++-\n refs.c | 9 +\n refs.h | 1 +\n t/t6302-for-each-ref-filter.sh | 105 +++++++++++\n t/t7004-tag.sh | 47 ++++-\n utf8.c | 21 +++\n utf8.h | 15 ++\n 12 files changed, 626 insertions(+), 361 deletions(-)\n\n\nInterdiff:\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 1997657..06d468e 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -133,7 +133,8 @@ align::\n \t`` is either left, right or middle and `` is\n \tthe total length of the content with alignment. If the\n \tcontents length is more than the width then no alignment is\n-\tperformed.\n+\tperformed. If used with '--quote' everything in between %(align:..)\n+\tand %(end) is quoted.\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/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex c2785d9..3803bf7 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -161,22 +161,15 @@ This option is only applicable when listing tags without annotation lines.\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+\tpointed at by a ref being shown. The format is the same as\n+\tthat of linkgit:git-for-each-ref[1]. When unspecified,\n+\tdefaults to `%(refname:short)`.\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 -------------\n By default, 'git tag' in sign-with-default mode (-s) will use your\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 665221b..f8b8fb7 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -65,18 +65,24 @@ struct align {\n \tunsigned int width;\n };\n \n-struct ref_formatting_state {\n-\tstruct ref_formatting_state *prev;\n+#define REF_FORMATTING_STATE_INIT { 0, NULL }\n+\n+struct ref_formatting_stack {\n+\tstruct ref_formatting_stack *prev;\n \tstruct strbuf output;\n-\tvoid (*at_end)(struct ref_formatting_state *state);\n+\tvoid (*at_end)(struct ref_formatting_stack *stack);\n \tvoid *cb_data;\n+};\n+\n+struct ref_formatting_state {\n \tint quote_style;\n+\tstruct ref_formatting_stack *stack;\n };\n \n struct atom_value {\n \tconst char *s;\n \tstruct align *align;\n-\tvoid (*handler)(struct atom_value *atomv, struct ref_formatting_state **state);\n+\tvoid (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);\n \tunsigned long ul; /* used for sorting when not FIELD_STR */\n };\n \n@@ -149,19 +155,19 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n \treturn at;\n }\n \n-static void push_new_state(struct ref_formatting_state **stack)\n+static void push_new_stack_element(struct ref_formatting_stack **stack)\n {\n-\tstruct ref_formatting_state *s = xcalloc(1, sizeof(struct ref_formatting_state));\n+\tstruct ref_formatting_stack *s = xcalloc(1, sizeof(struct ref_formatting_stack));\n \n \tstrbuf_init(&s->output, 0);\n \ts->prev = *stack;\n \t*stack = s;\n }\n \n-static void pop_state(struct ref_formatting_state **stack)\n+static void pop_stack_element(struct ref_formatting_stack **stack)\n {\n-\tstruct ref_formatting_state *current = *stack;\n-\tstruct ref_formatting_state *prev = current->prev;\n+\tstruct ref_formatting_stack *current = *stack;\n+\tstruct ref_formatting_stack *prev = current->prev;\n \n \tif (prev)\n \t\tstrbuf_addbuf(&prev->output, ¤t->output);\n@@ -640,34 +646,49 @@ static inline char *copy_advance(char *dst, const char *src)\n \treturn dst;\n }\n \n-static void align_handler(struct ref_formatting_state *state)\n+static void align_handler(struct ref_formatting_stack *stack)\n {\n-\tstruct align *align = (struct align *)state->cb_data;\n+\tstruct align *align = (struct align *)stack->cb_data;\n \tstruct strbuf s = STRBUF_INIT;\n \n-\tstrbuf_utf8_align(&s, align->position, align->width, state->output.buf);\n-\tstrbuf_reset(&state->output);\n-\tstrbuf_addbuf(&state->output, &s);\n+\tstrbuf_utf8_align(&s, align->position, align->width, stack->output.buf);\n+\tstrbuf_swap(&stack->output, &s);\n+\tstrbuf_release(&s);\n \tfree(align);\n }\n \n-static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_state **state)\n+static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n {\n-\tstruct ref_formatting_state *new;\n+\tstruct ref_formatting_stack *new;\n \n-\tpush_new_state(state);\n-\tnew = *state;\n+\tpush_new_stack_element(&state->stack);\n+\tnew = state->stack;\n \tnew->at_end = align_handler;\n \tnew->cb_data = atomv->align;\n }\n \n-static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state **state)\n+static void perform_quote_formatting(struct strbuf *s, const char *str, int quote_style);\n+\n+static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n {\n-\tstruct ref_formatting_state *current = *state;\n+\tstruct ref_formatting_stack *current = state->stack;\n+\tstruct strbuf s = STRBUF_INIT;\n+\n \tif (!current->at_end)\n \t\tdie(_(\"format: `end` atom used without a supporting atom\"));\n \tcurrent->at_end(current);\n-\tpop_state(state);\n+\t/*\n+\t * Whenever we have more than one stack element that means we\n+\t * are using a certain modifier atom. In that case we need to\n+\t * perform quote formatting.\n+\t */\n+\tif (!state->stack->prev->prev) {\n+\t\tperform_quote_formatting(&s, current->output.buf, state->quote_style);\n+\t\tstrbuf_reset(¤t->output);\n+\t\tstrbuf_addbuf(¤t->output, &s);\n+\t}\n+\tstrbuf_release(&s);\n+\tpop_stack_element(&state->stack);\n }\n \n /*\n@@ -766,25 +787,26 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\tcontinue;\n \t\t} else if (skip_prefix(name, \"align:\", &valp)) {\n \t\t\tstruct align *align = xmalloc(sizeof(struct align));\n-\t\t\tchar *ep = strchr(valp, ',');\n+\t\t\tstruct strbuf **s = strbuf_split_str(valp, ',', 0);\n \n-\t\t\tif (ep)\n-\t\t\t\t*ep = '\\0';\n+\t\t\t/* If the position is given trim the ',' from the first strbuf */\n+\t\t\tif (s[1])\n+\t\t\t\tstrbuf_remove(s[0], s[0]->len - 1, 1);\n \n-\t\t\tif (strtoul_ui(valp, 10, &align->width))\n-\t\t\t\tdie(_(\"positive width expected align:%s\"), valp);\n+\t\t\tif (strtoul_ui(s[0]->buf, 10, &align->width))\n+\t\t\t\tdie(_(\"positive width expected align:%s\"), s[0]->buf);\n \n-\t\t\tif (!ep || !strcmp(ep + 1, \"left\"))\n+\t\t\t/* If no position is given, default to ALIGN_LEFT */\n+\t\t\tif (!s[1] || !strcmp(s[1]->buf, \"left\"))\n \t\t\t\talign->position = ALIGN_LEFT;\n-\t\t\telse if (!strcmp(ep + 1, \"right\"))\n+\t\t\telse if (!strcmp(s[1]->buf, \"right\"))\n \t\t\t\talign->position = ALIGN_RIGHT;\n-\t\t\telse if (!strcmp(ep + 1, \"middle\"))\n+\t\t\telse if (!strcmp(s[1]->buf, \"middle\"))\n \t\t\t\talign->position = ALIGN_MIDDLE;\n \t\t\telse\n-\t\t\t\tdie(_(\"improper format entered align:%s\"), ep + 1);\n+\t\t\t\tdie(_(\"improper format entered align:%s\"), s[1]->buf);\n \n-\t\t\tif (ep)\n-\t\t\t\t*ep = ',';\n+\t\t\tstrbuf_list_free(s);\n \n \t\t\tv->align = align;\n \t\t\tv->handler = align_atom_handler;\n@@ -1378,27 +1400,33 @@ 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 append_atom(struct atom_value *v, struct ref_formatting_state *state)\n+static void perform_quote_formatting(struct strbuf *s, const char *str, int quote_style)\n {\n-\tswitch (state->quote_style) {\n+\tswitch (quote_style) {\n \tcase QUOTE_NONE:\n-\t\tstrbuf_addstr(&state->output, v->s);\n+\t\tstrbuf_addstr(s, str);\n \t\tbreak;\n \tcase QUOTE_SHELL:\n-\t\tsq_quote_buf(&state->output, v->s);\n+\t\tsq_quote_buf(s, str);\n \t\tbreak;\n \tcase QUOTE_PERL:\n-\t\tperl_quote_buf(&state->output, v->s);\n+\t\tperl_quote_buf(s, str);\n \t\tbreak;\n \tcase QUOTE_PYTHON:\n-\t\tpython_quote_buf(&state->output, v->s);\n+\t\tpython_quote_buf(s, str);\n \t\tbreak;\n \tcase QUOTE_TCL:\n-\t\ttcl_quote_buf(&state->output, v->s);\n+\t\ttcl_quote_buf(s, str);\n \t\tbreak;\n \t}\n }\n \n+static void append_atom(struct atom_value *v, struct ref_formatting_state *state)\n+{\n+\tstruct strbuf *s = &state->stack->output;\n+\tperform_quote_formatting(s, v->s, state->quote_style);\n+}\n+\n static int hex1(char ch)\n {\n \tif ('0' <= ch && ch <= '9')\n@@ -1419,6 +1447,8 @@ static int hex2(const char *cp)\n \n static void append_literal(const char *cp, const char *ep, struct ref_formatting_state *state)\n {\n+\tstruct strbuf *s = &state->stack->output;\n+\n \twhile (*cp && (!ep || cp < ep)) {\n \t\tif (*cp == '%') {\n \t\t\tif (cp[1] == '%')\n@@ -1426,13 +1456,13 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting\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(&state->output, ch);\n+\t\t\t\t\tstrbuf_addch(s, 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(&state->output, *cp);\n+\t\tstrbuf_addch(s, *cp);\n \t\tcp++;\n \t}\n }\n@@ -1485,25 +1515,34 @@ void show_ref_array_item(struct ref_array_item *info, const char *format,\n {\n \tconst char *cp, *sp, *ep;\n \tstruct strbuf *final_buf;\n-\tstruct ref_formatting_state *state = NULL;\n+\tstruct ref_formatting_state state = REF_FORMATTING_STATE_INIT;\n \n-\tpush_new_state(&state);\n-\tstate->quote_style = quote_style;\n+\tstate.quote_style = quote_style;\n+\tpush_new_stack_element(&state.stack);\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\tappend_literal(cp, sp, state);\n+\t\t\tappend_literal(cp, sp, &state);\n \t\tget_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);\n+\t\t/*\n+\t\t * If the atom is a modifier atom, then call the handler function.\n+\t\t * Else, if this is the first element on the stack, then we need to\n+\t\t * format the atom as per the given quote. Else we just add the atom value\n+\t\t * to the current stack element and handle quote formatting at the end.\n+\t\t */\n \t\tif (atomv->handler)\n \t\t\tatomv->handler(atomv, &state);\n-\t\tappend_atom(atomv, state);\n+\t\telse if (!state.stack->prev)\n+\t\t\tappend_atom(atomv, &state);\n+\t\telse\n+\t\t\tstrbuf_addstr(&state.stack->output, atomv->s);\n \t}\n \tif (*cp) {\n \t\tsp = cp + strlen(cp);\n-\t\tappend_literal(cp, sp, state);\n+\t\tappend_literal(cp, sp, &state);\n \t}\n \tif (need_color_reset_at_eol) {\n \t\tstruct atom_value resetv;\n@@ -1512,12 +1551,13 @@ void show_ref_array_item(struct ref_array_item *info, const char *format,\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\tappend_atom(&resetv, state);\n+\t\tappend_atom(&resetv, &state);\n \t}\n-\tfinal_buf = &state->output;\n+\tif (state.stack->prev)\n+\t\tdie(_(\"format: `end` atom missing\"));\n+\tfinal_buf = &state.stack->output;\n \tfwrite(final_buf->buf, 1, final_buf->len, stdout);\n-\tpop_state(&state);\n-\n+\tpop_stack_element(&state.stack);\n \tif (lines > 0) {\n \t\tstruct object_id oid;\n \t\thashcpy(oid.hash, info->objectname);\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 1c56879..38c99c9 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -129,6 +129,27 @@ test_expect_success 'right alignment' '\n \ttest_cmp expect actual\n '\n \n+# Everything in between the %(align)...%(end) atom must be quoted, hence we test this by\n+# introducing single quote's in %(align)...%(end), which must not be escaped.\n+\n+sq=\"'\"\n+\n+test_expect_success 'alignment with format quote' '\n+\tcat >expect <<-EOF &&\n+\trefname is ${sq} ${sq}\\\\${sq}${sq}master${sq}\\\\${sq}${sq} ${sq}|\n+\trefname is ${sq} ${sq}\\\\${sq}${sq}side${sq}\\\\${sq}${sq} ${sq}|\n+\trefname is ${sq} ${sq}\\\\${sq}${sq}odd/spot${sq}\\\\${sq}${sq} ${sq}|\n+\trefname is ${sq} ${sq}\\\\${sq}${sq}double-tag${sq}\\\\${sq}${sq} ${sq}|\n+\trefname is ${sq} ${sq}\\\\${sq}${sq}four${sq}\\\\${sq}${sq} ${sq}|\n+\trefname is ${sq} ${sq}\\\\${sq}${sq}one${sq}\\\\${sq}${sq} ${sq}|\n+\trefname is ${sq} ${sq}\\\\${sq}${sq}signed-tag${sq}\\\\${sq}${sq} ${sq}|\n+\trefname is ${sq} ${sq}\\\\${sq}${sq}three${sq}\\\\${sq}${sq} ${sq}|\n+\trefname is ${sq} ${sq}\\\\${sq}${sq}two${sq}\\\\${sq}${sq} ${sq}|\n+\tEOF\n+\tgit for-each-ref --shell --format=\"refname is %(align:30,middle)${sq}%(refname:short)${sq}%(end)|\" >actual &&\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 &&\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 335396e..3dd2f51 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1519,10 +1519,6 @@ 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\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 781d3e5..bbbcaed 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -23,7 +23,7 @@ static const char * const git_tag_usage[] = {\n N_(\"git tag [-a | -s | -u ] [-f] [-m | -F ] []\"),\n N_(\"git tag -d ...\"),\n N_(\"git tag -l [-n[]] [--contains ] [--points-at ]\"\n- \"\\n\\t\\t[--[no-]merged []] [...]\"),\n+ \"\\n\\t\\t[--format=] [--[no-]merged []] [...]\"),\n N_(\"git tag -v ...\"),\n NULL\n };\n@@ -40,10 +40,12 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con\n if (filter->lines == -1)\n filter->lines = 0;\n \n- if (filter->lines)\n- format = \"%(align:16,left)%(refname:short)%(end)\";\n- else if (!format)\n- format = \"%(refname:short)\";\n+ if (!format) {\n+ if (filter->lines)\n+ format = \"%(align:16,left)%(refname:short)%(end)\";\n+ else\n+ format = \"%(refname:short)\";\n+ }\n \n verify_ref_format(format);\n filter_refs(&array, filter, FILTER_REFS_TAGS);\n@@ -401,8 +403,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n copts.padding = 2;\n run_column_filter(colopts, &copts);\n }\n- if (format && (filter.lines != -1))\n- die(_(\"--format and -n are incompatible\"));\n filter.name_patterns = argv;\n ret = list_tags(&filter, sorting, format);\n if (column_active(colopts))\n\n-- \n2.5.0\n"},{"id":"268486","messageId":"1440214788-1309-2-git-send-email-Karthik.188@gmail.com","threadId":"40155","inReplyTo":"1440214788-1309-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v13 01/12] ref-filter: move `struct atom_value` to ref-filter.c","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-22T03:39:37Z","receivedAt":"2015-08-22T03:39:37Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Since atom_value is only required for the internal working of\nref-filter it doesn't belong in the public header.\n\nHelped-by: Eric Sunshine \nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 5 +++++\n ref-filter.h | 5 +----\n 2 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 46963a5..e53c77e 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -55,6 +55,11 @@ static struct {\n \t{ \"color\" },\n };\n \n+struct atom_value {\n+\tconst char *s;\n+\tunsigned long ul; /* used for sorting when not FIELD_STR */\n+};\n+\n /*\n * An atom is a valid field atom listed above, possibly prefixed with\n * a \"*\" to denote deref_tag().\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 6bf27d8..45026d0 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -16,10 +16,7 @@\n #define FILTER_REFS_INCLUDE_BROKEN 0x1\n #define FILTER_REFS_ALL 0x2\n \n-struct atom_value {\n-\tconst char *s;\n-\tunsigned long ul; /* used for sorting when not FIELD_STR */\n-};\n+struct atom_value;\n \n struct ref_sorting {\n \tstruct ref_sorting *next;\n-- \n2.5.0\n"},{"id":"268488","messageId":"1440214788-1309-3-git-send-email-Karthik.188@gmail.com","threadId":"40155","inReplyTo":"1440214788-1309-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v13 02/12] ref-filter: introduce ref_formatting_state and ref_formatting_stack","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-22T03:39:38Z","receivedAt":"2015-08-22T03:39:38Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Introduce ref_formatting_state which will hold the formatted output\nstrbuf instead of directly printing to stdout. This will help us in\ncreating modifier atoms which modify the format specified before\nprinting to stdout.\n\nImplement a stack machinery for ref_formatting_state, this allows us\nto push and pop elements onto the stack. Whenever we pop an element\nfrom the stack, the strbuf from that element is appended to the strbuf\nof the next element on the stack, this will allow us to support\nnesting of modifier atoms.\n\nRename some functions to reflect the changes made:\nprint_value() -> append_atom()\nemit() -> append_literal()\n\nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 78 +++++++++++++++++++++++++++++++++++++++++++++---------------\n 1 file changed, 59 insertions(+), 19 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex e53c77e..d0d8df0 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -55,6 +55,18 @@ static struct {\n \t{ \"color\" },\n };\n \n+#define REF_FORMATTING_STATE_INIT { 0, NULL }\n+\n+struct ref_formatting_stack {\n+\tstruct ref_formatting_stack *prev;\n+\tstruct strbuf output;\n+};\n+\n+struct ref_formatting_state {\n+\tint quote_style;\n+\tstruct ref_formatting_stack *stack;\n+};\n+\n struct atom_value {\n \tconst char *s;\n \tunsigned long ul; /* used for sorting when not FIELD_STR */\n@@ -129,6 +141,27 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n \treturn at;\n }\n \n+static void push_new_stack_element(struct ref_formatting_stack **stack)\n+{\n+\tstruct ref_formatting_stack *s = xcalloc(1, sizeof(struct ref_formatting_stack));\n+\n+\tstrbuf_init(&s->output, 0);\n+\ts->prev = *stack;\n+\t*stack = s;\n+}\n+\n+static void pop_stack_element(struct ref_formatting_stack **stack)\n+{\n+\tstruct ref_formatting_stack *current = *stack;\n+\tstruct ref_formatting_stack *prev = current->prev;\n+\n+\tif (prev)\n+\t\tstrbuf_addbuf(&prev->output, ¤t->output);\n+\tstrbuf_release(¤t->output);\n+\tfree(current);\n+\t*stack = prev;\n+}\n+\n /*\n * In a format string, find the next occurrence of %(atom).\n */\n@@ -1195,30 +1228,27 @@ 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 append_atom(struct atom_value *v, struct ref_formatting_state *state)\n {\n-\tstruct strbuf sb = STRBUF_INIT;\n-\tswitch (quote_style) {\n+\tstruct strbuf *s = &state->stack->output;\n+\n+\tswitch (state->quote_style) {\n \tcase QUOTE_NONE:\n-\t\tfputs(v->s, stdout);\n+\t\tstrbuf_addstr(s, v->s);\n \t\tbreak;\n \tcase QUOTE_SHELL:\n-\t\tsq_quote_buf(&sb, v->s);\n+\t\tsq_quote_buf(s, v->s);\n \t\tbreak;\n \tcase QUOTE_PERL:\n-\t\tperl_quote_buf(&sb, v->s);\n+\t\tperl_quote_buf(s, v->s);\n \t\tbreak;\n \tcase QUOTE_PYTHON:\n-\t\tpython_quote_buf(&sb, v->s);\n+\t\tpython_quote_buf(s, v->s);\n \t\tbreak;\n \tcase QUOTE_TCL:\n-\t\ttcl_quote_buf(&sb, v->s);\n+\t\ttcl_quote_buf(s, 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@@ -1239,8 +1269,10 @@ 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 append_literal(const char *cp, const char *ep, struct ref_formatting_state *state)\n {\n+\tstruct strbuf *s = &state->stack->output;\n+\n \twhile (*cp && (!ep || cp < ep)) {\n \t\tif (*cp == '%') {\n \t\t\tif (cp[1] == '%')\n@@ -1248,13 +1280,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(s, 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(s, *cp);\n \t\tcp++;\n \t}\n }\n@@ -1262,19 +1294,24 @@ 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 *final_buf;\n+\tstruct ref_formatting_state state = REF_FORMATTING_STATE_INIT;\n+\n+\tstate.quote_style = quote_style;\n+\tpush_new_stack_element(&state.stack);\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\tappend_literal(cp, sp, &state);\n \t\tget_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);\n-\t\tprint_value(atomv, quote_style);\n+\t\tappend_atom(atomv, &state);\n \t}\n \tif (*cp) {\n \t\tsp = cp + strlen(cp);\n-\t\temit(cp, sp);\n+\t\tappend_literal(cp, sp, &state);\n \t}\n \tif (need_color_reset_at_eol) {\n \t\tstruct atom_value resetv;\n@@ -1283,8 +1320,11 @@ 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\tappend_atom(&resetv, &state);\n \t}\n+\tfinal_buf = &state.stack->output;\n+\tfwrite(final_buf->buf, 1, final_buf->len, stdout);\n+\tpop_stack_element(&state.stack);\n \tputchar('\\n');\n }\n \n-- \n2.5.0\n"},{"id":"268487","messageId":"1440214788-1309-4-git-send-email-Karthik.188@gmail.com","threadId":"40155","inReplyTo":"1440214788-1309-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v13 03/12] utf8: add function to align a string into given strbuf","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-22T03:39:39Z","receivedAt":"2015-08-22T03:39:39Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Add strbuf_utf8_align() which will align a given string into a strbuf\nas per given align_type and width. If the width is greater than the\nstring length then no alignment is performed.\n\nHelped-by: Eric Sunshine \nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n utf8.c | 21 +++++++++++++++++++++\n utf8.h | 15 +++++++++++++++\n 2 files changed, 36 insertions(+)\n\ndiff --git a/utf8.c b/utf8.c\nindex 28e6d76..00e10c8 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -644,3 +644,24 @@ int skip_utf8_bom(char **text, size_t len)\n \t*text += strlen(utf8_bom);\n \treturn 1;\n }\n+\n+void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned int width,\n+\t\t const char *s)\n+{\n+\tint slen = strlen(s);\n+\tint display_len = utf8_strnwidth(s, slen, 0);\n+\tint utf8_compensation = slen - display_len;\n+\n+\tif (display_len >= width) {\n+\t\tstrbuf_addstr(buf, s);\n+\t\treturn;\n+\t}\n+\n+\tif (position == ALIGN_LEFT)\n+\t\tstrbuf_addf(buf, \"%-*s\", width + utf8_compensation, s);\n+\telse if (position == ALIGN_MIDDLE) {\n+\t\tint left = (width - display_len) / 2;\n+\t\tstrbuf_addf(buf, \"%*s%-*s\", left, \"\", width - left + utf8_compensation, s);\n+\t} else if (position == ALIGN_RIGHT)\n+\t\tstrbuf_addf(buf, \"%*s\", width + utf8_compensation, s);\n+}\ndiff --git a/utf8.h b/utf8.h\nindex 5a9e94b..7930b44 100644\n--- a/utf8.h\n+++ b/utf8.h\n@@ -55,4 +55,19 @@ int mbs_chrlen(const char **text, size_t *remainder_p, const char *encoding);\n */\n int is_hfs_dotgit(const char *path);\n \n+typedef enum {\n+\tALIGN_LEFT,\n+\tALIGN_MIDDLE,\n+\tALIGN_RIGHT\n+} align_type;\n+\n+/*\n+ * Align the string given and store it into a strbuf as per the\n+ * 'position' and 'width'. If the given string length is larger than\n+ * 'width' than then the input string is not truncated and no\n+ * alignment is done.\n+ */\n+void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned int width,\n+\t\t const char *s);\n+\n #endif\n-- \n2.5.0\n"},{"id":"268490","messageId":"1440214788-1309-5-git-send-email-Karthik.188@gmail.com","threadId":"40155","inReplyTo":"1440214788-1309-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v13 04/12] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-22T03:39:40Z","receivedAt":"2015-08-22T03:39:40Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Implement an `align` atom which left-, middle-, or right-aligns the\ncontent between %(align:..) and %(end).\n\nIt is followed by `:,`, where the `` is\neither left, right or middle and `` is the size of the area\ninto which the content will be placed. If the content between\n%(align:) and %(end) is more than the width then no alignment is\nperformed. e.g. to align a refname atom to the middle with a total\nwidth of 40 we can do: --format=\"%(align:middle,40)%(refname)%(end)\".\n\nWe now have a `handler()` for each atom_value which will be called\nwhen that atom_value is being parsed, and similarly an `at_end`\nfunction for each element of the stack which is to be called when the\n`end` atom is encountered. Using this we implement the `align` atom\nwhich aligns the given strbuf by calling `strbuf_utf8_align()` from\nutf8.c.\n\nExtract perform_quote_formatting() from append_atom(). Given a string\na quote_value and a strbuf, perform_quote_formatting() formats the\nstring based on the quote_value and stores it into the strbuf.\n\nEnsure that quote formatting is performed on the whole of\n%(align)...%(end) rather than individual atoms. We do this by skipping\nindividual quote formatting for atoms whenever the stack has more than\none element, and performing formatting for the entire stack element\nwhen the `%(end)` atoms is encountered.\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 | 9 +++\n ref-filter.c | 124 ++++++++++++++++++++++++++++++++++---\n t/t6302-for-each-ref-filter.sh | 69 +++++++++++++++++++++\n 3 files changed, 192 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex e49d578..943975d 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -127,6 +127,15 @@ color::\n \tChange output color. Followed by `:`, where names\n \tare described in `color.branch.*`.\n \n+align::\n+\tLeft-, middle-, or right-align the content between %(align:..)\n+\tand %(end). Followed by `:,`, where the\n+\t`` is either left, right or middle and `` is\n+\tthe total length of the content with alignment. If the\n+\tcontents length is more than the width then no alignment is\n+\tperformed. If used with '--quote' everything in between %(align:..)\n+\tand %(end) is quoted.\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 d0d8df0..ffec10a 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,13 @@ static struct {\n \t{ \"flag\" },\n \t{ \"HEAD\" },\n \t{ \"color\" },\n+\t{ \"align\" },\n+\t{ \"end\" },\n+};\n+\n+struct align {\n+\talign_type position;\n+\tunsigned int width;\n };\n \n #define REF_FORMATTING_STATE_INIT { 0, NULL }\n@@ -60,6 +68,8 @@ static struct {\n struct ref_formatting_stack {\n \tstruct ref_formatting_stack *prev;\n \tstruct strbuf output;\n+\tvoid (*at_end)(struct ref_formatting_stack *stack);\n+\tvoid *cb_data;\n };\n \n struct ref_formatting_state {\n@@ -69,6 +79,8 @@ struct ref_formatting_state {\n \n struct atom_value {\n \tconst char *s;\n+\tstruct align *align;\n+\tvoid (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);\n \tunsigned long ul; /* used for sorting when not FIELD_STR */\n };\n \n@@ -632,6 +644,51 @@ static inline char *copy_advance(char *dst, const char *src)\n \treturn dst;\n }\n \n+static void align_handler(struct ref_formatting_stack *stack)\n+{\n+\tstruct align *align = (struct align *)stack->cb_data;\n+\tstruct strbuf s = STRBUF_INIT;\n+\n+\tstrbuf_utf8_align(&s, align->position, align->width, stack->output.buf);\n+\tstrbuf_swap(&stack->output, &s);\n+\tstrbuf_release(&s);\n+\tfree(align);\n+}\n+\n+static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n+{\n+\tstruct ref_formatting_stack *new;\n+\n+\tpush_new_stack_element(&state->stack);\n+\tnew = state->stack;\n+\tnew->at_end = align_handler;\n+\tnew->cb_data = atomv->align;\n+}\n+\n+static void perform_quote_formatting(struct strbuf *s, const char *str, int quote_style);\n+\n+static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n+{\n+\tstruct ref_formatting_stack *current = state->stack;\n+\tstruct strbuf s = STRBUF_INIT;\n+\n+\tif (!current->at_end)\n+\t\tdie(_(\"format: `end` atom used without a supporting atom\"));\n+\tcurrent->at_end(current);\n+\t/*\n+\t * Whenever we have more than one stack element that means we\n+\t * are using a certain modifier atom. In that case we need to\n+\t * perform quote formatting.\n+\t */\n+\tif (!state->stack->prev->prev) {\n+\t\tperform_quote_formatting(&s, current->output.buf, state->quote_style);\n+\t\tstrbuf_reset(¤t->output);\n+\t\tstrbuf_addbuf(¤t->output, &s);\n+\t}\n+\tstrbuf_release(&s);\n+\tpop_stack_element(&state->stack);\n+}\n+\n /*\n * Parse the object referred by ref, and grab needed value.\n */\n@@ -660,6 +717,7 @@ static void populate_value(struct ref_array_item *ref)\n \t\tint deref = 0;\n \t\tconst char *refname;\n \t\tconst char *formatp;\n+\t\tconst char *valp;\n \t\tstruct branch *branch = NULL;\n \n \t\tif (*name == '*') {\n@@ -725,6 +783,35 @@ 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 (skip_prefix(name, \"align:\", &valp)) {\n+\t\t\tstruct align *align = xmalloc(sizeof(struct align));\n+\t\t\tstruct strbuf **s = strbuf_split_str(valp, ',', 0);\n+\n+\t\t\t/* If the position is given trim the ',' from the first strbuf */\n+\t\t\tif (s[1])\n+\t\t\t\tstrbuf_remove(s[0], s[0]->len - 1, 1);\n+\n+\t\t\tif (strtoul_ui(s[0]->buf, 10, &align->width))\n+\t\t\t\tdie(_(\"positive width expected align:%s\"), s[0]->buf);\n+\n+\t\t\t/* If no position is given, default to ALIGN_LEFT */\n+\t\t\tif (!s[1] || !strcmp(s[1]->buf, \"left\"))\n+\t\t\t\talign->position = ALIGN_LEFT;\n+\t\t\telse if (!strcmp(s[1]->buf, \"right\"))\n+\t\t\t\talign->position = ALIGN_RIGHT;\n+\t\t\telse if (!strcmp(s[1]->buf, \"middle\"))\n+\t\t\t\talign->position = ALIGN_MIDDLE;\n+\t\t\telse\n+\t\t\t\tdie(_(\"improper format entered align:%s\"), s[1]->buf);\n+\n+\t\t\tstrbuf_list_free(s);\n+\n+\t\t\tv->align = align;\n+\t\t\tv->handler = align_atom_handler;\n+\t\t\tcontinue;\n+\t\t} else if (!strcmp(name, \"end\")) {\n+\t\t\tv->handler = end_atom_handler;\n+\t\t\tcontinue;\n \t\t} else\n \t\t\tcontinue;\n \n@@ -1228,29 +1315,33 @@ 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 append_atom(struct atom_value *v, struct ref_formatting_state *state)\n+static void perform_quote_formatting(struct strbuf *s, const char *str, int quote_style)\n {\n-\tstruct strbuf *s = &state->stack->output;\n-\n-\tswitch (state->quote_style) {\n+\tswitch (quote_style) {\n \tcase QUOTE_NONE:\n-\t\tstrbuf_addstr(s, v->s);\n+\t\tstrbuf_addstr(s, str);\n \t\tbreak;\n \tcase QUOTE_SHELL:\n-\t\tsq_quote_buf(s, v->s);\n+\t\tsq_quote_buf(s, str);\n \t\tbreak;\n \tcase QUOTE_PERL:\n-\t\tperl_quote_buf(s, v->s);\n+\t\tperl_quote_buf(s, str);\n \t\tbreak;\n \tcase QUOTE_PYTHON:\n-\t\tpython_quote_buf(s, v->s);\n+\t\tpython_quote_buf(s, str);\n \t\tbreak;\n \tcase QUOTE_TCL:\n-\t\ttcl_quote_buf(s, v->s);\n+\t\ttcl_quote_buf(s, str);\n \t\tbreak;\n \t}\n }\n \n+static void append_atom(struct atom_value *v, struct ref_formatting_state *state)\n+{\n+\tstruct strbuf *s = &state->stack->output;\n+\tperform_quote_formatting(s, v->s, state->quote_style);\n+}\n+\n static int hex1(char ch)\n {\n \tif ('0' <= ch && ch <= '9')\n@@ -1307,7 +1398,18 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n \t\tif (cp < sp)\n \t\t\tappend_literal(cp, sp, &state);\n \t\tget_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);\n-\t\tappend_atom(atomv, &state);\n+\t\t/*\n+\t\t * If the atom is a modifier atom, then call the handler function.\n+\t\t * Else, if this is the first element on the stack, then we need to\n+\t\t * format the atom as per the given quote. Else we just add the atom value\n+\t\t * to the current stack element and handle quote formatting at the end.\n+\t\t */\n+\t\tif (atomv->handler)\n+\t\t\tatomv->handler(atomv, &state);\n+\t\telse if (!state.stack->prev)\n+\t\t\tappend_atom(atomv, &state);\n+\t\telse\n+\t\t\tstrbuf_addstr(&state.stack->output, atomv->s);\n \t}\n \tif (*cp) {\n \t\tsp = cp + strlen(cp);\n@@ -1322,6 +1424,8 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n \t\tresetv.s = color;\n \t\tappend_atom(&resetv, &state);\n \t}\n+\tif (state.stack->prev)\n+\t\tdie(_(\"format: `end` atom missing\"));\n \tfinal_buf = &state.stack->output;\n \tfwrite(final_buf->buf, 1, final_buf->len, stdout);\n \tpop_stack_element(&state.stack);\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 505a360..227992b 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -81,4 +81,73 @@ 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:30,left)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:30,middle)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:30,right)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+# Everything in between the %(align)...%(end) atom must be quoted, hence we test this by\n+# introducing single quote's in %(align)...%(end), which must not be escaped.\n+\n+sq=\"'\"\n+\n+test_expect_success 'alignment with format quote' '\n+\tcat >expect <<-EOF &&\n+\trefname is ${sq} ${sq}\\\\${sq}${sq}master${sq}\\\\${sq}${sq} ${sq}|\n+\trefname is ${sq} ${sq}\\\\${sq}${sq}side${sq}\\\\${sq}${sq} ${sq}|\n+\trefname is ${sq} ${sq}\\\\${sq}${sq}odd/spot${sq}\\\\${sq}${sq} ${sq}|\n+\trefname is ${sq} ${sq}\\\\${sq}${sq}double-tag${sq}\\\\${sq}${sq} ${sq}|\n+\trefname is ${sq} ${sq}\\\\${sq}${sq}four${sq}\\\\${sq}${sq} ${sq}|\n+\trefname is ${sq} ${sq}\\\\${sq}${sq}one${sq}\\\\${sq}${sq} ${sq}|\n+\trefname is ${sq} ${sq}\\\\${sq}${sq}signed-tag${sq}\\\\${sq}${sq} ${sq}|\n+\trefname is ${sq} ${sq}\\\\${sq}${sq}three${sq}\\\\${sq}${sq} ${sq}|\n+\trefname is ${sq} ${sq}\\\\${sq}${sq}two${sq}\\\\${sq}${sq} ${sq}|\n+\tEOF\n+\tgit for-each-ref --shell --format=\"refname is %(align:30,middle)${sq}%(refname:short)${sq}%(end)|\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.5.0\n"},{"id":"268489","messageId":"1440214788-1309-6-git-send-email-Karthik.188@gmail.com","threadId":"40155","inReplyTo":"1440214788-1309-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v13 05/12] ref-filter: add option to filter out tags, branches and remotes","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-22T03:39:41Z","receivedAt":"2015-08-22T03:39:41Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"From: Karthik Nayak \n\nAdd a function called 'for_each_reftype_fullpath()' to refs.{c,h}\nwhich iterates through each ref for the given path without trimming\nthe path and also accounting for broken refs, if mentioned.\n\nAdd 'filter_ref_kind()' in ref-filter.c to check the kind of ref being\nhandled and return the kind to 'ref_filter_handler()', where we\ndiscard refs which we do not need and assign the kind to needed refs.\n\nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----\n ref-filter.h | 12 ++++++++++--\n refs.c | 9 +++++++++\n refs.h | 1 +\n 4 files changed, 74 insertions(+), 7 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex ffec10a..d5fae1a 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1123,6 +1123,36 @@ static struct ref_array_item *new_ref_array_item(const char *refname,\n \treturn ref;\n }\n \n+static int filter_ref_kind(struct ref_filter *filter, const char *refname)\n+{\n+\tunsigned int i;\n+\n+\tstatic struct {\n+\t\tconst char *prefix;\n+\t\tunsigned int kind;\n+\t} ref_kind[] = {\n+\t\t{ \"refs/heads/\" , FILTER_REFS_BRANCHES },\n+\t\t{ \"refs/remotes/\" , FILTER_REFS_REMOTES },\n+\t\t{ \"refs/tags/\", FILTER_REFS_TAGS}\n+\t};\n+\n+\tif (filter->kind == FILTER_REFS_BRANCHES)\n+\t\treturn FILTER_REFS_BRANCHES;\n+\telse if (filter->kind == FILTER_REFS_REMOTES)\n+\t\treturn FILTER_REFS_REMOTES;\n+\telse if (filter->kind == FILTER_REFS_TAGS)\n+\t\treturn FILTER_REFS_TAGS;\n+\telse if (!strcmp(refname, \"HEAD\"))\n+\t\treturn FILTER_REFS_DETACHED_HEAD;\n+\n+\tfor (i = 0; i < ARRAY_SIZE(ref_kind); i++) {\n+\t\tif (starts_with(refname, ref_kind[i].prefix))\n+\t\t\treturn ref_kind[i].kind;\n+\t}\n+\n+\treturn FILTER_REFS_OTHERS;\n+}\n+\n /*\n * A call-back given to for_each_ref(). Filter refs and keep them for\n * later object processing.\n@@ -1133,6 +1163,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n \tstruct ref_filter *filter = ref_cbdata->filter;\n \tstruct ref_array_item *ref;\n \tstruct commit *commit = NULL;\n+\tunsigned int kind;\n \n \tif (flag & REF_BAD_NAME) {\n \t\twarning(\"ignoring ref with broken name %s\", refname);\n@@ -1144,6 +1175,10 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n \t\treturn 0;\n \t}\n \n+\tkind = filter_ref_kind(filter, refname);\n+\tif (!(kind & filter->kind))\n+\t\treturn 0;\n+\n \tif (*filter->name_patterns && !match_name_as_path(filter->name_patterns, refname))\n \t\treturn 0;\n \n@@ -1175,6 +1210,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n \n \tREALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);\n \tref_cbdata->array->items[ref_cbdata->array->nr++] = ref;\n+\tref->kind = kind;\n \treturn 0;\n }\n \n@@ -1251,16 +1287,29 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int\n {\n \tstruct ref_filter_cbdata ref_cbdata;\n \tint ret = 0;\n+\tunsigned int broken = 0;\n \n \tref_cbdata.array = array;\n \tref_cbdata.filter = filter;\n \n \t/* Simple per-ref filtering */\n-\tif (type & (FILTER_REFS_ALL | FILTER_REFS_INCLUDE_BROKEN))\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)\n+\tif (type & FILTER_REFS_INCLUDE_BROKEN) {\n+\t\ttype &= ~FILTER_REFS_INCLUDE_BROKEN;\n+\t\tbroken = 1;\n+\t}\n+\n+\tfilter->kind = type;\n+\tif (type == FILTER_REFS_BRANCHES)\n+\t\tret = for_each_reftype_fullpath(ref_filter_handler, \"refs/heads/\", broken, &ref_cbdata);\n+\telse if (type == FILTER_REFS_REMOTES)\n+\t\tret = for_each_reftype_fullpath(ref_filter_handler, \"refs/remotes/\", broken, &ref_cbdata);\n+\telse if (type == FILTER_REFS_TAGS)\n+\t\tret = for_each_reftype_fullpath(ref_filter_handler, \"refs/tags/\", broken, &ref_cbdata);\n+\telse if (type & FILTER_REFS_ALL) {\n+\t\tret = for_each_reftype_fullpath(ref_filter_handler, \"\", broken, &ref_cbdata);\n+\t\tif (type & FILTER_REFS_DETACHED_HEAD)\n+\t\t\thead_ref(ref_filter_handler, &ref_cbdata);\n+\t} else\n \t\tdie(\"filter_refs: invalid type\");\n \n \t/* Filters that need revision walking */\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 45026d0..99f081b 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -13,8 +13,14 @@\n #define QUOTE_PYTHON 4\n #define QUOTE_TCL 8\n \n-#define FILTER_REFS_INCLUDE_BROKEN 0x1\n-#define FILTER_REFS_ALL 0x2\n+#define FILTER_REFS_INCLUDE_BROKEN 0x0001\n+#define FILTER_REFS_TAGS 0x0002\n+#define FILTER_REFS_BRANCHES 0x0004\n+#define FILTER_REFS_REMOTES 0x0008\n+#define FILTER_REFS_OTHERS 0x0010\n+#define FILTER_REFS_ALL (FILTER_REFS_TAGS | FILTER_REFS_BRANCHES | \\\n+\t\t\t\t FILTER_REFS_REMOTES | FILTER_REFS_OTHERS)\n+#define FILTER_REFS_DETACHED_HEAD 0x0020\n \n struct atom_value;\n \n@@ -27,6 +33,7 @@ struct ref_sorting {\n struct ref_array_item {\n \tunsigned char objectname[20];\n \tint flag;\n+\tunsigned int kind;\n \tconst char *symref;\n \tstruct commit *commit;\n \tstruct atom_value *value;\n@@ -51,6 +58,7 @@ struct ref_filter {\n \tstruct commit *merge_commit;\n \n \tunsigned int with_commit_tag_algo : 1;\n+\tunsigned int kind;\n };\n \n struct ref_filter_cbdata {\ndiff --git a/refs.c b/refs.c\nindex 4e15f60..3266617 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2150,6 +2150,15 @@ int for_each_replace_ref(each_ref_fn fn, void *cb_data)\n \t\t\t strlen(git_replace_ref_base), 0, cb_data);\n }\n \n+int for_each_reftype_fullpath(each_ref_fn fn, char *type, unsigned int broken, void *cb_data)\n+{\n+\tunsigned int flag = 0;\n+\n+\tif (broken)\n+\t\tflag = DO_FOR_EACH_INCLUDE_BROKEN;\n+\treturn do_for_each_ref(&ref_cache, type, fn, 0, flag, cb_data);\n+}\n+\n int head_ref_namespaced(each_ref_fn fn, void *cb_data)\n {\n \tstruct strbuf buf = STRBUF_INIT;\ndiff --git a/refs.h b/refs.h\nindex e9a5f32..6e913ee 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -179,6 +179,7 @@ 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 extern int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data);\n extern int for_each_glob_ref_in(each_ref_fn fn, const char *pattern, const char *prefix, void *cb_data);\n+extern int for_each_reftype_fullpath(each_ref_fn fn, char *type, unsigned int broken, void *cb_data);\n \n extern int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);\n extern int for_each_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);\n-- \n2.5.0\n"},{"id":"268496","messageId":"1440214788-1309-7-git-send-email-Karthik.188@gmail.com","threadId":"40155","inReplyTo":"1440214788-1309-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v13 06/12] ref-filter: support printing N lines from tag annotation","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-22T03:39:42Z","receivedAt":"2015-08-22T03:39:42Z","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 | 51 +++++++++++++++++++++++++++++++++++++++++++++++++-\n ref-filter.h | 11 ++++++++---\n 4 files changed, 63 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 d5fae1a..515147b 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1431,7 +1431,51 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting\n \t}\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 *final_buf;\n@@ -1478,6 +1522,11 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n \tfinal_buf = &state.stack->output;\n \tfwrite(final_buf->buf, 1, final_buf->len, stdout);\n \tpop_stack_element(&state.stack);\n+\tif (lines > 0) {\n+\t\tstruct object_id oid;\n+\t\thashcpy(oid.hash, info->objectname);\n+\t\tshow_tag_lines(&oid, lines);\n+\t}\n \tputchar('\\n');\n }\n \ndiff --git a/ref-filter.h b/ref-filter.h\nindex 99f081b..c599ea2 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -58,7 +58,8 @@ struct ref_filter {\n \tstruct commit *merge_commit;\n \n \tunsigned int with_commit_tag_algo : 1;\n-\tunsigned int kind;\n+\tunsigned int kind,\n+\t\tlines;\n };\n \n struct ref_filter_cbdata {\n@@ -90,8 +91,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":"268492","messageId":"1440214788-1309-8-git-send-email-Karthik.188@gmail.com","threadId":"40155","inReplyTo":"1440214788-1309-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v13 07/12] ref-filter: add support to sort by version","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-22T03:39:43Z","receivedAt":"2015-08-22T03:39:43Z","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 eventually 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 943975d..06d468e 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -154,6 +154,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 515147b..b4a7d72 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@@ -1327,19 +1329,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@@ -1559,6 +1561,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 c599ea2..5aa2f40 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -27,7 +27,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 227992b..38c99c9 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -150,4 +150,40 @@ test_expect_success 'alignment with format quote' '\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":"268491","messageId":"1440214788-1309-9-git-send-email-Karthik.188@gmail.com","threadId":"40155","inReplyTo":"1440214788-1309-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v13 08/12] ref-filter: add option to match literal pattern","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-22T03:39:44Z","receivedAt":"2015-08-22T03:39:44Z","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 | 40 +++++++++++++++++++++++++++++++++++++---\n ref-filter.h | 3 ++-\n 3 files changed, 40 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 b4a7d72..f8b8fb7 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1055,9 +1055,33 @@ 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+\t skip_prefix(refname, \"refs/\", &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@@ -1078,6 +1102,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@@ -1181,7 +1215,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n \tif (!(kind & filter->kind))\n \t\treturn 0;\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 5aa2f40..8241066 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -58,7 +58,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 kind,\n \t\tlines;\n };\n-- \n2.5.0\n"},{"id":"268493","messageId":"1440214788-1309-10-git-send-email-Karthik.188@gmail.com","threadId":"40155","inReplyTo":"1440214788-1309-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v13 09/12] tag.c: use 'ref-filter' data structures","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-22T03:39:45Z","receivedAt":"2015-08-22T03:39:45Z","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":"268495","messageId":"1440214788-1309-11-git-send-email-Karthik.188@gmail.com","threadId":"40155","inReplyTo":"1440214788-1309-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v13 10/12] tag.c: use 'ref-filter' APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-22T03:39:46Z","receivedAt":"2015-08-22T03:39:46Z","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..501fc52 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:16,left)%(refname:short)%(end)\";\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..84153ef 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*\"\n '\n \n test_expect_success 'version sort with prerelease reordering' '\n-- \n2.5.0\n"},{"id":"268497","messageId":"1440214788-1309-12-git-send-email-Karthik.188@gmail.com","threadId":"40155","inReplyTo":"1440214788-1309-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v13 11/12] tag.c: implement '--format' option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-22T03:39:47Z","receivedAt":"2015-08-22T03:39:47Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Implement 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 | 8 +++++++-\n builtin/tag.c | 19 +++++++++++--------\n t/t7004-tag.sh | 12 ++++++++++++\n 3 files changed, 30 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 3ac4a96..0c7f4e6 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,11 @@ 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. The format is the same as\n+\tthat of linkgit:git-for-each-ref[1]. When unspecified,\n+\tdefaults to `%(refname:short)`.\n \n CONFIGURATION\n -------------\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 501fc52..4b8d6df 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -23,17 +23,16 @@ 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[--format=] [...]\"),\n \tN_(\"git tag -v ...\"),\n \tNULL\n };\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@@ -41,10 +40,12 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting)\n \tif (filter->lines == -1)\n \t\tfilter->lines = 0;\n \n-\tif (filter->lines)\n-\t\tformat = \"%(align:16,left)%(refname:short)%(end)\";\n-\telse\n-\t\tformat = \"%(refname:short)\";\n+\tif (!format) {\n+\t\tif (filter->lines)\n+\t\t\tformat = \"%(align:16,left)%(refname:short)%(end)\";\n+\t\telse\n+\t\t\tformat = \"%(refname:short)\";\n+\t}\n \n \tverify_ref_format(format);\n \tfilter_refs(&array, filter, FILTER_REFS_TAGS);\n@@ -327,6 +328,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 +361,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@@ -399,7 +402,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\t\trun_column_filter(colopts, &copts);\n \t\t}\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 84153ef..8987fb1 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1519,4 +1519,16 @@ EOF\"\n \ttest_cmp expect actual\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":"268494","messageId":"1440214788-1309-13-git-send-email-Karthik.188@gmail.com","threadId":"40155","inReplyTo":"1440214788-1309-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v13 12/12] tag.c: implement '--merged' and '--no-merged' options","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-22T03:39:48Z","receivedAt":"2015-08-22T03:39:48Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Use 'ref-filter' APIs to implement the '--merged' and '--no-merged'\noptions into 'tag.c'. The '--merged' option lets the user to only list\ntags merged into the named commit. The '--no-merged' option lets the\nuser to only list tags not merged into the named commit. If no object\nis 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 0c7f4e6..3803bf7 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@@ -165,6 +165,11 @@ This option is only applicable when listing tags without annotation lines.\n \tthat of linkgit:git-for-each-ref[1]. When unspecified,\n \tdefaults to `%(refname:short)`.\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 CONFIGURATION\n -------------\n By default, 'git tag' in sign-with-default mode (-s) will use your\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 4b8d6df..bbbcaed 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[--format=] [...]\"),\n+\t\t\"\\n\\t\\t[--format=] [--[no-]merged []] [...]\"),\n \tN_(\"git tag -v ...\"),\n \tNULL\n };\n@@ -355,6 +355,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 8987fb1..3dd2f51 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1531,4 +1531,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":"268545","messageId":"vpq37z9lqwp.fsf@anie.imag.fr","threadId":"40155","inReplyTo":"1440214788-1309-12-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v13 11/12] tag.c: implement '--format' option","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-08-23T19:56:54Z","receivedAt":"2015-08-23T19:56:54Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Karthik Nayak writes:\n\n> --- a/Documentation/git-tag.txt\n> +++ b/Documentation/git-tag.txt\n> @@ -13,7 +13,8 @@ SYNOPSIS\n> \t [ | ]\n> 'git tag' -d ...\n> 'git tag' [-n[]] -l [--contains ] [--points-at ]\n> -\t[--column[=] | --no-column] [--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,11 @@ 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\nShouldn't this be --format , not just ? We usually use\nthe named argument in the SYNOPSIS for positional arguments, but not for\narguments following an option.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"268546","messageId":"vpqzj1hkc5q.fsf@anie.imag.fr","threadId":"40155","inReplyTo":"1440214788-1309-1-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-08-23T20:00:49Z","receivedAt":"2015-08-23T20:00:49Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Karthik Nayak writes:\n\n> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\n> index 1997657..06d468e 100644\n> --- a/Documentation/git-for-each-ref.txt\n> +++ b/Documentation/git-for-each-ref.txt\n> @@ -133,7 +133,8 @@ align::\n> \t`` is either left, right or middle and `` is\n> \tthe total length of the content with alignment. If the\n> \tcontents length is more than the width then no alignment is\n> -\tperformed.\n> +\tperformed. If used with '--quote' everything in between %(align:..)\n> +\tand %(end) is quoted.\n\nThere's no --quote, there are --shell, --python, ... (well, actually, I\nwould have prefered to have a single --quote=language option, but that's\nnot how it is now).\n\nI had already commented on a preliminary version of this series\noff-list. I think all my previous comments have been taken into account.\n\nThanks,\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"268570","messageId":"CAOLa=ZR4LJSou5QqmZJm1M49tD74t4Besb64F+XOHefjnxo7xw@mail.gmail.com","threadId":"40155","inReplyTo":"vpq37z9lqwp.fsf@anie.imag.fr","subject":"Re: [PATCH v13 11/12] tag.c: implement '--format' option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-24T15:07:45Z","receivedAt":"2015-08-24T15:07:45Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Mon, Aug 24, 2015 at 1:26 AM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> --- a/Documentation/git-tag.txt\n>> +++ b/Documentation/git-tag.txt\n>> @@ -13,7 +13,8 @@ SYNOPSIS\n>> [ | ]\n>> 'git tag' -d ...\n>> 'git tag' [-n[]] -l [--contains ] [--points-at ]\n>> - [--column[=] | --no-column] [--create-reflog] [--sort=] [...]\n>> + [--column[=] | --no-column] [--create-reflog] [--sort=]\n>> + [--format=] [...]\n>> 'git tag' -v ...\n>>\n>> DESCRIPTION\n>> @@ -158,6 +159,11 @@ This option is only applicable when listing tags without annotation lines.\n>> The object that the new tag will refer to, usually a commit.\n>> Defaults to HEAD.\n>>\n>> +::\n>\n> Shouldn't this be --format , not just ? We usually use\n> the named argument in the SYNOPSIS for positional arguments, but not for\n> arguments following an option.\n>\n\nThis is how it was in for-each-ref Documentation, hence to keep it similar I\njust put .\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268571","messageId":"CAOLa=ZQx_eWdq5cLskAT_c1OjZ-7DwWwZTC9S=ZafEDYHFGncg@mail.gmail.com","threadId":"40155","inReplyTo":"vpqzj1hkc5q.fsf@anie.imag.fr","subject":"Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-24T15:09:51Z","receivedAt":"2015-08-24T15:09:51Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Mon, Aug 24, 2015 at 1:30 AM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\n>> index 1997657..06d468e 100644\n>> --- a/Documentation/git-for-each-ref.txt\n>> +++ b/Documentation/git-for-each-ref.txt\n>> @@ -133,7 +133,8 @@ align::\n>> `` is either left, right or middle and `` is\n>> the total length of the content with alignment. If the\n>> contents length is more than the width then no alignment is\n>> - performed.\n>> + performed. If used with '--quote' everything in between %(align:..)\n>> + and %(end) is quoted.\n>\n> There's no --quote, there are --shell, --python, ... (well, actually, I\n> would have prefered to have a single --quote=language option, but that's\n> not how it is now).\n\nThat'd be easy to implement, but I didn't because `git tag -l` is\nhuman readable and\nI didn't see the necessity for having something like `--` here.\n\nIt'd be better to just use `git for-each-ref refs/tags/` for that.\n\n>\n> I had already commented on a preliminary version of this series\n> off-list. I think all my previous comments have been taken into account.\n>\n> Thanks,\n>\n\nThanks for the review :)\n\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268572","messageId":"vpq37z8afci.fsf@anie.imag.fr","threadId":"40155","inReplyTo":"CAOLa=ZR4LJSou5QqmZJm1M49tD74t4Besb64F+XOHefjnxo7xw@mail.gmail.com","subject":"Re: [PATCH v13 11/12] tag.c: implement '--format' option","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-08-24T15:14:21Z","receivedAt":"2015-08-24T15:14:21Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Karthik Nayak writes:\n\n> On Mon, Aug 24, 2015 at 1:26 AM, Matthieu Moy\n> wrote:\n>> Karthik Nayak writes:\n>>\n>>> --- a/Documentation/git-tag.txt\n>>> +++ b/Documentation/git-tag.txt\n>>> @@ -13,7 +13,8 @@ SYNOPSIS\n>>> [ | ]\n>>> 'git tag' -d ...\n>>> 'git tag' [-n[]] -l [--contains ] [--points-at ]\n>>> - [--column[=] | --no-column] [--create-reflog] [--sort=] [...]\n>>> + [--column[=] | --no-column] [--create-reflog] [--sort=]\n>>> + [--format=] [...]\n>>> 'git tag' -v ...\n>>>\n>>> DESCRIPTION\n>>> @@ -158,6 +159,11 @@ This option is only applicable when listing tags without annotation lines.\n>>> The object that the new tag will refer to, usually a commit.\n>>> Defaults to HEAD.\n>>>\n>>> +::\n>>\n>> Shouldn't this be --format , not just ? We usually use\n>> the named argument in the SYNOPSIS for positional arguments, but not for\n>> arguments following an option.\n>\n> This is how it was in for-each-ref Documentation, hence to keep it similar I\n> just put .\n\n\"It's wrong in another place\" sounds like an argument to fix the other\nplace, not to get it wrong here too ;-).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"268573","messageId":"vpqvbc490oh.fsf@anie.imag.fr","threadId":"40155","inReplyTo":"CAOLa=ZQx_eWdq5cLskAT_c1OjZ-7DwWwZTC9S=ZafEDYHFGncg@mail.gmail.com","subject":"Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-08-24T15:16:30Z","receivedAt":"2015-08-24T15:16:30Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Karthik Nayak writes:\n\n> On Mon, Aug 24, 2015 at 1:30 AM, Matthieu Moy\n> wrote:\n>> Karthik Nayak writes:\n>>\n>>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\n>>> index 1997657..06d468e 100644\n>>> --- a/Documentation/git-for-each-ref.txt\n>>> +++ b/Documentation/git-for-each-ref.txt\n>>> @@ -133,7 +133,8 @@ align::\n>>> `` is either left, right or middle and `` is\n>>> the total length of the content with alignment. If the\n>>> contents length is more than the width then no alignment is\n>>> - performed.\n>>> + performed. If used with '--quote' everything in between %(align:..)\n>>> + and %(end) is quoted.\n>>\n>> There's no --quote, there are --shell, --python, ... (well, actually, I\n>> would have prefered to have a single --quote=language option, but that's\n>> not how it is now).\n>\n> That'd be easy to implement, but I didn't because `git tag -l` is\n> human readable and\n> I didn't see the necessity for having something like `--` here.\n\nAgreed: tag does not have --shell, --python & so and does not need it.\n\nBut that's not my point: you write \"if used with '--quote'\", and the\noption name is not --quote. It should be \"if used with `--shell`,\n`--python`, ... then everything ...\".\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"268574","messageId":"CAOLa=ZR6L1LjaD2pTsN1BCh5fHk7CvsE_9KeS3F4f1+RDDzrXw@mail.gmail.com","threadId":"40155","inReplyTo":"vpq37z8afci.fsf@anie.imag.fr","subject":"Re: [PATCH v13 11/12] tag.c: implement '--format' option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-24T15:21:54Z","receivedAt":"2015-08-24T15:21:54Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Mon, Aug 24, 2015 at 8:44 PM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> On Mon, Aug 24, 2015 at 1:26 AM, Matthieu Moy\n>> wrote:\n>>> Karthik Nayak writes:\n>>>\n>>>> --- a/Documentation/git-tag.txt\n>>>> +++ b/Documentation/git-tag.txt\n>>>> @@ -13,7 +13,8 @@ SYNOPSIS\n>>>> [ | ]\n>>>> 'git tag' -d ...\n>>>> 'git tag' [-n[]] -l [--contains ] [--points-at ]\n>>>> - [--column[=] | --no-column] [--create-reflog] [--sort=] [...]\n>>>> + [--column[=] | --no-column] [--create-reflog] [--sort=]\n>>>> + [--format=] [...]\n>>>> 'git tag' -v ...\n>>>>\n>>>> DESCRIPTION\n>>>> @@ -158,6 +159,11 @@ This option is only applicable when listing tags without annotation lines.\n>>>> The object that the new tag will refer to, usually a commit.\n>>>> Defaults to HEAD.\n>>>>\n>>>> +::\n>>>\n>>> Shouldn't this be --format , not just ? We usually use\n>>> the named argument in the SYNOPSIS for positional arguments, but not for\n>>> arguments following an option.\n>>\n>> This is how it was in for-each-ref Documentation, hence to keep it similar I\n>> just put .\n>\n> \"It's wrong in another place\" sounds like an argument to fix the other\n> place, not to get it wrong here too ;-).\n>\n\nOf course! That was just me justifying my action. I agree, it should\nbe corrected both places.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268575","messageId":"CAOLa=ZRbWDBEoSkeGRdL2qS5BCEka=zmJL0JXdE9fvomrmOGcQ@mail.gmail.com","threadId":"40155","inReplyTo":"vpqvbc490oh.fsf@anie.imag.fr","subject":"Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-24T15:22:45Z","receivedAt":"2015-08-24T15:22:45Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Mon, Aug 24, 2015 at 8:46 PM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> On Mon, Aug 24, 2015 at 1:30 AM, Matthieu Moy\n>> wrote:\n>>> Karthik Nayak writes:\n>>>\n>>>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\n>>>> index 1997657..06d468e 100644\n>>>> --- a/Documentation/git-for-each-ref.txt\n>>>> +++ b/Documentation/git-for-each-ref.txt\n>>>> @@ -133,7 +133,8 @@ align::\n>>>> `` is either left, right or middle and `` is\n>>>> the total length of the content with alignment. If the\n>>>> contents length is more than the width then no alignment is\n>>>> - performed.\n>>>> + performed. If used with '--quote' everything in between %(align:..)\n>>>> + and %(end) is quoted.\n>>>\n>>> There's no --quote, there are --shell, --python, ... (well, actually, I\n>>> would have prefered to have a single --quote=language option, but that's\n>>> not how it is now).\n>>\n>> That'd be easy to implement, but I didn't because `git tag -l` is\n>> human readable and\n>> I didn't see the necessity for having something like `--` here.\n>\n> Agreed: tag does not have --shell, --python & so and does not need it.\n>\n> But that's not my point: you write \"if used with '--quote'\", and the\n> option name is not --quote. It should be \"if used with `--shell`,\n> `--python`, ... then everything ...\".\n>\n\nOops! misread what you sent, maybe' --' instead.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268587","messageId":"xmqqy4h04mx4.fsf@gitster.dls.corp.google.com","threadId":"40155","inReplyTo":"1440214788-1309-1-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-24T17:27:19Z","receivedAt":"2015-08-24T17:27:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> Interdiff:\n>\n> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\n> index 1997657..06d468e 100644\n> --- a/Documentation/git-for-each-ref.txt\n> +++ b/Documentation/git-for-each-ref.txt\n> @@ -133,7 +133,8 @@ align::\n> \t`` is either left, right or middle and `` is\n> \tthe total length of the content with alignment. If the\n> \tcontents length is more than the width then no alignment is\n> -\tperformed.\n> +\tperformed. If used with '--quote' everything in between %(align:..)\n> +\tand %(end) is quoted.\n\nThat sounds like a buggy behaviour that we may want to fix later,\nthough. Perhaps document it as a known bug, e.g. \"Currently it does\nnot work well when used with language-specific quoting like --shell,\netc.\" (while punting on fixing the implementation for now)?\n"},{"id":"268592","messageId":"CAOLa=ZRYWNVmMeMCZSa+GcOTQ7NzuVB9RH_TFJ6-u7aFhrU=NA@mail.gmail.com","threadId":"40155","inReplyTo":"xmqqy4h04mx4.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-24T18:09:06Z","receivedAt":"2015-08-24T18:09:06Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Mon, Aug 24, 2015 at 10:57 PM, Junio C Hamano wrote:\n> Karthik Nayak writes:\n>\n>> Interdiff:\n>>\n>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\n>> index 1997657..06d468e 100644\n>> --- a/Documentation/git-for-each-ref.txt\n>> +++ b/Documentation/git-for-each-ref.txt\n>> @@ -133,7 +133,8 @@ align::\n>> `` is either left, right or middle and `` is\n>> the total length of the content with alignment. If the\n>> contents length is more than the width then no alignment is\n>> - performed.\n>> + performed. If used with '--quote' everything in between %(align:..)\n>> + and %(end) is quoted.\n>\n> That sounds like a buggy behaviour that we may want to fix later,\n> though. Perhaps document it as a known bug, e.g. \"Currently it does\n> not work well when used with language-specific quoting like --shell,\n> etc.\" (while punting on fixing the implementation for now)?\n>\n\nI might have misunderstood you, But based on the discussion held here\n(thread.gmane.org/gmane.comp.version-control.git/276140)\nI thought we wanted everything inside the %(align) .... %(end) atoms\nto be quoted.\nSo I'm a little confused by what you're trying to say.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268598","messageId":"xmqq8u904ix0.fsf@gitster.dls.corp.google.com","threadId":"40155","inReplyTo":"CAOLa=ZRYWNVmMeMCZSa+GcOTQ7NzuVB9RH_TFJ6-u7aFhrU=NA@mail.gmail.com","subject":"Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-24T18:53:47Z","receivedAt":"2015-08-24T18:53:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> On Mon, Aug 24, 2015 at 10:57 PM, Junio C Hamano wrote:\n>> Karthik Nayak writes:\n>> ...\n>>> + performed. If used with '--quote' everything in between %(align:..)\n>>> + and %(end) is quoted.\n> ...\n> I might have misunderstood you, But based on the discussion held here\n> (thread.gmane.org/gmane.comp.version-control.git/276140)\n> I thought we wanted everything inside the %(align) .... %(end) atoms\n> to be quoted.\n\nPerhaps I misunderstood your intention in the doc.\n\t\nI took \"everything in between %(align:...) and %(end) is quoted\" to\nmean that\n\n\t%(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end)\n\ncan never satisfy %(if:empty), because %(align)%(end) would expand\nto a string that has two single-quotes, that is not an empty string.\n\nIf that is not what would happen in the \"branch --list\" enhancement,\nthen the proposed behaviour is good, but the above documentation would\nneed to be updated when it happens, I think. It at least is misleading.\n\nThanks.\n"},{"id":"268610","messageId":"xmqqr3ms2vvh.fsf@gitster.dls.corp.google.com","threadId":"40155","inReplyTo":"1440214788-1309-3-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v13 02/12] ref-filter: introduce ref_formatting_state and ref_formatting_stack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-24T21:56:50Z","receivedAt":"2015-08-24T21:56:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> +static void push_new_stack_element(struct ref_formatting_stack **stack)\n> +{\n\nMicronit. Perhaps s/_new//;, as you do not call the other function\npop_old_stack_element().\n\nThe remainder of this step looks pretty straight-forward and was a\npleasant read.\n\nThanks.\n"},{"id":"268611","messageId":"xmqqmvxg2v3u.fsf@gitster.dls.corp.google.com","threadId":"40155","inReplyTo":"1440214788-1309-5-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v13 04/12] ref-filter: implement an `align` atom","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-24T22:13:25Z","receivedAt":"2015-08-24T22:13:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> +static void perform_quote_formatting(struct strbuf *s, const char *str, int quote_style);\n> +\n> +static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n> +{\n> +\tstruct ref_formatting_stack *current = state->stack;\n> +\tstruct strbuf s = STRBUF_INIT;\n> +\n> +\tif (!current->at_end)\n> +\t\tdie(_(\"format: `end` atom used without a supporting atom\"));\n> +\tcurrent->at_end(current);\n> +\t/*\n> +\t * Whenever we have more than one stack element that means we\n> +\t * are using a certain modifier atom. In that case we need to\n> +\t * perform quote formatting.\n> +\t */\n> +\tif (!state->stack->prev->prev) {\n\nThe comment and the condition seem to be saying opposite things.\nThe code says \"If the stack only has one prev that is the very\ninitial one, then we do the quoting, i.e. the result of expanding\nthe enclosed string in %(start-something)...%(end) is quoted only\nwhen that appears at the top level\", which feels more correct than\nthe comment that says \"if we are about to pop after seeing the\nfirst %(end) in %(start)...%(another)...%(end)...%(end) sequence,\nwe quote what is between %(another)...%(end)\".\n\n> +\t\tperform_quote_formatting(&s, current->output.buf, state->quote_style);\n> +\t\tstrbuf_reset(¤t->output);\n> +\t\tstrbuf_addbuf(¤t->output, &s);\n> +\t}\n> +\tstrbuf_release(&s);\n> +\tpop_stack_element(&state->stack);\n> +}\n> +\n\n> @@ -1228,29 +1315,33 @@ 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 append_atom(struct atom_value *v, struct ref_formatting_state *state)\n> +static void perform_quote_formatting(struct strbuf *s, const char *str, int quote_style)\n> {\n> -\tstruct strbuf *s = &state->stack->output;\n> -\n> -\tswitch (state->quote_style) {\n> +\tswitch (quote_style) {\n> \tcase QUOTE_NONE:\n> -\t\tstrbuf_addstr(s, v->s);\n> +\t\tstrbuf_addstr(s, str);\n> \t\tbreak;\n> \tcase QUOTE_SHELL:\n> -\t\tsq_quote_buf(s, v->s);\n> +\t\tsq_quote_buf(s, str);\n> \t\tbreak;\n> \tcase QUOTE_PERL:\n> -\t\tperl_quote_buf(s, v->s);\n> +\t\tperl_quote_buf(s, str);\n> \t\tbreak;\n> \tcase QUOTE_PYTHON:\n> -\t\tpython_quote_buf(s, v->s);\n> +\t\tpython_quote_buf(s, str);\n> \t\tbreak;\n> \tcase QUOTE_TCL:\n> -\t\ttcl_quote_buf(s, v->s);\n> +\t\ttcl_quote_buf(s, str);\n> \t\tbreak;\n> \t}\n> }\n> \n> +static void append_atom(struct atom_value *v, struct ref_formatting_state *state)\n> +{\n> +\tstruct strbuf *s = &state->stack->output;\n> +\tperform_quote_formatting(s, v->s, state->quote_style);\n\nHmmm, do we want to unconditionally do the quote here, or only when\nwe are not being captured by upcoming %(end) to be consistent with\nthe behaviour of end_atom_handler() above?\n\n> @@ -1307,7 +1398,18 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n> \t\tif (cp < sp)\n> \t\t\tappend_literal(cp, sp, &state);\n> \t\tget_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);\n> -\t\tappend_atom(atomv, &state);\n> +\t\t/*\n> +\t\t * If the atom is a modifier atom, then call the handler function.\n> +\t\t * Else, if this is the first element on the stack, then we need to\n> +\t\t * format the atom as per the given quote. Else we just add the atom value\n> +\t\t * to the current stack element and handle quote formatting at the end.\n> +\t\t */\n> +\t\tif (atomv->handler)\n> +\t\t\tatomv->handler(atomv, &state);\n> +\t\telse if (!state.stack->prev)\n> +\t\t\tappend_atom(atomv, &state);\n> +\t\telse\n> +\t\t\tstrbuf_addstr(&state.stack->output, atomv->s);\n\nAhh, this explains why you are not doing it above, but I do not\nthink if this is a good division of labor.\n\nYou can see that I expected that \"if !state.stack->prev\" check to be\ninside append_atom(), and I would imagine future readers would have\nthe same expectation when reading this code. I.e.\n\n\tappend_atom(struct atom_value *v, struct ref_f_s *state)\n {\n\t\tif (state->stack->prev)\n\t\t\tstrbuf_addstr(&state->stack->output, v->s);\n\t\telse\n \tquote_format(&state->stack->output, v->s, state->quote_style);\n\t}\n\nThe end result may be the same, but I do think \"append_atom is to\nalways quote, so we do an unquoted appending by hand\" is a bad way\nto do this.\n\nMoreover, notice that the function signature of append_atom() is\nexactly the same as atomv->handler's. I wonder if it would be\neasier to understand if you made append_atom() the handler for a\nnon-magic atoms, which would let you do the above without any if/else\nand just a single unconditional\n\n\tatomv->handler(atomv, &state);\n"},{"id":"268612","messageId":"xmqqfv382uzo.fsf@gitster.dls.corp.google.com","threadId":"40155","inReplyTo":"xmqqmvxg2v3u.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v13 04/12] ref-filter: implement an `align` atom","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-24T22:15:55Z","receivedAt":"2015-08-24T22:15:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Karthik Nayak writes:\n>\n>> +static void end_atom_handler(struct atom_value *atomv, struct\n>> ref_formatting_state *state)\n>> +{\n>> +\tstruct ref_formatting_stack *current = state->stack;\n>> +\tstruct strbuf s = STRBUF_INIT;\n>> +\n>> +\tif (!current->at_end)\n>> +\t\tdie(_(\"format: `end` atom used without a supporting atom\"));\n>> +\tcurrent->at_end(current);\n>> +\t/*\n>> +\t * Whenever we have more than one stack element that means we\n>> +\t * are using a certain modifier atom. In that case we need to\n>> +\t * perform quote formatting.\n>> +\t */\n>> +\tif (!state->stack->prev->prev) {\n>\n> The comment and the condition seem to be saying opposite things.\n> The code says \"If the stack only has one prev that is the very\n> initial one, then we do the quoting, i.e. the result of expanding\n> the enclosed string in %(start-something)...%(end) is quoted only\n> when that appears at the top level\", which feels more correct...\n\nAs this already allows us to use nested construct, I think we would\nwant to have test that uses\n\n %(align:30,left)%(align:20,right)%(refname:short)%(end)%(end)\n\nor something like that ;-)\n\nVery nice.\n"},{"id":"268613","messageId":"xmqqbndw2ul1.fsf@gitster.dls.corp.google.com","threadId":"40155","inReplyTo":"1440214788-1309-6-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v13 05/12] ref-filter: add option to filter out tags, branches and remotes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-24T22:24:42Z","receivedAt":"2015-08-24T22:24:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> From: Karthik Nayak \n>\n> Add a function called 'for_each_reftype_fullpath()' to refs.{c,h}\n> which iterates through each ref for the given path without trimming\n> the path and also accounting for broken refs, if mentioned.\n\nFor this part, I think you would want to borrow an extra pair of\neyes from Michael.\n\n>\n> Add 'filter_ref_kind()' in ref-filter.c to check the kind of ref being\n> handled and return the kind to 'ref_filter_handler()', where we\n> discard refs which we do not need and assign the kind to needed refs.\n>\n> Mentored-by: Christian Couder \n> Mentored-by: Matthieu Moy \n> Signed-off-by: Karthik Nayak \n> ---\n> ref-filter.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----\n> ref-filter.h | 12 ++++++++++--\n> refs.c | 9 +++++++++\n> refs.h | 1 +\n> 4 files changed, 74 insertions(+), 7 deletions(-)\n>\n> diff --git a/ref-filter.c b/ref-filter.c\n> index ffec10a..d5fae1a 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -1123,6 +1123,36 @@ static struct ref_array_item *new_ref_array_item(const char *refname,\n> \treturn ref;\n> }\n> \n> +static int filter_ref_kind(struct ref_filter *filter, const char *refname)\n> +{\n> +\tunsigned int i;\n> +\n> +\tstatic struct {\n> +\t\tconst char *prefix;\n> +\t\tunsigned int kind;\n> +\t} ref_kind[] = {\n> +\t\t{ \"refs/heads/\" , FILTER_REFS_BRANCHES },\n> +\t\t{ \"refs/remotes/\" , FILTER_REFS_REMOTES },\n> +\t\t{ \"refs/tags/\", FILTER_REFS_TAGS}\n> +\t};\n> +\n> +\tif (filter->kind == FILTER_REFS_BRANCHES)\n> +\t\treturn FILTER_REFS_BRANCHES;\n> +\telse if (filter->kind == FILTER_REFS_REMOTES)\n> +\t\treturn FILTER_REFS_REMOTES;\n> +\telse if (filter->kind == FILTER_REFS_TAGS)\n> +\t\treturn FILTER_REFS_TAGS;\n> +\telse if (!strcmp(refname, \"HEAD\"))\n> +\t\treturn FILTER_REFS_DETACHED_HEAD;\n> +\n> +\tfor (i = 0; i < ARRAY_SIZE(ref_kind); i++) {\n> +\t\tif (starts_with(refname, ref_kind[i].prefix))\n> +\t\t\treturn ref_kind[i].kind;\n> +\t}\n> +\n> +\treturn FILTER_REFS_OTHERS;\n> +}\n> +\n> /*\n> * A call-back given to for_each_ref(). Filter refs and keep them for\n> * later object processing.\n> @@ -1133,6 +1163,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n> \tstruct ref_filter *filter = ref_cbdata->filter;\n> \tstruct ref_array_item *ref;\n> \tstruct commit *commit = NULL;\n> +\tunsigned int kind;\n> \n> \tif (flag & REF_BAD_NAME) {\n> \t\twarning(\"ignoring ref with broken name %s\", refname);\n> @@ -1144,6 +1175,10 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n> \t\treturn 0;\n> \t}\n> \n> +\tkind = filter_ref_kind(filter, refname);\n> +\tif (!(kind & filter->kind))\n> +\t\treturn 0;\n> +\n\nWhen filter->kind is set to some single-bit thing (e.g.\nFILTER_REFS_BRANCHES) by the caller of ref_filter_handler(), then\nthis call of filter_ref_kind() will just parrot that without even\nlooking at refname. And then the if statement says \"yes, they have\ncommon bit(s)\". Even when refname is \"refs/tags/v1.0\" or \"HEAD\".\n\nAnd if this code knows that \"refs/tags/v1.0\" or \"HEAD\" will never\ncome when filter->kind is exactly FILTER_REFS_BRANCHES, then I do\nnot see the point of calling filter_ref_kind().\n\nI am not sure what this is trying to do. Can you clarify the\nthinking behind this as a comment to filter_ref_kind()?\n\n> @@ -1175,6 +1210,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n> \n> \tREALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);\n> \tref_cbdata->array->items[ref_cbdata->array->nr++] = ref;\n> +\tref->kind = kind;\n> \treturn 0;\n> }\n> \n> @@ -1251,16 +1287,29 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int\n> {\n> \tstruct ref_filter_cbdata ref_cbdata;\n> \tint ret = 0;\n> +\tunsigned int broken = 0;\n> \n> \tref_cbdata.array = array;\n> \tref_cbdata.filter = filter;\n> \n> \t/* Simple per-ref filtering */\n> -\tif (type & (FILTER_REFS_ALL | FILTER_REFS_INCLUDE_BROKEN))\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)\n> +\tif (type & FILTER_REFS_INCLUDE_BROKEN) {\n> +\t\ttype &= ~FILTER_REFS_INCLUDE_BROKEN;\n> +\t\tbroken = 1;\n> +\t}\n> +\n> +\tfilter->kind = type;\n> +\tif (type == FILTER_REFS_BRANCHES)\n> +\t\tret = for_each_reftype_fullpath(ref_filter_handler, \"refs/heads/\", broken, &ref_cbdata);\n> +\telse if (type == FILTER_REFS_REMOTES)\n> +\t\tret = for_each_reftype_fullpath(ref_filter_handler, \"refs/remotes/\", broken, &ref_cbdata);\n> +\telse if (type == FILTER_REFS_TAGS)\n> +\t\tret = for_each_reftype_fullpath(ref_filter_handler, \"refs/tags/\", broken, &ref_cbdata);\n> +\telse if (type & FILTER_REFS_ALL) {\n> +\t\tret = for_each_reftype_fullpath(ref_filter_handler, \"\", broken, &ref_cbdata);\n> +\t\tif (type & FILTER_REFS_DETACHED_HEAD)\n> +\t\t\thead_ref(ref_filter_handler, &ref_cbdata);\n> +\t} else\n> \t\tdie(\"filter_refs: invalid type\");\n> \n> \t/* Filters that need revision walking */\n> diff --git a/ref-filter.h b/ref-filter.h\n> index 45026d0..99f081b 100644\n> --- a/ref-filter.h\n> +++ b/ref-filter.h\n> @@ -13,8 +13,14 @@\n> #define QUOTE_PYTHON 4\n> #define QUOTE_TCL 8\n> \n> -#define FILTER_REFS_INCLUDE_BROKEN 0x1\n> -#define FILTER_REFS_ALL 0x2\n> +#define FILTER_REFS_INCLUDE_BROKEN 0x0001\n> +#define FILTER_REFS_TAGS 0x0002\n> +#define FILTER_REFS_BRANCHES 0x0004\n> +#define FILTER_REFS_REMOTES 0x0008\n> +#define FILTER_REFS_OTHERS 0x0010\n> +#define FILTER_REFS_ALL (FILTER_REFS_TAGS | FILTER_REFS_BRANCHES | \\\n> +\t\t\t\t FILTER_REFS_REMOTES | FILTER_REFS_OTHERS)\n> +#define FILTER_REFS_DETACHED_HEAD 0x0020\n> \n> struct atom_value;\n> \n> @@ -27,6 +33,7 @@ struct ref_sorting {\n> struct ref_array_item {\n> \tunsigned char objectname[20];\n> \tint flag;\n> +\tunsigned int kind;\n> \tconst char *symref;\n> \tstruct commit *commit;\n> \tstruct atom_value *value;\n> @@ -51,6 +58,7 @@ struct ref_filter {\n> \tstruct commit *merge_commit;\n> \n> \tunsigned int with_commit_tag_algo : 1;\n> +\tunsigned int kind;\n> };\n> \n> struct ref_filter_cbdata {\n> diff --git a/refs.c b/refs.c\n> index 4e15f60..3266617 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2150,6 +2150,15 @@ int for_each_replace_ref(each_ref_fn fn, void *cb_data)\n> \t\t\t strlen(git_replace_ref_base), 0, cb_data);\n> }\n> \n> +int for_each_reftype_fullpath(each_ref_fn fn, char *type, unsigned int broken, void *cb_data)\n> +{\n> +\tunsigned int flag = 0;\n> +\n> +\tif (broken)\n> +\t\tflag = DO_FOR_EACH_INCLUDE_BROKEN;\n> +\treturn do_for_each_ref(&ref_cache, type, fn, 0, flag, cb_data);\n> +}\n> +\n> int head_ref_namespaced(each_ref_fn fn, void *cb_data)\n> {\n> \tstruct strbuf buf = STRBUF_INIT;\n> diff --git a/refs.h b/refs.h\n> index e9a5f32..6e913ee 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -179,6 +179,7 @@ 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> extern int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data);\n> extern int for_each_glob_ref_in(each_ref_fn fn, const char *pattern, const char *prefix, void *cb_data);\n> +extern int for_each_reftype_fullpath(each_ref_fn fn, char *type, unsigned int broken, void *cb_data);\n> \n> extern int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);\n> extern int for_each_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);\n"},{"id":"268614","messageId":"xmqq7fok2u57.fsf@gitster.dls.corp.google.com","threadId":"40155","inReplyTo":"vpqzj1hkc5q.fsf@anie.imag.fr","subject":"Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-24T22:34:12Z","receivedAt":"2015-08-24T22:34:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> Karthik Nayak writes:\n>\n>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\n>> index 1997657..06d468e 100644\n>> --- a/Documentation/git-for-each-ref.txt\n>> +++ b/Documentation/git-for-each-ref.txt\n>> @@ -133,7 +133,8 @@ align::\n>> \t`` is either left, right or middle and `` is\n>> \tthe total length of the content with alignment. If the\n>> \tcontents length is more than the width then no alignment is\n>> -\tperformed.\n>> +\tperformed. If used with '--quote' everything in between %(align:..)\n>> +\tand %(end) is quoted.\n>\n> There's no --quote, there are --shell, --python, ... (well, actually, I\n> would have prefered to have a single --quote=language option, but that's\n> not how it is now).\n>\n> I had already commented on a preliminary version of this series\n> off-list. I think all my previous comments have been taken into account.\n\nThanks, both. I think this is pretty close to being satisfactory\n;-) There may probably be a handful of minor nits like the above\nthat need to be addressed, but I do not think I saw anything\nglaringly wrong that makes the series unsalvageable. It was a very\npleasant read.\n\nIt's almost there, and I am very happy to see how this and other\nseries evolved so far ;-)\n"},{"id":"268615","messageId":"xmqq37z82u2a.fsf@gitster.dls.corp.google.com","threadId":"40155","inReplyTo":"xmqq8u904ix0.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-24T22:35:57Z","receivedAt":"2015-08-24T22:35:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Karthik Nayak writes:\n>\n>> On Mon, Aug 24, 2015 at 10:57 PM, Junio C Hamano wrote:\n>>> Karthik Nayak writes:\n>>> ...\n>>>> + performed. If used with '--quote' everything in between %(align:..)\n>>>> + and %(end) is quoted.\n>> ...\n>> I might have misunderstood you, But based on the discussion held here\n>> (thread.gmane.org/gmane.comp.version-control.git/276140)\n>> I thought we wanted everything inside the %(align) .... %(end) atoms\n>> to be quoted.\n>\n> Perhaps I misunderstood your intention in the doc.\n> \t\n> I took \"everything in between %(align:...) and %(end) is quoted\" to\n> mean that\n>\n> \t%(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end)\n>\n> can never satisfy %(if:empty), because %(align)%(end) would expand\n> to a string that has two single-quotes, that is not an empty string.\n>\n> If that is not what would happen in the \"branch --list\" enhancement,\n> then the proposed behaviour is good, but the above documentation would\n> need to be updated when it happens, I think. It at least is misleading.\n\nOK, now I checked the code, and I _think_ the recursive logic is\ndoing the right thing (modulo minor nits on comment-vs-code\ndiscrepancy and code structure I sent separately).\n"},{"id":"268616","messageId":"xmqqy4h01egr.fsf@gitster.dls.corp.google.com","threadId":"40155","inReplyTo":"xmqq7fok2u57.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-24T22:58:12Z","receivedAt":"2015-08-24T22:58:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Matthieu Moy writes:\n>\n>> Karthik Nayak writes:\n>>\n>>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\n>>> index 1997657..06d468e 100644\n>>> --- a/Documentation/git-for-each-ref.txt\n>>> +++ b/Documentation/git-for-each-ref.txt\n>>> @@ -133,7 +133,8 @@ align::\n>>> \t`` is either left, right or middle and `` is\n>>> \tthe total length of the content with alignment. If the\n>>> \tcontents length is more than the width then no alignment is\n>>> -\tperformed.\n>>> +\tperformed. If used with '--quote' everything in between %(align:..)\n>>> +\tand %(end) is quoted.\n>>\n>> There's no --quote, there are --shell, --python, ... (well, actually, I\n>> would have prefered to have a single --quote=language option, but that's\n>> not how it is now).\n>>\n>> I had already commented on a preliminary version of this series\n>> off-list. I think all my previous comments have been taken into account.\n>\n> Thanks, both. I think this is pretty close to being satisfactory\n> ;-) There may probably be a handful of minor nits like the above\n> that need to be addressed, but I do not think I saw anything\n> glaringly wrong that makes the series unsalvageable. It was a very\n> pleasant read.\n>\n> It's almost there, and I am very happy to see how this and other\n> series evolved so far ;-)\n\nHaving said all that, it seems that there is some trivial typo or\nthinko in the formatting code to break t7004.\n\nHere is what I see...\n\nok 98 - verifying rfc1991 signature\n\nexpecting success:\n echo \"rfc1991\" >gpghome/gpg.conf &&\n echo \"rfc1991-signed-tag RFC1991 signed tag\" >expect &&\n git tag -l -n1 rfc1991-signed-tag >actual &&\n test_cmp expect actual &&\n git tag -l -n2 rfc1991-signed-tag >actual &&\n test_cmp expect actual &&\n git tag -l -n999 rfc1991-signed-tag >actual &&\n test_cmp expect actual\n\n--- expect 2015-08-24 22:54:44.607272653 +0000\n+++ actual 2015-08-24 22:54:44.611272643 +0000\n@@ -1 +1 @@\n-rfc1991-signed-tag RFC1991 signed tag\n+rfc1991-signed-tagRFC1991 signed tag\nnot ok 99 - list tag with rfc1991 signature\n"},{"id":"268627","messageId":"vpqk2sj7tlb.fsf@anie.imag.fr","threadId":"40155","inReplyTo":"xmqqmvxg2v3u.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v13 04/12] ref-filter: implement an `align` atom","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-08-25T06:47:12Z","receivedAt":"2015-08-25T06:47:12Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> You can see that I expected that \"if !state.stack->prev\" check to be\n> inside append_atom(), and I would imagine future readers would have\n> the same expectation when reading this code. I.e.\n>\n> \tappend_atom(struct atom_value *v, struct ref_f_s *state)\n> {\n> \t\tif (state->stack->prev)\n> \t\t\tstrbuf_addstr(&state->stack->output, v->s);\n> \t\telse\n> \tquote_format(&state->stack->output, v->s, state->quote_style);\n> \t}\n>\n> The end result may be the same,\n\nThere's another call to append_atom() when inserting the \"reset color at\nend of line if needed\", so moving this if inside append_atom means we\nwould do the check also for the reset color. It would not change the\nbehavior (by construction, we insert it only when the stack has only the\ninitial element), so it's OK.\n\nI agree that this is a good thing to do.\n\n> Moreover, notice that the function signature of append_atom() is\n> exactly the same as atomv->handler's. I wonder if it would be\n> easier to understand if you made append_atom() the handler for a\n> non-magic atoms, which would let you do the above without any if/else\n> and just a single unconditional\n\nI can't decide between \"ah, very elegant\" and \"no, too much magic\" ;-).\nI lean towards the former.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"268634","messageId":"CAOLa=ZRvzM4xyZi09aDrepAZZ9ovKxz08mA9aHjUNpqddfW-8w@mail.gmail.com","threadId":"40155","inReplyTo":"xmqqr3ms2vvh.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v13 02/12] ref-filter: introduce ref_formatting_state and ref_formatting_stack","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-25T10:26:56Z","receivedAt":"2015-08-25T10:26:56Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Aug 25, 2015 at 3:26 AM, Junio C Hamano wrote:\n> Karthik Nayak writes:\n>\n>> +static void push_new_stack_element(struct ref_formatting_stack **stack)\n>> +{\n>\n> Micronit. Perhaps s/_new//;, as you do not call the other function\n> pop_old_stack_element().\n>\n> The remainder of this step looks pretty straight-forward and was a\n> pleasant read.\n>\n> Thanks.\n\nWill do, thanks for reviewing :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268641","messageId":"CAOLa=ZTU0tBiCy10wrb6u5Prc4iEO9_iRkAk0nuw5jj4kB0UcA@mail.gmail.com","threadId":"40155","inReplyTo":"xmqqbndw2ul1.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v13 05/12] ref-filter: add option to filter out tags, branches and remotes","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-25T13:07:40Z","receivedAt":"2015-08-25T13:07:40Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Aug 25, 2015 at 3:54 AM, Junio C Hamano wrote:\n> Karthik Nayak writes:\n>\n>> From: Karthik Nayak \n>>\n>> Add a function called 'for_each_reftype_fullpath()' to refs.{c,h}\n>> which iterates through each ref for the given path without trimming\n>> the path and also accounting for broken refs, if mentioned.\n>\n> For this part, I think you would want to borrow an extra pair of\n> eyes from Michael.\n>\n>>\n>> Add 'filter_ref_kind()' in ref-filter.c to check the kind of ref being\n>> handled and return the kind to 'ref_filter_handler()', where we\n>> discard refs which we do not need and assign the kind to needed refs.\n>>\n>> Mentored-by: Christian Couder \n>> Mentored-by: Matthieu Moy \n>> Signed-off-by: Karthik Nayak \n>> ---\n>> ref-filter.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----\n>> ref-filter.h | 12 ++++++++++--\n>> refs.c | 9 +++++++++\n>> refs.h | 1 +\n>> 4 files changed, 74 insertions(+), 7 deletions(-)\n>>\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> index ffec10a..d5fae1a 100644\n>> --- a/ref-filter.c\n>> +++ b/ref-filter.c\n>> @@ -1123,6 +1123,36 @@ static struct ref_array_item *new_ref_array_item(const char *refname,\n>> return ref;\n>> }\n>>\n>> +static int filter_ref_kind(struct ref_filter *filter, const char *refname)\n>> +{\n>> + unsigned int i;\n>> +\n>> + static struct {\n>> + const char *prefix;\n>> + unsigned int kind;\n>> + } ref_kind[] = {\n>> + { \"refs/heads/\" , FILTER_REFS_BRANCHES },\n>> + { \"refs/remotes/\" , FILTER_REFS_REMOTES },\n>> + { \"refs/tags/\", FILTER_REFS_TAGS}\n>> + };\n>> +\n>> + if (filter->kind == FILTER_REFS_BRANCHES)\n>> + return FILTER_REFS_BRANCHES;\n>> + else if (filter->kind == FILTER_REFS_REMOTES)\n>> + return FILTER_REFS_REMOTES;\n>> + else if (filter->kind == FILTER_REFS_TAGS)\n>> + return FILTER_REFS_TAGS;\n>> + else if (!strcmp(refname, \"HEAD\"))\n>> + return FILTER_REFS_DETACHED_HEAD;\n>> +\n>> + for (i = 0; i < ARRAY_SIZE(ref_kind); i++) {\n>> + if (starts_with(refname, ref_kind[i].prefix))\n>> + return ref_kind[i].kind;\n>> + }\n>> +\n>> + return FILTER_REFS_OTHERS;\n>> +}\n>> +\n>> /*\n>> * A call-back given to for_each_ref(). Filter refs and keep them for\n>> * later object processing.\n>> @@ -1133,6 +1163,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n>> struct ref_filter *filter = ref_cbdata->filter;\n>> struct ref_array_item *ref;\n>> struct commit *commit = NULL;\n>> + unsigned int kind;\n>>\n>> if (flag & REF_BAD_NAME) {\n>> warning(\"ignoring ref with broken name %s\", refname);\n>> @@ -1144,6 +1175,10 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n>> return 0;\n>> }\n>>\n>> + kind = filter_ref_kind(filter, refname);\n>> + if (!(kind & filter->kind))\n>> + return 0;\n>> +\n>\n> When filter->kind is set to some single-bit thing (e.g.\n> FILTER_REFS_BRANCHES) by the caller of ref_filter_handler(), then\n> this call of filter_ref_kind() will just parrot that without even\n> looking at refname. And then the if statement says \"yes, they have\n> common bit(s)\". Even when refname is \"refs/tags/v1.0\" or \"HEAD\".\n>\n\nThis happens for branches remotes and tags, and this is intentional so as to\nskip the uncessary checking to be done, if you see filter_refs() if it's called\nwith a single bit (branches, tags or remotes) then it calls\nfor_each_reftype_fullpath()\nwith the corresponding path, which would only give us the refs which we needed\nhence checking them is not really needed. But the call to\nfilter_ref_kind() would just\ngive us the kind of the ref.\n\n> And if this code knows that \"refs/tags/v1.0\" or \"HEAD\" will never\n> come when filter->kind is exactly FILTER_REFS_BRANCHES, then I do\n> not see the point of calling filter_ref_kind().\n>\n\nThe point is to have a common function which returns the current ref kind,\nEven if we know that the current ref kind is the same as the filter->kind.\n\n> I am not sure what this is trying to do. Can you clarify the\n> thinking behind this as a comment to filter_ref_kind()?\n\nI should added a comment here, I will do so now.\n\n>\n>> @@ -1175,6 +1210,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n>>\n>> REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);\n>> ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;\n>> + ref->kind = kind;\n>> return 0;\n>> }\n>>\n>> @@ -1251,16 +1287,29 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int\n>> {\n>> struct ref_filter_cbdata ref_cbdata;\n>> int ret = 0;\n>> + unsigned int broken = 0;\n>>\n>> ref_cbdata.array = array;\n>> ref_cbdata.filter = filter;\n>>\n>> /* Simple per-ref filtering */\n>> - if (type & (FILTER_REFS_ALL | FILTER_REFS_INCLUDE_BROKEN))\n>> - ret = for_each_rawref(ref_filter_handler, &ref_cbdata);\n>> - else if (type & FILTER_REFS_ALL)\n>> - ret = for_each_ref(ref_filter_handler, &ref_cbdata);\n>> - else if (type)\n>> + if (type & FILTER_REFS_INCLUDE_BROKEN) {\n>> + type &= ~FILTER_REFS_INCLUDE_BROKEN;\n>> + broken = 1;\n>> + }\n>> +\n>> + filter->kind = type;\n>> + if (type == FILTER_REFS_BRANCHES)\n>> + ret = for_each_reftype_fullpath(ref_filter_handler, \"refs/heads/\", broken, &ref_cbdata);\n>> + else if (type == FILTER_REFS_REMOTES)\n>> + ret = for_each_reftype_fullpath(ref_filter_handler, \"refs/remotes/\", broken, &ref_cbdata);\n>> + else if (type == FILTER_REFS_TAGS)\n>> + ret = for_each_reftype_fullpath(ref_filter_handler, \"refs/tags/\", broken, &ref_cbdata);\n>> + else if (type & FILTER_REFS_ALL) {\n>> + ret = for_each_reftype_fullpath(ref_filter_handler, \"\", broken, &ref_cbdata);\n>> + if (type & FILTER_REFS_DETACHED_HEAD)\n>> + head_ref(ref_filter_handler, &ref_cbdata);\n>> + } else\n>> die(\"filter_refs: invalid type\");\n>>\n>> /* Filters that need revision walking */\n>> diff --git a/ref-filter.h b/ref-filter.h\n>> index 45026d0..99f081b 100644\n>> --- a/ref-filter.h\n>> +++ b/ref-filter.h\n>> @@ -13,8 +13,14 @@\n>> #define QUOTE_PYTHON 4\n>> #define QUOTE_TCL 8\n>>\n>> -#define FILTER_REFS_INCLUDE_BROKEN 0x1\n>> -#define FILTER_REFS_ALL 0x2\n>> +#define FILTER_REFS_INCLUDE_BROKEN 0x0001\n>> +#define FILTER_REFS_TAGS 0x0002\n>> +#define FILTER_REFS_BRANCHES 0x0004\n>> +#define FILTER_REFS_REMOTES 0x0008\n>> +#define FILTER_REFS_OTHERS 0x0010\n>> +#define FILTER_REFS_ALL (FILTER_REFS_TAGS | FILTER_REFS_BRANCHES | \\\n>> + FILTER_REFS_REMOTES | FILTER_REFS_OTHERS)\n>> +#define FILTER_REFS_DETACHED_HEAD 0x0020\n>>\n>> struct atom_value;\n>>\n>> @@ -27,6 +33,7 @@ struct ref_sorting {\n>> struct ref_array_item {\n>> unsigned char objectname[20];\n>> int flag;\n>> + unsigned int kind;\n>> const char *symref;\n>> struct commit *commit;\n>> struct atom_value *value;\n>> @@ -51,6 +58,7 @@ struct ref_filter {\n>> struct commit *merge_commit;\n>>\n>> unsigned int with_commit_tag_algo : 1;\n>> + unsigned int kind;\n>> };\n>>\n>> struct ref_filter_cbdata {\n>> diff --git a/refs.c b/refs.c\n>> index 4e15f60..3266617 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -2150,6 +2150,15 @@ int for_each_replace_ref(each_ref_fn fn, void *cb_data)\n>> strlen(git_replace_ref_base), 0, cb_data);\n>> }\n>>\n>> +int for_each_reftype_fullpath(each_ref_fn fn, char *type, unsigned int broken, void *cb_data)\n>> +{\n>> + unsigned int flag = 0;\n>> +\n>> + if (broken)\n>> + flag = DO_FOR_EACH_INCLUDE_BROKEN;\n>> + return do_for_each_ref(&ref_cache, type, fn, 0, flag, cb_data);\n>> +}\n>> +\n>> int head_ref_namespaced(each_ref_fn fn, void *cb_data)\n>> {\n>> struct strbuf buf = STRBUF_INIT;\n>> diff --git a/refs.h b/refs.h\n>> index e9a5f32..6e913ee 100644\n>> --- a/refs.h\n>> +++ b/refs.h\n>> @@ -179,6 +179,7 @@ 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>> extern int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data);\n>> extern int for_each_glob_ref_in(each_ref_fn fn, const char *pattern, const char *prefix, void *cb_data);\n>> +extern int for_each_reftype_fullpath(each_ref_fn fn, char *type, unsigned int broken, void *cb_data);\n>>\n>> extern int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);\n>> extern int for_each_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);\n\nThanks for the review.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268643","messageId":"CAOLa=ZTxNADuiHizF=HRK-KP2MuFJE=XS1BqRrmVQkyNVxbEgQ@mail.gmail.com","threadId":"40155","inReplyTo":"xmqq7fok2u57.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-25T13:23:34Z","receivedAt":"2015-08-25T13:23:34Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Aug 25, 2015 at 4:04 AM, Junio C Hamano wrote:\n> Matthieu Moy writes:\n>\n>> Karthik Nayak writes:\n>>\n>>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\n>>> index 1997657..06d468e 100644\n>>> --- a/Documentation/git-for-each-ref.txt\n>>> +++ b/Documentation/git-for-each-ref.txt\n>>> @@ -133,7 +133,8 @@ align::\n>>> `` is either left, right or middle and `` is\n>>> the total length of the content with alignment. If the\n>>> contents length is more than the width then no alignment is\n>>> - performed.\n>>> + performed. If used with '--quote' everything in between %(align:..)\n>>> + and %(end) is quoted.\n>>\n>> There's no --quote, there are --shell, --python, ... (well, actually, I\n>> would have prefered to have a single --quote=language option, but that's\n>> not how it is now).\n>>\n>> I had already commented on a preliminary version of this series\n>> off-list. I think all my previous comments have been taken into account.\n>\n> Thanks, both. I think this is pretty close to being satisfactory\n> ;-) There may probably be a handful of minor nits like the above\n> that need to be addressed, but I do not think I saw anything\n> glaringly wrong that makes the series unsalvageable. It was a very\n> pleasant read.\n>\n> It's almost there, and I am very happy to see how this and other\n> series evolved so far ;-)\n\nI like how it's totally different from what I started off with, it's\nbeen a great learning curve\nfor me. Thanks for reviews.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268644","messageId":"CAOLa=ZQUANdHoKU5=MP8sGhh1ZSH_93zFWY2=Fpf--ru3Q03NA@mail.gmail.com","threadId":"40155","inReplyTo":"xmqq8u904ix0.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-25T13:25:31Z","receivedAt":"2015-08-25T13:25:31Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Aug 25, 2015 at 12:23 AM, Junio C Hamano wrote:\n> Karthik Nayak writes:\n>\n>> On Mon, Aug 24, 2015 at 10:57 PM, Junio C Hamano wrote:\n>>> Karthik Nayak writes:\n>>> ...\n>>>> + performed. If used with '--quote' everything in between %(align:..)\n>>>> + and %(end) is quoted.\n>> ...\n>> I might have misunderstood you, But based on the discussion held here\n>> (thread.gmane.org/gmane.comp.version-control.git/276140)\n>> I thought we wanted everything inside the %(align) .... %(end) atoms\n>> to be quoted.\n>\n> Perhaps I misunderstood your intention in the doc.\n>\n> I took \"everything in between %(align:...) and %(end) is quoted\" to\n> mean that\n>\n> %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end)\n>\n> can never satisfy %(if:empty), because %(align)%(end) would expand\n> to a string that has two single-quotes, that is not an empty string.\n>\n> If that is not what would happen in the \"branch --list\" enhancement,\n> then the proposed behaviour is good, but the above documentation would\n> need to be updated when it happens, I think. It at least is misleading.\n>\n> Thanks.\n\nI'm not sure I checked this condition, will have a look at this, thanks for\npointing it out.\n\n> OK, now I checked the code, and I _think_ the recursive logic is\n> doing the right thing (modulo minor nits on comment-vs-code\n> discrepancy and code structure I sent separately).\n\nI'm not so sure about that, I'll get back on this. (I didn't think about empty\nstring alignment and its effect on %(if:empty) and so on).\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268645","messageId":"CAOLa=ZQwW9hpg4p8+DE2oZA28Av7mLrqAhEdcro=esuqHe35Xg@mail.gmail.com","threadId":"40155","inReplyTo":"xmqqy4h01egr.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-25T13:26:55Z","receivedAt":"2015-08-25T13:26:55Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Aug 25, 2015 at 4:28 AM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Matthieu Moy writes:\n>>\n>>> Karthik Nayak writes:\n>>>\n>>>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\n>>>> index 1997657..06d468e 100644\n>>>> --- a/Documentation/git-for-each-ref.txt\n>>>> +++ b/Documentation/git-for-each-ref.txt\n>>>> @@ -133,7 +133,8 @@ align::\n>>>> `` is either left, right or middle and `` is\n>>>> the total length of the content with alignment. If the\n>>>> contents length is more than the width then no alignment is\n>>>> - performed.\n>>>> + performed. If used with '--quote' everything in between %(align:..)\n>>>> + and %(end) is quoted.\n>>>\n>>> There's no --quote, there are --shell, --python, ... (well, actually, I\n>>> would have prefered to have a single --quote=language option, but that's\n>>> not how it is now).\n>>>\n>>> I had already commented on a preliminary version of this series\n>>> off-list. I think all my previous comments have been taken into account.\n>>\n>> Thanks, both. I think this is pretty close to being satisfactory\n>> ;-) There may probably be a handful of minor nits like the above\n>> that need to be addressed, but I do not think I saw anything\n>> glaringly wrong that makes the series unsalvageable. It was a very\n>> pleasant read.\n>>\n>> It's almost there, and I am very happy to see how this and other\n>> series evolved so far ;-)\n>\n> Having said all that, it seems that there is some trivial typo or\n> thinko in the formatting code to break t7004.\n>\n> Here is what I see...\n>\n> ok 98 - verifying rfc1991 signature\n>\n> expecting success:\n> echo \"rfc1991\" >gpghome/gpg.conf &&\n> echo \"rfc1991-signed-tag RFC1991 signed tag\" >expect &&\n> git tag -l -n1 rfc1991-signed-tag >actual &&\n> test_cmp expect actual &&\n> git tag -l -n2 rfc1991-signed-tag >actual &&\n> test_cmp expect actual &&\n> git tag -l -n999 rfc1991-signed-tag >actual &&\n> test_cmp expect actual\n>\n> --- expect 2015-08-24 22:54:44.607272653 +0000\n> +++ actual 2015-08-24 22:54:44.611272643 +0000\n> @@ -1 +1 @@\n> -rfc1991-signed-tag RFC1991 signed tag\n> +rfc1991-signed-tagRFC1991 signed tag\n> not ok 99 - list tag with rfc1991 signature\n>\n\nThats weird, I just ran all tests, and nothing failed.\nIs this mid-way of the patch series? I tried it on the entire\nseries and seems fine to me\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268646","messageId":"CAOLa=ZReeQQ6sV3WvmjcYqQTg+s2xm3cepVe25gEH4jcAx3ocg@mail.gmail.com","threadId":"40155","inReplyTo":"xmqqmvxg2v3u.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v13 04/12] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-25T13:28:28Z","receivedAt":"2015-08-25T13:28:28Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Aug 25, 2015 at 3:43 AM, Junio C Hamano wrote:\n> Karthik Nayak writes:\n>\n>> +static void perform_quote_formatting(struct strbuf *s, const char *str, int quote_style);\n>> +\n>> +static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n>> +{\n>> + struct ref_formatting_stack *current = state->stack;\n>> + struct strbuf s = STRBUF_INIT;\n>> +\n>> + if (!current->at_end)\n>> + die(_(\"format: `end` atom used without a supporting atom\"));\n>> + current->at_end(current);\n>> + /*\n>> + * Whenever we have more than one stack element that means we\n>> + * are using a certain modifier atom. In that case we need to\n>> + * perform quote formatting.\n>> + */\n>> + if (!state->stack->prev->prev) {\n>\n> The comment and the condition seem to be saying opposite things.\n> The code says \"If the stack only has one prev that is the very\n> initial one, then we do the quoting, i.e. the result of expanding\n> the enclosed string in %(start-something)...%(end) is quoted only\n> when that appears at the top level\", which feels more correct than\n> the comment that says \"if we are about to pop after seeing the\n> first %(end) in %(start)...%(another)...%(end)...%(end) sequence,\n> we quote what is between %(another)...%(end)\".\n>\n\nThat sounds misleading indeed will need to change that.\n\n>> + perform_quote_formatting(&s, current->output.buf, state->quote_style);\n>> + strbuf_reset(¤t->output);\n>> + strbuf_addbuf(¤t->output, &s);\n>> + }\n>> + strbuf_release(&s);\n>> + pop_stack_element(&state->stack);\n>> +}\n>> +\n>\n>> @@ -1228,29 +1315,33 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)\n>> qsort(array->items, array->nr, sizeof(struct ref_array_item *), compare_refs);\n>> }\n>>\n>> -static void append_atom(struct atom_value *v, struct ref_formatting_state *state)\n>> +static void perform_quote_formatting(struct strbuf *s, const char *str, int quote_style)\n>> {\n>> - struct strbuf *s = &state->stack->output;\n>> -\n>> - switch (state->quote_style) {\n>> + switch (quote_style) {\n>> case QUOTE_NONE:\n>> - strbuf_addstr(s, v->s);\n>> + strbuf_addstr(s, str);\n>> break;\n>> case QUOTE_SHELL:\n>> - sq_quote_buf(s, v->s);\n>> + sq_quote_buf(s, str);\n>> break;\n>> case QUOTE_PERL:\n>> - perl_quote_buf(s, v->s);\n>> + perl_quote_buf(s, str);\n>> break;\n>> case QUOTE_PYTHON:\n>> - python_quote_buf(s, v->s);\n>> + python_quote_buf(s, str);\n>> break;\n>> case QUOTE_TCL:\n>> - tcl_quote_buf(s, v->s);\n>> + tcl_quote_buf(s, str);\n>> break;\n>> }\n>> }\n>>\n>> +static void append_atom(struct atom_value *v, struct ref_formatting_state *state)\n>> +{\n>> + struct strbuf *s = &state->stack->output;\n>> + perform_quote_formatting(s, v->s, state->quote_style);\n>\n> Hmmm, do we want to unconditionally do the quote here, or only when\n> we are not being captured by upcoming %(end) to be consistent with\n> the behaviour of end_atom_handler() above?\n>\n>> @@ -1307,7 +1398,18 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n>> if (cp < sp)\n>> append_literal(cp, sp, &state);\n>> get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);\n>> - append_atom(atomv, &state);\n>> + /*\n>> + * If the atom is a modifier atom, then call the handler function.\n>> + * Else, if this is the first element on the stack, then we need to\n>> + * format the atom as per the given quote. Else we just add the atom value\n>> + * to the current stack element and handle quote formatting at the end.\n>> + */\n>> + if (atomv->handler)\n>> + atomv->handler(atomv, &state);\n>> + else if (!state.stack->prev)\n>> + append_atom(atomv, &state);\n>> + else\n>> + strbuf_addstr(&state.stack->output, atomv->s);\n>\n> Ahh, this explains why you are not doing it above, but I do not\n> think if this is a good division of labor.\n>\n> You can see that I expected that \"if !state.stack->prev\" check to be\n> inside append_atom(), and I would imagine future readers would have\n> the same expectation when reading this code. I.e.\n>\n> append_atom(struct atom_value *v, struct ref_f_s *state)\n> {\n> if (state->stack->prev)\n> strbuf_addstr(&state->stack->output, v->s);\n> else\n> quote_format(&state->stack->output, v->s, state->quote_style);\n> }\n>\n> The end result may be the same, but I do think \"append_atom is to\n> always quote, so we do an unquoted appending by hand\" is a bad way\n> to do this.\n>\n> Moreover, notice that the function signature of append_atom() is\n> exactly the same as atomv->handler's. I wonder if it would be\n> easier to understand if you made append_atom() the handler for a\n> non-magic atoms, which would let you do the above without any if/else\n> and just a single unconditional\n>\n> atomv->handler(atomv, &state);\n\nI like the atomv->handler() idea I think I'll work on that now.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268647","messageId":"CAOLa=ZQfVbyX=7LhTbsFRyWY180zyG7=aeYz6zOjnwd=yUUivw@mail.gmail.com","threadId":"40155","inReplyTo":"xmqqfv382uzo.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v13 04/12] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-25T13:28:56Z","receivedAt":"2015-08-25T13:28:56Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Aug 25, 2015 at 3:45 AM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Karthik Nayak writes:\n>>\n>>> +static void end_atom_handler(struct atom_value *atomv, struct\n>>> ref_formatting_state *state)\n>>> +{\n>>> + struct ref_formatting_stack *current = state->stack;\n>>> + struct strbuf s = STRBUF_INIT;\n>>> +\n>>> + if (!current->at_end)\n>>> + die(_(\"format: `end` atom used without a supporting atom\"));\n>>> + current->at_end(current);\n>>> + /*\n>>> + * Whenever we have more than one stack element that means we\n>>> + * are using a certain modifier atom. In that case we need to\n>>> + * perform quote formatting.\n>>> + */\n>>> + if (!state->stack->prev->prev) {\n>>\n>> The comment and the condition seem to be saying opposite things.\n>> The code says \"If the stack only has one prev that is the very\n>> initial one, then we do the quoting, i.e. the result of expanding\n>> the enclosed string in %(start-something)...%(end) is quoted only\n>> when that appears at the top level\", which feels more correct...\n>\n> As this already allows us to use nested construct, I think we would\n> want to have test that uses\n>\n> %(align:30,left)%(align:20,right)%(refname:short)%(end)%(end)\n>\n> or something like that ;-)\n>\n> Very nice.\n\nOk, will do that :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268648","messageId":"CAOLa=ZS1EiU3FFLwR3QUUebCtVkrwUS=qP9xvc93LHanA5eh=A@mail.gmail.com","threadId":"40155","inReplyTo":"vpqk2sj7tlb.fsf@anie.imag.fr","subject":"Re: [PATCH v13 04/12] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-25T13:30:13Z","receivedAt":"2015-08-25T13:30:13Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Aug 25, 2015 at 12:17 PM, Matthieu Moy\n wrote:\n> Junio C Hamano writes:\n>\n>> You can see that I expected that \"if !state.stack->prev\" check to be\n>> inside append_atom(), and I would imagine future readers would have\n>> the same expectation when reading this code. I.e.\n>>\n>> append_atom(struct atom_value *v, struct ref_f_s *state)\n>> {\n>> if (state->stack->prev)\n>> strbuf_addstr(&state->stack->output, v->s);\n>> else\n>> quote_format(&state->stack->output, v->s, state->quote_style);\n>> }\n>>\n>> The end result may be the same,\n>\n> There's another call to append_atom() when inserting the \"reset color at\n> end of line if needed\", so moving this if inside append_atom means we\n> would do the check also for the reset color. It would not change the\n> behavior (by construction, we insert it only when the stack has only the\n> initial element), so it's OK.\n>\n> I agree that this is a good thing to do.\n>\n>> Moreover, notice that the function signature of append_atom() is\n>> exactly the same as atomv->handler's. I wonder if it would be\n>> easier to understand if you made append_atom() the handler for a\n>> non-magic atoms, which would let you do the above without any if/else\n>> and just a single unconditional\n>\n> I can't decide between \"ah, very elegant\" and \"no, too much magic\" ;-).\n> I lean towards the former.\n>\n\nI like the idea of using atomv->handler() a lot, mostly cause this\nwould eventually\nhelp us clean up populate_atom() which currently seems like a huge dump of code.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268672","messageId":"xmqqpp2bz266.fsf@gitster.dls.corp.google.com","threadId":"40155","inReplyTo":"vpqk2sj7tlb.fsf@anie.imag.fr","subject":"Re: [PATCH v13 04/12] ref-filter: implement an `align` atom","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-25T17:52:01Z","receivedAt":"2015-08-25T17:52:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> Junio C Hamano writes:\n>\n>> You can see that I expected that \"if !state.stack->prev\" check to be\n>> inside append_atom(), and I would imagine future readers would have\n>> the same expectation when reading this code. I.e.\n>>\n>> \tappend_atom(struct atom_value *v, struct ref_f_s *state)\n>> {\n>> \t\tif (state->stack->prev)\n>> \t\t\tstrbuf_addstr(&state->stack->output, v->s);\n>> \t\telse\n>> \tquote_format(&state->stack->output, v->s, state->quote_style);\n>> \t}\n>>\n>> The end result may be the same,\n>\n> There's another call to append_atom() when inserting the \"reset color at\n> end of line if needed\", so moving this if inside append_atom means we\n> would do the check also for the reset color. It would not change the\n> behavior (by construction, we insert it only when the stack has only the\n> initial element), so it's OK.\n\nThanks for checking---I did overlook that other callsite and did not\ncheck if the suggested change was sensible there.\n"},{"id":"268673","messageId":"xmqqlhczz1yz.fsf@gitster.dls.corp.google.com","threadId":"40155","inReplyTo":"CAOLa=ZS1EiU3FFLwR3QUUebCtVkrwUS=qP9xvc93LHanA5eh=A@mail.gmail.com","subject":"Re: [PATCH v13 04/12] ref-filter: implement an `align` atom","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-25T17:56:20Z","receivedAt":"2015-08-25T17:56:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> I like the idea of using atomv->handler() a lot, mostly cause this\n> would eventually\n> help us clean up populate_atom() which currently seems like a huge dump of code.\n\nI think you already said that last time we had this discussion ;-)\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/275537/focus=275778\n"},{"id":"268681","messageId":"xmqqzj1fxjpj.fsf@gitster.dls.corp.google.com","threadId":"40155","inReplyTo":"CAOLa=ZQwW9hpg4p8+DE2oZA28Av7mLrqAhEdcro=esuqHe35Xg@mail.gmail.com","subject":"Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-25T19:16:08Z","receivedAt":"2015-08-25T19:16:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n>> Here is what I see...\n>>\n>> ok 98 - verifying rfc1991 signature\n>>\n>> expecting success:\n>> echo \"rfc1991\" >gpghome/gpg.conf &&\n>> echo \"rfc1991-signed-tag RFC1991 signed tag\" >expect &&\n>> git tag -l -n1 rfc1991-signed-tag >actual &&\n>> test_cmp expect actual &&\n>> git tag -l -n2 rfc1991-signed-tag >actual &&\n>> test_cmp expect actual &&\n>> git tag -l -n999 rfc1991-signed-tag >actual &&\n>> test_cmp expect actual\n>>\n>> --- expect 2015-08-24 22:54:44.607272653 +0000\n>> +++ actual 2015-08-24 22:54:44.611272643 +0000\n>> @@ -1 +1 @@\n>> -rfc1991-signed-tag RFC1991 signed tag\n>> +rfc1991-signed-tagRFC1991 signed tag\n>> not ok 99 - list tag with rfc1991 signature\n>\n> Thats weird, I just ran all tests, and nothing failed.\n\nYou may be missing GPG or RFC1991 prerequisites and not running this\nparticular test, which could be why you missed it.\n\nYour builtin/tag.c calls show_ref_array_item() with \n\n\t\"%(align:16,left)%(refname:short)%(end)\"\n\nas the format, and \"rfc1991-signed-tag\" is longer than 16.\n\nAnd immediately after showing that, there is this hack at the end of\nshow_ref_array_item() function, which I _think_ should not be there\nin ref-filter.c:show_ref_array_item() in the first place.\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\nThis belongs to the caller who knows that it is dealing with a tag.\n\nBut that broken design aside, the reason why this breaks is because\nyou do not have a separating SP after the aligned short refs.\n\nI didn't check how wide the original is supposed to be, but perhaps\nchanging builtin/tag.c this way\n\n\t\tif (filter->lines)\n-\t\t\tformat = \"%(align:16,left)%(refname:short)%(end)\";\n+\t\t\tformat = \"%(align:15,left)%(refname:short)%(end) \";\n\t\telse\n\t\t\tformat = \"%(refname:short)\";\n\t}\n\nmay be one way to fix it. Check the original \"tag -l\" output for\ntags whose names are 14, 15, 16 and 17 display-columns wide and try\nto match it.\n\nAnd then move the tag-specific code at the end of\nshow_ref_array_item() to here:\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+\tfor (i = 0; i < array.nr; i++) {\n\t\tshow_ref_array_item(array.items[i], format, QUOTE_NONE, filter->lines);\n+\t\tif (lines) {\n+\t\t\tstruct object_id oid;\n+\t\t\thashcpy(oid.hash, info->objectname);\n+\t\t\tshow_tag_lines(&oid, lines);\n+\t\t}\n+\t}\n\nperhaps.\n"},{"id":"268684","messageId":"xmqqvbc3xifj.fsf@gitster.dls.corp.google.com","threadId":"40155","inReplyTo":"xmqqzj1fxjpj.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-25T19:43:44Z","receivedAt":"2015-08-25T19:43:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I didn't check how wide the original is supposed to be, but perhaps\n> changing builtin/tag.c this way\n>\n> \t\tif (filter->lines)\n> -\t\t\tformat = \"%(align:16,left)%(refname:short)%(end)\";\n> +\t\t\tformat = \"%(align:15,left)%(refname:short)%(end) \";\n> \t\telse\n> \t\t\tformat = \"%(refname:short)\";\n> \t}\n>\n> may be one way to fix it. Check the original \"tag -l\" output for\n> tags whose names are 14, 15, 16 and 17 display-columns wide and try\n> to match it.\n\nHeh, I did it myself. %(align:15) with trailing whitespace is what\nyou want.\n\nAn alternative way to spell it would be\n\n \"%(align:16,left)%(refname:short) %(end)\"\n\nI don't know which one is more readable, though.\n"},{"id":"268713","messageId":"CAOLa=ZSu7XyZhWUL=25dnkiGm2mNDyFhXwS_JqudyfKPE5sisQ@mail.gmail.com","threadId":"40155","inReplyTo":"xmqqzj1fxjpj.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-26T05:56:58Z","receivedAt":"2015-08-26T05:56:58Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Aug 26, 2015 at 12:46 AM, Junio C Hamano wrote:\n> Karthik Nayak writes:\n>\n>>> Here is what I see...\n>>>\n>>> ok 98 - verifying rfc1991 signature\n>>>\n>>> expecting success:\n>>> echo \"rfc1991\" >gpghome/gpg.conf &&\n>>> echo \"rfc1991-signed-tag RFC1991 signed tag\" >expect &&\n>>> git tag -l -n1 rfc1991-signed-tag >actual &&\n>>> test_cmp expect actual &&\n>>> git tag -l -n2 rfc1991-signed-tag >actual &&\n>>> test_cmp expect actual &&\n>>> git tag -l -n999 rfc1991-signed-tag >actual &&\n>>> test_cmp expect actual\n>>>\n>>> --- expect 2015-08-24 22:54:44.607272653 +0000\n>>> +++ actual 2015-08-24 22:54:44.611272643 +0000\n>>> @@ -1 +1 @@\n>>> -rfc1991-signed-tag RFC1991 signed tag\n>>> +rfc1991-signed-tagRFC1991 signed tag\n>>> not ok 99 - list tag with rfc1991 signature\n>>\n>> Thats weird, I just ran all tests, and nothing failed.\n>\n> You may be missing GPG or RFC1991 prerequisites and not running this\n> particular test, which could be why you missed it.\n>\n\nThat explains.\n\n> Your builtin/tag.c calls show_ref_array_item() with\n>\n> \"%(align:16,left)%(refname:short)%(end)\"\n>\n> as the format, and \"rfc1991-signed-tag\" is longer than 16.\n>\n> And immediately after showing that, there is this hack at the end of\n> show_ref_array_item() function, which I _think_ should not be there\n> in ref-filter.c:show_ref_array_item() in the first place.\n>\n> if (lines > 0) {\n> struct object_id oid;\n> hashcpy(oid.hash, info->objectname);\n> show_tag_lines(&oid, lines);\n> }\n>\n> This belongs to the caller who knows that it is dealing with a tag.\n>\n\nExplained the idea behind this below.\n\n> But that broken design aside, the reason why this breaks is because\n> you do not have a separating SP after the aligned short refs.\n>\n\nMakes sense.\n\n> I didn't check how wide the original is supposed to be, but perhaps\n> changing builtin/tag.c this way\n>\n> if (filter->lines)\n> - format = \"%(align:16,left)%(refname:short)%(end)\";\n> + format = \"%(align:15,left)%(refname:short)%(end) \";\n> else\n> format = \"%(refname:short)\";\n> }\n>\n> may be one way to fix it. Check the original \"tag -l\" output for\n> tags whose names are 14, 15, 16 and 17 display-columns wide and try\n> to match it.\n>\n\nThat should be the fix, since it's a space problem.\n\n> And then move the tag-specific code at the end of\n> show_ref_array_item() to here:\n>\n> verify_ref_format(format);\n> filter_refs(&array, filter, FILTER_REFS_TAGS);\n> ref_array_sort(sorting, &array);\n>\n> - for (i = 0; i < array.nr; i++)\n> + for (i = 0; i < array.nr; i++) {\n> show_ref_array_item(array.items[i], format, QUOTE_NONE, filter->lines);\n> + if (lines) {\n> + struct object_id oid;\n> + hashcpy(oid.hash, info->objectname);\n> + show_tag_lines(&oid, lines);\n> + }\n> + }\n>\n> perhaps.\n\nThe problem with doing this is, the lines need to be displayed\nimmediately after the refname,\nfollowed by a newline, what you're suggesting breaks that flow.\n\nWe could pass a boolean to show_ref_array_item() and print a newline if no\nlines are to be printed and probably print the newline in tag.c\nitself, but seems confusing to me.\n\n\n> Heh, I did it myself. %(align:15) with trailing whitespace is what\n> you want.\n>\n> An alternative way to spell it would be\n>\n> \"%(align:16,left)%(refname:short) %(end)\"\n>\n> I don't know which one is more readable, though.\n\nI find this more readable, since the space is clearly visible, unlike\nformat = \"%(align:15,left)%(refname:short)%(end) \"; in which the space\nafter %(end) is easily unnoticeable.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268719","messageId":"CAOLa=ZTAaRrq+4_A8fs76xFQ4KQZpJscSP3s+bK2q5Rw9JJ34g@mail.gmail.com","threadId":"40155","inReplyTo":"xmqqlhczz1yz.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v13 04/12] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-26T06:41:21Z","receivedAt":"2015-08-26T06:41:21Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Aug 25, 2015 at 11:26 PM, Junio C Hamano wrote:\n> Karthik Nayak writes:\n>\n>> I like the idea of using atomv->handler() a lot, mostly cause this\n>> would eventually\n>> help us clean up populate_atom() which currently seems like a huge dump of code.\n>\n> I think you already said that last time we had this discussion ;-)\n>\n> http://thread.gmane.org/gmane.comp.version-control.git/275537/focus=275778\n>\n\nYes :) I really want to clean that up eventually, every since Duy\nmentioned about it.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268722","messageId":"CAOLa=ZS=Mtz0ny0tUWRBY0vfAgiRzaszeF0m_pxnK3+VGn1eVg@mail.gmail.com","threadId":"40155","inReplyTo":"xmqq37z82u2a.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-26T10:07:07Z","receivedAt":"2015-08-26T10:07:07Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Aug 25, 2015 at 4:05 AM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Karthik Nayak writes:\n>>\n>>> On Mon, Aug 24, 2015 at 10:57 PM, Junio C Hamano wrote:\n>>>> Karthik Nayak writes:\n>>>> ...\n>>>>> + performed. If used with '--quote' everything in between %(align:..)\n>>>>> + and %(end) is quoted.\n>>> ...\n>>> I might have misunderstood you, But based on the discussion held here\n>>> (thread.gmane.org/gmane.comp.version-control.git/276140)\n>>> I thought we wanted everything inside the %(align) .... %(end) atoms\n>>> to be quoted.\n>>\n>> Perhaps I misunderstood your intention in the doc.\n>>\n>> I took \"everything in between %(align:...) and %(end) is quoted\" to\n>> mean that\n>>\n>> %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end)\n>>\n>> can never satisfy %(if:empty), because %(align)%(end) would expand\n>> to a string that has two single-quotes, that is not an empty string.\n>>\n>> If that is not what would happen in the \"branch --list\" enhancement,\n>> then the proposed behaviour is good, but the above documentation would\n>> need to be updated when it happens, I think. It at least is misleading.\n>\n> OK, now I checked the code, and I _think_ the recursive logic is\n> doing the right thing (modulo minor nits on comment-vs-code\n> discrepancy and code structure I sent separately).\n>\n\nFor the current code %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end)\nwould print non-empty, I guess the documentation holds in that case.\nNot sure if we require it to print non-empty.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268724","messageId":"vpqd1yap8ml.fsf@anie.imag.fr","threadId":"40155","inReplyTo":"CAOLa=ZS=Mtz0ny0tUWRBY0vfAgiRzaszeF0m_pxnK3+VGn1eVg@mail.gmail.com","subject":"Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-08-26T11:54:58Z","receivedAt":"2015-08-26T11:54:58Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Karthik Nayak writes:\n\n> On Tue, Aug 25, 2015 at 4:05 AM, Junio C Hamano wrote:\n>> Junio C Hamano writes:\n>>\n>>> Karthik Nayak writes:\n>>>\n>>>> On Mon, Aug 24, 2015 at 10:57 PM, Junio C Hamano wrote:\n>>>>> Karthik Nayak writes:\n>>>>> ...\n>>>>>> + performed. If used with '--quote' everything in between %(align:..)\n>>>>>> + and %(end) is quoted.\n>>>> ...\n>>>> I might have misunderstood you, But based on the discussion held here\n>>>> (thread.gmane.org/gmane.comp.version-control.git/276140)\n>>>> I thought we wanted everything inside the %(align) .... %(end) atoms\n>>>> to be quoted.\n>>>\n>>> Perhaps I misunderstood your intention in the doc.\n>>>\n>>> I took \"everything in between %(align:...) and %(end) is quoted\" to\n>>> mean that\n>>>\n>>> %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end)\n>>>\n>>> can never satisfy %(if:empty), because %(align)%(end) would expand\n>>> to a string that has two single-quotes, that is not an empty string.\n>>>\n>>> If that is not what would happen in the \"branch --list\" enhancement,\n>>> then the proposed behaviour is good, but the above documentation would\n>>> need to be updated when it happens, I think. It at least is misleading.\n>>\n>> OK, now I checked the code, and I _think_ the recursive logic is\n>> doing the right thing (modulo minor nits on comment-vs-code\n>> discrepancy and code structure I sent separately).\n>>\n>\n> For the current code %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end)\n> would print non-empty, I guess the documentation holds in that case.\n> Not sure if we require it to print non-empty.\n\nYou don't want the %(if) condition to depend on whether\n--shell/--python/... is used. Since %(if:empty)%(align)%(end)%(then)\nholds when you don't use --shell, you also want it to hold when you\nquote. IOW, you should check for emptyness before (or actually without)\ndoing the quoting. I guess this is what you're doing, and if so, I think\nit's \"The Right Thing\".\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"268725","messageId":"xmqq6142w1xf.fsf@gitster.dls.corp.google.com","threadId":"40155","inReplyTo":"CAOLa=ZSu7XyZhWUL=25dnkiGm2mNDyFhXwS_JqudyfKPE5sisQ@mail.gmail.com","subject":"Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-26T14:37:48Z","receivedAt":"2015-08-26T14:37:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> On Wed, Aug 26, 2015 at 12:46 AM, Junio C Hamano wrote:\n>> Karthik Nayak writes:\n>>\n>> I didn't check how wide the original is supposed to be, but perhaps\n>> changing builtin/tag.c this way\n>>\n>> if (filter->lines)\n>> - format = \"%(align:16,left)%(refname:short)%(end)\";\n>> + format = \"%(align:15,left)%(refname:short)%(end) \";\n>> else\n>> format = \"%(refname:short)\";\n>> }\n>>\n>> may be one way to fix it. Check the original \"tag -l\" output for\n>> tags whose names are 14, 15, 16 and 17 display-columns wide and try\n>> to match it.\n>\n> That should be the fix, since it's a space problem.\n> ....\n> The problem with doing this is, the lines need to be displayed\n> immediately after the refname,\n> followed by a newline, what you're suggesting breaks that flow.\n\nThat is only because show_ref_array_item() does too much. The\nfunction is given a placeholder string and a set of data to fill the\nplaceholder with for an item, and the reason why the caller\nrepeatedly calls it, once per item it has, is because it wants to\nproduce a one-line-per-item output. An alternative valid way to\nstructure the API is to have it format into a string and leave the\nprinting to the caller. You can give a new format_ref_array_item()\nthat does not print but fills a strbuf to this caller, make\nshow_ref_array_item() a thin wrapper that calls it and prints it\nwith the final LF for other callers.\n\nAnother alternate approach, if you want to give \"tag -l\" annotation\navailable to for-each-ref, would be to do this:\n\n if (filter->lines)\n format = xstrfmt(\"%%(align:15,left)%%(refname:short)%%(end) \"\n \"%%(contents:lines=%s)\", filter->lines);\n else\n format = \"%(refname:short)\";\n\nand teach a new %(contents:lines=1) atom. That way, you can lose\nthe ugly special case call to show_tag_lines() that can only be at\nthe end of the output. I somehow think this is a better approach.\n\nOf course you can (and probably would want to) do both, giving a\nbit lower level \"emit to a strbuf\" function to the callers _and_\nlosing hardcoded call to show_tag_lines().\n"},{"id":"268727","messageId":"xmqqy4gyukag.fsf@gitster.dls.corp.google.com","threadId":"40155","inReplyTo":"vpqd1yap8ml.fsf@anie.imag.fr","subject":"Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-26T15:44:07Z","receivedAt":"2015-08-26T15:44:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n>> For the current code %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end)\n>> would print non-empty, I guess the documentation holds in that case.\n>> Not sure if we require it to print non-empty.\n>\n> You don't want the %(if) condition to depend on whether\n> --shell/--python/... is used. Since %(if:empty)%(align)%(end)%(then)\n> holds when you don't use --shell, you also want it to hold when you\n> quote. IOW, you should check for emptyness before (or actually without)\n> doing the quoting. I guess this is what you're doing, and if so, I think\n> it's \"The Right Thing\".\n\nI agree that %(align)%(end) should expand to empty and %(if:empty)...%(then)\nshould look at that empty string without quoting. So \n\n %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end)\n\nshould give \"Empty\"; otherwise the code is buggy, I think.\n"},{"id":"268728","messageId":"xmqqtwrmuk7b.fsf@gitster.dls.corp.google.com","threadId":"40155","inReplyTo":"xmqqy4gyukag.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-26T15:46:00Z","receivedAt":"2015-08-26T15:46:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Matthieu Moy writes:\n>\n>>> For the current code %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end)\n>>> would print non-empty, I guess the documentation holds in that case.\n>>> Not sure if we require it to print non-empty.\n>>\n>> You don't want the %(if) condition to depend on whether\n>> --shell/--python/... is used. Since %(if:empty)%(align)%(end)%(then)\n>> holds when you don't use --shell, you also want it to hold when you\n>> quote. IOW, you should check for emptyness before (or actually without)\n>> doing the quoting. I guess this is what you're doing, and if so, I think\n>> it's \"The Right Thing\".\n>\n> I agree that %(align)%(end) should expand to empty and %(if:empty)...%(then)\n> should look at that empty string without quoting. So \n>\n> %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end)\n>\n> should give \"Empty\"; otherwise the code is buggy, I think.\n\n(I shouldn't be typing while eating...)\n\nIt should give \"Empty\", but the --shell/--python/... may make the\nwhole \"Empty\", as the result of %(if:...)...%(end), be quoted. So\nyou may see \"'Empty'\" in the output.\n"},{"id":"268729","messageId":"vpq8u8ym4ns.fsf@anie.imag.fr","threadId":"40155","inReplyTo":"xmqqtwrmuk7b.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-08-26T15:48:55Z","receivedAt":"2015-08-26T15:48:55Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> Junio C Hamano writes:\n>\n>> Matthieu Moy writes:\n>>\n>>>> For the current code %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end)\n>>>> would print non-empty, I guess the documentation holds in that case.\n>>>> Not sure if we require it to print non-empty.\n>>>\n>>> You don't want the %(if) condition to depend on whether\n>>> --shell/--python/... is used. Since %(if:empty)%(align)%(end)%(then)\n>>> holds when you don't use --shell, you also want it to hold when you\n>>> quote. IOW, you should check for emptyness before (or actually without)\n>>> doing the quoting. I guess this is what you're doing, and if so, I think\n>>> it's \"The Right Thing\".\n>>\n>> I agree that %(align)%(end) should expand to empty and %(if:empty)...%(then)\n>> should look at that empty string without quoting. So \n>>\n>> %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end)\n>>\n>> should give \"Empty\"; otherwise the code is buggy, I think.\n>\n> (I shouldn't be typing while eating...)\n>\n> It should give \"Empty\", but the --shell/--python/... may make the\n> whole \"Empty\", as the result of %(if:...)...%(end), be quoted. So\n> you may see \"'Empty'\" in the output.\n\nAgreed (with both points).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"268733","messageId":"55DDE4DB.2070504@alum.mit.edu","threadId":"40155","inReplyTo":"1440214788-1309-6-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v13 05/12] ref-filter: add option to filter out tags, branches and remotes","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-08-26T16:10:03Z","receivedAt":"2015-08-26T16:10:03Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Comments inline.\n\nOn 08/22/2015 05:39 AM, Karthik Nayak wrote:\n> From: Karthik Nayak \n> \n> Add a function called 'for_each_reftype_fullpath()' to refs.{c,h}\n> which iterates through each ref for the given path without trimming\n> the path and also accounting for broken refs, if mentioned.\n> \n> Add 'filter_ref_kind()' in ref-filter.c to check the kind of ref being\n> handled and return the kind to 'ref_filter_handler()', where we\n> discard refs which we do not need and assign the kind to needed refs.\n> \n> Mentored-by: Christian Couder \n> Mentored-by: Matthieu Moy \n> Signed-off-by: Karthik Nayak \n> ---\n> ref-filter.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----\n> ref-filter.h | 12 ++++++++++--\n> refs.c | 9 +++++++++\n> refs.h | 1 +\n> 4 files changed, 74 insertions(+), 7 deletions(-)\n> \n> diff --git a/ref-filter.c b/ref-filter.c\n> index ffec10a..d5fae1a 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -1123,6 +1123,36 @@ static struct ref_array_item *new_ref_array_item(const char *refname,\n> \treturn ref;\n> }\n> \n> +static int filter_ref_kind(struct ref_filter *filter, const char *refname)\n> +{\n> +\tunsigned int i;\n> +\n> +\tstatic struct {\n> +\t\tconst char *prefix;\n> +\t\tunsigned int kind;\n> +\t} ref_kind[] = {\n> +\t\t{ \"refs/heads/\" , FILTER_REFS_BRANCHES },\n> +\t\t{ \"refs/remotes/\" , FILTER_REFS_REMOTES },\n> +\t\t{ \"refs/tags/\", FILTER_REFS_TAGS}\n> +\t};\n> +\n> +\tif (filter->kind == FILTER_REFS_BRANCHES)\n> +\t\treturn FILTER_REFS_BRANCHES;\n> +\telse if (filter->kind == FILTER_REFS_REMOTES)\n> +\t\treturn FILTER_REFS_REMOTES;\n> +\telse if (filter->kind == FILTER_REFS_TAGS)\n> +\t\treturn FILTER_REFS_TAGS;\n> +\telse if (!strcmp(refname, \"HEAD\"))\n> +\t\treturn FILTER_REFS_DETACHED_HEAD;\n\nI think this would be clearer written like so:\n\n if (filter->kind == FILTER_REFS_BRANCHES ||\n filter->kind == FILTER_REFS_REMOTES ||\n filter->kind == FILTER_REFS_TAGS)\n return filter->kind;\n if (!strcmp(refname, \"HEAD\"))\n return FILTER_REFS_DETACHED_HEAD;\n\nOr, even better, if you can set filter->kind to zero if it is not one of\nthese constants, then you could simplify this to\n\n if (filter->kind)\n return filter->kind;\n if (!strcmp(refname, \"HEAD\"))\n return FILTER_REFS_DETACHED_HEAD;\n\n> +\n> +\tfor (i = 0; i < ARRAY_SIZE(ref_kind); i++) {\n> +\t\tif (starts_with(refname, ref_kind[i].prefix))\n> +\t\t\treturn ref_kind[i].kind;\n> +\t}\n> +\n> +\treturn FILTER_REFS_OTHERS;\n> +}\n> +\n> /*\n> * A call-back given to for_each_ref(). Filter refs and keep them for\n> * later object processing.\n> @@ -1133,6 +1163,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n> \tstruct ref_filter *filter = ref_cbdata->filter;\n> \tstruct ref_array_item *ref;\n> \tstruct commit *commit = NULL;\n> +\tunsigned int kind;\n> \n> \tif (flag & REF_BAD_NAME) {\n> \t\twarning(\"ignoring ref with broken name %s\", refname);\n> @@ -1144,6 +1175,10 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n> \t\treturn 0;\n> \t}\n> \n> +\tkind = filter_ref_kind(filter, refname);\n> +\tif (!(kind & filter->kind))\n> +\t\treturn 0;\n> +\n> \tif (*filter->name_patterns && !match_name_as_path(filter->name_patterns, refname))\n> \t\treturn 0;\n> \n> @@ -1175,6 +1210,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n> \n> \tREALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);\n> \tref_cbdata->array->items[ref_cbdata->array->nr++] = ref;\n> +\tref->kind = kind;\n> \treturn 0;\n> }\n> \n> @@ -1251,16 +1287,29 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int\n> {\n> \tstruct ref_filter_cbdata ref_cbdata;\n> \tint ret = 0;\n> +\tunsigned int broken = 0;\n> \n> \tref_cbdata.array = array;\n> \tref_cbdata.filter = filter;\n> \n> \t/* Simple per-ref filtering */\n> -\tif (type & (FILTER_REFS_ALL | FILTER_REFS_INCLUDE_BROKEN))\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)\n> +\tif (type & FILTER_REFS_INCLUDE_BROKEN) {\n> +\t\ttype &= ~FILTER_REFS_INCLUDE_BROKEN;\n> +\t\tbroken = 1;\n> +\t}\n\nI wouldn't mask out the FILTER_REFS_INCLUDE_BROKEN bit here. Instead I\nwould write\n\n> +\n> +\tfilter->kind = type;\n\nas\n\n filter->kind = type & FILTER_REFS_KIND_MASK;\n\nwhere FILTER_REFS_KIND_MASK is the OR of all of the kind bits. The\nadvantage is that this approach would remain correct if more bits are\nadded to type. Then in the following if statements, test filter->kind\ninstead of type.\n\n> +\tif (type == FILTER_REFS_BRANCHES)\n> +\t\tret = for_each_reftype_fullpath(ref_filter_handler, \"refs/heads/\", broken, &ref_cbdata);\n> +\telse if (type == FILTER_REFS_REMOTES)\n> +\t\tret = for_each_reftype_fullpath(ref_filter_handler, \"refs/remotes/\", broken, &ref_cbdata);\n> +\telse if (type == FILTER_REFS_TAGS)\n> +\t\tret = for_each_reftype_fullpath(ref_filter_handler, \"refs/tags/\", broken, &ref_cbdata);\n> +\telse if (type & FILTER_REFS_ALL) {\n> +\t\tret = for_each_reftype_fullpath(ref_filter_handler, \"\", broken, &ref_cbdata);\n> +\t\tif (type & FILTER_REFS_DETACHED_HEAD)\n> +\t\t\thead_ref(ref_filter_handler, &ref_cbdata);\n\nThe usual promise of the for_each_ref functions is that they stop\niterating if the function ever returns a nonzero value. So the above\nshould be\n\n if (! ret && (type & FILTER_REFS_DETACHED_HEAD))\n ret = head_ref(ref_filter_handler, &ref_cbdata);\n\nAlso, these functions usually iterate in lexicographic order, so I think\nyou should process HEAD before the others.\n\nBut there's another problem here. It seems like\nFILTER_REFS_DETACHED_HEAD is only processed if (type & FILTER_REFS_ALL)\nis nonzero. But shouldn't it be allowed to process *only* HEAD?\n\nSo, finally, I think this code should look like\n\n else if (!filter->kind)\n die(\"filter_refs: invalid type\");\n else {\n if (filter->kind & FILTER_REFS_DETACHED_HEAD)\n ret = head_ref(ref_filter_handler, &ref_cbdata);\n if (! ret && (filter->kind & FILTER_REFS_ALL))\n ret =\nfor_each_reftype_fullpath(ref_filter_handler, \"\", broken, &ref_cbdata);\n }\n\n> +\t} else\n> \t\tdie(\"filter_refs: invalid type\");\n> \n> \t/* Filters that need revision walking */\n> diff --git a/ref-filter.h b/ref-filter.h\n> index 45026d0..99f081b 100644\n> --- a/ref-filter.h\n> +++ b/ref-filter.h\n> @@ -13,8 +13,14 @@\n> #define QUOTE_PYTHON 4\n> #define QUOTE_TCL 8\n> \n> -#define FILTER_REFS_INCLUDE_BROKEN 0x1\n> -#define FILTER_REFS_ALL 0x2\n> +#define FILTER_REFS_INCLUDE_BROKEN 0x0001\n> +#define FILTER_REFS_TAGS 0x0002\n> +#define FILTER_REFS_BRANCHES 0x0004\n> +#define FILTER_REFS_REMOTES 0x0008\n> +#define FILTER_REFS_OTHERS 0x0010\n> +#define FILTER_REFS_ALL (FILTER_REFS_TAGS | FILTER_REFS_BRANCHES | \\\n> +\t\t\t\t FILTER_REFS_REMOTES | FILTER_REFS_OTHERS)\n> +#define FILTER_REFS_DETACHED_HEAD 0x0020\n\nDo you expect it to be useful to OR these bits together, or will (in\npractice) all callers want to iterate over one or the other type of\nreference, or all references? I ask because allowing an arbitrary\nbitmask complicates the code a bit (otherwise filter_ref_kind() wouldn't\nbe needed).\n\nIf there is code that wants to iterate over, say, branches *and* tags,\nthen maybe it is an acceptable imposition for it to make two function calls,\n\n for_each_reftype_fullpath(fn, \"refs/heads/\", broken, cb_data) ||\n for_each_reftype_fullpath(fn, \"refs/tags/\", broken, cb_data);\n\nwhich might even be a bit more efficient because it only has to iterate\nover those two namespaces rather than all references.\n\nBut the more flexible bitmask code is not a lot of extra overhead, so if\nyou think it will have a use case then it is fine to keep this interface.\n\n> \n> struct atom_value;\n> \n> @@ -27,6 +33,7 @@ struct ref_sorting {\n> struct ref_array_item {\n> \tunsigned char objectname[20];\n> \tint flag;\n> +\tunsigned int kind;\n> \tconst char *symref;\n> \tstruct commit *commit;\n> \tstruct atom_value *value;\n> @@ -51,6 +58,7 @@ struct ref_filter {\n> \tstruct commit *merge_commit;\n> \n> \tunsigned int with_commit_tag_algo : 1;\n> +\tunsigned int kind;\n> };\n> \n> struct ref_filter_cbdata {\n> diff --git a/refs.c b/refs.c\n> index 4e15f60..3266617 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2150,6 +2150,15 @@ int for_each_replace_ref(each_ref_fn fn, void *cb_data)\n> \t\t\t strlen(git_replace_ref_base), 0, cb_data);\n> }\n> \n> +int for_each_reftype_fullpath(each_ref_fn fn, char *type, unsigned int broken, void *cb_data)\n> +{\n> +\tunsigned int flag = 0;\n> +\n> +\tif (broken)\n> +\t\tflag = DO_FOR_EACH_INCLUDE_BROKEN;\n> +\treturn do_for_each_ref(&ref_cache, type, fn, 0, flag, cb_data);\n> +}\n> +\n> int head_ref_namespaced(each_ref_fn fn, void *cb_data)\n> {\n> \tstruct strbuf buf = STRBUF_INIT;\n> diff --git a/refs.h b/refs.h\n> index e9a5f32..6e913ee 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -179,6 +179,7 @@ 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> extern int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data);\n> extern int for_each_glob_ref_in(each_ref_fn fn, const char *pattern, const char *prefix, void *cb_data);\n> +extern int for_each_reftype_fullpath(each_ref_fn fn, char *type, unsigned int broken, void *cb_data);\n\nThis function is most like for_each_ref_in(prefix, fn, cb_data).\nTherefore, I suggest that you rename the \"type\" parameter\" to \"prefix\",\nmaybe reorder its arguments, and maybe rename it (to\nfor_each_fullref_in()?) for consistency, and maybe put its declaration\nnext to that function's. (I see that the argument orders among these\nfunctions are already pretty inconsistent, but it seems best to be\nconsistent with the function that is most similar to it.)\n\nI think the \"type\"/\"prefix\" argument can be \"const char *\".\n\n> \n> extern int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);\n> extern int for_each_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);\n> \n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"268742","messageId":"CAOLa=ZQY83p96ECfyxeUr5s_zWo-u4ZPP1Ot79gSwaATiq__Pg@mail.gmail.com","threadId":"40155","inReplyTo":"vpq8u8ym4ns.fsf@anie.imag.fr","subject":"Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-26T19:11:49Z","receivedAt":"2015-08-26T19:11:49Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Aug 26, 2015 at 9:18 PM, Matthieu Moy\n wrote:\n> Junio C Hamano writes:\n>\n>> Junio C Hamano writes:\n>>\n>>> Matthieu Moy writes:\n>>>\n>>>>> For the current code %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end)\n>>>>> would print non-empty, I guess the documentation holds in that case.\n>>>>> Not sure if we require it to print non-empty.\n>>>>\n>>>> You don't want the %(if) condition to depend on whether\n>>>> --shell/--python/... is used. Since %(if:empty)%(align)%(end)%(then)\n>>>> holds when you don't use --shell, you also want it to hold when you\n>>>> quote. IOW, you should check for emptyness before (or actually without)\n>>>> doing the quoting. I guess this is what you're doing, and if so, I think\n>>>> it's \"The Right Thing\".\n>>>\n>>> I agree that %(align)%(end) should expand to empty and %(if:empty)...%(then)\n>>> should look at that empty string without quoting. So\n>>>\n>>> %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end)\n>>>\n>>> should give \"Empty\"; otherwise the code is buggy, I think.\n>>\n>> (I shouldn't be typing while eating...)\n>>\n>> It should give \"Empty\", but the --shell/--python/... may make the\n>> whole \"Empty\", as the result of %(if:...)...%(end), be quoted. So\n>> you may see \"'Empty'\" in the output.\n>\n> Agreed (with both points).\n>\n\nThanks, will work on this.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268743","messageId":"CAOLa=ZS3Fj8EFMp+BXEomeW2PfkBJemoO2-8Tv=RBRb9xGmEyw@mail.gmail.com","threadId":"40155","inReplyTo":"xmqq6142w1xf.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-26T19:14:44Z","receivedAt":"2015-08-26T19:14:44Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Aug 26, 2015 at 8:07 PM, Junio C Hamano wrote:\n> Karthik Nayak writes:\n>\n>> On Wed, Aug 26, 2015 at 12:46 AM, Junio C Hamano wrote:\n>>> Karthik Nayak writes:\n>>>\n>>> I didn't check how wide the original is supposed to be, but perhaps\n>>> changing builtin/tag.c this way\n>>>\n>>> if (filter->lines)\n>>> - format = \"%(align:16,left)%(refname:short)%(end)\";\n>>> + format = \"%(align:15,left)%(refname:short)%(end) \";\n>>> else\n>>> format = \"%(refname:short)\";\n>>> }\n>>>\n>>> may be one way to fix it. Check the original \"tag -l\" output for\n>>> tags whose names are 14, 15, 16 and 17 display-columns wide and try\n>>> to match it.\n>>\n>> That should be the fix, since it's a space problem.\n>> ....\n>> The problem with doing this is, the lines need to be displayed\n>> immediately after the refname,\n>> followed by a newline, what you're suggesting breaks that flow.\n>\n> That is only because show_ref_array_item() does too much. The\n> function is given a placeholder string and a set of data to fill the\n> placeholder with for an item, and the reason why the caller\n> repeatedly calls it, once per item it has, is because it wants to\n> produce a one-line-per-item output. An alternative valid way to\n> structure the API is to have it format into a string and leave the\n> printing to the caller. You can give a new format_ref_array_item()\n> that does not print but fills a strbuf to this caller, make\n> show_ref_array_item() a thin wrapper that calls it and prints it\n> with the final LF for other callers.\n>\n> Another alternate approach, if you want to give \"tag -l\" annotation\n> available to for-each-ref, would be to do this:\n>\n> if (filter->lines)\n> format = xstrfmt(\"%%(align:15,left)%%(refname:short)%%(end) \"\n> \"%%(contents:lines=%s)\", filter->lines);\n> else\n> format = \"%(refname:short)\";\n>\n> and teach a new %(contents:lines=1) atom. That way, you can lose\n> the ugly special case call to show_tag_lines() that can only be at\n> the end of the output. I somehow think this is a better approach.\n>\n\nThis seems like a good approach, since contents is already an atom, this would\nfit in easily.\n\n> Of course you can (and probably would want to) do both, giving a\n> bit lower level \"emit to a strbuf\" function to the callers _and_\n> losing hardcoded call to show_tag_lines().\n\nYou're saying remove show_ref_array_item() (even the wrapper you mentioned\nabove) and just have something like format_ref_array_item() which\nwould output to a strbuf. and let the caller worry about the printing?\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268753","messageId":"xmqq61413iqy.fsf@gitster.dls.corp.google.com","threadId":"40155","inReplyTo":"CAOLa=ZS3Fj8EFMp+BXEomeW2PfkBJemoO2-8Tv=RBRb9xGmEyw@mail.gmail.com","subject":"Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-26T20:19:33Z","receivedAt":"2015-08-26T20:19:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> On Wed, Aug 26, 2015 at 8:07 PM, Junio C Hamano wrote:\n>>\n>> ... You can give a new format_ref_array_item()\n>> that does not print but fills a strbuf to this caller, make\n>> show_ref_array_item() a thin wrapper that calls it and prints it\n>> with the final LF for other callers.\n>>\n> You're saying remove show_ref_array_item() (even the wrapper you mentioned\n> above) and just have something like format_ref_array_item() which\n> would output to a strbuf. and let the caller worry about the printing?\n\nAmong the current callers, the one in builtin/tag.c that wants to\ntrigger show_tag_lines() hack embedded in show_ref_array_item()\nfunction can stop calling show_ref_array_item() and instead can do\n\n\tfor (i = 0; i < array.nr; i++) {\n\t\tstruct strbuf out = STRBUF_INIT;\n \tformat_ref_array_item(&out, ...);\n if (filter->lines) {\n \t... append tag lines to out ...\n\t\t}\n printf(\"%s\\n\", out.buf);\n strbuf_reset(&out);\n\t}\n \nThe current and future callers of show_ref_array_item() that do not\nwant to trigger the show_tag_liens() hack embedded in there may\nstill want it to print the formatted string including the trailing\nLF, so you can keep show_ref_array_item() as a thin wrapper around\nformat_ref_array_item() for them to call, e.g.\n\n\tshow_ref_array_item(...) {\n\t\tstruct strbuf out = STRBUF_INIT;\n \tformat_ref_array_item(&out, ...);\n printf(\"%s\\n\", out.buf);\n strbuf_release(&out);\n\t}\n\nBut if it has only one caller each, you may not even want to have\nshow_ref_array_item(), if you are going to do the \"output to strbuf\"\nvariant.\n\n\n\t\t \t\n"},{"id":"268785","messageId":"CAOLa=ZQh0MNwjAOLameh1f22LB=JyD7=FeROzDRikpoRXse7cw@mail.gmail.com","threadId":"40155","inReplyTo":"55DDE4DB.2070504@alum.mit.edu","subject":"Re: [PATCH v13 05/12] ref-filter: add option to filter out tags, branches and remotes","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-27T12:42:25Z","receivedAt":"2015-08-27T12:42:25Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Aug 26, 2015 at 9:40 PM, Michael Haggerty wrote:\n> Comments inline.\n>\n> On 08/22/2015 05:39 AM, Karthik Nayak wrote:\n>> From: Karthik Nayak \n>>\n>> Add a function called 'for_each_reftype_fullpath()' to refs.{c,h}\n>> which iterates through each ref for the given path without trimming\n>> the path and also accounting for broken refs, if mentioned.\n>>\n>> Add 'filter_ref_kind()' in ref-filter.c to check the kind of ref being\n>> handled and return the kind to 'ref_filter_handler()', where we\n>> discard refs which we do not need and assign the kind to needed refs.\n>>\n>> Mentored-by: Christian Couder \n>> Mentored-by: Matthieu Moy \n>> Signed-off-by: Karthik Nayak \n>> ---\n>> ref-filter.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----\n>> ref-filter.h | 12 ++++++++++--\n>> refs.c | 9 +++++++++\n>> refs.h | 1 +\n>> 4 files changed, 74 insertions(+), 7 deletions(-)\n>>\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> index ffec10a..d5fae1a 100644\n>> --- a/ref-filter.c\n>> +++ b/ref-filter.c\n>> @@ -1123,6 +1123,36 @@ static struct ref_array_item *new_ref_array_item(const char *refname,\n>> return ref;\n>> }\n>>\n>> +static int filter_ref_kind(struct ref_filter *filter, const char *refname)\n>> +{\n>> + unsigned int i;\n>> +\n>> + static struct {\n>> + const char *prefix;\n>> + unsigned int kind;\n>> + } ref_kind[] = {\n>> + { \"refs/heads/\" , FILTER_REFS_BRANCHES },\n>> + { \"refs/remotes/\" , FILTER_REFS_REMOTES },\n>> + { \"refs/tags/\", FILTER_REFS_TAGS}\n>> + };\n>> +\n>> + if (filter->kind == FILTER_REFS_BRANCHES)\n>> + return FILTER_REFS_BRANCHES;\n>> + else if (filter->kind == FILTER_REFS_REMOTES)\n>> + return FILTER_REFS_REMOTES;\n>> + else if (filter->kind == FILTER_REFS_TAGS)\n>> + return FILTER_REFS_TAGS;\n>> + else if (!strcmp(refname, \"HEAD\"))\n>> + return FILTER_REFS_DETACHED_HEAD;\n>\n> I think this would be clearer written like so:\n>\n> if (filter->kind == FILTER_REFS_BRANCHES ||\n> filter->kind == FILTER_REFS_REMOTES ||\n> filter->kind == FILTER_REFS_TAGS)\n> return filter->kind;\n> if (!strcmp(refname, \"HEAD\"))\n> return FILTER_REFS_DETACHED_HEAD;\n>\n\nThis looks good.\n\n> Or, even better, if you can set filter->kind to zero if it is not one of\n> these constants, then you could simplify this to\n>\n> if (filter->kind)\n> return filter->kind;\n> if (!strcmp(refname, \"HEAD\"))\n> return FILTER_REFS_DETACHED_HEAD;\n>\n\nBut then that would mean we would loose the ability of having each\nref_array_item\nknow its kind, as we return the kind and set it in ref_filter_handler().\n\nThis would work in the current scenario. This is particularly needed\nin the future for\nbranch.c using ref-filter where we might have to append \"remotes/\" for\nprinting remotes\nvia \"git branch - a\".\n\n>> +\n>> + for (i = 0; i < ARRAY_SIZE(ref_kind); i++) {\n>> + if (starts_with(refname, ref_kind[i].prefix))\n>> + return ref_kind[i].kind;\n>> + }\n>> +\n>> + return FILTER_REFS_OTHERS;\n>> +}\n>> +\n>> /*\n>> * A call-back given to for_each_ref(). Filter refs and keep them for\n>> * later object processing.\n>> @@ -1133,6 +1163,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n>> struct ref_filter *filter = ref_cbdata->filter;\n>> struct ref_array_item *ref;\n>> struct commit *commit = NULL;\n>> + unsigned int kind;\n>>\n>> if (flag & REF_BAD_NAME) {\n>> warning(\"ignoring ref with broken name %s\", refname);\n>> @@ -1144,6 +1175,10 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n>> return 0;\n>> }\n>>\n>> + kind = filter_ref_kind(filter, refname);\n>> + if (!(kind & filter->kind))\n>> + return 0;\n>> +\n>> if (*filter->name_patterns && !match_name_as_path(filter->name_patterns, refname))\n>> return 0;\n>>\n>> @@ -1175,6 +1210,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n>>\n>> REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);\n>> ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;\n>> + ref->kind = kind;\n>> return 0;\n>> }\n>>\n>> @@ -1251,16 +1287,29 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int\n>> {\n>> struct ref_filter_cbdata ref_cbdata;\n>> int ret = 0;\n>> + unsigned int broken = 0;\n>>\n>> ref_cbdata.array = array;\n>> ref_cbdata.filter = filter;\n>>\n>> /* Simple per-ref filtering */\n>> - if (type & (FILTER_REFS_ALL | FILTER_REFS_INCLUDE_BROKEN))\n>> - ret = for_each_rawref(ref_filter_handler, &ref_cbdata);\n>> - else if (type & FILTER_REFS_ALL)\n>> - ret = for_each_ref(ref_filter_handler, &ref_cbdata);\n>> - else if (type)\n>> + if (type & FILTER_REFS_INCLUDE_BROKEN) {\n>> + type &= ~FILTER_REFS_INCLUDE_BROKEN;\n>> + broken = 1;\n>> + }\n>\n> I wouldn't mask out the FILTER_REFS_INCLUDE_BROKEN bit here. Instead I\n> would write\n>\n>> +\n>> + filter->kind = type;\n>\n> as\n>\n> filter->kind = type & FILTER_REFS_KIND_MASK;\n>\n> where FILTER_REFS_KIND_MASK is the OR of all of the kind bits. The\n> advantage is that this approach would remain correct if more bits are\n> added to type. Then in the following if statements, test filter->kind\n> instead of type.\n>\n\nThats brilliant.\n\n>> + if (type == FILTER_REFS_BRANCHES)\n>> + ret = for_each_reftype_fullpath(ref_filter_handler, \"refs/heads/\", broken, &ref_cbdata);\n>> + else if (type == FILTER_REFS_REMOTES)\n>> + ret = for_each_reftype_fullpath(ref_filter_handler, \"refs/remotes/\", broken, &ref_cbdata);\n>> + else if (type == FILTER_REFS_TAGS)\n>> + ret = for_each_reftype_fullpath(ref_filter_handler, \"refs/tags/\", broken, &ref_cbdata);\n>> + else if (type & FILTER_REFS_ALL) {\n>> + ret = for_each_reftype_fullpath(ref_filter_handler, \"\", broken, &ref_cbdata);\n>> + if (type & FILTER_REFS_DETACHED_HEAD)\n>> + head_ref(ref_filter_handler, &ref_cbdata);\n>\n> The usual promise of the for_each_ref functions is that they stop\n> iterating if the function ever returns a nonzero value. So the above\n> should be\n>\n> if (! ret && (type & FILTER_REFS_DETACHED_HEAD))\n> ret = head_ref(ref_filter_handler, &ref_cbdata);\n>\n> Also, these functions usually iterate in lexicographic order, so I think\n> you should process HEAD before the others.\n\nThis is done on purpose, cause we need to print the HEAD ref separately\nso we print the last ref_array_item in the ref_array, free that memory and\nsort and print the rest, hence HEAD ref is attached to the last.\n\n>\n> But there's another problem here. It seems like\n> FILTER_REFS_DETACHED_HEAD is only processed if (type & FILTER_REFS_ALL)\n> is nonzero. But shouldn't it be allowed to process *only* HEAD?\n>\n> So, finally, I think this code should look like\n>\n> else if (!filter->kind)\n> die(\"filter_refs: invalid type\");\n> else {\n> if (filter->kind & FILTER_REFS_DETACHED_HEAD)\n> ret = head_ref(ref_filter_handler, &ref_cbdata);\n> if (! ret && (filter->kind & FILTER_REFS_ALL))\n> ret =\n> for_each_reftype_fullpath(ref_filter_handler, \"\", broken, &ref_cbdata);\n> }\n>\n\nSo finally something like this perhaps\n\n if (!filter->kind)\n die(\"filter_refs: invalid type\");\n else {\n if (filter->kind == FILTER_REFS_BRANCHES)\n ret = for_each_reftype_fullpath(ref_filter_handler,\n\"refs/heads/\", broken, &ref_cbdata);\n else if (filter->kind == FILTER_REFS_REMOTES)\n ret = for_each_reftype_fullpath(ref_filter_handler,\n\"refs/remotes/\", broken, &ref_cbdata);\n else if (filter->kind == FILTER_REFS_TAGS)\n ret = for_each_reftype_fullpath(ref_filter_handler,\n\"refs/tags/\", broken, &ref_cbdata);\n else if (filter->kind & FILTER_REFS_ALL)\n ret = for_each_reftype_fullpath(ref_filter_handler, \"\",\nbroken, &ref_cbdata);\n if (filter->kind & FILTER_REFS_DETACHED_HEAD)\n head_ref(ref_filter_handler, &ref_cbdata);\n }\n\n>> + } else\n>> die(\"filter_refs: invalid type\");\n>>\n>> /* Filters that need revision walking */\n>> diff --git a/ref-filter.h b/ref-filter.h\n>> index 45026d0..99f081b 100644\n>> --- a/ref-filter.h\n>> +++ b/ref-filter.h\n>> @@ -13,8 +13,14 @@\n>> #define QUOTE_PYTHON 4\n>> #define QUOTE_TCL 8\n>>\n>> -#define FILTER_REFS_INCLUDE_BROKEN 0x1\n>> -#define FILTER_REFS_ALL 0x2\n>> +#define FILTER_REFS_INCLUDE_BROKEN 0x0001\n>> +#define FILTER_REFS_TAGS 0x0002\n>> +#define FILTER_REFS_BRANCHES 0x0004\n>> +#define FILTER_REFS_REMOTES 0x0008\n>> +#define FILTER_REFS_OTHERS 0x0010\n>> +#define FILTER_REFS_ALL (FILTER_REFS_TAGS | FILTER_REFS_BRANCHES | \\\n>> + FILTER_REFS_REMOTES | FILTER_REFS_OTHERS)\n>> +#define FILTER_REFS_DETACHED_HEAD 0x0020\n>\n> Do you expect it to be useful to OR these bits together, or will (in\n> practice) all callers want to iterate over one or the other type of\n> reference, or all references? I ask because allowing an arbitrary\n> bitmask complicates the code a bit (otherwise filter_ref_kind() wouldn't\n> be needed).\n>\n> If there is code that wants to iterate over, say, branches *and* tags,\n> then maybe it is an acceptable imposition for it to make two function calls,\n>\n> for_each_reftype_fullpath(fn, \"refs/heads/\", broken, cb_data) ||\n> for_each_reftype_fullpath(fn, \"refs/tags/\", broken, cb_data);\n>\n> which might even be a bit more efficient because it only has to iterate\n> over those two namespaces rather than all references.\n>\n> But the more flexible bitmask code is not a lot of extra overhead, so if\n> you think it will have a use case then it is fine to keep this interface.\n>\n\nYeah, 'git branch -l' has '-a' option which iterates over remotes, branches and\nalso the head ref. Hence the option. Thats also why if only a single\ntype is needed\nwe only iterate through those refs.\n\n>>\n>> struct atom_value;\n>>\n>> @@ -27,6 +33,7 @@ struct ref_sorting {\n>> struct ref_array_item {\n>> unsigned char objectname[20];\n>> int flag;\n>> + unsigned int kind;\n>> const char *symref;\n>> struct commit *commit;\n>> struct atom_value *value;\n>> @@ -51,6 +58,7 @@ struct ref_filter {\n>> struct commit *merge_commit;\n>>\n>> unsigned int with_commit_tag_algo : 1;\n>> + unsigned int kind;\n>> };\n>>\n>> struct ref_filter_cbdata {\n>> diff --git a/refs.c b/refs.c\n>> index 4e15f60..3266617 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -2150,6 +2150,15 @@ int for_each_replace_ref(each_ref_fn fn, void *cb_data)\n>> strlen(git_replace_ref_base), 0, cb_data);\n>> }\n>>\n>> +int for_each_reftype_fullpath(each_ref_fn fn, char *type, unsigned int broken, void *cb_data)\n>> +{\n>> + unsigned int flag = 0;\n>> +\n>> + if (broken)\n>> + flag = DO_FOR_EACH_INCLUDE_BROKEN;\n>> + return do_for_each_ref(&ref_cache, type, fn, 0, flag, cb_data);\n>> +}\n>> +\n>> int head_ref_namespaced(each_ref_fn fn, void *cb_data)\n>> {\n>> struct strbuf buf = STRBUF_INIT;\n>> diff --git a/refs.h b/refs.h\n>> index e9a5f32..6e913ee 100644\n>> --- a/refs.h\n>> +++ b/refs.h\n>> @@ -179,6 +179,7 @@ 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>> extern int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data);\n>> extern int for_each_glob_ref_in(each_ref_fn fn, const char *pattern, const char *prefix, void *cb_data);\n>> +extern int for_each_reftype_fullpath(each_ref_fn fn, char *type, unsigned int broken, void *cb_data);\n>\n> This function is most like for_each_ref_in(prefix, fn, cb_data).\n> Therefore, I suggest that you rename the \"type\" parameter\" to \"prefix\",\n> maybe reorder its arguments, and maybe rename it (to\n> for_each_fullref_in()?) for consistency, and maybe put its declaration\n> next to that function's. (I see that the argument orders among these\n> functions are already pretty inconsistent, but it seems best to be\n> consistent with the function that is most similar to it.)\n>\n> I think the \"type\"/\"prefix\" argument can be \"const char *\".\n>\n\nAgree with everything said here, will do.\n\n>>\n>> extern int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);\n>> extern int for_each_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);\n>>\n>\n> Michael\n>\n> --\n> Michael Haggerty\n> mhagger@alum.mit.edu\n>\n\nThanks for the review.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268794","messageId":"55DF2BA1.2020107@alum.mit.edu","threadId":"40155","inReplyTo":"CAOLa=ZQh0MNwjAOLameh1f22LB=JyD7=FeROzDRikpoRXse7cw@mail.gmail.com","subject":"Re: [PATCH v13 05/12] ref-filter: add option to filter out tags, branches and remotes","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-08-27T15:24:17Z","receivedAt":"2015-08-27T15:24:17Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/27/2015 02:42 PM, Karthik Nayak wrote:\n> On Wed, Aug 26, 2015 at 9:40 PM, Michael Haggerty wrote:\n>> On 08/22/2015 05:39 AM, Karthik Nayak wrote:\n>>> [...]\n>>> + if (type == FILTER_REFS_BRANCHES)\n>>> + ret = for_each_reftype_fullpath(ref_filter_handler, \"refs/heads/\", broken, &ref_cbdata);\n>>> + else if (type == FILTER_REFS_REMOTES)\n>>> + ret = for_each_reftype_fullpath(ref_filter_handler, \"refs/remotes/\", broken, &ref_cbdata);\n>>> + else if (type == FILTER_REFS_TAGS)\n>>> + ret = for_each_reftype_fullpath(ref_filter_handler, \"refs/tags/\", broken, &ref_cbdata);\n>>> + else if (type & FILTER_REFS_ALL) {\n>>> + ret = for_each_reftype_fullpath(ref_filter_handler, \"\", broken, &ref_cbdata);\n>>> + if (type & FILTER_REFS_DETACHED_HEAD)\n>>> + head_ref(ref_filter_handler, &ref_cbdata);\n>>\n>> The usual promise of the for_each_ref functions is that they stop\n>> iterating if the function ever returns a nonzero value. So the above\n>> should be\n>>\n>> if (! ret && (type & FILTER_REFS_DETACHED_HEAD))\n>> ret = head_ref(ref_filter_handler, &ref_cbdata);\n>>\n>> Also, these functions usually iterate in lexicographic order, so I think\n>> you should process HEAD before the others.\n> \n> This is done on purpose, cause we need to print the HEAD ref separately\n> so we print the last ref_array_item in the ref_array, free that memory and\n> sort and print the rest, hence HEAD ref is attached to the last.\n\nWithout having looked at the other patches, this makes me wonder whether\nit makes sense to store HEAD in the ref_array at all or whether it\nshould be handled separately.\n\n>> But there's another problem here. It seems like\n>> FILTER_REFS_DETACHED_HEAD is only processed if (type & FILTER_REFS_ALL)\n>> is nonzero. But shouldn't it be allowed to process *only* HEAD?\n>>\n>> So, finally, I think this code should look like\n>>\n>> else if (!filter->kind)\n>> die(\"filter_refs: invalid type\");\n>> else {\n>> if (filter->kind & FILTER_REFS_DETACHED_HEAD)\n>> ret = head_ref(ref_filter_handler, &ref_cbdata);\n>> if (! ret && (filter->kind & FILTER_REFS_ALL))\n>> ret =\n>> for_each_reftype_fullpath(ref_filter_handler, \"\", broken, &ref_cbdata);\n>> }\n>>\n> \n> So finally something like this perhaps\n> \n> if (!filter->kind)\n> die(\"filter_refs: invalid type\");\n> else {\n> if (filter->kind == FILTER_REFS_BRANCHES)\n> ret = for_each_reftype_fullpath(ref_filter_handler,\n> \"refs/heads/\", broken, &ref_cbdata);\n> else if (filter->kind == FILTER_REFS_REMOTES)\n> ret = for_each_reftype_fullpath(ref_filter_handler,\n> \"refs/remotes/\", broken, &ref_cbdata);\n> else if (filter->kind == FILTER_REFS_TAGS)\n> ret = for_each_reftype_fullpath(ref_filter_handler,\n> \"refs/tags/\", broken, &ref_cbdata);\n> else if (filter->kind & FILTER_REFS_ALL)\n> ret = for_each_reftype_fullpath(ref_filter_handler, \"\",\n> broken, &ref_cbdata);\n> if (filter->kind & FILTER_REFS_DETACHED_HEAD)\n> head_ref(ref_filter_handler, &ref_cbdata);\n> }\n\nYes, but the last test should be\n\n if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))\n\nunless you have a reason not to follow the usual convention that a\nnonzero return value from fn means that the iteration should be aborted.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"268795","messageId":"CAOLa=ZS3_sSaMjkSchdQGE1s5bgg00DPWe=F+KsUdjQt=opDyg@mail.gmail.com","threadId":"40155","inReplyTo":"55DF2BA1.2020107@alum.mit.edu","subject":"Re: [PATCH v13 05/12] ref-filter: add option to filter out tags, branches and remotes","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-27T15:35:08Z","receivedAt":"2015-08-27T15:35:08Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Thu, Aug 27, 2015 at 8:54 PM, Michael Haggerty wrote:\n> On 08/27/2015 02:42 PM, Karthik Nayak wrote:\n>> On Wed, Aug 26, 2015 at 9:40 PM, Michael Haggerty wrote:\n>>> On 08/22/2015 05:39 AM, Karthik Nayak wrote:\n>>>> [...]\n>>>> + if (type == FILTER_REFS_BRANCHES)\n>>>> + ret = for_each_reftype_fullpath(ref_filter_handler, \"refs/heads/\", broken, &ref_cbdata);\n>>>> + else if (type == FILTER_REFS_REMOTES)\n>>>> + ret = for_each_reftype_fullpath(ref_filter_handler, \"refs/remotes/\", broken, &ref_cbdata);\n>>>> + else if (type == FILTER_REFS_TAGS)\n>>>> + ret = for_each_reftype_fullpath(ref_filter_handler, \"refs/tags/\", broken, &ref_cbdata);\n>>>> + else if (type & FILTER_REFS_ALL) {\n>>>> + ret = for_each_reftype_fullpath(ref_filter_handler, \"\", broken, &ref_cbdata);\n>>>> + if (type & FILTER_REFS_DETACHED_HEAD)\n>>>> + head_ref(ref_filter_handler, &ref_cbdata);\n>>>\n>>> The usual promise of the for_each_ref functions is that they stop\n>>> iterating if the function ever returns a nonzero value. So the above\n>>> should be\n>>>\n>>> if (! ret && (type & FILTER_REFS_DETACHED_HEAD))\n>>> ret = head_ref(ref_filter_handler, &ref_cbdata);\n>>>\n>>> Also, these functions usually iterate in lexicographic order, so I think\n>>> you should process HEAD before the others.\n>>\n>> This is done on purpose, cause we need to print the HEAD ref separately\n>> so we print the last ref_array_item in the ref_array, free that memory and\n>> sort and print the rest, hence HEAD ref is attached to the last.\n>\n> Without having looked at the other patches, this makes me wonder whether\n> it makes sense to store HEAD in the ref_array at all or whether it\n> should be handled separately.\n>\n\nWell then we'd need another ref_array just for that, that also could\nbe an option.\nBut apart from printing it first, everything else is the same for all the refs.\n\n>>> But there's another problem here. It seems like\n>>> FILTER_REFS_DETACHED_HEAD is only processed if (type & FILTER_REFS_ALL)\n>>> is nonzero. But shouldn't it be allowed to process *only* HEAD?\n>>>\n>>> So, finally, I think this code should look like\n>>>\n>>> else if (!filter->kind)\n>>> die(\"filter_refs: invalid type\");\n>>> else {\n>>> if (filter->kind & FILTER_REFS_DETACHED_HEAD)\n>>> ret = head_ref(ref_filter_handler, &ref_cbdata);\n>>> if (! ret && (filter->kind & FILTER_REFS_ALL))\n>>> ret =\n>>> for_each_reftype_fullpath(ref_filter_handler, \"\", broken, &ref_cbdata);\n>>> }\n>>>\n>>\n>> So finally something like this perhaps\n>>\n>> if (!filter->kind)\n>> die(\"filter_refs: invalid type\");\n>> else {\n>> if (filter->kind == FILTER_REFS_BRANCHES)\n>> ret = for_each_reftype_fullpath(ref_filter_handler,\n>> \"refs/heads/\", broken, &ref_cbdata);\n>> else if (filter->kind == FILTER_REFS_REMOTES)\n>> ret = for_each_reftype_fullpath(ref_filter_handler,\n>> \"refs/remotes/\", broken, &ref_cbdata);\n>> else if (filter->kind == FILTER_REFS_TAGS)\n>> ret = for_each_reftype_fullpath(ref_filter_handler,\n>> \"refs/tags/\", broken, &ref_cbdata);\n>> else if (filter->kind & FILTER_REFS_ALL)\n>> ret = for_each_reftype_fullpath(ref_filter_handler, \"\",\n>> broken, &ref_cbdata);\n>> if (filter->kind & FILTER_REFS_DETACHED_HEAD)\n>> head_ref(ref_filter_handler, &ref_cbdata);\n>> }\n>\n> Yes, but the last test should be\n>\n> if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))\n>\n> unless you have a reason not to follow the usual convention that a\n> nonzero return value from fn means that the iteration should be aborted.\n>\n\nNo, of course, I missed that while typing here.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268803","messageId":"CAOLa=ZT7VpMaMvxaMeSLJOr3BYv0pcY7dDPR-g2LvDVXCOs1YQ@mail.gmail.com","threadId":"40155","inReplyTo":"xmqq61413iqy.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-27T18:00:41Z","receivedAt":"2015-08-27T18:00:41Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Thu, Aug 27, 2015 at 1:49 AM, Junio C Hamano wrote:\n> Karthik Nayak writes:\n>\n>> On Wed, Aug 26, 2015 at 8:07 PM, Junio C Hamano wrote:\n>>>\n>>> ... You can give a new format_ref_array_item()\n>>> that does not print but fills a strbuf to this caller, make\n>>> show_ref_array_item() a thin wrapper that calls it and prints it\n>>> with the final LF for other callers.\n>>>\n>> You're saying remove show_ref_array_item() (even the wrapper you mentioned\n>> above) and just have something like format_ref_array_item() which\n>> would output to a strbuf. and let the caller worry about the printing?\n>\n> Among the current callers, the one in builtin/tag.c that wants to\n> trigger show_tag_lines() hack embedded in show_ref_array_item()\n> function can stop calling show_ref_array_item() and instead can do\n>\n> for (i = 0; i < array.nr; i++) {\n> struct strbuf out = STRBUF_INIT;\n> format_ref_array_item(&out, ...);\n> if (filter->lines) {\n> ... append tag lines to out ...\n> }\n> printf(\"%s\\n\", out.buf);\n> strbuf_reset(&out);\n> }\n>\n> The current and future callers of show_ref_array_item() that do not\n> want to trigger the show_tag_liens() hack embedded in there may\n> still want it to print the formatted string including the trailing\n> LF, so you can keep show_ref_array_item() as a thin wrapper around\n> format_ref_array_item() for them to call, e.g.\n>\n> show_ref_array_item(...) {\n> struct strbuf out = STRBUF_INIT;\n> format_ref_array_item(&out, ...);\n> printf(\"%s\\n\", out.buf);\n> strbuf_release(&out);\n> }\n>\n> But if it has only one caller each, you may not even want to have\n> show_ref_array_item(), if you are going to do the \"output to strbuf\"\n> variant.\n>\n>\n>\n\nThis is exactly what I did at the moment, I'm also trying to get\n%(contents:lines=N)\nwork. Thanks for explaining though.\n\n-- \nRegards,\nKarthik Nayak\n"}]}