{"thread":{"id":"41425","subject":"[PATCH v5 01/12] strbuf: introduce strbuf_split_str_omit_term()","startedAt":"2016-02-16T19:00:03Z","lastAt":"2016-02-17T18:21:45Z","messageCount":32,"participants":["Karthik Nayak","Jeff King","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":5,"patchTotal":12},"messages":[{"id":"278370","messageId":"1455649215-23260-1-git-send-email-Karthik.188@gmail.com","threadId":"41425","inReplyTo":null,"subject":"[PATCH v5 00/12] ref-filter: use parsing functions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-02-16T19:00:03Z","receivedAt":"2016-02-16T19:00:03Z","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.\n\nv1: http://thread.gmane.org/gmane.comp.version-control.git/281180\nv2: http://thread.gmane.org/gmane.comp.version-control.git/282563\nv3: http://thread.gmane.org/gmane.comp.version-control.git/283350\nv4: http://thread.gmane.org/gmane.comp.version-control.git/285158\n\nChanges:\n* Fixed small errors in multiple 'die(..)' messages.\n* Removed unecessary braces.\n* In parse_align_position() use 'v', 's' to denote the vector of arguments\nand individual argument respectively rather than re-using 'arg'.\n* Fix error in parse_ref_filter_atom() where length of current atom wasn't\naccurately calculated.\n* Small code and indentation fixes\n\nThanks to Eric, Junio, Ramsay and Andreas for their comments on the previous\nversion. And everyone else who helped review the previous patch series.\n\nKarthik Nayak (12):\n strbuf: introduce strbuf_split_str_omit_term()\n ref-filter: use strbuf_split_str_omit_term()\n ref-filter: bump 'used_atom' and related code to the top\n ref-filter: introduce struct used_atom\n ref-filter: introduce parsing functions for each valid atom\n ref-filter: introduce color_atom_parser()\n ref-filter: introduce parse_align_position()\n ref-filter: introduce align_atom_parser()\n ref-filter: align: introduce long-form syntax\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 | 434 +++++++++++++++++++++----------------\n strbuf.c | 14 +-\n strbuf.h | 25 ++-\n t/t6302-for-each-ref-filter.sh | 42 ++++\n 5 files changed, 329 insertions(+), 206 deletions(-)\n\nInterdiff:\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex d48e2a3..21f4937 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -54,14 +54,14 @@ static void color_atom_parser(struct used_atom *atom, const char *color_value)\n \tif (!color_value)\n \t\tdie(_(\"expected format: %%(color:)\"));\n \tif (color_parse(color_value, atom->u.color) < 0)\n-\t\tdie(_(\"invalid color value: %s\"), atom->u.color);\n+\t\tdie(_(\"unrecognized color: %%(color:%s)\"), color_value);\n }\n \n static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)\n {\n-\tif (!arg) {\n+\tif (!arg)\n \t\tatom->u.remote_ref = RR_NORMAL;\n-\t} else if (!strcmp(arg, \"short\"))\n+\telse if (!strcmp(arg, \"short\"))\n \t\tatom->u.remote_ref = RR_SHORTEN;\n \telse if (!strcmp(arg, \"track\"))\n \t\tatom->u.remote_ref = RR_TRACK;\n@@ -74,14 +74,14 @@ static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)\n static void body_atom_parser(struct used_atom *atom, const char *arg)\n {\n \tif (arg)\n-\t\tdie(\"%%(body) atom does not take arguments\");\n+\t\tdie(\"%%(body) does not take arguments\");\n \tatom->u.contents.option = C_BODY_DEP;\n }\n \n static void subject_atom_parser(struct used_atom *atom, const char *arg)\n {\n \tif (arg)\n-\t\tdie(\"%%(subject) atom does not take arguments\");\n+\t\tdie(\"%%(subject) does not take arguments\");\n \tatom->u.contents.option = C_SUB;\n }\n \n@@ -127,34 +127,34 @@ static align_type parse_align_position(const char *s)\n static void align_atom_parser(struct used_atom *atom, const char *arg)\n {\n \tstruct align *align = &atom->u.align;\n-\tstruct strbuf **s, **to_free;\n+\tstruct strbuf **v, **to_free;\n \tunsigned int width = ~0U;\n \n \tif (!arg)\n \t\tdie(_(\"expected format: %%(align:,)\"));\n-\ts = to_free = strbuf_split_str_omit_term(arg, ',', 0);\n+\tv = to_free = strbuf_split_str_omit_term(arg, ',', 0);\n \n \talign->position = ALIGN_LEFT;\n \n-\twhile (*s) {\n+\twhile (*v) {\n \t\tint position;\n-\t\targ = s[0]->buf;\n+\t\tconst char *s = v[0]->buf;\n \n-\t\tif (skip_prefix(arg, \"position=\", &arg)) {\n-\t\t\tposition = parse_align_position(arg);\n+\t\tif (skip_prefix(s, \"position=\", &s)) {\n+\t\t\tposition = parse_align_position(s);\n \t\t\tif (position < 0)\n-\t\t\t\tdie(_(\"unrecognized position:%s\"), arg);\n+\t\t\t\tdie(_(\"unrecognized position:%s\"), s);\n \t\t\talign->position = position;\n-\t\t} else if (skip_prefix(arg, \"width=\", &arg)) {\n-\t\t\tif (strtoul_ui(arg, 10, &width))\n-\t\t\t\tdie(_(\"unrecognized width:%s\"), arg);\n-\t\t} else if (!strtoul_ui(arg, 10, &width))\n+\t\t} else if (skip_prefix(s, \"width=\", &s)) {\n+\t\t\tif (strtoul_ui(s, 10, &width))\n+\t\t\t\tdie(_(\"unrecognized width:%s\"), s);\n+\t\t} else if (!strtoul_ui(s, 10, &width))\n \t\t\t;\n-\t\telse if ((position = parse_align_position(arg)) >= 0)\n+\t\telse if ((position = parse_align_position(s)) >= 0)\n \t\t\talign->position = position;\n \t\telse\n-\t\t\tdie(_(\"unrecognized %%(align) argument: %s\"), arg);\n-\t\ts++;\n+\t\t\tdie(_(\"unrecognized %%(align) argument: %s\"), s);\n+\t\tv++;\n \t}\n \n \tif (width == ~0U)\n@@ -253,6 +253,7 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n \t/* Is the atom a valid one? */\n \tfor (i = 0; i < ARRAY_SIZE(valid_atom); i++) {\n \t\tint len = strlen(valid_atom[i].name);\n+\n \t\t/*\n \t\t * If the atom name has a colon, strip it and everything after\n \t\t * it off - it specifies the format for this entry, and\n@@ -260,7 +261,7 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n \t\t * table.\n \t\t */\n \t\targ = memchr(sp, ':', ep - sp);\n-\t\tif ((!arg || len == arg - sp) &&\n+\t\tif (len == (arg ? arg : ep) - sp &&\n \t\t !memcmp(valid_atom[i].name, sp, len))\n \t\t\tbreak;\n \t}\n@@ -775,8 +776,8 @@ 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].name;\n \t\tstruct used_atom *atom = &used_atom[i];\n+\t\tconst char *name = atom->name;\n \t\tstruct atom_value *v = &val[i];\n \t\tif (!!deref != (*name == '*'))\n \t\t\tcontinue;\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex f79355d..bcf472b 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -149,8 +149,8 @@ test_align_permutations() {\n \twhile read -r option\n \tdo\n \t\ttest_expect_success \"align:$option\" '\n-\t\tgit for-each-ref --format=\"|%(align:$option)refname is %(refname)%(end)|%(refname)\" >actual &&\n-\t\ttest_cmp expect actual\n+\t\t\tgit for-each-ref --format=\"|%(align:$option)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\t\t\ttest_cmp expect actual\n \t\t'\n \tdone\n }\n\n-- \n2.7.1\n"},{"id":"278368","messageId":"1455649215-23260-2-git-send-email-Karthik.188@gmail.com","threadId":"41425","inReplyTo":"1455649215-23260-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v5 01/12] strbuf: introduce strbuf_split_str_omit_term()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-02-16T19:00:04Z","receivedAt":"2016-02-16T19:00:04Z","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_omit_term()' which\nsplits a given string into strbufs without including the terminator.\n\nHelped-by: Eric Sunshine \nSigned-off-by: Karthik Nayak \n---\n strbuf.c | 14 +++++++++-----\n strbuf.h | 25 ++++++++++++++++---------\n 2 files changed, 25 insertions(+), 14 deletions(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex bab316d..4a93e2a 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,18 @@ struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n \n \twhile (slen) {\n \t\tint len = slen;\n+\t\tint copylen = len;\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\tif (end)\n+\t\t\tend = memchr(str, terminator, slen);\n+\t\t\tif (end) {\n \t\t\t\tlen = end - str + 1;\n+\t\t\t\tcopylen = len - !!omit_term;\n+\t\t\t}\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_init(t, copylen);\n+\t\tstrbuf_add(t, str, copylen);\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 f72fd14..6115e72 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -466,11 +466,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. If omit_term is true, the terminator will\n+ * be stripped from all substrings. Otherwise, substrings will include\n+ * the terminator, except for the final substring, if the original\n+ * string lacked 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@@ -481,19 +482,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.7.1\n"},{"id":"278371","messageId":"1455649215-23260-3-git-send-email-Karthik.188@gmail.com","threadId":"41425","inReplyTo":"1455649215-23260-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v5 02/12] ref-filter: use strbuf_split_str_omit_term()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-02-16T19:00:05Z","receivedAt":"2016-02-16T19:00:05Z","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\nHelped-by: Eric Sunshine \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 f097176..38f38d4 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -892,18 +892,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.7.1\n"},{"id":"278373","messageId":"1455649215-23260-4-git-send-email-Karthik.188@gmail.com","threadId":"41425","inReplyTo":"1455649215-23260-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v5 03/12] ref-filter: bump 'used_atom' and related code to the top","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-02-16T19:00:06Z","receivedAt":"2016-02-16T19:00:06Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Bump code to the top for usage in further patches.\n\nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 30 +++++++++++++++---------------\n 1 file changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 38f38d4..c3a8372 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -16,6 +16,21 @@\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 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 static struct {\n \tconst char *name;\n \tcmp_type cmp_type;\n@@ -92,21 +107,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-- \n2.7.1\n"},{"id":"278372","messageId":"1455649215-23260-5-git-send-email-Karthik.188@gmail.com","threadId":"41425","inReplyTo":"1455649215-23260-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v5 04/12] ref-filter: introduce struct used_atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-02-16T19:00:07Z","receivedAt":"2016-02-16T19:00:07Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Introduce the 'used_atom' structure to replace the existing\nimplementation of 'used_atom' (which is a list of atoms). This helps\nus parse atoms beforehand and store required details into the\n'used_atom' for future usage.\n\nHelped-by: Eric Sunshine \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 35 ++++++++++++++++++-----------------\n 1 file changed, 18 insertions(+), 17 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex c3a8372..3736dc3 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -26,8 +26,10 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\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 struct used_atom {\n+\tconst char *name;\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@@ -122,8 +124,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].name);\n+\t\tif (len == ep - atom && !memcmp(used_atom[i].name, atom, len))\n \t\t\treturn i;\n \t}\n \n@@ -150,12 +152,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].name = 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].name, \"symref\"))\n \t\tneed_symref = 1;\n \treturn at;\n }\n@@ -315,7 +316,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].name, \"color:\", &color))\n \t\t\tneed_color_reset_at_eol = !!strcmp(color, \"reset\");\n \t}\n \treturn 0;\n@@ -359,7 +360,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].name;\n \t\tstruct atom_value *v = &val[i];\n \t\tif (!!deref != (*name == '*'))\n \t\t\tcontinue;\n@@ -383,7 +384,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].name;\n \t\tstruct atom_value *v = &val[i];\n \t\tif (!!deref != (*name == '*'))\n \t\t\tcontinue;\n@@ -405,7 +406,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].name;\n \t\tstruct atom_value *v = &val[i];\n \t\tif (!!deref != (*name == '*'))\n \t\t\tcontinue;\n@@ -535,7 +536,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].name;\n \t\tstruct atom_value *v = &val[i];\n \t\tif (!!deref != (*name == '*'))\n \t\t\tcontinue;\n@@ -573,7 +574,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].name;\n \t\tstruct atom_value *v = &val[i];\n \t\tif (!!deref != (*name == '*'))\n \t\t\tcontinue;\n@@ -663,7 +664,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].name;\n \t\tstruct atom_value *v = &val[i];\n \t\tconst char *valp = NULL;\n \t\tif (!!deref != (*name == '*'))\n@@ -809,7 +810,7 @@ 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\tconst char *name = used_atom[i].name;\n \t\tstruct atom_value *v = &ref->value[i];\n \t\tint deref = 0;\n \t\tconst char *refname;\n@@ -1464,7 +1465,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.7.1\n"},{"id":"278369","messageId":"1455649215-23260-6-git-send-email-Karthik.188@gmail.com","threadId":"41425","inReplyTo":"1455649215-23260-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v5 05/12] ref-filter: introduce parsing functions for each valid atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-02-16T19:00:08Z","receivedAt":"2016-02-16T19:00:08Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Parsing atoms is done in populate_value(), this is repetitive and\nhence expensive. Introduce a parsing function which would let us parse\natoms beforehand and store the required details into the 'used_atom'\nstructure for further usage.\n\nHelped-by: Eric Sunshine \nHelped-by: Ramsay Jones \nHelped-by: Andreas Schwab \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 14 ++++++++++----\n 1 file changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 3736dc3..974c412 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -36,6 +36,7 @@ static int need_color_reset_at_eol;\n static struct {\n \tconst char *name;\n \tcmp_type cmp_type;\n+\tvoid (*parser)(struct used_atom *atom, const char *arg);\n } valid_atom[] = {\n \t{ \"refname\" },\n \t{ \"objecttype\" },\n@@ -114,6 +115,7 @@ struct atom_value {\n int parse_ref_filter_atom(const char *atom, const char *ep)\n {\n \tconst char *sp;\n+\tconst char *arg;\n \tint i, at;\n \n \tsp = atom;\n@@ -132,16 +134,16 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n \t/* Is the atom a valid one? */\n \tfor (i = 0; i < ARRAY_SIZE(valid_atom); i++) {\n \t\tint len = strlen(valid_atom[i].name);\n+\n \t\t/*\n \t\t * If the atom name has a colon, strip it and everything after\n \t\t * it off - it specifies the format for this entry, and\n \t\t * shouldn't be used for checking against the valid_atom\n \t\t * table.\n \t\t */\n-\t\tconst char *formatp = strchr(sp, ':');\n-\t\tif (!formatp || ep < formatp)\n-\t\t\tformatp = ep;\n-\t\tif (len == formatp - sp && !memcmp(valid_atom[i].name, sp, len))\n+\t\targ = memchr(sp, ':', ep - sp);\n+\t\tif (len == (arg ? arg : ep) - sp &&\n+\t\t !memcmp(valid_atom[i].name, sp, len))\n \t\t\tbreak;\n \t}\n \n@@ -154,6 +156,10 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n \tREALLOC_ARRAY(used_atom, used_atom_cnt);\n \tused_atom[at].name = xmemdupz(atom, ep - atom);\n \tused_atom[at].type = valid_atom[i].cmp_type;\n+\tif (arg)\n+\t\targ = used_atom[at].name + (arg - atom) + 1;\n+\tif (valid_atom[i].parser)\n+\t\tvalid_atom[i].parser(&used_atom[at], arg);\n \tif (*atom == '*')\n \t\tneed_tagged = 1;\n \tif (!strcmp(used_atom[at].name, \"symref\"))\n-- \n2.7.1\n"},{"id":"278376","messageId":"1455649215-23260-7-git-send-email-Karthik.188@gmail.com","threadId":"41425","inReplyTo":"1455649215-23260-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v5 06/12] ref-filter: introduce color_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-02-16T19:00:09Z","receivedAt":"2016-02-16T19:00:09Z","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 \"used_atom\" structure for further usage in\npopulate_value().\n\nHelped-by: Ramsay Jones \nHelped-by: Eric Sunshine \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 25 ++++++++++++++++---------\n 1 file changed, 16 insertions(+), 9 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 974c412..f401da6 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -29,10 +29,21 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n static struct used_atom {\n \tconst char *name;\n \tcmp_type type;\n+\tunion {\n+\t\tchar color[COLOR_MAXLEN];\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 \n+static void color_atom_parser(struct used_atom *atom, const char *color_value)\n+{\n+\tif (!color_value)\n+\t\tdie(_(\"expected format: %%(color:)\"));\n+\tif (color_parse(color_value, atom->u.color) < 0)\n+\t\tdie(_(\"unrecognized color: %%(color:%s)\"), color_value);\n+}\n+\n static struct {\n \tconst char *name;\n \tcmp_type cmp_type;\n@@ -70,7 +81,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@@ -158,6 +169,7 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n \tused_atom[at].type = valid_atom[i].cmp_type;\n \tif (arg)\n \t\targ = used_atom[at].name + (arg - atom) + 1;\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], arg);\n \tif (*atom == '*')\n@@ -816,6 +828,7 @@ 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\tstruct used_atom *atom = &used_atom[i];\n \t\tconst char *name = used_atom[i].name;\n \t\tstruct atom_value *v = &ref->value[i];\n \t\tint deref = 0;\n@@ -856,14 +869,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} else if (match_atom_name(name, \"color\", &valp)) {\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\t\tdie(_(\"unable to parse format\"));\n-\t\t\tv->s = xstrdup(color);\n+\t\t} else if (starts_with(name, \"color:\")) {\n+\t\t\tv->s = atom->u.color;\n \t\t\tcontinue;\n \t\t} else if (!strcmp(name, \"flag\")) {\n \t\t\tchar buf[256], *cp = buf;\n-- \n2.7.1\n"},{"id":"278374","messageId":"1455649215-23260-8-git-send-email-Karthik.188@gmail.com","threadId":"41425","inReplyTo":"1455649215-23260-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v5 07/12] ref-filter: introduce parse_align_position()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-02-16T19:00:10Z","receivedAt":"2016-02-16T19:00:10Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":">From populate_value() extract parse_align_position() which given a\nstring would give us the alignment position. This is a preparatory\npatch as to introduce prefixes for the %(align) atom and avoid\nredundancy in the code.\n\nHelped-by: Eric Sunshine \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 21 +++++++++++++++------\n 1 file changed, 15 insertions(+), 6 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex f401da6..a08ea83 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -44,6 +44,17 @@ static void color_atom_parser(struct used_atom *atom, const char *color_value)\n \t\tdie(_(\"unrecognized color: %%(color:%s)\"), color_value);\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 struct {\n \tconst char *name;\n \tcmp_type cmp_type;\n@@ -911,14 +922,12 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\talign->position = ALIGN_LEFT;\n \n \t\t\twhile (*s) {\n+\t\t\t\tint position;\n+\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 if ((position = parse_align_position(s[0]->buf)) >= 0)\n+\t\t\t\t\talign->position = position;\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-- \n2.7.1\n"},{"id":"278375","messageId":"1455649215-23260-9-git-send-email-Karthik.188@gmail.com","threadId":"41425","inReplyTo":"1455649215-23260-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v5 08/12] ref-filter: introduce align_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-02-16T19:00:11Z","receivedAt":"2016-02-16T19:00:11Z","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 'used_atom'\nstructure for further usage in populate_value().\n\nSince this patch removes the last usage of match_atom_name(), remove\nthe function from ref-filter.c.\n\nHelped-by: Eric Sunshine \nHelped-by: Ramsay Jones \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 91 ++++++++++++++++++++++++++----------------------------------\n 1 file changed, 40 insertions(+), 51 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex a08ea83..4a9711f 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\tchar color[COLOR_MAXLEN];\n+\t\tstruct align align;\n \t} u;\n } *used_atom;\n static int used_atom_cnt, need_tagged, need_symref;\n@@ -55,6 +61,37 @@ static align_type parse_align_position(const char *s)\n \treturn -1;\n }\n \n+static void align_atom_parser(struct used_atom *atom, const char *arg)\n+{\n+\tstruct align *align = &atom->u.align;\n+\tstruct strbuf **v, **to_free;\n+\tunsigned int width = ~0U;\n+\n+\tif (!arg)\n+\t\tdie(_(\"expected format: %%(align:,)\"));\n+\tv = to_free = strbuf_split_str_omit_term(arg, ',', 0);\n+\n+\talign->position = ALIGN_LEFT;\n+\n+\twhile (*v) {\n+\t\tint position;\n+\t\tconst char *s = v[0]->buf;\n+\n+\t\tif (!strtoul_ui(s, 10, &width))\n+\t\t\t;\n+\t\telse if ((position = parse_align_position(s)) >= 0)\n+\t\t\talign->position = position;\n+\t\telse\n+\t\t\tdie(_(\"unrecognized %%(align) argument: %s\"), s);\n+\t\tv++;\n+\t}\n+\n+\tif (width == ~0U)\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@@ -93,17 +130,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@@ -288,22 +320,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@@ -845,7 +861,6 @@ static void populate_value(struct ref_array_item *ref)\n \t\tint deref = 0;\n \t\tconst char *refname;\n \t\tconst char *formatp;\n-\t\tconst char *valp;\n \t\tstruct branch *branch = NULL;\n \n \t\tv->handler = append_atom;\n@@ -909,34 +924,8 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\telse\n \t\t\t\tv->s = \" \";\n \t\t\tcontinue;\n-\t\t} else if (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\tint position;\n-\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 ((position = parse_align_position(s[0]->buf)) >= 0)\n-\t\t\t\t\talign->position = position;\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} else if (starts_with(name, \"align\")) {\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.7.1\n"},{"id":"278379","messageId":"1455649215-23260-10-git-send-email-Karthik.188@gmail.com","threadId":"41425","inReplyTo":"1455649215-23260-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v5 09/12] ref-filter: align: introduce long-form syntax","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-02-16T19:00:12Z","receivedAt":"2016-02-16T19:00:12Z","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 Documentation and tests for the same.\n\nHelped-by: Eric Sunshine \nSigned-off-by: Karthik Nayak \n---\n Documentation/git-for-each-ref.txt | 20 ++++++++++--------\n ref-filter.c | 10 ++++++++-\n t/t6302-for-each-ref-filter.sh | 42 ++++++++++++++++++++++++++++++++++++++\n 3 files changed, 63 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 2e3e96f..012e8f9 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -133,14 +133,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 4a9711f..13a8280 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -77,7 +77,15 @@ static void align_atom_parser(struct used_atom *atom, const char *arg)\n \t\tint position;\n \t\tconst char *s = v[0]->buf;\n \n-\t\tif (!strtoul_ui(s, 10, &width))\n+\t\tif (skip_prefix(s, \"position=\", &s)) {\n+\t\t\tposition = parse_align_position(s);\n+\t\t\tif (position < 0)\n+\t\t\t\tdie(_(\"unrecognized position:%s\"), s);\n+\t\t\talign->position = position;\n+\t\t} else if (skip_prefix(s, \"width=\", &s)) {\n+\t\t\tif (strtoul_ui(s, 10, &width))\n+\t\t\t\tdie(_(\"unrecognized width:%s\"), s);\n+\t\t} else if (!strtoul_ui(s, 10, &width))\n \t\t\t;\n \t\telse if ((position = parse_align_position(s)) >= 0)\n \t\t\talign->position = position;\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex fe4796c..bcf472b 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -133,6 +133,48 @@ test_expect_success 'right alignment' '\n \ttest_cmp expect actual\n '\n \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+\n+test_align_permutations() {\n+\twhile read -r option\n+\tdo\n+\t\ttest_expect_success \"align:$option\" '\n+\t\t\tgit for-each-ref --format=\"|%(align:$option)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\t\t\ttest_cmp expect actual\n+\t\t'\n+\tdone\n+}\n+\n+test_align_permutations <<-\\EOF\n+\tmiddle,42\n+\t42,middle\n+\tposition=middle,42\n+\t42,position=middle\n+\tmiddle,width=42\n+\twidth=42,middle\n+\tposition=middle,width=42\n+\twidth=42,position=middle\n+EOF\n+\n+# Last one wins (silently) when multiple arguments of the same type are given\n+\n+test_align_permutations <<-\\EOF\n+\t32,width=42,middle\n+\twidth=30,42,middle\n+\twidth=42,position=right,middle\n+\t42,right,position=middle\n+EOF\n+\n # Individual atoms inside %(align:...) and %(end) must not be quoted.\n \n test_expect_success 'alignment with format quote' \"\n-- \n2.7.1\n"},{"id":"278377","messageId":"1455649215-23260-11-git-send-email-Karthik.188@gmail.com","threadId":"41425","inReplyTo":"1455649215-23260-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v5 10/12] ref-filter: introduce remote_ref_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-02-16T19:00:13Z","receivedAt":"2016-02-16T19:00:13Z","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 \nHelped-by: Eric Sunshine \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 103 +++++++++++++++++++++++++++++++++++------------------------\n 1 file changed, 61 insertions(+), 42 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 13a8280..8005e45 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -37,6 +37,8 @@ static struct used_atom {\n \tunion {\n \t\tchar color[COLOR_MAXLEN];\n \t\tstruct align align;\n+\t\tenum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }\n+\t\t\tremote_ref;\n \t} u;\n } *used_atom;\n static int used_atom_cnt, need_tagged, need_symref;\n@@ -50,6 +52,20 @@ static void color_atom_parser(struct used_atom *atom, const char *color_value)\n \t\tdie(_(\"unrecognized color: %%(color:%s)\"), color_value);\n }\n \n+static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)\n+{\n+\tif (!arg)\n+\t\tatom->u.remote_ref = RR_NORMAL;\n+\telse if (!strcmp(arg, \"short\"))\n+\t\tatom->u.remote_ref = RR_SHORTEN;\n+\telse if (!strcmp(arg, \"track\"))\n+\t\tatom->u.remote_ref = RR_TRACK;\n+\telse if (!strcmp(arg, \"trackshort\"))\n+\t\tatom->u.remote_ref = RR_TRACKSHORT;\n+\telse\n+\t\tdie(_(\"unrecognized format: %%(%s)\"), atom->name);\n+}\n+\n static align_type parse_align_position(const char *s)\n {\n \tif (!strcmp(s, \"right\"))\n@@ -132,8 +148,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@@ -840,6 +856,43 @@ static const char *strip_ref_components(const char *refname, const char *nr_arg)\n \treturn start;\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+\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 /* RR_NORMAL */\n+\t\t*s = refname;\n+}\n+\n /*\n * Parse the object referred by ref, and grab needed value.\n */\n@@ -891,8 +944,9 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\tbranch = branch_get(branch_name);\n \n \t\t\trefname = branch_get_upstream(branch, NULL);\n-\t\t\tif (!refname)\n-\t\t\t\tcontinue;\n+\t\t\tif (refname)\n+\t\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@@ -903,6 +957,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\tv->s = atom->u.color;\n \t\t\tcontinue;\n@@ -944,7 +1000,6 @@ 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 \t\t\tconst char *arg;\n \n \t\t\tformatp++;\n@@ -953,43 +1008,7 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\t\t\t\t warn_ambiguous_refs);\n \t\t\telse if (skip_prefix(formatp, \"strip=\", &arg))\n \t\t\t\trefname = strip_ref_components(refname, arg);\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.7.1\n"},{"id":"278378","messageId":"1455649215-23260-12-git-send-email-Karthik.188@gmail.com","threadId":"41425","inReplyTo":"1455649215-23260-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v5 11/12] ref-filter: introduce contents_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-02-16T19:00:14Z","receivedAt":"2016-02-16T19:00:14Z","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. Also introduce body_atom_parser()\nand subject_atom_parser() for parsing atoms '%(body)' and '%(subject)'\nrespectively.\n\nHelped-by: Ramsay Jones \nHelped-by: Eric Sunshine \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 79 ++++++++++++++++++++++++++++++++++++++----------------------\n 1 file changed, 50 insertions(+), 29 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 8005e45..3895a8c 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_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }\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 nlines;\n+\t\t} contents;\n \t} u;\n } *used_atom;\n static int used_atom_cnt, need_tagged, need_symref;\n@@ -66,6 +70,38 @@ static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)\n \t\tdie(_(\"unrecognized format: %%(%s)\"), atom->name);\n }\n \n+static void body_atom_parser(struct used_atom *atom, const char *arg)\n+{\n+\tif (arg)\n+\t\tdie(\"%%(body) does not take arguments\");\n+\tatom->u.contents.option = C_BODY_DEP;\n+}\n+\n+static void subject_atom_parser(struct used_atom *atom, const char *arg)\n+{\n+\tif (arg)\n+\t\tdie(\"%%(subject) does not take arguments\");\n+\tatom->u.contents.option = C_SUB;\n+}\n+\n+static void contents_atom_parser(struct used_atom *atom, const char *arg)\n+{\n+\tif (!arg)\n+\t\tatom->u.contents.option = C_BARE;\n+\telse if (!strcmp(arg, \"body\"))\n+\t\tatom->u.contents.option = C_BODY;\n+\telse if (!strcmp(arg, \"signature\"))\n+\t\tatom->u.contents.option = C_SIG;\n+\telse if (!strcmp(arg, \"subject\"))\n+\t\tatom->u.contents.option = C_SUB;\n+\telse if (skip_prefix(arg, \"lines=\", &arg)) {\n+\t\tatom->u.contents.option = C_LINES;\n+\t\tif (strtoul_ui(arg, 10, &atom->u.contents.nlines))\n+\t\t\tdie(_(\"positive value expected contents:lines=%s\"), arg);\n+\t} else\n+\t\tdie(_(\"unrecognized %%(contents) argument: %s\"), arg);\n+}\n+\n static align_type parse_align_position(const char *s)\n {\n \tif (!strcmp(s, \"right\"))\n@@ -145,9 +181,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, subject_atom_parser },\n+\t{ \"body\", FIELD_STR, body_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@@ -160,11 +196,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@@ -181,7 +212,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@@ -733,20 +763,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].name;\n+\t\tstruct used_atom *atom = &used_atom[i];\n+\t\tconst char *name = atom->name;\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@@ -754,28 +780,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\t\tv->s = copy_subject(subpos, sublen);\n-\t\telse if (!strcmp(name, \"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 (!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.nlines);\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.7.1\n"},{"id":"278380","messageId":"1455649215-23260-13-git-send-email-Karthik.188@gmail.com","threadId":"41425","inReplyTo":"1455649215-23260-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v5 12/12] ref-filter: introduce objectname_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-02-16T19:00:15Z","receivedAt":"2016-02-16T19:00:15Z","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 \nHelped-by: Eric Sunshine \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 35 ++++++++++++++++++++++++-----------\n 1 file changed, 24 insertions(+), 11 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 3895a8c..21f4937 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 nlines;\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@@ -102,6 +103,16 @@ static void contents_atom_parser(struct used_atom *atom, const char *arg)\n \t\tdie(_(\"unrecognized %%(contents) argument: %s\"), arg);\n }\n \n+static void objectname_atom_parser(struct used_atom *atom, const char *arg)\n+{\n+\tif (!arg)\n+\t\tatom->u.objectname = O_FULL;\n+\telse if (!strcmp(arg, \"short\"))\n+\t\tatom->u.objectname = O_SHORT;\n+\telse\n+\t\tdie(_(\"unrecognized %%(objectname) argument: %s\"), arg);\n+}\n+\n static align_type parse_align_position(const char *s)\n {\n \tif (!strcmp(s, \"right\"))\n@@ -160,7 +171,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@@ -440,15 +451,17 @@ 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} else\n+\t\t\tdie(\"BUG: unknown %%(objectname) option\");\n \t}\n \treturn 0;\n }\n@@ -472,7 +485,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->oid.hash, v);\n+\t\t\tgrab_objectname(name, obj->oid.hash, v, &used_atom[i]);\n \t}\n }\n \n@@ -996,7 +1009,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.7.1\n"},{"id":"278381","messageId":"20160216192231.GA16567@sigill.intra.peff.net","threadId":"41425","inReplyTo":"1455649215-23260-3-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v5 02/12] ref-filter: use strbuf_split_str_omit_term()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-16T19:22:32Z","receivedAt":"2016-02-16T19:22:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 17, 2016 at 12:30:05AM +0530, Karthik Nayak wrote:\n\n> Use the newly introduced strbuf_split_str_omit_term() rather than\n> using strbuf_split_str() and manually removing the ',' terminator.\n> \n> Helped-by: Eric Sunshine \n> Signed-off-by: Karthik Nayak \n> ---\n> ref-filter.c | 9 +--------\n> 1 file changed, 1 insertion(+), 8 deletions(-)\n\nDid you consider just using string_list_split for this? AFAICT, you\ndon't care about the results being strbufs themselves, and it would do\nwhat you want without having to bother with patch 1. The result would\nlook something like the patch below.\n\nSorry to waltz into a review of v5 with a suggestion to throw out all\nthe work done in previous iterations. :-/ I just think the strbuf_split\ninterface is kind of clunky and I'd be happy if we could slowly get rid\nof it rather than growing it. Maybe that's not realistic, though (some\nof the callsites _do_ want to do things like strbuf_trim() after\nsplitting).\n\n-Peff\n"},{"id":"278382","messageId":"20160216192312.GB16567@sigill.intra.peff.net","threadId":"41425","inReplyTo":"20160216192231.GA16567@sigill.intra.peff.net","subject":"Re: [PATCH v5 02/12] ref-filter: use strbuf_split_str_omit_term()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-16T19:23:12Z","receivedAt":"2016-02-16T19:23:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 16, 2016 at 02:22:32PM -0500, Jeff King wrote:\n\n> On Wed, Feb 17, 2016 at 12:30:05AM +0530, Karthik Nayak wrote:\n> \n> > Use the newly introduced strbuf_split_str_omit_term() rather than\n> > using strbuf_split_str() and manually removing the ',' terminator.\n> > \n> > Helped-by: Eric Sunshine \n> > Signed-off-by: Karthik Nayak \n> > ---\n> > ref-filter.c | 9 +--------\n> > 1 file changed, 1 insertion(+), 8 deletions(-)\n> \n> Did you consider just using string_list_split for this? AFAICT, you\n> don't care about the results being strbufs themselves, and it would do\n> what you want without having to bother with patch 1. The result would\n> look something like the patch below.\n\nProbably help if I actually included the patch.\n\n---\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 9ccfc51..45b3352 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -886,41 +886,34 @@ static void populate_value(struct ref_array_item *ref)\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\tstruct string_list params = STRING_LIST_INIT_DUP;\n+\t\t\tint i;\n \t\t\tint width = -1;\n \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-\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\tstring_list_split(¶ms, valp, ',', -1);\n+\t\t\tfor (i = 0; i < params.nr; i++) {\n+\t\t\t\tconst char *p = params.items[i].string;\n+\t\t\t\tif (!strtoul_ui(p, 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\telse if (!strcmp(p, \"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\telse if (!strcmp(p, \"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\telse if (!strcmp(p, \"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\t\tdie(_(\"improper format entered align:%s\"), p);\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\tstring_list_clear(¶ms, 0);\n \t\t\tv->handler = align_atom_handler;\n \t\t\tcontinue;\n \t\t} else if (!strcmp(name, \"end\")) {\n"},{"id":"278383","messageId":"CAPig+cTiwHs+dD+jqAp8SNkwjQ2OzDsC8yopRgF7gctrGi5uUw@mail.gmail.com","threadId":"41425","inReplyTo":"20160216192231.GA16567@sigill.intra.peff.net","subject":"Re: [PATCH v5 02/12] ref-filter: use strbuf_split_str_omit_term()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-16T20:12:29Z","receivedAt":"2016-02-16T20:12:29Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Feb 16, 2016 at 2:22 PM, Jeff King wrote:\n> On Wed, Feb 17, 2016 at 12:30:05AM +0530, Karthik Nayak wrote:\n>> Use the newly introduced strbuf_split_str_omit_term() rather than\n>> using strbuf_split_str() and manually removing the ',' terminator.\n>>\n>> Helped-by: Eric Sunshine \n>> Signed-off-by: Karthik Nayak \n>> ---\n> Did you consider just using string_list_split for this? AFAICT, you\n> don't care about the results being strbufs themselves, and it would do\n> what you want without having to bother with patch 1. [...]\n>\n> Sorry to waltz into a review of v5 with a suggestion to throw out all\n> the work done in previous iterations. :-/ I just think the strbuf_split\n> interface is kind of clunky and I'd be happy if we could slowly get rid\n> of it rather than growing it. [...]\n\nThat's a nice idea, however, I'm not sure if making it part of this\nseries this late in the game is a good idea. The series has gone\nthrough major changes and heavy review in each of the preceding\nversions, and turnaround time has been consequently quite slow (due\nboth to the amount of work required by Karthik for each version, and\nto the amount of time needed by reviewers to digest all the new\nchanges). v4 was the first one which had settled to the point where\nonly minor changes were needed, and we were hoping to land the series\nwith v5. (A few larger changes were also discussed in v4 reviews, but\nwe concluded that they could be done as follow-up patches.)\n\nWith that in mind, it might be better to make this change as a\nfollowup to this series. On the other hand, as you say, waiting would\nexpand the strbuf_split interface undesirably, so the alternative\nwould be for Karthik to submit v6 with this change only (to wit: drop\npatch 1 and rewrite patch 2 as you've shown). While such a change will\nagain require careful review, at least it is well localized, and\nKarthik's turnaround time shouldn't be too bad. So...\n"},{"id":"278387","messageId":"20160216204954.GC27484@sigill.intra.peff.net","threadId":"41425","inReplyTo":"CAPig+cTiwHs+dD+jqAp8SNkwjQ2OzDsC8yopRgF7gctrGi5uUw@mail.gmail.com","subject":"Re: [PATCH v5 02/12] ref-filter: use strbuf_split_str_omit_term()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-16T20:49:54Z","receivedAt":"2016-02-16T20:49:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 16, 2016 at 03:12:29PM -0500, Eric Sunshine wrote:\n\n> > Did you consider just using string_list_split for this? AFAICT, you\n> > don't care about the results being strbufs themselves, and it would do\n> > what you want without having to bother with patch 1. [...]\n> >\n> > Sorry to waltz into a review of v5 with a suggestion to throw out all\n> > the work done in previous iterations. :-/ I just think the strbuf_split\n> > interface is kind of clunky and I'd be happy if we could slowly get rid\n> > of it rather than growing it. [...]\n> \n> That's a nice idea, however, I'm not sure if making it part of this\n> series this late in the game is a good idea. The series has gone\n> through major changes and heavy review in each of the preceding\n> versions, and turnaround time has been consequently quite slow (due\n> both to the amount of work required by Karthik for each version, and\n> to the amount of time needed by reviewers to digest all the new\n> changes). v4 was the first one which had settled to the point where\n> only minor changes were needed, and we were hoping to land the series\n> with v5. (A few larger changes were also discussed in v4 reviews, but\n> we concluded that they could be done as follow-up patches.)\n> \n> With that in mind, it might be better to make this change as a\n> followup to this series. On the other hand, as you say, waiting would\n> expand the strbuf_split interface undesirably, so the alternative\n> would be for Karthik to submit v6 with this change only (to wit: drop\n> patch 1 and rewrite patch 2 as you've shown). While such a change will\n> again require careful review, at least it is well localized, and\n> Karthik's turnaround time shouldn't be too bad. So...\n\nYeah, I don't insist, and like I said, I'm not 100% sure we can get rid\nof the strbuf_split interface anyway. I thought it might actually make\nthings easier by making the series _shorter_ (so my regret was that\nmentioning earlier could have saved reviewing effort on patch 1).\n\nIt does mean extra review of the patch I posted, but my hope was that\nit's small and localized, and wouldn't impact the later stuff seriously\n(there are some textual tweaks to carry it forward, though).\n\nAnyway, I've said my piece, and you guys can do what you will with it.\n\n-Peff\n"},{"id":"278388","messageId":"CAPig+cQDs35Uirm5cG552tR8iCFOstNJoOzLCZiXCgnq+g7MRQ@mail.gmail.com","threadId":"41425","inReplyTo":"20160216204954.GC27484@sigill.intra.peff.net","subject":"Re: [PATCH v5 02/12] ref-filter: use strbuf_split_str_omit_term()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-16T21:09:53Z","receivedAt":"2016-02-16T21:09:53Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Feb 16, 2016 at 3:49 PM, Jeff King wrote:\n> On Tue, Feb 16, 2016 at 03:12:29PM -0500, Eric Sunshine wrote:\n>> > Did you consider just using string_list_split for this? AFAICT, you\n>> > don't care about the results being strbufs themselves, and it would do\n>> > what you want without having to bother with patch 1. [...]\n>>\n>> That's a nice idea, however, I'm not sure if making it part of this\n>> series this late in the game is a good idea. The series has gone\n>> through major changes and heavy review in each of the preceding\n>> versions, and turnaround time has been consequently quite slow (due\n>> both to the amount of work required by Karthik for each version, and\n>> to the amount of time needed by reviewers to digest all the new\n>> changes). v4 was the first one which had settled to the point where\n>> only minor changes were needed, and we were hoping to land the series\n>> with v5. [...]\n>>\n>> With that in mind, it might be better to make this change as a\n>> followup to this series. On the other hand, as you say, waiting would\n>> expand the strbuf_split interface undesirably, so the alternative\n>> would be for Karthik to submit v6 with this change only (to wit: drop\n>> patch 1 and rewrite patch 2 as you've shown). While such a change will\n>> again require careful review, at least it is well localized, and\n>> Karthik's turnaround time shouldn't be too bad. So...\n>\n> Yeah, I don't insist, and like I said, I'm not 100% sure we can get rid\n> of the strbuf_split interface anyway. I thought it might actually make\n> things easier by making the series _shorter_ (so my regret was that\n> mentioning earlier could have saved reviewing effort on patch 1).\n>\n> It does mean extra review of the patch I posted, but my hope was that\n> it's small and localized, and wouldn't impact the later stuff seriously\n> (there are some textual tweaks to carry it forward, though).\n\nMy initial reaction was negative due to the heavy review burden this\nseries has demanded thus far, however, my mind was changing even as I\ncomposed the above response. In retrospect, I think I'd be okay seeing\na v6, for the following reasons:\n\n- I already ended up reviewing the the suggested new changes pretty\nclosely as a side-effect of reading your proposal.\n\n- It would indeed be nice to avoid introducing\nstrbuf_split_str_omit_term() in the first place; thus one less thing\nto worry about if someone ever takes on the task of retiring the\nstrbuf_split interface.\n\n- It should be only a minimal amount of work for Karthik, thus\nturnaround time should be short.\n\nSo, I think I'm fine with it, if Karthik is game.\n"},{"id":"278401","messageId":"20160216223451.GB9014@sigill.intra.peff.net","threadId":"41425","inReplyTo":"CAPig+cQDs35Uirm5cG552tR8iCFOstNJoOzLCZiXCgnq+g7MRQ@mail.gmail.com","subject":"Re: [PATCH v5 02/12] ref-filter: use strbuf_split_str_omit_term()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-16T22:34:51Z","receivedAt":"2016-02-16T22:34:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 16, 2016 at 04:09:53PM -0500, Eric Sunshine wrote:\n\n> My initial reaction was negative due to the heavy review burden this\n> series has demanded thus far, however, my mind was changing even as I\n> composed the above response. In retrospect, I think I'd be okay seeing\n> a v6, for the following reasons:\n> \n> - I already ended up reviewing the the suggested new changes pretty\n> closely as a side-effect of reading your proposal.\n> \n> - It would indeed be nice to avoid introducing\n> strbuf_split_str_omit_term() in the first place; thus one less thing\n> to worry about if someone ever takes on the task of retiring the\n> strbuf_split interface.\n> \n> - It should be only a minimal amount of work for Karthik, thus\n> turnaround time should be short.\n> \n> So, I think I'm fine with it, if Karthik is game.\n\nI started to write up a commit message for my proposed change. But it\ndid make me think of a counter-argument. Right now we parse\n\"%(align:10,middle)\" but do not allow \"%(align: 10, middle)\".\n\nShould we? Or perhaps: might we? If the answer is yes, we are likely\nbetter off with strbuf_split, because then we are only a strbuf_trim()\naway from making that work.\n\n-Peff\n"},{"id":"278403","messageId":"CAPig+cS+i5QfpUbs8T+CqcDkC4ybaTygE9bguiqQMNgV4JhDOQ@mail.gmail.com","threadId":"41425","inReplyTo":"20160216223451.GB9014@sigill.intra.peff.net","subject":"Re: [PATCH v5 02/12] ref-filter: use strbuf_split_str_omit_term()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-16T22:49:19Z","receivedAt":"2016-02-16T22:49:19Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Feb 16, 2016 at 5:34 PM, Jeff King wrote:\n> On Tue, Feb 16, 2016 at 04:09:53PM -0500, Eric Sunshine wrote:\n>> My initial reaction was negative due to the heavy review burden this\n>> series has demanded thus far, however, my mind was changing even as I\n>> composed the above response. In retrospect, I think I'd be okay seeing\n>> a v6, for the following reasons:\n>>\n>> - I already ended up reviewing the the suggested new changes pretty\n>> closely as a side-effect of reading your proposal.\n>>\n>> - It would indeed be nice to avoid introducing\n>> strbuf_split_str_omit_term() in the first place; thus one less thing\n>> to worry about if someone ever takes on the task of retiring the\n>> strbuf_split interface.\n>>\n>> - It should be only a minimal amount of work for Karthik, thus\n>> turnaround time should be short.\n>>\n>> So, I think I'm fine with it, if Karthik is game.\n>\n> I started to write up a commit message for my proposed change. But it\n> did make me think of a counter-argument. Right now we parse\n> \"%(align:10,middle)\" but do not allow \"%(align: 10, middle)\".\n>\n> Should we? Or perhaps: might we? If the answer is yes, we are likely\n> better off with strbuf_split, because then we are only a strbuf_trim()\n> away from making that work.\n\nI also considered the issue of embedded whitespace very early on when\nreading your initial proposal, but didn't mention anything about it\ndue to a vague recollection from one of the early reviews (or possibly\na review of one of Karthik's other patch series) of someone (possibly\nJunio) saying or implying that embedded whitespace would not be\nsupported. Unfortunately, I can't locate that message (assuming it\neven exists and wasn't a figment of my imagination).\n"},{"id":"278409","messageId":"20160216231811.GA18634@sigill.intra.peff.net","threadId":"41425","inReplyTo":"CAPig+cS+i5QfpUbs8T+CqcDkC4ybaTygE9bguiqQMNgV4JhDOQ@mail.gmail.com","subject":"Re: [PATCH v5 02/12] ref-filter: use strbuf_split_str_omit_term()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-16T23:18:12Z","receivedAt":"2016-02-16T23:18:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 16, 2016 at 05:49:19PM -0500, Eric Sunshine wrote:\n\n> On Tue, Feb 16, 2016 at 5:34 PM, Jeff King wrote:\n> > On Tue, Feb 16, 2016 at 04:09:53PM -0500, Eric Sunshine wrote:\n> >> My initial reaction was negative due to the heavy review burden this\n> >> series has demanded thus far, however, my mind was changing even as I\n> >> composed the above response. In retrospect, I think I'd be okay seeing\n> >> a v6, for the following reasons:\n> >>\n> >> - I already ended up reviewing the the suggested new changes pretty\n> >> closely as a side-effect of reading your proposal.\n> >>\n> >> - It would indeed be nice to avoid introducing\n> >> strbuf_split_str_omit_term() in the first place; thus one less thing\n> >> to worry about if someone ever takes on the task of retiring the\n> >> strbuf_split interface.\n> >>\n> >> - It should be only a minimal amount of work for Karthik, thus\n> >> turnaround time should be short.\n> >>\n> >> So, I think I'm fine with it, if Karthik is game.\n> >\n> > I started to write up a commit message for my proposed change. But it\n> > did make me think of a counter-argument. Right now we parse\n> > \"%(align:10,middle)\" but do not allow \"%(align: 10, middle)\".\n> >\n> > Should we? Or perhaps: might we? If the answer is yes, we are likely\n> > better off with strbuf_split, because then we are only a strbuf_trim()\n> > away from making that work.\n> \n> I also considered the issue of embedded whitespace very early on when\n> reading your initial proposal, but didn't mention anything about it\n> due to a vague recollection from one of the early reviews (or possibly\n> a review of one of Karthik's other patch series) of someone (possibly\n> Junio) saying or implying that embedded whitespace would not be\n> supported. Unfortunately, I can't locate that message (assuming it\n> even exists and wasn't a figment of my imagination).\n\nYeah, I could not find any relevant reference (though I didn't spend all\nthat long digging).\n\nFor reference, I rebuilt Karthik's series on top of my proposal, and the\nchanges are fairly minor. I pushed it to:\n\n git://github.com/peff/git.git jk/tweaked-ref-filter\n\nThe tbdiff is below. Hopefully having that done makes it easier to\ndecide based on the outcome, rather than the pain of rebasing. :)\n\nTo be honest, though, I am now on the fence, considering the possible\nwhitespace issue.\n\n 1: 92de9c7 < --: ------- strbuf: introduce strbuf_split_str_omit_term()\n 2: 4845dc5 < --: ------- ref-filter: use strbuf_split_str_omit_term()\n--: ------- > 1: 29177cc ref-filter: use string_list_split over strbuf_split\n 3: 040e9ce = 2: ed284bc ref-filter: bump 'used_atom' and related code to the top\n 4: c7eb061 = 3: 2a99777 ref-filter: introduce struct used_atom\n 5: c3e24cf = 4: b18f23b ref-filter: introduce parsing functions for each valid atom\n 6: 0b7fe83 = 5: e5221cc ref-filter: introduce color_atom_parser()\n 7: ffb3afe ! 6: 454af9c ref-filter: introduce parse_align_position()\n @@ -32,21 +32,21 @@\n \tconst char *name;\n \tcmp_type cmp_type;\n @@\n - \t\t\talign->position = ALIGN_LEFT;\n - \n - \t\t\twhile (*s) {\n + \t\t\tstring_list_split(¶ms, valp, ',', -1);\n + \t\t\tfor (i = 0; i < params.nr; i++) {\n + \t\t\t\tconst char *s = params.items[i].string;\n +\t\t\t\tint position;\n +\n - \t\t\t\tif (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))\n + \t\t\t\tif (!strtoul_ui(s, 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\telse if (!strcmp(s, \"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\telse if (!strcmp(s, \"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\telse if (!strcmp(s, \"middle\"))\n -\t\t\t\t\talign->position = ALIGN_MIDDLE;\n -+\t\t\t\telse if ((position = parse_align_position(s[0]->buf)) >= 0)\n ++\t\t\t\telse if ((position = parse_align_position(s)) >= 0)\n +\t\t\t\t\talign->position = position;\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\t\tdie(_(\"improper format entered align:%s\"), s);\n + \t\t\t}\n 8: 0f0e596 ! 7: 0779954 ref-filter: introduce align_atom_parser()\n @@ -43,18 +43,19 @@\n +static void align_atom_parser(struct used_atom *atom, const char *arg)\n +{\n +\tstruct align *align = &atom->u.align;\n -+\tstruct strbuf **v, **to_free;\n ++\tstruct string_list params = STRING_LIST_INIT_DUP;\n ++\tint i;\n +\tunsigned int width = ~0U;\n +\n +\tif (!arg)\n +\t\tdie(_(\"expected format: %%(align:,)\"));\n -+\tv = to_free = strbuf_split_str_omit_term(arg, ',', 0);\n +\n +\talign->position = ALIGN_LEFT;\n +\n -+\twhile (*v) {\n ++\tstring_list_split(¶ms, arg, ',', -1);\n ++\tfor (i = 0; i < params.nr; i++) {\n ++\t\tconst char *s = params.items[i].string;\n +\t\tint position;\n -+\t\tconst char *s = v[0]->buf;\n +\n +\t\tif (!strtoul_ui(s, 10, &width))\n +\t\t\t;\n @@ -62,13 +63,12 @@\n +\t\t\talign->position = position;\n +\t\telse\n +\t\t\tdie(_(\"unrecognized %%(align) argument: %s\"), s);\n -+\t\tv++;\n +\t}\n +\n +\tif (width == ~0U)\n +\t\tdie(_(\"positive width expected with the %%(align) atom\"));\n +\talign->width = width;\n -+\tstrbuf_list_free(to_free);\n ++\tstring_list_clear(¶ms, 0);\n +}\n +\n static struct {\n @@ -130,32 +130,32 @@\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\tstruct string_list params = STRING_LIST_INIT_DUP;\n +-\t\t\tint i;\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\tstring_list_split(¶ms, valp, ',', -1);\n +-\t\t\tfor (i = 0; i < params.nr; i++) {\n +-\t\t\t\tconst char *s = params.items[i].string;\n -\t\t\t\tint position;\n -\n --\t\t\t\tif (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))\n +-\t\t\t\tif (!strtoul_ui(s, 10, (unsigned int *)&width))\n -\t\t\t\t\t;\n --\t\t\t\telse if ((position = parse_align_position(s[0]->buf)) >= 0)\n +-\t\t\t\telse if ((position = parse_align_position(s)) >= 0)\n -\t\t\t\t\talign->position = position;\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\t\tdie(_(\"improper format entered align:%s\"), s);\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\tstring_list_clear(¶ms, 0);\n +\t\t} else if (starts_with(name, \"align\")) {\n +\t\t\tv->u.align = atom->u.align;\n \t\t\tv->handler = align_atom_handler;\n 9: d3dc384 ! 8: 792c89a ref-filter: align: introduce long-form syntax\n @@ -45,8 +45,8 @@\n --- a/ref-filter.c\n +++ b/ref-filter.c\n @@\n + \t\tconst char *s = params.items[i].string;\n \t\tint position;\n - \t\tconst char *s = v[0]->buf;\n \n -\t\tif (!strtoul_ui(s, 10, &width))\n +\t\tif (skip_prefix(s, \"position=\", &s)) {\n10: 3ae28b5 = 9: 019fee7 ref-filter: introduce remote_ref_atom_parser()\n11: 06c70af = 10: f6e4f5a ref-filter: introduce contents_atom_parser()\n12: c9db181 = 11: 0a84b70 ref-filter: introduce objectname_atom_parser()\n"},{"id":"278419","messageId":"xmqqbn7gkxev.fsf@gitster.mtv.corp.google.com","threadId":"41425","inReplyTo":"20160216231811.GA18634@sigill.intra.peff.net","subject":"Re: [PATCH v5 02/12] ref-filter: use strbuf_split_str_omit_term()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-17T00:12:08Z","receivedAt":"2016-02-17T00:12:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> > Should we? Or perhaps: might we? If the answer is yes, we are likely\n>> > better off with strbuf_split, because then we are only a strbuf_trim()\n>> > away from making that work.\n>> \n>> I also considered the issue of embedded whitespace very early on when\n>> reading your initial proposal, but didn't mention anything about it\n>> due to a vague recollection from one of the early reviews (or possibly\n>> a review of one of Karthik's other patch series) of someone (possibly\n>> Junio) saying or implying that embedded whitespace would not be\n>> supported. Unfortunately, I can't locate that message (assuming it\n>> even exists and wasn't a figment of my imagination).\n>\n> Yeah, I could not find any relevant reference (though I didn't spend all\n> that long digging).\n>\n> For reference, I rebuilt Karthik's series on top of my proposal, and the\n> changes are fairly minor. I pushed it to:\n>\n> git://github.com/peff/git.git jk/tweaked-ref-filter\n>\n> The tbdiff is below. Hopefully having that done makes it easier to\n> decide based on the outcome, rather than the pain of rebasing. :)\n>\n> To be honest, though, I am now on the fence, considering the possible\n> whitespace issue.\n\nCertainly not having to see s[0]->buf over and over is a huge win ;-).\n\nIs the \"whitespace issue\" a big deal? Does it involve more than a\nsimilar sibling to string_list_split() that trims the whitespace\naround the delimiter (or allows a regexp as a delimiter \"\\s*,\\s*\")?\n"},{"id":"278421","messageId":"20160217002215.GA1187@sigill.intra.peff.net","threadId":"41425","inReplyTo":"xmqqbn7gkxev.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v5 02/12] ref-filter: use strbuf_split_str_omit_term()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-17T00:22:15Z","receivedAt":"2016-02-17T00:22:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 16, 2016 at 04:12:08PM -0800, Junio C Hamano wrote:\n\n> > To be honest, though, I am now on the fence, considering the possible\n> > whitespace issue.\n> \n> Certainly not having to see s[0]->buf over and over is a huge win ;-).\n> \n> Is the \"whitespace issue\" a big deal? Does it involve more than a\n> similar sibling to string_list_split() that trims the whitespace\n> around the delimiter (or allows a regexp as a delimiter \"\\s*,\\s*\")?\n\nI think that solution would work (and IMHO would actually be preferable\nto the split-then-trim that strbuf_split does). But it does mean writing\nnew code.\n\n-Peff\n"},{"id":"278422","messageId":"xmqq37sskwo5.fsf@gitster.mtv.corp.google.com","threadId":"41425","inReplyTo":"20160217002215.GA1187@sigill.intra.peff.net","subject":"Re: [PATCH v5 02/12] ref-filter: use strbuf_split_str_omit_term()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-17T00:28:10Z","receivedAt":"2016-02-17T00:28:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Feb 16, 2016 at 04:12:08PM -0800, Junio C Hamano wrote:\n>\n>> > To be honest, though, I am now on the fence, considering the possible\n>> > whitespace issue.\n>> \n>> Certainly not having to see s[0]->buf over and over is a huge win ;-).\n>> \n>> Is the \"whitespace issue\" a big deal? Does it involve more than a\n>> similar sibling to string_list_split() that trims the whitespace\n>> around the delimiter (or allows a regexp as a delimiter \"\\s*,\\s*\")?\n>\n> I think that solution would work (and IMHO would actually be preferable\n> to the split-then-trim that strbuf_split does). But it does mean writing\n> new code.\n\nTrue, but only when we decide to support trimming the whitespace,\nwhich can come later.\n\nI do not even know if it is wise to accept %(align:position=left, width=4)\nwhen %(align:position=left,width=4) would do the job just fine.\n"},{"id":"278424","messageId":"20160217003259.GD1187@sigill.intra.peff.net","threadId":"41425","inReplyTo":"xmqq37sskwo5.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v5 02/12] ref-filter: use strbuf_split_str_omit_term()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-17T00:32:59Z","receivedAt":"2016-02-17T00:32:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 16, 2016 at 04:28:10PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Tue, Feb 16, 2016 at 04:12:08PM -0800, Junio C Hamano wrote:\n> >\n> >> > To be honest, though, I am now on the fence, considering the possible\n> >> > whitespace issue.\n> >> \n> >> Certainly not having to see s[0]->buf over and over is a huge win ;-).\n> >> \n> >> Is the \"whitespace issue\" a big deal? Does it involve more than a\n> >> similar sibling to string_list_split() that trims the whitespace\n> >> around the delimiter (or allows a regexp as a delimiter \"\\s*,\\s*\")?\n> >\n> > I think that solution would work (and IMHO would actually be preferable\n> > to the split-then-trim that strbuf_split does). But it does mean writing\n> > new code.\n> \n> True, but only when we decide to support trimming the whitespace,\n> which can come later.\n> \n> I do not even know if it is wise to accept %(align:position=left, width=4)\n> when %(align:position=left,width=4) would do the job just fine.\n\nYeah, it was mostly just about being friendly to the user. But if nobody\nis complaining, it may not even be worth worrying about.\n\n-Peff\n"},{"id":"278466","messageId":"CAOLa=ZSgt=+OnSQsDvp0S5yKNekZ0XF4D1OncfyH4152Nvf6QQ@mail.gmail.com","threadId":"41425","inReplyTo":"20160216192231.GA16567@sigill.intra.peff.net","subject":"Re: [PATCH v5 02/12] ref-filter: use strbuf_split_str_omit_term()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-02-17T16:58:24Z","receivedAt":"2016-02-17T16:58:24Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Feb 17, 2016 at 12:52 AM, Jeff King wrote:\n> On Wed, Feb 17, 2016 at 12:30:05AM +0530, Karthik Nayak wrote:\n>\n>> Use the newly introduced strbuf_split_str_omit_term() rather than\n>> using strbuf_split_str() and manually removing the ',' terminator.\n>>\n>> Helped-by: Eric Sunshine \n>> Signed-off-by: Karthik Nayak \n>> ---\n>> ref-filter.c | 9 +--------\n>> 1 file changed, 1 insertion(+), 8 deletions(-)\n>\n> Did you consider just using string_list_split for this? AFAICT, you\n> don't care about the results being strbufs themselves, and it would do\n> what you want without having to bother with patch 1. The result would\n> look something like the patch below.\n>\n\nI haven't, thanks for bringing it up :)\n\n> Sorry to waltz into a review of v5 with a suggestion to throw out all\n> the work done in previous iterations. :-/ I just think the strbuf_split\n> interface is kind of clunky and I'd be happy if we could slowly get rid\n> of it rather than growing it. Maybe that's not realistic, though (some\n> of the callsites _do_ want to do things like strbuf_trim() after\n> splitting).\n>\n> -Peff\n\nThat's fine, as I see it, it's better to wait a while and get a better version\nof something.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"278467","messageId":"CAOLa=ZQO065j5VfJabbV6jww5Z2f3jbaRQDfDcG9NY4x2txrFQ@mail.gmail.com","threadId":"41425","inReplyTo":"CAPig+cQDs35Uirm5cG552tR8iCFOstNJoOzLCZiXCgnq+g7MRQ@mail.gmail.com","subject":"Re: [PATCH v5 02/12] ref-filter: use strbuf_split_str_omit_term()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-02-17T17:04:59Z","receivedAt":"2016-02-17T17:04:59Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Feb 17, 2016 at 2:39 AM, Eric Sunshine wrote:\n> On Tue, Feb 16, 2016 at 3:49 PM, Jeff King wrote:\n>> On Tue, Feb 16, 2016 at 03:12:29PM -0500, Eric Sunshine wrote:\n>>> > Did you consider just using string_list_split for this? AFAICT, you\n>>> > don't care about the results being strbufs themselves, and it would do\n>>> > what you want without having to bother with patch 1. [...]\n>>>\n>>> That's a nice idea, however, I'm not sure if making it part of this\n>>> series this late in the game is a good idea. The series has gone\n>>> through major changes and heavy review in each of the preceding\n>>> versions, and turnaround time has been consequently quite slow (due\n>>> both to the amount of work required by Karthik for each version, and\n>>> to the amount of time needed by reviewers to digest all the new\n>>> changes). v4 was the first one which had settled to the point where\n>>> only minor changes were needed, and we were hoping to land the series\n>>> with v5. [...]\n>>>\n>>> With that in mind, it might be better to make this change as a\n>>> followup to this series. On the other hand, as you say, waiting would\n>>> expand the strbuf_split interface undesirably, so the alternative\n>>> would be for Karthik to submit v6 with this change only (to wit: drop\n>>> patch 1 and rewrite patch 2 as you've shown). While such a change will\n>>> again require careful review, at least it is well localized, and\n>>> Karthik's turnaround time shouldn't be too bad. So...\n>>\n>> Yeah, I don't insist, and like I said, I'm not 100% sure we can get rid\n>> of the strbuf_split interface anyway. I thought it might actually make\n>> things easier by making the series _shorter_ (so my regret was that\n>> mentioning earlier could have saved reviewing effort on patch 1).\n>>\n>> It does mean extra review of the patch I posted, but my hope was that\n>> it's small and localized, and wouldn't impact the later stuff seriously\n>> (there are some textual tweaks to carry it forward, though).\n>\n> My initial reaction was negative due to the heavy review burden this\n> series has demanded thus far, however, my mind was changing even as I\n> composed the above response. In retrospect, I think I'd be okay seeing\n> a v6, for the following reasons:\n>\n> - I already ended up reviewing the the suggested new changes pretty\n> closely as a side-effect of reading your proposal.\n>\n> - It would indeed be nice to avoid introducing\n> strbuf_split_str_omit_term() in the first place; thus one less thing\n> to worry about if someone ever takes on the task of retiring the\n> strbuf_split interface.\n>\n> - It should be only a minimal amount of work for Karthik, thus\n> turnaround time should be short.\n>\n> So, I think I'm fine with it, if Karthik is game.\n\nSounds good to me.\n\nI just read the conversation between Jeff, Junio and You about the whitespace\ncounter-argument and I think its good to go ahead with v6 with Jeff's suggested\nchange.\n\nSince he's already pushed the changes on top of my changes to:\n\n git://github.com/peff/git.git jk/tweaked-ref-filter\n\nI'll just have a look and push that to the list as v6.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"278469","messageId":"CAPig+cTrh4u7vgQRXOT0a-5St2a6TV4qfhOMCVSetbQD0kGTrQ@mail.gmail.com","threadId":"41425","inReplyTo":"CAOLa=ZQO065j5VfJabbV6jww5Z2f3jbaRQDfDcG9NY4x2txrFQ@mail.gmail.com","subject":"Re: [PATCH v5 02/12] ref-filter: use strbuf_split_str_omit_term()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-17T17:39:38Z","receivedAt":"2016-02-17T17:39:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Feb 17, 2016 at 12:04 PM, Karthik Nayak wrote:\n> On Wed, Feb 17, 2016 at 2:39 AM, Eric Sunshine wrote:\n>> My initial reaction was negative due to the heavy review burden this\n>> series has demanded thus far, however, my mind was changing even as I\n>> composed the above response. [...]\n>>\n>> So, I think I'm fine with it, if Karthik is game.\n>\n> Sounds good to me.\n>\n> I just read the conversation between Jeff, Junio and You about the whitespace\n> counter-argument and I think its good to go ahead with v6 with Jeff's suggested\n> change.\n>\n> Since he's already pushed the changes on top of my changes to:\n> git://github.com/peff/git.git jk/tweaked-ref-filter\n> I'll just have a look and push that to the list as v6.\n\nI reviewed the entire series again, including Peff's changes, so this\nentire series is:\n\n Reviewed-by: Eric Sunshine \n\nKarthik, feel free to include my Reviewed-by: in all the patches\n(including Peff's) when you post v6.\n\nThanks.\n"},{"id":"278470","messageId":"xmqqio1njkex.fsf@gitster.mtv.corp.google.com","threadId":"41425","inReplyTo":"20160217003259.GD1187@sigill.intra.peff.net","subject":"Re: [PATCH v5 02/12] ref-filter: use strbuf_split_str_omit_term()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-17T17:50:30Z","receivedAt":"2016-02-17T17:50:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Feb 16, 2016 at 04:28:10PM -0800, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > On Tue, Feb 16, 2016 at 04:12:08PM -0800, Junio C Hamano wrote:\n>> >\n>> >> > To be honest, though, I am now on the fence, considering the possible\n>> >> > whitespace issue.\n>> >> \n>> >> Certainly not having to see s[0]->buf over and over is a huge win ;-).\n>> >> \n>> >> Is the \"whitespace issue\" a big deal? Does it involve more than a\n>> >> similar sibling to string_list_split() that trims the whitespace\n>> >> around the delimiter (or allows a regexp as a delimiter \"\\s*,\\s*\")?\n>> >\n>> > I think that solution would work (and IMHO would actually be preferable\n>> > to the split-then-trim that strbuf_split does). But it does mean writing\n>> > new code.\n>> \n>> True, but only when we decide to support trimming the whitespace,\n>> which can come later.\n>> \n>> I do not even know if it is wise to accept %(align:position=left, width=4)\n>> when %(align:position=left,width=4) would do the job just fine.\n>\n> Yeah, it was mostly just about being friendly to the user. But if nobody\n> is complaining, it may not even be worth worrying about.\n\nI was more worried about the possibility that we may have to support\nvalues with leading or trailing whitespaces in the future.\n\n 0. %(align:position=left,width=4)\n 1. %(align:position=left, width=4)\n 2. %(align: position =left, width =4)\n 3. %(align: position = left, width = 4)\n 4. %(align: position = left , width = 4)\n\nWe can probably accept 1. without ambiguity, and probably 2., too.\nThese examples are about keys with possible leading or trailing\nwhitespaces, and it is unlikely that we need to support them.\n\nTo those who do not think carefully themselves, however, going from\n1 & 2 that are handlable (and some might even argue that 1. is\neaslier to read) to 3 & 4 will not appear as a big syntax-breaking\nleap. But 3 & 4 are; these need disambiguation rule on the value\nside (we either introduce quoting, declare no values can have\nleading or trailing whitespaces, etc.).\n\nThat is why I suggested not to even accept 1. to avoid the slipperly\nslope.\n"},{"id":"278484","messageId":"CAOLa=ZQ4-Qwp84XgTS9joGW4rJRiw8VMTf+Y1Dzo5M-ZNPZXuA@mail.gmail.com","threadId":"41425","inReplyTo":"CAPig+cTrh4u7vgQRXOT0a-5St2a6TV4qfhOMCVSetbQD0kGTrQ@mail.gmail.com","subject":"Re: [PATCH v5 02/12] ref-filter: use strbuf_split_str_omit_term()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-02-17T18:07:44Z","receivedAt":"2016-02-17T18:07:44Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Feb 17, 2016 at 11:09 PM, Eric Sunshine wrote:\n> On Wed, Feb 17, 2016 at 12:04 PM, Karthik Nayak wrote:\n>> On Wed, Feb 17, 2016 at 2:39 AM, Eric Sunshine wrote:\n>>> My initial reaction was negative due to the heavy review burden this\n>>> series has demanded thus far, however, my mind was changing even as I\n>>> composed the above response. [...]\n>>>\n>>> So, I think I'm fine with it, if Karthik is game.\n>>\n>> Sounds good to me.\n>>\n>> I just read the conversation between Jeff, Junio and You about the whitespace\n>> counter-argument and I think its good to go ahead with v6 with Jeff's suggested\n>> change.\n>>\n>> Since he's already pushed the changes on top of my changes to:\n>> git://github.com/peff/git.git jk/tweaked-ref-filter\n>> I'll just have a look and push that to the list as v6.\n>\n> I reviewed the entire series again, including Peff's changes, so this\n> entire series is:\n>\n> Reviewed-by: Eric Sunshine \n>\n> Karthik, feel free to include my Reviewed-by: in all the patches\n> (including Peff's) when you post v6.\n>\n> Thanks.\n\nOops! I just pushed v6 before I even saw this mail.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"278485","messageId":"CAPig+cQzkQ2G2EbrkDXHq4eytywFbJKPO2kxFTOxrcACQJCZ6g@mail.gmail.com","threadId":"41425","inReplyTo":"CAOLa=ZQ4-Qwp84XgTS9joGW4rJRiw8VMTf+Y1Dzo5M-ZNPZXuA@mail.gmail.com","subject":"Re: [PATCH v5 02/12] ref-filter: use strbuf_split_str_omit_term()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-17T18:17:16Z","receivedAt":"2016-02-17T18:17:16Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Feb 17, 2016 at 1:07 PM, Karthik Nayak wrote:\n> On Wed, Feb 17, 2016 at 11:09 PM, Eric Sunshine wrote:\n>> I reviewed the entire series again, including Peff's changes, so this\n>> entire series is:\n>>\n>> Reviewed-by: Eric Sunshine \n>>\n>> Karthik, feel free to include my Reviewed-by: in all the patches\n>> (including Peff's) when you post v6.\n>\n> Oops! I just pushed v6 before I even saw this mail.\n\nNo problem. Junio can add my Reviewed-by: if he wants when he picks up\nthe series.\n"},{"id":"278486","messageId":"CAOLa=ZR83_79YTPA1QAB8wOPUDFu_RV2za4P4s9eO_PCgpXdNg@mail.gmail.com","threadId":"41425","inReplyTo":"CAPig+cQzkQ2G2EbrkDXHq4eytywFbJKPO2kxFTOxrcACQJCZ6g@mail.gmail.com","subject":"Re: [PATCH v5 02/12] ref-filter: use strbuf_split_str_omit_term()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-02-17T18:21:45Z","receivedAt":"2016-02-17T18:21:45Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Feb 17, 2016 at 11:47 PM, Eric Sunshine wrote:\n> On Wed, Feb 17, 2016 at 1:07 PM, Karthik Nayak wrote:\n>> On Wed, Feb 17, 2016 at 11:09 PM, Eric Sunshine wrote:\n>>> I reviewed the entire series again, including Peff's changes, so this\n>>> entire series is:\n>>>\n>>> Reviewed-by: Eric Sunshine \n>>>\n>>> Karthik, feel free to include my Reviewed-by: in all the patches\n>>> (including Peff's) when you post v6.\n>>\n>> Oops! I just pushed v6 before I even saw this mail.\n>\n> No problem. Junio can add my Reviewed-by: if he wants when he picks up\n> the series.\n\nThat would be great :) Thanks for reviewing this series.\n\n-- \nRegards,\nKarthik Nayak\n"}]}