Merge lp:~vila/bzr/config-lock-branch into lp:bzr

[Vincent Ladeuil]

Implement branch config locking.

This properly implements BranchStore locking by delegating to the associated Branch object.

Note that it takes care of not creating a ref cycle. This is not triggered today but I prefer to handle it now that I realise it.

I remove some assertions in the tests that were peaking under the covers about the lock implementation but didn't provide more checks.

[Andrew Bennetts (spiv)]

This appears to be in a broken state:

29 + self.branch = weakref.ref(branch)
30 +
31 + def lock_write(self, token=None):
32 + return self.branch.lock_write(token)

Surely that has to be “self.branch().lock_write(token)”? Does this branch pass tests?

review: Needs Fixing

[Vincent Ladeuil (vila)]

Bah, of course not (looks like I switched at the wrong time between my loom threads), already fixed in lp:~vila/bzr/config-lock-remote, but I will fix this one by not using a weakref here.

[Vincent Ladeuil (vila)]

ping, review comments addressed, next review eagerly wanted ;)

[Vincent Ladeuil (vila)]

sent to pqm by email

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2011-05-31 08:16:36 +0000
+++ bzrlib/config.py	2011-05-31 08:16:36 +0000
@@ -65,6 +65,7 @@
 import os
 import string
 import sys
+import weakref
 
 from bzrlib import commands
 from bzrlib.decorators import needs_write_lock
@@ -2364,14 +2365,28 @@
         super(LocationStore, self).__init__(t, 'locations.conf')
 
 
-# FIXME: We should rely on the branch itself to be locked (possibly checking
-# that even) but we shouldn't lock ourselves. This may make `bzr config` is
-# a bit trickier though but I punt for now -- vila 20110512
-class BranchStore(LockableIniFileStore):
+class BranchStore(IniFileStore):
 
     def __init__(self, branch):
         super(BranchStore, self).__init__(branch.control_transport,
                                           'branch.conf')
+        # FIXME: This creates a cycle -- vila 2011-05-27
+        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 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 SectionMatcher(object):
     """Select sections into a given Store.

=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2011-05-31 08:16:36 +0000
+++ bzrlib/tests/test_config.py	2011-05-31 08:16:36 +0000
@@ -75,7 +75,11 @@
 def build_branch_store(test):
     if getattr(test, 'branch', None) is None:
         test.branch = test.make_branch('branch')
-    return config.BranchStore(test.branch)
+    # Since we can be called to create different stores, we need to build them
+    # from different branch *objects*, even if they point to the same branch on
+    # disk, otherwise tests about conccurent updates won't be able to trigger
+    # LockContention
+    return config.BranchStore(branch.Branch.open('branch'))
 config.test_store_builder_registry.register('branch', build_branch_store)
 
 
@@ -87,7 +91,11 @@
 def build_branch_stack(test):
     if getattr(test, 'branch', None) is None:
         test.branch = test.make_branch('branch')
-    return config.BranchStack(test.branch)
+    # Since we can be called to create different stacks, we need to build them
+    # from different branch *objects*, even if they point to the same branch on
+    # disk, otherwise tests about conccurent updates won't be able to trigger
+    # LockContention
+    return config.BranchStack(branch.Branch.open('branch'))
 config.test_stack_builder_registry.register('branch', build_branch_stack)
 
 
@@ -2180,7 +2188,6 @@
         self.addCleanup(after_writing.set)
         t1.start()
         before_writing.wait()
-        self.assertTrue(c1.store._lock.is_held)
         self.assertRaises(errors.LockContention,
                           c2.set, 'one', 'c2')
         self.assertEquals('c1', c1.get('one'))
@@ -2216,7 +2223,6 @@
        t1.start()
        # Ensure the thread is ready to write
        ready_to_write.wait()
-       self.assertTrue(c1.store._lock.is_held)
        self.assertEquals('c1', c1.get('one'))
        # If we read during the write, we get the old value
        c2 = self.get_stack(self)