Simplify the syncing strategy a bit

The previous syncing strategy was doing more than we needed to and was a
bit underdocumented. This is an attempt to clean it up.

1) Do away with the previous different code paths depending on
whether there is a LocalStatus file or not (the isnewfolder() test). We
always use the same strategy now, which makes the strategy easier to
understand. This strategy is simply:

a) Sync remote to local folder first
b) Sync local to remote

Where each sync implies a 4 pass strategy which does basically the same
as before (explained below).

2) Don't delete messages on LOCAL which don't exist on REMOTE right at
the beginning anymore. This prevented us e.g. from keeping local
messages rather than redownloading everything once LocalStatus got
corrupted or deleted. This surprised many who put in an existing local
maildir and expected it to be synced to the remote place. Instead, the
local maildir was deleted. This is a data loss that actually occured to
people!

3) No need to separately sync the statusfolder, we update that one
simultanously with the destfolders...

3) Simplified the sync function API by only taking one destdir rather
than a list of destdirs, we never used more anyway. This makes the code
easier to read.

4) Added plenty of code comments while I was going through to make sure
the strategy is easy to understand.
-----------------------------------------

 Pass1: Transfer new local messages
        Upload msg with negative/no UIDs to dstfolder. dstfolder should
        assign that message a new UID. Update statusfolder.

 Pass2: Copy existing messages
        Copy messages in self, but not statusfolder to dstfolder if not
        already in dstfolder. Update statusfolder.

 Pass3: Remove deleted messages
        Get all UIDS in statusfolder but not self. These are messages
        that we have locally deleted. Delete those from dstfolder and
        statusfolder.

 Pass4: Synchronize flag changes
        Compare flags in self with those in statusfolder. If msg has a
        valid UID and exists on dstfolder (has not e.g.  been deleted
        there), sync the flag change to dstfolder and statusfolder.

The user visible implications of this change should be unnoticable
except in one situation:
 Blowing away LocalStatus will not require you to redownload ALL of
 your mails if you still have the local Maildir. It will simply recreate
 LocalStatus.

Signed-off-by: Sebastian Spaeth <Sebastian@SSpaeth.de>
Signed-off-by: Nicolas Sebrecht <nicolas.s-dev@laposte.net>

Changelog.draft.rst

@@ -15,7 +15,6 @@ New Features
 * Implement UIDPLUS extension support. OfflineIMAP will now not insert
   an X-OfflineIMAP header if the mail server supports the UIDPLUS
   extension.
-
 * SSL: support subjectAltName.
 
 Changes
@@ -26,6 +25,10 @@ Changes
 * Change UI names to Blinkenlights,TTYUI,Basic,Quiet,MachineUI.
   Old names will still work, but are deprecated.
   Document that we don't accept a list of UIs anymore.
+* Reworked the syncing strategy. The only user-visible change is that  
+  blowing away LocalStatus will not require you to redownload ALL of
+  your mails if you still have the local Maildir. It will simply
+  recreate LocalStatus.
 
 Bug Fixes
 ---------

offlineimap/accounts.py

