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

Jakub Narebski· Aug 27, 2006, 21:55 UTC · lore

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(-)

Show changes to gitweb/gitweb.perl +31 −32

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

Junio C Hamano· Aug 27, 2006, 23:11 UTC · re: Jakub Narebski · lore

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

Jakub Narebski <jnareb@gmail.com> writes:

Show 5 quoted lines

> 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.

RFC patchgitweb: Make git_print_log generic; git_print_simplified_log uses it