Merge lp:~vila/bzr/config-expand-options into lp:bzr

[Vincent Ladeuil]

Add the opt-in ability to expand options in config files

This implement option expansion in config files.

This is a limited implementation that only support references inside a given file. More steps will be necessary to allow cross-files references but this is a first significant steps and should already address many needs (including configuring the udd package importer both in production and for tests).

This proposed implementation detects loops and unknown references.

An exception is raised in the later case. There are possible variations there that we may prefer, feedback welcome !

For a first proposal I went with the conservative approach of throwing an exception but we can also:

- silently leave the unresolved references in place (the current version is a good testbed for this scenario as I realize while trying to submit this proposal :), we tend to display config options when something goes wrong and it's quite obvious then to catch typos

- replace the unresolved references by an empty string. This avoid exceptions but errors may be hard to diagnose. Not that this is what bash and other shells tends to implement though so people may prefer that.

My preference would be to leave the unresolved references in place in the future.

I didn't implement option expansion for dict values. I was convinced nobody ever used them until a test broke (per_branch.test_config) which has been introduced to fix bug #430382).

Such values are implemented as sub sections and supporting them seems unnecessary when they can be expressed with separate options (i.e. by concatenating the option name and the dict key).

Another issue with expanding options in dicts is that this can't be done for keys (even if the keys are unique without expansion they can collide after expansion).

Bottom line: options are basically strings, list of strings at most, trying to turn them into a database doesn't sound like somthing we want to pursue.

In the long term, I'd like to completely deprecate dicts as supported values in config files.

For now, a warning is emitted.

Back to the option expansion: get_user_option() now has a new 'expand' attribute to control the expansion and allow templates to be retrieved (with expand=False).

