[RFC/PATCH 0/3] gitweb: Refactor common parts of log-like views

[RFC/PATCH 0/3] gitweb: Refactor common parts of log-like views

[Jakub Narebski]

This series extracts common parts of log-like views: 'log', 'shortlog'
and 'history' view, into git_log_generic() subroutine.  This
unification of code reduces code duplication, and brings features such
as limiting the list of commits displayed using $hash_parent{,_base}
from 'shortlog' view (added in ec3e97b (gitweb: shortlog now also
obeys $hash_parent, 2008-08-08)).

I have tested this series a bit (it passes both t9500 and t9501 test),
but I didn't test all the corner cases.

This series is preparation for even more refactoring planned, to make
it easier to introduce new log-like action (new log formats).  It
doesn't include (for now) the 'search' view, neither in message search
not in pickaxe search form.

Jakub Narebski (3):
  gitweb: Refactor 'log' action generation, adding git_log_body()
  gitweb: Refactor common parts of 'log' and 'shortlog' views
  gitweb: Make 'history' view (re)use git_log_generic()

 gitweb/gitweb.perl |  269 +++++++++++++++++++++-------------------------------
 1 files changed, 110 insertions(+), 159 deletions(-)

-- 
Jakub Narebski

[PATCH 1/3] gitweb: Refactor 'log' action generation, adding git_log_body()

[Jakub Narebski]

Put the main part of 'log' view generation into git_log_body,
similarly how it is done for 'shortlog' and 'history' views (and
also for 'tags' and 'heads' views).

This is preparation for extracting common code between 'log',
'shortlog' and 'history' actions.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This is pure refactoring: output should change.

 gitweb/gitweb.perl |   81 ++++++++++++++++++++++++++++++---------------------
 1 files changed, 48 insertions(+), 33 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index e4cbfc3..68dbd9e 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -4361,6 +4361,46 @@ sub git_project_list_body {
 	print "</table>\n";
 }
 
