[MERGE] Policy-based repository acquisition

[Andrew Bennetts]

Aaron Bentley wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Hi all,
> 
> This patch is part of my work on supporting remote policies for stacking.
> 
> It implements policy-based repository creation, and lays the groundwork
> for policy-based stacking configuration.
> 
> It basically rearranges BzrDir._find_or_create_repository drastically so
> that the BzrDir can control more policy decisions than only whether to
> create a repository.  The rearrangement was necessary to avoid
> multiplying the number of server roundtrips required to determine the
> policy.
> 
> BzrDir.clone_on_transport is used by "bzr push" to create its branches,
> so it was the natural first place to implement this policy.

This seems basically reasonable to me, so:

bb:tweak

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.

[...]
> +class RepositoryPolicy(object):
> +    """Base class for other policies to inherit from"""

This name and docstring are a little bit too general.  If someone found this
class without having any other context, I think they'd be mystified about its
purpose.

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

> +
> +    def __init__(self):
> +        pass

You may as well remove this __init__.   In Python 2.4 things that inherit from
object always have an __init__, so you don't need a no-op one for subclasses.

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

> +class CreateRepository(RepositoryPolicy):
> +    """A policy of creating a new repository"""
> +
> +    def __init__(self, bzrdir):
> +        RepositoryPolicy.__init__(self)
> +        self._bzrdir = bzrdir
> +
> +    def acquire_repository(self, make_working_trees=None, shared=False):
> +        """Implementation of RepositoryPolicy.acquire_repository
> +
> +        Creates the desired repository
> +        """

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 @@
>          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?

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?

> +    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, even though
most implementations should just raise TestNotApplicable, because this test
ought to apply to RemoteBzrDir as well.  I suspect it doesn't pass at the
moment, I though I might be pleasantly surprised :)

-Andrew.