There is also a new expand_options(strings, env=None) method on the Config object (so all daughter alsses can use it) whose purpose is to expand options *or* keys from the additional 'env'.
mergetools and difftools are the primary targets for this method since they rely on templates embedding references to domain specific variables (paths mainly).
bugtrackers could also use this (but the current implementation is so simple I didn't change it except for calling get_user_option(..., expand=False).

I also fixed get_user_option_as_{bool|list} to use the public get_user_option so they benefit from the expansion too.

[Vincent Ladeuil (vila)]

For the record and context: http://bazaar.launchpad.net/~bzr-core/bzr/devnotes/view/head:/configuration.txt

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2/18/2011 10:22 AM, Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging lp:~vila/bzr/config-expand-options into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
> For more details, see:
> https://code.launchpad.net/~vila/bzr/config-expand-options/+merge/50355
>
> This implement option expansion in config files.
>
> This is a limited implementation that only support references inside a given file. More steps will be necessary to allow cross-files references but this is a first significant steps and should already address many needs (including configuring the udd package importer both in production and for tests).
>
> This proposed implementation detects loops and unknown references.
>
> An exception is raised in the later case. There are possible variations there that we may prefer, feedback welcome !
>
> For a first proposal I went with the conservative approach of throwing an exception but we can also:
>
> - silently leave the unresolved references in place (the current version is a good testbed for this scenario as I realize while trying to submit this proposal :), we tend to display config options when something goes wrong and it's quite obvious then to catch typos
>
> - replace the unresolved references by an empty string. This avoid exceptions but errors may be hard to diagnose. Not that this is what bash and other shells tends to implement though so people may prefer that.
>
> My preference would be to leave the unresolved references in place in the future.
>
> I didn't implement option expansion for dict values. I was convinced nobody ever used them until a test broke (per_branch.test_config) which has been introduced to fix bug #430382).

qbzr uses a dict for some of its stuff. I think the commit message
backlog is stored that way. (Because we've had failures for 'uncommit'
in lightweight checkouts because RemoteConfig didn't support dict
values, etc.)

>
> Such values are implemented as sub sections and supporting them seems unnecessary when they can be expressed with separate options (i.e. by concatenating the option name and the dict key).
>
> Another issue with expanding options in dicts is that this can't be done for keys (even if the keys are unique without expansion they can collide after expansion).
>
> Bottom line: options are basically strings, list of strings at most, trying to turn them into a database doesn't sound like somthing we want to pursue.
>
> In the long term, I'd like to completely deprecate dicts as supported values in config files.
>
> For now, a warning is emitted.
>
> Back to the option expansion: get_user_option() now has a new 'expand' attribute to control the expansion and allow templates to be retrieved (with expand=False).
>
> There is also a new expand_options(strings, env=None) method on the Config object (so all daughter alsses can use it) whose purpose is to expand options *or* keys from the additional 'env'.
> mergetools and difftools are the primary targets for this method since they rely on templates embedding references to domain specific variables (paths mainly).
> bugtrackers could also use this (but the cur...

[Vincent Ladeuil (vila)]

> I'm a bit concerned about scope here. For example if in locations.conf I do:
>
> [/one/branch/path]
> foo = hello
> bar = {foo}/2
>
> [/another/branch/path]
> bar = {foo}/2
>
> Does it also get expanded? or do we only expand within the given scope
> of a branch?

It shouldn't. Worth adding a test. Did you try ?

>
> For big changes like this, I tend to err on the side of 'fail noisily'
> until we figure out that we really want to do it differently. So a {foo}
> in a string with no option {foo} will fail, rather than silently get
> replaced by '' or left in line.

That's indeed the current proposal. You don't say whether you want to keep it in the long term or which alternative you prefer otherwise.

>
> Then again, maybe you can just notice that your path isn't correct.
>
>
> I'm a little concerned with stuff like 'uncommit' messages that get
> stored in the config. If I do:
>
>
> bzr commit -m "{foo} is now available for config entries"
> bzr uncommit
>
> # Note this needs something like qbzr to actually store the old commit
> # message
>

Yes, that was bug #430382 and how I discover there was at least one use of dict value in the wild.

> Will that new entry try to expand {foo}?

No. Because it's part of a dict value.

The correct way to deal with that sort of option *will* be to either properly quote them (or bencode them or whatever) or use get_user_option(..., expand=False) which makes sense. Either an option is used as a template or an arbitrary content and you should take care of what can be embedded there or it's a simple option where no references can appear.

[Vincent Ladeuil (vila)]

>>>>> John A Meinel <email address hidden> writes:

<snip/>
    > I'm a bit concerned about scope here. For example if in locations.conf I do:

    > [/one/branch/path]
    > foo = hello
    > bar = {foo}/2

    > [/another/branch/path]
    > bar = {foo}/2

A trickier example would be:

    > [/project]
    > foo = hello
    > bar = {foo}/2

    > [/project/branch]
    > bar = {foo}/3

And this case should be valid.

Tests coming soon...

[Vincent Ladeuil (vila)]

> The correct way to deal with that sort of option *will* be to either properly
> quote them (or bencode them or whatever) or use get_user_option(...,
> expand=False) which makes sense. Either an option is used as a template or an
> arbitrary content and you should take care of what can be embedded there or
> it's a simple option where no references can appear.

*blink*

Who am I to chose here ?

The default behaviour is not for me to decide:

  bzr.config.expand = <your choice>

... available in your nearest config file (RSN) :D

[Vincent Ladeuil (vila)]

And here comes the option controlling the option expansion.

Note that configobj *was* implementing expansion for ${option} so we're not entering a completely unexplored territory when it comes to protection values against expansion.

We never had reports about expansion corrupting data since bzr started using configobj so I'm not *that* concerned about such occurrences with the {option} syntax, but if it can help landing this proposal...

[Vincent Ladeuil (vila)]

Ping

Any objections to land this now ?

</ticking trigger>

[Jelmer Vernooij (jelmer)]

I think this is good to land now.

review: Approve

[Vincent Ladeuil (vila)]

sent to pqm by email

=== modified file 'bzrlib/bugtracker.py'
--- bzrlib/bugtracker.py	2010-09-25 00:42:22 +0000
+++ bzrlib/bugtracker.py	2011-02-20 11:14:15 +0000
@@ -231,7 +231,8 @@
 
 
 tracker_registry.register('gnome',
-    UniqueIntegerBugTracker('gnome', 'http://bugzilla.gnome.org/show_bug.cgi?id='))
+    UniqueIntegerBugTracker('gnome',
+                            'http://bugzilla.gnome.org/show_bug.cgi?id='))
 
 
 class URLParametrizedBugTracker(BugTracker):
@@ -246,7 +247,7 @@
     def get(self, abbreviation, branch):
         config = branch.get_config()
         url = config.get_user_option(
-            "%s_%s_url" % (self.type_name, abbreviation))
+            "%s_%s_url" % (self.type_name, abbreviation), expand=False)
         if url is None:
             return None
         self._base_url = url
