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

[PATCH v6 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.

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

Changes:
* As per Jeff's suggestion ($gmane/286425) we drop creation and
usage of strbuf_split_str_omit_term() and use string_list_split()
instead.

Jeff King (1):
  ref-filter: use string_list_split over strbuf_split

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

 Documentation/git-for-each-ref.txt |  20 +-
 ref-filter.c                       | 434 +++++++++++++++++++++----------------
 t/t6302-for-each-ref-filter.sh     |  42 ++++
 3 files changed, 304 insertions(+), 192 deletions(-)

Interdiff:

diff --git a/ref-filter.c b/ref-filter.c
index 21f4937..d13d002 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -127,18 +127,19 @@ static align_type parse_align_position(const char *s)
 static void align_atom_parser(struct used_atom *atom, const char *arg)
 {
 	struct align *align = &atom->u.align;
-	struct strbuf **v, **to_free;
+	struct string_list params = STRING_LIST_INIT_DUP;
+	int i;
 	unsigned int width = ~0U;
 
 	if (!arg)
 		die(_("expected format: %%(align:<width>,<position>)"));
-	v = to_free = strbuf_split_str_omit_term(arg, ',', 0);
 
 	align->position = ALIGN_LEFT;
 
-	while (*v) {
+	string_list_split(&params, arg, ',', -1);
+	for (i = 0; i < params.nr; i++) {
+		const char *s = params.items[i].string;
 		int position;
-		const char *s = v[0]->buf;
 
 		if (skip_prefix(s, "position=", &s)) {
 			position = parse_align_position(s);
@@ -154,13 +155,12 @@ static void align_atom_parser(struct used_atom *atom, const char *arg)
 			align->position = position;
 		else
 			die(_("unrecognized %%(align) argument: %s"), s);
-		v++;
 	}
 
 	if (width == ~0U)
 		die(_("positive width expected with the %%(align) atom"));
 	align->width = width;
-	strbuf_list_free(to_free);
+	string_list_clear(&params, 0);
 }
 
 static struct {
diff --git a/strbuf.c b/strbuf.c
index 4a93e2a..bab316d 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 omit_term)
+				 int terminator, int max)
 {
 	struct strbuf **ret = NULL;
 	size_t nr = 0, alloc = 0;
@@ -123,18 +123,14 @@ struct strbuf **strbuf_split_buf(const char *str, size_t slen,
 
 	while (slen) {
 		int len = slen;
-		int copylen = len;
-		const char *end = NULL;
 		if (max <= 0 || nr + 1 < max) {
-			end = memchr(str, terminator, slen);
-			if (end) {
+			const char *end = memchr(str, terminator, slen);
+			if (end)
 				len = end - str + 1;
-				copylen = len - !!omit_term;
-			}
 		}
 		t = xmalloc(sizeof(struct strbuf));
-		strbuf_init(t, copylen);
-		strbuf_add(t, str, copylen);
+		strbuf_init(t, len);
+		strbuf_add(t, str, len);
 		ALLOC_GROW(ret, nr + 2, alloc);
 		ret[nr++] = t;
 		str += len;
diff --git a/strbuf.h b/strbuf.h
index 6115e72..f72fd14 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -466,12 +466,11 @@ 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.  If omit_term is true, the terminator will
- * be stripped from all substrings. Otherwise, substrings will include
- * the terminator, except for the final substring, if the original
- * string lacked a terminator.  If max is positive, then split the
- * string into at most max substrings (with the last substring
- * containing everything following the (max-1)th terminator
+ * 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
  * character).
  *
  * The most generic form is `strbuf_split_buf`, which takes an arbitrary
@@ -482,25 +481,19 @@ 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 *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);
-}
+extern struct strbuf **strbuf_split_buf(const char *, size_t,
+					int terminator, int max);
 
 static inline struct strbuf **strbuf_split_str(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);
 }
 
 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, 0);
+	return strbuf_split_buf(sb->buf, sb->len, terminator, max);
 }
 
 static inline struct strbuf **strbuf_split(const struct strbuf *sb,


-- 
2.7.1

[PATCH v6 01/11] ref-filter: use string_list_split over strbuf_split

[Karthik Nayak]

From: Jeff King <peff@peff.net>

We don't do any post-processing on the resulting strbufs, so it is
simpler to just use string_list_split, which takes care of removing
the delimiter for us.

Written-by: Jeff King <peff@peff.net>
Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
---
 ref-filter.c | 29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index f097176..19367ce 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -886,41 +886,34 @@ static void populate_value(struct ref_array_item *ref)
 			continue;
 		} else if (match_atom_name(name, "align", &valp)) {
 			struct align *align = &v->u.align;
-			struct strbuf **s, **to_free;
+			struct string_list params = STRING_LIST_INIT_DUP;
+			int i;
 			int width = -1;
 
 			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);
