Merge lp:~ian-clatworthy/bzr/411413-plugin-disable into lp:bzr

[Ian Clatworthy]

[Ian Clatworthy (ian-clatworthy)]

This branch lets users disable selected plugins by adding settings to bazaar.conf. This can be helpful while tracking down problems and has been requested numerous times on the mailing list.

To generalise the feature, the setting for a given plugin can be an explicit (parent) directory to load a given plugin from. This lets developers/users run/test the default installation of a plugin say, rather than the first one found on the plugin search path.

Another test or two is required before this can land. I had a look at bzrib/tests/test_plugin.py and couldn't decide the best way to test this. TestLoadFromPath looks like the right class to extend but the way it monkey-patches the real code suggests otherwise? Input/advice/code welcome.

In terms of the big picture, my primary driver for this is better packaging. I'd like to see many more plugins bundled in the Windows and Mac installers but there's a risk in doing that. Being able to easily disable a suspect plugin reduces that risk a lot IMO.

[Martin Pool (mbp)]

That's a nice feature, thanks for adding it.

Eventually, if a plugin is disabled, it would be nice for this to be reflected in 'bzr plugins'.

It would be nice too to have -Ddisable_plugins=qbzr to set it at runtime but that's ok for now.

The documentation is unclear about whether you specify eg ~/.bazaar/plugins/qbzr or ~/.bazaar/plugins/.

I think setting it to empty to mean disabled is a bit strange but not necessarily wrong.

[Ian Clatworthy (ian-clatworthy)]

> Eventually, if a plugin is disabled, it would be nice for this to be reflected
> in 'bzr plugins'.

Agreed. That can easily come later.

> The documentation is unclear about whether you specify eg
> ~/.bazaar/plugins/qbzr or ~/.bazaar/plugins/.

It's the parent. I had hoped the example in the doc made that clear. Matching on the parent is the easiest to code and makes the most sense for plugins that aren't modules. I do wonder whether it's what users would expect though. OTOH, the 90% game here is disabling so maybe it's not worth worrying about too much?

[Robert Collins (lifeless)]

I suggest limiting the feature to just disabling plugins - overriding
plugins is a separately requested feature and does need to be able to
specify the exact path. Vincent and I have been talking about a
BZR_PLUGINS_PATH syntax for doing it.

-Rob

[Martin Pool (mbp)]

2010/1/4 Robert Collins <email address hidden>:
> I suggest limiting the feature to just disabling plugins - overriding
> plugins is a separately requested feature and does need to be able to
> specify the exact path. Vincent and I have been talking about a
> BZR_PLUGINS_PATH syntax for doing it.

I'd like to not add ad-hoc environment variables for things that could
reasonably be configured elsewhere. Let's put it into the config
system and then have a way to take config from the environment.

--
Martin <http://launchpad.net/~mbp/>

[Robert Collins (lifeless)]

On Mon, 2010-01-04 at 22:08 +1100, Martin Pool wrote:
> 2010/1/4 Robert Collins <email address hidden>:
> > I suggest limiting the feature to just disabling plugins - overriding
> > plugins is a separately requested feature and does need to be able to
> > specify the exact path. Vincent and I have been talking about a
> > BZR_PLUGINS_PATH syntax for doing it.
>
> I'd like to not add ad-hoc environment variables for things that could
> reasonably be configured elsewhere. Let's put it into the config
> system and then have a way to take config from the environment.

This isn't an adhoc environment variable - it already exists.

-Rob

[Martin Pool (mbp)]

2010/1/5 Robert Collins <email address hidden>:
> On Mon, 2010-01-04 at 22:08 +1100, Martin Pool wrote:
>> 2010/1/4 Robert Collins <email address hidden>:
>> > I suggest limiting the feature to just disabling plugins - overriding
>> > plugins is a separately requested feature and does need to be able to
>> > specify the exact path. Vincent and I have been talking about a
>> > BZR_PLUGINS_PATH syntax for doing it.
>>
>> I'd like to not add ad-hoc environment variables for things that could
>> reasonably be configured elsewhere.  Let's put it into the config
>> system and then have a way to take config from the environment.
>
> This isn't an adhoc environment variable - it already exists.

s/add variables/add features controlled by environment variables

--
Martin <http://launchpad.net/~mbp/>

[Robert Collins (lifeless)]

On Tue, 2010-01-05 at 01:06 +0000, Martin Pool wrote:

