Am 09.11.23 um 19:35 schrieb Jeff King:

On Thu, Nov 09, 2023 at 11:19:56AM +0000, Simon Ser wrote:

quoted

When writing the cover letter, the encode_email_headers option was
ignored. That is, UTF-8 subject lines and email addresses were
written out as-is, without any Q-encoding, even if
--encode-email-headers was passed on the command line.

This is due to encode_email_headers not being copied over from
struct rev_info to struct pretty_print_context. Fix that and add
a test.

That makes sense, and your patch looks the right thing to do as an
immediate fix. But I have to wonder:

  1. Are there other bits that need to be copied?

Good question.

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

(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?

  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.

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.

     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?

René