[PATCH/RFC] gitweb: Make git_print_log generic; git_print_simplified_log uses it

[PATCH/RFC] gitweb: Make git_print_log generic; git_print_simplified_log uses it

[Jakub Narebski]

Collapse git_print_log and git_print_simplified_log into one
subroutine git_print_log.  git_print_simplified_log now simply calls
git_print_log with proper options.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This patch doesn't change output any, but it makes it easy
to add signoff lines (to not remove them) to commitdiff and log views
(as requested by Linus).

The RFC is about style of git_print_log function. Is it a good idea
and good implementation to pass miscelaneus options as hash values
instead of using fixed order of parameters, and passing 1 or undef?
Is it a good naming of parameters like '-remove_title', or would
'remove_title' or 'remove-title' be better?


 gitweb/gitweb.perl |   63 ++++++++++++++++++++++++++--------------------------
 1 files changed, 31 insertions(+), 32 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index ae18c27..b6c1620 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1364,9 +1364,15 @@ sub git_print_page_path {
 	}
 }
 
-sub git_print_log {
+# sub git_print_log (\@;%) {
+sub git_print_log ($;%) {
 	my $log = shift;
+	my %opts = @_;
 
+	if ($opts{'-remove_title'}) {
+		# remove title, i.e. first line of log
+		shift @$log;
+	}
 	# remove leading empty lines
 	while (defined $log->[0] && $log->[0] eq "") {
 		shift @$log;
@@ -1376,6 +1382,19 @@ sub git_print_log {
 	my $signoff = 0;
 	my $empty = 0;
 	foreach my $line (@$log) {
+		if ($line =~ m/^ *(signed[ \-]off[ \-]by[ :]|acked[ \-]by[ :]|cc[ :])/i) {
+			$signoff = 1;
+			if (! $opts{'-remove_signoff'}) {
+				print "<span class=\"signoff\">" . esc_html($line) . "</span><br/>\n";
+				next;
+			} else {
+				# remove signoff lines
+				next;
+			}
+		} else {
+			$signoff = 0;
+		}
+
 		# print only one empty line
 		# do not print empty line after signoff
 		if ($line eq "") {
@@ -1384,13 +1403,13 @@ sub git_print_log {
 		} else {
 			$empty = 0;
 		}
-		if ($line =~ m/^ *(signed[ \-]off[ \-]by[ :]|acked[ \-]by[ :]|cc[ :])/i) {
-			$signoff = 1;
-			print "<span class=\"signoff\">" . esc_html($line) . "</span><br/>\n";
-		} else {
-			$signoff = 0;
-			print format_log_line_html($line) . "<br/>\n";
-		}
+
+		print format_log_line_html($line) . "<br/>\n";
+	}
+
+	if ($opts{'-final_empty_line'}) {
+		# end with single empty line
+		print "<br/>\n" unless $empty;
 	}
 }
 
@@ -1398,30 +1417,10 @@ sub git_print_simplified_log {
 	my $log = shift;
 	my $remove_title = shift;
 
-	shift @$log if $remove_title;
-	# remove leading empty lines
-	while (defined $log->[0] && $log->[0] eq "") {
-		shift @$log;
-	}
-
-	# simplify and print log
-	my $empty = 0;
-	foreach my $line (@$log) {
-		# remove signoff lines
-		if ($line =~ m/^ *(signed[ \-]off[ \-]by[ :]|acked[ \-]by[ :]|cc[ :])/i) {
-			next;
-		}
-		# print only one empty line
-		if ($line eq "") {
-			next if $empty;
-			$empty = 1;
-		} else {
-			$empty = 0;
-		}
-		print format_log_line_html($line) . "<br/>\n";
-	}
-	# end with single empty line
-	print "<br/>\n" unless $empty;
+	git_print_log($log,
+		-final_empty_line=> 1,
+		-remove_signoff => 1,
+		-remove_title => $remove_title);
 }
 
 ## ......................................................................
-- 
1.4.1.1

Re: [PATCH/RFC] gitweb: Make git_print_log generic; git_print_simplified_log uses it

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> The RFC is about style of git_print_log function. Is it a good idea
> and good implementation to pass miscelaneus options as hash values
> instead of using fixed order of parameters, and passing 1 or undef?
> Is it a good naming of parameters like '-remove_title', or would
> 'remove_title' or 'remove-title' be better?

I do not personally like the line-noise sub prototypes, since I
have not seen it buys you much in real programs.  Although some
cute hacks like rolling your own control structure lookalikes
cannot be done without them, they are just that -- cute hacks --
and tends to obfuscate what is really happening behind the scene
(not that writing anything in Perl is not already obfuscating
things ;-)).  But maybe it is just me.

I think you already use CGI.pm and the argument passing style
using things like "-remove_title" is the norm there, so I do not
have objections against it.  You might even be able to lift the
code CGI.pm uses to implement the hash-style passing with
defaults.