Merge lp:~vila/bzr/671050-config-policy into lp:bzr

Proposed by Vincent Ladeuil
Status: Merged
Approved by: Vincent Ladeuil
Approved revision: 5534
Merged at revision: 5542
Proposed branch: lp:~vila/bzr/671050-config-policy
Merge into: lp:bzr
Diff against target: 178 lines (+69/-3)
4 files modified
bzrlib/config.py (+13/-1)
bzrlib/tests/blackbox/test_config.py (+32/-0)
bzrlib/tests/test_config.py (+20/-2)
doc/en/release-notes/bzr-2.3.txt (+4/-0)
To merge this branch: bzr merge lp:~vila/bzr/671050-config-policy
Reviewer Review Type Date Requested Status
Martin Pool Approve
Review via email: mp+40343@code.launchpad.net

Commit message

Takes config policies into account and display the section names when needed.

Description of the change

This fixes bug #671050 by taking the option policies into account when displaying the value.

I also add displaying the section name when the option definitions are found in locations.conf.

To post a comment you must log in.
lp:~vila/bzr/671050-config-policy updated
5535. By Vincent Ladeuil

Merge bzr.dev into 671050-config-policy

Revision history for this message
John A Meinel (jameinel) wrote :

This looks like it contains code that I've already reviewed. It has the [.../tree] sections, which seems unrelated.

Are we sure it isn't a second submission of the same code?

Revision history for this message
Vincent Ladeuil (vila) wrote :

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

    > This looks like it contains code that I've already reviewed.

Really ? I don't have any trace of such a review... Did you approve it ?
:-D

    > It has the [.../tree] sections, which seems unrelated.

It *is* related as mentioned in the news entry (and the commit message):
``bzr config`` will now respect option policies when displaying the
value and display the definition sections when appropriate.

While fixing this bug I realized I didn't have tests where the same
option was defined in several section in the same config file which
requires displaying the section name. The lines you're referring to are
a fallout.

    > Are we sure it isn't a second submission of the same code?

Are you sure you didn't read it in the commit ML instead ?

