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@xxxxxxxxx>
> ---
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html