[PATCH] Re: properly check whether dstfolder supports labels

Nicolas Sebrecht nicolas.s-dev at laposte.net
Mon Apr 6 18:43:46 BST 2015


On Mon, Apr 06, 2015 at 06:08:11PM +0200, Abdo Roig-Maranges wrote:

> This adds a storeslabels() method to check whether a folder supports
> labels, and checks it in copymessageto and copymessageto_labels,
> replacing an exception-based check.

Good!

It's straightforward enough so that I'm inclined to merge even without
IMAP/Gmail support if the "warnings" and style issues get fixed.

> ---
>  offlineimap/folder/Base.py              |   4 +
>  offlineimap/folder/Gmail.py             | 110 +++++++++--------
>  offlineimap/folder/GmailMaildir.py      | 202 ++++++++++++++++----------------
>  offlineimap/folder/LocalStatus.py       |   4 +
>  offlineimap/folder/LocalStatusSQLite.py |   6 +-
>  5 files changed, 175 insertions(+), 151 deletions(-)
> 
> diff --git a/offlineimap/folder/Base.py b/offlineimap/folder/Base.py
> index 16b5819..88b233b 100644
> --- a/offlineimap/folder/Base.py
> +++ b/offlineimap/folder/Base.py
> @@ -134,6 +134,10 @@ class BaseFolder(object):
>  
>          return 1
>  
> +    def storeslabels(self):
> +        """Should be true for any backend that is able to store message labels."""
> +        return False
> +
>      def getvisiblename(self):
>          """The nametrans-transposed name of the folder's name."""
>  
> diff --git a/offlineimap/folder/Gmail.py b/offlineimap/folder/Gmail.py
> index 1afbe47..c17d819 100644
> --- a/offlineimap/folder/Gmail.py
> +++ b/offlineimap/folder/Gmail.py
> @@ -64,6 +64,11 @@ class GmailFolder(IMAPFolder):
>          self.ignorelabels = set([l for l in re.split(r'\s*,\s*', ignorelabels) if len(l)])
>  
>  
> +    def storeslabels(self):
> +        """Should be true for any backend that is able to store message labels."""
> +        return True
> +
> +
>      def getmessage(self, uid):
>          """Retrieve message with UID from the IMAP server (incl body).  Also
>             gets Gmail labels and embeds them into the message.
> @@ -290,16 +295,16 @@ class GmailFolder(IMAPFolder):
>  
>          # sync labels and mtime now when the message is new (the embedded labels are up to date)
>          # otherwise we may be spending time for nothing, as they will get updated on a later pass.
> +        # check that target folder supports labels.
>          if realcopy and self.synclabels:
> -            try:
> -                mtime = dstfolder.getmessagemtime(uid)
> -                labels = dstfolder.getmessagelabels(uid)
> -                statusfolder.savemessagelabels(uid, labels, mtime=mtime)
> -
> -            # dstfolder is not GmailMaildir.
> -            except NotImplementedError:
> +            if not dstfolder.storeslabels():
> +                self.ui.warn("Repository '%s' does not support labels." % dstfolder.repository.name)
>                  return

About the warnings:

- either we fully support Gmail/IMAP and warn about that is not
  expected

- or we don't support Gmaild/IMAP (yet) and we should stop syncing the
  current account.

>  
> +            mtime = dstfolder.getmessagemtime(uid)

Side note: so, mtime would be better called labels_mtime, right?

> +            labels = dstfolder.getmessagelabels(uid)
> +            statusfolder.savemessagelabels(uid, labels, mtime=mtime)
> +
>      def syncmessagesto_labels(self, dstfolder, statusfolder):
>          """Pass 4: Label Synchronization (Gmail only)
>  
> @@ -315,56 +320,57 @@ class GmailFolder(IMAPFolder):
>          # the fastest thing in the world though...
>          uidlist = []
>  
> +        # check that target folder supports labels.
> +        if not dstfolder.storeslabels():
> +            self.ui.warn("Repository '%s' does not support labels." % dstfolder.repository.name)

