Merge lp:~abentley/bzr/merge-into-empty into lp:bzr

[Aaron Bentley]

Support merge into empty tree.

This branch enables bzrlib to merge into the empty tree, i.e. the tree associated with the null revision. This tree is special because it has no root.

Such merges are not typically useful for bzr itself, but they are attempted when someone proposes a merge in Launchpad that merges a branch with commits into a branch that has no commits. (See bug #595328.) I would rather fix the root cause in bzrlib than special-case the diff generation code.

The code in TreeTransform.fixup_new_roots seemed more suitable than the code in Merger.fix_root, so I've switched to fixup_new_roots and deprecated fix_root.

There is some behavioural difference between the two. fixup_new_roots does not generate a conflict if a new root is added and the old root is not deleted. It happily merges the contents of the two roots. In such cases, the new root wins, whereas fix_root would have the old root win. Still, the new behaviour is in keeping with Merge's tendency of letting OTHER win in the case of ambiguity. And reducing duplication between fixup_new_roots and fix_root is also a win.

I've also changed Merge to ensure that root entries are processed.

I've had a bit of a revelation that TreeTransforms that delete the root aren't *invalid*, they simply can't be applied to a working tree. (They could be used to generate a _PreviewTree, for example) With that in mind, I've updated apply_removals to skip deletions of the root directory.

As a bonus, test_shelve_old_root_deleted no longer fails. I've taken the opportunity to rewrite it slightly using ExpectedException, which I find is clearer than assertRaises.

[John A Meinel (jameinel)]

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

On 6/9/2011 9:51 PM, Aaron Bentley wrote:
> Aaron Bentley has proposed merging lp:~abentley/bzr/merge-into-empty into lp:bzr with lp:~abentley/bzr/merge-into-empty-fixups as a prerequisite.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> Bug #595328 in Bazaar: "merge-proposal-jobs OOPS-1628MPJ1 in cancel_deletion when merging into empty tree"
> https://bugs.launchpad.net/bzr/+bug/595328
>
> For more details, see:
> https://code.launchpad.net/~abentley/bzr/merge-into-empty/+merge/64078
>
> This branch enables bzrlib to merge into the empty tree, i.e. the tree associated with the null revision. This tree is special because it has no root.
>
> Such merges are not typically useful for bzr itself, but they are attempted when someone proposes a merge in Launchpad that merges a branch with commits into a branch that has no commits. (See bug #595328.) I would rather fix the root cause in bzrlib than special-case the diff generation code.
>
> The code in TreeTransform.fixup_new_roots seemed more suitable than the code in Merger.fix_root, so I've switched to fixup_new_roots and deprecated fix_root.
>
> There is some behavioural difference between the two. fixup_new_roots does not generate a conflict if a new root is added and the old root is not deleted. It happily merges the contents of the two roots. In such cases, the new root wins, whereas fix_root would have the old root win. Still, the new behaviour is in keeping with Merge's tendency of letting OTHER win in the case of ambiguity. And reducing duplication between fixup_new_roots and fix_root is also a win.
>
> I've also changed Merge to ensure that root entries are processed.
>
> I've had a bit of a revelation that TreeTransforms that delete the root aren't *invalid*, they simply can't be applied to a working tree. (They could be used to generate a _PeviewTree, for example) With that in mind, I've updated apply_removals to skip deletions of the root directory.
>
> As a bonus, test_shelve_old_root_deleted no longer fails. I've taken the opportunity to rewrite it slightly using ExpectedException, which I find is clearer than assertRaises.

+ name = names[self.winner_idx[name_winner]]
+ if parent_id is not None or name is not None:
             # if we get here, name_winner and parent_winner are set to safe
             # values.
- - self.tt.adjust_path(names[self.winner_idx[name_winner]],
- - self.tt.trans_id_file_id(parent_id),
+ if parent_id is None and name is not None:
+ # if parent_id is None and name is non-None, current
file is
+ # the tree root.
+ if names[self.winner_idx[parent_id_winner]] != '':
+ raise AssertionError(
+ 'File looks like a root, but named %s' %
+ names[self.winner_idx[parent_id_winner]])

^- Why are you doing the lookup with the parent_id_winner rather than
the name_winner that you just used?

+ parent_trans_id = transform.ROOT_PARENT
+ else:
+ parent_trans_id = self.tt.tr...

review: Needs Fixing

[Vincent Ladeuil (vila)]

OK, so we're talking about brain surgery here... Thanks a lot for
tackling this one which looks indeed a bit far from your original
problem.

Great job (again), I'd like a few tweaks before landing, let's
discuss more if you disagree.

Given the area, I think it also deserves a second review but I
don't have particular hints for the other reviewer, in fact, I think
it's even better if he starts with a fresh mind (spoiler alert ;)

29 + # 'conflict'-- if there's no 'other', or it's not versioned, we
30 + # leave it alone.

[needsinfo] This comment puzzled me a bit so I dug and ended up with lp:~vila/bzr/merge-into-empty . Does the corresponding commit makes sense ?

- # it doesn't matter whether the result was 'other' or
- # 'conflict'-- if there's no 'other', or it's not versioned, we
- # leave it alone.
+ # it doesn't matter whether the result was 'other' or 'conflict'--
+ # if there's no 'other', or it's not versioned (unversioned being
+ # one such case), we leave it alone.

Additionally, do we still need:

43 + if names[self.winner_idx[parent_id_winner]] != '':
44 + raise AssertionError(
45 + 'File looks like a root, but named %s' %
46 + names[self.winner_idx[parent_id_winner]])

or do you feel better keeping it ? Hmm. I think *I*'d feel better keeping it but comments welcome.

65 + retcode = 1 if context == '.' or context == '' else 0

[needsfixing] At first I thought this wasn't really readable, but
digging a bit, I feel you're masking the fact that what really
happens there is that a conflict is created involving root. While
I realize this is out of scope for your proposal, that's not the
best way to indicate that this test is just... bad. Not only does
it rely on a side-effect: the merge with context == '' cannot
succeed at all if the previous context == '.' does not remember
the merge location but the root conflict itself deserves a better
test by itself.

In the context of this proposal, I think it's better to just get
rid of it or even better skip it and mentions bug #795456. I
started rewriting it but filed the bug instead when I realized
the implications.

78 - self.assertEqual(4, nb_conflicts)
79 + self.assertEqual(1, nb_conflicts)

Great, these conflicts were really ugly.

87 - # file4 could not be added to its original root, so it gets added to
88 - # the new root with a conflict.
89 - self.assertEqual(1, nb_conflicts)

I'm glad the conflict is gone, but can you fix the comment
instead of deleting it ? Especially since it looks like this is
as side-effect of your change that is not described in the cover
letter. As I understand it (imperfectly remembering the issues
this test evokes), file4 "should" not end up in the tree root but
instead should go into whatever the user decided to call the
directory he first merged. So not emitting a conflict here gives
no hint to the user that file4 is not where it should and that
could easily be missed in big merges (which is related to the fix
this test is associated with).

142 + def test_merge_reverse_revision_range(self):
143 + tree = self.make_branch_and_tree(".")

Oooh, here it is again :)

159 + tree.revert()
160 + se...

review: Needs Fixing

[Vincent Ladeuil (vila)]

For the record: I discovered jam's review after posting mine.

[Vincent Ladeuil (vila)]

> In the context of this proposal, I think it's better to just get
> rid of it or even better skip it and mentions bug #795456. I
> started rewriting it but filed the bug instead when I realized
> the implications.

Not that much after all but better tracked outside of this proposal IMHO, so:
https://code.launchpad.net/~vila/bzr/795456-eager-test/+merge/64164

In a nutshell: you can leave the test as-is in *this* proposal and review *that* proposal.

Adding a test that both create the conflict for the tree root and shows how to resolve it will still be a nice addition in *this* proposal.

[Aaron Bentley (abentley)]

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

On 11-06-10 06:04 AM, John A Meinel wrote:
> + if names[self.winner_idx[parent_id_winner]] != '':
> + raise AssertionError(
> + 'File looks like a root, but named %s' %
> + names[self.winner_idx[parent_id_winner]])
>
> ^- Why are you doing the lookup with the parent_id_winner rather than
> the name_winner that you just used?

The rule is that parent_id = None means "root entry" only if the
corresponding name is ''. If I use name_winner, I am not checking the
corresponding entry.

> ^- I think even though the syntax is supported by python2.6 we don't
> actually use inline if-else syntax.

Okay.

> I know it takes more lines, but *I* certainly find it easier to read.

But if you never read it, you'll never get used to it. :-)

> ^- Can you assert "0, nb_conflicts" instead of nothing?

Actually, these tests should actually be generating conflicts. They are
broken because make_outer_tree is merging from null:, and that behaviour
has changed slightly.

> @@ -310,11 +312,11 @@
> tree1.merge_from_branch(tree2.branch,
> from_revision=revision.NULL_REVISION)
> tree1.commit('Replaced root entry')
> + self.shelve_all(tree1, rev2)
> # This is essentially assertNotRaises(InconsistentDelta)
> - self.expectFailure('Cannot shelve replacing a root entry',
> - self.assertRaises, AssertionError,
> - self.assertRaises, errors.InconsistentDelta,
> - self.shelve_all, tree1, rev2)
> + with ExpectedException(AssertionError, ''):
> + with ExpectedException(errors.InconsistentDelta, ''):
> + self.shelve_all(tree1, rev2)
>
> ^- If 'self.shelve_all(tree1, rev2)' now passes, why would we have
> ExpectedException at all?

That first shelve_all was test code, and I shouldn't have committed it.
 I've fixed it.

> It seems like this is artificial and should
> just be removed completely. (It is an InconsistentDelta because you've
> already shelved the changes.)

People (e.g. vila, about test_apply_retains_root_directory) often
complain when a test has no assertions. Here we're asserting that
shelve_all will not raise an InconsistentDelta.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk3yY/YACgkQ0F+nu1YWqI3PJwCggPX6kA62ZRys1Xq2+IpTzTBc
X/EAn2uxtbmyHhyUTkq9Y4cSpua8aHWC
=miel
-----END PGP SIGNATURE-----

[Aaron Bentley (abentley)]

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

On 11-06-10 06:56 AM, Vincent Ladeuil wrote:
> 29 + # 'conflict'-- if there's no 'other', or it's not versioned, we
> 30 + # leave it alone.

Per our IRC discussion, I've changed this to:
            # it doesn't matter whether the result was 'other' or
            # 'conflict'-- if it has no file id, we leave it alone.

> Additionally, do we still need:
>
> 43 + if names[self.winner_idx[parent_id_winner]] != '':
> 44 + raise AssertionError(
> 45 + 'File looks like a root, but named %s' %
> 46 + names[self.winner_idx[parent_id_winner]])
>
> or do you feel better keeping it ? Hmm. I think *I*'d feel better keeping it but comments welcome.

I feel better keeping it. If we get a file with no parent_id and
accidentally treat it as a root entry, the errors we get will be really
confusing.

> In the context of this proposal, I think it's better to just get
> rid of it or even better skip it and mentions bug #795456. I
> started rewriting it but filed the bug instead when I realized
> the implications.

Okay.

>
> 78 - self.assertEqual(4, nb_conflicts)
> 79 + self.assertEqual(1, nb_conflicts)
>
> Great, these conflicts were really ugly.
>
> 87 - # file4 could not be added to its original root, so it gets added to
> 88 - # the new root with a conflict.
> 89 - self.assertEqual(1, nb_conflicts)
>
> I'm glad the conflict is gone, but can you fix the comment
> instead of deleting it ? Especially since it looks like this is
> as side-effect of your change that is not described in the cover
> letter. As I understand it (imperfectly remembering the issues
> this test evokes), file4 "should" not end up in the tree root but
> instead should go into whatever the user decided to call the
> directory he first merged. So not emitting a conflict here gives
> no hint to the user that file4 is not where it should and that
> could easily be missed in big merges (which is related to the fix
> this test is associated with).

As discussed on IRC, my changes actually broke these tests, which were
testing something different. We'll have to chat further about the right
way to fix them.

> How about:
> - # This is essentially assertNotRaises(InconsistentDelta)
> - with ExpectedException(AssertionError, ''):
> + with ExpectedException(AssertionError, 'InconsistentDelta not raised'):
> + # We expect errors.InconsistentDelta to not be raised.
> + # ExpectedException will raise an AssertionError that we'll catch
> + # in the enclosing 'with'

Too long-winded, but I've added 'InconsistentDelta not raised'.

> 202 + def test_apply_retains_root_directory(self):
> 203 + # Do not attempt to delete the physical root directory, because that
> 204 + # is impossible.
> 205 + transform, root = self.get_transform()
> 206 + with transform:
> 207 + transform.delete_contents(root)
> 208 + transform.apply()
>
> No checks here ? This is... surprising. Can you either add one

Done. It asserts that TransformRenameFailed is not raised.

> 221 - self.version_file(file_id, self._new_root)
> 222 + if file_id is not None:
> 223 + self.version_file(file_id, self._new_root)
>
> If I get rid o...

[John A Meinel (jameinel)]

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

On 06/10/2011 08:37 PM, Aaron Bentley wrote:
...

...

> That first shelve_all was test code, and I shouldn't have committed it.
> I've fixed it.
>
>> It seems like this is artificial and should
>> just be removed completely. (It is an InconsistentDelta because you've
>> already shelved the changes.)
>
> People (e.g. vila, about test_apply_retains_root_directory) often
> complain when a test has no assertions. Here we're asserting that
> shelve_all will not raise an InconsistentDelta.
>
> Aaron

2 double negatives isn't a great way to assert a positive. A simple
comment is probably much more obvious.

# If something is broken, this will raise InconsistentDelta
self.shelve_all(tree1, rev2)

I agree that 'with ExpectedException' is a clearer way of *doing* the
double negative, but it is still a really ugly way of asserting that
something doesn't raise an exception.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk3yp38ACgkQJdeBCYSNAAOSkwCfbngIikjAXdVm2cQtWEfNc/CD
JPcAoMyCwcFHTSdZFP1CdwfBIMSPWoEx
=Zdgd
-----END PGP SIGNATURE-----

[Vincent Ladeuil (vila)]

review: Approve

=== modified file 'bzrlib/merge.py'
--- bzrlib/merge.py	2011-05-21 16:43:19 +0000
+++ bzrlib/merge.py	2011-06-13 15:41:42 +0000
@@ -868,7 +868,7 @@
                     executable3, file_status, resolver=resolver)
         finally:
             child_pb.finished()
-        self.fix_root()
+        self.tt.fixup_new_roots()
         self._finish_computing_transform()
 
     def _finish_computing_transform(self):
@@ -1093,6 +1093,7 @@
                           ))
         return result
 
