Merge lp:~bzr/bzr/412930-plugin-path into lp:~bzr/bzr/trunk-old

[Vincent Ladeuil]

[Vincent Ladeuil (vila)]

This patch add the capability to fully control BZR_PLUGIN_PATH by defining magic values
for user, core and site directories and a way to remove them from the search path.

It's compatible with the actual implementation.

This will allow the test/build farm to test:
- the core plugins (the test are currently run with --no-plugins),
- a chosen set of plugins

Thanks in advance to the reviewer :-)

[John A Meinel (jameinel)]

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

Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging lp:~bzr/bzr/412930-plugin-path into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
> This patch add the capability to fully control BZR_PLUGIN_PATH by defining magic values
> for user, core and site directories and a way to remove them from the search path.
>
> It's compatible with the actual implementation.
>
> This will allow the test/build farm to test:
> - the core plugins (the test are currently run with --no-plugins),
> - a chosen set of plugins
>
> Thanks in advance to the reviewer :-)
>

So does this break the existing BZR_PLUGIN_PATH behavior? At first I
thought it did, but it looks like the existing code says "if
BZR_PLUGIN_PATH is set, override default location". Which matches your
"+user" version.

+By default all directories specified in ``BZR_PLUGIN_PATH``
+replace 'user' in the but keep searching in 'core' then 'site'.

^- bad grammar here.

By default if BZR_PLUGIN_PATH is set, it replaces searching in 'user'.
However it will continue to search in 'core' and 'site' unless they are
explicitly removed.

...

+ # Handle removals first
+ def unset_ref(p):
+ if p.startswith('-'):
+ key = p[1:]
+ if key in refs: # Needed to handle multiple removals
+ refs[key] = None
+ p = None
+ return p
+
+ defaults = [p for p in defaults if unset_ref(p) is not None]
+ env_paths = [p for p in env_paths if unset_ref(p) is not None]

^- This doesn't look right to me.

defaults = [p for p in defaults if unset_ref(p) is not None]

Specifically:
 1) I don't think you would ever have '-site' in defaults, for one
    thing, it is a local variable.
 2) If you did have '-site' in the list, then it would see that it is
    present in refs, set p to None, and then the 'if unset_ref(p) is not
    None' would return True, and '-site' is then included in the final
    defaults list.

I'm not 100% sure that doing:

BZR_PLUGIN_PATH='-site:+site'

Should ignore the site path. I would actually say 'last value wins'. So
that you can just do:

BZR_PLUGIN_PATH="$BZR_PLUGIN_PATH:+site" bzr test

I'm not stuck on this, but I certainly find it hard to follow exactly
what is going on in your get_standard_plugins_path() function. (The diff
makes things worse, because it mixes statements a little bit.)

I believe you are also trying to allow for altering the order of the
switch. So doing:

BZR_PLUGIN_PATH='+site:+user'

Would then search site locations before user locations.

I would probably do something more like:

env_path = os.environ.get('BZR_PLUGIN_PATH', '+user').split(os.pathsep)

default_core = True
default_site = True
plugin_paths = []
for path in env_path:
  if path.startswith('-'):
    if path == '-site':
      default_site = False
      if '+site' in plugin_paths:
        plugin_paths.remove('+site')
    elif path == '-core':
    ...
 elif path.startswith('+'):
    if path == '+site':
      default_site = False
      if '+site' in plugin_paths:
        plugin_paths.remove('+site')
    elif path == '+core':
      ...

    plugin_paths.append(path)
 else:
    ...

review: Needs Fixing

[Alexander Belchenko (bialix)]

30 +* the size specific plugin directory if applicable (containing
31 + the site plugins).

the size specific plugin directory? What is it?

[John A Meinel (jameinel)]

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

Alexander Belchenko wrote:
> 30 +* the size specific plugin directory if applicable (containing
> 31 + the site plugins).
>
> the size specific plugin directory? What is it?

site-specific. It only really exists on Linux where they split out the
python path because of wanting a separate "/usr/lib64" versus "/usr/lib"
and other such tweaks for the different between 64-bit compiled code,
and pure python code.

