{"thread":{"id":"40216","subject":"[PATCH v14 02/13] ref-filter: introduce ref_formatting_state and ref_formatting_stack","startedAt":"2015-08-29T14:12:04Z","lastAt":"2015-09-01T17:37:45Z","messageCount":52,"participants":["Karthik Nayak","Torsten Bögershausen","Eric Sunshine","Junio C Hamano","Matthieu Moy"],"isPatch":true,"patchVersion":14,"patchTotal":13},"messages":[{"id":"268894","messageId":"1440857537-13968-1-git-send-email-Karthik.188@gmail.com","threadId":"40216","inReplyTo":null,"subject":"[PATCH v14 00/13] Port tag.c to use ref-filter.c","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-29T14:12:04Z","receivedAt":"2015-08-29T14:12:04Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"This is part of porting tag.c to ref-filter APIs.\nVersion 13 can be found:\nthread.gmane.org/gmane.comp.version-control.git/276363\n\nChanges in this version:\n* Introduce format_ref_array_item() and make show_ref_array_item() a\nwrapper around the same.\n* Introduce %(contents:lines=X) which gets the first X lines from a\ngiven object.\n* Change code in 05/13 to make the code neater and consistent.\n* %(align) without arguments fails now.\n* Add test for nested alignment.\n* We perform quoting on each layer of nested alignment. \n\nKarthik Nayak (13):\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: introduce format_ref_array_item()\n ref-filter: add support for %(contents:lines=X)\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 | 13 ++\n Documentation/git-tag.txt | 27 ++-\n builtin/for-each-ref.c | 1 +\n builtin/tag.c | 368 ++++++--------------------------\n ref-filter.c | 418 ++++++++++++++++++++++++++++++++-----\n ref-filter.h | 32 ++-\n refs.c | 9 +\n refs.h | 1 +\n t/t6302-for-each-ref-filter.sh | 137 ++++++++++++\n t/t7004-tag.sh | 47 ++++-\n utf8.c | 21 ++\n utf8.h | 15 ++\n 12 files changed, 712 insertions(+), 377 deletions(-)\n\nInterdiff between version 13 and version 14\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 06d468e..1b48b95 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -149,6 +149,7 @@ Its first line is `contents:subject`, where subject is the concatenation\n of all lines of the commit message up to the first blank line. The next\n line is 'contents:body', where body is all of the lines after the first\n blank line. Finally, the optional GPG signature is `contents:signature`.\n+The first `N` lines of the object is obtained using `contents:lines=N`.\n \n For sorting purposes, fields with numeric values sort in numeric\n order (`objectsize`, `authordate`, `committerdate`, `taggerdate`).\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 3ad6a64..4e9f6c2 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -75,7 +75,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, 0);\n+\t\tshow_ref_array_item(array.items[i], format, quote_style);\n \tref_array_clear(&array);\n \treturn 0;\n }\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex bbbcaed..9fa1400 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -33,6 +33,7 @@ static unsigned int colopts;\n static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)\n {\n \tstruct ref_array array;\n+\tchar *to_free = NULL;\n \tint i;\n \n \tmemset(&array, 0, sizeof(array));\n@@ -42,7 +43,8 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con\n \n \tif (!format) {\n \t\tif (filter->lines)\n-\t\t\tformat = \"%(align:16,left)%(refname:short)%(end)\";\n+\t\t\tformat = to_free = xstrfmt(\"%%(align:15,left)%%(refname:short)%%(end) %%(contents:lines=%d)\",\n+\t\t\t\t\t\t filter->lines);\n \t\telse\n \t\t\tformat = \"%(refname:short)\";\n \t}\n@@ -52,8 +54,9 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con\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+\t\tshow_ref_array_item(array.items[i], format, 0);\n \tref_array_clear(&array);\n+\tfree(to_free);\n \n \treturn 0;\n }\ndiff --git a/ref-filter.c b/ref-filter.c\nindex f8b8fb7..f268cd7 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -58,6 +58,7 @@ static struct {\n \t{ \"color\" },\n \t{ \"align\" },\n \t{ \"end\" },\n+\t{ \"contents:lines\" },\n };\n \n struct align {\n@@ -65,6 +66,11 @@ struct align {\n \tunsigned int width;\n };\n \n+struct contents {\n+\tunsigned int lines;\n+\tstruct object_id oid;\n+};\n+\n #define REF_FORMATTING_STATE_INIT { 0, NULL }\n \n struct ref_formatting_stack {\n@@ -82,6 +88,7 @@ struct ref_formatting_state {\n struct atom_value {\n \tconst char *s;\n \tstruct align *align;\n+\tstruct contents *contents;\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@@ -155,7 +162,28 @@ 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+static void quote_formatting(struct strbuf *s, const char *str, int quote_style)\n+{\n+\tswitch (quote_style) {\n+\tcase QUOTE_NONE:\n+\t\tstrbuf_addstr(s, str);\n+\t\tbreak;\n+\tcase QUOTE_SHELL:\n+\t\tsq_quote_buf(s, str);\n+\t\tbreak;\n+\tcase QUOTE_PERL:\n+\t\tperl_quote_buf(s, str);\n+\t\tbreak;\n+\tcase QUOTE_PYTHON:\n+\t\tpython_quote_buf(s, str);\n+\t\tbreak;\n+\tcase QUOTE_TCL:\n+\t\ttcl_quote_buf(s, str);\n+\t\tbreak;\n+\t}\n+}\n+\n+static void push_stack_element(struct ref_formatting_stack **stack)\n {\n \tstruct ref_formatting_stack *s = xcalloc(1, sizeof(struct ref_formatting_stack));\n \n@@ -550,6 +578,61 @@ static void find_subpos(const char *buf, unsigned long sz,\n \t*nonsiglen = *sig - buf;\n }\n \n+/*\n+ * If 'lines' is greater than 0, append that many lines from the given\n+ * object_id 'oid' to the given strbuf.\n+ */\n+static void append_tag_lines(struct strbuf *out, 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\tstrbuf_addstr(out, \"\\n \");\n+\t\teol = memchr(sp, '\\n', size - (sp - buf));\n+\t\tlen = eol ? eol - sp : size - (sp - buf);\n+\t\tstrbuf_add(out, sp, len);\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 contents_lines_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n+{\n+\tstruct contents *contents = (struct contents *)atomv->contents;\n+\tstruct strbuf s = STRBUF_INIT;\n+\n+\tappend_tag_lines(&s, &contents->oid, contents->lines);\n+\tquote_formatting(&state->stack->output, s.buf, state->quote_style);\n+\tstrbuf_release(&s);\n+\n+\tfree(contents);\n+}\n+\n /* See grab_values */\n static void grab_sub_body_contents(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)\n {\n@@ -560,6 +643,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj\n \tfor (i = 0; i < used_atom_cnt; i++) {\n \t\tconst char *name = used_atom[i];\n \t\tstruct atom_value *v = &val[i];\n+\t\tconst char *valp = NULL;\n \t\tif (!!deref != (*name == '*'))\n \t\t\tcontinue;\n \t\tif (deref)\n@@ -569,7 +653,8 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj\n \t\t strcmp(name, \"contents\") &&\n \t\t strcmp(name, \"contents:subject\") &&\n \t\t strcmp(name, \"contents:body\") &&\n-\t\t strcmp(name, \"contents:signature\"))\n+\t\t strcmp(name, \"contents:signature\") &&\n+\t\t !starts_with(name, \"contents:lines=\"))\n \t\t\tcontinue;\n \t\tif (!subpos)\n \t\t\tfind_subpos(buf, sz,\n@@ -589,6 +674,15 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj\n \t\t\tv->s = xmemdupz(sigpos, siglen);\n \t\telse if (!strcmp(name, \"contents\"))\n \t\t\tv->s = xstrdup(subpos);\n+\t\telse if (skip_prefix(name, \"contents:lines=\", &valp)) {\n+\t\t\tstruct contents *contents = xmalloc(sizeof(struct contents));\n+\n+\t\t\tif (strtoul_ui(valp, 10, &contents->lines))\n+\t\t\t\tdie(_(\"positive width expected align:%s\"), valp);\n+\t\t\thashcpy(contents->oid.hash, obj->sha1);\n+\t\t\tv->handler = contents_lines_handler;\n+\t\t\tv->contents = contents;\n+\t\t}\n \t}\n }\n \n@@ -661,13 +755,25 @@ static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_s\n {\n \tstruct ref_formatting_stack *new;\n \n-\tpush_new_stack_element(&state->stack);\n+\tpush_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+static void append_atom(struct atom_value *v, struct ref_formatting_state *state)\n+{\n+\t/*\n+\t * Quote formatting is only done when the stack has a single\n+\t * element. Otherwise quote formatting is done on the\n+\t * element's entire output strbuf when the %(end) atom is\n+\t * encountered.\n+\t */\n+\tif (!state->stack->prev)\n+\t\tquote_formatting(&state->stack->output, v->s, state->quote_style);\n+\telse\n+\t\tstrbuf_addstr(&state->stack->output, v->s);\n+}\n \n static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n {\n@@ -682,8 +788,8 @@ static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_sta\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+\tif (state->stack->prev) {\n+\t\tquote_formatting(&s, current->output.buf, state->quote_style);\n \t\tstrbuf_reset(¤t->output);\n \t\tstrbuf_addbuf(¤t->output, &s);\n \t}\n@@ -722,6 +828,8 @@ static void populate_value(struct ref_array_item *ref)\n \t\tconst char *valp;\n \t\tstruct branch *branch = NULL;\n \n+\t\tv->handler = append_atom;\n+\n \t\tif (*name == '*') {\n \t\t\tderef = 1;\n \t\t\tname++;\n@@ -785,7 +893,9 @@ 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} else if (!strcmp(name, \"align\"))\n+\t\t\tdie(_(\"format: incomplete use of the `align` atom\"));\n+\t\telse 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@@ -1172,12 +1282,10 @@ static int filter_ref_kind(struct ref_filter *filter, const char *refname)\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+\tif (filter->kind == FILTER_REFS_BRANCHES ||\n+\t filter->kind == FILTER_REFS_REMOTES ||\n+\t filter->kind == FILTER_REFS_TAGS)\n+\t\treturn filter->kind;\n \telse if (!strcmp(refname, \"HEAD\"))\n \t\treturn FILTER_REFS_DETACHED_HEAD;\n \n@@ -1211,6 +1319,11 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n \t\treturn 0;\n \t}\n \n+\t/*\n+\t * Get the current ref kind. If we're filtering tags, remotes or local branches\n+\t * only then the current ref-kind is nothing but filter->kind and filter_ref_kind()\n+\t * will only return that value.\n+\t */\n \tkind = filter_ref_kind(filter, refname);\n \tif (!(kind & filter->kind))\n \t\treturn 0;\n@@ -1328,25 +1441,26 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int\n \tref_cbdata.array = array;\n \tref_cbdata.filter = filter;\n \n-\t/* Simple per-ref filtering */\n-\tif (type & FILTER_REFS_INCLUDE_BROKEN) {\n-\t\ttype &= ~FILTER_REFS_INCLUDE_BROKEN;\n+\tif (type & FILTER_REFS_INCLUDE_BROKEN)\n \t\tbroken = 1;\n-\t}\n+\tfilter->kind = type & FILTER_REFS_KIND_MASK;\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/* Simple per-ref filtering */\n+\tif (!filter->kind)\n \t\tdie(\"filter_refs: invalid type\");\n+\telse {\n+\t\tif (filter->kind == FILTER_REFS_BRANCHES)\n+\t\t\tret = for_each_fullref_in(\"refs/heads/\", ref_filter_handler, &ref_cbdata, broken);\n+\t\telse if (filter->kind == FILTER_REFS_REMOTES)\n+\t\t\tret = for_each_fullref_in(\"refs/remotes/\", ref_filter_handler, &ref_cbdata, broken);\n+\t\telse if (filter->kind == FILTER_REFS_TAGS)\n+\t\t\tret = for_each_fullref_in(\"refs/tags/\", ref_filter_handler, &ref_cbdata, broken);\n+\t\telse if (filter->kind & FILTER_REFS_ALL)\n+\t\t\tret = for_each_fullref_in(\"\", ref_filter_handler, &ref_cbdata, broken);\n+\t\tif (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))\n+\t\t\thead_ref(ref_filter_handler, &ref_cbdata);\n+\t}\n+\n \n \t/* Filters that need revision walking */\n \tif (filter->merge_commit)\n@@ -1400,33 +1514,6 @@ 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 perform_quote_formatting(struct strbuf *s, const char *str, int quote_style)\n-{\n-\tswitch (quote_style) {\n-\tcase QUOTE_NONE:\n-\t\tstrbuf_addstr(s, str);\n-\t\tbreak;\n-\tcase QUOTE_SHELL:\n-\t\tsq_quote_buf(s, str);\n-\t\tbreak;\n-\tcase QUOTE_PERL:\n-\t\tperl_quote_buf(s, str);\n-\t\tbreak;\n-\tcase QUOTE_PYTHON:\n-\t\tpython_quote_buf(s, str);\n-\t\tbreak;\n-\tcase QUOTE_TCL:\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@@ -1467,58 +1554,14 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting\n \t}\n }\n \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+void format_ref_array_item(struct strbuf *out, struct ref_array_item *info,\n+\t\t\t 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+\tpush_stack_element(&state.stack);\n \n \tfor (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {\n \t\tstruct atom_value *atomv;\n@@ -1527,18 +1570,7 @@ void show_ref_array_item(struct ref_array_item *info, const char *format,\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\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\tatomv->handler(atomv, &state);\n \t}\n \tif (*cp) {\n \t\tsp = cp + strlen(cp);\n@@ -1555,15 +1587,17 @@ void show_ref_array_item(struct ref_array_item *info, const char *format,\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+\tstrbuf_addbuf(out, &state.stack->output);\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+\n+void show_ref_array_item(struct ref_array_item *item, const char *format, unsigned int quote_style)\n+{\n+\tstruct strbuf out = STRBUF_INIT;\n+\tformat_ref_array_item(&out, item, format, quote_style);\n+\tfwrite(out.buf, out.len, 1, stdout);\n+\tprintf(\"\\n\");\n+\tstrbuf_release(&out);\n }\n \n /* If no sorting option is given, use refname to sort as default */\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 8241066..179944c 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -21,6 +21,7 @@\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+#define FILTER_REFS_KIND_MASK (FILTER_REFS_ALL | FILTER_REFS_DETACHED_HEAD)\n \n struct atom_value;\n \n@@ -93,12 +94,11 @@ int parse_ref_filter_atom(const char *atom, const char *ep);\n int verify_ref_format(const char *format);\n /* Sort the given ref_array as per the ref_sorting provided */\n void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);\n-/*\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+/* Format the ref as per given format and quote_style and store it into the strbuf */\n+void format_ref_array_item(struct strbuf *out, struct ref_array_item *info,\n+\t\t\t const char *format, int quote_style);\n+/* Wrapper around format_ref_array_item() which prints the given ref_array_item */\n+void show_ref_array_item(struct ref_array_item *item, const char *format, unsigned int quote_style);\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 */\ndiff --git a/refs.c b/refs.c\nindex 3266617..a9469c2 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2108,6 +2108,15 @@ int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data)\n \treturn do_for_each_ref(&ref_cache, prefix, fn, strlen(prefix), 0, cb_data);\n }\n \n+int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, unsigned int broken)\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, prefix, fn, 0, flag, cb_data);\n+}\n+\n int for_each_ref_in_submodule(const char *submodule, const char *prefix,\n \t\teach_ref_fn fn, void *cb_data)\n {\n@@ -2150,15 +2159,6 @@ 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 6e913ee..6d30c98 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -173,13 +173,13 @@ typedef int each_ref_fn(const char *refname,\n extern int head_ref(each_ref_fn fn, void *cb_data);\n extern int for_each_ref(each_ref_fn fn, void *cb_data);\n extern int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data);\n+extern int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, unsigned int broken);\n extern int for_each_tag_ref(each_ref_fn fn, void *cb_data);\n extern int for_each_branch_ref(each_ref_fn fn, void *cb_data);\n extern int for_each_remote_ref(each_ref_fn fn, void *cb_data);\n extern int for_each_replace_ref(each_ref_fn fn, void *cb_data);\n 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);\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 38c99c9..8f18f86 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -150,6 +150,38 @@ test_expect_success 'alignment with format quote' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'nested alignment' '\n+\tcat >expect <<-\\EOF &&\n+\t| master |\n+\t| side |\n+\t| odd/spot |\n+\t| double-tag |\n+\t| four |\n+\t| one |\n+\t| signed-tag |\n+\t| three |\n+\t| two |\n+\tEOF\n+\tgit for-each-ref --format=\"|%(align:30,left)%(align:15,right)%(refname:short)%(end)%(end)|\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check `%(contents:lines=X)`' '\n+\tcat >expect <<-\\EOF &&\n+\tmaster three\n+\tside four\n+\todd/spot three\n+\tdouble-tag Annonated doubly\n+\tfour four\n+\tone one\n+\tsigned-tag A signed tag message\n+\tthree three\n+\ttwo two\n+\tEOF\n+\tgit for-each-ref --format=\"%(refname:short) %(contents:lines=1)\" >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 &&\n\n\n-- \n2.5.0\n"},{"id":"268892","messageId":"1440857537-13968-2-git-send-email-Karthik.188@gmail.com","threadId":"40216","inReplyTo":"1440857537-13968-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v14 01/13] ref-filter: move `struct atom_value` to ref-filter.c","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-29T14:12:05Z","receivedAt":"2015-08-29T14:12:05Z","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":"268891","messageId":"1440857537-13968-3-git-send-email-Karthik.188@gmail.com","threadId":"40216","inReplyTo":"1440857537-13968-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v14 02/13] ref-filter: introduce ref_formatting_state and ref_formatting_stack","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-29T14:12:06Z","receivedAt":"2015-08-29T14:12:06Z","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..432cea0 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_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_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":"268893","messageId":"1440857537-13968-4-git-send-email-Karthik.188@gmail.com","threadId":"40216","inReplyTo":"1440857537-13968-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v14 03/13] utf8: add function to align a string into given strbuf","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-29T14:12:07Z","receivedAt":"2015-08-29T14:12:07Z","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":"268896","messageId":"1440857537-13968-5-git-send-email-Karthik.188@gmail.com","threadId":"40216","inReplyTo":"1440857537-13968-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v14 04/13] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-29T14:12:08Z","receivedAt":"2015-08-29T14:12:08Z","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 | 151 +++++++++++++++++++++++++++++++------\n t/t6302-for-each-ref-filter.sh | 85 +++++++++++++++++++++\n 3 files changed, 221 insertions(+), 24 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 432cea0..21c8b5f 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,84 @@ 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_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 quote_formatting(struct strbuf *s, const char *str, int quote_style)\n+{\n+\tswitch (quote_style) {\n+\tcase QUOTE_NONE:\n+\t\tstrbuf_addstr(s, str);\n+\t\tbreak;\n+\tcase QUOTE_SHELL:\n+\t\tsq_quote_buf(s, str);\n+\t\tbreak;\n+\tcase QUOTE_PERL:\n+\t\tperl_quote_buf(s, str);\n+\t\tbreak;\n+\tcase QUOTE_PYTHON:\n+\t\tpython_quote_buf(s, str);\n+\t\tbreak;\n+\tcase QUOTE_TCL:\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+\t/*\n+\t * Quote formatting is only done when the stack has a single\n+\t * element. Otherwise quote formatting is done on the\n+\t * element's entire output strbuf when the %(end) atom is\n+\t * encountered.\n+\t */\n+\tif (!state->stack->prev)\n+\t\tquote_formatting(&state->stack->output, v->s, state->quote_style);\n+\telse\n+\t\tstrbuf_addstr(&state->stack->output, v->s);\n+}\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) {\n+\t\tquote_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,8 +750,11 @@ 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\tv->handler = append_atom;\n+\n \t\tif (*name == '*') {\n \t\t\tderef = 1;\n \t\t\tname++;\n@@ -725,6 +818,37 @@ 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 (!strcmp(name, \"align\"))\n+\t\t\tdie(_(\"format: incomplete use of the `align` atom\"));\n+\t\telse 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 +1352,6 @@ 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-{\n-\tstruct strbuf *s = &state->stack->output;\n-\n-\tswitch (state->quote_style) {\n-\tcase QUOTE_NONE:\n-\t\tstrbuf_addstr(s, v->s);\n-\t\tbreak;\n-\tcase QUOTE_SHELL:\n-\t\tsq_quote_buf(s, v->s);\n-\t\tbreak;\n-\tcase QUOTE_PERL:\n-\t\tperl_quote_buf(s, v->s);\n-\t\tbreak;\n-\tcase QUOTE_PYTHON:\n-\t\tpython_quote_buf(s, v->s);\n-\t\tbreak;\n-\tcase QUOTE_TCL:\n-\t\ttcl_quote_buf(s, v->s);\n-\t\tbreak;\n-\t}\n-}\n-\n static int hex1(char ch)\n {\n \tif ('0' <= ch && ch <= '9')\n@@ -1307,7 +1408,7 @@ 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\tatomv->handler(atomv, &state);\n \t}\n \tif (*cp) {\n \t\tsp = cp + strlen(cp);\n@@ -1322,6 +1423,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..cef7a41 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -81,4 +81,89 @@ 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_expect_success 'nested alignment' '\n+\tcat >expect <<-\\EOF &&\n+\t| master |\n+\t| side |\n+\t| odd/spot |\n+\t| double-tag |\n+\t| four |\n+\t| one |\n+\t| signed-tag |\n+\t| three |\n+\t| two |\n+\tEOF\n+\tgit for-each-ref --format=\"|%(align:30,left)%(align:15,right)%(refname:short)%(end)%(end)|\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.5.0\n"},{"id":"268895","messageId":"1440857537-13968-6-git-send-email-Karthik.188@gmail.com","threadId":"40216","inReplyTo":"1440857537-13968-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v14 05/13] ref-filter: add option to filter out tags, branches and remotes","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-29T14:12:09Z","receivedAt":"2015-08-29T14:12:09Z","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_fullref_in()' to refs.{c,h} which\niterates through each ref for the given path without trimming the path\nand 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 | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-----\n ref-filter.h | 13 +++++++++++--\n refs.c | 9 +++++++++\n refs.h | 1 +\n 4 files changed, 79 insertions(+), 7 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 21c8b5f..5d4f93d 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1160,6 +1160,34 @@ 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 filter->kind == FILTER_REFS_REMOTES ||\n+\t filter->kind == FILTER_REFS_TAGS)\n+\t\treturn filter->kind;\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@@ -1170,6 +1198,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@@ -1181,6 +1210,15 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n \t\treturn 0;\n \t}\n \n+\t/*\n+\t * Get the current ref kind. If we're filtering tags, remotes or local branches\n+\t * only then the current ref-kind is nothing but filter->kind and filter_ref_kind()\n+\t * will only return that value.\n+\t */\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@@ -1212,6 +1250,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@@ -1288,17 +1327,31 @@ 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+\tif (type & FILTER_REFS_INCLUDE_BROKEN)\n+\t\tbroken = 1;\n+\tfilter->kind = type & FILTER_REFS_KIND_MASK;\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 (!filter->kind)\n \t\tdie(\"filter_refs: invalid type\");\n+\telse {\n+\t\tif (filter->kind == FILTER_REFS_BRANCHES)\n+\t\t\tret = for_each_fullref_in(\"refs/heads/\", ref_filter_handler, &ref_cbdata, broken);\n+\t\telse if (filter->kind == FILTER_REFS_REMOTES)\n+\t\t\tret = for_each_fullref_in(\"refs/remotes/\", ref_filter_handler, &ref_cbdata, broken);\n+\t\telse if (filter->kind == FILTER_REFS_TAGS)\n+\t\t\tret = for_each_fullref_in(\"refs/tags/\", ref_filter_handler, &ref_cbdata, broken);\n+\t\telse if (filter->kind & FILTER_REFS_ALL)\n+\t\t\tret = for_each_fullref_in(\"\", ref_filter_handler, &ref_cbdata, broken);\n+\t\tif (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))\n+\t\t\thead_ref(ref_filter_handler, &ref_cbdata);\n+\t}\n+\n \n \t/* Filters that need revision walking */\n \tif (filter->merge_commit)\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 45026d0..0913ba9 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -13,8 +13,15 @@\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+#define FILTER_REFS_KIND_MASK (FILTER_REFS_ALL | FILTER_REFS_DETACHED_HEAD)\n \n struct atom_value;\n \n@@ -27,6 +34,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 +59,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..a9469c2 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2108,6 +2108,15 @@ int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data)\n \treturn do_for_each_ref(&ref_cache, prefix, fn, strlen(prefix), 0, cb_data);\n }\n \n+int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, unsigned int broken)\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, prefix, fn, 0, flag, cb_data);\n+}\n+\n int for_each_ref_in_submodule(const char *submodule, const char *prefix,\n \t\teach_ref_fn fn, void *cb_data)\n {\ndiff --git a/refs.h b/refs.h\nindex e9a5f32..6d30c98 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -173,6 +173,7 @@ typedef int each_ref_fn(const char *refname,\n extern int head_ref(each_ref_fn fn, void *cb_data);\n extern int for_each_ref(each_ref_fn fn, void *cb_data);\n extern int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data);\n+extern int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, unsigned int broken);\n extern int for_each_tag_ref(each_ref_fn fn, void *cb_data);\n extern int for_each_branch_ref(each_ref_fn fn, void *cb_data);\n extern int for_each_remote_ref(each_ref_fn fn, void *cb_data);\n-- \n2.5.0\n"},{"id":"268898","messageId":"1440857537-13968-7-git-send-email-Karthik.188@gmail.com","threadId":"40216","inReplyTo":"1440857537-13968-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v14 06/13] ref-filter: introduce format_ref_array_item()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-29T14:12:10Z","receivedAt":"2015-08-29T14:12:10Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Create format_ref_array_item() out of show_ref_array_item(). This will\nstore the output format for the given ref_array_item into the provided\nstrbuf. Make show_ref_array_item() a wrapper around this to print the\ngiven ref_array_item with linefeed.\n\nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 59 +++++++++++++++++++++++++++++++++--------------------------\n ref-filter.h | 7 +++++--\n 2 files changed, 38 insertions(+), 28 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 5d4f93d..1e6754a 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -153,6 +153,27 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n \treturn at;\n }\n \n+static void quote_formatting(struct strbuf *s, const char *str, int quote_style)\n+{\n+\tswitch (quote_style) {\n+\tcase QUOTE_NONE:\n+\t\tstrbuf_addstr(s, str);\n+\t\tbreak;\n+\tcase QUOTE_SHELL:\n+\t\tsq_quote_buf(s, str);\n+\t\tbreak;\n+\tcase QUOTE_PERL:\n+\t\tperl_quote_buf(s, str);\n+\t\tbreak;\n+\tcase QUOTE_PYTHON:\n+\t\tpython_quote_buf(s, str);\n+\t\tbreak;\n+\tcase QUOTE_TCL:\n+\t\ttcl_quote_buf(s, str);\n+\t\tbreak;\n+\t}\n+}\n+\n static void push_stack_element(struct ref_formatting_stack **stack)\n {\n \tstruct ref_formatting_stack *s = xcalloc(1, sizeof(struct ref_formatting_stack));\n@@ -665,27 +686,6 @@ static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_s\n \tnew->cb_data = atomv->align;\n }\n \n-static void quote_formatting(struct strbuf *s, const char *str, int quote_style)\n-{\n-\tswitch (quote_style) {\n-\tcase QUOTE_NONE:\n-\t\tstrbuf_addstr(s, str);\n-\t\tbreak;\n-\tcase QUOTE_SHELL:\n-\t\tsq_quote_buf(s, str);\n-\t\tbreak;\n-\tcase QUOTE_PERL:\n-\t\tperl_quote_buf(s, str);\n-\t\tbreak;\n-\tcase QUOTE_PYTHON:\n-\t\tpython_quote_buf(s, str);\n-\t\tbreak;\n-\tcase QUOTE_TCL:\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 \t/*\n@@ -1445,10 +1445,10 @@ 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+void format_ref_array_item(struct strbuf *out, struct ref_array_item *info,\n+\t\t\t 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@@ -1478,10 +1478,17 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\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+\tstrbuf_addbuf(out, &state.stack->output);\n \tpop_stack_element(&state.stack);\n-\tputchar('\\n');\n+}\n+\n+void show_ref_array_item(struct ref_array_item *item, const char *format, unsigned int quote_style)\n+{\n+\tstruct strbuf out = STRBUF_INIT;\n+\tformat_ref_array_item(&out, item, format, quote_style);\n+\tfwrite(out.buf, out.len, 1, stdout);\n+\tprintf(\"\\n\");\n+\tstrbuf_release(&out);\n }\n \n /* If no sorting option is given, use refname to sort as default */\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 0913ba9..082bee5 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -91,8 +91,11 @@ int parse_ref_filter_atom(const char *atom, const char *ep);\n int verify_ref_format(const char *format);\n /* Sort the given ref_array as per the ref_sorting provided */\n void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);\n-/* Print the ref using the given format and quote_style */\n-void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style);\n+/* Format the ref as per given format and quote_style and store it into the strbuf */\n+void format_ref_array_item(struct strbuf *out, struct ref_array_item *info,\n+\t\t\t const char *format, int quote_style);\n+/* Wrapper around format_ref_array_item() which prints the given ref_array_item */\n+void show_ref_array_item(struct ref_array_item *item, const char *format, unsigned int quote_style);\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":"268897","messageId":"1440857537-13968-8-git-send-email-Karthik.188@gmail.com","threadId":"40216","inReplyTo":"1440857537-13968-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v14 07/13] ref-filter: add support for %(contents:lines=X)","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-29T14:12:11Z","receivedAt":"2015-08-29T14:12:11Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"In '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 it to support appending of N lines from the annotation of tags\nto the given strbuf.\n\nImplement %(contents:lines=X) where X lines of the given object are\nobtained.\n\nAdd documentation and test 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 | 1 +\n builtin/tag.c | 4 ++\n ref-filter.c | 75 +++++++++++++++++++++++++++++++++++++-\n ref-filter.h | 3 +-\n t/t6302-for-each-ref-filter.sh | 16 ++++++++\n 5 files changed, 97 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 943975d..7ca1fe4 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -149,6 +149,7 @@ Its first line is `contents:subject`, where subject is the concatenation\n of all lines of the commit message up to the first blank line. The next\n line is 'contents:body', where body is all of the lines after the first\n blank line. Finally, the optional GPG signature is `contents:signature`.\n+The first `N` lines of the object is obtained using `contents:lines=N`.\n \n For sorting purposes, fields with numeric values sort in numeric\n order (`objectsize`, `authordate`, `committerdate`, `taggerdate`).\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 1e6754a..ece7a23 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -56,6 +56,7 @@ static struct {\n \t{ \"color\" },\n \t{ \"align\" },\n \t{ \"end\" },\n+\t{ \"contents:lines\" },\n };\n \n struct align {\n@@ -63,6 +64,11 @@ struct align {\n \tunsigned int width;\n };\n \n+struct contents {\n+\tunsigned int lines;\n+\tstruct object_id oid;\n+};\n+\n #define REF_FORMATTING_STATE_INIT { 0, NULL }\n \n struct ref_formatting_stack {\n@@ -80,6 +86,7 @@ struct ref_formatting_state {\n struct atom_value {\n \tconst char *s;\n \tstruct align *align;\n+\tstruct contents *contents;\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@@ -569,6 +576,61 @@ static void find_subpos(const char *buf, unsigned long sz,\n \t*nonsiglen = *sig - buf;\n }\n \n+/*\n+ * If 'lines' is greater than 0, append that many lines from the given\n+ * object_id 'oid' to the given strbuf.\n+ */\n+static void append_tag_lines(struct strbuf *out, 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\tstrbuf_addstr(out, \"\\n \");\n+\t\teol = memchr(sp, '\\n', size - (sp - buf));\n+\t\tlen = eol ? eol - sp : size - (sp - buf);\n+\t\tstrbuf_add(out, sp, len);\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 contents_lines_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n+{\n+\tstruct contents *contents = (struct contents *)atomv->contents;\n+\tstruct strbuf s = STRBUF_INIT;\n+\n+\tappend_tag_lines(&s, &contents->oid, contents->lines);\n+\tquote_formatting(&state->stack->output, s.buf, state->quote_style);\n+\tstrbuf_release(&s);\n+\n+\tfree(contents);\n+}\n+\n /* See grab_values */\n static void grab_sub_body_contents(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)\n {\n@@ -579,6 +641,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj\n \tfor (i = 0; i < used_atom_cnt; i++) {\n \t\tconst char *name = used_atom[i];\n \t\tstruct atom_value *v = &val[i];\n+\t\tconst char *valp = NULL;\n \t\tif (!!deref != (*name == '*'))\n \t\t\tcontinue;\n \t\tif (deref)\n@@ -588,7 +651,8 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj\n \t\t strcmp(name, \"contents\") &&\n \t\t strcmp(name, \"contents:subject\") &&\n \t\t strcmp(name, \"contents:body\") &&\n-\t\t strcmp(name, \"contents:signature\"))\n+\t\t strcmp(name, \"contents:signature\") &&\n+\t\t !starts_with(name, \"contents:lines=\"))\n \t\t\tcontinue;\n \t\tif (!subpos)\n \t\t\tfind_subpos(buf, sz,\n@@ -608,6 +672,15 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj\n \t\t\tv->s = xmemdupz(sigpos, siglen);\n \t\telse if (!strcmp(name, \"contents\"))\n \t\t\tv->s = xstrdup(subpos);\n+\t\telse if (skip_prefix(name, \"contents:lines=\", &valp)) {\n+\t\t\tstruct contents *contents = xmalloc(sizeof(struct contents));\n+\n+\t\t\tif (strtoul_ui(valp, 10, &contents->lines))\n+\t\t\t\tdie(_(\"positive width expected align:%s\"), valp);\n+\t\t\thashcpy(contents->oid.hash, obj->sha1);\n+\t\t\tv->handler = contents_lines_handler;\n+\t\t\tv->contents = contents;\n+\t\t}\n \t}\n }\n \ndiff --git a/ref-filter.h b/ref-filter.h\nindex 082bee5..3759424 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -59,7 +59,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 {\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex cef7a41..0277498 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -166,4 +166,20 @@ test_expect_success 'nested alignment' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'check `%(contents:lines=X)`' '\n+\tcat >expect <<-\\EOF &&\n+\tmaster three\n+\tside four\n+\todd/spot three\n+\tdouble-tag Annonated doubly\n+\tfour four\n+\tone one\n+\tsigned-tag A signed tag message\n+\tthree three\n+\ttwo two\n+\tEOF\n+\tgit for-each-ref --format=\"%(refname:short) %(contents:lines=1)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.5.0\n"},{"id":"268900","messageId":"1440857537-13968-9-git-send-email-Karthik.188@gmail.com","threadId":"40216","inReplyTo":"1440857537-13968-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v14 08/13] ref-filter: add support to sort by version","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-29T14:12:12Z","receivedAt":"2015-08-29T14:12:12Z","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 7ca1fe4..1b48b95 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -155,6 +155,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 ece7a23..434f2c9 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@@ -1441,19 +1443,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@@ -1593,6 +1595,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 3759424..1282d70 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -28,7 +28,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 0277498..8f18f86 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -182,4 +182,40 @@ test_expect_success 'check `%(contents:lines=X)`' '\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":"268899","messageId":"1440857537-13968-10-git-send-email-Karthik.188@gmail.com","threadId":"40216","inReplyTo":"1440857537-13968-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v14 09/13] ref-filter: add option to match literal pattern","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-29T14:12:13Z","receivedAt":"2015-08-29T14:12:13Z","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 40f343b..4e9f6c2 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 434f2c9..f268cd7 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1165,9 +1165,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@@ -1188,6 +1212,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@@ -1294,7 +1328,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 1282d70..179944c 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -59,7 +59,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":"268901","messageId":"1440857537-13968-11-git-send-email-Karthik.188@gmail.com","threadId":"40216","inReplyTo":"1440857537-13968-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v14 10/13] tag.c: use 'ref-filter' data structures","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-29T14:12:14Z","receivedAt":"2015-08-29T14:12:14Z","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":"268902","messageId":"1440857537-13968-12-git-send-email-Karthik.188@gmail.com","threadId":"40216","inReplyTo":"1440857537-13968-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v14 11/13] tag.c: use 'ref-filter' APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-29T14:12:15Z","receivedAt":"2015-08-29T14:12:15Z","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 | 344 ++++++----------------------------------------\n t/t7004-tag.sh | 8 +-\n 3 files changed, 52 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..2d348f4 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -28,278 +28,34 @@ static const char * const git_tag_usage[] = {\n \tNULL\n };\n \n-#define STRCMP_SORT 0\t/* must be zero */\n-#define VERCMP_SORT 1\n-#define SORT_MASK 0x7fff\n-#define REVERSE_SORT 0x8000\n-\n-static int tag_sort;\n-\n static unsigned int colopts;\n \n-static int match_pattern(const char **patterns, const char *ref)\n-{\n-\t/* no pattern means match everything */\n-\tif (!*patterns)\n-\t\treturn 1;\n-\tfor (; *patterns; patterns++)\n-\t\tif (!wildmatch(*patterns, ref, 0, NULL))\n-\t\t\treturn 1;\n-\treturn 0;\n-}\n-\n-/*\n- * This is currently duplicated in ref-filter.c, and will eventually be\n- * removed as we port tag.c to use the ref-filter APIs.\n- */\n-static const unsigned char *match_points_at(const char *refname,\n-\t\t\t\t\t const unsigned char *sha1,\n-\t\t\t\t\t struct sha1_array *points_at)\n-{\n-\tconst unsigned char *tagged_sha1 = NULL;\n-\tstruct object *obj;\n-\n-\tif (sha1_array_lookup(points_at, sha1) >= 0)\n-\t\treturn sha1;\n-\tobj = parse_object(sha1);\n-\tif (!obj)\n-\t\tdie(_(\"malformed object at '%s'\"), refname);\n-\tif (obj->type == OBJ_TAG)\n-\t\ttagged_sha1 = ((struct tag *)obj)->tagged->sha1;\n-\tif (tagged_sha1 && sha1_array_lookup(points_at, tagged_sha1) >= 0)\n-\t\treturn tagged_sha1;\n-\treturn NULL;\n-}\n-\n-static int in_commit_list(const struct commit_list *want, struct commit *c)\n-{\n-\tfor (; want; want = want->next)\n-\t\tif (!hashcmp(want->item->object.sha1, c->object.sha1))\n-\t\t\treturn 1;\n-\treturn 0;\n-}\n-\n-/*\n- * The entire code segment for supporting the --contains option has been\n- * copied over to ref-filter.{c,h}. This will be deleted evetually when\n- * we port tag.c to use ref-filter APIs.\n- */\n-enum contains_result {\n-\tCONTAINS_UNKNOWN = -1,\n-\tCONTAINS_NO = 0,\n-\tCONTAINS_YES = 1\n-};\n-\n-/*\n- * Test whether the candidate or one of its parents is contained in the list.\n- * Do not recurse to find out, though, but return -1 if inconclusive.\n- */\n-static enum contains_result contains_test(struct commit *candidate,\n-\t\t\t const struct commit_list *want)\n-{\n-\t/* was it previously marked as containing a want commit? */\n-\tif (candidate->object.flags & TMP_MARK)\n-\t\treturn 1;\n-\t/* or marked as not possibly containing a want commit? */\n-\tif (candidate->object.flags & UNINTERESTING)\n-\t\treturn 0;\n-\t/* or are we it? */\n-\tif (in_commit_list(want, candidate)) {\n-\t\tcandidate->object.flags |= TMP_MARK;\n-\t\treturn 1;\n-\t}\n-\n-\tif (parse_commit(candidate) < 0)\n-\t\treturn 0;\n-\n-\treturn -1;\n-}\n-\n-/*\n- * Mimicking the real stack, this stack lives on the heap, avoiding stack\n- * overflows.\n- *\n- * At each recursion step, the stack items points to the commits whose\n- * ancestors are to be inspected.\n- */\n-struct stack {\n-\tint nr, alloc;\n-\tstruct stack_entry {\n-\t\tstruct commit *commit;\n-\t\tstruct commit_list *parents;\n-\t} *stack;\n-};\n-\n-static void push_to_stack(struct commit *candidate, struct stack *stack)\n-{\n-\tint index = stack->nr++;\n-\tALLOC_GROW(stack->stack, stack->nr, stack->alloc);\n-\tstack->stack[index].commit = candidate;\n-\tstack->stack[index].parents = candidate->parents;\n-}\n-\n-static enum contains_result contains(struct commit *candidate,\n-\t\tconst struct commit_list *want)\n-{\n-\tstruct stack stack = { 0, 0, NULL };\n-\tint result = contains_test(candidate, want);\n-\n-\tif (result != CONTAINS_UNKNOWN)\n-\t\treturn result;\n-\n-\tpush_to_stack(candidate, &stack);\n-\twhile (stack.nr) {\n-\t\tstruct stack_entry *entry = &stack.stack[stack.nr - 1];\n-\t\tstruct commit *commit = entry->commit;\n-\t\tstruct commit_list *parents = entry->parents;\n-\n-\t\tif (!parents) {\n-\t\t\tcommit->object.flags |= UNINTERESTING;\n-\t\t\tstack.nr--;\n-\t\t}\n-\t\t/*\n-\t\t * If we just popped the stack, parents->item has been marked,\n-\t\t * therefore contains_test will return a meaningful 0 or 1.\n-\t\t */\n-\t\telse switch (contains_test(parents->item, want)) {\n-\t\tcase CONTAINS_YES:\n-\t\t\tcommit->object.flags |= TMP_MARK;\n-\t\t\tstack.nr--;\n-\t\t\tbreak;\n-\t\tcase CONTAINS_NO:\n-\t\t\tentry->parents = parents->next;\n-\t\t\tbreak;\n-\t\tcase CONTAINS_UNKNOWN:\n-\t\t\tpush_to_stack(parents->item, &stack);\n-\t\t\tbreak;\n-\t\t}\n-\t}\n-\tfree(stack.stack);\n-\treturn contains_test(candidate, want);\n-}\n-\n-/*\n- * Currently duplicated in ref-filter, will eventually be removed as\n- * we port tag.c to use ref-filter APIs.\n- */\n-static void show_tag_lines(const struct object_id *oid, int lines)\n-{\n-\tint i;\n-\tunsigned long size;\n-\tenum object_type type;\n-\tchar *buf, *sp, *eol;\n-\tsize_t len;\n-\n-\tbuf = read_sha1_file(oid->hash, &type, &size);\n-\tif (!buf)\n-\t\tdie_errno(\"unable to read object %s\", oid_to_hex(oid));\n-\tif (type != OBJ_COMMIT && type != OBJ_TAG)\n-\t\tgoto free_return;\n-\tif (!size)\n-\t\tdie(\"an empty %s object %s?\",\n-\t\t typename(type), oid_to_hex(oid));\n-\n-\t/* skip header */\n-\tsp = strstr(buf, \"\\n\\n\");\n-\tif (!sp)\n-\t\tgoto free_return;\n-\n-\t/* only take up to \"lines\" lines, and strip the signature from a tag */\n-\tif (type == OBJ_TAG)\n-\t\tsize = parse_signature(buf, size);\n-\tfor (i = 0, sp += 2; i < lines && sp < buf + size; i++) {\n-\t\tif (i)\n-\t\t\tprintf(\"\\n \");\n-\t\teol = memchr(sp, '\\n', size - (sp - buf));\n-\t\tlen = eol ? eol - sp : size - (sp - buf);\n-\t\tfwrite(sp, len, 1, stdout);\n-\t\tif (!eol)\n-\t\t\tbreak;\n-\t\tsp = eol + 1;\n-\t}\n-free_return:\n-\tfree(buf);\n-}\n-\n-static void ref_array_append(struct ref_array *array, const char *refname)\n-{\n-\tsize_t len = strlen(refname);\n-\tstruct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);\n-\tmemcpy(ref->refname, refname, len);\n-\tref->refname[len] = '\\0';\n-\tREALLOC_ARRAY(array->items, array->nr + 1);\n-\tarray->items[array->nr++] = ref;\n-}\n-\n-static int show_reference(const char *refname, const struct object_id *oid,\n-\t\t\t int flag, void *cb_data)\n-{\n-\tstruct ref_filter_cbdata *data = cb_data;\n-\tstruct ref_array *array = data->array;\n-\tstruct ref_filter *filter = data->filter;\n-\n-\tif (match_pattern(filter->name_patterns, refname)) {\n-\t\tif (filter->with_commit) {\n-\t\t\tstruct commit *commit;\n-\n-\t\t\tcommit = lookup_commit_reference_gently(oid->hash, 1);\n-\t\t\tif (!commit)\n-\t\t\t\treturn 0;\n-\t\t\tif (!contains(commit, filter->with_commit))\n-\t\t\t\treturn 0;\n-\t\t}\n-\n-\t\tif (filter->points_at.nr && !match_points_at(refname, oid->hash, &filter->points_at))\n-\t\t\treturn 0;\n-\n-\t\tif (!filter->lines) {\n-\t\t\tif (tag_sort)\n-\t\t\t\tref_array_append(array, refname);\n-\t\t\telse\n-\t\t\t\tprintf(\"%s\\n\", refname);\n-\t\t\treturn 0;\n-\t\t}\n-\t\tprintf(\"%-15s \", refname);\n-\t\tshow_tag_lines(oid, filter->lines);\n-\t\tputchar('\\n');\n-\t}\n-\n-\treturn 0;\n-}\n-\n-static int sort_by_version(const void *a_, const void *b_)\n-{\n-\tconst struct ref_array_item *a = *((struct ref_array_item **)a_);\n-\tconst struct ref_array_item *b = *((struct ref_array_item **)b_);\n-\treturn versioncmp(a->refname, b->refname);\n-}\n-\n-static int list_tags(struct ref_filter *filter, int sort)\n+static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting)\n {\n \tstruct ref_array array;\n-\tstruct ref_filter_cbdata data;\n+\tchar *format, *to_free = NULL;\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 = to_free = xstrfmt(\"%%(align:15,left)%%(refname:short)%%(end) %%(contents:lines=%d)\",\n+\t\t\t\t\t filter->lines);\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, 0);\n+\tref_array_clear(&array);\n+\tfree(to_free);\n+\n \treturn 0;\n }\n \n@@ -366,35 +122,26 @@ static const char tag_template_nocleanup[] =\n \t\"Lines starting with '%c' will be kept; you may remove them\"\n \t\" yourself if you want to.\\n\");\n \n-/*\n- * Parse a sort string, and return 0 if parsed successfully. Will return\n- * non-zero when the sort string does not parse into a known type. If var is\n- * given, the error message becomes a warning and includes information about\n- * the configuration value.\n- */\n-static int parse_sort_string(const char *var, const char *arg, int *sort)\n+/* Parse arg given and add it the ref_sorting array */\n+static int parse_sorting_string(const char *arg, struct ref_sorting **sorting_tail)\n {\n-\tint type = 0, flags = 0;\n-\n-\tif (skip_prefix(arg, \"-\", &arg))\n-\t\tflags |= REVERSE_SORT;\n+\tstruct ref_sorting *s;\n+\tint len;\n \n-\tif (skip_prefix(arg, \"version:\", &arg) || skip_prefix(arg, \"v:\", &arg))\n-\t\ttype = VERCMP_SORT;\n-\telse\n-\t\ttype = STRCMP_SORT;\n+\ts = xcalloc(1, sizeof(*s));\n+\ts->next = *sorting_tail;\n+\t*sorting_tail = s;\n \n-\tif (strcmp(arg, \"refname\")) {\n-\t\tif (!var)\n-\t\t\treturn error(_(\"unsupported sort specification '%s'\"), arg);\n-\t\telse {\n-\t\t\twarning(_(\"unsupported sort specification '%s' in variable '%s'\"),\n-\t\t\t\tvar, arg);\n-\t\t\treturn -1;\n-\t\t}\n+\tif (*arg == '-') {\n+\t\ts->reverse = 1;\n+\t\targ++;\n \t}\n+\tif (skip_prefix(arg, \"version:\", &arg) ||\n+\t skip_prefix(arg, \"v:\", &arg))\n+\t\ts->version = 1;\n \n-\t*sort = (type | flags);\n+\tlen = strlen(arg);\n+\ts->atom = parse_ref_filter_atom(arg, arg+len);\n \n \treturn 0;\n }\n@@ -402,11 +149,12 @@ static int parse_sort_string(const char *var, const char *arg, int *sort)\n static int git_tag_config(const char *var, const char *value, void *cb)\n {\n \tint status;\n+\tstruct ref_sorting **sorting_tail = (struct ref_sorting **)cb;\n \n \tif (!strcmp(var, \"tag.sort\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n-\t\tparse_sort_string(var, value, &tag_sort);\n+\t\tparse_sorting_string(value, sorting_tail);\n \t\treturn 0;\n \t}\n \n@@ -564,13 +312,6 @@ static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)\n \treturn check_refname_format(sb->buf, 0);\n }\n \n-static int parse_opt_sort(const struct option *opt, const char *arg, int unset)\n-{\n-\tint *sort = opt->value;\n-\n-\treturn parse_sort_string(NULL, arg, sort);\n-}\n-\n int cmd_tag(int argc, const char **argv, const char *prefix)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -587,6 +328,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 +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\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 +364,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 +390,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 +400,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":"268904","messageId":"1440857537-13968-13-git-send-email-Karthik.188@gmail.com","threadId":"40216","inReplyTo":"1440857537-13968-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v14 12/13] tag.c: implement '--format' option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-29T14:12:16Z","receivedAt":"2015-08-29T14:12:16Z","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 | 22 +++++++++++++---------\n t/t7004-tag.sh | 12 ++++++++++++\n 3 files changed, 32 insertions(+), 10 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 2d348f4..dc4494d 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -23,17 +23,17 @@ 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, *to_free = NULL;\n+\tchar *to_free = NULL;\n \tint i;\n \n \tmemset(&array, 0, sizeof(array));\n@@ -41,11 +41,13 @@ 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 = to_free = xstrfmt(\"%%(align:15,left)%%(refname:short)%%(end) %%(contents:lines=%d)\",\n-\t\t\t\t\t filter->lines);\n-\telse\n-\t\tformat = \"%(refname:short)\";\n+\tif (!format) {\n+\t\tif (filter->lines)\n+\t\t\tformat = to_free = xstrfmt(\"%%(align:15,left)%%(refname:short)%%(end) %%(contents:lines=%d)\",\n+\t\t\t\t\t\t filter->lines);\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@@ -329,6 +331,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@@ -361,6 +364,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@@ -401,7 +405,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":"268903","messageId":"1440857537-13968-14-git-send-email-Karthik.188@gmail.com","threadId":"40216","inReplyTo":"1440857537-13968-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v14 13/13] tag.c: implement '--merged' and '--no-merged' options","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-29T14:12:17Z","receivedAt":"2015-08-29T14:12:17Z","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 dc4494d..9fa1400 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@@ -358,6 +358,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@@ -416,6 +418,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":"268907","messageId":"55E1E778.4030400@web.de","threadId":"40216","inReplyTo":"1440857537-13968-4-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v14 03/13] utf8: add function to align a string into given strbuf","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-08-29T17:10:16Z","receivedAt":"2015-08-29T17:10:16Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 29.08.15 16:12, Karthik Nayak wrote:\n> diff --git a/utf8.h b/utf8.h\n> index 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;\nshould this be called strbuf_align_type ?\n\nAnd is there a reason why the is in utf.c and not in stbuf.c ?\n\n(I know that there is a lot of strbuf in utf8.c, but I hadn't managed to send a patch\nto move everything into strbuf.c and make utf8.c un-aware of all strbub-business)\n"},{"id":"268908","messageId":"CAOLa=ZRq4aJMYcRBaaLV9XFJ0LkeO27cZwaaJUmRMvCnHD1SAQ@mail.gmail.com","threadId":"40216","inReplyTo":"55E1E778.4030400@web.de","subject":"Re: [PATCH v14 03/13] utf8: add function to align a string into given strbuf","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-29T17:33:04Z","receivedAt":"2015-08-29T17:33:04Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sat, Aug 29, 2015 at 10:40 PM, Torsten Bögershausen wrote:\n> On 29.08.15 16:12, Karthik Nayak wrote:\n>> diff --git a/utf8.h b/utf8.h\n>> index 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>> + ALIGN_LEFT,\n>> + ALIGN_MIDDLE,\n>> + ALIGN_RIGHT\n>> +} align_type;\n> should this be called strbuf_align_type ?\n>\n\nalign_type seemed descriptive and unique enough.\n\n> And is there a reason why the is in utf.c and not in stbuf.c ?\n>\n> (I know that there is a lot of strbuf in utf8.c, but I hadn't managed to send a patch\n> to move everything into strbuf.c and make utf8.c un-aware of all strbub-business)\n>\n\nThis was based on Eric's suggestions.\nhttp://article.gmane.org/gmane.comp.version-control.git/275456\n\nIt makes sense also, since rather than acting on a strbuf, this is more of just\nutilizing an strbuf to provide the result. whereas the real work is of\nalignment.\n\n\n\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268913","messageId":"CAPig+cRHRPehkd+9PwOqpXkRUvaJa42zLtCKMEfv2W=ZJUZJzA@mail.gmail.com","threadId":"40216","inReplyTo":"1440857537-13968-5-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v14 04/13] ref-filter: implement an `align` atom","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-30T03:27:23Z","receivedAt":"2015-08-30T03:27:23Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak wrote:\n> Implement an `align` atom which left-, middle-, or right-aligns the\n> content between %(align:..) and %(end).\n>\n> It is followed by `:,`, where the `` is\n> either left, right or middle and `` is the size of the area\n> into which the content will be placed. If the content between\n> %(align:) and %(end) is more than the width then no alignment is\n> performed. e.g. to align a refname atom to the middle with a total\n> width of 40 we can do: --format=\"%(align:middle,40)%(refname)%(end)\".\n>\n> We now have a `handler()` for each atom_value which will be called\n> when that atom_value is being parsed, and similarly an `at_end`\n> function 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\n> which aligns the given strbuf by calling `strbuf_utf8_align()` from\n> utf8.c.\n>\n> Extract perform_quote_formatting() from append_atom(). Given a string\n> a quote_value and a strbuf, perform_quote_formatting() formats the\n> string based on the quote_value and stores it into the strbuf.\n>\n> Ensure that quote formatting is performed on the whole of\n> %(align)...%(end) rather than individual atoms. We do this by skipping\n> individual quote formatting for atoms whenever the stack has more than\n> one element, and performing formatting for the entire stack element\n> when the `%(end)` atoms is encountered.\n\nThis patch seems to be conflating three distinct changes:\n\n1. adding %(align:) atom\n2. extracting quoting logic to a separate function\n3. quoting top-level %(align:) but not contained atoms\n\nIn fact, #3 seems too specially tied to %(align:)...%(end). One might\nexpect that the logic for determining when to quote should be\nindependent of any particular atom, which suggests that this logic is\nbeing handled at the wrong level, and that %(align:) shouldn't have to\nknow anything about quoting. I'd have thought the quoting logic would\nnaturally accompany the introduction of the formatting state stack\nmechanism in patch 2/13, and that that would generically work with all\natoms, including %(align:) and whatever new ones are added in the\nfuture.\n\nBut, I may not have been following the quoting discussion closely, so\nperhaps these observations are incorrect? See more below regarding\n%(if:).\n\n> Add documentation and tests for the same.\n>\n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 432cea0..21c8b5f 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -53,6 +54,13 @@ static struct {\n> { \"flag\" },\n> { \"HEAD\" },\n> { \"color\" },\n> + { \"align\" },\n> + { \"end\" },\n> +};\n> +\n> +struct align {\n> + align_type position;\n> + unsigned int width;\n> };\n>\n> #define REF_FORMATTING_STATE_INIT { 0, NULL }\n> @@ -69,6 +79,8 @@ struct ref_formatting_state {\n>\n> struct atom_value {\n> const char *s;\n> + struct align *align;\n\nWhy does 'align' need to be heap-allocated rather than just being a\ndirect member of 'atom_value'? Does 'align' need to exist beyond the\nlifetime of its 'atom_value'? If not, making it a direct member might\nsimplify resource management (no need to free it).\n\n> + void (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);\n> unsigned long ul; /* used for sorting when not FIELD_STR */\n> };\n>\n> @@ -632,6 +644,84 @@ static inline char *copy_advance(char *dst, const char *src)\n> +static void append_atom(struct atom_value *v, struct ref_formatting_state *state)\n> +{\n> + /*\n> + * Quote formatting is only done when the stack has a single\n> + * element. Otherwise quote formatting is done on the\n> + * element's entire output strbuf when the %(end) atom is\n> + * encountered.\n> + */\n> + if (!state->stack->prev)\n\nWith the disclaimer that I wasn't following the quoting discussion\nclosely: Is this condition going to be sufficient for all cases, such\nas an %(if:) atom? That is, if you have:\n\n %(if:notempty)%(bloop)%(then) --option=%(bloop)%(end)\n\nisn't the intention that, %(bloop) within the %(then) section should\nbe quoted but not the literal \"--option=\"?\n\nThe condition `!state->stack->prev' would be insufficient to handle\nthis if %(if:) pushes one or more states onto the stack, no? This\nimplies that you might want an explicit flag for enabling/disabling\nquoting rather than relying upon the state of the stack, and that\nindividual atom handlers would control that flag.\n\nOr, am I misunderstanding due to not having followed the discussion closely?\n\n> + quote_formatting(&state->stack->output, v->s, state->quote_style);\n> + else\n> + strbuf_addstr(&state->stack->output, v->s);\n> +}\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) {\n> + quote_formatting(&s, current->output.buf, state->quote_style);\n> + strbuf_reset(¤t->output);\n> + strbuf_addbuf(¤t->output, &s);\n\nstrbuf_swap() can replace the above two lines.\n\n> + }\n> + strbuf_release(&s);\n> + pop_stack_element(&state->stack);\n> +}\n> +\n> /*\n> * Parse the object referred by ref, and grab needed value.\n> */\n> @@ -725,6 +818,37 @@ static void populate_value(struct ref_array_item *ref)\n> else\n> v->s = \" \";\n> continue;\n> + } else if (!strcmp(name, \"align\"))\n> + die(_(\"format: incomplete use of the `align` atom\"));\n\nWhy does %(align) get flagged as a malformation of %(align:), whereas\n%(color) does not get flagged as a malformation of %(color:)? Why does\none deserve special treatment but not the other?\n\n> + else if (skip_prefix(name, \"align:\", &valp)) {\n> + struct align *align = xmalloc(sizeof(struct align));\n> + struct strbuf **s = strbuf_split_str(valp, ',', 0);\n> +\n> + /* If the position is given trim the ',' from the first strbuf */\n> + if (s[1])\n> + strbuf_remove(s[0], s[0]->len - 1, 1);\n\nThis is a truncation operation, which may be more idiomatically stated as:\n\n strbuf_setlen(s[0], s[0]->len - 1);\n\n> +\n> + if (strtoul_ui(s[0]->buf, 10, &align->width))\n> + die(_(\"positive width expected align:%s\"), s[0]->buf);\n> +\n> + /* If no position is given, default to ALIGN_LEFT */\n> + if (!s[1] || !strcmp(s[1]->buf, \"left\"))\n> + align->position = ALIGN_LEFT;\n\nIf you structured the code like this:\n\n if (!s[1])\n align->position = ALIGN_LEFT;\n else if (!strcmp(s[1]->buf, \"left\"))\n align->position = ALIGN_LEFT;\n else if ...\n\nthen the comment about \"default\" would become unnecessary, and it\nwould be easier to change the default in the future (if the need ever\narose), however, this is a very minor point, and I don't care\nstrongly.\n\n> + else if (!strcmp(s[1]->buf, \"right\"))\n> + align->position = ALIGN_RIGHT;\n> + else if (!strcmp(s[1]->buf, \"middle\"))\n> + align->position = ALIGN_MIDDLE;\n> + else\n> + die(_(\"improper format entered align:%s\"), s[1]->buf);\n> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\n> index 505a360..cef7a41 100755\n> --- a/t/t6302-for-each-ref-filter.sh\n> +++ b/t/t6302-for-each-ref-filter.sh\n> @@ -81,4 +81,89 @@ test_expect_success 'filtering with --contains' '\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> + cat >expect <<-EOF &&\n> + refname is ${sq} ${sq}\\\\${sq}${sq}master${sq}\\\\${sq}${sq} ${sq}|\n> + refname is ${sq} ${sq}\\\\${sq}${sq}side${sq}\\\\${sq}${sq} ${sq}|\n> + refname is ${sq} ${sq}\\\\${sq}${sq}odd/spot${sq}\\\\${sq}${sq} ${sq}|\n> + refname is ${sq} ${sq}\\\\${sq}${sq}double-tag${sq}\\\\${sq}${sq} ${sq}|\n> + refname is ${sq} ${sq}\\\\${sq}${sq}four${sq}\\\\${sq}${sq} ${sq}|\n> + refname is ${sq} ${sq}\\\\${sq}${sq}one${sq}\\\\${sq}${sq} ${sq}|\n> + refname is ${sq} ${sq}\\\\${sq}${sq}signed-tag${sq}\\\\${sq}${sq} ${sq}|\n> + refname is ${sq} ${sq}\\\\${sq}${sq}three${sq}\\\\${sq}${sq} ${sq}|\n> + refname is ${sq} ${sq}\\\\${sq}${sq}two${sq}\\\\${sq}${sq} ${sq}|\n> + EOF\n> + git for-each-ref --shell --format=\"refname is %(align:30,middle)${sq}%(refname:short)${sq}%(end)|\" >actual &&\n\nThis can be much less noisy if you change the second argument of\ntest_expect_success to use double-quote rather than single-quote. That\nwould allow you to use single-quote directly in the test and expected\noutput rather than having to use ${sq} indirection.\n\n test_expect_success 'alignment with format quote' \"\n cat >expect <<-\\EOF &&\n refname is ' '''master''' '|\n ...\n EOF\n git for-each-ref --shell --format='refname ...\n\\\\'%(rename:short)\\\\'...|'\n \"\n\nor something.\n\n> + test_cmp expect actual\n> +'\n"},{"id":"268914","messageId":"CAPig+cRHc29TL=sHv3dkEewVQzPo0m8O4d1VL+dWA6PC5pHowA@mail.gmail.com","threadId":"40216","inReplyTo":"1440857537-13968-6-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v14 05/13] ref-filter: add option to filter out tags, branches and remotes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-30T03:30:02Z","receivedAt":"2015-08-30T03:30:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak wrote:\n> Add a function called 'for_each_fullref_in()' to refs.{c,h} which\n> iterates through each ref for the given path without trimming the path\n> 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> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/refs.c b/refs.c\n> index 4e15f60..a9469c2 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2108,6 +2108,15 @@ int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data)\n> return do_for_each_ref(&ref_cache, prefix, fn, strlen(prefix), 0, cb_data);\n> }\n>\n> +int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, unsigned int broken)\n\nBooleans such as 'broken' are typically declared 'int' in this\ncodebase, rather than 'unsigned int'.\n\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, prefix, fn, 0, flag, cb_data);\n> +}\n> +\n> int for_each_ref_in_submodule(const char *submodule, const char *prefix,\n> each_ref_fn fn, void *cb_data)\n> {\n> diff --git a/refs.h b/refs.h\n> index e9a5f32..6d30c98 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -173,6 +173,7 @@ typedef int each_ref_fn(const char *refname,\n> extern int head_ref(each_ref_fn fn, void *cb_data);\n> extern int for_each_ref(each_ref_fn fn, void *cb_data);\n> extern int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data);\n> +extern int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, unsigned int broken);\n> extern int for_each_tag_ref(each_ref_fn fn, void *cb_data);\n> extern int for_each_branch_ref(each_ref_fn fn, void *cb_data);\n> extern int for_each_remote_ref(each_ref_fn fn, void *cb_data);\n"},{"id":"268915","messageId":"CAPig+cT2ySwYtTjv7Xe3uv35OH8Bhdyj3m_eqyJjMe_mB6Xj=w@mail.gmail.com","threadId":"40216","inReplyTo":"1440857537-13968-7-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v14 06/13] ref-filter: introduce format_ref_array_item()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-30T03:42:19Z","receivedAt":"2015-08-30T03:42:19Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak wrote:\n> Create format_ref_array_item() out of show_ref_array_item(). This will\n> store the output format for the given ref_array_item into the provided\n> strbuf. Make show_ref_array_item() a wrapper around this to print the\n> given ref_array_item with linefeed.\n\nPerhaps you could explain why this change is a good idea, such as that\na future patch, for reason, will need the\nformatting capability of format_ref_array_item() but not the printing\nwith newline done by show_ref_array_item().\n\n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 5d4f93d..1e6754a 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -153,6 +153,27 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n> return at;\n> }\n>\n> +static void quote_formatting(struct strbuf *s, const char *str, int quote_style)\n> +{\n> + switch (quote_style) {\n> + case QUOTE_NONE:\n> + strbuf_addstr(s, str);\n> + break;\n> + case QUOTE_SHELL:\n> + sq_quote_buf(s, str);\n> + break;\n> + case QUOTE_PERL:\n> + perl_quote_buf(s, str);\n> + break;\n> + case QUOTE_PYTHON:\n> + python_quote_buf(s, str);\n> + break;\n> + case QUOTE_TCL:\n> + tcl_quote_buf(s, str);\n> + break;\n> + }\n> +}\n\nThis code was already relocated once in patch 4/13, and is now being\nrelocated again in 6/13. If you instead place the code at the final\ndesired location in 4/13, then this patch will become less noisy.\n\nMore below.\n\n> static void push_stack_element(struct ref_formatting_stack **stack)\n> {\n> struct ref_formatting_stack *s = xcalloc(1, sizeof(struct ref_formatting_stack));\n> @@ -665,27 +686,6 @@ static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_s\n> new->cb_data = atomv->align;\n> }\n>\n> -static void quote_formatting(struct strbuf *s, const char *str, int quote_style)\n> -{\n> - switch (quote_style) {\n> - case QUOTE_NONE:\n> - strbuf_addstr(s, str);\n> - break;\n> - case QUOTE_SHELL:\n> - sq_quote_buf(s, str);\n> - break;\n> - case QUOTE_PERL:\n> - perl_quote_buf(s, str);\n> - break;\n> - case QUOTE_PYTHON:\n> - python_quote_buf(s, str);\n> - break;\n> - case QUOTE_TCL:\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> /*\n> @@ -1478,10 +1478,17 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n> }\n> if (state.stack->prev)\n> die(_(\"format: `end` atom missing\"));\n> - final_buf = &state.stack->output;\n> - fwrite(final_buf->buf, 1, final_buf->len, stdout);\n> + strbuf_addbuf(out, &state.stack->output);\n> pop_stack_element(&state.stack);\n> - putchar('\\n');\n> +}\n> +\n> +void show_ref_array_item(struct ref_array_item *item, const char *format, unsigned int quote_style)\n> +{\n> + struct strbuf out = STRBUF_INIT;\n> + format_ref_array_item(&out, item, format, quote_style);\n> + fwrite(out.buf, out.len, 1, stdout);\n> + printf(\"\\n\");\n\nputchar('\\n');\n\n> + strbuf_release(&out);\n> }\n>\n> /* If no sorting option is given, use refname to sort as default */\n"},{"id":"268918","messageId":"CAOLa=ZTiVS=3M8ckr2Ouz_46h7zaHBcRyxqHMbhGEAt3uWNaCQ@mail.gmail.com","threadId":"40216","inReplyTo":"CAPig+cT2ySwYtTjv7Xe3uv35OH8Bhdyj3m_eqyJjMe_mB6Xj=w@mail.gmail.com","subject":"Re: [PATCH v14 06/13] ref-filter: introduce format_ref_array_item()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-30T06:39:36Z","receivedAt":"2015-08-30T06:39:36Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sun, Aug 30, 2015 at 9:12 AM, Eric Sunshine wrote:\n> On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak wrote:\n>> Create format_ref_array_item() out of show_ref_array_item(). This will\n>> store the output format for the given ref_array_item into the provided\n>> strbuf. Make show_ref_array_item() a wrapper around this to print the\n>> given ref_array_item with linefeed.\n>\n> Perhaps you could explain why this change is a good idea, such as that\n> a future patch, for reason, will need the\n> formatting capability of format_ref_array_item() but not the printing\n> with newline done by show_ref_array_item().\n>\n\nYeah sure.\n\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> index 5d4f93d..1e6754a 100644\n>> --- a/ref-filter.c\n>> +++ b/ref-filter.c\n>> @@ -153,6 +153,27 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n>> return at;\n>> }\n>>\n>> +static void quote_formatting(struct strbuf *s, const char *str, int quote_style)\n>> +{\n>> + switch (quote_style) {\n>> + case QUOTE_NONE:\n>> + strbuf_addstr(s, str);\n>> + break;\n>> + case QUOTE_SHELL:\n>> + sq_quote_buf(s, str);\n>> + break;\n>> + case QUOTE_PERL:\n>> + perl_quote_buf(s, str);\n>> + break;\n>> + case QUOTE_PYTHON:\n>> + python_quote_buf(s, str);\n>> + break;\n>> + case QUOTE_TCL:\n>> + tcl_quote_buf(s, str);\n>> + break;\n>> + }\n>> +}\n>\n> This code was already relocated once in patch 4/13, and is now being\n> relocated again in 6/13. If you instead place the code at the final\n> desired location in 4/13, then this patch will become less noisy.\n>\n\nWill do.\n\n> More below.\n>\n>> static void push_stack_element(struct ref_formatting_stack **stack)\n>> {\n>> struct ref_formatting_stack *s = xcalloc(1, sizeof(struct ref_formatting_stack));\n>> @@ -665,27 +686,6 @@ static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_s\n>> new->cb_data = atomv->align;\n>> }\n>>\n>> -static void quote_formatting(struct strbuf *s, const char *str, int quote_style)\n>> -{\n>> - switch (quote_style) {\n>> - case QUOTE_NONE:\n>> - strbuf_addstr(s, str);\n>> - break;\n>> - case QUOTE_SHELL:\n>> - sq_quote_buf(s, str);\n>> - break;\n>> - case QUOTE_PERL:\n>> - perl_quote_buf(s, str);\n>> - break;\n>> - case QUOTE_PYTHON:\n>> - python_quote_buf(s, str);\n>> - break;\n>> - case QUOTE_TCL:\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>> /*\n>> @@ -1478,10 +1478,17 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n>> }\n>> if (state.stack->prev)\n>> die(_(\"format: `end` atom missing\"));\n>> - final_buf = &state.stack->output;\n>> - fwrite(final_buf->buf, 1, final_buf->len, stdout);\n>> + strbuf_addbuf(out, &state.stack->output);\n>> pop_stack_element(&state.stack);\n>> - putchar('\\n');\n>> +}\n>> +\n>> +void show_ref_array_item(struct ref_array_item *item, const char *format, unsigned int quote_style)\n>> +{\n>> + struct strbuf out = STRBUF_INIT;\n>> + format_ref_array_item(&out, item, format, quote_style);\n>> + fwrite(out.buf, out.len, 1, stdout);\n>> + printf(\"\\n\");\n>\n> putchar('\\n');\n\nThanks for the review.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268919","messageId":"CAOLa=ZQh_3aznJz80W4acLKEsFVWoxCv9CMo5uaq68JeKQ7x9Q@mail.gmail.com","threadId":"40216","inReplyTo":"CAPig+cT2ySwYtTjv7Xe3uv35OH8Bhdyj3m_eqyJjMe_mB6Xj=w@mail.gmail.com","subject":"Re: [PATCH v14 06/13] ref-filter: introduce format_ref_array_item()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-30T06:49:13Z","receivedAt":"2015-08-30T06:49:13Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sun, Aug 30, 2015 at 9:12 AM, Eric Sunshine wrote:\n> On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak wrote:\n>> Create format_ref_array_item() out of show_ref_array_item(). This will\n>> store the output format for the given ref_array_item into the provided\n>> strbuf. Make show_ref_array_item() a wrapper around this to print the\n>> given ref_array_item with linefeed.\n>\n> Perhaps you could explain why this change is a good idea, such as that\n> a future patch, for reason, will need the\n> formatting capability of format_ref_array_item() but not the printing\n> with newline done by show_ref_array_item().\n>\n\nThinking along these lines, it's aim was to use with printing lines,\nbut since that\nis done with %(contents:lines=X) it might not be useful here, I see it\nbeing used with\nfuture branch.c printing, but maybe it would make more sense to\nintroduce it there\nin that series.\n\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> index 5d4f93d..1e6754a 100644\n>> --- a/ref-filter.c\n>> +++ b/ref-filter.c\n>> @@ -153,6 +153,27 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n>> return at;\n>> }\n>>\n>> +static void quote_formatting(struct strbuf *s, const char *str, int quote_style)\n>> +{\n>> + switch (quote_style) {\n>> + case QUOTE_NONE:\n>> + strbuf_addstr(s, str);\n>> + break;\n>> + case QUOTE_SHELL:\n>> + sq_quote_buf(s, str);\n>> + break;\n>> + case QUOTE_PERL:\n>> + perl_quote_buf(s, str);\n>> + break;\n>> + case QUOTE_PYTHON:\n>> + python_quote_buf(s, str);\n>> + break;\n>> + case QUOTE_TCL:\n>> + tcl_quote_buf(s, str);\n>> + break;\n>> + }\n>> +}\n>\n> This code was already relocated once in patch 4/13, and is now being\n> relocated again in 6/13. If you instead place the code at the final\n> desired location in 4/13, then this patch will become less noisy.\n>\n> More below.\n>\n>> static void push_stack_element(struct ref_formatting_stack **stack)\n>> {\n>> struct ref_formatting_stack *s = xcalloc(1, sizeof(struct ref_formatting_stack));\n>> @@ -665,27 +686,6 @@ static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_s\n>> new->cb_data = atomv->align;\n>> }\n>>\n>> -static void quote_formatting(struct strbuf *s, const char *str, int quote_style)\n>> -{\n>> - switch (quote_style) {\n>> - case QUOTE_NONE:\n>> - strbuf_addstr(s, str);\n>> - break;\n>> - case QUOTE_SHELL:\n>> - sq_quote_buf(s, str);\n>> - break;\n>> - case QUOTE_PERL:\n>> - perl_quote_buf(s, str);\n>> - break;\n>> - case QUOTE_PYTHON:\n>> - python_quote_buf(s, str);\n>> - break;\n>> - case QUOTE_TCL:\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>> /*\n>> @@ -1478,10 +1478,17 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n>> }\n>> if (state.stack->prev)\n>> die(_(\"format: `end` atom missing\"));\n>> - final_buf = &state.stack->output;\n>> - fwrite(final_buf->buf, 1, final_buf->len, stdout);\n>> + strbuf_addbuf(out, &state.stack->output);\n>> pop_stack_element(&state.stack);\n>> - putchar('\\n');\n>> +}\n>> +\n>> +void show_ref_array_item(struct ref_array_item *item, const char *format, unsigned int quote_style)\n>> +{\n>> + struct strbuf out = STRBUF_INIT;\n>> + format_ref_array_item(&out, item, format, quote_style);\n>> + fwrite(out.buf, out.len, 1, stdout);\n>> + printf(\"\\n\");\n>\n> putchar('\\n');\n>\n>> + strbuf_release(&out);\n>> }\n>>\n>> /* If no sorting option is given, use refname to sort as default */\n\n\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268920","messageId":"CAOLa=ZSn6z03+jU1Ytn6nL0f0hQ36YAxG4629E1jTpyiaO7BZQ@mail.gmail.com","threadId":"40216","inReplyTo":"CAPig+cRHc29TL=sHv3dkEewVQzPo0m8O4d1VL+dWA6PC5pHowA@mail.gmail.com","subject":"Re: [PATCH v14 05/13] ref-filter: add option to filter out tags, branches and remotes","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-30T06:51:52Z","receivedAt":"2015-08-30T06:51:52Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sun, Aug 30, 2015 at 9:00 AM, Eric Sunshine wrote:\n> On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak wrote:\n>> Add a function called 'for_each_fullref_in()' to refs.{c,h} which\n>> iterates through each ref for the given path without trimming the path\n>> 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>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/refs.c b/refs.c\n>> index 4e15f60..a9469c2 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -2108,6 +2108,15 @@ int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data)\n>> return do_for_each_ref(&ref_cache, prefix, fn, strlen(prefix), 0, cb_data);\n>> }\n>>\n>> +int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, unsigned int broken)\n>\n> Booleans such as 'broken' are typically declared 'int' in this\n> codebase, rather than 'unsigned int'.\n>\n\nBut doesn't it make more sense to have it as unsigned, since its values are\neither 0 or 1?\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268921","messageId":"CAPig+cRK9Ljj_qO36JhYtt+2aGC0NfhC0AeuT65B_mYj25+nnw@mail.gmail.com","threadId":"40216","inReplyTo":"CAOLa=ZSn6z03+jU1Ytn6nL0f0hQ36YAxG4629E1jTpyiaO7BZQ@mail.gmail.com","subject":"Re: [PATCH v14 05/13] ref-filter: add option to filter out tags, branches and remotes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-30T07:16:06Z","receivedAt":"2015-08-30T07:16:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Aug 30, 2015 at 2:51 AM, Karthik Nayak wrote:\n> On Sun, Aug 30, 2015 at 9:00 AM, Eric Sunshine wrote:\n>> On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak wrote:\n>>> +int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, unsigned int broken)\n>>\n>> Booleans such as 'broken' are typically declared 'int' in this\n>> codebase, rather than 'unsigned int'.\n>\n> But doesn't it make more sense to have it as unsigned, since its values are\n> either 0 or 1?\n\nIn C, zero is false and any other value is true, so from that\nviewpoint, the type doesn't matter much. However, beside being raw\ninstructions for the computer, code should ideally convey its\nintention as clearly as possible to other programmers. 'int' for\nboolean has been idiomatic since C's inception, thus is a good way to\ndo so, whereas 'unsigned int' is typically used for magnitude or bit\nflags, thus is misleading.\n"},{"id":"268922","messageId":"CAPig+cQmAkFAjUcJNJNF2gwunX6TKK+oQsd2QnmJwxLT=EEvjQ@mail.gmail.com","threadId":"40216","inReplyTo":"1440857537-13968-8-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v14 07/13] ref-filter: add support for %(contents:lines=X)","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-30T07:53:31Z","receivedAt":"2015-08-30T07:53:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak wrote:\n> In 'tag.c' we can print N lines from the annotation of the tag using\n> the '-n' option. Copy code from 'tag.c' to 'ref-filter' and\n> modify it to support appending of N lines from the annotation of tags\n> to the given strbuf.\n>\n> Implement %(contents:lines=X) where X lines of the given object are\n> obtained.\n>\n> Add documentation and test for the same.\n>\n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index 471d6b1..0fc7557 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -63,6 +64,11 @@ struct align {\n> unsigned int width;\n> };\n>\n> +struct contents {\n> + unsigned int lines;\n> + struct object_id oid;\n> +};\n> +\n> #define REF_FORMATTING_STATE_INIT { 0, NULL }\n>\n> struct ref_formatting_stack {\n> @@ -80,6 +86,7 @@ struct ref_formatting_state {\n> struct atom_value {\n> const char *s;\n> struct align *align;\n> + struct contents *contents;\n\nSame question as for 'align': Does 'contents' need to be\nheap-allocated because it must exist beyond the lifetime of\n'atom_value'? If not, making it just a plain member of 'atom_value'\nwould simplify memory management (no need to free it).\n\nAlso, will 'align' and 'contents' ever be used at the same time? If\nnot, it might make sense to place them in a 'union' (not for the\nmemory saving, but to make it clear to the reader that their use is\nmutually exclusive).\n\nMore below.\n\n> void (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);\n> unsigned long ul; /* used for sorting when not FIELD_STR */\n> };\n> @@ -569,6 +576,61 @@ static void find_subpos(const char *buf, unsigned long sz,\n> *nonsiglen = *sig - buf;\n> }\n>\n> +/*\n> + * If 'lines' is greater than 0, append that many lines from the given\n> + * object_id 'oid' to the given strbuf.\n> + */\n> +static void append_tag_lines(struct strbuf *out, const struct object_id *oid, int lines)\n> +{\n> + int i;\n> + unsigned long size;\n> + enum object_type type;\n> + char *buf, *sp, *eol;\n> + size_t len;\n> +\n> + buf = read_sha1_file(oid->hash, &type, &size);\n> + if (!buf)\n> + die_errno(\"unable to read object %s\", oid_to_hex(oid));\n> + if (type != OBJ_COMMIT && type != OBJ_TAG)\n> + goto free_return;\n> + if (!size)\n> + die(\"an empty %s object %s?\",\n> + typename(type), oid_to_hex(oid));\n> +\n> + /* skip header */\n> + sp = strstr(buf, \"\\n\\n\");\n> + if (!sp)\n> + goto free_return;\n> +\n> + /* only take up to \"lines\" lines, and strip the signature from a tag */\n> + if (type == OBJ_TAG)\n> + size = parse_signature(buf, size);\n> + for (i = 0, sp += 2; i < lines && sp < buf + size; i++) {\n> + if (i)\n> + strbuf_addstr(out, \"\\n \");\n> + eol = memchr(sp, '\\n', size - (sp - buf));\n> + len = eol ? eol - sp : size - (sp - buf);\n> + strbuf_add(out, sp, len);\n> + if (!eol)\n> + break;\n> + sp = eol + 1;\n> + }\n> +free_return:\n> + free(buf);\n> +}\n\nI understand that you want to re-use this code from\ntag.c:show_tag_lines(), but (from a very cursory read) isn't this\nduplicating logic and processing already done elsewhere in\nref-filter.c? More about this below.\n\n> +\n> +static void contents_lines_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n> +{\n> + struct contents *contents = (struct contents *)atomv->contents;\n\nWhy is this cast needed?\n\n> + struct strbuf s = STRBUF_INIT;\n> +\n> + append_tag_lines(&s, &contents->oid, contents->lines);\n> + quote_formatting(&state->stack->output, s.buf, state->quote_style);\n> + strbuf_release(&s);\n> +\n> + free(contents);\n> +}\n> @@ -588,7 +651,8 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj\n> strcmp(name, \"contents\") &&\n> strcmp(name, \"contents:subject\") &&\n> strcmp(name, \"contents:body\") &&\n> - strcmp(name, \"contents:signature\"))\n> + strcmp(name, \"contents:signature\") &&\n> + !starts_with(name, \"contents:lines=\"))\n> continue;\n> if (!subpos)\n> find_subpos(buf, sz,\n> @@ -608,6 +672,15 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj\n> v->s = xmemdupz(sigpos, siglen);\n> else if (!strcmp(name, \"contents\"))\n> v->s = xstrdup(subpos);\n> + else if (skip_prefix(name, \"contents:lines=\", &valp)) {\n> + struct contents *contents = xmalloc(sizeof(struct contents));\n> +\n> + if (strtoul_ui(valp, 10, &contents->lines))\n> + die(_(\"positive width expected align:%s\"), valp);\n> + hashcpy(contents->oid.hash, obj->sha1);\n\nThe logic in append_tag_lines() which was copied from\ntag.c:show_tag_lines() goes through the effort of loading the object\nand parsing it, but hasn't the object already been loaded and parsed\nby the time you get to this point? Assuming I'm reading this\ncorrectly, wouldn't it make more sense to take advantage of the work\nalready done loading and parsing the object rather than repeating it\nall inside append_tag_lines()?\n\n> + v->handler = contents_lines_handler;\n> + v->contents = contents;\n> + }\n> }\n> }\n> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\n> index cef7a41..0277498 100755\n> --- a/t/t6302-for-each-ref-filter.sh\n> +++ b/t/t6302-for-each-ref-filter.sh\n> @@ -166,4 +166,20 @@ test_expect_success 'nested alignment' '\n> test_cmp expect actual\n> '\n>\n> +test_expect_success 'check `%(contents:lines=X)`' '\n> + cat >expect <<-\\EOF &&\n> + master three\n> + side four\n> + odd/spot three\n> + double-tag Annonated doubly\n> + four four\n> + one one\n> + signed-tag A signed tag message\n> + three three\n> + two two\n> + EOF\n> + git for-each-ref --format=\"%(refname:short) %(contents:lines=1)\" >actual &&\n\nMaybe also test some edge cases, such as line=0, lines=-1 (an invalid\nvalue), lines=2, lines=9999999 (a value larger than the number of\nlines in any object).\n\n> + test_cmp expect actual\n> +'\n> +\n> test_done\n> --\n> 2.5.0\n"},{"id":"268932","messageId":"CAOLa=ZTHtps2gJb9asSzG_4NOwx_xiA7OuCzjW=BTTFfkaYoQg@mail.gmail.com","threadId":"40216","inReplyTo":"CAPig+cRHRPehkd+9PwOqpXkRUvaJa42zLtCKMEfv2W=ZJUZJzA@mail.gmail.com","subject":"Re: [PATCH v14 04/13] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-30T13:38:57Z","receivedAt":"2015-08-30T13:38:57Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sun, Aug 30, 2015 at 8:57 AM, Eric Sunshine wrote:\n> On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak wrote:\n>> Implement an `align` atom which left-, middle-, or right-aligns the\n>> content between %(align:..) and %(end).\n>>\n>> It is followed by `:,`, where the `` is\n>> either left, right or middle and `` is the size of the area\n>> into which the content will be placed. If the content between\n>> %(align:) and %(end) is more than the width then no alignment is\n>> performed. e.g. to align a refname atom to the middle with a total\n>> width of 40 we can do: --format=\"%(align:middle,40)%(refname)%(end)\".\n>>\n>> We now have a `handler()` for each atom_value which will be called\n>> when that atom_value is being parsed, and similarly an `at_end`\n>> function 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\n>> which aligns the given strbuf by calling `strbuf_utf8_align()` from\n>> utf8.c.\n>>\n>> Extract perform_quote_formatting() from append_atom(). Given a string\n>> a quote_value and a strbuf, perform_quote_formatting() formats the\n>> string based on the quote_value and stores it into the strbuf.\n>>\n>> Ensure that quote formatting is performed on the whole of\n>> %(align)...%(end) rather than individual atoms. We do this by skipping\n>> individual quote formatting for atoms whenever the stack has more than\n>> one element, and performing formatting for the entire stack element\n>> when the `%(end)` atoms is encountered.\n>\n> This patch seems to be conflating three distinct changes:\n>\n> 1. adding %(align:) atom\n> 2. extracting quoting logic to a separate function\n> 3. quoting top-level %(align:) but not contained atoms\n>\n\nI'll extract 2 to a separate commit.\n\n> In fact, #3 seems too specially tied to %(align:)...%(end). One might\n> expect that the logic for determining when to quote should be\n> independent of any particular atom, which suggests that this logic is\n> being handled at the wrong level, and that %(align:) shouldn't have to\n> know anything about quoting. I'd have thought the quoting logic would\n> naturally accompany the introduction of the formatting state stack\n> mechanism in patch 2/13, and that that would generically work with all\n> atoms, including %(align:) and whatever new ones are added in the\n> future.\n>\n> But, I may not have been following the quoting discussion closely, so\n> perhaps these observations are incorrect? See more below regarding\n> %(if:).\n\nYou're right, more below.\n\n>\n>> Add documentation and tests for the same.\n>>\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> index 432cea0..21c8b5f 100644\n>> --- a/ref-filter.c\n>> +++ b/ref-filter.c\n>> @@ -53,6 +54,13 @@ static struct {\n>> { \"flag\" },\n>> { \"HEAD\" },\n>> { \"color\" },\n>> + { \"align\" },\n>> + { \"end\" },\n>> +};\n>> +\n>> +struct align {\n>> + align_type position;\n>> + unsigned int width;\n>> };\n>>\n>> #define REF_FORMATTING_STATE_INIT { 0, NULL }\n>> @@ -69,6 +79,8 @@ struct ref_formatting_state {\n>>\n>> struct atom_value {\n>> const char *s;\n>> + struct align *align;\n>\n> Why does 'align' need to be heap-allocated rather than just being a\n> direct member of 'atom_value'? Does 'align' need to exist beyond the\n> lifetime of its 'atom_value'? If not, making it a direct member might\n> simplify resource management (no need to free it).\n>\n>> + void (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);\n>> unsigned long ul; /* used for sorting when not FIELD_STR */\n>> };\n>>\n>> @@ -632,6 +644,84 @@ static inline char *copy_advance(char *dst, const char *src)\n>> +static void append_atom(struct atom_value *v, struct ref_formatting_state *state)\n>> +{\n>> + /*\n>> + * Quote formatting is only done when the stack has a single\n>> + * element. Otherwise quote formatting is done on the\n>> + * element's entire output strbuf when the %(end) atom is\n>> + * encountered.\n>> + */\n>> + if (!state->stack->prev)\n>\n> With the disclaimer that I wasn't following the quoting discussion\n> closely: Is this condition going to be sufficient for all cases, such\n> as an %(if:) atom? That is, if you have:\n>\n> %(if:notempty)%(bloop)%(then) --option=%(bloop)%(end)\n>\n> isn't the intention that, %(bloop) within the %(then) section should\n> be quoted but not the literal \"--option=\"?\n>\n> The condition `!state->stack->prev' would be insufficient to handle\n> this if %(if:) pushes one or more states onto the stack, no? This\n> implies that you might want an explicit flag for enabling/disabling\n> quoting rather than relying upon the state of the stack, and that\n> individual atom handlers would control that flag.\n>\n\n> Or, am I misunderstanding due to not having followed the discussion closely?\n\nYes this wont be work for what you've said.\n\nTo sum up the discussion:\nWe didn't want atoms within the %(align)....%(end) to be quoted separately\nrather the whole aligned buffer to be quoted at the end.\n\nBut this does conflict with %(if)...%(end). So it makes sense to change it to\nonly have checks for %(align) atom being used.\n\nSo probably:\n\nstatic void append_atom(struct atom_value *v, struct\nref_formatting_state *state)\n{\n if (state->stack->at_end == align_handler)\n strbuf_addstr(&state->stack->output, v->s);\n else\n quote_formatting(&state->stack->output, v->s, state->quote_style);\n}\n\nand\n\nstatic void end_atom_handler(struct atom_value *atomv, struct\nref_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 align modifier atom. In that case we need to\n * perform quote formatting.\n */\n if (current->at_end == align_handler) {\n 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>\n>> + quote_formatting(&state->stack->output, v->s, state->quote_style);\n>> + else\n>> + strbuf_addstr(&state->stack->output, v->s);\n>> +}\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) {\n>> + quote_formatting(&s, current->output.buf, state->quote_style);\n>> + strbuf_reset(¤t->output);\n>> + strbuf_addbuf(¤t->output, &s);\n>\n> strbuf_swap() can replace the above two lines.\n>\n\nYes, this makes sense.\n\n>> + }\n>> + strbuf_release(&s);\n>> + pop_stack_element(&state->stack);\n>> +}\n>> +\n>> /*\n>> * Parse the object referred by ref, and grab needed value.\n>> */\n>> @@ -725,6 +818,37 @@ static void populate_value(struct ref_array_item *ref)\n>> else\n>> v->s = \" \";\n>> continue;\n>> + } else if (!strcmp(name, \"align\"))\n>> + die(_(\"format: incomplete use of the `align` atom\"));\n>\n> Why does %(align) get flagged as a malformation of %(align:), whereas\n> %(color) does not get flagged as a malformation of %(color:)? Why does\n> one deserve special treatment but not the other?\n>\n\nDidn't see that, I think its needed to add a check for both like :\n\nelse if (!strcmp(name, \"align\") || !strcmp(name, \"color\"))\n die(_(\"format: improper usage of %s atom\"), name);\n\nI had a look if any other atoms need a subvalue to operate, couldn't\nfind any.\n\n>> + else if (skip_prefix(name, \"align:\", &valp)) {\n>> + struct align *align = xmalloc(sizeof(struct align));\n>> + struct strbuf **s = strbuf_split_str(valp, ',', 0);\n>> +\n>> + /* If the position is given trim the ',' from the first strbuf */\n>> + if (s[1])\n>> + strbuf_remove(s[0], s[0]->len - 1, 1);\n>\n> This is a truncation operation, which may be more idiomatically stated as:\n>\n> strbuf_setlen(s[0], s[0]->len - 1);\n>\n\nWill do.\n\n>> +\n>> + if (strtoul_ui(s[0]->buf, 10, &align->width))\n>> + die(_(\"positive width expected align:%s\"), s[0]->buf);\n>> +\n>> + /* If no position is given, default to ALIGN_LEFT */\n>> + if (!s[1] || !strcmp(s[1]->buf, \"left\"))\n>> + align->position = ALIGN_LEFT;\n>\n> If you structured the code like this:\n>\n> if (!s[1])\n> align->position = ALIGN_LEFT;\n> else if (!strcmp(s[1]->buf, \"left\"))\n> align->position = ALIGN_LEFT;\n> else if ...\n>\n> then the comment about \"default\" would become unnecessary, and it\n> would be easier to change the default in the future (if the need ever\n> arose), however, this is a very minor point, and I don't care\n> strongly.\n>\n\nSince there are other changes to be made, ill add this along.\n\n>> + else if (!strcmp(s[1]->buf, \"right\"))\n>> + align->position = ALIGN_RIGHT;\n>> + else if (!strcmp(s[1]->buf, \"middle\"))\n>> + align->position = ALIGN_MIDDLE;\n>> + else\n>> + die(_(\"improper format entered align:%s\"), s[1]->buf);\n>> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\n>> index 505a360..cef7a41 100755\n>> --- a/t/t6302-for-each-ref-filter.sh\n>> +++ b/t/t6302-for-each-ref-filter.sh\n>> @@ -81,4 +81,89 @@ test_expect_success 'filtering with --contains' '\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>> + cat >expect <<-EOF &&\n>> + refname is ${sq} ${sq}\\\\${sq}${sq}master${sq}\\\\${sq}${sq} ${sq}|\n>> + refname is ${sq} ${sq}\\\\${sq}${sq}side${sq}\\\\${sq}${sq} ${sq}|\n>> + refname is ${sq} ${sq}\\\\${sq}${sq}odd/spot${sq}\\\\${sq}${sq} ${sq}|\n>> + refname is ${sq} ${sq}\\\\${sq}${sq}double-tag${sq}\\\\${sq}${sq} ${sq}|\n>> + refname is ${sq} ${sq}\\\\${sq}${sq}four${sq}\\\\${sq}${sq} ${sq}|\n>> + refname is ${sq} ${sq}\\\\${sq}${sq}one${sq}\\\\${sq}${sq} ${sq}|\n>> + refname is ${sq} ${sq}\\\\${sq}${sq}signed-tag${sq}\\\\${sq}${sq} ${sq}|\n>> + refname is ${sq} ${sq}\\\\${sq}${sq}three${sq}\\\\${sq}${sq} ${sq}|\n>> + refname is ${sq} ${sq}\\\\${sq}${sq}two${sq}\\\\${sq}${sq} ${sq}|\n>> + EOF\n>> + git for-each-ref --shell --format=\"refname is %(align:30,middle)${sq}%(refname:short)${sq}%(end)|\" >actual &&\n>\n> This can be much less noisy if you change the second argument of\n> test_expect_success to use double-quote rather than single-quote. That\n> would allow you to use single-quote directly in the test and expected\n> output rather than having to use ${sq} indirection.\n>\n> test_expect_success 'alignment with format quote' \"\n> cat >expect <<-\\EOF &&\n> refname is ' '''master''' '|\n> ...\n> EOF\n> git for-each-ref --shell --format='refname ...\n> \\\\'%(rename:short)\\\\'...|'\n> \"\n>\n> or something.\n>\n\nWill check it out, thanks for the review.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268934","messageId":"CAOLa=ZRZ6RcRm6ERR=NVzng17qJx+1N8iGThuo=xQhEszJQ90w@mail.gmail.com","threadId":"40216","inReplyTo":"CAPig+cRHRPehkd+9PwOqpXkRUvaJa42zLtCKMEfv2W=ZJUZJzA@mail.gmail.com","subject":"Re: [PATCH v14 04/13] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-30T14:57:34Z","receivedAt":"2015-08-30T14:57:34Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sun, Aug 30, 2015 at 8:57 AM, Eric Sunshine wrote:\n>> Add documentation and tests for the same.\n>>\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> index 432cea0..21c8b5f 100644\n>> --- a/ref-filter.c\n>> +++ b/ref-filter.c\n>> @@ -53,6 +54,13 @@ static struct {\n>> { \"flag\" },\n>> { \"HEAD\" },\n>> { \"color\" },\n>> + { \"align\" },\n>> + { \"end\" },\n>> +};\n>> +\n>> +struct align {\n>> + align_type position;\n>> + unsigned int width;\n>> };\n>>\n>> #define REF_FORMATTING_STATE_INIT { 0, NULL }\n>> @@ -69,6 +79,8 @@ struct ref_formatting_state {\n>>\n>> struct atom_value {\n>> const char *s;\n>> + struct align *align;\n>\n> Why does 'align' need to be heap-allocated rather than just being a\n> direct member of 'atom_value'? Does 'align' need to exist beyond the\n> lifetime of its 'atom_value'? If not, making it a direct member might\n> simplify resource management (no need to free it).\n>\n\nBut it does, since we carry over the contents of align from atom_value to\ncb_data of ref_formatting_stack and that holds the value until we read\nthe %(end)\natom hence it seemed like a better choice to allocate it on the heap\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268938","messageId":"CAOLa=ZSgjrg60r6bdWfCHC+J9_q+ZWUHXc7j_9ePkdTBE1+-Kg@mail.gmail.com","threadId":"40216","inReplyTo":"CAPig+cQmAkFAjUcJNJNF2gwunX6TKK+oQsd2QnmJwxLT=EEvjQ@mail.gmail.com","subject":"Re: [PATCH v14 07/13] ref-filter: add support for %(contents:lines=X)","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-30T17:02:19Z","receivedAt":"2015-08-30T17:02:19Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sun, Aug 30, 2015 at 1:23 PM, Eric Sunshine wrote:\n> On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak wrote:\n>> In 'tag.c' we can print N lines from the annotation of the tag using\n>> the '-n' option. Copy code from 'tag.c' to 'ref-filter' and\n>> modify it to support appending of N lines from the annotation of tags\n>> to the given strbuf.\n>>\n>> Implement %(contents:lines=X) where X lines of the given object are\n>> obtained.\n>>\n>> Add documentation and test for the same.\n>>\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/builtin/tag.c b/builtin/tag.c\n>> index 471d6b1..0fc7557 100644\n>> --- a/builtin/tag.c\n>> +++ b/builtin/tag.c\n>> @@ -63,6 +64,11 @@ struct align {\n>> unsigned int width;\n>> };\n>>\n>> +struct contents {\n>> + unsigned int lines;\n>> + struct object_id oid;\n>> +};\n>> +\n>> #define REF_FORMATTING_STATE_INIT { 0, NULL }\n>>\n>> struct ref_formatting_stack {\n>> @@ -80,6 +86,7 @@ struct ref_formatting_state {\n>> struct atom_value {\n>> const char *s;\n>> struct align *align;\n>> + struct contents *contents;\n>\n> Same question as for 'align': Does 'contents' need to be\n> heap-allocated because it must exist beyond the lifetime of\n> 'atom_value'? If not, making it just a plain member of 'atom_value'\n> would simplify memory management (no need to free it).\n>\n\nIn this context that makes sense, as the value is only needed for the\ncurrent atom value.\n\n> Also, will 'align' and 'contents' ever be used at the same time? If\n> not, it might make sense to place them in a 'union' (not for the\n> memory saving, but to make it clear to the reader that their use is\n> mutually exclusive).\n>\n\nNot quite sure if it needs to be mutually exclusive (isn't that up to the user?)\nBut this can be done, as they're separate atoms and at a time only one of them\nis used.\n\n> More below.\n>\n>> void (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);\n>> unsigned long ul; /* used for sorting when not FIELD_STR */\n>> };\n>> @@ -569,6 +576,61 @@ static void find_subpos(const char *buf, unsigned long sz,\n>> *nonsiglen = *sig - buf;\n>> }\n>>\n>> +/*\n>> + * If 'lines' is greater than 0, append that many lines from the given\n>> + * object_id 'oid' to the given strbuf.\n>> + */\n>> +static void append_tag_lines(struct strbuf *out, const struct object_id *oid, int lines)\n>> +{\n>> + int i;\n>> + unsigned long size;\n>> + enum object_type type;\n>> + char *buf, *sp, *eol;\n>> + size_t len;\n>> +\n>> + buf = read_sha1_file(oid->hash, &type, &size);\n>> + if (!buf)\n>> + die_errno(\"unable to read object %s\", oid_to_hex(oid));\n>> + if (type != OBJ_COMMIT && type != OBJ_TAG)\n>> + goto free_return;\n>> + if (!size)\n>> + die(\"an empty %s object %s?\",\n>> + typename(type), oid_to_hex(oid));\n>> +\n>> + /* skip header */\n>> + sp = strstr(buf, \"\\n\\n\");\n>> + if (!sp)\n>> + goto free_return;\n>> +\n>> + /* only take up to \"lines\" lines, and strip the signature from a tag */\n>> + if (type == OBJ_TAG)\n>> + size = parse_signature(buf, size);\n>> + for (i = 0, sp += 2; i < lines && sp < buf + size; i++) {\n>> + if (i)\n>> + strbuf_addstr(out, \"\\n \");\n>> + eol = memchr(sp, '\\n', size - (sp - buf));\n>> + len = eol ? eol - sp : size - (sp - buf);\n>> + strbuf_add(out, sp, len);\n>> + if (!eol)\n>> + break;\n>> + sp = eol + 1;\n>> + }\n>> +free_return:\n>> + free(buf);\n>> +}\n>\n> I understand that you want to re-use this code from\n> tag.c:show_tag_lines(), but (from a very cursory read) isn't this\n> duplicating logic and processing already done elsewhere in\n> ref-filter.c? More about this below.\n>\n\nReplied below.\n\n>> +\n>> +static void contents_lines_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n>> +{\n>> + struct contents *contents = (struct contents *)atomv->contents;\n>\n> Why is this cast needed?\n>\n\nWill remove.\n\n>> + struct strbuf s = STRBUF_INIT;\n>> +\n>> + append_tag_lines(&s, &contents->oid, contents->lines);\n>> + quote_formatting(&state->stack->output, s.buf, state->quote_style);\n>> + strbuf_release(&s);\n>> +\n>> + free(contents);\n>> +}\n>> @@ -588,7 +651,8 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj\n>> strcmp(name, \"contents\") &&\n>> strcmp(name, \"contents:subject\") &&\n>> strcmp(name, \"contents:body\") &&\n>> - strcmp(name, \"contents:signature\"))\n>> + strcmp(name, \"contents:signature\") &&\n>> + !starts_with(name, \"contents:lines=\"))\n>> continue;\n>> if (!subpos)\n>> find_subpos(buf, sz,\n>> @@ -608,6 +672,15 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj\n>> v->s = xmemdupz(sigpos, siglen);\n>> else if (!strcmp(name, \"contents\"))\n>> v->s = xstrdup(subpos);\n>> + else if (skip_prefix(name, \"contents:lines=\", &valp)) {\n>> + struct contents *contents = xmalloc(sizeof(struct contents));\n>> +\n>> + if (strtoul_ui(valp, 10, &contents->lines))\n>> + die(_(\"positive width expected align:%s\"), valp);\n>> + hashcpy(contents->oid.hash, obj->sha1);\n>\n> The logic in append_tag_lines() which was copied from\n> tag.c:show_tag_lines() goes through the effort of loading the object\n> and parsing it, but hasn't the object already been loaded and parsed\n> by the time you get to this point? Assuming I'm reading this\n> correctly, wouldn't it make more sense to take advantage of the work\n> already done loading and parsing the object rather than repeating it\n> all inside append_tag_lines()?\n>\n\nMakes sense, I'll work on this.\n\n>> + v->handler = contents_lines_handler;\n>> + v->contents = contents;\n>> + }\n>> }\n>> }\n>> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\n>> index cef7a41..0277498 100755\n>> --- a/t/t6302-for-each-ref-filter.sh\n>> +++ b/t/t6302-for-each-ref-filter.sh\n>> @@ -166,4 +166,20 @@ test_expect_success 'nested alignment' '\n>> test_cmp expect actual\n>> '\n>>\n>> +test_expect_success 'check `%(contents:lines=X)`' '\n>> + cat >expect <<-\\EOF &&\n>> + master three\n>> + side four\n>> + odd/spot three\n>> + double-tag Annonated doubly\n>> + four four\n>> + one one\n>> + signed-tag A signed tag message\n>> + three three\n>> + two two\n>> + EOF\n>> + git for-each-ref --format=\"%(refname:short) %(contents:lines=1)\" >actual &&\n>\n> Maybe also test some edge cases, such as line=0, lines=-1 (an invalid\n> value), lines=2, lines=9999999 (a value larger than the number of\n> lines in any object).\n>\n\nWill do. Thanks for the review.\n\n--\nRegards,\nKarthik Nayak\n"},{"id":"268939","messageId":"CAPig+cROyc7vxddkfGSsBayT9i=VSacxcEuuNkEFf+oYoUs9kg@mail.gmail.com","threadId":"40216","inReplyTo":"CAOLa=ZSgjrg60r6bdWfCHC+J9_q+ZWUHXc7j_9ePkdTBE1+-Kg@mail.gmail.com","subject":"Re: [PATCH v14 07/13] ref-filter: add support for %(contents:lines=X)","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-30T17:09:23Z","receivedAt":"2015-08-30T17:09:23Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Aug 30, 2015 at 1:02 PM, Karthik Nayak wrote:\n> On Sun, Aug 30, 2015 at 1:23 PM, Eric Sunshine wrote:\n>> On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak wrote:\n>>> struct atom_value {\n>>> const char *s;\n>>> struct align *align;\n>>> + struct contents *contents;\n>>\n>> Same question as for 'align': Does 'contents' need to be\n>> heap-allocated because it must exist beyond the lifetime of\n>> 'atom_value'? If not, making it just a plain member of 'atom_value'\n>> would simplify memory management (no need to free it).\n>\n> In this context that makes sense, as the value is only needed for the\n> current atom value.\n>\n>> Also, will 'align' and 'contents' ever be used at the same time? If\n>> not, it might make sense to place them in a 'union' (not for the\n>> memory saving, but to make it clear to the reader that their use is\n>> mutually exclusive).\n>\n> Not quite sure if it needs to be mutually exclusive (isn't that up to the user?)\n> But this can be done, as they're separate atoms and at a time only one of them\n> is used.\n\nI meant \"mutually exclusive\" in the sense of only one or the other of\n'align' and 'contents' ever being used within a single 'atom_value'\ninstance. (I wasn't referring to the user experience.)\n"},{"id":"268940","messageId":"CAOLa=ZQn89f+Az56hwxu7X81VoifWs6OFxCC=G1PZ+c-dJWcRw@mail.gmail.com","threadId":"40216","inReplyTo":"CAPig+cROyc7vxddkfGSsBayT9i=VSacxcEuuNkEFf+oYoUs9kg@mail.gmail.com","subject":"Re: [PATCH v14 07/13] ref-filter: add support for %(contents:lines=X)","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-30T17:17:13Z","receivedAt":"2015-08-30T17:17:13Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sun, Aug 30, 2015 at 10:39 PM, Eric Sunshine wrote:\n>\n> I meant \"mutually exclusive\" in the sense of only one or the other of\n> 'align' and 'contents' ever being used within a single 'atom_value'\n> instance. (I wasn't referring to the user experience.)\n\nSorry for the confusion, we're on the same page :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268941","messageId":"xmqq8u8sznyo.fsf@gitster.mtv.corp.google.com","threadId":"40216","inReplyTo":"CAPig+cRHRPehkd+9PwOqpXkRUvaJa42zLtCKMEfv2W=ZJUZJzA@mail.gmail.com","subject":"Re: [PATCH v14 04/13] ref-filter: implement an `align` atom","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-30T17:27:11Z","receivedAt":"2015-08-30T17:27:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> With the disclaimer that I wasn't following the quoting discussion\n> closely: Is this condition going to be sufficient for all cases, such\n> as an %(if:) atom? That is, if you have:\n>\n> %(if:notempty)%(bloop)%(then) --option=%(bloop)%(end)\n>\n> isn't the intention that, %(bloop) within the %(then) section should\n> be quoted but not the literal \"--option=\"?\n\nI think you'll see that the intention of the above is to quote the\nentirty of the result of %(if...)...%(end) if you read the previous\ndiscussion. The \"quoting\" is used when you say you are making --format\nwrite a script in specified programming language, e.g.\n\n\tfor-each-ref --shell --format='\n \ta=%(atom) b=%(if...)...%(end)\n\t\tdo interesting things using $a and $b here\n\t' | sh\n\nYou are correct to point out in the earlier part of your message I\nam responding to that %(align) is not special and any nested thing\nincluding %(if) will uniformly trigger the same \"usually each atom\nis quoted separately, but with this opening atom, everything up to\nthe matching end atom is evaluated first and then the result is\nquoted\" logic.\n"},{"id":"268952","messageId":"CAPig+cRUJa3hRk4utH6A8iHv6y2=XukTkiLWFet4dkeEw1sLzA@mail.gmail.com","threadId":"40216","inReplyTo":"CAOLa=ZRZ6RcRm6ERR=NVzng17qJx+1N8iGThuo=xQhEszJQ90w@mail.gmail.com","subject":"Re: [PATCH v14 04/13] ref-filter: implement an `align` atom","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-30T21:59:16Z","receivedAt":"2015-08-30T21:59:16Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Aug 30, 2015 at 10:57 AM, Karthik Nayak wrote:\n> On Sun, Aug 30, 2015 at 8:57 AM, Eric Sunshine wrote:\n>>> +struct align {\n>>> + align_type position;\n>>> + unsigned int width;\n>>> };\n>>>\n>>> #define REF_FORMATTING_STATE_INIT { 0, NULL }\n>>> @@ -69,6 +79,8 @@ struct ref_formatting_state {\n>>>\n>>> struct atom_value {\n>>> const char *s;\n>>> + struct align *align;\n>>\n>> Why does 'align' need to be heap-allocated rather than just being a\n>> direct member of 'atom_value'? Does 'align' need to exist beyond the\n>> lifetime of its 'atom_value'? If not, making it a direct member might\n>> simplify resource management (no need to free it).\n>\n> But it does, since we carry over the contents of align from atom_value to\n> cb_data of ref_formatting_stack and that holds the value until we read\n> the %(end)\n> atom hence it seemed like a better choice to allocate it on the heap\n\nSo, you're saying that the 'atom_value' instance no longer exists at\nthe point that processing of %(end) needs to access the alignment\nproperties? If so, then heap allocation make sense. Thanks.\n"},{"id":"268953","messageId":"CAPig+cQF=1FPFP_7Bn2m2J+ay5ZupG25USWc9LT9SqO1=VDZvg@mail.gmail.com","threadId":"40216","inReplyTo":"CAOLa=ZTHtps2gJb9asSzG_4NOwx_xiA7OuCzjW=BTTFfkaYoQg@mail.gmail.com","subject":"Re: [PATCH v14 04/13] ref-filter: implement an `align` atom","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-30T22:10:37Z","receivedAt":"2015-08-30T22:10:37Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Aug 30, 2015 at 9:38 AM, Karthik Nayak wrote:\n> On Sun, Aug 30, 2015 at 8:57 AM, Eric Sunshine wrote:\n>> On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak wrote:\n>>> +static void append_atom(struct atom_value *v, struct ref_formatting_state *state)\n>>> +{\n>>> + /*\n>>> + * Quote formatting is only done when the stack has a single\n>>> + * element. Otherwise quote formatting is done on the\n>>> + * element's entire output strbuf when the %(end) atom is\n>>> + * encountered.\n>>> + */\n>>> + if (!state->stack->prev)\n>>\n>> With the disclaimer that I wasn't following the quoting discussion\n>> closely: Is this condition going to be sufficient for all cases, such\n>> as an %(if:) atom? That is, if you have:\n>>\n>> %(if:notempty)%(bloop)%(then) --option=%(bloop)%(end)\n>>\n>> isn't the intention that, %(bloop) within the %(then) section should\n>> be quoted but not the literal \"--option=\"?\n>>\n>> The condition `!state->stack->prev' would be insufficient to handle\n>> this if %(if:) pushes one or more states onto the stack, no? This\n>> implies that you might want an explicit flag for enabling/disabling\n>> quoting rather than relying upon the state of the stack, and that\n>> individual atom handlers would control that flag.\n>\n>> Or, am I misunderstanding due to not having followed the discussion closely?\n>\n> Yes this wont be work for what you've said.\n>\n> To sum up the discussion:\n> We didn't want atoms within the %(align)....%(end) to be quoted separately\n> rather the whole aligned buffer to be quoted at the end.\n>\n> But this does conflict with %(if)...%(end). So it makes sense to change it to\n> only have checks for %(align) atom being used.\n\nIf I understand Junio's response correctly, then this doesn't sound\ncorrect either. Rather than imbuing only %(align:) with special\nquoting knowledge, it sounds like quoting should be handled\ngenerically for all top-level atoms and pseudo-atoms, including %(if:)\nand others which may come later.\n\n(But perhaps I'm still misunderstanding...)\n\n> So probably:\n>\n> static void append_atom(struct atom_value *v, struct\n> ref_formatting_state *state)\n> {\n> if (state->stack->at_end == align_handler)\n\nThis couples append_atom() far too tightly with the %(align:) atom. If\nyou really need to do this sort of special-casing, then it probably\nwould make more sense to have an explicit flag saying whether or not\nquoting should be done, rather than tying it specifically to the\n%(align:) atom.\n\nHowever, (again, if I'm understanding Junio's response), your original\n`!state->stack->prev' condition might be sufficient after all.\n\n> strbuf_addstr(&state->stack->output, v->s);\n> else\n> quote_formatting(&state->stack->output, v->s, state->quote_style);\n> }\n>\n> and\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 align modifier atom. In that case we need to\n> * perform quote formatting.\n> */\n> if (current->at_end == align_handler) {\n\nDitto about being too tightly coupled to %(align:). Such logic should\nlikely be generic for any such atom.\n\n> 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>>> @@ -725,6 +818,37 @@ static void populate_value(struct ref_array_item *ref)\n>>> else\n>>> v->s = \" \";\n>>> continue;\n>>> + } else if (!strcmp(name, \"align\"))\n>>> + die(_(\"format: incomplete use of the `align` atom\"));\n>>\n>> Why does %(align) get flagged as a malformation of %(align:), whereas\n>> %(color) does not get flagged as a malformation of %(color:)? Why does\n>> one deserve special treatment but not the other?\n>\n> Didn't see that, I think its needed to add a check for both like :\n>\n> else if (!strcmp(name, \"align\") || !strcmp(name, \"color\"))\n> die(_(\"format: improper usage of %s atom\"), name);\n>\n> I had a look if any other atoms need a subvalue to operate, couldn't\n> find any.\n\nHmm, I'm not convinced that either %(align) or %(color) need to be\ncalled out specially. What is the current behavior when these\n\"malformations\" or any other misspelled atoms are used? Does it error\nout? Does it simply ignore them and pass them through to the output\nunmolested?\n"},{"id":"268954","messageId":"CAPig+cTYQmrFnf7p6zxNh9w6AKXth99nRu40chQtUYAkWVss=w@mail.gmail.com","threadId":"40216","inReplyTo":"1440857537-13968-8-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v14 07/13] ref-filter: add support for %(contents:lines=X)","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-30T22:13:35Z","receivedAt":"2015-08-30T22:13:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak wrote:\n> In 'tag.c' we can print N lines from the annotation of the tag using\n> the '-n' option. Copy code from 'tag.c' to 'ref-filter' and\n> modify it to support appending of N lines from the annotation of tags\n> to the given strbuf.\n>\n> Implement %(contents:lines=X) where X lines of the given object are\n> obtained.\n>\n> Add documentation and test for the same.\n>\n> Signed-off-by: Karthik Nayak \n> ---\n> @@ -608,6 +672,15 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj\n> v->s = xmemdupz(sigpos, siglen);\n> else if (!strcmp(name, \"contents\"))\n> v->s = xstrdup(subpos);\n> + else if (skip_prefix(name, \"contents:lines=\", &valp)) {\n> + struct contents *contents = xmalloc(sizeof(struct contents));\n> +\n> + if (strtoul_ui(valp, 10, &contents->lines))\n> + die(_(\"positive width expected align:%s\"), valp);\n\nI forgot to mention this when I reviewed the patch earlier[1], but you\ncopied this error message a bit too literally from the %(align:) atom.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/276807\n\n> + hashcpy(contents->oid.hash, obj->sha1);\n> + v->handler = contents_lines_handler;\n> + v->contents = contents;\n> + }\n> }\n> }\n"},{"id":"268955","messageId":"CAPig+cTqmku5DGm9g1VN8s5sBgkjZTBLyGrFGjU2J099QA32wg@mail.gmail.com","threadId":"40216","inReplyTo":"xmqq8u8sznyo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v14 04/13] ref-filter: implement an `align` atom","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-30T22:56:14Z","receivedAt":"2015-08-30T22:56:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Aug 30, 2015 at 1:27 PM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>> With the disclaimer that I wasn't following the quoting discussion\n>> closely: Is this condition going to be sufficient for all cases, such\n>> as an %(if:) atom? That is, if you have:\n>>\n>> %(if:notempty)%(bloop)%(then) --option=%(bloop)%(end)\n>>\n>> isn't the intention that, %(bloop) within the %(then) section should\n>> be quoted but not the literal \"--option=\"?\n>\n> I think you'll see that the intention of the above is to quote the\n> entirty of the result of %(if...)...%(end) if you read the previous\n> discussion. The \"quoting\" is used when you say you are making --format\n> write a script in specified programming language, e.g.\n>\n> for-each-ref --shell --format='\n> a=%(atom) b=%(if...)...%(end)\n> do interesting things using $a and $b here\n> ' | sh\n>\n> You are correct to point out in the earlier part of your message I\n> am responding to that %(align) is not special and any nested thing\n> including %(if) will uniformly trigger the same \"usually each atom\n> is quoted separately, but with this opening atom, everything up to\n> the matching end atom is evaluated first and then the result is\n> quoted\" logic.\n\nSo, if I'm understanding correctly, the semantic behavior of the\ncurrent patch seems to be more or less correct, but the implementation\n(and commit message) place perhaps too much emphasis on specializing\nquoting suppression only for %(align:), whereas it could/should be\ngeneralized?\n\nI am a bit concerned about this code from end_atom_handler():\n\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) {\n quote_formatting(&s, current->output.buf, state->quote_style);\n strbuf_reset(¤t->output);\n strbuf_addbuf(¤t->output, &s);\n }\n\nAren't both the comment and the condition backward? Shouldn't quoting\nbe done only for the top-most state on the stack rather than every\nstate other than the top-most? That is, shouldn't the condition be\n`!state->stack->prev' as it is in append_atom()?\n"},{"id":"268957","messageId":"CAOLa=ZTb0-eymVhvO5aqh94MptsAAbe=XSCR2TGG-Yea2cYu8g@mail.gmail.com","threadId":"40216","inReplyTo":"CAPig+cTYQmrFnf7p6zxNh9w6AKXth99nRu40chQtUYAkWVss=w@mail.gmail.com","subject":"Re: [PATCH v14 07/13] ref-filter: add support for %(contents:lines=X)","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-31T04:43:44Z","receivedAt":"2015-08-31T04:43:44Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sun, Aug 30, 2015 at 3:13 PM, Eric Sunshine wrote:\n> On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak wrote:\n>> In 'tag.c' we can print N lines from the annotation of the tag using\n>> the '-n' option. Copy code from 'tag.c' to 'ref-filter' and\n>> modify it to support appending of N lines from the annotation of tags\n>> to the given strbuf.\n>>\n>> Implement %(contents:lines=X) where X lines of the given object are\n>> obtained.\n>>\n>> Add documentation and test for the same.\n>>\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> @@ -608,6 +672,15 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj\n>> v->s = xmemdupz(sigpos, siglen);\n>> else if (!strcmp(name, \"contents\"))\n>> v->s = xstrdup(subpos);\n>> + else if (skip_prefix(name, \"contents:lines=\", &valp)) {\n>> + struct contents *contents = xmalloc(sizeof(struct contents));\n>> +\n>> + if (strtoul_ui(valp, 10, &contents->lines))\n>> + die(_(\"positive width expected align:%s\"), valp);\n>\n> I forgot to mention this when I reviewed the patch earlier[1], but you\n> copied this error message a bit too literally from the %(align:) atom.\n>\n> [1]: http://article.gmane.org/gmane.comp.version-control.git/276807\n>\n\nI fixed that with your other suggestions, should have mentioned it. Thanks\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268961","messageId":"vpqvbbwhrxp.fsf@anie.imag.fr","threadId":"40216","inReplyTo":"1440857537-13968-1-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v14 00/13] Port tag.c to use ref-filter.c","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-08-31T06:50:58Z","receivedAt":"2015-08-31T06:50: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> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\n> index 06d468e..1b48b95 100644\n> --- a/Documentation/git-for-each-ref.txt\n> +++ b/Documentation/git-for-each-ref.txt\n> @@ -149,6 +149,7 @@ Its first line is `contents:subject`, where subject is the concatenation\n> of all lines of the commit message up to the first blank line. The next\n> line is 'contents:body', where body is all of the lines after the first\n> blank line. Finally, the optional GPG signature is `contents:signature`.\n> +The first `N` lines of the object is obtained using `contents:lines=N`.\n\n\"Finally\" in the last line of the context is no longer accurate.\n\n> +test_expect_success 'check `%(contents:lines=X)`' '\n> +\tcat >expect <<-\\EOF &&\n> +\tmaster three\n> +\tside four\n> +\todd/spot three\n> +\tdouble-tag Annonated doubly\n> +\tfour four\n> +\tone one\n> +\tsigned-tag A signed tag message\n> +\tthree three\n> +\ttwo two\n> +\tEOF\n> +\tgit for-each-ref --format=\"%(refname:short) %(contents:lines=1)\" >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nNit: I would find it more readable with an actual separator (anything\nbut a space) between %(refname) and %(contents).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"268962","messageId":"vpqa8t7j4nf.fsf@anie.imag.fr","threadId":"40216","inReplyTo":"1440857537-13968-1-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v14 00/13] Port tag.c to use ref-filter.c","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-08-31T07:31:00Z","receivedAt":"2015-08-31T07:31:00Z","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> * We perform quoting on each layer of nested alignment. \n\nI do not understand why.\n\nFor example, using the tip of karthik/exp on GitHub (on top of this\nseries, d91419b (ref-filter: adopt get_head_description() from branch.c,\n2015-08-23)):\n\ngit for-each-ref --shell \\\n --format 'x=%(if)foo%(then)%(align:10)XXX%(end)%(else) not foo %(end)'\n\nI'd expect an output like:\n\nx='XXX '\n\nand instead I get:\n\nx=''\\''XXX '\\'''\n\nwhich assigns the value 'XXX ' (including the quotes) to $x. I do\nnot see a use-case for this (well, I could imagine one where we would\nlater call eval \"$x\", that seems rather far-fetched).\n\nI think the quoting should be:\n\n1) When the stack contains only the initial element, quote individual\n atoms.\n\n2) When the stack contains exactly two elements and encountering a %(end)\n or %(else), quote the entire strbuf of the 2nd level when appending to\n the 1st level.\n\n3) When the stack contains more than two elements, perform no quoting at\n all. The quoting will be done later by #2.\n\nI found a segfault while testing:\n\n$ git for-each-ref --format 'x=%(if)%(align:10)%(end)%(then)%(align:10)XXX%(end)%(else)%(end)' --shell\nzsh: segmentation fault\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"268967","messageId":"vpq37yzhncc.fsf@anie.imag.fr","threadId":"40216","inReplyTo":"1440857537-13968-5-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v14 04/13] ref-filter: implement an `align` atom","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-08-31T08:30:11Z","receivedAt":"2015-08-31T08:30:11Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Hi,\n\nJust a remark on the way iterations are going on with this series: I do\nagree that each version gets better than the previous one, which is\ngood. However, I have the feeling that we're turning a simple and easy\nto review series into a monster one (reading \"v14 .../13\" with a\nnon-trivial interdiff is a bit scary for reviewers).\n\nKarthik: I think you could (should?) have splitted the work again.\nYou're integrating other people's idea in the series, and sometimes I\nthink at some point, a better way would have been: \"OK, good idea, I'll\nimplement it in on top of this series\" (and possibly implement it on top\nbefore you resend, to make sure that the series is ready to welcome the\nnew feature). For example, %(contents:lines=X) is good, but could have\nwaited for the next series IMHO. This way, you get a shorter series to\nconverge faster (straightforward interdiff for the last iterations), and\nthen reviewers can focus on the next, short, series.\n\nThe opposite is Zeno paradox kind of series, where you add something new\nevery time you get close to getting merged, and you actually never reach\na stable state ;-).\n\nThat said, this particular series was a tough one for this, so I'm not\neven sure my advice would have been applicable ^^.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"268973","messageId":"CAOLa=ZS53BNNvnYv2TU6xQbEihi1GiKBaEEZ=KT6p_gDBR9Y9Q@mail.gmail.com","threadId":"40216","inReplyTo":"CAPig+cQF=1FPFP_7Bn2m2J+ay5ZupG25USWc9LT9SqO1=VDZvg@mail.gmail.com","subject":"Re: [PATCH v14 04/13] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-31T09:55:58Z","receivedAt":"2015-08-31T09:55:58Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sun, Aug 30, 2015 at 3:10 PM, Eric Sunshine wrote:\n> On Sun, Aug 30, 2015 at 9:38 AM, Karthik Nayak wrote:\n>> On Sun, Aug 30, 2015 at 8:57 AM, Eric Sunshine wrote:\n>>> On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak wrote:\n>>>> +static void append_atom(struct atom_value *v, struct ref_formatting_state *state)\n>>>> +{\n>>>> + /*\n>>>> + * Quote formatting is only done when the stack has a single\n>>>> + * element. Otherwise quote formatting is done on the\n>>>> + * element's entire output strbuf when the %(end) atom is\n>>>> + * encountered.\n>>>> + */\n>>>> + if (!state->stack->prev)\n>>>\n>>> With the disclaimer that I wasn't following the quoting discussion\n>>> closely: Is this condition going to be sufficient for all cases, such\n>>> as an %(if:) atom? That is, if you have:\n>>>\n>>> %(if:notempty)%(bloop)%(then) --option=%(bloop)%(end)\n>>>\n>>> isn't the intention that, %(bloop) within the %(then) section should\n>>> be quoted but not the literal \"--option=\"?\n>>>\n>>> The condition `!state->stack->prev' would be insufficient to handle\n>>> this if %(if:) pushes one or more states onto the stack, no? This\n>>> implies that you might want an explicit flag for enabling/disabling\n>>> quoting rather than relying upon the state of the stack, and that\n>>> individual atom handlers would control that flag.\n>>\n>>> Or, am I misunderstanding due to not having followed the discussion closely?\n>>\n>> Yes this wont be work for what you've said.\n>>\n>> To sum up the discussion:\n>> We didn't want atoms within the %(align)....%(end) to be quoted separately\n>> rather the whole aligned buffer to be quoted at the end.\n>>\n>> But this does conflict with %(if)...%(end). So it makes sense to change it to\n>> only have checks for %(align) atom being used.\n>\n> If I understand Junio's response correctly, then this doesn't sound\n> correct either. Rather than imbuing only %(align:) with special\n> quoting knowledge, it sounds like quoting should be handled\n> generically for all top-level atoms and pseudo-atoms, including %(if:)\n> and others which may come later.\n>\n> (But perhaps I'm still misunderstanding...)\n>\n\nI was sure about how %(align)...%(end) was to be quoted at the end.\nFrom Junio's comment it seems all top level atoms need to be quoted at the end\nso with regards to this, the new changes aren't needed and the old\nchanges will hold.\n\n>> So probably:\n>>\n>> static void append_atom(struct atom_value *v, struct\n>> ref_formatting_state *state)\n>> {\n>> if (state->stack->at_end == align_handler)\n>\n> This couples append_atom() far too tightly with the %(align:) atom. If\n> you really need to do this sort of special-casing, then it probably\n> would make more sense to have an explicit flag saying whether or not\n> quoting should be done, rather than tying it specifically to the\n> %(align:) atom.\n>\n> However, (again, if I'm understanding Junio's response), your original\n> `!state->stack->prev' condition might be sufficient after all.\n>\n\nsame as above.\n\n>> strbuf_addstr(&state->stack->output, v->s);\n>> else\n>> quote_formatting(&state->stack->output, v->s, state->quote_style);\n>> }\n>>\n>> and\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 align modifier atom. In that case we need to\n>> * perform quote formatting.\n>> */\n>> if (current->at_end == align_handler) {\n>\n> Ditto about being too tightly coupled to %(align:). Such logic should\n> likely be generic for any such atom.\n>\n\nShouldn't be needed, was mistaken, sticking to the old logic.\n\n>> 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>>>> @@ -725,6 +818,37 @@ static void populate_value(struct ref_array_item *ref)\n>>>> else\n>>>> v->s = \" \";\n>>>> continue;\n>>>> + } else if (!strcmp(name, \"align\"))\n>>>> + die(_(\"format: incomplete use of the `align` atom\"));\n>>>\n>>> Why does %(align) get flagged as a malformation of %(align:), whereas\n>>> %(color) does not get flagged as a malformation of %(color:)? Why does\n>>> one deserve special treatment but not the other?\n>>\n>> Didn't see that, I think its needed to add a check for both like :\n>>\n>> else if (!strcmp(name, \"align\") || !strcmp(name, \"color\"))\n>> die(_(\"format: improper usage of %s atom\"), name);\n>>\n>> I had a look if any other atoms need a subvalue to operate, couldn't\n>> find any.\n>\n> Hmm, I'm not convinced that either %(align) or %(color) need to be\n> called out specially. What is the current behavior when these\n> \"malformations\" or any other misspelled atoms are used? Does it error\n> out? Does it simply ignore them and pass them through to the output\n> unmolested?\n\nIt just simply ignores them currently, which is kinda bad, as the user\nis given no\nwarning, and the atom is ineffective.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268976","messageId":"CAOLa=ZSOqHxpDRXMgANxqtMC5BzxBaK=uX3b7Yqxbu6ZNqokCQ@mail.gmail.com","threadId":"40216","inReplyTo":"CAPig+cRUJa3hRk4utH6A8iHv6y2=XukTkiLWFet4dkeEw1sLzA@mail.gmail.com","subject":"Re: [PATCH v14 04/13] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-31T10:06:13Z","receivedAt":"2015-08-31T10:06:13Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Mon, Aug 31, 2015 at 3:29 AM, Eric Sunshine wrote:\n> On Sun, Aug 30, 2015 at 10:57 AM, Karthik Nayak wrote:\n>> On Sun, Aug 30, 2015 at 8:57 AM, Eric Sunshine wrote:\n>>>> +struct align {\n>>>> + align_type position;\n>>>> + unsigned int width;\n>>>> };\n>>>>\n>>>> #define REF_FORMATTING_STATE_INIT { 0, NULL }\n>>>> @@ -69,6 +79,8 @@ struct ref_formatting_state {\n>>>>\n>>>> struct atom_value {\n>>>> const char *s;\n>>>> + struct align *align;\n>>>\n>>> Why does 'align' need to be heap-allocated rather than just being a\n>>> direct member of 'atom_value'? Does 'align' need to exist beyond the\n>>> lifetime of its 'atom_value'? If not, making it a direct member might\n>>> simplify resource management (no need to free it).\n>>\n>> But it does, since we carry over the contents of align from atom_value to\n>> cb_data of ref_formatting_stack and that holds the value until we read\n>> the %(end)\n>> atom hence it seemed like a better choice to allocate it on the heap\n>\n> So, you're saying that the 'atom_value' instance no longer exists at\n> the point that processing of %(end) needs to access the alignment\n> properties? If so, then heap allocation make sense. Thanks.\n\nI was actually wrong there, if you see populate_value() the ref is\nfilled with atoms\nwhich aren't really deallocated, hence the atom_value remains with the ref in\nref->value[atom]. where atom is obtained using parse_ref_filter_atom() hence it\nmakes sense to make it static. Thanks\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268979","messageId":"CAOLa=ZSaVttejcGKwq1jzd7deLi-A4=kaJWW0zW4XdYPrsodJg@mail.gmail.com","threadId":"40216","inReplyTo":"CAPig+cTqmku5DGm9g1VN8s5sBgkjZTBLyGrFGjU2J099QA32wg@mail.gmail.com","subject":"Re: [PATCH v14 04/13] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-31T10:14:56Z","receivedAt":"2015-08-31T10:14:56Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Mon, Aug 31, 2015 at 4:26 AM, Eric Sunshine wrote:\n> On Sun, Aug 30, 2015 at 1:27 PM, Junio C Hamano wrote:\n>> Eric Sunshine writes:\n>>> With the disclaimer that I wasn't following the quoting discussion\n>>> closely: Is this condition going to be sufficient for all cases, such\n>>> as an %(if:) atom? That is, if you have:\n>>>\n>>> %(if:notempty)%(bloop)%(then) --option=%(bloop)%(end)\n>>>\n>>> isn't the intention that, %(bloop) within the %(then) section should\n>>> be quoted but not the literal \"--option=\"?\n>>\n>> I think you'll see that the intention of the above is to quote the\n>> entirty of the result of %(if...)...%(end) if you read the previous\n>> discussion. The \"quoting\" is used when you say you are making --format\n>> write a script in specified programming language, e.g.\n>>\n>> for-each-ref --shell --format='\n>> a=%(atom) b=%(if...)...%(end)\n>> do interesting things using $a and $b here\n>> ' | sh\n>>\n>> You are correct to point out in the earlier part of your message I\n>> am responding to that %(align) is not special and any nested thing\n>> including %(if) will uniformly trigger the same \"usually each atom\n>> is quoted separately, but with this opening atom, everything up to\n>> the matching end atom is evaluated first and then the result is\n>> quoted\" logic.\n>\n> So, if I'm understanding correctly, the semantic behavior of the\n> current patch seems to be more or less correct, but the implementation\n> (and commit message) place perhaps too much emphasis on specializing\n> quoting suppression only for %(align:), whereas it could/should be\n> generalized?\n>\n> I am a bit concerned about this code from end_atom_handler():\n>\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) {\n> quote_formatting(&s, current->output.buf, state->quote_style);\n> strbuf_reset(¤t->output);\n> strbuf_addbuf(¤t->output, &s);\n> }\n>\n> Aren't both the comment and the condition backward? Shouldn't quoting\n> be done only for the top-most state on the stack rather than every\n> state other than the top-most? That is, shouldn't the condition be\n> `!state->stack->prev' as it is in append_atom()?\n\nAfter seeing the example of quote usage given by Junio, yes you're right.\n`!state->stack->prev` is the way to go.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268982","messageId":"CAOLa=ZQ2=y88rqUuNgPvZvK+sMqbkCuC0hPUoB=5Ku6Wwk_e6g@mail.gmail.com","threadId":"40216","inReplyTo":"CAOLa=ZSaVttejcGKwq1jzd7deLi-A4=kaJWW0zW4XdYPrsodJg@mail.gmail.com","subject":"Re: [PATCH v14 04/13] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-31T10:28:35Z","receivedAt":"2015-08-31T10:28:35Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Mon, Aug 31, 2015 at 3:44 PM, Karthik Nayak wrote:\n> On Mon, Aug 31, 2015 at 4:26 AM, Eric Sunshine wrote:\n>> On Sun, Aug 30, 2015 at 1:27 PM, Junio C Hamano wrote:\n>>> Eric Sunshine writes:\n>>>> With the disclaimer that I wasn't following the quoting discussion\n>>>> closely: Is this condition going to be sufficient for all cases, such\n>>>> as an %(if:) atom? That is, if you have:\n>>>>\n>>>> %(if:notempty)%(bloop)%(then) --option=%(bloop)%(end)\n>>>>\n>>>> isn't the intention that, %(bloop) within the %(then) section should\n>>>> be quoted but not the literal \"--option=\"?\n>>>\n>>> I think you'll see that the intention of the above is to quote the\n>>> entirty of the result of %(if...)...%(end) if you read the previous\n>>> discussion. The \"quoting\" is used when you say you are making --format\n>>> write a script in specified programming language, e.g.\n>>>\n>>> for-each-ref --shell --format='\n>>> a=%(atom) b=%(if...)...%(end)\n>>> do interesting things using $a and $b here\n>>> ' | sh\n>>>\n>>> You are correct to point out in the earlier part of your message I\n>>> am responding to that %(align) is not special and any nested thing\n>>> including %(if) will uniformly trigger the same \"usually each atom\n>>> is quoted separately, but with this opening atom, everything up to\n>>> the matching end atom is evaluated first and then the result is\n>>> quoted\" logic.\n>>\n>> So, if I'm understanding correctly, the semantic behavior of the\n>> current patch seems to be more or less correct, but the implementation\n>> (and commit message) place perhaps too much emphasis on specializing\n>> quoting suppression only for %(align:), whereas it could/should be\n>> generalized?\n>>\n>> I am a bit concerned about this code from end_atom_handler():\n>>\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) {\n>> quote_formatting(&s, current->output.buf, state->quote_style);\n>> strbuf_reset(¤t->output);\n>> strbuf_addbuf(¤t->output, &s);\n>> }\n>>\n>> Aren't both the comment and the condition backward? Shouldn't quoting\n>> be done only for the top-most state on the stack rather than every\n>> state other than the top-most? That is, shouldn't the condition be\n>> `!state->stack->prev' as it is in append_atom()?\n>\n> After seeing the example of quote usage given by Junio, yes you're right.\n> `!state->stack->prev` is the way to go.\n>\n\nAlso I think you mean `!state->stack->prev->prev` as we push a new\nelement on the stack when an atom such as %(align) is encountered. And\nthis quoting done at the end should be only for such atoms. Hence it should be\n`!state->stack->prev->prev` and not `!state->stack->prev`.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268984","messageId":"CAOLa=ZR+TtqJsHG11P7L3v0S1T0cddFe9AoE8_ipfyaqs_B+aA@mail.gmail.com","threadId":"40216","inReplyTo":"vpq37yzhncc.fsf@anie.imag.fr","subject":"Re: [PATCH v14 04/13] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-31T10:59:52Z","receivedAt":"2015-08-31T10:59:52Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Mon, Aug 31, 2015 at 2:00 PM, Matthieu Moy\n wrote:\n> Hi,\n>\n> Just a remark on the way iterations are going on with this series: I do\n> agree that each version gets better than the previous one, which is\n> good. However, I have the feeling that we're turning a simple and easy\n> to review series into a monster one (reading \"v14 .../13\" with a\n> non-trivial interdiff is a bit scary for reviewers).\n>\n> Karthik: I think you could (should?) have splitted the work again.\n> You're integrating other people's idea in the series, and sometimes I\n> think at some point, a better way would have been: \"OK, good idea, I'll\n> implement it in on top of this series\" (and possibly implement it on top\n> before you resend, to make sure that the series is ready to welcome the\n> new feature). For example, %(contents:lines=X) is good, but could have\n> waited for the next series IMHO. This way, you get a shorter series to\n> converge faster (straightforward interdiff for the last iterations), and\n> then reviewers can focus on the next, short, series.\n>\n> The opposite is Zeno paradox kind of series, where you add something new\n> every time you get close to getting merged, and you actually never reach\n> a stable state ;-).\n>\n> That said, this particular series was a tough one for this, so I'm not\n> even sure my advice would have been applicable ^^.\n>\n\n\nWhat you're saying also makes sense, This series has been changing\nbased on the wonderful suggestions given by everyone on the list.\n\nIt has changed course and that's in a good way. But maybe you're right\nI'll probably stop it with the next iteration, which I've already worked on\nbased on Eric's various suggestion.\n\nSo I'll push that soon, and more changes can be then added to the top of the\nseries.\n\nThanks for the suggestion.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268985","messageId":"CAOLa=ZQyjfkbqMR6jSujVng-h=ezK0qPqwgJWynaEcvjeO3pSQ@mail.gmail.com","threadId":"40216","inReplyTo":"vpqvbbwhrxp.fsf@anie.imag.fr","subject":"Re: [PATCH v14 00/13] Port tag.c to use ref-filter.c","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-31T11:09:00Z","receivedAt":"2015-08-31T11:09:00Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Mon, Aug 31, 2015 at 12:20 PM, 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 06d468e..1b48b95 100644\n>> --- a/Documentation/git-for-each-ref.txt\n>> +++ b/Documentation/git-for-each-ref.txt\n>> @@ -149,6 +149,7 @@ Its first line is `contents:subject`, where subject is the concatenation\n>> of all lines of the commit message up to the first blank line. The next\n>> line is 'contents:body', where body is all of the lines after the first\n>> blank line. Finally, the optional GPG signature is `contents:signature`.\n>> +The first `N` lines of the object is obtained using `contents:lines=N`.\n>\n> \"Finally\" in the last line of the context is no longer accurate.\n>\n\nWill remove that.\n\n>> +test_expect_success 'check `%(contents:lines=X)`' '\n>> + cat >expect <<-\\EOF &&\n>> + master three\n>> + side four\n>> + odd/spot three\n>> + double-tag Annonated doubly\n>> + four four\n>> + one one\n>> + signed-tag A signed tag message\n>> + three three\n>> + two two\n>> + EOF\n>> + git for-each-ref --format=\"%(refname:short) %(contents:lines=1)\" >actual &&\n>> + test_cmp expect actual\n>> +'\n>\n> Nit: I would find it more readable with an actual separator (anything\n> but a space) between %(refname) and %(contents).\n>\n\nWill add.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268986","messageId":"CAOLa=ZRJC5FD08Ljh=exM5GU8vzDAo2YnjQpScB8nwa9qDdTeA@mail.gmail.com","threadId":"40216","inReplyTo":"vpqa8t7j4nf.fsf@anie.imag.fr","subject":"Re: [PATCH v14 00/13] Port tag.c to use ref-filter.c","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-31T11:36:15Z","receivedAt":"2015-08-31T11:36:15Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Mon, Aug 31, 2015 at 1:01 PM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> * We perform quoting on each layer of nested alignment.\n>\n> I do not understand why.\n>\n> For example, using the tip of karthik/exp on GitHub (on top of this\n> series, d91419b (ref-filter: adopt get_head_description() from branch.c,\n> 2015-08-23)):\n>\n> git for-each-ref --shell \\\n> --format 'x=%(if)foo%(then)%(align:10)XXX%(end)%(else) not foo %(end)'\n>\n> I'd expect an output like:\n>\n> x='XXX '\n>\n> and instead I get:\n>\n> x=''\\''XXX '\\'''\n>\n> which assigns the value 'XXX ' (including the quotes) to $x. I do\n> not see a use-case for this (well, I could imagine one where we would\n> later call eval \"$x\", that seems rather far-fetched).\n>\n> I think the quoting should be:\n>\n> 1) When the stack contains only the initial element, quote individual\n> atoms.\n>\n> 2) When the stack contains exactly two elements and encountering a %(end)\n> or %(else), quote the entire strbuf of the 2nd level when appending to\n> the 1st level.\n>\n> 3) When the stack contains more than two elements, perform no quoting at\n> all. The quoting will be done later by #2.\n>\n\nYea, That's what Eric was saying, I even made changes which sum up to\nwhat you're saying :)\n\n> I found a segfault while testing:\n>\n> $ git for-each-ref --format 'x=%(if)%(align:10)%(end)%(then)%(align:10)XXX%(end)%(else)%(end)' --shell\n> zsh: segmentation fault\n>\n\nI wouldn't worry about this ATM, I have made so many changes that the\ntip is barely changed to reflect those, though I'll have a look at it\n:)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"269004","messageId":"CAPig+cRAYeF0ZDn5FsHioZr1g4pH3Ay69_3KDb8ZF1USZxzcEg@mail.gmail.com","threadId":"40216","inReplyTo":"CAOLa=ZS53BNNvnYv2TU6xQbEihi1GiKBaEEZ=KT6p_gDBR9Y9Q@mail.gmail.com","subject":"Re: [PATCH v14 04/13] ref-filter: implement an `align` atom","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-31T17:16:23Z","receivedAt":"2015-08-31T17:16:23Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 31, 2015 at 5:55 AM, Karthik Nayak wrote:\n> On Sun, Aug 30, 2015 at 3:10 PM, Eric Sunshine wrote:\n>> On Sun, Aug 30, 2015 at 9:38 AM, Karthik Nayak wrote:\n>>> On Sun, Aug 30, 2015 at 8:57 AM, Eric Sunshine wrote:\n>>>> On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak wrote:\n>>>>> + } else if (!strcmp(name, \"align\"))\n>>>>> + die(_(\"format: incomplete use of the `align` atom\"));\n>>>>\n>>>> Why does %(align) get flagged as a malformation of %(align:), whereas\n>>>> %(color) does not get flagged as a malformation of %(color:)? Why does\n>>>> one deserve special treatment but not the other?\n>>>\n>>> Didn't see that, I think its needed to add a check for both like :\n>>>\n>>> else if (!strcmp(name, \"align\") || !strcmp(name, \"color\"))\n>>> die(_(\"format: improper usage of %s atom\"), name);\n>>>\n>>> I had a look if any other atoms need a subvalue to operate, couldn't\n>>> find any.\n>>\n>> Hmm, I'm not convinced that either %(align) or %(color) need to be\n>> called out specially. What is the current behavior when these\n>> \"malformations\" or any other misspelled atoms are used? Does it error\n>> out? Does it simply ignore them and pass them through to the output\n>> unmolested?\n>\n> It just simply ignores them currently, which is kinda bad, as the user\n> is given no warning, and the atom is ineffective.\n\nWarning about unrecognized atoms may indeed be a good idea, however,\nthe current behavior isn't a huge problem since user discovers the\nerror when the output fails to match his expectation. This behavior of\nignoring unrecognized atoms predates your work, doesn't it? If so,\nit's probably not something you need to address in this series.\n"},{"id":"269007","messageId":"vpqpp235pvg.fsf@anie.imag.fr","threadId":"40216","inReplyTo":"CAPig+cRAYeF0ZDn5FsHioZr1g4pH3Ay69_3KDb8ZF1USZxzcEg@mail.gmail.com","subject":"Re: [PATCH v14 04/13] ref-filter: implement an `align` atom","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-08-31T17:28:35Z","receivedAt":"2015-08-31T17:28:35Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Eric Sunshine writes:\n\n> On Mon, Aug 31, 2015 at 5:55 AM, Karthik Nayak wrote:\n>> On Sun, Aug 30, 2015 at 3:10 PM, Eric Sunshine wrote:\n>>> On Sun, Aug 30, 2015 at 9:38 AM, Karthik Nayak wrote:\n>>>> On Sun, Aug 30, 2015 at 8:57 AM, Eric Sunshine wrote:\n>>>>> On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak wrote:\n>>>>>> + } else if (!strcmp(name, \"align\"))\n>>>>>> + die(_(\"format: incomplete use of the `align` atom\"));\n>>>>>\n>>>>> Why does %(align) get flagged as a malformation of %(align:), whereas\n>>>>> %(color) does not get flagged as a malformation of %(color:)? Why does\n>>>>> one deserve special treatment but not the other?\n>>>>\n>>>> Didn't see that, I think its needed to add a check for both like :\n>>>>\n>>>> else if (!strcmp(name, \"align\") || !strcmp(name, \"color\"))\n>>>> die(_(\"format: improper usage of %s atom\"), name);\n>>>>\n>>>> I had a look if any other atoms need a subvalue to operate, couldn't\n>>>> find any.\n>>>\n>>> Hmm, I'm not convinced that either %(align) or %(color) need to be\n>>> called out specially. What is the current behavior when these\n>>> \"malformations\" or any other misspelled atoms are used? Does it error\n>>> out? Does it simply ignore them and pass them through to the output\n>>> unmolested?\n>>\n>> It just simply ignores them currently, which is kinda bad, as the user\n>> is given no warning, and the atom is ineffective.\n>\n> Warning about unrecognized atoms may indeed be a good idea, however,\n> the current behavior isn't a huge problem since user discovers the\n> error when the output fails to match his expectation.\n\nIt's a bit worse than it seems: without this change, using --format\n'%(align)%(end)' results in Git complaining about %(end) without\nmatching atom, which is really confusing since you do have a %(align) (I\ngot it for real while testing a preliminary version).\n\n> This behavior of ignoring unrecognized atoms predates your work,\n> doesn't it? If so, it's probably not something you need to address in\n> this series.\n\nI wouldn't insist in having it in the series, but now that it's here, I\nthink we can keep it (if only to shorten the interdiff for the next\niteration).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"269012","messageId":"CAPig+cRig+zk=D1SDF7wBHuQgcQqb-4cHkmeVgLs5BaWzk9WRg@mail.gmail.com","threadId":"40216","inReplyTo":"vpqpp235pvg.fsf@anie.imag.fr","subject":"Re: [PATCH v14 04/13] ref-filter: implement an `align` atom","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-31T18:02:33Z","receivedAt":"2015-08-31T18:02:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 31, 2015 at 1:28 PM, Matthieu Moy\n wrote:\n> Eric Sunshine writes:\n>> On Mon, Aug 31, 2015 at 5:55 AM, Karthik Nayak wrote:\n>>> On Sun, Aug 30, 2015 at 3:10 PM, Eric Sunshine wrote:\n>>>> On Sun, Aug 30, 2015 at 9:38 AM, Karthik Nayak wrote:\n>>>>> On Sun, Aug 30, 2015 at 8:57 AM, Eric Sunshine wrote:\n>>>>>> On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak wrote:\n>>>>>>> + } else if (!strcmp(name, \"align\"))\n>>>>>>> + die(_(\"format: incomplete use of the `align` atom\"));\n>>>>>>\n>>>>>> Why does %(align) get flagged as a malformation of %(align:), whereas\n>>>>>> %(color) does not get flagged as a malformation of %(color:)? Why does\n>>>>>> one deserve special treatment but not the other?\n>>>>>\n>>>>> Didn't see that, I think its needed to add a check for both like :\n>>>>>\n>>>>> else if (!strcmp(name, \"align\") || !strcmp(name, \"color\"))\n>>>>> die(_(\"format: improper usage of %s atom\"), name);\n>>>>>\n>>>>> I had a look if any other atoms need a subvalue to operate, couldn't\n>>>>> find any.\n>>>>\n>>>> Hmm, I'm not convinced that either %(align) or %(color) need to be\n>>>> called out specially. What is the current behavior when these\n>>>> \"malformations\" or any other misspelled atoms are used? Does it error\n>>>> out? Does it simply ignore them and pass them through to the output\n>>>> unmolested?\n>>>\n>>> It just simply ignores them currently, which is kinda bad, as the user\n>>> is given no warning, and the atom is ineffective.\n>>\n>> Warning about unrecognized atoms may indeed be a good idea, however,\n>> the current behavior isn't a huge problem since user discovers the\n>> error when the output fails to match his expectation.\n>\n> It's a bit worse than it seems: without this change, using --format\n> '%(align)%(end)' results in Git complaining about %(end) without\n> matching atom, which is really confusing since you do have a %(align) (I\n> got it for real while testing a preliminary version).\n>\n>> This behavior of ignoring unrecognized atoms predates your work,\n>> doesn't it? If so, it's probably not something you need to address in\n>> this series.\n>\n> I wouldn't insist in having it in the series, but now that it's here, I\n> think we can keep it (if only to shorten the interdiff for the next\n> iteration).\n\nThe unstated subtext in my original question is that the approach\ntaken by this patch of warning only about unrecognized %(align) is too\nspecial-case; if it's going to warn at all, it should do so\ngenerically for any unrecognized %(atom). Special-casing the warning\nabout malformed %(align) sets a poor precedent; it's code which will\neventually need to be removed anyhow when the generic warning\nmechanism is eventually implemented.\n"},{"id":"269089","messageId":"CAOLa=ZQV63ZETPswEPYjSMrXcZAK+W-Jek+Psq7wtmSsQHtb3g@mail.gmail.com","threadId":"40216","inReplyTo":"CAPig+cRig+zk=D1SDF7wBHuQgcQqb-4cHkmeVgLs5BaWzk9WRg@mail.gmail.com","subject":"Re: [PATCH v14 04/13] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-09-01T13:05:06Z","receivedAt":"2015-09-01T13:05:06Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Mon, Aug 31, 2015 at 11:32 PM, Eric Sunshine wrote:\n> On Mon, Aug 31, 2015 at 1:28 PM, Matthieu Moy\n> wrote:\n>> Eric Sunshine writes:\n>>>\n>>> Warning about unrecognized atoms may indeed be a good idea, however,\n>>> the current behavior isn't a huge problem since user discovers the\n>>> error when the output fails to match his expectation.\n>>\n>> It's a bit worse than it seems: without this change, using --format\n>> '%(align)%(end)' results in Git complaining about %(end) without\n>> matching atom, which is really confusing since you do have a %(align) (I\n>> got it for real while testing a preliminary version).\n>>\n>>> This behavior of ignoring unrecognized atoms predates your work,\n>>> doesn't it? If so, it's probably not something you need to address in\n>>> this series.\n>>\n>> I wouldn't insist in having it in the series, but now that it's here, I\n>> think we can keep it (if only to shorten the interdiff for the next\n>> iteration).\n>\n> The unstated subtext in my original question is that the approach\n> taken by this patch of warning only about unrecognized %(align) is too\n> special-case; if it's going to warn at all, it should do so\n> generically for any unrecognized %(atom). Special-casing the warning\n> about malformed %(align) sets a poor precedent; it's code which will\n> eventually need to be removed anyhow when the generic warning\n> mechanism is eventually implemented.\n\nProbably, I could just have a check within the align block and maybe build a\ngeneral case later.\n\nLike this:\n\n else if (skip_prefix(name, \"align\", &valp)) {\n struct align *align = &v->u.align;\n struct strbuf **s;\n\n if (valp[0] != ':')\n die(_(\"format: usage %%(align:,)\"));\n else\n valp++;\n ......\n ........\n}\n\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"269090","messageId":"vpqk2sa1dyp.fsf@anie.imag.fr","threadId":"40216","inReplyTo":"CAOLa=ZQV63ZETPswEPYjSMrXcZAK+W-Jek+Psq7wtmSsQHtb3g@mail.gmail.com","subject":"Re: [PATCH v14 04/13] ref-filter: implement an `align` atom","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-09-01T13:11:42Z","receivedAt":"2015-09-01T13:11:42Z","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> Like this:\n>\n> else if (skip_prefix(name, \"align\", &valp)) {\n> struct align *align = &v->u.align;\n> struct strbuf **s;\n>\n> if (valp[0] != ':')\n> die(_(\"format: usage %%(align:,)\"));\n> else\n> valp++;\n> ......\n> ........\n> }\n\nChecking the string's correctness as you read it seems good to me, yes.\n\nBut as you say, you should make this a bit more generic, for example by\nputting your \"if/else\" in a helper function, that other atoms could use.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"269095","messageId":"CAOLa=ZQfHViZa15SD-PZOr_AZqAEnN9RJ9ouPhUoEMUfSd56Gw@mail.gmail.com","threadId":"40216","inReplyTo":"vpqk2sa1dyp.fsf@anie.imag.fr","subject":"Re: [PATCH v14 04/13] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-09-01T15:13:56Z","receivedAt":"2015-09-01T15:13:56Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Sep 1, 2015 at 6:41 PM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> Like this:\n>>\n>> else if (skip_prefix(name, \"align\", &valp)) {\n>> struct align *align = &v->u.align;\n>> struct strbuf **s;\n>>\n>> if (valp[0] != ':')\n>> die(_(\"format: usage %%(align:,)\"));\n>> else\n>> valp++;\n>> ......\n>> ........\n>> }\n>\n> Checking the string's correctness as you read it seems good to me, yes.\n>\n> But as you say, you should make this a bit more generic, for example by\n> putting your \"if/else\" in a helper function, that other atoms could use.\n\nOkay then, I'll keep that in mind :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"269112","messageId":"CAOLa=ZS3jt57zMwKcWYF6Nfa=n4-rjCbhq9QSeo91hAsN3F2PA@mail.gmail.com","threadId":"40216","inReplyTo":"CAOLa=ZRJC5FD08Ljh=exM5GU8vzDAo2YnjQpScB8nwa9qDdTeA@mail.gmail.com","subject":"Re: [PATCH v14 00/13] Port tag.c to use ref-filter.c","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-09-01T17:37:45Z","receivedAt":"2015-09-01T17:37:45Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Mon, Aug 31, 2015 at 5:06 PM, Karthik Nayak wrote:\n> On Mon, Aug 31, 2015 at 1:01 PM, Matthieu Moy\n> wrote:\n>> Karthik Nayak writes:\n>>\n>>> * We perform quoting on each layer of nested alignment.\n>>\n>> I do not understand why.\n>>\n>> For example, using the tip of karthik/exp on GitHub (on top of this\n>> series, d91419b (ref-filter: adopt get_head_description() from branch.c,\n>> 2015-08-23)):\n>>\n>> git for-each-ref --shell \\\n>> --format 'x=%(if)foo%(then)%(align:10)XXX%(end)%(else) not foo %(end)'\n>>\n>> I'd expect an output like:\n>>\n>> x='XXX '\n>>\n>> and instead I get:\n>>\n>> x=''\\''XXX '\\'''\n>>\n>> which assigns the value 'XXX ' (including the quotes) to $x. I do\n>> not see a use-case for this (well, I could imagine one where we would\n>> later call eval \"$x\", that seems rather far-fetched).\n>>\n>> I think the quoting should be:\n>>\n>> 1) When the stack contains only the initial element, quote individual\n>> atoms.\n>>\n>> 2) When the stack contains exactly two elements and encountering a %(end)\n>> or %(else), quote the entire strbuf of the 2nd level when appending to\n>> the 1st level.\n>>\n>> 3) When the stack contains more than two elements, perform no quoting at\n>> all. The quoting will be done later by #2.\n>>\n>\n> Yea, That's what Eric was saying, I even made changes which sum up to\n> what you're saying :)\n>\n>> I found a segfault while testing:\n>>\n>> $ git for-each-ref --format 'x=%(if)%(align:10)%(end)%(then)%(align:10)XXX%(end)%(else)%(end)' --shell\n>> zsh: segmentation fault\n>>\n>\n> I wouldn't worry about this ATM, I have made so many changes that the\n> tip is barely changed to reflect those, though I'll have a look at it\n> :)\n>\n\nWas a dereferencing error, fixed it now :)\n\n-- \nRegards,\nKarthik Nayak\n"}]}