Re: [PATCH 2/4] trailer: avoid unnecessary splitting on lines

[Junio C Hamano <gitster@pobox.com>]

Christian Couder <christian.couder@gmail.com> writes:

> On Sat, Oct 29, 2016 at 2:05 AM, Jonathan Tan <jonathantanmy@google.com> wrote:
>> trailer.c currently splits lines while processing a buffer (and also
>> rejoins lines when needing to invoke ignore_non_trailer).
>>
>> Avoid such line splitting, except when generating the strings
>> corresponding to trailers (for ease of use by clients - a subsequent
>> patch will allow other components to obtain the layout of a trailer
>> block in a buffer, including the trailers themselves). The main purpose
>> of this is to make it easy to return pointers into the original buffer
>> (for a subsequent patch), but this also significantly reduces the number
>> of memory allocations required.
>>
>> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
>> ---
>>  trailer.c | 215 +++++++++++++++++++++++++++++++++-----------------------------
>>  1 file changed, 116 insertions(+), 99 deletions(-)
>
> IMHO it is telling that this needs 17 more lines.

Given that among which 13 are additional comments to explain what is
going on, I think this is a surprisingly good outcome, relative to
what the patch achieves (reducing a lot of split/rejoin operations
and need for allocation associated with them that are all made
unnecessary).

I agree with you that it is telling to see that this needs only 17
more lines (or 4, if you only count the true code additions) and
clearly shows that strbuf_split() helper function was a not good
match for the codepath touched by this patch.  The helper function
is very tempting in that with a simple single call it will give us
tons of strbufs in an array, each of which are _MUCH_ more flexible
than simple strings if we ever need to manipulate them by trimming,
splicing and inserting etc.  This patch shows us that if we do not
need the flexibility, doing strbuf_split() as the first thing on the
input and having to deal with an array of strbuf is an overall loss,
I would think.

Although I admit that I carefully read only up to [2/4] I am fairly
happy with this series.  It finally fixes the second of the two
issues I pointed out in the review of the original series (the other
one was fixed in the series that this topic depends on), paying down
technical debt.

>> @@ -954,7 +971,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
>>  {
>>         LIST_HEAD(head);
>>         LIST_HEAD(arg_head);
>> -       struct strbuf **lines;
>> +       struct strbuf sb = STRBUF_INIT;
>
> We often use "sb" as the name of strbuf variables, but I think at
> least here (and maybe in other places above) we could use something a
> bit more telling, like "input_buf" perhaps.

That sounds sensible.

>
>>         int trailer_end;
>>         FILE *outfile = stdout;
>>