CommitBuilder tests fail with nested trees

[Aaron Bentley]

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

Jelmer Vernooij wrote:
> I think it would make sense to require that
> CommitBuilder.record_entry_contents() always gets called for the root
> node before CommitBuilder.finish_inventory() gets called.

Okay, great.


> +        # FIXME: Should this really be an assert or rather a
> +        # more user friendly exception ?
> +        assert self.new_inventory.root != None

This is really for API users, so I think it's fine as an assert.  Making
it nicer wouldn't hurt.

For this test, the convention is to do an identity test, i.e.:
        assert self.new_inventory.root is not None

> +    def set_root_id(self, root_id):
> +        """Convenience function for setting the file id of the root node. 
> +
> +        :param root_id: The new file_id of the root node.
> +        """
> +        self.new_inventory.add_path('', 'directory', root_id)

Yes, abstracting this out is good.  (For now-- long-term good would be
adding root entries to the inventory the same way as normal entries)

> @@ -2017,7 +2027,11 @@
>          :param tree: The tree which contains this entry and should be used to 
>          obtain content.
>          """
> -        self.new_inventory.add(ie)
> +        if path == '':
> +            self.set_root_id(ie.file_id)
> +            return
> +        else:
> +            self.new_inventory.add(ie)

Nice, again.

>  
>          # ie.revision is always None if the InventoryEntry is considered
>          # for committing. ie.snapshot will record the correct revision 
> 
> === modified file bzrlib/tests/repository_implementations/test_commit_builder.p
> ... y
> --- bzrlib/tests/repository_implementations/test_commit_builder.py
> +++ bzrlib/tests/repository_implementations/test_commit_builder.py
> @@ -28,14 +28,23 @@
>          builder = tree.branch.get_commit_builder([])
>          self.assertIsInstance(builder, CommitBuilder)
>  
> +    def test_set_root_id(self):
> +        tree = self.make_branch_and_tree(".")
> +        builder = tree.branch.get_commit_builder([])
> +        self.assertEqual(None, builder.new_inventory.root)

Again, identity: self.assertIs(None, builder.new_inventory.root)

>      def test_finish_inventory(self):
>          tree = self.make_branch_and_tree(".")
>          builder = tree.branch.get_commit_builder([])
> +        builder.set_root_id(None)
>          builder.finish_inventory()

It might be nice to make None the default, and just do
builder.set_root_id().  The rationale being that you're not actually
setting it to None.

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

iD8DBQFEkd8j0F+nu1YWqI0RApXdAKCC9r5lR9fkKxlOeVX2/6MLO3whEACeOHLg
urOvbfLVtlrFSK4LA53nDjg=
=P2KE
-----END PGP SIGNATURE-----