@@ -350,29 +350,14 @@ def syncfolder(accountname, remoterepos, remotefolder, localrepos,
         ui.messagelistloaded(remoterepos, remotefolder,
                              len(remotefolder.getmessagelist().keys()))
 
-
-        #
-
-        if not statusfolder.isnewfolder():
-            # Delete local copies of remote messages.  This way,
-            # if a message's flag is modified locally but it has been
-            # deleted remotely, we'll delete it locally.  Otherwise, we
-            # try to modify a deleted message's flags!  This step
-            # need only be taken if a statusfolder is present; otherwise,
-            # there is no action taken *to* the remote repository.
-
-            remotefolder.syncmessagesto_delete(localfolder, [localfolder,
-                                                             statusfolder])
-            ui.syncingmessages(localrepos, localfolder, remoterepos, remotefolder)
-            localfolder.syncmessagesto(statusfolder, [remotefolder, statusfolder])
-
         # Synchronize remote changes.
         ui.syncingmessages(remoterepos, remotefolder, localrepos, localfolder)
-        remotefolder.syncmessagesto(localfolder, [localfolder, statusfolder])
+        remotefolder.syncmessagesto(localfolder, statusfolder)
+        # Synchronize local changes
+        ui.syncingmessages(localrepos, localfolder, remoterepos, remotefolder)
+        localfolder.syncmessagesto(remotefolder, statusfolder)
+
 
-        # Make sure the status folder is up-to-date.
-        ui.syncingmessages(localrepos, localfolder, statusrepos, statusfolder)
-        localfolder.syncmessagesto(statusfolder)
         statusfolder.save()
         localrepos.restore_atime()
     except (KeyboardInterrupt, SystemExit):

offlineimap/folder/Base.py

@@ -206,68 +206,89 @@ class BaseFolder:
         for uid in uidlist:
             self.deletemessage(uid)
 
-    def syncmessagesto_neguid_msg(self, uid, dest, applyto, register = 1):
+    def syncmessagesto_neguid_msg(self, uid, dstfolder, statusfolder,
-        if register:
+                                  register = 1):
-            self.ui.registerthread(self.getaccountname())
+        """Copy a single message from self to dests.
-        self.ui.copyingmessage(uid, self, applyto)
+
+        This is called by meth:`syncmessagesto_neguid`, possibly in a
+        new thread. It does not return anything.
+
+        :param dstfolder: A BaseFolder-derived instance
+        :param statusfolder: A LocalStatusFolder instance
+        :param register: If True, output that a new thread was created
+                         (and register it with the ui)."""
         successobject = None
         successuid = None
+
+        if register:
+            self.ui.registerthread(self.getaccountname())
+        self.ui.copyingmessage(uid, self, [dstfolder])
+
         message = self.getmessage(uid)
         flags = self.getmessageflags(uid)
         rtime = self.getmessagetime(uid)
-        for tryappend in applyto:
+
-            successuid = tryappend.savemessage(uid, message, flags, rtime)
+        #Save messages to dstfolder and see if a valid UID was returned
+        successuid = dstfolder.savemessage(uid, message, flags, rtime)
+
+        #Succeeded? -> IMAP actually assigned a UID
+        #If successuid remained negative, no server was willing to assign us
+        #an UID. Ignore message.
         if successuid >= 0:
-                successobject = tryappend
+            # Copy the message to the statusfolder
-                break
+            statusfolder.savemessage(successuid, message, flags, rtime)
-        # Did we succeed?
+            # Copy to its new name son the local server and delete
-        if successobject != None:
-            if successuid:       # Only if IMAP actually assigned a UID
-                # Copy the message to the other remote servers.
-                for appendserver in \
-                        [x for x in applyto if x != successobject]:
-                    appendserver.savemessage(successuid, message, flags, rtime)
-                    # Copy to its new name on the local server and delete
             # the one without a UID.
             self.savemessage(successuid, message, flags, rtime)
-            self.deletemessage(uid) # It'll be re-downloaded.
+            #TODO: above means, we read in the message and write it out
-        else:
+            #to the very same dir with a different UID. Investigate if we
-            # Did not find any server to take this message.  Ignore.
+            #cannot simply rename it.
-            pass
+            
+            # delete the negative uid message. We have it with a good UID now.
+            self.deletemessage(uid)
         
 
-    def syncmessagesto_neguid(self, dest, applyto):
+    def syncmessagesto_neguid(self, dstfolder, statusfolder):
         """Pass 1 of folder synchronization.
 
-        Look for messages in self with a negative uid.  These are messages in
+        Look for messages in self with a negative uid.  These are
-        Maildirs that were not added by us.  Try to add them to the dests,
+        messages in Maildirs that were not added by us.  Try to add them
-        and once that succeeds, get the UID, add it to the others for real,
+        to the dstfolder. If that succeeds, get the new UID, add
-        add it to local for real, and delete the fake one."""
+        it to the statusfolder, add it to local for real, and delete the
+        old fake (negative) one.
+
+        :param dstfolder: A BaseFolder-derived instance
+        :param statusfolder: A LocalStatusFolder instance"""
 
         uidlist = [uid for uid in self.getmessagelist().keys() if uid < 0]
         threads = []
 
-        usethread = None
-        if applyto != None:
-            usethread = applyto[0]
-        
         for uid in uidlist:
-            if usethread and usethread.suggeststhreads():
+            if dstfolder.suggeststhreads():
-                usethread.waitforthread()
+                dstfolder.waitforthread()
                 thread = threadutil.InstanceLimitedThread(\
-                    usethread.getcopyinstancelimit(),
+                    dstfolder.getcopyinstancelimit(),
                     target = self.syncmessagesto_neguid_msg,
                     name = "New msg sync from %s" % self.getvisiblename(),
-                    args = (uid, dest, applyto))
+                    args = (uid, dstfolder, statusfolder))
                 thread.setDaemon(1)
                 thread.start()
                 threads.append(thread)
             else:
-                self.syncmessagesto_neguid_msg(uid, dest, applyto, register = 0)
+                self.syncmessagesto_neguid_msg(uid, dstfolder, statusfolder,
+                                               register = 0)
+        #wait for all uploads to finish
         for thread in threads:
             thread.join()
 
-    def copymessageto(self, uid, applyto, register = 1):
+    def copymessageto(self, uid, dstfolder, statusfolder, register = 1):
+        """Copies a message from self to dst if needed, updating the status
+
+        :param uid: uid of the message to be copied.
+        :param dstfolder: A BaseFolder-derived instance
+        :param statusfolder: A LocalStatusFolder instance
+        :param register: whether we should register a new thread."
+        :returns: Nothing on success, or raises an Exception."""
         # Sometimes, it could be the case that if a sync takes awhile,
         # a message might be deleted from the maildir before it can be
         # synced to the status cache.  This is only a problem with
@@ -276,43 +297,56 @@ class BaseFolder:
         try:
             if register:
                 self.ui.registerthread(self.getaccountname())
-            self.ui.copyingmessage(uid, self, applyto)
-            message = ''
-            # If any of the destinations actually stores the message body,
-            # load it up.
 
-            for object in applyto:
+            message = None
-                if object.storesmessages():
-                    message = self.getmessage(uid)
-                    break
             flags = self.getmessageflags(uid)
             rtime = self.getmessagetime(uid)
-            for object in applyto:
+            
-                newuid = object.savemessage(uid, message, flags, rtime)
+            if uid in dstfolder.getmessagelist():
+                # dst has message with that UID already, only update status
+                statusfolder.savemessage(uid, None, flags, rtime)
+                return
+
+            # really need to copy to dst...
+            self.ui.copyingmessage(uid, self, [dstfolder])
+
+            # If any of the destinations actually stores the message body,
+            # load it up.            
+            if dstfolder.storesmessages():
+                message = self.getmessage(uid)
+
+            newuid = dstfolder.savemessage(uid, message, flags, rtime)
             if newuid > 0 and newuid != uid:
                 # Change the local uid.
                 self.savemessage(newuid, message, flags, rtime)
                 self.deletemessage(uid)
                 uid = newuid
+            statusfolder.savemessage(uid, message, flags, rtime)
         except (KeyboardInterrupt):
             raise
         except:
             self.ui.warn("ERROR attempting to copy message " + str(uid) \
-                 + " for account " + self.getaccountname() + ":" + traceback.format_exc())
+                 + " for account " + self.getaccountname() + ":" \
+                 + traceback.format_exc())
+            raise
 
+    def syncmessagesto_copy(self, dstfolder, statusfolder):
+        """Pass2: Copy locally existing messages
 
-    def syncmessagesto_copy(self, dest, applyto):
+        This will copy messages with a valid UID but are not on the
-        """Pass 2 of folder synchronization.
+        other side yet. The strategy is:
 
-        Look for messages present in self but not in dest.  If any, add
+        1) Look for messages present in self but not in statusfolder.
-        them to dest."""
+        2) invoke copymessageto() on those which:
+           - If dstfolder doesn't have it yet, add them to dstfolder.
+           - Update statusfolder
+        """
         threads = []
 
-	dest_messagelist = dest.getmessagelist()
+        copylist = filter(lambda uid: uid>=0 and not \
-        for uid in self.getmessagelist().keys():
+                              uid in statusfolder.getmessagelist(),
-            if uid < 0:                 # Ignore messages that pass 1 missed.
+                            self.getmessagelist().keys())
-                continue
+        for uid in copylist:
-            if not uid in dest_messagelist:
             if self.suggeststhreads():
                 self.waitforthread()
                 thread = threadutil.InstanceLimitedThread(\
@@ -320,112 +354,125 @@ class BaseFolder:
                     target = self.copymessageto,
                     name = "Copy message %d from %s" % (uid,
                                                         self.getvisiblename()),
-                        args = (uid, applyto))
+                    args = (uid, dstfolder, statusfolder))
                 thread.setDaemon(1)
                 thread.start()
                 threads.append(thread)
             else:
-                    self.copymessageto(uid, applyto, register = 0)
+                self.copymessageto(uid, dstfolder, statusfolder, register = 0)
+
         for thread in threads:
             thread.join()
 
-    def syncmessagesto_delete(self, dest, applyto):
+    def syncmessagesto_delete(self, dstfolder, statusfolder):
-        """Pass 3 of folder synchronization.
+        """Pass 3: Remove locally deleted messages on dst
 
-        Look for message present in dest but not in self.
+        Get all UIDS in statusfolder but not self. These are messages
-        If any, delete them."""
+        that were deleted in 'self'. Delete those from dstfolder and
-        deletelist = []
+        statusfolder."""
-	self_messagelist = self.getmessagelist()
+        deletelist = filter(lambda uid: uid>=0 \
-        for uid in dest.getmessagelist().keys():
+                                and not uid in self.getmessagelist(),
-            if uid < 0:
+                            statusfolder.getmessagelist().keys())
-                continue
-            if not uid in self_messagelist:
-                deletelist.append(uid)
         if len(deletelist):
-            self.ui.deletingmessages(deletelist, applyto)
+            self.ui.deletingmessages(deletelist, [dstfolder])
-            for object in applyto:
+            # delete in statusfolder first to play safe. In case of abort, we
-                object.deletemessages(deletelist)
+            # won't lose message, we will just retransmit some unneccessary.
+            for folder in [statusfolder, dstfolder]:
+                folder.deletemessages(deletelist)
 
-    def syncmessagesto_flags(self, dest, applyto):
+    def syncmessagesto_flags(self, dstfolder, statusfolder):
-        """Pass 4 of folder synchronization.
+        """Pass 4: Flag synchronization
-
-        Look for any flag matching issues -- set dest message to have the
-        same flags that we have."""
-
-        # As an optimization over previous versions, we store up which flags
-        # are being used for an add or a delete.  For each flag, we store
-        # a list of uids to which it should be added.  Then, we can call
-        # addmessagesflags() to apply them in bulk, rather than one
-        # call per message as before.  This should result in some significant
-        # performance improvements.
 
