Merge lp:~spiv/bzr/sprout-does-not-reopen-repo into lp:bzr

[Andrew Bennetts]

Avoid reopening (and relocking) the same branches/repositories in ControlDir.sprout.

This is a step towards bug 309682. It's not quite in a finished state, but I would appreciate feedback.

The idea here is pretty simple: ControlDir.sprout shouldn't open the same repository multiple times. This patch achieves that, and sure enough it causes the worst HPSS ratchet to improve by another notch.

There are a couple of dissatisfying aspects, though:

 1. sprout calls branch.sprout after opening the result repository. So this
    patch adds a new param (repository/found_repository) to Branch.sprout,
    Branch.create_branch, BranchFormat.open, etc. This touches a lot of
    signatures. Perhaps something more radical is called for, like a single
    API to acquire or create both a branch and a repo?

 2. For some reason, when stacking, the same fallback is added twice, which
    confuses _unstack. (Maybe this only happens for RemoteBranches?) I'm
    not sure exactly why this happens, but the hack to simply ignore
    duplicate _activate_fallback_location calls doesn't feel right. This
    is an XXX in the patch.

 3. It's at least theoretically possible that the repository object we've
    already acquired locally doesn't have the same location as the one
    suggested by the server (e.g. if the server has a bug, or there's a race
    with another client changing something, or perhaps it can even happen
    legitimately?). What ought to happen in this case? I suppose this is
    already an issue, which the code currently ignores because of the
    independent open_repository calls. This is an XXX and AssertionError
    in the patch.

Comments on these or any other parts of this patch are very welcome.

[lp:~spiv/bzr/sprout-does-not-reopen-repo]

5540. By Andrew Bennetts on 2010-11-24

Merge lp:bzr

5541. By Andrew Bennetts on 2010-11-30

found_repository test fix from fetch-all-tags-309682.

5542. By Andrew Bennetts on 2010-11-30

Another small test fix from fetch-all-tags-309682.

5543. By Andrew Bennetts on 2010-11-30

Better repository URL-mismatch logic in RemoteBranchFormat.initialize from fetch-all-tags-309682.

[John A Meinel (jameinel)]

Sorry about the delay on this. In general, I really like the idea of this patch.

I would at least mutter a warning about the double fallback, we don't have to display it on the screen. It would be nice to fix it, but you don't have to get everything right on the first attempt.

I really don't like the proliferation of "no, really, use this object" parameters. I think the problem is that BzrDir.sprout() wants to be the master, but it still defers some of the work to Branch/Repo/WorkingTree. I don't have a better answer, though. I know you mentioned this, too.

I'd personally really like to see a:
  X.open_write_locked(create=True)

Or something along those lines.

I think part of the confusion is also that if you got something which is a Branch, and you think there is a tree there, the standard use is:
 branch.bzrdir.open_workingtree()

However, that *reopens* the Branch and Repository.

We also have a lot of code that does stuff like:
  a) If there is a WT, do X
  b) If not, but there is a Branch, do Y
  c) If not, but there is a Repository, do Z

