git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Simon Ser <contact@emersion.fr>
Date
Nov 10, 2023, 10:36 UTC
Message-ID
<VTz8XT3MCqWUh1HFQon62NxmGJiYFfNmBeWTtR8MwmeuaSkovfBJ02P-S79GsD94XwlxlrL6W80YZ8OwfpDd1BqA0E4GwFQlDKN5DWq0Qtg=@emersion.fr>
In-Reply-To
<20231109183506.GB2711684@coredump.intra.peff.net>
On Thursday, November 9th, 2023 at 19:35, Jeff King <peff@peff.net> wrote:
Show 25 quoted lines
> 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.

Show 24 quoted lines
> > +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@example.org>" would be split into "André" and "andre@example.org"), then Q-encode the full name, then join the strings together again. In particular, it's incorrect to Q-encode the full string.

Previous: Jeff KingNext: Jeff King
Message 3 of 6 in “format-patch: fix ignored encode_email_headers for cover letter”
  1. format-patch: fix ignored encode_email_headers for cover letterSimon Ser, Nov 9, 2023
  2. Jeff KingNov 9, 2023
  3. Simon SerNov 10, 2023
  4. Jeff KingNov 10, 2023
  5. René ScharfeNov 12, 2023
  6. Jeff KingNov 13, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.