[PATCH v11 05/13] ref-filter: implement an `align` atom

[PATCH v11 05/13] ref-filter: implement an `align` atom

[Karthik Nayak]

Implement an `align` atom which left-, middle-, or right-aligns the
content between %(align:..) and %(end).

It is followed by `:<width>,<position>`, where the `<position>` is
either left, right or middle and `<width>` is the size of the area
into which the content will be placed. If the content between
%(align:) and %(end) is more than the width then no alignment is
performed. e.g. to align a refname atom to the middle with a total
width of 40 we can do: --format="%(align:middle,40)%(refname)%(end)".

This is done by calling the strbuf_utf8_align() function in utf8.c.

Add documentation and tests for the same.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-for-each-ref.txt |  8 ++++
 ref-filter.c                       | 81 +++++++++++++++++++++++++++++++++++---
 ref-filter.h                       |  1 +
 t/t6302-for-each-ref-filter.sh     | 48 ++++++++++++++++++++++
 4 files changed, 133 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index e49d578..2b82334 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -127,6 +127,14 @@ color::
 	Change output color.  Followed by `:<colorname>`, where names
 	are described in `color.branch.*`.
 
+align::
+	left-, middle-, or right-align the content between %(align:..)
+	and %(end). Followed by `:<position>,<width>`, where the
+	`<position>` is either left, right or middle 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.
+
 In addition to the above, for commit and tag objects, the header
 field names (`tree`, `parent`, `object`, `type`, and `tag`) can
 be used to specify the value in the header field.
diff --git a/ref-filter.c b/ref-filter.c
index 3259363..eac99d0 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -10,6 +10,7 @@
 #include "quote.h"
 #include "ref-filter.h"
 #include "revision.h"
+#include "utf8.h"
 
 typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 
@@ -53,16 +54,27 @@ static struct {
 	{ "flag" },
 	{ "HEAD" },
 	{ "color" },
+	{ "align" },
+	{ "end" },
+};
+
+struct align {
+	align_type position;
+	unsigned int width;
 };
 
 struct ref_formatting_state {
 	struct strbuf output;
 	struct ref_formatting_state *prev;
+	void (*attend)(struct ref_formatting_state *state);
+	void *cb_data;
 	int quote_style;
 };
 
 struct atom_value {
 	const char *s;
+	struct align *align;
+	void (*handler)(struct atom_value *atomv, struct ref_formatting_state **state);
 	unsigned long ul; /* used for sorting when not FIELD_STR */
 };
 
@@ -137,12 +149,12 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
 
 static struct ref_formatting_state *push_new_state(struct ref_formatting_state **state)
 {
-	struct ref_formatting_state *new_state = xcalloc(1, sizeof(struct ref_formatting_state));
-	struct ref_formatting_state *tmp = *state;
+	struct ref_formatting_state *new = xcalloc(1, sizeof(struct ref_formatting_state));
+	struct ref_formatting_state *old = *state;
 
-	*state = new_state;
-	new_state->prev = tmp;
-	return new_state;
+	*state = new;
+	new->prev = old;
+	return new;
 }
 
 static void pop_state(struct ref_formatting_state **state)
@@ -625,6 +637,34 @@ static inline char *copy_advance(char *dst, const char *src)
 	return dst;
 }
 
+static void align_handler(struct ref_formatting_state *state)
+{
+	struct strbuf aligned = STRBUF_INIT;
+	struct ref_formatting_state *return_to = state->prev;
+	struct align *align = (struct align *)state->cb_data;
+
+	strbuf_utf8_align(&aligned, align->position, align->width, state->output.buf);
+	strbuf_addbuf(&return_to->output, &aligned);
+	strbuf_release(&aligned);
+}
+
+static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_state **state)
+{
+	struct ref_formatting_state *new = push_new_state(state);
+	strbuf_init(&new->output, 0);
+	new->attend = align_handler;
+	new->cb_data = atomv->align;
+}
+
+static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state **state)
+{
+	struct ref_formatting_state *current = *state;
+	if (!current->attend)
+		die(_("format: `end` atom used without a supporting atom"));
+	current->attend(current);
+	pop_state(state);
+}
+
 /*
  * Parse the object referred by ref, and grab needed value.
  */