Ditto.

As a side note, please avoid the whitespace between " and % for string
substitutions.

> +            return
> +
>          # filter the uids (fast)
> -        try:
> -            for uid in self.getmessageuidlist():
> -                # bail out on CTRL-C or SIGTERM
> -                if offlineimap.accounts.Account.abort_NOW_signal.is_set():
> -                    break
> +        for uid in self.getmessageuidlist():
> +            # bail out on CTRL-C or SIGTERM
> +            if offlineimap.accounts.Account.abort_NOW_signal.is_set():
> +                break
> +
> +            # Ignore messages with negative UIDs missed by pass 1 and
> +            # don't do anything if the message has been deleted remotely
> +            if uid < 0 or not dstfolder.uidexists(uid):
> +                continue
>  
> -                # Ignore messages with negative UIDs missed by pass 1 and
> -                # don't do anything if the message has been deleted remotely
> -                if uid < 0 or not dstfolder.uidexists(uid):
> -                    continue
> +            selflabels = self.getmessagelabels(uid) - self.ignorelabels

Side note: I don't like "selflabels". It looks like a dot was forgot.ten.

>  
> -                selflabels = self.getmessagelabels(uid) - self.ignorelabels
> +            if statusfolder.uidexists(uid):
> +                statuslabels = statusfolder.getmessagelabels(uid) - self.ignorelabels
> +            else:
> +                statuslabels = set()
>  
> -                if statusfolder.uidexists(uid):
> -                    statuslabels = statusfolder.getmessagelabels(uid) - self.ignorelabels
> -                else:
> -                    statuslabels = set()
> +            if selflabels != statuslabels:
> +                uidlist.append(uid)
>  
> -                if selflabels != statuslabels:
> -                    uidlist.append(uid)
> +        # now sync labels (slow)

While at it, explain why it's slow!

> +        mtimes = {}
> +        labels = {}
> +        for i, uid in enumerate(uidlist):
> +            # bail out on CTRL-C or SIGTERM
> +            if offlineimap.accounts.Account.abort_NOW_signal.is_set():
> +                break
>  
> -            # now sync labels (slow)
> -            mtimes = {}
> -            labels = {}
> -            for i, uid in enumerate(uidlist):
> -                # bail out on CTRL-C or SIGTERM
> -                if offlineimap.accounts.Account.abort_NOW_signal.is_set():
> -                    break
> +            selflabels = self.getmessagelabels(uid) - self.ignorelabels
>  
> -                selflabels = self.getmessagelabels(uid) - self.ignorelabels
> +            if statusfolder.uidexists(uid):
> +                statuslabels = statusfolder.getmessagelabels(uid) - self.ignorelabels
> +            else:
> +                statuslabels = set()
>  
> -                if statusfolder.uidexists(uid):
> -                    statuslabels = statusfolder.getmessagelabels(uid) - self.ignorelabels
> -                else:
> -                    statuslabels = set()
> -
> -                if selflabels != statuslabels:
> -                    self.ui.settinglabels(uid, i+1, len(uidlist), sorted(selflabels), dstfolder)
> -                    if self.repository.account.dryrun:
> -                        continue #don't actually add in a dryrun
> -                    dstfolder.savemessagelabels(uid, selflabels, ignorelabels = self.ignorelabels)
> -                    mtime = dstfolder.getmessagemtime(uid)
> -                    mtimes[uid] = mtime
> -                    labels[uid] = selflabels
> -
> -            # Update statusfolder in a single DB transaction. It is safe, as if something fails,
> -            # statusfolder will be updated on the next run.
> -            statusfolder.savemessageslabelsbulk(labels)
> -            statusfolder.savemessagesmtimebulk(mtimes)
> -
> -        except NotImplementedError:
> -            self.ui.warn("Can't sync labels. You need to configure a local repository of type GmailMaildir")
> +            if selflabels != statuslabels:
> +                self.ui.settinglabels(uid, i+1, len(uidlist), sorted(selflabels), dstfolder)
> +                if self.repository.account.dryrun:
> +                    continue #don't actually add in a dryrun
                                ^^
                                Missing whitespace.