+    @deprecated_method(deprecated_in((2, 4, 0)))
     def fix_root(self):
         if self.tt.final_kind(self.tt.root) is None:
             self.tt.cancel_deletion(self.tt.root)
@@ -1316,16 +1317,26 @@
             self._raw_conflicts.append(('path conflict', trans_id, file_id,
                                         this_parent, this_name,
                                         other_parent, other_name))
-        if other_name is None:
+        if not self.other_tree.has_id(file_id):
             # it doesn't matter whether the result was 'other' or
-            # 'conflict'-- if there's no 'other', we leave it alone.
+            # 'conflict'-- if it has no file id, we leave it alone.
             return
         parent_id = parents[self.winner_idx[parent_id_winner]]
-        if parent_id is not None:
+        name = names[self.winner_idx[name_winner]]
+        if parent_id is not None or name is not None:
             # if we get here, name_winner and parent_winner are set to safe
             # values.
-            self.tt.adjust_path(names[self.winner_idx[name_winner]],
-                                self.tt.trans_id_file_id(parent_id),
+            if parent_id is None and name is not None:
+                # if parent_id is None and name is non-None, current file is
+                # the tree root.
+                if names[self.winner_idx[parent_id_winner]] != '':
+                    raise AssertionError(
+                        'File looks like a root, but named %s' %
+                        names[self.winner_idx[parent_id_winner]])
+                parent_trans_id = transform.ROOT_PARENT
+            else:
+                parent_trans_id = self.tt.trans_id_file_id(parent_id)
+            self.tt.adjust_path(name, parent_trans_id,
                                 self.tt.trans_id_file_id(file_id))
 
     def _do_merge_contents(self, file_id):

=== modified file 'bzrlib/tests/blackbox/test_merge.py'
--- bzrlib/tests/blackbox/test_merge.py	2011-05-27 09:45:44 +0000
+++ bzrlib/tests/blackbox/test_merge.py	2011-06-13 15:41:42 +0000
@@ -668,8 +668,9 @@
             self.build_tree([f])
             tree.add(f)
             tree.commit("added "+f)
-        for context in (".", "", "a"):
-            self.run_bzr("merge -r 1..0 " + context)
+        for context in (".", "", 'a'):
+            retcode = 1 if context == '.' or context == '' else 0
+            self.run_bzr("merge -r 1..0 " + context, retcode=retcode)
             self.assertPathDoesNotExist("a")
             tree.revert()
             self.assertPathExists("a")

=== modified file 'bzrlib/tests/per_workingtree/test_merge_from_branch.py'
--- bzrlib/tests/per_workingtree/test_merge_from_branch.py	2011-04-13 14:33:56 +0000
+++ bzrlib/tests/per_workingtree/test_merge_from_branch.py	2011-06-13 15:41:42 +0000
@@ -162,6 +162,8 @@
         outer.commit('added foo')
         inner = self.make_inner_branch()
         outer.merge_from_branch(inner, to_revision='1', from_revision='null:')
+        #retain original root id.
+        outer.set_root_id(outer.basis_tree().get_root_id())
         outer.commit('merge inner branch')
         outer.mkdir('dir-outer', 'dir-outer-id')
         outer.move(['dir', 'file3'], to_dir='dir-outer')

=== modified file 'bzrlib/tests/test_merge.py'
--- bzrlib/tests/test_merge.py	2011-05-26 08:05:45 +0000
+++ bzrlib/tests/test_merge.py	2011-06-13 15:41:42 +0000
@@ -121,6 +121,20 @@
         finally:
             wt1.unlock()
 
+    def test_merge_into_null_tree(self):
+        wt = self.make_branch_and_tree('tree')
+        null_tree = wt.basis_tree()
+        self.build_tree(['tree/file'])
+        wt.add('file')
+        wt.commit('tree with root')
+        merger = _mod_merge.Merge3Merger(null_tree, null_tree, null_tree, wt,
+                                         this_branch=wt.branch,
+                                         do_merge=False)
+        with merger.make_preview_transform() as tt:
+            self.assertEqual([], tt.find_conflicts())
+            preview = tt.get_preview_tree()
+            self.assertEqual(wt.get_root_id(), preview.get_root_id())
+
     def test_create_rename(self):
         """Rename an inventory entry while creating the file"""
         tree =self.make_branch_and_tree('.')
@@ -387,6 +401,26 @@
                              '>>>>>>> MERGE-SOURCE\n',
                              'this/file')
 
