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 :)