> +                dstfolder.savemessagelabels(uid, selflabels, ignorelabels = self.ignorelabels)
                                                                            ^^^
                                                                            Too much whitespaces

> +                mtime = dstfolder.getmessagemtime(uid)
> +                mtimes[uid] = mtime
> +                labels[uid] = selflabels
> +
> +        # Update statusfolder in a single DB transaction. It is safe, as if something fails,
> +        # statusfolder will be updated on the next run.
> +        statusfolder.savemessageslabelsbulk(labels)
> +        statusfolder.savemessagesmtimebulk(mtimes)
> diff --git a/offlineimap/folder/GmailMaildir.py b/offlineimap/folder/GmailMaildir.py
> index 894792d..acd4dff 100644
> --- a/offlineimap/folder/GmailMaildir.py
> +++ b/offlineimap/folder/GmailMaildir.py
> @@ -39,6 +39,12 @@ class GmailMaildirFolder(MaildirFolder):
>          if self.synclabels:
>              self.syncmessagesto_passes.append(('syncing labels', self.syncmessagesto_labels))
>  
> +
> +    def storeslabels(self):
> +        """Should be true for any backend that is able to store message labels."""
> +        return True
> +
> +
>      def quickchanged(self, statusfolder):
>          """Returns True if the Maildir has changed. Checks uids, flags and mtimes"""
>  
> @@ -205,14 +211,14 @@ class GmailMaildirFolder(MaildirFolder):
>          # for message which already existed on the remote, this is useless, as later the labels may
>          # get updated.
>          if realcopy and self.synclabels:
> -            try:
> -                labels = dstfolder.getmessagelabels(uid)
> -                statusfolder.savemessagelabels(uid, labels, mtime=self.getmessagemtime(uid))
> -
> -            # dstfolder is not GmailMaildir.
> -            except NotImplementedError:
> +            # check that target folder supports labels.
> +            if not dstfolder.storeslabels():
> +                self.ui.warn("Repository '%s' does not support labels." % dstfolder.repository.name)

Ditto.

