Re: [PATCH v2 1/3] revision: add a per-email field to rev-info

From: "Kristoffer Haugsbakk" <code@khaugsbakk.name>
To: "Jeff King" <peff@peff.net>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v2 1/3] revision: add a per-email field to rev-info
Date: Tue, 19 Mar 2024 22:41:28 +0100	[thread overview]
Message-ID: <9cf0dfba-355b-4670-baee-a30d53976832@app.fastmail.com> (raw)
In-Reply-To: <20240319212940.GE1159535@coredump.intra.peff.net>

On Tue, Mar 19, 2024, at 22:29, Jeff King wrote:
> On Tue, Mar 19, 2024 at 07:35:36PM +0100, Kristoffer Haugsbakk wrote:
>
>> Add `pe_header` to `rev_info` to store per-email headers.
>
> It is only just now that I realized that "pe" stands for per-email
> (though to be fair I was not really focused on the intent of the series
> when reading v1). Can we just call it per_email_headers or something?

For sure.

>> The next commit will add an option to `format-patch` which will allow
>> the user to store headers per-email; a complement to options like
>> `--add-header`.
>>
>> To make this possible we need a new field to store these headers. We
>> also need to take ownership of `extra_headers_p` in
>> `log_write_email_headers`; facilitate this by removing constness from
>> the relevant pointers.
>
> There are three pointers at play here:
>
>   - ctx.after_subject has its const removed, since it will now always be
>     allocated by log_write_email_headers(), and then freed by the
>     caller. Makes sense Though it looks like we only free in show_log(),
>     and the free in make_cover_letter() is not added until patch 2?
>
>   - rev_info.extra_headers has its const removed here, but I don't think
>     that is helping anything. We only use it to write into the "headers"
>     strbuf in log_write_email_headers(), which always returns
>     headers.buf (or NULL).
>
>   - rev.pe_headers is introduced as non-const because it is allocated
>     and freed for each email. That makes some sense, though if we
>     followed the pattern of rev.extra_headers, then the pointer is
>     conceptually "const" within the rev_info struct, and it is the
>     caller who keeps track of the allocation (using a to_free variable).
>     Possibly we should do the same here?
>
> I do still think this could be split in a more obvious way, leaving the
> pe_headers bits until they are actually needed. Let me see if I can
> sketch it up.

Nice :)