@@ -653,6 +693,7 @@ 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;
 
 		if (*name == '*') {
@@ -718,6 +759,34 @@ static void populate_value(struct ref_array_item *ref)
 			else
 				v->s = " ";
 			continue;
+		} else if (skip_prefix(name, "align:", &valp)) {
+			struct align *align = xmalloc(sizeof(struct align));
+			char *ep = strchr(valp, ',');
+
+			if (ep)
+				*ep = '\0';
+
+			if (strtoul_ui(valp, 10, &align->width))
+				die(_("positive width expected align:%s"), valp);
+
+			if (!ep || starts_with(ep + 1, "left"))
+				align->position = ALIGN_LEFT;
+			else if (starts_with(ep + 1, "right"))
+				align->position = ALIGN_RIGHT;
+			else if (starts_with(ep + 1, "middle"))
+				align->position = ALIGN_MIDDLE;
+			else
+				die(_("improper format entered align:%s"), ep + 1);
+
+			if (ep)
+				*ep = ',';
+
+			v->align = align;
+			v->handler = align_atom_handler;
+			continue;
+		} else if (!strcmp(name, "end")) {
+			v->handler = end_atom_handler;
+			continue;
 		} else
 			continue;
 
@@ -1296,6 +1365,8 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
 		if (cp < sp)
 			append_literal(cp, sp, state);
 		get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
+		if (atomv->handler)
+			atomv->handler(atomv, &state);
 		append_atom(atomv, state);
 	}
 	if (*cp) {
diff --git a/ref-filter.h b/ref-filter.h
index 45026d0..144a633 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -5,6 +5,7 @@
 #include "refs.h"
 #include "commit.h"
 #include "parse-options.h"
+#include "utf8.h"
 
 /* Quoting styles */
 #define QUOTE_NONE 0
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 505a360..b252a50 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -81,4 +81,52 @@ test_expect_success 'filtering with --contains' '
 	test_cmp expect actual
 '
 
+test_expect_success 'left 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
+	git for-each-ref --format="%(align:30,left)refname is %(refname)%(end)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'middle 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
+	git for-each-ref --format="|%(align:30,middle)refname is %(refname)%(end)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
+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
+	git for-each-ref --format="|%(align:30,right)refname is %(refname)%(end)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.5.0

Re: [PATCH v11 05/13] ref-filter: implement an `align` atom

[Karthik Nayak]

I think we need to squash this in

diff --git a/Documentation/git-for-each-ref.txt
b/Documentation/git-for-each-ref.txt
index 3099631..17bd15e 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -129,7 +129,7 @@ color::

 align::
        left-, middle-, or right-align the content between %(align:..)
-       and %(end). Followed by `:<position>,<width>`, where the
+       and %(end). Followed by `:<width>>,<position>`, where the
        `<position>` is either left, right or middle and `<width>` is
        the total length of the content with alignment. If the
        contents length is more than the width then no alignment is

Re: [PATCH v11 05/13] ref-filter: implement an `align` atom

[Andreas Schwab]

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

> I think we need to squash this in
>
> diff --git a/Documentation/git-for-each-ref.txt
> b/Documentation/git-for-each-ref.txt
> index 3099631..17bd15e 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -129,7 +129,7 @@ color::
>
>  align::
>         left-, middle-, or right-align the content between %(align:..)
> -       and %(end). Followed by `:<position>,<width>`, where the
> +       and %(end). Followed by `:<width>>,<position>`, where the

s/>>/>/

Andreas.

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

Re: [PATCH v11 05/13] ref-filter: implement an `align` atom

[Eric Sunshine]

On Sun, Aug 16, 2015 at 8:04 AM, Andreas Schwab <schwab@linux-m68k.org> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>> I think we need to squash this in
>>
>> diff --git a/Documentation/git-for-each-ref.txt
>> b/Documentation/git-for-each-ref.txt
>> index 3099631..17bd15e 100644
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -129,7 +129,7 @@ color::
>>
>>  align::
>>         left-, middle-, or right-align the content between %(align:..)
>> -       and %(end). Followed by `:<position>,<width>`, where the
>> +       and %(end). Followed by `:<width>>,<position>`, where the
>
> s/>>/>/

Also: s/left-/Left-/

Re: [PATCH v11 05/13] ref-filter: implement an `align` atom

[Eric Sunshine]

On Sat, Aug 15, 2015 at 2:00 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Implement an `align` atom which left-, middle-, or right-aligns the
> content between %(align:..) and %(end).
>
> It is followed by `:<width>,<position>`, where the `<position>` is
> either left, right or middle and `<width>` is the size of the area
> into which the content will be placed. If the content between
> %(align:) and %(end) is more than the width then no alignment is
> performed. e.g. to align a refname atom to the middle with a total
> width of 40 we can do: --format="%(align:middle,40)%(refname)%(end)".
>
> This is done by calling the strbuf_utf8_align() function in utf8.c.
>
> Add documentation and tests for the same.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> index 3259363..eac99d0 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -10,6 +10,7 @@
>  #include "quote.h"
>  #include "ref-filter.h"
>  #include "revision.h"
> +#include "utf8.h"
>
>  typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
>
> @@ -53,16 +54,27 @@ static struct {
>         { "flag" },
>         { "HEAD" },
>         { "color" },
> +       { "align" },
> +       { "end" },
> +};
> +
> +struct align {
> +       align_type position;
> +       unsigned int width;
>  };
>
>  struct ref_formatting_state {
>         struct strbuf output;
>         struct ref_formatting_state *prev;
> +       void (*attend)(struct ref_formatting_state *state);

Junio's suggestion for this member was "at end"; that is what to do
when you are "at"-the-%(end), not "attend", which isn't meaningful
here. You could also call it 'end_scope', 'finish' or 'close' or
'finalize' or something.

> +       void *cb_data;
>         int quote_style;
>  };
>
>  struct atom_value {
>         const char *s;
> +       struct align *align;
> +       void (*handler)(struct atom_value *atomv, struct ref_formatting_state **state);
>         unsigned long ul; /* used for sorting when not FIELD_STR */
>  };
>
> @@ -137,12 +149,12 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
>
>  static struct ref_formatting_state *push_new_state(struct ref_formatting_state **state)
>  {
> -       struct ref_formatting_state *new_state = xcalloc(1, sizeof(struct ref_formatting_state));
> -       struct ref_formatting_state *tmp = *state;
> +       struct ref_formatting_state *new = xcalloc(1, sizeof(struct ref_formatting_state));
> +       struct ref_formatting_state *old = *state;
>
> -       *state = new_state;
> -       new_state->prev = tmp;
> -       return new_state;
> +       *state = new;
> +       new->prev = old;
> +       return new;
>  }

What are these changes about? They appear only to be renaming some
variables which were introduced in patch 3. It would make more sense
to give them the desired names in the patch which introduces them.

>  static void pop_state(struct ref_formatting_state **state)
> @@ -625,6 +637,34 @@ static inline char *copy_advance(char *dst, const char *src)
>         return dst;
>  }
>
> +static void align_handler(struct ref_formatting_state *state)

