Merge lp:~vila/bzr/930182-display-reg-options into lp:bzr

[Vincent Ladeuil]

Fix RegistryOption display in bzr config output

bzr config was still relying too much on configobj for proper quoting.

After a bit if ping-pong to fix quoting fallouts, this proposal fixes bzr
config display for all values by *not* calling (indirectly) the
``from_unicode`` method for registered options.

Thanks to Jelmer for pushing me forward on this one as I first mistakenly
thought that stack.get() and stack.set() should be symmetric.

For the record, they are not and 'from_unicode' have been introduced
precisely to make dev's life easier. ``bool_from_store`` for example accepts
'off' to means False which already breaks this symmetry ('False' is an
accepted string for booleans which makes this fact a little less obvious).

stack.set() still relies on stringification for bool, int and lists and that
seem to be the most pragmatic thing to do.

A little care should still be taken when setting values for RegistryOptions
and devs have to remember that the values stored are keys for the registry
not object themselves (we can't (or rather don't want to)) store objects in
text files but this doesn't have to force them to avoid writing from_unicode
helpers.

[Vincent Ladeuil (vila)]

Wow, I just realized there was a nasty potential bug in the option expansion: conversion should never be attempted during expansion where all values should remain strings. The conversion should be done only for the outer get() call.

The fix is a one-liner.

[Jelmer Vernooij (jelmer)]

+ # Ensuring lazy loading is achieved by delaying section matching (which
s/Ensuring/Ensure/

+ # happen while expanding. Conversion should only occur for the origianl
^ typo

It would be nice to have some sort of test for iter_sections().

review: Needs Fixing

[Vincent Ladeuil (vila)]

> + # Ensuring lazy loading is achieved by delaying section matching
> (which
> s/Ensuring/Ensure/
 Are you sure ? 'Ensure ... is achieved' sounds weird.

>
> + # happen while expanding. Conversion should only occur for the
> origianl
> ^ typo

Thanks.
>
> It would be nice to have some sort of test for iter_sections().

Will do.

[Vincent Ladeuil (vila)]

sent to pqm by email

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2012-02-18 16:55:04 +0000
+++ bzrlib/config.py	2012-02-20 17:21:48 +0000
@@ -3645,7 +3645,17 @@
         self.store = store
         self.mutable_section_id = mutable_section_id
 
-    def get(self, name, expand=None):
+    def iter_sections(self):
+        """Iterate all the defined sections."""
+        # Ensuring lazy loading is achieved by delaying section matching (which
+        # implies querying the persistent storage) until it can't be avoided
+        # anymore by using callables to describe (possibly empty) section
+        # lists.
+        for sections in self.sections_def:
+            for store, section in sections():
+                yield store, section
+
+    def get(self, name, expand=None, convert=True):
         """Return the *first* option value found in the sections.
 
         This is where we guarantee that sections coming from Store are loaded
@@ -3658,6 +3668,9 @@
 
         :param expand: Whether options references should be expanded.
 
+        :param convert: Whether the option value should be converted from
+            unicode (do nothing for non-registered options).
+
         :returns: The value of the option.
         """
         # FIXME: No caching of options nor sections yet -- vila 20110503
@@ -3686,7 +3699,7 @@
                                       % (name, type(val)))
                 if opt is None:
                     val = found_store.unquote(val)
-                else:
+                elif convert:
                     val = opt.convert_from_unicode(found_store, val)
             return val
 
@@ -3696,17 +3709,10 @@
             value = opt.get_override()
             value = expand_and_convert(value)
         if value is None:
-            # Ensuring lazy loading is achieved by delaying section matching
-            # (which implies querying the persistent storage) until it can't be
-            # avoided anymore by using callables to describe (possibly empty)
-            # section lists.
-            for sections in self.sections_def:
-                for store, section in sections():
-                    value = section.get(name)
-                    if value is not None:
-                        found_store = store
-                        break
+            for store, section in self.iter_sections():
+                value = section.get(name)
                 if value is not None:
