git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] diff-highlight: Fix broken multibyte string

From
Kyle J. McKay <mackyle@gmail.com>
Date
Apr 3, 2015, 01:59 UTC
Message-ID
<1D1557A9-737A-4BF6-A3DE-BF4C0465BD36@gmail.com>
In-Reply-To
<20150403012430.GA16173@peff.net>
On Apr 2, 2015, at 18:24, Jeff King wrote:
Show 12 quoted lines
> On Thu, Apr 02, 2015 at 05:49:24PM -0700, Kyle J. McKay wrote:
>
>> Subject: [PATCH v2] diff-highlight: do not split multibyte characters
>>
>> 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.
>
> Thanks, I had a feeling we should be able to do something with perl's
> builtin utf8 support.  This doesn't help people with other encodings,

It should work as well as the original did for any 1-byte encoding. That is, if it's not valid UTF-8 it should pass through unchanged and any single byte encoding should just work. But, as you point out, multibyte encodings other than UTF-8 won't work, but they should behave the same as they did before.

> but I'm not sure the original was all that helpful either (in that we
> don't actually _know_ the file encodings in the first place).

I think it should work fine on any single byte encoding (i.e. ISO-8859- x, WINDOWS-1252, etc.).

> I timed this one versus the existing diff-highlight. It's about 7%
> slower.
I'd expect that, we're doing extra work we weren't doing before.
> 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_).

Must be the mbcs->strsplit routine has special case code for splitting on '' to just split on character boundaries.

Show 15 quoted lines
>> Fix this by putting Perl into character mode when splitting the
>> line and then back into byte mode after the split is finished.
>
> I also wondered if we could simply put stdin into utf8 mode. But it
> looks like it will barf whenever it gets invalid utf8. Checking for
> valid utf8 and only doing the multi-byte split in that case (as you do
> here) is a lot more robust.
>
>> While the utf8::xxx functions are built-in and do not require
>> any 'use' statement, the utf8::is_utf8 function did not appear
>> until Perl 5.8.1, but is identical to the Encode::is_utf8
>> function which is available in 5.8 so we use that instead of
>> utf8::is_utf8.
>
> Makes sense. I'm happy enough listing perl 5.8 as a dependency.

Maybe that should be added. The rest of Git's perl code seems to have a 'use 5.008;' already, so I figured that was a reasonable dependency. :)

-Kyle
Previous: Jeff KingNext: Jeff King
Message 5 of 13 in “diff-highlight: Fix broken multibyte string”
  1. diff-highlight: Fix broken multibyte stringYi EungJun, Mar 30, 2015
  2. Jeff KingMar 30, 2015
  3. Kyle J. McKayApr 3, 2015
  4. Jeff KingApr 3, 2015
  5. Kyle J. McKayApr 3, 2015
  6. Jeff KingApr 3, 2015
  7. Yi, EungJunApr 3, 2015
  8. Jeff KingApr 3, 2015
  9. Kyle J. McKayApr 3, 2015
  10. Jeff KingApr 4, 2015
  11. diff-highlight: do not split multibyte charactersKyle J. McKay, Apr 3, 2015
  12. Jeff KingApr 4, 2015
  13. Yi, EungJunApr 4, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.