{"thread":{"id":"40040","subject":"[PATCH v10 01/13] ref-filter: move `struct atom_value` to ref-filter.c","startedAt":"2015-08-09T14:11:33Z","lastAt":"2015-08-15T08:27:32Z","messageCount":40,"participants":["Karthik Nayak","Junio C Hamano","Eric Sunshine","Duy Nguyen"],"isPatch":true,"patchVersion":10,"patchTotal":13},"messages":[{"id":"267678","messageId":"1439129506-9989-1-git-send-email-Karthik.188@gmail.com","threadId":"40040","inReplyTo":null,"subject":"[PATCH v10 00/13] port tag.c to use ref-filter APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-09T14:11:33Z","receivedAt":"2015-08-09T14:11:33Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"This is part of my GSoC project to unify git tag -l, git branch -l,\ngit for-each-ref.  This patch series is continued from: Git (next)\nhttps://github.com/git/git/commit/bf5418f49ff0cebc6e5ce04ad1417e1a47c81b61\n\nThis series consists of porting tag.c over to using the ref-filter APIs\n\nVersion 8 can be found here:\nhttp://thread.gmane.org/gmane.comp.version-control.git/275252\n\nChanges in this version: \n* use fwrite instead of looping over single characters and printing them.\n* use strbuf_reset() at places instead of strbuf_release().\n* make ref_formatting_state hold a strbuf, rather than a pointer to one.\n* change a few function names to denote changes made to their implementations.\n* move alignment to utf8, and use an enum for align_type.\n* rename some variables to give better meanings.\n* combine skip_prefix and starts_with in populate_value().\n* fix utf8 checking.\n* more small changes in the interdiff.\n\nThanks to everyone for their suggestions, especially Eric.\n\nKarthik Nayak (13):\n  ref-filter: move `struct atom_value` to ref-filter.c\n  ref-filter: print output to strbuf for formatting\n  ref-filter: introduce ref_formatting_state\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 only tags\n  ref-filter: support printing N lines from tag annotation\n  ref-filter: add support to sort by version\n  ref-filter: add option to match literal pattern\n  tag.c: use 'ref-filter' data structures\n  tag.c: use 'ref-filter' APIs\n  tag.c: implement '--format' option\n  tag.c: implement '--merged' and '--no-merged' options\n\n Documentation/git-for-each-ref.txt |  12 ++\n Documentation/git-tag.txt          |  34 +++-\n builtin/for-each-ref.c             |   3 +-\n builtin/tag.c                      | 367 +++++++------------------------------\n ref-filter.c                       | 243 +++++++++++++++++++++---\n ref-filter.h                       |  22 ++-\n refs.c                             |   5 +\n refs.h                             |   1 +\n t/t6302-for-each-ref-filter.sh     |  84 +++++++++\n t/t7004-tag.sh                     |  51 +++++-\n utf8.c                             |  22 +++\n utf8.h                             |  13 ++\n 12 files changed, 500 insertions(+), 357 deletions(-)\n\nThe interdiff between v9 and v10:\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 6b6eb93..a795d5f 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -128,12 +128,13 @@ color::\n \tare described in `color.branch.*`.\n \n align::\n-\tAlign any string with or without %(atom) before the %(end)\n-\tatom to the right, left or middle. Followed by\n-\t`:<type>,<paddinglength>`, where the `<type>` is either left,\n-\tright or middle and `<paddinglength>` is the total length of\n-\tthe padding to be performed. If the string length is more than\n-\tthe padding length then no padding is performed.\n+\tImplement an `align` atom which left-, middle-, or\n+\tright-aligns the content between %(align:..)  and\n+\t%(end). Followed by `:<position>,<width>`, where the\n+\t`<position>` is either left, right or middle and `<width>` 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. Currently nested alignment is not supported.\n \n In addition to the above, for commit and tag objects, the header\n field names (`tree`, `parent`, `object`, `type`, and `tag`) can\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 529b29f..22efbba 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -41,7 +41,7 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con\n \t\tfilter->lines = 0;\n \n \tif (filter->lines)\n-\t\tformat = \"%(align:left,16)%(refname:short)\";\n+\t\tformat = \"%(align:left,16)%(refname:short)%(end)\";\n \telse if (!format)\n \t\tformat = \"%(refname:short)\";\n \ndiff --git a/ref-filter.c b/ref-filter.c\nindex de84dd4..d22bf66 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -60,6 +60,19 @@ static struct {\n \t{ \"end\" },\n };\n \n+struct align {\n+\talign_type position;\n+\tunsigned int width;\n+};\n+\n+struct atom_value{\n+\tconst char *s;\n+\tstruct align *align;\n+\tunsigned long ul; /* used for sorting when not FIELD_STR */\n+\tunsigned int modifier_atom : 1,\n+\t\tend : 1;\n+};\n+\n /*\n  * An atom is a valid field atom listed above, possibly prefixed with\n  * a \"*\" to denote deref_tag().\n@@ -625,8 +638,9 @@ static void populate_value(struct ref_array_item *ref)\n \t\tconst char *name = used_atom[i];\n \t\tstruct atom_value *v = &ref->value[i];\n \t\tint deref = 0;\n-\t\tconst char *refname = NULL;\n+\t\tconst char *refname;\n \t\tconst char *formatp;\n+\t\tconst char *valp;\n \t\tstruct branch *branch = NULL;\n \n \t\tif (*name == '*') {\n@@ -692,26 +706,23 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\telse\n \t\t\t\tv->s = \" \";\n \t\t\tcontinue;\n-\t\t} else if (starts_with(name, \"align:\")) {\n-\t\t\tconst char *valp = NULL;\n+\t\t} else if (skip_prefix(name, \"align:\", &valp)) {\n \t\t\tstruct align *align = xmalloc(sizeof(struct align));\n \n-\t\t\tskip_prefix(name, \"align:\", &valp);\n-\n \t\t\tif (skip_prefix(valp, \"left,\", &valp))\n-\t\t\t\talign->align_type = ALIGN_LEFT;\n+\t\t\t\talign->position = ALIGN_LEFT;\n \t\t\telse if (skip_prefix(valp, \"right,\", &valp))\n-\t\t\t\talign->align_type = ALIGN_RIGHT;\n+\t\t\t\talign->position = ALIGN_RIGHT;\n \t\t\telse if (skip_prefix(valp, \"middle,\", &valp))\n-\t\t\t\talign->align_type = ALIGN_MIDDLE;\n+\t\t\t\talign->position = ALIGN_MIDDLE;\n \t\t\telse\n-\t\t\t\tdie(_(\"align: improper format\"));\n-\t\t\tif (strtoul_ui(valp, 10, &align->align_value))\n-\t\t\t\tdie(_(\"align: positive value expected\"));\n+\t\t\t\tdie(_(\"improper format entered align:%s\"), valp);\n+\t\t\tif (strtoul_ui(valp, 10, &align->width))\n+\t\t\t\tdie(_(\"positive width expected align:%s\"), valp);\n \t\t\tv->align = align;\n \t\t\tv->modifier_atom = 1;\n \t\t\tcontinue;\n-\t\t} else if (starts_with(name, \"end\")) {\n+\t\t} else if (!strcmp(name, \"end\")) {\n \t\t\tv->end = 1;\n \t\t\tv->modifier_atom = 1;\n \t\t\tcontinue;\n@@ -1253,23 +1264,31 @@ 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, struct ref_formatting_state *state)\n+\n+struct ref_formatting_state {\n+\tstruct strbuf output;\n+\tstruct align *align;\n+\tint quote_style;\n+\tunsigned int end : 1;\n+};\n+\n+static void format_quote_value(struct atom_value *v, struct ref_formatting_state *state)\n {\n \tswitch (state->quote_style) {\n \tcase QUOTE_NONE:\n-\t\tstrbuf_addstr(state->output, v->s);\n+\t\tstrbuf_addstr(&state->output, v->s);\n \t\tbreak;\n \tcase QUOTE_SHELL:\n-\t\tsq_quote_buf(state->output, v->s);\n+\t\tsq_quote_buf(&state->output, v->s);\n \t\tbreak;\n \tcase QUOTE_PERL:\n-\t\tperl_quote_buf(state->output, v->s);\n+\t\tperl_quote_buf(&state->output, v->s);\n \t\tbreak;\n \tcase QUOTE_PYTHON:\n-\t\tpython_quote_buf(state->output, v->s);\n+\t\tpython_quote_buf(&state->output, v->s);\n \t\tbreak;\n \tcase QUOTE_TCL:\n-\t\ttcl_quote_buf(state->output, v->s);\n+\t\ttcl_quote_buf(&state->output, v->s);\n \t\tbreak;\n \t}\n }\n@@ -1292,7 +1311,7 @@ static int hex2(const char *cp)\n \t\treturn -1;\n }\n \n-static void emit(const char *cp, const char *ep, struct ref_formatting_state *state)\n+static void append_non_atom(const char *cp, const char *ep, struct ref_formatting_state *state)\n {\n \twhile (*cp && (!ep || cp < ep)) {\n \t\tif (*cp == '%') {\n@@ -1301,18 +1320,18 @@ static void emit(const char *cp, const char *ep, struct ref_formatting_state *st\n \t\t\telse {\n \t\t\t\tint ch = hex2(cp + 1);\n \t\t\t\tif (0 <= ch) {\n-\t\t\t\t\tstrbuf_addch(state->output, ch);\n+\t\t\t\t\tstrbuf_addch(&state->output, ch);\n \t\t\t\t\tcp += 3;\n \t\t\t\t\tcontinue;\n \t\t\t\t}\n \t\t\t}\n \t\t}\n-\t\tstrbuf_addch(state->output, *cp);\n+\t\tstrbuf_addch(&state->output, *cp);\n \t\tcp++;\n \t}\n }\n \n-static void process_formatting_state(struct atom_value *atomv, struct ref_formatting_state *state)\n+static void set_formatting_state(struct atom_value *atomv, struct ref_formatting_state *state)\n {\n \tif (atomv->align) {\n \t\tstate->align = atomv->align;\n@@ -1322,42 +1341,25 @@ static void process_formatting_state(struct atom_value *atomv, struct ref_format\n \t\tstate->end = 1;\n }\n \n-static void apply_formatting_state(struct ref_formatting_state *state, struct strbuf *final)\n+static int align_ref_strbuf(struct ref_formatting_state *state, struct strbuf *final)\n {\n \tif (state->align && state->end) {\n-\t\tstruct strbuf *value = state->output;\n-\t\tint len = 0, buf_len = value->len;\n \t\tstruct align *align = state->align;\n-\n-\t\tif (!value->buf)\n-\t\t\treturn;\n-\t\tif (!is_utf8(value->buf)) {\n-\t\t\tlen = value->len - utf8_strwidth(value->buf);\n-\t\t\tbuf_len -= len;\n-\t\t}\n-\n-\t\tif (align->align_value < buf_len) {\n-\t\t\tstate->align = NULL;\n-\t\t\tstrbuf_addbuf(final, value);\n-\t\t\tstrbuf_release(value);\n-\t\t\treturn;\n-\t\t}\n-\n-\t\tif (align->align_type == ALIGN_LEFT)\n-\t\t\tstrbuf_addf(final, \"%-*s\", len + align->align_value, value->buf);\n-\t\telse if (align->align_type == ALIGN_MIDDLE) {\n-\t\t\tint right = (align->align_value - buf_len)/2;\n-\t\t\tstrbuf_addf(final, \"%*s%-*s\", align->align_value - right + len,\n-\t\t\t\t    value->buf, right, \"\");\n-\t\t} else if (align->align_type == ALIGN_RIGHT)\n-\t\t\tstrbuf_addf(final, \"%*s\", align->align_value, value->buf);\n-\t\tstrbuf_release(value);\n+\t\tstrbuf_utf8_align(final, align->position, align->width, state->output.buf);\n+\t\tstrbuf_reset(&state->output);\n \t\tstate->align = NULL;\n-\t\treturn;\n+\t\treturn 1;\n \t} else if (state->align)\n+\t\treturn 1;\n+\treturn 0;\n+}\n+\n+static void perform_state_formatting(struct ref_formatting_state *state, struct strbuf *final)\n+{\n+\tif (align_ref_strbuf(state, final))\n \t\treturn;\n-\tstrbuf_addbuf(final, state->output);\n-\tstrbuf_release(state->output);\n+\tstrbuf_addbuf(final, &state->output);\n+\tstrbuf_reset(&state->output);\n }\n \n /*\n@@ -1407,41 +1409,38 @@ void show_ref_array_item(struct ref_array_item *info, const char *format,\n \t\t\t int quote_style, unsigned int lines)\n {\n \tconst char *cp, *sp, *ep;\n-\tstruct strbuf value = STRBUF_INIT;\n \tstruct strbuf final_buf = STRBUF_INIT;\n \tstruct ref_formatting_state state;\n-\tint i;\n \n-\tmemset(&state, 0, sizeof(state));\n \tstate.quote_style = quote_style;\n-\tstate.output = &value;\n+\tstrbuf_init(&state.output, 0);\n+\tstate.align = NULL;\n+\tstate.end = 0;\n \n \tfor (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {\n-\t\tstruct atom_value *atomv = NULL;\n+\t\tstruct atom_value *atomv;\n \n \t\tep = strchr(sp, ')');\n \t\tif (cp < sp) {\n-\t\t\temit(cp, sp, &state);\n-\t\t\tapply_formatting_state(&state, &final_buf);\n+\t\t\tappend_non_atom(cp, sp, &state);\n+\t\t\tperform_state_formatting(&state, &final_buf);\n \t\t}\n \t\tget_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);\n \t\tif (atomv->modifier_atom)\n-\t\t\tprocess_formatting_state(atomv, &state);\n+\t\t\tset_formatting_state(atomv, &state);\n \t\telse {\n-\t\t\tprint_value(atomv, &state);\n-\t\t\tapply_formatting_state(&state, &final_buf);\n-\t\t}\n-\t\tif (atomv->end) {\n-\t\t\tprint_value(atomv, &state);\n-\t\t\tapply_formatting_state(&state, &final_buf);\n+\t\t\tformat_quote_value(atomv, &state);\n+\t\t\tperform_state_formatting(&state, &final_buf);\n \t\t}\n+\t\tif (atomv->end)\n+\t\t\tperform_state_formatting(&state, &final_buf);\n \t}\n \tif (state.align)\n \t\tdie(_(\"format: align used without an `end` atom\"));\n \tif (*cp) {\n \t\tsp = cp + strlen(cp);\n-\t\temit(cp, sp, &state);\n-\t\tapply_formatting_state(&state, &final_buf);\n+\t\tappend_non_atom(cp, sp, &state);\n+\t\tperform_state_formatting(&state, &final_buf);\n \t}\n \tif (need_color_reset_at_eol) {\n \t\tstruct atom_value resetv;\n@@ -1450,13 +1449,13 @@ void show_ref_array_item(struct ref_array_item *info, const char *format,\n \t\tif (color_parse(\"reset\", color) < 0)\n \t\t\tdie(\"BUG: couldn't parse 'reset' as a color\");\n \t\tresetv.s = color;\n-\t\tprint_value(&resetv, &state);\n-\t\tapply_formatting_state(&state, &final_buf);\n+\t\tformat_quote_value(&resetv, &state);\n+\t\tperform_state_formatting(&state, &final_buf);\n \t}\n \n-\tfor (i = 0; i < final_buf.len; i++)\n-\t\tprintf(\"%c\", final_buf.buf[i]);\n+\tfwrite(final_buf.buf, 1, final_buf.len, stdout);\n \tstrbuf_release(&final_buf);\n+\tstrbuf_release(&state.output);\n \n \tif (lines > 0) {\n \t\tstruct object_id oid;\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 5be3e35..75bf989 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -5,6 +5,7 @@\n #include \"refs.h\"\n #include \"commit.h\"\n #include \"parse-options.h\"\n+#include \"utf8.h\"\n \n /* Quoting styles */\n #define QUOTE_NONE 0\n@@ -17,29 +18,7 @@\n #define FILTER_REFS_ALL 0x2\n #define FILTER_REFS_TAGS 0x4\n \n-#define ALIGN_LEFT 0x01\n-#define ALIGN_RIGHT 0x02\n-#define ALIGN_MIDDLE 0x04\n-\n-struct ref_formatting_state {\n-\tint quote_style;\n-\tstruct strbuf *output;\n-\tstruct align *align;\n-\tunsigned int end : 1;\n-};\n-\n-struct align {\n-\tunsigned int align_type,\n-\t\talign_value;\n-};\n-\n-struct atom_value {\n-\tconst char *s;\n-\tstruct align *align;\n-\tunsigned long ul; /* used for sorting when not FIELD_STR */\n-\tunsigned int modifier_atom : 1,\n-\t\tend : 1;\n-};\n+struct atom_value;\n \n struct ref_sorting {\n \tstruct ref_sorting *next;\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 272a7e4..ce455d9 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -103,7 +103,7 @@ test_expect_success 'middle alignment' '\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/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\ndiff --git a/utf8.c b/utf8.c\nindex 28e6d76..bcbc621 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -644,3 +644,25 @@ 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 display_len = utf8_strnwidth(s, strlen(s), 0);\n+\tint utf8_compenstation = strlen(s) - display_len;\n+\n+\tif (!strlen(s))\n+\t\treturn;\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_compenstation, 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 + ut8_compenstation, s);\n+\t} else if (position == ALIGN_RIGHT)\n+\t\tstrbuf_addf(buf, \"%*s\", width + utf8_compenstation, s);\n+}\ndiff --git a/utf8.h b/utf8.h\nindex 5a9e94b..db8ca63 100644\n--- a/utf8.h\n+++ b/utf8.h\n@@ -55,4 +55,17 @@ 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 type\n+ * and width.\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"},{"id":"267677","messageId":"1439129506-9989-2-git-send-email-Karthik.188@gmail.com","threadId":"40040","inReplyTo":"1439129506-9989-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v10 01/13] ref-filter: move `struct atom_value` to ref-filter.c","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-09T14:11:34Z","receivedAt":"2015-08-09T14:11:34Z","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 <sunshine@sunshineco.com>\nMentored-by: Christian Couder <christian.couder@gmail.com>\nMentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\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..07949f4 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":"267679","messageId":"1439129506-9989-3-git-send-email-Karthik.188@gmail.com","threadId":"40040","inReplyTo":"1439129506-9989-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v10 02/13] ref-filter: print output to strbuf for formatting","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-09T14:11:35Z","receivedAt":"2015-08-09T14:11:35Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Introduce a strbuf `output` which will act as a substitute rather than\nprinting directly to stdout. This will be used for formatting\neventually.\n\nRename some functions to reflect the changes made:\nprint_value() -> format_quote_value()\nemit()        -> append_non_atom()\n\nMentored-by: Christian Couder <christian.couder@gmail.com>\nMentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n ref-filter.c | 34 ++++++++++++++++------------------\n 1 file changed, 16 insertions(+), 18 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 07949f4..95f027b 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1195,30 +1195,25 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)\n \tqsort(array->items, array->nr, sizeof(struct ref_array_item *), compare_refs);\n }\n \n-static void print_value(struct atom_value *v, int quote_style)\n+static void format_quote_value(struct atom_value *v, int quote_style, struct strbuf *output)\n {\n-\tstruct strbuf sb = STRBUF_INIT;\n \tswitch (quote_style) {\n \tcase QUOTE_NONE:\n-\t\tfputs(v->s, stdout);\n+\t\tstrbuf_addstr(output, v->s);\n \t\tbreak;\n \tcase QUOTE_SHELL:\n-\t\tsq_quote_buf(&sb, v->s);\n+\t\tsq_quote_buf(output, v->s);\n \t\tbreak;\n \tcase QUOTE_PERL:\n-\t\tperl_quote_buf(&sb, v->s);\n+\t\tperl_quote_buf(output, v->s);\n \t\tbreak;\n \tcase QUOTE_PYTHON:\n-\t\tpython_quote_buf(&sb, v->s);\n+\t\tpython_quote_buf(output, v->s);\n \t\tbreak;\n \tcase QUOTE_TCL:\n-\t\ttcl_quote_buf(&sb, v->s);\n+\t\ttcl_quote_buf(output, v->s);\n \t\tbreak;\n \t}\n-\tif (quote_style != QUOTE_NONE) {\n-\t\tfputs(sb.buf, stdout);\n-\t\tstrbuf_release(&sb);\n-\t}\n }\n \n static int hex1(char ch)\n@@ -1239,7 +1234,7 @@ static int hex2(const char *cp)\n \t\treturn -1;\n }\n \n-static void emit(const char *cp, const char *ep)\n+static void append_non_atom(const char *cp, const char *ep, struct strbuf *output)\n {\n \twhile (*cp && (!ep || cp < ep)) {\n \t\tif (*cp == '%') {\n@@ -1248,13 +1243,13 @@ static void emit(const char *cp, const char *ep)\n \t\t\telse {\n \t\t\t\tint ch = hex2(cp + 1);\n \t\t\t\tif (0 <= ch) {\n-\t\t\t\t\tputchar(ch);\n+\t\t\t\t\tstrbuf_addch(output, ch);\n \t\t\t\t\tcp += 3;\n \t\t\t\t\tcontinue;\n \t\t\t\t}\n \t\t\t}\n \t\t}\n-\t\tputchar(*cp);\n+\t\tstrbuf_addch(output, *cp);\n \t\tcp++;\n \t}\n }\n@@ -1262,19 +1257,20 @@ static void emit(const char *cp, const char *ep)\n void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)\n {\n \tconst char *cp, *sp, *ep;\n+\tstruct strbuf output = STRBUF_INIT;\n \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_non_atom(cp, sp, &output);\n \t\tget_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);\n-\t\tprint_value(atomv, quote_style);\n+\t\tformat_quote_value(atomv, quote_style, &output);\n \t}\n \tif (*cp) {\n \t\tsp = cp + strlen(cp);\n-\t\temit(cp, sp);\n+\t\tappend_non_atom(cp, sp, &output);\n \t}\n \tif (need_color_reset_at_eol) {\n \t\tstruct atom_value resetv;\n@@ -1283,9 +1279,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\tformat_quote_value(&resetv, quote_style, &output);\n \t}\n+\tfwrite(output.buf, 1, output.len, stdout);\n \tputchar('\\n');\n+\tstrbuf_release(&output);\n }\n \n /*  If no sorting option is given, use refname to sort as default */\n-- \n2.5.0\n"},{"id":"267680","messageId":"1439129506-9989-4-git-send-email-Karthik.188@gmail.com","threadId":"40040","inReplyTo":"1439129506-9989-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v10 03/13] ref-filter: introduce ref_formatting_state","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-09T14:11:36Z","receivedAt":"2015-08-09T14:11:36Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Introduce a ref_formatting_state which will eventually hold the values\nof modifier atoms. Implement this within ref-filter.\n\nMentored-by: Christian Couder <christian.couder@gmail.com>\nMentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n ref-filter.c | 65 +++++++++++++++++++++++++++++++++++++++++++-----------------\n 1 file changed, 47 insertions(+), 18 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 95f027b..e2df209 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1195,23 +1195,29 @@ 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 format_quote_value(struct atom_value *v, int quote_style, struct strbuf *output)\n+\n+struct ref_formatting_state {\n+\tstruct strbuf output;\n+\tint quote_style;\n+};\n+\n+static void format_quote_value(struct atom_value *v, struct ref_formatting_state *state)\n {\n-\tswitch (quote_style) {\n+\tswitch (state->quote_style) {\n \tcase QUOTE_NONE:\n-\t\tstrbuf_addstr(output, v->s);\n+\t\tstrbuf_addstr(&state->output, v->s);\n \t\tbreak;\n \tcase QUOTE_SHELL:\n-\t\tsq_quote_buf(output, v->s);\n+\t\tsq_quote_buf(&state->output, v->s);\n \t\tbreak;\n \tcase QUOTE_PERL:\n-\t\tperl_quote_buf(output, v->s);\n+\t\tperl_quote_buf(&state->output, v->s);\n \t\tbreak;\n \tcase QUOTE_PYTHON:\n-\t\tpython_quote_buf(output, v->s);\n+\t\tpython_quote_buf(&state->output, v->s);\n \t\tbreak;\n \tcase QUOTE_TCL:\n-\t\ttcl_quote_buf(output, v->s);\n+\t\ttcl_quote_buf(&state->output, v->s);\n \t\tbreak;\n \t}\n }\n@@ -1234,7 +1240,7 @@ static int hex2(const char *cp)\n \t\treturn -1;\n }\n \n-static void append_non_atom(const char *cp, const char *ep, struct strbuf *output)\n+static void append_non_atom(const char *cp, const char *ep, struct ref_formatting_state *state)\n {\n \twhile (*cp && (!ep || cp < ep)) {\n \t\tif (*cp == '%') {\n@@ -1243,34 +1249,55 @@ static void append_non_atom(const char *cp, const char *ep, struct strbuf *outpu\n \t\t\telse {\n \t\t\t\tint ch = hex2(cp + 1);\n \t\t\t\tif (0 <= ch) {\n-\t\t\t\t\tstrbuf_addch(output, ch);\n+\t\t\t\t\tstrbuf_addch(&state->output, ch);\n \t\t\t\t\tcp += 3;\n \t\t\t\t\tcontinue;\n \t\t\t\t}\n \t\t\t}\n \t\t}\n-\t\tstrbuf_addch(output, *cp);\n+\t\tstrbuf_addch(&state->output, *cp);\n \t\tcp++;\n \t}\n }\n \n+static void set_formatting_state(struct atom_value *atomv, struct ref_formatting_state *state)\n+{\n+\t/* Based on the atomv values, the formatting state is set */\n+}\n+\n+static void perform_state_formatting(struct ref_formatting_state *state, struct strbuf *final)\n+{\n+\t/* More formatting options to be eventually added */\n+\tstrbuf_addbuf(final, &state->output);\n+\tstrbuf_reset(&state->output);\n+}\n+\n void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)\n {\n \tconst char *cp, *sp, *ep;\n-\tstruct strbuf output = STRBUF_INIT;\n+\tstruct strbuf final_buf = STRBUF_INIT;\n+\tstruct ref_formatting_state state;\n+\n+\tstate.quote_style = quote_style;\n+\tstrbuf_init(&state.output, 0);\n \n \tfor (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {\n \t\tstruct atom_value *atomv;\n \n \t\tep = strchr(sp, ')');\n-\t\tif (cp < sp)\n-\t\t\tappend_non_atom(cp, sp, &output);\n+\t\tif (cp < sp) {\n+\t\t\tappend_non_atom(cp, sp, &state);\n+\t\t\tperform_state_formatting(&state, &final_buf);\n+\t\t}\n \t\tget_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);\n-\t\tformat_quote_value(atomv, quote_style, &output);\n+\t\tset_formatting_state(atomv, &state);\n+\t\tformat_quote_value(atomv, &state);\n+\t\tperform_state_formatting(&state, &final_buf);\n \t}\n \tif (*cp) {\n \t\tsp = cp + strlen(cp);\n-\t\tappend_non_atom(cp, sp, &output);\n+\t\tappend_non_atom(cp, sp, &state);\n+\t\tperform_state_formatting(&state, &final_buf);\n \t}\n \tif (need_color_reset_at_eol) {\n \t\tstruct atom_value resetv;\n@@ -1279,11 +1306,13 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n \t\tif (color_parse(\"reset\", color) < 0)\n \t\t\tdie(\"BUG: couldn't parse 'reset' as a color\");\n \t\tresetv.s = color;\n-\t\tformat_quote_value(&resetv, quote_style, &output);\n+\t\tformat_quote_value(&resetv, &state);\n+\t\tperform_state_formatting(&state, &final_buf);\n \t}\n-\tfwrite(output.buf, 1, output.len, stdout);\n+\tfwrite(final_buf.buf, 1, final_buf.len, stdout);\n \tputchar('\\n');\n-\tstrbuf_release(&output);\n+\tstrbuf_release(&final_buf);\n+\tstrbuf_release(&state.output);\n }\n \n /*  If no sorting option is given, use refname to sort as default */\n-- \n2.5.0\n"},{"id":"267681","messageId":"1439129506-9989-5-git-send-email-Karthik.188@gmail.com","threadId":"40040","inReplyTo":"1439129506-9989-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v10 04/13] utf8: add function to align a string into given strbuf","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-09T14:11:37Z","receivedAt":"2015-08-09T14:11:37Z","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 <sunshine@sunshineco.com>\nMentored-by: Christian Couder <christian.couder@gmail.com>\nMentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n utf8.c | 22 ++++++++++++++++++++++\n utf8.h | 13 +++++++++++++\n 2 files changed, 35 insertions(+)\n\ndiff --git a/utf8.c b/utf8.c\nindex 28e6d76..65e55cc 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -644,3 +644,25 @@ 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 display_len = utf8_strnwidth(s, strlen(s), 0);\n+\tint utf8_compenstation = strlen(s) - display_len;\n+\n+\tif (!strlen(s))\n+\t\treturn;\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_compenstation, 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_compenstation, s);\n+\t} else if (position == ALIGN_RIGHT)\n+\t\tstrbuf_addf(buf, \"%*s\", width + utf8_compenstation, s);\n+}\ndiff --git a/utf8.h b/utf8.h\nindex 5a9e94b..db8ca63 100644\n--- a/utf8.h\n+++ b/utf8.h\n@@ -55,4 +55,17 @@ 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 type\n+ * and width.\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":"267683","messageId":"1439129506-9989-6-git-send-email-Karthik.188@gmail.com","threadId":"40040","inReplyTo":"1439129506-9989-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v10 05/13] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-09T14:11:38Z","receivedAt":"2015-08-09T14:11:38Z","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 `:<position>,<width>`, where the `<position>` is\neither left, right or middle and `<width>` 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\nThis is done by calling the strbuf_utf8_align() function in utf8.c and\ncurrently does not support nested alignment.\n\nAdd documentation and tests for the same.\n\nMentored-by: Christian Couder <christian.couder@gmail.com>\nMentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n Documentation/git-for-each-ref.txt |  9 +++++\n ref-filter.c                       | 72 +++++++++++++++++++++++++++++++++++---\n ref-filter.h                       |  1 +\n t/t6302-for-each-ref-filter.sh     | 48 +++++++++++++++++++++++++\n 4 files changed, 125 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex e49d578..d949812 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 `:<colorname>`, where names\n \tare described in `color.branch.*`.\n \n+align::\n+\tImplement an `align` atom which left-, middle-, or\n+\tright-aligns the content between %(align:..)  and\n+\t%(end). Followed by `:<position>,<width>`, where the\n+\t`<position>` is either left, right or middle and `<width>` 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. Currently nested alignment is not supported.\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 e2df209..57e988c 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,11 +54,21 @@ 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 struct atom_value{\n \tconst char *s;\n+\tstruct align *align;\n \tunsigned long ul; /* used for sorting when not FIELD_STR */\n+\tunsigned int modifier_atom : 1,\n+\t\tend : 1;\n };\n \n /*\n@@ -627,6 +638,7 @@ static void populate_value(struct ref_array_item *ref)\n \t\tint deref = 0;\n \t\tconst char *refname;\n \t\tconst char *formatp;\n+\t\tconst char *valp;\n \t\tstruct branch *branch = NULL;\n \n \t\tif (*name == '*') {\n@@ -692,6 +704,26 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\telse\n \t\t\t\tv->s = \" \";\n \t\t\tcontinue;\n+\t\t} else if (skip_prefix(name, \"align:\", &valp)) {\n+\t\t\tstruct align *align = xmalloc(sizeof(struct align));\n+\n+\t\t\tif (skip_prefix(valp, \"left,\", &valp))\n+\t\t\t\talign->position = ALIGN_LEFT;\n+\t\t\telse if (skip_prefix(valp, \"right,\", &valp))\n+\t\t\t\talign->position = ALIGN_RIGHT;\n+\t\t\telse if (skip_prefix(valp, \"middle,\", &valp))\n+\t\t\t\talign->position = ALIGN_MIDDLE;\n+\t\t\telse\n+\t\t\t\tdie(_(\"improper format entered align:%s\"), valp);\n+\t\t\tif (strtoul_ui(valp, 10, &align->width))\n+\t\t\t\tdie(_(\"positive width expected align:%s\"), valp);\n+\t\t\tv->align = align;\n+\t\t\tv->modifier_atom = 1;\n+\t\t\tcontinue;\n+\t\t} else if (!strcmp(name, \"end\")) {\n+\t\t\tv->end = 1;\n+\t\t\tv->modifier_atom = 1;\n+\t\t\tcontinue;\n \t\t} else\n \t\t\tcontinue;\n \n@@ -1198,7 +1230,9 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)\n \n struct ref_formatting_state {\n \tstruct strbuf output;\n+\tstruct align *align;\n \tint quote_style;\n+\tunsigned int end : 1;\n };\n \n static void format_quote_value(struct atom_value *v, struct ref_formatting_state *state)\n@@ -1262,12 +1296,31 @@ static void append_non_atom(const char *cp, const char *ep, struct ref_formattin\n \n static void set_formatting_state(struct atom_value *atomv, struct ref_formatting_state *state)\n {\n-\t/* Based on the atomv values, the formatting state is set */\n+\tif (atomv->align) {\n+\t\tstate->align = atomv->align;\n+\t\tatomv->align = NULL;\n+\t}\n+\tif (atomv->end)\n+\t\tstate->end = 1;\n+}\n+\n+static int align_ref_strbuf(struct ref_formatting_state *state, struct strbuf *final)\n+{\n+\tif (state->align && state->end) {\n+\t\tstruct align *align = state->align;\n+\t\tstrbuf_utf8_align(final, align->position, align->width, state->output.buf);\n+\t\tstrbuf_reset(&state->output);\n+\t\tstate->align = NULL;\n+\t\treturn 1;\n+\t} else if (state->align)\n+\t\treturn 1;\n+\treturn 0;\n }\n \n static void perform_state_formatting(struct ref_formatting_state *state, struct strbuf *final)\n {\n-\t/* More formatting options to be eventually added */\n+\tif (align_ref_strbuf(state, final))\n+\t\treturn;\n \tstrbuf_addbuf(final, &state->output);\n \tstrbuf_reset(&state->output);\n }\n@@ -1280,6 +1333,8 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n \n \tstate.quote_style = quote_style;\n \tstrbuf_init(&state.output, 0);\n+\tstate.align = NULL;\n+\tstate.end = 0;\n \n \tfor (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {\n \t\tstruct atom_value *atomv;\n@@ -1290,10 +1345,17 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n \t\t\tperform_state_formatting(&state, &final_buf);\n \t\t}\n \t\tget_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);\n-\t\tset_formatting_state(atomv, &state);\n-\t\tformat_quote_value(atomv, &state);\n-\t\tperform_state_formatting(&state, &final_buf);\n+\t\tif (atomv->modifier_atom)\n+\t\t\tset_formatting_state(atomv, &state);\n+\t\telse {\n+\t\t\tformat_quote_value(atomv, &state);\n+\t\t\tperform_state_formatting(&state, &final_buf);\n+\t\t}\n+\t\tif (atomv->end)\n+\t\t\tperform_state_formatting(&state, &final_buf);\n \t}\n+\tif (state.align)\n+\t\tdie(_(\"format: align used without an `end` atom\"));\n \tif (*cp) {\n \t\tsp = cp + strlen(cp);\n \t\tappend_non_atom(cp, sp, &state);\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 45026d0..144a633 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -5,6 +5,7 @@\n #include \"refs.h\"\n #include \"commit.h\"\n #include \"parse-options.h\"\n+#include \"utf8.h\"\n \n /* Quoting styles */\n #define QUOTE_NONE 0\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 505a360..f6b921b 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -81,4 +81,52 @@ test_expect_success 'filtering with --contains' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'left alignment' '\n+\tcat >expect <<-\\EOF &&\n+\trefname is refs/heads/master  |refs/heads/master\n+\trefname is refs/heads/side    |refs/heads/side\n+\trefname is refs/odd/spot      |refs/odd/spot\n+\trefname is refs/tags/double-tag|refs/tags/double-tag\n+\trefname is refs/tags/four     |refs/tags/four\n+\trefname is refs/tags/one      |refs/tags/one\n+\trefname is refs/tags/signed-tag|refs/tags/signed-tag\n+\trefname is refs/tags/three    |refs/tags/three\n+\trefname is refs/tags/two      |refs/tags/two\n+\tEOF\n+\tgit for-each-ref --format=\"%(align:left,30)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'middle alignment' '\n+\tcat >expect <<-\\EOF &&\n+\t| refname is refs/heads/master |refs/heads/master\n+\t|  refname is refs/heads/side  |refs/heads/side\n+\t|   refname is refs/odd/spot   |refs/odd/spot\n+\t|refname is refs/tags/double-tag|refs/tags/double-tag\n+\t|  refname is refs/tags/four   |refs/tags/four\n+\t|   refname is refs/tags/one   |refs/tags/one\n+\t|refname is refs/tags/signed-tag|refs/tags/signed-tag\n+\t|  refname is refs/tags/three  |refs/tags/three\n+\t|   refname is refs/tags/two   |refs/tags/two\n+\tEOF\n+\tgit for-each-ref --format=\"|%(align:middle,30)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'right alignment' '\n+\tcat >expect <<-\\EOF &&\n+\t|  refname is refs/heads/master|refs/heads/master\n+\t|    refname is refs/heads/side|refs/heads/side\n+\t|      refname is refs/odd/spot|refs/odd/spot\n+\t|refname is refs/tags/double-tag|refs/tags/double-tag\n+\t|     refname is refs/tags/four|refs/tags/four\n+\t|      refname is refs/tags/one|refs/tags/one\n+\t|refname is refs/tags/signed-tag|refs/tags/signed-tag\n+\t|    refname is refs/tags/three|refs/tags/three\n+\t|      refname is refs/tags/two|refs/tags/two\n+\tEOF\n+\tgit for-each-ref --format=\"|%(align:right,30)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.5.0\n"},{"id":"267682","messageId":"1439129506-9989-7-git-send-email-Karthik.188@gmail.com","threadId":"40040","inReplyTo":"1439129506-9989-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v10 06/13] ref-filter: add option to filter only tags","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-09T14:11:39Z","receivedAt":"2015-08-09T14:11:39Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"From: Karthik Nayak <karthik.188@gmail.com>\n\nAdd a functions called 'for_each_tag_ref_fullpath()' to refs.{c,h}\nwhich iterates through each tag ref without trimming the path.\n\nAdd an option in 'filter_refs()' to use 'for_each_tag_ref_fullpath()'\nand filter refs. This type checking is done by adding a\n'FILTER_REFS_TAGS' in 'ref-filter.h'\n\nMentored-by: Christian Couder <christian.couder@gmail.com>\nMentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n ref-filter.c | 2 ++\n ref-filter.h | 1 +\n refs.c       | 5 +++++\n refs.h       | 1 +\n 4 files changed, 9 insertions(+)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 57e988c..fb5d74e 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1172,6 +1172,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int\n \t\tret = for_each_rawref(ref_filter_handler, &ref_cbdata);\n \telse if (type & FILTER_REFS_ALL)\n \t\tret = for_each_ref(ref_filter_handler, &ref_cbdata);\n+\telse if (type & FILTER_REFS_TAGS)\n+\t\tret = for_each_tag_ref_fullpath(ref_filter_handler, &ref_cbdata);\n \telse if (type)\n \t\tdie(\"filter_refs: invalid type\");\n \ndiff --git a/ref-filter.h b/ref-filter.h\nindex 144a633..1886baa 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -16,6 +16,7 @@\n \n #define FILTER_REFS_INCLUDE_BROKEN 0x1\n #define FILTER_REFS_ALL 0x2\n+#define FILTER_REFS_TAGS 0x4\n \n struct atom_value;\n \ndiff --git a/refs.c b/refs.c\nindex 2db2975..0103a88 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2114,6 +2114,11 @@ int for_each_tag_ref(each_ref_fn fn, void *cb_data)\n \treturn for_each_ref_in(\"refs/tags/\", fn, cb_data);\n }\n \n+int for_each_tag_ref_fullpath(each_ref_fn fn, void *cb_data)\n+{\n+\treturn do_for_each_ref(&ref_cache, \"refs/tags/\", fn, 0, 0, cb_data);\n+}\n+\n int for_each_tag_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)\n {\n \treturn for_each_ref_in_submodule(submodule, \"refs/tags/\", fn, cb_data);\ndiff --git a/refs.h b/refs.h\nindex 6a3fa6d..0956255 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -174,6 +174,7 @@ extern int head_ref(each_ref_fn fn, void *cb_data);\n extern int for_each_ref(each_ref_fn fn, void *cb_data);\n extern int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data);\n extern int for_each_tag_ref(each_ref_fn fn, void *cb_data);\n+extern int for_each_tag_ref_fullpath(each_ref_fn fn, void *cb_data);\n extern int for_each_branch_ref(each_ref_fn fn, void *cb_data);\n extern int for_each_remote_ref(each_ref_fn fn, void *cb_data);\n extern int for_each_replace_ref(each_ref_fn fn, void *cb_data);\n-- \n2.5.0\n"},{"id":"267687","messageId":"1439129506-9989-8-git-send-email-Karthik.188@gmail.com","threadId":"40040","inReplyTo":"1439129506-9989-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v10 07/13] ref-filter: support printing N lines from tag annotation","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-09T14:11:40Z","receivedAt":"2015-08-09T14:11:40Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"From: Karthik Nayak <karthik.188@gmail.com>\n\nIn 'tag.c' we can print N lines from the annotation of the tag using\nthe '-n<num>' option. Copy code from 'tag.c' to 'ref-filter' and\nmodify 'ref-filter' to support printing of N lines from the annotation\nof tags.\n\nMentored-by: Christian Couder <christian.couder@gmail.com>\nMentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n builtin/for-each-ref.c |  2 +-\n builtin/tag.c          |  4 ++++\n ref-filter.c           | 55 ++++++++++++++++++++++++++++++++++++++++++++++++--\n ref-filter.h           |  9 +++++++--\n 4 files changed, 65 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 40f343b..e4a4f8a 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -74,7 +74,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \tif (!maxcount || array.nr < maxcount)\n \t\tmaxcount = array.nr;\n \tfor (i = 0; i < maxcount; i++)\n-\t\tshow_ref_array_item(array.items[i], format, quote_style);\n+\t\tshow_ref_array_item(array.items[i], format, quote_style, 0);\n \tref_array_clear(&array);\n \treturn 0;\n }\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 471d6b1..0fc7557 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -185,6 +185,10 @@ static enum contains_result contains(struct commit *candidate,\n \treturn contains_test(candidate, want);\n }\n \n+/*\n+ * Currently duplicated in ref-filter, will eventually be removed as\n+ * we port tag.c to use ref-filter APIs.\n+ */\n static void show_tag_lines(const struct object_id *oid, int lines)\n {\n \tint i;\ndiff --git a/ref-filter.c b/ref-filter.c\nindex fb5d74e..2042962 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1327,7 +1327,51 @@ static void perform_state_formatting(struct ref_formatting_state *state, struct\n \tstrbuf_reset(&state->output);\n }\n \n-void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)\n+/*\n+ * If 'lines' is greater than 0, print that many lines from the given\n+ * object_id 'oid'.\n+ */\n+static void show_tag_lines(const struct object_id *oid, int lines)\n+{\n+\tint i;\n+\tunsigned long size;\n+\tenum object_type type;\n+\tchar *buf, *sp, *eol;\n+\tsize_t len;\n+\n+\tbuf = read_sha1_file(oid->hash, &type, &size);\n+\tif (!buf)\n+\t\tdie_errno(\"unable to read object %s\", oid_to_hex(oid));\n+\tif (type != OBJ_COMMIT && type != OBJ_TAG)\n+\t\tgoto free_return;\n+\tif (!size)\n+\t\tdie(\"an empty %s object %s?\",\n+\t\t    typename(type), oid_to_hex(oid));\n+\n+\t/* skip header */\n+\tsp = strstr(buf, \"\\n\\n\");\n+\tif (!sp)\n+\t\tgoto free_return;\n+\n+\t/* only take up to \"lines\" lines, and strip the signature from a tag */\n+\tif (type == OBJ_TAG)\n+\t\tsize = parse_signature(buf, size);\n+\tfor (i = 0, sp += 2; i < lines && sp < buf + size; i++) {\n+\t\tif (i)\n+\t\t\tprintf(\"\\n    \");\n+\t\teol = memchr(sp, '\\n', size - (sp - buf));\n+\t\tlen = eol ? eol - sp : size - (sp - buf);\n+\t\tfwrite(sp, len, 1, stdout);\n+\t\tif (!eol)\n+\t\t\tbreak;\n+\t\tsp = eol + 1;\n+\t}\n+free_return:\n+\tfree(buf);\n+}\n+\n+void show_ref_array_item(struct ref_array_item *info, const char *format,\n+\t\t\t int quote_style, unsigned int lines)\n {\n \tconst char *cp, *sp, *ep;\n \tstruct strbuf final_buf = STRBUF_INIT;\n@@ -1373,10 +1417,17 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n \t\tformat_quote_value(&resetv, &state);\n \t\tperform_state_formatting(&state, &final_buf);\n \t}\n+\n \tfwrite(final_buf.buf, 1, final_buf.len, stdout);\n-\tputchar('\\n');\n \tstrbuf_release(&final_buf);\n \tstrbuf_release(&state.output);\n+\n+\tif (lines > 0) {\n+\t\tstruct object_id oid;\n+\t\thashcpy(oid.hash, info->objectname);\n+\t\tshow_tag_lines(&oid, lines);\n+\t}\n+\tputchar('\\n');\n }\n \n /*  If no sorting option is given, use refname to sort as default */\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 1886baa..72e21bb 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -53,6 +53,7 @@ struct ref_filter {\n \tstruct commit *merge_commit;\n \n \tunsigned int with_commit_tag_algo : 1;\n+\tunsigned int lines;\n };\n \n struct ref_filter_cbdata {\n@@ -84,8 +85,12 @@ int parse_ref_filter_atom(const char *atom, const char *ep);\n int verify_ref_format(const char *format);\n /*  Sort the given ref_array as per the ref_sorting provided */\n void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);\n-/*  Print the ref using the given format and quote_style */\n-void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style);\n+/*\n+ * Print the ref using the given format and quote_style. If 'lines' > 0,\n+ * print that many lines of the the given ref.\n+ */\n+void show_ref_array_item(struct ref_array_item *info, const char *format,\n+\t\t\t int quote_style, unsigned int lines);\n /*  Callback function for parsing the sort option */\n int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset);\n /*  Default sort option based on refname */\n-- \n2.5.0\n"},{"id":"267685","messageId":"1439129506-9989-9-git-send-email-Karthik.188@gmail.com","threadId":"40040","inReplyTo":"1439129506-9989-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v10 08/13] ref-filter: add support to sort by version","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-09T14:11:41Z","receivedAt":"2015-08-09T14:11:41Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"From: Karthik Nayak <karthik.188@gmail.com>\n\nAdd support to sort by version using the \"v:refname\" and\n\"version:refname\" option. This is achieved by using the 'versioncmp()'\nfunction as the comparing function for qsort.\n\nThis option is included to support sorting by versions in `git tag -l`\nwhich will eventaully be ported to use ref-filter APIs.\n\nAdd documentation and tests for the same.\n\nMentored-by: Christian Couder <christian.couder@gmail.com>\nMentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\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 d949812..a795d5f 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -154,6 +154,9 @@ For sorting purposes, fields with numeric values sort in numeric\n order (`objectsize`, `authordate`, `committerdate`, `taggerdate`).\n All other fields are used to sort in their byte-value order.\n \n+There is also an option to sort by versions, this can be done by using\n+the fieldname `version:refname` or its alias `v:refname`.\n+\n In any case, a field name that refers to a field inapplicable to\n the object referred by the ref does not cause an error.  It\n returns an empty string instead.\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 2042962..abb17d7 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@@ -1192,19 +1194,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@@ -1459,6 +1461,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 72e21bb..e824754 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -23,7 +23,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 f6b921b..ce455d9 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -129,4 +129,40 @@ test_expect_success 'right alignment' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'setup for version sort' '\n+\ttest_commit foo1.3 &&\n+\ttest_commit foo1.6 &&\n+\ttest_commit foo1.10\n+'\n+\n+test_expect_success 'version sort' '\n+\tgit for-each-ref --sort=version:refname --format=\"%(refname:short)\" refs/tags/ | grep \"foo\" >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfoo1.3\n+\tfoo1.6\n+\tfoo1.10\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'version sort (shortened)' '\n+\tgit for-each-ref --sort=v:refname --format=\"%(refname:short)\" refs/tags/ | grep \"foo\" >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfoo1.3\n+\tfoo1.6\n+\tfoo1.10\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'reverse version sort' '\n+\tgit for-each-ref --sort=-version:refname --format=\"%(refname:short)\" refs/tags/ | grep \"foo\" >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfoo1.10\n+\tfoo1.6\n+\tfoo1.3\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.5.0\n"},{"id":"267686","messageId":"1439129506-9989-10-git-send-email-Karthik.188@gmail.com","threadId":"40040","inReplyTo":"1439129506-9989-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v10 09/13] ref-filter: add option to match literal pattern","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-09T14:11:42Z","receivedAt":"2015-08-09T14:11:42Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"From: Karthik Nayak <karthik.188@gmail.com>\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 <christian.couder@gmail.com>\nMentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n builtin/for-each-ref.c |  1 +\n ref-filter.c           | 39 ++++++++++++++++++++++++++++++++++++---\n ref-filter.h           |  3 ++-\n 3 files changed, 39 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex e4a4f8a..3ad6a64 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -68,6 +68,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \tgit_config(git_default_config, NULL);\n \n \tfilter.name_patterns = argv;\n+\tfilter.match_as_path = 1;\n \tfilter_refs(&array, &filter, FILTER_REFS_ALL | FILTER_REFS_INCLUDE_BROKEN);\n \tref_array_sort(sorting, &array);\n \ndiff --git a/ref-filter.c b/ref-filter.c\nindex abb17d7..d22bf66 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -967,9 +967,32 @@ static int commit_contains(struct ref_filter *filter, struct commit *commit)\n \n /*\n  * Return 1 if the refname matches one of the patterns, otherwise 0.\n+ * A pattern can be a literal prefix (e.g. a refname \"refs/heads/master\"\n+ * matches a pattern \"refs/heads/mas\") or a wildcard (e.g. the same ref\n+ * matches \"refs/heads/mas*\", too).\n+ */\n+static int match_pattern(const char **patterns, const char *refname)\n+{\n+\t/*\n+\t * When no '--format' option is given we need to skip the prefix\n+\t * for matching refs of tags and branches.\n+\t */\n+\t(void)(skip_prefix(refname, \"refs/tags/\", &refname) ||\n+\t       skip_prefix(refname, \"refs/heads/\", &refname) ||\n+\t       skip_prefix(refname, \"refs/remotes/\", &refname));\n+\n+\tfor (; *patterns; patterns++) {\n+\t\tif (!wildmatch(*patterns, refname, 0, NULL))\n+\t\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n+/*\n+ * Return 1 if the refname matches one of the patterns, otherwise 0.\n  * A pattern can be path prefix (e.g. a refname \"refs/heads/master\"\n- * matches a pattern \"refs/heads/\") or a wildcard (e.g. the same ref\n- * matches \"refs/heads/m*\",too).\n+ * matches a pattern \"refs/heads/\" but not \"refs/heads/m\") or a\n+ * wildcard (e.g. the same ref matches \"refs/heads/m*\", too).\n  */\n static int match_name_as_path(const char **pattern, const char *refname)\n {\n@@ -990,6 +1013,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@@ -1058,7 +1091,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n \t\treturn 0;\n \t}\n \n-\tif (*filter->name_patterns && !match_name_as_path(filter->name_patterns, refname))\n+\tif (!filter_pattern_match(filter, refname))\n \t\treturn 0;\n \n \tif (filter->points_at.nr && !match_points_at(&filter->points_at, oid->hash, refname))\ndiff --git a/ref-filter.h b/ref-filter.h\nindex e824754..75bf989 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -53,7 +53,8 @@ struct ref_filter {\n \t} merge;\n \tstruct commit *merge_commit;\n \n-\tunsigned int with_commit_tag_algo : 1;\n+\tunsigned int with_commit_tag_algo : 1,\n+\t\tmatch_as_path : 1;\n \tunsigned int lines;\n };\n \n-- \n2.5.0\n"},{"id":"267684","messageId":"1439129506-9989-11-git-send-email-Karthik.188@gmail.com","threadId":"40040","inReplyTo":"1439129506-9989-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v10 10/13] tag.c: use 'ref-filter' data structures","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-09T14:11:43Z","receivedAt":"2015-08-09T14:11:43Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"From: Karthik Nayak <karthik.188@gmail.com>\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 <christian.couder@gmail.com>\nMentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\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 <key-id>] [-f] [-m <msg> | -F <file>] <tagname> [<head>]\"),\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 <n> 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":"267688","messageId":"1439129846-10612-1-git-send-email-Karthik.188@gmail.com","threadId":"40040","inReplyTo":"1439129506-9989-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v10 10/13] tag.c: use 'ref-filter' data structures","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-09T14:17:23Z","receivedAt":"2015-08-09T14:17:23Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"From: Karthik Nayak <karthik.188@gmail.com>\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 <christian.couder@gmail.com>\nMentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\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 <key-id>] [-f] [-m <msg> | -F <file>] <tagname> [<head>]\"),\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 <n> 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":"267689","messageId":"1439130761-15721-1-git-send-email-Karthik.188@gmail.com","threadId":"40040","inReplyTo":"1439129506-9989-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v10 11/13] tag.c: use 'ref-filter' APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-09T14:32:39Z","receivedAt":"2015-08-09T14:32:39Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"From: Karthik Nayak <karthik.188@gmail.com>\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 <christian.couder@gmail.com>\nMentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n Documentation/git-tag.txt |  16 ++-\n builtin/tag.c             | 342 ++++++----------------------------------------\n t/t7004-tag.sh            |   8 +-\n 3 files changed, 50 insertions(+), 316 deletions(-)\n\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 84f6496..3ac4a96 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -13,7 +13,7 @@ SYNOPSIS\n \t<tagname> [<commit> | <object>]\n 'git tag' -d <tagname>...\n 'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]\n-\t[--column[=<options>] | --no-column] [--create-reflog] [<pattern>...]\n+\t[--column[=<options>] | --no-column] [--create-reflog] [--sort=<key>] [<pattern>...]\n 'git tag' -v <tagname>...\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=<type>::\n-\tSort in a specific order. Supported type is \"refname\"\n-\t(lexicographic order), \"version:refname\" or \"v:refname\" (tag\n+--sort=<key>::\n+\tSort based on the key given.  Prefix `-` to sort in\n+\tdescending order of the value. You may use the --sort=<key> 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..e6a9a56 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -28,278 +28,32 @@ static const char * const git_tag_usage[] = {\n \tNULL\n };\n \n-#define STRCMP_SORT     0\t/* must be zero */\n-#define VERCMP_SORT     1\n-#define SORT_MASK       0x7fff\n-#define REVERSE_SORT    0x8000\n-\n-static int tag_sort;\n-\n static unsigned int colopts;\n \n-static int match_pattern(const char **patterns, const char *ref)\n-{\n-\t/* no pattern means match everything */\n-\tif (!*patterns)\n-\t\treturn 1;\n-\tfor (; *patterns; patterns++)\n-\t\tif (!wildmatch(*patterns, ref, 0, NULL))\n-\t\t\treturn 1;\n-\treturn 0;\n-}\n-\n-/*\n- * This is currently duplicated in ref-filter.c, and will eventually be\n- * removed as we port tag.c to use the ref-filter APIs.\n- */\n-static const unsigned char *match_points_at(const char *refname,\n-\t\t\t\t\t    const unsigned char *sha1,\n-\t\t\t\t\t    struct sha1_array *points_at)\n-{\n-\tconst unsigned char *tagged_sha1 = NULL;\n-\tstruct object *obj;\n-\n-\tif (sha1_array_lookup(points_at, sha1) >= 0)\n-\t\treturn sha1;\n-\tobj = parse_object(sha1);\n-\tif (!obj)\n-\t\tdie(_(\"malformed object at '%s'\"), refname);\n-\tif (obj->type == OBJ_TAG)\n-\t\ttagged_sha1 = ((struct tag *)obj)->tagged->sha1;\n-\tif (tagged_sha1 && sha1_array_lookup(points_at, tagged_sha1) >= 0)\n-\t\treturn tagged_sha1;\n-\treturn NULL;\n-}\n-\n-static int in_commit_list(const struct commit_list *want, struct commit *c)\n-{\n-\tfor (; want; want = want->next)\n-\t\tif (!hashcmp(want->item->object.sha1, c->object.sha1))\n-\t\t\treturn 1;\n-\treturn 0;\n-}\n-\n-/*\n- * The entire code segment for supporting the --contains option has been\n- * copied over to ref-filter.{c,h}. This will be deleted evetually when\n- * we port tag.c to use ref-filter APIs.\n- */\n-enum contains_result {\n-\tCONTAINS_UNKNOWN = -1,\n-\tCONTAINS_NO = 0,\n-\tCONTAINS_YES = 1\n-};\n-\n-/*\n- * Test whether the candidate or one of its parents is contained in the list.\n- * Do not recurse to find out, though, but return -1 if inconclusive.\n- */\n-static enum contains_result contains_test(struct commit *candidate,\n-\t\t\t    const struct commit_list *want)\n-{\n-\t/* was it previously marked as containing a want commit? */\n-\tif (candidate->object.flags & TMP_MARK)\n-\t\treturn 1;\n-\t/* or marked as not possibly containing a want commit? */\n-\tif (candidate->object.flags & UNINTERESTING)\n-\t\treturn 0;\n-\t/* or are we it? */\n-\tif (in_commit_list(want, candidate)) {\n-\t\tcandidate->object.flags |= TMP_MARK;\n-\t\treturn 1;\n-\t}\n-\n-\tif (parse_commit(candidate) < 0)\n-\t\treturn 0;\n-\n-\treturn -1;\n-}\n-\n-/*\n- * Mimicking the real stack, this stack lives on the heap, avoiding stack\n- * overflows.\n- *\n- * At each recursion step, the stack items points to the commits whose\n- * ancestors are to be inspected.\n- */\n-struct stack {\n-\tint nr, alloc;\n-\tstruct stack_entry {\n-\t\tstruct commit *commit;\n-\t\tstruct commit_list *parents;\n-\t} *stack;\n-};\n-\n-static void push_to_stack(struct commit *candidate, struct stack *stack)\n-{\n-\tint index = stack->nr++;\n-\tALLOC_GROW(stack->stack, stack->nr, stack->alloc);\n-\tstack->stack[index].commit = candidate;\n-\tstack->stack[index].parents = candidate->parents;\n-}\n-\n-static enum contains_result contains(struct commit *candidate,\n-\t\tconst struct commit_list *want)\n-{\n-\tstruct stack stack = { 0, 0, NULL };\n-\tint result = contains_test(candidate, want);\n-\n-\tif (result != CONTAINS_UNKNOWN)\n-\t\treturn result;\n-\n-\tpush_to_stack(candidate, &stack);\n-\twhile (stack.nr) {\n-\t\tstruct stack_entry *entry = &stack.stack[stack.nr - 1];\n-\t\tstruct commit *commit = entry->commit;\n-\t\tstruct commit_list *parents = entry->parents;\n-\n-\t\tif (!parents) {\n-\t\t\tcommit->object.flags |= UNINTERESTING;\n-\t\t\tstack.nr--;\n-\t\t}\n-\t\t/*\n-\t\t * If we just popped the stack, parents->item has been marked,\n-\t\t * therefore contains_test will return a meaningful 0 or 1.\n-\t\t */\n-\t\telse switch (contains_test(parents->item, want)) {\n-\t\tcase CONTAINS_YES:\n-\t\t\tcommit->object.flags |= TMP_MARK;\n-\t\t\tstack.nr--;\n-\t\t\tbreak;\n-\t\tcase CONTAINS_NO:\n-\t\t\tentry->parents = parents->next;\n-\t\t\tbreak;\n-\t\tcase CONTAINS_UNKNOWN:\n-\t\t\tpush_to_stack(parents->item, &stack);\n-\t\t\tbreak;\n-\t\t}\n-\t}\n-\tfree(stack.stack);\n-\treturn contains_test(candidate, want);\n-}\n-\n-/*\n- * Currently duplicated in ref-filter, will eventually be removed as\n- * we port tag.c to use ref-filter APIs.\n- */\n-static void show_tag_lines(const struct object_id *oid, int lines)\n-{\n-\tint i;\n-\tunsigned long size;\n-\tenum object_type type;\n-\tchar *buf, *sp, *eol;\n-\tsize_t len;\n-\n-\tbuf = read_sha1_file(oid->hash, &type, &size);\n-\tif (!buf)\n-\t\tdie_errno(\"unable to read object %s\", oid_to_hex(oid));\n-\tif (type != OBJ_COMMIT && type != OBJ_TAG)\n-\t\tgoto free_return;\n-\tif (!size)\n-\t\tdie(\"an empty %s object %s?\",\n-\t\t    typename(type), oid_to_hex(oid));\n-\n-\t/* skip header */\n-\tsp = strstr(buf, \"\\n\\n\");\n-\tif (!sp)\n-\t\tgoto free_return;\n-\n-\t/* only take up to \"lines\" lines, and strip the signature from a tag */\n-\tif (type == OBJ_TAG)\n-\t\tsize = parse_signature(buf, size);\n-\tfor (i = 0, sp += 2; i < lines && sp < buf + size; i++) {\n-\t\tif (i)\n-\t\t\tprintf(\"\\n    \");\n-\t\teol = memchr(sp, '\\n', size - (sp - buf));\n-\t\tlen = eol ? eol - sp : size - (sp - buf);\n-\t\tfwrite(sp, len, 1, stdout);\n-\t\tif (!eol)\n-\t\t\tbreak;\n-\t\tsp = eol + 1;\n-\t}\n-free_return:\n-\tfree(buf);\n-}\n-\n-static void ref_array_append(struct ref_array *array, const char *refname)\n-{\n-\tsize_t len = strlen(refname);\n-\tstruct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);\n-\tmemcpy(ref->refname, refname, len);\n-\tref->refname[len] = '\\0';\n-\tREALLOC_ARRAY(array->items, array->nr + 1);\n-\tarray->items[array->nr++] = ref;\n-}\n-\n-static int show_reference(const char *refname, const struct object_id *oid,\n-\t\t\t  int flag, void *cb_data)\n-{\n-\tstruct ref_filter_cbdata *data = cb_data;\n-\tstruct ref_array *array = data->array;\n-\tstruct ref_filter *filter = data->filter;\n-\n-\tif (match_pattern(filter->name_patterns, refname)) {\n-\t\tif (filter->with_commit) {\n-\t\t\tstruct commit *commit;\n-\n-\t\t\tcommit = lookup_commit_reference_gently(oid->hash, 1);\n-\t\t\tif (!commit)\n-\t\t\t\treturn 0;\n-\t\t\tif (!contains(commit, filter->with_commit))\n-\t\t\t\treturn 0;\n-\t\t}\n-\n-\t\tif (filter->points_at.nr && !match_points_at(refname, oid->hash, &filter->points_at))\n-\t\t\treturn 0;\n-\n-\t\tif (!filter->lines) {\n-\t\t\tif (tag_sort)\n-\t\t\t\tref_array_append(array, refname);\n-\t\t\telse\n-\t\t\t\tprintf(\"%s\\n\", refname);\n-\t\t\treturn 0;\n-\t\t}\n-\t\tprintf(\"%-15s \", refname);\n-\t\tshow_tag_lines(oid, filter->lines);\n-\t\tputchar('\\n');\n-\t}\n-\n-\treturn 0;\n-}\n-\n-static int sort_by_version(const void *a_, const void *b_)\n-{\n-\tconst struct ref_array_item *a = *((struct ref_array_item **)a_);\n-\tconst struct ref_array_item *b = *((struct ref_array_item **)b_);\n-\treturn versioncmp(a->refname, b->refname);\n-}\n-\n-static int list_tags(struct ref_filter *filter, int sort)\n+static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting)\n {\n \tstruct ref_array array;\n-\tstruct ref_filter_cbdata data;\n+\tchar *format;\n+\tint i;\n \n \tmemset(&array, 0, sizeof(array));\n-\tdata.array = &array;\n-\tdata.filter = filter;\n \n \tif (filter->lines == -1)\n \t\tfilter->lines = 0;\n \n-\tfor_each_tag_ref(show_reference, &data);\n-\tif (sort) {\n-\t\tint i;\n-\t\tif ((sort & SORT_MASK) == VERCMP_SORT)\n-\t\t\tqsort(array.items, array.nr,\n-\t\t\t      sizeof(struct ref_array_item *), sort_by_version);\n-\t\tif (sort & REVERSE_SORT)\n-\t\t\tfor (i = array.nr - 1; i >= 0; i--)\n-\t\t\t\tprintf(\"%s\\n\", array.items[i]->refname);\n-\t\telse\n-\t\t\tfor (i = 0; i < array.nr; i++)\n-\t\t\t\tprintf(\"%s\\n\", array.items[i]->refname);\n-\t\tref_array_clear(&array);\n-\t}\n+\tif (filter->lines)\n+\t\tformat = \"%(align:left,16)%(refname:short)%(end)\";\n+\telse\n+\t\tformat = \"%(refname:short)\";\n+\n+\tverify_ref_format(format);\n+\tfilter_refs(&array, filter, FILTER_REFS_TAGS);\n+\tref_array_sort(sorting, &array);\n+\n+\tfor (i = 0; i < array.nr; i++)\n+\t\tshow_ref_array_item(array.items[i], format, QUOTE_NONE, filter->lines);\n+\tref_array_clear(&array);\n+\n \treturn 0;\n }\n \n@@ -366,35 +120,26 @@ static const char tag_template_nocleanup[] =\n \t\"Lines starting with '%c' will be kept; you may remove them\"\n \t\" yourself if you want to.\\n\");\n \n-/*\n- * Parse a sort string, and return 0 if parsed successfully. Will return\n- * non-zero when the sort string does not parse into a known type. If var is\n- * given, the error message becomes a warning and includes information about\n- * the configuration value.\n- */\n-static int parse_sort_string(const char *var, const char *arg, int *sort)\n+/* Parse arg given and add it the ref_sorting array */\n+static int parse_sorting_string(const char *arg, struct ref_sorting **sorting_tail)\n {\n-\tint type = 0, flags = 0;\n-\n-\tif (skip_prefix(arg, \"-\", &arg))\n-\t\tflags |= REVERSE_SORT;\n+\tstruct ref_sorting *s;\n+\tint len;\n \n-\tif (skip_prefix(arg, \"version:\", &arg) || skip_prefix(arg, \"v:\", &arg))\n-\t\ttype = VERCMP_SORT;\n-\telse\n-\t\ttype = STRCMP_SORT;\n+\ts = xcalloc(1, sizeof(*s));\n+\ts->next = *sorting_tail;\n+\t*sorting_tail = s;\n \n-\tif (strcmp(arg, \"refname\")) {\n-\t\tif (!var)\n-\t\t\treturn error(_(\"unsupported sort specification '%s'\"), arg);\n-\t\telse {\n-\t\t\twarning(_(\"unsupported sort specification '%s' in variable '%s'\"),\n-\t\t\t\tvar, arg);\n-\t\t\treturn -1;\n-\t\t}\n+\tif (*arg == '-') {\n+\t\ts->reverse = 1;\n+\t\targ++;\n \t}\n+\tif (skip_prefix(arg, \"version:\", &arg) ||\n+\t    skip_prefix(arg, \"v:\", &arg))\n+\t\ts->version = 1;\n \n-\t*sort = (type | flags);\n+\tlen = strlen(arg);\n+\ts->atom = parse_ref_filter_atom(arg, arg+len);\n \n \treturn 0;\n }\n@@ -402,11 +147,12 @@ static int parse_sort_string(const char *var, const char *arg, int *sort)\n static int git_tag_config(const char *var, const char *value, void *cb)\n {\n \tint status;\n+\tstruct ref_sorting **sorting_tail = (struct ref_sorting **)cb;\n \n \tif (!strcmp(var, \"tag.sort\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n-\t\tparse_sort_string(var, value, &tag_sort);\n+\t\tparse_sorting_string(value, sorting_tail);\n \t\treturn 0;\n \t}\n \n@@ -564,13 +310,6 @@ static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)\n \treturn check_refname_format(sb->buf, 0);\n }\n \n-static int parse_opt_sort(const struct option *opt, const char *arg, int unset)\n-{\n-\tint *sort = opt->value;\n-\n-\treturn parse_sort_string(NULL, arg, sort);\n-}\n-\n int cmd_tag(int argc, const char **argv, const char *prefix)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -587,6 +326,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tstruct ref_transaction *transaction;\n \tstruct strbuf err = STRBUF_INIT;\n \tstruct ref_filter filter;\n+\tstatic struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;\n \tstruct option options[] = {\n \t\tOPT_CMDMODE('l', \"list\", &cmdmode, N_(\"list tag names\"), 'l'),\n \t\t{ OPTION_INTEGER, 'n', NULL, &filter.lines, N_(\"n\"),\n@@ -613,10 +353,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tOPT_COLUMN(0, \"column\", &colopts, N_(\"show tag list in columns\")),\n \t\tOPT_CONTAINS(&filter.with_commit, N_(\"print only tags that contain the commit\")),\n \t\tOPT_WITH(&filter.with_commit, N_(\"print only tags that contain the commit\")),\n-\t\t{\n-\t\t\tOPTION_CALLBACK, 0, \"sort\", &tag_sort, N_(\"type\"), N_(\"sort tags\"),\n-\t\t\tPARSE_OPT_NONEG, parse_opt_sort\n-\t\t},\n+\t\tOPT_CALLBACK(0 , \"sort\", sorting_tail, N_(\"key\"),\n+\t\t\t     N_(\"field name to sort on\"), &parse_opt_ref_sorting),\n \t\t{\n \t\t\tOPTION_CALLBACK, 0, \"points-at\", &filter.points_at, N_(\"object\"),\n \t\t\tN_(\"print only tags of the object\"), 0, parse_opt_object_name\n@@ -624,7 +362,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n-\tgit_config(git_tag_config, NULL);\n+\tgit_config(git_tag_config, sorting_tail);\n \n \tmemset(&opt, 0, sizeof(opt));\n \tmemset(&filter, 0, sizeof(filter));\n@@ -650,6 +388,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"--column and -n are incompatible\"));\n \t\tcolopts = 0;\n \t}\n+\tif (!sorting)\n+\t\tsorting = ref_default_sorting();\n \tif (cmdmode == 'l') {\n \t\tint ret;\n \t\tif (column_active(colopts)) {\n@@ -658,10 +398,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\t\tcopts.padding = 2;\n \t\t\trun_column_filter(colopts, &copts);\n \t\t}\n-\t\tif (filter.lines != -1 && tag_sort)\n-\t\t\tdie(_(\"--sort and -n are incompatible\"));\n \t\tfilter.name_patterns = argv;\n-\t\tret = list_tags(&filter, tag_sort);\n+\t\tret = list_tags(&filter, sorting);\n \t\tif (column_active(colopts))\n \t\t\tstop_column_filter();\n \t\treturn ret;\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex d31788c..1f066aa 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1462,13 +1462,7 @@ test_expect_success 'invalid sort parameter on command line' '\n \n test_expect_success 'invalid sort parameter in configuratoin' '\n \tgit config tag.sort \"v:notvalid\" &&\n-\tgit tag -l \"foo*\" >actual &&\n-\tcat >expect <<-\\EOF &&\n-\tfoo1.10\n-\tfoo1.3\n-\tfoo1.6\n-\tEOF\n-\ttest_cmp expect actual\n+\ttest_must_fail git tag -l \"foo*\" >actual\n '\n \n test_expect_success 'version sort with prerelease reordering' '\n-- \n2.5.0\n"},{"id":"267690","messageId":"1439130761-15721-2-git-send-email-Karthik.188@gmail.com","threadId":"40040","inReplyTo":"1439129506-9989-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v10 12/13] tag.c: implement '--format' option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-09T14:32:40Z","receivedAt":"2015-08-09T14:32:40Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"From: Karthik Nayak <karthik.188@gmail.com>\n\nImplement the '--format' option provided by 'ref-filter'.\nThis lets the user list tags as per desired format similar\nto the implementation in 'git for-each-ref'.\n\nAdd tests and documentation for the same.\n\nMentored-by: Christian Couder <christian.couder@gmail.com>\nMentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n Documentation/git-tag.txt | 15 ++++++++++++++-\n builtin/tag.c             | 11 +++++++----\n t/t7004-tag.sh            | 16 ++++++++++++++++\n 3 files changed, 37 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 3ac4a96..75703c5 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -13,7 +13,8 @@ SYNOPSIS\n \t<tagname> [<commit> | <object>]\n 'git tag' -d <tagname>...\n 'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]\n-\t[--column[=<options>] | --no-column] [--create-reflog] [--sort=<key>] [<pattern>...]\n+\t[--column[=<options>] | --no-column] [--create-reflog] [--sort=<key>]\n+\t[--format=<format>] [<pattern>...]\n 'git tag' -v <tagname>...\n \n DESCRIPTION\n@@ -158,6 +159,18 @@ This option is only applicable when listing tags without annotation lines.\n \tThe object that the new tag will refer to, usually a commit.\n \tDefaults to HEAD.\n \n+<format>::\n+\tA string that interpolates `%(fieldname)` from the object\n+\tpointed at by a ref being shown.  If `fieldname` is prefixed\n+\twith an asterisk (`*`) and the ref points at a tag object, the\n+\tvalue for the field in the object tag refers is used.  When\n+\tunspecified, defaults to `%(refname:short)`.  It also\n+\tinterpolates `%%` to `%`, and `%xx` where `xx` are hex digits\n+\tinterpolates to character with hex code `xx`; for example\n+\t`%00` interpolates to `\\0` (NUL), `%09` to `\\t` (TAB) and\n+\t`%0a` to `\\n` (LF).  The fields are same as those in `git\n+\tfor-each-ref`.\n+\n \n CONFIGURATION\n -------------\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex e6a9a56..affb460 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -30,10 +30,9 @@ static const char * const git_tag_usage[] = {\n \n static unsigned int colopts;\n \n-static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting)\n+static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)\n {\n \tstruct ref_array array;\n-\tchar *format;\n \tint i;\n \n \tmemset(&array, 0, sizeof(array));\n@@ -43,7 +42,7 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting)\n \n \tif (filter->lines)\n \t\tformat = \"%(align:left,16)%(refname:short)%(end)\";\n-\telse\n+\telse if (!format)\n \t\tformat = \"%(refname:short)\";\n \n \tverify_ref_format(format);\n@@ -327,6 +326,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tstruct strbuf err = STRBUF_INIT;\n \tstruct ref_filter filter;\n \tstatic struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;\n+\tconst char *format = NULL;\n \tstruct option options[] = {\n \t\tOPT_CMDMODE('l', \"list\", &cmdmode, N_(\"list tag names\"), 'l'),\n \t\t{ OPTION_INTEGER, 'n', NULL, &filter.lines, N_(\"n\"),\n@@ -359,6 +359,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\t\tOPTION_CALLBACK, 0, \"points-at\", &filter.points_at, N_(\"object\"),\n \t\t\tN_(\"print only tags of the object\"), 0, parse_opt_object_name\n \t\t},\n+\t\tOPT_STRING(  0 , \"format\", &format, N_(\"format\"), N_(\"format to use for the output\")),\n \t\tOPT_END()\n \t};\n \n@@ -398,8 +399,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\t\tcopts.padding = 2;\n \t\t\trun_column_filter(colopts, &copts);\n \t\t}\n+\t\tif (format && (filter.lines != -1))\n+\t\t\tdie(_(\"--format and -n are incompatible\"));\n \t\tfilter.name_patterns = argv;\n-\t\tret = list_tags(&filter, sorting);\n+\t\tret = list_tags(&filter, sorting, format);\n \t\tif (column_active(colopts))\n \t\t\tstop_column_filter();\n \t\treturn ret;\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 1f066aa..1809011 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1519,4 +1519,20 @@ EOF\"\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--format cannot be used with -n' '\n+\ttest_must_fail git tag -l -n4 --format=\"%(refname)\"\n+'\n+\n+test_expect_success '--format should list tags as per format given' '\n+\tcat >expect <<-\\EOF &&\n+\trefname : refs/tags/foo1.10\n+\trefname : refs/tags/foo1.3\n+\trefname : refs/tags/foo1.6\n+\trefname : refs/tags/foo1.6-rc1\n+\trefname : refs/tags/foo1.6-rc2\n+\tEOF\n+\tgit tag -l --format=\"refname : %(refname)\" \"foo*\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.5.0\n"},{"id":"267691","messageId":"1439130761-15721-3-git-send-email-Karthik.188@gmail.com","threadId":"40040","inReplyTo":"1439129506-9989-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v10 13/13] tag.c: implement '--merged' and '--no-merged' options","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-09T14:32:41Z","receivedAt":"2015-08-09T14:32:41Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"From: Karthik Nayak <karthik.188@gmail.com>\n\nUsing 'ref-filter' APIs implement the '--merged' and '--no-merged'\noptions into 'tag.c'. The '--merged' option lets the user to only\nlist tags merged into the named commit. The '--no-merged' option\nlets the user to only list tags not merged into the named commit.\nIf no object is provided it assumes HEAD as the object.\n\nAdd documentation and tests for the same.\n\nMentored-by: Christian Couder <christian.couder@gmail.com>\nMentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n Documentation/git-tag.txt |  7 ++++++-\n builtin/tag.c             |  6 +++++-\n t/t7004-tag.sh            | 27 +++++++++++++++++++++++++++\n 3 files changed, 38 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 75703c5..c2785d9 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -14,7 +14,7 @@ SYNOPSIS\n 'git tag' -d <tagname>...\n 'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]\n \t[--column[=<options>] | --no-column] [--create-reflog] [--sort=<key>]\n-\t[--format=<format>] [<pattern>...]\n+\t[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]\n 'git tag' -v <tagname>...\n \n DESCRIPTION\n@@ -171,6 +171,11 @@ This option is only applicable when listing tags without annotation lines.\n \t`%0a` to `\\n` (LF).  The fields are same as those in `git\n \tfor-each-ref`.\n \n+--[no-]merged [<commit>]::\n+\tOnly list tags whose tips are reachable, or not reachable\n+\tif '--no-merged' is used, from the specified commit ('HEAD'\n+\tif not specified).\n+\n \n CONFIGURATION\n -------------\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex affb460..22efbba 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 <key-id>] [-f] [-m <msg> | -F <file>] <tagname> [<head>]\"),\n \tN_(\"git tag -d <tagname>...\"),\n \tN_(\"git tag -l [-n[<num>]] [--contains <commit>] [--points-at <object>]\"\n-\t\t\"\\n\\t\\t[<pattern>...]\"),\n+\t\t\"\\n\\t\\t[--[no-]merged [<commit>]] [<pattern>...]\"),\n \tN_(\"git tag -v <tagname>...\"),\n \tNULL\n };\n@@ -353,6 +353,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tOPT_COLUMN(0, \"column\", &colopts, N_(\"show tag list in columns\")),\n \t\tOPT_CONTAINS(&filter.with_commit, N_(\"print only tags that contain the commit\")),\n \t\tOPT_WITH(&filter.with_commit, N_(\"print only tags that contain the commit\")),\n+\t\tOPT_MERGED(&filter, N_(\"print only tags that are merged\")),\n+\t\tOPT_NO_MERGED(&filter, N_(\"print only tags that are not merged\")),\n \t\tOPT_CALLBACK(0 , \"sort\", sorting_tail, N_(\"key\"),\n \t\t\t     N_(\"field name to sort on\"), &parse_opt_ref_sorting),\n \t\t{\n@@ -413,6 +415,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"--contains option is only allowed with -l.\"));\n \tif (filter.points_at.nr)\n \t\tdie(_(\"--points-at option is only allowed with -l.\"));\n+\tif (filter.merge_commit)\n+\t\tdie(_(\"--merged and --no-merged option are only allowed with -l\"));\n \tif (cmdmode == 'd')\n \t\treturn for_each_tag_name(argv, delete_tag);\n \tif (cmdmode == 'v')\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 1809011..5b73539 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1535,4 +1535,31 @@ test_expect_success '--format should list tags as per format given' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'setup --merged test tags' '\n+\tgit tag mergetest-1 HEAD~2 &&\n+\tgit tag mergetest-2 HEAD~1 &&\n+\tgit tag mergetest-3 HEAD\n+'\n+\n+test_expect_success '--merged cannot be used in non-list mode' '\n+\ttest_must_fail git tag --merged=mergetest-2 foo\n+'\n+\n+test_expect_success '--merged shows merged tags' '\n+\tcat >expect <<-\\EOF &&\n+\tmergetest-1\n+\tmergetest-2\n+\tEOF\n+\tgit tag -l --merged=mergetest-2 mergetest-* >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--no-merged show unmerged tags' '\n+\tcat >expect <<-\\EOF &&\n+\tmergetest-3\n+\tEOF\n+\tgit tag -l --no-merged=mergetest-2 mergetest-* >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.5.0\n"},{"id":"267839","messageId":"xmqqlhdhso8i.fsf@gitster.dls.corp.google.com","threadId":"40040","inReplyTo":"1439129506-9989-3-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v10 02/13] ref-filter: print output to strbuf for formatting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-11T17:56:29Z","receivedAt":"2015-08-11T17:56:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> -static void print_value(struct atom_value *v, int quote_style)\n> +static void format_quote_value(struct atom_value *v, int quote_style, struct strbuf *output)\n>  {\n\nHmph...\n\n> -static void emit(const char *cp, const char *ep)\n> +static void append_non_atom(const char *cp, const char *ep, struct strbuf *output)\n\nI was tempted to suggest s/non_atom/literal/, but this would do for now...\n\n> @@ -1262,19 +1257,20 @@ static void emit(const char *cp, const char *ep)\n>  void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)\n>  {\n>  \tconst char *cp, *sp, *ep;\n> +\tstruct strbuf output = STRBUF_INIT;\n>  \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_non_atom(cp, sp, &output);\n>  \t\tget_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);\n> -\t\tprint_value(atomv, quote_style);\n> +\t\tformat_quote_value(atomv, quote_style, &output);\n\nIf the one to add a literal string (with %hex escaping) is called \"append_\",\nthen this should be called append_quoted_atom() or something, no?\n"},{"id":"267840","messageId":"xmqqh9o5so22.fsf@gitster.dls.corp.google.com","threadId":"40040","inReplyTo":"1439129506-9989-3-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v10 02/13] ref-filter: print output to strbuf for formatting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-11T18:00:21Z","receivedAt":"2015-08-11T18:00:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> @@ -1283,9 +1279,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\tformat_quote_value(&resetv, quote_style, &output);\n\nMental note: I _think_ the logic to scan the string and set\nneed_color_reset_at_eol that happens at the beginning can be removed\nonce the code fully utilizes formatting-state information.  A\ncoloring atom would leave a bit in the formatting state to say that\nthe line color has been changed to something other than reset, and\nthen this \"at the end of line\" code can decide if that is the case\nand add a \"reset\" thing here (i.e. the code inside the \"if\n(need_color_reset_at_eol)\" block shown here does not need to change,\nbut the \"if\" condition would).\n"},{"id":"267843","messageId":"xmqqd1ytsnfx.fsf@gitster.dls.corp.google.com","threadId":"40040","inReplyTo":"1439129506-9989-4-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v10 03/13] ref-filter: introduce ref_formatting_state","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-11T18:13:38Z","receivedAt":"2015-08-11T18:13:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n>  \t\tget_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);\n> -\t\tformat_quote_value(atomv, quote_style, &output);\n> +\t\tset_formatting_state(atomv, &state);\n> +\t\tformat_quote_value(atomv, &state);\n> +\t\tperform_state_formatting(&state, &final_buf);\n>  \t}\n>  \tif (*cp) {\n>  \t\tsp = cp + strlen(cp);\n> -\t\tappend_non_atom(cp, sp, &output);\n> +\t\tappend_non_atom(cp, sp, &state);\n> +\t\tperform_state_formatting(&state, &final_buf);\n>  \t}\n\nWith the two helpers being very sketchy at this stage, it is very\nhard to judge if they make sense.  At the conceptual level, I can\nsee that set-formatting-state is to allow an atom to affect the\nstate before the value of the atom is emitted into the buffer.\nI cannot tell what perform-state-formatting is meant to do from\nthese call sites.\n"},{"id":"267845","messageId":"xmqq8u9hsn0j.fsf@gitster.dls.corp.google.com","threadId":"40040","inReplyTo":"1439129506-9989-5-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v10 04/13] utf8: add function to align a string into given strbuf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-11T18:22:52Z","receivedAt":"2015-08-11T18:22:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> +void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned int width,\n> +\t\t       const char *s)\n> +{\n> +\tint display_len = utf8_strnwidth(s, strlen(s), 0);\n> +\tint utf8_compenstation = strlen(s) - display_len;\n\ncompensation, perhaps?  But notice you are running two strlen and\nthen also a call to utf8-strnwidth here already, and then\n\n> +\tif (!strlen(s))\n> +\t\treturn;\n\nyou return here without doing anything.\n\nWorse yet, this logic looks very strange.  If you give it a width of\n8 because you want to align like-item on each record at that column,\na record with 1-display-column item will be shown in 8-display-column\nwith 7 SP padding (either at the beginning, or at the end, or on\nboth ends to center).  If it happens to be an empty string, the entire\n8-display-column disappears.\n\nIs that really what you meant to do?  The logic does not make much\nsense to me, even though the code may implement that logic correctly\nand clearly.\n\n> +\tif (display_len >= width) {\n> +\t\tstrbuf_addstr(buf, s);\n> +\t\treturn;\n> +\t}\n\nMental note: this function values the information more than\npresentation; it refuses to truncate (to lose information) and\ninstead accepts jaggy output.  OK.\n\n> +\tif (position == ALIGN_LEFT)\n> +\t\tstrbuf_addf(buf, \"%-*s\", width + utf8_compenstation, 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_compenstation, s);\n> +\t} else if (position == ALIGN_RIGHT)\n> +\t\tstrbuf_addf(buf, \"%*s\", width + utf8_compenstation, s);\n> +}\n"},{"id":"267848","messageId":"xmqqy4hhr72q.fsf@gitster.dls.corp.google.com","threadId":"40040","inReplyTo":"1439129506-9989-6-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v10 05/13] ref-filter: implement an `align` atom","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-11T18:52:29Z","receivedAt":"2015-08-11T18:52:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n>  struct atom_value{\n\nObviously not a problem with this step, but you need a SP before the\nopen brace.\n\n> @@ -692,6 +704,26 @@ static void populate_value(struct ref_array_item *ref)\n>  \t\t\telse\n>  \t\t\t\tv->s = \" \";\n>  \t\t\tcontinue;\n> +\t\t} else if (skip_prefix(name, \"align:\", &valp)) {\n> +\t\t\tstruct align *align = xmalloc(sizeof(struct align));\n> +\n> +\t\t\tif (skip_prefix(valp, \"left,\", &valp))\n> +\t\t\t\talign->position = ALIGN_LEFT;\n> +\t\t\telse if (skip_prefix(valp, \"right,\", &valp))\n> +\t\t\t\talign->position = ALIGN_RIGHT;\n> +\t\t\telse if (skip_prefix(valp, \"middle,\", &valp))\n> +\t\t\t\talign->position = ALIGN_MIDDLE;\n> +\t\t\telse\n> +\t\t\t\tdie(_(\"improper format entered align:%s\"), valp);\n> +\t\t\tif (strtoul_ui(valp, 10, &align->width))\n> +\t\t\t\tdie(_(\"positive width expected align:%s\"), valp);\n\nMinor nits on the design.  %(align:<width>[,<position>]) would let\nus write %(align:16)...%(end) and use the \"default position\", which\nmay be beneficial if one kind of alignment is prevalent (I guess all\nthe internal users left-align?)  %(align:<position>,<width>) forces\nusers to spell both out all the time.\n\n> @@ -1198,7 +1230,9 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)\n>  \n>  struct ref_formatting_state {\n>  \tstruct strbuf output;\n> +\tstruct align *align;\n>  \tint quote_style;\n> +\tunsigned int end : 1;\n>  };\n\nMental note: it is not clear why you need 'end' field in the state.\nPerhaps it is an indication that the division of labor is poorly\ndesigned between the helper that updates the formatting state and\nthe other helper that reflects the formatting state to the final\nstring.\n\n> @@ -1262,12 +1296,31 @@ static void append_non_atom(const char *cp, const char *ep, struct ref_formattin\n>  \n>  static void set_formatting_state(struct atom_value *atomv, struct ref_formatting_state *state)\n>  {\n> -\t/* Based on the atomv values, the formatting state is set */\n> +\tif (atomv->align) {\n> +\t\tstate->align = atomv->align;\n> +\t\tatomv->align = NULL;\n> +\t}\n> +\tif (atomv->end)\n> +\t\tstate->end = 1;\n> +}\n> +\n> +static int align_ref_strbuf(struct ref_formatting_state *state, struct strbuf *final)\n> +{\n> +\tif (state->align && state->end) {\n\n... and I think that is what I see.  If this function knows that we\nare processing %(end), i.e. perform-state-formatting is called for\neach atom and receives atomv, there wouldn't have to be a code like\nthis.\n\n> +\t\tstruct align *align = state->align;\n> +\t\tstrbuf_utf8_align(final, align->position, align->width, state->output.buf);\n> +\t\tstrbuf_reset(&state->output);\n> +\t\tstate->align = NULL;\n> +\t\treturn 1;\n> +\t} else if (state->align)\n> +\t\treturn 1;\n> +\treturn 0;\n>  }\n>  \n>  static void perform_state_formatting(struct ref_formatting_state *state, struct strbuf *final)\n>  {\n> -\t/* More formatting options to be eventually added */\n> +\tif (align_ref_strbuf(state, final))\n> +\t\treturn;\n\nAt the design level, I have a strong suspicion that it is a wrong\nway to go.  It piles more \"if (this state bit was left by the\nprevious atom) then do this\" on this function and will make an\nunmanageable mess.\n\nYou have a dictionary of all possible atoms somewhere.  Why not hook\na pointer to the \"handler\" function (or two) to each element in it,\ninstead of duplicating \"this one is special\" information down to\nindividual atom instantiations (i.e. atomv) as atomv.modifier_atom\nbit, an dstructure the caller more like this?\n\n\tget_ref_atom_value(info, parse_ref_filter_atom, &atomv);\n        if (atomv->pre_handler)\n        \tatomv->pre_handler(atomv, &state);\n\tformat_quote_value(atomv, &state);\n        if (atomv->post_handler)\n        \tatomv->post_handler(atomv, &state);\n\nActually, each atom could just have a single handler; an atom like\n%(refname:short) whose sole effect is to append atomv->s to the\nstate buffer can point a function to do so in its handler.\n\nOn the other hand, align atom's handler would push a new state on\nthe stack, marking that it is the one to handle diverted output.\n\n\talign_atom_handler(atomv, state)\n        {\n        \tstruct format_state *new = push_new_state(state);\n\t\tstrbuf_init(&new->output);\n                new->atend = align_handler;\n                new->return_to = atomv; /* or whatever that holds width,pos */\n\t}\n\nThen end atom's handler would pop the state from the stack, and the\nprocessing to be done\n\n\tend_atom_handler(atomv, state)\n\t{\n                state->atend(state);\n                pop_state(state);\n\t}                \n\nand the called align_handler would be something like:\n\n\talign_handler(state)\n\t{\n                struct strbuf aligned = STRBUF_INIT;\n\t\tstruct format_state *return_to = state->prev;\n                struct atom_value *atomv = state->return_to;\n\n                strbuf_utf8_align(&aligned,\n                        atomv->align.pos, atomv->align.width,\n                        state->output.buf);\n\t\tstrbuf_addbuf(&return_to->output, &aligned);\n                strbuf_release(&aligned);\n\t}\n\nWith an arrangement like that, the body of the loop in\nshow_ref_array_item() could be as simple and regular as:\n\n\tget_ref_atom_value(info, parse_ref_filter_atom, &atomv);\n       \tatomv->handler(atomv, &state);\n\nwithout any new \"ah, this %(end) is special so we need a new\nmechanism to pass information between set_formatting_state and\nperform_formatting\" logic introduced every time you add new things.\n"},{"id":"267901","messageId":"CAOLa=ZSo9wXj_2wPCfZ=kEYhnFHzqT2AUFKvoDoKh_BD9v+1Hw@mail.gmail.com","threadId":"40040","inReplyTo":"xmqqlhdhso8i.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v10 02/13] ref-filter: print output to strbuf for formatting","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-12T13:22:15Z","receivedAt":"2015-08-12T13:22:15Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Aug 11, 2015 at 11:26 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Karthik Nayak <karthik.188@gmail.com> writes:\n>\n>> -static void print_value(struct atom_value *v, int quote_style)\n>> +static void format_quote_value(struct atom_value *v, int quote_style, struct strbuf *output)\n>>  {\n>\n> Hmph...\n>\n>> -static void emit(const char *cp, const char *ep)\n>> +static void append_non_atom(const char *cp, const char *ep, struct strbuf *output)\n>\n> I was tempted to suggest s/non_atom/literal/, but this would do for now...\n>\n\nSince I'll be doing a re-roll I could do that.\n\n>> @@ -1262,19 +1257,20 @@ 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>>       const char *cp, *sp, *ep;\n>> +     struct strbuf output = STRBUF_INIT;\n>>\n>>       for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {\n>>               struct atom_value *atomv;\n>>\n>>               ep = strchr(sp, ')');\n>>               if (cp < sp)\n>> -                     emit(cp, sp);\n>> +                     append_non_atom(cp, sp, &output);\n>>               get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);\n>> -             print_value(atomv, quote_style);\n>> +             format_quote_value(atomv, quote_style, &output);\n>\n> If the one to add a literal string (with %hex escaping) is called \"append_\",\n> then this should be called append_quoted_atom() or something, no?\n\nAlthough it does append like \"append_non_atom\" this is more of formatting based\non the quote given hence the name.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267902","messageId":"CAOLa=ZT_2pZhfmumJM=twPcPryQfbDvGCRFxGVDQo8YZqwRVcw@mail.gmail.com","threadId":"40040","inReplyTo":"xmqqh9o5so22.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v10 02/13] ref-filter: print output to strbuf for formatting","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-12T13:22:57Z","receivedAt":"2015-08-12T13:22:57Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Aug 11, 2015 at 11:30 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Karthik Nayak <karthik.188@gmail.com> writes:\n>\n>> @@ -1283,9 +1279,11 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n>>               if (color_parse(\"reset\", color) < 0)\n>>                       die(\"BUG: couldn't parse 'reset' as a color\");\n>>               resetv.s = color;\n>> -             print_value(&resetv, quote_style);\n>> +             format_quote_value(&resetv, quote_style, &output);\n>\n> Mental note: I _think_ the logic to scan the string and set\n> need_color_reset_at_eol that happens at the beginning can be removed\n> once the code fully utilizes formatting-state information.  A\n> coloring atom would leave a bit in the formatting state to say that\n> the line color has been changed to something other than reset, and\n> then this \"at the end of line\" code can decide if that is the case\n> and add a \"reset\" thing here (i.e. the code inside the \"if\n> (need_color_reset_at_eol)\" block shown here does not need to change,\n> but the \"if\" condition would).\n>\n>\n\nThis could be done, when we implement color as a state :) Thanks for the note\n\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267903","messageId":"CAOLa=ZTKvNq0meFfHA8CjWsXOznmW4QUudQnizypzThhBQvFZg@mail.gmail.com","threadId":"40040","inReplyTo":"xmqqd1ytsnfx.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v10 03/13] ref-filter: introduce ref_formatting_state","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-12T13:26:40Z","receivedAt":"2015-08-12T13:26:40Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Aug 11, 2015 at 11:43 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Karthik Nayak <karthik.188@gmail.com> writes:\n>\n>>               get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);\n>> -             format_quote_value(atomv, quote_style, &output);\n>> +             set_formatting_state(atomv, &state);\n>> +             format_quote_value(atomv, &state);\n>> +             perform_state_formatting(&state, &final_buf);\n>>       }\n>>       if (*cp) {\n>>               sp = cp + strlen(cp);\n>> -             append_non_atom(cp, sp, &output);\n>> +             append_non_atom(cp, sp, &state);\n>> +             perform_state_formatting(&state, &final_buf);\n>>       }\n>\n> With the two helpers being very sketchy at this stage, it is very\n> hard to judge if they make sense.  At the conceptual level, I can\n> see that set-formatting-state is to allow an atom to affect the\n> state before the value of the atom is emitted into the buffer.\n> I cannot tell what perform-state-formatting is meant to do from\n> these call sites.\n\n\nTrue, set formatting state is to ensure that the state is manipulated for\na given atom.\n\nperform_state_formatting() is meant to act on the state set by the atom,\nIt performs formatting based on the state values, here is just copies the\nstrbuf set within the state to the final_buf.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267905","messageId":"CAOLa=ZRL-WV=9Q+_fo7wcbqT7jZVA02oYo8SfDcLrev=qsUvLg@mail.gmail.com","threadId":"40040","inReplyTo":"xmqq8u9hsn0j.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v10 04/13] utf8: add function to align a string into given strbuf","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-12T13:41:05Z","receivedAt":"2015-08-12T13:41:05Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Aug 11, 2015 at 11:52 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Karthik Nayak <karthik.188@gmail.com> writes:\n>\n>> +void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned int width,\n>> +                    const char *s)\n>> +{\n>> +     int display_len = utf8_strnwidth(s, strlen(s), 0);\n>> +     int utf8_compenstation = strlen(s) - display_len;\n>\n> compensation, perhaps?  But notice you are running two strlen and\n> then also a call to utf8-strnwidth here already, and then\n>\n\ncompensation it is.\nprobably have a single strlen() call and set the value to another variable.\n\n>> +     if (!strlen(s))\n>> +             return;\n>\n> you return here without doing anything.\n>\n> Worse yet, this logic looks very strange.  If you give it a width of\n> 8 because you want to align like-item on each record at that column,\n> a record with 1-display-column item will be shown in 8-display-column\n> with 7 SP padding (either at the beginning, or at the end, or on\n> both ends to center).  If it happens to be an empty string, the entire\n> 8-display-column disappears.\n>\n> Is that really what you meant to do?  The logic does not make much\n> sense to me, even though the code may implement that logic correctly\n> and clearly.\n>\n\nNot sure what you meant.\nBut this does not act on empty strings and that was intentional. It\ndoes not make\nsense to have an alignment on an empty string, where the caller could themselves\njust do a `strbuf_addchars(&buf, \" \", width)`.\n\n>> +     if (display_len >= width) {\n>> +             strbuf_addstr(buf, s);\n>> +             return;\n>> +     }\n>\n> Mental note: this function values the information more than\n> presentation; it refuses to truncate (to lose information) and\n> instead accepts jaggy output.  OK.\n>\n\nWith regards to its usage in ref-filter, this is needed, don't you think?\n\n>> +     if (position == ALIGN_LEFT)\n>> +             strbuf_addf(buf, \"%-*s\", width + utf8_compenstation, s);\n>> +     else if (position == ALIGN_MIDDLE) {\n>> +             int left = (width - display_len)/2;\n>> +             strbuf_addf(buf, \"%*s%-*s\", left, \"\", width - left + utf8_compenstation, s);\n>> +     } else if (position == ALIGN_RIGHT)\n>> +             strbuf_addf(buf, \"%*s\", width + utf8_compenstation, s);\n>> +}\n\nThanks!\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267915","messageId":"CAOLa=ZTnS0kL=CK8Lx-brO0tryB5YzOFUpapsM4LMW-m2Yf9pw@mail.gmail.com","threadId":"40040","inReplyTo":"xmqqy4hhr72q.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v10 05/13] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-12T16:24:29Z","receivedAt":"2015-08-12T16:24:29Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Aug 12, 2015 at 12:22 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Karthik Nayak <karthik.188@gmail.com> writes:\n>\n>>  struct atom_value{\n>\n> Obviously not a problem with this step, but you need a SP before the\n> open brace.\n>\n\nWill add.\n\n>> @@ -692,6 +704,26 @@ static void populate_value(struct ref_array_item *ref)\n>>                       else\n>>                               v->s = \" \";\n>>                       continue;\n>> +             } else if (skip_prefix(name, \"align:\", &valp)) {\n>> +                     struct align *align = xmalloc(sizeof(struct align));\n>> +\n>> +                     if (skip_prefix(valp, \"left,\", &valp))\n>> +                             align->position = ALIGN_LEFT;\n>> +                     else if (skip_prefix(valp, \"right,\", &valp))\n>> +                             align->position = ALIGN_RIGHT;\n>> +                     else if (skip_prefix(valp, \"middle,\", &valp))\n>> +                             align->position = ALIGN_MIDDLE;\n>> +                     else\n>> +                             die(_(\"improper format entered align:%s\"), valp);\n>> +                     if (strtoul_ui(valp, 10, &align->width))\n>> +                             die(_(\"positive width expected align:%s\"), valp);\n>\n> Minor nits on the design.  %(align:<width>[,<position>]) would let\n> us write %(align:16)...%(end) and use the \"default position\", which\n> may be beneficial if one kind of alignment is prevalent (I guess all\n> the internal users left-align?)  %(align:<position>,<width>) forces\n> users to spell both out all the time.\n>\n\nIsn't that better? I mean It sets a format which the others eventually\ncan follow\n%(atom:suboption,value).\nFor example: %(objectname:abbrev,size)\nChanging this would cause inconsistency according to me.\n\n>> @@ -1198,7 +1230,9 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)\n>>\n>>  struct ref_formatting_state {\n>>       struct strbuf output;\n>> +     struct align *align;\n>>       int quote_style;\n>> +     unsigned int end : 1;\n>>  };\n>\n> Mental note: it is not clear why you need 'end' field in the state.\n> Perhaps it is an indication that the division of labor is poorly\n> designed between the helper that updates the formatting state and\n> the other helper that reflects the formatting state to the final\n> string.\n>\n\nThis goes with what you've said below!\n\n>> @@ -1262,12 +1296,31 @@ static void append_non_atom(const char *cp, const char *ep, struct ref_formattin\n>>\n>>  static void set_formatting_state(struct atom_value *atomv, struct ref_formatting_state *state)\n>>  {\n>> -     /* Based on the atomv values, the formatting state is set */\n>> +     if (atomv->align) {\n>> +             state->align = atomv->align;\n>> +             atomv->align = NULL;\n>> +     }\n>> +     if (atomv->end)\n>> +             state->end = 1;\n>> +}\n>> +\n>> +static int align_ref_strbuf(struct ref_formatting_state *state, struct strbuf *final)\n>> +{\n>> +     if (state->align && state->end) {\n>\n> ... and I think that is what I see.  If this function knows that we\n> are processing %(end), i.e. perform-state-formatting is called for\n> each atom and receives atomv, there wouldn't have to be a code like\n> this.\n>\n\nAgreed, your suggestion below eradicates this\n\n>> +             struct align *align = state->align;\n>> +             strbuf_utf8_align(final, align->position, align->width, state->output.buf);\n>> +             strbuf_reset(&state->output);\n>> +             state->align = NULL;\n>> +             return 1;\n>> +     } else if (state->align)\n>> +             return 1;\n>> +     return 0;\n>>  }\n>>\n>>  static void perform_state_formatting(struct ref_formatting_state *state, struct strbuf *final)\n>>  {\n>> -     /* More formatting options to be eventually added */\n>> +     if (align_ref_strbuf(state, final))\n>> +             return;\n>\n> At the design level, I have a strong suspicion that it is a wrong\n> way to go.  It piles more \"if (this state bit was left by the\n> previous atom) then do this\" on this function and will make an\n> unmanageable mess.\n\nHmm, yeah makes sense.\n\n>\n> You have a dictionary of all possible atoms somewhere.  Why not hook\n> a pointer to the \"handler\" function (or two) to each element in it,\n> instead of duplicating \"this one is special\" information down to\n> individual atom instantiations (i.e. atomv) as atomv.modifier_atom\n> bit, an dstructure the caller more like this?\n>\n>         get_ref_atom_value(info, parse_ref_filter_atom, &atomv);\n>         if (atomv->pre_handler)\n>                 atomv->pre_handler(atomv, &state);\n>         format_quote_value(atomv, &state);\n>         if (atomv->post_handler)\n>                 atomv->post_handler(atomv, &state);\n>\n> Actually, each atom could just have a single handler; an atom like\n> %(refname:short) whose sole effect is to append atomv->s to the\n> state buffer can point a function to do so in its handler.\n>\n> On the other hand, align atom's handler would push a new state on\n> the stack, marking that it is the one to handle diverted output.\n>\n>         align_atom_handler(atomv, state)\n>         {\n>                 struct format_state *new = push_new_state(state);\n>                 strbuf_init(&new->output);\n>                 new->atend = align_handler;\n>                 new->return_to = atomv; /* or whatever that holds width,pos */\n>         }\n>\n> Then end atom's handler would pop the state from the stack, and the\n> processing to be done\n>\n>         end_atom_handler(atomv, state)\n>         {\n>                 state->atend(state);\n>                 pop_state(state);\n>         }\n>\n> and the called align_handler would be something like:\n>\n>         align_handler(state)\n>         {\n>                 struct strbuf aligned = STRBUF_INIT;\n>                 struct format_state *return_to = state->prev;\n>                 struct atom_value *atomv = state->return_to;\n>\n>                 strbuf_utf8_align(&aligned,\n>                         atomv->align.pos, atomv->align.width,\n>                         state->output.buf);\n>                 strbuf_addbuf(&return_to->output, &aligned);\n>                 strbuf_release(&aligned);\n>         }\n>\n> With an arrangement like that, the body of the loop in\n> show_ref_array_item() could be as simple and regular as:\n>\n>         get_ref_atom_value(info, parse_ref_filter_atom, &atomv);\n>         atomv->handler(atomv, &state);\n>\n> without any new \"ah, this %(end) is special so we need a new\n> mechanism to pass information between set_formatting_state and\n> perform_formatting\" logic introduced every time you add new things.\n>\n\nThis I agree is a way better scheme, This could also eventually help cleanup\npopulate_value() which is a huge function at the moment.\n\nI like your suggestion, I'll work on this, Thanks a lot :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267917","messageId":"xmqq7fp0mpwz.fsf@gitster.dls.corp.google.com","threadId":"40040","inReplyTo":"CAOLa=ZSo9wXj_2wPCfZ=kEYhnFHzqT2AUFKvoDoKh_BD9v+1Hw@mail.gmail.com","subject":"Re: [PATCH v10 02/13] ref-filter: print output to strbuf for formatting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-12T16:29:00Z","receivedAt":"2015-08-12T16:29:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n>>> +             format_quote_value(atomv, quote_style, &output);\n>>\n>> If the one to add a literal string (with %hex escaping) is called \"append_\",\n>> then this should be called append_quoted_atom() or something, no?\n>\n> Although it does append like \"append_non_atom\" this is more of formatting based\n> on the quote given hence the name.\n\nAppending formatted values is still appending, no?\n"},{"id":"267918","messageId":"xmqq37zompd7.fsf@gitster.dls.corp.google.com","threadId":"40040","inReplyTo":"CAOLa=ZRL-WV=9Q+_fo7wcbqT7jZVA02oYo8SfDcLrev=qsUvLg@mail.gmail.com","subject":"Re: [PATCH v10 04/13] utf8: add function to align a string into given strbuf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-12T16:40:52Z","receivedAt":"2015-08-12T16:40:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> On Tue, Aug 11, 2015 at 11:52 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Karthik Nayak <karthik.188@gmail.com> writes:\n>>\n>>> +void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned int width,\n>>> +                    const char *s)\n>>> +{\n>>> +     int display_len = utf8_strnwidth(s, strlen(s), 0);\n>>> +     int utf8_compenstation = strlen(s) - display_len;\n>>\n>> compensation, perhaps?  But notice you are running two strlen and\n>> then also a call to utf8-strnwidth here already, and then\n>>\n>\n> compensation it is.\n> probably have a single strlen() call and set the value to another variable.\n\nThat is not what I meant.  If you want to keep the early return of\n\"doing nothing for an empty string\" (which you should *NOT*, by the\nway), declare these variables upfront, do the early return and then\ncall utf8_strnwidth() to compute the value you are going to use,\nonly when you know you are going to use them.  That is what I meant.\n\n>>> +     if (!strlen(s))\n>>> +             return;\n>>\n>> you return here without doing anything.\n>>\n>> Worse yet, this logic looks very strange.  If you give it a width of\n>> 8 because you want to align like-item on each record at that column,\n>> a record with 1-display-column item will be shown in 8-display-column\n>> with 7 SP padding (either at the beginning, or at the end, or on\n>> both ends to center).  If it happens to be an empty string, the entire\n>> 8-display-column disappears.\n>>\n>> Is that really what you meant to do?  The logic does not make much\n>> sense to me, even though the code may implement that logic correctly\n>> and clearly.\n>\n> Not sure what you meant.\n> But this does not act on empty strings and that was intentional.\n\nIf it is truly intentional, then the design is wrong.  You are\nwriting a public function that can be called by other people.\n\nWhich one makes more sense?  Remember that you are going to have\nmany callers over time in the course of project in the future.\n\n (A) The caller is forced to always check the string that he is\n     merely passing on, i.e.\n\n\tstruct strbuf buf = STRBUF_INIT;\n        struct strbuf out = STRBUF_INIT;\n\n\tfill_some_placeholder(&buf, fmt, args);\n        if (buf.len)\n        \tstrbuf_utf8_align(&out, ALIGN_LEFT, 8, buf.buf);\n        else\n        \tstrbuf_addchars(&out, ' ', 8);\n\n     only because the callee strbuf_utf8_align() refuses to do the\n     trivial work.\n\n (B) The caller does not have to care, i.e.\n\n\tstruct strbuf buf = STRBUF_INIT;\n        struct strbuf out = STRBUF_INIT;\n\n\tfill_some_placeholder(&buf, fmt, args);\n       \tstrbuf_utf8_align(&out, ALIGN_LEFT, 8, buf.buf);\n\n> It\n> does not make\n> sense to have an alignment on an empty string, where the caller could themselves\n> just do a `strbuf_addchars(&buf, \" \", width)`.\n\nIt simply does not make sense to force the caller to even care.\nWhat they want is a series of lines, whose columns are aligned.\n\n>>> +     if (display_len >= width) {\n>>> +             strbuf_addstr(buf, s);\n>>> +             return;\n>>> +     }\n>>\n>> Mental note: this function values the information more than\n>> presentation; it refuses to truncate (to lose information) and\n>> instead accepts jaggy output.  OK.\n>\n> With regards to its usage in ref-filter, this is needed, don't you think?\n\nThat is exactly why I said \"OK\".\n\nI was primarily trying to lead other reviewers by example,\ndemonstrating that a review will become more credible if it shows\nthat the reviewer actually read the patch by explaining the thinking\nbehind what the code does in his own words.  I see too many \"FWIW,\nreviewed by me\" without indicating if it is \"from just a cursory\nread it looks nice\" or \"I fully read and understood it and I agree\nwith the design choices it makes\", which does not help me very much\nwhen queuing a patch.\n"},{"id":"267920","messageId":"CAOLa=ZSCnTz-ZuKebSxN_UO-rGb5drow6p0AezgWNPD4fL_Acw@mail.gmail.com","threadId":"40040","inReplyTo":"xmqq7fp0mpwz.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v10 02/13] ref-filter: print output to strbuf for formatting","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-12T16:56:59Z","receivedAt":"2015-08-12T16:56:59Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Aug 12, 2015 at 9:59 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Karthik Nayak <karthik.188@gmail.com> writes:\n>\n>>>> +             format_quote_value(atomv, quote_style, &output);\n>>>\n>>> If the one to add a literal string (with %hex escaping) is called \"append_\",\n>>> then this should be called append_quoted_atom() or something, no?\n>>\n>> Although it does append like \"append_non_atom\" this is more of formatting based\n>> on the quote given hence the name.\n>\n> Appending formatted values is still appending, no?\n\nOf course :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267921","messageId":"CAOLa=ZSKMfDS=mby1OyNLziF7bX_3kGAhhLTRM6DUL-zn-21eQ@mail.gmail.com","threadId":"40040","inReplyTo":"xmqq37zompd7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v10 04/13] utf8: add function to align a string into given strbuf","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-12T17:10:23Z","receivedAt":"2015-08-12T17:10:23Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Aug 12, 2015 at 10:10 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Karthik Nayak <karthik.188@gmail.com> writes:\n>\n>> On Tue, Aug 11, 2015 at 11:52 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>> Karthik Nayak <karthik.188@gmail.com> writes:\n>>>\n>>>> +void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned int width,\n>>>> +                    const char *s)\n>>>> +{\n>>>> +     int display_len = utf8_strnwidth(s, strlen(s), 0);\n>>>> +     int utf8_compenstation = strlen(s) - display_len;\n>>>\n>>> compensation, perhaps?  But notice you are running two strlen and\n>>> then also a call to utf8-strnwidth here already, and then\n>>>\n>>\n>> compensation it is.\n>> probably have a single strlen() call and set the value to another variable.\n>\n> That is not what I meant.  If you want to keep the early return of\n> \"doing nothing for an empty string\" (which you should *NOT*, by the\n> way), declare these variables upfront, do the early return and then\n> call utf8_strnwidth() to compute the value you are going to use,\n> only when you know you are going to use them.  That is what I meant.\n>\n\nOh okay, after reading your mail now that makes sense.\n\n>>>> +     if (!strlen(s))\n>>>> +             return;\n>>>\n>>> you return here without doing anything.\n>>>\n>>> Worse yet, this logic looks very strange.  If you give it a width of\n>>> 8 because you want to align like-item on each record at that column,\n>>> a record with 1-display-column item will be shown in 8-display-column\n>>> with 7 SP padding (either at the beginning, or at the end, or on\n>>> both ends to center).  If it happens to be an empty string, the entire\n>>> 8-display-column disappears.\n>>>\n>>> Is that really what you meant to do?  The logic does not make much\n>>> sense to me, even though the code may implement that logic correctly\n>>> and clearly.\n>>\n>> Not sure what you meant.\n>> But this does not act on empty strings and that was intentional.\n>\n> If it is truly intentional, then the design is wrong.  You are\n> writing a public function that can be called by other people.\n>\n> Which one makes more sense?  Remember that you are going to have\n> many callers over time in the course of project in the future.\n>\n>  (A) The caller is forced to always check the string that he is\n>      merely passing on, i.e.\n>\n>         struct strbuf buf = STRBUF_INIT;\n>         struct strbuf out = STRBUF_INIT;\n>\n>         fill_some_placeholder(&buf, fmt, args);\n>         if (buf.len)\n>                 strbuf_utf8_align(&out, ALIGN_LEFT, 8, buf.buf);\n>         else\n>                 strbuf_addchars(&out, ' ', 8);\n>\n>      only because the callee strbuf_utf8_align() refuses to do the\n>      trivial work.\n>\n>  (B) The caller does not have to care, i.e.\n>\n>         struct strbuf buf = STRBUF_INIT;\n>         struct strbuf out = STRBUF_INIT;\n>\n>         fill_some_placeholder(&buf, fmt, args);\n>         strbuf_utf8_align(&out, ALIGN_LEFT, 8, buf.buf);\n>\n\nYea, good point here, thanks for putting it out :)\n\n>> It\n>> does not make\n>> sense to have an alignment on an empty string, where the caller could themselves\n>> just do a `strbuf_addchars(&buf, \" \", width)`.\n>\n> It simply does not make sense to force the caller to even care.\n> What they want is a series of lines, whose columns are aligned.\n>\n\nMy ignorance, sorry!\n\n>>>> +     if (display_len >= width) {\n>>>> +             strbuf_addstr(buf, s);\n>>>> +             return;\n>>>> +     }\n>>>\n>>> Mental note: this function values the information more than\n>>> presentation; it refuses to truncate (to lose information) and\n>>> instead accepts jaggy output.  OK.\n>>\n>> With regards to its usage in ref-filter, this is needed, don't you think?\n>\n> That is exactly why I said \"OK\".\n>\n> I was primarily trying to lead other reviewers by example,\n> demonstrating that a review will become more credible if it shows\n> that the reviewer actually read the patch by explaining the thinking\n> behind what the code does in his own words.  I see too many \"FWIW,\n> reviewed by me\" without indicating if it is \"from just a cursory\n> read it looks nice\" or \"I fully read and understood it and I agree\n> with the design choices it makes\", which does not help me very much\n> when queuing a patch.\n>\n>\n\nAh! okay! was just wondering if you were looking for an explanation :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267923","messageId":"xmqqpp2sl9au.fsf@gitster.dls.corp.google.com","threadId":"40040","inReplyTo":"CAOLa=ZTnS0kL=CK8Lx-brO0tryB5YzOFUpapsM4LMW-m2Yf9pw@mail.gmail.com","subject":"Re: [PATCH v10 05/13] ref-filter: implement an `align` atom","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-12T17:13:13Z","receivedAt":"2015-08-12T17:13:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> On Wed, Aug 12, 2015 at 12:22 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n>> Minor nits on the design.  %(align:<width>[,<position>]) would let\n>> us write %(align:16)...%(end) and use the \"default position\", which\n>> may be beneficial if one kind of alignment is prevalent (I guess all\n>> the internal users left-align?)  %(align:<position>,<width>) forces\n>> users to spell both out all the time.\n>>\n>\n> Isn't that better? I mean It sets a format which the others eventually\n> can follow\n> %(atom:suboption,value).\n> For example: %(objectname:abbrev,size)\n\nNo, I do not think it is better.  First of all, the similarity you\nare perceiving does not exist.  For 'objectname:abbrev', the 'size'\nis a property of the 'abbrev'.  For 'align:left', the 'width' is not\na property of the position.  It is a property given to 'align'\n(i.e. you have this many display columns to work with).\n\nAlso, if there are ways other than 'abbrev' that '8' could affect\nthe way how %(objectname) is modified, then it should be spelled as\n%(objectname:abbrev=8).  To specify two modification magics, each of\nwhich takes a number, the user would say e.g.\n\n    %(objectname:abbrev=8,magic=4)\n\nThe syntax %(objectname:abbrev,size) would not allow you to extend\nit nicely---you would end up with %(objectname:abbrev,8,magic,4) or\nsomething silly like that.\n\nSeeing your %(objectname:abbrev,size), I'd imagine that you are\nassuming that you will never allow any magic other than 'abbrev'\nthat takes a number to %(objectname).  And under that assumption\n%(objectname:8) would be a short-hand for %(objectname:8,abbrev).\n\nAnd that would be following %(align:8).  Both 'left' (implied\ndefault) and '8' are instructing 'align' what to do.\n"},{"id":"267944","messageId":"CAOLa=ZQbjVnJCxVu5SgKDUEVnidY=zw9nVYqG_z3nWhK8QnLTA@mail.gmail.com","threadId":"40040","inReplyTo":"xmqqpp2sl9au.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v10 05/13] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-12T20:07:17Z","receivedAt":"2015-08-12T20:07:17Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Aug 12, 2015 at 10:43 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Karthik Nayak <karthik.188@gmail.com> writes:\n>\n>> On Wed, Aug 12, 2015 at 12:22 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>>> Minor nits on the design.  %(align:<width>[,<position>]) would let\n>>> us write %(align:16)...%(end) and use the \"default position\", which\n>>> may be beneficial if one kind of alignment is prevalent (I guess all\n>>> the internal users left-align?)  %(align:<position>,<width>) forces\n>>> users to spell both out all the time.\n>>>\n>>\n>> Isn't that better? I mean It sets a format which the others eventually\n>> can follow\n>> %(atom:suboption,value).\n>> For example: %(objectname:abbrev,size)\n>\n> No, I do not think it is better.  First of all, the similarity you\n> are perceiving does not exist.  For 'objectname:abbrev', the 'size'\n> is a property of the 'abbrev'.  For 'align:left', the 'width' is not\n> a property of the position.  It is a property given to 'align'\n> (i.e. you have this many display columns to work with).\n>\n> Also, if there are ways other than 'abbrev' that '8' could affect\n> the way how %(objectname) is modified, then it should be spelled as\n> %(objectname:abbrev=8).  To specify two modification magics, each of\n> which takes a number, the user would say e.g.\n>\n>     %(objectname:abbrev=8,magic=4)\n>\n> The syntax %(objectname:abbrev,size) would not allow you to extend\n> it nicely---you would end up with %(objectname:abbrev,8,magic,4) or\n> something silly like that.\n>\n> Seeing your %(objectname:abbrev,size), I'd imagine that you are\n> assuming that you will never allow any magic other than 'abbrev'\n> that takes a number to %(objectname).  And under that assumption\n> %(objectname:8) would be a short-hand for %(objectname:8,abbrev).\n>\n\nI never thought about this, thanks for explaining.\n\n> And that would be following %(align:8).  Both 'left' (implied\n> default) and '8' are instructing 'align' what to do.\n>\n\nWill follow this. :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267945","messageId":"xmqqr3n8jlvv.fsf@gitster.dls.corp.google.com","threadId":"40040","inReplyTo":"CAOLa=ZQbjVnJCxVu5SgKDUEVnidY=zw9nVYqG_z3nWhK8QnLTA@mail.gmail.com","subject":"Re: [PATCH v10 05/13] ref-filter: implement an `align` atom","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-12T20:24:20Z","receivedAt":"2015-08-12T20:24:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> On Wed, Aug 12, 2015 at 10:43 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> ...\n>> %(objectname:abbrev=8).  To specify two modification magics, each of\n>> which takes a number, the user would say e.g.\n>>\n>>     %(objectname:abbrev=8,magic=4)\n>> ...\n>> And that would be following %(align:8).  Both 'left' (implied\n>> default) and '8' are instructing 'align' what to do.\n>\n> Will follow this. :)\n\nI think the most generic way to think about this is to consider that\nthe most fully spelled form of align would be this:\n\n\t%(align:width=12,position=left)\n\nAnd another rule you would have is that the user is allowed to omit\n\"<attr>=\" when it is obvious from its value.  For \"align\", 'left'\ncan only possibly be the value for 'position' and similarly '12' for\n'width'.  That is why the \"objectname\" example says \"abbrev=8\", and\nnot \"abbrev,8\", because from the value of \"8\" without the attribute\nname, you cannot tell if the user meant abbrev=8 or magic=8.\n\nThat '\"<attr>=\" can be omitted' rule makes both of these valid\nconstructs:\n\n\t%(align:12,left) %(align:left,12)\n\nMoreover, if you make \"left aligned\" the default behaviour when\nposition is not specified, you can make %(align:12) the shortest way\nto spell the same thing.  Note that I said \"if you make\"; I do not\noffhand know if all the internal callers of this mechanism your\nupdates to \"branch -l\" and \"tag -l\" would want left aligned output\n(if so, that is one argument to favor making left-aligned the\nimplicit default; if not, it may be better to require the position\nalways specified).\n"},{"id":"268005","messageId":"CAPig+cRhgfOozy42UiDs24JFQTw5k8CYS0c+3DTOFB+XkPrLRQ@mail.gmail.com","threadId":"40040","inReplyTo":"1439129506-9989-6-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v10 05/13] ref-filter: implement an `align` atom","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-13T18:26:31Z","receivedAt":"2015-08-13T18:26:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Aug 9, 2015 at 10:11 AM, Karthik Nayak <karthik.188@gmail.com> wrote:\n> Implement an `align` atom which left-, middle-, or right-aligns the\n> content between %(align:..) and %(end).\n>\n> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n> ---\n> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\n> index e49d578..d949812 100644\n> --- a/Documentation/git-for-each-ref.txt\n> +++ b/Documentation/git-for-each-ref.txt\n> @@ -127,6 +127,15 @@ color::\n>         Change output color.  Followed by `:<colorname>`, where names\n>         are described in `color.branch.*`.\n>\n> +align::\n> +       Implement an `align` atom which left-, middle-, or\n> +       right-aligns the content between %(align:..)  and\n\nThis documentation seems to be copied a bit too literally from the\ncommit message. Saying \"Implement an `align` atom\" makes sense for the\ncommit message, but not for the documentation of the 'align' atom.\nInstead,\n\n    Left-, middle-, or right-align the content between\n    %(align:...) and %(end).\n\nwould sound better.\n\n> +       %(end). Followed by `:<position>,<width>`, where the\n> +       `<position>` is either left, right or middle and `<width>` is\n> +       the total length of the content with alignment. If the\n> +       contents length is more than the width then no alignment is\n> +       performed. Currently nested alignment is not supported.\n"},{"id":"268021","messageId":"CAPig+cS+rS=xKX-LrJSTqFgaopuLzbh3hqURFWXfzMXH=XUk2w@mail.gmail.com","threadId":"40040","inReplyTo":"1439129506-9989-5-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v10 04/13] utf8: add function to align a string into given strbuf","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-13T19:08:54Z","receivedAt":"2015-08-13T19:08:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Aug 9, 2015 at 10:11 AM, Karthik Nayak <karthik.188@gmail.com> wrote:\n> Add strbuf_utf8_align() which will align a given string into a strbuf\n> as per given align_type and width. If the width is greater than the\n> string length then no alignment is performed.\n\nIn addition to Junio's valuable comments...\n\n> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n> ---\n> diff --git a/utf8.h b/utf8.h\n> index 5a9e94b..db8ca63 100644\n> --- a/utf8.h\n> +++ b/utf8.h\n> @@ -55,4 +55,17 @@ 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> +\n> +/*\n> + * Align the string given and store it into a strbuf as per the type\n> + * and width.\n> + */\n\nPlease extend this documentation to state explicitly that this\nfunction preserves (does not truncate) the input string if it is wider\nthan 'width'. That's quite important information for the caller to\nknow.\n\n(Aside: I could easily see this function being extended to support\noptional truncation, but that's a separate topic, and something that\ncan be done by someone else when the feature is actually needed; it's\nnot your responsibility.)\n\n> +void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned int width,\n> +                      const char *s);\n> +\n>  #endif\n"},{"id":"268027","messageId":"CAOLa=ZSQ-YiBdtTByxJZFk8_Tj2RAAzzuZqbdk28httMM+rW-g@mail.gmail.com","threadId":"40040","inReplyTo":"CAPig+cRhgfOozy42UiDs24JFQTw5k8CYS0c+3DTOFB+XkPrLRQ@mail.gmail.com","subject":"Re: [PATCH v10 05/13] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-13T20:29:27Z","receivedAt":"2015-08-13T20:29:27Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Thu, Aug 13, 2015 at 11:56 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Sun, Aug 9, 2015 at 10:11 AM, Karthik Nayak <karthik.188@gmail.com> wrote:\n>> Implement an `align` atom which left-, middle-, or right-aligns the\n>> content between %(align:..) and %(end).\n>>\n>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n>> ---\n>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\n>> index e49d578..d949812 100644\n>> --- a/Documentation/git-for-each-ref.txt\n>> +++ b/Documentation/git-for-each-ref.txt\n>> @@ -127,6 +127,15 @@ color::\n>>         Change output color.  Followed by `:<colorname>`, where names\n>>         are described in `color.branch.*`.\n>>\n>> +align::\n>> +       Implement an `align` atom which left-, middle-, or\n>> +       right-aligns the content between %(align:..)  and\n>\n> This documentation seems to be copied a bit too literally from the\n> commit message. Saying \"Implement an `align` atom\" makes sense for the\n> commit message, but not for the documentation of the 'align' atom.\n> Instead,\n>\n>     Left-, middle-, or right-align the content between\n>     %(align:...) and %(end).\n>\n> would sound better.\n>\n\nOkay! Will do thanks!\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268029","messageId":"CAOLa=ZSZyfrHOe1Gjw457mbJCP5yW1uesDAW8zuv8DqoE+yuJQ@mail.gmail.com","threadId":"40040","inReplyTo":"CAPig+cS+rS=xKX-LrJSTqFgaopuLzbh3hqURFWXfzMXH=XUk2w@mail.gmail.com","subject":"Re: [PATCH v10 04/13] utf8: add function to align a string into given strbuf","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-13T20:55:44Z","receivedAt":"2015-08-13T20:55:44Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Fri, Aug 14, 2015 at 12:38 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Sun, Aug 9, 2015 at 10:11 AM, Karthik Nayak <karthik.188@gmail.com> wrote:\n>> Add strbuf_utf8_align() which will align a given string into a strbuf\n>> as per given align_type and width. If the width is greater than the\n>> string length then no alignment is performed.\n>\n> In addition to Junio's valuable comments...\n>\n>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n>> ---\n>> diff --git a/utf8.h b/utf8.h\n>> index 5a9e94b..db8ca63 100644\n>> --- a/utf8.h\n>> +++ b/utf8.h\n>> @@ -55,4 +55,17 @@ 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>> +\n>> +/*\n>> + * Align the string given and store it into a strbuf as per the type\n>> + * and width.\n>> + */\n>\n> Please extend this documentation to state explicitly that this\n> function preserves (does not truncate) the input string if it is wider\n> than 'width'. That's quite important information for the caller to\n> know.\n>\n> (Aside: I could easily see this function being extended to support\n> optional truncation, but that's a separate topic, and something that\n> can be done by someone else when the feature is actually needed; it's\n> not your responsibility.)\n>\n>> +void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned int width,\n>> +                      const char *s);\n>> +\n>>  #endif\n\nYeah will do :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268041","messageId":"CAOLa=ZQcd37Vq8j9sRYwnio0T3ua5C3frMc7-uEqLWsLo6eYYg@mail.gmail.com","threadId":"40040","inReplyTo":"xmqqr3n8jlvv.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v10 05/13] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-14T15:46:32Z","receivedAt":"2015-08-14T15:46:32Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Thu, Aug 13, 2015 at 1:54 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Karthik Nayak <karthik.188@gmail.com> writes:\n>\n>> On Wed, Aug 12, 2015 at 10:43 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> ...\n>>> %(objectname:abbrev=8).  To specify two modification magics, each of\n>>> which takes a number, the user would say e.g.\n>>>\n>>>     %(objectname:abbrev=8,magic=4)\n>>> ...\n>>> And that would be following %(align:8).  Both 'left' (implied\n>>> default) and '8' are instructing 'align' what to do.\n>>\n>> Will follow this. :)\n>\n> I think the most generic way to think about this is to consider that\n> the most fully spelled form of align would be this:\n>\n>         %(align:width=12,position=left)\n>\n> And another rule you would have is that the user is allowed to omit\n> \"<attr>=\" when it is obvious from its value.  For \"align\", 'left'\n> can only possibly be the value for 'position' and similarly '12' for\n> 'width'.  That is why the \"objectname\" example says \"abbrev=8\", and\n> not \"abbrev,8\", because from the value of \"8\" without the attribute\n> name, you cannot tell if the user meant abbrev=8 or magic=8.\n>\n> That '\"<attr>=\" can be omitted' rule makes both of these valid\n> constructs:\n>\n>         %(align:12,left) %(align:left,12)\n>\n\nAre you sure you want it to be so flexible? I mean It's nice to have it,\nbut it would include a lot more processing, I'd prefer having either the\ngeneric type you mentioned about so users know what they're typing\nor else a predefined way like\n\n           %(align:<width>,<position>)\n\n> Moreover, if you make \"left aligned\" the default behaviour when\n> position is not specified, you can make %(align:12) the shortest way\n> to spell the same thing.  Note that I said \"if you make\"; I do not\n> offhand know if all the internal callers of this mechanism your\n> updates to \"branch -l\" and \"tag -l\" would want left aligned output\n> (if so, that is one argument to favor making left-aligned the\n> implicit default; if not, it may be better to require the position\n> always specified).\n\nLeft align is what I'd prefer as default for branch -l and tag -l so\nthat will be\ndone.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268043","messageId":"xmqq1tf5kg7e.fsf@gitster.dls.corp.google.com","threadId":"40040","inReplyTo":"CAPig+cS+rS=xKX-LrJSTqFgaopuLzbh3hqURFWXfzMXH=XUk2w@mail.gmail.com","subject":"Re: [PATCH v10 04/13] utf8: add function to align a string into given strbuf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-14T16:06:13Z","receivedAt":"2015-08-14T16:06:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> Please extend this documentation to state explicitly that this\n> function preserves (does not truncate) the input string if it is wider\n> than 'width'. That's quite important information for the caller to\n> know.\n>\n> (Aside: I could easily see this function being extended to support\n> optional truncation, but that's a separate topic, and something that\n> can be done by someone else when the feature is actually needed; it's\n> not your responsibility.)\n\nThanks for being explicit.  That is exactly what I had in mind when\nI commented on this part with \"mental note\".\n\nExtending what the function does is outside the scope of this topic,\nbut documenting the current design decision is, in order to help\nthose who might want to do so.\n"},{"id":"268049","messageId":"xmqqh9o1ix6x.fsf@gitster.dls.corp.google.com","threadId":"40040","inReplyTo":"CAOLa=ZQcd37Vq8j9sRYwnio0T3ua5C3frMc7-uEqLWsLo6eYYg@mail.gmail.com","subject":"Re: [PATCH v10 05/13] ref-filter: implement an `align` atom","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-14T17:42:14Z","receivedAt":"2015-08-14T17:42:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> On Thu, Aug 13, 2015 at 1:54 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n>> I think the most generic way to think about this is to consider that\n>> the most fully spelled form of align would be this:\n>>\n>>         %(align:width=12,position=left)\n>>\n>> And another rule you would have is that the user is allowed to omit\n>> \"<attr>=\" when it is obvious from its value.  For \"align\", 'left'\n>> can only possibly be the value for 'position' and similarly '12' for\n>> 'width'.  That is why the \"objectname\" example says \"abbrev=8\", and\n>> not \"abbrev,8\", because from the value of \"8\" without the attribute\n>> name, you cannot tell if the user meant abbrev=8 or magic=8.\n>>\n>> That '\"<attr>=\" can be omitted' rule makes both of these valid\n>> constructs:\n>>\n>>         %(align:12,left) %(align:left,12)\n>>\n>\n> Are you sure you want it to be so flexible?\n\nEventually, yes.\n\nBut that does not mean you must do it by next week, and that is why\nI said \"way to think about is to consider\", not \"way to design this\nis to implement\".\n\nIn the worst case, if the only-allowed form the end-users can give\nwere limited to \"align:12,left\" and \"align:12\" and \"align:left,12\"\nfailed in the version you complete by the end of GSoC, it is still\nOK to ship it in 2.7, with a \"known bugs\" section for somebody\n(maybe you) to further polish in the future.  As long as we leave\nthe door open for such an enhancement that would not make what used\nto be valid invalid and only would allow what used to be invalid\nvalid, we would be fine---we can make progress without harming\nexisting users.\n"},{"id":"268117","messageId":"CACsJy8AFwcLZGQqg2Cv+fb2JBiZjh2tBpG4=8uxQdbUUmdZ-Pw@mail.gmail.com","threadId":"40040","inReplyTo":"1439129506-9989-5-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v10 04/13] utf8: add function to align a string into given strbuf","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-08-15T08:27:32Z","receivedAt":"2015-08-15T08:27:32Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Aug 9, 2015 at 9:11 PM, Karthik Nayak <karthik.188@gmail.com> wrote:\n> Add strbuf_utf8_align() which will align a given string into a strbuf\n> as per given align_type and width. If the width is greater than the\n> string length then no alignment is performed.\n\nI smell an opportunity to reuse this code and kill some in pretty.c's\nformat_and_pad_commit(). If you want, you can do it. Else this is my\npersonal note so I don't forget to do it later :)\n-- \nDuy\n"}]}