Re: [PATCH/RFC] gitweb: New improved patchset view

Re: [PATCH/RFC] gitweb: New improved patchset view

[Jakub Narebski]

By the way, one of the reasons for change is that current view is ill 
prepared for "combined diff" format I'd like to add later.

P.S. This affects of course both commitdiff and blobdiff views.
-- 
Jakub Narebski

[PATCH/RFC] gitweb: New improved patchset view

[Jakub Narebski]

You should be able to check new commitdiff view at
  http://roke_DOT_dyndns_DOT_info/cgi-bin/gitweb/gitweb.cgi
(URL mangling courtesy vger banned words list, aaaarrghhh)
if I didn't screwed something again with firewall, and when my machine
is up; should be for at least an hour. Check for example:
  ?p=git.git;a=commitdiff;h=origin
  ?p=git.git;a=commitdiff;h=161332a521fe10c41979bcd493d95e2ac562b7f
  ?p=git.git;a=commitdiff;h=e12c095aa69d8aca0326eb11960427d9bf9e2db7
  ?p=git.git;a=commitdiff;h=82560983997c961d9deafe0074b787c8484c2e1d
and compare to (for example)
  http://repo.or.cz/w/git.git?a=commitdiff;h=82560983997c961d9deafe0074b787c8484c2e1d
or (even older gitweb)
  http://www.kernel.org/git/?p=git/git.git;a=commitdiff;h=887a612fef942dd3e7dae452e2dc582738b0fb41
BTW. this gitweb has also my previous "Slight improvement" patch applied.

Do you like it? What should be changed (code, output, style)?
-- >8 --
Replace "gitweb diff header" with its full sha1 of blobs with
"git diff" header and extended diff header. Change also
highlighting of diffs.

Changes:
* "gitweb diff header" which looked for example like below:
    file:_<sha1 before>_ -> file:_<sha1 after>_
  where 'file' is file type and '<sha1>' is full sha1 of blob, is link
  and uses default link style is changed to
    diff --git a/<file before> b/<file after>
  where <file> is hidden link (i.e. underline on hover, only)
  to appropriate version of file. If file is added, a/<file> is not
  hyperlinked, if file is deleted, b/<file> is not hyperlinked.
* there is added "extended diff header", with <path> and <hash>
  hyperlinked (and <hash> shortened to 7 characters), and <mode>
  explained: '<mode>' is extnded to '<mode>/<symbolic mode> (<file type>)'.
* <file> hyperlinking should work also when <file> is originally
  quoted. For now we present filename quoted. This needed changes to
  parse_difftree_raw_line subroutine.
* from-file/to-file two-line header lines have slightly darker color
  than removed/added lines.
* chunk header has now delicate line above for easier finding chunk
  boundary, and top margin of 1px.

Controversial ideas:
* All links in patch header are hidden
* Hashes are shortened to 7 characters
* Filenames are presented quoted
* Marking of chunk beginning
* No hyperlink for renamed from/to header (bug)

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.css  |   46 ++++++++++++---
 gitweb/gitweb.perl |  159 ++++++++++++++++++++++++++++++---------------------
 2 files changed, 131 insertions(+), 74 deletions(-)

diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index 83d900d..3aeceed 100644
--- a/gitweb/gitweb.css
+++ b/gitweb/gitweb.css
@@ -303,6 +303,33 @@ td.mode {
 	font-family: monospace;
 }
 
+
+div.diff.header,
+div.diff.extended_header {
+	white-space: normal;
+}
+
+div.diff.header {
+	font-weight: bold;
+
+	background-color: #edece6;
+
+	margin-top: 4px;
+	padding: 4px 0px 2px 0px;
+	border: solid #d9d8d1;
+	border-width: 1px 0px 1px 0px;
+}
+
+div.diff.extended_header,
+div.diff.extended_header a.list {
+	color: #777777;
+}
+
+div.diff.extended_header {
+	background-color: #f6f5ee;
+	padding: 2px 0px 2px 0px;
+}
+
 div.diff a.list {
 	text-decoration: none;
 }
@@ -312,31 +339,34 @@ div.diff a.list:hover {
 }
 
 div.diff.to_file a.list,
-div.diff.to_file,
+div.diff.to_file {
+	color: #007000;
+}
+
 div.diff.add {
 	color: #008800;
 }
 
 div.diff.from_file a.list,
-div.diff.from_file,
+div.diff.from_file {
+	color: #aa0000;
+}
+
 div.diff.rem {
 	color: #cc0000;
 }
 
 div.diff.chunk_header {
 	color: #990099;
+	border: dotted #ffbbff;
+	border-width: 1px 0px 0px 0px;
+	margin-top: 1px;
 }
 
 div.diff.incomplete {
 	color: #cccccc;
 }
 
