Code review comment for lp:~bzr/bzr/412930-plugin-path

-----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:
    plugin_paths.append(path)

if default_core:
  plugin_paths.append('+core')
if default_site:
  plugin_paths.append('+site')

# Now expand +core and +site and +user into the real values
def expand(p):
  if p.startswith('+'):
    ...
plugin_paths = [expand(p) for p in plugin_paths]

This is complex enough, that I would also like to see it in a simple
helper function, that probably has direct unit tests.

But the basic idea is:
  1) If nothing is said about site, then it just gets appended where
     appropriate.
  2) If + or - site is given, then the default is unset.
  3) If +site is given, then its position is reset to the current
     location. So doing:

       BZR_PLUGIN_PATH='+site:+user:+site'

     Would end up loading site last, albeit before +core because that
     was not explicitly specified.

  review: needs_fixing

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

iEYEARECAAYFAkqNejsACgkQJdeBCYSNAAMbOgCdH1zqp2Gc8RNy9P8Iu0YFD6bi
7q0AoNV/40dceioRLBMN4a+eN0tCgMIV
=Xm08
-----END PGP SIGNATURE-----