patchgitweb: use decode_utf8 directly

> Hi,
>
> gitweb currently uses Encode::decode function with a wrapper like this :
>
> # very thin wrapper for decode("utf8", $str, Encode::FB_DEFAULT);
> sub to_utf8 {
>        my $str = shift;
>        return decode("utf8", $str, Encode::FB_DEFAULT);
> }
>
> But for me this gives the following error when I try to view RSS feed for
> Linux kernel GIT repo (local checkout) :
>
> Cannot decode string with wide characters
> at /usr/lib/perl5/vendor_perl/5.8.8/i686-linux/Encode.pm line 162.
>
> I Google'd a bit but the relevant information seems to be missing about
> this error. Anyhow there is no need for a wrapper at all as Encode class
> has a decode_utf8 function which fixes the problem I am experiencing too
> and chops off the unneeded wrapper.
>
> Patch against git 1.5.1.2 is attached. Comments welcome.
>
> P.S: I am using Encode 2.20 from CPAN which is the latest stable version
> available.

>> I Google'd a bit but the relevant information seems to be missing about
>> this error. Anyhow there is no need for a wrapper at all as Encode class
>> has a decode_utf8 function which fixes the problem I am experiencing too
>> and chops off the unneeded wrapper.
>>
>> Patch against git 1.5.1.2 is attached. Comments welcome.
>>
>> P.S: I am using Encode 2.20 from CPAN which is the latest stable version
>> available.
>
> Ping? This patch should be harmless and it fixes a real error, can it be 
> applied please?

> Ismail Dönmez <ismail@pardus.org.tr> writes:
> >> I Google'd a bit but the relevant information seems to be missing about
> >> this error. Anyhow there is no need for a wrapper at all as Encode class
> >> has a decode_utf8 function which fixes the problem I am experiencing too
> >> and chops off the unneeded wrapper.
> >>
> >> Patch against git 1.5.1.2 is attached. Comments welcome.
> >>
> >> P.S: I am using Encode 2.20 from CPAN which is the latest stable version
> >> available.
> >
> > Ping? This patch should be harmless and it fixes a real error, can it be
> > applied please?
>
> I cannot tell if it is harmless.  The original used
>
> 	decode("utf8", $str, Encode::FB_DEFAULT);
>
> and you made them to:
>
> 	decode_utf8($str);
>
> According to the documentation, decode_utf8($octets [,CHECK])
> should be equivalent to decode("utf8", $octets [,CHECK]), and
> the documentation further says that without CHECK, these
> functions assume Encode::FB_DEFAULT; in other words, these two
> should be equivalent.
>
> Which means that there is something else going on.  Your change
> may fix what you observed (I do not doubt that it fixed what you
> observed for you), but without understanding what really is
> going on (iow, why it is a fix, when the documentation clearly
> indicates they should be equivalent and it should not fix
> anything), we cannot tell what *ELSE* we are breaking with this
> change.

>> Which means that there is something else going on.  Your change
>> may fix what you observed (I do not doubt that it fixed what you
>> observed for you), but without understanding what really is
>> going on (iow, why it is a fix, when the documentation clearly
>> indicates they should be equivalent and it should not fix
>> anything), we cannot tell what *ELSE* we are breaking with this
>> change.
>
> That might be a bug in Encode itself indeed, I will dig a bit more. Thanks.

> Ismail Dönmez <ismail@pardus.org.tr> writes:
> >> Which means that there is something else going on.  Your change
> >> may fix what you observed (I do not doubt that it fixed what you
> >> observed for you), but without understanding what really is
> >> going on (iow, why it is a fix, when the documentation clearly
> >> indicates they should be equivalent and it should not fix
> >> anything), we cannot tell what *ELSE* we are breaking with this
> >> change.
> >
> > That might be a bug in Encode itself indeed, I will dig a bit more.
> > Thanks.
>
> Thanks.

