Rev 2635: Review comments. in http://people.ubuntu.com/~robertc/baz2.0/index

[Robert Collins]

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

------------------------------------------------------------
revno: 2635
revision-id: robertc at robertcollins.net-20070728013353-qk7394wehmg2iiph
parent: robertc at robertcollins.net-20070727141454-q70m5m38oyp0el4c
committer: Robert Collins <robertc at robertcollins.net>
branch nick: index
timestamp: Sat 2007-07-28 11:33:53 +1000
message:
  Review comments.
modified:
  bzrlib/index.py                index.py-20070712131115-lolkarso50vjr64s-1
  bzrlib/tests/test_index.py     test_index.py-20070712131115-lolkarso50vjr64s-2
=== modified file 'bzrlib/index.py'
--- a/bzrlib/index.py	2007-07-27 14:14:54 +0000
+++ b/bzrlib/index.py	2007-07-28 01:33:53 +0000
@@ -112,19 +112,17 @@
                 key_value = key, value
             # possibly should do this on-demand, but it seems likely it is 
             # always wanted
-            subkey = list(reversed(key[:-1]))
-            while len(subkey):
-                if subkey[-1] not in key_dict:
-                    key_dict[subkey[-1]] = {}
-                key_dict = key_dict[subkey[-1]]
-                subkey.pop(-1)
+            # For a key of (foo, bar, baz) create
+            # _nodes_by_key[foo][bar][baz] = key_value
+            for subkey in key[:-1]:
+                key_dict = key_dict.setdefault(subkey, {})
             key_dict[key[-1]] = key_value
 
     def finish(self):
         lines = [_SIGNATURE]
         lines.append(_OPTION_NODE_REFS + str(self.reference_lists) + '\n')
         lines.append(_OPTION_KEY_ELEMENTS + str(self._key_length) + '\n')
-        prefix_length = len(lines[0]) + len(lines[1]) + len(lines[2])
+        prefix_length = sum(len(x) for x in lines)
         # references are byte offsets. To avoid having to do nasty
         # polynomial work to resolve offsets (references to later in the 
         # file cannot be determined until all the inbetween references have
@@ -195,7 +193,7 @@
                     ref_addresses.append(format_string % key_addresses[reference])
                 flattened_references.append('\r'.join(ref_addresses))
             string_key = '\x00'.join(key)
-            lines.append("%s\0%s\0%s\0%s\n" % (string_key, absent,
+            lines.append("%s\x00%s\x00%s\x00%s\n" % (string_key, absent,
                 '\t'.join(flattened_references), value))
         lines.append('\n')
         result = StringIO(''.join(lines))
@@ -243,6 +241,7 @@
         """
         stream = self._transport.get(self._name)
         self._read_prefix(stream)
+        expected_elements = 3 + self._key_length
         line_count = 0
         # raw data keyed by offset
         self._keys_by_offset = {}
@@ -256,6 +255,8 @@
                 trailers += 1
                 continue
             elements = line.split('\0')
+            if len(elements) != expected_elements:
+                raise errors.BadIndexData(self)
             # keys are tuples
             key = tuple(elements[:self._key_length])
             absent, references, value = elements[-3:]
@@ -289,11 +290,10 @@
                     key_value = key, node_value
                 # possibly should do this on-demand, but it seems likely it is 
                 # always wanted
-                while len(subkey):
-                    if subkey[-1] not in key_dict:
-                        key_dict[subkey[-1]] = {}
-                    key_dict = key_dict[subkey[-1]]
-                    subkey.pop(-1)
+                # For a key of (foo, bar, baz) create
+                # _nodes_by_key[foo][bar][baz] = key_value
+                for subkey in key[:-1]:
+                    key_dict = key_dict.setdefault(subkey, {})
                 key_dict[key[-1]] = key_value
         self._keys = set(self._nodes)
         if trailers != 1:
@@ -404,7 +404,7 @@
             # find what it refers to:
             key_dict = self._nodes_by_key
             elements = list(key)
-            # find the subdict to return
+            # find the subdict whose contents should be returned.
             try:
                 while len(elements) and elements[0] is not None:
                     key_dict = key_dict[elements[0]]
@@ -424,8 +424,11 @@
                     else:
                         # yield keys
                         for value in key_dict.itervalues():
+                            # each value is the key:value:node refs tuple
+                            # ready to yield.
                             yield value
             else:
+                # the last thing looked up was a terminal element
                 yield key_dict
 
     def _signature(self):

=== modified file 'bzrlib/tests/test_index.py'
--- a/bzrlib/tests/test_index.py	2007-07-27 14:14:54 +0000
+++ b/bzrlib/tests/test_index.py	2007-07-28 01:33:53 +0000
@@ -64,6 +64,9 @@
             "akey\x00\x00\x00data\n\n", contents)
 
     def test_build_index_one_node_2_element_keys(self):
+        # multipart keys are separated by \x00 - because they are fixed length,
+        # not variable this does not cause any issues, and seems clearer to the
+        # author.
         builder = GraphIndexBuilder(key_elements=2)
         builder.add_node(('akey', 'secondpart'), 'data')
         stream = builder.finish()