Re: [PATCH] gitweb: gpg signature status indication for commits

Re: [PATCH] gitweb: gpg signature status indication for commits

[Victor Kartashov]

show gpg signature (if any) for commit message in gitweb
in case of valid signature highlight it with green
in case of invalid signature highlight it with red

Signed-off-by: Victor Kartashov <victor.kartashov@gmail.com>
---
here's new patch
fixed remarks by Eric Sunshine
"pop @commit_lines" in parse_commit_text() leads to a loss of the last line in commit message ('sign-off' line, for example), so I search for '\0' before removing it.

 gitweb/gitweb.perl       | 36 +++++++++++++++++++++++++++++-------
 gitweb/static/gitweb.css | 11 +++++++++++
 2 files changed, 40 insertions(+), 7 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 79057b7..ccde90f 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3430,8 +3430,9 @@ sub parse_commit_text {
 	my ($commit_text, $withparents) = @_;
 	my @commit_lines = split '\n', $commit_text;
 	my %co;
+	my @signature = ();
 
-	pop @commit_lines; # Remove '\0'
+	pop @commit_lines if ($commit_lines[-1] =~ "\0"); # Remove '\0'
 
 	if (! @commit_lines) {
 		return;
@@ -3469,6 +3470,9 @@ sub parse_commit_text {
 				$co{'committer_name'} = $co{'committer'};
 			}
 		}
+		elsif ($line =~ /^gpg: /) {
+			push @signature, $line;
+		}
 	}
 	if (!defined $co{'tree'}) {
 		return;
@@ -3508,6 +3512,10 @@ sub parse_commit_text {
 	foreach my $line (@commit_lines) {
 		$line =~ s/^    //;
 	}
+	push(@commit_lines, "") if scalar @signature;
+	foreach my $sig (@signature) {
+		push(@commit_lines, $sig);
+	}
 	$co{'comment'} = \@commit_lines;
 
 	my $age = time - $co{'committer_epoch'};
@@ -3530,13 +3538,13 @@ sub parse_commit {
 
 	local $/ = "\0";
 
-	open my $fd, "-|", git_cmd(), "rev-list",
-		"--parents",
-		"--header",
-		"--max-count=1",
+	open my $fd, "-|", git_cmd(), "show",
+		"--quiet",
+		"--date=raw",
+		"--pretty=format:%H %P%ntree %T%nparent %P%nauthor %an <%ae> %ad%ncommitter %cn <%ce> %cd%n%GG%n%s%n%n%b",
 		$commit_id,
 		"--",
-		or die_error(500, "Open git-rev-list failed");
+		or die_error(500, "Open git-show failed");
 	%co = parse_commit_text(<$fd>, 1);
 	close $fd;
 
@@ -4571,7 +4579,21 @@ sub git_print_log {
 	# print log
 	my $skip_blank_line = 0;
 	foreach my $line (@$log) {
-		if ($line =~ m/^\s*([A-Z][-A-Za-z]*-[Bb]y|C[Cc]): /) {
+		if ($line =~ m/^gpg:(.)+Good(.)+/) {
+			if (! $opts{'-remove_signoff'}) {
+				print "<span class=\"good_sign\">" . esc_html($line) . "</span><br/>\n";
+				$skip_blank_line = 1;
+			}
+			next;
+		}
+		elsif ($line =~ m/^gpg:(.)+BAD(.)+/) {
+			if (! $opts{'-remove_signoff'}) {
+				print "<span class=\"bad_sign\">" . esc_html($line) . "</span><br/>\n";
+				$skip_blank_line = 1;
+			}
+			next;
+		}
+		elsif ($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;
diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css
index 3212601..e99e223 100644
--- a/gitweb/static/gitweb.css
+++ b/gitweb/static/gitweb.css
@@ -136,6 +136,17 @@ span.signoff {
 	color: #888888;
 }
 
+span.good_sign {
+	font-weight: bold;
+	background-color: #aaffaa;
+}
+
+span.bad_sign {
+	font-weight: bold;
+	background-color: #880000;
+	color: #ffffff
+}
+
 div.log_link {
 	padding: 0px 8px;
 	font-size: 70%;
-- 
1.8.3.rc0.10.g8974033

Re: [PATCH] gitweb: gpg signature status indication for commits

[Junio C Hamano]

Victor Kartashov <v.kartashov@npo-echelon.ru> writes:

> show gpg signature (if any) for commit message in gitweb
> in case of valid signature highlight it with green
> in case of invalid signature highlight it with red

If that is a single sentence, please write it as such:

   Show gpg signature (if any) for commit message in gitweb in case of
   valid signature highlight it with green in case of invalid signature
   highlight it with red.

But that is almost unparsable ;-)

   Teach gitweb to show GPG signature verification status when
   showing a commit that is signed.  Highlight in green or red to
   differentiate valid and invalid signatures.

or something?

Is it a good idea to do this unconditionally in all repositories?

> Signed-off-by: Victor Kartashov <victor.kartashov@gmail.com>
> ---
>  gitweb/gitweb.perl       | 36 +++++++++++++++++++++++++++++-------
>  gitweb/static/gitweb.css | 11 +++++++++++
>  2 files changed, 40 insertions(+), 7 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 79057b7..ccde90f 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3430,8 +3430,9 @@ sub parse_commit_text {
>  	my ($commit_text, $withparents) = @_;
>  	my @commit_lines = split '\n', $commit_text;
>  	my %co;
> +	my @signature = ();
>  
> -	pop @commit_lines; # Remove '\0'
> +	pop @commit_lines if ($commit_lines[-1] =~ "\0"); # Remove '\0'

This comment, which only says what it intends to do without saying
why it wants to do so, does not explain why this is a good change.

Does the code even know if $commit_lines[-1] is a non-empty string?
Is it safe to assume if $commit_lines[-1] has a NUL anywhere, it is
always the last line that ends the record, which is not part of the
commit log message?

I am assuming that this $commit_text is read from "log -z" (or
"rev-list -z") output and split at NUL boundary, but if that is the
case, it might be cleaner to remove the trailing NUL from $commit_text
before even attempting to split it into an array at LFs, but that is
an unrelated tangent.

A bigger question is why does the incoming data sometimes ends with
NUL that must be stripped out, and sometimes does not?  I see that
you are updating the data producer in the later part of the patch;
wouldn't it be saner if you have the data producer produce the input
to this function in a way that is consistent with the current code,
i.e. always terminate the output with a NUL?

> @@ -3469,6 +3470,9 @@ sub parse_commit_text {
>  				$co{'committer_name'} = $co{'committer'};
>  			}
>  		}
> +		elsif ($line =~ /^gpg: /) {

I think Eric already pointed out the style issue on this line.

> @@ -3508,6 +3512,10 @@ sub parse_commit_text {
>  	foreach my $line (@commit_lines) {
>  		$line =~ s/^    //;
>  	}
> +	push(@commit_lines, "") if scalar @signature;
> +	foreach my $sig (@signature) {
> +		push(@commit_lines, $sig);
> +	}

Hmm, is it different from doing:

	push @commit_lines, @signature;

in some way?

> @@ -4571,7 +4579,21 @@ sub git_print_log {
>  	# print log
>  	my $skip_blank_line = 0;
>  	foreach my $line (@$log) {
> -		if ($line =~ m/^\s*([A-Z][-A-Za-z]*-[Bb]y|C[Cc]): /) {
> +		if ($line =~ m/^gpg:(.)+Good(.)+/) {
> +			if (! $opts{'-remove_signoff'}) {

Sorry, but I fail to see what the "remove-signoff" option has to do
with this new feature.  The interaction needs to be explained in the
log message.

> +				print "<span class=\"good_sign\">" . esc_html($line) . "</span><br/>\n";
> +				$skip_blank_line = 1;
> +			}
> +			next;
> +		}
> +		elsif ($line =~ m/^gpg:(.)+BAD(.)+/) {
> +			if (! $opts{'-remove_signoff'}) {
> +				print "<span class=\"bad_sign\">" . esc_html($line) . "</span><br/>\n";
> +				$skip_blank_line = 1;
> +			}
> +			next;
> +		}
> +		elsif ($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;
> diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css
> index 3212601..e99e223 100644
> --- a/gitweb/static/gitweb.css
> +++ b/gitweb/static/gitweb.css
> @@ -136,6 +136,17 @@ span.signoff {
>  	color: #888888;
>  }
>  
> +span.good_sign {
> +	font-weight: bold;
> +	background-color: #aaffaa;
> +}
> +
> +span.bad_sign {
> +	font-weight: bold;
> +	background-color: #880000;
> +	color: #ffffff
> +}
> +
>  div.log_link {
>  	padding: 0px 8px;
>  	font-size: 70%;

Re: [PATCH] gitweb: gpg signature status indication for commits

[Victor Kartashov]

 On 28 March 2014 21:47, Junio C Hamano <gitster@pobox.com> wrote:
>
>
>    Teach gitweb to show GPG signature verification status when
>    showing a commit that is signed.  Highlight in green or red to
>    differentiate valid and invalid signatures.
>
> or something?


 Yes, kind of :)

> Is it a good idea to do this unconditionally in all repositories?


 Actually, I don't know. It's a question to discuss. This patch
doesn't affect commits without signature. And if there is a signature,
you robably would like to validate it.
 There is an option "remove_signoff" that, as I thought, would have an
effect on this. But I couldn't find where it's defined.

>
>
> This comment, which only says what it intends to do without saying
> why it wants to do so, does not explain why this is a good change.
>
> Does the code even know if $commit_lines[-1] is a non-empty string?
> Is it safe to assume if $commit_lines[-1] has a NUL anywhere, it is
> always the last line that ends the record, which is not part of the
> commit log message?
>
> I am assuming that this $commit_text is read from "log -z" (or
> "rev-list -z") output and split at NUL boundary, but if that is the
> case, it might be cleaner to remove the trailing NUL from $commit_text
> before even attempting to split it into an array at LFs, but that is
> an unrelated tangent.
>
> A bigger question is why does the incoming data sometimes ends with
> NUL that must be stripped out, and sometimes does not?  I see that
> you are updating the data producer in the later part of the patch;
> wouldn't it be saner if you have the data producer produce the input
> to this function in a way that is consistent with the current code,
> i.e. always terminate the output with a NUL?


 It seems to be a good idea. I'll try to do that.

> > @@ -3469,6 +3470,9 @@ sub parse_commit_text {
> >                               $co{'committer_name'} = $co{'committer'};
> >                       }
> >               }
> > +             elsif ($line =~ /^gpg: /) {
>
> I think Eric already pointed out the style issue on this line.
>
> > @@ -3508,6 +3512,10 @@ sub parse_commit_text {
> >       foreach my $line (@commit_lines) {
> >               $line =~ s/^    //;
> >       }
> > +     push(@commit_lines, "") if scalar @signature;
> > +     foreach my $sig (@signature) {
> > +             push(@commit_lines, $sig);
> > +     }
>
> Hmm, is it different from doing:
>
>         push @commit_lines, @signature;
>
> in some way?
>
> > @@ -4571,7 +4579,21 @@ sub git_print_log {
> >       # print log
> >       my $skip_blank_line = 0;
> >       foreach my $line (@$log) {
> > -             if ($line =~ m/^\s*([A-Z][-A-Za-z]*-[Bb]y|C[Cc]): /) {
> > +             if ($line =~ m/^gpg:(.)+Good(.)+/) {
> > +                     if (! $opts{'-remove_signoff'}) {
>
> Sorry, but I fail to see what the "remove-signoff" option has to do
> with this new feature.  The interaction needs to be explained in the
> log message.


 I explained it above. From my point of view, one may want to remove
gpg signature and "sign-off" inscription together.
 Maybe, we should discuss this question, and after that I'll prepare new patch.

>
>
> > +                             print "<span class=\"good_sign\">" . esc_html($line) . "</span><br/>\n";
> > +                             $skip_blank_line = 1;
> > +                     }
> > +                     next;
> > +             }
> > +             elsif ($line =~ m/^gpg:(.)+BAD(.)+/) {
> > +                     if (! $opts{'-remove_signoff'}) {
> > +                             print "<span class=\"bad_sign\">" . esc_html($line) . "</span><br/>\n";
> > +                             $skip_blank_line = 1;
> > +                     }
> > +                     next;
> > +             }
> > +             elsif ($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;
> > diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css
> > index 3212601..e99e223 100644
> > --- a/gitweb/static/gitweb.css
> > +++ b/gitweb/static/gitweb.css
> > @@ -136,6 +136,17 @@ span.signoff {
> >       color: #888888;
> >  }
> >
> > +span.good_sign {
> > +     font-weight: bold;
> > +     background-color: #aaffaa;
> > +}
> > +
> > +span.bad_sign {
> > +     font-weight: bold;
> > +     background-color: #880000;
> > +     color: #ffffff
> > +}
> > +
> >  div.log_link {
> >       padding: 0px 8px;
> >       font-size: 70%;

[PATCH] gitweb: gpg signature status indication for commits

[Victor Kartashov]

shows gpg signature (if any) for commit message in gitweb
in case of successfully verifying the signature highlights it with green

Signed-off-by: Victor Kartashov <victor.kartashov@gmail.com>
---
 gitweb/gitweb.perl       | 33 ++++++++++++++++++++++++++-------
 gitweb/static/gitweb.css |  5 +++++
 2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 79057b7..0b41392 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3430,8 +3430,9 @@ sub parse_commit_text {
 	my ($commit_text, $withparents) = @_;
 	my @commit_lines = split '\n', $commit_text;
 	my %co;
+	my @signature = ();
 
-	pop @commit_lines; # Remove '\0'
+	pop @commit_lines if ($commit_lines[-1] eq "\0"); # Remove '\0'
 
 	if (! @commit_lines) {
 		return;
@@ -3469,6 +3470,10 @@ sub parse_commit_text {
 				$co{'committer_name'} = $co{'committer'};
 			}
 		}
+		elsif ($line =~ /^gpg: /)
+		{
+			push @signature, $line;
+		}
 	}
 	if (!defined $co{'tree'}) {
 		return;
@@ -3508,6 +3513,11 @@ sub parse_commit_text {
 	foreach my $line (@commit_lines) {
 		$line =~ s/^    //;
 	}
+	push(@commit_lines, "") if(scalar(@signature) > 0);
+	foreach my $sig (@signature)
+	{
+		push(@commit_lines, $sig);
+	}
 	$co{'comment'} = \@commit_lines;
 
 	my $age = time - $co{'committer_epoch'};
@@ -3530,13 +3540,15 @@ sub parse_commit {
 
 	local $/ = "\0";
 
-	open my $fd, "-|", git_cmd(), "rev-list",
-		"--parents",
-		"--header",
-		"--max-count=1",
+
+
+	open my $fd, "-|", git_cmd(), "show",
+		"--quiet",
+		"--date=raw",
+		"--pretty=format:%H %P%ntree %T%nparent %P%nauthor %an <%ae> %ad%ncommitter %cn <%ce> %cd%n%GG%n%s%n%n%b",
 		$commit_id,
 		"--",
-		or die_error(500, "Open git-rev-list failed");
+		or die_error(500, "Open git-show failed");
 	%co = parse_commit_text(<$fd>, 1);
 	close $fd;
 
@@ -4571,7 +4583,14 @@ sub git_print_log {
 	# print log
 	my $skip_blank_line = 0;
 	foreach my $line (@$log) {
-		if ($line =~ m/^\s*([A-Z][-A-Za-z]*-[Bb]y|C[Cc]): /) {
+		if ($line =~ m/^gpg:(.)+Good(.)+/) {
+			if (! $opts{'-remove_signoff'}) {
+				print "<span class=\"good_sign\">" . esc_html($line) . "</span><br/>\n";
+				$skip_blank_line = 1;
+			}
+			next;
+		}
+		elsif ($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;
diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css
index 3212601..0b7479c 100644
--- a/gitweb/static/gitweb.css
+++ b/gitweb/static/gitweb.css
@@ -136,6 +136,11 @@ span.signoff {
 	color: #888888;
 }
 
+span.good_sign {
+	font-weight: bold;
+	background-color: #aaffaa;
+}
+
 div.log_link {
 	padding: 0px 8px;
 	font-size: 70%;
-- 
1.8.3.2

Re: [PATCH] gitweb: gpg signature status indication for commits

[Eric Sunshine]

On Thu, Mar 27, 2014 at 10:56 AM, Victor Kartashov
<victor.kartashov@gmail.com> wrote:
> shows gpg signature (if any) for commit message in gitweb
> in case of successfully verifying the signature highlights it with green

Write in imperative mood: "Show gpg ... highlight it..."

As a corollary, would it be meaningful to highlight a bad signature with red?

> Signed-off-by: Victor Kartashov <victor.kartashov@gmail.com>
> ---
>  gitweb/gitweb.perl       | 33 ++++++++++++++++++++++++++-------
>  gitweb/static/gitweb.css |  5 +++++
>  2 files changed, 31 insertions(+), 7 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 79057b7..0b41392 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3430,8 +3430,9 @@ sub parse_commit_text {
>         my ($commit_text, $withparents) = @_;
>         my @commit_lines = split '\n', $commit_text;
>         my %co;
> +       my @signature = ();
>
> -       pop @commit_lines; # Remove '\0'
> +       pop @commit_lines if ($commit_lines[-1] eq "\0"); # Remove '\0'

What is this change about? Is it related to your gpg change or something else?

>         if (! @commit_lines) {
>                 return;
> @@ -3469,6 +3470,10 @@ sub parse_commit_text {
>                                 $co{'committer_name'} = $co{'committer'};
>                         }
>                 }
> +               elsif ($line =~ /^gpg: /)

Inconsistent 'elsif' placement. (Cuddle it with the close-brace.)

> +               {

Inconsistent open-brace placement.

> +                       push @signature, $line;
> +               }
>         }
>         if (!defined $co{'tree'}) {
>                 return;
> @@ -3508,6 +3513,11 @@ sub parse_commit_text {
>         foreach my $line (@commit_lines) {
>                 $line =~ s/^    //;
>         }
> +       push(@commit_lines, "") if(scalar(@signature) > 0);

Missing space after 'if'.

In this Perl file, it would be more consistent to drop the '> 0' and
say merely 'if scalar @signature'.

> +       foreach my $sig (@signature)
> +       {

Brace placement.

> +               push(@commit_lines, $sig);
> +       }
>         $co{'comment'} = \@commit_lines;
>
>         my $age = time - $co{'committer_epoch'};
> @@ -3530,13 +3540,15 @@ sub parse_commit {
>
>         local $/ = "\0";
>
> -       open my $fd, "-|", git_cmd(), "rev-list",
> -               "--parents",
> -               "--header",
> -               "--max-count=1",
> +
> +

Unnecessary two extra blank lines.

> +       open my $fd, "-|", git_cmd(), "show",
> +               "--quiet",
> +               "--date=raw",
> +               "--pretty=format:%H %P%ntree %T%nparent %P%nauthor %an <%ae> %ad%ncommitter %cn <%ce> %cd%n%GG%n%s%n%n%b",
>                 $commit_id,
>                 "--",
> -               or die_error(500, "Open git-rev-list failed");
> +               or die_error(500, "Open git-show failed");
>         %co = parse_commit_text(<$fd>, 1);
>         close $fd;
>
> @@ -4571,7 +4583,14 @@ sub git_print_log {
>         # print log
>         my $skip_blank_line = 0;
>         foreach my $line (@$log) {
> -               if ($line =~ m/^\s*([A-Z][-A-Za-z]*-[Bb]y|C[Cc]): /) {
> +               if ($line =~ m/^gpg:(.)+Good(.)+/) {
> +                       if (! $opts{'-remove_signoff'}) {
> +                               print "<span class=\"good_sign\">" . esc_html($line) . "</span><br/>\n";
> +                               $skip_blank_line = 1;
> +                       }
> +                       next;
> +               }
> +               elsif ($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;
> diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css
> index 3212601..0b7479c 100644
> --- a/gitweb/static/gitweb.css
> +++ b/gitweb/static/gitweb.css
> @@ -136,6 +136,11 @@ span.signoff {
>         color: #888888;
>  }
>
> +span.good_sign {
> +       font-weight: bold;
> +       background-color: #aaffaa;
> +}
> +
>  div.log_link {
>         padding: 0px 8px;
>         font-size: 70%;
> --
> 1.8.3.2