Merge lp:~vila/bzr/832042-shared-stores into lp:bzr

[Vincent Ladeuil]

Share and cache local config files

This fixes bug #832042 by sharing and caching local config files, opening
the doors wide for adding more config options or files.

The implementation is the simplest one: the library state gains a dict
attribute for all used stores.

A fallback is needed for scripts that never call bzrlib.intialize() until we
fix the bug(s) about always requiring a library state object to be available
whatever script is used and pass these state objects around in the code base
(repo/ranch/wts should know about which state they are used with (somehow
like transports need to be shared)).

The fallback is a bit ugly, but not much than missing the library state :)

Compared to the corresponding change for branches config file, this one if
far lighter and only a couple of tests needed to be fixed. Even there the
use case is at the limit to what is expected to be supported: communication
with a subprocess (for which config files are a bad medium anyway).

To describe impact, numbers speak better than words, so, with the following
patch:

=== modified file 'bzrlib/lockdir.py'
--- bzrlib/lockdir.py 2011-12-19 13:23:58 +0000
+++ bzrlib/lockdir.py 2012-08-02 08:52:49 +0000
@@ -858,6 +858,6 @@
     as it gives some clue who the user is.
     """
     try:
- return config.GlobalConfig().username()
+ return config.GlobalStack().get('email')
     except errors.NoWhoami:
         return osutils.getuser_unicode()

Running the full test suite with -Econfig_stats allows going from:

config.load : 53265
config.save : 4005
config.get : 453640
config.set : 4667
config.remove : 26
old_config.load : 294868
old_config.save : 608
old_config.get : 166067
old_config.set : 475
old_config.remove: 6

to

config.load : 53342
config.save : 4005
config.get : 627394
config.set : 4667
config.remove : 26
old_config.load : 124371
old_config.save : 608
old_config.get : 166067
old_config.set : 475
old_config.remove: 6

I.e. old_config.load goes from 294,868 to 124,371 (-170,497) whereas
config.load goes from 53,265 to 53,342 (+77), or in marketing speak a
221,424% improvement !11!! ;)

More seriously, in this specific case, most of the tests rely on BZR_EMAIL
anyway so what the new design really provides is *avoiding* the IO since the
env var takes precedence (whereas the old design would read the file anyway
*before* looking at the env var). This explains why config.get goes from
453640 to 627394 (+ 173,754) whereas old_config.get stays stable and
config.load only goes up moderately.

So overall, the first revno I started collected statistics for is 5981:

config.load : 296
config.save : 106
config.get : 82
config.set : 52
config.remove : 8
old_config.load : 1002308
old_config.save : 5018
old_config.get : 383768
old_config.set : 4873
old_config.remove: 14

the significant ratio being old_config.get / old_config.load: 0.38

and with this proposal (with still some old config uses) we are at:

config.load : 53310
config.save : 4006
config.get : 627375
config.set : 4686
config.remove : 26
old_config.load : 124352
old_config.save : 588
old_config.get : 166068
old_config.set : 456
old_config.remove: 6

config.get / config.load ratio: 11.76

Since there were a bunch of changes the numbers cannot be compared directly
bu the trend is obvious a single IO for multiple config options instead of
several IOs for a single option. In proper English: feel free to use more
options and config files, they are basically free once you use at least one
(which all bzr commands do).

This patch also provides fresh shared stores for tests or they'll use the
fallback which means they aren't saved until selftest exits (which make no
test (well almost) fail but is still a kind of leak).

On a related note, many tests use TestCaseWithMemoryTransport and/or
branchbuilder relying on the limited ability to handle a tree/branch/repo in
a MemoryTransport, including locking. Locking use the 'email' config option
and as such looks for bazaar.conf... which doesn't exist for a
MemoryTransport... which is fine since we have a sane default and bzr copes
with non-existing config files. Pfew. I wrongly chased these tests as
potential culprits for not using TestCaseInTempDir before realizing they
were indeed allowed to run without an existing bazaar.conf. I thought it was
worth mentioning for those following from home. This is why _shared_stores
is overriden during setup at the TestCaseWithMemoryTransport level instead
of TestCaseInTempDir.

I moved setUp at the beginning of the class for TestCaseWithMemoryTransport
and TestCaseWithTransport as I was fed up with missing them otherwise
(that's the usual pattern in the rest of the code base anyway).

Also:

- split the eager test in bt.test_debug,
- use proper unique urls for remote config files (for both better error
  messages and sharing),

Next possible steps are to:
- remove (finally !) BranchOnlyStack since the cheap optimization that
  triggered its need is not worth the code duplication and complexity
  anymore,
- properly test the library state stuff (http://pad.lv/865368) and get rid
  of the fallback.

[Vincent Ladeuil (vila)]

Can I have a review ? Pretty please ?

[Martin Packman (gz)]

Nice improvement, and all the test updates look good to me. A few comments, only the last really needs some kind of action.

+_shared_stores = {}
+_shared_stores_at_exit_installed = False

Rather than using two variables for this hack, could just start with `_shared_stores = None` then set to empty dict and add atexit function if None.

+ url = store.external_url()
+ try:
+ return stores[url]
+ except KeyError:
+ stores[url] = store

This introduces another incidence of assuming that transport.base is normalised, and also requires a one-to-one mapping between urls and underlying configs across the library lifetime. As only GlobalStore and LocationStore get passed here and we don't expect the return of config_dir() to change across the lifetime of the process (outside of tests which you handle), this is okay. Some documentation pointing out that shared stores are only for per-process-user not per-branch config would be good.

The shared stores dict is also an unbounded cache. It would be possible to use weakrefs, which would also remove the need for seperate process exit cleanup. Given we only expect 2(?) items in the dict the current code is fine.

Always initialising a GlobalStore/LocalStore object to pass in to get_shared_store, then possibly getting a different one back is also inelegant. Why not expect a class rather than instance passed as store to the function instead, which would then enforce the Store could be constructed with no arguments. It seems the possible_transports arg is never given anyway?

+ if exc_type is None:
+ # Save config changes
+ for k, store in self.config_stores.iteritems():
+ store.save_changes()

Why only save config changes if no error has been raised? This logic differs from the pre-library state version where the atexit function will run regardless. Always running but catching errors seems like a better behaviour, the error may be due to some completely unrelated later action and losing an earlier config change would be suprising.

review: Approve

[Vincent Ladeuil (vila)]

> Nice improvement, and all the test updates look good to me. A few comments,
> only the last really needs some kind of action.
>
> +_shared_stores = {}
> +_shared_stores_at_exit_installed = False
>
> Rather than using two variables for this hack, could just start with
> `_shared_stores = None` then set to empty dict and add atexit function if
> None.

But then the same trick should be applied in library_state and make the hack even uglier, I'd rather stop uglifying here ;)

>
> + url = store.external_url()
> + try:
> + return stores[url]
> + except KeyError:
> + stores[url] = store
>
> This introduces another incidence of assuming that transport.base is
> normalised, and also requires a one-to-one mapping between urls and underlying
> configs across the library lifetime.

Well, U in url stands for unique and the design here is that you cannot have different stores at the same url.

I know we had (and still have) issues with badly normalized urls but here there aren't several ways to get a store.

> As only GlobalStore and LocationStore get
> passed here and we don't expect the return of config_dir() to change across
> the lifetime of the process (outside of tests which you handle), this is okay.
> Some documentation pointing out that shared stores are only for per-process-
> user not per-branch config would be good.

Yup, comments added.

>
> The shared stores dict is also an unbounded cache.

Right, that why it should be part of the library state and taken care of there.

> It would be possible to use
> weakrefs, which would also remove the need for seperate process exit cleanup.

I fail to see why weakrefs will help there...

> Given we only expect 2(?) items in the dict the current code is fine.
>
> Always initialising a GlobalStore/LocalStore object to pass in to
> get_shared_store, then possibly getting a different one back is also
> inelegant.

Well, you get a different one "pysically", but since stores are uniquely identified by urls, this *has* to be the same "logical" one.

> Why not expect a class rather than instance passed as store to the
> function instead,

See below.

> which would then enforce the Store could be constructed with
> no arguments.

And why should we force this decision *inside* some inaccessible method/function ? Letting the caller build its own object however it sees fit reduce coupling, that's the point.

> It seems the possible_transports arg is never given anyway?

Not until bzrlib implement all working tree operations across the whole code base, no.

The actual design for Stores is that they use only transport operations, it has been discussed in the past that local config files doesn't *have* to but there is no gain in using direct system calls so I stick with the transport API.

>
> + if exc_type is None:
> + # Save config changes
> + for k, store in self.config_stores.iteritems():
> + store.save_changes()
>
> Why only save config changes if no error has been raised?

Because across bzrlib, config changes are supposed to be saved only when the operation succeeds. If there is a special need to save these (or some...

[Vincent Ladeuil (vila)]

sent to pqm by email

[Martin Packman (gz)]

> > Rather than using two variables for this hack, could just start with
> > `_shared_stores = None` then set to empty dict and add atexit function if
> > None.
>
> But then the same trick should be applied in library_state and make the hack
> even uglier, I'd rather stop uglifying here ;)

The point is library_state only has a single variable and handles cleanup regardless.

> > This introduces another incidence of assuming that transport.base is
> > normalised, and also requires a one-to-one mapping between urls and
> underlying
> > configs across the library lifetime.
>
> Well, U in url stands for unique and the design here is that you cannot have
> different stores at the same url.

That's a gotcha, it actually stands for 'uniform resource locator' and there's no guarentee of uniqueness. Webservers are perfectly within their rights to 'internally redirect' different urls to the same location, and expose multiple resources under the same url.

As mentioned, only sharing local configuration is fine though.

> I fail to see why weakrefs will help there...

You can pass a cleanup function that runs when a weakref is collected, which could save as soon as the config reference was dropped. Would also change behaviour back to re-opening config files unless some part of bzrlib was seperately persisting them, so isn't really what we want for the global/location case.

> And why should we force this decision *inside* some inaccessible
> method/function ? Letting the caller build its own object however it sees fit
> reduce coupling, that's the point.

It does extra work, that all but the first config fetch will be immediately discarded. As this touches disk and so on, it's not entirely trivial, constructing GlobalStore times at 212 usec here. Not very important though, and passing possible_transports would probably also do.

> Not until bzrlib implement all working tree operations across the whole code
> base, no.

I don't understand this...

> I disagree there and we had (and probably still have) cases where some config
> changes are saved *despite* an error occurring (to be honest I'm thinking more
> about branch options here).

I'm less worried about the specifics of the behaviour, and more about it diverging depending on if bzrlib.initialize is used. Perhaps that's not easily avoidable, but if the stores in question are seldom modified by bzr (outside of `bzr config`?) as you suggest then we're unlikely to run into problems. Not relying on library state cleanup would be much nicer, but you know the tradeoffs better than me here.

[Vincent Ladeuil (vila)]

    >> > Rather than using two variables for this hack, could just start with
    >> > `_shared_stores = None` then set to empty dict and add atexit function if
    >> > None.
    >>
    >> But then the same trick should be applied in library_state and make the hack
    >> even uglier, I'd rather stop uglifying here ;)

    > The point is library_state only has a single variable and handles
    > cleanup regardless.

Indeed, and that should be only place when the hack is deleted.

    >> > This introduces another incidence of assuming that transport.base is
    >> > normalised, and also requires a one-to-one mapping between urls and
    >> underlying
    >> > configs across the library lifetime.
    >>
    >> Well, U in url stands for unique and the design here is that you cannot have
    >> different stores at the same url.

    > That's a gotcha, it actually stands for 'uniform resource locator'
    > and there's no guarentee of uniqueness. Webservers are perfectly
    > within their rights to 'internally redirect' different urls to the
    > same location, and expose multiple resources under the same url.

Ha right, uniform not unique, sorry. But the point here is that the same
url cannot provide different stores. Now, if your concern is that people
may be able to access the same store via several urls, well, too bad for
them, they won't share. Will that be an issue ? Not more than trying to
update the same config files from two different processes which will
indicate a problem somewhere (and should lead to warnings if the same
options are modified).

    > As mentioned, only sharing local configuration is fine though.

    >> I fail to see why weakrefs will help there...

    > You can pass a cleanup function that runs when a weakref is
    > collected, which could save as soon as the config reference was
    > dropped.

As you said below, that would save the changes too often (and trigger a
file reload too). The model intended here is to consider the bzrlib
operations atomic rather than the config option modifications. That's
why a library state should be associated to such operations (I'm talking
high-level operations here, roughly the bzr commands).

    > Would also change behaviour back to re-opening config files unless
    > some part of bzrlib was seperately persisting them, so isn't
    > really what we want for the global/location case.

Yup, that's the idea, the file should be opened only once for a given
scope and that scope should be the library state (strictly speaking it
could be the process itself but the lib state is good enough).

    >> And why should we force this decision *inside* some inaccessible
    >> method/function ? Letting the caller build its own object however it sees fit
    >> reduce coupling, that's the point.

    > It does extra work,

Minimal, no IO occurs when building the store, everything is delayed up
to the first option access.

    > that all but the first config fetch will be immediately
    > discarded. As this touches disk and so on, it's not entirely
    > trivial, constructing GlobalStore times at 212 usec here.

Yeah, and how much for a GlobalConfig ?

    > Not very important though, and...

[Vincent Ladeuil (vila)]

sent to pqm by email

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2012-08-01 09:27:40 +0000
+++ bzrlib/config.py	2012-08-23 14:01:19 +0000
@@ -3288,6 +3288,7 @@
         # anyway.
         return 'In-Process Store, no URL'
 
+
 class TransportIniFileStore(IniFileStore):
     """IniFileStore that loads files from a transport.
 