@@ -261,9 +262,12 @@
         return urlutils.join(self._base_url, self._bug_area) + str(bug_id)
 
 
-class URLParametrizedIntegerBugTracker(IntegerBugTracker, URLParametrizedBugTracker):
-    """A type of bug tracker that can be found on a variety of different sites,
-    and thus needs to have the base URL configured, but only allows integer bug IDs.
+class URLParametrizedIntegerBugTracker(IntegerBugTracker,
+                                       URLParametrizedBugTracker):
+    """A type of bug tracker that  only allows integer bug IDs.
+
+    This can be found on a variety of different sites, and thus needs to have
+    the base URL configured.
 
     Looks for a config setting in the form '<type_name>_<abbreviation>_url'.
     `type_name` is the name of the type of tracker (e.g. 'bugzilla' or 'trac')

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2011-01-20 04:44:14 +0000
+++ bzrlib/config.py	2011-02-20 11:14:15 +0000
@@ -129,25 +129,50 @@
 STORE_BRANCH = 3
 STORE_GLOBAL = 4
 
-_ConfigObj = None
-def ConfigObj(*args, **kwargs):
-    global _ConfigObj
-    if _ConfigObj is None:
-        class ConfigObj(configobj.ConfigObj):
-
-            def get_bool(self, section, key):
-                return self[section].as_bool(key)
-
-            def get_value(self, section, name):
-                # Try [] for the old DEFAULT section.
-                if section == "DEFAULT":
-                    try:
-                        return self[name]
-                    except KeyError:
-                        pass
-                return self[section][name]
-        _ConfigObj = ConfigObj
-    return _ConfigObj(*args, **kwargs)
+
+class ConfigObj(configobj.ConfigObj):
+
+    def __init__(self, infile=None, **kwargs):
+        # We define our own interpolation mechanism calling it option expansion
+        super(ConfigObj, self).__init__(infile=infile,
+                                        interpolation=False,
+                                        **kwargs)
+
+
+    def get_bool(self, section, key):
+        return self[section].as_bool(key)
+
+    def get_value(self, section, name):
+        # Try [] for the old DEFAULT section.
+        if section == "DEFAULT":
+            try:
+                return self[name]
+            except KeyError:
+                pass
+        return self[section][name]
+
+
+# FIXME: Until we can guarantee that each config file is loaded once and 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).This shouldn't be part of 2.4.0 final,
+# yell at mgz^W vila and the RM if this is still present at that time
+# -- 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):
@@ -189,21 +214,167 @@
     def _get_signing_policy(self):
         """Template method to override signature creation policy."""
 