It would be nice to simplify a lot of that if we could. That is some of what "BzrDir.open_containing_tree_or_branch" is supposed to be. Though many times I want "open_containing_tree_or_direct_branch" (don't recurse for a branch, but you can recurse for a Tree).

This is potentially disruptive to bzr-svn, because it adds a new parameter that BzrDir will be passing down the stack. As such, adding a cleaner new api may be an easier way to get what we want. It would let us add a compatability shim for objects that don't implement the new interface, and could be cleaner overall.

Then again, the practical side of me says, you've done the work, it works, even if it isn't perfect, it is better than what we have.

review: Needs Information

[Vincent Ladeuil (vila)]

I would almost agree with John about the practical aspect (it works, let's land).

On the other hand, you have already spent a good amount of time on this which will be necessary again if we want to provide a cleaner solution.

But...

267 registry,
268 + repository,
269 revision as _mod_revision,

seems weird. I'd like jelmer's feedback here, but I'm pretty sure we pull controldir out of bzrdir to make sure it was isolated from bzr specifics and having to reintroduce repository here sounds like a step backward.

Also,

10 + # This fallback is already configured. This probably only
11 + # happens because BzrDir.sprout is a horrible mess. To avoid
12 + # confusing _unstack we don't add this a second time.
13 + # XXX: log a warning, maybe?
14 + return

is a sign that you're close to nailing the issue and I think you should ;)

On the overall, we have a 'repository' attribute on the branch object, so having to provide it should only occur when we are creating this object. Do we have that many different ways to create branches that you end up modifying so many signatures ? Doesn't it reveal a deeper issue ?

review: Needs Information

[lp:~spiv/bzr/sprout-does-not-reopen-repo]

5544. By Andrew Bennetts on 2010-12-14

Garden the imports in controldir.py.

[Andrew Bennetts (spiv)]

Vincent Ladeuil wrote:
> Review: Needs Information
> I would almost agree with John about the practical aspect (it works, let's land).
>
> On the other hand, you have already spent a good amount of time on this which will be necessary again if we want to provide a cleaner solution.
>
> But...
>
> 267 registry,
> 268 + repository,
> 269 revision as _mod_revision,
>
> seems weird. I'd like jelmer's feedback here, but I'm pretty sure we pull
> controldir out of bzrdir to make sure it was isolated from bzr specifics and
> having to reintroduce repository here sounds like a step backward.

That seems to be a leftover from an earlier version of this patch. It's
not actually used. I've removed that import (and gardened a couple of
others noticed by pyflakes). Thanks for spotting that.

> Also,
>
> 10 + # This fallback is already configured. This probably only
> 11 + # happens because BzrDir.sprout is a horrible mess. To avoid
> 12 + # confusing _unstack we don't add this a second time.
> 13 + # XXX: log a warning, maybe?
> 14 + return
>
> is a sign that you're close to nailing the issue and I think you should ;)

If I thought I was close I'd have nailed it already!

> On the overall, we have a 'repository' attribute on the branch object, so
> having to provide it should only occur when we are creating this object. Do we
> have that many different ways to create branches that you end up modifying so
> many signatures ? Doesn't it reveal a deeper issue ?

I agree, I think there's a deeper issue here, as my cover letter tried
to say. I'm not sure what a good way solution is.

Regarding the number of ways to create branches, here's one way to count
that, based on this diff:

 * 8 definitions of BranchFormat.initialize
 * 4 definitions of ControlDir.create_branch
 * 1 definition of Branch.sprout

That doesn't count definitions in test code.

Plus there's open methods, which create branch objects, which are also
affected:

 * 3 definitions of BranchFormat.open

So yes, we do have 16 different ways to create a branch object, hence
why I touch so many methods. I don't see an obvious solution to that.

A possible approach might be to have the low-level
BranchFormat.open/initialize methods be designed to return Branches
without the repository object being set, so returning only partially
initialized Branch objects. Then have higher-level APIs (along the
lines of the existing open_containg_tree_or_branch? or something new
here too?) that assemble these into fully initialized objects with
.repository attributes filled in. There's the related consideration
that parts of the code want to open and/or create some combination of
branch/repo/tree in one logical step (and ideally one network
roundtrip), but the current design opens each of the bzrdir, repository,
and branch separately. I think this is part of the cause of the double
add_fallback ugliness that's an XXX for that matter.

So my suspicion is a fairly fundamental redesign of how we open/create
these things would be help a lot... but getting it right may take some
effort. And it's likely to be way more disruptive to bzr-svn and
friends than a new keyword param! Until someone has a concrete ...

[lp:~spiv/bzr/sprout-does-not-reopen-repo]

5545. By Andrew Bennetts on 2010-12-14

Do as the XXX and John's review suggest: log a warning about duplicate fallback activation.

[Jelmer Vernooij (jelmer)]

I've only reviewed this for possible issues with foreign VCS implementations, and it seems fine in that regard (bearing in mind the new repository= parameter).

[Vincent Ladeuil (vila)]

Thanks, Jelmer !

Ok, so my main concerns are addressed, just a few remarks, possibly tweaks:

379 + if not isinstance(repository, RemoteRepository):
380 + raise AssertionError('xxx %r' % (repository,))

replacing this 'xxx' with something meaningful can only help people confronted with the raised assertion ;)

406 + raise AssertionError(
407 + 'diff %r: %r vs. (%r + %r)' %
408 + (url_diff, repository.user_url, medium.base, repo_path))

 ditto (less sure here, the tb may be enough)

And finally, you summary in your reply above would be nice to keep in devnotes IMHO. Let's not redo this the next time we want to address the deeper issue.

review: Approve

[lp:~spiv/bzr/sprout-does-not-reopen-repo]

5546. By Andrew Bennetts on 2010-12-14

Give more helpful message in AssertionErrors, just in case.

5547. By Andrew Bennetts on 2010-12-14

Merge lp:bzr.

5548. By Andrew Bennetts on 2010-12-14

Update release-notes and whats-new.

5549. By Andrew Bennetts on 2010-12-15

Fix a test failure (_TestBranch needs to implement lock_read/unlock).

[Andrew Bennetts (spiv)]

sent to pqm by email

=== modified file 'bzrlib/branch.py'
--- bzrlib/branch.py	2010-11-18 00:22:24 +0000
+++ bzrlib/branch.py	2010-12-15 00:41:26 +0000
@@ -105,6 +105,13 @@
 
     def _activate_fallback_location(self, url):
         """Activate the branch/repository from url as a fallback repository."""
+        for existing_fallback_repo in self.repository._fallback_repositories:
+            if existing_fallback_repo.user_url == url:
+                # This fallback is already configured.  This probably only
+                # happens because BzrDir.sprout is a horrible mess.  To avoid
+                # confusing _unstack we don't add this a second time.
+                mutter('duplicate activation of fallback %r on %r', url, self)
+                return
         repo = self._get_fallback_repository(url)
         if repo.has_same_location(self.repository):
             raise errors.UnstackableLocationError(self.user_url, url)
@@ -809,7 +816,8 @@
             old_repository = self.repository
             if len(old_repository._fallback_repositories) != 1:
                 raise AssertionError("can't cope with fallback repositories "
-                    "of %r" % (self.repository,))
+                    "of %r (fallbacks: %r)" % (old_repository,
+                        old_repository._fallback_repositories))
             # Open the new repository object.
             # Repositories don't offer an interface to remove fallback
             # repositories today; take the conceptually simpler option and just