> >> I'd like to not add ad-hoc environment variables for things that could
> >> reasonably be configured elsewhere. Let's put it into the config
> >> system and then have a way to take config from the environment.
> >
> > This isn't an adhoc environment variable - it already exists.
>
> s/add variables/add features controlled by environment variables

I think this is an intereseting goal, but setting config options from
the environment could be pretty complex (in terms of UI); we already
support controlling plugin loading from the environment, so I think its
entirely appropriate *in this case* to control it from the existing
variable - that or we should be aiming to deprecate this (and other)
variables.

Put another way: saying that we shouldn't do this from the existing
variable makes making this feature usable from an environment variable
hostage to an unwritten, unspecified (and AFAIK) undiscussed feature.

All the uses I've heard of up until this bug for precise controlling of
plugin paths have been for testing in precommit hooks, an environment
where using the configuration to control it will be awkward at best
(cannot use the branch config, shouldn't be making persistent changes to
the global config)

-Rob

[Vincent Ladeuil (vila)]

>>>>> "robert" == Robert Collins <email address hidden> writes:

    robert> On Tue, 2010-01-05 at 01:06 +0000, Martin Pool wrote:
    >> >> I'd like to not add ad-hoc environment variables for things that could
    >> >> reasonably be configured elsewhere. Let's put it into the config
    >> >> system and then have a way to take config from the environment.
    >> >
    >> > This isn't an adhoc environment variable - it already exists.
    >>
    >> s/add variables/add features controlled by environment variables

    robert> I think this is an intereseting goal, but setting config options from
    robert> the environment could be pretty complex (in terms of UI);

Not really if we implement -O and then some more glue as Martin
suggested in bug #I-dont-remember.

    robert> we already support controlling plugin loading from
    robert> the environment, so I think its entirely appropriate
    robert> *in this case* to control it from the existing
    robert> variable - that or we should be aiming to deprecate
    robert> this (and other) variables.

But I agree there.

    robert> Put another way: saying that we shouldn't do this
    robert> from the existing variable makes making this feature
    robert> usable from an environment variable hostage to an
    robert> unwritten, unspecified (and AFAIK) undiscussed
    robert> feature.

And here too. I'm also uncomfortable having plugins controlled by
*both* an environment variable and configuration variables.

    robert> All the uses I've heard of up until this bug for
    robert> precise controlling of plugin paths have been for
    robert> testing in precommit hooks, an environment where
    robert> using the configuration to control it will be awkward
    robert> at best (cannot use the branch config, shouldn't be
    robert> making persistent changes to the global config)

Yup. plugins are loaded very early, I looked at config variables
once and avoid them precisely for this reason.

Also, setting a config variable is not really better than issuing
a 'mv plugin DISABLED/' to temporarily disable a plugin or
issuing 'ln -s whatever-path ~/.bazaar/plugins/plugin' when
installing a new plugin.

It's even worse than relying on the file system since you can't
have various link farms for various plugin configurations.

So, regarding this patch:

pros:

- allows both disabling a plugin and selecting a specific version
  when several are available in the path.

cons:

- still rely on BZR_PLUGIN_PATH,
- add complexity to plugins handling,
- doesn't address using a dev version of a plugin (since it
  requires specifying the parent directory you still need to use
  the plugin name as the directory name which is not ideal for
  maintaining several versions of the same plugin),
- can't be restricted to a single bzr run (as opposed to
  'BZR_PLUGIN_PATH=-site bzr whatever'

I'd much prefer we implement the feature discussed with Robert
'BZR_PLUGIN_PATH=<name>@path' to allow finer control of where a
plugin is supposed to be loaded from.

We still need to add support to disable a particular plugin, so
may be we can override '-' for that as in 'BZR_PLUGIN_PATH=-svn'
too disable the svn plugin comple...

[Martin Pool (mbp)]

I'm +0 on merging this; I think Vincent has good points but incremental steps are good too.

[Ian Clatworthy (ian-clatworthy)]

> Also, setting a config variable is not really better than issuing
> a 'mv plugin DISABLED/' to temporarily disable a plugin or
> issuing 'ln -s whatever-path ~/.bazaar/plugins/plugin' when
> installing a new plugin.

I disagree here. For Windows users in particular, the primary use case is disabling one or more plugins that came in the bundled installer. That's *much* nicer to do by editing bazaar.conf (either directly or via intelligence added to qplugins or qconfig) than asking users to hack zip files buried in system level directories. Or asking them to set magic environment variable for that matter.

