Re: [PATCH 5/6] format-patch: return an allocated string from log_write_email_headers()

["Kristoffer Haugsbakk" <code@khaugsbakk.name>]

On Wed, Mar 20, 2024, at 01:35, Jeff King wrote:
> When pretty-printing a commit in the email format, we have to fill in
> the "after subject" field of the pretty_print_context with any extra
> headers the user provided (e.g., from "--to" or "--cc" options) plus any
> special MIME headers.
>
> We return an out-pointer that sometimes points to a newly heap-allocated
> string and sometimes not. To avoid leaking, we store the allocated
> version in a buffer with static lifetime, which is ugly. Worse, as we
> extend the header feature, we'll end up having to repeat this ugly
> pattern.
>
> Instead, let's have our out-pointer pass ownership back to the caller,
> and duplicate the string when necessary. This does mean one extra
> allocation per commit when you use extra headers, but in the context of
> format-patch which is showing diffs, I don't think that's even
> measurable.
>
> Signed-off-by: Jeff King <peff@peff.net>

Good presentation of motivation here.

> ---
> I don't think the extra allocation is a big deal, but if we do, there
> are some other options:
>
>   - instead of an out-pointer we could take a strbuf, and the caller
>     could reset and reuse a strbuf for each commit
>
>   - the after_subject stuff could become a callback; we discussed this a
>     long time ago (I had no recollection of the thread until finding it
>     in the archive just now):
>
>
> https://lore.kernel.org/git/20170325211149.yyvocmdfw4zbjyoi@sigill.intra.peff.net/
>
>   - this log_write_email_headers() function prints part of its output to
>     stdout, and shoves part of it into the after_subject field to be
>     shown by the pretty-printer. I wonder if it could just format the
>     subject itself (though that would make "rev-list --format=email"
>     even more awkward, I guess).

I don’t quite understand all of these alternatives but the first one
makes sense. Leave the responsibility to the caller. That could work.

-- 
Kristoffer Haugsbakk