Re: [PATCH 1/3] trailer: add a trailer.trimEmpty config option

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 10, 2015, 21:58 UTC
Message-ID: <xmqqsiedv37o.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CAP8UFD1pWH5yaJaJ_gV1t5q5qfgs58AtcSr+ZqDTnWzfHK1uBw@mail.gmail.com>

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

Show 25 quoted lines

> I think that very few new features are now needed to make it possible
> to use the code in other commands like commit, format-patch, am, etc,
> but this patch implements one of the needed features.
>
>>     - do "trailer stuff" by calling a central helper that does
>>       "trailer stuff" a pointer to the middle, trailers, struct.
>>
>>       - when the trailer becomes a reusable subsystem, this central
>>         helper will become the primary entry point to the API.
>>
>>       - "trailer stuff" will include adding new ones, removing
>>         existing ones, and rewriting lines.  What you do in the
>>         current process_command_line_args() and
>>         process_trailers_lists() [*1*] would come here.
>>
>>     - write out the result, given the outermost struct.  This will
>>       become another entry point in the API.
>>
>> Structured that way, callers will supply that outermost structure,
>> and can trust that the trailers subsystem will not molest
>> message_proper or lines_after_trailers part.
>
> I don't think it is a big improvement because it is easy to see that
> the current code doesn't molest the part before and after the
> trailers.

You force the callers that want only the "trailer" thing to happen to:

 - pass first and last around.

 - keep each line of the message body in separate strbuf and have
   it in the same array as the trailers

Neither of which is necessary. I recall that during the review of the previous rounds your own code had to work this around by first concatenating lines (each of which are unnecessarily in separate "struct strbuf"s) into a single strbuf, only to call a helper that takes a single strbuf to count what to ignore in it, and then iterate over the array of strbuf to add up the lengths of them, which would have been unnecessary if the underlying data structure were saner.