[PATCH] cascading lookup support in LocationConfig (bug 33430)

[Aaron Bentley]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

James Henstridge wrote:
> Attached is a bundle to make LocationConfig._get_user_option() check
> multiple config file sections rather than the most specific section.
> This fixes the problem of config values getting shadowed (which can
> happen when bzr adds new sections to locations.conf).

Yay!

> There was one test that seemed designed to make use of the shadowing
> feature though

The shadowing was once considered a feature, but I don't think anyone
feels that way now.

> With cascading configs, an equivalent feature would be a flag to block
> checking of parent sections, which should be pretty easy to implement.
> Does anyone have an idea for a good key name for this?

exact?

> @@ -423,10 +418,18 @@
> +    def _get_user_option(self, option_name):
> +        """See Config._get_user_option."""
> +        for section in self._get_matching_sections():
> +            try:
> +                return self._get_parser().get_value(section, option_name)
> +            except KeyError:
> +                pass
> +        else:
> +            return None

I think I'd prefer for the cascading to happen in BranchConfig, rather
than LocationConfig, because BranchConfig sets policy.

An exact LocationConfig should be preferred over a TreeConfig match,
because the user always controls LocationConfig, but may not control
TreeConfig.

But a TreeConfig should be preferred over a recursive LocationConfig
match, because it is more specific.

One way of simulating this while doing the actual cascade in
LocationConfig would be to have a method that returned both the value
and the unmatched portion.  Say, get_user_option_cascade().  For an
exact match, it would return (value, ''), but for an inexact match, it
would return the (value, tail).

Supporting get_option_cascade() would give BranchConfig enough
information to make policy decisions, yet also allow LocationConfig to
function well.

It also allows you to specify policy for a location, and have
subdirectories on each side mirror one another.

For example:

[/home/abentley/bzr]
push = sftp://panoramicfeedback.com/var/www/opensource/bzr

If I query for "/home/abentley/bzr/nested-trees", I'd like to get back
("sftp://panoramicfeedback.com/var/www/opensource/bzr",
"/nested-trees").  That would let me produce the url
"sftp://panoramicfeedback.com/var/www/opensource/bzr/nested-trees",
which I think is a much better result.

Also, I notice that you've dropped _get_section(), even though it is
specified by Config.  Do you think it would make sense to change the
Config API so that it expected _get_matching_sections(), and used
_get_section() for backwards compatibility?  Your get_user_option could
then become the sole implementation.

If it weren't for the match-order issue, I'd approve your bundle as-is.
 We may yet decide that it's not desirable for TreeConfig matches to
come between recursive LocationConfig and exact LocationConfig.  It's
not something we've discussed a lot.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFFAXh60F+nu1YWqI0RAjacAJ9G7xidzz809sbo7oKQN5MG1wgSWACfZPmb
zlPPAeYc/wZ6edf5kJdmtVc=
=UfCH
-----END PGP SIGNATURE-----