Re: [PATCH 5/6] format-patch: return an allocated string from log_write_email_headers()
- From
Kristoffer Haugsbakk <code@khaugsbakk.name>
- Date
- Mar 22, 2024, 22:06 UTC
- Message-ID
- <c94e0ba2-87b9-4272-afce-67f5faf0275f@app.fastmail.com>
- In-Reply-To
- <20240320003533.GE904136@coredump.intra.peff.net>
On Wed, Mar 20, 2024, at 01:35, Jeff King wrote:
Show 18 quoted lines
> 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.
Show 19 quoted lines
> --- > 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