Merge lp:~jelmer/bzr/switch-possible-transports into lp:bzr

[Jelmer Vernooij]

Use possible_transports in 'bzr switch'.

This means that switching a lightweight checkout from one branch on Launchpad
to another branch on Launchpad no longer requires five connections, but just one.
This greatly speeds up switching.

Add a possible_transports argument to BzrDir.open_workingtree(). This is
necessary, as it opens branches, so it needs to pass possible_transports along
(e.g. in case the local branch is a branch reference).

Switch could still be improved to actually do fewer open_branch() calls,
but there's a separate bug about that - bug #812295

[Martin Packman (gz)]

This seems like a pretty reasonable interface change overall. Though not reopening the same branch should be the end goal, you'd still want this so that opening different remote branches over the same transport would work.

Does seem like some automated testing is needed. Even if we don't have infrastructure for tracking connection opens at the moment for the switch side, checking that open_workingtree passes the transport down correctly seems necessary.

review: Needs Fixing

[Jelmer Vernooij (jelmer)]

I've now updated it to test the number of connections. You can nicely see the HPSS call count down from 38 down to 24, and the number of connections from 7 to 1. :-)

[Vincent Ladeuil (vila)]

... profit ;)

28 + possible_transports.append(br_from.bzrdir.root_transport)
75 + possible_transports.append(branch.bzrdir.transport)
97 + possible_transports.append(to_branch.bzrdir.transport)

Huh ? This shouldn't be required, you already passed possible_transports
which, being a container, should have received the new transport (or reused
an existing one).

May be you added them before adding possible_transport as a parameter where
needed ? Having to do both has a weird smell otherwise...

I won't block on that but would like to understand what I'm missing here... (I would approve with both hands otherwise ;).

review: Needs Information

[Jelmer Vernooij (jelmer)]

Am 21/12/11 11:36, schrieb Vincent Ladeuil:
> Review: Needs Information
>
> ... profit ;)
>
> 28 + possible_transports.append(br_from.bzrdir.root_transport)
> 75 + possible_transports.append(branch.bzrdir.transport)
> 97 + possible_transports.append(to_branch.bzrdir.transport)
>
> Huh ? This shouldn't be required, you already passed possible_transports
> which, being a container, should have received the new transport (or reused
> an existing one).
possible_transports is read-only as far as I know. It's always the
callers that update it, as far as I can tell. None of our code updates
possible_transports when it is passed AFAIK.

> May be you added them before adding possible_transport as a parameter where
> needed ? Having to do both has a weird smell otherwise...
>
> I won't block on that but would like to understand what I'm missing here... (I would approve with both hands otherwise ;).
:-) Thanks

Cheers,

Jelmer

[Jelmer Vernooij (jelmer)]

You were right, bzrlib.transport already takes care of updating possible_transports.

I've updated the branch to not mess with possible_transports in bzrlib.builtins, and the tests still pass.

[Vincent Ladeuil (vila)]

28 + possible_transports.append(br_from.bzrdir.root_transport)

Left over ?

138 + if possible_transports is None:
139 + possible_transports = []
140 + else:
141 + possible_transports = list(possible_transports) + [
142 + self.root_transport]

Are you sure you need that ? That seems very bad at it creates a new object which breaks the sharing with callers. You should just use .append() if you really need to, but do you ?

review: Needs Fixing

[Jelmer Vernooij (jelmer)]

On 12/21/2011 06:37 PM, Vincent Ladeuil wrote:
> Review: Needs Fixing
>
> 28 + possible_transports.append(br_from.bzrdir.root_transport)
>
> Left over ?
Yep, removed now.
>
>
> 138 + if possible_transports is None:
> 139 + possible_transports = []
> 140 + else:
> 141 + possible_transports = list(possible_transports) + [
> 142 + self.root_transport]
>
> Are you sure you need that ? That seems very bad at it creates a new
object which breaks the sharing with callers. You should just use
.append() if you really need to, but do you ?
Apparently it's not necessary. Removed now as well.

Cheers,

Jelmer

[Vincent Ladeuil (vila)]

155 + if possible_transports is None:
156 + possible_transports = []
157 + else:
158 + possible_transports = list(possible_transports) + [
159 + self.root_transport]

Again ????

You should only need:

if possible_transports is None:
    possible_transports = []