+        Compare flag mismatches in self with those in statusfolder. If
+        msg has a valid UID and exists on dstfolder (has not e.g. been
+        deleted there), sync the flag change to both dstfolder and
+        statusfolder.
+        """
+        # For each flag, we store a list of uids to which it should be
+        # added.  Then, we can call addmessagesflags() to apply them in
+        # bulk, rather than one call per message.
         addflaglist = {}
         delflaglist = {}
-        
         for uid in self.getmessagelist().keys():
-            if uid < 0:                 # Ignore messages missed by pass 1
+            # Ignore messages with negative UIDs missed by pass 1
+            # also don't do anything if the message has been deleted remotely
+            if uid < 0 or not uid in dstfolder.getmessagelist():
                 continue
-            selfflags = self.getmessageflags(uid)
-            destflags = dest.getmessageflags(uid)
 
-            addflags = [x for x in selfflags if x not in destflags]
+            selfflags = self.getmessageflags(uid)
+            statusflags = statusfolder.getmessageflags(uid)
+            #if we could not get message flags from LocalStatus, assume empty.
+            if statusflags is None:
+                statusflags = []
+            addflags = [x for x in selfflags if x not in statusflags]
 
             for flag in addflags:
                 if not flag in addflaglist:
                     addflaglist[flag] = []
                 addflaglist[flag].append(uid)
 
-            delflags = [x for x in destflags if x not in selfflags]
+            delflags = [x for x in statusflags if x not in selfflags]
             for flag in delflags:
                 if not flag in delflaglist:
                     delflaglist[flag] = []
                 delflaglist[flag].append(uid)
 
-        for object in applyto:
         for flag in addflaglist.keys():
-                self.ui.addingflags(addflaglist[flag], flag, [object])
+            self.ui.addingflags(addflaglist[flag], flag, [dstfolder])
-                object.addmessagesflags(addflaglist[flag], [flag])
+            dstfolder.addmessagesflags(addflaglist[flag], [flag])
+            statusfolder.addmessagesflags(addflaglist[flag], [flag])
+
         for flag in delflaglist.keys():
-                self.ui.deletingflags(delflaglist[flag], flag, [object])
+            self.ui.deletingflags(delflaglist[flag], flag, [dstfolder])
-                object.deletemessagesflags(delflaglist[flag], [flag])
+            dstfolder.deletemessagesflags(delflaglist[flag], [flag])
+            statusfolder.deletemessagesflags(delflaglist[flag], [flag])
                 
-    def syncmessagesto(self, dest, applyto = None):
+    def syncmessagesto(self, dstfolder, statusfolder):
-        """Syncs messages in this folder to the destination.
+        """Syncs messages in this folder to the destination dstfolder.
-        If applyto is specified, it should be a list of folders (don't forget
-        to include dest!) to which all write actions should be applied.
-        It defaults to [dest] if not specified.  It is important that
-        the UID generator be listed first in applyto; that is, the other
-        applyto ones should be the ones that "copy" the main action."""
-        if applyto == None:
-            applyto = [dest]
 
