[PATCHv6 0/8] gitweb: gravatar support

[PATCHv6 0/8] gitweb: gravatar support

[Giuseppe Bilotta]

A new iteration for my gitweb gravatar patch series. This one includes a
bunch of new patches, some of which make sense regardless of the
gravatar support (particularly patches #3 and #7).

Significant changes from the previous iteration are:

* the feature has been renamed to 'avatar', and 'gravatar' is a possible
  value for it (currently the only sensible value, other than '');
* an 'alt' attribute is added to img tags, to be more friendly towards
  text-only browser; no special text is put there, only the avatar
  provider name (i.e. 'gravatar', currently);
* the last patch adds avatars to signoff lines.

I also trimmed the Cc: list for the series, because it was getting very
long and I started feeling like I was spamming around by including
everybody ever commented on the series. Please don't feel left out ;-)

Giuseppe Bilotta (8):
  gitweb: refactor author name insertion
  gitweb: uniform author info for commit and commitdiff
  gitweb: right-align date cell in shortlog
  gitweb: (gr)avatar support
  gitweb: gravatar url cache
  gitweb: add 'alt' to avatar images
  gitweb: recognize 'trivial' acks
  gitweb: add avatar in signoff lines

 gitweb/gitweb.css  |   13 ++++-
 gitweb/gitweb.perl |  170 +++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 148 insertions(+), 35 deletions(-)

[PATCHv6 1/8] gitweb: refactor author name insertion

[Giuseppe Bilotta]

Collect all author display code in appropriate functions, making it
easiser to extend them on the CGI side, and rely on CSS rather than
hard-coded HTML formatting for easier customization.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.css  |    5 ++-
 gitweb/gitweb.perl |   80 +++++++++++++++++++++++++++++++---------------------
 2 files changed, 52 insertions(+), 33 deletions(-)

diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index a01eac8..68b22ff 100644
--- a/gitweb/gitweb.css
+++ b/gitweb/gitweb.css
@@ -132,11 +132,14 @@ div.list_head {
 	font-style: italic;
 }
 
+.author_date, .author {
+	font-style: italic;
+}
+
 div.author_date {
 	padding: 8px;
 	border: solid #d9d8d1;
 	border-width: 0px 0px 1px 0px;
-	font-style: italic;
 }
 
 a.list {
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 1e7e2d8..9b60418 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1469,6 +1469,16 @@ sub format_subject_html {
 	}
 }
 
+# format the author name of the given commit with the given tag
+# the author name is chopped and escaped according to the other
+# optional parameters (see chop_str).
+sub format_author_html {
+	my $tag = shift;
+	my $co = shift;
+	my $author = chop_and_escape_str($co->{'author_name'}, @_);
+	return "<$tag class=\"author\">" . $author . "</$tag>\n";
+}
+
 # format git diff header line, i.e. "diff --(git|combined|cc) ..."
 sub format_git_diff_header_line {
 	my $line = shift;
@@ -3214,13 +3224,36 @@ sub git_print_header_div {
 	      "\n</div>\n";
 }
 
+# Outputs the author name and date in long form
 sub git_print_authorship {
 	my $co = shift;
+	my %params = @_;
+	my $tag = $params{'tag'} || 'div';
 
 	my %ad = parse_date($co->{'author_epoch'}, $co->{'author_tz'});
-	print "<div class=\"author_date\">" .
+	print "<$tag class=\"author_date\">" .
 	      esc_html($co->{'author_name'}) .
 	      " [$ad{'rfc2822'}";
+	if ($params{'localtime'}) {
+		if ($ad{'hour_local'} < 6) {
+			printf(" (<span class=\"atnight\">%02d:%02d</span> %s)",
+			       $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
+		} else {
+			printf(" (%02d:%02d %s)",
+			       $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
+		}
+	}
+	print "]</$tag>\n";
+}
+
+# Outputs table rows containign the full author and commiter information.
+sub git_print_full_authorship {
+	my $co = shift;
+	my %ad = parse_date($co->{'author_epoch'}, $co->{'author_tz'});
+	my %cd = parse_date($co->{'committer_epoch'}, $co->{'committer_tz'});
+	print "<tr><td>author</td><td>" . esc_html($co->{'author'}) . "</td></tr>\n".
+	      "<tr>" .
+	      "<td></td><td> $ad{'rfc2822'}";
 	if ($ad{'hour_local'} < 6) {
 		printf(" (<span class=\"atnight\">%02d:%02d</span> %s)",
 		       $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
@@ -3228,7 +3261,12 @@ sub git_print_authorship {
 		printf(" (%02d:%02d %s)",
 		       $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
 	}
-	print "]</div>\n";
+	print "</td>" .
+	      "</tr>\n";
+	print "<tr><td>committer</td><td>" . esc_html($co->{'committer'}) . "</td></tr>\n";
+	print "<tr><td></td><td> $cd{'rfc2822'}" .
+	      sprintf(" (%02d:%02d %s)", $cd{'hour_local'}, $cd{'minute_local'}, $cd{'tz_local'}) .
+	      "</td></tr>\n";
 }
 
 sub git_print_page_path {
@@ -4142,11 +4180,9 @@ sub git_shortlog_body {
 			print "<tr class=\"light\">\n";
 		}
 		$alternate ^= 1;
-		my $author = chop_and_escape_str($co{'author_name'}, 10);
 		# git_summary() used print "<td><i>$co{'age_string'}</i></td>\n" .
 		print "<td title=\"$co{'age_string_age'}\"><i>$co{'age_string_date'}</i></td>\n" .
-		      "<td><i>" . $author . "</i></td>\n" .
-		      "<td>";
+		      format_author_html('td', \%co, 10) . "<td>";
 		print format_subject_html($co{'title'}, $co{'title_short'},
 		                          href(action=>"commit", hash=>$commit), $ref);
 		print "</td>\n" .
@@ -4193,11 +4229,9 @@ sub git_history_body {
 			print "<tr class=\"light\">\n";
 		}
 		$alternate ^= 1;
-	# shortlog uses      chop_str($co{'author_name'}, 10)
-		my $author = chop_and_escape_str($co{'author_name'}, 15, 3);
 		print "<td title=\"$co{'age_string_age'}\"><i>$co{'age_string_date'}</i></td>\n" .
-		      "<td><i>" . $author . "</i></td>\n" .
-		      "<td>";
+	# shortlog:   format_author_html('td', \%co, 10)
+		      format_author_html('td', \%co, 15, 3) . "<td>";
 		# originally git_history used chop_str($co{'title'}, 50)
 		print format_subject_html($co{'title'}, $co{'title_short'},
 		                          href(action=>"commit", hash=>$commit), $ref);
@@ -4350,9 +4384,8 @@ sub git_search_grep_body {
 			print "<tr class=\"light\">\n";
 		}
 		$alternate ^= 1;
-		my $author = chop_and_escape_str($co{'author_name'}, 15, 5);
 		print "<td title=\"$co{'age_string_age'}\"><i>$co{'age_string_date'}</i></td>\n" .
-		      "<td><i>" . $author . "</i></td>\n" .
+		      format_author_html('td', \%co, 15, 5) .
 		      "<td>" .
 		      $cgi->a({-href => href(action=>"commit", hash=>$co{'id'}),
 		               -class => "list subject"},
@@ -5094,9 +5127,9 @@ sub git_log {
 		      " | " .
 		      $cgi->a({-href => href(action=>"tree", hash=>$commit, hash_base=>$commit)}, "tree") .
 		      "<br/>\n" .
-		      "</div>\n" .
-		      "<i>" . esc_html($co{'author_name'}) .  " [$ad{'rfc2822'}]</i><br/>\n" .
 		      "</div>\n";
+		      git_print_authorship(\%co);
+		      print "<br/>\n</div>\n";
 
 		print "<div class=\"log_body\">\n";
 		git_print_log($co{'comment'}, -final_empty_line=> 1);
@@ -5115,8 +5148,6 @@ sub git_commit {
 	$hash ||= $hash_base || "HEAD";
 	my %co = parse_commit($hash)
 	    or die_error(404, "Unknown commit object");
-	my %ad = parse_date($co{'author_epoch'}, $co{'author_tz'});
-	my %cd = parse_date($co{'committer_epoch'}, $co{'committer_tz'});
 
 	my $parent  = $co{'parent'};
 	my $parents = $co{'parents'}; # listref
@@ -5183,22 +5214,7 @@ sub git_commit {
 	}
 	print "<div class=\"title_text\">\n" .
 	      "<table class=\"object_header\">\n";
-	print "<tr><td>author</td><td>" . esc_html($co{'author'}) . "</td></tr>\n".
-	      "<tr>" .
-	      "<td></td><td> $ad{'rfc2822'}";
-	if ($ad{'hour_local'} < 6) {
-		printf(" (<span class=\"atnight\">%02d:%02d</span> %s)",
-		       $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
-	} else {
-		printf(" (%02d:%02d %s)",
-		       $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
-	}
-	print "</td>" .
-	      "</tr>\n";
-	print "<tr><td>committer</td><td>" . esc_html($co{'committer'}) . "</td></tr>\n";
-	print "<tr><td></td><td> $cd{'rfc2822'}" .
-	      sprintf(" (%02d:%02d %s)", $cd{'hour_local'}, $cd{'minute_local'}, $cd{'tz_local'}) .
-	      "</td></tr>\n";
+	git_print_full_authorship(\%co);
 	print "<tr><td>commit</td><td class=\"sha1\">$co{'id'}</td></tr>\n";
 	print "<tr>" .
 	      "<td>tree</td>" .
@@ -5579,7 +5595,7 @@ sub git_commitdiff {
 		git_header_html(undef, $expires);
 		git_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash, $formats_nav);
 		git_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash);
-		git_print_authorship(\%co);
+		git_print_authorship(\%co, 'localtime' => 1);
 		print "<div class=\"page_body\">\n";
 		if (@{$co{'comment'}} > 1) {
 			print "<div class=\"log\">\n";
-- 
1.6.3.rc1.192.gdbfcb

[PATCHv6 2/8] gitweb: uniform author info for commit and commitdiff

[Giuseppe Bilotta]

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 9b60418..cdfd1d5 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5595,7 +5595,11 @@ sub git_commitdiff {
 		git_header_html(undef, $expires);
 		git_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash, $formats_nav);
 		git_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash);
-		git_print_authorship(\%co, 'localtime' => 1);
+		print "<div class=\"title_text\">\n" .
+		      "<table class=\"object_header\">\n";
+		git_print_full_authorship(\%co);
+		print "</table>".
+		      "</div>\n";
 		print "<div class=\"page_body\">\n";
 		if (@{$co{'comment'}} > 1) {
 			print "<div class=\"log\">\n";
-- 
1.6.3.rc1.192.gdbfcb

[PATCHv6 3/8] gitweb: right-align date cell in shortlog

[Giuseppe Bilotta]

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.css |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index 68b22ff..7240ed7 100644
--- a/gitweb/gitweb.css
+++ b/gitweb/gitweb.css
@@ -180,6 +180,10 @@ table {
 	border-spacing: 0;
 }
 
+table.shortlog td:first-child{
+	text-align: right;
+}
+
 table.diff_tree {
 	font-family: monospace;
 }
-- 
1.6.3.rc1.192.gdbfcb

[PATCHv6 4/8] gitweb: (gr)avatar support

[Giuseppe Bilotta]

Introduce avatar support: the featuer adds the appropriate img tag next
to author and committer in commit(diff), history, shortlog and log view.
Multiple avatar providers are possible, but only gravatar is implemented
at the moment (and not enabled by default).

Gravatar support depends on Digest::MD5, which is a core package since
Perl 5.8. If gravatars are activated but Digest::MD5 cannot be found,
the feature will be automatically disabled.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.css  |    4 +++
 gitweb/gitweb.perl |   67 ++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 67 insertions(+), 4 deletions(-)

diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index 7240ed7..ad82f86 100644
--- a/gitweb/gitweb.css
+++ b/gitweb/gitweb.css
@@ -28,6 +28,10 @@ img.logo {
 	border-width: 0px;
 }
 
+img.avatar {
+	vertical-align: middle;
+}
+
 div.page_header {
 	height: 25px;
 	padding: 8px;
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index cdfd1d5..f2e0cfe 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -195,6 +195,14 @@ our %known_snapshot_format_aliases = (
 	'x-zip' => undef, '' => undef,
 );
 
+# Pixel sizes for icons and avatars. If the default font sizes or lineheights
+# are changed, it may be appropriate to change these values too via
+# $GITWEB_CONFIG.
+our %avatar_size = (
+	'default' => 16,
+	'double'  => 32
+);
+
 # You define site-wide feature defaults here; override them with
 # $GITWEB_CONFIG as necessary.
 our %feature = (
@@ -365,6 +373,22 @@ our %feature = (
 		'sub' => \&feature_patches,
 		'override' => 0,
 		'default' => [16]},
+
+	# Avatar support. When this feature is enabled, views such as
+	# shortlog or commit will display an avatar associated with
+	# the email of the committer(s) and/or author(s).
+
+	# Currently only the gravatar provider is available, and it
+	# depends on Digest::MD5.
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'avatar'}{'default'} = ['gravatar'];
+	# To have project specific config enable override in $GITWEB_CONFIG
+	# $feature{'avatar'}{'override'} = 1;
+	# and in project config gitweb.avatar = gravatar;
+	'avatar' => {
+		'override' => 0,
+		'default' => ['']},
 );
 
 sub gitweb_get_feature {
@@ -814,6 +838,13 @@ $git_dir = "$projectroot/$project" if $project;
 our @snapshot_fmts = gitweb_get_feature('snapshot');
 @snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);
 
+# check if avatars are enabled and dependencies are satisfied
+our ($git_avatar) = gitweb_get_feature('avatar');
+if (($git_avatar eq 'gravatar') &&
+   !(eval { require Digest::MD5; 1; })) {
+	$git_avatar = '';
+}
+
 # dispatch
 if (!defined $action) {
 	if (defined $hash) {
@@ -1476,7 +1507,9 @@ sub format_author_html {
 	my $tag = shift;
 	my $co = shift;
 	my $author = chop_and_escape_str($co->{'author_name'}, @_);
-	return "<$tag class=\"author\">" . $author . "</$tag>\n";
+	return "<$tag class=\"author\">" .
+	       git_get_avatar($co->{'author_email'}, 'pad_after' => 1) .
+	       $author . "</$tag>\n";
 }
 
 # format git diff header line, i.e. "diff --(git|combined|cc) ..."
@@ -3224,6 +3257,29 @@ sub git_print_header_div {
 	      "\n</div>\n";
 }
 
+# Insert an avatar for the given $email at the given $size if the feature
+# is enabled.
+sub git_get_avatar {
+	my ($email, %params) = @_;
+	my $pre_white  = ($params{'pad_before'} ? "&nbsp;" : "");
+	my $post_white = ($params{'pad_after'}  ? "&nbsp;" : "");
+	my $size = $avatar_size{$params{'size'}} || $avatar_size{'default'};
+	my $url = "";
+	if ($git_avatar eq 'gravatar') {
+		$url = "http://www.gravatar.com/avatar.php?gravatar_id=" .
+			Digest::MD5::md5_hex(lc $email) . "&amp;size=$size";
+	}
+	# Currently only gravatars are supported, but other forms such as
+	# picons can be added by putting an else up here and defining $url
+	# as needed. If no variant puts something in $url, we assume avatars
+	# are completely disabled/unavailable.
+	if ($url) {
+		return $pre_white . "<img class=\"avatar\" src=\"$url\" />" . $post_white;
+	} else {
+		return "";
+	}
+}
+
 # Outputs the author name and date in long form
 sub git_print_authorship {
 	my $co = shift;
@@ -3243,7 +3299,8 @@ sub git_print_authorship {
 			       $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
 		}
 	}
-	print "]</$tag>\n";
+	print "]" . git_get_avatar($co->{'author_email'}, 'pad_before' => 1)
+		  . "</$tag>\n";
 }
 
 # Outputs table rows containign the full author and commiter information.
@@ -3251,7 +3308,8 @@ sub git_print_full_authorship {
 	my $co = shift;
 	my %ad = parse_date($co->{'author_epoch'}, $co->{'author_tz'});
 	my %cd = parse_date($co->{'committer_epoch'}, $co->{'committer_tz'});
-	print "<tr><td>author</td><td>" . esc_html($co->{'author'}) . "</td></tr>\n".
+	print "<tr><td>author</td><td>" . esc_html($co->{'author'}) . "</td>".
+	      "<td rowspan=\"2\">" .git_get_avatar($co->{'author_email'}, 'size' => 'double') . "</td></tr>\n" .
 	      "<tr>" .
 	      "<td></td><td> $ad{'rfc2822'}";
 	if ($ad{'hour_local'} < 6) {
@@ -3263,7 +3321,8 @@ sub git_print_full_authorship {
 	}
 	print "</td>" .
 	      "</tr>\n";
-	print "<tr><td>committer</td><td>" . esc_html($co->{'committer'}) . "</td></tr>\n";
+	print "<tr><td>committer</td><td>" . esc_html($co->{'committer'}) . "</td>".
+	      "<td rowspan=\"2\">" .git_get_avatar($co->{'committer_email'}, 'size' => 'double') . "</td></tr>\n";
 	print "<tr><td></td><td> $cd{'rfc2822'}" .
 	      sprintf(" (%02d:%02d %s)", $cd{'hour_local'}, $cd{'minute_local'}, $cd{'tz_local'}) .
 	      "</td></tr>\n";
-- 
1.6.3.rc1.192.gdbfcb

[PATCHv6 5/8] gitweb: gravatar url cache

[Giuseppe Bilotta]

Views which contain many occurrences of the same email address (e.g.
shortlog view) benefit from not having to recalculate the MD5 of the
email address every time.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |   24 ++++++++++++++++++++++--
 1 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index f2e0cfe..d3bc849 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3257,6 +3257,27 @@ sub git_print_header_div {
 	      "\n</div>\n";
 }
 
+# Rather than recomputing the url for an email multiple times, we cache it
+# after the first hit. This gives a visible benefit in views where the avatar
+# for the same email is used repeatedly (e.g. shortlog).
+# The cache is shared by all avatar engines (currently gravatar only), which
+# are free to use it as preferred. Since only one avatar engine is used for any
+# given page, there's no risk for cache conflicts.
+our %avatar_cache = ();
+
+# Compute the gravatar url for a given email, if it's not in the cache already.
+# Gravatar stores only the part of the URL before the size, since that's the
+# one computationally more expensive. This also allows reuse of the cache for
+# different sizes (for this particular engine).
+sub gravatar_url {
+	my $email = lc shift;
+	my $size = shift;
+	$avatar_cache{$email} ||=
+		"http://www.gravatar.com/avatar.php?gravatar_id=" .
+			Digest::MD5::md5_hex($email) . "&amp;size=";
+	return $avatar_cache{$email} . $size;
+}
+
 # Insert an avatar for the given $email at the given $size if the feature
 # is enabled.
 sub git_get_avatar {
@@ -3266,8 +3287,7 @@ sub git_get_avatar {
 	my $size = $avatar_size{$params{'size'}} || $avatar_size{'default'};
 	my $url = "";
 	if ($git_avatar eq 'gravatar') {
-		$url = "http://www.gravatar.com/avatar.php?gravatar_id=" .
-			Digest::MD5::md5_hex(lc $email) . "&amp;size=$size";
+		$url = gravatar_url($email, $size);
 	}
 	# Currently only gravatars are supported, but other forms such as
 	# picons can be added by putting an else up here and defining $url
-- 
1.6.3.rc1.192.gdbfcb

[PATCHv6 6/8] gitweb: add 'alt' to avatar images

[Giuseppe Bilotta]

Without it, text-only browsers display the URL of the avatar, which can
be long and not very informative. We use the avatar provider name for
the alt text.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index d3bc849..3e6786b 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3294,7 +3294,7 @@ sub git_get_avatar {
 	# as needed. If no variant puts something in $url, we assume avatars
 	# are completely disabled/unavailable.
 	if ($url) {
-		return $pre_white . "<img class=\"avatar\" src=\"$url\" />" . $post_white;
+		return $pre_white . "<img class=\"avatar\" src=\"$url\" alt=\"$git_avatar\" />" . $post_white;
 	} else {
 		return "";
 	}
-- 
1.6.3.rc1.192.gdbfcb

[PATCHv6 7/8] gitweb: recognize 'trivial' acks

[Giuseppe Bilotta]

Sometimes patches are trivially acked rather than just acked, so
extend the signoff regexp to include this case.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 3e6786b..7ca115f 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3403,7 +3403,7 @@ sub git_print_log {
 	my $signoff = 0;
 	my $empty = 0;
 	foreach my $line (@$log) {
-		if ($line =~ m/^ *(signed[ \-]off[ \-]by[ :]|acked[ \-]by[ :]|cc[ :])/i) {
+		if ($line =~ m/^ *(signed[ \-]off[ \-]by[ :]|(?:trivially[ \-])?acked[ \-]by[ :]|cc[ :])/i) {
 			$signoff = 1;
 			$empty = 0;
 			if (! $opts{'-remove_signoff'}) {
-- 
1.6.3.rc1.192.gdbfcb

[PATCHv6 8/8] gitweb: add avatar in signoff lines

[Giuseppe Bilotta]

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---

I can't say I'm really satisfied with the layout given by this patch.
A significant improvement could be obtained by turning the signoff
line block into a table with three/four columns (signoff, name,
email/avatar). But we cannot guarantee that signoff lines come all
together in a block, so this would be more complex to implement.

 gitweb/gitweb.perl |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 7ca115f..d385f55 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3407,7 +3407,10 @@ sub git_print_log {
 			$signoff = 1;
 			$empty = 0;
 			if (! $opts{'-remove_signoff'}) {
-				print "<span class=\"signoff\">" . esc_html($line) . "</span><br/>\n";
+				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";
 				next;
 			} else {
 				# remove signoff lines
-- 
1.6.3.rc1.192.gdbfcb

Re: [PATCHv6 0/8] gitweb: gravatar support

[Jakub Narebski]

Giuseppe Bilotta wrote:

> Significant changes from the previous iteration are:
> 
> * the feature has been renamed to 'avatar', and 'gravatar' is a possible
>   value for it (currently the only sensible value, other than '');

By the way, I think it might be better solution to provide picon URL
as 'default' attribute for gravatar URL, so it is used if there is no
gravatar for given email.

> * the last patch adds avatars to signoff lines.

Perhaps it would be better to add gravatars at beginning of line?


I'll try to post my comments today (i.e. within 24 hours)... but it
looks good.

-- 
Jakub Narebski
Poland

Re: [PATCHv6 0/8] gitweb: gravatar support

[Giuseppe Bilotta]

2009/6/25 Jakub Narebski <jnareb@gmail.com>:
> Giuseppe Bilotta wrote:
>
>> Significant changes from the previous iteration are:
>>
>> * the feature has been renamed to 'avatar', and 'gravatar' is a possible
>>   value for it (currently the only sensible value, other than '');
>
> By the way, I think it might be better solution to provide picon URL
> as 'default' attribute for gravatar URL, so it is used if there is no
> gravatar for given email.

I was thinking about some form of fallback like that too, but I
haven't the slightest idea how picons work, so I'm afraid I'll leave
that enhancement to some later time.

>> * the last patch adds avatars to signoff lines.
>
> Perhaps it would be better to add gravatars at beginning of line?

I'm not sure. As I mention in the email for that commit, I'm totally
not satisfied with the layout. I'm lookint into turning signoff blocks
into tables.

> I'll try to post my comments today (i.e. within 24 hours)... but it
> looks good.

Thanks a lot.

-- 
Giuseppe "Oblomov" Bilotta

[PATCHv6 9/8] gitweb: put signoff lines in a table

[Giuseppe Bilotta]

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.

 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;
 }
 
+table.signoff td:first-child {
+	text-align: right;
+}
+
 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;
 			$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;
+			}
+			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'}) {
-- 
1.6.3.rc1.192.gdbfcb

Re: [PATCHv6 0/8] gitweb: gravatar support

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

> I was thinking about some form of fallback like that too, but I
> haven't the slightest idea how picons work, so I'm afraid I'll leave
> that enhancement to some later time.

Yeah, let's not go overboard with the initial series.

In order to lay a sound groundwork so that later patches can build on more
cleanly, it is good to talk about possibilities of adding support for
other services _later_ and assess how easy/hard it would be with the
proposed code structure.  Once it's done, and everybody is happy with the
result that it will be easy to work with, then we should leave later later
and move forward.

Re: [PATCHv6 0/8] gitweb: gravatar support

[Giuseppe Bilotta]

On Thu, Jun 25, 2009 at 7:07 PM, Junio C Hamano<gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> I was thinking about some form of fallback like that too, but I
>> haven't the slightest idea how picons work, so I'm afraid I'll leave
>> that enhancement to some later time.
>
> Yeah, let's not go overboard with the initial series.

Well, I'll confess that I've been on a coding frenzy all day, so
expect a new release with preliminary picon support as soon as the
review for the last patchset is done 8-D

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv6 0/8] gitweb: gravatar support

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

> On Thu, Jun 25, 2009 at 7:07 PM, Junio C Hamano<gitster@pobox.com> wrote:
>> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>>
>>> I was thinking about some form of fallback like that too, but I
>>> haven't the slightest idea how picons work, so I'm afraid I'll leave
>>> that enhancement to some later time.
>>
>> Yeah, let's not go overboard with the initial series.
>
> Well, I'll confess that I've been on a coding frenzy all day, so
> expect a new release with preliminary picon support as soon as the
> review for the last patchset is done 8-D

Ok, I saved v6 in my to-queue box planning to queue it in 'pu', but I'll
discard them and wait until the dust settles in v$n ($n >= 7).

Thanks.

Re: [PATCHv6 1/8] gitweb: refactor author name insertion

[Jakub Narebski]

On Thu, 25 June 2009, Giuseppe Bilotta wrote:

> Collect all author display code in appropriate functions, making it
> easiser to extend them on the CGI side, and rely on CSS rather than
> hard-coded HTML formatting for easier customization.

Typo: s/easiser/easier/ 

This paragraph could have been written better, perhaps by dividing it
in two sentences: one talking about collecting/factoring common code,
one talking about moving from presentational HTML (<i>, <b> tags) to
styling using CSS and class attribute.

Do I understand it correctly that this was meant as pure refactoring,
i.e. that none of gitweb output should have changed?  Because you made
a mistake, and 'log' view is broken (and it doesn't look like it did
before).  See comments below for cause and (simple) solution.

> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---

What are the changes as compared to previous (or just earlier) version?

>  gitweb/gitweb.css  |    5 ++-
>  gitweb/gitweb.perl |   80 +++++++++++++++++++++++++++++++---------------------
>  2 files changed, 52 insertions(+), 33 deletions(-)
> 
> diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
> index a01eac8..68b22ff 100644
> --- a/gitweb/gitweb.css
> +++ b/gitweb/gitweb.css
> @@ -132,11 +132,14 @@ div.list_head {
>  	font-style: italic;
>  }
>  
> +.author_date, .author {
> +	font-style: italic;
> +}
> +
>  div.author_date {
>  	padding: 8px;
>  	border: solid #d9d8d1;
>  	border-width: 0px 0px 1px 0px;
> -	font-style: italic;
>  }

Nice.

>  
>  a.list {
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 1e7e2d8..9b60418 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1469,6 +1469,16 @@ sub format_subject_html {
>  	}
>  }
>  
> +# format the author name of the given commit with the given tag
> +# the author name is chopped and escaped according to the other
> +# optional parameters (see chop_str).
> +sub format_author_html {
> +	my $tag = shift;
> +	my $co = shift;
> +	my $author = chop_and_escape_str($co->{'author_name'}, @_);
> +	return "<$tag class=\"author\">" . $author . "</$tag>\n";
> +}

Good... although I wonder if we should not get rid of chop_and_escape_str
altogether, and for example add title attribute (if needed due to having
to do shortening) directly to $tag, and not to inner <span> element.

Should "\n" be in returned string? Just asking.

> +
>  # format git diff header line, i.e. "diff --(git|combined|cc) ..."
>  sub format_git_diff_header_line {
>  	my $line = shift;
> @@ -3214,13 +3224,36 @@ sub git_print_header_div {
>  	      "\n</div>\n";
>  }
>  
> +# Outputs the author name and date in long form
>  sub git_print_authorship {
>  	my $co = shift;
> +	my %params = @_;
> +	my $tag = $params{'tag'} || 'div';

Nice using of hash to have named optional params.

Usually though we use %opts and not %params for the name of this
hash, and we use CGI-like keys prefixed by '-', for example
'-z' in parse_ls_tree_line(), '-nbsp' in esc_html, '-nohtml' in 
quot_cec(),  '-remove_title', '-remove_signoff' and '-final_empty_line'
in git_print_log().  git_commitdiff() uses %params, but it doesn't
have non-optional parameters (still, I guess we should use %opts
for consistency), and it uses '-format' and '-single' as names.

href() subroutine uses %params... but those are not extra named
optional parameters to subroutine; they are CGI query parameters.


NOTE: Default tag (as used in git_log) should be not generic block 
element tag: 'div', but rather generic inline element: 'span'.  The
presentational HTML element '<i>' which we are replacing is inline
(layout) element.

It is because it is '<div>' (and probably because some "random" CSS
rules in gitweb.css match it) it breaks layout or 'log' view.  There
two horizontal lines: one for <div> element, and one for unnecessary
now <br> element.  As div.author_date it has extra 8px padding, from
which the top padding is visible as extra unnecessary vertical space.

>  
>  	my %ad = parse_date($co->{'author_epoch'}, $co->{'author_tz'});
> -	print "<div class=\"author_date\">" .
> +	print "<$tag class=\"author_date\">" .
>  	      esc_html($co->{'author_name'}) .
>  	      " [$ad{'rfc2822'}";
> +	if ($params{'localtime'}) {
> +		if ($ad{'hour_local'} < 6) {
> +			printf(" (<span class=\"atnight\">%02d:%02d</span> %s)",
> +			       $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
> +		} else {
> +			printf(" (%02d:%02d %s)",
> +			       $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
> +		}
> +	}
> +	print "]</$tag>\n";
> +}

Gaah, git has chosen to show this diff a bit strangely...

> +
> +# Outputs table rows containign the full author and commiter information.

Typo: s/containign/containing/

You could say that it uses format used to show author and comitter fields
(headers) in 'commit' view.

> +sub git_print_full_authorship {
> +	my $co = shift;
> +	my %ad = parse_date($co->{'author_epoch'}, $co->{'author_tz'});
> +	my %cd = parse_date($co->{'committer_epoch'}, $co->{'committer_tz'});
> +	print "<tr><td>author</td><td>" . esc_html($co->{'author'}) . "</td></tr>\n".
> +	      "<tr>" .
> +	      "<td></td><td> $ad{'rfc2822'}";
>  	if ($ad{'hour_local'} < 6) {
>  		printf(" (<span class=\"atnight\">%02d:%02d</span> %s)",
>  		       $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
> @@ -3228,7 +3261,12 @@ sub git_print_authorship {
>  		printf(" (%02d:%02d %s)",
>  		       $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
>  	}
> -	print "]</div>\n";
> +	print "</td>" .
> +	      "</tr>\n";
> +	print "<tr><td>committer</td><td>" . esc_html($co->{'committer'}) . "</td></tr>\n";
> +	print "<tr><td></td><td> $cd{'rfc2822'}" .
> +	      sprintf(" (%02d:%02d %s)", $cd{'hour_local'}, $cd{'minute_local'}, $cd{'tz_local'}) .
> +	      "</td></tr>\n";
>  }

By the way, what about author / tagger info used in 'tag' view?
Wouldn't it be better to factor out generating table rows for single
author / committer / tagger header (field) info?

>  
>  sub git_print_page_path {
> @@ -4142,11 +4180,9 @@ sub git_shortlog_body {
>  			print "<tr class=\"light\">\n";
>  		}
>  		$alternate ^= 1;
> -		my $author = chop_and_escape_str($co{'author_name'}, 10);
>  		# git_summary() used print "<td><i>$co{'age_string'}</i></td>\n" .
>  		print "<td title=\"$co{'age_string_age'}\"><i>$co{'age_string_date'}</i></td>\n" .
> -		      "<td><i>" . $author . "</i></td>\n" .
> -		      "<td>";
> +		      format_author_html('td', \%co, 10) . "<td>";
>  		print format_subject_html($co{'title'}, $co{'title_short'},
>  		                          href(action=>"commit", hash=>$commit), $ref);
>  		print "</td>\n" .

Nice refactoring and simplification.

> @@ -4193,11 +4229,9 @@ sub git_history_body {
>  			print "<tr class=\"light\">\n";
>  		}
>  		$alternate ^= 1;
> -	# shortlog uses      chop_str($co{'author_name'}, 10)
> -		my $author = chop_and_escape_str($co{'author_name'}, 15, 3);
>  		print "<td title=\"$co{'age_string_age'}\"><i>$co{'age_string_date'}</i></td>\n" .
> -		      "<td><i>" . $author . "</i></td>\n" .
> -		      "<td>";
> +	# shortlog:   format_author_html('td', \%co, 10)
> +		      format_author_html('td', \%co, 15, 3) . "<td>";
>  		# originally git_history used chop_str($co{'title'}, 50)
>  		print format_subject_html($co{'title'}, $co{'title_short'},
>  		                          href(action=>"commit", hash=>$commit), $ref);

Nice refactoring and simplification.
Good comment.

> @@ -4350,9 +4384,8 @@ sub git_search_grep_body {
>  			print "<tr class=\"light\">\n";
>  		}
>  		$alternate ^= 1;
> -		my $author = chop_and_escape_str($co{'author_name'}, 15, 5);
>  		print "<td title=\"$co{'age_string_age'}\"><i>$co{'age_string_date'}</i></td>\n" .
> -		      "<td><i>" . $author . "</i></td>\n" .
> +		      format_author_html('td', \%co, 15, 5) .
>  		      "<td>" .
>  		      $cgi->a({-href => href(action=>"commit", hash=>$co{'id'}),
>  		               -class => "list subject"},

Nice refactoring and simplification.

> @@ -5094,9 +5127,9 @@ sub git_log {
>  		      " | " .
>  		      $cgi->a({-href => href(action=>"tree", hash=>$commit, hash_base=>$commit)}, "tree") .
>  		      "<br/>\n" .
> -		      "</div>\n" .
> -		      "<i>" . esc_html($co{'author_name'}) .  " [$ad{'rfc2822'}]</i><br/>\n" .
>  		      "</div>\n";
> +		      git_print_authorship(\%co);
> +		      print "<br/>\n</div>\n";
>  
>  		print "<div class=\"log_body\">\n";
>  		git_print_log($co{'comment'}, -final_empty_line=> 1);

Here you replace inline <i> element with <div> _block_ element, 
destroying layout.  Replacing default of 'div' by default of
inline <span> element restores old layout.

> @@ -5115,8 +5148,6 @@ sub git_commit {
>  	$hash ||= $hash_base || "HEAD";
>  	my %co = parse_commit($hash)
>  	    or die_error(404, "Unknown commit object");
> -	my %ad = parse_date($co{'author_epoch'}, $co{'author_tz'});
> -	my %cd = parse_date($co{'committer_epoch'}, $co{'committer_tz'});
>  
>  	my $parent  = $co{'parent'};
>  	my $parents = $co{'parents'}; # listref
> @@ -5183,22 +5214,7 @@ sub git_commit {
>  	}
>  	print "<div class=\"title_text\">\n" .
>  	      "<table class=\"object_header\">\n";
> -	print "<tr><td>author</td><td>" . esc_html($co{'author'}) . "</td></tr>\n".
> -	      "<tr>" .
> -	      "<td></td><td> $ad{'rfc2822'}";
> -	if ($ad{'hour_local'} < 6) {
> -		printf(" (<span class=\"atnight\">%02d:%02d</span> %s)",
> -		       $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
> -	} else {
> -		printf(" (%02d:%02d %s)",
> -		       $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
> -	}
> -	print "</td>" .
> -	      "</tr>\n";
> -	print "<tr><td>committer</td><td>" . esc_html($co{'committer'}) . "</td></tr>\n";
> -	print "<tr><td></td><td> $cd{'rfc2822'}" .
> -	      sprintf(" (%02d:%02d %s)", $cd{'hour_local'}, $cd{'minute_local'}, $cd{'tz_local'}) .
> -	      "</td></tr>\n";
> +	git_print_full_authorship(\%co);
>  	print "<tr><td>commit</td><td class=\"sha1\">$co{'id'}</td></tr>\n";
>  	print "<tr>" .
>  	      "<td>tree</td>" .

I'd rather use here (as mentioned in comment about git_print_full_authorship
subroutine) something like the following:

+	git_print_authorship_header(\%co, 'author');
+	git_print_authorship_header(\%co, 'committer');

Or something like that.  But this might be a matter of taste.

> @@ -5579,7 +5595,7 @@ sub git_commitdiff {
>  		git_header_html(undef, $expires);
>  		git_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash, $formats_nav);
>  		git_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash);
> -		git_print_authorship(\%co);
> +		git_print_authorship(\%co, 'localtime' => 1);
>  		print "<div class=\"page_body\">\n";
>  		if (@{$co{'comment'}} > 1) {
>  			print "<div class=\"log\">\n";

Nice.  Although... 'localtime' / '-localtime'?  Perhaps '-show_localtime'?
But that is just nitpicking (naming is hard), and I am not sure if 
proposed name is any better.

> -- 
> 1.6.3.rc1.192.gdbfcb
> 
> 

Thank you for diligent work on this patch series!

-- 
Jakub Narebski
Poland

Re: [PATCHv6 1/8] gitweb: refactor author name insertion

[Jakub Narebski]

On Fri, 26 June 2009, Jakub Narebski wrote:
> On Thu, 25 June 2009, Giuseppe Bilotta wrote:

> > @@ -5094,9 +5127,9 @@ sub git_log {
> >  		      " | " .
> >  		      $cgi->a({-href => href(action=>"tree", hash=>$commit, hash_base=>$commit)}, "tree") .
> >  		      "<br/>\n" .
> > -		      "</div>\n" .
> > -		      "<i>" . esc_html($co{'author_name'}) .  " [$ad{'rfc2822'}]</i><br/>\n" .
> >  		      "</div>\n";
> > +		      git_print_authorship(\%co);
> > +		      print "<br/>\n</div>\n";
> >  
> >  		print "<div class=\"log_body\">\n";
> >  		git_print_log($co{'comment'}, -final_empty_line=> 1);
> 
> Here you replace inline <i> element with <div> _block_ element, 
> destroying layout.  Replacing default of 'div' by default of
> inline <span> element restores old layout.

Note: here we want <span>.

> > @@ -5579,7 +5595,7 @@ sub git_commitdiff {
> >  		git_header_html(undef, $expires);
> >  		git_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash, $formats_nav);
> >  		git_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash);
> > -		git_print_authorship(\%co);
> > +		git_print_authorship(\%co, 'localtime' => 1);
> >  		print "<div class=\"page_body\">\n";
> >  		if (@{$co{'comment'}} > 1) {
> >  			print "<div class=\"log\">\n";

Note: here we want <div>.

Whether default would be 'span' or 'div', at least one of those has to
pass explicit 'tag' / '-tag' parameter.  I guess it would be easier to
pass 'tag' => 'span' in git_log().

-- 
Jakub Narebski
Poland

Re: [PATCHv6 2/8] gitweb: uniform author info for commit and commitdiff

[Jakub Narebski]

On Thu, 25 June 2009, Giuseppe Bilotta wrote:

Here I would write that 'commitdiff' view moves from

  Giuseppe Bilotta [Mon, 22 Jun 2009 22:49:58 +0000 (00:49 +0200)]

to

  author      Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
              Mon, 22 Jun 2009 22:49:58 +0000 (00:49 +0200)
  committer   Jakub Narebski <jnareb@gmail.com>
              Tue, 23 Jun 2009 18:02:21 +0000 (20:02 +0200)"

(perhaps with A U Thor and C O Mitter as example names).

> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.perl |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 9b60418..cdfd1d5 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -5595,7 +5595,11 @@ sub git_commitdiff {
>  		git_header_html(undef, $expires);
>  		git_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash, $formats_nav);
>  		git_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash);
> -		git_print_authorship(\%co, 'localtime' => 1);
> +		print "<div class=\"title_text\">\n" .
> +		      "<table class=\"object_header\">\n";
> +		git_print_full_authorship(\%co);
> +		print "</table>".
> +		      "</div>\n";
>  		print "<div class=\"page_body\">\n";
>  		if (@{$co{'comment'}} > 1) {
>  			print "<div class=\"log\">\n";

Nice and short thanks to refactoring you have done in previous patch.


Very good that you put this in separate patch, so it can be evaluated
independently, and decided independently whether it is worth having more
detailed authorship information in 'commitdiff', making it more like
'commit' view, or be more like 'log' view with similar, but slightly
extended authorship information.

I personally am a bit ambivalent about this issue...

> -- 
> 1.6.3.rc1.192.gdbfcb
> 
> 

-- 
Jakub Narebski
Poland

Re: [PATCHv6 0/8] gitweb: gravatar support

[Jakub Narebski]

On Thu, 25 June 2009 14:55, Jakub Narebski wrote:

> I'll try to post my comments today (i.e. within 24 hours)... but it
> looks good.

I'm sorry, I will post comments for the rest of patches in series 
tomorrow (but I'll try to be within those 24 hours).

-- 
Jakub Narebski
Poland

Re: [PATCHv6 1/8] gitweb: refactor author name insertion

[Giuseppe Bilotta]

2009/6/26 Jakub Narebski <jnareb@gmail.com>:
> Do I understand it correctly that this was meant as pure refactoring,
> i.e. that none of gitweb output should have changed?  Because you made
> a mistake, and 'log' view is broken (and it doesn't look like it did
> before).  See comments below for cause and (simple) solution.

Thanks for noticing. And yes, the problem is indeed that I forgot to
specify tag => span in git log view (I'm keeping div the default
because that's what was there in the first place in
git_print_authorship).

>> +# format the author name of the given commit with the given tag
>> +# the author name is chopped and escaped according to the other
>> +# optional parameters (see chop_str).
>> +sub format_author_html {
>> +     my $tag = shift;
>> +     my $co = shift;
>> +     my $author = chop_and_escape_str($co->{'author_name'}, @_);
>> +     return "<$tag class=\"author\">" . $author . "</$tag>\n";
>> +}
>
> Good... although I wonder if we should not get rid of chop_and_escape_str
> altogether, and for example add title attribute (if needed due to having
> to do shortening) directly to $tag, and not to inner <span> element.

This would require some additional refactoring, and I don't have a
clear idea on how to best implement it right now. I'm afraid it'll
have to wait for another time.

> Should "\n" be in returned string? Just asking.

You're right, we probably don't want to force the newline there. The
real question is, do we want the callers to put a newline there? I'm
thinking no, because it's mostly used in table cells and a newline is
better done at the row level, but I'm not sure either way. I'll just
remove it for the time being, it should only have effects at the
sourcecode level, not on the layout.

> Usually though we use %opts and not %params for the name of this
> hash, and we use CGI-like keys prefixed by '-', for example
> '-z' in parse_ls_tree_line(), '-nbsp' in esc_html, '-nohtml' in
> quot_cec(),  '-remove_title', '-remove_signoff' and '-final_empty_line'
> in git_print_log().  git_commitdiff() uses %params, but it doesn't
> have non-optional parameters (still, I guess we should use %opts
> for consistency), and it uses '-format' and '-single' as names.
>
> href() subroutine uses %params... but those are not extra named
> optional parameters to subroutine; they are CGI query parameters.

I'll adjust the code accordingly. BTW the %params in git_commitdiff is
my fault too, IIRC.

>>       my %ad = parse_date($co->{'author_epoch'}, $co->{'author_tz'});
>> -     print "<div class=\"author_date\">" .
>> +     print "<$tag class=\"author_date\">" .
>>             esc_html($co->{'author_name'}) .
>>             " [$ad{'rfc2822'}";
>> +     if ($params{'localtime'}) {
>> +             if ($ad{'hour_local'} < 6) {
>> +                     printf(" (<span class=\"atnight\">%02d:%02d</span> %s)",
>> +                            $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
>> +             } else {
>> +                     printf(" (%02d:%02d %s)",
>> +                            $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
>> +             }
>> +     }
>> +     print "]</$tag>\n";
>> +}
>
> Gaah, git has chosen to show this diff a bit strangely...

Oh, very funny indeed. I hadn't realized it went that way. Wonder if
the patience diff would have helped here.

> By the way, what about author / tagger info used in 'tag' view?

I totally forgot about that.

> Wouldn't it be better to factor out generating table rows for single
> author / committer / tagger header (field) info?

Good idea. I'm not sure the tagger field has all the relevant data. I'll check.

> I'd rather use here (as mentioned in comment about git_print_full_authorship
> subroutine) something like the following:
>
> +       git_print_authorship_header(\%co, 'author');
> +       git_print_authorship_header(\%co, 'committer');
>
> Or something like that.  But this might be a matter of taste.

I renamed the sub to git_print_authorship_rows, and I'm making it
accept a list of people to print info for. I'll make it default to
both author and committer though.


-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv6 3/8] gitweb: right-align date cell in shortlog

[Jakub Narebski]

On Thu, 25 June 2009, Giuseppe Bilotta wrote:

> diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
> index 68b22ff..7240ed7 100644
> --- a/gitweb/gitweb.css
> +++ b/gitweb/gitweb.css
> @@ -180,6 +180,10 @@ table {

> +table.shortlog td:first-child{
> +	text-align: right;
> +}

First, there is no space between ':first-child' pseudo-class selector
and opening '{'.  It should be "td:first-child {".

Second, I'd rather avoid more advanced CSS constructs; not all web
browsers support ':first-child' selector.  On the other hand adding
class attribute to handle this would make page slightly larger.

Last, and most important: I don't agree with this change.  In my
opinion it does not improve layout (and you didn't provide support
for this change).  Right-align justification should be sparingly,
as it is not natural in left-to-right languages.

NAK from me for this change.
-- 
Jakub Narebski
Poland

Re: [PATCHv6 2/8] gitweb: uniform author info for commit and commitdiff

[Giuseppe Bilotta]

2009/6/26 Jakub Narebski <jnareb@gmail.com>:
> On Thu, 25 June 2009, Giuseppe Bilotta wrote:
>
> Here I would write that 'commitdiff' view moves from
>
>  Giuseppe Bilotta [Mon, 22 Jun 2009 22:49:58 +0000 (00:49 +0200)]
>
> to
>
>  author      Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
>              Mon, 22 Jun 2009 22:49:58 +0000 (00:49 +0200)
>  committer   Jakub Narebski <jnareb@gmail.com>
>              Tue, 23 Jun 2009 18:02:21 +0000 (20:02 +0200)"
>
> (perhaps with A U Thor and C O Mitter as example names).

Done.

> Very good that you put this in separate patch, so it can be evaluated
> independently, and decided independently whether it is worth having more
> detailed authorship information in 'commitdiff', making it more like
> 'commit' view, or be more like 'log' view with similar, but slightly
> extended authorship information.
>
> I personally am a bit ambivalent about this issue...

I really don't understand why commit and commitdiff are so different,
but this is something we went over already. One thing that should be
kept in mind is that commitdiff can span multiple commits, but in this
case it only display the information about the first one (but this is
not something that is changed by my patch). commitdiff view probably
needs a complete rethinking.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv6 3/8] gitweb: right-align date cell in shortlog

[Giuseppe Bilotta]

2009/6/26 Jakub Narebski <jnareb@gmail.com>:
> On Thu, 25 June 2009, Giuseppe Bilotta wrote:
>
>> diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
>> index 68b22ff..7240ed7 100644
>> --- a/gitweb/gitweb.css
>> +++ b/gitweb/gitweb.css
>> @@ -180,6 +180,10 @@ table {
>
>> +table.shortlog td:first-child{
>> +     text-align: right;
>> +}
>
> First, there is no space between ':first-child' pseudo-class selector
> and opening '{'.  It should be "td:first-child {".

Right.

> Second, I'd rather avoid more advanced CSS constructs; not all web
> browsers support ':first-child' selector.  On the other hand adding
> class attribute to handle this would make page slightly larger.

IIRC :first-child is supported from IE7 onwards. There are hacks to
make it work on IE6, but I think they are definitely not worth it.

> Last, and most important: I don't agree with this change.  In my
> opinion it does not improve layout (and you didn't provide support
> for this change).  Right-align justification should be sparingly,
> as it is not natural in left-to-right languages.

Of course, in my opinion it does improve layout.

The effect is to right-laign the first column of shortlog view, i.e.
the one holding the date. For dates that are presented as yyyy-mm-dd
it makes not difference, but when the phrasing is 'X days ago' it
provides the benefit of aligning the 'days ago' part instead of having
it ragged. See it live at

http://git.oblomov.eu/git/shortlog

and judge for yourselves.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv6 4/8] gitweb: (gr)avatar support

[Jakub Narebski]

On Thu, 25 June 2009, Giuseppe Bilotta wrote:

> Introduce avatar support: the featuer adds the appropriate img tag next
> to author and committer in commit(diff), history, shortlog and log view.

You forgot about 'tag' view (but I guess it would be done in next 
version of this patch series).

There is also 'feed' action (Atom and RSS formats), but that is certainly
separate issue, for a separate patch.

> Multiple avatar providers are possible, but only gravatar is implemented
> at the moment (and not enabled by default).
> 
> Gravatar support depends on Digest::MD5, which is a core package since
> Perl 5.8. If gravatars are activated but Digest::MD5 cannot be found,
> the feature will be automatically disabled.
> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

Almost-Acked-by: Jakub Narebski <jnareb@gmail.com>

> ---
>  gitweb/gitweb.css  |    4 +++
>  gitweb/gitweb.perl |   67 ++++++++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 67 insertions(+), 4 deletions(-)

You would _probably_ want to squash the following, just in case:

diff --git i/t/t9500-gitweb-standalone-no-errors.sh w/t/t9500-gitweb-standalone-no-errors.sh
index d539619..e8b57c5 100755
--- i/t/t9500-gitweb-standalone-no-errors.sh
+++ w/t/t9500-gitweb-standalone-no-errors.sh
@@ -660,6 +660,7 @@ cat >>gitweb_config.perl <<EOF
 
 \$feature{'blame'}{'override'} = 1;
 \$feature{'snapshot'}{'override'} = 1;
+\$feature{'avatar'}{'override'} = 1;
 EOF
 
 test_expect_success \
@@ -678,6 +679,7 @@ test_expect_success \
 	'config override: tree view, features enabled in repo config (1)' \
 	'git config gitweb.blame yes &&
 	 git config gitweb.snapshot "zip,tgz, tbz2" &&
+	 git config gitweb.avatar gravatar &&
 	 gitweb_run "p=.git;a=tree"'
 test_debug 'cat gitweb.log'
 
(this is not yet a proper solution).

But even if you don't squash it, please run t9500 with this patch
applied, to catch Perl errors and warnings.

> 
> diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
> index 7240ed7..ad82f86 100644
> --- a/gitweb/gitweb.css
> +++ b/gitweb/gitweb.css
> @@ -28,6 +28,10 @@ img.logo {
>  	border-width: 0px;
>  }
>  
> +img.avatar {
> +	vertical-align: middle;
> +}
> +

Good.  All concerns were addressed.

>  div.page_header {
>  	height: 25px;
>  	padding: 8px;
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index cdfd1d5..f2e0cfe 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -195,6 +195,14 @@ our %known_snapshot_format_aliases = (
>  	'x-zip' => undef, '' => undef,
>  );
>  
> +# Pixel sizes for icons and avatars. If the default font sizes or lineheights
> +# are changed, it may be appropriate to change these values too via
> +# $GITWEB_CONFIG.
> +our %avatar_size = (
> +	'default' => 16,
> +	'double'  => 32
> +);
> +

Nice.  

>  # You define site-wide feature defaults here; override them with
>  # $GITWEB_CONFIG as necessary.
>  our %feature = (
> @@ -365,6 +373,22 @@ our %feature = (
>  		'sub' => \&feature_patches,
>  		'override' => 0,
>  		'default' => [16]},
> +
> +	# Avatar support. When this feature is enabled, views such as
> +	# shortlog or commit will display an avatar associated with
> +	# the email of the committer(s) and/or author(s).
> +
> +	# Currently only the gravatar provider is available, and it
> +	# depends on Digest::MD5.

Sidenote: Gravatar API description[1] mentions 'identicon', 'monsterid',
'wavatar'.  There are 'picons' (personal icons)[2].  Also avatars doesn't
need to be global: they can be some local static image somewhere in web
server which serves gitweb script, or they can be stored somewhere in
repository following some convention.

Current implementation is flexible enough to leave place for extending
this feature, but also doesn't try to plan too far in advance.  YAGNI
(You Ain't Gonna Need It).

[1] http://www.gravatar.com/site/implement/url
[2] http://www.cs.indiana.edu/picons/ftp/faq.html

> +
> +	# To enable system wide have in $GITWEB_CONFIG
> +	# $feature{'avatar'}{'default'} = ['gravatar'];
> +	# To have project specific config enable override in $GITWEB_CONFIG
> +	# $feature{'avatar'}{'override'} = 1;
> +	# and in project config gitweb.avatar = gravatar;
> +	'avatar' => {
> +		'override' => 0,
> +		'default' => ['']},

Note that to disable feature with non-boolean 'default' we use empty
list [] (which means 'undef' when parsing, which is false); see 
description of features 'snapshot', 'actions'; 'ctags' what is strange
uses [0] here...  Using [''] is a bit strange; and does not protect
you, I think.

>  );
>  
>  sub gitweb_get_feature {
> @@ -814,6 +838,13 @@ $git_dir = "$projectroot/$project" if $project;
>  our @snapshot_fmts = gitweb_get_feature('snapshot');
>  @snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);
>  
> +# check if avatars are enabled and dependencies are satisfied
> +our ($git_avatar) = gitweb_get_feature('avatar');

IMPORTANT!!!

Because you now allow possibility that there can be other avatars
than those provided by Gravatar, you should explain in comment
what this check below does (e.g. something like "load Digest::MD5,
required for gravatar support, and disable it if it does not exist"),
so people adding (in the future) support for other kind of avatars
would know that they should put similar test there, if needed.

> +if (($git_avatar eq 'gravatar') &&
> +   !(eval { require Digest::MD5; 1; })) {
> +	$git_avatar = '';
> +}

Here you would have to protect against $git_avatar being undefined...
but you should do it anyway, as gitweb_get_feature() can return 
undef / empty list.

So either

  +our ($git_avatar) = gitweb_get_feature('avatar') || '';

or

  +if (defined $git_avatar && $git_avatar eq 'gravatar' &&
  +   !(eval { require Digest::MD5; 1; })) {
  +	$git_avatar = '';
  +}


> +
>  # dispatch
>  if (!defined $action) {
>  	if (defined $hash) {
> @@ -1476,7 +1507,9 @@ sub format_author_html {
>  	my $tag = shift;
>  	my $co = shift;
>  	my $author = chop_and_escape_str($co->{'author_name'}, @_);
> -	return "<$tag class=\"author\">" . $author . "</$tag>\n";
> +	return "<$tag class=\"author\">" .
> +	       git_get_avatar($co->{'author_email'}, 'pad_after' => 1) .
> +	       $author . "</$tag>\n";
>  }

Ah, it uses the fact that format_author_html is used in 'history' and
'shortlog' views, to format only author name for 'Author' column. 

BTW. wouldn't it be better if git_get_avatar was defined _before_ this
use?


This might be good enough starting point, but I wonder if it wouldn't
be a better solution to provide additional column with avatar image
when avatar support is enabled.  You would get a better layout in
a very rare case[3] when 'Author' column is too narrow and author is
info is wrapped:

  [#] Jonathan
  H. Random

versus in separate columns case:

  [#] | Jonathan
      | H. Random

But this is a very minor problem, which can be left for separate patch.

[3] unless you use netbook or phone to browse...

>  
>  # format git diff header line, i.e. "diff --(git|combined|cc) ..."
> @@ -3224,6 +3257,29 @@ sub git_print_header_div {
>  	      "\n</div>\n";
>  }
>  
> +# Insert an avatar for the given $email at the given $size if the feature
> +# is enabled.
> +sub git_get_avatar {
> +	my ($email, %params) = @_;
> +	my $pre_white  = ($params{'pad_before'} ? "&nbsp;" : "");
> +	my $post_white = ($params{'pad_after'}  ? "&nbsp;" : "");
> +	my $size = $avatar_size{$params{'size'}} || $avatar_size{'default'};

The same comment as for first patch in series: gitweb uses 
%opts not %params, and '-size' not 'size' (CGI-like).

> +	my $url = "";
> +	if ($git_avatar eq 'gravatar') {
> +		$url = "http://www.gravatar.com/avatar.php?gravatar_id=" .
> +			Digest::MD5::md5_hex(lc $email) . "&amp;size=$size";
> +	}
> +	# Currently only gravatars are supported, but other forms such as
> +	# picons can be added by putting an else up here and defining $url
> +	# as needed. If no variant puts something in $url, we assume avatars
> +	# are completely disabled/unavailable.

Very nice solution to provide support for future alternate avatar
sources, like picons or local images (for a gitweb installation).

> +	if ($url) {
> +		return $pre_white . "<img class=\"avatar\" src=\"$url\" />" . $post_white;
> +	} else {
> +		return "";
> +	}
> +}

Nice idea.

> +
>  # Outputs the author name and date in long form
>  sub git_print_authorship {
>  	my $co = shift;
> @@ -3243,7 +3299,8 @@ sub git_print_authorship {
>  			       $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
>  		}
>  	}
> -	print "]</$tag>\n";
> +	print "]" . git_get_avatar($co->{'author_email'}, 'pad_before' => 1)
> +		  . "</$tag>\n";
>  }

Nice solution for 'log' and perhaps 'commitdiff' views.

>  
>  # Outputs table rows containign the full author and commiter information.
> @@ -3251,7 +3308,8 @@ sub git_print_full_authorship {
>  	my $co = shift;
>  	my %ad = parse_date($co->{'author_epoch'}, $co->{'author_tz'});
>  	my %cd = parse_date($co->{'committer_epoch'}, $co->{'committer_tz'});
> -	print "<tr><td>author</td><td>" . esc_html($co->{'author'}) . "</td></tr>\n".
> +	print "<tr><td>author</td><td>" . esc_html($co->{'author'}) . "</td>".
> +	      "<td rowspan=\"2\">" .git_get_avatar($co->{'author_email'}, 'size' => 'double') . "</td></tr>\n" .

Nitpick: perhaps it would be better to put git_get_avatar invocation
in separate line, to limit line length.

Minor nitpick: use ". git_get_avatar(...", i.e. space between '.' string
concatenation operator and 'git_get_avatar'.

>  	      "<tr>" .
>  	      "<td></td><td> $ad{'rfc2822'}";
>  	if ($ad{'hour_local'} < 6) {
> @@ -3263,7 +3321,8 @@ sub git_print_full_authorship {
>  	}
>  	print "</td>" .
>  	      "</tr>\n";
> -	print "<tr><td>committer</td><td>" . esc_html($co->{'committer'}) . "</td></tr>\n";
> +	print "<tr><td>committer</td><td>" . esc_html($co->{'committer'}) . "</td>".
> +	      "<td rowspan=\"2\">" .git_get_avatar($co->{'committer_email'}, 'size' => 'double') . "</td></tr>\n";
>  	print "<tr><td></td><td> $cd{'rfc2822'}" .
>  	      sprintf(" (%02d:%02d %s)", $cd{'hour_local'}, $cd{'minute_local'}, $cd{'tz_local'}) .
>  	      "</td></tr>\n";

Same as above.

BTW. if you refactor printing _single_ authorship info, either into
separate subroutine or as code in (short) loop, you wouldn't have
this code repetition.  OTOH there would be also 'committing by night'
warning, like current 'coding by night' (localtime)...


Nice solution for 'commit' and with 2nd patch in series also 
'commitdiff' view.

> -- 
> 1.6.3.rc1.192.gdbfcb
> 
> 

-- 
Jakub Narebski
Poland

Re: [PATCHv6 4/8] gitweb: (gr)avatar support

[Giuseppe Bilotta]

2009/6/26 Jakub Narebski <jnareb@gmail.com>:
> On Thu, 25 June 2009, Giuseppe Bilotta wrote:
>
>> Introduce avatar support: the featuer adds the appropriate img tag next
>> to author and committer in commit(diff), history, shortlog and log view.
>
> You forgot about 'tag' view (but I guess it would be done in next
> version of this patch series).

Indeed, it's automatically addressed because patch view follows the refactoring.

> There is also 'feed' action (Atom and RSS formats), but that is certainly
> separate issue, for a separate patch.

I'm not entirely sure we want avatars there.

> You would _probably_ want to squash the following, just in case:

[snip]

> But even if you don't squash it, please run t9500 with this patch
> applied, to catch Perl errors and warnings.

I'll squash it in.

> Sidenote: Gravatar API description[1] mentions 'identicon', 'monsterid',
> 'wavatar'.  There are 'picons' (personal icons)[2].  Also avatars doesn't
> need to be global: they can be some local static image somewhere in web
> server which serves gitweb script, or they can be stored somewhere in
> repository following some convention.
>
> Current implementation is flexible enough to leave place for extending
> this feature, but also doesn't try to plan too far in advance.  YAGNI
> (You Ain't Gonna Need It).
>
> [1] http://www.gravatar.com/site/implement/url
> [2] http://www.cs.indiana.edu/picons/ftp/faq.html

The forthcoming series has picons provider and gravatar fallback;
however, we might want to have some way to make the gravatar fallback
configurable.

>> +     # To enable system wide have in $GITWEB_CONFIG
>> +     # $feature{'avatar'}{'default'} = ['gravatar'];
>> +     # To have project specific config enable override in $GITWEB_CONFIG
>> +     # $feature{'avatar'}{'override'} = 1;
>> +     # and in project config gitweb.avatar = gravatar;
>> +     'avatar' => {
>> +             'override' => 0,
>> +             'default' => ['']},
>
> Note that to disable feature with non-boolean 'default' we use empty
> list [] (which means 'undef' when parsing, which is false); see
> description of features 'snapshot', 'actions'; 'ctags' what is strange
> uses [0] here...  Using [''] is a bit strange; and does not protect
> you, I think.

Using an empty string (or 0 like ctags do) is nice because it spares
the undef check you mention later on, and since empty strings and 0
evaluate to false in Perl, it's a good way to handle it. Moreover, any
string which is not an actual provider would result in no avatars.
More about this later.

>> +# check if avatars are enabled and dependencies are satisfied
>> +our ($git_avatar) = gitweb_get_feature('avatar');
>
> IMPORTANT!!!
>
> Because you now allow possibility that there can be other avatars
> than those provided by Gravatar, you should explain in comment
> what this check below does (e.g. something like "load Digest::MD5,
> required for gravatar support, and disable it if it does not exist"),
> so people adding (in the future) support for other kind of avatars
> would know that they should put similar test there, if needed.

I'll make the check and subsequent switch a little cleaner.

>> +if (($git_avatar eq 'gravatar') &&
>> +   !(eval { require Digest::MD5; 1; })) {
>> +     $git_avatar = '';
>> +}
>
> Here you would have to protect against $git_avatar being undefined...
> but you should do it anyway, as gitweb_get_feature() can return
> undef / empty list.

Using '' as defalt instead of [] shields me from this problem, and
works properly for boolean checks.

> This might be good enough starting point, but I wonder if it wouldn't
> be a better solution to provide additional column with avatar image
> when avatar support is enabled.  You would get a better layout in
> a very rare case[3] when 'Author' column is too narrow and author is
> info is wrapped:
>
>  [#] Jonathan
>  H. Random
>
> versus in separate columns case:
>
>  [#] | Jonathan
>      | H. Random
>
> But this is a very minor problem, which can be left for separate patch.
>
> [3] unless you use netbook or phone to browse...

I had considered going this way, but it made the code somewhat more
complex so I went for the simpler solution. I'll look into putting it
in separate cells further on.


-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv6 3/8] gitweb: right-align date cell in shortlog

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

> See it live at
>
> http://git.oblomov.eu/git/shortlog
>
> and judge for yourselves.

Yay, very nice to see these faces!  Thanks ;-)

Regardless of other points that may or may not have to be addressed, I
had to say this.

Re: [PATCHv6 3/8] gitweb: right-align date cell in shortlog

[Giuseppe Bilotta]

On Sat, Jun 27, 2009 at 12:34 AM, Junio C Hamano<gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> See it live at
>>
>> http://git.oblomov.eu/git/shortlog
>>
>> and judge for yourselves.
>
> Yay, very nice to see these faces!  Thanks ;-)
>
> Regardless of other points that may or may not have to be addressed, I
> had to say this.

I'm glad you like the faces 8-)

What's your opinion on the alignment for the date column in the shortlog?


-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv6 4/8] gitweb: (gr)avatar support

[Jakub Narebski]

On Sat, 27 Jun 2009, Giuseppe Bilotta wrote:
> 2009/6/26 Jakub Narebski <jnareb@gmail.com>:
>> On Thu, 25 June 2009, Giuseppe Bilotta wrote:
>>
>>> Introduce avatar support: the featuer adds the appropriate img tag next
>>> to author and committer in commit(diff), history, shortlog and log view.

[...]
>> There is also 'feed' action (Atom and RSS formats), but that is certainly
>> separate issue, for a separate patch.
> 
> I'm not entirely sure we want avatars there.

I think you are right. I have thought that Atom/RSS have place for icons
for each entry in feed, but it is not the case. Also feed reader can
use gravatars by itself. Sorry for the noise, then.

>> Sidenote: Gravatar API description[1] mentions 'identicon', 'monsterid',
>> 'wavatar'.  There are 'picons' (personal icons)[2].  Also avatars doesn't
>> need to be global: they can be some local static image somewhere in web
>> server which serves gitweb script, or they can be stored somewhere in
>> repository following some convention.
>>
>> Current implementation is flexible enough to leave place for extending
>> this feature, but also doesn't try to plan too far in advance.  YAGNI
>> (You Ain't Gonna Need It).
>>
>> [1] http://www.gravatar.com/site/implement/url
>> [2] http://www.cs.indiana.edu/picons/ftp/faq.html
> 
> The forthcoming series has picons provider and gravatar fallback;
> however, we might want to have some way to make the gravatar fallback
> configurable.

Do I understand this correctly that there is additional patch planned
in new release of this series providing support for gitweb.avatar = picon?

>>> +     # To enable system wide have in $GITWEB_CONFIG
>>> +     # $feature{'avatar'}{'default'} = ['gravatar'];
>>> +     # To have project specific config enable override in $GITWEB_CONFIG
>>> +     # $feature{'avatar'}{'override'} = 1;
>>> +     # and in project config gitweb.avatar = gravatar;
>>> +     'avatar' => {
>>> +             'override' => 0,
>>> +             'default' => ['']},

NOTE, NOTE, NOTE!

One thing I forgot about (and would be discovered when running t9500
with provided patch... among other errors) is that you need to provide
'sub' subroutine for feature to be overridable.

...And that subroutine would be responsible for returning '' (empty
string) when feature is overridable.  See other feature_* subroutines.

>>
>> Note that to disable feature with non-boolean 'default' we use empty
>> list [] (which means 'undef' when parsing, which is false); see
>> description of features 'snapshot', 'actions'; 'ctags' what is strange
>> uses [0] here...  Using [''] is a bit strange; and does not protect
>> you, I think.
> 
> Using an empty string (or 0 like ctags do) is nice because it spares
> the undef check you mention later on, and since empty strings and 0
> evaluate to false in Perl, it's a good way to handle it. Moreover, any
> string which is not an actual provider would result in no avatars.
> More about this later.

[...]
>>> +if (($git_avatar eq 'gravatar') &&
>>> +   !(eval { require Digest::MD5; 1; })) {
>>> +     $git_avatar = '';
>>> +}
>>
>> Here you would have to protect against $git_avatar being undefined...
>> but you should do it anyway, as gitweb_get_feature() can return
>> undef / empty list.
> 
> Using '' as defalt instead of [] shields me from this problem, and
> works properly for boolean checks.

Well, I can understand that.  I was wrong: it is up to (currently not
defined) 'sub' to ensure that gitweb_get_feature would return ''
('default').


Still,

  +our ($git_avatar) = gitweb_get_feature('avatar') || '';

is quite simple... and can be in future subtly wrong (unless it would
be gitweb_check_feature, which always return scalar / single element).


>> This might be good enough starting point, but I wonder if it wouldn't
>> be a better solution to provide additional column with avatar image
>> when avatar support is enabled.  You would get a better layout in
>> a very rare case[3] when 'Author' column is too narrow and author is
>> info is wrapped:
>>
>>  [#] Jonathan
>>  H. Random
>>
>> versus in separate columns case:
>>
>>  [#] | Jonathan
>>      | H. Random
>>
>> But this is a very minor problem, which can be left for separate patch.
>>
>> [3] unless you use netbook or phone to browse...
> 
> I had considered going this way, but it made the code somewhat more
> complex so I went for the simpler solution. I'll look into putting it
> in separate cells further on.

Well, by "left for later" here I thought about later as in after this
patch series about gravatars get accepted :-)

-- 
Jakub Narebski
Poland

Re: [PATCHv6 5/8] gitweb: gravatar url cache

[Jakub Narebski]

On Thu, 25 Jun 2009, Giuseppe Bilotta wrote:

> Views which contain many occurrences of the same email address (e.g.
> shortlog view) benefit from not having to recalculate the MD5 of the
> email address every time.

It would be nice to have some benchmarks comparing performance before
and after this patch.

> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

Acked-by: Jakub Narebski <jnareb@gmail.com>

> ---
>  gitweb/gitweb.perl |   24 ++++++++++++++++++++++--
>  1 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index f2e0cfe..d3bc849 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3257,6 +3257,27 @@ sub git_print_header_div {
>  	      "\n</div>\n";
>  }
>  
> +# Rather than recomputing the url for an email multiple times, we cache it
> +# after the first hit. This gives a visible benefit in views where the avatar
> +# for the same email is used repeatedly (e.g. shortlog).
> +# The cache is shared by all avatar engines (currently gravatar only), which
> +# are free to use it as preferred. Since only one avatar engine is used for any
> +# given page, there's no risk for cache conflicts.
> +our %avatar_cache = ();

Nice explanation.

> +
> +# Compute the gravatar url for a given email, if it's not in the cache already.
> +# Gravatar stores only the part of the URL before the size, since that's the
> +# one computationally more expensive. This also allows reuse of the cache for
> +# different sizes (for this particular engine).
> +sub gravatar_url {
> +	my $email = lc shift;
> +	my $size = shift;
> +	$avatar_cache{$email} ||=
> +		"http://www.gravatar.com/avatar.php?gravatar_id=" .
> +			Digest::MD5::md5_hex($email) . "&amp;size=";
> +	return $avatar_cache{$email} . $size;
> +}

Nice solution. Very good.

I guess it is not worth it to _not_ use cache for few avatars views 
such as 'commit', 'commitdiff', in the future also 'tag' view, isn't it?

BTW. http://www.gravatar.com/site/implement/url recommends
http://www.gravatar.com/avatar/3b3be63a4c2a439b013787725dfce802 rather than
http://www.gravatar.com/avatar.php?gravatar_id=3b3be63a4c2a439b013787725dfce802
you use, following http://www.gravatar.com/site/implement/perl
Hmmm...

> +
>  # Insert an avatar for the given $email at the given $size if the feature
>  # is enabled.
>  sub git_get_avatar {
> @@ -3266,8 +3287,7 @@ sub git_get_avatar {
>  	my $size = $avatar_size{$params{'size'}} || $avatar_size{'default'};
>  	my $url = "";
>  	if ($git_avatar eq 'gravatar') {
> -		$url = "http://www.gravatar.com/avatar.php?gravatar_id=" .
> -			Digest::MD5::md5_hex(lc $email) . "&amp;size=$size";
> +		$url = gravatar_url($email, $size);
>  	}
>  	# Currently only gravatars are supported, but other forms such as
>  	# picons can be added by putting an else up here and defining $url

Very nice.

> -- 
> 1.6.3.rc1.192.gdbfcb
> 
> 

-- 
Jakub Narebski
Poland

Re: [PATCHv6 4/8] gitweb: (gr)avatar support

[Giuseppe Bilotta]

On Sat, Jun 27, 2009 at 12:58 AM, Jakub Narebski<jnareb@gmail.com> wrote:
> Do I understand this correctly that there is additional patch planned
> in new release of this series providing support for gitweb.avatar = picon?

Yes.

And a separate patch makes the picon the fallback for gravatar too,
but I'm thinking about having something like a gitweb.gravatar_default
(or something like that) to make that configurable.

>>>> +     # To enable system wide have in $GITWEB_CONFIG
>>>> +     # $feature{'avatar'}{'default'} = ['gravatar'];
>>>> +     # To have project specific config enable override in $GITWEB_CONFIG
>>>> +     # $feature{'avatar'}{'override'} = 1;
>>>> +     # and in project config gitweb.avatar = gravatar;
>>>> +     'avatar' => {
>>>> +             'override' => 0,
>>>> +             'default' => ['']},
>
> NOTE, NOTE, NOTE!
>
> One thing I forgot about (and would be discovered when running t9500
> with provided patch... among other errors) is that you need to provide
> 'sub' subroutine for feature to be overridable.

Duh I had forgotten aout that. But I ran the test and didn't see that
error ... maybe I'm reading the log incorrectly.

> ...And that subroutine would be responsible for returning '' (empty
> string) when feature is overridable.  See other feature_* subroutines.

I've tried an implementation. Hopefully it's correct 8-/

>> I had considered going this way, but it made the code somewhat more
>> complex so I went for the simpler solution. I'll look into putting it
>> in separate cells further on.
>
> Well, by "left for later" here I thought about later as in after this
> patch series about gravatars get accepted :-)

Me too 8-D

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv6 4/8] gitweb: (gr)avatar support

[Jakub Narebski]

Dnia sobota 27. czerwca 2009 01:14, Giuseppe Bilotta napisał:
> On Sat, Jun 27, 2009 at 12:58 AM, Jakub Narebski<jnareb@gmail.com> wrote:

> > Do I understand this correctly that there is additional patch planned
> > in new release of this series providing support for gitweb.avatar = picon?
> 
> Yes.
> 
> And a separate patch makes the picon the fallback for gravatar too,
> but I'm thinking about having something like a gitweb.gravatar_default
> (or something like that) to make that configurable.

I'm not sure if having picons as 'default' for gravatar is a good idea,
because the rules for resolving picons are complicated[1]... which 
I didn't realize when writing this (sketch of) idea.

[1] http://www.cs.indiana.edu/picons/ftp/faq.html
-- 
Jakub Narebski
Poland

Re: [PATCHv6 5/8] gitweb: gravatar url cache

[Giuseppe Bilotta]

2009/6/27 Jakub Narebski <jnareb@gmail.com>:
> On Thu, 25 Jun 2009, Giuseppe Bilotta wrote:
>
>> Views which contain many occurrences of the same email address (e.g.
>> shortlog view) benefit from not having to recalculate the MD5 of the
>> email address every time.
>
> It would be nice to have some benchmarks comparing performance before
> and after this patch.

Indeed it would.

Oh, you mean I should provide them? 8-D

(I'll see if I can get around it this weekend)

> I guess it is not worth it to _not_ use cache for few avatars views
> such as 'commit', 'commitdiff', in the future also 'tag' view, isn't it?

I would say not.

> BTW. http://www.gravatar.com/site/implement/url recommends
> http://www.gravatar.com/avatar/3b3be63a4c2a439b013787725dfce802 rather than
> http://www.gravatar.com/avatar.php?gravatar_id=3b3be63a4c2a439b013787725dfce802
> you use, following http://www.gravatar.com/site/implement/perl

I think the perl code there is just obsolete (the /avarar/ thing is
more recent). I'll update to the new one because it's more compact.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv6 6/8] gitweb: add 'alt' to avatar images

[Jakub Narebski]

On Thu, 25 June 2009, Giuseppe Bilotta wrote:

> Without it, text-only browsers display the URL of the avatar, which can
> be long and not very informative. 

Actually different text-only web browsers behave differently (with
default settings).

Before this patch:
 * lynx show basename of the URL of the avatar, which ain't pretty
   [avatar.php?gravatar_id=955680802bc3d50476786bb3ca9cfc52&size=16]
 * elinks doesn't show by default images at all, which means that only
     used for padding will be visible (don't use  ?)
 * w3m show basename of avatar URL without extension nor query string:
   [avatar]

After this patch:
 * lynx   show alt text, i.e. 'gravatar' (no [] to mark as image)
 * elinks show alt text, i.e. 'gravatar' (no [] to mark as image)
 * w3m show alt text, i.e. gravatar, in separate color (also no [])

Lynx Version 2.8.5rel.1
ELinks 0.10.3
w3m version w3m/0.5.1+cvs-1.946

> We use the avatar provider name for the alt text.

I am not sure if 'type of avatar' wouldn't be easier to understand than
'avatar provider name'.  OTOH the latter is more correct.  Perhaps
"We use the avatar provider (type of avatar) for the alt text"?
But that is just nitpicking...

> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.perl |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index d3bc849..3e6786b 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3294,7 +3294,7 @@ sub git_get_avatar {
>  	# as needed. If no variant puts something in $url, we assume avatars
>  	# are completely disabled/unavailable.
>  	if ($url) {
> -		return $pre_white . "<img class=\"avatar\" src=\"$url\" />" . $post_white;
> +		return $pre_white . "<img class=\"avatar\" src=\"$url\" alt=\"$git_avatar\" />" . $post_white;
>  	} else {
>  		return "";
>  	}

Nice, simple improvement.

> -- 
> 1.6.3.rc1.192.gdbfcb
> 
> 

-- 
Jakub Narebski
Poland

Re: [PATCHv6 5/8] gitweb: gravatar url cache

[Jakub Narebski]

On Sat, 27 Jun 2009, Giuseppe Bilotta wrote:
> 2009/6/27 Jakub Narebski <jnareb@gmail.com>:

> > BTW. http://www.gravatar.com/site/implement/url recommends
> > http://www.gravatar.com/avatar/3b3be63a4c2a439b013787725dfce802 rather than
> > http://www.gravatar.com/avatar.php?gravatar_id=3b3be63a4c2a439b013787725dfce802
> > you use, following http://www.gravatar.com/site/implement/perl
> 
> I think the perl code there is just obsolete (the /avatar/ thing is
> more recent). I'll update to the new one because it's more compact.

I think that Gravatar::URL module on CPAN[1] (which we cannot use in
gitweb because it is extra nonstandard non-core not needed dependency)
also uses newer path_info form.

[1] http://p3rl.org/Gravatar::URL

-- 
Jakub Narebski
Poland

Re: [PATCHv6 3/8] gitweb: right-align date cell in shortlog

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

> What's your opinion on the alignment for the date column in the shortlog?

Even though I am very used to seeing the current output style that aligns
to the left edge, your output didn't bother me.  

Because visual preference typically is influenced very heavily by inertia,
I take this as a sign that (1) left or right it does not matter very much,
and/or (2) aligning to the right edge is slightly better.

But that is just my impression.

Re: [PATCHv6 6/8] gitweb: add 'alt' to avatar images

[Thomas Adam]

2009/6/27 Jakub Narebski <jnareb@gmail.com>:
> After this patch:
>  * lynx   show alt text, i.e. 'gravatar' (no [] to mark as image)
>  * elinks show alt text, i.e. 'gravatar' (no [] to mark as image)
>  * w3m show alt text, i.e. gravatar, in separate color (also no [])
>
> Lynx Version 2.8.5rel.1
> ELinks 0.10.3

That's an old version of ELinks.  ELinks 0.12 I think changed this
behaviour to at least try and display ALT tags by default.

-- Thomas Adam

Re: [PATCHv6 7/8] gitweb: recognize 'trivial' acks

[Jakub Narebski]

On Thu, 25 Jun 2009, Giuseppe Bilotta wrote:
> Sometimes patches are trivially acked rather than just acked, so
> extend the signoff regexp to include this case.
> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

This is straghtforward enough.

Acked-by: Jakub Narebski <jnareb@gmail.com>

> ---
>  gitweb/gitweb.perl |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 3e6786b..7ca115f 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3403,7 +3403,7 @@ sub git_print_log {
>  	my $signoff = 0;
>  	my $empty = 0;
>  	foreach my $line (@$log) {
> -		if ($line =~ m/^ *(signed[ \-]off[ \-]by[ :]|acked[ \-]by[ :]|cc[ :])/i) {
> +		if ($line =~ m/^ *(signed[ \-]off[ \-]by[ :]|(?:trivially[ \-])?acked[ \-]by[ :]|cc[ :])/i) {
>  			$signoff = 1;
>  			$empty = 0;
>  			if (! $opts{'-remove_signoff'}) {
> -- 
> 1.6.3.rc1.192.gdbfcb
> 
> 

-- 
Jakub Narebski
Poland

Re: [PATCHv6 4/8] gitweb: (gr)avatar support

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> I'm not sure if having picons as 'default' for gravatar is a good idea,
> because the rules for resolving picons are complicated[1]... which 
> I didn't realize when writing this (sketch of) idea.
>
> [1] http://www.cs.indiana.edu/picons/ftp/faq.html

Also picons these days look somewhat antiquated with its rather strict
limitation to the colormaps (IIRC, you practically have to go grayscale if
you want to use a real photo), and it would make sense to try gravatar
first and then fall back to picons.

Re: [PATCHv6 4/8] gitweb: (gr)avatar support

[Giuseppe Bilotta]

On Sat, Jun 27, 2009 at 2:29 AM, Junio C Hamano<gitster@pobox.com> wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>
>> I'm not sure if having picons as 'default' for gravatar is a good idea,
>> because the rules for resolving picons are complicated[1]... which
>> I didn't realize when writing this (sketch of) idea.
>>
>> [1] http://www.cs.indiana.edu/picons/ftp/faq.html
>
> Also picons these days look somewhat antiquated with its rather strict
> limitation to the colormaps (IIRC, you practically have to go grayscale if
> you want to use a real photo), and it would make sense to try gravatar
> first and then fall back to picons.

That's what gravatar's default is for: if the image is not set, use
whatever is provided as default (with this patch, the picon).


-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv6 7/8] gitweb: recognize 'trivial' acks

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

>  	foreach my $line (@$log) {
> -		if ($line =~ m/^ *(signed[ \-]off[ \-]by[ :]|acked[ \-]by[ :]|cc[ :])/i) {
> +		if ($line =~ m/^ *(signed[ \-]off[ \-]by[ :]|(?:trivially[ \-])?acked[ \-]by[ :]|cc[ :])/i) {

Wouldn't it make more sense to do something like:

	if ($line =~ m/^[-a-z]+: (.*)$/i && looks_like_a_name_and_email($1)) {
            	... do the avatar thing ...
	}

and limit this processing only to the last run of no-blank lines?

Re: [PATCHv6 7/8] gitweb: recognize 'trivial' acks

[Giuseppe Bilotta]

On Sat, Jun 27, 2009 at 3:03 AM, Junio C Hamano<gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>>       foreach my $line (@$log) {
>> -             if ($line =~ m/^ *(signed[ \-]off[ \-]by[ :]|acked[ \-]by[ :]|cc[ :])/i) {
>> +             if ($line =~ m/^ *(signed[ \-]off[ \-]by[ :]|(?:trivially[ \-])?acked[ \-]by[ :]|cc[ :])/i) {
>
> Wouldn't it make more sense to do something like:
>
>        if ($line =~ m/^[-a-z]+: (.*)$/i && looks_like_a_name_and_email($1)) {
>                ... do the avatar thing ...
>        }
>
> and limit this processing only to the last run of no-blank lines?

I've been thinking about doing something like this, especially since
the next iteration this patch also adds 'Looks-fine-to-me-by' (spearce
really _loves_ customizing its signoffs lines during reviews 8-D).

The main difference between the two sections of the code is that the
original code allows whitespace instead of dashes in the signoff
lead-in, and the lead-in itself is OPTIONALLY terminated by a colon.

If we can lose this, then ^\s*[-a-z]+: followed by what looks like a
name and email can be a detector. An alternative would be to keep the
current signoff detector as-is and ADD the generic one, possibly under
the condition that we are already parsing signoffs.

The idea of accepting those lines as signoffs only at the end of the
commit log is also a good idea, but has the problem of requiring some
form of look-ahead. Two possible logics:

(1) when something that looks like a signoff is met, stash it away,
and keep collecting signoff-lookalikes and empty lines until either a
non-signoff-lookalike (in which case the previous lines are NOT
signoff) or the log end is met (in which case we print them as signoff
lines).

(2) same as before, but two or more consecutive signoff-lookalikes are
a signoff block regardless of what follows them.

Which one should I go with?


-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv6 8/8] gitweb: add avatar in signoff lines

[Jakub Narebski]

On Thu, 25 June 2009, Giuseppe Bilotta wrote:

> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

BTW. if it is an RFC, it should be marked as RFC in subject
("[RFC PATCHv6 8/8] gitweb: add avatar in signoff lines").

And I guess this issue should be left for later.

> ---
> 
> I can't say I'm really satisfied with the layout given by this patch.
> A significant improvement could be obtained by turning the signoff
> line block into a table with three/four columns (signoff, name,
> email/avatar). 

First, I think adding (gr)avatars to signoff lines might be not worth
it.  Neither GitHub nor Gitorious show gravatars for signoff lines
(note that they use larger images, and therefore easier to view).

Second, it is not the only possible layout.  Let's use one of existing
commits (e1d3793) in git repository as example:

  completion: add --thread=deep/shallow to format-patch

  [1] Signed-off-by: Stephen Boyd <bebarino@gmail.com> [2]          [3]            [4]|
  [1] Trivially-Acked-By: Shawn O. Pearce <spearce@spearce.org> [2] [3]            [4]|
  [1] Signed-off-by: Junio C Hamano <gitster@pobox.com> [2]         [3]            [4]|

Even without changing layout of signoff lines (so they look exactly
like they look in git-show or git-log output, modulo highlighting
and (gr)avatars), there are more possibilities:

 1. On the left side of signoff lines
 2. Current version: on the right side of signoff lines, just after
 3. On the right hand side, aligned; would probably need table
 4. On the right hand side, flushed (floated) right

There is also more complicated solution of having (gr)avatars appear
only on mouseover, either all avatars on hover over signoff block,
or single (and perhaps larger size) avatar on hover over signoff line.
This can be done using pure CSS, without JavaScript[1]

[1] http://meyerweb.com/eric/css/edge/popups/demo2.html

> But we cannot guarantee that signoff lines come all
> together in a block, so this would be more complex to implement.

Actually I think we can assume that signoff lines come all together
in single block at the very end of commit message; we should take
into account possibility of spurious empty lines between signoff
lines, though (it did happen, see e.g. 8dfb17e1).

> 
>  gitweb/gitweb.perl |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 7ca115f..d385f55 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3407,7 +3407,10 @@ sub git_print_log {
>  			$signoff = 1;
>  			$empty = 0;
>  			if (! $opts{'-remove_signoff'}) {
> -				print "<span class=\"signoff\">" . esc_html($line) . "</span><br/>\n";
> +				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";
>  				next;
>  			} else {
>  				# remove signoff lines
> -- 

And here is version with (gr)avatar on the left side of signoff lines
(take a look if it is not better layout):

diff --git c/gitweb/gitweb.perl w/gitweb/gitweb.perl
index 301bdd8..7701bac 100755
--- c/gitweb/gitweb.perl
+++ w/gitweb/gitweb.perl
@@ -3407,6 +3407,8 @@ sub git_print_log {
 			$signoff = 1;
 			$empty = 0;
 			if (! $opts{'-remove_signoff'}) {
+				my ($email) = $line =~ /<(\S+@\S+)>/;
+				print git_get_avatar($email, 'pad_after' => 1) if $email;
 				print "<span class=\"signoff\">" . esc_html($line) . "</span><br/>\n";
 				next;
 			} else {


-- 
Jakub Narebski
Poland

Re: [PATCHv6 9/8] gitweb: put signoff lines in a table

[Jakub Narebski]

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

Re: [PATCHv6 8/8] gitweb: add avatar in signoff lines

[Giuseppe Bilotta]

2009/6/27 Jakub Narebski <jnareb@gmail.com>:
> On Thu, 25 June 2009, Giuseppe Bilotta wrote:
>
>> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
>
> BTW. if it is an RFC, it should be marked as RFC in subject
> ("[RFC PATCHv6 8/8] gitweb: add avatar in signoff lines").
>
> And I guess this issue should be left for later.


You're right. My next patchset will not include this part (I'll send
it separately).

>> I can't say I'm really satisfied with the layout given by this patch.
>> A significant improvement could be obtained by turning the signoff
>> line block into a table with three/four columns (signoff, name,
>> email/avatar).
>
> First, I think adding (gr)avatars to signoff lines might be not worth
> it.  Neither GitHub nor Gitorious show gravatars for signoff lines
> (note that they use larger images, and therefore easier to view).

Well, just because the others don't do it, it doesn't mean we
shouldn't either ;-)

But yes, I have to confess I love toying around like this.

> Second, it is not the only possible layout.  Let's use one of existing
> commits (e1d3793) in git repository as example:
>
>  completion: add --thread=deep/shallow to format-patch
>
>  [1] Signed-off-by: Stephen Boyd <bebarino@gmail.com> [2]          [3]            [4]|
>  [1] Trivially-Acked-By: Shawn O. Pearce <spearce@spearce.org> [2] [3]            [4]|
>  [1] Signed-off-by: Junio C Hamano <gitster@pobox.com> [2]         [3]            [4]|
>
> Even without changing layout of signoff lines (so they look exactly
> like they look in git-show or git-log output, modulo highlighting
> and (gr)avatars), there are more possibilities:
>
>  1. On the left side of signoff lines
>  2. Current version: on the right side of signoff lines, just after
>  3. On the right hand side, aligned; would probably need table
>  4. On the right hand side, flushed (floated) right

I have a table implementation running @ http://git.oblomov.eu/git/commit/e1d3793

> There is also more complicated solution of having (gr)avatars appear
> only on mouseover, either all avatars on hover over signoff block,
> or single (and perhaps larger size) avatar on hover over signoff line.
> This can be done using pure CSS, without JavaScript[1]
>
> [1] http://meyerweb.com/eric/css/edge/popups/demo2.html

Oh, that'd be an interesting variant. Straightforward to implement in
the table case, too.

> And here is version with (gr)avatar on the left side of signoff lines
> (take a look if it is not better layout):
>
> diff --git c/gitweb/gitweb.perl w/gitweb/gitweb.perl
> index 301bdd8..7701bac 100755
> --- c/gitweb/gitweb.perl
> +++ w/gitweb/gitweb.perl
> @@ -3407,6 +3407,8 @@ sub git_print_log {
>                        $signoff = 1;
>                        $empty = 0;
>                        if (! $opts{'-remove_signoff'}) {
> +                               my ($email) = $line =~ /<(\S+@\S+)>/;
> +                               print git_get_avatar($email, 'pad_after' => 1) if $email;
>                                print "<span class=\"signoff\">" . esc_html($line) . "</span><br/>\n";
>                                next;
>                        } else {

I think that putting them aligned on the right is somewhat better
because both in commit(diff) and in log view the author/committer
avatar is already on the right.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv6 3/8] gitweb: right-align date cell in shortlog

[Jakub Narebski]

On Fri, 26 June 2009, Giuseppe Bilotta wrote:
> 2009/6/26 Jakub Narebski <jnareb@gmail.com>:
>> On Thu, 25 June 2009, Giuseppe Bilotta wrote:
>>
>>> diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
>>> index 68b22ff..7240ed7 100644
>>> --- a/gitweb/gitweb.css
>>> +++ b/gitweb/gitweb.css
>>> @@ -180,6 +180,10 @@ table {
>>
>>> +table.shortlog td:first-child{
>>> +     text-align: right;
>>> +}

>> Second, I'd rather avoid more advanced CSS constructs; not all web
>> browsers support ':first-child' selector.  On the other hand adding
>> class attribute to handle this would make page slightly larger.
> 
> IIRC :first-child is supported from IE7 onwards. There are hacks to
> make it work on IE6, but I think they are definitely not worth it.

I was thinking here about more exotic web browsers, like Lynx, ELinks,
w3m (and w3m in Emacs), Konqueror (KHTML).  I think that both Opera
and Safari (and other browsers based on WebKit engine) support 
':first-child' pseudo-class selector.  Also I'd rather not start trend
to use more advanced parts of CSS...

On the other hand if we introduce 'age coloring' (as used in projects
list view), by using classes age0..age2, then we would be able to use
class selector instead of :first-child pseudo-class selector for that.

>> Last, and most important: I don't agree with this change.  In my
>> opinion it does not improve layout (and you didn't provide support
>> for this change).  Right-align justification should be sparingly,
>> as it is not natural in left-to-right languages.
> 
> Of course, in my opinion it does improve layout.
> 
> The effect is to right-align the first column of shortlog view, i.e.
> the one holding the date. For dates that are presented as yyyy-mm-dd
> it makes not difference, but when the phrasing is 'X days ago' it
> provides the benefit of aligning the 'days ago' part instead of having
> it ragged. See it live at
> 
> http://git.oblomov.eu/git/shortlog
> 
> and judge for yourselves.

First, it would be nice to have

  See it live at http://git.oblomov.eu/git/shortlog

in the patch comment (between "---\n" line and diffstat).

And I took a look how it looks like, with:
 $ <mark whole thread>
 $ <save as>
 $ git am -3 <file>
 $ stg uncommit -n <n>
 $ stg pop -a
 $ stg push
 $ gitweb-update.sh
 $ <view http://localhost/cgi-bin/gitweb/gitweb.cgi>
 $ ...


Second, even disregarding using ':first-child' pseudo-class selector
(it is not that important issue that it is required to have this
supported in all browsers), there is problem that this change is
_incomplete_.  Take a look at 'summary' view (probably most used
project specific action): in 'shortlog' you have date right-aligned,
while date column in 'heads' and 'tags' parts below you have date
left-aligned.

Third, in my opinion it does not improve layout.  You align on least
important part of relative date specification: on the word "ago".
Unit specifiers in relative date specification are of different length
so you don't have align (or rather have align in sort subsequences).

Compare:

  15 min ago
  6 hours ago
  10 hours ago
  2 days ago
  2 weeks ago
  6 months ago
  2009-06-12

with

    15 min ago
   6 hours ago
  10 hours ago
    2 days ago
   2 weeks ago
  6 months ago
    2009-06-12

What you probably want to have (and which I am not sure if it is worth
complication) is to align on first space/whitespace (align="char" char=" ")

  15 min ago
   6 hours ago
  10 hours ago
   2 days ago
   2 weeks ago
   6 months ago
  2009-06-12

or even

  15 min    ago
   6 hours  ago
  10 hours  ago
   2 days   ago
   2 weeks  ago
   6 months ago
  2009-06-12

-- 
Jakub Narebski
Poland

Re: [PATCHv6 3/8] gitweb: right-align date cell in shortlog

[Jakub Narebski]

On Sat, 27 Jun 2009, Jakub Narebski wrote:
> On Fri, 26 June 2009, Giuseppe Bilotta wrote:
>> 2009/6/26 Jakub Narebski <jnareb@gmail.com>:

>>> Last, and most important: I don't agree with this change.  In my
>>> opinion it does not improve layout (and you didn't provide support
>>> for this change).  Right-align justification should be sparingly,
>>> as it is not natural in left-to-right languages.
>> 
>> Of course, in my opinion it does improve layout.

[..]
> Compare:
> 
>   15 min ago
>   6 hours ago
>   10 hours ago
>   2 days ago
>   2 weeks ago
>   6 months ago
>   2009-06-12
> 
> with
> 
>     15 min ago
>    6 hours ago
>   10 hours ago
>     2 days ago
>    2 weeks ago
>   6 months ago
>     2009-06-12

It looks IMHO even worse if, because of limited screen width, 'date'
column needs to be wrapped.  See

  6 months
  ago

versus

   6 months
        ago

-- 
Jakub Narebski
Poland