Jeff King <peff@xxxxxxxx> wrote: > 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. Yeah, I completely agree with that. > 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. Very nicely done. > > > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html