-div.diff_info {
-	font-family: monospace;
-	color: #000099;
-	background-color: #edece6;
-	font-style: italic;
-}
 
 div.index_include {
 	border: solid #d9d8d1;
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index cbab3c9..2d971ac 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1255,9 +1255,12 @@ sub parse_difftree_raw_line {
 		$res{'status'} = $5;
 		$res{'similarity'} = $6;
 		if ($res{'status'} eq 'R' || $res{'status'} eq 'C') { # renamed or copied
-			($res{'from_file'}, $res{'to_file'}) = map { unquote($_) } split("\t", $7);
+			($res{'from_file_raw'}, $res{'to_file_raw'}) = split("\t", $7);
+			$res{'from_file'} = unquote($res{'from_file_raw'});
+			$res{'to_file'}   = unquote($res{'to_file_raw'});
 		} else {
-			$res{'file'} = unquote($7);
+			$res{'file_raw'} = $7;
+			$res{'file'} = unquote($res{'file_raw'});
 		}
 	}
 	# 'c512b523472485aef4fff9e57b229d9d243c967f'
@@ -2024,6 +2027,7 @@ sub git_patchset_body {
 	my $in_header = 0;
 	my $patch_found = 0;
 	my $diffinfo;
+	my (@from_subst, @to_subst);
 
 	print "<div class=\"patchset\">\n";
 
@@ -2033,6 +2037,7 @@ sub git_patchset_body {
 
 		if ($patch_line =~ m/^diff /) { # "git diff" header
 			# beginning of patch (in patchset)
+
 			if ($patch_found) {
 				# close previous patch
 				print "</div>\n"; # class="patch"
@@ -2042,11 +2047,59 @@ sub git_patchset_body {
 			}
 			print "<div class=\"patch\" id=\"patch". ($patch_idx+1) ."\">\n";
 
+			# read and prepare patch information
 			if (ref($difftree->[$patch_idx]) eq "HASH") {
+				# pre-parsed (or generated by hand)
 				$diffinfo = $difftree->[$patch_idx];
 			} else {
 				$diffinfo = parse_difftree_raw_line($difftree->[$patch_idx]);
 			}
+			if ($diffinfo->{'status'} ne "A") { # not new (added) file
+				my $quot = '';
+				my $from_text;
+				my $file_raw = $diffinfo->{'from_file_raw'} || $diffinfo->{'file_raw'};
+				if ($file_raw =~ s/^"(.*)"$/\1/) {
+					$from_text = qq("a/$file_raw");
+					$quot = '"';
+				} else {
+					$from_text =  qq(a/$file_raw);
+				}
+				my $file = $diffinfo->{'from_file'} || $diffinfo->{'file'};
+				my $from_link =
+					$cgi->a({-href => href(action=>"blob", hash_base=>$hash_base,
+					                      hash=>$diffinfo->{'from_id'}, file_name=>$file),
+					        -class => "list"}, esc_html($file_raw));
+				my $hash_link =
+					$cgi->a({-href => href(action=>"blob", hash_base=>$hash_base,
+					                      hash=>$diffinfo->{'to_id'}, file_name=>$file),
+					        -class => "list"}, substr($diffinfo->{'from_id'},0,7));
+				@from_subst = 
+					($from_text, "${quot}a/$from_link${quot}",
+					$diffinfo->{'from_id'} . '\.\.', "$hash_link..");
+			}
+			if ($diffinfo->{'status'} ne "D") { # not deleted file
+				my $quot = '';
+				my $to_text;
+				my $file_raw = $diffinfo->{'to_file_raw'} || $diffinfo->{'file_raw'};
+				if ($file_raw =~ s/^"(.*)"$/\1/) {
+					$to_text = qq("b/$file_raw");
+					$quot = '"';
+				} else {
+					$to_text =  qq(b/$file_raw);
+				}
+				my $file = $diffinfo->{'to_file'} || $diffinfo->{'file'};
+				my $to_link =
+					$cgi->a({-href => href(action=>"blob", hash_base=>$hash_base,
+					                      hash=>$diffinfo->{'to_id'}, file_name=>$file),
+					        -class => "list"}, esc_html($file_raw));
+				my $hash_link =
+					$cgi->a({-href => href(action=>"blob", hash_base=>$hash_base,
+					                      hash=>$diffinfo->{'to_id'}, file_name=>$file),
+					        -class => "list"}, substr($diffinfo->{'to_id'},0,7));
+				@to_subst =
+					($to_text, "${quot}b/$to_link${quot}",
+					 '\.\.' . $diffinfo->{'to_id'}, "..$hash_link");
+			}
 			$patch_idx++;
 
 			# for now we skip empty patches
@@ -2056,82 +2109,56 @@ sub git_patchset_body {
 				next LINE;
 			}
 
-			if ($diffinfo->{'status'} eq "A") { # added
-				print "<div class=\"diff_info\">" . file_type($diffinfo->{'to_mode'}) . ":" .
-				      $cgi->a({-href => href(action=>"blob", hash_base=>$hash,
-				                             hash=>$diffinfo->{'to_id'}, file_name=>$diffinfo->{'file'})},
-				              $diffinfo->{'to_id'}) . " (new)" .
-				      "</div>\n"; # class="diff_info"
-
-			} elsif ($diffinfo->{'status'} eq "D") { # deleted
-				print "<div class=\"diff_info\">" . file_type($diffinfo->{'from_mode'}) . ":" .
-				      $cgi->a({-href => href(action=>"blob", hash_base=>$hash_parent,
-				                             hash=>$diffinfo->{'from_id'}, file_name=>$diffinfo->{'file'})},
-				              $diffinfo->{'from_id'}) . " (deleted)" .
-				      "</div>\n"; # class="diff_info"
-
-			} elsif ($diffinfo->{'status'} eq "R" || # renamed
-			         $diffinfo->{'status'} eq "C" || # copied
-			         $diffinfo->{'status'} eq "2") { # with two filenames (from git_blobdiff)
-				print "<div class=\"diff_info\">" .
-				      file_type($diffinfo->{'from_mode'}) . ":" .
-				      $cgi->a({-href => href(action=>"blob", hash_base=>$hash_parent,
-				                             hash=>$diffinfo->{'from_id'}, file_name=>$diffinfo->{'from_file'})},
-				              $diffinfo->{'from_id'}) .
-				      " -> " .
-				      file_type($diffinfo->{'to_mode'}) . ":" .
-				      $cgi->a({-href => href(action=>"blob", hash_base=>$hash,
-				                             hash=>$diffinfo->{'to_id'}, file_name=>$diffinfo->{'to_file'})},
-				              $diffinfo->{'to_id'});
-				print "</div>\n"; # class="diff_info"
-
-			} else { # modified, mode changed, ...
-				print "<div class=\"diff_info\">" .
-				      file_type($diffinfo->{'from_mode'}) . ":" .
-				      $cgi->a({-href => href(action=>"blob", hash_base=>$hash_parent,
-				                             hash=>$diffinfo->{'from_id'}, file_name=>$diffinfo->{'file'})},
-				              $diffinfo->{'from_id'}) .
-				      " -> " .
-				      file_type($diffinfo->{'to_mode'}) . ":" .
-				      $cgi->a({-href => href(action=>"blob", hash_base=>$hash,
-				                             hash=>$diffinfo->{'to_id'}, file_name=>$diffinfo->{'file'})},
-				              $diffinfo->{'to_id'});
-				print "</div>\n"; # class="diff_info"
-			}
+			# print "git diff" header
+			$patch_line =~ s/$from_subst[0]/$from_subst[1]/ if @from_subst;
+			$patch_line =~ s/$to_subst[0]/$to_subst[1]/ if @to_subst;
+			print "<div class=\"diff header\">$patch_line</div>\n";
 
-			#print "<div class=\"diff extended_header\">\n";
+			print "<div class=\"diff extended_header\">\n";
 			$in_header = 1;
 			next LINE;
 		} # start of patch in patchset
 
+		if ($in_header) {
+			if ($patch_line !~ m/^---/) {
+				# match <path>
+				if ($patch_line =~ m|a/|) {
+					$patch_line =~ s/$from_subst[0]/$from_subst[1]/ if @from_subst;
+				}
+				if ($patch_line =~ m|b/|) {
+					$patch_line =~ s/$to_subst[0]/$to_subst[1]/ if @to_subst;
+				}
+				# match <mode>
+				if ($patch_line =~ m/\s(\d{6})$/) {
+					$patch_line .= '/' . mode_str($1) . ' (' . file_type($1) . ')';
+				}
+				# match <hash>
+				if ($patch_line =~ m/^index/) {
+					$patch_line =~ s/0{40}/'0' x 7/e;
+					$patch_line =~ s/$from_subst[2]/$from_subst[3]/ if @from_subst;
+					$patch_line =~ s/$to_subst[2]/$to_subst[3]/ if @to_subst;
+				}
+				print $patch_line . "<br/>\n";
 
-		if ($in_header && $patch_line =~ m/^---/) {
-			#print "</div>\n"; # class="diff extended_header"
-			$in_header = 0;
+			} else {
+				#$patch_line =~ m/^---/;
+				print "</div>\n"; # class="diff extended_header"
+				$in_header = 0;
+
+				$patch_line =~ s/$from_subst[0]/$from_subst[1]/ if @from_subst;
+				print "<div class=\"diff from_file\">$patch_line</div>\n";
 
-			my $file = $diffinfo->{'from_file'};
-			$file  ||= $diffinfo->{'file'};
-			$file = $cgi->a({-href => href(action=>"blob", hash_base=>$hash_parent,
-			                               hash=>$diffinfo->{'from_id'}, file_name=>$file),
-			                -class => "list"}, esc_html($file));
-			$patch_line =~ s|a/.*$|a/$file|g;
-			print "<div class=\"diff from_file\">$patch_line</div>\n";
+				$patch_line = <$fd>;
+				chomp $patch_line;
 
-			$patch_line = <$fd>;
-			chomp $patch_line;
+				#$patch_line =~ m/^+++/;
+				$patch_line =~ s/$to_subst[0]/$to_subst[1]/ if @to_subst;
+				print "<div class=\"diff to_file\">$patch_line</div>\n";
 
-			#$patch_line =~ m/^+++/;
-			$file    = $diffinfo->{'to_file'};
-			$file  ||= $diffinfo->{'file'};
-			$file = $cgi->a({-href => href(action=>"blob", hash_base=>$hash,
-			                               hash=>$diffinfo->{'to_id'}, file_name=>$file),
-			                -class => "list"}, esc_html($file));
-			$patch_line =~ s|b/.*|b/$file|g;
-			print "<div class=\"diff to_file\">$patch_line</div>\n";
+			}
 
 			next LINE;
 		}
-		next LINE if $in_header;
 
 		print format_diff_line($patch_line);
 	}
-- 
1.4.3.3



-------------------------------------------------------

-- 
Jakub Narebski

Re: [PATCH/RFC] gitweb: New improved patchset view

[Jakub Narebski]

I had to resend the patch due to "DOT info" being on list of banned 
words...

-- 
Jakub Narebski

Re: [PATCH/RFC] gitweb: New improved patchset view

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Changes:
> * "gitweb diff header" which looked for example like below:
>     file:_<sha1 before>_ -> file:_<sha1 after>_
>   where 'file' is file type and '<sha1>' is full sha1 of blob, is link
>   and uses default link style is changed to
>     diff --git a/<file before> b/<file after>
>   where <file> is hidden link (i.e. underline on hover, only)
>   to appropriate version of file. If file is added, a/<file> is not
>   hyperlinked, if file is deleted, b/<file> is not hyperlinked.

I do not have time to look at the code right now, but here are 
quick comments on the output.

I personally do not mind "hidden" but it might be more obvious
to make them normal links -- the filenames in the header are not
part of long text people need to "read".  On the other hand,
it feels a bit wasteful to have these hidden links both on "diff
--git" line *and* ---/+++ lines (these three are very close to
each other).

> * there is added "extended diff header", with <path> and <hash>
>   hyperlinked (and <hash> shortened to 7 characters), and <mode>
>   explained: '<mode>' is extnded to '<mode>/<symbolic mode> (<file type>)'.

It somehow feels that deviating from what "git diff" gives makes
it very distracting; I would feel better if "/-rw-r--r-- (file)"
were not there.

Also I think arguing over 7 or 8 hexdigits is pointless; if you
are reading this from "git diff", it is probably the easiest to
match what "git diff" gave you.  One thing we _might_ want to do
in the future is to change "git diff" to use DEFAULT_ABBREV
hexdigits at the minimum but more if needed to disambiguate; I
think it currently does not do the "more if needed" part.

> * <file> hyperlinking should work also when <file> is originally
>   quoted. For now we present filename quoted. This needed changes to
>   parse_difftree_raw_line subroutine.

This feels Ok.

> * from-file/to-file two-line header lines have slightly darker color
>   than removed/added lines.

This visually feels right.

> * chunk header has now delicate line above for easier finding chunk
>   boundary, and top margin of 1px.

This visually feels right.

Re: [PATCH/RFC] gitweb: New improved patchset view

[Jakub Narebski]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> Changes:
>> * "gitweb diff header" which looked for example like below:
>>     file:_<sha1 before>_ -> file:_<sha1 after>_
>>   where 'file' is file type and '<sha1>' is full sha1 of blob, is link
>>   and uses default link style is changed to
>>     diff --git a/<file before> b/<file after>
>>   where <file> is hidden link (i.e. underline on hover, only)
>>   to appropriate version of file. If file is added, a/<file> is not
>>   hyperlinked, if file is deleted, b/<file> is not hyperlinked.
> 
> I do not have time to look at the code right now, but here are 
> quick comments on the output.

Code is a bit horrible I think in trying to be smart and avoid
code repetition.

> I personally do not mind "hidden" but it might be more obvious
> to make them normal links -- the filenames in the header are not
> part of long text people need to "read".  On the other hand,
> it feels a bit wasteful to have these hidden links both on "diff
> --git" line *and* ---/+++ lines (these three are very close to
> each other).

Good idea. We have lost visible links with the change. And easy to
implement: just remove style override for .diff.header a.list from
CSS (or what would be better but harder to code remove "list" class
from <a> element in "git header").

Originally there was visible link with full sha1 of blob as text,
and hidden link with filename as text; perhaps dropping the latter
---/+++ line link is better idea (here hyperlink should I think be
hidden, if there is any, to match git-diff and GNU diff -u output).
And this would simplify code somewhat...

The hidden (but here I think we can change to not-hidden links) links
in the "index" extended header line are to stay I think: this is the
only place where we can have links to all the versions of file in the
future combined commitdiff format.

>> * there is added "extended diff header", with <path> and <hash>
>>   hyperlinked (and <hash> shortened to 7 characters), and <mode>
>>   explained: '<mode>' is extnded to '<mode>/<symbolic mode> (<file type>)'.
> 
> It somehow feels that deviating from what "git diff" gives makes
> it very distracting; I would feel better if "/-rw-r--r-- (file)"
> were not there.

Well, the old version had "(<file type>)" in gitweb diff header.
Not all git/gitweb users are familiar with POSIX numeric modes;
perhaps changing the style (color for example) of added stuff
to mark it as added would be enough?

> Also I think arguing over 7 or 8 hexdigits is pointless; if you
> are reading this from "git diff", it is probably the easiest to
> match what "git diff" gave you.  One thing we _might_ want to do
> in the future is to change "git diff" to use DEFAULT_ABBREV
> hexdigits at the minimum but more if needed to disambiguate; I
> think it currently does not do the "more if needed" part.

I used 7 hexdigits because git-diff uses 7 by default.

>> * <file> hyperlinking should work also when <file> is originally
>>   quoted. For now we present filename quoted. This needed changes to
>>   parse_difftree_raw_line subroutine.
> 
> This feels Ok.

I'm not sure if we shouldn't unquote filename always, or at least
remove double quotes surrounding filename, or [ab]/filename. The decision
to leave visible filename quoted was based on the troubles to code
it correctly in "git diff" header where one of the filenames might
not be hyperlinked: if file was added or deleted.

>> * from-file/to-file two-line header lines have slightly darker color
>>   than removed/added lines.
> 
> This visually feels right.

What is left is fine-tuning the color.

>> * chunk header has now delicate line above for easier finding chunk
>>   boundary, and top margin of 1px.
> 
> This visually feels right.

What is left is fine-tuning the line and vertical space.
-- 
Jakub Narebski

Re: [PATCH/RFC] gitweb: New improved patchset view

[Anand Kumria]

On Sun, 29 Oct 2006 11:22:30 +0100, Jakub Narebski wrote:

> You should be able to check new commitdiff view at
>   http://roke_DOT_dyndns_DOT_info/cgi-bin/gitweb/gitweb.cgi
> (URL mangling courtesy vger banned words list, aaaarrghhh)
> if I didn't screwed something again with firewall, and when my machine
> is up; should be for at least an hour. Check for example:
>   ?p=git.git;a=commitdiff;h=origin
>   ?p=git.git;a=commitdiff;h=161332a521fe10c41979bcd493d95e2ac562b7f
>   ?p=git.git;a=commitdiff;h=e12c095aa69d8aca0326eb11960427d9bf9e2db7
>   ?p=git.git;a=commitdiff;h=82560983997c961d9deafe0074b787c8484c2e1d
> and compare to (for example)
>   http://repo.or.cz/w/git.git?a=commitdiff;h=82560983997c961d9deafe0074b787c8484c2e1d
> or (even older gitweb)
>   http://www.kernel.org/git/?p=git/git.git;a=commitdiff;h=887a612fef942dd3e7dae452e2dc582738b0fb41
> BTW. this gitweb has also my previous "Slight improvement" patch applied.
> 
> Do you like it? What should be changed (code, output, style)?

I think your version looks a lot nicer but I believe that the commit
message area should be in a fixed-point font. In general it won't be a
problem but if anyone should choose to insert a diagram or something else
that assume a fixed-width display - it won't look good.

Anand

Re: [PATCH/RFC] gitweb: New improved patchset view

[Jakub Narebski]

Jakub Narebski wrote:
> Junio C Hamano wrote:
>> Jakub Narebski <jnareb@gmail.com> writes:
>>>
>>> * there is added "extended diff header", with <path> and <hash>
>>>   hyperlinked (and <hash> shortened to 7 characters), and <mode>
>>>   explained: '<mode>' is extnded to '<mode>/<symbolic mode> (<file type>)'.
>> 
>> It somehow feels that deviating from what "git diff" gives makes
>> it very distracting; I would feel better if "/-rw-r--r-- (file)"
>> were not there.
> 
> Well, the old version had "(<file type>)" in gitweb diff header.
> Not all git/gitweb users are familiar with POSIX numeric modes;
> perhaps changing the style (color for example) of added stuff
> to mark it as added would be enough?

Or we could use the following code:

  <abbr title="100755/-rwxr-xr-x (file)">100755</abbr>

which gives explanation on mouse-over.
-- 
Jakub Narebski

Re: [PATCH/RFC] gitweb: New improved patchset view

[Jakub Narebski]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> Changes:
[...]
>> * <file> hyperlinking should work also when <file> is originally
>>   quoted. For now we present filename quoted. This needed changes to
>>   parse_difftree_raw_line subroutine.
> 
> This feels Ok.

Emphasisis on "should". Check
  http://roke(.)dyndns(.)info/cgi-bin/gitweb/gitweb.cgi?p=git.git;a=commitdiff;h=gitweb/test

By the way, should we use quoted or unquoted filename?
-- 
Jakub Narebski

Re: [PATCH/RFC] gitweb: New improved patchset view

[Jakub Narebski]

A couple of questions regarding new patchset/diff look for gitweb.
Currently patch starts with "git diff" header

  diff --git a/file1 b/file2

then we have a couple of extended header lines

  old|new|deleted file|new file mode <mode>
  copy|rename from|to <path>
  (dis)?similarity index <percent>
  index <hash>..<hash> [<mode>]

then we have two-line from-file/to-file header

  --- a/file1
  +++ b/file2

then patch itself.


1. Which parts to convert to hyperlinks, and which to do not? Which
   links have visible and which hidden (underline on mouseover, the same
   color as neighbouring text)?

   I think that a/file1 in "git diff" header should be turned into
   visible hyperlink unless file is created, and b/file2 should be link
   unless file is deleted.

   Also both <hash>-es in "index" extended header lines should be turned
   into links, as it is the only way to have hyperlink to all previous
   versions of the file in the case of "combined diff" format (to be
   added later). The question is if those hyperlinks should be visible;
   I don't have compelling reason against. Should we use title attribute
   to give filename perhaps, or is it unnecessary?

   Currently file1 and file2 in "--- a/file1" and "+++ b/file2" are
   turned into hidden links. Should we leave it, or should we remove
   this link as we have similar link just above? If we decide to have
   this link, should we also hyperlink <path> in "copy|rename" extended
   header line?

2. Use quoted or unquoted filename, remove or leave surrounding quotes
   in quoted filename? Should we unquote the not hyperlinked filename
   in the case of creation/deletion? What should be span of link:
     "a/_file1_", "_a/file1_", _"a/file1"_
     "_file_", _"file"_ 
   (where '_' marks beginning and end of link) for quoted filenames?
   What should be span of link for unquoted filenames:
      a/_file_, _a/file_
   Currently gitweb uses a/_file_ in ---/+++ line.

3. How (and if) to explain numerical mode: the currently used
   <mode>/<symbolic mode> (<file type>) e.g. 100755/-rwxr-xr-x (file).
   Or <mode> (<file type>) should be enough? Should we mark the addition
   compared to git-diff output? Or should we explain <mode> only on
   mouseover, using for example:
     <abbr title="100644/-rw-r--r-- (file)">100644</abbr>
   or just
     <abbr title="executable file">100755</abbr>?
-- 
Jakub Narebski

Re: [PATCH/RFC] gitweb: New improved patchset view

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> A couple of questions regarding new patchset/diff look for gitweb.
> Currently patch starts with "git diff" header
>
>   diff --git a/file1 b/file2
>
> then we have a couple of extended header lines
>
>   old|new|deleted file|new file mode <mode>
>   copy|rename from|to <path>
>   (dis)?similarity index <percent>
>   index <hash>..<hash> [<mode>]
>
> then we have two-line from-file/to-file header
>
>   --- a/file1
>   +++ b/file2
>
> then patch itself.

Before answering 3 questions, I think we need to ask a bigger
question.  What is the primary target audience of gitweb?

If it is for git-uninitiated, then staying as close to what GNU
diff would give would be a better idea.  I would say we at least
can assume that the user has some familiarity with SCM, and
knows what kind of information is kept track of and is shown as
differences between versions, and what files, directories and
symlinks are, but not how git represents these.  On the other
hand, if the user uses git to track a project (not necessarily
the project the user is looking at with gitweb) and is familiar
with the way git-diff presents these information, deviating from
git-diff output is distractiing.

At least to me /-rw-rw-... part made me feel uneasy for that
reason.

WIth that in mind, I'll think aloud what I would like if I were
not familiar with git:

 * "diff --git a/file b/file" would not use /dev/null but
   ---/+++ does.  If the former is shown as link, it should be
   visible which side is a link and which side is not for
   creation or deletion diff; otherwise you would need a second
   to realize it is not a bug that clicking on a/file on the
   "diff --git" line did not show anything for a creation diff.

 * I think showing object names in "index xxx..yyy mode" line is
   not very useful to humans (they are added for tools).  I do
   agree that we would want some clickable handle in combined
   diff output, but people not familiar with git would not know
   that "index xxx,yyy..zzz" is where you would find the
   parents, so that line needs to be munged anyway.

   Side note: Even though some git people (Luben, for example)
   claim they recognize some abbreviated object names, I suspect
   that are mostly recent commits and not blobs.

 * Mode on the "index" line may be useful, but as you say 100644
   is probably too git specific; however if our audience is
   git-uninitiated, I doubt -rw-r--r-- is much better (it is
   UNIXism, which I personally do not mind).  Spelling them out
   like "regular file", "executable file", or "symbolic link"
   might be more readable.  And spelling them out is more
   technically correct; when git says 100644, it does _not_ mean
   "-rw-r--r--", but it means "regular file, not executable"
   (IOW, we borrowed the octal bits from POSIX but it means
   slightly a different thing).

 * So the above two together suggests that I probably would want
   to see something like:

	diff --git a/file b/file (regular file)

   with a/file and b/file as visible links for one parent diff.
   For combined,

        diff --cc a/file b/file (regular file)

   with perhaps a/file be a pop-up window that has a list of
   links to parent blobs (b/file is a link to the merge result
   blob), or maybe 

        diff --cc a/file (parent1 parent2) b/file (regular file)

   where parent#N is a link to the blob in the parent (a/file is
   not clickable, b/file is the link to the result blob).

 * I think redundant links to blobs in extended headers (rename
   from ... to ...) and ---/+++ lines are Ok, but as other
   redundant links they probably are better kept half-hidden
   (i.e. react to mouse-over like commit titles) to avoid visual
   distraction.

 * I think the filename quoting is better left as-is, since it
   is a way to indicate something funny is going on. 

   One thing I noticed while browsing gitweb/ part is that we
   quote byte values above 128; we might want to change
   quote_c_style_counted() to either use unsigned char to avoid
   this, or explicitly use signed char to keep this across
   platforms (char could be unsigned char).

Re: [PATCH/RFC] gitweb: New improved patchset view

[Luben Tuikov]

--- Jakub Narebski <jnareb@gmail.com> wrote:
> Changes:
> * "gitweb diff header" which looked for example like below:
>     file:_<sha1 before>_ -> file:_<sha1 after>_
>   where 'file' is file type and '<sha1>' is full sha1 of blob, is link
>   and uses default link style is changed to
>     diff --git a/<file before> b/<file after>
>   where <file> is hidden link (i.e. underline on hover, only)
>   to appropriate version of file. If file is added, a/<file> is not
>   hyperlinked, if file is deleted, b/<file> is not hyperlinked.

I like it and I like the "hidden" links.

> * there is added "extended diff header", with <path> and <hash>
>   hyperlinked (and <hash> shortened to 7 characters), and <mode>
>   explained: '<mode>' is extnded to '<mode>/<symbolic mode> (<file type>)'.

I like that too, but would leave "100644/-rw-r--r-- (file)" out.

There is a bug in the code: the first index link is the same
as the second, while the first should upload the sha-7 it points to.
For example:
index 743f02b..c821e22
Both point to c821e22.  Please fix that.

> * <file> hyperlinking should work also when <file> is originally
>   quoted. For now we present filename quoted. This needed changes to
>   parse_difftree_raw_line subroutine.

I didn't see where you've quoted file names, but I'm a bit hesitant
to quote anything unnecessarily in the visual output.

> * from-file/to-file two-line header lines have slightly darker color
>   than removed/added lines.

Good.

> * chunk header has now delicate line above for easier finding chunk
>   boundary, and top margin of 1px.

Good.

> Controversial ideas:
> * All links in patch header are hidden

Love those hidden gems.

> * Hashes are shortened to 7 characters

That's ok as long as the output is consisent with real git diff.
That is, a user using git can recognize this commitdiff output and a user
using this gitweb commitdiff output can recognize a git diff output.

> * Filenames are presented quoted

I wouldn't do that.

> * Marking of chunk beginning

I like the fine lines and their color as it is shown in your server.

> * No hyperlink for renamed from/to header (bug)

I'm sure you'll fix that.

Overall I like this patch.  Why?  Because it makes gitweb commitdiff
output as similar as possible to git-diff output. This is always a good
thing, since both beginners and advanced git users can recognize one
or the other depending where they are coming from (gitweb or git).

   Luben


> 
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> ---
>  gitweb/gitweb.css  |   46 ++++++++++++---
>  gitweb/gitweb.perl |  159 ++++++++++++++++++++++++++++++---------------------
>  2 files changed, 131 insertions(+), 74 deletions(-)
> 
> diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
> index 83d900d..3aeceed 100644
> --- a/gitweb/gitweb.css
> +++ b/gitweb/gitweb.css
> @@ -303,6 +303,33 @@ td.mode {
>  	font-family: monospace;
>  }
>  
> +
> +div.diff.header,
> +div.diff.extended_header {
> +	white-space: normal;
> +}
> +
> +div.diff.header {
> +	font-weight: bold;
> +
> +	background-color: #edece6;
> +
> +	margin-top: 4px;
> +	padding: 4px 0px 2px 0px;
> +	border: solid #d9d8d1;
> +	border-width: 1px 0px 1px 0px;
> +}
> +
> +div.diff.extended_header,
> +div.diff.extended_header a.list {
> +	color: #777777;
> +}
> +
> +div.diff.extended_header {
> +	background-color: #f6f5ee;
> +	padding: 2px 0px 2px 0px;
> +}
> +
>  div.diff a.list {
>  	text-decoration: none;
>  }
> @@ -312,31 +339,34 @@ div.diff a.list:hover {
>  }
>  
>  div.diff.to_file a.list,
> -div.diff.to_file,
> +div.diff.to_file {
> +	color: #007000;
> +}
> +
>  div.diff.add {
>  	color: #008800;
>  }
>  
>  div.diff.from_file a.list,
> -div.diff.from_file,
> +div.diff.from_file {
> +	color: #aa0000;
> +}
> +
>  div.diff.rem {
>  	color: #cc0000;
>  }
>  
>  div.diff.chunk_header {
>  	color: #990099;
> +	border: dotted #ffbbff;
> +	border-width: 1px 0px 0px 0px;
> +	margin-top: 1px;
>  }
>  
>  div.diff.incomplete {
>  	color: #cccccc;
>  }
>  
> -div.diff_info {
> -	font-family: monospace;
> -	color: #000099;
> -	background-color: #edece6;
> -	font-style: italic;
> -}
>  
>  div.index_include {
>  	border: solid #d9d8d1;
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index cbab3c9..2d971ac 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1255,9 +1255,12 @@ sub parse_difftree_raw_line {
>  		$res{'status'} = $5;
>  		$res{'similarity'} = $6;
>  		if ($res{'status'} eq 'R' || $res{'status'} eq 'C') { # renamed or copied
> -			($res{'from_file'}, $res{'to_file'}) = map { unquote($_) } split("\t", $7);
> +			($res{'from_file_raw'}, $res{'to_file_raw'}) = split("\t", $7);
> +			$res{'from_file'} = unquote($res{'from_file_raw'});
> +			$res{'to_file'}   = unquote($res{'to_file_raw'});
>  		} else {
> -			$res{'file'} = unquote($7);
> +			$res{'file_raw'} = $7;
> +			$res{'file'} = unquote($res{'file_raw'});
>  		}
>  	}
>  	# 'c512b523472485aef4fff9e57b229d9d243c967f'
> @@ -2024,6 +2027,7 @@ sub git_patchset_body {
>  	my $in_header = 0;
>  	my $patch_found = 0;
>  	my $diffinfo;
> +	my (@from_subst, @to_subst);
>  
>  	print "<div class=\"patchset\">\n";
>  
> @@ -2033,6 +2037,7 @@ sub git_patchset_body {
>  
>  		if ($patch_line =~ m/^diff /) { # "git diff" header
>  			# beginning of patch (in patchset)
> +
>  			if ($patch_found) {
>  				# close previous patch
>  				print "</div>\n"; # class="patch"
> @@ -2042,11 +2047,59 @@ sub git_patchset_body {
>  			}
>  			print "<div class=\"patch\" id=\"patch". ($patch_idx+1) ."\">\n";
>  
> +			# read and prepare patch information
>  			if (ref($difftree->[$patch_idx]) eq "HASH") {
> +				# pre-parsed (or generated by hand)
>  				$diffinfo = $difftree->[$patch_idx];
>  			} else {
>  				$diffinfo = parse_difftree_raw_line($difftree->[$patch_idx]);
>  			}
> +			if ($diffinfo->{'status'} ne "A") { # not new (added) file
> +				my $quot = '';
> +				my $from_text;
> +				my $file_raw = $diffinfo->{'from_file_raw'} || $diffinfo->{'file_raw'};
> +				if ($file_raw =~ s/^"(.*)"$/\1/) {
> +					$from_text = qq("a/$file_raw");
> +					$quot = '"';
> +				} else {
> +					$from_text =  qq(a/$file_raw);
> +				}
> +				my $file = $diffinfo->{'from_file'} || $diffinfo->{'file'};
> +				my $from_link =
> +					$cgi->a({-href => href(action=>"blob", hash_base=>$hash_base,
> +					                      hash=>$diffinfo->{'from_id'}, file_name=>$file),
> +					        -class => "list"}, esc_html($file_raw));
> +				my $hash_link =
> +					$cgi->a({-href => href(action=>"blob", hash_base=>$hash_base,
> +					                      hash=>$diffinfo->{'to_id'}, file_name=>$file),
> +					        -class => "list"}, substr($diffinfo->{'from_id'},0,7));
> +				@from_subst = 
> +					($from_text, "${quot}a/$from_link${quot}",
> +					$diffinfo->{'from_id'} . '\.\.', "$hash_link..");
> +			}
> +			if ($diffinfo->{'status'} ne "D") { # not deleted file
> +				my $quot = '';
> +				my $to_text;
> +				my $file_raw = $diffinfo->{'to_file_raw'} || $diffinfo->{'file_raw'};
> +				if ($file_raw =~ s/^"(.*)"$/\1/) {
> +					$to_text = qq("b/$file_raw");
> +					$quot = '"';
> +				} else {
> +					$to_text =  qq(b/$file_raw);
> +				}
> +				my $file = $diffinfo->{'to_file'} || $diffinfo->{'file'};
> +				my $to_link =
> +					$cgi->a({-href => href(action=>"blob", hash_base=>$hash_base,
> +					                      hash=>$diffinfo->{'to_id'}, file_name=>$file),
> +					        -class => "list"}, esc_html($file_raw));
> +				my $hash_link =
> +					$cgi->a({-href => href(action=>"blob", hash_base=>$hash_base,
> +					                      hash=>$diffinfo->{'to_id'}, file_name=>$file),
> +					        -class => "list"}, substr($diffinfo->{'to_id'},0,7));
> +				@to_subst =
> +					($to_text, "${quot}b/$to_link${quot}",
> +					 '\.\.' . $diffinfo->{'to_id'}, "..$hash_link");
> +			}
>  			$patch_idx++;
>  
>  			# for now we skip empty patches
> @@ -2056,82 +2109,56 @@ sub git_patchset_body {
>  				next LINE;
>  			}
>  
> -			if ($diffinfo->{'status'} eq "A") { # added
> -				print "<div class=\"diff_info\">" . file_type($diffinfo->{'to_mode'}) . ":" .
> -				      $cgi->a({-href => href(action=>"blob", hash_base=>$hash,
> -				                             hash=>$diffinfo->{'to_id'}, file_name=>$diffinfo->{'file'})},
> -				              $diffinfo->{'to_id'}) . " (new)" .
> -				      "</div>\n"; # class="diff_info"
> -
> -			} elsif ($diffinfo->{'status'} eq "D") { # deleted
> -				print "<div class=\"diff_info\">" . file_type($diffinfo->{'from_mode'}) . ":" .
> -				      $cgi->a({-href => href(action=>"blob", hash_base=>$hash_parent,
> -				                             hash=>$diffinfo->{'from_id'},
> file_name=>$diffinfo->{'file'})},
> -				              $diffinfo->{'from_id'}) . " (deleted)" .
> -				      "</div>\n"; # class="diff_info"
> -
> -			} elsif ($diffinfo->{'status'} eq "R" || # renamed
> -			         $diffinfo->{'status'} eq "C" || # copied
> -			         $diffinfo->{'status'} eq "2") { # with two filenames (from git_blobdiff)
> -				print "<div class=\"diff_info\">" .
> -				      file_type($diffinfo->{'from_mode'}) . ":" .
> -				      $cgi->a({-href => href(action=>"blob", hash_base=>$hash_parent,
> -				                             hash=>$diffinfo->{'from_id'},
> file_name=>$diffinfo->{'from_file'})},
> -				              $diffinfo->{'from_id'}) .
> -				      " -> " .
> -				      file_type($diffinfo->{'to_mode'}) . ":" .
> -				      $cgi->a({-href => href(action=>"blob", hash_base=>$hash,
> -				                             hash=>$diffinfo->{'to_id'},
> file_name=>$diffinfo->{'to_file'})},
> -				              $diffinfo->{'to_id'});
> -				print "</div>\n"; # class="diff_info"
> -
> -			} else { # modified, mode changed, ...
> -				print "<div class=\"diff_info\">" .
> -				      file_type($diffinfo->{'from_mode'}) . ":" .
> -				      $cgi->a({-href => href(action=>"blob", hash_base=>$hash_parent,
> -				                             hash=>$diffinfo->{'from_id'},
> file_name=>$diffinfo->{'file'})},
> -				              $diffinfo->{'from_id'}) .
> -				      " -> " .
> -				      file_type($diffinfo->{'to_mode'}) . ":" .
> -				      $cgi->a({-href => href(action=>"blob", hash_base=>$hash,
> -				                             hash=>$diffinfo->{'to_id'}, file_name=>$diffinfo->{'file'})},
> -				              $diffinfo->{'to_id'});
> -				print "</div>\n"; # class="diff_info"
> -			}
> +			# print "git diff" header
> +			$patch_line =~ s/$from_subst[0]/$from_subst[1]/ if @from_subst;
> +			$patch_line =~ s/$to_subst[0]/$to_subst[1]/ if @to_subst;
> +			print "<div class=\"diff header\">$patch_line</div>\n";
>  
> -			#print "<div class=\"diff extended_header\">\n";
> +			print "<div class=\"diff extended_header\">\n";
>  			$in_header = 1;
>  			next LINE;
>  		} # start of patch in patchset
>  
> +		if ($in_header) {
> +			if ($patch_line !~ m/^---/) {
> +				# match <path>
> +				if ($patch_line =~ m|a/|) {
> +					$patch_line =~ s/$from_subst[0]/$from_subst[1]/ if @from_subst;
> +				}
> +				if ($patch_line =~ m|b/|) {
> +					$patch_line =~ s/$to_subst[0]/$to_subst[1]/ if @to_subst;
> +				}
> +				# match <mode>
> +				if ($patch_line =~ m/\s(\d{6})$/) {
> +					$patch_line .= '/' . mode_str($1) . ' (' . file_type($1) . ')';
> +				}
> +				# match <hash>
> +				if ($patch_line =~ m/^index/) {
> +					$patch_line =~ s/0{40}/'0' x 7/e;
> +					$patch_line =~ s/$from_subst[2]/$from_subst[3]/ if @from_subst;
> +					$patch_line =~ s/$to_subst[2]/$to_subst[3]/ if @to_subst;
> +				}
> +				print $patch_line . "<br/>\n";
>  
> -		if ($in_header && $patch_line =~ m/^---/) {
> -			#print "</div>\n"; # class="diff extended_header"
> -			$in_header = 0;
> +			} else {
> +				#$patch_line =~ m/^---/;
> +				print "</div>\n"; # class="diff extended_header"
> +				$in_header = 0;
> +
> +				$patch_line =~ s/$from_subst[0]/$from_subst[1]/ if @from_subst;
> +				print "<div class=\"diff from_file\">$patch_line</div>\n";
>  
> -			my $file = $diffinfo->{'from_file'};
> -			$file  ||= $diffinfo->{'file'};
> -			$file = $cgi->a({-href => href(action=>"blob", hash_base=>$hash_parent,
> -			                               hash=>$diffinfo->{'from_id'}, file_name=>$file),
> -			                -class => "list"}, esc_html($file));
> -			$patch_line =~ s|a/.*$|a/$file|g;
> -			print "<div class=\"diff from_file\">$patch_line</div>\n";
> +				$patch_line = <$fd>;
> +				chomp $patch_line;
>  
> -			$patch_line = <$fd>;
> -			chomp $patch_line;
> +				#$patch_line =~ m/^+++/;
> +				$patch_line =~ s/$to_subst[0]/$to_subst[1]/ if @to_subst;
> +				print "<div class=\"diff to_file\">$patch_line</div>\n";
>  
> -			#$patch_line =~ m/^+++/;
> -			$file    = $diffinfo->{'to_file'};
> -			$file  ||= $diffinfo->{'file'};
> -			$file = $cgi->a({-href => href(action=>"blob", hash_base=>$hash,
> -			                               hash=>$diffinfo->{'to_id'}, file_name=>$file),
> -			                -class => "list"}, esc_html($file));
> -			$patch_line =~ s|b/.*|b/$file|g;
> -			print "<div class=\"diff to_file\">$patch_line</div>\n";
> +			}
>  
>  			next LINE;
>  		}
> -		next LINE if $in_header;
>  
>  		print format_diff_line($patch_line);
>  	}
> -- 
> 1.4.3.3
> 
> 
> 
> -------------------------------------------------------
> 
> -- 
> Jakub Narebski
> Poland
> -
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH/RFC] gitweb: New improved patchset view

[Luben Tuikov]

--- Junio C Hamano <junkio@cox.net> wrote:
> It somehow feels that deviating from what "git diff" gives makes
> it very distracting; I would feel better if "/-rw-r--r-- (file)"
> were not there.

Agree.

> Also I think arguing over 7 or 8 hexdigits is pointless; if you
> are reading this from "git diff", it is probably the easiest to
> match what "git diff" gave you.  One thing we _might_ want to do

Agree.

    Luben

Re: [PATCH/RFC] gitweb: New improved patchset view

[Luben Tuikov]

--- Anand Kumria <wildfire@progsoc.org> wrote:
> I think your version looks a lot nicer but I believe that the commit
> message area should be in a fixed-point font. In general it won't be a
> problem but if anyone should choose to insert a diagram or something else
> that assume a fixed-width display - it won't look good.

Thanks for catching this!

Jakub, I believe I submitted a patch to change that to fixed point
and you can see the reasons in the commit message.  See commit
4b7ce6e2d6ba088da50de1df38b040ea2c0b8f18.

Please change it to fixed point.

    Luben

Re: [PATCH/RFC] gitweb: New improved patchset view

[Luben Tuikov]

--- Jakub Narebski <jnareb@gmail.com> wrote:
> Junio C Hamano wrote:
> > Jakub Narebski <jnareb@gmail.com> writes:
> > 
> >> Changes:
> [...]
> >> * <file> hyperlinking should work also when <file> is originally
> >>   quoted. For now we present filename quoted. This needed changes to
> >>   parse_difftree_raw_line subroutine.
> > 
> > This feels Ok.
> 
> Emphasisis on "should". Check
>   http://roke(.)dyndns(.)info/cgi-bin/gitweb/gitweb.cgi?p=git.git;a=commitdiff;h=gitweb/test
> 
> By the way, should we use quoted or unquoted filename?

For dear life, I cannot understand  *WTF*  is this argument about
"quoted" and "unquoted".  Can this stop please?  PLEASE?

Unquoted, natural, normal.

    Luben
P.S. I don't want to punish the good people who do not put \n or \r or whatnot
in their filenames, just for this one ... "person" who does.  UTF8 is a different
story.

Re: [PATCH/RFC] gitweb: New improved patchset view

[Luben Tuikov]

--- Jakub Narebski <jnareb@gmail.com> wrote:
> A couple of questions regarding new patchset/diff look for gitweb.
> Currently patch starts with "git diff" header
> 
>   diff --git a/file1 b/file2
> 
> then we have a couple of extended header lines
> 
>   old|new|deleted file|new file mode <mode>
>   copy|rename from|to <path>
>   (dis)?similarity index <percent>
>   index <hash>..<hash> [<mode>]
> 
> then we have two-line from-file/to-file header

First revert back to monospace in the commit message.
As I mentioned in that other email, where the person
didn't CC you and thus so I didn't, see commit 
4b7ce6e2d6ba088da50de1df38b040ea2c0b8f18.

Can you please hyperlink what you had intended to
quote?  I.e. filenames which can lead the user browsing to a
state (pre-rename, post-rename, etc).  Thanks.

> 
>   --- a/file1
>   +++ b/file2
> 
> then patch itself.
> 
> 
> 1. Which parts to convert to hyperlinks, and which to do not? Which
>    links have visible and which hidden (underline on mouseover, the same
>    color as neighbouring text)?

I think the current state of your patch is good, sans the comments
received so far: monospace, that bug in the index links, etc.
Lets get that in, and then you can RFC another improvement on top of this.

>    I think that a/file1 in "git diff" header should be turned into
>    visible hyperlink unless file is created, and b/file2 should be link
>    unless file is deleted.

This is intuitive and makes sense.

>    Also both <hash>-es in "index" extended header lines should be turned
>    into links, as it is the only way to have hyperlink to all previou

Fix the bug though.

>    versions of the file in the case of "combined diff" format (to be
>    added later). The question is if those hyperlinks should be visible;

I personally love little hidden gems, but some people like everything
to be overly obvious to them.  Hidden gems are part of the learning,
but I'm sure I'm not going to convince everyone.

Hidden.

>    I don't have compelling reason against. Should we use title attribute
>    to give filename perhaps, or is it unnecessary?
> 
>    Currently file1 and file2 in "--- a/file1" and "+++ b/file2" are
>    turned into hidden links. Should we leave it, or should we remove
>    this link as we have similar link just above? If we decide to have

Leave it -- it's cool.

>    this link, should we also hyperlink <path> in "copy|rename" extended
>    header line?

If it makes sense.

> 2. Use quoted or unquoted filename, remove or leave surrounding quotes
>    in quoted filename? Should we unquote the not hyperlinked filename

Are you familiar with the term "legalism"?

>    in the case of creation/deletion? What should be span of link:
>      "a/_file1_", "_a/file1_", _"a/file1"_
>      "_file_", _"file"_ 
>    (where '_' marks beginning and end of link) for quoted filenames?
>    What should be span of link for unquoted filenames:
>       a/_file_, _a/file_
>    Currently gitweb uses a/_file_ in ---/+++ line.

Unquoted!  Now that we've solved this "problem", lets move on to more
interesting things. ;-)

> 3. How (and if) to explain numerical mode: the currently used

Let's not explain it for now.  Let's have your patch go in sans the
comments already posted.  We can always debate on that later.

Thanks,
   Luben


>    <mode>/<symbolic mode> (<file type>) e.g. 100755/-rwxr-xr-x (file).
>    Or <mode> (<file type>) should be enough? Should we mark the addition
>    compared to git-diff output? Or should we explain <mode> only on
>    mouseover, using for example:
>      <abbr title="100644/-rw-r--r-- (file)">100644</abbr>
>    or just
>      <abbr title="executable file">100755</abbr>?
> -- 
> Jakub Narebski
> Poland
> -
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH/RFC] gitweb: New improved patchset view

[Jakub Narebski]

Luben Tuikov wrote:
> --- Jakub Narebski <jnareb@gmail.com> wrote:
>>
>> By the way, should we use quoted or unquoted filename?
> 
> For dear life, I cannot understand  *WTF*  is this argument about
> "quoted" and "unquoted".  Can this stop please?  PLEASE?
> 
> Unquoted, natural, normal.
> 
>     Luben
> P.S. I don't want to punish the good people who do not put \n or \r or whatnot
> in their filenames, just for this one ... "person" who does.  UTF8 is a different
> story.

The filenames which don't need quoting are not quoted. Only filenames
which need quoting (which have LF ('\n'), TAB ('\t'), \ ('\\') and " (\"))
are quoted and surronded in quotes to mark as quoted.

The only thing that is questionable is quoting files which have \ and "
and no other strange names.
-- 
Jakub Narebski

Re: [PATCH/RFC] gitweb: New improved patchset view

[Junio C Hamano]

Luben Tuikov <ltuikov@yahoo.com> writes:

> For dear life, I cannot understand  *WTF*  is this argument about
> "quoted" and "unquoted".  Can this stop please?  PLEASE?
>
> Unquoted, natural, normal.

I do not think Jakub is talking about quoting normal filenames a/foo
that is not quoted by "git diff" into "a/foo" with his code.

I think when "git diff" quotes because of unusual characters
gitweb should show it quoted, i.e. as it receives from "git diff".

Re: [PATCH/RFC] gitweb: New improved patchset view

[Jakub Narebski]

Luben Tuikov wrote:
> --- Jakub Narebski <jnareb@gmail.com> wrote:
> > A couple of questions regarding new patchset/diff look for gitweb.
> > Currently patch starts with "git diff" header
> > 
> >   diff --git a/file1 b/file2
> > 
> > then we have a couple of extended header lines
> > 
> >   old|new|deleted file|new file mode <mode>
> >   copy|rename from|to <path>
> >   (dis)?similarity index <percent>
> >   index <hash>..<hash> [<mode>]
> > 
> > then we have two-line from-file/to-file header
> 
> First revert back to monospace in the commit message.
> As I mentioned in that other email, where the person
> didn't CC you and thus so I didn't, see commit 
> 4b7ce6e2d6ba088da50de1df38b040ea2c0b8f18.

This regression was caused by the not-accepted (I think) patch
"gitweb: Slight visual improvements to commitdiff view",
and is not caused by _this_ patch.

I'll redo abovementioned patch correctly later.

> Can you please hyperlink what you had intended to
> quote?  I.e. filenames which can lead the user browsing to a
> state (pre-rename, post-rename, etc).  Thanks.

I don't understand this comment. The above is pre-changes git-diff
patch output. The changes are/were mentioned below.

> > 
> >   --- a/file1
> >   +++ b/file2
> > 
> > then patch itself.
> > 
> > 
> > 1. Which parts to convert to hyperlinks, and which to do not? Which
> >    links have visible and which hidden (underline on mouseover, the same
> >    color as neighbouring text)?
> 
> I think the current state of your patch is good, sans the comments
> received so far: monospace, that bug in the index links, etc.
> Lets get that in, and then you can RFC another improvement on top of this.

O.K. I send the corrected version (as "take 2") in a while.

-- 
Jakub Narebski

Re: [PATCH/RFC] gitweb: New improved patchset view

[Jakub Narebski]

Jakub Narebski wrote:

> The filenames which don't need quoting are not quoted. Only filenames
> which need quoting (which have LF ('\n'), TAB ('\t'), \ ('\\') and " (\"))
> are quoted and surronded in quotes to mark as quoted.

By the way, I think that unquote subroutine in gitweb is broken.
I think I'll try for new patchser view to work for ordinary filenames
(perhaps with metacharacters in them), and only then try to debug
for filenames which need quoting.
-- 
Jakub Narebski

[PATCH/RFC (take 2)] gitweb: New improved patchset view

[Jakub Narebski]

Replace "gitweb diff header" with its full sha1 of blobs and replace
it by "git diff" header and extended diff header. Change also somewhat
highlighting of diffs.

Changes:
* "gitweb diff header" which looked for example like below:
    file:_<sha1 before>_ -> file:_<sha1 after>_
  where 'file' is file type and '<sha1>' is full sha1 of blob is
  changed to
    diff --git _a/<file before>_ _b/<file after>_
  In both cases links are visible and use default link style. If file
  is added, a/<file> is not hyperlinked, if file is deleted, b/<file>
  is not hyperlinked.
* there is added "extended diff header", with <path> and <hash>
  hyperlinked (and <hash> shortened to 7 characters), and <mode>
  explained: '<mode>' is extended to '<mode> (<file type>)'.
* <file> hyperlinking should work also when <file> is originally
  quoted. For now we present filename quoted. This needed changes to
  parse_difftree_raw_line subroutine. And doesn't work: perhaps
  unquote is broken.
* from-file/to-file two-line header lines have slightly darker color
  than removed/added lines.
* chunk header has now delicate line above for easier finding chunk
  boundary, and top margin of 1px.

WORK IN PROGRESS: might not work (and actually doesn't work correctly)
for strange filenames, i.e. filenames contaning either metacharacters
or having TAB, LF, backslash or doublequote in them.

Code should be much more clean, by the way.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.css  |   46 +++++++++++---
 gitweb/gitweb.perl |  178 +++++++++++++++++++++++++++++++++-------------------
 2 files changed, 151 insertions(+), 73 deletions(-)

diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index 83d900d..3aeceed 100644
--- a/gitweb/gitweb.css
+++ b/gitweb/gitweb.css
@@ -303,6 +303,33 @@ td.mode {
 	font-family: monospace;
 }
 
+
+div.diff.header,
+div.diff.extended_header {
+	white-space: normal;
+}
+
+div.diff.header {
+	font-weight: bold;
+
+	background-color: #edece6;
+
+	margin-top: 4px;
+	padding: 4px 0px 2px 0px;
+	border: solid #d9d8d1;
+	border-width: 1px 0px 1px 0px;
+}
+
+div.diff.extended_header,
+div.diff.extended_header a.list {
+	color: #777777;
+}
+
+div.diff.extended_header {
+	background-color: #f6f5ee;
+	padding: 2px 0px 2px 0px;
+}
+
 div.diff a.list {
 	text-decoration: none;
 }
@@ -312,31 +339,34 @@ div.diff a.list:hover {
 }
 
 div.diff.to_file a.list,
-div.diff.to_file,
+div.diff.to_file {
+	color: #007000;
+}
+
 div.diff.add {
 	color: #008800;
 }
 
 div.diff.from_file a.list,
-div.diff.from_file,
+div.diff.from_file {
+	color: #aa0000;
+}
+
 div.diff.rem {
 	color: #cc0000;
 }
 
 div.diff.chunk_header {
 	color: #990099;
+	border: dotted #ffbbff;
+	border-width: 1px 0px 0px 0px;
+	margin-top: 1px;
 }
 
 div.diff.incomplete {
 	color: #cccccc;
 }
 
-div.diff_info {
-	font-family: monospace;
-	color: #000099;
-	background-color: #edece6;
-	font-style: italic;
-}
 
 div.index_include {
 	border: solid #d9d8d1;
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index cbab3c9..a5a140c 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1255,9 +1255,12 @@ sub parse_difftree_raw_line {
 		$res{'status'} = $5;
 		$res{'similarity'} = $6;
 		if ($res{'status'} eq 'R' || $res{'status'} eq 'C') { # renamed or copied
-			($res{'from_file'}, $res{'to_file'}) = map { unquote($_) } split("\t", $7);
+			($res{'from_file_raw'}, $res{'to_file_raw'}) = split("\t", $7);
+			$res{'from_file'} = unquote($res{'from_file_raw'});
+			$res{'to_file'}   = unquote($res{'to_file_raw'});
 		} else {
-			$res{'file'} = unquote($7);
+			$res{'file_raw'} = $7;
+			$res{'file'} = unquote($res{'file_raw'});
 		}
 	}
 	# 'c512b523472485aef4fff9e57b229d9d243c967f'
@@ -2023,7 +2026,9 @@ sub git_patchset_body {
 	my $patch_idx = 0;
 	my $in_header = 0;
 	my $patch_found = 0;
+	my $skip_patch = 0;
 	my $diffinfo;
+	my (%from, %to);
 
 	print "<div class=\"patchset\">\n";
 
@@ -2033,6 +2038,8 @@ sub git_patchset_body {
 
 		if ($patch_line =~ m/^diff /) { # "git diff" header
 			# beginning of patch (in patchset)
+			$skip_patch = 0;
+
 			if ($patch_found) {
 				# close previous patch
 				print "</div>\n"; # class="patch"
@@ -2042,96 +2049,137 @@ sub git_patchset_body {
 			}
 			print "<div class=\"patch\" id=\"patch". ($patch_idx+1) ."\">\n";
 
+			# read and prepare patch information
 			if (ref($difftree->[$patch_idx]) eq "HASH") {
+				# pre-parsed (or generated by hand)
 				$diffinfo = $difftree->[$patch_idx];
 			} else {
 				$diffinfo = parse_difftree_raw_line($difftree->[$patch_idx]);
 			}
+			if ($diffinfo->{'status'} ne "A") { # not new (added) file
+				$from{'name'} = $diffinfo->{'from_file_raw'} || $diffinfo->{'file_raw'};
+				 # because of "a/file" not a/"file"
+				$from{'quoted'} = ($from{'name'} =~ s/^"(.*)"$/$1/);
+
+				my $file = $diffinfo->{'from_file'} || $diffinfo->{'file'};
+				$from{'href'} = href(action=>"blob", hash_base=>$hash_parent,
+				                     hash=>$diffinfo->{'from_id'}, file_name=>$file);
+			}
+			if ($diffinfo->{'status'} ne "D") { # not deleted file
+				$to{'name'} = $diffinfo->{'to_file_raw'} || $diffinfo->{'file_raw'};
+				# because of "b/file" not b/"file"
+				$to{'quoted'} = ($to{'name'} =~ s/^"(.*)"$/$1/);
+
+				my $file = $diffinfo->{'to_file'} || $diffinfo->{'file'};
+				$to{'href'} = href(action=>"blob", hash_base=>$hash,
+				                   hash=>$diffinfo->{'to_id'}, file_name=>$file);
+			}
 			$patch_idx++;
 
 			# for now we skip empty patches
 			if ($diffinfo->{'from_id'} eq $diffinfo->{'to_id'}) {
 				# no change, empty patch
 				$in_header = 1;
+				$skip_patch = 0;
 				next LINE;
 			}
 
-			if ($diffinfo->{'status'} eq "A") { # added
-				print "<div class=\"diff_info\">" . file_type($diffinfo->{'to_mode'}) . ":" .
-				      $cgi->a({-href => href(action=>"blob", hash_base=>$hash,
-				                             hash=>$diffinfo->{'to_id'}, file_name=>$diffinfo->{'file'})},
-				              $diffinfo->{'to_id'}) . " (new)" .
-				      "</div>\n"; # class="diff_info"
-
-			} elsif ($diffinfo->{'status'} eq "D") { # deleted
-				print "<div class=\"diff_info\">" . file_type($diffinfo->{'from_mode'}) . ":" .
-				      $cgi->a({-href => href(action=>"blob", hash_base=>$hash_parent,
-				                             hash=>$diffinfo->{'from_id'}, file_name=>$diffinfo->{'file'})},
-				              $diffinfo->{'from_id'}) . " (deleted)" .
-				      "</div>\n"; # class="diff_info"
-
-			} elsif ($diffinfo->{'status'} eq "R" || # renamed
-			         $diffinfo->{'status'} eq "C" || # copied
-			         $diffinfo->{'status'} eq "2") { # with two filenames (from git_blobdiff)
-				print "<div class=\"diff_info\">" .
-				      file_type($diffinfo->{'from_mode'}) . ":" .
-				      $cgi->a({-href => href(action=>"blob", hash_base=>$hash_parent,
-				                             hash=>$diffinfo->{'from_id'}, file_name=>$diffinfo->{'from_file'})},
-				              $diffinfo->{'from_id'}) .
-				      " -> " .
-				      file_type($diffinfo->{'to_mode'}) . ":" .
-				      $cgi->a({-href => href(action=>"blob", hash_base=>$hash,
-				                             hash=>$diffinfo->{'to_id'}, file_name=>$diffinfo->{'to_file'})},
-				              $diffinfo->{'to_id'});
-				print "</div>\n"; # class="diff_info"
-
-			} else { # modified, mode changed, ...
-				print "<div class=\"diff_info\">" .
-				      file_type($diffinfo->{'from_mode'}) . ":" .
-				      $cgi->a({-href => href(action=>"blob", hash_base=>$hash_parent,
-				                             hash=>$diffinfo->{'from_id'}, file_name=>$diffinfo->{'file'})},
-				              $diffinfo->{'from_id'}) .
-				      " -> " .
-				      file_type($diffinfo->{'to_mode'}) . ":" .
-				      $cgi->a({-href => href(action=>"blob", hash_base=>$hash,
-				                             hash=>$diffinfo->{'to_id'}, file_name=>$diffinfo->{'file'})},
-				              $diffinfo->{'to_id'});
-				print "</div>\n"; # class="diff_info"
+			# print "git diff" header
+			if ($from{'name'}) {
+				my $from_link = $cgi->a({-href => $from{'href'}, -class => "path"},
+				                        'a/' . esc_html($from{'name'}));
+				my ($q, $qq) = $from{'quoted'} ? ('"', '&quot;') : ('', '');
+				$patch_line =~ s|${q}a/\Q$from{'name'}\E${q}|${qq}$from_link${qq}|;
+			} else {
+				# at least one of %from and %to must be set
+				$patch_line =~ s|(["]?a/\Q$to{'name'}\E["]?)|esc_html($1)|e;
+			}
+			if ($to{'name'}) {
+				my $to_link = $cgi->a({-href => $to{'href'}, -class => "path"},
+				                      'b/' . esc_html($to{'name'}));
+				my ($q, $qq) = $to{'quoted'} ? ('"', '&quot;') : ('', '');
+				$patch_line =~ s|${q}b/\Q$to{'name'}\E${q}$|${qq}$to_link${qq}|;
+			} else {
+				# at least one of %from and %to must be set
+				$patch_line =~ s|(["]?b/\Q$from{'name'}\E["]?)$|esc_html($1)|e;
 			}
 
-			#print "<div class=\"diff extended_header\">\n";
+			print "<div class=\"diff header\">$patch_line</div>\n";
+			print "<div class=\"diff extended_header\">\n";
 			$in_header = 1;
 			next LINE;
+		} else {
+			next LINE if $skip_patch;
 		} # start of patch in patchset
 
+		if ($in_header) {
+			if ($patch_line !~ m/^---/) {
+				# match <path>
+				if ($patch_line =~ m!^(copy|rename) from ! && $from{'name'}) {
+					my $qq = $from{'quoted'} ? '&quot;' : '';
+					my $from_link = $cgi->a({-href=>$from{'href'},-class=>"list"},
+					                        esc_html($from{'name'}));
+					$patch_line =~ s!from .*$!from $qq$from_link$qq!;
+				}
+				if ($patch_line =~ m!^(copy|rename) to ! && $to{'name'}) {
+					my $qq = $to{'quoted'} ? '&quot;' : '';
+					my $to_link = $cgi->a({-href=>$to{'href'},-class=>"list"},
+					                      esc_html($to{'name'}));
+					$patch_line =~ s!to .*$!to $qq$to_link$qq!;
+				}
+				# match <mode>
+				if ($patch_line =~ m/\s(\d{6})$/) {
+					$patch_line .= '<span class="info"> (' . file_type($1) . ')</span>';
+				}
+				# match <hash>
+				if ($patch_line =~ m/^index/) {
+					my ($from_link, $to_link);
+					if ($from{'href'}) {
+						$from_link = $cgi->a({-href=>$from{'href'},-class=>"list"},
+						                     substr($diffinfo->{'from_id'},0,7));
+					} else {
+						$from_link = '0' x 7;
+					}
+					if ($to{'href'}) {
+						$to_link = $cgi->a({-href=>$to{'href'},-class=>"list"},
+						                   substr($diffinfo->{'to_id'},0,7));
+					} else {
+						$to_link = '0' x 7;
+					}
+					my ($from_id, $to_id) = ($diffinfo->{'from_id'}, $diffinfo->{'to_id'});
+					$patch_line =~ s!$from_id\.\.$to_id!$from_link..$to_link!;
+				}
+				print $patch_line . "<br/>\n";
 
-		if ($in_header && $patch_line =~ m/^---/) {
-			#print "</div>\n"; # class="diff extended_header"
-			$in_header = 0;
+			} else {
+				#$patch_line =~ m/^---/;
+				print "</div>\n"; # class="diff extended_header"
+				$in_header = 0;
+
+				if ($from{'name'}) {
+					my $qq = $from{'quoted'} ? '&quot;' : '';
+					my $from_link = $cgi->a({-href=>$from{'href'},-class=>"list"},
+					                        esc_html($from{'name'}));
+					$patch_line =~ s!["]?a/.*$!${qq}a/$from_link${qq}!;
+				}
+				print "<div class=\"diff from_file\">$patch_line</div>\n";
 
-			my $file = $diffinfo->{'from_file'};
-			$file  ||= $diffinfo->{'file'};
-			$file = $cgi->a({-href => href(action=>"blob", hash_base=>$hash_parent,
-			                               hash=>$diffinfo->{'from_id'}, file_name=>$file),
-			                -class => "list"}, esc_html($file));
-			$patch_line =~ s|a/.*$|a/$file|g;
-			print "<div class=\"diff from_file\">$patch_line</div>\n";
+				$patch_line = <$fd>;
+				chomp $patch_line;
 
-			$patch_line = <$fd>;
-			chomp $patch_line;
+				#$patch_line =~ m/^+++/;
+				if ($to{'name'}) {
+					my $qq = $to{'quoted'} ? '&quot;' : '';
+					my $from_link = $cgi->a({-href=>$to{'href'},-class=>"list"},
+					                        esc_html($to{'name'}));
+					$patch_line =~ s!["]?b/.*$!${qq}b/$from_link${qq}!;
+				}
+				print "<div class=\"diff to_file\">$patch_line</div>\n";
 
-			#$patch_line =~ m/^+++/;
-			$file    = $diffinfo->{'to_file'};
-			$file  ||= $diffinfo->{'file'};
-			$file = $cgi->a({-href => href(action=>"blob", hash_base=>$hash,
-			                               hash=>$diffinfo->{'to_id'}, file_name=>$file),
-			                -class => "list"}, esc_html($file));
-			$patch_line =~ s|b/.*|b/$file|g;
-			print "<div class=\"diff to_file\">$patch_line</div>\n";
+			}
 
 			next LINE;
 		}
-		next LINE if $in_header;
 
 		print format_diff_line($patch_line);
 	}
-- 
1.4.3.3

Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view

[Jakub Narebski]

Few other questions, probably to be adressed in the future patches, and 
not added to this one.

0. git-ls-tree and git-diff-tree without -z does quote not only !isprint 
characters like LF '\n' and non-space whitespace characters like TAB 
'\t' (and of course quoting characters '\' and '"'), but for example 
also UTF-8 characters. See for example git-ls-tree output for 
gitweb/test directory. This screws somewhat idea how we should treat 
filenames which are quoted.

[BTW. How git should deal with being deployed in the environment where 
filesystem pathnames coding might not be UTF-8, and console (terminal) 
coding might be not UTF-8?]


1. Current version doesn't display empty patches (i.e. pure rename and 
mode change combinations) and doesn't provide links to them from 
difftree. This is legacy of old /usr/bin/diff using code, which did not 
generated extended diff header, which is only output for "empty 
patches". Should we change this, or leave as is?

2. Schould we change syntax highlighting of chunk header line, namely 
changing slightly syntax coloring of "in which function are we" part of 
chunk header?

3. Should we make from-range/to-range in chunk header hyperlink to the 
start of given bunch of lines in appropriate file? Or perhaps to the 
middle of the bunch of lines? Or to first changed line (omitting 
context)?

-- 
Jakub Narebski

Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Few other questions, probably to be adressed in the future patches, and 
> not added to this one.
>
> 0. git-ls-tree and git-diff-tree without -z does quote not only ...
> also UTF-8 characters.

I've already mentioned this in an earlier message to you:

	Message-ID: <7v3b972bzq.fsf@assigned-by-dhcp.cox.net>

Let's illustrate what I mean by an untested patch; this does:

 0. Use explicitly "unsigned char" so that (ch < ' ') does not
    catch bytes in 0x80- range.  The original meant to catch the
    control characters only so this is a bugfix;

 1. We still worry about control characters in 0x80-0x9f range;
    if there are some, that is not a valid UTF-8 string (or
    other encodings that is compatible with ASCII), and quoting
    only these bytes and not quoting 0xa0- range can result
    in letters chopped in the middle, so we would quote all
    bytes in 0xa0- range when we have them;

 2. Otherwise we do not quote bytes in 0xa0- range.

-- >8 --
diff --git a/quote.c b/quote.c
index ee7d62c..4f086fb 100644
--- a/quote.c
+++ b/quote.c
@@ -199,18 +199,32 @@ static int quote_c_style_counted(const c
 
 #define EMITQ() EMIT('\\')
 
-	const char *sp;
-	int ch, count = 0, needquote = 0;
+	const unsigned char *name_u = (const unsigned char *)name;
+	const unsigned char *sp;
+	int ch, count = 0, needquote = 0, has_high_ctrl = 0;
+
+	/* Check if we have control character in 0x80-0x9f range */
+	for (sp = name_u; sp < name_u + namelen; sp++) {
+		ch = *sp;
+		if (!ch)
+			break;
+		if ((ch < ' ') || (ch == '"') || (ch == '\\') ||
+		    (ch == 0177) || (ch == 0377))
+			needquote = 1;
+		else if (0x80 <= ch && ch <= 0x9f)
+			needquote = has_high_ctrl = 1;
+	}
 
 	if (!no_dq)
 		EMIT('"');
-	for (sp = name; sp < name + namelen; sp++) {
+
+	for (sp = name_u; sp < name_u + namelen; sp++) {
 		ch = *sp;
 		if (!ch)
 			break;
 		if ((ch < ' ') || (ch == '"') || (ch == '\\') ||
-		    (ch == 0177) || (ch == 0377)) {
-			needquote = 1;
+		    (ch == 0177) ||
+		    (has_high_ctrl && 0x80 <= ch)) {
 			switch (ch) {
 			case '\a': EMITQ(); ch = 'a'; break;
 			case '\b': EMITQ(); ch = 'b'; break;
-- 8< --

> 1. Current version doesn't display empty patches (i.e. pure rename and 
> mode change combinations) and doesn't provide links to them from 
> difftree. This is legacy of old /usr/bin/diff using code, which did not 
> generated extended diff header, which is only output for "empty 
> patches". Should we change this, or leave as is?

I think this needs to be fixed.

> 2. Schould we change syntax highlighting of chunk header line, namely 
> changing slightly syntax coloring of "in which function are we" part of 
> chunk header?

Probably matching "git diff --color" would be sensible; by
following what has already been done, you do not have to think
about what to color and how yourself.

> 3. Should we make from-range/to-range in chunk header hyperlink to the 
> start of given bunch of lines in appropriate file? Or perhaps to the 
> middle of the bunch of lines? Or to first changed line (omitting 
> context)?

I do not see what usage pattern this link would help.  Care to
explain a bit better?

Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view

[Jakub Narebski]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>> 3. Should we make from-range/to-range in chunk header hyperlink to the 
>> start of given bunch of lines in appropriate file? Or perhaps to the 
>> middle of the bunch of lines? Or to first changed line (omitting 
>> context)?
> 
> I do not see what usage pattern this link would help.  Care to
> explain a bit better?

For example for the following header

diff --git _a/gitweb/gitweb.perl_ _b/gitweb/gitweb.perl_
index _cbab3c9_.._a5a140c_ 100755
--- a/_gitweb/gitweb.perl_
+++ b/_gitweb/gitweb.perl_
@@ _-1255,9_ _+1255,12_ @@ sub parse_difftree_raw_line {

The '-1255,9' would be link to older version of gitweb/gitweb.perl file,
directly to the line #1255 (or to first changed line, or to the middle
of the chunk i.e. 1255+9/2 line).

The '+1255,12' would be link to newer version of file, to line 1255.
-- 
Jakub Narebski

Re: [PATCH/RFC] gitweb: New improved patchset view

[Luben Tuikov]

--- Junio C Hamano <junkio@cox.net> wrote:
> Before answering 3 questions, I think we need to ask a bigger
> question.  What is the primary target audience of gitweb?
> 
> If it is for git-uninitiated, then staying as close to what GNU
> diff would give would be a better idea.  I would say we at least
> can assume that the user has some familiarity with SCM, and
> knows what kind of information is kept track of and is shown as
> differences between versions, and what files, directories and
> symlinks are, but not how git represents these.  On the other
> hand, if the user uses git to track a project (not necessarily
> the project the user is looking at with gitweb) and is familiar
> with the way git-diff presents these information, deviating from
> git-diff output is distractiing.

I agree.

gitweb's primary audience is the developers who type "git ..."
at shell prompt all the time.

> At least to me /-rw-rw-... part made me feel uneasy for that
> reason.

It made me feel uneasy to see it where it was because it
didn't belong there either way.

> WIth that in mind, I'll think aloud what I would like if I were
> not familiar with git:
> 
>  * "diff --git a/file b/file" would not use /dev/null but
>    ---/+++ does.  If the former is shown as link, it should be
>    visible which side is a link and which side is not for
>    creation or deletion diff; otherwise you would need a second

I'd argue otherwise: trying to click on /dev/null and failing
but succeeding on b/file has already taught something to the
uinitiated user.

OTOH, if one is trying to "click" on /dev/null in gitweb commitdiff
view -- they have other problems to resolve first...

>    to realize it is not a bug that clicking on a/file on the
>    "diff --git" line did not show anything for a creation diff.
> 
>  * I think showing object names in "index xxx..yyy mode" line is
>    not very useful to humans (they are added for tools).  I do

I like to see it because I might need to know the sha in order to
go back to shell prompt and do something with the object.  Instead
of having to git-ls-tree -r .... in order to find it from the sha
of the commitdiff.

>    agree that we would want some clickable handle in combined
>    diff output, but people not familiar with git would not know
>    that "index xxx,yyy..zzz" is where you would find the
>    parents, so that line needs to be munged anyway.

I think gitweb should first and foremost cater to git users.

>    Side note: Even though some git people (Luben, for example)
>    claim they recognize some abbreviated object names, I suspect
>    that are mostly recent commits and not blobs.

Yes, commit-8 for the day, blob-8 for less than a minute
which allows me to move the mouse pointer from to the xterm.

>  * Mode on the "index" line may be useful, but as you say 100644
>    is probably too git specific; however if our audience is
>    git-uninitiated, I doubt -rw-r--r-- is much better (it is
>    UNIXism, which I personally do not mind).  Spelling them out
>    like "regular file", "executable file", or "symbolic link"
>    might be more readable.

It would be more readable, but then it would be more readable.
Ideally I'd like it to be less readable, i.e. less to read.

>  * I think the filename quoting is better left as-is, since it
>    is a way to indicate something funny is going on. 

I agree.

   Luben

Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view

[Luben Tuikov]

--- Jakub Narebski <jnareb@gmail.com> wrote:
> Replace "gitweb diff header" with its full sha1 of blobs and replace
> it by "git diff" header and extended diff header. Change also somewhat
> highlighting of diffs.
> 
> Changes:
> * "gitweb diff header" which looked for example like below:
>     file:_<sha1 before>_ -> file:_<sha1 after>_
>   where 'file' is file type and '<sha1>' is full sha1 of blob is
>   changed to
>     diff --git _a/<file before>_ _b/<file after>_
>   In both cases links are visible and use default link style. If file
>   is added, a/<file> is not hyperlinked, if file is deleted, b/<file>
>   is not hyperlinked.

"Everything clickable underlined" isn't the best way to represent things.
Anyway, my 2 cents is that I don't like the overly explicit underlineing.
I liked it the way it was in take 1.

> * there is added "extended diff header", with <path> and <hash>
>   hyperlinked (and <hash> shortened to 7 characters), and <mode>
>   explained: '<mode>' is extended to '<mode> (<file type>)'.
> * <file> hyperlinking should work also when <file> is originally
>   quoted. For now we present filename quoted. This needed changes to
>   parse_difftree_raw_line subroutine. And doesn't work: perhaps
>   unquote is broken.

In which case we shouldn't commit this.  IOW, let's commit things
which we _know_ to work.

Why not resubmit your original patch with the bugfixes as few comments
as mentioned?

> * from-file/to-file two-line header lines have slightly darker color
>   than removed/added lines.
> * chunk header has now delicate line above for easier finding chunk
>   boundary, and top margin of 1px.

Wouldn't this be confusing with the other fine lines?
I personally don't like this chunk separation.  Chunk separation
already exists as is and we view it all the time elsewhere.

If you'd like to separate chunks, why not darken the background
of the section of line the chunk header is printed at?  I.e.
anything between the @@ including the @@.

    Luben

Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view

[Jakub Narebski]

Luben Tuikov wrote:
> --- Jakub Narebski <jnareb@gmail.com> wrote:
>> Replace "gitweb diff header" with its full sha1 of blobs and replace
>> it by "git diff" header and extended diff header. Change also somewhat
>> highlighting of diffs.
>> 
>> Changes:
>> * "gitweb diff header" which looked for example like below:
>>     file:_<sha1 before>_ -> file:_<sha1 after>_
>>   where 'file' is file type and '<sha1>' is full sha1 of blob is
>>   changed to
>>     diff --git _a/<file before>_ _b/<file after>_
>>   In both cases links are visible and use default link style. If file
>>   is added, a/<file> is not hyperlinked, if file is deleted, b/<file>
>>   is not hyperlinked.
> 
> "Everything clickable underlined" isn't the best way to represent things.
> Anyway, my 2 cents is that I don't like the overly explicit underlineing.
> I liked it the way it was in take 1.

Thet is the only "obviously link" per patch. And I think there should be
at least one non-hidden link.

BTW. comments like this are the reason I've sent the patch as-is, without
resolving the strange filenames problem (it would be nice if somebody was
to send code; well Junio send patch to address core git filename quoting
issue).

>> * there is added "extended diff header", with <path> and <hash>
>>   hyperlinked (and <hash> shortened to 7 characters), and <mode>
>>   explained: '<mode>' is extended to '<mode> (<file type>)'.
>> * <file> hyperlinking should work also when <file> is originally
>>   quoted. For now we present filename quoted. This needed changes to
>>   parse_difftree_raw_line subroutine. And doesn't work: perhaps
>>   unquote is broken.
> 
> In which case we shouldn't commit this.  IOW, let's commit things
> which we _know_ to work.
> 
> Why not resubmit your original patch with the bugfixes as few comments
> as mentioned?

I'll do that, but for now quoting/unquoting filename is broken, both
in gitweb but also to lesser extent in git core (quoting perfectly valid
UTF-8 characters).

I'll try to adress that, but I wanted to send next RFC patch for review.

>> * from-file/to-file two-line header lines have slightly darker color
>>   than removed/added lines.
>> * chunk header has now delicate line above for easier finding chunk
>>   boundary, and top margin of 1px.
> 
> Wouldn't this be confusing with the other fine lines?
> I personally don't like this chunk separation.  Chunk separation
> already exists as is and we view it all the time elsewhere.

But not always the program displaying diff can display such line
separating chunks, for example on text terminal it can't.

But if you think that the dotted 1px #ffbbff line is too intrusive,
we can remove it (and perhaps increase vertical space a few pixels).
I'd like to have more opinions first.

BTW. you can easily override it in your CSS file.
 
> If you'd like to separate chunks, why not darken the background
> of the section of line the chunk header is printed at?  I.e.
> anything between the @@ including the @@.

I'd rather have this one in a separate commit (this needs changes
to format_diff_line, not only to git_patchset_body).

-- 
Jakub Narebski

Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> ..., without
> resolving the strange filenames problem (it would be nice if somebody was
> to send code; well Junio send patch to address core git filename quoting
> issue).

Having showed that patch, I do not think it is a good way to go.

I think the UI layer like gitweb should have freedom to choose
its own pathname handling, and should read from -z output.

The git plumbing is agnostic to the character encoding issues,
and does not care what character set pathnames are encoded, and
what character set the file contents are encoded.  These are
very project specific and should be handled per project.

One of the primary purposes, aside from being understandable by
humans, of human readable "git diff" format is to convey machine
readable information without corruption in text form transport
(think "e-mail"), so quoting pathnames to avoid high-bits has
clear advantage (pathname character set and file contents
character set are often different, and when both are non-ascii
we would need to sacrifice one if we spit diff out in a single
text file --- we choose to quote pathnames so that it would not
interfere with file contents, which would be more important to
be inspectable by humans reading "diff" output).  People in the
past suggested converting everything to UTF-8 but that is not a
usable robust solution in the real world.

The HTML output generated by gitweb do not have the "machine
readably robust" requirement, so it _can_ stress more on
readability by humans, including converting both pathname and
contents to UTF-8; for this, the project needs to tell gitweb
what its pathname character encoding is, and blob contents
encoding, at the worst case blob-by-blob, but more often
path-by-path.

Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> Jakub Narebski <jnareb@gmail.com> writes:
>
>> ..., without
>> resolving the strange filenames problem (it would be nice if somebody was
>> to send code; well Junio send patch to address core git filename quoting
>> issue).
>
> Having showed that patch, I do not think it is a good way to go.
>
> I think the UI layer like gitweb should have freedom to choose
> its own pathname handling, and should read from -z output.
>
> The git plumbing is agnostic to the character encoding issues,
> and does not care what character set pathnames are encoded, and
> what character set the file contents are encoded.  These are
> very project specific and should be handled per project.

Side note.

There is one action item for shorter term I realized we would
need.  We need to make patch-id generation use unquoted
filenames, so that no matter how differently we might
reimplement quote_c_style() later, we would get the same
patch-id from the diff between the same two trees.  Currently I
think it hashes the quoted filename that appear on ---/+++ lines
(and "rename from"/"rename to" lines as well, but usually you do
not use -M/-C when computing the patch-id, so these are not
really an issue).

I think longer term plan for git should include standardizing on
how pathname encoding and content encoding are handled at the UI
layer.

And that plan _should_ eventually include the textual diff part.
But textual diff has a machine readability requirements, so we
need to proceed very carefully.  I think the patch I sent out
earlier is probably good enough if we only care about utf-8, but
would not correctly handle other sane pathname encodings such as
extended UNIX code (let alone broken but still widely used
encodings such as MS-Kanji aka Shift_JIS).

Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view

[Jakub Narebski]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> ..., without
>> resolving the strange filenames problem (it would be nice if somebody was
>> to send code; well Junio send patch to address core git filename quoting
>> issue).
> 
> Having showed that patch, I do not think it is a good way to go.
> 
> I think the UI layer like gitweb should have freedom to choose
> its own pathname handling, and should read from -z output.

That's a very good idea. I'll send separate patch (if noone else will
do this, that is) which would convert gitweb to always use -z output,
both git-ls-tree and git-diff-tree... oh, well, there is no -z patch
output, so in patch part we would have to replace git quoted part by
gitweb quoted part.

BTW gitweb has to do it's own pathname handling, at least LF, perhaps
TAB, perhaps other control characters like ESC; quote quoting character
and mark filename as quoted somewhat (it can be with CSS style, not
necessary by surrounding with some kind of quotes; besides that can
also be added via CSS in modern enough browsers using :before and
:after pseudo-elements and 'content' property).

-- 
Jakub Narebski

Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view

[Jakub Narebski]

Jakub Narebski wrote:
> Junio C Hamano wrote:
>>
>> I think the UI layer like gitweb should have freedom to choose
>> its own pathname handling, and should read from -z output.
> 
> That's a very good idea. I'll send separate patch (if noone else will
> do this, that is) which would convert gitweb to always use -z output,
> both git-ls-tree and git-diff-tree... oh, well, there is no -z patch
> output, so in patch part we would have to replace git quoted part by
> gitweb quoted part.

I have realized that it is not as easy as it sounds, at least for the
git-diff-tree output. For the LF-terminated output (without '-z') you
know that LF separates records, and you can split on LF ('\n'). It is
not the case for '-z' '\0' delimited output: NUL ('\0') might also mean
end of one of the filenames in the rename/copy case, and is used to 
separate filename(s) from the score (although here TAB would be 
enough). And that is probably the case that gitweb uses default 
git-diff-tree output, and _tries_ to unescape(...) filename.

The solution would be perhaps to add '--zz' option to use '-z' output 
but to separate records by double NUL, i.e. '\0\0'...

By the way, why diff-tree "raw" format for merge gives only one, final,
filename?
-- 
Jakub Narebski

Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view

[Luben Tuikov]

--- Jakub Narebski <jnareb@gmail.com> wrote:
> > Wouldn't this be confusing with the other fine lines?
> > I personally don't like this chunk separation.  Chunk separation
> > already exists as is and we view it all the time elsewhere.
> 
> But not always the program displaying diff can display such line
> separating chunks, for example on text terminal it can't.

What I meant is that since I stare at diffs exactly on text terminals,
my eyes have found other ways to discern chunk blocks.

> But if you think that the dotted 1px #ffbbff line is too intrusive,
> we can remove it (and perhaps increase vertical space a few pixels).
> I'd like to have more opinions first.

No, I just think that it should be as close as possible to what
we see now and what we see on text terminals -- no extra vertical
space please.  Between the two evils, I'd prefer the thin "dotted" line.

> BTW. you can easily override it in your CSS file.

Why should we allow something to go into gitweb and disrupt the current
default behavior only so that people have to change their own css file
to keep current default behaviour.  Please don't shove this down our
throats.  Please?

   Luben

Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view

[Jakub Narebski]

Luben Tuikov wrote:
> --- Jakub Narebski <jnareb@gmail.com> wrote:
>>> Wouldn't this be confusing with the other fine lines?
>>> I personally don't like this chunk separation.  Chunk separation
>>> already exists as is and we view it all the time elsewhere.
>> 
>> But not always the program displaying diff can display such line
>> separating chunks, for example on text terminal it can't.
> 
> What I meant is that since I stare at diffs exactly on text terminals,
> my eyes have found other ways to discern chunk blocks.

I'm just saying that with HTML diffs, presented via gitweb in graphical
web browser, you have more possibilities, more formatting to use.
Why not make use of it?

>> But if you think that the dotted 1px #ffbbff line is too intrusive,
>> we can remove it (and perhaps increase vertical space a few pixels).
>> I'd like to have more opinions first.
> 
> No, I just think that it should be as close as possible to what
> we see now and what we see on text terminals -- no extra vertical
> space please.  Between the two evils, I'd prefer the thin "dotted" line.

Well, I'll make it nearly invisible in the "take 3". BTW. some people
liked this line, some were indifferent.

>> BTW. you can easily override it in your CSS file.
> 
> Why should we allow something to go into gitweb and disrupt the current
> default behavior only so that people have to change their own css file
> to keep current default behaviour.  Please don't shove this down our
> throats.  Please?

That was just to note that if you don't agree with default, you can change
it very easily. It is probably the time where people would disagree (for
example infamous "redundant links" debate) on the gitweb UI; the possibility
to tailor it easily to your own UI concepts and ideas is in my opinion
very important (and very nice).

-- 
Jakub Narebski

Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view

[Edgar Toernig]

Jakub Narebski wrote:
>
> I'm just saying that with HTML diffs, presented via gitweb in graphical
> web browser, you have more possibilities, more formatting to use.

It would be nice though, when the gitweb output would be readable
on non css-capable browsers (i.e. w3m) too.  At the moment, gitweb
is mostly usable - the only problematic case is code and diffs.
These are presented via div-tags so in a non-css browser, all spaces
are collapsed thereby removing all indentation.  Couldn't code
fragments be presented via (styled) pre-tags for backward compatibility?
Pretty please :)

Btw, while the css version looks nice, Opera seems to have extreme
performance problems with gitweb's project page when there are a lot
of repositories.  I.e. trying to view http://gitweb.freedesktop.org/
brings my system to its knees.  Turning off style sheets cures it
but then diffs are unusable ...

Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view

[Jakub Narebski]

Edgar Toernig wrote:
> Jakub Narebski wrote:
>>
>> I'm just saying that with HTML diffs, presented via gitweb in graphical
>> web browser, you have more possibilities, more formatting to use.
> 
> It would be nice though, when the gitweb output would be readable
> on non css-capable browsers (i.e. w3m) too.  At the moment, gitweb
> is mostly usable - the only problematic case is code and diffs.
> These are presented via div-tags so in a non-css browser, all spaces
> are collapsed thereby removing all indentation.  Couldn't code
> fragments be presented via (styled) pre-tags for backward compatibility?
> Pretty please :)

Well, we replaced using s/ / /g with .pre class woth white-space: pre.
Perhaps we can go halfway, and add <pre>...</pre> wrapping line.
 
> Btw, while the css version looks nice, Opera seems to have extreme
> performance problems with gitweb's project page when there are a lot
> of repositories.  I.e. trying to view http://gitweb.freedesktop.org/
> brings my system to its knees.  Turning off style sheets cures it
> but then diffs are unusable ...

Strange. It's just a simple table. Could you and would you be able to
debug it further (e.g. by bisecting CSS)?

-- 
Jakub Narebski

Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view

[Luben Tuikov]

--- Jakub Narebski <jnareb@gmail.com> wrote:
> I'm just saying that with HTML diffs, presented via gitweb in graphical
> web browser, you have more possibilities, more formatting to use.
> Why not make use of it?

That sounds fine.

The question is where one draws the line.

> >> BTW. you can easily override it in your CSS file.
> > 
> > Why should we allow something to go into gitweb and disrupt the current
> > default behavior only so that people have to change their own css file
> > to keep current default behaviour.  Please don't shove this down our
> > throats.  Please?
> 
> That was just to note that if you don't agree with default, you can change
> it very easily. It is probably the time where people would disagree (for
> example infamous "redundant links" debate) on the gitweb UI; the possibility
> to tailor it easily to your own UI concepts and ideas is in my opinion
> very important (and very nice).

I would like to keep the visual default as stable as possible.

   Luben

Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> I have realized that it is not as easy as it sounds, at least for the
> git-diff-tree output...

I do not think so.a The output is designed to be machine
parsable.  See Documentation/diff-format.txt and also please
refer to 65a9289d:diff-helper.c for a sample code to see how to
parse the output.

Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view

[Junio C Hamano]

Luben Tuikov <ltuikov@yahoo.com> writes:

>> That was just to note that if you don't agree with default, you can change
>> it very easily. It is probably the time where people would disagree (for
>> example infamous "redundant links" debate) on the gitweb UI; the possibility
>> to tailor it easily to your own UI concepts and ideas is in my opinion
>> very important (and very nice).
>
> I would like to keep the visual default as stable as possible.
>
>    Luben

Seconded.

Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view

[Jakub Narebski]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> I have realized that it is not as easy as it sounds, at least for the
>> git-diff-tree output...
> 
> I do not think so.a The output is designed to be machine
> parsable.  See Documentation/diff-format.txt and also please
> refer to 65a9289d:diff-helper.c for a sample code to see how to
> parse the output.

What I wanted to say is that the output is not easy to use in
"split first, parse later", with sometimes counting number of entries
in between. With current format you rather have to pass in one go,
for '-z' format reading filenames from the stream.

By the way, git-ls-tree -z uses TAB as a start of filename, while
git-diff-tree -z uses NUL as a start of (first) filename. Why?
-- 
Jakub Narebski

Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> By the way, git-ls-tree -z uses TAB as a start of filename, while
> git-diff-tree -z uses NUL as a start of (first) filename. Why?

I do not know a good explanation other than hysterical raisins.

Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view

[Edgar Toernig]

Jakub Narebski wrote:
>
> Edgar Toernig wrote:
>  
> > Btw, while the css version looks nice, Opera seems to have extreme
> > performance problems with gitweb's project page when there are a lot
> > of repositories.  I.e. trying to view http://gitweb.freedesktop.org/
> > brings my system to its knees.  Turning off style sheets cures it
> > but then diffs are unusable ...
> 
> Strange. It's just a simple table. Could you and would you be able to
> debug it further (e.g. by bisecting CSS)?

It's the combination of tr.light/dark:hover and background-color.
Changing the foreground instead of the background color is fast.
Maybe it recalculates the complete table when the background of
a table cell changes.

I've reported the problem to Opera ...