Merge lp:~vila/bzr/1020007-branchformat4-lock into lp:bzr

[Vincent Ladeuil]

Properly save ``branch.conf`` changes when unlocking a BzrBranch4.

I ran into bug #1020007 while working on a plugin: the all-in-one format has
an issue with the way the last unlock is recognized for ``branch.conf``
saving.

This went unnoticed as no test was explicitly testing BranchFormat4.

I've added such a test and split the lock_{read_write} and unlock methods
betwenn Branch and BzrBranch4.

This makes the weave_format plugin sole responsible for the specific lock
strategy and leave the Branch class cleaner.

Since the bug was introduced with the bug #948339 fix, only bzr.dev needs to
be fixed.

Since BzrBranch4 is not widely (to say the least) used anymore, I doubt
anyone encounter the bug. Yet, it has the potential to break backwards
compatibility and looks like a good cleanup to have (and completes the work
to isolate weaves into their decicated plugin).

[Jelmer Vernooij (jelmer)]

review: Approve

[Vincent Ladeuil (vila)]

sent to pqm by email

[John A Meinel (jameinel)]

sent to pqm by email

[Vincent Ladeuil (vila)]

sent to pqm by email

[John A Meinel (jameinel)]

sent to pqm by email

=== modified file 'bzrlib/branch.py'
--- bzrlib/branch.py	2012-06-26 15:33:56 +0000
+++ bzrlib/branch.py	2012-07-02 10:51:23 +0000
@@ -2437,9 +2437,6 @@
         """
         if not self.is_locked():
             self._note_lock('w')
-        # 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._warn_if_deprecated(self)
             self.repository.lock_write()
             took_lock = True
@@ -2460,9 +2457,6 @@
         """
         if not self.is_locked():
             self._note_lock('r')
-        # 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._warn_if_deprecated(self)
             self.repository.lock_read()
             took_lock = True
@@ -2483,12 +2477,8 @@
         try:
             self.control_files.unlock()
         finally:
-            # 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():
+                self.repository.unlock()
                 # we just released the lock
                 self._clear_cached_state()
 

=== modified file 'bzrlib/plugins/weave_fmt/branch.py'
--- bzrlib/plugins/weave_fmt/branch.py	2012-01-07 01:06:26 +0000
+++ bzrlib/plugins/weave_fmt/branch.py	2012-07-02 10:51:23 +0000
@@ -23,10 +23,17 @@
     lockable_files,
     )
 
+from bzrlib.decorators import (
+    needs_read_lock,
+    needs_write_lock,
+    only_raises,
+    )
+from bzrlib.lock import LogicalLockResult
 from bzrlib.trace import mutter
 
 from bzrlib.branch import (
     BranchFormat,
+    BranchWriteLockResult,
     FullHistoryBzrBranch,
     )
 
@@ -34,6 +41,55 @@
 class BzrBranch4(FullHistoryBzrBranch):
     """Branch format 4."""
 
+    def lock_write(self, token=None):
+        """Lock the branch for write operations.
+
+        :param token: A token to permit reacquiring a previously held and
+            preserved lock.
+        :return: A BranchWriteLockResult.
+        """
+        if not self.is_locked():
+            self._note_lock('w')
+        # All-in-one needs to always unlock/lock.
+        self.repository._warn_if_deprecated(self)
+        self.repository.lock_write()
+        try:
+            return BranchWriteLockResult(self.unlock,
+                self.control_files.lock_write(token=token))
+        except:
+            self.repository.unlock()
+            raise
+
+    def lock_read(self):
+        """Lock the branch for read operations.
+
+        :return: A bzrlib.lock.LogicalLockResult.
+        """
+        if not self.is_locked():
+            self._note_lock('r')
+        # All-in-one needs to always unlock/lock.
+        self.repository._warn_if_deprecated(self)
+        self.repository.lock_read()
+        try:
+            self.control_files.lock_read()
+            return LogicalLockResult(self.unlock)
+        except:
+            self.repository.unlock()
+            raise
+
+    @only_raises(errors.LockNotHeld, errors.LockBroken)
+    def unlock(self):
+        if self.control_files._lock_count == 2 and self.conf_store is not None:
+            self.conf_store.save_changes()
+        try:
+            self.control_files.unlock()
+        finally:
+            # All-in-one needs to always unlock/lock.
+            self.repository.unlock()
+            if not self.control_files.is_locked():
+                # we just released the lock
+                self._clear_cached_state()
+
     def _get_checkout_format(self, lightweight=False):
         """Return the most suitable metadir for a checkout of this branch.
         """

=== modified file 'bzrlib/tests/per_branch/test_config.py'
--- bzrlib/tests/per_branch/test_config.py	2011-02-18 11:24:21 +0000
+++ bzrlib/tests/per_branch/test_config.py	2012-07-02 10:51:23 +0000
@@ -35,4 +35,12 @@
         config.set_user_option('name', value_dict.copy())
         self.assertEqual(value_dict, config.get_user_option('name'))
 
+    def test_set_submit_branch(self):
+        # Make sure setting a config option persists on disk
+        b = self.make_branch('.')
+        b.set_submit_branch('foo')
+        # Refresh the branch
+        b = branch.Branch.open('.')
+        self.assertEquals('foo', b.get_submit_branch())
+
 

=== modified file 'doc/en/release-notes/bzr-2.6.txt'
--- doc/en/release-notes/bzr-2.6.txt	2012-06-26 15:40:44 +0000
+++ doc/en/release-notes/bzr-2.6.txt	2012-07-02 10:51:23 +0000
@@ -41,9 +41,13 @@
 * "bzr missing" now shows tag names when displaying revision information.
   (#559072, Neil Martinsen-Burrell)
 
+* Fix ``branch.conf`` saving when unlocking the branch for BranchFormat4.
+  (Vincent Ladeuil, #1020007)
+
 * Implement ``ResponseFile.readline`` and ``ReponseFile.tell``,
   fixing some clones over HTTP. (Jelmer Vernooij, #963769)
 
+
 Documentation
 *************