review: Needs Fixing

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2011-12-21 16:52:39 +0000
+++ bzrlib/builtins.py	2011-12-21 18:08:32 +0000
@@ -1342,7 +1342,9 @@
             to_location = getattr(br_from, "name", None)
             if to_location is None:
                 to_location = urlutils.derive_to_location(from_location)
-        to_transport = transport.get_transport(to_location)
+        possible_transports = []
+        to_transport = transport.get_transport(to_location,
+            possible_transports=possible_transports)
         try:
             to_transport.mkdir('.')
         except errors.FileExists:
@@ -1371,15 +1373,14 @@
             try:
                 # preserve whatever source format we have.
                 to_dir = br_from.bzrdir.sprout(to_transport.base, revision_id,
-                                            possible_transports=[to_transport],
-                                            accelerator_tree=accelerator_tree,
-                                            hardlink=hardlink, stacked=stacked,
-                                            force_new_repo=standalone,
-                                            create_tree_if_local=not no_tree,
-                                            source_branch=br_from)
+                                    possible_transports=possible_transports,
+                                    accelerator_tree=accelerator_tree,
+                                    hardlink=hardlink, stacked=stacked,
+                                    force_new_repo=standalone,
+                                    create_tree_if_local=not no_tree,
+                                    source_branch=br_from)
                 branch = to_dir.open_branch(
-                    possible_transports=[
-                        br_from.bzrdir.root_transport, to_transport])
+                    possible_transports=possible_transports)
             except errors.NoSuchRevision:
                 to_transport.delete_tree('.')
                 msg = gettext("The branch {0} has no revision {1}.").format(
@@ -1399,13 +1400,15 @@
             note(ngettext('Branched %d revision.', 'Branched %d revisions.', branch.revno()) % branch.revno())
         if bind:
             # Bind to the parent
-            parent_branch = Branch.open(from_location)
+            parent_branch = Branch.open(from_location,
+                possible_transports=possible_transports)
             branch.bind(parent_branch)
             note(gettext('New branch bound to %s') % from_location)
         if switch:
             # Switch to the new branch
             wt, _ = WorkingTree.open_containing('.')
-            _mod_switch.switch(wt.bzrdir, branch)
+            _mod_switch.switch(wt.bzrdir, branch,
+                possible_transports=possible_transports)
             note(gettext('Switched to branch: %s'),
                 urlutils.unescape_for_display(branch.base, 'utf-8'))
 
@@ -6109,14 +6112,17 @@
         from bzrlib import switch
         tree_location = directory
         revision = _get_one_revision('switch', revision)
-        control_dir = controldir.ControlDir.open_containing(tree_location)[0]
+        possible_transports = []
+        control_dir = controldir.ControlDir.open_containing(
+            tree_location, possible_transports=possible_transports)[0]
         if to_location is None:
             if revision is None:
                 raise errors.BzrCommandError(gettext('You must supply either a'
                                              ' revision or a location'))
             to_location = tree_location
         try:
-            branch = control_dir.open_branch()
+            branch = control_dir.open_branch(
+                possible_transports=possible_transports)
             had_explicit_nick = branch.get_config().has_explicit_nickname()
         except errors.NotBranchError:
             branch = None
@@ -6133,7 +6139,7 @@
                 # Perhaps the target control dir supports colocated branches?
                 try:
                     root = controldir.ControlDir.open(this_url,
-                        possible_transports=[control_dir.user_transport])
+                        possible_transports=possible_transports)
                 except errors.NotBranchError:
                     colocated = False
                 else:
@@ -6145,25 +6151,31 @@
                     to_location = urlutils.join(
                         this_url, '..', urlutils.escape(to_location))
             to_branch = branch.bzrdir.sprout(to_location,
-                                 possible_transports=[branch.bzrdir.root_transport],
-                                 source_branch=branch).open_branch()
+                                 possible_transports=possible_transports,
+                                 source_branch=branch).open_branch(
+                possible_transports=possible_transports)
         else:
             # Perhaps it's a colocated branch?
             try:
-                to_branch = control_dir.open_branch(to_location)
+                to_branch = control_dir.open_branch(
+                    to_location, possible_transports=possible_transports)
             except (errors.NotBranchError, errors.NoColocatedBranchSupport):
                 try:
