Re: [PATCH] format-patch: fix ignored encode_email_headers for cover letter

From: Jeff King <peff@peff.net>
Date: Nov 13, 2023, 19:00 UTC
Message-ID: <20231113190027.GC3838361@coredump.intra.peff.net>
In-Reply-To: <0c0d685c-29e5-462c-a743-4573a56d7e04@web.de>

On Sun, Nov 12, 2023 at 07:38:22PM +0100, René Scharfe wrote:

Show 9 quoted lines

> > Grepping for other
> >      code that does the same thing, I see that show_log() and
> >      cmd_format_patch() copy a lot more.
> 
> show_log() copies almost half of the struct 6d167fd7cc members
> from struct rev_info!
> 
> cmd_format_patch() doesn't seem have a struct pretty_print_context,
> though...

Doh, you're right. I grepped for spots setting encode_email_headers, but the one in cmd_format_patch() is copying it from the config-default into the rev_info, not into the pretty-print context.

Which makes sense. It is going to call show_log() to show each commit, which is where the value is copied into the pretty-print context.

Show 6 quoted lines

> > (For that matter, why doesn't
> >      make_cover_letter() just use the context set up by
> >      cmd_format_patch()?).
> 
> ... which answers this question, but did you perhaps mean a different
> function?

Right, I was just confused.

Show 13 quoted lines

> >   2. Why are we copying this stuff at all? When we introduced the
> >      pretty-print context back in 6bf139440c (clean up calling
> >      conventions for pretty.c functions, 2011-05-26), the idea was just
> >      to keep all of the format options together. But later, 6d167fd7cc
> >      (pretty: use fmt_output_email_subject(), 2017-03-01) added a
> >      pointer to the rev_info directly.
> 
> Hmm.  Was sticking the rev_info pointer in unwise?  Does it tangle up
> things that should be kept separate?  It uses force_in_body_from,
> grep_filter, sources, nr, total and subject_prefix from struct rev_info.
> That seems a lot, but is just a small fraction of its total members and
> we could just copy those we need.  Or prepare the subject string and
> pass it in, as before 6d167fd7cc.

I don't know that it was unwise. I was mostly just noting the history because that explains why we _didn't_ simply refer to ctx->revs in 6bf139440c. Has the separation between the two been valuable since then? I'm not sure. We do have some code paths that do not have a rev_info at all (e.g., pp_commit_easy(), which makes an ad-hoc empty context struct).

Show 7 quoted lines

> > So could/should we just be using
> >      pp->rev->encode_email_headers here?
> 
> Perhaps.  If we want struct pretty_print_context to be a collection of
> named parameters, though, then it makes sense to avoid using
> complicated types to provide a nice interface to its callers, and
> struct rev_info is one of our largest structs we have.

Yeah, philosophically it may be better to keep the modules separated. But if we end up having to just copy a ton of fields, it may not be as practical. If we can at least factor that out into a single spot, though, it may not be so bad.

Show 9 quoted lines

> >      Or if that field is not always set (or we want to override some
> >      elements), should there be a single helper function to initialize
> >      the pretty_print_context from a rev_info, that could be shared
> >      between spots like show_log() and make_cover_letter()?
> 
> That would help avoid forgetting to copy something.  But copying is
> questionable in general, as you mentioned.  Given the extent of the
> overlap, would it make sense to embed struct pretty_print_context in
> struct rev_info instead?  Or a subset thereof?

I had a similar thought, but the pretty_print_context carries both input options from the caller, as well as computed state used internally by the pretty-print code. So you'd have to split those two up, I would think. And now all of the pretty-print functions have to pass around _two_ contexts.

That's more annoying, but arguably is a cleaner design (the internal struct can be a private thing that is not even defined outside of pretty.c). I dunno.

-Peff