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

>>>>> "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_PLUGIN_PATH:+site" bzr test

No (and last value wins is evil, what did you smoke ? :-).

Because BZR_PLUGIN_PATH='-site' is the idiom to get: 'core' and
in itself is a very good idiom since with it we can do:

   BZR_PLUGIN_PATH=-site ./bzr selftest

instead of the infamous:

   ./bzr selftest --no-plugins

I.e.:
env_paths = ['-site']
defaults = ['+core', '+site']

And you see why '-site' should:

1 - Be handled in two passes (a first path to identify the paths
    to remove, the second one to remove them, I changed that bit
    to make it clearer).

2 - Remove all occurrences of '[+-]site' in both env_paths and
    defaults

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

Well, there is a limit to what one can do in terms of tricking
our diff algorithm. May be you should just look at the resulting
branch.

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

    jam> BZR_PLUGIN_PATH='+site:+user'

    jam> Would then search site locations before user locations.

Yes, as exposed in the tests in test_fallback_policy and test_override_policy.

fallback policy is '+core:+site:+user', very paranoiac :) You can
only add new plugins, never override existing or installed ones.

In fact, an intermediate version of this patch did that, but then
I reconsider breaking the compatibility as '+user:+core:+site'
(test_adhoc_policy) is a very good default policy and I added the
rationale in comment in get_standard_plugins_path().

    jam> I would probably do something more like:

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

<snip/>

Quite but that didn't take the right assumptions into account. My
version was a bit awkward though, I've simplified it a bit.

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

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

Err, that's precisely what this patch does, all the test are
against get_standard_plugins_path().

    jam> But the basic idea is:

    jam> 1) If nothing is said about site, then it just gets appended where
    jam> appropriate.

Don't fly see above.

    jam> 2) If + or - site is given, then the default is unset.

Correct.

    jam> 3) If +site is given, then its position is reset to the current
    jam> location. So doing:

    jam> BZR_PLUGIN_PATH='+site:+user:+site'

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

This is as counter intuitive as it can get 8-/

The root cause is that we need a way to act upon the "hidden"
default paths.

A simpler solution (but incompatible) will have been to say:

    env_paths = os.environ.get('BZR_PLUGIN_PATH', '+user:+core:+site').split(os.pathsep)

and give full control to the user from there. I tried that, but
finally felt that it wasn't worth breaking the compatibility.

So the code is a bit more complex because of that[1] but I feel
it's a small price to pay to keep the compatibility.

I can reconsider that if we decide to break the compatibility
though.

I think the tests are pretty easy to play with (and if you look
at the branch history you will see that I tried various default
policies with little changes) so I encourage you or anyone
interested to play a bit with:

  ./bzr selftest -s bt.test_plugins.TestEnvPluginPath

it really helps understanding the various cases.

    jam> review: needs_fixing

I think we don't agree enough so I'll ask another review.

[1]: But I remove some cruft thanks to your remarks.