@@ -1266,7 +1274,8 @@
         return result
 
     @needs_read_lock
-    def sprout(self, to_bzrdir, revision_id=None, repository_policy=None):
+    def sprout(self, to_bzrdir, revision_id=None, repository_policy=None,
+            repository=None):
         """Create a new line of development from the branch, into to_bzrdir.
 
         to_bzrdir controls the branch format.
@@ -1277,7 +1286,7 @@
         if (repository_policy is not None and
             repository_policy.requires_stacking()):
             to_bzrdir._format.require_stacking(_skip_repo=True)
-        result = to_bzrdir.create_branch()
+        result = to_bzrdir.create_branch(repository=repository)
         result.lock_write()
         try:
             if repository_policy is not None:
@@ -1634,7 +1643,8 @@
             hook(params)
 
     def _initialize_helper(self, a_bzrdir, utf8_files, name=None,
-                           lock_type='metadir', set_format=True):
+                           repository=None, lock_type='metadir',
+                           set_format=True):
         """Initialize a branch in a bzrdir, with specified files
 
         :param a_bzrdir: The bzrdir to initialize the branch in
@@ -1674,11 +1684,12 @@
         finally:
             if lock_taken:
                 control_files.unlock()
-        branch = self.open(a_bzrdir, name, _found=True)
+        branch = self.open(a_bzrdir, name, _found=True,
+                found_repository=repository)
         self._run_post_branch_init_hooks(a_bzrdir, name, branch)
         return branch
 
-    def initialize(self, a_bzrdir, name=None):
+    def initialize(self, a_bzrdir, name=None, repository=None):
         """Create a branch of this format in a_bzrdir.
         
         :param name: Name of the colocated branch to create.
@@ -1718,7 +1729,8 @@
         """
         raise NotImplementedError(self.network_name)
 
