Code review comment for lp:~vila/bzr/948339-config-caching

> Summarising from discussion on IRC yesterday,
>
> The lock count change doesn't seem to be directly asserted by
> test_set_delays_write. Checking now though the test does in fact fail without
> the fix, as does test_set_from_config_stack_get_from_config in a similar
> manner, so that's fine.

Yeah, the more I think about remarks like this (or one I do myself during reviews), the more I believe I should keep more detailed notes while working on the fix and have a way to mention the narrative in the cover letter... You rightly understand how I proceeded, well done Watson ;) Err, Sherlock ? ;-p

>
> Flipping the expectedFailure assertions where the behaviour isn't worth fixing
> is a little odd,

It's more than 'not worth fixing', it's really that trying to maintain compatibility between the two APIs can't be done while still reducing the IOs. The old API *really* relies in many ways on the fact that each and every change is saved to disk. No matter how you use the old API, you will never miss a change: from different objects in the same process, from different processes, everything works. This came with a price: read the config file for each and every option. Read *and* write the config file for each and every change.

The new implementation already dropped some possible optimisations to ease the transition and is safe (AFAIK) to mix with the old one *as long* as one API only is used for a given option.

So dropping the IO reduction for compatibility sake ? I say no. Some migrations will be harder than others but well, they'll have to happen, so the sooner the better.

> though the added reasoning is nice. Ideally, it would live
> not here but somewhere that people trying to use config and running into
> problems might actually find.

Should I refer to these tests in doc/developers/configuration.txt then ?

>
> The added test_mutable_section_shared is nice and easy to follow, so even
> though I find the list->dict fixes slightly mysterious without working out
> more about the config details, I'm happy with the change.

Ok. I'll add a comment to better explain why it's important that MutableSection should stay unique to be truly shared in the __init__ method then, would that have helped ?