[PATCH] gitweb: convert from perl internal to utf8 for commitdiff_plain

[PATCH] gitweb: convert from perl internal to utf8 for commitdiff_plain

[Yasushi SHOJI]

commitdiff with raw, or plain format if you are reading the code,
doesn't convert any word from perl internal to utf8, which is set to
charset in http header.  this cause a problem when commit includes non
ascii code.

here is a few example in the git tree:

http://git.kernel.org/?p=git/git.git;a=commitdiff_plain;h=6ba78238a824282816944550edc4297dd2808a72
http://git.kernel.org/?p=git/git.git;a=commitdiff_plain;h=e360bebf713b6b03768c62de8b94ddf9350b0953
http://git.kernel.org/?p=git/git.git;a=commitdiff_plain;h=9459aa77a032621a29d53605542844641cca843a

This patch effectively revert the commitdiff plain part of the commit

	59b9f61a3f76762dc975e99cc05335a3b97ad1f9

which converted from print to here-doc. but it doesn't
explain why in the commit log.
---
 gitweb/gitweb.perl |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 6256641..5d9ac1d 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5048,16 +5048,16 @@ sub git_commitdiff {
 			-expires => $expires,
 			-content_disposition => 'inline; filename="' . "$filename" . '"');
 		my %ad = parse_date($co{'author_epoch'}, $co{'author_tz'});
-		print <<TEXT;
-From: $co{'author'}
-Date: $ad{'rfc2822'} ($ad{'tz_local'})
-Subject: $co{'title'}
-TEXT
+		print "From: " . to_utf8($co{'author'}) . "\n";
+		print "Date: " . to_utf8($ad{'rfc2822'}) . " "
+			       . to_utf8($ad{'tz_local'}) . "\n";
+		print "Subject: " . to_utf8($co{'title'}) . "\n";
+
 		print "X-Git-Tag: $tagname\n" if $tagname;
 		print "X-Git-Url: " . $cgi->self_url() . "\n\n";
 
 		foreach my $line (@{$co{'comment'}}) {
-			print "$line\n";
+			print to_utf8($line) . "\n";
 		}
 		print "---\n\n";
 	}
-- 
1.5.4.rc5.1.g0d200

Re: [PATCH] gitweb: convert from perl internal to utf8 for commitdiff_plain

[Ismail Dönmez]

At Tuesday 29 January 2008 around 04:14:00 Yasushi SHOJI wrote:
> commitdiff with raw, or plain format if you are reading the code,
> doesn't convert any word from perl internal to utf8, which is set to
> charset in http header.  this cause a problem when commit includes non
> ascii code.
>
> here is a few example in the git tree:
>
> http://git.kernel.org/?p=git/git.git;a=commitdiff_plain;h=6ba78238a82428281
>6944550edc4297dd2808a72
> http://git.kernel.org/?p=git/git.git;a=commitdiff_plain;h=e360bebf713b6b037
>68c62de8b94ddf9350b0953
> http://git.kernel.org/?p=git/git.git;a=commitdiff_plain;h=9459aa77a032621a2
>9d53605542844641cca843a
>
> This patch effectively revert the commitdiff plain part of the commit
>
> 	59b9f61a3f76762dc975e99cc05335a3b97ad1f9
>
> which converted from print to here-doc. but it doesn't
> explain why in the commit log.
> ---
>  gitweb/gitweb.perl |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 6256641..5d9ac1d 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -5048,16 +5048,16 @@ sub git_commitdiff {
>  			-expires => $expires,
>  			-content_disposition => 'inline; filename="' . "$filename" . '"');
>  		my %ad = parse_date($co{'author_epoch'}, $co{'author_tz'});
> -		print <<TEXT;
> -From: $co{'author'}
> -Date: $ad{'rfc2822'} ($ad{'tz_local'})
> -Subject: $co{'title'}
> -TEXT
> +		print "From: " . to_utf8($co{'author'}) . "\n";
> +		print "Date: " . to_utf8($ad{'rfc2822'}) . " "
> +			       . to_utf8($ad{'tz_local'}) . "\n";
> +		print "Subject: " . to_utf8($co{'title'}) . "\n";
> +
>  		print "X-Git-Tag: $tagname\n" if $tagname;
>  		print "X-Git-Url: " . $cgi->self_url() . "\n\n";
>
>  		foreach my $line (@{$co{'comment'}}) {
> -			print "$line\n";
> +			print to_utf8($line) . "\n";
>  		}
>  		print "---\n\n";
>  	}

