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

> 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