Locally created mailboxes not synchronized to IMAP
Sebastian Spaeth
Sebastian at SSpaeth.de
Tue Aug 9 09:28:21 BST 2011
On Mon, 1 Aug 2011 10:44:05 +0200, Vladimir Marek <Vladimir.Marek at Oracle.COM> wrote:
Non-text part: multipart/mixed
> Hi,
>
> I found that if I create local mailbox, it is not replicated on IMAP.
> Other direction works fine. I was playing with it yesterday afternoon
> and I finally made it working. But it's not completely trivial fix, so
> as usual I might have overlooked something. I am attaching the diff I
> have, just to have some chance for comments. It's doing several things
>
> - if there's directory on one side, and it's not on other, I first
> consult local status folder to see if we knew the directory before.
> If it's completely new, let's sync it. If we knew the directory, it
> means it was erased on one side and I just print warning
See my folderfilters example in the other mail. If I run "offlineimap -f
INBOX" to only sync my INBOX I would get many warnings. The creation
logic seems sound even when folderfilters is being used.
> - reformatted tabs -> spaces (mixed tabs spaces break my autoindenting
> in vim, and I'm too lazy to make it more robust :) )
Please sort this out in a separate patch please, its very hard to
review. Perhaps we can do a "tab->space" correction for all files at the
same time?
> - syncfoldersto now accepts just one copyfolder, not list of them. This
> is something I haven't understood why it is there ...
Yep, remnant of the past and should be going away. It's code cleanup
that I have on my list too. But it's probably stuff for separate patches.
> - I had to implement 'getfolders' methond in repository/LocalStatus.py,
> so that I can check if we knew given mailbox in the past. It's not
> very robust at the moment and I haven't tried SQL local status.
> - repository/Maildir.py had a strange check
> - if self.getsep() == '/' and dirname != '.':
> + if dirname != '.':
> that was breaking nested mailboxes for me. By removing it I probably
> broke something else, but it works for me now. I have to investigate.
> Strange is that self.getsep() is by default "." so I wonder if the
> recursion worked in the past?
Careful here, you break stuff, see comments in code review below.
> I'll appreciate any comments. I'm quite sure you already love me
> spamming you with strange diffs, but there's not may other things I
> would like to improve, as it is nearly perfect anyway. So I'll stop
> bothering you soon :)
Cool functionality, I would have appreciated to see this as a patch
series of about 4 patches or so:
1) Fix whitespace
2) Fix repository/LocalStatus.py:getfolders() to actually work
3) Cleanup syncfoldersto to accept a single folder and no list
(this could go into a separate cleanup topic together with patch 1)
4) Implement actual folder creation logic
I know it's bothersome and requires more git-foo than one wants but it's
clean and separated small patches that can easily be analyzed (in case
things go wrong). Personally, I often use "git gui" to pick my changes into
several commits.
Sorry, if the above sounds a tad harsh, it's meant constructively :)
Sebastian
> diff --git a/offlineimap/accounts.py b/offlineimap/accounts.py
> index a989441..be139da 100644
> --- a/offlineimap/accounts.py
> +++ b/offlineimap/accounts.py
> @@ -232,7 +232,7 @@ class SyncableAccount(Account):
> # replicate the folderstructure from REMOTE to LOCAL
> if not localrepos.getconf('readonly', False):
> self.ui.syncfolders(remoterepos, localrepos)
> - remoterepos.syncfoldersto(localrepos, [statusrepos])
> + remoterepos.syncfoldersto(localrepos, statusrepos)
>
> # iterate through all folders on the remote repo and sync
> for remotefolder in remoterepos.getfolders():
> diff --git a/offlineimap/repository/Base.py b/offlineimap/repository/Base.py
> index 34f2f4a..a5dc2e5 100644
> --- a/offlineimap/repository/Base.py
> +++ b/offlineimap/repository/Base.py
> @@ -18,6 +18,7 @@
>
> import os.path
> import traceback
> +import pprint
> from offlineimap import CustomConfig
> from offlineimap.ui import getglobalui
>
> @@ -42,14 +43,14 @@ class BaseRepository(object, CustomConfig.ConfigHelperMixin):
> # The 'restoreatime' config parameter only applies to local Maildir
> # mailboxes.
> def restore_atime(self):
> - if self.config.get('Repository ' + self.name, 'type').strip() != \
> - 'Maildir':
> - return
> + if self.config.get('Repository ' + self.name, 'type').strip() != \
> + 'Maildir':
> + return
>
> - if not self.config.has_option('Repository ' + self.name, 'restoreatime') or not self.config.getboolean('Repository ' + self.name, 'restoreatime'):
> - return
> + if not self.config.has_option('Repository ' + self.name, 'restoreatime') or not self.config.getboolean('Repository ' + self.name, 'restoreatime'):
> + return
>
> - return self.restore_folder_atimes()
> + return self.restore_folder_atimes()
>
> def connect(self):
> """Establish a connection to the remote, if necessary. This exists
> @@ -114,42 +115,65 @@ class BaseRepository(object, CustomConfig.ConfigHelperMixin):
> def getfolder(self, foldername):
> raise NotImplementedError
>
> - def syncfoldersto(self, dest, copyfolders):
> + def syncfoldersto(self, dest, copyfolder):
> """Syncs the folders in this repository to those in dest.
> It does NOT sync the contents of those folders.
>
> - For every time dest.makefolder() is called, also call makefolder()
> - on each folder in copyfolders."""
> + For every time makefolder() is called, also call makefolder() in
> + copyfolder."""
> src = self
> srcfolders = src.getfolders()
> destfolders = dest.getfolders()
> + copyfoldedrs = copyfolder.getfolders()
> + copyfolders = copyfolder.getfolders()
>
> # Create hashes with the names, but convert the source folders
> # to the dest folder's sep.
>
> srchash = {}
> +# pprint.pprint(srcfolders)
> for folder in srcfolders:
> - srchash[folder.getvisiblename().replace(src.getsep(), dest.getsep())] = \
> - folder
> + srchash[folder.getvisiblename().replace(src.getsep(), "/")] = folder
> desthash = {}
> +# pprint.pprint(destfolders)
> for folder in destfolders:
> - desthash[folder.getvisiblename()] = folder
> -
> + desthash[folder.getvisiblename().replace(dest.getsep(), "/")] = folder
> + copyhash = {}
> +# pprint.pprint(copyfolders)
> + for folder in copyfolders:
> + copyhash[folder.getvisiblename().replace(copyfolder.getsep(), "/")] = folder
> +
> + def syncfoldersto_internal(where, key):
> + self.ui._msg("Creating folder %s" % key)
> + try:
> + where.makefolder(key.replace("/", where.getsep()))
> + copyfolder.makefolder(key.replace("/", copyfolder.getsep()))
> + except (KeyboardInterrupt):
> + raise
> + except:
> + self.ui.warn("ERROR Attempting to create folder " \
> + + key + ":" +traceback.format_exc())
> +
> +# print "srchash"
> +# print srchash
> +# print "desthash"
> +# print desthash
> +# print "copyhash"
> +# print copyhash
> #
> # Find new folders.
> #
> -
> - for key in srchash.keys():
> - if not key in desthash:
> - try:
> - dest.makefolder(key)
> - for copyfolder in copyfolders:
> - copyfolder.makefolder(key.replace(dest.getsep(), copyfolder.getsep()))
> - except (KeyboardInterrupt):
> - raise
> - except:
> - self.ui.warn("ERROR Attempting to create folder " \
> - + key + ":" +traceback.format_exc())
> + for key in set(srchash.keys()) - set(desthash.keys()):
> + if key in copyhash:
> + self.ui._msg("Should delete %s" % key)
> + else:
> + syncfoldersto_internal(dest, key)
> +
> + for key in set(desthash.keys()) - set(srchash.keys()):
> + if key in copyhash:
> + self.ui._msg("Should delete %s" % key)
> + else:
> + syncfoldersto_internal(self, key)
>
> #
> # Find deleted folders.
> diff --git a/offlineimap/repository/LocalStatus.py b/offlineimap/repository/LocalStatus.py
> index 022be21..d9cac9e 100644
> --- a/offlineimap/repository/LocalStatus.py
> +++ b/offlineimap/repository/LocalStatus.py
> @@ -83,11 +83,11 @@ class LocalStatusRepository(BaseRepository):
> """Returns a list of ALL folders on this server.
>
> This is currently nowhere used in the code."""
> - if self._folders != None:
> - return self._folders
> -
> - for folder in os.listdir(self.directory):
> - self._folders = retval.append(self.getfolder(folder))
> + # def __init__(self, root, name, repository, accountname, config):
> + if self._folders == None:
> + self._folders = []
> + for folder in os.listdir(self.directory):
> + self._folders.append(self.LocalStatusFolderClass(self.directory, folder, self, self.account, self.config))
> return self._folders
Is there a reason to replace the nice:
if <already_inited>:
return results
do_lots_stuff...
return results
with the deeper indented version:
if <not_inited>:
do_lots_stuff
return results
which does not look like an improvement to me.
The change to fix: > + self._folders = [] is correct though,
and I have a patch queued to do the same already. :-)
Also, why does the following change stops reusing code (getfolder()) and
duplicates it with the long initialization of the folder manually?
> - self._folders = retval.append(self.getfolder(folder))
> +
> self._folders.append(self.LocalStatusFolderClass(self.directory, folder, self, self.account, self.config))
> diff --git a/offlineimap/repository/Maildir.py b/offlineimap/repository/Maildir.py
> index 069748a..c4b527d 100644
> --- a/offlineimap/repository/Maildir.py
> +++ b/offlineimap/repository/Maildir.py
> @@ -132,8 +132,10 @@ class MaildirRepository(BaseRepository):
> toppath = root
> self.debug(" toppath = %s" % toppath)
>
> + #print "!!!!!!!!!!!!!!!!"
> + #print os.listdir(toppath)
> # Iterate over directories in top & top itself.
> - for dirname in os.listdir(toppath) + ['.']:
> + for dirname in os.listdir(toppath):
> self.debug(" *** top of loop")
Careful here! I just had to fix this code as I had it broken before. We
do want to examine all directories *including* the current one. What is
the reason for removing this?
You are omitting the top directory now!
> @@ -166,8 +170,10 @@ class MaildirRepository(BaseRepository):
> - if self.getsep() == '/' and dirname != '.':
> + #print "getsep=%s dirname=%s" % (self.getsep(), dirname)
> + if dirname != '.':
> # Recursively check sub-directories for folders too.
> + #print "recursing extension=%s root=%s foldername=%s" % (extension, root, foldername)
Careful here! getsep== '/' is true only when we use hierarchical nested
maildir folders. In the normal case, maildir folders are flat and we
don't want to recurse into deeper directory structures (which may be
folders completely unrelated to the maildir). Why was this breaking
things?
This change is definitely not the right way to fix things.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://alioth-lists.debian.net/pipermail/offlineimap-project/attachments/20110809/c9dcdbfc/attachment-0001.sig>
More information about the OfflineIMAP-project
mailing list