Nice find, looks correct. You could print "Date" on one line instead of two, 
but FWIW

Acked-by: İsmail Dönmez <ismail@pardus.org.tr>

-- 
Never learn by your mistakes, if you do you may never dare to try again.

Re: [PATCH] gitweb: convert from perl internal to utf8 for commitdiff_plain

[Junio C Hamano]

Yasushi SHOJI <yashi@atmark-techno.com> writes:

> This patch effectively revert the commitdiff plain part of the commit
>
> 	59b9f61a3f76762dc975e99cc05335a3b97ad1f9
>
> which converted from print to here-doc. but it doesn't
> explain why in the commit log.

I think the patch makes sense but the above is misleading.  Read
it again.

Doesn't it sound like you are accusing that 59b9f61a introduced
a regression when it converted existsing "print utf8()" to
"print <<here-doc" without saying that is what it is doing?

I think the change in that commit was meant for writability (I
do not necessarily think here-doc is good for readability), and
I personally do not care---Perl is ugly to read either way ;-)

Re: [PATCH] gitweb: convert from perl internal to utf8 for commitdiff_plain

[Yasushi SHOJI]

At Mon, 28 Jan 2008 21:39:27 -0800,
Junio C Hamano wrote:
> 
> Yasushi SHOJI <yashi@atmark-techno.com> writes:
> 
> > This patch effectively revert the commitdiff plain part of the commit
> >
> > 	59b9f61a3f76762dc975e99cc05335a3b97ad1f9
> >
> > which converted from print to here-doc. but it doesn't
> > explain why in the commit log.
> 
> I think the patch makes sense but the above is misleading.  Read
> it again.
> 
> Doesn't it sound like you are accusing that 59b9f61a introduced
> a regression when it converted existsing "print utf8()" to
> "print <<here-doc" without saying that is what it is doing?

sorry about my stupid english.  What I meant was that because commit
log doesn't say _why_ it changed to here-doc, I couldn't be sure it
was ok to overwrite the change introduced by the commit 59b9f61a3.

IOW, I was tring to ask, "is it ok to revert back to print?"
-- 
         yashi

Re: [PATCH] gitweb: convert from perl internal to utf8 for commitdiff_plain

[Junio C Hamano]

Yasushi SHOJI <yashi@atmark-techno.com> writes:

> At Mon, 28 Jan 2008 21:39:27 -0800,
> Junio C Hamano wrote:
>> 
>> Yasushi SHOJI <yashi@atmark-techno.com> writes:
>> 
>> > This patch effectively revert the commitdiff plain part of the commit
>> >
>> > 	59b9f61a3f76762dc975e99cc05335a3b97ad1f9
>> >
>> > which converted from print to here-doc. but it doesn't
>> > explain why in the commit log.
>> 
>> I think the patch makes sense but the above is misleading.  Read
>> it again.
>> 
>> Doesn't it sound like you are accusing that 59b9f61a introduced
>> a regression when it converted existsing "print utf8()" to
>> "print <<here-doc" without saying that is what it is doing?
>
> sorry about my stupid english.  What I meant was that because commit
> log doesn't say _why_ it changed to here-doc, I couldn't be sure it
> was ok to overwrite the change introduced by the commit 59b9f61a3.
>
> IOW, I was tring to ask, "is it ok to revert back to print?"

Sure.  Can I forge your Sign-off?

Re: [PATCH] gitweb: convert from perl internal to utf8 for commitdiff_plain

[Jakub Narebski]

Yasushi SHOJI <yashi@atmark-techno.com> writes:

> commitdiff with raw, or plain format if you are reading the code,
> doesn't convert any word from perl internal to utf8, which is set to
> charset in http header.  this cause a problem when commit includes non
> ascii code.

Nice catch. Thanks.
 
> here is a few example in the git tree:
> 
> http://git.kernel.org/?p=git/git.git;a=commitdiff_plain;h=6ba78238a824282816944550edc4297dd2808a72
> http://git.kernel.org/?p=git/git.git;a=commitdiff_plain;h=e360bebf713b6b03768c62de8b94ddf9350b0953
> http://git.kernel.org/?p=git/git.git;a=commitdiff_plain;h=9459aa77a032621a29d53605542844641cca843a

