[PATCH v4 00/12] ref-filter: use parsing functions

[PATCH v4 00/12] ref-filter: use parsing functions

[Karthik Nayak]

This series cleans up populate_value() in ref-filter, by moving out
the parsing part of atoms to separate parsing functions. This ensures
that parsing is only done once and also improves the modularity of the
code.

v1: http://thread.gmane.org/gmane.comp.version-control.git/281180
v2: http://thread.gmane.org/gmane.comp.version-control.git/282563
v3: http://thread.gmane.org/gmane.comp.version-control.git/283350

Changes:
* The parsing functions now take the arguments of the atom as
function parameteres, instead of parsing it inside the fucntion.
* Rebased on top of pu:jk/list-tag-2.7-regression
* In strbuf use a copylen variable rather than using multiplication
to perform a logical operation.
* Code movement for easier review and general improvement.
* Use COLOR_MAXLEN as the maximum size for the color variable.
* Small code changes.
* Documentation changes.
* Fixed incorrect style of test (t6302).

Karthik Nayak (12):
  strbuf: introduce strbuf_split_str_omit_term()
  ref-filter: use strbuf_split_str_omit_term()
  ref-filter: bump 'used_atom' and related code to the top
  ref-filter: introduce struct used_atom
  ref-filter: introduce parsing functions for each valid atom
  ref-filter: introduce color_atom_parser()
  ref-filter: introduce parse_align_position()
  ref-filter: introduce align_atom_parser()
  ref-filter: align: introduce long-form syntax
  ref-filter: introduce remote_ref_atom_parser()
  ref-filter: introduce contents_atom_parser()
  ref-filter: introduce objectname_atom_parser()

Interdiff:

