[MERGE] Add an optional 'token' keyword argument to LockableFiles.lock_write

[Robert Collins]

On Thu, 2007-04-12 at 15:37 +1000, Andrew Bennetts wrote:

> I'm also a little bit uncertain about the "leave_lock_in_place" and
> "dont_leave_lock_in_place" APIs; it would be possible to slice this problem
> differently to allow more direct manipulation of whether or not an object thinks
> its locked, but that perhaps would make it easier to get objects into invalid
> states.

I think it would. The current api is very hard to force into invalid
behaviour, and efficient. A more flexible API would probably end up
doing multiple disk reads in the common case which we should avoid.


=== modified file NEWS
--- NEWS
+++ NEWS
@@ -41,6 +41,15 @@
       bzrlib/transport/remote.py contains just the Transport classes that used
       to be in bzrlib/transport/smart.py.  (Andrew Bennetts)
  
+    * The ``lock_write`` method of ``LockableFiles`` and ``Repository`` now
+      accept a ``token`` keyword argument, so that a seperate instances of those
+      objects can share a lock if it has the right token.  (Andrew Bennetts,
+      Robert Collins)

'separate'.

+    * ``Branch.lock_write()`` now accepts a ``tokens`` keyword argument, which
+      is like the other ``lock_write`` methods, except that it also allows for
+      sharing a repository lock.  (Andrew Bennetts, Robert Collins)

I think that Branch.lock_write should only accept a single token - so the expected use will be:
branch.repository.lock_write(repo_token)
branch.lock_write(branch_token)

This makes the branch lock_write simpler and more likely to be factorable out in the future.


         
@@ -1223,13 +1239,26 @@
     def is_locked(self):
         return self.control_files.is_locked()
 
-    def lock_write(self):
-        self.repository.lock_write()
+    def lock_write(self, tokens=None):
+        if tokens is not None:
+            branch_token, repo_token = tokens
+        else:
+            branch_token = repo_token = None
+        repo_token = self.repository.lock_write(token=repo_token)
         try:
-            self.control_files.lock_write()
+            branch_token = self.control_files.lock_write(token=branch_token)
         except:
             self.repository.unlock()
             raise
+        else:
+            tokens = (branch_token, repo_token)
+            assert tokens == (None, None) or None not in tokens, (
+                'Both branch and repository locks must return tokens, or else '
+                'neither must return tokens.  Got %r.' % (tokens,))

I dont this assertion is useful. Making Branch.lock_write only return its own token would make it irrelevant anyway.





=== modified file bzrlib/lockable_files.py // last-changed:andrew.bennetts at cano
... nical.com-20070411133532-u6x6edf3dmzamnaq
--- bzrlib/lockable_files.py
+++ bzrlib/lockable_files.py
...
+    def dont_leave_in_place(self):
+        """Set this LockableFiles to clear the physical lock on unlock."""
+        # XXX: think about renaming this!
+        self._lock.dont_leave_in_place()

I dont the the XXX is needed here. If you cant think of a better name,
just deal - if someone comes up with a better name later, we can adjust
then.

...
+            # XXX: add test for the case that requires self._token_from_lock:
+            # token = x.lock_write(); assert(x.lock_write() == token)
+            self._token_from_lock = token_from_lock
+            return token_from_lock

Is that XXX done? It seems straight forward.



=== modified file bzrlib/tests/repository_implementations/test_repository.py //
...  last-changed:andrew.bennetts at canonical.com-20070411133532-u6x6edf3dmzamnaq
--- bzrlib/tests/repository_implementations/test_repository.py
+++ bzrlib/tests/repository_implementations/test_repository.py
@@ -434,6 +434,58 @@
 
 class TestRepositoryLocking(TestCaseWithRepository):
 
+    def setUp(self):
+        TestCaseWithRepository.setUp(self)
+        self.reduceLockdirTimeout()

^^ This is unneeded now.



=== modified file bzrlib/tests/test_lockable_files.py // last-changed:andrew.be
... nnetts at canonical.com-20070411133532-u6x6edf3dmzamnaq
--- bzrlib/tests/test_lockable_files.py
+++ bzrlib/tests/test_lockable_files.py
@@ -21,6 +21,7 @@
 import bzrlib.errors as errors
 from bzrlib.errors import BzrBadParameterNotString, NoSuchFile, ReadOnlyError
 from bzrlib.lockable_files import LockableFiles, TransportLock
+from bzrlib import lockdir
 from bzrlib.lockdir import LockDir
 from bzrlib.tests import TestCaseInTempDir
 from bzrlib.tests.test_transactions import DummyWeave
@@ -34,6 +35,9 @@
 # these tests are applied in each parameterized suite for LockableFiles
 class _TestLockableFiles_mixin(object):
 
+    def setUp(self):
+        self.reduceLockdirTimeout()
+

^^ This is unneeded now.


+1 with my comments addressed.

-Rob

-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070412/8bcc626b/attachment.pgp