[MERGE] Fix RemoteRepository.get_parent_map stacking
aaron at aaronbentley.com
Tue Nov 18 18:21:44 GMT 2008
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:
> 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.
> 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
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 10669 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20081118/fe30f4e4/attachment-0001.bin
More information about the bazaar