At least, that is how I understand it.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkqNlz0ACgkQJdeBCYSNAAMxxQCeOJopPvaV0JgNHuf+5pRpyl6m
5y4An0jzQ0IjHY4ZVHIm/+370zinGdpX
=WfNs
-----END PGP SIGNATURE-----

[Vincent Ladeuil (vila)]

>>>>> "jam" == John A Meinel <email address hidden> writes:

    jam> Review: Needs Fixing
    jam> Vincent Ladeuil wrote:
    >> Vincent Ladeuil has proposed merging lp:~bzr/bzr/412930-plugin-path into lp:bzr.
    >>
    >> Requested reviews:
    >> bzr-core (bzr-core)
    >>
    >> This patch add the capability to fully control BZR_PLUGIN_PATH by defining magic values
    >> for user, core and site directories and a way to remove them from the search path.
    >>
    >> It's compatible with the actual implementation.
    >>
    >> This will allow the test/build farm to test:
    >> - the core plugins (the test are currently run with --no-plugins),
    >> - a chosen set of plugins
    >>
    >> Thanks in advance to the reviewer :-)
    >>

    jam> So does this break the existing BZR_PLUGIN_PATH behavior?

No. But it makes the code more complex.

    jam> At first I thought it did, but it looks like the
    jam> existing code says "if BZR_PLUGIN_PATH is set, override
    jam> default location". Which matches your "+user" version.

Yes. That's the complicated part, 'core' and 'site' cannot be
removed without trickery.

    jam> +By default all directories specified in ``BZR_PLUGIN_PATH``
    jam> +replace 'user' in the but keep searching in 'core' then 'site'.

    jam> ^- bad grammar here.

    jam> By default if BZR_PLUGIN_PATH is set, it replaces searching in 'user'.
    jam> However it will continue to search in 'core' and 'site' unless they are
    jam> explicitly removed.

Thanks, fixed.

    jam> ...

    jam> + # Handle removals first
    jam> + def unset_ref(p):
    jam> + if p.startswith('-'):
    jam> + key = p[1:]
    jam> + if key in refs: # Needed to handle multiple removals
    jam> + refs[key] = None
    jam> + p = None
    jam> + return p
    jam> +
    jam> + defaults = [p for p in defaults if unset_ref(p) is not None]
    jam> + env_paths = [p for p in env_paths if unset_ref(p) is not None]

    jam> ^- This doesn't look right to me.

Then forget about it I removed that part :)

    jam> defaults = [p for p in defaults if unset_ref(p) is not None]

    jam> Specifically:

    jam> 1) I don't think you would ever have '-site' in defaults, for one
    jam> thing, it is a local variable.

You won't have '-site', but you have '+site', and you want it to
get removed when '-site' is specified. That's even the only way
to ensure that s 'site' plugin is not involved.

Both https://bugs.edge.launchpad.net/bzr/+bug/412930 and
https://bugs.edge.launchpad.net/bzr/+bug/316192 needs that
feature.

    jam> 2) If you did have '-site' in the list, then it would
    jam> see that it is present in refs, set p to None, and then
    jam> the 'if unset_ref(p) is not None' would return True,

???

if p is None, 'unset_ref(p) is not None' is False

    jam> and '-site' is then included in the final defaults
    jam> list.

No.

    jam> I'm not 100% sure that doing:

    jam> BZR_PLUGIN_PATH='-site:+site'

    jam> Should ignore the site path. I would actually say 'last
    jam> value wins'. So that you can just do:

    jam> BZR_PLUGIN_PATH="$BZR_PLU...

[Vincent Ladeuil (vila)]

>>>>> "jam" == John A Meinel <email address hidden> writes:

    jam> Alexander Belchenko wrote:
    >> 30 +* the size specific plugin directory if applicable (containing
    >> 31 + the site plugins).
    >>
    >> the size specific plugin directory? What is it?

    jam> site-specific.

Right. The intent is to give access to plugins installed by
whoever administer the site, this directory contains plugins that
can be used by all users.

    jam> It only really exists on Linux

Wrong.

