threads / patch / 30948

v2, 3 partsgitweb: Handle other types of tag in git_print_log

Subject: [PATCH v2 2/3] gitweb: Handle other types of tag in git_print_log

## tl;dr

5 messages between Jul 4, 2012 and Jul 5, 2012. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Namhyung Kim· Jul 4, 2012, 02:47 UTC · lore

[PATCH v2 1/3] gitweb: Cleanup git_print_log()

If $signoff set to 1, the $line would be handled in the if statement for the both cases. So the outer of the conditional always sees the $signoff always set to 0 and no need to check it. Thus we can finally get rid of it.

Also rename $empty to more clear $skip_blank_line as suggested by Junio.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 gitweb/gitweb.perl |   21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)
Show changes to gitweb/gitweb.perl +7 −14
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 55e0e9e..82c5da7 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -4484,30 +4484,23 @@ sub git_print_log {
 	}
 
 	# print log
-	my $signoff = 0;
-	my $empty = 0;
+	my $skip_blank_line = 0;
 	foreach my $line (@$log) {
 		if ($line =~ m/^ *(signed[ \-]off[ \-]by[ :]|acked[ \-]by[ :]|cc[ :])/i) {
-			$signoff = 1;
-			$empty = 0;
 			if (! $opts{'-remove_signoff'}) {
 				print "<span class=\"signoff\">" . esc_html($line) . "</span><br/>\n";
-				next;
-			} else {
-				# remove signoff lines
-				next;
+				$skip_blank_line = 1;
 			}
-		} else {
-			$signoff = 0;
+			next;
 		}
 
 		# print only one empty line
 		# do not print empty line after signoff
 		if ($line eq "") {
-			next if ($empty || $signoff);
-			$empty = 1;
+			next if ($skip_blank_line);
+			$skip_blank_line = 1;
 		} else {
-			$empty = 0;
+			$skip_blank_line = 0;
 		}
 
 		print format_log_line_html($line) . "<br/>\n";
@@ -4515,7 +4508,7 @@ sub git_print_log {
 
 	if ($opts{'-final_empty_line'}) {
 		# end with single empty line
-		print "<br/>\n" unless $empty;
+		print "<br/>\n" unless $skip_blank_line;
 	}
 }
 

-- 
1.7.10.4
Namhyung Kim· Jul 4, 2012, 02:47 UTC · re: Namhyung Kim · lore

There are many types of tags used in S-o-b area [1]. Update the regex to handle them properly. It requires the tag should be started by a capital letter and ended by '-by: ' or '-By: '. The only exception is 'Cc: '.

[1] http://lwn.net/Articles/503829/
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 gitweb/gitweb.perl |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to gitweb/gitweb.perl +1 −1
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 82c5da7..362784d 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -4486,7 +4486,7 @@ sub git_print_log {
 	# print log
 	my $skip_blank_line = 0;
 	foreach my $line (@$log) {
-		if ($line =~ m/^ *(signed[ \-]off[ \-]by[ :]|acked[ \-]by[ :]|cc[ :])/i) {
+		if ($line =~ m/^\s*([A-Z][-A-Za-z]*-[Bb]y|C[Cc]): /) {
 			if (! $opts{'-remove_signoff'}) {
 				print "<span class=\"signoff\">" . esc_html($line) . "</span><br/>\n";
 				$skip_blank_line = 1;

-- 
1.7.10.4
Namhyung Kim· Jul 4, 2012, 02:47 UTC · re: Namhyung Kim · lore

[PATCH v2 3/3] gitweb: Add support to Link: tag

The tip tree is the one of major subsystem tree in the Linux kernel project. On the tip tree, the Link: (or similar Buglink:) tag is used for tracking the original discussion or context. Since it's ususally in the S-o-b area, it'd be better using same style with others.

Also as it tends to contain a message-id sent from git send-email, a part of the line would set a wrong hyperlink like [1]. Fix it by not using format_log_line_html().

[1] git.kernel.org/?p=linux/kernel/git/tip/tip.git;a=commit;h=08942f6d5d992e9486b07653fd87ea8182a22fa0
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 gitweb/gitweb.perl |   10 ++++++++++
 1 file changed, 10 insertions(+)
Show changes to gitweb/gitweb.perl +10 −0
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 362784d..3d6a705 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -4494,6 +4494,16 @@ sub git_print_log {
 			next;
 		}
 
+		if ($line =~ m,\s*([a-z]*link): (https?://\S+),i) {
+			if (! $opts{'-remove_signoff'}) {
+				print "<span class=\"signoff\">" . esc_html($1) . ": " .
+					"<a href=\"" . esc_html($2) . "\">" . esc_html($2) . "</a>" .
+					"</span><br/>\n";
+				$skip_blank_line = 1;
+			}
+			next;
+		}
+
 		# print only one empty line
 		# do not print empty line after signoff
 		if ($line eq "") {

-- 
1.7.10.4
Junio C Hamano· Jul 5, 2012, 22:29 UTC · re: Namhyung Kim · lore

Re: [PATCH v2 3/3] gitweb: Add support to Link: tag

Namhyung Kim <namhyung@kernel.org> writes:
Show 30 quoted lines
> The tip tree is the one of major subsystem tree in the
> Linux kernel project. On the tip tree, the Link: (or
> similar Buglink:) tag is used for tracking the original
> discussion or context. Since it's ususally in the S-o-b
> area, it'd be better using same style with others.
>
> Also as it tends to contain a message-id sent from git
> send-email, a part of the line would set a wrong hyperlink
> like [1]. Fix it by not using format_log_line_html().
>
> [1] git.kernel.org/?p=linux/kernel/git/tip/tip.git;a=commit;h=08942f6d5d992e9486b07653fd87ea8182a22fa0
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  gitweb/gitweb.perl |   10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 362784d..3d6a705 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -4494,6 +4494,16 @@ sub git_print_log {
>  			next;
>  		}
>  
> +		if ($line =~ m,\s*([a-z]*link): (https?://\S+),i) {
> +			if (! $opts{'-remove_signoff'}) {
> +				print "<span class=\"signoff\">" . esc_html($1) . ": " .
> +					"<a href=\"" . esc_html($2) . "\">" . esc_html($2) . "</a>" .
> +					"</span><br/>\n";

Thanks. Is the first use of esc_html($2) correct (I am always confused between esc_html, esc_param and esc_url)?

Show 8 quoted lines
> +				$skip_blank_line = 1;
> +			}
> +			next;
> +		}
> +
>  		# print only one empty line
>  		# do not print empty line after signoff
>  		if ($line eq "") {
Junio C Hamano· Jul 5, 2012, 22:22 UTC · re: Namhyung Kim · lore

Re: [PATCH v2 1/3] gitweb: Cleanup git_print_log()

Namhyung Kim <namhyung@kernel.org> writes:
Show 8 quoted lines
> If $signoff set to 1, the $line would be handled in
> the if statement for the both cases. So the outer of
> the conditional always sees the $signoff always set
> to 0 and no need to check it. Thus we can finally get
> rid of it.
>
> Also rename $empty to more clear $skip_blank_line as
> suggested by Junio.

Thanks. It does not make it clear that you fixed a bug in the original, so let me rephrase it like so:

-- >8 --
From: Namhyung Kim <namhyung@kernel.org>
Date: Wed, 4 Jul 2012 11:47:24 +0900
Subject: [PATCH] gitweb: Cleanup git_print_log()

When we see a signed-off-by line (and its friends), we set $signoff to true, but then we process the next line after we are done without giving control to the rest of the loop. And when the line we saw is not a signed-off-by line, we reset $signoff to false before running the remainder of the loop.

Hence, the check for $signoff that attempts to remove an extra empty line between two signed-off-by line was not doing anything useful.

Rename $empty to a more explicit name $skip_blank_line to tell us to skip a blank line when we see one, set it after we see and emit a blank line (to avoid showing more than one empty lines in a raw) or after we handle a signed-off-by line (to avoid empty lines after such a line), to fix this bug, and get rid of the $signoff variable that is not useful.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

← back to recent threads