Re: [PATCH] diff-highlight: Fix broken multibyte string
- From
Jeff King <peff@peff.net>
- Date
- Apr 3, 2015, 22:08 UTC
- Message-ID
- <20150403220821.GB11220@peff.net>
- In-Reply-To
- <CAFT+Tg8-tUBAvgX1bTni7joye_ZuZ_NOT_mmamnnm5GdWzEhrg@mail.gmail.com>
On Fri, Apr 03, 2015 at 11:19:24AM +0900, Yi, EungJun wrote:
Show 15 quoted lines
> > I timed this one versus the existing diff-highlight. It's about 7%
> > slower. That's not great, but is acceptable to me. The String::Multibyte
> > version was a lot faster, which was nice (but I'm still unclear on
> > _why_).
>
> I think the reason is here:
>
> > sub split_line {
> > local $_ = shift;
> > return map { /$COLOR/ ? $_ : ($mbcs ? $mbcs->strsplit('', $_) : split //) }
> > split /($COLOR)/;
> > }
>
> I removed "*" from "split /($COLOR*)/". Actually I don't know why "*"
> was required but I need to remove it to make my patch works correctly.Ah, OK, that makes more sense. The "*" was meant to handle the case of multiple groups of ANSI colors in a row. But I think it should have been "+" in that case, as we would otherwise split on the empty field, which would mean character-by-character. And the second "split" in the map would then be superfluous, which would break your patch (we've already split the multi-byte characters before we even hit $mbcs->strsplit).
Kyle's patch does not care, because it tweaks the string so that normal split works. Which means there is an easy speedup here. :)
Doing:
diff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight index 08c88bb..1c4b599 100755 --- a/contrib/diff-highlight/diff-highlight +++ b/contrib/diff-highlight/diff-highlight @@ -165,7 +165,7 @@ sub highlight_pair { sub split_line { local $_ = shift; return map { /$COLOR/ ? $_ : (split //) } - split /($COLOR*)/; + split /($COLOR+)/; } sub highlight_line { gives me a 25% speed improvement, and the same output processing git.git's entire "git log -p" output. I thought that meant we could also optimize out the "map" call entirely, and just use the first split (with "*") to end up with a list of $COLOR chunks and single characters, but it does not seem to work. So maybe I am misreading something about what is going on. -Peff