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

[Vincent Ladeuil]

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

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.

[lp:~vila/bzr/671050-config-policy]

5535. By Vincent Ladeuil on 2010-11-12

Merge bzr.dev into 671050-config-policy

[John A Meinel (jameinel)]

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?

[Vincent Ladeuil (vila)]

>>>>> 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) ?

[Martin Pool (mbp)]

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]

5536. By Vincent Ladeuil on 2010-11-17

Tweak comment as per poolie's suggestion.

[Vincent Ladeuil (vila)]

sent to pqm by email

[Vincent Ladeuil (vila)]

sent to pqm by email

[Vincent Ladeuil (vila)]

sent to pqm by email

[lp:~vila/bzr/671050-config-policy]

5537. By Vincent Ladeuil on 2010-11-18

Merge bzr.dev into 671050-config-policy

=== 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 @@
             for (oname, value, section, conf_id) in c._get_options():
                 if name == oname:
                     # Display only the first value and exit
-                    self.outf.write('%s\n' % (value))
+                    # FIXME: We need to use get_user_option to take policies
+                    # into account and we need to make sure the option exists
+                    # too (hence the two for loops), this needs a better API --
+                    # vila 20101117
+                    self.outf.write('%s\n' % c.get_user_option(name))
                     displayed = True
                     break
         if not displayed:
@@ -1877,6 +1881,7 @@
         # avoid the delay introduced by the lazy regexp.
         name._compile_and_collapse()
         cur_conf_id = None
+        cur_section = None
         for c in self._get_configs(directory, scope):
             for (oname, value, section, conf_id) in c._get_options():
                 if name.search(oname):
@@ -1884,6 +1889,13 @@
                         # Explain where the options are defined
                         self.outf.write('%s:\n' % (conf_id,))
                         cur_conf_id = conf_id
+                        cur_section = None
+                    if (section not in (None, 'DEFAULT')
+                        and cur_section != section):
+                        # Display the section if it's not the default (or only)
+                        # one.
+                        self.outf.write('  [%s]\n' % section)
+                        cur_section = section
                     self.outf.write('  %s = %s\n' % (oname, value))
 
     def _set_config_option(self, name, value, directory, scope):

=== 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 @@
         script.run_script(self, '''\
             $ bzr config -d tree
             locations:
+              [.../tree]
               hello = world
             branch:
               hello = you
@@ -102,6 +103,33 @@
               hello = world
             ''')
 
+class TestConfigDisplayWithPolicy(tests.TestCaseWithTransport):
+
+    def test_location_with_policy(self):
+        # LocationConfig is the only one dealing with policies so far.
+        self.make_branch_and_tree('tree')
+        config_text = """\
+[%(dir)s]
+url = dir
+url:policy = appendpath
+[%(dir)s/tree]
+url = tree
+""" % {'dir': self.test_dir}
+        # We don't use the config directly so we save it to disk
+        config.LocationConfig.from_string(config_text, 'tree', save=True)
+        # policies are displayed with their options since they are part of
+        # their definition, likewise the path is not appended, we are just
+        # presenting the relevant portions of the config files
+        script.run_script(self, '''\
+            $ bzr config -d tree --all url
+            locations:
+              [.../work/tree]
+              url = tree
+              [.../work]
+              url = dir
+              url:policy = appendpath
+            ''')
+
 
 class TestConfigActive(tests.TestCaseWithTransport):
 
@@ -162,6 +190,7 @@
             $ bzr config -d tree --scope locations hello=world
             $ bzr config -d tree --all hello
             locations:
+              [.../work/tree]
               hello = world
             ''')
 
@@ -197,6 +226,7 @@
             $ bzr config --scope bazaar --remove file
             $ bzr config -d tree --all file
             locations:
+              [.../work/tree]
               file = locations
             branch:
               file = branch
@@ -207,6 +237,7 @@
             $ bzr config -d tree --scope bazaar --remove file
             $ bzr config -d tree --all file
             locations:
+              [.../work/tree]
               file = locations
             branch:
               file = branch
@@ -243,6 +274,7 @@
             $ bzr config -d tree --scope branch --remove file
             $ bzr config -d tree --all file
             locations:
+              [.../work/tree]
               file = locations
             bazaar:
               file = bazaar

=== 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 @@
             'http://www.example.com', 'appendpath_option'),
             config.POLICY_APPENDPATH)
 
+    def test__get_options_with_policy(self):
+        self.get_branch_config('/dir/subdir',
+                               location_config="""\
+[/dir]
+other_url = /other-dir
+other_url:policy = appendpath
+[/dir/subdir]
+other_url = /other-subdir
+""")
+        self.assertEqual(
+            [(u'other_url', u'/other-subdir', u'/dir/subdir', 'locations'),
+             (u'other_url', u'/other-dir', u'/dir', 'locations'),
+             (u'other_url:policy', u'appendpath', u'/dir', 'locations')],
+            list(self.my_location_config._get_options()))
+
     def test_location_without_username(self):
         self.get_branch_config('http://www.example.com/ignoreparent')
         self.assertEqual(u'Erik B\u00e5gfors <[e-mail]>',
@@ -1157,15 +1172,18 @@
         self.assertEqual('bzrlib.tests.test_config.post_commit',
                          self.my_config.post_commit())
 
-    def get_branch_config(self, location, global_config=None):
+    def get_branch_config(self, location, global_config=None,
+                          location_config=None):
         my_branch = FakeBranch(location)
         if global_config is None:
             global_config = sample_config_text
+        if location_config is None:
+            location_config = sample_branches_text
 
         my_global_config = config.GlobalConfig.from_string(global_config,
                                                            save=True)
         my_location_config = config.LocationConfig.from_string(
-            sample_branches_text, my_branch.base, save=True)
+            location_config, my_branch.base, save=True)
         my_config = config.BranchConfig(my_branch)
         self.my_config = my_config
         self.my_location_config = my_config._get_location_config()

=== 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 @@
 * Better message if there is an error while setting ownership of
   ``.bazaar`` directory. (Parth Malwankar, #657553)
 
+* ``bzr config`` will now respect option policies when displaying the value
+  and display the definition sections when appropriate.
+  (Vincent Ladeuil, #671050)
+
 * Don't create commit message files in the current directory to avoid nasty
   interactions with emacs (which tries to establish the status of the file
   during the commit which breaks on windows). (Vincent Ladeuil, #673637)