[PATCH v3 00/15] ref-filter: use parsing functions

[PATCH v3 00/15] 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

Changes in this version:
* Change variable name in 'used_atom' from 'str' to 'name'.
* Split introduction of 'used_atom' into [3/15], [4/15] and [5/15].
* In [8/15] parse the color in color_atom_parser() itself and pass the
value to 'v->s' in populate_value().
* Introduce [11/15] which changes 'width' in align_atom_parser() from
int to unsigned int.
* Fix error in code in align_atom_parser(). and split it into [9/15],
[10/15], [11/15] and [12/15].
* Change 'align' atom tests to a table form.
* Change 'no_lines' to 'nlines' in used_atom.
* Switch to starts_with()/strcmp() instead of match_atom_name() as required.
* Changes in commit message and comments.
* Improve readability of code.

Eric suggested that I make match_atom_name() not return a value [0]. I
haven't done that as we use match_atom_name() in [14/15] for matching
'subject' and 'body' in contents_atom_parser() and although Eric
suggested I use strcmp() instead, this would not work as we need to
check for derefernced 'subject' and 'body' atoms.

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

Karthik Nayak (15):
  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-fitler: bump match_atom() name to the top
  ref-filter: skip deref specifier in match_atom_name()
  ref-filter: introduce color_atom_parser()
  ref-filter: introduce align_atom_parser()
  ref-filter: introduce parse_align_position()
  ref-filter: convert variable 'width' to an unsigned int
  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                       | 455 ++++++++++++++++++++++---------------
 strbuf.c                           |   7 +-
 strbuf.h                           |  25 +-
 t/t6302-for-each-ref-filter.sh     |  41 ++++
 5 files changed, 349 insertions(+), 199 deletions(-)

-- 
2.6.4

[PATCH v3 01/15] 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 |  7 ++++---
 strbuf.h | 25 ++++++++++++++++---------
 2 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/strbuf.c b/strbuf.c
