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

 On 28 March 2014 21:47, Junio C Hamano [off-list ref] 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.

quoted

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

quoted

@@ -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?

quoted

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

quoted

+                             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%;