Merge lp:~jelmer/bzr/sprout-to-bzrdir into lp:bzr

[Jelmer Vernooij]

Move bzr-specific implementation of ControlDir.sprout to Bzrdir.

Move the implementation of ControlDir.sprout() to BzrDir.sprout(). Instead, simply raise NotImplementedError.

The current implementation is too BzrDir-specific, so it makes more sense to just have the foreign plugins provide their own implementation.

[Andrew Bennetts (spiv)]

This seems like a reasonable change to me.

It'd be nice to slowly converge on having foreign formats and BzrDir do as much in common as possible, but as a first step acknowledging that sprout is currently quite BzrDir-specific is fair enough.

review: Approve

[Andrew Bennetts (spiv)]

(Don't forget to mention this in release-notes)

[Jelmer Vernooij (jelmer)]

sent to pqm by email

=== modified file 'bzrlib/bzrdir.py'
--- bzrlib/bzrdir.py	2011-03-22 12:11:20 +0000
+++ bzrlib/bzrdir.py	2011-03-24 00:18:30 +0000
@@ -33,9 +33,11 @@
 
 import bzrlib
 from bzrlib import (
+    cleanup,
     config,
     controldir,
     errors,
+    fetch,
     graph,
     lockable_files,
     lockdir,
@@ -60,6 +62,7 @@
 """)
 
 from bzrlib.trace import (
+    mutter,
     note,
     )
 
@@ -387,6 +390,161 @@
         policy = self.determine_repository_policy(force_new_repo)
         return policy.acquire_repository()[0]
 
+    def _find_source_repo(self, add_cleanup, source_branch):
+        """Find the source branch and repo for a sprout operation.
+        
+        This is helper intended for use by _sprout.
+
+        :returns: (source_branch, source_repository).  Either or both may be
+            None.  If not None, they will be read-locked (and their unlock(s)
+            scheduled via the add_cleanup param).
+        """
+        if source_branch is not None:
+            add_cleanup(source_branch.lock_read().unlock)
+            return source_branch, source_branch.repository
+        try:
+            source_branch = self.open_branch()
+            source_repository = source_branch.repository
+        except errors.NotBranchError:
+            source_branch = None
+            try:
+                source_repository = self.open_repository()
+            except errors.NoRepositoryPresent:
+                source_repository = None
+            else:
+                add_cleanup(source_repository.lock_read().unlock)
+        else:
+            add_cleanup(source_branch.lock_read().unlock)
+        return source_branch, source_repository
+
+    def sprout(self, url, revision_id=None, force_new_repo=False,
+               recurse='down', possible_transports=None,
+               accelerator_tree=None, hardlink=False, stacked=False,
+               source_branch=None, create_tree_if_local=True):
+        """Create a copy of this controldir prepared for use as a new line of
+        development.
+
+        If url's last component does not exist, it will be created.
+
+        Attributes related to the identity of the source branch like
+        branch nickname will be cleaned, a working tree is created
+        whether one existed before or not; and a local branch is always
+        created.
+
+        if revision_id is not None, then the clone operation may tune
+            itself to download less data.
+        :param accelerator_tree: A tree which can be used for retrieving file
+            contents more quickly than the revision tree, i.e. a workingtree.
+            The revision tree will be used for cases where accelerator_tree's
+            content is different.
+        :param hardlink: If true, hard-link files from accelerator_tree,
+            where possible.
+        :param stacked: If true, create a stacked branch referring to the
+            location of this control directory.
+        :param create_tree_if_local: If true, a working-tree will be created
+            when working locally.
+        """
+        operation = cleanup.OperationWithCleanups(self._sprout)
+        return operation.run(url, revision_id=revision_id,
+            force_new_repo=force_new_repo, recurse=recurse,
+            possible_transports=possible_transports,
+            accelerator_tree=accelerator_tree, hardlink=hardlink,
+            stacked=stacked, source_branch=source_branch,
+            create_tree_if_local=create_tree_if_local)
+
+    def _sprout(self, op, url, revision_id=None, force_new_repo=False,
+               recurse='down', possible_transports=None,
+               accelerator_tree=None, hardlink=False, stacked=False,
+               source_branch=None, create_tree_if_local=True):
+        add_cleanup = op.add_cleanup
+        fetch_spec_factory = fetch.FetchSpecFactory()
+        if revision_id is not None:
+            fetch_spec_factory.add_revision_ids([revision_id])
+            fetch_spec_factory.source_branch_stop_revision_id = revision_id
+        target_transport = _mod_transport.get_transport(url,
+            possible_transports)
+        target_transport.ensure_base()
+        cloning_format = self.cloning_metadir(stacked)
+        # Create/update the result branch
+        result = cloning_format.initialize_on_transport(target_transport)
+        source_branch, source_repository = self._find_source_repo(
+            add_cleanup, source_branch)
+        fetch_spec_factory.source_branch = source_branch
+        # if a stacked branch wasn't requested, we don't create one
+        # even if the origin was stacked
+        if stacked and source_branch is not None:
+            stacked_branch_url = self.root_transport.base
+        else:
+            stacked_branch_url = None
+        repository_policy = result.determine_repository_policy(
+            force_new_repo, stacked_branch_url, require_stacking=stacked)
+        result_repo, is_new_repo = repository_policy.acquire_repository()
+        add_cleanup(result_repo.lock_write().unlock)
+        fetch_spec_factory.source_repo = source_repository
+        fetch_spec_factory.target_repo = result_repo
+        if stacked or (len(result_repo._fallback_repositories) != 0):
+            target_repo_kind = fetch.TargetRepoKinds.STACKED
+        elif is_new_repo:
+            target_repo_kind = fetch.TargetRepoKinds.EMPTY
+        else:
+            target_repo_kind = fetch.TargetRepoKinds.PREEXISTING
+        fetch_spec_factory.target_repo_kind = target_repo_kind
+        if source_repository is not None:
+            fetch_spec = fetch_spec_factory.make_fetch_spec()
+            result_repo.fetch(source_repository, fetch_spec=fetch_spec)
+
+        if source_branch is None:
+            # this is for sprouting a controldir without a branch; is that
+            # actually useful?
+            # Not especially, but it's part of the contract.
+            result_branch = result.create_branch()
+        else:
+            result_branch = source_branch.sprout(result,
+                revision_id=revision_id, repository_policy=repository_policy,
+                repository=result_repo)
+        mutter("created new branch %r" % (result_branch,))
+
+        # Create/update the result working tree
+        if (create_tree_if_local and
+            isinstance(target_transport, local.LocalTransport) and
+            (result_repo is None or result_repo.make_working_trees())):
+            wt = result.create_workingtree(accelerator_tree=accelerator_tree,
+                hardlink=hardlink, from_branch=result_branch)
+            wt.lock_write()
+            try:
+                if wt.path2id('') is None:
+                    try:
+                        wt.set_root_id(self.open_workingtree.get_root_id())
+                    except errors.NoWorkingTree:
+                        pass
+            finally:
+                wt.unlock()
+        else:
+            wt = None
+        if recurse == 'down':
+            basis = None
+            if wt is not None:
+                basis = wt.basis_tree()
+            elif result_branch is not None:
+                basis = result_branch.basis_tree()
+            elif source_branch is not None:
+                basis = source_branch.basis_tree()
+            if basis is not None:
+                add_cleanup(basis.lock_read().unlock)
+                subtrees = basis.iter_references()
+            else:
+                subtrees = []
+            for path, file_id in subtrees:
+                target = urlutils.join(url, urlutils.escape(path))
+                sublocation = source_branch.reference_parent(file_id, path)
+                sublocation.bzrdir.sprout(target,
+                    basis.get_reference_revision(file_id, path),
+                    force_new_repo=force_new_repo, recurse=recurse,
+                    stacked=stacked)
+        return result
+
+
+
     @staticmethod
     def create_branch_convenience(base, force_new_repo=False,
                                   force_new_tree=None, format=None,

=== modified file 'bzrlib/controldir.py'
--- bzrlib/controldir.py	2011-03-23 14:31:38 +0000
+++ bzrlib/controldir.py	2011-03-24 00:18:30 +0000
@@ -27,9 +27,7 @@
 import textwrap
 
 from bzrlib import (
-    cleanup,
     errors,
-    fetch,
     revision as _mod_revision,
     transport as _mod_transport,
     ui,
@@ -38,9 +36,6 @@
 from bzrlib.push import (
     PushResult,
     )
-from bzrlib.trace import (
-    mutter,
-    )
 from bzrlib.transport import (
     local,
     )
@@ -336,131 +331,7 @@
         :param create_tree_if_local: If true, a working-tree will be created
             when working locally.
         """
-        operation = cleanup.OperationWithCleanups(self._sprout)
-        return operation.run(url, revision_id=revision_id,
-            force_new_repo=force_new_repo, recurse=recurse,
-            possible_transports=possible_transports,
-            accelerator_tree=accelerator_tree, hardlink=hardlink,
-            stacked=stacked, source_branch=source_branch,
-            create_tree_if_local=create_tree_if_local)
-
-    def _sprout(self, op, url, revision_id=None, force_new_repo=False,
-               recurse='down', possible_transports=None,
-               accelerator_tree=None, hardlink=False, stacked=False,
-               source_branch=None, create_tree_if_local=True):
-        add_cleanup = op.add_cleanup
-        fetch_spec_factory = fetch.FetchSpecFactory()
-        if revision_id is not None:
-            fetch_spec_factory.add_revision_ids([revision_id])
-            fetch_spec_factory.source_branch_stop_revision_id = revision_id
-        target_transport = _mod_transport.get_transport(url,
-            possible_transports)
-        target_transport.ensure_base()
-        cloning_format = self.cloning_metadir(stacked)
-        # Create/update the result branch
-        result = cloning_format.initialize_on_transport(target_transport)
-        source_branch, source_repository = self._find_source_repo(
-            add_cleanup, source_branch)
-        fetch_spec_factory.source_branch = source_branch
-        # if a stacked branch wasn't requested, we don't create one
-        # even if the origin was stacked
-        if stacked and source_branch is not None:
-            stacked_branch_url = self.root_transport.base
-        else:
-            stacked_branch_url = None
-        repository_policy = result.determine_repository_policy(
-            force_new_repo, stacked_branch_url, require_stacking=stacked)
-        result_repo, is_new_repo = repository_policy.acquire_repository()
-        add_cleanup(result_repo.lock_write().unlock)
-        fetch_spec_factory.source_repo = source_repository
-        fetch_spec_factory.target_repo = result_repo
-        if stacked or (len(result_repo._fallback_repositories) != 0):
-            target_repo_kind = fetch.TargetRepoKinds.STACKED
-        elif is_new_repo:
-            target_repo_kind = fetch.TargetRepoKinds.EMPTY
-        else:
-            target_repo_kind = fetch.TargetRepoKinds.PREEXISTING
-        fetch_spec_factory.target_repo_kind = target_repo_kind
-        if source_repository is not None:
-            fetch_spec = fetch_spec_factory.make_fetch_spec()
-            result_repo.fetch(source_repository, fetch_spec=fetch_spec)
-
-        if source_branch is None:
-            # this is for sprouting a controldir without a branch; is that
-            # actually useful?
-            # Not especially, but it's part of the contract.
-            result_branch = result.create_branch()
-        else:
-            result_branch = source_branch.sprout(result,
-                revision_id=revision_id, repository_policy=repository_policy,
-                repository=result_repo)
-        mutter("created new branch %r" % (result_branch,))
-
-        # Create/update the result working tree
-        if (create_tree_if_local and
-            isinstance(target_transport, local.LocalTransport) and
-            (result_repo is None or result_repo.make_working_trees())):
-            wt = result.create_workingtree(accelerator_tree=accelerator_tree,
-                hardlink=hardlink, from_branch=result_branch)
-            wt.lock_write()
-            try:
-                if wt.path2id('') is None:
-                    try:
-                        wt.set_root_id(self.open_workingtree.get_root_id())
-                    except errors.NoWorkingTree:
-                        pass
-            finally:
-                wt.unlock()
-        else:
-            wt = None
-        if recurse == 'down':
-            basis = None
-            if wt is not None:
-                basis = wt.basis_tree()
-            elif result_branch is not None:
-                basis = result_branch.basis_tree()
-            elif source_branch is not None:
-                basis = source_branch.basis_tree()
-            if basis is not None:
-                add_cleanup(basis.lock_read().unlock)
-                subtrees = basis.iter_references()
-            else:
-                subtrees = []
-            for path, file_id in subtrees:
-                target = urlutils.join(url, urlutils.escape(path))
-                sublocation = source_branch.reference_parent(file_id, path)
-                sublocation.bzrdir.sprout(target,
-                    basis.get_reference_revision(file_id, path),
-                    force_new_repo=force_new_repo, recurse=recurse,
-                    stacked=stacked)
-        return result
-
-    def _find_source_repo(self, add_cleanup, source_branch):
-        """Find the source branch and repo for a sprout operation.
-        
-        This is helper intended for use by _sprout.
-
-        :returns: (source_branch, source_repository).  Either or both may be
-            None.  If not None, they will be read-locked (and their unlock(s)
-            scheduled via the add_cleanup param).
-        """
-        if source_branch is not None:
-            add_cleanup(source_branch.lock_read().unlock)
-            return source_branch, source_branch.repository
-        try:
-            source_branch = self.open_branch()
-            source_repository = source_branch.repository
-        except errors.NotBranchError:
-            source_branch = None
-            try:
-                source_repository = self.open_repository()
-            except errors.NoRepositoryPresent:
-                source_repository = None
-            else:
-                add_cleanup(source_repository.lock_read().unlock)
-        else:
-            add_cleanup(source_branch.lock_read().unlock)
-        return source_branch, source_repository
+        raise NotImplementedError(self.sprout)
 
     def push_branch(self, source, revision_id=None, overwrite=False, 
         remember=False, create_prefix=False):

=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2011-03-23 12:56:01 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2011-03-24 00:18:30 +0000
@@ -230,6 +230,10 @@
   ``import_last_revision_info_and_tags`` method instead.
   (Andrew Bennetts)
 
+* Because it was too specific to BzrDir implementations,
+  ``ControlDir.sprout`` no longer has a default implementation; it now
+  raises ``NotImplementedError``. (Jelmer Vernooij, #717937)
+
 * ``ControlDirFormat.register_format`` has been removed. Instead,
   ``Prober`` implementations should now implement a ``known_formats``
   method. (Jelmer Vernooij)