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

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

[Karthik Nayak]

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

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

Changes:
* Fixed small errors in multiple 'die(..)' messages.
* Removed unecessary braces.
* In parse_align_position() use 'v', 's' to denote the vector of arguments
and individual argument respectively rather than re-using 'arg'.
* Fix error in parse_ref_filter_atom() where length of current atom wasn't
accurately calculated.
* Small code and indentation fixes

Thanks to Eric, Junio, Ramsay and Andreas for their comments on the previous
version. And everyone else who helped review the previous patch series.

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

 Documentation/git-for-each-ref.txt |  20 +-
 ref-filter.c                       | 434 +++++++++++++++++++++----------------
 strbuf.c                           |  14 +-
 strbuf.h                           |  25 ++-
 t/t6302-for-each-ref-filter.sh     |  42 ++++
 5 files changed, 329 insertions(+), 206 deletions(-)

Interdiff:

diff --git a/ref-filter.c b/ref-filter.c
index d48e2a3..21f4937 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -54,14 +54,14 @@ static void color_atom_parser(struct used_atom *atom, const char *color_value)
 	if (!color_value)
 		die(_("expected format: %%(color:<color>)"));
 	if (color_parse(color_value, atom->u.color) < 0)
-		die(_("invalid color value: %s"), atom->u.color);
+		die(_("unrecognized color: %%(color:%s)"), color_value);
 }
 
 static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
 {
-	if (!arg) {
+	if (!arg)
 		atom->u.remote_ref = RR_NORMAL;
-	} else if (!strcmp(arg, "short"))
+	else if (!strcmp(arg, "short"))
 		atom->u.remote_ref = RR_SHORTEN;
 	else if (!strcmp(arg, "track"))
 		atom->u.remote_ref = RR_TRACK;
@@ -74,14 +74,14 @@ static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
 static void body_atom_parser(struct used_atom *atom, const char *arg)
 {
 	if (arg)
-		die("%%(body) atom does not take arguments");
+		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) atom does not take arguments");
+		die("%%(subject) does not take arguments");
 	atom->u.contents.option = C_SUB;
 }
 
@@ -127,34 +127,34 @@ static align_type parse_align_position(const char *s)
 static void align_atom_parser(struct used_atom *atom, const char *arg)
 {
 	struct align *align = &atom->u.align;
-	struct strbuf **s, **to_free;
+	struct strbuf **v, **to_free;
 	unsigned int width = ~0U;
 
 	if (!arg)
 		die(_("expected format: %%(align:<width>,<position>)"));
-	s = to_free = strbuf_split_str_omit_term(arg, ',', 0);
+	v = to_free = strbuf_split_str_omit_term(arg, ',', 0);
 
 	align->position = ALIGN_LEFT;
 
-	while (*s) {
+	while (*v) {
 		int position;
-		arg = s[0]->buf;
+		const char *s = v[0]->buf;
 
-		if (skip_prefix(arg, "position=", &arg)) {
-			position = parse_align_position(arg);
+		if (skip_prefix(s, "position=", &s)) {
+			position = parse_align_position(s);
 			if (position < 0)
-				die(_("unrecognized position:%s"), arg);
+				die(_("unrecognized position:%s"), s);
 			align->position = position;
-		} else if (skip_prefix(arg, "width=", &arg)) {
-			if (strtoul_ui(arg, 10, &width))
-				die(_("unrecognized width:%s"), arg);
-		} else if (!strtoul_ui(arg, 10, &width))
+		} else if (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(arg)) >= 0)
+		else if ((position = parse_align_position(s)) >= 0)
 			align->position = position;
 		else
-			die(_("unrecognized %%(align) argument: %s"), arg);
-		s++;
+			die(_("unrecognized %%(align) argument: %s"), s);
+		v++;
 	}
 
 	if (width == ~0U)
@@ -253,6 +253,7 @@ 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
@@ -260,7 +261,7 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
 		 * table.
 		 */
 		arg = memchr(sp, ':', ep - sp);
-		if ((!arg || len == arg - sp) &&
+		if (len == (arg ? arg : ep) - sp &&
 		    !memcmp(valid_atom[i].name, sp, len))
 			break;
 	}
@@ -775,8 +776,8 @@ 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];
 		if (!!deref != (*name == '*'))
 			continue;
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index f79355d..bcf472b 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -149,8 +149,8 @@ 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
+			git for-each-ref --format="|%(align:$option)refname is %(refname)%(end)|%(refname)" >actual &&
+			test_cmp expect actual
 		'
 	done
 }

-- 
2.7.1

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

[Karthik Nayak]

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

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

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

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

[Karthik Nayak]

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

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

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

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

[Karthik Nayak]

Bump code to the top for usage in further patches.

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

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

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

[Karthik Nayak]

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

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

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

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

[Karthik Nayak]

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

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
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 3736dc3..974c412 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 v5 06/12] ref-filter: introduce color_atom_parser()

[Karthik Nayak]

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

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

