{"thread":{"id":"60498","subject":"[PATCH] format-patch: fix ignored encode_email_headers for cover letter","startedAt":"2023-11-09T11:20:15Z","lastAt":"2023-11-13T19:00:30Z","messageCount":6,"participants":["Simon Ser","Jeff King","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"484641","messageId":"20231109111950.387219-1-contact@emersion.fr","threadId":"60498","inReplyTo":null,"subject":"[PATCH] format-patch: fix ignored encode_email_headers for cover letter","fromName":"Simon Ser","fromEmail":"contact@emersion.fr","sentAt":"2023-11-09T11:19:56Z","receivedAt":"2023-11-09T11:20:15Z","isPatch":true,"sender":{"key":"contact@emersion.fr","avatar":"https://avatars.githubusercontent.com/u/506932?v=4"},"body":"When writing the cover letter, the encode_email_headers option was\nignored. That is, UTF-8 subject lines and email addresses were\nwritten out as-is, without any Q-encoding, even if\n--encode-email-headers was passed on the command line.\n\nThis is due to encode_email_headers not being copied over from\nstruct rev_info to struct pretty_print_context. Fix that and add\na test.\n\nSigned-off-by: Simon Ser <contact@emersion.fr>\n---\n builtin/log.c           |  1 +\n t/t4014-format-patch.sh | 10 ++++++++++\n 2 files changed, 11 insertions(+)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex ba775d7b5cf8..87fd1c8560de 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1364,6 +1364,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,\n \tpp.date_mode.type = DATE_RFC2822;\n \tpp.rev = rev;\n \tpp.print_email_subject = 1;\n+\tpp.encode_email_headers = rev->encode_email_headers;\n \tpp_user_info(&pp, NULL, &sb, committer, encoding);\n \tprepare_cover_text(&pp, description_file, branch_name, &sb,\n \t\t\t   encoding, need_8bit_cte);\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 5ced27ed4571..e37a1411ee24 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1906,6 +1906,16 @@ body\" &&\n \tgrep \"^body$\" actual\n '\n \n+test_expect_success 'cover letter with --cover-from-description subject (UTF-8 subject line)' '\n+\ttest_config branch.rebuild-1.description \"Café?\n+\n+body\" &&\n+\tgit checkout rebuild-1 &&\n+\tgit format-patch --stdout --cover-letter --cover-from-description subject --encode-email-headers main >actual &&\n+\tgrep \"^Subject: \\[PATCH 0/2\\] =?UTF-8?q?Caf=C3=A9=3F?=$\" actual &&\n+\t! grep \"Café\" actual\n+'\n+\n test_expect_success 'cover letter with format.coverFromDescription = auto (short subject line)' '\n \ttest_config branch.rebuild-1.description \"config subject\n \n-- \n2.42.0\n\n\n"},{"id":"484653","messageId":"20231109183506.GB2711684@coredump.intra.peff.net","threadId":"60498","inReplyTo":"20231109111950.387219-1-contact@emersion.fr","subject":"Re: [PATCH] format-patch: fix ignored encode_email_headers for cover letter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-09T18:35:06Z","receivedAt":"2023-11-09T18:35:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 09, 2023 at 11:19:56AM +0000, Simon Ser wrote:\n\n> When writing the cover letter, the encode_email_headers option was\n> ignored. That is, UTF-8 subject lines and email addresses were\n> written out as-is, without any Q-encoding, even if\n> --encode-email-headers was passed on the command line.\n> \n> This is due to encode_email_headers not being copied over from\n> struct rev_info to struct pretty_print_context. Fix that and add\n> a test.\n\nThat makes sense, and your patch looks the right thing to do as an\nimmediate fix. But I have to wonder:\n\n  1. Are there other bits that need to be copied? Grepping for other\n     code that does the same thing, I see that show_log() and\n     cmd_format_patch() copy a lot more. (For that matter, why doesn't\n     make_cover_letter() just use the context set up by\n     cmd_format_patch()?).\n\n  2. Why are we copying this stuff at all? When we introduced the\n     pretty-print context back in 6bf139440c (clean up calling\n     conventions for pretty.c functions, 2011-05-26), the idea was just\n     to keep all of the format options together. But later, 6d167fd7cc\n     (pretty: use fmt_output_email_subject(), 2017-03-01) added a\n     pointer to the rev_info directly. So could/should we just be using\n     pp->rev->encode_email_headers here?\n\n     Or if that field is not always set (or we want to override some\n     elements), should there be a single helper function to initialize\n     the pretty_print_context from a rev_info, that could be shared\n     between spots like show_log() and make_cover_letter()?\n\nI don't think that answering those questions needs to hold up your\npatch. We can take it as a quick fix for a real bug, and then anybody\ninterested can dig further as a separate topic on top.\n\n> diff --git a/builtin/log.c b/builtin/log.c\n> index ba775d7b5cf8..87fd1c8560de 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -1364,6 +1364,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,\n>  \tpp.date_mode.type = DATE_RFC2822;\n>  \tpp.rev = rev;\n>  \tpp.print_email_subject = 1;\n> +\tpp.encode_email_headers = rev->encode_email_headers;\n>  \tpp_user_info(&pp, NULL, &sb, committer, encoding);\n>  \tprepare_cover_text(&pp, description_file, branch_name, &sb,\n>  \t\t\t   encoding, need_8bit_cte);\n\nThis part looks obviously good.\n\n> +test_expect_success 'cover letter with --cover-from-description subject (UTF-8 subject line)' '\n> +\ttest_config branch.rebuild-1.description \"Café?\n> +\n> +body\" &&\n> +\tgit checkout rebuild-1 &&\n> +\tgit format-patch --stdout --cover-letter --cover-from-description subject --encode-email-headers main >actual &&\n> +\tgrep \"^Subject: \\[PATCH 0/2\\] =?UTF-8?q?Caf=C3=A9=3F?=$\" actual &&\n> +\t! grep \"Café\" actual\n> +'\n\nThe test looks correct to me.\n\nSome of these long lines (and the in-string newlines!) make this ugly\nand hard to read. But it is also just copying the already-ugly style of\nnearby tests. So I'm OK with that. But a prettier version might be:\n\n  test_expect_success 'cover letter respects --encode-email-headers' '\n        test_config branch.rebuild-1.description \"Café?\" &&\n        git checkout rebuild-1 &&\n        git format-patch --stdout --encode-email-headers \\\n                --cover-letter --cover-from-description=subject \\\n                main >actual &&\n        ...\n  '\n\nI also wondered if we could be just be testing this much more easily\nwith another header like \"--to\". But I guess that would be found in both\nthe cover letter and the actual patches (we also don't seem to encode\nit even in the regular patches; is that a bug?).\n\n-Peff\n"},{"id":"484707","messageId":"VTz8XT3MCqWUh1HFQon62NxmGJiYFfNmBeWTtR8MwmeuaSkovfBJ02P-S79GsD94XwlxlrL6W80YZ8OwfpDd1BqA0E4GwFQlDKN5DWq0Qtg=@emersion.fr","threadId":"60498","inReplyTo":"20231109183506.GB2711684@coredump.intra.peff.net","subject":"Re: [PATCH] format-patch: fix ignored encode_email_headers for cover letter","fromName":"Simon Ser","fromEmail":"contact@emersion.fr","sentAt":"2023-11-10T10:36:22Z","receivedAt":"2023-11-10T10:36:50Z","isPatch":true,"sender":{"key":"contact@emersion.fr","avatar":"https://avatars.githubusercontent.com/u/506932?v=4"},"body":"On Thursday, November 9th, 2023 at 19:35, Jeff King <peff@peff.net> wrote:\n\n> That makes sense, and your patch looks the right thing to do as an\n> immediate fix. But I have to wonder:\n> \n>   1. Are there other bits that need to be copied? Grepping for other\n>      code that does the same thing, I see that show_log() and\n>      cmd_format_patch() copy a lot more. (For that matter, why doesn't\n>      make_cover_letter() just use the context set up by\n>      cmd_format_patch()?).\n> \n>   2. Why are we copying this stuff at all? When we introduced the\n>      pretty-print context back in 6bf139440c (clean up calling\n>      conventions for pretty.c functions, 2011-05-26), the idea was just\n>      to keep all of the format options together. But later, 6d167fd7cc\n>      (pretty: use fmt_output_email_subject(), 2017-03-01) added a\n>      pointer to the rev_info directly. So could/should we just be using\n>      pp->rev->encode_email_headers here?\n> \n>      Or if that field is not always set (or we want to override some\n>      elements), should there be a single helper function to initialize\n>      the pretty_print_context from a rev_info, that could be shared\n>      between spots like show_log() and make_cover_letter()?\n> \n> I don't think that answering those questions needs to hold up your\n> patch. We can take it as a quick fix for a real bug, and then anybody\n> interested can dig further as a separate topic on top.\n\nThese are good questions indeed. Unfortunately I don't hink I'll have time to\nwork on this though.\n\n> > +test_expect_success 'cover letter with --cover-from-description subject (UTF-8 subject line)' '\n> > +\ttest_config branch.rebuild-1.description \"Café?\n> > +\n> > +body\" &&\n> > +\tgit checkout rebuild-1 &&\n> > +\tgit format-patch --stdout --cover-letter --cover-from-description subject --encode-email-headers main >actual &&\n> > +\tgrep \"^Subject: \\[PATCH 0/2\\] =?UTF-8?q?Caf=C3=A9=3F?=$\" actual &&\n> > +\t! grep \"Café\" actual\n> > +'\n> \n> The test looks correct to me.\n> \n> Some of these long lines (and the in-string newlines!) make this ugly\n> and hard to read. But it is also just copying the already-ugly style of\n> nearby tests. So I'm OK with that. But a prettier version might be:\n> \n>   test_expect_success 'cover letter respects --encode-email-headers' '\n>         test_config branch.rebuild-1.description \"Café?\" &&\n>         git checkout rebuild-1 &&\n>         git format-patch --stdout --encode-email-headers \\\n>                 --cover-letter --cover-from-description=subject \\\n>                 main >actual &&\n>         ...\n>   '\n\nYeah, that sounds better indeed. Let me know if you want me to resend a cleaner\nversion of the test.\n\n> I also wondered if we could be just be testing this much more easily\n> with another header like \"--to\". But I guess that would be found in both\n> the cover letter and the actual patches (we also don't seem to encode\n> it even in the regular patches; is that a bug?).\n\nThat sounds like another bug indeed… But maybe that'll be harder to fix. To\nQ-encode this field one needs to split off the full name and actual mail\naddress (\"André <andre@example.org>\" would be split into \"André\" and\n\"andre@example.org\"), then Q-encode the full name, then join the strings\ntogether again. In particular, it's incorrect to Q-encode the full string.\n"},{"id":"484718","messageId":"20231110214859.GE2758295@coredump.intra.peff.net","threadId":"60498","inReplyTo":"VTz8XT3MCqWUh1HFQon62NxmGJiYFfNmBeWTtR8MwmeuaSkovfBJ02P-S79GsD94XwlxlrL6W80YZ8OwfpDd1BqA0E4GwFQlDKN5DWq0Qtg=@emersion.fr","subject":"Re: [PATCH] format-patch: fix ignored encode_email_headers for cover letter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-10T21:48:59Z","receivedAt":"2023-11-10T21:49:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 10, 2023 at 10:36:22AM +0000, Simon Ser wrote:\n\n> > I don't think that answering those questions needs to hold up your\n> > patch. We can take it as a quick fix for a real bug, and then anybody\n> > interested can dig further as a separate topic on top.\n> \n> These are good questions indeed. Unfortunately I don't hink I'll have time to\n> work on this though.\n\nThat's OK. I think it's fine to stop here for now.\n\n> > Some of these long lines (and the in-string newlines!) make this ugly\n> > and hard to read. But it is also just copying the already-ugly style of\n> > nearby tests. So I'm OK with that. But a prettier version might be:\n> > \n> >   test_expect_success 'cover letter respects --encode-email-headers' '\n> >         test_config branch.rebuild-1.description \"Café?\" &&\n> >         git checkout rebuild-1 &&\n> >         git format-patch --stdout --encode-email-headers \\\n> >                 --cover-letter --cover-from-description=subject \\\n> >                 main >actual &&\n> >         ...\n> >   '\n> \n> Yeah, that sounds better indeed. Let me know if you want me to resend a cleaner\n> version of the test.\n\nI don't have a strong opinion, so I'd leave it up to you.\n\n> > I also wondered if we could be just be testing this much more easily\n> > with another header like \"--to\". But I guess that would be found in both\n> > the cover letter and the actual patches (we also don't seem to encode\n> > it even in the regular patches; is that a bug?).\n> \n> That sounds like another bug indeed… But maybe that'll be harder to fix. To\n> Q-encode this field one needs to split off the full name and actual mail\n> address (\"André <andre@example.org>\" would be split into \"André\" and\n> \"andre@example.org\"), then Q-encode the full name, then join the strings\n> together again. In particular, it's incorrect to Q-encode the full string.\n\nYeah, without even looking at the code, I had a suspicion that this\nwould be an issue. I doubt that format-patch is doing much parsing at\nall of what we feed it via --to.\n\n-Peff\n"},{"id":"484776","messageId":"0c0d685c-29e5-462c-a743-4573a56d7e04@web.de","threadId":"60498","inReplyTo":"20231109183506.GB2711684@coredump.intra.peff.net","subject":"Re: [PATCH] format-patch: fix ignored encode_email_headers for cover letter","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-11-12T18:38:22Z","receivedAt":"2023-11-12T18:38:44Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.11.23 um 19:35 schrieb Jeff King:\n> On Thu, Nov 09, 2023 at 11:19:56AM +0000, Simon Ser wrote:\n>\n>> When writing the cover letter, the encode_email_headers option was\n>> ignored. That is, UTF-8 subject lines and email addresses were\n>> written out as-is, without any Q-encoding, even if\n>> --encode-email-headers was passed on the command line.\n>>\n>> This is due to encode_email_headers not being copied over from\n>> struct rev_info to struct pretty_print_context. Fix that and add\n>> a test.\n>\n> That makes sense, and your patch looks the right thing to do as an\n> immediate fix. But I have to wonder:\n>\n>   1. Are there other bits that need to be copied?\n\nGood question.\n\n> Grepping for other\n>      code that does the same thing, I see that show_log() and\n>      cmd_format_patch() copy a lot more.\n\nshow_log() copies almost half of the struct 6d167fd7cc members\nfrom struct rev_info!\n\ncmd_format_patch() doesn't seem have a struct pretty_print_context,\nthough...\n\n> (For that matter, why doesn't\n>      make_cover_letter() just use the context set up by\n>      cmd_format_patch()?).\n\n... which answers this question, but did you perhaps mean a different\nfunction?\n\n>   2. Why are we copying this stuff at all? When we introduced the\n>      pretty-print context back in 6bf139440c (clean up calling\n>      conventions for pretty.c functions, 2011-05-26), the idea was just\n>      to keep all of the format options together. But later, 6d167fd7cc\n>      (pretty: use fmt_output_email_subject(), 2017-03-01) added a\n>      pointer to the rev_info directly.\n\nHmm.  Was sticking the rev_info pointer in unwise?  Does it tangle up\nthings that should be kept separate?  It uses force_in_body_from,\ngrep_filter, sources, nr, total and subject_prefix from struct rev_info.\nThat seems a lot, but is just a small fraction of its total members and\nwe could just copy those we need.  Or prepare the subject string and\npass it in, as before 6d167fd7cc.\n\n> So could/should we just be using\n>      pp->rev->encode_email_headers here?\n\nPerhaps.  If we want struct pretty_print_context to be a collection of\nnamed parameters, though, then it makes sense to avoid using\ncomplicated types to provide a nice interface to its callers, and\nstruct rev_info is one of our largest structs we have.\n\n>      Or if that field is not always set (or we want to override some\n>      elements), should there be a single helper function to initialize\n>      the pretty_print_context from a rev_info, that could be shared\n>      between spots like show_log() and make_cover_letter()?\n\nThat would help avoid forgetting to copy something.  But copying is\nquestionable in general, as you mentioned.  Given the extent of the\noverlap, would it make sense to embed struct pretty_print_context in\nstruct rev_info instead?  Or a subset thereof?\n\nRené\n"},{"id":"484811","messageId":"20231113190027.GC3838361@coredump.intra.peff.net","threadId":"60498","inReplyTo":"0c0d685c-29e5-462c-a743-4573a56d7e04@web.de","subject":"Re: [PATCH] format-patch: fix ignored encode_email_headers for cover letter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-13T19:00:27Z","receivedAt":"2023-11-13T19:00:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 12, 2023 at 07:38:22PM +0100, René Scharfe wrote:\n\n> > Grepping for other\n> >      code that does the same thing, I see that show_log() and\n> >      cmd_format_patch() copy a lot more.\n> \n> show_log() copies almost half of the struct 6d167fd7cc members\n> from struct rev_info!\n> \n> cmd_format_patch() doesn't seem have a struct pretty_print_context,\n> though...\n\nDoh, you're right. I grepped for spots setting encode_email_headers, but\nthe one in cmd_format_patch() is copying it from the config-default into\nthe rev_info, not into the pretty-print context.\n\nWhich makes sense. It is going to call show_log() to show each commit,\nwhich is where the value is copied into the pretty-print context.\n\n> > (For that matter, why doesn't\n> >      make_cover_letter() just use the context set up by\n> >      cmd_format_patch()?).\n> \n> ... which answers this question, but did you perhaps mean a different\n> function?\n\nRight, I was just confused.\n\n> >   2. Why are we copying this stuff at all? When we introduced the\n> >      pretty-print context back in 6bf139440c (clean up calling\n> >      conventions for pretty.c functions, 2011-05-26), the idea was just\n> >      to keep all of the format options together. But later, 6d167fd7cc\n> >      (pretty: use fmt_output_email_subject(), 2017-03-01) added a\n> >      pointer to the rev_info directly.\n> \n> Hmm.  Was sticking the rev_info pointer in unwise?  Does it tangle up\n> things that should be kept separate?  It uses force_in_body_from,\n> grep_filter, sources, nr, total and subject_prefix from struct rev_info.\n> That seems a lot, but is just a small fraction of its total members and\n> we could just copy those we need.  Or prepare the subject string and\n> pass it in, as before 6d167fd7cc.\n\nI don't know that it was unwise. I was mostly just noting the history\nbecause that explains why we _didn't_ simply refer to ctx->revs in\n6bf139440c. Has the separation between the two been valuable since then?\nI'm not sure. We do have some code paths that do not have a rev_info at\nall (e.g., pp_commit_easy(), which makes an ad-hoc empty context\nstruct).\n\n> > So could/should we just be using\n> >      pp->rev->encode_email_headers here?\n> \n> Perhaps.  If we want struct pretty_print_context to be a collection of\n> named parameters, though, then it makes sense to avoid using\n> complicated types to provide a nice interface to its callers, and\n> struct rev_info is one of our largest structs we have.\n\nYeah, philosophically it may be better to keep the modules separated.\nBut if we end up having to just copy a ton of fields, it may not be as\npractical. If we can at least factor that out into a single spot,\nthough, it may not be so bad.\n\n> >      Or if that field is not always set (or we want to override some\n> >      elements), should there be a single helper function to initialize\n> >      the pretty_print_context from a rev_info, that could be shared\n> >      between spots like show_log() and make_cover_letter()?\n> \n> That would help avoid forgetting to copy something.  But copying is\n> questionable in general, as you mentioned.  Given the extent of the\n> overlap, would it make sense to embed struct pretty_print_context in\n> struct rev_info instead?  Or a subset thereof?\n\nI had a similar thought, but the pretty_print_context carries both input\noptions from the caller, as well as computed state used internally by\nthe pretty-print code. So you'd have to split those two up, I would\nthink. And now all of the pretty-print functions have to pass around\n_two_ contexts.\n\nThat's more annoying, but arguably is a cleaner design (the internal\nstruct can be a private thing that is not even defined outside of\npretty.c). I dunno.\n\n-Peff\n"}]}