Rev 6500: Save branch config options only during the final unlock in file:///home/vila/src/bzr/bugs/948339-config-caching/

Vincent Ladeuil v.ladeuil+lp at free.fr
Tue Mar 13 16:28:30 UTC 2012


At file:///home/vila/src/bzr/bugs/948339-config-caching/

------------------------------------------------------------
revno: 6500
revision-id: v.ladeuil+lp at free.fr-20120313162830-83ekrd4ghqru0whh
parent: pqm at pqm.ubuntu.com-20120313093600-lb7qlh29e7d7kr68
fixes bug: https://launchpad.net/bugs/948339
committer: Vincent Ladeuil <v.ladeuil+lp at free.fr>
branch nick: 948339-config-caching
timestamp: Tue 2012-03-13 17:28:30 +0100
message:
  Save branch config options only during the final unlock
-------------- next part --------------
=== modified file 'bzrlib/branch.py'
--- a/bzrlib/branch.py	2012-02-23 23:41:51 +0000
+++ b/bzrlib/branch.py	2012-03-13 16:28:30 +0000
@@ -986,8 +986,8 @@
         This means the next call to revision_history will need to call
         _gen_revision_history.
 
-        This API is semi-public; it only for use by subclasses, all other code
-        should consider it to be private.
+        This API is semi-public; it is only for use by subclasses, all other
+        code should consider it to be private.
         """
         self._revision_history_cache = None
         self._revision_id_to_revno_cache = None
@@ -2560,7 +2560,7 @@
 
     @only_raises(errors.LockNotHeld, errors.LockBroken)
     def unlock(self):
-        if self.conf_store is not None:
+        if self.control_files._lock_count == 1 and self.conf_store is not None:
             self.conf_store.save_changes()
         try:
             self.control_files.unlock()

=== modified file 'bzrlib/config.py'
--- a/bzrlib/config.py	2012-03-08 18:30:33 +0000
+++ b/bzrlib/config.py	2012-03-13 16:28:30 +0000
@@ -2965,7 +2965,7 @@
             # Report concurrent updates in an ad-hoc way. This should only
             # occurs when different processes try to update the same option
             # which is not supported (as in: the config framework is not meant
-            # to be used a sharing mechanism).
+            # to be used as a sharing mechanism).
             if expected != reloaded:
                 if actual is _DeletedOption:
                     actual = '<DELETED>'
@@ -2991,8 +2991,8 @@
     mutable_section_class = MutableSection
 
     def __init__(self):
-        # Which sections need to be saved
-        self.dirty_sections = []
+        # Which sections need to be saved (by section id)
+        self.dirty_sections = {}
 
     def is_loaded(self):
         """Returns True if the Store has been loaded.
@@ -3041,7 +3041,7 @@
         raise NotImplementedError(self.save)
 
     def _need_saving(self):
-        for s in self.dirty_sections:
+        for s in self.dirty_sections.values():
             if s.orig:
                 # At least one dirty section contains a modification
                 return True
@@ -3061,11 +3061,11 @@
         # get_mutable_section() call below.
         self.unload()
         # Apply the changes from the preserved dirty sections
-        for dirty in dirty_sections:
-            clean = self.get_mutable_section(dirty.id)
+        for section_id, dirty in dirty_sections.iteritems():
+            clean = self.get_mutable_section(section_id)
             clean.apply_changes(dirty, self)
         # Everything is clean now
-        self.dirty_sections = []
+        self.dirty_sections = {}
 
     def save_changes(self):
         """Saves the Store to persistent storage if changes occurred.
@@ -3151,7 +3151,7 @@
 
     def unload(self):
         self._config_obj = None
-        self.dirty_sections = []
+        self.dirty_sections = {}
 
     def _load_content(self):
         """Load the config file bytes.
@@ -3205,8 +3205,7 @@
         if not self._need_saving():
             return
         # Preserve the current version
-        current = self._config_obj
-        dirty_sections = list(self.dirty_sections)
+        dirty_sections = dict(self.dirty_sections.items())
         self.apply_changes(dirty_sections)
         # Save to the persistent storage
         self.save()
@@ -3247,13 +3246,16 @@
         except errors.NoSuchFile:
             # The file doesn't exist, let's pretend it was empty
             self._load_from_string('')
+        if section_id in self.dirty_sections:
+            # We already created a mutable section for this id
+            return self.dirty_sections[section_id]
         if section_id is None:
             section = self._config_obj
         else:
             section = self._config_obj.setdefault(section_id, {})
         mutable_section = self.mutable_section_class(section_id, section)
         # All mutable sections can become dirty
-        self.dirty_sections.append(mutable_section)
+        self.dirty_sections[section_id] = mutable_section
         return mutable_section
 
     def quote(self, value):

=== modified file 'bzrlib/plugins/changelog_merge/tests/test_changelog_merge.py'
--- a/bzrlib/plugins/changelog_merge/tests/test_changelog_merge.py	2012-01-27 22:18:07 +0000
+++ b/bzrlib/plugins/changelog_merge/tests/test_changelog_merge.py	2012-03-13 16:28:30 +0000
@@ -186,7 +186,10 @@
             base_text, True)
         builder.change_contents('clog-id', other=other_text, this=this_text)
         merger = builder.make_merger(merge.Merge3Merger, ['clog-id'])
