{"thread":{"id":"29082","subject":"[RFD] Handling of non-UTF8 data in gitweb","startedAt":"2011-12-04T16:09:30Z","lastAt":"2012-01-06T16:35:31Z","messageCount":7,"participants":["Jakub Narebski","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"180296","messageId":"201112041709.32212.jnareb@gmail.com","threadId":"29082","inReplyTo":null,"subject":"[RFD] Handling of non-UTF8 data in gitweb","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-12-04T16:09:30Z","receivedAt":"2011-12-04T16:09:30Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Hello!\n\nCurrently gitweb converts data it receives from git commands to Perl \ninternal utf8 representation via to_utf8() subroutine\n\n # decode sequences of octets in utf8 into Perl's internal form,\n # which is utf-8 with utf8 flag set if needed.  gitweb writes out\n # in utf-8 thanks to \"binmode STDOUT, ':utf8'\" at beginning\n sub to_utf8 {\n \tmy $str = shift;\n \treturn undef unless defined $str;\n \tif (utf8::valid($str)) {\n \t\tutf8::decode($str);\n \t\treturn $str;\n \t} else {\n \t\treturn decode($fallback_encoding, $str, Encode::FB_DEFAULT);\n \t}\n }\n\nEach part of data must be handled separately.  It is quite error prone\nprocess, as can be seen from quite a number of patches that fix handling\nof UTF-8 data (latest from Jürgen).\n\n\nMuch, much simpler would be to force opening of all files (including \noutput pipes from git commands) in ':utf8' mode:\n\n  use open qw(:std :utf8);\n\n[Note: perhaps instead of ':utf8' it should be ':encoding(UTF-8)' \n there...]\n\nBut doing this would change gitweb behavior.  Currently when \nencountering something (usually line of output) that is not valid \nUTF-8, we decode it (to UTF-8) using $fallback_encoding, by default\n'latin1'.  Note however that this value is per gitweb installation,\nnot per repository.\n\nUsing \"use open qw(:std :utf8);\" would be like changing the value of \n$fallback_encoding to 'utf8' -- errors would be ignored, and characters \nwhich are invalid in UTF-8 encoding would get replaced[1] with \nsubstitution character '�' U+FFFD.\n\nThough at least for HTML output we could use Encode::FB_HTMLCREF \nhandling (which would produce &#NNN;) or Encode::FB_XMLCREF (which\nwould produce &#xHHHH;), though this must be done after HTML escaping...\nand is probaby not worth it (FYI this can be done by setting \n$PerlIO::encoding::fallback to either of those values[2])\n\n[1] http://perldoc.perl.org/Encode.html#Handling-Malformed-Data\n    http://p3rl.org/Encode\n[2] http://perldoc.perl.org/PerlIO/encoding.html\n    http://p3rl.org/PerlIO::encoding\n\nI don't know if people are relying on the old behavior.  I guess\nit could be emulated by defining our own 'utf-8-with-fallback'\nencoding, or by defining our own PerlIO layer with PerlIO::via.\nBut it no longer be simple solution (though still automatic).\n\n\nAlternate approach would be to audit gitweb code, and call to_utf8\nbefore storing extracted output of git command in variable (excluding\nsave types like SHA-1, filemode, timestamp and timezone).  The fact\nthat to_utf8 is idempotent and can be called multiple times would\nhelp here, I think.\n\n\nThe correct solution would be of course to respect `gui.encoding`\nper-repository config variable, and `encoding` gitattribute...\nthough the latter is hampered by the fact that there is currently\nno way to read attribute with \"git check-attr\" from a given tree:\nthink of a diff of change of encoding of a file!\n\n-- \nJakub Narebski\nPoland\n"},{"id":"180329","messageId":"20111206010749.GB12760@sigill.intra.peff.net","threadId":"29082","inReplyTo":"201112041709.32212.jnareb@gmail.com","subject":"Re: [RFD] Handling of non-UTF8 data in gitweb","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-06T01:07:49Z","receivedAt":"2011-12-06T01:07:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 04, 2011 at 05:09:30PM +0100, Jakub Narebski wrote:\n\n> The correct solution would be of course to respect `gui.encoding`\n> per-repository config variable, and `encoding` gitattribute...\n> though the latter is hampered by the fact that there is currently\n> no way to read attribute with \"git check-attr\" from a given tree:\n> think of a diff of change of encoding of a file!\n\nWe deal with the same problem at GitHub.\n\nThere really isn't a good way to specify per-file encodings. Something\nlike gui.encoding is too coarse. As you mentioned, we don't do per-tree\ngitattribute lookups, so the encoding attribute has problems when the\nencoding of a file changes. But even if we implemented them, you still\nhave the problem of getting a raw sha1 (e.g., git diff 9624865 e0a3260).\nThere's no way to look up attributes for that.\n\nIt would be nice if you could put an \"encoding\" header into the blob\nobject. You could use the .gitattributes in place at \"git add\" time to\nset it. And then at lookup time, you either have the encoding, or you\nassume it's in utf8 (if it isn't binary, of course).\n\nBut there's no room in the blob format for headers; the content starts\nright after the size header.\n\nYou can get around this by searching the history for a tree that\ncontains the blob, and then checking the gitattributes. It's expensive,\nbut you could build a cache over time. However, it's not guaranteed to\nprovide a single answer; you could have multiple trees that mention the\nblobs, each with different attributes.\n\nAnd even if you implement all that, we have the problem that older blobs\nwon't have gotten an encoding header, even if they would have under the\nnew rules. So rather than assuming utf8, you have to make a guess\nanyway.\n\nAt GitHub, we talked about a lot of these options and ended up just\nusing an encoding-detection library to make a best guess. It seems to\nwork well in practice, but it's only been deployed for a couple of\nmonths.\n\n-Peff\n"},{"id":"180433","messageId":"7vehwhcj3q.fsf@alter.siamese.dyndns.org","threadId":"29082","inReplyTo":"201112041709.32212.jnareb@gmail.com","subject":"Re: [RFD] Handling of non-UTF8 data in gitweb","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-07T00:37:29Z","receivedAt":"2011-12-07T00:37:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> But doing this would change gitweb behavior.  Currently when \n> encountering something (usually line of output) that is not valid \n> UTF-8, we decode it (to UTF-8) using $fallback_encoding, by default\n> 'latin1'.  Note however that this value is per gitweb installation,\n> not per repository.\n\nI think we added and you acked 00f429a (gitweb: Handle non UTF-8 text\nbetter, 2007-06-03) for a good reason, and I think the above argues that\nwhatever issue the commit tried to address is a non-issue. Is it really\ntrue?\n\n> ... I guess\n> it could be emulated by defining our own 'utf-8-with-fallback'\n> encoding, or by defining our own PerlIO layer with PerlIO::via.\n> But it no longer be simple solution (though still automatic).\n\nBetween the current \"everybody who read from the input must remember to\ncall to_utf8\" and \"everybody gets to read utf8 automatically for internal\nprocessing\", even though the latter may involve one-time pain to set up\nthe framework to do so, the pros-and-cons feels obvious to me.\n"},{"id":"180805","messageId":"201112101718.34848.jnareb@gmail.com","threadId":"29082","inReplyTo":"7vehwhcj3q.fsf@alter.siamese.dyndns.org","subject":"Re: [RFD] Handling of non-UTF8 data in gitweb","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-12-10T16:18:33Z","receivedAt":"2011-12-10T16:18:33Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, 7 Dec 2011, Junio C Hamano wrote:\n> Jakub Narebski <jnareb@gmail.com> writes:\n> \n> > But doing this would change gitweb behavior.  Currently when \n> > encountering something (usually line of output) that is not valid \n> > UTF-8, we decode it (to UTF-8) using $fallback_encoding, by default\n> > 'latin1'.  Note however that this value is per gitweb installation,\n> > not per repository.\n> \n> I think we added and you acked 00f429a (gitweb: Handle non UTF-8 text\n> better, 2007-06-03) for a good reason, and I think the above argues that\n> whatever issue the commit tried to address is a non-issue. Is it really\n> true?\n\nI think that UTF-8 is much more prevalent character encoding in operating\nsystems, programming languages, APIs, and software applications than it\nwas 4 years ago.\n\nAlso the solution implemented in said commit was a good start, but it\nremains incomplete: $fallback_encoding is per-installation which is too\nbig granularity (there is `gui.encoding` per-repository config... but it\nis about main not fallback encoding; best would be to use gitattribute\nbut currently there is no way to check attribute value at given revision).\n\nThe proposed\n\n  use open qw(:std :utf8);\n\nand removal of to_utf8 and $fallback_encoding would be regression compared\nto post-00f429a... but the tradeoff of more robust UTF-8 handling might be\nworth it.\n\n\nNote that to_utf8 handles git command output part by part, not as a whole;\nfor UTF-8 vs latin1 (i.e. iso-8859-1) it does not matter though because\nlatin1 is very unlikely to be recognized as valid utf-8[1], and ASCII\ncharacters pass-through for UTF-8.\n\n[1]: http://en.wikipedia.org/wiki/UTF-8#Advantages\n\n> > ... I guess\n> > it could be emulated by defining our own 'utf-8-with-fallback'\n> > encoding, or by defining our own PerlIO layer with PerlIO::via.\n> > But it no longer be simple solution (though still automatic).\n> \n> Between the current \"everybody who read from the input must remember to\n> call to_utf8\" and \"everybody gets to read utf8 automatically for internal\n> processing\", even though the latter may involve one-time pain to set up\n> the framework to do so, the pros-and-cons feels obvious to me.\n\nThere is also a matter of performance (':utf8' and ':encoding(UTF-8)'\nare AFAIK implemented in C, both the Encode part and PerlIO part).\n-- \nJakub Narebski\nPoland\n"},{"id":"180841","messageId":"7vhb169x92.fsf@alter.siamese.dyndns.org","threadId":"29082","inReplyTo":"201112101718.34848.jnareb@gmail.com","subject":"Re: [RFD] Handling of non-UTF8 data in gitweb","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-12T05:26:01Z","receivedAt":"2011-12-12T05:26:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> On Wed, 7 Dec 2011, Junio C Hamano wrote:\n>> I think we added and you acked 00f429a (gitweb: Handle non UTF-8 text\n>> better, 2007-06-03) for a good reason, and I think the above argues that\n>> whatever issue the commit tried to address is a non-issue. Is it really\n>> true?\n>\n> I think that UTF-8 is much more prevalent character encoding in operating\n> systems, programming languages, APIs, and software applications than it\n> was 4 years ago.\n\nYeah, that was the kind of \"reasoning behind it\" explanation I was hoping\nto see spelled out for people to agree or disagree.\n\nBut then the updated gitweb won't have trouble showing history of some\nprojects that has 4 yours or longer history (hopefully Git itself is not\namong them).\n\n> The proposed\n>\n>   use open qw(:std :utf8);\n>\n> and removal of to_utf8 and $fallback_encoding would be regression compared\n> to post-00f429a... but the tradeoff of more robust UTF-8 handling might be\n> worth it.\n>\n>> > ... I guess\n>> > it could be emulated by defining our own 'utf-8-with-fallback'\n>> > encoding, or by defining our own PerlIO layer with PerlIO::via.\n>> > But it no longer be simple solution (though still automatic).\n>> \n>> Between the current \"everybody who read from the input must remember to\n>> call to_utf8\" and \"everybody gets to read utf8 automatically for internal\n>> processing\", even though the latter may involve one-time pain to set up\n>> the framework to do so, the pros-and-cons feels obvious to me.\n>\n> There is also a matter of performance (':utf8' and ':encoding(UTF-8)'\n> are AFAIK implemented in C, both the Encode part and PerlIO part).\n\nWould a reasonable first step be to replace the calls to bare \"open\" with\na wrapper that simulates the \"open\" interface (e.g. \"sub git_open\"), but\nstill keep the same behaviour as post-00f429a that could be much slower\nthan the native one?  Then a separate patch can build a \"regression but\nuses native and much faster\" alternative on top, no?\n"},{"id":"181444","messageId":"201112182300.59409.jnareb@gmail.com","threadId":"29082","inReplyTo":"7vehwhcj3q.fsf@alter.siamese.dyndns.org","subject":"Re: [RFD] Handling of non-UTF8 data in gitweb","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-12-18T22:00:58Z","receivedAt":"2011-12-18T22:00:58Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, 7 Dec 2011, Junio C Hamano wrote:\n> Jakub Narebski <jnareb@gmail.com> writes:\n> \n> > But doing this would change gitweb behavior.  Currently when \n> > encountering something (usually line of output) that is not valid \n> > UTF-8, we decode it (to UTF-8) using $fallback_encoding, by default\n> > 'latin1'.  Note however that this value is per gitweb installation,\n> > not per repository.\n> \n> I think we added and you acked 00f429a (gitweb: Handle non UTF-8 text\n> better, 2007-06-03) for a good reason, and I think the above argues that\n> whatever issue the commit tried to address is a non-issue. Is it really\n> true?\n\nActually... the change in \n\n  00f429a (gitweb: Handle non UTF-8 text better, 2007-06-03)\n\nworked correctly, but since \n\n  e5d3de5 (gitweb: use Perl built-in utf8 function for UTF-8 decoding., 2007-12-04)\n\nit was changed to a NON-WORKING version - and *nobody* protested.\n\n   sub to_utf8 {\n   \tmy $str = shift;\n  -\tmy $res;\n  -\teval { $res = decode_utf8($str, Encode::FB_CROAK); };\n  -\tif (defined $res) {\n  -\t\treturn $res;\n  +\tif (utf8::valid($str)) {\n  +\t\tutf8::decode($str);\n  +\t\treturn $str;\n   \t} else {\n   \t\treturn decode($fallback_encoding, $str, Encode::FB_DEFAULT);\n   \t}\n\nWell, it works for utf8 and latin1 _only_ (with $fallback_encoding being\nset to 'latin1' by default), and for latin1 by historical accident... that\nmight be why nobody noticed.  $fallback_encoding != 'latin1' or 'utf8'\ndidn't work.\n\nutf8::valid(STRING) is an internal function that tests whether STRING is\nin a consistent state regarding UTF-8.  It returns true is well-formed\nUTF-8 and has the UTF-8 flag on _or_ if string is held as bytes (both\nthese states are 'consistent').  For gitweb the second option was true.\n\nNote that utf8:decode(STRING) returns false if STRING is invalid as UTF-8.\n\nWhat makes it all work is the fact that utf8:decode(STRING) turns on UTF-8\nflag only if source string is valid UTF-8 and contains multi-byte UTF-8\ncharacters... and that if string doesn't have UTF-8 flag set it is treated\nas in native Perl encoding, i.e. 'latin1' / 'iso-8859-1' (unless it is EBCDIC).\nIt is ':utf8' layer that actually convert 'latin1' to 'utf8'.\n\n-- >8 --\nSubject: [PATCH] gitweb: Fix fallback mode of to_utf8 subroutine\n\ne5d3de5 (gitweb: use Perl built-in utf8 function for UTF-8 decoding.,\n2007-12-04) was meant to make gitweb faster by using Perl's internals\n(see subsection \"Messing with Perl's Internals\" in Encode(3pm) manpage)\n\nSimple benchmark confirms that (old = 00f429a, new = this version):\n\n        old  new\n  old    -- -65%\n  new  189%   --\n\nUnfortunately it made fallback mode of to_utf8 do not work...  except\nfor default value 'latin1' of $fallback_encoding ('latin1' is Perl\nnative encoding), which is why it was not noticed for such long time.\n\nutf8::valid(STRING) is an internal function that tests whether STRING\nis in a _consistent state_ regarding UTF-8.  It returns true is\nwell-formed UTF-8 and has the UTF-8 flag on _*or*_ if string is held\nas bytes (both these states are 'consistent').  For gitweb the second\noption was true, as output from git commands is opened without ':utf8'\nlayer.\n\nWhat made it work at all for STRING in 'latin1' encoding is the fact\nthat utf8:decode(STRING) turns on UTF-8 flag only if source string is\nvalid UTF-8 and contains multi-byte UTF-8 characters... and that if\nstring doesn't have UTF-8 flag set it is treated as in native Perl\nencoding, i.e.  'latin1' / 'iso-8859-1' (unless native encoding it is\nEBCDIC ;-)).  It was ':utf8' layer that actually converted 'latin1'\n(no UTF-8 flag == native == 'latin1) to 'utf8'.\n\n\nLet's make use of the fact that utf8:decode(STRING) returns false if\nSTRING is invalid as UTF-8 to check whether to enable fallback mode.\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nI'm sorry for overly wordy commit message...\n\n gitweb/gitweb.perl |    3 +--\n 1 files changed, 1 insertions(+), 2 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex d24763b..75b0970 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1443,8 +1443,7 @@ sub validate_refname {\n sub to_utf8 {\n \tmy $str = shift;\n \treturn undef unless defined $str;\n-\tif (utf8::valid($str)) {\n-\t\tutf8::decode($str);\n+\tif (utf8::valid($str) && utf8::decode($str)) {\n \t\treturn $str;\n \t} else {\n \t\treturn decode($fallback_encoding, $str, Encode::FB_DEFAULT);\n-- \n1.7.6\n"},{"id":"182028","messageId":"201201061735.32908.jnareb@gmail.com","threadId":"29082","inReplyTo":"201112041709.32212.jnareb@gmail.com","subject":"Re: [RFD] Handling of non-UTF8 data in gitweb","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-01-06T16:35:31Z","receivedAt":"2012-01-06T16:35:31Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sun, 4 Dec 2011, Jakub Narebski wrote:\n> \n> Currently gitweb converts data it receives from git commands to Perl \n> internal utf8 representation via to_utf8() subroutine\n[...]\n> Each part of data must be handled separately.  It is quite error prone\n> process, as can be seen from quite a number of patches that fix handling\n> of UTF-8 data (latest from Jürgen).\n> \n> \n> Much, much simpler would be to force opening of all files (including \n> output pipes from git commands) in ':utf8' mode:\n> \n>   use open qw(:std :utf8);\n> \n> [Note: perhaps instead of ':utf8' it should be ':encoding(UTF-8)' \n>  there...]\n> \n> But doing this would change gitweb behavior.  [...]\n[...]\n> I don't know if people are relying on the old behavior.  I guess\n> it could be emulated by defining our own 'utf-8-with-fallback'\n> encoding, or by defining our own PerlIO layer with PerlIO::via.\n> But it no longer be simple solution (though still automatic).\n\nI have now created simple Encode::UTF8WithFallback module, so that\n\n  use Encode::UTF8WithFallback;\n  use open IN => ':encoding(utf8-with-fallback)';\n\nshould be able to replace all calls to to_utf8() without any change\nin behavior; at least simple tests shows that.\n\n\nThere however are two problems with this solution:\n\n1. Encode::UTF8WithFallback should really be a separate Perl module\n   in a separate file (e.g. 'gitweb/lib/Encode/UTF8WithFallback.pm');\n   I was not able to make it work without a separate file.\n\n   This means that it very much requires the change that allows splitting\n   gitweb into many files and/or load extra helper modules, and/or require\n   extra non-core modules but provide and install them with gitweb if they\n   are not available.  These changes are ready, and can be find in \n\n     'gitweb/split'\n   \n   branch in my git.git repositories:\n\n     http://repo.or.cz/w/git/jnareb-git.git\n     https://github.com/jnareb/git\n\n\n2. It turned out that the \"open\" pragma 1.04 from Perl v5.8.6 does not\n   work correctly.  We need at least \"open\" 1.06 (version 1.05 consists\n   supposedly only of documentation-only change).\n\n   Because \"open\" is a core Perl module (core pragma), this means that\n   gitweb will require in practice Perl v5.8.9 at least, increasing\n   version requirement from current v5.8.0\n \n-- \nJakub Narebski\nPoland\n"}]}