Merge lp:~vila/bzr/cached-branch-store into lp:bzr

[Vincent Ladeuil]

Cache the branch config store to avoid useless IOs.

This is it !

Well, this doesn't fix bug #832042 yet (the local config files still writes
the config file on every option change but the related fix will be 1) far
simpler than this one, 2) raises different but simpler issues. So I focus on
the branch config here).

Summary: Instead of writing the branch.conf config file on each
modification, it keeps track of the modifications and write the file when
the branch is unlocked (reading it again to guard against users of the old
config API).

I'm confident this proposal is technically correct both in the
implementation and in the implied code and test fixes introduced by the
change in locking. Yet, while these fixes are quite easy once you understand
the issues, it seems a bit risky to land it so late in the series
cycle. Something is missing for that and I'd like feedback about it.

Long story:

To avoid writing the config file for every branch option change, all the
changes are batched and the write happens at the last possible time.

Since this is a write operation on a branch, it requires a locked
branch. Therefore the write happens when the branch is unlocked.

I can't describe it more concisely but this doesn't clearly expose the
fallouts.

The real question here is: when should the branch be *locked* ?

There are two possible answers to that:

1 - before the first change is attempted (set() or remove()),

2 - before the config file needs to be saved.

In the actual implementation (and that's true for both the configobj based
implementation and the stack based one), *every* change takes a lock on the
branch *and* write the file (this is implemented in _CompatibleStack so it
applies to the local config files as well except they use a lockdir instead
of a branch of course).

Avoiding these writes means we keep the changes in memory. As such the
branch doesn't need to be locked which makes (2) compelling.

The main drawback is of course that these changes are not saved to disk and
the main consequence is that blackbox tests become broken: modifying an
option in a branch object is not enough to make it available to a subsequent
run_bzr() call. All config changes should now happen in an explicitely
locked branch.

Likewise, if the run_bzr() call modifies a config option, the config file
needs to be reloaded to see this change. This is a bit more suprising and
requires an additional explanation: the *old* design were reading the config
file for each option and as such was immune from this issue. The new design
caches the config file so a change introduced by a run_bzr() call is not
seen. This happens when a branch config option is migrated to the new design
but not for the other options. It really depends on how and when they are
used but all the cases I've encountered so far make sense, see
bb.test_bound_branches for example where calling run_bzr(unbind) requires
acquiring the branch (via its wt) *after* the run rather than before to get
the right behavior. The changes in test_bound_branches are required as soon
as 'bound_location' is using the new design *and* this proposal. Note that
some tests were already aware of similar issues: search for 're-open tree as
external run_bzr modified it' in bb.test_merge for such examples.

So, not a new issue but an almost unknown one that becomes far more common
with this proposal.

Back to the proposal, you'll see four kind of changes:

- implement the caching for the config store in Branch and RemoteBranch,
  this includes sharing the same store between a RemoteBranch and its
  related _real_branch (the core of this proposal),

- fixes to code so the option changes are saved to disk at the right time
  Converter5to6, Converter6to7 in branch.py, cmd_pull, cmd_init, cmd_whoami,
  cmd_merge_directive in builtins.py, cmd_config in config.py,
  controldir.ControlComponent.push_branch, cmd_dpush in foreign.py for
  examples. Special note for the infamous stacked_on_url that needs to be
  seen by both the client and the server in all cases.

- fixes blackbox tests that requires a locked branch for option changes to
  be saved on disk (the idiom is an added lock_write(branch)/try:set
  options/finally: unlock(). Suggestions welcome to make this easier to
  spell, a context manager thingy to do with locked(branch): ?

- fixes blackbox tests that requires a fresh object to observe option
  changes, the idiom is br = br.bzrdir.open_branch()

So the missing bit I alluded in the introduction is probably something along
the following lines:

- set()/remove() requires a lock and triggers a config file write for
  backward compatibility

- we fix the spurious writes above by locking the branch at a higher level
  when/where needed as I did here both in code and in tests.

- add a reload() method to stacks so blackbox tests can reload only the
  config stack instead of the whole object.

I deleted
bzrlib.tests.per_branch.test_branch.TestBound.test_unlocked_does_not_cache_master_branch
as the trick used in the test is unlikely to be used in real life and not
valid anymore anyway.

[Vincent Ladeuil (vila)]

Change set/remove to require a lock for the branch config files.

This means that tests (or any plugin for that matter) do not requires an
explicit lock on the branch anymore to change a single option. This also
means the optimisation becomes "opt-in" and as such won't be as
spectacular as it may be and/or harder to get right (nothing fails
anymore).

This reduces the diff by ~300 lines.

Code/tests that were updating more than one config option is still taking
a lock to at least avoid some IOs and demonstrate the benefits through
the decreased number of hpss calls.

The duplication between BranchStack and BranchOnlyStack will be removed
once the same sharing is in place for local config files, at which point
the Stack class itself may be able to host the changes.

If nothing else, once the same change has been made to the local config
files the BranchOnlyStack will be come useless since it's intended purpose
was more to avoid the IOs associated with the locl config files than
forbidding setting stacked_on_location in locl config files (while setting
stacked_on_location from a local file may sound weird, it still makes sense
to allow it on the server side (launchpad excluded given its implementation)
or even for expert users (including from the command line with -O) in
specific cases).

Overall removing the constraint on branch locking sounds less risky to land
especially at the beginning of the 2.6 cycle.

So, please review :)

I'll make a further proposal on top of this one for sharing the local config files.

[Vincent Ladeuil (vila)]

Forgot to mention: I didn't implement .reload() for stacks, not enough tests will benefit from that and that still needs to be fixed on a case by case basis (where/when it fails).

In the wild, there shouldn't be much cases of people using the new config API while relying on the config files to magically refresh themselves (or expecting to observe a side effect of a bzr command in a different process via a config option).

[Martin Packman (gz)]

Much nicer with the reduced lock/unlock requirement, and approach seems good overall.

One thing I'm not clear on is whether all the branch methods like Branch.set_submit_branch and other config writing helper methods also want the @needs_write_lock decorator. I guess as BranchStack mutating methods do that anyway so it's unimportant.

If a plugin author wants to reduce the amount of pointless config-writing in their code, how would they go about tracking down where they need to add locking?

Documenting in what's new and landing on 2.5 seems required to me.

review: Approve

[Vincent Ladeuil (vila)]

Thanks a lot for the review !

> Much nicer with the reduced lock/unlock requirement, and approach seems good
> overall.
>
> One thing I'm not clear on is whether all the branch methods like
> Branch.set_submit_branch and other config writing helper methods also want the
> @needs_write_lock decorator.

No, because ultimately they will call stack.set().

> I guess as BranchStack mutating methods do that
> anyway so it's unimportant.

Indeed.

>
> If a plugin author wants to reduce the amount of pointless config-writing in
> their code, how would they go about tracking down where they need to add
> locking?

That's the main loss of making the behavior "opt-in". There is no obvious failure anymore.

As plugins migrate to the new scheme we'll get more feedback ?

>
> Documenting in what's new and landing on 2.5 seems required to me.

I need to think a bit more about it (I'd like a full fix for bug #832042 first which I'm working on right now, mainly the local config stores should be cached too).

[Vincent Ladeuil (vila)]

Branch refreshed, will land to trunk

[Vincent Ladeuil (vila)]

Funnily enough, merging trunk made 3 tests fail. Cause ? Less hpss calls !

[Vincent Ladeuil (vila)]

sent to pqm by email

=== modified file 'bzrlib/branch.py'
--- bzrlib/branch.py	2012-01-27 14:56:06 +0000
+++ bzrlib/branch.py	2012-02-14 17:36:32 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005-2011 Canonical Ltd
+# Copyright (C) 2005-2012 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -2481,6 +2481,7 @@
         self.control_files = _control_files
         self._transport = _control_files._transport
         self.repository = _repository
+        self.conf_store = None
         Branch.__init__(self, possible_transports)
 
     def __str__(self):
@@ -2502,7 +2503,9 @@
         return _mod_config.TransportConfig(self._transport, 'branch.conf')
 
     def _get_config_store(self):
-        return _mod_config.BranchStore(self)
+        if self.conf_store is None:
+            self.conf_store =  _mod_config.BranchStore(self)
+        return self.conf_store
 
     def is_locked(self):
         return self.control_files.is_locked()
@@ -2557,6 +2560,8 @@
 
     @only_raises(errors.LockNotHeld, errors.LockBroken)
     def unlock(self):
+        if self.conf_store is not None:
+            self.conf_store.save_changes()
         try:
             self.control_files.unlock()
         finally:
@@ -3245,9 +3250,13 @@
 
         # Copy source data into target
         new_branch._write_last_revision_info(*branch.last_revision_info())
