[MERGE] Policy-based repository acquisition
aaron at aaronbentley.com
Wed Apr 9 14:18:47 BST 2008
-----BEGIN PGP SIGNED MESSAGE-----
Andrew Bennetts wrote:
> Aaron Bentley wrote:
> This seems basically reasonable to me, so:
Thanks for your review
> I particularly like that in general things are broken up into smaller pieces by
> this change.
>> === modified file 'bzrlib/bzrdir.py'
>> + def cloning_metadir(self):
>> + """Produce a metadir suitable for cloning with."""
>> + return self._format.__class__()
> I don't think this method is going to produce the right result with
> RemoteBzrDirs. You are missing some test coverage for this I suspect.
I don't understand. I'm implementing a method on BzrDirPreSplitOut,
which RemoteBzrDir does not derive from.
> How about:
> """Abstract base class for repository acquisition policies.
> A repository acquisition policy decides how a BzrDir finds a repository.
> e.g. if a new repository should be created in this BzrDir, or an existing
> repository from elsewhere should be reused.
>> + def acquire_repository(self, make_working_trees=None, shared=False):
>> + """Apply any configuration data from this policy to the branch"""
>> + raise NotImplemented(RepositoryPolicy.acquire_repository)
> This docstring isn't quite right :)
Well, it's more right when the actual stacking policy stuff lands, but
I'll fix it.
> I'd expand slightly, and say “Creates the desired repository on the bzrdir we
> already have.”
>> === modified file 'bzrlib/tests/test_bzrdir.py'
>> --- bzrlib/tests/test_bzrdir.py 2008-02-13 22:11:40 +0000
>> +++ bzrlib/tests/test_bzrdir.py 2008-04-04 19:32:36 +0000
>> @@ -453,6 +453,12 @@
>> + def test_acquire_repository_standalone(self):
>> + my_bzrdir = self.make_bzrdir('.')
>> + repo_policy = my_bzrdir.determine_repository_policy()
>> + repo = repo_policy.acquire_repository()
>> + self.assertEqual(repo.bzrdir.root_transport.base,
>> + my_bzrdir.root_transport.base)
> This seems like a strange method to have on TestBzrDirFormat, which is described
> as "Tests for the BzrDirFormat facility." Maybe it'd be better to make a new
> case for this?
> I don't think the intent of this test is entirely self-evident, either. How
> about adding a docstring like: "A bzrdir will by default use itself when
> acquiring a repository (creating one if necessary.)" Did I guess right?
The bit about creating one if necessary is wrong-- it always creates it.
>> + def test_clone_on_transport_preserves_repo_format(self):
>> + source_branch = self.make_branch('source', format='knit')
>> + # Ensure no format data is cached
>> + a_dir = bzrlib.branch.Branch.open_from_transport(
>> + self.get_transport('source')).bzrdir
>> + target_transport = a_dir.root_transport.clone('..').clone('target')
>> + target_bzrdir = a_dir.clone_on_transport(target_transport)
>> + target_repo = target_bzrdir.open_repository()
>> + self.assertEqual(target_repo._format, source_branch.repository._format)
> I think this probably ought to be a bzrdir_implementations test
No. I tried it and it just didn't work properly. It seemed like the
test infrastructure was interfering with creating a knit repo.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
-----END PGP SIGNATURE-----
More information about the bazaar