Locally created mailboxes not synchronized to IMAP

[Vladimir Marek]

Hi,

thanks for the review!


> 1) It would be great to split out white space changes and functional
> changes into separate patches, as it makes reviewing the patches much
> harder. (OK, enough nitpicking :-))

That's right. I'll surely do some cleanup, it was just current state
snapshot to show my intentions.


> 3) Creating (and deleteing) new folders on both sides would be trivial
> if it were not for 2 things: folderfilters and nametrans. Both make
> things more complicated.
[...]

I understand that now. Indeed I overlooked those.



> nametrans is the tougher one. It is 1-way fudging of names and as you
> remark you would need a corresponding "nametrans" setting on the local
> repository that reverses names exactly. Given the difficulties of
> setting up correct nametrans rules (often including horrible regexes),
> it will be *very* difficult to find the exact reverse functions. I would
> be more comfortable to have something like this after we introduce a
> "--dryrun" option that allows to see what offlineimap would do when it
> would actually be run.
>
> For now I would be happy to have patches that enable folder creation
> only when nametrans is not being used.


Suppose we have two namentrans functions (nomral and reverse). For each
folder we can check that they give 1:1 mapping. Just pass any folder
name to be created or deleted through normal and the result through
reverse (or vice versa for the other direction) and see if we get the
same result. If not, don't do anything destructive and tell user to fix
his config. To me this should not create any risk to user and he does
not need --dryrun and checking if nametrans is in use. Surely those are
valid options too.



> Glad to see you sending patches for more functionality. I am holding my
> 15 feature branches back, as I am waiting for us to release the next
> version before sending more intrusive things :).

Oh I'm looking forward to see them :)




> > 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.

I see. That cries out loudly for a test suite.



> >  - 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?

Good idea. Mixed tabs/spaces makes my vim sick for some reason :-/



> >  - 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.

More test suite screaming.


> 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

will do.


> 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.

In fact, I'm glad that I started to touch git, since I'm starting to
like it :) 'git rebase --interactive' is a powerfull tool!


> Sorry, if the above sounds a tad harsh, it's meant constructively :)

Not at all, I truly appreciate it.


> >          """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.

Not really. I think I copied that block from some other file.



> The change to fix: > +            self._folders = [] is correct though,
> and I have a patch queued to do the same already. :-)

Nice, I'll wait for your changes and re-work my patch afterwards.



> 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))

I don't remember exactly, but it didn't work. I didn't understand the
code and intention behind 100%. I'll investigate more.




> > 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?

It made every folder examined twice, so I thought at first it's a bug.
(someone said test suite?)

> You are omitting the top directory now!

There will be better way surely.


> > @@ -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.

Can't tell you now, it was wild hack'n'slash coding session, just to
have first concept. And I was quite sure that this is not right :) But I
didn't want to study it exactly before having some comment on the
overall approach. Which I have now.

I want to take a look on some possibility of creating the tests at least
for the features I would break with my patch.

Thank you
-- 
	Vlad