Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff

["Michał Kiedrowicz" <michal.kiedrowicz@gmail.com>]

Jeff King <peff@peff.net> wrote:

> On Tue, Feb 14, 2012 at 07:23:40PM +0100, Michał Kiedrowicz wrote:
> 
> > > > > BTW GitHub is closed source, but we can check what algorithm does Trac
> > > > > use for diff refinement highlighting (highlighting changed portions of
> > > > > diff).
> > > > > 
> > > > I think it's
> > > > http://trac.edgewall.org/browser/trunk/trac/versioncontrol/diff.py
> > > > (see markup intraline_changes()).
> > [...]
> > [...]
> > > The markup_intraline_changes() function compares lines from preimage and
> > > from postimage pairwise, requiring that number of lines matches, the same
> > > like in your algorithm.
> > 
> > So using Jeff's diff-highlight we remain quite consistent with Trac
> > output. There's nothing we can "steal" from it.
> 
> Neat. When I originally wrote diff-highlight, I was inspired by seeing
> what Trac and GitHub did, but came up with the algorithm on my own.
> Learning that Trac does the multiline thing (as we started doing with
> recent patches) makes me feel even better that it's a good strategy.
> 
> As an aside, I looked at what GitHub does. It turns on highlighting only
> for the single-line case, and does the same prefix/suffix thing.
> 
> It's really not very complex code; most of the hassle in diff-highlight
> is ignoring (but preserving) embedded colors, so that we build on top of
> existing colorization. A web tool will be doing the line coloring itself
> anyway, so it can be much simpler. Elsewhere, I suggested lib-ifying
> diff-highlight for gitweb to use. But once you remove the embedded color
> handling, there really is not that much code, and it's probably simpler
> to just rewrite it.
> 
> -Peff

Sure, now it's a simple algorithm, but if we add more code, we will
have problems with making it consistent in both gitweb and
diff-highlight (which is nice-to-have IMO). Note that my patches to
gitweb already support combined diffs (in obvious cases) while
diff-highlight will fail on them badly (see for example 09bb4eb4f14c).
I haven't done it in diff-highlight because I noticed that problem
while working on patches for gitweb.

Anyway, thanks for looking at Trac/GitHub code and sharing your opinion.