@@ -3383,6 +3384,10 @@
 # on the relevant parts of the API that needs testing -- vila 20110503 (based
 # on a poolie's remark)
 class GlobalStore(LockableIniFileStore):
+    """A config store for global options.
+
+    There is a single GlobalStore for a given process.
+    """
 
     def __init__(self, possible_transports=None):
         t = transport.get_transport_from_path(
@@ -3392,6 +3397,10 @@
 
 
 class LocationStore(LockableIniFileStore):
+    """A config store for global options.
+
+    There is a single GlobalStore for a given process.
+    """
 
     def __init__(self, possible_transports=None):
         t = transport.get_transport_from_path(
@@ -3401,6 +3410,10 @@
 
 
 class BranchStore(TransportIniFileStore):
+    """A config store for branch options.
+
+    There is a single BranchStore for a given branch.
+    """
 
     def __init__(self, branch):
         super(BranchStore, self).__init__(branch.control_transport,
@@ -3623,6 +3636,10 @@
         yield is_ref, chunk
         is_ref = not is_ref
 
+# FIXME: _shared_stores should be an attribute of a library state once a
+# library_state object is always available.
+_shared_stores = {}
+_shared_stores_at_exit_installed = False
 
 class Stack(object):
     """A stack of configurations where an option can be defined"""
@@ -3817,11 +3834,47 @@
         return "<config.%s(%s)>" % (self.__class__.__name__, id(self))
 
     def _get_overrides(self):
-        # Hack around library_state.initialize never called
+        # FIXME: Hack around library_state.initialize never called
         if bzrlib.global_state is not None:
             return bzrlib.global_state.cmdline_overrides.get_sections()
         return []
 
+    def get_shared_store(self, store, state=None):
+        """Get a known shared store.
+
+        Store urls uniquely identify them and are used to ensure a single copy
+        is shared across all users.
+
+        :param store: The store known to the caller.
+
+        :param state: The library state where the known stores are kept.
+
+        :returns: The store received if it's not a known one, an already known
+            otherwise.
+        """
+        if state is None:
+            state = bzrlib.global_state
+        if state is None:
+            global _shared_stores_at_exit_installed
+            stores = _shared_stores
+            def save_config_changes():
+                for k, store in stores.iteritems():
+                    store.save_changes()
+            if not _shared_stores_at_exit_installed:
+                # FIXME: Ugly hack waiting for library_state to always be
+                # available. -- vila 20120731
+                import atexit
+                atexit.register(save_config_changes)
+                _shared_stores_at_exit_installed = True
+        else:
+            stores = state.config_stores
+        url = store.external_url()
+        try:
+            return stores[url]
+        except KeyError:
+            stores[url] = store
+            return store
+
 
 class MemoryStack(Stack):
     """A configuration stack defined from a string.
@@ -3877,7 +3930,7 @@
         self.store.save()
 
 
-class GlobalStack(_CompatibleStack):
+class GlobalStack(Stack):
     """Global options only stack.
 
     The following sections are queried:
@@ -3891,14 +3944,14 @@
     """
 
     def __init__(self):
-        gstore = GlobalStore()
+        gstore = self.get_shared_store(GlobalStore())
         super(GlobalStack, self).__init__(
             [self._get_overrides,
              NameMatcher(gstore, 'DEFAULT').get_sections],
             gstore, mutable_section_id='DEFAULT')
 
 
-class LocationStack(_CompatibleStack):
+class LocationStack(Stack):
     """Per-location options falling back to global options stack.
 
 
@@ -3920,10 +3973,10 @@
         """Make a new stack for a location and global configuration.
 
         :param location: A URL prefix to """
-        lstore = LocationStore()
+        lstore = self.get_shared_store(LocationStore())
         if location.startswith('file://'):
             location = urlutils.local_path_from_url(location)
-        gstore = GlobalStore()
+        gstore = self.get_shared_store(GlobalStore())
         super(LocationStack, self).__init__(
             [self._get_overrides,
              LocationMatcher(lstore, location).get_sections,
@@ -3951,9 +4004,9 @@
     """
 
     def __init__(self, branch):
-        lstore = LocationStore()
+        lstore = self.get_shared_store(LocationStore())
         bstore = branch._get_config_store()
-        gstore = GlobalStore()
+        gstore = self.get_shared_store(GlobalStore())
         super(BranchStack, self).__init__(
             [self._get_overrides,
              LocationMatcher(lstore, branch.base).get_sections,
@@ -3981,7 +4034,7 @@
         # unlock saves all the changes.
 
 
-class RemoteControlStack(_CompatibleStack):
+class RemoteControlStack(Stack):
     """Remote control-only options stack."""
 
     # FIXME 2011-11-22 JRV This should probably be renamed to avoid confusion

=== modified file 'bzrlib/library_state.py'
--- bzrlib/library_state.py	2011-12-19 13:23:58 +0000
+++ bzrlib/library_state.py	2012-08-23 14:01:19 +0000
@@ -76,6 +76,8 @@
         # There is no overrides by default, they are set later when the command
         # arguments are parsed.
         self.cmdline_overrides = config.CommandLineStore()
+        # No config stores are cached to start with
+        self.config_stores = {} # By url
         self.started = False
 
     def __enter__(self):
@@ -106,6 +108,10 @@
         self.started = True
 
     def __exit__(self, exc_type, exc_val, exc_tb):
+        if exc_type is None:
+            # Save config changes
+            for k, store in self.config_stores.iteritems():
+                store.save_changes()
         self.cleanups.cleanup_now()
         trace._flush_stdout_stderr()
         trace._flush_trace()

=== modified file 'bzrlib/lockdir.py'
--- bzrlib/lockdir.py	2011-12-19 13:23:58 +0000
+++ bzrlib/lockdir.py	2012-08-23 14:01:19 +0000
@@ -858,6 +858,6 @@
     as it gives some clue who the user is.
     """
     try:
-        return config.GlobalConfig().username()
+        return config.GlobalStack().get('email')
     except errors.NoWhoami:
         return osutils.getuser_unicode()

=== modified file 'bzrlib/plugins/launchpad/test_register.py'
--- bzrlib/plugins/launchpad/test_register.py	2012-02-14 17:22:37 +0000
+++ bzrlib/plugins/launchpad/test_register.py	2012-08-23 14:01:19 +0000
@@ -331,6 +331,9 @@
         service = LaunchpadService()
         g_conf = config.GlobalStack()
         g_conf.set('email', 'Test User <[e-mail]>')
+        g_conf.store.save()
+        # FIXME: auth_path base dir exists only because bazaar.conf has just
+        # been saved, brittle... -- vila 20120731
         f = open(auth_path, 'wb')
         try:
             scheme, hostinfo = urlparse.urlsplit(service.service_url)[:2]
@@ -352,6 +355,7 @@
         self.assertIs(None, service.registrant_password)
         g_conf = config.GlobalStack()
         g_conf.set('email', 'Test User <[e-mail]>')
+        g_conf.store.save()
         stdout = tests.StringIOWrapper()
         stderr = tests.StringIOWrapper()
         ui.ui_factory = tests.TestUIFactory(stdin='userpass\n',

=== modified file 'bzrlib/remote.py'
--- bzrlib/remote.py	2012-07-19 18:28:25 +0000
+++ bzrlib/remote.py	2012-08-23 14:01:19 +0000
@@ -384,7 +384,7 @@
             self._real_store = _mod_config.ControlStore(self.bzrdir)
 
     def external_url(self):
-        return self.bzrdir.user_url
+        return urlutils.join(self.branch.user_url, 'control.conf')
 
     def _load_content(self):
         medium = self.bzrdir._client._medium
@@ -3251,7 +3251,7 @@
         self._real_store = None
 
     def external_url(self):
-        return self.branch.user_url
+        return urlutils.join(self.branch.user_url, 'branch.conf')
 
     def _load_content(self):
         path = self.branch._remote_path()

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2012-08-03 09:35:46 +0000
+++ bzrlib/tests/__init__.py	2012-08-23 14:01:19 +0000
@@ -1001,6 +1001,8 @@
     def setUp(self):
         super(TestCase, self).setUp()
 
+        # At this point we're still accessing the config files in $BZR_HOME (as
+        # set by the user running selftest).
         timeout = config.GlobalStack().get('selftest.timeout')
         if timeout:
             timeout_fixture = fixtures.TimeoutFixture(timeout)
@@ -2447,6 +2449,34 @@
         self.transport_readonly_server = None
         self.__vfs_server = None
 
+    def setUp(self):
+        super(TestCaseWithMemoryTransport, self).setUp()
+
+        def _add_disconnect_cleanup(transport):
+            """Schedule disconnection of given transport at test cleanup
+
+            This needs to happen for all connected transports or leaks occur.
+
+            Note reconnections may mean we call disconnect multiple times per
+            transport which is suboptimal but seems harmless.
+            """
+            self.addCleanup(transport.disconnect)
+
+        _mod_transport.Transport.hooks.install_named_hook('post_connect',
+            _add_disconnect_cleanup, None)
+
+        self._make_test_root()
+        self.addCleanup(os.chdir, os.getcwdu())
+        self.makeAndChdirToTestDir()
+        self.overrideEnvironmentForTesting()
+        self.__readonly_server = None
+        self.__server = None
+        self.reduceLockdirTimeout()
+        # Each test may use its own config files even if the local config files
+        # don't actually exist. They'll rightly fail if they try to create them
+        # though.
+        self.overrideAttr(config, '_shared_stores', {})
+
     def get_transport(self, relpath=None):
         """Return a writeable transport.
 
@@ -2733,30 +2763,6 @@
         self.overrideEnv('HOME', test_home_dir)
         self.overrideEnv('BZR_HOME', test_home_dir)
 
-    def setUp(self):
-        super(TestCaseWithMemoryTransport, self).setUp()
-
-        def _add_disconnect_cleanup(transport):
-            """Schedule disconnection of given transport at test cleanup
-
-            This needs to happen for all connected transports or leaks occur.
-
-            Note reconnections may mean we call disconnect multiple times per
-            transport which is suboptimal but seems harmless.
-            """
-            self.addCleanup(transport.disconnect)
- 
-        _mod_transport.Transport.hooks.install_named_hook('post_connect',
-            _add_disconnect_cleanup, None)
-
-        self._make_test_root()
-        self.addCleanup(os.chdir, os.getcwdu())
-        self.makeAndChdirToTestDir()
-        self.overrideEnvironmentForTesting()
-        self.__readonly_server = None
-        self.__server = None
-        self.reduceLockdirTimeout()
-
     def setup_smart_server_with_call_log(self):
         """Sets up a smart server as the transport server with a call log."""
         self.transport_server = test_server.SmartTCPServer_for_testing
@@ -2949,6 +2955,10 @@
     readwrite one must both define get_url() as resolving to os.getcwd().
     """
 
+    def setUp(self):
+        super(TestCaseWithTransport, self).setUp()
+        self.__vfs_server = None
+
     def get_vfs_only_server(self):
         """See TestCaseWithMemoryTransport.
 
@@ -3037,17 +3047,13 @@
         self.assertFalse(differences.has_changed(),
             "Trees %r and %r are different: %r" % (left, right, differences))
 
-    def setUp(self):
-        super(TestCaseWithTransport, self).setUp()
-        self.__vfs_server = None
-
     def disable_missing_extensions_warning(self):
         """Some tests expect a precise stderr content.
 
         There is no point in forcing them to duplicate the extension related
         warning.
         """
-        config.GlobalConfig().set_user_option('ignore_missing_extensions', True)
+        config.GlobalStack().set('ignore_missing_extensions', True)
 
 
 class ChrootedTestCase(TestCaseWithTransport):

=== modified file 'bzrlib/tests/blackbox/test_init.py'
--- bzrlib/tests/blackbox/test_init.py	2012-08-04 14:27:47 +0000
+++ bzrlib/tests/blackbox/test_init.py	2012-08-23 14:01:19 +0000
@@ -168,10 +168,12 @@
 
     def test_init_default_format_option(self):
         """bzr init should read default format from option default_format"""
-        conf = _mod_config.GlobalConfig.from_string('''
+        g_store = _mod_config.GlobalStore()
+        g_store._load_from_string('''
 [DEFAULT]
 default_format = 1.9
-''', save=True)
+''')
+        g_store.save()
         out, err = self.run_bzr_subprocess('init')
         self.assertContainsRe(out, '1.9')
 

=== modified file 'bzrlib/tests/blackbox/test_serve.py'
--- bzrlib/tests/blackbox/test_serve.py	2012-03-21 13:58:39 +0000
+++ bzrlib/tests/blackbox/test_serve.py	2012-08-23 14:01:19 +0000
@@ -274,6 +274,8 @@
     def test_bzr_serve_supports_configurable_timeout(self):
         gs = config.GlobalStack()
         gs.set('serve.client_timeout', 0.2)
+        # Save the config as the subprocess will use it
+        gs.store.save()
         process, url = self.start_server_port()
         self.build_tree_contents([('a_file', 'contents\n')])
         # We can connect and issue a request
@@ -281,9 +283,6 @@
         self.assertEqual('contents\n', t.get_bytes('a_file'))
         # However, if we just wait for more content from the server, it will
         # eventually disconnect us.
-        # TODO: Use something like signal.alarm() so that if the server doesn't
-        #       properly handle the timeout, we end up failing the test instead
-        #       of hanging forever.
         m = t.get_smart_medium()
         m.read_bytes(1)
         # Now, we wait for timeout to trigger

=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2012-07-28 15:19:25 +0000
+++ bzrlib/tests/test_config.py	2012-08-23 14:01:19 +0000
@@ -4347,6 +4347,15 @@
         self.assertSectionNames(['ALIASES'], self.bazaar_config, 'ALIASES')
 
 
+class TestSharedStores(tests.TestCaseInTempDir):
+
+    def test_bazaar_conf_shared(self):
+        g1 = config.GlobalStack()
+        g2 = config.GlobalStack()
+        # The two stacks share the same store
+        self.assertIs(g1.store, g2.store)
+
+
 class TestAuthenticationConfigFile(tests.TestCase):
     """Test the authentication.conf file matching"""
 

=== modified file 'bzrlib/tests/test_debug.py'
--- bzrlib/tests/test_debug.py	2011-11-16 17:19:13 +0000
+++ bzrlib/tests/test_debug.py	2012-08-23 14:01:19 +0000
@@ -26,11 +26,14 @@
 
 class TestDebugFlags(tests.TestCaseInTempDir):
 
-    def test_set_debug_flags_from_config(self):
-        # test both combinations because configobject automatically splits up
-        # comma-separated lists
+    def test_set_no_debug_flags_from_config(self):
+        self.assertDebugFlags([], '')
+
+    def test_set_single_debug_flags_from_config(self):
+        self.assertDebugFlags(['hpss'], 'debug_flags = hpss\n')
+
+    def test_set_multiple_debug_flags_from_config(self):
         self.assertDebugFlags(['hpss', 'error'], 'debug_flags = hpss, error\n')
-        self.assertDebugFlags(['hpss'], 'debug_flags = hpss\n')
 
     def assertDebugFlags(self, expected_flags, conf_bytes):
         conf = config.GlobalStack()

=== modified file 'bzrlib/tests/test_lockdir.py'
--- bzrlib/tests/test_lockdir.py	2012-01-27 22:09:19 +0000
+++ bzrlib/tests/test_lockdir.py	2012-08-23 14:01:19 +0000
@@ -734,7 +734,7 @@
             lambda: 'aproperhostname')
         # This is off by default at present; see the discussion in the bug.
         # If you change the default, don't forget to update the docs.
-        config.GlobalConfig().set_user_option('locks.steal_dead', True)
+        config.GlobalStack().set('locks.steal_dead', True)
         # Create a lock pretending to come from a different nonexistent
         # process on the same machine.
         l1 = LockDir(self.get_transport(), 'a',

=== modified file 'bzrlib/tests/test_ui.py'
--- bzrlib/tests/test_ui.py	2012-04-30 08:59:57 +0000
+++ bzrlib/tests/test_ui.py	2012-08-23 14:01:19 +0000
@@ -56,8 +56,7 @@
 
     def test_output_encoding_configuration(self):
         enc = fixtures.generate_unicode_encodings().next()
-        config.GlobalConfig().set_user_option('output_encoding',
-            enc)
+        config.GlobalStack().set('output_encoding', enc)
         ui = tests.TestUIFactory(stdin=None,
             stdout=tests.StringIOWrapper(),
             stderr=tests.StringIOWrapper())

=== modified file 'doc/en/release-notes/bzr-2.6.txt'
--- doc/en/release-notes/bzr-2.6.txt	2012-07-28 20:20:38 +0000
+++ doc/en/release-notes/bzr-2.6.txt	2012-08-23 14:01:19 +0000
@@ -28,13 +28,17 @@
 Improvements
 ************
 
-``bzr lp-find-proposal`` now only cares about the revision-id that is
-specified, not the branch you use.  This was enabled by a new API call in
-Launchpad's web service.  (Aaron Bentley)
+* ``bzr lp-find-proposal`` now only cares about the revision-id that is
+  specified, not the branch you use.  This was enabled by a new API call in
+  Launchpad's web service.  (Aaron Bentley)
 
 * Implement authentication.conf password obfuscation, the password_encoding
   option can now be set to base64. (Florian Dorn)
 
+* Local configurations files (i.e. accessed on the local file system like
+  ``bazaar.conf`` and ``locations.conf``) are now shared, reducing the
+  number of IOs when querying a configuation option. (Vincent Ladeuil, #832042)
+
 Bug Fixes
 *********