Rev 5760: Merge config-concrete-stores into config-stack in file:///home/vila/src/bzr/experimental/config/

Vincent Ladeuil v.ladeuil+lp at free.fr
Wed Apr 6 17:33:55 UTC 2011


At file:///home/vila/src/bzr/experimental/config/

------------------------------------------------------------
revno: 5760 [merge]
revision-id: v.ladeuil+lp at free.fr-20110406173355-hzqyw9rl4os57yhm
parent: v.ladeuil+lp at free.fr-20110406093841-h1gqat7d5ozp8hfc
parent: v.ladeuil+lp at free.fr-20110406173330-ek2qi1q6fzy5ee2m
committer: Vincent Ladeuil <v.ladeuil+lp at free.fr>
branch nick: config-stack
timestamp: Wed 2011-04-06 19:33:55 +0200
message:
  Merge config-concrete-stores into config-stack
modified:
  bzrlib/config.py               config.py-20051011043216-070c74f4e9e338e8
  bzrlib/tests/test_config.py    testconfig.py-20051011041908-742d0c15d8d8c8eb
-------------- next part --------------
=== modified file 'bzrlib/config.py'
--- a/bzrlib/config.py	2011-04-06 09:38:41 +0000
+++ b/bzrlib/config.py	2011-04-06 17:33:55 +0000
@@ -848,7 +848,7 @@
         # LockableConfig for other kind of transports, we will need to reuse
         # whatever connection is already established -- vila 20100929
         self.transport = transport.get_transport(self.dir)
-        self._lock = lockdir.LockDir(self.transport, 'lock')
+        self._lock = lockdir.LockDir(self.transport, self.lock_name)
 
     def _create_from_string(self, unicode_bytes, save):
         super(LockableConfig, self)._create_from_string(unicode_bytes, False)
@@ -2117,7 +2117,7 @@
             # parameter (bug #750169) :-/ -- vila 2011-04-04
             # The following will do in the interim but maybe we don't want to
             # expose a path here but rather a config ID and its associated
-            # object (hand wawing).
+            # object </hand wawe>.
             file_path = os.path.join(self.transport.external_url(),
                                      self.file_name)
             raise errors.ParseConfigError(e.errors, file_path)
@@ -2153,10 +2153,48 @@
         return MutableSection(section_name, section)
 
 
+class LockableConfigObjStore(ConfigObjStore):
+    """A ConfigObjStore using locks on save to ensure store integrity."""
+
+    def __init__(self, transport, file_name, lock_dir_name=None):
+        """A config Store using ConfigObj for storage.
+
+        :param transport: The transport object where the config file is located.
+
+        :param file_name: The config file basename in the transport directory.
+        """
+        if lock_dir_name is None:
+            lock_dir_name = 'lock'
+        self.lock_dir_name = lock_dir_name
+        super(LockableConfigObjStore, self).__init__(transport, file_name)
+        self._lock = lockdir.LockDir(self.transport, self.lock_dir_name)
+
+    def lock_write(self, token=None):
+        """Takes a write lock in the directory containing the config file.
+
+        If the directory doesn't exist it is created.
+        """
+        # FIXME: This doesn't check the ownership of the created directories as
+        # ensure_config_dir_exists does. It should if the transport is local
+        # -- vila 2011-04-06
+        self.transport.create_prefix()
+        return self._lock.lock_write(token)
+
+    def unlock(self):
+        self._lock.unlock()
+
+    def break_lock(self):
+        self._lock.break_lock()
+
+    @needs_write_lock
+    def save(self):
+        super(LockableConfigObjStore, self).save()
+
+
 # FIXME: global, bazaar, shouldn't that be 'user' instead or even
 # 'user_defaults' as opposed to 'user_overrides', 'system_defaults'
 # (/etc/bzr/bazaar.conf) and 'system_overrides' ? -- vila 2011-04-05
