{"thread":{"id":"31694","subject":"mailinfo: don't require \"text\" mime type for attachments","startedAt":"2012-09-30T22:10:48Z","lastAt":"2012-10-01T13:27:46Z","messageCount":2,"participants":["Linus Torvalds","Don Zickus"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"200225","messageId":"alpine.LFD.2.02.1209301458540.11079@i5.linux-foundation.org","threadId":"31694","inReplyTo":null,"subject":"mailinfo: don't require \"text\" mime type for attachments","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2012-09-30T22:10:48Z","receivedAt":"2012-09-30T22:10:48Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nCurrently \"git am\" does insane things if the mbox it is given contains \nattachments with a MIME type that aren't \"text/*\".\n\nIn particular, it will still decode them, and pass them \"one line at a \ntime\" to the mail body filter, but because it has determined that they \naren't text (without actually looking at the contents, just at the mime \ntype) the \"line\" will be the encoding line (eg 'base64') rather than a \nline of *content*.\n\nWhich then will cause the text filtering to fail, because we won't \ncorrectly notice when the attachment text switches from the commit message \nto the actual patch. Resulting in a patch failure, even if patch may be a \nperfectly well-formed attachment, it's just that the message type may be \n(for example) \"application/octet-stream\" instead of \"text/plain\".\n\nJust remove all the bogus games with the message_type. The only difference \nthat code creates is how the data is passed to the filter function \n(chunked per-pred-code line or per post-decode line), and that difference \nis *wrong*, since chunking things per pre-decode line can never be a \nsensible operation, and cannot possibly matter for binary data anyway.\n\nThis code goes all the way back to March of 2007, in commit 87ab79923463 \n(\"builtin-mailinfo.c infrastrcture changes\"), and apparently Don used to \npass random mbox contents to git. However, the pre-decode vs post-decode \nlogic really shouldn't matter even for that case, and more importantly, \"I \nfed git am crap\" is not a valid reason to break *real* patch attachments.\n\nIf somebody really cares, and determines that some attachment is binary \ndata (by looking at the data, not the MIME-type), the whole attachment \nshould be dismissed, rather than fed in random-sized chunks to \n\"handle_filter()\".\n\nSigned-off-by: Linus Torvalds \nCc: Don Zickus \n---\n builtin/mailinfo.c | 11 -----------\n 1 file changed, 11 deletions(-)\n\ndiff --git a/builtin/mailinfo.c b/builtin/mailinfo.c\nindex 2b3f4d955eaa..da231400b327 100644\n--- a/builtin/mailinfo.c\n+++ b/builtin/mailinfo.c\n@@ -19,9 +19,6 @@ static struct strbuf email = STRBUF_INIT;\n static enum {\n \tTE_DONTCARE, TE_QP, TE_BASE64\n } transfer_encoding;\n-static enum {\n-\tTYPE_TEXT, TYPE_OTHER\n-} message_type;\n \n static struct strbuf charset = STRBUF_INIT;\n static int patch_lines;\n@@ -184,8 +181,6 @@ static void handle_content_type(struct strbuf *line)\n \tstruct strbuf *boundary = xmalloc(sizeof(struct strbuf));\n \tstrbuf_init(boundary, line->len);\n \n-\tif (!strcasestr(line->buf, \"text/\"))\n-\t\t message_type = TYPE_OTHER;\n \tif (slurp_attr(line->buf, \"boundary=\", boundary)) {\n \t\tstrbuf_insert(boundary, 0, \"--\", 2);\n \t\tif (++content_top > &content[MAX_BOUNDARIES]) {\n@@ -657,7 +652,6 @@ again:\n \t/* set some defaults */\n \ttransfer_encoding = TE_DONTCARE;\n \tstrbuf_reset(&charset);\n-\tmessage_type = TYPE_TEXT;\n \n \t/* slurp in this section's info */\n \twhile (read_one_header_line(&line, fin))\n@@ -871,11 +865,6 @@ static void handle_body(void)\n \t\t\tstrbuf_insert(&line, 0, prev.buf, prev.len);\n \t\t\tstrbuf_reset(&prev);\n \n-\t\t\t/* binary data most likely doesn't have newlines */\n-\t\t\tif (message_type != TYPE_TEXT) {\n-\t\t\t\thandle_filter(&line);\n-\t\t\t\tbreak;\n-\t\t\t}\n \t\t\t/*\n \t\t\t * This is a decoded line that may contain\n \t\t\t * multiple new lines. Pass only one chunk\n"},{"id":"200258","messageId":"20121001132746.GV1969@redhat.com","threadId":"31694","inReplyTo":"alpine.LFD.2.02.1209301458540.11079@i5.linux-foundation.org","subject":"Re: mailinfo: don't require \"text\" mime type for attachments","fromName":"Don Zickus","fromEmail":"dzickus@redhat.com","sentAt":"2012-10-01T13:27:46Z","receivedAt":"2012-10-01T13:27:46Z","isPatch":false,"sender":{"key":"dzickus@redhat.com","avatar":null},"body":"On Sun, Sep 30, 2012 at 03:10:48PM -0700, Linus Torvalds wrote:\n> This code goes all the way back to March of 2007, in commit 87ab79923463 \n> (\"builtin-mailinfo.c infrastrcture changes\"), and apparently Don used to \n> pass random mbox contents to git. However, the pre-decode vs post-decode \n> logic really shouldn't matter even for that case, and more importantly, \"I \n> fed git am crap\" is not a valid reason to break *real* patch attachments.\n> \n> If somebody really cares, and determines that some attachment is binary \n> data (by looking at the data, not the MIME-type), the whole attachment \n> should be dismissed, rather than fed in random-sized chunks to \n> \"handle_filter()\".\n\nHeh. Years ago when I tried using git as a patch-control-management\nsystem instead of a traditional SCM, I fed my custom git-am script an\ninternal kernel-mail-archives list to help process the meta data for\npatches (acks, nacks, needinfo, bugzillas, etc). It served its purpose\nuntil we switched to a fork'd copy of patch-work.\n\nSo I haven't done 'insane' stuff in years. :-) I'm sure this patch is\nright, but it doesn't affect me anymore.\n\nSorry for any problems that arose..\n\nCheers,\nDon\n"}]}