diff --git a/ref-filter.c b/ref-filter.c
index 7a634e6..d48e2a3 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -35,7 +35,7 @@ static struct used_atom {
 	const char *name;
 	cmp_type type;
 	union {
-		char *color;
+		char color[COLOR_MAXLEN];
 		struct align align;
 		enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }
 			remote_ref;
@@ -49,99 +49,68 @@ static struct used_atom {
 static int used_atom_cnt, need_tagged, need_symref;
 static int need_color_reset_at_eol;
 
-static int match_atom_name(const char *name, const char *atom_name, const char **val)
+static void color_atom_parser(struct used_atom *atom, const char *color_value)
 {
-	const char *body;
-
-	/* skip the deref specifier */
-	if (name[0] == '*')
-		name++;
-
-	if (!skip_prefix(name, atom_name, &body))
-		return 0; /* doesn't even begin with "atom_name" */
-	if (!body[0]) {
-		*val = NULL; /* %(atom_name) and no customization */
-		return 1;
-	}
-	if (body[0] != ':')
-		return 0; /* "atom_namefoo" is not "atom_name" or "atom_name:..." */
-	*val = body + 1; /* "atom_name:val" */
-	return 1;
-}
-
-static void color_atom_parser(struct used_atom *atom)
-{
-	if (!match_atom_name(atom->name, "color", (const char **)&atom->u.color))
-		die("BUG: parsing non-'color'");
-	if (!atom->u.color)
+	if (!color_value)
 		die(_("expected format: %%(color:<color>)"));
-	/* atom->u.color points to part of atom->name */
-	atom->u.color = xstrdup(atom->u.color);
-	if (color_parse(atom->u.color, atom->u.color) < 0)
+	if (color_parse(color_value, atom->u.color) < 0)
 		die(_("invalid color value: %s"), atom->u.color);
 }
 
-static void remote_ref_atom_parser(struct used_atom *atom)
+static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
 {
-	const char *buf;
-
-	buf = strchr(atom->name, ':');
-	if (!buf) {
+	if (!arg) {
 		atom->u.remote_ref = RR_NORMAL;
-		return;
-	}
-	buf++;
-	if (!strcmp(buf, "short"))
+	} else if (!strcmp(arg, "short"))
 		atom->u.remote_ref = RR_SHORTEN;
-	else if (!strcmp(buf, "track"))
+	else if (!strcmp(arg, "track"))
 		atom->u.remote_ref = RR_TRACK;
-	else if (!strcmp(buf, "trackshort"))
+	else if (!strcmp(arg, "trackshort"))
 		atom->u.remote_ref = RR_TRACKSHORT;
 	else
 		die(_("unrecognized format: %%(%s)"), atom->name);
 }
 
-static void contents_atom_parser(struct used_atom *atom)
+static void body_atom_parser(struct used_atom *atom, const char *arg)
 {
-	const char * buf;
+	if (arg)
+		die("%%(body) atom does not take arguments");
+	atom->u.contents.option = C_BODY_DEP;
+}
 
-	if (match_atom_name(atom->name, "subject", &buf)) {
-		atom->u.contents.option = C_SUB;
-		return;
-	} else if (match_atom_name(atom->name, "body", &buf)) {
-		atom->u.contents.option = C_BODY_DEP;
-		return;
-	} if (!match_atom_name(atom->name, "contents", &buf))
-		  die("BUG: parsing non-'contents'");
+static void subject_atom_parser(struct used_atom *atom, const char *arg)
+{
+	if (arg)
+		die("%%(subject) atom does not take arguments");
+	atom->u.contents.option = C_SUB;
+}
 
-	if (!buf)
+static void contents_atom_parser(struct used_atom *atom, const char *arg)
+{
+	if (!arg)
 		atom->u.contents.option = C_BARE;
-	else if (!strcmp(buf, "body"))
+	else if (!strcmp(arg, "body"))
 		atom->u.contents.option = C_BODY;
-	else if (!strcmp(buf, "signature"))
+	else if (!strcmp(arg, "signature"))
 		atom->u.contents.option = C_SIG;
-	else if (!strcmp(buf, "subject"))
+	else if (!strcmp(arg, "subject"))
 		atom->u.contents.option = C_SUB;
-	else if (skip_prefix(buf, "lines=", &buf)) {
+	else if (skip_prefix(arg, "lines=", &arg)) {
 		atom->u.contents.option = C_LINES;
-		if (strtoul_ui(buf, 10, &atom->u.contents.nlines))
-			die(_("positive value expected contents:lines=%s"), buf);
+		if (strtoul_ui(arg, 10, &atom->u.contents.nlines))
+			die(_("positive value expected contents:lines=%s"), arg);
 	} else
-		die(_("unrecognized %%(contents) argument: %s"), buf);
+		die(_("unrecognized %%(contents) argument: %s"), arg);
 }
 
-static void objectname_atom_parser(struct used_atom *atom)
+static void objectname_atom_parser(struct used_atom *atom, const char *arg)
 {
-	const char * buf;
-
-	if (!match_atom_name(atom->name, "objectname", &buf))
-		die("BUG: parsing non-'objectname'");
-	if (!buf)
+	if (!arg)
 		atom->u.objectname = O_FULL;
-	else if (!strcmp(buf, "short"))
+	else if (!strcmp(arg, "short"))
 		atom->u.objectname = O_SHORT;
 	else
-		die(_("unrecognized %%(objectname) argument: %s"), buf);
+		die(_("unrecognized %%(objectname) argument: %s"), arg);
 }
 
 static align_type parse_align_position(const char *s)
@@ -155,39 +124,36 @@ static align_type parse_align_position(const char *s)
 	return -1;
 }
 
-static void align_atom_parser(struct used_atom *atom)
+static void align_atom_parser(struct used_atom *atom, const char *arg)
 {
 	struct align *align = &atom->u.align;
-	const char *buf = NULL;
 	struct strbuf **s, **to_free;
 	unsigned int width = ~0U;
 
-	if (!match_atom_name(atom->name, "align", &buf))
-		die("BUG: parsing non-'align'");
-	if (!buf)
+	if (!arg)
 		die(_("expected format: %%(align:<width>,<position>)"));
-	s = to_free = strbuf_split_str_omit_term(buf, ',', 0);
+	s = to_free = strbuf_split_str_omit_term(arg, ',', 0);
 
 	align->position = ALIGN_LEFT;
 
 	while (*s) {
 		int position;
-		buf = s[0]->buf;
+		arg = s[0]->buf;
 
-		if (skip_prefix(buf, "position=", &buf)) {
-			position = parse_align_position(buf);
+		if (skip_prefix(arg, "position=", &arg)) {
+			position = parse_align_position(arg);
 			if (position < 0)
-				die(_("unrecognized position:%s"), buf);
+				die(_("unrecognized position:%s"), arg);
 			align->position = position;
-		} else if (skip_prefix(buf, "width=", &buf)) {
-			if (strtoul_ui(buf, 10, &width))
-				die(_("unrecognized width:%s"), buf);
-		} else if (!strtoul_ui(buf, 10, &width))
+		} else if (skip_prefix(arg, "width=", &arg)) {
+			if (strtoul_ui(arg, 10, &width))
+				die(_("unrecognized width:%s"), arg);
+		} else if (!strtoul_ui(arg, 10, &width))
 			;
-		else if ((position = parse_align_position(buf)) >= 0)
+		else if ((position = parse_align_position(arg)) >= 0)
 			align->position = position;
 		else
-			die(_("unrecognized %%(align) argument: %s"), buf);
+			die(_("unrecognized %%(align) argument: %s"), arg);
 		s++;
 	}
 
@@ -200,7 +166,7 @@ static void align_atom_parser(struct used_atom *atom)
 static struct {
 	const char *name;
 	cmp_type cmp_type;
-	void (*parser)(struct used_atom *atom);
+	void (*parser)(struct used_atom *atom, const char *arg);
 } valid_atom[] = {
 	{ "refname" },
 	{ "objecttype" },
@@ -226,8 +192,8 @@ static struct {
 	{ "taggerdate", FIELD_TIME },
 	{ "creator" },
 	{ "creatordate", FIELD_TIME },
-	{ "subject", FIELD_STR, contents_atom_parser },
-	{ "body", FIELD_STR, contents_atom_parser },
+	{ "subject", FIELD_STR, subject_atom_parser },
+	{ "body", FIELD_STR, body_atom_parser },
 	{ "contents", FIELD_STR, contents_atom_parser },
 	{ "upstream", FIELD_STR, remote_ref_atom_parser },
 	{ "push", FIELD_STR, remote_ref_atom_parser },
@@ -268,6 +234,7 @@ struct atom_value {
 int parse_ref_filter_atom(const char *atom, const char *ep)
 {
 	const char *sp;
+	const char *arg;
 	int i, at;
 
 	sp = atom;
@@ -292,10 +259,9 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
 		 * shouldn't be used for checking against the valid_atom
 		 * table.
 		 */
-		const char *formatp = strchr(sp, ':');
-		if (!formatp || ep < formatp)
-			formatp = ep;
-		if (len == formatp - sp && !memcmp(valid_atom[i].name, sp, len))
+		arg = memchr(sp, ':', ep - sp);
+		if ((!arg || len == arg - sp) &&
+		    !memcmp(valid_atom[i].name, sp, len))
 			break;
 	}
 
@@ -308,9 +274,11 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
 	REALLOC_ARRAY(used_atom, used_atom_cnt);
 	used_atom[at].name = xmemdupz(atom, ep - atom);
 	used_atom[at].type = valid_atom[i].cmp_type;
+	if (arg)
+		arg = used_atom[at].name + (arg - atom) + 1;
 	memset(&used_atom[at].u, 0, sizeof(used_atom[at].u));
 	if (valid_atom[i].parser)
-		valid_atom[i].parser(&used_atom[at]);
+		valid_atom[i].parser(&used_atom[at], arg);
 	if (*atom == '*')
 		need_tagged = 1;
 	if (!strcmp(used_atom[at].name, "symref"))
@@ -516,7 +484,7 @@ static void grab_common_values(struct atom_value *val, int deref, struct object
 			v->s = xstrfmt("%lu", sz);
 		}
 		else if (deref)
-			grab_objectname(name, obj->sha1, v, &used_atom[i]);
+			grab_objectname(name, obj->oid.hash, v, &used_atom[i]);
 	}
 }
 
@@ -538,7 +506,7 @@ static void grab_tag_values(struct atom_value *val, int deref, struct object *ob
 		else if (!strcmp(name, "type") && tag->tagged)
 			v->s = typename(tag->tagged->type);
 		else if (!strcmp(name, "object") && tag->tagged)
-			v->s = xstrdup(sha1_to_hex(tag->tagged->sha1));
+			v->s = xstrdup(oid_to_hex(&tag->tagged->oid));
 	}
 }
 
@@ -556,7 +524,7 @@ static void grab_commit_values(struct atom_value *val, int deref, struct object
 		if (deref)
 			name++;
 		if (!strcmp(name, "tree")) {
-			v->s = xstrdup(sha1_to_hex(commit->tree->object.sha1));
+			v->s = xstrdup(oid_to_hex(&commit->tree->object.oid));
 		}
 		else if (!strcmp(name, "numparent")) {
 			v->ul = commit_list_count(commit->parents);
@@ -569,7 +537,7 @@ static void grab_commit_values(struct atom_value *val, int deref, struct object
 				struct commit *parent = parents->item;
 				if (parents != commit->parents)
 					strbuf_addch(&s, ' ');
-				strbuf_addstr(&s, sha1_to_hex(parent->object.sha1));
+				strbuf_addstr(&s, oid_to_hex(&parent->object.oid));
 			}
 			v->s = strbuf_detach(&s, NULL);
 		}
@@ -931,7 +922,7 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
 			*s = ">";
 		else
 			*s = "<>";
-	} else if (atom->u.remote_ref == RR_NORMAL)
+	} else /* RR_NORMAL */
 		*s = refname;
 }
 
@@ -986,9 +977,8 @@ static void populate_value(struct ref_array_item *ref)
 			branch = branch_get(branch_name);
 
 			refname = branch_get_upstream(branch, NULL);
-			if (!refname)
-				continue;
-			fill_remote_ref_details(atom, refname, branch, &v->s);
+			if (refname)
+				fill_remote_ref_details(atom, refname, branch, &v->s);
 			continue;
 		} else if (starts_with(name, "push")) {
 			const char *branch_name;
--- a/strbuf.c
+++ b/strbuf.c
@@ -123,15 +123,18 @@ struct strbuf **strbuf_split_buf(const char *str, size_t slen,
 
 	while (slen) {
 		int len = slen;
+		int copylen = len;
 		const char *end = NULL;
 		if (max <= 0 || nr + 1 < max) {
 			end = memchr(str, terminator, slen);
-			if (end)
+			if (end) {
 				len = end - str + 1;
+				copylen = len - !!omit_term;
+			}
 		}
 		t = xmalloc(sizeof(struct strbuf));
-		strbuf_init(t, len);
-		strbuf_add(t, str, len - !!end * !!omit_term);
+		strbuf_init(t, copylen);
+		strbuf_add(t, str, copylen);
 		ALLOC_GROW(ret, nr + 2, alloc);
 		ret[nr++] = t;
 		str += len;

-- 
2.7.0

[PATCH v4 01/12] strbuf: introduce strbuf_split_str_omit_term()

[Karthik Nayak]

The current implementation of 'strbuf_split_buf()' includes the
terminator at the end of each strbuf post splitting. Add an option
wherein we can drop the terminator if desired. In this context
introduce a wrapper function 'strbuf_split_str_omit_term()' which
splits a given string into strbufs without including the terminator.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
---
 strbuf.c | 14 +++++++++-----
 strbuf.h | 25 ++++++++++++++++---------
 2 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/strbuf.c b/strbuf.c
index bab316d..4a93e2a 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -115,7 +115,7 @@ void strbuf_tolower(struct strbuf *sb)
 }
 
 struct strbuf **strbuf_split_buf(const char *str, size_t slen,
-				 int terminator, int max)
+				 int terminator, int max, int omit_term)
 {
 	struct strbuf **ret = NULL;
 	size_t nr = 0, alloc = 0;
@@ -123,14 +123,18 @@ struct strbuf **strbuf_split_buf(const char *str, size_t slen,
 
 	while (slen) {
 		int len = slen;
+		int copylen = len;
+		const char *end = NULL;
 		if (max <= 0 || nr + 1 < max) {
-			const char *end = memchr(str, terminator, slen);
-			if (end)
+			end = memchr(str, terminator, slen);
+			if (end) {
 				len = end - str + 1;
+				copylen = len - !!omit_term;
+			}
 		}
 		t = xmalloc(sizeof(struct strbuf));
-		strbuf_init(t, len);
-		strbuf_add(t, str, len);
+		strbuf_init(t, copylen);
+		strbuf_add(t, str, copylen);
 		ALLOC_GROW(ret, nr + 2, alloc);
 		ret[nr++] = t;
 		str += len;
diff --git a/strbuf.h b/strbuf.h
index f72fd14..6115e72 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -466,11 +466,12 @@ static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)
 /**
  * Split str (of length slen) at the specified terminator character.
  * Return a null-terminated array of pointers to strbuf objects
- * holding the substrings.  The substrings include the terminator,
- * except for the last substring, which might be unterminated if the
- * original string did not end with a terminator.  If max is positive,
- * then split the string into at most max substrings (with the last
- * substring containing everything following the (max-1)th terminator
+ * holding the substrings.  If omit_term is true, the terminator will
+ * be stripped from all substrings. Otherwise, substrings will include
+ * the terminator, except for the final substring, if the original
+ * string lacked a terminator.  If max is positive, then split the
+ * string into at most max substrings (with the last substring
+ * containing everything following the (max-1)th terminator
  * character).
  *
  * The most generic form is `strbuf_split_buf`, which takes an arbitrary
@@ -481,19 +482,25 @@ static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)
  * For lighter-weight alternatives, see string_list_split() and
  * string_list_split_in_place().
  */
-extern struct strbuf **strbuf_split_buf(const char *, size_t,
-					int terminator, int max);
+extern struct strbuf **strbuf_split_buf(const char *str, size_t slen,
+					int terminator, int max, int omit_term);
+
+static inline struct strbuf **strbuf_split_str_omit_term(const char *str,
+							    int terminator, int max)
+{
+	return strbuf_split_buf(str, strlen(str), terminator, max, 1);
+}
 
 static inline struct strbuf **strbuf_split_str(const char *str,
 					       int terminator, int max)
 {
-	return strbuf_split_buf(str, strlen(str), terminator, max);
+	return strbuf_split_buf(str, strlen(str), terminator, max, 0);
 }
 
 static inline struct strbuf **strbuf_split_max(const struct strbuf *sb,
 						int terminator, int max)
 {
-	return strbuf_split_buf(sb->buf, sb->len, terminator, max);
+	return strbuf_split_buf(sb->buf, sb->len, terminator, max, 0);
 }
 
 static inline struct strbuf **strbuf_split(const struct strbuf *sb,
-- 
2.7.0

[PATCH v4 02/12] ref-filter: use strbuf_split_str_omit_term()

[Karthik Nayak]

Use the newly introduced strbuf_split_str_omit_term() rather than
using strbuf_split_str() and manually removing the ',' terminator.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
---
 ref-filter.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index f097176..38f38d4 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -892,18 +892,11 @@ static void populate_value(struct ref_array_item *ref)
 			if (!valp)
 				die(_("expected format: %%(align:<width>,<position>)"));
 
-			/*
-			 * TODO: Implement a function similar to strbuf_split_str()
-			 * which would omit the separator from the end of each value.
-			 */
-			s = to_free = strbuf_split_str(valp, ',', 0);
+			s = to_free = strbuf_split_str_omit_term(valp, ',', 0);
 
 			align->position = ALIGN_LEFT;
 
 			while (*s) {
-				/*  Strip trailing comma */
-				if (s[1])
-					strbuf_setlen(s[0], s[0]->len - 1);
 				if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))
 					;
 				else if (!strcmp(s[0]->buf, "left"))
-- 
2.7.0

[PATCH v4 03/12] ref-filter: bump 'used_atom' and related code to the top

[Karthik Nayak]

Bump code to the top for usage in further patches.
---
 ref-filter.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 38f38d4..c3a8372 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -16,6 +16,21 @@
 
 typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 
+/*
+ * An atom is a valid field atom listed below, possibly prefixed with
+ * a "*" to denote deref_tag().
+ *
+ * We parse given format string and sort specifiers, and make a list
+ * of properties that we need to extract out of objects.  ref_array_item
+ * structure will hold an array of values extracted that can be
+ * indexed with the "atom number", which is an index into this
+ * array.
+ */
+static const char **used_atom;
+static cmp_type *used_atom_type;
+static int used_atom_cnt, need_tagged, need_symref;
+static int need_color_reset_at_eol;
+
 static struct {
 	const char *name;
 	cmp_type cmp_type;
@@ -92,21 +107,6 @@ struct atom_value {
 };
 
 /*
- * An atom is a valid field atom listed above, possibly prefixed with
- * a "*" to denote deref_tag().
- *
- * We parse given format string and sort specifiers, and make a list
- * of properties that we need to extract out of objects.  ref_array_item
- * structure will hold an array of values extracted that can be
- * indexed with the "atom number", which is an index into this
- * array.
- */
-static const char **used_atom;
-static cmp_type *used_atom_type;
-static int used_atom_cnt, need_tagged, need_symref;
-static int need_color_reset_at_eol;
-
-/*
  * Used to parse format string and sort specifiers
  */
 int parse_ref_filter_atom(const char *atom, const char *ep)
-- 
2.7.0

[PATCH v4 04/12] ref-filter: introduce struct used_atom

[Karthik Nayak]

Introduce the 'used_atom' structure to replace the existing
implementation of 'used_atom' (which is a list of atoms). This helps
us parse atoms beforehand and store required details into the
'used_atom' for future usage.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
---
 ref-filter.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index c3a8372..3736dc3 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -26,8 +26,10 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
  * indexed with the "atom number", which is an index into this
  * array.
  */
-static const char **used_atom;
-static cmp_type *used_atom_type;
+static struct used_atom {
+	const char *name;
+	cmp_type type;
+} *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
 static int need_color_reset_at_eol;
 
@@ -122,8 +124,8 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
 
 	/* Do we have the atom already used elsewhere? */
 	for (i = 0; i < used_atom_cnt; i++) {
-		int len = strlen(used_atom[i]);
-		if (len == ep - atom && !memcmp(used_atom[i], atom, len))
+		int len = strlen(used_atom[i].name);
+		if (len == ep - atom && !memcmp(used_atom[i].name, atom, len))
 			return i;
 	}
 
@@ -150,12 +152,11 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
 	at = used_atom_cnt;
 	used_atom_cnt++;
 	REALLOC_ARRAY(used_atom, used_atom_cnt);
-	REALLOC_ARRAY(used_atom_type, used_atom_cnt);
-	used_atom[at] = xmemdupz(atom, ep - atom);
-	used_atom_type[at] = valid_atom[i].cmp_type;
+	used_atom[at].name = xmemdupz(atom, ep - atom);
+	used_atom[at].type = valid_atom[i].cmp_type;
 	if (*atom == '*')
 		need_tagged = 1;
-	if (!strcmp(used_atom[at], "symref"))
+	if (!strcmp(used_atom[at].name, "symref"))
 		need_symref = 1;
 	return at;
 }
@@ -315,7 +316,7 @@ int verify_ref_format(const char *format)
 		at = parse_ref_filter_atom(sp + 2, ep);
 		cp = ep + 1;
 
-		if (skip_prefix(used_atom[at], "color:", &color))
+		if (skip_prefix(used_atom[at].name, "color:", &color))
 			need_color_reset_at_eol = !!strcmp(color, "reset");
 	}
 	return 0;
@@ -359,7 +360,7 @@ static void grab_common_values(struct atom_value *val, int deref, struct object
 	int i;
 
 	for (i = 0; i < used_atom_cnt; i++) {
-		const char *name = used_atom[i];
+		const char *name = used_atom[i].name;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;
@@ -383,7 +384,7 @@ static void grab_tag_values(struct atom_value *val, int deref, struct object *ob
 	struct tag *tag = (struct tag *) obj;
 
 	for (i = 0; i < used_atom_cnt; i++) {
-		const char *name = used_atom[i];
+		const char *name = used_atom[i].name;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;
@@ -405,7 +406,7 @@ static void grab_commit_values(struct atom_value *val, int deref, struct object
 	struct commit *commit = (struct commit *) obj;
 
 	for (i = 0; i < used_atom_cnt; i++) {
-		const char *name = used_atom[i];
+		const char *name = used_atom[i].name;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;
@@ -535,7 +536,7 @@ static void grab_person(const char *who, struct atom_value *val, int deref, stru
 	const char *wholine = NULL;
 
 	for (i = 0; i < used_atom_cnt; i++) {
-		const char *name = used_atom[i];
+		const char *name = used_atom[i].name;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;
@@ -573,7 +574,7 @@ static void grab_person(const char *who, struct atom_value *val, int deref, stru
 	if (!wholine)
 		return;
 	for (i = 0; i < used_atom_cnt; i++) {
-		const char *name = used_atom[i];
+		const char *name = used_atom[i].name;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;
@@ -663,7 +664,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
 	unsigned long sublen = 0, bodylen = 0, nonsiglen = 0, siglen = 0;
 
 	for (i = 0; i < used_atom_cnt; i++) {
-		const char *name = used_atom[i];
+		const char *name = used_atom[i].name;
 		struct atom_value *v = &val[i];
 		const char *valp = NULL;
 		if (!!deref != (*name == '*'))
@@ -809,7 +810,7 @@ static void populate_value(struct ref_array_item *ref)
 
 	/* Fill in specials first */
 	for (i = 0; i < used_atom_cnt; i++) {
-		const char *name = used_atom[i];
+		const char *name = used_atom[i].name;
 		struct atom_value *v = &ref->value[i];
 		int deref = 0;
 		const char *refname;
@@ -1464,7 +1465,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
 {
 	struct atom_value *va, *vb;
 	int cmp;
-	cmp_type cmp_type = used_atom_type[s->atom];
+	cmp_type cmp_type = used_atom[s->atom].type;
 
 	get_ref_atom_value(a, s->atom, &va);
 	get_ref_atom_value(b, s->atom, &vb);
-- 
2.7.0

[PATCH v4 05/12] ref-filter: introduce parsing functions for each valid atom

[Karthik Nayak]

Parsing atoms is done in populate_value(), this is repetitive and
hence expensive. Introduce a parsing function which would let us parse
atoms beforehand and store the required details into the 'used_atom'
structure for further usage.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
---
 ref-filter.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 3736dc3..92b4419 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -36,6 +36,7 @@ static int need_color_reset_at_eol;
 static struct {
 	const char *name;
 	cmp_type cmp_type;
+	void (*parser)(struct used_atom *atom, const char *arg);
 } valid_atom[] = {
 	{ "refname" },
 	{ "objecttype" },
@@ -114,6 +115,7 @@ struct atom_value {
 int parse_ref_filter_atom(const char *atom, const char *ep)
 {
 	const char *sp;
+	const char *arg;
 	int i, at;
 
 	sp = atom;
@@ -138,10 +140,9 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
 		 * shouldn't be used for checking against the valid_atom
 		 * table.
 		 */
-		const char *formatp = strchr(sp, ':');
-		if (!formatp || ep < formatp)
-			formatp = ep;
-		if (len == formatp - sp && !memcmp(valid_atom[i].name, sp, len))
+		arg = memchr(sp, ':', ep - sp);
+		if ((!arg || len == arg - sp) &&
+		    !memcmp(valid_atom[i].name, sp, len))
 			break;
 	}
 
@@ -154,6 +155,10 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
 	REALLOC_ARRAY(used_atom, used_atom_cnt);
 	used_atom[at].name = xmemdupz(atom, ep - atom);
 	used_atom[at].type = valid_atom[i].cmp_type;
+	if (arg)
+		arg = used_atom[at].name + (arg - atom) + 1;
+	if (valid_atom[i].parser)
+		valid_atom[i].parser(&used_atom[at], arg);
 	if (*atom == '*')
 		need_tagged = 1;
 	if (!strcmp(used_atom[at].name, "symref"))
-- 
2.7.0

[PATCH v4 06/12] ref-filter: introduce color_atom_parser()

[Karthik Nayak]

Introduce color_atom_parser() which will parse a "color" atom and
store its color in the "used_atom" structure for further usage in
populate_value().

Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
---
 ref-filter.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 92b4419..7adff67 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -29,10 +29,21 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 static struct used_atom {
 	const char *name;
 	cmp_type type;
+	union {
+		char color[COLOR_MAXLEN];
+	} u;
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
 static int need_color_reset_at_eol;
 
+static void color_atom_parser(struct used_atom *atom, const char *color_value)
+{
+	if (!color_value)
+		die(_("expected format: %%(color:<color>)"));
+	if (color_parse(color_value, atom->u.color) < 0)
+		die(_("invalid color value: %s"), atom->u.color);
+}
+
 static struct {
 	const char *name;
 	cmp_type cmp_type;
@@ -70,7 +81,7 @@ static struct {
 	{ "symref" },
 	{ "flag" },
 	{ "HEAD" },
-	{ "color" },
+	{ "color", FIELD_STR, color_atom_parser },
 	{ "align" },
 	{ "end" },
 };
@@ -157,6 +168,7 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
 	used_atom[at].type = valid_atom[i].cmp_type;
 	if (arg)
 		arg = used_atom[at].name + (arg - atom) + 1;
+	memset(&used_atom[at].u, 0, sizeof(used_atom[at].u));
 	if (valid_atom[i].parser)
 		valid_atom[i].parser(&used_atom[at], arg);
 	if (*atom == '*')
@@ -815,6 +827,7 @@ static void populate_value(struct ref_array_item *ref)
 
 	/* Fill in specials first */
 	for (i = 0; i < used_atom_cnt; i++) {
+		struct used_atom *atom = &used_atom[i];
 		const char *name = used_atom[i].name;
 		struct atom_value *v = &ref->value[i];
 		int deref = 0;
@@ -855,14 +868,8 @@ static void populate_value(struct ref_array_item *ref)
 			refname = branch_get_push(branch, NULL);
 			if (!refname)
 				continue;
-		} else if (match_atom_name(name, "color", &valp)) {
-			char color[COLOR_MAXLEN] = "";
-
-			if (!valp)
-				die(_("expected format: %%(color:<color>)"));
-			if (color_parse(valp, color) < 0)
-				die(_("unable to parse format"));
-			v->s = xstrdup(color);
+		} else if (starts_with(name, "color:")) {
+			v->s = atom->u.color;
 			continue;
 		} else if (!strcmp(name, "flag")) {
 			char buf[256], *cp = buf;
-- 
2.7.0

[PATCH v4 07/12] ref-filter: introduce parse_align_position()

[Karthik Nayak]

>From populate_value() extract parse_align_position() which given a
string would give us the alignment position. This is a preparatory
patch as to introduce prefixes for the %(align) atom and avoid
redundancy in the code.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
---
 ref-filter.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 7adff67..e6b6b55 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -44,6 +44,17 @@ static void color_atom_parser(struct used_atom *atom, const char *color_value)
 		die(_("invalid color value: %s"), atom->u.color);
 }
 
+static align_type parse_align_position(const char *s)
+{
+	if (!strcmp(s, "right"))
+		return ALIGN_RIGHT;
+	else if (!strcmp(s, "middle"))
+		return ALIGN_MIDDLE;
+	else if (!strcmp(s, "left"))
+		return ALIGN_LEFT;
+	return -1;
+}
+
 static struct {
 	const char *name;
 	cmp_type cmp_type;
@@ -910,14 +921,12 @@ static void populate_value(struct ref_array_item *ref)
 			align->position = ALIGN_LEFT;
 
 			while (*s) {
+				int position;
+
 				if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))
 					;
-				else if (!strcmp(s[0]->buf, "left"))
-					align->position = ALIGN_LEFT;
-				else if (!strcmp(s[0]->buf, "right"))
-					align->position = ALIGN_RIGHT;
-				else if (!strcmp(s[0]->buf, "middle"))
-					align->position = ALIGN_MIDDLE;
+				else if ((position = parse_align_position(s[0]->buf)) >= 0)
+					align->position = position;
 				else
 					die(_("improper format entered align:%s"), s[0]->buf);
 				s++;
-- 
2.7.0

[PATCH v4 08/12] ref-filter: introduce align_atom_parser()

[Karthik Nayak]

Introduce align_atom_parser() which will parse an 'align' atom and
store the required alignment position and width in the 'used_atom'
structure for further usage in populate_value().

Since this patch removes the last usage of match_atom_name(), remove
the function from ref-filter.c.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
---
 ref-filter.c | 91 ++++++++++++++++++++++++++----------------------------------
 1 file changed, 40 insertions(+), 51 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index e6b6b55..79a7e07 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -16,6 +16,11 @@
 
 typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 
+struct align {
+	align_type position;
+	unsigned int width;
+};
+
 /*
  * An atom is a valid field atom listed below, possibly prefixed with
  * a "*" to denote deref_tag().
@@ -31,6 +36,7 @@ static struct used_atom {
 	cmp_type type;
 	union {
 		char color[COLOR_MAXLEN];
+		struct align align;
 	} u;
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
@@ -55,6 +61,37 @@ static align_type parse_align_position(const char *s)
 	return -1;
 }
 
+static void align_atom_parser(struct used_atom *atom, const char *arg)
+{
+	struct align *align = &atom->u.align;
+	struct strbuf **s, **to_free;
+	unsigned int width = ~0U;
+
+	if (!arg)
+		die(_("expected format: %%(align:<width>,<position>)"));
+	s = to_free = strbuf_split_str_omit_term(arg, ',', 0);
+
+	align->position = ALIGN_LEFT;
+
+	while (*s) {
+		int position;
+		arg = s[0]->buf;
+
+		if (!strtoul_ui(arg, 10, &width))
+			;
+		else if ((position = parse_align_position(arg)) >= 0)
+			align->position = position;
+		else
+			die(_("unrecognized %%(align) argument: %s"), arg);
+		s++;
+	}
+
+	if (width == ~0U)
+		die(_("positive width expected with the %%(align) atom"));
+	align->width = width;
+	strbuf_list_free(to_free);
+}
+
 static struct {
 	const char *name;
 	cmp_type cmp_type;
@@ -93,17 +130,12 @@ static struct {
 	{ "flag" },
 	{ "HEAD" },
 	{ "color", FIELD_STR, color_atom_parser },
-	{ "align" },
+	{ "align", FIELD_STR, align_atom_parser },
 	{ "end" },
 };
 
 #define REF_FORMATTING_STATE_INIT  { 0, NULL }
 
-struct align {
-	align_type position;
-	unsigned int width;
-};
-
 struct contents {
 	unsigned int lines;
 	struct object_id oid;
@@ -287,22 +319,6 @@ static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_sta
 	pop_stack_element(&state->stack);
 }
 
-static int match_atom_name(const char *name, const char *atom_name, const char **val)
-{
-	const char *body;
-
-	if (!skip_prefix(name, atom_name, &body))
-		return 0; /* doesn't even begin with "atom_name" */
-	if (!body[0]) {
-		*val = NULL; /* %(atom_name) and no customization */
-		return 1;
-	}
-	if (body[0] != ':')
-		return 0; /* "atom_namefoo" is not "atom_name" or "atom_name:..." */
-	*val = body + 1; /* "atom_name:val" */
-	return 1;
-}
-
 /*
  * In a format string, find the next occurrence of %(atom).
  */
@@ -844,7 +860,6 @@ static void populate_value(struct ref_array_item *ref)
 		int deref = 0;
 		const char *refname;
 		const char *formatp;
-		const char *valp;
 		struct branch *branch = NULL;
 
 		v->handler = append_atom;
@@ -908,34 +923,8 @@ static void populate_value(struct ref_array_item *ref)
 			else
 				v->s = " ";
 			continue;
-		} else if (match_atom_name(name, "align", &valp)) {
-			struct align *align = &v->u.align;
-			struct strbuf **s, **to_free;
-			int width = -1;
-
-			if (!valp)
-				die(_("expected format: %%(align:<width>,<position>)"));
-
-			s = to_free = strbuf_split_str_omit_term(valp, ',', 0);
-
-			align->position = ALIGN_LEFT;
-
-			while (*s) {
-				int position;
-
-				if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))
-					;
-				else if ((position = parse_align_position(s[0]->buf)) >= 0)
-					align->position = position;
-				else
-					die(_("improper format entered align:%s"), s[0]->buf);
-				s++;
-			}
-
-			if (width < 0)
-				die(_("positive width expected with the %%(align) atom"));
-			align->width = width;
-			strbuf_list_free(to_free);
+		} else if (starts_with(name, "align")) {
+			v->u.align = atom->u.align;
 			v->handler = align_atom_handler;
 			continue;
 		} else if (!strcmp(name, "end")) {
-- 
2.7.0

[PATCH v4 09/12] ref-filter: align: introduce long-form syntax

[Karthik Nayak]

Introduce optional prefixes "width=" and "position=" for the align atom
so that the atom can be used as "%(align:width=<width>,position=<position>)".

Add Documentation and tests for the same.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
---
 Documentation/git-for-each-ref.txt | 20 ++++++++++--------
 ref-filter.c                       | 10 ++++++++-
 t/t6302-for-each-ref-filter.sh     | 42 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 2e3e96f..012e8f9 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -133,14 +133,18 @@ color::
 
 align::
 	Left-, middle-, or right-align the content between
-	%(align:...) and %(end). The "align:" is followed by `<width>`
-	and `<position>` in any order separated by a comma, where the
-	`<position>` is either left, right or middle, default being
-	left and `<width>` is the total length of the content with
-	alignment. If the contents length is more than the width then
-	no alignment is performed. If used with '--quote' everything
-	in between %(align:...) and %(end) is quoted, but if nested
-	then only the topmost level performs quoting.
+	%(align:...) and %(end). The "align:" is followed by
+	`width=<width>` and `position=<position>` in any order
+	separated by a comma, where the `<position>` is either left,
+	right or middle, default being left and `<width>` is the total
+	length of the content with alignment. For brevity, the
+	"width=" and/or "position=" prefixes may be omitted, and bare
+	<width> and <position> used instead.  For instance,
+	`%(align:<width>,<position>)`. If the contents length is more
+	than the width then no alignment is performed. If used with
+	'--quote' everything in between %(align:...) and %(end) is
+	quoted, but if nested then only the topmost level performs
+	quoting.
 
 In addition to the above, for commit and tag objects, the header
 field names (`tree`, `parent`, `object`, `type`, and `tag`) can
diff --git a/ref-filter.c b/ref-filter.c
index 79a7e07..58d433f 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -77,7 +77,15 @@ static void align_atom_parser(struct used_atom *atom, const char *arg)
 		int position;
 		arg = s[0]->buf;
 
-		if (!strtoul_ui(arg, 10, &width))
+		if (skip_prefix(arg, "position=", &arg)) {
+			position = parse_align_position(arg);
+			if (position < 0)
+				die(_("unrecognized position:%s"), arg);
+			align->position = position;
+		} else if (skip_prefix(arg, "width=", &arg)) {
+			if (strtoul_ui(arg, 10, &width))
+				die(_("unrecognized width:%s"), arg);
+		} else if (!strtoul_ui(arg, 10, &width))
 			;
 		else if ((position = parse_align_position(arg)) >= 0)
 			align->position = position;
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index fe4796c..f79355d 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -133,6 +133,48 @@ test_expect_success 'right alignment' '
 	test_cmp expect actual
 '
 
+cat >expect <<-\EOF
+|       refname is refs/heads/master       |refs/heads/master
+|        refname is refs/heads/side        |refs/heads/side
+|         refname is refs/odd/spot         |refs/odd/spot
+|     refname is refs/tags/double-tag      |refs/tags/double-tag
+|        refname is refs/tags/four         |refs/tags/four
+|         refname is refs/tags/one         |refs/tags/one
+|     refname is refs/tags/signed-tag      |refs/tags/signed-tag
+|        refname is refs/tags/three        |refs/tags/three
+|         refname is refs/tags/two         |refs/tags/two
+EOF
+
+test_align_permutations() {
+	while read -r option
+	do
+		test_expect_success "align:$option" '
+		git for-each-ref --format="|%(align:$option)refname is %(refname)%(end)|%(refname)" >actual &&
+		test_cmp expect actual
+		'
+	done
+}
+
+test_align_permutations <<-\EOF
+	middle,42
+	42,middle
+	position=middle,42
+	42,position=middle
+	middle,width=42
+	width=42,middle
+	position=middle,width=42
+	width=42,position=middle
+EOF
+
+# Last one wins (silently) when multiple arguments of the same type are given
+
+test_align_permutations <<-\EOF
+	32,width=42,middle
+	width=30,42,middle
+	width=42,position=right,middle
+	42,right,position=middle
+EOF
+
 # Individual atoms inside %(align:...) and %(end) must not be quoted.
 
 test_expect_success 'alignment with format quote' "
-- 
2.7.0

[PATCH v4 11/12] ref-filter: introduce contents_atom_parser()

[Karthik Nayak]

Introduce contents_atom_parser() which will parse the '%(contents)'
atom and store information into the 'used_atom' structure based on the
modifiers used along with the atom. Also introduce body_atom_parser()
and subject_atom_parser() for parsing atoms '%(body)' and '%(subject)'
respectively.

Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
---
 ref-filter.c | 77 ++++++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 49 insertions(+), 28 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 99c152d..b2043a0 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -39,6 +39,10 @@ static struct used_atom {
 		struct align align;
 		enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }
 			remote_ref;
+		struct {
+			enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
+			unsigned int nlines;
+		} contents;
 	} u;
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
@@ -66,6 +70,38 @@ static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
 		die(_("unrecognized format: %%(%s)"), atom->name);
 }
 
+static void body_atom_parser(struct used_atom *atom, const char *arg)
+{
+	if (arg)
+		die("%%(body) atom does not take arguments");
+	atom->u.contents.option = C_BODY_DEP;
+}
+
+static void subject_atom_parser(struct used_atom *atom, const char *arg)
+{
+	if (arg)
+		die("%%(subject) atom does not take arguments");
+	atom->u.contents.option = C_SUB;
+}
+
+static void contents_atom_parser(struct used_atom *atom, const char *arg)
+{
+	if (!arg)
+		atom->u.contents.option = C_BARE;
+	else if (!strcmp(arg, "body"))
+		atom->u.contents.option = C_BODY;
+	else if (!strcmp(arg, "signature"))
+		atom->u.contents.option = C_SIG;
+	else if (!strcmp(arg, "subject"))
+		atom->u.contents.option = C_SUB;
+	else if (skip_prefix(arg, "lines=", &arg)) {
+		atom->u.contents.option = C_LINES;
+		if (strtoul_ui(arg, 10, &atom->u.contents.nlines))
+			die(_("positive value expected contents:lines=%s"), arg);
+	} else
+		die(_("unrecognized %%(contents) argument: %s"), arg);
+}
+
 static align_type parse_align_position(const char *s)
 {
 	if (!strcmp(s, "right"))
@@ -145,9 +181,9 @@ static struct {
 	{ "taggerdate", FIELD_TIME },
 	{ "creator" },
 	{ "creatordate", FIELD_TIME },
-	{ "subject" },
-	{ "body" },
-	{ "contents" },
+	{ "subject", FIELD_STR, subject_atom_parser },
+	{ "body", FIELD_STR, body_atom_parser },
+	{ "contents", FIELD_STR, contents_atom_parser },
 	{ "upstream", FIELD_STR, remote_ref_atom_parser },
 	{ "push", FIELD_STR, remote_ref_atom_parser },
 	{ "symref" },
@@ -160,11 +196,6 @@ static struct {
 
 #define REF_FORMATTING_STATE_INIT  { 0, NULL }
 
-struct contents {
-	unsigned int lines;
-	struct object_id oid;
-};
-
 struct ref_formatting_stack {
 	struct ref_formatting_stack *prev;
 	struct strbuf output;
@@ -181,7 +212,6 @@ struct atom_value {
 	const char *s;
 	union {
 		struct align align;
-		struct contents contents;
 	} u;
 	void (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);
 	unsigned long ul; /* used for sorting when not FIELD_STR */
@@ -733,19 +763,15 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
 
 	for (i = 0; i < used_atom_cnt; i++) {
 		const char *name = used_atom[i].name;
+		struct used_atom *atom = &used_atom[i];
 		struct atom_value *v = &val[i];
-		const char *valp = NULL;
 		if (!!deref != (*name == '*'))
 			continue;
 		if (deref)
 			name++;
 		if (strcmp(name, "subject") &&
 		    strcmp(name, "body") &&
-		    strcmp(name, "contents") &&
-		    strcmp(name, "contents:subject") &&
-		    strcmp(name, "contents:body") &&
-		    strcmp(name, "contents:signature") &&
-		    !starts_with(name, "contents:lines="))
+		    !starts_with(name, "contents"))
 			continue;
 		if (!subpos)
 			find_subpos(buf, sz,
@@ -753,28 +779,23 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
 				    &bodypos, &bodylen, &nonsiglen,
 				    &sigpos, &siglen);
 
-		if (!strcmp(name, "subject"))
+		if (atom->u.contents.option == C_SUB)
 			v->s = copy_subject(subpos, sublen);
-		else if (!strcmp(name, "contents:subject"))
-			v->s = copy_subject(subpos, sublen);
-		else if (!strcmp(name, "body"))
+		else if (atom->u.contents.option == C_BODY_DEP)
 			v->s = xmemdupz(bodypos, bodylen);
-		else if (!strcmp(name, "contents:body"))
+		else if (atom->u.contents.option == C_BODY)
 			v->s = xmemdupz(bodypos, nonsiglen);
-		else if (!strcmp(name, "contents:signature"))
+		else if (atom->u.contents.option == C_SIG)
 			v->s = xmemdupz(sigpos, siglen);
-		else if (!strcmp(name, "contents"))
-			v->s = xstrdup(subpos);
-		else if (skip_prefix(name, "contents:lines=", &valp)) {
+		else if (atom->u.contents.option == C_LINES) {
 			struct strbuf s = STRBUF_INIT;
 			const char *contents_end = bodylen + bodypos - siglen;
 
-			if (strtoul_ui(valp, 10, &v->u.contents.lines))
-				die(_("positive value expected contents:lines=%s"), valp);
 			/*  Size is the length of the message after removing the signature */
-			append_lines(&s, subpos, contents_end - subpos, v->u.contents.lines);
+			append_lines(&s, subpos, contents_end - subpos, atom->u.contents.nlines);
 			v->s = strbuf_detach(&s, NULL);
-		}
+		} else if (atom->u.contents.option == C_BARE)
+			v->s = xstrdup(subpos);
 	}
 }
 
-- 
2.7.0

[PATCH v4 12/12] ref-filter: introduce objectname_atom_parser()

[Karthik Nayak]

Introduce objectname_atom_parser() which will parse the
'%(objectname)' atom and store information into the 'used_atom'
structure based on the modifiers used along with the atom.

Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
---
 ref-filter.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index b2043a0..d48e2a3 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -43,6 +43,7 @@ static struct used_atom {
 			enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
 			unsigned int nlines;
 		} contents;
+		enum { O_FULL, O_SHORT } objectname;
 	} u;
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
@@ -102,6 +103,16 @@ static void contents_atom_parser(struct used_atom *atom, const char *arg)
 		die(_("unrecognized %%(contents) argument: %s"), arg);
 }
 
+static void objectname_atom_parser(struct used_atom *atom, const char *arg)
+{
+	if (!arg)
+		atom->u.objectname = O_FULL;
+	else if (!strcmp(arg, "short"))
+		atom->u.objectname = O_SHORT;
+	else
+		die(_("unrecognized %%(objectname) argument: %s"), arg);
+}
+
 static align_type parse_align_position(const char *s)
 {
 	if (!strcmp(s, "right"))
@@ -160,7 +171,7 @@ static struct {
 	{ "refname" },
 	{ "objecttype" },
 	{ "objectsize", FIELD_ULONG },
-	{ "objectname" },
+	{ "objectname", FIELD_STR, objectname_atom_parser },
 	{ "tree" },
 	{ "parent" },
 	{ "numparent", FIELD_ULONG },
@@ -439,15 +450,17 @@ static void *get_obj(const unsigned char *sha1, struct object **obj, unsigned lo
 }
 
 static int grab_objectname(const char *name, const unsigned char *sha1,
-			    struct atom_value *v)
+			   struct atom_value *v, struct used_atom *atom)
 {
-	if (!strcmp(name, "objectname")) {
-		v->s = xstrdup(sha1_to_hex(sha1));
-		return 1;
-	}
-	if (!strcmp(name, "objectname:short")) {
-		v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));
-		return 1;
+	if (starts_with(name, "objectname")) {
+		if (atom->u.objectname == O_SHORT) {
+			v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));
+			return 1;
+		} else if (atom->u.objectname == O_FULL) {
+			v->s = xstrdup(sha1_to_hex(sha1));
+			return 1;
+		} else
+			die("BUG: unknown %%(objectname) option");
 	}
 	return 0;
 }
@@ -471,7 +484,7 @@ static void grab_common_values(struct atom_value *val, int deref, struct object
 			v->s = xstrfmt("%lu", sz);
 		}
 		else if (deref)
-			grab_objectname(name, obj->oid.hash, v);
+			grab_objectname(name, obj->oid.hash, v, &used_atom[i]);
 	}
 }
 
@@ -995,7 +1008,7 @@ static void populate_value(struct ref_array_item *ref)
 				v->s = xstrdup(buf + 1);
 			}
 			continue;
-		} else if (!deref && grab_objectname(name, ref->objectname, v)) {
+		} else if (!deref && grab_objectname(name, ref->objectname, v, atom)) {
 			continue;
 		} else if (!strcmp(name, "HEAD")) {
 			const char *head;
-- 
2.7.0

Re: [PATCH v4 03/12] ref-filter: bump 'used_atom' and related code to the top

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> Bump code to the top for usage in further patches.
> ---

Sign-off?

>  ref-filter.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 38f38d4..c3a8372 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -16,6 +16,21 @@
>  
>  typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
>  
> +/*
> + * An atom is a valid field atom listed below, possibly prefixed with
> + * a "*" to denote deref_tag().
> + *
> + * We parse given format string and sort specifiers, and make a list
> + * of properties that we need to extract out of objects.  ref_array_item
> + * structure will hold an array of values extracted that can be
> + * indexed with the "atom number", which is an index into this
> + * array.
> + */
> +static const char **used_atom;
> +static cmp_type *used_atom_type;
> +static int used_atom_cnt, need_tagged, need_symref;
> +static int need_color_reset_at_eol;
> +
>  static struct {
>  	const char *name;
>  	cmp_type cmp_type;
> @@ -92,21 +107,6 @@ struct atom_value {
>  };
>  
>  /*
> - * An atom is a valid field atom listed above, possibly prefixed with
> - * a "*" to denote deref_tag().
> - *
> - * We parse given format string and sort specifiers, and make a list
> - * of properties that we need to extract out of objects.  ref_array_item
> - * structure will hold an array of values extracted that can be
> - * indexed with the "atom number", which is an index into this
> - * array.
> - */
> -static const char **used_atom;
> -static cmp_type *used_atom_type;
> -static int used_atom_cnt, need_tagged, need_symref;
> -static int need_color_reset_at_eol;
> -
> -/*
>   * Used to parse format string and sort specifiers
>   */
>  int parse_ref_filter_atom(const char *atom, const char *ep)

Re: [PATCH v4 00/12] ref-filter: use parsing functions

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> This series cleans up populate_value() in ref-filter, by moving out
> the parsing part of atoms to separate parsing functions. This ensures
> that parsing is only done once and also improves the modularity of the
> code.
>
> v1: http://thread.gmane.org/gmane.comp.version-control.git/281180
> v2: http://thread.gmane.org/gmane.comp.version-control.git/282563
> v3: http://thread.gmane.org/gmane.comp.version-control.git/283350
>
> Changes:
> * The parsing functions now take the arguments of the atom as
> function parameteres, instead of parsing it inside the fucntion.
> * Rebased on top of pu:jk/list-tag-2.7-regression
> * In strbuf use a copylen variable rather than using multiplication
> to perform a logical operation.
> * Code movement for easier review and general improvement.
> * Use COLOR_MAXLEN as the maximum size for the color variable.
> * Small code changes.
> * Documentation changes.
> * Fixed incorrect style of test (t6302).
>
> Karthik Nayak (12):
>   strbuf: introduce strbuf_split_str_omit_term()
>   ref-filter: use strbuf_split_str_omit_term()
>   ref-filter: bump 'used_atom' and related code to the top
>   ref-filter: introduce struct used_atom
>   ref-filter: introduce parsing functions for each valid atom
>   ref-filter: introduce color_atom_parser()
>   ref-filter: introduce parse_align_position()
>   ref-filter: introduce align_atom_parser()
>   ref-filter: align: introduce long-form syntax
>   ref-filter: introduce remote_ref_atom_parser()
>   ref-filter: introduce contents_atom_parser()
>   ref-filter: introduce objectname_atom_parser()

Hmm, 10/12 didn't make it to the list?

Re: [PATCH v4 00/12] ref-filter: use parsing functions

[Eric Sunshine]

On Mon, Feb 1, 2016 at 5:25 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> This series cleans up populate_value() in ref-filter, by moving out
>> the parsing part of atoms to separate parsing functions. This ensures
>> that parsing is only done once and also improves the modularity of the
>> code.
>>
>> v1: http://thread.gmane.org/gmane.comp.version-control.git/281180
>> v2: http://thread.gmane.org/gmane.comp.version-control.git/282563
>> v3: http://thread.gmane.org/gmane.comp.version-control.git/283350
>>
>> Changes:
>> * The parsing functions now take the arguments of the atom as
>> function parameteres, instead of parsing it inside the fucntion.
>> * Rebased on top of pu:jk/list-tag-2.7-regression
>> * In strbuf use a copylen variable rather than using multiplication
>> to perform a logical operation.
>> * Code movement for easier review and general improvement.
>> * Use COLOR_MAXLEN as the maximum size for the color variable.
>> * Small code changes.
>> * Documentation changes.
>> * Fixed incorrect style of test (t6302).
>>
>> Karthik Nayak (12):
>>   strbuf: introduce strbuf_split_str_omit_term()
>>   ref-filter: use strbuf_split_str_omit_term()
>>   ref-filter: bump 'used_atom' and related code to the top
>>   ref-filter: introduce struct used_atom
>>   ref-filter: introduce parsing functions for each valid atom
>>   ref-filter: introduce color_atom_parser()
>>   ref-filter: introduce parse_align_position()
>>   ref-filter: introduce align_atom_parser()
>>   ref-filter: align: introduce long-form syntax
>>   ref-filter: introduce remote_ref_atom_parser()
>>   ref-filter: introduce contents_atom_parser()
>>   ref-filter: introduce objectname_atom_parser()
>
> Hmm, 10/12 didn't make it to the list?

Not surprising. Somehow, Karthik did git-add on the compiled
test-fake-ssh before committing, so the patch sent to the list
contains an rather huge binary resource. I did receive it since I was
a direct addressee of the email; I'll reply to the message on-list
without modifying anything (other than stripping out the binary
resource) so that other reviewers get a chance to see it.

Re: [PATCH v4 10/12] ref-filter: introduce remote_ref_atom_parser()

[Eric Sunshine]

This is a re-send of patch 10/12 on Karthik's behalf to give other
reviewers a chance at it. The original did not make it to the mailing
list since it contained a rather large binary resource Karthik
accidentally included in the commit (which has been stripped from
this re-send).

On Sun, Jan 31, 2016 at 11:12:54PM +0530, Karthik Nayak wrote:
> Introduce remote_ref_atom_parser() which will parse the '%(upstream)'
> and '%(push)' atoms and store information into the 'used_atom'
> structure based on the modifiers used along with the corresponding
> atom.
> 
> Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
>  ref-filter.c  | 103 ++++++++++++++++++++++++++++++++++------------------------
>  test-fake-ssh | Bin 0 -> 4668264 bytes
>  2 files changed, 61 insertions(+), 42 deletions(-)
>  create mode 100755 test-fake-ssh
> 
> diff --git a/ref-filter.c b/ref-filter.c
> index 58d433f..99c152d 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -37,6 +37,8 @@ static struct used_atom {
>  	union {
>  		char color[COLOR_MAXLEN];
>  		struct align align;
> +		enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }
> +			remote_ref;
>  	} u;
>  } *used_atom;
>  static int used_atom_cnt, need_tagged, need_symref;
> @@ -50,6 +52,20 @@ static void color_atom_parser(struct used_atom *atom, const char *color_value)
>  		die(_("invalid color value: %s"), atom->u.color);
>  }
>  
> +static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
> +{
> +	if (!arg) {
> +		atom->u.remote_ref = RR_NORMAL;
> +	} else if (!strcmp(arg, "short"))
> +		atom->u.remote_ref = RR_SHORTEN;
> +	else if (!strcmp(arg, "track"))
> +		atom->u.remote_ref = RR_TRACK;
> +	else if (!strcmp(arg, "trackshort"))
> +		atom->u.remote_ref = RR_TRACKSHORT;
> +	else
> +		die(_("unrecognized format: %%(%s)"), atom->name);
> +}
> +
>  static align_type parse_align_position(const char *s)
>  {
>  	if (!strcmp(s, "right"))
> @@ -132,8 +148,8 @@ static struct {
>  	{ "subject" },
>  	{ "body" },
>  	{ "contents" },
> -	{ "upstream" },
> -	{ "push" },
> +	{ "upstream", FIELD_STR, remote_ref_atom_parser },
> +	{ "push", FIELD_STR, remote_ref_atom_parser },
>  	{ "symref" },
>  	{ "flag" },
>  	{ "HEAD" },
> @@ -839,6 +855,43 @@ static const char *strip_ref_components(const char *refname, const char *nr_arg)
>  	return start;
>  }
>  
> +static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
> +				    struct branch *branch, const char **s)
> +{
> +	int num_ours, num_theirs;
> +	if (atom->u.remote_ref == RR_SHORTEN)
> +		*s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);
> +	else if (atom->u.remote_ref == RR_TRACK) {
> +		if (stat_tracking_info(branch, &num_ours,
> +				       &num_theirs, NULL))
> +			return;
> +
> +		if (!num_ours && !num_theirs)
> +			*s = "";
> +		else if (!num_ours)
> +			*s = xstrfmt("[behind %d]", num_theirs);
> +		else if (!num_theirs)
> +			*s = xstrfmt("[ahead %d]", num_ours);
> +		else
> +			*s = xstrfmt("[ahead %d, behind %d]",
> +				     num_ours, num_theirs);
> +	} else if (atom->u.remote_ref == RR_TRACKSHORT) {
> +		if (stat_tracking_info(branch, &num_ours,
> +				       &num_theirs, NULL))
> +			return;
> +
> +		if (!num_ours && !num_theirs)
> +			*s = "=";
> +		else if (!num_ours)
> +			*s = "<";
> +		else if (!num_theirs)
> +			*s = ">";
> +		else
> +			*s = "<>";
> +	} else /* RR_NORMAL */
> +		*s = refname;
> +}
> +
>  /*
>   * Parse the object referred by ref, and grab needed value.
>   */
> @@ -890,8 +943,9 @@ static void populate_value(struct ref_array_item *ref)
>  			branch = branch_get(branch_name);
>  
>  			refname = branch_get_upstream(branch, NULL);
> -			if (!refname)
> -				continue;
> +			if (refname)
> +				fill_remote_ref_details(atom, refname, branch, &v->s);
> +			continue;
>  		} else if (starts_with(name, "push")) {
>  			const char *branch_name;
>  			if (!skip_prefix(ref->refname, "refs/heads/",
> @@ -902,6 +956,8 @@ static void populate_value(struct ref_array_item *ref)
>  			refname = branch_get_push(branch, NULL);
>  			if (!refname)
>  				continue;
> +			fill_remote_ref_details(atom, refname, branch, &v->s);
> +			continue;
>  		} else if (starts_with(name, "color:")) {
>  			v->s = atom->u.color;
>  			continue;
> @@ -943,7 +999,6 @@ static void populate_value(struct ref_array_item *ref)
>  
>  		formatp = strchr(name, ':');
>  		if (formatp) {
> -			int num_ours, num_theirs;
>  			const char *arg;
>  
>  			formatp++;
> @@ -952,43 +1007,7 @@ static void populate_value(struct ref_array_item *ref)
>  						      warn_ambiguous_refs);
>  			else if (skip_prefix(formatp, "strip=", &arg))
>  				refname = strip_ref_components(refname, arg);
> -			else if (!strcmp(formatp, "track") &&
> -				 (starts_with(name, "upstream") ||
> -				  starts_with(name, "push"))) {
> -
> -				if (stat_tracking_info(branch, &num_ours,
> -						       &num_theirs, NULL))
> -					continue;
> -
> -				if (!num_ours && !num_theirs)
> -					v->s = "";
> -				else if (!num_ours)
> -					v->s = xstrfmt("[behind %d]", num_theirs);
> -				else if (!num_theirs)
> -					v->s = xstrfmt("[ahead %d]", num_ours);
> -				else
> -					v->s = xstrfmt("[ahead %d, behind %d]",
> -						       num_ours, num_theirs);
> -				continue;
> -			} else if (!strcmp(formatp, "trackshort") &&
> -				   (starts_with(name, "upstream") ||
> -				    starts_with(name, "push"))) {
> -				assert(branch);
> -
> -				if (stat_tracking_info(branch, &num_ours,
> -							&num_theirs, NULL))
> -					continue;
> -
> -				if (!num_ours && !num_theirs)
> -					v->s = "=";
> -				else if (!num_ours)
> -					v->s = "<";
> -				else if (!num_theirs)
> -					v->s = ">";
> -				else
> -					v->s = "<>";
> -				continue;
> -			} else
> +			else
>  				die("unknown %.*s format %s",
>  				    (int)(formatp - name), name, formatp);
>  		}

Re: [PATCH v4 10/12] ref-filter: introduce remote_ref_atom_parser()

[Karthik Nayak]

On Tue, Feb 2, 2016 at 6:29 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> This is a re-send of patch 10/12 on Karthik's behalf to give other
> reviewers a chance at it. The original did not make it to the mailing
> list since it contained a rather large binary resource Karthik
> accidentally included in the commit (which has been stripped from
> this re-send).
>

Thanks a lot, wondering how that happened.

> On Sun, Jan 31, 2016 at 11:12:54PM +0530, Karthik Nayak wrote:
>> Introduce remote_ref_atom_parser() which will parse the '%(upstream)'
>> and '%(push)' atoms and store information into the 'used_atom'
>> structure based on the modifiers used along with the corresponding
>> atom.
>>
>> Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
>> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>>  ref-filter.c  | 103 ++++++++++++++++++++++++++++++++++------------------------
>>  test-fake-ssh | Bin 0 -> 4668264 bytes
>>  2 files changed, 61 insertions(+), 42 deletions(-)
>>  create mode 100755 test-fake-ssh
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 58d433f..99c152d 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -37,6 +37,8 @@ static struct used_atom {
>>       union {
>>               char color[COLOR_MAXLEN];
>>               struct align align;
>> +             enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }
>> +                     remote_ref;
>>       } u;
>>  } *used_atom;
>>  static int used_atom_cnt, need_tagged, need_symref;
>> @@ -50,6 +52,20 @@ static void color_atom_parser(struct used_atom *atom, const char *color_value)
>>               die(_("invalid color value: %s"), atom->u.color);
>>  }
>>
>> +static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
>> +{
>> +     if (!arg) {
>> +             atom->u.remote_ref = RR_NORMAL;
>> +     } else if (!strcmp(arg, "short"))
>> +             atom->u.remote_ref = RR_SHORTEN;
>> +     else if (!strcmp(arg, "track"))
>> +             atom->u.remote_ref = RR_TRACK;
>> +     else if (!strcmp(arg, "trackshort"))
>> +             atom->u.remote_ref = RR_TRACKSHORT;
>> +     else
>> +             die(_("unrecognized format: %%(%s)"), atom->name);
>> +}
>> +
>>  static align_type parse_align_position(const char *s)
>>  {
>>       if (!strcmp(s, "right"))
>> @@ -132,8 +148,8 @@ static struct {
>>       { "subject" },
>>       { "body" },
>>       { "contents" },
>> -     { "upstream" },
>> -     { "push" },
>> +     { "upstream", FIELD_STR, remote_ref_atom_parser },
>> +     { "push", FIELD_STR, remote_ref_atom_parser },
>>       { "symref" },
>>       { "flag" },
>>       { "HEAD" },
>> @@ -839,6 +855,43 @@ static const char *strip_ref_components(const char *refname, const char *nr_arg)
>>       return start;
>>  }
>>
>> +static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
>> +                                 struct branch *branch, const char **s)
>> +{
>> +     int num_ours, num_theirs;
>> +     if (atom->u.remote_ref == RR_SHORTEN)
>> +             *s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);
>> +     else if (atom->u.remote_ref == RR_TRACK) {
>> +             if (stat_tracking_info(branch, &num_ours,
>> +                                    &num_theirs, NULL))
>> +                     return;
>> +
>> +             if (!num_ours && !num_theirs)
>> +                     *s = "";
>> +             else if (!num_ours)
>> +                     *s = xstrfmt("[behind %d]", num_theirs);
>> +             else if (!num_theirs)
>> +                     *s = xstrfmt("[ahead %d]", num_ours);
>> +             else
>> +                     *s = xstrfmt("[ahead %d, behind %d]",
>> +                                  num_ours, num_theirs);
>> +     } else if (atom->u.remote_ref == RR_TRACKSHORT) {
>> +             if (stat_tracking_info(branch, &num_ours,
>> +                                    &num_theirs, NULL))
>> +                     return;
>> +
>> +             if (!num_ours && !num_theirs)
>> +                     *s = "=";
>> +             else if (!num_ours)
>> +                     *s = "<";
>> +             else if (!num_theirs)
>> +                     *s = ">";
>> +             else
>> +                     *s = "<>";
>> +     } else /* RR_NORMAL */
>> +             *s = refname;
>> +}
>> +
>>  /*
>>   * Parse the object referred by ref, and grab needed value.
>>   */
>> @@ -890,8 +943,9 @@ static void populate_value(struct ref_array_item *ref)
>>                       branch = branch_get(branch_name);
>>
>>                       refname = branch_get_upstream(branch, NULL);
>> -                     if (!refname)
>> -                             continue;
>> +                     if (refname)
>> +                             fill_remote_ref_details(atom, refname, branch, &v->s);
>> +                     continue;
>>               } else if (starts_with(name, "push")) {
>>                       const char *branch_name;
>>                       if (!skip_prefix(ref->refname, "refs/heads/",
>> @@ -902,6 +956,8 @@ static void populate_value(struct ref_array_item *ref)
>>                       refname = branch_get_push(branch, NULL);
>>                       if (!refname)
>>                               continue;
>> +                     fill_remote_ref_details(atom, refname, branch, &v->s);
>> +                     continue;
>>               } else if (starts_with(name, "color:")) {
>>                       v->s = atom->u.color;
>>                       continue;
>> @@ -943,7 +999,6 @@ static void populate_value(struct ref_array_item *ref)
>>
>>               formatp = strchr(name, ':');
>>               if (formatp) {
>> -                     int num_ours, num_theirs;
>>                       const char *arg;
>>
>>                       formatp++;
>> @@ -952,43 +1007,7 @@ static void populate_value(struct ref_array_item *ref)
>>                                                     warn_ambiguous_refs);
>>                       else if (skip_prefix(formatp, "strip=", &arg))
>>                               refname = strip_ref_components(refname, arg);
>> -                     else if (!strcmp(formatp, "track") &&
>> -                              (starts_with(name, "upstream") ||
>> -                               starts_with(name, "push"))) {
>> -
>> -                             if (stat_tracking_info(branch, &num_ours,
>> -                                                    &num_theirs, NULL))
>> -                                     continue;
>> -
>> -                             if (!num_ours && !num_theirs)
>> -                                     v->s = "";
>> -                             else if (!num_ours)
>> -                                     v->s = xstrfmt("[behind %d]", num_theirs);
>> -                             else if (!num_theirs)
>> -                                     v->s = xstrfmt("[ahead %d]", num_ours);
>> -                             else
>> -                                     v->s = xstrfmt("[ahead %d, behind %d]",
>> -                                                    num_ours, num_theirs);
>> -                             continue;
>> -                     } else if (!strcmp(formatp, "trackshort") &&
>> -                                (starts_with(name, "upstream") ||
>> -                                 starts_with(name, "push"))) {
>> -                             assert(branch);
>> -
>> -                             if (stat_tracking_info(branch, &num_ours,
>> -                                                     &num_theirs, NULL))
>> -                                     continue;
>> -
>> -                             if (!num_ours && !num_theirs)
>> -                                     v->s = "=";
>> -                             else if (!num_ours)
>> -                                     v->s = "<";
>> -                             else if (!num_theirs)
>> -                                     v->s = ">";
>> -                             else
>> -                                     v->s = "<>";
>> -                             continue;
>> -                     } else
>> +                     else
>>                               die("unknown %.*s format %s",
>>                                   (int)(formatp - name), name, formatp);
>>               }



-- 
Regards,
Karthik Nayak

Re: [PATCH v4 00/12] ref-filter: use parsing functions

[Karthik Nayak]

On Tue, Feb 2, 2016 at 6:07 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, Feb 1, 2016 at 5:25 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> This series cleans up populate_value() in ref-filter, by moving out
>>> the parsing part of atoms to separate parsing functions. This ensures
>>> that parsing is only done once and also improves the modularity of the
>>> code.
>>>
>>> v1: http://thread.gmane.org/gmane.comp.version-control.git/281180
>>> v2: http://thread.gmane.org/gmane.comp.version-control.git/282563
>>> v3: http://thread.gmane.org/gmane.comp.version-control.git/283350
>>>
>>> Changes:
>>> * The parsing functions now take the arguments of the atom as
>>> function parameteres, instead of parsing it inside the fucntion.
>>> * Rebased on top of pu:jk/list-tag-2.7-regression
>>> * In strbuf use a copylen variable rather than using multiplication
>>> to perform a logical operation.
>>> * Code movement for easier review and general improvement.
>>> * Use COLOR_MAXLEN as the maximum size for the color variable.
>>> * Small code changes.
>>> * Documentation changes.
>>> * Fixed incorrect style of test (t6302).
>>>
>>> Karthik Nayak (12):
>>>   strbuf: introduce strbuf_split_str_omit_term()
>>>   ref-filter: use strbuf_split_str_omit_term()
>>>   ref-filter: bump 'used_atom' and related code to the top
>>>   ref-filter: introduce struct used_atom
>>>   ref-filter: introduce parsing functions for each valid atom
>>>   ref-filter: introduce color_atom_parser()
>>>   ref-filter: introduce parse_align_position()
>>>   ref-filter: introduce align_atom_parser()
>>>   ref-filter: align: introduce long-form syntax
>>>   ref-filter: introduce remote_ref_atom_parser()
>>>   ref-filter: introduce contents_atom_parser()
>>>   ref-filter: introduce objectname_atom_parser()
>>
>> Hmm, 10/12 didn't make it to the list?
>
> Not surprising. Somehow, Karthik did git-add on the compiled
> test-fake-ssh before committing, so the patch sent to the list
> contains an rather huge binary resource. I did receive it since I was
> a direct addressee of the email; I'll reply to the message on-list
> without modifying anything (other than stripping out the binary
> resource) so that other reviewers get a chance to see it.

Thanks Eric, i didn't notice doing that.

-- 
Regards,
Karthik Nayak

Re: [PATCH v4 03/12] ref-filter: bump 'used_atom' and related code to the top

[Karthik Nayak]

On Tue, Feb 2, 2016 at 3:52 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Bump code to the top for usage in further patches.
>> ---
>
> Sign-off?

Shall reply with patch, missed that somehow.

>
>>  ref-filter.c | 30 +++++++++++++++---------------
>>  1 file changed, 15 insertions(+), 15 deletions(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 38f38d4..c3a8372 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -16,6 +16,21 @@
>>
>>  typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
>>
>> +/*
>> + * An atom is a valid field atom listed below, possibly prefixed with
>> + * a "*" to denote deref_tag().
>> + *
>> + * We parse given format string and sort specifiers, and make a list
>> + * of properties that we need to extract out of objects.  ref_array_item
>> + * structure will hold an array of values extracted that can be
>> + * indexed with the "atom number", which is an index into this
>> + * array.
>> + */
>> +static const char **used_atom;
>> +static cmp_type *used_atom_type;
>> +static int used_atom_cnt, need_tagged, need_symref;
>> +static int need_color_reset_at_eol;
>> +
>>  static struct {
>>       const char *name;
>>       cmp_type cmp_type;
>> @@ -92,21 +107,6 @@ struct atom_value {
>>  };
>>
>>  /*
>> - * An atom is a valid field atom listed above, possibly prefixed with
>> - * a "*" to denote deref_tag().
>> - *
>> - * We parse given format string and sort specifiers, and make a list
>> - * of properties that we need to extract out of objects.  ref_array_item
>> - * structure will hold an array of values extracted that can be
>> - * indexed with the "atom number", which is an index into this
>> - * array.
>> - */
>> -static const char **used_atom;
>> -static cmp_type *used_atom_type;
>> -static int used_atom_cnt, need_tagged, need_symref;
>> -static int need_color_reset_at_eol;
>> -
>> -/*
>>   * Used to parse format string and sort specifiers
>>   */
>>  int parse_ref_filter_atom(const char *atom, const char *ep)



-- 
Regards,
Karthik Nayak

[PATCH v4 03/12] ref-filter: bump 'used_atom' and related code to the top

[Karthik Nayak]

Bump code to the top for usage in further patches.

Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
---
 ref-filter.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 38f38d4..c3a8372 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -16,6 +16,21 @@
 
 typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 
+/*
+ * An atom is a valid field atom listed below, possibly prefixed with
+ * a "*" to denote deref_tag().
+ *
+ * We parse given format string and sort specifiers, and make a list
+ * of properties that we need to extract out of objects.  ref_array_item
+ * structure will hold an array of values extracted that can be
+ * indexed with the "atom number", which is an index into this
+ * array.
+ */
+static const char **used_atom;
+static cmp_type *used_atom_type;
+static int used_atom_cnt, need_tagged, need_symref;
+static int need_color_reset_at_eol;
+
 static struct {
 	const char *name;
 	cmp_type cmp_type;
@@ -92,21 +107,6 @@ struct atom_value {
 };
 
 /*
- * An atom is a valid field atom listed above, possibly prefixed with
- * a "*" to denote deref_tag().
- *
- * We parse given format string and sort specifiers, and make a list
- * of properties that we need to extract out of objects.  ref_array_item
- * structure will hold an array of values extracted that can be
- * indexed with the "atom number", which is an index into this
- * array.
- */
-static const char **used_atom;
-static cmp_type *used_atom_type;
-static int used_atom_cnt, need_tagged, need_symref;
-static int need_color_reset_at_eol;
-
-/*
  * Used to parse format string and sort specifiers
  */
 int parse_ref_filter_atom(const char *atom, const char *ep)
-- 
2.7.0

Re: [PATCH v4 05/12] ref-filter: introduce parsing functions for each valid atom

[Eric Sunshine]

On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Parsing atoms is done in populate_value(), this is repetitive and
> hence expensive. Introduce a parsing function which would let us parse
> atoms beforehand and store the required details into the 'used_atom'
> structure for further usage.
>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -36,6 +36,7 @@ static int need_color_reset_at_eol;
>  static struct {
>         const char *name;
>         cmp_type cmp_type;
> +       void (*parser)(struct used_atom *atom, const char *arg);

It's a little bit weird to pass in 'arg' as an additional argument
considering that the parser already has access to the same information
via the atom's 'name' field. I guess you're doing it as a convenience
so that parsers don't have to do the strchr(':') or memchr(':')
themselves (and because parse_ref_filter_atom() already has the
information at hand), right? A typical parser interested in a
(possibly optional) argument currently looks like this:

    void parse_foo(struct used_atom *a, const char *arg) {
        if (!arg)
            /* default behavior: arg absent */
        else
            /* process arg */
    }

That doesn't change much if you drop the 'arg' argument:

    void parse_foo(struct used_atom *a) {
        const char *arg = strchr(a->name, ':')
        if (!arg)
            /* default behavior: arg absent */
        else
            /* process arg */
    }

It does mean a very minimal amount of duplicated code (the single
strchr() line per parser), but it also would remove a bit of the
complexity which this patch adds to parse_ref_filter_atom(). So, I
dunno...

>  } valid_atom[] = {
>         { "refname" },
>         { "objecttype" },
> @@ -138,10 +140,9 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
>                  * shouldn't be used for checking against the valid_atom
>                  * table.
>                  */
> -               const char *formatp = strchr(sp, ':');
> -               if (!formatp || ep < formatp)
> -                       formatp = ep;
> -               if (len == formatp - sp && !memcmp(valid_atom[i].name, sp, len))
> +               arg = memchr(sp, ':', ep - sp);

Why this change from strchr() to memchr()? I understand that you're
taking advantage of the fact that you know the extent of the string
via 'sp' and 'ep', however, was the original strchr() doing extra
work? Even if this change is desirable, it seems somewhat unrelated to
the overall purpose of this patch, thus might deserves its own.

Aside from that, although the "expensive" strchr() / memchr() resides
within the loop, it will always return the same value since it doesn't
depend upon any condition local to the loop. This implies that it
ought to be hoisted out of the loop. (This problem is not new to this
patch; it's already present in the existing code.)

> +               if ((!arg || len == arg - sp) &&
> +                   !memcmp(valid_atom[i].name, sp, len))
>                         break;
>         }
>
> @@ -154,6 +155,10 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
>         REALLOC_ARRAY(used_atom, used_atom_cnt);
>         used_atom[at].name = xmemdupz(atom, ep - atom);
>         used_atom[at].type = valid_atom[i].cmp_type;
> +       if (arg)
> +               arg = used_atom[at].name + (arg - atom) + 1;

This is a harder to understand than it ought to be because it's
difficult to tell at first glance that you don't actually care about
'arg' in relation to the original incoming string, but instead use it
only to compute an offset into the string which is ultimately stored
in the newly allocated used_atom[]. Re-using 'arg' for a different
purpose (in a manner of speaking) confuses the issue further.

The intention might be easier to follow if you made it clear that you
were interested in the *offset* of the argument in the string, rather
than a pointer into the incoming string which you ultimately don't
use. A variable named 'arg_offset' might go a long way toward
clarifying this intention.

> +       if (valid_atom[i].parser)
> +               valid_atom[i].parser(&used_atom[at], arg);
>         if (*atom == '*')
>                 need_tagged = 1;
>         if (!strcmp(used_atom[at].name, "symref"))
> --
> 2.7.0

Re: [PATCH v4 05/12] ref-filter: introduce parsing functions for each valid atom

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> @@ -138,10 +140,9 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
>>                  * shouldn't be used for checking against the valid_atom
>>                  * table.
>>                  */
>> -               const char *formatp = strchr(sp, ':');
>> -               if (!formatp || ep < formatp)
>> -                       formatp = ep;
>> -               if (len == formatp - sp && !memcmp(valid_atom[i].name, sp, len))
>> +               arg = memchr(sp, ':', ep - sp);
>
> Why this change from strchr() to memchr()? I understand that you're
> taking advantage of the fact that you know the extent of the string
> via 'sp' and 'ep', however, was the original strchr() doing extra
> work? Even if this change is desirable, it seems somewhat unrelated to
> the overall purpose of this patch, thus might deserves its own.

I think the original strchr() is a bug.  If you are given a
substring as a range, you shouldn't be allowing strchr() to go
beyond ep to find a NUL that may or may not exist.  That is not a
performance thing, but more about the best practice to ensure
correctness.  The caller of this function may not have such a
problem, but imagine the case where the bytes beyond ep did not have
any NUL and there is an unmapped page after that.

Re: [PATCH v4 06/12] ref-filter: introduce color_atom_parser()

[Eric Sunshine]

On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Introduce color_atom_parser() which will parse a "color" atom and
> store its color in the "used_atom" structure for further usage in
> populate_value().
>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -29,10 +29,21 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
>  static struct used_atom {
>         const char *name;
>         cmp_type type;
> +       union {
> +               char color[COLOR_MAXLEN];
> +       } u;
>  } *used_atom;
>  static int used_atom_cnt, need_tagged, need_symref;
>  static int need_color_reset_at_eol;
>
> +static void color_atom_parser(struct used_atom *atom, const char *color_value)
> +{
> +       if (!color_value)
> +               die(_("expected format: %%(color:<color>)"));
> +       if (color_parse(color_value, atom->u.color) < 0)
> +               die(_("invalid color value: %s"), atom->u.color);

Shouldn't this be:

    die(_("invalid color value: %s"), color_value);

?

> +}

Re: [PATCH v4 08/12] ref-filter: introduce align_atom_parser()

[Eric Sunshine]

On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Introduce align_atom_parser() which will parse an 'align' atom and
> store the required alignment position and width in the 'used_atom'
> structure for further usage in populate_value().
>
> Since this patch removes the last usage of match_atom_name(), remove
> the function from ref-filter.c.
>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -55,6 +61,37 @@ static align_type parse_align_position(const char *s)
> +static void align_atom_parser(struct used_atom *atom, const char *arg)
> +{
> +       struct align *align = &atom->u.align;
> +       struct strbuf **s, **to_free;
> +       unsigned int width = ~0U;
> +
> +       if (!arg)
> +               die(_("expected format: %%(align:<width>,<position>)"));
> +       s = to_free = strbuf_split_str_omit_term(arg, ',', 0);
> +
> +       align->position = ALIGN_LEFT;
> +
> +       while (*s) {
> +               int position;
> +               arg = s[0]->buf;

It's confusing to see 'arg' being re-used here for a different
purpose, and leads the reader to wonder if this is done because the
s[0]->buf might be needed outside the loop (when, in fact, it isn't).
It would be better to declare a new variable here in the scope of the
'while' loop to hold this value.

(I might have named the result of the strbuf split 'tokens' or even
short-and-sweet 'v' -- for vector -- and then used 's' for the name of
the new variable here in the 'while' loop, but these name suggestions
aren't particularly important; it is important to declare a new
variable here -- whatever you name it -- rather than re-using 'arg'.)

> +
> +               if (!strtoul_ui(arg, 10, &width))
> +                       ;
> +               else if ((position = parse_align_position(arg)) >= 0)
> +                       align->position = position;
> +               else
> +                       die(_("unrecognized %%(align) argument: %s"), arg);
> +               s++;
> +       }
> +
> +       if (width == ~0U)
> +               die(_("positive width expected with the %%(align) atom"));
> +       align->width = width;
> +       strbuf_list_free(to_free);
> +}

Re: [PATCH v4 09/12] ref-filter: align: introduce long-form syntax

[Eric Sunshine]

On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Introduce optional prefixes "width=" and "position=" for the align atom
> so that the atom can be used as "%(align:width=<width>,position=<position>)".
>
> Add Documentation and tests for the same.
>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> @@ -133,6 +133,48 @@ test_expect_success 'right alignment' '
> +cat >expect <<-\EOF
> +|       refname is refs/heads/master       |refs/heads/master
> +|        refname is refs/heads/side        |refs/heads/side
> +|         refname is refs/odd/spot         |refs/odd/spot
> +|     refname is refs/tags/double-tag      |refs/tags/double-tag
> +|        refname is refs/tags/four         |refs/tags/four
> +|         refname is refs/tags/one         |refs/tags/one
> +|     refname is refs/tags/signed-tag      |refs/tags/signed-tag
> +|        refname is refs/tags/three        |refs/tags/three
> +|         refname is refs/tags/two         |refs/tags/two
> +EOF
> +
> +test_align_permutations() {
> +       while read -r option
> +       do
> +               test_expect_success "align:$option" '
> +               git for-each-ref --format="|%(align:$option)refname is %(refname)%(end)|%(refname)" >actual &&
> +               test_cmp expect actual
> +               '

I think I mentioned this in the last round: The two lines following
test_expect_success() are actually the test body, thus should be
indented one more level. (Not necessarily worth a re-roll, though...)

> +       done
> +}
> +
> +test_align_permutations <<-\EOF
> +       middle,42
> +       42,middle
> +       position=middle,42
> +       42,position=middle
> +       middle,width=42
> +       width=42,middle
> +       position=middle,width=42
> +       width=42,position=middle
> +EOF
> +
> +# Last one wins (silently) when multiple arguments of the same type are given
> +
> +test_align_permutations <<-\EOF
> +       32,width=42,middle
> +       width=30,42,middle
> +       width=42,position=right,middle
> +       42,right,position=middle
> +EOF
> +
>  # Individual atoms inside %(align:...) and %(end) must not be quoted.
>
>  test_expect_success 'alignment with format quote' "
> --
> 2.7.0

Re: [PATCH v4 10/12] ref-filter: introduce remote_ref_atom_parser()

[Eric Sunshine]

On Sun, Jan 31, 2016 at 11:12:54PM +0530, Karthik Nayak wrote:
> Introduce remote_ref_atom_parser() which will parse the '%(upstream)'
> and '%(push)' atoms and store information into the 'used_atom'
> structure based on the modifiers used along with the corresponding
> atom.
> 
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -50,6 +52,20 @@ static void color_atom_parser(struct used_atom *atom, const char *color_value)
> +static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
> +{
> +	if (!arg) {
> +		atom->u.remote_ref = RR_NORMAL;
> +	} else if (!strcmp(arg, "short"))

Style: drop unnecessary braces

> +		atom->u.remote_ref = RR_SHORTEN;
> +	else if (!strcmp(arg, "track"))
> +		atom->u.remote_ref = RR_TRACK;
> +	else if (!strcmp(arg, "trackshort"))
> +		atom->u.remote_ref = RR_TRACKSHORT;
> +	else
> +		die(_("unrecognized format: %%(%s)"), atom->name);
> +}

Re: [PATCH v4 11/12] ref-filter: introduce contents_atom_parser()

[Eric Sunshine]

On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Introduce contents_atom_parser() which will parse the '%(contents)'
> atom and store information into the 'used_atom' structure based on the
> modifiers used along with the atom. Also introduce body_atom_parser()
> and subject_atom_parser() for parsing atoms '%(body)' and '%(subject)'
> respectively.

These latter two parsers are conceptually distinct from introduction
of the %(contents) parser, thus could be done in a follow-on patch or
two (though I don't care strongly enough to insist upon it).

> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -66,6 +70,38 @@ static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
> +static void body_atom_parser(struct used_atom *atom, const char *arg)
> +{
> +       if (arg)
> +               die("%%(body) atom does not take arguments");

None of the other error messages bothers saying "atom" literally
following a %(foo). For consistency, this likely should say merely:

    %(body) does not take arguments

> +       atom->u.contents.option = C_BODY_DEP;
> +}
> +
> +static void subject_atom_parser(struct used_atom *atom, const char *arg)
> +{
> +       if (arg)
> +               die("%%(subject) atom does not take arguments");

Ditto.

> +       atom->u.contents.option = C_SUB;
> +}
> @@ -733,19 +763,15 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
>
>         for (i = 0; i < used_atom_cnt; i++) {
>                 const char *name = used_atom[i].name;
> +               struct used_atom *atom = &used_atom[i];

Not a big deal, but if you re-order these two lines, then the second,
which extracts name, could do so from the variable declared by the
first:

    struct used_atom *atom = &used_atom[i];
    const char *name = atom->name;

>                 struct atom_value *v = &val[i];
> -               const char *valp = NULL;
>                 if (!!deref != (*name == '*'))
>                         continue;
>                 if (deref)
>                         name++;
>                 if (strcmp(name, "subject") &&
>                     strcmp(name, "body") &&
> -                   strcmp(name, "contents") &&
> -                   strcmp(name, "contents:subject") &&
> -                   strcmp(name, "contents:body") &&
> -                   strcmp(name, "contents:signature") &&
> -                   !starts_with(name, "contents:lines="))
> +                   !starts_with(name, "contents"))
>                         continue;

This changes behavior in that it will also now match
"contentsanything", whereas the original was much more strict. Is that
desirable? (Genuine question.)

>                 if (!subpos)
>                         find_subpos(buf, sz,

Re: [PATCH v4 00/12] ref-filter: use parsing functions

[Eric Sunshine]

Karthik Nayak <karthik.188@gmail.com> writes:
> This series cleans up populate_value() in ref-filter, by moving out
> the parsing part of atoms to separate parsing functions. This ensures
> that parsing is only done once and also improves the modularity of the
> code.
>
> v1: http://thread.gmane.org/gmane.comp.version-control.git/281180
> v2: http://thread.gmane.org/gmane.comp.version-control.git/282563
> v3: http://thread.gmane.org/gmane.comp.version-control.git/283350
>
> Changes:
> * The parsing functions now take the arguments of the atom as
> function parameteres, instead of parsing it inside the fucntion.
> * Rebased on top of pu:jk/list-tag-2.7-regression
> * In strbuf use a copylen variable rather than using multiplication
> to perform a logical operation.
> * Code movement for easier review and general improvement.
> * Use COLOR_MAXLEN as the maximum size for the color variable.
> * Small code changes.
> * Documentation changes.
> * Fixed incorrect style of test (t6302).

v4 is a nice improvement. With the retirement of match_atom_name() and
its misleading and confusing use in the parsers, overall the parsers
are now more concise, straightforward, and easier (in fact, dead
simple) to comprehend.

As most of my review comments this round were relatively minor, and
there don't seem to be any major problems, hopefully this series will
wrap up with v5. Thanks.

Re: [PATCH v4 05/12] ref-filter: introduce parsing functions for each valid atom

[Karthik Nayak]

On Thu, Feb 4, 2016 at 3:49 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Parsing atoms is done in populate_value(), this is repetitive and
>> hence expensive. Introduce a parsing function which would let us parse
>> atoms beforehand and store the required details into the 'used_atom'
>> structure for further usage.
>>
>> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -36,6 +36,7 @@ static int need_color_reset_at_eol;
>>  static struct {
>>         const char *name;
>>         cmp_type cmp_type;
>> +       void (*parser)(struct used_atom *atom, const char *arg);
>
> It's a little bit weird to pass in 'arg' as an additional argument
> considering that the parser already has access to the same information
> via the atom's 'name' field. I guess you're doing it as a convenience
> so that parsers don't have to do the strchr(':') or memchr(':')
> themselves (and because parse_ref_filter_atom() already has the
> information at hand), right? A typical parser interested in a
> (possibly optional) argument currently looks like this:
>
>     void parse_foo(struct used_atom *a, const char *arg) {
>         if (!arg)
>             /* default behavior: arg absent */
>         else
>             /* process arg */
>     }
>
> That doesn't change much if you drop the 'arg' argument:
>
>     void parse_foo(struct used_atom *a) {
>         const char *arg = strchr(a->name, ':')
>         if (!arg)
>             /* default behavior: arg absent */
>         else
>             /* process arg */
>     }
>
> It does mean a very minimal amount of duplicated code (the single
> strchr() line per parser), but it also would remove a bit of the
> complexity which this patch adds to parse_ref_filter_atom(). So, I
> dunno...
>

This change is introduced as a result of he suggestion given from Junio[1].
Although we're adding a little complexity to parse_ref_filter_atom() I feel its
justified by the interface provided as you mentioned. This ensures that parser
functions don't need to implement their own methods for getting the arguments
and can rely on being provided the arguments to them.

1: http://article.gmane.org/gmane.comp.version-control.git/283499

>>  } valid_atom[] = {
>>         { "refname" },
>>         { "objecttype" },
>> @@ -138,10 +140,9 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
>>                  * shouldn't be used for checking against the valid_atom
>>                  * table.
>>                  */
>> -               const char *formatp = strchr(sp, ':');
>> -               if (!formatp || ep < formatp)
>> -                       formatp = ep;
>> -               if (len == formatp - sp && !memcmp(valid_atom[i].name, sp, len))
>> +               arg = memchr(sp, ':', ep - sp);
>
> Why this change from strchr() to memchr()? I understand that you're
> taking advantage of the fact that you know the extent of the string
> via 'sp' and 'ep', however, was the original strchr() doing extra
> work? Even if this change is desirable, it seems somewhat unrelated to
> the overall purpose of this patch, thus might deserves its own.
>

I'm thinking I'll make a patch for that separately. i.e remove strchr()
and introduce memchr() outside the loop.

> Aside from that, although the "expensive" strchr() / memchr() resides
> within the loop, it will always return the same value since it doesn't
> depend upon any condition local to the loop. This implies that it
> ought to be hoisted out of the loop. (This problem is not new to this
> patch; it's already present in the existing code.)
>

Yes, makes sense.

>> +               if ((!arg || len == arg - sp) &&
>> +                   !memcmp(valid_atom[i].name, sp, len))
>>                         break;
>>         }
>>
>> @@ -154,6 +155,10 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
>>         REALLOC_ARRAY(used_atom, used_atom_cnt);
>>         used_atom[at].name = xmemdupz(atom, ep - atom);
>>         used_atom[at].type = valid_atom[i].cmp_type;
>> +       if (arg)
>> +               arg = used_atom[at].name + (arg - atom) + 1;
>
> This is a harder to understand than it ought to be because it's
> difficult to tell at first glance that you don't actually care about
> 'arg' in relation to the original incoming string, but instead use it
> only to compute an offset into the string which is ultimately stored
> in the newly allocated used_atom[]. Re-using 'arg' for a different
> purpose (in a manner of speaking) confuses the issue further.
>
> The intention might be easier to follow if you made it clear that you
> were interested in the *offset* of the argument in the string, rather
> than a pointer into the incoming string which you ultimately don't
> use. A variable named 'arg_offset' might go a long way toward
> clarifying this intention.
>

I hope you mean something like this.

if (arg) {
    int arg_offset;

    arg_offset = (arg - atom) + 1;
    arg = used_atom[at].name + arg_offset;
}


-- 
Regards,
Karthik Nayak

Re: [PATCH v4 05/12] ref-filter: introduce parsing functions for each valid atom

[Karthik Nayak]

On Sun, Jan 31, 2016 at 11:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> @@ -138,10 +140,9 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
>                  * shouldn't be used for checking against the valid_atom
>                  * table.
>                  */
> -               const char *formatp = strchr(sp, ':');
> -               if (!formatp || ep < formatp)
> -                       formatp = ep;
> -               if (len == formatp - sp && !memcmp(valid_atom[i].name, sp, len))
> +               arg = memchr(sp, ':', ep - sp);
> +               if ((!arg || len == arg - sp) &&
> +                   !memcmp(valid_atom[i].name, sp, len))
>                         break;
>         }
>

Also having a look at this, this breaks the previous error checking we
had at parse_ref_filter_atom().
e.g: git for-each-ref --format="%(refnameboo)" would not throw an error.

I think the code needs to be changed to:

-               if ((!arg || len == arg - sp) &&
+               if ((arg || len == ep - sp) &&
+                   (!arg || len == arg - sp) &&

-- 
Regards,
Karthik Nayak

Re: [PATCH v4 06/12] ref-filter: introduce color_atom_parser()

[Karthik Nayak]

On Fri, Feb 5, 2016 at 3:55 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Introduce color_atom_parser() which will parse a "color" atom and
>> store its color in the "used_atom" structure for further usage in
>> populate_value().
>>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -29,10 +29,21 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
>>  static struct used_atom {
>>         const char *name;
>>         cmp_type type;
>> +       union {
>> +               char color[COLOR_MAXLEN];
>> +       } u;
>>  } *used_atom;
>>  static int used_atom_cnt, need_tagged, need_symref;
>>  static int need_color_reset_at_eol;
>>
>> +static void color_atom_parser(struct used_atom *atom, const char *color_value)
>> +{
>> +       if (!color_value)
>> +               die(_("expected format: %%(color:<color>)"));
>> +       if (color_parse(color_value, atom->u.color) < 0)
>> +               die(_("invalid color value: %s"), atom->u.color);
>
> Shouldn't this be:
>
>     die(_("invalid color value: %s"), color_value);
>
> ?

Oops. You're right, it should.
Also the error is reported already in color_parse(...), so seems duplicated.

e.g.

git for-each-ref  --format="%(color:sfadf)%(align:middle,30)%(refname)%(end)"
error: invalid color value: sfadf
fatal: invalid color value: sfadf

What would be an ideal way around this?

-- 
Regards,
Karthik Nayak

Re: [PATCH v4 08/12] ref-filter: introduce align_atom_parser()

[Karthik Nayak]

On Fri, Feb 5, 2016 at 5:18 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Introduce align_atom_parser() which will parse an 'align' atom and
>> store the required alignment position and width in the 'used_atom'
>> structure for further usage in populate_value().
>>
>> Since this patch removes the last usage of match_atom_name(), remove
>> the function from ref-filter.c.
>>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -55,6 +61,37 @@ static align_type parse_align_position(const char *s)
>> +static void align_atom_parser(struct used_atom *atom, const char *arg)
>> +{
>> +       struct align *align = &atom->u.align;
>> +       struct strbuf **s, **to_free;
>> +       unsigned int width = ~0U;
>> +
>> +       if (!arg)
>> +               die(_("expected format: %%(align:<width>,<position>)"));
>> +       s = to_free = strbuf_split_str_omit_term(arg, ',', 0);
>> +
>> +       align->position = ALIGN_LEFT;
>> +
>> +       while (*s) {
>> +               int position;
>> +               arg = s[0]->buf;
>
> It's confusing to see 'arg' being re-used here for a different
> purpose, and leads the reader to wonder if this is done because the
> s[0]->buf might be needed outside the loop (when, in fact, it isn't).
> It would be better to declare a new variable here in the scope of the
> 'while' loop to hold this value.
>
> (I might have named the result of the strbuf split 'tokens' or even
> short-and-sweet 'v' -- for vector -- and then used 's' for the name of
> the new variable here in the 'while' loop, but these name suggestions
> aren't particularly important; it is important to declare a new
> variable here -- whatever you name it -- rather than re-using 'arg'.)
>

You're right, that is indeed confusing, I should stop reusing variables
and trying to micromanage.

I also like the naming scheme you suggested, so will stick to that.
Thanks.

-- 
Regards,
Karthik Nayak

Re: [PATCH v4 06/12] ref-filter: introduce color_atom_parser()

[Christian Couder]

On Sat, Feb 6, 2016 at 4:20 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Fri, Feb 5, 2016 at 3:55 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>> Introduce color_atom_parser() which will parse a "color" atom and
>>> store its color in the "used_atom" structure for further usage in
>>> populate_value().
>>>
>>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>>> ---
>>> diff --git a/ref-filter.c b/ref-filter.c
>>> @@ -29,10 +29,21 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
>>>  static struct used_atom {
>>>         const char *name;
>>>         cmp_type type;
>>> +       union {
>>> +               char color[COLOR_MAXLEN];
>>> +       } u;
>>>  } *used_atom;
>>>  static int used_atom_cnt, need_tagged, need_symref;
>>>  static int need_color_reset_at_eol;
>>>
>>> +static void color_atom_parser(struct used_atom *atom, const char *color_value)
>>> +{
>>> +       if (!color_value)
>>> +               die(_("expected format: %%(color:<color>)"));
>>> +       if (color_parse(color_value, atom->u.color) < 0)
>>> +               die(_("invalid color value: %s"), atom->u.color);
>>
>> Shouldn't this be:
>>
>>     die(_("invalid color value: %s"), color_value);
>>
>> ?
>
> Oops. You're right, it should.
> Also the error is reported already in color_parse(...), so seems duplicated.
>
> e.g.
>
> git for-each-ref  --format="%(color:sfadf)%(align:middle,30)%(refname)%(end)"
> error: invalid color value: sfadf
> fatal: invalid color value: sfadf
>
> What would be an ideal way around this?

Maybe it has already been discussed a lot and I missed the discussion,
but if possible the argument, the parameter or the atom itself might
just be ignored with a warning instead of dying when an atom argument,
format or parameter is not recognized, because in the next Git
versions we might want to add new arguments, formats and parameter and
it would be sad if old versions of Git die when those new things are
passed to them.

Re: [PATCH v4 09/12] ref-filter: align: introduce long-form syntax

[Karthik Nayak]

On Fri, Feb 5, 2016 at 5:30 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Introduce optional prefixes "width=" and "position=" for the align atom
>> so that the atom can be used as "%(align:width=<width>,position=<position>)".
>>
>> Add Documentation and tests for the same.
>>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
>> @@ -133,6 +133,48 @@ test_expect_success 'right alignment' '
>> +cat >expect <<-\EOF
>> +|       refname is refs/heads/master       |refs/heads/master
>> +|        refname is refs/heads/side        |refs/heads/side
>> +|         refname is refs/odd/spot         |refs/odd/spot
>> +|     refname is refs/tags/double-tag      |refs/tags/double-tag
>> +|        refname is refs/tags/four         |refs/tags/four
>> +|         refname is refs/tags/one         |refs/tags/one
>> +|     refname is refs/tags/signed-tag      |refs/tags/signed-tag
>> +|        refname is refs/tags/three        |refs/tags/three
>> +|         refname is refs/tags/two         |refs/tags/two
>> +EOF
>> +
>> +test_align_permutations() {
>> +       while read -r option
>> +       do
>> +               test_expect_success "align:$option" '
>> +               git for-each-ref --format="|%(align:$option)refname is %(refname)%(end)|%(refname)" >actual &&
>> +               test_cmp expect actual
>> +               '
>
> I think I mentioned this in the last round: The two lines following
> test_expect_success() are actually the test body, thus should be
> indented one more level. (Not necessarily worth a re-roll, though...)
>

I must have missed it, will change it.

>> +       done
>> +}
>> +
>> +test_align_permutations <<-\EOF
>> +       middle,42
>> +       42,middle
>> +       position=middle,42
>> +       42,position=middle
>> +       middle,width=42
>> +       width=42,middle
>> +       position=middle,width=42
>> +       width=42,position=middle
>> +EOF
>> +
>> +# Last one wins (silently) when multiple arguments of the same type are given
>> +
>> +test_align_permutations <<-\EOF
>> +       32,width=42,middle
>> +       width=30,42,middle
>> +       width=42,position=right,middle
>> +       42,right,position=middle
>> +EOF
>> +
>>  # Individual atoms inside %(align:...) and %(end) must not be quoted.
>>
>>  test_expect_success 'alignment with format quote' "
>> --
>> 2.7.0



-- 
Regards,
Karthik Nayak

Re: [PATCH v4 10/12] ref-filter: introduce remote_ref_atom_parser()

[Karthik Nayak]

On Fri, Feb 5, 2016 at 5:35 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sun, Jan 31, 2016 at 11:12:54PM +0530, Karthik Nayak wrote:
>> Introduce remote_ref_atom_parser() which will parse the '%(upstream)'
>> and '%(push)' atoms and store information into the 'used_atom'
>> structure based on the modifiers used along with the corresponding
>> atom.
>>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -50,6 +52,20 @@ static void color_atom_parser(struct used_atom *atom, const char *color_value)
>> +static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
>> +{
>> +     if (!arg) {
>> +             atom->u.remote_ref = RR_NORMAL;
>> +     } else if (!strcmp(arg, "short"))
>
> Style: drop unnecessary braces
>

Will do.

>> +             atom->u.remote_ref = RR_SHORTEN;
>> +     else if (!strcmp(arg, "track"))
>> +             atom->u.remote_ref = RR_TRACK;
>> +     else if (!strcmp(arg, "trackshort"))
>> +             atom->u.remote_ref = RR_TRACKSHORT;
>> +     else
>> +             die(_("unrecognized format: %%(%s)"), atom->name);
>> +}



-- 
Regards,
Karthik Nayak

Re: [PATCH v4 11/12] ref-filter: introduce contents_atom_parser()

[Karthik Nayak]

On Fri, Feb 5, 2016 at 5:52 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Introduce contents_atom_parser() which will parse the '%(contents)'
>> atom and store information into the 'used_atom' structure based on the
>> modifiers used along with the atom. Also introduce body_atom_parser()
>> and subject_atom_parser() for parsing atoms '%(body)' and '%(subject)'
>> respectively.
>
> These latter two parsers are conceptually distinct from introduction
> of the %(contents) parser, thus could be done in a follow-on patch or
> two (though I don't care strongly enough to insist upon it).
>

I think they go together, considering they use the same contents structure in
used_atom, Introducing separate patches would leave us in a grey area where
%(contents:...) uses used_atom->contents whereas body and subject don't.
So I'd prefer them being together.

>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -66,6 +70,38 @@ static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
>> +static void body_atom_parser(struct used_atom *atom, const char *arg)
>> +{
>> +       if (arg)
>> +               die("%%(body) atom does not take arguments");
>
> None of the other error messages bothers saying "atom" literally
> following a %(foo). For consistency, this likely should say merely:
>
>     %(body) does not take arguments
>

Will change.

>> +       atom->u.contents.option = C_BODY_DEP;
>> +}
>> +
>> +static void subject_atom_parser(struct used_atom *atom, const char *arg)
>> +{
>> +       if (arg)
>> +               die("%%(subject) atom does not take arguments");
>
> Ditto.
>

Will change.

>> +       atom->u.contents.option = C_SUB;
>> +}
>> @@ -733,19 +763,15 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
>>
>>         for (i = 0; i < used_atom_cnt; i++) {
>>                 const char *name = used_atom[i].name;
>> +               struct used_atom *atom = &used_atom[i];
>
> Not a big deal, but if you re-order these two lines, then the second,
> which extracts name, could do so from the variable declared by the
> first:
>
>     struct used_atom *atom = &used_atom[i];
>     const char *name = atom->name;
>

Seems good, will incorporate.

>>                 struct atom_value *v = &val[i];
>> -               const char *valp = NULL;
>>                 if (!!deref != (*name == '*'))
>>                         continue;
>>                 if (deref)
>>                         name++;
>>                 if (strcmp(name, "subject") &&
>>                     strcmp(name, "body") &&
>> -                   strcmp(name, "contents") &&
>> -                   strcmp(name, "contents:subject") &&
>> -                   strcmp(name, "contents:body") &&
>> -                   strcmp(name, "contents:signature") &&
>> -                   !starts_with(name, "contents:lines="))
>> +                   !starts_with(name, "contents"))
>>                         continue;
>
> This changes behavior in that it will also now match
> "contentsanything", whereas the original was much more strict. Is that
> desirable? (Genuine question.)
>

Well, we wont have something like that come through because
parse_ref_filter_atom()
would throw an error for %(contentsanything), but if in the future
sometime if we
introduce an atom %(contentsfoo) this might end up being a problem.

-- 
Regards,
Karthik Nayak

Re: [PATCH v4 05/12] ref-filter: introduce parsing functions for each valid atom

[Eric Sunshine]

On Sat, Feb 6, 2016 at 10:15 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Sun, Jan 31, 2016 at 11:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> @@ -138,10 +140,9 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
>>                  * shouldn't be used for checking against the valid_atom
>>                  * table.
>>                  */
>> -               const char *formatp = strchr(sp, ':');
>> -               if (!formatp || ep < formatp)
>> -                       formatp = ep;
>> -               if (len == formatp - sp && !memcmp(valid_atom[i].name, sp, len))
>> +               arg = memchr(sp, ':', ep - sp);
>> +               if ((!arg || len == arg - sp) &&
>> +                   !memcmp(valid_atom[i].name, sp, len))
>>                         break;
>>         }
>
> Also having a look at this, this breaks the previous error checking we
> had at parse_ref_filter_atom().
> e.g: git for-each-ref --format="%(refnameboo)" would not throw an error.
>
> I think the code needs to be changed to:
>
> -               if ((!arg || len == arg - sp) &&
> +               if ((arg || len == ep - sp) &&
> +                   (!arg || len == arg - sp) &&

For completeness, for people reading the mailing list archive, a
couple alternate fixes were presented elsewhere[1], with a personal
bias toward:

    arg = memchr(...);
    if (!arg)
        arg = ep;
    if (len == arg - sp && !memcmp(...))
        ...

[1]: http://git.661346.n2.nabble.com/PATCH-ref-filter-c-don-t-stomp-on-memory-tp7647432p7647433.html

Re: [PATCH v4 05/12] ref-filter: introduce parsing functions for each valid atom

[Eric Sunshine]

On Sat, Feb 6, 2016 at 9:36 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Thu, Feb 4, 2016 at 3:49 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>> -               const char *formatp = strchr(sp, ':');
>>> -               if (!formatp || ep < formatp)
>>> -                       formatp = ep;
>>> -               if (len == formatp - sp && !memcmp(valid_atom[i].name, sp, len))
>>> +               arg = memchr(sp, ':', ep - sp);
>>
>> Why this change from strchr() to memchr()? I understand that you're
>> taking advantage of the fact that you know the extent of the string
>> via 'sp' and 'ep', however, was the original strchr() doing extra
>> work? Even if this change is desirable, it seems somewhat unrelated to
>> the overall purpose of this patch, thus might deserves its own.
>>
>> Aside from that, although the "expensive" strchr() / memchr() resides
>> within the loop, it will always return the same value since it doesn't
>> depend upon any condition local to the loop. This implies that it
>> ought to be hoisted out of the loop. (This problem is not new to this
>> patch; it's already present in the existing code.)
>
> I'm thinking I'll make a patch for that separately. i.e remove strchr()
> and introduce memchr() outside the loop.

I'd almost suggest making it two patches: (1) change strchr() to
memchr(), and (2) hoist it outside the loop. However, it would be nice
to see this series land with v5, and adding more refactoring patches
could delay its landing if problems are found with those new patches.
Consequently, it might make sense to forego any additional refactoring
for now and just keep the patch as is, except for fixing the
relatively minor issues (and bug or two) raised in the v4 review. If
you take that approach, then hoisting memchr() out of the loop can be
done as a follow-up patch after this series lands.

>>> +               if ((!arg || len == arg - sp) &&
>>> +                   !memcmp(valid_atom[i].name, sp, len))
>>>                         break;
>>>         }
>>>
>>> @@ -154,6 +155,10 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
>>>         REALLOC_ARRAY(used_atom, used_atom_cnt);
>>>         used_atom[at].name = xmemdupz(atom, ep - atom);
>>>         used_atom[at].type = valid_atom[i].cmp_type;
>>> +       if (arg)
>>> +               arg = used_atom[at].name + (arg - atom) + 1;
>>
>> This is a harder to understand than it ought to be because it's
>> difficult to tell at first glance that you don't actually care about
>> 'arg' in relation to the original incoming string, but instead use it
>> only to compute an offset into the string which is ultimately stored
>> in the newly allocated used_atom[]. Re-using 'arg' for a different
>> purpose (in a manner of speaking) confuses the issue further.
>>
>> The intention might be easier to follow if you made it clear that you
>> were interested in the *offset* of the argument in the string, rather
>> than a pointer into the incoming string which you ultimately don't
>> use. A variable named 'arg_offset' might go a long way toward
>> clarifying this intention.
>
> I hope you mean something like this.
>
> if (arg) {
>     int arg_offset;
>
>     arg_offset = (arg - atom) + 1;
>     arg = used_atom[at].name + arg_offset;
> }

That's one way, but I was actually thinking about computing arg_offset
earlier in the "is it a valid atom?" loop, which would make it clear
that you care only about the offset at that point, rather than the
pointer to the ':' in the original string (since that pointer is never
itself used other than to compute the offset). However, having tried
it myself, the code ends up being nosier, thus not necessarily a win,
so maybe just leave this as is for now.

Re: [PATCH v4 06/12] ref-filter: introduce color_atom_parser()

[Eric Sunshine]

On Sat, Feb 6, 2016 at 10:20 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Fri, Feb 5, 2016 at 3:55 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>> +static void color_atom_parser(struct used_atom *atom, const char *color_value)
>>> +{
>>> +       if (!color_value)
>>> +               die(_("expected format: %%(color:<color>)"));
>>> +       if (color_parse(color_value, atom->u.color) < 0)
>>> +               die(_("invalid color value: %s"), atom->u.color);
>>
>> Shouldn't this be:
>>
>>     die(_("invalid color value: %s"), color_value);
>>
>> ?
>
> Oops. You're right, it should.
> Also the error is reported already in color_parse(...), so seems duplicated.
>
> git for-each-ref  --format="%(color:sfadf)%(align:middle,30)%(refname)%(end)"
> error: invalid color value: sfadf
> fatal: invalid color value: sfadf
>
> What would be an ideal way around this?

According to f6c5a29 (color_parse: do not mention variable name in
error message, 2014-10-07), the doubled diagnostic messages are
intentional, so I don't think you need to do anything about it in this
series. If you want to make the "fatal" message a bit more helpful,
you could add a %(color:) annotation, like this:

    die(_("unrecognized color: %%(color:%s)"), color_value);

which would give you the slightly more helpful:

    error: invalid color value: sfadf
    fatal: unrecognized color: %(color:sfadf)

Re: [PATCH v4 06/12] ref-filter: introduce color_atom_parser()

[Eric Sunshine]

On Sat, Feb 6, 2016 at 10:51 AM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Sat, Feb 6, 2016 at 4:20 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Also the error is reported already in color_parse(...), so seems duplicated.
>>
>> git for-each-ref  --format="%(color:sfadf)%(align:middle,30)%(refname)%(end)"
>> error: invalid color value: sfadf
>> fatal: invalid color value: sfadf
>>
>> What would be an ideal way around this?
>
> Maybe it has already been discussed a lot and I missed the discussion,
> but if possible the argument, the parameter or the atom itself might
> just be ignored with a warning instead of dying when an atom argument,
> format or parameter is not recognized, because in the next Git
> versions we might want to add new arguments, formats and parameter and
> it would be sad if old versions of Git die when those new things are
> passed to them.

The current behavior of die()ing is inherited from existing code which
Karthik refactored to create ref-filter.c, so it is not a new issue,
and old versions of Git are already afflicted. Whether die()ing is
desirable or not is unrelated to the current series which is primarily
aimed at optimizing and slightly generalizing ref-filter.c.

Re: [PATCH v4 05/12] ref-filter: introduce parsing functions for each valid atom

[Karthik Nayak]

On Sun, Feb 7, 2016 at 12:03 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sat, Feb 6, 2016 at 10:15 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> On Sun, Jan 31, 2016 at 11:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>> @@ -138,10 +140,9 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
>>>                  * shouldn't be used for checking against the valid_atom
>>>                  * table.
>>>                  */
>>> -               const char *formatp = strchr(sp, ':');
>>> -               if (!formatp || ep < formatp)
>>> -                       formatp = ep;
>>> -               if (len == formatp - sp && !memcmp(valid_atom[i].name, sp, len))
>>> +               arg = memchr(sp, ':', ep - sp);
>>> +               if ((!arg || len == arg - sp) &&
>>> +                   !memcmp(valid_atom[i].name, sp, len))
>>>                         break;
>>>         }
>>
>> Also having a look at this, this breaks the previous error checking we
>> had at parse_ref_filter_atom().
>> e.g: git for-each-ref --format="%(refnameboo)" would not throw an error.
>>
>> I think the code needs to be changed to:
>>
>> -               if ((!arg || len == arg - sp) &&
>> +               if ((arg || len == ep - sp) &&
>> +                   (!arg || len == arg - sp) &&
>
> For completeness, for people reading the mailing list archive, a
> couple alternate fixes were presented elsewhere[1], with a personal
> bias toward:
>
>     arg = memchr(...);
>     if (!arg)
>         arg = ep;
>     if (len == arg - sp && !memcmp(...))
>         ...
>
> [1]: http://git.661346.n2.nabble.com/PATCH-ref-filter-c-don-t-stomp-on-memory-tp7647432p7647433.html

There is a slight issue with this solution though, as you see 'arg'
gets modified
here, hence 'arg' passed to parser functions will never will null.

We could do something like
if (arg ==ep)
    arg = NULL;
if (arg)
    arg = used_atom[at].name + (arg - atom) + 1;
if (valid_atom[i].parser)
    valid_atom[i].parser(&used_atom[at], arg);

Else we could avoid this assignment and re-assignment by letting 'arg'
hold the value it gets from memcmp(...) and use the solution provided
by me or Ramsay (preferably)

Ramsay's solution being

 ref-filter.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ref-filter.c b/ref-filter.c
index d48e2a3..c98065e 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -260,7 +260,8 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
                 * table.
                 */
                arg = memchr(sp, ':', ep - sp);
-               if ((!arg || len == arg - sp) &&
+               if ((( arg && len == arg - sp)  ||
+                    (!arg && len == ep - sp )) &&
                    !memcmp(valid_atom[i].name, sp, len))
                        break;
        }

-- 
Regards,
Karthik Nayak

Re: [PATCH v4 05/12] ref-filter: introduce parsing functions for each valid atom

[Karthik Nayak]

On Sun, Feb 7, 2016 at 12:33 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sat, Feb 6, 2016 at 9:36 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> On Thu, Feb 4, 2016 at 3:49 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>>> -               const char *formatp = strchr(sp, ':');
>>>> -               if (!formatp || ep < formatp)
>>>> -                       formatp = ep;
>>>> -               if (len == formatp - sp && !memcmp(valid_atom[i].name, sp, len))
>>>> +               arg = memchr(sp, ':', ep - sp);
>>>
>>> Why this change from strchr() to memchr()? I understand that you're
>>> taking advantage of the fact that you know the extent of the string
>>> via 'sp' and 'ep', however, was the original strchr() doing extra
>>> work? Even if this change is desirable, it seems somewhat unrelated to
>>> the overall purpose of this patch, thus might deserves its own.
>>>
>>> Aside from that, although the "expensive" strchr() / memchr() resides
>>> within the loop, it will always return the same value since it doesn't
>>> depend upon any condition local to the loop. This implies that it
>>> ought to be hoisted out of the loop. (This problem is not new to this
>>> patch; it's already present in the existing code.)
>>
>> I'm thinking I'll make a patch for that separately. i.e remove strchr()
>> and introduce memchr() outside the loop.
>
> I'd almost suggest making it two patches: (1) change strchr() to
> memchr(), and (2) hoist it outside the loop. However, it would be nice
> to see this series land with v5, and adding more refactoring patches
> could delay its landing if problems are found with those new patches.
> Consequently, it might make sense to forego any additional refactoring
> for now and just keep the patch as is, except for fixing the
> relatively minor issues (and bug or two) raised in the v4 review. If
> you take that approach, then hoisting memchr() out of the loop can be
> done as a follow-up patch after this series lands.
>

That makes sense too, I'll keep it the way it is in v4 and send a patch
post this series. Thanks

>>>> +               if ((!arg || len == arg - sp) &&
>>>> +                   !memcmp(valid_atom[i].name, sp, len))
>>>>                         break;
>>>>         }
>>>>
>>>> @@ -154,6 +155,10 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
>>>>         REALLOC_ARRAY(used_atom, used_atom_cnt);
>>>>         used_atom[at].name = xmemdupz(atom, ep - atom);
>>>>         used_atom[at].type = valid_atom[i].cmp_type;
>>>> +       if (arg)
>>>> +               arg = used_atom[at].name + (arg - atom) + 1;
>>>
>>> This is a harder to understand than it ought to be because it's
>>> difficult to tell at first glance that you don't actually care about
>>> 'arg' in relation to the original incoming string, but instead use it
>>> only to compute an offset into the string which is ultimately stored
>>> in the newly allocated used_atom[]. Re-using 'arg' for a different
>>> purpose (in a manner of speaking) confuses the issue further.
>>>
>>> The intention might be easier to follow if you made it clear that you
>>> were interested in the *offset* of the argument in the string, rather
>>> than a pointer into the incoming string which you ultimately don't
>>> use. A variable named 'arg_offset' might go a long way toward
>>> clarifying this intention.
>>
>> I hope you mean something like this.
>>
>> if (arg) {
>>     int arg_offset;
>>
>>     arg_offset = (arg - atom) + 1;
>>     arg = used_atom[at].name + arg_offset;
>> }
>
> That's one way, but I was actually thinking about computing arg_offset
> earlier in the "is it a valid atom?" loop, which would make it clear
> that you care only about the offset at that point, rather than the
> pointer to the ':' in the original string (since that pointer is never
> itself used other than to compute the offset). However, having tried
> it myself, the code ends up being nosier, thus not necessarily a win,
> so maybe just leave this as is for now.

True, letting it be seems to make sense.

Thanks.

-- 
Regards,
Karthik Nayak

Re: [PATCH v4 06/12] ref-filter: introduce color_atom_parser()

[Karthik Nayak]

On Sun, Feb 7, 2016 at 1:13 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sat, Feb 6, 2016 at 10:20 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> On Fri, Feb 5, 2016 at 3:55 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>>> +static void color_atom_parser(struct used_atom *atom, const char *color_value)
>>>> +{
>>>> +       if (!color_value)
>>>> +               die(_("expected format: %%(color:<color>)"));
>>>> +       if (color_parse(color_value, atom->u.color) < 0)
>>>> +               die(_("invalid color value: %s"), atom->u.color);
>>>
>>> Shouldn't this be:
>>>
>>>     die(_("invalid color value: %s"), color_value);
>>>
>>> ?
>>
>> Oops. You're right, it should.
>> Also the error is reported already in color_parse(...), so seems duplicated.
>>
>> git for-each-ref  --format="%(color:sfadf)%(align:middle,30)%(refname)%(end)"
>> error: invalid color value: sfadf
>> fatal: invalid color value: sfadf
>>
>> What would be an ideal way around this?
>
> According to f6c5a29 (color_parse: do not mention variable name in
> error message, 2014-10-07), the doubled diagnostic messages are
> intentional, so I don't think you need to do anything about it in this
> series. If you want to make the "fatal" message a bit more helpful,
> you could add a %(color:) annotation, like this:
>
>     die(_("unrecognized color: %%(color:%s)"), color_value);
>
> which would give you the slightly more helpful:
>
>     error: invalid color value: sfadf
>     fatal: unrecognized color: %(color:sfadf)

That seems to be a good way around the repetitive message.
will add thanks.

-- 
Regards,
Karthik Nayak

Re: [PATCH v4 05/12] ref-filter: introduce parsing functions for each valid atom

[Eric Sunshine]

On Sun, Feb 7, 2016 at 4:01 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Sun, Feb 7, 2016 at 12:03 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sat, Feb 6, 2016 at 10:15 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>> I think the code needs to be changed to:
>>>
>>> -               if ((!arg || len == arg - sp) &&
>>> +               if ((arg || len == ep - sp) &&
>>> +                   (!arg || len == arg - sp) &&
>>
>> For completeness, for people reading the mailing list archive, a
>> couple alternate fixes were presented elsewhere[1], with a personal
>> bias toward:
>>
>>     arg = memchr(...);
>>     if (!arg)
>>         arg = ep;
>
> There is a slight issue with this solution though, as you see 'arg'
> gets modified
> here, hence 'arg' passed to parser functions will never will null.
> [...]
> Else we could avoid this assignment and re-assignment by letting 'arg'
> hold the value it gets from memcmp(...) and use the solution provided
> by me or Ramsay (preferably)
>
> Ramsay's solution being
>
>                 arg = memchr(sp, ':', ep - sp);
> -               if ((!arg || len == arg - sp) &&
> +               if ((( arg && len == arg - sp)  ||
> +                    (!arg && len == ep - sp )) &&
>                     !memcmp(valid_atom[i].name, sp, len))
>                         break;

Yep, Ramsey's fix is preferable.

Re: [PATCH v4 05/12] ref-filter: introduce parsing functions for each valid atom

[Andreas Schwab]

Karthik Nayak <karthik.188@gmail.com> writes:

> +               if ((( arg && len == arg - sp)  ||
> +                    (!arg && len == ep - sp )) &&

                      len == (arg ? arg : ep) - sp &&

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH v4 05/12] ref-filter: introduce parsing functions for each valid atom

[Karthik Nayak]

On Sun, Feb 7, 2016 at 7:17 PM, Andreas Schwab <schwab@linux-m68k.org> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +               if ((( arg && len == arg - sp)  ||
>> +                    (!arg && len == ep - sp )) &&
>
>                       len == (arg ? arg : ep) - sp &&

This is better I feel. Thank You

-- 
Regards,
Karthik Nayak