copy an email message file that doesn't have cr/lf and it will give an error and not copy over last line of message. cause of problem is here: http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsLocalMailFolder.cpp#2382 error is from left over buffer reported here: http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsLocalMailFolder.cpp#2516 doesn't happen when downloading IMAP messages, but a cr/lf is added to downloaded imap messages. Existing unit test may prove this, I'll check later if not addressed in meantime.

This takes away the value of saving a partial line, and adding to new data on the next call. The CopyFileMessage doesn't do this so it needs CopyData to do the whole thing, crlf at eof or not. I need to pass through a variable for those calls that don't want the wholeFile functionality but to save partial lines. I left it alone for now to see if you have a better idea. So far I think there is only one other call to CopyData.

Comment on attachment 384326 [details] [diff] [review] copies over the last line that is 'leftover' mbox format requires a blank line at end of message so I'll fix this and resubmit patch to add a blank line to files as needed. This will make the CopyData work for all callers.

This takes care of CopyFileMessage to account for files that do not have a crlf at the eof. But I'm not sure if the users of CopyData need this functionality. David, please check and see if we want to set this boolean value on the end of other CopyData uses. Otherwise the init is setting it to false.

Comment on attachment 384797 [details] [diff] [review] adds test for complete file copy and checks for crlf This file doesn't use K&R braces, so this patch shouldn't... m_completeFile is an ambiguous var name - I assume the sense of it is whether we should a trailing blank line? Could you use a name that reflects that? On Mac/Linux, don't we just want a LF, not a CRLF?

variable suggestion makes much better sense. Other comments fixed.

Comment on attachment 385498 [details] [diff] [review] per review comments this patch doesn't apply cleanly, but more seriously, I think it adds a blank line at the end of every block, not just at the end of the message.

(In reply to comment #6) > (From update of attachment 385498 [details] [diff] [review]) > this patch doesn't apply cleanly, but more seriously, I think it adds a blank > line at the end of every block, not just at the end of the message. I had this depending on bug 499304. Both bugs are important to CopyFileMessage. But I think I can change the patch order if needed. CopyFileMessage sets the m_addNewLineToEndOfMsg and calls CopyData with length of complete file (that was original intent of using m_completeFile as name, although I did change it to that from my first idea of m_wholeFile). The 'while (1)' block has to copy the whole file, a line at a time, and if the last line of data is not terminated with a linebreak it adds it.

The CopyData method is called from an other place, in a loop, so it's a bit scary to add code that could add newlines in the middle of a message. I can see why you did it there, because you want to make sure the added CRLF goes through the normal line parsing stuff... What might make this a lot cleaner is a comment explaining what the situation is, and what the code is doing. Otherwise, it just looks like magic. Something like this, where you handle the case of not finding a cr or lf: In the case of copying a whole file (i.e., called from CopyFileMessage), if we don't find a trailing LINEBREAK, we must be at the last line of the file, and it must not have a LINEBREAK, so we're going to add it to the buffer.

I quasi adapted your comment suggestion but feel free to edit it and the variable name if it's still not clear enough.

ok, thx, yes, I'm going to edit this a little to make it clear that the whole message is being copied in a single call to CopyData - that's really the crucial point. I'll tweak this when the tree opens again, and land it; thx for the patch! I'll mark it r/sr=me when I attach the tweaked version later.

(In reply to comment #11) > crucial point. I'll tweak this when the tree opens again, and land it; thx for I don't know if you applied bug 499304 but remember that needs to go in first or else you need to have me fix this patch to build on trunk.

just to save time when you are ready to land this. I did a diff off the trunk as of today.

Comment on attachment 389324 [details] [diff] [review] patch diff to trunk I tweaked the variable name and added some comments to make it clearer what's going on, and checked in. Thx for the patch.

fix checked in.

Comment on attachment 387814 [details] [diff] [review] header file change see other version for comments clearing request

oups