+    option_ref_re = None
+
+    def expand_options(self, string, env=None):
+        """Expand option references in the string in the configuration context.
+
+        :param string: The string containing option to expand.
+
+        :param env: An option dict defining additional configuration options or
+            overriding existing ones.
+
+        :returns: The expanded string.
+        """
+        return self._expand_options_in_string(string, env)
+
+    def _expand_options_in_list(self, slist, env=None, _ref_stack=None):
+        """Expand options in  a list of strings in the configuration context.
+
+        :param slist: A list of strings.
+
+        :param env: An option dict defining additional configuration options or
+            overriding existing ones.
+
+        :param _ref_stack: Private list containing the options being
+            expanded to detect loops.
+
+        :returns: The flatten list of expanded strings.
+        """
+        # expand options in each value separately flattening lists
+        result = []
+        for s in slist:
+            value = self._expand_options_in_string(s, env, _ref_stack)
+            if isinstance(value, list):
+                result.extend(value)
+            else:
+                result.append(value)
+        return result
+
+    def _expand_options_in_string(self, string, env=None, _ref_stack=None):
+        """Expand options in the string in the configuration context.
+
+        :param string: The string to be expanded.
+
+        :param env: An option dict defining additional configuration options or
+            overriding existing ones.
+
+        :param _ref_stack: Private list containing the options being
+            expanded to detect loops.
+
+        :returns: The expanded string.
+        """
+        if string is None:
+            # Not much to expand there
+            return None
+        if _ref_stack is None:
+            # What references are currently resolved (to detect loops)
+            _ref_stack = []
+        if self.option_ref_re is None:
+            # We want to match the most embedded reference first (i.e. for
+            # '{{foo}}' we will get '{foo}',
+            # for '{bar{baz}}' we will get '{baz}'
+            self.option_ref_re = re.compile('({[^{}]+})')
+        result = string
+        # We need to iterate until no more refs appear ({{foo}} will need two
+        # iterations for example).
+        while True:
+            try:
+                raw_chunks = self.option_ref_re.split(result)
+            except TypeError:
+                import pdb; pdb.set_trace()
+            if len(raw_chunks) == 1:
+                # Shorcut the trivial case: no refs
+                return result
+            chunks = []
+            list_value = False
+            # Split will isolate refs so that every other chunk is a ref
+            chunk_is_ref = False
+            for chunk in raw_chunks:
+                if not chunk_is_ref:
+                    if chunk:
+                        # Keep only non-empty strings (or we get bogus empty
+                        # slots when a list value is involved).
+                        chunks.append(chunk)
+                    chunk_is_ref = True
+                else:
+                    name = chunk[1:-1]
+                    if name in _ref_stack:
+                        raise errors.OptionExpansionLoop(string, _ref_stack)
+                    _ref_stack.append(name)
+                    value = self._expand_option(name, env, _ref_stack)
+                    if value is None:
+                        raise errors.ExpandingUnknownOption(name, string)
+                    if isinstance(value, list):
+                        list_value = True
+                        chunks.extend(value)
+                    else:
+                        chunks.append(value)
+                    _ref_stack.pop()
+                    chunk_is_ref = False
+            if list_value:
+                # Once a list appears as the result of an expansion, all
+                # callers will get a list result. This allows a consistent
+                # behavior even when some options in the expansion chain
+                # defined as strings (no comma in their value) but their
+                # expanded value is a list.
+                return self._expand_options_in_list(chunks, env, _ref_stack)
+            else:
+                result = ''.join(chunks)
+        return result
+
+    def _expand_option(self, name, env, _ref_stack):
+        if env is not None and name in env:
+            # Special case, values provided in env takes precedence over
+            # anything else
+            value = env[name]
+        else:
+            # FIXME: This is a limited implementation, what we really need is a
+            # way to query the bzr config for the value of an option,
+            # respecting the scope rules (That is, once we implement fallback
+            # configs, getting the option value should restart from the top
+            # config, not the current one) -- vila 20101222
+            value = self.get_user_option(name, expand=False)
+            if isinstance(value, list):
+                value = self._expand_options_in_list(value, env, _ref_stack)
+            else:
+                value = self._expand_options_in_string(value, env, _ref_stack)
+        return value
+
     def _get_user_option(self, option_name):
         """Template method to provide a user option."""
         return None
 
