[MERGE] Fix RemoteRepository.get_parent_map stacking

[Aaron Bentley]

John Arbash Meinel wrote:
> -    def get_parent_map(self, keys):
> +    def get_parent_map(self, revision_ids):
> +        return self._make_parents_provider.get_parent_map(revision_ids)
> +
> +    def _get_parent_map(self, keys):
> 
> ^- I'm pretty positive that you need an extra set of parentheses here:
> self._make_parents_provider().get_parent_map(revision_ids)

Doh, yes.

> 
> Which also indicates that this code path isn't actually being tested.

Actually, it is tested, but I didn't run the full suite.  This version
is fully tested.

> It also seems a bit circuitous to have get_parent_map() return a locally
> defined class whose only purpose is to call _get_parent_map().

I agree.  The only reason Repositories support get_parent_map is so they
can act as ParentsProviders.  But now, that has been hijacked and we can
no longer expect that sane callers will call get_graph.  So we must
provide a ParentsProvider for RemoteRepository.

> I would say that:
> 
> 1) We really don't want a locally defined class, as that causes the
> class to have to be recompiled every time we call
> _make_parents_provider, and as you are doing that anytime we call
> Repository.get_parent_map() it seems a bit much.
> 
> Probably fine to just define it as an extra module-level class (prefixed
> with _), and then pass in the repository as part of the constructor.

Done.  In fact, it is instantiated lazily.

I should mention that Repository.get_parent_map is not expected to be
efficient.  On pack repositories, that will give inferior caching.  If
you care about efficiency, use Graph.

> 2) How much overhead does this introduce versus the fairly common case
> of no fallback repositories? Is it worth just doing:
> 
> if not self._fallback_repositories:
>   return self

That would be infinite recursion:
self.get_parent_map -> self._make_parents_provider -> self.get_parent_map

> 3) Updating the docstring for "_get_parent_map()" to indicate that this
> is "get parents from only this repository and no stacked on locations"
> would probably be a reasonable way to clarify how it is different from
> the existing functions.

Done.

> 4) It seems like something that should be codified into a test,
> something like stacked_get_parent_map that just asserts that
> _get_parent_map() doesn't return the values from stacked-on locations.
> (More importantly, doesn't *look* at stacked on locations, repeating the
> work that is done elsewhere.)

Are you saying you want an effort test?  I think this is overkill for a
bugfix.  I don't mind adding a direct test that
_get_parent_map(['rev1']) returns nothing.


> 5) I'm not particularly fond of RemoteRepository.get_parent_map() taking
> an api that is meant to be fairly lightweight and deal with simple dicts
> and tuples, into creating temporary objects to wrap Repository requests
> and then get thrown away. It isn't terrible, but I do wonder if it
> wouldn't be better overall to just teach RemoteBranch.get_parent_map()
> to use (pseudocode):

No, that would be a DRY violation.  We would then have to test this code
thoroghly, instead of relying on the fact that StackedParentsProvider is
already tested.

This is intended to be a minimal bugfix, because I want it in pronto,
because this is hurting launchpad developers every day.  I don't want to
introduce more codepaths to test, if I can help it.

IMO, an ideal fix would remove all this special-case code for caching
RemoteRepositories and generalize it into ParentsProvider classes.  But
I don't want this bugfix to wait for ideal.

Aaron
-------------- next part --------------
A non-text attachment was scrubbed...
Name: unstacked-parents-provider.diff
Type: text/x-patch
Size: 10669 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20081118/fe30f4e4/attachment-0001.bin