Rev 6512: (vila) Properly share mutable config sections and save the branch config in file:///srv/pqm.bazaar-vcs.org/archives/thelove/bzr/%2Btrunk/
Patch Queue Manager
pqm at pqm.ubuntu.com
Wed Mar 28 16:13:50 UTC 2012
At file:///srv/pqm.bazaar-vcs.org/archives/thelove/bzr/%2Btrunk/
------------------------------------------------------------
revno: 6512 [merge]
revision-id: pqm at pqm.ubuntu.com-20120328161349-2gsc0g11fcu43hlc
parent: pqm at pqm.ubuntu.com-20120316150505-30l88aa34gy2j4hx
parent: v.ladeuil+lp at free.fr-20120328154713-yjgbccufbkol5w4s
committer: Patch Queue Manager <pqm at pqm.ubuntu.com>
branch nick: +trunk
timestamp: Wed 2012-03-28 16:13:49 +0000
message:
(vila) Properly share mutable config sections and save the branch config
only during the final unlock (Vincent Ladeuil)
modified:
bzrlib/branch.py branch.py-20050309040759-e4baf4e0d046576e
bzrlib/config.py config.py-20051011043216-070c74f4e9e338e8
bzrlib/plugins/changelog_merge/tests/test_changelog_merge.py test_changelog_merge-20110310080015-9c3muqni567c1qux-3
bzrlib/tests/test_branch.py test_branch.py-20060116013032-97819aa07b8ab3b5
bzrlib/tests/test_config.py testconfig.py-20051011041908-742d0c15d8d8c8eb
bzrlib/tests/test_merge_core.py test_merge_core.py-20050824132511-eb99b23a0eec641b
doc/developers/configuration.txt configuration.txt-20110408142435-korjxxnskvq44sta-1
doc/en/release-notes/bzr-2.6.txt bzr2.6.txt-20120116134316-8w1xxom1c7vcu1t5-1
=== modified file 'bzrlib/branch.py'
--- a/bzrlib/branch.py 2012-03-14 15:26:01 +0000
+++ b/bzrlib/branch.py 2012-03-28 16:13:49 +0000
@@ -968,8 +968,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
@@ -2495,7 +2495,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-14 09:30:48 +0000
+++ b/bzrlib/config.py 2012-03-28 16:13:49 +0000
@@ -2956,7 +2956,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>'
@@ -2982,8 +2982,9 @@
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). We use a dict here
+ # so the dirty sections can be shared by multiple callers.
+ self.dirty_sections = {}
def is_loaded(self):
"""Returns True if the Store has been loaded.
@@ -3032,7 +3033,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
@@ -3052,11 +3053,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.
@@ -3142,7 +3143,7 @@
def unload(self):
self._config_obj = None
- self.dirty_sections = []
+ self.dirty_sections = {}
def _load_content(self):
"""Load the config file bytes.
@@ -3196,8 +3197,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()
@@ -3238,13 +3238,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-14 15:26:01 +0000
+++ b/bzrlib/tests/test_branch.py 2012-03-28 16:13:49 +0000
@@ -661,10 +661,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()
@@ -679,18 +682,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-14 09:30:48 +0000
+++ b/bzrlib/tests/test_config.py 2012-03-28 16:13:49 +0000
@@ -2860,6 +2860,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/developers/configuration.txt'
--- a/doc/developers/configuration.txt 2012-01-28 15:37:27 +0000
+++ b/doc/developers/configuration.txt 2012-03-13 16:42:20 +0000
@@ -98,6 +98,13 @@
* convert the unicode string provided by the user into a suitable
representation (integer, list, etc).
+If you start migrating a given option to the config stacks, don't stop
+mid-way, all its uses should be covered (tests included). There are some
+edge cases where updates via one API will be not be seen by the other API
+(see http://pad.lv/948339 and http://pad.lv/948344 for details). Roughly,
+the old API always trigger an IO while the new one cache values to avoid
+them. This works fine as long as a given option is handled via a single API.
+
Adding a new stack
------------------
=== modified file 'doc/en/release-notes/bzr-2.6.txt'
--- a/doc/en/release-notes/bzr-2.6.txt 2012-03-16 15:05:05 +0000
+++ b/doc/en/release-notes/bzr-2.6.txt 2012-03-28 16:13:49 +0000
@@ -117,6 +117,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