-        new_branch.set_parent(branch.get_parent())
-        new_branch.set_bound_location(branch.get_bound_location())
-        new_branch.set_push_location(branch.get_push_location())
+        new_branch.lock_write()
+        try:
+            new_branch.set_parent(branch.get_parent())
+            new_branch.set_bound_location(branch.get_bound_location())
+            new_branch.set_push_location(branch.get_push_location())
+        finally:
+            new_branch.unlock()
 
         # New branch has no tags by default
         new_branch.tags._set_tag_dict({})
@@ -3259,11 +3268,15 @@
 
         # Clean up old files
         new_branch._transport.delete('revision-history')
+        branch.lock_write()
         try:
-            branch.set_parent(None)
-        except errors.NoSuchFile:
-            pass
-        branch.set_bound_location(None)
+            try:
+                branch.set_parent(None)
+            except errors.NoSuchFile:
+                pass
+            branch.set_bound_location(None)
+        finally:
+            branch.unlock()
 
 
 class Converter6to7(object):

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2012-02-07 13:18:40 +0000
+++ bzrlib/builtins.py	2012-02-14 17:36:32 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005-2011 Canonical Ltd
+# Copyright (C) 2005-2012 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -1205,6 +1205,8 @@
             # Remembers if asked explicitly or no previous location is set
             if (remember
                 or (remember is None and branch_to.get_parent() is None)):
+                # FIXME: This shouldn't be done before the pull
+                # succeeds... -- vila 2012-01-02
                 branch_to.set_parent(branch_from.base)
 
         if revision is not None:
@@ -3861,7 +3863,9 @@
             if directory is None:
                 c = Branch.open_containing(u'.')[0].get_config_stack()
             else:
-                c = Branch.open(directory).get_config_stack()
+                b = Branch.open(directory)
+                self.add_cleanup(b.lock_write().unlock)
+                c = b.get_config_stack()
         else:
             c = _mod_config.GlobalStack()
         c.set('email', name)
@@ -5261,10 +5265,12 @@
             else:
                 if location is None:
                     if b.get_bound_location() is not None:
-                        raise errors.BzrCommandError(gettext('Branch is already bound'))
+                        raise errors.BzrCommandError(
+                            gettext('Branch is already bound'))
                     else:
-                        raise errors.BzrCommandError(gettext('No location supplied '
-                            'and no previous location known'))
+                        raise errors.BzrCommandError(
+                            gettext('No location supplied'
+                                    ' and no previous location known'))
         b_other = Branch.open(location)
         try:
             b.bind(b_other)
@@ -5680,6 +5686,7 @@
         if public_branch is None:
             public_branch = stored_public_branch
         elif stored_public_branch is None:
