Merge lp:~vila/bzr/new-config into lp:bzr

[Vincent Ladeuil]

This is not for review, more like a sneak preview.

If you want to look at it, you'd probably want to branch it locally (beware, it's a loom) and look at the contained threads:

   next
  doc-new-config
=>config-concrete-stacks
  config-stack
  config-section-matchers
  config-concrete-stores
  config-abstract-store
  config-section
  refactor-locationconfig-matching
  bzr.dev

Forget about doc-new-config and next.

This provides the building blocks to replace our actual config files, mainly:

- Store objects which are associated with persistent config files (the only implementation so far is based on ConfigObj)

- Stack objects which should replace GlobalConfig, LocationConfig and BranchConfig objects

- some test infrastructure to make it easier to write new config files or add features

- ensure that all config stores use the transport API

What is still missing:

- user and dev docs (in progress)

- option value converters (mainly for boolean, the plan is to add a parameter to Stack.get())

- an implementation mimicking the actual behavior regarding IOs: Stack.get() should force a load of the config file, Stack.set() should force a write of the config file,

With this, we get an implementation that can replace:
 GlobalConfig().get_user_option('blah')
with
 config.GlobalStack().get('blah')
or maybe even
 config.Global.get('blah')
but the later probably needs discussionS

With that, it becomes possible to progressively deploy the new implementation option by option, feature by feature, discussing issues as they are discovered.

I'll file more mps for each thread later.

[Martin Pool (mbp)]

> config.GlobalStack().get('blah')
> or maybe even
> config.Global.get('blah')

I'd like to talk about this; istm if we have the right expression to put in config-using code we will know some aspects of the infrastructure are also right (or can be gradually made right.)

'FooBar()' should almost always construct an object of that class; cases where we've fudged that to actually construct something else cause unclearness. If it might need to do something else (either make a different class, or return an existing object), we should use a factory function.

In these cases we generally want to be reusing existing objects, not making new ones.

If this is a new api maybe it should hang off the library_state object; though there can be a convenient caller interface that doesn't explicitly need to do so.

I guess 'global' here means (eventually): from the command line, environment variables, user config, system-wide config, built in defaults. Perhaps others. Callers ought to be expressing essentially constraints on what values could be relevant, without knowing the policy of how that's matched up.

The more interesting case there is something more restricted than global. I was contemplating perhaps having them say

config_stack_for([branch, working_tree, destination_transport])

where they pass in a list of objects that are relevant to their current scope, and then the config mechanism works out which sections/sources to use from that.

[Vincent Ladeuil (vila)]

> I'd like to talk about this; istm if we have the right expression to put in config-using code we will know some aspects of the infrastructure are also right (or can be gradually made right.)

+1

My plan is to make small proposals on top of this one to try various approaches.

Things like:
- adding helpers to define Option with policies (boolean or registry values)
- new SectionMatchers (I find the current LocationMatcher hard to understand for users in several edge cases, istm that real PathMatcher or GlobMatcher would be clearer)
- the new locking scheme described in the devnotes

> 'FooBar()' should almost always construct an object of that class; cases where we've fudged that to actually construct something else cause unclearness. If it might need to do something else (either make a different class, or return an existing object), we should use a factory function.

