gitweb: convert from perl internal to utf8 for commitdiff_plain

PATCH In review Yasushi SHOJI 4 participants 9 messages Jun 15, 2016

Share & tools

Yasushi SHOJI

author

2016-06-15 22:44 UTC #0 lore raw

PATCH 1/1 gitweb: convert from perl internal to utf8 for commitdiff_plain

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

gitweb/gitweb.perl +6 -6

index 6256641..5d9ac1d 100755
@@ -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";
 	}

Signature · 1 line

1.5.4.rc5.1.g0d200

Ismail Dönmez

Acked-by on v1 1/1

2016-06-15 22:44 UTC #1 lore raw

Quoted 41 lines · 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(-)
gitweb/gitweb.perl +4 -5
index 6256641..5d9ac1d 100755
@@ -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";

gitweb/gitweb.perl sub git_commitdiff { · line 5063

Quoted 10 lines

gitweb/gitweb.perl +2 -1

+
 		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 <[email protected]>

Signature · 1 line

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

Junio C Hamano

2016-06-15 22:44 UTC #2 lore raw

In reply to Yasushi SHOJI

Quoted 6 lines · Yasushi SHOJI

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

author

2016-06-15 22:44 UTC #3 lore raw

At Mon, 28 Jan 2008 21:39:27 -0800,

Quoted 10 lines · Junio C Hamano

Quoted 6 lines · Yasushi SHOJI
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.

Quoted 6 lines

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

Signature · 1 line

         yashi

Junio C Hamano

2016-06-15 22:44 UTC #4 lore raw

Quoted 19 lines · Yasushi SHOJI

At Mon, 28 Jan 2008 21:39:27 -0800,
Quoted 10 lines · Junio C Hamano
Quoted 6 lines · Yasushi SHOJI
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.
Quoted 6 lines
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?

Quoted 5 lines

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

author Acked-by İsmail Dönmez Acked-by Jakub Narebski

2016-06-15 22:44 UTC #5 lore raw

At Mon, 28 Jan 2008 21:52:17 -0800,

Quoted 22 lines · Junio C Hamano

Quoted 19 lines · Yasushi SHOJI
At Mon, 28 Jan 2008 21:39:27 -0800,
Quoted 16 lines · Junio C Hamano
Quoted 6 lines · Yasushi SHOJI
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?

Quoted 7 lines

Quoted 5 lines
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 <[email protected]>
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 <[email protected]>

Acked-by İsmail Dönmez <[email protected]>

Acked-by Jakub Narebski <[email protected]>

---

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

gitweb/gitweb.perl +5 -6

index 6256641..80e3d0a 100755
@@ -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";
 	}

Signature · 1 line

1.5.4.rc5.1.g0d200

Jakub Narebski

2016-06-15 22:44 UTC #6 lore raw

Quoted 1 line · Yasushi SHOJI

Junio, please use this if you are ok.

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

Signature · 2 lines

Jakub Narebski
Poland

Jakub Narebski

Acked-by İsmail Dönmez Acked-by Acked-by on v1 1/1

2016-06-15 22:44 UTC #8 lore raw

In reply to Yasushi SHOJI

Quoted 4 lines · 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.

Nice catch. Thanks.

Quoted 5 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 <[email protected]>

Acked-by İsmail Dönmez <[email protected]>

Acked-by Jakub Narebski <[email protected]>

-- >8 --

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

Quoted 10 lines

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

gitweb/gitweb.perl +0 -0

index 6256641..5d9ac1d 100755
@@ -5048,16 +5048,16 @@ sub git_commitdiff {
 			-expires => $expires,

gitweb/gitweb.perl sub git_commitdiff { · line 5053

Quoted 10 lines

gitweb/gitweb.perl +3 -5

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

gitweb/gitweb.perl sub git_commitdiff { · line 5063

Quoted 11 lines

gitweb/gitweb.perl +3 -1

+		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 <[email protected]>

Signature · 3 lines

Jakub Narebski
Poland
ShadeHawk on #git

Junio C Hamano

2016-06-15 22:44 UTC #7 lore raw

In reply to Yasushi SHOJI

Quoted 9 lines · Yasushi SHOJI

Quoted 1 line
Quoted 1 line
sorry about my stupid english. What I meant was that because commi=
t
Quoted 6 lines
Quoted 4 lines
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?

Quoted 9 lines

=46rom 127a6abaf23991394b3b2c5455c2522f9da1e8ac Mon Sep 17 00:00:00 200= 1
=46rom: Yasushi SHOJI <[email protected]>
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.

Quoted 10 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 <[email protected]>
Acked-by =C4=B0smail D=C3=B6nmez <[email protected]>
Acked-by Jakub Narebski <[email protected]>

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

Thanks.