+            # FIXME: Should be done only if we succeed ? -- vila 2012-01-03
             branch.set_public_branch(public_branch)
         if not include_bundle and public_branch is None:
             raise errors.BzrCommandError(gettext('No public branch specified or'

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2012-02-07 00:49:58 +0000
+++ bzrlib/config.py	2012-02-14 17:36:32 +0000
@@ -3416,20 +3416,6 @@
         self.branch = branch
         self.id = 'branch'
 
-    def lock_write(self, token=None):
-        return self.branch.lock_write(token)
-
-    def unlock(self):
-        return self.branch.unlock()
-
-    @needs_write_lock
-    def save(self):
-        # We need to be able to override the undecorated implementation
-        self.save_without_locking()
-
-    def save_without_locking(self):
-        super(BranchStore, self).save()
-
 
 class ControlStore(LockableIniFileStore):
 
@@ -3939,7 +3925,7 @@
             lstore, mutable_section_id=location)
 
 
-class BranchStack(_CompatibleStack):
+class BranchStack(Stack):
     """Per-location options falling back to branch then global options stack.
 
     The following sections are queried:
@@ -3970,6 +3956,24 @@
             bstore)
         self.branch = branch
 
+    def lock_write(self, token=None):
+        return self.branch.lock_write(token)
+
+    def unlock(self):
+        return self.branch.unlock()
+
+    @needs_write_lock
+    def set(self, name, value):
+        super(BranchStack, self).set(name, value)
+        # Unlocking the branch will trigger a store.save_changes() so the last
+        # unlock saves all the changes.
+
+    @needs_write_lock
+    def remove(self, name):
+        super(BranchStack, self).remove(name)
+        # Unlocking the branch will trigger a store.save_changes() so the last
+        # unlock saves all the changes.
+
 
 class RemoteControlStack(_CompatibleStack):
     """Remote control-only options stack."""
@@ -3986,7 +3990,7 @@
         self.bzrdir = bzrdir
 
 
-class BranchOnlyStack(_CompatibleStack):
+class BranchOnlyStack(Stack):
     """Branch-only options stack."""
 
     # FIXME: _BranchOnlyStack only uses branch.conf and is used only for the
@@ -4000,6 +4004,24 @@
             bstore)
         self.branch = branch
 
+    def lock_write(self, token=None):
+        return self.branch.lock_write(token)
+
+    def unlock(self):
+        return self.branch.unlock()
+
+    @needs_write_lock
+    def set(self, name, value):
+        super(BranchOnlyStack, self).set(name, value)
+        # Force a write to persistent storage
+        self.store.save_changes()
+
+    @needs_write_lock
+    def remove(self, name):
+        super(BranchOnlyStack, self).remove(name)
+        # Force a write to persistent storage
+        self.store.save_changes()
+
 
 # Use a an empty dict to initialize an empty configobj avoiding all
 # parsing and encoding checks
@@ -4072,12 +4094,15 @@
                 # Set the option value
                 self._set_config_option(name, value, directory, scope)
 
-    def _get_stack(self, directory, scope=None):
+    def _get_stack(self, directory, scope=None, write_access=False):
         """Get the configuration stack specified by ``directory`` and ``scope``.
 
         :param directory: Where the configurations are derived from.
 
         :param scope: A specific config to start from.
+
+        :param write_access: Whether a write access to the stack will be
+            attempted.
         """
         # FIXME: scope should allow access to plugin-specific stacks (even
         # reduced to the plugin-specific store), related to
@@ -4091,6 +4116,8 @@
                 (_, br, _) = (
                     controldir.ControlDir.open_containing_tree_or_branch(
                         directory))
+                if write_access:
+                    self.add_cleanup(br.lock_write().unlock)
                 return br.get_config_stack()
             raise errors.NoSuchConfig(scope)
         else:
@@ -4098,6 +4125,8 @@
                 (_, br, _) = (
                     controldir.ControlDir.open_containing_tree_or_branch(
                         directory))
+                if write_access:
+                    self.add_cleanup(br.lock_write().unlock)
                 return br.get_config_stack()
             except errors.NotBranchError:
                 return LocationStack(directory)
@@ -4148,14 +4177,14 @@
                         self.outf.write('  %s = %s\n' % (oname, value))
 
     def _set_config_option(self, name, value, directory, scope):
-        conf = self._get_stack(directory, scope)
+        conf = self._get_stack(directory, scope, write_access=True)
         conf.set(name, value)
 
     def _remove_config_option(self, name, directory, scope):
         if name is None:
             raise errors.BzrCommandError(
                 '--remove expects an option to remove.')
-        conf = self._get_stack(directory, scope)
+        conf = self._get_stack(directory, scope, write_access=True)
         try:
             conf.remove(name)
         except KeyError:

=== modified file 'bzrlib/controldir.py'
--- bzrlib/controldir.py	2012-01-31 15:43:17 +0000
+++ bzrlib/controldir.py	2012-02-14 17:36:32 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2010, 2011 Canonical Ltd
+# Copyright (C) 2010, 2011, 2012 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -406,6 +406,7 @@
             repository_to.fetch(source.repository, revision_id=revision_id)
             br_to = source.clone(self, revision_id=revision_id)
             if source.get_push_location() is None or remember:
+                # FIXME: Should be done only if we succeed ? -- vila 2012-01-18
                 source.set_push_location(br_to.base)
             push_result.stacked_on = None
             push_result.branch_push_result = None
@@ -417,6 +418,7 @@
         else:
             # We have successfully opened the branch, remember if necessary:
             if source.get_push_location() is None or remember:
+                # FIXME: Should be done only if we succeed ? -- vila 2012-01-18
                 source.set_push_location(br_to.base)
             try:
                 tree_to = self.open_workingtree()

=== modified file 'bzrlib/foreign.py'
--- bzrlib/foreign.py	2011-12-19 13:23:58 +0000
+++ bzrlib/foreign.py	2012-02-14 17:36:32 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2008-2011 Canonical Ltd
+# Copyright (C) 2008-2012 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -323,6 +323,7 @@
                     source_branch, target_branch))
             # We successfully created the target, remember it
             if source_branch.get_push_location() is None or remember:
+                # FIXME: Should be done only if we succeed ? -- vila 2012-01-18
                 source_branch.set_push_location(target_branch.base)
             if not no_rebase:
                 old_last_revid = source_branch.last_revision()

=== modified file 'bzrlib/plugins/launchpad/test_lp_open.py'
--- bzrlib/plugins/launchpad/test_lp_open.py	2010-12-02 09:23:10 +0000
+++ bzrlib/plugins/launchpad/test_lp_open.py	2012-02-14 17:36:32 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2009, 2010 Canonical Ltd
+# Copyright (C) 2009, 2010, 2012 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -51,8 +51,7 @@
 
     def test_launchpad_branch_with_public_location(self):
         branch = self.make_branch('lp')
-        branch.set_public_branch(
-            'bzr+ssh://bazaar.launchpad.net/~foo/bar/baz')
+        branch.set_public_branch('bzr+ssh://bazaar.launchpad.net/~foo/bar/baz')
         self.assertEqual(
             ['Opening https://code.launchpad.net/~foo/bar/baz in web '
              'browser'],
@@ -60,10 +59,14 @@
 
     def test_launchpad_branch_with_public_and_push_location(self):
         branch = self.make_branch('lp')
-        branch.set_public_branch(
-            'bzr+ssh://bazaar.launchpad.net/~foo/bar/public')
-        branch.set_push_location(
-            'bzr+ssh://bazaar.launchpad.net/~foo/bar/push')
+        branch.lock_write()
+        try:
+            branch.set_public_branch(
+                'bzr+ssh://bazaar.launchpad.net/~foo/bar/public')
+            branch.set_push_location(
+                'bzr+ssh://bazaar.launchpad.net/~foo/bar/push')
+        finally:
+            branch.unlock()
         self.assertEqual(
             ['Opening https://code.launchpad.net/~foo/bar/public in web '
              'browser'],
@@ -73,8 +76,7 @@
         # lp-open falls back to the push location if it cannot find a public
         # location.
         branch = self.make_branch('lp')
-        branch.set_push_location(
-            'bzr+ssh://bazaar.launchpad.net/~foo/bar/baz')
+        branch.set_push_location('bzr+ssh://bazaar.launchpad.net/~foo/bar/baz')
         self.assertEqual(
             ['Opening https://code.launchpad.net/~foo/bar/baz in web '
              'browser'],

=== modified file 'bzrlib/push.py'
--- bzrlib/push.py	2012-01-19 17:23:00 +0000
+++ bzrlib/push.py	2012-02-14 17:36:32 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2008, 2009, 2010 Canonical Ltd
+# Copyright (C) 2008-2012 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -135,6 +135,7 @@
         # Remembers if asked explicitly or no previous location is set
         if (remember
             or (remember is None and br_from.get_push_location() is None)):
+            # FIXME: Should be done only if we succeed ? -- vila 2012-01-18
             br_from.set_push_location(br_to.base)
     else:
         if stacked_on is not None:

=== modified file 'bzrlib/remote.py'
--- bzrlib/remote.py	2012-01-28 00:56:56 +0000
+++ bzrlib/remote.py	2012-02-14 17:36:32 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006-2011 Canonical Ltd
+# Copyright (C) 2006-2012 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -3262,20 +3262,6 @@
         self.id = "branch"
         self._real_store = None
 
-    def lock_write(self, token=None):
-        return self.branch.lock_write(token)
-
-    def unlock(self):
-        return self.branch.unlock()
-
-    @needs_write_lock
-    def save(self):
-        # We need to be able to override the undecorated implementation
-        self.save_without_locking()
-
-    def save_without_locking(self):
-        super(RemoteBranchStore, self).save()
-
     def external_url(self):
         return self.branch.user_url
 
@@ -3366,6 +3352,7 @@
         self._repo_lock_token = None
         self._lock_count = 0
         self._leave_lock = False
+        self.conf_store = None
         # Setup a format: note that we cannot call _ensure_real until all the
         # attributes above are set: This code cannot be moved higher up in this
         # function.
@@ -3414,7 +3401,9 @@
         return RemoteBranchConfig(self)
 
     def _get_config_store(self):
-        return RemoteBranchStore(self)
+        if self.conf_store is None:
+            self.conf_store =  RemoteBranchStore(self)
+        return self.conf_store
 
     def _get_real_transport(self):
         # if we try vfs access, return the real branch's vfs transport
@@ -3440,6 +3429,10 @@
             self.bzrdir._ensure_real()
             self._real_branch = self.bzrdir._real_bzrdir.open_branch(
                 ignore_fallbacks=self._real_ignore_fallbacks, name=self._name)
+            # The remote branch and the real branch shares the same store. If
+            # we don't, there will always be cases where one of the stores
+            # doesn't see an update made on the other.
+            self._real_branch.conf_store = self.conf_store
             if self.repository._real_repository is None:
                 # Give the remote repository the matching real repo.
                 real_repo = self._real_branch.repository
@@ -3523,6 +3516,13 @@
 
     def set_stacked_on_url(self, url):
         branch.Branch.set_stacked_on_url(self, url)
+        # We need the stacked_on_url to be visible both locally (to not query
+        # it repeatedly) and remotely (so smart verbs can get it server side)
+        # Without the following line,
+        # bzrlib.tests.per_branch.test_create_clone.TestCreateClone
+        # .test_create_clone_on_transport_stacked_hooks_get_stacked_branch
+        # fails for remote branches -- vila 2012-01-04
+        self.conf_store.save_changes()
         if not url:
             self._is_stacked = False
         else:
@@ -3656,6 +3656,8 @@
         try:
             self._lock_count -= 1
             if not self._lock_count:
+                if self.conf_store is not None:
+                    self.conf_store.save_changes()
                 self._clear_cached_state()
                 mode = self._lock_mode
                 self._lock_mode = None
@@ -3951,8 +3953,7 @@
             last_rev=last_rev,other_branch=other_branch))
 
     def set_push_location(self, location):
-        self._ensure_real()
-        return self._real_branch.set_push_location(location)
+        self._set_config_location('push_location', location)
 
     def heads_to_fetch(self):
         if self._format._use_default_local_heads_to_fetch():

=== modified file 'bzrlib/switch.py'
--- bzrlib/switch.py	2012-01-26 15:31:02 +0000
+++ bzrlib/switch.py	2012-02-14 17:36:32 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2007, 2009, 2010 Canonical Ltd.
+# Copyright (C) 2007, 2009-2012 Canonical Ltd.
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -105,11 +105,15 @@
                         'Unable to connect to current master branch %(target)s: '
                         '%(error)s To switch anyway, use --force.') %
                         e.__dict__)
-            b.set_bound_location(None)
-            b.pull(to_branch, overwrite=True,
-                possible_transports=possible_transports)
-            b.set_bound_location(to_branch.base)
-            b.set_parent(b.get_master_branch().get_parent())
+            b.lock_write()
+            try:
+                b.set_bound_location(None)
+                b.pull(to_branch, overwrite=True,
+                       possible_transports=possible_transports)
+                b.set_bound_location(to_branch.base)
+                b.set_parent(b.get_master_branch().get_parent())
+            finally:
+                b.unlock()
         else:
             # If this is a standalone tree and the new branch
             # is derived from this one, create a lightweight checkout.

=== modified file 'bzrlib/tests/blackbox/test_bound_branches.py'
--- bzrlib/tests/blackbox/test_bound_branches.py	2012-01-05 13:02:31 +0000
+++ bzrlib/tests/blackbox/test_bound_branches.py	2012-02-14 17:36:32 +0000
@@ -127,12 +127,13 @@
 
     def test_double_binding(self):
         child_tree = self.create_branches()[1]
-
-        child2_tree = child_tree.bzrdir.sprout('child2').open_workingtree()
+        child_tree.bzrdir.sprout('child2')
 
         # Double binding succeeds, but committing to child2 should fail
         self.run_bzr('bind ../child', working_dir='child2')
 
+        # Refresh the child tree object as 'unbind' modified it
+        child2_tree = bzrdir.BzrDir.open('child2').open_workingtree()
         self.assertRaises(errors.CommitToDoubleBoundBranch,
                 child2_tree.commit, message='child2', allow_pointless=True)
 
@@ -150,6 +151,8 @@
         self.run_bzr("commit -m child", retcode=3, working_dir='child')
         self.check_revno(1, 'child')
         self.run_bzr('unbind', working_dir='child')
+        # Refresh the child tree/branch objects as 'unbind' modified them
+        child_tree = child_tree.bzrdir.open_workingtree()
         child_tree.commit(message='child')
         self.check_revno(2, 'child')
 
@@ -200,6 +203,8 @@
 
         self.run_bzr('unbind', working_dir='child')
 
+        # Refresh the child tree/branch objects as 'unbind' modified them
+        child_tree = child_tree.bzrdir.open_workingtree()
         child_tree.commit(message='child', allow_pointless=True)
         self.check_revno(2, 'child')
 
@@ -210,6 +215,8 @@
         # These branches have diverged, but bind should succeed anyway
         self.run_bzr('bind ../base', working_dir='child')
 
+        # Refresh the child tree/branch objects as 'bind' modified them
+        child_tree = child_tree.bzrdir.open_workingtree()
         # This should turn the local commit into a merge
         child_tree.update()
         child_tree.commit(message='merged')
@@ -248,6 +255,8 @@
         child_tree = self.create_branches()[1]
 
         self.run_bzr('unbind', working_dir='child')
+        # Refresh the child tree/branch objects as 'bind' modified them
+        child_tree = child_tree.bzrdir.open_workingtree()
         child_tree.commit(message='child', allow_pointless=True)
         self.check_revno(2, 'child')
         self.check_revno(1, 'base')

=== modified file 'bzrlib/tests/blackbox/test_checkout.py'
--- bzrlib/tests/blackbox/test_checkout.py	2012-01-20 21:33:24 +0000
+++ bzrlib/tests/blackbox/test_checkout.py	2012-02-14 17:36:32 +0000
@@ -223,5 +223,5 @@
         # 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(15, self.hpss_calls)
+        self.assertLength(13, self.hpss_calls)
         self.assertThat(self.hpss_calls, ContainsNoVfsCalls)

=== modified file 'bzrlib/tests/blackbox/test_commit.py'
--- bzrlib/tests/blackbox/test_commit.py	2012-02-07 00:49:58 +0000
+++ bzrlib/tests/blackbox/test_commit.py	2012-02-14 17:36:32 +0000
@@ -887,7 +887,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(214, self.hpss_calls)
+        self.assertLength(211, self.hpss_calls)
         self.assertLength(2, self.hpss_connections)
         self.expectFailure("commit still uses VFS calls",
             self.assertThat, self.hpss_calls, ContainsNoVfsCalls)

=== modified file 'bzrlib/tests/blackbox/test_dpush.py'
--- bzrlib/tests/blackbox/test_dpush.py	2011-09-22 12:13:12 +0000
+++ bzrlib/tests/blackbox/test_dpush.py	2012-02-14 17:36:32 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2009, 2010, 2011 Canonical Ltd
+# Copyright (C) 2009-2012 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -19,6 +19,7 @@
 
 
 from bzrlib import (
+    branch,
     tests,
     )
 from bzrlib.tests import (
@@ -144,10 +145,8 @@
             'to', format=test_foreign.DummyForeignVcsDirFormat())
 
     def set_config_push_strict(self, value):
-        # set config var (any of bazaar.conf, locations.conf, branch.conf
-        # should do)
-        conf = self.tree.branch.get_config_stack()
-        conf.set('dpush_strict', value)
+        br = branch.Branch.open('local')
+        br.get_config_stack().set('dpush_strict', value)
 
     _default_command = ['dpush', '../to']
 

=== modified file 'bzrlib/tests/blackbox/test_info.py'
--- bzrlib/tests/blackbox/test_info.py	2012-01-26 15:36:39 +0000
+++ bzrlib/tests/blackbox/test_info.py	2012-02-14 17:36:32 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006-2010 Canonical Ltd
+# Copyright (C) 2006-2012 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -1548,7 +1548,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(12, self.hpss_calls)
+        self.assertLength(10, self.hpss_calls)
         self.assertLength(1, self.hpss_connections)
         self.assertThat(self.hpss_calls, ContainsNoVfsCalls)
 
@@ -1565,6 +1565,6 @@
         # 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(16, self.hpss_calls)
+        self.assertLength(14, self.hpss_calls)
         self.assertLength(1, self.hpss_connections)
         self.assertThat(self.hpss_calls, ContainsNoVfsCalls)

=== modified file 'bzrlib/tests/blackbox/test_log.py'
--- bzrlib/tests/blackbox/test_log.py	2011-12-21 14:50:43 +0000
+++ bzrlib/tests/blackbox/test_log.py	2012-02-14 17:36:32 +0000
@@ -1096,7 +1096,7 @@
         # upwards without agreement from bzr's network support maintainers.
         self.assertThat(self.hpss_calls, ContainsNoVfsCalls)
         self.assertLength(1, self.hpss_connections)
-        self.assertLength(10, self.hpss_calls)
+        self.assertLength(9, self.hpss_calls)
 
     def test_verbose_log(self):
         self.setup_smart_server_with_call_log()
@@ -1111,7 +1111,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(11, self.hpss_calls)
+        self.assertLength(10, self.hpss_calls)
         self.assertLength(1, self.hpss_connections)
         self.assertThat(self.hpss_calls, ContainsNoVfsCalls)
 
@@ -1128,6 +1128,6 @@
         # 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(15, self.hpss_calls)
+        self.assertLength(14, self.hpss_calls)
         self.assertLength(1, self.hpss_connections)
         self.assertThat(self.hpss_calls, ContainsNoVfsCalls)

=== modified file 'bzrlib/tests/blackbox/test_merge.py'
--- bzrlib/tests/blackbox/test_merge.py	2012-01-05 13:02:31 +0000
+++ bzrlib/tests/blackbox/test_merge.py	2012-02-14 17:36:32 +0000
@@ -278,6 +278,8 @@
 
         base = urlutils.local_path_from_url(branch_a.base)
         self.assertEndsWith(err, '+N  b\nAll changes applied successfully.\n')
+        # re-open branch as external run_bzr modified it
+        branch_b = branch_b.bzrdir.open_branch()
         self.assertEquals(osutils.abspath(branch_b.get_submit_branch()),
                           osutils.abspath(parent))
         # test implicit --remember when committing new file
@@ -295,6 +297,8 @@
                                 working_dir='branch_b')
         self.assertEquals(out, '')
         self.assertEquals(err, '+N  c\nAll changes applied successfully.\n')
+        # re-open branch as external run_bzr modified it
+        branch_b = branch_b.bzrdir.open_branch()
         self.assertEquals(osutils.abspath(branch_b.get_submit_branch()),
                           osutils.abspath(branch_c.bzrdir.root_transport.base))
         # re-open tree as external run_bzr modified it
@@ -553,10 +557,16 @@
         tree_b = tree_a.bzrdir.sprout('b').open_workingtree()
         tree_c = tree_a.bzrdir.sprout('c').open_workingtree()
         out, err = self.run_bzr(['merge', '-d', 'c'])
-        self.assertContainsRe(err, 'Merging from remembered parent location .*a\/')
-        tree_c.branch.set_submit_branch(tree_b.bzrdir.root_transport.base)
+        self.assertContainsRe(err,
+                              'Merging from remembered parent location .*a\/')
+        tree_c.branch.lock_write()
+        try:
+            tree_c.branch.set_submit_branch(tree_b.bzrdir.root_transport.base)
+        finally:
+            tree_c.branch.unlock()
         out, err = self.run_bzr(['merge', '-d', 'c'])
-        self.assertContainsRe(err, 'Merging from remembered submit location .*b\/')
+        self.assertContainsRe(err,
+                              'Merging from remembered submit location .*b\/')
 
     def test_remember_sets_submit(self):
         tree_a = self.make_branch_and_tree('a')
@@ -566,11 +576,13 @@
 
         # Remember should not happen if using default from parent
         out, err = self.run_bzr(['merge', '-d', 'b'])
-        self.assertIs(tree_b.branch.get_submit_branch(), None)
+        refreshed = workingtree.WorkingTree.open('b')
+        self.assertIs(refreshed.branch.get_submit_branch(), None)
 
         # Remember should happen if user supplies location
         out, err = self.run_bzr(['merge', '-d', 'b', 'a'])
-        self.assertEqual(tree_b.branch.get_submit_branch(),
+        refreshed = workingtree.WorkingTree.open('b')
+        self.assertEqual(refreshed.branch.get_submit_branch(),
                          tree_a.bzrdir.root_transport.base)
 
     def test_no_remember_dont_set_submit(self):

=== modified file 'bzrlib/tests/blackbox/test_merge_directive.py'
--- bzrlib/tests/blackbox/test_merge_directive.py	2012-01-05 17:46:44 +0000
+++ bzrlib/tests/blackbox/test_merge_directive.py	2012-02-14 17:36:32 +0000
@@ -223,7 +223,8 @@
 
     def test_mail_uses_config(self):
         tree1, tree2 = self.prepare_merge_directive()
-        tree1.branch.get_config_stack().set('smtp_server', 'bogushost')
+        br = tree1.branch
+        br.get_config_stack().set('smtp_server', 'bogushost')
         md_text, errr, connect_calls, sendmail_calls =\
             self.run_bzr_fakemail('merge-directive --mail-to'
                                   ' [e-mail] --plain ../tree2 .')

=== modified file 'bzrlib/tests/blackbox/test_pull.py'
--- bzrlib/tests/blackbox/test_pull.py	2012-01-05 17:46:44 +0000
+++ bzrlib/tests/blackbox/test_pull.py	2012-02-14 17:36:32 +0000
@@ -236,6 +236,7 @@
         tree_a.commit('commit b')
         # reset parent
         parent = branch_b.get_parent()
+        branch_b = branch.Branch.open('branch_b')
         branch_b.set_parent(None)
         self.assertEqual(None, branch_b.get_parent())
         # test pull for failure without parent set
@@ -252,16 +253,22 @@
                 ('','bzr: ERROR: These branches have diverged.'
                     ' Use the missing command to see how.\n'
                     'Use the merge command to reconcile them.\n'))
-        self.assertEqual(branch_b.get_parent(), parent)
+        tree_b = tree_b.bzrdir.open_workingtree()
+        branch_b = tree_b.branch
+        self.assertEqual(parent, branch_b.get_parent())
         # test implicit --remember after resolving previous failure
         uncommit.uncommit(branch=branch_b, tree=tree_b)
         t.delete('branch_b/d')
         self.run_bzr('pull', working_dir='branch_b')
+        # Refresh the branch object as 'pull' modified it
+        branch_b = branch_b.bzrdir.open_branch()
         self.assertEqual(branch_b.get_parent(), parent)
         # test explicit --remember
         self.run_bzr('pull ../branch_c --remember', working_dir='branch_b')
-        self.assertEqual(branch_b.get_parent(),
-                          branch_c.bzrdir.root_transport.base)
+        # Refresh the branch object as 'pull' modified it
+        branch_b = branch_b.bzrdir.open_branch()
+        self.assertEqual(branch_c.bzrdir.root_transport.base,
+                         branch_b.get_parent())
 
     def test_pull_bundle(self):
         from bzrlib.testament import Testament
@@ -360,8 +367,7 @@
     def test_pull_verbose_uses_default_log(self):
         tree = self.example_branch('source')
         target = self.make_branch_and_tree('target')
-        target_config = target.branch.get_config_stack()
-        target_config.set('log_format', 'short')
+        target.branch.get_config_stack().set('log_format', 'short')
         out = self.run_bzr('pull -v source -d target')[0]
         self.assertContainsRe(out, r'\n {4}1 .*\n {6}setup\n')
         self.assertNotContainsRe(

=== modified file 'bzrlib/tests/blackbox/test_push.py'
--- bzrlib/tests/blackbox/test_push.py	2011-12-24 10:10:59 +0000
+++ bzrlib/tests/blackbox/test_push.py	2012-02-14 17:36:32 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006-2011 Canonical Ltd
+# Copyright (C) 2006-2012 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -117,6 +117,8 @@
         self.assertEquals(out,
                 ('','bzr: ERROR: These branches have diverged.  '
                  'See "bzr help diverged-branches" for more information.\n'))
+        # Refresh the branch as 'push' modified it
+        branch_a = branch_a.bzrdir.open_branch()
         self.assertEquals(osutils.abspath(branch_a.get_push_location()),
                           osutils.abspath(branch_b.bzrdir.root_transport.base))
 
@@ -124,6 +126,8 @@
         uncommit.uncommit(branch=branch_b, tree=tree_b)
         transport.delete('branch_b/c')
         out, err = self.run_bzr('push', working_dir='branch_a')
+        # Refresh the branch as 'push' modified it
+        branch_a = branch_a.bzrdir.open_branch()
         path = branch_a.get_push_location()
         self.assertEqual(err,
                          'Using saved push location: %s\n'
@@ -134,6 +138,8 @@
                          branch_b.bzrdir.root_transport.base)
         # test explicit --remember
         self.run_bzr('push ../branch_c --remember', working_dir='branch_a')
+        # Refresh the branch as 'push' modified it
+        branch_a = branch_a.bzrdir.open_branch()
         self.assertEquals(branch_a.get_push_location(),
                           branch_c.bzrdir.root_transport.base)
 
@@ -176,11 +182,12 @@
         t.add('file')
         t.commit('commit 1')
         self.run_bzr('push -d tree pushed-to')
-        path = t.branch.get_push_location()
+        # Refresh the branch as 'push' modified it and get the push location
+        push_loc = t.branch.bzrdir.open_branch().get_push_location()
         out, err = self.run_bzr('push', working_dir="tree")
         self.assertEqual('Using saved push location: %s\n'
                          'No new revisions or tags to push.\n' %
-                         urlutils.local_path_from_url(path), err)
+                         urlutils.local_path_from_url(push_loc), err)
         out, err = self.run_bzr('push -q', working_dir="tree")
         self.assertEqual('', out)
         self.assertEqual('', err)
@@ -285,7 +292,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(14, self.hpss_calls)
+        self.assertLength(15, self.hpss_calls)
         self.assertLength(1, self.hpss_connections)
         self.assertThat(self.hpss_calls, ContainsNoVfsCalls)
         remote = branch.Branch.open('public')
@@ -506,7 +513,8 @@
         trunk_public = self.make_branch('public_trunk', format='1.9')
         trunk_public.pull(trunk_tree.branch)
         trunk_public_url = self.get_readonly_url('public_trunk')
-        trunk_tree.branch.set_public_branch(trunk_public_url)
+        br = trunk_tree.branch
+        br.set_public_branch(trunk_public_url)
         # now we do a stacked push, which should determine the public location
         # for us.
         out, err = self.run_bzr(['push', '--stacked',
@@ -703,10 +711,8 @@
         self.tree.commit('modify file', rev_id='modified')
 
     def set_config_push_strict(self, value):
-        # set config var (any of bazaar.conf, locations.conf, branch.conf
-        # should do)
-        conf = self.tree.branch.get_config_stack()
-        conf.set('push_strict', value)
+        br = branch.Branch.open('local')
+        br.get_config_stack().set('push_strict', value)
 
     _default_command = ['push', '../to']
     _default_wd = 'local'

=== modified file 'bzrlib/tests/blackbox/test_reconfigure.py'
--- bzrlib/tests/blackbox/test_reconfigure.py	2011-09-06 18:37:06 +0000
+++ bzrlib/tests/blackbox/test_reconfigure.py	2012-02-14 17:36:32 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2007, 2009 Canonical Ltd
+# Copyright (C) 2007-2012 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -242,20 +242,19 @@
         branch_2 = tree_2.branch
         # now reconfigure to be stacked
         out, err = self.run_bzr('reconfigure --stacked-on b1 b2')
-        self.assertContainsRe(out,
-            '^.*/b2/ is now stacked on ../b1\n$')
+        self.assertContainsRe(out, '^.*/b2/ is now stacked on ../b1\n$')
         self.assertEquals('', err)
         # can also give the absolute URL of the branch, and it gets stored 
         # as a relative path if possible
         out, err = self.run_bzr('reconfigure --stacked-on %s b2'
-            % (self.get_url('b1'),))
-        self.assertContainsRe(out,
-            '^.*/b2/ is now stacked on ../b1\n$')
+                                % (self.get_url('b1'),))
+        self.assertContainsRe(out, '^.*/b2/ is now stacked on ../b1\n$')
         self.assertEquals('', err)
+        # Refresh the branch as 'reconfigure' modified it
+        branch_2 = branch_2.bzrdir.open_branch()
         # It should be given a relative URL to the destination, if possible,
         # because that's most likely to work across different transports
-        self.assertEquals(branch_2.get_stacked_on_url(),
-            '../b1')
+        self.assertEquals('../b1', branch_2.get_stacked_on_url())
         # commit, and it should be stored into b2's repo
         self.build_tree_contents([('foo', 'new foo')])
         tree_2.commit('update foo')
@@ -264,8 +263,9 @@
         self.assertContainsRe(out,
             '^.*/b2/ is now not stacked\n$')
         self.assertEquals('', err)
-        self.assertRaises(errors.NotStacked,
-            branch_2.get_stacked_on_url)
+        # Refresh the branch as 'reconfigure' modified it
+        branch_2 = branch_2.bzrdir.open_branch()
+        self.assertRaises(errors.NotStacked, branch_2.get_stacked_on_url)
 
     # XXX: Needs a test for reconfiguring stacking and shape at the same time;
     # no branch at location; stacked-on is not a branch; quiet mode.

=== modified file 'bzrlib/tests/blackbox/test_send.py'
--- bzrlib/tests/blackbox/test_send.py	2012-02-01 19:55:27 +0000
+++ bzrlib/tests/blackbox/test_send.py	2012-02-14 17:36:32 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006-2011 Canonical Ltd
+# Copyright (C) 2006-2012 Canonical Ltd
 # Authors: Aaron Bentley
 #
 # This program is free software; you can redistribute it and/or modify
@@ -230,12 +230,13 @@
                             'send -f branch -o- --format=0.999')[0]
 
     def test_format_child_option(self):
-        parent_config = branch.Branch.open('parent').get_config_stack()
-        parent_config.set('child_submit_format', '4')
+        br = branch.Branch.open('parent')
+        conf = br.get_config_stack()
+        conf.set('child_submit_format', '4')
         md = self.get_MD([])
         self.assertIs(merge_directive.MergeDirective2, md.__class__)
 
-        parent_config.set('child_submit_format', '0.9')
+        conf.set('child_submit_format', '0.9')
         md = self.get_MD([])
         self.assertFormatIs('# Bazaar revision bundle v0.9', md)
 
@@ -243,7 +244,7 @@
         self.assertFormatIs('# Bazaar revision bundle v0.9', md)
         self.assertIs(merge_directive.MergeDirective, md.__class__)
 
-        parent_config.set('child_submit_format', '0.999')
+        conf.set('child_submit_format', '0.999')
         self.run_bzr_error(["No such send format '0.999'"],
                             'send -f branch -o-')[0]
 
@@ -293,10 +294,8 @@
     _default_additional_warning = 'Uncommitted changes will not be sent.'
 
     def set_config_send_strict(self, value):
-        # set config var (any of bazaar.conf, locations.conf, branch.conf
-        # should do)
-        conf = self.local_tree.branch.get_config_stack()
-        conf.set('send_strict', value)
+        br = branch.Branch.open('local')
+        br.get_config_stack().set('send_strict', value)
 
     def assertSendFails(self, args):
         out, err = self.run_send(args, rc=3, err_re=self._default_errors)
@@ -463,6 +462,6 @@
         # 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(9, self.hpss_calls)
+        self.assertLength(7, self.hpss_calls)
         self.assertLength(1, self.hpss_connections)
         self.assertThat(self.hpss_calls, ContainsNoVfsCalls)

=== modified file 'bzrlib/tests/blackbox/test_switch.py'
--- bzrlib/tests/blackbox/test_switch.py	2012-01-19 12:36:22 +0000
+++ bzrlib/tests/blackbox/test_switch.py	2012-02-14 17:36:32 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2007-2010 Canonical Ltd
+# Copyright (C) 2007-2012 Canonical Ltd
 # -*- coding: utf-8 -*-
 #
 # This program is free software; you can redistribute it and/or modify
@@ -153,8 +153,11 @@
         branchb_id = tree2.commit('bar')
         checkout = tree1.branch.create_checkout('heavyco/a', lightweight=False)
         self.run_bzr(['switch', 'branchb'], working_dir='heavyco/a')
+        # Refresh checkout as 'switch' modified it
+        checkout = checkout.bzrdir.open_workingtree()
         self.assertEqual(branchb_id, checkout.last_revision())
-        self.assertEqual(tree2.branch.base, checkout.branch.get_bound_location())
+        self.assertEqual(tree2.branch.base,
+                         checkout.branch.get_bound_location())
 
     def test_switch_finds_relative_unicode_branch(self):
         """Switch will find 'foo' relative to the branch the checkout is of."""

=== modified file 'bzrlib/tests/blackbox/test_whoami.py'
--- bzrlib/tests/blackbox/test_whoami.py	2012-01-05 13:43:16 +0000
+++ bzrlib/tests/blackbox/test_whoami.py	2012-02-14 17:36:32 +0000
@@ -19,6 +19,7 @@
 
 import bzrlib
 from bzrlib import (
+    branch,
     config,
     errors,
     tests,
@@ -50,11 +51,14 @@
         out = self.run_bzr("whoami --email 'foo <[e-mail]>'", 3)[0]
         self.assertEquals("", out)
 
+    def set_branch_email(self, b, email):
+        b.get_config_stack().set('email', email)
+
     def test_whoami_branch(self):
         """branch specific user identity works."""
         wt = self.make_branch_and_tree('.')
         b = bzrlib.branch.Branch.open('.')
-        b.get_config_stack().set('email', 'Branch Identity <[e-mail]>')
+        self.set_branch_email(b, 'Branch Identity <[e-mail]>')
         self.assertWhoAmI('Branch Identity <[e-mail]>')
         self.assertWhoAmI('[e-mail]', '--email')
 
@@ -79,8 +83,7 @@
         """
         wt = self.make_branch_and_tree('.')
         b = bzrlib.branch.Branch.open('.')
-        b.get_config_stack().set(
-            'email', u'Branch Identity \u20ac ' + '<[e-mail]>')
+        self.set_branch_email(b, u'Branch Identity \u20ac <[e-mail]>')
         self.assertWhoAmI('Branch Identity ? <[e-mail]>',
                           encoding='ascii')
         self.assertWhoAmI('[e-mail]', '--email',
@@ -100,20 +103,20 @@
         self.overrideEnv('EMAIL', None)
         self.overrideEnv('BZR_EMAIL', None)
         # Also, make sure that it's not inferred from mailname.
-        self.overrideAttr(config, '_auto_user_id',
-            lambda: (None, None))
+        self.overrideAttr(config, '_auto_user_id', lambda: (None, None))
         out, err = self.run_bzr(['whoami'], 3)
         self.assertContainsRe(err, 'Unable to determine your name')
 
     def test_whoami_directory(self):
         """Test --directory option."""
         wt = self.make_branch_and_tree('subdir')
-        c = wt.branch.get_config_stack()
-        c.set('email', 'Branch Identity <[e-mail]>')
+        self.set_branch_email(wt.branch, 'Branch Identity <[e-mail]>')
         self.assertWhoAmI('Branch Identity <[e-mail]>',
                           '--directory', 'subdir')
         self.run_bzr(['whoami', '--directory', 'subdir', '--branch',
                       'Changed Identity <[e-mail]>'])
+        # Refresh wt as 'whoami' modified it
+        wt = wt.bzrdir.open_workingtree()
         c = wt.branch.get_config_stack()
         self.assertEquals('Changed Identity <[e-mail]>',
                           c.get('email'))
@@ -121,8 +124,7 @@
     def test_whoami_remote_directory(self):
         """Test --directory option with a remote directory."""
         wt = self.make_branch_and_tree('subdir')
-        c = wt.branch.get_config_stack()
-        c.set('email', 'Branch Identity <[e-mail]>')
+        self.set_branch_email(wt.branch, 'Branch Identity <[e-mail]>')
         url = self.get_readonly_url() + '/subdir'
         self.assertWhoAmI('Branch Identity <[e-mail]>',
                           '--directory', url)
@@ -131,7 +133,7 @@
                       'Changed Identity <[e-mail]>'])
         # The identity has been set in the branch config (but not the global
         # config)
