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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Sep 5, 2015 at 2:52 PM, Karthik Nayak <karthik.188@xxxxxxxxx> 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)".
>
> 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@xxxxxxxxx>
> ---
> 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.

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

> +                        */
> +                       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.)

> +                       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;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]