On 28 March 2014 21:47, Junio C Hamano <[email protected]> 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%;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
