Merge lp:~vila/bzr/1331999-test-config-warnings into lp:bzr

[Vincent Ladeuil]

Bzr config should save the changes explicitly when needed

An atexit handler is used to make bzrlib.config easier for users and save changes in any config store.

This has an side-effect for some blackbox tests and leads to warnings to stderr. It has recently been causing issues with autopkgtest.

Reproduced with:

$ ./bzr selftest -s bt.test_config -s bb.test_config
bzr selftest: /home/vila/src/bzr/bugs/1331999-test-config-warnings/bzr
   /home/vila/src/bzr/bugs/1331999-test-config-warnings/bzrlib
   bzr-2.7.0dev1 python-2.7.6 Linux-3.13.0-29-generic-x86_64-with-Ubuntu-14.04-trusty

----------------------------------------------------------------------
Ran 794 tests in 4.406s

OK
Option file in section /tmp/testbzr-6xM4Mq.tmp/bzrlib.tests.blackbox.test_config.TestConfigRemoveOption.test_branch_config_default/work/tree of file:///tmp/testbzr-6xM4Mq.tmp/bzrlib.tests.blackbox.test_config.TestConfigRemoveOption.test_branch_config_default/home/.bazaar/locations.conf was changed from locations to <CREATED>. The <DELETED> value will be saved.
Option file in section DEFAULT of file:///tmp/testbzr-6xM4Mq.tmp/bzrlib.tests.blackbox.test_config.TestConfigRemoveOption.test_bazaar_config_outside_branch/home/.bazaar/bazaar.conf was changed from bazaar to <CREATED>. The <DELETED> value will be saved.
Option file in section /tmp/testbzr-6xM4Mq.tmp/bzrlib.tests.blackbox.test_config.TestConfigRemoveOption.test_locations_config_inside_branch/work/tree of file:///tmp/testbzr-6xM4Mq.tmp/bzrlib.tests.blackbox.test_config.TestConfigRemoveOption.test_locations_config_inside_branch/home/.bazaar/locations.conf was changed from locations to <CREATED>. The <DELETED> value will be saved.
Option file in section /tmp/testbzr-6xM4Mq.tmp/bzrlib.tests.blackbox.test_config.TestConfigRemoveOption.test_branch_config_forcing_branch/work/tree of file:///tmp/testbzr-6xM4Mq.tmp/bzrlib.tests.blackbox.test_config.TestConfigRemoveOption.test_branch_config_forcing_branch/home/.bazaar/locations.conf was changed from locations to <CREATED>. The <DELETED> value will be saved.
Option file in section DEFAULT of file:///tmp/testbzr-6xM4Mq.tmp/bzrlib.tests.blackbox.test_config.TestConfigRemoveOption.test_bazaar_config_inside_branch/home/.bazaar/bazaar.conf was changed from bazaar to <CREATED>. The <DELETED> value will be saved.
Option file in section /tmp/testbzr-6xM4Mq.tmp/bzrlib.tests.blackbox.test_config.TestConfigActive.test_active_in_branch/work/tree of file:///tmp/testbzr-6xM4Mq.tmp/bzrlib.tests.blackbox.test_config.TestConfigActive.test_active_in_branch/home/.bazaar/locations.conf was changed from locations to <CREATED>. The <DELETED> value will be saved.

With this fix:

$ ./bzr selftest -s bt.test_config -s bb.test_config
bzr selftest: /home/vila/src/bzr/bugs/1331999-test-config-warnings/bzr
   /home/vila/src/bzr/bugs/1331999-test-config-warnings/bzrlib
   bzr-2.7.0dev1 python-2.7.6 Linux-3.13.0-29-generic-x86_64-with-Ubuntu-14.04-trusty

----------------------------------------------------------------------
Ran 794 tests in 4.705s

OK

[Richard Wilbur (richard-wilbur)]

Thanks Vincent, for the elegant fix!
+2
P.S. 'sections' looks like it was unused. Is that correct?

review: Approve

[Vincent Ladeuil (vila)]

> Thanks Vincent, for the elegant fix!
> +2

Thanks ;)

> P.S. 'sections' looks like it was unused. Is that correct?

Yes.

[Vincent Ladeuil (vila)]

sent to pqm by email

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2014-02-14 10:29:49 +0000
+++ bzrlib/config.py	2014-06-19 09:58:21 +0000
@@ -3559,7 +3559,6 @@
         """
         location_parts = self.location.rstrip('/').split('/')
         store = self.store
-        sections = []
         # Later sections are more specific, they should be returned first
         for _, section in reversed(list(store.get_sections())):
             if section.id is None:
@@ -4246,6 +4245,8 @@
     def _set_config_option(self, name, value, directory, scope):
         conf = self._get_stack(directory, scope, write_access=True)
         conf.set(name, value)
+        # Explicitly save the changes
+        conf.store.save_changes()
 
     def _remove_config_option(self, name, directory, scope):
         if name is None:
@@ -4254,6 +4255,8 @@
         conf = self._get_stack(directory, scope, write_access=True)
         try:
             conf.remove(name)
+            # Explicitly save the changes
+            conf.store.save_changes()
         except KeyError:
             raise errors.NoSuchConfigOption(name)
 
 
=== modified file 'doc/en/release-notes/bzr-2.7.txt'
--- doc/en/release-notes/bzr-2.7.txt	2014-05-07 22:20:27 +0000
+++ doc/en/release-notes/bzr-2.7.txt	2014-06-19 09:58:21 +0000
@@ -72,6 +72,11 @@
    suite.  This can include new facilities for writing tests, fixes to 
    spurious test failures and changes to the way things should be tested.
 
+* Fix warnings on stderr caused by the atexit handler triggering for the
+  wrong reason: the 'config' command should explicitly save the changes when
+  modifying or removing an option and not rely on the atexit
+  handler. (Vincent Ladeuil, #1331999)
+
 * Handle (minor) incompatible change in python 2.7.6 leading to test
   failures. Only tests are affected. (Vincent Ladeuil, #1303879)