{"thread":{"id":"24045","subject":"[PATCH 1/2] mailsplit: Remove any '>' characters used to escape From_ lines in mbox.","startedAt":"2010-06-08T20:02:28Z","lastAt":"2010-06-08T22:10:22Z","messageCount":8,"participants":["Carl Worth","H. Peter Anvin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"143268","messageId":"1276027349-4064-1-git-send-email-cworth@cworth.org","threadId":"24045","inReplyTo":"87hbldjo0s.fsf@yoom.home.cworth.org","subject":"[PATCH 1/2] mailsplit: Remove any '>' characters used to escape From_ lines in mbox.","fromName":"Carl Worth","fromEmail":"cworth@cworth.org","sentAt":"2010-06-08T20:02:28Z","receivedAt":"2010-06-08T20:02:28Z","isPatch":true,"sender":{"key":"cworth@cworth.org","avatar":"https://gravatar.com/avatar/3746dc28cde609bdbd7f939058356e7e2bbd16d21e32274df0725eb3d998bc5b?d=mp&s=160"},"body":"In order to encode an email message in an mbox, a client must notice any\nlines in the email body that look like so-called From_ lines, (that is\nlines begin with \"From \"), and add a preceding '>' character.\n\n>From Jonathan de Boyne Pollard[*] we learn of two long-standing (since 1995\nat least) conventions used for this escaping. The original \"mboxo\" format\ndoes only the escaping described above, which leads to unavoidable\ncorruption of some messages. The newer \"mboxrd\" format also adds a '>' to\nany line originally beginning with one or more '>' characters followed by\n\"From \". This ensures that the original email can be extracted without\ncorruption.\n\nGit wasn't formerly un-escaping these lines in any case, so invocations of\n\"git am\" would lead to errant '>' characters in the commit message. Here,\nwe now fix git-mailsplit to perform the necessary un-escaping. We assume\nmboxrd format, since designing for the original mboxo format would\nguarantee corruption in at least some cases.\n\n[*] http://homepage.ntlworld.com/jonathan.deboynepollard/FGA/mail-mbox-formats.html\n\nSigned-off-by: Carl Worth <cworth@cworth.org>\n---\n builtin/mailsplit.c |   26 +++++++++++++++++++++++++-\n 1 files changed, 25 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/mailsplit.c b/builtin/mailsplit.c\nindex cdfc1b7..a3fb9f7 100644\n--- a/builtin/mailsplit.c\n+++ b/builtin/mailsplit.c\n@@ -46,6 +46,30 @@ static int is_from_line(const char *line, int len)\n static struct strbuf buf = STRBUF_INIT;\n static int keep_cr;\n \n+/* Write the line in 'buf' to 'output', but if we are splitting an mbox,\n+ * then remove the first '>' from any line that begins with one or more\n+ * '>' characters followed by \"From \".\n+ *\n+ * Return 0 if successful, 1 for any write error.\n+ */\n+static int write_buf_unescaping(FILE *output, int is_mbox)\n+{\n+\tconst char *line = buf.buf;\n+\tsize_t len = buf.len;\n+\n+\tif (is_mbox && *line == '>') {\n+\t\tconst char *s = line;\n+\t\twhile (*s == '>')\n+\t\t\ts++;\n+\t\tif (strncmp (s, \"From \", 5) == 0) {\n+\t\t\tline = line + 1;\n+\t\t\tlen = len - 1;\n+\t\t}\n+\t}\n+\n+\treturn fwrite(line, 1, len, output) != len;\n+}\n+\n /* Called with the first line (potentially partial)\n  * already in buf[] -- normally that should begin with\n  * the Unix \"From \" line.  Write it into the specified\n@@ -76,7 +100,7 @@ static int split_one(FILE *mbox, const char *name, int allow_bare)\n \t\t\tstrbuf_addch(&buf, '\\n');\n \t\t}\n \n-\t\tif (fwrite(buf.buf, 1, buf.len, output) != buf.len)\n+\t\tif (write_buf_unescaping(output, !is_bare))\n \t\t\tdie_errno(\"cannot write output\");\n \n \t\tif (strbuf_getwholeline(&buf, mbox, '\\n')) {\n-- \n1.7.0.4\n"},{"id":"143269","messageId":"1276027349-4064-2-git-send-email-cworth@cworth.org","threadId":"24045","inReplyTo":"1276027349-4064-1-git-send-email-cworth@cworth.org","subject":"[PATCH 2/2] Add test from From_-line escaping.","fromName":"Carl Worth","fromEmail":"cworth@cworth.org","sentAt":"2010-06-08T20:02:29Z","receivedAt":"2010-06-08T20:02:29Z","isPatch":true,"sender":{"key":"cworth@cworth.org","avatar":"https://gravatar.com/avatar/3746dc28cde609bdbd7f939058356e7e2bbd16d21e32274df0725eb3d998bc5b?d=mp&s=160"},"body":"As implemented in the previous commit. We test that when applying from an\nmbox that all escaped From_ lines are properly unescaped. We also test that\nwhen applying from an email message the unescaping does not occur.\n\nSigned-off-by: Carl Worth <cworth@cworth.org>\n---\n t/t4152-am-From_.sh |   64 +++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 64 insertions(+), 0 deletions(-)\n create mode 100755 t/t4152-am-From_.sh\n\ndiff --git a/t/t4152-am-From_.sh b/t/t4152-am-From_.sh\nnew file mode 100755\nindex 0000000..02821ee\n--- /dev/null\n+++ b/t/t4152-am-From_.sh\n@@ -0,0 +1,64 @@\n+#!/bin/sh\n+\n+test_description='git am properly unescaping From_ lines'\n+\n+. ./test-lib.sh\n+\n+cat >msg <<EOF\n+From_ lines\n+\n+This is a commit message that contains a From_ line, which is line\n+that begins with the characters \"From \". Get ready for it, now...\n+From this time forward, we'll have no From_-line bugs.\n+\n+Additionally, we'll also test lines that are escaped versions of From_\n+lines. These are lines that begin with one or more '>' characters that\n+are then followed by the characters \"From \". We want to ensure that\n+none of these intentional '>' characters get swallowed. Let's try that\n+with three variations, (with 1, 2, and 3 leading '>' characters):\n+\n+>From now on (with one leading '>')\n+>>From there to here (with two leading '>' characters)\n+>>>From Here to Eternity (with three leading '>' characters)\n+\n+EOF\n+\n+test_expect_success setup '\n+\techo hello >file &&\n+\tgit add file &&\n+\ttest_tick &&\n+\tgit commit -m first &&\n+\tgit tag first &&\n+\techo world >>file &&\n+\tgit add file &&\n+\ttest_tick &&\n+\tgit commit -s -F msg &&\n+\tgit tag second &&\n+\tgit format-patch --stdout first | sed -e \"1{p;d};s/^\\(>*From \\)/>\\1/\" > From_ &&\n+\t{\n+\t\techo \"X-Fake-Field: Line One\" &&\n+\t\techo \"X-Fake-Field: Line Two\" &&\n+\t\techo \"X-Fake-Field: Line Three\" &&\n+\t\tgit format-patch --stdout first | sed -e \"1d\"\n+\t} > From_.eml\n+'\n+\n+test_expect_success 'am unescapes From_ lines from mbox' '\n+\tgit checkout first &&\n+\tgit am From_ &&\n+\t! test -d .git/rebase-apply &&\n+\ttest -z \"$(git diff second)\" &&\n+\ttest \"$(git rev-parse second)\" = \"$(git rev-parse HEAD)\" &&\n+\ttest \"$(git rev-parse second^)\" = \"$(git rev-parse HEAD^)\"\n+'\n+\n+test_expect_success 'am does not unescape From_ lines from email' '\n+\tgit checkout first &&\n+\tgit am From_.eml &&\n+\t! test -d .git/rebase-apply &&\n+\ttest -z \"$(git diff second)\" &&\n+\ttest \"$(git rev-parse second)\" = \"$(git rev-parse HEAD)\" &&\n+\ttest \"$(git rev-parse second^)\" = \"$(git rev-parse HEAD^)\"\n+'\n+\n+test_done\n-- \n1.7.0.4\n"},{"id":"143275","messageId":"87d3w1jlp0.fsf@yoom.home.cworth.org","threadId":"24045","inReplyTo":"87hbldjo0s.fsf@yoom.home.cworth.org","subject":"Re: Make \"git am\" properly unescape lines matching \">>*From \"","fromName":"Carl Worth","fromEmail":"cworth@cworth.org","sentAt":"2010-06-08T20:47:39Z","receivedAt":"2010-06-08T20:47:39Z","isPatch":false,"sender":{"key":"cworth@cworth.org","avatar":"https://gravatar.com/avatar/3746dc28cde609bdbd7f939058356e7e2bbd16d21e32274df0725eb3d998bc5b?d=mp&s=160"},"body":"On Tue, 08 Jun 2010 12:57:23 -0700, Carl Worth <cworth@cworth.org> wrote:\n> I'm adding support to notmuch[1] to more easily pipe a thread full of\n> But I noticed that \"git am\" wasn't removing any of these added '>'\n> characters, so I was getting corrupted commit messages.\n\nI've also noticed that format-patch is generating bogus mbox files\nwithout any escaping. (The only way it gets away with this is that\nmailsplit only treats \"From \" lines as separators if they end with\nsomething that looks quite a bit like the output of asctime.)\n\nThis does mean that without changing format-patch, the patched \"git am\"\ncould corrupt a commit message. This could happen if the commit message\noriginally contained a line matching \"^From \" which would previously be\npassed through directly but will now be un-escaped to \"From \".\n\nThis does seem less likely than a message containing a line matching\n\"^From \" (which is the case that gets corrupted with an unpatched \"git\nam\") so one option would be to ignore this, and apply my patch. That's\nwhat I recommend for now.\n\nAlternately, we could fix format-patch to add the correct, (and\nreversible), escaping that is now expected by git-am.\n\nAny attempt to add escaping to format-patch should recognize that many\nusers use the output of format-patch directly as content handed to their\nMUA. Such users will *not* want escaping, (they are effectively treating\nthe format-patch output as a bare email message, not an mbox).\n\nSo if someone were to attempt this, I'd suggest first changing\nformat-patch to actually generate bare email messages when generating\nfiles containing only a single message. This is instead of the invalid\nmbox files it is generating now. This would be as simple as not emitting\nthe initial \"From \" line.\n\nThen, when generating an actual mbox with multiple files, format-patch\nshould do the correct escaping, (which is now expected by \"git am\"), and\nall of these cases of potential commit-message corruption should be\neliminated.\n\nThe other thing that would need to be fixed in this approach is to fix\n\"git send-email\" to do the right thing with a bare email message. From a\nquick glance at the code, it appears to be looking for an initial \"From\n\" line, even though it doesn't appear to handle an mbox with multiple\nmessages. It looks for this line to distinguish an email message from\nsome custom \"send lots of email\" format. It should be simple to instead\ndistinguish a bare email message from the \"send lots of email\" format by\na first line which looks like an email header.\n\n-Carl\n\n-- \ncarl.d.worth@intel.com\n"},{"id":"143276","messageId":"4C0EAD00.8000706@zytor.com","threadId":"24045","inReplyTo":"87hbldjo0s.fsf@yoom.home.cworth.org","subject":"Re: Make \"git am\" properly unescape lines matching \">>*From \"","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2010-06-08T20:50:08Z","receivedAt":"2010-06-08T20:50:08Z","isPatch":false,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"On 06/08/2010 12:57 PM, Carl Worth wrote:\n> I'm adding support to notmuch[1] to more easily pipe a thread full of\n> patches to \"git am\". So I added support for notmuch to format a thread\n> (or any search) as an mbox.\n> \n> When I did that, I was careful to escape lines from the bodies of email\n> messages that begin with zero or more '>' characters followed\n> immediately by \"From \" (From_ lines) by adding an initial '>'. [2]\n> \n> But I noticed that \"git am\" wasn't removing any of these added '>'\n> characters, so I was getting corrupted commit messages.\n> \n> I'll follow up this message with a patch that fixes that by making\n> git-mailsplit un-escape these lines. It's careful to do this only when\n> processing an actual mbox, using the existing detection of a bare email\n> message and not doing any un-escaping in that case.\n> \n> I'll also follow up with a new test for both cases, (using \"git am\" with\n> both an mbox with escaped From_ lines and an email message without\n> escaped From_ lines).\n> \n\nThe problem with that is that it is not universally applied.  For what\nI've seen, some mbox-based programs simply rely on there being a\nContent-Length: header and don't need From lines to be escaped at all\n(and don't do anything useful if they are), some do the leading > trick\n(usually not reversably at all).\n\nAs far as I can tell, the Content-Length: is the most reliably handled\nformat and probably is what we should use.  This is the \"mboxcl2\" format\nin your list.[*]  Unfortunately \"mboxcl2\" and \"mboxrd\" cannot be\ndistinguished from each other by inspection, which is a major defect of\nboth formats.\n\nThe statement that \"the entire \"mbox\" family of mailbox formats is\ngradually becoming irrelevant, and of only historical interest\" is also\npretty silly -- mbox is still the preferred format for moving groups of\nemail from MUA to MUA, even if it is no longer used for active live\nspool storage.  But, of course, you knew that already.\n\n\t-hpa\n\n[*] There are apparently some MTA/MUAs which simply bypass the entire\nproblem by base64-encoding any email that contains /^From /, just as if\nit contained NUL bytes.  It's a heavyweight, but thoroughly unambiguous\nway of dealing with the problem.\n"},{"id":"143278","messageId":"4C0EAE08.60904@zytor.com","threadId":"24045","inReplyTo":"87d3w1jlp0.fsf@yoom.home.cworth.org","subject":"Re: Make \"git am\" properly unescape lines matching \">>*From \"","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2010-06-08T20:54:32Z","receivedAt":"2010-06-08T20:54:32Z","isPatch":false,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"On 06/08/2010 01:47 PM, Carl Worth wrote:\n> On Tue, 08 Jun 2010 12:57:23 -0700, Carl Worth <cworth@cworth.org> wrote:\n>> I'm adding support to notmuch[1] to more easily pipe a thread full of\n>> But I noticed that \"git am\" wasn't removing any of these added '>'\n>> characters, so I was getting corrupted commit messages.\n> \n> I've also noticed that format-patch is generating bogus mbox files\n> without any escaping. (The only way it gets away with this is that\n> mailsplit only treats \"From \" lines as separators if they end with\n> something that looks quite a bit like the output of asctime.)\n> \n\nAt the same time, it would be a fairly major lose to not be able to\ngenerate individual messages easily.  I have personally considered the\nfact that git format-patch produces something-vaguely-like mboxes rather\nthan individual plain RFC 2822 messages to be a bug; fixable by \"tail\"\nbut annoying.\n\n\t-hpa\n"},{"id":"143282","messageId":"87wru9i55a.fsf@yoom.home.cworth.org","threadId":"24045","inReplyTo":"4C0EAE08.60904@zytor.com","subject":"Re: Make \"git am\" properly unescape lines matching \">>*From \"","fromName":"Carl Worth","fromEmail":"cworth@cworth.org","sentAt":"2010-06-08T21:30:25Z","receivedAt":"2010-06-08T21:30:25Z","isPatch":false,"sender":{"key":"cworth@cworth.org","avatar":"https://gravatar.com/avatar/3746dc28cde609bdbd7f939058356e7e2bbd16d21e32274df0725eb3d998bc5b?d=mp&s=160"},"body":"On Tue, 08 Jun 2010 13:54:32 -0700, \"H. Peter Anvin\" <hpa@zytor.com> wrote:\n> On 06/08/2010 01:47 PM, Carl Worth wrote:\n> > I've also noticed that format-patch is generating bogus mbox files\n> > without any escaping. (The only way it gets away with this is that\n> > mailsplit only treats \"From \" lines as separators if they end with\n> > something that looks quite a bit like the output of asctime.)\n> \n> At the same time, it would be a fairly major lose to not be able to\n> generate individual messages easily.  I have personally considered the\n> fact that git format-patch produces something-vaguely-like mboxes rather\n> than individual plain RFC 2822 messages to be a bug; fixable by \"tail\"\n> but annoying.\n\nI totally agree. I said as much later on in the message. We should fix\nformat-patch to not emit the \"From \" line when generating files for\nindividual messages, (and we should fix send-email to accept such a bare\nfile).\n\nThat much is easy to agree on since it involves using mbox less, so the\nwhole \"which mbox format to use?\" question goes away, (for these uses at\nleast).\n\n-Carl\n\n-- \ncarl.d.worth@intel.com\n"},{"id":"143286","messageId":"87typdi44d.fsf@yoom.home.cworth.org","threadId":"24045","inReplyTo":"4C0EAD00.8000706@zytor.com","subject":"Re: Make \"git am\" properly unescape lines matching \">>*From \"","fromName":"Carl Worth","fromEmail":"cworth@cworth.org","sentAt":"2010-06-08T21:52:34Z","receivedAt":"2010-06-08T21:52:34Z","isPatch":false,"sender":{"key":"cworth@cworth.org","avatar":"https://gravatar.com/avatar/3746dc28cde609bdbd7f939058356e7e2bbd16d21e32274df0725eb3d998bc5b?d=mp&s=160"},"body":"On Tue, 08 Jun 2010 13:50:08 -0700, \"H. Peter Anvin\" <hpa@zytor.com> wrote:\n> On 06/08/2010 12:57 PM, Carl Worth wrote:\n> > When I did that, I was careful to escape lines from the bodies of email\n> > messages that begin with zero or more '>' characters followed\n> > immediately by \"From \" (From_ lines) by adding an initial '>'. [2]\n...\n> The problem with that is that it is not universally applied.\n\nRight. And since I can't fix this universe, I'd like to at least start\nwith getting notmuch and git to use the same thing. Currently, git is\nusing a non-standard not-quite-safe mbox format while notmuch doesn't\nyet emit anything like mbox. So we have a nice opportunity to fix these\ntwo projects to at least work well together, (if we can agree on a\nformat).\n\n> As far as I can tell, the Content-Length: is the most reliably handled\n> format and probably is what we should use.  This is the \"mboxcl2\" format\n> in your list.[*]  Unfortunately \"mboxcl2\" and \"mboxrd\" cannot be\n> distinguished from each other by inspection, which is a major defect of\n> both formats.\n\nWhat do you mean by \"most reliably handled format\"?\n\nOf the four mbox formats listed on the page I cited[*], \"mboxo\" and\n\"mboxcl\" are easy to discard as they both irreversibly corrupt messages.\n\nThat leaves both \"mboxrd\" and \"mboxcl2\" as candidates. Either of these\nformats is reliable if both the reader and writer use the same\nformat. When the reader and writer don't agree, then there are problems\nas follows (\"W:\" indicates writing, \"R:\" indicates reading expecting a\nparticular format):\n\nW:mboxrd  then R:mboxcl2 -> Reader may corrupt by failing to remove '>'\n\t\t\t    Reader must give up/guess without CL headers\n\t\t\t    Guessing is at least unlikely to mis-split messages\n\nW:mboxcl2 then R:mboxrd  -> Reader may corrupt by erroneously removing '>'\n\t\t\t    Reader may mis-split messages on \"From \" in content\n\nI preferred to implement mboxrd over mboxcl2 for several reasons:\n\n  1. The mboxrd writer implementation is much simpler. This format\n     affords a simple streaming implementation where mboxcl2 requires\n     knowing the length of the message in advance.\n\n  2. The mboxrd format is robust in the face of file changes that\n     invalidate the Content-Length headers, (for example, a person\n     can hand-edit an mboxrd file without invalidating it, but cannot do\n     the same with an mboxcl2 file).\n\n  3. The mboxrd reader implementation is much simpler. An mboxcl2 reader\n     necessarily has special-cases that an mboxrd implementation does\n     not. What to do if there is no Content-Length header? What to do if\n     the Content-Length header appears wrong? etc. Recovery code for\n     these cases might well be to fallback to something like an mboxrd\n     implementation, which demonstrates the increased complexity here.\n\nAs can be seen in my patch, doing an mboxrd reader in git-mailsplit was\nquite simple. An mboxcl2 reader would be quite a bit more complicated,\nbut with no actual benefit in reliability, (assuming that the reader\nmatches the writer).\n\n> The statement that \"the entire \"mbox\" family of mailbox formats is\n> gradually becoming irrelevant, and of only historical interest\" is also\n> pretty silly -- mbox is still the preferred format for moving groups of\n> email from MUA to MUA, even if it is no longer used for active live\n> spool storage.  But, of course, you knew that already.\n\nIndeed. Though I was surprised to recently find that postfix does still\nby default deliver to /var/mail/$user in \"mboxo\" format (ugh).\n\n-Carl\n\n[*] http://homepage.ntlworld.com/jonathan.deboynepollard/FGA/mail-mbox-formats.html\n\n-- \ncarl.d.worth@intel.com\n"},{"id":"143289","messageId":"4C0EBFCE.4050700@zytor.com","threadId":"24045","inReplyTo":"87typdi44d.fsf@yoom.home.cworth.org","subject":"Re: Make \"git am\" properly unescape lines matching \">>*From \"","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2010-06-08T22:10:22Z","receivedAt":"2010-06-08T22:10:22Z","isPatch":false,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"On 06/08/2010 02:52 PM, Carl Worth wrote:\n> \n> What do you mean by \"most reliably handled format\"?\n> \n\nI have to say there is definitely part of me that thinks that using\nbase64 or quoted-unprintable for \"^From \"-containing mails might really\nbe the best solution... as much as I normally hate that crap.\n\n\t-hpa\n"}]}