Merge lp:~vila/bzr/824513-fadatasync-options into lp:bzr/2.4

[Vincent Ladeuil]

Properly handle boolean options.

This allows repository.fdatasync and dirstate.fdatasync to be
disabled by setting them to any valid boolean value in the config
files.

Great care will be needed when merging upstream as I
cherry-picked *part* of the initial boolean option implementation.

[Martin Pool (mbp)]

> This allows repository.fdatasync and dirstate.fdatasync to be disabled by setting them to any valid boolean value in the config files.

I assume you mean setting them to any valid equivalent of 'false'?

The patch looks ok.

review: Approve

[Vincent Ladeuil (vila)]

> I assume you mean setting them to any valid equivalent of 'false'?

Right, any boolean value can be used but disabling is only achieved for 'false' values of course.

[Vincent Ladeuil (vila)]

sent to pqm by email

[Vincent Ladeuil (vila)]

sent to pqm by email

[John A Meinel (jameinel)]

sent to pqm by email

[Vincent Ladeuil (vila)]

sent to pqm by email

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2011-08-19 08:12:12 +0000
+++ bzrlib/config.py	2011-08-24 08:04:25 +0000
@@ -2264,14 +2264,21 @@
     value, in which config files it can be stored, etc (TBC).
     """
 
-    def __init__(self, name, default=None):
+    def __init__(self, name, default=None, from_unicode=None):
         self.name = name
         self.default = default
+        self.from_unicode = from_unicode
 
     def get_default(self):
         return self.default
 
 
+# Predefined converters to get proper values from store
+
+def bool_from_store(unicode_str):
+    return ui.bool_from_string(unicode_str)
+
+
 # Options registry
 
 option_registry = registry.Registry()
@@ -2282,12 +2289,13 @@
     help='The command called to launch an editor to enter a message.')
 
 option_registry.register(
-    'dirstate.fdatasync', Option('dirstate.fdatasync', default=True),
+    'dirstate.fdatasync', Option('dirstate.fdatasync', default=True,
+                                 from_unicode=bool_from_store),
     help='Flush dirstate changes onto physical disk?')
 
 option_registry.register(
     'repository.fdatasync',
-    Option('repository.fdatasync', default=True),
+    Option('repository.fdatasync', default=True, from_unicode=bool_from_store),
     help='Flush repository changes onto physical disk?')
 
 
@@ -2746,15 +2754,28 @@
                     break
             if value is not None:
                 break
+        # If the option is registered, it may provide additional info about
+        # value handling
+        try:
+            opt = option_registry.get(name)
+        except KeyError:
+            # Not registered
+            opt = None
+        if (opt is not None and opt.from_unicode is not None
+            and value is not None):
+            # If a value exists and the option provides a converter, use it
+            try:
+                converted = opt.from_unicode(value)
+            except (ValueError, TypeError):
+                # Invalid values are ignored
+                converted = None
+            value = converted
         if value is None:
             # If the option is registered, it may provide a default value
-            try:
-                opt = option_registry.get(name)
-            except KeyError:
-                # Not registered
-                opt = None
             if opt is not None:
                 value = opt.get_default()
+        if opt is not None and value is None:
+            value = opt.get_default()
         for hook in ConfigHooks['get']:
             hook(self, name, value)
         return value

=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2011-08-19 08:12:12 +0000
+++ bzrlib/tests/test_config.py	2011-08-24 08:04:25 +0000
@@ -2940,23 +2940,60 @@
 
 class TestStackGet(TestStackWithTransport):
 
+    def setUp(self):
+        super(TestStackGet, self).setUp()
+        self.conf = self.get_stack(self)
+
     def test_get_for_empty_stack(self):
-        conf = self.get_stack(self)
-        self.assertEquals(None, conf.get('foo'))
+        self.assertEquals(None, self.conf.get('foo'))
 
     def test_get_hook(self):
-        conf = self.get_stack(self)
-        conf.store._load_from_string('foo=bar')
+        self.conf.store._load_from_string('foo=bar')
         calls = []
         def hook(*args):
             calls.append(args)
         config.ConfigHooks.install_named_hook('get', hook, None)
         self.assertLength(0, calls)
-        value = conf.get('foo')
+        value = self.conf.get('foo')
         self.assertEquals('bar', value)
         self.assertLength(1, calls)
-        self.assertEquals((conf, 'foo', 'bar'), calls[0])
-
+        self.assertEquals((self.conf, 'foo', 'bar'), calls[0])
+
+
+class TestStackGetWithConverter(TestStackGet):
+
+    def setUp(self):
+        super(TestStackGetWithConverter, self).setUp()
+        self.overrideAttr(config, 'option_registry', registry.Registry())
+        self.registry = config.option_registry
+
+    def register_bool_option(self, name, default):
+        b = config.Option(name, default=default,
+                          from_unicode=config.bool_from_store)
+        self.registry.register(b.name, b,  help='A boolean.')
+
+    def test_get_with_bool_not_defined_default_true(self):
+        self.register_bool_option('foo', True)
+        self.assertEquals(True, self.conf.get('foo'))
+
+    def test_get_with_bool_not_defined_default_false(self):
+        self.register_bool_option('foo', False)
+        self.assertEquals(False, self.conf.get('foo'))
+
+    def test_get_with_bool_converter_not_default(self):
+        self.register_bool_option('foo', False)
+        self.conf.store._load_from_string('foo=yes')
+        self.assertEquals(True, self.conf.get('foo'))
+
+    def test_get_with_bool_converter_invalid_string(self):
+        self.register_bool_option('foo', False)
+        self.conf.store._load_from_string('foo=not-a-boolean')
+        self.assertEquals(False, self.conf.get('foo'))
+
+    def test_get_with_bool_converter_invalid_list(self):
+        self.register_bool_option('foo', False)
+        self.conf.store._load_from_string('foo=not,a,boolean')
+        self.assertEquals(False, self.conf.get('foo'))
 
 class TestStackSet(TestStackWithTransport):
 

=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2011-08-23 15:05:57 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2011-08-24 08:04:25 +0000
@@ -35,6 +35,9 @@
 * ``config.LocationMatcher`` properly excludes unrelated sections.
   (Vincent Ladeuil, #829237)
 
+* ``dirstate.fdatasync`` and ``repository.fdatasync`` can now properly be
+  disabled. (Vincent Ladeuil, #824513)
+
 * Fix i18n use when no environment variables are set. (Jelmer Vernooij, #810701)
 
 Documentation