Rev 2440: (John Arbash Meinel) Fix bug #105479: properly handle moving a directory with children that have been added, renamed, removed in file:///home/pqm/archives/thelove/bzr/%2Btrunk/

Canonical.com Patch Queue Manager pqm at pqm.ubuntu.com
Sat Apr 21 16:11:42 BST 2007


At file:///home/pqm/archives/thelove/bzr/%2Btrunk/

------------------------------------------------------------
revno: 2440
revision-id: pqm at pqm.ubuntu.com-20070421151139-5wau2ukbpx5z1hv2
parent: pqm at pqm.ubuntu.com-20070421144713-wrfv38pfywoeg408
parent: john at arbash-meinel.com-20070421144319-c0lnova7qdlvvr70
committer: Canonical.com Patch Queue Manager<pqm at pqm.ubuntu.com>
branch nick: +trunk
timestamp: Sat 2007-04-21 16:11:39 +0100
message:
  (John Arbash Meinel) Fix bug #105479: properly handle moving a directory with children that have been added, renamed, removed
modified:
  NEWS                           NEWS-20050323055033-4e00b5db738777ff
  bzrlib/dirstate.py             dirstate.py-20060728012006-d6mvoihjb3je9peu-1
  bzrlib/tests/workingtree_implementations/test_move.py test_move.py-20070225171927-mohn2vqj5fx7edc6-1
  bzrlib/workingtree_4.py        workingtree_4.py-20070208044105-5fgpc5j3ljlh5q6c-1
    ------------------------------------------------------------
    revno: 2438.1.16
    merged: john at arbash-meinel.com-20070421144319-c0lnova7qdlvvr70
    parent: john at arbash-meinel.com-20070421143950-glyo2gbmjwopp2w7
    committer: John Arbash Meinel <john at arbash-meinel.com>
    branch nick: rename_directory_105479
    timestamp: Sat 2007-04-21 09:43:19 -0500
    message:
      NEWS for fixing bug #105479
    ------------------------------------------------------------
    revno: 2438.1.15
    merged: john at arbash-meinel.com-20070421143950-glyo2gbmjwopp2w7
    parent: john at arbash-meinel.com-20070420203953-qljfem7wrpujktl9
    committer: John Arbash Meinel <john at arbash-meinel.com>
    branch nick: rename_directory_105479
    timestamp: Sat 2007-04-21 09:39:50 -0500
    message:
      Remove a superfluous 'else' (recommended by Martin)
    ------------------------------------------------------------
    revno: 2438.1.14
    merged: john at arbash-meinel.com-20070420203953-qljfem7wrpujktl9
    parent: john at arbash-meinel.com-20070420203630-tmc7pwdid0mhkg77
    committer: John Arbash Meinel <john at arbash-meinel.com>
    branch nick: rename_directory_105479
    timestamp: Fri 2007-04-20 15:39:53 -0500
    message:
      Clean up assert helper for missing parent.
    ------------------------------------------------------------
    revno: 2438.1.13
    merged: john at arbash-meinel.com-20070420203630-tmc7pwdid0mhkg77
    parent: john at arbash-meinel.com-20070420202856-mnb25dnlfwgkkqmh
    committer: John Arbash Meinel <john at arbash-meinel.com>
    branch nick: rename_directory_105479
    timestamp: Fri 2007-04-20 15:36:30 -0500
    message:
      Add a test for moving a directory where a child has been moved into a subdir.
    ------------------------------------------------------------
    revno: 2438.1.12
    merged: john at arbash-meinel.com-20070420202856-mnb25dnlfwgkkqmh
    parent: john at arbash-meinel.com-20070420202552-mz87cu22t6kqnyg2
    committer: John Arbash Meinel <john at arbash-meinel.com>
    branch nick: rename_directory_105479
    timestamp: Fri 2007-04-20 15:28:56 -0500
    message:
      Add a test with children that have been swapped
    ------------------------------------------------------------
    revno: 2438.1.11
    merged: john at arbash-meinel.com-20070420202552-mz87cu22t6kqnyg2
    parent: john at arbash-meinel.com-20070420195824-ec0j48hfl8w5rii4
    committer: John Arbash Meinel <john at arbash-meinel.com>
    branch nick: rename_directory_105479
    timestamp: Fri 2007-04-20 15:25:52 -0500
    message:
      Add a test for moving a directory with a renamed child.
    ------------------------------------------------------------
    revno: 2438.1.10
    merged: john at arbash-meinel.com-20070420195824-ec0j48hfl8w5rii4
    parent: john at arbash-meinel.com-20070420195649-hx0feab8y15wdxdj
    committer: John Arbash Meinel <john at arbash-meinel.com>
    branch nick: rename_directory_105479
    timestamp: Fri 2007-04-20 14:58:24 -0500
    message:
      Make WT4.move() properly handle moving a directory with a renamed child.
    ------------------------------------------------------------
    revno: 2438.1.9
    merged: john at arbash-meinel.com-20070420195649-hx0feab8y15wdxdj
    parent: john at arbash-meinel.com-20070420194857-g37q43yd3rsqqk85
    committer: John Arbash Meinel <john at arbash-meinel.com>
    branch nick: rename_directory_105479
    timestamp: Fri 2007-04-20 14:56:49 -0500
    message:
      Add another (failing) test when we move an entry which has a renamed child.
    ------------------------------------------------------------
    revno: 2438.1.8
    merged: john at arbash-meinel.com-20070420194857-g37q43yd3rsqqk85
    parent: john at arbash-meinel.com-20070420194239-ee3ze9cwyoealgl6
    committer: John Arbash Meinel <john at arbash-meinel.com>
    branch nick: rename_directory_105479
    timestamp: Fri 2007-04-20 14:48:57 -0500
    message:
      Fix the case when renaming a directory with deleted children.
    ------------------------------------------------------------
    revno: 2438.1.7
    merged: john at arbash-meinel.com-20070420194239-ee3ze9cwyoealgl6
    parent: john at arbash-meinel.com-20070420193709-m0f5ga3ixhlugbnn
    committer: John Arbash Meinel <john at arbash-meinel.com>
    branch nick: rename_directory_105479
    timestamp: Fri 2007-04-20 14:42:39 -0500
    message:
      While in this area, add a test for renaming a directory with removed children.
      Which actually currently fails.
    ------------------------------------------------------------
    revno: 2438.1.6
    merged: john at arbash-meinel.com-20070420193709-m0f5ga3ixhlugbnn
    parent: john at arbash-meinel.com-20070420193042-d2v07ewsqzemihcp
    committer: John Arbash Meinel <john at arbash-meinel.com>
    branch nick: rename_directory_105479
    timestamp: Fri 2007-04-20 14:37:09 -0500
    message:
      Simplify the test since all we require is renaming a newly added entry's parent dir.
    ------------------------------------------------------------
    revno: 2438.1.5
    merged: john at arbash-meinel.com-20070420193042-d2v07ewsqzemihcp
    parent: john at arbash-meinel.com-20070420182016-yalhqlbsw2fmfl3w
    committer: John Arbash Meinel <john at arbash-meinel.com>
    branch nick: rename_directory_105479
    timestamp: Fri 2007-04-20 14:30:42 -0500
    message:
      Fix the bug by not iterating over the same list we are modifying.
      If a record only exists in one tree (current) move_one() will remove it from
      the current block. Which means that the block we are
      iterating over may change underneath us. So grab a copy before iterating.
      Do some other small doc updates.
    ------------------------------------------------------------
    revno: 2438.1.4
    merged: john at arbash-meinel.com-20070420182016-yalhqlbsw2fmfl3w
    parent: john at arbash-meinel.com-20070420181339-dzr68u9iywq2elxs
    committer: John Arbash Meinel <john at arbash-meinel.com>
    branch nick: rename_directory_105479
    timestamp: Fri 2007-04-20 13:20:16 -0500
    message:
      Extend the test slightly.
      Oddly enough, both newly added records are moved correctly,
      but the existing directory 'c/b/d' is not moved.
    ------------------------------------------------------------
    revno: 2438.1.3
    merged: john at arbash-meinel.com-20070420181339-dzr68u9iywq2elxs
    parent: john at arbash-meinel.com-20070420175713-64x3r196vbqad3y2
    committer: John Arbash Meinel <john at arbash-meinel.com>
    branch nick: rename_directory_105479
    timestamp: Fri 2007-04-20 13:13:39 -0500
    message:
      Add 2 new WT.move() tests, one of which exposes bug #105479
    ------------------------------------------------------------
    revno: 2438.1.2
    merged: john at arbash-meinel.com-20070420175713-64x3r196vbqad3y2
    parent: john at arbash-meinel.com-20070420164821-ml7cw68e09gcr54d
    committer: John Arbash Meinel <john at arbash-meinel.com>
    branch nick: rename_directory_105479
    timestamp: Fri 2007-04-20 12:57:13 -0500
    message:
      Add a check that all entries have a valid parent entry.
      If we have a non-absent entry, it must have a valid directory entry as a parent.
      (You can't have a present entry in an absent directory, or in a missing dir)
    ------------------------------------------------------------
    revno: 2438.1.1
    merged: john at arbash-meinel.com-20070420164821-ml7cw68e09gcr54d
    parent: pqm at pqm.ubuntu.com-20070420154033-kkrk7tn575z1o491
    committer: John Arbash Meinel <john at arbash-meinel.com>
    branch nick: rename_directory_105479
    timestamp: Fri 2007-04-20 11:48:21 -0500
    message:
      Create an assertion to help track down what the bug is.
=== modified file 'NEWS'
--- a/NEWS	2007-04-21 14:15:08 +0000
+++ b/NEWS	2007-04-21 15:11:39 +0000
@@ -124,6 +124,9 @@
     * "dirstate" and "dirstate-tags" formats now produce branches compatible
       with old versions of bzr. (Aaron Bentley, #107168))
 
+    * Handle moving a directory when children have been added, removed,
+      and renamed. (John Arbash Meinel, #105479)
+
   TESTING:
 
     * Added ``bzrlib.strace.strace`` which will strace a single callable and

=== modified file 'bzrlib/dirstate.py'
--- a/bzrlib/dirstate.py	2007-04-20 08:05:24 +0000
+++ b/bzrlib/dirstate.py	2007-04-21 14:39:50 +0000
@@ -2212,6 +2212,28 @@
                     "dirblock for %r is not sorted:\n%s" % \
                     (dirblock[0], pformat(dirblock)))
 
+
+        def check_valid_parent():
+            """Check that the current entry has a valid parent.
+
+            This makes sure that the parent has a record,
+            and that the parent isn't marked as "absent" in the
+            current tree. (It is invalid to have a non-absent file in an absent
+            directory.)
+            """
+            if entry[0][0:2] == ('', ''):
+                # There should be no parent for the root row
+                return
+            parent_entry = self._get_entry(tree_index, path_utf8=entry[0][0])
+            if parent_entry == (None, None):
+                raise AssertionError(
+                    "no parent entry for: %s in tree %s"
+                    % (this_path, tree_index))
+            if parent_entry[1][tree_index][0] != 'd':
+                raise AssertionError(
+                    "Parent entry for %s is not marked as a valid"
+                    " directory. %s" % (this_path, parent_entry,))
+
         # For each file id, for each tree: either
         # the file id is not present at all; all rows with that id in the
         # key have it marked as 'absent'
@@ -2258,6 +2280,7 @@
                             raise AssertionError(
                             "entry %r inconsistent with previous path %r" % \
                             (entry, previous_path))
+                        check_valid_parent()
                 else:
                     if minikind == 'a':
                         # absent; should not occur anywhere else
@@ -2267,6 +2290,7 @@
                         this_tree_map[file_id] = tree_state[1]
                     else:
                         this_tree_map[file_id] = this_path
+                        check_valid_parent()
 
     def _wipe_state(self):
         """Forget all state information about the dirstate."""

=== modified file 'bzrlib/tests/workingtree_implementations/test_move.py'
--- a/bzrlib/tests/workingtree_implementations/test_move.py	2007-03-22 23:47:20 +0000
+++ b/bzrlib/tests/workingtree_implementations/test_move.py	2007-04-20 20:36:30 +0000
@@ -358,6 +358,180 @@
                                ('a/c/d', 'd-id')], tree.basis_tree())
         tree._validate()
 
