Rev 3693: (spiv) Fix bug #237067 by having RemoteBranch properly lock its in file:///home/pqm/archives/thelove/bzr/%2Btrunk/
Canonical.com Patch Queue Manager
pqm at pqm.ubuntu.com
Fri Sep 5 22:25:55 BST 2008
At file:///home/pqm/archives/thelove/bzr/%2Btrunk/
------------------------------------------------------------
revno: 3693
revision-id: pqm at pqm.ubuntu.com-20080905212548-ig8wqqpv4vb8b2v4
parent: pqm at pqm.ubuntu.com-20080905055502-uq3g4uwzl6agbyy4
parent: andrew.bennetts at canonical.com-20080905104803-6g72dz6wcldosfs2
committer: Canonical.com Patch Queue Manager <pqm at pqm.ubuntu.com>
branch nick: +trunk
timestamp: Fri 2008-09-05 22:25:48 +0100
message:
(spiv) Fix bug #237067 by having RemoteBranch properly lock its
RemoteRepository.
modified:
NEWS NEWS-20050323055033-4e00b5db738777ff
bzrlib/remote.py remote.py-20060720103555-yeeg2x51vn0rbtdp-1
bzrlib/tests/branch_implementations/test_locking.py test_locking.py-20060707151933-tav3o2hpibwi53u4-4
bzrlib/tests/test_remote.py test_remote.py-20060720103555-yeeg2x51vn0rbtdp-2
------------------------------------------------------------
revno: 3692.1.6
revision-id: andrew.bennetts at canonical.com-20080905104803-6g72dz6wcldosfs2
parent: andrew.bennetts at canonical.com-20080905102540-118qgqvl1a6nt019
committer: Andrew Bennetts <andrew.bennetts at canonical.com>
branch nick: objectnotlocked
timestamp: Fri 2008-09-05 20:48:03 +1000
message:
Remove monkey-patching of branch._ensure_real from test_remote.py.
modified:
bzrlib/tests/test_remote.py test_remote.py-20060720103555-yeeg2x51vn0rbtdp-2
------------------------------------------------------------
revno: 3692.1.5
revision-id: andrew.bennetts at canonical.com-20080905102540-118qgqvl1a6nt019
parent: andrew.bennetts at canonical.com-20080905102129-a7v2uzv07tdo495e
committer: Andrew Bennetts <andrew.bennetts at canonical.com>
branch nick: objectnotlocked
timestamp: Fri 2008-09-05 20:25:40 +1000
message:
Fix bug revealed by removing _ensure_real call from RemoteBranch.lock_write.
modified:
bzrlib/remote.py remote.py-20060720103555-yeeg2x51vn0rbtdp-1
------------------------------------------------------------
revno: 3692.1.4
revision-id: andrew.bennetts at canonical.com-20080905102129-a7v2uzv07tdo495e
parent: andrew.bennetts at canonical.com-20080905101704-6g8iio31vb1wb2pf
committer: Andrew Bennetts <andrew.bennetts at canonical.com>
branch nick: objectnotlocked
timestamp: Fri 2008-09-05 20:21:29 +1000
message:
Add NEWS entry.
modified:
NEWS NEWS-20050323055033-4e00b5db738777ff
------------------------------------------------------------
revno: 3692.1.3
revision-id: andrew.bennetts at canonical.com-20080905101704-6g8iio31vb1wb2pf
parent: andrew.bennetts at canonical.com-20080905085529-o3abc8tnazsx2qth
committer: Andrew Bennetts <andrew.bennetts at canonical.com>
branch nick: objectnotlocked
timestamp: Fri 2008-09-05 20:17:04 +1000
message:
Delete some cruft (like the _ensure_real call in RemoteBranch.lock_write), improve some comments, and wrap some long lines.
modified:
bzrlib/remote.py remote.py-20060720103555-yeeg2x51vn0rbtdp-1
------------------------------------------------------------
revno: 3692.1.2
revision-id: andrew.bennetts at canonical.com-20080905085529-o3abc8tnazsx2qth
parent: andrew.bennetts at canonical.com-20080905083730-mobjr4wfkz8rxdcr
committer: Andrew Bennetts <andrew.bennetts at canonical.com>
branch nick: objectnotlocked
timestamp: Fri 2008-09-05 18:55:29 +1000
message:
Fix regression introduced by fix, and add a test for that regression.
modified:
bzrlib/remote.py remote.py-20060720103555-yeeg2x51vn0rbtdp-1
bzrlib/tests/branch_implementations/test_locking.py test_locking.py-20060707151933-tav3o2hpibwi53u4-4
------------------------------------------------------------
revno: 3692.1.1
revision-id: andrew.bennetts at canonical.com-20080905083730-mobjr4wfkz8rxdcr
parent: pqm at pqm.ubuntu.com-20080905055502-uq3g4uwzl6agbyy4
committer: Andrew Bennetts <andrew.bennetts at canonical.com>
branch nick: objectnotlocked
timestamp: Fri 2008-09-05 18:37:30 +1000
message:
Make RemoteBranch.lock_write lock the repository too.
modified:
bzrlib/remote.py remote.py-20060720103555-yeeg2x51vn0rbtdp-1
bzrlib/tests/test_remote.py test_remote.py-20060720103555-yeeg2x51vn0rbtdp-2
=== modified file 'NEWS'
--- a/NEWS 2008-09-05 05:12:35 +0000
+++ b/NEWS 2008-09-05 10:21:29 +0000
@@ -65,6 +65,10 @@
pycurl try to send a body request to an HTTP/1.0 server which has
already refused to handle the request. (Vincent Ladeuil, #225020)
+ * Fix ``ObjectNotLocked`` errors when using various commands
+ (including ``bzr cat`` and ``bzr annotate``) in combination with a
+ smart server URL. (Andrew Bennetts, #237067)
+
* ``FTPTransport.stat()`` would return ``0000`` as the permission bits
for the containing ``.bzr/`` directory (it does not implement
permissions). This would cause us to set all subdirectories to
=== modified file 'bzrlib/remote.py'
--- a/bzrlib/remote.py 2008-09-01 06:19:29 +0000
+++ b/bzrlib/remote.py 2008-09-05 10:25:40 +0000
@@ -356,10 +356,10 @@
Used before calls to self._real_repository.
"""
- if not self._real_repository:
+ if self._real_repository is None:
self.bzrdir._ensure_real()
- #self._real_repository = self.bzrdir._real_bzrdir.open_repository()
- self._set_real_repository(self.bzrdir._real_bzrdir.open_repository())
+ self._set_real_repository(
+ self.bzrdir._real_bzrdir.open_repository())
def _translate_error(self, err, **context):
self.bzrdir._translate_error(err, repository=self, **context)
@@ -549,9 +549,15 @@
else:
raise errors.UnexpectedSmartServerResponse(response)
- def lock_write(self, token=None):
+ def lock_write(self, token=None, _skip_rpc=False):
if not self._lock_mode:
- self._lock_token = self._remote_lock_write(token)
+ if _skip_rpc:
+ if self._lock_token is not None:
+ if token != self._lock_token:
+ raise TokenMismatch(token, self._lock_token)
+ self._lock_token = token
+ else:
+ self._lock_token = self._remote_lock_write(token)
# if self._lock_token is None, then this is something like packs or
# svn where we don't get to lock the repo, or a weave style repository
# where we cannot lock it over the wire and attempts to do so will
@@ -589,6 +595,8 @@
:param repository: The repository to fallback to for non-hpss
implemented operations.
"""
+ if self._real_repository is not None:
+ raise AssertionError('_real_repository is already set')
if isinstance(repository, RemoteRepository):
raise AssertionError()
self._real_repository = repository
@@ -703,7 +711,8 @@
# FIXME: It ought to be possible to call this without immediately
# triggering _ensure_real. For now it's the easiest thing to do.
self._ensure_real()
- builder = self._real_repository.get_commit_builder(branch, parents,
+ real_repo = self._real_repository
+ builder = real_repo.get_commit_builder(branch, parents,
config, timestamp=timestamp, timezone=timezone,
committer=committer, revprops=revprops, revision_id=revision_id)
return builder
@@ -1301,17 +1310,20 @@
'to use vfs implementation')
self.bzrdir._ensure_real()
self._real_branch = self.bzrdir._real_bzrdir.open_branch()
- # Give the remote repository the matching real repo.
- real_repo = self._real_branch.repository
- if isinstance(real_repo, RemoteRepository):
- real_repo._ensure_real()
- real_repo = real_repo._real_repository
- self.repository._set_real_repository(real_repo)
- # Give the branch the remote repository to let fast-pathing happen.
+ if self.repository._real_repository is None:
+ # Give the remote repository the matching real repo.
+ real_repo = self._real_branch.repository
+ if isinstance(real_repo, RemoteRepository):
+ real_repo._ensure_real()
+ real_repo = real_repo._real_repository
+ self.repository._set_real_repository(real_repo)
+ # Give the real branch the remote repository to let fast-pathing
+ # happen.
self._real_branch.repository = self.repository
- # XXX: deal with _lock_mode == 'w'
if self._lock_mode == 'r':
self._real_branch.lock_read()
+ elif self._lock_mode == 'w':
+ self._real_branch.lock_write(token=self._lock_token)
def _translate_error(self, err, **context):
self.repository._translate_error(err, branch=self, **context)
@@ -1365,6 +1377,7 @@
return self._real_branch.get_stacked_on_url()
def lock_read(self):
+ self.repository.lock_read()
if not self._lock_mode:
self._lock_mode = 'r'
self._lock_count = 1
@@ -1393,29 +1406,20 @@
def lock_write(self, token=None):
if not self._lock_mode:
+ # Lock the branch and repo in one remote call.
remote_tokens = self._remote_lock_write(token)
self._lock_token, self._repo_lock_token = remote_tokens
if not self._lock_token:
raise SmartProtocolError('Remote server did not return a token!')
- # TODO: We really, really, really don't want to call _ensure_real
- # here, but it's the easiest way to ensure coherency between the
- # state of the RemoteBranch and RemoteRepository objects and the
- # physical locks. If we don't materialise the real objects here,
- # then getting everything in the right state later is complex, so
- # for now we just do it the lazy way.
- # -- Andrew Bennetts, 2007-02-22.
- self._ensure_real()
+ # Tell the self.repository object that it is locked.
+ self.repository.lock_write(
+ self._repo_lock_token, _skip_rpc=True)
+
if self._real_branch is not None:
- self._real_branch.repository.lock_write(
- token=self._repo_lock_token)
- try:
- self._real_branch.lock_write(token=self._lock_token)
- finally:
- self._real_branch.repository.unlock()
+ self._real_branch.lock_write(token=self._lock_token)
if token is not None:
self._leave_lock = True
else:
- # XXX: this case seems to be unreachable; token cannot be None.
self._leave_lock = False
self._lock_mode = 'w'
self._lock_count = 1
@@ -1423,11 +1427,14 @@
raise errors.ReadOnlyTransaction
else:
if token is not None:
- # A token was given to lock_write, and we're relocking, so check
- # that the given token actually matches the one we already have.
+ # A token was given to lock_write, and we're relocking, so
+ # check that the given token actually matches the one we
+ # already have.
if token != self._lock_token:
raise errors.TokenMismatch(token, self._lock_token)
self._lock_count += 1
+ # Re-lock the repository too.
+ self.repository.lock_write(self._repo_lock_token)
return self._lock_token or None
def _unlock(self, branch_token, repo_token):
@@ -1442,32 +1449,35 @@
raise errors.UnexpectedSmartServerResponse(response)
def unlock(self):
- self._lock_count -= 1
- if not self._lock_count:
- self._clear_cached_state()
- mode = self._lock_mode
- self._lock_mode = None
- if self._real_branch is not None:
- if (not self._leave_lock and mode == 'w' and
- self._repo_lock_token):
- # If this RemoteBranch will remove the physical lock for the
- # repository, make sure the _real_branch doesn't do it
- # first. (Because the _real_branch's repository is set to
- # be the RemoteRepository.)
- self._real_branch.repository.leave_lock_in_place()
- self._real_branch.unlock()
- if mode != 'w':
- # Only write-locked branched need to make a remote method call
- # to perfom the unlock.
- return
- if not self._lock_token:
- raise AssertionError('Locked, but no token!')
- branch_token = self._lock_token
- repo_token = self._repo_lock_token
- self._lock_token = None
- self._repo_lock_token = None
- if not self._leave_lock:
- self._unlock(branch_token, repo_token)
+ try:
+ self._lock_count -= 1
+ if not self._lock_count:
+ self._clear_cached_state()
+ mode = self._lock_mode
+ self._lock_mode = None
+ if self._real_branch is not None:
+ if (not self._leave_lock and mode == 'w' and
+ self._repo_lock_token):
+ # If this RemoteBranch will remove the physical lock
+ # for the repository, make sure the _real_branch
+ # doesn't do it first. (Because the _real_branch's
+ # repository is set to be the RemoteRepository.)
+ self._real_branch.repository.leave_lock_in_place()
+ self._real_branch.unlock()
+ if mode != 'w':
+ # Only write-locked branched need to make a remote method
+ # call to perfom the unlock.
+ return
+ if not self._lock_token:
+ raise AssertionError('Locked, but no token!')
+ branch_token = self._lock_token
+ repo_token = self._repo_lock_token
+ self._lock_token = None
+ self._repo_lock_token = None
+ if not self._leave_lock:
+ self._unlock(branch_token, repo_token)
+ finally:
+ self.repository.unlock()
def break_lock(self):
self._ensure_real()
@@ -1518,7 +1528,9 @@
raise errors.UnexpectedSmartServerResponse(response)
new_revno, new_revision_id = response[1:]
self._last_revision_info_cache = new_revno, new_revision_id
- self._real_branch._last_revision_info_cache = new_revno, new_revision_id
+ if self._real_branch is not None:
+ cache = new_revno, new_revision_id
+ self._real_branch._last_revision_info_cache = cache
def _set_last_revision(self, revision_id):
path = self.bzrdir._path_for_remote_call(self._client)
=== modified file 'bzrlib/tests/branch_implementations/test_locking.py'
--- a/bzrlib/tests/branch_implementations/test_locking.py 2007-11-26 20:30:08 +0000
+++ b/bzrlib/tests/branch_implementations/test_locking.py 2008-09-05 08:55:29 +0000
@@ -515,3 +515,10 @@
branch.repository.unlock()
finally:
branch.unlock()
+
+ def test_lock_and_unlock_leaves_repo_unlocked(self):
+ branch = self.make_branch('b')
+ branch.lock_write()
+ branch.unlock()
+ self.assertRaises(errors.LockNotHeld, branch.repository.unlock)
+
=== modified file 'bzrlib/tests/test_remote.py'
--- a/bzrlib/tests/test_remote.py 2008-07-25 06:42:08 +0000
+++ b/bzrlib/tests/test_remote.py 2008-09-05 10:48:03 +0000
@@ -427,7 +427,23 @@
return OldSmartClient()
-class TestBranchLastRevisionInfo(tests.TestCase):
+class RemoteBranchTestCase(tests.TestCase):
+
+ def make_remote_branch(self, transport, client):
+ """Make a RemoteBranch using 'client' as its _SmartClient.
+
+ A RemoteBzrDir and RemoteRepository will also be created to fill out
+ the RemoteBranch, albeit with stub values for some of their attributes.
+ """
+ # we do not want bzrdir to make any remote calls, so use False as its
+ # _client. If it tries to make a remote call, this will fail
+ # immediately.
+ bzrdir = RemoteBzrDir(transport, _client=False)
+ repo = RemoteRepository(bzrdir, None, _client=client)
+ return RemoteBranch(bzrdir, repo, _client=client)
+
+
+class TestBranchLastRevisionInfo(RemoteBranchTestCase):
def test_empty_branch(self):
# in an empty branch we decode the response properly
@@ -436,9 +452,7 @@
client.add_success_response('ok', '0', 'null:')
transport.mkdir('quack')
transport = transport.clone('quack')
- # we do not want bzrdir to make any remote calls
- bzrdir = RemoteBzrDir(transport, _client=False)
- branch = RemoteBranch(bzrdir, None, _client=client)
+ branch = self.make_remote_branch(transport, client)
result = branch.last_revision_info()
self.assertEqual(
@@ -454,9 +468,7 @@
client.add_success_response('ok', '2', revid)
transport.mkdir('kwaak')
transport = transport.clone('kwaak')
- # we do not want bzrdir to make any remote calls
- bzrdir = RemoteBzrDir(transport, _client=False)
- branch = RemoteBranch(bzrdir, None, _client=client)
+ branch = self.make_remote_branch(transport, client)
result = branch.last_revision_info()
self.assertEqual(
@@ -465,7 +477,7 @@
self.assertEqual((2, revid), result)
-class TestBranchSetLastRevision(tests.TestCase):
+class TestBranchSetLastRevision(RemoteBranchTestCase):
def test_set_empty(self):
# set_revision_history([]) is translated to calling
@@ -481,11 +493,7 @@
client.add_success_response('ok')
# unlock
client.add_success_response('ok')
- bzrdir = RemoteBzrDir(transport, _client=False)
- branch = RemoteBranch(bzrdir, None, _client=client)
- # This is a hack to work around the problem that RemoteBranch currently
- # unnecessarily invokes _ensure_real upon a call to lock_write.
- branch._ensure_real = lambda: None
+ branch = self.make_remote_branch(transport, client)
branch.lock_write()
client._calls = []
result = branch.set_revision_history([])
@@ -510,11 +518,7 @@
client.add_success_response('ok')
# unlock
client.add_success_response('ok')
- bzrdir = RemoteBzrDir(transport, _client=False)
- branch = RemoteBranch(bzrdir, None, _client=client)
- # This is a hack to work around the problem that RemoteBranch currently
- # unnecessarily invokes _ensure_real upon a call to lock_write.
- branch._ensure_real = lambda: None
+ branch = self.make_remote_branch(transport, client)
# Lock the branch, reset the record of remote calls.
branch.lock_write()
client._calls = []
@@ -540,10 +544,7 @@
# unlock
client.add_success_response('ok')
- bzrdir = RemoteBzrDir(transport, _client=False)
- repo = RemoteRepository(bzrdir, None, _client=client)
- branch = RemoteBranch(bzrdir, repo, _client=client)
- branch._ensure_real = lambda: None
+ branch = self.make_remote_branch(transport, client)
branch.lock_write()
client._calls = []
@@ -568,10 +569,7 @@
# unlock
client.add_success_response('ok')
- bzrdir = RemoteBzrDir(transport, _client=False)
- repo = RemoteRepository(bzrdir, None, _client=client)
- branch = RemoteBranch(bzrdir, repo, _client=client)
- branch._ensure_real = lambda: None
+ branch = self.make_remote_branch(transport, client)
branch.lock_write()
self.addCleanup(branch.unlock)
client._calls = []
@@ -586,7 +584,7 @@
self.assertEqual(rejection_msg_unicode, err.msg)
-class TestBranchSetLastRevisionInfo(tests.TestCase):
+class TestBranchSetLastRevisionInfo(RemoteBranchTestCase):
def test_set_last_revision_info(self):
# set_last_revision_info(num, 'rev-id') is translated to calling
@@ -602,11 +600,7 @@
# unlock
client.add_success_response('ok')
- bzrdir = RemoteBzrDir(transport, _client=False)
- branch = RemoteBranch(bzrdir, None, _client=client)
- # This is a hack to work around the problem that RemoteBranch currently
- # unnecessarily invokes _ensure_real upon a call to lock_write.
- branch._ensure_real = lambda: None
+ branch = self.make_remote_branch(transport, client)
# Lock the branch, reset the record of remote calls.
branch.lock_write()
client._calls = []
@@ -631,12 +625,7 @@
# unlock
client.add_success_response('ok')
- bzrdir = RemoteBzrDir(transport, _client=False)
- repo = RemoteRepository(bzrdir, None, _client=client)
- branch = RemoteBranch(bzrdir, repo, _client=client)
- # This is a hack to work around the problem that RemoteBranch currently
- # unnecessarily invokes _ensure_real upon a call to lock_write.
- branch._ensure_real = lambda: None
+ branch = self.make_remote_branch(transport, client)
# Lock the branch, reset the record of remote calls.
branch.lock_write()
client._calls = []
@@ -651,6 +640,9 @@
branch._lock_count = 2
branch._lock_token = 'branch token'
branch._repo_lock_token = 'repo token'
+ branch.repository._lock_mode = 'w'
+ branch.repository._lock_count = 2
+ branch.repository._lock_token = 'repo token'
def test_backwards_compatibility(self):
"""If the server does not support the Branch.set_last_revision_info
@@ -669,8 +661,7 @@
transport = transport.clone('branch')
client = FakeClient(transport.base)
client.add_unknown_method_response('Branch.set_last_revision_info')
- bzrdir = RemoteBzrDir(transport, _client=False)
- branch = RemoteBranch(bzrdir, None, _client=client)
+ branch = self.make_remote_branch(transport, client)
class StubRealBranch(object):
def __init__(self):
self.calls = []
@@ -707,12 +698,7 @@
# unlock
client.add_success_response('ok')
- bzrdir = RemoteBzrDir(transport, _client=False)
- repo = RemoteRepository(bzrdir, None, _client=client)
- branch = RemoteBranch(bzrdir, repo, _client=client)
- # This is a hack to work around the problem that RemoteBranch currently
- # unnecessarily invokes _ensure_real upon a call to lock_write.
- branch._ensure_real = lambda: None
+ branch = self.make_remote_branch(transport, client)
# Lock the branch, reset the record of remote calls.
branch.lock_write()
client._calls = []
@@ -738,12 +724,7 @@
# unlock
client.add_success_response('ok')
- bzrdir = RemoteBzrDir(transport, _client=False)
- repo = RemoteRepository(bzrdir, None, _client=client)
- branch = RemoteBranch(bzrdir, repo, _client=client)
- # This is a hack to work around the problem that RemoteBranch currently
- # unnecessarily invokes _ensure_real upon a call to lock_write.
- branch._ensure_real = lambda: None
+ branch = self.make_remote_branch(transport, client)
# Lock the branch, reset the record of remote calls.
branch.lock_write()
self.addCleanup(branch.unlock)
@@ -783,7 +764,7 @@
## client._calls)
-class TestBranchLockWrite(tests.TestCase):
+class TestBranchLockWrite(RemoteBranchTestCase):
def test_lock_write_unlockable(self):
transport = MemoryTransport()
@@ -791,10 +772,7 @@
client.add_error_response('UnlockableTransport')
transport.mkdir('quack')
transport = transport.clone('quack')
- # we do not want bzrdir to make any remote calls
- bzrdir = RemoteBzrDir(transport, _client=False)
- repo = RemoteRepository(bzrdir, None, _client=client)
- branch = RemoteBranch(bzrdir, repo, _client=client)
+ branch = self.make_remote_branch(transport, client)
self.assertRaises(errors.UnlockableTransport, branch.lock_write)
self.assertEqual(
[('call', 'Branch.lock_write', ('quack/', '', ''))],
More information about the bazaar-commits
mailing list