[MERGE] BranchBuilder updates

[Martin Pool]

Martin Pool has voted tweak.
Status is now: Conditionally approved
Comment:
It looks like a nice api.

Could you please also add a paragraph or two to the 'testing bazaar'
section of the developer guide to point people towards this.  At the
moment there is a weight of existing code telling them to do it the hard
way.

=== modified file 'bzrlib/branchbuilder.py'
--- bzrlib/branchbuilder.py     2007-04-27 06:16:29 +0000
+++ bzrlib/branchbuilder.py     2008-07-22 20:40:34 +0000
@@ -47,8 +47,11 @@
              transport.mkdir('.')
          if format is None:
              format = 'default'
+        if isinstance(format, str):
+            format = bzrdir.format_registry.make_bzrdir(format)
          self._branch = 
bzrdir.BzrDir.create_branch_convenience(transport.base,
-            format=bzrdir.format_registry.make_bzrdir(format))
+            format=format, force_new_tree=False)
+        self._tree = None


If you intend to change the api to allow passing a BzrDirFormat object
(which seems reasonable) you should change the docstring to say so.

I would like an :ivar: about _tree saying what it is, what it contains,
when it's locked, etc.  (even though it's semi-private)


+    def _move_branch_pointer(self, new_revision_id):
+        """Point self._branch to a different revision id."""
+        self._branch.lock_write()
+        try:
+            # We don't seem to have a simple set_last_revision(), so we
+            # implement it here.
+            cur_revno, cur_revision_id = 
self._branch.last_revision_info()
+            g = self._branch.repository.get_graph()
+            new_revno = g.find_distance_to_null(new_revision_id,
+                                                [(cur_revision_id, 
cur_revno)])
+            self._branch.set_last_revision_info(new_revno, 
new_revision_id)

Maybe we should have such a method, or allow the revno to be given as 
None
to set_last_revision_info?  I guess that is out of scope for your 
change,
and maybe it is the kind of too-small method that would encourage people
to use it inefficiently.

+    def start_series(self):
+        """We will be creating a series of commits.
+
+        This allows us to hold open the locks while we are processing.
+
+        Make sure to call 'finish_series' when you are done.
+        """
+        self._tree = 
memorytree.MemoryTree.create_on_branch(self._branch)
+        self._tree.lock_write()

I think asserting _tree is None might catch some leakage from tests.
(Obviously an if/raise not a literal assert.)

+    def build_snapshot(self, revision_id, parent_ids, actions,
+                       message=None):
+        """Build a commit, shaped in a specific way.
+
+        :param revision_id: The handle for the new commit, can be None
+        :param parent_ids: A list of parent_ids to use for the commit.
+            It can be None, which indicates to use the last commit.
+        :param actions: A list of actions to perform. Supported actions 
are:
+            ('add', ('path', 'file-id', 'kind', 'content' or None))
+            ('modify', ('file-id', 'new-content'))
+            ('unversion', 'file-id')
+            # not supported yet: ('rename', ('orig-path', 'new-path'))
+        :param message: An optional commit message, if not supplied, a 
default
+            commit message will be written.

+        ;return: The revision_id of the new commit

s/;/:/

You should bump the copyright date on this file.

=== modified file 'bzrlib/errors.py'
--- bzrlib/errors.py    2008-07-14 16:16:48 +0000
+++ bzrlib/errors.py    2008-07-22 17:35:59 +0000
@@ -584,6 +584,16 @@
          self.key = key


+class UnknownBuildAction(BzrError):
+    """Raised when a BranchBuilder gets an action it doesn't know"""
+
+    _fmt = "Unknown build action: %(action)s"
+
+    def __init__(self, action):
+        BzrError.__init__(self)
+        self.action = action
+
+

This is not a user error or environment error, and I think not something
that calling code would ever want to catch specifically, because it
probably indicates a bug in the calling test.  So I don't think it needs 
a
new exception class of its own; a ValueError would be enough.

+    def test_make_branch_builder_with_format(self):
+        format = bzrdir.BzrDirMetaFormat1()
+        format.repository_format = weaverepo.RepositoryFormat7()
+        builder = self.make_branch_builder('dir', format=format)
+        the_branch = builder.get_branch()
+        # Guard against regression into MemoryTransport leaking
+        # files to disk instead of keeping them in memory.
+        self.failIf(osutils.lexists('dir'))
+        self.assertEqual(format.repository_format.__class__,
+                         the_branch.repository._format.__class__)
+
+    def test_make_branch_builder_with_format_name(self):
+        builder = self.make_branch_builder('dir', format='knit')
+        the_branch = builder.get_branch()
+        # Guard against regression into MemoryTransport leaking
+        # files to disk instead of keeping them in memory.
+        self.failIf(osutils.lexists('dir'))
+        dir_format = bzrdir.format_registry.make_bzrdir('knit')
+        self.assertEqual(dir_format.repository_format.__class__,
+                         the_branch.repository._format.__class__)

Maybe here you should look into the transport to see the actual created
repository is of the expected type, with something like

   self.assertEqual('Bazaar-NG Knit Repository Format 1',
self.get_transport().get_file_bytes('dir/.bzr/repository/format'))


--
Martin


For details, see: 
http://bundlebuggy.aaronbentley.com/request/%3C48864712.1000309%40arbash-meinel.com%3E