Rev 4753: (spiv) Fix bug in CHKMap.apply_delta that was generating non-canonical CHK in file:///home/pqm/archives/thelove/bzr/2.0/

[Canonical.com Patch Queue Manager]

At file:///home/pqm/archives/thelove/bzr/2.0/

------------------------------------------------------------
revno: 4753 [merge]
revision-id: pqm at pqm.ubuntu.com-20100714093935-jtbipw6qj2uvzhzk
parent: pqm at pqm.ubuntu.com-20100713120413-c0euzhlqz4aenrye
parent: andrew.bennetts at canonical.com-20100714080610-6obmjlgn3nwsbtaw
committer: Canonical.com Patch Queue Manager <pqm at pqm.ubuntu.com>
branch nick: 2.0
timestamp: Wed 2010-07-14 10:39:35 +0100
message:
  (spiv) Fix bug in CHKMap.apply_delta that was generating non-canonical CHK
   maps when entries are removed. (Andrew Bennetts)
modified:
  NEWS                           NEWS-20050323055033-4e00b5db738777ff
  bzrlib/chk_map.py              chk_map.py-20081001014447-ue6kkuhofvdecvxa-1
  bzrlib/tests/test_chk_map.py   test_chk_map.py-20081001014447-ue6kkuhofvdecvxa-2
=== modified file 'NEWS'
--- a/NEWS	2010-07-13 10:21:19 +0000
+++ b/NEWS	2010-07-14 07:46:55 +0000
@@ -29,6 +29,11 @@
 * Don't traceback trying to unversion children files of an already
   unversioned directory.  (Vincent Ladeuil, #494221)
 
+* Prevent ``CHKMap.apply_delta`` from generating non-canonical CHK maps,
+  which can result in "missing referenced chk root keys" errors when
+  fetching from repositories with affected revisions.
+  (Andrew Bennetts, #522637)
+
 * Raise ValueError instead of a string exception.
   (John Arbash Meinel, #586926)
 

=== modified file 'bzrlib/chk_map.py'
--- a/bzrlib/chk_map.py	2009-08-04 14:10:09 +0000
+++ b/bzrlib/chk_map.py	2010-07-14 07:46:55 +0000
@@ -67,8 +67,6 @@
 _INTERESTING_NEW_SIZE = 50
 # If a ChildNode shrinks by more than this amount, we check for a remap
 _INTERESTING_SHRINKAGE_LIMIT = 20
-# If we delete more than this many nodes applying a delta, we check for a remap
-_INTERESTING_DELETES_LIMIT = 5
 
 
 def _search_key_plain(key):
@@ -110,7 +108,7 @@
             into the map; if old_key is not None, then the old mapping
             of old_key is removed.
         """
-        delete_count = 0
+        has_deletes = False
         # Check preconditions first.
         new_items = set([key for (old, key, value) in delta if key is not None
             and old is None])
@@ -122,12 +120,11 @@
         for old, new, value in delta:
             if old is not None and old != new:
                 self.unmap(old, check_remap=False)
-                delete_count += 1
+                has_deletes = True
         for old, new, value in delta:
             if new is not None:
                 self.map(new, value)
-        if delete_count > _INTERESTING_DELETES_LIMIT:
-            trace.mutter("checking remap as %d deletions", delete_count)
+        if has_deletes:
             self._check_remap()
         return self._save()
 
@@ -535,7 +532,7 @@
         """Check if nodes can be collapsed."""
         self._ensure_root()
         if type(self._root_node) is InternalNode:
-            self._root_node._check_remap(self._store)
+            self._root_node = self._root_node._check_remap(self._store)
 
     def _save(self):
         """Save the map completely.

=== modified file 'bzrlib/tests/test_chk_map.py'
--- a/bzrlib/tests/test_chk_map.py	2009-07-16 23:28:49 +0000
+++ b/bzrlib/tests/test_chk_map.py	2010-07-14 08:06:10 +0000
@@ -466,6 +466,25 @@
         # updated key.
         self.assertEqual(new_root, chkmap._root_node._key)
 
+    def test_apply_delete_to_internal_node(self):
+        # applying a delta should be convert an internal root node to a leaf
+        # node if the delta shrinks the map enough.
+        store = self.get_chk_bytes()
+        chkmap = CHKMap(store, None)
+        # Add three items: 2 small enough to fit in one node, and one huge to
+        # force multiple nodes.
+        chkmap._root_node.set_maximum_size(100)
+        chkmap.map(('small',), 'value')
+        chkmap.map(('little',), 'value')
+        chkmap.map(('very-big',), 'x' * 100)
+        # (Check that we have constructed the scenario we want to test)
+        self.assertIsInstance(chkmap._root_node, InternalNode)
+        # Delete the huge item so that the map fits in one node again.
+        delta = [(('very-big',), None, None)]
+        chkmap.apply_delta(delta)
+        self.assertCanonicalForm(chkmap)
+        self.assertIsInstance(chkmap._root_node, LeafNode)
+
     def test_apply_new_keys_must_be_new(self):
         # applying a delta (None, "a", "b") to a map with 'a' in it generates
         # an error.