-
 			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))
+			string_list_split(&params, valp, ',', -1);
+			for (i = 0; i < params.nr; i++) {
+				const char *s = params.items[i].string;
+				if (!strtoul_ui(s, 10, (unsigned int *)&width))
 					;
-				else if (!strcmp(s[0]->buf, "left"))
+				else if (!strcmp(s, "left"))
 					align->position = ALIGN_LEFT;
-				else if (!strcmp(s[0]->buf, "right"))
+				else if (!strcmp(s, "right"))
 					align->position = ALIGN_RIGHT;
-				else if (!strcmp(s[0]->buf, "middle"))
+				else if (!strcmp(s, "middle"))
 					align->position = ALIGN_MIDDLE;
 				else
-					die(_("improper format entered align:%s"), s[0]->buf);
-				s++;
+					die(_("improper format entered align:%s"), s);
 			}
 
 			if (width < 0)
 				die(_("positive width expected with the %%(align) atom"));
 			align->width = width;
-			strbuf_list_free(to_free);
+			string_list_clear(&params, 0);
 			v->handler = align_atom_handler;
 			continue;
 		} else if (!strcmp(name, "end")) {
-- 
2.7.1

[PATCH v6 02/11] ref-filter: bump 'used_atom' and related code to the top

[Karthik Nayak]

Bump code to the top for usage in further patches.

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

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

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

[Karthik Nayak]

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

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

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

[PATCH v6 04/11] ref-filter: introduce parsing functions for each valid atom

[Karthik Nayak]

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

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Helped-by: Andreas Schwab <schwab@linux-m68k.org>
Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
---
 ref-filter.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 8139709..8a34ba1 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -36,6 +36,7 @@ static int need_color_reset_at_eol;
 static struct {
 	const char *name;
 	cmp_type cmp_type;
+	void (*parser)(struct used_atom *atom, const char *arg);
 } valid_atom[] = {
 	{ "refname" },
 	{ "objecttype" },
@@ -114,6 +115,7 @@ struct atom_value {
 int parse_ref_filter_atom(const char *atom, const char *ep)
 {
 	const char *sp;
+	const char *arg;
 	int i, at;
 
 	sp = atom;
@@ -132,16 +134,16 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
 	/* Is the atom a valid one? */
 	for (i = 0; i < ARRAY_SIZE(valid_atom); i++) {
 		int len = strlen(valid_atom[i].name);
+
 		/*
 		 * If the atom name has a colon, strip it and everything after
 		 * it off - it specifies the format for this entry, and
 		 * shouldn't be used for checking against the valid_atom
 		 * table.
 		 */
-		const char *formatp = strchr(sp, ':');
-		if (!formatp || ep < formatp)
-			formatp = ep;
-		if (len == formatp - sp && !memcmp(valid_atom[i].name, sp, len))
+		arg = memchr(sp, ':', ep - sp);
+		if (len == (arg ? arg : ep) - sp &&
+		    !memcmp(valid_atom[i].name, sp, len))
 			break;
 	}
 
@@ -154,6 +156,10 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
 	REALLOC_ARRAY(used_atom, used_atom_cnt);
 	used_atom[at].name = xmemdupz(atom, ep - atom);
 	used_atom[at].type = valid_atom[i].cmp_type;
+	if (arg)
+		arg = used_atom[at].name + (arg - atom) + 1;
+	if (valid_atom[i].parser)
+		valid_atom[i].parser(&used_atom[at], arg);
 	if (*atom == '*')
 		need_tagged = 1;
 	if (!strcmp(used_atom[at].name, "symref"))
-- 
2.7.1

[PATCH v6 05/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 "used_atom" structure for further usage in
populate_value().

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

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

[PATCH v6 06/11] ref-filter: introduce parse_align_position()

[Karthik Nayak]

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

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

diff --git a/ref-filter.c b/ref-filter.c
index c90d2f4..e8b076d 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -44,6 +44,17 @@ static void color_atom_parser(struct used_atom *atom, const char *color_value)
 		die(_("unrecognized color: %%(color:%s)"), color_value);
 }
 
+static align_type parse_align_position(const char *s)
+{
+	if (!strcmp(s, "right"))
+		return ALIGN_RIGHT;
+	else if (!strcmp(s, "middle"))
+		return ALIGN_MIDDLE;
+	else if (!strcmp(s, "left"))
+		return ALIGN_LEFT;
+	return -1;
+}
+
 static struct {
 	const char *name;
 	cmp_type cmp_type;
@@ -912,14 +923,12 @@ static void populate_value(struct ref_array_item *ref)
 			string_list_split(&params, valp, ',', -1);
 			for (i = 0; i < params.nr; i++) {
 				const char *s = params.items[i].string;
+				int position;
+
 				if (!strtoul_ui(s, 10, (unsigned int *)&width))
 					;
-				else if (!strcmp(s, "left"))
-					align->position = ALIGN_LEFT;
-				else if (!strcmp(s, "right"))
-					align->position = ALIGN_RIGHT;
-				else if (!strcmp(s, "middle"))
-					align->position = ALIGN_MIDDLE;
+				else if ((position = parse_align_position(s)) >= 0)
+					align->position = position;
 				else
 					die(_("improper format entered align:%s"), s);
 			}
-- 
2.7.1

[PATCH v6 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 'used_atom'
structure for further usage in populate_value().

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

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

diff --git a/ref-filter.c b/ref-filter.c
index e8b076d..797f9fe 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -16,6 +16,11 @@
 
 typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 
+struct align {
+	align_type position;
+	unsigned int width;
+};
+
 /*
  * An atom is a valid field atom listed below, possibly prefixed with
  * a "*" to denote deref_tag().
@@ -31,6 +36,7 @@ static struct used_atom {
 	cmp_type type;
 	union {
 		char color[COLOR_MAXLEN];
+		struct align align;
 	} u;
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
@@ -55,6 +61,37 @@ static align_type parse_align_position(const char *s)
 	return -1;
 }
 
+static void align_atom_parser(struct used_atom *atom, const char *arg)
+{
+	struct align *align = &atom->u.align;
+	struct string_list params = STRING_LIST_INIT_DUP;
+	int i;
+	unsigned int width = ~0U;
+
+	if (!arg)
+		die(_("expected format: %%(align:<width>,<position>)"));
+
+	align->position = ALIGN_LEFT;
+
+	string_list_split(&params, arg, ',', -1);
+	for (i = 0; i < params.nr; i++) {
+		const char *s = params.items[i].string;
+		int position;
+
+		if (!strtoul_ui(s, 10, &width))
+			;
+		else if ((position = parse_align_position(s)) >= 0)
+			align->position = position;
+		else
+			die(_("unrecognized %%(align) argument: %s"), s);
+	}
+
+	if (width == ~0U)
+		die(_("positive width expected with the %%(align) atom"));
+	align->width = width;
+	string_list_clear(&params, 0);
+}
+
 static struct {
 	const char *name;
 	cmp_type cmp_type;
@@ -93,17 +130,12 @@ static struct {
 	{ "flag" },
 	{ "HEAD" },
 	{ "color", FIELD_STR, color_atom_parser },
-	{ "align" },
+	{ "align", FIELD_STR, align_atom_parser },
 	{ "end" },
 };
 
 #define REF_FORMATTING_STATE_INIT  { 0, NULL }
 
-struct align {
-	align_type position;
-	unsigned int width;
-};
-
 struct contents {
 	unsigned int lines;
 	struct object_id oid;
@@ -288,22 +320,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).
  */
@@ -845,7 +861,6 @@ static void populate_value(struct ref_array_item *ref)
 		int deref = 0;
 		const char *refname;
 		const char *formatp;
-		const char *valp;
 		struct branch *branch = NULL;
 
 		v->handler = append_atom;
@@ -909,34 +924,8 @@ static void populate_value(struct ref_array_item *ref)
 			else
 				v->s = " ";
 			continue;
-		} else if (match_atom_name(name, "align", &valp)) {
-			struct align *align = &v->u.align;
-			struct string_list params = STRING_LIST_INIT_DUP;
-			int i;
-			int width = -1;
-
-			if (!valp)
-				die(_("expected format: %%(align:<width>,<position>)"));
-
-			align->position = ALIGN_LEFT;
-
-			string_list_split(&params, valp, ',', -1);
-			for (i = 0; i < params.nr; i++) {
-				const char *s = params.items[i].string;
-				int position;
-
-				if (!strtoul_ui(s, 10, (unsigned int *)&width))
-					;
-				else if ((position = parse_align_position(s)) >= 0)
-					align->position = position;
-				else
-					die(_("improper format entered align:%s"), s);
-			}
-
-			if (width < 0)
-				die(_("positive width expected with the %%(align) atom"));
-			align->width = width;
-			string_list_clear(&params, 0);
+		} else if (starts_with(name, "align")) {
+			v->u.align = atom->u.align;
 			v->handler = align_atom_handler;
 			continue;
 		} else if (!strcmp(name, "end")) {
-- 
2.7.1

[PATCH v6 08/11] ref-filter: align: introduce long-form syntax

[Karthik Nayak]

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

Add Documentation and tests for the same.

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

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

[PATCH v6 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>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
---
 ref-filter.c | 103 +++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 61 insertions(+), 42 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 2255dcc..7df5085 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -37,6 +37,8 @@ static struct used_atom {
 	union {
 		char color[COLOR_MAXLEN];
 		struct align align;
+		enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }
+			remote_ref;
 	} u;
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
@@ -50,6 +52,20 @@ static void color_atom_parser(struct used_atom *atom, const char *color_value)
 		die(_("unrecognized color: %%(color:%s)"), color_value);
 }
 
+static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
+{
+	if (!arg)
+		atom->u.remote_ref = RR_NORMAL;
+	else if (!strcmp(arg, "short"))
+		atom->u.remote_ref = RR_SHORTEN;
+	else if (!strcmp(arg, "track"))
+		atom->u.remote_ref = RR_TRACK;
+	else if (!strcmp(arg, "trackshort"))
+		atom->u.remote_ref = RR_TRACKSHORT;
+	else
+		die(_("unrecognized format: %%(%s)"), atom->name);
+}
+
 static align_type parse_align_position(const char *s)
 {
 	if (!strcmp(s, "right"))
@@ -132,8 +148,8 @@ static struct {
 	{ "subject" },
 	{ "body" },
 	{ "contents" },
-	{ "upstream" },
-	{ "push" },
+	{ "upstream", FIELD_STR, remote_ref_atom_parser },
+	{ "push", FIELD_STR, remote_ref_atom_parser },
 	{ "symref" },
 	{ "flag" },
 	{ "HEAD" },
@@ -840,6 +856,43 @@ static const char *strip_ref_components(const char *refname, const char *nr_arg)
 	return start;
 }
 
+static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
+				    struct branch *branch, const char **s)
+{
+	int num_ours, num_theirs;
+	if (atom->u.remote_ref == RR_SHORTEN)
+		*s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);
+	else if (atom->u.remote_ref == RR_TRACK) {
+		if (stat_tracking_info(branch, &num_ours,
+				       &num_theirs, NULL))
+			return;
+
+		if (!num_ours && !num_theirs)
+			*s = "";
+		else if (!num_ours)
+			*s = xstrfmt("[behind %d]", num_theirs);
+		else if (!num_theirs)
+			*s = xstrfmt("[ahead %d]", num_ours);
+		else
+			*s = xstrfmt("[ahead %d, behind %d]",
+				     num_ours, num_theirs);
+	} else if (atom->u.remote_ref == RR_TRACKSHORT) {
+		if (stat_tracking_info(branch, &num_ours,
+				       &num_theirs, NULL))
+			return;
+
+		if (!num_ours && !num_theirs)
+			*s = "=";
+		else if (!num_ours)
+			*s = "<";
+		else if (!num_theirs)
+			*s = ">";
+		else
+			*s = "<>";
+	} else /* RR_NORMAL */
+		*s = refname;
+}
+
 /*
  * Parse the object referred by ref, and grab needed value.
  */
@@ -891,8 +944,9 @@ static void populate_value(struct ref_array_item *ref)
 			branch = branch_get(branch_name);
 
 			refname = branch_get_upstream(branch, NULL);
-			if (!refname)
-				continue;
+			if (refname)
+				fill_remote_ref_details(atom, refname, branch, &v->s);
+			continue;
 		} else if (starts_with(name, "push")) {
 			const char *branch_name;
 			if (!skip_prefix(ref->refname, "refs/heads/",
@@ -903,6 +957,8 @@ static void populate_value(struct ref_array_item *ref)
 			refname = branch_get_push(branch, NULL);
 			if (!refname)
 				continue;
+			fill_remote_ref_details(atom, refname, branch, &v->s);
+			continue;
 		} else if (starts_with(name, "color:")) {
 			v->s = atom->u.color;
 			continue;
@@ -944,7 +1000,6 @@ static void populate_value(struct ref_array_item *ref)
 
 		formatp = strchr(name, ':');
 		if (formatp) {
-			int num_ours, num_theirs;
 			const char *arg;
 
 			formatp++;
@@ -953,43 +1008,7 @@ static void populate_value(struct ref_array_item *ref)
 						      warn_ambiguous_refs);
 			else if (skip_prefix(formatp, "strip=", &arg))
 				refname = strip_ref_components(refname, arg);
-			else if (!strcmp(formatp, "track") &&
-				 (starts_with(name, "upstream") ||
-				  starts_with(name, "push"))) {
-
-				if (stat_tracking_info(branch, &num_ours,
-						       &num_theirs, NULL))
-					continue;
-
-				if (!num_ours && !num_theirs)
-					v->s = "";
-				else if (!num_ours)
-					v->s = xstrfmt("[behind %d]", num_theirs);
-				else if (!num_theirs)
-					v->s = xstrfmt("[ahead %d]", num_ours);
-				else
-					v->s = xstrfmt("[ahead %d, behind %d]",
-						       num_ours, num_theirs);
-				continue;
-			} else if (!strcmp(formatp, "trackshort") &&
-				   (starts_with(name, "upstream") ||
-				    starts_with(name, "push"))) {
-				assert(branch);
-
-				if (stat_tracking_info(branch, &num_ours,
-							&num_theirs, NULL))
-					continue;
-
-				if (!num_ours && !num_theirs)
-					v->s = "=";
-				else if (!num_ours)
-					v->s = "<";
-				else if (!num_theirs)
-					v->s = ">";
-				else
-					v->s = "<>";
-				continue;
-			} else
+			else
 				die("unknown %.*s format %s",
 				    (int)(formatp - name), name, formatp);
 		}
-- 
2.7.1

[PATCH v6 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. Also introduce body_atom_parser()
and subject_atom_parser() for parsing atoms '%(body)' and '%(subject)'
respectively.

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

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

[PATCH v6 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>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
---
 ref-filter.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

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

Re: [PATCH v6 01/11] ref-filter: use string_list_split over strbuf_split

[Eric Sunshine]

On Wed, Feb 17, 2016 at 1:06 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> From: Jeff King <peff@peff.net>
>
> We don't do any post-processing on the resulting strbufs, so it is
> simpler to just use string_list_split, which takes care of removing
> the delimiter for us.
>
> Written-by: Jeff King <peff@peff.net>

Perhaps Peff can give his sign-off...

> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
>  ref-filter.c | 29 +++++++++++------------------
>  1 file changed, 11 insertions(+), 18 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index f097176..19367ce 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -886,41 +886,34 @@ static void populate_value(struct ref_array_item *ref)
>                         continue;
>                 } else if (match_atom_name(name, "align", &valp)) {
>                         struct align *align = &v->u.align;
> -                       struct strbuf **s, **to_free;
> +                       struct string_list params = STRING_LIST_INIT_DUP;
> +                       int i;
>                         int width = -1;
>
>                         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);
> -
>                         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))
> +                       string_list_split(&params, valp, ',', -1);
> +                       for (i = 0; i < params.nr; i++) {
> +                               const char *s = params.items[i].string;
> +                               if (!strtoul_ui(s, 10, (unsigned int *)&width))
>                                         ;
> -                               else if (!strcmp(s[0]->buf, "left"))
> +                               else if (!strcmp(s, "left"))
>                                         align->position = ALIGN_LEFT;
> -                               else if (!strcmp(s[0]->buf, "right"))
> +                               else if (!strcmp(s, "right"))
>                                         align->position = ALIGN_RIGHT;
> -                               else if (!strcmp(s[0]->buf, "middle"))
> +                               else if (!strcmp(s, "middle"))
>                                         align->position = ALIGN_MIDDLE;
>                                 else
> -                                       die(_("improper format entered align:%s"), s[0]->buf);
> -                               s++;
> +                                       die(_("improper format entered align:%s"), s);
>                         }
>
>                         if (width < 0)
>                                 die(_("positive width expected with the %%(align) atom"));
>                         align->width = width;
> -                       strbuf_list_free(to_free);
> +                       string_list_clear(&params, 0);
>                         v->handler = align_atom_handler;
>                         continue;
>                 } else if (!strcmp(name, "end")) {
> --
> 2.7.1

Re: [PATCH v6 01/11] ref-filter: use string_list_split over strbuf_split

[Jeff King]

On Wed, Feb 17, 2016 at 05:11:50PM -0500, Eric Sunshine wrote:

> On Wed, Feb 17, 2016 at 1:06 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> > From: Jeff King <peff@peff.net>
> >
> > We don't do any post-processing on the resulting strbufs, so it is
> > simpler to just use string_list_split, which takes care of removing
> > the delimiter for us.
> >
> > Written-by: Jeff King <peff@peff.net>
> 
> Perhaps Peff can give his sign-off...

Ah, right. I usually sign-off when sending to the list, so the version
he pulled from GitHub didn't have it.

Definitely:

  Signed-off-by: Jeff King <peff@peff.net>

And I don't think "Written-by" was doing much here, anyway; I am already
the author by the From header at the top. :)

-Peff

> 
> > Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> > ---
> >  ref-filter.c | 29 +++++++++++------------------
> >  1 file changed, 11 insertions(+), 18 deletions(-)
> >
> > diff --git a/ref-filter.c b/ref-filter.c
> > index f097176..19367ce 100644
> > --- a/ref-filter.c
> > +++ b/ref-filter.c
> > @@ -886,41 +886,34 @@ static void populate_value(struct ref_array_item *ref)
> >                         continue;
> >                 } else if (match_atom_name(name, "align", &valp)) {
> >                         struct align *align = &v->u.align;
> > -                       struct strbuf **s, **to_free;
> > +                       struct string_list params = STRING_LIST_INIT_DUP;
> > +                       int i;
> >                         int width = -1;
> >
> >                         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);
> > -
> >                         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))
> > +                       string_list_split(&params, valp, ',', -1);
> > +                       for (i = 0; i < params.nr; i++) {
> > +                               const char *s = params.items[i].string;
> > +                               if (!strtoul_ui(s, 10, (unsigned int *)&width))
> >                                         ;
> > -                               else if (!strcmp(s[0]->buf, "left"))
> > +                               else if (!strcmp(s, "left"))
> >                                         align->position = ALIGN_LEFT;
> > -                               else if (!strcmp(s[0]->buf, "right"))
> > +                               else if (!strcmp(s, "right"))
> >                                         align->position = ALIGN_RIGHT;
> > -                               else if (!strcmp(s[0]->buf, "middle"))
> > +                               else if (!strcmp(s, "middle"))
> >                                         align->position = ALIGN_MIDDLE;
> >                                 else
> > -                                       die(_("improper format entered align:%s"), s[0]->buf);
> > -                               s++;
> > +                                       die(_("improper format entered align:%s"), s);
> >                         }
> >
> >                         if (width < 0)
> >                                 die(_("positive width expected with the %%(align) atom"));
> >                         align->width = width;
> > -                       strbuf_list_free(to_free);
> > +                       string_list_clear(&params, 0);
> >                         v->handler = align_atom_handler;
> >                         continue;
> >                 } else if (!strcmp(name, "end")) {
> > --
> > 2.7.1

Re: [PATCH v6 01/11] ref-filter: use string_list_split over strbuf_split

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Feb 17, 2016 at 05:11:50PM -0500, Eric Sunshine wrote:
>
>> On Wed, Feb 17, 2016 at 1:06 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> > From: Jeff King <peff@peff.net>
>> >
>> > We don't do any post-processing on the resulting strbufs, so it is
>> > simpler to just use string_list_split, which takes care of removing
>> > the delimiter for us.
>> >
>> > Written-by: Jeff King <peff@peff.net>
>> 
>> Perhaps Peff can give his sign-off...
>
> Ah, right. I usually sign-off when sending to the list, so the version
> he pulled from GitHub didn't have it.
>
> Definitely:
>
>   Signed-off-by: Jeff King <peff@peff.net>
>
> And I don't think "Written-by" was doing much here, anyway; I am already
> the author by the From header at the top. :)

