[MERGE] Move RemoteRepository caching logic to CachedParentsProvider

[Andrew Bennetts]

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.

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.

> > 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.
Making an assertion with an explicit if and raise is fine.

> > If only we
> > had a ParentProvider abstract base class/interface class, so there was a
> > clearly canonical place to document get_parent_map.  Oh well.
> 
> Duck typing, man.  It's the way of the future.

Actually, if you look at Python 3, abstract base classes are!

[...]
> > AFAICT, RemoteRepository.get_graph should just be identical to
> > Repository.get_graph.
> 
> I disagree.  It doesn't make much sense to put a StackedParentsProvider
> inside a StackedParentsProvider.  We should just create a
> StackedParentsProvider that includes:
> - self._unstacked_parents_provider
> - all fallback providers
> - other_repository's parents provider
> 
> 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.  I can see how it might not be optimal or as clear
as an alternative in given situation, but I don't see anything fundamentally
nonsensical about it.

I am ok with a get_graph implementation that is different for reasons like
putting other_repository first.  I'm just confused by objection to nesting.

[...]
> >> +        self.assertEqual({'b':None}, self.caching_pp._parents_map)
> > 
> > Why aren't these tests consistent about whether they use get_cached_map() or
> > _parents_map?
> 
> get_cached_map does not include cache misses.

Oh, I see.  Makes sense.

-Andrew.