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

["René Scharfe" <l.s.r@web.de>]

Am 09.11.23 um 19:35 schrieb Jeff King:
> On Thu, Nov 09, 2023 at 11:19:56AM +0000, Simon Ser wrote:
>
>> 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é