-    def open(self, a_bzrdir, name=None, _found=False, ignore_fallbacks=False):
+    def open(self, a_bzrdir, name=None, _found=False, ignore_fallbacks=False,
+            found_repository=None):
         """Return the branch object for a_bzrdir
 
         :param a_bzrdir: A BzrDir that contains a branch.
@@ -2018,8 +2030,11 @@
         """See BranchFormat.get_format_description()."""
         return "Branch format 4"
 
-    def initialize(self, a_bzrdir, name=None):
+    def initialize(self, a_bzrdir, name=None, repository=None):
         """Create a branch of this format in a_bzrdir."""
+        if repository is not None:
+            raise NotImplementedError(
+                "initialize(repository=<not None>) on %r" % (self,))
         utf8_files = [('revision-history', ''),
                       ('branch-name', ''),
                       ]
@@ -2034,16 +2049,19 @@
         """The network name for this format is the control dirs disk label."""
         return self._matchingbzrdir.get_format_string()
 
-    def open(self, a_bzrdir, name=None, _found=False, ignore_fallbacks=False):
+    def open(self, a_bzrdir, name=None, _found=False, ignore_fallbacks=False,
+            found_repository=None):
         """See BranchFormat.open()."""
         if not _found:
             # we are being called directly and must probe.
             raise NotImplementedError
+        if found_repository is None:
+            found_repository = a_bzrdir.open_repository()
         return BzrBranch(_format=self,
                          _control_files=a_bzrdir._control_files,
                          a_bzrdir=a_bzrdir,
                          name=name,
-                         _repository=a_bzrdir.open_repository())
+                         _repository=found_repository)
 
     def __str__(self):
         return "Bazaar-NG branch format 4"
@@ -2063,7 +2081,8 @@
         """
         return self.get_format_string()
 
-    def open(self, a_bzrdir, name=None, _found=False, ignore_fallbacks=False):
+    def open(self, a_bzrdir, name=None, _found=False, ignore_fallbacks=False,
+            found_repository=None):
         """See BranchFormat.open()."""
         if not _found:
             format = BranchFormat.find_format(a_bzrdir, name=name)
@@ -2074,11 +2093,13 @@
         try:
             control_files = lockable_files.LockableFiles(transport, 'lock',
                                                          lockdir.LockDir)
+            if found_repository is None:
+                found_repository = a_bzrdir.find_repository()
             return self._branch_class()(_format=self,
                               _control_files=control_files,
                               name=name,
                               a_bzrdir=a_bzrdir,
-                              _repository=a_bzrdir.find_repository(),
+                              _repository=found_repository,
                               ignore_fallbacks=ignore_fallbacks)
         except errors.NoSuchFile:
             raise errors.NotBranchError(path=transport.base, bzrdir=a_bzrdir)
@@ -2116,12 +2137,12 @@
         """See BranchFormat.get_format_description()."""
         return "Branch format 5"
 
-    def initialize(self, a_bzrdir, name=None):
+    def initialize(self, a_bzrdir, name=None, repository=None):
         """Create a branch of this format in a_bzrdir."""
         utf8_files = [('revision-history', ''),
                       ('branch-name', ''),
                       ]
-        return self._initialize_helper(a_bzrdir, utf8_files, name)
+        return self._initialize_helper(a_bzrdir, utf8_files, name, repository)
 
     def supports_tags(self):
         return False
@@ -2149,13 +2170,13 @@
         """See BranchFormat.get_format_description()."""
         return "Branch format 6"
 
-    def initialize(self, a_bzrdir, name=None):
+    def initialize(self, a_bzrdir, name=None, repository=None):
         """Create a branch of this format in a_bzrdir."""
         utf8_files = [('last-revision', '0 null:\n'),
                       ('branch.conf', ''),
                       ('tags', ''),
                       ]
-        return self._initialize_helper(a_bzrdir, utf8_files, name)
+        return self._initialize_helper(a_bzrdir, utf8_files, name, repository)
 
     def make_tags(self, branch):
         """See bzrlib.branch.BranchFormat.make_tags()."""
@@ -2179,14 +2200,14 @@
         """See BranchFormat.get_format_description()."""
         return "Branch format 8"
 
-    def initialize(self, a_bzrdir, name=None):
+    def initialize(self, a_bzrdir, name=None, repository=None):
         """Create a branch of this format in a_bzrdir."""
         utf8_files = [('last-revision', '0 null:\n'),
                       ('branch.conf', ''),
                       ('tags', ''),
                       ('references', '')
                       ]
-        return self._initialize_helper(a_bzrdir, utf8_files, name)
+        return self._initialize_helper(a_bzrdir, utf8_files, name, repository)
 
     def __init__(self):
         super(BzrBranchFormat8, self).__init__()