+    def test_move_directory_into_parent(self):
+        tree = self.make_branch_and_tree('.')
+        self.build_tree(['c/', 'c/b/', 'c/b/d/'])
+        tree.add(['c', 'c/b', 'c/b/d'],
+                 ['c-id', 'b-id', 'd-id'])
+        tree.commit('initial', rev_id='rev-1')
+        root_id = tree.get_root_id()
+
+        self.assertEqual([('c/b', 'b')],
+                         tree.move(['c/b'], ''))
+        self.assertTreeLayout([('', root_id),
+                               ('b', 'b-id'),
+                               ('c', 'c-id'),
+                               ('b/d', 'd-id'),
+                              ], tree)
+        tree._validate()
+
+    def test_move_directory_with_children_in_subdir(self):
+        tree = self.make_branch_and_tree('.')
+        self.build_tree(['a/', 'a/b', 'a/c/', 'd/'])
+        tree.add(['a', 'a/b', 'a/c', 'd'],
+                 ['a-id', 'b-id', 'c-id', 'd-id'])
+        tree.commit('initial', rev_id='rev-1')
+        root_id = tree.get_root_id()
+
+
+        tree.rename_one('a/b', 'a/c/b')
+        self.assertTreeLayout([('', root_id),
+                               ('a', 'a-id'),
+                               ('d', 'd-id'),
+                               ('a/c', 'c-id'),
+                               ('a/c/b', 'b-id'),
+                              ], tree)
+        self.assertEqual([('a', 'd/a')],
+                         tree.move(['a'], 'd'))
+        self.assertTreeLayout([('', root_id),
+                               ('d', 'd-id'),
+                               ('d/a', 'a-id'),
+                               ('d/a/c', 'c-id'),
+                               ('d/a/c/b', 'b-id'),
+                              ], tree)
+        tree._validate()
+
+    def test_move_directory_with_deleted_children(self):
+        tree = self.make_branch_and_tree('.')
+        self.build_tree(['a/', 'a/b', 'a/c', 'a/d', 'b/'])
+        tree.add(['a', 'b', 'a/b', 'a/c', 'a/d'],
+                 ['a-id', 'b-id', 'ab-id', 'ac-id', 'ad-id'])
+        tree.commit('initial', rev_id='rev-1')
+        root_id = tree.get_root_id()
+
+        tree.remove(['a/b', 'a/d'])
+
+        self.assertEqual([('a', 'b/a')],
+                         tree.move(['a'], 'b'))
+        self.assertTreeLayout([('', root_id),
+                               ('b', 'b-id'),
+                               ('b/a', 'a-id'),
+                               ('b/a/c', 'ac-id'),
+                              ], tree)
+        tree._validate()
+
+    def test_move_directory_with_new_children(self):
+        tree = self.make_branch_and_tree('.')
+        self.build_tree(['a/', 'a/c', 'b/'])
+        tree.add(['a', 'b', 'a/c'], ['a-id', 'b-id', 'ac-id'])
+        tree.commit('initial', rev_id='rev-1')
+        root_id = tree.get_root_id()
+
+        self.build_tree(['a/b', 'a/d'])
+        tree.add(['a/b', 'a/d'], ['ab-id', 'ad-id'])
+
+        self.assertEqual([('a', 'b/a')],
+                         tree.move(['a'], 'b'))
+        self.assertTreeLayout([('', root_id),
+                               ('b', 'b-id'),
+                               ('b/a', 'a-id'),
+                               ('b/a/b', 'ab-id'),
+                               ('b/a/c', 'ac-id'),
+                               ('b/a/d', 'ad-id'),
+                              ], tree)
+        tree._validate()
+
+    def test_move_directory_with_moved_children(self):
+        tree = self.make_branch_and_tree('.')
+        self.build_tree(['a/', 'a/b', 'a/c', 'd', 'e/'])
+        tree.add(['a', 'a/b', 'a/c', 'd', 'e'],
+                 ['a-id', 'b-id', 'c-id', 'd-id', 'e-id'])
+        tree.commit('initial', rev_id='rev-1')
+        root_id = tree.get_root_id()
+
+        self.assertEqual([('a/b', 'b')],
+                         tree.move(['a/b'], ''))
+        self.assertTreeLayout([('', root_id),
+                               ('a', 'a-id'),
+                               ('b', 'b-id'),
+                               ('d', 'd-id'),
+                               ('e', 'e-id'),
+                               ('a/c', 'c-id'),
+                              ], tree)
+        self.assertEqual([('d', 'a/d')],
+                         tree.move(['d'], 'a'))
+        self.assertTreeLayout([('', root_id),
+                               ('a', 'a-id'),
+                               ('b', 'b-id'),
+                               ('e', 'e-id'),
+                               ('a/c', 'c-id'),
+                               ('a/d', 'd-id'),
+                              ], tree)
+        self.assertEqual([('a', 'e/a')],
+                         tree.move(['a'], 'e'))
+        self.assertTreeLayout([('', root_id),
+                               ('b', 'b-id'),
+                               ('e', 'e-id'),
+                               ('e/a', 'a-id'),
+                               ('e/a/c', 'c-id'),
+                               ('e/a/d', 'd-id'),
+                              ], tree)
+        tree._validate()
+
+    def test_move_directory_with_renamed_child(self):
+        tree = self.make_branch_and_tree('.')
+        self.build_tree(['a/', 'a/b', 'a/c', 'd/'])
+        tree.add(['a', 'a/b', 'a/c', 'd'],
+                 ['a-id', 'b-id', 'c-id', 'd-id'])
+        tree.commit('initial', rev_id='rev-1')
+        root_id = tree.get_root_id()
+
+        tree.rename_one('a/b', 'a/d')
+        self.assertTreeLayout([('', root_id),
+                               ('a', 'a-id'),
+                               ('d', 'd-id'),
+                               ('a/c', 'c-id'),
+                               ('a/d', 'b-id'),
+                              ], tree)
+        self.assertEqual([('a', 'd/a')],
+                         tree.move(['a'], 'd'))
+        self.assertTreeLayout([('', root_id),
+                               ('d', 'd-id'),
+                               ('d/a', 'a-id'),
+                               ('d/a/c', 'c-id'),
+                               ('d/a/d', 'b-id'),
+                              ], tree)
+        tree._validate()
+
+    def test_move_directory_with_swapped_children(self):
+        tree = self.make_branch_and_tree('.')
+        self.build_tree(['a/', 'a/b', 'a/c', 'a/d', 'e/'])
+        tree.add(['a', 'a/b', 'a/c', 'a/d', 'e'],
+                 ['a-id', 'b-id', 'c-id', 'd-id', 'e-id'])
+        tree.commit('initial', rev_id='rev-1')
+        root_id = tree.get_root_id()
+
+        tree.rename_one('a/b', 'a/bb')
+        tree.rename_one('a/d', 'a/b')
+        tree.rename_one('a/bb', 'a/d')
+        self.assertTreeLayout([('', root_id),
+                               ('a', 'a-id'),
+                               ('e', 'e-id'),
+                               ('a/b', 'd-id'),
+                               ('a/c', 'c-id'),
+                               ('a/d', 'b-id'),
+                              ], tree)
+        self.assertEqual([('a', 'e/a')],
+                         tree.move(['a'], 'e'))
+        self.assertTreeLayout([('', root_id),
+                               ('e', 'e-id'),
+                               ('e/a', 'a-id'),
+                               ('e/a/b', 'd-id'),
+                               ('e/a/c', 'c-id'),
+                               ('e/a/d', 'b-id'),
+                              ], tree)
+        tree._validate()
+
     def test_move_moved(self):
         """Moving a moved entry works as expected."""
         tree = self.make_branch_and_tree('.')

