[MERGE] updated index bisection

Martin Pool mbp at canonical.com
Thu Oct 11 10:47:40 BST 2007


Martin Pool has voted comment.
Status is now: Semi-approved
Comment:
I think you need a little bit of commentary at the Index level 
explaining the concepts which are added:

  key references
  that the index can be partially parsed into memory, and
  we remember both what keys we've seen and what bytes they were

          self._nodes = None

Since you're adding comments about the others it would be good to add 
one for
_nodes, particularly since its function is less clear if the index is
partially parsed.

+    def _resolve_references(self, references):
+        """Return the resolved key references for references."""

The docstring doesn't tell me much more than the method name...


+    def lookup_keys_via_location(self, location_keys):
+        """Public interface for implementing bisection.

Is this intended to be a public interface?  It looks more like a special
callback for bisection and so it shouldn't be public.

This method is really a bit long.  It looks like it should be possible
to split up into its several sub-tasks of searching the cache, getting
the headers,

+        :param location_keys: A list of location, key tuples.
+        :return: A list of (location_key, result) tuples as expected by
+            bzrlib.bisect_multi.bisect_multi_bytes.

And location is a byte offset?

You have some cases of missing spaces around = for assignment.

It looks like the index has three almost disjoint states and behaviors: 
entirely loaded, partially loaded, and not loaded at all.  I wonder if 
it would be simpler to factor out an inner object with two classes for 
the different strategies.

s/ajust/adjust

to be continued...


For details, see: 
http://bundlebuggy.aaronbentley.com/request/%3C1192078676.15845.1.camel%40lifeless-64%3E



More information about the bazaar mailing list