[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