[PATCH] Re: introducing xattrMaildir: a Maildir repository where message files have all IMAP flags in an extended attribute

[Nicolas Sebrecht]

On Sun, Mar 10, 2013 at 09:48:25PM +0100, Erik Quaeghebeur wrote:
> Dear maintainers & other interested subscribers,
> 
> 
> To be applied on top of the previous patch I sent to this mailing list, 
> this patch adds a new repository type, xattrMaildir, a Maildir 
> repository where message files have all IMAP flags in an extended 
> attribute 'user.org.offlineimap.flags'. (It is perfectly backwards 
> compatible with the Maildir specification.)
> 
> As before I have done a (successful) test that is minimal: for one 
> message, I synced from an IMAP server to an xattrMaildir repository and 
> verified (using the getfattr command-line tool) that all IMAP flags were 
> indeed synced.
> 
> The code uses the xattr module (http://pyxattr.k1024.org/module.html) as 
> an extra dependency. Actually, very little has changed:
> 
> * in  folder/__init__.py  and  repository/__init__.py  only boilerplate 
> needed to be added

It would make sense to import the library only if required at runtime to
have the dependency optionnal (this should then be documented in the
sample configuration file).

> * the new  repository/xattrMaildir.py  is also essentially boilerplate; 
> it is identical to  repository/Maildir.py  except for a replacement of 
> reference to folder.Maildir.Maildir to folder.xattrMaildir.xattrMaildir

Didn't check: do you use polymorphism?

> * the new  folder/xattrMaildir.py  modifies three methods from 
> folder/Maildir.py, and in a minimal fashion:
> 
>    - in quickchanged the check whether the flags have been changed as 
> reverted to an equality check (back from the subset check introduced in 
> my previous patch)
> 
>    - _scanfolder and savemessageflags we actually read and write the 
> extended attributes (look for 'xattr.get' and 'xattr.set'), but these 
> are essentially one-line changes/additions.

Did you refactored original methods to avoid duplicated code? Such
"oneline" changes in the code looks like good place for refactoring
(with an additional optional parameter in the original methods, I guess).

> Based on the realization that the changes needed are so small, it may be 
> considered that no new repository is introduced, but that the 
> functionality is activated by an option that controls the then three 
> necessary conditionals.

I think the new repository type approach is good because of the library
dependency issue.

> Again, your testing and critique is very much welcome. The code is a 
> proof-of-concept and therefore very rough.
> 
> 
> Best,
> 
> Erik
> 
> P.S.: As with my previous patch, I've also added it in attachment to 
> deal with copy-paste introduced linebreaks.

Good. :-)

Sorry, I don't have time to review the code much.

-- 
Nicolas Sebrecht