{"thread":{"id":"35842","subject":"[PATCH] contrib/diff-highlight: multibyte characters diff","startedAt":"2014-02-11T09:09:10Z","lastAt":"2014-02-13T19:14:16Z","messageCount":8,"participants":["Yoshihiro Sugi","Junio C Hamano","Jeff King","Thomas Adam","brian m. carlson"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"234609","messageId":"1392109750-47852-1-git-send-email-sugi1982@gmail.com","threadId":"35842","inReplyTo":null,"subject":"[PATCH] contrib/diff-highlight: multibyte characters diff","fromName":"Yoshihiro Sugi","fromEmail":"sugi1982@gmail.com","sentAt":"2014-02-11T09:09:10Z","receivedAt":"2014-02-11T09:09:10Z","isPatch":true,"sender":{"key":"sugi1982@gmail.com","avatar":null},"body":"Signed-off-by: Yoshihiro Sugi \n\ndiff-highlight split each hunks and compare them as byte sequences.\nit causes problems when diff hunks include multibyte characters.\nThis change enable to work on such cases by decoding inputs and encoding output as utf8 string.\n---\n contrib/diff-highlight/diff-highlight | 16 ++++++++++------\n 1 file changed, 10 insertions(+), 6 deletions(-)\n\ndiff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight\nindex c4404d4..49b4f53 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 qw(decode_utf8 encode_utf8);\n \n # Highlight by reversing foreground and background. You could do\n # other things like bold or underline if you prefer.\n@@ -15,8 +16,9 @@ my @added;\n my $in_hunk;\n \n while (<>) {\n+\t$_ = decode_utf8($_);\n \tif (!$in_hunk) {\n-\t\tprint;\n+\t\tprint encode_utf8($_);\n \t\t$in_hunk = /^$COLOR*\\@/;\n \t}\n \telsif (/^$COLOR*-/) {\n@@ -30,7 +32,7 @@ while (<>) {\n \t\t@removed = ();\n \t\t@added = ();\n \n-\t\tprint;\n+\t\tprint encode_utf8($_);\n \t\t$in_hunk = /^$COLOR*[\\@ ]/;\n \t}\n \n@@ -58,7 +60,8 @@ sub show_hunk {\n \n \t# If one side is empty, then there is nothing to compare or highlight.\n \tif (!@$a || !@$b) {\n-\t\tprint @$a, @$b;\n+\t\tprint encode_utf8($_) for @$a;\n+\t\tprint encode_utf8($_) for @$b;\n \t\treturn;\n \t}\n \n@@ -67,17 +70,18 @@ sub show_hunk {\n \t# stupid, and only handle multi-line hunks that remove and add the same\n \t# number of lines.\n \tif (@$a != @$b) {\n-\t\tprint @$a, @$b;\n+\t\tprint encode_utf8($_) for @$a;\n+\t\tprint encode_utf8($_) for @$b;\n \t\treturn;\n \t}\n \n \tmy @queue;\n \tfor (my $i = 0; $i < @$a; $i++) {\n \t\tmy ($rm, $add) = highlight_pair($a->[$i], $b->[$i]);\n-\t\tprint $rm;\n+\t\tprint encode_utf8($rm);\n \t\tpush @queue, $add;\n \t}\n-\tprint @queue;\n+\tprint encode_utf8($_) for @queue;\n }\n \n sub highlight_pair {\n-- \n1.8.5.3\n"},{"id":"234624","messageId":"xmqqioslphuu.fsf@gitster.dls.corp.google.com","threadId":"35842","inReplyTo":"1392109750-47852-1-git-send-email-sugi1982@gmail.com","subject":"Re: [PATCH] contrib/diff-highlight: multibyte characters diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-11T19:30:33Z","receivedAt":"2014-02-11T19:30:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yoshihiro Sugi writes:\n\n> Signed-off-by: Yoshihiro Sugi \n>\n> diff-highlight split each hunks and compare them as byte sequences.\n> it causes problems when diff hunks include multibyte characters.\n> This change enable to work on such cases by decoding inputs and encoding output as utf8 string.\n> ---\n> contrib/diff-highlight/diff-highlight | 16 ++++++++++------\n> 1 file changed, 10 insertions(+), 6 deletions(-)\n>\n> diff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight\n> index c4404d4..49b4f53 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 qw(decode_utf8 encode_utf8);\n> \n> # Highlight by reversing foreground and background. You could do\n> # other things like bold or underline if you prefer.\n> @@ -15,8 +16,9 @@ my @added;\n> my $in_hunk;\n> \n> while (<>) {\n> +\t$_ = decode_utf8($_);\n> \tif (!$in_hunk) {\n> -\t\tprint;\n> +\t\tprint encode_utf8($_);\n> \t\t$in_hunk = /^$COLOR*\\@/;\n> \t}\n> \telsif (/^$COLOR*-/) {\n> @@ -30,7 +32,7 @@ while (<>) {\n> \t\t@removed = ();\n> \t\t@added = ();\n> \n> -\t\tprint;\n> +\t\tprint encode_utf8($_);\n> \t\t$in_hunk = /^$COLOR*[\\@ ]/;\n> \t}\n> \n> @@ -58,7 +60,8 @@ sub show_hunk {\n> \n> \t# If one side is empty, then there is nothing to compare or highlight.\n> \tif (!@$a || !@$b) {\n> -\t\tprint @$a, @$b;\n> +\t\tprint encode_utf8($_) for @$a;\n> +\t\tprint encode_utf8($_) for @$b;\n> \t\treturn;\n> \t}\n> \n> @@ -67,17 +70,18 @@ sub show_hunk {\n> \t# stupid, and only handle multi-line hunks that remove and add the same\n> \t# number of lines.\n> \tif (@$a != @$b) {\n> -\t\tprint @$a, @$b;\n> +\t\tprint encode_utf8($_) for @$a;\n> +\t\tprint encode_utf8($_) for @$b;\n> \t\treturn;\n> \t}\n> \n> \tmy @queue;\n> \tfor (my $i = 0; $i < @$a; $i++) {\n> \t\tmy ($rm, $add) = highlight_pair($a->[$i], $b->[$i]);\n> -\t\tprint $rm;\n> +\t\tprint encode_utf8($rm);\n> \t\tpush @queue, $add;\n> \t}\n> -\tprint @queue;\n> +\tprint encode_utf8($_) for @queue;\n> }\n> \n> sub highlight_pair {\n"},{"id":"234691","messageId":"20140212205948.GA4453@sigill.intra.peff.net","threadId":"35842","inReplyTo":"1392109750-47852-1-git-send-email-sugi1982@gmail.com","subject":"Re: [PATCH] contrib/diff-highlight: multibyte characters diff","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-12T20:59:48Z","receivedAt":"2014-02-12T20:59:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 11, 2014 at 06:09:10PM +0900, Yoshihiro Sugi wrote:\n\n> diff-highlight split each hunks and compare them as byte sequences.\n> it causes problems when diff hunks include multibyte characters.\n> This change enable to work on such cases by decoding inputs and encoding output as utf8 string.\n\nThanks for looking at this. I didn't consider multibyte characters at\nall when I wrote the original.\n\nSadly, applying your patch seems to cause diff-highlight to take about\n1.5x as much CPU (I just tried it on the complete output of \"git log\n--all -p\" in my git.git repository, which went from ~60s to ~90s). That\nis not necessarily a deal-breaker, as it is more important to be\ncorrect. But I wonder if there is any way we can optimize it. From my\nunderstanding, it may not be the encoding/decoding itself, but rather\nthat the utf8-aware text routines are slower. More on that below.\n\n> # Highlight by reversing foreground and background. You could do\n> # other things like bold or underline if you prefer.\n> @@ -15,8 +16,9 @@ my @added;\n> my $in_hunk;\n> \n> while (<>) {\n> +\t$_ = decode_utf8($_);\n\nWhat happens when the diff content is not utf8 at all? The default\nfailure mode for decode_utf8 seems to be to replace non-utf8 sequences\nwith a substitution character. Which means we would corrupt something\nlike iso8859-1, which works fine in the current code.\n\n> \tif (!$in_hunk) {\n> -\t\tprint;\n> +\t\tprint encode_utf8($_);\n\nAnd we have the opposite question here. We know that what we have in the\nstring is perl's internal format. But how do we know that what the\nuser's terminal wants is utf8?\n\nI think in the most general case, we would need to auto-detect the\nencoding for each string (since there is no guarantee that a file even\nretains the same encoding through its history). And then probably punt on\nhighlighting when comparing two lines in different encodings, and if\nthey are in the same encoding, treat them as a sequence of code points\nrather than octets when comparing.\n\nThat's probably way too slow and complicated to worry about. And\nfrankly, I am OK with making the tool work _best_ with UTF-8, as long as\nit can fail gracefully for other encodings (i.e., if you can turn off\nthe utf8 magic when you have another encoding, and we will pass it\nthrough blindly as octets).\n\nWould it be enough to do something like the patch below? The specific\nthings I am shooting for are:\n\n - the only part of the code that cares about the sequence is the\n split/highlight bit, and most lines do not get highlighted at all.\n Therefore it tries to lazily do the decode step, which gets back our\n lost performance (I measured ~12% slowdown).\n\n - we try the utf8 decode and if it fails, fall back to treating it\n like a sequence of bytes. That doesn't help people with other\n multibyte encodings, but at least lets single-byte encodings besides\n utf8 continue to work.\n\nI constructed a very simple test of:\n\n git init &&\n printf 'cont\\xc3\\xa9nt\\n' >file &&\n git add file &&\n printf 'cont\\xc3\\xb6nt\\n' >file &&\n git diff\n\nand it seemed to work. Can you confirm that it does the right thing on\nyour more complicated cases?\n\n-Peff\n\n---\ndiff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight\nindex c4404d4..9447ba2 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 qw(decode_utf8 encode_utf8);\n \n # Highlight by reversing foreground and background. You could do\n # other things like bold or underline if you prefer.\n@@ -73,13 +74,23 @@ sub show_hunk {\n \n \tmy @queue;\n \tfor (my $i = 0; $i < @$a; $i++) {\n-\t\tmy ($rm, $add) = highlight_pair($a->[$i], $b->[$i]);\n-\t\tprint $rm;\n-\t\tpush @queue, $add;\n+\t\tmy ($a_dec, $encode_rm) = decode($a->[$i]);\n+\t\tmy ($b_dec, $encode_add) = decode($b->[$i]);\n+\t\tmy ($rm, $add) = highlight_pair($a_dec, $b_dec);\n+\t\tprint $encode_rm->($rm);\n+\t\tpush @queue, $encode_add->($add);\n \t}\n \tprint @queue;\n }\n \n+sub decode {\n+\tmy $orig = shift;\n+\tmy $decoded = eval { decode_utf8($orig, Encode::FB_CROAK) };\n+\treturn defined $decoded ?\n+\t ($decoded, sub { encode_utf8(shift) }) :\n+\t ($orig, sub { shift });\n+}\n+\n sub highlight_pair {\n \tmy @a = split_line(shift);\n \tmy @b = split_line(shift);\n"},{"id":"234700","messageId":"CA+39Oz5TSPNzYVvFytJwwUhRYjbEp5f_BdBWKT2tcYzpbF1WyQ@mail.gmail.com","threadId":"35842","inReplyTo":"20140212205948.GA4453@sigill.intra.peff.net","subject":"Re: [PATCH] contrib/diff-highlight: multibyte characters diff","fromName":"Thomas Adam","fromEmail":"thomas@xteddy.org","sentAt":"2014-02-12T23:10:49Z","receivedAt":"2014-02-12T23:10:49Z","isPatch":true,"sender":{"key":"thomas@xteddy.org","avatar":"https://gravatar.com/avatar/e7256db4738e501e5d2e84f00bb0bd99503165729573848a03330301fc2adc4a?d=mp&s=160"},"body":"On 12 February 2014 20:59, Jeff King wrote:\n> +sub decode {\n> + my $orig = shift;\n> + my $decoded = eval { decode_utf8($orig, Encode::FB_CROAK) };\n> + return defined $decoded ?\n\nI'd still advocate checking $@ here, rather than the defined $decoded check.\n\n> + ($decoded, sub { encode_utf8(shift) }) :\n> + ($orig, sub { shift });\n> +}\n> +\n\n-- Thomas Adam\n"},{"id":"234702","messageId":"20140212232740.GA11098@sigill.intra.peff.net","threadId":"35842","inReplyTo":"CA+39Oz5TSPNzYVvFytJwwUhRYjbEp5f_BdBWKT2tcYzpbF1WyQ@mail.gmail.com","subject":"Re: [PATCH] contrib/diff-highlight: multibyte characters diff","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-12T23:27:40Z","receivedAt":"2014-02-12T23:27:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 12, 2014 at 11:10:49PM +0000, Thomas Adam wrote:\n\n> On 12 February 2014 20:59, Jeff King wrote:\n> > +sub decode {\n> > + my $orig = shift;\n> > + my $decoded = eval { decode_utf8($orig, Encode::FB_CROAK) };\n> > + return defined $decoded ?\n> \n> I'd still advocate checking $@ here, rather than the defined $decoded check.\n\nI don't mind changing it, but for my edification, what is the advantage?\n\n-Peff\n"},{"id":"234704","messageId":"20140213011753.GD4582@vauxhall.crustytoothpaste.net","threadId":"35842","inReplyTo":"20140212232740.GA11098@sigill.intra.peff.net","subject":"Re: [PATCH] contrib/diff-highlight: multibyte characters diff","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2014-02-13T01:17:54Z","receivedAt":"2014-02-13T01:17:54Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, Feb 12, 2014 at 06:27:40PM -0500, Jeff King wrote:\n> On Wed, Feb 12, 2014 at 11:10:49PM +0000, Thomas Adam wrote:\n> \n> > On 12 February 2014 20:59, Jeff King wrote:\n> > > +sub decode {\n> > > + my $orig = shift;\n> > > + my $decoded = eval { decode_utf8($orig, Encode::FB_CROAK) };\n> > > + return defined $decoded ?\n> > \n> > I'd still advocate checking $@ here, rather than the defined $decoded check.\n> \n> I don't mind changing it, but for my edification, what is the advantage?\n\nThe documentation for decode_utf8 isn't clear, but I don't know if it\ncan ever return undef. What, for example, does it return if $orig is\nnot defined? That's the benefit: it's immediately clear to the user\nthat you're interested in whether it threw an exception, rather than\nwhether it produced a given value.\n\nThat said, $DAYJOB is a Perl shop, and I would certainly not reject this\ncode in review, and depending on the situation, I might even write\nsomething like this. I personally think it's fine.\n\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"234706","messageId":"20140213013751.GA11355@sigill.intra.peff.net","threadId":"35842","inReplyTo":"20140213011753.GD4582@vauxhall.crustytoothpaste.net","subject":"Re: [PATCH] contrib/diff-highlight: multibyte characters diff","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-13T01:37:51Z","receivedAt":"2014-02-13T01:37:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 13, 2014 at 01:17:54AM +0000, brian m. carlson wrote:\n\n> On Wed, Feb 12, 2014 at 06:27:40PM -0500, Jeff King wrote:\n> > On Wed, Feb 12, 2014 at 11:10:49PM +0000, Thomas Adam wrote:\n> > \n> > > On 12 February 2014 20:59, Jeff King wrote:\n> > > > +sub decode {\n> > > > + my $orig = shift;\n> > > > + my $decoded = eval { decode_utf8($orig, Encode::FB_CROAK) };\n> > > > + return defined $decoded ?\n> > > \n> > > I'd still advocate checking $@ here, rather than the defined $decoded check.\n> > \n> > I don't mind changing it, but for my edification, what is the advantage?\n> \n> The documentation for decode_utf8 isn't clear, but I don't know if it\n> can ever return undef. What, for example, does it return if $orig is\n> not defined? That's the benefit: it's immediately clear to the user\n> that you're interested in whether it threw an exception, rather than\n> whether it produced a given value.\n\nI'd argue that I am more interested in whether it returned a value. Let\nus imagine for a moment that decode_utf8 could return undef without\nthrowing an exception. What should the function return in such a case?\n\nI think the only sensible thing is the original (and to indicate that\nthe result was not converted).\n\n-Peff\n"},{"id":"234739","messageId":"1392318856-55920-1-git-send-email-sugi1982@gmail.com","threadId":"35842","inReplyTo":"20140212205948.GA4453@sigill.intra.peff.net","subject":"Re: [PATCH] contrib/diff-highlight: multibyte characters diff","fromName":"Yoshihiro Sugi","fromEmail":"sugi1982@gmail.com","sentAt":"2014-02-13T19:14:16Z","receivedAt":"2014-02-13T19:14:16Z","isPatch":true,"sender":{"key":"sugi1982@gmail.com","avatar":null},"body":"Thanks for reviewing.\nas you wrote, diff content may not be utf8 at all. and we don't know that the user's terminal watns is utf8.\nI think your trying utf8 decode and fall back approach is better than my patch, and do work well.\n\nis using \"$@\" for catching error like the patch below?\nAccording to perldoc Encode.pm, encode/decode with \"FB_CROAK\" may destroy original string. We should probabry use \"LEAVE_SRC\" on decode_utf8's second argument.\n\n---\n contrib/diff-highlight/diff-highlight | 17 ++++++++++++++---\n 1 file changed, 14 insertions(+), 3 deletions(-)\n\ndiff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight\nindex c4404d4..0743851 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 qw(decode_utf8 encode_utf8);\n \n # Highlight by reversing foreground and background. You could do\n # other things like bold or underline if you prefer.\n@@ -73,13 +74,23 @@ sub show_hunk {\n \n \tmy @queue;\n \tfor (my $i = 0; $i < @$a; $i++) {\n-\t\tmy ($rm, $add) = highlight_pair($a->[$i], $b->[$i]);\n-\t\tprint $rm;\n-\t\tpush @queue, $add;\n+\t\tmy ($a_dec, $encode_rm) = decode($a->[$i]);\n+\t\tmy ($b_dec, $encode_add) = decode($b->[$i]);\n+\t\tmy ($rm, $add) = highlight_pair($a_dec, $b_dec);\n+\t\tprint $encode_rm->($rm);\n+\t\tpush @queue, $encode_add->($add);\n \t}\n \tprint @queue;\n }\n \n+sub decode {\n+\tmy $orig = shift;\n+\tmy $decoded = eval { decode_utf8($orig, Encode::FB_CROAK | Encode::LEAVE_SRC) };\n+\treturn $@ ?\n+\t ($orig, sub { shift }) :\n+\t ($decoded, sub { encode_utf8(shift) });\n+}\n+\n sub highlight_pair {\n \tmy @a = split_line(shift);\n \tmy @b = split_line(shift);\n-- \n1.8.5.3\n"}]}