[PATCH 0/6] gitweb: Further refactoring

[PATCH 0/6] gitweb: Further refactoring

[Jakub Narebski]

This series of patches, on top of my merged series
'[PATCH 7/5] Merge changes in "split patch 1" series'
although probably would apply on top of 'next'.

 * [PATCH 1/5] gitweb: Refactor untabifying - converting tabs to spaces
 * [PATCH 2/6] gitweb: Simplify git_diff_print
 * [PATCH 3/6] gitweb: Remove unused parse_date invocation from
   git_shortlog_body
 * [PATCH 4/6] gitweb: Make blob diff -p1 like commit diff
 * [PATCH 5/6] gitweb: Refactor printing shortened title in
   git_shortlog_body and git_tags_body
 * [PATCH 6/6] gitweb: Refactor git_history_body

-- 
Jakub Narebski
Poland

[PATCH 1/6] gitweb: Refactor untabifying - converting tabs to spaces

[Jakub Narebski]

Add untabify subroutine and use it.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index bf1b10f..60113da 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -222,6 +222,21 @@ sub unquote {
 	return $str;
 }
 
+# escape tabs (convert tabs to spaces)
+sub untabify {
+	my $line = shift;
+
+	while ((my $pos = index($line, "\t")) != -1) {
+		# one out of three implementations used ($pos - 1) instead of $pos
+		if (my $count = (8 - ($pos % 8))) {
+			my $spaces = ' ' x $count;
+			$line =~ s/\t/$spaces/;
+		}
+	}
+
+	return $line;
+}
+
 ## ----------------------------------------------------------------------
 ## HTML aware string manipulation
 
@@ -1255,12 +1270,7 @@ sub git_diff_print {
 				# skip errors
 				next;
 			}
-			while ((my $pos = index($line, "\t")) != -1) {
-				if (my $count = (8 - (($pos-1) % 8))) {
-					my $spaces = ' ' x $count;
-					$line =~ s/\t/$spaces/;
-				}
-			}
+			$line = untabify($line);
 			print "<div class=\"diff$diff_class\">" . esc_html($line) . "</div>\n";
 		}
 	}
@@ -1600,13 +1610,8 @@ HTML
 		$age_class  = age_class($age);
 		$author     = esc_html ($author);
 		$author     =~ s/ /&nbsp;/g;
-		# escape tabs
-		while ((my $pos = index($data, "\t")) != -1) {
-			if (my $count = (8 - ($pos % 8))) {
-				my $spaces = ' ' x $count;
-				$data =~ s/\t/$spaces/;
-			}
-		}
+
+		$data = untabify($data);
 		$data = esc_html ($data);
 
 		print <<HTML;
