[PATCH v3] diff-highlight: do not split multibyte characters

From: Kyle J. McKay <mackyle@gmail.com>
Date: Apr 3, 2015, 22:15 UTC
Message-ID: <6a8dcc870e53040e1f54d7c36a1b33a@74d39fa044aa309eaea14b9f57fe79c>
In-Reply-To: <CAFT+Tg8-tUBAvgX1bTni7joye_ZuZ_NOT_mmamnnm5GdWzEhrg@mail.gmail.com>

When the input is UTF-8 and Perl is operating on bytes instead of characters, a diff that changes one multibyte character to another that shares an initial byte sequence will result in a broken diff display as the common byte sequence prefix will be separated from the rest of the bytes in the multibyte character.

For example, if a single line contains only the unicode character U+C9C4 (encoded as UTF-8 0xEC, 0xA7, 0x84) and that line is then changed to the unicode character U+C9C0 (encoded as UTF-8 0xEC, 0xA7, 0x80), when operating on bytes diff-highlight will show only the single byte change from 0x84 to 0x80 thus creating invalid UTF-8 and a broken diff display.

Fix this by putting Perl into character mode when splitting the line and then back into byte mode after the split is finished.

The utf8::xxx functions require Perl 5.8 so we require that as well.

Also, since we are mucking with code in the split_line function, we change a '*' quantifier to a '+' quantifier when matching the $COLOR expression which has the side effect of speeding everything up while eliminating useless '' elements in the returned array.

Reported-by: Yi EungJun <semtlenori@gmail.com>
Signed-off-by: Kyle J. McKay <mackyle@gmail.com>
---

On Apr 2, 2015, at 19:19, Yi, EungJun wrote:

Show 20 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.
>
> On Fri, Apr 3, 2015 at 10:24 AM, Jeff King <peff@peff.net> wrote:
>> EungJun, does this version meet your needs?

This version differs from the former as follows:

1) Slightly faster code that eliminates the need for Encode::is_utf8.

2) The '*' quantifier is changed to '+' in the split_line regexs which  
was probably the original intent anyway as using '*' generates useless  
empty elements.  This has the side effect of greatly increasing the  
speed so the tiny speed penalty for the UTF-8 checking is vastly  
overwhelmed by the overall speed up. :)

3) The 'use 5.008;' line has been added since the utf8::xxx functions  
require Perl 5.8

-Kyle

 contrib/diff-highlight/diff-highlight | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight
index 08c88bbc..ffefc31a 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -1,5 +1,6 @@
 #!/usr/bin/perl
 
+use 5.008;
 use warnings FATAL => 'all';
 use strict;
 
@@ -164,8 +165,12 @@ sub highlight_pair {
 
 sub split_line {
 	local $_ = shift;
-	return map { /$COLOR/ ? $_ : (split //) }
-	       split /($COLOR*)/;
+	return utf8::decode($_) ?
+		map { utf8::encode($_); $_ }
+			map { /$COLOR/ ? $_ : (split //) }
+			split /($COLOR+)/ :
+		map { /$COLOR/ ? $_ : (split //) }
+		split /($COLOR+)/;
 }
 
 sub highlight_line {
---