[PATCH v9 03/11] ref-filter: implement an `align` atom

[PATCH v9 03/11] ref-filter: implement an `align` atom

[Karthik Nayak]

Implement an `align` atom which will act as a modifier atom and align
any string with or without an %(atom) appearing before a %(end) atom
to the right, left or middle.

It is followed by `:<type>,<paddinglength>`, where the `<type>` is
either left, right or middle and `<paddinglength>` is the total length
of the padding to be performed. If the atom length is more than the
padding length then no padding is performed. e.g. to pad a succeeding
atom to the middle with a total padding size of 40 we can do a
--format="%(align:middle,40).."

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                       | 84 +++++++++++++++++++++++++++++++++++---
 ref-filter.h                       | 18 +++++++-
 t/t6302-for-each-ref-filter.sh     | 48 ++++++++++++++++++++++
 4 files changed, 151 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index e49d578..d865f98 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::
+	Align any string with or without %(atom) before the %(end)
+	atom to the right, left or middle. Followed by
+	`:<type>,<paddinglength>`, where the `<type>` is either left,
+	right or middle and `<paddinglength>` is the total length of
+	the padding to be performed. If the string length is more than
+	the padding length then no padding 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 2c074a1..d123299 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,6 +54,8 @@ static struct {
 	{ "flag" },
 	{ "HEAD" },
 	{ "color" },
+	{ "align" },
+	{ "end" },
 };
 
 /*
@@ -620,7 +623,7 @@ static void populate_value(struct ref_array_item *ref)
 		const char *name = used_atom[i];
 		struct atom_value *v = &ref->value[i];
 		int deref = 0;
-		const char *refname;
+		const char *refname = NULL;
 		const char *formatp;
 		struct branch *branch = NULL;
 
@@ -687,6 +690,29 @@ static void populate_value(struct ref_array_item *ref)
 			else
 				v->s = " ";
 			continue;
+		} else if (starts_with(name, "align:")) {
+			const char *valp = NULL;
+			struct align *align = xmalloc(sizeof(struct align));
+
+			skip_prefix(name, "align:", &valp);
+
+			if (skip_prefix(valp, "left,", &valp))
+				align->align_type = ALIGN_LEFT;
+			else if (skip_prefix(valp, "right,", &valp))
+				align->align_type = ALIGN_RIGHT;
+			else if (skip_prefix(valp, "middle,", &valp))
+				align->align_type = ALIGN_MIDDLE;
+			else
+				die(_("align: improper format"));
+			if (strtoul_ui(valp, 10, &align->align_value))
+				die(_("align: positive value expected"));
+			v->align = align;
+			v->modifier_atom = 1;
+			continue;
+		} else if (starts_with(name, "end")) {
+			v->end = 1;
+			v->modifier_atom = 1;
+			continue;
 		} else
 			continue;
 
@@ -1251,12 +1277,48 @@ static void emit(const char *cp, const char *ep, struct ref_formatting_state *st
 
 static void process_formatting_state(struct atom_value *atomv, struct ref_formatting_state *state)
 {
-	/* Based on the atomv values, the formatting state is set */
+	if (atomv->align) {
+		state->align = atomv->align;
+		atomv->align = NULL;
+	}
+	if (atomv->end)
+		state->end = 1;
 }
 
 static void apply_formatting_state(struct ref_formatting_state *state, struct strbuf *final)
 {
-	/* More formatting options to be evetually added */
+	if (state->align && state->end) {
+		struct strbuf *value = state->output;
+		int len = 0, buf_len = value->len;
+		struct align *align = state->align;
+
+		if (!value->buf)
+			return;
+		if (!is_utf8(value->buf)) {
+			len = value->len - utf8_strwidth(value->buf);
+			buf_len -= len;
+		}
+
+		if (align->align_value < buf_len) {
+			state->align = NULL;
+			strbuf_addbuf(final, value);
+			strbuf_release(value);
+			return;
+		}
+
+		if (align->align_type == ALIGN_LEFT)
+			strbuf_addf(final, "%-*s", len + align->align_value, value->buf);
+		else if (align->align_type == ALIGN_MIDDLE) {
+			int right = (align->align_value - buf_len)/2;
+			strbuf_addf(final, "%*s%-*s", align->align_value - right + len,
+				    value->buf, right, "");
+		} else if (align->align_type == ALIGN_RIGHT)
+			strbuf_addf(final, "%*s", align->align_value, value->buf);
+		strbuf_release(value);
+		state->align = NULL;
+		return;
+	} else if (state->align)
+		return;
 	strbuf_addbuf(final, state->output);
 	strbuf_release(state->output);
 }
@@ -1282,10 +1344,19 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
 			apply_formatting_state(&state, &final_buf);
 		}
 		get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
-		process_formatting_state(atomv, &state);
-		print_value(atomv, &state);
-		apply_formatting_state(&state, &final_buf);
+		if (atomv->modifier_atom)
+			process_formatting_state(atomv, &state);
+		else {
+			print_value(atomv, &state);
+			apply_formatting_state(&state, &final_buf);
+		}
+		if (atomv->end) {
+			print_value(atomv, &state);
+			apply_formatting_state(&state, &final_buf);
+		}
 	}
+	if (state.align)
+		die(_("format: align used without an `end` atom"));
 	if (*cp) {
 		sp = cp + strlen(cp);
 		emit(cp, sp, &state);
@@ -1301,6 +1372,7 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
 		print_value(&resetv, &state);
 		apply_formatting_state(&state, &final_buf);
 	}
+
 	for (i = 0; i < final_buf.len; i++)
 		printf("%c", final_buf.buf[i]);
 	putchar('\n');
diff --git a/ref-filter.h b/ref-filter.h
index 9e6c2d4..5575fe9 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -16,14 +16,30 @@
 #define FILTER_REFS_INCLUDE_BROKEN 0x1
 #define FILTER_REFS_ALL 0x2
 
+typedef enum {
+	ALIGN_LEFT,
+	ALIGN_MIDDLE,
+	ALIGN_RIGHT
+} align_type;
+
 struct ref_formatting_state {
-	int quote_style;
 	struct strbuf *output;
+	struct align *align;
+	int quote_style;
+	unsigned int end : 1;
+};
+
+struct align {
+	align_type align_type;
+	unsigned int align_value;
 };
 
 struct atom_value {
 	const char *s;
+	struct align *align;
 	unsigned long ul; /* used for sorting when not FIELD_STR */
+	unsigned int modifier_atom : 1,
+		end : 1;
 };
 
 struct ref_sorting {
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 505a360..76041a2 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:left,30)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:middle,30)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:right,30)refname is %(refname)%(end)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.5.0

Re: [PATCH v9 03/11] ref-filter: implement an `align` atom

[Eric Sunshine]

On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Implement an `align` atom which will act as a modifier atom and align
> any string with or without an %(atom) appearing before a %(end) atom
> to the right, left or middle.

For someone not familiar with the evolution of this patch series,
"align any string with or without an %(atom)" is confusing and
distracting and seems to imply that something extra mysterious is
going on behind the scenes. Keeping it simple makes it easier to
understand:

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

> It is followed by `:<type>,<paddinglength>`, where the `<type>` is

'type' may not be the best name. Perhaps 'style' or 'position' or
something else would be better?

> either left, right or middle and `<paddinglength>` is the total length

'paddinglength' is misleading. You're really describing the container
width in which alignment takes places, so perhaps call it 'width' or
'fieldwidth' or something.

> of the padding to be performed. If the atom length is more than the
> padding length then no padding is performed. e.g. to pad a succeeding
> atom to the middle with a total padding size of 40 we can do a

It's odd to have alignment described in terms of "padding" and
"padding length", especially in the case of "center" alignment. It
might be better to rephrase the discussion in terms of field width or
such.

> --format="%(align:middle,40).."

I may have missed the discussion, but why was comma chosen as a
separator here, rather than, say, colon? For instance:

    %(align:middle:40)

> Add documentation and tests for the same.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index e49d578..d865f98 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -127,6 +127,14 @@ color::
> +align::
> +       Align any string with or without %(atom) before the %(end)
> +       atom to the right, left or middle. Followed by

Ditto regarding "any string with or without %(atom)" being confusing
to someone not familiar with the evolution of this patch series.
Instead, perhaps:

    Left-, middle-, or right-align content between %(align:...)
    and %(end).

