Bazaar

Merge lp:~spiv/bzr/chk-apply-delta-522637-2.0 into lp:bzr/2.0

  1. chk-apply-delta-522637-2.0
  2. Merge into 2.0
Proposed by Andrew Bennetts
Status: Merged
Approved by: Andrew Bennetts
Approved revision: no longer in the source branch.
Merged at revision: 4753
Proposed branch: lp:~spiv/bzr/chk-apply-delta-522637-2.0
Merge into: lp:bzr/2.0
Diff against target: 91 lines (+28/-7)
3 files modified
NEWS (+5/-0)
bzrlib/chk_map.py (+4/-7)
bzrlib/tests/test_chk_map.py (+19/-0)
To merge this branch: bzr merge lp:~spiv/bzr/chk-apply-delta-522637-2.0
Reviewer Review Type Date Requested Status
Andrew Bennetts Approve
Review via email: mp+29840@code.launchpad.net

Commit message

Fix bug in CHKMap.apply_delta that was generating non-canonical CHK maps when entries are removed.

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Andrew Bennetts (spiv) wrote :

John approves.

review: Approve
Revision history for this message
Andrew Bennetts (spiv) wrote :

sent to pqm by email

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2010-07-13 10:21:19 +0000
3+++ NEWS 2010-07-14 08:28:43 +0000
4@@ -29,6 +29,11 @@
5 * Don't traceback trying to unversion children files of an already
6 unversioned directory. (Vincent Ladeuil, #494221)
7
8+* Prevent ``CHKMap.apply_delta`` from generating non-canonical CHK maps,
9+ which can result in "missing referenced chk root keys" errors when
10+ fetching from repositories with affected revisions.
11+ (Andrew Bennetts, #522637)
12+
13 * Raise ValueError instead of a string exception.
14 (John Arbash Meinel, #586926)
15
16
17=== modified file 'bzrlib/chk_map.py'
18--- bzrlib/chk_map.py 2009-08-04 14:10:09 +0000
19+++ bzrlib/chk_map.py 2010-07-14 08:28:43 +0000
20@@ -67,8 +67,6 @@
21 _INTERESTING_NEW_SIZE = 50
22 # If a ChildNode shrinks by more than this amount, we check for a remap
23 _INTERESTING_SHRINKAGE_LIMIT = 20
24-# If we delete more than this many nodes applying a delta, we check for a remap
25-_INTERESTING_DELETES_LIMIT = 5
26
27
28 def _search_key_plain(key):
29@@ -110,7 +108,7 @@
30 into the map; if old_key is not None, then the old mapping
31 of old_key is removed.
32 """
33- delete_count = 0
34+ has_deletes = False
35 # Check preconditions first.
36 new_items = set([key for (old, key, value) in delta if key is not None
37 and old is None])
38@@ -122,12 +120,11 @@
39 for old, new, value in delta:
40 if old is not None and old != new:
41 self.unmap(old, check_remap=False)
42- delete_count += 1
43+ has_deletes = True
44 for old, new, value in delta:
45 if new is not None:
46 self.map(new, value)
47- if delete_count > _INTERESTING_DELETES_LIMIT:
48- trace.mutter("checking remap as %d deletions", delete_count)
49+ if has_deletes:
50 self._check_remap()
51 return self._save()
52
53@@ -535,7 +532,7 @@
54 """Check if nodes can be collapsed."""
55 self._ensure_root()
56 if type(self._root_node) is InternalNode:
57- self._root_node._check_remap(self._store)
58+ self._root_node = self._root_node._check_remap(self._store)
59
60 def _save(self):
61 """Save the map completely.
62
63=== modified file 'bzrlib/tests/test_chk_map.py'
64--- bzrlib/tests/test_chk_map.py 2009-07-16 23:28:49 +0000
65+++ bzrlib/tests/test_chk_map.py 2010-07-14 08:28:43 +0000
66@@ -466,6 +466,25 @@
67 # updated key.
68 self.assertEqual(new_root, chkmap._root_node._key)
69
70+ def test_apply_delete_to_internal_node(self):
71+ # applying a delta should be convert an internal root node to a leaf
72+ # node if the delta shrinks the map enough.
73+ store = self.get_chk_bytes()
74+ chkmap = CHKMap(store, None)
75+ # Add three items: 2 small enough to fit in one node, and one huge to
76+ # force multiple nodes.
77+ chkmap._root_node.set_maximum_size(100)
78+ chkmap.map(('small',), 'value')
79+ chkmap.map(('little',), 'value')
80+ chkmap.map(('very-big',), 'x' * 100)
81+ # (Check that we have constructed the scenario we want to test)
82+ self.assertIsInstance(chkmap._root_node, InternalNode)
83+ # Delete the huge item so that the map fits in one node again.
84+ delta = [(('very-big',), None, None)]
85+ chkmap.apply_delta(delta)
86+ self.assertCanonicalForm(chkmap)
87+ self.assertIsInstance(chkmap._root_node, LeafNode)
88+
89 def test_apply_new_keys_must_be_new(self):
90 # applying a delta (None, "a", "b") to a map with 'a' in it generates
91 # an error.

Subscribers

People subscribed via source and target branches