Re: [PATCH] diff-highlight: Work for multiline changes too

From: Jeff King <peff@peff.net>
Date: Feb 14, 2012, 00:22 UTC
Message-ID: <20120214002209.GA23171@sigill.intra.peff.net>
In-Reply-To: <7v8vk6b7pq.fsf@alter.siamese.dyndns.org>

On Mon, Feb 13, 2012 at 04:05:05PM -0800, Junio C Hamano wrote:

Show 10 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > I ended up pulling your changes out into a few distinct commits. That
> > made it easier for me to review and understand what was going on (and
> > hopefully ditto for other reviewers, or people who end up bisecting or
> > reading the log later). I'll post that series in a moment.
> 
> This is all nice.  As I am lazy and this is a long neglected contrib/
> material I didn't even know it existed ;-), I am tempted to apply them
> directly on top of 'master'.

Yeah, given the contrib nature, I'm comfortable just applying. I wanted to wait on the attribution question from Michał before sending a final version, though.

Show 16 quoted lines

> This shows the first hunk of your "diff-highlight: refactor to prepare for
> multi-line hunks" like this to me, by the way.
> 
> @@ -23,7 +23,7 @@ while (<>) {
>             $window[2] =~ /^$COLOR*\+/ &&
>             $window[3] !~ /^$COLOR*\+/) {
>                 print shift @window;
> {-               show_pair}(shift @window, shift @window);
> +               show_{hunk}(shift @window, shift @window);
>         }
>         else {
>                 print shift @window;
> 
> Is this intended, or is setting "diff.color.old = red reverse" not
> supported (without the custom configuration, the leading blank on the old
> line is not highlighted)?

No, it's not intended, and should be:

   -                show-{pair}...
   +                show+{hunk}...

(and appears that way with my config). I suspect it is because the default highlight color is a subset of your "old" configured color. It is ANSI "reverse video", but sadly that does not work as a toggle (i.e., it does not reverse your reverse, but simply is a no-op).

I chose reverse because I like the way it looks, and because it should Just Work if people have selected alternate colors (I never dreamed somebody would use "reverse" all the time, as I find it horribly ugly. But to each his own). We could read the highlight color from diff.highlight{Old,New}. That would also let people highlight in purple or something if they care.

-Peff