+        This is the high level entry for syncing messages in one direction.
+        Syncsteps are:
+
+        Pass1: Transfer new local messages
+         Upload msg with negative/no UIDs to dstfolder. dstfolder
+         might assign that message a new UID. Update statusfolder.
+
+        Pass2: Copy locally existing messages
+         Copy messages in self, but not statusfolder to dstfolder if not
+         already in dstfolder. Update statusfolder.
+
+        Pass3: Remove locally deleted messages
+         Get all UIDS in statusfolder but not self. These are messages
+         that were deleted in 'self'. Delete those from dstfolder and
+         statusfolder.
+
+         After this pass, the message lists should be identical wrt the
+         uids present (except for potential negative uids that couldn't
+         be placed anywhere).
+
+        Pass4: Synchronize flag changes 
+         Compare flag mismatches in self with those in statusfolder. If
+         msg has a valid UID and exists on dstfolder (has not e.g. been
+         deleted there), sync the flag change to both dstfolder and
+         statusfolder.
+
+
+        :param dstfolder: Folderinstance to sync the msgs to.
+        :param statusfolder: LocalStatus instance to sync against.
+        """
+        passes = [('uploading negative UIDs', self.syncmessagesto_neguid),
+                  ('copying messages'       , self.syncmessagesto_copy),
+                  ('deleting messages'      , self.syncmessagesto_delete),
+                  ('syncing flags'          , self.syncmessagesto_flags)]
+
+        for (passdesc, action) in passes:
             try:
-            self.syncmessagesto_neguid(dest, applyto)
+                action(dstfolder, statusfolder)
-        except (KeyboardInterrupt):
-            raise
-        except:
-            self.ui.warn("ERROR attempting to handle negative uids " \
-                + "for account " + self.getaccountname() + ":" + traceback.format_exc())
-
-        #all threads launched here are in try / except clauses when they copy anyway...
-        self.syncmessagesto_copy(dest, applyto)
-
-        try:
-            self.syncmessagesto_delete(dest, applyto)
-        except (KeyboardInterrupt):
-            raise
-        except:
-            self.ui.warn("ERROR attempting to delete messages " \
-                + "for account " + self.getaccountname() + ":" + traceback.format_exc())
-
-        # Now, the message lists should be identical wrt the uids present.
-        # (except for potential negative uids that couldn't be placed
-        # anywhere)
-
-        try:
-            self.syncmessagesto_flags(dest, applyto)
             except (KeyboardInterrupt):
                 raise
             except:
                 self.ui.warn("ERROR attempting to sync flags " \
-                + "for account " + self.getaccountname() + ":" + traceback.format_exc())
+                             + "for account " + self.getaccountname() \
+                             + ":" + traceback.format_exc())
+
+                raise