Re: [PATCH v12 05/13] ref-filter: implement an `align` atom

[Karthik Nayak <karthik.188@gmail.com>]

On Fri, Aug 21, 2015 at 1:53 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Aug 18, 2015 at 2:37 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 now have a `handler()` for each atom_value which will be called
>> when that atom_value is being parsed, and similarly an `at_end`
>> function for each state which is to be called when the `end` atom is
>> encountered. Using this implement the `align` atom which aligns the
>> given strbuf by calling `strbuf_utf8_align()` from utf8.c
>>
>> This is done by assigning a 'handler' function to each atom_value and
>> a related 'at_end' function for each state. The align_handler()
>> defined uses strbuf_utf8_align() from utf8.c to perform the alignment.
>>
>> 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 74532d3..ecbcc5a 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -626,6 +638,36 @@ static inline char *copy_advance(char *dst, const char *src)
>>         return dst;
>>  }
>>
>> +static void align_handler(struct ref_formatting_state *state)
>> +{
>> +       struct align *align = (struct align *)state->cb_data;
>> +       struct strbuf s = STRBUF_INIT;
>> +
>> +       strbuf_utf8_align(&s, align->position, align->width, state->output.buf);
>> +       strbuf_reset(&state->output);
>> +       strbuf_addbuf(&state->output, &s);
>> +       free(align);
>> +}
>
> Leaking 'strbuf s' here.
>
> Also, this operation can be expressed more concisely as:
>
>     strbuf_utf8_align(&s, ...);
>     strbuf_swap(&state->output, &s);
>     strbuf_release(&s);
>

This seems neater.

> Seeing this is also making me question my earlier suggestion of making
> pop_state() responsible for appending the current state's output to
> the previous state's output. The reason is that if align_handler() is
> responsible for appending to the previous state's output, then all the
> above string handling collapses to the one line:
>
>     strbuf_utf8_align(&state->prev->output, ..., state->output.buf);
>
> which is even simpler, and doesn't involve a temporary strbuf or
> swapping of strbuf contents.
>
> On the other hand, it won't always be the case that all handlers can
> get by with such simple code, and they might end up creating temporary
> strbuf(s) and such anyhow, so I don't feel too strongly about it, and
> it can always be changed at a later date (by someone) if that approach
> ends up being better.

I did try to tell the same on your previous suggestion, maybe I wasn't
clear enough.
But I see pros and cons for both, so with only one modifier atom at the moment,
It's hard to fixate on one method. Maybe we could change it 'if needed' when we
introduce new atoms eventually.

-- 
Regards,
Karthik Nayak