[MERGE] Move RemoteRepository caching logic to CachedParentsProvider

[Andrew Bennetts]

Aaron Bentley wrote:
> Andrew Bennetts wrote:
[...]
> > 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.

Not AFAIK.  The current code base (277 raises of AssertionError) and the
HACKING file both seem to agree with my understanding.

> 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.

We can (and arguably should) change the failureException on
bzrlib.tests.TestCase to be a subclass of AssertionError.  But in practice I
don't think anyone really cares about the difference between FAIL and ERROR;
either way the test failed, and the distinction only matters as a hint for
debugging why.

> >> 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.

Ok.  I agree with the analysis of that, I just don't agree with the
severity.  It strikes me a very minor issue, minor enough that I personally
wouldn't introduce extra complexity just to avoid it, as I'd consider extra
complexity to be worse.

-Andrew.