> So, regarding this patch:

> cons:
>
> - still rely on BZR_PLUGIN_PATH,

I think it's fine to have that environment variable continue to be the path searched for plugins while also having configuration settings controlling disabling or otherwise. The question then becomes what are the sensible values for the configuration setting for a given plugin?

One option is 'on' vs 'off'.

Another is '' (disabled) vs 'use this one if multiple exist'.

I went with the latter because it was more powerful. I'm fine going with the first one (though it's a shame to dumb this feature down given your preferred alternative isn't likely to make the 2.1 IIUIC).

[Robert Collins (lifeless)]

On Mon, 2010-01-11 at 07:48 +0000, Ian Clatworthy wrote:
>
> Another is '' (disabled) vs 'use this one if multiple exist'.
>
> I went with the latter because it was more powerful. I'm fine going
> with the first one (though it's a shame to dumb this feature down
> given your preferred alternative isn't likely to make the 2.1 IIUIC).

By default we search two places for plugins:
 - the installed library
 - homedir

and homedir wins over system installed plugins (or at least it used
too :)).

I don't see any need for specific selection of the installed library
plugin over homedir plugins via config options - just delete the homedir
plugin - uninstall it.

So it seems to really be just disabling that is really useful for the
use cases you're putting forward here (though I consider it a bug if a
plugin needs to be 'not loaded' to be unintrusive: we shouldn't be
shipping problematic plugins by default.)

-Rob

[Vincent Ladeuil (vila)]

>>>>> "Ian" == Ian Clatworthy <email address hidden> writes:

    >> Also, setting a config variable is not really better than
    Ian> issuing
    >> a 'mv plugin DISABLED/' to temporarily disable a plugin or
    >> issuing 'ln -s whatever-path ~/.bazaar/plugins/plugin' when
    >> installing a new plugin.

    Ian> I disagree here. For Windows users in particular,

That's not in particular, windows is the only platform where we
have this problem.

    Ian> the primary use case is disabling one or more plugins
    Ian> that came in the bundled installer. That's *much* nicer
    Ian> to do by editing bazaar.conf (either directly or via
    Ian> intelligence added to qplugins or qconfig) than asking
    Ian> users to hack zip files buried in system level
    Ian> directories.

Users shouldn't have to hack a zip file. I expressed concerns
about that weeks ago and I considered (and still consider) it a
bug in the windows installer.

Moreover it also means people installing bzr from source on
windows can't easily use the plugins embedded in the installer.

And I'm back to the idea that you're doubling file systems
operations without addressing the root cause (since epoch,
disabling a plugin has been done by moving it out of the way (the
trash being one possible target of the move :).

    Ian> Or asking them to set magic environment variable for
    Ian> that matter.

PATH is as magic as BZR_PLUGIN_PATH, I don't think it's relevant
here.

    >> So, regarding this patch:

    >> cons:
    >>
    >> - still rely on BZR_PLUGIN_PATH,

    Ian> I think it's fine to have that environment variable continue to
    Ian> be the path searched for plugins while also having configuration
    Ian> settings controlling disabling or otherwise. The question then
    Ian> becomes what are the sensible values for the configuration
    Ian> setting for a given plugin?

You haven't even reply to the other conses....

    Ian> One option is 'on' vs 'off'.

    Ian> Another is '' (disabled) vs 'use this one if multiple exist'.

And you're already bringing back a feature which should clearly
be addressed by using BZR_PLUGIN_PATH.

    Ian> I went with the latter because it was more powerful. I'm
    Ian> fine going with the first one (though it's a shame to
    Ian> dumb this feature down given your preferred alternative
    Ian> isn't likely to make the 2.1 IIUIC).

Feel free to enhance BZR_PLUGIN_PATH handling :) Shame or not
there are two features that has been defined and that will
address the same needs, you can either work on them or update the
docs to explain why bzr use two different mechanisms and cross
reference the two in the docs.

I still think using two different mechanisms will confuse users
more than it will help them.

Using config variables here sets a precedent, landing that patch
means we will have to continue supporting it even if
BZR_PLUGIN_PATH handling is enhanced: more work to come.

[Vincent Ladeuil (vila)]

>>>>> "robert" == Robert Collins <email address hidden> writes:

    robert> On Mon, 2010-01-11 at 07:48 +0000, Ian Clatworthy wrote:
    >>
    >> Another is '' (disabled) vs 'use this one if multiple exist'.
    >>
    >> I went with the latter because it was more powerful. I'm fine going
    >> with the first one (though it's a shame to dumb this feature down
    >> given your preferred alternative isn't likely to make the 2.1 IIUIC).

    robert> By default we search two places for plugins:
    robert> - the installed library
    robert> - homedir

Add:
- the system-wide installed plugins

While the later has been more or less a hack when it was
introduced, the net result is that it is now *used* that way when
people use distributions where bzr *and* plugins are installed
system-wide.

    robert> and homedir wins over system installed plugins (or at
    robert> least it used too :)).

That's still the default: user/core/site (see
bzrlib.plugin.get_standard_plugin_path).