+                    found_store = store
                     break
             value = expand_and_convert(value)
             if opt is not None and value is None:
@@ -3778,7 +3784,7 @@
             # anything else
             value = env[name]
         else:
-            value = self.get(name, expand=False)
+            value = self.get(name, expand=False, convert=False)
             value = self._expand_options_in_string(value, env, _refs)
         return value
 
@@ -4025,11 +4031,6 @@
         self.store.save_changes()
 
 
-# Use a an empty dict to initialize an empty configobj avoiding all
-# parsing and encoding checks
-_quoting_config = configobj.ConfigObj(
-    {}, encoding='utf-8', interpolation=False, list_values=True)
-
 class cmd_config(commands.Command):
     __doc__ = """Display, set or remove a configuration option.
 
@@ -4133,12 +4134,17 @@
             except errors.NotBranchError:
                 return LocationStack(directory)
 
+    def _quote_multiline(self, value):
+        if '\n' in value:
+            value = '"""' + value + '"""'
+        return value
+
     def _show_value(self, name, directory, scope):
         conf = self._get_stack(directory, scope)
-        value = conf.get(name, expand=True)
+        value = conf.get(name, expand=True, convert=False)
         if value is not None:
             # Quote the value appropriately
-            value = _quoting_config._quote(value)
+            value = self._quote_multiline(value)
             self.outf.write('%s\n' % (value,))
         else:
             raise errors.NoSuchConfigOption(name)
@@ -4152,31 +4158,23 @@
         cur_store_id = None
         cur_section = None
         conf = self._get_stack(directory, scope)
-        for sections in conf.sections_def:
-            for store, section in sections():
-                for oname in section.iter_option_names():
-                    if name.search(oname):
-                        if cur_store_id != store.id:
-                            # Explain where the options are defined
-                            self.outf.write('%s:\n' % (store.id,))
-                            cur_store_id = store.id
-                            cur_section = None
-                        if (section.id is not None
-                            and cur_section != section.id):
-                            # Display the section id as it appears in the store
-                            # (None doesn't appear by definition)
-                            self.outf.write('  [%s]\n' % (section.id,))
-                            cur_section = section.id
-                        value = section.get(oname, expand=False)
-                        # Since we don't use the stack, we need to restore a
-                        # proper quoting.
-                        try:
-                            opt = option_registry.get(oname)
-                            value = opt.convert_from_unicode(store, value)
-                        except KeyError:
-                            value = store.unquote(value)
-                        value = _quoting_config._quote(value)
-                        self.outf.write('  %s = %s\n' % (oname, value))
+        for store, section in conf.iter_sections():
+            for oname in section.iter_option_names():
+                if name.search(oname):
+                    if cur_store_id != store.id:
+                        # Explain where the options are defined
+                        self.outf.write('%s:\n' % (store.id,))
+                        cur_store_id = store.id
+                        cur_section = None
+                    if (section.id is not None and cur_section != section.id):
+                        # Display the section id as it appears in the store
+                        # (None doesn't appear by definition)
+                        self.outf.write('  [%s]\n' % (section.id,))
+                        cur_section = section.id
+                    value = section.get(oname, expand=False)
+                    # Quote the value appropriately
+                    value = self._quote_multiline(value)
+                    self.outf.write('  %s = %s\n' % (oname, value))
 
     def _set_config_option(self, name, value, directory, scope):
         conf = self._get_stack(directory, scope, write_access=True)

=== modified file 'bzrlib/tests/blackbox/test_config.py'
--- bzrlib/tests/blackbox/test_config.py	2012-01-03 12:56:06 +0000
+++ bzrlib/tests/blackbox/test_config.py	2012-02-20 17:21:48 +0000
@@ -95,7 +95,7 @@
             """
             ''')
 
-    def test_list_all_values(self):
+    def test_list_value_all(self):
         config.option_registry.register(config.ListOption('list'))
         self.addCleanup(config.option_registry.remove, 'list')
         self.bazaar_config.set_user_option('list', [1, 'a', 'with, a comma'])
@@ -106,7 +106,7 @@
               list = 1, a, "with, a comma"
             ''')
 
