# Bug: format-patch MIME boundary not added to cover letter when attach enabled 8 messages from 2018-04-30 to 2018-05-02. Participants: Patrick Hemmer, brian m. carlson, Junio C Hamano, Eric Sunshine. Thread: https://gitlist.dev/t/48379 ## Patrick Hemmer, 2018-04-30 01:40 Subject: Bug: format-patch MIME boundary not added to cover letter when attach enabled Message-ID: <5e409069-835f-3c85-f55e-c27f534dc9e2@stormcloud9.net> URL: https://gitlist.dev/e/5e409069-835f-3c85-f55e-c27f534dc9e2%40stormcloud9.net ``` When you use `git format-patch --cover-letter --attach`, the cover letter does not have the trailing MIME boundary. RFC2046 states that the last part must be followed by a closing boundary. This causes some email clients (Thunderbird in my case) to discard the message body. This is experienced with git 2.16.3. For example: $ git format-patch --cover-letter --attach --root -o /tmp/out /tmp/out/0000-cover-letter.patch /tmp/out/0001-hello-world.patch $ cat /tmp/out/0000-cover-letter.patch From a25ac88e6216131e8b000335d32bb99d4e5185ac Mon Sep 17 00:00:00 2001 From: Patrick Hemmer Date: Sun, 29 Apr 2018 21:26:45 -0400 Subject: [PATCH 0/1] *** SUBJECT HERE *** MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="------------2.16.3" This is a multi-part message in MIME format. --------------2.16.3 Content-Type: text/plain; charset=UTF-8; format=fixed Content-Transfer-Encoding: 8bit *** BLURB HERE *** Patrick Hemmer (1): hello world -- 2.16.3 ``` ## brian m. carlson, 2018-04-30 02:09 Subject: Re: Bug: format-patch MIME boundary not added to cover letter when attach enabled Message-ID: <20180430020930.GA13217@genre.crustytoothpaste.net> URL: https://gitlist.dev/e/20180430020930.GA13217%40genre.crustytoothpaste.net In-Reply-To: <5e409069-835f-3c85-f55e-c27f534dc9e2@stormcloud9.net> ``` On Sun, Apr 29, 2018 at 09:40:13PM -0400, Patrick Hemmer wrote: > When you use `git format-patch --cover-letter --attach`, the cover > letter does not have the trailing MIME boundary. RFC2046 states that the > last part must be followed by a closing boundary. This causes some email > clients (Thunderbird in my case) to discard the message body. > This is experienced with git 2.16.3. Thanks for reporting this. I can confirm this with a reasonably recent next. Let me see if I can come up with a patch. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 ``` ## Junio C Hamano, 2018-04-30 03:30 Subject: Re: Bug: format-patch MIME boundary not added to cover letter when attach enabled Message-ID: URL: https://gitlist.dev/e/xmqqzi1lxte6.fsf%40gitster-ct.c.googlers.com In-Reply-To: <20180430020930.GA13217@genre.crustytoothpaste.net> ``` "brian m. carlson" writes: > On Sun, Apr 29, 2018 at 09:40:13PM -0400, Patrick Hemmer wrote: >> When you use `git format-patch --cover-letter --attach`, the cover >> letter does not have the trailing MIME boundary. RFC2046 states that the >> last part must be followed by a closing boundary. This causes some email >> clients (Thunderbird in my case) to discard the message body. >> This is experienced with git 2.16.3. > > Thanks for reporting this. I can confirm this with a reasonably recent > next. Let me see if I can come up with a patch. Thanks. It is true that the current output from the tool is corrupt mime multi-part, and we need to do something about it. I however have to wonder if it even makes sense for --cover to pay attention to --attach and produce the cover template that has "BLURB HERE" etc. in a multi-part format. Shouldn't we be making a simple plain text file instead? ``` ## brian m. carlson, 2018-04-30 11:59 Subject: Re: Bug: format-patch MIME boundary not added to cover letter when attach enabled Message-ID: <20180430115909.GB13217@genre.crustytoothpaste.net> URL: https://gitlist.dev/e/20180430115909.GB13217%40genre.crustytoothpaste.net In-Reply-To: ``` On Mon, Apr 30, 2018 at 12:30:57PM +0900, Junio C Hamano wrote: > Thanks. It is true that the current output from the tool is corrupt > mime multi-part, and we need to do something about it. > > I however have to wonder if it even makes sense for --cover to pay > attention to --attach and produce the cover template that has "BLURB > HERE" etc. in a multi-part format. Shouldn't we be making a simple > plain text file instead? I agree that multipart/mixed is not a useful content-type for only one plain text part. I have a patch to add the trailing boundary, but I think you make a good argument that perhaps omitting the entire multipart portion would be better. I'll have to work on this after work, so expect a patch later today. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 ``` ## brian m. carlson, 2018-05-01 00:02 Subject: [PATCH] format-patch: make cover letters always text/plain Message-ID: <20180501000214.833766-1-sandals@crustytoothpaste.net> URL: https://gitlist.dev/e/20180501000214.833766-1-sandals%40crustytoothpaste.net In-Reply-To: <20180430020930.GA13217@genre.crustytoothpaste.net> ``` When formatting a series of patches using --attach and --cover-letter, the cover letter lacks the closing MIME boundary, violating RFC 2046. Certain clients, such as Thunderbird, discard the message body in such a case. Since the cover letter is just one part and sending it as multipart/mixed is not very useful, always emit it as text/plain, avoiding the boundary problem altogether. Reported-by: Patrick Hemmer Signed-off-by: brian m. carlson --- builtin/log.c | 2 +- log-tree.c | 7 ++++--- log-tree.h | 3 ++- t/t4014-format-patch.sh | 9 +++++++++ 4 files changed, 16 insertions(+), 5 deletions(-) diff --git a/builtin/log.c b/builtin/log.c index 71f68a3e4f..24868ed070 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1019,7 +1019,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout, open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet)) return; - log_write_email_headers(rev, head, &pp.after_subject, &need_8bit_cte); + log_write_email_headers(rev, head, &pp.after_subject, &need_8bit_cte, 0); for (i = 0; !need_8bit_cte && i < nr; i++) { const char *buf = get_commit_buffer(list[i], NULL); diff --git a/log-tree.c b/log-tree.c index d1c0bedf24..9f5eb346a4 100644 --- a/log-tree.c +++ b/log-tree.c @@ -362,7 +362,8 @@ void fmt_output_email_subject(struct strbuf *sb, struct rev_info *opt) void log_write_email_headers(struct rev_info *opt, struct commit *commit, const char **extra_headers_p, - int *need_8bit_cte_p) + int *need_8bit_cte_p, + int maybe_multipart) { const char *extra_headers = opt->extra_headers; const char *name = oid_to_hex(opt->zero_commit ? @@ -385,7 +386,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit, opt->ref_message_ids->items[i].string); graph_show_oneline(opt->graph); } - if (opt->mime_boundary) { + if (opt->mime_boundary && maybe_multipart) { static char subject_buffer[1024]; static char buffer[1024]; struct strbuf filename = STRBUF_INIT; @@ -610,7 +611,7 @@ void show_log(struct rev_info *opt) if (cmit_fmt_is_mail(opt->commit_format)) { log_write_email_headers(opt, commit, &extra_headers, - &ctx.need_8bit_cte); + &ctx.need_8bit_cte, 1); ctx.rev = opt; ctx.print_email_subject = 1; } else if (opt->commit_format != CMIT_FMT_USERFORMAT) { diff --git a/log-tree.h b/log-tree.h index deba035187..e668628074 100644 --- a/log-tree.h +++ b/log-tree.h @@ -27,7 +27,8 @@ void format_decorations_extended(struct strbuf *sb, const struct commit *commit, void show_decorations(struct rev_info *opt, struct commit *commit); void log_write_email_headers(struct rev_info *opt, struct commit *commit, const char **extra_headers_p, - int *need_8bit_cte_p); + int *need_8bit_cte_p, + int maybe_multipart); void load_ref_decorations(struct decoration_filter *filter, int flags); #define FORMAT_PATCH_NAME_MAX 64 diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh index 482112ca33..83c596a842 100755 --- a/t/t4014-format-patch.sh +++ b/t/t4014-format-patch.sh @@ -1661,6 +1661,15 @@ test_expect_success 'format-patch --base with --attach' ' test_write_lines 1 2 >expect && test_cmp expect actual ' +test_expect_success 'format-patch --attach cover-letter only is non-multipart' ' + test_when_finished "rm -r patches" && + git format-patch -o patches --cover-letter --attach=mimemime --base=HEAD~ -1 && + ! egrep "^--+mimemime" patches/0000*.patch && + egrep "^--+mimemime$" patches/0001*.patch >output && + test_line_count = 2 output && + egrep "^--+mimemime--$" patches/0001*.patch >output && + test_line_count = 1 output +' test_expect_success 'format-patch --pretty=mboxrd' ' sp=" " && ``` ## Eric Sunshine, 2018-05-01 01:53 Subject: Re: [PATCH] format-patch: make cover letters always text/plain Message-ID: URL: https://gitlist.dev/e/CAPig%2BcRucJfzm8Z0XhPa0CMw%2BLiBdY_b5%3DbMz0LkezJxvC9qJA%40mail.gmail.com In-Reply-To: <20180501000214.833766-1-sandals@crustytoothpaste.net> ``` On Mon, Apr 30, 2018 at 8:02 PM, brian m. carlson wrote: > When formatting a series of patches using --attach and --cover-letter, > the cover letter lacks the closing MIME boundary, violating RFC 2046. > Certain clients, such as Thunderbird, discard the message body in such a > case. > > Since the cover letter is just one part and sending it as > multipart/mixed is not very useful, always emit it as text/plain, > avoiding the boundary problem altogether. > > Reported-by: Patrick Hemmer > Signed-off-by: brian m. carlson > --- > diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh > @@ -1661,6 +1661,15 @@ test_expect_success 'format-patch --base with --attach' ' > +test_expect_success 'format-patch --attach cover-letter only is non-multipart' ' > + test_when_finished "rm -r patches" && > + git format-patch -o patches --cover-letter --attach=mimemime --base=HEAD~ -1 && Nit: "rm -rf" would be a bit more robust against git-format-patch somehow crashing before creating the "patches" directory. > + ! egrep "^--+mimemime" patches/0000*.patch && > + egrep "^--+mimemime$" patches/0001*.patch >output && > + test_line_count = 2 output && > + egrep "^--+mimemime--$" patches/0001*.patch >output && > + test_line_count = 1 output > +' ``` ## brian m. carlson, 2018-05-02 00:16 Subject: Re: [PATCH] format-patch: make cover letters always text/plain Message-ID: <20180502001606.GI13217@genre.crustytoothpaste.net> URL: https://gitlist.dev/e/20180502001606.GI13217%40genre.crustytoothpaste.net In-Reply-To: ``` On Mon, Apr 30, 2018 at 09:53:33PM -0400, Eric Sunshine wrote: > On Mon, Apr 30, 2018 at 8:02 PM, brian m. carlson > wrote: > > When formatting a series of patches using --attach and --cover-letter, > > the cover letter lacks the closing MIME boundary, violating RFC 2046. > > Certain clients, such as Thunderbird, discard the message body in such a > > case. > > > > Since the cover letter is just one part and sending it as > > multipart/mixed is not very useful, always emit it as text/plain, > > avoiding the boundary problem altogether. > > > > Reported-by: Patrick Hemmer > > Signed-off-by: brian m. carlson > > --- > > diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh > > @@ -1661,6 +1661,15 @@ test_expect_success 'format-patch --base with --attach' ' > > +test_expect_success 'format-patch --attach cover-letter only is non-multipart' ' > > + test_when_finished "rm -r patches" && > > + git format-patch -o patches --cover-letter --attach=mimemime --base=HEAD~ -1 && > > Nit: "rm -rf" would be a bit more robust against git-format-patch > somehow crashing before creating the "patches" directory. Sure, I can reroll with that change. I had considered doing that, but decided against it. I hadn't thought of resilience against a failed git format-patch, though. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 ``` ## brian m. carlson, 2018-05-02 02:20 Subject: [PATCH v2] format-patch: make cover letters always text/plain Message-ID: <20180502022052.939511-1-sandals@crustytoothpaste.net> URL: https://gitlist.dev/e/20180502022052.939511-1-sandals%40crustytoothpaste.net In-Reply-To: <20180501000214.833766-1-sandals@crustytoothpaste.net> ``` When formatting a series of patches using --attach and --cover-letter, the cover letter lacks the closing MIME boundary, violating RFC 2046. Certain clients, such as Thunderbird, discard the message body in such a case. Since the cover letter is just one part and sending it as multipart/mixed is not very useful, always emit it as text/plain, avoiding the boundary problem altogether. Reported-by: Patrick Hemmer Signed-off-by: brian m. carlson --- Changes from v1: * Switch from rm -r to rm -fr. builtin/log.c | 2 +- log-tree.c | 7 ++++--- log-tree.h | 3 ++- t/t4014-format-patch.sh | 9 +++++++++ 4 files changed, 16 insertions(+), 5 deletions(-) diff --git a/builtin/log.c b/builtin/log.c index 71f68a3e4f..24868ed070 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1019,7 +1019,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout, open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet)) return; - log_write_email_headers(rev, head, &pp.after_subject, &need_8bit_cte); + log_write_email_headers(rev, head, &pp.after_subject, &need_8bit_cte, 0); for (i = 0; !need_8bit_cte && i < nr; i++) { const char *buf = get_commit_buffer(list[i], NULL); diff --git a/log-tree.c b/log-tree.c index d1c0bedf24..9f5eb346a4 100644 --- a/log-tree.c +++ b/log-tree.c @@ -362,7 +362,8 @@ void fmt_output_email_subject(struct strbuf *sb, struct rev_info *opt) void log_write_email_headers(struct rev_info *opt, struct commit *commit, const char **extra_headers_p, - int *need_8bit_cte_p) + int *need_8bit_cte_p, + int maybe_multipart) { const char *extra_headers = opt->extra_headers; const char *name = oid_to_hex(opt->zero_commit ? @@ -385,7 +386,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit, opt->ref_message_ids->items[i].string); graph_show_oneline(opt->graph); } - if (opt->mime_boundary) { + if (opt->mime_boundary && maybe_multipart) { static char subject_buffer[1024]; static char buffer[1024]; struct strbuf filename = STRBUF_INIT; @@ -610,7 +611,7 @@ void show_log(struct rev_info *opt) if (cmit_fmt_is_mail(opt->commit_format)) { log_write_email_headers(opt, commit, &extra_headers, - &ctx.need_8bit_cte); + &ctx.need_8bit_cte, 1); ctx.rev = opt; ctx.print_email_subject = 1; } else if (opt->commit_format != CMIT_FMT_USERFORMAT) { diff --git a/log-tree.h b/log-tree.h index deba035187..e668628074 100644 --- a/log-tree.h +++ b/log-tree.h @@ -27,7 +27,8 @@ void format_decorations_extended(struct strbuf *sb, const struct commit *commit, void show_decorations(struct rev_info *opt, struct commit *commit); void log_write_email_headers(struct rev_info *opt, struct commit *commit, const char **extra_headers_p, - int *need_8bit_cte_p); + int *need_8bit_cte_p, + int maybe_multipart); void load_ref_decorations(struct decoration_filter *filter, int flags); #define FORMAT_PATCH_NAME_MAX 64 diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh index 482112ca33..6ea08fd5e9 100755 --- a/t/t4014-format-patch.sh +++ b/t/t4014-format-patch.sh @@ -1661,6 +1661,15 @@ test_expect_success 'format-patch --base with --attach' ' test_write_lines 1 2 >expect && test_cmp expect actual ' +test_expect_success 'format-patch --attach cover-letter only is non-multipart' ' + test_when_finished "rm -fr patches" && + git format-patch -o patches --cover-letter --attach=mimemime --base=HEAD~ -1 && + ! egrep "^--+mimemime" patches/0000*.patch && + egrep "^--+mimemime$" patches/0001*.patch >output && + test_line_count = 2 output && + egrep "^--+mimemime--$" patches/0001*.patch >output && + test_line_count = 1 output +' test_expect_success 'format-patch --pretty=mboxrd' ' sp=" " && ```