Re: [PATCHv6 9/8] gitweb: put signoff lines in a table
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Jun 27, 2009, 09:55 UTC
- Message-ID
- <200906271155.04602.jnareb@gmail.com>
- In-Reply-To
- <1245936097-29538-1-git-send-email-giuseppe.bilotta@gmail.com>
On Thu, 25 June 2009, Giuseppe Bilotta wrote:
Show 6 quoted lines
> This allows us to give better alignments to the components. > > Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com> > --- > > Better, but still far from perfect.
I don't like it. NAK from me (for this experimental patch).
First it breaks correspondence between gitweb's 'commit'/'commitdiff' view and git-show, and between gitweb's 'log' view and git-log. I'd rather we kept that gitweb output is similar to CLI output, so somebody familiar with one of them would have it easy understanding the other. Consistency in output.
Second, I have checked how it looks like in few examples: e1d37937 (different types of signoff) and 8dfb17e1 (empty line in signoff block) and I have the following complaints: * There is extra vertical whitespace between signoff lines * The ':' character terminating signoffs is lost * Empty line vanished (which might be considered good thing).
Show 17 quoted lines
>
> gitweb/gitweb.css | 6 +++++-
> gitweb/gitweb.perl | 47 +++++++++++++++++++++++++++++++++++++++++------
> 2 files changed, 46 insertions(+), 7 deletions(-)
>
> diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
> index ad82f86..21c24fa 100644
> --- a/gitweb/gitweb.css
> +++ b/gitweb/gitweb.css
> @@ -115,10 +115,14 @@ span.age {
> font-style: italic;
> }
>
> -span.signoff {
> +.signoff {
> color: #888888;
> }This change might be good to have nevertheless, for future extendability.
>
> +table.signoff td:first-child {
> + text-align: right;
> +}Advanced CSS selector. Not all web browsers support it (although nowadays I suppose most do support ':first-child' pseudo-class).
Show 18 quoted lines
> +
> div.log_link {
> padding: 0px 8px;
> font-size: 70%;
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index d385f55..53b8817 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3402,15 +3402,31 @@ sub git_print_log {
> # print log
> my $signoff = 0;
> my $empty = 0;
> + my $signoff_table = 0;
> foreach my $line (@$log) {
> - if ($line =~ m/^ *(signed[ \-]off[ \-]by[ :]|(?:trivially[ \-])?acked[ \-]by[ :]|cc[ :])/i) {
> - $signoff = 1;
> + if ($line =~ s/^ *(signed[ \-]off[ \-]by|(?:trivially[ \-])?acked[ \-]by|cc|looks[ \-]right[ \-]to[ \-]me[ \-]by)[ :]//i) {
> + $signoff = $1;Extending regexp for signoff matching is _independent_ change, and IMHO should be put in separate commit (perhaps squashed in 7/8). We really need to do something about it, as this regexp starts to be unwieldingly long... but this issue is already discussed in subthread for patch 7/8 in this series.
You changed "$signoff = 1;" to "$signoff = $1;" and later catch $email... why not do it in the same line, using single (more complicated) regexp?
Also you don't catch terminating ':' in $signoff (see complain above).
Show 40 quoted lines
> $empty = 0;
> if (! $opts{'-remove_signoff'}) {
> - my ($email) = $line =~ /<(\S+@\S+)>/;
> - print "<span class=\"signoff\">" . esc_html($line) . "</span>";
> - print git_get_avatar($email, 'pad_before' => 1) if $email;
> - print "<br/>\n";
> + if (!$signoff_table) {
> + print "<table class=\"signoff\">\n";
> + $signoff_table = 1;
> + }
> + my $email;
> + if ($line =~ s/\s*<(\S+@\S+)>//) {
> + $email = $1;
> + }
> + print "<tr>";
> + print "<td>$signoff</td>";
> + print "<td>" . esc_html($line) . "</td>";
> + if ($email && $git_avatar) {
> + print "<td>";
> + print git_get_avatar($email);
> + print "</td>";
> + } else {
> + print "<td>" . esc_html("<$email>") . "</td>";
> + }
> + print "</tr>\n";
> next;
> } else {
> # remove signoff lines
> @@ -3429,7 +3445,26 @@ sub git_print_log {
> $empty = 0;
> }
>
> + # if we're in a signoff block, empty lines
> + # are empty rows, other lines terminate
> + # the block
> + if ($signoff_table) {
> + if ($empty) {
> + print "<tr />\n";
> + next;
> + }I'd rather use "<tr></tr>\n" here instead.
Show 16 quoted lines
> + print "</table>\n";
> + $signoff_table = 0;
> + }
> +
> print format_log_line_html($line) . "<br/>\n";
> +
> + }
> +
> + # close the signoff table if it's still open
> + if ($signoff_table) {
> + print "</table>\n";
> + $signoff_table = 0;
> }
>
> if ($opts{'-final_empty_line'}) {
> -- Much more complicated code, not much gain IMHO. It is not worth it (even if you think that the layout is better; I don't think that).
-- Jakub Narebski Poland