=== modified file 'bzrlib/workingtree_4.py'
--- a/bzrlib/workingtree_4.py	2007-04-20 15:40:33 +0000
+++ b/bzrlib/workingtree_4.py	2007-04-20 20:39:53 +0000
@@ -670,7 +670,6 @@
             new_entry = to_block[1][added_entry_index]
             rollbacks.append(lambda:state._make_absent(new_entry))
 
-        # create rename entries and tuples
         for from_rel in from_paths:
             # from_rel is 'pathinroot/foo/bar'
             from_rel_utf8 = from_rel.encode('utf8')
@@ -774,11 +773,7 @@
 
                 if minikind == 'd':
                     def update_dirblock(from_dir, to_key, to_dir_utf8):
-                        """all entries in this block need updating.
-
-                        TODO: This is pretty ugly, and doesn't support
-                        reverting, but it works.
-                        """
+                        """Recursively update all entries in this dirblock."""
                         assert from_dir != '', "renaming root not supported"
                         from_key = (from_dir, '')
                         from_block_idx, present = \
@@ -795,13 +790,21 @@
                         to_block_index = state._ensure_block(
                             to_block_index, to_entry_index, to_dir_utf8)
                         to_block = state._dirblocks[to_block_index]
-                        for entry in from_block[1]:
+
+                        # Grab a copy since move_one may update the list.
+                        for entry in from_block[1][:]:
                             assert entry[0][0] == from_dir
                             cur_details = entry[1][0]
                             to_key = (to_dir_utf8, entry[0][1], entry[0][2])
                             from_path_utf8 = osutils.pathjoin(entry[0][0], entry[0][1])
                             to_path_utf8 = osutils.pathjoin(to_dir_utf8, entry[0][1])
                             minikind = cur_details[0]
+                            if minikind in 'ar':
+                                # Deleted children of a renamed directory
+                                # Do not need to be updated.
+                                # Children that have been renamed out of this
+                                # directory should also not be updated
+                                continue
                             move_one(entry, from_path_utf8=from_path_utf8,
                                      minikind=minikind,
                                      executable=cur_details[3],
@@ -1920,6 +1923,9 @@
                     #       parent entry will be the same as the source entry.
                     target_parent_entry = state._get_entry(target_index,
                                                            path_utf8=new_dirname)
+                    assert target_parent_entry != (None, None), (
+                        "Could not find target parent in wt: %s\nparent of: %s"
+                        % (new_dirname, entry))
                     target_parent_id = target_parent_entry[0][2]
                     if target_parent_id == entry[0][2]:
                         # This is the root, so the parent is None




More information about the bazaar-commits mailing list