# The gitweb author initials feature from a36817b doesn't work with i18n names 11 messages from 2013-08-29 to 2013-08-31. Participants: Ævar Arnfjörð Bjarmason, Jakub Narębski, Simon Ruderich, Junio C Hamano, Kyle J. McKay. Thread: https://gitlist.dev/t/34798 ## Ævar Arnfjörð Bjarmason, 2013-08-29 14:26 Subject: The gitweb author initials feature from a36817b doesn't work with i18n names Message-ID: URL: https://gitlist.dev/e/CACBZZX4i-Tu1G07e9%3D5DYFjY9EtRGO8gLJf71d5aLeaE-cj-8w%40mail.gmail.com ``` The @author_initials feature Jakub added in a36817b claims to use a i18n regexp (/\b([[:upper:]])\B/g), but in Perl this doesn't actually do anything unless the string being matched against has the UTF8 flag. So as a result it abbreviates me to "AB" not "ÆAB". Here's something that demonstrates the issue: $ cat author-initials.pl #!/usr/bin/env perl use strict; use warnings; #binmode STDOUT, ':utf8'; open my $fd, "-|", "git", "blame", "--incremental", "--", "Makefile" or die "Can't open: $!"; #binmode $fd, ":utf8"; while (my $line = <$fd>) { next unless my ($author) = $line =~ /^author (.*)/; my @author_initials = ($author =~ /\b([[:upper:]])\B/g); printf "%s (%s)\n", join("", @author_initials), $author; } With those two binmode commands commented out: $ perl author-initials.pl |sort|uniq -c|sort -nr|head -n 5 99 JH (Junio C Hamano) 35 JN (Jonathan Nieder) 35 JK (Jeff King) 20 JS (Johannes Schindelin) 16 AB (Ævar Arnfjörð Bjarmason) And uncommented: $ perl author-initials.pl |sort|uniq -c|sort -nr|head -n 5 99 JH (Junio C Hamano) 35 JN (Jonathan Nieder) 35 JK (Jeff King) 20 JS (Johannes Schindelin) 16 ÆAB (Ævar Arnfjörð Bjarmason) Jakub, do you see a reason not to just apply this: diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index f429f75..29b3fb5 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -6631,6 +6631,7 @@ sub git_blame_common { $hash_base, '--', $file_name or die_error(500, "Open git-blame --porcelain failed"); } + binmode $fd, ":utf8"; # incremental blame data returns early if ($format eq 'data') { I haven't gotten an env where I can test gitweb running, but that looks like it should work to me. ``` ## Jakub Narębski, 2013-08-29 14:44 Subject: Re: The gitweb author initials feature from a36817b doesn't work with i18n names Message-ID: URL: https://gitlist.dev/e/CANQwDwcyV7N6xPxaYFUnRQptVabwkd_mFconZPGDm9iGS3LArA%40mail.gmail.com In-Reply-To: ``` [Ævar, sorry for duplication but I accidentally send HTML email; stupid Gmail] On Thu, Aug 29, 2013 at 4:26 PM, Ævar Arnfjörð Bjarmason wrote: > The @author_initials feature Jakub added in a36817b claims to use a > i18n regexp (/\b([[:upper:]])\B/g), but in Perl this doesn't actually > do anything unless the string being matched against has the UTF8 flag. > > So as a result it abbreviates me to "AB" not "ÆAB". [...] Thanks for catching this, and for an analysis. > Jakub, do you see a reason not to just apply this: > > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl > index f429f75..29b3fb5 100755 > --- a/gitweb/gitweb.perl > +++ b/gitweb/gitweb.perl > @@ -6631,6 +6631,7 @@ sub git_blame_common { > $hash_base, '--', $file_name > or die_error(500, "Open git-blame --porcelain failed"); > } > + binmode $fd, ":utf8"; > > # incremental blame data returns early > if ($format eq 'data') { > > I haven't gotten an env where I can test gitweb running, but that > looks like it should work to me. Unfortunetly I cannot check this either, but it looks obviously correct. ACK. -- Jakub Narebski ``` ## Simon Ruderich, 2013-08-29 16:39 Subject: Re: The gitweb author initials feature from a36817b doesn't work with i18n names Message-ID: <20130829163935.GA9689@ruderich.org> URL: https://gitlist.dev/e/20130829163935.GA9689%40ruderich.org In-Reply-To: ``` On Thu, Aug 29, 2013 at 04:26:29PM +0200, Ævar Arnfjörð Bjarmason wrote: > I haven't gotten an env where I can test gitweb running, but that > looks like it should work to me. I've tested the patch and it works fine. Tested-by: Simon Ruderich Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 ``` ## Ævar Arnfjörð Bjarmason, 2013-08-30 08:37 Subject: [PATCH] gitweb: Fix the author initials in blame for non-ASCII names Message-ID: <1377851821-5412-1-git-send-email-avarab@gmail.com> URL: https://gitlist.dev/e/1377851821-5412-1-git-send-email-avarab%40gmail.com In-Reply-To: <20130829163935.GA9689@ruderich.org> ``` Change the @author_initials feature Jakub added in v1.6.4-rc2-14-ga36817b to match non-ASCII author initials as intended. The regexp Jakub added was intended to match non-ASCII (/\b([[:upper:]])\B/g). But in Perl this doesn't actually match non-ASCII upper-case characters unless the string being matched against has the UTF8 flag. So when we open a pipe to "git blame" we need to mark the file descriptor we're opening as utf8 explicitly. So as a result it abbreviates me to "AB" not "ÆAB", entirely because "Æ" isn't /[[:upper:]]/ unless the string being matched against has the UTF8 flag. Here's something that demonstrates the issue: #!/usr/bin/env perl use strict; use warnings; binmode STDOUT, ':utf8' if $ENV{UTF8}; open my $fd, "-|", "git", "blame", "--incremental", "--", "Makefile" or die "Can't open: $!"; binmode $fd, ":utf8" if $ENV{UTF8}; while (my $line = <$fd>) { next unless my ($author) = $line =~ /^author (.*)/; my @author_initials = ($author =~ /\b([[:upper:]])\B/g); printf "%s (%s)\n", join("", @author_initials), $author; } When that's run with and without UTF8 being true in the environment it gives, on git.git: $ UTF8=0 perl author-initials.pl | sort | uniq -c | sort -nr | head -n 5 99 JH (Junio C Hamano) 35 JN (Jonathan Nieder) 35 JK (Jeff King) 20 JS (Johannes Schindelin) 16 AB (Ævar Arnfjörð Bjarmason) $ UTF8=1 perl author-initials.pl | sort | uniq -c | sort -nr | head -n 5 99 JH (Junio C Hamano) 35 JN (Jonathan Nieder) 35 JK (Jeff King) 20 JS (Johannes Schindelin) 16 ÆAB (Ævar Arnfjörð Bjarmason) Acked-by: Jakub Narębski Tested-by: Ævar Arnfjörð Bjarmason Tested-by: Simon Ruderich --- gitweb/gitweb.perl | 1 + 1 file changed, 1 insertion(+) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index f429f75..ad48a5a 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -6631,6 +6631,7 @@ sub git_blame_common { $hash_base, '--', $file_name or die_error(500, "Open git-blame --porcelain failed"); } + binmode $fh, ':utf8'; # incremental blame data returns early if ($format eq 'data') { -- 1.8.4.rc2 ``` ## Junio C Hamano, 2013-08-30 18:05 Subject: Re: [PATCH] gitweb: Fix the author initials in blame for non-ASCII names Message-ID: URL: https://gitlist.dev/e/xmqqppsvyrzj.fsf%40gitster.dls.corp.google.com In-Reply-To: <1377851821-5412-1-git-send-email-avarab@gmail.com> ``` Ævar Arnfjörð Bjarmason writes: > Acked-by: Jakub Narębski > Tested-by: Ævar Arnfjörð Bjarmason > Tested-by: Simon Ruderich > --- > gitweb/gitweb.perl | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl > index f429f75..ad48a5a 100755 > --- a/gitweb/gitweb.perl > +++ b/gitweb/gitweb.perl > @@ -6631,6 +6631,7 @@ sub git_blame_common { > $hash_base, '--', $file_name > or die_error(500, "Open git-blame --porcelain failed"); > } > + binmode $fh, ':utf8'; > > # incremental blame data returns early > if ($format eq 'data') { [Fri Aug 30 17:48:17 2013] gitweb.perl: Global symbol "$fh" requires explicit package name at /home/gitster/w/buildfarm/next/t/../gitweb/gitweb.perl line 6634. [Fri Aug 30 17:48:17 2013] gitweb.perl: Execution of /home/gitster/w/buildfarm/next/t/../gitweb/gitweb.perl aborted due to compilation errors. ``` ## Jakub Narębski, 2013-08-30 18:08 Subject: Re: [PATCH] gitweb: Fix the author initials in blame for non-ASCII names Message-ID: URL: https://gitlist.dev/e/CANQwDwcyTWgk0jhhGUgP-UZDLQ5EGhh5oueitGD5fpgS5bLtzw%40mail.gmail.com In-Reply-To: ``` On Fri, Aug 30, 2013 at 8:05 PM, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason writes: >> Acked-by: Jakub Narębski >> Tested-by: Ævar Arnfjörð Bjarmason >> Tested-by: Simon Ruderich >> --- >> gitweb/gitweb.perl | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl >> index f429f75..ad48a5a 100755 >> --- a/gitweb/gitweb.perl >> +++ b/gitweb/gitweb.perl >> @@ -6631,6 +6631,7 @@ sub git_blame_common { >> $hash_base, '--', $file_name >> or die_error(500, "Open git-blame --porcelain failed"); >> } >> + binmode $fh, ':utf8'; Should be $fd, not $fh. > [Fri Aug 30 17:48:17 2013] gitweb.perl: Global symbol "$fh" requires > explicit package name at /home/gitster/w/buildfarm/next/t/../gitweb/gitweb.perl line 6634. > [Fri Aug 30 17:48:17 2013] gitweb.perl: Execution of /home/gitster/w/buildfarm/next/t/../gitweb/gitweb.perl aborted due to compilation errors. I wonder how it passed Ævar and Simon tests. Perhaps proposal had $fd? -- Jakub Narebski ``` ## Junio C Hamano, 2013-08-30 18:13 Subject: Re: [PATCH] gitweb: Fix the author initials in blame for non-ASCII names Message-ID: URL: https://gitlist.dev/e/xmqqhae7yrls.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Junio C Hamano writes: > Ævar Arnfjörð Bjarmason writes: > >> Acked-by: Jakub Narębski >> Tested-by: Ævar Arnfjörð Bjarmason >> Tested-by: Simon Ruderich >> --- >> +++ b/gitweb/gitweb.perl >> @@ -6631,6 +6631,7 @@ sub git_blame_common { >> ... >> + binmode $fh, ':utf8'; > > [Fri Aug 30 17:48:17 2013] gitweb.perl: Global symbol "$fh" requires > explicit package name at /home/gitster/w/buildfarm/next/t/../gitweb/gitweb.perl line 6634. > [Fri Aug 30 17:48:17 2013] gitweb.perl: Execution of /home/gitster/w/buildfarm/next/t/../gitweb/gitweb.perl aborted due to compilation errors. I think in this function the filehandle is called $fd, not $fh. Has any of you really tested this??? ``` ## Simon Ruderich, 2013-08-30 18:31 Subject: Re: [PATCH] gitweb: Fix the author initials in blame for non-ASCII names Message-ID: <20130830183108.GA4240@ruderich.org> URL: https://gitlist.dev/e/20130830183108.GA4240%40ruderich.org In-Reply-To: ``` On Fri, Aug 30, 2013 at 11:13:19AM -0700, Junio C Hamano wrote: > I think in this function the filehandle is called $fd, not $fh. Has > any of you really tested this??? I did, but I applied the change by hand without applying the patch directly and didn't notice the difference. Sorry for that. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 ``` ## Kyle J. McKay, 2013-08-30 21:39 Subject: Re: [PATCH] gitweb: Fix the author initials in blame for non-ASCII names Message-ID: <74435A51-B404-4EC8-8CF8-6289F49E6DBD@gmail.com> URL: https://gitlist.dev/e/74435A51-B404-4EC8-8CF8-6289F49E6DBD%40gmail.com In-Reply-To: ``` On Aug 30, 2013, at 11:13, Junio C Hamano wrote: > Junio C Hamano writes: > >> Ævar Arnfjörð Bjarmason writes: >> >>> Acked-by: Jakub Narębski >>> Tested-by: Ævar Arnfjörð Bjarmason >>> Tested-by: Simon Ruderich >>> --- >>> +++ b/gitweb/gitweb.perl >>> @@ -6631,6 +6631,7 @@ sub git_blame_common { >>> ... >>> + binmode $fh, ':utf8'; > >> >> [Fri Aug 30 17:48:17 2013] gitweb.perl: Global symbol "$fh" requires >> explicit package name at /home/gitster/w/buildfarm/next/t/../gitweb/ >> gitweb.perl line 6634. >> [Fri Aug 30 17:48:17 2013] gitweb.perl: Execution of /home/gitster/ >> w/buildfarm/next/t/../gitweb/gitweb.perl aborted due to compilation >> errors. > > I think in this function the filehandle is called $fd, not $fh. Has > any of you really tested this??? What happens if the author name is written in ISO-8859-1 instead of UTF-8 in the actual commit object itself? I'm pretty sure I've seen this where older commits have a ISO-8859-1 author name and then newer commits have a UTF-8 version of the same author's name. In fact, in the git repository itself, look at commit 0cb3f80d (UTF-8) and commit 7eb93c89 (ISO-8859-1) to see this in action. ``` ## Ævar Arnfjörð Bjarmason, 2013-08-31 16:29 Subject: Re: [PATCH] gitweb: Fix the author initials in blame for non-ASCII names Message-ID: URL: https://gitlist.dev/e/CACBZZX4R2PRRRa%3DiqXQu_NhvFsTWM5%2Bktt-x4%3DsddvB_vptpqA%40mail.gmail.com In-Reply-To: ``` I did. I just clumsily sent out the wrong patch. I.e. tested it manually on another system, and then fat-fingered $fh instead of $fd. Should I send another patch or do you want to just fix this one up? On Fri, Aug 30, 2013 at 8:13 PM, Junio C Hamano wrote: > Junio C Hamano writes: > >> Ævar Arnfjörð Bjarmason writes: >> >>> Acked-by: Jakub Narębski >>> Tested-by: Ævar Arnfjörð Bjarmason >>> Tested-by: Simon Ruderich >>> --- >>> +++ b/gitweb/gitweb.perl >>> @@ -6631,6 +6631,7 @@ sub git_blame_common { >>> ... >>> + binmode $fh, ':utf8'; > >> >> [Fri Aug 30 17:48:17 2013] gitweb.perl: Global symbol "$fh" requires >> explicit package name at /home/gitster/w/buildfarm/next/t/../gitweb/gitweb.perl line 6634. >> [Fri Aug 30 17:48:17 2013] gitweb.perl: Execution of /home/gitster/w/buildfarm/next/t/../gitweb/gitweb.perl aborted due to compilation errors. > > I think in this function the filehandle is called $fd, not $fh. Has > any of you really tested this??? ``` ## Jakub Narębski, 2013-08-31 16:42 Subject: Re: [PATCH] gitweb: Fix the author initials in blame for non-ASCII names Message-ID: URL: https://gitlist.dev/e/CANQwDwdrmyMLZ299t4T-o6uGmfbHL_A83R37_zy0-jyFXEq1%2BA%40mail.gmail.com In-Reply-To: <74435A51-B404-4EC8-8CF8-6289F49E6DBD@gmail.com> ``` On Fri, Aug 30, 2013 at 11:39 PM, Kyle J. McKay wrote: > On Aug 30, 2013, at 11:13, Junio C Hamano wrote: >> Junio C Hamano writes: >>> Ævar Arnfjörð Bjarmason writes: >>>> + binmode $fh, ':utf8'; > What happens if the author name is written in ISO-8859-1 instead of UTF-8 in > the actual commit object itself? > > I'm pretty sure I've seen this where older commits have a ISO-8859-1 author > name and then newer commits have a UTF-8 version of the same author's name. > > In fact, in the git repository itself, look at commit 0cb3f80d (UTF-8) and > commit 7eb93c89 (ISO-8859-1) to see this in action. Well, then you have a problem, though it is only with old history (before introduction of "encoding" header in commit object). Better and more complete solution would be to use to_utf8() function instead of 'utf8' layer, which when finding invalid UTF-8 sequence uses $fallback_encoding (by default "latin1", i.e. ISO-8859-1) instead. In my TODO list is creating PerlIO layer ':utf8-with-fallback' which would replace all those to_utf8() calls... -- Jakub Narebski ```