...but commit message could be improved :-)

For example:

-- >8 --
gitweb: Convert generated contents to utf8 in commitdiff_plain

If the commit message, or commit author contains non-ascii, it must be
converted from Perl internal representation to utf-8, to follow what
got declared in HTTP header.  Use to_utf8() to do the conversion.

This necessarily replaces here-doc with "print" statements.

Signed-off-by: Yasushi SHOJI <yashi@atmark-techno.com>
Acked-by: İsmail Dönmez <ismail@pardus.org.tr>
Acked-by: Jakub Narebski <jnareb@gmail.com>
-- >8 --

> This patch effectively revert the commitdiff plain part of the commit
> 
> 	59b9f61a3f76762dc975e99cc05335a3b97ad1f9
> 
> which converted from print to here-doc. but it doesn't
> explain why in the commit log.

Sorry about that.

IMVHO using here-doc for longer sequence of output lines is more
readable than long "print" command, or sequence of print's. But of
course if you have to parse / transform some parts of output it is
simply not possible.

> ---
>  gitweb/gitweb.perl |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 6256641..5d9ac1d 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -5048,16 +5048,16 @@ sub git_commitdiff {
>  			-expires => $expires,
>  			-content_disposition => 'inline; filename="' . "$filename" . '"');
>  		my %ad = parse_date($co{'author_epoch'}, $co{'author_tz'});
> -		print <<TEXT;
> -From: $co{'author'}
> -Date: $ad{'rfc2822'} ($ad{'tz_local'})
> -Subject: $co{'title'}
> -TEXT
> +		print "From: " . to_utf8($co{'author'}) . "\n";
> +		print "Date: " . to_utf8($ad{'rfc2822'}) . " "
> +			       . to_utf8($ad{'tz_local'}) . "\n";

I think that date, or at least timezone would never have characters
outside US-ASCII, so to_uft8 is not really necessary, but I guess that
it is better to be safe than sorry.

> +		print "Subject: " . to_utf8($co{'title'}) . "\n";
> +
>  		print "X-Git-Tag: $tagname\n" if $tagname;
>  		print "X-Git-Url: " . $cgi->self_url() . "\n\n";
>  
>  		foreach my $line (@{$co{'comment'}}) {
> -			print "$line\n";
> +			print to_utf8($line) . "\n";
>  		}
>  		print "---\n\n";
>  	}

By the way, I guess that with new git we could just use --pretty=email
option to git-log / git-rev-list, and add X-Git-Tag and X-Git-Url at
the beginning (or insert it after headers).  Perhaps also generate
diff with the same diff command... but I think this improvement is to
be done _after_ release.

For what is worth:

Acked-by: Jakub Narebski <jnareb@gmail.com>

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH] gitweb: convert from perl internal to utf8 for commitdiff_plain

[Yasushi SHOJI]

At Mon, 28 Jan 2008 21:52:17 -0800,
Junio C Hamano wrote:
> 
> Yasushi SHOJI <yashi@atmark-techno.com> writes:
> 
> > At Mon, 28 Jan 2008 21:39:27 -0800,
> > Junio C Hamano wrote:
> >> 
> >> Yasushi SHOJI <yashi@atmark-techno.com> writes:
> >> 
> >> > This patch effectively revert the commitdiff plain part of the commit
> >> >
> >> > 	59b9f61a3f76762dc975e99cc05335a3b97ad1f9
> >> >
> >> > which converted from print to here-doc. but it doesn't
> >> > explain why in the commit log.
> >> 
> >> I think the patch makes sense but the above is misleading.  Read
> >> it again.
> >> 
> >> Doesn't it sound like you are accusing that 59b9f61a introduced
> >> a regression when it converted existsing "print utf8()" to
> >> "print <<here-doc" without saying that is what it is doing?
> >
> > sorry about my stupid english.  What I meant was that because commit
> > log doesn't say _why_ it changed to here-doc, I couldn't be sure it
> > was ok to overwrite the change introduced by the commit 59b9f61a3.
> >
> > IOW, I was tring to ask, "is it ok to revert back to print?"
> 
> Sure.  Can I forge your Sign-off?

