On Thursday, November 9th, 2023 at 19:35, Jeff King <peff@xxxxxxxx> wrote:
> 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? Grepping for other
> code that does the same thing, I see that show_log() and
> cmd_format_patch() copy a lot more. (For that matter, why doesn't
> make_cover_letter() just use the context set up by
> cmd_format_patch()?).
>
> 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. So could/should we just be using
> pp->rev->encode_email_headers here?
>
> 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()?
>
> I don't think that answering those questions needs to hold up your
> patch. We can take it as a quick fix for a real bug, and then anybody
> interested can dig further as a separate topic on top.
These are good questions indeed. Unfortunately I don't hink I'll have time to
work on this though.
> > +test_expect_success 'cover letter with --cover-from-description subject (UTF-8 subject line)' '
> > + test_config branch.rebuild-1.description "Café?
> > +
> > +body" &&
> > + git checkout rebuild-1 &&
> > + git format-patch --stdout --cover-letter --cover-from-description subject --encode-email-headers main >actual &&
> > + grep "^Subject: \[PATCH 0/2\] =?UTF-8?q?Caf=C3=A9=3F?=$" actual &&
> > + ! grep "Café" actual
> > +'
>
> The test looks correct to me.
>
> Some of these long lines (and the in-string newlines!) make this ugly
> and hard to read. But it is also just copying the already-ugly style of
> nearby tests. So I'm OK with that. But a prettier version might be:
>
> test_expect_success 'cover letter respects --encode-email-headers' '
> test_config branch.rebuild-1.description "Café?" &&
> git checkout rebuild-1 &&
> git format-patch --stdout --encode-email-headers \
> --cover-letter --cover-from-description=subject \
> main >actual &&
> ...
> '
Yeah, that sounds better indeed. Let me know if you want me to resend a cleaner
version of the test.
> I also wondered if we could be just be testing this much more easily
> with another header like "--to". But I guess that would be found in both
> the cover letter and the actual patches (we also don't seem to encode
> it even in the regular patches; is that a bug?).
That sounds like another bug indeed… But maybe that'll be harder to fix. To
Q-encode this field one needs to split off the full name and actual mail
address ("André <andre@xxxxxxxxxxx>" would be split into "André" and
"andre@xxxxxxxxxxx"), then Q-encode the full name, then join the strings
together again. In particular, it's incorrect to Q-encode the full string.
