[PATCH] Re: Avoid trying to synchronize folders that have empty names

[Dodji Seketeli]

Hello Sebastian,

Thank you for the review.

Sebastian Spaeth <Sebastian at SSpaeth.de> a écrit:

> Nicolas Sebrecht <nicolas.s-dev at laposte.net> writes:
>
>> On Tue, Jun 26, 2012 at 03:45:03PM +0200, Dodji Seketeli wrote:
>
>>> Since commit a279aa7 [2], it can happen that offlineimap wrongly tries
>>> to create a folder named '/' on the remote server.
>
> Actually, I have come to the conclusion that it correctly tries to
> create a folder named "/" on the remote server in these cases. Do you
> have a top-level /cur/new/tmp in your maildir? And how is your nametrans
> rule?

I have no nametrans rule.  I do have cur/new/tmp in the top-level.  But
they are empty, because the only maildir folder that actually contain
emails are in sub-directories of the top-directory.  After looking
closely, I realize that it's my local dovecot server that adds these
cur/new/tmp directories.  And yes, my local maildir is served by a local
Dovecot server.

The problem here is that the remote server is saying that it disallows
creating a directory named '/'.  Is the server I am using "special" in
refusing the creation of a folder named '/'?  That would be unfortunate
because it's a Zimbra server, and it's quite popular.  I have tried the
same thing on a dovecot 2.1.9 server and it forbids it too.

>>> I am hitting the same issue as Jes Sorensen[1], where at some point,
>>> offlineimap 6.5.4 would error out with the message:
>
> the patch in question was added, to make top-level folders work
> correctly in some cases. For example when one has "." as separators in a
> local maildir. We would try to create folder names "." in our directory,
> which of course does not work.

I see.  Thank you for the explanation.

> The handling of top-level folders needs to be carefully addressed so it
> works with all configuration options (separator rules) and nametrans
> rules. (what if a nametrans rule ends up as "empty"?)

Well, if a nametrans rule resolves to an path in the context of the
remote repository, I'd say that means "do not sync this repository".
Otherwise, what other reasonable meaning would that have?

> So what if our local maildir looks like this:
>
> ~
> ~/mail
> ~/mail/cur
> ~/mail/new
> ~/mail/tmp
> ~/mail/Sent
> ~/mail/Sent/new
> ~/mail/Sent/cur
> ~/mail/Sent/tmp
>
> With your patch we would never scan the top-level directory, missing out
> the main cur/new/tmp folders there. Am I right with that assumption?

Yes you are right, but in my case, ~/mail/cur, ~/mail/new ~/mail/tmp
don't contain any email.  Only ~/mail/Sent/{new,cur,tmp} would.

So I am proposing the patch below, which acts *after* nametrans.  The
idea is to give a chance to users to actually write a nametrans rule
that would sync emails that are directly under ~/mail with a remote
directory that actually has a non-empty name, i.e, not "/".

Note that the patch allows the creation of an empty path for servers
that support "reference" root folders.  In that case, as the
concatenation of the reference and the "" makes up a non-empty path, the
"" is allowed.

Would that be better?  If the idea is suitable, I can send a proper
patch with a proper commit log.

Thanks.

diff --git a/offlineimap/repository/Base.py b/offlineimap/repository/Base.py
index 81050b0..d1d3aa7 100644
--- a/offlineimap/repository/Base.py
+++ b/offlineimap/repository/Base.py
@@ -136,6 +136,11 @@ class BaseRepository(CustomConfig.ConfigHelperMixin, object):
         'readonly' or by using the 'createfolders' setting."""  
         return self._readonly or self.getconfboolean('createfolders', True)
 
+    def folder_path_is_empty (self, foldername):
+        """ Return True if foldername resolves to an empty path in the
+        context of the current repository, False otherwise."""
+        raise NotImplementedError
+
     def makefolder(self, foldername):
         """Create a new folder"""
         raise NotImplementedError
@@ -167,12 +172,28 @@ class BaseRepository(CustomConfig.ConfigHelperMixin, object):
         # to the dest folder's sep.
         src_hash = {}
         for folder in src_folders:
-            src_hash[folder.getvisiblename().replace(
-                    src_repo.getsep(), dst_repo.getsep())] = folder
+            src_folder_name_in_remote_repo = folder.getvisiblename().replace(src_repo.getsep(),
+                                                                             dst_repo.getsep())
+            if dst_repo.folder_path_is_empty(src_folder_name_in_remote_repo):
+                # So the visible folder path (i.e, after nametrans has
+                # been applied) is empty.  This doesn't make sense, so
+                # let's skip it.  If the user really wants this
+                # directory to be sync'ed, she should write a
+                # nametrans rules that transforms the empty path into
+                # something non empty.
+                self.ui.debug('', "Skipping local folder with empty path");
+                continue
+            src_hash[src_folder_name_in_remote_repo] = folder
         dst_hash = {}
         for folder in dst_folders:
-            dst_hash[folder.getvisiblename().replace(
-                    dst_repo.getsep(), src_repo.getsep())] = folder
+            dst_folder_name_in_local_repo = folder.getvisiblename().replace(dst_repo.getsep(),
+                                                                            src_repo.getsep())
+
+            if src_repo.folder_path_is_empty(dst_folder_name_in_local_repo):
+                # Like the case above, skip this directory with empty path.
+                self.ui.debug('', "Skipping remote folder with empty path");
+                continue
+            dst_hash[dst_folder_name_in_local_repo] = folder
 
         # Find new folders on src_repo.
         for src_name_t, src_folder in src_hash.iteritems():
diff --git a/offlineimap/repository/IMAP.py b/offlineimap/repository/IMAP.py
index 5ad787a..9b0ff2f 100644
--- a/offlineimap/repository/IMAP.py
+++ b/offlineimap/repository/IMAP.py
@@ -332,6 +332,15 @@ class IMAPRepository(BaseRepository):
         self.folders = retval
         return self.folders
 
+    def folder_path_is_empty (self, foldername):
+        full_path = ''
+        if self.getreference:
+            full_path = self.getreference() + self.getsep() + foldername
+
+        if full_path == '' or full_path == self.getsep():
+            return True
+        return False
+
     def makefolder(self, foldername):
         """Create a folder on the IMAP server
 
diff --git a/offlineimap/repository/Maildir.py b/offlineimap/repository/Maildir.py
index f197002..5b94fce 100644
--- a/offlineimap/repository/Maildir.py
+++ b/offlineimap/repository/Maildir.py
@@ -69,6 +69,12 @@ class MaildirRepository(BaseRepository):
     def getsep(self):
         return self.getconf('sep', '.').strip()
 
+    def folder_path_is_empty (self, foldername):
+        full_path = os.path.abspath(os.path.join(self.root, foldername))
+        if full_path == os.sep:
+            return True
+        return False
+
     def makefolder(self, foldername):
         """Create new Maildir folder if necessary

-- 
		Dodji