On Wed, Aug 17, 2016 at 08:31:24AM -0700, Brian Henderson wrote:
> contrib/diff-highlight/diff-highlight | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
Junio already commented on the tests, and I think everything he said in
his review is sensible.
As for the code itself, this looks much simpler than some of the things
we discussed off-list, which is good. There is one place that you did
not touch that I think is interesting: split_line.
It splits on $COLOR to mark those bits in their own list elements (so they
can be skipped). But we don't do anything special for $GRAPH there.
Obviously it would be wrong to find $GRAPH in the middle of the line.
But I think we end up in highlight_pair() with a sequence like this for
each line:
[
color-graph,
"|"
color-reset,
" ",
... possibly more graph pipes ...
"-" (or "+")
... actual line data ...
]
We ignore "$COLOR" in the middle of that list, but not the other
syntactic bits. I think this just works because we have to skip forward
to the "-" or "+" element, and that happens to skip over all of the
graph cruft, as well.
In a more general sense, it might have been better for split_line() to
return a list of records, with each one containing a bit for "am I
interesting?" along with the actual token data. But I think because the
graph data cannot contain "-" or "+", it's acceptable to simply leave
this as it is. It might be worth a comment (either in-code, or
describing the strategy in the commit message).
-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