+    def test_merge_reverse_revision_range(self):
+        tree = self.make_branch_and_tree(".")
+        tree.lock_write()
+        self.addCleanup(tree.unlock)
+        self.build_tree(['a'])
+        tree.add('a')
+        tree.commit("added a")
+        first_rev = tree.branch.revision_history()[0]
+        merger = _mod_merge.Merger.from_revision_ids(None, tree,
+                                          _mod_revision.NULL_REVISION,
+                                          first_rev)
+        merger.merge_type = _mod_merge.Merge3Merger
+        merger.interesting_files = 'a'
+        conflict_count = merger.do_merge()
+        self.assertEqual(0, conflict_count)
+
+        self.assertPathDoesNotExist("a")
+        tree.revert()
+        self.assertPathExists("a")
+
     def test_make_merger(self):
         this_tree = self.make_branch_and_tree('this')
         this_tree.commit('rev1', rev_id='rev1')

=== modified file 'bzrlib/tests/test_shelf.py'
--- bzrlib/tests/test_shelf.py	2011-05-13 12:51:05 +0000
+++ bzrlib/tests/test_shelf.py	2011-06-13 15:41:42 +0000
@@ -568,19 +568,18 @@
         list(creator.iter_shelvable())
         creator.shelve_deletion('foo-id')
         creator.shelve_deletion('bar-id')
