Rev 3177: Implement get_parent_map for RemoteRepository with caching, based on get_revision_graph. in http://people.ubuntu.com/~robertc/baz2.0/remote.graph

[Robert Collins]

At http://people.ubuntu.com/~robertc/baz2.0/remote.graph

------------------------------------------------------------
revno: 3177
revision-id:robertc at robertcollins.net-20080111055647-tdn8bkdri6q5i39w
parent: robertc at robertcollins.net-20080111053222-dxepbgap3vobkvqd
committer: Robert Collins <robertc at robertcollins.net>
branch nick: remote.graph
timestamp: Fri 2008-01-11 16:56:47 +1100
message:
  Implement get_parent_map for RemoteRepository with caching, based on get_revision_graph.
modified:
  bzrlib/remote.py               remote.py-20060720103555-yeeg2x51vn0rbtdp-1
  bzrlib/tests/test_remote.py    test_remote.py-20060720103555-yeeg2x51vn0rbtdp-2
=== modified file 'bzrlib/remote.py'
--- a/bzrlib/remote.py	2008-01-11 05:32:22 +0000
+++ b/bzrlib/remote.py	2008-01-11 05:56:47 +0000
@@ -265,6 +265,8 @@
         self._lock_token = None
         self._lock_count = 0
         self._leave_lock = False
+        # A cache of looked up revision parent data; reset at unlock time.
+        self._parents_map = None
         # For tests:
         # These depend on the actual remote format, so force them off for
         # maximum compatibility. XXX: In future these should depend on the
@@ -468,6 +470,7 @@
         if not self._lock_mode:
             self._lock_mode = 'r'
             self._lock_count = 1
+            self._parents_map = {}
             if self._real_repository is not None:
                 self._real_repository.lock_read()
         else:
@@ -505,6 +508,7 @@
                 self._leave_lock = False
             self._lock_mode = 'w'
             self._lock_count = 1
+            self._parents_map = {}
         elif self._lock_mode == 'r':
             raise errors.ReadOnlyError(self)
         else:
@@ -564,6 +568,7 @@
         self._lock_count -= 1
         if self._lock_count > 0:
             return
+        self._parents_map = None
         old_mode = self._lock_mode
         self._lock_mode = None
         try:
@@ -732,6 +737,15 @@
         self._ensure_real()
         return self._real_repository.iter_files_bytes(desired_files)
 
+    def get_parent_map(self, keys):
+        """See bzrlib.Graph.get_parent_map()."""
+        # Hack to build up the caching logic.
+        ancestry = self._parents_map
+        missing_revisions = set(key for key in keys if key not in ancestry)
+        if missing_revisions:
+            self._parents_map.update(self.get_revision_graph())
+        return dict((k, ancestry[k]) for k in keys if k in ancestry)
+
     @needs_read_lock
     def get_signature_text(self, revision_id):
         self._ensure_real()
@@ -926,12 +940,6 @@
     def _make_parents_provider(self):
         return self
 
-    def get_parent_map(self, keys):
-        # Thunk across to real for now.
-        self._ensure_real()
-        return self._real_repository._make_parents_provider().get_parent_map(
-            keys)
-
 
 class RemoteBranchLockableFiles(LockableFiles):
     """A 'LockableFiles' implementation that talks to a smart server.

=== modified file 'bzrlib/tests/test_remote.py'
--- a/bzrlib/tests/test_remote.py	2008-01-11 05:32:22 +0000
+++ b/bzrlib/tests/test_remote.py	2008-01-11 05:56:47 +0000
@@ -484,18 +484,6 @@
         return repo, client
 
 
-class TestRepositoryGetGraph(TestRemoteRepository):
-
-    def test_get_graph(self):
-        # get_graph returns a graph with get_parent_map as the repository.
-        responses = []
-        transport_path = 'quack'
-        repo, client = self.setup_fake_client_and_repository(
-            responses, transport_path)
-        graph = repo.get_graph()
-        self.assertEqual(graph._parents_provider, repo)
-
-
 class TestRepositoryGatherStats(TestRemoteRepository):
 
     def test_revid_none(self):
@@ -556,6 +544,61 @@
                          result)
 
 
+class TestRepositoryGetGraph(TestRemoteRepository):
+
+    def test_get_graph(self):
+        # get_graph returns a graph with get_parent_map as the repository.
+        responses = []
+        transport_path = 'quack'
+        repo, client = self.setup_fake_client_and_repository(
+            responses, transport_path)
+        graph = repo.get_graph()
+        self.assertEqual(graph._parents_provider, repo)
+
+
+class TestRepositoryGetParentMap(TestRemoteRepository):
+
+    def test_get_parent_map_caching(self):
+        # get_parent_map returns from cache until unlock()
+        # setup a reponse with two revisions
+        r1 = u'\u0e33'.encode('utf8')
+        r2 = u'\u0dab'.encode('utf8')
+        lines = [' '.join([r2, r1]), r1]
+        encoded_body = '\n'.join(lines)
+        responses = [(('ok', ), encoded_body), (('ok', ), encoded_body)]
+
+        transport_path = 'quack'
+        repo, client = self.setup_fake_client_and_repository(
+            responses, transport_path)
+        repo.lock_read()
+        graph = repo.get_graph()
+        parents = graph.get_parent_map([r2])
+        self.assertEqual({r2: (r1,)}, parents)
+        # locking and unlocking deeper should not reset
+        repo.lock_read()
+        repo.unlock()
+        parents = graph.get_parent_map([r1])
+        self.assertEqual({r1: ()}, parents)
+        self.assertEqual(
+            [('call_expecting_body', 'Repository.get_revision_graph',
+             ('///quack/', ''))],
+            client._calls)
+        repo.unlock()
+        # now we call again, and it should use the second response.
+        repo.lock_read()
+        graph = repo.get_graph()
+        parents = graph.get_parent_map([r2])
+        self.assertEqual({r2: (r1,)}, parents)
+        self.assertEqual(
+            [('call_expecting_body', 'Repository.get_revision_graph',
+              ('///quack/', '')),
+             ('call_expecting_body', 'Repository.get_revision_graph',
+              ('///quack/', ''))
+            ],
+            client._calls)
+        repo.unlock()
+
+
 class TestRepositoryGetRevisionGraph(TestRemoteRepository):
     
     def test_null_revision(self):
@@ -719,7 +762,7 @@
             responses, transport_path)
 
         # The null revision is always there, so has_revision(None) == True.
-        self.assertEqual(True, repo.has_revision(None))
+        self.assertEqual(True, repo.has_revision(NULL_REVISION))
 
         # The remote repo shouldn't be accessed.
         self.assertEqual([], client._calls)