The names 'align_handler' and 'align_atom_handler' are confusingly
similar. Perhaps name this end_align() or do_align() or
apply_alignment() or something?

> +{
> +       struct strbuf aligned = STRBUF_INIT;
> +       struct ref_formatting_state *return_to = state->prev;
> +       struct align *align = (struct align *)state->cb_data;
> +
> +       strbuf_utf8_align(&aligned, align->position, align->width, state->output.buf);
> +       strbuf_addbuf(&return_to->output, &aligned);

A couple comments:

First, why is 'strbuf aligned' needed? Can't you instead just invoke
strbuf_utf8_align(&return_to->output, ...)?

Second, I realize that Junio suggested the 'return_to' idea, but it
seems like it could become overly painful since each handler of this
sort is going to have to perform the same manipulation to append its
collected output to its parent state's output. What if you instead
make it the responsibility of pop_state() to append the 'output' from
the state being popped to the "prev" state's 'output'? This way, it
happens automatically, thus reducing code in each individual handler,
and reducing the burden of having to keep writing the same code.

> +       strbuf_release(&aligned);
> +}
> +
> +static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_state **state)
> +{
> +       struct ref_formatting_state *new = push_new_state(state);
> +       strbuf_init(&new->output, 0);

I think this strbuf_init() should be the responsibility of
push_new_state(), as mentioned in my patch 3 review, otherwise every
caller of push_new_state() will have to remember to do this.

> +       new->attend = align_handler;
> +       new->cb_data = atomv->align;
> +}
> +
> +static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state **state)
> +{
> +       struct ref_formatting_state *current = *state;
> +       if (!current->attend)
> +               die(_("format: `end` atom used without a supporting atom"));
> +       current->attend(current);
> +       pop_state(state);
> +}
> +
>  /*
>   * Parse the object referred by ref, and grab needed value.
>   */
> @@ -653,6 +693,7 @@ 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;
>
>                 if (*name == '*') {
> @@ -718,6 +759,34 @@ static void populate_value(struct ref_array_item *ref)
>                         else
>                                 v->s = " ";
>                         continue;
> +               } else if (skip_prefix(name, "align:", &valp)) {
> +                       struct align *align = xmalloc(sizeof(struct align));

Who is responsible for freeing this memory?

> +                       char *ep = strchr(valp, ',');
> +
> +                       if (ep)
> +                               *ep = '\0';
> +
> +                       if (strtoul_ui(valp, 10, &align->width))
> +                               die(_("positive width expected align:%s"), valp);
> +
> +                       if (!ep || starts_with(ep + 1, "left"))
> +                               align->position = ALIGN_LEFT;
> +                       else if (starts_with(ep + 1, "right"))
> +                               align->position = ALIGN_RIGHT;
> +                       else if (starts_with(ep + 1, "middle"))
> +                               align->position = ALIGN_MIDDLE;

Shouldn't these be strcmp() rather than starts_with()? You don't want
to match "leftfoot" as "left".

> +                       else
> +                               die(_("improper format entered align:%s"), ep + 1);
> +
> +                       if (ep)
> +                               *ep = ',';

What's this conditional about? Why restore the comma?

> +
> +                       v->align = align;
> +                       v->handler = align_atom_handler;

Junio's proposal for using handlers[1] is a pretty big change which
would generalize atom processing overall, and which you haven't
implemented here. Instead, your use of handlers is just to avoid
having special-case 'align' and 'end' conditionals spread throughout
several functions. Am I understanding that correctly?

Is the idea to leave that larger change for a later date (and possibly
a different programmer)?

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

> +                       continue;
> +               } else if (!strcmp(name, "end")) {
> +                       v->handler = end_atom_handler;
> +                       continue;
>                 } else
>                         continue;
>
> @@ -1296,6 +1365,8 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
>                 if (cp < sp)
>                         append_literal(cp, sp, state);
>                 get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
> +               if (atomv->handler)
> +                       atomv->handler(atomv, &state);
>                 append_atom(atomv, state);
>         }
>         if (*cp) {
> diff --git a/ref-filter.h b/ref-filter.h
> index 45026d0..144a633 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -5,6 +5,7 @@
>  #include "refs.h"
>  #include "commit.h"
>  #include "parse-options.h"
> +#include "utf8.h"

Why does this need to be #included here?

>  /* Quoting styles */
>  #define QUOTE_NONE 0

Re: [PATCH v11 05/13] ref-filter: implement an `align` atom

[Karthik Nayak]

On Mon, Aug 17, 2015 at 7:37 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sat, Aug 15, 2015 at 2:00 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Implement an `align` atom which left-, middle-, or right-aligns the
>> content between %(align:..) and %(end).
>>
>> It is followed by `:<width>,<position>`, where the `<position>` is
>> either left, right or middle and `<width>` is the size of the area
>> into which the content will be placed. If the content between
>> %(align:) and %(end) is more than the width then no alignment is
>> performed. e.g. to align a refname atom to the middle with a total
>> width of 40 we can do: --format="%(align:middle,40)%(refname)%(end)".
>>
>> This is done by calling the strbuf_utf8_align() function in utf8.c.
>>
>> Add documentation and tests for the same.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 3259363..eac99d0 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -10,6 +10,7 @@
>>  #include "quote.h"
>>  #include "ref-filter.h"
>>  #include "revision.h"
>> +#include "utf8.h"
>>
>>  typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
>>
>> @@ -53,16 +54,27 @@ static struct {
>>         { "flag" },
>>         { "HEAD" },
>>         { "color" },
>> +       { "align" },
>> +       { "end" },
>> +};
>> +
>> +struct align {
>> +       align_type position;
>> +       unsigned int width;
>>  };
>>
>>  struct ref_formatting_state {
>>         struct strbuf output;
>>         struct ref_formatting_state *prev;
>> +       void (*attend)(struct ref_formatting_state *state);
>
> Junio's suggestion for this member was "at end"; that is what to do
> when you are "at"-the-%(end), not "attend", which isn't meaningful
> here. You could also call it 'end_scope', 'finish' or 'close' or
> 'finalize' or something.
>

Weirdly, attend made sense to me, its a function you attend to at the end
types. probably "at_end" would be better.

>> +       void *cb_data;
>>         int quote_style;
>>  };
>>
>>  struct atom_value {
>>         const char *s;
>> +       struct align *align;
>> +       void (*handler)(struct atom_value *atomv, struct ref_formatting_state **state);
>>         unsigned long ul; /* used for sorting when not FIELD_STR */
>>  };
>>
>> @@ -137,12 +149,12 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
>>
>>  static struct ref_formatting_state *push_new_state(struct ref_formatting_state **state)
>>  {
>> -       struct ref_formatting_state *new_state = xcalloc(1, sizeof(struct ref_formatting_state));
>> -       struct ref_formatting_state *tmp = *state;
>> +       struct ref_formatting_state *new = xcalloc(1, sizeof(struct ref_formatting_state));
>> +       struct ref_formatting_state *old = *state;
>>
>> -       *state = new_state;
>> -       new_state->prev = tmp;
>> -       return new_state;
>> +       *state = new;
>> +       new->prev = old;
>> +       return new;
>>  }
>
> What are these changes about? They appear only to be renaming some
> variables which were introduced in patch 3. It would make more sense
> to give them the desired names in the patch which introduces them.
>

Agreed, will scrap them off.

>>  static void pop_state(struct ref_formatting_state **state)
>> @@ -625,6 +637,34 @@ static inline char *copy_advance(char *dst, const char *src)
>>         return dst;
>>  }
>>
>> +static void align_handler(struct ref_formatting_state *state)
>
> The names 'align_handler' and 'align_atom_handler' are confusingly
> similar. Perhaps name this end_align() or do_align() or
> apply_alignment() or something?
>
>> +{
>> +       struct strbuf aligned = STRBUF_INIT;
>> +       struct ref_formatting_state *return_to = state->prev;
>> +       struct align *align = (struct align *)state->cb_data;
>> +
>> +       strbuf_utf8_align(&aligned, align->position, align->width, state->output.buf);
>> +       strbuf_addbuf(&return_to->output, &aligned);
>
> A couple comments:
>
> First, why is 'strbuf aligned' needed? Can't you instead just invoke
> strbuf_utf8_align(&return_to->output, ...)?

Yeah, will remove it.

>
> Second, I realize that Junio suggested the 'return_to' idea, but it
> seems like it could become overly painful since each handler of this
> sort is going to have to perform the same manipulation to append its
> collected output to its parent state's output. What if you instead
> make it the responsibility of pop_state() to append the 'output' from
> the state being popped to the "prev" state's 'output'? This way, it
> happens automatically, thus reducing code in each individual handler,
> and reducing the burden of having to keep writing the same code.
>

Good question, what if we don't want to append to strbuf at all?
For e.g., We were discussing an "%(if).....%(then)......%(end)"
atom structure, here if the if condition isn't met we wouldn't want to
append to the prev strbuf, hence I thought it's better if the handler
decided whether or not to append to prev strbuf.

>> +       strbuf_release(&aligned);
>> +}
>> +
>> +static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_state **state)
>> +{
>> +       struct ref_formatting_state *new = push_new_state(state);
>> +       strbuf_init(&new->output, 0);
>
> I think this strbuf_init() should be the responsibility of
> push_new_state(), as mentioned in my patch 3 review, otherwise every
> caller of push_new_state() will have to remember to do this.
>

As mentioned in your previous review, this will be changed.

>> +       new->attend = align_handler;
>> +       new->cb_data = atomv->align;
>> +}
>> +
>> +static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state **state)
>> +{
>> +       struct ref_formatting_state *current = *state;
>> +       if (!current->attend)
>> +               die(_("format: `end` atom used without a supporting atom"));
>> +       current->attend(current);
>> +       pop_state(state);
>> +}
>> +
>>  /*
>>   * Parse the object referred by ref, and grab needed value.
>>   */
>> @@ -653,6 +693,7 @@ 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;
>>
>>                 if (*name == '*') {
>> @@ -718,6 +759,34 @@ static void populate_value(struct ref_array_item *ref)
>>                         else
>>                                 v->s = " ";
>>                         continue;
>> +               } else if (skip_prefix(name, "align:", &valp)) {
>> +                       struct align *align = xmalloc(sizeof(struct align));
>
> Who is responsible for freeing this memory?
>

Should have been done in align_handler().

>> +                       char *ep = strchr(valp, ',');
>> +
>> +                       if (ep)
>> +                               *ep = '\0';
>> +
>> +                       if (strtoul_ui(valp, 10, &align->width))
>> +                               die(_("positive width expected align:%s"), valp);
>> +
>> +                       if (!ep || starts_with(ep + 1, "left"))
>> +                               align->position = ALIGN_LEFT;
>> +                       else if (starts_with(ep + 1, "right"))
>> +                               align->position = ALIGN_RIGHT;
>> +                       else if (starts_with(ep + 1, "middle"))
>> +                               align->position = ALIGN_MIDDLE;
>
> Shouldn't these be strcmp() rather than starts_with()? You don't want
> to match "leftfoot" as "left".
>

Makes sense.

>> +                       else
>> +                               die(_("improper format entered align:%s"), ep + 1);
>> +
>> +                       if (ep)
>> +                               *ep = ',';
>
> What's this conditional about? Why restore the comma?

because we go through the atoms, once when we verify the format
using verify_format() and again when we call show_ref_array_item()
and this if the atom is changed this would cause a segfault in the second
pho

>
>> +
>> +                       v->align = align;
>> +                       v->handler = align_atom_handler;
>
> Junio's proposal for using handlers[1] is a pretty big change which
> would generalize atom processing overall, and which you haven't
> implemented here. Instead, your use of handlers is just to avoid
> having special-case 'align' and 'end' conditionals spread throughout
> several functions. Am I understanding that correctly?
>
> Is the idea to leave that larger change for a later date (and possibly
> a different programmer)?
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/275710
>

If you're referring to how he suggested everything could just use the
handler and
work on that, yes, I'm leaving that out for the moment, probably me or
someone else
could work on that after this.

>> +                       continue;
>> +               } else if (!strcmp(name, "end")) {
>> +                       v->handler = end_atom_handler;
>> +                       continue;
>>                 } else
>>                         continue;
>>
>> @@ -1296,6 +1365,8 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
>>                 if (cp < sp)
>>                         append_literal(cp, sp, state);
>>                 get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
>> +               if (atomv->handler)
>> +                       atomv->handler(atomv, &state);
>>                 append_atom(atomv, state);
>>         }
>>         if (*cp) {
>> diff --git a/ref-filter.h b/ref-filter.h
>> index 45026d0..144a633 100644
>> --- a/ref-filter.h
>> +++ b/ref-filter.h
>> @@ -5,6 +5,7 @@
>>  #include "refs.h"
>>  #include "commit.h"
>>  #include "parse-options.h"
>> +#include "utf8.h"
>
> Why does this need to be #included here?
>

I'll remove that.

-- 
Regards,
Karthik Nayak

Re: [PATCH v11 05/13] ref-filter: implement an `align` atom

[Eric Sunshine]

On Mon, Aug 17, 2015 at 10:28 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Mon, Aug 17, 2015 at 7:37 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sat, Aug 15, 2015 at 2:00 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>> +{
>>> +       struct strbuf aligned = STRBUF_INIT;
>>> +       struct ref_formatting_state *return_to = state->prev;
>>> +       struct align *align = (struct align *)state->cb_data;
>>> +
>>> +       strbuf_utf8_align(&aligned, align->position, align->width, state->output.buf);
>>> +       strbuf_addbuf(&return_to->output, &aligned);
>>
>> Second, I realize that Junio suggested the 'return_to' idea, but it
>> seems like it could become overly painful since each handler of this
>> sort is going to have to perform the same manipulation to append its
>> collected output to its parent state's output. What if you instead
>> make it the responsibility of pop_state() to append the 'output' from
>> the state being popped to the "prev" state's 'output'? This way, it
>> happens automatically, thus reducing code in each individual handler,
>> and reducing the burden of having to keep writing the same code.
>
> Good question, what if we don't want to append to strbuf at all?
> For e.g., We were discussing an "%(if).....%(then)......%(end)"
> atom structure, here if the if condition isn't met we wouldn't want to
> append to the prev strbuf, hence I thought it's better if the handler
> decided whether or not to append to prev strbuf.

An %(if)/%(then)/%(end) construct should not present a problem. As
long as the processing of the conditional ensures that the current
state's 'output' contains no content, when pop_state() appends that
(empty) content to the parent state's 'output', then the net result is
exactly the desired behavior.

The question of "how" the conditional processing ensures that the
current state's 'output' is empty when the %(if) condition is false is
unimportant (for this discussion). It might be the case that it just
doesn't collect any content at all for a false condition, or it
collects it but throws it away before the state is popped. Either way,
that's an implementation detail which needn't impact the decision to
retire 'return_to' and instead make pop_state() responsible for
appending the current state's output to the parent state's.

Re: [PATCH v11 05/13] ref-filter: implement an `align` atom

[Junio C Hamano]

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

> On Mon, Aug 17, 2015 at 7:37 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> ...
>> Second, I realize that Junio suggested the 'return_to' idea, but it
>> seems like it could become overly painful since each handler of this
>> sort is going to have to perform the same manipulation to append its
>> collected output to its parent state's output. What if you instead
>> make it the responsibility of pop_state() to append the 'output' from
>> the state being popped to the "prev" state's 'output'? This way, it
>> happens automatically, thus reducing code in each individual handler,
>> and reducing the burden of having to keep writing the same code.
>
> Good question, what if we don't want to append to strbuf at all?
> For e.g., We were discussing an "%(if).....%(then)......%(end)"
> atom structure, here if the if condition isn't met we wouldn't want to
> append to the prev strbuf, hence I thought it's better if the handler
> decided whether or not to append to prev strbuf.

I'd imagine the implementation of these to be along the lines of
(thinking aloud):

 - "%(if:[nonempty|empty|...])" pushes a new stack, and sets its
   attend/at_end/end_scope function to signal a syntax error.  It
   also records what condition (e.g. "nonempty") to use in the new
   stack.

 - "%(then)" inspects the top-of-stack output and uses the condition
   recorded by the %(if) that created the stack to decide true or
   false.  The stack element pushed by %(if) is then removed.
   Notice that the end_scope function prepared by %(if) is never
   called.

   Then (no pun intended):

   - If true, that means we would want the (expansion of) text up to
     "%(end)" or "%(else)", whichever comes first, appended to the
     surrounding output.  Push a new stack and set its end_scope
     function to the one that appends the top-of-stack output to the
     surrounding output, expecting %(end) will follow without
     %(else).

   - If false, that means we would want the (expansion of) text up
     to "%(end)" or "%(else)", whichever comes first, discarded.
     Push a new stack and set its end_scope function to the one that
     discards the top-of-stack output, expecting %(end) will follow
     without %(else).

 - "%(else)" inspects the top of the stack, and if it is not left by
   "%(then)", signal a syntax error.

   Else (no pun intended), it runs the end_scope function left by
   "%(then)" on the top-of-stack output (e.g. if "%(then)" found
   that the condition holds true, the accumulated output at this
   point should be appended to the surrounding output, and it was
   expected to be done by "%(end)" if this "%(else)" weren't
   present.  We do it here before waiting for "%(end)").

   Truncate the top-of-stack output, flip the end_scope function to
   the one opposite from the one left by "%(then)".  And let "%(end)"
   take care of it.

 - "%(end)" just unconditionally runs the end_scope function on the
   top of the stack output.

Eric's suggestion is let the caller of the end_scope function to
always append the output of the top-of-stack, and I think it makes
sense.  It makes a common "%(atom)" implementation simpler.  Things
like %(then) and %(else) above need to make sure that they reset the
top-of-stack output to empty as necessary, but that is not such a
huge implementation burden---their operation is already unusual and
needs to be more complex than the plain-vanilla %(atom)s anyway.

Re: [PATCH v11 05/13] ref-filter: implement an `align` atom

[Karthik Nayak]

On Mon, Aug 17, 2015 at 11:52 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, Aug 17, 2015 at 10:28 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> On Mon, Aug 17, 2015 at 7:37 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Sat, Aug 15, 2015 at 2:00 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>>> +{
>>>> +       struct strbuf aligned = STRBUF_INIT;
>>>> +       struct ref_formatting_state *return_to = state->prev;
>>>> +       struct align *align = (struct align *)state->cb_data;
>>>> +
>>>> +       strbuf_utf8_align(&aligned, align->position, align->width, state->output.buf);
>>>> +       strbuf_addbuf(&return_to->output, &aligned);
>>>
>>> Second, I realize that Junio suggested the 'return_to' idea, but it
>>> seems like it could become overly painful since each handler of this
>>> sort is going to have to perform the same manipulation to append its
>>> collected output to its parent state's output. What if you instead
>>> make it the responsibility of pop_state() to append the 'output' from
>>> the state being popped to the "prev" state's 'output'? This way, it
>>> happens automatically, thus reducing code in each individual handler,
>>> and reducing the burden of having to keep writing the same code.
>>
>> Good question, what if we don't want to append to strbuf at all?
>> For e.g., We were discussing an "%(if).....%(then)......%(end)"
>> atom structure, here if the if condition isn't met we wouldn't want to
>> append to the prev strbuf, hence I thought it's better if the handler
>> decided whether or not to append to prev strbuf.
>
> An %(if)/%(then)/%(end) construct should not present a problem. As
> long as the processing of the conditional ensures that the current
> state's 'output' contains no content, when pop_state() appends that
> (empty) content to the parent state's 'output', then the net result is
> exactly the desired behavior.
>
> The question of "how" the conditional processing ensures that the
> current state's 'output' is empty when the %(if) condition is false is
> unimportant (for this discussion). It might be the case that it just
> doesn't collect any content at all for a false condition, or it
> collects it but throws it away before the state is popped. Either way,
> that's an implementation detail which needn't impact the decision to
> retire 'return_to' and instead make pop_state() responsible for
> appending the current state's output to the parent state's.

I guess what you're saying also makes sense. We could make it common
for pop to push the strbuf into the prev state.

On Tue, Aug 18, 2015 at 12:09 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Mon, Aug 17, 2015 at 7:37 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> ...
>>> Second, I realize that Junio suggested the 'return_to' idea, but it
>>> seems like it could become overly painful since each handler of this
>>> sort is going to have to perform the same manipulation to append its
>>> collected output to its parent state's output. What if you instead
>>> make it the responsibility of pop_state() to append the 'output' from
>>> the state being popped to the "prev" state's 'output'? This way, it
>>> happens automatically, thus reducing code in each individual handler,
>>> and reducing the burden of having to keep writing the same code.
>>
>> Good question, what if we don't want to append to strbuf at all?
>> For e.g., We were discussing an "%(if).....%(then)......%(end)"
>> atom structure, here if the if condition isn't met we wouldn't want to
>> append to the prev strbuf, hence I thought it's better if the handler
>> decided whether or not to append to prev strbuf.
>
> I'd imagine the implementation of these to be along the lines of
> (thinking aloud):
>
>  - "%(if:[nonempty|empty|...])" pushes a new stack, and sets its
>    attend/at_end/end_scope function to signal a syntax error.  It
>    also records what condition (e.g. "nonempty") to use in the new
>    stack.
>
>  - "%(then)" inspects the top-of-stack output and uses the condition
>    recorded by the %(if) that created the stack to decide true or
>    false.  The stack element pushed by %(if) is then removed.
>    Notice that the end_scope function prepared by %(if) is never
>    called.
>
>    Then (no pun intended):
>
>    - If true, that means we would want the (expansion of) text up to
>      "%(end)" or "%(else)", whichever comes first, appended to the
>      surrounding output.  Push a new stack and set its end_scope
>      function to the one that appends the top-of-stack output to the
>      surrounding output, expecting %(end) will follow without
>      %(else).
>
>    - If false, that means we would want the (expansion of) text up
>      to "%(end)" or "%(else)", whichever comes first, discarded.
>      Push a new stack and set its end_scope function to the one that
>      discards the top-of-stack output, expecting %(end) will follow
>      without %(else).
>
>  - "%(else)" inspects the top of the stack, and if it is not left by
>    "%(then)", signal a syntax error.
>
>    Else (no pun intended), it runs the end_scope function left by
>    "%(then)" on the top-of-stack output (e.g. if "%(then)" found
>    that the condition holds true, the accumulated output at this
>    point should be appended to the surrounding output, and it was
>    expected to be done by "%(end)" if this "%(else)" weren't
>    present.  We do it here before waiting for "%(end)").
>
>    Truncate the top-of-stack output, flip the end_scope function to
>    the one opposite from the one left by "%(then)".  And let "%(end)"
>    take care of it.
>
>  - "%(end)" just unconditionally runs the end_scope function on the
>    top of the stack output.
>

That seems like the way to go, thanks for summing it up. Will follow this
when implementing the %(if) %(then) %(else) %(end) atom series.

> Eric's suggestion is let the caller of the end_scope function to
> always append the output of the top-of-stack, and I think it makes
> sense.  It makes a common "%(atom)" implementation simpler.  Things
> like %(then) and %(else) above need to make sure that they reset the
> top-of-stack output to empty as necessary, but that is not such a
> huge implementation burden---their operation is already unusual and
> needs to be more complex than the plain-vanilla %(atom)s anyway.

Yea, I get why you're suggesting it too, It'd remove that extra work each
handler has to do and make sure is done. By moving it to pop_state()
we don't have to leave it to the handler to be taken care of.

-- 
Regards,
Karthik Nayak