[MERGE] BranchBuilder updates

[John Arbash Meinel]

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

Martin Pool wrote:
| 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)
|

Done for both. Including commenting that _tree is not meant for callers.

|
| +    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.

Yeah, I was actually suprised we don't have a 'set_last_revision()'
function in the public api, but it seems the update_revisions implements
this on its own. Which is the actual api used by push/pull.

As a long-term thing, I think we may want to consider it. Because
branches shouldn't *need* the revno, we only cache it here because we
don't cache it elsewhere, and it is helpful for things like RevisionSpec
lookups.

If we switch to caching the distance_from_null (aka revno) for
revision_ids elsewhere, then I would defer to that rather than setting
it on all the branches.

However, as you say, out-of-scope for this patch.

|
| +    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.)

Done


...

| +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.
|

Well, I did want to test that an exception was raised with a meaningful
error, so that people using the API to build their trees could quickly
know that they did something wrong and possibly help them fix it. But I
can do that with ValueError.


| +    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'))
|

Done.

|
| --
| Martin


I also had a patch which implemented 'rename' support, but was waiting
because I didn't want to pester the list more. So now I'm posting that
for review. (I need it for my merge testing anyway.)

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

iEYEARECAAYFAkiPRf8ACgkQJdeBCYSNAAMixgCePhNKi2533fvk0Pbk8t0QaEpk
GB0An0kfXj2JPR40U256Ft4HsCLhNypZ
=ZPzi
-----END PGP SIGNATURE-----
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: branch_builder_rename.patch
Url: https://lists.ubuntu.com/archives/bazaar/attachments/20080729/1a083fee/attachment.diff