-                    to_branch = Branch.open(to_location)
+                    to_branch = Branch.open(to_location,
+                        possible_transports=possible_transports)
                 except errors.NotBranchError:
                     this_url = self._get_branch_location(control_dir)
                     to_branch = Branch.open(
                         urlutils.join(
-                            this_url, '..', urlutils.escape(to_location)))
+                            this_url, '..', urlutils.escape(to_location)),
+                        possible_transports=possible_transports)
         if revision is not None:
             revision = revision.as_revision_id(to_branch)
-        switch.switch(control_dir, to_branch, force, revision_id=revision)
+        switch.switch(control_dir, to_branch, force, revision_id=revision,
+                      possible_transports=possible_transports)
         if had_explicit_nick:
-            branch = control_dir.open_branch() #get the new branch!
+            #get the new branch!
+            branch = control_dir.open_branch(possible_transports=possible_transports)
             branch.nick = to_branch.nick
         note(gettext('Switched to branch: %s'),
             urlutils.unescape_for_display(to_branch.base, 'utf-8'))

=== modified file 'bzrlib/bzrdir.py'
--- bzrlib/bzrdir.py	2011-12-19 13:23:58 +0000
+++ bzrlib/bzrdir.py	2011-12-21 18:08:32 +0000
@@ -373,13 +373,8 @@
         if revision_id is not None:
             fetch_spec_factory.add_revision_ids([revision_id])
             fetch_spec_factory.source_branch_stop_revision_id = revision_id
-        if possible_transports is None:
-            possible_transports = []
-        else:
-            possible_transports = list(possible_transports) + [
-                self.root_transport]
         target_transport = _mod_transport.get_transport(url,
-            possible_transports)
+            possible_transports=possible_transports)
         target_transport.ensure_base()
         cloning_format = self.cloning_metadir(stacked)
         # Create/update the result branch
@@ -974,13 +969,19 @@
         return format.open(self, _found=True)
 
     def open_workingtree(self, unsupported=False,
-            recommend_upgrade=True):
+            recommend_upgrade=True, possible_transports=None):
         """See BzrDir.open_workingtree."""
         from bzrlib.workingtree import WorkingTreeFormatMetaDir
         format = WorkingTreeFormatMetaDir.find_format(self)
         format.check_support_status(unsupported, recommend_upgrade,
             basedir=self.root_transport.base)
-        return format.open(self, _found=True)
+        if possible_transports is None:
+            possible_transports = []
+        else:
+            possible_transports = list(possible_transports) + [
+                self.root_transport]
+        return format.open(self, possible_transports=possible_transports,
+            _found=True)
 
     def _get_config(self):
         return config.TransportConfig(self.transport, 'control.conf')

=== modified file 'bzrlib/controldir.py'
--- bzrlib/controldir.py	2011-12-19 13:23:58 +0000
+++ bzrlib/controldir.py	2011-12-21 18:08:32 +0000
@@ -263,7 +263,8 @@
         raise NotImplementedError(self.find_repository)
 
     def open_workingtree(self, _unsupported=False,
-                         recommend_upgrade=True, from_branch=None):
+                         recommend_upgrade=True, from_branch=None,
+                         possible_transports=None):
         """Open the workingtree object at this ControlDir if one is present.
 
         :param recommend_upgrade: Optional keyword parameter, when True (the

=== modified file 'bzrlib/plugins/weave_fmt/bzrdir.py'
--- bzrlib/plugins/weave_fmt/bzrdir.py	2011-12-19 13:23:58 +0000
+++ bzrlib/plugins/weave_fmt/bzrdir.py	2011-12-21 18:08:32 +0000
@@ -966,14 +966,14 @@
         from bzrlib.plugins.weave_fmt.repository import RepositoryFormat5
         return RepositoryFormat5().open(self, _found=True)
 
-    def open_workingtree(self, _unsupported=False,
+    def open_workingtree(self, possible_transports=None, _unsupported=False,
             recommend_upgrade=True):
         """See BzrDir.create_workingtree."""
         from bzrlib.plugins.weave_fmt.workingtree import WorkingTreeFormat2
         wt_format = WorkingTreeFormat2()
         # we don't warn here about upgrades; that ought to be handled for the
         # bzrdir as a whole
-        return wt_format.open(self, _found=True)
+        return wt_format.open(self, possible_transports, _found=True)
 
 
 class BzrDir6(BzrDirPreSplitOut):

=== modified file 'bzrlib/plugins/weave_fmt/workingtree.py'
--- bzrlib/plugins/weave_fmt/workingtree.py	2011-12-19 13:23:58 +0000
+++ bzrlib/plugins/weave_fmt/workingtree.py	2011-12-21 18:08:32 +0000
@@ -120,7 +120,7 @@
         from bzrlib.plugins.weave_fmt.bzrdir import BzrDirFormat6
         self._matchingbzrdir = BzrDirFormat6()
 
-    def open(self, a_bzrdir, _found=False):
+    def open(self, a_bzrdir, possible_transports=None, _found=False):
         """Return the WorkingTree object for a_bzrdir
 
         _found is a private parameter, do not use it. It is used to indicate
