Rev 6045: Some cleanups suggested by Vincent. in http://bazaar.launchpad.net/~jameinel/bzr/2.4-get_cached_parent_map-388269

[John Arbash Meinel]

At http://bazaar.launchpad.net/~jameinel/bzr/2.4-get_cached_parent_map-388269

------------------------------------------------------------
revno: 6045
revision-id: john at arbash-meinel.com-20110826112629-ft6pgyb2r39t7ifq
parent: john at arbash-meinel.com-20110825175350-x4rs77617rll1tth
committer: John Arbash Meinel <john at arbash-meinel.com>
branch nick: 2.4-get_cached_parent_map-388269
timestamp: Fri 2011-08-26 13:26:29 +0200
message:
  Some cleanups suggested by Vincent.
-------------- next part --------------
=== modified file 'bzrlib/graph.py'
--- a/bzrlib/graph.py	2011-08-25 17:53:50 +0000
+++ b/bzrlib/graph.py	2011-08-26 11:26:29 +0000
@@ -58,16 +58,16 @@
     def __repr__(self):
         return 'DictParentsProvider(%r)' % self.ancestry
 
+    # Note: DictParentsProvider does not implement get_cached_parent_map
+    #       Arguably, the data is clearly cached in memory. However, this class
+    #       is mostly used for testing, and it keeps the tests clean to not
+    #       change it.
+
     def get_parent_map(self, keys):
         """See StackedParentsProvider.get_parent_map"""
         ancestry = self.ancestry
         return dict((k, ancestry[k]) for k in keys if k in ancestry)
 
-    # Note: DictParentsProvider does not implement get_cached_parent_map
-    #       Argubly, the data is clearly cached in memory. However, this class
-    #       is mostly used for testing, and it keeps the tests clean to not
-    #       change it.
-
 
 class StackedParentsProvider(object):
     """A parents provider which stacks (or unions) multiple providers.
@@ -96,8 +96,8 @@
         """
         found = {}
         remaining = set(keys)
-        # TODO: This adds a getattr() call to each get_parent_map call.
-        #       Performance test it.
+        # TODO: jam 20110826 This adds a getattr() call to each get_parent_map
+        #       call.  Performance test it.
         for parents_provider in self._parent_providers:
             get_cached = getattr(parents_provider, 'get_cached_parent_map',
                                  None)

=== modified file 'bzrlib/tests/per_repository_reference/test_graph.py'
--- a/bzrlib/tests/per_repository_reference/test_graph.py	2011-08-25 17:50:15 +0000
+++ b/bzrlib/tests/per_repository_reference/test_graph.py	2011-08-26 11:26:29 +0000
@@ -68,6 +68,22 @@
         builder.finish_series()
         return master_b, stacked_b
 
+    def assertParentMapCalls(self, expected):
+        """Check that self.hpss_calls has the expected get_parent_map calls."""
+        get_parent_map_calls = []
+        for c in self.hpss_calls:
+            # Right now, the only RPCs that get called are get_parent_map. If
+            # this changes in the future, we can change this to:
+            # if c.call.method != 'Repository.get_parent_map':
+            #    continue
+            self.assertEqual('Repository.get_parent_map', c.call.method)
+            args = c.call.args
+            location = args[0]
+            self.assertEqual('include-missing:', args[1])
+            revisions = sorted(args[2:])
+            get_parent_map_calls.append((location, revisions))
+        self.assertEqual(expected, get_parent_map_calls)
+
     def test_doesnt_call_get_parent_map_on_all_fallback_revs(self):
         if not isinstance(self.repository_format,
                           remote.RemoteRepositoryFormat):
@@ -82,21 +98,7 @@
         res = target_b.repository.search_missing_revision_ids(
                 stacked_b.repository, revision_ids=['F'],
                 find_ghosts=False)
-        # The hook format is a bit raw, so lets clean it up a bit for the
-        # assertion
-        get_parent_map_calls = []
-        for c in self.hpss_calls:
-            # Right now, the only RPCs that get called are get_parent_map. If
-            # this changes in the future, we could change this to:
-            # if c.call.method != 'Repository.get_parent_map':
-            #    continue
-            self.assertEqual('Repository.get_parent_map', c.call.method)
-            args = c.call.args
-            location = args[0]
-            self.assertEqual('include-missing:', args[1])
-            revisions = sorted(args[2:])
-            get_parent_map_calls.append((location, revisions))
-        self.assertEqual([
+        self.assertParentMapCalls([
             # One call to stacked to start, which returns F=>E, and that E
             # itself is missing, so when we step, we won't look for it.
             ('extra/stacked/', ['F']),
@@ -106,7 +108,7 @@
             # And then one get_parent_map call to the target, to see if it
             # already has any of these revisions.
             ('extra/target_repo/branch/', ['A', 'B', 'C', 'D', 'E', 'F']),
-            ], get_parent_map_calls)
+            ])
         # Before bug #388269 was fixed, there would be a bunch of extra calls
         # to 'extra/stacked', ['D'] then ['C'], then ['B'], then ['A'].
         # One-at-a-time for the rest of the ancestry.