Windows is the only platform were things are blurry because
there is no concept of 'site' there.

I consider it a bug but couldn't get agreement from bialix nor
jam on how to address that in a simple way. The causes were 1)
windows doesn't have a package manager nor a standardized
directory layout, 2) the installer add plugins in bzrlib.zip
(mixing the 'core' and 'site' directories into a single one).

<snip/>

    robert> So it seems to really be just disabling that is
    robert> really useful for the use cases you're putting
    robert> forward here (though I consider it a bug if a plugin
    robert> needs to be 'not loaded' to be unintrusive: we
    robert> shouldn't be shipping problematic plugins by
    robert> default.)

Or we do, we should allow users to install fixed versions.

[Ian Clatworthy (ian-clatworthy)]

Vincent Ladeuil wrote:

> That's not in particular, windows is the only platform where we
> have this problem.

The issues I'm looking to solve impact all platforms IMO. Even on Linux,
a user may wish to disable a plugin that's installed system-wide and
they may not have permissions to touch the system paths where that
plugin is installed.

> PATH is as magic as BZR_PLUGIN_PATH, I don't think it's relevant
> here.

I think you and I are trying to solve very different Use Cases. My
drivers are:

1. More batteries should be included out-of-the-box in installers. Some
users will want or need to disable some of those batteries and they'll
most likely want to do it "forever". In some cases, plugins will be
broken and they'll want to turn them off. Or they'll hit and bug and
want to see whether having certain plugins installed fix the problem or
otherwise. Others may simply prefer a smaller footprint, e.g. less
entries in the list of commands when running qrun, less parts that might
break or introduce errors, etc.

2. There should be a feature in qplugins for enabling/disabling plugins.
That data ought to be permanently stored somewhere sensible like
bazaar.conf. An environment variable just doesn't cut it.

My focus is end-users, e.g. someone who clicks on a plugin in qplugins
and is given the option to disable it or to select a particular instance
of it found on the plugins path. Your focus seems to be test runners and
plugin developers where the needs are far more short term.

> You haven't even reply to the other conses....

Fair enough:

* "adds complexity" is a motherhood statement. Of course it does.
  So do 99% of other new features.

* "Developers can't give an explicit path". Agreed. I could
  fix that. OTOH, current mechanisms (like symlinks) works fine
  right now so it didn't feel worth it.

* "Restriction to a single run". I like your idea of environment
  variables for that case. You could just as nicely solve it by
  using an environment variable pointing to a custom bazaar.conf, yes?
  In that case, you could more easily test any custom settings in
  bazaar.conf, not just custom plugin paths.

> And you're already bringing back a feature which should clearly
> be addressed by using BZR_PLUGIN_PATH.

I'm sorry but that statement is simply not true. For your Use Cases,
environment variables sound fine. For mine, they are the wrong choice.

Having multiple mechanisms may indeed be the right answer here, given
the different problems we're trying to address. Regardless, having zero
mechanisms isn't the right number. :-)

Ian C.

[Christophe Simonis (OpenERP) (kangol)]

How handle the case of a plugin enable for just one repository ?

[Robert Collins (lifeless)]

On Mon, 2010-01-11 at 11:08 +0000, Christophe (OpenERP) wrote:
> How handle the case of a plugin enable for just one repository ?

There are two different things for plugins:
 - being loaded (enabled/disabled)
 - being active for a branch/repository

Plugins that you want active on one repository should look for a
configuration option about the repository (or branch).

Doing it this way avoids a bunch of issues related to security and
safety that turn up when repositories or branches can enable plugins.

-Rob

[Martin Pool (mbp)]

