From: Jeff King <peff@peff.net>
To: Simon Ser <contact@emersion.fr>
Cc: "René Scharfe" <l.s.r@web.de>,
git@vger.kernel.org, "Junio C Hamano" <gitster@pobox.com>
Subject: Re: [PATCH] format-patch: fix ignored encode_email_headers for cover letter
Date: Thu, 9 Nov 2023 13:35:06 -0500 [thread overview]
Message-ID: <20231109183506.GB2711684@coredump.intra.peff.net> (raw)
In-Reply-To: <20231109111950.387219-1-contact@emersion.fr>
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? 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.
> diff --git a/builtin/log.c b/builtin/log.c
> index ba775d7b5cf8..87fd1c8560de 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1364,6 +1364,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
> pp.date_mode.type = DATE_RFC2822;
> pp.rev = rev;
> pp.print_email_subject = 1;
> + pp.encode_email_headers = rev->encode_email_headers;
> pp_user_info(&pp, NULL, &sb, committer, encoding);
> prepare_cover_text(&pp, description_file, branch_name, &sb,
> encoding, need_8bit_cte);
This part looks obviously good.
> +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 &&
...
'
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?).
-Peff
next prev parent reply other threads:[~2023-11-09 18:35 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-09 11:19 [PATCH] format-patch: fix ignored encode_email_headers for cover letter Simon Ser
2023-11-09 18:35 ` Jeff King [this message]
2023-11-10 10:36 ` Simon Ser
2023-11-10 21:48 ` Jeff King
2023-11-12 18:38 ` René Scharfe
2023-11-13 19:00 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20231109183506.GB2711684@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=contact@emersion.fr \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=l.s.r@web.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.