From: Jakub Narebski <jnareb@gmail.com>
To: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCHv6 9/8] gitweb: put signoff lines in a table
Date: Sat, 27 Jun 2009 11:55:04 +0200 [thread overview]
Message-ID: <200906271155.04602.jnareb@gmail.com> (raw)
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
next prev parent reply other threads:[~2009-06-27 9:55 UTC|newest]
Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-06-25 10:42 [PATCHv6 0/8] gitweb: gravatar support Giuseppe Bilotta
2009-06-25 10:43 ` [PATCHv6 1/8] gitweb: refactor author name insertion Giuseppe Bilotta
2009-06-25 10:43 ` [PATCHv6 2/8] gitweb: uniform author info for commit and commitdiff Giuseppe Bilotta
2009-06-25 10:43 ` [PATCHv6 3/8] gitweb: right-align date cell in shortlog Giuseppe Bilotta
2009-06-25 10:43 ` [PATCHv6 4/8] gitweb: (gr)avatar support Giuseppe Bilotta
2009-06-25 10:43 ` [PATCHv6 5/8] gitweb: gravatar url cache Giuseppe Bilotta
2009-06-25 10:43 ` [PATCHv6 6/8] gitweb: add 'alt' to avatar images Giuseppe Bilotta
2009-06-25 10:43 ` [PATCHv6 7/8] gitweb: recognize 'trivial' acks Giuseppe Bilotta
2009-06-25 10:43 ` [PATCHv6 8/8] gitweb: add avatar in signoff lines Giuseppe Bilotta
2009-06-25 13:21 ` [PATCHv6 9/8] gitweb: put signoff lines in a table Giuseppe Bilotta
2009-06-27 9:55 ` Jakub Narebski [this message]
2009-06-27 9:26 ` [PATCHv6 8/8] gitweb: add avatar in signoff lines Jakub Narebski
2009-06-27 10:21 ` Giuseppe Bilotta
2009-06-27 0:19 ` [PATCHv6 7/8] gitweb: recognize 'trivial' acks Jakub Narebski
2009-06-27 1:03 ` Junio C Hamano
2009-06-27 9:04 ` Giuseppe Bilotta
2009-06-26 23:39 ` [PATCHv6 6/8] gitweb: add 'alt' to avatar images Jakub Narebski
2009-06-27 0:08 ` Thomas Adam
2009-06-26 23:11 ` [PATCHv6 5/8] gitweb: gravatar url cache Jakub Narebski
2009-06-26 23:27 ` Giuseppe Bilotta
2009-06-26 23:53 ` Jakub Narebski
2009-06-26 19:42 ` [PATCHv6 4/8] gitweb: (gr)avatar support Jakub Narebski
2009-06-26 22:08 ` Giuseppe Bilotta
2009-06-26 22:58 ` Jakub Narebski
2009-06-26 23:14 ` Giuseppe Bilotta
2009-06-26 23:25 ` Jakub Narebski
2009-06-27 0:29 ` Junio C Hamano
2009-06-27 0:32 ` Giuseppe Bilotta
2009-06-26 9:33 ` [PATCHv6 3/8] gitweb: right-align date cell in shortlog Jakub Narebski
2009-06-26 18:06 ` Giuseppe Bilotta
2009-06-26 22:34 ` Junio C Hamano
2009-06-26 22:57 ` Giuseppe Bilotta
2009-06-26 23:57 ` Junio C Hamano
2009-06-27 12:14 ` Jakub Narebski
2009-06-27 12:49 ` Jakub Narebski
2009-06-25 23:14 ` [PATCHv6 2/8] gitweb: uniform author info for commit and commitdiff Jakub Narebski
2009-06-26 17:52 ` Giuseppe Bilotta
2009-06-25 22:55 ` [PATCHv6 1/8] gitweb: refactor author name insertion Jakub Narebski
2009-06-25 23:01 ` Jakub Narebski
2009-06-25 23:41 ` Giuseppe Bilotta
2009-06-25 12:55 ` [PATCHv6 0/8] gitweb: gravatar support Jakub Narebski
2009-06-25 13:15 ` Giuseppe Bilotta
2009-06-25 17:07 ` Junio C Hamano
2009-06-25 18:46 ` Giuseppe Bilotta
2009-06-25 18:56 ` Junio C Hamano
2009-06-25 23:17 ` Jakub Narebski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=200906271155.04602.jnareb@gmail.com \
--to=jnareb@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=giuseppe.bilotta@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).