The get_python_lib() doc string in distutils says:

    """Return the directory containing the Python library (standard or
    site additions).

    If 'plat_specific' is true, return the directory containing
    platform-specific modules, i.e. any module from a non-pure-Python
    module distribution; otherwise, return the platform-shared library
    directory. If 'standard_lib' is true, return the directory
    containing standard Python library modules; otherwise, return the
    directory for site-specific modules.

    If 'prefix' is supplied, use it instead of sys.prefix or
    sys.exec_prefix -- i.e., ignore 'plat_specific'.
    """

On linux it can be 'dist-packages' or 'site-packages' in the
python hierarchy.

On mac it's 'Lib/site-packages' but again the installers may be
wrong here (I don't know them enough to be sure, they may well be
right).

On os.name == "nt" (windows right ?) it seems to be
'Lib/site-packages'.

So, as I understand it, it makes no sense for bzr.exe (but we may
want to say it's 'All Users/Application Data/Bazaar/Plugins') and
when running from sources (or from easy_install when it will
work), we may want to activate it (in addition to 'All Users/...')

In the mean time, I kept the compatibility with previous versions
and add the win32 check that I accidentally removed in my patch.

Feedback welcome even if I think that should be the subject of
another submission are there are certainly tweaks to be done in
the windows installers.

[Alexander Belchenko (bialix)]

Vincent Ladeuil пишет:
>>>>>> "jam" == John A Meinel <email address hidden> writes:
>
> jam> Alexander Belchenko wrote:
> >> 30 +* the size specific plugin directory if applicable (containing
> >> 31 + the site plugins).
> >>
> >> the size specific plugin directory? What is it?
>
> jam> site-specific.

I understand, it was light joke. But it was not very good joke. Move on.

> Right. The intent is to give access to plugins installed by
> whoever administer the site, this directory contains plugins that
> can be used by all users.
>
> jam> It only really exists on Linux
>
> Wrong.

Really?

> The get_python_lib() doc string in distutils says:
[...]
>
> On linux it can be 'dist-packages' or 'site-packages' in the
> python hierarchy.
>
> On mac it's 'Lib/site-packages' but again the installers may be
> wrong here (I don't know them enough to be sure, they may well be
> right).
>
> On os.name == "nt" (windows right ?) it seems to be
> 'Lib/site-packages'.

Yes, so there is no distinction between "site" and "core" for Windows?

Because if I run bzr from sources current bzrlib/plugin.py (as I know)
even don't try to load plugins from
C:\PythonXX\Lib\site-packages\bzrlib\plugins

And perhaps it's right? No? Why?

If yes, what should be the right thing if I run bzr 3.0 from sources and
have installed bzr 2.x in site-packages? Do you think they will be
compatible? Quick responses on IRC reveal that it's not.

> So, as I understand it, it makes no sense for bzr.exe

For bzr.exe there is only "core" plugins I think. They are installed as
$(INSTALL_DIR)\plugins\, e.g. C:\Program Files\Bazaar\plugins

> (but we may
> want to say it's 'All Users/Application Data/Bazaar/Plugins')

I'm recall vaguely it was even discussed. But honestly: I don't want
this. Actually the less places bzr will use to look for plugins the
faster its startup time will be. Because on Windows it makes huge
difference if you need to read 1 file of 10MB and 100 files of 100K. The
less disk IO the faster application.

> and
> when running from sources (or from easy_install when it will
> work), we may want to activate it (in addition to 'All Users/...')

I have no opinion on using bzr in easy_install way. I love bzr.exe and
think it's the best we have today.

> In the mean time, I kept the compatibility with previous versions
> and add the win32 check that I accidentally removed in my patch.

If you return this code back then I should not worried?

> Feedback welcome even if I think that should be the subject of
> another submission are there are certainly tweaks to be done in
> the windows installers.

I'm still trying to understand what question I should answer.
I hope said above will help you. Ask me more and preferably directly,
I'm not subscribed on merge proposal comments.

[Alexander Belchenko (bialix)]

My 5 kopecks: as hardcore windows user I don't really like idea of Vincent example:

BZR_PLUGIN_PATH=-site bzr selftest ...

Because on pure Windows I can't set env variable only for one command. But I can't imagine why I need the proposed feature so I'm actually indifferent on this. But as windows users I'd prefer global command-line options similar to --no-plugins. Maybe:

--plugins=-site

would work as alternate syntax. Or maybe it will be even more obscure.
I dunno.

=== modified file 'NEWS'
--- NEWS	2009-08-21 03:17:13 +0000
+++ NEWS	2009-08-21 09:35:17 +0000
@@ -81,6 +81,11 @@
 New Features
 ************
 
+* Give more control on BZR_PLUGIN_PATH by providing a way to refer to or
+  disable the user, site and core plugin directories.
+  (Vincent Ladeuil, #412930, #316192, #145612)
+
+
 Bug Fixes
 *********
 

=== modified file 'bzrlib/help_topics/en/configuration.txt'
--- bzrlib/help_topics/en/configuration.txt	2009-06-26 18:13:41 +0000
+++ bzrlib/help_topics/en/configuration.txt	2009-08-21 09:35:17 +0000
@@ -63,6 +63,60 @@
 ~~~~~~~~~~~~~~~
 
 The path to the plugins directory that Bazaar should use.
+If not set, Bazaar will search for plugins in:
+
+* the user specific plugin directory (containing the ``user`` plugins),
+
+* the bzrlib directory (containing the ``core`` plugins),
+
+* the site specific plugin directory if applicable (containing
+  the ``site`` plugins).
+
+If ``BZR_PLUGIN_PATH`` is set in any fashion, it will change the
+the way the plugin are searched. 
+
+As for the ``PATH`` variables, if multiple directories are
+specified in ``BZR_PLUGIN_PATH`` they should be separated by the
+platform specific appropriate character (':' on Unix/Linux/etc,
+';' on windows)
+
+By default if ``BZR_PLUGIN_PATH`` is set, it replaces searching
+in ``user``.  However it will continue to search in ``core`` and
+``site`` unless they are explicitly removed.
+
+If you need to change the order or remove one of these
+directories, you should use special values:
+
+* ``-user``, ``-core``, ``-site`` will remove the corresponding
+  path from the default values,
+
+* ``+user``, ``+core``, ``+site`` will add the corresponding path
+  before the remaining default values (and also remove it from
+  the default values).
+
+Note that the special values 'user', 'core' and 'site' should be
+used literally, they will be substituted by the corresponding,
+platform specific, values.
+
+Examples:
+^^^^^^^^^
+
+The examples below uses ':' as the separator, windows users
+should use ';'.
+
+Overriding the default user plugin directory:
+``BZR_PLUGIN_PATH='/path/to/my/other/plugins'``
+
+Disabling the site directory while retaining the user directory:
+``BZR_PLUGIN_PATH='-site:+user'``
+
+Disabling all plugins (better achieved with --no-plugins):
+``BZR_PLUGIN_PATH='-user:-core:-site'``
+
+Overriding the default site plugin directory:
+``BZR_PLUGIN_PATH='/path/to/my/site/plugins:-site':+user``
+
+
 
 BZRPATH
 ~~~~~~~

=== modified file 'bzrlib/plugin.py'
--- bzrlib/plugin.py	2009-03-24 01:53:42 +0000
+++ bzrlib/plugin.py	2009-08-21 09:35:17 +0000
@@ -52,12 +52,16 @@
 from bzrlib import plugins as _mod_plugins
 """)
 
-from bzrlib.symbol_versioning import deprecated_function
+from bzrlib.symbol_versioning import (
+    deprecated_function,
+    deprecated_in,
+    )
 
 
 DEFAULT_PLUGIN_PATH = None
 _loaded = False
 
+@deprecated_function(deprecated_in((2, 0, 0)))
 def get_default_plugin_path():
     """Get the DEFAULT_PLUGIN_PATH"""
     global DEFAULT_PLUGIN_PATH
@@ -91,13 +95,15 @@
     return path
 
 
-def get_standard_plugins_path():
-    """Determine a plugin path suitable for general use."""
-    path = os.environ.get('BZR_PLUGIN_PATH',
-                          get_default_plugin_path()).split(os.pathsep)
-    # Get rid of trailing slashes, since Python can't handle them when
-    # it tries to import modules.
-    path = map(_strip_trailing_sep, path)
+def _append_new_path(paths, new_path):
+    """Append a new path if it set and not already known."""
+    if new_path is not None and new_path not in paths:
+        paths.append(new_path)
+    return paths
+
+
+def get_core_plugin_path():
+    core_path = None
     bzr_exe = bool(getattr(sys, 'frozen', None))
     if bzr_exe:    # expand path for bzr.exe
         # We need to use relative path to system-wide plugin
@@ -110,25 +116,83 @@
         # then plugins directory is
         # C:\Program Files\Bazaar\plugins
         # so relative path is ../../../plugins
-        path.append(osutils.abspath(osutils.pathjoin(
-            osutils.dirname(__file__), '../../../plugins')))
-    if not bzr_exe:     # don't look inside library.zip
+        core_path = osutils.abspath(osutils.pathjoin(
+                osutils.dirname(__file__), '../../../plugins'))
+    else:     # don't look inside library.zip
         # search the plugin path before the bzrlib installed dir
-        path.append(os.path.dirname(_mod_plugins.__file__))
-    # search the arch independent path if we can determine that and
-    # the plugin is found nowhere else
-    if sys.platform != 'win32':
-        try:
-            from distutils.sysconfig import get_python_lib
-        except ImportError:
-            # If distutuils is not available, we just won't add that path
-            pass
-        else:
-            archless_path = osutils.pathjoin(get_python_lib(), 'bzrlib',
-                    'plugins')
-            if archless_path not in path:
-                path.append(archless_path)
-    return path
+        core_path = os.path.dirname(_mod_plugins.__file__)
+    return core_path
+
+
+def get_site_plugin_path():
+    """Returns the path for the site installed plugins."""
+    if sys.platform == 'win32':
+        # We don't have (yet) a good answer for windows since that is certainly
+        # related to the way we build the installers. -- vila20090821
+        return None
+    site_path = None
+    try:
+        from distutils.sysconfig import get_python_lib
+    except ImportError:
+        # If distutuils is not available, we just don't know where they are
+        pass
+    else:
+        site_path = osutils.pathjoin(get_python_lib(), 'bzrlib', 'plugins')
+    return site_path
+
+
+def get_user_plugin_path():
+    return osutils.pathjoin(config.config_dir(), 'plugins')
+
+
+def get_standard_plugins_path():
+    """Determine a plugin path suitable for general use."""
+    # Ad-Hoc default: core is not overriden by site but user can overrides both
+    # The rationale is that:
+    # - 'site' comes last, because these plugins should always be available and
+    #   are supposed to be in sync with the bzr installed on site.
+    # - 'core' comes before 'site' so that running bzr from sources or a user
+    #   installed version overrides the site version.
+    # - 'user' comes first, because... user is always right.
+    # - the above rules clearly defines which plugin version will be loaded if
+    #   several exist. Yet, it is sometimes desirable to disable some directory
+    #   so that a set of plugin is disabled as once. This can be done via
+    #   -site, -core, -user.
+
+    env_paths = os.environ.get('BZR_PLUGIN_PATH', '+user').split(os.pathsep)
+    defaults = ['+core', '+site']
+
+    # The predefined references
+    refs = dict(core=get_core_plugin_path(),
+                site=get_site_plugin_path(),
+                user=get_user_plugin_path())
+
+    # Unset paths that should be removed
+    for k,v in refs.iteritems():
+        removed = '-%s' % k
+        # defaults can never mention removing paths as that will make it
+        # impossible for the user to revoke these removals.
+        if removed in env_paths:
+            env_paths.remove(removed)
+            refs[k] = None
+
+    # Expand references
+    paths = []
+    for p in env_paths + defaults:
+        if p.startswith('+'):
+            # Resolve reference if they are known
+            try:
+                p = refs[p[1:]]
+            except KeyError:
+                # Leave them untouched otherwise, user may have paths starting
+                # with '+'...
+                pass
+        _append_new_path(paths, p)
+
+    # Get rid of trailing slashes, since Python can't handle them when
+    # it tries to import modules.
+    paths = map(_strip_trailing_sep, paths)
+    return paths
 
 
 def load_plugins(path=None):

=== modified file 'bzrlib/tests/test_plugins.py'
--- bzrlib/tests/test_plugins.py	2009-06-10 03:56:49 +0000
+++ bzrlib/tests/test_plugins.py	2009-08-21 09:35:17 +0000
@@ -26,7 +26,11 @@
 import sys
 import zipfile
 
-from bzrlib import plugin, tests
+from bzrlib import (
+    osutils,
+    plugin,
+    tests,
+    )
 import bzrlib.plugin
 import bzrlib.plugins
 import bzrlib.commands
@@ -454,41 +458,6 @@
                 delattr(bzrlib.plugins, 'myplug')
 
 
-class TestSetPluginsPath(TestCase):
-
-    def test_set_plugins_path(self):
-        """set_plugins_path should set the module __path__ correctly."""
-        old_path = bzrlib.plugins.__path__
-        try:
-            bzrlib.plugins.__path__ = []
-            expected_path = bzrlib.plugin.set_plugins_path()
-            self.assertEqual(expected_path, bzrlib.plugins.__path__)
-        finally:
-            bzrlib.plugins.__path__ = old_path
-
-    def test_set_plugins_path_with_trailing_slashes(self):
-        """set_plugins_path should set the module __path__ based on
-        BZR_PLUGIN_PATH after removing all trailing slashes."""
-        old_path = bzrlib.plugins.__path__
-        old_env = os.environ.get('BZR_PLUGIN_PATH')
-        try:
-            bzrlib.plugins.__path__ = []
-            os.environ['BZR_PLUGIN_PATH'] = "first\\//\\" + os.pathsep + \
-                "second/\\/\\/"
-            bzrlib.plugin.set_plugins_path()
-            # We expect our nominated paths to have all path-seps removed,
-            # and this is testing only that.
-            expected_path = ['first', 'second']
-            self.assertEqual(expected_path,
-                bzrlib.plugins.__path__[:len(expected_path)])
-        finally:
-            bzrlib.plugins.__path__ = old_path
-            if old_env is not None:
-                os.environ['BZR_PLUGIN_PATH'] = old_env
-            else:
-                del os.environ['BZR_PLUGIN_PATH']
-
-
 class TestHelpIndex(tests.TestCase):
     """Tests for the PluginsHelpIndex class."""
 
@@ -597,41 +566,42 @@
         self.assertEqual('foo_bar', topic.get_help_topic())
 
 
-def clear_plugins(test_case):
-    # Save the attributes that we're about to monkey-patch.
-    old_plugins_path = bzrlib.plugins.__path__
-    old_loaded = plugin._loaded
-    old_load_from_path = plugin.load_from_path
-    # Change bzrlib.plugin to think no plugins have been loaded yet.
-    bzrlib.plugins.__path__ = []
-    plugin._loaded = False
-    # Monkey-patch load_from_path to stop it from actually loading anything.
-    def load_from_path(dirs):
-        pass
-    plugin.load_from_path = load_from_path
-    def restore_plugins():
-        bzrlib.plugins.__path__ = old_plugins_path
-        plugin._loaded = old_loaded
-        plugin.load_from_path = old_load_from_path
-    test_case.addCleanup(restore_plugins)
-
-
-class TestPluginPaths(tests.TestCase):
+class TestLoadFromPath(tests.TestCaseInTempDir):
+
+    def setUp(self):
+        super(TestLoadFromPath, self).setUp()
+        # Save the attributes that we're about to monkey-patch.
+        old_plugins_path = bzrlib.plugins.__path__
+        old_loaded = plugin._loaded
+        old_load_from_path = plugin.load_from_path
+
+        def restore():
+            bzrlib.plugins.__path__ = old_plugins_path
+            plugin._loaded = old_loaded
+            plugin.load_from_path = old_load_from_path
+
+        self.addCleanup(restore)
+
+        # Change bzrlib.plugin to think no plugins have been loaded yet.
+        bzrlib.plugins.__path__ = []
+        plugin._loaded = False
+
+        # Monkey-patch load_from_path to stop it from actually loading anything.
+        def load_from_path(dirs):
+            pass
+        plugin.load_from_path = load_from_path
 
     def test_set_plugins_path_with_args(self):
-        clear_plugins(self)
         plugin.set_plugins_path(['a', 'b'])
         self.assertEqual(['a', 'b'], bzrlib.plugins.__path__)
 
     def test_set_plugins_path_defaults(self):
-        clear_plugins(self)
         plugin.set_plugins_path()
         self.assertEqual(plugin.get_standard_plugins_path(),
                          bzrlib.plugins.__path__)
 
     def test_get_standard_plugins_path(self):
         path = plugin.get_standard_plugins_path()
-        self.assertEqual(plugin.get_default_plugin_path(), path[0])
         for directory in path:
             self.assertNotContainsRe(directory, r'\\/$')
         try:
@@ -649,13 +619,11 @@
 
     def test_get_standard_plugins_path_env(self):
         os.environ['BZR_PLUGIN_PATH'] = 'foo/'
-        self.assertEqual('foo', plugin.get_standard_plugins_path()[0])
-
-
-class TestLoadPlugins(tests.TestCaseInTempDir):
+        path = plugin.get_standard_plugins_path()
+        for directory in path:
+            self.assertNotContainsRe(directory, r'\\/$')
 
     def test_load_plugins(self):
-        clear_plugins(self)
         plugin.load_plugins(['.'])
         self.assertEqual(bzrlib.plugins.__path__, ['.'])
         # subsequent loads are no-ops
@@ -663,7 +631,107 @@
         self.assertEqual(bzrlib.plugins.__path__, ['.'])
 
     def test_load_plugins_default(self):
-        clear_plugins(self)
         plugin.load_plugins()
         path = plugin.get_standard_plugins_path()
         self.assertEqual(path, bzrlib.plugins.__path__)
+
+
+class TestEnvPluginPath(tests.TestCaseInTempDir):
+
+    def setUp(self):
+        super(TestEnvPluginPath, self).setUp()
+        old_default = plugin.DEFAULT_PLUGIN_PATH
+
+        def restore():
+            plugin.DEFAULT_PLUGIN_PATH = old_default
+
+        self.addCleanup(restore)
+
+        plugin.DEFAULT_PLUGIN_PATH = None
+
+        self.user = plugin.get_user_plugin_path()
+        self.site = plugin.get_site_plugin_path()
+        self.core = plugin.get_core_plugin_path()
+
+    def _list2paths(self, *args):
+        paths = []
+        for p in args:
+            plugin._append_new_path(paths, p)
+        return paths
+
+    def _set_path(self, *args):
+        path = os.pathsep.join(self._list2paths(*args))
+        osutils.set_or_unset_env('BZR_PLUGIN_PATH', path)
+
+    def check_path(self, expected_dirs, setting_dirs):
+        if setting_dirs:
+            self._set_path(*setting_dirs)
+        actual = plugin.get_standard_plugins_path()
+        self.assertEquals(self._list2paths(*expected_dirs), actual)
+
+    def test_default(self):
+        self.check_path([self.user, self.core, self.site],
+                        None)
+
+    def test_adhoc_policy(self):
+        self.check_path([self.user, self.core, self.site],
+                        ['+user', '+core', '+site'])
+
+    def test_fallback_policy(self):
+        self.check_path([self.core, self.site, self.user],
+                        ['+core', '+site', '+user'])
+
+    def test_override_policy(self):
+        self.check_path([self.user, self.site, self.core],
+                        ['+user', '+site', '+core'])
+
+    def test_disable_user(self):
+        self.check_path([self.core, self.site], ['-user'])
+
+    def test_disable_user_twice(self):
+        # Ensures multiple removals don't left cruft
+        self.check_path([self.core, self.site], ['-user', '-user'])
+
+    def test_duplicates_are_removed(self):
+        self.check_path([self.user, self.core, self.site],
+                        ['+user', '+user'])
+        # And only the first reference is kept (since the later references will
+        # onnly produce <plugin> already loaded mutters)
+        self.check_path([self.user, self.core, self.site],
+                        ['+user', '+user', '+core',
+                         '+user', '+site', '+site',
+                         '+core'])
+
+    def test_disable_overrides_disable(self):
+        self.check_path([self.core, self.site], ['-user', '+user'])
+
+    def test_disable_core(self):
+        self.check_path([self.site], ['-core'])
+        self.check_path([self.user, self.site], ['+user', '-core'])
+
+    def test_disable_site(self):
+        self.check_path([self.core], ['-site'])
+        self.check_path([self.user, self.core], ['-site', '+user'])
+
+    def test_override_site(self):
+        self.check_path(['mysite', self.user, self.core],
+                        ['mysite', '-site', '+user'])
+        self.check_path(['mysite', self.core],
+                        ['mysite', '-site'])
+
+    def test_override_core(self):
+        self.check_path(['mycore', self.user, self.site],
+                        ['mycore', '-core', '+user', '+site'])
+        self.check_path(['mycore', self.site],
+                        ['mycore', '-core'])
+
+    def test_my_plugin_only(self):
+        self.check_path(['myplugin'], ['myplugin', '-user', '-core', '-site'])
+
+    def test_my_plugin_first(self):
+        self.check_path(['myplugin', self.core, self.site, self.user],
+                        ['myplugin', '+core', '+site', '+user'])
+
+    def test_bogus_references(self):
+        self.check_path(['+foo', '-bar', self.core, self.site],
+                        ['+foo', '-bar'])

=== modified file 'doc/en/user-guide/plugins.txt'
--- doc/en/user-guide/plugins.txt	2007-12-14 07:35:49 +0000
+++ doc/en/user-guide/plugins.txt	2009-08-21 09:35:17 +0000
@@ -56,23 +56,11 @@
 Alternative plugin locations
 ----------------------------
 
-If you have the necessary permissions, plugins can also be installed on
-a system-wide basis. Two locations are currently checked for plugins:
-
- 1. the system location - bzrlib/plugins
- 2. the personal location - $BZR_HOME/plugins.
-
-On a Linux installation, these locations are typically
-``/usr/lib/python2.4/site-packages/bzrlib/plugins/`` and
-``$HOME/.bazaar/plugins/``.
-On a Windows installation, the system location might be
-``C:\\Program Files\\Bazaar\\plugins``
-while the personal location might be
-``C:\Documents and Settings\<username>\Application Data\Bazaar\2.0\plugins``.
-
-One can additionally override the personal plugins location
-by setting the environment variable ``BZR_PLUGIN_PATH``
-to a directory that contains plugins.
+If you have the necessary permissions, plugins can also be installed on a
+system-wide basis.  One can additionally override the personal plugins
+location by setting the environment variable ``BZR_PLUGIN_PATH`` (see `User
+Reference <../user-reference/bzr_man.html#bzr-plugin-path>`_ for a detailed
+explanation).
 
 Listing the installed plugins
 -----------------------------

=== modified file 'doc/en/user-guide/writing_a_plugin.txt'
--- doc/en/user-guide/writing_a_plugin.txt	2008-10-15 19:34:48 +0000
+++ doc/en/user-guide/writing_a_plugin.txt	2009-08-21 09:35:17 +0000
@@ -32,11 +32,14 @@
 Plugin searching rules
 ----------------------
 
-Bzr will scan ``bzrlib/plugins`` and ``~/.bazaar/plugins`` for plugins
-by default.  You can override this with ``BZR_PLUGIN_PATH``.  Plugins
-may be either modules or packages.  If your plugin is a single file,
-you can structure it as a module.  If it has multiple files, or if you
-want to distribute it as a bzr branch, you should structure it as a
+Bzr will scan ``~/.bazaar/plugins``  and ``bzrlib/plugins`` for plugins
+by default.  You can override this with  ``BZR_PLUGIN_PATH``
+(see `User Reference <../user-reference/bzr_man.html#bzr-plugin-path>`_
+for details).
+
+Plugins may be either modules or packages.  If your plugin is a single
+file, you can structure it as a module.  If it has multiple files, or if
+you want to distribute it as a bzr branch, you should structure it as a
 package, i.e. a directory with an ``__init__.py`` file.
 
 More information