{"thread":{"id":"41125","subject":"[PATCH v3 00/15] ref-filter: use parsing functions","startedAt":"2016-01-05T08:02:59Z","lastAt":"2016-01-26T11:34:56Z","messageCount":52,"participants":["Karthik Nayak","Junio C Hamano","Eric Sunshine","Christian Couder"],"isPatch":true,"patchVersion":3,"patchTotal":15},"messages":[{"id":"275349","messageId":"1451980994-26865-1-git-send-email-Karthik.188@gmail.com","threadId":"41125","inReplyTo":null,"subject":"[PATCH v3 00/15] ref-filter: use parsing functions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-05T08:02:59Z","receivedAt":"2016-01-05T08:02:59Z","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\n\nChanges in this version:\n* Change variable name in 'used_atom' from 'str' to 'name'.\n* Split introduction of 'used_atom' into [3/15], [4/15] and [5/15].\n* In [8/15] parse the color in color_atom_parser() itself and pass the\nvalue to 'v->s' in populate_value().\n* Introduce [11/15] which changes 'width' in align_atom_parser() from\nint to unsigned int.\n* Fix error in code in align_atom_parser(). and split it into [9/15],\n[10/15], [11/15] and [12/15].\n* Change 'align' atom tests to a table form.\n* Change 'no_lines' to 'nlines' in used_atom.\n* Switch to starts_with()/strcmp() instead of match_atom_name() as required.\n* Changes in commit message and comments.\n* Improve readability of code.\n\nEric suggested that I make match_atom_name() not return a value [0]. I\nhaven't done that as we use match_atom_name() in [14/15] for matching\n'subject' and 'body' in contents_atom_parser() and although Eric\nsuggested I use strcmp() instead, this would not work as we need to\ncheck for derefernced 'subject' and 'body' atoms.\n\n[0]: http://article.gmane.org/gmane.comp.version-control.git/282701\n\nKarthik Nayak (15):\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-fitler: bump match_atom() name to the top\n ref-filter: skip deref specifier in match_atom_name()\n ref-filter: introduce color_atom_parser()\n ref-filter: introduce align_atom_parser()\n ref-filter: introduce parse_align_position()\n ref-filter: convert variable 'width' to an unsigned int\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 | 455 ++++++++++++++++++++++---------------\n strbuf.c | 7 +-\n strbuf.h | 25 +-\n t/t6302-for-each-ref-filter.sh | 41 ++++\n 5 files changed, 349 insertions(+), 199 deletions(-)\n\n-- \n2.6.4\n"},{"id":"275351","messageId":"1451980994-26865-2-git-send-email-Karthik.188@gmail.com","threadId":"41125","inReplyTo":"1451980994-26865-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v3 01/15] strbuf: introduce strbuf_split_str_omit_term()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-05T08:03:00Z","receivedAt":"2016-01-05T08:03:00Z","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 | 7 ++++---\n strbuf.h | 25 ++++++++++++++++---------\n 2 files changed, 20 insertions(+), 12 deletions(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex b552a13..b62508e 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -115,7 +115,7 @@ void strbuf_tolower(struct strbuf *sb)\n }\n \n struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n-\t\t\t\t int terminator, int max)\n+\t\t\t\t int terminator, int max, int omit_term)\n {\n \tstruct strbuf **ret = NULL;\n \tsize_t nr = 0, alloc = 0;\n@@ -123,14 +123,15 @@ struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n \n \twhile (slen) {\n \t\tint len = slen;\n+\t\tconst char *end = NULL;\n \t\tif (max <= 0 || nr + 1 < max) {\n-\t\t\tconst char *end = memchr(str, terminator, slen);\n+\t\t\tend = memchr(str, terminator, slen);\n \t\t\tif (end)\n \t\t\t\tlen = end - str + 1;\n \t\t}\n \t\tt = xmalloc(sizeof(struct strbuf));\n \t\tstrbuf_init(t, len);\n-\t\tstrbuf_add(t, str, len);\n+\t\tstrbuf_add(t, str, len - !!end * !!omit_term);\n \t\tALLOC_GROW(ret, nr + 2, alloc);\n \t\tret[nr++] = t;\n \t\tstr += len;\ndiff --git a/strbuf.h b/strbuf.h\nindex 6ae7a72..1de816d 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -450,11 +450,12 @@ static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)\n /**\n * Split str (of length slen) at the specified terminator character.\n * Return a null-terminated array of pointers to strbuf objects\n- * holding the substrings. The substrings include the terminator,\n- * except for the last substring, which might be unterminated if the\n- * original string did not end with a terminator. If max is positive,\n- * then split the string into at most max substrings (with the last\n- * substring containing everything following the (max-1)th terminator\n+ * holding the substrings. 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@@ -465,19 +466,25 @@ static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)\n * For lighter-weight alternatives, see string_list_split() and\n * string_list_split_in_place().\n */\n-extern struct strbuf **strbuf_split_buf(const char *, size_t,\n-\t\t\t\t\tint terminator, int max);\n+extern struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n+\t\t\t\t\tint terminator, int max, int omit_term);\n+\n+static inline struct strbuf **strbuf_split_str_omit_term(const char *str,\n+\t\t\t\t\t\t\t int terminator, int max)\n+{\n+\treturn strbuf_split_buf(str, strlen(str), terminator, max, 1);\n+}\n \n static inline struct strbuf **strbuf_split_str(const char *str,\n \t\t\t\t\t int terminator, int max)\n {\n-\treturn strbuf_split_buf(str, strlen(str), terminator, max);\n+\treturn strbuf_split_buf(str, strlen(str), terminator, max, 0);\n }\n \n static inline struct strbuf **strbuf_split_max(const struct strbuf *sb,\n \t\t\t\t\t\tint terminator, int max)\n {\n-\treturn strbuf_split_buf(sb->buf, sb->len, terminator, max);\n+\treturn strbuf_split_buf(sb->buf, sb->len, terminator, max, 0);\n }\n \n static inline struct strbuf **strbuf_split(const struct strbuf *sb,\n-- \n2.6.4\n"},{"id":"275350","messageId":"1451980994-26865-3-git-send-email-Karthik.188@gmail.com","threadId":"41125","inReplyTo":"1451980994-26865-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v3 02/15] ref-filter: use strbuf_split_str_omit_term()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-05T08:03:01Z","receivedAt":"2016-01-05T08:03:01Z","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 e205dd2..6263710 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -869,18 +869,11 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\tif (!valp)\n \t\t\t\tdie(_(\"expected format: %%(align:,)\"));\n \n-\t\t\t/*\n-\t\t\t * TODO: Implement a function similar to strbuf_split_str()\n-\t\t\t * which would omit the separator from the end of each value.\n-\t\t\t */\n-\t\t\ts = to_free = strbuf_split_str(valp, ',', 0);\n+\t\t\ts = to_free = strbuf_split_str_omit_term(valp, ',', 0);\n \n \t\t\talign->position = ALIGN_LEFT;\n \n \t\t\twhile (*s) {\n-\t\t\t\t/* Strip trailing comma */\n-\t\t\t\tif (s[1])\n-\t\t\t\t\tstrbuf_setlen(s[0], s[0]->len - 1);\n \t\t\t\tif (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))\n \t\t\t\t\t;\n \t\t\t\telse if (!strcmp(s[0]->buf, \"left\"))\n-- \n2.6.4\n"},{"id":"275353","messageId":"1451980994-26865-4-git-send-email-Karthik.188@gmail.com","threadId":"41125","inReplyTo":"1451980994-26865-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v3 03/15] ref-filter: bump 'used_atom' and related code to the top","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-05T08:03:02Z","receivedAt":"2016-01-05T08:03:02Z","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---\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 6263710..388e2dd 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 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 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.6.4\n"},{"id":"275352","messageId":"1451980994-26865-5-git-send-email-Karthik.188@gmail.com","threadId":"41125","inReplyTo":"1451980994-26865-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v3 04/15] ref-filter: introduce struct used_atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-05T08:03:03Z","receivedAt":"2016-01-05T08:03:03Z","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 | 37 +++++++++++++++++++------------------\n 1 file changed, 19 insertions(+), 18 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 388e2dd..75ebd03 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -17,7 +17,7 @@\n typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n \n /*\n- * An atom is a valid field atom listed above, possibly prefixed with\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@@ -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@@ -786,7 +787,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@@ -1438,7 +1439,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n {\n \tstruct atom_value *va, *vb;\n \tint cmp;\n-\tcmp_type cmp_type = used_atom_type[s->atom];\n+\tcmp_type cmp_type = used_atom[s->atom].type;\n \n \tget_ref_atom_value(a, s->atom, &va);\n \tget_ref_atom_value(b, s->atom, &vb);\n-- \n2.6.4\n"},{"id":"275354","messageId":"1451980994-26865-6-git-send-email-Karthik.188@gmail.com","threadId":"41125","inReplyTo":"1451980994-26865-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v3 05/15] ref-filter: introduce parsing functions for each valid atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-05T08:03:04Z","receivedAt":"2016-01-05T08:03:04Z","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 \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 75ebd03..efa247a 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);\n } valid_atom[] = {\n \t{ \"refname\" },\n \t{ \"objecttype\" },\n@@ -154,6 +155,8 @@ 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 (valid_atom[i].parser)\n+\t\tvalid_atom[i].parser(&used_atom[at]);\n \tif (*atom == '*')\n \t\tneed_tagged = 1;\n \tif (!strcmp(used_atom[at].name, \"symref\"))\n-- \n2.6.4\n"},{"id":"275356","messageId":"1451980994-26865-7-git-send-email-Karthik.188@gmail.com","threadId":"41125","inReplyTo":"1451980994-26865-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v3 06/15] ref-fitler: bump match_atom() name to the top","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-05T08:03:05Z","receivedAt":"2016-01-05T08:03:05Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Bump match_atom() to the top for usage in further patches.\n\nHelped-by: Eric Sunshine \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 32 ++++++++++++++++----------------\n 1 file changed, 16 insertions(+), 16 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex efa247a..575d293 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -33,6 +33,22 @@ static struct used_atom {\n static int used_atom_cnt, need_tagged, need_symref;\n static int need_color_reset_at_eol;\n \n+static int match_atom_name(const char *name, const char *atom_name, const char **val)\n+{\n+\tconst char *body;\n+\n+\tif (!skip_prefix(name, atom_name, &body))\n+\t\treturn 0; /* doesn't even begin with \"atom_name\" */\n+\tif (!body[0]) {\n+\t\t*val = NULL; /* %(atom_name) and no customization */\n+\t\treturn 1;\n+\t}\n+\tif (body[0] != ':')\n+\t\treturn 0; /* \"atom_namefoo\" is not \"atom_name\" or \"atom_name:...\" */\n+\t*val = body + 1; /* \"atom_name:val\" */\n+\treturn 1;\n+}\n+\n static struct {\n \tconst char *name;\n \tcmp_type cmp_type;\n@@ -262,22 +278,6 @@ static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_sta\n \tpop_stack_element(&state->stack);\n }\n \n-static int match_atom_name(const char *name, const char *atom_name, const char **val)\n-{\n-\tconst char *body;\n-\n-\tif (!skip_prefix(name, atom_name, &body))\n-\t\treturn 0; /* doesn't even begin with \"atom_name\" */\n-\tif (!body[0]) {\n-\t\t*val = NULL; /* %(atom_name) and no customization */\n-\t\treturn 1;\n-\t}\n-\tif (body[0] != ':')\n-\t\treturn 0; /* \"atom_namefoo\" is not \"atom_name\" or \"atom_name:...\" */\n-\t*val = body + 1; /* \"atom_name:val\" */\n-\treturn 1;\n-}\n-\n /*\n * In a format string, find the next occurrence of %(atom).\n */\n-- \n2.6.4\n"},{"id":"275359","messageId":"1451980994-26865-8-git-send-email-Karthik.188@gmail.com","threadId":"41125","inReplyTo":"1451980994-26865-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v3 07/15] ref-filter: skip deref specifier in match_atom_name()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-05T08:03:06Z","receivedAt":"2016-01-05T08:03:06Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"In upcoming patches we make calls to match_atom_name() with the '*'\nderef specifier still attached to the atom name. Subsequent patches\nwill call match_atom_name() with the '*' deref specifier still\nattached to the atom name so, as a convenience, skip over it on their\nbehalf.\n\nHelped-by: Eric Sunshine \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 575d293..b54c872 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -37,6 +37,10 @@ static int match_atom_name(const char *name, const char *atom_name, const char *\n {\n \tconst char *body;\n \n+\t/* skip the deref specifier */\n+\tif (name[0] == '*')\n+\t\tname++;\n+\n \tif (!skip_prefix(name, atom_name, &body))\n \t\treturn 0; /* doesn't even begin with \"atom_name\" */\n \tif (!body[0]) {\n-- \n2.6.4\n"},{"id":"275355","messageId":"1451980994-26865-9-git-send-email-Karthik.188@gmail.com","threadId":"41125","inReplyTo":"1451980994-26865-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v3 08/15] ref-filter: introduce color_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-05T08:03:07Z","receivedAt":"2016-01-05T08:03:07Z","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 | 29 ++++++++++++++++++++---------\n 1 file changed, 20 insertions(+), 9 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex b54c872..9708d67 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -29,6 +29,9 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n static struct used_atom {\n \tconst char *name;\n \tcmp_type type;\n+\tunion {\n+\t\tchar *color;\n+\t} u;\n } *used_atom;\n static int used_atom_cnt, need_tagged, need_symref;\n static int need_color_reset_at_eol;\n@@ -53,6 +56,18 @@ static int match_atom_name(const char *name, const char *atom_name, const char *\n \treturn 1;\n }\n \n+static void color_atom_parser(struct used_atom *atom)\n+{\n+\tif (!match_atom_name(atom->name, \"color\", (const char **)&atom->u.color))\n+\t\tdie(\"BUG: parsing non-'color'\");\n+\tif (!atom->u.color)\n+\t\tdie(_(\"expected format: %%(color:)\"));\n+\t/* atom->u.color points to part of atom->name */\n+\tatom->u.color = xstrdup(atom->u.color);\n+\tif (color_parse(atom->u.color, atom->u.color) < 0)\n+\t\tdie(_(\"invalid color value: %s\"), atom->u.color);\n+}\n+\n static struct {\n \tconst char *name;\n \tcmp_type cmp_type;\n@@ -90,7 +105,7 @@ static struct {\n \t{ \"symref\" },\n \t{ \"flag\" },\n \t{ \"HEAD\" },\n-\t{ \"color\" },\n+\t{ \"color\", FIELD_STR, color_atom_parser },\n \t{ \"align\" },\n \t{ \"end\" },\n };\n@@ -175,6 +190,7 @@ 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+\tmemset(&used_atom[at].u, 0, sizeof(used_atom[at].u));\n \tif (valid_atom[i].parser)\n \t\tvalid_atom[i].parser(&used_atom[at]);\n \tif (*atom == '*')\n@@ -794,6 +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\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@@ -834,14 +851,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.6.4\n"},{"id":"275362","messageId":"1451980994-26865-10-git-send-email-Karthik.188@gmail.com","threadId":"41125","inReplyTo":"1451980994-26865-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v3 09/15] ref-filter: introduce align_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-05T08:03:08Z","receivedAt":"2016-01-05T08:03:08Z","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\nHelped-by: Eric Sunshine \nHelped-by: Ramsay Jones \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 83 +++++++++++++++++++++++++++++++++---------------------------\n 1 file changed, 46 insertions(+), 37 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 9708d67..fa081a8 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;\n+\t\tstruct align align;\n \t} u;\n } *used_atom;\n static int used_atom_cnt, need_tagged, need_symref;\n@@ -68,6 +74,43 @@ static void color_atom_parser(struct used_atom *atom)\n \t\tdie(_(\"invalid color value: %s\"), atom->u.color);\n }\n \n+static void align_atom_parser(struct used_atom *atom)\n+{\n+\tstruct align *align = &atom->u.align;\n+\tconst char *buf = NULL;\n+\tstruct strbuf **s, **to_free;\n+\tint width = -1;\n+\n+\tif (!match_atom_name(atom->name, \"align\", &buf))\n+\t\tdie(\"BUG: parsing non-'align'\");\n+\tif (!buf)\n+\t\tdie(_(\"expected format: %%(align:,)\"));\n+\ts = to_free = strbuf_split_str_omit_term(buf, ',', 0);\n+\n+\talign->position = ALIGN_LEFT;\n+\n+\twhile (*s) {\n+\t\tbuf = s[0]->buf;\n+\n+\t\tif (!strtoul_ui(buf, 10, (unsigned int *)&width))\n+\t\t\t;\n+\t\telse if (!strcmp(buf, \"left\"))\n+\t\t\talign->position = ALIGN_LEFT;\n+\t\telse if (!strcmp(buf, \"right\"))\n+\t\t\talign->position = ALIGN_RIGHT;\n+\t\telse if (!strcmp(buf, \"middle\"))\n+\t\t\talign->position = ALIGN_MIDDLE;\n+\t\telse\n+\t\t\tdie(_(\"unrecognized %%(align) argument: %s\"), buf);\n+\t\ts++;\n+\t}\n+\n+\tif (width < 0)\n+\t\tdie(_(\"positive width expected with the %%(align) atom\"));\n+\talign->width = width;\n+\tstrbuf_list_free(to_free);\n+}\n+\n static struct {\n \tconst char *name;\n \tcmp_type cmp_type;\n@@ -106,17 +149,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@@ -816,7 +854,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@@ -880,36 +917,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\tif (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))\n-\t\t\t\t\t;\n-\t\t\t\telse if (!strcmp(s[0]->buf, \"left\"))\n-\t\t\t\t\talign->position = ALIGN_LEFT;\n-\t\t\t\telse if (!strcmp(s[0]->buf, \"right\"))\n-\t\t\t\t\talign->position = ALIGN_RIGHT;\n-\t\t\t\telse if (!strcmp(s[0]->buf, \"middle\"))\n-\t\t\t\t\talign->position = ALIGN_MIDDLE;\n-\t\t\t\telse\n-\t\t\t\t\tdie(_(\"improper format entered align:%s\"), s[0]->buf);\n-\t\t\t\ts++;\n-\t\t\t}\n-\n-\t\t\tif (width < 0)\n-\t\t\t\tdie(_(\"positive width expected with the %%(align) atom\"));\n-\t\t\talign->width = width;\n-\t\t\tstrbuf_list_free(to_free);\n+\t\t} 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.6.4\n"},{"id":"275357","messageId":"1451980994-26865-11-git-send-email-Karthik.188@gmail.com","threadId":"41125","inReplyTo":"1451980994-26865-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v3 10/15] ref-filter: introduce parse_align_position()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-05T08:03:09Z","receivedAt":"2016-01-05T08:03:09Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":">From align_atom_parser() form 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 | 20 ++++++++++++++------\n 1 file changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex fa081a8..ccad4c3 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -74,6 +74,17 @@ static void color_atom_parser(struct used_atom *atom)\n \t\tdie(_(\"invalid color value: %s\"), atom->u.color);\n }\n \n+static align_type parse_align_position(const char *s)\n+{\n+\tif (!strcmp(s, \"right\"))\n+\t\treturn ALIGN_RIGHT;\n+\telse if (!strcmp(s, \"middle\"))\n+\t\treturn ALIGN_MIDDLE;\n+\telse if (!strcmp(s, \"left\"))\n+\t\treturn ALIGN_LEFT;\n+\treturn -1;\n+}\n+\n static void align_atom_parser(struct used_atom *atom)\n {\n \tstruct align *align = &atom->u.align;\n@@ -90,16 +101,13 @@ static void align_atom_parser(struct used_atom *atom)\n \talign->position = ALIGN_LEFT;\n \n \twhile (*s) {\n+\t\tint position;\n \t\tbuf = s[0]->buf;\n \n \t\tif (!strtoul_ui(buf, 10, (unsigned int *)&width))\n \t\t\t;\n-\t\telse if (!strcmp(buf, \"left\"))\n-\t\t\talign->position = ALIGN_LEFT;\n-\t\telse if (!strcmp(buf, \"right\"))\n-\t\t\talign->position = ALIGN_RIGHT;\n-\t\telse if (!strcmp(buf, \"middle\"))\n-\t\t\talign->position = ALIGN_MIDDLE;\n+\t\telse if ((position = parse_align_position(buf)) >= 0)\n+\t\t\talign->position = position;\n \t\telse\n \t\t\tdie(_(\"unrecognized %%(align) argument: %s\"), buf);\n \t\ts++;\n-- \n2.6.4\n"},{"id":"275358","messageId":"1451980994-26865-12-git-send-email-Karthik.188@gmail.com","threadId":"41125","inReplyTo":"1451980994-26865-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v3 11/15] ref-filter: convert variable 'width' to an unsigned int","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-05T08:03:10Z","receivedAt":"2016-01-05T08:03:10Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"In align_atom_parser() the 'width' variable is an int, which requires\nan explicit type conversion when used in strtoul_ui(). Hence make it\nan unsigned int.\n\nHelped-by: Eric Sunshine \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex ccad4c3..4f623a0 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -90,7 +90,7 @@ static void align_atom_parser(struct used_atom *atom)\n \tstruct align *align = &atom->u.align;\n \tconst char *buf = NULL;\n \tstruct strbuf **s, **to_free;\n-\tint width = -1;\n+\tunsigned int width = ~0U;\n \n \tif (!match_atom_name(atom->name, \"align\", &buf))\n \t\tdie(\"BUG: parsing non-'align'\");\n@@ -104,7 +104,7 @@ static void align_atom_parser(struct used_atom *atom)\n \t\tint position;\n \t\tbuf = s[0]->buf;\n \n-\t\tif (!strtoul_ui(buf, 10, (unsigned int *)&width))\n+\t\tif (!strtoul_ui(buf, 10, &width))\n \t\t\t;\n \t\telse if ((position = parse_align_position(buf)) >= 0)\n \t\t\talign->position = position;\n@@ -113,7 +113,7 @@ static void align_atom_parser(struct used_atom *atom)\n \t\ts++;\n \t}\n \n-\tif (width < 0)\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-- \n2.6.4\n"},{"id":"275364","messageId":"1451980994-26865-13-git-send-email-Karthik.188@gmail.com","threadId":"41125","inReplyTo":"1451980994-26865-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v3 12/15] ref-filter: align: introduce long-form syntax","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-05T08:03:11Z","receivedAt":"2016-01-05T08:03:11Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Introduce optional prefixes \"width=\" and \"position=\" for the align atom\nso that the atom can be used as \"%(align:width=,position=)\".\n\nAdd Documetation and tests for the same.\n\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 | 41 ++++++++++++++++++++++++++++++++++++++\n 3 files changed, 62 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex c6f073c..9af0f53 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -129,14 +129,18 @@ color::\n \n align::\n \tLeft-, middle-, or right-align the content between\n-\t%(align:...) and %(end). The \"align:\" is followed by ``\n-\tand `` in any order separated by a comma, where the\n-\t`` is either left, right or middle, default being\n-\tleft and `` is the total length of the content with\n-\talignment. If the contents length is more than the width then\n-\tno alignment is performed. If used with '--quote' everything\n-\tin between %(align:...) and %(end) is quoted, but if nested\n-\tthen only the topmost level performs quoting.\n+\t%(align:...) and %(end). The \"align:\" is followed by\n+\t`width=` and `position=` in any order\n+\tseparated by a comma, where the `` is either left,\n+\tright or middle, default being left and `` is the total\n+\tlength of the content with alignment. For brevity, the\n+\t\"width=\" and/or \"position=\" prefixes may be omitted, and bare\n+\t and used instead. For instance,\n+\t`%(align:,)`. If the contents length is more\n+\tthan the width then no alignment is performed. If used with\n+\t'--quote' everything in between %(align:...) and %(end) is\n+\tquoted, but if nested then only the topmost level performs\n+\tquoting.\n \n In addition to the above, for commit and tag objects, the header\n field names (`tree`, `parent`, `object`, `type`, and `tag`) can\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 4f623a0..df0b114 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -104,7 +104,15 @@ static void align_atom_parser(struct used_atom *atom)\n \t\tint position;\n \t\tbuf = s[0]->buf;\n \n-\t\tif (!strtoul_ui(buf, 10, &width))\n+\t\tif (skip_prefix(buf, \"position=\", &buf)) {\n+\t\t\tposition = parse_align_position(buf);\n+\t\t\tif (position < 0)\n+\t\t\t\tdie(_(\"unrecognized position:%s\"), buf);\n+\t\t\talign->position = position;\n+\t\t} else if (skip_prefix(buf, \"width=\", &buf)) {\n+\t\t\tif (strtoul_ui(buf, 10, &width))\n+\t\t\t\tdie(_(\"unrecognized width:%s\"), buf);\n+\t\t} else if (!strtoul_ui(buf, 10, &width))\n \t\t\t;\n \t\telse if ((position = parse_align_position(buf)) >= 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..0c4417f 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -133,6 +133,47 @@ 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; do\n+\t\ttest_expect_success 'align permutations' '\n+\t\tgit for-each-ref --format=\"|%(align:$option)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\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.6.4\n"},{"id":"275363","messageId":"1451980994-26865-14-git-send-email-Karthik.188@gmail.com","threadId":"41125","inReplyTo":"1451980994-26865-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v3 13/15] ref-filter: introduce remote_ref_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-05T08:03:12Z","receivedAt":"2016-01-05T08:03:12Z","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 | 107 ++++++++++++++++++++++++++++++++++++-----------------------\n 1 file changed, 66 insertions(+), 41 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex df0b114..32b4674 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;\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@@ -74,6 +76,26 @@ static void color_atom_parser(struct used_atom *atom)\n \t\tdie(_(\"invalid color value: %s\"), atom->u.color);\n }\n \n+static void remote_ref_atom_parser(struct used_atom *atom)\n+{\n+\tconst char *buf;\n+\n+\tbuf = strchr(atom->name, ':');\n+\tif (!buf) {\n+\t\tatom->u.remote_ref = RR_NORMAL;\n+\t\treturn;\n+\t}\n+\tbuf++;\n+\tif (!strcmp(buf, \"short\"))\n+\t\tatom->u.remote_ref = RR_SHORTEN;\n+\telse if (!strcmp(buf, \"track\"))\n+\t\tatom->u.remote_ref = RR_TRACK;\n+\telse if (!strcmp(buf, \"trackshort\"))\n+\t\tatom->u.remote_ref = RR_TRACKSHORT;\n+\telse\n+\t\tdie(_(\"unrecognized format: %%(%s)\"), atom->name);\n+}\n+\n static align_type parse_align_position(const char *s)\n {\n \tif (!strcmp(s, \"right\"))\n@@ -159,8 +181,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@@ -841,6 +863,43 @@ static inline char *copy_advance(char *dst, const char *src)\n \treturn dst;\n }\n \n+static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n+\t\t\t\t struct branch *branch, const char **s)\n+{\n+\tint num_ours, num_theirs;\n+\tif (atom->u.remote_ref == RR_SHORTEN)\n+\t\t*s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);\n+\telse if (atom->u.remote_ref == RR_TRACK) {\n+\t\tif (stat_tracking_info(branch, &num_ours,\n+\t\t\t\t &num_theirs, NULL))\n+\t\t\treturn;\n+\n+\t\tif (!num_ours && !num_theirs)\n+\t\t\t*s = \"\";\n+\t\telse if (!num_ours)\n+\t\t\t*s = xstrfmt(\"[behind %d]\", num_theirs);\n+\t\telse if (!num_theirs)\n+\t\t\t*s = xstrfmt(\"[ahead %d]\", num_ours);\n+\t\telse\n+\t\t\t*s = xstrfmt(\"[ahead %d, behind %d]\",\n+\t\t\t\t num_ours, num_theirs);\n+\t} else if (atom->u.remote_ref == RR_TRACKSHORT) {\n+\t\tif (stat_tracking_info(branch, &num_ours,\n+\t\t\t\t &num_theirs, NULL))\n+\t\t\treturn;\n+\n+\t\tif (!num_ours && !num_theirs)\n+\t\t\t*s = \"=\";\n+\t\telse if (!num_ours)\n+\t\t\t*s = \"<\";\n+\t\telse if (!num_theirs)\n+\t\t\t*s = \">\";\n+\t\telse\n+\t\t\t*s = \"<>\";\n+\t} else if (atom->u.remote_ref == RR_NORMAL)\n+\t\t*s = refname;\n+}\n+\n /*\n * Parse the object referred by ref, and grab needed value.\n */\n@@ -894,6 +953,8 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\trefname = branch_get_upstream(branch, NULL);\n \t\t\tif (!refname)\n \t\t\t\tcontinue;\n+\t\t\tfill_remote_ref_details(atom, refname, branch, &v->s);\n+\t\t\tcontinue;\n \t\t} else if (starts_with(name, \"push\")) {\n \t\t\tconst char *branch_name;\n \t\t\tif (!skip_prefix(ref->refname, \"refs/heads/\",\n@@ -904,6 +965,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@@ -945,49 +1008,11 @@ static void populate_value(struct ref_array_item *ref)\n \n \t\tformatp = strchr(name, ':');\n \t\tif (formatp) {\n-\t\t\tint num_ours, num_theirs;\n-\n \t\t\tformatp++;\n \t\t\tif (!strcmp(formatp, \"short\"))\n \t\t\t\trefname = shorten_unambiguous_ref(refname,\n \t\t\t\t\t\t warn_ambiguous_refs);\n-\t\t\telse if (!strcmp(formatp, \"track\") &&\n-\t\t\t\t (starts_with(name, \"upstream\") ||\n-\t\t\t\t starts_with(name, \"push\"))) {\n-\n-\t\t\t\tif (stat_tracking_info(branch, &num_ours,\n-\t\t\t\t\t\t &num_theirs, NULL))\n-\t\t\t\t\tcontinue;\n-\n-\t\t\t\tif (!num_ours && !num_theirs)\n-\t\t\t\t\tv->s = \"\";\n-\t\t\t\telse if (!num_ours)\n-\t\t\t\t\tv->s = xstrfmt(\"[behind %d]\", num_theirs);\n-\t\t\t\telse if (!num_theirs)\n-\t\t\t\t\tv->s = xstrfmt(\"[ahead %d]\", num_ours);\n-\t\t\t\telse\n-\t\t\t\t\tv->s = xstrfmt(\"[ahead %d, behind %d]\",\n-\t\t\t\t\t\t num_ours, num_theirs);\n-\t\t\t\tcontinue;\n-\t\t\t} else if (!strcmp(formatp, \"trackshort\") &&\n-\t\t\t\t (starts_with(name, \"upstream\") ||\n-\t\t\t\t starts_with(name, \"push\"))) {\n-\t\t\t\tassert(branch);\n-\n-\t\t\t\tif (stat_tracking_info(branch, &num_ours,\n-\t\t\t\t\t\t\t&num_theirs, NULL))\n-\t\t\t\t\tcontinue;\n-\n-\t\t\t\tif (!num_ours && !num_theirs)\n-\t\t\t\t\tv->s = \"=\";\n-\t\t\t\telse if (!num_ours)\n-\t\t\t\t\tv->s = \"<\";\n-\t\t\t\telse if (!num_theirs)\n-\t\t\t\t\tv->s = \">\";\n-\t\t\t\telse\n-\t\t\t\t\tv->s = \"<>\";\n-\t\t\t\tcontinue;\n-\t\t\t} else\n+\t\t\telse\n \t\t\t\tdie(\"unknown %.*s format %s\",\n \t\t\t\t (int)(formatp - name), name, formatp);\n \t\t}\n-- \n2.6.4\n"},{"id":"275361","messageId":"1451980994-26865-15-git-send-email-Karthik.188@gmail.com","threadId":"41125","inReplyTo":"1451980994-26865-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v3 14/15] ref-filter: introduce contents_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-05T08:03:13Z","receivedAt":"2016-01-05T08:03:13Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Introduce contents_atom_parser() which will parse the '%(contents)'\natom and store information into the 'used_atom' structure based on the\nmodifiers used along with the atom.\n\nHelped-by: Ramsay Jones \nHelped-by: Eric Sunshine \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 74 +++++++++++++++++++++++++++++++++++++-----------------------\n 1 file changed, 46 insertions(+), 28 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 32b4674..9e61530 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@@ -96,6 +100,35 @@ static void remote_ref_atom_parser(struct used_atom *atom)\n \t\tdie(_(\"unrecognized format: %%(%s)\"), atom->name);\n }\n \n+static void contents_atom_parser(struct used_atom *atom)\n+{\n+\tconst char * buf;\n+\n+\tif (match_atom_name(atom->name, \"subject\", &buf) && !buf) {\n+\t\tatom->u.contents.option = C_SUB;\n+\t\treturn;\n+\t} else if (match_atom_name(atom->name, \"body\", &buf) && !buf) {\n+\t\tatom->u.contents.option = C_BODY_DEP;\n+\t\treturn;\n+\t} if (!match_atom_name(atom->name, \"contents\", &buf))\n+\t\t die(\"BUG: parsing non-'contents'\");\n+\n+\tif (!buf)\n+\t\tatom->u.contents.option = C_BARE;\n+\telse if (!strcmp(buf, \"body\"))\n+\t\tatom->u.contents.option = C_BODY;\n+\telse if (!strcmp(buf, \"signature\"))\n+\t\tatom->u.contents.option = C_SIG;\n+\telse if (!strcmp(buf, \"subject\"))\n+\t\tatom->u.contents.option = C_SUB;\n+\telse if (skip_prefix(buf, \"lines=\", &buf)) {\n+\t\tatom->u.contents.option = C_LINES;\n+\t\tif (strtoul_ui(buf, 10, &atom->u.contents.nlines))\n+\t\t\tdie(_(\"positive value expected contents:lines=%s\"), buf);\n+\t} else\n+\t\tdie(_(\"unrecognized %%(contents) argument: %s\"), buf);\n+}\n+\n static align_type parse_align_position(const char *s)\n {\n \tif (!strcmp(s, \"right\"))\n@@ -178,9 +211,9 @@ static struct {\n \t{ \"taggerdate\", FIELD_TIME },\n \t{ \"creator\" },\n \t{ \"creatordate\", FIELD_TIME },\n-\t{ \"subject\" },\n-\t{ \"body\" },\n-\t{ \"contents\" },\n+\t{ \"subject\", FIELD_STR, contents_atom_parser },\n+\t{ \"body\", FIELD_STR, contents_atom_parser },\n+\t{ \"contents\", FIELD_STR, contents_atom_parser },\n \t{ \"upstream\", FIELD_STR, remote_ref_atom_parser },\n \t{ \"push\", FIELD_STR, remote_ref_atom_parser },\n \t{ \"symref\" },\n@@ -193,11 +226,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@@ -214,7 +242,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@@ -764,19 +791,15 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj\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\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@@ -784,28 +807,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.6.4\n"},{"id":"275360","messageId":"1451980994-26865-16-git-send-email-Karthik.188@gmail.com","threadId":"41125","inReplyTo":"1451980994-26865-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH v3 15/15] ref-filter: introduce objectname_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-05T08:03:14Z","receivedAt":"2016-01-05T08:03:14Z","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 | 39 ++++++++++++++++++++++++++++-----------\n 1 file changed, 28 insertions(+), 11 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 9e61530..57b4280 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@@ -129,6 +130,20 @@ static void contents_atom_parser(struct used_atom *atom)\n \t\tdie(_(\"unrecognized %%(contents) argument: %s\"), buf);\n }\n \n+static void objectname_atom_parser(struct used_atom *atom)\n+{\n+\tconst char * buf;\n+\n+\tif (!match_atom_name(atom->name, \"objectname\", &buf))\n+\t\tdie(\"BUG: parsing non-'objectname'\");\n+\tif (!buf)\n+\t\tatom->u.objectname = O_FULL;\n+\telse if (!strcmp(buf, \"short\"))\n+\t\tatom->u.objectname = O_SHORT;\n+\telse\n+\t\tdie(_(\"unrecognized %%(objectname) argument: %s\"), buf);\n+}\n+\n static align_type parse_align_position(const char *s)\n {\n \tif (!strcmp(s, \"right\"))\n@@ -190,7 +205,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@@ -467,15 +482,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@@ -499,7 +516,7 @@ static void grab_common_values(struct atom_value *val, int deref, struct object\n \t\t\tv->s = xstrfmt(\"%lu\", sz);\n \t\t}\n \t\telse if (deref)\n-\t\t\tgrab_objectname(name, obj->sha1, v);\n+\t\t\tgrab_objectname(name, obj->sha1, v, &used_atom[i]);\n \t}\n }\n \n@@ -1001,7 +1018,7 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\t\tv->s = xstrdup(buf + 1);\n \t\t\t}\n \t\t\tcontinue;\n-\t\t} else if (!deref && grab_objectname(name, ref->objectname, v)) {\n+\t\t} else if (!deref && grab_objectname(name, ref->objectname, v, atom)) {\n \t\t\tcontinue;\n \t\t} else if (!strcmp(name, \"HEAD\")) {\n \t\t\tconst char *head;\n-- \n2.6.4\n"},{"id":"275392","messageId":"xmqq7fjnkexv.fsf@gitster.mtv.corp.google.com","threadId":"41125","inReplyTo":"1451980994-26865-2-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v3 01/15] strbuf: introduce strbuf_split_str_omit_term()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-05T19:24:12Z","receivedAt":"2016-01-05T19:24:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> The current implementation of 'strbuf_split_buf()' includes the\n> terminator at the end of each strbuf post splitting. Add an option\n> wherein we can drop the terminator if desired. In this context\n> introduce a wrapper function 'strbuf_split_str_omit_term()' which\n> splits a given string into strbufs without including the terminator.\n>\n> Helped-by: Eric Sunshine \n> Signed-off-by: Karthik Nayak \n> ---\n> strbuf.c | 7 ++++---\n> strbuf.h | 25 ++++++++++++++++---------\n> 2 files changed, 20 insertions(+), 12 deletions(-)\n>\n> diff --git a/strbuf.c b/strbuf.c\n> index b552a13..b62508e 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -115,7 +115,7 @@ void strbuf_tolower(struct strbuf *sb)\n> }\n> \n> struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n> -\t\t\t\t int terminator, int max)\n> +\t\t\t\t int terminator, int max, int omit_term)\n> {\n> \tstruct strbuf **ret = NULL;\n> \tsize_t nr = 0, alloc = 0;\n> @@ -123,14 +123,15 @@ struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n> \n> \twhile (slen) {\n> \t\tint len = slen;\n> +\t\tconst char *end = NULL;\n> \t\tif (max <= 0 || nr + 1 < max) {\n> -\t\t\tconst char *end = memchr(str, terminator, slen);\n> +\t\t\tend = memchr(str, terminator, slen);\n> \t\t\tif (end)\n> \t\t\t\tlen = end - str + 1;\n> \t\t}\n> \t\tt = xmalloc(sizeof(struct strbuf));\n> \t\tstrbuf_init(t, len);\n> -\t\tstrbuf_add(t, str, len);\n> +\t\tstrbuf_add(t, str, len - !!end * !!omit_term);\n\nYou initialize with \"len\" but sometimes copy less than that, which\nlooks somewhat sloppy.\n\nMaybe I am old-fashioned, but use of a multiplication when you do\nnot mean to numerically multiply but merely to perform a logical\noperation made me go \"Huh?\".\n\nPerhaps using another variable would make it easier to follow?\nEither using a boolean that tells us that the terminating byte\nis to be omitted, i.e.\n\n\tint len = slen;\n int omit = 0;\n if ( ... we are still splitting ... ) {\n\t\tconst char *end = memchr(...);\n if (end) {\n \tlen = end - str + 1;\n\t\t\tomit = !!omit_term;\n\t\t}\n\t}\n\tstrbuf_init(t, len - omit);\n strbuf_add(t, str, len - omit);\n\nor an integer \"copylen\" that tells us how many bytes to copy, which\noften is the same as \"len\" but sometimes different by 1 byte?\n"},{"id":"275397","messageId":"xmqqwprniwfq.fsf@gitster.mtv.corp.google.com","threadId":"41125","inReplyTo":"1451980994-26865-9-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v3 08/15] ref-filter: introduce color_atom_parser()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-05T20:49:13Z","receivedAt":"2016-01-05T20:49:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> Introduce color_atom_parser() which will parse a \"color\" atom and\n> store its color in the \"used_atom\" structure for further usage in\n> populate_value().\n>\n> Helped-by: Ramsay Jones \n> Helped-by: Eric Sunshine \n> Signed-off-by: Karthik Nayak \n> ---\n> ref-filter.c | 29 ++++++++++++++++++++---------\n> 1 file changed, 20 insertions(+), 9 deletions(-)\n>\n> diff --git a/ref-filter.c b/ref-filter.c\n> index b54c872..9708d67 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -29,6 +29,9 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n> static struct used_atom {\n> \tconst char *name;\n> \tcmp_type type;\n> +\tunion {\n> +\t\tchar *color;\n> +\t} u;\n> } *used_atom;\n> static int used_atom_cnt, need_tagged, need_symref;\n> static int need_color_reset_at_eol;\n> @@ -53,6 +56,18 @@ static int match_atom_name(const char *name, const char *atom_name, const char *\n> \treturn 1;\n> }\n> \n> +static void color_atom_parser(struct used_atom *atom)\n> +{\n> +\tif (!match_atom_name(atom->name, \"color\", (const char **)&atom->u.color))\n> +\t\tdie(\"BUG: parsing non-'color'\");\n> +\tif (!atom->u.color)\n> +\t\tdie(_(\"expected format: %%(color:)\"));\n> +\t/* atom->u.color points to part of atom->name */\n> +\tatom->u.color = xstrdup(atom->u.color);\n> +\tif (color_parse(atom->u.color, atom->u.color) < 0)\n> +\t\tdie(_(\"invalid color value: %s\"), atom->u.color);\n\nIs this calling color_parse() from color.c?\n\nThe function wants the destination to be at least COLOR_MAXLEN, but\nI do not see where the piece memory pointed by atom->u.color is\nguaranteed to be that long in the new code. Looking at the code\nremoved by this patch, it used to correctly use a buffer that is\nCOLOR_MAXLEN bytes long. So...\n\n\tconst char *color_value;\n\n\tif (!match_atom_name(atom->name, \"color\", color_value))\n\t\tdie(\"BUG: parsing non-'color'\");\n\tif (!color_value)\n\t\tdie(_(\"expected format: %%(color:)\"));\n\tatom->u.color = xmalloc(COLOR_MAXLEN);\n if (color_parse(color_value, atom->u.color) < 0)\n\t\tdie(_(\"invalid color value: %s\"), color_value);\n\nor even define it in the union, i.e.\n\n\tunion {\n \tchar color[COLOR_MAXLEN];\n\t} u;\n\nand then use atom->u.color[] in-place?\n"},{"id":"275398","messageId":"xmqqr3hvivml.fsf@gitster.mtv.corp.google.com","threadId":"41125","inReplyTo":"1451980994-26865-9-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v3 08/15] ref-filter: introduce color_atom_parser()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-05T21:06:42Z","receivedAt":"2016-01-05T21:06:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> @@ -90,7 +105,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\nThis is minor, as I do not think anybody sane would say\n\"for-each-ref --sort=color\" and expect anything sensible, but\n\nI think we shouldn't mark these bogus attributes [*1*] as FIELD_STR\n(and it is not FIELD_ULONG, either). Perhaps add a new FIELD_BOGUS\n(or some better name to make it clear that this is not a \"value\"\nthat belongs to the ref and can be used to sort, e.g. \"FAKE\") value\nand mark them as such?\n\nThat would allow us to error out when they are specified as a\nsorting criteria.\n\n\n[Footnote]\n\n*1* Bogus from the point of view of \"what are the various attributes\nspecific to these items that the command is iterating over,\ni.e. refs (or the objects at the tips of these refs)\", as color,\nalign, etc. are not attributes per refs.\n"},{"id":"275400","messageId":"xmqqmvsjivcv.fsf@gitster.mtv.corp.google.com","threadId":"41125","inReplyTo":"1451980994-26865-12-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v3 11/15] ref-filter: convert variable 'width' to an unsigned int","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-05T21:12:32Z","receivedAt":"2016-01-05T21:12:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> In align_atom_parser() the 'width' variable is an int, which requires\n> an explicit type conversion when used in strtoul_ui(). Hence make it\n> an unsigned int.\n>\n> Helped-by: Eric Sunshine \n> Signed-off-by: Karthik Nayak \n> ---\n\nShouldn't this be done in [09/15] from the beginning? As it's not\nlike [09/15] was a pure code movement, this step looks like \"Oops,\nI screwed up earlier, and here is a fixup\".\n\nIf we want to do this as a separate step, it would be easier to\nfollow if it is done _before_ 09/15 to the original of the code\nmovement, I think.\n\n> ref-filter.c | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/ref-filter.c b/ref-filter.c\n> index ccad4c3..4f623a0 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -90,7 +90,7 @@ static void align_atom_parser(struct used_atom *atom)\n> \tstruct align *align = &atom->u.align;\n> \tconst char *buf = NULL;\n> \tstruct strbuf **s, **to_free;\n> -\tint width = -1;\n> +\tunsigned int width = ~0U;\n> \n> \tif (!match_atom_name(atom->name, \"align\", &buf))\n> \t\tdie(\"BUG: parsing non-'align'\");\n> @@ -104,7 +104,7 @@ static void align_atom_parser(struct used_atom *atom)\n> \t\tint position;\n> \t\tbuf = s[0]->buf;\n> \n> -\t\tif (!strtoul_ui(buf, 10, (unsigned int *)&width))\n> +\t\tif (!strtoul_ui(buf, 10, &width))\n> \t\t\t;\n> \t\telse if ((position = parse_align_position(buf)) >= 0)\n> \t\t\talign->position = position;\n> @@ -113,7 +113,7 @@ static void align_atom_parser(struct used_atom *atom)\n> \t\ts++;\n> \t}\n> \n> -\tif (width < 0)\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"},{"id":"275402","messageId":"xmqqio37iuvj.fsf@gitster.mtv.corp.google.com","threadId":"41125","inReplyTo":"1451980994-26865-15-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v3 14/15] ref-filter: introduce contents_atom_parser()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-05T21:22:56Z","receivedAt":"2016-01-05T21:22:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> Introduce contents_atom_parser() which will parse the '%(contents)'\n> atom and store information into the 'used_atom' structure based on the\n> modifiers used along with the atom.\n>\n> Helped-by: Ramsay Jones \n> Helped-by: Eric Sunshine \n> Signed-off-by: Karthik Nayak \n> ---\n> ref-filter.c | 74 +++++++++++++++++++++++++++++++++++++-----------------------\n> 1 file changed, 46 insertions(+), 28 deletions(-)\n>\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 32b4674..9e61530 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> @@ -96,6 +100,35 @@ static void remote_ref_atom_parser(struct used_atom *atom)\n> \t\tdie(_(\"unrecognized format: %%(%s)\"), atom->name);\n> }\n> \n> +static void contents_atom_parser(struct used_atom *atom)\n> +{\n> +\tconst char * buf;\n> +\n> +\tif (match_atom_name(atom->name, \"subject\", &buf) && !buf) {\n> +\t\tatom->u.contents.option = C_SUB;\n> +\t\treturn;\n> +\t} else if (match_atom_name(atom->name, \"body\", &buf) && !buf) {\n> +\t\tatom->u.contents.option = C_BODY_DEP;\n> +\t\treturn;\n> +\t} if (!match_atom_name(atom->name, \"contents\", &buf))\n> +\t\t die(\"BUG: parsing non-'contents'\");\n\nDid you really intend to say \"if\" here, not \"else if\"?\n\nI also wonder if the \"&& !buf\" in the first two are correct. What\nshould happen to \"%(subject:foo)\", which gives you a non-empty buf?\nIt may or may not be an error, but it should not fall thru and cause\n\"BUG:parsing non-contents\", should it?\n\n> +\tif (!buf)\n> +\t\tatom->u.contents.option = C_BARE;\n> +\telse if (!strcmp(buf, \"body\"))\n> +\t\tatom->u.contents.option = C_BODY;\n> +...\n"},{"id":"275403","messageId":"xmqqegdviugx.fsf@gitster.mtv.corp.google.com","threadId":"41125","inReplyTo":"1451980994-26865-16-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v3 15/15] ref-filter: introduce objectname_atom_parser()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-05T21:31:42Z","receivedAt":"2016-01-05T21:31:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> @@ -467,15 +482,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\nThe original used to reject \"objectnamefoo\", but the updated one is\nmore sloppy. Shouldn't it be doing the same match_atom_name() here?\n\nThis comment applies to many remaining strcmp() and starts_with()\nthat is reachable from populate_value().\n"},{"id":"275415","messageId":"CAOLa=ZTKiGM1XLr7UuNVX_e_qN4RAGfvOh6daaxcOeZ9xa2TdA@mail.gmail.com","threadId":"41125","inReplyTo":"xmqq7fjnkexv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 01/15] strbuf: introduce strbuf_split_str_omit_term()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-06T07:27:52Z","receivedAt":"2016-01-06T07:27:52Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Jan 6, 2016 at 12:54 AM, Junio C Hamano wrote:\n> Karthik Nayak writes:\n>\n>> The current implementation of 'strbuf_split_buf()' includes the\n>> terminator at the end of each strbuf post splitting. Add an option\n>> wherein we can drop the terminator if desired. In this context\n>> introduce a wrapper function 'strbuf_split_str_omit_term()' which\n>> splits a given string into strbufs without including the terminator.\n>>\n>> Helped-by: Eric Sunshine \n>> Signed-off-by: Karthik Nayak \n>> ---\n>> strbuf.c | 7 ++++---\n>> strbuf.h | 25 ++++++++++++++++---------\n>> 2 files changed, 20 insertions(+), 12 deletions(-)\n>>\n>> diff --git a/strbuf.c b/strbuf.c\n>> index b552a13..b62508e 100644\n>> --- a/strbuf.c\n>> +++ b/strbuf.c\n>> @@ -115,7 +115,7 @@ void strbuf_tolower(struct strbuf *sb)\n>> }\n>>\n>> struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n>> - int terminator, int max)\n>> + int terminator, int max, int omit_term)\n>> {\n>> struct strbuf **ret = NULL;\n>> size_t nr = 0, alloc = 0;\n>> @@ -123,14 +123,15 @@ struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n>>\n>> while (slen) {\n>> int len = slen;\n>> + const char *end = NULL;\n>> if (max <= 0 || nr + 1 < max) {\n>> - const char *end = memchr(str, terminator, slen);\n>> + end = memchr(str, terminator, slen);\n>> if (end)\n>> len = end - str + 1;\n>> }\n>> t = xmalloc(sizeof(struct strbuf));\n>> strbuf_init(t, len);\n>> - strbuf_add(t, str, len);\n>> + strbuf_add(t, str, len - !!end * !!omit_term);\n>\n> You initialize with \"len\" but sometimes copy less than that, which\n> looks somewhat sloppy.\n>\n> Maybe I am old-fashioned, but use of a multiplication when you do\n> not mean to numerically multiply but merely to perform a logical\n> operation made me go \"Huh?\".\n>\n> Perhaps using another variable would make it easier to follow?\n> Either using a boolean that tells us that the terminating byte\n> is to be omitted, i.e.\n>\n> int len = slen;\n> int omit = 0;\n> if ( ... we are still splitting ... ) {\n> const char *end = memchr(...);\n> if (end) {\n> len = end - str + 1;\n> omit = !!omit_term;\n> }\n> }\n> strbuf_init(t, len - omit);\n> strbuf_add(t, str, len - omit);\n>\n> or an integer \"copylen\" that tells us how many bytes to copy, which\n> often is the same as \"len\" but sometimes different by 1 byte?\n\nThis is done based on Eric's suggestion [1]. Although its a little off normal\nconvention. I find it small and simple. So I'm okay with either, your suggested\nchange or the existing code.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/281882\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"275416","messageId":"CAOLa=ZTqtR4Nn+pRtN0HmDF6exYiq-B8g=zVvcu_8=AqFz9zug@mail.gmail.com","threadId":"41125","inReplyTo":"xmqqwprniwfq.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 08/15] ref-filter: introduce color_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-06T07:52:07Z","receivedAt":"2016-01-06T07:52:07Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Jan 6, 2016 at 2:19 AM, Junio C Hamano wrote:\n> Karthik Nayak writes:\n>\n>> Introduce color_atom_parser() which will parse a \"color\" atom and\n>> store its color in the \"used_atom\" structure for further usage in\n>> populate_value().\n>>\n>> Helped-by: Ramsay Jones \n>> Helped-by: Eric Sunshine \n>> Signed-off-by: Karthik Nayak \n>> ---\n>> ref-filter.c | 29 ++++++++++++++++++++---------\n>> 1 file changed, 20 insertions(+), 9 deletions(-)\n>>\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> index b54c872..9708d67 100644\n>> --- a/ref-filter.c\n>> +++ b/ref-filter.c\n>> @@ -29,6 +29,9 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n>> static struct used_atom {\n>> const char *name;\n>> cmp_type type;\n>> + union {\n>> + char *color;\n>> + } u;\n>> } *used_atom;\n>> static int used_atom_cnt, need_tagged, need_symref;\n>> static int need_color_reset_at_eol;\n>> @@ -53,6 +56,18 @@ static int match_atom_name(const char *name, const char *atom_name, const char *\n>> return 1;\n>> }\n>>\n>> +static void color_atom_parser(struct used_atom *atom)\n>> +{\n>> + if (!match_atom_name(atom->name, \"color\", (const char **)&atom->u.color))\n>> + die(\"BUG: parsing non-'color'\");\n>> + if (!atom->u.color)\n>> + die(_(\"expected format: %%(color:)\"));\n>> + /* atom->u.color points to part of atom->name */\n>> + atom->u.color = xstrdup(atom->u.color);\n>> + if (color_parse(atom->u.color, atom->u.color) < 0)\n>> + die(_(\"invalid color value: %s\"), atom->u.color);\n>\n> Is this calling color_parse() from color.c?\n>\n\nYes it is!\n\n> The function wants the destination to be at least COLOR_MAXLEN, but\n> I do not see where the piece memory pointed by atom->u.color is\n> guaranteed to be that long in the new code. Looking at the code\n> removed by this patch, it used to correctly use a buffer that is\n> COLOR_MAXLEN bytes long. So...\n>\n> const char *color_value;\n>\n> if (!match_atom_name(atom->name, \"color\", color_value))\n> die(\"BUG: parsing non-'color'\");\n> if (!color_value)\n> die(_(\"expected format: %%(color:)\"));\n> atom->u.color = xmalloc(COLOR_MAXLEN);\n> if (color_parse(color_value, atom->u.color) < 0)\n> die(_(\"invalid color value: %s\"), color_value);\n>\n> or even define it in the union, i.e.\n>\n> union {\n> char color[COLOR_MAXLEN];\n> } u;\n>\n> and then use atom->u.color[] in-place?\n\nI like the in-place suggestion, I wasn't wary of the Minimum length requirement\nof the dest in color_parser(). Thanks for bringing it up. Will change.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"275428","messageId":"CAOLa=ZRaviYJNmnq0jucBdQLZrDKr7xNR30k3jP_j_n9UCJ91Q@mail.gmail.com","threadId":"41125","inReplyTo":"xmqqr3hvivml.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 08/15] ref-filter: introduce color_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-06T10:16:46Z","receivedAt":"2016-01-06T10:16:46Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Jan 6, 2016 at 2:36 AM, Junio C Hamano wrote:\n> Karthik Nayak writes:\n>\n>> @@ -90,7 +105,7 @@ static struct {\n>> { \"symref\" },\n>> { \"flag\" },\n>> { \"HEAD\" },\n>> - { \"color\" },\n>> + { \"color\", FIELD_STR, color_atom_parser },\n>> { \"align\" },\n>> { \"end\" },\n>> };\n>\n> This is minor, as I do not think anybody sane would say\n> \"for-each-ref --sort=color\" and expect anything sensible, but\n>\n\nI completely understand and the only reason that I added FIELD_STR\ncause that was the default either ways.\n\n> I think we shouldn't mark these bogus attributes [*1*] as FIELD_STR\n> (and it is not FIELD_ULONG, either). Perhaps add a new FIELD_BOGUS\n> (or some better name to make it clear that this is not a \"value\"\n> that belongs to the ref and can be used to sort, e.g. \"FAKE\") value\n> and mark them as such?\n>\n> That would allow us to error out when they are specified as a\n> sorting criteria.\n>\n>\n> [Footnote]\n>\n> *1* Bogus from the point of view of \"what are the various attributes\n> specific to these items that the command is iterating over,\n> i.e. refs (or the objects at the tips of these refs)\", as color,\n> align, etc. are not attributes per refs.\n\nI'm sure something like FIELD_BOGUS would make a lot of sense, but I wont\ninclude that into this series. Maybe after this?\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"275429","messageId":"CAOLa=ZT=_t62eKpkPbf+5k9TC0MoeN63+nwVTWLccG0rJF09kQ@mail.gmail.com","threadId":"41125","inReplyTo":"xmqqmvsjivcv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 11/15] ref-filter: convert variable 'width' to an unsigned int","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-06T10:17:59Z","receivedAt":"2016-01-06T10:17:59Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Jan 6, 2016 at 2:42 AM, Junio C Hamano wrote:\n> Karthik Nayak writes:\n>\n>> In align_atom_parser() the 'width' variable is an int, which requires\n>> an explicit type conversion when used in strtoul_ui(). Hence make it\n>> an unsigned int.\n>>\n>> Helped-by: Eric Sunshine \n>> Signed-off-by: Karthik Nayak \n>> ---\n>\n> Shouldn't this be done in [09/15] from the beginning? As it's not\n> like [09/15] was a pure code movement, this step looks like \"Oops,\n> I screwed up earlier, and here is a fixup\".\n>\n> If we want to do this as a separate step, it would be easier to\n> follow if it is done _before_ 09/15 to the original of the code\n> movement, I think.\n>\n\nHaha, does seem that way. Will merge it in.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"275445","messageId":"CAOLa=ZQY09yqDVELb9ObTnWfU-9nRyxJiV=_2tHbZPk_oe8sGQ@mail.gmail.com","threadId":"41125","inReplyTo":"xmqqio37iuvj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 14/15] ref-filter: introduce contents_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-06T18:22:03Z","receivedAt":"2016-01-06T18:22:03Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Jan 6, 2016 at 2:52 AM, Junio C Hamano wrote:\n> Karthik Nayak writes:\n>\n>> Introduce contents_atom_parser() which will parse the '%(contents)'\n>> atom and store information into the 'used_atom' structure based on the\n>> modifiers used along with the atom.\n>>\n>> Helped-by: Ramsay Jones \n>> Helped-by: Eric Sunshine \n>> Signed-off-by: Karthik Nayak \n>> ---\n>> ref-filter.c | 74 +++++++++++++++++++++++++++++++++++++-----------------------\n>> 1 file changed, 46 insertions(+), 28 deletions(-)\n>>\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> index 32b4674..9e61530 100644\n>> --- a/ref-filter.c\n>> +++ b/ref-filter.c\n>> @@ -39,6 +39,10 @@ static struct used_atom {\n>> struct align align;\n>> enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }\n>> remote_ref;\n>> + struct {\n>> + enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;\n>> + unsigned int nlines;\n>> + } contents;\n>> } u;\n>> } *used_atom;\n>> static int used_atom_cnt, need_tagged, need_symref;\n>> @@ -96,6 +100,35 @@ static void remote_ref_atom_parser(struct used_atom *atom)\n>> die(_(\"unrecognized format: %%(%s)\"), atom->name);\n>> }\n>>\n>> +static void contents_atom_parser(struct used_atom *atom)\n>> +{\n>> + const char * buf;\n>> +\n>> + if (match_atom_name(atom->name, \"subject\", &buf) && !buf) {\n>> + atom->u.contents.option = C_SUB;\n>> + return;\n>> + } else if (match_atom_name(atom->name, \"body\", &buf) && !buf) {\n>> + atom->u.contents.option = C_BODY_DEP;\n>> + return;\n>> + } if (!match_atom_name(atom->name, \"contents\", &buf))\n>> + die(\"BUG: parsing non-'contents'\");\n>\n> Did you really intend to say \"if\" here, not \"else if\"?\n>\n\nNot that it makes a difference here since both the previous\ncondition return. I think \"else if\" would be better.\n\n> I also wonder if the \"&& !buf\" in the first two are correct. What\n> should happen to \"%(subject:foo)\", which gives you a non-empty buf?\n> It may or may not be an error, but it should not fall thru and cause\n> \"BUG:parsing non-contents\", should it?\n>\n\nI think It would be better to add specific messages there\n\nif (match_atom_name(atom->name, \"subject\", &buf)) {\n if (buf)\n die(\"%%(subject) atom does not take arguments\");\n atom->u.contents.option = C_SUB;\n return;\n} else if (match_atom_name(atom->name, \"body\", &buf)) {\n if (buf)\n die(\"%%(body) atom does not take arguments\");\n atom->u.contents.option = C_BODY_DEP;\n return;\n} else if (!match_atom_name(atom->name, \"contents\", &buf))\n die(\"BUG: parsing non-'contents'\");\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"275446","messageId":"CAOLa=ZQxFTSAfdVf6moRkJYKGDKG35FBUZ+Tg2+d4Ue0biYsuw@mail.gmail.com","threadId":"41125","inReplyTo":"xmqqegdviugx.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 15/15] ref-filter: introduce objectname_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-06T18:27:56Z","receivedAt":"2016-01-06T18:27:56Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Jan 6, 2016 at 3:01 AM, Junio C Hamano wrote:\n> Karthik Nayak writes:\n>\n>> @@ -467,15 +482,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>> - struct atom_value *v)\n>> + struct atom_value *v, struct used_atom *atom)\n>> {\n>> - if (!strcmp(name, \"objectname\")) {\n>> - v->s = xstrdup(sha1_to_hex(sha1));\n>> - return 1;\n>> - }\n>> - if (!strcmp(name, \"objectname:short\")) {\n>> - v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));\n>> - return 1;\n>> + if (starts_with(name, \"objectname\")) {\n>\n> The original used to reject \"objectnamefoo\", but the updated one is\n> more sloppy. Shouldn't it be doing the same match_atom_name() here?\n>\n> This comment applies to many remaining strcmp() and starts_with()\n> that is reachable from populate_value().\n\nShould we worry about such extensions of atoms? I see that\nparse_ref_filter_atom()\ntakes care of something like that in the beginning itself\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"275450","messageId":"CAPig+cRjXHmBnnmHbbibeV-gcvwMFQc7-5ce4s2OHu+wQwMy4Q@mail.gmail.com","threadId":"41125","inReplyTo":"1451980994-26865-1-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v3 00/15] ref-filter: use parsing functions","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-01-06T21:14:41Z","receivedAt":"2016-01-06T21:14:41Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jan 5, 2016 at 3:02 AM, Karthik Nayak wrote:\n> Eric suggested that I make match_atom_name() not return a value [0]. I\n> haven't done that as we use match_atom_name() in [14/15] for matching\n> 'subject' and 'body' in contents_atom_parser() and although Eric\n> suggested I use strcmp() instead, this would not work as we need to\n> check for derefernced 'subject' and 'body' atoms.\n> [0]: http://article.gmane.org/gmane.comp.version-control.git/282701\n\nI don't understand the difficulty. It should be easy to manually skip\nthe 'deref' for this one particular case:\n\n const char *name = atom->name;\n if (*name == '*')\n name++;\n\nWhich would allow this unnecessarily complicated code from patch 14/15:\n\n if (match_atom_name(atom->name, \"subject\", &buf) && !buf) {\n ...\n return;\n } else if (match_atom_name(atom->name, \"body\", &buf) && !buf) {\n ...\n return;\n } if (!match_atom_name(atom->name, \"contents\", &buf))\n die(\"BUG: parsing non-'contents'\");\n\nto be simplified to the more easily understood form suggested during\nreview[1] of v2:\n\n if (!strcmp(name, \"subject\")) {\n ...\n return;\n } else if (!strcmp(name, \"body\")) {\n ...\n return;\n } else if (!match_atom_name(name,\"contents\", &buf))\n die(\"BUG: expected 'contents' or 'contents:'\");\n\nYou could also just use (!strcmp(\"body\") || !strcmp(\"*body\")) rather\nthan skipping \"*\" manually, but the repetition makes that a bit\nnoisier and uglier.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/282645\n"},{"id":"275489","messageId":"CAOLa=ZSi0c61G8oXsCC0UfprDhf3hESaqKw8eGNRZJ7MTvNY4g@mail.gmail.com","threadId":"41125","inReplyTo":"CAPig+cRjXHmBnnmHbbibeV-gcvwMFQc7-5ce4s2OHu+wQwMy4Q@mail.gmail.com","subject":"Re: [PATCH v3 00/15] ref-filter: use parsing functions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-07T14:25:41Z","receivedAt":"2016-01-07T14:25:41Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Thu, Jan 7, 2016 at 2:44 AM, Eric Sunshine wrote:\n> On Tue, Jan 5, 2016 at 3:02 AM, Karthik Nayak wrote:\n>> Eric suggested that I make match_atom_name() not return a value [0]. I\n>> haven't done that as we use match_atom_name() in [14/15] for matching\n>> 'subject' and 'body' in contents_atom_parser() and although Eric\n>> suggested I use strcmp() instead, this would not work as we need to\n>> check for derefernced 'subject' and 'body' atoms.\n>> [0]: http://article.gmane.org/gmane.comp.version-control.git/282701\n>\n> I don't understand the difficulty. It should be easy to manually skip\n> the 'deref' for this one particular case:\n>\n> const char *name = atom->name;\n> if (*name == '*')\n> name++;\n>\n> Which would allow this unnecessarily complicated code from patch 14/15:\n>\n> if (match_atom_name(atom->name, \"subject\", &buf) && !buf) {\n> ...\n> return;\n> } else if (match_atom_name(atom->name, \"body\", &buf) && !buf) {\n> ...\n> return;\n> } if (!match_atom_name(atom->name, \"contents\", &buf))\n> die(\"BUG: parsing non-'contents'\");\n>\n> to be simplified to the more easily understood form suggested during\n> review[1] of v2:\n>\n> if (!strcmp(name, \"subject\")) {\n> ...\n> return;\n> } else if (!strcmp(name, \"body\")) {\n> ...\n> return;\n> } else if (!match_atom_name(name,\"contents\", &buf))\n> die(\"BUG: expected 'contents' or 'contents:'\");\n>\n> You could also just use (!strcmp(\"body\") || !strcmp(\"*body\")) rather\n> than skipping \"*\" manually, but the repetition makes that a bit\n> noisier and uglier.\n>\n> [1]: http://article.gmane.org/gmane.comp.version-control.git/282645\n\nDefinitely not a difficulty per se. Just that it seems like something\nmatch_atom_name()\nseems to be fit for. As the function name suggests that we're matching\nthe atom name\nand the check for '!buf' indicates that no options are to be included\nfor that particular atom.\n\nAlso after Junio's suggestion[1], I think It looks better now[2]. But\neither ways, I'm not\nstrongly against what you're saying, so my opinion on this matter is\nquite flexible.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/283404\n[2]: http://article.gmane.org/gmane.comp.version-control.git/283449\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"275493","messageId":"xmqqziwh4669.fsf@gitster.mtv.corp.google.com","threadId":"41125","inReplyTo":"CAOLa=ZQY09yqDVELb9ObTnWfU-9nRyxJiV=_2tHbZPk_oe8sGQ@mail.gmail.com","subject":"Re: [PATCH v3 14/15] ref-filter: introduce contents_atom_parser()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-07T18:04:46Z","receivedAt":"2016-01-07T18:04:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n>>> +static void contents_atom_parser(struct used_atom *atom)\n>>> +{\n>>> + const char * buf;\n\nconst char *buf;\n\n>>> +\n>>> + if (match_atom_name(atom->name, \"subject\", &buf) && !buf) {\n>>> + atom->u.contents.option = C_SUB;\n>>> + return;\n>>> + } else if (match_atom_name(atom->name, \"body\", &buf) && !buf) {\n>>> + atom->u.contents.option = C_BODY_DEP;\n>>> + return;\n>>> + } if (!match_atom_name(atom->name, \"contents\", &buf))\n>>> + die(\"BUG: parsing non-'contents'\");\n>>\n>> Did you really intend to say \"if\" here, not \"else if\"?\n>\n> Not that it makes a difference here since both the previous\n> condition return. I think \"else if\" would be better.\n\nI am not sure if it is \"Y would be better even though X and Y both\nwould work\". It looks to me \"X and Y behave differently, X is a bug\nand Y is correct\".\n\nThe above would behave differently between \"if\" and \"else if\" (and\nby the way, the code layout suggests it is \"else if\"; otherwise you\nwould be starting \"if\" on its own line) when you feed \"subject:foo\",\nno?\n"},{"id":"275494","messageId":"xmqqvb7544de.fsf@gitster.mtv.corp.google.com","threadId":"41125","inReplyTo":"CAOLa=ZSi0c61G8oXsCC0UfprDhf3hESaqKw8eGNRZJ7MTvNY4g@mail.gmail.com","subject":"Re: [PATCH v3 00/15] ref-filter: use parsing functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-07T18:43:41Z","receivedAt":"2016-01-07T18:43:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n>> I don't understand the difficulty. It should be easy to manually skip\n>> the 'deref' for this one particular case:\n>>\n>> const char *name = atom->name;\n>> if (*name == '*')\n>> name++;\n>>\n>> Which would allow this unnecessarily complicated code from patch 14/15:\n>>\n>> if (match_atom_name(atom->name, \"subject\", &buf) && !buf) {\n>> ...\n>> return;\n>> } else if (match_atom_name(atom->name, \"body\", &buf) && !buf) {\n>> ...\n>> return;\n>> } if (!match_atom_name(atom->name, \"contents\", &buf))\n>> die(\"BUG: parsing non-'contents'\");\n>>\n>> to be simplified to the more easily understood form suggested during\n>> review[1] of v2:\n>>\n>> if (!strcmp(name, \"subject\")) {\n>> ...\n>> return;\n>> } else if (!strcmp(name, \"body\")) {\n>> ...\n>> return;\n>> } else if (!match_atom_name(name,\"contents\", &buf))\n>> die(\"BUG: expected 'contents' or 'contents:'\");\n>>\n>> You could also just use (!strcmp(\"body\") || !strcmp(\"*body\")) rather\n>> than skipping \"*\" manually, but the repetition makes that a bit\n>> noisier and uglier.\n>>\n>> [1]: http://article.gmane.org/gmane.comp.version-control.git/282645\n>\n> Definitely not a difficulty per se. Just that it seems like something\n> match_atom_name()\n> seems to be fit for. As the function name suggests that we're matching\n> the atom name\n> and the check for '!buf' indicates that no options are to be included\n> for that particular atom.\n>\n> Also after Junio's suggestion[1], I think It looks better now[2]. But\n> either ways, I'm not\n> strongly against what you're saying, so my opinion on this matter is\n> quite flexible.\n>\n> [1]: http://article.gmane.org/gmane.comp.version-control.git/283404\n> [2]: http://article.gmane.org/gmane.comp.version-control.git/283449\n\nSorry, but I suspect I was looking at a leaf function without\nthinking about the larger picture.\n\nI suspect that the interface to customized parsing function called\nby parse_ref_filter_atom() is misdesigned. I understand that the\noverall parsing that starts at verify_ref_format() goes like this:\n\n * Iterate over the string and find a matching \"%(\",\")\" pair.\n\n - For each such pair found, use parse_ref_filter_atom() on\n what is inside that matching pair.\n\n - parse_ref_filter_atom() iterates over the table of known\n atoms, and finds the entry in that table.\n\n Note that at this point, it knows that \"%(\" is followed by\n 'contents' or 'contents:' when it picked the \"contents\" atom\n from the table, for example.\n\n - if the entry we found in that table for the atom being parsed\n has a custom parse function, that function is called, but the\n calling convention does not pass the fact that we already\n know what we are seeing inside \"%(\",\")\" pair is 'contents',\n for example, and we know what argument it is given if any.\n\nSo it appears to me that match_atom_name() is a misguided helper\nfunction that you shouldn't have to use too often. If the signature\nof parse() functions is changed to take not just the atom but the\npointer to its argument (could be NULL, if we are seeing\n\"%(contents)\", for example) that is already available as \"formatp\"\nin the function, then contents_atom_parser() could become more like:\n\ncontents_atom_parser(struct used_atom *atom, const char *arg)\n{\n\tif (args)\n \tatom->u.contents.option = C_BARE;\n\telse if (!strcmp(arg, \"body\"))\n \tatom->u.contents.option = C_BODY;\n\t...\n}\n\nand there is no reason for this caller to even look at atom->name or\nworry about that it might have the dereferencing asterisk in front.\n\nIf we really want to avoid having separate subject_atom_parser() and\nbody_atom_parser(), they can be folded into the same function and it\nbecomes necessary to switch on atom->name like you did in the code\nbeing discussed in the quoted part above. For that, as Eric said,\nskipping '*' manually would not be a big deal, as that should not\nhappen so often in the code _anyway_. It is not a good idea to\nswitch on atom->name inside contents_atom_parser() like you did.\nYou are better off having separate {subject,body}_atom_parser()\nfunctions.\n\nFor one thing, you are not reusing or sharing any code by squishing\nthese three functions into one. A conceptually larger problem is\nthat you are adding two extra !strcmp() calls to figure out the\ncaller _already_ knows (notice I said this is \"conceptual\", this is\nnot about performance). parse_ref_filter_atom() knows that it is a\n\"%(subject)\" or \"%(subject:...)\" atom, but because you throw away\nthat information and call contents_atom_parser() by saying that it\nis one of the contents, subject or body, the called function has to\nredo strcmp in order to figure it out itself.\n"},{"id":"275497","messageId":"CAOLa=ZR=id7826YQZ4+Gk-6RzWVKrVomSFvzPqToeLcgt8Jr+Q@mail.gmail.com","threadId":"41125","inReplyTo":"xmqqziwh4669.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 14/15] ref-filter: introduce contents_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-07T20:03:54Z","receivedAt":"2016-01-07T20:03:54Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Thu, Jan 7, 2016 at 11:34 PM, Junio C Hamano wrote:\n> Karthik Nayak writes:\n>\n>>>> +static void contents_atom_parser(struct used_atom *atom)\n>>>> +{\n>>>> + const char * buf;\n>\n> const char *buf;\n>\n\nwill change that.\n\n>>>> +\n>>>> + if (match_atom_name(atom->name, \"subject\", &buf) && !buf) {\n>>>> + atom->u.contents.option = C_SUB;\n>>>> + return;\n>>>> + } else if (match_atom_name(atom->name, \"body\", &buf) && !buf) {\n>>>> + atom->u.contents.option = C_BODY_DEP;\n>>>> + return;\n>>>> + } if (!match_atom_name(atom->name, \"contents\", &buf))\n>>>> + die(\"BUG: parsing non-'contents'\");\n>>>\n>>> Did you really intend to say \"if\" here, not \"else if\"?\n>>\n>> Not that it makes a difference here since both the previous\n>> condition return. I think \"else if\" would be better.\n>\n> I am not sure if it is \"Y would be better even though X and Y both\n> would work\". It looks to me \"X and Y behave differently, X is a bug\n> and Y is correct\".\n>\n> The above would behave differently between \"if\" and \"else if\" (and\n> by the way, the code layout suggests it is \"else if\"; otherwise you\n> would be starting \"if\" on its own line) when you feed \"subject:foo\",\n> no?\n\nIt is indeed an \"else if\". What I was referring to was that since its like\n\nif (cond_a) {\n...\nreturn;\n} else if (cond_b) {\n...\nreturn;\n} if (cond_c) {\n...\n}\n\ncond_c is only checked if cond_a and cond_b don't hold good. Similar to\nhow 'else if' would work, because cond_a and cond_b return. Sorry for the\nconfusion.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"275498","messageId":"CAOLa=ZSoAdSet5LL9fxJA-xj6Z_giuVL3+x_N7qTs_yFujrhWA@mail.gmail.com","threadId":"41125","inReplyTo":"xmqqvb7544de.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 00/15] ref-filter: use parsing functions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-07T20:20:00Z","receivedAt":"2016-01-07T20:20:00Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Fri, Jan 8, 2016 at 12:13 AM, Junio C Hamano wrote:\n> Karthik Nayak writes:\n>\n>>> I don't understand the difficulty. It should be easy to manually skip\n>>> the 'deref' for this one particular case:\n>>>\n>>> const char *name = atom->name;\n>>> if (*name == '*')\n>>> name++;\n>>>\n>>> Which would allow this unnecessarily complicated code from patch 14/15:\n>>>\n>>> if (match_atom_name(atom->name, \"subject\", &buf) && !buf) {\n>>> ...\n>>> return;\n>>> } else if (match_atom_name(atom->name, \"body\", &buf) && !buf) {\n>>> ...\n>>> return;\n>>> } if (!match_atom_name(atom->name, \"contents\", &buf))\n>>> die(\"BUG: parsing non-'contents'\");\n>>>\n>>> to be simplified to the more easily understood form suggested during\n>>> review[1] of v2:\n>>>\n>>> if (!strcmp(name, \"subject\")) {\n>>> ...\n>>> return;\n>>> } else if (!strcmp(name, \"body\")) {\n>>> ...\n>>> return;\n>>> } else if (!match_atom_name(name,\"contents\", &buf))\n>>> die(\"BUG: expected 'contents' or 'contents:'\");\n>>>\n>>> You could also just use (!strcmp(\"body\") || !strcmp(\"*body\")) rather\n>>> than skipping \"*\" manually, but the repetition makes that a bit\n>>> noisier and uglier.\n>>>\n>>> [1]: http://article.gmane.org/gmane.comp.version-control.git/282645\n>>\n>> Definitely not a difficulty per se. Just that it seems like something\n>> match_atom_name()\n>> seems to be fit for. As the function name suggests that we're matching\n>> the atom name\n>> and the check for '!buf' indicates that no options are to be included\n>> for that particular atom.\n>>\n>> Also after Junio's suggestion[1], I think It looks better now[2]. But\n>> either ways, I'm not\n>> strongly against what you're saying, so my opinion on this matter is\n>> quite flexible.\n>>\n>> [1]: http://article.gmane.org/gmane.comp.version-control.git/283404\n>> [2]: http://article.gmane.org/gmane.comp.version-control.git/283449\n>\n> Sorry, but I suspect I was looking at a leaf function without\n> thinking about the larger picture.\n>\n> I suspect that the interface to customized parsing function called\n> by parse_ref_filter_atom() is misdesigned. I understand that the\n> overall parsing that starts at verify_ref_format() goes like this:\n>\n> * Iterate over the string and find a matching \"%(\",\")\" pair.\n>\n> - For each such pair found, use parse_ref_filter_atom() on\n> what is inside that matching pair.\n>\n> - parse_ref_filter_atom() iterates over the table of known\n> atoms, and finds the entry in that table.\n>\n> Note that at this point, it knows that \"%(\" is followed by\n> 'contents' or 'contents:' when it picked the \"contents\" atom\n> from the table, for example.\n>\n> - if the entry we found in that table for the atom being parsed\n> has a custom parse function, that function is called, but the\n> calling convention does not pass the fact that we already\n> know what we are seeing inside \"%(\",\")\" pair is 'contents',\n> for example, and we know what argument it is given if any.\n>\n\nAbsolutely correct.\n\n> So it appears to me that match_atom_name() is a misguided helper\n> function that you shouldn't have to use too often. If the signature\n> of parse() functions is changed to take not just the atom but the\n> pointer to its argument (could be NULL, if we are seeing\n> \"%(contents)\", for example) that is already available as \"formatp\"\n> in the function, then contents_atom_parser() could become more like:\n>\n\nI see, I think this does make sense, since we have extracted any arguments\nfollowing ':' already in parse_ref_filter_atom() it only makes sense to use it\nwithout doing the same type of work again.\n\n> contents_atom_parser(struct used_atom *atom, const char *arg)\n> {\n> if (args)\n> atom->u.contents.option = C_BARE;\n> else if (!strcmp(arg, \"body\"))\n> atom->u.contents.option = C_BODY;\n> ...\n> }\n>\n> and there is no reason for this caller to even look at atom->name or\n> worry about that it might have the dereferencing asterisk in front.\n>\n\nThis looks good.\n\n> If we really want to avoid having separate subject_atom_parser() and\n> body_atom_parser(), they can be folded into the same function and it\n> becomes necessary to switch on atom->name like you did in the code\n> being discussed in the quoted part above. For that, as Eric said,\n> skipping '*' manually would not be a big deal, as that should not\n> happen so often in the code _anyway_. It is not a good idea to\n> switch on atom->name inside contents_atom_parser() like you did.\n> You are better off having separate {subject,body}_atom_parser()\n> functions.\n>\n> For one thing, you are not reusing or sharing any code by squishing\n> these three functions into one. A conceptually larger problem is\n> that you are adding two extra !strcmp() calls to figure out the\n> caller _already_ knows (notice I said this is \"conceptual\", this is\n> not about performance). parse_ref_filter_atom() knows that it is a\n> \"%(subject)\" or \"%(subject:...)\" atom, but because you throw away\n> that information and call contents_atom_parser() by saying that it\n> is one of the contents, subject or body, the called function has to\n> redo strcmp in order to figure it out itself.\n\nI understand what you're saying and keeping these functions separate\nseems like the way to go. Also this might also remove all uses of\nmatch_atom_name() from ref-filter.c. This Idea seems good to me,\nthanks for your suggestions, I will work on it.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"275501","messageId":"CAPig+cSJ-6v2x4CY7h9OZQwSR4y9dXGnhxUv2Ykt997vx6gcZg@mail.gmail.com","threadId":"41125","inReplyTo":"xmqqvb7544de.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 00/15] ref-filter: use parsing functions","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-01-07T20:36:59Z","receivedAt":"2016-01-07T20:36:59Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jan 7, 2016 at 1:43 PM, Junio C Hamano wrote:\n> If we really want to avoid having separate subject_atom_parser() and\n> body_atom_parser(), they can be folded into the same function and it\n> becomes necessary to switch on atom->name like you did in the code\n> being discussed in the quoted part above. For that, as Eric said,\n> skipping '*' manually would not be a big deal, as that should not\n> happen so often in the code _anyway_. It is not a good idea to\n> switch on atom->name inside contents_atom_parser() like you did.\n> You are better off having separate {subject,body}_atom_parser()\n> functions.\n>\n> For one thing, you are not reusing or sharing any code by squishing\n> these three functions into one. A conceptually larger problem is\n> that you are adding two extra !strcmp() calls to figure out the\n> caller _already_ knows (notice I said this is \"conceptual\", this is\n> not about performance). parse_ref_filter_atom() knows that it is a\n> \"%(subject)\" or \"%(subject:...)\" atom, but because you throw away\n> that information and call contents_atom_parser() by saying that it\n> is one of the contents, subject or body, the called function has to\n> redo strcmp in order to figure it out itself.\n\nThanks for spelling this all out. I had come to the same conclusion\nyesterday after posting my message but never got back to the computer\nlong enough to compose a proper email explaining the issue. I fully\nagree that this would be a better design.\n"},{"id":"275502","messageId":"CAOLa=ZSzPeqsObgno8q0hpbAGUgZgFJ5x8Oj7YtA7_uPLvG0Pw@mail.gmail.com","threadId":"41125","inReplyTo":"xmqqvb7544de.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 00/15] ref-filter: use parsing functions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-07T20:44:40Z","receivedAt":"2016-01-07T20:44:40Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Fri, Jan 8, 2016 at 12:13 AM, Junio C Hamano wrote:\n>\n> So it appears to me that match_atom_name() is a misguided helper\n> function that you shouldn't have to use too often. If the signature\n> of parse() functions is changed to take not just the atom but the\n> pointer to its argument (could be NULL, if we are seeing\n> \"%(contents)\", for example) that is already available as \"formatp\"\n> in the function, then contents_atom_parser() could become more like:\n>\n> contents_atom_parser(struct used_atom *atom, const char *arg)\n> {\n> if (args)\n> atom->u.contents.option = C_BARE;\n> else if (!strcmp(arg, \"body\"))\n> atom->u.contents.option = C_BODY;\n> ...\n> }\n>\n> and there is no reason for this caller to even look at atom->name or\n> worry about that it might have the dereferencing asterisk in front.\n>\n\nSo we something like this for the parsing function:\n\n int parse_ref_filter_atom(const char *atom, const char *ep)\n {\n const char *sp;\n+ char *arg;\n int i, at;\n\n sp = atom;\n@@ -141,6 +143,7 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n const char *formatp = strchr(sp, ':');\n if (!formatp || ep < formatp)\n formatp = ep;\n+ arg = (char *)formatp;\n if (len == formatp - sp && !memcmp(valid_atom[i].name, sp, len))\n break;\n }\n@@ -154,6 +157,13 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n REALLOC_ARRAY(used_atom, used_atom_cnt);\n used_atom[at].name = xmemdupz(atom, ep - atom);\n used_atom[at].type = valid_atom[i].cmp_type;\n+ if (arg != ep)\n+ arg = xstrndup(arg + 1, ep - arg - 1);\n+ else\n+ arg = NULL;\n+ if (valid_atom[i].parser)\n+ valid_atom[i].parser(&used_atom[at], arg);\n+ free(arg);\n if (*atom == '*')\n need_tagged = 1;\n if (!strcmp(used_atom[at].name, \"symref\"))\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"275503","messageId":"xmqqd1td3wqk.fsf@gitster.mtv.corp.google.com","threadId":"41125","inReplyTo":"CAOLa=ZSzPeqsObgno8q0hpbAGUgZgFJ5x8Oj7YtA7_uPLvG0Pw@mail.gmail.com","subject":"Re: [PATCH v3 00/15] ref-filter: use parsing functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-07T21:28:35Z","receivedAt":"2016-01-07T21:28:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> So we something like this for the parsing function:\n>\n> int parse_ref_filter_atom(const char *atom, const char *ep)\n> {\n> const char *sp;\n> + char *arg;\n\nI think this and the new parameter to .parser() function should be\n\"const char *\".\n\n> @@ -141,6 +143,7 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n> const char *formatp = strchr(sp, ':');\n> if (!formatp || ep < formatp)\n> formatp = ep;\n> + arg = (char *)formatp;\n> if (len == formatp - sp && !memcmp(valid_atom[i].name, sp, len))\n> break;\n\nAnd this part can just use arg without formatp. The original is a\nbit sloppy and will keep looking for a colon past ep, but we already\nknow between sp and ep there is no NUL, so we could do this:\n\n\t\targ = memchr(sp, ':', ep - sp);\n\t\tif ((!arg || len == arg - sp) &&\n\t\t !memcmp(valid_atom[i].name, sp, len))\n\t\t\tbreak;\n\n> @@ -154,6 +157,13 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n> REALLOC_ARRAY(used_atom, used_atom_cnt);\n> used_atom[at].name = xmemdupz(atom, ep - atom);\n> used_atom[at].type = valid_atom[i].cmp_type;\n> + if (arg != ep)\n> + arg = xstrndup(arg + 1, ep - arg - 1);\n> + else\n> + arg = NULL;\n\nWhy even copy? The original that used match_atom_name() borrowed\npart of existing string via (const char **val), so you know whatever\nused that &buf you grabbed out of match_atom_name() should only be\nreading the values not writing into the memory, no?\n\nThat is why I think arg should be \"const char *\".\n\nAs the above memchr() alrady took care of \"we didn't find a colon\"\ncase, we only need to do this here, I think:\n\n\tif (arg)\n \targ = used_atom[at].name + (arg - atom);\n\nand without later free().\n\nAlternatively, we could add an int field to elements of used_atom[]\narray that says what byte-offset in the used_atom[].name the atom\narguments start (if any). Then .parser() does not have to take the\nparameter separately [*1*].\n\n> + if (valid_atom[i].parser)\n> + valid_atom[i].parser(&used_atom[at], arg);\n> + free(arg);\n> if (*atom == '*')\n> need_tagged = 1;\n> if (!strcmp(used_atom[at].name, \"symref\"))\n\n\n\n[Footnote]\n\n*1* Thinking about it more, perhaps used_atom[].type should be\nremoved and instead used_atom[].atom should be a pointer into the\nvalid_atom[] array. Then any reference to used_atom[].type will\nbecome used_atom[].atom->cmp_type, which is much nicer for two\nreasons: (1) one less useless copy (2) one less field that has a\nname \"type\" that is overly generic.\n\nThat does not remove the need for recording where the atom argument\nis, though, in used_atom[]. We could add a bit \"has_deref\" to\nused_atom[] and then do something like this:\n\n arg = used_atom[i].name + used_atom[i].atom->namelen +\n used_atom[i].has_deref;\n\nbut I do not think we want to go there. It would hardcode the\nknowledge that used_atom[i].name is either used_atom[i].atom->name\nor one asterisk prefixed to it, making future extension of the\nsyntax even harder.\n"},{"id":"275593","messageId":"CAOLa=ZQzSgNftM_gTKAo9SmZVm0DrXTRoaZHAniEh+qvB0syoQ@mail.gmail.com","threadId":"41125","inReplyTo":"xmqqd1td3wqk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 00/15] ref-filter: use parsing functions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-09T09:00:22Z","receivedAt":"2016-01-09T09:00:22Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Fri, Jan 8, 2016 at 2:58 AM, Junio C Hamano wrote:\n> Karthik Nayak writes:\n>\n>> So we something like this for the parsing function:\n>>\n>> int parse_ref_filter_atom(const char *atom, const char *ep)\n>> {\n>> const char *sp;\n>> + char *arg;\n>\n> I think this and the new parameter to .parser() function should be\n> \"const char *\".\n>\n\nYes. Will do that.\n\n>> @@ -141,6 +143,7 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n>> const char *formatp = strchr(sp, ':');\n>> if (!formatp || ep < formatp)\n>> formatp = ep;\n>> + arg = (char *)formatp;\n>> if (len == formatp - sp && !memcmp(valid_atom[i].name, sp, len))\n>> break;\n>\n> And this part can just use arg without formatp. The original is a\n> bit sloppy and will keep looking for a colon past ep, but we already\n> know between sp and ep there is no NUL, so we could do this:\n>\n> arg = memchr(sp, ':', ep - sp);\n> if ((!arg || len == arg - sp) &&\n> !memcmp(valid_atom[i].name, sp, len))\n> break;\n>\n\nThis even removes the need for formatp. Thanks\n\n>> @@ -154,6 +157,13 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n>> REALLOC_ARRAY(used_atom, used_atom_cnt);\n>> used_atom[at].name = xmemdupz(atom, ep - atom);\n>> used_atom[at].type = valid_atom[i].cmp_type;\n>> + if (arg != ep)\n>> + arg = xstrndup(arg + 1, ep - arg - 1);\n>> + else\n>> + arg = NULL;\n>\n> Why even copy? The original that used match_atom_name() borrowed\n> part of existing string via (const char **val), so you know whatever\n> used that &buf you grabbed out of match_atom_name() should only be\n> reading the values not writing into the memory, no?\n>\n\nI totally missed that we could use the existing used_atom[at].name\nto get the argument. Thanks\n\n> That is why I think arg should be \"const char *\".\n>\n> As the above memchr() alrady took care of \"we didn't find a colon\"\n> case, we only need to do this here, I think:\n>\n> if (arg)\n> arg = used_atom[at].name + (arg - atom);\n>\n> and without later free().\n>\n if (arg)\n arg = used_atom[at].name + (arg - atom) + 1;\n\n'+ 1' to skip over the colon perhaps?\n\n> Alternatively, we could add an int field to elements of used_atom[]\n> array that says what byte-offset in the used_atom[].name the atom\n> arguments start (if any). Then .parser() does not have to take the\n> parameter separately [*1*].\n>\n> [Footnote]\n>\n> *1* Thinking about it more, perhaps used_atom[].type should be\n> removed and instead used_atom[].atom should be a pointer into the\n> valid_atom[] array. Then any reference to used_atom[].type will\n> become used_atom[].atom->cmp_type, which is much nicer for two\n> reasons: (1) one less useless copy (2) one less field that has a\n> name \"type\" that is overly generic.\n>\n> That does not remove the need for recording where the atom argument\n> is, though, in used_atom[]. We could add a bit \"has_deref\" to\n> used_atom[] and then do something like this:\n>\n> arg = used_atom[i].name + used_atom[i].atom->namelen +\n> used_atom[i].has_deref;\n>\n> but I do not think we want to go there. It would hardcode the\n> knowledge that used_atom[i].name is either used_atom[i].atom->name\n> or one asterisk prefixed to it, making future extension of the\n> syntax even harder.\n\nMakes sense, to sum it up, we'll provide arguments to the parser function:\n\n void (*parser)(struct used_atom *atom, const char *arg);\n\nSo that we provide the arguments to the functions and ensure no need of parsing\natom->name.\n\nAlso ensure that we have separate parser functions for 'body',\n'subject' and 'contents'.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"276501","messageId":"CAPig+cSRDhbHmn5NSp_S4J99igFhBEpWfVMBKBDHCWAHN7-8qQ@mail.gmail.com","threadId":"41125","inReplyTo":"1451980994-26865-5-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v3 04/15] ref-filter: introduce struct used_atom","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-01-21T19:04:52Z","receivedAt":"2016-01-21T19:04:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tuesday, January 5, 2016, Karthik Nayak wrote:\n> Introduce the 'used_atom' structure to replace the existing\n> implementation of 'used_atom' (which is a list of atoms). This helps\n> us parse atoms beforehand and store required details into the\n> 'used_atom' for future usage.\n>\n> Helped-by: Eric Sunshine \n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> @@ -17,7 +17,7 @@\n> typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n>\n> /*\n> - * An atom is a valid field atom listed above, possibly prefixed with\n> + * An atom is a valid field atom listed below, possibly prefixed with\n\nThis particular change should be in patch 3/15 which moved this comment block.\n\n(Yes, it's true that this will make the patch something other than\n\"pure code movement\", but this change keeps the moved block logically\nconsistent and that it's an appropriate thing to do -- plus it's quite\nminor.)\n\n> * a \"*\" to denote deref_tag().\n> *\n> * We parse given format string and sort specifiers, and make a list\n"},{"id":"276504","messageId":"CAPig+cQ2-ddENz4dKsvTW-tOrwugUGa4g-3PySMTBsHsrtO93w@mail.gmail.com","threadId":"41125","inReplyTo":"CAOLa=ZTKiGM1XLr7UuNVX_e_qN4RAGfvOh6daaxcOeZ9xa2TdA@mail.gmail.com","subject":"Re: [PATCH v3 01/15] strbuf: introduce strbuf_split_str_omit_term()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-01-21T19:47:55Z","receivedAt":"2016-01-21T19:47:55Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 6, 2016 at 2:27 AM, Karthik Nayak wrote:\n> On Wed, Jan 6, 2016 at 12:54 AM, Junio C Hamano wrote:\n>> Karthik Nayak writes:\n>>> while (slen) {\n>>> int len = slen;\n>>> + const char *end = NULL;\n>>> if (max <= 0 || nr + 1 < max) {\n>>> - const char *end = memchr(str, terminator, slen);\n>>> + end = memchr(str, terminator, slen);\n>>> if (end)\n>>> len = end - str + 1;\n>>> }\n>>> t = xmalloc(sizeof(struct strbuf));\n>>> strbuf_init(t, len);\n>>> - strbuf_add(t, str, len);\n>>> + strbuf_add(t, str, len - !!end * !!omit_term);\n>>\n>> Perhaps using another variable would make it easier to follow?\n>> Either using a boolean that tells us that the terminating byte\n>> is to be omitted, i.e.\n>>\n>> int len = slen;\n>> int omit = 0;\n>> if ( ... we are still splitting ... ) {\n>> const char *end = memchr(...);\n>> if (end) {\n>> len = end - str + 1;\n>> omit = !!omit_term;\n>> }\n>> }\n>> strbuf_init(t, len - omit);\n>> strbuf_add(t, str, len - omit);\n>>\n>> or an integer \"copylen\" that tells us how many bytes to copy, which\n>> often is the same as \"len\" but sometimes different by 1 byte?\n>\n> This is done based on Eric's suggestion [1]. Although its a little off normal\n> convention. I find it small and simple. So I'm okay with either, your suggested\n> change or the existing code.\n\nA \"copylen\" variable would probably result in the clearest code since\nit states explicitly what an otherwise opaque expression like (!!end *\n!!omit_term) means, thus is easier to reason about.\n"},{"id":"276696","messageId":"CAOLa=ZRhViUntR4yO66imd7D6KN+CCqkREGPVRfTfyN3zLQoAw@mail.gmail.com","threadId":"41125","inReplyTo":"CAPig+cQ2-ddENz4dKsvTW-tOrwugUGa4g-3PySMTBsHsrtO93w@mail.gmail.com","subject":"Re: [PATCH v3 01/15] strbuf: introduce strbuf_split_str_omit_term()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-25T06:12:08Z","receivedAt":"2016-01-25T06:12:08Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Fri, Jan 22, 2016 at 1:17 AM, Eric Sunshine wrote:\n> On Wed, Jan 6, 2016 at 2:27 AM, Karthik Nayak wrote:\n>> On Wed, Jan 6, 2016 at 12:54 AM, Junio C Hamano wrote:\n>>> Karthik Nayak writes:\n>>>> while (slen) {\n>>>> int len = slen;\n>>>> + const char *end = NULL;\n>>>> if (max <= 0 || nr + 1 < max) {\n>>>> - const char *end = memchr(str, terminator, slen);\n>>>> + end = memchr(str, terminator, slen);\n>>>> if (end)\n>>>> len = end - str + 1;\n>>>> }\n>>>> t = xmalloc(sizeof(struct strbuf));\n>>>> strbuf_init(t, len);\n>>>> - strbuf_add(t, str, len);\n>>>> + strbuf_add(t, str, len - !!end * !!omit_term);\n>>>\n>>> Perhaps using another variable would make it easier to follow?\n>>> Either using a boolean that tells us that the terminating byte\n>>> is to be omitted, i.e.\n>>>\n>>> int len = slen;\n>>> int omit = 0;\n>>> if ( ... we are still splitting ... ) {\n>>> const char *end = memchr(...);\n>>> if (end) {\n>>> len = end - str + 1;\n>>> omit = !!omit_term;\n>>> }\n>>> }\n>>> strbuf_init(t, len - omit);\n>>> strbuf_add(t, str, len - omit);\n>>>\n>>> or an integer \"copylen\" that tells us how many bytes to copy, which\n>>> often is the same as \"len\" but sometimes different by 1 byte?\n>>\n>> This is done based on Eric's suggestion [1]. Although its a little off normal\n>> convention. I find it small and simple. So I'm okay with either, your suggested\n>> change or the existing code.\n>\n> A \"copylen\" variable would probably result in the clearest code since\n> it states explicitly what an otherwise opaque expression like (!!end *\n> !!omit_term) means, thus is easier to reason about.\n\nSure, I think this would do:\n\ndiff --git a/strbuf.c b/strbuf.c\nindex b552a13..81e279d 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- int terminator, int max)\n+ int terminator, int max, int omit_term)\n {\n struct strbuf **ret = NULL;\n size_t nr = 0, alloc = 0;\n@@ -123,14 +123,18 @@ struct strbuf **strbuf_split_buf(const char\n*str, size_t slen,\n\n while (slen) {\n int len = slen;\n+ int copylen = len;\n+ const char *end = NULL;\n if (max <= 0 || nr + 1 < max) {\n- const char *end = memchr(str, terminator, slen);\n- if (end)\n+ end = memchr(str, terminator, slen);\n+ if (end) {\n len = end - str + 1;\n+ copylen = len - !!omit_term;\n+ }\n }\n t = xmalloc(sizeof(struct strbuf));\n- strbuf_init(t, len);\n- strbuf_add(t, str, len);\n+ strbuf_init(t, copylen);\n+ strbuf_add(t, str, copylen);\n ALLOC_GROW(ret, nr + 2, alloc);\n ret[nr++] = t;\n str += len;\n\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"276697","messageId":"CAOLa=ZTpmJECi+ftX_p_3ZXu7zSohPc+umJcw=WepbBBYs1uWA@mail.gmail.com","threadId":"41125","inReplyTo":"CAPig+cSRDhbHmn5NSp_S4J99igFhBEpWfVMBKBDHCWAHN7-8qQ@mail.gmail.com","subject":"Re: [PATCH v3 04/15] ref-filter: introduce struct used_atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-25T06:13:43Z","receivedAt":"2016-01-25T06:13:43Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Fri, Jan 22, 2016 at 12:34 AM, Eric Sunshine wrote:\n> On Tuesday, January 5, 2016, Karthik Nayak wrote:\n>> Introduce the 'used_atom' structure to replace the existing\n>> implementation of 'used_atom' (which is a list of atoms). This helps\n>> us parse atoms beforehand and store required details into the\n>> 'used_atom' for future usage.\n>>\n>> Helped-by: Eric Sunshine \n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> @@ -17,7 +17,7 @@\n>> typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n>>\n>> /*\n>> - * An atom is a valid field atom listed above, possibly prefixed with\n>> + * An atom is a valid field atom listed below, possibly prefixed with\n>\n> This particular change should be in patch 3/15 which moved this comment block.\n>\n> (Yes, it's true that this will make the patch something other than\n> \"pure code movement\", but this change keeps the moved block logically\n> consistent and that it's an appropriate thing to do -- plus it's quite\n> minor.)\n\nWill squeeze that in. Like you said it's too small so it would make\nsense to bundle it\nwithin 3/15.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"276738","messageId":"CAPig+cSUBQiW396qyiczcdPV9qU9o_8imuL89O6jLWLY8sX2-A@mail.gmail.com","threadId":"41125","inReplyTo":"1451980994-26865-11-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v3 10/15] ref-filter: introduce parse_align_position()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-01-25T21:49:03Z","receivedAt":"2016-01-25T21:49:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jan 5, 2016 at 3:03 AM, Karthik Nayak wrote:\n> From align_atom_parser() form parse_align_position() which given a\n> string would give us the alignment position. This is a preparatory\n> patch as to introduce prefixes for the %(align) atom and avoid\n> redundancy in the code.\n>\n> Helped-by: Eric Sunshine \n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> @@ -74,6 +74,17 @@ static void color_atom_parser(struct used_atom *atom)\n> +static align_type parse_align_position(const char *s)\n> +{\n> + if (!strcmp(s, \"right\"))\n> + return ALIGN_RIGHT;\n> + else if (!strcmp(s, \"middle\"))\n> + return ALIGN_MIDDLE;\n> + else if (!strcmp(s, \"left\"))\n> + return ALIGN_LEFT;\n> + return -1;\n> +}\n\nThis code was just moved in patch 9/15 and is being relocated again\nhere in patch 10/15. If you change the order of the patches so that\nthis preparatory refactoring is done first, the diff of the \"introduce\nalign_atom_parser()\" patch will become smaller and be a bit easier to\nreview. (Plus it just makes sense to do preparation first.)\n\n> static void align_atom_parser(struct used_atom *atom)\n> {\n> struct align *align = &atom->u.align;\n> @@ -90,16 +101,13 @@ static void align_atom_parser(struct used_atom *atom)\n> align->position = ALIGN_LEFT;\n>\n> while (*s) {\n> + int position;\n> buf = s[0]->buf;\n>\n> if (!strtoul_ui(buf, 10, (unsigned int *)&width))\n> ;\n> - else if (!strcmp(buf, \"left\"))\n> - align->position = ALIGN_LEFT;\n> - else if (!strcmp(buf, \"right\"))\n> - align->position = ALIGN_RIGHT;\n> - else if (!strcmp(buf, \"middle\"))\n> - align->position = ALIGN_MIDDLE;\n> + else if ((position = parse_align_position(buf)) >= 0)\n> + align->position = position;\n> else\n> die(_(\"unrecognized %%(align) argument: %s\"), buf);\n> s++;\n> --\n> 2.6.4\n"},{"id":"276745","messageId":"CAPig+cT56AiO-3GNzia7UsGZFM5zu5zsEC31XPuq-by1p4+sbw@mail.gmail.com","threadId":"41125","inReplyTo":"1451980994-26865-13-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v3 12/15] ref-filter: align: introduce long-form syntax","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-01-25T22:58:22Z","receivedAt":"2016-01-25T22:58:22Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jan 5, 2016 at 3:03 AM, Karthik Nayak wrote:\n> Introduce optional prefixes \"width=\" and \"position=\" for the align atom\n> so that the atom can be used as \"%(align:width=,position=)\".\n>\n> Add Documetation and tests for the same.\n>\n> Helped-by: Eric Sunshine \n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\n> index fe4796c..0c4417f 100755\n> --- a/t/t6302-for-each-ref-filter.sh\n> +++ b/t/t6302-for-each-ref-filter.sh\n> @@ -133,6 +133,47 @@ test_expect_success 'right alignment' '\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\nStyle: in shell scripts, add space before ()\n\n> + while read -r option; do\n\nStyle: drop semi-colon and place 'do' on its own line\n\n> + test_expect_success 'align permutations' '\n\nThis title is not as illuminating as it could be. It would be better\nto indicate which permutation is being tested. For instance:\n\n test_expect_success \"align:$option\" ...\n\nNote the double-quotes. Or:\n\n test_expect_success \"align permutation: $option\" ...\n\nor something.\n\n> + git for-each-ref --format=\"|%(align:$option)refname is %(refname)%(end)|%(refname)\" >actual &&\n\nThis is not wrong, per se, but referencing $option inside the\nnon-interpolating single-quote context of the test body makes it a bit\nharder to understand than it need be. As it is, at the time that the\ntest body actually gets executed, $option still evaluates to the\ndesired permutation value so it works. However, it would show intent\nmore clearly and be more robust to use a double-quote context to\ninterpolate $option into the git-for-each-ref invocation directly\nrather than allowing the test body to pick up the value at execution\ntime.\n\nFixing this means using double- rather than single-quotes for the test\nbody, which means you'd also want to flip the double-quotes wrapping\nthe --format= argument over to single-quotes. Also, for style\nconsistency, indent the test body. The end result should be something\nlike this:\n\n test_align_permutations () {\n while ...\n do\n test_expect_success \"align:$option\" \"\n git for-each-ref --format='...$option...' >actual &&\n ...\n \"\n done\n }\n\n> + test_cmp expect actual\n> + '\n> + done;\n\nStyle: drop the semi-colon\n\nMore below...\n\n> +}\n> +\n> +test_align_permutations <<-\\EOF\n> + middle,42\n> + 42,middle\n> + position=middle,42\n> + 42,position=middle\n> + middle,width=42\n> + width=42,middle\n> + position=middle,width=42\n> + width=42,position=middle\n> +EOF\n> +\n> +# Last one wins (silently) when multiple arguments of the same type are given\n> +\n> +test_align_permutations <<-\\EOF\n> + 32,width=42,middle\n> + width=30,42,middle\n> + width=42,position=right,middle\n> + 42,right,position=middle\n> +EOF\n\nOverall, this version is much nicer now that the tests are\ntable-driven rather than each permutation being copied/pasted.\n\nThis is a tangent, but it's disappointing that this entire test script\nis skipped if GPG is not installed, especially since none of these\ntests seem to care at all about signed tags. By requiring GPG\n(unnecessarily), these tests likely are not getting run as widely as\nthey ought to. Consequently, it probably would be a good idea to drop\nthe GPG requirement from the top of the file and have the \"setup\" test\ncreate lightweight tags (\"git tag ...\") rather than signed ones (\"git\ntag -s ...\").\n"},{"id":"276748","messageId":"xmqqa8ntdxzs.fsf@gitster.mtv.corp.google.com","threadId":"41125","inReplyTo":"CAPig+cT56AiO-3GNzia7UsGZFM5zu5zsEC31XPuq-by1p4+sbw@mail.gmail.com","subject":"Re: [PATCH v3 12/15] ref-filter: align: introduce long-form syntax","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-25T23:45:43Z","receivedAt":"2016-01-25T23:45:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> + git for-each-ref --format=\"|%(align:$option)refname is %(refname)%(end)|%(refname)\" >actual &&\n>\n> This is not wrong, per se, but referencing $option inside the\n> non-interpolating single-quote context of the test body makes it a bit\n> harder to understand than it need be. As it is, at the time that the\n> test body actually gets executed, $option still evaluates to the\n> desired permutation value so it works. \n\nI think I said this in one of my recent reviews to somebody else,\nbut this needs repeating.\n\nMy position on this has been and still is a complete opposite. It\nis a designed behaviour that variable references made inside test\nbody that is quoted with sq pair can see the values of the variables\nthat are defined outside test_expect_success, and you can freely use\nthe global $TRASH_DIRECTORY, $_x40, etc. inside the test body that\nis quoted with single quote. The $option here is no different.\n\nIn fact, it is more desirable to avoid double-quoting the test body.\nThe problem with using double-quote around the test body and\nformulating the executable shell script using variable interpolation\nbefore test_expect_success even runs is that you would be tempted to\ndo something silly like this:\n\n HEAD_SHA1=$(git rev-parse --verify HEAD)\n\n test_expect_success \"check something\" \"\n\tHEAD_SHA1=$(git rev-parse --verify HEAD) &&\n\t... do other things that should not move the HEAD ... &&\n\ttest '$(git rev-parse HEAD)' = '$HEAD_SHA1' &&\n\ttest '$SOME_VAR' = '$OTHER_VAR' &&\n \"\n\nIt is not just error prone (if variable reference had a shell\nmetacharacter in it, e.g. a single-quote in OTHER_VAR's value, you'd\nhave a syntax error in the test body), but because two invocations\nof rev-parse in the above are both made before test_expect_success\nruns, and would yield the same value. It is not even testing the\nright thing. If you broke rev-parse, the invocation will not fail\ninside test_expect_success but outside when the arguments to that\nhelper is being prepared.\n\nSo please do not write something like this:\n\n> test_align_permutations () {\n> while ...\n> do\n> test_expect_success \"align:$option\" \"\n> git for-each-ref --format='...$option...' >actual &&\n> ...\n> \"\n> done\n> }\n\nbut make it more like this:\n\n\tfor option in ...\n do\n \ttest_expect_success \"align:$option\" '\n \tgit for-each-ref --format=\"...$option...\" &&\n ...\n\t\t'\n\tdone\n"},{"id":"276750","messageId":"CAPig+cTLxhrh3d0ouGLGq+ke8mzFKqCz0ABAECSA4vS+=epv9Q@mail.gmail.com","threadId":"41125","inReplyTo":"1451980994-26865-14-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v3 13/15] ref-filter: introduce remote_ref_atom_parser()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-01-26T00:28:32Z","receivedAt":"2016-01-26T00:28:32Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jan 5, 2016 at 3:03 AM, Karthik Nayak wrote:\n> Introduce remote_ref_atom_parser() which will parse the '%(upstream)'\n> and '%(push)' atoms and store information into the 'used_atom'\n> structure based on the modifiers used along with the corresponding\n> atom.\n>\n> Helped-by: Ramsay Jones \n> Helped-by: Eric Sunshine \n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> @@ -841,6 +863,43 @@ static inline char *copy_advance(char *dst, const char *src)\n> +static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n> + struct branch *branch, const char **s)\n> +{\n> + int num_ours, num_theirs;\n> + if (atom->u.remote_ref == RR_SHORTEN)\n> + *s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);\n> + else if (atom->u.remote_ref == RR_TRACK) {\n> + if (stat_tracking_info(branch, &num_ours,\n> + &num_theirs, NULL))\n> + return;\n> +\n> + if (!num_ours && !num_theirs)\n> + *s = \"\";\n> + else if (!num_ours)\n> + *s = xstrfmt(\"[behind %d]\", num_theirs);\n> + else if (!num_theirs)\n> + *s = xstrfmt(\"[ahead %d]\", num_ours);\n> + else\n> + *s = xstrfmt(\"[ahead %d, behind %d]\",\n> + num_ours, num_theirs);\n> + } else if (atom->u.remote_ref == RR_TRACKSHORT) {\n> + if (stat_tracking_info(branch, &num_ours,\n> + &num_theirs, NULL))\n> + return;\n> +\n> + if (!num_ours && !num_theirs)\n> + *s = \"=\";\n> + else if (!num_ours)\n> + *s = \"<\";\n> + else if (!num_theirs)\n> + *s = \">\";\n> + else\n> + *s = \"<>\";\n> + } else if (atom->u.remote_ref == RR_NORMAL)\n> + *s = refname;\n\nI think I mentioned this in a previous review: If the code falls past\nthis final 'else if' for some reason (programmer error), then *s won't\nget assigned at all, which is probably undesirable. To protect against\nsuch a case, you might want either to add a final 'else':\n\n else\n die(\"BUG: ...\");\n\nor just consider RR_NORMAL the catchall case, and turn the final 'else\nif' into a plain 'else':\n\n else /* RR_NORMAL */\n *s = refname;\n\n> +}\n> @@ -894,6 +953,8 @@ static void populate_value(struct ref_array_item *ref)\n> refname = branch_get_upstream(branch, NULL);\n> if (!refname)\n> continue;\n> + fill_remote_ref_details(atom, refname, branch, &v->s);\n> + continue;\n\nThere are now two 'continue' statements very close together here. Have\nyou considered this instead?\n\n if (refname)\n fill_remote_ref_details(...);\n continue;\n\nIt might make the code a bit more straightforward. (Genuine question;\nI don't feel too strongly about it.)\n\n> } else if (starts_with(name, \"push\")) {\n> const char *branch_name;\n> if (!skip_prefix(ref->refname, \"refs/heads/\",\n> @@ -904,6 +965,8 @@ static void populate_value(struct ref_array_item *ref)\n> refname = branch_get_push(branch, NULL);\n> if (!refname)\n> continue;\n> + fill_remote_ref_details(atom, refname, branch, &v->s);\n> + continue;\n\nDitto.\n\n> } else if (starts_with(name, \"color:\")) {\n> v->s = atom->u.color;\n> continue;\n"},{"id":"276756","messageId":"CAP8UFD0NUoZtJxo_+M2U20AZcHEZiwPms6TBAU_xqBU0+zSk4Q@mail.gmail.com","threadId":"41125","inReplyTo":"1451980994-26865-13-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v3 12/15] ref-filter: align: introduce long-form syntax","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-01-26T05:16:24Z","receivedAt":"2016-01-26T05:16:24Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Jan 5, 2016 at 9:03 AM, Karthik Nayak wrote:\n> Introduce optional prefixes \"width=\" and \"position=\" for the align atom\n> so that the atom can be used as \"%(align:width=,position=)\".\n>\n> Add Documetation and tests for the same.\n\ns/Documetation/Documentation/\n\nThanks!\n"},{"id":"276769","messageId":"CAOLa=ZSQJQXEQ05+jqDBCp3QUBB9VYFVbaMT+k228HKKZkXKnA@mail.gmail.com","threadId":"41125","inReplyTo":"CAPig+cT56AiO-3GNzia7UsGZFM5zu5zsEC31XPuq-by1p4+sbw@mail.gmail.com","subject":"Re: [PATCH v3 12/15] ref-filter: align: introduce long-form syntax","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-26T09:30:41Z","receivedAt":"2016-01-26T09:30:41Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Jan 26, 2016 at 4:28 AM, Eric Sunshine wrote:\n> On Tue, Jan 5, 2016 at 3:03 AM, Karthik Nayak wrote:\n>> Introduce optional prefixes \"width=\" and \"position=\" for the align atom\n>> so that the atom can be used as \"%(align:width=,position=)\".\n>>\n>> Add Documetation and tests for the same.\n>>\n>> Helped-by: Eric Sunshine \n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\n>> index fe4796c..0c4417f 100755\n>> --- a/t/t6302-for-each-ref-filter.sh\n>> +++ b/t/t6302-for-each-ref-filter.sh\n>> @@ -133,6 +133,47 @@ test_expect_success 'right alignment' '\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>\n> Style: in shell scripts, add space before ()\n>\n\nWill change.\n\n>> + while read -r option; do\n>\n> Style: drop semi-colon and place 'do' on its own line\n\nWill change.\n\n>\n>> + test_expect_success 'align permutations' '\n>\n> This title is not as illuminating as it could be. It would be better\n> to indicate which permutation is being tested. For instance:\n>\n> test_expect_success \"align:$option\" ...\n>\n> Note the double-quotes. Or:\n>\n> test_expect_success \"align permutation: $option\" ...\n>\n> or something.\n>\n\n\"align:$option\" seems right, will change.\n\n>> + git for-each-ref --format=\"|%(align:$option)refname is %(refname)%(end)|%(refname)\" >actual &&\n>\n> This is not wrong, per se, but referencing $option inside the\n> non-interpolating single-quote context of the test body makes it a bit\n> harder to understand than it need be. As it is, at the time that the\n> test body actually gets executed, $option still evaluates to the\n> desired permutation value so it works. However, it would show intent\n> more clearly and be more robust to use a double-quote context to\n> interpolate $option into the git-for-each-ref invocation directly\n> rather than allowing the test body to pick up the value at execution\n> time.\n>\n> Fixing this means using double- rather than single-quotes for the test\n> body, which means you'd also want to flip the double-quotes wrapping\n> the --format= argument over to single-quotes. Also, for style\n> consistency, indent the test body. The end result should be something\n> like this:\n>\n> test_align_permutations () {\n> while ...\n> do\n> test_expect_success \"align:$option\" \"\n> git for-each-ref --format='...$option...' >actual &&\n> ...\n> \"\n> done\n> }\n>\n\nAfter reading this and Junio's detailed description stating otherwise.\nIt's safe to\ngo with something similar to what Junio suggested :\n\n for option in ...\n do\n test_expect_success \"align:$option\" '\n git for-each-ref --format=\"...$option...\" &&\n ...\n '\n done\n\n>> + test_cmp expect actual\n>> + '\n>> + done;\n>\n> Style: drop the semi-colon\n>\n\nWill change.\n\n> More below...\n>\n>> +}\n>> +\n>> +test_align_permutations <<-\\EOF\n>> + middle,42\n>> + 42,middle\n>> + position=middle,42\n>> + 42,position=middle\n>> + middle,width=42\n>> + width=42,middle\n>> + position=middle,width=42\n>> + width=42,position=middle\n>> +EOF\n>> +\n>> +# Last one wins (silently) when multiple arguments of the same type are given\n>> +\n>> +test_align_permutations <<-\\EOF\n>> + 32,width=42,middle\n>> + width=30,42,middle\n>> + width=42,position=right,middle\n>> + 42,right,position=middle\n>> +EOF\n>\n> Overall, this version is much nicer now that the tests are\n> table-driven rather than each permutation being copied/pasted.\n>\n\nThanks.\n\n> This is a tangent, but it's disappointing that this entire test script\n> is skipped if GPG is not installed, especially since none of these\n> tests seem to care at all about signed tags. By requiring GPG\n> (unnecessarily), these tests likely are not getting run as widely as\n> they ought to. Consequently, it probably would be a good idea to drop\n> the GPG requirement from the top of the file and have the \"setup\" test\n> create lightweight tags (\"git tag ...\") rather than signed ones (\"git\n> tag -s ...\").\n\nMaybe an independent patch post this series. I'll keep a note to myself.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"276774","messageId":"CAOLa=ZSVJXqNUELV-01xFPOhSG9zbFf8iY70NDMB5=2DF420zw@mail.gmail.com","threadId":"41125","inReplyTo":"CAP8UFD0NUoZtJxo_+M2U20AZcHEZiwPms6TBAU_xqBU0+zSk4Q@mail.gmail.com","subject":"Re: [PATCH v3 12/15] ref-filter: align: introduce long-form syntax","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-26T09:39:57Z","receivedAt":"2016-01-26T09:39:57Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Jan 26, 2016 at 10:46 AM, Christian Couder\n wrote:\n> On Tue, Jan 5, 2016 at 9:03 AM, Karthik Nayak wrote:\n>> Introduce optional prefixes \"width=\" and \"position=\" for the align atom\n>> so that the atom can be used as \"%(align:width=,position=)\".\n>>\n>> Add Documetation and tests for the same.\n>\n> s/Documetation/Documentation/\n>\n> Thanks!\n\nAh! thanks for that.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"276775","messageId":"CAOLa=ZSnDe=q8SpicYW6gNKmNts3K=FpCFq6q8t_pUv+8AZhnQ@mail.gmail.com","threadId":"41125","inReplyTo":"xmqqa8ntdxzs.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 12/15] ref-filter: align: introduce long-form syntax","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-26T09:40:44Z","receivedAt":"2016-01-26T09:40:44Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Jan 26, 2016 at 5:15 AM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>\n>>> + git for-each-ref --format=\"|%(align:$option)refname is %(refname)%(end)|%(refname)\" >actual &&\n>>\n>> This is not wrong, per se, but referencing $option inside the\n>> non-interpolating single-quote context of the test body makes it a bit\n>> harder to understand than it need be. As it is, at the time that the\n>> test body actually gets executed, $option still evaluates to the\n>> desired permutation value so it works.\n>\n> I think I said this in one of my recent reviews to somebody else,\n> but this needs repeating.\n>\n> My position on this has been and still is a complete opposite. It\n> is a designed behaviour that variable references made inside test\n> body that is quoted with sq pair can see the values of the variables\n> that are defined outside test_expect_success, and you can freely use\n> the global $TRASH_DIRECTORY, $_x40, etc. inside the test body that\n> is quoted with single quote. The $option here is no different.\n>\n> In fact, it is more desirable to avoid double-quoting the test body.\n> The problem with using double-quote around the test body and\n> formulating the executable shell script using variable interpolation\n> before test_expect_success even runs is that you would be tempted to\n> do something silly like this:\n>\n> HEAD_SHA1=$(git rev-parse --verify HEAD)\n>\n> test_expect_success \"check something\" \"\n> HEAD_SHA1=$(git rev-parse --verify HEAD) &&\n> ... do other things that should not move the HEAD ... &&\n> test '$(git rev-parse HEAD)' = '$HEAD_SHA1' &&\n> test '$SOME_VAR' = '$OTHER_VAR' &&\n> \"\n>\n> It is not just error prone (if variable reference had a shell\n> metacharacter in it, e.g. a single-quote in OTHER_VAR's value, you'd\n> have a syntax error in the test body), but because two invocations\n> of rev-parse in the above are both made before test_expect_success\n> runs, and would yield the same value. It is not even testing the\n> right thing. If you broke rev-parse, the invocation will not fail\n> inside test_expect_success but outside when the arguments to that\n> helper is being prepared.\n>\n> So please do not write something like this:\n>\n>> test_align_permutations () {\n>> while ...\n>> do\n>> test_expect_success \"align:$option\" \"\n>> git for-each-ref --format='...$option...' >actual &&\n>> ...\n>> \"\n>> done\n>> }\n>\n> but make it more like this:\n>\n> for option in ...\n> do\n> test_expect_success \"align:$option\" '\n> git for-each-ref --format=\"...$option...\" &&\n> ...\n> '\n> done\n>\n\nThanks for this descriptive explanation, I shall use the suggested format.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"276780","messageId":"CAOLa=ZQNqia7j8bESD47xAQVW3_7dX6Qw8swwLL3WtesN5BEBg@mail.gmail.com","threadId":"41125","inReplyTo":"CAPig+cTLxhrh3d0ouGLGq+ke8mzFKqCz0ABAECSA4vS+=epv9Q@mail.gmail.com","subject":"Re: [PATCH v3 13/15] ref-filter: introduce remote_ref_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-26T10:02:25Z","receivedAt":"2016-01-26T10:02:25Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Jan 26, 2016 at 5:58 AM, Eric Sunshine wrote:\n> On Tue, Jan 5, 2016 at 3:03 AM, Karthik Nayak wrote:\n>> Introduce remote_ref_atom_parser() which will parse the '%(upstream)'\n>> and '%(push)' atoms and store information into the 'used_atom'\n>> structure based on the modifiers used along with the corresponding\n>> atom.\n>>\n>> Helped-by: Ramsay Jones \n>> Helped-by: Eric Sunshine \n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> @@ -841,6 +863,43 @@ static inline char *copy_advance(char *dst, const char *src)\n>> +static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n>> + struct branch *branch, const char **s)\n>> +{\n>> + int num_ours, num_theirs;\n>> + if (atom->u.remote_ref == RR_SHORTEN)\n>> + *s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);\n>> + else if (atom->u.remote_ref == RR_TRACK) {\n>> + if (stat_tracking_info(branch, &num_ours,\n>> + &num_theirs, NULL))\n>> + return;\n>> +\n>> + if (!num_ours && !num_theirs)\n>> + *s = \"\";\n>> + else if (!num_ours)\n>> + *s = xstrfmt(\"[behind %d]\", num_theirs);\n>> + else if (!num_theirs)\n>> + *s = xstrfmt(\"[ahead %d]\", num_ours);\n>> + else\n>> + *s = xstrfmt(\"[ahead %d, behind %d]\",\n>> + num_ours, num_theirs);\n>> + } else if (atom->u.remote_ref == RR_TRACKSHORT) {\n>> + if (stat_tracking_info(branch, &num_ours,\n>> + &num_theirs, NULL))\n>> + return;\n>> +\n>> + if (!num_ours && !num_theirs)\n>> + *s = \"=\";\n>> + else if (!num_ours)\n>> + *s = \"<\";\n>> + else if (!num_theirs)\n>> + *s = \">\";\n>> + else\n>> + *s = \"<>\";\n>> + } else if (atom->u.remote_ref == RR_NORMAL)\n>> + *s = refname;\n>\n> I think I mentioned this in a previous review: If the code falls past\n> this final 'else if' for some reason (programmer error), then *s won't\n> get assigned at all, which is probably undesirable. To protect against\n> such a case, you might want either to add a final 'else':\n>\n> else\n> die(\"BUG: ...\");\n>\n> or just consider RR_NORMAL the catchall case, and turn the final 'else\n> if' into a plain 'else':\n>\n> else /* RR_NORMAL */\n> *s = refname;\n>\n\nThe latter seems to make sense, will implement.\n\n>> +}\n>> @@ -894,6 +953,8 @@ static void populate_value(struct ref_array_item *ref)\n>> refname = branch_get_upstream(branch, NULL);\n>> if (!refname)\n>> continue;\n>> + fill_remote_ref_details(atom, refname, branch, &v->s);\n>> + continue;\n>\n> There are now two 'continue' statements very close together here. Have\n> you considered this instead?\n>\n> if (refname)\n> fill_remote_ref_details(...);\n> continue;\n>\n> It might make the code a bit more straightforward. (Genuine question;\n> I don't feel too strongly about it.)\n\nNo, I didn't consider that, and I think it's a good change to implement.\nThanks. ( I don't see any problems with it)\n\n>\n>> } else if (starts_with(name, \"push\")) {\n>> const char *branch_name;\n>> if (!skip_prefix(ref->refname, \"refs/heads/\",\n>> @@ -904,6 +965,8 @@ static void populate_value(struct ref_array_item *ref)\n>> refname = branch_get_push(branch, NULL);\n>> if (!refname)\n>> continue;\n>> + fill_remote_ref_details(atom, refname, branch, &v->s);\n>> + continue;\n>\n> Ditto.\n\nNoted.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"276781","messageId":"CAOLa=ZS8MP_88Ox9YUKsPUC0hDw5YGoHR=JhY4Jx6ip80L=DWg@mail.gmail.com","threadId":"41125","inReplyTo":"CAPig+cSUBQiW396qyiczcdPV9qU9o_8imuL89O6jLWLY8sX2-A@mail.gmail.com","subject":"Re: [PATCH v3 10/15] ref-filter: introduce parse_align_position()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-26T11:34:56Z","receivedAt":"2016-01-26T11:34:56Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Jan 26, 2016 at 3:19 AM, Eric Sunshine wrote:\n> On Tue, Jan 5, 2016 at 3:03 AM, Karthik Nayak wrote:\n>> From align_atom_parser() form parse_align_position() which given a\n>> string would give us the alignment position. This is a preparatory\n>> patch as to introduce prefixes for the %(align) atom and avoid\n>> redundancy in the code.\n>>\n>> Helped-by: Eric Sunshine \n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> @@ -74,6 +74,17 @@ static void color_atom_parser(struct used_atom *atom)\n>> +static align_type parse_align_position(const char *s)\n>> +{\n>> + if (!strcmp(s, \"right\"))\n>> + return ALIGN_RIGHT;\n>> + else if (!strcmp(s, \"middle\"))\n>> + return ALIGN_MIDDLE;\n>> + else if (!strcmp(s, \"left\"))\n>> + return ALIGN_LEFT;\n>> + return -1;\n>> +}\n>\n> This code was just moved in patch 9/15 and is being relocated again\n> here in patch 10/15. If you change the order of the patches so that\n> this preparatory refactoring is done first, the diff of the \"introduce\n> align_atom_parser()\" patch will become smaller and be a bit easier to\n> review. (Plus it just makes sense to do preparation first.)\n>\n\nIt does reduce the diff as you said, I have swapped the order as you suggested.\n\n-- \nRegards,\nKarthik Nayak\n"}]}