-class GlobalStore(ConfigObjStore):
+class GlobalStore(LockableConfigObjStore):
 
     def __init__(self, possible_transports=None):
         t = transport.get_transport(config_dir(),
@@ -2164,7 +2202,7 @@
         super(GlobalStore, self).__init__(t, 'bazaar.conf')
 
 
-class LocationStore(ConfigObjStore):
+class LocationStore(LockableConfigObjStore):
 
     def __init__(self, possible_transports=None):
         t = transport.get_transport(config_dir(),

=== modified file 'bzrlib/tests/test_config.py'
--- a/bzrlib/tests/test_config.py	2011-04-06 09:38:41 +0000
+++ b/bzrlib/tests/test_config.py	2011-04-06 17:33:55 +0000
@@ -831,7 +831,7 @@
         def c1_write_config_file():
             before_writing.set()
             c1_orig()
-            # The lock is held we wait for the main thread to decide when to
+            # The lock is held. We wait for the main thread to decide when to
             # continue
             after_writing.wait()
         c1._write_config_file = c1_write_config_file
@@ -864,7 +864,7 @@
        c1_orig = c1._write_config_file
        def c1_write_config_file():
            ready_to_write.set()
-           # The lock is held we wait for the main thread to decide when to
+           # The lock is held. We wait for the main thread to decide when to
            # continue
            do_writing.wait()
            c1_orig()
@@ -1889,19 +1889,45 @@
         self.assertEquals(config._Created, section.orig['foo'])
 
 
+def get_ConfigObjStore(transport, file_name, content=None):
+    """Build a ConfigObjStore.
+
+    :param transport: The transport where the store lives.
+
+    :param file_name: The name of the store.
+
+    :param content: A provided content to inject into the built store.
+
+    If provided, the content is added to the store but without saving it on
+    disk. It should be a string or a unicode string in the ConfigObj syntax.
+    While this poses a constraint on other store implementations, it keeps a
+    simple syntax usable by test writers.
+    """
+    if content is None:
+        store = config.ConfigObjStore(transport, file_name)
+    else:
+        store = config.ConfigObjStore.from_string(content, transport, file_name)
+    return store
+
+
 class TestStore(tests.TestCaseWithTransport):
 
-    # FIXME: parametrize against all valid (store, transport) combinations
-
-    def get_store(self, name=None, content=None):
-        if name is None:
-            name = 'foo.conf'
-        if content is None:
-            store = config.ConfigObjStore(self.get_transport(), name)
-        else:
-            store = config.ConfigObjStore.from_string(
-                content, self.get_transport(), name)
-        return store
+    def assertSectionContent(self, expected, section):
+        """Assert that some options have the proper values in a section."""
+        expected_name, expected_options = expected
+        self.assertEquals(expected_name, section.id)
+        self.assertEquals(
+            expected_options,
+            dict([(k, section.get(k)) for k in expected_options.keys()]))
+
+
+class TestReadonlyStore(TestStore):
+
+    scenarios = [('configobj', {'_get_store': get_ConfigObjStore})]
+
+    def get_store(self, file_name, content=None):
+        return self._get_store(
+            self.get_readonly_transport(), file_name, content=content)
 
     def test_delayed_load(self):
         self.build_tree_contents([('foo.conf', '')])
@@ -1914,68 +1940,105 @@
         store = self.get_store('foo.conf', '')
         self.assertEquals(False, store.loaded)
         store.load()
+        # We loaded the store from the provided content
         self.assertEquals(True, store.loaded)
-        # We use from_string and don't save, so the file shouldn't be created
-        self.failIfExists('foo.conf')
+
+    def test_get_no_sections_for_empty(self):
+        store = self.get_store('foo.conf', '')
+        store.load()
+        self.assertEquals([], list(store.get_sections()))
+
+    def test_get_default_section(self):
+        store = self.get_store('foo.conf', 'foo=bar')
+        sections = list(store.get_sections())
+        self.assertLength(1, sections)
+        self.assertSectionContent((None, {'foo': 'bar'}), sections[0])
+
+    def test_get_named_section(self):
+        store = self.get_store('foo.conf', '[baz]\nfoo=bar')
+        sections = list(store.get_sections())
+        self.assertLength(1, sections)
+        self.assertSectionContent(('baz', {'foo': 'bar'}), sections[0])
+
+
+class TestMutableStore(TestStore):
+
+    scenarios = [('configobj', {'_get_store': get_ConfigObjStore})]
+
+    def get_store(self, file_name, content=None):
+        return self._get_store(
+            self.get_transport(), file_name, content=content)
+
+    def test_save_empty_succeeds(self):
+        store = self.get_store('foo.conf', '')
+        store.load()
+        self.assertEquals(False, self.get_transport().has('foo.conf'))
+        store.save()
+        self.assertEquals(True, self.get_transport().has('foo.conf'))
+
+    def test_save_with_content_succeeds(self):
+        store = self.get_store('foo.conf', 'foo=bar\n')
+        store.load()
+        self.assertEquals(False, self.get_transport().has('foo.conf'))
+        store.save()
+        self.assertEquals(True, self.get_transport().has('foo.conf'))
+        modified_store = self.get_store('foo.conf')
+        sections = list(modified_store.get_sections())
+        self.assertLength(1, sections)
+        self.assertSectionContent((None, {'foo': 'bar'}), sections[0])
+
+    def test_set_option_in_empty_store(self):
+        store = self.get_store('foo.conf')
+        section = store.get_mutable_section(None)
+        section.set('foo', 'bar')
+        store.save()
+        modified_store = self.get_store('foo.conf')
+        sections = list(modified_store.get_sections())
+        self.assertLength(1, sections)
+        self.assertSectionContent((None, {'foo': 'bar'}), sections[0])
+
+    def test_set_option_in_default_section(self):
+        store = self.get_store('foo.conf', '')
+        section = store.get_mutable_section(None)
+        section.set('foo', 'bar')
+        store.save()
+        modified_store = self.get_store('foo.conf')
+        sections = list(modified_store.get_sections())
+        self.assertLength(1, sections)
+        self.assertSectionContent((None, {'foo': 'bar'}), sections[0])
+
+    def test_set_option_in_named_section(self):
+        store = self.get_store('foo.conf', '')
+        section = store.get_mutable_section('baz')
+        section.set('foo', 'bar')
+        store.save()
+        modified_store = self.get_store('foo.conf')
+        sections = list(modified_store.get_sections())
+        self.assertLength(1, sections)
+        self.assertSectionContent(('baz', {'foo': 'bar'}), sections[0])
+
+
+class TestConfigObjStore(TestStore):
 
     def test_loading_unknown_file_fails(self):
         store = config.ConfigObjStore(self.get_transport(), 'I-do-not-exist')
         self.assertRaises(errors.NoSuchFile, store.load)
 
     def test_invalid_content(self):
-        store = self.get_store('foo.conf', 'this is invalid !')
+        store = config.ConfigObjStore.from_string(
+            'this is invalid !', self.get_transport(), 'foo.conf', )
         self.assertEquals(False, store.loaded)
         exc = self.assertRaises(errors.ParseConfigError, store.load)
         self.assertEndsWith(exc.filename, 'foo.conf')
         # And the load failed
         self.assertEquals(False, store.loaded)
 
-    def test_save_empty_succeeds(self):
-        store = self.get_store('foo.conf', '')
-        store.load()
-        self.failIfExists('foo.conf')
-        store.save()
-        self.failUnlessExists('foo.conf')
-
-    def test_save_with_content_succeeds(self):
-        store = self.get_store('foo.conf', 'foo=bar\n')
-        store.load()
-        self.failIfExists('foo.conf')
-        store.save()
-        self.failUnlessExists('foo.conf')
-        # FIXME: Far too ConfigObj specific
-        self.assertFileEqual('foo = bar\n', 'foo.conf')
-
-    def test_get_no_sections_for_empty(self):
-        store = self.get_store('foo.conf', '')
-        store.load()
-        self.assertEquals([], list(store.get_sections()))
-
-    def assertSectionContent(self, expected, section):
-        """Assert that some options have the proper values in a section."""
-        expected_name, expected_options = expected
-        self.assertEquals(expected_name, section.id)
-        self.assertEquals(
-            expected_options,
-            dict([(k, section.get(k)) for k in expected_options.keys()]))
-
-    def test_get_default_section(self):
-        store = self.get_store('foo.conf', 'foo=bar')
-        sections = list(store.get_sections())
-        self.assertLength(1, sections)
-        self.assertSectionContent((None, {'foo': 'bar'}), sections[0])
-
-    def test_get_named_section(self):
-        store = self.get_store('foo.conf', '[baz]\nfoo=bar')
-        sections = list(store.get_sections())
-        self.assertLength(1, sections)
-        self.assertSectionContent(('baz', {'foo': 'bar'}), sections[0])
-
     def test_get_embedded_sections(self):
         # A more complicated example (which also shows that section names and
         # option names share the same name space...)
-        # FIXME: This is really specific to ConfigObjStore -- vila 2011-04-05
-        store = self.get_store('foo.conf', '''
+        # FIXME: This should be fixed by forbidding dicts as values ?
+        # -- vila 2011-04-05
+        store = config.ConfigObjStore.from_string('''
 foo=bar
 l=1,2
 [DEFAULT]
@@ -1986,7 +2049,7 @@
 foo_in_baz=barbaz
 [[qux]]
 foo_in_qux=quux
-''')
+''', self.get_transport(), 'foo.conf')
         sections = list(store.get_sections())
         self.assertLength(4, sections)
         # The default section has no name.
@@ -2002,26 +2065,18 @@
             ('baz', {'foo_in_baz': 'barbaz', 'qux': {'foo_in_qux': 'quux'}}),
             sections[3])
 
-    def test_set_option_in_empty_file(self):
-        store = self.get_store('foo.conf')
-        section = store.get_mutable_section(None)
-        section.set('foo', 'bar')
-        store.save()
-        self.assertFileEqual('foo = bar\n', 'foo.conf')
-
-    def test_set_option_in_default_section(self):
-        store = self.get_store('foo.conf', '')
-        section = store.get_mutable_section(None)
-        section.set('foo', 'bar')
-        store.save()
-        self.assertFileEqual('foo = bar\n', 'foo.conf')
-
-    def test_set_option_in_named_section(self):
-        store = self.get_store('foo.conf', '')
-        section = store.get_mutable_section('baz')
-        section.set('foo', 'bar')
-        store.save()
-        self.assertFileEqual('[baz]\nfoo = bar\n', 'foo.conf')
+
+class TestLockableConfigObjStore(TestStore):
+
+    def test_create_store_in_created_dir(self):
+        t = self.get_transport('dir/subdir')
+        store = config.LockableConfigObjStore(t, 'foo.conf')
+        store.get_mutable_section(None).set('foo', 'bar')
+        store.save()
+
+    # FIXME: We should adapt the tests in TestLockableConfig about concurrent
+    # writes, for now, I'll just rely on using the same code (copied, but
+    # pretty trivial) -- vila 20110-04-06
 
 
 class TestConfigObjStore(tests.TestCaseWithTransport):



More information about the bazaar-commits mailing list