-        c = wt.branch.get_config_stack()
+        c = branch.Branch.open(url).get_config_stack()
         self.assertEquals('Changed Identity <[e-mail]>',
                           c.get('email'))
         # Ensuring that the value does not come from the bazaar.conf file

=== modified file 'bzrlib/tests/per_branch/test_branch.py'
--- bzrlib/tests/per_branch/test_branch.py	2012-02-06 11:36:02 +0000
+++ bzrlib/tests/per_branch/test_branch.py	2012-02-14 17:36:32 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005-2011 Canonical Ltd
+# Copyright (C) 2005-2012 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -779,22 +779,6 @@
         branch.unbind()
         self.assertEqual(None, branch.get_master_branch())
 
-    def test_unlocked_does_not_cache_master_branch(self):
-        """Unlocked branches do not cache the result of get_master_branch."""
-        master = self.make_branch('master')
-        branch1 = self.make_branch('branch')
-        try:
-            branch1.bind(master)
-        except errors.UpgradeRequired:
-            raise tests.TestNotApplicable('Format does not support binding')
-        # Open branch1 again
-        branch2 = branch1.bzrdir.open_branch()
-        self.assertNotEqual(None, branch1.get_master_branch())
-        # Unbind the branch via branch2.  branch1 isn't locked so will
-        # immediately return the new value for get_master_branch.
-        branch2.unbind()
-        self.assertEqual(None, branch1.get_master_branch())
-
     def test_bind_clears_cached_master_branch(self):
         """b.bind clears any cached value of b.get_master_branch."""
         master1 = self.make_branch('master1')

