<div>Thank you for your patch.</div><div><br></div>Can you provide me a way to test it (what fs should I use, options and so on) ?<div>How different is the new xattrMaildir file to the existing one ? Would it be possible to derive from the initial class and change the method that needs to be changed ?</div>
<div>What's the purpose of this, compared to the version with the modified filenames ?</div><div>Would it work with an external tool (or the synced flags will be ignored ?)</div><div><br></div><div>Best regards,</div>
<div>Cyril<br><br><div class="gmail_quote">On Mon, Mar 11, 2013 at 7:01 PM, Nicolas Sebrecht <span dir="ltr"><<a href="mailto:nicolas.s-dev@laposte.net" target="_blank">nicolas.s-dev@laposte.net</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Sun, Mar 10, 2013 at 09:48:25PM +0100, Erik Quaeghebeur wrote:<br>
> Dear maintainers & other interested subscribers,<br>
><br>
><br>
> To be applied on top of the previous patch I sent to this mailing list,<br>
> this patch adds a new repository type, xattrMaildir, a Maildir<br>
> repository where message files have all IMAP flags in an extended<br>
> attribute 'user.org.offlineimap.flags'. (It is perfectly backwards<br>
> compatible with the Maildir specification.)<br>
><br>
> As before I have done a (successful) test that is minimal: for one<br>
> message, I synced from an IMAP server to an xattrMaildir repository and<br>
> verified (using the getfattr command-line tool) that all IMAP flags were<br>
> indeed synced.<br>
><br>
> The code uses the xattr module (<a href="http://pyxattr.k1024.org/module.html" target="_blank">http://pyxattr.k1024.org/module.html</a>) as<br>
> an extra dependency. Actually, very little has changed:<br>
><br>
> * in  folder/__init__.py  and  repository/__init__.py  only boilerplate<br>
> needed to be added<br>
<br>
</div>It would make sense to import the library only if required at runtime to<br>
have the dependency optionnal (this should then be documented in the<br>
sample configuration file).<br>
<div class="im"><br>
> * the new  repository/xattrMaildir.py  is also essentially boilerplate;<br>
> it is identical to  repository/Maildir.py  except for a replacement of<br>
> reference to folder.Maildir.Maildir to folder.xattrMaildir.xattrMaildir<br>
<br>
</div>Didn't check: do you use polymorphism?<br>
<div class="im"><br>
> * the new  folder/xattrMaildir.py  modifies three methods from<br>
> folder/Maildir.py, and in a minimal fashion:<br>
><br>
>    - in quickchanged the check whether the flags have been changed as<br>
> reverted to an equality check (back from the subset check introduced in<br>
> my previous patch)<br>
><br>
>    - _scanfolder and savemessageflags we actually read and write the<br>
> extended attributes (look for 'xattr.get' and 'xattr.set'), but these<br>
> are essentially one-line changes/additions.<br>
<br>
</div>Did you refactored original methods to avoid duplicated code? Such<br>
"oneline" changes in the code looks like good place for refactoring<br>
(with an additional optional parameter in the original methods, I guess).<br>
<div class="im"><br>
> Based on the realization that the changes needed are so small, it may be<br>
> considered that no new repository is introduced, but that the<br>
> functionality is activated by an option that controls the then three<br>
> necessary conditionals.<br>
<br>
</div>I think the new repository type approach is good because of the library<br>
dependency issue.<br>
<div class="im"><br>
> Again, your testing and critique is very much welcome. The code is a<br>
> proof-of-concept and therefore very rough.<br>
><br>
><br>
> Best,<br>
><br>
> Erik<br>
><br>
> P.S.: As with my previous patch, I've also added it in attachment to<br>
> deal with copy-paste introduced linebreaks.<br>
<br>
</div>Good. :-)<br>
<br>
Sorry, I don't have time to review the code much.<br>
<span class="HOEnZb"><font color="#888888"><br>
--<br>
Nicolas Sebrecht<br>
<br>
_______________________________________________<br>
OfflineIMAP-project mailing list<br>
<a href="mailto:OfflineIMAP-project@lists.alioth.debian.org">OfflineIMAP-project@lists.alioth.debian.org</a><br>
<a href="http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/offlineimap-project" target="_blank">http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/offlineimap-project</a><br>
<br>
OfflineIMAP homepage: <a href="http://software.complete.org/offlineimap" target="_blank">http://software.complete.org/offlineimap</a><br>
</font></span></blockquote></div><br></div>