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

From: Jeff King <peff@peff.net>
To: Kristoffer Haugsbakk <code@khaugsbakk.name>
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 20:25:55 -0400	[thread overview]
Message-ID: <20240320002555.GB903718@coredump.intra.peff.net> (raw)
In-Reply-To: <20240319212940.GE1159535@coredump.intra.peff.net>

On Tue, Mar 19, 2024 at 05:29:40PM -0400, Jeff King wrote:

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

OK, this rabbit hole went much deeper than I expected. ;)

I see why you wanted to drop the const from rev_info.extra_headers here.
We need the local extra_headers variable in show_log() to be non-const
(since it receives the output of log_write_email_headers). But we also
assign rev_info.extra_headers to that variable, and if it is const, the
compiler will complain.

But as it turns out, that assignment is not really necessary at all! It
is only used when you have extra headers along with a non-email format.
In most cases we simply ignore the headers for those formats, and in the
one case where we do respect them, I think it is doing the wrong thing.

So here are some patches which clean things up. They would make a
suitable base for your changes, I think, but IMHO they also stand on
their own as cleanups.

Having now stared at this code for a bit, I do think there's another,
much simpler option for your series: keep the same ugly static-strbuf
allocation pattern in log_write_email_headers(), but extend it further.
I'll show that in a moment, too.

  [1/6]: shortlog: stop setting pp.print_email_subject
  [2/6]: pretty: split oneline and email subject printing
  [3/6]: pretty: drop print_email_subject flag
  [4/6]: log: do not set up extra_headers for non-email formats
  [5/6]: format-patch: return an allocated string from log_write_email_headers()
  [6/6]: format-patch: simplify after-subject MIME header handling

 builtin/log.c      |  4 ++--
 builtin/rev-list.c |  1 +
 builtin/shortlog.c |  1 -
 log-tree.c         | 22 +++++++++-------------
 log-tree.h         |  2 +-
 pretty.c           | 43 ++++++++++++++++++++-----------------------
 pretty.h           | 11 +++++------
 7 files changed, 38 insertions(+), 46 deletions(-)

-Peff