;-).

So, is everybody happy with this round?

With another series on top for the "conditional" stuff, I guess we
are ready to do the formatting for "git branch --list", which would
be a big step forward.

Thanks.

Re: [PATCH v6 01/11] ref-filter: use string_list_split over strbuf_split

[Eric Sunshine]

On Wed, Feb 17, 2016 at 5:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>> On Wed, Feb 17, 2016 at 05:11:50PM -0500, Eric Sunshine wrote:
>>> On Wed, Feb 17, 2016 at 1:06 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>> > From: Jeff King <peff@peff.net>
>>> >
>>> > We don't do any post-processing on the resulting strbufs, so it is
>>> > simpler to just use string_list_split, which takes care of removing
>>> > the delimiter for us.
>>> >
>>> > Written-by: Jeff King <peff@peff.net>
>>>
>>> Perhaps Peff can give his sign-off...
>>
>> Ah, right. I usually sign-off when sending to the list, so the version
>> he pulled from GitHub didn't have it.
>>
>> Definitely:
>>
>>   Signed-off-by: Jeff King <peff@peff.net>
>>
>> And I don't think "Written-by" was doing much here, anyway; I am already
>> the author by the From header at the top. :)
>
> ;-).
>
> So, is everybody happy with this round?

Yes, v6 looks good to me and is:

Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>

