From: Jakub Narebski <jnareb@gmail.com>
Date: Sat, 27 Jun 2009 09:55:04 GMT
Subject: Re: [PATCHv6 9/8] gitweb: put signoff lines in a table
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:

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

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

> +
>  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).

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

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