{"thread":{"id":"38944","subject":"[PATCH] diff-highlight: Fix broken multibyte string","startedAt":"2015-03-30T15:55:33Z","lastAt":"2015-04-04T14:47:05Z","messageCount":13,"participants":["Yi EungJun","Jeff King","Kyle J. McKay","Yi, EungJun"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"258674","messageId":"1427730933-26189-1-git-send-email-eungjun.yi@navercorp.com","threadId":"38944","inReplyTo":null,"subject":"[PATCH] diff-highlight: Fix broken multibyte string","fromName":"Yi EungJun","fromEmail":"semtlenori@gmail.com","sentAt":"2015-03-30T15:55:33Z","receivedAt":"2015-03-30T15:55:33Z","isPatch":true,"sender":{"key":"semtlenori@gmail.com","avatar":"https://gravatar.com/avatar/8363435d2badb3450df0dd7c4ec2113f6e1d62c44dd3cf7dfe83c5a389b8a9bf?d=mp&s=160"},"body":"From: Yi EungJun \n\nHighlighted string might be broken if the common subsequence is a proper subset\nof a multibyte character. For example, if the old string is \"진\" and the new\nstring is \"지\", then we expect the diff is rendered as follows:\n\n\t-진\n\t+지\n\nbut actually it was rendered as follows:\n\n -<84>\n +<80>\n\nThis fixes the bug by splitting the string by multibyte characters.\n---\n contrib/diff-highlight/diff-highlight | 25 +++++++++++++++++++++++--\n 1 file changed, 23 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight\nindex 08c88bb..2662c1a 100755\n--- a/contrib/diff-highlight/diff-highlight\n+++ b/contrib/diff-highlight/diff-highlight\n@@ -2,6 +2,9 @@\n \n use warnings FATAL => 'all';\n use strict;\n+use File::Basename;\n+use File::Spec::Functions qw( catdir );\n+use String::Multibyte;\n \n # Highlight by reversing foreground and background. You could do\n # other things like bold or underline if you prefer.\n@@ -24,6 +27,8 @@ my @removed;\n my @added;\n my $in_hunk;\n \n+my $mbcs = get_mbcs();\n+\n # Some scripts may not realize that SIGPIPE is being ignored when launching the\n # pager--for instance scripts written in Python.\n $SIG{PIPE} = 'DEFAULT';\n@@ -164,8 +169,8 @@ sub highlight_pair {\n \n sub split_line {\n \tlocal $_ = shift;\n-\treturn map { /$COLOR/ ? $_ : (split //) }\n-\t split /($COLOR*)/;\n+\treturn map { /$COLOR/ ? $_ : ($mbcs ? $mbcs->strsplit('', $_) : split //) }\n+\t split /($COLOR)/;\n }\n \n sub highlight_line {\n@@ -211,3 +216,19 @@ sub is_pair_interesting {\n \t $suffix_a !~ /^$BORING*$/ ||\n \t $suffix_b !~ /^$BORING*$/;\n }\n+\n+# Returns an instance of String::Multibyte based on the charset defined by\n+# i18n.commitencoding or UTF-8, or undef if String::Multibyte doesn't support\n+# the charset.\n+sub get_mbcs {\n+\tmy $dir = catdir(dirname($INC{'String/Multibyte.pm'}), 'Multibyte');\n+\topendir my $dh, $dir or return;\n+\tmy @mbcs_charsets = grep s/[.]pm\\z//, readdir $dh;\n+\tclose $dh;\n+\tmy $expected_charset = `git config i18n.commitencoding` || \"UTF-8\";\n+\t$expected_charset =~ s/-//g;\n+\tmy @matches = grep {/^$expected_charset$/i} @mbcs_charsets;\n+\tmy $charset = shift @matches;\n+\n+\treturn eval 'String::Multibyte->new($charset)';\n+}\n-- \n2.3.2.209.gd67f9d5.dirty\n"},{"id":"258699","messageId":"20150330221635.GB25212@peff.net","threadId":"38944","inReplyTo":"1427730933-26189-1-git-send-email-eungjun.yi@navercorp.com","subject":"Re: [PATCH] diff-highlight: Fix broken multibyte string","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-30T22:16:35Z","receivedAt":"2015-03-30T22:16:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 31, 2015 at 12:55:33AM +0900, Yi EungJun wrote:\n\n> From: Yi EungJun \n> \n> Highlighted string might be broken if the common subsequence is a proper subset\n> of a multibyte character. For example, if the old string is \"진\" and the new\n> string is \"지\", then we expect the diff is rendered as follows:\n> \n> \t-진\n> \t+지\n> \n> but actually it was rendered as follows:\n> \n> -<84>\n> +<80>\n> \n> This fixes the bug by splitting the string by multibyte characters.\n\nYeah, I agree the current output is not ideal, and this should address\nthe problem. I was worried that multi-byte splitting would make things\nslower, but in my tests, it actually speeds things up!\n\nThat surprised me. The big difference is calling $mbcs->strsplit instead\nof regular split. Could it be that it's much faster than regular split?\nOr is it that the resulting strings are faster for the rest of the\nprocessing (e.g., perl hits a \"slow path\" on the sheared characters)? It\ndoesn't really matter, I guess, but certainly I was curious.\n\n> +use File::Basename;\n> +use File::Spec::Functions qw( catdir );\n> +use String::Multibyte;\n\nUnfortunately, String::Multibyte is not a standard module, and is not\neven packed for Debian systems (I got mine from CPAN). Can we make this\na conditional include (e.g., 'eval \"require String::Multibyte\"' in\nget_mbcs, and return undef if that fails?). Then people without it can\nstill use the script.\n\n> +# Returns an instance of String::Multibyte based on the charset defined by\n> +# i18n.commitencoding or UTF-8, or undef if String::Multibyte doesn't support\n> +# the charset.\n\nHrm. The characters we are processing are not in the commit message, but\nin the files themselves. In fact, there may be many different charsets\n(i.e., a different one for each file), and we really don't have a good\nway of knowing which is in play. I'd say that using the commit\nencoding is our best guess, though. What happens with $mbcs->split when\nthe input is not a valid character in the charset (i.e., when we guess\nwrong)?\n\nIf we are going to use the commit encoding, wouldn't\ni18n.logOutputEncoding be a better choice?\n\n> +sub get_mbcs {\n> +\tmy $dir = catdir(dirname($INC{'String/Multibyte.pm'}), 'Multibyte');\n> +\topendir my $dh, $dir or return;\n> +\tmy @mbcs_charsets = grep s/[.]pm\\z//, readdir $dh;\n\nYuck. This is a lot more intimate with String::Multibyte's\nimplementation than I'd like to be. Could we perhaps just run the\nconstructor on any candidates charsets, and then return the first hit\nthat gives us something besides undef?\n\n-Peff\n"},{"id":"258911","messageId":"ffa56a1b1257732077c287a5cfdd138@74d39fa044aa309eaea14b9f57fe79c","threadId":"38944","inReplyTo":"20150330221635.GB25212@peff.net","subject":"Re: [PATCH] diff-highlight: Fix broken multibyte string","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2015-04-03T00:49:24Z","receivedAt":"2015-04-03T00:49:24Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Mar 30, 2015, at 15:16, Jeff King wrote:\n\n> Yeah, I agree the current output is not ideal, and this should address\n> the problem. I was worried that multi-byte splitting would make things\n> slower, but in my tests, it actually speeds things up!\n\n[...]\n\n> Unfortunately, String::Multibyte is not a standard module, and is not\n> even packed for Debian systems (I got mine from CPAN). Can we make\n> this\n> a conditional include (e.g., 'eval \"require String::Multibyte\"' in\n> get_mbcs, and return undef if that fails?). Then people without it can\n> still use the script.\n\n[...]\n\n> Yuck. This is a lot more intimate with String::Multibyte's\n> implementation than I'd like to be.\n\nSo I was curious about this and played with it and was able to\nreproduce the problem as described.\n\nHere's an alternate fix that should work for everyone with Perl 5.8\nor later.\n\n-Kyle\n\n-- 8< --\nSubject: [PATCH v2] diff-highlight: do not split multibyte characters\n\nWhen the input is UTF-8 and Perl is operating on bytes instead\nof characters, a diff that changes one multibyte character to\nanother that shares an initial byte sequence will result in a\nbroken diff display as the common byte sequence prefix will be\nseparated from the rest of the bytes in the multibyte character.\n\nFor example, if a single line contains only the unicode\ncharacter U+C9C4 (encoded as UTF-8 0xEC, 0xA7, 0x84) and that\nline is then changed to the unicode character U+C9C0 (encoded as\nUTF-8 0xEC, 0xA7, 0x80), when operating on bytes diff-highlight\nwill show only the single byte change from 0x84 to 0x80 thus\ncreating invalid UTF-8 and a broken diff display.\n\nFix this by putting Perl into character mode when splitting the\nline and then back into byte mode after the split is finished.\n\nWhile the utf8::xxx functions are built-in and do not require\nany 'use' statement, the utf8::is_utf8 function did not appear\nuntil Perl 5.8.1, but is identical to the Encode::is_utf8\nfunction which is available in 5.8 so we use that instead of\nutf8::is_utf8.\n\nReported-by: Yi EungJun \nSigned-off-by: Kyle J. McKay \n---\n contrib/diff-highlight/diff-highlight | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight\nindex 08c88bbc..8e9b5ada 100755\n--- a/contrib/diff-highlight/diff-highlight\n+++ b/contrib/diff-highlight/diff-highlight\n@@ -2,6 +2,7 @@\n \n use warnings FATAL => 'all';\n use strict;\n+use Encode ();\n \n # Highlight by reversing foreground and background. You could do\n # other things like bold or underline if you prefer.\n@@ -164,8 +165,10 @@ sub highlight_pair {\n \n sub split_line {\n \tlocal $_ = shift;\n-\treturn map { /$COLOR/ ? $_ : (split //) }\n-\t split /($COLOR*)/;\n+\tutf8::decode($_);\n+\treturn map { utf8::encode($_) if Encode::is_utf8($_); $_ }\n+\t\tmap { /$COLOR/ ? $_ : (split //) }\n+\t\tsplit /($COLOR*)/;\n }\n \n sub highlight_line {\n---\n"},{"id":"258914","messageId":"20150403012430.GA16173@peff.net","threadId":"38944","inReplyTo":"ffa56a1b1257732077c287a5cfdd138@74d39fa044aa309eaea14b9f57fe79c","subject":"Re: [PATCH] diff-highlight: Fix broken multibyte string","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-03T01:24:30Z","receivedAt":"2015-04-03T01:24:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 02, 2015 at 05:49:24PM -0700, Kyle J. McKay wrote:\n\n> Subject: [PATCH v2] diff-highlight: do not split multibyte characters\n> \n> When the input is UTF-8 and Perl is operating on bytes instead\n> of characters, a diff that changes one multibyte character to\n> another that shares an initial byte sequence will result in a\n> broken diff display as the common byte sequence prefix will be\n> separated from the rest of the bytes in the multibyte character.\n\nThanks, I had a feeling we should be able to do something with perl's\nbuiltin utf8 support. This doesn't help people with other encodings,\nbut I'm not sure the original was all that helpful either (in that we\ndon't actually _know_ the file encodings in the first place).\n\nI briefly confirmed that this seems to do the right thing on po/bg.po,\nwhich has a couple of sheared characters when viewed with the existing\ncode.\n\nI timed this one versus the existing diff-highlight. It's about 7%\nslower. That's not great, but is acceptable to me. The String::Multibyte\nversion was a lot faster, which was nice (but I'm still unclear on\n_why_).\n\n> Fix this by putting Perl into character mode when splitting the\n> line and then back into byte mode after the split is finished.\n\nI also wondered if we could simply put stdin into utf8 mode. But it\nlooks like it will barf whenever it gets invalid utf8. Checking for\nvalid utf8 and only doing the multi-byte split in that case (as you do\nhere) is a lot more robust.\n\n> While the utf8::xxx functions are built-in and do not require\n> any 'use' statement, the utf8::is_utf8 function did not appear\n> until Perl 5.8.1, but is identical to the Encode::is_utf8\n> function which is available in 5.8 so we use that instead of\n> utf8::is_utf8.\n\nMakes sense. I'm happy enough listing perl 5.8 as a dependency.\n\nEungJun, does this version meet your needs?\n\n-Peff\n"},{"id":"258916","messageId":"1D1557A9-737A-4BF6-A3DE-BF4C0465BD36@gmail.com","threadId":"38944","inReplyTo":"20150403012430.GA16173@peff.net","subject":"Re: [PATCH] diff-highlight: Fix broken multibyte string","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2015-04-03T01:59:50Z","receivedAt":"2015-04-03T01:59:50Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Apr 2, 2015, at 18:24, Jeff King wrote:\n\n> On Thu, Apr 02, 2015 at 05:49:24PM -0700, Kyle J. McKay wrote:\n>\n>> Subject: [PATCH v2] diff-highlight: do not split multibyte characters\n>>\n>> When the input is UTF-8 and Perl is operating on bytes instead\n>> of characters, a diff that changes one multibyte character to\n>> another that shares an initial byte sequence will result in a\n>> broken diff display as the common byte sequence prefix will be\n>> separated from the rest of the bytes in the multibyte character.\n>\n> Thanks, I had a feeling we should be able to do something with perl's\n> builtin utf8 support. This doesn't help people with other encodings,\n\nIt should work as well as the original did for any 1-byte encoding. \nThat is, if it's not valid UTF-8 it should pass through unchanged and \nany single byte encoding should just work. But, as you point out, \nmultibyte encodings other than UTF-8 won't work, but they should \nbehave the same as they did before.\n\n> but I'm not sure the original was all that helpful either (in that we\n> don't actually _know_ the file encodings in the first place).\n\nI think it should work fine on any single byte encoding (i.e. ISO-8859- \nx, WINDOWS-1252, etc.).\n\n> I timed this one versus the existing diff-highlight. It's about 7%\n> slower.\n\nI'd expect that, we're doing extra work we weren't doing before.\n\n> That's not great, but is acceptable to me. The String::Multibyte\n> version was a lot faster, which was nice (but I'm still unclear on\n> _why_).\n\nMust be the mbcs->strsplit routine has special case code for splitting \non '' to just split on character boundaries.\n\n>> Fix this by putting Perl into character mode when splitting the\n>> line and then back into byte mode after the split is finished.\n>\n> I also wondered if we could simply put stdin into utf8 mode. But it\n> looks like it will barf whenever it gets invalid utf8. Checking for\n> valid utf8 and only doing the multi-byte split in that case (as you do\n> here) is a lot more robust.\n>\n>> While the utf8::xxx functions are built-in and do not require\n>> any 'use' statement, the utf8::is_utf8 function did not appear\n>> until Perl 5.8.1, but is identical to the Encode::is_utf8\n>> function which is available in 5.8 so we use that instead of\n>> utf8::is_utf8.\n>\n> Makes sense. I'm happy enough listing perl 5.8 as a dependency.\n\nMaybe that should be added. The rest of Git's perl code seems to have \na 'use 5.008;' already, so I figured that was a reasonable \ndependency. :)\n\n-Kyle\n"},{"id":"258917","messageId":"CAFT+Tg8-tUBAvgX1bTni7joye_ZuZ_NOT_mmamnnm5GdWzEhrg@mail.gmail.com","threadId":"38944","inReplyTo":"20150403012430.GA16173@peff.net","subject":"Re: [PATCH] diff-highlight: Fix broken multibyte string","fromName":"Yi, EungJun","fromEmail":"semtlenori@gmail.com","sentAt":"2015-04-03T02:19:24Z","receivedAt":"2015-04-03T02:19:24Z","isPatch":true,"sender":{"key":"semtlenori@gmail.com","avatar":"https://gravatar.com/avatar/8363435d2badb3450df0dd7c4ec2113f6e1d62c44dd3cf7dfe83c5a389b8a9bf?d=mp&s=160"},"body":"> I timed this one versus the existing diff-highlight. It's about 7%\n> slower. That's not great, but is acceptable to me. The String::Multibyte\n> version was a lot faster, which was nice (but I'm still unclear on\n> _why_).\n\nI think the reason is here:\n\n> sub split_line {\n> local $_ = shift;\n> return map { /$COLOR/ ? $_ : ($mbcs ? $mbcs->strsplit('', $_) : split //) }\n> split /($COLOR)/;\n> }\n\nI removed \"*\" from \"split /($COLOR*)/\". Actually I don't know why \"*\"\nwas required but I need to remove it to make my patch works correctly.\n\n\nOn Fri, Apr 3, 2015 at 10:24 AM, Jeff King wrote:\n> On Thu, Apr 02, 2015 at 05:49:24PM -0700, Kyle J. McKay wrote:\n>\n>> Subject: [PATCH v2] diff-highlight: do not split multibyte characters\n>>\n>> When the input is UTF-8 and Perl is operating on bytes instead\n>> of characters, a diff that changes one multibyte character to\n>> another that shares an initial byte sequence will result in a\n>> broken diff display as the common byte sequence prefix will be\n>> separated from the rest of the bytes in the multibyte character.\n>\n> Thanks, I had a feeling we should be able to do something with perl's\n> builtin utf8 support. This doesn't help people with other encodings,\n> but I'm not sure the original was all that helpful either (in that we\n> don't actually _know_ the file encodings in the first place).\n>\n> I briefly confirmed that this seems to do the right thing on po/bg.po,\n> which has a couple of sheared characters when viewed with the existing\n> code.\n>\n> I timed this one versus the existing diff-highlight. It's about 7%\n> slower. That's not great, but is acceptable to me. The String::Multibyte\n> version was a lot faster, which was nice (but I'm still unclear on\n> _why_).\n>\n>> Fix this by putting Perl into character mode when splitting the\n>> line and then back into byte mode after the split is finished.\n>\n> I also wondered if we could simply put stdin into utf8 mode. But it\n> looks like it will barf whenever it gets invalid utf8. Checking for\n> valid utf8 and only doing the multi-byte split in that case (as you do\n> here) is a lot more robust.\n>\n>> While the utf8::xxx functions are built-in and do not require\n>> any 'use' statement, the utf8::is_utf8 function did not appear\n>> until Perl 5.8.1, but is identical to the Encode::is_utf8\n>> function which is available in 5.8 so we use that instead of\n>> utf8::is_utf8.\n>\n> Makes sense. I'm happy enough listing perl 5.8 as a dependency.\n>\n> EungJun, does this version meet your needs?\n>\n> -Peff\n"},{"id":"258951","messageId":"20150403214729.GA11220@peff.net","threadId":"38944","inReplyTo":"1D1557A9-737A-4BF6-A3DE-BF4C0465BD36@gmail.com","subject":"Re: [PATCH] diff-highlight: Fix broken multibyte string","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-03T21:47:29Z","receivedAt":"2015-04-03T21:47:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 02, 2015 at 06:59:50PM -0700, Kyle J. McKay wrote:\n\n> It should work as well as the original did for any 1-byte encoding. That\n> is, if it's not valid UTF-8 it should pass through unchanged and any single\n> byte encoding should just work. But, as you point out, multibyte encodings\n> other than UTF-8 won't work, but they should behave the same as they did\n> before.\n\nYeah, sorry, I should have been more clear that I meant multibyte\nencodings. UTF-8 is the only common multibyte encoding I run across, but\nthat's because Latin1 served most of my pre-UTF-8 needs. I suspect\nthings are very different for people in Asia. I don't know how\nbadly they would want support for other encodings. I'm happy to go with\na UTF-8 solution for now, and see if anybody wants to expand it further\nlater.\n\n> >I timed this one versus the existing diff-highlight. It's about 7%\n> >slower.\n> \n> I'd expect that, we're doing extra work we weren't doing before.\n\nI was worried would be 200% or something. :)\n\n> >Makes sense. I'm happy enough listing perl 5.8 as a dependency.\n> \n> Maybe that should be added. The rest of Git's perl code seems to have a\n> 'use 5.008;' already, so I figured that was a reasonable dependency. :)\n\nI shouldn't have said \"listing\". I just meant \"have\" as a dependency. I\nam also happy with adding \"use 5.008\", but I agree it's probably not\nnecessary at this point. It was released in 2002 (wow, has it really\nbeen that long?).\n\n-Peff\n"},{"id":"258956","messageId":"20150403220821.GB11220@peff.net","threadId":"38944","inReplyTo":"CAFT+Tg8-tUBAvgX1bTni7joye_ZuZ_NOT_mmamnnm5GdWzEhrg@mail.gmail.com","subject":"Re: [PATCH] diff-highlight: Fix broken multibyte string","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-03T22:08:22Z","receivedAt":"2015-04-03T22:08:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 03, 2015 at 11:19:24AM +0900, Yi, EungJun wrote:\n\n> > I timed this one versus the existing diff-highlight. It's about 7%\n> > slower. That's not great, but is acceptable to me. The String::Multibyte\n> > version was a lot faster, which was nice (but I'm still unclear on\n> > _why_).\n> \n> I think the reason is here:\n> \n> > sub split_line {\n> > local $_ = shift;\n> > return map { /$COLOR/ ? $_ : ($mbcs ? $mbcs->strsplit('', $_) : split //) }\n> > split /($COLOR)/;\n> > }\n> \n> I removed \"*\" from \"split /($COLOR*)/\". Actually I don't know why \"*\"\n> was required but I need to remove it to make my patch works correctly.\n\nAh, OK, that makes more sense. The \"*\" was meant to handle the case of\nmultiple groups of ANSI colors in a row. But I think it should have been\n\"+\" in that case, as we would otherwise split on the empty field, which\nwould mean character-by-character. And the second \"split\" in the map\nwould then be superfluous, which would break your patch (we've already\nsplit the multi-byte characters before we even hit $mbcs->strsplit).\n\nKyle's patch does not care, because it tweaks the string so that normal\nsplit works. Which means there is an easy speedup here. :)\n\nDoing:\n\ndiff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight\nindex 08c88bb..1c4b599 100755\n--- a/contrib/diff-highlight/diff-highlight\n+++ b/contrib/diff-highlight/diff-highlight\n@@ -165,7 +165,7 @@ sub highlight_pair {\n sub split_line {\n \tlocal $_ = shift;\n \treturn map { /$COLOR/ ? $_ : (split //) }\n-\t split /($COLOR*)/;\n+\t split /($COLOR+)/;\n }\n \n sub highlight_line {\n\ngives me a 25% speed improvement, and the same output processing\ngit.git's entire \"git log -p\" output.\n\nI thought that meant we could also optimize out the \"map\" call entirely,\nand just use the first split (with \"*\") to end up with a list of $COLOR\nchunks and single characters, but it does not seem to work. So maybe I\nam misreading something about what is going on.\n\n-Peff\n"},{"id":"258957","messageId":"6a8dcc870e53040e1f54d7c36a1b33a@74d39fa044aa309eaea14b9f57fe79c","threadId":"38944","inReplyTo":"CAFT+Tg8-tUBAvgX1bTni7joye_ZuZ_NOT_mmamnnm5GdWzEhrg@mail.gmail.com","subject":"[PATCH v3] diff-highlight: do not split multibyte characters","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2015-04-03T22:15:14Z","receivedAt":"2015-04-03T22:15:14Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"When the input is UTF-8 and Perl is operating on bytes instead of\ncharacters, a diff that changes one multibyte character to another\nthat shares an initial byte sequence will result in a broken diff\ndisplay as the common byte sequence prefix will be separated from\nthe rest of the bytes in the multibyte character.\n\nFor example, if a single line contains only the unicode character\nU+C9C4 (encoded as UTF-8 0xEC, 0xA7, 0x84) and that line is then\nchanged to the unicode character U+C9C0 (encoded as UTF-8 0xEC,\n0xA7, 0x80), when operating on bytes diff-highlight will show only\nthe single byte change from 0x84 to 0x80 thus creating invalid UTF-8\nand a broken diff display.\n\nFix this by putting Perl into character mode when splitting the line\nand then back into byte mode after the split is finished.\n\nThe utf8::xxx functions require Perl 5.8 so we require that as well.\n\nAlso, since we are mucking with code in the split_line function, we\nchange a '*' quantifier to a '+' quantifier when matching the $COLOR\nexpression which has the side effect of speeding everything up while\neliminating useless '' elements in the returned array.\n\nReported-by: Yi EungJun \nSigned-off-by: Kyle J. McKay \n---\n\nOn Apr 2, 2015, at 19:19, Yi, EungJun wrote:\n>> I timed this one versus the existing diff-highlight. It's about 7%\n>> slower. That's not great, but is acceptable to me. The \n>> String::Multibyte\n>> version was a lot faster, which was nice (but I'm still unclear on\n>> _why_).\n>\n> I think the reason is here:\n>\n>> sub split_line {\n>> local $_ = shift;\n>> return map { /$COLOR/ ? $_ : ($mbcs ? $mbcs->strsplit('', $_) : \n>> split //) }\n>> split /($COLOR)/;\n>> }\n>\n> I removed \"*\" from \"split /($COLOR*)/\". Actually I don't know why \"*\"\n> was required but I need to remove it to make my patch works correctly.\n>\n> On Fri, Apr 3, 2015 at 10:24 AM, Jeff King wrote:\n>> EungJun, does this version meet your needs?\n\nThis version differs from the former as follows:\n\n1) Slightly faster code that eliminates the need for Encode::is_utf8.\n\n2) The '*' quantifier is changed to '+' in the split_line regexs which \nwas probably the original intent anyway as using '*' generates useless \nempty elements. This has the side effect of greatly increasing the \nspeed so the tiny speed penalty for the UTF-8 checking is vastly \noverwhelmed by the overall speed up. :)\n\n3) The 'use 5.008;' line has been added since the utf8::xxx functions \nrequire Perl 5.8\n\n-Kyle\n\n contrib/diff-highlight/diff-highlight | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight\nindex 08c88bbc..ffefc31a 100755\n--- a/contrib/diff-highlight/diff-highlight\n+++ b/contrib/diff-highlight/diff-highlight\n@@ -1,5 +1,6 @@\n #!/usr/bin/perl\n \n+use 5.008;\n use warnings FATAL => 'all';\n use strict;\n \n@@ -164,8 +165,12 @@ sub highlight_pair {\n \n sub split_line {\n \tlocal $_ = shift;\n-\treturn map { /$COLOR/ ? $_ : (split //) }\n-\t split /($COLOR*)/;\n+\treturn utf8::decode($_) ?\n+\t\tmap { utf8::encode($_); $_ }\n+\t\t\tmap { /$COLOR/ ? $_ : (split //) }\n+\t\t\tsplit /($COLOR+)/ :\n+\t\tmap { /$COLOR/ ? $_ : (split //) }\n+\t\tsplit /($COLOR+)/;\n }\n \n sub highlight_line {\n---\n"},{"id":"258958","messageId":"CDAB9DBC-0F59-4176-BD9F-620A124EA300@gmail.com","threadId":"38944","inReplyTo":"20150403220821.GB11220@peff.net","subject":"Re: [PATCH] diff-highlight: Fix broken multibyte string","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2015-04-03T22:24:09Z","receivedAt":"2015-04-03T22:24:09Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Apr 3, 2015, at 15:08, Jeff King wrote:\n> Doing:\n>\n> diff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff- \n> highlight/diff-highlight\n> index 08c88bb..1c4b599 100755\n> --- a/contrib/diff-highlight/diff-highlight\n> +++ b/contrib/diff-highlight/diff-highlight\n> @@ -165,7 +165,7 @@ sub highlight_pair {\n> sub split_line {\n> \tlocal $_ = shift;\n> \treturn map { /$COLOR/ ? $_ : (split //) }\n> -\t split /($COLOR*)/;\n> +\t split /($COLOR+)/;\n> }\n>\n> sub highlight_line {\n>\n> gives me a 25% speed improvement, and the same output processing\n> git.git's entire \"git log -p\" output.\n>\n> I thought that meant we could also optimize out the \"map\" call \n> entirely,\n> and just use the first split (with \"*\") to end up with a list of \n> $COLOR\n> chunks and single characters, but it does not seem to work. So maybe I\n> am misreading something about what is going on.\n\nI think our emails crossed in flight...\n\nUsing just the first split (with \"*\") produces useless empty elements \nwhich I think ends up causing problems. I suppose you could surround \nit with a grep /./ to remove them but that would defeat the point of \nthe optimization.\n\n-Kyle\n"},{"id":"258990","messageId":"20150404140902.GA25455@peff.net","threadId":"38944","inReplyTo":"6a8dcc870e53040e1f54d7c36a1b33a@74d39fa044aa309eaea14b9f57fe79c","subject":"Re: [PATCH v3] diff-highlight: do not split multibyte characters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-04T14:09:02Z","receivedAt":"2015-04-04T14:09:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 03, 2015 at 03:15:14PM -0700, Kyle J. McKay wrote:\n\n> When the input is UTF-8 and Perl is operating on bytes instead of\n> characters, a diff that changes one multibyte character to another\n> that shares an initial byte sequence will result in a broken diff\n> display as the common byte sequence prefix will be separated from\n> the rest of the bytes in the multibyte character.\n> \n> For example, if a single line contains only the unicode character\n> U+C9C4 (encoded as UTF-8 0xEC, 0xA7, 0x84) and that line is then\n> changed to the unicode character U+C9C0 (encoded as UTF-8 0xEC,\n> 0xA7, 0x80), when operating on bytes diff-highlight will show only\n> the single byte change from 0x84 to 0x80 thus creating invalid UTF-8\n> and a broken diff display.\n> \n> Fix this by putting Perl into character mode when splitting the line\n> and then back into byte mode after the split is finished.\n> \n> The utf8::xxx functions require Perl 5.8 so we require that as well.\n> \n> Also, since we are mucking with code in the split_line function, we\n> change a '*' quantifier to a '+' quantifier when matching the $COLOR\n> expression which has the side effect of speeding everything up while\n> eliminating useless '' elements in the returned array.\n> \n> Reported-by: Yi EungJun \n> Signed-off-by: Kyle J. McKay \n\nThis version looks good to me. I looked over the diff of running \"git\nlog -p --color\" on git.git through diff-highlight before and after this\npatch, and everything looks like an improvement.\n\n Acked-by: Jeff King \n\nThanks both of you for working on this.\n\n-Peff\n"},{"id":"258991","messageId":"20150404141026.GB25455@peff.net","threadId":"38944","inReplyTo":"CDAB9DBC-0F59-4176-BD9F-620A124EA300@gmail.com","subject":"Re: [PATCH] diff-highlight: Fix broken multibyte string","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-04T14:10:26Z","receivedAt":"2015-04-04T14:10:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 03, 2015 at 03:24:09PM -0700, Kyle J. McKay wrote:\n\n> >I thought that meant we could also optimize out the \"map\" call entirely,\n> >and just use the first split (with \"*\") to end up with a list of $COLOR\n> >chunks and single characters, but it does not seem to work. So maybe I\n> >am misreading something about what is going on.\n> \n> I think our emails crossed in flight...\n> \n> Using just the first split (with \"*\") produces useless empty elements which\n> I think ends up causing problems. I suppose you could surround it with a\n> grep /./ to remove them but that would defeat the point of the optimization.\n\nYeah, the problem is the use of (). We want to keep the $COLOR\ndelimiters, but not the empty ones. Perhaps you could do:\n\n split /($COLOR+)|/\n\nbut I didn't try it. I think what you posted is good and a lot less\nsubtle.\n\n-Peff\n"},{"id":"258992","messageId":"CAFT+Tg_dFvpxauPJgRi86qTFo4k6dXa6WST+UPTguisA9ma83Q@mail.gmail.com","threadId":"38944","inReplyTo":"20150404140902.GA25455@peff.net","subject":"Re: [PATCH v3] diff-highlight: do not split multibyte characters","fromName":"Yi, EungJun","fromEmail":"semtlenori@gmail.com","sentAt":"2015-04-04T14:47:05Z","receivedAt":"2015-04-04T14:47:05Z","isPatch":true,"sender":{"key":"semtlenori@gmail.com","avatar":"https://gravatar.com/avatar/8363435d2badb3450df0dd7c4ec2113f6e1d62c44dd3cf7dfe83c5a389b8a9bf?d=mp&s=160"},"body":"On Fri, Apr 3, 2015 at 10:24 AM, Jeff King wrote:\n>\n> EungJun, does this version meet your needs?\n>\n> -Peff\n\nYes, this patch is enough to meet my needs because it works well on\nUTF-8, the only encoding I use. And this patch looks better than my\none because it is smaller, doesn't depend on String::Multibyte and\nseems to have no side-effect.\n\nI hope someone who use another multibyte encoding will send a patch to\nsupport the encoding in future... :)\n\nOn Sat, Apr 4, 2015 at 11:09 PM, Jeff King wrote:\n> On Fri, Apr 03, 2015 at 03:15:14PM -0700, Kyle J. McKay wrote:\n>\n>> When the input is UTF-8 and Perl is operating on bytes instead of\n>> characters, a diff that changes one multibyte character to another\n>> that shares an initial byte sequence will result in a broken diff\n>> display as the common byte sequence prefix will be separated from\n>> the rest of the bytes in the multibyte character.\n>>\n>> For example, if a single line contains only the unicode character\n>> U+C9C4 (encoded as UTF-8 0xEC, 0xA7, 0x84) and that line is then\n>> changed to the unicode character U+C9C0 (encoded as UTF-8 0xEC,\n>> 0xA7, 0x80), when operating on bytes diff-highlight will show only\n>> the single byte change from 0x84 to 0x80 thus creating invalid UTF-8\n>> and a broken diff display.\n>>\n>> Fix this by putting Perl into character mode when splitting the line\n>> and then back into byte mode after the split is finished.\n>>\n>> The utf8::xxx functions require Perl 5.8 so we require that as well.\n>>\n>> Also, since we are mucking with code in the split_line function, we\n>> change a '*' quantifier to a '+' quantifier when matching the $COLOR\n>> expression which has the side effect of speeding everything up while\n>> eliminating useless '' elements in the returned array.\n>>\n>> Reported-by: Yi EungJun \n>> Signed-off-by: Kyle J. McKay \n>\n> This version looks good to me. I looked over the diff of running \"git\n> log -p --color\" on git.git through diff-highlight before and after this\n> patch, and everything looks like an improvement.\n>\n> Acked-by: Jeff King \n>\n> Thanks both of you for working on this.\n>\n> -Peff\n"}]}