@@ -131,11 +131,12 @@
             raise NotImplementedError
         if not isinstance(a_bzrdir.transport, LocalTransport):
             raise errors.NotLocalUrl(a_bzrdir.transport.base)
+        branch = a_bzrdir.open_branch(possible_transports)
         wt = WorkingTree2(a_bzrdir.root_transport.local_abspath('.'),
                            _internal=True,
                            _format=self,
                            _bzrdir=a_bzrdir,
-                           _control_files=a_bzrdir.open_branch().control_files)
+                           _control_files=branch.control_files)
         return wt
 
 

=== modified file 'bzrlib/switch.py'
--- bzrlib/switch.py	2011-12-18 15:28:38 +0000
+++ bzrlib/switch.py	2011-12-21 18:08:32 +0000
@@ -32,7 +32,9 @@
     for hook in hooks:
         hook(params)
 
-def switch(control_dir, to_branch, force=False, quiet=False, revision_id=None):
+
+def switch(control_dir, to_branch, force=False, quiet=False, revision_id=None,
+           possible_transports=None):
     """Switch the branch associated with a checkout.
 
     :param control_dir: ControlDir of the checkout to change
@@ -40,23 +42,28 @@
     :param force: skip the check for local commits in a heavy checkout
     :param revision_id: revision ID to switch to.
     """
-    _check_pending_merges(control_dir, force)
+    _check_pending_merges(control_dir, possible_transports, force)
     try:
-        source_repository = control_dir.open_branch().repository
+        from_branch = control_dir.open_branch(
+            possible_transports=possible_transports)
+        source_repository = from_branch.repository
     except errors.NotBranchError:
         source_repository = to_branch.repository
     _set_branch_location(control_dir, to_branch, force)
-    tree = control_dir.open_workingtree()
+    tree = control_dir.open_workingtree(
+        possible_transports=possible_transports)
     _update(tree, source_repository, quiet, revision_id)
     _run_post_switch_hooks(control_dir, to_branch, force, revision_id)
 
-def _check_pending_merges(control, force=False):
+
+def _check_pending_merges(control, possible_transports=None, force=False):
     """Check that there are no outstanding pending merges before switching.
 
     :param control: ControlDir of the branch to check
     """
     try:
-        tree = control.open_workingtree()
+        tree = control.open_workingtree(
+            possible_transports=possible_transports)
     except errors.NotBranchError, ex:
         # Lightweight checkout and branch is no longer there
         if force:

=== modified file 'bzrlib/tests/blackbox/test_switch.py'
--- bzrlib/tests/blackbox/test_switch.py	2011-12-21 18:08:30 +0000
+++ bzrlib/tests/blackbox/test_switch.py	2011-12-21 18:08:32 +0000
@@ -449,5 +449,5 @@
         # become necessary for this use case. Please do not adjust this number
         # upwards without agreement from bzr's network support maintainers.
         self.assertLength(24, self.hpss_calls)
-        self.assertLength(5, self.hpss_connections)
+        self.assertLength(1, self.hpss_connections)
         self.assertThat(self.hpss_calls, ContainsNoVfsCalls)

=== modified file 'bzrlib/tests/test_workingtree.py'
--- bzrlib/tests/test_workingtree.py	2011-12-11 02:43:03 +0000
+++ bzrlib/tests/test_workingtree.py	2011-12-21 18:08:32 +0000
@@ -149,7 +149,7 @@
     def is_supported(self):
         return False
 
-    def open(self, transport, _found=False):
+    def open(self, path, possible_transports=None, _found=False):
         return "opened tree."
 
 
@@ -169,7 +169,7 @@
     def is_supported(self):
         return False
 
