[PATCH v2 00/11] ref-filter: use parsing functions

[PATCH v2 00/11] 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.
The previous version can be found here:
http://thread.gmane.org/gmane.comp.version-control.git/281180

Changes in this version:
1. Re-ordered the patch series and split patches.
2. Used 'enums' rather than 'structs' in 'used_atom' structure.
3. Made all functions static to remove global scope (Thanks to Ramsay Jones).
4. Added "subject" and "body" atom under contents_atom_parser.
5. Changes in error messages, function names and variable names.
6. Changes in comments and documentation.
7. Added extensive test cases for the %(align) atom.

Thanks to Eric and Junio for suggestions on v1.
TODO: Implement an enum to denote current atom in used_atom,
so as to get rid of repetitive checks performed.
(http://article.gmane.org/gmane.comp.version-control.git/282320).

Karthik Nayak (11):
  strbuf: introduce strbuf_split_str_without_term()
  ref-filter: use strbuf_split_str_omit_term()
  ref-filter: introduce struct used_atom
  ref-fitler: bump match_atom() name to the top
  ref-filter: skip deref specifier in match_atom_name()
  ref-filter: introduce color_atom_parser()
  ref-filter: introduce align_atom_parser()
  ref-filter: introduce prefixes for the align atom
  ref-filter: introduce remote_ref_atom_parser()
  ref-filter: introduce contents_atom_parser()
  ref-filter: introduce objectname_atom_parser()

 Documentation/git-for-each-ref.txt |  20 +-
 ref-filter.c                       | 441 ++++++++++++++++++++++---------------
 strbuf.c                           |   7 +-
 strbuf.h                           |  25 ++-
 t/t6302-for-each-ref-filter.sh     | 162 ++++++++++++++
 5 files changed, 462 insertions(+), 193 deletions(-)

Interdiff:

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 56ffdc1..9af0f53 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -133,8 +133,10 @@ align::
 	`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. The prefix for the
-	arguments is not mandatory. If the contents length is more
+	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
diff --git a/ref-filter.c b/ref-filter.c
index 3bd3e45..a8fd178 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -37,23 +37,13 @@ static struct used_atom {
 	union {
 		const char *color;
 		struct align align;
+		enum { RR_SHORTEN, RR_TRACK, RR_TRACKSHORT, RR_NORMAL }
+			remote_ref;
 		struct {
-			unsigned int shorten : 1,
-				track : 1,
-				trackshort : 1;
-		} remote_ref;
-		struct {
-			unsigned int subject : 1,
-				body : 1,
-				signature : 1,
-				all : 1,
-				lines : 1,
-				no_lines;
+			enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
+			unsigned int no_lines;
 		} contents;
-		struct {
-			unsigned int shorten : 1,
-				full : 1;
-		} objectname;
+		enum { O_FULL, O_SHORT } objectname;
 	} u;
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
@@ -79,81 +69,89 @@ static int match_atom_name(const char *name, const char *atom_name, const char *
 	return 1;
 }
 
-void color_atom_parser(struct used_atom *atom)
+static void color_atom_parser(struct used_atom *atom)
 {
 	match_atom_name(atom->str, "color", &atom->u.color);
 	if (!atom->u.color)
 		die(_("expected format: %%(color:<color>)"));
 }
 
-void remote_ref_atom_parser(struct used_atom *atom)
+static void remote_ref_atom_parser(struct used_atom *atom)
 {
 	const char *buf;
 
 	buf = strchr(atom->str, ':');
+	atom->u.remote_ref = RR_NORMAL;
 	if (!buf)
 		return;
 	buf++;
 	if (!strcmp(buf, "short"))
-		atom->u.remote_ref.shorten = 1;
+		atom->u.remote_ref = RR_SHORTEN;
 	else if (!strcmp(buf, "track"))
-		atom->u.remote_ref.track = 1;
+		atom->u.remote_ref = RR_TRACK;
 	else if (!strcmp(buf, "trackshort"))
-		atom->u.remote_ref.trackshort = 1;
+		atom->u.remote_ref = RR_TRACKSHORT;
 	else
-		die(_("improper format entered align:%s"), buf);
+		die(_("unrecognized format: %%(%s)"), atom->str);
 }
 
-void contents_atom_parser(struct used_atom *atom)
+static void contents_atom_parser(struct used_atom *atom)
 {
 	const char * buf;
 
 	if (match_atom_name(atom->str, "contents", &buf))
-		atom->u.contents.all = 1;
-
+		atom->u.contents.option = C_BARE;
+	else if (match_atom_name(atom->str, "subject", &buf)) {
+		atom->u.contents.option = C_SUB;
+		return;
+	} else if (match_atom_name(atom->str, "body", &buf)) {
+		atom->u.contents.option = C_BODY_DEP;
+		return;
+	}
 	if (!buf)
 		return;
+
 	if (!strcmp(buf, "body"))
-		atom->u.contents.body = 1;
+		atom->u.contents.option = C_BODY;
 	else if (!strcmp(buf, "signature"))
-		atom->u.contents.signature = 1;
+		atom->u.contents.option = C_SIG;
 	else if (!strcmp(buf, "subject"))
-		atom->u.contents.subject = 1;
+		atom->u.contents.option = C_SUB;
 	else if (skip_prefix(buf, "lines=", &buf)) {
-		atom->u.contents.lines = 1;
+		atom->u.contents.option = C_LINES;
 		if (strtoul_ui(buf, 10, &atom->u.contents.no_lines))
 			die(_("positive value expected contents:lines=%s"), buf);
 	} else
-		die(_("improper format entered contents:%s"), buf);
+		die(_("unrecognized %%(contents) argument: %s"), buf);
 }
 
-void objectname_atom_parser(struct used_atom *atom)
+static void objectname_atom_parser(struct used_atom *atom)
 {
 	const char * buf;
 
 	if (match_atom_name(atom->str, "objectname", &buf))
-		atom->u.objectname.full = 1;
-
+		atom->u.objectname = O_FULL;
 	if (!buf)
 		return;
+
 	if (!strcmp(buf, "short"))
-		atom->u.objectname.shorten = 1;
+		atom->u.objectname = O_SHORT;
 	else
-		die(_("improper format entered objectname:%s"), buf);
+		die(_("unrecognized %%(objectname) argument: %s"), buf);
 }
 
-static align_type get_align_position(const char *type)
+static align_type parse_align_position(const char *s)
 {
-	if (!strcmp(type, "right"))
+	if (!strcmp(s, "right"))
 		return ALIGN_RIGHT;
-	else if (!strcmp(type, "middle"))
+	else if (!strcmp(s, "middle"))
 		return ALIGN_MIDDLE;
-	else if (!strcmp(type, "left"))
+	else if (!strcmp(s, "left"))
 		return ALIGN_LEFT;
 	return -1;
 }
 
-void align_atom_parser(struct used_atom *atom)
+static void align_atom_parser(struct used_atom *atom)
 {
 	struct align *align = &atom->u.align;
 	const char *buf = NULL;
@@ -163,31 +161,28 @@ void align_atom_parser(struct used_atom *atom)
 	match_atom_name(atom->str, "align", &buf);
 	if (!buf)
 		die(_("expected format: %%(align:<width>,<position>)"));
-	s = to_free = strbuf_split_str_without_term(buf, ',', 0);
+	s = to_free = strbuf_split_str_omit_term(buf, ',', 0);
 
-	/*  By default align to ALGIN_LEFT */
 	align->position = ALIGN_LEFT;
 
 	while (*s) {
 		int position;
 		buf = s[0]->buf;
 
-		position = get_align_position(buf);
-
 		if (skip_prefix(buf, "position=", &buf)) {
-			position = get_align_position(buf);
+			position = parse_align_position(buf);
 			if (position == -1)
-				die(_("improper format entered align:%s"), s[0]->buf);
+				die(_("unrecognized position:%s"), buf);
 			align->position = position;
 		} else if (skip_prefix(buf, "width=", &buf)) {
 			if (strtoul_ui(buf, 10, (unsigned int *)&width))
-				die(_("improper format entered align:%s"), s[0]->buf);
+				die(_("unrecognized width:%s"), buf);
 		} else if (!strtoul_ui(buf, 10, (unsigned int *)&width))
-				;
-		else if (position != -1)
+			;
+		else if ((position = parse_align_position(buf)) != -1)
 			align->position = position;
 		else
-			die(_("improper format entered align:%s"), s[0]->buf);
+			die(_("unrecognized %%(align) argument: %s"), s[0]->buf);
 		s++;
 	}
 
@@ -226,8 +221,8 @@ static struct {
 	{ "taggerdate", FIELD_TIME },
 	{ "creator" },
 	{ "creatordate", FIELD_TIME },
-	{ "subject" },
-	{ "body" },
+	{ "subject", FIELD_STR, contents_atom_parser },
+	{ "body", FIELD_STR, contents_atom_parser },
 	{ "contents", FIELD_STR, contents_atom_parser },
 	{ "upstream", FIELD_STR, remote_ref_atom_parser },
 	{ "push", FIELD_STR, remote_ref_atom_parser },
@@ -485,10 +480,10 @@ static int grab_objectname(const char *name, const unsigned char *sha1,
 			   struct atom_value *v, struct used_atom *atom)
 {
 	if (starts_with(name, "objectname")) {
-		if (atom->u.objectname.shorten) {
+		if (atom->u.objectname == O_SHORT) {
 			v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));
 			return 1;
-		} else if (atom->u.objectname.full) {
+		} else if (atom->u.objectname == O_FULL) {
 			v->s = xstrdup(sha1_to_hex(sha1));
 			return 1;
 		}
@@ -815,7 +810,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
 			name++;
 		if (strcmp(name, "subject") &&
 		    strcmp(name, "body") &&
-		    !atom->u.contents.all)
+		    !starts_with(name, "contents"))
 			continue;
 		if (!subpos)
 			find_subpos(buf, sz,
@@ -823,24 +818,22 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
 				    &bodypos, &bodylen, &nonsiglen,
 				    &sigpos, &siglen);
 
-		if (!strcmp(name, "subject"))
-			v->s = copy_subject(subpos, sublen);
-		else if (atom->u.contents.subject)
+		if (atom->u.contents.option == C_SUB)
 			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 (atom->u.contents.body)
+		else if (atom->u.contents.option == C_BODY)
 			v->s = xmemdupz(bodypos, nonsiglen);
-		else if (atom->u.contents.signature)
+		else if (atom->u.contents.option == C_SIG)
 			v->s = xmemdupz(sigpos, siglen);
-		else if (atom->u.contents.lines) {
+		else if (atom->u.contents.option == C_LINES) {
 			struct strbuf s = STRBUF_INIT;
 			const char *contents_end = bodylen + bodypos - siglen;
 
 			/*  Size is the length of the message after removing the signature */
 			append_lines(&s, subpos, contents_end - subpos, atom->u.contents.no_lines);
 			v->s = strbuf_detach(&s, NULL);
-		} else /*  For %(contents) without modifiers */
+		} else if (atom->u.contents.option == C_BARE)
 			v->s = xstrdup(subpos);
 	}
 }
@@ -903,9 +896,9 @@ 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.shorten)
+	if (atom->u.remote_ref == RR_SHORTEN)
 		*s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);
-	else if (atom->u.remote_ref.track) {
+	else if (atom->u.remote_ref == RR_TRACK) {
 		if (stat_tracking_info(branch, &num_ours,
 				       &num_theirs, NULL))
 			return;
@@ -918,7 +911,7 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
 		else
 			*s = xstrfmt("[ahead %d, behind %d]",
 				     num_ours, num_theirs);
-	} else if (atom->u.remote_ref.trackshort) {
+	} else if (atom->u.remote_ref == RR_TRACKSHORT) {
 		if (stat_tracking_info(branch, &num_ours,
 				       &num_theirs, NULL))
 			return;
@@ -931,7 +924,7 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
 			*s = ">";
 		else
 			*s = "<>";
-	} else
+	} else if (atom->u.remote_ref == RR_NORMAL)
 		*s = refname;
 }
 
@@ -1003,11 +996,10 @@ static void populate_value(struct ref_array_item *ref)
 				continue;
 			fill_remote_ref_details(atom, refname, branch, &v->s);
 			continue;
-		} else if (starts_with(name, "color")) {
+		} else if (starts_with(name, "color:")) {
 			char color[COLOR_MAXLEN] = "";
-			const char *valp = atom->u.color;
 
-			if (color_parse(valp, color) < 0)
+			if (color_parse(atom->u.color, color) < 0)
 				die(_("unable to parse format"));
 			v->s = xstrdup(color);
 			continue;
diff --git a/strbuf.c b/strbuf.c
index d31336f..b62508e 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 with_term)
+				 int terminator, int max, int omit_term)
 {
 	struct strbuf **ret = NULL;
 	size_t nr = 0, alloc = 0;
@@ -123,27 +123,19 @@ struct strbuf **strbuf_split_buf(const char *str, size_t slen,
 
 	while (slen) {
 		int len = slen;
-		int term = with_term;
+		const char *end = NULL;
 		if (max <= 0 || nr + 1 < max) {
-			const char *end = memchr(str, terminator, slen);
+			end = memchr(str, terminator, slen);
 			if (end)
-				len = end - str + term;
-			else
-				/*  When no terminator present, we must add the last character */
-				term = 1;
+				len = end - str + 1;
 		}
 		t = xmalloc(sizeof(struct strbuf));
 		strbuf_init(t, len);
-		strbuf_add(t, str, len);
+		strbuf_add(t, str, len - !!end * !!omit_term);
 		ALLOC_GROW(ret, nr + 2, alloc);
 		ret[nr++] = t;
-		if (!term) {
-			str += len + 1;
-			slen -= len + 1;
-		} else {
-			str += len;
-			slen -= len;
-		}
+		str += len;
+		slen -= len;
 	}
 	ALLOC_GROW(ret, nr + 1, alloc); /* In case string was empty */
 	ret[nr] = NULL;
diff --git a/strbuf.h b/strbuf.h
index 63e1e69..a865a74 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -450,11 +450,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.  The substrings include the terminator if
+ * the value of omit_term is '0' else the terminator is excluded.  The
+ * last substring, 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
  * character).
  *
  * The most generic form is `strbuf_split_buf`, which takes an arbitrary
@@ -466,24 +467,24 @@ static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)
  * string_list_split_in_place().
  */
 extern struct strbuf **strbuf_split_buf(const char *str, size_t slen,
-					int terminator, int max, int with_term);
+					int terminator, int max, int omit_term);
 
-static inline struct strbuf **strbuf_split_str_without_term(const char *str,
+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, 0);
+	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, 1);
+	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, 1);
+	return strbuf_split_buf(sb->buf, sb->len, terminator, max, 0);
 }
 
 static inline struct strbuf **strbuf_split(const struct strbuf *sb,
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 688751e..bcb6771 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -97,7 +97,7 @@ test_expect_success 'left alignment is default' '
 	refname is refs/tags/three    |refs/tags/three
 	refname is refs/tags/two      |refs/tags/two
 	EOF
-	git for-each-ref --format="%(align:width=30)refname is %(refname)%(end)|%(refname)" >actual &&
+	git for-each-ref --format="%(align:30)refname is %(refname)%(end)|%(refname)" >actual &&
 	test_cmp expect actual
 '
 
@@ -113,7 +113,7 @@ test_expect_success 'middle alignment' '
 	|  refname is refs/tags/three  |refs/tags/three
 	|   refname is refs/tags/two   |refs/tags/two
 	EOF
-	git for-each-ref --format="|%(align:position=middle,30)refname is %(refname)%(end)|%(refname)" >actual &&
+	git for-each-ref --format="|%(align:middle,30)refname is %(refname)%(end)|%(refname)" >actual &&
 	test_cmp expect actual
 '
 
@@ -133,6 +133,168 @@ test_expect_success 'right alignment' '
 	test_cmp expect actual
 '
 
+test_expect_success 'alignment with "position" prefix' '
+	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
+	git for-each-ref --format="|%(align:30,position=right)refname is %(refname)%(end)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'alignment with "position" prefix' '
+	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
+	git for-each-ref --format="|%(align:position=right,30)refname is %(refname)%(end)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'alignment with "width" prefix' '
+	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
+	git for-each-ref --format="|%(align:width=30,right)refname is %(refname)%(end)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'alignment with "width" prefix' '
+	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
+	git for-each-ref --format="|%(align:right,width=30)refname is %(refname)%(end)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'alignment with "position" and "width" prefix' '
+	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
+	git for-each-ref --format="|%(align:width=30,position=right)refname is %(refname)%(end)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'alignment with "position" and "width" prefix' '
+	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
+	git for-each-ref --format="|%(align:position=right,width=30)refname is %(refname)%(end)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
+# Last one wins (silently) when multiple arguments of the same type are given
+
+test_expect_success 'alignment with multiple "<width>" values' '
+	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
+	git for-each-ref --format="|%(align:32,width=30)refname is %(refname)%(end)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'alignment with multiple "<width>" values' '
+	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
+	git for-each-ref --format="|%(align:width=30,32)refname is %(refname)%(end)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'alignment with multiple "<position>" values' '
+	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
+	git for-each-ref --format="|%(align:width=30,position=middle,right)refname is %(refname)%(end)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'alignment with multiple "<position>" values' '
+	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
+	git for-each-ref --format="|%(align:30,right,position=middle)refname is %(refname)%(end)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
 # Individual atoms inside %(align:...) and %(end) must not be quoted.
 
 test_expect_success 'alignment with format quote' "

-- 
2.6.4

[PATCH v2 01/11] strbuf: introduce strbuf_split_str_without_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_without_term()' which
splits a given string into strbufs without including the terminator.

Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
---
 strbuf.c |  7 ++++---
 strbuf.h | 25 ++++++++++++++++---------
 2 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/strbuf.c b/strbuf.c
index b552a13..b62508e 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,15 @@ struct strbuf **strbuf_split_buf(const char *str, size_t slen,
 
 	while (slen) {
 		int len = slen;
+		const char *end = NULL;
 		if (max <= 0 || nr + 1 < max) {
-			const char *end = memchr(str, terminator, slen);
+			end = memchr(str, terminator, slen);
 			if (end)
 				len = end - str + 1;
 		}
 		t = xmalloc(sizeof(struct strbuf));
 		strbuf_init(t, len);
-		strbuf_add(t, str, len);
+		strbuf_add(t, str, len - !!end * !!omit_term);
 		ALLOC_GROW(ret, nr + 2, alloc);
 		ret[nr++] = t;
 		str += len;
diff --git a/strbuf.h b/strbuf.h
index 6ae7a72..a865a74 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -450,11 +450,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.  The substrings include the terminator if
+ * the value of omit_term is '0' else the terminator is excluded.  The
+ * last substring, 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
  * character).
  *
  * The most generic form is `strbuf_split_buf`, which takes an arbitrary
@@ -465,19 +466,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.6.4

[PATCH v2 02/11] 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.

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 e205dd2..6263710 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -869,18 +869,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.6.4

[PATCH v2 03/11] ref-filter: introduce struct used_atom

[Karthik Nayak]

Introduce the 'used_array' structure which would replace the existing
implementation of 'used_array' (which a list of atoms). This helps us
parse atom's before hand and store required details into the
'used_array' for future usage.

Also introduce a parsing function for each atom in valid_atom. Using
this we can define special parsing functions for each of the atoms.

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

diff --git a/ref-filter.c b/ref-filter.c
index 6263710..043a02a 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -16,9 +16,27 @@
 
 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 struct used_atom {
+	const char *str;
+	cmp_type type;
+} *used_atom;
+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;
+	void (*parser)(struct used_atom *atom);
 } valid_atom[] = {
 	{ "refname" },
 	{ "objecttype" },
@@ -92,21 +110,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)
@@ -122,8 +125,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].str);
+		if (len == ep - atom && !memcmp(used_atom[i].str, atom, len))
 			return i;
 	}
 
@@ -150,12 +153,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].str = 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].str, "symref"))
 		need_symref = 1;
 	return at;
 }
@@ -315,7 +317,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].str, "color:", &color))
 			need_color_reset_at_eol = !!strcmp(color, "reset");
 	}
 	return 0;
@@ -359,7 +361,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].str;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;
@@ -383,7 +385,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].str;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;
@@ -405,7 +407,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].str;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;
@@ -535,7 +537,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].str;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;
@@ -573,7 +575,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].str;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;
@@ -663,7 +665,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].str;
 		struct atom_value *v = &val[i];
 		const char *valp = NULL;
 		if (!!deref != (*name == '*'))
@@ -786,7 +788,8 @@ 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];
+		struct used_atom *atom = &used_atom[i];
+		const char *name = atom->str;
 		struct atom_value *v = &ref->value[i];
 		int deref = 0;
 		const char *refname;
@@ -1438,7 +1441,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.6.4

[PATCH v2 04/11] ref-fitler: bump match_atom() name to the top

[Karthik Nayak]

Bump match_atom() to the top for usage in further patches.

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

diff --git a/ref-filter.c b/ref-filter.c
index 043a02a..f4a6414 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -33,6 +33,22 @@ 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)
+{
+	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;
+}
+
 static struct {
 	const char *name;
 	cmp_type cmp_type;
@@ -260,22 +276,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).
  */
-- 
2.6.4

[PATCH v2 05/11] ref-filter: skip deref specifier in match_atom_name()

[Karthik Nayak]

In upcoming patches we make calls to match_atom_name() with the '*'
deref specifier still attached to the atom name. This causes
undesirable errors, hence, if present skip over the '*' deref
specifier in the atom name.

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

diff --git a/ref-filter.c b/ref-filter.c
index f4a6414..7d33b83 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -37,6 +37,10 @@ static int match_atom_name(const char *name, const char *atom_name, const char *
 {
 	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]) {
-- 
2.6.4

[PATCH v2 06/11] ref-filter: introduce color_atom_parser()

[Karthik Nayak]

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

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

diff --git a/ref-filter.c b/ref-filter.c
index 7d33b83..3b61c62 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -29,6 +29,9 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 static struct used_atom {
 	const char *str;
 	cmp_type type;
+	union {
+		const char *color;
+	} u;
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
 static int need_color_reset_at_eol;
@@ -53,6 +56,13 @@ static int match_atom_name(const char *name, const char *atom_name, const char *
 	return 1;
 }
 
+static void color_atom_parser(struct used_atom *atom)
+{
+	match_atom_name(atom->str, "color", &atom->u.color);
+	if (!atom->u.color)
+		die(_("expected format: %%(color:<color>)"));
+}
+
 static struct {
 	const char *name;
 	cmp_type cmp_type;
@@ -90,7 +100,7 @@ static struct {
 	{ "symref" },
 	{ "flag" },
 	{ "HEAD" },
-	{ "color" },
+	{ "color", FIELD_STR, color_atom_parser },
 	{ "align" },
 	{ "end" },
 };
@@ -175,6 +185,9 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
 	REALLOC_ARRAY(used_atom, used_atom_cnt);
 	used_atom[at].str = xmemdupz(atom, ep - atom);
 	used_atom[at].type = valid_atom[i].cmp_type;
+	memset(&used_atom[at].u, 0, sizeof(used_atom[at].u));
+	if (valid_atom[i].parser)
+		valid_atom[i].parser(&used_atom[at]);
 	if (*atom == '*')
 		need_tagged = 1;
 	if (!strcmp(used_atom[at].str, "symref"))
@@ -833,12 +846,10 @@ 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)) {
+		} else if (starts_with(name, "color:")) {
 			char color[COLOR_MAXLEN] = "";
 
-			if (!valp)
-				die(_("expected format: %%(color:<color>)"));
-			if (color_parse(valp, color) < 0)
+			if (color_parse(atom->u.color, color) < 0)
 				die(_("unable to parse format"));
 			v->s = xstrdup(color);
 			continue;
-- 
2.6.4

[PATCH v2 07/11] 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 "use_atom"
structure for further usage in 'populate_value()'.

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

diff --git a/ref-filter.c b/ref-filter.c
index 3b61c62..a44673c 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 {
 		const char *color;
+		struct align align;
 	} u;
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
@@ -63,6 +69,49 @@ static void color_atom_parser(struct used_atom *atom)
 		die(_("expected format: %%(color:<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 void align_atom_parser(struct used_atom *atom)
+{
+	struct align *align = &atom->u.align;
+	const char *buf = NULL;
+	struct strbuf **s, **to_free;
+	int width = -1;
+
+	match_atom_name(atom->str, "align", &buf);
+	if (!buf)
+		die(_("expected format: %%(align:<width>,<position>)"));
+	s = to_free = strbuf_split_str_omit_term(buf, ',', 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(_("unrecognized %%(align) argument: %s"), s[0]->buf);
+		s++;
+	}
+
+	if (width < 0)
+		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;
@@ -101,17 +150,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;
@@ -880,35 +924,7 @@ static void populate_value(struct ref_array_item *ref)
 				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) {
-				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
-					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);
+			v->u.align = atom->u.align;
 			v->handler = align_atom_handler;
 			continue;
 		} else if (!strcmp(name, "end")) {
-- 
2.6.4

[PATCH v2 08/11] ref-filter: introduce prefixes for the align atom

[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 Documetation and tests for the same.

Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
---
 Documentation/git-for-each-ref.txt |  20 +++--
 ref-filter.c                       |  13 ++-
 t/t6302-for-each-ref-filter.sh     | 162 +++++++++++++++++++++++++++++++++++++
 3 files changed, 185 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index c6f073c..9af0f53 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -129,14 +129,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 a44673c..985423b 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -96,10 +96,19 @@ static void align_atom_parser(struct used_atom *atom)
 
 	while (*s) {
 		int position;
+		buf = s[0]->buf;
 
-		if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))
+		if (skip_prefix(buf, "position=", &buf)) {
+			position = parse_align_position(buf);
+			if (position == -1)
+				die(_("unrecognized position:%s"), buf);
+			align->position = position;
+		} else if (skip_prefix(buf, "width=", &buf)) {
+			if (strtoul_ui(buf, 10, (unsigned int *)&width))
+				die(_("unrecognized width:%s"), buf);
+		} else if (!strtoul_ui(buf, 10, (unsigned int *)&width))
 			;
-		else if ((position = parse_align_position(s[0]->buf)) > 0)
+		else if ((position = parse_align_position(buf)) != -1)
 			align->position = position;
 		else
 			die(_("unrecognized %%(align) argument: %s"), s[0]->buf);
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index fe4796c..bcb6771 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -133,6 +133,168 @@ test_expect_success 'right alignment' '
 	test_cmp expect actual
 '
 
+test_expect_success 'alignment with "position" prefix' '
+	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
+	git for-each-ref --format="|%(align:30,position=right)refname is %(refname)%(end)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'alignment with "position" prefix' '
+	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
+	git for-each-ref --format="|%(align:position=right,30)refname is %(refname)%(end)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'alignment with "width" prefix' '
+	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
+	git for-each-ref --format="|%(align:width=30,right)refname is %(refname)%(end)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'alignment with "width" prefix' '
+	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
+	git for-each-ref --format="|%(align:right,width=30)refname is %(refname)%(end)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'alignment with "position" and "width" prefix' '
+	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
+	git for-each-ref --format="|%(align:width=30,position=right)refname is %(refname)%(end)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'alignment with "position" and "width" prefix' '
+	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
+	git for-each-ref --format="|%(align:position=right,width=30)refname is %(refname)%(end)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
+# Last one wins (silently) when multiple arguments of the same type are given
+
+test_expect_success 'alignment with multiple "<width>" values' '
+	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
+	git for-each-ref --format="|%(align:32,width=30)refname is %(refname)%(end)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'alignment with multiple "<width>" values' '
+	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
+	git for-each-ref --format="|%(align:width=30,32)refname is %(refname)%(end)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'alignment with multiple "<position>" values' '
+	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
+	git for-each-ref --format="|%(align:width=30,position=middle,right)refname is %(refname)%(end)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'alignment with multiple "<position>" values' '
+	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
+	git for-each-ref --format="|%(align:30,right,position=middle)refname is %(refname)%(end)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
 # Individual atoms inside %(align:...) and %(end) must not be quoted.
 
 test_expect_success 'alignment with format quote' "
-- 
2.6.4

[PATCH v2 09/11] ref-filter: introduce remote_ref_atom_parser()

[Karthik Nayak]

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>
Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
---
 ref-filter.c | 105 ++++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 64 insertions(+), 41 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 985423b..44670e3 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -37,6 +37,8 @@ static struct used_atom {
 	union {
 		const char *color;
 		struct align align;
+		enum { RR_SHORTEN, RR_TRACK, RR_TRACKSHORT, RR_NORMAL }
+			remote_ref;
 	} u;
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
@@ -69,6 +71,25 @@ static void color_atom_parser(struct used_atom *atom)
 		die(_("expected format: %%(color:<color>)"));
 }
 
+static void remote_ref_atom_parser(struct used_atom *atom)
+{
+	const char *buf;
+
+	buf = strchr(atom->str, ':');
+	atom->u.remote_ref = RR_NORMAL;
+	if (!buf)
+		return;
+	buf++;
+	if (!strcmp(buf, "short"))
+		atom->u.remote_ref = RR_SHORTEN;
+	else if (!strcmp(buf, "track"))
+		atom->u.remote_ref = RR_TRACK;
+	else if (!strcmp(buf, "trackshort"))
+		atom->u.remote_ref = RR_TRACKSHORT;
+	else
+		die(_("unrecognized format: %%(%s)"), atom->str);
+}
+
 static align_type parse_align_position(const char *s)
 {
 	if (!strcmp(s, "right"))
@@ -153,8 +174,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" },
@@ -835,6 +856,42 @@ static inline char *copy_advance(char *dst, const char *src)
 	return dst;
 }
 
+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 if (atom->u.remote_ref == RR_NORMAL)
+		*s = refname;
+}
+
 /*
  * Parse the object referred by ref, and grab needed value.
  */
@@ -889,6 +946,8 @@ static void populate_value(struct ref_array_item *ref)
 			refname = branch_get_upstream(branch, NULL);
 			if (!refname)
 				continue;
+			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/",
@@ -899,6 +958,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:")) {
 			char color[COLOR_MAXLEN] = "";
 
@@ -944,49 +1005,11 @@ static void populate_value(struct ref_array_item *ref)
 
 		formatp = strchr(name, ':');
 		if (formatp) {
-			int num_ours, num_theirs;
-
 			formatp++;
 			if (!strcmp(formatp, "short"))
 				refname = shorten_unambiguous_ref(refname,
 						      warn_ambiguous_refs);
-			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);
 		}
-- 
2.6.4

[PATCH v2 10/11] 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.

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

diff --git a/ref-filter.c b/ref-filter.c
index 44670e3..ce02933 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -39,6 +39,10 @@ static struct used_atom {
 		struct align align;
 		enum { RR_SHORTEN, RR_TRACK, RR_TRACKSHORT, RR_NORMAL }
 			remote_ref;
+		struct {
+			enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
+			unsigned int no_lines;
+		} contents;
 	} u;
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
@@ -90,6 +94,36 @@ static void remote_ref_atom_parser(struct used_atom *atom)
 		die(_("unrecognized format: %%(%s)"), atom->str);
 }
 
+static void contents_atom_parser(struct used_atom *atom)
+{
+	const char * buf;
+
+	if (match_atom_name(atom->str, "contents", &buf))
+		atom->u.contents.option = C_BARE;
+	else if (match_atom_name(atom->str, "subject", &buf)) {
+		atom->u.contents.option = C_SUB;
+		return;
+	} else if (match_atom_name(atom->str, "body", &buf)) {
+		atom->u.contents.option = C_BODY_DEP;
+		return;
+	}
+	if (!buf)
+		return;
+
+	if (!strcmp(buf, "body"))
+		atom->u.contents.option = C_BODY;
+	else if (!strcmp(buf, "signature"))
+		atom->u.contents.option = C_SIG;
+	else if (!strcmp(buf, "subject"))
+		atom->u.contents.option = C_SUB;
+	else if (skip_prefix(buf, "lines=", &buf)) {
+		atom->u.contents.option = C_LINES;
+		if (strtoul_ui(buf, 10, &atom->u.contents.no_lines))
+			die(_("positive value expected contents:lines=%s"), buf);
+	} else
+		die(_("unrecognized %%(contents) argument: %s"), buf);
+}
+
 static align_type parse_align_position(const char *s)
 {
 	if (!strcmp(s, "right"))
@@ -171,9 +205,9 @@ static struct {
 	{ "taggerdate", FIELD_TIME },
 	{ "creator" },
 	{ "creatordate", FIELD_TIME },
-	{ "subject" },
-	{ "body" },
-	{ "contents" },
+	{ "subject", FIELD_STR, contents_atom_parser },
+	{ "body", FIELD_STR, contents_atom_parser },
+	{ "contents", FIELD_STR, contents_atom_parser },
 	{ "upstream", FIELD_STR, remote_ref_atom_parser },
 	{ "push", FIELD_STR, remote_ref_atom_parser },
 	{ "symref" },
@@ -186,11 +220,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;
@@ -207,7 +236,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 */
@@ -756,20 +784,16 @@ 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].str;
+		struct used_atom *atom = &used_atom[i];
+		const char *name = atom->str;
 		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,
@@ -777,28 +801,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.no_lines);
 			v->s = strbuf_detach(&s, NULL);
-		}
+		} else if (atom->u.contents.option == C_BARE)
+			v->s = xstrdup(subpos);
 	}
 }
 
-- 
2.6.4

[PATCH v2 11/11] 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>
Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
---
 ref-filter.c | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index ce02933..a8fd178 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 no_lines;
 		} contents;
+		enum { O_FULL, O_SHORT } objectname;
 	} u;
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
@@ -124,6 +125,21 @@ static void contents_atom_parser(struct used_atom *atom)
 		die(_("unrecognized %%(contents) argument: %s"), buf);
 }
 
+static void objectname_atom_parser(struct used_atom *atom)
+{
+	const char * buf;
+
+	if (match_atom_name(atom->str, "objectname", &buf))
+		atom->u.objectname = O_FULL;
+	if (!buf)
+		return;
+
+	if (!strcmp(buf, "short"))
+		atom->u.objectname = O_SHORT;
+	else
+		die(_("unrecognized %%(objectname) argument: %s"), buf);
+}
+
 static align_type parse_align_position(const char *s)
 {
 	if (!strcmp(s, "right"))
@@ -184,7 +200,7 @@ static struct {
 	{ "refname" },
 	{ "objecttype" },
 	{ "objectsize", FIELD_ULONG },
-	{ "objectname" },
+	{ "objectname", FIELD_STR, objectname_atom_parser },
 	{ "tree" },
 	{ "parent" },
 	{ "numparent", FIELD_ULONG },
@@ -461,15 +477,16 @@ 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;
+		}
 	}
 	return 0;
 }
@@ -493,7 +510,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);
+			grab_objectname(name, obj->sha1, v, &used_atom[i]);
 	}
 }
 
@@ -999,7 +1016,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.6.4

Re: [PATCH v2 01/11] strbuf: introduce strbuf_split_str_without_term()

[Eric Sunshine]

On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> strbuf: introduce strbuf_split_str_without_term()

s/without/omit/

> 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_without_term()' which

s/without/omit/

> splits a given string into strbufs without including the terminator.
>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
> diff --git 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,15 @@ struct strbuf **strbuf_split_buf(const char *str, size_t slen,
>
>         while (slen) {
>                 int len = slen;
> +               const char *end = NULL;
>                 if (max <= 0 || nr + 1 < max) {
> -                       const char *end = memchr(str, terminator, slen);
> +                       end = memchr(str, terminator, slen);
>                         if (end)
>                                 len = end - str + 1;
>                 }
>                 t = xmalloc(sizeof(struct strbuf));
>                 strbuf_init(t, len);
> -               strbuf_add(t, str, len);
> +               strbuf_add(t, str, len - !!end * !!omit_term);

This version of the patch with its minimal[1] change is much easier to
review for correctness than the original attempt[2].

[1]: http://article.gmane.org/gmane.comp.version-control.git/281882
[2]: http://article.gmane.org/gmane.comp.version-control.git/281189

>                 ALLOC_GROW(ret, nr + 2, alloc);
>                 ret[nr++] = t;
>                 str += len;
> diff --git a/strbuf.h b/strbuf.h
> index 6ae7a72..a865a74 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -450,11 +450,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.  The substrings include the terminator if
> + * the value of omit_term is '0' else the terminator is excluded.  The

Perhaps just say "if omit_term is false" rather than "if the value of ... is 0".

> + * last substring, might be unterminated if the original string did
> + * not end with a terminator.  If max is positive, then split the

This bit about the last substring is perhaps too disconnected from the
previous sentence. What if you re-word the entire thing something like
this:

    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.

> + * 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
> @@ -465,19 +466,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.6.4

Re: [PATCH v2 03/11] ref-filter: introduce struct used_atom

[Eric Sunshine]

On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Introduce the 'used_array' structure which would replace the existing
> implementation of 'used_array' (which a list of atoms). This helps us
> parse atom's before hand and store required details into the
> 'used_array' for future usage.

All my v1 review comments[1] about the commit message still apply to
this version.

[1]: http://article.gmane.org/gmane.comp.version-control.git/281860

> Also introduce a parsing function for each atom in valid_atom. Using
> this we can define special parsing functions for each of the atoms.

This is a conceptually distinct change which probably deserves its own
patch. In particular, the new patch would add this field to
valid_atom[] *and* add the code which invokes the custom parser. (That
code is currently commingled with introduction of the color parser in
patch 6/11.)

More below...

> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -16,9 +16,27 @@
> +/*
> + * 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 struct used_atom {
> +       const char *str;
> +       cmp_type type;
> +} *used_atom;
> +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;
> +       void (*parser)(struct used_atom *atom);
>  } valid_atom[] = {
>         { "refname" },
>         { "objecttype" },
> @@ -786,7 +788,8 @@ 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];
> +               struct used_atom *atom = &used_atom[i];
> +               const char *name = atom->str;

Same question as my previous review[1]: Why not just:

    const char *name = used_atom[i].str;

?

>                 struct atom_value *v = &ref->value[i];
>                 int deref = 0;
>                 const char *refname;

Re: [PATCH v2 05/11] ref-filter: skip deref specifier in match_atom_name()

[Eric Sunshine]

On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> In upcoming patches we make calls to match_atom_name() with the '*'
> deref specifier still attached to the atom name. This causes
> undesirable errors, hence, if present skip over the '*' deref
> specifier in the atom name.

I'd drop the second sentence since it doesn't add much or any value.
Instead, you might want to explain that skipping '*' is done as a
convenience.

    Subsequent patches will call match_atom_name() with the '*' deref
    specifier still attached to the atom name so, as a convenience,
    skip over it on their behalf.

> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -37,6 +37,10 @@ static int match_atom_name(const char *name, const char *atom_name, const char *
>  {
>         const char *body;
>
> +       /*  skip the deref specifier*/

Too many spaces before "skip".
Too few spaces after "specifier".

> +       if (name[0] == '*')
> +               name++;
> +
>         if (!skip_prefix(name, atom_name, &body))
>                 return 0; /* doesn't even begin with "atom_name" */
>         if (!body[0]) {
> --
> 2.6.4

Re: [PATCH v2 06/11] ref-filter: introduce color_atom_parser()

[Eric Sunshine]

On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Introduce color_atom_parser() which will parse a "color" atom and
> store its color in the "use_atom" structure for further usage in

Same comment as last time: s/use_atom/used_atom/

> 'populate_value()'.

s/'//g

More below...

> Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -29,6 +29,9 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
>  static struct used_atom {
>         const char *str;
>         cmp_type type;
> +       union {
> +               const char *color;
> +       } u;
>  } *used_atom;
>  static int used_atom_cnt, need_tagged, need_symref;
>  static int need_color_reset_at_eol;
> @@ -53,6 +56,13 @@ static int match_atom_name(const char *name, const char *atom_name, const char *
> +static void color_atom_parser(struct used_atom *atom)
> +{
> +       match_atom_name(atom->str, "color", &atom->u.color);
> +       if (!atom->u.color)
> +               die(_("expected format: %%(color:<color>)"));
> +}
> +
> @@ -833,12 +846,10 @@ 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)) {
> +               } else if (starts_with(name, "color:")) {
>                         char color[COLOR_MAXLEN] = "";
>
> -                       if (!valp)
> -                               die(_("expected format: %%(color:<color>)"));
> -                       if (color_parse(valp, color) < 0)
> +                       if (color_parse(atom->u.color, color) < 0)

It would make a lot more sense to invoke color_parse() with the
unchanging argument to "color:" just once in color_atom_parser()
rather than doing it here repeatedly. (Also, you'd drop 'const' from
used_atom.u.color declaration.)

>                                 die(_("unable to parse format"));
>                         v->s = xstrdup(color);

Does v->s get freed each time through the loop? If not, then, assuming
you parse the color just once in color_atom_parser(), then you could
just assign the parsed color directly to v->s rather than duplicating
it.

>                         continue;
> --
> 2.6.4

Re: [PATCH v2 07/11] ref-filter: introduce align_atom_parser()

[Eric Sunshine]

On Wed, Dec 16, 2015 at 10:29 AM, 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 "use_atom"
> structure for further usage in 'populate_value()'.
>
> Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -63,6 +69,49 @@ static void color_atom_parser(struct used_atom *atom)
> +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 void align_atom_parser(struct used_atom *atom)
> +{
> +       struct align *align = &atom->u.align;
> +       const char *buf = NULL;
> +       struct strbuf **s, **to_free;
> +       int width = -1;
> +
> +       match_atom_name(atom->str, "align", &buf);
> +       if (!buf)
> +               die(_("expected format: %%(align:<width>,<position>)"));
> +       s = to_free = strbuf_split_str_omit_term(buf, ',', 0);
> +
> +       align->position = ALIGN_LEFT;
> +
> +       while (*s) {
> +               int position;
> +
> +               if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))

This casting is pretty ugly. It probably would be cleaner to declare
'width' as 'unsigned int' and initialize it with ~0U than to do this
ugly and potentially dangerous casting. Likewise, below where you
check 'width < 0', you'd check instead for ~0U. However, such a change
should not be part of the current patch, but rather as a separate
preparatory patch.

> +                       ;
> +               else if ((position = parse_align_position(s[0]->buf)) > 0)

Shouldn't this be '>=' rather than '>'?

This likely would have been easier to spot if parse_align_position()
had been factored out in its own patch, as suggested by my v1
review[1], since the caller would have been trivially inspectable
rather than having to keep track of both code movement and changes.

[1]: http://article.gmane.org/gmane.comp.version-control.git/281916

> +                       align->position = position;
> +               else
> +                       die(_("unrecognized %%(align) argument: %s"), s[0]->buf);
> +               s++;
> +       }
> +
> +       if (width < 0)
> +               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;
> @@ -880,35 +924,7 @@ static void populate_value(struct ref_array_item *ref)
>                                 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) {
> -                               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
> -                                       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);
> +                       v->u.align = atom->u.align;
>                         v->handler = align_atom_handler;
>                         continue;
>                 } else if (!strcmp(name, "end")) {
> --
> 2.6.4

Re: [PATCH v2 01/11] strbuf: introduce strbuf_split_str_without_term()

[Karthik Nayak]

On Thu, Dec 17, 2015 at 2:05 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> strbuf: introduce strbuf_split_str_without_term()
>
> s/without/omit/
>
>> 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_without_term()' which
>
> s/without/omit/
>

Oops! will change that,

>> splits a given string into strbufs without including the terminator.
>>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>> diff --git 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,15 @@ struct strbuf **strbuf_split_buf(const char *str, size_t slen,
>>
>>         while (slen) {
>>                 int len = slen;
>> +               const char *end = NULL;
>>                 if (max <= 0 || nr + 1 < max) {
>> -                       const char *end = memchr(str, terminator, slen);
>> +                       end = memchr(str, terminator, slen);
>>                         if (end)
>>                                 len = end - str + 1;
>>                 }
>>                 t = xmalloc(sizeof(struct strbuf));
>>                 strbuf_init(t, len);
>> -               strbuf_add(t, str, len);
>> +               strbuf_add(t, str, len - !!end * !!omit_term);
>
> This version of the patch with its minimal[1] change is much easier to
> review for correctness than the original attempt[2].
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/281882
> [2]: http://article.gmane.org/gmane.comp.version-control.git/281189
>
>>                 ALLOC_GROW(ret, nr + 2, alloc);
>>                 ret[nr++] = t;
>>                 str += len;
>> diff --git a/strbuf.h b/strbuf.h
>> index 6ae7a72..a865a74 100644
>> --- a/strbuf.h
>> +++ b/strbuf.h
>> @@ -450,11 +450,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.  The substrings include the terminator if
>> + * the value of omit_term is '0' else the terminator is excluded.  The
>
> Perhaps just say "if omit_term is false" rather than "if the value of ... is 0".
>
>> + * last substring, might be unterminated if the original string did
>> + * not end with a terminator.  If max is positive, then split the
>
> This bit about the last substring is perhaps too disconnected from the
> previous sentence. What if you re-word the entire thing something like
> this:
>
>     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.
>

Im quite bad at this, thanks :)

>>
>> This version of the patch with its minimal[1] change is much easier to
>> review for correctness than the original attempt[2].
>
> Perhaps worthy of a Helped-by:?

Definitely! Sorry for not including that, slipped off my mind.

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 08/11] ref-filter: introduce prefixes for the align atom

[Eric Sunshine]

On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> ref-filter: introduce prefixes for the align atom

The prefixes are actually for the arguments to the 'align' atom, not
for the atom itself. However, it might be better to describe this at a
bit higher level. Perhaps:

    ref-filter: align: introduce long-form syntax

or something.

> Introduce optional prefixes "width=" and "position=" for the align atom
> so that the atom can be used as "%(align:width=<width>,position=<position>)".
>
> Add Documetation and tests for the same.
>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -96,10 +96,19 @@ static void align_atom_parser(struct used_atom *atom)
>
>         while (*s) {
>                 int position;
> +               buf = s[0]->buf;

It probably would be better to do this assignment in the previous
patch (7/11) since its presence here introduces unwanted noise
(textual replacement of "s[0]->buf" with "buf") in several locations
below which slightly obscure the real changes of this patch.

> -               if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))
> +               if (skip_prefix(buf, "position=", &buf)) {
> +                       position = parse_align_position(buf);
> +                       if (position == -1)

It may be more idiomatic in this codebase to detect errors via '< 0'
rather than explicit '== -1'. Ditto below.

> +                               die(_("unrecognized position:%s"), buf);
> +                       align->position = position;
> +               } else if (skip_prefix(buf, "width=", &buf)) {
> +                       if (strtoul_ui(buf, 10, (unsigned int *)&width))
> +                               die(_("unrecognized width:%s"), buf);
> +               } else if (!strtoul_ui(buf, 10, (unsigned int *)&width))
>                         ;
> -               else if ((position = parse_align_position(s[0]->buf)) > 0)
> +               else if ((position = parse_align_position(buf)) != -1)
>                         align->position = position;
>                 else
>                         die(_("unrecognized %%(align) argument: %s"), s[0]->buf);

s/s[0]->//

More below...

> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> @@ -133,6 +133,168 @@ test_expect_success 'right alignment' '
>         test_cmp expect actual
>  '
>
> +test_expect_success 'alignment with "position" prefix' '
> +       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
> +       git for-each-ref --format="|%(align:30,position=right)refname is %(refname)%(end)|%(refname)" >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'alignment with "position" prefix' '
> +       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
> +       git for-each-ref --format="|%(align:position=right,30)refname is %(refname)%(end)|%(refname)" >actual &&
> +       test_cmp expect actual
> +'

This (and below) is a lot of copy/paste code which makes it difficult
to read the tests and maintain (change) them. Since 'expect' doesn't
appear to change from test to test, one way to eliminate some of this
noisiness would be to create 'expect' once outside of the tests.

However, even better, especially from a comprehension,
maintainability, and extensibility standpoints would be to make this
all table-driven. In particular, I'd expect to see a table with
exactly the list of test inputs mentioned in my earlier review[1], and
have that table passed to a shell function which performs the test for
each element of the table. For instance, something like:

    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

where test_align_permutations is the name of the shell function which
reads each line of it stdin and performs the "git for-each-ref
--format=..." test with the given %(align:) arguments.

Ditto regarding the below "last one wins (silently) tests".

[1]: http://article.gmane.org/gmane.comp.version-control.git/281916

> +
> +test_expect_success 'alignment with "width" prefix' '
> +       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
> +       git for-each-ref --format="|%(align:width=30,right)refname is %(refname)%(end)|%(refname)" >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'alignment with "width" prefix' '
> +       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
> +       git for-each-ref --format="|%(align:right,width=30)refname is %(refname)%(end)|%(refname)" >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'alignment with "position" and "width" prefix' '
> +       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
> +       git for-each-ref --format="|%(align:width=30,position=right)refname is %(refname)%(end)|%(refname)" >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'alignment with "position" and "width" prefix' '
> +       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
> +       git for-each-ref --format="|%(align:position=right,width=30)refname is %(refname)%(end)|%(refname)" >actual &&
> +       test_cmp expect actual
> +'
> +
> +# Last one wins (silently) when multiple arguments of the same type are given
> +
> +test_expect_success 'alignment with multiple "<width>" values' '
> +       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
> +       git for-each-ref --format="|%(align:32,width=30)refname is %(refname)%(end)|%(refname)" >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'alignment with multiple "<width>" values' '
> +       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
> +       git for-each-ref --format="|%(align:width=30,32)refname is %(refname)%(end)|%(refname)" >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'alignment with multiple "<position>" values' '
> +       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
> +       git for-each-ref --format="|%(align:width=30,position=middle,right)refname is %(refname)%(end)|%(refname)" >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'alignment with multiple "<position>" values' '
> +       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
> +       git for-each-ref --format="|%(align:30,right,position=middle)refname is %(refname)%(end)|%(refname)" >actual &&
> +       test_cmp expect actual
> +'
> +
>  # Individual atoms inside %(align:...) and %(end) must not be quoted.
>
>  test_expect_success 'alignment with format quote' "
> --
> 2.6.4
>

Re: [PATCH v2 09/11] ref-filter: introduce remote_ref_atom_parser()

[Eric Sunshine]

On Wed, Dec 16, 2015 at 10:30 AM, Karthik Nayak <karthik.188@gmail.com> 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>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -37,6 +37,8 @@ static struct used_atom {
>         union {
>                 const char *color;
>                 struct align align;
> +               enum { RR_SHORTEN, RR_TRACK, RR_TRACKSHORT, RR_NORMAL }

Nit: I'd have expected to see the normal/plain case first rather than
last (but not itself worth a re-roll).

> +                       remote_ref;
>         } u;
>  } *used_atom;
>  static int used_atom_cnt, need_tagged, need_symref;
> @@ -69,6 +71,25 @@ static void color_atom_parser(struct used_atom *atom)
> +static void remote_ref_atom_parser(struct used_atom *atom)
> +{
> +       const char *buf;
> +
> +       buf = strchr(atom->str, ':');
> +       atom->u.remote_ref = RR_NORMAL;
> +       if (!buf)
> +               return;

This code is not as clear as it could be due to the way the 'buf'
assignment and check for NULL are split apart. It can be made clearer
either by doing this:

    atom->u.remote_ref = RR_NORMAL;
    buf = strchr(...);
    if (!buf)
        return;

or (even better) this:

    buf = strchr(...);
    if (!buf) {
        atom->u.remote_ref = RR_NORMAL;
        return;
    }

> +       buf++;
> +       if (!strcmp(buf, "short"))
> +               atom->u.remote_ref = RR_SHORTEN;
> +       else if (!strcmp(buf, "track"))
> +               atom->u.remote_ref = RR_TRACK;
> +       else if (!strcmp(buf, "trackshort"))
> +               atom->u.remote_ref = RR_TRACKSHORT;
> +       else
> +               die(_("unrecognized format: %%(%s)"), atom->str);
> +}
> +
> @@ -835,6 +856,42 @@ static inline char *copy_advance(char *dst, const char *src)
> +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;

The RR_TRACKSHORT branch below has a blank line following the
'return', but this branch lacks it, which is inconsistent.

> +               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))

What happened to the assert(branch) which was in the original code
from which this was derived (below)? Is it no longer needed?

> +                       return;
> +
> +               if (!num_ours && !num_theirs)
> +                       *s = "=";
> +               else if (!num_ours)
> +                       *s = "<";
> +               else if (!num_theirs)
> +                       *s = ">";
> +               else
> +                       *s = "<>";
> +       } else if (atom->u.remote_ref == RR_NORMAL)
> +               *s = refname;
> +}
> +
>  /*
>   * Parse the object referred by ref, and grab needed value.
>   */
> @@ -889,6 +946,8 @@ static void populate_value(struct ref_array_item *ref)
>                         refname = branch_get_upstream(branch, NULL);
>                         if (!refname)
>                                 continue;
> +                       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/",
> @@ -899,6 +958,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:")) {
>                         char color[COLOR_MAXLEN] = "";
>
> @@ -944,49 +1005,11 @@ static void populate_value(struct ref_array_item *ref)
>
>                 formatp = strchr(name, ':');
>                 if (formatp) {
> -                       int num_ours, num_theirs;
> -
>                         formatp++;
>                         if (!strcmp(formatp, "short"))
>                                 refname = shorten_unambiguous_ref(refname,
>                                                       warn_ambiguous_refs);
> -                       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);
>                 }
> --
> 2.6.4

Re: [PATCH v2 10/11] ref-filter: introduce contents_atom_parser()

[Eric Sunshine]

On Wed, Dec 16, 2015 at 10:30 AM, 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.
>
> Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -39,6 +39,10 @@ static struct used_atom {
>                 struct align align;
>                 enum { RR_SHORTEN, RR_TRACK, RR_TRACKSHORT, RR_NORMAL }
>                         remote_ref;
> +               struct {
> +                       enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
> +                       unsigned int no_lines;

'no_lines' sounds like "no lines!". How about 'nlines' instead?

> +               } contents;
>         } u;
>  } *used_atom;
>  static int used_atom_cnt, need_tagged, need_symref;
> @@ -90,6 +94,36 @@ static void remote_ref_atom_parser(struct used_atom *atom)
> +static void contents_atom_parser(struct used_atom *atom)
> +{
> +       const char * buf;
> +
> +       if (match_atom_name(atom->str, "contents", &buf))
> +               atom->u.contents.option = C_BARE;
> +       else if (match_atom_name(atom->str, "subject", &buf)) {

The original code used strcmp() and matched only "subject", however
the new code will incorrectly match both "subject" and
"subject:whatever". Therefore, you should be using strcmp() here
rather than match_atom_name().

Ditto for "body".

> +               atom->u.contents.option = C_SUB;
> +               return;
> +       } else if (match_atom_name(atom->str, "body", &buf)) {
> +               atom->u.contents.option = C_BODY_DEP;
> +               return;
> +       }
> +       if (!buf)
> +               return;

It's not easy to see that this 'if (!buf)' check relates to the
"contents" check at the very top of the if/else if/ chain since there
are entirely unrelated checks in between. Reorganizing it can improve
clarity:

    if (!strcmp("subject")) {
        ...
        return;
    } else if (!strcmp("body")) {
        ...
        return;
    } else if (!match_atom_name(...,"contents", &buf))
        die("BUG: expected 'contents' or 'contents:'");

    if (!buf) {
        atom->u.contents.option = C_BARE;
        return;
    }

> +       if (!strcmp(buf, "body"))
> +               atom->u.contents.option = C_BODY;
> +       else if (!strcmp(buf, "signature"))
> +               atom->u.contents.option = C_SIG;
> +       else if (!strcmp(buf, "subject"))
> +               atom->u.contents.option = C_SUB;
> +       else if (skip_prefix(buf, "lines=", &buf)) {
> +               atom->u.contents.option = C_LINES;
> +               if (strtoul_ui(buf, 10, &atom->u.contents.no_lines))
> +                       die(_("positive value expected contents:lines=%s"), buf);
> +       } else
> +               die(_("unrecognized %%(contents) argument: %s"), buf);
> +}
> @@ -777,28 +801,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.no_lines);
>                         v->s = strbuf_detach(&s, NULL);
> -               }
> +               } else if (atom->u.contents.option == C_BARE)
> +                       v->s = xstrdup(subpos);
>         }
>  }
>
> --
> 2.6.4

Re: [PATCH v2 07/11] ref-filter: introduce align_atom_parser()

[Eric Sunshine]

On Wed, Dec 16, 2015 at 10:29 AM, 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 "use_atom"
> structure for further usage in 'populate_value()'.
>
> Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -63,6 +69,49 @@ static void color_atom_parser(struct used_atom *atom)
> +static void align_atom_parser(struct used_atom *atom)
> +{
> +       [...]
> +       match_atom_name(atom->str, "align", &buf);
> +       if (!buf)
> +               die(_("expected format: %%(align:<width>,<position>)"));
> +       [...]
> +}
> @@ -880,35 +924,7 @@ static void populate_value(struct ref_array_item *ref)
>                                 v->s = " ";
>                         continue;
>                 } else if (match_atom_name(name, "align", &valp)) {

Hmm, align_atom_parser() has already been called before we get here,
right? And it has already invoked match_atom_name() and died if the
argument to "align:" was missing, correct? If so, then this invocation
of match_atom_name() in populate_value() is unnecessary and should be
replaced with a simpler starts_with("align:").

Plus, 'valp' is not used, aside from this unnecessary
match_atom_name(), thus it can be removed as well.

> -                       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) {
> -                               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
> -                                       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);
> +                       v->u.align = atom->u.align;
>                         v->handler = align_atom_handler;
>                         continue;
>                 } else if (!strcmp(name, "end")) {
> --
> 2.6.4

Re: [PATCH v2 11/11] ref-filter: introduce objectname_atom_parser()

[Eric Sunshine]

On Wed, Dec 16, 2015 at 10:30 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> 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>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
> diff --git 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 no_lines;
>                 } contents;
> +               enum { O_FULL, O_SHORT } objectname;
>         } u;
>  } *used_atom;
> @@ -124,6 +125,21 @@ static void contents_atom_parser(struct used_atom *atom)
> +static void objectname_atom_parser(struct used_atom *atom)
> +{
> +       const char * buf;
> +
> +       if (match_atom_name(atom->str, "objectname", &buf))
> +               atom->u.objectname = O_FULL;
> +       if (!buf)
> +               return;

Let me make sure that I understand this correctly.

make_atom_name("objectname") will return true only for "objectname" or
"objectname:", and will return false for anything else, such as
"objectnamely" or "schmorf". Furthermore, the only way
objectname_atom_parser() can be called is when %(objectname) or
%(objectname:...) is seen, thus match_atom_name() *must* return true
here, which means the above conditional is misleading, suggesting that
it could somehow return false.

And, if match_atom_name() did return false here, then that indicates a
programming error: objectname_atom_parser() somehow got called for
something other than %(objectname) or %(objectname:...). This implies
that the code should instead be structured like this:

    if (!match_atom_name(..., "objectname", &buf)
        die("BUG: parsing non-'objectname'")
    if (!buf)
        atom->u.objectname = O_FULL;
    else if (!strcmp(buf, "short"))
        atom->u.objectname = O_SHORT;
    else
        die(_("unrecognized %%(objectname) argument: %s"), buf);

However, this can be simplified further by recognizing that, following
this patch series, match_atom_name() is *only* called by these new
parse functions[1], which means that, as a convenience,
match_atom_name() itself could become a void rather than boolean
function and die() if the expected atom name is not found. Thus, the
code would become:

    match_atom_name(...);
    if (!buf)
        ...
    else if (!strcmp(...))
        ...
    ...

By the way, the above commentary isn't specific to this patch and
%(objectname), but is in fact also relevant for all of the preceding
patches which introduce parse functions calling match_atom_name().

More below...

[1]: ...assuming you replace the unnecessary match_atom_name() in
populate_value() with starts_with() as suggested in my patch 7/11
review addendum[2].

[2]: http://article.gmane.org/gmane.comp.version-control.git/282700

> +
> +       if (!strcmp(buf, "short"))
> +               atom->u.objectname = O_SHORT;
> +       else
> +               die(_("unrecognized %%(objectname) argument: %s"), buf);
> +}
> +
> @@ -461,15 +477,16 @@ 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;
> +               }

Since 'objectname' can only ever be O_SHORT or O_FULL wouldn't it be a
programming error if it ever falls through to this point after the
closing brace? Perhaps a die("BUG:...") would be appropriate?

>         }
>         return 0;
>  }
> @@ -493,7 +510,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);
> +                       grab_objectname(name, obj->sha1, v, &used_atom[i]);

This patch hunk is somehow corrupt and doesn't apply. Was there some
hand-editing involved, or were some earlier patches regenerated after
this patch was made or something?

>         }
>  }
>
> @@ -999,7 +1016,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.6.4

Re: [PATCH v2 03/11] ref-filter: introduce struct used_atom

[Karthik Nayak]

On Thu, Dec 17, 2015 at 2:27 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Introduce the 'used_array' structure which would replace the existing
>> implementation of 'used_array' (which a list of atoms). This helps us
>> parse atom's before hand and store required details into the
>> 'used_array' for future usage.
>
> All my v1 review comments[1] about the commit message still apply to
> this version.
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/281860
>

I totally missed this out, thanks for bringing it up.

>> Also introduce a parsing function for each atom in valid_atom. Using
>> this we can define special parsing functions for each of the atoms.
>
> This is a conceptually distinct change which probably deserves its own
> patch. In particular, the new patch would add this field to
> valid_atom[] *and* add the code which invokes the custom parser. (That
> code is currently commingled with introduction of the color parser in
> patch 6/11.)
>go

I guess that could be done, I was thinking it goes together, but it
makes sense to have a
separate patch for introduction of the parsing function and its invocations.

> More below...
>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -16,9 +16,27 @@
>> +/*
>> + * 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 struct used_atom {
>> +       const char *str;
>> +       cmp_type type;
>> +} *used_atom;
>> +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;
>> +       void (*parser)(struct used_atom *atom);
>>  } valid_atom[] = {
>>         { "refname" },
>>         { "objecttype" },
>> @@ -786,7 +788,8 @@ 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];
>> +               struct used_atom *atom = &used_atom[i];
>> +               const char *name = atom->str;
>
> Same question as my previous review[1]: Why not just:
>
>     const char *name = used_atom[i].str;
>
> ?

I think It's leftover code, I was using the atom variable also before.
I'll remove it.

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 05/11] ref-filter: skip deref specifier in match_atom_name()

[Karthik Nayak]

On Thu, Dec 17, 2015 at 2:41 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> In upcoming patches we make calls to match_atom_name() with the '*'
>> deref specifier still attached to the atom name. This causes
>> undesirable errors, hence, if present skip over the '*' deref
>> specifier in the atom name.
>
> I'd drop the second sentence since it doesn't add much or any value.
> Instead, you might want to explain that skipping '*' is done as a
> convenience.
>
>     Subsequent patches will call match_atom_name() with the '*' deref
>     specifier still attached to the atom name so, as a convenience,
>     skip over it on their behalf.
>

Thanks will put that in.

>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -37,6 +37,10 @@ static int match_atom_name(const char *name, const char *atom_name, const char *
>>  {
>>         const char *body;
>>
>> +       /*  skip the deref specifier*/
>
> Too many spaces before "skip".
> Too few spaces after "specifier".
>

Will fix.

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 06/11] ref-filter: introduce color_atom_parser()

[Karthik Nayak]

On Thu, Dec 17, 2015 at 2:51 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Introduce color_atom_parser() which will parse a "color" atom and
>> store its color in the "use_atom" structure for further usage in
>
> Same comment as last time: s/use_atom/used_atom/
>

Will change.

>> 'populate_value()'.
>
> s/'//g
>

Will do.

> More below...
>
>> Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -29,6 +29,9 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
>>  static struct used_atom {
>>         const char *str;
>>         cmp_type type;
>> +       union {
>> +               const char *color;
>> +       } u;
>>  } *used_atom;
>>  static int used_atom_cnt, need_tagged, need_symref;
>>  static int need_color_reset_at_eol;
>> @@ -53,6 +56,13 @@ static int match_atom_name(const char *name, const char *atom_name, const char *
>> +static void color_atom_parser(struct used_atom *atom)
>> +{
>> +       match_atom_name(atom->str, "color", &atom->u.color);
>> +       if (!atom->u.color)
>> +               die(_("expected format: %%(color:<color>)"));
>> +}
>> +
>> @@ -833,12 +846,10 @@ 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)) {
>> +               } else if (starts_with(name, "color:")) {
>>                         char color[COLOR_MAXLEN] = "";
>>
>> -                       if (!valp)
>> -                               die(_("expected format: %%(color:<color>)"));
>> -                       if (color_parse(valp, color) < 0)
>> +                       if (color_parse(atom->u.color, color) < 0)
>
> It would make a lot more sense to invoke color_parse() with the
> unchanging argument to "color:" just once in color_atom_parser()
> rather than doing it here repeatedly. (Also, you'd drop 'const' from
> used_atom.u.color declaration.)
>

This makes sense, I'll put have color_parse() in color_atom_parser().
This would also require us to allocate memory once in color_atom_parser.

>
> Does v->s get freed each time through the loop? If not, then, assuming
> you parse the color just once in color_atom_parser(), then you could
> just assign the parsed color directly to v->s rather than duplicating
> it.

No it doesn't. Yup will do.

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 07/11] ref-filter: introduce align_atom_parser()

[Karthik Nayak]

On Thu, Dec 17, 2015 at 3:24 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Dec 16, 2015 at 10:29 AM, 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 "use_atom"
>> structure for further usage in 'populate_value()'.
>>
>> Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -63,6 +69,49 @@ static void color_atom_parser(struct used_atom *atom)
>> +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 void align_atom_parser(struct used_atom *atom)
>> +{
>> +       struct align *align = &atom->u.align;
>> +       const char *buf = NULL;
>> +       struct strbuf **s, **to_free;
>> +       int width = -1;
>> +
>> +       match_atom_name(atom->str, "align", &buf);
>> +       if (!buf)
>> +               die(_("expected format: %%(align:<width>,<position>)"));
>> +       s = to_free = strbuf_split_str_omit_term(buf, ',', 0);
>> +
>> +       align->position = ALIGN_LEFT;
>> +
>> +       while (*s) {
>> +               int position;
>> +
>> +               if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))
>
> This casting is pretty ugly. It probably would be cleaner to declare
> 'width' as 'unsigned int' and initialize it with ~0U than to do this
> ugly and potentially dangerous casting. Likewise, below where you
> check 'width < 0', you'd check instead for ~0U. However, such a change
> should not be part of the current patch, but rather as a separate
> preparatory patch.
>

Will do.

>> +                       ;
>> +               else if ((position = parse_align_position(s[0]->buf)) > 0)
>
> Shouldn't this be '>=' rather than '>'?
>
> This likely would have been easier to spot if parse_align_position()
> had been factored out in its own patch, as suggested by my v1
> review[1], since the caller would have been trivially inspectable
> rather than having to keep track of both code movement and changes.
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/281916
>

Yes. it should. Will split this into three patches.

1. Creation of align_atom_parser().
2. Introduce parse_align_position().
3. make 'width' an unsigned int.

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 07/11] ref-filter: introduce align_atom_parser()

[Karthik Nayak]

On Fri, Dec 18, 2015 at 11:20 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Dec 16, 2015 at 10:29 AM, 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 "use_atom"
>> structure for further usage in 'populate_value()'.
>>
>> Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -63,6 +69,49 @@ static void color_atom_parser(struct used_atom *atom)
>> +static void align_atom_parser(struct used_atom *atom)
>> +{
>> +       [...]
>> +       match_atom_name(atom->str, "align", &buf);
>> +       if (!buf)
>> +               die(_("expected format: %%(align:<width>,<position>)"));
>> +       [...]
>> +}
>> @@ -880,35 +924,7 @@ static void populate_value(struct ref_array_item *ref)
>>                                 v->s = " ";
>>                         continue;
>>                 } else if (match_atom_name(name, "align", &valp)) {
>
> Hmm, align_atom_parser() has already been called before we get here,
> right? And it has already invoked match_atom_name() and died if the
> argument to "align:" was missing, correct? If so, then this invocation
> of match_atom_name() in populate_value() is unnecessary and should be
> replaced with a simpler starts_with("align:").
>
> Plus, 'valp' is not used, aside from this unnecessary
> match_atom_name(), thus it can be removed as well.
>

WIll do. Thanks for noticing this. I missed it.

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 09/11] ref-filter: introduce remote_ref_atom_parser()

[Karthik Nayak]

On Thu, Dec 17, 2015 at 2:52 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Dec 16, 2015 at 10:30 AM, Karthik Nayak <karthik.188@gmail.com> 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>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -37,6 +37,8 @@ static struct used_atom {
>>         union {
>>                 const char *color;
>>                 struct align align;
>> +               enum { RR_SHORTEN, RR_TRACK, RR_TRACKSHORT, RR_NORMAL }
>
> Nit: I'd have expected to see the normal/plain case first rather than
> last (but not itself worth a re-roll).
>

Will add it in. That'll put it in an alphabetical order too.

>> +                       remote_ref;
>>         } u;
>>  } *used_atom;
>>  static int used_atom_cnt, need_tagged, need_symref;
>> @@ -69,6 +71,25 @@ static void color_atom_parser(struct used_atom *atom)
>> +static void remote_ref_atom_parser(struct used_atom *atom)
>> +{
>> +       const char *buf;
>> +
>> +       buf = strchr(atom->str, ':');
>> +       atom->u.remote_ref = RR_NORMAL;
>> +       if (!buf)
>> +               return;
>
> This code is not as clear as it could be due to the way the 'buf'
> assignment and check for NULL are split apart. It can be made clearer
> either by doing this:
>
>     atom->u.remote_ref = RR_NORMAL;
>     buf = strchr(...);
>     if (!buf)
>         return;
>
> or (even better) this:
>
>     buf = strchr(...);
>     if (!buf) {
>         atom->u.remote_ref = RR_NORMAL;
>         return;
>     }
>

Will do the latter, thanks.

>> +       buf++;
>> +       if (!strcmp(buf, "short"))
>> +               atom->u.remote_ref = RR_SHORTEN;
>> +       else if (!strcmp(buf, "track"))
>> +               atom->u.remote_ref = RR_TRACK;
>> +       else if (!strcmp(buf, "trackshort"))
>> +               atom->u.remote_ref = RR_TRACKSHORT;
>> +       else
>> +               die(_("unrecognized format: %%(%s)"), atom->str);
>> +}
>> +
>> @@ -835,6 +856,42 @@ static inline char *copy_advance(char *dst, const char *src)
>> +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;
>
> The RR_TRACKSHORT branch below has a blank line following the
> 'return', but this branch lacks it, which is inconsistent.
>

will add.

>> +               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))
>
> What happened to the assert(branch) which was in the original code
> from which this was derived (below)? Is it no longer needed?
>

stat_tracking_info() takes care of that, instead of aborting, we gracefully
continue while leaving that value empty.

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 10/11] ref-filter: introduce contents_atom_parser()

[Karthik Nayak]

On Thu, Dec 17, 2015 at 3:09 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Dec 16, 2015 at 10:30 AM, 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.
>>
>> Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -39,6 +39,10 @@ static struct used_atom {
>>                 struct align align;
>>                 enum { RR_SHORTEN, RR_TRACK, RR_TRACKSHORT, RR_NORMAL }
>>                         remote_ref;
>> +               struct {
>> +                       enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
>> +                       unsigned int no_lines;
>
> 'no_lines' sounds like "no lines!". How about 'nlines' instead?
>

Sure, will do.

>> +               } contents;
>>         } u;
>>  } *used_atom;
>>  static int used_atom_cnt, need_tagged, need_symref;
>> @@ -90,6 +94,36 @@ static void remote_ref_atom_parser(struct used_atom *atom)
>> +static void contents_atom_parser(struct used_atom *atom)
>> +{
>> +       const char * buf;
>> +
>> +       if (match_atom_name(atom->str, "contents", &buf))
>> +               atom->u.contents.option = C_BARE;
>> +       else if (match_atom_name(atom->str, "subject", &buf)) {
>
> The original code used strcmp() and matched only "subject", however
> the new code will incorrectly match both "subject" and
> "subject:whatever". Therefore, you should be using strcmp() here
> rather than match_atom_name().
>
> Ditto for "body".

Will change.

>
>> +               atom->u.contents.option = C_SUB;
>> +               return;
>> +       } else if (match_atom_name(atom->str, "body", &buf)) {
>> +               atom->u.contents.option = C_BODY_DEP;
>> +               return;
>> +       }
>> +       if (!buf)
>> +               return;
>
> It's not easy to see that this 'if (!buf)' check relates to the
> "contents" check at the very top of the if/else if/ chain since there
> are entirely unrelated checks in between. Reorganizing it can improve
> clarity:
>
>     if (!strcmp("subject")) {
>         ...
>         return;
>     } else if (!strcmp("body")) {
>         ...
>         return;
>     } else if (!match_atom_name(...,"contents", &buf))
>         die("BUG: expected 'contents' or 'contents:'");
>
>     if (!buf) {
>         atom->u.contents.option = C_BARE;
>         return;
>     }
>

This looks good. Thanks.

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 11/11] ref-filter: introduce objectname_atom_parser()

[Karthik Nayak]

On Fri, Dec 18, 2015 at 11:54 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Dec 16, 2015 at 10:30 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> 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>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>> diff --git 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 no_lines;
>>                 } contents;
>> +               enum { O_FULL, O_SHORT } objectname;
>>         } u;
>>  } *used_atom;
>> @@ -124,6 +125,21 @@ static void contents_atom_parser(struct used_atom *atom)
>> +static void objectname_atom_parser(struct used_atom *atom)
>> +{
>> +       const char * buf;
>> +
>> +       if (match_atom_name(atom->str, "objectname", &buf))
>> +               atom->u.objectname = O_FULL;
>> +       if (!buf)
>> +               return;
>
> Let me make sure that I understand this correctly.
>
> make_atom_name("objectname") will return true only for "objectname" or
> "objectname:", and will return false for anything else, such as
> "objectnamely" or "schmorf". Furthermore, the only way
> objectname_atom_parser() can be called is when %(objectname) or
> %(objectname:...) is seen, thus match_atom_name() *must* return true
> here, which means the above conditional is misleading, suggesting that
> it could somehow return false.
>
> And, if match_atom_name() did return false here, then that indicates a
> programming error: objectname_atom_parser() somehow got called for
> something other than %(objectname) or %(objectname:...). This implies
> that the code should instead be structured like this:
>
>     if (!match_atom_name(..., "objectname", &buf)
>         die("BUG: parsing non-'objectname'")
>     if (!buf)
>         atom->u.objectname = O_FULL;
>     else if (!strcmp(buf, "short"))
>         atom->u.objectname = O_SHORT;
>     else
>         die(_("unrecognized %%(objectname) argument: %s"), buf);
>
> However, this can be simplified further by recognizing that, following
> this patch series, match_atom_name() is *only* called by these new
> parse functions[1], which means that, as a convenience,
> match_atom_name() itself could become a void rather than boolean
> function and die() if the expected atom name is not found. Thus, the
> code would become:
>
>     match_atom_name(...);
>     if (!buf)
>         ...
>     else if (!strcmp(...))
>         ...
>     ...
>
> By the way, the above commentary isn't specific to this patch and
> %(objectname), but is in fact also relevant for all of the preceding
> patches which introduce parse functions calling match_atom_name().
>

Ah! Thats some good observation, makes sense, match_atom_name()
is only called after the atom name, so making it return a indication of
success or failure doesn't make sense.

I think this change would go nicely with the introduction of 'enum atom_type'
which you mentioned[0] in the previous series.

[0]: http://article.gmane.org/gmane.comp.version-control.git/282320

> More below...
>
> [1]: ...assuming you replace the unnecessary match_atom_name() in
> populate_value() with starts_with() as suggested in my patch 7/11
> review addendum[2].
>
> [2]: http://article.gmane.org/gmane.comp.version-control.git/282700
>
>> +
>> +       if (!strcmp(buf, "short"))
>> +               atom->u.objectname = O_SHORT;
>> +       else
>> +               die(_("unrecognized %%(objectname) argument: %s"), buf);
>> +}
>> +
>> @@ -461,15 +477,16 @@ 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;
>> +               }
>
> Since 'objectname' can only ever be O_SHORT or O_FULL wouldn't it be a
> programming error if it ever falls through to this point after the
> closing brace? Perhaps a die("BUG:...") would be appropriate?
>

Yeah, will add that in.

>>         }
>>         return 0;
>>  }
>> @@ -493,7 +510,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);
>> +                       grab_objectname(name, obj->sha1, v, &used_atom[i]);
>
> This patch hunk is somehow corrupt and doesn't apply. Was there some
> hand-editing involved, or were some earlier patches regenerated after
> this patch was made or something?
>

Nothing of that sort, weird.

>>         }
>>  }
>>
>> @@ -999,7 +1016,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.6.4



-- 
Regards,
Karthik Nayak

Re: [PATCH v2 11/11] ref-filter: introduce objectname_atom_parser()

[Eric Sunshine]

On Fri, Dec 25, 2015 at 8:44 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Fri, Dec 18, 2015 at 11:54 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Wed, Dec 16, 2015 at 10:30 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>> +static void objectname_atom_parser(struct used_atom *atom)
>>> +{
>>> +       const char * buf;
>>> +
>>> +       if (match_atom_name(atom->str, "objectname", &buf))
>>> +               atom->u.objectname = O_FULL;
>>> +       if (!buf)
>>> +               return;
>>
>> make_atom_name("objectname") will return true only for "objectname" or
>> "objectname:", and will return false for anything else, such as
>> "objectnamely" or "schmorf". Furthermore, the only way
>> objectname_atom_parser() can be called is when %(objectname) or
>> %(objectname:...) is seen, thus match_atom_name() *must* return true
>> here, which means the above conditional is misleading, suggesting that
>> it could somehow return false.
>>
>> And, if match_atom_name() did return false here, then that indicates a
>> programming error: objectname_atom_parser() somehow got called for
>> something other than %(objectname) or %(objectname:...). This implies
>> that the code should instead be structured like this:
>>
>>     if (!match_atom_name(..., "objectname", &buf)
>>         die("BUG: parsing non-'objectname'")
>>     if (!buf)
>>         atom->u.objectname = O_FULL;
>>     else if (!strcmp(buf, "short"))
>>         atom->u.objectname = O_SHORT;
>>     else
>>         die(_("unrecognized %%(objectname) argument: %s"), buf);
>>
>> However, this can be simplified further by recognizing that, following
>> this patch series, match_atom_name() is *only* called by these new
>> parse functions[1], which means that, as a convenience,
>> match_atom_name() itself could become a void rather than boolean
>> function and die() if the expected atom name is not found. Thus, the
>> code would become:
>>
>>     match_atom_name(...);
>>     if (!buf)
>>         ...
>>     else if (!strcmp(...))
>>         ...
>>     ...
>>
>> By the way, the above commentary isn't specific to this patch and
>> %(objectname), but is in fact also relevant for all of the preceding
>> patches which introduce parse functions calling match_atom_name().
>
> Ah! Thats some good observation, makes sense, match_atom_name()
> is only called after the atom name, so making it return a indication of
> success or failure doesn't make sense.
>
> I think this change would go nicely with the introduction of 'enum atom_type'
> which you mentioned[0] in the previous series.

I'm not sure to which change you refer as going nicely with the 'enum
atom_type'.

The observation above is about misleading logic in this series which
makes the code confusing. At the very least, the present series should
clean up the logic to reflect the first example above.

Changing match_atom_name() from boolean to void, as in the second
example above, is a nice little cleanup, but less important and
needn't be in this series (though it would just be one minor patch at
the end of the series).

Re: [PATCH v2 11/11] ref-filter: introduce objectname_atom_parser()

[Karthik Nayak]

On Fri, Dec 25, 2015 at 11:39 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Fri, Dec 25, 2015 at 8:44 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> On Fri, Dec 18, 2015 at 11:54 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Wed, Dec 16, 2015 at 10:30 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>>> +static void objectname_atom_parser(struct used_atom *atom)
>>>> +{
>>>> +       const char * buf;
>>>> +
>>>> +       if (match_atom_name(atom->str, "objectname", &buf))
>>>> +               atom->u.objectname = O_FULL;
>>>> +       if (!buf)
>>>> +               return;
>>>
>>> make_atom_name("objectname") will return true only for "objectname" or
>>> "objectname:", and will return false for anything else, such as
>>> "objectnamely" or "schmorf". Furthermore, the only way
>>> objectname_atom_parser() can be called is when %(objectname) or
>>> %(objectname:...) is seen, thus match_atom_name() *must* return true
>>> here, which means the above conditional is misleading, suggesting that
>>> it could somehow return false.
>>>
>>> And, if match_atom_name() did return false here, then that indicates a
>>> programming error: objectname_atom_parser() somehow got called for
>>> something other than %(objectname) or %(objectname:...). This implies
>>> that the code should instead be structured like this:
>>>
>>>     if (!match_atom_name(..., "objectname", &buf)
>>>         die("BUG: parsing non-'objectname'")
>>>     if (!buf)
>>>         atom->u.objectname = O_FULL;
>>>     else if (!strcmp(buf, "short"))
>>>         atom->u.objectname = O_SHORT;
>>>     else
>>>         die(_("unrecognized %%(objectname) argument: %s"), buf);
>>>
>>> However, this can be simplified further by recognizing that, following
>>> this patch series, match_atom_name() is *only* called by these new
>>> parse functions[1], which means that, as a convenience,
>>> match_atom_name() itself could become a void rather than boolean
>>> function and die() if the expected atom name is not found. Thus, the
>>> code would become:
>>>
>>>     match_atom_name(...);
>>>     if (!buf)
>>>         ...
>>>     else if (!strcmp(...))
>>>         ...
>>>     ...
>>>
>>> By the way, the above commentary isn't specific to this patch and
>>> %(objectname), but is in fact also relevant for all of the preceding
>>> patches which introduce parse functions calling match_atom_name().
>>
>> Ah! Thats some good observation, makes sense, match_atom_name()
>> is only called after the atom name, so making it return a indication of
>> success or failure doesn't make sense.
>>
>> I think this change would go nicely with the introduction of 'enum atom_type'
>> which you mentioned[0] in the previous series.
>
> I'm not sure to which change you refer as going nicely with the 'enum
> atom_type'.
>
> The observation above is about misleading logic in this series which
> makes the code confusing. At the very least, the present series should
> clean up the logic to reflect the first example above.
>
> Changing match_atom_name() from boolean to void, as in the second
> example above, is a nice little cleanup, but less important and
> needn't be in this series (though it would just be one minor patch at
> the end of the series).

I was referring to the second part as you just mentioned, But I could
at it to the
end of the series, I have restructured things as you mentioned in the example
though. Sorry for the confusion :)

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 08/11] ref-filter: introduce prefixes for the align atom

[Karthik Nayak]

On Thu, Dec 17, 2015 at 2:29 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> ref-filter: introduce prefixes for the align atom
>
> The prefixes are actually for the arguments to the 'align' atom, not
> for the atom itself. However, it might be better to describe this at a
> bit higher level. Perhaps:
>
>     ref-filter: align: introduce long-form syntax
>
> or something.

Makes sense, thanks.

>
>> Introduce optional prefixes "width=" and "position=" for the align atom
>> so that the atom can be used as "%(align:width=<width>,position=<position>)".
>>
>> Add Documetation and tests for the same.
>>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -96,10 +96,19 @@ static void align_atom_parser(struct used_atom *atom)
>>
>>         while (*s) {
>>                 int position;
>> +               buf = s[0]->buf;
>
> It probably would be better to do this assignment in the previous
> patch (7/11) since its presence here introduces unwanted noise
> (textual replacement of "s[0]->buf" with "buf") in several locations
> below which slightly obscure the real changes of this patch.
>

This makes sense from a reviewers perspective, will do.

>> -               if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))
>> +               if (skip_prefix(buf, "position=", &buf)) {
>> +                       position = parse_align_position(buf);
>> +                       if (position == -1)
>
> It may be more idiomatic in this codebase to detect errors via '< 0'
> rather than explicit '== -1'. Ditto below.
>

I think Junio also mentioned this once. Thanks for reminding.

>> +                               die(_("unrecognized position:%s"), buf);
>> +                       align->position = position;
>> +               } else if (skip_prefix(buf, "width=", &buf)) {
>> +                       if (strtoul_ui(buf, 10, (unsigned int *)&width))
>> +                               die(_("unrecognized width:%s"), buf);
>> +               } else if (!strtoul_ui(buf, 10, (unsigned int *)&width))
>>                         ;
>> -               else if ((position = parse_align_position(s[0]->buf)) > 0)
>> +               else if ((position = parse_align_position(buf)) != -1)
>>                         align->position = position;
>>                 else
>>                         die(_("unrecognized %%(align) argument: %s"), s[0]->buf);
>
> s/s[0]->//
>

Thanks.


> More below...
>
>> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
>> @@ -133,6 +133,168 @@ test_expect_success 'right alignment' '
>>         test_cmp expect actual
>>  '
>>
>> +test_expect_success 'alignment with "position" prefix' '
>> +       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
>> +       git for-each-ref --format="|%(align:30,position=right)refname is %(refname)%(end)|%(refname)" >actual &&
>> +       test_cmp expect actual
>> +'
>> +
>> +test_expect_success 'alignment with "position" prefix' '
>> +       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
>> +       git for-each-ref --format="|%(align:position=right,30)refname is %(refname)%(end)|%(refname)" >actual &&
>> +       test_cmp expect actual
>> +'
>
> This (and below) is a lot of copy/paste code which makes it difficult
> to read the tests and maintain (change) them. Since 'expect' doesn't
> appear to change from test to test, one way to eliminate some of this
> noisiness would be to create 'expect' once outside of the tests.
>
> However, even better, especially from a comprehension,
> maintainability, and extensibility standpoints would be to make this
> all table-driven. In particular, I'd expect to see a table with
> exactly the list of test inputs mentioned in my earlier review[1], and
> have that table passed to a shell function which performs the test for
> each element of the table. For instance, something like:
>
>     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
>
> where test_align_permutations is the name of the shell function which
> reads each line of it stdin and performs the "git for-each-ref
> --format=..." test with the given %(align:) arguments.
>
> Ditto regarding the below "last one wins (silently) tests".
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/281916
>

This seems like a good idea, I implemented both of those together.

test_align_permutations() {
while read -r option; do
test_expect_success 'align permutations' '
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

Thanks :)

--
Regards,
Karthik Nayak