{"thread":{"id":"28791","subject":"imap-send badly handles commit bodies beginning with \"From <\"","startedAt":"2011-10-28T18:00:44Z","lastAt":"2011-11-01T16:14:12Z","messageCount":8,"participants":["Andrew Eikum","Jeff King","Magnus Bäck","Michael Haggerty"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"178480","messageId":"20111028180044.GA3966@foghorn.codeweavers.com","threadId":"28791","inReplyTo":null,"subject":"imap-send badly handles commit bodies beginning with \"From <\"","fromName":"Andrew Eikum","fromEmail":"aeikum@codeweavers.com","sentAt":"2011-10-28T18:00:44Z","receivedAt":"2011-10-28T18:00:44Z","isPatch":false,"sender":{"key":"aeikum@codeweavers.com","avatar":null},"body":"Ran into this today. I had a commit message that looked like:\n\n---\nDo something\n\n>From :\nWords\n---\n\nI put it through imap-send to email it to my project, and ended up\nwith this output:\n\nsending 1 messages\n 200% (2/1) done\n\nOn the server side, it was split into two mails on either side of that\ncommit message's From line with neither mail actually containing the\nFrom line. To fix it, I just changed it to \"Copied from :\" :-P\n\nAin't mbox grand?\n"},{"id":"178489","messageId":"20111028203256.GA15082@sigill.intra.peff.net","threadId":"28791","inReplyTo":"20111028180044.GA3966@foghorn.codeweavers.com","subject":"Re: imap-send badly handles commit bodies beginning with \"From <\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-28T20:32:57Z","receivedAt":"2011-10-28T20:32:57Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 28, 2011 at 01:00:44PM -0500, Andrew Eikum wrote:\n\n> On the server side, it was split into two mails on either side of that\n> commit message's From line with neither mail actually containing the\n> From line. To fix it, I just changed it to \"Copied from :\" :-P\n> \n> Ain't mbox grand?\n\nMbox does have this problem, but I think in this case it is a\nparticularly crappy implementation of mbox in imap-send. Look at\nimap-send.c:split_msg; it just looks for \"From \".\n\nIt should at least check for something that looks like a timestamp, like\ngit-mailsplit does. Maybe mailsplit's is_from_line should be factored\nout so that it can be reused in imap-send.\n\nWant to work on a patch?\n\n-Peff\n"},{"id":"178492","messageId":"20111028212122.GB3966@foghorn.codeweavers.com","threadId":"28791","inReplyTo":"20111028203256.GA15082@sigill.intra.peff.net","subject":"Re: imap-send badly handles commit bodies beginning with \"From <\"","fromName":"Andrew Eikum","fromEmail":"aeikum@codeweavers.com","sentAt":"2011-10-28T21:21:22Z","receivedAt":"2011-10-28T21:21:22Z","isPatch":false,"sender":{"key":"aeikum@codeweavers.com","avatar":null},"body":"On Fri, Oct 28, 2011 at 01:32:57PM -0700, Jeff King wrote:\n> Mbox does have this problem, but I think in this case it is a\n> particularly crappy implementation of mbox in imap-send. Look at\n> imap-send.c:split_msg; it just looks for \"From \".\n> \n> It should at least check for something that looks like a timestamp, like\n> git-mailsplit does. Maybe mailsplit's is_from_line should be factored\n> out so that it can be reused in imap-send.\n\nSince we have a program called \"mailsplit,\" wouldn't it make more\nsense to have imap-send use its implementation to split mail instead\nof sharing just the From line detection?\n\n> Want to work on a patch?\n\nI was hoping it'd be a quick matter of pulling mailsplit's\nimplementation out of builtin and into the top level, but I see it's\ngot some global variables that are tangled enough that I actually have\nto understand the code before I can pull it apart :)\n\nIf no one beats me to it, I'll work on this next week. It's late on\nFriday and I'm moving house this weekend.\n\nQuick question, since I'm not intimately familiar with Git's code: I\nwas thinking of creating a new compilation unit at the top level,\nmailutils.{c,h}, and referencing it from both imap-send.c and\nbuiltin/splitmail.c. Does that seem like the right approach? Is there\nan existing compilation unit I should be placing splitmail's guts into\ninstead?\n\nAndrew\n"},{"id":"178493","messageId":"20111028213703.GA1454@sigill.intra.peff.net","threadId":"28791","inReplyTo":"20111028212122.GB3966@foghorn.codeweavers.com","subject":"Re: imap-send badly handles commit bodies beginning with \"From <\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-28T21:37:04Z","receivedAt":"2011-10-28T21:37:04Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 28, 2011 at 04:21:22PM -0500, Andrew Eikum wrote:\n\n> Since we have a program called \"mailsplit,\" wouldn't it make more\n> sense to have imap-send use its implementation to split mail instead\n> of sharing just the From line detection?\n\nPotentially, yeah. I was thinking of just pulling over the from line\ndetection (which is the real black magic bit), but it looks like\nimap-send's mbox handling could use some general attention (maybe it\nwould be possible to not read the entire mbox into memory, for example).\n\n> I was hoping it'd be a quick matter of pulling mailsplit's\n> implementation out of builtin and into the top level, but I see it's\n> got some global variables that are tangled enough that I actually have\n> to understand the code before I can pull it apart :)\n>\n> If no one beats me to it, I'll work on this next week. It's late on\n> Friday and I'm moving house this weekend.\n\nNo rush. Let us know if you have questions.\n\n> Quick question, since I'm not intimately familiar with Git's code: I\n> was thinking of creating a new compilation unit at the top level,\n> mailutils.{c,h}, and referencing it from both imap-send.c and\n> builtin/splitmail.c. Does that seem like the right approach? Is there\n> an existing compilation unit I should be placing splitmail's guts into\n> instead?\n\nYes, I think a new file makes sense here. Make sure to update LIB_H and\nLIB_OBJS in the Makefile.\n\n-Peff\n"},{"id":"178525","messageId":"20111030090111.GA1624@jpl.local","threadId":"28791","inReplyTo":"20111028203256.GA15082@sigill.intra.peff.net","subject":"Re: imap-send badly handles commit bodies beginning with \"From <\"","fromName":"Magnus Bäck","fromEmail":"magnus.back@sonyericsson.com","sentAt":"2011-10-30T09:01:11Z","receivedAt":"2011-10-30T09:01:11Z","isPatch":false,"sender":{"key":"magnus.back@sonyericsson.com","avatar":null},"body":"On Friday, October 28, 2011 at 22:32 CEST,\n Jeff King wrote:\n\n> On Fri, Oct 28, 2011 at 01:00:44PM -0500, Andrew Eikum wrote:\n> \n> > On the server side, it was split into two mails on either side\n> > of that commit message's From line with neither mail actually\n> > containing the From line. To fix it, I just changed it to \"Copied\n> > from :\" :-P\n> > \n> > Ain't mbox grand?\n> \n> Mbox does have this problem, but I think in this case it is a\n> particularly crappy implementation of mbox in imap-send. Look at\n> imap-send.c:split_msg; it just looks for \"From \".\n\nWhile there seems to be about a million different implementations of\nmbox creation and parsing, the relevant RFC[0] points to [1] as an\nauthoritative source. The latter claims that lines matching \"^From \"\ndenote a message boundary and that lines within a message that match\nthe same pattern should be quoted with \">\". That would suggest that\nthe problem isn't imap-send.c but whatever code produces the mbox\nfile in the first place. Of course, if that software isn't part of\nGit I guess we'll have to deal with the situation anyway. And whatever\nthe RFCs say, we still need to be as compatible is possible with\nwhatever software is out there.\n\n> It should at least check for something that looks like a timestamp,\n> like git-mailsplit does. Maybe mailsplit's is_from_line should be\n> factored out so that it can be reused in imap-send.\n\nI guess that's a reasonable \"liberal in what you accept\" mitigation.\n\n(As a sidenote, I'm getting the \">From\" quoting in my maildir message\nfiles where no such quoting is expected, so \"From\" lines are shown as\n\">From\" in my MUA. I don't know if it's Procmail screwing things up or\nwhat's going on.)\n\n[0] http://tools.ietf.org/html/rfc4155\n[1] http://qmail.org./man/man5/mbox.html\n\n-- \nMagnus Bäck Opinions are my own and do not necessarily\nSW Configuration Manager represent the ones of my employer, etc.\nSony Ericsson\n"},{"id":"178628","messageId":"20111101153803.GB5552@sigill.intra.peff.net","threadId":"28791","inReplyTo":"20111030090111.GA1624@jpl.local","subject":"Re: imap-send badly handles commit bodies beginning with \"From <\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-11-01T15:38:03Z","receivedAt":"2011-11-01T15:38:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 30, 2011 at 10:01:11AM +0100, Magnus Bäck wrote:\n\n> > Mbox does have this problem, but I think in this case it is a\n> > particularly crappy implementation of mbox in imap-send. Look at\n> > imap-send.c:split_msg; it just looks for \"From \".\n> \n> While there seems to be about a million different implementations of\n> mbox creation and parsing, the relevant RFC[0] points to [1] as an\n> authoritative source. The latter claims that lines matching \"^From \"\n> denote a message boundary and that lines within a message that match\n> the same pattern should be quoted with \">\". That would suggest that\n> the problem isn't imap-send.c but whatever code produces the mbox\n> file in the first place. Of course, if that software isn't part of\n> Git I guess we'll have to deal with the situation anyway. And whatever\n> the RFCs say, we still need to be as compatible is possible with\n> whatever software is out there.\n\nRight. If you properly quote and unquote \"From \" lines, then mbox can be\nunambiguous. But many pieces of software don't quote them (including\ngit, I think, but I didn't check), so it's prudent when reading to look\nfor something that actually appears to be a \"From\" line.\n\nIf somebody wants to tackle >From quoting of commit messages in\ngit-format-patch, they can certainly do so. In practice, it doesn't tend\nto come up (because sane readers expect there to be a date at the end of\nthe line), so nobody has put forth the effort.\n\n-Peff\n"},{"id":"178629","messageId":"4EB01918.8080604@alum.mit.edu","threadId":"28791","inReplyTo":"20111101153803.GB5552@sigill.intra.peff.net","subject":"Re: imap-send badly handles commit bodies beginning with \"From <\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-11-01T16:06:48Z","receivedAt":"2011-11-01T16:06:48Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 11/01/2011 04:38 PM, Jeff King wrote:\n> Right. If you properly quote and unquote \"From \" lines, then mbox can be\n> unambiguous.\n\nThat is not quite true. The RFC says only that lines matching \"^From \"\nshould be quoted, not lines matching \"^>From \" (or, generally, \"^>*From\n\"). So the quoting is lossy; it is *not* possible to tell whether a\nline starting with \">From \" should be unquoted (it could have been\n\">From \" in the original).\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"178630","messageId":"20111101161412.GA7796@sigill.intra.peff.net","threadId":"28791","inReplyTo":"4EB01918.8080604@alum.mit.edu","subject":"Re: imap-send badly handles commit bodies beginning with \"From <\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-11-01T16:14:12Z","receivedAt":"2011-11-01T16:14:12Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 01, 2011 at 05:06:48PM +0100, Michael Haggerty wrote:\n\n> On 11/01/2011 04:38 PM, Jeff King wrote:\n> > Right. If you properly quote and unquote \"From \" lines, then mbox can be\n> > unambiguous.\n> \n> That is not quite true. The RFC says only that lines matching \"^From \"\n> should be quoted, not lines matching \"^>From \" (or, generally, \"^>*From\n> \"). So the quoting is lossy; it is *not* possible to tell whether a\n> line starting with \">From \" should be unquoted (it could have been\n> \">From \" in the original).\n\nThat was what I meant by \"properly\". Note that the second link Magnus\nmentioned (and which is referred to in the RFC in the paragraph\nimmediately following the discussion of \"from\" quoting) discusses this\nexplicitly.\n\nThe real issue with mbox is not that it can't be done well, but that you\nhave no clue which variant the writing end used. In practice, it works\nOK because it's simple and those corner cases just don't come up much\n(at least for a reasonably defensive reader).\n\n-Peff\n"}]}