Re: [PATCH v6 01/11] ref-filter: use string_list_split over strbuf_split

[Jeff King]

On Wed, Feb 17, 2016 at 02:19:39PM -0800, Junio C Hamano wrote:

> So, is everybody happy with this round?
> 
> With another series on top for the "conditional" stuff, I guess we
> are ready to do the formatting for "git branch --list", which would
> be a big step forward.

I have not looked with nearly as close an eye as Eric, but I did not see
anything objectionable (and I trust the reviews that have led us up to
v6 in the first place).

Thanks, Karthik, for your continued work on this (and to reviewers, of
course :) ).

-Peff

Re: [PATCH v6 01/11] ref-filter: use string_list_split over strbuf_split

[Karthik Nayak]

On Thu, Feb 18, 2016 at 3:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> On Wed, Feb 17, 2016 at 05:11:50PM -0500, Eric Sunshine wrote:
>>
>>> On Wed, Feb 17, 2016 at 1:06 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>> > From: Jeff King <peff@peff.net>
>>> >
>>> > We don't do any post-processing on the resulting strbufs, so it is
>>> > simpler to just use string_list_split, which takes care of removing
>>> > the delimiter for us.
>>> >
>>> > Written-by: Jeff King <peff@peff.net>
>>>
>>> Perhaps Peff can give his sign-off...
>>
>> Ah, right. I usually sign-off when sending to the list, so the version
>> he pulled from GitHub didn't have it.
>>
>> Definitely:
>>
>>   Signed-off-by: Jeff King <peff@peff.net>
>>
>> And I don't think "Written-by" was doing much here, anyway; I am already
>> the author by the From header at the top. :)
>
> ;-).
>
> So, is everybody happy with this round?
>
> With another series on top for the "conditional" stuff, I guess we
> are ready to do the formatting for "git branch --list", which would
> be a big step forward.
>
> Thanks.

Yes, I'll be sending that soon :D

> Yes, v6 looks good to me and is:
>
> Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>

> I have not looked with nearly as close an eye as Eric, but I did not see
> anything objectionable (and I trust the reviews that have led us up to
> v6 in the first place).
>
> Thanks, Karthik, for your continued work on this (and to reviewers, of
> course :) ).
>
> -Peff

Thanks to everyone who helped, especially reviewers for going through such
long patch series.

-- 
Regards,
Karthik Nayak