2010/1/11 Ian Clatworthy <email address hidden>:
> Vincent Ladeuil wrote:
>
>> That's not in particular, windows is the only platform where we
>> have this problem.
>
> The issues I'm looking to solve impact all platforms IMO. Even on Linux,
> a user may wish to disable a plugin that's installed system-wide and
> they may not have permissions to touch the system paths where that
> plugin is installed.

This is definitely true. I commonly want to do it myself; obviously I
know how but moving the directory is a bit ugly. There are also bugs
where we want to ask people "can you reproduce this without bzr-svn"
and that is more complicated than it should be.

>> PATH is as magic as BZR_PLUGIN_PATH, I don't think it's relevant
>> here.
>
> I think you and I are trying to solve very different Use Cases. My
> drivers are:
>
> 1. More batteries should be included out-of-the-box in installers. Some
> users will want or need to disable some of those batteries and they'll
> most likely want to do it "forever". In some cases, plugins will be
> broken and they'll want to turn them off. Or they'll hit and bug and
> want to see whether having certain plugins installed fix the problem or
> otherwise. Others may simply prefer a smaller footprint, e.g. less
> entries in the list of commands when running qrun, less parts that might
> break or introduce errors, etc.
>
> 2. There should be a feature in qplugins for enabling/disabling plugins.
> That data ought to be permanently stored somewhere sensible like
> bazaar.conf. An environment variable just doesn't cut it.
>
> My focus is end-users, e.g. someone who clicks on a plugin in qplugins
> and is given the option to disable it or to select a particular instance
> of it found on the plugins path. Your focus seems to be test runners and
> plugin developers where the needs are far more short term.
>
>> You haven't even reply to the other conses....
>
> Fair enough:
>
> * "adds complexity" is a motherhood statement. Of course it does.
>  So do 99% of other new features.
>
> * "Developers can't give an explicit path". Agreed. I could
>  fix that. OTOH, current mechanisms (like symlinks) works fine
>  right now so it didn't feel worth it.

I suspect if you point to the parent directory, people will commonly
get it wrong and be confused. Well that will probably happen whatever
you do, but I think perhaps more so this way.

> * "Restriction to a single run". I like your idea of environment
>  variables for that case. You could just as nicely solve it by
>  using an environment variable pointing to a custom bazaar.conf, yes?
>  In that case, you could more easily test any custom settings in
>  bazaar.conf, not just custom plugin paths.
>
>> And you're already bringing back a feature which should clearly
>> be addressed by using BZR_PLUGIN_PATH.
>
> I'm sorry but that statement is simply not true. For your Use Cases,
> environment variables sound fine. For mine, they are the wrong choice.
>
> Having multiple mechanisms may indeed be the right answer here, given
> the different problems we're trying to address. Regardless, having zero
> mechanisms isn't the right number. :-)

It's reasonable to have this be per...

[lp:~ian-clatworthy/bzr/411413-plugin-disable]

4931. By Ian Clatworthy on 2010-01-12

Simplify to just on vs off configuration

4932. By Ian Clatworthy on 2010-01-12

just check disabled vs otherwise

[Ian Clatworthy (ian-clatworthy)]

I've simplified this to just support disabling. Please re-review.

[Martin Pool (mbp)]

I think finding these based on the section name is a bit limiting, because it will make it harder to put them into other places, if we add a say -O option or something. What do you think of making it something like "plugin:gtk=off"?

[Ian Clatworthy (ian-clatworthy)]

Martin Pool wrote:
> I think finding these based on the section name is a bit limiting, because it will make it harder to put them into other places, if we add a say -O option or something. What do you think of making it something like "plugin:gtk=off"?

Sounds fine to me.

Ian C.

[John A Meinel (jameinel)]

161 + try:
162 + enabled = load_rules[f]
163 + if not enabled:
164 + trace.mutter('Plugin name %s disabled', f)
165 + continue
166 + except KeyError:
167 + pass

I think this could be simplified to:

if not load_rules.get(f, True):
 trace.mutter(...)
 continue

Also, I think this:
22 + def get_plugin_load_rules(self):
23 + """Return the plugins section."""
24 + if 'PLUGINS' in self._get_parser():
25 + values = self._get_parser()['PLUGINS']
26 + return dict([(k, not(v in ['off', 'disable', 'false']))
27 + for k, v in values.items()])
28 + else:
29 + return {}
30 +