+sub git_log_body {
+	# uses global variable $project
+	my ($commitlist, $from, $to, $refs, $extra) = @_;
+
+	$from = 0 unless defined $from;
+	$to = $#{$commitlist} if (!defined $to || $#{$commitlist} < $to);
+
+	for (my $i = 0; $i <= $to; $i++) {
+		my %co = %{$commitlist->[$i]};
+		next if !%co;
+		my $commit = $co{'id'};
+		my $ref = format_ref_marker($refs, $commit);
+		my %ad = parse_date($co{'author_epoch'});
+		git_print_header_div('commit',
+		               "<span class=\"age\">$co{'age_string'}</span>" .
+		               esc_html($co{'title'}) . $ref,
+		               $commit);
+		print "<div class=\"title_text\">\n" .
+		      "<div class=\"log_link\">\n" .
+		      $cgi->a({-href => href(action=>"commit", hash=>$commit)}, "commit") .
+		      " | " .
+		      $cgi->a({-href => href(action=>"commitdiff", hash=>$commit)}, "commitdiff") .
+		      " | " .
+		      $cgi->a({-href => href(action=>"tree", hash=>$commit, hash_base=>$commit)}, "tree") .
+		      "<br/>\n" .
+		      "</div>\n";
+		      git_print_authorship(\%co, -tag => 'span');
+		      print "<br/>\n</div>\n";
+
+		print "<div class=\"log_body\">\n";
+		git_print_log($co{'comment'}, -final_empty_line=> 1);
+		print "</div>\n";
+	}
+	if ($extra) {
+		print "<div class=\"page_nav\">\n";
+		print "$extra\n";
+		print "</div>\n";
+	}
+}
+
 sub git_shortlog_body {
 	# uses global variable $project
 	my ($commitlist, $from, $to, $refs, $extra) = @_;
@@ -5309,7 +5349,12 @@ sub git_log {
 	my @commitlist = parse_commits($hash, 101, (100 * $page));
 
 	my $paging_nav = format_paging_nav('log', $hash, $head, $page, $#commitlist >= 100);
-
+	my $next_link;
+	if ($#commitlist >= 100) {
+		$next_link =
+			$cgi->a({-href => href(-replay=>1, page=>$page+1),
+			         -accesskey => "n", -title => "Alt-n"}, "next");
+	}
 	my ($patch_max) = gitweb_get_feature('patches');
 	if ($patch_max) {
 		if ($patch_max < 0 || @commitlist <= $patch_max) {
@@ -5328,39 +5373,9 @@ sub git_log {
 		git_print_header_div('summary', $project);
 		print "<div class=\"page_body\"> Last change $co{'age_string'}.<br/><br/></div>\n";
 	}
-	my $to = ($#commitlist >= 99) ? (99) : ($#commitlist);
-	for (my $i = 0; $i <= $to; $i++) {
-		my %co = %{$commitlist[$i]};
-		next if !%co;
-		my $commit = $co{'id'};
-		my $ref = format_ref_marker($refs, $commit);
-		my %ad = parse_date($co{'author_epoch'});
-		git_print_header_div('commit',
-		               "<span class=\"age\">$co{'age_string'}</span>" .
-		               esc_html($co{'title'}) . $ref,
-		               $commit);
-		print "<div class=\"title_text\">\n" .
-		      "<div class=\"log_link\">\n" .
-		      $cgi->a({-href => href(action=>"commit", hash=>$commit)}, "commit") .
-		      " | " .
-		      $cgi->a({-href => href(action=>"commitdiff", hash=>$commit)}, "commitdiff") .
-		      " | " .
-		      $cgi->a({-href => href(action=>"tree", hash=>$commit, hash_base=>$commit)}, "tree") .
-		      "<br/>\n" .
-		      "</div>\n";
-		      git_print_authorship(\%co, -tag => 'span');
-		      print "<br/>\n</div>\n";
 
-		print "<div class=\"log_body\">\n";
-		git_print_log($co{'comment'}, -final_empty_line=> 1);
-		print "</div>\n";
-	}
-	if ($#commitlist >= 100) {
-		print "<div class=\"page_nav\">\n";
-		print $cgi->a({-href => href(-replay=>1, page=>$page+1),
-			       -accesskey => "n", -title => "Alt-n"}, "next");
-		print "</div>\n";
-	}
+	git_log_body(\@commitlist, 0, 99, $refs, $next_link);
+
 	git_footer_html();
 }
 
-- 
1.6.5

[PATCH 2/3] gitweb: Refactor common parts of 'log' and 'shortlog' views

[Jakub Narebski]

Put the common parts of git_log and git_shortlog into git_log_generic
subroutine: git_log and git_shortlog are now thin wrappers calling
git_log_generic with appropriate arguments.

The unification of code responsible for 'log' and 'shorlog' actions
lead to the following changes in gitweb output
 * 'tree' link in page_nav now uses $hash parameter, as was the case
   for 'shortlog' but not for 'log'
 * 'log' view now respect $hash_parent limiting, like 'shortlog' did
 * 'log' view doesn't have special case for empty list anymore, and it
   always uses page_header linking to summary view, like 'shortlog'
   did.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 68dbd9e..14661cc 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5336,7 +5336,9 @@ sub git_snapshot {
 	close $fd;
 }
 
-sub git_log {
+sub git_log_generic {
+	my ($fmt_name, $body_subr) = @_;
+
 	my $head = git_get_head_hash($project);
 	if (!defined $hash) {
 		$hash = $head;
@@ -5346,16 +5348,21 @@ sub git_log {
 	}
 	my $refs = git_get_references();
 
-	my @commitlist = parse_commits($hash, 101, (100 * $page));
+	my $commit_hash = $hash;
+	if (defined $hash_parent) {
+		$commit_hash = "$hash_parent..$hash";
+	}
+	my @commitlist = parse_commits($commit_hash, 101, (100 * $page));
 
-	my $paging_nav = format_paging_nav('log', $hash, $head, $page, $#commitlist >= 100);
-	my $next_link;
+	my $paging_nav = format_paging_nav($fmt_name, $hash, $head,
+	                                   $page, $#commitlist >= 100);
+	my $next_link = '';
 	if ($#commitlist >= 100) {
 		$next_link =
 			$cgi->a({-href => href(-replay=>1, page=>$page+1),
 			         -accesskey => "n", -title => "Alt-n"}, "next");
 	}
-	my ($patch_max) = gitweb_get_feature('patches');
+	my $patch_max = gitweb_get_feature('patches');
 	if ($patch_max) {
 		if ($patch_max < 0 || @commitlist <= $patch_max) {
 			$paging_nav .= " &sdot; " .
@@ -5365,20 +5372,18 @@ sub git_log {
 	}
 
 	git_header_html();
-	git_print_page_nav('log','', $hash,undef,undef, $paging_nav);
-
-	if (!@commitlist) {
-		my %co = parse_commit($hash);
-
-		git_print_header_div('summary', $project);
-		print "<div class=\"page_body\"> Last change $co{'age_string'}.<br/><br/></div>\n";
-	}
+	git_print_page_nav($fmt_name,'', $hash,$hash,$hash, $paging_nav);
+	git_print_header_div('summary', $project);
 
-	git_log_body(\@commitlist, 0, 99, $refs, $next_link);
+	$body_subr->(\@commitlist, 0, 99, $refs, $next_link);
 
 	git_footer_html();
 }
 
+sub git_log {
+	git_log_generic('log', \&git_log_body);
+}
+
 sub git_commit {
 	$hash ||= $hash_base || "HEAD";
 	my %co = parse_commit($hash)
@@ -6242,44 +6247,7 @@ EOT
 }
 
 sub git_shortlog {
-	my $head = git_get_head_hash($project);
-	if (!defined $hash) {
-		$hash = $head;
-	}
-	if (!defined $page) {
-		$page = 0;
-	}
-	my $refs = git_get_references();
-
-	my $commit_hash = $hash;
-	if (defined $hash_parent) {
-		$commit_hash = "$hash_parent..$hash";
-	}
-	my @commitlist = parse_commits($commit_hash, 101, (100 * $page));
-
-	my $paging_nav = format_paging_nav('shortlog', $hash, $head, $page, $#commitlist >= 100);
-	my $next_link = '';
-	if ($#commitlist >= 100) {
-		$next_link =
-			$cgi->a({-href => href(-replay=>1, page=>$page+1),
-			         -accesskey => "n", -title => "Alt-n"}, "next");
-	}
-	my $patch_max = gitweb_check_feature('patches');
-	if ($patch_max) {
-		if ($patch_max < 0 || @commitlist <= $patch_max) {
-			$paging_nav .= " &sdot; " .
-				$cgi->a({-href => href(action=>"patches", -replay=>1)},
-					"patches");
-		}
-	}
-
-	git_header_html();
-	git_print_page_nav('shortlog','', $hash,$hash,$hash, $paging_nav);
-	git_print_header_div('summary', $project);
-
-	git_shortlog_body(\@commitlist, 0, 99, $refs, $next_link);
-
-	git_footer_html();
+	git_log_generic('shortlog', \&git_shortlog_body);
 }
 
 ## ......................................................................
-- 
1.6.5

[PATCH 3/3] gitweb: Make 'history' view (re)use git_log_generic()

[Jakub Narebski]

Make git_history use git_log_generic, passing git_history_body as one
of its paramaters.  This required changes to git_log_generic, in
particular passing more things as parameters.

While refactoring common code of 'log', 'shortlog' and 'history' view,
we did unify pagination, using always the form used by 'history' view,
namely
  first * prev * next
in place of
  HEAD * prev * next
used by 'log' and 'shortlog' views.

The 'history' view now supports commit limiting via 'hpb' parameter,
similarly to 'shortlog' (and 'log') view.  Performance of 'history'
view got improved a bit, as it doesn't run git_get_hash_by_path for
"current" version in a loop.  Error detection and reporting for
'history' view changed a bit.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 14661cc..5cfa507 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3363,22 +3363,18 @@ sub git_print_page_nav {
 }
 
 sub format_paging_nav {
-	my ($action, $hash, $head, $page, $has_next_link) = @_;
+	my ($action, $page, $has_next_link) = @_;
 	my $paging_nav;
 
 
-	if ($hash ne $head || $page) {
-		$paging_nav .= $cgi->a({-href => href(action=>$action)}, "HEAD");
-	} else {
-		$paging_nav .= "HEAD";
-	}
-
 	if ($page > 0) {
-		$paging_nav .= " &sdot; " .
+		$paging_nav .=
+			$cgi->a({-href => href(-replay=>1, page=>undef)}, "first") .
+			" &sdot; " .
 			$cgi->a({-href => href(-replay=>1, page=>$page-1),
 			         -accesskey => "p", -title => "Alt-p"}, "prev");
 	} else {
-		$paging_nav .= " &sdot; prev";
+		$paging_nav .= "first &sdot; prev";
 	}
 
 	if ($has_next_link) {
@@ -4447,7 +4443,8 @@ sub git_shortlog_body {
 
 sub git_history_body {
 	# Warning: assumes constant type (blob or tree) during history
-	my ($commitlist, $from, $to, $refs, $hash_base, $ftype, $extra) = @_;
+	my ($commitlist, $from, $to, $refs, $extra,
+	    $file_name, $file_hash, $ftype) = @_;
 
 	$from = 0 unless defined $from;
 	$to = $#{$commitlist} unless (defined $to && $to <= $#{$commitlist});
@@ -4481,7 +4478,7 @@ sub git_history_body {
 		      $cgi->a({-href => href(action=>"commitdiff", hash=>$commit)}, "commitdiff");
 
 		if ($ftype eq 'blob') {
-			my $blob_current = git_get_hash_by_path($hash_base, $file_name);
+			my $blob_current = $file_hash;
 			my $blob_parent  = git_get_hash_by_path($commit, $file_name);
 			if (defined $blob_current && defined $blob_parent &&
 					$blob_current ne $blob_parent) {
@@ -5337,25 +5334,48 @@ sub git_snapshot {
 }
 
 sub git_log_generic {
-	my ($fmt_name, $body_subr) = @_;
+	my ($fmt_name, $body_subr, $base, $parent, $file_name, $file_hash) = @_;
 
 	my $head = git_get_head_hash($project);
-	if (!defined $hash) {
-		$hash = $head;
+	if (!defined $base) {
+		$base = $head;
 	}
 	if (!defined $page) {
 		$page = 0;
 	}
 	my $refs = git_get_references();
 
-	my $commit_hash = $hash;
-	if (defined $hash_parent) {
-		$commit_hash = "$hash_parent..$hash";
+	my $commit_hash = $base;
+	if (defined $parent) {
+		$commit_hash = "$parent..$base";
+	}
+	my @commitlist =
+		parse_commits($commit_hash, 101, (100 * $page),
+		              defined $file_name ? ($file_name, "--full-history") : ());
+
+	my $ftype;
+	if (!defined $file_hash && defined $file_name) {
+		# some commits could have deleted file in question,
+		# and not have it in tree, but one of them has to have it
+		for (my $i = 0; $i < @commitlist; $i++) {
+			$file_hash = git_get_hash_by_path($commitlist[$i]{'id'}, $file_name);
+			last if defined $file_hash;
+		}
+	}
+	if (defined $file_hash) {
+		$ftype = git_get_type($file_hash);
+	}
+	if (defined $file_name && !defined $ftype) {
+		die_error(500, "Unknown type of object");
+	}
+	my %co;
+	if (defined $file_name) {
+		%co = parse_commit($base)
+			or die_error(404, "Unknown commit object");
 	}
-	my @commitlist = parse_commits($commit_hash, 101, (100 * $page));
 
-	my $paging_nav = format_paging_nav($fmt_name, $hash, $head,
-	                                   $page, $#commitlist >= 100);
+
+	my $paging_nav = format_paging_nav($fmt_name, $page, $#commitlist >= 100);
 	my $next_link = '';
 	if ($#commitlist >= 100) {
 		$next_link =
@@ -5363,7 +5383,7 @@ sub git_log_generic {
 			         -accesskey => "n", -title => "Alt-n"}, "next");
 	}
 	my $patch_max = gitweb_get_feature('patches');
-	if ($patch_max) {
+	if ($patch_max && !defined $file_name) {
 		if ($patch_max < 0 || @commitlist <= $patch_max) {
 			$paging_nav .= " &sdot; " .
 				$cgi->a({-href => href(action=>"patches", -replay=>1)},
@@ -5373,15 +5393,23 @@ sub git_log_generic {
 
 	git_header_html();
 	git_print_page_nav($fmt_name,'', $hash,$hash,$hash, $paging_nav);
-	git_print_header_div('summary', $project);
+	if (defined $file_name) {
+		git_print_header_div('commit', esc_html($co{'title'}), $base);
+	} else {
+		git_print_header_div('summary', $project)
+	}
+	git_print_page_path($file_name, $ftype, $hash_base)
+		if (defined $file_name);
 
-	$body_subr->(\@commitlist, 0, 99, $refs, $next_link);
+	$body_subr->(\@commitlist, 0, 99, $refs, $next_link,
+	             $file_name, $file_hash, $ftype);
 
 	git_footer_html();
 }
 
 sub git_log {
-	git_log_generic('log', \&git_log_body);
+	git_log_generic('log', \&git_log_body,
+	                $hash, $hash_parent);
 }
 
 sub git_commit {
@@ -5920,70 +5948,9 @@ sub git_patches {
 }
 
 sub git_history {
-	if (!defined $hash_base) {
-		$hash_base = git_get_head_hash($project);
-	}
-	if (!defined $page) {
-		$page = 0;
-	}
-	my $ftype;
-	my %co = parse_commit($hash_base)
-	    or die_error(404, "Unknown commit object");
-
-	my $refs = git_get_references();
-	my $limit = sprintf("--max-count=%i", (100 * ($page+1)));
-
-	my @commitlist = parse_commits($hash_base, 101, (100 * $page),
-	                               $file_name, "--full-history")
-	    or die_error(404, "No such file or directory on given branch");
-
-	if (!defined $hash && defined $file_name) {
-		# some commits could have deleted file in question,
-		# and not have it in tree, but one of them has to have it
-		for (my $i = 0; $i <= @commitlist; $i++) {
-			$hash = git_get_hash_by_path($commitlist[$i]{'id'}, $file_name);
-			last if defined $hash;
-		}
-	}
-	if (defined $hash) {
-		$ftype = git_get_type($hash);
-	}
-	if (!defined $ftype) {
-		die_error(500, "Unknown type of object");
-	}
-
-	my $paging_nav = '';
-	if ($page > 0) {
-		$paging_nav .=
-			$cgi->a({-href => href(action=>"history", hash=>$hash, hash_base=>$hash_base,
-			                       file_name=>$file_name)},
-			        "first");
-		$paging_nav .= " &sdot; " .
-			$cgi->a({-href => href(-replay=>1, page=>$page-1),
-			         -accesskey => "p", -title => "Alt-p"}, "prev");
-	} else {
-		$paging_nav .= "first";
-		$paging_nav .= " &sdot; prev";
-	}
-	my $next_link = '';
-	if ($#commitlist >= 100) {
-		$next_link =
-			$cgi->a({-href => href(-replay=>1, page=>$page+1),
-			         -accesskey => "n", -title => "Alt-n"}, "next");
-		$paging_nav .= " &sdot; $next_link";
-	} else {
-		$paging_nav .= " &sdot; next";
-	}
-
-	git_header_html();
-	git_print_page_nav('history','', $hash_base,$co{'tree'},$hash_base, $paging_nav);
-	git_print_header_div('commit', esc_html($co{'title'}), $hash_base);
-	git_print_page_path($file_name, $ftype, $hash_base);
-
-	git_history_body(\@commitlist, 0, 99,
-	                 $refs, $hash_base, $ftype, $next_link);
-
-	git_footer_html();
+	git_log_generic('history', \&git_history_body,
+	                $hash_base, $hash_parent_base,
+	                $file_name, $hash);
 }
 
 sub git_search {
@@ -6247,7 +6214,8 @@ EOT
 }
 
 sub git_shortlog {
-	git_log_generic('shortlog', \&git_shortlog_body);
+	git_log_generic('shortlog', \&git_shortlog_body,
+	                $hash, $hash_parent);
 }
 
 ## ......................................................................
-- 
1.6.5

Re: [PATCH 1/3] gitweb: Refactor 'log' action generation, adding git_log_body()

[Nanako Shiraishi]

Quoting Jakub Narebski <jnareb@gmail.com>

> Put the main part of 'log' view generation into git_log_body,
> similarly how it is done for 'shortlog' and 'history' views (and
> also for 'tags' and 'heads' views).
>
> This is preparation for extracting common code between 'log',
> 'shortlog' and 'history' actions.
>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> ---
> This is pure refactoring: output should change.

Did you mean "should not change"?

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Re: [PATCH 1/3] gitweb: Refactor 'log' action generation, adding git_log_body()

[Jakub Narebski]

On Sat, 14 Nov 2009, Nanako Shiraishi wrote:
> Quoting Jakub Narebski <jnareb@gmail.com>
> 
> > Put the main part of 'log' view generation into git_log_body,
> > similarly how it is done for 'shortlog' and 'history' views (and
> > also for 'tags' and 'heads' views).
> >
> > This is preparation for extracting common code between 'log',
> > 'shortlog' and 'history' actions.
> >
> > Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> > ---
> > This is pure refactoring: output should change.
> 
> Did you mean "should not change"?

Yes, of course, it should be "output should not change" (as opposed
to later patches, which unify not only code but some of looks).

-- 
Jakub Narebski
Poland