Re: [PATCH v2 3/3] diff-highlight: add support for --graph output.

From: Jeff King <peff@peff.net>
Date: Aug 19, 2016, 14:37 UTC
Message-ID: <20160819143725.eex2fcmbhn3s77or@sigill.intra.peff.net>
In-Reply-To: <20160817153124.7770-4-henderson.bj@gmail.com>

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