Code review comment for lp:~ian-clatworthy/bzr/411413-plugin-disable

>>>>> "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 completely. That will forbid using
'user', 'core' and 'site' as plugin names, but I don't think it
will be a big problem as is.

        Vincent