-        shelf_file = open('shelf', 'w+b')
-        self.addCleanup(shelf_file.close)
-        creator.write_shelf(shelf_file)
-        creator.transform()
-        creator.finalize()
+        with open('shelf', 'w+b') as shelf_file:
+            creator.write_shelf(shelf_file)
+            creator.transform()
+            creator.finalize()
         # validate the test setup
         self.assertTrue('foo-id' in tree)
         self.assertTrue('bar-id' in tree)
         self.assertFileEqual('baz', 'tree/foo/bar')
-        shelf_file.seek(0)
-        unshelver = shelf.Unshelver.from_tree_and_shelf(tree, shelf_file)
-        self.addCleanup(unshelver.finalize)
-        unshelver.make_merger().do_merge()
+        with open('shelf', 'r+b') as shelf_file:
+            unshelver = shelf.Unshelver.from_tree_and_shelf(tree, shelf_file)
+            self.addCleanup(unshelver.finalize)
+            unshelver.make_merger().do_merge()
         self.assertFalse('foo-id' in tree)
         self.assertFalse('bar-id' in tree)
 

=== modified file 'bzrlib/tests/test_shelf_ui.py'
--- bzrlib/tests/test_shelf_ui.py	2011-06-09 14:40:22 +0000
+++ bzrlib/tests/test_shelf_ui.py	2011-06-13 15:41:42 +0000
@@ -311,10 +311,15 @@
                                 from_revision=revision.NULL_REVISION)
         tree1.commit('Replaced root entry')
         # This is essentially assertNotRaises(InconsistentDelta)