-    def get_user_option(self, option_name):
-        """Get a generic option - no special process, no default."""
-        return self._get_user_option(option_name)
-
-    def get_user_option_as_bool(self, option_name):
+    def get_user_option(self, option_name, expand=None):
+        """Get a generic option - no special process, no default.
+
+        :param option_name: The queried option.
+
+        :param expand: Whether options references should be expanded.
+
+        :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):
+                value = self._expand_options_in_list(value)
+            elif isinstance(value, dict):
+                trace.warning('Cannot expand "%s":'
+                              ' Dicts do not support option expansion'
+                              % (option_name,))
+            else:
+                value = self._expand_options_in_string(value)
+        return value
+
+    def get_user_option_as_bool(self, option_name, expand=None):
         """Get a generic option as a boolean - no special process, no default.
 
         :return None if the option doesn't exist or its value can't be
             interpreted as a boolean. Returns True or False otherwise.
         """
-        s = self._get_user_option(option_name)
+        s = self.get_user_option(option_name, expand=expand)
         if s is None:
             # The option doesn't exist
             return None
@@ -214,15 +385,16 @@
                           s, option_name)
         return val
 
-    def get_user_option_as_list(self, option_name):
+    def get_user_option_as_list(self, option_name, expand=None):
         """Get a generic option as a list - no special process, no default.
 
         :return None if the option doesn't exist. Returns the value as a list
             otherwise.
         """
-        l = self._get_user_option(option_name)
+        l = self.get_user_option(option_name, expand=expand)
         if isinstance(l, (str, unicode)):
-            # A single value, most probably the user forgot the final ','
+            # A single value, most probably the user forgot (or didn't care to
+            # add) the final ','
             l = [l]
         return l
 
@@ -372,8 +544,9 @@
         # be found in the known_merge_tools if it's not found in the config.
         # This should be done through the proposed config defaults mechanism
         # when it becomes available in the future.
-        command_line = (self.get_user_option('bzr.mergetool.%s' % name) or
-                        mergetools.known_merge_tools.get(name, None))
+        command_line = (self.get_user_option('bzr.mergetool.%s' % name,
+                                             expand=False)
+                        or mergetools.known_merge_tools.get(name, None))
         return command_line
 
 
@@ -672,6 +845,11 @@
     def __init__(self, file_name):
         super(LockableConfig, self).__init__(file_name=file_name)
         self.dir = osutils.dirname(osutils.safe_unicode(self.file_name))
+        # FIXME: It doesn't matter that we don't provide possible_transports
+        # below since this is currently used only for local config files ;
+        # local transports are not shared. But if/when we start using
+        # LockableConfig for other kind of transports, we will need to reuse
+        # whatever connection is already established -- vila 20100929
         self.transport = transport.get_transport(self.dir)
         self._lock = lockdir.LockDir(self.transport, 'lock')
 

=== modified file 'bzrlib/errors.py'
--- bzrlib/errors.py	2011-01-26 20:02:52 +0000
+++ bzrlib/errors.py	2011-02-20 11:14:15 +0000
@@ -3221,3 +3221,22 @@
     def __init__(self, branch_url):
         self.branch_url = branch_url
 
+
+# FIXME: I would prefer to define the config related exception classes in
+# config.py but the lazy import mechanism proscribes this -- vila 20101222
+class OptionExpansionLoop(BzrError):
+
+    _fmt = 'Loop involving %(refs)r while expanding "%(string)s".'
+
+    def __init__(self, string, refs):
+        self.string = string
+        self.refs = '->'.join(refs)
+
+
+class ExpandingUnknownOption(BzrError):
+
+    _fmt = 'Option %(name)s is not defined while expanding "%(string)s".'
+
+    def __init__(self, name, string):
+        self.name = name
+        self.string = string

=== modified file 'bzrlib/help_topics/en/configuration.txt'
--- bzrlib/help_topics/en/configuration.txt	2011-01-16 01:12:01 +0000
+++ bzrlib/help_topics/en/configuration.txt	2011-02-20 11:14:15 +0000
@@ -258,6 +258,13 @@
     email            = John Doe <[e-mail]>
     check_signatures = require
 
+A variable can reference other variables **in the same configuration file** by
+enclosing them in curly brackets::
+
+    my_branch_name = feature_x
+    my_server      = bzr+ssh://example.com
+    push_location   = {my_server}/project/{my_branch_name}
+
 
 Variable policies
 ^^^^^^^^^^^^^^^^^

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2011-02-11 17:12:35 +0000
+++ bzrlib/tests/__init__.py	2011-02-20 11:14:15 +0000
@@ -954,6 +954,10 @@
         # 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)
 
     def debug(self):
         # debug a frame up.

=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2011-01-20 04:44:14 +0000
+++ bzrlib/tests/test_config.py	2011-02-20 11:14:15 +0000
@@ -514,6 +514,7 @@
             ' Use IniBasedConfig(_content=xxx) instead.'],
             conf._get_parser, file=config_file)
 
+
 class TestIniConfigSaving(tests.TestCaseInTempDir):
 
     def test_cant_save_without_a_file_name(self):
@@ -527,6 +528,198 @@
         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.
+
+    What we really want here is to test the Config level, but the class being
+    abstract as far as storing values is concerned, this can't be done
+    properly (yet).
+    """
+    # FIXME: This should be rewritten when all configs share a storage
+    # implementation -- vila 2011-02-18
+
+    def get_config(self, string=None):
+        if string is None:
+            string = ''
+        c = config.IniBasedConfig.from_string(string)
+        return c
+
+    def assertExpansion(self, expected, conf, string, env=None):
+        self.assertEquals(expected, conf.expand_options(string, env))
+
+    def test_no_expansion(self):
+        c = self.get_config('')
+        self.assertExpansion('foo', c, 'foo')
+
+    def test_env_adding_options(self):
+        c = self.get_config('')
+        self.assertExpansion('bar', c, '{foo}', {'foo': 'bar'})
+
+    def test_env_overriding_options(self):
+        c = self.get_config('foo=baz')
+        self.assertExpansion('bar', c, '{foo}', {'foo': 'bar'})
+
+    def test_simple_ref(self):
+        c = self.get_config('foo=xxx')
+        self.assertExpansion('xxx', c, '{foo}')
+
+    def test_unknown_ref(self):
+        c = self.get_config('')
+        self.assertRaises(errors.ExpandingUnknownOption,
+                          c.expand_options, '{foo}')
+
+    def test_indirect_ref(self):
+        c = self.get_config('''
+foo=xxx
+bar={foo}
+''')
+        self.assertExpansion('xxx', c, '{bar}')
+
+    def test_embedded_ref(self):
+        c = self.get_config('''
+foo=xxx
+bar=foo
+''')
+        self.assertExpansion('xxx', c, '{{bar}}')
+
+    def test_simple_loop(self):
+        c = self.get_config('foo={foo}')
+        self.assertRaises(errors.OptionExpansionLoop, c.expand_options, '{foo}')
+
+    def test_indirect_loop(self):
+        c = self.get_config('''
+foo={bar}
+bar={baz}
+baz={foo}''')
+        e = self.assertRaises(errors.OptionExpansionLoop,
+                              c.expand_options, '{foo}')
+        self.assertEquals('foo->bar->baz', e.refs)
+        self.assertEquals('{foo}', e.string)
+
+    def test_list(self):
+        conf = self.get_config('''
+foo=start
+bar=middle
+baz=end
+list={foo},{bar},{baz}
+''')
+        self.assertEquals(['start', 'middle', 'end'],
+                           conf.get_user_option('list', expand=True))
+
+    def test_cascading_list(self):
+        conf = self.get_config('''
+foo=start,{bar}
+bar=middle,{baz}
+baz=end
+list={foo}
+''')
+        self.assertEquals(['start', 'middle', 'end'],
+                           conf.get_user_option('list', expand=True))
+
+    def test_pathological_hidden_list(self):
+        conf = self.get_config('''
+foo=bin
+bar=go
+start={foo
+middle=},{
+end=bar}
+hidden={start}{middle}{end}
+''')
+        # Nope, it's either a string or a list, and the list wins as soon as a
+        # ',' appears, so the string concatenation never occur.
+        self.assertEquals(['{foo', '}', '{', 'bar}'],
+                          conf.get_user_option('hidden', expand=True))
+
+class TestLocationConfigOptionExpansion(tests.TestCaseInTempDir):
+
+    def get_config(self, location, string=None):
+        if string is None:
+            string = ''
+        # Since we don't save the config we won't strictly require to inherit
+        # from TestCaseInTempDir, but an error occurs so quickly...
+        c = config.LocationConfig.from_string(string, location)
+        return c
+
+    def test_dont_cross_unrelated_section(self):
+        c = self.get_config('/another/branch/path','''
+[/one/branch/path]
+foo = hello
+bar = {foo}/2
+
+[/another/branch/path]
+bar = {foo}/2
+''')
+        self.assertRaises(errors.ExpandingUnknownOption,
+                          c.get_user_option, 'bar', expand=True)
+
+    def test_cross_related_sections(self):
+        c = self.get_config('/project/branch/path','''
+[/project]
+foo = qu
+
+[/project/branch/path]
+bar = {foo}ux
+''')
+        self.assertEquals('quux', c.get_user_option('bar', expand=True))
+
+
 class TestIniBaseConfigOnDisk(tests.TestCaseInTempDir):
 
     def test_cannot_reload_without_name(self):