-    def open(self, transport, _found=False):
+    def open(self, path, possible_transports=None, _found=False):
         raise NotImplementedError(self.open)
 
 

=== modified file 'bzrlib/workingtree_3.py'
--- bzrlib/workingtree_3.py	2011-12-19 13:23:58 +0000
+++ bzrlib/workingtree_3.py	2011-12-21 18:08:32 +0000
@@ -237,7 +237,7 @@
     def _initial_inventory(self):
         return inventory.Inventory()
 
-    def open(self, a_bzrdir, _found=False):
+    def open(self, a_bzrdir, possible_transports=None, _found=False):
         """Return the WorkingTree object for a_bzrdir
 
         _found is a private parameter, do not use it. It is used to indicate

=== modified file 'bzrlib/workingtree_4.py'
--- bzrlib/workingtree_4.py	2011-12-19 17:39:35 +0000
+++ bzrlib/workingtree_4.py	2011-12-21 18:08:32 +0000
@@ -1560,7 +1560,7 @@
         :param wt: the WorkingTree object
         """
 
-    def open(self, a_bzrdir, _found=False):
+    def open(self, a_bzrdir, possible_transports=None, _found=False):
         """Return the WorkingTree object for a_bzrdir
 
         _found is a private parameter, do not use it. It is used to indicate
@@ -1571,17 +1571,19 @@
             raise NotImplementedError
         if not isinstance(a_bzrdir.transport, LocalTransport):
             raise errors.NotLocalUrl(a_bzrdir.transport.base)
-        wt = self._open(a_bzrdir, self._open_control_files(a_bzrdir))
+        wt = self._open(a_bzrdir, self._open_control_files(a_bzrdir),
+                        possible_transports)
         return wt
 
-    def _open(self, a_bzrdir, control_files):
+    def _open(self, a_bzrdir, control_files, possible_transports=None):
         """Open the tree itself.
 
         :param a_bzrdir: the dir for the tree.
         :param control_files: the control files for the tree.
         """
         return self._tree_class(a_bzrdir.root_transport.local_abspath('.'),
-                           branch=a_bzrdir.open_branch(),
+                           branch=a_bzrdir.open_branch(
+                               possible_transports=possible_transports),
                            _format=self,
                            _bzrdir=a_bzrdir,
                            _control_files=control_files)

=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- doc/en/release-notes/bzr-2.5.txt	2011-12-21 17:42:34 +0000
+++ doc/en/release-notes/bzr-2.5.txt	2011-12-21 18:08:32 +0000
@@ -36,21 +36,33 @@
 
 * New HPSS call for ``Repository.reconcile``. (Jelmer Vernooij, #894455)
 
+<<<<<<< TREE
 * Merge now has two new hooks ``pre_merge`` and ``post_merge``
   that are called before and after a merge and can make
   additional modifications to the trees involved.
   (Jelmer Vernooij, #906877)
 
+=======
+* ``bzr send`` now only opens a single connection, rather than two,
+  to the target branch.  (Jelmer Vernooij)
+
+>>>>>>> MERGE-SOURCE
 * Override the value returned by ``sys.getfilesystemencoding()`` for the bzr
   script to utf-8 when it would otherwise be ascii on a posix system. This
   will mean bzr works with non-ascii files when no locale or an incorrect
   locale is set. (Martin Packman, #794353)
 
+<<<<<<< TREE
 * ``bzr branches`` now indicates the active colocated branch.
   (Jelmer Vernooij, #891667)
 
 * ``bzr send`` now only opens a single connection, rather than two,
   to the target branch.  (Jelmer Vernooij)
+=======
+* Switching a lightweight checkout to a new branch opens only a single
+  external connection now per branch rather than 5.
+  (Jelmer Vernooij)
+>>>>>>> MERGE-SOURCE
 
 Bug Fixes
 *********
@@ -95,6 +107,9 @@
 .. Changes that may require updates in plugins or other code that uses
    bzrlib.
 
+* ``BzrDir.open_workingtree`` and ``WorkingTreeFormat.open`` now take an optional
+  ``possible_transports`` argument. (Jelmer Vernooij)
+
 * ``Config.signature_needed``, ``Config.signing_policy``,
   ``Config.gpg_signing_key``, ``Config.gpg_signing_command``,
   ``Config.checking_policy`` and ``Config.post_commit`` are now deprecated.