Rev 4292: (robertc) Add a Branch.set_parent_location verb and reduce lock in file:///home/pqm/archives/thelove/bzr/%2Btrunk/
Canonical.com Patch Queue Manager
pqm at pqm.ubuntu.com
Wed Apr 15 09:20:37 BST 2009
At file:///home/pqm/archives/thelove/bzr/%2Btrunk/
------------------------------------------------------------
revno: 4292
revision-id: pqm at pqm.ubuntu.com-20090415082033-tds4zs962x6kwc2c
parent: pqm at pqm.ubuntu.com-20090415031036-ikndntbkaaj5zjya
parent: robertc at robertcollins.net-20090415073034-plcl46p8poz08kzb
committer: Canonical.com Patch Queue Manager <pqm at pqm.ubuntu.com>
branch nick: +trunk
timestamp: Wed 2009-04-15 09:20:33 +0100
message:
(robertc) Add a Branch.set_parent_location verb and reduce lock
churning to reduce network round trips. (Robert Collins)
modified:
NEWS NEWS-20050323055033-4e00b5db738777ff
bzrlib/branch.py branch.py-20050309040759-e4baf4e0d046576e
bzrlib/remote.py remote.py-20060720103555-yeeg2x51vn0rbtdp-1
bzrlib/smart/branch.py branch.py-20061124031907-mzh3pla28r83r97f-1
bzrlib/smart/request.py request.py-20061108095550-gunadhxmzkdjfeek-1
bzrlib/tests/blackbox/test_branch.py test_branch.py-20060524161337-noms9gmcwqqrfi8y-1
bzrlib/tests/blackbox/test_push.py test_push.py-20060329002750-929af230d5d22663
bzrlib/tests/branch_implementations/test_locking.py test_locking.py-20060707151933-tav3o2hpibwi53u4-4
bzrlib/tests/branch_implementations/test_parent.py test_parent.py-20050830052751-5e62766623c32222
bzrlib/tests/lock_helpers.py LockHelpers.py-20060707151933-tav3o2hpibwi53u4-1
bzrlib/tests/test_remote.py test_remote.py-20060720103555-yeeg2x51vn0rbtdp-2
bzrlib/tests/test_smart.py test_smart.py-20061122024551-ol0l0o0oofsu9b3t-2
------------------------------------------------------------
revno: 4288.1.12
revision-id: robertc at robertcollins.net-20090415073034-plcl46p8poz08kzb
parent: robertc at robertcollins.net-20090415053938-9w379adk8993sq1q
committer: Robert Collins <robertc at robertcollins.net>
branch nick: push.roundtrips
timestamp: Wed 2009-04-15 17:30:34 +1000
message:
Review feedback.
modified:
bzrlib/branch.py branch.py-20050309040759-e4baf4e0d046576e
bzrlib/tests/lock_helpers.py LockHelpers.py-20060707151933-tav3o2hpibwi53u4-1
------------------------------------------------------------
revno: 4288.1.11
revision-id: robertc at robertcollins.net-20090415053938-9w379adk8993sq1q
parent: robertc at robertcollins.net-20090415044506-30fnvsa3ukai60xb
committer: Robert Collins <robertc at robertcollins.net>
branch nick: push.roundtrips
timestamp: Wed 2009-04-15 15:39:38 +1000
message:
Hopefully fix locking tests to match the new code (and still be good statements of intent).
modified:
bzrlib/branch.py branch.py-20050309040759-e4baf4e0d046576e
bzrlib/tests/branch_implementations/test_locking.py test_locking.py-20060707151933-tav3o2hpibwi53u4-4
bzrlib/tests/lock_helpers.py LockHelpers.py-20060707151933-tav3o2hpibwi53u4-1
------------------------------------------------------------
revno: 4288.1.10
revision-id: robertc at robertcollins.net-20090415044506-30fnvsa3ukai60xb
parent: robertc at robertcollins.net-20090415034524-ivssdg89z2lzs9pw
committer: Robert Collins <robertc at robertcollins.net>
branch nick: push.roundtrips
timestamp: Wed 2009-04-15 14:45:06 +1000
message:
Fix up lock correctness to deal with adding fallback repositories to locked branch objects.
modified:
bzrlib/branch.py branch.py-20050309040759-e4baf4e0d046576e
bzrlib/remote.py remote.py-20060720103555-yeeg2x51vn0rbtdp-1
bzrlib/tests/blackbox/test_branch.py test_branch.py-20060524161337-noms9gmcwqqrfi8y-1
------------------------------------------------------------
revno: 4288.1.9
revision-id: robertc at robertcollins.net-20090415034524-ivssdg89z2lzs9pw
parent: robertc at robertcollins.net-20090415032048-u7ccp2v2ee14ebjt
committer: Robert Collins <robertc at robertcollins.net>
branch nick: push.roundtrips
timestamp: Wed 2009-04-15 13:45:24 +1000
message:
Fix up test usable of _set_parent_location on unlocked branches.
modified:
bzrlib/tests/branch_implementations/test_parent.py test_parent.py-20050830052751-5e62766623c32222
bzrlib/tests/test_smart.py test_smart.py-20061122024551-ol0l0o0oofsu9b3t-2
------------------------------------------------------------
revno: 4288.1.8
revision-id: robertc at robertcollins.net-20090415032048-u7ccp2v2ee14ebjt
parent: robertc at robertcollins.net-20090415020735-poizrhi1b98mtdgk
committer: Robert Collins <robertc at robertcollins.net>
branch nick: push.roundtrips
timestamp: Wed 2009-04-15 13:20:48 +1000
message:
Lock new branches while we configure them in clone and sprout for less lock churn.
modified:
bzrlib/branch.py branch.py-20050309040759-e4baf4e0d046576e
bzrlib/tests/blackbox/test_push.py test_push.py-20060329002750-929af230d5d22663
------------------------------------------------------------
revno: 4288.1.7
revision-id: robertc at robertcollins.net-20090415020735-poizrhi1b98mtdgk
parent: robertc at robertcollins.net-20090415020527-yyh3muljj0kftcjr
committer: Robert Collins <robertc at robertcollins.net>
branch nick: push.roundtrips
timestamp: Wed 2009-04-15 12:07:35 +1000
message:
Add new remote server verb Branch.set_parent_location, dropping roundtrips further on push operations.
modified:
NEWS NEWS-20050323055033-4e00b5db738777ff
bzrlib/remote.py remote.py-20060720103555-yeeg2x51vn0rbtdp-1
bzrlib/smart/branch.py branch.py-20061124031907-mzh3pla28r83r97f-1
bzrlib/smart/request.py request.py-20061108095550-gunadhxmzkdjfeek-1
bzrlib/tests/blackbox/test_branch.py test_branch.py-20060524161337-noms9gmcwqqrfi8y-1
bzrlib/tests/blackbox/test_push.py test_push.py-20060329002750-929af230d5d22663
bzrlib/tests/test_remote.py test_remote.py-20060720103555-yeeg2x51vn0rbtdp-2
bzrlib/tests/test_smart.py test_smart.py-20061122024551-ol0l0o0oofsu9b3t-2
=== modified file 'NEWS'
--- a/NEWS 2009-04-15 03:10:36 +0000
+++ b/NEWS 2009-04-15 08:20:33 +0000
@@ -54,6 +54,10 @@
class and will call ``_set_parent_location`` after doing unicode
encoding. (Robert Collins)
+* ``bzrlib.remote.RemoteBranch._set_parent_location`` will use a new verb
+ ``Branch.set_parent_location`` removing further VFS operations.
+ (Robert Collins)
+
* ``bzrlib.bzrdir.BzrDir._get_config`` now returns a ``TransportConfig``
or similar when the dir supports configuration settings. The base class
defaults to None. There is a matching new server verb
=== modified file 'bzrlib/branch.py'
--- a/bzrlib/branch.py 2009-04-14 07:11:01 +0000
+++ b/bzrlib/branch.py 2009-04-15 07:30:34 +0000
@@ -99,10 +99,14 @@
def _open_hook(self):
"""Called by init to allow simpler extension of the base class."""
- def _activate_fallback_location(self, url):
+ def _activate_fallback_location(self, url, lock_style):
"""Activate the branch/repository from url as a fallback repository."""
- self.repository.add_fallback_repository(
- self._get_fallback_repository(url))
+ repo = self._get_fallback_repository(url)
+ if lock_style == 'write':
+ repo.lock_write()
+ elif lock_style == 'read':
+ repo.lock_read()
+ self.repository.add_fallback_repository(repo)
def break_lock(self):
"""Break a lock if one is present from another instance.
@@ -608,6 +612,7 @@
url = urlutils.relative_url(self.base, url)
self._set_parent_location(url)
+ @needs_write_lock
def set_stacked_on_url(self, url):
"""Set the URL this branch is stacked against.
@@ -626,10 +631,13 @@
errors.UnstackableRepositoryFormat):
return
url = ''
+ # XXX: Lock correctness - should unlock our old repo if we were
+ # locked.
# repositories don't offer an interface to remove fallback
# repositories today; take the conceptually simpler option and just
# reopen it.
self.repository = self.bzrdir.find_repository()
+ self.repository.lock_write()
# for every revision reference the branch has, ensure it is pulled
# in.
source_repository = self._get_fallback_repository(old_url)
@@ -638,7 +646,7 @@
self.repository.fetch(source_repository, revision_id,
find_ghosts=True)
else:
- self._activate_fallback_location(url)
+ self._activate_fallback_location(url, 'write')
# write this out after the repository is stacked to avoid setting a
# stacked config that doesn't work.
self._set_config_location('stacked_on_location', url)
@@ -997,10 +1005,14 @@
be truncated to end with revision_id.
"""
result = to_bzrdir.create_branch()
- if repository_policy is not None:
- repository_policy.configure_branch(result)
- self.copy_content_into(result, revision_id=revision_id)
- return result
+ result.lock_write()
+ try:
+ if repository_policy is not None:
+ repository_policy.configure_branch(result)
+ self.copy_content_into(result, revision_id=revision_id)
+ finally:
+ result.unlock()
+ return result
@needs_read_lock
def sprout(self, to_bzrdir, revision_id=None, repository_policy=None):
@@ -1012,10 +1024,14 @@
be truncated to end with revision_id.
"""
result = to_bzrdir.create_branch()
- if repository_policy is not None:
- repository_policy.configure_branch(result)
- self.copy_content_into(result, revision_id=revision_id)
- result.set_parent(self.bzrdir.root_transport.base)
+ result.lock_write()
+ try:
+ if repository_policy is not None:
+ repository_policy.configure_branch(result)
+ self.copy_content_into(result, revision_id=revision_id)
+ result.set_parent(self.bzrdir.root_transport.base)
+ finally:
+ result.unlock()
return result
def _synchronize_history(self, destination, revision_id):
@@ -1914,31 +1930,47 @@
return self.control_files.is_locked()
def lock_write(self, token=None):
- repo_token = self.repository.lock_write()
+ # All-in-one needs to always unlock/lock.
+ repo_control = getattr(self.repository, 'control_files', None)
+ if self.control_files == repo_control or not self.is_locked():
+ self.repository.lock_write()
+ took_lock = True
+ else:
+ took_lock = False
try:
- token = self.control_files.lock_write(token=token)
+ return self.control_files.lock_write(token=token)
except:
- self.repository.unlock()
+ if took_lock:
+ self.repository.unlock()
raise
- return token
def lock_read(self):
- self.repository.lock_read()
+ # All-in-one needs to always unlock/lock.
+ repo_control = getattr(self.repository, 'control_files', None)
+ if self.control_files == repo_control or not self.is_locked():
+ self.repository.lock_read()
+ took_lock = True
+ else:
+ took_lock = False
try:
self.control_files.lock_read()
except:
- self.repository.unlock()
+ if took_lock:
+ self.repository.unlock()
raise
def unlock(self):
- # TODO: test for failed two phase locks. This is known broken.
try:
self.control_files.unlock()
finally:
- self.repository.unlock()
- if not self.control_files.is_locked():
- # we just released the lock
- self._clear_cached_state()
+ # All-in-one needs to always unlock/lock.
+ repo_control = getattr(self.repository, 'control_files', None)
+ if (self.control_files == repo_control or
+ not self.control_files.is_locked()):
+ self.repository.unlock()
+ if not self.control_files.is_locked():
+ # we just released the lock
+ self._clear_cached_state()
def peek_lock_mode(self):
if self.control_files._lock_count == 0:
@@ -2363,7 +2395,7 @@
raise AssertionError(
"'transform_fallback_location' hook %s returned "
"None, not a URL." % hook_name)
- self._activate_fallback_location(url)
+ self._activate_fallback_location(url, None)
def __init__(self, *args, **kwargs):
self._ignore_fallbacks = kwargs.get('ignore_fallbacks', False)
=== modified file 'bzrlib/remote.py'
--- a/bzrlib/remote.py 2009-04-15 00:00:35 +0000
+++ b/bzrlib/remote.py 2009-04-15 04:45:06 +0000
@@ -1961,7 +1961,7 @@
except (errors.NotStacked, errors.UnstackableBranchFormat,
errors.UnstackableRepositoryFormat), e:
return
- self._activate_fallback_location(fallback_url)
+ self._activate_fallback_location(fallback_url, None)
def _get_config(self):
return RemoteBranchConfig(self)
@@ -2301,6 +2301,23 @@
return self._real_branch._get_parent_location()
def _set_parent_location(self, url):
+ medium = self._client._medium
+ if medium._is_remote_before((1, 15)):
+ return self._vfs_set_parent_location(url)
+ try:
+ call_url = url or ''
+ if type(call_url) is not str:
+ raise AssertionError('url must be a str or None (%s)' % url)
+ response = self._call('Branch.set_parent_location',
+ self._remote_path(), self._lock_token, self._repo_lock_token,
+ call_url)
+ except errors.UnknownSmartMethod:
+ medium._remember_remote_is_before((1, 15))
+ return self._vfs_set_parent_location(url)
+ if response != ():
+ raise errors.UnexpectedSmartServerResponse(response)
+
+ def _vfs_set_parent_location(self, url):
self._ensure_real()
return self._real_branch._set_parent_location(url)
=== modified file 'bzrlib/smart/branch.py'
--- a/bzrlib/smart/branch.py 2009-04-03 00:37:53 +0000
+++ b/bzrlib/smart/branch.py 2009-04-15 02:07:35 +0000
@@ -237,6 +237,17 @@
return SuccessfulSmartServerResponse(('ok',))
+class SmartServerBranchRequestSetParentLocation(SmartServerLockedBranchRequest):
+ """Set the parent location for a branch.
+
+ Takes a location to set, which must be utf8 encoded.
+ """
+
+ def do_with_locked_branch(self, branch, location):
+ branch._set_parent_location(location)
+ return SuccessfulSmartServerResponse(())
+
+
class SmartServerBranchRequestLockWrite(SmartServerBranchRequest):
def do_with_branch(self, branch, branch_token='', repo_token=''):
=== modified file 'bzrlib/smart/request.py'
--- a/bzrlib/smart/request.py 2009-04-14 04:33:41 +0000
+++ b/bzrlib/smart/request.py 2009-04-15 02:07:35 +0000
@@ -461,6 +461,9 @@
'Branch.set_last_revision_ex', 'bzrlib.smart.branch',
'SmartServerBranchRequestSetLastRevisionEx')
request_handlers.register_lazy(
+ 'Branch.set_parent_location', 'bzrlib.smart.branch',
+ 'SmartServerBranchRequestSetParentLocation')
+request_handlers.register_lazy(
'Branch.unlock', 'bzrlib.smart.branch', 'SmartServerBranchRequestUnlock')
request_handlers.register_lazy(
'BzrDir.cloning_metadir', 'bzrlib.smart.bzrdir',
=== modified file 'bzrlib/tests/blackbox/test_branch.py'
--- a/bzrlib/tests/blackbox/test_branch.py 2009-04-14 07:11:01 +0000
+++ b/bzrlib/tests/blackbox/test_branch.py 2009-04-15 04:45:06 +0000
@@ -272,7 +272,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(47, self.hpss_calls)
+ self.assertLength(39, self.hpss_calls)
def test_branch_from_trivial_branch_streaming_acceptance(self):
self.setup_smart_server_with_call_log()
=== modified file 'bzrlib/tests/blackbox/test_push.py'
--- a/bzrlib/tests/blackbox/test_push.py 2009-04-15 00:00:35 +0000
+++ b/bzrlib/tests/blackbox/test_push.py 2009-04-15 03:20:48 +0000
@@ -217,7 +217,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(23, self.hpss_calls)
remote = Branch.open('public')
self.assertEndsWith(remote.get_stacked_on_url(), '/parent')
=== modified file 'bzrlib/tests/branch_implementations/test_locking.py'
--- a/bzrlib/tests/branch_implementations/test_locking.py 2009-03-23 14:59:43 +0000
+++ b/bzrlib/tests/branch_implementations/test_locking.py 2009-04-15 05:39:38 +0000
@@ -171,7 +171,9 @@
b.repository._other.unlock()
def test_04_lock_fail_unlock_control(self):
- # Make sure repository.unlock() is called, if we fail to unlock self
+ # Make sure repository.unlock() is not called, if we fail to unlock
+ # self leaving ourselves still locked, so that attempts to recover
+ # don't encounter an unlocked repository.
b = self.get_instrumented_branch()
b.control_files.disable_unlock()
@@ -183,10 +185,7 @@
self.assertTrue(b.repository.is_locked())
self.assertRaises(TestPreventLocking, b.unlock)
self.assertTrue(b.is_locked())
- if self.combined_control:
- self.assertTrue(b.repository.is_locked())
- else:
- self.assertFalse(b.repository.is_locked())
+ self.assertTrue(b.repository.is_locked())
# We unlock the repository even if
# we fail to unlock the control files
@@ -206,7 +205,6 @@
('bc', 'lw', True),
('b', 'ul', True),
('bc', 'ul', False),
- ('r', 'ul', True),
], self.locks)
finally:
=== modified file 'bzrlib/tests/branch_implementations/test_parent.py'
--- a/bzrlib/tests/branch_implementations/test_parent.py 2009-03-23 14:59:43 +0000
+++ b/bzrlib/tests/branch_implementations/test_parent.py 2009-04-15 03:45:24 +0000
@@ -71,7 +71,9 @@
# paths as well? Nobody has complained about it.
pass
else:
+ b.lock_write()
b._set_parent_location('/local/abs/path')
+ b.unlock()
self.assertEqual('file:///local/abs/path', b.get_parent())
def test_get_invalid_parent(self):
@@ -83,7 +85,9 @@
# Force the relative path to be something invalid
# This should attempt to go outside the filesystem
path = ('../'*(n_dirs+5)) + 'foo'
+ b.lock_write()
b._set_parent_location(path)
+ b.unlock()
# With an invalid branch parent, just return None
self.assertRaises(bzrlib.errors.InaccessibleParent, b.get_parent)
=== modified file 'bzrlib/tests/lock_helpers.py'
--- a/bzrlib/tests/lock_helpers.py 2009-03-23 14:59:43 +0000
+++ b/bzrlib/tests/lock_helpers.py 2009-04-15 07:30:34 +0000
@@ -44,6 +44,12 @@
self.__dict__['_allow_read'] = True
self.__dict__['_allow_unlock'] = True
+ def __eq__(self, other):
+ # Branch objects look for controlfiles == repo.controlfiles.
+ if type(other) is LockWrapper:
+ return self._other == other._other
+ return False
+
def __getattr__(self, attr):
return getattr(self._other, attr)
=== modified file 'bzrlib/tests/test_remote.py'
--- a/bzrlib/tests/test_remote.py 2009-04-14 04:33:41 +0000
+++ b/bzrlib/tests/test_remote.py 2009-04-15 02:07:35 +0000
@@ -844,6 +844,54 @@
branch = self.make_remote_branch(transport, client)
result = branch.get_parent()
self.assertEqual('http://foo/', result)
+ client.finished_test()
+
+
+class TestBranchSetParentLocation(RemoteBranchTestCase):
+
+ def test_no_parent(self):
+ # We call the verb when setting parent to None
+ transport = MemoryTransport()
+ client = FakeClient(transport.base)
+ client.add_expected_call(
+ 'Branch.get_stacked_on_url', ('quack/',),
+ 'error', ('NotStacked',))
+ client.add_expected_call(
+ 'Branch.set_parent_location', ('quack/', 'b', 'r', ''),
+ 'success', ())
+ transport.mkdir('quack')
+ transport = transport.clone('quack')
+ branch = self.make_remote_branch(transport, client)
+ branch._lock_token = 'b'
+ branch._repo_lock_token = 'r'
+ branch._set_parent_location(None)
+ client.finished_test()
+
+ def test_parent(self):
+ transport = MemoryTransport()
+ client = FakeClient(transport.base)
+ client.add_expected_call(
+ 'Branch.get_stacked_on_url', ('kwaak/',),
+ 'error', ('NotStacked',))
+ client.add_expected_call(
+ 'Branch.set_parent_location', ('kwaak/', 'b', 'r', 'foo'),
+ 'success', ())
+ transport.mkdir('kwaak')
+ transport = transport.clone('kwaak')
+ branch = self.make_remote_branch(transport, client)
+ branch._lock_token = 'b'
+ branch._repo_lock_token = 'r'
+ branch._set_parent_location('foo')
+ client.finished_test()
+
+ def test_backwards_compat(self):
+ self.setup_smart_server_with_call_log()
+ branch = self.make_branch('.')
+ self.reset_smart_call_log()
+ verb = 'Branch.set_parent_location'
+ self.disable_verb(verb)
+ branch.set_parent('http://foo/')
+ self.assertLength(12, self.hpss_calls)
class TestBranchGetTagsBytes(RemoteBranchTestCase):
=== modified file 'bzrlib/tests/test_smart.py'
--- a/bzrlib/tests/test_smart.py 2009-04-14 04:33:41 +0000
+++ b/bzrlib/tests/test_smart.py 2009-04-15 03:45:24 +0000
@@ -792,6 +792,41 @@
response)
+class TestSmartServerBranchRequestSetParent(tests.TestCaseWithMemoryTransport):
+
+ def test_set_parent_none(self):
+ branch = self.make_branch('base', format="1.9")
+ branch.lock_write()
+ branch._set_parent_location('foo')
+ branch.unlock()
+ request = smart.branch.SmartServerBranchRequestSetParentLocation(
+ self.get_transport())
+ branch_token = branch.lock_write()
+ repo_token = branch.repository.lock_write()
+ try:
+ response = request.execute('base', branch_token, repo_token, '')
+ finally:
+ branch.repository.unlock()
+ branch.unlock()
+ self.assertEqual(SuccessfulSmartServerResponse(()), response)
+ self.assertEqual(None, branch.get_parent())
+
+ def test_set_parent_something(self):
+ branch = self.make_branch('base', format="1.9")
+ request = smart.branch.SmartServerBranchRequestSetParentLocation(
+ self.get_transport())
+ branch_token = branch.lock_write()
+ repo_token = branch.repository.lock_write()
+ try:
+ response = request.execute('base', branch_token, repo_token,
+ 'http://bar/')
+ finally:
+ branch.repository.unlock()
+ branch.unlock()
+ self.assertEqual(SuccessfulSmartServerResponse(()), response)
+ self.assertEqual('http://bar/', branch.get_parent())
+
+
class TestSmartServerBranchRequestGetTagsBytes(tests.TestCaseWithMemoryTransport):
# Only called when the branch format and tags match [yay factory
# methods] so only need to test straight forward cases.
@@ -1432,6 +1467,10 @@
smart.branch.SmartServerBranchRequestSetLastRevision)
self.assertHandlerEqual('Branch.set_last_revision_info',
smart.branch.SmartServerBranchRequestSetLastRevisionInfo)
+ self.assertHandlerEqual('Branch.set_last_revision_ex',
+ smart.branch.SmartServerBranchRequestSetLastRevisionEx)
+ self.assertHandlerEqual('Branch.set_parent_location',
+ smart.branch.SmartServerBranchRequestSetParentLocation)
self.assertHandlerEqual('Branch.unlock',
smart.branch.SmartServerBranchRequestUnlock)
self.assertHandlerEqual('BzrDir.find_repository',
More information about the bazaar-commits
mailing list