-        merger.this_branch.get_config_stack().set(
+        # The following can't use config stacks until the plugin itself does
+        # ('this_branch' is already write locked at this point and as such
+        # won't write the new value to disk where get_user_option can get it).
+        merger.this_branch.get_config().set_user_option(
             'changelog_merge_files', 'ChangeLog')
         merge_hook_params = merge.MergeFileHookParams(merger, 'clog-id', None,
             'file', 'file', 'conflict')

=== modified file 'bzrlib/tests/test_branch.py'
--- a/bzrlib/tests/test_branch.py	2012-03-08 16:23:53 +0000
+++ b/bzrlib/tests/test_branch.py	2012-03-13 16:28:30 +0000
@@ -702,10 +702,13 @@
         finally:
             copy.unlock()
         self.assertFalse(self.branch.is_locked())
-        result = self.branch.get_config_stack().get('foo')
-        # Bug: https://bugs.launchpad.net/bzr/+bug/948339
-        self.expectFailure('Unlocked branches cache their configs',
-            self.assertEqual, 'bar', result)
+        # Since the branch is locked, the option value won't be saved on disk
+        # so trying to access the config of locked branch via another older
+        # non-locked branch object pointing to the same branch is not supported
+        self.assertEqual(None, self.branch.get_config_stack().get('foo'))
+        # Using a newly created branch object works as expected
+        fresh = _mod_branch.Branch.open(self.branch.base)
+        self.assertEqual('bar', fresh.get_config_stack().get('foo'))
 
     def test_set_from_config_get_from_config_stack(self):
         self.branch.lock_write()
@@ -720,18 +723,21 @@
         self.branch.lock_write()
         self.addCleanup(self.branch.unlock)
         self.branch.get_config_stack().set('foo', 'bar')
-        self.assertEqual('bar',
+        # Since the branch is locked, the option value won't be saved on disk
+        # so mixing get() and get_user_option() is broken by design.
+        self.assertEqual(None,
                          self.branch.get_config().get_user_option('foo'))
 
-    def test_set_delays_write(self):
+    def test_set_delays_write_when_branch_is_locked(self):
         self.branch.lock_write()
         self.addCleanup(self.branch.unlock)
         self.branch.get_config_stack().set('foo', 'bar')
         copy = _mod_branch.Branch.open(self.branch.base)
         result = copy.get_config_stack().get('foo')
-        # Bug: https://bugs.launchpad.net/bzr/+bug/948339
-        self.expectFailure("Config writes are not cached.", self.assertIs,
-                           None, result)
+        # Accessing from a different branch object is like accessing from a
+        # different process: the option has not been saved yet and the new
+        # value cannot be seen.
+        self.assertIs(None, result)
 
 
 class TestPullResult(tests.TestCase):

=== modified file 'bzrlib/tests/test_config.py'
--- a/bzrlib/tests/test_config.py	2012-03-08 18:30:33 +0000
+++ b/bzrlib/tests/test_config.py	2012-03-13 16:28:30 +0000
@@ -2872,6 +2872,20 @@
         store.save()
         self.assertEquals(False, self.has_store(store))
 
+    def test_mutable_section_shared(self):
+        store = self.get_store(self)
+        store._load_from_string('foo=bar\n')
+        # FIXME: There should be a better way than relying on the test
+        # parametrization to identify branch.conf -- vila 2011-0526
+        if self.store_id in ('branch', 'remote_branch'):
+            # branch stores requires write locked branches
+            self.addCleanup(store.branch.lock_write().unlock)
+        section1 = store.get_mutable_section(None)
+        section2 = store.get_mutable_section(None)
+        # If we get different sections, different callers won't share the
+        # modification
+        self.assertIs(section1, section2)
+
     def test_save_emptied_succeeds(self):
         store = self.get_store(self)
         store._load_from_string('foo=bar\n')

=== modified file 'bzrlib/tests/test_merge_core.py'
--- a/bzrlib/tests/test_merge_core.py	2012-02-23 23:26:35 +0000
+++ b/bzrlib/tests/test_merge_core.py	2012-03-13 16:28:30 +0000
@@ -43,6 +43,7 @@
 
 
 class MergeBuilder(object):
+
     def __init__(self, dir=None):
         self.dir = osutils.mkdtemp(prefix="merge-test", dir=dir)
         self.tree_root = generate_ids.gen_root_id()

=== modified file 'doc/en/release-notes/bzr-2.6.txt'
--- a/doc/en/release-notes/bzr-2.6.txt	2012-03-12 13:38:09 +0000
+++ b/doc/en/release-notes/bzr-2.6.txt	2012-03-13 16:28:30 +0000
@@ -59,6 +59,9 @@
 * Fix ``bzr config`` display for ``RegistryOption`` values.
   (Vincent Ladeuil, #930182)
 
+* Option values set on locked branches should be saved only when the branch
+  is finally unlocked. (Vincent Ladeuil, #948339)
+
 Documentation
 *************
 



More information about the bazaar-commits mailing list