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

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 [off-list ref]
Mentored-by: Matthieu Moy [off-list ref]
Signed-off-by: Karthik Nayak <redacted>
---
 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

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

On Sat, Aug 15, 2015 at 2:00 PM, Karthik Nayak [off-list ref] wrote:

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.

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.

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.

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

Why does this need to be #included here?

 /* Quoting styles */
 #define QUOTE_NONE 0

On Mon, Aug 17, 2015 at 7:37 AM, Eric Sunshine [off-list ref] wrote:

On Sat, Aug 15, 2015 at 2:00 PM, Karthik Nayak [off-list ref] wrote:

quoted

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 <redacted>
---

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.

quoted

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

quoted

 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?

quoted

+{
+       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.

quoted

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

quoted

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

quoted

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

quoted

+                       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

quoted

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

quoted

+                       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

On Mon, Aug 17, 2015 at 10:28 AM, Karthik Nayak [off-list ref] wrote:

On Mon, Aug 17, 2015 at 7:37 AM, Eric Sunshine [off-list ref] wrote:

quoted

On Sat, Aug 15, 2015 at 2:00 PM, Karthik Nayak [off-list ref] wrote:

quoted

+{
+       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.