>                  return
>  
> +            labels = dstfolder.getmessagelabels(uid)
> +            statusfolder.savemessagelabels(uid, labels, mtime=self.getmessagemtime(uid))
> +
>      def syncmessagesto_labels(self, dstfolder, statusfolder):
>          """Pass 4: Label Synchronization (Gmail only)
>  
> @@ -233,95 +239,95 @@ class GmailMaildirFolder(MaildirFolder):
>          dellabellist = {}
>          uidlist = []
>  
> -        try:
> -            # filter uids (fast)
> -            for uid in self.getmessageuidlist():
> -                # bail out on CTRL-C or SIGTERM
> -                if offlineimap.accounts.Account.abort_NOW_signal.is_set():
> -                    break
> -
> -                # Ignore messages with negative UIDs missed by pass 1 and
> -                # don't do anything if the message has been deleted remotely
> -                if uid < 0 or not dstfolder.uidexists(uid):
> -                    continue
> -
> -                selfmtime = self.getmessagemtime(uid)
> -
> -                if statusfolder.uidexists(uid):
> -                    statusmtime = statusfolder.getmessagemtime(uid)
> -                else:
> -                    statusmtime = 0
> -
> -                if selfmtime > statusmtime:
> -                    uidlist.append(uid)
> -
> -
> -            self.ui.collectingdata(uidlist, self)
> -            # This can be slow if there is a lot of modified files
> -            for uid in uidlist:
> -                # bail out on CTRL-C or SIGTERM
> -                if offlineimap.accounts.Account.abort_NOW_signal.is_set():
> -                    break
> -
> -                selflabels = self.getmessagelabels(uid)
> -
> -                if statusfolder.uidexists(uid):
> -                    statuslabels = statusfolder.getmessagelabels(uid)
> -                else:
> -                    statuslabels = set()
> -
> -                addlabels = selflabels - statuslabels
> -                dellabels = statuslabels - selflabels
> -
> -                for lb in addlabels:
> -                    if not lb in addlabellist:
> -                        addlabellist[lb] = []
> -                    addlabellist[lb].append(uid)
> -
> -                for lb in dellabels:
> -                    if not lb in dellabellist:
> -                        dellabellist[lb] = []
> -                    dellabellist[lb].append(uid)
> -
> -            for lb, uids in addlabellist.items():
> -                # bail out on CTRL-C or SIGTERM
> -                if offlineimap.accounts.Account.abort_NOW_signal.is_set():
> -                    break
> -
> -                self.ui.addinglabels(uids, lb, dstfolder)
> -                if self.repository.account.dryrun:
> -                    continue #don't actually add in a dryrun
> -                dstfolder.addmessageslabels(uids, set([lb]))
> -                statusfolder.addmessageslabels(uids, set([lb]))
> -
> -            for lb, uids in dellabellist.items():
> -                # bail out on CTRL-C or SIGTERM
> -                if offlineimap.accounts.Account.abort_NOW_signal.is_set():
> -                    break
> -
> -                self.ui.deletinglabels(uids, lb, dstfolder)
> -                if self.repository.account.dryrun:
> -                    continue #don't actually remove in a dryrun
> -                dstfolder.deletemessageslabels(uids, set([lb]))
> -                statusfolder.deletemessageslabels(uids, set([lb]))
> -
> -            # Update mtimes on StatusFolder. It is done last to be safe. If something els fails
> -            # and the mtime is not updated, the labels will still be synced next time.
> -            mtimes = {}
> -            for uid in uidlist:
> -                # bail out on CTRL-C or SIGTERM
> -                if offlineimap.accounts.Account.abort_NOW_signal.is_set():
> -                    break
> -
> -                if self.repository.account.dryrun:
> -                    continue #don't actually update statusfolder
> -
> -                filename = self.messagelist[uid]['filename']
> -                filepath = os.path.join(self.getfullname(), filename)
> -                mtimes[uid] = long(os.stat(filepath).st_mtime)
> -
> -            # finally update statusfolder in a single DB transaction
> -            statusfolder.savemessagesmtimebulk(mtimes)
> -
> -        except NotImplementedError:
> -            self.ui.warn("Can't sync labels. You need to configure a remote repository of type Gmail.")
> +        # check that target folder supports labels.
> +        if not dstfolder.storeslabels():
> +            self.ui.warn("Repository '%s' does not support labels." % dstfolder.repository.name)
> +            return
> +
> +        # filter uids (fast)
> +        for uid in self.getmessageuidlist():
> +            # bail out on CTRL-C or SIGTERM
> +            if offlineimap.accounts.Account.abort_NOW_signal.is_set():
> +                break
> +
> +            # Ignore messages with negative UIDs missed by pass 1 and
> +            # don't do anything if the message has been deleted remotely
> +            if uid < 0 or not dstfolder.uidexists(uid):
> +                continue
> +
> +            selfmtime = self.getmessagemtime(uid)
> +
> +            if statusfolder.uidexists(uid):
> +                statusmtime = statusfolder.getmessagemtime(uid)
> +            else:
> +                statusmtime = 0
> +
> +            if selfmtime > statusmtime:
> +                uidlist.append(uid)
> +
> +        self.ui.collectingdata(uidlist, self)
> +        # This can be slow if there is a lot of modified files
> +        for uid in uidlist:
> +            # bail out on CTRL-C or SIGTERM
> +            if offlineimap.accounts.Account.abort_NOW_signal.is_set():
> +                break
> +
> +            selflabels = self.getmessagelabels(uid)
> +
> +            if statusfolder.uidexists(uid):
> +                statuslabels = statusfolder.getmessagelabels(uid)
> +            else:
> +                statuslabels = set()
> +
> +            addlabels = selflabels - statuslabels
> +            dellabels = statuslabels - selflabels
> +
> +            for lb in addlabels:
> +                if not lb in addlabellist:
> +                    addlabellist[lb] = []
> +                addlabellist[lb].append(uid)
> +
> +            for lb in dellabels:
> +                if not lb in dellabellist:
> +                    dellabellist[lb] = []
> +                dellabellist[lb].append(uid)
> +
> +        for lb, uids in addlabellist.items():
> +            # bail out on CTRL-C or SIGTERM
> +            if offlineimap.accounts.Account.abort_NOW_signal.is_set():
> +                break
> +
> +            self.ui.addinglabels(uids, lb, dstfolder)
> +            if self.repository.account.dryrun:
> +                continue #don't actually add in a dryrun
> +            dstfolder.addmessageslabels(uids, set([lb]))
> +            statusfolder.addmessageslabels(uids, set([lb]))
> +
> +        for lb, uids in dellabellist.items():
> +            # bail out on CTRL-C or SIGTERM
> +            if offlineimap.accounts.Account.abort_NOW_signal.is_set():
> +                break
> +
> +            self.ui.deletinglabels(uids, lb, dstfolder)
> +            if self.repository.account.dryrun:
> +                continue #don't actually remove in a dryrun
> +            dstfolder.deletemessageslabels(uids, set([lb]))
> +            statusfolder.deletemessageslabels(uids, set([lb]))
> +
> +        # Update mtimes on StatusFolder. It is done last to be safe. If something els fails
> +        # and the mtime is not updated, the labels will still be synced next time.
> +        mtimes = {}
> +        for uid in uidlist:
> +            # bail out on CTRL-C or SIGTERM
> +            if offlineimap.accounts.Account.abort_NOW_signal.is_set():
> +                break
> +
> +            if self.repository.account.dryrun:
> +                continue #don't actually update statusfolder
> +
> +            filename = self.messagelist[uid]['filename']
> +            filepath = os.path.join(self.getfullname(), filename)
> +            mtimes[uid] = long(os.stat(filepath).st_mtime)
> +
> +        # finally update statusfolder in a single DB transaction
> +        statusfolder.savemessagesmtimebulk(mtimes)
> diff --git a/offlineimap/folder/LocalStatus.py b/offlineimap/folder/LocalStatus.py
> index 78f2134..2c3d9bb 100644
> --- a/offlineimap/folder/LocalStatus.py
> +++ b/offlineimap/folder/LocalStatus.py
> @@ -43,6 +43,10 @@ class LocalStatusFolder(BaseFolder):
>      def storesmessages(self):
>          return 0
>  
> +    def storeslabels(self):
> +        """Should be true for any backend that is able to store message labels."""
> +        return True
> +
>      def isnewfolder(self):
>          return not os.path.exists(self.filename)
>  
> diff --git a/offlineimap/folder/LocalStatusSQLite.py b/offlineimap/folder/LocalStatusSQLite.py
> index 64adcd1..d0e897f 100644
> --- a/offlineimap/folder/LocalStatusSQLite.py
> +++ b/offlineimap/folder/LocalStatusSQLite.py
> @@ -97,6 +97,10 @@ class LocalStatusSQLiteFolder(BaseFolder):
>      def storesmessages(self):
>          return False
>  
> +    def storeslabels(self):
> +        """Should be true for any backend that is able to store message labels."""
> +        return True
> +
>      def getfullname(self):
>          return self.filename
>  
> @@ -338,7 +342,7 @@ class LocalStatusSQLiteFolder(BaseFolder):
>      def savemessageslabelsbulk(self, labels):
>          """
>          Saves labels from a dictionary in a single database operation.
> -        
> +
>          """
>          data = [(', '.join(sorted(l)), uid) for uid, l in labels.items()]
>          self.__sql_write('UPDATE status SET labels=? WHERE id=?', data, executemany=True)
-- 
Nicolas Sebrecht




More information about the OfflineIMAP-project mailing list