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

[Jeff King <peff@peff.net>]

On Fri, Feb 10, 2012 at 10:47:13PM +0100, Michał Kiedrowicz wrote:

>  contrib/diff-highlight/diff-highlight |   96 ++++++++++++++++++++++-----------
>  1 files changed, 65 insertions(+), 31 deletions(-)

Thanks for sending. I looked at a whole bunch of patches, and I was
pleasantly surprised to find how infrequently we hit false positives in
practice. In fact, the only things that looked worse with your patch
were places where your patch happened to turn on highlighting for lines
where the existing heuristics already were a little ugly (i.e., the
problem was not your patch, but that the existing heuristic is sometimes
non-optimal).

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.

> After looking at outputs I noticed that it can also ignore lines with
> prefixes/suffixes that consist only of punctuation (asterisk, semicolon, dot,
> etc), because otherwise whole line is highlighted except for terminating
> punctuation.

I missed this note when I applied the patch and started looking at the
outputs, and ended up having a similar thought. However, I don't know
that it buys much in practice, and it's nice to be fairly agnostic about
content. I did leave that open to easy tweaking in my series, though.

  [1/5]: diff-highlight: make perl strict and warnings fatal
  [2/5]: diff-highlight: don't highlight whole lines
  [3/5]: diff-highlight: refactor to prepare for multi-line hunks
  [4/5]: diff-highlight: match multi-line hunks
  [5/5]: diff-highlight: document some non-optimal cases

-Peff