{"thread":{"id":"33440","subject":"[PATCH 1/4] gitweb: Fix utf8 encoding for blob_plain, blobdiff_plain, commitdiff_plain, and patch","startedAt":"2013-04-08T20:08:20Z","lastAt":"2013-04-10T10:23:39Z","messageCount":5,"participants":["Jürgen Kreileder","Junio C Hamano","Jakub Narębski"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"213598","messageId":"m2zjx8bxaj.fsf@blackdown.de","threadId":"33440","inReplyTo":null,"subject":"[PATCH 1/4] gitweb: Fix utf8 encoding for blob_plain, blobdiff_plain, commitdiff_plain, and patch","fromName":"Jürgen Kreileder","fromEmail":"jk@blackdown.de","sentAt":"2013-04-08T20:08:20Z","receivedAt":"2013-04-08T20:08:20Z","isPatch":true,"sender":{"key":"jk@blackdown.de","avatar":"https://avatars.githubusercontent.com/u/4390544?v=4"},"body":"Fixes the encoding for several _plain actions and for text/* and */*+xml blobs. \n\nSigned-off-by: Jürgen Kreileder <jk@blackdown.de>\n---\n gitweb/gitweb.perl |    8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 1309196..9cfe5b5 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -3823,7 +3823,7 @@ sub blob_contenttype {\n \tmy ($fd, $file_name, $type) = @_;\n \n \t$type ||= blob_mimetype($fd, $file_name);\n-\tif ($type eq 'text/plain' && defined $default_text_plain_charset) {\n+\tif (($type =~ m!^text/\\w[-\\w]*$! || $type =~ m!^\\w[-\\w]*/\\w[-\\w]*\\+xml$!) && defined $default_text_plain_charset) {\n \t\t$type .= \"; charset=$default_text_plain_charset\";\n \t}\n \n@@ -7637,7 +7637,9 @@ sub git_blobdiff {\n \t\t\tlast if $line =~ m!^\\+\\+\\+!;\n \t\t}\n \t\tlocal $/ = undef;\n+\t\tbinmode STDOUT, ':raw';\n \t\tprint <$fd>;\n+\t\tbinmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi\n \t\tclose $fd;\n \t}\n }\n@@ -7884,12 +7886,16 @@ sub git_commitdiff {\n \n \t} elsif ($format eq 'plain') {\n \t\tlocal $/ = undef;\n+\t\tbinmode STDOUT, ':raw';\n \t\tprint <$fd>;\n+\t\tbinmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi\n \t\tclose $fd\n \t\t\tor print \"Reading git-diff-tree failed\\n\";\n \t} elsif ($format eq 'patch') {\n \t\tlocal $/ = undef;\n+\t\tbinmode STDOUT, ':raw';\n \t\tprint <$fd>;\n+\t\tbinmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi\n \t\tclose $fd\n \t\t\tor print \"Reading git-format-patch failed\\n\";\n \t}\n-- \n1.7.10.4\n"},{"id":"213620","messageId":"7vd2u4vg4o.fsf@alter.siamese.dyndns.org","threadId":"33440","inReplyTo":"m2zjx8bxaj.fsf@blackdown.de","subject":"Re: [PATCH 1/4] gitweb: Fix utf8 encoding for blob_plain, blobdiff_plain, commitdiff_plain, and patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-08T21:58:47Z","receivedAt":"2013-04-08T21:58:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"jk@blackdown.de (Jürgen Kreileder) writes:\n\n> Fixes the encoding for several _plain actions and for text/* and */*+xml blobs. \n>\n> Signed-off-by: Jürgen Kreileder <jk@blackdown.de>\n> ---\n\nThanks, will queue but not hold until I hear something from Jakub.\n\n>  gitweb/gitweb.perl |    8 +++++++-\n>  1 file changed, 7 insertions(+), 1 deletion(-)\n>\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 1309196..9cfe5b5 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -3823,7 +3823,7 @@ sub blob_contenttype {\n>  \tmy ($fd, $file_name, $type) = @_;\n>  \n>  \t$type ||= blob_mimetype($fd, $file_name);\n> -\tif ($type eq 'text/plain' && defined $default_text_plain_charset) {\n> +\tif (($type =~ m!^text/\\w[-\\w]*$! || $type =~ m!^\\w[-\\w]*/\\w[-\\w]*\\+xml$!) && defined $default_text_plain_charset) {\n>  \t\t$type .= \"; charset=$default_text_plain_charset\";\n>  \t}\n>  \n> @@ -7637,7 +7637,9 @@ sub git_blobdiff {\n>  \t\t\tlast if $line =~ m!^\\+\\+\\+!;\n>  \t\t}\n>  \t\tlocal $/ = undef;\n> +\t\tbinmode STDOUT, ':raw';\n>  \t\tprint <$fd>;\n> +\t\tbinmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi\n>  \t\tclose $fd;\n>  \t}\n>  }\n> @@ -7884,12 +7886,16 @@ sub git_commitdiff {\n>  \n>  \t} elsif ($format eq 'plain') {\n>  \t\tlocal $/ = undef;\n> +\t\tbinmode STDOUT, ':raw';\n>  \t\tprint <$fd>;\n> +\t\tbinmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi\n>  \t\tclose $fd\n>  \t\t\tor print \"Reading git-diff-tree failed\\n\";\n>  \t} elsif ($format eq 'patch') {\n>  \t\tlocal $/ = undef;\n> +\t\tbinmode STDOUT, ':raw';\n>  \t\tprint <$fd>;\n> +\t\tbinmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi\n>  \t\tclose $fd\n>  \t\t\tor print \"Reading git-format-patch failed\\n\";\n>  \t}\n"},{"id":"213649","messageId":"5163FC2D.9050408@gmail.com","threadId":"33440","inReplyTo":"7vd2u4vg4o.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/4] gitweb: Fix utf8 encoding for blob_plain, blobdiff_plain, commitdiff_plain, and patch","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2013-04-09T11:31:57Z","receivedAt":"2013-04-09T11:31:57Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On 08.04.2013, Junio C Hamano wrote:\n> jk@blackdown.de (Jürgen Kreileder) writes:\n> \n>> Fixes the encoding for several _plain actions and for text/* and */*+xml blobs. \n>>\n>> Signed-off-by: Jürgen Kreileder <jk@blackdown.de>\n\nI see that this patch does (or tries to do) two _independent_ things,\nand should be split into at least two separate commits:\n\n>> ---\n> \n> Thanks, will queue but not hold until I hear something from Jakub.\n> \n>>  gitweb/gitweb.perl |    8 +++++++-\n>>  1 file changed, 7 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>> index 1309196..9cfe5b5 100755\n>> --- a/gitweb/gitweb.perl\n>> +++ b/gitweb/gitweb.perl\n>> @@ -3823,7 +3823,7 @@ sub blob_contenttype {\n>>  \tmy ($fd, $file_name, $type) = @_;\n>>  \n>>  \t$type ||= blob_mimetype($fd, $file_name);\n>> -\tif ($type eq 'text/plain' && defined $default_text_plain_charset) {\n>> +\tif (($type =~ m!^text/\\w[-\\w]*$! || $type =~ m!^\\w[-\\w]*/\\w[-\\w]*\\+xml$!) && defined $default_text_plain_charset) {\n>>  \t\t$type .= \"; charset=$default_text_plain_charset\";\n>>  \t}\n\nFirst, it extends adding \"; charset=$default_text_plain_charset\" to\nother mimetypes for 'blob_plain' view to all 'text/*' and '*/*+xml'\nmimetypes (without changing name of variable... though this is more\ncomplicated as it is configuration variable and we would want to\npreserve backward compatibility, but at least a comment would be,\nI think, needed).\n \nOriginally it applied only to 'text/plain' files, which can be\ndisplayed inline by web browser, and which need charset in\n'Content-Type:' HTTP header to be displayed correctly, as they\ndo not include such information inside the file.\n\n'text/html' and 'application/xhtml+xml' can include such information\ninside them (meta http-equiv for 'text/html' and <?xml ...> for\n'application/xhtml+xml').  I don't know what browser does when there\nis conflicting information about charset, i.e. which one wins, but\nit is certainly something to consider.\n\nIt might be a good change; I don't know what web browser do when\nserving 'text/css', 'text/javascript', 'text/xml' to client to\nview without media type known.\n\n\nBTW I have noticed that we do $prevent_xss dance outside\nblob_contenttype(), in it's only caller i.e. git_blob_plain()...\nwhich for example means that 'text/html' converted to 'text/plain'\ndon't get '; charset=...' added.  I guess that it *might* be\nwhat prompted this part of change... but if it is so, it needs\nto be fixed at source, e.g. by moving $prevent_xss to\nblob_contenttype() subroutine.\n\n\nAbout implementation:\n\n>> +\tif (($type =~ m!^text/\\w[-\\w]*$! || $type =~ m!^\\w[-\\w]*/\\w[-\\w]*\\+xml$!) && defined $default_text_plain_charset) {\n>>  \t\t$type .= \"; charset=$default_text_plain_charset\";\n\nwould be better with line split\n\n>> +\tif (($type =~ m!^text/\\w[-\\w]*$! || $type =~ m!^\\w[-\\w]*/\\w[-\\w]*\\+xml$!) &&\n>> +\t    defined $default_text_plain_charset) {\n>>  \t\t$type .= \"; charset=$default_text_plain_charset\";\n\nAlso: do we need to be strict with '\\w[-\\w]*', or would '.*' suffice?\n\n>> @@ -7637,7 +7637,9 @@ sub git_blobdiff {\n>>  \t\t\tlast if $line =~ m!^\\+\\+\\+!;\n>>  \t\t}\n>>  \t\tlocal $/ = undef;\n>> +\t\tbinmode STDOUT, ':raw';\n>>  \t\tprint <$fd>;\n>> +\t\tbinmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi\n>>  \t\tclose $fd;\n>>  \t}\n>>  }\n>> @@ -7884,12 +7886,16 @@ sub git_commitdiff {\n>>  \n>>  \t} elsif ($format eq 'plain') {\n>>  \t\tlocal $/ = undef;\n>> +\t\tbinmode STDOUT, ':raw';\n>>  \t\tprint <$fd>;\n>> +\t\tbinmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi\n>>  \t\tclose $fd\n>>  \t\t\tor print \"Reading git-diff-tree failed\\n\";\n>>  \t} elsif ($format eq 'patch') {\n>>  \t\tlocal $/ = undef;\n>> +\t\tbinmode STDOUT, ':raw';\n>>  \t\tprint <$fd>;\n>> +\t\tbinmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi\n>>  \t\tclose $fd\n>>  \t\t\tor print \"Reading git-format-patch failed\\n\";\n>>  \t}\n\nSecond it changes 'blobdiff_plain', 'commitdiff_plain' (which I think\nthat should be abandoned in favor of 'patch' view; but that is\na separate issue) and 'patch' views so they use binary-safe output.\n\nNote that in all cases (I think) we use\n\n   $cgi->header(\n     -type => 'text/plain',\n     -charset => 'utf-8',\n     ...\n   );\n\npromising web browser that output is as whole in 'utf-8' encoding.\n\nIt is not explained in the commit message what is the reason for this\nchange.  Is it better handing of a situation where files being diff-ed\nbeing in other encoding (like for example in commit that changes\nencoding of files from legacy encoding such like e.g. iso-8859-2\nor cp1250 to utf-8)?  But what about  -charset => 'utf-8'  then?\n\n\nAbout implementation: I think after this change common code crosses\nthreshold for refactoring it into subroutine, for example:\n\n  sub dump_fh_raw {\n  \tmy $fh = shift;\n\n  \tlocal $/ = undef;\n  \tbinmode STDOUT, ':raw';\n  \tprint <$fh>;\n  \tbinmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi\n\n  \treturn $fh;\n  }\n\n-- \nJakub Narębski\n"},{"id":"213727","messageId":"m28v4r9xgs.fsf@zahir.fritz.box","threadId":"33440","inReplyTo":"5163FC2D.9050408@gmail.com","subject":"Re: [PATCH 1/4] gitweb: Fix utf8 encoding for blob_plain, blobdiff_plain, commitdiff_plain, and patch","fromName":"Jürgen Kreileder","fromEmail":"jk@blackdown.de","sentAt":"2013-04-09T21:59:47Z","receivedAt":"2013-04-09T21:59:47Z","isPatch":true,"sender":{"key":"jk@blackdown.de","avatar":"https://avatars.githubusercontent.com/u/4390544?v=4"},"body":"Jakub Narębski <jnareb@gmail.com> writes:\n\n> On 08.04.2013, Junio C Hamano wrote:\n>> jk@blackdown.de (Jürgen Kreileder) writes:\n>> \n>>> Fixes the encoding for several _plain actions and for text/* and */*+xml blobs. \n>>>\n>>> Signed-off-by: Jürgen Kreileder <jk@blackdown.de>\n>\n> I see that this patch does (or tries to do) two _independent_ things,\n> and should be split into at least two separate commits:\n\nAgreed.\n\n>>> ---\n>> \n>> Thanks, will queue but not hold until I hear something from Jakub.\n>> \n>>>  gitweb/gitweb.perl |    8 +++++++-\n>>>  1 file changed, 7 insertions(+), 1 deletion(-)\n>>>\n>>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>>> index 1309196..9cfe5b5 100755\n>>> --- a/gitweb/gitweb.perl\n>>> +++ b/gitweb/gitweb.perl\n>>> @@ -3823,7 +3823,7 @@ sub blob_contenttype {\n>>>  \tmy ($fd, $file_name, $type) = @_;\n>>>  \n>>>  \t$type ||= blob_mimetype($fd, $file_name);\n>>> -\tif ($type eq 'text/plain' && defined $default_text_plain_charset) {\n>>> +\tif (($type =~ m!^text/\\w[-\\w]*$! || $type =~ m!^\\w[-\\w]*/\\w[-\\w]*\\+xml$!) && defined $default_text_plain_charset) {\n>>>  \t\t$type .= \"; charset=$default_text_plain_charset\";\n>>>  \t}\n>\n> First, it extends adding \"; charset=$default_text_plain_charset\" to\n> other mimetypes for 'blob_plain' view to all 'text/*' and '*/*+xml'\n> mimetypes (without changing name of variable... though this is more\n> complicated as it is configuration variable and we would want to\n> preserve backward compatibility, but at least a comment would be,\n> I think, needed).\n>  \n> Originally it applied only to 'text/plain' files, which can be\n> displayed inline by web browser, and which need charset in\n> 'Content-Type:' HTTP header to be displayed correctly, as they\n> do not include such information inside the file.\n\nWhat prompted the change is that some browsers (Chrome and Safari) also\ndisplay other file types inline: text/x-chdr, text/x-java, text/x-objc,\ntext/x-sh, ...\n\nAt least on my system these files do get served with charset set!\n(ISO-8859-1 even though Apache has \"AddDefaultCharset UTF-8\"...)\n\n    $ curl -I \"https://git.blackdown.de/old.cgi?p=contactalbum.git;a=blob_plain;f=Classes/ContactAlbum.h;hb=HEAD\"\n    HTTP/1.1 200 OK\n    Date: Tue, 09 Apr 2013 21:47:30 GMT\n    Server: Apache\n    Content-disposition: inline; filename=\"Classes/ContactAlbum.h\"\n    X-Frame-Options: SAMEORIGIN\n    Vary: User-Agent\n    X-UA-Compatible: IE=edge\n    Strict-Transport-Security: max-age=31536000\n    Content-Type: text/x-chdr; charset=ISO-8859-1\n\n> 'text/html' and 'application/xhtml+xml' can include such information\n> inside them (meta http-equiv for 'text/html' and <?xml ...> for\n> 'application/xhtml+xml').  I don't know what browser does when there\n> is conflicting information about charset, i.e. which one wins, but\n> it is certainly something to consider.\n\nAs shown above, even without my patch, this already can happen!\n\n> It might be a good change; I don't know what web browser do when\n> serving 'text/css', 'text/javascript', 'text/xml' to client to\n> view without media type known.\n>\n>\n> BTW I have noticed that we do $prevent_xss dance outside\n> blob_contenttype(), in it's only caller i.e. git_blob_plain()...\n> which for example means that 'text/html' converted to 'text/plain'\n> don't get '; charset=...' added.  I guess that it *might* be\n> what prompted this part of change... but if it is so, it needs\n> to be fixed at source, e.g. by moving $prevent_xss to\n> blob_contenttype() subroutine.\n\nJep.\n\n[...]\n\n> Second it changes 'blobdiff_plain', 'commitdiff_plain' (which I think\n> that should be abandoned in favor of 'patch' view; but that is\n> a separate issue) and 'patch' views so they use binary-safe output.\n>\n> Note that in all cases (I think) we use\n>\n>    $cgi->header(\n>      -type => 'text/plain',\n>      -charset => 'utf-8',\n>      ...\n>    );\n>\n> promising web browser that output is as whole in 'utf-8' encoding.\n\nYes.\n\n> It is not explained in the commit message what is the reason for this\n> change.  Is it better handing of a situation where files being diff-ed\n> being in other encoding (like for example in commit that changes\n> encoding of files from legacy encoding such like e.g. iso-8859-2\n> or cp1250 to utf-8)?\n\nI do see encoding problems when comparing utf8 to utf8 files (i.e. no\nencoding change).  For instance:\nhttps://git.blackdown.de/old.cgi?p=contactalbum.git;a=commitdiff_plain;h=cc4eaa64c2b399dd9bdbf1f67f6d621aa24df5f8\n\nI don't claim to be an expert in Perl's utf8 handling but I guess this\nbecause Perl's internal utf8 form differs from the normal utf8 out from\ngit commands.  Switching to :raw fixes that: We write plain utf8 (and,\nas noted above, charset is set to utf8 already)\n\nWith the patch: https://git.blackdown.de/?p=contactalbum.git;a=commitdiff_plain;h=cc4eaa64c2b399dd9bdbf1f67f6d621aa24df5f8\n\n> But what about -charset => 'utf-8'  then?\n\nThat's a good question.  I think I never tried git with anything besides\nISO-8859-1 (rarely), UTF8 (mostly), and UTF16 (some Xcode files).\n(UTF16 definitely causes problems for gitweb.)\n\n> About implementation: I think after this change common code crosses\n> threshold for refactoring it into subroutine, for example:\n>\n>   sub dump_fh_raw {\n>   \tmy $fh = shift;\n>\n>   \tlocal $/ = undef;\n>   \tbinmode STDOUT, ':raw';\n>   \tprint <$fh>;\n>   \tbinmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi\n>\n>   \treturn $fh;\n>   }\n\nOK.\n\nI'll submit an updated patch for the second part ('blobdiff_plain',\n'commitdiff_plain', and 'plain') tomorrow.\n\nNot sure how to proceed with the 'blob_plain' issue.\n\n\n    Juergen\n\n== \nhttps://blackdown.de/\n"},{"id":"213762","messageId":"51653DAB.4070907@gmail.com","threadId":"33440","inReplyTo":"m28v4r9xgs.fsf@zahir.fritz.box","subject":"Re: [PATCH 1/4] gitweb: Fix utf8 encoding for blob_plain, blobdiff_plain, commitdiff_plain, and patch","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2013-04-10T10:23:39Z","receivedAt":"2013-04-10T10:23:39Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On 09.04.2013 at 23:59, Jürgen Kreileder wrote:\n> Jakub Narębski <jnareb@gmail.com> writes:\n> \n>> On 08.04.2013, Junio C Hamano wrote:\n>>> jk@blackdown.de (Jürgen Kreileder) writes:\n>>>\n>>>> Fixes the encoding for several _plain actions and for text/* and */*+xml blobs. \n>>>>\n>>>> Signed-off-by: Jürgen Kreileder <jk@blackdown.de>\n>>\n>> I see that this patch does (or tries to do) two _independent_ things,\n>> and should be split into at least two separate commits:\n[...]\n>> First, it extends adding \"; charset=$default_text_plain_charset\" to\n>> other mimetypes for 'blob_plain' view to all 'text/*' and '*/*+xml'\n>> mimetypes (without changing name of variable... though this is more\n>> complicated as it is configuration variable and we would want to\n>> preserve backward compatibility, but at least a comment would be,\n>> I think, needed).\n>>  \n>> Originally it applied only to 'text/plain' files, which can be\n>> displayed inline by web browser, and which need charset in\n>> 'Content-Type:' HTTP header to be displayed correctly, as they\n>> do not include such information inside the file.\n> \n> What prompted the change is that some browsers (Chrome and Safari) also\n> display other file types inline: text/x-chdr, text/x-java, text/x-objc,\n> text/x-sh, ...\n> \n> At least on my system these files do get served with charset set!\n> (ISO-8859-1 even though Apache has \"AddDefaultCharset UTF-8\"...)\n[...]\n\nI think this (the reason behind this change) should be explained in the\ncommit message.\n\n>> 'text/html' and 'application/xhtml+xml' can include such information\n>> inside them (meta http-equiv for 'text/html' and <?xml ...> for\n>> 'application/xhtml+xml').  I don't know what browser does when there\n>> is conflicting information about charset, i.e. which one wins, but\n>> it is certainly something to consider.\n> \n> As shown above, even without my patch, this already can happen!\n> \n>> It might be a good change; I don't know what web browser do when\n>> serving 'text/css', 'text/javascript', 'text/xml' to client to\n>> view without media type known.\n\nThere are few options on how to handle this.\n\nFirst, there is an issue of $default_text_plain_charset configuration\nvariable.  Any changes to its use (or adding new configuration\nvariables) should be accompanied with changing / adding comment near the\nplace where the default value is set, and changing / adding to\nthe documentation, namely gitweb.conf.txt\n\nWe can either:\n1.) Use $default_text_plain_charset variable both for 'text/plain'\n    and 'text/*' etc. content types in 'blob_plain' (aka 'raw') view\n    for backwards compatibility, just add comment that it applies more\n    than 'text/plain'\n\n2.) Add new configuration variable, e.g. $default_blob_plain_charset\n    and use it in place of $default_text_plain_charset as above,\n    initializing it to $default_text_plain_charset.\n\n    This practically renames $default_text_plain_charset preserving\n    backward compatibility.  Documentation would talk now about new\n    variable name.\n\n3.) Add new configuration variable, e.g. $default_text_other_charset,\n    or $default_blob_plain_charset, or $default_inline_charset, etc.\n    which will be used for files other than 'text/plain'.  This would\n    make it possible to turn this new feature on and off.\n\n3.a.) New feature default to on, i.e. to $default_text_plain_charset\n3.b.) New feature default to off, i.e. undef\n\n\nSecond, there is an issue of file types, like HTML or XML (or XHTML,\nor sometimes for LaTeX), which have the information about encoding\nembedded in file.\n\nWe can:\n\nA.) Skip this issue, and always add charset for 'text/*' and\n    '*/*-xml' files\n\nB.) Threat those few media types in special way, excluding them from\n    adding charset\n\nC.) Make which types to be added charset configurable.\n\n\nAnyway I think this feature is much less urgent.  It fixes an annoyance\nrather than bug, as you can always choose which charset to use to\ndisplay content in a browser.  We can take time to implement it well,\nespecially that it interacts with config (and backward compatibility\nof config variables).\n\n>> BTW I have noticed that we do $prevent_xss dance outside\n>> blob_contenttype(), in it's only caller i.e. git_blob_plain()...\n>> which for example means that 'text/html' converted to 'text/plain'\n>> don't get '; charset=...' added.  I guess that it *might* be\n>> what prompted this part of change... but if it is so, it needs\n>> to be fixed at source, e.g. by moving $prevent_xss to\n>> blob_contenttype() subroutine.\n> \n> Jep.\n\nBut I see this is yet another separate issue.\n\n>> Second it changes 'blobdiff_plain', 'commitdiff_plain' (which I think\n>> that should be abandoned in favor of 'patch' view; but that is\n>> a separate issue) and 'patch' views so they use binary-safe output.\n>>\n>> Note that in all cases (I think) we use\n>>\n>>    $cgi->header(\n>>      -type => 'text/plain',\n>>      -charset => 'utf-8',\n>>      ...\n>>    );\n>>\n>> promising web browser that output is as whole in 'utf-8' encoding.\n> \n> Yes.\n> \n>> It is not explained in the commit message what is the reason for this\n>> change.  Is it better handing of a situation where files being diff-ed\n>> being in other encoding (like for example in commit that changes\n>> encoding of files from legacy encoding such like e.g. iso-8859-2\n>> or cp1250 to utf-8)?\n> \n> I do see encoding problems when comparing utf8 to utf8 files (i.e. no\n> encoding change).\n\nAh, O.K.\n\n>  For instance:\n> https://git.blackdown.de/old.cgi?p=contactalbum.git;a=commitdiff_plain;h=cc4eaa64c2b399dd9bdbf1f67f6d621aa24df5f8\n\nNice to have online example, BTW.\n\n> I don't claim to be an expert in Perl's utf8 handling but I guess this\n> because Perl's internal utf8 form differs from the normal utf8 out from\n> git commands.  Switching to :raw fixes that: We write plain utf8 (and,\n> as noted above, charset is set to utf8 already)\n\nHmmm... I wonder if it is yet another case of concatenating content\nmarked as utf8 and not marked as utf8...\n\nAnyway this fact that it is workaround, or that it fixes symptoms\nof said issue, should be in the commit message (perhaps with TODO\nexplained).  This way we would be able to see it and perhaps fix\nunderlying issue, maybe with 'open' pragma, i.e.\n\n  use open IN => ':encoding(utf-8)';\n\n(though then we loose $fallback_encoding), or\n\n  use open IN => ':encoding(utf-8-with-fallback)'\n\n(which needs writing a Perl module for gitweb use).\n\n> With the patch: https://git.blackdown.de/?p=contactalbum.git;a=commitdiff_plain;h=cc4eaa64c2b399dd9bdbf1f67f6d621aa24df5f8\n> \n>> But what about -charset => 'utf-8'  then?\n> \n> That's a good question.  I think I never tried git with anything besides\n> ISO-8859-1 (rarely), UTF8 (mostly), and UTF16 (some Xcode files).\n> (UTF16 definitely causes problems for gitweb.)\n> \n>> About implementation: I think after this change common code crosses\n>> threshold for refactoring it into subroutine, for example:\n>>\n>>   sub dump_fh_raw {\n>>   \tmy $fh = shift;\n>>\n>>   \tlocal $/ = undef;\n>>   \tbinmode STDOUT, ':raw';\n>>   \tprint <$fh>;\n>>   \tbinmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi\n>>\n>>   \treturn $fh;\n>>   }\n> \n> OK.\n> \n> I'll submit an updated patch for the second part ('blobdiff_plain',\n> 'commitdiff_plain', and 'plain') tomorrow.\n\nThanks in advance.\n-- \nJakub Narębski\n"}]}