Merge lp:~jelmer/bzr/merge-inner-require-tree-root into lp:bzr

[Jelmer Vernooij]

Re-introduce root when merge result is empty tree.

Now that merge uses the transform functionality to fix up roots, it no longer re-adds a tree root if the target tree is empty.

This breaks when the target tree is a working tree, which can not be root-less.

This makes TreeTransform.fixup_new_roots() check that a tree root is present, and re-add the old tree root if there is none. This is consistent with the
old behaviour in merge.

[John A Meinel (jameinel)]

I think the feature flag is YAGNI, but the rest seems fine.
Tweak
John
=:->
On Jun 28, 2011 2:05 PM, "Jelmer Vernooij" <email address hidden> wrote:
> Jelmer Vernooij has proposed merging
lp:~jelmer/bzr/merge-inner-require-tree-root into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> Bug #801257 in Bazaar: "newer versions break bzr-loom"
> https://bugs.launchpad.net/bzr/+bug/801257
>
> For more details, see:
>
https://code.launchpad.net/~jelmer/bzr/merge-inner-require-tree-root/+merge/66133
>
> Now that merge uses the transform functionality to fix up roots, it no
longer re-adds a tree root if the target tree is empty.
>
> This breaks when the target tree is a working tree, which can not be
root-less.
>
> This makes TreeTransform.fixup_new_roots() check that a tree root is
present, and re-add the old tree root if there is none. This is consistent
with the
> old behaviour in merge.
> --
>
https://code.launchpad.net/~jelmer/bzr/merge-inner-require-tree-root/+merge/66133
> Your team bzr-core is requested to review the proposed merge of
lp:~jelmer/bzr/merge-inner-require-tree-root into lp:bzr.

[Aaron Bentley (abentley)]

This change is at the wrong level. A TreeTransform whose result lacks a tree root is valid, because it can be used to produce an empty PreviewTree.

Instead, it should be done in TreeTransform.apply. That code already has functionality to ensure the root directory is not deleted, which makes your "if self.final_kind(self.root) is None: self.cancel_deletion(self.root)" redundant.

review: Disapprove

[Jelmer Vernooij (jelmer)]

Hi Aaron,

On 12/07/11 19:39, Aaron Bentley wrote:
> Review: Disapprove
> This change is at the wrong level. A TreeTransform whose result lacks a tree root is valid, because it can be used to produce an empty PreviewTree.
>
> Instead, it should be done in TreeTransform.apply. That code already has functionality to ensure the root directory is not deleted, which makes your "if self.final_kind(self.root) is None: self.cancel_deletion(self.root)" redundant.
This already landed before your review, but I don't really understand
how TreeTransform.apply is better than TreeTransform.fix_tree_roots.

This particular code *is* necessary because without it the loom test
suite breaks (as discussed in Dublin). What alternative do you suggest
exactly?

Cheers,

Jelmer

[Aaron Bentley (abentley)]

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

So first off, I fixed this issue in

https://code.launchpad.net/~abentley/bzr/unbreak-transform/+merge/67872
landed as r6025

On 11-08-06 12:29 PM, Jelmer Vernooij wrote:
> Hi Aaron,
>
> On 12/07/11 19:39, Aaron Bentley wrote:
>> Review: Disapprove
>> This change is at the wrong level. A TreeTransform whose result lacks
>> a tree root is valid, because it can be used to produce an empty
>> PreviewTree.
>>
>> Instead, it should be done in TreeTransform.apply. That code already
>> has functionality to ensure the root directory is not deleted, which
>> makes your "if self.final_kind(self.root) is None:
>> self.cancel_deletion(self.root)" redundant.
> This already landed before your review, but I don't really understand
> how TreeTransform.apply is better than TreeTransform.fix_tree_roots.

The loom code is trying to change a working tree into a shape that's
illegal for working trees, but legal for PreviewTrees.
TreeTransform.apply only affects WorkingTrees, but
TreeTransformBase.fix_tree_roots affects PreviewTree generation as well.
 So it only affects the cases that need the change.

> This particular code *is* necessary because without it the loom test
> suite breaks (as discussed in Dublin).

I was saying that the code is in the wrong place. I was saying only
part of it is redundant.

> What alternative do you suggest
> exactly?

(See my merge proposal.)

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

