Bazaar

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

  1. 671050-config-policy
  2. Merge into bzr.dev
Proposed by Vincent Ladeuil
Status: Merged
Approved by: Vincent Ladeuil
Approved revision: no longer in the source branch.
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.
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
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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'bzrlib/config.py'
--- bzrlib/config.py 2010-11-05 11:13:05 +0000
+++ bzrlib/config.py 2010-11-18 16:59:10 +0000
@@ -1865,7 +1865,11 @@
1865 for (oname, value, section, conf_id) in c._get_options():1865 for (oname, value, section, conf_id) in c._get_options():
1866 if name == oname:1866 if name == oname:
1867 # Display only the first value and exit1867 # Display only the first value and exit
1868 self.outf.write('%s\n' % (value))1868 # FIXME: We need to use get_user_option to take policies
1869 # into account and we need to make sure the option exists
1870 # too (hence the two for loops), this needs a better API --
1871 # vila 20101117
1872 self.outf.write('%s\n' % c.get_user_option(name))
1869 displayed = True1873 displayed = True
1870 break1874 break
1871 if not displayed:1875 if not displayed:
@@ -1877,6 +1881,7 @@
1877 # avoid the delay introduced by the lazy regexp.1881 # avoid the delay introduced by the lazy regexp.
1878 name._compile_and_collapse()1882 name._compile_and_collapse()
1879 cur_conf_id = None1883 cur_conf_id = None
1884 cur_section = None
1880 for c in self._get_configs(directory, scope):1885 for c in self._get_configs(directory, scope):
1881 for (oname, value, section, conf_id) in c._get_options():1886 for (oname, value, section, conf_id) in c._get_options():
1882 if name.search(oname):1887 if name.search(oname):
@@ -1884,6 +1889,13 @@
1884 # Explain where the options are defined1889 # Explain where the options are defined
1885 self.outf.write('%s:\n' % (conf_id,))1890 self.outf.write('%s:\n' % (conf_id,))
1886 cur_conf_id = conf_id1891 cur_conf_id = conf_id
1892 cur_section = None
1893 if (section not in (None, 'DEFAULT')
1894 and cur_section != section):
1895 # Display the section if it's not the default (or only)
1896 # one.
1897 self.outf.write(' [%s]\n' % section)
1898 cur_section = section
1887 self.outf.write(' %s = %s\n' % (oname, value))1899 self.outf.write(' %s = %s\n' % (oname, value))
18881900
1889 def _set_config_option(self, name, value, directory, scope):1901 def _set_config_option(self, name, value, directory, scope):
18901902
=== modified file 'bzrlib/tests/blackbox/test_config.py'
--- bzrlib/tests/blackbox/test_config.py 2010-11-05 11:13:05 +0000
+++ bzrlib/tests/blackbox/test_config.py 2010-11-18 16:59:10 +0000
@@ -88,6 +88,7 @@
88 script.run_script(self, '''\88 script.run_script(self, '''\
89 $ bzr config -d tree89 $ bzr config -d tree
90 locations:90 locations:
91 [.../tree]
91 hello = world92 hello = world
92 branch:93 branch:
93 hello = you94 hello = you
@@ -102,6 +103,33 @@
102 hello = world103 hello = world
103 ''')104 ''')
104105
106class TestConfigDisplayWithPolicy(tests.TestCaseWithTransport):
107
108 def test_location_with_policy(self):
109 # LocationConfig is the only one dealing with policies so far.
110 self.make_branch_and_tree('tree')
111 config_text = """\
112[%(dir)s]
113url = dir
114url:policy = appendpath
115[%(dir)s/tree]
116url = tree
117""" % {'dir': self.test_dir}
118 # We don't use the config directly so we save it to disk
119 config.LocationConfig.from_string(config_text, 'tree', save=True)
120 # policies are displayed with their options since they are part of
121 # their definition, likewise the path is not appended, we are just
122 # presenting the relevant portions of the config files
123 script.run_script(self, '''\
124 $ bzr config -d tree --all url
125 locations:
126 [.../work/tree]
127 url = tree
128 [.../work]
129 url = dir
130 url:policy = appendpath
131 ''')
132
105133
106class TestConfigActive(tests.TestCaseWithTransport):134class TestConfigActive(tests.TestCaseWithTransport):
107135
@@ -162,6 +190,7 @@
162 $ bzr config -d tree --scope locations hello=world190 $ bzr config -d tree --scope locations hello=world
163 $ bzr config -d tree --all hello191 $ bzr config -d tree --all hello
164 locations:192 locations:
193 [.../work/tree]
165 hello = world194 hello = world
166 ''')195 ''')
167196
@@ -197,6 +226,7 @@
197 $ bzr config --scope bazaar --remove file226 $ bzr config --scope bazaar --remove file
198 $ bzr config -d tree --all file227 $ bzr config -d tree --all file
199 locations:228 locations:
229 [.../work/tree]
200 file = locations230 file = locations
201 branch:231 branch:
202 file = branch232 file = branch
@@ -207,6 +237,7 @@
207 $ bzr config -d tree --scope bazaar --remove file237 $ bzr config -d tree --scope bazaar --remove file
208 $ bzr config -d tree --all file238 $ bzr config -d tree --all file
209 locations:239 locations:
240 [.../work/tree]
210 file = locations241 file = locations
211 branch:242 branch:
212 file = branch243 file = branch
@@ -243,6 +274,7 @@
243 $ bzr config -d tree --scope branch --remove file274 $ bzr config -d tree --scope branch --remove file
244 $ bzr config -d tree --all file275 $ bzr config -d tree --all file
245 locations:276 locations:
277 [.../work/tree]
246 file = locations278 file = locations
247 bazaar:279 bazaar:
248 file = bazaar280 file = bazaar
249281
=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py 2010-10-29 16:53:22 +0000
+++ bzrlib/tests/test_config.py 2010-11-18 16:59:10 +0000
@@ -1016,6 +1016,21 @@
1016 'http://www.example.com', 'appendpath_option'),1016 'http://www.example.com', 'appendpath_option'),
1017 config.POLICY_APPENDPATH)1017 config.POLICY_APPENDPATH)
10181018
1019 def test__get_options_with_policy(self):
1020 self.get_branch_config('/dir/subdir',
1021 location_config="""\
1022[/dir]
1023other_url = /other-dir
1024other_url:policy = appendpath
1025[/dir/subdir]
1026other_url = /other-subdir
1027""")
1028 self.assertEqual(
1029 [(u'other_url', u'/other-subdir', u'/dir/subdir', 'locations'),
1030 (u'other_url', u'/other-dir', u'/dir', 'locations'),
1031 (u'other_url:policy', u'appendpath', u'/dir', 'locations')],
1032 list(self.my_location_config._get_options()))
1033
1019 def test_location_without_username(self):1034 def test_location_without_username(self):
1020 self.get_branch_config('http://www.example.com/ignoreparent')1035 self.get_branch_config('http://www.example.com/ignoreparent')
1021 self.assertEqual(u'Erik B\u00e5gfors <erik@bagfors.nu>',1036 self.assertEqual(u'Erik B\u00e5gfors <erik@bagfors.nu>',
@@ -1157,15 +1172,18 @@
1157 self.assertEqual('bzrlib.tests.test_config.post_commit',1172 self.assertEqual('bzrlib.tests.test_config.post_commit',
1158 self.my_config.post_commit())1173 self.my_config.post_commit())
11591174
1160 def get_branch_config(self, location, global_config=None):1175 def get_branch_config(self, location, global_config=None,
1176 location_config=None):
1161 my_branch = FakeBranch(location)1177 my_branch = FakeBranch(location)
1162 if global_config is None:1178 if global_config is None:
1163 global_config = sample_config_text1179 global_config = sample_config_text
1180 if location_config is None:
1181 location_config = sample_branches_text
11641182
1165 my_global_config = config.GlobalConfig.from_string(global_config,1183 my_global_config = config.GlobalConfig.from_string(global_config,
1166 save=True)1184 save=True)
1167 my_location_config = config.LocationConfig.from_string(1185 my_location_config = config.LocationConfig.from_string(
1168 sample_branches_text, my_branch.base, save=True)1186 location_config, my_branch.base, save=True)
1169 my_config = config.BranchConfig(my_branch)1187 my_config = config.BranchConfig(my_branch)
1170 self.my_config = my_config1188 self.my_config = my_config
1171 self.my_location_config = my_config._get_location_config()1189 self.my_location_config = my_config._get_location_config()
11721190
=== modified file 'doc/en/release-notes/bzr-2.3.txt'
--- doc/en/release-notes/bzr-2.3.txt 2010-11-18 16:22:46 +0000
+++ doc/en/release-notes/bzr-2.3.txt 2010-11-18 16:59:10 +0000
@@ -48,6 +48,10 @@
48* Better message if there is an error while setting ownership of48* Better message if there is an error while setting ownership of
49 ``.bazaar`` directory. (Parth Malwankar, #657553)49 ``.bazaar`` directory. (Parth Malwankar, #657553)
5050
51* ``bzr config`` will now respect option policies when displaying the value
52 and display the definition sections when appropriate.
53 (Vincent Ladeuil, #671050)
54
51* Don't create commit message files in the current directory to avoid nasty55* Don't create commit message files in the current directory to avoid nasty
52 interactions with emacs (which tries to establish the status of the file56 interactions with emacs (which tries to establish the status of the file
53 during the commit which breaks on windows). (Vincent Ladeuil, #673637)57 during the commit which breaks on windows). (Vincent Ladeuil, #673637)