index b552a13..b62508e 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -115,7 +115,7 @@ void strbuf_tolower(struct strbuf *sb)
 }
 
 struct strbuf **strbuf_split_buf(const char *str, size_t slen,
-				 int terminator, int max)
+				 int terminator, int max, int omit_term)
 {
 	struct strbuf **ret = NULL;
 	size_t nr = 0, alloc = 0;
@@ -123,14 +123,15 @@ struct strbuf **strbuf_split_buf(const char *str, size_t slen,
 
 	while (slen) {
 		int len = slen;
+		const char *end = NULL;
 		if (max <= 0 || nr + 1 < max) {
-			const char *end = memchr(str, terminator, slen);
+			end = memchr(str, terminator, slen);
 			if (end)
 				len = end - str + 1;
 		}
 		t = xmalloc(sizeof(struct strbuf));
 		strbuf_init(t, len);
-		strbuf_add(t, str, len);
+		strbuf_add(t, str, len - !!end * !!omit_term);
 		ALLOC_GROW(ret, nr + 2, alloc);
 		ret[nr++] = t;
 		str += len;
diff --git a/strbuf.h b/strbuf.h
index 6ae7a72..1de816d 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -450,11 +450,12 @@ static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)
 /**
  * Split str (of length slen) at the specified terminator character.
  * Return a null-terminated array of pointers to strbuf objects
- * holding the substrings.  The substrings include the terminator,
- * except for the last substring, which might be unterminated if the
- * original string did not end with a terminator.  If max is positive,
- * then split the string into at most max substrings (with the last
- * substring containing everything following the (max-1)th terminator
+ * holding the substrings.  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
@@ -465,19 +466,25 @@ static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)
  * For lighter-weight alternatives, see string_list_split() and
  * string_list_split_in_place().
  */
-extern struct strbuf **strbuf_split_buf(const char *, size_t,
-					int terminator, int max);
+extern struct strbuf **strbuf_split_buf(const char *str, size_t slen,
+					int terminator, int max, int omit_term);
+
+static inline struct strbuf **strbuf_split_str_omit_term(const char *str,
+							    int terminator, int max)
+{
+	return strbuf_split_buf(str, strlen(str), terminator, max, 1);
+}
 
 static inline struct strbuf **strbuf_split_str(const char *str,
 					       int terminator, int max)
 {
-	return strbuf_split_buf(str, strlen(str), terminator, max);
+	return strbuf_split_buf(str, strlen(str), terminator, max, 0);
 }
 
 static inline struct strbuf **strbuf_split_max(const struct strbuf *sb,
 						int terminator, int max)
 {
-	return strbuf_split_buf(sb->buf, sb->len, terminator, max);
+	return strbuf_split_buf(sb->buf, sb->len, terminator, max, 0);
 }
 
 static inline struct strbuf **strbuf_split(const struct strbuf *sb,
-- 
2.6.4

Re: [PATCH v3 01/15] strbuf: introduce strbuf_split_str_omit_term()

[Junio C Hamano]

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

> 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 |  7 ++++---
>  strbuf.h | 25 ++++++++++++++++---------
>  2 files changed, 20 insertions(+), 12 deletions(-)
>
> diff --git a/strbuf.c b/strbuf.c
> index b552a13..b62508e 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -115,7 +115,7 @@ void strbuf_tolower(struct strbuf *sb)
>  }
>  
>  struct strbuf **strbuf_split_buf(const char *str, size_t slen,
> -				 int terminator, int max)
> +				 int terminator, int max, int omit_term)
>  {
>  	struct strbuf **ret = NULL;
>  	size_t nr = 0, alloc = 0;
> @@ -123,14 +123,15 @@ struct strbuf **strbuf_split_buf(const char *str, size_t slen,
>  
>  	while (slen) {
>  		int len = slen;
> +		const char *end = NULL;
>  		if (max <= 0 || nr + 1 < max) {
> -			const char *end = memchr(str, terminator, slen);
> +			end = memchr(str, terminator, slen);
>  			if (end)
>  				len = end - str + 1;
>  		}
>  		t = xmalloc(sizeof(struct strbuf));
>  		strbuf_init(t, len);
> -		strbuf_add(t, str, len);
> +		strbuf_add(t, str, len - !!end * !!omit_term);

You initialize with "len" but sometimes copy less than that, which
looks somewhat sloppy.

Maybe I am old-fashioned, but use of a multiplication when you do
not mean to numerically multiply but merely to perform a logical
operation made me go "Huh?".

Perhaps using another variable would make it easier to follow?
Either using a boolean that tells us that the terminating byte
is to be omitted, i.e.

	int len = slen;
        int omit = 0;
        if ( ... we are still splitting ... ) {
		const char *end = memchr(...);
                if (end) {
                	len = end - str + 1;
			omit = !!omit_term;
		}
	}
	strbuf_init(t, len - omit);
        strbuf_add(t, str, len - omit);

or an integer "copylen" that tells us how many bytes to copy, which
often is the same as "len" but sometimes different by 1 byte?

Re: [PATCH v3 01/15] strbuf: introduce strbuf_split_str_omit_term()

[Karthik Nayak]

On Wed, Jan 6, 2016 at 12:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> 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 |  7 ++++---
>>  strbuf.h | 25 ++++++++++++++++---------
>>  2 files changed, 20 insertions(+), 12 deletions(-)
>>
>> diff --git a/strbuf.c b/strbuf.c
>> index b552a13..b62508e 100644
>> --- a/strbuf.c
>> +++ b/strbuf.c
>> @@ -115,7 +115,7 @@ void strbuf_tolower(struct strbuf *sb)
>>  }
>>
>>  struct strbuf **strbuf_split_buf(const char *str, size_t slen,
>> -                              int terminator, int max)
>> +                              int terminator, int max, int omit_term)
>>  {
>>       struct strbuf **ret = NULL;
>>       size_t nr = 0, alloc = 0;
>> @@ -123,14 +123,15 @@ struct strbuf **strbuf_split_buf(const char *str, size_t slen,
>>
>>       while (slen) {
>>               int len = slen;
>> +             const char *end = NULL;
>>               if (max <= 0 || nr + 1 < max) {
>> -                     const char *end = memchr(str, terminator, slen);
>> +                     end = memchr(str, terminator, slen);
>>                       if (end)
>>                               len = end - str + 1;
>>               }
>>               t = xmalloc(sizeof(struct strbuf));
>>               strbuf_init(t, len);
>> -             strbuf_add(t, str, len);
>> +             strbuf_add(t, str, len - !!end * !!omit_term);
>
> You initialize with "len" but sometimes copy less than that, which
> looks somewhat sloppy.
>
> Maybe I am old-fashioned, but use of a multiplication when you do
> not mean to numerically multiply but merely to perform a logical
> operation made me go "Huh?".
>
> Perhaps using another variable would make it easier to follow?
> Either using a boolean that tells us that the terminating byte
> is to be omitted, i.e.
>
>         int len = slen;
>         int omit = 0;
>         if ( ... we are still splitting ... ) {
>                 const char *end = memchr(...);
>                 if (end) {
>                         len = end - str + 1;
>                         omit = !!omit_term;
>                 }
>         }
>         strbuf_init(t, len - omit);
>         strbuf_add(t, str, len - omit);
>
> or an integer "copylen" that tells us how many bytes to copy, which
> often is the same as "len" but sometimes different by 1 byte?

This is done based on Eric's suggestion [1]. Although its a little off normal
convention. I find it small and simple. So I'm okay with either, your suggested
change or the existing code.

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

-- 
Regards,
Karthik Nayak

Re: [PATCH v3 01/15] strbuf: introduce strbuf_split_str_omit_term()

[Eric Sunshine]

On Wed, Jan 6, 2016 at 2:27 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Wed, Jan 6, 2016 at 12:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>       while (slen) {
>>>               int len = slen;
>>> +             const char *end = NULL;
>>>               if (max <= 0 || nr + 1 < max) {
>>> -                     const char *end = memchr(str, terminator, slen);
>>> +                     end = memchr(str, terminator, slen);
>>>                       if (end)
>>>                               len = end - str + 1;
>>>               }
>>>               t = xmalloc(sizeof(struct strbuf));
>>>               strbuf_init(t, len);
>>> -             strbuf_add(t, str, len);
>>> +             strbuf_add(t, str, len - !!end * !!omit_term);
>>
>> Perhaps using another variable would make it easier to follow?
>> Either using a boolean that tells us that the terminating byte
>> is to be omitted, i.e.
>>
>>         int len = slen;
>>         int omit = 0;
>>         if ( ... we are still splitting ... ) {
>>                 const char *end = memchr(...);
>>                 if (end) {
>>                         len = end - str + 1;
>>                         omit = !!omit_term;
>>                 }
>>         }
>>         strbuf_init(t, len - omit);
>>         strbuf_add(t, str, len - omit);
>>
>> or an integer "copylen" that tells us how many bytes to copy, which
>> often is the same as "len" but sometimes different by 1 byte?
>
> This is done based on Eric's suggestion [1]. Although its a little off normal
> convention. I find it small and simple. So I'm okay with either, your suggested
> change or the existing code.

A "copylen" variable would probably result in the clearest code since
it states explicitly what an otherwise opaque expression like (!!end *
!!omit_term) means, thus is easier to reason about.

Re: [PATCH v3 01/15] strbuf: introduce strbuf_split_str_omit_term()

[Karthik Nayak]

On Fri, Jan 22, 2016 at 1:17 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Jan 6, 2016 at 2:27 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> On Wed, Jan 6, 2016 at 12:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>>       while (slen) {
>>>>               int len = slen;
>>>> +             const char *end = NULL;
>>>>               if (max <= 0 || nr + 1 < max) {
>>>> -                     const char *end = memchr(str, terminator, slen);
>>>> +                     end = memchr(str, terminator, slen);
>>>>                       if (end)
>>>>                               len = end - str + 1;
>>>>               }
>>>>               t = xmalloc(sizeof(struct strbuf));
>>>>               strbuf_init(t, len);
>>>> -             strbuf_add(t, str, len);
>>>> +             strbuf_add(t, str, len - !!end * !!omit_term);
>>>
>>> Perhaps using another variable would make it easier to follow?
>>> Either using a boolean that tells us that the terminating byte
>>> is to be omitted, i.e.
>>>
>>>         int len = slen;
>>>         int omit = 0;
>>>         if ( ... we are still splitting ... ) {
>>>                 const char *end = memchr(...);
>>>                 if (end) {
>>>                         len = end - str + 1;
>>>                         omit = !!omit_term;
>>>                 }
>>>         }
>>>         strbuf_init(t, len - omit);
>>>         strbuf_add(t, str, len - omit);
>>>
>>> or an integer "copylen" that tells us how many bytes to copy, which
>>> often is the same as "len" but sometimes different by 1 byte?
>>
>> This is done based on Eric's suggestion [1]. Although its a little off normal
>> convention. I find it small and simple. So I'm okay with either, your suggested
>> change or the existing code.
>
> A "copylen" variable would probably result in the clearest code since
> it states explicitly what an otherwise opaque expression like (!!end *
> !!omit_term) means, thus is easier to reason about.

Sure, I think this would do:

diff --git a/strbuf.c b/strbuf.c
index b552a13..81e279d 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;


-- 
Regards,
Karthik Nayak

[PATCH v3 02/15] 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 e205dd2..6263710 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -869,18 +869,11 @@ static void populate_value(struct ref_array_item *ref)
 			if (!valp)
 				die(_("expected format: %%(align:<width>,<position>)"));
 
-			/*
-			 * TODO: Implement a function similar to strbuf_split_str()
-			 * which would omit the separator from the end of each value.
-			 */
-			s = to_free = strbuf_split_str(valp, ',', 0);
+			s = to_free = strbuf_split_str_omit_term(valp, ',', 0);
 
 			align->position = ALIGN_LEFT;
 
 			while (*s) {
-				/*  Strip trailing comma */
-				if (s[1])
-					strbuf_setlen(s[0], s[0]->len - 1);
 				if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))
 					;
 				else if (!strcmp(s[0]->buf, "left"))
-- 
2.6.4

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

[Karthik Nayak]

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

diff --git a/ref-filter.c b/ref-filter.c
index 6263710..388e2dd 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 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;
+
 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.6.4

[PATCH v3 04/15] 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 | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 388e2dd..75ebd03 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -17,7 +17,7 @@
 typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 
 /*
- * An atom is a valid field atom listed above, possibly prefixed with
+ * 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
@@ -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 == '*'))
@@ -786,7 +787,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;
@@ -1438,7 +1439,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
 {
 	struct atom_value *va, *vb;
 	int cmp;
-	cmp_type cmp_type = used_atom_type[s->atom];
+	cmp_type cmp_type = used_atom[s->atom].type;
 
 	get_ref_atom_value(a, s->atom, &va);
 	get_ref_atom_value(b, s->atom, &vb);
-- 
2.6.4

Re: [PATCH v3 04/15] ref-filter: introduce struct used_atom

[Eric Sunshine]

On Tuesday, January 5, 2016, Karthik Nayak <karthik.188@gmail.com> wrote:
> 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>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -17,7 +17,7 @@
>  typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
>
>  /*
> - * An atom is a valid field atom listed above, possibly prefixed with
> + * An atom is a valid field atom listed below, possibly prefixed with

This particular change should be in patch 3/15 which moved this comment block.

(Yes, it's true that this will make the patch something other than
"pure code movement", but this change keeps the moved block logically
consistent and that it's an appropriate thing to do -- plus it's quite
minor.)

>   * a "*" to denote deref_tag().
>   *
>   * We parse given format string and sort specifiers, and make a list

Re: [PATCH v3 04/15] ref-filter: introduce struct used_atom

[Karthik Nayak]

On Fri, Jan 22, 2016 at 12:34 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tuesday, January 5, 2016, Karthik Nayak <karthik.188@gmail.com> wrote:
>> 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>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -17,7 +17,7 @@
>>  typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
>>
>>  /*
>> - * An atom is a valid field atom listed above, possibly prefixed with
>> + * An atom is a valid field atom listed below, possibly prefixed with
>
> This particular change should be in patch 3/15 which moved this comment block.
>
> (Yes, it's true that this will make the patch something other than
> "pure code movement", but this change keeps the moved block logically
> consistent and that it's an appropriate thing to do -- plus it's quite
> minor.)

Will squeeze that in. Like you said it's too small so it would make
sense to bundle it
within 3/15.

-- 
Regards,
Karthik Nayak

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

[Karthik Nayak]

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

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

diff --git a/ref-filter.c b/ref-filter.c
index 75ebd03..efa247a 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);
 } valid_atom[] = {
 	{ "refname" },
 	{ "objecttype" },
@@ -154,6 +155,8 @@ 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 (valid_atom[i].parser)
+		valid_atom[i].parser(&used_atom[at]);
 	if (*atom == '*')
 		need_tagged = 1;
 	if (!strcmp(used_atom[at].name, "symref"))
-- 
2.6.4

[PATCH v3 06/15] ref-fitler: bump match_atom() name to the top

[Karthik Nayak]

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

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

diff --git a/ref-filter.c b/ref-filter.c
index efa247a..575d293 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -33,6 +33,22 @@ static struct used_atom {
 static int used_atom_cnt, need_tagged, need_symref;
 static int need_color_reset_at_eol;
 
+static int match_atom_name(const char *name, const char *atom_name, const char **val)
+{
+	const char *body;
+
+	if (!skip_prefix(name, atom_name, &body))
+		return 0; /* doesn't even begin with "atom_name" */
+	if (!body[0]) {
+		*val = NULL; /* %(atom_name) and no customization */
+		return 1;
+	}
+	if (body[0] != ':')
+		return 0; /* "atom_namefoo" is not "atom_name" or "atom_name:..." */
+	*val = body + 1; /* "atom_name:val" */
+	return 1;
+}
+
 static struct {
 	const char *name;
 	cmp_type cmp_type;
@@ -262,22 +278,6 @@ static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_sta
 	pop_stack_element(&state->stack);
 }
 
-static int match_atom_name(const char *name, const char *atom_name, const char **val)
-{
-	const char *body;
-
-	if (!skip_prefix(name, atom_name, &body))
-		return 0; /* doesn't even begin with "atom_name" */
-	if (!body[0]) {
-		*val = NULL; /* %(atom_name) and no customization */
-		return 1;
-	}
-	if (body[0] != ':')
-		return 0; /* "atom_namefoo" is not "atom_name" or "atom_name:..." */
-	*val = body + 1; /* "atom_name:val" */
-	return 1;
-}
-
 /*
  * In a format string, find the next occurrence of %(atom).
  */
-- 
2.6.4

[PATCH v3 07/15] ref-filter: skip deref specifier in match_atom_name()

[Karthik Nayak]

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

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

diff --git a/ref-filter.c b/ref-filter.c
index 575d293..b54c872 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -37,6 +37,10 @@ static int match_atom_name(const char *name, const char *atom_name, const char *
 {
 	const char *body;
 
+	/* skip the deref specifier */
+	if (name[0] == '*')
+		name++;
+
 	if (!skip_prefix(name, atom_name, &body))
 		return 0; /* doesn't even begin with "atom_name" */
 	if (!body[0]) {
-- 
2.6.4

[PATCH v3 08/15] 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 | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index b54c872..9708d67 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -29,6 +29,9 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 static struct used_atom {
 	const char *name;
 	cmp_type type;
+	union {
+		char *color;
+	} u;
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
 static int need_color_reset_at_eol;
@@ -53,6 +56,18 @@ static int match_atom_name(const char *name, const char *atom_name, const char *
 	return 1;
 }
 
+static void color_atom_parser(struct used_atom *atom)
+{
+	if (!match_atom_name(atom->name, "color", (const char **)&atom->u.color))
+		die("BUG: parsing non-'color'");
+	if (!atom->u.color)
+		die(_("expected format: %%(color:<color>)"));
+	/* atom->u.color points to part of atom->name */
+	atom->u.color = xstrdup(atom->u.color);
+	if (color_parse(atom->u.color, atom->u.color) < 0)
+		die(_("invalid color value: %s"), atom->u.color);
+}
+
 static struct {
 	const char *name;
 	cmp_type cmp_type;
@@ -90,7 +105,7 @@ static struct {
 	{ "symref" },
 	{ "flag" },
 	{ "HEAD" },
-	{ "color" },
+	{ "color", FIELD_STR, color_atom_parser },
 	{ "align" },
 	{ "end" },
 };
@@ -175,6 +190,7 @@ 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;
+	memset(&used_atom[at].u, 0, sizeof(used_atom[at].u));
 	if (valid_atom[i].parser)
 		valid_atom[i].parser(&used_atom[at]);
 	if (*atom == '*')
@@ -794,6 +810,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;
@@ -834,14 +851,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.6.4

Re: [PATCH v3 08/15] ref-filter: introduce color_atom_parser()

[Junio C Hamano]

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

> 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 | 29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index b54c872..9708d67 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -29,6 +29,9 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
>  static struct used_atom {
>  	const char *name;
>  	cmp_type type;
> +	union {
> +		char *color;
> +	} u;
>  } *used_atom;
>  static int used_atom_cnt, need_tagged, need_symref;
>  static int need_color_reset_at_eol;
> @@ -53,6 +56,18 @@ static int match_atom_name(const char *name, const char *atom_name, const char *
>  	return 1;
>  }
>  
> +static void color_atom_parser(struct used_atom *atom)
> +{
> +	if (!match_atom_name(atom->name, "color", (const char **)&atom->u.color))
> +		die("BUG: parsing non-'color'");
> +	if (!atom->u.color)
> +		die(_("expected format: %%(color:<color>)"));
> +	/* atom->u.color points to part of atom->name */
> +	atom->u.color = xstrdup(atom->u.color);
> +	if (color_parse(atom->u.color, atom->u.color) < 0)
> +		die(_("invalid color value: %s"), atom->u.color);

Is this calling color_parse() from color.c?

The function wants the destination to be at least COLOR_MAXLEN, but
I do not see where the piece memory pointed by atom->u.color is
guaranteed to be that long in the new code.  Looking at the code
removed by this patch, it used to correctly use a buffer that is
COLOR_MAXLEN bytes long.  So...

	const char *color_value;

	if (!match_atom_name(atom->name, "color", color_value))
		die("BUG: parsing non-'color'");
	if (!color_value)
		die(_("expected format: %%(color:<color>)"));
	atom->u.color = xmalloc(COLOR_MAXLEN);
        if (color_parse(color_value, atom->u.color) < 0)
		die(_("invalid color value: %s"), color_value);

or even define it in the union, i.e.

	union {
        	char color[COLOR_MAXLEN];
	} u;

and then use atom->u.color[] in-place?

Re: [PATCH v3 08/15] ref-filter: introduce color_atom_parser()

[Karthik Nayak]

On Wed, Jan 6, 2016 at 2:19 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> 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 | 29 ++++++++++++++++++++---------
>>  1 file changed, 20 insertions(+), 9 deletions(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index b54c872..9708d67 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -29,6 +29,9 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
>>  static struct used_atom {
>>       const char *name;
>>       cmp_type type;
>> +     union {
>> +             char *color;
>> +     } u;
>>  } *used_atom;
>>  static int used_atom_cnt, need_tagged, need_symref;
>>  static int need_color_reset_at_eol;
>> @@ -53,6 +56,18 @@ static int match_atom_name(const char *name, const char *atom_name, const char *
>>       return 1;
>>  }
>>
>> +static void color_atom_parser(struct used_atom *atom)
>> +{
>> +     if (!match_atom_name(atom->name, "color", (const char **)&atom->u.color))
>> +             die("BUG: parsing non-'color'");
>> +     if (!atom->u.color)
>> +             die(_("expected format: %%(color:<color>)"));
>> +     /* atom->u.color points to part of atom->name */
>> +     atom->u.color = xstrdup(atom->u.color);
>> +     if (color_parse(atom->u.color, atom->u.color) < 0)
>> +             die(_("invalid color value: %s"), atom->u.color);
>
> Is this calling color_parse() from color.c?
>

Yes it is!

> The function wants the destination to be at least COLOR_MAXLEN, but
> I do not see where the piece memory pointed by atom->u.color is
> guaranteed to be that long in the new code.  Looking at the code
> removed by this patch, it used to correctly use a buffer that is
> COLOR_MAXLEN bytes long.  So...
>
>         const char *color_value;
>
>         if (!match_atom_name(atom->name, "color", color_value))
>                 die("BUG: parsing non-'color'");
>         if (!color_value)
>                 die(_("expected format: %%(color:<color>)"));
>         atom->u.color = xmalloc(COLOR_MAXLEN);
>         if (color_parse(color_value, atom->u.color) < 0)
>                 die(_("invalid color value: %s"), color_value);
>
> or even define it in the union, i.e.
>
>         union {
>                 char color[COLOR_MAXLEN];
>         } u;
>
> and then use atom->u.color[] in-place?

I like the in-place suggestion, I wasn't wary of the Minimum length requirement
of the dest in color_parser(). Thanks for bringing it up. Will change.

-- 
Regards,
Karthik Nayak

Re: [PATCH v3 08/15] ref-filter: introduce color_atom_parser()

[Junio C Hamano]

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

> @@ -90,7 +105,7 @@ static struct {
>  	{ "symref" },
>  	{ "flag" },
>  	{ "HEAD" },
> -	{ "color" },
> +	{ "color", FIELD_STR, color_atom_parser },
>  	{ "align" },
>  	{ "end" },
>  };

This is minor, as I do not think anybody sane would say
"for-each-ref --sort=color" and expect anything sensible, but

I think we shouldn't mark these bogus attributes [*1*] as FIELD_STR
(and it is not FIELD_ULONG, either).  Perhaps add a new FIELD_BOGUS
(or some better name to make it clear that this is not a "value"
that belongs to the ref and can be used to sort, e.g. "FAKE") value
and mark them as such?

That would allow us to error out when they are specified as a
sorting criteria.


[Footnote]

*1* Bogus from the point of view of "what are the various attributes
specific to these items that the command is iterating over,
i.e. refs (or the objects at the tips of these refs)", as color,
align, etc. are not attributes per refs.

Re: [PATCH v3 08/15] ref-filter: introduce color_atom_parser()

[Karthik Nayak]

On Wed, Jan 6, 2016 at 2:36 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> @@ -90,7 +105,7 @@ static struct {
>>       { "symref" },
>>       { "flag" },
>>       { "HEAD" },
>> -     { "color" },
>> +     { "color", FIELD_STR, color_atom_parser },
>>       { "align" },
>>       { "end" },
>>  };
>
> This is minor, as I do not think anybody sane would say
> "for-each-ref --sort=color" and expect anything sensible, but
>

I completely understand and the only reason that I added FIELD_STR
cause that was the default either ways.

> I think we shouldn't mark these bogus attributes [*1*] as FIELD_STR
> (and it is not FIELD_ULONG, either).  Perhaps add a new FIELD_BOGUS
> (or some better name to make it clear that this is not a "value"
> that belongs to the ref and can be used to sort, e.g. "FAKE") value
> and mark them as such?
>
> That would allow us to error out when they are specified as a
> sorting criteria.
>
>
> [Footnote]
>
> *1* Bogus from the point of view of "what are the various attributes
> specific to these items that the command is iterating over,
> i.e. refs (or the objects at the tips of these refs)", as color,
> align, etc. are not attributes per refs.

I'm sure something like FIELD_BOGUS would make a lot of sense, but I wont
include that into this series. Maybe after this?

-- 
Regards,
Karthik Nayak

[PATCH v3 09/15] 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().

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 | 83 +++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 46 insertions(+), 37 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 9708d67..fa081a8 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;
+		struct align align;
 	} u;
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
@@ -68,6 +74,43 @@ static void color_atom_parser(struct used_atom *atom)
 		die(_("invalid color value: %s"), atom->u.color);
 }
 
+static void align_atom_parser(struct used_atom *atom)
+{
+	struct align *align = &atom->u.align;
+	const char *buf = NULL;
+	struct strbuf **s, **to_free;
+	int width = -1;
+
+	if (!match_atom_name(atom->name, "align", &buf))
+		die("BUG: parsing non-'align'");
+	if (!buf)
+		die(_("expected format: %%(align:<width>,<position>)"));
+	s = to_free = strbuf_split_str_omit_term(buf, ',', 0);
+
+	align->position = ALIGN_LEFT;
+
+	while (*s) {
+		buf = s[0]->buf;
+
+		if (!strtoul_ui(buf, 10, (unsigned int *)&width))
+			;
+		else if (!strcmp(buf, "left"))
+			align->position = ALIGN_LEFT;
+		else if (!strcmp(buf, "right"))
+			align->position = ALIGN_RIGHT;
+		else if (!strcmp(buf, "middle"))
+			align->position = ALIGN_MIDDLE;
+		else
+			die(_("unrecognized %%(align) argument: %s"), buf);
+		s++;
+	}
+
+	if (width < 0)
+		die(_("positive width expected with the %%(align) atom"));
+	align->width = width;
+	strbuf_list_free(to_free);
+}
+
 static struct {
 	const char *name;
 	cmp_type cmp_type;
@@ -106,17 +149,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;
@@ -816,7 +854,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;
@@ -880,36 +917,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) {
-				if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))
-					;
-				else if (!strcmp(s[0]->buf, "left"))
-					align->position = ALIGN_LEFT;
-				else if (!strcmp(s[0]->buf, "right"))
-					align->position = ALIGN_RIGHT;
-				else if (!strcmp(s[0]->buf, "middle"))
-					align->position = ALIGN_MIDDLE;
-				else
-					die(_("improper format entered align:%s"), s[0]->buf);
-				s++;
-			}
-
-			if (width < 0)
-				die(_("positive width expected with the %%(align) atom"));
-			align->width = width;
-			strbuf_list_free(to_free);
+		} else if (starts_with(name, "align")) {
+			v->u.align = atom->u.align;
 			v->handler = align_atom_handler;
 			continue;
 		} else if (!strcmp(name, "end")) {
-- 
2.6.4

[PATCH v3 10/15] ref-filter: introduce parse_align_position()

[Karthik Nayak]

>From align_atom_parser() form 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 | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index fa081a8..ccad4c3 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -74,6 +74,17 @@ static void color_atom_parser(struct used_atom *atom)
 		die(_("invalid color value: %s"), atom->u.color);
 }
 
+static align_type parse_align_position(const char *s)
+{
+	if (!strcmp(s, "right"))
+		return ALIGN_RIGHT;
+	else if (!strcmp(s, "middle"))
+		return ALIGN_MIDDLE;
+	else if (!strcmp(s, "left"))
+		return ALIGN_LEFT;
+	return -1;
+}
+
 static void align_atom_parser(struct used_atom *atom)
 {
 	struct align *align = &atom->u.align;
@@ -90,16 +101,13 @@ static void align_atom_parser(struct used_atom *atom)
 	align->position = ALIGN_LEFT;
 
 	while (*s) {
+		int position;
 		buf = s[0]->buf;
 
 		if (!strtoul_ui(buf, 10, (unsigned int *)&width))
 			;
-		else if (!strcmp(buf, "left"))
-			align->position = ALIGN_LEFT;
-		else if (!strcmp(buf, "right"))
-			align->position = ALIGN_RIGHT;
-		else if (!strcmp(buf, "middle"))
-			align->position = ALIGN_MIDDLE;
+		else if ((position = parse_align_position(buf)) >= 0)
+			align->position = position;
 		else
 			die(_("unrecognized %%(align) argument: %s"), buf);
 		s++;
-- 
2.6.4

Re: [PATCH v3 10/15] ref-filter: introduce parse_align_position()

[Eric Sunshine]

On Tue, Jan 5, 2016 at 3:03 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> From align_atom_parser() form 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>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -74,6 +74,17 @@ static void color_atom_parser(struct used_atom *atom)
> +static align_type parse_align_position(const char *s)
> +{
> +       if (!strcmp(s, "right"))
> +               return ALIGN_RIGHT;
> +       else if (!strcmp(s, "middle"))
> +               return ALIGN_MIDDLE;
> +       else if (!strcmp(s, "left"))
> +               return ALIGN_LEFT;
> +       return -1;
> +}

This code was just moved in patch 9/15 and is being relocated again
here in patch 10/15. If you change the order of the patches so that
this preparatory refactoring is done first, the diff of the "introduce
align_atom_parser()" patch will become smaller and be a bit easier to
review. (Plus it just makes sense to do preparation first.)

>  static void align_atom_parser(struct used_atom *atom)
>  {
>         struct align *align = &atom->u.align;
> @@ -90,16 +101,13 @@ static void align_atom_parser(struct used_atom *atom)
>         align->position = ALIGN_LEFT;
>
>         while (*s) {
> +               int position;
>                 buf = s[0]->buf;
>
>                 if (!strtoul_ui(buf, 10, (unsigned int *)&width))
>                         ;
> -               else if (!strcmp(buf, "left"))
> -                       align->position = ALIGN_LEFT;
> -               else if (!strcmp(buf, "right"))
> -                       align->position = ALIGN_RIGHT;
> -               else if (!strcmp(buf, "middle"))
> -                       align->position = ALIGN_MIDDLE;
> +               else if ((position = parse_align_position(buf)) >= 0)
> +                       align->position = position;
>                 else
>                         die(_("unrecognized %%(align) argument: %s"), buf);
>                 s++;
> --
> 2.6.4

Re: [PATCH v3 10/15] ref-filter: introduce parse_align_position()

[Karthik Nayak]

On Tue, Jan 26, 2016 at 3:19 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Jan 5, 2016 at 3:03 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> From align_atom_parser() form 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>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -74,6 +74,17 @@ static void color_atom_parser(struct used_atom *atom)
>> +static align_type parse_align_position(const char *s)
>> +{
>> +       if (!strcmp(s, "right"))
>> +               return ALIGN_RIGHT;
>> +       else if (!strcmp(s, "middle"))
>> +               return ALIGN_MIDDLE;
>> +       else if (!strcmp(s, "left"))
>> +               return ALIGN_LEFT;
>> +       return -1;
>> +}
>
> This code was just moved in patch 9/15 and is being relocated again
> here in patch 10/15. If you change the order of the patches so that
> this preparatory refactoring is done first, the diff of the "introduce
> align_atom_parser()" patch will become smaller and be a bit easier to
> review. (Plus it just makes sense to do preparation first.)
>

It does reduce the diff as you said, I have swapped the order as you suggested.

-- 
Regards,
Karthik Nayak

[PATCH v3 11/15] ref-filter: convert variable 'width' to an unsigned int

[Karthik Nayak]

In align_atom_parser() the 'width' variable is an int, which requires
an explicit type conversion when used in strtoul_ui(). Hence make it
an unsigned int.

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

diff --git a/ref-filter.c b/ref-filter.c
index ccad4c3..4f623a0 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -90,7 +90,7 @@ static void align_atom_parser(struct used_atom *atom)
 	struct align *align = &atom->u.align;
 	const char *buf = NULL;
 	struct strbuf **s, **to_free;
-	int width = -1;
+	unsigned int width = ~0U;
 
 	if (!match_atom_name(atom->name, "align", &buf))
 		die("BUG: parsing non-'align'");
@@ -104,7 +104,7 @@ static void align_atom_parser(struct used_atom *atom)
 		int position;
 		buf = s[0]->buf;
 
-		if (!strtoul_ui(buf, 10, (unsigned int *)&width))
+		if (!strtoul_ui(buf, 10, &width))
 			;
 		else if ((position = parse_align_position(buf)) >= 0)
 			align->position = position;
@@ -113,7 +113,7 @@ static void align_atom_parser(struct used_atom *atom)
 		s++;
 	}
 
-	if (width < 0)
+	if (width == ~0U)
 		die(_("positive width expected with the %%(align) atom"));
 	align->width = width;
 	strbuf_list_free(to_free);
-- 
2.6.4

Re: [PATCH v3 11/15] ref-filter: convert variable 'width' to an unsigned int

[Junio C Hamano]

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

> In align_atom_parser() the 'width' variable is an int, which requires
> an explicit type conversion when used in strtoul_ui(). Hence make it
> an unsigned int.
>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---

Shouldn't this be done in [09/15] from the beginning?  As it's not
like [09/15] was a pure code movement, this step looks like "Oops,
I screwed up earlier, and here is a fixup".

If we want to do this as a separate step, it would be easier to
follow if it is done _before_ 09/15 to the original of the code
movement, I think.

>  ref-filter.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index ccad4c3..4f623a0 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -90,7 +90,7 @@ static void align_atom_parser(struct used_atom *atom)
>  	struct align *align = &atom->u.align;
>  	const char *buf = NULL;
>  	struct strbuf **s, **to_free;
> -	int width = -1;
> +	unsigned int width = ~0U;
>  
>  	if (!match_atom_name(atom->name, "align", &buf))
>  		die("BUG: parsing non-'align'");
> @@ -104,7 +104,7 @@ static void align_atom_parser(struct used_atom *atom)
>  		int position;
>  		buf = s[0]->buf;
>  
> -		if (!strtoul_ui(buf, 10, (unsigned int *)&width))
> +		if (!strtoul_ui(buf, 10, &width))
>  			;
>  		else if ((position = parse_align_position(buf)) >= 0)
>  			align->position = position;
> @@ -113,7 +113,7 @@ static void align_atom_parser(struct used_atom *atom)
>  		s++;
>  	}
>  
> -	if (width < 0)
> +	if (width == ~0U)
>  		die(_("positive width expected with the %%(align) atom"));
>  	align->width = width;
>  	strbuf_list_free(to_free);

Re: [PATCH v3 11/15] ref-filter: convert variable 'width' to an unsigned int

[Karthik Nayak]

On Wed, Jan 6, 2016 at 2:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> In align_atom_parser() the 'width' variable is an int, which requires
>> an explicit type conversion when used in strtoul_ui(). Hence make it
>> an unsigned int.
>>
>> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>
> Shouldn't this be done in [09/15] from the beginning?  As it's not
> like [09/15] was a pure code movement, this step looks like "Oops,
> I screwed up earlier, and here is a fixup".
>
> If we want to do this as a separate step, it would be easier to
> follow if it is done _before_ 09/15 to the original of the code
> movement, I think.
>

Haha, does seem that way. Will merge it in.

-- 
Regards,
Karthik Nayak

[PATCH v3 12/15] 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 Documetation 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     | 41 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index c6f073c..9af0f53 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -129,14 +129,18 @@ color::
 
 align::
 	Left-, middle-, or right-align the content between
-	%(align:...) and %(end). The "align:" is followed by `<width>`
-	and `<position>` in any order separated by a comma, where the
-	`<position>` is either left, right or middle, default being
-	left and `<width>` is the total length of the content with
-	alignment. If the contents length is more than the width then
-	no alignment is performed. If used with '--quote' everything
-	in between %(align:...) and %(end) is quoted, but if nested
-	then only the topmost level performs quoting.
+	%(align:...) and %(end). The "align:" is followed by
+	`width=<width>` and `position=<position>` in any order
+	separated by a comma, where the `<position>` is either left,
+	right or middle, default being left and `<width>` is the total
+	length of the content with alignment. For brevity, the
+	"width=" and/or "position=" prefixes may be omitted, and bare
+	<width> and <position> used instead.  For instance,
+	`%(align:<width>,<position>)`. If the contents length is more
+	than the width then no alignment is performed. If used with
+	'--quote' everything in between %(align:...) and %(end) is
+	quoted, but if nested then only the topmost level performs
+	quoting.
 
 In addition to the above, for commit and tag objects, the header
 field names (`tree`, `parent`, `object`, `type`, and `tag`) can
diff --git a/ref-filter.c b/ref-filter.c
index 4f623a0..df0b114 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -104,7 +104,15 @@ static void align_atom_parser(struct used_atom *atom)
 		int position;
 		buf = s[0]->buf;
 
-		if (!strtoul_ui(buf, 10, &width))
+		if (skip_prefix(buf, "position=", &buf)) {
+			position = parse_align_position(buf);
+			if (position < 0)
+				die(_("unrecognized position:%s"), buf);
+			align->position = position;
+		} else if (skip_prefix(buf, "width=", &buf)) {
+			if (strtoul_ui(buf, 10, &width))
+				die(_("unrecognized width:%s"), buf);
+		} else if (!strtoul_ui(buf, 10, &width))
 			;
 		else if ((position = parse_align_position(buf)) >= 0)
 			align->position = position;
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index fe4796c..0c4417f 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -133,6 +133,47 @@ 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 permutations' '
+		git for-each-ref --format="|%(align:$option)refname is %(refname)%(end)|%(refname)" >actual &&
+		test_cmp expect actual
+		'
+	done;
+}
+
+test_align_permutations <<-\EOF
+	middle,42
+	42,middle
+	position=middle,42
+	42,position=middle
+	middle,width=42
+	width=42,middle
+	position=middle,width=42
+	width=42,position=middle
+EOF
+
+# Last one wins (silently) when multiple arguments of the same type are given
+
+test_align_permutations <<-\EOF
+	32,width=42,middle
+	width=30,42,middle
+	width=42,position=right,middle
+	42,right,position=middle
+EOF
+
 # Individual atoms inside %(align:...) and %(end) must not be quoted.
 
 test_expect_success 'alignment with format quote' "
-- 
2.6.4

Re: [PATCH v3 12/15] ref-filter: align: introduce long-form syntax

[Eric Sunshine]

On Tue, Jan 5, 2016 at 3:03 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Introduce optional prefixes "width=" and "position=" for the align atom
> so that the atom can be used as "%(align:width=<width>,position=<position>)".
>
> Add Documetation and tests for the same.
>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> index fe4796c..0c4417f 100755
> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -133,6 +133,47 @@ test_expect_success 'right alignment' '
> +cat >expect <<-\EOF
> +|       refname is refs/heads/master       |refs/heads/master
> +|        refname is refs/heads/side        |refs/heads/side
> +|         refname is refs/odd/spot         |refs/odd/spot
> +|     refname is refs/tags/double-tag      |refs/tags/double-tag
> +|        refname is refs/tags/four         |refs/tags/four
> +|         refname is refs/tags/one         |refs/tags/one
> +|     refname is refs/tags/signed-tag      |refs/tags/signed-tag
> +|        refname is refs/tags/three        |refs/tags/three
> +|         refname is refs/tags/two         |refs/tags/two
> +EOF
> +
> +test_align_permutations() {

Style: in shell scripts, add space before ()

> +       while read -r option; do

Style: drop semi-colon and place 'do' on its own line

> +               test_expect_success 'align permutations' '

This title is not as illuminating as it could be. It would be better
to indicate which permutation is being tested. For instance:

    test_expect_success "align:$option" ...

Note the double-quotes. Or:

    test_expect_success "align permutation: $option" ...

or something.

> +               git for-each-ref --format="|%(align:$option)refname is %(refname)%(end)|%(refname)" >actual &&

This is not wrong, per se, but referencing $option inside the
non-interpolating single-quote context of the test body makes it a bit
harder to understand than it need be. As it is, at the time that the
test body actually gets executed, $option still evaluates to the
desired permutation value so it works. However, it would show intent
more clearly and be more robust to use a double-quote context to
interpolate $option into the git-for-each-ref invocation directly
rather than allowing the test body to pick up the value at execution
time.

Fixing this means using double- rather than single-quotes for the test
body, which means you'd also want to flip the  double-quotes wrapping
the --format= argument over to single-quotes. Also, for style
consistency, indent the test body. The end result should be something
like this:

    test_align_permutations () {
        while ...
        do
            test_expect_success "align:$option" "
                git for-each-ref --format='...$option...' >actual &&
                ...
            "
        done
    }

> +               test_cmp expect actual
> +               '
> +       done;

Style: drop the semi-colon

More below...

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

Overall, this version is much nicer now that the tests are
table-driven rather than each permutation being copied/pasted.

This is a tangent, but it's disappointing that this entire test script
is skipped if GPG is not installed, especially since none of these
tests seem to care at all about signed tags. By requiring GPG
(unnecessarily), these tests likely are not getting run as widely as
they ought to. Consequently, it probably would be a good idea to drop
the GPG requirement from the top of the file and have the "setup" test
create lightweight tags ("git tag ...") rather than signed ones ("git
tag -s ...").

Re: [PATCH v3 12/15] ref-filter: align: introduce long-form syntax

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> +               git for-each-ref --format="|%(align:$option)refname is %(refname)%(end)|%(refname)" >actual &&
>
> This is not wrong, per se, but referencing $option inside the
> non-interpolating single-quote context of the test body makes it a bit
> harder to understand than it need be. As it is, at the time that the
> test body actually gets executed, $option still evaluates to the
> desired permutation value so it works. 

I think I said this in one of my recent reviews to somebody else,
but this needs repeating.

My position on this has been and still is a complete opposite.  It
is a designed behaviour that variable references made inside test
body that is quoted with sq pair can see the values of the variables
that are defined outside test_expect_success, and you can freely use
the global $TRASH_DIRECTORY, $_x40, etc. inside the test body that
is quoted with single quote.  The $option here is no different.

In fact, it is more desirable to avoid double-quoting the test body.
The problem with using double-quote around the test body and
formulating the executable shell script using variable interpolation
before test_expect_success even runs is that you would be tempted to
do something silly like this:

    HEAD_SHA1=$(git rev-parse --verify HEAD)

    test_expect_success "check something" "
	HEAD_SHA1=$(git rev-parse --verify HEAD) &&
	... do other things that should not move the HEAD ... &&
	test '$(git rev-parse HEAD)' = '$HEAD_SHA1' &&
	test '$SOME_VAR' = '$OTHER_VAR' &&
    "

It is not just error prone (if variable reference had a shell
metacharacter in it, e.g. a single-quote in OTHER_VAR's value, you'd
have a syntax error in the test body), but because two invocations
of rev-parse in the above are both made before test_expect_success
runs, and would yield the same value.  It is not even testing the
right thing.  If you broke rev-parse, the invocation will not fail
inside test_expect_success but outside when the arguments to that
helper is being prepared.

So please do not write something like this:

>     test_align_permutations () {
>         while ...
>         do
>             test_expect_success "align:$option" "
>                 git for-each-ref --format='...$option...' >actual &&
>                 ...
>             "
>         done
>     }

but make it more like this:

	for option in ...
        do
        	test_expect_success "align:$option" '
                	git for-each-ref --format="...$option..." &&
                        ...
		'
	done

Re: [PATCH v3 12/15] ref-filter: align: introduce long-form syntax

[Karthik Nayak]

On Tue, Jan 26, 2016 at 5:15 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>>> +               git for-each-ref --format="|%(align:$option)refname is %(refname)%(end)|%(refname)" >actual &&
>>
>> This is not wrong, per se, but referencing $option inside the
>> non-interpolating single-quote context of the test body makes it a bit
>> harder to understand than it need be. As it is, at the time that the
>> test body actually gets executed, $option still evaluates to the
>> desired permutation value so it works.
>
> I think I said this in one of my recent reviews to somebody else,
> but this needs repeating.
>
> My position on this has been and still is a complete opposite.  It
> is a designed behaviour that variable references made inside test
> body that is quoted with sq pair can see the values of the variables
> that are defined outside test_expect_success, and you can freely use
> the global $TRASH_DIRECTORY, $_x40, etc. inside the test body that
> is quoted with single quote.  The $option here is no different.
>
> In fact, it is more desirable to avoid double-quoting the test body.
> The problem with using double-quote around the test body and
> formulating the executable shell script using variable interpolation
> before test_expect_success even runs is that you would be tempted to
> do something silly like this:
>
>     HEAD_SHA1=$(git rev-parse --verify HEAD)
>
>     test_expect_success "check something" "
>         HEAD_SHA1=$(git rev-parse --verify HEAD) &&
>         ... do other things that should not move the HEAD ... &&
>         test '$(git rev-parse HEAD)' = '$HEAD_SHA1' &&
>         test '$SOME_VAR' = '$OTHER_VAR' &&
>     "
>
> It is not just error prone (if variable reference had a shell
> metacharacter in it, e.g. a single-quote in OTHER_VAR's value, you'd
> have a syntax error in the test body), but because two invocations
> of rev-parse in the above are both made before test_expect_success
> runs, and would yield the same value.  It is not even testing the
> right thing.  If you broke rev-parse, the invocation will not fail
> inside test_expect_success but outside when the arguments to that
> helper is being prepared.
>
> So please do not write something like this:
>
>>     test_align_permutations () {
>>         while ...
>>         do
>>             test_expect_success "align:$option" "
>>                 git for-each-ref --format='...$option...' >actual &&
>>                 ...
>>             "
>>         done
>>     }
>
> but make it more like this:
>
>         for option in ...
>         do
>                 test_expect_success "align:$option" '
>                         git for-each-ref --format="...$option..." &&
>                         ...
>                 '
>         done
>

Thanks for this descriptive explanation, I shall use the suggested format.

-- 
Regards,
Karthik Nayak

Re: [PATCH v3 12/15] ref-filter: align: introduce long-form syntax

[Karthik Nayak]

On Tue, Jan 26, 2016 at 4:28 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Jan 5, 2016 at 3:03 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Introduce optional prefixes "width=" and "position=" for the align atom
>> so that the atom can be used as "%(align:width=<width>,position=<position>)".
>>
>> Add Documetation and tests for the same.
>>
>> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
>> index fe4796c..0c4417f 100755
>> --- a/t/t6302-for-each-ref-filter.sh
>> +++ b/t/t6302-for-each-ref-filter.sh
>> @@ -133,6 +133,47 @@ test_expect_success 'right alignment' '
>> +cat >expect <<-\EOF
>> +|       refname is refs/heads/master       |refs/heads/master
>> +|        refname is refs/heads/side        |refs/heads/side
>> +|         refname is refs/odd/spot         |refs/odd/spot
>> +|     refname is refs/tags/double-tag      |refs/tags/double-tag
>> +|        refname is refs/tags/four         |refs/tags/four
>> +|         refname is refs/tags/one         |refs/tags/one
>> +|     refname is refs/tags/signed-tag      |refs/tags/signed-tag
>> +|        refname is refs/tags/three        |refs/tags/three
>> +|         refname is refs/tags/two         |refs/tags/two
>> +EOF
>> +
>> +test_align_permutations() {
>
> Style: in shell scripts, add space before ()
>

Will change.

>> +       while read -r option; do
>
> Style: drop semi-colon and place 'do' on its own line

Will change.

>
>> +               test_expect_success 'align permutations' '
>
> This title is not as illuminating as it could be. It would be better
> to indicate which permutation is being tested. For instance:
>
>     test_expect_success "align:$option" ...
>
> Note the double-quotes. Or:
>
>     test_expect_success "align permutation: $option" ...
>
> or something.
>

"align:$option" seems right, will change.

>> +               git for-each-ref --format="|%(align:$option)refname is %(refname)%(end)|%(refname)" >actual &&
>
> This is not wrong, per se, but referencing $option inside the
> non-interpolating single-quote context of the test body makes it a bit
> harder to understand than it need be. As it is, at the time that the
> test body actually gets executed, $option still evaluates to the
> desired permutation value so it works. However, it would show intent
> more clearly and be more robust to use a double-quote context to
> interpolate $option into the git-for-each-ref invocation directly
> rather than allowing the test body to pick up the value at execution
> time.
>
> Fixing this means using double- rather than single-quotes for the test
> body, which means you'd also want to flip the  double-quotes wrapping
> the --format= argument over to single-quotes. Also, for style
> consistency, indent the test body. The end result should be something
> like this:
>
>     test_align_permutations () {
>         while ...
>         do
>             test_expect_success "align:$option" "
>                 git for-each-ref --format='...$option...' >actual &&
>                 ...
>             "
>         done
>     }
>

After reading this and Junio's detailed description stating otherwise.
It's safe to
go with something similar to what Junio suggested :

        for option in ...
        do
                test_expect_success "align:$option" '
                        git for-each-ref --format="...$option..." &&
                        ...
                '
        done

>> +               test_cmp expect actual
>> +               '
>> +       done;
>
> Style: drop the semi-colon
>

Will change.

> More below...
>
>> +}
>> +
>> +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
>
> Overall, this version is much nicer now that the tests are
> table-driven rather than each permutation being copied/pasted.
>

Thanks.

> This is a tangent, but it's disappointing that this entire test script
> is skipped if GPG is not installed, especially since none of these
> tests seem to care at all about signed tags. By requiring GPG
> (unnecessarily), these tests likely are not getting run as widely as
> they ought to. Consequently, it probably would be a good idea to drop
> the GPG requirement from the top of the file and have the "setup" test
> create lightweight tags ("git tag ...") rather than signed ones ("git
> tag -s ...").

Maybe an independent patch post this series. I'll keep a note to myself.

-- 
Regards,
Karthik Nayak

Re: [PATCH v3 12/15] ref-filter: align: introduce long-form syntax

[Christian Couder]

On Tue, Jan 5, 2016 at 9:03 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Introduce optional prefixes "width=" and "position=" for the align atom
> so that the atom can be used as "%(align:width=<width>,position=<position>)".
>
> Add Documetation and tests for the same.

s/Documetation/Documentation/

Thanks!

Re: [PATCH v3 12/15] ref-filter: align: introduce long-form syntax

[Karthik Nayak]

On Tue, Jan 26, 2016 at 10:46 AM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Tue, Jan 5, 2016 at 9:03 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Introduce optional prefixes "width=" and "position=" for the align atom
>> so that the atom can be used as "%(align:width=<width>,position=<position>)".
>>
>> Add Documetation and tests for the same.
>
> s/Documetation/Documentation/
>
> Thanks!

Ah! thanks for that.

-- 
Regards,
Karthik Nayak

[PATCH v3 13/15] 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 | 107 ++++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 66 insertions(+), 41 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index df0b114..32b4674 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -37,6 +37,8 @@ static struct used_atom {
 	union {
 		char *color;
 		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;
@@ -74,6 +76,26 @@ static void color_atom_parser(struct used_atom *atom)
 		die(_("invalid color value: %s"), atom->u.color);
 }
 
+static void remote_ref_atom_parser(struct used_atom *atom)
+{
+	const char *buf;
+
+	buf = strchr(atom->name, ':');
+	if (!buf) {
+		atom->u.remote_ref = RR_NORMAL;
+		return;
+	}
+	buf++;
+	if (!strcmp(buf, "short"))
+		atom->u.remote_ref = RR_SHORTEN;
+	else if (!strcmp(buf, "track"))
+		atom->u.remote_ref = RR_TRACK;
+	else if (!strcmp(buf, "trackshort"))
+		atom->u.remote_ref = RR_TRACKSHORT;
+	else
+		die(_("unrecognized format: %%(%s)"), atom->name);
+}
+
 static align_type parse_align_position(const char *s)
 {
 	if (!strcmp(s, "right"))
@@ -159,8 +181,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" },
@@ -841,6 +863,43 @@ static inline char *copy_advance(char *dst, const char *src)
 	return dst;
 }
 
+static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
+				    struct branch *branch, const char **s)
+{
+	int num_ours, num_theirs;
+	if (atom->u.remote_ref == RR_SHORTEN)
+		*s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);
+	else if (atom->u.remote_ref == RR_TRACK) {
+		if (stat_tracking_info(branch, &num_ours,
+				       &num_theirs, NULL))
+			return;
+
+		if (!num_ours && !num_theirs)
+			*s = "";
+		else if (!num_ours)
+			*s = xstrfmt("[behind %d]", num_theirs);
+		else if (!num_theirs)
+			*s = xstrfmt("[ahead %d]", num_ours);
+		else
+			*s = xstrfmt("[ahead %d, behind %d]",
+				     num_ours, num_theirs);
+	} else if (atom->u.remote_ref == RR_TRACKSHORT) {
+		if (stat_tracking_info(branch, &num_ours,
+				       &num_theirs, NULL))
+			return;
+
+		if (!num_ours && !num_theirs)
+			*s = "=";
+		else if (!num_ours)
+			*s = "<";
+		else if (!num_theirs)
+			*s = ">";
+		else
+			*s = "<>";
+	} else if (atom->u.remote_ref == RR_NORMAL)
+		*s = refname;
+}
+
 /*
  * Parse the object referred by ref, and grab needed value.
  */
@@ -894,6 +953,8 @@ static void populate_value(struct ref_array_item *ref)
 			refname = branch_get_upstream(branch, NULL);
 			if (!refname)
 				continue;
+			fill_remote_ref_details(atom, refname, branch, &v->s);
+			continue;
 		} else if (starts_with(name, "push")) {
 			const char *branch_name;
 			if (!skip_prefix(ref->refname, "refs/heads/",
@@ -904,6 +965,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;
@@ -945,49 +1008,11 @@ static void populate_value(struct ref_array_item *ref)
 
 		formatp = strchr(name, ':');
 		if (formatp) {
-			int num_ours, num_theirs;
-
 			formatp++;
 			if (!strcmp(formatp, "short"))
 				refname = shorten_unambiguous_ref(refname,
 						      warn_ambiguous_refs);
-			else if (!strcmp(formatp, "track") &&
-				 (starts_with(name, "upstream") ||
-				  starts_with(name, "push"))) {
-
-				if (stat_tracking_info(branch, &num_ours,
-						       &num_theirs, NULL))
-					continue;
-
-				if (!num_ours && !num_theirs)
-					v->s = "";
-				else if (!num_ours)
-					v->s = xstrfmt("[behind %d]", num_theirs);
-				else if (!num_theirs)
-					v->s = xstrfmt("[ahead %d]", num_ours);
-				else
-					v->s = xstrfmt("[ahead %d, behind %d]",
-						       num_ours, num_theirs);
-				continue;
-			} else if (!strcmp(formatp, "trackshort") &&
-				   (starts_with(name, "upstream") ||
-				    starts_with(name, "push"))) {
-				assert(branch);
-
-				if (stat_tracking_info(branch, &num_ours,
-							&num_theirs, NULL))
-					continue;
-
-				if (!num_ours && !num_theirs)
-					v->s = "=";
-				else if (!num_ours)
-					v->s = "<";
-				else if (!num_theirs)
-					v->s = ">";
-				else
-					v->s = "<>";
-				continue;
-			} else
+			else
 				die("unknown %.*s format %s",
 				    (int)(formatp - name), name, formatp);
 		}
-- 
2.6.4

Re: [PATCH v3 13/15] ref-filter: introduce remote_ref_atom_parser()

[Eric Sunshine]

On Tue, Jan 5, 2016 at 3:03 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Introduce remote_ref_atom_parser() which will parse the '%(upstream)'
> and '%(push)' atoms and store information into the 'used_atom'
> structure based on the modifiers used along with the corresponding
> atom.
>
> Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -841,6 +863,43 @@ static inline char *copy_advance(char *dst, const char *src)
> +static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
> +                                   struct branch *branch, const char **s)
> +{
> +       int num_ours, num_theirs;
> +       if (atom->u.remote_ref == RR_SHORTEN)
> +               *s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);
> +       else if (atom->u.remote_ref == RR_TRACK) {
> +               if (stat_tracking_info(branch, &num_ours,
> +                                      &num_theirs, NULL))
> +                       return;
> +
> +               if (!num_ours && !num_theirs)
> +                       *s = "";
> +               else if (!num_ours)
> +                       *s = xstrfmt("[behind %d]", num_theirs);
> +               else if (!num_theirs)
> +                       *s = xstrfmt("[ahead %d]", num_ours);
> +               else
> +                       *s = xstrfmt("[ahead %d, behind %d]",
> +                                    num_ours, num_theirs);
> +       } else if (atom->u.remote_ref == RR_TRACKSHORT) {
> +               if (stat_tracking_info(branch, &num_ours,
> +                                      &num_theirs, NULL))
> +                       return;
> +
> +               if (!num_ours && !num_theirs)
> +                       *s = "=";
> +               else if (!num_ours)
> +                       *s = "<";
> +               else if (!num_theirs)
> +                       *s = ">";
> +               else
> +                       *s = "<>";
> +       } else if (atom->u.remote_ref == RR_NORMAL)
> +               *s = refname;

I think I mentioned this in a previous review: If the code falls past
this final 'else if' for some reason (programmer error), then *s won't
get assigned at all, which is probably undesirable. To protect against
such a case, you might want either to add a final 'else':

    else
        die("BUG: ...");

or just consider RR_NORMAL the catchall case, and turn the final 'else
if' into a plain 'else':

    else /* RR_NORMAL */
        *s = refname;

> +}
> @@ -894,6 +953,8 @@ static void populate_value(struct ref_array_item *ref)
>                         refname = branch_get_upstream(branch, NULL);
>                         if (!refname)
>                                 continue;
> +                       fill_remote_ref_details(atom, refname, branch, &v->s);
> +                       continue;

There are now two 'continue' statements very close together here. Have
you considered this instead?

    if (refname)
        fill_remote_ref_details(...);
    continue;

It might make the code a bit more straightforward. (Genuine question;
I don't feel too strongly about it.)

>                 } else if (starts_with(name, "push")) {
>                         const char *branch_name;
>                         if (!skip_prefix(ref->refname, "refs/heads/",
> @@ -904,6 +965,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;

Ditto.

>                 } else if (starts_with(name, "color:")) {
>                         v->s = atom->u.color;
>                         continue;

Re: [PATCH v3 13/15] ref-filter: introduce remote_ref_atom_parser()

[Karthik Nayak]

On Tue, Jan 26, 2016 at 5:58 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Jan 5, 2016 at 3:03 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Introduce remote_ref_atom_parser() which will parse the '%(upstream)'
>> and '%(push)' atoms and store information into the 'used_atom'
>> structure based on the modifiers used along with the corresponding
>> atom.
>>
>> Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
>> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -841,6 +863,43 @@ static inline char *copy_advance(char *dst, const char *src)
>> +static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
>> +                                   struct branch *branch, const char **s)
>> +{
>> +       int num_ours, num_theirs;
>> +       if (atom->u.remote_ref == RR_SHORTEN)
>> +               *s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);
>> +       else if (atom->u.remote_ref == RR_TRACK) {
>> +               if (stat_tracking_info(branch, &num_ours,
>> +                                      &num_theirs, NULL))
>> +                       return;
>> +
>> +               if (!num_ours && !num_theirs)
>> +                       *s = "";
>> +               else if (!num_ours)
>> +                       *s = xstrfmt("[behind %d]", num_theirs);
>> +               else if (!num_theirs)
>> +                       *s = xstrfmt("[ahead %d]", num_ours);
>> +               else
>> +                       *s = xstrfmt("[ahead %d, behind %d]",
>> +                                    num_ours, num_theirs);
>> +       } else if (atom->u.remote_ref == RR_TRACKSHORT) {
>> +               if (stat_tracking_info(branch, &num_ours,
>> +                                      &num_theirs, NULL))
>> +                       return;
>> +
>> +               if (!num_ours && !num_theirs)
>> +                       *s = "=";
>> +               else if (!num_ours)
>> +                       *s = "<";
>> +               else if (!num_theirs)
>> +                       *s = ">";
>> +               else
>> +                       *s = "<>";
>> +       } else if (atom->u.remote_ref == RR_NORMAL)
>> +               *s = refname;
>
> I think I mentioned this in a previous review: If the code falls past
> this final 'else if' for some reason (programmer error), then *s won't
> get assigned at all, which is probably undesirable. To protect against
> such a case, you might want either to add a final 'else':
>
>     else
>         die("BUG: ...");
>
> or just consider RR_NORMAL the catchall case, and turn the final 'else
> if' into a plain 'else':
>
>     else /* RR_NORMAL */
>         *s = refname;
>

The latter seems to make sense, will implement.

>> +}
>> @@ -894,6 +953,8 @@ static void populate_value(struct ref_array_item *ref)
>>                         refname = branch_get_upstream(branch, NULL);
>>                         if (!refname)
>>                                 continue;
>> +                       fill_remote_ref_details(atom, refname, branch, &v->s);
>> +                       continue;
>
> There are now two 'continue' statements very close together here. Have
> you considered this instead?
>
>     if (refname)
>         fill_remote_ref_details(...);
>     continue;
>
> It might make the code a bit more straightforward. (Genuine question;
> I don't feel too strongly about it.)

No, I didn't consider that, and I think it's a good change to implement.
Thanks. ( I don't see any problems with it)

>
>>                 } else if (starts_with(name, "push")) {
>>                         const char *branch_name;
>>                         if (!skip_prefix(ref->refname, "refs/heads/",
>> @@ -904,6 +965,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;
>
> Ditto.

Noted.

-- 
Regards,
Karthik Nayak

[PATCH v3 14/15] ref-filter: introduce contents_atom_parser()

[Karthik Nayak]

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

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

diff --git a/ref-filter.c b/ref-filter.c
index 32b4674..9e61530 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;
@@ -96,6 +100,35 @@ static void remote_ref_atom_parser(struct used_atom *atom)
 		die(_("unrecognized format: %%(%s)"), atom->name);
 }
 
+static void contents_atom_parser(struct used_atom *atom)
+{
+	const char * buf;
+
+	if (match_atom_name(atom->name, "subject", &buf) && !buf) {
+		atom->u.contents.option = C_SUB;
+		return;
+	} else if (match_atom_name(atom->name, "body", &buf) && !buf) {
+		atom->u.contents.option = C_BODY_DEP;
+		return;
+	} if (!match_atom_name(atom->name, "contents", &buf))
+		  die("BUG: parsing non-'contents'");
+
+	if (!buf)
+		atom->u.contents.option = C_BARE;
+	else if (!strcmp(buf, "body"))
+		atom->u.contents.option = C_BODY;
+	else if (!strcmp(buf, "signature"))
+		atom->u.contents.option = C_SIG;
+	else if (!strcmp(buf, "subject"))
+		atom->u.contents.option = C_SUB;
+	else if (skip_prefix(buf, "lines=", &buf)) {
+		atom->u.contents.option = C_LINES;
+		if (strtoul_ui(buf, 10, &atom->u.contents.nlines))
+			die(_("positive value expected contents:lines=%s"), buf);
+	} else
+		die(_("unrecognized %%(contents) argument: %s"), buf);
+}
+
 static align_type parse_align_position(const char *s)
 {
 	if (!strcmp(s, "right"))
@@ -178,9 +211,9 @@ static struct {
 	{ "taggerdate", FIELD_TIME },
 	{ "creator" },
 	{ "creatordate", FIELD_TIME },
-	{ "subject" },
-	{ "body" },
-	{ "contents" },
+	{ "subject", FIELD_STR, contents_atom_parser },
+	{ "body", FIELD_STR, contents_atom_parser },
+	{ "contents", FIELD_STR, contents_atom_parser },
 	{ "upstream", FIELD_STR, remote_ref_atom_parser },
 	{ "push", FIELD_STR, remote_ref_atom_parser },
 	{ "symref" },
@@ -193,11 +226,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;
@@ -214,7 +242,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 */
@@ -764,19 +791,15 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
 
 	for (i = 0; i < used_atom_cnt; i++) {
 		const char *name = used_atom[i].name;
+		struct used_atom *atom = &used_atom[i];
 		struct atom_value *v = &val[i];
-		const char *valp = NULL;
 		if (!!deref != (*name == '*'))
 			continue;
 		if (deref)
 			name++;
 		if (strcmp(name, "subject") &&
 		    strcmp(name, "body") &&
-		    strcmp(name, "contents") &&
-		    strcmp(name, "contents:subject") &&
-		    strcmp(name, "contents:body") &&
-		    strcmp(name, "contents:signature") &&
-		    !starts_with(name, "contents:lines="))
+		    !starts_with(name, "contents"))
 			continue;
 		if (!subpos)
 			find_subpos(buf, sz,
@@ -784,28 +807,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.6.4

Re: [PATCH v3 14/15] ref-filter: introduce contents_atom_parser()

[Junio C Hamano]

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

> Introduce contents_atom_parser() which will parse the '%(contents)'
> atom and store information into the 'used_atom' structure based on the
> modifiers used along with the atom.
>
> Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
>  ref-filter.c | 74 +++++++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 46 insertions(+), 28 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 32b4674..9e61530 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;
> @@ -96,6 +100,35 @@ static void remote_ref_atom_parser(struct used_atom *atom)
>  		die(_("unrecognized format: %%(%s)"), atom->name);
>  }
>  
> +static void contents_atom_parser(struct used_atom *atom)
> +{
> +	const char * buf;
> +
> +	if (match_atom_name(atom->name, "subject", &buf) && !buf) {
> +		atom->u.contents.option = C_SUB;
> +		return;
> +	} else if (match_atom_name(atom->name, "body", &buf) && !buf) {
> +		atom->u.contents.option = C_BODY_DEP;
> +		return;
> +	} if (!match_atom_name(atom->name, "contents", &buf))
> +		  die("BUG: parsing non-'contents'");

Did you really intend to say "if" here, not "else if"?

I also wonder if the "&& !buf" in the first two are correct.  What
should happen to "%(subject:foo)", which gives you a non-empty buf?
It may or may not be an error, but it should not fall thru and cause
"BUG:parsing non-contents", should it?

> +	if (!buf)
> +		atom->u.contents.option = C_BARE;
> +	else if (!strcmp(buf, "body"))
> +		atom->u.contents.option = C_BODY;
> +...

Re: [PATCH v3 14/15] ref-filter: introduce contents_atom_parser()

[Karthik Nayak]

On Wed, Jan 6, 2016 at 2:52 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Introduce contents_atom_parser() which will parse the '%(contents)'
>> atom and store information into the 'used_atom' structure based on the
>> modifiers used along with the atom.
>>
>> Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
>> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>>  ref-filter.c | 74 +++++++++++++++++++++++++++++++++++++-----------------------
>>  1 file changed, 46 insertions(+), 28 deletions(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 32b4674..9e61530 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;
>> @@ -96,6 +100,35 @@ static void remote_ref_atom_parser(struct used_atom *atom)
>>               die(_("unrecognized format: %%(%s)"), atom->name);
>>  }
>>
>> +static void contents_atom_parser(struct used_atom *atom)
>> +{
>> +     const char * buf;
>> +
>> +     if (match_atom_name(atom->name, "subject", &buf) && !buf) {
>> +             atom->u.contents.option = C_SUB;
>> +             return;
>> +     } else if (match_atom_name(atom->name, "body", &buf) && !buf) {
>> +             atom->u.contents.option = C_BODY_DEP;
>> +             return;
>> +     } if (!match_atom_name(atom->name, "contents", &buf))
>> +               die("BUG: parsing non-'contents'");
>
> Did you really intend to say "if" here, not "else if"?
>

Not that it makes a difference here since both the previous
condition return. I think "else if" would be better.

> I also wonder if the "&& !buf" in the first two are correct.  What
> should happen to "%(subject:foo)", which gives you a non-empty buf?
> It may or may not be an error, but it should not fall thru and cause
> "BUG:parsing non-contents", should it?
>

I think It would be better to add specific messages there

if (match_atom_name(atom->name, "subject", &buf)) {
    if (buf)
        die("%%(subject) atom does not take arguments");
    atom->u.contents.option = C_SUB;
    return;
} else if (match_atom_name(atom->name, "body", &buf)) {
    if (buf)
        die("%%(body) atom does not take arguments");
    atom->u.contents.option = C_BODY_DEP;
    return;
} else if (!match_atom_name(atom->name, "contents", &buf))
     die("BUG: parsing non-'contents'");

-- 
Regards,
Karthik Nayak

Re: [PATCH v3 14/15] ref-filter: introduce contents_atom_parser()

[Junio C Hamano]

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

>>> +static void contents_atom_parser(struct used_atom *atom)
>>> +{
>>> +     const char * buf;

const char *buf;

>>> +
>>> +     if (match_atom_name(atom->name, "subject", &buf) && !buf) {
>>> +             atom->u.contents.option = C_SUB;
>>> +             return;
>>> +     } else if (match_atom_name(atom->name, "body", &buf) && !buf) {
>>> +             atom->u.contents.option = C_BODY_DEP;
>>> +             return;
>>> +     } if (!match_atom_name(atom->name, "contents", &buf))
>>> +               die("BUG: parsing non-'contents'");
>>
>> Did you really intend to say "if" here, not "else if"?
>
> Not that it makes a difference here since both the previous
> condition return. I think "else if" would be better.

I am not sure if it is "Y would be better even though X and Y both
would work".  It looks to me "X and Y behave differently, X is a bug
and Y is correct".

The above would behave differently between "if" and "else if" (and
by the way, the code layout suggests it is "else if"; otherwise you
would be starting "if" on its own line) when you feed "subject:foo",
no?

Re: [PATCH v3 14/15] ref-filter: introduce contents_atom_parser()

[Karthik Nayak]

On Thu, Jan 7, 2016 at 11:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>>>> +static void contents_atom_parser(struct used_atom *atom)
>>>> +{
>>>> +     const char * buf;
>
> const char *buf;
>

will change that.

>>>> +
>>>> +     if (match_atom_name(atom->name, "subject", &buf) && !buf) {
>>>> +             atom->u.contents.option = C_SUB;
>>>> +             return;
>>>> +     } else if (match_atom_name(atom->name, "body", &buf) && !buf) {
>>>> +             atom->u.contents.option = C_BODY_DEP;
>>>> +             return;
>>>> +     } if (!match_atom_name(atom->name, "contents", &buf))
>>>> +               die("BUG: parsing non-'contents'");
>>>
>>> Did you really intend to say "if" here, not "else if"?
>>
>> Not that it makes a difference here since both the previous
>> condition return. I think "else if" would be better.
>
> I am not sure if it is "Y would be better even though X and Y both
> would work".  It looks to me "X and Y behave differently, X is a bug
> and Y is correct".
>
> The above would behave differently between "if" and "else if" (and
> by the way, the code layout suggests it is "else if"; otherwise you
> would be starting "if" on its own line) when you feed "subject:foo",
> no?

It is indeed an "else if". What I was referring to was that since its like

if (cond_a) {
...
return;
} else if (cond_b) {
...
return;
} if (cond_c) {
...
}

cond_c is only checked if cond_a and cond_b don't hold good. Similar to
how 'else if' would work, because cond_a and cond_b return. Sorry for the
confusion.

-- 
Regards,
Karthik Nayak

[PATCH v3 15/15] 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 | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 9e61530..57b4280 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;
@@ -129,6 +130,20 @@ static void contents_atom_parser(struct used_atom *atom)
 		die(_("unrecognized %%(contents) argument: %s"), buf);
 }
 
+static void objectname_atom_parser(struct used_atom *atom)
+{
+	const char * buf;
+
+	if (!match_atom_name(atom->name, "objectname", &buf))
+		die("BUG: parsing non-'objectname'");
+	if (!buf)
+		atom->u.objectname = O_FULL;
+	else if (!strcmp(buf, "short"))
+		atom->u.objectname = O_SHORT;
+	else
+		die(_("unrecognized %%(objectname) argument: %s"), buf);
+}
+
 static align_type parse_align_position(const char *s)
 {
 	if (!strcmp(s, "right"))
@@ -190,7 +205,7 @@ static struct {
 	{ "refname" },
 	{ "objecttype" },
 	{ "objectsize", FIELD_ULONG },
-	{ "objectname" },
+	{ "objectname", FIELD_STR, objectname_atom_parser },
 	{ "tree" },
 	{ "parent" },
 	{ "numparent", FIELD_ULONG },
@@ -467,15 +482,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;
 }
@@ -499,7 +516,7 @@ static void grab_common_values(struct atom_value *val, int deref, struct object
 			v->s = xstrfmt("%lu", sz);
 		}
 		else if (deref)
-			grab_objectname(name, obj->sha1, v);
+			grab_objectname(name, obj->sha1, v, &used_atom[i]);
 	}
 }
 
@@ -1001,7 +1018,7 @@ static void populate_value(struct ref_array_item *ref)
 				v->s = xstrdup(buf + 1);
 			}
 			continue;
-		} else if (!deref && grab_objectname(name, ref->objectname, v)) {
+		} else if (!deref && grab_objectname(name, ref->objectname, v, atom)) {
 			continue;
 		} else if (!strcmp(name, "HEAD")) {
 			const char *head;
-- 
2.6.4

Re: [PATCH v3 15/15] ref-filter: introduce objectname_atom_parser()

[Junio C Hamano]

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

> @@ -467,15 +482,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")) {

The original used to reject "objectnamefoo", but the updated one is
more sloppy.  Shouldn't it be doing the same match_atom_name() here?

This comment applies to many remaining strcmp() and starts_with()
that is reachable from populate_value().

Re: [PATCH v3 15/15] ref-filter: introduce objectname_atom_parser()

[Karthik Nayak]

On Wed, Jan 6, 2016 at 3:01 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> @@ -467,15 +482,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")) {
>
> The original used to reject "objectnamefoo", but the updated one is
> more sloppy.  Shouldn't it be doing the same match_atom_name() here?
>
> This comment applies to many remaining strcmp() and starts_with()
> that is reachable from populate_value().

Should we worry about such extensions of atoms? I see that
parse_ref_filter_atom()
takes care of something like that in the beginning itself

-- 
Regards,
Karthik Nayak

Re: [PATCH v3 00/15] ref-filter: use parsing functions

[Eric Sunshine]

On Tue, Jan 5, 2016 at 3:02 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Eric suggested that I make match_atom_name() not return a value [0]. I
> haven't done that as we use match_atom_name() in [14/15] for matching
> 'subject' and 'body' in contents_atom_parser() and although Eric
> suggested I use strcmp() instead, this would not work as we need to
> check for derefernced 'subject' and 'body' atoms.
> [0]: http://article.gmane.org/gmane.comp.version-control.git/282701

I don't understand the difficulty. It should be easy to manually skip
the 'deref' for this one particular case:

    const char *name = atom->name;
    if (*name == '*')
        name++;

Which would allow this unnecessarily complicated code from patch 14/15:

    if (match_atom_name(atom->name, "subject", &buf) && !buf) {
        ...
        return;
    } else if (match_atom_name(atom->name, "body", &buf) && !buf) {
        ...
        return;
    } if (!match_atom_name(atom->name, "contents", &buf))
        die("BUG: parsing non-'contents'");

to be simplified to the more easily understood form suggested during
review[1] of v2:

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

You could also just use (!strcmp("body") || !strcmp("*body")) rather
than skipping "*" manually, but the repetition makes that a bit
noisier and uglier.

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

Re: [PATCH v3 00/15] ref-filter: use parsing functions

[Karthik Nayak]

On Thu, Jan 7, 2016 at 2:44 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Jan 5, 2016 at 3:02 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Eric suggested that I make match_atom_name() not return a value [0]. I
>> haven't done that as we use match_atom_name() in [14/15] for matching
>> 'subject' and 'body' in contents_atom_parser() and although Eric
>> suggested I use strcmp() instead, this would not work as we need to
>> check for derefernced 'subject' and 'body' atoms.
>> [0]: http://article.gmane.org/gmane.comp.version-control.git/282701
>
> I don't understand the difficulty. It should be easy to manually skip
> the 'deref' for this one particular case:
>
>     const char *name = atom->name;
>     if (*name == '*')
>         name++;
>
> Which would allow this unnecessarily complicated code from patch 14/15:
>
>     if (match_atom_name(atom->name, "subject", &buf) && !buf) {
>         ...
>         return;
>     } else if (match_atom_name(atom->name, "body", &buf) && !buf) {
>         ...
>         return;
>     } if (!match_atom_name(atom->name, "contents", &buf))
>         die("BUG: parsing non-'contents'");
>
> to be simplified to the more easily understood form suggested during
> review[1] of v2:
>
>     if (!strcmp(name, "subject")) {
>         ...
>         return;
>     } else if (!strcmp(name, "body")) {
>         ...
>         return;
>     } else if (!match_atom_name(name,"contents", &buf))
>         die("BUG: expected 'contents' or 'contents:'");
>
> You could also just use (!strcmp("body") || !strcmp("*body")) rather
> than skipping "*" manually, but the repetition makes that a bit
> noisier and uglier.
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/282645

Definitely not a difficulty per se. Just that it seems like something
match_atom_name()
seems to be fit for. As the function name suggests that we're matching
the atom name
and the check for '!buf' indicates that no options are to be included
for that particular atom.

Also after Junio's suggestion[1], I think It looks better now[2]. But
either ways, I'm not
strongly against what you're saying, so my opinion on this matter is
quite flexible.

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

-- 
Regards,
Karthik Nayak

Re: [PATCH v3 00/15] ref-filter: use parsing functions

[Junio C Hamano]

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

>> I don't understand the difficulty. It should be easy to manually skip
>> the 'deref' for this one particular case:
>>
>>     const char *name = atom->name;
>>     if (*name == '*')
>>         name++;
>>
>> Which would allow this unnecessarily complicated code from patch 14/15:
>>
>>     if (match_atom_name(atom->name, "subject", &buf) && !buf) {
>>         ...
>>         return;
>>     } else if (match_atom_name(atom->name, "body", &buf) && !buf) {
>>         ...
>>         return;
>>     } if (!match_atom_name(atom->name, "contents", &buf))
>>         die("BUG: parsing non-'contents'");
>>
>> to be simplified to the more easily understood form suggested during
>> review[1] of v2:
>>
>>     if (!strcmp(name, "subject")) {
>>         ...
>>         return;
>>     } else if (!strcmp(name, "body")) {
>>         ...
>>         return;
>>     } else if (!match_atom_name(name,"contents", &buf))
>>         die("BUG: expected 'contents' or 'contents:'");
>>
>> You could also just use (!strcmp("body") || !strcmp("*body")) rather
>> than skipping "*" manually, but the repetition makes that a bit
>> noisier and uglier.
>>
>> [1]: http://article.gmane.org/gmane.comp.version-control.git/282645
>
> Definitely not a difficulty per se. Just that it seems like something
> match_atom_name()
> seems to be fit for. As the function name suggests that we're matching
> the atom name
> and the check for '!buf' indicates that no options are to be included
> for that particular atom.
>
> Also after Junio's suggestion[1], I think It looks better now[2]. But
> either ways, I'm not
> strongly against what you're saying, so my opinion on this matter is
> quite flexible.
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/283404
> [2]: http://article.gmane.org/gmane.comp.version-control.git/283449

Sorry, but I suspect I was looking at a leaf function without
thinking about the larger picture.

I suspect that the interface to customized parsing function called
by parse_ref_filter_atom() is misdesigned.  I understand that the
overall parsing that starts at verify_ref_format() goes like this:

 * Iterate over the string and find a matching "%(",")" pair.

   - For each such pair found, use parse_ref_filter_atom() on
     what is inside that matching pair.

     - parse_ref_filter_atom() iterates over the table of known
       atoms, and finds the entry in that table.

       Note that at this point, it knows that "%(" is followed by
       'contents' or 'contents:' when it picked the "contents" atom
       from the table, for example.

     - if the entry we found in that table for the atom being parsed
       has a custom parse function, that function is called, but the
       calling convention does not pass the fact that we already
       know what we are seeing inside "%(",")" pair is 'contents',
       for example, and we know what argument it is given if any.

So it appears to me that match_atom_name() is a misguided helper
function that you shouldn't have to use too often.  If the signature
of parse() functions is changed to take not just the atom but the
pointer to its argument (could be NULL, if we are seeing
"%(contents)", for example) that is already available as "formatp"
in the function, then contents_atom_parser() could become more like:

contents_atom_parser(struct used_atom *atom, const char *arg)
{
	if (args)
        	atom->u.contents.option = C_BARE;
	else if (!strcmp(arg, "body"))
        	atom->u.contents.option = C_BODY;
	...
}

and there is no reason for this caller to even look at atom->name or
worry about that it might have the dereferencing asterisk in front.

If we really want to avoid having separate subject_atom_parser() and
body_atom_parser(), they can be folded into the same function and it
becomes necessary to switch on atom->name like you did in the code
being discussed in the quoted part above.  For that, as Eric said,
skipping '*' manually would not be a big deal, as that should not
happen so often in the code _anyway_.  It is not a good idea to
switch on atom->name inside contents_atom_parser() like you did.
You are better off having separate {subject,body}_atom_parser()
functions.

For one thing, you are not reusing or sharing any code by squishing
these three functions into one.  A conceptually larger problem is
that you are adding two extra !strcmp() calls to figure out the
caller _already_ knows (notice I said this is "conceptual", this is
not about performance).  parse_ref_filter_atom() knows that it is a
"%(subject)" or "%(subject:...)" atom, but because you throw away
that information and call contents_atom_parser() by saying that it
is one of the contents, subject or body, the called function has to
redo strcmp in order to figure it out itself.

Re: [PATCH v3 00/15] ref-filter: use parsing functions

[Karthik Nayak]

On Fri, Jan 8, 2016 at 12:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>>> I don't understand the difficulty. It should be easy to manually skip
>>> the 'deref' for this one particular case:
>>>
>>>     const char *name = atom->name;
>>>     if (*name == '*')
>>>         name++;
>>>
>>> Which would allow this unnecessarily complicated code from patch 14/15:
>>>
>>>     if (match_atom_name(atom->name, "subject", &buf) && !buf) {
>>>         ...
>>>         return;
>>>     } else if (match_atom_name(atom->name, "body", &buf) && !buf) {
>>>         ...
>>>         return;
>>>     } if (!match_atom_name(atom->name, "contents", &buf))
>>>         die("BUG: parsing non-'contents'");
>>>
>>> to be simplified to the more easily understood form suggested during
>>> review[1] of v2:
>>>
>>>     if (!strcmp(name, "subject")) {
>>>         ...
>>>         return;
>>>     } else if (!strcmp(name, "body")) {
>>>         ...
>>>         return;
>>>     } else if (!match_atom_name(name,"contents", &buf))
>>>         die("BUG: expected 'contents' or 'contents:'");
>>>
>>> You could also just use (!strcmp("body") || !strcmp("*body")) rather
>>> than skipping "*" manually, but the repetition makes that a bit
>>> noisier and uglier.
>>>
>>> [1]: http://article.gmane.org/gmane.comp.version-control.git/282645
>>
>> Definitely not a difficulty per se. Just that it seems like something
>> match_atom_name()
>> seems to be fit for. As the function name suggests that we're matching
>> the atom name
>> and the check for '!buf' indicates that no options are to be included
>> for that particular atom.
>>
>> Also after Junio's suggestion[1], I think It looks better now[2]. But
>> either ways, I'm not
>> strongly against what you're saying, so my opinion on this matter is
>> quite flexible.
>>
>> [1]: http://article.gmane.org/gmane.comp.version-control.git/283404
>> [2]: http://article.gmane.org/gmane.comp.version-control.git/283449
>
> Sorry, but I suspect I was looking at a leaf function without
> thinking about the larger picture.
>
> I suspect that the interface to customized parsing function called
> by parse_ref_filter_atom() is misdesigned.  I understand that the
> overall parsing that starts at verify_ref_format() goes like this:
>
>  * Iterate over the string and find a matching "%(",")" pair.
>
>    - For each such pair found, use parse_ref_filter_atom() on
>      what is inside that matching pair.
>
>      - parse_ref_filter_atom() iterates over the table of known
>        atoms, and finds the entry in that table.
>
>        Note that at this point, it knows that "%(" is followed by
>        'contents' or 'contents:' when it picked the "contents" atom
>        from the table, for example.
>
>      - if the entry we found in that table for the atom being parsed
>        has a custom parse function, that function is called, but the
>        calling convention does not pass the fact that we already
>        know what we are seeing inside "%(",")" pair is 'contents',
>        for example, and we know what argument it is given if any.
>

Absolutely correct.

> So it appears to me that match_atom_name() is a misguided helper
> function that you shouldn't have to use too often.  If the signature
> of parse() functions is changed to take not just the atom but the
> pointer to its argument (could be NULL, if we are seeing
> "%(contents)", for example) that is already available as "formatp"
> in the function, then contents_atom_parser() could become more like:
>

I see, I think this does make sense, since we have extracted any arguments
following ':' already in parse_ref_filter_atom() it only makes sense to use it
without doing the same type of work again.

> contents_atom_parser(struct used_atom *atom, const char *arg)
> {
>         if (args)
>                 atom->u.contents.option = C_BARE;
>         else if (!strcmp(arg, "body"))
>                 atom->u.contents.option = C_BODY;
>         ...
> }
>
> and there is no reason for this caller to even look at atom->name or
> worry about that it might have the dereferencing asterisk in front.
>

This looks good.

> If we really want to avoid having separate subject_atom_parser() and
> body_atom_parser(), they can be folded into the same function and it
> becomes necessary to switch on atom->name like you did in the code
> being discussed in the quoted part above.  For that, as Eric said,
> skipping '*' manually would not be a big deal, as that should not
> happen so often in the code _anyway_.  It is not a good idea to
> switch on atom->name inside contents_atom_parser() like you did.
> You are better off having separate {subject,body}_atom_parser()
> functions.
>
> For one thing, you are not reusing or sharing any code by squishing
> these three functions into one.  A conceptually larger problem is
> that you are adding two extra !strcmp() calls to figure out the
> caller _already_ knows (notice I said this is "conceptual", this is
> not about performance).  parse_ref_filter_atom() knows that it is a
> "%(subject)" or "%(subject:...)" atom, but because you throw away
> that information and call contents_atom_parser() by saying that it
> is one of the contents, subject or body, the called function has to
> redo strcmp in order to figure it out itself.

I understand what you're saying and keeping these functions separate
seems like the way to go. Also this might also remove all uses of
match_atom_name() from ref-filter.c. This Idea seems good to me,
thanks for your suggestions, I will work on it.

-- 
Regards,
Karthik Nayak

Re: [PATCH v3 00/15] ref-filter: use parsing functions

[Eric Sunshine]

On Thu, Jan 7, 2016 at 1:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
> If we really want to avoid having separate subject_atom_parser() and
> body_atom_parser(), they can be folded into the same function and it
> becomes necessary to switch on atom->name like you did in the code
> being discussed in the quoted part above.  For that, as Eric said,
> skipping '*' manually would not be a big deal, as that should not
> happen so often in the code _anyway_.  It is not a good idea to
> switch on atom->name inside contents_atom_parser() like you did.
> You are better off having separate {subject,body}_atom_parser()
> functions.
>
> For one thing, you are not reusing or sharing any code by squishing
> these three functions into one.  A conceptually larger problem is
> that you are adding two extra !strcmp() calls to figure out the
> caller _already_ knows (notice I said this is "conceptual", this is
> not about performance).  parse_ref_filter_atom() knows that it is a
> "%(subject)" or "%(subject:...)" atom, but because you throw away
> that information and call contents_atom_parser() by saying that it
> is one of the contents, subject or body, the called function has to
> redo strcmp in order to figure it out itself.

Thanks for spelling this all out. I had come to the same conclusion
yesterday after posting my message but never got back to the computer
long enough to compose a proper email explaining the issue. I fully
agree that this would be a better design.

Re: [PATCH v3 00/15] ref-filter: use parsing functions

[Karthik Nayak]

On Fri, Jan 8, 2016 at 12:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> So it appears to me that match_atom_name() is a misguided helper
> function that you shouldn't have to use too often.  If the signature
> of parse() functions is changed to take not just the atom but the
> pointer to its argument (could be NULL, if we are seeing
> "%(contents)", for example) that is already available as "formatp"
> in the function, then contents_atom_parser() could become more like:
>
> contents_atom_parser(struct used_atom *atom, const char *arg)
> {
>         if (args)
>                 atom->u.contents.option = C_BARE;
>         else if (!strcmp(arg, "body"))
>                 atom->u.contents.option = C_BODY;
>         ...
> }
>
> and there is no reason for this caller to even look at atom->name or
> worry about that it might have the dereferencing asterisk in front.
>

So we something like this for the parsing function:

 int parse_ref_filter_atom(const char *atom, const char *ep)
 {
        const char *sp;
+       char *arg;
        int i, at;

        sp = atom;
@@ -141,6 +143,7 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
                const char *formatp = strchr(sp, ':');
                if (!formatp || ep < formatp)
                        formatp = ep;
+               arg = (char *)formatp;
                if (len == formatp - sp && !memcmp(valid_atom[i].name, sp, len))
                        break;
        }
@@ -154,6 +157,13 @@ 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 != ep)
+               arg = xstrndup(arg + 1, ep - arg - 1);
+       else
+               arg = NULL;
+       if (valid_atom[i].parser)
+               valid_atom[i].parser(&used_atom[at], arg);
+       free(arg);
        if (*atom == '*')
                need_tagged = 1;
        if (!strcmp(used_atom[at].name, "symref"))

-- 
Regards,
Karthik Nayak

Re: [PATCH v3 00/15] ref-filter: use parsing functions

[Junio C Hamano]

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

> So we something like this for the parsing function:
>
>  int parse_ref_filter_atom(const char *atom, const char *ep)
>  {
>         const char *sp;
> +       char *arg;

I think this and the new parameter to .parser() function should be
"const char *".

> @@ -141,6 +143,7 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
>                 const char *formatp = strchr(sp, ':');
>                 if (!formatp || ep < formatp)
>                         formatp = ep;
> +               arg = (char *)formatp;
>                 if (len == formatp - sp && !memcmp(valid_atom[i].name, sp, len))
>                         break;

And this part can just use arg without formatp.  The original is a
bit sloppy and will keep looking for a colon past ep, but we already
know between sp and ep there is no NUL, so we could do this:

		arg = memchr(sp, ':', ep - sp);
		if ((!arg || len == arg - sp) &&
		    !memcmp(valid_atom[i].name, sp, len))
			break;

> @@ -154,6 +157,13 @@ 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 != ep)
> +               arg = xstrndup(arg + 1, ep - arg - 1);
> +       else
> +               arg = NULL;

Why even copy?  The original that used match_atom_name() borrowed
part of existing string via (const char **val), so you know whatever
used that &buf you grabbed out of match_atom_name() should only be
reading the values not writing into the memory, no?

That is why I think arg should be "const char *".

As the above memchr() alrady took care of "we didn't find a colon"
case, we only need to do this here, I think:

	if (arg)
        	arg = used_atom[at].name + (arg - atom);

and without later free().

Alternatively, we could add an int field to elements of used_atom[]
array that says what byte-offset in the used_atom[].name the atom
arguments start (if any).  Then .parser() does not have to take the
parameter separately [*1*].

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



[Footnote]

*1* Thinking about it more, perhaps used_atom[].type should be
removed and instead used_atom[].atom should be a pointer into the
valid_atom[] array.  Then any reference to used_atom[].type will
become used_atom[].atom->cmp_type, which is much nicer for two
reasons: (1) one less useless copy (2) one less field that has a
name "type" that is overly generic.

That does not remove the need for recording where the atom argument
is, though, in used_atom[].  We could add a bit "has_deref" to
used_atom[] and then do something like this:

    arg = used_atom[i].name + used_atom[i].atom->namelen +
          used_atom[i].has_deref;

but I do not think we want to go there.  It would hardcode the
knowledge that used_atom[i].name is either used_atom[i].atom->name
or one asterisk prefixed to it, making future extension of the
syntax even harder.

Re: [PATCH v3 00/15] ref-filter: use parsing functions

[Karthik Nayak]

On Fri, Jan 8, 2016 at 2:58 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> So we something like this for the parsing function:
>>
>>  int parse_ref_filter_atom(const char *atom, const char *ep)
>>  {
>>         const char *sp;
>> +       char *arg;
>
> I think this and the new parameter to .parser() function should be
> "const char *".
>

Yes. Will do that.

>> @@ -141,6 +143,7 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
>>                 const char *formatp = strchr(sp, ':');
>>                 if (!formatp || ep < formatp)
>>                         formatp = ep;
>> +               arg = (char *)formatp;
>>                 if (len == formatp - sp && !memcmp(valid_atom[i].name, sp, len))
>>                         break;
>
> And this part can just use arg without formatp.  The original is a
> bit sloppy and will keep looking for a colon past ep, but we already
> know between sp and ep there is no NUL, so we could do this:
>
>                 arg = memchr(sp, ':', ep - sp);
>                 if ((!arg || len == arg - sp) &&
>                     !memcmp(valid_atom[i].name, sp, len))
>                         break;
>

This even removes the need for formatp. Thanks

>> @@ -154,6 +157,13 @@ 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 != ep)
>> +               arg = xstrndup(arg + 1, ep - arg - 1);
>> +       else
>> +               arg = NULL;
>
> Why even copy?  The original that used match_atom_name() borrowed
> part of existing string via (const char **val), so you know whatever
> used that &buf you grabbed out of match_atom_name() should only be
> reading the values not writing into the memory, no?
>

I totally missed that we could use the existing used_atom[at].name
to get the argument. Thanks

> That is why I think arg should be "const char *".
>
> As the above memchr() alrady took care of "we didn't find a colon"
> case, we only need to do this here, I think:
>
>         if (arg)
>                 arg = used_atom[at].name + (arg - atom);
>
> and without later free().
>
          if (arg)
                  arg = used_atom[at].name + (arg - atom) + 1;

'+ 1' to skip over the colon perhaps?

> Alternatively, we could add an int field to elements of used_atom[]
> array that says what byte-offset in the used_atom[].name the atom
> arguments start (if any).  Then .parser() does not have to take the
> parameter separately [*1*].
>
> [Footnote]
>
> *1* Thinking about it more, perhaps used_atom[].type should be
> removed and instead used_atom[].atom should be a pointer into the
> valid_atom[] array.  Then any reference to used_atom[].type will
> become used_atom[].atom->cmp_type, which is much nicer for two
> reasons: (1) one less useless copy (2) one less field that has a
> name "type" that is overly generic.
>
> That does not remove the need for recording where the atom argument
> is, though, in used_atom[].  We could add a bit "has_deref" to
> used_atom[] and then do something like this:
>
>     arg = used_atom[i].name + used_atom[i].atom->namelen +
>           used_atom[i].has_deref;
>
> but I do not think we want to go there.  It would hardcode the
> knowledge that used_atom[i].name is either used_atom[i].atom->name
> or one asterisk prefixed to it, making future extension of the
> syntax even harder.

Makes sense, to sum it up, we'll provide arguments to the parser function:

        void (*parser)(struct used_atom *atom, const char *arg);

So that we provide the arguments to the functions and ensure no need of parsing
atom->name.

Also ensure that we have separate parser functions for 'body',
'subject' and 'contents'.

-- 
Regards,
Karthik Nayak