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