[PATCH 1/1] Re: make maxage use UIDs to avoid timezone issues (edit: correction)

Nicolas Sebrecht nicolas.s-dev at laposte.net
Sat Mar 21 02:48:52 GMT 2015


Here is my quick review. If I'm wrong with my comments, I'll take the
patch. I'm much better satisfied on all the aspects: how it looks like
logic reliability and it's finally much less complex.

Thanks for the good work.

On Fri, Mar 20, 2015 at 04:23:01PM -0400, Janna Martl wrote:

> When using maxage, local and remote messagelists are supposed to only
> contain messages from "maxage days ago". But local and remote used
> different timezones to calculate what "maxage days ago" means, resulting
> in loss of mail. Now, we ask the local folder for maxage days' worth of
> mail, find the lowest UID, and then ask the remote folder for all UID's
> starting with that lowest one.

Very good commit message. Clear and concise.

> Signed-off-by: Janna Martl <janna.martl109 at gmail.com>
> ---
>  offlineimap/accounts.py            | 30 ++++++++++++++++++++++++------
>  offlineimap/folder/Gmail.py        |  5 +++--
>  offlineimap/folder/GmailMaildir.py |  4 ++--
>  offlineimap/folder/IMAP.py         | 25 ++++++++++++++-----------
>  offlineimap/folder/Maildir.py      | 21 +++++++++++++--------
>  5 files changed, 56 insertions(+), 29 deletions(-)
> 
> diff --git a/offlineimap/accounts.py b/offlineimap/accounts.py
> index 62ed5c3..e7d6c3c 100644
> --- a/offlineimap/accounts.py
> +++ b/offlineimap/accounts.py
> @@ -429,6 +429,15 @@ def syncfolder(account, remotefolder, quick):
>  
>          statusfolder.cachemessagelist()
>  
> +        maxage = localfolder.config.getdefaultint("Account %s"%
> +            localfolder.accountname, "maxage", None)
> +
> +        # Load local folder.
> +        ui.syncingfolder(remoterepos, remotefolder, localrepos, localfolder)
> +        ui.loadmessagelist(localrepos, localfolder)
> +        localfolder.cachemessagelist(maxage=maxage)
> +        ui.messagelistloaded(localrepos, localfolder, localfolder.getmessagecount())
> +
>          if quick:
>              if (not localfolder.quickchanged(statusfolder) and
>                  not remotefolder.quickchanged(statusfolder)):
> @@ -436,11 +445,6 @@ def syncfolder(account, remotefolder, quick):
>                  localrepos.restore_atime()
>                  return
>  
> -        # Load local folder.
> -        ui.syncingfolder(remoterepos, remotefolder, localrepos, localfolder)
> -        ui.loadmessagelist(localrepos, localfolder)
> -        localfolder.cachemessagelist()
> -        ui.messagelistloaded(localrepos, localfolder, localfolder.getmessagecount())
>  
>          # If either the local or the status folder has messages and
>          # there is a UID validity problem, warn and abort.  If there are
> @@ -463,7 +467,21 @@ def syncfolder(account, remotefolder, quick):
>  
>          # Load remote folder.
>          ui.loadmessagelist(remoterepos, remotefolder)
> -        remotefolder.cachemessagelist()
> +        # Enforce maxage on remote folder by restricting to the lowest UID in
> +        # the local messagelist (which has already been restricted to > maxage)
> +        if maxage:
> +            if localfolder.getmessageuidlist():
> +                # local messagelist might contain new messages (with uid's < 0)
> +                min_uid = min(filter(lambda uid: uid > 0,
> +                    localfolder.getmessageuidlist()))
> +                remotefolder.cachemessagelist(min_uid=min_uid)
> +            else: # no messages in range in localfolder
> +                # maxage is calculated with respect to folder timezone, and
> +                # remote folder timezone might be different from local, so be
> +                # safe and make sure the range isn't bigger than in local
> +                remotefolder.cachemessagelist(maxage=maxage - 1)

Very good.

