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

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