Rev 5333: Final cleanup. in file:///home/vila/src/bzr/bugs/525571-lock-bazaar-conf-files/

[Vincent Ladeuil]

At file:///home/vila/src/bzr/bugs/525571-lock-bazaar-conf-files/

------------------------------------------------------------
revno: 5333
revision-id: v.ladeuil+lp at free.fr-20100630145307-kivkluj078vsl07d
parent: v.ladeuil+lp at free.fr-20100630141957-su7t86t2fo1c1n1u
committer: Vincent Ladeuil <v.ladeuil+lp at free.fr>
branch nick: 525571-lock-bazaar-conf-files
timestamp: Wed 2010-06-30 16:53:07 +0200
message:
  Final cleanup.
-------------- next part --------------
=== modified file 'NEWS'
--- a/NEWS	2010-06-30 10:29:33 +0000
+++ b/NEWS	2010-06-30 14:53:07 +0000
@@ -36,8 +36,8 @@
 New Features
 ************
 
-* ``bzr break-lock`` now defines a --config parameter that is required
-  to break locks on config files. (Vincent Ladeuil, #525571)
+* ``bzr break-lock --conf [location]`` can now break config files
+  locks. (Vincent Ladeuil, #525571)
 
 * ``bzrlib.config.LockableConfig`` is a base class for config files that
   needs to be protected against multiple writers. All methods that

=== modified file 'bzrlib/builtins.py'
--- a/bzrlib/builtins.py	2010-06-30 14:19:57 +0000
+++ b/bzrlib/builtins.py	2010-06-30 14:53:07 +0000
@@ -4850,7 +4850,7 @@
     takes_args = ['location?']
     takes_options = [
         Option('conf',
-               help='LOCATION is a directory containing configuration files.'),
+               help='LOCATION is the directory where the config lock is.'),
         ]
 
     def run(self, location=None, conf=False):

=== modified file 'bzrlib/config.py'
--- a/bzrlib/config.py	2010-06-30 14:19:57 +0000
+++ b/bzrlib/config.py	2010-06-30 14:53:07 +0000
@@ -386,6 +386,11 @@
         self._parser = p
         return p
 
+    def reload(self):
+        """Reload the config file from disk."""
+        if self._parser is not None:
+            self._parser.reload()
+
     def _get_matching_sections(self):
         """Return an ordered list of (section_name, extra_path) pairs.
 
@@ -511,6 +516,22 @@
 
     If several processes try to write the config file, the accesses need to be
     serialized.
+
+    Daughter classes should decorate all methods that update a config with the
+    ``@needs_write_lock`` decorator (they call, directly or indirectly, the
+    ``_write_config_file()`` method. These methods (typically ``set_option()``
+    and variants must reload the config file from disk before calling
+    ``_write_config_file()``, this can be achieved by calling the
+    ``self.reload()`` method.
+
+    This should be enough to implement the following logic:
+    - lock for exclusive write access,
+    - reload the config file from disk,
+    - set the new value
+    - unlock
+
+    This logic guarantees that a writer can update a value without erasing an
+    update made by another writer.
     """
 
     lock_name = 'lock'
@@ -522,6 +543,10 @@
         self._lock = None
 
     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.
+        """
         if self._lock is None:
             ensure_config_dir_exists(self.dir)
             self._lock = lockdir.LockDir(self.transport, self.lock_name)
@@ -567,8 +592,9 @@
 
     def get_aliases(self):
         """Return the aliases section."""
-        if 'ALIASES' in self._get_parser():
-            return self._get_parser()['ALIASES']
+        p = self._get_parser()
+        if 'ALIASES' in p:
+            return p['ALIASES']
         else:
             return {}
 
@@ -580,6 +606,7 @@
     @needs_write_lock
     def unset_alias(self, alias_name):
         """Unset an existing alias."""
+        self.reload()
         aliases = self._get_parser().get('ALIASES')
         if not aliases or alias_name not in aliases:
             raise errors.NoSuchAlias(alias_name)
@@ -587,8 +614,7 @@
         self._write_config_file()
 
     def _set_option(self, option, value, section):
-        if self._parser is not None:
-            self._parser.reload()
+        self.reload()
         self._get_parser().setdefault(section, {})[option] = value
         self._write_config_file()
 
@@ -717,17 +743,16 @@
                          STORE_LOCATION_APPENDPATH]:
             raise ValueError('bad storage policy %r for %r' %
                 (store, option))
-        if self._parser is not None:
-            self._parser.reload()
+        self.reload()
+        p = self._get_parser()
         location = self.location
         if location.endswith('/'):
             location = location[:-1]
-        if (not location in self._get_parser() and
-            not location + '/' in self._get_parser()):
-            self._get_parser()[location]={}
-        elif location + '/' in self._get_parser():
+        if not location in p and not location + '/' in p:
+            p[location] = {}
+        elif location + '/' in p:
             location = location + '/'
-        self._get_parser()[location][option]=value
+        p[location][option] = value
         # the allowed values of store match the config policies
         self._set_option_policy(location, option, store)
         self._write_config_file()