From 127a6abaf23991394b3b2c5455c2522f9da1e8ac Mon Sep 17 00:00:00 2001
From: Yasushi SHOJI <yashi@wat.atmark-techno.com>
Date: Tue, 29 Jan 2008 20:48:33 +0900
Subject: [PATCH] gitweb: Convert generated contents to utf8 in commitdiff_plain
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit

If the commit message, or commit author contains non-ascii, it must be
converted from Perl internal representation to utf-8, to follow what
got declared in HTTP header.  Use to_utf8() to do the conversion.

This necessarily replaces here-doc with "print" statements.

Signed-off-by: Yasushi SHOJI <yashi@atmark-techno.com>
Acked-by: İsmail Dönmez <ismail@pardus.org.tr>
Acked-by: Jakub Narebski <jnareb@gmail.com>
---

Jakub, thanks for much better commit log.  I just copy & pasted to new
commit.

One thing I've changed from the last patch was, as Jakub and Ismail
pointed out, the Date line.  I've:

 - removed to_utf8(), and
 - made it one line

I also noticed that I've removed parentheses from the line by
accident, so I put them back.

Junio, please use this if you are ok.

Thanks,



 gitweb/gitweb.perl |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 6256641..80e3d0a 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5048,16 +5048,15 @@ sub git_commitdiff {
 			-expires => $expires,
 			-content_disposition => 'inline; filename="' . "$filename" . '"');
 		my %ad = parse_date($co{'author_epoch'}, $co{'author_tz'});
-		print <<TEXT;
-From: $co{'author'}
-Date: $ad{'rfc2822'} ($ad{'tz_local'})
-Subject: $co{'title'}
-TEXT
+		print "From: " . to_utf8($co{'author'}) . "\n";
+		print "Date: $ad{'rfc2822'} ($ad{'tz_local'})\n";
+		print "Subject: " . to_utf8($co{'title'}) . "\n";
+
 		print "X-Git-Tag: $tagname\n" if $tagname;
 		print "X-Git-Url: " . $cgi->self_url() . "\n\n";
 
 		foreach my $line (@{$co{'comment'}}) {
-			print "$line\n";
+			print to_utf8($line) . "\n";
 		}
 		print "---\n\n";
 	}
-- 
1.5.4.rc5.1.g0d200

Re: [PATCH] gitweb: convert from perl internal to utf8 for commitdiff_plain

[Jakub Narebski]

Yasushi SHOJI wrote:

> Junio, please use this if you are ok.

While we are now in feature freeze, I think this can be considered
a bugfix.

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: convert from perl internal to utf8 for commitdiff_plain

[Junio C Hamano]

Yasushi SHOJI <yashi@atmark-techno.com> writes:

>> > sorry about my stupid english.  What I meant was that because commi=
> t
>> > log doesn't say _why_ it changed to here-doc, I couldn't be sure it
>> > was ok to overwrite the change introduced by the commit 59b9f61a3.
>> >
>> > IOW, I was tring to ask, "is it ok to revert back to print?"
>>=20
>> Sure.  Can I forge your Sign-off?
>
> =46rom 127a6abaf23991394b3b2c5455c2522f9da1e8ac Mon Sep 17 00:00:00 200=
> 1
> =46rom: Yasushi SHOJI <yashi@wat.atmark-techno.com>
> Date: Tue, 29 Jan 2008 20:48:33 +0900
> Subject: [PATCH] gitweb: Convert generated contents to utf8 in commitdi=
> ff_plain
> MIME-Version: 1.0
> Content-Type: text/plain; charset=3Dutf-8
> Content-Transfer-Encoding: 8bit

Please don't do this.  MIME headers do not belong to the body of
the message.

>
> If the commit message, or commit author contains non-ascii, it must be
> converted from Perl internal representation to utf-8, to follow what
> got declared in HTTP header.  Use to_utf8() to do the conversion.
>
> This necessarily replaces here-doc with "print" statements.
>
> Signed-off-by: Yasushi SHOJI <yashi@atmark-techno.com>
> Acked-by: =C4=B0smail D=C3=B6nmez <ismail@pardus.org.tr>
> Acked-by: Jakub Narebski <jnareb@gmail.com>

Will try to apply by hand; if you do not hear from me about this
patch again, no need for resend.

Thanks.