iEYEARECAAYFAk5BPXEACgkQ0F+nu1YWqI0p0gCeOO6QidHXqIrXf2M6LgSVr0ev
kxEAn2BrqzZkLVnufwYOpLtYko8VP/Wy
=MbvA
-----END PGP SIGNATURE-----

=== modified file 'bzrlib/merge.py'
--- bzrlib/merge.py	2011-06-21 22:02:33 +0000
+++ bzrlib/merge.py	2011-06-28 13:45:49 +0000
@@ -1966,7 +1966,7 @@
     """
     if this_tree is None:
         raise errors.BzrError("bzrlib.merge.merge_inner requires a this_tree "
-                              "parameter as of bzrlib version 0.8.")
+                              "parameter")
     merger = Merger(this_branch, other_tree, base_tree, this_tree=this_tree,
                     pb=pb, change_reporter=change_reporter)
     merger.backup_files = backup_files

=== modified file 'bzrlib/tests/test_merge.py'
--- bzrlib/tests/test_merge.py	2011-06-09 18:56:55 +0000
+++ bzrlib/tests/test_merge.py	2011-06-28 13:45:49 +0000
@@ -490,6 +490,24 @@
         finally:
             tree_file.close()
 
+    def test_merge_require_tree_root(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")
+        old_root_id = tree.get_root_id()
+        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
+        conflict_count = merger.do_merge()
+        self.assertEqual(0, conflict_count)
+        self.assertEquals(set([old_root_id]), tree.all_file_ids())
+        tree.set_parent_ids([])
+
     def test_merge_add_into_deleted_root(self):
         # Yes, people actually do this.  And report bugs if it breaks.
         source = self.make_branch_and_tree('source', format='rich-root-pack')

=== modified file 'bzrlib/tests/test_transform.py'
--- bzrlib/tests/test_transform.py	2011-06-13 16:25:08 +0000
+++ bzrlib/tests/test_transform.py	2011-06-28 13:45:49 +0000
@@ -291,6 +291,21 @@
         transform.fixup_new_roots()
         self.assertNotIn(transform.root, transform._new_id)
 
+    def test_remove_root_fixup(self):
+        transform, root = self.get_transform()
+        old_root_id = self.wt.get_root_id()
+        self.assertNotEqual('new-root-id', old_root_id)
+        transform.delete_contents(root)
+        transform.unversion_file(root)
+        transform.fixup_new_roots()
+        transform.apply()
+        self.assertEqual(old_root_id, self.wt.get_root_id())
+
+        transform, root = self.get_transform()
+        new_trans_id = transform.new_directory('', ROOT_PARENT, 'new-root-id')
+        new_trans_id = transform.new_directory('', ROOT_PARENT, 'alt-root-id')
+        self.assertRaises(ValueError, transform.fixup_new_roots)
+
     def test_apply_retains_root_directory(self):
         # Do not attempt to delete the physical root directory, because that
         # is impossible.

=== modified file 'bzrlib/transform.py'
--- bzrlib/transform.py	2011-06-21 15:19:18 +0000
+++ bzrlib/transform.py	2011-06-28 13:45:49 +0000
@@ -226,10 +226,16 @@
         This means that the old root trans-id becomes obsolete, so it is
         recommended only to invoke this after the root trans-id has become
         irrelevant.
+
         """
         new_roots = [k for k, v in self._new_parent.iteritems() if v is
                      ROOT_PARENT]
         if len(new_roots) < 1:
+            if self.final_kind(self.root) is None:
+                self.cancel_deletion(self.root)
+            if self.final_file_id(self.root) is None:
+                self.version_file(self.tree_file_id(self.root),
+                                     self.root)
             return
         if len(new_roots) != 1:
             raise ValueError('A tree cannot have two roots!')

=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2011-06-28 12:18:33 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2011-06-28 13:45:49 +0000
@@ -42,6 +42,9 @@
 * ``GraphThunkIdsToKeys.merge_sort`` now properly returns
   keys rather than ids. (Jelmer Vernooij, #799677)
 
+* ``TreeTransformBase.fixup_new_roots`` can now check that a tree root
+  is present. (Jelmer Vernooij, #801257)
+
 .. Fixes for situations where bzr would previously crash or give incorrect
    or undesirable results.