> +       `:<type>,<paddinglength>`, where the `<type>` is either left,
> +       right or middle and `<paddinglength>` is the total length of
> +       the padding to be performed. If the string length is more than
> +       the padding length then no padding is performed.

Ditto regarding above observations about 'type', 'paddinglength', and "padding".

> diff --git a/ref-filter.c b/ref-filter.c
> index 2c074a1..d123299 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -620,7 +623,7 @@ static void populate_value(struct ref_array_item *ref)
>                 const char *name = used_atom[i];
>                 struct atom_value *v = &ref->value[i];
>                 int deref = 0;
> -               const char *refname;
> +               const char *refname = NULL;

What is this change about?

>                 const char *formatp;
>                 struct branch *branch = NULL;
>
> @@ -687,6 +690,29 @@ static void populate_value(struct ref_array_item *ref)
>                         else
>                                 v->s = " ";
>                         continue;
> +               } else if (starts_with(name, "align:")) {
> +                       const char *valp = NULL;

Unnecessary NULL assignment.

> +                       struct align *align = xmalloc(sizeof(struct align));
> +
> +                       skip_prefix(name, "align:", &valp);

You could simplify the code by combining this skip_prefix() with the
earlier starts_with() in the conditional:

    } else if (skip_prefix(name, "align:", &valp)) {
        struct align *align = xmalloc(sizeof(struct align));
        ...

> +                       if (skip_prefix(valp, "left,", &valp))
> +                               align->align_type = ALIGN_LEFT;
> +                       else if (skip_prefix(valp, "right,", &valp))
> +                               align->align_type = ALIGN_RIGHT;
> +                       else if (skip_prefix(valp, "middle,", &valp))
> +                               align->align_type = ALIGN_MIDDLE;
> +                       else
> +                               die(_("align: improper format"));

You could do a better job of helping the user track down the offending
"improper format" by actually including it in the error message.

> +                       if (strtoul_ui(valp, 10, &align->align_value))
> +                               die(_("align: positive value expected"));

Ditto.

> +                       v->align = align;
> +                       v->modifier_atom = 1;
> +                       continue;
> +               } else if (starts_with(name, "end")) {
> +                       v->end = 1;
> +                       v->modifier_atom = 1;
> +                       continue;
>                 } else
>                         continue;
>
> @@ -1251,12 +1277,48 @@ static void emit(const char *cp, const char *ep, struct ref_formatting_state *st
>  static void apply_formatting_state(struct ref_formatting_state *state, struct strbuf *final)
>  {
> -       /* More formatting options to be evetually added */
> +       if (state->align && state->end) {
> +               struct strbuf *value = state->output;
> +               int len = 0, buf_len = value->len;
> +               struct align *align = state->align;
> +
> +               if (!value->buf)
> +                       return;
> +               if (!is_utf8(value->buf)) {
> +                       len = value->len - utf8_strwidth(value->buf);

What is this doing, exactly? If the string is *not* utf-8, then you're
asking it for its utf-8 width. Am I reading that correctly?

Also, what is 'len' supposed to represent? I guess you want it to be
the difference between the byte length and the display length, but the
name 'len' doesn't convey that at all, nor does it help the reader
understand the code below where you do the actual formatting.

In fact, if I'm reading this correctly, then 'len' is always zero in
your tests (because the tests never trigger this conditional), so this
functionality is never being exercised.

> +                       buf_len -= len;
> +               }
> +
> +               if (align->align_value < buf_len) {

Shouldn't this be '<=' rather than '<'? There's no point going through
the formatting gyrations below if the string fills the alignment width
exactly.

Also, what's the purpose of 'buf_len'? It's value is always
(value->len - len), so you could just as easily say:

    if (align->align_value <= value->len - len) {

In fact, the misleading name 'len', along with 'buf_len' and
value->len tend to make this code difficult to comprehend. If,
instead, you had a variable named 'display_len', then its meaning
would be obvious, and computations involving it would be more easily
understood. The value of 'display_len' could be computed via:

    display_len = utf8_strnwidth(value->buf, value->len, 0);

which would give you the utf-8 width if utf-8, or value->len if not.

And, the above conditional would become the more readable:

    if (align->align_value <= display_len) {

although, I find it easier to understand the logic with the condition
switched around:

    if (display_len >= align->align_value) {
        ...don't bother with alignment gyrations...

(but that's just subjective)

> +                       state->align = NULL;
> +                       strbuf_addbuf(final, value);
> +                       strbuf_release(value);
> +                       return;
> +               }
> +
> +               if (align->align_type == ALIGN_LEFT)
> +                       strbuf_addf(final, "%-*s", len + align->align_value, value->buf);
> +               else if (align->align_type == ALIGN_MIDDLE) {
> +                       int right = (align->align_value - buf_len)/2;
> +                       strbuf_addf(final, "%*s%-*s", align->align_value - right + len,
> +                                   value->buf, right, "");

Why do you use the left-justifying format "%-*s" when interpolating
the zero-length string considering that "%*s" works just as well?

An aesthetic aside: When (align_value - buf_len) is an odd number,
this implementation favors placing more whitespace to the left of the
string, and less to the right. In practice, this often tends to look a
bit more awkward than the inverse of placing more whitespace to the
right, and less to the left (but that again is subjective).

> +               } else if (align->align_type == ALIGN_RIGHT)
> +                       strbuf_addf(final, "%*s", align->align_value, value->buf);

Why doesn't this case need to take 'len' into account like the other cases?

This is a tangent, but I could easily see all of the code from 'if
(align->align_value < buf_len)' down to this point being placed in
utf8.c as a general alignment utility function. Doing so would make
this function shorter, and the patch easier to review overall (which
might be an important consideration -- especially given that I've
already spent several hours reviewing this one patch).

> +               strbuf_release(value);
> +               state->align = NULL;
> +               return;
> +       } else if (state->align)
> +               return;

Do you expect additional types of state processing in the future? If
so, this function is likely to get very long. It might make sense to
break the alignment logic out into its own function which is called
from this one.

>         strbuf_addbuf(final, state->output);
>         strbuf_release(state->output);
>  }
> @@ -1301,6 +1372,7 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
>                 print_value(&resetv, &state);
>                 apply_formatting_state(&state, &final_buf);
>         }
> +

Sneaking in a whitespace change which an earlier patch perhaps should
have formatted correctly in the first place?

>         for (i = 0; i < final_buf.len; i++)
>                 printf("%c", final_buf.buf[i]);
>         putchar('\n');
> diff --git a/ref-filter.h b/ref-filter.h
> index 9e6c2d4..5575fe9 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -16,14 +16,30 @@
>  struct ref_formatting_state {
> -       int quote_style;
>         struct strbuf *output;
> +       struct align *align;
> +       int quote_style;

Perhaps decide where you'd like 'quote_style' to reside from the start
so that you don't have to add it at one location in its introductory
patch and then move it in a later patch. Also, why move it here?

> +       unsigned int end : 1;
> +};
> +
> +struct align {
> +       align_type align_type;

No need for an "align_" prefix on the members of 'struct align'.
That's just unneeded verbosity.

As mentioned above, 'type' may not be the best name. Perhaps 'style'
or 'position' or something better.

> +       unsigned int align_value;

Ditto. 'value' doesn't say much, whereas 'width' or 'fieldwidth' would
be more meaningful.

>  };
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> index 505a360..76041a2 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:left,30)refname is %(refname)%(end)|%(refname)" >actual &&
> +       test_cmp expect actual
> +'

Alluding to a previous review comment, for this left-alignment test,
perhaps add a third column to prove that the %(align:) atom isn't
leaking from column 1 to column 2 since it's not obvious to the
reader, given that trailing whitespace is not otherwise visible.

> +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:middle,30)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:right,30)refname is %(refname)%(end)|%(refname)" >actual &&
> +       test_cmp expect actual
> +'
> +
>  test_done
> --
> 2.5.0

Re: [PATCH v9 03/11] ref-filter: implement an `align` atom

[Eric Sunshine]

On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Implement an `align` atom which will act as a modifier atom and align
> any string with or without an %(atom) appearing before a %(end) atom
> to the right, left or middle.
>
> It is followed by `:<type>,<paddinglength>`, where the `<type>` is
> either left, right or middle and `<paddinglength>` is the total length
> of the padding to be performed. If the atom length is more than the
> padding length then no padding is performed. e.g. to pad a succeeding
> atom to the middle with a total padding size of 40 we can do a
> --format="%(align:middle,40).."
>
> Add documentation and tests for the same.

I forgot to mention in my earlier review of this patch that you should
explain in the commit message, and probably the documentation, this
this implementation (assuming I'm understanding the code) does not
correctly support nested %(foo)...%(end) constructs, where %(foo)
might be %(if:), %(truncate:), %(cut:), or even a nested %(align:), or
some as yet unimagined modifier. Supporting nesting of these
constructs will require pushing the formatting states onto a stack (or
invoking the parser recursively).

> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>

Re: [PATCH v9 03/11] ref-filter: implement an `align` atom

[Karthik Nayak]

On Fri, Aug 7, 2015 at 8:57 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Implement an `align` atom which will act as a modifier atom and align
>> any string with or without an %(atom) appearing before a %(end) atom
>> to the right, left or middle.
>
> For someone not familiar with the evolution of this patch series,
> "align any string with or without an %(atom)" is confusing and
> distracting and seems to imply that something extra mysterious is
> going on behind the scenes. Keeping it simple makes it easier to
> understand:
>
>     Add an `align` atom which left-, middle-, or right-aligns the
>     content between %(align:...) and %(end).
>

Ok will change this.

>> It is followed by `:<type>,<paddinglength>`, where the `<type>` is
>
> 'type' may not be the best name. Perhaps 'style' or 'position' or
> something else would be better?
>

position is a better term.

>> either left, right or middle and `<paddinglength>` is the total length
>
> 'paddinglength' is misleading. You're really describing the container
> width in which alignment takes places, so perhaps call it 'width' or
> 'fieldwidth' or something.
>

width seems good to go.

>> of the padding to be performed. If the atom length is more than the
>> padding length then no padding is performed. e.g. to pad a succeeding
>> atom to the middle with a total padding size of 40 we can do a
>
> It's odd to have alignment described in terms of "padding" and
> "padding length", especially in the case of "center" alignment. It
> might be better to rephrase the discussion in terms of field width or
> such.
>
>> --format="%(align:middle,40).."

Ok this makes sense,
I'll rephrase as :

`<width>` is the total length of the content with alignment.
If the atom length is more than the width then no alignment is performed.
e.g. to align a succeeding atom to the middle with a total width of 40
we can do:
--format="%(align:middle,40).."

>
> I may have missed the discussion, but why was comma chosen as a
> separator here, rather than, say, colon? For instance:
>
>     %(align:middle:40)
>

I think it's based of this:
http://thread.gmane.org/gmane.comp.version-control.git/275119

>> Add documentation and tests for the same.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>> index e49d578..d865f98 100644
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -127,6 +127,14 @@ color::
>> +align::
>> +       Align any string with or without %(atom) before the %(end)
>> +       atom to the right, left or middle. Followed by
>
> Ditto regarding "any string with or without %(atom)" being confusing
> to someone not familiar with the evolution of this patch series.
> Instead, perhaps:
>
>     Left-, middle-, or right-align content between %(align:...)
>     and %(end).
>

Will change as per change in the commit message.

>> +       `:<type>,<paddinglength>`, where the `<type>` is either left,
>> +       right or middle and `<paddinglength>` is the total length of
>> +       the padding to be performed. If the string length is more than
>> +       the padding length then no padding is performed.
>
> Ditto regarding above observations about 'type', 'paddinglength', and "padding".
>

Noted.

>> diff --git a/ref-filter.c b/ref-filter.c
>> index 2c074a1..d123299 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -620,7 +623,7 @@ static void populate_value(struct ref_array_item *ref)
>>                 const char *name = used_atom[i];
>>                 struct atom_value *v = &ref->value[i];
>>                 int deref = 0;
>> -               const char *refname;
>> +               const char *refname = NULL;
>
> What is this change about?
>

Was to remove the compiler error, can be removed now.

>>                 const char *formatp;
>>                 struct branch *branch = NULL;
>>
>> @@ -687,6 +690,29 @@ static void populate_value(struct ref_array_item *ref)
>>                         else
>>                                 v->s = " ";
>>                         continue;
>> +               } else if (starts_with(name, "align:")) {
>> +                       const char *valp = NULL;
>
> Unnecessary NULL assignment.
>

Thats required for the second skip_prefix and so on.
Else we get: "warning: ‘valp’ may be used uninitialized in this
function [-Wmaybe-uninitialized]"

>> +                       struct align *align = xmalloc(sizeof(struct align));
>> +
>> +                       skip_prefix(name, "align:", &valp);
>
> You could simplify the code by combining this skip_prefix() with the
> earlier starts_with() in the conditional:
>
>     } else if (skip_prefix(name, "align:", &valp)) {
>         struct align *align = xmalloc(sizeof(struct align));
>         ...
>

That would require valp to be previously defined. Hence the split.

>> +                       if (skip_prefix(valp, "left,", &valp))
>> +                               align->align_type = ALIGN_LEFT;
>> +                       else if (skip_prefix(valp, "right,", &valp))
>> +                               align->align_type = ALIGN_RIGHT;
>> +                       else if (skip_prefix(valp, "middle,", &valp))
>> +                               align->align_type = ALIGN_MIDDLE;
>> +                       else
>> +                               die(_("align: improper format"));
>
> You could do a better job of helping the user track down the offending
> "improper format" by actually including it in the error message.
>

Ok will do.

>> +                       if (strtoul_ui(valp, 10, &align->align_value))
>> +                               die(_("align: positive value expected"));
>
> Ditto.
>

Will change.

>> +                       v->align = align;
>> +                       v->modifier_atom = 1;
>> +                       continue;
>> +               } else if (starts_with(name, "end")) {
>> +                       v->end = 1;
>> +                       v->modifier_atom = 1;
>> +                       continue;
>>                 } else
>>                         continue;
>>
>> @@ -1251,12 +1277,48 @@ static void emit(const char *cp, const char *ep, struct ref_formatting_state *st
>>  static void apply_formatting_state(struct ref_formatting_state *state, struct strbuf *final)
>>  {
>> -       /* More formatting options to be evetually added */
>> +       if (state->align && state->end) {
>> +               struct strbuf *value = state->output;
>> +               int len = 0, buf_len = value->len;
>> +               struct align *align = state->align;
>> +
>> +               if (!value->buf)
>> +                       return;
>> +               if (!is_utf8(value->buf)) {
>> +                       len = value->len - utf8_strwidth(value->buf);
>
> What is this doing, exactly? If the string is *not* utf-8, then you're
> asking it for its utf-8 width. Am I reading that correctly?
>
> Also, what is 'len' supposed to represent? I guess you want it to be
> the difference between the byte length and the display length, but the
> name 'len' doesn't convey that at all, nor does it help the reader
> understand the code below where you do the actual formatting.
>
> In fact, if I'm reading this correctly, then 'len' is always zero in
> your tests (because the tests never trigger this conditional), so this
> functionality is never being exercised.
>

There shouldn't be a "!" there, will change.
I guess 'utf8_compensation' would be a better name.

>> +                       buf_len -= len;
>> +               }
>> +
>> +               if (align->align_value < buf_len) {
>
> Shouldn't this be '<=' rather than '<'? There's no point going through
> the formatting gyrations below if the string fills the alignment width
> exactly.
>

Good point.

> Also, what's the purpose of 'buf_len'? It's value is always
> (value->len - len), so you could just as easily say:
>
>     if (align->align_value <= value->len - len) {
>
> In fact, the misleading name 'len', along with 'buf_len' and
> value->len tend to make this code difficult to comprehend. If,
> instead, you had a variable named 'display_len', then its meaning
> would be obvious, and computations involving it would be more easily
> understood. The value of 'display_len' could be computed via:
>
>     display_len = utf8_strnwidth(value->buf, value->len, 0);

This is brilliant, would also remove the if statement for checking if it's
utf8.

>
> which would give you the utf-8 width if utf-8, or value->len if not.
>
> And, the above conditional would become the more readable:
>
>     if (align->align_value <= display_len) {
>
> although, I find it easier to understand the logic with the condition
> switched around:
>
>     if (display_len >= align->align_value) {
>         ...don't bother with alignment gyrations...
>
> (but that's just subjective)
>

Yea with the name diplay_len that'd make sense, as you're saying is the length
is greater than alignment value, don't bother with it.

>> +                       state->align = NULL;
>> +                       strbuf_addbuf(final, value);
>> +                       strbuf_release(value);
>> +                       return;
>> +               }
>> +
>> +               if (align->align_type == ALIGN_LEFT)
>> +                       strbuf_addf(final, "%-*s", len + align->align_value, value->buf);
>> +               else if (align->align_type == ALIGN_MIDDLE) {
>> +                       int right = (align->align_value - buf_len)/2;
>> +                       strbuf_addf(final, "%*s%-*s", align->align_value - right + len,
>> +                                   value->buf, right, "");
>
> Why do you use the left-justifying format "%-*s" when interpolating
> the zero-length string considering that "%*s" works just as well?
>

Yes that also would work.

> An aesthetic aside: When (align_value - buf_len) is an odd number,
> this implementation favors placing more whitespace to the left of the
> string, and less to the right. In practice, this often tends to look a
> bit more awkward than the inverse of placing more whitespace to the
> right, and less to the left (but that again is subjective).
>

I know that, maybe we could add an additional padding to even out the value
given?

>> +               } else if (align->align_type == ALIGN_RIGHT)
>> +                       strbuf_addf(final, "%*s", align->align_value, value->buf);
>
> Why doesn't this case need to take 'len' into account like the other cases?
>

This needs to be changed.

> This is a tangent, but I could easily see all of the code from 'if
> (align->align_value < buf_len)' down to this point being placed in
> utf8.c as a general alignment utility function. Doing so would make
> this function shorter, and the patch easier to review overall (which
> might be an important consideration -- especially given that I've
> already spent several hours reviewing this one patch).
>

That's a valid suggestion, will do that, thanks, but that'd mean we need to
send an align struct to utf8.c which is only defined in ref-filter.h.
Either this
is fine or we need to move the definition of struct align to utf8.h.

I think personally move the align structure and enum over to utf8.h

>> +               strbuf_release(value);
>> +               state->align = NULL;
>> +               return;
>> +       } else if (state->align)
>> +               return;
>
> Do you expect additional types of state processing in the future? If
> so, this function is likely to get very long. It might make sense to
> break the alignment logic out into its own function which is called
> from this one.
>

Makes sense, will do.

>>         strbuf_addbuf(final, state->output);
>>         strbuf_release(state->output);
>>  }
>> @@ -1301,6 +1372,7 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
>>                 print_value(&resetv, &state);
>>                 apply_formatting_state(&state, &final_buf);
>>         }
>> +
>
> Sneaking in a whitespace change which an earlier patch perhaps should
> have formatted correctly in the first place?
>

will change.

>>         for (i = 0; i < final_buf.len; i++)
>>                 printf("%c", final_buf.buf[i]);
>>         putchar('\n');
>> diff --git a/ref-filter.h b/ref-filter.h
>> index 9e6c2d4..5575fe9 100644
>> --- a/ref-filter.h
>> +++ b/ref-filter.h
>> @@ -16,14 +16,30 @@
>>  struct ref_formatting_state {
>> -       int quote_style;
>>         struct strbuf *output;
>> +       struct align *align;
>> +       int quote_style;
>
> Perhaps decide where you'd like 'quote_style' to reside from the start
> so that you don't have to add it at one location in its introductory
> patch and then move it in a later patch. Also, why move it here?
>

Cause that'd save memory on a 64 bit processor, where the pointers would
be 8 bytes long and int would be 4 bytes long, this would bring in padding if
int is placed before the pointers. Will change before hand.

>> +       unsigned int end : 1;
>> +};
>> +
>> +struct align {
>> +       align_type align_type;
>
> No need for an "align_" prefix on the members of 'struct align'.
> That's just unneeded verbosity.
>

Noted, will change.

> As mentioned above, 'type' may not be the best name. Perhaps 'style'
> or 'position' or something better.
>

Position seems better choice, will change that throughout.

>> +       unsigned int align_value;
>
> Ditto. 'value' doesn't say much, whereas 'width' or 'fieldwidth' would
> be more meaningful.
>

width seems good.

>>  };
>> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
>> index 505a360..76041a2 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:left,30)refname is %(refname)%(end)|%(refname)" >actual &&
>> +       test_cmp expect actual
>> +'
>
> Alluding to a previous review comment, for this left-alignment test,
> perhaps add a third column to prove that the %(align:) atom isn't
> leaking from column 1 to column 2 since it's not obvious to the
> reader, given that trailing whitespace is not otherwise visible.
>

Well its kinda pointless here cause you need an %(end) atom for alignment,
so if there was a possible leak its hard to tell where the leak would
extend till
as we need an %(end) atom to process alignment.

>> +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:middle,30)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:right,30)refname is %(refname)%(end)|%(refname)" >actual &&
>> +       test_cmp expect actual
>> +'
>> +
>>  test_done
>> --
>> 2.5.0



-- 
Regards,
Karthik Nayak

Re: [PATCH v9 03/11] ref-filter: implement an `align` atom

[Karthik Nayak]

On Fri, Aug 7, 2015 at 9:34 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Implement an `align` atom which will act as a modifier atom and align
>> any string with or without an %(atom) appearing before a %(end) atom
>> to the right, left or middle.
>>
>> It is followed by `:<type>,<paddinglength>`, where the `<type>` is
>> either left, right or middle and `<paddinglength>` is the total length
>> of the padding to be performed. If the atom length is more than the
>> padding length then no padding is performed. e.g. to pad a succeeding
>> atom to the middle with a total padding size of 40 we can do a
>> --format="%(align:middle,40).."
>>
>> Add documentation and tests for the same.
>
> I forgot to mention in my earlier review of this patch that you should
> explain in the commit message, and probably the documentation, this
> this implementation (assuming I'm understanding the code) does not
> correctly support nested %(foo)...%(end) constructs, where %(foo)
> might be %(if:), %(truncate:), %(cut:), or even a nested %(align:), or
> some as yet unimagined modifier. Supporting nesting of these
> constructs will require pushing the formatting states onto a stack (or
> invoking the parser recursively).
>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>

Good point, I have been working on this parallely and it works for now,
I'll send that with the %(if) and %(end) feature. But for now, it should be
documented and added in the commit message.

Using a linked list of sorts where whenever a new modifier atom is encountered
a new state is created, and once %(end) is encountered we can pop that state
into the previous state.

-- 
Regards,
Karthik Nayak

Re: [PATCH v9 03/11] ref-filter: implement an `align` atom

[Eric Sunshine]

On Sat, Aug 8, 2015 at 2:35 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Fri, Aug 7, 2015 at 8:57 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>> of the padding to be performed. If the atom length is more than the
>>> padding length then no padding is performed. e.g. to pad a succeeding
>>> atom to the middle with a total padding size of 40 we can do a
>>
>> It's odd to have alignment described in terms of "padding" and
>> "padding length", especially in the case of "center" alignment. It
>> might be better to rephrase the discussion in terms of field width or
>> such.
>>
>>> --format="%(align:middle,40).."
>
> Ok this makes sense,
> I'll rephrase as :
>
> `<width>` is the total length of the content with alignment.

This doesn't really make sense. <width> isn't the content length; it's
the size of the area into which the content will be placed.

> If the atom length is more than the width then no alignment is performed.

What "atom"? I think you mean the content between %(align:) and %(end)
rather than "atom". The description might be clearer if you actually
say "content between %(align:) and %(end)" to indicate specifically
what is being aligned.

> e.g. to align a succeeding atom to the middle with a total width of 40
> we can do:
> --format="%(align:middle,40).."
>>> @@ -687,6 +690,29 @@ static void populate_value(struct ref_array_item *ref)
>>>                         else
>>>                                 v->s = " ";
>>>                         continue;
>>> +               } else if (starts_with(name, "align:")) {
>>> +                       const char *valp = NULL;
>>
>> Unnecessary NULL assignment.
>
> Thats required for the second skip_prefix and so on.
> Else we get: "warning: ‘valp’ may be used uninitialized in this
> function [-Wmaybe-uninitialized]"

Okay, so that's because skip_prefix() is inline, and it doesn't touch
its "out" argument unless it actually skips the prefix. Ugly, but
makes sense, although I think this issue would go away if you combined
the starts_with() and skips_prefix() as suggested earlier.

>>> +                       struct align *align = xmalloc(sizeof(struct align));
>>> +
>>> +                       skip_prefix(name, "align:", &valp);
>>
>> You could simplify the code by combining this skip_prefix() with the
>> earlier starts_with() in the conditional:
>>
>>     } else if (skip_prefix(name, "align:", &valp)) {
>>         struct align *align = xmalloc(sizeof(struct align));
>>         ...
>
> That would require valp to be previously defined. Hence the split.

Yes, it would require declaring 'valp' earlier, but that seems a
reasonable tradeoff for cleaner, simpler, less redundant code.

>>>  static void apply_formatting_state(struct ref_formatting_state *state, struct strbuf *final)
>>>  {
>>> -       /* More formatting options to be evetually added */
>>> +       if (state->align && state->end) {
>>> +               struct strbuf *value = state->output;
>>> +               int len = 0, buf_len = value->len;
>>> +               struct align *align = state->align;
>>> +
>>> +               if (!value->buf)
>>> +                       return;
>>> +               if (!is_utf8(value->buf)) {
>>> +                       len = value->len - utf8_strwidth(value->buf);
>>
>> What is this doing, exactly? If the string is *not* utf-8, then you're
>> asking it for its utf-8 width. Am I reading that correctly?
>>
>> Also, what is 'len' supposed to represent? I guess you want it to be
>> the difference between the byte length and the display length, but the
>> name 'len' doesn't convey that at all, nor does it help the reader
>> understand the code below where you do the actual formatting.
>>
>> In fact, if I'm reading this correctly, then 'len' is always zero in
>> your tests (because the tests never trigger this conditional), so this
>> functionality is never being exercised.
>
> There shouldn't be a "!" there, will change.
> I guess 'utf8_compensation' would be a better name.

Definitely better than 'len'.

>>> +               else if (align->align_type == ALIGN_MIDDLE) {
>>> +                       int right = (align->align_value - buf_len)/2;
>>> +                       strbuf_addf(final, "%*s%-*s", align->align_value - right + len,
>>> +                                   value->buf, right, "");
>>
>> An aesthetic aside: When (align_value - buf_len) is an odd number,
>> this implementation favors placing more whitespace to the left of the
>> string, and less to the right. In practice, this often tends to look a
>> bit more awkward than the inverse of placing more whitespace to the
>> right, and less to the left (but that again is subjective).
>
> I know that, maybe we could add an additional padding to even out the value
> given?

I don't understand your question. I was merely suggesting (purely
subjectively), for the "odd length" case, putting the extra space
after the centered text rather than before it. For instance:

    int left = (align->align_value - buf_len) / 2;
    strbuf_addf(final, "%*s%-*s", left, "",
        align->align_value - left + len, value->buf);

or any similar variation which would give the same result.

>> This is a tangent, but I could easily see all of the code from 'if
>> (align->align_value < buf_len)' down to this point being placed in
>> utf8.c as a general alignment utility function. Doing so would make
>> this function shorter, and the patch easier to review overall (which
>> might be an important consideration -- especially given that I've
>> already spent several hours reviewing this one patch).
>
> That's a valid suggestion, will do that, thanks, but that'd mean we need to
> send an align struct to utf8.c which is only defined in ref-filter.h.
> Either this
> is fine or we need to move the definition of struct align to utf8.h.
> I think personally move the align structure and enum over to utf8.h

No, you don't need to move the 'struct align' to utf8.h. That
structure is specific to ref-filter and should stay there. Instead,
you only need to move the enum. For instance, you'd add something like
this to utf8.h:

    enum utf8_alignment {
        ALIGN_LEFT,
        ALIGN_MIDDLE,
        ALIGN_RIGHT
    };

    void strbuf_utf8_align(struct strbuf *buf,
        utf8_alignment where, int width, const char *s);

By the way, I forgot to say earlier that this should be done as a
separate patch (in order to make the current patch smaller).

That raises another question. Why are 'struct ref_formatting_state',
'struct align', 'struct atom_value', etc. defined in ref-filter.h at
all? Aren't those private implementation details of ref-filter.c, or
do you expect other code to be using them?

>>>         for (i = 0; i < final_buf.len; i++)
>>>                 printf("%c", final_buf.buf[i]);
>>>         putchar('\n');
>>> diff --git a/ref-filter.h b/ref-filter.h
>>> index 9e6c2d4..5575fe9 100644
>>> --- a/ref-filter.h
>>> +++ b/ref-filter.h
>>> @@ -16,14 +16,30 @@
>>>  struct ref_formatting_state {
>>> -       int quote_style;
>>>         struct strbuf *output;
>>> +       struct align *align;
>>> +       int quote_style;
>>
>> Perhaps decide where you'd like 'quote_style' to reside from the start
>> so that you don't have to add it at one location in its introductory
>> patch and then move it in a later patch. Also, why move it here?
>
> Cause that'd save memory on a 64 bit processor, where the pointers would
> be 8 bytes long and int would be 4 bytes long, this would bring in padding if
> int is placed before the pointers. Will change before hand.

As I understand it, you're not likely to have many
ref_fomratting_state's around at any given time, so this sounds like
premature memory micro-optimization.

Re: [PATCH v9 03/11] ref-filter: implement an `align` atom

[Karthik Nayak]

On Sun, Aug 9, 2015 at 9:12 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sat, Aug 8, 2015 at 2:35 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> On Fri, Aug 7, 2015 at 8:57 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>>> of the padding to be performed. If the atom length is more than the
>>>> padding length then no padding is performed. e.g. to pad a succeeding
>>>> atom to the middle with a total padding size of 40 we can do a
>>>
>>> It's odd to have alignment described in terms of "padding" and
>>> "padding length", especially in the case of "center" alignment. It
>>> might be better to rephrase the discussion in terms of field width or
>>> such.
>>>
>>>> --format="%(align:middle,40).."
>>
>> Ok this makes sense,
>> I'll rephrase as :
>>
>> `<width>` is the total length of the content with alignment.
>
> This doesn't really make sense. <width> isn't the content length; it's
> the size of the area into which the content will be placed.
>

Will change this.

>> If the atom length is more than the width then no alignment is performed.
>
> What "atom"? I think you mean the content between %(align:) and %(end)
> rather than "atom". The description might be clearer if you actually
> say "content between %(align:) and %(end)" to indicate specifically
> what is being aligned.

Yes, that's what I meant.

>
>> e.g. to align a succeeding atom to the middle with a total width of 40
>> we can do:
>> --format="%(align:middle,40).."
>>>> @@ -687,6 +690,29 @@ static void populate_value(struct ref_array_item *ref)
>>>>                         else
>>>>                                 v->s = " ";
>>>>                         continue;
>>>> +               } else if (starts_with(name, "align:")) {
>>>> +                       const char *valp = NULL;
>>>
>>> Unnecessary NULL assignment.
>>
>> Thats required for the second skip_prefix and so on.
>> Else we get: "warning: ‘valp’ may be used uninitialized in this
>> function [-Wmaybe-uninitialized]"
>
> Okay, so that's because skip_prefix() is inline, and it doesn't touch
> its "out" argument unless it actually skips the prefix. Ugly, but
> makes sense, although I think this issue would go away if you combined
> the starts_with() and skips_prefix() as suggested earlier.
>

Okay then I'll declare valp prehand to get rid of this and also to
remove redundant, starts_with() and skip_prefix().

>>>> +                       struct align *align = xmalloc(sizeof(struct align));
>>>> +
>>>> +                       skip_prefix(name, "align:", &valp);
>>>
>>> You could simplify the code by combining this skip_prefix() with the
>>> earlier starts_with() in the conditional:
>>>
>>>     } else if (skip_prefix(name, "align:", &valp)) {
>>>         struct align *align = xmalloc(sizeof(struct align));
>>>         ...
>>
>> That would require valp to be previously defined. Hence the split.
>
> Yes, it would require declaring 'valp' earlier, but that seems a
> reasonable tradeoff for cleaner, simpler, less redundant code.
>

Yes. will do this.

>>>>  static void apply_formatting_state(struct ref_formatting_state *state, struct strbuf *final)
>>>>  {
>>>> -       /* More formatting options to be evetually added */
>>>> +       if (state->align && state->end) {
>>>> +               struct strbuf *value = state->output;
>>>> +               int len = 0, buf_len = value->len;
>>>> +               struct align *align = state->align;
>>>> +
>>>> +               if (!value->buf)
>>>> +                       return;
>>>> +               if (!is_utf8(value->buf)) {
>>>> +                       len = value->len - utf8_strwidth(value->buf);
>>>
>>> What is this doing, exactly? If the string is *not* utf-8, then you're
>>> asking it for its utf-8 width. Am I reading that correctly?
>>>
>>> Also, what is 'len' supposed to represent? I guess you want it to be
>>> the difference between the byte length and the display length, but the
>>> name 'len' doesn't convey that at all, nor does it help the reader
>>> understand the code below where you do the actual formatting.
>>>
>>> In fact, if I'm reading this correctly, then 'len' is always zero in
>>> your tests (because the tests never trigger this conditional), so this
>>> functionality is never being exercised.
>>
>> There shouldn't be a "!" there, will change.
>> I guess 'utf8_compensation' would be a better name.
>
> Definitely better than 'len'.
>
>>>> +               else if (align->align_type == ALIGN_MIDDLE) {
>>>> +                       int right = (align->align_value - buf_len)/2;
>>>> +                       strbuf_addf(final, "%*s%-*s", align->align_value - right + len,
>>>> +                                   value->buf, right, "");
>>>
>>> An aesthetic aside: When (align_value - buf_len) is an odd number,
>>> this implementation favors placing more whitespace to the left of the
>>> string, and less to the right. In practice, this often tends to look a
>>> bit more awkward than the inverse of placing more whitespace to the
>>> right, and less to the left (but that again is subjective).
>>
>> I know that, maybe we could add an additional padding to even out the value
>> given?
>
> I don't understand your question. I was merely suggesting (purely
> subjectively), for the "odd length" case, putting the extra space
> after the centered text rather than before it. For instance:
>
>     int left = (align->align_value - buf_len) / 2;
>     strbuf_addf(final, "%*s%-*s", left, "",
>         align->align_value - left + len, value->buf);
>
> or any similar variation which would give the same result.
>

I get this could be done, what I was asking was, Consider given a alignment
width of 25 would be better to make that 26 so that we have even padding on
both sides. But I don't like the adding of manipulating user given data.

>>> This is a tangent, but I could easily see all of the code from 'if
>>> (align->align_value < buf_len)' down to this point being placed in
>>> utf8.c as a general alignment utility function. Doing so would make
>>> this function shorter, and the patch easier to review overall (which
>>> might be an important consideration -- especially given that I've
>>> already spent several hours reviewing this one patch).
>>
>> That's a valid suggestion, will do that, thanks, but that'd mean we need to
>> send an align struct to utf8.c which is only defined in ref-filter.h.
>> Either this
>> is fine or we need to move the definition of struct align to utf8.h.
>> I think personally move the align structure and enum over to utf8.h
>
> No, you don't need to move the 'struct align' to utf8.h. That
> structure is specific to ref-filter and should stay there. Instead,
> you only need to move the enum. For instance, you'd add something like
> this to utf8.h:
>
>     enum utf8_alignment {
>         ALIGN_LEFT,
>         ALIGN_MIDDLE,
>         ALIGN_RIGHT
>     };
>
>     void strbuf_utf8_align(struct strbuf *buf,
>         utf8_alignment where, int width, const char *s);
>

Okay will do this.

> By the way, I forgot to say earlier that this should be done as a
> separate patch (in order to make the current patch smaller).
>

Of course, that was obvious :)

> That raises another question. Why are 'struct ref_formatting_state',
> 'struct align', 'struct atom_value', etc. defined in ref-filter.h at
> all? Aren't those private implementation details of ref-filter.c, or
> do you expect other code to be using them?
>

I guess struct ref_formatting_state and struct align could be moved to
ref-filter.c. About struct atom_value its referenced by ref_array_item()
so any reader reading about this, would find it easier if atom_value()
is at the same place.

>>>>         for (i = 0; i < final_buf.len; i++)
>>>>                 printf("%c", final_buf.buf[i]);
>>>>         putchar('\n');
>>>> diff --git a/ref-filter.h b/ref-filter.h
>>>> index 9e6c2d4..5575fe9 100644
>>>> --- a/ref-filter.h
>>>> +++ b/ref-filter.h
>>>> @@ -16,14 +16,30 @@
>>>>  struct ref_formatting_state {
>>>> -       int quote_style;
>>>>         struct strbuf *output;
>>>> +       struct align *align;
>>>> +       int quote_style;
>>>
>>> Perhaps decide where you'd like 'quote_style' to reside from the start
>>> so that you don't have to add it at one location in its introductory
>>> patch and then move it in a later patch. Also, why move it here?
>>
>> Cause that'd save memory on a 64 bit processor, where the pointers would
>> be 8 bytes long and int would be 4 bytes long, this would bring in padding if
>> int is placed before the pointers. Will change before hand.
>
> As I understand it, you're not likely to have many
> ref_fomratting_state's around at any given time, so this sounds like
> premature memory micro-optimization.

Agreed, its a micro-optimization, but why leave it out when we only need
to re-structure code? I'll probably change it beforehand.

-- 
Regards,
Karthik Nayak

Re: [PATCH v9 03/11] ref-filter: implement an `align` atom

[Matthieu Moy]

Le 8 août 2015 09:03:03 GMT+02:00, Karthik Nayak <karthik.188@gmail.com> a écrit :
>On Fri, Aug 7, 2015 at 9:34 AM, Eric Sunshine <sunshine@sunshineco.com>
>wrote:
>> On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak <karthik.188@gmail.com>
>wrote:
>>> Implement an `align` atom which will act as a modifier atom and
>align
>>> any string with or without an %(atom) appearing before a %(end) atom
>>> to the right, left or middle.
>>>
>>> It is followed by `:<type>,<paddinglength>`, where the `<type>` is
>>> either left, right or middle and `<paddinglength>` is the total
>length
>>> of the padding to be performed. If the atom length is more than the
>>> padding length then no padding is performed. e.g. to pad a
>succeeding
>>> atom to the middle with a total padding size of 40 we can do a
>>> --format="%(align:middle,40).."
>>>
>>> Add documentation and tests for the same.
>>
>> I forgot to mention in my earlier review of this patch that you
>should
>> explain in the commit message, and probably the documentation, this
>> this implementation (assuming I'm understanding the code) does not
>> correctly support nested %(foo)...%(end) constructs, where %(foo)
>> might be %(if:), %(truncate:), %(cut:), or even a nested %(align:),
>or
>> some as yet unimagined modifier. Supporting nesting of these
>> constructs will require pushing the formatting states onto a stack
>(or
>> invoking the parser recursively).
>>
>>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>
>Good point, I have been working on this parallely and it works for now,
>I'll send that with the %(if) and %(end) feature. But for now, it
>should be
>documented and added in the commit message.
>
>Using a linked list of sorts where whenever a new modifier atom is
>encountered
>a new state is created, and once %(end) is encountered we can pop that
>state
>into the previous state.

Good, but keep in mind that this is not needed to port tag/branch, and your GSoC end soon. So keep your priorities in mind... IMHO, the nestable implementation can wait. 

Cheers,

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v9 03/11] ref-filter: implement an `align` atom

[Eric Sunshine]

On Sun, Aug 9, 2015 at 2:55 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Sun, Aug 9, 2015 at 9:12 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sat, Aug 8, 2015 at 2:35 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>> On Fri, Aug 7, 2015 at 8:57 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>>> On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>>>> +               else if (align->align_type == ALIGN_MIDDLE) {
>>>>> +                       int right = (align->align_value - buf_len)/2;
>>>>> +                       strbuf_addf(final, "%*s%-*s", align->align_value - right + len,
>>>>> +                                   value->buf, right, "");
>>>>
>>>> An aesthetic aside: When (align_value - buf_len) is an odd number,
>>>> this implementation favors placing more whitespace to the left of the
>>>> string, and less to the right. In practice, this often tends to look a
>>>> bit more awkward than the inverse of placing more whitespace to the
>>>> right, and less to the left (but that again is subjective).
>>>
>>> I know that, maybe we could add an additional padding to even out the value
>>> given?
>>
>> I don't understand your question. I was merely suggesting (purely
>> subjectively), for the "odd length" case, putting the extra space
>> after the centered text rather than before it. For instance:
>>
>>     int left = (align->align_value - buf_len) / 2;
>>     strbuf_addf(final, "%*s%-*s", left, "",
>>         align->align_value - left + len, value->buf);
>>
>> or any similar variation which would give the same result.
>
> I get this could be done, what I was asking was, Consider given a alignment
> width of 25 would be better to make that 26 so that we have even padding on
> both sides. But I don't like the adding of manipulating user given data.

I thought you might be asking that, but wasn't certain. I do agree
with your conclusion that second-guessing the user is a bad idea, and
that you should give the user exactly what was requested.

>> That raises another question. Why are 'struct ref_formatting_state',
>> 'struct align', 'struct atom_value', etc. defined in ref-filter.h at
>> all? Aren't those private implementation details of ref-filter.c, or
>> do you expect other code to be using them?
>
> I guess struct ref_formatting_state and struct align could be moved to
> ref-filter.c. About struct atom_value its referenced by ref_array_item()
> so any reader reading about this, would find it easier if atom_value()
> is at the same place.

Do you expect callers ever to be manipulating or otherwise accessing
the atom_value of ref_array_item? If callers have no business mucking
with atom_value, then one option would be to simply forward declare
atom_value in the header:

    struct atom_value;

    struct ref_array_item {
        ...
        struct atom_value *value;
        ...
    };

which makes atom_value opaque to clients of ref-filter. The actual
declaration of atom_value would then be moved to ref-filter.c, thus
kept private.

Re: [PATCH v9 03/11] ref-filter: implement an `align` atom

[Karthik Nayak]

On Sun, Aug 9, 2015 at 1:34 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sun, Aug 9, 2015 at 2:55 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> On Sun, Aug 9, 2015 at 9:12 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Sat, Aug 8, 2015 at 2:35 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>>> On Fri, Aug 7, 2015 at 8:57 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>>>> On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>>>>> +               else if (align->align_type == ALIGN_MIDDLE) {
>>>>>> +                       int right = (align->align_value - buf_len)/2;
>>>>>> +                       strbuf_addf(final, "%*s%-*s", align->align_value - right + len,
>>>>>> +                                   value->buf, right, "");
>>>>>
>>>>> An aesthetic aside: When (align_value - buf_len) is an odd number,
>>>>> this implementation favors placing more whitespace to the left of the
>>>>> string, and less to the right. In practice, this often tends to look a
>>>>> bit more awkward than the inverse of placing more whitespace to the
>>>>> right, and less to the left (but that again is subjective).
>>>>
>>>> I know that, maybe we could add an additional padding to even out the value
>>>> given?
>>>
>>> I don't understand your question. I was merely suggesting (purely
>>> subjectively), for the "odd length" case, putting the extra space
>>> after the centered text rather than before it. For instance:
>>>
>>>     int left = (align->align_value - buf_len) / 2;
>>>     strbuf_addf(final, "%*s%-*s", left, "",
>>>         align->align_value - left + len, value->buf);
>>>
>>> or any similar variation which would give the same result.
>>
>> I get this could be done, what I was asking was, Consider given a alignment
>> width of 25 would be better to make that 26 so that we have even padding on
>> both sides. But I don't like the adding of manipulating user given data.
>
> I thought you might be asking that, but wasn't certain. I do agree
> with your conclusion that second-guessing the user is a bad idea, and
> that you should give the user exactly what was requested.
>

In that case I'll be doing what you suggested, thanks :)

>>> That raises another question. Why are 'struct ref_formatting_state',
>>> 'struct align', 'struct atom_value', etc. defined in ref-filter.h at
>>> all? Aren't those private implementation details of ref-filter.c, or
>>> do you expect other code to be using them?
>>
>> I guess struct ref_formatting_state and struct align could be moved to
>> ref-filter.c. About struct atom_value its referenced by ref_array_item()
>> so any reader reading about this, would find it easier if atom_value()
>> is at the same place.
>
> Do you expect callers ever to be manipulating or otherwise accessing
> the atom_value of ref_array_item? If callers have no business mucking
> with atom_value, then one option would be to simply forward declare
> atom_value in the header:
>
>     struct atom_value;
>
>     struct ref_array_item {
>         ...
>         struct atom_value *value;
>         ...
>     };
>
> which makes atom_value opaque to clients of ref-filter. The actual
> declaration of atom_value would then be moved to ref-filter.c, thus
> kept private.

Also the code that this was done in has been excepted into `next`
so either I send a new series for the same, or write a patch just to
move this from ref-filter.h to ref-filter.c. So what would you suggest?

-- 
Regards,
Karthik Nayak

Re: [PATCH v9 03/11] ref-filter: implement an `align` atom

[Karthik Nayak]

On Sun, Aug 9, 2015 at 1:30 PM, Matthieu Moy <matthieu.moy@imag.fr> wrote:
>
>
> Le 8 août 2015 09:03:03 GMT+02:00, Karthik Nayak <karthik.188@gmail.com> a écrit :
>>On Fri, Aug 7, 2015 at 9:34 AM, Eric Sunshine <sunshine@sunshineco.com>
>>wrote:
>>> On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak <karthik.188@gmail.com>
>>wrote:
>>>> Implement an `align` atom which will act as a modifier atom and
>>align
>>>> any string with or without an %(atom) appearing before a %(end) atom
>>>> to the right, left or middle.
>>>>
>>>> It is followed by `:<type>,<paddinglength>`, where the `<type>` is
>>>> either left, right or middle and `<paddinglength>` is the total
>>length
>>>> of the padding to be performed. If the atom length is more than the
>>>> padding length then no padding is performed. e.g. to pad a
>>succeeding
>>>> atom to the middle with a total padding size of 40 we can do a
>>>> --format="%(align:middle,40).."
>>>>
>>>> Add documentation and tests for the same.
>>>
>>> I forgot to mention in my earlier review of this patch that you
>>should
>>> explain in the commit message, and probably the documentation, this
>>> this implementation (assuming I'm understanding the code) does not
>>> correctly support nested %(foo)...%(end) constructs, where %(foo)
>>> might be %(if:), %(truncate:), %(cut:), or even a nested %(align:),
>>or
>>> some as yet unimagined modifier. Supporting nesting of these
>>> constructs will require pushing the formatting states onto a stack
>>(or
>>> invoking the parser recursively).
>>>
>>>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>>
>>Good point, I have been working on this parallely and it works for now,
>>I'll send that with the %(if) and %(end) feature. But for now, it
>>should be
>>documented and added in the commit message.
>>
>>Using a linked list of sorts where whenever a new modifier atom is
>>encountered
>>a new state is created, and once %(end) is encountered we can pop that
>>state
>>into the previous state.
>
> Good, but keep in mind that this is not needed to port tag/branch, and your GSoC end soon. So keep your priorities in mind... IMHO, the nestable implementation can wait.
>
> Cheers,
>

Agreed, but it was just something I had already worked on. Probably
will push that series after GSoC :)


-- 
Regards,
Karthik Nayak

Re: [PATCH v9 03/11] ref-filter: implement an `align` atom

[Eric Sunshine]

On Sun, Aug 9, 2015 at 4:09 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Sun, Aug 9, 2015 at 1:34 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sun, Aug 9, 2015 at 2:55 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>> On Sun, Aug 9, 2015 at 9:12 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>>> That raises another question. Why are 'struct ref_formatting_state',
>>>> 'struct align', 'struct atom_value', etc. defined in ref-filter.h at
>>>> all? Aren't those private implementation details of ref-filter.c, or
>>>> do you expect other code to be using them?
>>>
>>> I guess struct ref_formatting_state and struct align could be moved to
>>> ref-filter.c. About struct atom_value its referenced by ref_array_item()
>>> so any reader reading about this, would find it easier if atom_value()
>>> is at the same place.
>>
>> Do you expect callers ever to be manipulating or otherwise accessing
>> the atom_value of ref_array_item? If callers have no business mucking
>> with atom_value, then one option would be to simply forward declare
>> atom_value in the header:
>>
>>     struct atom_value;
>>
>>     struct ref_array_item {
>>         ...
>>         struct atom_value *value;
>>         ...
>>     };
>>
>> which makes atom_value opaque to clients of ref-filter. The actual
>> declaration of atom_value would then be moved to ref-filter.c, thus
>> kept private.
>
> Also the code that this was done in has been excepted into `next`
> so either I send a new series for the same, or write a patch just to
> move this from ref-filter.h to ref-filter.c. So what would you suggest?

To my eye, atom_value seems to encapsulate a bunch of state local to
and only meaningful to ref-filter's internal workings, so it doesn't
really belong in the public header. Assuming that you don't foresee
any callers ever needing to access the properties of atom_value, then
it might indeed be reasonable to introduce a patch which moves it from
the .h file to the .c file (while leaving only a forward declaration
in the .h file).

Re: [PATCH v9 03/11] ref-filter: implement an `align` atom

[Karthik Nayak]

On Sun, Aug 9, 2015 at 1:49 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sun, Aug 9, 2015 at 4:09 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> On Sun, Aug 9, 2015 at 1:34 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Sun, Aug 9, 2015 at 2:55 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>>> On Sun, Aug 9, 2015 at 9:12 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>>>> That raises another question. Why are 'struct ref_formatting_state',
>>>>> 'struct align', 'struct atom_value', etc. defined in ref-filter.h at
>>>>> all? Aren't those private implementation details of ref-filter.c, or
>>>>> do you expect other code to be using them?
>>>>
>>>> I guess struct ref_formatting_state and struct align could be moved to
>>>> ref-filter.c. About struct atom_value its referenced by ref_array_item()
>>>> so any reader reading about this, would find it easier if atom_value()
>>>> is at the same place.
>>>
>>> Do you expect callers ever to be manipulating or otherwise accessing
>>> the atom_value of ref_array_item? If callers have no business mucking
>>> with atom_value, then one option would be to simply forward declare
>>> atom_value in the header:
>>>
>>>     struct atom_value;
>>>
>>>     struct ref_array_item {
>>>         ...
>>>         struct atom_value *value;
>>>         ...
>>>     };
>>>
>>> which makes atom_value opaque to clients of ref-filter. The actual
>>> declaration of atom_value would then be moved to ref-filter.c, thus
>>> kept private.
>>
>> Also the code that this was done in has been excepted into `next`
>> so either I send a new series for the same, or write a patch just to
>> move this from ref-filter.h to ref-filter.c. So what would you suggest?
>
> To my eye, atom_value seems to encapsulate a bunch of state local to
> and only meaningful to ref-filter's internal workings, so it doesn't
> really belong in the public header. Assuming that you don't foresee
> any callers ever needing to access the properties of atom_value, then
> it might indeed be reasonable to introduce a patch which moves it from
> the .h file to the .c file (while leaving only a forward declaration
> in the .h file).

Thanks! will add it to the series.

-- 
Regards,
Karthik Nayak

[PATCH v9 03/11] ref-filter: implement an `align` atom

[Karthik Nayak]

Implement an `align` atom which will act as a modifier atom and align
any string with or without an %(atom) appearing before a %(end) atom
to the right, left or middle.

It is followed by `:<type>,<paddinglength>`, where the `<type>` is
either left, right or middle and `<paddinglength>` is the total length
of the padding to be performed. If the atom length is more than the
padding length then no padding is performed. e.g. to pad a succeeding
atom to the middle with a total padding size of 40 we can do a
--format="%(align:middle,40).."

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                       | 84 +++++++++++++++++++++++++++++++++++---
 ref-filter.h                       | 14 +++++++
 t/t6302-for-each-ref-filter.sh     | 48 ++++++++++++++++++++++
 4 files changed, 148 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index e49d578..d865f98 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::
+	Align any string with or without %(atom) before the %(end)
+	atom to the right, left or middle. Followed by
+	`:<type>,<paddinglength>`, where the `<type>` is either left,
+	right or middle and `<paddinglength>` is the total length of
+	the padding to be performed. If the string length is more than
+	the padding length then no padding 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 2c074a1..d123299 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,6 +54,8 @@ static struct {
 	{ "flag" },
 	{ "HEAD" },
 	{ "color" },
+	{ "align" },
+	{ "end" },
 };
 
 /*
@@ -620,7 +623,7 @@ static void populate_value(struct ref_array_item *ref)
 		const char *name = used_atom[i];
 		struct atom_value *v = &ref->value[i];
 		int deref = 0;
-		const char *refname;
+		const char *refname = NULL;
 		const char *formatp;
 		struct branch *branch = NULL;
 
@@ -687,6 +690,29 @@ static void populate_value(struct ref_array_item *ref)
 			else
 				v->s = " ";
 			continue;
+		} else if (starts_with(name, "align:")) {
+			const char *valp = NULL;
+			struct align *align = xmalloc(sizeof(struct align));
+
+			skip_prefix(name, "align:", &valp);
+
+			if (skip_prefix(valp, "left,", &valp))
+				align->align_type = ALIGN_LEFT;
+			else if (skip_prefix(valp, "right,", &valp))
+				align->align_type = ALIGN_RIGHT;
+			else if (skip_prefix(valp, "middle,", &valp))
+				align->align_type = ALIGN_MIDDLE;
+			else
+				die(_("align: improper format"));
+			if (strtoul_ui(valp, 10, &align->align_value))
+				die(_("align: positive value expected"));
+			v->align = align;
+			v->modifier_atom = 1;
+			continue;
+		} else if (starts_with(name, "end")) {
+			v->end = 1;
+			v->modifier_atom = 1;
+			continue;
 		} else
 			continue;
 
@@ -1251,12 +1277,48 @@ static void emit(const char *cp, const char *ep, struct ref_formatting_state *st
 
 static void process_formatting_state(struct atom_value *atomv, struct ref_formatting_state *state)
 {
-	/* Based on the atomv values, the formatting state is set */
+	if (atomv->align) {
+		state->align = atomv->align;
+		atomv->align = NULL;
+	}
+	if (atomv->end)
+		state->end = 1;
 }
 
 static void apply_formatting_state(struct ref_formatting_state *state, struct strbuf *final)
 {
-	/* More formatting options to be evetually added */
+	if (state->align && state->end) {
+		struct strbuf *value = state->output;
+		int len = 0, buf_len = value->len;
+		struct align *align = state->align;
+
+		if (!value->buf)
+			return;
+		if (!is_utf8(value->buf)) {
+			len = value->len - utf8_strwidth(value->buf);
+			buf_len -= len;
+		}
+
+		if (align->align_value < buf_len) {
+			state->align = NULL;
+			strbuf_addbuf(final, value);
+			strbuf_release(value);
+			return;
+		}
+
+		if (align->align_type == ALIGN_LEFT)
+			strbuf_addf(final, "%-*s", len + align->align_value, value->buf);
+		else if (align->align_type == ALIGN_MIDDLE) {
+			int right = (align->align_value - buf_len)/2;
+			strbuf_addf(final, "%*s%-*s", align->align_value - right + len,
+				    value->buf, right, "");
+		} else if (align->align_type == ALIGN_RIGHT)
+			strbuf_addf(final, "%*s", align->align_value, value->buf);
+		strbuf_release(value);
+		state->align = NULL;
+		return;
+	} else if (state->align)
+		return;
 	strbuf_addbuf(final, state->output);
 	strbuf_release(state->output);
 }
@@ -1282,10 +1344,19 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
 			apply_formatting_state(&state, &final_buf);
 		}
 		get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
-		process_formatting_state(atomv, &state);
-		print_value(atomv, &state);
-		apply_formatting_state(&state, &final_buf);
+		if (atomv->modifier_atom)
+			process_formatting_state(atomv, &state);
+		else {
+			print_value(atomv, &state);
+			apply_formatting_state(&state, &final_buf);
+		}
+		if (atomv->end) {
+			print_value(atomv, &state);
+			apply_formatting_state(&state, &final_buf);
+		}
 	}
+	if (state.align)
+		die(_("format: align used without an `end` atom"));
 	if (*cp) {
 		sp = cp + strlen(cp);
 		emit(cp, sp, &state);
@@ -1301,6 +1372,7 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
 		print_value(&resetv, &state);
 		apply_formatting_state(&state, &final_buf);
 	}
+
 	for (i = 0; i < final_buf.len; i++)
 		printf("%c", final_buf.buf[i]);
 	putchar('\n');
diff --git a/ref-filter.h b/ref-filter.h
index 9e6c2d4..81de1e2 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -16,14 +16,28 @@
 #define FILTER_REFS_INCLUDE_BROKEN 0x1
 #define FILTER_REFS_ALL 0x2
 
+#define ALIGN_LEFT 0x01
+#define ALIGN_RIGHT 0x02
+#define ALIGN_MIDDLE 0x04
+
 struct ref_formatting_state {
 	int quote_style;
 	struct strbuf *output;
+	struct align *align;
+	unsigned int end : 1;
+};
+
+struct align {
+	unsigned int align_type,
+		align_value;
 };
 
 struct atom_value {
 	const char *s;
+	struct align *align;
 	unsigned long ul; /* used for sorting when not FIELD_STR */
+	unsigned int modifier_atom : 1,
+		end : 1;
 };
 
 struct ref_sorting {
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 505a360..76041a2 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:left,30)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:middle,30)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:right,30)refname is %(refname)%(end)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.5.0