threads / patch / 11777

patchgitweb: convert from perl internal to utf8 for commitdiff_plain

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

## tl;dr

9 messages between Jan 29, 2008 and Jan 30, 2008. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Yasushi SHOJI· Jan 29, 2008, 02:14 UTC · lore

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(-)

Show changes to gitweb/gitweb.perl +6 −6

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

Ismail Dönmez· Jan 29, 2008, 03:11 UTC · re: Yasushi SHOJI · lore

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

At Tuesday 29 January 2008 around 04:14:00 Yasushi SHOJI wrote:

Show 51 quoted lines

> 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.

Junio C Hamano· Jan 29, 2008, 05:39 UTC · re: Yasushi SHOJI · lore

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

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

Show 6 quoted lines

> 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 ;-)

Yasushi SHOJI· Jan 29, 2008, 05:45 UTC · re: Junio C Hamano · lore

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

At Mon, 28 Jan 2008 21:39:27 -0800, Junio C Hamano wrote:

Show 16 quoted lines

> 
> 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

Junio C Hamano· Jan 29, 2008, 05:52 UTC · re: Yasushi SHOJI · lore

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

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

Show 24 quoted lines

> 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?

Yasushi SHOJI· Jan 29, 2008, 12:16 UTC · re: Junio C Hamano · lore

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

At Mon, 28 Jan 2008 21:52:17 -0800, Junio C Hamano wrote:

Show 29 quoted lines

> 
> 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(-)

Show changes to gitweb/gitweb.perl +5 −6

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

Jakub Narebski· Jan 29, 2008, 12:40 UTC · re: Yasushi SHOJI · lore

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

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

Junio C Hamano· Jan 30, 2008, 05:10 UTC · re: Yasushi SHOJI · lore

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

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

Show 18 quoted lines

>> > 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.

Show 10 quoted lines

>
> 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.

Jakub Narebski· Jan 29, 2008, 11:09 UTC · re: Yasushi SHOJI · lore

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

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.

Show 5 quoted lines

> 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

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 --

Show 6 quoted lines

> 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.

Show 20 quoted lines

> ---
>  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.

Show 11 quoted lines

> +		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