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

[James Henstridge]

On 08/09/06, Aaron Bentley <aaron.bentley at utoronto.ca> wrote:
> -----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?

"exact" seems to be more appropriate for the existing "recurse=False"
mode (which I've preserved).  I'll try to explain what I mean with an
example.

Say we have two branches "/a/b" and "/a/b/c".   There are two ways we
might want to adjust how the cascade works:

1. settings that only apply to "/a/b".  This can be done by adding
recurse=False to the section for that branch:
    [/a/b]
    recurse=False
    key1=value1
    key2=value2

These settings won't apply to the "/a/b/c" branch.  This may be
clearer if the special key was exact=True.

2. prevent cascading to configuration data for "/a".  Lets call this
setting cascade=False:
    [/a/b]
    cascade=False
    key1=value1
    key2=value2

Here, branches /a/b and /a/b/c won't see any configuration data from
"/a" (or anything above "/a/b").

So (1) is a potentially useful feature but maybe not the best name.
(2) would be pretty easy to implement and would allow users to
essentially cut off parts of the filesystem so that they don't get
their parent's config.

>
> > @@ -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.

LocationConfig seemed the most appropriate place given how the
existing code worked, but I guess we can change that.


> 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.

I wonder if this indicates that the decision on whether to cascade
should be decided on a key-by-key basis?

The "push" config key definitely shouldn't cascade (unless it can
adjust the path like you've sketched out here), so the current
behaviour is a potential source of bugs.

Other keys we'd pretty much always want to cascade, such as "email".

Would it make sense to expose the two behaviours to the rest of the
code, or provide a way to register the policy for particular key
names.  What do you think?


> 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.

Sounds like a good idea.


> 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.

Thanks for the review.

James.