@@ -985,7 +1178,7 @@
         conf = self._get_empty_config()
         cmdline = conf.find_merge_tool('kdiff3')
         self.assertEquals('kdiff3 {base} {this} {other} -o {result}', cmdline)
-        
+
     def test_find_merge_tool_override_known(self):
         conf = self._get_empty_config()
         conf.set_user_option('bzr.mergetool.kdiff3', 'kdiff3 blah')

=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2011-02-19 17:37:45 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2011-02-20 11:14:15 +0000
@@ -26,6 +26,14 @@
 * External merge tools can now be configured in bazaar.conf. See
   ``bzr help configuration`` for more information.  (Gordon Tyler, #489915)
 
+* Configuration options can now use references to other options in the same
+  file by enclosing them with curly brackets (``{other_opt}``). This makes it
+  possible to use, for example,
+  ``push_location=lp:~vila/bzr/config-{nickname}`` in ``branch.conf`` when
+  using a loom. During the beta period, the default behaviour is to disable
+  this feature. It can be activated by declaring ``bzr.config.expand = True``
+  in ``bazaar.conf``. (Vincent Ladeuil)
+
 Improvements
 ************
 

=== modified file 'doc/en/whats-new/whats-new-in-2.4.txt'
--- doc/en/whats-new/whats-new-in-2.4.txt	2011-02-07 04:29:44 +0000
+++ doc/en/whats-new/whats-new-in-2.4.txt	2011-02-20 11:14:15 +0000
@@ -32,6 +32,14 @@
 revisions from tags will always be present, so that operations like ``bzr
 log -r tag:foo`` will always work.
 
+Configuration files
+*******************
+
+Option values can now refer to other options in the same configuration file by
+enclosing them in curly brackets (``{option}``). This is an opt-in feature
+during the beta period controlled by the ``bzr.config.expand`` option that
+should be declared in ``bazaar.conf`` and no other file.
+
 Further information
 *******************