[PATCH] diff: enhance highlighting with color (issue3034)

[Matt Mackall]

On Wed, 2011-10-05 at 03:35 +0400, Kirill Elagin wrote:
> # HG changeset patch
> # User Kirill Elagin <kirelagin at gmail.com>
> # Date 1317770811 -10800
> # Node ID 92931d6d500b30472fecd188d87a1b343a5c3a5e
> # Parent  6dc67dced8c122f6139ae20ccdc03a6b11e8b765
> diff: enhance highlighting with color (issue3034)
> 
> Now the highlighter knows whether it is in the header of the patch or not.

On the BTS, you write:

 Here it goes.

 Just to note: I had to rename “diff.diffline” style to “diff.header” because
 it makes sense to output _every_ header line bold (“diff...” line not being 
 something special) and this is how my patch works (this behaviour can be 
 easily reverted if you really don't like it for some reason).


Actually, no, I don't like it: I consider rolling unrelated changes
together like this to be bad engineering practice. Please send the fix
and only the fix, and leave all the other behavior and code and
formatting as constant as reasonably possible. Then feel free to follow
up with any other behavioral changes you'd like for separate discussion.

Otherwise, the patch looks pretty good except:

> +    in_head = False

We don't like underscores in identifiers:

http://mercurial.selenic.com/wiki/CodingStyle#Naming_conventions

> -            if line and line[0] in '+-':
> +            if not in_head and line.startswith(('+', '-')):

This is an unrelated change that introduces a dependency on Python 2.5.
This is a good example of why unrelated changes are bad: this change
could have easily slipped in and broken our next release for a bunch of
Py2.4 users with no upside. This sort of change also makes your patch
gratuitously more complex and thus harder to review.

-- 
Mathematics is the supreme nostalgia of our time.