-        self.expectFailure('Cannot shelve replacing a root entry',
-                           self.assertRaises, AssertionError,
-                           self.assertRaises, errors.InconsistentDelta,
-                           self.shelve_all, tree1, rev2)
+        # With testtools 0.99, it can be rewritten as:
+        # with ExpectedException(AssertionError,
+        #                        'InconsistentDelta not raised'):
+        #     with ExpectedException(errors.InconsistentDelta, ''):
+        #         self.shelve_all(tree1, rev2)
+        e = self.assertRaises(AssertionError, self.assertRaises,
+                              errors.InconsistentDelta, self.shelve_all, tree1,
+                              rev2)
+        self.assertContainsRe('InconsistentDelta not raised', str(e))
 
     def test_shelve_split(self):
         outer_tree = self.make_branch_and_tree('outer')

=== modified file 'bzrlib/tests/test_transform.py'
--- bzrlib/tests/test_transform.py	2011-06-10 16:19:57 +0000
+++ bzrlib/tests/test_transform.py	2011-06-13 15:41:42 +0000
@@ -285,6 +285,23 @@
         new_trans_id = transform.new_directory('', ROOT_PARENT, 'alt-root-id')
         self.assertRaises(ValueError, transform.fixup_new_roots)
 
+    def test_add_unversioned_root(self):
+        transform, root = self.get_transform()
+        new_trans_id = transform.new_directory('', ROOT_PARENT, None)
+        transform.fixup_new_roots()
+        self.assertNotIn(transform.root, transform._new_id)
+
+    def test_apply_retains_root_directory(self):
+        # Do not attempt to delete the physical root directory, because that
+        # is impossible.
+        transform, root = self.get_transform()
+        with transform:
+            transform.delete_contents(root)
+            e = self.assertRaises(AssertionError, self.assertRaises,
+                                  errors.TransformRenameFailed,
+                                  transform.apply)
+        self.assertContainsRe('TransformRenameFailed not raised', str(e))
+
     def test_hardlink(self):
         self.requireFeature(HardlinkFeature)
         transform, root = self.get_transform()

