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

[James Henstridge]

On 19/09/06, Aaron Bentley <aaron.bentley at utoronto.ca> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> James Henstridge wrote:
> > On 14/09/06, Aaron Bentley <aaron.bentley at utoronto.ca> wrote:
> >> But if they want to set policy for their own use, they can subclass
> >> BranchConfig, as pqm-submit does.
> >
> >
> > As John mentioned, pqm-submit doesn't subclass BranchConfig anymore.
> > Furthermore, I don't think we should completely discount plugins
> > sharing configuration.
>
> [...]
>
> > If the caller is going to be making policy decisions about how the key
> > is looked up (recusive vs. non-recursive), then it would be a bug for
> > two bits of code to use different policies.  It would be nice to catch
> > these sorts of problems.  Upfront registration of the policy is one
> > way of doing this.
>
> Are you proposing that both plugins would register the policy?  Because
> otherwise, you'd have an undesirable dependency, IMHO.  And so the
> policy registration system would have to permit multiple registrations,
> but raise an exception if two conflicting policies were registered.
>
> Is that what you had in mind?

That's exactly what I was thinking of.  Also, by registering the
policy once it makes sure that the key is accessed in the same way
everywhere, which would be more robust than having plugins or bzrlib
code specify the policy each time they use a config value.


> The other way for plugins to collaborate on policy is for them to set
> and respect the recurse="True"/"False" flag in locations.conf
>
> This also allows the configuration system to be more flexible, so that
> per-location configurations are not recursive by default, but users can
> set a repository-wide value if they choose.

The problem I see with this is that it affects all configuration data
inside the section.  Consider the following locations.conf file:

    [/some/branch]
    email = My email

Suppose that "/some/branch" and "/some/branch/subdir" are Bazaar
branches, which are set to use the email specified in locations.conf.
Now I do a push in "/some/branch", causing the config to change to:

    [/some/branch]
    email = My email
    push_location = sftp://...
    recurse=False

Now the the email config doesn't affect "/some/branch/subdir".  This
is similar to the current shadowing problem we have, but in reverse
direction.

I suppose another method would be to encode the policy into the
section name.  Maybe something like this?

    [/some/branch]
    email = My email

    [/some/branch#no-recurse]
    push_location = sftp://...

Something similar could be done for the "append remaining path
components to config value" policy you mentioned earlier in this
thread.  An alternative to the above syntax might be "?recurse=false"
-- both couldn't be confused with URI path components.


> > I've attached an updated branch that exposes the choice of whether to
> > do a recursive lookup for the key or not in get_user_option().  I've
> > set the default for this argument to "True", which most closely
> > matches the current behaviour of using parent directory
> > configurations.
> >
> > I haven't looked at updating any code to pick the recurse=False
> > policy, but Branch.get_push_location() is one candidate.
>
> I think a better approach is to set the "recurse=False" flag when
> setting this value.  That way, a user can can configure push to be
> recursive for a particular repository.  By analogy with submit, it seems
> reaonable to make that behaviour possible, even if it is not the default
> for automatic values.
>
> > -            if len(section_names) < len(location_names):
> > -                try:
> > -                    if not self._get_parser()[section].as_bool('recurse'):
> > -                        continue
>
> It appears you have disabled the "recurse=" functionality.  I think this
> is a bad idea, because it is a useful feature, and because doing this
> will break existing configurations.

If we do the policy-setting on set_user_option(), then I agree that
the feature should stay.  However I think using an option inside the
section like this is a problem.


> > +        for (length, section, extra_path) in matches:
> > +            sections.append((section, extra_path))
> > +            # should we stop looking for parent configs here?
> > +            try:
> > +                if self._get_parser()[section].as_bool('ignore_parents'):
> > +                    break
>
> If recurse=False were set for all automatic values, do you think
> ignore_parents would still be useful?

It depends on whether anyone was relying on the current shadowing
behaviour or not.  Maybe it would be better to leave it out and see if
anyone asks for the feature.

James.