[MERGE] Fix #296620 by allowing plugins instead of requiring them.

[Aaron Bentley]

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

Vincent Ladeuil wrote:
>>>>>> "aaron" == Aaron Bentley <aaron at aaronbentley.com> writes:
> 
>     aaron> It's misleading to say "allowing plugins instead of requiring them."
>     aaron> Bzrtools is not required for the shelf UI to run.
> 
> Poor wording from me. I meant: allow plugins to change some bzrlib
> feature (in that case colordiffs) instead of bzr requiring a plugin to
> implement a feature.
> 
>     aaron> The bug, IMHO, is that --no-plugins does not prevent plugins
>     aaron> from loading.
> 
> Yeah right, but no point in trying to fix that IMNSHO.

I don't know why you would say that.  It's a single-line fix.

> 
>     aaron> Requiring bzrlib to never import from a plugin, even when
>     aaron> there's a fallback, makes bzrlib too fragile.
> 
> bzrlib never imported plugins before, I see little point in allowing
> it. Plugins are there to enrich bzrlib, bzrlib should never depends on
> them. If we really need a plugin for bzrlib purposes, we should make it
> part of bzrlib instead.

There's a big difference between depending on a plugin, which I would
agree bzrlib must not do, and being able to use one if available.

> And I'd say bzrlib never imported plugins because that just makes no
> sense, so it didn't happen (and will continue to not happen) because of
> that.

Shelf wants to colourize its diffs.  There's a well-known plugin that
can do this for it.  I'd say it makes a lot of sense.

> I'm not a english native speaker but for me, plugin is plug *in* from
> outside not plug-into-myself-from-inside.
> 
> And just think about the nightmare it will become to check and handle
> API compatibility of the plugin from bzrlib.... so please, let's focus
> on making it work well *from* the plugin instead of trying to allow
> bzrlib to use plugins which seems pointless to me.

Okay, so your patch also fails on those grounds.  Because with your
patch, we can no longer tell what diff_writer is.  Especially, we can't
tell whether it would output the diff in colour.  And that means that we
can't provide a --no-color flag in cmd_shelf, as users have requested.

There are other ways we could accomplish this.  A registry of
diff_writers is the most obvious approach that comes to mind.

>     aaron> Vincent Ladeuil wrote:
>     >> Not extra fanycness, but get the job done: restore test suite usability.
>     >> 
>     >> I don't really know how to proceed for the bzrtools related patch, so
>     >> I'll attach it to the bug report.
> 
>     aaron> Because the bug is also a bzr bug, my mail client filtered it
>     aaron> into bzr bugs and it got lost in the noise.
> 
> I think the bug is really in both bzr and bzrtools right now.

I don't contest this.  I was explaining why I hadn't reacted to the bug
report before now.

> I think
> the patch I proposed for bzr is enough to fix the problem there, but the
> bzrtools patch is needed to restore colordiff support when bzrtools is
> installed, but as noted in the comments, it may not be sufficient in
> bzrtools since some users want that color diff support to be optional
> (and since I don't use color diff nor shelf right now, I don't feel
> qualified to enhance that patch).


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

iEYEARECAAYFAkkaPkAACgkQ0F+nu1YWqI2/1QCeInfNQqIJHbWFu0vhda685M0u
CDoAnR3e3ISZYFn9zX/Plfk9aPtyPDQN
=EarD
-----END PGP SIGNATURE-----