[PATCH 0/5] gitweb: New patchset view corrections and improvements, and few other things

[PATCH 0/5] gitweb: New patchset view corrections and improvements, and few other things

[Jakub Narebski]

Table of contents:
 [PATCH 1/5] gitweb: Protect against possible warning in git_commitdiff
 [PATCH 2/5] gitweb: Buffer diff header to deal with split patches 
                     + git_patchset_body refactoring
 [PATCH 3/5] gitweb: New improved formatting of chunk header in diff
 [PATCH 4/5] gitweb: Default to $hash_base or HEAD for $hash
                     in "commit" and "commitdiff"
 [PATCH 5/5] gitweb: Add an option to href() to return full URL

Diffstat:
 gitweb/gitweb.css  |   13 +++
 gitweb/gitweb.perl |  264 ++++++++++++++++++++++++++++++++--------------------
 2 files changed, 176 insertions(+), 101 deletions(-)

-- 
Jakub Narebski

[PATCH 1/5] gitweb: Protect against possible warning in git_commitdiff

[Jakub Narebski]

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
I'm not sure _when_ this eror (warning) occur, as it happened to me
once, but I couldn't repeat it; but I didn't try too hard.

I think it is better coding practice, and more readable.

 gitweb/gitweb.perl |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 7587595..b2482fe 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3731,7 +3731,8 @@ sub git_commitdiff {
 			$hash_parent, $hash, "--"
 			or die_error(undef, "Open git-diff-tree failed");
 
-		while (chomp(my $line = <$fd>)) {
+		while (my $line = <$fd>) {
+			chomp $line;
 			# empty line ends raw part of diff-tree output
 			last unless $line;
 			push @difftree, $line;
-- 
1.4.3.4

[PATCH 2/5] gitweb: Buffer diff header to deal with split patches + git_patchset_body refactoring

[Jakub Narebski]

There are some cases when one line from "raw" git-diff output (raw format)
corresponds to more than one patch in the patchset git-diff output. To deal
with this buffer git diff header and extended diff header (everything up to
actual patch) to check from information from "index <hash>..<hash>" extended
header line if the patch corresponds to the same or next difftree raw line.

This could also be used to gather information needed for hyperlinking, and
used for printing gitweb quoted filenames, from extended diff header instead
of raw git-diff output.

While at it, refactor git_patchset_body subroutine from the event-driven,
AWK-like state-machine parsing to sequential parsing: for each patch
parse (and output) git diff header, parse extended diff header, parse two-line
from-file/to-file diff header, parse patch itself; patch ends with the end
of input [file] or the line matching m/^diff /.

For better understanding the code, there were added assertions in the
comments a la Carp::Assert module. Just in case there is commented out code
dealing with unexpected end of input (should not happen, hence commented
out).

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
While starting to write buffering part of this patch I have realized
that git_patchset_body needs refactoring. I think it is now much more
readable, at least for people who know the patch format.

 gitweb/gitweb.perl |  235 ++++++++++++++++++++++++++++++----------------------
 1 files changed, 136 insertions(+), 99 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index b2482fe..bf58c3b 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2202,31 +2202,56 @@ sub git_patchset_body {
 	my ($fd, $difftree, $hash, $hash_parent) = @_;
 
 	my $patch_idx = 0;
-	my $in_header = 0;
-	my $patch_found = 0;
+	my $patch_line;
 	my $diffinfo;
 	my (%from, %to);
+	my ($from_id, $to_id);
 
 	print "<div class=\"patchset\">\n";
 
-	LINE:
-	while (my $patch_line = <$fd>) {
+	# skip to first patch
+	while ($patch_line = <$fd>) {
 		chomp $patch_line;
 
-		if ($patch_line =~ m/^diff /) { # "git diff" header
-			# beginning of patch (in patchset)
-			if ($patch_found) {
-				# close extended header for previous empty patch
-				if ($in_header) {
-					print "</div>\n" # class="diff extended_header"
-				}
-				# close previous patch
-				print "</div>\n"; # class="patch"
-			} else {
-				# first patch in patchset
-				$patch_found = 1;
+		last if ($patch_line =~ m/^diff /);
+	}
+
+ PATCH:
+	while ($patch_line) {
+		my @diff_header;
+
+		# git diff header
+		#assert($patch_line =~ m/^diff /) if DEBUG;
+		#assert($patch_line !~ m!$/$!) if DEBUG; # is chomp-ed
+		push @diff_header, $patch_line;
+
+		# extended diff header
+	EXTENDED_HEADER:
+		while ($patch_line = <$fd>) {
+			chomp $patch_line;
+
+			last EXTENDED_HEADER if ($patch_line =~ m/^--- /);
+
+			if ($patch_line =~ m/^index ([0-9a-fA-F]{40})..([0-9a-fA-F]{40})/) {
+				$from_id = $1;
+				$to_id   = $2;
 			}
-			print "<div class=\"patch\" id=\"patch". ($patch_idx+1) ."\">\n";
+
+			push @diff_header, $patch_line;
+		}
+		#last PATCH unless $patch_line;
+		my $last_patch_line = $patch_line;
+
+		# check if current patch belong to current raw line
+		# and parse raw git-diff line if needed
+		if (defined $diffinfo &&
+		    $diffinfo->{'from_id'} eq $from_id &&
+		    $diffinfo->{'to_id'}   eq $to_id) {
+			# this is split patch
+			print "<div class=\"patch cont\">\n";
+		} else {
+			# advance raw git-diff output if needed
+			$patch_idx++ if defined $diffinfo;
 
 			# read and prepare patch information
 			if (ref($difftree->[$patch_idx]) eq "HASH") {
@@ -2247,100 +2272,112 @@ sub git_patchset_body {
 				                   hash=>$diffinfo->{'to_id'},
 				                   file_name=>$to{'file'});
 			}
-			$patch_idx++;
-
-			# print "git diff" header
-			$patch_line =~ s!^(diff (.*?) )"?a/.*$!$1!;
-			if ($from{'href'}) {
-				$patch_line .= $cgi->a({-href => $from{'href'}, -class => "path"},
-				                       'a/' . esc_path($from{'file'}));
-			} else { # file was added
-				$patch_line .= 'a/' . esc_path($from{'file'});
-			}
-			$patch_line .= ' ';
-			if ($to{'href'}) {
-				$patch_line .= $cgi->a({-href => $to{'href'}, -class => "path"},
-				                       'b/' . esc_path($to{'file'}));
-			} else { # file was deleted
-				$patch_line .= 'b/' . esc_path($to{'file'});
-			}
-
-			print "<div class=\"diff header\">$patch_line</div>\n";
-			print "<div class=\"diff extended_header\">\n";
-			$in_header = 1;
-			next LINE;
+			# this is first patch for raw difftree line with $patch_idx index
+			# we index @$difftree array from 0, but number patches from 1
+			print "<div class=\"patch\" id=\"patch". ($patch_idx+1) ."\">\n";
 		}
 
-		if ($in_header) {
-			if ($patch_line !~ m/^---/) {
-				# match <path>
-				if ($patch_line =~ s!^((copy|rename) from ).*$!$1! && $from{'href'}) {
-					$patch_line .= $cgi->a({-href=>$from{'href'}, -class=>"path"},
-					                        esc_path($from{'file'}));
-				}
-				if ($patch_line =~ s!^((copy|rename) to ).*$!$1! && $to{'href'}) {
-					$patch_line = $cgi->a({-href=>$to{'href'}, -class=>"path"},
-					                      esc_path($to{'file'}));
-				}
-				# match <mode>
-				if ($patch_line =~ m/\s(\d{6})$/) {
-					$patch_line .= '<span class="info"> (' .
-					               file_type_long($1) .
-					               ')</span>';
+		# print "git diff" header
+		$patch_line = shift @diff_header;
+		$patch_line =~ s!^(diff (.*?) )"?a/.*$!$1!;
+		if ($from{'href'}) {
+			$patch_line .= $cgi->a({-href => $from{'href'}, -class => "path"},
+			                       'a/' . esc_path($from{'file'}));
+		} else { # file was added
+			$patch_line .= 'a/' . esc_path($from{'file'});
+		}
+		$patch_line .= ' ';
+		if ($to{'href'}) {
+			$patch_line .= $cgi->a({-href => $to{'href'}, -class => "path"},
+			                       'b/' . esc_path($to{'file'}));
+		} else { # file was deleted
+			$patch_line .= 'b/' . esc_path($to{'file'});
+		}
+		print "<div class=\"diff header\">$patch_line</div>\n";
+
+		# print extended diff header
+		print "<div class=\"diff extended_header\">\n" if (@diff_header > 0);
+	EXTENDED_HEADER:
+		foreach $patch_line (@diff_header) {
+			# match <path>
+			if ($patch_line =~ s!^((copy|rename) from ).*$!$1! && $from{'href'}) {
+				$patch_line .= $cgi->a({-href=>$from{'href'}, -class=>"path"},
+				                        esc_path($from{'file'}));
+			}
+			if ($patch_line =~ s!^((copy|rename) to ).*$!$1! && $to{'href'}) {
+				$patch_line = $cgi->a({-href=>$to{'href'}, -class=>"path"},
+				                      esc_path($to{'file'}));
+			}
+			# match <mode>
+			if ($patch_line =~ m/\s(\d{6})$/) {
+				$patch_line .= '<span class="info"> (' .
+				               file_type_long($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=>"hash"},
+					                     substr($diffinfo->{'from_id'},0,7));
+				} else {
+					$from_link = '0' x 7;
 				}
-				# match <hash>
-				if ($patch_line =~ m/^index/) {
-					my ($from_link, $to_link);
-					if ($from{'href'}) {
-						$from_link = $cgi->a({-href=>$from{'href'}, -class=>"hash"},
-						                     substr($diffinfo->{'from_id'},0,7));
-					} else {
-						$from_link = '0' x 7;
-					}
-					if ($to{'href'}) {
-						$to_link = $cgi->a({-href=>$to{'href'}, -class=>"hash"},
-						                   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!;
+				if ($to{'href'}) {
+					$to_link = $cgi->a({-href=>$to{'href'}, -class=>"hash"},
+					                   substr($diffinfo->{'to_id'},0,7));
+				} else {
+					$to_link = '0' x 7;
 				}
-				print $patch_line . "<br/>\n";
-
-			} else {
-				#$in_header && $patch_line =~ m/^---/;
-				print "</div>\n"; # class="diff extended_header"
-				$in_header = 0;
+				#affirm {
+				#	my ($from_hash, $to_hash) =
+				#		($patch_line =~ m/^index ([0-9a-fA-F]{40})..([0-9a-fA-F]{40})/);
+				#	my ($from_id, $to_id) =
+				#		($diffinfo->{'from_id'}, $diffinfo->{'to_id'});
+				#	($from_hash eq $from_id) && ($to_hash eq $to_id);
+				#} if DEBUG;
+				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";
+		}
+		print "</div>\n"  if (@diff_header > 0); # class="diff extended_header"
+
+		# from-file/to-file diff header
+		$patch_line = $last_patch_line;
+		#assert($patch_line =~ m/^---/) if DEBUG;
+		if ($from{'href'}) {
+			$patch_line = '--- a/' .
+			              $cgi->a({-href=>$from{'href'}, -class=>"path"},
+			                      esc_path($from{'file'}));
+		}
+		print "<div class=\"diff from_file\">$patch_line</div>\n";
 
-				if ($from{'href'}) {
-					$patch_line = '--- a/' .
-					              $cgi->a({-href=>$from{'href'}, -class=>"path"},
-					                      esc_path($from{'file'}));
-				}
-				print "<div class=\"diff from_file\">$patch_line</div>\n";
+		$patch_line = <$fd>;
+		#last PATCH unless $patch_line;
+		chomp $patch_line;
 
-				$patch_line = <$fd>;
-				chomp $patch_line;
+		#assert($patch_line =~ m/^+++/) if DEBUG;
+		if ($to{'href'}) {
+			$patch_line = '+++ b/' .
+			              $cgi->a({-href=>$to{'href'}, -class=>"path"},
+			                      esc_path($to{'file'}));
+		}
+		print "<div class=\"diff to_file\">$patch_line</div>\n";
 
-				#$patch_line =~ m/^+++/;
-				if ($to{'href'}) {
-					$patch_line = '+++ b/' .
-					              $cgi->a({-href=>$to{'href'}, -class=>"path"},
-					                      esc_path($to{'file'}));
-				}
-				print "<div class=\"diff to_file\">$patch_line</div>\n";
+		# the patch itself
+	LINE:
+		while ($patch_line = <$fd>) {
+			chomp $patch_line;
 
-			}
+			next PATCH if ($patch_line =~ m/^diff /);
 
-			next LINE;
+			print format_diff_line($patch_line);
 		}
 
-		print format_diff_line($patch_line);
+	} continue {
+		print "</div>\n"; # class="patch"
 	}
-	print "</div>\n" if $in_header; # extended header
-
-	print "</div>\n" if $patch_found; # class="patch"
 
 	print "</div>\n"; # class="patchset"
 }
-- 
1.4.3.4

[PATCH 3/5] gitweb: New improved formatting of chunk header in diff

[Jakub Narebski]

If we have provided enough info, and diff is not combined diff,
and if provided diff line is chunk header, then:
* split chunk header into .chunk_info and .section span elements,
  first containing proper chunk header, second section heading
  (aka. which function), for separate styling: the proper chunk
  header is on non-white background, section heading part uses
  slightly lighter color.
* hyperlink from-file-range to starting line of from-file, if file
  was not created.
* hyperlink to-file-range to starting line of to-file, if file
  was not deleted.
Links are of invisible variety (and "list" class).

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This version deals correctly with hunks of form "@@ -n +m,l @@" and
friends. The number of lines in from/to part ($from_lines/$to_lines
variables) is not used.

 gitweb/gitweb.css  |   13 +++++++++++++
 gitweb/gitweb.perl |   23 ++++++++++++++++++++++-
 2 files changed, 35 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index 974b47f..7177c6e 100644
--- a/gitweb/gitweb.css
+++ b/gitweb/gitweb.css
@@ -334,11 +334,13 @@ div.diff.extended_header {
 	padding: 2px 0px 2px 0px;
 }
 
+div.diff a.list,
 div.diff a.path,
 div.diff a.hash {
 	text-decoration: none;
 }
 
+div.diff a.list:hover,
 div.diff a.path:hover,
 div.diff a.hash:hover {
 	text-decoration: underline;
@@ -362,14 +364,25 @@ div.diff.rem {
 	color: #cc0000;
 }
 
+div.diff.chunk_header a,
 div.diff.chunk_header {
 	color: #990099;
+}
 
+div.diff.chunk_header {
 	border: dotted #ffe0ff;
 	border-width: 1px 0px 0px 0px;
 	margin-top: 2px;
 }
 
+div.diff.chunk_header span.chunk_info {
+	background-color: #ffeeff;
+}
+
+div.diff.chunk_header span.section {
+	color: #aa22aa;
+}
+
 div.diff.incomplete {
 	color: #cccccc;
 }
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index bf58c3b..eadaa31 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -874,8 +874,10 @@ sub format_subject_html {
 	}
 }
 
+# format patch (diff) line (rather not to be used for diff headers)
 sub format_diff_line {
 	my $line = shift;
+	my ($from, $to) = @_;
 	my $char = substr($line, 0, 1);
 	my $diff_class = "";
 
@@ -891,6 +893,25 @@ sub format_diff_line {
 		$diff_class = " incomplete";
 	}
 	$line = untabify($line);
+	if ($from && $to && $line =~ m/^\@{2} /) {
+		my ($from_text, $from_start, $from_lines, $to_text, $to_start, $to_lines, $section) =
+			$line =~ m/^\@{2} (-(\d+)(?:,(\d+))?) (\+(\d+)(?:,(\d+))?) \@{2}(.*)$/;
+
+		$from_lines = 0 unless defined $from_lines;
+		$to_lines   = 0 unless defined $to_lines;
+
+		if ($from->{'href'}) {
+			$from_text = $cgi->a({-href=>"$from->{'href'}#l$from_start",
+			                     -class=>"list"}, $from_text);
+		}
+		if ($to->{'href'}) {
+			$to_text   = $cgi->a({-href=>"$to->{'href'}#l$to_start",
+			                     -class=>"list"}, $to_text);
+		}
+		$line = "<span class=\"chunk_info\">@@ $from_text $to_text @@</span>" .
+		        "<span class=\"section\">" . esc_html($section, -nbsp=>1) . "</span>";
+		return "<div class=\"diff$diff_class\">$line</div>\n";
+	}
 	return "<div class=\"diff$diff_class\">" . esc_html($line, -nbsp=>1) . "</div>\n";
 }
 
@@ -2372,7 +2393,7 @@ sub git_patchset_body {
 
 			next PATCH if ($patch_line =~ m/^diff /);
 
-			print format_diff_line($patch_line);
+			print format_diff_line($patch_line, \%from, \%to);
 		}
 
 	} continue {
-- 
1.4.3.4

[PATCH 4/5] gitweb: Default to $hash_base or HEAD for $hash in "commit" and "commitdiff"

[Jakub Narebski]

Set $hash parameter to $hash_base || "HEAD" if it is not set (if it is
not true to be more exact). This allows [hand-edited] URLs with 'action'
"commit" or "commitdiff" but without 'hash' parameter.

If there is 'h' (hash) parameter provided, then gitweb tries
to use this. HEAD is used _only_ if nether hash, nor hash_base
are provided, i.e. for URL like below
  URL?p=project.git;a=commit
i.e. without neither 'h' nor 'hb'.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index eadaa31..5875ba0 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3450,6 +3450,7 @@ sub git_log {
 }
 
 sub git_commit {
+	$hash ||= $hash_base || "HEAD";
 	my %co = parse_commit($hash);
 	if (!%co) {
 		die_error(undef, "Unknown commit object");
@@ -3727,6 +3728,7 @@ sub git_blobdiff_plain {
 
 sub git_commitdiff {
 	my $format = shift || 'html';
+	$hash ||= $hash_base || "HEAD";
 	my %co = parse_commit($hash);
 	if (!%co) {
 		die_error(undef, "Unknown commit object");
-- 
1.4.3.4

[PATCH 5/5] gitweb: Add an option to href() to return full URL

[Jakub Narebski]

href subroutine by default generates absolute URL (generated using
CGI::url(-absolute=>1), and saved in $my_uri) using $my_uri as base;
add an option to generate full URL using $my_url as base.

New feature usage: href(..., -full=>1)

To be used in web feeds (RSS, Atom, OPML) and other places which need
full URL rather than absolute or relative.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 5875ba0..8739501 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -459,7 +459,8 @@ exit;
 
 sub href(%) {
 	my %params = @_;
-	my $href = $my_uri;
+	# default is to use -absolute url() i.e. $my_uri
+	my $href = $params{-full} ? $my_url : $my_uri;
 
 	# XXX: Warning: If you touch this, check the search form for updating,
 	# too.
-- 
1.4.3.4

Re: [PATCH 1/5] gitweb: Protect against possible warning in git_commitdiff

[Jakub Narebski]

Possible _undefined value_ warning.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git