> Ok found out the reason. decode() tries to decode data that is already UTF-8 
> and borks.
>
> This is from Encode.pm :
>
> sub decode_utf8($;$) {
>     my ( $str, $check ) = @_;
>     return $str if is_utf8($str); <--- Checks if the $str is already UTF-8
>     if ($check) {
>         return decode( "utf8", $str, $check ); <--- Else do what gitweb does
>     [...]
>
> So my patch is indeed correct.

> Ismail Dönmez <ismail@pardus.org.tr> writes:
> > Ok found out the reason. decode() tries to decode data that is already
> > UTF-8 and borks.
> >
> > This is from Encode.pm :
> >
> > sub decode_utf8($;$) {
> >     my ( $str, $check ) = @_;
> >     return $str if is_utf8($str); <--- Checks if the $str is already
> > UTF-8 if ($check) {
> >         return decode( "utf8", $str, $check ); <--- Else do what gitweb
> > does [...]
> >
> > So my patch is indeed correct.
>
> Ok, I think that makes it an improvement from the current code,
> so I'd apply.
>
> But at the same time I wonder why should the callers be feeding
> an already decoded string to to_utf8().  It might be that some
> callers needs fixing.

> On Wednesday 02 May 2007 00:39:34 you wrote:
> > Ismail Dönmez <ismail@pardus.org.tr> writes:
> > > Ok found out the reason. decode() tries to decode data that is already
> > > UTF-8 and borks.
> > >
> > > This is from Encode.pm :
> > >
> > > sub decode_utf8($;$) {
> > >     my ( $str, $check ) = @_;
> > >     return $str if is_utf8($str); <--- Checks if the $str is already
> > > UTF-8 if ($check) {
> > >         return decode( "utf8", $str, $check ); <--- Else do what gitweb
> > > does [...]
> > >
> > > So my patch is indeed correct.
> >
> > Ok, I think that makes it an improvement from the current code,
> > so I'd apply.
> >
> > But at the same time I wonder why should the callers be feeding
> > an already decoded string to to_utf8().  It might be that some
> > callers needs fixing.
>
> FWIW it was passing my name "İsmail Dönmez" based on user info I guess.

> Ismail Dönmez <ismail@pardus.org.tr> writes:
> > Ok found out the reason. decode() tries to decode data that is already
> > UTF-8 and borks.
> >
> > This is from Encode.pm :
> >
> > sub decode_utf8($;$) {
> >     my ( $str, $check ) = @_;
> >     return $str if is_utf8($str); <--- Checks if the $str is already
> > UTF-8 if ($check) {
> >         return decode( "utf8", $str, $check ); <--- Else do what gitweb
> > does [...]
> >
> > So my patch is indeed correct.
>
> Ok, I think that makes it an improvement from the current code,
> so I'd apply.
>
> But at the same time I wonder why should the callers be feeding
> an already decoded string to to_utf8().  It might be that some
> callers needs fixing.

>> But at the same time I wonder why should the callers be feeding
>> an already decoded string to to_utf8().  It might be that some
>> callers needs fixing.
>
> Is the patch OK do you want more investigation? Asking because its still not 
> in git.git.

> I would say that the patch is an improvement from the current
> code so it should hit 'master'; I was a bit busy lately and then
> am sick, and also we are post -rc1 freeze now and I was being
> cautious, just in case some nacks from more informed parties
> arrive late.

> Junio C Hamano <junkio@cox.net> writes:
> > I would say that the patch is an improvement from the current
> > code so it should hit 'master'; I was a bit busy lately and then
> > am sick, and also we are post -rc1 freeze now and I was being
> > cautious, just in case some nacks from more informed parties
> > arrive late.
>
> Sorry for the late nack, but it turns out that this patch breaks diff
> output on the Wine server for files that are not utf-8.

> The cause is apparently that decode_utf8() returns undef for invalid
> sequences instead of substituting a replacement char like
> decode("utf8") does.
>
> That may be considered an Encode bug since we are running a fairly old
> version (1.99, coming with Debian 3.1), but I'd rather not upgrade
> perl on the server. Could the patch be reverted, or done differently?

> Sorry for the late nack, but it turns out that this patch breaks diff
> output on the Wine server for files that are not utf-8.
>
> The cause is apparently that decode_utf8() returns undef for invalid
> sequences instead of substituting a replacement char like
> decode("utf8") does.

> Alexandre Julliard <julliard@winehq.org> writes:
> > Sorry for the late nack, but it turns out that this patch breaks diff
> > output on the Wine server for files that are not utf-8.
> >
> > The cause is apparently that decode_utf8() returns undef for invalid
> > sequences instead of substituting a replacement char like
> > decode("utf8") does.
>
> Thanks for noticing.  Will revert.

> On Friday 01 June 2007 22:44:36 Junio C Hamano wrote:
>> Alexandre Julliard <julliard@winehq.org> writes:
>> > Sorry for the late nack, but it turns out that this patch breaks diff
>> > output on the Wine server for files that are not utf-8.
>> >
>> > The cause is apparently that decode_utf8() returns undef for invalid
>> > sequences instead of substituting a replacement char like
>> > decode("utf8") does.
>>
>> Thanks for noticing.  Will revert.
>
> Why are reverting a correct bugfix? :( He's at most using outdated software. 
> *sigh*

> Ismail Dönmez <ismail@pardus.org.tr> writes:
> > On Friday 01 June 2007 22:44:36 Junio C Hamano wrote:
> >> Alexandre Julliard <julliard@winehq.org> writes:
> >> > Sorry for the late nack, but it turns out that this patch breaks diff
> >> > output on the Wine server for files that are not utf-8.
> >> >
> >> > The cause is apparently that decode_utf8() returns undef for invalid
> >> > sequences instead of substituting a replacement char like
> >> > decode("utf8") does.
> >>
> >> Thanks for noticing.  Will revert.
> >
> > Why are reverting a correct bugfix? :( He's at most using outdated
> > software. *sigh*
>
> I would assume that on top of a revert, with an additional
>
> 	return $str if is_utf8($str);
>
> to to_utf8() you should be able to fix both installations that
> has old or new Encode.pm?

> The cause is apparently that decode_utf8() returns undef for invalid
> sequences instead of substituting a replacement char like
> decode("utf8") does.
> 
> That may be considered an Encode bug since we are running a fairly old
> version (1.99, coming with Debian 3.1), but I'd rather not upgrade
> perl on the server. Could the patch be reverted, or done differently?