Merge lp:~jameinel/bzr/2.0.4-dirstate-set-root-504390 into lp:bzr/2.0

[John A Meinel]

[John A Meinel (jameinel)]

This is a follow up to my earlier patch for handling root-id changes in 'bzr revert'.

I don't quite understand why "merge.merge_inner()" doesn't handle setting the root id, but WT.revert() *does*. But for now, I'll live with it, and set the root id where the existing code was doing it.

The primary difference for pull and update is that we had lines saying "if basis.inventory.root is None: set_root_id()", and this changes those lines to use "if basis.inventory.root != new_root: set_root_id()".

This exposed a bug in the dirstate logic. It turned out that "set_path_id()" was doing:
- if self._id_index is not None:
- self._id_index.setdefault(new_id, set()).add(entry[0])

Which looked correct at first (and second) glances. However, it turns out that 'entry' at that point is the *old* entry. ('', '', 'TREE_ROOT') not the new entry ('', '', 'second-file-id').

What that meant was that when we did the _id_index lookup, we might get back an entry key whose file-id *doesn't* match the actual file-id. And depending on the ordering of iter(set()) we would see that the entry that we thought matched our key was marked absent in the corresponding tree.

update_minimal() had already sorted out what needed to be done to the _id_index, so the line was both superfluous and wrong (I didn't need to change it to the new entry).

The net effect is that with this patch, 'bzr pull', 'bzr revert', and 'bzr update' with a root-id change will all leave the tree with the correct root-id.

[Robert Collins (lifeless)]

 review: +1

might be nice to do a _validate on the tree objects in the per-tree
test, just as an extra check that its all ok [only for these changed/new
tests].

-Rob

review: Approve

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Robert Collins wrote:
> Review: Approve
> review: +1
>
> might be nice to do a _validate on the tree objects in the per-tree
> test, just as an extra check that its all ok [only for these changed/new
> tests].
>
> -Rob
>

Done. I also added a quick integrity check of self._id_index, since that
would have also caught the bug.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAktM+a4ACgkQJdeBCYSNAAOQJgCgkOratSV741fn05kvisDdNxNO
sM8An3S9U4N6rv1S0CoyLTQ+RybU+Y7y
=XBQG
-----END PGP SIGNATURE-----

=== modified file 'NEWS'
--- NEWS	2010-01-12 07:02:03 +0000
+++ NEWS	2010-01-12 22:38:12 +0000
@@ -57,9 +57,8 @@
 * ``TreeTransform`` can now handle when a delta says that the file id for
   the tree root changes. Rather than trying to rename your working
   directory, or failing early saying that you can't have multiple
-  tree roots. This also fixes ``bzr revert`` when the root id changes.
-  (Currently ``bzr update`` is still broken.)
-  (John Arbash Meinel, #494269)
+  tree roots. This also fixes revert, update, and pull when the root id
+  changes.  (John Arbash Meinel, #494269, #504390)
 
 * ``_update_current_block`` no longer suppresses exceptions, so ^C at just
   the right time will get propagated, rather than silently failing to move

=== modified file 'bzrlib/dirstate.py'
--- bzrlib/dirstate.py	2009-10-01 00:56:56 +0000
+++ bzrlib/dirstate.py	2010-01-12 22:38:11 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006, 2007, 2008 Canonical Ltd
+# Copyright (C) 2006-2010 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -1997,6 +1997,8 @@
                 entry_index, present = self._find_entry_index(key, block)
                 if present:
                     entry = self._dirblocks[block_index][1][entry_index]
+                    # TODO: We might want to assert that entry[0][2] ==
+                    #       fileid_utf8.
                     if entry[1][tree_index][0] in 'fdlt':
                         # this is the result we are looking for: the
                         # real home of this file_id in this tree.
@@ -2354,8 +2356,6 @@
         self.update_minimal(('', '', new_id), 'd',
             path_utf8='', packed_stat=entry[1][0][4])
         self._dirblock_state = DirState.IN_MEMORY_MODIFIED
-        if self._id_index is not None:
-            self._id_index.setdefault(new_id, set()).add(entry[0])
 
     def set_parent_trees(self, trees, ghosts):
         """Set the parent trees for the dirstate.
@@ -3013,6 +3013,13 @@
             if absent_positions == tree_count:
                 raise AssertionError(
                     "entry %r has no data for any tree." % (entry,))
+        if self._id_index is not None:
+            for file_id, entry_keys in self._id_index.iteritems():
+                for entry_key in entry_keys:
+                    if entry_key[2] != file_id:
+                        raise AssertionError(
+                            'file_id %r did not match entry key %s'
+                            % (file_id, entry_key))
 
     def _wipe_state(self):
         """Forget all state information about the dirstate."""

=== modified file 'bzrlib/tests/per_workingtree/test_pull.py'
--- bzrlib/tests/per_workingtree/test_pull.py	2009-07-10 07:14:02 +0000
+++ bzrlib/tests/per_workingtree/test_pull.py	2010-01-12 22:38:12 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006 Canonical Ltd
+# Copyright (C) 2006, 2007, 2009, 2010 Canonical Ltd
 # Authors:  Robert Collins <[e-mail]>
 #
 # This program is free software; you can redistribute it and/or modify
@@ -56,3 +56,15 @@
         tree_b.pull(tree_a.branch)
         self.assertFileEqual('contents of from/file\n', 'to/file')
 
+    def test_pull_changes_root_id(self):
+        tree = self.make_branch_and_tree('from')
+        tree.set_root_id('first_root_id')
+        self.build_tree(['from/file'])
+        tree.add(['file'])
+        tree.commit('first')
+        to_tree = tree.bzrdir.sprout('to').open_workingtree()
+        self.assertEqual('first_root_id', to_tree.get_root_id())
+        tree.set_root_id('second_root_id')
+        tree.commit('second')
+        to_tree.pull(tree.branch)
+        self.assertEqual('second_root_id', to_tree.get_root_id())

=== modified file 'bzrlib/tests/per_workingtree/test_set_root_id.py'
--- bzrlib/tests/per_workingtree/test_set_root_id.py	2009-09-10 06:32:42 +0000
+++ bzrlib/tests/per_workingtree/test_set_root_id.py	2010-01-12 22:38:12 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006 Canonical Ltd
+# Copyright (C) 2006-2010 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -16,7 +16,7 @@
 
 """Tests for WorkingTree.set_root_id"""
 
-from bzrlib import inventory
+from bzrlib import errors, inventory
 from bzrlib.tests.per_workingtree import TestCaseWithWorkingTree
 
 
@@ -48,3 +48,19 @@
         # should still be retained
         tree = tree.bzrdir.open_workingtree()
         self.assertEqual(root_id, tree.get_root_id())
+        tree._validate()
+
+    def test_set_root_id(self):
+        tree = self.make_branch_and_tree('.')
+        tree.lock_write()
+        self.addCleanup(tree.unlock)
+        orig_root_id = tree.get_root_id()
+        self.assertNotEqual('custom-root-id', orig_root_id)
+        self.assertEqual('', tree.id2path(orig_root_id))
+        self.assertRaises(errors.NoSuchId, tree.id2path, 'custom-root-id')
+        tree.set_root_id('custom-root-id')
+        self.assertEqual('custom-root-id', tree.get_root_id())
+        self.assertEqual('custom-root-id', tree.path2id(''))
+        self.assertEqual('', tree.id2path('custom-root-id'))
+        self.assertRaises(errors.NoSuchId, tree.id2path, orig_root_id)
+        tree._validate()

=== modified file 'bzrlib/tests/per_workingtree/test_workingtree.py'
--- bzrlib/tests/per_workingtree/test_workingtree.py	2009-08-20 04:09:58 +0000
+++ bzrlib/tests/per_workingtree/test_workingtree.py	2010-01-12 22:38:11 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005, 2006, 2007 Canonical Ltd
+# Copyright (C) 2006, 2007, 2008, 2009, 2010 Canonical Ltd
 # Authors:  Robert Collins <[e-mail]>
 #           and others
 #
@@ -478,6 +478,20 @@
         self.assertEqual(wt.get_root_id(), checkout.get_root_id())
         self.assertNotEqual(None, wt.get_root_id())
 
+    def test_update_sets_updated_root_id(self):
+        wt = self.make_branch_and_tree('tree')
+        wt.set_root_id('first_root_id')
+        self.assertEqual('first_root_id', wt.get_root_id())
+        self.build_tree(['tree/file'])
+        wt.add(['file'])
+        wt.commit('first')
+        co = wt.branch.create_checkout('checkout')
+        wt.set_root_id('second_root_id')
+        wt.commit('second')
+        self.assertEqual('second_root_id', wt.get_root_id())
+        self.assertEqual(0, co.update())
+        self.assertEqual('second_root_id', co.get_root_id())
+
     def test_update_returns_conflict_count(self):
         # working tree formats from the meta-dir format and newer support
         # setting the last revision on a tree independently of that on the

=== modified file 'bzrlib/tests/test_dirstate.py'
--- bzrlib/tests/test_dirstate.py	2009-08-17 03:33:56 +0000
+++ bzrlib/tests/test_dirstate.py	2010-01-12 22:38:11 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006, 2007 Canonical Ltd
+# Copyright (C) 2006-2010 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -873,15 +873,23 @@
         state = dirstate.DirState.initialize('dirstate')
         try:
             # check precondition to be sure the state does change appropriately.
-            self.assertEqual(
-                [(('', '', 'TREE_ROOT'), [('d', '', 0, False,
-                   'xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx')])],
-                list(state._iter_entries()))
-            state.set_path_id('', 'foobarbaz')
-            expected_rows = [
-                (('', '', 'foobarbaz'), [('d', '', 0, False,
-                   'xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx')])]
+            root_entry = (('', '', 'TREE_ROOT'), [('d', '', 0, False, 'x'*32)])
+            self.assertEqual([root_entry], list(state._iter_entries()))
+            self.assertEqual(root_entry, state._get_entry(0, path_utf8=''))
+            self.assertEqual(root_entry,
+                             state._get_entry(0, fileid_utf8='TREE_ROOT'))
+            self.assertEqual((None, None),
+                             state._get_entry(0, fileid_utf8='second-root-id'))
+            state.set_path_id('', 'second-root-id')
+            new_root_entry = (('', '', 'second-root-id'),
+                              [('d', '', 0, False, 'x'*32)])
+            expected_rows = [new_root_entry]
             self.assertEqual(expected_rows, list(state._iter_entries()))
+            self.assertEqual(new_root_entry, state._get_entry(0, path_utf8=''))
+            self.assertEqual(new_root_entry, 
+                             state._get_entry(0, fileid_utf8='second-root-id'))
+            self.assertEqual((None, None),
+                             state._get_entry(0, fileid_utf8='TREE_ROOT'))
             # should work across save too
             state.save()
         finally:
@@ -905,21 +913,36 @@
         state._validate()
         try:
             state.set_parent_trees([('parent-revid', rt)], ghosts=[])
-            state.set_path_id('', 'foobarbaz')
+            root_entry = (('', '', 'TREE_ROOT'),
+                          [('d', '', 0, False, 'x'*32),
+                           ('d', '', 0, False, 'parent-revid')])
+            self.assertEqual(root_entry, state._get_entry(0, path_utf8=''))
+            self.assertEqual(root_entry,
+                             state._get_entry(0, fileid_utf8='TREE_ROOT'))
+            self.assertEqual((None, None),
+                             state._get_entry(0, fileid_utf8='Asecond-root-id'))
+            state.set_path_id('', 'Asecond-root-id')
             state._validate()
             # now see that it is what we expected
-            expected_rows = [
-                (('', '', 'TREE_ROOT'),
-                    [('a', '', 0, False, ''),
-                     ('d', '', 0, False, 'parent-revid'),
-                     ]),
-                (('', '', 'foobarbaz'),
-                    [('d', '', 0, False, ''),
-                     ('a', '', 0, False, ''),
-                     ]),
-                ]
+            old_root_entry = (('', '', 'TREE_ROOT'),
+                              [('a', '', 0, False, ''),
+                               ('d', '', 0, False, 'parent-revid')])
+            new_root_entry = (('', '', 'Asecond-root-id'),
+                              [('d', '', 0, False, ''),
+                               ('a', '', 0, False, '')])
+            expected_rows = [new_root_entry, old_root_entry]
             state._validate()
             self.assertEqual(expected_rows, list(state._iter_entries()))
+            self.assertEqual(new_root_entry, state._get_entry(0, path_utf8=''))
+            self.assertEqual(old_root_entry, state._get_entry(1, path_utf8=''))
+            self.assertEqual((None, None),
+                             state._get_entry(0, fileid_utf8='TREE_ROOT'))
+            self.assertEqual(old_root_entry,
+                             state._get_entry(1, fileid_utf8='TREE_ROOT'))
+            self.assertEqual(new_root_entry,
+                             state._get_entry(0, fileid_utf8='Asecond-root-id'))
+            self.assertEqual((None, None),
+                             state._get_entry(1, fileid_utf8='Asecond-root-id'))
             # should work across save too
             state.save()
         finally:

=== modified file 'bzrlib/tests/test_shelf.py'
--- bzrlib/tests/test_shelf.py	2009-09-10 06:32:42 +0000
+++ bzrlib/tests/test_shelf.py	2010-01-12 22:38:12 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2008 Canonical Ltd
+# Copyright (C) 2008, 2009, 2010 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -108,6 +108,29 @@
         creator.shelve_change(('rename', 'baz-id', 'foo/baz', 'bar/baz'))
         self.check_shelve_move(creator, tree)
 
+    def test_shelve_changed_root_id(self):
+        tree = self.make_branch_and_tree('.')
+        self.build_tree(['foo'])
+        tree.set_root_id('first-root-id')
+        tree.add(['foo'], ['foo-id'])
+        tree.commit('foo')
+        tree.set_root_id('second-root-id')
+        tree.lock_tree_write()
+        self.addCleanup(tree.unlock)
+        creator = shelf.ShelfCreator(tree, tree.basis_tree())
+        self.addCleanup(creator.finalize)
+        self.expectFailure('shelf doesn\'t support shelving root changes yet',
+            self.assertEqual, [
+                ('delete file', 'first-root-id', 'directory', ''),
+                ('add file', 'second-root-id', 'directory', ''),
+                ('rename', 'foo-id', u'foo', u'foo'),
+                ], list(creator.iter_shelvable()))
+
+        self.assertEqual([('delete file', 'first-root-id', 'directory', ''),
+                          ('add file', 'second-root-id', 'directory', ''),
+                          ('rename', 'foo-id', u'foo', u'foo'),
+                         ], list(creator.iter_shelvable()))
+
     def assertShelvedFileEqual(self, expected_content, creator, file_id):
         s_trans_id = creator.shelf_transform.trans_id_file_id(file_id)
         shelf_file = creator.shelf_transform._limbo_name(s_trans_id)

=== modified file 'bzrlib/tests/test_switch.py'
--- bzrlib/tests/test_switch.py	2009-05-07 05:08:46 +0000
+++ bzrlib/tests/test_switch.py	2010-01-12 22:38:12 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2007 Canonical Ltd
+# Copyright (C) 2007-2010 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -100,6 +100,18 @@
         self.assertContainsRe(str(err),
             "Pending merges must be committed or reverted before using switch")
 
+    def test_switch_changing_root_id(self):
+        tree = self._setup_tree()
+        tree2 = self.make_branch_and_tree('tree-2')
+        tree2.set_root_id('custom-root-id')
+        self.build_tree(['tree-2/file-2'])
+        tree2.add(['file-2'])
+        tree2.commit('rev1b')
+        checkout = tree.branch.create_checkout('checkout',
+            lightweight=self.lightweight)
+        switch.switch(checkout.bzrdir, tree2.branch)
+        self.assertEqual('custom-root-id', tree2.get_root_id())
+
 
 class TestSwitchHeavyweight(TestSwitch):
 

=== modified file 'bzrlib/workingtree.py'
--- bzrlib/workingtree.py	2009-08-26 05:38:16 +0000
+++ bzrlib/workingtree.py	2010-01-12 22:38:12 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005, 2006, 2007, 2008, 2009 Canonical Ltd
+# Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -1624,9 +1624,10 @@
                                 this_tree=self,
                                 pb=pb,
                                 change_reporter=change_reporter)
-                    if (basis_tree.inventory.root is None and
-                        new_basis_tree.inventory.root is not None):
-                        self.set_root_id(new_basis_tree.get_root_id())
+                    basis_root_id = basis_tree.get_root_id()
+                    new_root_id = new_basis_tree.get_root_id()
+                    if basis_root_id != new_root_id:
+                        self.set_root_id(new_root_id)
                 finally:
                     pb.finished()
                     basis_tree.unlock()
@@ -2245,8 +2246,10 @@
             basis.lock_read()
             try:
                 to_tree = self.branch.basis_tree()
-                if basis.inventory.root is None:
-                    self.set_root_id(to_tree.get_root_id())
+                to_root_id = to_tree.get_root_id()
+                if (basis.inventory.root is None
+                    or basis.inventory.root.file_id != to_root_id):
+                    self.set_root_id(to_root_id)
                     self.flush()
                 result += merge.merge_inner(
                                       self.branch,