@@ -2215,13 +2236,13 @@
     This format was introduced in bzr 1.6.
     """
 
-    def initialize(self, a_bzrdir, name=None):
+    def initialize(self, a_bzrdir, name=None, repository=None):
         """Create a branch of this format in a_bzrdir."""
         utf8_files = [('last-revision', '0 null:\n'),
                       ('branch.conf', ''),
                       ('tags', ''),
                       ]
-        return self._initialize_helper(a_bzrdir, utf8_files, name)
+        return self._initialize_helper(a_bzrdir, utf8_files, name, repository)
 
     def _branch_class(self):
         return BzrBranch7
@@ -2269,7 +2290,8 @@
         transport = a_bzrdir.get_branch_transport(None, name=name)
         location = transport.put_bytes('location', to_branch.base)
 
-    def initialize(self, a_bzrdir, name=None, target_branch=None):
+    def initialize(self, a_bzrdir, name=None, target_branch=None,
+            repository=None):
         """Create a branch of this format in a_bzrdir."""
         if target_branch is None:
             # this format does not implement branch itself, thus the implicit
@@ -2303,7 +2325,8 @@
         return clone
 
     def open(self, a_bzrdir, name=None, _found=False, location=None,
-             possible_transports=None, ignore_fallbacks=False):
+             possible_transports=None, ignore_fallbacks=False,
+             found_repository=None):
         """Return the branch that the branch reference in a_bzrdir points at.
 
         :param a_bzrdir: A BzrDir that contains a branch.

=== modified file 'bzrlib/bzrdir.py'
--- bzrlib/bzrdir.py	2010-12-07 09:06:39 +0000
+++ bzrlib/bzrdir.py	2010-12-15 00:41:26 +0000
@@ -1027,8 +1027,11 @@
             tree.clone(result)
         return result
 
-    def create_branch(self, name=None):
+    def create_branch(self, name=None, repository=None):
         """See BzrDir.create_branch."""
+        if repository is not None:
+            raise NotImplementedError(
+                "create_branch(repository=<not None>) on %r" % (self,))
         return self._format.get_branch_format().initialize(self, name=name)
 
     def destroy_branch(self, name=None):
@@ -1264,9 +1267,10 @@
         """See BzrDir.can_convert_format()."""
         return True
 
-    def create_branch(self, name=None):
+    def create_branch(self, name=None, repository=None):
         """See BzrDir.create_branch."""
-        return self._format.get_branch_format().initialize(self, name=name)
+        return self._format.get_branch_format().initialize(self, name=name,
+                repository=repository)
 
     def destroy_branch(self, name=None):
         """See BzrDir.create_branch."""

