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

[Aaron Bentley]

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

Russ Brown wrote:
> Aaron Bentley wrote:
>> In that case, perhaps it would be worth explaining your vision for how
>> this framework will work.

I'd like to understand more about how the individual commands are going
to do colorization.  Are we doing pattern-matching on the output text?
Are we going to tag bits of output so it can be optionally-colorized?
Where  do Reporter objects like CommitReporter and ChangeReporter fit in?

>> 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

> Granted, but these methods won't be consistent with how the rest of the
> commands end up working, (goal 1 above). 

I can easily tweak cdiff to decorate diff and provide
- --color=yes/no/auto.  It wouldn't make particular sense for "diff
- --using", though.

> 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 am sorry to contradict other people on this point, but I do think my
concerns are reasonable.

>>> 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.

> 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 one way to approach this would be to start with colordiffrc support,
and then supplement it with another mechanism or extend it.

> 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.

Code is a cost, not an asset, because all code must be maintained.  It
is better to make intrusive changes if that reduces the amount of
resulting code.

> 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.

I don't think it would fly as a separate command that didn't do diff
colorization.  The existing set-up is very convenient for reviewing
one's work.

> If you take option 2, what you basically end up with is what I already
> have in place

Except that cdiff goes away, so the landscape is clearer for users.

> plus configuration that is colordiff-centric

Until/unless you implement something better.

>, and
> detection that only deals with ANSI terminals

Aren't windows terminals ANSI?  Certainly, DOS could support ANSI with
the right device driver.

> that has a comment above
> specifying that a rewrite is needed anyway.

The rewrite would involve not hardcoding ANSI strings, but instead using
the ncurses facilities.  You don't appear to be championing such an
approach, so I could just as easily write that comment on your code.

> 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.

The point is to avoid confusing users:

Q: How do I get colorized diffs?

A: If you want color, you can use diff --color=yes
   But if you want to configure the colors used, install bzrtools and
   use cdiff.
   But if you want auto-detection of color, you should use diff
   --color=auto
   But if you want context diffs, you should install colordiff and use
   bzr diff --using 'colordiff -c'

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

iD8DBQFIpf+D0F+nu1YWqI0RApxHAJ4nyiM80HHfEPEkfXihb+B8WhE3AACfQL6P
X5+h8gUN+wHhf8ln4iIexfk=
=kHEn
-----END PGP SIGNATURE-----