[MERGE] Policy-based repository acquisition

[Aaron Bentley]

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

Andrew Bennetts wrote:
> Aaron Bentley wrote:
> This seems basically reasonable to me, so:
> 
> bb:tweak

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:
> 
> RepositoryAcquistionPolicy(object):
>     """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.
>     """

Sure.

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

Okay.

>> === 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 @@
>>          branch.bzrdir.open_repository()
>>          branch.bzrdir.open_workingtree()
>>  
>> +    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?

Sure.

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

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

iD8DBQFH/MI30F+nu1YWqI0RAn9eAJ42iaSsrmlmljGTxZam3NKH1aUzRACfcGg7
b7TPaK+CAFShaaUgcfua8gY=
=WiTn
-----END PGP SIGNATURE-----