> +        else:
> +            remotefolder.cachemessagelist()
>          ui.messagelistloaded(remoterepos, remotefolder,
>                               remotefolder.getmessagecount())
>  
> diff --git a/offlineimap/folder/Gmail.py b/offlineimap/folder/Gmail.py
> index 1afbe47..93e8eee 100644
> --- a/offlineimap/folder/Gmail.py
> +++ b/offlineimap/folder/Gmail.py
> @@ -121,9 +121,10 @@ class GmailFolder(IMAPFolder):
>  
>      # TODO: merge this code with the parent's cachemessagelist:
>      # TODO: they have too much common logics.
> -    def cachemessagelist(self):
> +    def cachemessagelist(self, maxage=None, min_uid=None):
>          if not self.synclabels:
> -            return super(GmailFolder, self).cachemessagelist()
> +            return super(GmailFolder, self).cachemessagelist(maxage=maxage,
> +                min_uid=min_uid)
>  
>          self.messagelist = {}
>  
> diff --git a/offlineimap/folder/GmailMaildir.py b/offlineimap/folder/GmailMaildir.py
> index 894792d..9a092cb 100644
> --- a/offlineimap/folder/GmailMaildir.py
> +++ b/offlineimap/folder/GmailMaildir.py
> @@ -64,9 +64,9 @@ class GmailMaildirFolder(MaildirFolder):
>                  'filename': '/no-dir/no-such-file/', 'mtime': 0}
>  
>  
> -    def cachemessagelist(self):
> +    def cachemessagelist(self, maxage=None, min_uid=None):
>          if self.ismessagelistempty():
> -            self.messagelist = self._scanfolder()
> +            self.messagelist = self._scanfolder(maxage=maxage, min_uid=min_uid)
>  
>          # Get mtimes
>          if self.synclabels:
> diff --git a/offlineimap/folder/IMAP.py b/offlineimap/folder/IMAP.py
> index ccc83b0..2b59840 100644
> --- a/offlineimap/folder/IMAP.py
> +++ b/offlineimap/folder/IMAP.py
> @@ -144,7 +144,7 @@ class IMAPFolder(BaseFolder):
>          return False
>  
>  
> -    def _msgs_to_fetch(self, imapobj):
> +    def _msgs_to_fetch(self, imapobj, maxage=None, min_uid=None):
>          """Determines sequence numbers of messages to be fetched.
>  
>          Message sequence numbers (MSNs) are more easily compacted
> @@ -164,16 +164,14 @@ class IMAPFolder(BaseFolder):
>          # By default examine all messages in this folder
>          msgsToFetch = '1:*'
>  
> -        maxage = self.config.getdefaultint(
> -            "Account %s"% self.accountname, "maxage", -1)
>          maxsize = self.config.getdefaultint(
> -            "Account %s"% self.accountname, "maxsize", -1)
> +            "Account %s"% self.accountname, "maxsize", None)
>  
>          # Build search condition
> -        if (maxage != -1) | (maxsize != -1):
> +        if maxage or min_uid or maxsize:
>              search_cond = "(";
>  
> -            if(maxage != -1):
> +            if maxage:
>                  #find out what the oldest message is that we should look at
>                  oldest_struct = time.gmtime(time.time() - (60*60*24*maxage))
>                  if oldest_struct[0] < 1900:
> @@ -184,9 +182,13 @@ class IMAPFolder(BaseFolder):
>                      oldest_struct[2],
>                      MonthNames[oldest_struct[1]],
>                      oldest_struct[0])
> +            if min_uid:
> +                if maxage: # There are two conditions, add space

I might be missing of code context but I don't get why this if/if nested
clauses.

We should either be searching with a maxage restriction or min_uid but
not both.

> +                    search_cond += " "
> +                search_cond += "UID %d:*"% min_uid

Good SEARCH condition.

>  
> -            if(maxsize != -1):
> -                if(maxage != -1): # There are two conditions, add space
> +            if maxsize:
> +                if maxage or min_uid: # There are at least two conditions, add space
>                      search_cond += " "
>                  search_cond += "SMALLER %d"% maxsize
>  
> @@ -210,19 +212,19 @@ class IMAPFolder(BaseFolder):
>  
>  
>      # Interface from BaseFolder
> -    def cachemessagelist(self):
> +    def cachemessagelist(self, maxage=None, min_uid=None):
>          self.messagelist = {}
>  
>          imapobj = self.imapserver.acquireconnection()
>          try:
> -            msgsToFetch = self._msgs_to_fetch(imapobj)
> +            msgsToFetch = self._msgs_to_fetch(imapobj, maxage=maxage, min_uid=min_uid)
>              if not msgsToFetch:
>                  return # No messages to sync
>  
>              # Get the flags and UIDs for these. single-quotes prevent
>              # imaplib2 from quoting the sequence.
>              res_type, response = imapobj.fetch("'%s'"%
> -                msgsToFetch, '(FLAGS UID)')
> +                msgsToFetch, '(FLAGS UID INTERNALDATE)')

Finally there it is. But at least, we don't have to workaround it and
compute date times. I feel much much better! ,-)

>              if res_type != 'OK':
>                  raise OfflineImapError("FETCHING UIDs in folder [%s]%s failed. "
>                      "Server responded '[%s] %s'"% (self.getrepository(), self,
> @@ -840,6 +842,7 @@ class IMAPFolder(BaseFolder):
>          uidlist = [uid for uid in uidlist if self.uidexists(uid)]
>          if not len(uidlist):
>              return
> +        uid = uidlist[0]

Nitpick: I'm missing of context again but the variable name for
uidlist[0] looks poor.

>  
>          self.__addmessagesflags_noconvert(uidlist, set('T'))
>          imapobj = self.imapserver.acquireconnection()
> diff --git a/offlineimap/folder/Maildir.py b/offlineimap/folder/Maildir.py
> index 77f7ebb..d06d8f6 100644
> --- a/offlineimap/folder/Maildir.py
> +++ b/offlineimap/folder/Maildir.py
> @@ -150,7 +150,7 @@ class MaildirFolder(BaseFolder):
>              flags = set((c for c in flagmatch.group(1) if not c.islower()))
>          return prefix, uid, fmd5, flags
>  
> -    def _scanfolder(self):
> +    def _scanfolder(self, maxage=None, min_uid=None):
>          """Cache the message list from a Maildir.
>  
>          Maildir flags are: R (replied) S (seen) T (trashed) D (draft) F
> @@ -158,8 +158,6 @@ class MaildirFolder(BaseFolder):
>          :returns: dict that can be used as self.messagelist.
>          """
>  
> -        maxage = self.config.getdefaultint("Account " + self.accountname,
> -                                           "maxage", None)
>          maxsize = self.config.getdefaultint("Account " + self.accountname,
>                                              "maxsize", None)
>          retval = {}
> @@ -185,6 +183,9 @@ class MaildirFolder(BaseFolder):
>                  uid = nouidcounter
>                  nouidcounter -= 1
>              else:                       # It comes from our folder.
> +                # Check min_uid if this message should be considered.
> +                if min_uid and uid < min_uid:
> +                    continue
>                  uidmatch = re_uidmatch.search(filename)
>                  uid = None
>                  if not uidmatch:
> @@ -192,6 +193,7 @@ class MaildirFolder(BaseFolder):
>                      nouidcounter -= 1
>                  else:
>                      uid = long(uidmatch.group(1))
> +
>              # 'filename' is 'dirannex/filename', e.g. cur/123,U=1,FMD5=1:2,S
>              retval[uid] = self.msglist_item_initializer(uid)
>              retval[uid]['flags'] = flags
> @@ -200,8 +202,8 @@ class MaildirFolder(BaseFolder):
>  
>      # Interface from BaseFolder
>      def quickchanged(self, statusfolder):
> -        """Returns True if the Maildir has changed"""
> -        self.cachemessagelist()
> +        """Returns True if the Maildir has changed
> +        Assumes cachemessagelist() has already been called """
>          # Folder has different uids than statusfolder => TRUE.
>          if sorted(self.getmessageuidlist()) != \
>                  sorted(statusfolder.getmessageuidlist()):
> @@ -218,9 +220,9 @@ class MaildirFolder(BaseFolder):
>          return {'flags': set(), 'filename': '/no-dir/no-such-file/'}
>  
>      # Interface from BaseFolder
> -    def cachemessagelist(self):
> +    def cachemessagelist(self, maxage=None, min_uid=None):
>          if self.ismessagelistempty():
> -            self.messagelist = self._scanfolder()
> +            self.messagelist = self._scanfolder(maxage=maxage, min_uid=min_uid)
>  
>      # Interface from BaseFolder
>      def getmessagelist(self):
> @@ -448,7 +450,10 @@ class MaildirFolder(BaseFolder):
>              os.unlink(filepath)
>          except OSError:
>              # Can't find the file -- maybe already deleted?
> -            newmsglist = self._scanfolder()
> +            maxage = localfolder.config.getdefaultint("Account %s"%
> +                localfolder.accountname, "maxage", None)

We tend to have lot of these

  maxage = config.getBlabla

Is there anything we could do about that? Perhaps a Folder().getMaxage()?

> +            # get more than enough messages to be sure we captured this uid
> +            newmsglist = self._scanfolder(maxage=maxage + 1)

Yes.

>              if uid in newmsglist:       # Nope, try new filename.
>                  filename = newmsglist[uid]['filename']
>                  filepath = os.path.join(self.getfullname(), filename)

-- 
Nicolas Sebrecht




More information about the OfflineIMAP-project mailing list