{"thread":{"id":"41023","subject":"[PATCH v2 00/11] ref-filter: use parsing functions","startedAt":"2015-12-16T15:29:51Z","lastAt":"2015-12-31T13:19:50Z","messageCount":34,"participants":["Karthik Nayak","Eric Sunshine"],"isPatch":true,"patchVersion":2,"patchTotal":11},"messages":[{"id":"274578","messageId":"1450279802-29414-1-git-send-email-Karthik.188@gmail.com","threadId":"41023","inReplyTo":null,"subject":"[PATCH v2 00/11] ref-filter: use parsing functions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-16T15:29:51Z","receivedAt":"2015-12-16T15:29:51Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"This series cleans up populate_value() in ref-filter, by moving out\nthe parsing part of atoms to separate parsing functions. This ensures\nthat parsing is only done once and also improves the modularity of the\ncode.\nThe previous version can be found here:\nhttp://thread.gmane.org/gmane.comp.version-control.git/281180\n\nChanges in this version:\n1. Re-ordered the patch series and split patches.\n2. Used 'enums' rather than 'structs' in 'used_atom' structure.\n3. Made all functions static to remove global scope (Thanks to Ramsay Jones).\n4. Added \"subject\" and \"body\" atom under contents_atom_parser.\n5. Changes in error messages, function names and variable names.\n6. Changes in comments and documentation.\n7. Added extensive test cases for the %(align) atom.\n\nThanks to Eric and Junio for suggestions on v1.\nTODO: Implement an enum to denote current atom in used_atom,\nso as to get rid of repetitive checks performed.\n(http://article.gmane.org/gmane.comp.version-control.git/282320).\n\nKarthik Nayak (11):\n strbuf: introduce strbuf_split_str_without_term()\n ref-filter: use strbuf_split_str_omit_term()\n ref-filter: introduce struct used_atom\n ref-fitler: bump match_atom() name to the top\n ref-filter: skip deref specifier in match_atom_name()\n ref-filter: introduce color_atom_parser()\n ref-filter: introduce align_atom_parser()\n ref-filter: introduce prefixes for the align atom\n ref-filter: introduce remote_ref_atom_parser()\n ref-filter: introduce contents_atom_parser()\n ref-filter: introduce objectname_atom_parser()\n\n Documentation/git-for-each-ref.txt | 20 +-\n ref-filter.c | 441 ++++++++++++++++++++++---------------\n strbuf.c | 7 +-\n strbuf.h | 25 ++-\n t/t6302-for-each-ref-filter.sh | 162 ++++++++++++++\n 5 files changed, 462 insertions(+), 193 deletions(-)\n\nInterdiff:\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 56ffdc1..9af0f53 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -133,8 +133,10 @@ align::\n \t`width=` and `position=` in any order\n \tseparated by a comma, where the `` is either left,\n \tright or middle, default being left and `` is the total\n-\tlength of the content with alignment. The prefix for the\n-\targuments is not mandatory. If the contents length is more\n+\tlength of the content with alignment. For brevity, the\n+\t\"width=\" and/or \"position=\" prefixes may be omitted, and bare\n+\t and used instead. For instance,\n+\t`%(align:,)`. If the contents length is more\n \tthan the width then no alignment is performed. If used with\n \t'--quote' everything in between %(align:...) and %(end) is\n \tquoted, but if nested then only the topmost level performs\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 3bd3e45..a8fd178 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -37,23 +37,13 @@ static struct used_atom {\n \tunion {\n \t\tconst char *color;\n \t\tstruct align align;\n+\t\tenum { RR_SHORTEN, RR_TRACK, RR_TRACKSHORT, RR_NORMAL }\n+\t\t\tremote_ref;\n \t\tstruct {\n-\t\t\tunsigned int shorten : 1,\n-\t\t\t\ttrack : 1,\n-\t\t\t\ttrackshort : 1;\n-\t\t} remote_ref;\n-\t\tstruct {\n-\t\t\tunsigned int subject : 1,\n-\t\t\t\tbody : 1,\n-\t\t\t\tsignature : 1,\n-\t\t\t\tall : 1,\n-\t\t\t\tlines : 1,\n-\t\t\t\tno_lines;\n+\t\t\tenum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;\n+\t\t\tunsigned int no_lines;\n \t\t} contents;\n-\t\tstruct {\n-\t\t\tunsigned int shorten : 1,\n-\t\t\t\tfull : 1;\n-\t\t} objectname;\n+\t\tenum { O_FULL, O_SHORT } objectname;\n \t} u;\n } *used_atom;\n static int used_atom_cnt, need_tagged, need_symref;\n@@ -79,81 +69,89 @@ static int match_atom_name(const char *name, const char *atom_name, const char *\n \treturn 1;\n }\n \n-void color_atom_parser(struct used_atom *atom)\n+static void color_atom_parser(struct used_atom *atom)\n {\n \tmatch_atom_name(atom->str, \"color\", &atom->u.color);\n \tif (!atom->u.color)\n \t\tdie(_(\"expected format: %%(color:)\"));\n }\n \n-void remote_ref_atom_parser(struct used_atom *atom)\n+static void remote_ref_atom_parser(struct used_atom *atom)\n {\n \tconst char *buf;\n \n \tbuf = strchr(atom->str, ':');\n+\tatom->u.remote_ref = RR_NORMAL;\n \tif (!buf)\n \t\treturn;\n \tbuf++;\n \tif (!strcmp(buf, \"short\"))\n-\t\tatom->u.remote_ref.shorten = 1;\n+\t\tatom->u.remote_ref = RR_SHORTEN;\n \telse if (!strcmp(buf, \"track\"))\n-\t\tatom->u.remote_ref.track = 1;\n+\t\tatom->u.remote_ref = RR_TRACK;\n \telse if (!strcmp(buf, \"trackshort\"))\n-\t\tatom->u.remote_ref.trackshort = 1;\n+\t\tatom->u.remote_ref = RR_TRACKSHORT;\n \telse\n-\t\tdie(_(\"improper format entered align:%s\"), buf);\n+\t\tdie(_(\"unrecognized format: %%(%s)\"), atom->str);\n }\n \n-void contents_atom_parser(struct used_atom *atom)\n+static void contents_atom_parser(struct used_atom *atom)\n {\n \tconst char * buf;\n \n \tif (match_atom_name(atom->str, \"contents\", &buf))\n-\t\tatom->u.contents.all = 1;\n-\n+\t\tatom->u.contents.option = C_BARE;\n+\telse if (match_atom_name(atom->str, \"subject\", &buf)) {\n+\t\tatom->u.contents.option = C_SUB;\n+\t\treturn;\n+\t} else if (match_atom_name(atom->str, \"body\", &buf)) {\n+\t\tatom->u.contents.option = C_BODY_DEP;\n+\t\treturn;\n+\t}\n \tif (!buf)\n \t\treturn;\n+\n \tif (!strcmp(buf, \"body\"))\n-\t\tatom->u.contents.body = 1;\n+\t\tatom->u.contents.option = C_BODY;\n \telse if (!strcmp(buf, \"signature\"))\n-\t\tatom->u.contents.signature = 1;\n+\t\tatom->u.contents.option = C_SIG;\n \telse if (!strcmp(buf, \"subject\"))\n-\t\tatom->u.contents.subject = 1;\n+\t\tatom->u.contents.option = C_SUB;\n \telse if (skip_prefix(buf, \"lines=\", &buf)) {\n-\t\tatom->u.contents.lines = 1;\n+\t\tatom->u.contents.option = C_LINES;\n \t\tif (strtoul_ui(buf, 10, &atom->u.contents.no_lines))\n \t\t\tdie(_(\"positive value expected contents:lines=%s\"), buf);\n \t} else\n-\t\tdie(_(\"improper format entered contents:%s\"), buf);\n+\t\tdie(_(\"unrecognized %%(contents) argument: %s\"), buf);\n }\n \n-void objectname_atom_parser(struct used_atom *atom)\n+static void objectname_atom_parser(struct used_atom *atom)\n {\n \tconst char * buf;\n \n \tif (match_atom_name(atom->str, \"objectname\", &buf))\n-\t\tatom->u.objectname.full = 1;\n-\n+\t\tatom->u.objectname = O_FULL;\n \tif (!buf)\n \t\treturn;\n+\n \tif (!strcmp(buf, \"short\"))\n-\t\tatom->u.objectname.shorten = 1;\n+\t\tatom->u.objectname = O_SHORT;\n \telse\n-\t\tdie(_(\"improper format entered objectname:%s\"), buf);\n+\t\tdie(_(\"unrecognized %%(objectname) argument: %s\"), buf);\n }\n \n-static align_type get_align_position(const char *type)\n+static align_type parse_align_position(const char *s)\n {\n-\tif (!strcmp(type, \"right\"))\n+\tif (!strcmp(s, \"right\"))\n \t\treturn ALIGN_RIGHT;\n-\telse if (!strcmp(type, \"middle\"))\n+\telse if (!strcmp(s, \"middle\"))\n \t\treturn ALIGN_MIDDLE;\n-\telse if (!strcmp(type, \"left\"))\n+\telse if (!strcmp(s, \"left\"))\n \t\treturn ALIGN_LEFT;\n \treturn -1;\n }\n \n-void align_atom_parser(struct used_atom *atom)\n+static void align_atom_parser(struct used_atom *atom)\n {\n \tstruct align *align = &atom->u.align;\n \tconst char *buf = NULL;\n@@ -163,31 +161,28 @@ void align_atom_parser(struct used_atom *atom)\n \tmatch_atom_name(atom->str, \"align\", &buf);\n \tif (!buf)\n \t\tdie(_(\"expected format: %%(align:,)\"));\n-\ts = to_free = strbuf_split_str_without_term(buf, ',', 0);\n+\ts = to_free = strbuf_split_str_omit_term(buf, ',', 0);\n \n-\t/* By default align to ALGIN_LEFT */\n \talign->position = ALIGN_LEFT;\n \n \twhile (*s) {\n \t\tint position;\n \t\tbuf = s[0]->buf;\n \n-\t\tposition = get_align_position(buf);\n-\n \t\tif (skip_prefix(buf, \"position=\", &buf)) {\n-\t\t\tposition = get_align_position(buf);\n+\t\t\tposition = parse_align_position(buf);\n \t\t\tif (position == -1)\n-\t\t\t\tdie(_(\"improper format entered align:%s\"), s[0]->buf);\n+\t\t\t\tdie(_(\"unrecognized position:%s\"), buf);\n \t\t\talign->position = position;\n \t\t} else if (skip_prefix(buf, \"width=\", &buf)) {\n \t\t\tif (strtoul_ui(buf, 10, (unsigned int *)&width))\n-\t\t\t\tdie(_(\"improper format entered align:%s\"), s[0]->buf);\n+\t\t\t\tdie(_(\"unrecognized width:%s\"), buf);\n \t\t} else if (!strtoul_ui(buf, 10, (unsigned int *)&width))\n-\t\t\t\t;\n-\t\telse if (position != -1)\n+\t\t\t;\n+\t\telse if ((position = parse_align_position(buf)) != -1)\n \t\t\talign->position = position;\n \t\telse\n-\t\t\tdie(_(\"improper format entered align:%s\"), s[0]->buf);\n+\t\t\tdie(_(\"unrecognized %%(align) argument: %s\"), s[0]->buf);\n \t\ts++;\n \t}\n \n@@ -226,8 +221,8 @@ static struct {\n \t{ \"taggerdate\", FIELD_TIME },\n \t{ \"creator\" },\n \t{ \"creatordate\", FIELD_TIME },\n-\t{ \"subject\" },\n-\t{ \"body\" },\n+\t{ \"subject\", FIELD_STR, contents_atom_parser },\n+\t{ \"body\", FIELD_STR, contents_atom_parser },\n \t{ \"contents\", FIELD_STR, contents_atom_parser },\n \t{ \"upstream\", FIELD_STR, remote_ref_atom_parser },\n \t{ \"push\", FIELD_STR, remote_ref_atom_parser },\n@@ -485,10 +480,10 @@ static int grab_objectname(const char *name, const unsigned char *sha1,\n \t\t\t struct atom_value *v, struct used_atom *atom)\n {\n \tif (starts_with(name, \"objectname\")) {\n-\t\tif (atom->u.objectname.shorten) {\n+\t\tif (atom->u.objectname == O_SHORT) {\n \t\t\tv->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));\n \t\t\treturn 1;\n-\t\t} else if (atom->u.objectname.full) {\n+\t\t} else if (atom->u.objectname == O_FULL) {\n \t\t\tv->s = xstrdup(sha1_to_hex(sha1));\n \t\t\treturn 1;\n \t\t}\n@@ -815,7 +810,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj\n \t\t\tname++;\n \t\tif (strcmp(name, \"subject\") &&\n \t\t strcmp(name, \"body\") &&\n-\t\t !atom->u.contents.all)\n+\t\t !starts_with(name, \"contents\"))\n \t\t\tcontinue;\n \t\tif (!subpos)\n \t\t\tfind_subpos(buf, sz,\n@@ -823,24 +818,22 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj\n \t\t\t\t &bodypos, &bodylen, &nonsiglen,\n \t\t\t\t &sigpos, &siglen);\n \n-\t\tif (!strcmp(name, \"subject\"))\n-\t\t\tv->s = copy_subject(subpos, sublen);\n-\t\telse if (atom->u.contents.subject)\n+\t\tif (atom->u.contents.option == C_SUB)\n \t\t\tv->s = copy_subject(subpos, sublen);\n-\t\telse if (!strcmp(name, \"body\"))\n+\t\telse if (atom->u.contents.option == C_BODY_DEP)\n \t\t\tv->s = xmemdupz(bodypos, bodylen);\n-\t\telse if (atom->u.contents.body)\n+\t\telse if (atom->u.contents.option == C_BODY)\n \t\t\tv->s = xmemdupz(bodypos, nonsiglen);\n-\t\telse if (atom->u.contents.signature)\n+\t\telse if (atom->u.contents.option == C_SIG)\n \t\t\tv->s = xmemdupz(sigpos, siglen);\n-\t\telse if (atom->u.contents.lines) {\n+\t\telse if (atom->u.contents.option == C_LINES) {\n \t\t\tstruct strbuf s = STRBUF_INIT;\n \t\t\tconst char *contents_end = bodylen + bodypos - siglen;\n \n \t\t\t/* Size is the length of the message after removing the signature */\n \t\t\tappend_lines(&s, subpos, contents_end - subpos, atom->u.contents.no_lines);\n \t\t\tv->s = strbuf_detach(&s, NULL);\n-\t\t} else /* For %(contents) without modifiers */\n+\t\t} else if (atom->u.contents.option == C_BARE)\n \t\t\tv->s = xstrdup(subpos);\n \t}\n }\n@@ -903,9 +896,9 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n \t\t\t\t struct branch *branch, const char **s)\n {\n \tint num_ours, num_theirs;\n-\tif (atom->u.remote_ref.shorten)\n+\tif (atom->u.remote_ref == RR_SHORTEN)\n \t\t*s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);\n-\telse if (atom->u.remote_ref.track) {\n+\telse if (atom->u.remote_ref == RR_TRACK) {\n \t\tif (stat_tracking_info(branch, &num_ours,\n \t\t\t\t &num_theirs, NULL))\n \t\t\treturn;\n@@ -918,7 +911,7 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n \t\telse\n \t\t\t*s = xstrfmt(\"[ahead %d, behind %d]\",\n \t\t\t\t num_ours, num_theirs);\n-\t} else if (atom->u.remote_ref.trackshort) {\n+\t} else if (atom->u.remote_ref == RR_TRACKSHORT) {\n \t\tif (stat_tracking_info(branch, &num_ours,\n \t\t\t\t &num_theirs, NULL))\n \t\t\treturn;\n@@ -931,7 +924,7 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n \t\t\t*s = \">\";\n \t\telse\n \t\t\t*s = \"<>\";\n-\t} else\n+\t} else if (atom->u.remote_ref == RR_NORMAL)\n \t\t*s = refname;\n }\n \n@@ -1003,11 +996,10 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\t\tcontinue;\n \t\t\tfill_remote_ref_details(atom, refname, branch, &v->s);\n \t\t\tcontinue;\n-\t\t} else if (starts_with(name, \"color\")) {\n+\t\t} else if (starts_with(name, \"color:\")) {\n \t\t\tchar color[COLOR_MAXLEN] = \"\";\n-\t\t\tconst char *valp = atom->u.color;\n \n-\t\t\tif (color_parse(valp, color) < 0)\n+\t\t\tif (color_parse(atom->u.color, color) < 0)\n \t\t\t\tdie(_(\"unable to parse format\"));\n \t\t\tv->s = xstrdup(color);\n \t\t\tcontinue;\ndiff --git a/strbuf.c b/strbuf.c\nindex d31336f..b62508e 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -115,7 +115,7 @@ void strbuf_tolower(struct strbuf *sb)\n }\n \n struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n-\t\t\t\t int terminator, int max, int with_term)\n+\t\t\t\t int terminator, int max, int omit_term)\n {\n \tstruct strbuf **ret = NULL;\n \tsize_t nr = 0, alloc = 0;\n@@ -123,27 +123,19 @@ struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n \n \twhile (slen) {\n \t\tint len = slen;\n-\t\tint term = with_term;\n+\t\tconst char *end = NULL;\n \t\tif (max <= 0 || nr + 1 < max) {\n-\t\t\tconst char *end = memchr(str, terminator, slen);\n+\t\t\tend = memchr(str, terminator, slen);\n \t\t\tif (end)\n-\t\t\t\tlen = end - str + term;\n-\t\t\telse\n-\t\t\t\t/* When no terminator present, we must add the last character */\n-\t\t\t\tterm = 1;\n+\t\t\t\tlen = end - str + 1;\n \t\t}\n \t\tt = xmalloc(sizeof(struct strbuf));\n \t\tstrbuf_init(t, len);\n-\t\tstrbuf_add(t, str, len);\n+\t\tstrbuf_add(t, str, len - !!end * !!omit_term);\n \t\tALLOC_GROW(ret, nr + 2, alloc);\n \t\tret[nr++] = t;\n-\t\tif (!term) {\n-\t\t\tstr += len + 1;\n-\t\t\tslen -= len + 1;\n-\t\t} else {\n-\t\t\tstr += len;\n-\t\t\tslen -= len;\n-\t\t}\n+\t\tstr += len;\n+\t\tslen -= len;\n \t}\n \tALLOC_GROW(ret, nr + 1, alloc); /* In case string was empty */\n \tret[nr] = NULL;\ndiff --git a/strbuf.h b/strbuf.h\nindex 63e1e69..a865a74 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -450,11 +450,12 @@ static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)\n /**\n * Split str (of length slen) at the specified terminator character.\n * Return a null-terminated array of pointers to strbuf objects\n- * holding the substrings. The substrings include the terminator,\n- * except for the last substring, which might be unterminated if the\n- * original string did not end with a terminator. If max is positive,\n- * then split the string into at most max substrings (with the last\n- * substring containing everything following the (max-1)th terminator\n+ * holding the substrings. The substrings include the terminator if\n+ * the value of omit_term is '0' else the terminator is excluded. The\n+ * last substring, might be unterminated if the original string did\n+ * not end with a terminator. If max is positive, then split the\n+ * string into at most max substrings (with the last substring\n+ * containing everything following the (max-1)th terminator\n * character).\n *\n * The most generic form is `strbuf_split_buf`, which takes an arbitrary\n@@ -466,24 +467,24 @@ static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)\n * string_list_split_in_place().\n */\n extern struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n-\t\t\t\t\tint terminator, int max, int with_term);\n+\t\t\t\t\tint terminator, int max, int omit_term);\n \n-static inline struct strbuf **strbuf_split_str_without_term(const char *str,\n+static inline struct strbuf **strbuf_split_str_omit_term(const char *str,\n \t\t\t\t\t\t\t int terminator, int max)\n {\n-\treturn strbuf_split_buf(str, strlen(str), terminator, max, 0);\n+\treturn strbuf_split_buf(str, strlen(str), terminator, max, 1);\n }\n \n static inline struct strbuf **strbuf_split_str(const char *str,\n \t\t\t\t\t int terminator, int max)\n {\n-\treturn strbuf_split_buf(str, strlen(str), terminator, max, 1);\n+\treturn strbuf_split_buf(str, strlen(str), terminator, max, 0);\n }\n \n static inline struct strbuf **strbuf_split_max(const struct strbuf *sb,\n \t\t\t\t\t\tint terminator, int max)\n {\n-\treturn strbuf_split_buf(sb->buf, sb->len, terminator, max, 1);\n+\treturn strbuf_split_buf(sb->buf, sb->len, terminator, max, 0);\n }\n \n static inline struct strbuf **strbuf_split(const struct strbuf *sb,\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 688751e..bcb6771 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -97,7 +97,7 @@ test_expect_success 'left alignment is default' '\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:width=30)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\tgit for-each-ref --format=\"%(align:30)refname is %(refname)%(end)|%(refname)\" >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -113,7 +113,7 @@ test_expect_success 'middle alignment' '\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:position=middle,30)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\tgit for-each-ref --format=\"|%(align:middle,30)refname is %(refname)%(end)|%(refname)\" >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -133,6 +133,168 @@ test_expect_success 'right alignment' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'alignment with \"position\" prefix' '\n+\tcat >expect <<-\\EOF &&\n+\t| refname is refs/heads/master|refs/heads/master\n+\t| refname is refs/heads/side|refs/heads/side\n+\t| refname is refs/odd/spot|refs/odd/spot\n+\t|refname is refs/tags/double-tag|refs/tags/double-tag\n+\t| refname is refs/tags/four|refs/tags/four\n+\t| refname is refs/tags/one|refs/tags/one\n+\t|refname is refs/tags/signed-tag|refs/tags/signed-tag\n+\t| refname is refs/tags/three|refs/tags/three\n+\t| refname is refs/tags/two|refs/tags/two\n+\tEOF\n+\tgit for-each-ref --format=\"|%(align:30,position=right)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'alignment with \"position\" prefix' '\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:position=right,30)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'alignment with \"width\" prefix' '\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:width=30,right)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'alignment with \"width\" prefix' '\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,width=30)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'alignment with \"position\" and \"width\" prefix' '\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:width=30,position=right)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'alignment with \"position\" and \"width\" prefix' '\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:position=right,width=30)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+# Last one wins (silently) when multiple arguments of the same type are given\n+\n+test_expect_success 'alignment with multiple \"\" values' '\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:32,width=30)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'alignment with multiple \"\" values' '\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:width=30,32)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'alignment with multiple \"\" values' '\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:width=30,position=middle,right)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'alignment with multiple \"\" values' '\n+\tcat >expect <<-\\EOF &&\n+\t| refname is refs/heads/master |refs/heads/master\n+\t| refname is refs/heads/side |refs/heads/side\n+\t| refname is refs/odd/spot |refs/odd/spot\n+\t|refname is refs/tags/double-tag|refs/tags/double-tag\n+\t| refname is refs/tags/four |refs/tags/four\n+\t| refname is refs/tags/one |refs/tags/one\n+\t|refname is refs/tags/signed-tag|refs/tags/signed-tag\n+\t| refname is refs/tags/three |refs/tags/three\n+\t| refname is refs/tags/two |refs/tags/two\n+\tEOF\n+\tgit for-each-ref --format=\"|%(align:30,right,position=middle)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n # Individual atoms inside %(align:...) and %(end) must not be quoted.\n \n test_expect_success 'alignment with format quote' \"\n\n-- \n2.6.4\n"},{"id":"274579","messageId":"1450279802-29414-2-git-send-email-Karthik.188@gmail.com","threadId":"41023","inReplyTo":"1450279802-29414-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v2 01/11] strbuf: introduce strbuf_split_str_without_term()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-16T15:29:52Z","receivedAt":"2015-12-16T15:29:52Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The current implementation of 'strbuf_split_buf()' includes the\nterminator at the end of each strbuf post splitting. Add an option\nwherein we can drop the terminator if desired. In this context\nintroduce a wrapper function 'strbuf_split_str_without_term()' which\nsplits a given string into strbufs without including the terminator.\n\nSigned-off-by: Karthik Nayak \n---\n strbuf.c | 7 ++++---\n strbuf.h | 25 ++++++++++++++++---------\n 2 files changed, 20 insertions(+), 12 deletions(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex b552a13..b62508e 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -115,7 +115,7 @@ void strbuf_tolower(struct strbuf *sb)\n }\n \n struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n-\t\t\t\t int terminator, int max)\n+\t\t\t\t int terminator, int max, int omit_term)\n {\n \tstruct strbuf **ret = NULL;\n \tsize_t nr = 0, alloc = 0;\n@@ -123,14 +123,15 @@ struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n \n \twhile (slen) {\n \t\tint len = slen;\n+\t\tconst char *end = NULL;\n \t\tif (max <= 0 || nr + 1 < max) {\n-\t\t\tconst char *end = memchr(str, terminator, slen);\n+\t\t\tend = memchr(str, terminator, slen);\n \t\t\tif (end)\n \t\t\t\tlen = end - str + 1;\n \t\t}\n \t\tt = xmalloc(sizeof(struct strbuf));\n \t\tstrbuf_init(t, len);\n-\t\tstrbuf_add(t, str, len);\n+\t\tstrbuf_add(t, str, len - !!end * !!omit_term);\n \t\tALLOC_GROW(ret, nr + 2, alloc);\n \t\tret[nr++] = t;\n \t\tstr += len;\ndiff --git a/strbuf.h b/strbuf.h\nindex 6ae7a72..a865a74 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -450,11 +450,12 @@ static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)\n /**\n * Split str (of length slen) at the specified terminator character.\n * Return a null-terminated array of pointers to strbuf objects\n- * holding the substrings. The substrings include the terminator,\n- * except for the last substring, which might be unterminated if the\n- * original string did not end with a terminator. If max is positive,\n- * then split the string into at most max substrings (with the last\n- * substring containing everything following the (max-1)th terminator\n+ * holding the substrings. The substrings include the terminator if\n+ * the value of omit_term is '0' else the terminator is excluded. The\n+ * last substring, might be unterminated if the original string did\n+ * not end with a terminator. If max is positive, then split the\n+ * string into at most max substrings (with the last substring\n+ * containing everything following the (max-1)th terminator\n * character).\n *\n * The most generic form is `strbuf_split_buf`, which takes an arbitrary\n@@ -465,19 +466,25 @@ static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)\n * For lighter-weight alternatives, see string_list_split() and\n * string_list_split_in_place().\n */\n-extern struct strbuf **strbuf_split_buf(const char *, size_t,\n-\t\t\t\t\tint terminator, int max);\n+extern struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n+\t\t\t\t\tint terminator, int max, int omit_term);\n+\n+static inline struct strbuf **strbuf_split_str_omit_term(const char *str,\n+\t\t\t\t\t\t\t int terminator, int max)\n+{\n+\treturn strbuf_split_buf(str, strlen(str), terminator, max, 1);\n+}\n \n static inline struct strbuf **strbuf_split_str(const char *str,\n \t\t\t\t\t int terminator, int max)\n {\n-\treturn strbuf_split_buf(str, strlen(str), terminator, max);\n+\treturn strbuf_split_buf(str, strlen(str), terminator, max, 0);\n }\n \n static inline struct strbuf **strbuf_split_max(const struct strbuf *sb,\n \t\t\t\t\t\tint terminator, int max)\n {\n-\treturn strbuf_split_buf(sb->buf, sb->len, terminator, max);\n+\treturn strbuf_split_buf(sb->buf, sb->len, terminator, max, 0);\n }\n \n static inline struct strbuf **strbuf_split(const struct strbuf *sb,\n-- \n2.6.4\n"},{"id":"274581","messageId":"1450279802-29414-3-git-send-email-Karthik.188@gmail.com","threadId":"41023","inReplyTo":"1450279802-29414-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v2 02/11] ref-filter: use strbuf_split_str_omit_term()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-16T15:29:53Z","receivedAt":"2015-12-16T15:29:53Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Use the newly introduced strbuf_split_str_omit_term() rather than\nusing strbuf_split_str() and manually removing the ',' terminator.\n\nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 9 +--------\n 1 file changed, 1 insertion(+), 8 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex e205dd2..6263710 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -869,18 +869,11 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\tif (!valp)\n \t\t\t\tdie(_(\"expected format: %%(align:,)\"));\n \n-\t\t\t/*\n-\t\t\t * TODO: Implement a function similar to strbuf_split_str()\n-\t\t\t * which would omit the separator from the end of each value.\n-\t\t\t */\n-\t\t\ts = to_free = strbuf_split_str(valp, ',', 0);\n+\t\t\ts = to_free = strbuf_split_str_omit_term(valp, ',', 0);\n \n \t\t\talign->position = ALIGN_LEFT;\n \n \t\t\twhile (*s) {\n-\t\t\t\t/* Strip trailing comma */\n-\t\t\t\tif (s[1])\n-\t\t\t\t\tstrbuf_setlen(s[0], s[0]->len - 1);\n \t\t\t\tif (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))\n \t\t\t\t\t;\n \t\t\t\telse if (!strcmp(s[0]->buf, \"left\"))\n-- \n2.6.4\n"},{"id":"274580","messageId":"1450279802-29414-4-git-send-email-Karthik.188@gmail.com","threadId":"41023","inReplyTo":"1450279802-29414-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v2 03/11] ref-filter: introduce struct used_atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-16T15:29:54Z","receivedAt":"2015-12-16T15:29:54Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Introduce the 'used_array' structure which would replace the existing\nimplementation of 'used_array' (which a list of atoms). This helps us\nparse atom's before hand and store required details into the\n'used_array' for future usage.\n\nAlso introduce a parsing function for each atom in valid_atom. Using\nthis we can define special parsing functions for each of the atoms.\n\nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 63 +++++++++++++++++++++++++++++++-----------------------------\n 1 file changed, 33 insertions(+), 30 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 6263710..043a02a 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -16,9 +16,27 @@\n \n typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n \n+/*\n+ * An atom is a valid field atom listed below, possibly prefixed with\n+ * a \"*\" to denote deref_tag().\n+ *\n+ * We parse given format string and sort specifiers, and make a list\n+ * of properties that we need to extract out of objects. ref_array_item\n+ * structure will hold an array of values extracted that can be\n+ * indexed with the \"atom number\", which is an index into this\n+ * array.\n+ */\n+static struct used_atom {\n+\tconst char *str;\n+\tcmp_type type;\n+} *used_atom;\n+static int used_atom_cnt, need_tagged, need_symref;\n+static int need_color_reset_at_eol;\n+\n static struct {\n \tconst char *name;\n \tcmp_type cmp_type;\n+\tvoid (*parser)(struct used_atom *atom);\n } valid_atom[] = {\n \t{ \"refname\" },\n \t{ \"objecttype\" },\n@@ -92,21 +110,6 @@ struct atom_value {\n };\n \n /*\n- * An atom is a valid field atom listed above, possibly prefixed with\n- * a \"*\" to denote deref_tag().\n- *\n- * We parse given format string and sort specifiers, and make a list\n- * of properties that we need to extract out of objects. ref_array_item\n- * structure will hold an array of values extracted that can be\n- * indexed with the \"atom number\", which is an index into this\n- * array.\n- */\n-static const char **used_atom;\n-static cmp_type *used_atom_type;\n-static int used_atom_cnt, need_tagged, need_symref;\n-static int need_color_reset_at_eol;\n-\n-/*\n * Used to parse format string and sort specifiers\n */\n int parse_ref_filter_atom(const char *atom, const char *ep)\n@@ -122,8 +125,8 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n \n \t/* Do we have the atom already used elsewhere? */\n \tfor (i = 0; i < used_atom_cnt; i++) {\n-\t\tint len = strlen(used_atom[i]);\n-\t\tif (len == ep - atom && !memcmp(used_atom[i], atom, len))\n+\t\tint len = strlen(used_atom[i].str);\n+\t\tif (len == ep - atom && !memcmp(used_atom[i].str, atom, len))\n \t\t\treturn i;\n \t}\n \n@@ -150,12 +153,11 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n \tat = used_atom_cnt;\n \tused_atom_cnt++;\n \tREALLOC_ARRAY(used_atom, used_atom_cnt);\n-\tREALLOC_ARRAY(used_atom_type, used_atom_cnt);\n-\tused_atom[at] = xmemdupz(atom, ep - atom);\n-\tused_atom_type[at] = valid_atom[i].cmp_type;\n+\tused_atom[at].str = xmemdupz(atom, ep - atom);\n+\tused_atom[at].type = valid_atom[i].cmp_type;\n \tif (*atom == '*')\n \t\tneed_tagged = 1;\n-\tif (!strcmp(used_atom[at], \"symref\"))\n+\tif (!strcmp(used_atom[at].str, \"symref\"))\n \t\tneed_symref = 1;\n \treturn at;\n }\n@@ -315,7 +317,7 @@ int verify_ref_format(const char *format)\n \t\tat = parse_ref_filter_atom(sp + 2, ep);\n \t\tcp = ep + 1;\n \n-\t\tif (skip_prefix(used_atom[at], \"color:\", &color))\n+\t\tif (skip_prefix(used_atom[at].str, \"color:\", &color))\n \t\t\tneed_color_reset_at_eol = !!strcmp(color, \"reset\");\n \t}\n \treturn 0;\n@@ -359,7 +361,7 @@ static void grab_common_values(struct atom_value *val, int deref, struct object\n \tint i;\n \n \tfor (i = 0; i < used_atom_cnt; i++) {\n-\t\tconst char *name = used_atom[i];\n+\t\tconst char *name = used_atom[i].str;\n \t\tstruct atom_value *v = &val[i];\n \t\tif (!!deref != (*name == '*'))\n \t\t\tcontinue;\n@@ -383,7 +385,7 @@ static void grab_tag_values(struct atom_value *val, int deref, struct object *ob\n \tstruct tag *tag = (struct tag *) obj;\n \n \tfor (i = 0; i < used_atom_cnt; i++) {\n-\t\tconst char *name = used_atom[i];\n+\t\tconst char *name = used_atom[i].str;\n \t\tstruct atom_value *v = &val[i];\n \t\tif (!!deref != (*name == '*'))\n \t\t\tcontinue;\n@@ -405,7 +407,7 @@ static void grab_commit_values(struct atom_value *val, int deref, struct object\n \tstruct commit *commit = (struct commit *) obj;\n \n \tfor (i = 0; i < used_atom_cnt; i++) {\n-\t\tconst char *name = used_atom[i];\n+\t\tconst char *name = used_atom[i].str;\n \t\tstruct atom_value *v = &val[i];\n \t\tif (!!deref != (*name == '*'))\n \t\t\tcontinue;\n@@ -535,7 +537,7 @@ static void grab_person(const char *who, struct atom_value *val, int deref, stru\n \tconst char *wholine = NULL;\n \n \tfor (i = 0; i < used_atom_cnt; i++) {\n-\t\tconst char *name = used_atom[i];\n+\t\tconst char *name = used_atom[i].str;\n \t\tstruct atom_value *v = &val[i];\n \t\tif (!!deref != (*name == '*'))\n \t\t\tcontinue;\n@@ -573,7 +575,7 @@ static void grab_person(const char *who, struct atom_value *val, int deref, stru\n \tif (!wholine)\n \t\treturn;\n \tfor (i = 0; i < used_atom_cnt; i++) {\n-\t\tconst char *name = used_atom[i];\n+\t\tconst char *name = used_atom[i].str;\n \t\tstruct atom_value *v = &val[i];\n \t\tif (!!deref != (*name == '*'))\n \t\t\tcontinue;\n@@ -663,7 +665,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj\n \tunsigned long sublen = 0, bodylen = 0, nonsiglen = 0, siglen = 0;\n \n \tfor (i = 0; i < used_atom_cnt; i++) {\n-\t\tconst char *name = used_atom[i];\n+\t\tconst char *name = used_atom[i].str;\n \t\tstruct atom_value *v = &val[i];\n \t\tconst char *valp = NULL;\n \t\tif (!!deref != (*name == '*'))\n@@ -786,7 +788,8 @@ static void populate_value(struct ref_array_item *ref)\n \n \t/* Fill in specials first */\n \tfor (i = 0; i < used_atom_cnt; i++) {\n-\t\tconst char *name = used_atom[i];\n+\t\tstruct used_atom *atom = &used_atom[i];\n+\t\tconst char *name = atom->str;\n \t\tstruct atom_value *v = &ref->value[i];\n \t\tint deref = 0;\n \t\tconst char *refname;\n@@ -1438,7 +1441,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n {\n \tstruct atom_value *va, *vb;\n \tint cmp;\n-\tcmp_type cmp_type = used_atom_type[s->atom];\n+\tcmp_type cmp_type = used_atom[s->atom].type;\n \n \tget_ref_atom_value(a, s->atom, &va);\n \tget_ref_atom_value(b, s->atom, &vb);\n-- \n2.6.4\n"},{"id":"274582","messageId":"1450279802-29414-5-git-send-email-Karthik.188@gmail.com","threadId":"41023","inReplyTo":"1450279802-29414-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v2 04/11] ref-fitler: bump match_atom() name to the top","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-16T15:29:55Z","receivedAt":"2015-12-16T15:29:55Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Bump match_atom() to the top for usage in further patches.\n\nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 32 ++++++++++++++++----------------\n 1 file changed, 16 insertions(+), 16 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 043a02a..f4a6414 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -33,6 +33,22 @@ static struct used_atom {\n static int used_atom_cnt, need_tagged, need_symref;\n static int need_color_reset_at_eol;\n \n+static int match_atom_name(const char *name, const char *atom_name, const char **val)\n+{\n+\tconst char *body;\n+\n+\tif (!skip_prefix(name, atom_name, &body))\n+\t\treturn 0; /* doesn't even begin with \"atom_name\" */\n+\tif (!body[0]) {\n+\t\t*val = NULL; /* %(atom_name) and no customization */\n+\t\treturn 1;\n+\t}\n+\tif (body[0] != ':')\n+\t\treturn 0; /* \"atom_namefoo\" is not \"atom_name\" or \"atom_name:...\" */\n+\t*val = body + 1; /* \"atom_name:val\" */\n+\treturn 1;\n+}\n+\n static struct {\n \tconst char *name;\n \tcmp_type cmp_type;\n@@ -260,22 +276,6 @@ static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_sta\n \tpop_stack_element(&state->stack);\n }\n \n-static int match_atom_name(const char *name, const char *atom_name, const char **val)\n-{\n-\tconst char *body;\n-\n-\tif (!skip_prefix(name, atom_name, &body))\n-\t\treturn 0; /* doesn't even begin with \"atom_name\" */\n-\tif (!body[0]) {\n-\t\t*val = NULL; /* %(atom_name) and no customization */\n-\t\treturn 1;\n-\t}\n-\tif (body[0] != ':')\n-\t\treturn 0; /* \"atom_namefoo\" is not \"atom_name\" or \"atom_name:...\" */\n-\t*val = body + 1; /* \"atom_name:val\" */\n-\treturn 1;\n-}\n-\n /*\n * In a format string, find the next occurrence of %(atom).\n */\n-- \n2.6.4\n"},{"id":"274584","messageId":"1450279802-29414-6-git-send-email-Karthik.188@gmail.com","threadId":"41023","inReplyTo":"1450279802-29414-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v2 05/11] ref-filter: skip deref specifier in match_atom_name()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-16T15:29:56Z","receivedAt":"2015-12-16T15:29:56Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"In upcoming patches we make calls to match_atom_name() with the '*'\nderef specifier still attached to the atom name. This causes\nundesirable errors, hence, if present skip over the '*' deref\nspecifier in the atom name.\n\nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex f4a6414..7d33b83 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -37,6 +37,10 @@ static int match_atom_name(const char *name, const char *atom_name, const char *\n {\n \tconst char *body;\n \n+\t/* skip the deref specifier*/\n+\tif (name[0] == '*')\n+\t\tname++;\n+\n \tif (!skip_prefix(name, atom_name, &body))\n \t\treturn 0; /* doesn't even begin with \"atom_name\" */\n \tif (!body[0]) {\n-- \n2.6.4\n"},{"id":"274583","messageId":"1450279802-29414-7-git-send-email-Karthik.188@gmail.com","threadId":"41023","inReplyTo":"1450279802-29414-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v2 06/11] ref-filter: introduce color_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-16T15:29:57Z","receivedAt":"2015-12-16T15:29:57Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Introduce color_atom_parser() which will parse a \"color\" atom and\nstore its color in the \"use_atom\" structure for further usage in\n'populate_value()'.\n\nHelped-by: Ramsay Jones \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 21 ++++++++++++++++-----\n 1 file changed, 16 insertions(+), 5 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 7d33b83..3b61c62 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -29,6 +29,9 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n static struct used_atom {\n \tconst char *str;\n \tcmp_type type;\n+\tunion {\n+\t\tconst char *color;\n+\t} u;\n } *used_atom;\n static int used_atom_cnt, need_tagged, need_symref;\n static int need_color_reset_at_eol;\n@@ -53,6 +56,13 @@ static int match_atom_name(const char *name, const char *atom_name, const char *\n \treturn 1;\n }\n \n+static void color_atom_parser(struct used_atom *atom)\n+{\n+\tmatch_atom_name(atom->str, \"color\", &atom->u.color);\n+\tif (!atom->u.color)\n+\t\tdie(_(\"expected format: %%(color:)\"));\n+}\n+\n static struct {\n \tconst char *name;\n \tcmp_type cmp_type;\n@@ -90,7 +100,7 @@ static struct {\n \t{ \"symref\" },\n \t{ \"flag\" },\n \t{ \"HEAD\" },\n-\t{ \"color\" },\n+\t{ \"color\", FIELD_STR, color_atom_parser },\n \t{ \"align\" },\n \t{ \"end\" },\n };\n@@ -175,6 +185,9 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n \tREALLOC_ARRAY(used_atom, used_atom_cnt);\n \tused_atom[at].str = xmemdupz(atom, ep - atom);\n \tused_atom[at].type = valid_atom[i].cmp_type;\n+\tmemset(&used_atom[at].u, 0, sizeof(used_atom[at].u));\n+\tif (valid_atom[i].parser)\n+\t\tvalid_atom[i].parser(&used_atom[at]);\n \tif (*atom == '*')\n \t\tneed_tagged = 1;\n \tif (!strcmp(used_atom[at].str, \"symref\"))\n@@ -833,12 +846,10 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\trefname = branch_get_push(branch, NULL);\n \t\t\tif (!refname)\n \t\t\t\tcontinue;\n-\t\t} else if (match_atom_name(name, \"color\", &valp)) {\n+\t\t} else if (starts_with(name, \"color:\")) {\n \t\t\tchar color[COLOR_MAXLEN] = \"\";\n \n-\t\t\tif (!valp)\n-\t\t\t\tdie(_(\"expected format: %%(color:)\"));\n-\t\t\tif (color_parse(valp, color) < 0)\n+\t\t\tif (color_parse(atom->u.color, color) < 0)\n \t\t\t\tdie(_(\"unable to parse format\"));\n \t\t\tv->s = xstrdup(color);\n \t\t\tcontinue;\n-- \n2.6.4\n"},{"id":"274585","messageId":"1450279802-29414-8-git-send-email-Karthik.188@gmail.com","threadId":"41023","inReplyTo":"1450279802-29414-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v2 07/11] ref-filter: introduce align_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-16T15:29:58Z","receivedAt":"2015-12-16T15:29:58Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Introduce align_atom_parser() which will parse an \"align\" atom and\nstore the required alignment position and width in the \"use_atom\"\nstructure for further usage in 'populate_value()'.\n\nHelped-by: Ramsay Jones \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 86 +++++++++++++++++++++++++++++++++++-------------------------\n 1 file changed, 51 insertions(+), 35 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 3b61c62..a44673c 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -16,6 +16,11 @@\n \n typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n \n+struct align {\n+\talign_type position;\n+\tunsigned int width;\n+};\n+\n /*\n * An atom is a valid field atom listed below, possibly prefixed with\n * a \"*\" to denote deref_tag().\n@@ -31,6 +36,7 @@ static struct used_atom {\n \tcmp_type type;\n \tunion {\n \t\tconst char *color;\n+\t\tstruct align align;\n \t} u;\n } *used_atom;\n static int used_atom_cnt, need_tagged, need_symref;\n@@ -63,6 +69,49 @@ static void color_atom_parser(struct used_atom *atom)\n \t\tdie(_(\"expected format: %%(color:)\"));\n }\n \n+static align_type parse_align_position(const char *s)\n+{\n+\tif (!strcmp(s, \"right\"))\n+\t\treturn ALIGN_RIGHT;\n+\telse if (!strcmp(s, \"middle\"))\n+\t\treturn ALIGN_MIDDLE;\n+\telse if (!strcmp(s, \"left\"))\n+\t\treturn ALIGN_LEFT;\n+\treturn -1;\n+}\n+\n+static void align_atom_parser(struct used_atom *atom)\n+{\n+\tstruct align *align = &atom->u.align;\n+\tconst char *buf = NULL;\n+\tstruct strbuf **s, **to_free;\n+\tint width = -1;\n+\n+\tmatch_atom_name(atom->str, \"align\", &buf);\n+\tif (!buf)\n+\t\tdie(_(\"expected format: %%(align:,)\"));\n+\ts = to_free = strbuf_split_str_omit_term(buf, ',', 0);\n+\n+\talign->position = ALIGN_LEFT;\n+\n+\twhile (*s) {\n+\t\tint position;\n+\n+\t\tif (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))\n+\t\t\t;\n+\t\telse if ((position = parse_align_position(s[0]->buf)) > 0)\n+\t\t\talign->position = position;\n+\t\telse\n+\t\t\tdie(_(\"unrecognized %%(align) argument: %s\"), s[0]->buf);\n+\t\ts++;\n+\t}\n+\n+\tif (width < 0)\n+\t\tdie(_(\"positive width expected with the %%(align) atom\"));\n+\talign->width = width;\n+\tstrbuf_list_free(to_free);\n+}\n+\n static struct {\n \tconst char *name;\n \tcmp_type cmp_type;\n@@ -101,17 +150,12 @@ static struct {\n \t{ \"flag\" },\n \t{ \"HEAD\" },\n \t{ \"color\", FIELD_STR, color_atom_parser },\n-\t{ \"align\" },\n+\t{ \"align\", FIELD_STR, align_atom_parser },\n \t{ \"end\" },\n };\n \n #define REF_FORMATTING_STATE_INIT { 0, NULL }\n \n-struct align {\n-\talign_type position;\n-\tunsigned int width;\n-};\n-\n struct contents {\n \tunsigned int lines;\n \tstruct object_id oid;\n@@ -880,35 +924,7 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\t\tv->s = \" \";\n \t\t\tcontinue;\n \t\t} else if (match_atom_name(name, \"align\", &valp)) {\n-\t\t\tstruct align *align = &v->u.align;\n-\t\t\tstruct strbuf **s, **to_free;\n-\t\t\tint width = -1;\n-\n-\t\t\tif (!valp)\n-\t\t\t\tdie(_(\"expected format: %%(align:,)\"));\n-\n-\t\t\ts = to_free = strbuf_split_str_omit_term(valp, ',', 0);\n-\n-\t\t\talign->position = ALIGN_LEFT;\n-\n-\t\t\twhile (*s) {\n-\t\t\t\tif (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))\n-\t\t\t\t\t;\n-\t\t\t\telse if (!strcmp(s[0]->buf, \"left\"))\n-\t\t\t\t\talign->position = ALIGN_LEFT;\n-\t\t\t\telse if (!strcmp(s[0]->buf, \"right\"))\n-\t\t\t\t\talign->position = ALIGN_RIGHT;\n-\t\t\t\telse if (!strcmp(s[0]->buf, \"middle\"))\n-\t\t\t\t\talign->position = ALIGN_MIDDLE;\n-\t\t\t\telse\n-\t\t\t\t\tdie(_(\"improper format entered align:%s\"), s[0]->buf);\n-\t\t\t\ts++;\n-\t\t\t}\n-\n-\t\t\tif (width < 0)\n-\t\t\t\tdie(_(\"positive width expected with the %%(align) atom\"));\n-\t\t\talign->width = width;\n-\t\t\tstrbuf_list_free(to_free);\n+\t\t\tv->u.align = atom->u.align;\n \t\t\tv->handler = align_atom_handler;\n \t\t\tcontinue;\n \t\t} else if (!strcmp(name, \"end\")) {\n-- \n2.6.4\n"},{"id":"274586","messageId":"1450279802-29414-9-git-send-email-Karthik.188@gmail.com","threadId":"41023","inReplyTo":"1450279802-29414-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v2 08/11] ref-filter: introduce prefixes for the align atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-16T15:29:59Z","receivedAt":"2015-12-16T15:29:59Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Introduce optional prefixes \"width=\" and \"position=\" for the align atom\nso that the atom can be used as \"%(align:width=,position=)\".\n\nAdd Documetation and tests for the same.\n\nSigned-off-by: Karthik Nayak \n---\n Documentation/git-for-each-ref.txt | 20 +++--\n ref-filter.c | 13 ++-\n t/t6302-for-each-ref-filter.sh | 162 +++++++++++++++++++++++++++++++++++++\n 3 files changed, 185 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex c6f073c..9af0f53 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -129,14 +129,18 @@ color::\n \n align::\n \tLeft-, middle-, or right-align the content between\n-\t%(align:...) and %(end). The \"align:\" is followed by ``\n-\tand `` in any order separated by a comma, where the\n-\t`` is either left, right or middle, default being\n-\tleft and `` is the total length of the content with\n-\talignment. If the contents length is more than the width then\n-\tno alignment is performed. If used with '--quote' everything\n-\tin between %(align:...) and %(end) is quoted, but if nested\n-\tthen only the topmost level performs quoting.\n+\t%(align:...) and %(end). The \"align:\" is followed by\n+\t`width=` and `position=` in any order\n+\tseparated by a comma, where the `` is either left,\n+\tright or middle, default being left and `` is the total\n+\tlength of the content with alignment. For brevity, the\n+\t\"width=\" and/or \"position=\" prefixes may be omitted, and bare\n+\t and used instead. For instance,\n+\t`%(align:,)`. If the contents length is more\n+\tthan the width then no alignment is performed. If used with\n+\t'--quote' everything in between %(align:...) and %(end) is\n+\tquoted, but if nested then only the topmost level performs\n+\tquoting.\n \n In addition to the above, for commit and tag objects, the header\n field names (`tree`, `parent`, `object`, `type`, and `tag`) can\ndiff --git a/ref-filter.c b/ref-filter.c\nindex a44673c..985423b 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -96,10 +96,19 @@ static void align_atom_parser(struct used_atom *atom)\n \n \twhile (*s) {\n \t\tint position;\n+\t\tbuf = s[0]->buf;\n \n-\t\tif (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))\n+\t\tif (skip_prefix(buf, \"position=\", &buf)) {\n+\t\t\tposition = parse_align_position(buf);\n+\t\t\tif (position == -1)\n+\t\t\t\tdie(_(\"unrecognized position:%s\"), buf);\n+\t\t\talign->position = position;\n+\t\t} else if (skip_prefix(buf, \"width=\", &buf)) {\n+\t\t\tif (strtoul_ui(buf, 10, (unsigned int *)&width))\n+\t\t\t\tdie(_(\"unrecognized width:%s\"), buf);\n+\t\t} else if (!strtoul_ui(buf, 10, (unsigned int *)&width))\n \t\t\t;\n-\t\telse if ((position = parse_align_position(s[0]->buf)) > 0)\n+\t\telse if ((position = parse_align_position(buf)) != -1)\n \t\t\talign->position = position;\n \t\telse\n \t\t\tdie(_(\"unrecognized %%(align) argument: %s\"), s[0]->buf);\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex fe4796c..bcb6771 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -133,6 +133,168 @@ test_expect_success 'right alignment' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'alignment with \"position\" prefix' '\n+\tcat >expect <<-\\EOF &&\n+\t| refname is refs/heads/master|refs/heads/master\n+\t| refname is refs/heads/side|refs/heads/side\n+\t| refname is refs/odd/spot|refs/odd/spot\n+\t|refname is refs/tags/double-tag|refs/tags/double-tag\n+\t| refname is refs/tags/four|refs/tags/four\n+\t| refname is refs/tags/one|refs/tags/one\n+\t|refname is refs/tags/signed-tag|refs/tags/signed-tag\n+\t| refname is refs/tags/three|refs/tags/three\n+\t| refname is refs/tags/two|refs/tags/two\n+\tEOF\n+\tgit for-each-ref --format=\"|%(align:30,position=right)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'alignment with \"position\" prefix' '\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:position=right,30)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'alignment with \"width\" prefix' '\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:width=30,right)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'alignment with \"width\" prefix' '\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,width=30)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'alignment with \"position\" and \"width\" prefix' '\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:width=30,position=right)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'alignment with \"position\" and \"width\" prefix' '\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:position=right,width=30)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+# Last one wins (silently) when multiple arguments of the same type are given\n+\n+test_expect_success 'alignment with multiple \"\" values' '\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:32,width=30)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'alignment with multiple \"\" values' '\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:width=30,32)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'alignment with multiple \"\" values' '\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:width=30,position=middle,right)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'alignment with multiple \"\" values' '\n+\tcat >expect <<-\\EOF &&\n+\t| refname is refs/heads/master |refs/heads/master\n+\t| refname is refs/heads/side |refs/heads/side\n+\t| refname is refs/odd/spot |refs/odd/spot\n+\t|refname is refs/tags/double-tag|refs/tags/double-tag\n+\t| refname is refs/tags/four |refs/tags/four\n+\t| refname is refs/tags/one |refs/tags/one\n+\t|refname is refs/tags/signed-tag|refs/tags/signed-tag\n+\t| refname is refs/tags/three |refs/tags/three\n+\t| refname is refs/tags/two |refs/tags/two\n+\tEOF\n+\tgit for-each-ref --format=\"|%(align:30,right,position=middle)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n # Individual atoms inside %(align:...) and %(end) must not be quoted.\n \n test_expect_success 'alignment with format quote' \"\n-- \n2.6.4\n"},{"id":"274588","messageId":"1450279802-29414-10-git-send-email-Karthik.188@gmail.com","threadId":"41023","inReplyTo":"1450279802-29414-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v2 09/11] ref-filter: introduce remote_ref_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-16T15:30:00Z","receivedAt":"2015-12-16T15:30:00Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Introduce remote_ref_atom_parser() which will parse the '%(upstream)'\nand '%(push)' atoms and store information into the 'used_atom'\nstructure based on the modifiers used along with the corresponding\natom.\n\nHelped-by: Ramsay Jones \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 105 ++++++++++++++++++++++++++++++++++++-----------------------\n 1 file changed, 64 insertions(+), 41 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 985423b..44670e3 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -37,6 +37,8 @@ static struct used_atom {\n \tunion {\n \t\tconst char *color;\n \t\tstruct align align;\n+\t\tenum { RR_SHORTEN, RR_TRACK, RR_TRACKSHORT, RR_NORMAL }\n+\t\t\tremote_ref;\n \t} u;\n } *used_atom;\n static int used_atom_cnt, need_tagged, need_symref;\n@@ -69,6 +71,25 @@ static void color_atom_parser(struct used_atom *atom)\n \t\tdie(_(\"expected format: %%(color:)\"));\n }\n \n+static void remote_ref_atom_parser(struct used_atom *atom)\n+{\n+\tconst char *buf;\n+\n+\tbuf = strchr(atom->str, ':');\n+\tatom->u.remote_ref = RR_NORMAL;\n+\tif (!buf)\n+\t\treturn;\n+\tbuf++;\n+\tif (!strcmp(buf, \"short\"))\n+\t\tatom->u.remote_ref = RR_SHORTEN;\n+\telse if (!strcmp(buf, \"track\"))\n+\t\tatom->u.remote_ref = RR_TRACK;\n+\telse if (!strcmp(buf, \"trackshort\"))\n+\t\tatom->u.remote_ref = RR_TRACKSHORT;\n+\telse\n+\t\tdie(_(\"unrecognized format: %%(%s)\"), atom->str);\n+}\n+\n static align_type parse_align_position(const char *s)\n {\n \tif (!strcmp(s, \"right\"))\n@@ -153,8 +174,8 @@ static struct {\n \t{ \"subject\" },\n \t{ \"body\" },\n \t{ \"contents\" },\n-\t{ \"upstream\" },\n-\t{ \"push\" },\n+\t{ \"upstream\", FIELD_STR, remote_ref_atom_parser },\n+\t{ \"push\", FIELD_STR, remote_ref_atom_parser },\n \t{ \"symref\" },\n \t{ \"flag\" },\n \t{ \"HEAD\" },\n@@ -835,6 +856,42 @@ static inline char *copy_advance(char *dst, const char *src)\n \treturn dst;\n }\n \n+static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n+\t\t\t\t struct branch *branch, const char **s)\n+{\n+\tint num_ours, num_theirs;\n+\tif (atom->u.remote_ref == RR_SHORTEN)\n+\t\t*s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);\n+\telse if (atom->u.remote_ref == RR_TRACK) {\n+\t\tif (stat_tracking_info(branch, &num_ours,\n+\t\t\t\t &num_theirs, NULL))\n+\t\t\treturn;\n+\t\tif (!num_ours && !num_theirs)\n+\t\t\t*s = \"\";\n+\t\telse if (!num_ours)\n+\t\t\t*s = xstrfmt(\"[behind %d]\", num_theirs);\n+\t\telse if (!num_theirs)\n+\t\t\t*s = xstrfmt(\"[ahead %d]\", num_ours);\n+\t\telse\n+\t\t\t*s = xstrfmt(\"[ahead %d, behind %d]\",\n+\t\t\t\t num_ours, num_theirs);\n+\t} else if (atom->u.remote_ref == RR_TRACKSHORT) {\n+\t\tif (stat_tracking_info(branch, &num_ours,\n+\t\t\t\t &num_theirs, NULL))\n+\t\t\treturn;\n+\n+\t\tif (!num_ours && !num_theirs)\n+\t\t\t*s = \"=\";\n+\t\telse if (!num_ours)\n+\t\t\t*s = \"<\";\n+\t\telse if (!num_theirs)\n+\t\t\t*s = \">\";\n+\t\telse\n+\t\t\t*s = \"<>\";\n+\t} else if (atom->u.remote_ref == RR_NORMAL)\n+\t\t*s = refname;\n+}\n+\n /*\n * Parse the object referred by ref, and grab needed value.\n */\n@@ -889,6 +946,8 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\trefname = branch_get_upstream(branch, NULL);\n \t\t\tif (!refname)\n \t\t\t\tcontinue;\n+\t\t\tfill_remote_ref_details(atom, refname, branch, &v->s);\n+\t\t\tcontinue;\n \t\t} else if (starts_with(name, \"push\")) {\n \t\t\tconst char *branch_name;\n \t\t\tif (!skip_prefix(ref->refname, \"refs/heads/\",\n@@ -899,6 +958,8 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\trefname = branch_get_push(branch, NULL);\n \t\t\tif (!refname)\n \t\t\t\tcontinue;\n+\t\t\tfill_remote_ref_details(atom, refname, branch, &v->s);\n+\t\t\tcontinue;\n \t\t} else if (starts_with(name, \"color:\")) {\n \t\t\tchar color[COLOR_MAXLEN] = \"\";\n \n@@ -944,49 +1005,11 @@ static void populate_value(struct ref_array_item *ref)\n \n \t\tformatp = strchr(name, ':');\n \t\tif (formatp) {\n-\t\t\tint num_ours, num_theirs;\n-\n \t\t\tformatp++;\n \t\t\tif (!strcmp(formatp, \"short\"))\n \t\t\t\trefname = shorten_unambiguous_ref(refname,\n \t\t\t\t\t\t warn_ambiguous_refs);\n-\t\t\telse if (!strcmp(formatp, \"track\") &&\n-\t\t\t\t (starts_with(name, \"upstream\") ||\n-\t\t\t\t starts_with(name, \"push\"))) {\n-\n-\t\t\t\tif (stat_tracking_info(branch, &num_ours,\n-\t\t\t\t\t\t &num_theirs, NULL))\n-\t\t\t\t\tcontinue;\n-\n-\t\t\t\tif (!num_ours && !num_theirs)\n-\t\t\t\t\tv->s = \"\";\n-\t\t\t\telse if (!num_ours)\n-\t\t\t\t\tv->s = xstrfmt(\"[behind %d]\", num_theirs);\n-\t\t\t\telse if (!num_theirs)\n-\t\t\t\t\tv->s = xstrfmt(\"[ahead %d]\", num_ours);\n-\t\t\t\telse\n-\t\t\t\t\tv->s = xstrfmt(\"[ahead %d, behind %d]\",\n-\t\t\t\t\t\t num_ours, num_theirs);\n-\t\t\t\tcontinue;\n-\t\t\t} else if (!strcmp(formatp, \"trackshort\") &&\n-\t\t\t\t (starts_with(name, \"upstream\") ||\n-\t\t\t\t starts_with(name, \"push\"))) {\n-\t\t\t\tassert(branch);\n-\n-\t\t\t\tif (stat_tracking_info(branch, &num_ours,\n-\t\t\t\t\t\t\t&num_theirs, NULL))\n-\t\t\t\t\tcontinue;\n-\n-\t\t\t\tif (!num_ours && !num_theirs)\n-\t\t\t\t\tv->s = \"=\";\n-\t\t\t\telse if (!num_ours)\n-\t\t\t\t\tv->s = \"<\";\n-\t\t\t\telse if (!num_theirs)\n-\t\t\t\t\tv->s = \">\";\n-\t\t\t\telse\n-\t\t\t\t\tv->s = \"<>\";\n-\t\t\t\tcontinue;\n-\t\t\t} else\n+\t\t\telse\n \t\t\t\tdie(\"unknown %.*s format %s\",\n \t\t\t\t (int)(formatp - name), name, formatp);\n \t\t}\n-- \n2.6.4\n"},{"id":"274589","messageId":"1450279802-29414-11-git-send-email-Karthik.188@gmail.com","threadId":"41023","inReplyTo":"1450279802-29414-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v2 10/11] ref-filter: introduce contents_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-16T15:30:01Z","receivedAt":"2015-12-16T15:30:01Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Introduce contents_atom_parser() which will parse the '%(contents)'\natom and store information into the 'used_atom' structure based on the\nmodifiers used along with the atom.\n\nHelped-by: Ramsay Jones \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 77 +++++++++++++++++++++++++++++++++++++-----------------------\n 1 file changed, 48 insertions(+), 29 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 44670e3..ce02933 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -39,6 +39,10 @@ static struct used_atom {\n \t\tstruct align align;\n \t\tenum { RR_SHORTEN, RR_TRACK, RR_TRACKSHORT, RR_NORMAL }\n \t\t\tremote_ref;\n+\t\tstruct {\n+\t\t\tenum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;\n+\t\t\tunsigned int no_lines;\n+\t\t} contents;\n \t} u;\n } *used_atom;\n static int used_atom_cnt, need_tagged, need_symref;\n@@ -90,6 +94,36 @@ static void remote_ref_atom_parser(struct used_atom *atom)\n \t\tdie(_(\"unrecognized format: %%(%s)\"), atom->str);\n }\n \n+static void contents_atom_parser(struct used_atom *atom)\n+{\n+\tconst char * buf;\n+\n+\tif (match_atom_name(atom->str, \"contents\", &buf))\n+\t\tatom->u.contents.option = C_BARE;\n+\telse if (match_atom_name(atom->str, \"subject\", &buf)) {\n+\t\tatom->u.contents.option = C_SUB;\n+\t\treturn;\n+\t} else if (match_atom_name(atom->str, \"body\", &buf)) {\n+\t\tatom->u.contents.option = C_BODY_DEP;\n+\t\treturn;\n+\t}\n+\tif (!buf)\n+\t\treturn;\n+\n+\tif (!strcmp(buf, \"body\"))\n+\t\tatom->u.contents.option = C_BODY;\n+\telse if (!strcmp(buf, \"signature\"))\n+\t\tatom->u.contents.option = C_SIG;\n+\telse if (!strcmp(buf, \"subject\"))\n+\t\tatom->u.contents.option = C_SUB;\n+\telse if (skip_prefix(buf, \"lines=\", &buf)) {\n+\t\tatom->u.contents.option = C_LINES;\n+\t\tif (strtoul_ui(buf, 10, &atom->u.contents.no_lines))\n+\t\t\tdie(_(\"positive value expected contents:lines=%s\"), buf);\n+\t} else\n+\t\tdie(_(\"unrecognized %%(contents) argument: %s\"), buf);\n+}\n+\n static align_type parse_align_position(const char *s)\n {\n \tif (!strcmp(s, \"right\"))\n@@ -171,9 +205,9 @@ static struct {\n \t{ \"taggerdate\", FIELD_TIME },\n \t{ \"creator\" },\n \t{ \"creatordate\", FIELD_TIME },\n-\t{ \"subject\" },\n-\t{ \"body\" },\n-\t{ \"contents\" },\n+\t{ \"subject\", FIELD_STR, contents_atom_parser },\n+\t{ \"body\", FIELD_STR, contents_atom_parser },\n+\t{ \"contents\", FIELD_STR, contents_atom_parser },\n \t{ \"upstream\", FIELD_STR, remote_ref_atom_parser },\n \t{ \"push\", FIELD_STR, remote_ref_atom_parser },\n \t{ \"symref\" },\n@@ -186,11 +220,6 @@ static struct {\n \n #define REF_FORMATTING_STATE_INIT { 0, NULL }\n \n-struct contents {\n-\tunsigned int lines;\n-\tstruct object_id oid;\n-};\n-\n struct ref_formatting_stack {\n \tstruct ref_formatting_stack *prev;\n \tstruct strbuf output;\n@@ -207,7 +236,6 @@ struct atom_value {\n \tconst char *s;\n \tunion {\n \t\tstruct align align;\n-\t\tstruct contents contents;\n \t} u;\n \tvoid (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);\n \tunsigned long ul; /* used for sorting when not FIELD_STR */\n@@ -756,20 +784,16 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj\n \tunsigned long sublen = 0, bodylen = 0, nonsiglen = 0, siglen = 0;\n \n \tfor (i = 0; i < used_atom_cnt; i++) {\n-\t\tconst char *name = used_atom[i].str;\n+\t\tstruct used_atom *atom = &used_atom[i];\n+\t\tconst char *name = atom->str;\n \t\tstruct atom_value *v = &val[i];\n-\t\tconst char *valp = NULL;\n \t\tif (!!deref != (*name == '*'))\n \t\t\tcontinue;\n \t\tif (deref)\n \t\t\tname++;\n \t\tif (strcmp(name, \"subject\") &&\n \t\t strcmp(name, \"body\") &&\n-\t\t strcmp(name, \"contents\") &&\n-\t\t strcmp(name, \"contents:subject\") &&\n-\t\t strcmp(name, \"contents:body\") &&\n-\t\t strcmp(name, \"contents:signature\") &&\n-\t\t !starts_with(name, \"contents:lines=\"))\n+\t\t !starts_with(name, \"contents\"))\n \t\t\tcontinue;\n \t\tif (!subpos)\n \t\t\tfind_subpos(buf, sz,\n@@ -777,28 +801,23 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj\n \t\t\t\t &bodypos, &bodylen, &nonsiglen,\n \t\t\t\t &sigpos, &siglen);\n \n-\t\tif (!strcmp(name, \"subject\"))\n+\t\tif (atom->u.contents.option == C_SUB)\n \t\t\tv->s = copy_subject(subpos, sublen);\n-\t\telse if (!strcmp(name, \"contents:subject\"))\n-\t\t\tv->s = copy_subject(subpos, sublen);\n-\t\telse if (!strcmp(name, \"body\"))\n+\t\telse if (atom->u.contents.option == C_BODY_DEP)\n \t\t\tv->s = xmemdupz(bodypos, bodylen);\n-\t\telse if (!strcmp(name, \"contents:body\"))\n+\t\telse if (atom->u.contents.option == C_BODY)\n \t\t\tv->s = xmemdupz(bodypos, nonsiglen);\n-\t\telse if (!strcmp(name, \"contents:signature\"))\n+\t\telse if (atom->u.contents.option == C_SIG)\n \t\t\tv->s = xmemdupz(sigpos, siglen);\n-\t\telse if (!strcmp(name, \"contents\"))\n-\t\t\tv->s = xstrdup(subpos);\n-\t\telse if (skip_prefix(name, \"contents:lines=\", &valp)) {\n+\t\telse if (atom->u.contents.option == C_LINES) {\n \t\t\tstruct strbuf s = STRBUF_INIT;\n \t\t\tconst char *contents_end = bodylen + bodypos - siglen;\n \n-\t\t\tif (strtoul_ui(valp, 10, &v->u.contents.lines))\n-\t\t\t\tdie(_(\"positive value expected contents:lines=%s\"), valp);\n \t\t\t/* Size is the length of the message after removing the signature */\n-\t\t\tappend_lines(&s, subpos, contents_end - subpos, v->u.contents.lines);\n+\t\t\tappend_lines(&s, subpos, contents_end - subpos, atom->u.contents.no_lines);\n \t\t\tv->s = strbuf_detach(&s, NULL);\n-\t\t}\n+\t\t} else if (atom->u.contents.option == C_BARE)\n+\t\t\tv->s = xstrdup(subpos);\n \t}\n }\n \n-- \n2.6.4\n"},{"id":"274587","messageId":"1450279802-29414-12-git-send-email-Karthik.188@gmail.com","threadId":"41023","inReplyTo":"1450279802-29414-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v2 11/11] ref-filter: introduce objectname_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-16T15:30:02Z","receivedAt":"2015-12-16T15:30:02Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Introduce objectname_atom_parser() which will parse the\n'%(objectname)' atom and store information into the 'used_atom'\nstructure based on the modifiers used along with the atom.\n\nHelped-by: Ramsay Jones \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 39 ++++++++++++++++++++++++++++-----------\n 1 file changed, 28 insertions(+), 11 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex ce02933..a8fd178 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -43,6 +43,7 @@ static struct used_atom {\n \t\t\tenum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;\n \t\t\tunsigned int no_lines;\n \t\t} contents;\n+\t\tenum { O_FULL, O_SHORT } objectname;\n \t} u;\n } *used_atom;\n static int used_atom_cnt, need_tagged, need_symref;\n@@ -124,6 +125,21 @@ static void contents_atom_parser(struct used_atom *atom)\n \t\tdie(_(\"unrecognized %%(contents) argument: %s\"), buf);\n }\n \n+static void objectname_atom_parser(struct used_atom *atom)\n+{\n+\tconst char * buf;\n+\n+\tif (match_atom_name(atom->str, \"objectname\", &buf))\n+\t\tatom->u.objectname = O_FULL;\n+\tif (!buf)\n+\t\treturn;\n+\n+\tif (!strcmp(buf, \"short\"))\n+\t\tatom->u.objectname = O_SHORT;\n+\telse\n+\t\tdie(_(\"unrecognized %%(objectname) argument: %s\"), buf);\n+}\n+\n static align_type parse_align_position(const char *s)\n {\n \tif (!strcmp(s, \"right\"))\n@@ -184,7 +200,7 @@ static struct {\n \t{ \"refname\" },\n \t{ \"objecttype\" },\n \t{ \"objectsize\", FIELD_ULONG },\n-\t{ \"objectname\" },\n+\t{ \"objectname\", FIELD_STR, objectname_atom_parser },\n \t{ \"tree\" },\n \t{ \"parent\" },\n \t{ \"numparent\", FIELD_ULONG },\n@@ -461,15 +477,16 @@ static void *get_obj(const unsigned char *sha1, struct object **obj, unsigned lo\n }\n \n static int grab_objectname(const char *name, const unsigned char *sha1,\n-\t\t\t struct atom_value *v)\n+\t\t\t struct atom_value *v, struct used_atom *atom)\n {\n-\tif (!strcmp(name, \"objectname\")) {\n-\t\tv->s = xstrdup(sha1_to_hex(sha1));\n-\t\treturn 1;\n-\t}\n-\tif (!strcmp(name, \"objectname:short\")) {\n-\t\tv->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));\n-\t\treturn 1;\n+\tif (starts_with(name, \"objectname\")) {\n+\t\tif (atom->u.objectname == O_SHORT) {\n+\t\t\tv->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));\n+\t\t\treturn 1;\n+\t\t} else if (atom->u.objectname == O_FULL) {\n+\t\t\tv->s = xstrdup(sha1_to_hex(sha1));\n+\t\t\treturn 1;\n+\t\t}\n \t}\n \treturn 0;\n }\n@@ -493,7 +510,7 @@ static void grab_common_values(struct atom_value *val, int deref, struct object\n \t\t\tv->s = xstrfmt(\"%lu\", sz);\n \t\t}\n \t\telse if (deref)\n-\t\t\tgrab_objectname(name, obj->sha1, v);\n+\t\t\tgrab_objectname(name, obj->sha1, v, &used_atom[i]);\n \t}\n }\n \n@@ -999,7 +1016,7 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\t\tv->s = xstrdup(buf + 1);\n \t\t\t}\n \t\t\tcontinue;\n-\t\t} else if (!deref && grab_objectname(name, ref->objectname, v)) {\n+\t\t} else if (!deref && grab_objectname(name, ref->objectname, v, atom)) {\n \t\t\tcontinue;\n \t\t} else if (!strcmp(name, \"HEAD\")) {\n \t\t\tconst char *head;\n-- \n2.6.4\n"},{"id":"274601","messageId":"CAPig+cQcCGVuzqYT3zXUMK=sF+QLCXUbbzcoqmaAe2zf+faWsA@mail.gmail.com","threadId":"41023","inReplyTo":"1450279802-29414-2-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v2 01/11] strbuf: introduce strbuf_split_str_without_term()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-16T20:35:48Z","receivedAt":"2015-12-16T20:35:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak wrote:\n> strbuf: introduce strbuf_split_str_without_term()\n\ns/without/omit/\n\n> The current implementation of 'strbuf_split_buf()' includes the\n> terminator at the end of each strbuf post splitting. Add an option\n> wherein we can drop the terminator if desired. In this context\n> introduce a wrapper function 'strbuf_split_str_without_term()' which\n\ns/without/omit/\n\n> splits a given string into strbufs without including the terminator.\n>\n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/strbuf.c b/strbuf.c\n> @@ -115,7 +115,7 @@ void strbuf_tolower(struct strbuf *sb)\n> struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n> - int terminator, int max)\n> + int terminator, int max, int omit_term)\n> {\n> struct strbuf **ret = NULL;\n> size_t nr = 0, alloc = 0;\n> @@ -123,14 +123,15 @@ struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n>\n> while (slen) {\n> int len = slen;\n> + const char *end = NULL;\n> if (max <= 0 || nr + 1 < max) {\n> - const char *end = memchr(str, terminator, slen);\n> + end = memchr(str, terminator, slen);\n> if (end)\n> len = end - str + 1;\n> }\n> t = xmalloc(sizeof(struct strbuf));\n> strbuf_init(t, len);\n> - strbuf_add(t, str, len);\n> + strbuf_add(t, str, len - !!end * !!omit_term);\n\nThis version of the patch with its minimal[1] change is much easier to\nreview for correctness than the original attempt[2].\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/281882\n[2]: http://article.gmane.org/gmane.comp.version-control.git/281189\n\n> ALLOC_GROW(ret, nr + 2, alloc);\n> ret[nr++] = t;\n> str += len;\n> diff --git a/strbuf.h b/strbuf.h\n> index 6ae7a72..a865a74 100644\n> --- a/strbuf.h\n> +++ b/strbuf.h\n> @@ -450,11 +450,12 @@ static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)\n> /**\n> * Split str (of length slen) at the specified terminator character.\n> * Return a null-terminated array of pointers to strbuf objects\n> - * holding the substrings. The substrings include the terminator,\n> - * except for the last substring, which might be unterminated if the\n> - * original string did not end with a terminator. If max is positive,\n> - * then split the string into at most max substrings (with the last\n> - * substring containing everything following the (max-1)th terminator\n> + * holding the substrings. The substrings include the terminator if\n> + * the value of omit_term is '0' else the terminator is excluded. The\n\nPerhaps just say \"if omit_term is false\" rather than \"if the value of ... is 0\".\n\n> + * last substring, might be unterminated if the original string did\n> + * not end with a terminator. If max is positive, then split the\n\nThis bit about the last substring is perhaps too disconnected from the\nprevious sentence. What if you re-word the entire thing something like\nthis:\n\n If omit_term is true, the terminator will be stripped from all\n substrings. Otherwise, substrings will include the terminator,\n except for the final substring, if the original string lacked a\n terminator.\n\n> + * string into at most max substrings (with the last substring\n> + * containing everything following the (max-1)th terminator\n> * character).\n> *\n> * The most generic form is `strbuf_split_buf`, which takes an arbitrary\n> @@ -465,19 +466,25 @@ static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)\n> * For lighter-weight alternatives, see string_list_split() and\n> * string_list_split_in_place().\n> */\n> -extern struct strbuf **strbuf_split_buf(const char *, size_t,\n> - int terminator, int max);\n> +extern struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n> + int terminator, int max, int omit_term);\n> +\n> +static inline struct strbuf **strbuf_split_str_omit_term(const char *str,\n> + int terminator, int max)\n> +{\n> + return strbuf_split_buf(str, strlen(str), terminator, max, 1);\n> +}\n>\n> static inline struct strbuf **strbuf_split_str(const char *str,\n> int terminator, int max)\n> {\n> - return strbuf_split_buf(str, strlen(str), terminator, max);\n> + return strbuf_split_buf(str, strlen(str), terminator, max, 0);\n> }\n>\n> static inline struct strbuf **strbuf_split_max(const struct strbuf *sb,\n> int terminator, int max)\n> {\n> - return strbuf_split_buf(sb->buf, sb->len, terminator, max);\n> + return strbuf_split_buf(sb->buf, sb->len, terminator, max, 0);\n> }\n>\n> static inline struct strbuf **strbuf_split(const struct strbuf *sb,\n> --\n> 2.6.4\n"},{"id":"274604","messageId":"CAPig+cR5=8urU+Vq_doC53+T4WCmEWJdKXyMEsbL3gWxsNpmxg@mail.gmail.com","threadId":"41023","inReplyTo":"1450279802-29414-4-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v2 03/11] ref-filter: introduce struct used_atom","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-16T20:57:51Z","receivedAt":"2015-12-16T20:57:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak wrote:\n> Introduce the 'used_array' structure which would replace the existing\n> implementation of 'used_array' (which a list of atoms). This helps us\n> parse atom's before hand and store required details into the\n> 'used_array' for future usage.\n\nAll my v1 review comments[1] about the commit message still apply to\nthis version.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/281860\n\n> Also introduce a parsing function for each atom in valid_atom. Using\n> this we can define special parsing functions for each of the atoms.\n\nThis is a conceptually distinct change which probably deserves its own\npatch. In particular, the new patch would add this field to\nvalid_atom[] *and* add the code which invokes the custom parser. (That\ncode is currently commingled with introduction of the color parser in\npatch 6/11.)\n\nMore below...\n\n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> @@ -16,9 +16,27 @@\n> +/*\n> + * An atom is a valid field atom listed below, possibly prefixed with\n> + * a \"*\" to denote deref_tag().\n> + *\n> + * We parse given format string and sort specifiers, and make a list\n> + * of properties that we need to extract out of objects. ref_array_item\n> + * structure will hold an array of values extracted that can be\n> + * indexed with the \"atom number\", which is an index into this\n> + * array.\n> + */\n> +static struct used_atom {\n> + const char *str;\n> + cmp_type type;\n> +} *used_atom;\n> +static int used_atom_cnt, need_tagged, need_symref;\n> +static int need_color_reset_at_eol;\n> +\n> static struct {\n> const char *name;\n> cmp_type cmp_type;\n> + void (*parser)(struct used_atom *atom);\n> } valid_atom[] = {\n> { \"refname\" },\n> { \"objecttype\" },\n> @@ -786,7 +788,8 @@ static void populate_value(struct ref_array_item *ref)\n>\n> /* Fill in specials first */\n> for (i = 0; i < used_atom_cnt; i++) {\n> - const char *name = used_atom[i];\n> + struct used_atom *atom = &used_atom[i];\n> + const char *name = atom->str;\n\nSame question as my previous review[1]: Why not just:\n\n const char *name = used_atom[i].str;\n\n?\n\n> struct atom_value *v = &ref->value[i];\n> int deref = 0;\n> const char *refname;\n"},{"id":"274605","messageId":"CAPig+cROdhYVW2f9ciwsMeXHNvu3BQP5bp4-no93x26dxor5cA@mail.gmail.com","threadId":"41023","inReplyTo":"1450279802-29414-6-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v2 05/11] ref-filter: skip deref specifier in match_atom_name()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-16T21:11:28Z","receivedAt":"2015-12-16T21:11:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak wrote:\n> In upcoming patches we make calls to match_atom_name() with the '*'\n> deref specifier still attached to the atom name. This causes\n> undesirable errors, hence, if present skip over the '*' deref\n> specifier in the atom name.\n\nI'd drop the second sentence since it doesn't add much or any value.\nInstead, you might want to explain that skipping '*' is done as a\nconvenience.\n\n Subsequent patches will call match_atom_name() with the '*' deref\n specifier still attached to the atom name so, as a convenience,\n skip over it on their behalf.\n\n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> @@ -37,6 +37,10 @@ static int match_atom_name(const char *name, const char *atom_name, const char *\n> {\n> const char *body;\n>\n> + /* skip the deref specifier*/\n\nToo many spaces before \"skip\".\nToo few spaces after \"specifier\".\n\n> + if (name[0] == '*')\n> + name++;\n> +\n> if (!skip_prefix(name, atom_name, &body))\n> return 0; /* doesn't even begin with \"atom_name\" */\n> if (!body[0]) {\n> --\n> 2.6.4\n"},{"id":"274606","messageId":"CAPig+cTNEXA75Gh_jXrdhLOJpXYgyZ97TJCPHXz4DodOcjWheA@mail.gmail.com","threadId":"41023","inReplyTo":"1450279802-29414-7-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v2 06/11] ref-filter: introduce color_atom_parser()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-16T21:21:00Z","receivedAt":"2015-12-16T21:21:00Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak wrote:\n> Introduce color_atom_parser() which will parse a \"color\" atom and\n> store its color in the \"use_atom\" structure for further usage in\n\nSame comment as last time: s/use_atom/used_atom/\n\n> 'populate_value()'.\n\ns/'//g\n\nMore below...\n\n> Helped-by: Ramsay Jones \n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> @@ -29,6 +29,9 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n> static struct used_atom {\n> const char *str;\n> cmp_type type;\n> + union {\n> + const char *color;\n> + } u;\n> } *used_atom;\n> static int used_atom_cnt, need_tagged, need_symref;\n> static int need_color_reset_at_eol;\n> @@ -53,6 +56,13 @@ static int match_atom_name(const char *name, const char *atom_name, const char *\n> +static void color_atom_parser(struct used_atom *atom)\n> +{\n> + match_atom_name(atom->str, \"color\", &atom->u.color);\n> + if (!atom->u.color)\n> + die(_(\"expected format: %%(color:)\"));\n> +}\n> +\n> @@ -833,12 +846,10 @@ static void populate_value(struct ref_array_item *ref)\n> refname = branch_get_push(branch, NULL);\n> if (!refname)\n> continue;\n> - } else if (match_atom_name(name, \"color\", &valp)) {\n> + } else if (starts_with(name, \"color:\")) {\n> char color[COLOR_MAXLEN] = \"\";\n>\n> - if (!valp)\n> - die(_(\"expected format: %%(color:)\"));\n> - if (color_parse(valp, color) < 0)\n> + if (color_parse(atom->u.color, color) < 0)\n\nIt would make a lot more sense to invoke color_parse() with the\nunchanging argument to \"color:\" just once in color_atom_parser()\nrather than doing it here repeatedly. (Also, you'd drop 'const' from\nused_atom.u.color declaration.)\n\n> die(_(\"unable to parse format\"));\n> v->s = xstrdup(color);\n\nDoes v->s get freed each time through the loop? If not, then, assuming\nyou parse the color just once in color_atom_parser(), then you could\njust assign the parsed color directly to v->s rather than duplicating\nit.\n\n> continue;\n> --\n> 2.6.4\n"},{"id":"274611","messageId":"CAPig+cR=dYnMfaHUYcq_kz8M+C1nCxOX=SN=_6+ep1aWvVCUow@mail.gmail.com","threadId":"41023","inReplyTo":"1450279802-29414-8-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v2 07/11] ref-filter: introduce align_atom_parser()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-16T21:54:41Z","receivedAt":"2015-12-16T21:54:41Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak wrote:\n> Introduce align_atom_parser() which will parse an \"align\" atom and\n> store the required alignment position and width in the \"use_atom\"\n> structure for further usage in 'populate_value()'.\n>\n> Helped-by: Ramsay Jones \n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> @@ -63,6 +69,49 @@ static void color_atom_parser(struct used_atom *atom)\n> +static align_type parse_align_position(const char *s)\n> +{\n> + if (!strcmp(s, \"right\"))\n> + return ALIGN_RIGHT;\n> + else if (!strcmp(s, \"middle\"))\n> + return ALIGN_MIDDLE;\n> + else if (!strcmp(s, \"left\"))\n> + return ALIGN_LEFT;\n> + return -1;\n> +}\n> +\n> +static void align_atom_parser(struct used_atom *atom)\n> +{\n> + struct align *align = &atom->u.align;\n> + const char *buf = NULL;\n> + struct strbuf **s, **to_free;\n> + int width = -1;\n> +\n> + match_atom_name(atom->str, \"align\", &buf);\n> + if (!buf)\n> + die(_(\"expected format: %%(align:,)\"));\n> + s = to_free = strbuf_split_str_omit_term(buf, ',', 0);\n> +\n> + align->position = ALIGN_LEFT;\n> +\n> + while (*s) {\n> + int position;\n> +\n> + if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))\n\nThis casting is pretty ugly. It probably would be cleaner to declare\n'width' as 'unsigned int' and initialize it with ~0U than to do this\nugly and potentially dangerous casting. Likewise, below where you\ncheck 'width < 0', you'd check instead for ~0U. However, such a change\nshould not be part of the current patch, but rather as a separate\npreparatory patch.\n\n> + ;\n> + else if ((position = parse_align_position(s[0]->buf)) > 0)\n\nShouldn't this be '>=' rather than '>'?\n\nThis likely would have been easier to spot if parse_align_position()\nhad been factored out in its own patch, as suggested by my v1\nreview[1], since the caller would have been trivially inspectable\nrather than having to keep track of both code movement and changes.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/281916\n\n> + align->position = position;\n> + else\n> + die(_(\"unrecognized %%(align) argument: %s\"), s[0]->buf);\n> + s++;\n> + }\n> +\n> + if (width < 0)\n> + die(_(\"positive width expected with the %%(align) atom\"));\n> + align->width = width;\n> + strbuf_list_free(to_free);\n> +}\n> +\n> static struct {\n> const char *name;\n> cmp_type cmp_type;\n> @@ -880,35 +924,7 @@ static void populate_value(struct ref_array_item *ref)\n> v->s = \" \";\n> continue;\n> } else if (match_atom_name(name, \"align\", &valp)) {\n> - struct align *align = &v->u.align;\n> - struct strbuf **s, **to_free;\n> - int width = -1;\n> -\n> - if (!valp)\n> - die(_(\"expected format: %%(align:,)\"));\n> -\n> - s = to_free = strbuf_split_str_omit_term(valp, ',', 0);\n> -\n> - align->position = ALIGN_LEFT;\n> -\n> - while (*s) {\n> - if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))\n> - ;\n> - else if (!strcmp(s[0]->buf, \"left\"))\n> - align->position = ALIGN_LEFT;\n> - else if (!strcmp(s[0]->buf, \"right\"))\n> - align->position = ALIGN_RIGHT;\n> - else if (!strcmp(s[0]->buf, \"middle\"))\n> - align->position = ALIGN_MIDDLE;\n> - else\n> - die(_(\"improper format entered align:%s\"), s[0]->buf);\n> - s++;\n> - }\n> -\n> - if (width < 0)\n> - die(_(\"positive width expected with the %%(align) atom\"));\n> - align->width = width;\n> - strbuf_list_free(to_free);\n> + v->u.align = atom->u.align;\n> v->handler = align_atom_handler;\n> continue;\n> } else if (!strcmp(name, \"end\")) {\n> --\n> 2.6.4\n"},{"id":"274656","messageId":"CAOLa=ZTDjo=eXWes1Ryzpj81gBoO7Lto3jEMGaeFioBfK_YOxA@mail.gmail.com","threadId":"41023","inReplyTo":"CAPig+cQcCGVuzqYT3zXUMK=sF+QLCXUbbzcoqmaAe2zf+faWsA@mail.gmail.com","subject":"Re: [PATCH v2 01/11] strbuf: introduce strbuf_split_str_without_term()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-17T08:24:04Z","receivedAt":"2015-12-17T08:24:04Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Thu, Dec 17, 2015 at 2:05 AM, Eric Sunshine wrote:\n> On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak wrote:\n>> strbuf: introduce strbuf_split_str_without_term()\n>\n> s/without/omit/\n>\n>> The current implementation of 'strbuf_split_buf()' includes the\n>> terminator at the end of each strbuf post splitting. Add an option\n>> wherein we can drop the terminator if desired. In this context\n>> introduce a wrapper function 'strbuf_split_str_without_term()' which\n>\n> s/without/omit/\n>\n\nOops! will change that,\n\n>> splits a given string into strbufs without including the terminator.\n>>\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/strbuf.c b/strbuf.c\n>> @@ -115,7 +115,7 @@ void strbuf_tolower(struct strbuf *sb)\n>> struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n>> - int terminator, int max)\n>> + int terminator, int max, int omit_term)\n>> {\n>> struct strbuf **ret = NULL;\n>> size_t nr = 0, alloc = 0;\n>> @@ -123,14 +123,15 @@ struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n>>\n>> while (slen) {\n>> int len = slen;\n>> + const char *end = NULL;\n>> if (max <= 0 || nr + 1 < max) {\n>> - const char *end = memchr(str, terminator, slen);\n>> + end = memchr(str, terminator, slen);\n>> if (end)\n>> len = end - str + 1;\n>> }\n>> t = xmalloc(sizeof(struct strbuf));\n>> strbuf_init(t, len);\n>> - strbuf_add(t, str, len);\n>> + strbuf_add(t, str, len - !!end * !!omit_term);\n>\n> This version of the patch with its minimal[1] change is much easier to\n> review for correctness than the original attempt[2].\n>\n> [1]: http://article.gmane.org/gmane.comp.version-control.git/281882\n> [2]: http://article.gmane.org/gmane.comp.version-control.git/281189\n>\n>> ALLOC_GROW(ret, nr + 2, alloc);\n>> ret[nr++] = t;\n>> str += len;\n>> diff --git a/strbuf.h b/strbuf.h\n>> index 6ae7a72..a865a74 100644\n>> --- a/strbuf.h\n>> +++ b/strbuf.h\n>> @@ -450,11 +450,12 @@ static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)\n>> /**\n>> * Split str (of length slen) at the specified terminator character.\n>> * Return a null-terminated array of pointers to strbuf objects\n>> - * holding the substrings. The substrings include the terminator,\n>> - * except for the last substring, which might be unterminated if the\n>> - * original string did not end with a terminator. If max is positive,\n>> - * then split the string into at most max substrings (with the last\n>> - * substring containing everything following the (max-1)th terminator\n>> + * holding the substrings. The substrings include the terminator if\n>> + * the value of omit_term is '0' else the terminator is excluded. The\n>\n> Perhaps just say \"if omit_term is false\" rather than \"if the value of ... is 0\".\n>\n>> + * last substring, might be unterminated if the original string did\n>> + * not end with a terminator. If max is positive, then split the\n>\n> This bit about the last substring is perhaps too disconnected from the\n> previous sentence. What if you re-word the entire thing something like\n> this:\n>\n> If omit_term is true, the terminator will be stripped from all\n> substrings. Otherwise, substrings will include the terminator,\n> except for the final substring, if the original string lacked a\n> terminator.\n>\n\nIm quite bad at this, thanks :)\n\n>>\n>> This version of the patch with its minimal[1] change is much easier to\n>> review for correctness than the original attempt[2].\n>\n> Perhaps worthy of a Helped-by:?\n\nDefinitely! Sorry for not including that, slipped off my mind.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"274657","messageId":"CAPig+cRmoOoSscuLcVHWkpH4uVU2Gei791qqn0GjyZ4b9fA45w@mail.gmail.com","threadId":"41023","inReplyTo":"1450279802-29414-9-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v2 08/11] ref-filter: introduce prefixes for the align atom","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-17T08:59:24Z","receivedAt":"2015-12-17T08:59:24Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak wrote:\n> ref-filter: introduce prefixes for the align atom\n\nThe prefixes are actually for the arguments to the 'align' atom, not\nfor the atom itself. However, it might be better to describe this at a\nbit higher level. Perhaps:\n\n ref-filter: align: introduce long-form syntax\n\nor something.\n\n> Introduce optional prefixes \"width=\" and \"position=\" for the align atom\n> so that the atom can be used as \"%(align:width=,position=)\".\n>\n> Add Documetation and tests for the same.\n>\n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> @@ -96,10 +96,19 @@ static void align_atom_parser(struct used_atom *atom)\n>\n> while (*s) {\n> int position;\n> + buf = s[0]->buf;\n\nIt probably would be better to do this assignment in the previous\npatch (7/11) since its presence here introduces unwanted noise\n(textual replacement of \"s[0]->buf\" with \"buf\") in several locations\nbelow which slightly obscure the real changes of this patch.\n\n> - if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))\n> + if (skip_prefix(buf, \"position=\", &buf)) {\n> + position = parse_align_position(buf);\n> + if (position == -1)\n\nIt may be more idiomatic in this codebase to detect errors via '< 0'\nrather than explicit '== -1'. Ditto below.\n\n> + die(_(\"unrecognized position:%s\"), buf);\n> + align->position = position;\n> + } else if (skip_prefix(buf, \"width=\", &buf)) {\n> + if (strtoul_ui(buf, 10, (unsigned int *)&width))\n> + die(_(\"unrecognized width:%s\"), buf);\n> + } else if (!strtoul_ui(buf, 10, (unsigned int *)&width))\n> ;\n> - else if ((position = parse_align_position(s[0]->buf)) > 0)\n> + else if ((position = parse_align_position(buf)) != -1)\n> align->position = position;\n> else\n> die(_(\"unrecognized %%(align) argument: %s\"), s[0]->buf);\n\ns/s[0]->//\n\nMore below...\n\n> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\n> @@ -133,6 +133,168 @@ test_expect_success 'right alignment' '\n> test_cmp expect actual\n> '\n>\n> +test_expect_success 'alignment with \"position\" prefix' '\n> + cat >expect <<-\\EOF &&\n> + | refname is refs/heads/master|refs/heads/master\n> + | refname is refs/heads/side|refs/heads/side\n> + | refname is refs/odd/spot|refs/odd/spot\n> + |refname is refs/tags/double-tag|refs/tags/double-tag\n> + | refname is refs/tags/four|refs/tags/four\n> + | refname is refs/tags/one|refs/tags/one\n> + |refname is refs/tags/signed-tag|refs/tags/signed-tag\n> + | refname is refs/tags/three|refs/tags/three\n> + | refname is refs/tags/two|refs/tags/two\n> + EOF\n> + git for-each-ref --format=\"|%(align:30,position=right)refname is %(refname)%(end)|%(refname)\" >actual &&\n> + test_cmp expect actual\n> +'\n> +\n> +test_expect_success 'alignment with \"position\" prefix' '\n> + cat >expect <<-\\EOF &&\n> + | refname is refs/heads/master|refs/heads/master\n> + | refname is refs/heads/side|refs/heads/side\n> + | refname is refs/odd/spot|refs/odd/spot\n> + |refname is refs/tags/double-tag|refs/tags/double-tag\n> + | refname is refs/tags/four|refs/tags/four\n> + | refname is refs/tags/one|refs/tags/one\n> + |refname is refs/tags/signed-tag|refs/tags/signed-tag\n> + | refname is refs/tags/three|refs/tags/three\n> + | refname is refs/tags/two|refs/tags/two\n> + EOF\n> + git for-each-ref --format=\"|%(align:position=right,30)refname is %(refname)%(end)|%(refname)\" >actual &&\n> + test_cmp expect actual\n> +'\n\nThis (and below) is a lot of copy/paste code which makes it difficult\nto read the tests and maintain (change) them. Since 'expect' doesn't\nappear to change from test to test, one way to eliminate some of this\nnoisiness would be to create 'expect' once outside of the tests.\n\nHowever, even better, especially from a comprehension,\nmaintainability, and extensibility standpoints would be to make this\nall table-driven. In particular, I'd expect to see a table with\nexactly the list of test inputs mentioned in my earlier review[1], and\nhave that table passed to a shell function which performs the test for\neach element of the table. For instance, something like:\n\n test_align_permutations <<-\\EOF\n middle,42\n 42,middle\n position=middle,42\n 42,position=middle\n middle,width=42\n width=42,middle\n position=middle,width=42\n width=42,position=middle\n EOF\n\nwhere test_align_permutations is the name of the shell function which\nreads each line of it stdin and performs the \"git for-each-ref\n--format=...\" test with the given %(align:) arguments.\n\nDitto regarding the below \"last one wins (silently) tests\".\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/281916\n\n> +\n> +test_expect_success 'alignment with \"width\" prefix' '\n> + cat >expect <<-\\EOF &&\n> + | refname is refs/heads/master|refs/heads/master\n> + | refname is refs/heads/side|refs/heads/side\n> + | refname is refs/odd/spot|refs/odd/spot\n> + |refname is refs/tags/double-tag|refs/tags/double-tag\n> + | refname is refs/tags/four|refs/tags/four\n> + | refname is refs/tags/one|refs/tags/one\n> + |refname is refs/tags/signed-tag|refs/tags/signed-tag\n> + | refname is refs/tags/three|refs/tags/three\n> + | refname is refs/tags/two|refs/tags/two\n> + EOF\n> + git for-each-ref --format=\"|%(align:width=30,right)refname is %(refname)%(end)|%(refname)\" >actual &&\n> + test_cmp expect actual\n> +'\n> +\n> +test_expect_success 'alignment with \"width\" prefix' '\n> + cat >expect <<-\\EOF &&\n> + | refname is refs/heads/master|refs/heads/master\n> + | refname is refs/heads/side|refs/heads/side\n> + | refname is refs/odd/spot|refs/odd/spot\n> + |refname is refs/tags/double-tag|refs/tags/double-tag\n> + | refname is refs/tags/four|refs/tags/four\n> + | refname is refs/tags/one|refs/tags/one\n> + |refname is refs/tags/signed-tag|refs/tags/signed-tag\n> + | refname is refs/tags/three|refs/tags/three\n> + | refname is refs/tags/two|refs/tags/two\n> + EOF\n> + git for-each-ref --format=\"|%(align:right,width=30)refname is %(refname)%(end)|%(refname)\" >actual &&\n> + test_cmp expect actual\n> +'\n> +\n> +test_expect_success 'alignment with \"position\" and \"width\" prefix' '\n> + cat >expect <<-\\EOF &&\n> + | refname is refs/heads/master|refs/heads/master\n> + | refname is refs/heads/side|refs/heads/side\n> + | refname is refs/odd/spot|refs/odd/spot\n> + |refname is refs/tags/double-tag|refs/tags/double-tag\n> + | refname is refs/tags/four|refs/tags/four\n> + | refname is refs/tags/one|refs/tags/one\n> + |refname is refs/tags/signed-tag|refs/tags/signed-tag\n> + | refname is refs/tags/three|refs/tags/three\n> + | refname is refs/tags/two|refs/tags/two\n> + EOF\n> + git for-each-ref --format=\"|%(align:width=30,position=right)refname is %(refname)%(end)|%(refname)\" >actual &&\n> + test_cmp expect actual\n> +'\n> +\n> +test_expect_success 'alignment with \"position\" and \"width\" prefix' '\n> + cat >expect <<-\\EOF &&\n> + | refname is refs/heads/master|refs/heads/master\n> + | refname is refs/heads/side|refs/heads/side\n> + | refname is refs/odd/spot|refs/odd/spot\n> + |refname is refs/tags/double-tag|refs/tags/double-tag\n> + | refname is refs/tags/four|refs/tags/four\n> + | refname is refs/tags/one|refs/tags/one\n> + |refname is refs/tags/signed-tag|refs/tags/signed-tag\n> + | refname is refs/tags/three|refs/tags/three\n> + | refname is refs/tags/two|refs/tags/two\n> + EOF\n> + git for-each-ref --format=\"|%(align:position=right,width=30)refname is %(refname)%(end)|%(refname)\" >actual &&\n> + test_cmp expect actual\n> +'\n> +\n> +# Last one wins (silently) when multiple arguments of the same type are given\n> +\n> +test_expect_success 'alignment with multiple \"\" values' '\n> + cat >expect <<-\\EOF &&\n> + |refname is refs/heads/master |refs/heads/master\n> + |refname is refs/heads/side |refs/heads/side\n> + |refname is refs/odd/spot |refs/odd/spot\n> + |refname is refs/tags/double-tag|refs/tags/double-tag\n> + |refname is refs/tags/four |refs/tags/four\n> + |refname is refs/tags/one |refs/tags/one\n> + |refname is refs/tags/signed-tag|refs/tags/signed-tag\n> + |refname is refs/tags/three |refs/tags/three\n> + |refname is refs/tags/two |refs/tags/two\n> + EOF\n> + git for-each-ref --format=\"|%(align:32,width=30)refname is %(refname)%(end)|%(refname)\" >actual &&\n> + test_cmp expect actual\n> +'\n> +\n> +test_expect_success 'alignment with multiple \"\" values' '\n> + cat >expect <<-\\EOF &&\n> + |refname is refs/heads/master |refs/heads/master\n> + |refname is refs/heads/side |refs/heads/side\n> + |refname is refs/odd/spot |refs/odd/spot\n> + |refname is refs/tags/double-tag |refs/tags/double-tag\n> + |refname is refs/tags/four |refs/tags/four\n> + |refname is refs/tags/one |refs/tags/one\n> + |refname is refs/tags/signed-tag |refs/tags/signed-tag\n> + |refname is refs/tags/three |refs/tags/three\n> + |refname is refs/tags/two |refs/tags/two\n> + EOF\n> + git for-each-ref --format=\"|%(align:width=30,32)refname is %(refname)%(end)|%(refname)\" >actual &&\n> + test_cmp expect actual\n> +'\n> +\n> +test_expect_success 'alignment with multiple \"\" values' '\n> + cat >expect <<-\\EOF &&\n> + | refname is refs/heads/master|refs/heads/master\n> + | refname is refs/heads/side|refs/heads/side\n> + | refname is refs/odd/spot|refs/odd/spot\n> + |refname is refs/tags/double-tag|refs/tags/double-tag\n> + | refname is refs/tags/four|refs/tags/four\n> + | refname is refs/tags/one|refs/tags/one\n> + |refname is refs/tags/signed-tag|refs/tags/signed-tag\n> + | refname is refs/tags/three|refs/tags/three\n> + | refname is refs/tags/two|refs/tags/two\n> + EOF\n> + git for-each-ref --format=\"|%(align:width=30,position=middle,right)refname is %(refname)%(end)|%(refname)\" >actual &&\n> + test_cmp expect actual\n> +'\n> +\n> +test_expect_success 'alignment with multiple \"\" values' '\n> + cat >expect <<-\\EOF &&\n> + | refname is refs/heads/master |refs/heads/master\n> + | refname is refs/heads/side |refs/heads/side\n> + | refname is refs/odd/spot |refs/odd/spot\n> + |refname is refs/tags/double-tag|refs/tags/double-tag\n> + | refname is refs/tags/four |refs/tags/four\n> + | refname is refs/tags/one |refs/tags/one\n> + |refname is refs/tags/signed-tag|refs/tags/signed-tag\n> + | refname is refs/tags/three |refs/tags/three\n> + | refname is refs/tags/two |refs/tags/two\n> + EOF\n> + git for-each-ref --format=\"|%(align:30,right,position=middle)refname is %(refname)%(end)|%(refname)\" >actual &&\n> + test_cmp expect actual\n> +'\n> +\n> # Individual atoms inside %(align:...) and %(end) must not be quoted.\n>\n> test_expect_success 'alignment with format quote' \"\n> --\n> 2.6.4\n>\n"},{"id":"274658","messageId":"CAPig+cRG_HTWL=VNqOxDnv_M0NbYZSReCHSsGe1CJiFQS8h-XQ@mail.gmail.com","threadId":"41023","inReplyTo":"1450279802-29414-10-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v2 09/11] ref-filter: introduce remote_ref_atom_parser()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-17T09:22:09Z","receivedAt":"2015-12-17T09:22:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 16, 2015 at 10:30 AM, Karthik Nayak wrote:\n> Introduce remote_ref_atom_parser() which will parse the '%(upstream)'\n> and '%(push)' atoms and store information into the 'used_atom'\n> structure based on the modifiers used along with the corresponding\n> atom.\n>\n> Helped-by: Ramsay Jones \n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> @@ -37,6 +37,8 @@ static struct used_atom {\n> union {\n> const char *color;\n> struct align align;\n> + enum { RR_SHORTEN, RR_TRACK, RR_TRACKSHORT, RR_NORMAL }\n\nNit: I'd have expected to see the normal/plain case first rather than\nlast (but not itself worth a re-roll).\n\n> + remote_ref;\n> } u;\n> } *used_atom;\n> static int used_atom_cnt, need_tagged, need_symref;\n> @@ -69,6 +71,25 @@ static void color_atom_parser(struct used_atom *atom)\n> +static void remote_ref_atom_parser(struct used_atom *atom)\n> +{\n> + const char *buf;\n> +\n> + buf = strchr(atom->str, ':');\n> + atom->u.remote_ref = RR_NORMAL;\n> + if (!buf)\n> + return;\n\nThis code is not as clear as it could be due to the way the 'buf'\nassignment and check for NULL are split apart. It can be made clearer\neither by doing this:\n\n atom->u.remote_ref = RR_NORMAL;\n buf = strchr(...);\n if (!buf)\n return;\n\nor (even better) this:\n\n buf = strchr(...);\n if (!buf) {\n atom->u.remote_ref = RR_NORMAL;\n return;\n }\n\n> + buf++;\n> + if (!strcmp(buf, \"short\"))\n> + atom->u.remote_ref = RR_SHORTEN;\n> + else if (!strcmp(buf, \"track\"))\n> + atom->u.remote_ref = RR_TRACK;\n> + else if (!strcmp(buf, \"trackshort\"))\n> + atom->u.remote_ref = RR_TRACKSHORT;\n> + else\n> + die(_(\"unrecognized format: %%(%s)\"), atom->str);\n> +}\n> +\n> @@ -835,6 +856,42 @@ static inline char *copy_advance(char *dst, const char *src)\n> +static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n> + struct branch *branch, const char **s)\n> +{\n> + int num_ours, num_theirs;\n> + if (atom->u.remote_ref == RR_SHORTEN)\n> + *s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);\n> + else if (atom->u.remote_ref == RR_TRACK) {\n> + if (stat_tracking_info(branch, &num_ours,\n> + &num_theirs, NULL))\n> + return;\n\nThe RR_TRACKSHORT branch below has a blank line following the\n'return', but this branch lacks it, which is inconsistent.\n\n> + if (!num_ours && !num_theirs)\n> + *s = \"\";\n> + else if (!num_ours)\n> + *s = xstrfmt(\"[behind %d]\", num_theirs);\n> + else if (!num_theirs)\n> + *s = xstrfmt(\"[ahead %d]\", num_ours);\n> + else\n> + *s = xstrfmt(\"[ahead %d, behind %d]\",\n> + num_ours, num_theirs);\n> + } else if (atom->u.remote_ref == RR_TRACKSHORT) {\n> + if (stat_tracking_info(branch, &num_ours,\n> + &num_theirs, NULL))\n\nWhat happened to the assert(branch) which was in the original code\nfrom which this was derived (below)? Is it no longer needed?\n\n> + return;\n> +\n> + if (!num_ours && !num_theirs)\n> + *s = \"=\";\n> + else if (!num_ours)\n> + *s = \"<\";\n> + else if (!num_theirs)\n> + *s = \">\";\n> + else\n> + *s = \"<>\";\n> + } else if (atom->u.remote_ref == RR_NORMAL)\n> + *s = refname;\n> +}\n> +\n> /*\n> * Parse the object referred by ref, and grab needed value.\n> */\n> @@ -889,6 +946,8 @@ static void populate_value(struct ref_array_item *ref)\n> refname = branch_get_upstream(branch, NULL);\n> if (!refname)\n> continue;\n> + fill_remote_ref_details(atom, refname, branch, &v->s);\n> + continue;\n> } else if (starts_with(name, \"push\")) {\n> const char *branch_name;\n> if (!skip_prefix(ref->refname, \"refs/heads/\",\n> @@ -899,6 +958,8 @@ static void populate_value(struct ref_array_item *ref)\n> refname = branch_get_push(branch, NULL);\n> if (!refname)\n> continue;\n> + fill_remote_ref_details(atom, refname, branch, &v->s);\n> + continue;\n> } else if (starts_with(name, \"color:\")) {\n> char color[COLOR_MAXLEN] = \"\";\n>\n> @@ -944,49 +1005,11 @@ static void populate_value(struct ref_array_item *ref)\n>\n> formatp = strchr(name, ':');\n> if (formatp) {\n> - int num_ours, num_theirs;\n> -\n> formatp++;\n> if (!strcmp(formatp, \"short\"))\n> refname = shorten_unambiguous_ref(refname,\n> warn_ambiguous_refs);\n> - else if (!strcmp(formatp, \"track\") &&\n> - (starts_with(name, \"upstream\") ||\n> - starts_with(name, \"push\"))) {\n> -\n> - if (stat_tracking_info(branch, &num_ours,\n> - &num_theirs, NULL))\n> - continue;\n> -\n> - if (!num_ours && !num_theirs)\n> - v->s = \"\";\n> - else if (!num_ours)\n> - v->s = xstrfmt(\"[behind %d]\", num_theirs);\n> - else if (!num_theirs)\n> - v->s = xstrfmt(\"[ahead %d]\", num_ours);\n> - else\n> - v->s = xstrfmt(\"[ahead %d, behind %d]\",\n> - num_ours, num_theirs);\n> - continue;\n> - } else if (!strcmp(formatp, \"trackshort\") &&\n> - (starts_with(name, \"upstream\") ||\n> - starts_with(name, \"push\"))) {\n> - assert(branch);\n> -\n> - if (stat_tracking_info(branch, &num_ours,\n> - &num_theirs, NULL))\n> - continue;\n> -\n> - if (!num_ours && !num_theirs)\n> - v->s = \"=\";\n> - else if (!num_ours)\n> - v->s = \"<\";\n> - else if (!num_theirs)\n> - v->s = \">\";\n> - else\n> - v->s = \"<>\";\n> - continue;\n> - } else\n> + else\n> die(\"unknown %.*s format %s\",\n> (int)(formatp - name), name, formatp);\n> }\n> --\n> 2.6.4\n"},{"id":"274660","messageId":"CAPig+cRmUe4SRzmiV13XWSYou8M7TuDE3Ko37g8p-q2kEQNU+w@mail.gmail.com","threadId":"41023","inReplyTo":"1450279802-29414-11-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v2 10/11] ref-filter: introduce contents_atom_parser()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-17T09:39:02Z","receivedAt":"2015-12-17T09:39:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 16, 2015 at 10:30 AM, Karthik Nayak wrote:\n> Introduce contents_atom_parser() which will parse the '%(contents)'\n> atom and store information into the 'used_atom' structure based on the\n> modifiers used along with the atom.\n>\n> Helped-by: Ramsay Jones \n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> @@ -39,6 +39,10 @@ static struct used_atom {\n> struct align align;\n> enum { RR_SHORTEN, RR_TRACK, RR_TRACKSHORT, RR_NORMAL }\n> remote_ref;\n> + struct {\n> + enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;\n> + unsigned int no_lines;\n\n'no_lines' sounds like \"no lines!\". How about 'nlines' instead?\n\n> + } contents;\n> } u;\n> } *used_atom;\n> static int used_atom_cnt, need_tagged, need_symref;\n> @@ -90,6 +94,36 @@ static void remote_ref_atom_parser(struct used_atom *atom)\n> +static void contents_atom_parser(struct used_atom *atom)\n> +{\n> + const char * buf;\n> +\n> + if (match_atom_name(atom->str, \"contents\", &buf))\n> + atom->u.contents.option = C_BARE;\n> + else if (match_atom_name(atom->str, \"subject\", &buf)) {\n\nThe original code used strcmp() and matched only \"subject\", however\nthe new code will incorrectly match both \"subject\" and\n\"subject:whatever\". Therefore, you should be using strcmp() here\nrather than match_atom_name().\n\nDitto for \"body\".\n\n> + atom->u.contents.option = C_SUB;\n> + return;\n> + } else if (match_atom_name(atom->str, \"body\", &buf)) {\n> + atom->u.contents.option = C_BODY_DEP;\n> + return;\n> + }\n> + if (!buf)\n> + return;\n\nIt's not easy to see that this 'if (!buf)' check relates to the\n\"contents\" check at the very top of the if/else if/ chain since there\nare entirely unrelated checks in between. Reorganizing it can improve\nclarity:\n\n if (!strcmp(\"subject\")) {\n ...\n return;\n } else if (!strcmp(\"body\")) {\n ...\n return;\n } else if (!match_atom_name(...,\"contents\", &buf))\n die(\"BUG: expected 'contents' or 'contents:'\");\n\n if (!buf) {\n atom->u.contents.option = C_BARE;\n return;\n }\n\n> + if (!strcmp(buf, \"body\"))\n> + atom->u.contents.option = C_BODY;\n> + else if (!strcmp(buf, \"signature\"))\n> + atom->u.contents.option = C_SIG;\n> + else if (!strcmp(buf, \"subject\"))\n> + atom->u.contents.option = C_SUB;\n> + else if (skip_prefix(buf, \"lines=\", &buf)) {\n> + atom->u.contents.option = C_LINES;\n> + if (strtoul_ui(buf, 10, &atom->u.contents.no_lines))\n> + die(_(\"positive value expected contents:lines=%s\"), buf);\n> + } else\n> + die(_(\"unrecognized %%(contents) argument: %s\"), buf);\n> +}\n> @@ -777,28 +801,23 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj\n> &bodypos, &bodylen, &nonsiglen,\n> &sigpos, &siglen);\n>\n> - if (!strcmp(name, \"subject\"))\n> + if (atom->u.contents.option == C_SUB)\n> v->s = copy_subject(subpos, sublen);\n> - else if (!strcmp(name, \"contents:subject\"))\n> - v->s = copy_subject(subpos, sublen);\n> - else if (!strcmp(name, \"body\"))\n> + else if (atom->u.contents.option == C_BODY_DEP)\n> v->s = xmemdupz(bodypos, bodylen);\n> - else if (!strcmp(name, \"contents:body\"))\n> + else if (atom->u.contents.option == C_BODY)\n> v->s = xmemdupz(bodypos, nonsiglen);\n> - else if (!strcmp(name, \"contents:signature\"))\n> + else if (atom->u.contents.option == C_SIG)\n> v->s = xmemdupz(sigpos, siglen);\n> - else if (!strcmp(name, \"contents\"))\n> - v->s = xstrdup(subpos);\n> - else if (skip_prefix(name, \"contents:lines=\", &valp)) {\n> + else if (atom->u.contents.option == C_LINES) {\n> struct strbuf s = STRBUF_INIT;\n> const char *contents_end = bodylen + bodypos - siglen;\n>\n> - if (strtoul_ui(valp, 10, &v->u.contents.lines))\n> - die(_(\"positive value expected contents:lines=%s\"), valp);\n> /* Size is the length of the message after removing the signature */\n> - append_lines(&s, subpos, contents_end - subpos, v->u.contents.lines);\n> + append_lines(&s, subpos, contents_end - subpos, atom->u.contents.no_lines);\n> v->s = strbuf_detach(&s, NULL);\n> - }\n> + } else if (atom->u.contents.option == C_BARE)\n> + v->s = xstrdup(subpos);\n> }\n> }\n>\n> --\n> 2.6.4\n"},{"id":"274715","messageId":"CAPig+cTvKL9C0M3NKF395dq6yCP94AL+iDe0EGRaK9xYDZ0YuQ@mail.gmail.com","threadId":"41023","inReplyTo":"1450279802-29414-8-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v2 07/11] ref-filter: introduce align_atom_parser()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-18T05:50:30Z","receivedAt":"2015-12-18T05:50:30Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak wrote:\n> Introduce align_atom_parser() which will parse an \"align\" atom and\n> store the required alignment position and width in the \"use_atom\"\n> structure for further usage in 'populate_value()'.\n>\n> Helped-by: Ramsay Jones \n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> @@ -63,6 +69,49 @@ static void color_atom_parser(struct used_atom *atom)\n> +static void align_atom_parser(struct used_atom *atom)\n> +{\n> + [...]\n> + match_atom_name(atom->str, \"align\", &buf);\n> + if (!buf)\n> + die(_(\"expected format: %%(align:,)\"));\n> + [...]\n> +}\n> @@ -880,35 +924,7 @@ static void populate_value(struct ref_array_item *ref)\n> v->s = \" \";\n> continue;\n> } else if (match_atom_name(name, \"align\", &valp)) {\n\nHmm, align_atom_parser() has already been called before we get here,\nright? And it has already invoked match_atom_name() and died if the\nargument to \"align:\" was missing, correct? If so, then this invocation\nof match_atom_name() in populate_value() is unnecessary and should be\nreplaced with a simpler starts_with(\"align:\").\n\nPlus, 'valp' is not used, aside from this unnecessary\nmatch_atom_name(), thus it can be removed as well.\n\n> - struct align *align = &v->u.align;\n> - struct strbuf **s, **to_free;\n> - int width = -1;\n> -\n> - if (!valp)\n> - die(_(\"expected format: %%(align:,)\"));\n> -\n> - s = to_free = strbuf_split_str_omit_term(valp, ',', 0);\n> -\n> - align->position = ALIGN_LEFT;\n> -\n> - while (*s) {\n> - if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))\n> - ;\n> - else if (!strcmp(s[0]->buf, \"left\"))\n> - align->position = ALIGN_LEFT;\n> - else if (!strcmp(s[0]->buf, \"right\"))\n> - align->position = ALIGN_RIGHT;\n> - else if (!strcmp(s[0]->buf, \"middle\"))\n> - align->position = ALIGN_MIDDLE;\n> - else\n> - die(_(\"improper format entered align:%s\"), s[0]->buf);\n> - s++;\n> - }\n> -\n> - if (width < 0)\n> - die(_(\"positive width expected with the %%(align) atom\"));\n> - align->width = width;\n> - strbuf_list_free(to_free);\n> + v->u.align = atom->u.align;\n> v->handler = align_atom_handler;\n> continue;\n> } else if (!strcmp(name, \"end\")) {\n> --\n> 2.6.4\n"},{"id":"274716","messageId":"CAPig+cSFG86U=+onvJ1aaosmX3k+fzD-431q-hFTnWHpg=pZVw@mail.gmail.com","threadId":"41023","inReplyTo":"1450279802-29414-12-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v2 11/11] ref-filter: introduce objectname_atom_parser()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-18T06:24:50Z","receivedAt":"2015-12-18T06:24:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 16, 2015 at 10:30 AM, Karthik Nayak wrote:\n> Introduce objectname_atom_parser() which will parse the\n> '%(objectname)' atom and store information into the 'used_atom'\n> structure based on the modifiers used along with the atom.\n>\n> Helped-by: Ramsay Jones \n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> @@ -43,6 +43,7 @@ static struct used_atom {\n> enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;\n> unsigned int no_lines;\n> } contents;\n> + enum { O_FULL, O_SHORT } objectname;\n> } u;\n> } *used_atom;\n> @@ -124,6 +125,21 @@ static void contents_atom_parser(struct used_atom *atom)\n> +static void objectname_atom_parser(struct used_atom *atom)\n> +{\n> + const char * buf;\n> +\n> + if (match_atom_name(atom->str, \"objectname\", &buf))\n> + atom->u.objectname = O_FULL;\n> + if (!buf)\n> + return;\n\nLet me make sure that I understand this correctly.\n\nmake_atom_name(\"objectname\") will return true only for \"objectname\" or\n\"objectname:\", and will return false for anything else, such as\n\"objectnamely\" or \"schmorf\". Furthermore, the only way\nobjectname_atom_parser() can be called is when %(objectname) or\n%(objectname:...) is seen, thus match_atom_name() *must* return true\nhere, which means the above conditional is misleading, suggesting that\nit could somehow return false.\n\nAnd, if match_atom_name() did return false here, then that indicates a\nprogramming error: objectname_atom_parser() somehow got called for\nsomething other than %(objectname) or %(objectname:...). This implies\nthat the code should instead be structured like this:\n\n if (!match_atom_name(..., \"objectname\", &buf)\n die(\"BUG: parsing non-'objectname'\")\n if (!buf)\n atom->u.objectname = O_FULL;\n else if (!strcmp(buf, \"short\"))\n atom->u.objectname = O_SHORT;\n else\n die(_(\"unrecognized %%(objectname) argument: %s\"), buf);\n\nHowever, this can be simplified further by recognizing that, following\nthis patch series, match_atom_name() is *only* called by these new\nparse functions[1], which means that, as a convenience,\nmatch_atom_name() itself could become a void rather than boolean\nfunction and die() if the expected atom name is not found. Thus, the\ncode would become:\n\n match_atom_name(...);\n if (!buf)\n ...\n else if (!strcmp(...))\n ...\n ...\n\nBy the way, the above commentary isn't specific to this patch and\n%(objectname), but is in fact also relevant for all of the preceding\npatches which introduce parse functions calling match_atom_name().\n\nMore below...\n\n[1]: ...assuming you replace the unnecessary match_atom_name() in\npopulate_value() with starts_with() as suggested in my patch 7/11\nreview addendum[2].\n\n[2]: http://article.gmane.org/gmane.comp.version-control.git/282700\n\n> +\n> + if (!strcmp(buf, \"short\"))\n> + atom->u.objectname = O_SHORT;\n> + else\n> + die(_(\"unrecognized %%(objectname) argument: %s\"), buf);\n> +}\n> +\n> @@ -461,15 +477,16 @@ static void *get_obj(const unsigned char *sha1, struct object **obj, unsigned lo\n> static int grab_objectname(const char *name, const unsigned char *sha1,\n> - struct atom_value *v)\n> + struct atom_value *v, struct used_atom *atom)\n> {\n> - if (!strcmp(name, \"objectname\")) {\n> - v->s = xstrdup(sha1_to_hex(sha1));\n> - return 1;\n> - }\n> - if (!strcmp(name, \"objectname:short\")) {\n> - v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));\n> - return 1;\n> + if (starts_with(name, \"objectname\")) {\n> + if (atom->u.objectname == O_SHORT) {\n> + v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));\n> + return 1;\n> + } else if (atom->u.objectname == O_FULL) {\n> + v->s = xstrdup(sha1_to_hex(sha1));\n> + return 1;\n> + }\n\nSince 'objectname' can only ever be O_SHORT or O_FULL wouldn't it be a\nprogramming error if it ever falls through to this point after the\nclosing brace? Perhaps a die(\"BUG:...\") would be appropriate?\n\n> }\n> return 0;\n> }\n> @@ -493,7 +510,7 @@ static void grab_common_values(struct atom_value *val, int deref, struct object\n> v->s = xstrfmt(\"%lu\", sz);\n> }\n> else if (deref)\n> - grab_objectname(name, obj->sha1, v);\n> + grab_objectname(name, obj->sha1, v, &used_atom[i]);\n\nThis patch hunk is somehow corrupt and doesn't apply. Was there some\nhand-editing involved, or were some earlier patches regenerated after\nthis patch was made or something?\n\n> }\n> }\n>\n> @@ -999,7 +1016,7 @@ static void populate_value(struct ref_array_item *ref)\n> v->s = xstrdup(buf + 1);\n> }\n> continue;\n> - } else if (!deref && grab_objectname(name, ref->objectname, v)) {\n> + } else if (!deref && grab_objectname(name, ref->objectname, v, atom)) {\n> continue;\n> } else if (!strcmp(name, \"HEAD\")) {\n> const char *head;\n> --\n> 2.6.4\n"},{"id":"274747","messageId":"CAOLa=ZTfBqkYiYpRyZQP5pNisa6kEz29FBpr-QKcAGYwrKk9Tw@mail.gmail.com","threadId":"41023","inReplyTo":"CAPig+cR5=8urU+Vq_doC53+T4WCmEWJdKXyMEsbL3gWxsNpmxg@mail.gmail.com","subject":"Re: [PATCH v2 03/11] ref-filter: introduce struct used_atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-19T04:42:39Z","receivedAt":"2015-12-19T04:42:39Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Thu, Dec 17, 2015 at 2:27 AM, Eric Sunshine wrote:\n> On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak wrote:\n>> Introduce the 'used_array' structure which would replace the existing\n>> implementation of 'used_array' (which a list of atoms). This helps us\n>> parse atom's before hand and store required details into the\n>> 'used_array' for future usage.\n>\n> All my v1 review comments[1] about the commit message still apply to\n> this version.\n>\n> [1]: http://article.gmane.org/gmane.comp.version-control.git/281860\n>\n\nI totally missed this out, thanks for bringing it up.\n\n>> Also introduce a parsing function for each atom in valid_atom. Using\n>> this we can define special parsing functions for each of the atoms.\n>\n> This is a conceptually distinct change which probably deserves its own\n> patch. In particular, the new patch would add this field to\n> valid_atom[] *and* add the code which invokes the custom parser. (That\n> code is currently commingled with introduction of the color parser in\n> patch 6/11.)\n>go\n\nI guess that could be done, I was thinking it goes together, but it\nmakes sense to have a\nseparate patch for introduction of the parsing function and its invocations.\n\n> More below...\n>\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> @@ -16,9 +16,27 @@\n>> +/*\n>> + * An atom is a valid field atom listed below, possibly prefixed with\n>> + * a \"*\" to denote deref_tag().\n>> + *\n>> + * We parse given format string and sort specifiers, and make a list\n>> + * of properties that we need to extract out of objects. ref_array_item\n>> + * structure will hold an array of values extracted that can be\n>> + * indexed with the \"atom number\", which is an index into this\n>> + * array.\n>> + */\n>> +static struct used_atom {\n>> + const char *str;\n>> + cmp_type type;\n>> +} *used_atom;\n>> +static int used_atom_cnt, need_tagged, need_symref;\n>> +static int need_color_reset_at_eol;\n>> +\n>> static struct {\n>> const char *name;\n>> cmp_type cmp_type;\n>> + void (*parser)(struct used_atom *atom);\n>> } valid_atom[] = {\n>> { \"refname\" },\n>> { \"objecttype\" },\n>> @@ -786,7 +788,8 @@ static void populate_value(struct ref_array_item *ref)\n>>\n>> /* Fill in specials first */\n>> for (i = 0; i < used_atom_cnt; i++) {\n>> - const char *name = used_atom[i];\n>> + struct used_atom *atom = &used_atom[i];\n>> + const char *name = atom->str;\n>\n> Same question as my previous review[1]: Why not just:\n>\n> const char *name = used_atom[i].str;\n>\n> ?\n\nI think It's leftover code, I was using the atom variable also before.\nI'll remove it.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"274748","messageId":"CAOLa=ZRiWVy604j0bFyo0C4dkr-r65gLTCFxXCrzp5-CS30AGQ@mail.gmail.com","threadId":"41023","inReplyTo":"CAPig+cROdhYVW2f9ciwsMeXHNvu3BQP5bp4-no93x26dxor5cA@mail.gmail.com","subject":"Re: [PATCH v2 05/11] ref-filter: skip deref specifier in match_atom_name()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-19T05:07:04Z","receivedAt":"2015-12-19T05:07:04Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Thu, Dec 17, 2015 at 2:41 AM, Eric Sunshine wrote:\n> On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak wrote:\n>> In upcoming patches we make calls to match_atom_name() with the '*'\n>> deref specifier still attached to the atom name. This causes\n>> undesirable errors, hence, if present skip over the '*' deref\n>> specifier in the atom name.\n>\n> I'd drop the second sentence since it doesn't add much or any value.\n> Instead, you might want to explain that skipping '*' is done as a\n> convenience.\n>\n> Subsequent patches will call match_atom_name() with the '*' deref\n> specifier still attached to the atom name so, as a convenience,\n> skip over it on their behalf.\n>\n\nThanks will put that in.\n\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> @@ -37,6 +37,10 @@ static int match_atom_name(const char *name, const char *atom_name, const char *\n>> {\n>> const char *body;\n>>\n>> + /* skip the deref specifier*/\n>\n> Too many spaces before \"skip\".\n> Too few spaces after \"specifier\".\n>\n\nWill fix.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"274749","messageId":"CAOLa=ZTRJFrOFbYbqMOsicinJmzrMhi0NB3ySf1=-DTWY6sc+w@mail.gmail.com","threadId":"41023","inReplyTo":"CAPig+cTNEXA75Gh_jXrdhLOJpXYgyZ97TJCPHXz4DodOcjWheA@mail.gmail.com","subject":"Re: [PATCH v2 06/11] ref-filter: introduce color_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-19T06:00:21Z","receivedAt":"2015-12-19T06:00:21Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Thu, Dec 17, 2015 at 2:51 AM, Eric Sunshine wrote:\n> On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak wrote:\n>> Introduce color_atom_parser() which will parse a \"color\" atom and\n>> store its color in the \"use_atom\" structure for further usage in\n>\n> Same comment as last time: s/use_atom/used_atom/\n>\n\nWill change.\n\n>> 'populate_value()'.\n>\n> s/'//g\n>\n\nWill do.\n\n> More below...\n>\n>> Helped-by: Ramsay Jones \n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> @@ -29,6 +29,9 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n>> static struct used_atom {\n>> const char *str;\n>> cmp_type type;\n>> + union {\n>> + const char *color;\n>> + } u;\n>> } *used_atom;\n>> static int used_atom_cnt, need_tagged, need_symref;\n>> static int need_color_reset_at_eol;\n>> @@ -53,6 +56,13 @@ static int match_atom_name(const char *name, const char *atom_name, const char *\n>> +static void color_atom_parser(struct used_atom *atom)\n>> +{\n>> + match_atom_name(atom->str, \"color\", &atom->u.color);\n>> + if (!atom->u.color)\n>> + die(_(\"expected format: %%(color:)\"));\n>> +}\n>> +\n>> @@ -833,12 +846,10 @@ static void populate_value(struct ref_array_item *ref)\n>> refname = branch_get_push(branch, NULL);\n>> if (!refname)\n>> continue;\n>> - } else if (match_atom_name(name, \"color\", &valp)) {\n>> + } else if (starts_with(name, \"color:\")) {\n>> char color[COLOR_MAXLEN] = \"\";\n>>\n>> - if (!valp)\n>> - die(_(\"expected format: %%(color:)\"));\n>> - if (color_parse(valp, color) < 0)\n>> + if (color_parse(atom->u.color, color) < 0)\n>\n> It would make a lot more sense to invoke color_parse() with the\n> unchanging argument to \"color:\" just once in color_atom_parser()\n> rather than doing it here repeatedly. (Also, you'd drop 'const' from\n> used_atom.u.color declaration.)\n>\n\nThis makes sense, I'll put have color_parse() in color_atom_parser().\nThis would also require us to allocate memory once in color_atom_parser.\n\n>\n> Does v->s get freed each time through the loop? If not, then, assuming\n> you parse the color just once in color_atom_parser(), then you could\n> just assign the parsed color directly to v->s rather than duplicating\n> it.\n\nNo it doesn't. Yup will do.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"274755","messageId":"CAOLa=ZQH4sb1e87Hrze_xThkH9fCKMzzFQQ106cXf3w83wr1aA@mail.gmail.com","threadId":"41023","inReplyTo":"CAPig+cR=dYnMfaHUYcq_kz8M+C1nCxOX=SN=_6+ep1aWvVCUow@mail.gmail.com","subject":"Re: [PATCH v2 07/11] ref-filter: introduce align_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-19T11:12:55Z","receivedAt":"2015-12-19T11:12:55Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Thu, Dec 17, 2015 at 3:24 AM, Eric Sunshine wrote:\n> On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak wrote:\n>> Introduce align_atom_parser() which will parse an \"align\" atom and\n>> store the required alignment position and width in the \"use_atom\"\n>> structure for further usage in 'populate_value()'.\n>>\n>> Helped-by: Ramsay Jones \n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> @@ -63,6 +69,49 @@ static void color_atom_parser(struct used_atom *atom)\n>> +static align_type parse_align_position(const char *s)\n>> +{\n>> + if (!strcmp(s, \"right\"))\n>> + return ALIGN_RIGHT;\n>> + else if (!strcmp(s, \"middle\"))\n>> + return ALIGN_MIDDLE;\n>> + else if (!strcmp(s, \"left\"))\n>> + return ALIGN_LEFT;\n>> + return -1;\n>> +}\n>> +\n>> +static void align_atom_parser(struct used_atom *atom)\n>> +{\n>> + struct align *align = &atom->u.align;\n>> + const char *buf = NULL;\n>> + struct strbuf **s, **to_free;\n>> + int width = -1;\n>> +\n>> + match_atom_name(atom->str, \"align\", &buf);\n>> + if (!buf)\n>> + die(_(\"expected format: %%(align:,)\"));\n>> + s = to_free = strbuf_split_str_omit_term(buf, ',', 0);\n>> +\n>> + align->position = ALIGN_LEFT;\n>> +\n>> + while (*s) {\n>> + int position;\n>> +\n>> + if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))\n>\n> This casting is pretty ugly. It probably would be cleaner to declare\n> 'width' as 'unsigned int' and initialize it with ~0U than to do this\n> ugly and potentially dangerous casting. Likewise, below where you\n> check 'width < 0', you'd check instead for ~0U. However, such a change\n> should not be part of the current patch, but rather as a separate\n> preparatory patch.\n>\n\nWill do.\n\n>> + ;\n>> + else if ((position = parse_align_position(s[0]->buf)) > 0)\n>\n> Shouldn't this be '>=' rather than '>'?\n>\n> This likely would have been easier to spot if parse_align_position()\n> had been factored out in its own patch, as suggested by my v1\n> review[1], since the caller would have been trivially inspectable\n> rather than having to keep track of both code movement and changes.\n>\n> [1]: http://article.gmane.org/gmane.comp.version-control.git/281916\n>\n\nYes. it should. Will split this into three patches.\n\n1. Creation of align_atom_parser().\n2. Introduce parse_align_position().\n3. make 'width' an unsigned int.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"274756","messageId":"CAOLa=ZTvuGeKbstvWi939_VD2ciJf0Fhine7VjkDqi_DjPT_9Q@mail.gmail.com","threadId":"41023","inReplyTo":"CAOLa=ZQH4sb1e87Hrze_xThkH9fCKMzzFQQ106cXf3w83wr1aA@mail.gmail.com","subject":"Re: [PATCH v2 07/11] ref-filter: introduce align_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-19T11:35:53Z","receivedAt":"2015-12-19T11:35:53Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Fri, Dec 18, 2015 at 11:20 AM, Eric Sunshine wrote:\n> On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak wrote:\n>> Introduce align_atom_parser() which will parse an \"align\" atom and\n>> store the required alignment position and width in the \"use_atom\"\n>> structure for further usage in 'populate_value()'.\n>>\n>> Helped-by: Ramsay Jones \n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> @@ -63,6 +69,49 @@ static void color_atom_parser(struct used_atom *atom)\n>> +static void align_atom_parser(struct used_atom *atom)\n>> +{\n>> + [...]\n>> + match_atom_name(atom->str, \"align\", &buf);\n>> + if (!buf)\n>> + die(_(\"expected format: %%(align:,)\"));\n>> + [...]\n>> +}\n>> @@ -880,35 +924,7 @@ static void populate_value(struct ref_array_item *ref)\n>> v->s = \" \";\n>> continue;\n>> } else if (match_atom_name(name, \"align\", &valp)) {\n>\n> Hmm, align_atom_parser() has already been called before we get here,\n> right? And it has already invoked match_atom_name() and died if the\n> argument to \"align:\" was missing, correct? If so, then this invocation\n> of match_atom_name() in populate_value() is unnecessary and should be\n> replaced with a simpler starts_with(\"align:\").\n>\n> Plus, 'valp' is not used, aside from this unnecessary\n> match_atom_name(), thus it can be removed as well.\n>\n\nWIll do. Thanks for noticing this. I missed it.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"274981","messageId":"CAOLa=ZQ1SXm6GG0jzokq7shrm0=4B8vvME13Qbh_QCEWt2X=0A@mail.gmail.com","threadId":"41023","inReplyTo":"CAPig+cRG_HTWL=VNqOxDnv_M0NbYZSReCHSsGe1CJiFQS8h-XQ@mail.gmail.com","subject":"Re: [PATCH v2 09/11] ref-filter: introduce remote_ref_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-24T07:42:28Z","receivedAt":"2015-12-24T07:42:28Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Thu, Dec 17, 2015 at 2:52 PM, Eric Sunshine wrote:\n> On Wed, Dec 16, 2015 at 10:30 AM, Karthik Nayak wrote:\n>> Introduce remote_ref_atom_parser() which will parse the '%(upstream)'\n>> and '%(push)' atoms and store information into the 'used_atom'\n>> structure based on the modifiers used along with the corresponding\n>> atom.\n>>\n>> Helped-by: Ramsay Jones \n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> @@ -37,6 +37,8 @@ static struct used_atom {\n>> union {\n>> const char *color;\n>> struct align align;\n>> + enum { RR_SHORTEN, RR_TRACK, RR_TRACKSHORT, RR_NORMAL }\n>\n> Nit: I'd have expected to see the normal/plain case first rather than\n> last (but not itself worth a re-roll).\n>\n\nWill add it in. That'll put it in an alphabetical order too.\n\n>> + remote_ref;\n>> } u;\n>> } *used_atom;\n>> static int used_atom_cnt, need_tagged, need_symref;\n>> @@ -69,6 +71,25 @@ static void color_atom_parser(struct used_atom *atom)\n>> +static void remote_ref_atom_parser(struct used_atom *atom)\n>> +{\n>> + const char *buf;\n>> +\n>> + buf = strchr(atom->str, ':');\n>> + atom->u.remote_ref = RR_NORMAL;\n>> + if (!buf)\n>> + return;\n>\n> This code is not as clear as it could be due to the way the 'buf'\n> assignment and check for NULL are split apart. It can be made clearer\n> either by doing this:\n>\n> atom->u.remote_ref = RR_NORMAL;\n> buf = strchr(...);\n> if (!buf)\n> return;\n>\n> or (even better) this:\n>\n> buf = strchr(...);\n> if (!buf) {\n> atom->u.remote_ref = RR_NORMAL;\n> return;\n> }\n>\n\nWill do the latter, thanks.\n\n>> + buf++;\n>> + if (!strcmp(buf, \"short\"))\n>> + atom->u.remote_ref = RR_SHORTEN;\n>> + else if (!strcmp(buf, \"track\"))\n>> + atom->u.remote_ref = RR_TRACK;\n>> + else if (!strcmp(buf, \"trackshort\"))\n>> + atom->u.remote_ref = RR_TRACKSHORT;\n>> + else\n>> + die(_(\"unrecognized format: %%(%s)\"), atom->str);\n>> +}\n>> +\n>> @@ -835,6 +856,42 @@ static inline char *copy_advance(char *dst, const char *src)\n>> +static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n>> + struct branch *branch, const char **s)\n>> +{\n>> + int num_ours, num_theirs;\n>> + if (atom->u.remote_ref == RR_SHORTEN)\n>> + *s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);\n>> + else if (atom->u.remote_ref == RR_TRACK) {\n>> + if (stat_tracking_info(branch, &num_ours,\n>> + &num_theirs, NULL))\n>> + return;\n>\n> The RR_TRACKSHORT branch below has a blank line following the\n> 'return', but this branch lacks it, which is inconsistent.\n>\n\nwill add.\n\n>> + if (!num_ours && !num_theirs)\n>> + *s = \"\";\n>> + else if (!num_ours)\n>> + *s = xstrfmt(\"[behind %d]\", num_theirs);\n>> + else if (!num_theirs)\n>> + *s = xstrfmt(\"[ahead %d]\", num_ours);\n>> + else\n>> + *s = xstrfmt(\"[ahead %d, behind %d]\",\n>> + num_ours, num_theirs);\n>> + } else if (atom->u.remote_ref == RR_TRACKSHORT) {\n>> + if (stat_tracking_info(branch, &num_ours,\n>> + &num_theirs, NULL))\n>\n> What happened to the assert(branch) which was in the original code\n> from which this was derived (below)? Is it no longer needed?\n>\n\nstat_tracking_info() takes care of that, instead of aborting, we gracefully\ncontinue while leaving that value empty.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"274982","messageId":"CAOLa=ZTCnOwAa1DDmFiNeCdYFo0tkTfDM-fC_AkPStFA6rFU5w@mail.gmail.com","threadId":"41023","inReplyTo":"CAPig+cRmUe4SRzmiV13XWSYou8M7TuDE3Ko37g8p-q2kEQNU+w@mail.gmail.com","subject":"Re: [PATCH v2 10/11] ref-filter: introduce contents_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-24T08:27:18Z","receivedAt":"2015-12-24T08:27:18Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Thu, Dec 17, 2015 at 3:09 PM, Eric Sunshine wrote:\n> On Wed, Dec 16, 2015 at 10:30 AM, Karthik Nayak wrote:\n>> Introduce contents_atom_parser() which will parse the '%(contents)'\n>> atom and store information into the 'used_atom' structure based on the\n>> modifiers used along with the atom.\n>>\n>> Helped-by: Ramsay Jones \n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> @@ -39,6 +39,10 @@ static struct used_atom {\n>> struct align align;\n>> enum { RR_SHORTEN, RR_TRACK, RR_TRACKSHORT, RR_NORMAL }\n>> remote_ref;\n>> + struct {\n>> + enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;\n>> + unsigned int no_lines;\n>\n> 'no_lines' sounds like \"no lines!\". How about 'nlines' instead?\n>\n\nSure, will do.\n\n>> + } contents;\n>> } u;\n>> } *used_atom;\n>> static int used_atom_cnt, need_tagged, need_symref;\n>> @@ -90,6 +94,36 @@ static void remote_ref_atom_parser(struct used_atom *atom)\n>> +static void contents_atom_parser(struct used_atom *atom)\n>> +{\n>> + const char * buf;\n>> +\n>> + if (match_atom_name(atom->str, \"contents\", &buf))\n>> + atom->u.contents.option = C_BARE;\n>> + else if (match_atom_name(atom->str, \"subject\", &buf)) {\n>\n> The original code used strcmp() and matched only \"subject\", however\n> the new code will incorrectly match both \"subject\" and\n> \"subject:whatever\". Therefore, you should be using strcmp() here\n> rather than match_atom_name().\n>\n> Ditto for \"body\".\n\nWill change.\n\n>\n>> + atom->u.contents.option = C_SUB;\n>> + return;\n>> + } else if (match_atom_name(atom->str, \"body\", &buf)) {\n>> + atom->u.contents.option = C_BODY_DEP;\n>> + return;\n>> + }\n>> + if (!buf)\n>> + return;\n>\n> It's not easy to see that this 'if (!buf)' check relates to the\n> \"contents\" check at the very top of the if/else if/ chain since there\n> are entirely unrelated checks in between. Reorganizing it can improve\n> clarity:\n>\n> if (!strcmp(\"subject\")) {\n> ...\n> return;\n> } else if (!strcmp(\"body\")) {\n> ...\n> return;\n> } else if (!match_atom_name(...,\"contents\", &buf))\n> die(\"BUG: expected 'contents' or 'contents:'\");\n>\n> if (!buf) {\n> atom->u.contents.option = C_BARE;\n> return;\n> }\n>\n\nThis looks good. Thanks.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"274996","messageId":"CAOLa=ZSV9NmAcC+yguacWqcwLDnmBZd-arwzk+WnjtM2J1eC_Q@mail.gmail.com","threadId":"41023","inReplyTo":"CAPig+cSFG86U=+onvJ1aaosmX3k+fzD-431q-hFTnWHpg=pZVw@mail.gmail.com","subject":"Re: [PATCH v2 11/11] ref-filter: introduce objectname_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-25T13:44:22Z","receivedAt":"2015-12-25T13:44:22Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Fri, Dec 18, 2015 at 11:54 AM, Eric Sunshine wrote:\n> On Wed, Dec 16, 2015 at 10:30 AM, Karthik Nayak wrote:\n>> Introduce objectname_atom_parser() which will parse the\n>> '%(objectname)' atom and store information into the 'used_atom'\n>> structure based on the modifiers used along with the atom.\n>>\n>> Helped-by: Ramsay Jones \n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> @@ -43,6 +43,7 @@ static struct used_atom {\n>> enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;\n>> unsigned int no_lines;\n>> } contents;\n>> + enum { O_FULL, O_SHORT } objectname;\n>> } u;\n>> } *used_atom;\n>> @@ -124,6 +125,21 @@ static void contents_atom_parser(struct used_atom *atom)\n>> +static void objectname_atom_parser(struct used_atom *atom)\n>> +{\n>> + const char * buf;\n>> +\n>> + if (match_atom_name(atom->str, \"objectname\", &buf))\n>> + atom->u.objectname = O_FULL;\n>> + if (!buf)\n>> + return;\n>\n> Let me make sure that I understand this correctly.\n>\n> make_atom_name(\"objectname\") will return true only for \"objectname\" or\n> \"objectname:\", and will return false for anything else, such as\n> \"objectnamely\" or \"schmorf\". Furthermore, the only way\n> objectname_atom_parser() can be called is when %(objectname) or\n> %(objectname:...) is seen, thus match_atom_name() *must* return true\n> here, which means the above conditional is misleading, suggesting that\n> it could somehow return false.\n>\n> And, if match_atom_name() did return false here, then that indicates a\n> programming error: objectname_atom_parser() somehow got called for\n> something other than %(objectname) or %(objectname:...). This implies\n> that the code should instead be structured like this:\n>\n> if (!match_atom_name(..., \"objectname\", &buf)\n> die(\"BUG: parsing non-'objectname'\")\n> if (!buf)\n> atom->u.objectname = O_FULL;\n> else if (!strcmp(buf, \"short\"))\n> atom->u.objectname = O_SHORT;\n> else\n> die(_(\"unrecognized %%(objectname) argument: %s\"), buf);\n>\n> However, this can be simplified further by recognizing that, following\n> this patch series, match_atom_name() is *only* called by these new\n> parse functions[1], which means that, as a convenience,\n> match_atom_name() itself could become a void rather than boolean\n> function and die() if the expected atom name is not found. Thus, the\n> code would become:\n>\n> match_atom_name(...);\n> if (!buf)\n> ...\n> else if (!strcmp(...))\n> ...\n> ...\n>\n> By the way, the above commentary isn't specific to this patch and\n> %(objectname), but is in fact also relevant for all of the preceding\n> patches which introduce parse functions calling match_atom_name().\n>\n\nAh! Thats some good observation, makes sense, match_atom_name()\nis only called after the atom name, so making it return a indication of\nsuccess or failure doesn't make sense.\n\nI think this change would go nicely with the introduction of 'enum atom_type'\nwhich you mentioned[0] in the previous series.\n\n[0]: http://article.gmane.org/gmane.comp.version-control.git/282320\n\n> More below...\n>\n> [1]: ...assuming you replace the unnecessary match_atom_name() in\n> populate_value() with starts_with() as suggested in my patch 7/11\n> review addendum[2].\n>\n> [2]: http://article.gmane.org/gmane.comp.version-control.git/282700\n>\n>> +\n>> + if (!strcmp(buf, \"short\"))\n>> + atom->u.objectname = O_SHORT;\n>> + else\n>> + die(_(\"unrecognized %%(objectname) argument: %s\"), buf);\n>> +}\n>> +\n>> @@ -461,15 +477,16 @@ static void *get_obj(const unsigned char *sha1, struct object **obj, unsigned lo\n>> static int grab_objectname(const char *name, const unsigned char *sha1,\n>> - struct atom_value *v)\n>> + struct atom_value *v, struct used_atom *atom)\n>> {\n>> - if (!strcmp(name, \"objectname\")) {\n>> - v->s = xstrdup(sha1_to_hex(sha1));\n>> - return 1;\n>> - }\n>> - if (!strcmp(name, \"objectname:short\")) {\n>> - v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));\n>> - return 1;\n>> + if (starts_with(name, \"objectname\")) {\n>> + if (atom->u.objectname == O_SHORT) {\n>> + v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));\n>> + return 1;\n>> + } else if (atom->u.objectname == O_FULL) {\n>> + v->s = xstrdup(sha1_to_hex(sha1));\n>> + return 1;\n>> + }\n>\n> Since 'objectname' can only ever be O_SHORT or O_FULL wouldn't it be a\n> programming error if it ever falls through to this point after the\n> closing brace? Perhaps a die(\"BUG:...\") would be appropriate?\n>\n\nYeah, will add that in.\n\n>> }\n>> return 0;\n>> }\n>> @@ -493,7 +510,7 @@ static void grab_common_values(struct atom_value *val, int deref, struct object\n>> v->s = xstrfmt(\"%lu\", sz);\n>> }\n>> else if (deref)\n>> - grab_objectname(name, obj->sha1, v);\n>> + grab_objectname(name, obj->sha1, v, &used_atom[i]);\n>\n> This patch hunk is somehow corrupt and doesn't apply. Was there some\n> hand-editing involved, or were some earlier patches regenerated after\n> this patch was made or something?\n>\n\nNothing of that sort, weird.\n\n>> }\n>> }\n>>\n>> @@ -999,7 +1016,7 @@ static void populate_value(struct ref_array_item *ref)\n>> v->s = xstrdup(buf + 1);\n>> }\n>> continue;\n>> - } else if (!deref && grab_objectname(name, ref->objectname, v)) {\n>> + } else if (!deref && grab_objectname(name, ref->objectname, v, atom)) {\n>> continue;\n>> } else if (!strcmp(name, \"HEAD\")) {\n>> const char *head;\n>> --\n>> 2.6.4\n\n\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"274997","messageId":"CAPig+cRb2rWX53dkrbEU5+QUf7z69J7X-P-JUsCLuRYZ4ny-Ag@mail.gmail.com","threadId":"41023","inReplyTo":"CAOLa=ZSV9NmAcC+yguacWqcwLDnmBZd-arwzk+WnjtM2J1eC_Q@mail.gmail.com","subject":"Re: [PATCH v2 11/11] ref-filter: introduce objectname_atom_parser()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-25T18:09:46Z","receivedAt":"2015-12-25T18:09:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Dec 25, 2015 at 8:44 AM, Karthik Nayak wrote:\n> On Fri, Dec 18, 2015 at 11:54 AM, Eric Sunshine wrote:\n>> On Wed, Dec 16, 2015 at 10:30 AM, Karthik Nayak wrote:\n>>> +static void objectname_atom_parser(struct used_atom *atom)\n>>> +{\n>>> + const char * buf;\n>>> +\n>>> + if (match_atom_name(atom->str, \"objectname\", &buf))\n>>> + atom->u.objectname = O_FULL;\n>>> + if (!buf)\n>>> + return;\n>>\n>> make_atom_name(\"objectname\") will return true only for \"objectname\" or\n>> \"objectname:\", and will return false for anything else, such as\n>> \"objectnamely\" or \"schmorf\". Furthermore, the only way\n>> objectname_atom_parser() can be called is when %(objectname) or\n>> %(objectname:...) is seen, thus match_atom_name() *must* return true\n>> here, which means the above conditional is misleading, suggesting that\n>> it could somehow return false.\n>>\n>> And, if match_atom_name() did return false here, then that indicates a\n>> programming error: objectname_atom_parser() somehow got called for\n>> something other than %(objectname) or %(objectname:...). This implies\n>> that the code should instead be structured like this:\n>>\n>> if (!match_atom_name(..., \"objectname\", &buf)\n>> die(\"BUG: parsing non-'objectname'\")\n>> if (!buf)\n>> atom->u.objectname = O_FULL;\n>> else if (!strcmp(buf, \"short\"))\n>> atom->u.objectname = O_SHORT;\n>> else\n>> die(_(\"unrecognized %%(objectname) argument: %s\"), buf);\n>>\n>> However, this can be simplified further by recognizing that, following\n>> this patch series, match_atom_name() is *only* called by these new\n>> parse functions[1], which means that, as a convenience,\n>> match_atom_name() itself could become a void rather than boolean\n>> function and die() if the expected atom name is not found. Thus, the\n>> code would become:\n>>\n>> match_atom_name(...);\n>> if (!buf)\n>> ...\n>> else if (!strcmp(...))\n>> ...\n>> ...\n>>\n>> By the way, the above commentary isn't specific to this patch and\n>> %(objectname), but is in fact also relevant for all of the preceding\n>> patches which introduce parse functions calling match_atom_name().\n>\n> Ah! Thats some good observation, makes sense, match_atom_name()\n> is only called after the atom name, so making it return a indication of\n> success or failure doesn't make sense.\n>\n> I think this change would go nicely with the introduction of 'enum atom_type'\n> which you mentioned[0] in the previous series.\n\nI'm not sure to which change you refer as going nicely with the 'enum\natom_type'.\n\nThe observation above is about misleading logic in this series which\nmakes the code confusing. At the very least, the present series should\nclean up the logic to reflect the first example above.\n\nChanging match_atom_name() from boolean to void, as in the second\nexample above, is a nice little cleanup, but less important and\nneedn't be in this series (though it would just be one minor patch at\nthe end of the series).\n"},{"id":"274998","messageId":"CAOLa=ZRRDPtJvyJUDd2L=HeYNLPQOPLDUGgw=1xuHRk5dr6xmQ@mail.gmail.com","threadId":"41023","inReplyTo":"CAPig+cRb2rWX53dkrbEU5+QUf7z69J7X-P-JUsCLuRYZ4ny-Ag@mail.gmail.com","subject":"Re: [PATCH v2 11/11] ref-filter: introduce objectname_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-25T18:24:03Z","receivedAt":"2015-12-25T18:24:03Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Fri, Dec 25, 2015 at 11:39 PM, Eric Sunshine wrote:\n> On Fri, Dec 25, 2015 at 8:44 AM, Karthik Nayak wrote:\n>> On Fri, Dec 18, 2015 at 11:54 AM, Eric Sunshine wrote:\n>>> On Wed, Dec 16, 2015 at 10:30 AM, Karthik Nayak wrote:\n>>>> +static void objectname_atom_parser(struct used_atom *atom)\n>>>> +{\n>>>> + const char * buf;\n>>>> +\n>>>> + if (match_atom_name(atom->str, \"objectname\", &buf))\n>>>> + atom->u.objectname = O_FULL;\n>>>> + if (!buf)\n>>>> + return;\n>>>\n>>> make_atom_name(\"objectname\") will return true only for \"objectname\" or\n>>> \"objectname:\", and will return false for anything else, such as\n>>> \"objectnamely\" or \"schmorf\". Furthermore, the only way\n>>> objectname_atom_parser() can be called is when %(objectname) or\n>>> %(objectname:...) is seen, thus match_atom_name() *must* return true\n>>> here, which means the above conditional is misleading, suggesting that\n>>> it could somehow return false.\n>>>\n>>> And, if match_atom_name() did return false here, then that indicates a\n>>> programming error: objectname_atom_parser() somehow got called for\n>>> something other than %(objectname) or %(objectname:...). This implies\n>>> that the code should instead be structured like this:\n>>>\n>>> if (!match_atom_name(..., \"objectname\", &buf)\n>>> die(\"BUG: parsing non-'objectname'\")\n>>> if (!buf)\n>>> atom->u.objectname = O_FULL;\n>>> else if (!strcmp(buf, \"short\"))\n>>> atom->u.objectname = O_SHORT;\n>>> else\n>>> die(_(\"unrecognized %%(objectname) argument: %s\"), buf);\n>>>\n>>> However, this can be simplified further by recognizing that, following\n>>> this patch series, match_atom_name() is *only* called by these new\n>>> parse functions[1], which means that, as a convenience,\n>>> match_atom_name() itself could become a void rather than boolean\n>>> function and die() if the expected atom name is not found. Thus, the\n>>> code would become:\n>>>\n>>> match_atom_name(...);\n>>> if (!buf)\n>>> ...\n>>> else if (!strcmp(...))\n>>> ...\n>>> ...\n>>>\n>>> By the way, the above commentary isn't specific to this patch and\n>>> %(objectname), but is in fact also relevant for all of the preceding\n>>> patches which introduce parse functions calling match_atom_name().\n>>\n>> Ah! Thats some good observation, makes sense, match_atom_name()\n>> is only called after the atom name, so making it return a indication of\n>> success or failure doesn't make sense.\n>>\n>> I think this change would go nicely with the introduction of 'enum atom_type'\n>> which you mentioned[0] in the previous series.\n>\n> I'm not sure to which change you refer as going nicely with the 'enum\n> atom_type'.\n>\n> The observation above is about misleading logic in this series which\n> makes the code confusing. At the very least, the present series should\n> clean up the logic to reflect the first example above.\n>\n> Changing match_atom_name() from boolean to void, as in the second\n> example above, is a nice little cleanup, but less important and\n> needn't be in this series (though it would just be one minor patch at\n> the end of the series).\n\nI was referring to the second part as you just mentioned, But I could\nat it to the\nend of the series, I have restructured things as you mentioned in the example\nthough. Sorry for the confusion :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"275245","messageId":"CAOLa=ZRrv26j4kHT3tQZ-ch8aycAKG4cQp4DcTTXEwOvMtZx7w@mail.gmail.com","threadId":"41023","inReplyTo":"CAPig+cRmoOoSscuLcVHWkpH4uVU2Gei791qqn0GjyZ4b9fA45w@mail.gmail.com","subject":"Re: [PATCH v2 08/11] ref-filter: introduce prefixes for the align atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-31T13:19:50Z","receivedAt":"2015-12-31T13:19:50Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Thu, Dec 17, 2015 at 2:29 PM, Eric Sunshine wrote:\n> On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak wrote:\n>> ref-filter: introduce prefixes for the align atom\n>\n> The prefixes are actually for the arguments to the 'align' atom, not\n> for the atom itself. However, it might be better to describe this at a\n> bit higher level. Perhaps:\n>\n> ref-filter: align: introduce long-form syntax\n>\n> or something.\n\nMakes sense, thanks.\n\n>\n>> Introduce optional prefixes \"width=\" and \"position=\" for the align atom\n>> so that the atom can be used as \"%(align:width=,position=)\".\n>>\n>> Add Documetation and tests for the same.\n>>\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> @@ -96,10 +96,19 @@ static void align_atom_parser(struct used_atom *atom)\n>>\n>> while (*s) {\n>> int position;\n>> + buf = s[0]->buf;\n>\n> It probably would be better to do this assignment in the previous\n> patch (7/11) since its presence here introduces unwanted noise\n> (textual replacement of \"s[0]->buf\" with \"buf\") in several locations\n> below which slightly obscure the real changes of this patch.\n>\n\nThis makes sense from a reviewers perspective, will do.\n\n>> - if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))\n>> + if (skip_prefix(buf, \"position=\", &buf)) {\n>> + position = parse_align_position(buf);\n>> + if (position == -1)\n>\n> It may be more idiomatic in this codebase to detect errors via '< 0'\n> rather than explicit '== -1'. Ditto below.\n>\n\nI think Junio also mentioned this once. Thanks for reminding.\n\n>> + die(_(\"unrecognized position:%s\"), buf);\n>> + align->position = position;\n>> + } else if (skip_prefix(buf, \"width=\", &buf)) {\n>> + if (strtoul_ui(buf, 10, (unsigned int *)&width))\n>> + die(_(\"unrecognized width:%s\"), buf);\n>> + } else if (!strtoul_ui(buf, 10, (unsigned int *)&width))\n>> ;\n>> - else if ((position = parse_align_position(s[0]->buf)) > 0)\n>> + else if ((position = parse_align_position(buf)) != -1)\n>> align->position = position;\n>> else\n>> die(_(\"unrecognized %%(align) argument: %s\"), s[0]->buf);\n>\n> s/s[0]->//\n>\n\nThanks.\n\n\n> More below...\n>\n>> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\n>> @@ -133,6 +133,168 @@ test_expect_success 'right alignment' '\n>> test_cmp expect actual\n>> '\n>>\n>> +test_expect_success 'alignment with \"position\" prefix' '\n>> + cat >expect <<-\\EOF &&\n>> + | refname is refs/heads/master|refs/heads/master\n>> + | refname is refs/heads/side|refs/heads/side\n>> + | refname is refs/odd/spot|refs/odd/spot\n>> + |refname is refs/tags/double-tag|refs/tags/double-tag\n>> + | refname is refs/tags/four|refs/tags/four\n>> + | refname is refs/tags/one|refs/tags/one\n>> + |refname is refs/tags/signed-tag|refs/tags/signed-tag\n>> + | refname is refs/tags/three|refs/tags/three\n>> + | refname is refs/tags/two|refs/tags/two\n>> + EOF\n>> + git for-each-ref --format=\"|%(align:30,position=right)refname is %(refname)%(end)|%(refname)\" >actual &&\n>> + test_cmp expect actual\n>> +'\n>> +\n>> +test_expect_success 'alignment with \"position\" prefix' '\n>> + cat >expect <<-\\EOF &&\n>> + | refname is refs/heads/master|refs/heads/master\n>> + | refname is refs/heads/side|refs/heads/side\n>> + | refname is refs/odd/spot|refs/odd/spot\n>> + |refname is refs/tags/double-tag|refs/tags/double-tag\n>> + | refname is refs/tags/four|refs/tags/four\n>> + | refname is refs/tags/one|refs/tags/one\n>> + |refname is refs/tags/signed-tag|refs/tags/signed-tag\n>> + | refname is refs/tags/three|refs/tags/three\n>> + | refname is refs/tags/two|refs/tags/two\n>> + EOF\n>> + git for-each-ref --format=\"|%(align:position=right,30)refname is %(refname)%(end)|%(refname)\" >actual &&\n>> + test_cmp expect actual\n>> +'\n>\n> This (and below) is a lot of copy/paste code which makes it difficult\n> to read the tests and maintain (change) them. Since 'expect' doesn't\n> appear to change from test to test, one way to eliminate some of this\n> noisiness would be to create 'expect' once outside of the tests.\n>\n> However, even better, especially from a comprehension,\n> maintainability, and extensibility standpoints would be to make this\n> all table-driven. In particular, I'd expect to see a table with\n> exactly the list of test inputs mentioned in my earlier review[1], and\n> have that table passed to a shell function which performs the test for\n> each element of the table. For instance, something like:\n>\n> test_align_permutations <<-\\EOF\n> middle,42\n> 42,middle\n> position=middle,42\n> 42,position=middle\n> middle,width=42\n> width=42,middle\n> position=middle,width=42\n> width=42,position=middle\n> EOF\n>\n> where test_align_permutations is the name of the shell function which\n> reads each line of it stdin and performs the \"git for-each-ref\n> --format=...\" test with the given %(align:) arguments.\n>\n> Ditto regarding the below \"last one wins (silently) tests\".\n>\n> [1]: http://article.gmane.org/gmane.comp.version-control.git/281916\n>\n\nThis seems like a good idea, I implemented both of those together.\n\ntest_align_permutations() {\nwhile read -r option; do\ntest_expect_success 'align permutations' '\ngit for-each-ref --format=\"|%(align:$option)refname is\n%(refname)%(end)|%(refname)\" >actual &&\ntest_cmp expect actual\n'\ndone;\n}\n\ntest_align_permutations <<-\\EOF\nmiddle,42\n42,middle\nposition=middle,42\n42,position=middle\nmiddle,width=42\nwidth=42,middle\nposition=middle,width=42\nwidth=42,position=middle\nEOF\n\n# Last one wins (silently) when multiple arguments of the same type are given\n\ntest_align_permutations <<-\\EOF\n32,width=42,middle\nwidth=30,42,middle\nwidth=42,position=right,middle\n42,right,position=middle\nEOF\n\nThanks :)\n\n--\nRegards,\nKarthik Nayak\n"}]}