Rev 5069: Fix a bug in update_minimal. in http://bazaar.launchpad.net/~jameinel/bzr/2.2-dirstate-update-minimal-bug

[John Arbash Meinel]

At http://bazaar.launchpad.net/~jameinel/bzr/2.2-dirstate-update-minimal-bug

------------------------------------------------------------
revno: 5069
revision-id: john at arbash-meinel.com-20100802213115-a2x1bdjcze9iwwdb
parent: pqm at pqm.ubuntu.com-20100729120106-psygyxky8i3jh7j7
committer: John Arbash Meinel <john at arbash-meinel.com>
branch nick: 2.2-dirstate-update-minimal-bug
timestamp: Mon 2010-08-02 16:31:15 -0500
message:
  Fix a bug in update_minimal.
  
  If the _maybe_remove_row actually removes a row, then we can't
  be positive that our pointer is still correct.
  In fact, in this test it is wrong, and leads to an incorrectly
  sorted dirstate.
-------------- next part --------------
=== modified file 'bzrlib/dirstate.py'
--- a/bzrlib/dirstate.py	2010-01-13 23:06:42 +0000
+++ b/bzrlib/dirstate.py	2010-08-02 21:31:15 +0000
@@ -1566,7 +1566,7 @@
             return
         id_index = self._get_id_index()
         for file_id in new_ids:
-            for key in id_index.get(file_id, []):
+            for key in id_index.get(file_id, ()):
                 block_i, entry_i, d_present, f_present = \
                     self._get_block_entry_index(key[0], key[1], tree_index)
                 if not f_present:
@@ -1980,7 +1980,7 @@
                                           ' tree_index, file_id and path')
             return entry
         else:
-            possible_keys = self._get_id_index().get(fileid_utf8, None)
+            possible_keys = self._get_id_index().get(fileid_utf8, ())
             if not possible_keys:
                 return None, None
             for key in possible_keys:
@@ -2785,8 +2785,11 @@
                     # loop.
                     other_entry = other_block[other_entry_index]
                     other_entry[1][0] = ('r', path_utf8, 0, False, '')
-                    self._maybe_remove_row(other_block, other_entry_index,
-                        id_index)
+                    if self._maybe_remove_row(other_block, other_entry_index,
+                                              id_index):
+                        # If the row holding this was removed, we need to
+                        # recompute where this entry goes
+                        entry_index, _ = self._find_entry_index(key, block)
 
                 # This loop:
                 # adds a tuple to the new details for each column
@@ -2833,7 +2836,8 @@
             # converted to relocated.
             if path_utf8 is None:
                 raise AssertionError('no path')
-            for entry_key in id_index.setdefault(key[2], set()):
+            assert key in id_index[key[2]]
+            for entry_key in id_index.get(key[2], ()):
                 # TODO:PROFILING: It might be faster to just update
                 # rather than checking if we need to, and then overwrite
                 # the one we are located at.
@@ -2850,6 +2854,7 @@
                         raise AssertionError('not present: %r', entry_key)
                     self._dirblocks[block_index][1][entry_index][1][0] = \
                         ('r', path_utf8, 0, False, '')
+            assert key in id_index[key[2]]
         # add a containing dirblock if needed.
         if new_details[0] == 'd':
             subdir_key = (osutils.pathjoin(*key[0:2]), '', '')
@@ -2863,6 +2868,7 @@
         """Remove index if it is absent or relocated across the row.
         
         id_index is updated accordingly.
+        :return: True if we removed the row, False otherwise
         """
         present_in_row = False
         entry = block[index]
@@ -2873,6 +2879,8 @@
         if not present_in_row:
             block.pop(index)
             id_index[entry[0][2]].remove(entry[0])
+            return True
+        return False
 
     def _validate(self):
         """Check that invariants on the dirblock are correct.

=== modified file 'bzrlib/tests/test_dirstate.py'
--- a/bzrlib/tests/test_dirstate.py	2010-01-25 17:48:22 +0000
+++ b/bzrlib/tests/test_dirstate.py	2010-08-02 21:31:15 +0000
@@ -730,6 +730,22 @@
 
 class TestDirStateManipulations(TestCaseWithDirState):
 
+    def test_update_minimal_updates_id_index(self):
+        state = self.create_dirstate_with_root_and_subdir()
+        self.addCleanup(state.unlock)
+        id_index = state._get_id_index()
+        self.assertEqual(['a-root-value', 'subdir-id'], sorted(id_index))
+        state.add('file-name', 'file-id', 'file', None, '')
+        self.assertEqual(['a-root-value', 'file-id', 'subdir-id'],
+                         sorted(id_index))
+        state.update_minimal(('', 'new-name', 'file-id'), 'f',
+                             path_utf8='new-name')
+        self.assertEqual(['a-root-value', 'file-id', 'subdir-id'],
+                         sorted(id_index))
+        self.assertEqual([('', 'new-name', 'file-id')],
+                         sorted(id_index['file-id']))
+        state._validate()
+
     def test_set_state_from_inventory_no_content_no_parents(self):
         # setting the current inventory is a slow but important api to support.
         tree1 = self.make_branch_and_memory_tree('tree1')