=== modified file 'bzrlib/transform.py'
--- bzrlib/transform.py	2011-06-10 16:19:57 +0000
+++ bzrlib/transform.py	2011-06-13 15:41:42 +0000
@@ -252,7 +252,8 @@
         if (self.tree_file_id(self._new_root) is not None and
             self._new_root not in self._removed_id):
             self.unversion_file(self._new_root)
-        self.version_file(file_id, self._new_root)
+        if file_id is not None:
+            self.version_file(file_id, self._new_root)
 
         # Now move children of new root into old root directory.
         # Ensure all children are registered with the transaction, but don't
@@ -1826,8 +1827,10 @@
         tree_paths.sort(reverse=True)
         child_pb = ui.ui_factory.nested_progress_bar()
         try:
-            for num, data in enumerate(tree_paths):
-                path, trans_id = data
+            for num, (path, trans_id) in enumerate(tree_paths):
+                # do not attempt to move root into a subdirectory of itself.
+                if path == '':
+                    continue
                 child_pb.update('removing file', num, len(tree_paths))
                 full_path = self._tree.abspath(path)
                 if trans_id in self._removed_contents:

=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2011-06-09 11:59:08 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2011-06-13 15:41:42 +0000
@@ -73,6 +73,8 @@
   ``UIFactory.get_password`` and ``UIFactory.get_boolean`` now require a
   unicode prompt to be passed in. (Jelmer Vernooij, #592083)
 
+* Support merging into the empty tree. (Aaron Bentley, #595328)
+
 Documentation
 *************