I'm not sure what you're targeting here, but yeah, some registries are still missing in the proposed design (because they weren't required to reach an ~equivalent implementation).

> In these cases we generally want to be reusing existing objects, not making new ones.

If by that you mean making sure we don't have to create a ConfigStack each time we want to get an option value, I fully agree.

> If this is a new api maybe it should hang off the library_state object; though there can be a convenient caller interface that doesn't explicitly need to do so.

For GlobalStack and LocationStack, that's indeed the plan.

Also note that it should be possible to add some sort of cloning facility at this level without needing to re-read any config file: say you have a LocationStack and you want one but for a different location.

> I guess 'global' here means (eventually): from the command line, environment variables, user config, system-wide config, built in defaults. Perhaps others. Callers ought to be expressing essentially constraints on what values could be relevant, without knowing the policy of how that's matched up.

Roughly yes. Since os.environ and command-line options are *not* supported today, the proposal didn't mention them.

But the idea is that they should be added at the stack level (just look at the concrete stacks there, adding them is just adding the right dict (yes, the python object) into the stack.sections list at the right place.

> Making it easy to share and reuse these facilities when creating different stacks is not implemented here but shouldn't be hard either.

> The more interesting case there is something more restricted than global. I was contemplating perhaps having them say

> config_stack_for([branch, working_tree, destination_transport])

> where they pass in a list of objects that are relevant to their current scope, and then the config mechanism works out which sections/sources to use from that.

Yes, that's the idea, I don't know yet how this will be implemented but some patterns are already known and mentioned in the devnotes, the relevant ones should naturally emerge as we deploy.

[Martin Pool (mbp)]

My point about using a camelcaps name in the api is

1- the expression 'GlobalConfig()' should construct a new GlobalConfig object
2- we don't want to build new objects every time someone wants to look
up a value
therefore
3- the expression to look up a value should not include something that
looks like object construction

you also mention

> config.Global.get('blah')

which looks like access to a class method, and I think we don't want
that either, because it won't scale when we need access to a
particular instance of a type of configuration.

That's why I come back to saying there should be something like a
function that gives you the stack relevant to particular scopes.

Martin

On 8 April 2011 18:12, Vincent Ladeuil <email address hidden> wrote:
>> I'd like to talk about this; istm if we have the right expression to put in config-using code we will know some aspects of the infrastructure are also right (or can be gradually made right.)
>
> +1
>
> My plan is to make small proposals on top of this one to try various approaches.
>
> Things like:
> - adding helpers to define Option with policies (boolean or registry values)
> - new SectionMatchers (I find the current LocationMatcher hard to understand for users in several edge cases, istm that real PathMatcher or GlobMatcher would be clearer)
> - the new locking scheme described in the devnotes
>
>
>> 'FooBar()' should almost always construct an object of that class; cases where we've fudged that to actually construct something else cause unclearness.  If it might need to do something else (either make a different class, or return an existing object), we should use a factory function.
>
> I'm not sure what you're targeting here, but yeah, some registries are still missing in the proposed design (because they weren't required to reach an ~equivalent implementation).
>
>> In these cases we generally want to be reusing existing objects, not making new ones.
>
> If by that you mean making sure we don't have to create a ConfigStack each time we want to get an option value, I fully agree.
>
>> If this is a new api maybe it should hang off the library_state object; though there can be a convenient caller interface that doesn't explicitly need to do so.
>
> For GlobalStack and LocationStack, that's indeed the plan.
>
> Also note that it should be possible to add some sort of cloning facility at this level without needing to re-read any config file: say you have a LocationStack and you want one but for a different location.
>
>> I guess 'global' here means (eventually): from the command line, environment variables, user config, system-wide config, built in defaults.  Perhaps others.  Callers ought to be expressing essentially constraints on what values could be relevant, without knowing the policy of how that's matched up.
>
> Roughly yes. Since os.environ and command-line options are *not* supported today, the proposal didn't mention them.
>
> But the idea is that they should be added at the stack level (just look at the concrete stacks there, adding them is just adding the right dict (yes, the python object) into the stack.sections list at the right place.
>
>> Making it easy to share and reuse ...

[Vincent Ladeuil (vila)]

> My point about using a camelcaps name in the api is
>
> 1- the expression 'GlobalConfig()' should construct a new GlobalConfig object
> 2- we don't want to build new objects every time someone wants to look
> up a value
> therefore
> 3- the expression to look up a value should not include something that
> looks like object construction
>
> you also mention
>
> > config.Global.get('blah')
>

Full agreement there. What I was confusingly trying to express was that either we let people build their own objects (even for GlobalStack) or we end up being able to control that there only one GLobalStack, in which case we may achieve an even simpler syntax. We'll see.

[Martin Pool (mbp)]

I've read all the individual branches, and though there are a few questions in detail, I think the overall shape looks pretty nice.

I am finishing off Sergei's rules patch, and in the context of that I was wondering whether we should really have a major structural separation between per-file rules and the other configuration schemes. They are all just name-value pairs. The query to get the value for a particular file in a particular well could equally well go through a config-type api, and also read the general configuration stuff as well as per-file rules. (Indeed it can do either of them but I think quite reasonably it could do both.)

So for instance if you wanted to set eol=native just globally or in locations.conf you could do that.

Obviously not all settings make sense to read per-file. Also, as we will eventually read some settings from the tree contents, not all of them are safe to read from the untrusted content in there. But that should be pretty easy to do, just by constructing the right stack. We can perhaps also constrain this when we later get a registry of available options: mark some of them as per-file and some as safe. (eol conversion is safe; sending email or running external commands is not safe.)

We would have to think carefully about the performance considerations because we couldn't afford to construct a new section or stack for every file we want to find out about. But if there is a get_tree_file_config_stack(...) then perhaps that can be smart about reusing existing objects.

[lp:~vila/bzr/new-config]

5472. By Vincent Ladeuil on 2011-05-09

Merge doc-new-config into next

5473. By Vincent Ladeuil on 2011-05-11

Merge doc-new-config into next

5474. By Vincent Ladeuil on 2011-05-14

Merge doc-new-config into next

5475. By Vincent Ladeuil on 2011-05-15

Merge doc-new-config into next

5476. By Vincent Ladeuil on 2011-05-17

Merge config-editor-option into next

5477. By Vincent Ladeuil on 2011-05-17

Mention that the config lazy loading is defeated.

5478. By Vincent Ladeuil on 2011-05-17

Merge config-editor-option into next

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2011-04-05 14:47:26 +0000
+++ bzrlib/config.py	2011-04-12 12:36:46 +0000
@@ -848,7 +848,7 @@
         # 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')
+        self._lock = lockdir.LockDir(self.transport, self.lock_name)
 
     def _create_from_string(self, unicode_bytes, save):
         super(LockableConfig, self)._create_from_string(unicode_bytes, False)
@@ -967,6 +967,61 @@
         super(LockableConfig, self).remove_user_option(option_name,
                                                        section_name)
 
+def _iter_for_location_by_parts(sections, location):
+    """Keep only the sessions matching the specified location.
+
+    :param sections: An iterable of section names.
+
+    :param location: An url or a local path to match against.
+
+    :returns: An iterator of (section, extra_path, nb_parts) where nb is the
+        number of path components in the section name, section is the section
+        name and extra_path is the difference between location and the section
+        name.
+    """
+    location_parts = location.rstrip('/').split('/')
+
+    for section in sections:
+        # location is a local path if possible, so we need
+        # to convert 'file://' urls to local paths if necessary.
+
+        # FIXME: I don't think the above comment is still up to date,
+        # LocationConfig is always instantiated with an url -- vila 2011-04-07
+
+        # This also avoids having file:///path be a more exact
+        # match than '/path'.
+
+        # FIXME: Not sure about the above either, but since the path components
+        # are compared in sync, adding two empty components (//) is likely to
+        # trick the comparison and also trick the check on the number of
+        # components, so we *should* take only the relevant part of the url. On
+        # the other hand, this means 'file://' urls *can't* be used in sections
+        # so more work is probably needed -- vila 2011-04-07
+
+        if section.startswith('file://'):
+            section_path = urlutils.local_path_from_url(section)
+        else:
+            section_path = section
+        section_parts = section_path.rstrip('/').split('/')
+
+        matched = True
+        if len(section_parts) > len(location_parts):
+            # More path components in the section, they can't match
+            matched = False
+        else:
+            # Rely on zip truncating in length to the length of the shortest
+            # argument sequence.
+            names = zip(location_parts, section_parts)
+            for name in names:
+                if not fnmatch.fnmatch(name[0], name[1]):
+                    matched = False
+                    break
+        if not matched:
+            continue
+        # build the path difference between the section and the location
+        extra_path = '/'.join(location_parts[len(section_parts):])
+        yield section, extra_path, len(section_parts)
+
 
 class LocationConfig(LockableConfig):
     """A configuration object that gives the policy for a location."""
@@ -1001,49 +1056,20 @@
 
     def _get_matching_sections(self):
         """Return an ordered list of section names matching this location."""
-        sections = self._get_parser()
-        location_names = self.location.split('/')
-        if self.location.endswith('/'):
-            del location_names[-1]
-        matches=[]
-        for section in sections:
-            # location is a local path if possible, so we need
-            # to convert 'file://' urls to local paths if necessary.
-            # This also avoids having file:///path be a more exact
-            # match than '/path'.
-            if section.startswith('file://'):
-                section_path = urlutils.local_path_from_url(section)
-            else:
-                section_path = section
-            section_names = section_path.split('/')
-            if section.endswith('/'):
-                del section_names[-1]
-            names = zip(location_names, section_names)
-            matched = True
-            for name in names:
-                if not fnmatch.fnmatch(name[0], name[1]):
-                    matched = False
-                    break
-            if not matched:
-                continue
-            # so, for the common prefix they matched.
-            # if section is longer, no match.
-            if len(section_names) > len(location_names):
-                continue
-            matches.append((len(section_names), section,
-                            '/'.join(location_names[len(section_names):])))
+        matches = list(_iter_for_location_by_parts(self._get_parser(),
+                                                   self.location))
         # put the longest (aka more specific) locations first
-        matches.sort(reverse=True)
-        sections = []
-        for (length, section, extra_path) in matches:
-            sections.append((section, extra_path))
+        matches.sort(
+            key=lambda (section, extra_path, length): (length, section),
+            reverse=True)
+        for (section, extra_path, length) in matches:
+            yield section, extra_path
             # should we stop looking for parent configs here?
             try:
                 if self._get_parser()[section].as_bool('ignore_parents'):
                     break
             except KeyError:
                 pass
-        return sections
 
     def _get_sections(self, name=None):
         """See IniBasedConfig._get_sections()."""
@@ -2068,6 +2094,422 @@
         self._transport.put_file(self._filename, out_file)
 
 
+class ReadOnlySection(object):
+    """A section defines a dict of options.
+
+    This is merely a read-only dict which can add some knowledge about the
+    options.
+    """
+
+    def __init__(self, section_id, options):
+        self.id = section_id
+        # We re-use the dict-like object received
+        self.options = options
+
+    def get(self, name, default=None):
+        return self.options.get(name, default)
+
+
+_NewlyCreatedOption = object()
+"""Was the option created during the MutableSection lifetime"""
+
+
+class MutableSection(ReadOnlySection):
+    """A section allowing changes and keeping track of the original values."""
+
+    def __init__(self, section_id, options):
+        super(MutableSection, self).__init__(section_id, options)
+        self.orig = {}
+
+    def set(self, name, value):
+        if name not in self.options:
+            # This is a new option
+            self.orig[name] = _NewlyCreatedOption
+        elif name not in self.orig:
+            self.orig[name] = self.get(name, None)
+        self.options[name] = value
+
+    def remove(self, name):
+        if name not in self.orig:
+            self.orig[name] = self.get(name, None)
+        del self.options[name]
+
+
+class Store(object):
+    """Abstract interface to persistent storage for configuration options."""
+
+    readonly_section_class = ReadOnlySection
+    mutable_section_class = MutableSection
+
+    @property
+    def loaded(self):
+        raise NotImplementedError(self.loaded)
+
+    def load(self):
+        raise NotImplementedError(self.load)
+
+    def _load_from_string(self, str_or_unicode):
+        """Create a store from a string in configobj syntax.
+
+        :param str_or_unicode: A string representing the file content. This will
+            be encoded to suit store needs internally.
+
+        This is for tests and should not be used in production unless a
+        convincing use case can be demonstrated :)
+        """
+        raise NotImplementedError(self._load_from_string)
+
+    def save(self):
+        raise NotImplementedError(self.save)
+
+    def external_url(self):
+        raise NotImplementedError(self.external_url)
+
+    def get_sections(self):
+        """Returns an ordered iterable of existing sections.
+
+        :returns: An iterable of (name, dict).
+        """
+        raise NotImplementedError(self.get_sections)
+
+    def get_mutable_section(self, section_name=None):
+        """Returns the specified mutable section.
+
+        :param section_name: The section identifier
+        """
+        raise NotImplementedError(self.get_mutable_section)
+
+
+class ConfigObjStore(Store):
+
+    def __init__(self, transport, file_name):
+        """A config Store using ConfigObj for storage.
+
+        :param transport: The transport object where the config file is located.
+
+        :param file_name: The config file basename in the transport directory.
+        """
+        super(ConfigObjStore, self).__init__()
+        self.transport = transport
+        self.file_name = file_name
+        self._config_obj = None
+
+    @property
+    def loaded(self):
+        return self._config_obj != None
+
+    def load(self):
+        """Load the store from the associated file."""
+        if self.loaded:
+            return
+        content = self.transport.get_bytes(self.file_name)
+        self._load_from_string(content)
+
+    def _load_from_string(self, str_or_unicode):
+        """Create a config store from a string.
+
+        :param str_or_unicode: A string representing the file content. This will
+            be utf-8 encoded internally.
+
+        This is for tests and should not be used in production unless a
+        convincing use case can be demonstrated :)
+        """
+        if self.loaded:
+            raise AssertionError('Already loaded: %r' % (self._config_obj,))
+        co_input = StringIO(str_or_unicode.encode('utf-8'))
+        try:
+            # The config files are always stored utf8-encoded
+            self._config_obj = ConfigObj(co_input, encoding='utf-8')
+        except configobj.ConfigObjError, e:
+            self._config_obj = None
+            raise errors.ParseConfigError(e.errors, self.external_url())
+
+    def save(self):
+        if not self.loaded:
+            # Nothing to save
+            return
+        out = StringIO()
+        self._config_obj.write(out)
+        self.transport.put_bytes(self.file_name, out.getvalue())
+
+    def external_url(self):
+        # FIXME: external_url should really accepts an optional relpath
+        # parameter (bug #750169) :-/ -- vila 2011-04-04
+        # The following will do in the interim but maybe we don't want to
+        # expose a path here but rather a config ID and its associated
+        # object </hand wawe>.
+        return os.path.join(self.transport.external_url(), self.file_name)
+
+    def get_sections(self):
+        """Get the configobj section in the file order.
+
+        :returns: An iterable of (name, dict).
+        """
+        # We need a loaded store
+        self.load()
+        cobj = self._config_obj
+        if cobj.scalars:
+            yield self.readonly_section_class(None, cobj)
+        for section_name in cobj.sections:
+            yield self.readonly_section_class(section_name, cobj[section_name])
+
+    def get_mutable_section(self, section_name=None):
+        # We need a loaded store
+        try:
+            self.load()
+        except errors.NoSuchFile:
+            # The file doesn't exist, let's pretend it was empty
+            self._load_from_string('')
+        if section_name is None:
+            section = self._config_obj
+        else:
+            section = self._config_obj.setdefault(section_name, {})
+        return self.mutable_section_class(section_name, section)
+
+
+# Note that LockableConfigObjStore inherits from ConfigObjStore because we need
+# unlockable stores for use with objects that can already ensure the locking
+# (think branches). If different stores (not based on ConfigObj) are created,
+# they may face the same issue.
+
+
+class LockableConfigObjStore(ConfigObjStore):
+    """A ConfigObjStore using locks on save to ensure store integrity."""
+
+    def __init__(self, transport, file_name, lock_dir_name=None):
+        """A config Store using ConfigObj for storage.
+
+        :param transport: The transport object where the config file is located.
+
+        :param file_name: The config file basename in the transport directory.
+        """
+        if lock_dir_name is None:
+            lock_dir_name = 'lock'
+        self.lock_dir_name = lock_dir_name
+        super(LockableConfigObjStore, self).__init__(transport, file_name)
+        self._lock = lockdir.LockDir(self.transport, self.lock_dir_name)
+
+    def lock_write(self, token=None):
+        """Takes a write lock in the directory containing the config file.
+
+        If the directory doesn't exist it is created.
+        """
+        # FIXME: This doesn't check the ownership of the created directories as
+        # ensure_config_dir_exists does. It should if the transport is local
+        # -- vila 2011-04-06
+        self.transport.create_prefix()
+        return self._lock.lock_write(token)
+
+    def unlock(self):
+        self._lock.unlock()
+
+    def break_lock(self):
+        self._lock.break_lock()
+
+    @needs_write_lock
+    def save(self):
+        super(LockableConfigObjStore, self).save()
+
+
+# FIXME: global, bazaar, shouldn't that be 'user' instead or even
+# 'user_defaults' as opposed to 'user_overrides', 'system_defaults'
+# (/etc/bzr/bazaar.conf) and 'system_overrides' ? -- vila 2011-04-05
+class GlobalStore(LockableConfigObjStore):
+
+    def __init__(self, possible_transports=None):
+        t = transport.get_transport(config_dir(),
+                                    possible_transports=possible_transports)
+        super(GlobalStore, self).__init__(t, 'bazaar.conf')
+
+
+class LocationStore(LockableConfigObjStore):
+
+    def __init__(self, possible_transports=None):
+        t = transport.get_transport(config_dir(),
+                                    possible_transports=possible_transports)
+        super(LocationStore, self).__init__(t, 'locations.conf')
+
+
+class BranchStore(ConfigObjStore):
+
+    def __init__(self, branch):
+        super(BranchStore, self).__init__(branch.control_transport,
+                                          'branch.conf')
+
+class SectionMatcher(object):
+    """Select sections into a given Store.
+
+    This intended to be used to postpone getting an iterable of sections from a
+    store.
+    """
+
+    def __init__(self, store):
+        self.store = store
+
+    def get_sections(self):
+        # This is where we require loading the store so we can see all defined
+        # sections.
+        sections = self.store.get_sections()
+        # Walk the revisions in the order provided
+        for s in sections:
+            if self.match(s):
+                yield s
+
+    def match(self, secion):
+        raise NotImplementedError(self.match)
+
+
+class LocationSection(ReadOnlySection):
+
+    def __init__(self, section, length, extra_path):
+        super(LocationSection, self).__init__(section.id, section.options)
+        self.length = length
+        self.extra_path = extra_path
+
+    def get(self, name, default=None):
+        value = super(LocationSection, self).get(name, default)
+        if value is not None:
+            policy_name = self.get(name + ':policy', None)
+            policy = _policy_value.get(policy_name, POLICY_NONE)
+            if policy == POLICY_APPENDPATH:
+                value = urlutils.join(value, self.extra_path)
+        return value
+
+
+class LocationMatcher(SectionMatcher):
+
+    def __init__(self, store, location):
+        super(LocationMatcher, self).__init__(store)
+        self.location = location
+
+    def get_sections(self):
+        # Override the default implementation as we want to change the order
+
+        # The following is a bit hackish but ensures compatibility with
+        # LocationConfig by reusing the same code
+        sections = list(self.store.get_sections())
+        filtered_sections = _iter_for_location_by_parts(
+            [s.id for s in sections], self.location)
+        iter_sections = iter(sections)
+        matching_sections = []
+        for section_id, extra_path, length in filtered_sections:
+            # a section id is unique for a given store so it's safe to iterate
+            # again
+            section = iter_sections.next()
+            if section_id == section.id:
+                matching_sections.append(
+                    LocationSection(section, length, extra_path))
+        # We want the longest (aka more specific) locations first
+        sections = sorted(matching_sections,
+                          key=lambda section: (section.length, section.id),
+                          reverse=True)
+        # Sections mentioning 'ignore_parents' restrict the selection
+        for section in sections:
+            # FIXME: We really want to use as_bool below -- vila 2011-04-07
+            ignore = section.get('ignore_parents', None)
+            if ignore is not None:
+                ignore = ui.bool_from_string(ignore)
+            if ignore:
+                break
+            # Finally, we have a valid section
+            yield section
+
+
+class ConfigStack(object):
+    """A stack of configurations where an option can be defined"""
+
+    def __init__(self, sections=None, get_mutable_section=None):
+        """Creates a stack of sections with an optional store for changes.
+
+        :param sections: A list of ReadOnlySection or callables that returns an
+            iterable of ReadOnlySection.
+
+        :param get_mutable_section: A callable that returns a MutableSection
+            where changes are recorded.
+        """
+        if sections is None:
+            sections = []
+        self.sections = sections
+        self.get_mutable_section = get_mutable_section
+
+    def get(self, name):
+        """Return the *first* option value found in the sections.
+
+        This is where we guarantee that sections coming from Store are loaded
+        lazily: the loading is delayed until we need to either check that an
+        option exists or get its value, which in turn may require to discover
+        in which sections it can be defined. Both of these (section and option
+        existence) require loading the store (even partially).
+        """
+        # Ensuring lazy loading is achieved by delaying section matching until
+        # it can be avoided anymore by using callables to describe (possibly
+        # empty) section lists.
+        for section_or_callable in self.sections:
+            # Each section can expand to multiple ones when a callable is used
+            if callable(section_or_callable):
+                sections = section_or_callable()
+            else:
+                sections = [section_or_callable]
+            for section in sections:
+                value = section.get(name)
+                if value is not None:
+                    return value
+        # No definition was found
+        return None
+
+    def set(self, name, value):
+        """Set a new value for the option.
+
+        This is where we guarantee that the mutable section is lazily loaded:
+        this means we won't load the corresponding store before setting a value.
+        """
+        section = self.get_mutable_section()
+        section.set(name, value)
+
+    def remove(self, name):
+        """Remove an existing option.
+
+        This is where we guarantee that the mutable section is lazily loaded:
+        this means we won't load the correspoding store before trying to delete
+        an option. In practice the store will often be loaded but this allows
+        catching some programming errors.
+        """
+        section = self.get_mutable_section()
+        section.remove(name)
+
+
+class GlobalStack(ConfigStack):
+
+    def __init__(self):
+        # Get a GlobalStore
+        gstore = GlobalStore()
+        super(GlobalStack, self).__init__([gstore.get_sections],
+                                          gstore.get_mutable_section)
+
+
+class LocationStack(ConfigStack):
+
+    def __init__(self, location):
+        lstore = LocationStore()
+        matcher = LocationMatcher(lstore, location)
+        gstore = GlobalStore()
+        super(LocationStack, self).__init__(
+            [matcher.get_sections, gstore.get_sections],
+            lstore.get_mutable_section)
+
+
+class BranchStack(ConfigStack):
+
+    def __init__(self, branch):
+        bstore = BranchStore(branch)
+        lstore = LocationStore()
+        matcher = LocationMatcher(lstore, branch.base)
+        gstore = GlobalStore()
+        super(BranchStack, self).__init__(
+            [matcher.get_sections, bstore.get_sections, gstore.get_sections],
+            bstore.get_mutable_section)
+
+
 class cmd_config(commands.Command):
     __doc__ = """Display, set or remove a configuration option.
 

=== modified file 'bzrlib/errors.py'
--- bzrlib/errors.py	2011-03-12 23:58:55 +0000
+++ bzrlib/errors.py	2011-04-12 12:36:46 +0000
@@ -1766,12 +1766,12 @@
 
 class ParseConfigError(BzrError):
 
+    _fmt = "Error(s) parsing config file %(filename)s:\n%(errors)s"
+
     def __init__(self, errors, filename):
-        if filename is None:
-            filename = ""
-        message = "Error(s) parsing config file %s:\n%s" % \
-            (filename, ('\n'.join(e.msg for e in errors)))
-        BzrError.__init__(self, message)
+        BzrError.__init__(self)
+        self.filename = filename
+        self.errors = '\n'.join(e.msg for e in errors)
 
 
 class NoEmailInUsername(BzrError):

=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2011-04-05 12:11:26 +0000
+++ bzrlib/tests/test_config.py	2011-04-12 12:36:46 +0000
@@ -36,6 +36,7 @@
     mergetools,
     ui,
     urlutils,
+    registry,
     tests,
     trace,
     transport,
@@ -62,6 +63,23 @@
 
 load_tests = scenarios.load_tests_apply_scenarios
 
+# We need adpaters that can build a config store in a test context. Test
+# classes, based on TestCaseWithTransport, can use the registry to parametrize
+# themselves. The builder will receive a test instance and should return a
+# ready-to-use store.  Plugins that defines new stores can also register
+# themselves here to be tested against the tests defined below.
+test_store_builder_registry = registry.Registry()
+test_store_builder_registry.register(
+    'configobj', lambda test: config.ConfigObjStore(test.get_transport(),
+                                                    'configobj.conf'))
+test_store_builder_registry.register(
+    'bazaar', lambda test: config.GlobalStore())
+test_store_builder_registry.register(
+    'location', lambda test: config.LocationStore())
+test_store_builder_registry.register(
+    'branch', lambda test: config.BranchStore(test.branch))
+
+
 
 sample_long_alias="log -r-15..-1 --line"
 sample_config_text = u"""
@@ -832,7 +850,7 @@
         def c1_write_config_file():
             before_writing.set()
             c1_orig()
-            # The lock is held we wait for the main thread to decide when to
+            # The lock is held. We wait for the main thread to decide when to
             # continue
             after_writing.wait()
         c1._write_config_file = c1_write_config_file
@@ -865,7 +883,7 @@
        c1_orig = c1._write_config_file
        def c1_write_config_file():
            ready_to_write.set()
-           # The lock is held we wait for the main thread to decide when to
+           # The lock is held. We wait for the main thread to decide when to
            # continue
            do_writing.wait()
            c1_orig()
@@ -1264,55 +1282,52 @@
         self.failUnless(isinstance(global_config, config.GlobalConfig))
         self.failUnless(global_config is my_config._get_global_config())
 
+    def assertLocationMatching(self, expected):
+        self.assertEqual(expected,
+                         list(self.my_location_config._get_matching_sections()))
+
     def test__get_matching_sections_no_match(self):
         self.get_branch_config('/')
-        self.assertEqual([], self.my_location_config._get_matching_sections())
+        self.assertLocationMatching([])
 
     def test__get_matching_sections_exact(self):
         self.get_branch_config('http://www.example.com')
-        self.assertEqual([('http://www.example.com', '')],
-                         self.my_location_config._get_matching_sections())
+        self.assertLocationMatching([('http://www.example.com', '')])
 
     def test__get_matching_sections_suffix_does_not(self):
         self.get_branch_config('http://www.example.com-com')
-        self.assertEqual([], self.my_location_config._get_matching_sections())
+        self.assertLocationMatching([])
 
     def test__get_matching_sections_subdir_recursive(self):
         self.get_branch_config('http://www.example.com/com')
-        self.assertEqual([('http://www.example.com', 'com')],
-                         self.my_location_config._get_matching_sections())
+        self.assertLocationMatching([('http://www.example.com', 'com')])
 
     def test__get_matching_sections_ignoreparent(self):
         self.get_branch_config('http://www.example.com/ignoreparent')
-        self.assertEqual([('http://www.example.com/ignoreparent', '')],
-                         self.my_location_config._get_matching_sections())
+        self.assertLocationMatching([('http://www.example.com/ignoreparent',
+                                      '')])
 
     def test__get_matching_sections_ignoreparent_subdir(self):
         self.get_branch_config(
             'http://www.example.com/ignoreparent/childbranch')
-        self.assertEqual([('http://www.example.com/ignoreparent',
-                           'childbranch')],
-                         self.my_location_config._get_matching_sections())
+        self.assertLocationMatching([('http://www.example.com/ignoreparent',
+                                      'childbranch')])
 
     def test__get_matching_sections_subdir_trailing_slash(self):
         self.get_branch_config('/b')
-        self.assertEqual([('/b/', '')],
-                         self.my_location_config._get_matching_sections())
+        self.assertLocationMatching([('/b/', '')])
 
     def test__get_matching_sections_subdir_child(self):
         self.get_branch_config('/a/foo')
-        self.assertEqual([('/a/*', ''), ('/a/', 'foo')],
-                         self.my_location_config._get_matching_sections())
+        self.assertLocationMatching([('/a/*', ''), ('/a/', 'foo')])
 
     def test__get_matching_sections_subdir_child_child(self):
         self.get_branch_config('/a/foo/bar')
-        self.assertEqual([('/a/*', 'bar'), ('/a/', 'foo/bar')],
-                         self.my_location_config._get_matching_sections())
+        self.assertLocationMatching([('/a/*', 'bar'), ('/a/', 'foo/bar')])
 
     def test__get_matching_sections_trailing_slash_with_children(self):
         self.get_branch_config('/a/')
-        self.assertEqual([('/a/', '')],
-                         self.my_location_config._get_matching_sections())
+        self.assertLocationMatching([('/a/', '')])
 
     def test__get_matching_sections_explicit_over_glob(self):
         # XXX: 2006-09-08 jamesh
@@ -1320,8 +1335,7 @@
         # was a config section for '/a/?', it would get precedence
         # over '/a/c'.
         self.get_branch_config('/a/c')
-        self.assertEqual([('/a/c', ''), ('/a/*', ''), ('/a/', 'c')],
-                         self.my_location_config._get_matching_sections())
+        self.assertLocationMatching([('/a/c', ''), ('/a/*', ''), ('/a/', 'c')])
 
     def test__get_option_policy_normal(self):
         self.get_branch_config('http://www.example.com')
@@ -1818,13 +1832,443 @@
         self.assertIs(None, bzrdir_config.get_default_stack_on())
 
 
+class TestConfigReadOnlySection(tests.TestCase):
+
+    # FIXME: Parametrize so that all sections produced by Stores run these
+    # tests -- vila 2011-04-01
+
+    def test_get_a_value(self):
+        a_dict = dict(foo='bar')
+        section = config.ReadOnlySection('myID', a_dict)
+        self.assertEquals('bar', section.get('foo'))
+
+    def test_get_unkown_option(self):
+        a_dict = dict()
+        section = config.ReadOnlySection(None, a_dict)
+        self.assertEquals('out of thin air',
+                          section.get('foo', 'out of thin air'))
+
+    def test_options_is_shared(self):
+        a_dict = dict()
+        section = config.ReadOnlySection(None, a_dict)
+        self.assertIs(a_dict, section.options)
+
+
+class TestConfigMutableSection(tests.TestCase):
+
+    # FIXME: Parametrize so that all sections (including os.environ and the
+    # ones produced by Stores) run these tests -- vila 2011-04-01
+
+    def test_set(self):
+        a_dict = dict(foo='bar')
+        section = config.MutableSection('myID', a_dict)
+        section.set('foo', 'new_value')
+        self.assertEquals('new_value', section.get('foo'))
+        # The change appears in the shared section
+        self.assertEquals('new_value', a_dict.get('foo'))
+        # We keep track of the change
+        self.assertTrue('foo' in section.orig)
+        self.assertEquals('bar', section.orig.get('foo'))
+
+    def test_set_preserve_original_once(self):
+        a_dict = dict(foo='bar')
+        section = config.MutableSection('myID', a_dict)
+        section.set('foo', 'first_value')
+        section.set('foo', 'second_value')
+        # We keep track of the original value
+        self.assertTrue('foo' in section.orig)
+        self.assertEquals('bar', section.orig.get('foo'))
+
+    def test_remove(self):
+        a_dict = dict(foo='bar')
+        section = config.MutableSection('myID', a_dict)
+        section.remove('foo')
+        # We get None for unknown options via the default value
+        self.assertEquals(None, section.get('foo'))
+        # Or we just get the default value
+        self.assertEquals('unknown', section.get('foo', 'unknown'))
+        self.assertFalse('foo' in section.options)
+        # We keep track of the deletion
+        self.assertTrue('foo' in section.orig)
+        self.assertEquals('bar', section.orig.get('foo'))
+
+    def test_remove_new_option(self):
+        a_dict = dict()
+        section = config.MutableSection('myID', a_dict)
+        section.set('foo', 'bar')
+        section.remove('foo')
+        self.assertFalse('foo' in section.options)
+        # The option didn't exist initially so it we need to keep track of it
+        # with a special value
+        self.assertTrue('foo' in section.orig)
+        self.assertEquals(config._NewlyCreatedOption, section.orig['foo'])
+
+
+class TestStore(tests.TestCaseWithTransport):
+
+    def assertSectionContent(self, expected, section):
+        """Assert that some options have the proper values in a section."""
+        expected_name, expected_options = expected
+        self.assertEquals(expected_name, section.id)
+        self.assertEquals(
+            expected_options,
+            dict([(k, section.get(k)) for k in expected_options.keys()]))
+
+
+class TestReadonlyStore(TestStore):
+
+    scenarios = [(key, {'get_store': builder})
+                 for key, builder in test_store_builder_registry.iteritems()]
+
+    def setUp(self):
+        super(TestReadonlyStore, self).setUp()
+        self.branch = self.make_branch('branch')
+
+    def test_building_delays_load(self):
+        store = self.get_store(self)
+        self.assertEquals(False, store.loaded)
+        store._load_from_string('')
+        self.assertEquals(True, store.loaded)
+
+    def test_get_no_sections_for_empty(self):
+        store = self.get_store(self)
+        store._load_from_string('')
+        self.assertEquals([], list(store.get_sections()))
+
+    def test_get_default_section(self):
+        store = self.get_store(self)
+        store._load_from_string('foo=bar')
+        sections = list(store.get_sections())
+        self.assertLength(1, sections)
+        self.assertSectionContent((None, {'foo': 'bar'}), sections[0])
+
+    def test_get_named_section(self):
+        store = self.get_store(self)
+        store._load_from_string('[baz]\nfoo=bar')
+        sections = list(store.get_sections())
+        self.assertLength(1, sections)
+        self.assertSectionContent(('baz', {'foo': 'bar'}), sections[0])
+
+    def test_load_from_string_fails_for_non_empty_store(self):
+        store = self.get_store(self)
+        store._load_from_string('foo=bar')
+        self.assertRaises(AssertionError, store._load_from_string, 'bar=baz')
+
+
+class TestMutableStore(TestStore):
+
+    scenarios = [(key, {'store_id': key, 'get_store': builder})
+                 for key, builder in test_store_builder_registry.iteritems()]
+
+    def setUp(self):
+        super(TestMutableStore, self).setUp()
+        self.transport = self.get_transport()
+        self.branch = self.make_branch('branch')
+
+    def has_store(self, store):
+        store_basename = urlutils.relative_url(self.transport.external_url(),
+                                               store.external_url())
+        return self.transport.has(store_basename)
+
+    def test_save_empty_creates_no_file(self):
+        if self.store_id == 'branch':
+            raise tests.TestNotApplicable(
+                'branch.conf is *always* created when a branch is initialized')
+        store = self.get_store(self)
+        store.save()
+        self.assertEquals(False, self.has_store(store))
+
+    def test_save_emptied_succeeds(self):
+        store = self.get_store(self)
+        store._load_from_string('foo=bar\n')
+        section = store.get_mutable_section(None)
+        section.remove('foo')
+        store.save()
+        self.assertEquals(True, self.has_store(store))
+        modified_store = self.get_store(self)
+        sections = list(modified_store.get_sections())
+        self.assertLength(0, sections)
+
+    def test_save_with_content_succeeds(self):
+        if self.store_id == 'branch':
+            raise tests.TestNotApplicable(
+                'branch.conf is *always* created when a branch is initialized')
+        store = self.get_store(self)
+        store._load_from_string('foo=bar\n')
+        self.assertEquals(False, self.has_store(store))
+        store.save()
+        self.assertEquals(True, self.has_store(store))
+        modified_store = self.get_store(self)
+        sections = list(modified_store.get_sections())
+        self.assertLength(1, sections)
+        self.assertSectionContent((None, {'foo': 'bar'}), sections[0])
+
+    def test_set_option_in_empty_store(self):
+        store = self.get_store(self)
+        section = store.get_mutable_section(None)
+        section.set('foo', 'bar')
+        store.save()
+        modified_store = self.get_store(self)
+        sections = list(modified_store.get_sections())
+        self.assertLength(1, sections)
+        self.assertSectionContent((None, {'foo': 'bar'}), sections[0])
+
+    def test_set_option_in_default_section(self):
+        store = self.get_store(self)
+        store._load_from_string('')
+        section = store.get_mutable_section(None)
+        section.set('foo', 'bar')
+        store.save()
+        modified_store = self.get_store(self)
+        sections = list(modified_store.get_sections())
+        self.assertLength(1, sections)
+        self.assertSectionContent((None, {'foo': 'bar'}), sections[0])
+
+    def test_set_option_in_named_section(self):
+        store = self.get_store(self)
+        store._load_from_string('')
+        section = store.get_mutable_section('baz')
+        section.set('foo', 'bar')
+        store.save()
+        modified_store = self.get_store(self)
+        sections = list(modified_store.get_sections())
+        self.assertLength(1, sections)
+        self.assertSectionContent(('baz', {'foo': 'bar'}), sections[0])
+
+
+class TestConfigObjStore(TestStore):
+
+    def test_loading_unknown_file_fails(self):
+        store = config.ConfigObjStore(self.get_transport(), 'I-do-not-exist')
+        self.assertRaises(errors.NoSuchFile, store.load)
+
+    def test_invalid_content(self):
+        store = config.ConfigObjStore(self.get_transport(), 'foo.conf', )
+        self.assertEquals(False, store.loaded)
+        exc = self.assertRaises(
+            errors.ParseConfigError, store._load_from_string,
+            'this is invalid !')
+        self.assertEndsWith(exc.filename, 'foo.conf')
+        # And the load failed
+        self.assertEquals(False, store.loaded)
+
+    def test_get_embedded_sections(self):
+        # A more complicated example (which also shows that section names and
+        # option names share the same name space...)
+        # FIXME: This should be fixed by forbidding dicts as values ?
+        # -- vila 2011-04-05
+        store = config.ConfigObjStore(self.get_transport(), 'foo.conf', )
+        store._load_from_string('''
+foo=bar
+l=1,2
+[DEFAULT]
+foo_in_DEFAULT=foo_DEFAULT
+[bar]
+foo_in_bar=barbar
+[baz]
+foo_in_baz=barbaz
+[[qux]]
+foo_in_qux=quux
+''')
+        sections = list(store.get_sections())
+        self.assertLength(4, sections)
+        # The default section has no name.
+        # List values are provided as lists
+        self.assertSectionContent((None, {'foo': 'bar', 'l': ['1', '2']}),
+                                  sections[0])
+        self.assertSectionContent(
+            ('DEFAULT', {'foo_in_DEFAULT': 'foo_DEFAULT'}), sections[1])
+        self.assertSectionContent(
+            ('bar', {'foo_in_bar': 'barbar'}), sections[2])
+        # sub sections are provided as embedded dicts.
+        self.assertSectionContent(
+            ('baz', {'foo_in_baz': 'barbaz', 'qux': {'foo_in_qux': 'quux'}}),
+            sections[3])
+
+
+class TestLockableConfigObjStore(TestStore):
+
+    def test_create_store_in_created_dir(self):
+        t = self.get_transport('dir/subdir')
+        store = config.LockableConfigObjStore(t, 'foo.conf')
+        store.get_mutable_section(None).set('foo', 'bar')
+        store.save()
+
+    # FIXME: We should adapt the tests in TestLockableConfig about concurrent
+    # writes. Since this requires a clearer rewrite, I'll just rely on using
+    # the same code in LockableConfigObjStore (copied from LockableConfig, but
+    # trivial enough, the main difference is that we add @needs_write_lock on
+    # save() instead of set_user_option() and remove_user_option()). The intent
+    # is to ensure that we always get a valid content for the store even when
+    # concurrent accesses occur, read/write, write/write. It may be worth
+    # looking into removing the lock dir when it;s not needed anymore and look
+    # at possible fallouts for concurrent lockers -- vila 20110-04-06
+
+
+class TestSectionMatcher(TestStore):
+
+    scenarios = [('location', {'matcher': config.LocationMatcher})]
+
+    def get_store(self, file_name):
+        return config.ConfigObjStore(self.get_readonly_transport(), file_name)
+
+    def test_no_matches_for_empty_stores(self):
+        store = self.get_store('foo.conf')
+        store._load_from_string('')
+        matcher = self.matcher(store, '/bar')
+        self.assertEquals([], list(matcher.get_sections()))
+
+    def test_build_doesnt_load_store(self):
+        store = self.get_store('foo.conf')
+        matcher = self.matcher(store, '/bar')
+        self.assertFalse(store.loaded)
+
+
+class TestLocationSection(tests.TestCase):
+
+    def get_section(self, options, extra_path):
+        section = config.ReadOnlySection('foo', options)
+        # We don't care about the length so we use '0'
+        return config.LocationSection(section, 0, extra_path)
+
+    def test_simple_option(self):
+        section = self.get_section({'foo': 'bar'}, '')
+        self.assertEquals('bar', section.get('foo'))
+
+    def test_option_with_extra_path(self):
+        section = self.get_section({'foo': 'bar', 'foo:policy': 'appendpath'},
+                                   'baz')
+        self.assertEquals('bar/baz', section.get('foo'))
+
+    def test_invalid_policy(self):
+        section = self.get_section({'foo': 'bar', 'foo:policy': 'die'},
+                                   'baz')
+        # invalid policies are ignored
+        self.assertEquals('bar', section.get('foo'))
+
+
+class TestLocationMatcher(TestStore):
+
+    def get_store(self, file_name):
+        return config.ConfigObjStore(self.get_readonly_transport(), file_name)
+
+    def test_more_specific_sections_first(self):
+        store = self.get_store('foo.conf')
+        store._load_from_string('''
+[/foo]
+section=/foo
+[/foo/bar]
+section=/foo/bar
+''')
+        self.assertEquals(['/foo', '/foo/bar'],
+                          [section.id for section in store.get_sections()])
+        matcher = config.LocationMatcher(store, '/foo/bar/baz')
+        sections = list(matcher.get_sections())
+        self.assertEquals([3, 2],
+                          [section.length for section in sections])
+        self.assertEquals(['/foo/bar', '/foo'],
+                          [section.id for section in sections])
+        self.assertEquals(['baz', 'bar/baz'],
+                          [section.extra_path for section in sections])
+
+
+class TestConfigStackGet(tests.TestCase):
+
+    # FIXME: This should be parametrized for all known ConfigStack or dedicated
+    # paramerized tests created to avoid bloating -- vila 2011-03-31
+
+    def test_single_config_get(self):
+        conf = dict(foo='bar')
+        conf_stack = config.ConfigStack([conf])
+        self.assertEquals('bar', conf_stack.get('foo'))
+
+    def test_get_first_definition(self):
+        conf1 = dict(foo='bar')
+        conf2 = dict(foo='baz')
+        conf_stack = config.ConfigStack([conf1, conf2])
+        self.assertEquals('bar', conf_stack.get('foo'))
+
+    def test_get_embedded_definition(self):
+        conf1 = dict(yy='12')
+        conf2 = config.ConfigStack([dict(xx='42'), dict(foo='baz')])
+        conf_stack = config.ConfigStack([conf1, conf2])
+        self.assertEquals('baz', conf_stack.get('foo'))
+
+    def test_get_for_empty_stack(self):
+        conf_stack = config.ConfigStack()
+        self.assertEquals(None, conf_stack.get('foo'))
+
+    def test_get_for_empty_section_callable(self):
+        conf_stack = config.ConfigStack([lambda : []])
+        self.assertEquals(None, conf_stack.get('foo'))
+
+
+class TestConfigStackSet(tests.TestCaseWithTransport):
+
+    # FIXME: This should be parametrized for all known ConfigStack or dedicated
+    # paramerized tests created to avoid bloating -- vila 2011-04-05
+
+    def test_simple_set(self):
+        store = config.ConfigObjStore(self.get_transport(), 'test.conf')
+        store._load_from_string('foo=bar')
+        conf = config.ConfigStack(
+            [store.get_sections], store.get_mutable_section)
+        self.assertEquals('bar', conf.get('foo'))
+        conf.set('foo', 'baz')
+        # Did we get it back ?
+        self.assertEquals('baz', conf.get('foo'))
+
+    def test_set_creates_a_new_section(self):
+        store = config.ConfigObjStore(self.get_transport(), 'test.conf')
+        conf = config.ConfigStack(
+            [store.get_sections], store.get_mutable_section)
+        conf.set('foo', 'baz')
+        self.assertEquals, 'baz', conf.get('foo')
+
+
+class TestConfigStackRemove(tests.TestCaseWithTransport):
+
+    # FIXME: This should be parametrized for all known ConfigStack or dedicated
+    # paramerized tests created to avoid bloating -- vila 2011-04-06
+
+    def test_remove_existing(self):
+        store = config.ConfigObjStore(self.get_transport(), 'test.conf')
+        store._load_from_string('foo=bar')
+        conf = config.ConfigStack(
+            [store.get_sections], store.get_mutable_section)
+        self.assertEquals('bar', conf.get('foo'))
+        conf.remove('foo')
+        # Did we get it back ?
+        self.assertEquals(None, conf.get('foo'))
+
+    def test_remove_unknown(self):
+        store = config.ConfigObjStore(self.get_transport(), 'test.conf')
+        conf = config.ConfigStack(
+            [store.get_sections], store.get_mutable_section)
+        self.assertRaises(KeyError, conf.remove, 'I_do_not_exist')
+
+
+class TestConcreteStacks(tests.TestCaseWithTransport):
+
+    # basic smoke tests
+
+    def test_global_stack(self):
+        stack = config.GlobalStack()
+
+    def test_location_store(self):
+        stack = config.LocationStack('.')
+
+    def test_branch_store(self):
+        b = self.make_branch('.')
+        stack = config.BranchStack(b)
+
+
 class TestConfigGetOptions(tests.TestCaseWithTransport, TestOptionsMixin):
 
     def setUp(self):
         super(TestConfigGetOptions, self).setUp()
         create_configs(self)
 
-    # One variable in none of the above
     def test_no_variable(self):
         # Using branch should query branch, locations and bazaar
         self.assertOptions([], self.branch_config)

=== added file 'doc/developers/configuration.txt'
--- doc/developers/configuration.txt	1970-01-01 00:00:00 +0000
+++ doc/developers/configuration.txt	2011-04-12 12:36:46 +0000
@@ -0,0 +1,102 @@
+Configuring Bazaar
+==================
+
+A configuration option has:
+
+- a name: a valid python identifier (even if it's not used as an
+  identifier in python itself)
+
+- a value: a unicode string
+
+Sections
+--------
+
+Options are grouped into sections which share some properties with the well
+known dict objects:
+
+- the key is the name,
+- you can get, set and remove an option,
+- the value is a unicode string.
+
+MutableSection are needed to set or remove an option, ReadOnlySection should
+be used otherwise.
+
+Stores
+------
+
+Options can persistent in which case they are saved into Stores.
+
+``config.Store`` defines the abstract interface that all stores should
+implement.
+
+This object doesn't provide a direct access to the options, it only provides
+access to Sections. This is deliberate to ensure that sections can be properly
+shared by reusing the same underlying objects. Accessing options should be
+done via the ``Section`` objects.
+
+A ``Store`` can contain one or more sections, each section is uniquely
+identified by a unicode string.
+
+``config.ConfigObjStore`` is an implementation that use ``ConfigObj``.
+
+Depending on the object it is associated with (or not) a ``Store`` also needs
+to implement a locking mechanism. ``LockableConfigObjStore`` implements such a
+mechanism for ``ConfigObj`` based stores.
+
+Classes are provided for the usual Bazaar configuration files and could be
+used as examples to define new ones if needed. The associated tests provides a
+basis for new classes which only need to register themselves in the right
+places to inherit from the existing basic tests and add their own specific
+ones.
+
+Filtering sections
+------------------
+
+For some contexts, only some sections from a given store will apply. Defining
+which is what the ``SectionMatcher`` are about.
+
+The main constraint here is that a ``SectionMatcher`` should delay the loading
+of the associated store as long as possible. The constructor should collect
+all data needed for the selection and uses it while processing the sections in
+``get_sections``.
+
+Only ``ReadOnlySection`` objects are manipulated here but a ``SectionMatcher``
+can return dedicated ``Section`` to provide additional context (the
+``LocationSection`` add an ``extra_path`` attribute to implement the
+``appendpath`` policy for example).
+
+.. FIXME: Replace the appendpath example if/when it's deprecated ;)
+
+Stacks
+------
+
+An option can take different values depending on the context it is used. Such
+a context can involve configuration files, options from the command line,
+default values in bzrlib and then some.
+
+Such a context is implemented by creating a list of ``Section`` stacked upon
+each other. A ``Stack`` can then be asked for an option value and returns the
+first definition found.
+
+This provides a great flexibility to decide priorities between sections when
+the stack is defined without to worry about them in the code itself.
+
+A stack also defines a mutable section (which can be None) to handle
+modifications.
+
+Many sections (or even stores) are aimed at providing default values for an
+option but these sections shouldn't be modified lightly as modifying an option
+used for different contexts will indeed be seen by all these contexts.
+
+Default values in configuration files are defined by users, developers
+shouldn't have to modify them, as such, no mechanism nor heuristics are used
+to find which section (or sections) should be modified, a ``Stack`` defines a
+mutable section when there is no ambiguity, if there is one, then the *user*
+should be able to decide and this case a new ``Stack`` can be created cheaply.
+
+Different stacks can be created for different purposes, the existing
+``Global.Stack``, ``LocationStack`` and ``BranchStack`` can be used as basis
+or examples. These classes are the only ones that should be used in code,
+``Stores`` can be used to build them but shouldn't be used otherwise, ditto
+for sections. Again, the associated tests could and should be used against the
+created stacks.

=== modified file 'doc/developers/index.txt'
--- doc/developers/index.txt	2011-01-06 06:26:23 +0000
+++ doc/developers/index.txt	2011-04-12 12:36:46 +0000
@@ -36,9 +36,10 @@
 .. toctree::
    :maxdepth: 1
 
+   configuration
+   fetch
    transports
    ui
-   fetch
 
 Releasing and Packaging
 =======================

=== modified file 'doc/en/user-guide/configuring_bazaar.txt'
--- doc/en/user-guide/configuring_bazaar.txt	2011-02-07 01:39:42 +0000
+++ doc/en/user-guide/configuring_bazaar.txt	2011-04-12 12:36:46 +0000
@@ -37,6 +37,24 @@
 which shouldn't be reached by the proxy.  (See
 <http://docs.python.org/library/urllib.html> for more details.)
 
+Various ways to configure
+-------------------------
+
+As shown in the example above, there are various ways to
+configure Bazaar, they all share some common properties though,
+an option has:
+
+- a name which is generally a valid python identifier,
+
+- a value which is a string. In some cases, Bazzar will be able
+  to recognize special values like 'True', 'False' to infer a
+  boolean type, but basically, as a user, you will always specify
+  a value as a string.
+
+Options are grouped in various contexts so their name uniquely
+identify them in this context. When needed, options can be made
+persistent by recording them in a configuration file.
+
 
 Configuration files
 -------------------