Rev 6386: Stores allow Stacks to control when values are quoted/unquoted in file:///home/vila/src/bzr/experimental/quoting-stores/
Vincent Ladeuil
v.ladeuil+lp at free.fr
Wed Dec 21 08:52:43 UTC 2011
At file:///home/vila/src/bzr/experimental/quoting-stores/
------------------------------------------------------------
revno: 6386
revision-id: v.ladeuil+lp at free.fr-20111221085241-d6971vcz327saju1
parent: pqm at pqm.ubuntu.com-20111219133031-97tc08321g9zz6dd
fixes bug: https://launchpad.net/bugs/906897
committer: Vincent Ladeuil <v.ladeuil+lp at free.fr>
branch nick: quoting-stores
timestamp: Wed 2011-12-21 09:52:41 +0100
message:
Stores allow Stacks to control when values are quoted/unquoted
-------------- next part --------------
=== modified file 'bzrlib/config.py'
--- a/bzrlib/config.py 2011-12-16 14:04:01 +0000
+++ b/bzrlib/config.py 2011-12-21 08:52:41 +0000
@@ -2340,7 +2340,8 @@
"""
def __init__(self, name, default=None, default_from_env=None,
- help=None, from_unicode=None, invalid=None):
+ help=None, from_unicode=None, invalid=None,
+ unquote=True):
"""Build an option definition.
:param name: the name used to refer to the option.
@@ -2368,6 +2369,11 @@
TypeError. Accepted values are: None (ignore invalid values),
'warning' (emit a warning), 'error' (emit an error message and
terminates).
+
+ :param unquote: should the unicode value be unquoted before conversion.
+ This should be used only when the store providing the values cannot
+ safely unquote them (see http://pad.lv/906897). It is provided so
+ daughter classes can handle the quoting themselves.
"""
if default_from_env is None:
default_from_env = []
@@ -2394,11 +2400,14 @@
self.default_from_env = default_from_env
self.help = help
self.from_unicode = from_unicode
+ self.unquote = unquote
if invalid and invalid not in ('warning', 'error'):
raise AssertionError("%s not supported for 'invalid'" % (invalid,))
self.invalid = invalid
- def convert_from_unicode(self, unicode_value):
+ def convert_from_unicode(self, store, unicode_value):
+ if self.unquote and store is not None and unicode_value is not None:
+ unicode_value = store.unquote(unicode_value)
if self.from_unicode is None or unicode_value is None:
# Don't convert or nothing to convert
return unicode_value
@@ -2465,28 +2474,41 @@
{}, encoding='utf-8', list_values=True, interpolation=False)
-def list_from_store(unicode_str):
- if not isinstance(unicode_str, basestring):
- raise TypeError
- # Now inject our string directly as unicode. All callers got their value
- # from configobj, so values that need to be quoted are already properly
- # quoted.
- _list_converter_config.reset()
- _list_converter_config._parse([u"list=%s" % (unicode_str,)])
- maybe_list = _list_converter_config['list']
- # ConfigObj return '' instead of u''. Use 'str' below to catch all cases.
- if isinstance(maybe_list, basestring):
- if maybe_list:
- # A single value, most probably the user forgot (or didn't care to
- # add) the final ','
- l = [maybe_list]
+class ListOption(Option):
+
+ def __init__(self, name, default=None, default_from_env=None,
+ help=None, invalid=None):
+ """A list Option definition.
+
+ This overrides the base class so the conversion from a unicode string
+ can take quoting into account.
+ """
+ super(ListOption, self).__init__(
+ name, default=default, default_from_env=default_from_env,
+ from_unicode=self.from_unicode, help=help,
+ invalid=invalid, unquote=False)
+
+ def from_unicode(self, unicode_value):
+ if not isinstance(unicode_value, basestring):
+ raise TypeError
+ # Now inject our string directly as unicode. All callers got their
+ # value from configobj, so values that need to be quoted are already
+ # properly quoted.
+ _list_converter_config.reset()
+ _list_converter_config._parse([u"list=%s" % (unicode_value,)])
+ maybe_list = _list_converter_config['list']
+ if isinstance(maybe_list, basestring):
+ if maybe_list:
+ # A single value, most probably the user forgot (or didn't care
+ # to add) the final ','
+ l = [maybe_list]
+ else:
+ # The empty string, convert to empty list
+ l = []
else:
- # The empty string, convert to empty list
- l = []
- else:
- # We rely on ConfigObj providing us with a list already
- l = maybe_list
- return l
+ # We rely on ConfigObj providing us with a list already
+ l = maybe_list
+ return l
class OptionRegistry(registry.Registry):
@@ -2542,8 +2564,8 @@
existing mainline of the branch.
'''))
option_registry.register(
- Option('acceptable_keys',
- default=None, from_unicode=list_from_store,
+ ListOption('acceptable_keys',
+ default=None,
help="""\
List of GPG key patterns which are acceptable for verification.
"""))
@@ -2593,7 +2615,7 @@
should not be lost if the machine crashes. See also repository.fdatasync.
'''))
option_registry.register(
- Option('debug_flags', default=[], from_unicode=list_from_store,
+ ListOption('debug_flags', default=[],
help='Debug flags to activate.'))
option_registry.register(
Option('default_format', default='2a',
@@ -2803,6 +2825,20 @@
"""
raise NotImplementedError(self.unload)
+ def quote(self, value):
+ """Quote a configuration option value for storing purposes.
+
+ This allows Stacks to present values as they will be stored.
+ """
+ return value
+
+ def unquote(self, value):
+ """Unquote a configuration option value into unicode.
+
+ The received value is quoted as stored.
+ """
+ return value
+
def save(self):
"""Saves the Store to persistent storage."""
raise NotImplementedError(self.save)
@@ -2975,6 +3011,20 @@
section = self._config_obj.setdefault(section_id, {})
return self.mutable_section_class(section_id, section)
+ def quote(self, value):
+ try:
+ # configobj conflates automagical list values and quoting
+ self._config_obj.list_values = True
+ return self._config_obj._quote(value)
+ finally:
+ self._config_obj.list_values = False
+
+ def unquote(self, value):
+ if value:
+ # _unquote doesn't handle None nor empty strings
+ value = self._config_obj._unquote(value)
+ return value
+
class TransportIniFileStore(IniFileStore):
"""IniFileStore that loads files from a transport.
@@ -3309,10 +3359,12 @@
# implies querying the persistent storage) until it can't be avoided
# anymore by using callables to describe (possibly empty) section
# lists.
+ found_store = None # Where the option value has been found
for sections in self.sections_def:
for store, section in sections():
value = section.get(name)
if value is not None:
+ found_store = store
break
if value is not None:
break
@@ -3334,8 +3386,10 @@
trace.warning('Cannot expand "%s":'
' %s does not support option expansion'
% (name, type(val)))
- if opt is not None:
- val = opt.convert_from_unicode(val)
+ if opt is None:
+ val = found_store.unquote(val)
+ else:
+ val = opt.convert_from_unicode(found_store, val)
return val
value = expand_and_convert(value)
if opt is not None and value is None:
@@ -3419,19 +3473,20 @@
or deleting an option. In practice the store will often be loaded but
this helps catching some programming errors.
"""
- section = self.store.get_mutable_section(self.mutable_section_id)
- return section
+ store = self.store
+ section = store.get_mutable_section(self.mutable_section_id)
+ return store, section
def set(self, name, value):
"""Set a new value for the option."""
- section = self._get_mutable_section()
- section.set(name, value)
+ store, section = self._get_mutable_section()
+ section.set(name, store.quote(value))
for hook in ConfigHooks['set']:
hook(self, name, value)
def remove(self, name):
"""Remove an existing option."""
- section = self._get_mutable_section()
+ _, section = self._get_mutable_section()
section.remove(name)
for hook in ConfigHooks['remove']:
hook(self, name)
@@ -3593,10 +3648,11 @@
bstore)
self.branch = branch
+
# 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)
+ {}, encoding='utf-8', interpolation=False, list_values=True)
class cmd_config(commands.Command):
__doc__ = """Display, set or remove a configuration option.
@@ -3729,6 +3785,13 @@
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))
=== modified file 'bzrlib/plugins/po_merge/po_merge.py'
--- a/bzrlib/plugins/po_merge/po_merge.py 2011-11-29 10:41:36 +0000
+++ b/bzrlib/plugins/po_merge/po_merge.py 2011-12-21 08:52:41 +0000
@@ -56,9 +56,8 @@
''')
-po_dirs_option = config.Option(
+po_dirs_option = config.ListOption(
'po_merge.po_dirs', default='po,debian/po',
- from_unicode=config.list_from_store,
help='List of dirs containing .po files that the hook applies to.')
=== modified file 'bzrlib/tests/blackbox/test_config.py'
--- a/bzrlib/tests/blackbox/test_config.py 2011-12-14 12:15:44 +0000
+++ b/bzrlib/tests/blackbox/test_config.py 2011-12-21 08:52:41 +0000
@@ -95,22 +95,22 @@
''')
def test_list_all_values(self):
- # FIXME: we should register the option as a list or it's displayed as
- # astring and as such, quoted.
+ 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'])
script.run_script(self, '''\
$ bzr config -d tree
bazaar:
- list = '1, a, "with, a comma"'
+ list = 1, a, "with, a comma"
''')
def test_list_value_only(self):
- # FIXME: we should register the option as a list or it's displayed as
- # astring and as such, quoted.
+ 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'])
script.run_script(self, '''\
$ bzr config -d tree list
- '1, a, "with, a comma"'
+ 1, a, "with, a comma"
''')
def test_bazaar_config(self):
@@ -142,6 +142,7 @@
hello = world
''')
+
class TestConfigDisplayWithPolicy(tests.TestCaseWithTransport):
def test_location_with_policy(self):
=== modified file 'bzrlib/tests/test_config.py'
--- a/bzrlib/tests/test_config.py 2011-12-15 11:53:48 +0000
+++ b/bzrlib/tests/test_config.py 2011-12-21 08:52:41 +0000
@@ -2358,169 +2358,74 @@
class TestOptionConverterMixin(object):
def assertConverted(self, expected, opt, value):
- self.assertEquals(expected, opt.convert_from_unicode(value))
-
- def assertWarns(self, opt, value):
- warnings = []
- def warning(*args):
- warnings.append(args[0] % args[1:])
- self.overrideAttr(trace, 'warning', warning)
- self.assertEquals(None, opt.convert_from_unicode(value))
- self.assertLength(1, warnings)
- self.assertEquals(
- 'Value "%s" is not valid for "%s"' % (value, opt.name),
- warnings[0])
-
- def assertErrors(self, opt, value):
- self.assertRaises(errors.ConfigOptionValueError,
- opt.convert_from_unicode, value)
-
- def assertConvertInvalid(self, opt, invalid_value):
- opt.invalid = None
- self.assertEquals(None, opt.convert_from_unicode(invalid_value))
- opt.invalid = 'warning'
- self.assertWarns(opt, invalid_value)
- opt.invalid = 'error'
- self.assertErrors(opt, invalid_value)
-
-
-class TestOptionWithBooleanConverter(tests.TestCase, TestOptionConverterMixin):
-
- def get_option(self):
- return config.Option('foo', help='A boolean.',
- from_unicode=config.bool_from_store)
-
- def test_convert_invalid(self):
- opt = self.get_option()
- # A string that is not recognized as a boolean
- self.assertConvertInvalid(opt, u'invalid-boolean')
- # A list of strings is never recognized as a boolean
- self.assertConvertInvalid(opt, [u'not', u'a', u'boolean'])
-
- def test_convert_valid(self):
- opt = self.get_option()
- self.assertConverted(True, opt, u'True')
- self.assertConverted(True, opt, u'1')
- self.assertConverted(False, opt, u'False')
-
-
-class TestOptionWithIntegerConverter(tests.TestCase, TestOptionConverterMixin):
-
- def get_option(self):
- return config.Option('foo', help='An integer.',
- from_unicode=config.int_from_store)
-
- def test_convert_invalid(self):
- opt = self.get_option()
- # A string that is not recognized as an integer
- self.assertConvertInvalid(opt, u'forty-two')
- # A list of strings is never recognized as an integer
- self.assertConvertInvalid(opt, [u'a', u'list'])
-
- def test_convert_valid(self):
- opt = self.get_option()
- self.assertConverted(16, opt, u'16')
-
-
-class TestOptionWithListConverter(tests.TestCase, TestOptionConverterMixin):
-
- def get_option(self):
- return config.Option('foo', help='A list.',
- from_unicode=config.list_from_store)
-
- def test_convert_invalid(self):
- # No string is invalid as all forms can be converted to a list
- pass
-
- def test_convert_valid(self):
- opt = self.get_option()
- # An empty string is an empty list
- self.assertConverted([], opt, '') # Using a bare str() just in case
- self.assertConverted([], opt, u'')
- # A boolean
- self.assertConverted([u'True'], opt, u'True')
- # An integer
- self.assertConverted([u'42'], opt, u'42')
- # A single string
- self.assertConverted([u'bar'], opt, u'bar')
- # A list remains a list (configObj will turn a string containing commas
- # into a list, but that's not what we're testing here)
- self.assertConverted([u'foo', u'1', u'True'],
- opt, [u'foo', u'1', u'True'])
-
-
-class TestOptionConverterMixin(object):
-
- def assertConverted(self, expected, opt, value):
- self.assertEquals(expected, opt.convert_from_unicode(value))
-
- def assertWarns(self, opt, value):
- warnings = []
- def warning(*args):
- warnings.append(args[0] % args[1:])
- self.overrideAttr(trace, 'warning', warning)
- self.assertEquals(None, opt.convert_from_unicode(value))
- self.assertLength(1, warnings)
- self.assertEquals(
- 'Value "%s" is not valid for "%s"' % (value, opt.name),
- warnings[0])
-
- def assertErrors(self, opt, value):
- self.assertRaises(errors.ConfigOptionValueError,
- opt.convert_from_unicode, value)
-
- def assertConvertInvalid(self, opt, invalid_value):
- opt.invalid = None
- self.assertEquals(None, opt.convert_from_unicode(invalid_value))
- opt.invalid = 'warning'
- self.assertWarns(opt, invalid_value)
- opt.invalid = 'error'
- self.assertErrors(opt, invalid_value)
-
-
-class TestOptionWithBooleanConverter(tests.TestCase, TestOptionConverterMixin):
-
- def get_option(self):
- return config.Option('foo', help='A boolean.',
- from_unicode=config.bool_from_store)
-
- def test_convert_invalid(self):
- opt = self.get_option()
- # A string that is not recognized as a boolean
- self.assertConvertInvalid(opt, u'invalid-boolean')
- # A list of strings is never recognized as a boolean
- self.assertConvertInvalid(opt, [u'not', u'a', u'boolean'])
-
- def test_convert_valid(self):
- opt = self.get_option()
- self.assertConverted(True, opt, u'True')
- self.assertConverted(True, opt, u'1')
- self.assertConverted(False, opt, u'False')
-
-
-class TestOptionWithIntegerConverter(tests.TestCase, TestOptionConverterMixin):
-
- def get_option(self):
- return config.Option('foo', help='An integer.',
- from_unicode=config.int_from_store)
-
- def test_convert_invalid(self):
- opt = self.get_option()
- # A string that is not recognized as an integer
- self.assertConvertInvalid(opt, u'forty-two')
- # A list of strings is never recognized as an integer
- self.assertConvertInvalid(opt, [u'a', u'list'])
-
- def test_convert_valid(self):
- opt = self.get_option()
- self.assertConverted(16, opt, u'16')
-
-
-class TestOptionWithListConverter(tests.TestCase, TestOptionConverterMixin):
-
- def get_option(self):
- return config.Option('foo', help='A list.',
- from_unicode=config.list_from_store)
+ self.assertEquals(expected, opt.convert_from_unicode(None, value))
+
+ def assertWarns(self, opt, value):
+ warnings = []
+ def warning(*args):
+ warnings.append(args[0] % args[1:])
+ self.overrideAttr(trace, 'warning', warning)
+ self.assertEquals(None, opt.convert_from_unicode(None, value))
+ self.assertLength(1, warnings)
+ self.assertEquals(
+ 'Value "%s" is not valid for "%s"' % (value, opt.name),
+ warnings[0])
+
+ def assertErrors(self, opt, value):
+ self.assertRaises(errors.ConfigOptionValueError,
+ opt.convert_from_unicode, None, value)
+
+ def assertConvertInvalid(self, opt, invalid_value):
+ opt.invalid = None
+ self.assertEquals(None, opt.convert_from_unicode(None, invalid_value))
+ opt.invalid = 'warning'
+ self.assertWarns(opt, invalid_value)
+ opt.invalid = 'error'
+ self.assertErrors(opt, invalid_value)
+
+
+class TestOptionWithBooleanConverter(tests.TestCase, TestOptionConverterMixin):
+
+ def get_option(self):
+ return config.Option('foo', help='A boolean.',
+ from_unicode=config.bool_from_store)
+
+ def test_convert_invalid(self):
+ opt = self.get_option()
+ # A string that is not recognized as a boolean
+ self.assertConvertInvalid(opt, u'invalid-boolean')
+ # A list of strings is never recognized as a boolean
+ self.assertConvertInvalid(opt, [u'not', u'a', u'boolean'])
+
+ def test_convert_valid(self):
+ opt = self.get_option()
+ self.assertConverted(True, opt, u'True')
+ self.assertConverted(True, opt, u'1')
+ self.assertConverted(False, opt, u'False')
+
+
+class TestOptionWithIntegerConverter(tests.TestCase, TestOptionConverterMixin):
+
+ def get_option(self):
+ return config.Option('foo', help='An integer.',
+ from_unicode=config.int_from_store)
+
+ def test_convert_invalid(self):
+ opt = self.get_option()
+ # A string that is not recognized as an integer
+ self.assertConvertInvalid(opt, u'forty-two')
+ # A list of strings is never recognized as an integer
+ self.assertConvertInvalid(opt, [u'a', u'list'])
+
+ def test_convert_valid(self):
+ opt = self.get_option()
+ self.assertConverted(16, opt, u'16')
+
+
+class TestListOption(tests.TestCase, TestOptionConverterMixin):
+
+ def get_option(self):
+ return config.ListOption('foo', help='A list.')
def test_convert_invalid(self):
opt = self.get_option()
@@ -2676,6 +2581,7 @@
def setUp(self):
super(TestCommandLineStore, self).setUp()
self.store = config.CommandLineStore()
+ self.overrideAttr(config, 'option_registry', config.OptionRegistry())
def get_section(self):
"""Get the unique section for the command line overrides."""
@@ -2696,12 +2602,15 @@
self.assertEqual('b', section.get('a'))
def test_list_override(self):
+ opt = config.ListOption('l')
+ config.option_registry.register(opt)
self.store._from_cmdline(['l=1,2,3'])
val = self.get_section().get('l')
self.assertEqual('1,2,3', val)
# Reminder: lists should be registered as such explicitely, otherwise
# the conversion needs to be done afterwards.
- self.assertEqual(['1', '2', '3'], config.list_from_store(val))
+ self.assertEqual(['1', '2', '3'],
+ opt.convert_from_unicode(self.store, val))
def test_multiple_overrides(self):
self.store._from_cmdline(['a=b', 'x=y'])
@@ -2972,6 +2881,25 @@
self.assertEquals((store,), calls[0])
+class TestQuotingIniFileStore(tests.TestCaseWithTransport):
+
+ def get_store(self):
+ return config.TransportIniFileStore(self.get_transport(), 'foo.conf')
+
+ def test_get_quoted_string(self):
+ store = self.get_store()
+ store._load_from_string('foo= " abc "')
+ stack = config.Stack([store.get_sections])
+ self.assertEquals(' abc ', stack.get('foo'))
+
+ def test_set_quoted_string(self):
+ store = self.get_store()
+ stack = config.Stack([store.get_sections], store)
+ stack.set('foo', ' a b c ')
+ store.save()
+ self.assertFileEqual('foo = " a b c "\n', 'foo.conf')
+
+
class TestTransportIniFileStore(TestStore):
def test_loading_unknown_file_fails(self):
@@ -3485,9 +3413,8 @@
self.assertEquals(12, self.conf.get('foo'))
def register_list_option(self, name, default=None, default_from_env=None):
- l = config.Option(name, help='A list.',
- default=default, default_from_env=default_from_env,
- from_unicode=config.list_from_store)
+ l = config.ListOption(name, help='A list.', default=default,
+ default_from_env=default_from_env)
self.registry.register(l)
def test_get_default_list_None(self):
@@ -3640,7 +3567,7 @@
list={foo},{bar},{baz}
''')
self.registry.register(
- config.Option('list', from_unicode=config.list_from_store))
+ config.ListOption('list'))
self.assertEquals(['start', 'middle', 'end'],
self.conf.get('list', expand=True))
@@ -3652,7 +3579,7 @@
list={foo}
''')
self.registry.register(
- config.Option('list', from_unicode=config.list_from_store))
+ config.ListOption('list'))
self.assertEquals(['start', 'middle', 'end'],
self.conf.get('list', expand=True))
@@ -3667,8 +3594,7 @@
''')
# What matters is what the registration says, the conversion happens
# only after all expansions have been performed
- self.registry.register(
- config.Option('hidden', from_unicode=config.list_from_store))
+ self.registry.register(config.ListOption('hidden'))
self.assertEquals(['bin', 'go'],
self.conf.get('hidden', expand=True))
=== modified file 'doc/developers/configuration.txt'
--- a/doc/developers/configuration.txt 2011-12-14 16:30:09 +0000
+++ b/doc/developers/configuration.txt 2011-12-21 08:52:41 +0000
@@ -128,8 +128,6 @@
Adding a new store
------------------
-
-
The following stores are used by ``bzr`` in ways that illustrate various
uses of sections.
@@ -251,6 +249,16 @@
places to inherit from the existing basic tests and add their own specific
ones.
+A ``Store`` defines how option values are stored, this includes:
+
+* defining the sections where the options are grouped,
+
+* defining how the values are quoted/unquoted for storage purposes. Stacks
+ use the unquoted values internally (default value handling and option
+ expansion are simpler this way) and ``bzr config`` quote them when they
+ need to be displayed.
+
+
Filtering sections
------------------
=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- a/doc/en/release-notes/bzr-2.5.txt 2011-12-19 12:09:27 +0000
+++ b/doc/en/release-notes/bzr-2.5.txt 2011-12-21 08:52:41 +0000
@@ -53,6 +53,10 @@
* Allow configuration option default value to be a python callable at
registration. (Vincent Ladeuil, #832064)
+* Configuration stores can now provides a specific quoting mechanism. This
+ is required to workaround ``configobj`` conflating quoting and list values
+ automatic conversion. (Vincent Ladeuil, #906897)
+
* Create obsolete_packs directory when repacking if it does not
exist. (Jonathan Riddell, Jelmer Vernooij, #314314)
More information about the bazaar-commits
mailing list