Recommendations for patches (was: Re: [PATCH] Make sure folder filters are always respected.)

[Rogério Brito]

Hi there.

On Thu, 19 Jul 2012 19:18:57 -0400, Dave Abrahams wrote:

> * Value of sync_this is determined at Folder __init__ time so it is
>   never inadvertently skipped.
> 
> * Fix omitted check for local folder's sync_this value in
>   SyncableAccount.sync()
> 
> * Add should_sync_folder() method to Repository
> 
> * Eliminate DRY violations
> 
>   - Factor out the code to find a local folder given a remote folder
> 
>   - Factor out the code that reports on skipped
>     folders (sync_this_verbose)
(...)

I am not any one of the offlineimap developers, but I guess that it would 
be good to have this patch (or *any* series of patches) smaller. In 
particular, this should, most likely, be a set of patches with at least 2 
patches:

* 1 for adding the new properties etc. that you want (not changing the 
behavior of the code).
* 1 for actually enabling the functionality of the code that you added.
* 1 for factoring out (without modifying the behavior of the code).

Maybe more. This way (and, in particular, leaving the factoring out at 
last) makes things easier for other tasks. Some that spring to mind are:

* it's easier to review the code.
* cherry-pick specific patches (say, one intends to maintain a stable 
branch of offlineimap).
* make git-bisect even possible to catch regressions.

These are big things, IMVHO, and I just had some problems with git 
bisect'ing offlineimap right now, as not every point that git bisect 
chose was able to run.

In general, it would be good to commit something in, check that it works, 
commit more things, check etc. Of course, things may depend on your setup 
(configuration file, etc), but, anyway, that's a very valuable goal to 
try to achieve.


Regards,

-- 
Rogério Brito : rbrito@{ime.usp.br,gmail.com} : GPG key 4096R/BCFCAAAA
http://rb.doesntexist.org/blog : Projects : https://github.com/rbrito/
DebianQA: http://qa.debian.org/developer.php?login=rbrito%40ime.usp.br