On Mon, Sep 7, 2015 at 1:40 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> 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.
>
Will add that.
>> + 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/, /,/
>
Will change.
>> + /*
>> + * 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."
>
Will change.
>> + */
>> + 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.)
You're right, probably something on the lines of what Junio suggested here
http://article.gmane.org/gmane.comp.version-control.git/277041
--
Regards,
Karthik Nayak
--
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