Re: [PATCH v16 06/14] ref-filter: implement an `align` atom

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sep 6, 2015, 20:10 UTC
Message-ID: <CAPig+cS28v552BZ63PTKrchYc4M-GYMhhEyfcHXW89vqvYhcOQ@mail.gmail.com>
In-Reply-To: <1441479135-5285-7-git-send-email-Karthik.188@gmail.com>

On Sat, Sep 5, 2015 at 2:52 PM, Karthik Nayak <karthik.188@gmail.com> wrote:

Show 36 quoted lines

> 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)".
>
> We introduce an `at_end` function for each element of the stack which
> is to be called when the `end` atom is encountered. Using this we
> implement end_align_handler() for the `align` atom, this aligns the
> final strbuf by calling `strbuf_utf8_align()` from utf8.c.
>
> Ensure that quote formatting is performed on the whole of
> %(align:...)...%(end) rather than individual atoms inside. We skip
> quote formatting for individual atoms when the current stack element
> is handling an %(align:...) atom and perform quote formatting at the
> end when we encounter the %(end) atom of the second element of then
> stack.
>
> 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..b23412d 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -127,6 +127,16 @@ color::
> +align::
> +       Left-, middle-, or right-align the content between %(align:...)
> +       and %(end). Followed by `:<width>,<position>`, where the
> +       `<position>` is either left, right or middle and `<width>` is
> +       the total length of the content with alignment. If the

This should mention that <position> is optional and default to "left" if omitted.

Show 18 quoted lines

> +       contents length is more than the width then no alignment is
> +       performed. If used with '--quote' everything in between
> +       %(align:...) and %(end) is quoted, but if nested then only the
> +       topmost level performs quoting.
> diff --git a/ref-filter.c b/ref-filter.c
> index e99c342..6c9ef08 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -773,6 +837,50 @@ static void populate_value(struct ref_array_item *ref)
>                         else
>                                 v->s = " ";
>                         continue;
> +               } else if (match_atom_name(name, "align", &valp)) {
> +                       struct align *align = &v->u.align;
> +                       struct strbuf **s;
> +
> +                       if (!valp)
> +                               die(_("expected format: %%(align:<width>, <position>)"));

I'm pretty sure this parsing code won't deal well with a space after the comma, so the space should be dropped from the diagnostic message advising the user of the correct format: s/, /,/

> +                       /*
> +                        * TODO: Implement a function similar to strbuf_split_str()
> +                        * which would strip the terminator at the end.

"...which would omit the separator from the end of each value."

Show 8 quoted lines

> +                        */
> +                       s = strbuf_split_str(valp, ',', 0);
> +
> +                       /* If the position is given trim the ',' from the first strbuf */
> +                       if (s[1])
> +                               strbuf_setlen(s[0], s[0]->len - 1);
> +                       if (s[2])
> +                               die(_("align:<width>,<position> followed by garbage: %s"), s[2]->buf);

If <position> is omitted, strbuf_split_str("42", ...) will return the array ["42", NULL] which won't have an element [2], which means this code will access beyond end-of-array. (This is another good argument for looping over s[] as Junio suggested rather than assuming these fixed yet optional positions. It can be hard to get it right.)

Show 27 quoted lines

> +                       if (strtoul_ui(s[0]->buf, 10, &align->width))
> +                               die(_("positive width expected align:%s"), s[0]->buf);
> +
> +                       /*
> +                        * TODO: Implement a more general check, so that the values
> +                        * do not always have to be in a specific order.
> +                        */
> +                       if (!s[1])
> +                               align->position = ALIGN_LEFT;
> +                       else if (!strcmp(s[1]->buf, "left"))
> +                               align->position = ALIGN_LEFT;
> +                       else if (!strcmp(s[1]->buf, "right"))
> +                               align->position = ALIGN_RIGHT;
> +                       else if (!strcmp(s[1]->buf, "middle"))
> +                               align->position = ALIGN_MIDDLE;
> +                       else
> +                               die(_("improper format entered align:%s"), s[1]->buf);
> +
> +                       strbuf_list_free(s);
> +
> +                       v->handler = align_atom_handler;
> +                       continue;
> +               } else if (!strcmp(name, "end")) {
> +                       v->handler = end_atom_handler;
> +                       continue;
>                 } else
>                         continue;