[REVIEW] Add basic --color support to bzr diff

[Russ Brown]

Aaron Bentley wrote:
> Russ Brown wrote:
>> I think I probably titled this email incorrectly and gave the wrong
>> impression. The primary goal of this endeavour is to add a useful
>> framework for coloured output to bzr in the generic sense, so it can be
>> used by all commands and plugins equally.
> 
> In that case, perhaps it would be worth explaining your vision for how
> this framework will work.
> 

In short:

 1. A common, consistent way for the user to enable/disable coloured
    output for any command (i.e. color=always/auto/never)
 2. A common library for colouring up terminal output that is cross
    platform (including win32), has capability detection, and is
    available to all commands and plugins to use
 3. A common, consistent way for users to customise the colours used for
    all commands.

Current state of this:

 1. --color/--no-color options in place now, but following our
    discussion this needs to be replaced by --color=always/never/auto
 2. A new 'color' package exists which provides returns an object that
    can be used to write coloured strings with modifiers for
    foreground, background and brightness. Currently this object is
    hard-coded to always return the AnsiFilter object. Win32 is planned
    to be implemented by way of a sister class of AnsiFilter which will
    be returned to the client code transparently when the platform and
    capability is checked. The rest of the code won't care what platform
    it is running on.
 3. Requires further thought/discussion. Probably needs to happen after
    a number of commands are already coloured up first so we know what
    is required.

>> The diff command was just a
>> starting point: I could have started with status, for example. My goal
>> is not simply to replace cdiff.
> 
> Maybe  it would be better to work on status.  We already have two ways
> of generating a colorized diff:
> 1. bzr cdiff
> 2. bzr diff --using colordiff
> 
> Adding another option is confusing to users, and when it doesn't have
> any apparent advantages, it's not something I'd recommend.
> 

Granted, but these methods won't be consistent with how the rest of the
commands end up working, (goal 1 above). However, until there is
precedent in the form of support in other commands, I see your point,
which adds to the suggestion of working on other commands first (it is
however worth pointing out that in the ML/IRC discussions I was advised
to start with diff).

>> I actually did use some of the code from cdiff, but much of it is very
>> much geared to colouring diff specifically, while this patch is focused
>> on adding colour support to bzr core that can be easily used from any
>> command.
> 
> I don't see what you mean about cdiff's code being especially
> diff-centric.  Another approach is to start by importing the cdiff
> command and then adapting the code to support other commands.  Evolution
> rather than revolution.  That would have negated all my concerns and
> gotten you further faster.
> 

The configuration of cdiff seems to be driven by colordiff's config
file. This makes sense for diff, but not necessarily for everything else
(goal 3 above). So in order to achieve goal 3, the configuration needs
to make sense to the user system-wide as well as taking into account the
colordiff configuration (which does make sense to use in some way). So
it requires further though/discussion, which is why in the ML and IRC
discussions I was advised to leave this part until later.

My other concern is that a lot of cdiff's code is for the style checker,
which I don't think belongs in bzr core, so moving cdiff to core would
involve a lot of unpicking of its existing functionality. The approach I
took was very unintrisuve on the existing code, and it works.

So for the next step, I could:

 1. Junk everything I've done, move cdiff into core and make it work
    with other commands
 2. Junk everything I've done, move cdiff into core and unpick the style
    checker stuff, and make it work with other commands
 3. Undo the diff part of what I've done and move onto other commands
 4. Just move onto other commands

1 and 2 depend on the general consensus as to whether code checking
belongs in core or not. My opinion is not, and once bzr itself can
output colored diffs natively I had the idea that cdiff could become
'checkdiff' which just focuses on those checks. From there it could
evolve beyond the checks that are already in place into a fully
configurable checker, but that's another topic entirely.

If you take option 2, what you basically end up with is what I already
have in place, plus configuration that is colordiff-centric, and
detection that only deals with ANSI terminals that has a comment above
specifying that a rewrite is needed anyway.

I don't see the point to doing 3, since diff will need to be added at
some point anyway, so might as well leave it in.

I think I should be doing 4, and dipping into cdiff when I need to start
involving colordiff configuration and taking hints from the platform
detection code and comments that are there.

It all depends on what everyone else thinks though.

> Aaron