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

> On Sat, Sep 5, 2015 at 2:52 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)".
>>
>> 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.
>

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