Merge lp:~vila/bzr/expand-default-true into lp:bzr

[Vincent Ladeuil]

This turns config option expansion on by default.

This turns config option expansion on by default.

This has been introduced as an opt-in feature long ago (well before we
started migrating options) and I've opt-in myself since day one.

There has been no negative feedback on this so I'd like to get rid of the
option all together since I've since never encountered a valid use case
where a refernce ({option}) needs to not be expanded.

It's trivially true for most of the options used inside brzlib *except* for
templates which obviously want to delay the option expansion.

Cleaning up the code revealed a single case where expand=False was required
(mergetools) and this was indeed a template.

I'm targetting trunk to ensure this doesn't break use cases I'm not aware of, but we may want to backport to 2.5 if nothing bad happens.

[Martin Packman (gz)]

Seems reasonable for 2.6 where any remaining issues should get shaken out. The qbzr external merge should be sharing the same config now, I'm not sure what other plugins might do their own templating.

review: Approve

[Vincent Ladeuil (vila)]

sent to pqm by email

[Vincent Ladeuil (vila)]

sent to pqm by email

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2012-03-06 19:49:19 +0000
+++ bzrlib/config.py	2012-03-08 18:36:26 +0000
@@ -195,27 +195,6 @@
         return self[section][name]
 
 
-# FIXME: Until we can guarantee that each config file is loaded once and
-# only once for a given bzrlib session, we don't want to re-read the file every
-# time we query for an option so we cache the value (bad ! watch out for tests
-# needing to restore the proper value). -- vila 20110219
-_expand_default_value = None
-def _get_expand_default_value():
-    global _expand_default_value
-    if _expand_default_value is not None:
-        return _expand_default_value
-    conf = GlobalConfig()
-    # Note that we must not use None for the expand value below or we'll run
-    # into infinite recursion. Using False really would be quite silly ;)
-    expand = conf.get_user_option_as_bool('bzr.config.expand', expand=True)
-    if expand is None:
-        # This is an opt-in feature, you *really* need to clearly say you want
-        # to activate it !
-        expand = False
-    _expand_default_value = expand
-    return expand
-
-
 class Config(object):
     """A configuration policy - what username, editor, gpg needs etc."""
 
@@ -373,7 +352,7 @@
         """Template method to provide a user option."""
         return None
 
-    def get_user_option(self, option_name, expand=None):
+    def get_user_option(self, option_name, expand=True):
         """Get a generic option - no special process, no default.
 
         :param option_name: The queried option.
@@ -382,8 +361,6 @@
 
         :returns: The value of the option.
         """
-        if expand is None:
-            expand = _get_expand_default_value()
         value = self._get_user_option(option_name)
         if expand:
             if isinstance(value, list):
@@ -637,7 +614,7 @@
         for (oname, value, section, conf_id, parser) in self._get_options():
             if oname.startswith('bzr.mergetool.'):
                 tool_name = oname[len('bzr.mergetool.'):]
-                tools[tool_name] = self.get_user_option(oname)
+                tools[tool_name] = self.get_user_option(oname, False)
         trace.mutter('loaded merge tools: %r' % tools)
         return tools
 
@@ -3658,7 +3635,7 @@
             for store, section in sections():
                 yield store, section
 
-    def get(self, name, expand=None, convert=True):
+    def get(self, name, expand=True, convert=True):
         """Return the *first* option value found in the sections.
 
         This is where we guarantee that sections coming from Store are loaded
@@ -3677,8 +3654,6 @@
         :returns: The value of the option.
         """
         # FIXME: No caching of options nor sections yet -- vila 20110503
-        if expand is None:
-            expand = _get_expand_default_value()
         value = None
         found_store = None # Where the option value has been found
         # If the option is registered, it may provide additional info about

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2012-02-23 23:26:35 +0000
+++ bzrlib/tests/__init__.py	2012-03-08 18:36:26 +0000
@@ -1026,10 +1026,6 @@
         # between tests.  We should get rid of this altogether: bug 656694. --
         # mbp 20101008
         self.overrideAttr(bzrlib.trace, '_verbosity_level', 0)
-        # Isolate config option expansion until its default value for bzrlib is
-        # settled on or a the FIXME associated with _get_expand_default_value
-        # is addressed -- vila 20110219
-        self.overrideAttr(config, '_expand_default_value', None)
         self._log_files = set()
         # Each key in the ``_counters`` dict holds a value for a different
         # counter. When the test ends, addDetail() should be used to output the

=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2012-03-06 19:49:19 +0000
+++ bzrlib/tests/test_config.py	2012-03-08 18:36:26 +0000
@@ -689,63 +689,6 @@
         self.assertFileEqual(content, 'test.conf')
 
 
-class TestIniConfigOptionExpansionDefaultValue(tests.TestCaseInTempDir):
-    """What is the default value of expand for config options.
-
-    This is an opt-in beta feature used to evaluate whether or not option
-    references can appear in dangerous place raising exceptions, disapearing
-    (and as such corrupting data) or if it's safe to activate the option by
-    default.
-
-    Note that these tests relies on config._expand_default_value being already
-    overwritten in the parent class setUp.
-    """
-
-    def setUp(self):
-        super(TestIniConfigOptionExpansionDefaultValue, self).setUp()
-        self.config = None
-        self.warnings = []
-        def warning(*args):
-            self.warnings.append(args[0] % args[1:])
-        self.overrideAttr(trace, 'warning', warning)
-
-    def get_config(self, expand):
-        c = config.GlobalConfig.from_string('bzr.config.expand=%s' % (expand,),
-                                            save=True)
-        return c
-
-    def assertExpandIs(self, expected):
-        actual = config._get_expand_default_value()
-        #self.config.get_user_option_as_bool('bzr.config.expand')
-        self.assertEquals(expected, actual)
-
-    def test_default_is_None(self):
-        self.assertEquals(None, config._expand_default_value)
-
-    def test_default_is_False_even_if_None(self):
-        self.config = self.get_config(None)
-        self.assertExpandIs(False)
-
-    def test_default_is_False_even_if_invalid(self):
-        self.config = self.get_config('<your choice>')
-        self.assertExpandIs(False)
-        # ...
-        # Huh ? My choice is False ? Thanks, always happy to hear that :D
-        # Wait, you've been warned !
-        self.assertLength(1, self.warnings)
-        self.assertEquals(
-            'Value "<your choice>" is not a boolean for "bzr.config.expand"',
-            self.warnings[0])
-
-    def test_default_is_True(self):
-        self.config = self.get_config(True)
-        self.assertExpandIs(True)
-
-    def test_default_is_False(self):
-        self.config = self.get_config(False)
-        self.assertExpandIs(False)
-
-
 class TestIniConfigOptionExpansion(tests.TestCase):
     """Test option expansion from the IniConfig level.
 

=== modified file 'doc/en/release-notes/bzr-2.6.txt'
--- doc/en/release-notes/bzr-2.6.txt	2012-03-06 18:54:54 +0000
+++ doc/en/release-notes/bzr-2.6.txt	2012-03-08 18:36:26 +0000
@@ -86,6 +86,11 @@
 .. Major internal changes, unlikely to be visible to users or plugin 
    developers, but interesting for bzr developers.
 
+* Turn config option expansion on by default. The only options for which
+  this should be disabled are templates which should already have used
+  conf.get(option, expand=False) or conf.get_user_option(option,
+  expand=False). (Vincent Ladeuil)
+
 Testing
 *******