1) Should probably use 'v.lower()'
2) Maybe should use 'ui.bool_from_string()'.

I don't really like allowing garbage settings to have 'default' results, rather than failing and telling the user that there is a problem with their configuration.

review: Needs Fixing

Unmerged revisions

4932. By Ian Clatworthy on 2010-01-12

just check disabled vs otherwise

4931. By Ian Clatworthy on 2010-01-12

Simplify to just on vs off configuration

4930. By Ian Clatworthy on 2010-01-02

add documentation

4929. By Ian Clatworthy on 2010-01-02

support plugin disabling and explicit locations

4928. By Ian Clatworthy on 2010-01-02

Add PLUGINS section to bazaar.conf

=== modified file 'NEWS'
--- NEWS	2010-01-11 13:15:01 +0000
+++ NEWS	2010-01-12 06:28:14 +0000
@@ -36,6 +36,9 @@
   (Ted Gould, Matthew Fuller, Vincent Ladeuil)
 
 
+* ``bazaar.conf`` now supports a PLUGINS section where plugins
+  can be disabled. (Ian Clatworthy, #411413)
+  
 Bug Fixes
 *********
 

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2009-12-17 10:01:25 +0000
+++ bzrlib/config.py	2010-01-12 06:28:14 +0000
@@ -514,6 +514,15 @@
         self._get_parser().write(f)
         f.close()
 
+    def get_plugin_load_rules(self):
+        """Return the plugins section."""
+        if 'PLUGINS' in self._get_parser():
+            values = self._get_parser()['PLUGINS']
+            return dict([(k, not(v in ['off', 'disable', 'false']))
+                for k, v in values.items()])
+        else:
+            return {}
+
 
 class LocationConfig(IniBasedConfig):
     """A configuration object that gives the policy for a location."""

=== modified file 'bzrlib/help_topics/en/configuration.txt'
--- bzrlib/help_topics/en/configuration.txt	2010-01-03 03:33:10 +0000
+++ bzrlib/help_topics/en/configuration.txt	2010-01-12 06:28:14 +0000
@@ -172,9 +172,10 @@
 
     [DEFAULT]
 
-The only valid section headers for bazaar.conf currently are [DEFAULT] and
-[ALIASES].  Section headers are case sensitive. The default section provides for
-setting variables which can be overridden with the branch config file.
+The only valid section headers for bazaar.conf currently are [DEFAULT],
+[ALIASES] and [PLUGINS].  Section headers are case sensitive. The DEFAULT
+section provides for setting variables which can be overridden with the
+branch config file.
 
 For ``locations.conf``, the variables from the section with the
 longest matching section header are used to the exclusion of other
@@ -228,8 +229,8 @@
 The main configuration file, bazaar.conf
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
-``bazaar.conf`` allows two sections: ``[DEFAULT]`` and ``[ALIASES]``.
-The default section contains the default
+``bazaar.conf`` supports ``[DEFAULT]``, ``[ALIASES]`` and ``[PLUGINS]``
+sections.  The default section contains the default
 configuration options for all branches. The default section can be
 overriden by providing a branch-specific section in ``locations.conf``.
 
@@ -241,6 +242,17 @@
     check_signatures  = check-available
     create_signatures = when-required
 
+The PLUGINS section lets you disable plugins found on the path.
+Keys are plugin names. A value of ``off``, ``disable`` or ``false``
+will disable a plugin.  Otherwise, the plugin will be enabled.
+For example::
+
+    [PLUGINS]
+    foo = off
+    bar = on
+
+To debug plugin load problems, check the trace messages in .bzr.log.
+
 
 The branch location configuration file, locations.conf
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

=== modified file 'bzrlib/plugin.py'
--- bzrlib/plugin.py	2009-09-04 15:36:48 +0000
+++ bzrlib/plugin.py	2010-01-12 06:28:14 +0000
@@ -195,7 +195,7 @@
     return paths
 
 
-def load_plugins(path=None):
+def load_plugins(path=None, load_rules=None):
     """Load bzrlib plugins.
 
     The environment variable BZR_PLUGIN_PATH is considered a delimited
@@ -203,12 +203,16 @@
     files (and whatever other extensions are used in the platform,
     such as *.pyd).
 
-    load_from_dirs() provides the underlying mechanism and is called with
+    load_from_path() provides the underlying mechanism and is called with
     the default directory list to provide the normal behaviour.
 
     :param path: The list of paths to search for plugins.  By default,
         path will be determined using get_standard_plugins_path.
         if path is [], no plugins can be loaded.
+    :param load_rules: If a plugin name is found in this map, only load
+        it if the corresponding value is True.
+        If this map is None, config.GlobalConfig().get_plugin_load_rules()
+        is used.
     """
     global _loaded
     if _loaded:
@@ -216,11 +220,14 @@
         return
     _loaded = True
 
+    if load_rules is None:
+        load_rules = config.GlobalConfig().get_plugin_load_rules()
+
     # scan for all plugins in the path.
-    load_from_path(set_plugins_path(path))
-
-
-def load_from_path(dirs):
+    load_from_path(set_plugins_path(path), load_rules)
+
+
+def load_from_path(dirs, load_rules=None):
     """Load bzrlib plugins found in each dir in dirs.
 
     Loading a plugin means importing it into the python interpreter.
@@ -243,7 +250,7 @@
             continue
         trace.mutter('looking for plugins in %s', d)
         if os.path.isdir(d):
-            load_from_dir(d)
+            load_from_dir(d, load_rules)
 
 
 # backwards compatability: load_from_dirs was the old name
@@ -251,7 +258,7 @@
 load_from_dirs = load_from_path
 
 
-def load_from_dir(d):
+def load_from_dir(d, load_rules=None):
     """Load the plugins in directory d.
 
     d must be in the plugins module path already.
@@ -262,6 +269,7 @@
     valid_suffixes = [suffix for suffix, mod_type, flags in imp.get_suffixes()
                               if flags in (imp.PY_SOURCE, imp.PY_COMPILED)]
     package_entries = ['__init__'+suffix for suffix in valid_suffixes]
+    load_rules = load_rules or {}
     plugin_names = set()
     for f in os.listdir(d):
         path = osutils.pathjoin(d, f)
@@ -287,6 +295,13 @@
         elif getattr(_mod_plugins, f, None):
             trace.mutter('Plugin name %s already loaded', f)
         else:
+            try:
+                enabled = load_rules[f]
+                if not enabled:
+                    trace.mutter('Plugin name %s disabled', f)
+                    continue
+            except KeyError:
+                pass
             # trace.mutter('add plugin name %s', f)
             plugin_names.add(f)
 

=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2009-12-22 23:09:50 +0000
+++ bzrlib/tests/test_config.py	2010-01-12 06:28:14 +0000
@@ -52,6 +52,17 @@
 ll=""" + sample_long_alias + "\n"
 
 
+sample_plugin_load_rules = """
+[PLUGINS]
+p1=on
+p2=off
+p3=enable
+p4=disable
+p5=true
+p6=false
+"""
+
+
 sample_always_signatures = """
 [DEFAULT]
 check_signatures=ignore
@@ -692,6 +703,24 @@
         change_editor = my_config.get_change_editor('old', 'new')
         self.assertIs(None, change_editor)
 
+    def test_get_plugin_load_rules(self):
+        config_file = StringIO(sample_plugin_load_rules)
+        my_config = config.GlobalConfig()
+        my_config._parser = my_config._get_parser(file=config_file)
+        load_rules = my_config.get_plugin_load_rules()
+        self.assertEqual(6, len(load_rules))
+        self.assertEqual(True, load_rules['p1'])
+        self.assertEqual(False, load_rules['p2'])
+        self.assertEqual(True, load_rules['p3'])
+        self.assertEqual(False, load_rules['p4'])
+        self.assertEqual(True, load_rules['p5'])
+        self.assertEqual(False, load_rules['p6'])
+
+    def test_get_no_plugin_locations(self):
+        my_config = self._get_sample_config()
+        locations = my_config.get_plugin_locations()
+        self.assertEqual(0, len(locations))
+
 
 class TestGlobalConfigSavingOptions(tests.TestCaseInTempDir):
 

=== modified file 'bzrlib/tests/test_plugins.py'
--- bzrlib/tests/test_plugins.py	2009-09-18 14:53:37 +0000
+++ bzrlib/tests/test_plugins.py	2010-01-12 06:28:14 +0000
@@ -592,7 +592,7 @@
         plugin._loaded = False
 
         # Monkey-patch load_from_path to stop it from actually loading anything.
-        def load_from_path(dirs):
+        def load_from_path(dirs, load_rules=None):
             pass
         plugin.load_from_path = load_from_path