[MERGE] BranchBuilder updates
John Arbash Meinel
john at arbash-meinel.com
Tue Jul 29 17:31:59 BST 2008
-----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
More information about the bazaar
mailing list