v11, 13 partsref-filter: implement an `align` atom

> 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

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

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

> +       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 = ',';

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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