[MERGE] Move RemoteRepository caching logic to CachedParentsProvider

[Aaron Bentley]

Andrew Bennetts wrote:
> Aaron Bentley wrote:
>> Andrew Bennetts wrote:
> [...]
>>> enable_cache not only enables caching, it also clears an already enabled
>>> cache.  I think that's a bit dangerous.
>> How?  It's a cache.  It shouldn't contain anything we can't get again.
> 
> “dangerous” was a bit harsh.  “easy to misuse in a detrimental way” is what
> I was trying to say, but is bit a wordier :)
> 
> I think the current combination of behaviour and method name is very likely
> to lead to someone calling “enable_cache” on an already enabled and
> populated cache, thus silently emptying the cache.

Maybe I lack imagination, but I thought that was pretty unlikely.

> So I think that either this method should not empty an already active cache,
> or it should raise an exception if someone tries to double-enable.  I'm
> pretty sure that no-one will call enable_cache() and expect *less* caching
> to happen than if they hadn't called it.

Okay.

>>> How about making it raise
>>> AssertionError if self._parents_map is not None?
>> I don't think that the current policy against assertions is sensible.
>> But since there is one, I don't think I should be raising
>> AssertionErrors, either.
> 
> There's no policy against assertions.  There's a policy against assert
> *statements*, because of the unhelpful way that Python implements them.

I thought the argument was that if it was worth dealing with at all, it
was worth doing more than raising an assertion.

So I decided that the double-enable issue wasn't worth doing more for.

> Making an assertion with an explicit if and raise is fine.

Raising an AssertionError could cause a test case to fail rather than
erroring, so it seems sub-optimal to me.

>> RemoteRepository.get_graph should assume that access to self is slower
>> than access to other_repository, and so it should put other_repository
>> first in the stacking order.
> 
> I don't see how it doesn't “make much sense” to have nested
> StackedParentsProviders.

You get the same results, but it's harder to follow the flow of control,
and it's observably slower.

Aaron
-------------- next part --------------
A non-text attachment was scrubbed...
Name: enable_cache_error.patch
Type: text/x-patch
Size: 4001 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20081210/6cb04d74/attachment.bin