=== modified file 'bzrlib/controldir.py'
--- bzrlib/controldir.py	2010-09-10 09:46:15 +0000
+++ bzrlib/controldir.py	2010-12-15 00:41:26 +0000
@@ -27,11 +27,10 @@
 import textwrap
 
 from bzrlib import (
+    cleanup,
     errors,
     graph,
-    registry,
     revision as _mod_revision,
-    symbol_versioning,
     urlutils,
     )
 from bzrlib.push import (
@@ -47,6 +46,8 @@
 
 """)
 
+from bzrlib import registry
+
 
 class ControlComponent(object):
     """Abstract base class for control directory components.
@@ -143,7 +144,7 @@
         """Destroy the repository in this ControlDir."""
         raise NotImplementedError(self.destroy_repository)
 
-    def create_branch(self, name=None):
+    def create_branch(self, name=None, repository=None):
         """Create a branch in this ControlDir.
 
         :param name: Name of the colocated branch to create, None for
@@ -364,6 +365,19 @@
         :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
         target_transport = get_transport(url, possible_transports)
         target_transport.ensure_base()
         cloning_format = self.cloning_metadir(stacked)
@@ -373,6 +387,7 @@
         # even if the origin was stacked
         stacked_branch_url = None
         if source_branch is not None:
+            add_cleanup(source_branch.lock_read().unlock)
             if stacked:
                 stacked_branch_url = self.root_transport.base
             source_repository = source_branch.repository
@@ -388,9 +403,14 @@
                     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)
         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)
         is_stacked = stacked or (len(result_repo._fallback_repositories) != 0)
         if is_new_repo and revision_id is not None and not is_stacked:
             fetch_spec = graph.PendingAncestryResult(
@@ -412,7 +432,8 @@
             result_branch = result.create_branch()
         else:
             result_branch = source_branch.sprout(result,
-                revision_id=revision_id, repository_policy=repository_policy)
+                revision_id=revision_id, repository_policy=repository_policy,
+                repository=result_repo)
         mutter("created new branch %r" % (result_branch,))
 
         # Create/update the result working tree
@@ -420,7 +441,7 @@
             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)
+                hardlink=hardlink, from_branch=result_branch)
             wt.lock_write()
             try:
                 if wt.path2id('') is None:

=== modified file 'bzrlib/remote.py'
--- bzrlib/remote.py	2010-12-02 10:41:05 +0000
+++ bzrlib/remote.py	2010-12-15 00:41:26 +0000
@@ -33,6 +33,7 @@
     revision as _mod_revision,
     static_tuple,
     symbol_versioning,
+    urlutils,
 )
 from bzrlib.branch import BranchReferenceFormat, BranchWriteLockResult
 from bzrlib.bzrdir import BzrDir, RemoteBzrDirFormat
@@ -246,14 +247,17 @@
         self._ensure_real()
         self._real_bzrdir.destroy_repository()
 
-    def create_branch(self, name=None):
+    def create_branch(self, name=None, repository=None):
         # as per meta1 formats - just delegate to the format object which may
         # be parameterised.
         real_branch = self._format.get_branch_format().initialize(self,
-            name=name)
+            name=name, repository=repository)
         if not isinstance(real_branch, RemoteBranch):
-            result = RemoteBranch(self, self.find_repository(), real_branch,
-                                  name=name)
+            if not isinstance(repository, RemoteRepository):
+                raise AssertionError(
+                    'need a RemoteRepository to use with RemoteBranch, got %r'
+                    % (repository,))
+            result = RemoteBranch(self, repository, real_branch, name=name)
         else:
             result = real_branch
         # BzrDir.clone_on_transport() uses the result of create_branch but does
@@ -2093,7 +2097,7 @@
                                   name=name)
         return result
 
-    def initialize(self, a_bzrdir, name=None):
+    def initialize(self, a_bzrdir, name=None, repository=None):
         # 1) get the network name to use.
         if self._custom_format:
             network_name = self._custom_format.network_name()
@@ -2127,13 +2131,25 @@
         # Turn the response into a RemoteRepository object.
         format = RemoteBranchFormat(network_name=response[1])
         repo_format = response_tuple_to_repo_format(response[3:])
-        if response[2] == '':
-            repo_bzrdir = a_bzrdir
+        repo_path = response[2]
+        if repository is not None:
+            remote_repo_url = urlutils.join(medium.base, repo_path)
+            url_diff = urlutils.relative_url(repository.user_url,
+                    remote_repo_url)
+            if url_diff != '.':
+                raise AssertionError(
+                    'repository.user_url %r does not match URL from server '
+                    'response (%r + %r)'
+                    % (repository.user_url, medium.base, repo_path))
+            remote_repo = repository
         else:
-            repo_bzrdir = RemoteBzrDir(
-                a_bzrdir.root_transport.clone(response[2]), a_bzrdir._format,
-                a_bzrdir._client)
-        remote_repo = RemoteRepository(repo_bzrdir, repo_format)
+            if repo_path == '':
+                repo_bzrdir = a_bzrdir
+            else:
+                repo_bzrdir = RemoteBzrDir(
+                    a_bzrdir.root_transport.clone(repo_path), a_bzrdir._format,
+                    a_bzrdir._client)
+            remote_repo = RemoteRepository(repo_bzrdir, repo_format)
         remote_branch = RemoteBranch(a_bzrdir, remote_repo,
             format=format, setup_stacking=False, name=name)
         # XXX: We know this is a new branch, so it must have revno 0, revid

=== modified file 'bzrlib/tests/blackbox/test_branch.py'
--- bzrlib/tests/blackbox/test_branch.py	2010-12-02 10:41:05 +0000
+++ bzrlib/tests/blackbox/test_branch.py	2010-12-15 00:41:26 +0000
@@ -414,7 +414,7 @@
         # being too low. If rpc_count increases, more network roundtrips have
         # become necessary for this use case. Please do not adjust this number
         # upwards without agreement from bzr's network support maintainers.
-        self.assertLength(37, self.hpss_calls)
+        self.assertLength(36, self.hpss_calls)
 
     def test_branch_from_trivial_branch_streaming_acceptance(self):
         self.setup_smart_server_with_call_log()

=== modified file 'bzrlib/tests/test_branch.py'
--- bzrlib/tests/test_branch.py	2010-12-07 09:06:39 +0000
+++ bzrlib/tests/test_branch.py	2010-12-15 00:41:26 +0000
@@ -113,7 +113,7 @@
         """See BzrBranchFormat.get_format_string()."""
         return "Sample branch format."
 
-    def initialize(self, a_bzrdir, name=None):
+    def initialize(self, a_bzrdir, name=None, repository=None):
         """Format 4 branches cannot be created."""
         t = a_bzrdir.get_branch_transport(self, name=name)
         t.put_bytes('format', self.get_format_string())

=== modified file 'bzrlib/tests/test_bzrdir.py'
--- bzrlib/tests/test_bzrdir.py	2010-10-15 14:21:03 +0000
+++ bzrlib/tests/test_bzrdir.py	2010-12-15 00:41:26 +0000
@@ -29,6 +29,7 @@
     controldir,
     errors,
     help_topics,
+    lock,
     repository,
     osutils,
     remote,
@@ -1349,6 +1350,12 @@
     def set_parent(self, parent):
         self._parent = parent
 
+    def lock_read(self):
+        return lock.LogicalLockResult(self.unlock)
+
+    def unlock(self):
+        return
+
 
 class TestBzrDirSprout(TestCaseWithMemoryTransport):
 

=== modified file 'bzrlib/tests/test_foreign.py'
--- bzrlib/tests/test_foreign.py	2010-08-02 18:36:31 +0000
+++ bzrlib/tests/test_foreign.py	2010-12-15 00:41:26 +0000
@@ -173,17 +173,19 @@
         super(DummyForeignVcsBranchFormat, self).__init__()
         self._matchingbzrdir = DummyForeignVcsDirFormat()
 
-    def open(self, a_bzrdir, name=None, _found=False):
+    def open(self, a_bzrdir, name=None, _found=False, found_repository=None):
         if not _found:
             raise NotImplementedError
         try:
             transport = a_bzrdir.get_branch_transport(None, name=name)
             control_files = lockable_files.LockableFiles(transport, 'lock',
                                                          lockdir.LockDir)
+            if found_repository is None:
+                found_repository = a_bzrdir.find_repository()
             return DummyForeignVcsBranch(_format=self,
                               _control_files=control_files,
                               a_bzrdir=a_bzrdir,
-                              _repository=a_bzrdir.find_repository())
+                              _repository=found_repository)
         except errors.NoSuchFile:
             raise errors.NotBranchError(path=transport.base)
 

=== modified file 'doc/en/release-notes/bzr-2.3.txt'
--- doc/en/release-notes/bzr-2.3.txt	2010-12-14 23:00:51 +0000
+++ doc/en/release-notes/bzr-2.3.txt	2010-12-15 00:41:26 +0000
@@ -31,6 +31,10 @@
   missing inventories.  This removes at least one network roundtrip when
   pushing to a stacked branch.  (Andrew Bennetts)
 
+* ``ControlDir.sprout`` no longer opens the target repository more than
+  once.  This avoids some unnecessary IO, and removes a network roundtrip
+  when doing ``bzr branch`` to a smart server URL.  (Andrew Bennetts)
+
 * ``bzr resolve`` now accepts ``--take-this`` and ``--take-other`` actions
   for text conflicts. This *replace* the whole file with the content
   designated by the action. This will *ignore* all differences that would
@@ -53,6 +57,14 @@
 .. Changes that may require updates in plugins or other code that uses
    bzrlib.
 
+* ``Branch.sprout``, ``BranchFormat.initalize`` and
+  ``ControlDir.create_branch`` now take an optional ``repository`` keyword
+  argument, and ``BranchFormat.open`` now takes an optional
+  ``found_repository`` keyword argument.  These provide the repository
+  object for new branch object to use (for cases when the caller has
+  already opened that repository).  Implementations of these APIs will
+  need to be updated to accept these arguments.  (Andrew Bennetts)
+
 Internals
 *********
 

=== modified file 'doc/en/whats-new/whats-new-in-2.3.txt'
--- doc/en/whats-new/whats-new-in-2.3.txt	2010-12-14 09:33:53 +0000
+++ doc/en/whats-new/whats-new-in-2.3.txt	2010-12-15 00:41:26 +0000
@@ -69,8 +69,9 @@
 * ``bzr send`` uses less memory.
   (John Arbash Meinel, #614576)
 
-* Fetches involving stacked branches and branches with tags are now faster.
-  Some redundant network reads were removed.  (Andrew Bennetts)
+* Fetches involving stacked branches and branches with tags now do slightly less
+  I/O, and so does branching from an existing branch.  This also improves the
+  network performance of these operations.  (Andrew Bennetts)
 
 * Inventory entries now consume less memory (on 32-bit Ubuntu file entries
   have dropped from 68 bytes to 40, and directory entries from 120 bytes