Bug: format-patch MIME boundary not added to cover letter when attach enabled

Bug: format-patch MIME boundary not added to cover letter when attach enabled

[Patrick Hemmer]

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 <git@stormcloud9.net>
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

Re: Bug: format-patch MIME boundary not added to cover letter when attach enabled

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 596 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]

Re: Bug: format-patch MIME boundary not added to cover letter when attach enabled

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> 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?

Re: Bug: format-patch MIME boundary not added to cover letter when attach enabled

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 837 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]

[PATCH] format-patch: make cover letters always text/plain

[brian m. carlson]

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 <git@stormcloud9.net>
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 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=" " &&

Re: [PATCH] format-patch: make cover letters always text/plain

[Eric Sunshine]

On Mon, Apr 30, 2018 at 8:02 PM, brian m. carlson
<sandals@crustytoothpaste.net> 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 <git@stormcloud9.net>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> 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
> +'

Re: [PATCH] format-patch: make cover letters always text/plain

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1490 bytes --]

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
> <sandals@crustytoothpaste.net> 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 <git@stormcloud9.net>
> > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> > ---
> > 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]

[PATCH v2] format-patch: make cover letters always text/plain

[brian m. carlson]

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 <git@stormcloud9.net>
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
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=" " &&