[MERGE] Fix RemoteRepository.get_parent_map stacking

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Aaron Bentley wrote:

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

That is what I'm asking for.

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


=== modified file 'NEWS'
- --- NEWS	2008-11-14 02:13:07 +0000
+++ NEWS	2008-11-18 17:06:33 +0000
@@ -34,6 +34,9 @@
       tries to abort a write group.  This allows the root cause (e.g. a
       network interruption) to be reported.  (Andrew Bennetts, #297014)

+    * RemoteRepository.get_parent_map now uses fallback repositories.
+      (Aaron Bentley)
+

^- Isn't there a bug number associated with this?


 class RemoteRepository(_RpcHelper):
     """Repository accessed over rpc.

@@ -334,6 +345,7 @@
         self.base = self.bzrdir.transport.base
         # Additional places to query for data.
         self._fallback_repositories = []
+        self._parents_provider = None

...

     def _make_parents_provider(self):
- -        return self
+        if self._parents_provider is None:
+            providers = [_UnstackedParentsProvider(self)]
+            providers.extend(r._make_parents_provider() for r in
+                             self._fallback_repositories)
+            self._parents_provider =
graph._StackedParentsProvider(providers)
+        return self._parents_provider

^- I sort of like that it is cached, but I'm concerned about someone
mutating self._fallback_repositories that doesn't know to reset
self._parents_provider.


Certainly I think you need to go change
"Repository.add_fallback_repository()" to set self._parents_provider =
None, and probably need a test case surrounding it.

(Possibly we need to add RemoteRepository.add_fallback_repository() and
only set it there.)

Sorry I didn't really think about it earlier, but as you can mutate the
fallback repositories on an existing repo, either we shouldn't cache the
parent provider, or we need to nuke it appropriately.

Another question is whether it should be maintained across locks, etc.
But at the moment it just needs to have the same lifetime as the exact
set of fallback repos.


Speaking of which, we already have a bug when adding a fallback
repository to a locked repo. Namely that calling "unlock()" calls unlock
to all of the stacked-on repositories (because lock_read/write() does
the reverse), but we don't know if the fallback repo has the same count
as the current repo. This certainly isn't anything to do with you, and
you don't need to fix it, I just saw it while inspecting the code.


I agree that this is necessary to get stacked branches to function
properly, and we should consider getting this merged and getting a 1.9.1
with it as well. (Did you happen to build your changes against 1.9 to
start with?)

BB:tweak

You can chose whether you want to track down when you need to get rid of
the current "self._parents_provider" or just not cache the value. I'm
leaning toward the latter as the quick fix.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkkjFAoACgkQJdeBCYSNAAOKXACdG12gLivV2PgHscRYKTEA3xOc
6u0AoIyq/KGz3XI2b5Is161SEDp46f8F
=S974
-----END PGP SIGNATURE-----