diff --git a/ref-filter.c b/ref-filter.c
index 974c412..f401da6 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 v5 07/12] ref-filter: introduce parse_align_position()

[Karthik Nayak]

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

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

diff --git a/ref-filter.c b/ref-filter.c
index f401da6..a08ea83 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;
@@ -911,14 +922,12 @@ static void populate_value(struct ref_array_item *ref)
 			align->position = ALIGN_LEFT;
 
 			while (*s) {
+				int position;
+
 				if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))
 					;
-				else if (!strcmp(s[0]->buf, "left"))
-					align->position = ALIGN_LEFT;
-				else if (!strcmp(s[0]->buf, "right"))
-					align->position = ALIGN_RIGHT;
-				else if (!strcmp(s[0]->buf, "middle"))
-					align->position = ALIGN_MIDDLE;
+				else if ((position = parse_align_position(s[0]->buf)) >= 0)
+					align->position = position;
 				else
 					die(_("improper format entered align:%s"), s[0]->buf);
 				s++;
-- 
2.7.1

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

[Karthik Nayak]

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

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

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

diff --git a/ref-filter.c b/ref-filter.c
index a08ea83..4a9711f 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -16,6 +16,11 @@
 
 typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 
+struct align {
+	align_type position;
+	unsigned int width;
+};
+
 /*
  * An atom is a valid field atom listed below, possibly prefixed with
  * a "*" to denote deref_tag().
@@ -31,6 +36,7 @@ static struct used_atom {
 	cmp_type type;
 	union {
 		char color[COLOR_MAXLEN];
+		struct align align;
 	} u;
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
@@ -55,6 +61,37 @@ static align_type parse_align_position(const char *s)
 	return -1;
 }
 
+static void align_atom_parser(struct used_atom *atom, const char *arg)
+{
+	struct align *align = &atom->u.align;
+	struct strbuf **v, **to_free;
+	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) {
+		int position;
+		const char *s = v[0]->buf;
+
+		if (!strtoul_ui(s, 10, &width))
+			;
+		else if ((position = parse_align_position(s)) >= 0)
+			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);
+}
+
 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 strbuf **s, **to_free;
-			int width = -1;
-
-			if (!valp)
-				die(_("expected format: %%(align:<width>,<position>)"));
-
-			s = to_free = strbuf_split_str_omit_term(valp, ',', 0);
-
-			align->position = ALIGN_LEFT;
-
-			while (*s) {
-				int position;
-
-				if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))
-					;
-				else if ((position = parse_align_position(s[0]->buf)) >= 0)
-					align->position = position;
-				else
-					die(_("improper format entered align:%s"), s[0]->buf);
-				s++;
-			}
-
-			if (width < 0)
-				die(_("positive width expected with the %%(align) atom"));
-			align->width = width;
-			strbuf_list_free(to_free);
+		} else if (starts_with(name, "align")) {
+			v->u.align = atom->u.align;
 			v->handler = align_atom_handler;
 			continue;
 		} else if (!strcmp(name, "end")) {
-- 
2.7.1

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

[Karthik Nayak]

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

Add Documentation and tests for the same.

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

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 2e3e96f..012e8f9 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -133,14 +133,18 @@ color::
 
 align::
 	Left-, middle-, or right-align the content between
-	%(align:...) and %(end). The "align:" is followed by `<width>`
-	and `<position>` in any order separated by a comma, where the
-	`<position>` is either left, right or middle, default being
-	left and `<width>` is the total length of the content with
-	alignment. If the contents length is more than the width then
-	no alignment is performed. If used with '--quote' everything
-	in between %(align:...) and %(end) is quoted, but if nested
-	then only the topmost level performs quoting.
+	%(align:...) and %(end). The "align:" is followed by
+	`width=<width>` and `position=<position>` in any order
+	separated by a comma, where the `<position>` is either left,
+	right or middle, default being left and `<width>` is the total
+	length of the content with alignment. For brevity, the
+	"width=" and/or "position=" prefixes may be omitted, and bare
+	<width> and <position> used instead.  For instance,
+	`%(align:<width>,<position>)`. If the contents length is more
+	than the width then no alignment is performed. If used with
+	'--quote' everything in between %(align:...) and %(end) is
+	quoted, but if nested then only the topmost level performs
+	quoting.
 
 In addition to the above, for commit and tag objects, the header
 field names (`tree`, `parent`, `object`, `type`, and `tag`) can
diff --git a/ref-filter.c b/ref-filter.c
index 4a9711f..13a8280 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -77,7 +77,15 @@ static void align_atom_parser(struct used_atom *atom, const char *arg)
 		int position;
 		const char *s = v[0]->buf;
 
-		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 v5 10/12] 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 13a8280..8005e45 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 v5 11/12] ref-filter: introduce contents_atom_parser()

[Karthik Nayak]

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

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

diff --git a/ref-filter.c b/ref-filter.c
index 8005e45..3895a8c 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 v5 12/12] ref-filter: introduce objectname_atom_parser()

[Karthik Nayak]

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

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

diff --git a/ref-filter.c b/ref-filter.c
index 3895a8c..21f4937 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 v5 02/12] ref-filter: use strbuf_split_str_omit_term()

[Jeff King]

On Wed, Feb 17, 2016 at 12:30:05AM +0530, Karthik Nayak wrote:

> Use the newly introduced strbuf_split_str_omit_term() rather than
> using strbuf_split_str() and manually removing the ',' terminator.
> 
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
>  ref-filter.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)

Did you consider just using string_list_split for this? AFAICT, you
don't care about the results being strbufs themselves, and it would do
what you want without having to bother with patch 1. The result would
look something like the patch below.

Sorry to waltz into a review of v5 with a suggestion to throw out all
the work done in previous iterations. :-/ I just think the strbuf_split
interface is kind of clunky and I'd be happy if we could slowly get rid
of it rather than growing it. Maybe that's not realistic, though (some
of the callsites _do_ want to do things like strbuf_trim() after
splitting).

-Peff

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

[Jeff King]

On Tue, Feb 16, 2016 at 02:22:32PM -0500, Jeff King wrote:

> On Wed, Feb 17, 2016 at 12:30:05AM +0530, Karthik Nayak wrote:
> 
> > Use the newly introduced strbuf_split_str_omit_term() rather than
> > using strbuf_split_str() and manually removing the ',' terminator.
> > 
> > Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> > Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> > ---
> >  ref-filter.c | 9 +--------
> >  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> Did you consider just using string_list_split for this? AFAICT, you
> don't care about the results being strbufs themselves, and it would do
> what you want without having to bother with patch 1. The result would
> look something like the patch below.

Probably help if I actually included the patch.

---
diff --git a/ref-filter.c b/ref-filter.c
index 9ccfc51..45b3352 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 *p = params.items[i].string;
+				if (!strtoul_ui(p, 10, (unsigned int *)&width))
 					;
-				else if (!strcmp(s[0]->buf, "left"))
+				else if (!strcmp(p, "left"))
 					align->position = ALIGN_LEFT;
-				else if (!strcmp(s[0]->buf, "right"))
+				else if (!strcmp(p, "right"))
 					align->position = ALIGN_RIGHT;
-				else if (!strcmp(s[0]->buf, "middle"))
+				else if (!strcmp(p, "middle"))
 					align->position = ALIGN_MIDDLE;
 				else
-					die(_("improper format entered align:%s"), s[0]->buf);
-				s++;
+					die(_("improper format entered align:%s"), p);
 			}
 
 			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")) {

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

[Eric Sunshine]

On Tue, Feb 16, 2016 at 2:22 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Feb 17, 2016 at 12:30:05AM +0530, Karthik Nayak wrote:
>> Use the newly introduced strbuf_split_str_omit_term() rather than
>> using strbuf_split_str() and manually removing the ',' terminator.
>>
>> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
> Did you consider just using string_list_split for this? AFAICT, you
> don't care about the results being strbufs themselves, and it would do
> what you want without having to bother with patch 1. [...]
>
> Sorry to waltz into a review of v5 with a suggestion to throw out all
> the work done in previous iterations. :-/ I just think the strbuf_split
> interface is kind of clunky and I'd be happy if we could slowly get rid
> of it rather than growing it. [...]

That's a nice idea, however, I'm not sure if making it part of this
series this late in the game is a good idea. The series has gone
through major changes and heavy review in each of the preceding
versions, and turnaround time has been consequently quite slow (due
both to the amount of work required by Karthik for each version, and
to the amount of time needed by reviewers to digest all the new
changes). v4 was the first one which had settled to the point where
only minor changes were needed, and we were hoping to land the series
with v5. (A few larger changes were also discussed in v4 reviews, but
we concluded that they could be done as follow-up patches.)

With that in mind, it might be better to make this change as a
followup to this series. On the other hand, as you say, waiting would
expand the strbuf_split interface undesirably, so the alternative
would be for Karthik to submit v6 with this change only (to wit: drop
patch 1 and rewrite patch 2 as you've shown). While such a change will
again require careful review, at least it is well localized, and
Karthik's turnaround time shouldn't be too bad. So...

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

[Jeff King]

On Tue, Feb 16, 2016 at 03:12:29PM -0500, Eric Sunshine wrote:

> > Did you consider just using string_list_split for this? AFAICT, you
> > don't care about the results being strbufs themselves, and it would do
> > what you want without having to bother with patch 1. [...]
> >
> > Sorry to waltz into a review of v5 with a suggestion to throw out all
> > the work done in previous iterations. :-/ I just think the strbuf_split
> > interface is kind of clunky and I'd be happy if we could slowly get rid
> > of it rather than growing it. [...]
> 
> That's a nice idea, however, I'm not sure if making it part of this
> series this late in the game is a good idea. The series has gone
> through major changes and heavy review in each of the preceding
> versions, and turnaround time has been consequently quite slow (due
> both to the amount of work required by Karthik for each version, and
> to the amount of time needed by reviewers to digest all the new
> changes). v4 was the first one which had settled to the point where
> only minor changes were needed, and we were hoping to land the series
> with v5. (A few larger changes were also discussed in v4 reviews, but
> we concluded that they could be done as follow-up patches.)
> 
> With that in mind, it might be better to make this change as a
> followup to this series. On the other hand, as you say, waiting would
> expand the strbuf_split interface undesirably, so the alternative
> would be for Karthik to submit v6 with this change only (to wit: drop
> patch 1 and rewrite patch 2 as you've shown). While such a change will
> again require careful review, at least it is well localized, and
> Karthik's turnaround time shouldn't be too bad. So...

Yeah, I don't insist, and like I said, I'm not 100% sure we can get rid
of the strbuf_split interface anyway. I thought it might actually make
things easier by making the series _shorter_ (so my regret was that
mentioning earlier could have saved reviewing effort on patch 1).

It does mean extra review of the patch I posted, but my hope was that
it's small and localized, and wouldn't impact the later stuff seriously
(there are some textual tweaks to carry it forward, though).

Anyway, I've said my piece, and you guys can do what you will with it.

-Peff

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

[Eric Sunshine]

On Tue, Feb 16, 2016 at 3:49 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Feb 16, 2016 at 03:12:29PM -0500, Eric Sunshine wrote:
>> > Did you consider just using string_list_split for this? AFAICT, you
>> > don't care about the results being strbufs themselves, and it would do
>> > what you want without having to bother with patch 1. [...]
>>
>> That's a nice idea, however, I'm not sure if making it part of this
>> series this late in the game is a good idea. The series has gone
>> through major changes and heavy review in each of the preceding
>> versions, and turnaround time has been consequently quite slow (due
>> both to the amount of work required by Karthik for each version, and
>> to the amount of time needed by reviewers to digest all the new
>> changes). v4 was the first one which had settled to the point where
>> only minor changes were needed, and we were hoping to land the series
>> with v5. [...]
>>
>> With that in mind, it might be better to make this change as a
>> followup to this series. On the other hand, as you say, waiting would
>> expand the strbuf_split interface undesirably, so the alternative
>> would be for Karthik to submit v6 with this change only (to wit: drop
>> patch 1 and rewrite patch 2 as you've shown). While such a change will
>> again require careful review, at least it is well localized, and
>> Karthik's turnaround time shouldn't be too bad. So...
>
> Yeah, I don't insist, and like I said, I'm not 100% sure we can get rid
> of the strbuf_split interface anyway. I thought it might actually make
> things easier by making the series _shorter_ (so my regret was that
> mentioning earlier could have saved reviewing effort on patch 1).
>
> It does mean extra review of the patch I posted, but my hope was that
> it's small and localized, and wouldn't impact the later stuff seriously
> (there are some textual tweaks to carry it forward, though).

My initial reaction was negative due to the heavy review burden this
series has demanded thus far, however, my mind was changing even as I
composed the above response. In retrospect, I think I'd be okay seeing
a v6, for the following reasons:

- I already ended up reviewing the the suggested new changes pretty
closely as a side-effect of reading your proposal.

- It would indeed be nice to avoid introducing
strbuf_split_str_omit_term() in the first place; thus one less thing
to worry about if someone ever takes on the task of retiring the
strbuf_split interface.

- It should be only a minimal amount of work for Karthik, thus
turnaround time should be short.

So, I think I'm fine with it, if Karthik is game.

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

[Jeff King]

On Tue, Feb 16, 2016 at 04:09:53PM -0500, Eric Sunshine wrote:

> My initial reaction was negative due to the heavy review burden this
> series has demanded thus far, however, my mind was changing even as I
> composed the above response. In retrospect, I think I'd be okay seeing
> a v6, for the following reasons:
> 
> - I already ended up reviewing the the suggested new changes pretty
> closely as a side-effect of reading your proposal.
> 
> - It would indeed be nice to avoid introducing
> strbuf_split_str_omit_term() in the first place; thus one less thing
> to worry about if someone ever takes on the task of retiring the
> strbuf_split interface.
> 
> - It should be only a minimal amount of work for Karthik, thus
> turnaround time should be short.
> 
> So, I think I'm fine with it, if Karthik is game.

I started to write up a commit message for my proposed change. But it
did make me think of a counter-argument. Right now we parse
"%(align:10,middle)" but do not allow "%(align: 10, middle)".

Should we? Or perhaps: might we? If the answer is yes, we are likely
better off with strbuf_split, because then we are only a strbuf_trim()
away from making that work.

-Peff

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

[Eric Sunshine]

On Tue, Feb 16, 2016 at 5:34 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Feb 16, 2016 at 04:09:53PM -0500, Eric Sunshine wrote:
>> My initial reaction was negative due to the heavy review burden this
>> series has demanded thus far, however, my mind was changing even as I
>> composed the above response. In retrospect, I think I'd be okay seeing
>> a v6, for the following reasons:
>>
>> - I already ended up reviewing the the suggested new changes pretty
>> closely as a side-effect of reading your proposal.
>>
>> - It would indeed be nice to avoid introducing
>> strbuf_split_str_omit_term() in the first place; thus one less thing
>> to worry about if someone ever takes on the task of retiring the
>> strbuf_split interface.
>>
>> - It should be only a minimal amount of work for Karthik, thus
>> turnaround time should be short.
>>
>> So, I think I'm fine with it, if Karthik is game.
>
> I started to write up a commit message for my proposed change. But it
> did make me think of a counter-argument. Right now we parse
> "%(align:10,middle)" but do not allow "%(align: 10, middle)".
>
> Should we? Or perhaps: might we? If the answer is yes, we are likely
> better off with strbuf_split, because then we are only a strbuf_trim()
> away from making that work.

I also considered the issue of embedded whitespace very early on when
reading your initial proposal, but didn't mention anything about it
due to a vague recollection from one of the early reviews (or possibly
a review of one of Karthik's other patch series) of someone (possibly
Junio) saying or implying that embedded whitespace would not be
supported. Unfortunately, I can't locate that message (assuming it
even exists and wasn't a figment of my imagination).

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

[Jeff King]

On Tue, Feb 16, 2016 at 05:49:19PM -0500, Eric Sunshine wrote:

> On Tue, Feb 16, 2016 at 5:34 PM, Jeff King <peff@peff.net> wrote:
> > On Tue, Feb 16, 2016 at 04:09:53PM -0500, Eric Sunshine wrote:
> >> My initial reaction was negative due to the heavy review burden this
> >> series has demanded thus far, however, my mind was changing even as I
> >> composed the above response. In retrospect, I think I'd be okay seeing
> >> a v6, for the following reasons:
> >>
> >> - I already ended up reviewing the the suggested new changes pretty
> >> closely as a side-effect of reading your proposal.
> >>
> >> - It would indeed be nice to avoid introducing
> >> strbuf_split_str_omit_term() in the first place; thus one less thing
> >> to worry about if someone ever takes on the task of retiring the
> >> strbuf_split interface.
> >>
> >> - It should be only a minimal amount of work for Karthik, thus
> >> turnaround time should be short.
> >>
> >> So, I think I'm fine with it, if Karthik is game.
> >
> > I started to write up a commit message for my proposed change. But it
> > did make me think of a counter-argument. Right now we parse
> > "%(align:10,middle)" but do not allow "%(align: 10, middle)".
> >
> > Should we? Or perhaps: might we? If the answer is yes, we are likely
> > better off with strbuf_split, because then we are only a strbuf_trim()
> > away from making that work.
> 
> I also considered the issue of embedded whitespace very early on when
> reading your initial proposal, but didn't mention anything about it
> due to a vague recollection from one of the early reviews (or possibly
> a review of one of Karthik's other patch series) of someone (possibly
> Junio) saying or implying that embedded whitespace would not be
> supported. Unfortunately, I can't locate that message (assuming it
> even exists and wasn't a figment of my imagination).

Yeah, I could not find any relevant reference (though I didn't spend all
that long digging).

For reference, I rebuilt Karthik's series on top of my proposal, and the
changes are fairly minor. I pushed it to:

  git://github.com/peff/git.git jk/tweaked-ref-filter

The tbdiff is below. Hopefully having that done makes it easier to
decide based on the outcome, rather than the pain of rebasing. :)

To be honest, though, I am now on the fence, considering the possible
whitespace issue.

 1:  92de9c7 < --:  ------- strbuf: introduce strbuf_split_str_omit_term()
 2:  4845dc5 < --:  ------- ref-filter: use strbuf_split_str_omit_term()
--:  ------- >  1:  29177cc ref-filter: use string_list_split over strbuf_split
 3:  040e9ce =  2:  ed284bc ref-filter: bump 'used_atom' and related code to the top
 4:  c7eb061 =  3:  2a99777 ref-filter: introduce struct used_atom
 5:  c3e24cf =  4:  b18f23b ref-filter: introduce parsing functions for each valid atom
 6:  0b7fe83 =  5:  e5221cc ref-filter: introduce color_atom_parser()
 7:  ffb3afe !  6:  454af9c ref-filter: introduce parse_align_position()
    @@ -32,21 +32,21 @@
      	const char *name;
      	cmp_type cmp_type;
     @@
    - 			align->position = ALIGN_LEFT;
    - 
    - 			while (*s) {
    + 			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[0]->buf, 10, (unsigned int *)&width))
    + 				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 if ((position = parse_align_position(s[0]->buf)) >= 0)
    ++				else if ((position = parse_align_position(s)) >= 0)
     +					align->position = position;
      				else
    - 					die(_("improper format entered align:%s"), s[0]->buf);
    - 				s++;
    + 					die(_("improper format entered align:%s"), s);
    + 			}
 8:  0f0e596 !  7:  0779954 ref-filter: introduce align_atom_parser()
    @@ -43,18 +43,19 @@
     +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 (!strtoul_ui(s, 10, &width))
     +			;
    @@ -62,13 +63,12 @@
     +			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 {
    @@ -130,32 +130,32 @@
      			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>)"));
     -
    --			s = to_free = strbuf_split_str_omit_term(valp, ',', 0);
    --
     -			align->position = ALIGN_LEFT;
     -
    --			while (*s) {
    +-			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[0]->buf, 10, (unsigned int *)&width))
    +-				if (!strtoul_ui(s, 10, (unsigned int *)&width))
     -					;
    --				else if ((position = parse_align_position(s[0]->buf)) >= 0)
    +-				else if ((position = parse_align_position(s)) >= 0)
     -					align->position = position;
     -				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);
     +		} else if (starts_with(name, "align")) {
     +			v->u.align = atom->u.align;
      			v->handler = align_atom_handler;
 9:  d3dc384 !  8:  792c89a ref-filter: align: introduce long-form syntax
    @@ -45,8 +45,8 @@
     --- a/ref-filter.c
     +++ b/ref-filter.c
     @@
    + 		const char *s = params.items[i].string;
      		int position;
    - 		const char *s = v[0]->buf;
      
     -		if (!strtoul_ui(s, 10, &width))
     +		if (skip_prefix(s, "position=", &s)) {
10:  3ae28b5 =  9:  019fee7 ref-filter: introduce remote_ref_atom_parser()
11:  06c70af = 10:  f6e4f5a ref-filter: introduce contents_atom_parser()
12:  c9db181 = 11:  0a84b70 ref-filter: introduce objectname_atom_parser()

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> > Should we? Or perhaps: might we? If the answer is yes, we are likely
>> > better off with strbuf_split, because then we are only a strbuf_trim()
>> > away from making that work.
>> 
>> I also considered the issue of embedded whitespace very early on when
>> reading your initial proposal, but didn't mention anything about it
>> due to a vague recollection from one of the early reviews (or possibly
>> a review of one of Karthik's other patch series) of someone (possibly
>> Junio) saying or implying that embedded whitespace would not be
>> supported. Unfortunately, I can't locate that message (assuming it
>> even exists and wasn't a figment of my imagination).
>
> Yeah, I could not find any relevant reference (though I didn't spend all
> that long digging).
>
> For reference, I rebuilt Karthik's series on top of my proposal, and the
> changes are fairly minor. I pushed it to:
>
>   git://github.com/peff/git.git jk/tweaked-ref-filter
>
> The tbdiff is below. Hopefully having that done makes it easier to
> decide based on the outcome, rather than the pain of rebasing. :)
>
> To be honest, though, I am now on the fence, considering the possible
> whitespace issue.

Certainly not having to see s[0]->buf over and over is a huge win ;-).

Is the "whitespace issue" a big deal?  Does it involve more than a
similar sibling to string_list_split() that trims the whitespace
around the delimiter (or allows a regexp as a delimiter "\s*,\s*")?

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

[Jeff King]

On Tue, Feb 16, 2016 at 04:12:08PM -0800, Junio C Hamano wrote:

> > To be honest, though, I am now on the fence, considering the possible
> > whitespace issue.
> 
> Certainly not having to see s[0]->buf over and over is a huge win ;-).
> 
> Is the "whitespace issue" a big deal?  Does it involve more than a
> similar sibling to string_list_split() that trims the whitespace
> around the delimiter (or allows a regexp as a delimiter "\s*,\s*")?

I think that solution would work (and IMHO would actually be preferable
to the split-then-trim that strbuf_split does). But it does mean writing
new code.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Feb 16, 2016 at 04:12:08PM -0800, Junio C Hamano wrote:
>
>> > To be honest, though, I am now on the fence, considering the possible
>> > whitespace issue.
>> 
>> Certainly not having to see s[0]->buf over and over is a huge win ;-).
>> 
>> Is the "whitespace issue" a big deal?  Does it involve more than a
>> similar sibling to string_list_split() that trims the whitespace
>> around the delimiter (or allows a regexp as a delimiter "\s*,\s*")?
>
> I think that solution would work (and IMHO would actually be preferable
> to the split-then-trim that strbuf_split does). But it does mean writing
> new code.

True, but only when we decide to support trimming the whitespace,
which can come later.

I do not even know if it is wise to accept %(align:position=left, width=4)
when %(align:position=left,width=4) would do the job just fine.

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

[Jeff King]

On Tue, Feb 16, 2016 at 04:28:10PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Tue, Feb 16, 2016 at 04:12:08PM -0800, Junio C Hamano wrote:
> >
> >> > To be honest, though, I am now on the fence, considering the possible
> >> > whitespace issue.
> >> 
> >> Certainly not having to see s[0]->buf over and over is a huge win ;-).
> >> 
> >> Is the "whitespace issue" a big deal?  Does it involve more than a
> >> similar sibling to string_list_split() that trims the whitespace
> >> around the delimiter (or allows a regexp as a delimiter "\s*,\s*")?
> >
> > I think that solution would work (and IMHO would actually be preferable
> > to the split-then-trim that strbuf_split does). But it does mean writing
> > new code.
> 
> True, but only when we decide to support trimming the whitespace,
> which can come later.
> 
> I do not even know if it is wise to accept %(align:position=left, width=4)
> when %(align:position=left,width=4) would do the job just fine.

Yeah, it was mostly just about being friendly to the user. But if nobody
is complaining, it may not even be worth worrying about.

-Peff

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

[Karthik Nayak]

On Wed, Feb 17, 2016 at 12:52 AM, Jeff King <peff@peff.net> wrote:
> On Wed, Feb 17, 2016 at 12:30:05AM +0530, Karthik Nayak wrote:
>
>> Use the newly introduced strbuf_split_str_omit_term() rather than
>> using strbuf_split_str() and manually removing the ',' terminator.
>>
>> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>>  ref-filter.c | 9 +--------
>>  1 file changed, 1 insertion(+), 8 deletions(-)
>
> Did you consider just using string_list_split for this? AFAICT, you
> don't care about the results being strbufs themselves, and it would do
> what you want without having to bother with patch 1. The result would
> look something like the patch below.
>

I haven't, thanks for bringing it up :)

> Sorry to waltz into a review of v5 with a suggestion to throw out all
> the work done in previous iterations. :-/ I just think the strbuf_split
> interface is kind of clunky and I'd be happy if we could slowly get rid
> of it rather than growing it. Maybe that's not realistic, though (some
> of the callsites _do_ want to do things like strbuf_trim() after
> splitting).
>
> -Peff

That's fine, as I see it, it's better to wait a while and get a better version
of something.

-- 
Regards,
Karthik Nayak

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

[Karthik Nayak]

On Wed, Feb 17, 2016 at 2:39 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Feb 16, 2016 at 3:49 PM, Jeff King <peff@peff.net> wrote:
>> On Tue, Feb 16, 2016 at 03:12:29PM -0500, Eric Sunshine wrote:
>>> > Did you consider just using string_list_split for this? AFAICT, you
>>> > don't care about the results being strbufs themselves, and it would do
>>> > what you want without having to bother with patch 1. [...]
>>>
>>> That's a nice idea, however, I'm not sure if making it part of this
>>> series this late in the game is a good idea. The series has gone
>>> through major changes and heavy review in each of the preceding
>>> versions, and turnaround time has been consequently quite slow (due
>>> both to the amount of work required by Karthik for each version, and
>>> to the amount of time needed by reviewers to digest all the new
>>> changes). v4 was the first one which had settled to the point where
>>> only minor changes were needed, and we were hoping to land the series
>>> with v5. [...]
>>>
>>> With that in mind, it might be better to make this change as a
>>> followup to this series. On the other hand, as you say, waiting would
>>> expand the strbuf_split interface undesirably, so the alternative
>>> would be for Karthik to submit v6 with this change only (to wit: drop
>>> patch 1 and rewrite patch 2 as you've shown). While such a change will
>>> again require careful review, at least it is well localized, and
>>> Karthik's turnaround time shouldn't be too bad. So...
>>
>> Yeah, I don't insist, and like I said, I'm not 100% sure we can get rid
>> of the strbuf_split interface anyway. I thought it might actually make
>> things easier by making the series _shorter_ (so my regret was that
>> mentioning earlier could have saved reviewing effort on patch 1).
>>
>> It does mean extra review of the patch I posted, but my hope was that
>> it's small and localized, and wouldn't impact the later stuff seriously
>> (there are some textual tweaks to carry it forward, though).
>
> My initial reaction was negative due to the heavy review burden this
> series has demanded thus far, however, my mind was changing even as I
> composed the above response. In retrospect, I think I'd be okay seeing
> a v6, for the following reasons:
>
> - I already ended up reviewing the the suggested new changes pretty
> closely as a side-effect of reading your proposal.
>
> - It would indeed be nice to avoid introducing
> strbuf_split_str_omit_term() in the first place; thus one less thing
> to worry about if someone ever takes on the task of retiring the
> strbuf_split interface.
>
> - It should be only a minimal amount of work for Karthik, thus
> turnaround time should be short.
>
> So, I think I'm fine with it, if Karthik is game.

Sounds good to me.

I just read the conversation between Jeff, Junio and You about the whitespace
counter-argument and I think its good to go ahead with v6 with Jeff's suggested
change.

Since he's already pushed the changes on top of my changes to:

 git://github.com/peff/git.git jk/tweaked-ref-filter

I'll just have a look and push that to the list as v6.

-- 
Regards,
Karthik Nayak

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

[Eric Sunshine]

On Wed, Feb 17, 2016 at 12:04 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Wed, Feb 17, 2016 at 2:39 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> My initial reaction was negative due to the heavy review burden this
>> series has demanded thus far, however, my mind was changing even as I
>> composed the above response. [...]
>>
>> So, I think I'm fine with it, if Karthik is game.
>
> Sounds good to me.
>
> I just read the conversation between Jeff, Junio and You about the whitespace
> counter-argument and I think its good to go ahead with v6 with Jeff's suggested
> change.
>
> Since he's already pushed the changes on top of my changes to:
>  git://github.com/peff/git.git jk/tweaked-ref-filter
> I'll just have a look and push that to the list as v6.

I reviewed the entire series again, including Peff's changes, so this
entire series is:

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

Karthik, feel free to include my Reviewed-by: in all the patches
(including Peff's) when you post v6.

Thanks.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Feb 16, 2016 at 04:28:10PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > On Tue, Feb 16, 2016 at 04:12:08PM -0800, Junio C Hamano wrote:
>> >
>> >> > To be honest, though, I am now on the fence, considering the possible
>> >> > whitespace issue.
>> >> 
>> >> Certainly not having to see s[0]->buf over and over is a huge win ;-).
>> >> 
>> >> Is the "whitespace issue" a big deal?  Does it involve more than a
>> >> similar sibling to string_list_split() that trims the whitespace
>> >> around the delimiter (or allows a regexp as a delimiter "\s*,\s*")?
>> >
>> > I think that solution would work (and IMHO would actually be preferable
>> > to the split-then-trim that strbuf_split does). But it does mean writing
>> > new code.
>> 
>> True, but only when we decide to support trimming the whitespace,
>> which can come later.
>> 
>> I do not even know if it is wise to accept %(align:position=left, width=4)
>> when %(align:position=left,width=4) would do the job just fine.
>
> Yeah, it was mostly just about being friendly to the user. But if nobody
> is complaining, it may not even be worth worrying about.

I was more worried about the possibility that we may have to support
values with leading or trailing whitespaces in the future.

    0. %(align:position=left,width=4)
    1. %(align:position=left, width=4)
    2. %(align: position =left, width =4)
    3. %(align: position = left, width = 4)
    4. %(align: position = left , width = 4)

We can probably accept 1. without ambiguity, and probably 2., too.
These examples are about keys with possible leading or trailing
whitespaces, and it is unlikely that we need to support them.

To those who do not think carefully themselves, however, going from
1 & 2 that are handlable (and some might even argue that 1. is
easlier to read) to 3 & 4 will not appear as a big syntax-breaking
leap.  But 3 & 4 are; these need disambiguation rule on the value
side (we either introduce quoting, declare no values can have
leading or trailing whitespaces, etc.).

That is why I suggested not to even accept 1. to avoid the slipperly
slope.

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

[Karthik Nayak]

On Wed, Feb 17, 2016 at 11:09 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Feb 17, 2016 at 12:04 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> On Wed, Feb 17, 2016 at 2:39 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> My initial reaction was negative due to the heavy review burden this
>>> series has demanded thus far, however, my mind was changing even as I
>>> composed the above response. [...]
>>>
>>> So, I think I'm fine with it, if Karthik is game.
>>
>> Sounds good to me.
>>
>> I just read the conversation between Jeff, Junio and You about the whitespace
>> counter-argument and I think its good to go ahead with v6 with Jeff's suggested
>> change.
>>
>> Since he's already pushed the changes on top of my changes to:
>>  git://github.com/peff/git.git jk/tweaked-ref-filter
>> I'll just have a look and push that to the list as v6.
>
> I reviewed the entire series again, including Peff's changes, so this
> entire series is:
>
>     Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
>
> Karthik, feel free to include my Reviewed-by: in all the patches
> (including Peff's) when you post v6.
>
> Thanks.

Oops! I just pushed v6 before I even saw this mail.

-- 
Regards,
Karthik Nayak

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

[Eric Sunshine]

On Wed, Feb 17, 2016 at 1:07 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Wed, Feb 17, 2016 at 11:09 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> I reviewed the entire series again, including Peff's changes, so this
>> entire series is:
>>
>>     Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
>>
>> Karthik, feel free to include my Reviewed-by: in all the patches
>> (including Peff's) when you post v6.
>
> Oops! I just pushed v6 before I even saw this mail.

No problem. Junio can add my Reviewed-by: if he wants when he picks up
the series.

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

[Karthik Nayak]

On Wed, Feb 17, 2016 at 11:47 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Feb 17, 2016 at 1:07 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> On Wed, Feb 17, 2016 at 11:09 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> I reviewed the entire series again, including Peff's changes, so this
>>> entire series is:
>>>
>>>     Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
>>>
>>> Karthik, feel free to include my Reviewed-by: in all the patches
>>> (including Peff's) when you post v6.
>>
>> Oops! I just pushed v6 before I even saw this mail.
>
> No problem. Junio can add my Reviewed-by: if he wants when he picks up
> the series.

That would be great :) Thanks for reviewing this series.

-- 
Regards,
Karthik Nayak