[PATCH] Make sure folder filters are always respected.

[Dave Abrahams]

on Fri Aug 31 2012, Sebastian Spaeth <Sebastian-AT-SSpaeth.de> wrote:

> Dave Abrahams <dave at boostpro.com> writes:
>
> Hi Dave, very cool. THanks for that. And sorry for the long delay. I
> agree that the patch should be split in smaller batches. WIll make
> bisecting and reviewing easier. I have split them in 3 and pushed to the
> next branch. Some comments below.
>
>> * Value of sync_this is determined at Folder __init__ time so it is
>>   never inadvertently skipped.
>
> Was this a problem somewhere or is it "just" a cleanup? I agree that
> doing this at init time is much cleaner and nicer than sprinkling the
> code throughout, so I am all for that. This went in the first patch.

I don't remember; it's been too long.

>> * Fix omitted check for local folder's sync_this value in
>>   SyncableAccount.sync()
>
> The check was not omitted, but came later in syncfilter() when the
> localfolder had been loaded. But it's better to check both at the same time.
> Included
>
>> * Add should_sync_folder() method to Repository
>
> This is the second patch.
>> * Eliminate DRY violations
>
> Huh? DRY? dry-run or an acronym that I should be knowing? :-)

The latter: http://en.wikipedia.org/wiki/Don't_repeat_yourself

>>   - Factor out the code to find a local folder given a remote folder
>
> Patch #1
>
>>   - Factor out the code that reports on skipped
>>     folders (sync_this_verbose)
>
> This is the only thin not included for now. All the ui stuff is
> historically in the ui classes and we call something like
> ui.ignore_folder(folder) which outputs the message.

As someone who wants granular commits, I can't understand your
hesitation about a patch consolidating repeated code in the file where
it appears just because the original logic should have been somewhere
else in the first place.  Consolidate first, then feel free to move it
wherever you like.

> This is basically the 4. patch that I haven't got to create yet.
>
> Do check commits:
> 7d1d528 No need to filter Maildir folders here
> e94642b Determine folder syncing on Folder initialization
> e7ca5b2 Combine checks for ignored folders in one place
>
> in the next branch and see if they are according to your intentions.

You dropped the check for localfolder's need to synchronize in
accounts.SyncableAccount.sync and accounts.syncfolder.  You dropped the
use of self._sync_this in BaseFolder and the call to
repository.should_sync_folder

on Fri Aug 31 2012, Sebastian Spaeth <Sebastian-AT-SSpaeth.de> wrote:

> Dave Abrahams <dave at boostpro.com> writes:
>
>> * Fix omitted check for local folder's sync_this value in
>>   SyncableAccount.sync()
>
> Hi Dave, I just pushed a patch fixing the fallout from your
> change. There was a reason that we would not call getfolder() until we
> had made sure that the remote folder exists and is not filtered
> out....

I am almost 100% certain this is due to your changes to patch.  Why not
try the test suite after applying the patch I sent you and then, if you
must split it up, verify that the splitting doesn't introduce a
difference?


-- 
Dave Abrahams
BoostPro Computing                  Software Development        Training
http://www.boostpro.com             Clang/LLVM/EDG Compilers  C++  Boost