@@ -1728,12 +1733,7 @@ sub git_blob {
 	while (my $line = <$fd>) {
 		chomp $line;
 		$nr++;
-		while ((my $pos = index($line, "\t")) != -1) {
-			if (my $count = (8 - ($pos % 8))) {
-				my $spaces = ' ' x $count;
-				$line =~ s/\t/$spaces/;
-			}
-		}
+		$line = untabify($line);
 		printf "<div class=\"pre\"><a id=\"l%i\" href=\"#l%i\" class=\"linenr\">%4i</a> %s</div>\n", $nr, $nr, $nr, esc_html($line);
 	}
 	close $fd or print "Reading blob failed.\n";
-- 
1.4.1.1

[PATCH 2/6] gitweb: Simplify git_diff_print

[Jakub Narebski]

Copy to temporaty file more directly, not using temporary variable @file.
Use list form of open for diff invocation (we cannot use git-diff because
first it doesn't support -L/--label option, and we cannot generate diff
between /dev/null and blob given by it's sha1 identifier). 

Use "local $/ = undef;" for (temporary) slurp mode.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 60113da..b72f12f 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1230,31 +1230,31 @@ sub git_diff_print {
 
 	# create tmp from-file
 	if (defined $from) {
+		local $/ = undef;
 		$from_tmp = "$git_temp/gitweb_" . $$ . "_from";
 		open my $fd2, "> $from_tmp";
 		open my $fd, "-|", $GIT, "cat-file", "blob", $from;
-		my @file = <$fd>;
-		print $fd2 @file;
+		print { $fd2 } <$fd>;
 		close $fd2;
 		close $fd;
 	}
 
 	# create tmp to-file
 	if (defined $to) {
+		local $/ = undef;
 		$to_tmp = "$git_temp/gitweb_" . $$ . "_to";
 		open my $fd2, "> $to_tmp";
 		open my $fd, "-|", $GIT, "cat-file", "blob", $to;
-		my @file = <$fd>;
-		print $fd2 @file;
+		print { $fd2 } <$fd>;
 		close $fd2;
 		close $fd;
 	}
 
-	open my $fd, "-|", "/usr/bin/diff -u -p -L \'$from_name\' -L \'$to_name\' $from_tmp $to_tmp";
+	open my $fd, "-|", "/usr/bin/diff",
+		'-u', '-p', '-L', $from_name, '-L', $to_name, $from_tmp, $to_tmp;
 	if ($format eq "plain") {
-		undef $/;
+		local $/ = undef;
 		print <$fd>;
-		$/ = "\n";
 	} else {
 		while (my $line = <$fd>) {
 			chomp $line;
-- 
1.4.1.1

[PATCH 3/6] gitweb: Remove unused parse_date invocation from git_shortlog_body

[Jakub Narebski]

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Remainder from refactoring

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index b72f12f..cdce481 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1077,7 +1077,6 @@ sub git_shortlog_body {
 		#my $ref = defined $refs ? format_mark_referencing($refs, $commit) : '';
 		my $ref = format_mark_referencing($refs, $commit);
 		my %co = parse_commit($commit);
-		my %ad = parse_date($co{'author_epoch'});
 		if ($alternate) {
 			print "<tr class=\"dark\">\n";
 		} else {
-- 
1.4.1.1

[PATCH 4/6] gitweb: Make blob diff -p1 like commit diff

[Jakub Narebski]

Add 'a/' and 'b/' as prefix for blobdiff, blobdiff_plain, like it is
in commitdiff, commitdiff_plain.  Ensure that label for /dev/null is
actually /dev/null.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
For consistency.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index cdce481..f402c8f 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1236,6 +1236,8 @@ sub git_diff_print {
 		print { $fd2 } <$fd>;
 		close $fd2;
 		close $fd;
+	} else {
+		$from_name = "/dev/null";
 	}
 
 	# create tmp to-file
@@ -1247,6 +1249,8 @@ sub git_diff_print {
 		print { $fd2 } <$fd>;
 		close $fd2;
 		close $fd;
+	} else {
+		$to_name = "/dev/null";
 	}
 
 	open my $fd, "-|", "/usr/bin/diff",
@@ -2105,7 +2109,8 @@ sub git_blobdiff {
 	      " -> blob:" .
 	      $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blob;h=$hash;hb=$hash_base;f=$file_name")}, $hash) .
 	      "</div>\n";
-	git_diff_print($hash_parent, $file_name || $hash_parent, $hash, $file_name || $hash);
+	git_diff_print($hash_parent, 'a/' . ($file_name || $hash_parent),
+	               $hash,        'b/' . ($file_name || $hash));
 	print "</div>";
 	git_footer_html();
 }
@@ -2113,7 +2118,8 @@ sub git_blobdiff {
 sub git_blobdiff_plain {
 	mkdir($git_temp, 0700);
 	print $cgi->header(-type => "text/plain", -charset => 'utf-8');
-	git_diff_print($hash_parent, $file_name || $hash_parent, $hash, $file_name || $hash, "plain");
+	git_diff_print($hash_parent, 'a/' . ($file_name || $hash_parent),
+	               $hash,        'b/' . ($file_name || $hash), "plain");
 }
 
 sub git_commitdiff {
-- 
1.4.1.1

[PATCH 5/6] gitweb: Refactor printing shortened title in git_shortlog_body and git_tags_body

[Jakub Narebski]

Uses ugly workaround with $bold argument because style is not set via
CSS, but via presentation element <b>...</b>.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Probably needs better name, too

 gitweb/gitweb.perl |   39 +++++++++++++++++++++++----------------
 1 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index f402c8f..7ea52b1 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1062,6 +1062,27 @@ sub git_print_page_path {
 }
 
 ## ......................................................................
+## functions printing or outputting HTML: inline elements
+
+# print, perhaps shortened and with markers, title line
+sub print_title_html {
+	# $bold argument is workaround until style will be set via CSS
+	my ($long, $short, $query, $bold, $extra) = @_;
+	$extra = '' unless defined($extra);
+
+	if (length($short) < length($long)) {
+		print $cgi->a({-href => "$my_uri?" . esc_param($query),
+		               -class => "list", -title => $long},
+		      ($bold ? "<b>" : "") . esc_html($short) . $extra . ($bold ? "</b>" : ""));
+	} else {
+		print $cgi->a({-href => "$my_uri?" . esc_param($query),
+		               -class => "list"},
+		      ($bold ? "<b>" : "") . esc_html($long)  . $extra . ($bold ? "</b>" : ""));
+	}
+}
+
+
+## ......................................................................
 ## functions printing large fragments of HTML
 
 sub git_shortlog_body {
@@ -1087,15 +1108,7 @@ sub git_shortlog_body {
 		print "<td title=\"$co{'age_string_age'}\"><i>$co{'age_string_date'}</i></td>\n" .
 		      "<td><i>" . esc_html(chop_str($co{'author_name'}, 10)) . "</i></td>\n" .
 		      "<td>";
-		if (length($co{'title_short'}) < length($co{'title'})) {
-			print $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=commit;h=$commit"),
-			               -class => "list", -title => "$co{'title'}"},
-			      "<b>" . esc_html($co{'title_short'}) . "$ref</b>");
-		} else {
-			print $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=commit;h=$commit"),
-			               -class => "list"},
-			      "<b>" . esc_html($co{'title'}) . "$ref</b>");
-		}
+		print_title_html($co{'title'}, $co{'title_short'}, "p=$project;a=commit;h=$commit", 1, $ref);
 		print "</td>\n" .
 		      "<td class=\"link\">" .
 		      $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=commit;h=$commit")}, "commit") . " | " .
@@ -1141,13 +1154,7 @@ sub git_tags_body {
 		      "</td>\n" .
 		      "<td>";
 		if (defined $comment) {
-			if (length($comment_short) < length($comment)) {
-				print $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=tag;h=$tag{'id'}"),
-				               -class => "list", -title => $comment}, $comment_short);
-			} else {
-				print $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=tag;h=$tag{'id'}"),
-				               -class => "list"}, $comment);
-			}
+			print_title_html($comment, $comment_short, "p=$project;a=tag;h=$tag{'id'}");
 		}
 		print "</td>\n" .
 		      "<td class=\"selflink\">";
-- 
1.4.1.1

[PATCH 6/6] gitweb: Refactor git_history_body

[Jakub Narebski]

Separates main part of git_history into git_history_body subroutine,
and make output more similar to git_shortlog.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
We couldn't just modify git_shortlog, because git_shortlog reads full
(--max-count limited) output of rev-list to array, which is needed 
for dividing into pages and pagination navigation bar, while git_history
reads rev-list output line by line.

 gitweb/gitweb.perl |   93 ++++++++++++++++++++++++++++++++--------------------
 1 files changed, 57 insertions(+), 36 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 7ea52b1..2a39145 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1124,6 +1124,62 @@ sub git_shortlog_body {
 	print "</table>\n";
 }
 
+sub git_history_body {
+	# Warning: assumes constant type (blob or tree) during history
+	my ($fd, $refs, $hash_base, $ftype, $extra) = @_;
+
+	print "<table class=\"history\" cellspacing=\"0\">\n";
+	my $alternate = 0;
+	while (my $line = <$fd>) {
+		if ($line !~ m/^([0-9a-fA-F]{40})/) {
+			next;
+		}
+
+		my $commit = $1;
+		my %co = parse_commit($commit);
+		if (!%co) {
+			next;
+		}
+
+		my $ref = format_mark_referencing($refs, $commit);
+
+		if ($alternate) {
+			print "<tr class=\"dark\">\n";
+		} else {
+			print "<tr class=\"light\">\n";
+		}
+		$alternate ^= 1;
+		print "<td title=\"$co{'age_string_age'}\"><i>$co{'age_string_date'}</i></td>\n" .
+		      # shortlog uses      chop_str($co{'author_name'}, 10)
+		      "<td><i>" . esc_html(chop_str($co{'author_name'}, 15, 3)) . "</i></td>\n" .
+		      "<td>";
+		# originally git_history used chop_str($co{'title'}, 50)
+		print_title_html($co{'title'}, $co{'title_short'}, "p=$project;a=commit;h=$commit", 1, $ref);
+		print "</td>\n" .
+		      "<td class=\"link\">" .
+		      $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=commit;h=$commit")}, "commit") . " | " .
+		      $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=commitdiff;h=$commit")}, "commitdiff") . " | " .
+		      $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=$ftype;hb=$commit;f=$file_name")}, $ftype);
+
+		my $blob_current = git_get_hash_by_path($hash_base, $file_name);
+		my $blob_parent  = git_get_hash_by_path($commit, $file_name);
+		if (defined $blob_current && defined $blob_parent &&
+				$blob_current ne $blob_parent) {
+			print " | " .
+				$cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blobdiff;h=$blob_current;hp=$blob_parent;hb=$commit;f=$file_name")},
+				"diff to current");
+		}
+		print "</td>\n" .
+		      "</tr>\n";
+	}
+	if (defined $extra) {
+		print "<tr>\n" .
+		      "<td colspan=\"4\">$extra</td>\n" .
+		      "</tr>\n";
+	}
+	print "</table>\n";
+}
+
 sub git_tags_body {
 	# uses global variable $project
 	my ($taglist, $from, $to, $extra) = @_;
@@ -2295,42 +2351,7 @@ sub git_history {
 
 	open my $fd, "-|",
 		$GIT, "rev-list", "--full-history", $hash_base, "--", $file_name;
-	print "<table cellspacing=\"0\">\n";
-	my $alternate = 0;
-	while (my $line = <$fd>) {
-		if ($line =~ m/^([0-9a-fA-F]{40})/){
-			my $commit = $1;
-			my %co = parse_commit($commit);
-			if (!%co) {
-				next;
-			}
-			my $ref = format_mark_referencing($refs, $commit);
-			if ($alternate) {
-				print "<tr class=\"dark\">\n";
-			} else {
-				print "<tr class=\"light\">\n";
-			}
-			$alternate ^= 1;
-			print "<td title=\"$co{'age_string_age'}\"><i>$co{'age_string_date'}</i></td>\n" .
-			      "<td><i>" . esc_html(chop_str($co{'author_name'}, 15, 3)) . "</i></td>\n" .
-			      "<td>" . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=commit;h=$commit"), -class => "list"}, "<b>" .
-			      esc_html(chop_str($co{'title'}, 50)) . "$ref</b>") . "</td>\n" .
-			      "<td class=\"link\">" .
-			      $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=commit;h=$commit")}, "commit") .
-			      " | " . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=commitdiff;h=$commit")}, "commitdiff") .
-			      " | " . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=$ftype;hb=$commit;f=$file_name")}, $ftype);
-			my $blob = git_get_hash_by_path($hash_base, $file_name);
-			my $blob_parent = git_get_hash_by_path($commit, $file_name);
-			if (defined $blob && defined $blob_parent && $blob ne $blob_parent) {
-				print " | " .
-				$cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blobdiff;h=$blob;hp=$blob_parent;hb=$commit;f=$file_name")},
-				"diff to current");
-			}
-			print "</td>\n" .
-			      "</tr>\n";
-		}
-	}
-	print "</table>\n";
+	git_history_body($fd, $refs, $hash_base, $ftype);
 	close $fd;
 	git_footer_html();
 }
-- 
1.4.1.1

Re: [PATCH 1/6] gitweb: Refactor untabifying - converting tabs to spaces

[Matthias Lederhofer]

Jakub Narebski <jnareb@gmail.com> wrote:
> +		# one out of three implementations used ($pos - 1) instead of $pos
Perhaps this should be in the commit message instead of the source?
I think no one will later understand this comment without the commit.

Re: [PATCH 2/6] gitweb: Simplify git_diff_print

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Copy to temporaty file more directly, not using temporary variable @file.
> Use list form of open for diff invocation (we cannot use git-diff because
> first it doesn't support -L/--label option, and we cannot generate diff
> between /dev/null and blob given by it's sha1 identifier). 
>
> Use "local $/ = undef;" for (temporary) slurp mode.
>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>

I think this part of the gitweb code predates the git-diff
support by the core level.  If I recall correctly, back then we
did not even have "git-diff-{tree,files,index}" when this part
was written.

I think you could use "git-diff $from $to" instead of using
temporary files and running /usr/bin/diff on it with today's
git.  Which is a big win from both security and ease of
administration point of view.  Not having to write anything into
temporary file from gitweb.cgi means you do not have to worry
about stray gitweb process leaving temporary files dangling
behind, for example.

You obviously would need to emulate the -L$from_name -L$to_name
part by hand in the loop that reads from the diff output.

One worrysome pathological case is what happens if files being
tracked happen to have names that consist of 40-byte hexdigits.
I think "git-diff $from $to --" should do the right thing for
gitweb use ($from and $to should not mistaken taken as pathnames
with the explicit -- disambiguator) and "git-diff -- $from $to"
also should do the right thing to take $from and $to as pathnames
even if there are revisions with the same object names, but
somebody may want to verify that (and send patches to fix if it
is not the case).

Re: [PATCH 0/6] gitweb: Further refactoring

[Junio C Hamano]

The earlier 9-series looked fine, so I intend to apply them all
to "next".  However, they were all tab damaged and it was very
painful to fix up the patch.

This series, PATCH 1/5 (6?) looks fine (it is a bugfix for
git_diff_print), but among others, one of them had a seriously
fishy preimage (e.g. as you can see in "next", I haven't
accepted the function rename patch but 3/6 talks about
format_mark_referencing), and at that point my patience ran out.

Please check what I pushed out on "next"/"pu" tonight, and
consider patches missing from there dropped on the floor for now
(which does not mean they are rejected, except for the one that
touches diff_print and still uses /usr/bin/diff, which I
commented on).  It probably is a good idea to make sure further
patches resent are based on (or at least apply cleanly to) the
commit f16db173a4680aebc2f14c103a1e125c3f4d4531.

Please note that I am primarily concentrating on polishing what
is in "master" to finish 1.4.2 for now, so I might have missed
breakage in these gitweb patches that I queued for "next".

Comments, tests and fixups are all very welcomed.

Re: [PATCH 2/6] gitweb: Simplify git_diff_print

[Jakub Narebski]

Junio C Hamano wrote:

> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> Copy to temporaty file more directly, not using temporary variable @file.
>> Use list form of open for diff invocation (we cannot use git-diff because
>> first it doesn't support -L/--label option, and we cannot generate diff
>> between /dev/null and blob given by it's sha1 identifier). 
>>
>> Use "local $/ = undef;" for (temporary) slurp mode.
>>
>> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> 
> I think this part of the gitweb code predates the git-diff
> support by the core level.  If I recall correctly, back then we
> did not even have "git-diff-{tree,files,index}" when this part
> was written.
> 
> I think you could use "git-diff $from $to" instead of using
> temporary files and running /usr/bin/diff on it with today's
> git.  Which is a big win from both security and ease of
> administration point of view.  Not having to write anything into
> temporary file from gitweb.cgi means you do not have to worry
> about stray gitweb process leaving temporary files dangling
> behind, for example.

If I remember gitweb code correctly, diff to /dev/null is called
_only_ in commitdiff, where we can use git-diff -p to generate patch
directly, instead of generating difftree and calling diff on entries.

Still it would be nice if git-diff could diff between blob and arbitrary
file on filesystem (including /dev/null), and acquired -L/--label option.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH 0/6] gitweb: Further refactoring

[Jakub Narebski]

Junio C Hamano wrote:

> The earlier 9-series looked fine, so I intend to apply them all
> to "next".  However, they were all tab damaged and it was very
> painful to fix up the patch.

I'm sorry for that. As far as I checked, KNode (news reader which I used for
sending 9-(micro)series) preserves tabs when attaching file inline. I guess
that GMane news-to-mail gateway might munge whitespace. I won't use KNode
to send patches, then.

By the way, can anyone tell me how to setup git-send-email to send email via
GMail (authenticated SMTP, via STARTTLS/SSL, on ports 25 or 465 or 587)?

> This series, PATCH 1/5 (6?) looks fine (it is a bugfix for
> git_diff_print), but among others, one of them had a seriously
> fishy preimage (e.g. as you can see in "next", I haven't
> accepted the function rename patch but 3/6 talks about
> format_mark_referencing), and at that point my patience ran out.

I _have_ notified that this 6-series is on top of previous series,
namely '[PATCH 7/5] Merge changes in "split patch 1" series' patch.

But I try to redo not accepted changes on top of next, and put
"gitweb: Great subroutines renaming" on top of series. (BTW. this 
is simple search'n'replace on subroutine names... well, perhaps after
1.4.2...).

[...]
> Please note that I am primarily concentrating on polishing what
> is in "master" to finish 1.4.2 for now, so I might have missed
> breakage in these gitweb patches that I queued for "next".

I'm trying to check my gitweb patches, by checking the output of all
possible actions, althought I think it doesn't encompass all the cases...

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git