Merge lp:~vila/bzr/stack-remove-unknown into lp:bzr

Proposed by Vincent Ladeuil on 2012-09-17
Status: Merged
Approved by: Vincent Ladeuil on 2012-09-17
Approved revision: 6555
Merged at revision: 6561
Proposed branch: lp:~vila/bzr/stack-remove-unknown
Merge into: lp:bzr
Diff against target: 11 lines (+1/-1)
1 file modified
bzrlib/config.py (+1/-1)
To merge this branch: bzr merge lp:~vila/bzr/stack-remove-unknown
Reviewer Review Type Date Requested Status
Martin Packman (community) 2012-09-17 Approve on 2012-09-17
Review via email: mp+124622@code.launchpad.net

Commit message

Fix obscure and misleading warning when trying to delete an unknown config option.

Description of the change

There is a bug in trunk:

$ ./bzr selftest -s bt.test_config.TestStackRemove.test_remove_unknown -v
running 5 tests...
bzr selftest: /home/vila/src/bzr/bugs/stack-remove-unknown/bzr
   /home/vila/src/bzr/bugs/stack-remove-unknown/bzrlib
   bzr-2.6.0dev3 python-2.7.3 Linux-3.2.0-30-generic-x86_64-with-Ubuntu-12.04-precise

test_config.TestStackRemove.test_remove_unknown(branch_only) OK 86ms
..._config.TestStackRemove.test_remove_unknown(remote_control) OK 22ms
test_config.TestStackRemove.test_remove_unknown(bazaar) OK 1ms
test_config.TestStackRemove.test_remove_unknown(location) OK 1ms
test_config.TestStackRemove.test_remove_unknown(branch) OK 10ms
----------------------------------------------------------------------
Ran 5 tests in 0.130s

OK
Option I_do_not_exist in section . of file:///tmp/testbzr-datShw.tmp/bzrlib.tests.test_config.TestStackRemove.test_remove_unknown%28location%29/home/.bazaar/locations.conf was changed from None to <CREATED>. The <DELETED> value will be saved.
Option I_do_not_exist in section DEFAULT of file:///tmp/testbzr-datShw.tmp/bzrlib.tests.test_config.TestStackRemove.test_remove_unknown%28bazaar%29/home/.bazaar/bazaar.conf was changed from None to <CREATED>. The <DELETED> value will be saved.

this has been introduced in revno 6554.

The fix is to not try to record an option deletion in a section if it
doesn't exist (because the deletion will failed, there is no deletion to
record). Why I missed that when submitting revno 6554 is a bit unclear, the
spurious warnings don't make the tests fail but are enough to demonstrate
both the bug and this proposed fix.

To post a comment you must log in.
Martin Packman (gz) wrote :

Seems fine. Does this want a separate test case, rather than just being seen in an absence of kipple on the existing tests?

review: Approve
Vincent Ladeuil (vila) wrote :

> Seems fine.

Thanks.

> Does this want a separate test case, rather than just being seen
> in an absence of kipple on the existing tests?

I thought about it, but really, the warning caught that mistake and that should be enough.

Even the warning didn't really make sense and there is no unknown territory to explore with a new test.

Vincent Ladeuil (vila) wrote :

Fix obscure and misleading warning when trying to delete an unknown config option.

Vincent Ladeuil (vila) wrote :

Meh, that was the commit message and also an additional comment answering mgz's question ;)

Vincent Ladeuil (vila) wrote :

sent to pqm by email

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/config.py'
2--- bzrlib/config.py 2012-08-23 13:58:07 +0000
3+++ bzrlib/config.py 2012-09-17 08:52:23 +0000
4@@ -2944,7 +2944,7 @@
5 self.options[name] = value
6
7 def remove(self, name):
8- if name not in self.orig:
9+ if name not in self.orig and name in self.options:
10 self.orig[name] = self.get(name, None)
11 del self.options[name]
12