-    def test_list_value_only(self):
+    def test_list_value_one(self):
         config.option_registry.register(config.ListOption('list'))
         self.addCleanup(config.option_registry.remove, 'list')
         self.bazaar_config.set_user_option('list', [1, 'a', 'with, a comma'])
@@ -115,6 +115,24 @@
             1, a, "with, a comma"
             ''')
 
+    def test_registry_value_all(self):
+        self.bazaar_config.set_user_option('bzr.transform.orphan_policy',
+                                           u'move')
+        script.run_script(self, '''\
+            $ bzr config -d tree
+            bazaar:
+              [DEFAULT]
+              bzr.transform.orphan_policy = move
+            ''')
+
+    def test_registry_value_one(self):
+        self.bazaar_config.set_user_option('bzr.transform.orphan_policy',
+                                           u'move')
+        script.run_script(self, '''\
+            $ bzr config -d tree bzr.transform.orphan_policy
+            move
+            ''')
+
     def test_bazaar_config(self):
         self.bazaar_config.set_user_option('hello', 'world')
         script.run_script(self, '''\

=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2012-02-18 16:55:04 +0000
+++ bzrlib/tests/test_config.py	2012-02-20 17:21:48 +0000
@@ -3674,6 +3674,41 @@
         self.assertEquals('bar', conf.get('foo'))
 
 
+class TestStackIterSections(tests.TestCase):
+
+    def test_empty_stack(self):
+        conf = config.Stack([])
+        sections = list(conf.iter_sections())
+        self.assertLength(0, sections)
+
+    def test_empty_store(self):
+        store = config.IniFileStore()
+        store._load_from_string('')
+        conf = config.Stack([store.get_sections])
+        sections = list(conf.iter_sections())
+        self.assertLength(0, sections)
+
+    def test_simple_store(self):
+        store = config.IniFileStore()
+        store._load_from_string('foo=bar')
+        conf = config.Stack([store.get_sections])
+        tuples = list(conf.iter_sections())
+        self.assertLength(1, tuples)
+        (found_store, found_section) = tuples[0]
+        self.assertIs(store, found_store)
+
+    def test_two_stores(self):
+        store1 = config.IniFileStore()
+        store1._load_from_string('foo=bar')
+        store2 = config.IniFileStore()
+        store2._load_from_string('bar=qux')
+        conf = config.Stack([store1.get_sections, store2.get_sections])
+        tuples = list(conf.iter_sections())
+        self.assertLength(2, tuples)
+        self.assertIs(store1, tuples[0][0])
+        self.assertIs(store2, tuples[1][0])
+
+
 class TestStackWithTransport(tests.TestCaseWithTransport):
 
     scenarios = [(key, {'get_stack': builder}) for key, builder
@@ -3959,8 +3994,11 @@
 baz=end
 list={foo}
 ''')
-        self.registry.register(
-            config.ListOption('list'))
+        self.registry.register(config.ListOption('list'))
+        # Register an intermediate option as a list to ensure no conversion
+        # happen while expanding. Conversion should only occur for the original
+        # option ('list' here).
+        self.registry.register(config.ListOption('baz'))
         self.assertEquals(['start', 'middle', 'end'],
                            self.conf.get('list', expand=True))
 

=== modified file 'doc/en/release-notes/bzr-2.6.txt'
--- doc/en/release-notes/bzr-2.6.txt	2012-02-14 17:49:28 +0000
+++ doc/en/release-notes/bzr-2.6.txt	2012-02-20 17:21:48 +0000
@@ -40,6 +40,9 @@
 .. Fixes for situations where bzr would previously crash or give incorrect
    or undesirable results.
 
+* Fix ``bzr config`` display for ``RegistryOption`` values.
+  (Vincent Ladeuil, #930182)
+
 Documentation
 *************