I remember a discussion though, so may be it was on IRC and you didn't
put your comments in the mp (can't find it though) ?

Revision history for this message
Martin Pool (mbp) wrote :

That looks reasonable. The comment you add:

+ # Display only the first value and exit (We need to use
+ # get_user_option to take policies into account and we need
+ # to make sure the option exists too :-/)
+ self.outf.write('%s\n' % c.get_user_option(name))

makes me wonder if it should have an XXX, especially the emoticon? Or is there just missing punctuation and could be a little clearer?

review: Approve
lp:~vila/bzr/671050-config-policy updated
5536. By Vincent Ladeuil

Tweak comment as per poolie's suggestion.

Revision history for this message
Vincent Ladeuil (vila) wrote :

sent to pqm by email

Revision history for this message
Vincent Ladeuil (vila) wrote :

sent to pqm by email

Revision history for this message
Vincent Ladeuil (vila) wrote :

sent to pqm by email

lp:~vila/bzr/671050-config-policy updated
5537. By Vincent Ladeuil

Merge bzr.dev into 671050-config-policy

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 2010-11-05 11:13:05 +0000
3+++ bzrlib/config.py 2010-11-18 16:59:10 +0000
4@@ -1865,7 +1865,11 @@
5 for (oname, value, section, conf_id) in c._get_options():
6 if name == oname:
7 # Display only the first value and exit
8- self.outf.write('%s\n' % (value))
9+ # FIXME: We need to use get_user_option to take policies
10+ # into account and we need to make sure the option exists
11+ # too (hence the two for loops), this needs a better API --
12+ # vila 20101117
13+ self.outf.write('%s\n' % c.get_user_option(name))
14 displayed = True
15 break
16 if not displayed:
17@@ -1877,6 +1881,7 @@
18 # avoid the delay introduced by the lazy regexp.
19 name._compile_and_collapse()
20 cur_conf_id = None
21+ cur_section = None
22 for c in self._get_configs(directory, scope):
23 for (oname, value, section, conf_id) in c._get_options():
24 if name.search(oname):
25@@ -1884,6 +1889,13 @@
26 # Explain where the options are defined
27 self.outf.write('%s:\n' % (conf_id,))
28 cur_conf_id = conf_id
29+ cur_section = None
30+ if (section not in (None, 'DEFAULT')
31+ and cur_section != section):
32+ # Display the section if it's not the default (or only)
33+ # one.
34+ self.outf.write(' [%s]\n' % section)
35+ cur_section = section
36 self.outf.write(' %s = %s\n' % (oname, value))
37
38 def _set_config_option(self, name, value, directory, scope):
39
40=== modified file 'bzrlib/tests/blackbox/test_config.py'
41--- bzrlib/tests/blackbox/test_config.py 2010-11-05 11:13:05 +0000
42+++ bzrlib/tests/blackbox/test_config.py 2010-11-18 16:59:10 +0000
43@@ -88,6 +88,7 @@
44 script.run_script(self, '''\
45 $ bzr config -d tree
46 locations:
47+ [.../tree]
48 hello = world
49 branch:
50 hello = you
51@@ -102,6 +103,33 @@
52 hello = world
53 ''')
54
55+class TestConfigDisplayWithPolicy(tests.TestCaseWithTransport):
56+
57+ def test_location_with_policy(self):
58+ # LocationConfig is the only one dealing with policies so far.
59+ self.make_branch_and_tree('tree')
60+ config_text = """\
61+[%(dir)s]
62+url = dir
63+url:policy = appendpath
64+[%(dir)s/tree]
65+url = tree
66+""" % {'dir': self.test_dir}
67+ # We don't use the config directly so we save it to disk
68+ config.LocationConfig.from_string(config_text, 'tree', save=True)
69+ # policies are displayed with their options since they are part of
70+ # their definition, likewise the path is not appended, we are just
71+ # presenting the relevant portions of the config files
72+ script.run_script(self, '''\
73+ $ bzr config -d tree --all url
74+ locations:
75+ [.../work/tree]
76+ url = tree
77+ [.../work]
78+ url = dir
79+ url:policy = appendpath
80+ ''')
81+
82
83 class TestConfigActive(tests.TestCaseWithTransport):
84
85@@ -162,6 +190,7 @@
86 $ bzr config -d tree --scope locations hello=world
87 $ bzr config -d tree --all hello
88 locations:
89+ [.../work/tree]
90 hello = world
91 ''')
92
93@@ -197,6 +226,7 @@
94 $ bzr config --scope bazaar --remove file
95 $ bzr config -d tree --all file
96 locations:
97+ [.../work/tree]
98 file = locations
99 branch:
100 file = branch
101@@ -207,6 +237,7 @@
102 $ bzr config -d tree --scope bazaar --remove file
103 $ bzr config -d tree --all file
104 locations:
105+ [.../work/tree]
106 file = locations
107 branch:
108 file = branch
109@@ -243,6 +274,7 @@
110 $ bzr config -d tree --scope branch --remove file
111 $ bzr config -d tree --all file
112 locations:
113+ [.../work/tree]
114 file = locations
115 bazaar:
116 file = bazaar
117
118=== modified file 'bzrlib/tests/test_config.py'
119--- bzrlib/tests/test_config.py 2010-10-29 16:53:22 +0000
120+++ bzrlib/tests/test_config.py 2010-11-18 16:59:10 +0000
121@@ -1016,6 +1016,21 @@
122 'http://www.example.com', 'appendpath_option'),
123 config.POLICY_APPENDPATH)
124
125+ def test__get_options_with_policy(self):
126+ self.get_branch_config('/dir/subdir',
127+ location_config="""\
128+[/dir]
129+other_url = /other-dir
130+other_url:policy = appendpath
131+[/dir/subdir]
132+other_url = /other-subdir
133+""")
134+ self.assertEqual(
135+ [(u'other_url', u'/other-subdir', u'/dir/subdir', 'locations'),
136+ (u'other_url', u'/other-dir', u'/dir', 'locations'),
137+ (u'other_url:policy', u'appendpath', u'/dir', 'locations')],
138+ list(self.my_location_config._get_options()))
139+
140 def test_location_without_username(self):
141 self.get_branch_config('http://www.example.com/ignoreparent')
142 self.assertEqual(u'Erik B\u00e5gfors <erik@bagfors.nu>',
143@@ -1157,15 +1172,18 @@
144 self.assertEqual('bzrlib.tests.test_config.post_commit',
145 self.my_config.post_commit())
146
147- def get_branch_config(self, location, global_config=None):
148+ def get_branch_config(self, location, global_config=None,
149+ location_config=None):
150 my_branch = FakeBranch(location)
151 if global_config is None:
152 global_config = sample_config_text
153+ if location_config is None:
154+ location_config = sample_branches_text
155
156 my_global_config = config.GlobalConfig.from_string(global_config,
157 save=True)
158 my_location_config = config.LocationConfig.from_string(
159- sample_branches_text, my_branch.base, save=True)
160+ location_config, my_branch.base, save=True)
161 my_config = config.BranchConfig(my_branch)
162 self.my_config = my_config
163 self.my_location_config = my_config._get_location_config()
164
165=== modified file 'doc/en/release-notes/bzr-2.3.txt'
166--- doc/en/release-notes/bzr-2.3.txt 2010-11-18 16:22:46 +0000
167+++ doc/en/release-notes/bzr-2.3.txt 2010-11-18 16:59:10 +0000
168@@ -48,6 +48,10 @@
169 * Better message if there is an error while setting ownership of
170 ``.bazaar`` directory. (Parth Malwankar, #657553)
171
172+* ``bzr config`` will now respect option policies when displaying the value
173+ and display the definition sections when appropriate.
174+ (Vincent Ladeuil, #671050)
175+
176 * Don't create commit message files in the current directory to avoid nasty
177 interactions with emacs (which tries to establish the status of the file
178 during the commit which breaks on windows). (Vincent Ladeuil, #673637)