Closed Bug 402392 Opened 17 years ago Closed 12 years ago

Support other message storage formats. (prelude to pluggable mail stores)

Categories

(MailNews Core :: Backend, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 12.0

People

(Reporter: jcranmer, Assigned: Bienvenu)

References

(Depends on 2 open bugs, Blocks 6 open bugs, )

Details

Attachments

(3 files, 24 obsolete files)

387.83 KB, patch
Details | Diff | Splinter Review
941 bytes, patch
Bienvenu
: feedback+
Details | Diff | Splinter Review
429 bytes, patch
Details | Diff | Splinter Review
Per a discussion on IRC (on a weekend, unfortunately), an idea was raised to have pluggable file format supports for message storage.

Questions I had:
1. What level should this be? Compile-time, profile, account, or folder ?
2. Which format should be the default?
3. What formats should we support?
4. What scope should we have for choices? Choosing *.msf/*.mab formats as well?

jwq's answers:
1. mailbox storage formats should be choosable on a per-account basis at account setup time. In an ideal word, they'd be changeable after this as well.
2. mbox should be the default for the reason set out in <http://www.jwz.org/doc/mailsum.html> and the pro-mbox arguments in Bug 58308 and Bug 361807. Basically, compatibility.
3. We should support whatever we (i.e. you guys :-) ) can code. There are requests for maildir and SQLite db.
4. The .msf format should change with the account choice. The only strong opinion on .mab I have is that it shouldn't be mork.
mbox falls over as your storage size increases.

http://www.jwz.org/doc/mailsum.html was not totally idiotic in 2000 but with the storage sizes entities like google offer now it's clear we're well past the limit mbox is a good idea (at least for big mail users)

That thunderbird is not ready for 2007 mail needs is in big part due to following blindly http://www.jwz.org/doc/mailsum.html (change nothing! is always a powerful argument).
How is this really different from bug 58308? I mean, there's no way we could drop mbox even if maildir got implemented. (They would have to both work.)
This is different because this is a meta-bug that includes any other format, depending on other sub-bugs such as bug 58308 for Maildir format or bug 361807 for SQLite format.
Is there a log of the chat from IRC?

My answers for Joshua Cranmer:
1. Folder - Mail, regardless of format, could be imported by moving the file into the directory such as is possible now.

2. I agree with jwq - mbox has been around the longest and is more compatible with most other mail clients.  IMO - it is probably the simplest format of any available.  Any other formats should be available from addons.mozilla.org.

3. IMO - This should not necessarily be a question for this topic.  Thunderbird should have a mail API that accepts any format.  The plugin should have to deal with any calls to the message store.  This will help keep the source clean and more modular.  The API would make the core program transparent to the storage method.
Certain hooks and calls should be available from the API - such as the availability to create threads for various storage tasks and a call to flush buffers for certain events (shutdown, folder compacting, etc.)

Should folders be able to have more than one storage plugin?  Bug 290057 and Bug 377249 (Apple Spotlight and Vista Desktop search) might be able to use this API framework (or a part of it) as well.  It might be necessary for someone to have a live off-site backup of their email in case of hardware failure.

4. I do not know if this kind of framework already exists for the address book, but it would be neat to be able to have an a-book in a corporate environment that worked from an SQL DB over a network.


Out of curiosity, aside from obvious OS and filesystem limitations, are there any metrics available that show the size limits of the mbox format especially in regards to the implementation in Thunderbird?  What are the limitations of other possible storage methods?  Are the limits artificially imposed by hardware and other software interactions such as Anti-Virus or spyware?
I've had similar discussion on the mailing lists some months ago (dev-apps-seamonkey@lists.mozilla.org Jun/Jul 2007), the summary (according to Joshua Cranmer's questions) would be:

1. Per folder setting with the possibility to easily convert from one to another format being strongly desirable (yet quite easily solvable).

2. default per mail/newsgroup account. Global default for now should be probably mbox (compatibility reasons as mentioned in comment #0)

3. Split message handling into two parts - general message functions which call backend handlers. Available backend selection at compile time or via plug-ins. Formats should include mbox, maildir++, some db (sqlite, mysql, ...), imap and pop3. Well-defined API will give possibility for other formats (e.g. encrypted versions of these as well - could be done in an VFS-like fashion)).

4. Everyone wants to get rid of mork.

The biggest problem is probably the fact, that as the existing code is very complex, people willing to do the coding (including myself) will need lots of guidance from experienced mozilla hackers.
I'd vote against a per-folder selection of the mail storage backend. The reason is that searching and overall mail handling like moving from one folder to another can be done much more efficient when all folders are in the same storage domain. This obviously only holds true for systems that use a single file, like e.g. SQLite. For Maildir, it's all the same.

Also note that Thunderbird is used by many users that have no idea how their mail is actually stored on disk. And they don't care! But it's only those "insiders" who may require any sort of compatibility at all. So I'd suggest, considering the far more stable/reliable/performant mechanics of Maildir or a database, selecting one of those formats for newly created accounts. Importing accounts (like when updating Thunderbird) should of course keep whatever is selected, currently mbox.
> 2. mbox should be the default for the reason set out in
<http://www.jwz.org/doc/mailsum.html> and the pro-mbox arguments in Bug 58308
and Bug 361807. Basically, compatibility.

I think most users would prefer a GUI for import/export, so compatibility is really no issue for them.
Using mbox for compatiblity seems like a weak argument, especially when there are reliability issues.

> The reason
is that searching and overall mail handling like moving from one folder to
another can be done much more efficient when all folders are in the same
storage domain.

You're not required to use multiple storage domains, are you?
(In reply to comment #0)
> Questions I had:
> 1. What level should this be? Compile-time, profile, account, or folder ?

The finer granularity we have, the more complexity that is required, the bigger the testing matrix, etc.  I suspect that having more flexibility than per-profile is likely to be more effort than it's worth.

> 2. Which format should be the default?

We can't possibly answer this until we have all the other questions answered as well as some amount of code written so that we can compare the various possible implementations.  It's also going to be a huge distraction from all the rest of the concrete work described here.  Let's worry about that later.

> 3. What formats should we support?

We should have a generic API that can be implemented for any arbitrary backend, including by an extension.  Some obvious existing candidates for implementation include:

mbox/msf
mbox/sqlite
maildir/sqlite
sqlite/sqlite

> 4. What scope should we have for choices? Choosing *.msf/*.mab formats as well?

Crawl -> walk -> run would suggest the following steps:

1) make the existing API plugable
2) change the existing mailbox/msf impl to purely use the plugable API
3) write one or more other impls, modifying the plugable API as necessary

Product: Core → MailNews Core
Something to condisider may be that incremental backups (such as Mozy but perhaps others as well) seem to re-backup a file everytime it changed. So for large mbox files (or any other monolithic format) that means large re-uploads for the smallest change.

I'm not sure what the right solution to this particular issue is (in other words, which pluggable format would solve it, if any), but I do think it gives a small additional impetus to the overall change being suggested in this bug.  It doesn't seem solvable to me until it becomes possible to use other formats.
As for backups, rsync might help. For local backups carefully selected revision control system might be of use (a bit overkill though).
Would you consider creating a storage module based on a new XML format?

As an emerging standard, this has its drawbacks.  But this article makes it sound promising.
http://futurearchives.blogspot.com/2008/09/xml-schema-for-archiving-email-accounts.html
Page 4, discusses XML compatibility with RFC 2822.
http://www.digitaleduurzaamheid.nl/bibliotheek/docs/email-xml-imp.pdf

From a superficial point of view, it would seem it could be on par with maildir in performance, but have the added benefits of being readable by a larger set of apps and already be in a archiavable format.
IMO, XML is no more stable than mbox and should therefore not be used. (Or was it the other bug that was about mbox stability issues?)

Nothing seems to happen anyway. My Dad's mbox files were totally **** - again - yesterday. Thunderbird even ended up uploading this impossible garbage to the IMAP server exceeding its huge quota by 400%. The solution was to kill Thunderbird, erase all local mail storage files, delete and recreate the Trash directory on the mail server (Maildir! contained ~67,000 files - poor filesystem had to work hard) and restart Thunderbird then. Absolutely no chance to repair it with a client alone.
(In reply to comment #11)

> From a superficial point of view, it would seem it could be on par with maildir
> in performance, but have the added benefits of being readable by a larger set
> of apps and already be in a archiavable format.

Thunderbird is about the only major FLOSS software involved in mail that does not do maildir. None that I know of uses your format. That relativises the "larger set of apps" bit
Please do not request specific storage formats in this bug: this bug is about the backend architectural work needed to support pluggable storage formats.
Does it really have to be "pluggable" only because we cannot decide on one of the superior storage formats? This sounds like a lot more work that doesn't even have a direct result. (You still need the plugs then.) After all the average user certainly doesn't want to mess with this.
Yes, it has to as it offers greater flexibility to both users and developers.
Just a note is that another benefit of this change might be to alleviate Bug #384017.
Depends on: 492044
Depends on: 527654
Assignee: nobody → bienvenu
Attached patch pluggable store wip (obsolete) — Splinter Review
this is the start of the pluggable store work. Do *not* try running with it, since it will eat your mail and emit a satisfied burp.

So far, I've got pop3 using the berkeley mailbox pluggable store for 
1. folder discovery
2. new folder creation
3. pop3 download (no filters, please)
4. reading pop3 messages
5. renaming folders
6. deleting folders

Next, I'm going to work on message move/copying, and flag setting. There's still a ton of core code that assumes berkeley mailboxes and our current profile directory structure, but that will slowly get excised.
David, are you planning to create a branch in the mercurial repository (comm-central), where we could follow the development?
Petr, yes, I'm still planning on doing that. I'd like to get a little further along, but I haven't had a chance to work on this for a month...
Some comments on the WIP:

How is UI going to know which pluggable storage type is available? There's likely to be so few types that "hardcoding" (à la account wizard's creation screen) is feasible, but a more extensible approach would be to use a category-manager based approach, like import.

What about offline message stores? Presumably, they could rely on nsIMsgIncomingServer::msgStore for the offline message store. However, I can see this causing some forward compatibility issues, although I suspect that such a thing is a non-goal.

This patch also seems to entrench a separation of the database from the message store. While I'm not advocating taking the time now to brute-force other database types into the picture, the early comments on this bug do suggest that message stores should eventually be allowed to bring their own databases. From an API perspective, I guess, it means that nsILocalFile getSummaryFile(in nsIMsgFolder aFolder); would instead be nsIMsgDatabase getDatabase(in nsIMsgFolder aFolder);

While I'm already spamming people, let me invert the dependencies of the bug.
No longer depends on: 58308, 361807, 382116, 492044, 527654
Keywords: meta
(In reply to comment #21)
> How is UI going to know which pluggable storage type is available? There's
> likely to be so few types that "hardcoding" (à la account wizard's creation
> screen) is feasible, but a more extensible approach would be to use a
> category-manager based approach, like import.

yeah, category manager is typically how this has been done in the past. But I haven't thought about how the UI will expose pluggable stores at all and I'm not sure that we will out of the box, since we really want to push towards maildir. Which is not to say that extensions couldn't add UI.

> 
> What about offline message stores? Presumably, they could rely on
> nsIMsgIncomingServer::msgStore for the offline message store. However, I can
> see this causing some forward compatibility issues, although I suspect that
> such a thing is a non-goal.

They'll use the incoming server's pluggable store, yes. I haven't worried about migration yet, though it's conceptually pretty simple. Offline stores have the same or less forward compatibility issues as local folders.

> 
> This patch also seems to entrench a separation of the database from the message
> store. While I'm not advocating taking the time now to brute-force other
> database types into the picture, the early comments on this bug do suggest that
> message stores should eventually be allowed to bring their own databases. From
> an API perspective, I guess, it means that nsILocalFile getSummaryFile(in
> nsIMsgFolder aFolder); would instead be nsIMsgDatabase getDatabase(in
> nsIMsgFolder aFolder);

That's outside the scope of what I was going to do initially. In theory, .msf file manipulation could be pushed into the nsIMsgDatabase interface or the nsIMsgDBService interface, if someone convinced me that plugging an other database under nsIMsgDatabase was a truly useful thing.
This moves the updating of folder flags out of the nsMailDatabase.cpp code into the pluggable store code (though I haven't converted all the callers to go through the right code yet, I don't think).

I tweaked the API some to support stream re-use, and to allow the store to override message move/copies (though I haven't hooked up that call yet either).
Attachment #439407 - Attachment is obsolete: true
This adds support for editing keywords (untested), calls the pluggable store copy method in case the store wants to override copy, and redoes the uidl state handling to use the pluggable store.
Attachment #444415 - Attachment is obsolete: true
The next 3 things I plan on doing are moving the berkeley mailbox rebuild index and folder compaction code into the pluggable store, and make the imap&news offline store code use a pluggable store.
Attachment #447013 - Attachment is obsolete: true
Attached patch get last patch to compile (obsolete) — Splinter Review
This patch builds (specifically on platforms that do movemail), and uses the pluggable store for offline storage.
Attachment #448231 - Attachment is obsolete: true
Attachment #448644 - Attachment is obsolete: true
I need to figure out what to do about compacting offline stores. Right now, we copy messages whose headers have the offline flag set to a temp offline store, and then copy that offline store over the original. I don't think we can do what we do for compacting normal local folders because we need to set the message offset for each message while we're copying it, at least for the berkeley mailbox offline storage format. For maildir, deleting an imap message should delete the message file immediately (so I probably need to add a call to the pluggable store delete method when an imap message is deleted).
This changes the name of the input stream getter to getMsgInputStream to be consistent with the other methods, and makes the berkeley mailbox store seek to the end of the stream before returning the stream, so that it's "write ready", and removes the seeks the core code was doing on the returned store. I also fixed the berkeley mailbox code that gets an offline store output stream to create the output file if it doesn't exist.
Attachment #449168 - Attachment is obsolete: true
Attachment #449645 - Attachment is obsolete: true
used the assertion about not calling FinishNewMessage to fix some cases where we weren't calling FinishNewMessage.
Attachment #450798 - Attachment is obsolete: true
I've been following this bug, waiting to see where you were going before I created my own message store. I've got a few comments.

First, I don't see an overall philosophy of sync versus async for this. It would be good to take the opportunity to make sure that all calls have a clear plan for whether they are handled async or not, and if async document what is the notification method for them. So, for example, you have:

void DiscoverSubFolders(in nsIMsgFolder aParentFolder, in boolean aDeep);

Is that allowed to be async? If so, what is the notification for completion? This should really be in the .idl documentation.

Second, I seem to be bothered by:

void initWithServerPath(in nsILocalFile aServerPath);

That is, you seem to be clearly thinking of file-oriented message stores. There are only a few places where you assume that in the interface. It seems to me that it would make more sense to use a more generic term here, such as the URL spec for the server, and let the message store decide whether it wants to use that as a map to a local folder, or something else (such as a database connection). I realize you are hoping to allow people to move file storage to another location, but I believe that should be a specific preference on the datastore, and not forced onto this interface.

I'd also be curious to know when you think this is stable enough that I could start trying to use it myself in a message store.
(In reply to comment #33)
> I've been following this bug, waiting to see where you were going before I
> created my own message store. I've got a few comments.

Cool, please keep letting me know of issues as they arise.

> 
> First, I don't see an overall philosophy of sync versus async for this. 

rule of thumb - if there's no listener, then it's got to be sync. And if there is a listener, than it's allowed to be async. But you're right; it should be documented in the idl. In general, I'd like as much as possible to be async/


> would be good to take the opportunity to make sure that all calls have a clear
> plan for whether they are handled async or not, and if async document what is
> the notification method for them. So, for example, you have:
> 
> void DiscoverSubFolders(in nsIMsgFolder aParentFolder, in boolean aDeep);
> 
> Is that allowed to be async?

This is really a request for what folders are known about in the store, not what's on the server. For things like IMAP, there's still a reconciliation that happens between the folder list on the server, and what we know about locally.

I don't think the pluggable store is the mechanism we'd use to add new server objects (e.g., Exchange, or some other datasource). I think for that we'd be better off defining a new, simple interface, something like nsIMsgIncomingServer-lite-lite, with just a few methods. But that's something we can discuss at the summit, or earlier.

> This should really be in the .idl documentation.
> 
> Second, I seem to be bothered by:
> 
> void initWithServerPath(in nsILocalFile aServerPath);
> 
> That is, you seem to be clearly thinking of file-oriented message stores.

I have to support file-oriented message stores. A sqlite store could completely ignore this method if it wants. But this is the root of the per-server profile directory. If the store doesn't care where that directory is, it can ignore this. sqlite does need a place to put the db, of course, but a sqlite store probably wants one global store, not a per-server one...

I suppose that method could take the server object, and not the path, and file-oriented stores could ask the server for their root directory. Other kinds of stores could set attributes on the server object that could be retrieved by the store.

 
> I'd also be curious to know when you think this is stable enough that I could
> start trying to use it myself in a message store.

It's pretty stable for me, but I don't let it touch my main profile, since the initial code ate local mail folders. But nothing bad has happened to my test profile in a month.  The API is changing a bit as we try to add maildir support, but I think it's more valuable to try to do your store now, while the API is still malleable.
This work is now going on in one of my hg repo's : http://hg.mozilla.org/users/bienvenu_nventure.com/plugstore/

I've integrated Andrey's maildir pluggable store module and turned on maildir support by default. It doesn't quite work yet, but we're making good progress. But don't even think of pointing a build from that repo at an existing profile!
Sorry to not raise this point before, but would it be possible for there to be a variant of getMsgInputStream that does not require an nsIMsgDBHdr?  Or perhaps leave it requiring an nsIMsgDBHdr but for maildir (and SQLite if it gets implemented) document very explicitly what bits of nsIMsgDBHdr it wants to use/what properties it wants to read?

The goal is for gloda to be able to stream a message without having the nsIMsgDatabase loaded.  I think we've discussed such a thing in the past.
(In reply to comment #36)
> Sorry to not raise this point before, but would it be possible for there to be
> a variant of getMsgInputStream that does not require an nsIMsgDBHdr?

No problem, now is the time to hash these things out.

> perhaps leave it requiring an nsIMsgDBHdr but for maildir (and SQLite if it
> gets implemented) document very explicitly what bits of nsIMsgDBHdr it wants to
> use/what properties it wants to read?

The store needs some way of knowing what message to get the input stream for, obviously, and different stores need different pieces of information (berkeley mailbox needs a message offset, maildir needs a filename, sqlite probably will need some sort of key). All of these could be expressed as a string, I suppose. So we could have a well-known string attribute on the nsIMsgDBHdr storeKey and have the store set that attribute when it stores the message, and pass that in to getMsgInputStream from the places we call it.
> 
> The goal is for gloda to be able to stream a message without having the
> nsIMsgDatabase loaded.  I think we've discussed such a thing in the past.

Would gloda cache the store key, then? It would still have to get from the db hdr at some point, which would mean the db would have had to be open at some point.
(In reply to comment #37)
> So we could have a well-known string attribute on the nsIMsgDBHdr storeKey and
> have the store set that attribute when it stores the message, and pass that in
> to getMsgInputStream from the places we call it.

This is what I've done - getMsgInputStream now takes a folder and a storeToken (a cstring) for the message. It's stored as a string property "storeToken" on the nsIMsgDBHdr.

Andrey, this will invalidate any existing maildir store you have since I changed the attribute property from the maildir-specific "FILENAME" to the generic "storeToken" which all stores should use, as documented in the .idl
Great, I'll update maildir shortly.
Has anyone tried using Tokyo Cabinet as a message store?
In my experience, it behaves very nicely until ~10mln keys is reached and works very nicely with LZO (& other compression schemes)
I realize it's not a "standard" store but the average user doesn't care (especially as long as there's a way to convert back and forward)
Attached patch cumulative mailnews diffs (obsolete) — Splinter Review
this is just a diff of the  mailnews files in my pluggable store repo vs comm-central trunk. It's not completely clean, in that there are some unrelated changes), but it gives some idea of the changes coming. I'll clean up this patch, run it through review board, extract anything that could land on the trunk, look at remaining TODO items, etc.
Attachment #451150 - Attachment is obsolete: true
This is the current diff between my repo and the trunk. It's down to 370K, so I'm making progress at making the diff minimal :-)
Attachment #541867 - Attachment is obsolete: true
Attached patch patch for review (obsolete) — Splinter Review
Might as well get the review process started. I'm going to request a try server build with this patch as well.

I'm running this patch against my production profile, but most of my accounts are IMAP. You should be cautious about using this against a production profile.

In order to handle existing profiles, I had to make GetMsgInputStream take an optional msgHdr. The alternative was not to support going back and forth between builds that use pluggable store and not. Gloda can still avoid holding onto msghdrs by checking if the store token is empty or not.
Attachment #544089 - Attachment is obsolete: true
Attachment #549985 - Flags: superreview?(neil)
Attachment #549985 - Flags: review?(mbanner)
Comment on attachment 549985 [details] [diff] [review]
patch for review

Review of attachment 549985 [details] [diff] [review]:
-----------------------------------------------------------------

Some random drive-by comments...

::: mailnews/base/public/nsIMsgIncomingServer.idl
@@ +168,5 @@
>    attribute nsILocalFile localPath;
>  
> +  /* The pluggable store interface to be used for this server */
> +  readonly attribute nsIMsgPluggableStore msgStore;
> +

Doxygen syntax, please?

::: mailnews/db/msgdb/public/nsIMsgDatabase.idl
@@ +286,5 @@
>     *                        The database is present (and was opened), but the
>     *                        summary file is missing.
>     */
> +//  void Open(in nsILocalFile aFolderName, in boolean aCreate,
> +//            in boolean aLeaveInvalidDB);

Why is this commented out?

::: mailnews/local/public/nsIMsgParseMailMsgState.idl
@@ +63,4 @@
>      void Clear();
>  
>      void ParseAFolderLine(in string line, in unsigned long lineLength);
> +  attribute nsIMsgDBHdr newMsgHdr;

No doc comment?

::: mailnews/local/test/unit/test_over2GBMailboxes.js
@@ +2,5 @@
>  
>  /* Test of accessing over 2GB local folder */
>  
> +Services.prefs.setCharPref("mail.defaultStoreContractID",
> +                           "@mozilla.org/msgstore/berkeleystore;1");

Is it worth making a global utility function "forceMboxStorage()" or something for tests?
try server builds were giving a mac error that I'm not seeing, but I was seeing a different error, which I fixed. I've requested a new try server build.
Attachment #549985 - Attachment is obsolete: true
Attachment #550100 - Flags: superreview?(neil)
Attachment #550100 - Flags: review?(mbanner)
Attachment #549985 - Flags: superreview?(neil)
Attachment #549985 - Flags: review?(mbanner)
I wish the message arrays were normal arrays and not nsIArrays :-(

(From update of attachment 550100 [details] [diff] [review])
>-        server = do_QueryElementAt(allServers, ++serverIndex);
>+        server = do_QueryElementAt(allServers, serverIndex++);
>       }
>       while (serverIndex < numServers);
I don't think this change is correct. You might want to move the preincrement to the condition (thus avoiding repeating yourself before the continue).

>+    if (storeContractID.IsEmpty())
IMHO we need to assume an mbox store in this case. We can have a pref for the default, but we would have to copy it for each new server.

>+NS_DEFINE_NAMED_CID(NS_BRKMMBOXSTORE_CID);
Too many Ms?

>+    nsIMsgDatabase *msgDB = pMessageDB->m_folder ?
nsCOMPtr and #ifdef DEBUG (or I believe it is possible to write this as a single assertion statement.)

>+  nsString leafName
#ifdef DEBUG

>   nsCOMPtr <nsIMsgDatabase> msgDB = do_CreateInstance(NS_MAILBOXDB_CONTRACTID, &rv);
>   NS_ENSURE_SUCCESS(rv, rv);
>-  rv = msgDB->Open(aFolderName, aCreate, aLeaveInvalidDB);
>+  nsMsgDatabase *msgDatabase = static_cast<nsMsgDatabase *>(msgDB.get());
>+  rv = msgDatabase->Open(dbPath, aCreate, aLeaveInvalidDB);
It would possibly be better to use
nsRefPtr<nsMsgDatabase> msgDatabase = new nsMsgDatabase();

-NS_IMETHODIMP nsMsgLocalMailFolder::CreateLocalSubfolder(const nsAString &aName,
Method seems to have moved, but see below

NS_IMETHODIMP nsMsgLocalMailFolder::AddSubfolder(const nsAString &name,
Could probably simply remove this override completely.

To be continued...
(In reply to neil@parkwaycc.co.uk from comment #48)
> >+  nsString leafName
> #ifdef DEBUG

Would having thse in PRLOG useful ?
I started at the end working backwards this time, but failed to meet in the middle...

(From update of attachment 550100 [details] [diff] [review])
>+  if (NS_FAILED(NS_CopyUnicodeToNative(safeName, newDiskName)))
...
>+  rv = oldPathFile->MoveToNative(nsnull, newDiskName);
rv = oldPathFile->MoveTo(nsnull, safeName);
Similarly for the .msf, although you managed to do the .sbd correctly.

>+  nsCOMPtr<nsISupports> supports = do_QueryInterface(static_cast<nsIMsgParseMailMsgState*>(this));
I suppose it's possible that the line you copied predated
CallQueryInterface(this, getter_AddRefs(supports));
Or these days you can even do
nsCOMPtr<nsISupports> supports = do_QueryObject(this);

>+    // Only berkeley store cares about quaranting, I think. We should probably
>+    // make this an attribute on the pluggable store, though.
Would it be possible for the berkeley store to handle the quarantining i.e. when you get a new message and the pref is set, it always creates a temp file, then when you finish or discard the new message it can clean up appropriately?
Comment on attachment 550100 [details] [diff] [review]
fix movemail bustage and address drive by comments

>+ * The Initial Developer of the Original Code is
>+ * Mozilla Messaging.
MoFo?

>+ * If no room, we can't do anything until the folder is compacted and another
>+ * x-mozilla-keys header is added. In that case, we set a property
>+ * on the header, which the compaction code will check.
Does this apply to maildir stores?

>+#ifdef MOZILLA_INTERNAL_API
>+        PRInt32 errorCode = 0;
>+#else
>+        nsresult errorCode = NS_OK;
>+#endif
You can safely use nsresult even without including nsMsgUtils.h now.

>+  virtual ~nsMsgLocalStoreUtils();
Not sure why you need this.

>+  nsresult AddDirectorySeparator(nsILocalFile *path);
>+  PRBool nsShouldIgnoreFile(nsAString& name);
>+  void ChangeKeywordsHelper(nsIMsgDBHdr *message,
Shouldn't these be static?
Comment on attachment 550100 [details] [diff] [review]
fix movemail bustage and address drive by comments

>+  // we have to treat the root folder specially, because its name
>+  // doesn't end with .sbd
This comment may be lost. It looks relevant, but not to this exact spot.

>+  PRInt32 newFlags = nsMsgFolderFlags::Mail;
>+  if (directory)
>+  {
>+    newFlags |= (nsMsgFolderFlags::Directory | nsMsgFolderFlags::Elided);
>+    aParentFolder->SetFlag(newFlags);
Not quite sure what the point of this is, it seems to be the same as
aParentFolder->SetFlag(Mail | Directory | Elided);

>+  rv = NS_CopyUnicodeToNative(safeFolderName, nativeFolderName);
>+  if (NS_FAILED(rv) || nativeFolderName.IsEmpty())
>+    return NS_MSG_ERROR_INVALID_FOLDER_NAME;
>+
>+  path->AppendNative(nativeFolderName);
MsgHashIfNecessary would have prevented this error, but there's no point copying the folder name to native to pass to AppendNative, you should always work with files in Unicode if you have it i.e. path->Append(safeFolderName);

>+  child.swap(*aResult);
child.forget(aResult);

>+  rv = aFolder->GetFilePath(getter_AddRefs(pathFile));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  newSummaryLocation->InitWithFile(pathFile);
GetFilePath returns a new file object each time, so you can just write
rv = aFolder->GetFilePath(getter_AddRefs(newSummaryLocation));
etc.

>+  NS_IF_ADDREF(*aSummaryFile = newSummaryLocation);
newSummaryLocation.forget(aSummaryFile);

>+  // clone file because nsLocalFile caches sizes and dates.
Ditto.

>+  PRTime  temp64;
>+  PRInt64 thousand;
>+  LL_I2L(thousand, PR_MSEC_PER_SEC);
>+  LL_DIV(temp64, lastModTime, thousand);
>+  LL_L2UI(*aDate, temp64);
All our compilers are 64-bit safe, so we shouldn't need these macros.

>+  // Since we support file systems w/o truly case-sensitive file names,
>+  // Check if we're just changing the case of the name.
>+  if (existingName.Equals(aNewName, nsCaseInsensitiveStringComparator()))
>+    return NS_MSG_FOLDER_EXISTS;
[Always wondered what the point of this was... as far as I know the OS doesn't mind if we rename files to change their case.]

>+  nsCOMPtr<nsISupports> parentSupport = do_QueryInterface(parentFolder);
Unused?

>+  if (NS_FAILED(NS_CopyUnicodeToNative(safeName, newDiskName)))
>+    return NS_ERROR_FAILURE;
>+
>+  nsCAutoString oldLeafName;
>+  oldPathFile->GetNativeLeafName(oldLeafName);
More native encoding stuff to avoid like the plague.

>+    localDirFile->MoveTo(nsnull, newNameDirStr);
You got this one right :-)

>+      nsCOMPtr<nsISupports> srcSupport(do_QueryInterface(aSrcFolder));
>+      localNewFolder->OnCopyCompleted(srcSupport, PR_TRUE);
[I should file a bug to change this to use a more useful type.]

I'm confused. There's GetNewMsgOutputStream, which removes a stream from the hash, but adds a new stream to the hash; there's GetMsgInputStream, which removes a stream from the hash, but doesn't add one; there's GetOutputStream, which doesn't remove a stream in the hash, but adds a new stream to the hash.
(In reply to Ludovic Hirlimann [:Usul] from comment #49)
> (In reply to neil@parkwaycc.co.uk from comment #48)
> > >+  nsString leafName
> > #ifdef DEBUG
> 
> Would having thse in PRLOG useful ?

No, I think it would only happen if we introduced a new bug, which we should catch in development, not in the wild.
This attempts to address almost all of Neil's comments, and fixes the one xpcshell test failure I (and I believe Mark) was seeing.

The one comment I haven't addressed yet is the stream caching question Neil had. 

I think other stores will probably want the option of doing quarantining - I know there's interest in doing a variant on berkeley mailbox as a pluggable store.

maildir stores don't currently handle keyword field overflow in the face of database regeneration. I've put a comment in the code to that effect. maildir stores db's shouldn't get regenerated as often as berkeley stores, but we should fix it before we let the user choose maildir stores.
Attachment #550100 - Attachment is obsolete: true
Attachment #562504 - Flags: superreview?(neil)
Attachment #562504 - Flags: review?(mbanner)
Attachment #550100 - Flags: superreview?(neil)
Attachment #550100 - Flags: review?(mbanner)
Some drive-by comments:
1. At least nsIMsgIncomingServer and nsIMsgDBService appear to be missing a uuid rev.
2. The file offset parameter for the input stream-for-message method on nsIMsgFolder goes from an unsigned value to a signed value; this appears to be because of the seek parameter in nsISeekableStream. However, the pluggable store still has the unsigned value. I'm guessing the original position values were derived from the unsigned long long messageOffset on nsIMsgDBHdr.

The disparity between unsigned and signed values for offset locations is liable to cause problems, especially if some underlying API chooses to return -1 and that gets converted into an unsigned value. I think I would like to see a more amicable resolution of the unsigned/signed disparity, but I think this can be handled in a follow-on patch.

3. Something else that brought up to my mind is that it's not clear to me how some of the implicitly-used-by-mbox attributes interact with maildir, particularly nsIMsgDBHdr::messageOffset. Again, if you're punting on some maildir stuff already, you are more than welcome to defer this kind of documentation in my eyes. Especially since it's the kind of thing that ought to go on MDC instead of in the code base.

4. I've been told s/Mozilla Messaging/Mozilla Foundation/ in the copyright headers in new files is more accurate now, but you might want verification on that.

5. Nits:
  Misaligned * on nsIMsgPluggableStore.idl:156
  The new nsIMsgIncomingServer attribute has a half-completed doc comment

6. Another design point that hit me recently. Compaction is an asynchronous event while most operations are synchronous. Suppose someone starts a compaction and then adds a new message to the folder. Should my folder store:
  a) refuse to add a new message,
  b) add the new message immediately and then try to make compaction handle that scenario, or
  c) block on adding the new message until after compaction completes?
  If the assumption is b, I can see people attempting to make custom stores failing hard and causing potential dataloss. I can also see that a lot of calling code would not be happy with a or c as alternatives.

In short, I think there ought to be a strong MDC guide on how to write a new pluggable store, as well as a pretty thorough test-suite to help ensure that authors of new stores are not introducing extra dataloss scenarios.
Your pluggable store should lock the folder during the time that it can't handle messages getting added to a folder. This has always been true of any code in the backend so it's not any different with pluggable stores, except that pluggable stores have to play by the same rules. But in general, these things should be documented.
the try server build of the latest patch passed all the xpcshell tests, but there are some mozmill failures which I'll look into today.
Bug 681188 bit-rotted the hell out of this patch, so I'll try to get a new patch up in a day or two...
Attachment #562504 - Attachment is obsolete: true
Attachment #562504 - Flags: superreview?(neil)
Attachment #562504 - Flags: review?(mbanner)
Attached patch de-bitrotted patch (obsolete) — Splinter Review
still has mozmill failures, which I have yet to figure out.
Attachment #563554 - Attachment is obsolete: true
Re the stream caching in the berkeley mailbox pluggable store, I had wanted to make it easy for client code to have streams cached, to avoid re-opening the same file over and over again. But it turns out that StreamMessage opens a stream, but makes it a bit awkward to access the stream after the fact, and if the stream is cached, that leaves the file locked. This breaks the folder compaction code, which streams a bunch of messages, and then tries to move the temp file over the original folder. This fails because we still have an input stream opened on the original folder.

The folder compaction code is technically part of the berkeley pluggable mail store, so it could handle this internally somehow. But I rather suspect it might take a while to whack all the moles input stream caching might cause. I've ameliorated some of the performance issues by having the pluggable store interfaces take batches of message to perform operations on, so the pluggable store can use the same stream.
I'll request a try server build w/ this patch.
Attachment #563560 - Attachment is obsolete: true
Attachment #564560 - Flags: superreview?(neil)
Attachment #564560 - Flags: review?(mbanner)
try server builds mostly failed because try server is broken, but I think there were successful mozmill and xpcshell tests on at least one platform.
Comment on attachment 564560 [details] [diff] [review]
fix mozmill tests, and address more review comments

Review of attachment 564560 [details] [diff] [review]:
-----------------------------------------------------------------

Initial comments for idls and other build config files. More coming later.

::: mailnews/base/public/nsIMsgFolder.idl
@@ +570,5 @@
> +   *
> +   * @param aHdr hdr of message to get outputstream for
> +   * @returns An output stream to write to.
> +   */
> +  nsIOutputStream getOfflineStoreOutputStream(in nsIMsgDBHdr aHdr);

nit: blank line after this line please (and actually before the comment as well).

::: mailnews/base/public/nsIMsgPluggableStore.idl
@@ +77,5 @@
> +   *                      This will be the root folder for the server object.
> +   * @param aDeep true if we should discover all descendents. Would we ever
> +   *              not want to do this?
> +   */
> +  void discoverSubFolders(in nsIMsgFolder aParentFolder, in boolean aDeep);

nit: need a blank line after this line.

@@ +107,5 @@
> +   */
> +  void deleteFolder(in nsIMsgFolder aFolder);
> +
> +  /**
> +   * Rename an existing folder.

Presumably this is just on the storage and not the server as well? Maybe a better summary would be rename storage for an existing folder.

@@ +177,5 @@
> +   *
> +   * @param aOutputStream stream we were writing the message to be discarded to
> +   * @param aNewHdr header of message to discard
> +   */
> +  void discardNewMessage(in nsIOutputStream aOutputStream,

Its a little confusing to me why we'd start writing a message only to find out it is a duplicate and then abandon that write? Surely we'd have either not started writing the message or completed it by then?

@@ +181,5 @@
> +  void discardNewMessage(in nsIOutputStream aOutputStream,
> +                         in nsIMsgDBHdr aNewHdr);
> +
> +  /**
> +   * Called when the pluggable store has finished writing a message to the

This is the interface for the pluggable store, so this comment doesn't sound quite right.

::: mailnews/mailnews.js
@@ +479,5 @@
>  pref("mail.server.default.autosync_max_age_days", -1);
>  
> +// we don't use mail.server.default because we want to ensure that the
> +// store contract id is always written out to prefs.js
> +pref("mail.defaultStoreContractID", "@mozilla.org/msgstore/berkeleystore;1");

If we change this, will it just affect new folders? or rebuilt or ...?
Comment on attachment 564560 [details] [diff] [review]
fix mozmill tests, and address more review comments

Review of attachment 564560 [details] [diff] [review]:
-----------------------------------------------------------------

Somehow this duplicated the previous review as well as adding what I added this time. Hopefully not next time. Note that I have added an extra couple of comments to nsIMsgPluggableStore.idl in this second step.

::: mail/base/modules/MailUtils.js
@@ +400,4 @@
>          // set the property; this may open the database...
>          folder.setStringProperty(aPropertyName, aPropertyValue);
>          // force the reference to be forgotten.
> +        // folder.msgDatabase = null;

We should probably drop this line and the comment if its no longer required.

::: mailnews/base/public/nsIMsgFolder.idl
@@ +570,5 @@
> +   *
> +   * @param aHdr hdr of message to get outputstream for
> +   * @returns An output stream to write to.
> +   */
> +  nsIOutputStream getOfflineStoreOutputStream(in nsIMsgDBHdr aHdr);

nit: blank line after this line please (and actually before the comment as well).

::: mailnews/base/public/nsIMsgPluggableStore.idl
@@ +77,5 @@
> +   *                      This will be the root folder for the server object.
> +   * @param aDeep true if we should discover all descendents. Would we ever
> +   *              not want to do this?
> +   */
> +  void discoverSubFolders(in nsIMsgFolder aParentFolder, in boolean aDeep);

nit: need a blank line after this line.

@@ +107,5 @@
> +   */
> +  void deleteFolder(in nsIMsgFolder aFolder);
> +
> +  /**
> +   * Rename an existing folder.

Presumably this is just on the storage and not the server as well? Maybe a better summary would be rename storage for an existing folder.

@@ +177,5 @@
> +   *
> +   * @param aOutputStream stream we were writing the message to be discarded to
> +   * @param aNewHdr header of message to discard
> +   */
> +  void discardNewMessage(in nsIOutputStream aOutputStream,

Its a little confusing to me why we'd start writing a message only to find out it is a duplicate and then abandon that write? Surely we'd have either not started writing the message or completed it by then?

@@ +181,5 @@
> +  void discardNewMessage(in nsIOutputStream aOutputStream,
> +                         in nsIMsgDBHdr aNewHdr);
> +
> +  /**
> +   * Called when the pluggable store has finished writing a message to the

This is the interface for the pluggable store, so this comment doesn't sound quite right.

@@ +219,5 @@
> +   * @param aOffset offset in the returned stream of the message.
> +   * @param[optional] aHdr msgHdr to use in case storeToken is not set. This is
> +   *                  for upgrade from existing profiles.
> +   */
> +  nsIInputStream getMsgInputStream(in nsIMsgFolder aFolder,

You need to add aReusable to the documentation.

@@ +221,5 @@
> +   *                  for upgrade from existing profiles.
> +   */
> +  nsIInputStream getMsgInputStream(in nsIMsgFolder aFolder,
> +                                   in ACString aMsgToken,
> +                                   out unsigned long long aOffset,

Shouldn't this just be a long long as that's what nsISeekableStream::Seek takes, and what you've changed elsewhere?

::: mailnews/base/src/nsCopyMessageStreamListener.cpp
@@ -153,4 @@
>  	bool copySucceeded = (aStatus == NS_BINDING_SUCCEEDED);
>  	rv = mDestination->EndCopy(copySucceeded);
>  	//If this is a move and we finished the copy, delete the old message.
> -	if(NS_SUCCEEDED(rv))

I think this function (nsCopyMessageStreamListener::EndCopy) is messy enough and touched enough that we should just re-indent it all.

::: mailnews/base/src/nsMsgFolderCompactor.cpp
@@ +141,1 @@
>                                       getter_AddRefs(m_db));

nit: might as well fix the indentation here.

@@ +493,5 @@
>    {
> +    nsCOMPtr<nsIMsgDBService> msgDBService =
> +      do_GetService(NS_MSGDB_SERVICE_CONTRACTID, &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    rv = msgDBService->OpenFolderDB(m_folder, PR_TRUE, getter_AddRefs(m_db));

nit: this rv isn't checked, should it be?

::: mailnews/base/util/msgStoreMigrator.js
@@ +1,1 @@
> +/* ***** BEGIN LICENSE BLOCK *****

msgStoreMigrator.js - is this actually used at the moment? (it doesn't seem to be included in the build and it looks incomplete).

@@ +41,5 @@
> +
> +var EXPORTED_SYMBOLS = [ "migrateMsgStore" ];
> +
> +Components.utils.import("resource:///modules/errUtils.js");
> +//Components.utils.import("resource:///modules/Services.js");

nit: drop the commented out line.

@@ +43,5 @@
> +
> +Components.utils.import("resource:///modules/errUtils.js");
> +//Components.utils.import("resource:///modules/Services.js");
> +const Ci = Components.interfaces;
> +var gPrefs;

nit: gPrefs isn't used.

::: mailnews/base/util/nsMsgIncomingServer.cpp
@@ +985,5 @@
> +    // that we do always write a store pref.
> +    GetCharValue("storeContractID", storeContractID);
> +    if (storeContractID.IsEmpty())
> +    {
> +      storeContractID.Assign("@mozilla.org/msgstore/berkeleystore;1");

Shouldn't this be using the value of "mail.defaultStoreContractID" from preferences? (which doesn't actually get used anywhere else afaict).

::: mailnews/mailnews.js
@@ +479,5 @@
>  pref("mail.server.default.autosync_max_age_days", -1);
>  
> +// we don't use mail.server.default because we want to ensure that the
> +// store contract id is always written out to prefs.js
> +pref("mail.defaultStoreContractID", "@mozilla.org/msgstore/berkeleystore;1");

If we change this, will it just affect new folders? or rebuilt or ...?
Attachment #564560 - Attachment is obsolete: true
Attachment #565627 - Flags: superreview?(neil)
Attachment #565627 - Flags: review?(mbanner)
Attachment #564560 - Flags: superreview?(neil)
Attachment #564560 - Flags: review?(mbanner)
> Shouldn't this be using the value of "mail.defaultStoreContractID" from
> preferences? (which doesn't actually get used anywhere else afaict).
> 
dueling review comments - Neil pointed out that servers without a pluggable store contract id should definitely use berkeley mailbox. The pref is now intended to be used when new servers are created, but since we haven't hooked up any UI for picking/switching pluggable stores, I haven't done that yet.

> ::: mailnews/mailnews.js
> @@ +479,5 @@
> >  pref("mail.server.default.autosync_max_age_days", -1);
> >  
> > +// we don't use mail.server.default because we want to ensure that the
> > +// store contract id is always written out to prefs.js
> > +pref("mail.defaultStoreContractID", "@mozilla.org/msgstore/berkeleystore;1");
> 
> If we change this, will it just affect new folders? or rebuilt or ...?

defaultStoreContractID is intended to be the default for *new* servers.
(In reply to David :Bienvenu from comment #67)
> > Shouldn't this be using the value of "mail.defaultStoreContractID" from
> > preferences? (which doesn't actually get used anywhere else afaict).
> > 
> dueling review comments - Neil pointed out that servers without a pluggable
> store contract id should definitely use berkeley mailbox. The pref is now
> intended to be used when new servers are created, but since we haven't
> hooked up any UI for picking/switching pluggable stores, I haven't done that
> yet.

My only thought with hooking it up now in at least the backend, is it'll allow those who accept a bit more risk now to start testing maildir. If you think we need a bit more work on maildir before we do that, then lets just drop the mailnews.js pref addition until we're ready to make it do something.

> > ::: mailnews/mailnews.js
> > @@ +479,5 @@
> > >  pref("mail.server.default.autosync_max_age_days", -1);
> > >  
> > > +// we don't use mail.server.default because we want to ensure that the
> > > +// store contract id is always written out to prefs.js
> > > +pref("mail.defaultStoreContractID", "@mozilla.org/msgstore/berkeleystore;1");
> > 
> > If we change this, will it just affect new folders? or rebuilt or ...?
> 
> defaultStoreContractID is intended to be the default for *new* servers.

Hmm, how about calling it mail.serverDefaultStoreContractID ? I know its a bit longer, but it at least puts the server bit in there.
per Comment 68:
> My only thought with hooking it up now in at least the backend, is
> it'll allow those who accept a bit more risk now to start testing
> maildir.

fwiw, I would be available to perform at least minimal (if not more) testing for a maildir-based Thunderbird.  Am desperately seeking solutions to massive problems experienced with TB in large IMAP environments (11+ GB email dataset, 8000+ folders).  Hoping that between an maildir implementation (I suspect the mbox-based "locking" might be contributing to problems in my environment) and a possible fix (that I may try to write) for Bug 685790 that I can somehow get a stable TB in my environment.

So again, am motivated to test--f this effort is looking to produce a maildir-based TB.  Hope to hear from whoever's in leading this charge...
I did a new patch for using the default store when creating new servers, but it's on a different machine that I don't have access to at the moment.

hydrostarr, I can generate a try server build for you to try with maildir support turned on, but it might take a few days for me to get that. It's also the case that you'd need to start with a new profile to get maildir support turned on, because we don't have migration tools yet. But I don't think it will have anything to do with the issues you're seeing.
Comment on attachment 565627 [details] [diff] [review]
address most of the prev comments

Review of attachment 565627 [details] [diff] [review]:
-----------------------------------------------------------------

This time I've reviewed all the unit test changes. I'll aim to give it all a final once over on Monday.

::: mail/test/mozmill/folder-display/test-invalid-db-folder-load.js
@@ +66,4 @@
>   *
>   */
>  function test_load_folder_with_invalidDB() {
> +//  be_in_folder(inboxFolder);

nit: do we need this line?

::: mailnews/base/test/unit/test_bug428427.js
@@ +43,4 @@
>  const dbviewContractId = "@mozilla.org/messenger/msgdbview;1?type=" + "quicksearch";
>  const dbView = Cc[dbviewContractId].createInstance(Ci.nsIMsgDBView);
>  const bugmail1 = do_get_file("../../../data/bugmail1");
> +const msgDBService = Cc["@mozilla.org/msgDatabase/msgDBService;1"]

do we still want this enabled/loaded?

::: mailnews/base/test/unit/test_copyChaining.js
@@ -98,4 @@
>    messages = messages.concat(scenarioFactory.directReply(10));
>    gCopySource = gLocalIncomingServer.rootMsgFolder.createLocalSubfolder("copySource");
>    addMessagesToFolder(messages, gCopySource);
> -

nit: we could probably just drop this file from the diff.

::: mailnews/base/test/unit/test_folderCompact.js
@@ +211,4 @@
>      var testFn = gTestArray[test-1];
> +    // Set a limit of 300 seconds; if the notifications haven't arrived by
> +    // then, there's a problem.
> +    do_timeout(300000, function() {

Why increase the timeout? Is that really necessary?

::: mailnews/base/test/unit/test_nsIMsgFolderListenerLocal.js
@@ +271,1 @@
>      compactFolder(gLocalInboxFolder);

nit: needs indenting

::: mailnews/base/test/unit/test_quarantineFilterMove.js
@@ +117,4 @@
>    if (!gLocalInboxFolder)
>      loadLocalMailAccount();
>  
> +  gMoveFolder = gLocalIncomingServer.rootMsgFolder.

nit: prefer . on the start of the line.

::: mailnews/imap/test/unit/test_imapCopyTimeout.js
@@ +220,1 @@
>      async_driver();

nit: needs indenting.
Attached patch address more review comments (obsolete) — Splinter Review
Attachment #565627 - Attachment is obsolete: true
Attachment #567367 - Flags: superreview?(neil)
Attachment #567367 - Flags: review?(mbanner)
Attachment #565627 - Flags: superreview?(neil)
Attachment #565627 - Flags: review?(mbanner)
Attachment #567367 - Attachment is obsolete: true
Attachment #567437 - Flags: superreview?(neil)
Attachment #567437 - Flags: review?(mbanner)
Attachment #567367 - Flags: superreview?(neil)
Attachment #567367 - Flags: review?(mbanner)
Attachment #567437 - Attachment is obsolete: true
Attachment #567591 - Flags: review?(mbanner)
Attachment #567437 - Flags: superreview?(neil)
Attachment #567437 - Flags: review?(mbanner)
Comment on attachment 567591 [details] [diff] [review]
standard8 points out that I inadvertently removed mailnewsMigrator.js

Review of attachment 567591 [details] [diff] [review]:
-----------------------------------------------------------------

A few more comments, but I think this is ready for landing (modulo getting sr of course).

::: mailnews/local/src/nsMsgBrkMBoxStore.cpp
@@ +89,5 @@
> +                                                    bool aDeep)
> +{
> +  NS_ENSURE_ARG_POINTER(aParentFolder);
> +  bool isServer;
> +  nsresult rv = aParentFolder->GetIsServer(&isServer);

nit: rv not checked.

@@ +446,5 @@
> +  oldPathFile->Clone(getter_AddRefs(dirFile));
> +  nsCOMPtr<nsILocalFile> localDirFile(do_QueryInterface(dirFile));
> +
> +  if (numChildren > 0)
> +    rv = CreateDirectoryForFolder(localDirFile);

nit: rv not checked.

@@ +465,5 @@
> +  {
> +    nsAutoString leafName;
> +    parentPathFile->GetLeafName(leafName);
> +    leafName.AppendLiteral(FOLDER_SUFFIX);
> +    rv = parentPathFile->SetLeafName(leafName);

nit: rv not checked

@@ +541,5 @@
> +  // without copying it. If it fails and filespec exist and is not zero sized
> +  // there is real problem
> +  // Copy the file to the new dir
> +  rv = summaryFile->CopyTo(newPath, EmptyString());
> +  if (! NS_SUCCEEDED(rv)) // Test if the copy is successful

Should be NS_FAILED

::: mailnews/local/src/nsMsgMaildirStore.cpp
@@ +84,5 @@
> +
> +// helper to get file name from nsIFile
> +const char *GetFileName(nsIFile *file)
> +{
> +  static nsCAutoString path;

I'm pretty sure this will officially leak memory, or at least cause a static constructor that I believe we're trying to get rid of. Would be good if we could come up with a different solution here.

@@ +160,5 @@
> +  }
> +  return rv;
> +}
> +
> +NS_IMETHODIMP nsMsgMaildirStore::InitFromServer(nsIMsgIncomingServer *aServer)

Both the InitFromServer functions just check the arg pointer and return nothing. Do we really need them? If we do want them for extensibility, I'm not sure there's any point in having the arg pointer check.

@@ +285,5 @@
> +    path->Remove(PR_TRUE); // recursive
> +    return rv;
> +  }
> +
> +  PR_LOG(MailDirLog, PR_LOG_ALWAYS, ("CreateFolder - create db\n"));

I suspect when we enable this, we don't want to always be logging this (and the other PR_LOGs you've added).

@@ +812,5 @@
> +{
> +  NS_ENSURE_ARG_POINTER(aNewHdr);
> +  NS_ENSURE_ARG_POINTER(aDestFolder);
> +  NS_ENSURE_ARG_POINTER(aResult);
> +  // This should move the new message from the new sub-directory of the inbox to cur sub-directory

I guess this is something to be done when we enable maildir store?
Attachment #567591 - Flags: review?(mbanner) → review+
Attached patch address review comments (obsolete) — Splinter Review
This addresses Standard8's comments. I removed most of the PR Logging from maildir, leaving just a few of the error situations. I also fixed PR_FALSE/PR_TRUE in the files I added, and fixed a bug copying imap offline message bodies. It also quiets a runtime warning trying to get subfolders of non-directories.
Attachment #567591 - Attachment is obsolete: true
Attachment #567920 - Flags: superreview?(neil)
Attachment #567920 - Flags: review+
realized in the middle of the night that the offline store change I made yesterday only worked for Berkeley mailboxes - fixed in this patch.
Attachment #567920 - Attachment is obsolete: true
Attachment #568147 - Flags: superreview?(neil)
Attachment #567920 - Flags: superreview?(neil)
(From update of attachment 568147 [details] [diff] [review])
>+// This is the default store contractID for newly created servers.
>+// Ee don't use mail.server.default because we want to ensure that the
>+// store contract id is always written out to prefs.js
[Very nearly finished superreview. Happened to notice this typo though.]
Comment on attachment 568147 [details] [diff] [review]
fix offline store copying of imap messages

>+      bool isDirectory = false;
>+      currentFile->IsDirectory(&isDirectory);
>+      if (isDirectory && !nsShouldIgnoreFile(leafName))
>+        currentDirEntries.AppendObject(currentFile);
nsShouldIgnoreFile is not much use at this point, since it's a directory.
Where do the "cur", "tmp" and "new" directories get ignored?

>+        bool directory = false;
>+        path->IsDirectory(&directory);
>+        if (directory)
Aren't all folders directories under maildir? After all, we just checked them in the loop above.

>+  path->Append(aFolderName);
IMHO this is unexpected behaviour and the caller should do it.

>+  leaf->SetNativeLeafName(NS_LITERAL_CSTRING("new"));
What's this folder used for?

>+  bool exists;
>+  pathFile->Exists(&exists);
>+  if (exists)
>+    pathFile->Remove(true);
Since you're ignoring the error, why check for existence first?

>+    rv = db->CreateNewHdr(nsMsgKey_None, aNewMsgHdr);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    (*aNewMsgHdr)->SetMessageOffset(0);
[What's the default message offset?]

>+  // generate new file name
>+  nsCAutoString newName;
>+  newName.AppendInt(PR_Now());
Is this going to be sufficiently unique?
Blocks: 703068
Any chances for this making it for Thunderbird 10?
(In reply to heraldo from comment #80)
> Any chances for this making it for Thunderbird 10?

No, TB 10 is already feature frozen. Thunderbird 11 is the earliest if the reviews are completed by then. Note that this also won't change anything by default in TB yet. There will be a bit more work before we can turn on maildir.
(In reply to Mark Banner (:standard8) from comment #81)
> Note that this also won't change anything by
> default in TB yet. There will be a bit more work before we can turn on
> maildir.

I hope something will change. At least, as David wrote in a comment to bug 462665, this fix will
> remove the 4GB limit for berkeley mail stores
Attached patch fix addressing comments (obsolete) — Splinter Review
this patch also implements maildir store copying of messages directly in the store
Attachment #568147 - Attachment is obsolete: true
Attachment #577656 - Flags: superreview?(neil)
Attachment #568147 - Flags: superreview?(neil)
(In reply to neil@parkwaycc.co.uk from comment #79)

> >+      bool isDirectory = false;
> >+      currentFile->IsDirectory(&isDirectory);
> >+      if (isDirectory && !nsShouldIgnoreFile(leafName))
> >+        currentDirEntries.AppendObject(currentFile);
> nsShouldIgnoreFile is not much use at this point, since it's a directory.
nsShouldIgnoreFile also ignores .sbd and .mozmsgs directories.

> Where do the "cur", "tmp" and "new" directories get ignored?
Those directories are under the mail folder directory. Sub-directories of a mail folder are in the .sbd directory.
> 
> >+        bool directory = false;
> >+        path->IsDirectory(&directory);
> >+        if (directory)
> Aren't all folders directories under maildir? After all, we just checked
> them in the loop above.
This is looking at the .sbd dir, not the mail folder directory. I've commented in the code to that effect.
> 
> >+  path->Append(aFolderName);
> IMHO this is unexpected behaviour and the caller should do it.
fixed.
> 
> >+  leaf->SetNativeLeafName(NS_LITERAL_CSTRING("new"));
> What's this folder used for?
It really only applies to server maildir impls, so I've removed it, and added a comment.
> 
> >+    (*aNewMsgHdr)->SetMessageOffset(0);
> [What's the default message offset?]
The message key, for backwards compatibility.
> 
> >+  // generate new file name
> >+  nsCAutoString newName;
> >+  newName.AppendInt(PR_Now());
> Is this going to be sufficiently unique?

Yes, most likely, but I've added a call to CreateUnique just to be a little safer.
Comment on attachment 577656 [details] [diff] [review]
fix addressing comments

>+/**
>+ *Create a Maildir-style folder with "tmp", " and "cur" subfolders
>+ * but no "new" subfolder, because it's not sensical in the mail client context.
>+ ("new" directory is for messages on the server that haven't been seen by a
>+*  mail client).
>+ * aFolderName is already "safe" - it has been through NS_MsgHashIfNecessary
>+ */
Comment got really mangled, also refers to obsolete parameter.

>+  // CreateUnique, in case we get more than one message per millisecond :-)
Oh, apparently we fixed that on Windows some time ago. But doesn't hurt ;-)

>+    if (fileName.IsEmpty())
>+      return NS_ERROR_FAILURE;
>+
>+    if (fileName.IsEmpty())
>+    {
>+      PR_LOG(MailDirLog, PR_LOG_ALWAYS,
>+             ("GetMsgInputStream - empty storeToken!!\n"));
>+      return NS_ERROR_FAILURE;
>+    }
Just being really sure, I take it? ;-)

>+    bool exists;
>+    destFile->Exists(&exists);
>+    if (exists)
>+    {
>+      rv = destFile->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 0600);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      destFile->GetNativeLeafName(fileName);
>+    }
Is it worth doing the extra exists check? After all, CreateUnique effectively does one for you.

I guess that it's unlikely that a move will fail halfway through. What happens if a copy fails halfway through, you just end up with a partial copy?
Attachment #577656 - Flags: superreview?(neil) → superreview+
this fixes a few xpcshell and mozmill tests. This is what I hope to land when I get some time, probably over the holidays. There are a couple test failures with try server but I can't reproduce them locally.
Attachment #577656 - Attachment is obsolete: true
checked in - http://hg.mozilla.org/comm-central/rev/097bc36f180d
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 12.0
Attached patch suite partSplinter Review
David, I presume suite should have this part as well to support the new storage backend work.

Is this enough for us? (I admit to not really knowing this side of the code at all, but my mxr searches seems to make me think its enough)
Attachment #584243 - Flags: review?(dbienvenu)
Comment on attachment 584243 [details] [diff] [review]
suite part

I don't know if I can review SM code, but yeah, that looks OK - is that what TB is doing for adding virtual folders?
Attachment #584243 - Flags: review?(dbienvenu) → feedback+
Comment on attachment 584243 [details] [diff] [review]
suite part

>+    var newFolder;
>     try
>     {
>-      var newFolder = parentFolder.addSubfolder(newName);
[Not sure why you moved the variable out of the block.]

>+      if (parentFolder instanceof(Components.interfaces.nsIMsgLocalMailFolder))
Nit: instanceof is an operator, doesn't need ()s
I don't know why that test ever worked - it was trying to get two messages to be the same to test for dups, but never set the messageid in the message, so they were auto-assigned. For some reason, in debug mode, they'd get the same message id, but not in release mode. In any case, fixing the test data fixes the test.
Comment on attachment 581370 [details] [diff] [review]
fix some unit tests and marking messages read

>-      nsCOMPtr<nsIAtom> folderRenameAtom = MsgGetAtom("RenameCompleted");
>+    nsCOMPtr<nsIAtom> folderRenameAtom = do_GetAtom("RenameCompleted");
Oops, how did I not notice this? Please change it back to MsgGetAtom.
http://hg.mozilla.org/comm-central/rev/4548f08f3f76 pushed for MsgGetAtom fix
Blocks: 713767
Blocks: 713768
Depends on: 713683
Blocks: 713873
Depends on: 713958
Depends on: 714182
Depends on: 713611
Depends on: 715685
Depends on: 715922
Depends on: 713491
Depends on: 719037
Depends on: 718514
No longer blocks: 713873
Depends on: 713873
Mark Banner wrote "Thunderbird 11 is the earliest if the reviews are completed by then." I just tried 12 but didn't see any evidence of this feature. So I'm wondering what date/version are we likely to see maildir support?
The target is set to TB12, where this landed.
However it is my understanding that this is only some backend enabling work. There is no UI provided yet. So you can't just switch your production mailbox to maildir straight away, if that is what you look for.
See the bugs in the "Blocks" field for that features.
It's "just" an API change , for maildir you want to follow 58308.
I have been looking for some time to upgrade from PMMail 2000 (windows ver) which just worked reliably for what I needed. As of a decade ago, it had features such as maildir type storage. Unfortunately, the development had stopped and some things are broken now, forcing an upgrade. I am really hoping Thunderbird would be it. MBOX is too unreliable and cannot be trust by any serious email user.

Looking forward to maildir support in TB! Keep up awesome work guys!
Blocks: 462665
Depends on: 715924
Depends on: 736539
Depends on: 741027
Blocks: 648108
Summary: Support other message storage formats → Support other message storage formats. (prelude to pluggable mail stores)
Blocks: 748090
No longer blocks: 748090
Depends on: 748090
Depends on: 748597
Depends on: 748726
Depends on: 748432
Depends on: 748770
Depends on: 748997
Depends on: 748593
Depends on: 753147
Depends on: 725810
No longer depends on: 725810
Depends on: 725810
Blocks: 748899
Depends on: 772760
Depends on: 790912
Blocks: 805626
No longer blocks: 859011
Depends on: 840418
Depends on: 818325
Depends on: 807878
Depends on: 1134024
No longer blocks: 1135309
Comment on attachment 581370 [details] [diff] [review]
fix some unit tests and marking messages read

>-    PRInt64 fileSize;
>-    pMessageDB->GetMailboxModProperties(&fileSize, &actualFolderTimeStamp);
>-    pMessageDB->m_dbFolderInfo->SetFolderSize((PRUint32) fileSize);
SetFolderSize was already 64-bit at this point! I wonder how long this would have gone unnoticed otherwise.
No longer depends on: 772760
Regressions: 772760
You need to log in before you can comment on or make changes to this bug.