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:

    " ",
    ... 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).

To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to