{"thread":{"id":"40776","subject":"[PATCH/RFC 00/10] ref-filter: use parsing functions","startedAt":"2015-11-11T19:44:26Z","lastAt":"2015-12-16T14:45:19Z","messageCount":50,"participants":["Karthik Nayak","Eric Sunshine","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":10},"messages":[{"id":"273207","messageId":"1447271075-15364-1-git-send-email-Karthik.188@gmail.com","threadId":"40776","inReplyTo":null,"subject":"[PATCH/RFC 00/10] ref-filter: use parsing functions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-11-11T19:44:26Z","receivedAt":"2015-11-11T19:44:26Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Carried over\nhttp://thread.gmane.org/gmane.comp.version-control.git/279226/focus=279352. Where\nwe were talking about pre-parsing most of the atoms so that we do not\nhave to parse them in ref-filter:populate_value(), where we could now\ninstead only fill in necessary values. This series aims to introduce\nparsing functions for atoms so that they maybe parsed before hand and\nthe necessary values maybe stored the introduce used_atom structure.\n\nKarthik Nayak (10):\n ref-filter: introduce a parsing function for each atom in valid_atom\n ref-filter: introduce struct used_atom\n ref-fitler: bump match_atom() name to the top\n ref-filter: skip deref specifier in match_atom_name()\n ref-filter: introduce color_atom_parser()\n strbuf: introduce strbuf_split_str_without_term()\n ref-filter: introduce align_atom_parser()\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 | 18 +-\n ref-filter.c | 483 ++++++++++++++++++++++---------------\n strbuf.c | 17 +-\n strbuf.h | 14 +-\n t/t6302-for-each-ref-filter.sh | 4 +-\n 5 files changed, 328 insertions(+), 208 deletions(-)\n\n-- \n2.6.2\n"},{"id":"273208","messageId":"1447271075-15364-2-git-send-email-Karthik.188@gmail.com","threadId":"40776","inReplyTo":"1447271075-15364-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH/RFC 01/10] ref-filter: introduce a parsing function for each atom in valid_atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-11-11T19:44:27Z","receivedAt":"2015-11-11T19:44:27Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Introduce a parsing function for each atom in valid_atom. Using this\nwe can define special parsing functions for each of the atoms. Since\nwe have a third field in valid_atom structure, we now fill out missing\ncmp_type values.\n\nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 59 ++++++++++++++++++++++++++++++-----------------------------\n 1 file changed, 30 insertions(+), 29 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex e205dd2..fbbda17 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -19,42 +19,43 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\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+\t{ \"refname\", FIELD_STR },\n+\t{ \"objecttype\", FIELD_STR },\n \t{ \"objectsize\", FIELD_ULONG },\n-\t{ \"objectname\" },\n-\t{ \"tree\" },\n-\t{ \"parent\" },\n+\t{ \"objectname\", FIELD_STR },\n+\t{ \"tree\", FIELD_STR },\n+\t{ \"parent\", FIELD_STR },\n \t{ \"numparent\", FIELD_ULONG },\n-\t{ \"object\" },\n-\t{ \"type\" },\n-\t{ \"tag\" },\n-\t{ \"author\" },\n-\t{ \"authorname\" },\n-\t{ \"authoremail\" },\n+\t{ \"object\", FIELD_STR },\n+\t{ \"type\", FIELD_STR },\n+\t{ \"tag\", FIELD_STR },\n+\t{ \"author\", FIELD_STR },\n+\t{ \"authorname\", FIELD_STR },\n+\t{ \"authoremail\", FIELD_STR },\n \t{ \"authordate\", FIELD_TIME },\n-\t{ \"committer\" },\n-\t{ \"committername\" },\n-\t{ \"committeremail\" },\n+\t{ \"committer\", FIELD_STR },\n+\t{ \"committername\", FIELD_STR },\n+\t{ \"committeremail\", FIELD_STR },\n \t{ \"committerdate\", FIELD_TIME },\n-\t{ \"tagger\" },\n-\t{ \"taggername\" },\n-\t{ \"taggeremail\" },\n+\t{ \"tagger\", FIELD_STR },\n+\t{ \"taggername\", FIELD_STR },\n+\t{ \"taggeremail\", FIELD_STR },\n \t{ \"taggerdate\", FIELD_TIME },\n-\t{ \"creator\" },\n+\t{ \"creator\", FIELD_STR },\n \t{ \"creatordate\", FIELD_TIME },\n-\t{ \"subject\" },\n-\t{ \"body\" },\n-\t{ \"contents\" },\n-\t{ \"upstream\" },\n-\t{ \"push\" },\n-\t{ \"symref\" },\n-\t{ \"flag\" },\n-\t{ \"HEAD\" },\n-\t{ \"color\" },\n-\t{ \"align\" },\n-\t{ \"end\" },\n+\t{ \"subject\", FIELD_STR },\n+\t{ \"body\", FIELD_STR },\n+\t{ \"contents\", FIELD_STR },\n+\t{ \"upstream\", FIELD_STR },\n+\t{ \"push\", FIELD_STR },\n+\t{ \"symref\", FIELD_STR },\n+\t{ \"flag\", FIELD_STR },\n+\t{ \"HEAD\", FIELD_STR },\n+\t{ \"color\", FIELD_STR },\n+\t{ \"align\", FIELD_STR },\n+\t{ \"end\", FIELD_STR },\n };\n \n #define REF_FORMATTING_STATE_INIT { 0, NULL }\n-- \n2.6.2\n"},{"id":"273210","messageId":"1447271075-15364-3-git-send-email-Karthik.188@gmail.com","threadId":"40776","inReplyTo":"1447271075-15364-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH/RFC 02/10] ref-filter: introduce struct used_atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-11-11T19:44:28Z","receivedAt":"2015-11-11T19:44:28Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Introduce the 'used_array' structure which would replace the existing\nimplementation of 'used_array' (which a list of atoms). This helps us\nparse atom's before hand and store required details into the\n'used_array' for future usage.\n\nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 62 +++++++++++++++++++++++++++++++-----------------------------\n 1 file changed, 32 insertions(+), 30 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex fbbda17..748a2fe 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -16,6 +16,23 @@\n \n typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n \n+/*\n+ * An atom is a valid field atom listed below, possibly prefixed with\n+ * a \"*\" to denote deref_tag().\n+ *\n+ * We parse given format string and sort specifiers, and make a list\n+ * of properties that we need to extract out of objects. ref_array_item\n+ * structure will hold an array of values extracted that can be\n+ * indexed with the \"atom number\", which is an index into this\n+ * array.\n+ */\n+static struct used_atom {\n+\tconst char *str;\n+\tcmp_type type;\n+} *used_atom;\n+static int used_atom_cnt, need_tagged, need_symref;\n+static int need_color_reset_at_eol;\n+\n static struct {\n \tconst char *name;\n \tcmp_type cmp_type;\n@@ -93,21 +110,6 @@ struct atom_value {\n };\n \n /*\n- * An atom is a valid field atom listed above, possibly prefixed with\n- * a \"*\" to denote deref_tag().\n- *\n- * We parse given format string and sort specifiers, and make a list\n- * of properties that we need to extract out of objects. ref_array_item\n- * structure will hold an array of values extracted that can be\n- * indexed with the \"atom number\", which is an index into this\n- * array.\n- */\n-static const char **used_atom;\n-static cmp_type *used_atom_type;\n-static int used_atom_cnt, need_tagged, need_symref;\n-static int need_color_reset_at_eol;\n-\n-/*\n * Used to parse format string and sort specifiers\n */\n int parse_ref_filter_atom(const char *atom, const char *ep)\n@@ -123,8 +125,8 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n \n \t/* Do we have the atom already used elsewhere? */\n \tfor (i = 0; i < used_atom_cnt; i++) {\n-\t\tint len = strlen(used_atom[i]);\n-\t\tif (len == ep - atom && !memcmp(used_atom[i], atom, len))\n+\t\tint len = strlen(used_atom[i].str);\n+\t\tif (len == ep - atom && !memcmp(used_atom[i].str, atom, len))\n \t\t\treturn i;\n \t}\n \n@@ -151,12 +153,11 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n \tat = used_atom_cnt;\n \tused_atom_cnt++;\n \tREALLOC_ARRAY(used_atom, used_atom_cnt);\n-\tREALLOC_ARRAY(used_atom_type, used_atom_cnt);\n-\tused_atom[at] = xmemdupz(atom, ep - atom);\n-\tused_atom_type[at] = valid_atom[i].cmp_type;\n+\tused_atom[at].str = xmemdupz(atom, ep - atom);\n+\tused_atom[at].type = valid_atom[i].cmp_type;\n \tif (*atom == '*')\n \t\tneed_tagged = 1;\n-\tif (!strcmp(used_atom[at], \"symref\"))\n+\tif (!strcmp(used_atom[at].str, \"symref\"))\n \t\tneed_symref = 1;\n \treturn at;\n }\n@@ -316,7 +317,7 @@ int verify_ref_format(const char *format)\n \t\tat = parse_ref_filter_atom(sp + 2, ep);\n \t\tcp = ep + 1;\n \n-\t\tif (skip_prefix(used_atom[at], \"color:\", &color))\n+\t\tif (skip_prefix(used_atom[at].str, \"color:\", &color))\n \t\t\tneed_color_reset_at_eol = !!strcmp(color, \"reset\");\n \t}\n \treturn 0;\n@@ -360,7 +361,7 @@ static void grab_common_values(struct atom_value *val, int deref, struct object\n \tint i;\n \n \tfor (i = 0; i < used_atom_cnt; i++) {\n-\t\tconst char *name = used_atom[i];\n+\t\tconst char *name = used_atom[i].str;\n \t\tstruct atom_value *v = &val[i];\n \t\tif (!!deref != (*name == '*'))\n \t\t\tcontinue;\n@@ -384,7 +385,7 @@ static void grab_tag_values(struct atom_value *val, int deref, struct object *ob\n \tstruct tag *tag = (struct tag *) obj;\n \n \tfor (i = 0; i < used_atom_cnt; i++) {\n-\t\tconst char *name = used_atom[i];\n+\t\tconst char *name = used_atom[i].str;\n \t\tstruct atom_value *v = &val[i];\n \t\tif (!!deref != (*name == '*'))\n \t\t\tcontinue;\n@@ -406,7 +407,7 @@ static void grab_commit_values(struct atom_value *val, int deref, struct object\n \tstruct commit *commit = (struct commit *) obj;\n \n \tfor (i = 0; i < used_atom_cnt; i++) {\n-\t\tconst char *name = used_atom[i];\n+\t\tconst char *name = used_atom[i].str;\n \t\tstruct atom_value *v = &val[i];\n \t\tif (!!deref != (*name == '*'))\n \t\t\tcontinue;\n@@ -536,7 +537,7 @@ static void grab_person(const char *who, struct atom_value *val, int deref, stru\n \tconst char *wholine = NULL;\n \n \tfor (i = 0; i < used_atom_cnt; i++) {\n-\t\tconst char *name = used_atom[i];\n+\t\tconst char *name = used_atom[i].str;\n \t\tstruct atom_value *v = &val[i];\n \t\tif (!!deref != (*name == '*'))\n \t\t\tcontinue;\n@@ -574,7 +575,7 @@ static void grab_person(const char *who, struct atom_value *val, int deref, stru\n \tif (!wholine)\n \t\treturn;\n \tfor (i = 0; i < used_atom_cnt; i++) {\n-\t\tconst char *name = used_atom[i];\n+\t\tconst char *name = used_atom[i].str;\n \t\tstruct atom_value *v = &val[i];\n \t\tif (!!deref != (*name == '*'))\n \t\t\tcontinue;\n@@ -664,7 +665,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj\n \tunsigned long sublen = 0, bodylen = 0, nonsiglen = 0, siglen = 0;\n \n \tfor (i = 0; i < used_atom_cnt; i++) {\n-\t\tconst char *name = used_atom[i];\n+\t\tconst char *name = used_atom[i].str;\n \t\tstruct atom_value *v = &val[i];\n \t\tconst char *valp = NULL;\n \t\tif (!!deref != (*name == '*'))\n@@ -787,7 +788,8 @@ static void populate_value(struct ref_array_item *ref)\n \n \t/* Fill in specials first */\n \tfor (i = 0; i < used_atom_cnt; i++) {\n-\t\tconst char *name = used_atom[i];\n+\t\tstruct used_atom *atom = &used_atom[i];\n+\t\tconst char *name = atom->str;\n \t\tstruct atom_value *v = &ref->value[i];\n \t\tint deref = 0;\n \t\tconst char *refname;\n@@ -1446,7 +1448,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.2\n"},{"id":"273212","messageId":"1447271075-15364-4-git-send-email-Karthik.188@gmail.com","threadId":"40776","inReplyTo":"1447271075-15364-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH/RFC 03/10] ref-fitler: bump match_atom() name to the top","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-11-11T19:44:29Z","receivedAt":"2015-11-11T19:44:29Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Bump match_atom() to the top for usage in further patches.\n\nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 32 ++++++++++++++++----------------\n 1 file changed, 16 insertions(+), 16 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 748a2fe..1542f5f 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -33,6 +33,22 @@ static struct used_atom {\n static int used_atom_cnt, need_tagged, need_symref;\n static int need_color_reset_at_eol;\n \n+static int match_atom_name(const char *name, const char *atom_name, const char **val)\n+{\n+\tconst char *body;\n+\n+\tif (!skip_prefix(name, atom_name, &body))\n+\t\treturn 0; /* doesn't even begin with \"atom_name\" */\n+\tif (!body[0]) {\n+\t\t*val = NULL; /* %(atom_name) and no customization */\n+\t\treturn 1;\n+\t}\n+\tif (body[0] != ':')\n+\t\treturn 0; /* \"atom_namefoo\" is not \"atom_name\" or \"atom_name:...\" */\n+\t*val = body + 1; /* \"atom_name:val\" */\n+\treturn 1;\n+}\n+\n static struct {\n \tconst char *name;\n \tcmp_type cmp_type;\n@@ -260,22 +276,6 @@ static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_sta\n \tpop_stack_element(&state->stack);\n }\n \n-static int match_atom_name(const char *name, const char *atom_name, const char **val)\n-{\n-\tconst char *body;\n-\n-\tif (!skip_prefix(name, atom_name, &body))\n-\t\treturn 0; /* doesn't even begin with \"atom_name\" */\n-\tif (!body[0]) {\n-\t\t*val = NULL; /* %(atom_name) and no customization */\n-\t\treturn 1;\n-\t}\n-\tif (body[0] != ':')\n-\t\treturn 0; /* \"atom_namefoo\" is not \"atom_name\" or \"atom_name:...\" */\n-\t*val = body + 1; /* \"atom_name:val\" */\n-\treturn 1;\n-}\n-\n /*\n * In a format string, find the next occurrence of %(atom).\n */\n-- \n2.6.2\n"},{"id":"273211","messageId":"1447271075-15364-5-git-send-email-Karthik.188@gmail.com","threadId":"40776","inReplyTo":"1447271075-15364-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH/RFC 04/10] ref-filter: skip deref specifier in match_atom_name()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-11-11T19:44:30Z","receivedAt":"2015-11-11T19:44:30Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Signed-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 1542f5f..4af28ef 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.2\n"},{"id":"273209","messageId":"1447271075-15364-6-git-send-email-Karthik.188@gmail.com","threadId":"40776","inReplyTo":"1447271075-15364-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH/RFC 05/10] ref-filter: introduce color_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-11-11T19:44:31Z","receivedAt":"2015-11-11T19:44:31Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Introduce color_atom_parser() which will parse a \"color\" atom and\nstore its color in the \"use_atom\" structure for further usage in\n'populate_value()'.\n\nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 20 ++++++++++++++++----\n 1 file changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 4af28ef..0523d54 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -29,6 +29,9 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n static struct used_atom {\n \tconst char *str;\n \tcmp_type type;\n+\tunion {\n+\t\tconst char *color;\n+\t} u;\n } *used_atom;\n static int used_atom_cnt, need_tagged, need_symref;\n static int need_color_reset_at_eol;\n@@ -53,6 +56,13 @@ static int match_atom_name(const char *name, const char *atom_name, const char *\n \treturn 1;\n }\n \n+void color_atom_parser(struct used_atom *atom)\n+{\n+\tmatch_atom_name(atom->str, \"color\", &atom->u.color);\n+\tif (!atom->u.color)\n+\t\tdie(_(\"expected format: %%(color:)\"));\n+}\n+\n static struct {\n \tconst char *name;\n \tcmp_type cmp_type;\n@@ -90,7 +100,7 @@ static struct {\n \t{ \"symref\", FIELD_STR },\n \t{ \"flag\", FIELD_STR },\n \t{ \"HEAD\", FIELD_STR },\n-\t{ \"color\", FIELD_STR },\n+\t{ \"color\", FIELD_STR, color_atom_parser },\n \t{ \"align\", FIELD_STR },\n \t{ \"end\", FIELD_STR },\n };\n@@ -175,6 +185,9 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n \tREALLOC_ARRAY(used_atom, used_atom_cnt);\n \tused_atom[at].str = xmemdupz(atom, ep - atom);\n \tused_atom[at].type = valid_atom[i].cmp_type;\n+\tmemset(&used_atom[at].u, 0, sizeof(used_atom[at].u));\n+\tif (valid_atom[i].parser)\n+\t\tvalid_atom[i].parser(&used_atom[at]);\n \tif (*atom == '*')\n \t\tneed_tagged = 1;\n \tif (!strcmp(used_atom[at].str, \"symref\"))\n@@ -833,11 +846,10 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\trefname = branch_get_push(branch, NULL);\n \t\t\tif (!refname)\n \t\t\t\tcontinue;\n-\t\t} else if (match_atom_name(name, \"color\", &valp)) {\n+\t\t} else if (starts_with(name, \"color\")) {\n \t\t\tchar color[COLOR_MAXLEN] = \"\";\n+\t\t\tconst char *valp = atom->u.color;\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-- \n2.6.2\n"},{"id":"273216","messageId":"1447271075-15364-7-git-send-email-Karthik.188@gmail.com","threadId":"40776","inReplyTo":"1447271075-15364-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH/RFC 06/10] strbuf: introduce strbuf_split_str_without_term()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-11-11T19:44:32Z","receivedAt":"2015-11-11T19:44:32Z","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. Include an option\nwherein we can drop the terminator if required. In this context\nintroduce a wrapper function 'strbuf_split_str_without_term()' which\nsplits a given string into strbufs without including the terminator.\n\nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 5 +----\n strbuf.c | 17 +++++++++++++----\n strbuf.h | 14 ++++++++++----\n 3 files changed, 24 insertions(+), 12 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 0523d54..4e8b3c9 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -892,14 +892,11 @@ static void populate_value(struct ref_array_item *ref)\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_without_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\"))\ndiff --git a/strbuf.c b/strbuf.c\nindex b552a13..d31336f 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 with_term)\n {\n \tstruct strbuf **ret = NULL;\n \tsize_t nr = 0, alloc = 0;\n@@ -123,18 +123,27 @@ struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n \n \twhile (slen) {\n \t\tint len = slen;\n+\t\tint term = with_term;\n \t\tif (max <= 0 || nr + 1 < max) {\n \t\t\tconst char *end = memchr(str, terminator, slen);\n \t\t\tif (end)\n-\t\t\t\tlen = end - str + 1;\n+\t\t\t\tlen = end - str + term;\n+\t\t\telse\n+\t\t\t\t/* When no terminator present, we must add the last character */\n+\t\t\t\tterm = 1;\n \t\t}\n \t\tt = xmalloc(sizeof(struct strbuf));\n \t\tstrbuf_init(t, len);\n \t\tstrbuf_add(t, str, len);\n \t\tALLOC_GROW(ret, nr + 2, alloc);\n \t\tret[nr++] = t;\n-\t\tstr += len;\n-\t\tslen -= len;\n+\t\tif (!term) {\n+\t\t\tstr += len + 1;\n+\t\t\tslen -= len + 1;\n+\t\t} else {\n+\t\t\tstr += len;\n+\t\t\tslen -= len;\n+\t\t}\n \t}\n \tALLOC_GROW(ret, nr + 1, alloc); /* In case string was empty */\n \tret[nr] = NULL;\ndiff --git a/strbuf.h b/strbuf.h\nindex 6ae7a72..63e1e69 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -465,19 +465,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 with_term);\n+\n+static inline struct strbuf **strbuf_split_str_without_term(const char *str,\n+\t\t\t\t\t\t\t int terminator, int max)\n+{\n+\treturn strbuf_split_buf(str, strlen(str), terminator, max, 0);\n+}\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, 1);\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, 1);\n }\n \n static inline struct strbuf **strbuf_split(const struct strbuf *sb,\n-- \n2.6.2\n"},{"id":"273213","messageId":"1447271075-15364-8-git-send-email-Karthik.188@gmail.com","threadId":"40776","inReplyTo":"1447271075-15364-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH/RFC 07/10] ref-filter: introduce align_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-11-11T19:44:33Z","receivedAt":"2015-11-11T19:44:33Z","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 'align' atoms and store\nthe required width and position into the 'used_atom' structure. While\nwe're here, add support for the usage of 'width=' and 'position=' when\nusing the 'align' atom (e.g. %(align:position=middle,width=30)).\n\nAdd documentation and modify the existing tests in t6302 to reflect\nthe same.\n\nSigned-off-by: Karthik Nayak \n---\n Documentation/git-for-each-ref.txt | 18 ++++---\n ref-filter.c | 102 +++++++++++++++++++++++--------------\n t/t6302-for-each-ref-filter.sh | 4 +-\n 3 files changed, 75 insertions(+), 49 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex c6f073c..56ffdc1 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -129,14 +129,16 @@ 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. The prefix for the\n+\targuments is not mandatory. 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 4e8b3c9..049e6b9 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -16,6 +16,11 @@\n \n typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n \n+struct align {\n+\talign_type position;\n+\tunsigned int width;\n+};\n+\n /*\n * An atom is a valid field atom listed below, possibly prefixed with\n * a \"*\" to denote deref_tag().\n@@ -31,6 +36,7 @@ static struct used_atom {\n \tcmp_type type;\n \tunion {\n \t\tconst char *color;\n+\t\tstruct align align;\n \t} u;\n } *used_atom;\n static int used_atom_cnt, need_tagged, need_symref;\n@@ -63,6 +69,61 @@ void color_atom_parser(struct used_atom *atom)\n \t\tdie(_(\"expected format: %%(color:)\"));\n }\n \n+static align_type get_align_position(const char *type)\n+{\n+\tif (!strcmp(type, \"right\"))\n+\t\treturn ALIGN_RIGHT;\n+\telse if (!strcmp(type, \"middle\"))\n+\t\treturn ALIGN_MIDDLE;\n+\telse if (!strcmp(type, \"left\"))\n+\t\treturn ALIGN_LEFT;\n+\treturn -1;\n+}\n+\n+void align_atom_parser(struct used_atom *atom)\n+{\n+\tstruct align *align = &atom->u.align;\n+\tconst char *buf = NULL;\n+\tstruct strbuf **s, **to_free;\n+\tint width = -1;\n+\n+\tmatch_atom_name(atom->str, \"align\", &buf);\n+\tif (!buf)\n+\t\tdie(_(\"expected format: %%(align:,)\"));\n+\ts = to_free = strbuf_split_str_without_term(buf, ',', 0);\n+\n+\t/* By default align to ALGIN_LEFT */\n+\talign->position = ALIGN_LEFT;\n+\n+\twhile (*s) {\n+\t\tint position;\n+\t\tbuf = s[0]->buf;\n+\n+\t\tposition = get_align_position(buf);\n+\n+\t\tif (skip_prefix(buf, \"position=\", &buf)) {\n+\t\t\tposition = get_align_position(buf);\n+\t\t\tif (position == -1)\n+\t\t\t\tdie(_(\"improper format entered align:%s\"), s[0]->buf);\n+\t\t\talign->position = position;\n+\t\t} else if (skip_prefix(buf, \"width=\", &buf)) {\n+\t\t\tif (strtoul_ui(buf, 10, (unsigned int *)&width))\n+\t\t\t\tdie(_(\"improper format entered align:%s\"), s[0]->buf);\n+\t\t} else if (!strtoul_ui(buf, 10, (unsigned int *)&width))\n+\t\t\t\t;\n+\t\telse if (position != -1)\n+\t\t\talign->position = position;\n+\t\telse\n+\t\t\tdie(_(\"improper format entered align:%s\"), s[0]->buf);\n+\t\ts++;\n+\t}\n+\n+\tif (width < 0)\n+\t\tdie(_(\"positive width expected with the %%(align) atom\"));\n+\talign->width = width;\n+\tstrbuf_list_free(to_free);\n+}\n+\n static struct {\n \tconst char *name;\n \tcmp_type cmp_type;\n@@ -101,17 +162,12 @@ static struct {\n \t{ \"flag\", FIELD_STR },\n \t{ \"HEAD\", FIELD_STR },\n \t{ \"color\", FIELD_STR, color_atom_parser },\n-\t{ \"align\", FIELD_STR },\n+\t{ \"align\", FIELD_STR, align_atom_parser },\n \t{ \"end\", FIELD_STR },\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@@ -881,39 +937,7 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\t\tv->s = \" \";\n \t\t\tcontinue;\n \t\t} else if (match_atom_name(name, \"align\", &valp)) {\n-\t\t\tstruct align *align = &v->u.align;\n-\t\t\tstruct strbuf **s, **to_free;\n-\t\t\tint width = -1;\n-\n-\t\t\tif (!valp)\n-\t\t\t\tdie(_(\"expected format: %%(align:,)\"));\n-\n-\t\t\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_without_term(valp, ',', 0);\n-\n-\t\t\talign->position = ALIGN_LEFT;\n-\n-\t\t\twhile (*s) {\n-\t\t\t\tif (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))\n-\t\t\t\t\t;\n-\t\t\t\telse if (!strcmp(s[0]->buf, \"left\"))\n-\t\t\t\t\talign->position = ALIGN_LEFT;\n-\t\t\t\telse if (!strcmp(s[0]->buf, \"right\"))\n-\t\t\t\t\talign->position = ALIGN_RIGHT;\n-\t\t\t\telse if (!strcmp(s[0]->buf, \"middle\"))\n-\t\t\t\t\talign->position = ALIGN_MIDDLE;\n-\t\t\t\telse\n-\t\t\t\t\tdie(_(\"improper format entered align:%s\"), s[0]->buf);\n-\t\t\t\ts++;\n-\t\t\t}\n-\n-\t\t\tif (width < 0)\n-\t\t\t\tdie(_(\"positive width expected with the %%(align) atom\"));\n-\t\t\talign->width = width;\n-\t\t\tstrbuf_list_free(to_free);\n+\t\t\tv->u.align = atom->u.align;\n \t\t\tv->handler = align_atom_handler;\n \t\t\tcontinue;\n \t\t} else if (!strcmp(name, \"end\")) {\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex fe4796c..688751e 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -97,7 +97,7 @@ test_expect_success 'left alignment is default' '\n \trefname is refs/tags/three |refs/tags/three\n \trefname is refs/tags/two |refs/tags/two\n \tEOF\n-\tgit for-each-ref --format=\"%(align:30)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\tgit for-each-ref --format=\"%(align:width=30)refname is %(refname)%(end)|%(refname)\" >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -113,7 +113,7 @@ test_expect_success 'middle alignment' '\n \t| refname is refs/tags/three |refs/tags/three\n \t| refname is refs/tags/two |refs/tags/two\n \tEOF\n-\tgit for-each-ref --format=\"|%(align:middle,30)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\tgit for-each-ref --format=\"|%(align:position=middle,30)refname is %(refname)%(end)|%(refname)\" >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.6.2\n"},{"id":"273214","messageId":"1447271075-15364-9-git-send-email-Karthik.188@gmail.com","threadId":"40776","inReplyTo":"1447271075-15364-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH/RFC 08/10] ref-filter: introduce remote_ref_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-11-11T19:44:34Z","receivedAt":"2015-11-11T19:44:34Z","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\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 049e6b9..802629b 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -37,6 +37,11 @@ static struct used_atom {\n \tunion {\n \t\tconst char *color;\n \t\tstruct align align;\n+\t\tstruct {\n+\t\t\tunsigned int shorten : 1,\n+\t\t\t\ttrack : 1,\n+\t\t\t\ttrackshort : 1;\n+\t\t} remote_ref;\n \t} u;\n } *used_atom;\n static int used_atom_cnt, need_tagged, need_symref;\n@@ -69,6 +74,24 @@ void color_atom_parser(struct used_atom *atom)\n \t\tdie(_(\"expected format: %%(color:)\"));\n }\n \n+void remote_ref_atom_parser(struct used_atom *atom)\n+{\n+\tconst char *buf;\n+\n+\tbuf = strchr(atom->str, ':');\n+\tif (!buf)\n+\t\treturn;\n+\tbuf++;\n+\tif (!strcmp(buf, \"short\"))\n+\t\tatom->u.remote_ref.shorten = 1;\n+\telse if (!strcmp(buf, \"track\"))\n+\t\tatom->u.remote_ref.track = 1;\n+\telse if (!strcmp(buf, \"trackshort\"))\n+\t\tatom->u.remote_ref.trackshort = 1;\n+\telse\n+\t\tdie(_(\"improper format entered align:%s\"), buf);\n+}\n+\n static align_type get_align_position(const char *type)\n {\n \tif (!strcmp(type, \"right\"))\n@@ -156,8 +179,8 @@ static struct {\n \t{ \"subject\", FIELD_STR },\n \t{ \"body\", FIELD_STR },\n \t{ \"contents\", FIELD_STR },\n-\t{ \"upstream\", FIELD_STR },\n-\t{ \"push\", FIELD_STR },\n+\t{ \"upstream\", FIELD_STR, remote_ref_atom_parser },\n+\t{ \"push\", FIELD_STR, remote_ref_atom_parser },\n \t{ \"symref\", FIELD_STR },\n \t{ \"flag\", FIELD_STR },\n \t{ \"HEAD\", FIELD_STR },\n@@ -838,6 +861,42 @@ static inline char *copy_advance(char *dst, const char *src)\n \treturn dst;\n }\n \n+static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n+\t\t\t\t struct branch *branch, const char **s)\n+{\n+\tint num_ours, num_theirs;\n+\tif (atom->u.remote_ref.shorten)\n+\t\t*s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);\n+\telse if (atom->u.remote_ref.track) {\n+\t\tif (stat_tracking_info(branch, &num_ours,\n+\t\t\t\t &num_theirs, NULL))\n+\t\t\treturn;\n+\t\tif (!num_ours && !num_theirs)\n+\t\t\t*s = \"\";\n+\t\telse if (!num_ours)\n+\t\t\t*s = xstrfmt(\"[behind %d]\", num_theirs);\n+\t\telse if (!num_theirs)\n+\t\t\t*s = xstrfmt(\"[ahead %d]\", num_ours);\n+\t\telse\n+\t\t\t*s = xstrfmt(\"[ahead %d, behind %d]\",\n+\t\t\t\t num_ours, num_theirs);\n+\t} else if (atom->u.remote_ref.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\n+\t\t*s = refname;\n+}\n+\n /*\n * Parse the object referred by ref, and grab needed value.\n */\n@@ -892,6 +951,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@@ -902,6 +963,8 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\trefname = branch_get_push(branch, NULL);\n \t\t\tif (!refname)\n \t\t\t\tcontinue;\n+\t\t\tfill_remote_ref_details(atom, refname, branch, &v->s);\n+\t\t\tcontinue;\n \t\t} else if (starts_with(name, \"color\")) {\n \t\t\tchar color[COLOR_MAXLEN] = \"\";\n \t\t\tconst char *valp = atom->u.color;\n@@ -948,49 +1011,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.2\n"},{"id":"273215","messageId":"1447271075-15364-10-git-send-email-Karthik.188@gmail.com","threadId":"40776","inReplyTo":"1447271075-15364-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH/RFC 09/10] ref-filter: introduce contents_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-11-11T19:44:35Z","receivedAt":"2015-11-11T19:44:35Z","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\nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 66 ++++++++++++++++++++++++++++++++++++++----------------------\n 1 file changed, 42 insertions(+), 24 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 802629b..117bbbb 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -42,6 +42,14 @@ static struct used_atom {\n \t\t\t\ttrack : 1,\n \t\t\t\ttrackshort : 1;\n \t\t} remote_ref;\n+\t\tstruct {\n+\t\t\tunsigned int subject : 1,\n+\t\t\t\tbody : 1,\n+\t\t\t\tsignature : 1,\n+\t\t\t\tall : 1,\n+\t\t\t\tlines : 1,\n+\t\t\t\tno_lines;\n+\t\t} contents;\n \t} u;\n } *used_atom;\n static int used_atom_cnt, need_tagged, need_symref;\n@@ -92,6 +100,29 @@ void remote_ref_atom_parser(struct used_atom *atom)\n \t\tdie(_(\"improper format entered align:%s\"), buf);\n }\n \n+void contents_atom_parser(struct used_atom *atom)\n+{\n+\tconst char * buf;\n+\n+\tif (match_atom_name(atom->str, \"contents\", &buf))\n+\t\tatom->u.contents.all = 1;\n+\n+\tif (!buf)\n+\t\treturn;\n+\tif (!strcmp(buf, \"body\"))\n+\t\tatom->u.contents.body = 1;\n+\telse if (!strcmp(buf, \"signature\"))\n+\t\tatom->u.contents.signature = 1;\n+\telse if (!strcmp(buf, \"subject\"))\n+\t\tatom->u.contents.subject = 1;\n+\telse if (skip_prefix(buf, \"lines=\", &buf)) {\n+\t\tatom->u.contents.lines = 1;\n+\t\tif (strtoul_ui(buf, 10, &atom->u.contents.no_lines))\n+\t\t\tdie(_(\"positive value expected contents:lines=%s\"), buf);\n+\t} else\n+\t\tdie(_(\"improper format entered contents:%s\"), buf);\n+}\n+\n static align_type get_align_position(const char *type)\n {\n \tif (!strcmp(type, \"right\"))\n@@ -178,7 +209,7 @@ static struct {\n \t{ \"creatordate\", FIELD_TIME },\n \t{ \"subject\", FIELD_STR },\n \t{ \"body\", FIELD_STR },\n-\t{ \"contents\", FIELD_STR },\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\", FIELD_STR },\n@@ -191,11 +222,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@@ -212,7 +238,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@@ -761,20 +786,16 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj\n \tunsigned long sublen = 0, bodylen = 0, nonsiglen = 0, siglen = 0;\n \n \tfor (i = 0; i < used_atom_cnt; i++) {\n-\t\tconst char *name = used_atom[i].str;\n+\t\tstruct used_atom *atom = &used_atom[i];\n+\t\tconst char *name = atom->str;\n \t\tstruct atom_value *v = &val[i];\n-\t\tconst char *valp = NULL;\n \t\tif (!!deref != (*name == '*'))\n \t\t\tcontinue;\n \t\tif (deref)\n \t\t\tname++;\n \t\tif (strcmp(name, \"subject\") &&\n \t\t strcmp(name, \"body\") &&\n-\t\t strcmp(name, \"contents\") &&\n-\t\t strcmp(name, \"contents:subject\") &&\n-\t\t strcmp(name, \"contents:body\") &&\n-\t\t strcmp(name, \"contents:signature\") &&\n-\t\t !starts_with(name, \"contents:lines=\"))\n+\t\t !atom->u.contents.all)\n \t\t\tcontinue;\n \t\tif (!subpos)\n \t\t\tfind_subpos(buf, sz,\n@@ -784,26 +805,23 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj\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\telse if (atom->u.contents.subject)\n \t\t\tv->s = copy_subject(subpos, sublen);\n \t\telse if (!strcmp(name, \"body\"))\n \t\t\tv->s = xmemdupz(bodypos, bodylen);\n-\t\telse if (!strcmp(name, \"contents:body\"))\n+\t\telse if (atom->u.contents.body)\n \t\t\tv->s = xmemdupz(bodypos, nonsiglen);\n-\t\telse if (!strcmp(name, \"contents:signature\"))\n+\t\telse if (atom->u.contents.signature)\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.lines) {\n \t\t\tstruct strbuf s = STRBUF_INIT;\n \t\t\tconst char *contents_end = bodylen + bodypos - siglen;\n \n-\t\t\tif (strtoul_ui(valp, 10, &v->u.contents.lines))\n-\t\t\t\tdie(_(\"positive value expected contents:lines=%s\"), valp);\n \t\t\t/* Size is the length of the message after removing the signature */\n-\t\t\tappend_lines(&s, subpos, contents_end - subpos, v->u.contents.lines);\n+\t\t\tappend_lines(&s, subpos, contents_end - subpos, atom->u.contents.no_lines);\n \t\t\tv->s = strbuf_detach(&s, NULL);\n-\t\t}\n+\t\t} else /* For %(contents) without modifiers */\n+\t\t\tv->s = xstrdup(subpos);\n \t}\n }\n \n-- \n2.6.2\n"},{"id":"273625","messageId":"CAPig+cRYEmbsye78ESOLfmSi56sBFeKpaEkpGSEwF=qrLZWx8g@mail.gmail.com","threadId":"40776","inReplyTo":"1447271075-15364-2-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH/RFC 01/10] ref-filter: introduce a parsing function for each atom in valid_atom","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-11-23T23:44:35Z","receivedAt":"2015-11-23T23:44:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak wrote:\n> Introduce a parsing function for each atom in valid_atom. Using this\n> we can define special parsing functions for each of the atoms. Since\n> we have a third field in valid_atom structure, we now fill out missing\n> cmp_type values.\n\nI don't get it. Why do you need to \"fill out missing cmp_type values\"\nconsidering that you're never assigning the third field in this patch?\nAre you planning on filling in the third field in a future patch?\n\n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> @@ -19,42 +19,43 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n> static struct {\n> const char *name;\n> cmp_type cmp_type;\n> + void (*parser)(struct used_atom *atom);\n\nCompiler diagnostic:\n\n warning: declaration of 'struct used_atom' will not be\n visible outside of this function [-Wvisibility]\n\nIndeed, it seems rather odd to introduce the new field in this patch\nbut never actually do anything with it. It's difficult to understand\nthe intention.\n\n> } valid_atom[] = {\n> - { \"refname\" },\n> - { \"objecttype\" },\n> + { \"refname\", FIELD_STR },\n> + { \"objecttype\", FIELD_STR },\n> { \"objectsize\", FIELD_ULONG },\n> - { \"objectname\" },\n> - { \"tree\" },\n> - { \"parent\" },\n> + { \"objectname\", FIELD_STR },\n> + { \"tree\", FIELD_STR },\n> + { \"parent\", FIELD_STR },\n> { \"numparent\", FIELD_ULONG },\n> - { \"object\" },\n> - { \"type\" },\n> - { \"tag\" },\n> - { \"author\" },\n> - { \"authorname\" },\n> - { \"authoremail\" },\n> + { \"object\", FIELD_STR },\n> + { \"type\", FIELD_STR },\n> + { \"tag\", FIELD_STR },\n> + { \"author\", FIELD_STR },\n> + { \"authorname\", FIELD_STR },\n> + { \"authoremail\", FIELD_STR },\n> { \"authordate\", FIELD_TIME },\n> - { \"committer\" },\n> - { \"committername\" },\n> - { \"committeremail\" },\n> + { \"committer\", FIELD_STR },\n> + { \"committername\", FIELD_STR },\n> + { \"committeremail\", FIELD_STR },\n> { \"committerdate\", FIELD_TIME },\n> - { \"tagger\" },\n> - { \"taggername\" },\n> - { \"taggeremail\" },\n> + { \"tagger\", FIELD_STR },\n> + { \"taggername\", FIELD_STR },\n> + { \"taggeremail\", FIELD_STR },\n> { \"taggerdate\", FIELD_TIME },\n> - { \"creator\" },\n> + { \"creator\", FIELD_STR },\n> { \"creatordate\", FIELD_TIME },\n> - { \"subject\" },\n> - { \"body\" },\n> - { \"contents\" },\n> - { \"upstream\" },\n> - { \"push\" },\n> - { \"symref\" },\n> - { \"flag\" },\n> - { \"HEAD\" },\n> - { \"color\" },\n> - { \"align\" },\n> - { \"end\" },\n> + { \"subject\", FIELD_STR },\n> + { \"body\", FIELD_STR },\n> + { \"contents\", FIELD_STR },\n> + { \"upstream\", FIELD_STR },\n> + { \"push\", FIELD_STR },\n> + { \"symref\", FIELD_STR },\n> + { \"flag\", FIELD_STR },\n> + { \"HEAD\", FIELD_STR },\n> + { \"color\", FIELD_STR },\n> + { \"align\", FIELD_STR },\n> + { \"end\", FIELD_STR },\n> };\n>\n> #define REF_FORMATTING_STATE_INIT { 0, NULL }\n> --\n> 2.6.2\n"},{"id":"273657","messageId":"20151124214842.GA4848@sigill.intra.peff.net","threadId":"40776","inReplyTo":"1447271075-15364-1-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH/RFC 00/10] ref-filter: use parsing functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-24T21:48:42Z","receivedAt":"2015-11-24T21:48:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 12, 2015 at 01:14:26AM +0530, Karthik Nayak wrote:\n\n> Karthik Nayak (10):\n> ref-filter: introduce a parsing function for each atom in valid_atom\n> ref-filter: introduce struct used_atom\n> ref-fitler: bump match_atom() name to the top\n> ref-filter: skip deref specifier in match_atom_name()\n> ref-filter: introduce color_atom_parser()\n> strbuf: introduce strbuf_split_str_without_term()\n> ref-filter: introduce align_atom_parser()\n> ref-filter: introduce remote_ref_atom_parser()\n> ref-filter: introduce contents_atom_parser()\n> ref-filter: introduce objectname_atom_parser()\n\nHmm, your patch 10 does not seem to have made it to the list (at least I\ndid not ever get it, and gmane seems to be down, so I cannot check there).\n\n-Peff\n"},{"id":"273728","messageId":"CAOLa=ZRW8F9yB7Uw=3r9cp+Vi-6Lv9C-C475y8yi5-fih3dGcA@mail.gmail.com","threadId":"40776","inReplyTo":"20151124214842.GA4848@sigill.intra.peff.net","subject":"Re: [PATCH/RFC 00/10] ref-filter: use parsing functions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-11-25T12:07:23Z","receivedAt":"2015-11-25T12:07:23Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Nov 25, 2015 at 3:18 AM, Jeff King wrote:\n> On Thu, Nov 12, 2015 at 01:14:26AM +0530, Karthik Nayak wrote:\n>\n>> Karthik Nayak (10):\n>> ref-filter: introduce a parsing function for each atom in valid_atom\n>> ref-filter: introduce struct used_atom\n>> ref-fitler: bump match_atom() name to the top\n>> ref-filter: skip deref specifier in match_atom_name()\n>> ref-filter: introduce color_atom_parser()\n>> strbuf: introduce strbuf_split_str_without_term()\n>> ref-filter: introduce align_atom_parser()\n>> ref-filter: introduce remote_ref_atom_parser()\n>> ref-filter: introduce contents_atom_parser()\n>> ref-filter: introduce objectname_atom_parser()\n>\n> Hmm, your patch 10 does not seem to have made it to the list (at least I\n> did not ever get it, and gmane seems to be down, so I cannot check there).\n>\n> -Peff\n\nThat's weird, I'll reply to this mail with patch 10.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"273726","messageId":"CAOLa=ZTFeKMMSRCTWi9RkSvuCh7ZecPudSgsJB4TV76U4aGJJA@mail.gmail.com","threadId":"40776","inReplyTo":"CAPig+cRYEmbsye78ESOLfmSi56sBFeKpaEkpGSEwF=qrLZWx8g@mail.gmail.com","subject":"Re: [PATCH/RFC 01/10] ref-filter: introduce a parsing function for each atom in valid_atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-11-25T12:10:30Z","receivedAt":"2015-11-25T12:10:30Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Nov 24, 2015 at 5:14 AM, Eric Sunshine wrote:\n> On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak wrote:\n>> Introduce a parsing function for each atom in valid_atom. Using this\n>> we can define special parsing functions for each of the atoms. Since\n>> we have a third field in valid_atom structure, we now fill out missing\n>> cmp_type values.\n>\n> I don't get it. Why do you need to \"fill out missing cmp_type values\"\n> considering that you're never assigning the third field in this patch?\n> Are you planning on filling in the third field in a future patch?\n>\n\nI plan on filling that in upcoming patches. Probably, should mention that in\nthe commit message.\n\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> @@ -19,42 +19,43 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n>> static struct {\n>> const char *name;\n>> cmp_type cmp_type;\n>> + void (*parser)(struct used_atom *atom);\n>\n> Compiler diagnostic:\n>\n> warning: declaration of 'struct used_atom' will not be\n> visible outside of this function [-Wvisibility]\n>\n> Indeed, it seems rather odd to introduce the new field in this patch\n> but never actually do anything with it. It's difficult to understand\n> the intention.\n>\n\nThis is to make way for upcoming patches. But the compiler error is\naccurate used_atom only becomes a structure in the next patch.\nShould change that.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"273734","messageId":"1448459082-24492-1-git-send-email-Karthik.188@gmail.com","threadId":"40776","inReplyTo":"20151124214842.GA4848@sigill.intra.peff.net","subject":"[PATCH/RFC 10/10] ref-filter: introduce objectname_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-11-25T13:44:42Z","receivedAt":"2015-11-25T13:44:42Z","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\nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 42 +++++++++++++++++++++++++++++++-----------\n 1 file changed, 31 insertions(+), 11 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 117bbbb..f2add19 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -50,6 +50,10 @@ static struct used_atom {\n \t\t\t\tlines : 1,\n \t\t\t\tno_lines;\n \t\t} contents;\n+\t\tstruct {\n+\t\t\tunsigned int shorten : 1,\n+\t\t\t\tfull : 1;\n+\t\t} objectname;\n \t} u;\n } *used_atom;\n static int used_atom_cnt, need_tagged, need_symref;\n@@ -123,6 +127,21 @@ void contents_atom_parser(struct used_atom *atom)\n \t\tdie(_(\"improper format entered contents:%s\"), buf);\n }\n \n+void objectname_atom_parser(struct used_atom *atom)\n+{\n+\tconst char * buf;\n+\n+\tif (match_atom_name(atom->str, \"objectname\", &buf))\n+\t\tatom->u.objectname.full = 1;\n+\n+\tif (!buf)\n+\t\treturn;\n+\tif (!strcmp(buf, \"short\"))\n+\t\tatom->u.objectname.shorten = 1;\n+\telse\n+\t\tdie(_(\"improper format entered objectname:%s\"), buf);\n+}\n+\n static align_type get_align_position(const char *type)\n {\n \tif (!strcmp(type, \"right\"))\n@@ -186,7 +205,7 @@ static struct {\n \t{ \"refname\", FIELD_STR },\n \t{ \"objecttype\", FIELD_STR },\n \t{ \"objectsize\", FIELD_ULONG },\n-\t{ \"objectname\", FIELD_STR },\n+\t{ \"objectname\", FIELD_STR, objectname_atom_parser },\n \t{ \"tree\", FIELD_STR },\n \t{ \"parent\", FIELD_STR },\n \t{ \"numparent\", FIELD_ULONG },\n@@ -463,15 +482,16 @@ static void *get_obj(const unsigned char *sha1, struct object **obj, unsigned lo\n }\n \n static int grab_objectname(const char *name, const unsigned char *sha1,\n-\t\t\t struct atom_value *v)\n+\t\t\t struct atom_value *v, struct used_atom *atom)\n {\n-\tif (!strcmp(name, \"objectname\")) {\n-\t\tv->s = xstrdup(sha1_to_hex(sha1));\n-\t\treturn 1;\n-\t}\n-\tif (!strcmp(name, \"objectname:short\")) {\n-\t\tv->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));\n-\t\treturn 1;\n+\tif (starts_with(name, \"objectname\")) {\n+\t\tif (atom->u.objectname.shorten) {\n+\t\t\tv->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));\n+\t\t\treturn 1;\n+\t\t} else if (atom->u.objectname.full) {\n+\t\t\tv->s = xstrdup(sha1_to_hex(sha1));\n+\t\t\treturn 1;\n+\t\t}\n \t}\n \treturn 0;\n }\n@@ -495,7 +515,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@@ -1004,7 +1024,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.2\n"},{"id":"273755","messageId":"CAPig+cRcxr7uT2OJX=TnaVf+gXQdw9ydp+7X+Kis4Vb5+6RHiA@mail.gmail.com","threadId":"40776","inReplyTo":"CAOLa=ZTFeKMMSRCTWi9RkSvuCh7ZecPudSgsJB4TV76U4aGJJA@mail.gmail.com","subject":"Re: [PATCH/RFC 01/10] ref-filter: introduce a parsing function for each atom in valid_atom","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-11-25T19:41:15Z","receivedAt":"2015-11-25T19:41:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Nov 25, 2015 at 7:10 AM, Karthik Nayak wrote:\n> On Tue, Nov 24, 2015 at 5:14 AM, Eric Sunshine wrote:\n>> On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak wrote:\n>>> Introduce a parsing function for each atom in valid_atom. Using this\n>>> we can define special parsing functions for each of the atoms. Since\n>>> we have a third field in valid_atom structure, we now fill out missing\n>>> cmp_type values.\n>>\n>> I don't get it. Why do you need to \"fill out missing cmp_type values\"\n>> considering that you're never assigning the third field in this patch?\n>> Are you planning on filling in the third field in a future patch?\n>\n> I plan on filling that in upcoming patches. Probably, should mention that in\n> the commit message.\n\nMaking it clear that this patch is preparatory for introduction of\n'valid_atom' is a good idea, however, adding the unused 'valid_atom'\nfield in this patch is not recommended. It would be better to\nintroduce 'valid_atom' in the patch which actually needs it.\n\n>>> Signed-off-by: Karthik Nayak \n>>> ---\n>>> diff --git a/ref-filter.c b/ref-filter.c\n>>> @@ -19,42 +19,43 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n>>> static struct {\n>>> const char *name;\n>>> cmp_type cmp_type;\n>>> + void (*parser)(struct used_atom *atom);\n>>\n>> Compiler diagnostic:\n>>\n>> warning: declaration of 'struct used_atom' will not be\n>> visible outside of this function [-Wvisibility]\n>>\n>> Indeed, it seems rather odd to introduce the new field in this patch\n>> but never actually do anything with it. It's difficult to understand\n>> the intention.\n>\n> This is to make way for upcoming patches. But the compiler error is\n> accurate used_atom only becomes a structure in the next patch.\n> Should change that.\n\nThis problem will go away if you introduce the 'valid_atom' field in\nthe patch which actually needs it (as suggested above) rather than in\nthis patch.\n"},{"id":"273786","messageId":"CAOLa=ZTHaety8=cxP4PqcXqrRv9OAEAcRyTs9drY6nSFGKvyOQ@mail.gmail.com","threadId":"40776","inReplyTo":"CAPig+cRcxr7uT2OJX=TnaVf+gXQdw9ydp+7X+Kis4Vb5+6RHiA@mail.gmail.com","subject":"Re: [PATCH/RFC 01/10] ref-filter: introduce a parsing function for each atom in valid_atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-11-26T18:01:24Z","receivedAt":"2015-11-26T18:01:24Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Thu, Nov 26, 2015 at 1:11 AM, Eric Sunshine wrote:\n> On Wed, Nov 25, 2015 at 7:10 AM, Karthik Nayak wrote:\n>> On Tue, Nov 24, 2015 at 5:14 AM, Eric Sunshine wrote:\n>>> On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak wrote:\n>>>> Introduce a parsing function for each atom in valid_atom. Using this\n>>>> we can define special parsing functions for each of the atoms. Since\n>>>> we have a third field in valid_atom structure, we now fill out missing\n>>>> cmp_type values.\n>>>\n>>> I don't get it. Why do you need to \"fill out missing cmp_type values\"\n>>> considering that you're never assigning the third field in this patch?\n>>> Are you planning on filling in the third field in a future patch?\n>>\n>> I plan on filling that in upcoming patches. Probably, should mention that in\n>> the commit message.\n>\n> Making it clear that this patch is preparatory for introduction of\n> 'valid_atom' is a good idea, however, adding the unused 'valid_atom'\n> field in this patch is not recommended. It would be better to\n> introduce 'valid_atom' in the patch which actually needs it.\n>\n\nI get your point, will do as you suggested.\n\n>>>> Signed-off-by: Karthik Nayak \n>>>> ---\n>>>> diff --git a/ref-filter.c b/ref-filter.c\n>>>> @@ -19,42 +19,43 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n>>>> static struct {\n>>>> const char *name;\n>>>> cmp_type cmp_type;\n>>>> + void (*parser)(struct used_atom *atom);\n>>>\n>>> Compiler diagnostic:\n>>>\n>>> warning: declaration of 'struct used_atom' will not be\n>>> visible outside of this function [-Wvisibility]\n>>>\n>>> Indeed, it seems rather odd to introduce the new field in this patch\n>>> but never actually do anything with it. It's difficult to understand\n>>> the intention.\n>>\n>> This is to make way for upcoming patches. But the compiler error is\n>> accurate used_atom only becomes a structure in the next patch.\n>> Should change that.\n>\n> This problem will go away if you introduce the 'valid_atom' field in\n> the patch which actually needs it (as suggested above) rather than in\n> this patch.\n\nYup, agreed.\nThanks for your suggestions.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"273881","messageId":"CAPig+cRthCCK51XO2mju2VBe6Yvhy_Rp9nVvVUiWd9LuT9BxLQ@mail.gmail.com","threadId":"40776","inReplyTo":"1447271075-15364-3-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH/RFC 02/10] ref-filter: introduce struct used_atom","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-01T23:00:57Z","receivedAt":"2015-12-01T23:00:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak wrote:\n> Introduce the 'used_array' structure which would replace the existing\n\nI guess you meant s/used_array/used_atom/ or something?\n\nAlso, s/which would/to/\n\n> implementation of 'used_array' (which a list of atoms). This helps us\n\ns/which a/which is a/\n\n> parse atom's before hand and store required details into the\n\ns/atom's/atoms/\ns/before hand/beforehand/\n\n> 'used_array' for future usage.\n>\n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> @@ -16,6 +16,23 @@\n> +/*\n> + * An atom is a valid field atom listed below, possibly prefixed with\n> + * a \"*\" to denote deref_tag().\n> + *\n> + * We parse given format string and sort specifiers, and make a list\n> + * of properties that we need to extract out of objects. ref_array_item\n> + * structure will hold an array of values extracted that can be\n> + * indexed with the \"atom number\", which is an index into this\n> + * array.\n> + */\n> +static struct used_atom {\n> + const char *str;\n\nThis is really the atom's name, isn't it? If so, perhaps \"name\" would\nbe a better field name.\n\n> + cmp_type type;\n> +} *used_atom;\n> +static int used_atom_cnt, need_tagged, need_symref;\n> +static int need_color_reset_at_eol;\n> +\n> static struct {\n> const char *name;\n> cmp_type cmp_type;\n> @@ -93,21 +110,6 @@ struct atom_value {\n> };\n>\n> /*\n> - * An atom is a valid field atom listed above, possibly prefixed with\n> - * a \"*\" to denote deref_tag().\n> - *\n> - * We parse given format string and sort specifiers, and make a list\n> - * of properties that we need to extract out of objects. ref_array_item\n> - * structure will hold an array of values extracted that can be\n> - * indexed with the \"atom number\", which is an index into this\n> - * array.\n> - */\n> -static const char **used_atom;\n> -static cmp_type *used_atom_type;\n> -static int used_atom_cnt, need_tagged, need_symref;\n> -static int need_color_reset_at_eol;\n\nYou're moving this block of declarations up above the valid_atom[]\narray because the previous patch added a new field named \"parser\" to\nvalid_atom[] which references 'struct used_atom' added by this patch\n(2). I wonder if this movement should be done as a separate\npreparatory patch to make it easier to review since, as it stands, the\nreviewer has to read much more carefully to detect changes in the\nmoved block.\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> @@ -787,7 +788,8 @@ static void populate_value(struct ref_array_item *ref)\n>\n> /* Fill in specials first */\n> for (i = 0; i < used_atom_cnt; i++) {\n> - const char *name = used_atom[i];\n> + struct used_atom *atom = &used_atom[i];\n> + const char *name = atom->str;\n\nWhy not just:\n\n const char *name = used_atom[i].str;\n\n?\n\n> struct atom_value *v = &ref->value[i];\n> int deref = 0;\n> const char *refname;\n"},{"id":"273883","messageId":"CAPig+cTNshvpwn1=VSbnDE8AQMkfKmd1wnP0hNLT1iS3cDEb_A@mail.gmail.com","threadId":"40776","inReplyTo":"1447271075-15364-5-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH/RFC 04/10] ref-filter: skip deref specifier in match_atom_name()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-01T23:11:54Z","receivedAt":"2015-12-01T23:11:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak wrote:\n> Signed-off-by: Karthik Nayak \n\nA bit of explanation about why this change is desirable would be\nwelcome. I'm guessing it's because a future patch is going to make\ncalls to match_atom_name() with the '*' deref indicator still attached\nto the name, whereas existing code does not do so.\n\n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> @@ -37,6 +37,10 @@ static int match_atom_name(const char *name, const char *atom_name, const char *\n> {\n> const char *body;\n>\n> + /* skip the deref specifier*/\n> + if (name[0] == '*')\n> + name++;\n> +\n> if (!skip_prefix(name, atom_name, &body))\n> return 0; /* doesn't even begin with \"atom_name\" */\n> if (!body[0]) {\n> --\n> 2.6.2\n"},{"id":"273885","messageId":"CAPig+cRXgLqmUf1ZK2QnHbHM6-1Ya=iTx6Jq9YdnkO8aOsq+tQ@mail.gmail.com","threadId":"40776","inReplyTo":"1447271075-15364-6-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH/RFC 05/10] ref-filter: introduce color_atom_parser()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-01T23:27:28Z","receivedAt":"2015-12-01T23:27:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak wrote:\n> Introduce color_atom_parser() which will parse a \"color\" atom and\n> store its color in the \"use_atom\" structure for further usage in\n\ns/use_atom/used_atom/\n\n> 'populate_value()'.\n>\n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> @@ -29,6 +29,9 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n> static struct used_atom {\n> const char *str;\n> cmp_type type;\n> + union {\n> + const char *color;\n> + } u;\n> } *used_atom;\n> @@ -53,6 +56,13 @@ static int match_atom_name(const char *name, const char *atom_name, const char *\n> +void color_atom_parser(struct used_atom *atom)\n> +{\n> + match_atom_name(atom->str, \"color\", &atom->u.color);\n> + if (!atom->u.color)\n> + die(_(\"expected format: %%(color:)\"));\n> +}\n> +\n> @@ -175,6 +185,9 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n> REALLOC_ARRAY(used_atom, used_atom_cnt);\n> used_atom[at].str = xmemdupz(atom, ep - atom);\n> used_atom[at].type = valid_atom[i].cmp_type;\n> + memset(&used_atom[at].u, 0, sizeof(used_atom[at].u));\n> + if (valid_atom[i].parser)\n> + valid_atom[i].parser(&used_atom[at]);\n> if (*atom == '*')\n> need_tagged = 1;\n> if (!strcmp(used_atom[at].str, \"symref\"))\n> @@ -833,11 +846,10 @@ static void populate_value(struct ref_array_item *ref)\n> refname = branch_get_push(branch, NULL);\n> if (!refname)\n> continue;\n> - } else if (match_atom_name(name, \"color\", &valp)) {\n> + } else if (starts_with(name, \"color\")) {\n\nHmm, so this will also match \"colorize\". Is that desirable?\n\n> char color[COLOR_MAXLEN] = \"\";\n> + const char *valp = atom->u.color;\n>\n> - if (!valp)\n> - die(_(\"expected format: %%(color:)\"));\n> if (color_parse(valp, color) < 0)\n\nRather than declaring variable 'valp', why not just say:\n\n if (color_parse(atom->u.color, color) < 0)\n\n?\n\n> die(_(\"unable to parse format\"));\n> v->s = xstrdup(color);\n> --\n> 2.6.2\n"},{"id":"273903","messageId":"CAPig+cSxWybzunt_KWT+prZ=wXMi1wqM44jQZbp1pcz+spJW6A@mail.gmail.com","threadId":"40776","inReplyTo":"1447271075-15364-7-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH/RFC 06/10] strbuf: introduce strbuf_split_str_without_term()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-02T08:04:31Z","receivedAt":"2015-12-02T08:04:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak wrote:\n> The current implementation of 'strbuf_split_buf()' includes the\n> terminator at the end of each strbuf post splitting. Include an option\n\ns/Include an/Add an/\n\n> wherein we can drop the terminator if required. In this context\n\ns/required/desired/\n\n> introduce a wrapper function 'strbuf_split_str_without_term()' which\n> splits a given string into strbufs without including the terminator.\n>\n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> @@ -892,14 +892,11 @@ static void populate_value(struct ref_array_item *ref)\n> * TODO: Implement a function similar to strbuf_split_str()\n> * which would omit the separator from the end of each value.\n> */\n> - s = to_free = strbuf_split_str(valp, ',', 0);\n> + s = to_free = strbuf_split_str_without_term(valp, ',', 0);\n>\n> align->position = ALIGN_LEFT;\n>\n> while (*s) {\n> - /* Strip trailing comma */\n> - if (s[1])\n> - strbuf_setlen(s[0], s[0]->len - 1);\n\nI'd prefer to see this ref-filter.c change split out as a separate\npatch so as not to pollute the otherwise single-purpose change\nintroduced by this patch (i.e. capability to omit the terminator).\n\nAlso, it might make sense to move this patch to the head of the\nseries, since it's conceptually distinct from the rest of the patches,\nand could conceivably prove useful on its own, regardless of how the\nrest of the series fares.\n\n> if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))\n> ;\n> else if (!strcmp(s[0]->buf, \"left\"))\n> diff --git a/strbuf.c 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 with_term)\n\n\"with_term\" might undesirably be interpreted as meaning \"use this\nparticular term\". Perhaps a different name, such as \"include_term\",\n\"drop_term\", or \"omit_term\" would be a bit less ambiguous. (I think I\nprefer \"omit_term\".)\n\n> {\n> struct strbuf **ret = NULL;\n> size_t nr = 0, alloc = 0;\n> @@ -123,18 +123,27 @@ struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n>\n> while (slen) {\n> int len = slen;\n> + int term = with_term;\n\n\"term\" is not a great variable name, and is easily confused with the\nexisting \"terminator\" input argument. This is really being used as a\nlength adjustment, so perhaps a name such as 'term_adjust' or\n'len_adjust' or something better would be preferable.\n\nAlso, since the value of 'with_term' never changes, then 'term' will\nhave the same value each time through the loop, thus you could\n(should) hoist the declaration and initialization of 'term' outside of\nthe loop.\n\nDue to the way you're using this variable ('term'), you want its value\nalways to be 0 or 1 but you don't do anything to ensure that. What if\nthe user passes in 42 rather than 0 or 1? That would mess up your\n(below) calculations. Worse, what if the user passes in -42? That\nwould be particularly alarming. To turn this into a boolean value (0\nor 1), do this instead:\n\n int term = !!with_term;\n\n> if (max <= 0 || nr + 1 < max) {\n> const char *end = memchr(str, terminator, slen);\n> if (end)\n> - len = end - str + 1;\n> + len = end - str + term;\n> + else\n> + /* When no terminator present, we must add the last character */\n> + term = 1;\n> }\n> t = xmalloc(sizeof(struct strbuf));\n> strbuf_init(t, len);\n> strbuf_add(t, str, len);\n> ALLOC_GROW(ret, nr + 2, alloc);\n> ret[nr++] = t;\n> - str += len;\n> - slen -= len;\n> + if (!term) {\n> + str += len + 1;\n> + slen -= len + 1;\n> + } else {\n> + str += len;\n> + slen -= len;\n> + }\n\nThis new logic is complex and confusing, thus difficult to review for\ncorrectness. Rather than messing with 'len' and the existing logic,\nhow about instead, just adjusting the amount you store in the strbuf?\nThat is, instead of all the above changes, you might be able to get by\nwith one little change, something like this (untested):\n\n - strbuf_add(t, str, len);\n + strbuf_add(t, str, len - !!end * !!with_term);\n\n> }\n> ALLOC_GROW(ret, nr + 1, alloc); /* In case string was empty */\n> ret[nr] = NULL;\n> diff --git a/strbuf.h b/strbuf.h\n> @@ -465,19 +465,25 @@ static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)\n> * For lighter-weight alternatives, see string_list_split() and\n> * string_list_split_in_place().\n> */\n> -extern struct strbuf **strbuf_split_buf(const char *, size_t,\n> - int terminator, int max);\n> +extern struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n> + int terminator, int max, int with_term);\n\nYou also need to update the comment block above this declaration since\nit still says that each substring includes the terminator. It also\nfails to mention the new 'with_term' argument added by this patch.\n\n> +static inline struct strbuf **strbuf_split_str_without_term(const char *str,\n> + int terminator, int max)\n\nThis is an uncomfortably long function name. Unfortunately, short and\nsweet strbuf_split() is already taken. Perhaps\nstrbuf_split_str_drop_term()? strbuf_split_str_omit_term()?\nstrbuf_split_str_no_term()? strbuf_split_noterm()?\n\n> +{\n> + return strbuf_split_buf(str, strlen(str), terminator, max, 0);\n> +}\n>\n> static inline struct strbuf **strbuf_split_str(const char *str,\n> int terminator, int max)\n> {\n> - return strbuf_split_buf(str, strlen(str), terminator, max);\n> + return strbuf_split_buf(str, strlen(str), terminator, max, 1);\n> }\n>\n> static inline struct strbuf **strbuf_split_max(const struct strbuf *sb,\n> int terminator, int max)\n> {\n> - return strbuf_split_buf(sb->buf, sb->len, terminator, max);\n> + return strbuf_split_buf(sb->buf, sb->len, terminator, max, 1);\n> }\n>\n> static inline struct strbuf **strbuf_split(const struct strbuf *sb,\n> --\n> 2.6.2\n"},{"id":"273937","messageId":"CAPig+cTYt1qaSLLOmwyZ-BQzOjWGxZ5koJNma1qc67UipPFW5w@mail.gmail.com","threadId":"40776","inReplyTo":"1447271075-15364-8-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH/RFC 07/10] ref-filter: introduce align_atom_parser()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-02T21:23:59Z","receivedAt":"2015-12-02T21:23:59Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak wrote:\n> Introduce align_atom_parser() which will parse 'align' atoms and store\n> the required width and position into the 'used_atom' structure. While\n> we're here, add support for the usage of 'width=' and 'position=' when\n> using the 'align' atom (e.g. %(align:position=middle,width=30)).\n\nThis patch is doing too much by both moving code around and modifying\nthat code (somewhat dramatically), thus it is difficult for reviewers\nto compare the old and new behaviors. It deserves to be split apart\ninto at least two patches. First, the code movement patch which\nintroduces align_atom_parser() (and possibly get_align_position())\nwithout any behavior or logical change; then the patch which changes\nbehavior to recognize the spelled-out forms \"width=\" and \"position=\".\nYou may even want to spilt it into more patches, for instance by doing\nthe get_align_position() extraction in its own patch.\n\n> Add documentation and modify the existing tests in t6302 to reflect\n> the same.\n>\n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\n> @@ -129,14 +129,16 @@ color::\n> align::\n> Left-, middle-, or right-align the content between\n> - %(align:...) and %(end). The \"align:\" is followed by ``\n> - and `` in any order separated by a comma, where the\n> - `` is either left, right or middle, default being\n> - left and `` is the total length of the content with\n> - alignment. If the contents length is more than the width then\n> - no alignment is performed. If used with '--quote' everything\n> - in between %(align:...) and %(end) is quoted, but if nested\n> - then only the topmost level performs quoting.\n> + %(align:...) and %(end). The \"align:\" is followed by\n> + `width=` and `position=` in any order\n> + separated by a comma, where the `` is either left,\n> + right or middle, default being left and `` is the total\n> + length of the content with alignment. The prefix for the\n> + arguments is not mandatory. If the contents length is more\n\nThis paragraph is so bulky that it's very easy to overlook the bit\nabout the \"prefix for the arguments\" being optional, and it's not\nnecessarily even clear to the casual reader what that means. It might,\ntherefore, be a good idea to spell it out explicitly. For instance,\nyou might say something like:\n\n For brevity, the \"width=\" and/or \"position=\" prefixes may be\n omitted, and bare and used instead.\n For instance, `%(align:,)`.\n\n> + than the width then no alignment is performed. If used with\n> + '--quote' everything in between %(align:...) and %(end) is\n> + quoted, but if nested then only the topmost level performs\n> + quoting.\n> diff --git a/ref-filter.c b/ref-filter.c\n> @@ -63,6 +69,61 @@ void color_atom_parser(struct used_atom *atom)\n> +static align_type get_align_position(const char *type)\n\nTaken in context of the callers, this isn't a great function name, as\nit implies that it is retrieving some value, when in fact it is\nparsing the input argument. A better name might be\nparse_align_position().\n\nLikewise, 'type' isn't necessarily a great argument name. You might\ninstead call it 'pos' or even just short and sweet 's'.\n\n> +{\n> + if (!strcmp(type, \"right\"))\n> + return ALIGN_RIGHT;\n> + else if (!strcmp(type, \"middle\"))\n> + return ALIGN_MIDDLE;\n> + else if (!strcmp(type, \"left\"))\n> + return ALIGN_LEFT;\n> + return -1;\n> +}\n> +\n> +void align_atom_parser(struct used_atom *atom)\n> +{\n> + struct align *align = &atom->u.align;\n> + const char *buf = NULL;\n> + struct strbuf **s, **to_free;\n> + int width = -1;\n> +\n> + match_atom_name(atom->str, \"align\", &buf);\n> + if (!buf)\n> + die(_(\"expected format: %%(align:,)\"));\n\nIs this still the way you want this error message to appear, or should\nit show the long-form of the arguments? (I don't care strongly.)\n\n> + s = to_free = strbuf_split_str_without_term(buf, ',', 0);\n> +\n> + /* By default align to ALGIN_LEFT */\n\nWhat is ALGIN? Regardless of the answer, this comment is not\nparticularly useful since it merely repeats what the code itself\nalready states clearly.\n\n> + align->position = ALIGN_LEFT;\n> +\n> + while (*s) {\n> + int position;\n> + buf = s[0]->buf;\n> +\n> + position = get_align_position(buf);\n\nWhy is this assignment way up here rather than down below in the\npenultimate 'else' arm where its result is actually being checked? By\nmoving it closer to the point of use, the logic becomes easier to\nunderstand.\n\n> + if (skip_prefix(buf, \"position=\", &buf)) {\n> + position = get_align_position(buf);\n> + if (position == -1)\n> + die(_(\"improper format entered align:%s\"), s[0]->buf);\n\nAt this point, you can give a better error message since you know that\nyou were parsing a \"position=\" argument. Maybe something like\n\"unrecognized position: %s\".\n\n> + align->position = position;\n> + } else if (skip_prefix(buf, \"width=\", &buf)) {\n> + if (strtoul_ui(buf, 10, (unsigned int *)&width))\n> + die(_(\"improper format entered align:%s\"), s[0]->buf);\n\nDitto regarding better error message.\n\n> + } else if (!strtoul_ui(buf, 10, (unsigned int *)&width))\n> + ;\n> + else if (position != -1)\n> + align->position = position;\n> + else\n> + die(_(\"improper format entered align:%s\"), s[0]->buf);\n\nHere too, it would be more user-friendly to say \"unrecognized\n%%(align) argument: %s\".\n\n> + s++;\n> + }\n> +\n> + if (width < 0)\n> + die(_(\"positive width expected with the %%(align) atom\"));\n> + align->width = width;\n> + strbuf_list_free(to_free);\n> +}\n> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\n> @@ -97,7 +97,7 @@ test_expect_success 'left alignment is default' '\n> refname is refs/tags/three |refs/tags/three\n> refname is refs/tags/two |refs/tags/two\n> EOF\n> - git for-each-ref --format=\"%(align:30)refname is %(refname)%(end)|%(refname)\" >actual &&\n> + git for-each-ref --format=\"%(align:width=30)refname is %(refname)%(end)|%(refname)\" >actual &&\n> test_cmp expect actual\n> '\n>\n> @@ -113,7 +113,7 @@ test_expect_success 'middle alignment' '\n> | refname is refs/tags/three |refs/tags/three\n> | refname is refs/tags/two |refs/tags/two\n> EOF\n> - git for-each-ref --format=\"|%(align:middle,30)refname is %(refname)%(end)|%(refname)\" >actual &&\n> + git for-each-ref --format=\"|%(align:position=middle,30)refname is %(refname)%(end)|%(refname)\" >actual &&\n> test_cmp expect actual\n> '\n\nWhile it may sometimes be reasonable to re-purpose existing tests like\nthis, this probably is not one of those cases. Instead, you should be\nadding new tests to check all the permutations of the new argument\nhandling. For instance:\n\n %(align:42)\n %(align:middle,42)\n %(align:42,middle)\n %(align:position=middle,42)\n %(align:42,position=middle)\n %(align:middle,width=42)\n %(align:width=42,middle)\n %(align:position=middle,width=42)\n %(align:width=42,position=middle)\n\nAnd, it wouldn't hurt to test handling of redundant or extra position\nand width arguments. Should multiple arguments of the same type result\nin an error, or should \"last one wins (sliently)\" be the policy? Once\nyou decide upon a policy, add tests to check that that policy works as\nexpected.\n\nIn this case, \"last one wins (silently)\" may be more friendly to\nscript writers, so it might be the better choice. You'd want to add\nappropriate tests, using the various permutations. For instance:\n\n %(align:42,width=43)\n %(align:width=43,42)\n %(align:42,position=middle,right)\n %(align:42,right,position=middle)\n"},{"id":"273967","messageId":"CAOLa=ZR2zQivyEDVwPxHxWvucYeGE9Z+bbe8xUYYfTgMh8ajgA@mail.gmail.com","threadId":"40776","inReplyTo":"CAPig+cTNshvpwn1=VSbnDE8AQMkfKmd1wnP0hNLT1iS3cDEb_A@mail.gmail.com","subject":"Re: [PATCH/RFC 04/10] ref-filter: skip deref specifier in match_atom_name()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-03T06:34:23Z","receivedAt":"2015-12-03T06:34:23Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Dec 2, 2015 at 4:41 AM, Eric Sunshine wrote:\n> On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak wrote:\n>> Signed-off-by: Karthik Nayak \n>\n> A bit of explanation about why this change is desirable would be\n> welcome. I'm guessing it's because a future patch is going to make\n> calls to match_atom_name() with the '*' deref indicator still attached\n> to the name, whereas existing code does not do so.\n>\n\nYes, you're correct!\nWill add this in, thanks.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"273975","messageId":"CAOLa=ZQY3PMtfemVAsbHydtguQG9pc2uVTiJ6Qh+jLf6mY4dFw@mail.gmail.com","threadId":"40776","inReplyTo":"CAPig+cRXgLqmUf1ZK2QnHbHM6-1Ya=iTx6Jq9YdnkO8aOsq+tQ@mail.gmail.com","subject":"Re: [PATCH/RFC 05/10] ref-filter: introduce color_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-03T13:35:17Z","receivedAt":"2015-12-03T13:35:17Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Dec 2, 2015 at 4:57 AM, Eric Sunshine wrote:\n> On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak wrote:\n>> Introduce color_atom_parser() which will parse a \"color\" atom and\n>> store its color in the \"use_atom\" structure for further usage in\n>\n> s/use_atom/used_atom/\n>\n\nwill change.\n\n>> 'populate_value()'.\n>>\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> @@ -29,6 +29,9 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n>> static struct used_atom {\n>> const char *str;\n>> cmp_type type;\n>> + union {\n>> + const char *color;\n>> + } u;\n>> } *used_atom;\n>> @@ -53,6 +56,13 @@ static int match_atom_name(const char *name, const char *atom_name, const char *\n>> +void color_atom_parser(struct used_atom *atom)\n>> +{\n>> + match_atom_name(atom->str, \"color\", &atom->u.color);\n>> + if (!atom->u.color)\n>> + die(_(\"expected format: %%(color:)\"));\n>> +}\n>> +\n>> @@ -175,6 +185,9 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n>> REALLOC_ARRAY(used_atom, used_atom_cnt);\n>> used_atom[at].str = xmemdupz(atom, ep - atom);\n>> used_atom[at].type = valid_atom[i].cmp_type;\n>> + memset(&used_atom[at].u, 0, sizeof(used_atom[at].u));\n>> + if (valid_atom[i].parser)\n>> + valid_atom[i].parser(&used_atom[at]);\n>> if (*atom == '*')\n>> need_tagged = 1;\n>> if (!strcmp(used_atom[at].str, \"symref\"))\n>> @@ -833,11 +846,10 @@ static void populate_value(struct ref_array_item *ref)\n>> refname = branch_get_push(branch, NULL);\n>> if (!refname)\n>> continue;\n>> - } else if (match_atom_name(name, \"color\", &valp)) {\n>> + } else if (starts_with(name, \"color\")) {\n>\n> Hmm, so this will also match \"colorize\". Is that desirable?\n>\n\nWell the error checking is done when we parse the atom in color_atom_parser()\nso here we don't need to worry about something like this.\n\n>> char color[COLOR_MAXLEN] = \"\";\n>> + const char *valp = atom->u.color;\n>>\n>> - if (!valp)\n>> - die(_(\"expected format: %%(color:)\"));\n>> if (color_parse(valp, color) < 0)\n>\n> Rather than declaring variable 'valp', why not just say:\n>\n> if (color_parse(atom->u.color, color) < 0)\n>\n> ?\n\nYes, will change, thanks.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"273980","messageId":"CAOLa=ZQz-NE3uKtU72NUyR23QaJE4jf=UqiMptnM+hOmAE9GRw@mail.gmail.com","threadId":"40776","inReplyTo":"CAPig+cSxWybzunt_KWT+prZ=wXMi1wqM44jQZbp1pcz+spJW6A@mail.gmail.com","subject":"Re: [PATCH/RFC 06/10] strbuf: introduce strbuf_split_str_without_term()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-03T18:12:40Z","receivedAt":"2015-12-03T18:12:40Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Dec 2, 2015 at 1:34 PM, Eric Sunshine wrote:\n> On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak wrote:\n>> The current implementation of 'strbuf_split_buf()' includes the\n>> terminator at the end of each strbuf post splitting. Include an option\n>\n> s/Include an/Add an/\n>\n>> wherein we can drop the terminator if required. In this context\n>\n> s/required/desired/\n>\n\nwill change.\n\n>> introduce a wrapper function 'strbuf_split_str_without_term()' which\n>> splits a given string into strbufs without including the terminator.\n>>\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> @@ -892,14 +892,11 @@ static void populate_value(struct ref_array_item *ref)\n>> * TODO: Implement a function similar to strbuf_split_str()\n>> * which would omit the separator from the end of each value.\n>> */\n>> - s = to_free = strbuf_split_str(valp, ',', 0);\n>> + s = to_free = strbuf_split_str_without_term(valp, ',', 0);\n>>\n>> align->position = ALIGN_LEFT;\n>>\n>> while (*s) {\n>> - /* Strip trailing comma */\n>> - if (s[1])\n>> - strbuf_setlen(s[0], s[0]->len - 1);\n>\n> I'd prefer to see this ref-filter.c change split out as a separate\n> patch so as not to pollute the otherwise single-purpose change\n> introduced by this patch (i.e. capability to omit the terminator).\n>\n> Also, it might make sense to move this patch to the head of the\n> series, since it's conceptually distinct from the rest of the patches,\n> and could conceivably prove useful on its own, regardless of how the\n> rest of the series fares.\n>\n\nI guess it makes sense to split this into two separate patches. I'll do that and\npush it to the top of the series.\n\n>> if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))\n>> ;\n>> else if (!strcmp(s[0]->buf, \"left\"))\n>> diff --git a/strbuf.c 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 with_term)\n>\n> \"with_term\" might undesirably be interpreted as meaning \"use this\n> particular term\". Perhaps a different name, such as \"include_term\",\n> \"drop_term\", or \"omit_term\" would be a bit less ambiguous. (I think I\n> prefer \"omit_term\".)\n>\n\nTrue. I too prefer \"omit_term\" from what you mentioned I'll stick to that.\n\n>> {\n>> struct strbuf **ret = NULL;\n>> size_t nr = 0, alloc = 0;\n>> @@ -123,18 +123,27 @@ struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n>>\n>> while (slen) {\n>> int len = slen;\n>> + int term = with_term;\n>\n> \"term\" is not a great variable name, and is easily confused with the\n> existing \"terminator\" input argument. This is really being used as a\n> length adjustment, so perhaps a name such as 'term_adjust' or\n> 'len_adjust' or something better would be preferable.\n\n'len_adjust' seems better, will change.\n\n>\n> Also, since the value of 'with_term' never changes, then 'term' will\n> have the same value each time through the loop, thus you could\n> (should) hoist the declaration and initialization of 'term' outside of\n> the loop.\n>\n\nTrue, will move it out of the loop.\n\n> Due to the way you're using this variable ('term'), you want its value\n> always to be 0 or 1 but you don't do anything to ensure that. What if\n> the user passes in 42 rather than 0 or 1? That would mess up your\n> (below) calculations. Worse, what if the user passes in -42? That\n> would be particularly alarming. To turn this into a boolean value (0\n> or 1), do this instead:\n>\n> int term = !!with_term;\n\nMakes sense, will change.\n\n>\n>> if (max <= 0 || nr + 1 < max) {\n>> const char *end = memchr(str, terminator, slen);\n>> if (end)\n>> - len = end - str + 1;\n>> + len = end - str + term;\n>> + else\n>> + /* When no terminator present, we must add the last character */\n>> + term = 1;\n>> }\n>> t = xmalloc(sizeof(struct strbuf));\n>> strbuf_init(t, len);\n>> strbuf_add(t, str, len);\n>> ALLOC_GROW(ret, nr + 2, alloc);\n>> ret[nr++] = t;\n>> - str += len;\n>> - slen -= len;\n>> + if (!term) {\n>> + str += len + 1;\n>> + slen -= len + 1;\n>> + } else {\n>> + str += len;\n>> + slen -= len;\n>> + }\n>\n> This new logic is complex and confusing, thus difficult to review for\n> correctness. Rather than messing with 'len' and the existing logic,\n> how about instead, just adjusting the amount you store in the strbuf?\n> That is, instead of all the above changes, you might be able to get by\n> with one little change, something like this (untested):\n>\n> - strbuf_add(t, str, len);\n> + strbuf_add(t, str, len - !!end * !!with_term);\n>\n\nThat seems about right, this should be easier to understand.\n\n>> }\n>> ALLOC_GROW(ret, nr + 1, alloc); /* In case string was empty */\n>> ret[nr] = NULL;\n>> diff --git a/strbuf.h b/strbuf.h\n>> @@ -465,19 +465,25 @@ static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)\n>> * For lighter-weight alternatives, see string_list_split() and\n>> * string_list_split_in_place().\n>> */\n>> -extern struct strbuf **strbuf_split_buf(const char *, size_t,\n>> - int terminator, int max);\n>> +extern struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n>> + int terminator, int max, int with_term);\n>\n> You also need to update the comment block above this declaration since\n> it still says that each substring includes the terminator. It also\n> fails to mention the new 'with_term' argument added by this patch.\n>\n\nWill do :)\n\n>> +static inline struct strbuf **strbuf_split_str_without_term(const char *str,\n>> + int terminator, int max)\n>\n> This is an uncomfortably long function name. Unfortunately, short and\n> sweet strbuf_split() is already taken. Perhaps\n> strbuf_split_str_drop_term()? strbuf_split_str_omit_term()?\n> strbuf_split_str_no_term()? strbuf_split_noterm()?\n>\n\nstrbuf_split_str_omit_term() would be nice keeping it consistent with the\nvariable name omit_term.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"274115","messageId":"CAOLa=ZSE=zfyxSOQkv+uE=Z4qK9bcMN11kMm2R7gZE+M3mDNFQ@mail.gmail.com","threadId":"40776","inReplyTo":"CAPig+cTYt1qaSLLOmwyZ-BQzOjWGxZ5koJNma1qc67UipPFW5w@mail.gmail.com","subject":"Re: [PATCH/RFC 07/10] ref-filter: introduce align_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-07T17:00:32Z","receivedAt":"2015-12-07T17:00:32Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Thu, Dec 3, 2015 at 2:53 AM, Eric Sunshine wrote:\n> On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak wrote:\n>> Introduce align_atom_parser() which will parse 'align' atoms and store\n>> the required width and position into the 'used_atom' structure. While\n>> we're here, add support for the usage of 'width=' and 'position=' when\n>> using the 'align' atom (e.g. %(align:position=middle,width=30)).\n>\n> This patch is doing too much by both moving code around and modifying\n> that code (somewhat dramatically), thus it is difficult for reviewers\n> to compare the old and new behaviors. It deserves to be split apart\n> into at least two patches. First, the code movement patch which\n> introduces align_atom_parser() (and possibly get_align_position())\n> without any behavior or logical change; then the patch which changes\n> behavior to recognize the spelled-out forms \"width=\" and \"position=\".\n> You may even want to spilt it into more patches, for instance by doing\n> the get_align_position() extraction in its own patch.\n>\n\nI split it into two separate patches:\n1. add align_atom_parser()\n2. introduce \"width=\" and \"position=\" prefixes.\n\nI think now it seems easier to follow.\n\n>> Add documentation and modify the existing tests in t6302 to reflect\n>> the same.\n>>\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\n>> @@ -129,14 +129,16 @@ color::\n>> align::\n>> Left-, middle-, or right-align the content between\n>> - %(align:...) and %(end). The \"align:\" is followed by ``\n>> - and `` in any order separated by a comma, where the\n>> - `` is either left, right or middle, default being\n>> - left and `` is the total length of the content with\n>> - alignment. If the contents length is more than the width then\n>> - no alignment is performed. If used with '--quote' everything\n>> - in between %(align:...) and %(end) is quoted, but if nested\n>> - then only the topmost level performs quoting.\n>> + %(align:...) and %(end). The \"align:\" is followed by\n>> + `width=` and `position=` in any order\n>> + separated by a comma, where the `` is either left,\n>> + right or middle, default being left and `` is the total\n>> + length of the content with alignment. The prefix for the\n>> + arguments is not mandatory. If the contents length is more\n>\n> This paragraph is so bulky that it's very easy to overlook the bit\n> about the \"prefix for the arguments\" being optional, and it's not\n> necessarily even clear to the casual reader what that means. It might,\n> therefore, be a good idea to spell it out explicitly. For instance,\n> you might say something like:\n>\n> For brevity, the \"width=\" and/or \"position=\" prefixes may be\n> omitted, and bare and used instead.\n> For instance, `%(align:,)`.\n>\n\nAdded this in.\n\n>> + than the width then no alignment is performed. If used with\n>> + '--quote' everything in between %(align:...) and %(end) is\n>> + quoted, but if nested then only the topmost level performs\n>> + quoting.\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> @@ -63,6 +69,61 @@ void color_atom_parser(struct used_atom *atom)\n>> +static align_type get_align_position(const char *type)\n>\n> Taken in context of the callers, this isn't a great function name, as\n> it implies that it is retrieving some value, when in fact it is\n> parsing the input argument. A better name might be\n> parse_align_position().\n>\n> Likewise, 'type' isn't necessarily a great argument name. You might\n> instead call it 'pos' or even just short and sweet 's'.\n>\n\nWill change.\n\n>> +{\n>> + if (!strcmp(type, \"right\"))\n>> + return ALIGN_RIGHT;\n>> + else if (!strcmp(type, \"middle\"))\n>> + return ALIGN_MIDDLE;\n>> + else if (!strcmp(type, \"left\"))\n>> + return ALIGN_LEFT;\n>> + return -1;\n>> +}\n>> +\n>> +void align_atom_parser(struct used_atom *atom)\n>> +{\n>> + struct align *align = &atom->u.align;\n>> + const char *buf = NULL;\n>> + struct strbuf **s, **to_free;\n>> + int width = -1;\n>> +\n>> + match_atom_name(atom->str, \"align\", &buf);\n>> + if (!buf)\n>> + die(_(\"expected format: %%(align:,)\"));\n>\n> Is this still the way you want this error message to appear, or should\n> it show the long-form of the arguments? (I don't care strongly.)\n>\n\nI think it still holds good, not too keen on changing it either.\n\n>> + s = to_free = strbuf_split_str_without_term(buf, ',', 0);\n>> +\n>> + /* By default align to ALGIN_LEFT */\n>\n> What is ALGIN? Regardless of the answer, this comment is not\n> particularly useful since it merely repeats what the code itself\n> already states clearly.\n>\n\nwill do.\n\n>> + align->position = ALIGN_LEFT;\n>> +\n>> + while (*s) {\n>> + int position;\n>> + buf = s[0]->buf;\n>> +\n>> + position = get_align_position(buf);\n>\n> Why is this assignment way up here rather than down below in the\n> penultimate 'else' arm where its result is actually being checked? By\n> moving it closer to the point of use, the logic becomes easier to\n> understand.\n>\n\nmakes sense, will do.\n\n>> + if (skip_prefix(buf, \"position=\", &buf)) {\n>> + position = get_align_position(buf);\n>> + if (position == -1)\n>> + die(_(\"improper format entered align:%s\"), s[0]->buf);\n>\n> At this point, you can give a better error message since you know that\n> you were parsing a \"position=\" argument. Maybe something like\n> \"unrecognized position: %s\".\n>\n\nthanks, will add this in.\n\n>> + align->position = position;\n>> + } else if (skip_prefix(buf, \"width=\", &buf)) {\n>> + if (strtoul_ui(buf, 10, (unsigned int *)&width))\n>> + die(_(\"improper format entered align:%s\"), s[0]->buf);\n>\n> Ditto regarding better error message.\n>\n\nwill do.\n\n>> + } else if (!strtoul_ui(buf, 10, (unsigned int *)&width))\n>> + ;\n>> + else if (position != -1)\n>> + align->position = position;\n>> + else\n>> + die(_(\"improper format entered align:%s\"), s[0]->buf);\n>\n> Here too, it would be more user-friendly to say \"unrecognized\n> %%(align) argument: %s\".\n>\n\nwill change.\n\n>> + s++;\n>> + }\n>> +\n>> + if (width < 0)\n>> + die(_(\"positive width expected with the %%(align) atom\"));\n>> + align->width = width;\n>> + strbuf_list_free(to_free);\n>> +}\n>> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\n>> @@ -97,7 +97,7 @@ test_expect_success 'left alignment is default' '\n>> refname is refs/tags/three |refs/tags/three\n>> refname is refs/tags/two |refs/tags/two\n>> EOF\n>> - git for-each-ref --format=\"%(align:30)refname is %(refname)%(end)|%(refname)\" >actual &&\n>> + git for-each-ref --format=\"%(align:width=30)refname is %(refname)%(end)|%(refname)\" >actual &&\n>> test_cmp expect actual\n>> '\n>>\n>> @@ -113,7 +113,7 @@ test_expect_success 'middle alignment' '\n>> | refname is refs/tags/three |refs/tags/three\n>> | refname is refs/tags/two |refs/tags/two\n>> EOF\n>> - git for-each-ref --format=\"|%(align:middle,30)refname is %(refname)%(end)|%(refname)\" >actual &&\n>> + git for-each-ref --format=\"|%(align:position=middle,30)refname is %(refname)%(end)|%(refname)\" >actual &&\n>> test_cmp expect actual\n>> '\n>\n> While it may sometimes be reasonable to re-purpose existing tests like\n> this, this probably is not one of those cases. Instead, you should be\n> adding new tests to check all the permutations of the new argument\n> handling. For instance:\n>\n> %(align:42)\n> %(align:middle,42)\n> %(align:42,middle)\n> %(align:position=middle,42)\n> %(align:42,position=middle)\n> %(align:middle,width=42)\n> %(align:width=42,middle)\n> %(align:position=middle,width=42)\n> %(align:width=42,position=middle)\n>\n> And, it wouldn't hurt to test handling of redundant or extra position\n> and width arguments. Should multiple arguments of the same type result\n> in an error, or should \"last one wins (sliently)\" be the policy? Once\n> you decide upon a policy, add tests to check that that policy works as\n> expected.\n>\n\nCurrently like you said, it works on a \"last one wins(silently)\" policy.\nI think this is what I'd stick with.\nSo like you said I'll implement the combinations of tests as suggested\nand those suggested below too.\n\n> In this case, \"last one wins (silently)\" may be more friendly to\n> script writers, so it might be the better choice. You'd want to add\n> appropriate tests, using the various permutations. For instance:\n>\n> %(align:42,width=43)\n> %(align:width=43,42)\n> %(align:42,position=middle,right)\n> %(align:42,right,position=middle)\n\nThanks for the review :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"274295","messageId":"xmqqio44eijo.fsf@gitster.mtv.corp.google.com","threadId":"40776","inReplyTo":"CAOLa=ZTHaety8=cxP4PqcXqrRv9OAEAcRyTs9drY6nSFGKvyOQ@mail.gmail.com","subject":"Re: [PATCH/RFC 01/10] ref-filter: introduce a parsing function for each atom in valid_atom","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-11T22:18:03Z","receivedAt":"2015-12-11T22:18:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n>> This problem will go away if you introduce the 'valid_atom' field in\n>> the patch which actually needs it (as suggested above) rather than in\n>> this patch.\n>\n> Yup, agreed.\n> Thanks for your suggestions.\n\nIn addition, most of the lines in this patch should become\nunnecessary even after you start using the \"parser\" field.\n\nAs the majority of fields are compared as strings, and only a\nselected few fields need custom parsers, you do not have to\nexplicitly write FIELD_STR everywhere, as long as you make sure the\nvalue of FIELD_STR is zero (and use parser==NULL as a sign that no\ncustom parser is used, which I think you already do).\n"},{"id":"274296","messageId":"xmqq8u50ehxk.fsf@gitster.mtv.corp.google.com","threadId":"40776","inReplyTo":"CAOLa=ZQz-NE3uKtU72NUyR23QaJE4jf=UqiMptnM+hOmAE9GRw@mail.gmail.com","subject":"Re: [PATCH/RFC 06/10] strbuf: introduce strbuf_split_str_without_term()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-11T22:31:19Z","receivedAt":"2015-12-11T22:31:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n>>> diff --git a/ref-filter.c b/ref-filter.c\n>>> @@ -892,14 +892,11 @@ static void populate_value(struct ref_array_item *ref)\n>>> * TODO: Implement a function similar to strbuf_split_str()\n>>> * which would omit the separator from the end of each value.\n>>> */\n>>> - s = to_free = strbuf_split_str(valp, ',', 0);\n>>> + s = to_free = strbuf_split_str_without_term(valp, ',', 0);\n>>>\n>>> align->position = ALIGN_LEFT;\n>>>\n>>> while (*s) {\n>>> - /* Strip trailing comma */\n>>> - if (s[1])\n>>> - strbuf_setlen(s[0], s[0]->len - 1);\n>>\n>> I'd prefer to see this ref-filter.c change split out as a separate\n>> patch so as not to pollute the otherwise single-purpose change\n>> introduced by this patch (i.e. capability to omit the terminator).\n>>\n>> Also, it might make sense to move this patch to the head of the\n>> series, since it's conceptually distinct from the rest of the patches,\n>> and could conceivably prove useful on its own, regardless of how the\n>> rest of the series fares.\n>>\n>\n> I guess it makes sense to split this into two separate patches. I'll do that and\n> push it to the top of the series.\n\nSounds good. I also notice that the \"TODO: Implement a function\nsimilar to...\" we see in the precontext can now be removed, as that\nis what is done in this patch?\n"},{"id":"274297","messageId":"xmqq1taseh2x.fsf@gitster.mtv.corp.google.com","threadId":"40776","inReplyTo":"1447271075-15364-1-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH/RFC 00/10] ref-filter: use parsing functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-11T22:49:42Z","receivedAt":"2015-12-11T22:49:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> Karthik Nayak (10):\n> ref-filter: introduce a parsing function for each atom in valid_atom\n> ref-filter: introduce struct used_atom\n> ref-fitler: bump match_atom() name to the top\n> ref-filter: skip deref specifier in match_atom_name()\n> ref-filter: introduce color_atom_parser()\n> strbuf: introduce strbuf_split_str_without_term()\n> ref-filter: introduce align_atom_parser()\n> ref-filter: introduce remote_ref_atom_parser()\n> ref-filter: introduce contents_atom_parser()\n> ref-filter: introduce objectname_atom_parser()\n\nIt seems that this series had seen quite a good inputs, mostly from\nEric. I finished reading it over and I didn't find anything more to\nadd. The patches are mostly good and would be ready once these\npoints raised during the review are addressed, I think\n\nThanks.\n"},{"id":"274317","messageId":"CAOLa=ZQeK4VGf3n7vOtUspaak9gXn+G-DV9rwWDjr_n2_LeU6g@mail.gmail.com","threadId":"40776","inReplyTo":"xmqq8u50ehxk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH/RFC 06/10] strbuf: introduce strbuf_split_str_without_term()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-12T16:04:00Z","receivedAt":"2015-12-12T16:04:00Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sat, Dec 12, 2015 at 4:01 AM, Junio C Hamano wrote:\n> Karthik Nayak writes:\n>\n>>>> diff --git a/ref-filter.c b/ref-filter.c\n>>>> @@ -892,14 +892,11 @@ static void populate_value(struct ref_array_item *ref)\n>>>> * TODO: Implement a function similar to strbuf_split_str()\n>>>> * which would omit the separator from the end of each value.\n>>>> */\n>>>> - s = to_free = strbuf_split_str(valp, ',', 0);\n>>>> + s = to_free = strbuf_split_str_without_term(valp, ',', 0);\n>>>>\n>>>> align->position = ALIGN_LEFT;\n>>>>\n>>>> while (*s) {\n>>>> - /* Strip trailing comma */\n>>>> - if (s[1])\n>>>> - strbuf_setlen(s[0], s[0]->len - 1);\n>>>\n>>> I'd prefer to see this ref-filter.c change split out as a separate\n>>> patch so as not to pollute the otherwise single-purpose change\n>>> introduced by this patch (i.e. capability to omit the terminator).\n>>>\n>>> Also, it might make sense to move this patch to the head of the\n>>> series, since it's conceptually distinct from the rest of the patches,\n>>> and could conceivably prove useful on its own, regardless of how the\n>>> rest of the series fares.\n>>>\n>>\n>> I guess it makes sense to split this into two separate patches. I'll do that and\n>> push it to the top of the series.\n>\n> Sounds good. I also notice that the \"TODO: Implement a function\n> similar to...\" we see in the precontext can now be removed, as that\n> is what is done in this patch?\n\nYes I noticed that when going through the changes Eric suggested. I have\nremoved it.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"274318","messageId":"CAOLa=ZQKQE-iyNY2gyZXMxPANHzJr0GoDJVuts3qb-izDKVVsQ@mail.gmail.com","threadId":"40776","inReplyTo":"xmqqio44eijo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH/RFC 01/10] ref-filter: introduce a parsing function for each atom in valid_atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-12T16:05:19Z","receivedAt":"2015-12-12T16:05:19Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sat, Dec 12, 2015 at 3:48 AM, Junio C Hamano wrote:\n> Karthik Nayak writes:\n>\n>>> This problem will go away if you introduce the 'valid_atom' field in\n>>> the patch which actually needs it (as suggested above) rather than in\n>>> this patch.\n>>\n>> Yup, agreed.\n>> Thanks for your suggestions.\n>\n> In addition, most of the lines in this patch should become\n> unnecessary even after you start using the \"parser\" field.\n>\n> As the majority of fields are compared as strings, and only a\n> selected few fields need custom parsers, you do not have to\n> explicitly write FIELD_STR everywhere, as long as you make sure the\n> value of FIELD_STR is zero (and use parser==NULL as a sign that no\n> custom parser is used, which I think you already do).\n\nTrue, currently I've changed it to only add the new parser function values\nonly wherever required.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"274329","messageId":"CAPig+cQrTvDQdCiJGLs8iFj2nS1RvtzbSrDRBpx500JRsAiEtQ@mail.gmail.com","threadId":"40776","inReplyTo":"1447271075-15364-9-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH/RFC 08/10] ref-filter: introduce remote_ref_atom_parser()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-13T00:53:41Z","receivedAt":"2015-12-13T00:53:41Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Nov 11, 2015 at 2:44 PM, 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> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> @@ -37,6 +37,11 @@ static struct used_atom {\n> union {\n> const char *color;\n> struct align align;\n> + struct {\n> + unsigned int shorten : 1,\n> + track : 1,\n> + trackshort : 1;\n> + } remote_ref;\n\nAre 'shorten', 'track', and 'trackshort' mutually exclusive? If so, a\nsimple enum would be clearer than bitfields:\n\n union {\n const char *color;\n struct align align;\n enum { RR_PLAIN, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }\n remote_ref;\n };\n\nOr something.\n\n> } u;\n> } *used_atom;\n> static int used_atom_cnt, need_tagged, need_symref;\n> @@ -69,6 +74,24 @@ void color_atom_parser(struct used_atom *atom)\n> +void remote_ref_atom_parser(struct used_atom *atom)\n> +{\n> + const char *buf;\n> +\n> + buf = strchr(atom->str, ':');\n> + if (!buf)\n> + return;\n> + buf++;\n> + if (!strcmp(buf, \"short\"))\n> + atom->u.remote_ref.shorten = 1;\n> + else if (!strcmp(buf, \"track\"))\n> + atom->u.remote_ref.track = 1;\n> + else if (!strcmp(buf, \"trackshort\"))\n> + atom->u.remote_ref.trackshort = 1;\n> + else\n> + die(_(\"improper format entered align:%s\"), buf);\n\n\"align:\"? Also, how about a more grammatically-friendly error message?\n\n> +}\n> +\n> @@ -838,6 +861,42 @@ static inline char *copy_advance(char *dst, const char *src)\n> +static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n> + struct branch *branch, const char **s)\n> +{\n> + int num_ours, num_theirs;\n> + if (atom->u.remote_ref.shorten)\n> + *s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);\n> + else if (atom->u.remote_ref.track) {\n> + if (stat_tracking_info(branch, &num_ours,\n> + &num_theirs, NULL))\n> + return;\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\nTangent: These xstrfmt()'d strings are getting leaked, right? Is that\nsomething that we need to worry about (if, for instance, a repository\ncontains a lot of tracking refs)? Should there be a NEEDSWORK comment\nhere regarding the issue?\n\n> + } else if (atom->u.remote_ref.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\n> + *s = refname;\n> +}\n> +\n> /*\n> * Parse the object referred by ref, and grab needed value.\n> */\n> @@ -948,49 +1011,11 @@ static void populate_value(struct ref_array_item *ref)\n>\n> formatp = strchr(name, ':');\n> if (formatp) {\n> - int num_ours, num_theirs;\n> -\n> formatp++;\n> if (!strcmp(formatp, \"short\"))\n> refname = shorten_unambiguous_ref(refname,\n> warn_ambiguous_refs);\n\nIs this duplicating work already done by fill_remote_ref_details()?\n\n> - else if (!strcmp(formatp, \"track\") &&\n> - (starts_with(name, \"upstream\") ||\n> - starts_with(name, \"push\"))) {\n> -\n> - if (stat_tracking_info(branch, &num_ours,\n> - &num_theirs, NULL))\n> - continue;\n> -\n> - if (!num_ours && !num_theirs)\n> - v->s = \"\";\n> - else if (!num_ours)\n> - v->s = xstrfmt(\"[behind %d]\", num_theirs);\n> - else if (!num_theirs)\n> - v->s = xstrfmt(\"[ahead %d]\", num_ours);\n> - else\n> - v->s = xstrfmt(\"[ahead %d, behind %d]\",\n> - num_ours, num_theirs);\n> - continue;\n> - } else if (!strcmp(formatp, \"trackshort\") &&\n> - (starts_with(name, \"upstream\") ||\n> - starts_with(name, \"push\"))) {\n> - assert(branch);\n> -\n> - if (stat_tracking_info(branch, &num_ours,\n> - &num_theirs, NULL))\n> - continue;\n> -\n> - if (!num_ours && !num_theirs)\n> - v->s = \"=\";\n> - else if (!num_ours)\n> - v->s = \"<\";\n> - else if (!num_theirs)\n> - v->s = \">\";\n> - else\n> - v->s = \"<>\";\n> - continue;\n> - } else\n> + else\n> die(\"unknown %.*s format %s\",\n> (int)(formatp - name), name, formatp);\n> }\n> --\n> 2.6.2\n"},{"id":"274332","messageId":"CAPig+cTbT60cWk+=6EARte68mmQ2xkS5xFgsgh0FbpoueFcBbg@mail.gmail.com","threadId":"40776","inReplyTo":"1447271075-15364-10-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH/RFC 09/10] ref-filter: introduce contents_atom_parser()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-13T03:10:16Z","receivedAt":"2015-12-13T03:10:16Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak wrote:\n> Introduce contents_atom_parser() which will parse the '%(contents)'\n> atom and store information into the 'used_atom' structure based on the\n> modifiers used along with the atom.\n>\n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 802629b..117bbbb 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -42,6 +42,14 @@ static struct used_atom {\n> track : 1,\n> trackshort : 1;\n> } remote_ref;\n> + struct {\n> + unsigned int subject : 1,\n> + body : 1,\n> + signature : 1,\n> + all : 1,\n> + lines : 1,\n> + no_lines;\n> + } contents;\n\nSame question as patch 8/10: With the exception of non-bitfield\n'no_lines', are these 'contents' flags mutually exclusive? If so, an\nenum would be a more natural representation than bitfields.\n\n> } u;\n> } *used_atom;\n> static int used_atom_cnt, need_tagged, need_symref;\n> @@ -92,6 +100,29 @@ void remote_ref_atom_parser(struct used_atom *atom)\n> +void contents_atom_parser(struct used_atom *atom)\n> +{\n> + const char * buf;\n> +\n> + if (match_atom_name(atom->str, \"contents\", &buf))\n> + atom->u.contents.all = 1;\n> +\n> + if (!buf)\n> + return;\n\nHmm, I'd have placed the blank line after the 'if (!buf) return;'\nrather than before it.\n\n> + if (!strcmp(buf, \"body\"))\n> + atom->u.contents.body = 1;\n> + else if (!strcmp(buf, \"signature\"))\n> + atom->u.contents.signature = 1;\n> + else if (!strcmp(buf, \"subject\"))\n> + atom->u.contents.subject = 1;\n> + else if (skip_prefix(buf, \"lines=\", &buf)) {\n> + atom->u.contents.lines = 1;\n> + if (strtoul_ui(buf, 10, &atom->u.contents.no_lines))\n> + die(_(\"positive value expected contents:lines=%s\"), buf);\n> + } else\n> + die(_(\"improper format entered contents:%s\"), buf);\n\nPerhaps a more grammatically-friendly error message?\n\n> +}\n> +\n> static align_type get_align_position(const char *type)\n> {\n> if (!strcmp(type, \"right\"))\n> @@ -761,20 +786,16 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj\n> unsigned long sublen = 0, bodylen = 0, nonsiglen = 0, siglen = 0;\n>\n> for (i = 0; i < used_atom_cnt; i++) {\n> - const char *name = used_atom[i].str;\n> + struct used_atom *atom = &used_atom[i];\n> + const char *name = atom->str;\n> struct atom_value *v = &val[i];\n> - const char *valp = NULL;\n> if (!!deref != (*name == '*'))\n> continue;\n> if (deref)\n> name++;\n> if (strcmp(name, \"subject\") &&\n> strcmp(name, \"body\") &&\n> - strcmp(name, \"contents\") &&\n> - strcmp(name, \"contents:subject\") &&\n> - strcmp(name, \"contents:body\") &&\n> - strcmp(name, \"contents:signature\") &&\n> - !starts_with(name, \"contents:lines=\"))\n> + !atom->u.contents.all)\n> continue;\n> if (!subpos)\n> find_subpos(buf, sz,\n> @@ -784,26 +805,23 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj\n>\n> if (!strcmp(name, \"subject\"))\n> v->s = copy_subject(subpos, sublen);\n> - else if (!strcmp(name, \"contents:subject\"))\n> + else if (atom->u.contents.subject)\n> v->s = copy_subject(subpos, sublen);\n\nWith the disclaimer that I haven't fully digested the existing logic,\nis there a reason that you don't also preprocess bare \"subject\" as you\npreprocess \"contents:subject\"? Isn't \"subject\" just historic aliases\nfor \"contents:subject\"?\n\nA similar observation may be made about \"body\" and \"contents:body\",\nalthough I see they mean slightly different things (for, I suppose,\nhistorical reasons).\n\n> else if (!strcmp(name, \"body\"))\n> v->s = xmemdupz(bodypos, bodylen);\n> - else if (!strcmp(name, \"contents:body\"))\n> + else if (atom->u.contents.body)\n> v->s = xmemdupz(bodypos, nonsiglen);\n> - else if (!strcmp(name, \"contents:signature\"))\n> + else if (atom->u.contents.signature)\n> v->s = xmemdupz(sigpos, siglen);\n> - else if (!strcmp(name, \"contents\"))\n> - v->s = xstrdup(subpos);\n> - else if (skip_prefix(name, \"contents:lines=\", &valp)) {\n> + else if (atom->u.contents.lines) {\n> struct strbuf s = STRBUF_INIT;\n> const char *contents_end = bodylen + bodypos - siglen;\n>\n> - if (strtoul_ui(valp, 10, &v->u.contents.lines))\n> - die(_(\"positive value expected contents:lines=%s\"), valp);\n> /* Size is the length of the message after removing the signature */\n> - append_lines(&s, subpos, contents_end - subpos, v->u.contents.lines);\n> + append_lines(&s, subpos, contents_end - subpos, atom->u.contents.no_lines);\n> v->s = strbuf_detach(&s, NULL);\n> - }\n> + } else /* For %(contents) without modifiers */\n\nToo many blanks following '/*'; downcase 'for' or drop it altogether:\n\n /* bare %(contents) */\n\n> + v->s = xstrdup(subpos);\n> }\n> }\n>\n> --\n> 2.6.2\n"},{"id":"274333","messageId":"CAPig+cTVqTc1S8gduLq3qiyuzJNb=LNQiXijEHwqPPzi1YU8UQ@mail.gmail.com","threadId":"40776","inReplyTo":"CAPig+cTbT60cWk+=6EARte68mmQ2xkS5xFgsgh0FbpoueFcBbg@mail.gmail.com","subject":"Re: [PATCH/RFC 09/10] ref-filter: introduce contents_atom_parser()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-13T04:41:18Z","receivedAt":"2015-12-13T04:41:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Dec 12, 2015 at 10:10 PM, Eric Sunshine wrote:\n> On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak wrote:\n>> +void contents_atom_parser(struct used_atom *atom)\n>> +{\n>> + const char * buf;\n>> +\n>> + if (match_atom_name(atom->str, \"contents\", &buf))\n>> + atom->u.contents.all = 1;\n>> +\n>> + if (!buf)\n>> + return;\n\nAlso, isn't this logic somewhat bogus? u.contents.all is set to 1 for\nboth bare %(contents) and decorated %(contents:whatever). Then, below,\nyou also set one of .body, .signature, .subject, or .lines if a\ndecoration is specified. So, now you have both .all and one of the\nother attributes set to 1, which is rather nonsensical (if I\nunderstand correctly).\n\nIf you change this to an enum as suggested in my previous email, then\nthe problem goes away.\n\n>> + if (!strcmp(buf, \"body\"))\n>> + atom->u.contents.body = 1;\n>> + else if (!strcmp(buf, \"signature\"))\n>> + atom->u.contents.signature = 1;\n>> + else if (!strcmp(buf, \"subject\"))\n>> + atom->u.contents.subject = 1;\n>> + else if (skip_prefix(buf, \"lines=\", &buf)) {\n>> + atom->u.contents.lines = 1;\n>> + if (strtoul_ui(buf, 10, &atom->u.contents.no_lines))\n>> + die(_(\"positive value expected contents:lines=%s\"), buf);\n>> + } else\n>> + die(_(\"improper format entered contents:%s\"), buf);\n>> +}\n"},{"id":"274334","messageId":"CAPig+cTduYxSWh5ghmv3vM04saAqrn9TUpBZ0J=TuDOgr05itQ@mail.gmail.com","threadId":"40776","inReplyTo":"1448459082-24492-1-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH/RFC 10/10] ref-filter: introduce objectname_atom_parser()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-13T04:49:33Z","receivedAt":"2015-12-13T04:49:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Nov 25, 2015 at 8:44 AM, Karthik Nayak wrote:\n> Introduce objectname_atom_parser() which will parse the\n> '%(objectname)' atom and store information into the 'used_atom'\n> structure based on the modifiers used along with the atom.\n>\n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> @@ -50,6 +50,10 @@ static struct used_atom {\n> lines : 1,\n> no_lines;\n> } contents;\n> + struct {\n> + unsigned int shorten : 1,\n> + full : 1;\n> + } objectname;\n\nSame comment as in my patch 8 and 9 reviews: If 'shorten' and 'full'\nare mutually exclusive, then an enum would be clearer. In fact, if\nthere are only these two states (full and short), then this could be a\nsimple boolean named 'shorten'.\n\n> } u;\n> } *used_atom;\n> static int used_atom_cnt, need_tagged, need_symref;\n> @@ -123,6 +127,21 @@ void contents_atom_parser(struct used_atom *atom)\n> +void objectname_atom_parser(struct used_atom *atom)\n> +{\n> + const char * buf;\n> +\n> + if (match_atom_name(atom->str, \"objectname\", &buf))\n> + atom->u.objectname.full = 1;\n\nSame comment about bogus logic as in patch 9 review: u.objectname.full\nand u.objectname.shorten are both set to 1 for %(objectname:short).\n\n> +\n> + if (!buf)\n> + return;\n\nSame comment about misplaced blank line: Put the blank line after the\nconditional rather than before or drop it altogether.\n\n> + if (!strcmp(buf, \"short\"))\n> + atom->u.objectname.shorten = 1;\n> + else\n> + die(_(\"improper format entered objectname:%s\"), buf);\n\nMaybe just \"unrecognized objectname:%s\" or something?\n\n> +}\n> +\n> @@ -463,15 +482,16 @@ static void *get_obj(const unsigned char *sha1, struct object **obj, unsigned lo\n> }\n>\n> static int grab_objectname(const char *name, const unsigned char *sha1,\n> - struct atom_value *v)\n> + struct atom_value *v, struct used_atom *atom)\n> {\n> - if (!strcmp(name, \"objectname\")) {\n> - v->s = xstrdup(sha1_to_hex(sha1));\n> - return 1;\n> - }\n> - if (!strcmp(name, \"objectname:short\")) {\n> - v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));\n> - return 1;\n> + if (starts_with(name, \"objectname\")) {\n> + if (atom->u.objectname.shorten) {\n> + v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));\n> + return 1;\n> + } else if (atom->u.objectname.full) {\n> + v->s = xstrdup(sha1_to_hex(sha1));\n> + return 1;\n> + }\n> }\n> return 0;\n> }\n> @@ -495,7 +515,7 @@ static void grab_common_values(struct atom_value *val, int deref, struct object\n> v->s = xstrfmt(\"%lu\", sz);\n> }\n> else if (deref)\n> - grab_objectname(name, obj->sha1, v);\n> + grab_objectname(name, obj->sha1, v, &used_atom[i]);\n> }\n> }\n>\n> @@ -1004,7 +1024,7 @@ static void populate_value(struct ref_array_item *ref)\n> v->s = xstrdup(buf + 1);\n> }\n> continue;\n> - } else if (!deref && grab_objectname(name, ref->objectname, v)) {\n> + } else if (!deref && grab_objectname(name, ref->objectname, v, atom)) {\n> continue;\n> } else if (!strcmp(name, \"HEAD\")) {\n> const char *head;\n> --\n> 2.6.2\n"},{"id":"274335","messageId":"CAPig+cQtoUXuOZjGjev8MGYUMyjd+n_=o+jOVXkhPReSkWgxmw@mail.gmail.com","threadId":"40776","inReplyTo":"xmqq1taseh2x.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH/RFC 00/10] ref-filter: use parsing functions","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-13T05:40:35Z","receivedAt":"2015-12-13T05:40:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Dec 11, 2015 at 5:49 PM, Junio C Hamano wrote:\n> Karthik Nayak writes:\n>> ref-filter: introduce a parsing function for each atom in valid_atom\n>> ref-filter: introduce struct used_atom\n>> ref-fitler: bump match_atom() name to the top\n>> ref-filter: skip deref specifier in match_atom_name()\n>> ref-filter: introduce color_atom_parser()\n>> strbuf: introduce strbuf_split_str_without_term()\n>> ref-filter: introduce align_atom_parser()\n>> ref-filter: introduce remote_ref_atom_parser()\n>> ref-filter: introduce contents_atom_parser()\n>> ref-filter: introduce objectname_atom_parser()\n>\n> It seems that this series had seen quite a good inputs, mostly from\n> Eric. I finished reading it over and I didn't find anything more to\n> add. The patches are mostly good and would be ready once these\n> points raised during the review are addressed, I think\n\nI'm still a bit fuzzy about what this series is trying to achieve. It\nfeels like it wants to avoid doing repeated processing of unchanging\nbits of %(foo:bar) atoms for each ref processed, but it only partly\nachieves that goal. For instance, there are still an awful lot of\nstrcmp()s and starts_with()s in that inner loop, and even the\nunchanging %(color:) argument gets re-evaulated repeatedly, which is\nprobably quite expensive.\n\nIf the intention is to rid that inner loop of much of the expensive\nprocessing, then wouldn't we want to introduce an enum of valid atoms\nwhich is to be a member of 'struct used_atom', and have\npopulate_value() switch on the enum value rather than doing all the\nexpensive strcmp()s and starts_with()?\n\n enum atom_type {\n AT_REFNAME,\n AT_OBJECTTYPE,\n ...\n AT_COLOR,\n AT_ALIGN\n };\n\n static struct used_atom {\n enum atom_type atom;\n cmp_type cmp;\n union {\n char *color; /* parsed color */\n struct align align;\n enum { ... } remote_ref;\n struct {\n enum { ... } portion;\n unsigned int nlines;\n } contents;\n int short_objname;\n } u;\n } *used_atom;\n\nIn fact, the 'cmp_type cmp' field can be dropped altogether since it\ncan just as easily be looked up when needed by keeping 'enum\natom_type' and valid_atoms[] in-sync and indexing into valid_atoms[]\nby atom_type:\n\n struct used_atom *a = ...;\n cmp_type cmp = valid_atoms[a->atom].cmp_type;\n\nAs a bonus, an 'enum atom_type' would resolve the problem of how\nstarts_with() is abused in populate_value() for certain cases\n(assuming I'm understanding the logic), such as how matching of\n\"color\" could incorrectly match some yet-to-be-added atom named\n\"colorize\".\n"},{"id":"274336","messageId":"CAOLa=ZQKR4+a-hpL-8xjE-93btWpUt4zAfCGTHBGWwhvLtQoRg@mail.gmail.com","threadId":"40776","inReplyTo":"CAPig+cQrTvDQdCiJGLs8iFj2nS1RvtzbSrDRBpx500JRsAiEtQ@mail.gmail.com","subject":"Re: [PATCH/RFC 08/10] ref-filter: introduce remote_ref_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-13T06:02:09Z","receivedAt":"2015-12-13T06:02:09Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sun, Dec 13, 2015 at 6:23 AM, Eric Sunshine wrote:\n> On Wed, Nov 11, 2015 at 2:44 PM, 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>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> @@ -37,6 +37,11 @@ static struct used_atom {\n>> union {\n>> const char *color;\n>> struct align align;\n>> + struct {\n>> + unsigned int shorten : 1,\n>> + track : 1,\n>> + trackshort : 1;\n>> + } remote_ref;\n>\n> Are 'shorten', 'track', and 'trackshort' mutually exclusive? If so, a\n> simple enum would be clearer than bitfields:\n>\n> union {\n> const char *color;\n> struct align align;\n> enum { RR_PLAIN, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }\n> remote_ref;\n> };\n>\n> Or something.\n>\n\nSure, will do that.\n\n>> } u;\n>> } *used_atom;\n>> static int used_atom_cnt, need_tagged, need_symref;\n>> @@ -69,6 +74,24 @@ void color_atom_parser(struct used_atom *atom)\n>> +void remote_ref_atom_parser(struct used_atom *atom)\n>> +{\n>> + const char *buf;\n>> +\n>> + buf = strchr(atom->str, ':');\n>> + if (!buf)\n>> + return;\n>> + buf++;\n>> + if (!strcmp(buf, \"short\"))\n>> + atom->u.remote_ref.shorten = 1;\n>> + else if (!strcmp(buf, \"track\"))\n>> + atom->u.remote_ref.track = 1;\n>> + else if (!strcmp(buf, \"trackshort\"))\n>> + atom->u.remote_ref.trackshort = 1;\n>> + else\n>> + die(_(\"improper format entered align:%s\"), buf);\n>\n> \"align:\"? Also, how about a more grammatically-friendly error message?\n>\n\nBad copy-paste, will change that.\n\n>> +}\n>> +\n>> @@ -838,6 +861,42 @@ static inline char *copy_advance(char *dst, const char *src)\n>> +static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n>> + struct branch *branch, const char **s)\n>> +{\n>> + int num_ours, num_theirs;\n>> + if (atom->u.remote_ref.shorten)\n>> + *s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);\n>> + else if (atom->u.remote_ref.track) {\n>> + if (stat_tracking_info(branch, &num_ours,\n>> + &num_theirs, NULL))\n>> + return;\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>\n> Tangent: These xstrfmt()'d strings are getting leaked, right? Is that\n> something that we need to worry about (if, for instance, a repository\n> contains a lot of tracking refs)? Should there be a NEEDSWORK comment\n> here regarding the issue?\n>\n\nThis is sort of a problem with most of the values in ref-filter, we dynamically\nallocate memory and do not free it, since the program exits soon after and\nwe leave it to the Operating System to do the garbage collection.\n\nNot sure if we'd want to work on this though.\n\n>> + } else if (atom->u.remote_ref.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\n>> + *s = refname;\n>> +}\n>> +\n>> /*\n>> * Parse the object referred by ref, and grab needed value.\n>> */\n>> @@ -948,49 +1011,11 @@ static void populate_value(struct ref_array_item *ref)\n>>\n>> formatp = strchr(name, ':');\n>> if (formatp) {\n>> - int num_ours, num_theirs;\n>> -\n>> formatp++;\n>> if (!strcmp(formatp, \"short\"))\n>> refname = shorten_unambiguous_ref(refname,\n>> warn_ambiguous_refs);\n>\n> Is this duplicating work already done by fill_remote_ref_details()?\n>\n\nNo, this is only activated when using \"refname\". fill_remote_ref_details()\nis used by the %(upstream) and %(push) atoms, both of which skip the\nloop using \"continue\" in populate_value().\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"274337","messageId":"CAPig+cSC8fP1KGM1wCXzcfj2QHgvoUUjfBncRH0jKLv1EnYKCg@mail.gmail.com","threadId":"40776","inReplyTo":"CAOLa=ZQY3PMtfemVAsbHydtguQG9pc2uVTiJ6Qh+jLf6mY4dFw@mail.gmail.com","subject":"Re: [PATCH/RFC 05/10] ref-filter: introduce color_atom_parser()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-13T06:05:57Z","receivedAt":"2015-12-13T06:05:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Dec 3, 2015 at 8:35 AM, Karthik Nayak wrote:\n> On Wed, Dec 2, 2015 at 4:57 AM, Eric Sunshine wrote:\n>> On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak wrote:\n>>> @@ -833,11 +846,10 @@ static void populate_value(struct ref_array_item *ref)\n>>> refname = branch_get_push(branch, NULL);\n>>> if (!refname)\n>>> continue;\n>>> - } else if (match_atom_name(name, \"color\", &valp)) {\n>>> + } else if (starts_with(name, \"color\")) {\n>>\n>> Hmm, so this will also match \"colorize\". Is that desirable?\n>\n> Well the error checking is done when we parse the atom in color_atom_parser()\n> so here we don't need to worry about something like this.\n\nI'm not sure that I understand your response. Let's say that, in the\nfuture, someone adds a new atom named \"colorize\" (which may or may not\nhave its own parser in the valid_atom[] table). color_atom_parser()\nwill never see that atom, thus error checking in color_atom_parser()\nis not relevant to this case. What is relevant is that the original\ncode:\n\n } if (match_atom_name(name, \"color\", &valp)) {\n\nonly matched %(color) or %(color:whatever). It did not match\n%(colorize). However, the new code:\n\n } else if (starts_with(name, \"color\")) {\n\nis far looser and will match %(colorize) and %(color) and\n%(color:whatever) and %(coloranything), which is potentially\nundesirable. It's true that the person adding %(colorize) could be\ncareful and ensure that the if/else chain checks %(colorize) first:\n\n } else if (!strcmp(name, \"colorize\") {\n ...\n } else if (starts_with(name, \"color\")) {\n ...\n } else ...\n\nbut that places a certain extra burden on that person. Alternately,\nyou can tighten the matching so that it is as strict as the original:\n\n } else if (!strcmp(name, \"color\") || starts_with(name, \"color:\")) {\n ...\n } else ...\n\nOr perhaps upgrade match_atom_name() to make the 'val' argument\noptional, in which case you might be able to do something like this:\n\n } else if (match_atom_name(name, \"color\", NULL) {\n\n(However, if you introduce an 'enum atom_type' as suggested in my\nresponse to the cover letter, then this problem goes away because\nyou'd be switching on the enum value, which is determinate, rather\nthan on a partial string, which may be ambiguous, as illustrated.)\n"},{"id":"274338","messageId":"CAPig+cRtKezyzkBpTmc3sgrkk3zDXX3ZnmkW57rf=AVZNaCrvA@mail.gmail.com","threadId":"40776","inReplyTo":"CAOLa=ZQKR4+a-hpL-8xjE-93btWpUt4zAfCGTHBGWwhvLtQoRg@mail.gmail.com","subject":"Re: [PATCH/RFC 08/10] ref-filter: introduce remote_ref_atom_parser()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-13T06:15:02Z","receivedAt":"2015-12-13T06:15:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Dec 13, 2015 at 1:02 AM, Karthik Nayak wrote:\n> On Sun, Dec 13, 2015 at 6:23 AM, Eric Sunshine wrote:\n>> On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak wrote:\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>>\n>> Tangent: These xstrfmt()'d strings are getting leaked, right? Is that\n>> something that we need to worry about (if, for instance, a repository\n>> contains a lot of tracking refs)? Should there be a NEEDSWORK comment\n>> here regarding the issue?\n>\n> This is sort of a problem with most of the values in ref-filter, we dynamically\n> allocate memory and do not free it, since the program exits soon after and\n> we leave it to the Operating System to do the garbage collection.\n\nI'm not worried about memory dynamically allocated for the used_atom[]\narray being leaked (and cleaned up automatically at program exit), but\nrather about memory being leaked for each processed reference, which\nmight become substantial for a project with a lot of references.\n\n> Not sure if we'd want to work on this though.\n\nIt's likely outside the scope of the current patch series anyhow, and\nprobably not something that needs to be tackled right away (or perhaps\never), which is why a NEEDSWORK comment might be appropriate, as a\nreminder that the situation could be improved.\n"},{"id":"274340","messageId":"CAOLa=ZQD1ZhkOGr6Y3ZTwkRC--gMVjwXhpvGpBUGuoHjw9hoJQ@mail.gmail.com","threadId":"40776","inReplyTo":"CAOLa=ZQKR4+a-hpL-8xjE-93btWpUt4zAfCGTHBGWwhvLtQoRg@mail.gmail.com","subject":"Re: [PATCH/RFC 08/10] ref-filter: introduce remote_ref_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-13T08:32:28Z","receivedAt":"2015-12-13T08:32:28Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sun, Dec 13, 2015 at 11:32 AM, Karthik Nayak wrote:\n> On Sun, Dec 13, 2015 at 6:23 AM, Eric Sunshine wrote:\n>> On Wed, Nov 11, 2015 at 2:44 PM, 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>>> Signed-off-by: Karthik Nayak \n>>> ---\n>>> diff --git a/ref-filter.c b/ref-filter.c\n>>> @@ -37,6 +37,11 @@ static struct used_atom {\n>>> union {\n>>> const char *color;\n>>> struct align align;\n>>> + struct {\n>>> + unsigned int shorten : 1,\n>>> + track : 1,\n>>> + trackshort : 1;\n>>> + } remote_ref;\n>>\n>> Are 'shorten', 'track', and 'trackshort' mutually exclusive? If so, a\n>> simple enum would be clearer than bitfields:\n>>\n>> union {\n>> const char *color;\n>> struct align align;\n>> enum { RR_PLAIN, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }\n>> remote_ref;\n>> };\n>>\n>> Or something.\n>>\n>\n> Sure, will do that.\n>\n\nThere's also a slight problem with using enum's with the current implementation.\nThe problem is the enum is set to 0 by default (since we use memset).\nso the first\nvalue is set by default, not something we'd want. So either we stick\nto the structure\nwith unsigned bits or we introduce a pseudo value in the enum. I\nprefer the former.\n\n\n\nOn Sun, Dec 13, 2015 at 11:45 AM, Eric Sunshine wrote:\n> On Sun, Dec 13, 2015 at 1:02 AM, Karthik Nayak wrote:\n>> On Sun, Dec 13, 2015 at 6:23 AM, Eric Sunshine wrote:\n>>> On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak wrote:\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>>>\n>>> Tangent: These xstrfmt()'d strings are getting leaked, right? Is that\n>>> something that we need to worry about (if, for instance, a repository\n>>> contains a lot of tracking refs)? Should there be a NEEDSWORK comment\n>>> here regarding the issue?\n>>\n>> This is sort of a problem with most of the values in ref-filter, we dynamically\n>> allocate memory and do not free it, since the program exits soon after and\n>> we leave it to the Operating System to do the garbage collection.\n>\n> I'm not worried about memory dynamically allocated for the used_atom[]\n> array being leaked (and cleaned up automatically at program exit), but\n> rather about memory being leaked for each processed reference, which\n> might become substantial for a project with a lot of references.\n>\n>> Not sure if we'd want to work on this though.\n>\n> It's likely outside the scope of the current patch series anyhow, and\n> probably not something that needs to be tackled right away (or perhaps\n> ever), which is why a NEEDSWORK comment might be appropriate, as a\n> reminder that the situation could be improved.\n\nYes I got what you're saying. I'm talking about other values which are\ndynamically\nallocated in ref-filter itself.\n\nFor e.g.\n\nWhen we use the deref option\nv->s = xstrfmt(\"%s^{}\", refname);\n\nor the color option\nv->s = xstrdup(color);\n\nSo seems like we need a way to go around all of these.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"274341","messageId":"CAPig+cRwD_W5pQSpiHR8izQ+=6stY+XwpXEJ5HU4gi+MQFaH2Q@mail.gmail.com","threadId":"40776","inReplyTo":"CAOLa=ZQD1ZhkOGr6Y3ZTwkRC--gMVjwXhpvGpBUGuoHjw9hoJQ@mail.gmail.com","subject":"Re: [PATCH/RFC 08/10] ref-filter: introduce remote_ref_atom_parser()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-13T08:45:06Z","receivedAt":"2015-12-13T08:45:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Dec 13, 2015 at 3:32 AM, Karthik Nayak wrote:\n>> On Sun, Dec 13, 2015 at 6:23 AM, Eric Sunshine wrote:\n>>> On Wed, Nov 11, 2015 at 2:44 PM, 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>>>> Signed-off-by: Karthik Nayak \n>>>> ---\n>>>> diff --git a/ref-filter.c b/ref-filter.c\n>>>> @@ -37,6 +37,11 @@ static struct used_atom {\n>>>> union {\n>>>> const char *color;\n>>>> struct align align;\n>>>> + struct {\n>>>> + unsigned int shorten : 1,\n>>>> + track : 1,\n>>>> + trackshort : 1;\n>>>> + } remote_ref;\n>>>\n>>> Are 'shorten', 'track', and 'trackshort' mutually exclusive? If so, a\n>>> simple enum would be clearer than bitfields:\n>>>\n>>> union {\n>>> const char *color;\n>>> struct align align;\n>>> enum { RR_PLAIN, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }\n>>> remote_ref;\n>>> };\n>>>\n>>> Or something.\n>\n> There's also a slight problem with using enum's with the current implementation.\n> The problem is the enum is set to 0 by default (since we use memset).\n> so the first value is set by default, not something we'd want.\n\nI'm afraid I don't see the problem. Doesn't the RR_PLAIN in the\nexample cover this case?\n\n> So either we stick to the structure\n> with unsigned bits or we introduce a pseudo value in the enum. I\n> prefer the former.\n\nIt's not a pseudo-value, but rather just one of the mutually exclusive states.\n"},{"id":"274342","messageId":"CAOLa=ZQ=GL9F8ecOfa3rQ_rXP6_eEpA+ms8zOz5TBbmY9frnaw@mail.gmail.com","threadId":"40776","inReplyTo":"CAPig+cRwD_W5pQSpiHR8izQ+=6stY+XwpXEJ5HU4gi+MQFaH2Q@mail.gmail.com","subject":"Re: [PATCH/RFC 08/10] ref-filter: introduce remote_ref_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-13T08:53:22Z","receivedAt":"2015-12-13T08:53:22Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sun, Dec 13, 2015 at 2:15 PM, Eric Sunshine wrote:\n> On Sun, Dec 13, 2015 at 3:32 AM, Karthik Nayak wrote:\n>>> On Sun, Dec 13, 2015 at 6:23 AM, Eric Sunshine wrote:\n>>>> On Wed, Nov 11, 2015 at 2:44 PM, 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>>>>> Signed-off-by: Karthik Nayak \n>>>>> ---\n>>>>> diff --git a/ref-filter.c b/ref-filter.c\n>>>>> @@ -37,6 +37,11 @@ static struct used_atom {\n>>>>> union {\n>>>>> const char *color;\n>>>>> struct align align;\n>>>>> + struct {\n>>>>> + unsigned int shorten : 1,\n>>>>> + track : 1,\n>>>>> + trackshort : 1;\n>>>>> + } remote_ref;\n>>>>\n>>>> Are 'shorten', 'track', and 'trackshort' mutually exclusive? If so, a\n>>>> simple enum would be clearer than bitfields:\n>>>>\n>>>> union {\n>>>> const char *color;\n>>>> struct align align;\n>>>> enum { RR_PLAIN, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }\n>>>> remote_ref;\n>>>> };\n>>>>\n>>>> Or something.\n>>\n>> There's also a slight problem with using enum's with the current implementation.\n>> The problem is the enum is set to 0 by default (since we use memset).\n>> so the first value is set by default, not something we'd want.\n>\n> I'm afraid I don't see the problem. Doesn't the RR_PLAIN in the\n> example cover this case?\n>\n>> So either we stick to the structure\n>> with unsigned bits or we introduce a pseudo value in the enum. I\n>> prefer the former.\n>\n> It's not a pseudo-value, but rather just one of the mutually exclusive states.\n\nThis example is actually fine, the next one \"%(contents)\" is more of\nthe problem,\nthe check is done in grab_sub_body_contents() where previously \"contents.all\"\nwould be enough to check if we need to add contents value. Now the first enum\nvalue is selected. Maybe have a \"NOT_VALID\" field in the enum.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"274343","messageId":"CAOLa=ZSEGf7TAKNkYCogw7nz9UMUxEZ7Vg2vGXHqFP2s=tnWcg@mail.gmail.com","threadId":"40776","inReplyTo":"CAPig+cQtoUXuOZjGjev8MGYUMyjd+n_=o+jOVXkhPReSkWgxmw@mail.gmail.com","subject":"Re: [PATCH/RFC 00/10] ref-filter: use parsing functions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-13T09:31:29Z","receivedAt":"2015-12-13T09:31:29Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sun, Dec 13, 2015 at 11:10 AM, Eric Sunshine wrote:\n> On Fri, Dec 11, 2015 at 5:49 PM, Junio C Hamano wrote:\n>> Karthik Nayak writes:\n>>> ref-filter: introduce a parsing function for each atom in valid_atom\n>>> ref-filter: introduce struct used_atom\n>>> ref-fitler: bump match_atom() name to the top\n>>> ref-filter: skip deref specifier in match_atom_name()\n>>> ref-filter: introduce color_atom_parser()\n>>> strbuf: introduce strbuf_split_str_without_term()\n>>> ref-filter: introduce align_atom_parser()\n>>> ref-filter: introduce remote_ref_atom_parser()\n>>> ref-filter: introduce contents_atom_parser()\n>>> ref-filter: introduce objectname_atom_parser()\n>>\n>> It seems that this series had seen quite a good inputs, mostly from\n>> Eric. I finished reading it over and I didn't find anything more to\n>> add. The patches are mostly good and would be ready once these\n>> points raised during the review are addressed, I think\n>\n> I'm still a bit fuzzy about what this series is trying to achieve. It\n> feels like it wants to avoid doing repeated processing of unchanging\n> bits of %(foo:bar) atoms for each ref processed, but it only partly\n> achieves that goal. For instance, there are still an awful lot of\n> strcmp()s and starts_with()s in that inner loop, and even the\n> unchanging %(color:) argument gets re-evaulated repeatedly, which is\n> probably quite expensive.\n>\n\nYes, you're right. It started off as per Junio's suggestion which you\ncan find here: http://thread.gmane.org/gmane.comp.version-control.git/279254\n\n> If the intention is to rid that inner loop of much of the expensive\n> processing, then wouldn't we want to introduce an enum of valid atoms\n> which is to be a member of 'struct used_atom', and have\n> populate_value() switch on the enum value rather than doing all the\n> expensive strcmp()s and starts_with()?\n>\n> enum atom_type {\n> AT_REFNAME,\n> AT_OBJECTTYPE,\n> ...\n> AT_COLOR,\n> AT_ALIGN\n> };\n>\n> static struct used_atom {\n> enum atom_type atom;\n> cmp_type cmp;\n> union {\n> char *color; /* parsed color */\n> struct align align;\n> enum { ... } remote_ref;\n> struct {\n> enum { ... } portion;\n> unsigned int nlines;\n> } contents;\n> int short_objname;\n> } u;\n> } *used_atom;\n>\n> In fact, the 'cmp_type cmp' field can be dropped altogether since it\n> can just as easily be looked up when needed by keeping 'enum\n> atom_type' and valid_atoms[] in-sync and indexing into valid_atoms[]\n> by atom_type:\n>\n> struct used_atom *a = ...;\n> cmp_type cmp = valid_atoms[a->atom].cmp_type;\n>\n> As a bonus, an 'enum atom_type' would resolve the problem of how\n> starts_with() is abused in populate_value() for certain cases\n> (assuming I'm understanding the logic), such as how matching of\n> \"color\" could incorrectly match some yet-to-be-added atom named\n> \"colorize\".\n\nThis actually makes sense to me, especially how we can eliminate most of\nthe starts_with() in populate_value(). Well then I guess I'll work on this and\nsee what I can come up with, Thanks for the review :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"274355","messageId":"CAOLa=ZQhFXUYvYjJjWDzN6z+HDiXrhJqRbd3CWKTGmDtXn3oJw@mail.gmail.com","threadId":"40776","inReplyTo":"CAPig+cSC8fP1KGM1wCXzcfj2QHgvoUUjfBncRH0jKLv1EnYKCg@mail.gmail.com","subject":"Re: [PATCH/RFC 05/10] ref-filter: introduce color_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-13T18:46:41Z","receivedAt":"2015-12-13T18:46:41Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sun, Dec 13, 2015 at 11:35 AM, Eric Sunshine wrote:\n> On Thu, Dec 3, 2015 at 8:35 AM, Karthik Nayak wrote:\n>> On Wed, Dec 2, 2015 at 4:57 AM, Eric Sunshine wrote:\n>>> On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak wrote:\n>>>> @@ -833,11 +846,10 @@ static void populate_value(struct ref_array_item *ref)\n>>>> refname = branch_get_push(branch, NULL);\n>>>> if (!refname)\n>>>> continue;\n>>>> - } else if (match_atom_name(name, \"color\", &valp)) {\n>>>> + } else if (starts_with(name, \"color\")) {\n>>>\n>>> Hmm, so this will also match \"colorize\". Is that desirable?\n>>\n>> Well the error checking is done when we parse the atom in color_atom_parser()\n>> so here we don't need to worry about something like this.\n>\n> I'm not sure that I understand your response. Let's say that, in the\n> future, someone adds a new atom named \"colorize\" (which may or may not\n> have its own parser in the valid_atom[] table). color_atom_parser()\n> will never see that atom, thus error checking in color_atom_parser()\n> is not relevant to this case. What is relevant is that the original\n> code:\n>\n> } if (match_atom_name(name, \"color\", &valp)) {\n>\n> only matched %(color) or %(color:whatever). It did not match\n> %(colorize). However, the new code:\n>\n> } else if (starts_with(name, \"color\")) {\n>\n> is far looser and will match %(colorize) and %(color) and\n> %(color:whatever) and %(coloranything), which is potentially\n> undesirable. It's true that the person adding %(colorize) could be\n> careful and ensure that the if/else chain checks %(colorize) first:\n>\n> } else if (!strcmp(name, \"colorize\") {\n> ...\n> } else if (starts_with(name, \"color\")) {\n> ...\n> } else ...\n>\n> but that places a certain extra burden on that person. Alternately,\n> you can tighten the matching so that it is as strict as the original:\n>\n> } else if (!strcmp(name, \"color\") || starts_with(name, \"color:\")) {\n> ...\n> } else ...\n>\n> Or perhaps upgrade match_atom_name() to make the 'val' argument\n> optional, in which case you might be able to do something like this:\n>\n> } else if (match_atom_name(name, \"color\", NULL) {\n>\n> (However, if you introduce an 'enum atom_type' as suggested in my\n> response to the cover letter, then this problem goes away because\n> you'd be switching on the enum value, which is determinate, rather\n> than on a partial string, which may be ambiguous, as illustrated.)\n\nAh, Thanks for putting it out. I understand, I'll work on the enum atom_type\nnow. that should take care of this :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"274360","messageId":"CAOLa=ZTdyWe421m=G0ktnh4McH-xE86ek46uu5nbijSCFifcuw@mail.gmail.com","threadId":"40776","inReplyTo":"CAPig+cTbT60cWk+=6EARte68mmQ2xkS5xFgsgh0FbpoueFcBbg@mail.gmail.com","subject":"Re: [PATCH/RFC 09/10] ref-filter: introduce contents_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-13T19:33:48Z","receivedAt":"2015-12-13T19:33:48Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sun, Dec 13, 2015 at 8:40 AM, Eric Sunshine wrote:\n> On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak wrote:\n>> Introduce contents_atom_parser() which will parse the '%(contents)'\n>> atom and store information into the 'used_atom' structure based on the\n>> modifiers used along with the atom.\n>>\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> index 802629b..117bbbb 100644\n>> --- a/ref-filter.c\n>> +++ b/ref-filter.c\n>> @@ -42,6 +42,14 @@ static struct used_atom {\n>> track : 1,\n>> trackshort : 1;\n>> } remote_ref;\n>> + struct {\n>> + unsigned int subject : 1,\n>> + body : 1,\n>> + signature : 1,\n>> + all : 1,\n>> + lines : 1,\n>> + no_lines;\n>> + } contents;\n>\n> Same question as patch 8/10: With the exception of non-bitfield\n> 'no_lines', are these 'contents' flags mutually exclusive? If so, an\n> enum would be a more natural representation than bitfields.\n>\n\nLike I said, an enum would cause problems here.\nIf you see the code flow when we check for contents we currently (as\nper this patch)\nuse the 'all' bit to denote usage of contents, but when using an enum we would\nset the enum to the first value when using memset.\n\nBut this should go away with the 'enum atom_type' implementation which\nyou suggested.\n\n>> } u;\n>> } *used_atom;\n>> static int used_atom_cnt, need_tagged, need_symref;\n>> @@ -92,6 +100,29 @@ void remote_ref_atom_parser(struct used_atom *atom)\n>> +void contents_atom_parser(struct used_atom *atom)\n>> +{\n>> + const char * buf;\n>> +\n>> + if (match_atom_name(atom->str, \"contents\", &buf))\n>> + atom->u.contents.all = 1;\n>> +\n>> + if (!buf)\n>> + return;\n>\n> Hmm, I'd have placed the blank line after the 'if (!buf) return;'\n> rather than before it.\n>\n\nWill change that.\n\n>> + if (!strcmp(buf, \"body\"))\n>> + atom->u.contents.body = 1;\n>> + else if (!strcmp(buf, \"signature\"))\n>> + atom->u.contents.signature = 1;\n>> + else if (!strcmp(buf, \"subject\"))\n>> + atom->u.contents.subject = 1;\n>> + else if (skip_prefix(buf, \"lines=\", &buf)) {\n>> + atom->u.contents.lines = 1;\n>> + if (strtoul_ui(buf, 10, &atom->u.contents.no_lines))\n>> + die(_(\"positive value expected contents:lines=%s\"), buf);\n>> + } else\n>> + die(_(\"improper format entered contents:%s\"), buf);\n>\n> Perhaps a more grammatically-friendly error message?\n>\n\nWill do.\n\n>> +}\n>> +\n>> static align_type get_align_position(const char *type)\n>> {\n>> if (!strcmp(type, \"right\"))\n>> @@ -761,20 +786,16 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj\n>> unsigned long sublen = 0, bodylen = 0, nonsiglen = 0, siglen = 0;\n>>\n>> for (i = 0; i < used_atom_cnt; i++) {\n>> - const char *name = used_atom[i].str;\n>> + struct used_atom *atom = &used_atom[i];\n>> + const char *name = atom->str;\n>> struct atom_value *v = &val[i];\n>> - const char *valp = NULL;\n>> if (!!deref != (*name == '*'))\n>> continue;\n>> if (deref)\n>> name++;\n>> if (strcmp(name, \"subject\") &&\n>> strcmp(name, \"body\") &&\n>> - strcmp(name, \"contents\") &&\n>> - strcmp(name, \"contents:subject\") &&\n>> - strcmp(name, \"contents:body\") &&\n>> - strcmp(name, \"contents:signature\") &&\n>> - !starts_with(name, \"contents:lines=\"))\n>> + !atom->u.contents.all)\n>> continue;\n>> if (!subpos)\n>> find_subpos(buf, sz,\n>> @@ -784,26 +805,23 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj\n>>\n>> if (!strcmp(name, \"subject\"))\n>> v->s = copy_subject(subpos, sublen);\n>> - else if (!strcmp(name, \"contents:subject\"))\n>> + else if (atom->u.contents.subject)\n>> v->s = copy_subject(subpos, sublen);\n>\n> With the disclaimer that I haven't fully digested the existing logic,\n> is there a reason that you don't also preprocess bare \"subject\" as you\n> preprocess \"contents:subject\"? Isn't \"subject\" just historic aliases\n> for \"contents:subject\"?\n>\n> A similar observation may be made about \"body\" and \"contents:body\",\n> although I see they mean slightly different things (for, I suppose,\n> historical reasons).\n>\n\nActually I missed that out, now that you mention it, i'll see how I\ncan come around this.\n\n>> else if (!strcmp(name, \"body\"))\n>> v->s = xmemdupz(bodypos, bodylen);\n>> - else if (!strcmp(name, \"contents:body\"))\n>> + else if (atom->u.contents.body)\n>> v->s = xmemdupz(bodypos, nonsiglen);\n>> - else if (!strcmp(name, \"contents:signature\"))\n>> + else if (atom->u.contents.signature)\n>> v->s = xmemdupz(sigpos, siglen);\n>> - else if (!strcmp(name, \"contents\"))\n>> - v->s = xstrdup(subpos);\n>> - else if (skip_prefix(name, \"contents:lines=\", &valp)) {\n>> + else if (atom->u.contents.lines) {\n>> struct strbuf s = STRBUF_INIT;\n>> const char *contents_end = bodylen + bodypos - siglen;\n>>\n>> - if (strtoul_ui(valp, 10, &v->u.contents.lines))\n>> - die(_(\"positive value expected contents:lines=%s\"), valp);\n>> /* Size is the length of the message after removing the signature */\n>> - append_lines(&s, subpos, contents_end - subpos, v->u.contents.lines);\n>> + append_lines(&s, subpos, contents_end - subpos, atom->u.contents.no_lines);\n>> v->s = strbuf_detach(&s, NULL);\n>> - }\n>> + } else /* For %(contents) without modifiers */\n>\n> Too many blanks following '/*'; downcase 'for' or drop it altogether:\n>\n> /* bare %(contents) */\n>\n\nWill do thanks.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"274361","messageId":"CAOLa=ZQ4wpeerP2x5dKy51hDosqWQ7wgasP-8hbbOU_ydm3N8A@mail.gmail.com","threadId":"40776","inReplyTo":"CAPig+cTVqTc1S8gduLq3qiyuzJNb=LNQiXijEHwqPPzi1YU8UQ@mail.gmail.com","subject":"Re: [PATCH/RFC 09/10] ref-filter: introduce contents_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-13T19:36:09Z","receivedAt":"2015-12-13T19:36:09Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sun, Dec 13, 2015 at 10:11 AM, Eric Sunshine wrote:\n> On Sat, Dec 12, 2015 at 10:10 PM, Eric Sunshine wrote:\n>> On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak wrote:\n>>> +void contents_atom_parser(struct used_atom *atom)\n>>> +{\n>>> + const char * buf;\n>>> +\n>>> + if (match_atom_name(atom->str, \"contents\", &buf))\n>>> + atom->u.contents.all = 1;\n>>> +\n>>> + if (!buf)\n>>> + return;\n>\n> Also, isn't this logic somewhat bogus? u.contents.all is set to 1 for\n> both bare %(contents) and decorated %(contents:whatever). Then, below,\n> you also set one of .body, .signature, .subject, or .lines if a\n> decoration is specified. So, now you have both .all and one of the\n> other attributes set to 1, which is rather nonsensical (if I\n> understand correctly).\n>\n\nThe problem is its not mutually exclusive here, the 'all' was supposed to\nact as a way of checking if its a contents atom, since populate_value() doesn't\nreally check that again.\n\nSo if any of the others were selected we would implement %(contents:)\nelse the bare %(contents:) would be selected.\n\n> If you change this to an enum as suggested in my previous email, then\n> the problem goes away.\n>\n\nYup. Will do. Thanks\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"274362","messageId":"CAOLa=ZRivWR_0qMVqRU51PrX4OhSdBYdpwQm6Ci8H2wvAzPypw@mail.gmail.com","threadId":"40776","inReplyTo":"CAPig+cTduYxSWh5ghmv3vM04saAqrn9TUpBZ0J=TuDOgr05itQ@mail.gmail.com","subject":"Re: [PATCH/RFC 10/10] ref-filter: introduce objectname_atom_parser()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-13T19:40:54Z","receivedAt":"2015-12-13T19:40:54Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sun, Dec 13, 2015 at 10:19 AM, Eric Sunshine wrote:\n> On Wed, Nov 25, 2015 at 8:44 AM, Karthik Nayak wrote:\n>> Introduce objectname_atom_parser() which will parse the\n>> '%(objectname)' atom and store information into the 'used_atom'\n>> structure based on the modifiers used along with the atom.\n>>\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> @@ -50,6 +50,10 @@ static struct used_atom {\n>> lines : 1,\n>> no_lines;\n>> } contents;\n>> + struct {\n>> + unsigned int shorten : 1,\n>> + full : 1;\n>> + } objectname;\n>\n> Same comment as in my patch 8 and 9 reviews: If 'shorten' and 'full'\n> are mutually exclusive, then an enum would be clearer. In fact, if\n> there are only these two states (full and short), then this could be a\n> simple boolean named 'shorten'.\n>\n>> } u;\n>> } *used_atom;\n>> static int used_atom_cnt, need_tagged, need_symref;\n>> @@ -123,6 +127,21 @@ void contents_atom_parser(struct used_atom *atom)\n>> +void objectname_atom_parser(struct used_atom *atom)\n>> +{\n>> + const char * buf;\n>> +\n>> + if (match_atom_name(atom->str, \"objectname\", &buf))\n>> + atom->u.objectname.full = 1;\n>\n> Same comment about bogus logic as in patch 9 review: u.objectname.full\n> and u.objectname.shorten are both set to 1 for %(objectname:short).\n>\n\nI guess I responded to the same issue, will work on it.\n\n>> +\n>> + if (!buf)\n>> + return;\n>\n> Same comment about misplaced blank line: Put the blank line after the\n> conditional rather than before or drop it altogether.\n>\n\nWill change.\n\n>> + if (!strcmp(buf, \"short\"))\n>> + atom->u.objectname.shorten = 1;\n>> + else\n>> + die(_(\"improper format entered objectname:%s\"), buf);\n>\n> Maybe just \"unrecognized objectname:%s\" or something?\n>\n\ndie(_(\"unrecognized %%(objectname) argument: %s\"), buf);\nto keep things consistent\n\n>> +}\n>> +\n>> @@ -463,15 +482,16 @@ static void *get_obj(const unsigned char *sha1, struct object **obj, unsigned lo\n>> }\n>>\n>> static int grab_objectname(const char *name, const unsigned char *sha1,\n>> - struct atom_value *v)\n>> + struct atom_value *v, struct used_atom *atom)\n>> {\n>> - if (!strcmp(name, \"objectname\")) {\n>> - v->s = xstrdup(sha1_to_hex(sha1));\n>> - return 1;\n>> - }\n>> - if (!strcmp(name, \"objectname:short\")) {\n>> - v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));\n>> - return 1;\n>> + if (starts_with(name, \"objectname\")) {\n>> + if (atom->u.objectname.shorten) {\n>> + v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));\n>> + return 1;\n>> + } else if (atom->u.objectname.full) {\n>> + v->s = xstrdup(sha1_to_hex(sha1));\n>> + return 1;\n>> + }\n>> }\n>> return 0;\n>> }\n>> @@ -495,7 +515,7 @@ static void grab_common_values(struct atom_value *val, int deref, struct object\n>> v->s = xstrfmt(\"%lu\", sz);\n>> }\n>> else if (deref)\n>> - grab_objectname(name, obj->sha1, v);\n>> + grab_objectname(name, obj->sha1, v, &used_atom[i]);\n>> }\n>> }\n>>\n>> @@ -1004,7 +1024,7 @@ static void populate_value(struct ref_array_item *ref)\n>> v->s = xstrdup(buf + 1);\n>> }\n>> continue;\n>> - } else if (!deref && grab_objectname(name, ref->objectname, v)) {\n>> + } else if (!deref && grab_objectname(name, ref->objectname, v, atom)) {\n>> continue;\n>> } else if (!strcmp(name, \"HEAD\")) {\n>> const char *head;\n>> --\n>> 2.6.2\n\n\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"274367","messageId":"CAPig+cSTpLFgeVJuPyFVuRGkwBH_axW1Vik9+6cY4g2Ajeu4iw@mail.gmail.com","threadId":"40776","inReplyTo":"CAOLa=ZSEGf7TAKNkYCogw7nz9UMUxEZ7Vg2vGXHqFP2s=tnWcg@mail.gmail.com","subject":"Re: [PATCH/RFC 00/10] ref-filter: use parsing functions","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-13T21:55:03Z","receivedAt":"2015-12-13T21:55:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Dec 13, 2015 at 4:31 AM, Karthik Nayak wrote:\n> On Sun, Dec 13, 2015 at 11:10 AM, Eric Sunshine wrote:\n>> If the intention is to rid that inner loop of much of the expensive\n>> processing, then wouldn't we want to introduce an enum of valid atoms\n>> which is to be a member of 'struct used_atom', and have\n>> populate_value() switch on the enum value rather than doing all the\n>> expensive strcmp()s and starts_with()?\n>>\n>> enum atom_type {\n>> AT_REFNAME,\n>> AT_OBJECTTYPE,\n>> ...\n>> AT_COLOR,\n>> AT_ALIGN\n>> };\n>>\n>> static struct used_atom {\n>> enum atom_type atom;\n>> cmp_type cmp;\n>> union {\n>> char *color; /* parsed color */\n>> struct align align;\n>> enum { ... } remote_ref;\n>> struct {\n>> enum { ... } portion;\n>> unsigned int nlines;\n>> } contents;\n>> int short_objname;\n>> } u;\n>> } *used_atom;\n>>\n>> In fact, the 'cmp_type cmp' field can be dropped altogether since it\n>> can just as easily be looked up when needed by keeping 'enum\n>> atom_type' and valid_atoms[] in-sync and indexing into valid_atoms[]\n>> by atom_type:\n>>\n>> struct used_atom *a = ...;\n>> cmp_type cmp = valid_atoms[a->atom].cmp_type;\n>>\n>> As a bonus, an 'enum atom_type' would resolve the problem of how\n>> starts_with() is abused in populate_value() for certain cases\n>> (assuming I'm understanding the logic), such as how matching of\n>> \"color\" could incorrectly match some yet-to-be-added atom named\n>> \"colorize\".\n>\n> This actually makes sense to me, especially how we can eliminate most of\n> the starts_with() in populate_value(). Well then I guess I'll work on this and\n> see what I can come up with, Thanks for the review :)\n\nThe 'enum atom_type' approach doesn't necessarily need to be part of\nthis patch series; it may make sense to layer it atop the current\nseries, which is already long enough to make review onerous. Patches\nin the current series are things you would want to do anyhow for the\n'enum atom_type' approach, and there isn't really anything in the\ncurrent series which should present a roadblock for layering 'enum\natom_type' atop. Plus, by layering 'enum atom_type' atop, you get a\nchance to polish the current series (based upon review comments) and\nlet it settle down, which should make working on 'enum atom_type'\neasier.\n"},{"id":"274394","messageId":"xmqqr3iokfxy.fsf@gitster.mtv.corp.google.com","threadId":"40776","inReplyTo":"CAPig+cQtoUXuOZjGjev8MGYUMyjd+n_=o+jOVXkhPReSkWgxmw@mail.gmail.com","subject":"Re: [PATCH/RFC 00/10] ref-filter: use parsing functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-14T19:06:49Z","receivedAt":"2015-12-14T19:06:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Fri, Dec 11, 2015 at 5:49 PM, Junio C Hamano wrote:\n>> Karthik Nayak writes:\n>>> ref-filter: introduce a parsing function for each atom in valid_atom\n>>> ref-filter: introduce struct used_atom\n>>> ref-fitler: bump match_atom() name to the top\n>>> ref-filter: skip deref specifier in match_atom_name()\n>>> ref-filter: introduce color_atom_parser()\n>>> strbuf: introduce strbuf_split_str_without_term()\n>>> ref-filter: introduce align_atom_parser()\n>>> ref-filter: introduce remote_ref_atom_parser()\n>>> ref-filter: introduce contents_atom_parser()\n>>> ref-filter: introduce objectname_atom_parser()\n>>\n>> It seems that this series had seen quite a good inputs, mostly from\n>> Eric. I finished reading it over and I didn't find anything more to\n>> add. The patches are mostly good and would be ready once these\n>> points raised during the review are addressed, I think\n>\n> I'm still a bit fuzzy about what this series is trying to achieve. It\n> feels like it wants to avoid doing repeated processing of unchanging\n> bits of %(foo:bar) atoms for each ref processed, but it only partly\n> achieves that goal.\n\nThat's very true.\n\nIt seems you two already have hashed it out in the downthread, and I\nthink that is in line with an earlier suggestion by Matthieu to\nfully pre-parse in the earlier thread, which was made in response to\n(and is much better than) my \"let's start with a half-way solution\"\nin $gmane/279254.\n\nThanks.\n\n> strcmp()s and starts_with()s in that inner loop, and even the\n> unchanging %(color:) argument gets re-evaulated repeatedly, which is\n> probably quite expensive.\n>\n> If the intention is to rid that inner loop of much of the expensive\n> processing, then wouldn't we want to introduce an enum of valid atoms\n> which is to be a member of 'struct used_atom', and have\n> populate_value() switch on the enum value rather than doing all the\n> expensive strcmp()s and starts_with()?\n>\n> enum atom_type {\n> AT_REFNAME,\n> AT_OBJECTTYPE,\n> ...\n> AT_COLOR,\n> AT_ALIGN\n> };\n>\n> static struct used_atom {\n> enum atom_type atom;\n> cmp_type cmp;\n> union {\n> char *color; /* parsed color */\n> struct align align;\n> enum { ... } remote_ref;\n> struct {\n> enum { ... } portion;\n> unsigned int nlines;\n> } contents;\n> int short_objname;\n> } u;\n> } *used_atom;\n>\n> In fact, the 'cmp_type cmp' field can be dropped altogether since it\n> can just as easily be looked up when needed by keeping 'enum\n> atom_type' and valid_atoms[] in-sync and indexing into valid_atoms[]\n> by atom_type:\n>\n> struct used_atom *a = ...;\n> cmp_type cmp = valid_atoms[a->atom].cmp_type;\n>\n> As a bonus, an 'enum atom_type' would resolve the problem of how\n> starts_with() is abused in populate_value() for certain cases\n> (assuming I'm understanding the logic), such as how matching of\n> \"color\" could incorrectly match some yet-to-be-added atom named\n> \"colorize\".\n"},{"id":"274575","messageId":"CAOLa=ZS-iBYds4uqy=YQD3fH9332HufrE34twcftPkjZh4gDmw@mail.gmail.com","threadId":"40776","inReplyTo":"CAPig+cSTpLFgeVJuPyFVuRGkwBH_axW1Vik9+6cY4g2Ajeu4iw@mail.gmail.com","subject":"Re: [PATCH/RFC 00/10] ref-filter: use parsing functions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-12-16T14:45:19Z","receivedAt":"2015-12-16T14:45:19Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Mon, Dec 14, 2015 at 3:25 AM, Eric Sunshine wrote:\n> On Sun, Dec 13, 2015 at 4:31 AM, Karthik Nayak wrote:\n>> On Sun, Dec 13, 2015 at 11:10 AM, Eric Sunshine wrote:\n>>> If the intention is to rid that inner loop of much of the expensive\n>>> processing, then wouldn't we want to introduce an enum of valid atoms\n>>> which is to be a member of 'struct used_atom', and have\n>>> populate_value() switch on the enum value rather than doing all the\n>>> expensive strcmp()s and starts_with()?\n>>>\n>>> enum atom_type {\n>>> AT_REFNAME,\n>>> AT_OBJECTTYPE,\n>>> ...\n>>> AT_COLOR,\n>>> AT_ALIGN\n>>> };\n>>>\n>>> static struct used_atom {\n>>> enum atom_type atom;\n>>> cmp_type cmp;\n>>> union {\n>>> char *color; /* parsed color */\n>>> struct align align;\n>>> enum { ... } remote_ref;\n>>> struct {\n>>> enum { ... } portion;\n>>> unsigned int nlines;\n>>> } contents;\n>>> int short_objname;\n>>> } u;\n>>> } *used_atom;\n>>>\n>>> In fact, the 'cmp_type cmp' field can be dropped altogether since it\n>>> can just as easily be looked up when needed by keeping 'enum\n>>> atom_type' and valid_atoms[] in-sync and indexing into valid_atoms[]\n>>> by atom_type:\n>>>\n>>> struct used_atom *a = ...;\n>>> cmp_type cmp = valid_atoms[a->atom].cmp_type;\n>>>\n>>> As a bonus, an 'enum atom_type' would resolve the problem of how\n>>> starts_with() is abused in populate_value() for certain cases\n>>> (assuming I'm understanding the logic), such as how matching of\n>>> \"color\" could incorrectly match some yet-to-be-added atom named\n>>> \"colorize\".\n>>\n>> This actually makes sense to me, especially how we can eliminate most of\n>> the starts_with() in populate_value(). Well then I guess I'll work on this and\n>> see what I can come up with, Thanks for the review :)\n>\n> The 'enum atom_type' approach doesn't necessarily need to be part of\n> this patch series; it may make sense to layer it atop the current\n> series, which is already long enough to make review onerous. Patches\n> in the current series are things you would want to do anyhow for the\n> 'enum atom_type' approach, and there isn't really anything in the\n> current series which should present a roadblock for layering 'enum\n> atom_type' atop. Plus, by layering 'enum atom_type' atop, you get a\n> chance to polish the current series (based upon review comments) and\n> let it settle down, which should make working on 'enum atom_type'\n> easier.\n\nYa could do that, I have the series ready then, I was gonna work on the\n'enum atom_type' but I'll work on top of this, like you suggested.\n\nThanks.\n\n-- \nRegards,\nKarthik Nayak\n"}]}