=== modified file 'bzrlib/tests/per_branch/test_break_lock.py'
--- bzrlib/tests/per_branch/test_break_lock.py	2011-09-30 13:35:45 +0000
+++ bzrlib/tests/per_branch/test_break_lock.py	2012-02-14 17:36:32 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006-2010 Canonical Ltd
+# Copyright (C) 2006-2012 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -86,7 +86,8 @@
         master.lock_write()
         ui.ui_factory = ui.CannedInputUIFactory([True, True])
         try:
-            self.unused_branch.break_lock()
+            fresh = _mod_branch.Branch.open(self.unused_branch.base)
+            fresh.break_lock()
         except NotImplementedError:
             # branch does not support break_lock
             master.unlock()

=== modified file 'bzrlib/tests/per_branch/test_create_clone.py'
--- bzrlib/tests/per_branch/test_create_clone.py	2011-10-04 13:29:25 +0000
+++ bzrlib/tests/per_branch/test_create_clone.py	2012-02-14 17:36:32 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2009, 2010 Canonical Ltd
+# Copyright (C) 2009-2012 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -133,7 +133,6 @@
         trunk = tree.branch.create_clone_on_transport(
             self.get_transport('trunk'))
         revid = tree.commit('a second commit')
-        source = tree.branch
         target_transport = self.get_transport('target')
         self.hook_calls = []
         branch.Branch.hooks.install_named_hook(

=== modified file 'bzrlib/tests/test_branch.py'
--- bzrlib/tests/test_branch.py	2012-01-18 17:47:06 +0000
+++ bzrlib/tests/test_branch.py	2012-02-14 17:36:32 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006-2011 Canonical Ltd
+# Copyright (C) 2006-2012 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -356,8 +356,8 @@
         self.assertPathDoesNotExist('a/.bzr/branch/parent')
         self.assertEqual('http://example.com', branch.get_parent())
         branch.set_push_location('sftp://example.com')
-        config = branch.get_config_stack()
-        self.assertEqual('sftp://example.com', config.get('push_location'))
+        conf = branch.get_config_stack()
+        self.assertEqual('sftp://example.com', conf.get('push_location'))
         branch.set_bound_location('ftp://example.com')
         self.assertPathDoesNotExist('a/.bzr/branch/bound')
         self.assertEqual('ftp://example.com', branch.get_bound_location())

=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2012-02-03 10:28:47 +0000
+++ bzrlib/tests/test_config.py	2012-02-14 17:36:32 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005-2011 Canonical Ltd
+# Copyright (C) 2005-2012 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -2924,6 +2924,11 @@
     def test_save_emptied_succeeds(self):
         store = self.get_store(self)
         store._load_from_string('foo=bar\n')
+        # FIXME: There should be a better way than relying on the test
+        # parametrization to identify branch.conf -- vila 2011-0526
+        if self.store_id in ('branch', 'remote_branch'):
+            # branch stores requires write locked branches
+            self.addCleanup(store.branch.lock_write().unlock)
         section = store.get_mutable_section(None)
         section.remove('foo')
         store.save()
@@ -2950,6 +2955,11 @@
 
     def test_set_option_in_empty_store(self):
         store = self.get_store(self)
+        # FIXME: There should be a better way than relying on the test
+        # parametrization to identify branch.conf -- vila 2011-0526
+        if self.store_id in ('branch', 'remote_branch'):
+            # branch stores requires write locked branches
+            self.addCleanup(store.branch.lock_write().unlock)
         section = store.get_mutable_section(None)
         section.set('foo', 'bar')
         store.save()
@@ -2961,6 +2971,11 @@
     def test_set_option_in_default_section(self):
         store = self.get_store(self)
         store._load_from_string('')
+        # FIXME: There should be a better way than relying on the test
+        # parametrization to identify branch.conf -- vila 2011-0526
+        if self.store_id in ('branch', 'remote_branch'):
+            # branch stores requires write locked branches
+            self.addCleanup(store.branch.lock_write().unlock)
         section = store.get_mutable_section(None)
         section.set('foo', 'bar')
         store.save()
@@ -2972,6 +2987,11 @@
     def test_set_option_in_named_section(self):
         store = self.get_store(self)
         store._load_from_string('')
+        # FIXME: There should be a better way than relying on the test
+        # parametrization to identify branch.conf -- vila 2011-0526
+        if self.store_id in ('branch', 'remote_branch'):
+            # branch stores requires write locked branches
+            self.addCleanup(store.branch.lock_write().unlock)
         section = store.get_mutable_section('baz')
         section.set('foo', 'bar')
         store.save()
@@ -2981,8 +3001,13 @@
         self.assertSectionContent(('baz', {'foo': 'bar'}), sections[0])
 
     def test_load_hook(self):
-        # We first needs to ensure that the store exists
+        # First, we need to ensure that the store exists
         store = self.get_store(self)
+        # FIXME: There should be a better way than relying on the test
+        # parametrization to identify branch.conf -- vila 2011-0526
+        if self.store_id in ('branch', 'remote_branch'):
+            # branch stores requires write locked branches
+            self.addCleanup(store.branch.lock_write().unlock)
         section = store.get_mutable_section('baz')
         section.set('foo', 'bar')
         store.save()
@@ -3004,6 +3029,11 @@
         config.ConfigHooks.install_named_hook('save', hook, None)
         self.assertLength(0, calls)
         store = self.get_store(self)
+        # FIXME: There should be a better way than relying on the test
+        # parametrization to identify branch.conf -- vila 2011-0526
+        if self.store_id in ('branch', 'remote_branch'):
+            # branch stores requires write locked branches
+            self.addCleanup(store.branch.lock_write().unlock)
         section = store.get_mutable_section('baz')
         section.set('foo', 'bar')
         store.save()
@@ -3835,7 +3865,8 @@
         super(TestStackExpandOptions, self).setUp()
         self.overrideAttr(config, 'option_registry', config.OptionRegistry())
         self.registry = config.option_registry
-        self.conf = build_branch_stack(self)
+        store = config.TransportIniFileStore(self.get_transport(), 'foo.conf')
+        self.conf = config.Stack([store.get_sections], store)
 
     def assertExpansion(self, expected, string, env=None):
         self.assertEquals(expected, self.conf.expand_options(string, env))

=== modified file 'bzrlib/tests/test_directory_service.py'
--- bzrlib/tests/test_directory_service.py	2011-11-29 01:35:03 +0000
+++ bzrlib/tests/test_directory_service.py	2012-02-14 17:36:32 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2008, 2009, 2010 Canonical Ltd
+# Copyright (C) 2008-2012 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -65,55 +65,53 @@
 
 class TestAliasDirectory(TestCaseWithTransport):
 
+    def setUp(self):
+        super(TestAliasDirectory, self).setUp()
+        self.branch = self.make_branch('.')
+
+    def assertAliasFromBranch(self, setter, value, alias):
+        setter(value)
+        self.assertEquals(value, directories.dereference(alias))
+
     def test_lookup_parent(self):
-        branch = self.make_branch('.')
-        branch.set_parent('http://a')
-        self.assertEqual('http://a', directories.dereference(':parent'))
+        self.assertAliasFromBranch(self.branch.set_parent, 'http://a',
+                                  ':parent')
 
     def test_lookup_submit(self):
-        branch = self.make_branch('.')
-        branch.set_submit_branch('http://b')
-        self.assertEqual('http://b', directories.dereference(':submit'))
+        self.assertAliasFromBranch(self.branch.set_submit_branch, 'http://b',
+                                   ':submit')
 
     def test_lookup_public(self):
-        branch = self.make_branch('.')
-        branch.set_public_branch('http://c')
-        self.assertEqual('http://c', directories.dereference(':public'))
+        self.assertAliasFromBranch(self.branch.set_public_branch, 'http://c',
+                                   ':public')
 
     def test_lookup_bound(self):
-        branch = self.make_branch('.')
-        branch.set_bound_location('http://d')
-        self.assertEqual('http://d', directories.dereference(':bound'))
+        self.assertAliasFromBranch(self.branch.set_bound_location, 'http://d',
+                                   ':bound')
 
     def test_lookup_push(self):
-        branch = self.make_branch('.')
-        branch.set_push_location('http://e')
-        self.assertEqual('http://e', directories.dereference(':push'))
+        self.assertAliasFromBranch(self.branch.set_push_location, 'http://e',
+                                   ':push')
 
     def test_lookup_this(self):
-        branch = self.make_branch('.')
-        self.assertEqual(branch.base, directories.dereference(':this'))
+        self.assertEqual(self.branch.base, directories.dereference(':this'))
 
     def test_extra_path(self):
-        branch = self.make_branch('.')
-        self.assertEqual(urlutils.join(branch.base, 'arg'),
+        self.assertEqual(urlutils.join(self.branch.base, 'arg'),
                          directories.dereference(':this/arg'))
 
     def test_lookup_badname(self):
-        branch = self.make_branch('.')
         e = self.assertRaises(errors.InvalidLocationAlias,
                               directories.dereference, ':booga')
         self.assertEqual('":booga" is not a valid location alias.',
                          str(e))
 
     def test_lookup_badvalue(self):
-        branch = self.make_branch('.')
         e = self.assertRaises(errors.UnsetLocationAlias,
                               directories.dereference, ':parent')
         self.assertEqual('No parent location assigned.', str(e))
 
     def test_register_location_alias(self):
-        branch = self.make_branch('.')
         self.addCleanup(AliasDirectory.branch_aliases.remove, "booga")
         AliasDirectory.branch_aliases.register("booga",
             lambda b: "UHH?", help="Nobody knows")

=== modified file 'bzrlib/tests/test_info.py'
--- bzrlib/tests/test_info.py	2012-01-19 16:23:53 +0000
+++ bzrlib/tests/test_info.py	2012-02-14 17:36:32 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2007-2011 Canonical Ltd
+# Copyright (C) 2007-2012 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -322,10 +322,14 @@
 
     def test_gather_related_braches(self):
         branch = self.make_branch('.')
-        branch.set_public_branch('baz')
-        branch.set_push_location('bar')
-        branch.set_parent('foo')
-        branch.set_submit_branch('qux')
+        branch.lock_write()
+        try:
+            branch.set_public_branch('baz')
+            branch.set_push_location('bar')
+            branch.set_parent('foo')
+            branch.set_submit_branch('qux')
+        finally:
+            branch.unlock()
         self.assertEqual(
             [('public branch', 'baz'), ('push branch', 'bar'),
              ('parent branch', 'foo'), ('submit branch', 'qux')],

=== modified file 'bzrlib/tests/test_reconfigure.py'
--- bzrlib/tests/test_reconfigure.py	2011-12-05 17:31:43 +0000
+++ bzrlib/tests/test_reconfigure.py	2012-02-14 17:36:32 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2007, 2008, 2009 Canonical Ltd
+# Copyright (C) 2007, 2008, 2009, 2011, 2012 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -72,7 +72,8 @@
         checkout = branch.create_checkout('checkout')
         reconfiguration = reconfigure.Reconfigure.to_branch(checkout.bzrdir)
         reconfiguration.apply()
-        self.assertIs(None, checkout.branch.get_bound_location())
+        reconfigured = bzrdir.BzrDir.open('checkout').open_branch()
+        self.assertIs(None, reconfigured.get_bound_location())
 
     def prepare_lightweight_checkout_to_branch(self):
         branch = self.make_branch('branch')
@@ -147,16 +148,24 @@
         self.assertRaises(errors.NoBindLocation,
                           reconfiguration._select_bind_location)
         branch.set_parent('http://parent')
+        reconfiguration = reconfigure.Reconfigure(branch.bzrdir)
         self.assertEqual('http://parent',
                          reconfiguration._select_bind_location())
         branch.set_push_location('sftp://push')
+        reconfiguration = reconfigure.Reconfigure(branch.bzrdir)
         self.assertEqual('sftp://push',
                          reconfiguration._select_bind_location())
-        branch.set_bound_location('bzr://foo/old-bound')
-        branch.set_bound_location(None)
+        branch.lock_write()
+        try:
+            branch.set_bound_location('bzr://foo/old-bound')
+            branch.set_bound_location(None)
+        finally:
+            branch.unlock()
+        reconfiguration = reconfigure.Reconfigure(branch.bzrdir)
         self.assertEqual('bzr://foo/old-bound',
                          reconfiguration._select_bind_location())
         branch.set_bound_location('bzr://foo/cur-bound')
+        reconfiguration = reconfigure.Reconfigure(branch.bzrdir)
         self.assertEqual('bzr://foo/cur-bound',
                          reconfiguration._select_bind_location())
         reconfiguration.new_bound_location = 'ftp://user-specified'
@@ -180,6 +189,7 @@
         self.assertRaises(errors.NoBindLocation, reconfiguration.apply)
         # setting a parent allows it to become a checkout
         tree.branch.set_parent(parent.base)
+        reconfiguration = reconfigure.Reconfigure.to_checkout(tree.bzrdir)
         reconfiguration.apply()
         # supplying a location allows it to become a checkout
         tree2 = self.make_branch_and_tree('tree2')
@@ -198,6 +208,8 @@
         self.assertRaises(errors.NoBindLocation, reconfiguration.apply)
         # setting a parent allows it to become a checkout
         tree.branch.set_parent(parent.base)
+        reconfiguration = reconfigure.Reconfigure.to_lightweight_checkout(
+            tree.bzrdir)
         reconfiguration.apply()
         # supplying a location allows it to become a checkout
         tree2 = self.make_branch_and_tree('tree2')

=== modified file 'bzrlib/tests/test_remote.py'
--- bzrlib/tests/test_remote.py	2012-01-28 01:43:42 +0000
+++ bzrlib/tests/test_remote.py	2012-02-14 17:36:32 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006-2011 Canonical Ltd
+# Copyright (C) 2006-2012 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -1296,7 +1296,7 @@
         verb = 'Branch.set_parent_location'
         self.disable_verb(verb)
         branch.set_parent('http://foo/')
-        self.assertLength(13, self.hpss_calls)
+        self.assertLength(14, self.hpss_calls)
 
 
 class TestBranchGetTagsBytes(RemoteBranchTestCase):
@@ -1455,34 +1455,31 @@
         return branch
 
     def test_backwards_compatible(self):
-        branch = self.make_branch_with_tags()
-        c = branch.get_config_stack()
-        c.set('branch.fetch_tags', True)
-        self.addCleanup(branch.lock_read().unlock)
+        br = self.make_branch_with_tags()
+        br.get_config_stack().set('branch.fetch_tags', True)
+        self.addCleanup(br.lock_read().unlock)
         # Disable the heads_to_fetch verb
         verb = 'Branch.heads_to_fetch'
         self.disable_verb(verb)
         self.reset_smart_call_log()
-        result = branch.heads_to_fetch()
+        result = br.heads_to_fetch()
         self.assertEqual((set(['tip']), set(['rev-1', 'rev-2'])), result)
         self.assertEqual(
-            ['Branch.last_revision_info', 'Branch.get_config_file',
-             'Branch.get_tags_bytes'],
+            ['Branch.last_revision_info', 'Branch.get_tags_bytes'],
             [call.call.method for call in self.hpss_calls])
 
     def test_backwards_compatible_no_tags(self):
-        branch = self.make_branch_with_tags()
-        c = branch.get_config_stack()
-        c.set('branch.fetch_tags', False)
-        self.addCleanup(branch.lock_read().unlock)
+        br = self.make_branch_with_tags()
+        br.get_config_stack().set('branch.fetch_tags', False)
+        self.addCleanup(br.lock_read().unlock)
         # Disable the heads_to_fetch verb
         verb = 'Branch.heads_to_fetch'
         self.disable_verb(verb)
         self.reset_smart_call_log()
-        result = branch.heads_to_fetch()
+        result = br.heads_to_fetch()
         self.assertEqual((set(['tip']), set()), result)
         self.assertEqual(
-            ['Branch.last_revision_info', 'Branch.get_config_file'],
+            ['Branch.last_revision_info'],
             [call.call.method for call in self.hpss_calls])
 
 
@@ -2073,6 +2070,9 @@
             'Branch.get_config_file', ('memory:///', ),
             'success', ('ok', ), "# line 1\n")
         client.add_expected_call(
+            'Branch.get_config_file', ('memory:///', ),
+            'success', ('ok', ), "# line 1\n")
+        client.add_expected_call(
             'Branch.put_config_file', ('memory:///', 'branch token',
             'repo token'),
             'success', ('ok',))
@@ -2090,6 +2090,7 @@
             [('call', 'Branch.get_stacked_on_url', ('memory:///',)),
              ('call', 'Branch.lock_write', ('memory:///', '', '')),
              ('call_expecting_body', 'Branch.get_config_file', ('memory:///',)),
+             ('call_expecting_body', 'Branch.get_config_file', ('memory:///',)),
              ('call_with_body_bytes_expecting_body', 'Branch.put_config_file',
                  ('memory:///', 'branch token', 'repo token'),
                  '# line 1\nemail = The Dude <[e-mail]>\n'),

=== modified file 'bzrlib/tests/test_smart.py'
--- bzrlib/tests/test_smart.py	2012-01-30 14:18:22 +0000
+++ bzrlib/tests/test_smart.py	2012-02-14 17:36:32 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006-2011 Canonical Ltd
+# Copyright (C) 2006-2012 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -1323,6 +1323,8 @@
         finally:
             branch.unlock()
         self.assertEqual(smart_req.SuccessfulSmartServerResponse(()), response)
+        # Refresh branch as SetParentLocation modified it
+        branch = branch.bzrdir.open_branch()
         self.assertEqual(None, branch.get_parent())
 
     def test_set_parent_something(self):
@@ -1332,11 +1334,12 @@
         branch_token, repo_token = self.get_lock_tokens(branch)
         try:
             response = request.execute('base', branch_token, repo_token,
-            'http://bar/')
+                                       'http://bar/')
         finally:
             branch.unlock()
         self.assertEqual(smart_req.SuccessfulSmartServerResponse(()), response)
-        self.assertEqual('http://bar/', branch.get_parent())
+        refreshed = _mod_branch.Branch.open(branch.base)
+        self.assertEqual('http://bar/', refreshed.get_parent())
 
 
 class TestSmartServerBranchRequestGetTagsBytes(