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

[Russ Brown]

Hi,

I've reached a point where colour output is working, and before I
continue I'd like some feedback to make sure I am on the right track.

I have gone with adding the --color option to diff: I agree with the
discussion about always/auto/never, but I think that is more applicable
to the config file side of things than the command line options. I'm
open to more discussion on that though. So for now, --color will force
colour to be on, --no-color will force it off, with the default being off.

I have been as unintrusive as possible with the existing code.
cmd_diff.run now has a small amount of logic above the main call to
show_diff_trees which switches output streams. This allows the rest of
the code to remain the same without needing to worry about colour at
all. If --color is not specified this bit of logic is the only
additional overhead added by this patch.

If colour is enabled, sys.stdout is effectively hidden behind a layer of
two objects: a ColorDiff object and a colour filter object, which itself
writes to sys.stdout.

ColorDiff is what the rest of the diff code sees as the file it is
writing to. Right now it just implements write(): hopefully that is
enough. It seems to be in my testing, but I'm wondering if it maybe also
need to be able to handle writelines() as well in case diff makes use of
it in the future. The purpose of this class is to decide which colours
to use based on the text of the diff as it is being written by the diff
code. The colors it selects are currently hard-coded: this should
probably need to be configurable at some point, though I'm not sure it
is required for this first phase.
 
The next layer is the color filter object itself, which is defined in
the new color module.  When the call is made to obtain the filter
object, there will need to be platform and capability detection, but
currently that is not present. The purpose of this class is to abstract
the colouring of the terminal output, and is based on the simple
"TermColour" class that I added to the diffstat plugin a while back. It
just deals with generic colours, and is therefore going to be of use
when adding colour to other commands.

Oh, and hopefully I've been consistent with my spelling of 'color' in
the code. :)

In my testing I saw a 2% performance penalty for using --color on a tree
with the following diffstat:

80 files changed, 4588 insertions(+), 1588 deletions(-)

Other than changes from this review, my remaining task lists appears to be:

 * unit tests
 * Capability/platform detection (including win32 detection)

Feedback appreciated.

-- 

Russ.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: colour_in_core-3618.patch
Type: text/x-patch
Size: 10952 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20080809/687c19a3/attachment.bin