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

From: Jeff King <peff@peff.net>
Date: Mar 19, 2024, 21:29 UTC
Message-ID: <20240319212940.GE1159535@coredump.intra.peff.net>
In-Reply-To: <9a7102b708e4afe78447e48e4baf5b6d66ca50d1.1710873210.git.code@khaugsbakk.name>

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?

Show 8 quoted lines

> 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.

-Peff