[MERGE] Harmonize branch.conf with locations.conf and bazaar.conf

[John Arbash Meinel]

Aaron Bentley wrote:
> Hi all,
> 
> This patch brings branch.conf into the same family as locations.conf and
> bazaar.conf.  It can provide any option that locations.conf or
> bazaar.conf can provide, except for security-sensitive ones.
> 
> Similarly, locations.conf can now provide a branch nick.
> 
> The order of precedence is
> 1. locations.conf
> 2. branch.conf
> 3. bazaar.conf
> 
> locations.conf has the highest priority, because it is local data, so it
> can be used to override inappropriate branch.conf options.
> 
> Unfortunately, branch.conf may be more specific than locations.conf.  I
> think an ideal precedence might be
> 1. locations.conf exact match
> 2. branch.conf
> 3. locations.conf recusive match
> 4. locations.conf recursive match #2
> 5. locations.conf recursive match #3
> ...
> 10. bazaar.conf
> 
> But I won'be be implementing that today.
> 
> I've moved the fall-through behaviour from LocationConfig into BranchConfig.
> 
> By default, options are set in branch.conf, so that they are visible to
> all users of the branch, but it is easy to set them in locations.conf if
> desired.
> 
> Aaron

=== modified file bzrlib/branch.py
--- bzrlib/branch.py
+++ bzrlib/branch.py
@@ -157,13 +157,10 @@
         self.cache_root = cache_root

     def _get_nick(self):
-        cfg = self.tree_config()
-        return cfg.get_option(u"nickname",
default=self.base.split('/')[-2])
+        return bzrlib.config.BranchConfig(self).get_nickname()

     def _set_nick(self, nick):
-        cfg = self.tree_config()
-        cfg.set_option(nick, "nickname")
-        assert cfg.get_option("nickname") == nick
+        bzrlib.config.BranchConfig(self).set_user_option('nickname', nick)

     nick = property(_get_nick, _set_nick)

So, I've seen the pattern:

	config = bzrlib.config.BranchConfig(self)
and
	config = bzrlib.config.BranchConfig(branch)
an awful lot.
It seems like it would be better to refactor these calls into:

config = branch.get_config()


...


...

Interesting factory method.

+    def _get_best_value(self, option_name):
+        """This returns a user option from local, tree or global config.
+
+        They are tried in that order.  Use get_safe_value if trusted values
+        are necessary.
+        """
+        for source in self.option_sources:
+            value = getattr(source(), option_name)()
+            if value is not None:
+                return value
+        return None
+
+    def _get_safe_value(self, option_name):
+        """This variant of get_best_value never returns untrusted values.
+
+        It does not return values from the branch data, because the
branch may
+        not be controlled by the user.
+
+        We may wish to allow locations.conf to control whether branches are
+        trusted in the future.
+        """
+        for source in (self._get_location_config, self._get_global_config):
+            value = getattr(source(), option_name)()
+            if value is not None:
+                return value
+        return None
+

...


...
Is this the test where you are testing that if the signature rules are
set in 'branch.conf' it doesn't get used?

     def test_signatures_forced(self):
-        branch = FakeBranch()
-        my_config = config.BranchConfig(branch)
-        config_file = StringIO(sample_always_signatures)
-        (my_config._get_location_config().
-            _get_global_config()._get_parser(config_file))
+        my_config = self.get_branch_config(
+            global_config=sample_always_signatures)
+        self.assertEqual(config.CHECK_NEVER,
my_config.signature_checking())
+        self.assertEqual(config.SIGN_ALWAYS, my_config.signing_policy())
+        self.assertTrue(my_config.signature_needed())
+
+    def test_signatures_forced_branch(self):
+        my_config = self.get_branch_config(
+            global_config=sample_ignore_signatures,
+            branch_data_config=sample_always_signatures)
         self.assertEqual(config.CHECK_NEVER,
my_config.signature_checking())
         self.assertEqual(config.SIGN_ALWAYS, my_config.signing_policy())
         self.assertTrue(my_config.signature_needed())
@@ -650,30 +706,30 @@
         branch = FakeBranch()
         my_config = config.BranchConfig(branch)
         config_file = StringIO(sample_config_text.encode('utf-8'))
-        (my_config._get_location_config().
-            _get_global_config()._get_parser(config_file))
+        my_config._get_global_config()._get_parser(config_file)
         self.assertEqual('gnome-gpg', my_config.gpg_signing_command())


I didn't see a clear test that makes sure about bazaar.conf vs
branch.conf vs locations.conf, but in general things looked okay. The
tests just seem a little bit difficult to read.

John
=:->

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060621/e213a1d2/attachment.pgp