[PATCHv7 0/9] gitweb: avatar support

[PATCHv7 0/9] gitweb: avatar support

[Giuseppe Bilotta]

One more attempt at adding avatar support to gitweb.

The most significant change since the previous iteration is the addition
of a picon provider, and its use as default from gravatar.

Most other patches were changed to follow Jakub's suggestions. In
particular, the two-line author/committer information was refactored
to reduce code duplication, with the benefit that it could also be used
in 'tag' view.

Giuseppe Bilotta (9):
  gitweb: refactor author name insertion
  gitweb: uniform author info for commit and commitdiff
  gitweb: use git_print_authorship_rows in 'tag' view too
  gitweb: right-align date cell in shortlog
  gitweb: (gr)avatar support
  gitweb: gravatar url cache
  gitweb: picon avatar provider
  gitweb: use picon for gravatar fallback
  gitweb: add alt text to avatar img

 gitweb/gitweb.css                      |   13 ++-
 gitweb/gitweb.perl                     |  238 ++++++++++++++++++++++++++------
 t/t9500-gitweb-standalone-no-errors.sh |    2 +
 3 files changed, 207 insertions(+), 46 deletions(-)

[PATCHv7 1/9] gitweb: refactor author name insertion

[Giuseppe Bilotta]

Collect all author display code in appropriate functions, making it
easier to extend these functions on the CGI side.

We also move some of the presentation code from hard-coded HTML to CSS,
for easier customization.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.css  |    5 ++-
 gitweb/gitweb.perl |   96 +++++++++++++++++++++++++++++++--------------------
 2 files changed, 62 insertions(+), 39 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..9be723c 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>";
+}
+
 # format git diff header line, i.e. "diff --(git|combined|cc) ..."
 sub format_git_diff_header_line {
 	my $line = shift;
@@ -3214,21 +3224,53 @@ 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 %opts = @_;
+	my $tag = $opts{-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 ($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'});
+	if ($opts{-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 containing the full author or committer information,
+# in the format expected for 'commit' view (& similia).
+# Parameters are a commit hash reference, followed by the list of people
+# to output information for. If the list is empty it defalts to both
+# author and committer.
+sub git_print_authorship_rows {
+	my $co = shift;
+	# too bad we can't use @people = @_ || ('author', 'committer')
+	my @people = @_;
+	@people = ('author', 'committer') unless @people;
+	foreach my $who (@people) {
+		my %ad = parse_date($co->{$who . '_epoch'}, $co->{$who . '_tz'});
+		print "<tr><td>$who</td><td>" . esc_html($co->{$who}) . "</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 "]</div>\n";
 }
 
 sub git_print_page_path {
@@ -4142,11 +4184,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 +4233,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 +4388,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 +5131,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, -tag => 'span');
+		      print "<br/>\n</div>\n";
 
 		print "<div class=\"log_body\">\n";
 		git_print_log($co{'comment'}, -final_empty_line=> 1);
@@ -5115,8 +5152,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 +5218,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_authorship_rows(\%co);
 	print "<tr><td>commit</td><td class=\"sha1\">$co{'id'}</td></tr>\n";
 	print "<tr>" .
 	      "<td>tree</td>" .
@@ -5579,7 +5599,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

[PATCHv7 2/9] gitweb: uniform author info for commit and commitdiff

[Giuseppe Bilotta]

Switch from

A U Thor <email@example.com> [date time]

to

author	A U Thor <email@example.com>
	date time
committer	C O Mitter <other.email@example.com>
	committer date time

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 9be723c..0d8005d 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5599,7 +5599,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_authorship_rows(\%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

[PATCHv7 3/9] gitweb: use git_print_authorship_rows in 'tag' view too

[Giuseppe Bilotta]

parse_tag has to be adapted to use the hash keys expected by
git_print_authorship_rows, which is not a problem since git_tag
is the only user of this sub.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 0d8005d..7183ad2 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2409,8 +2409,14 @@ sub parse_tag {
 			$tag{'name'} = $1;
 		} elsif ($line =~ m/^tagger (.*) ([0-9]+) (.*)$/) {
 			$tag{'author'} = $1;
-			$tag{'epoch'} = $2;
-			$tag{'tz'} = $3;
+			$tag{'author_epoch'} = $2;
+			$tag{'author_tz'} = $3;
+			if ($tag{'author'} =~ m/^([^<]+) <([^>]*)>/) {
+				$tag{'author_name'}  = $1;
+				$tag{'author_email'} = $2;
+			} else {
+				$tag{'author_name'} = $tag{'author'};
+			}
 		} elsif ($line =~ m/--BEGIN/) {
 			push @comment, $line;
 			last;
@@ -4623,11 +4629,7 @@ sub git_tag {
 	                                      $tag{'type'}) . "</td>\n" .
 	      "</tr>\n";
 	if (defined($tag{'author'})) {
-		my %ad = parse_date($tag{'epoch'}, $tag{'tz'});
-		print "<tr><td>author</td><td>" . esc_html($tag{'author'}) . "</td></tr>\n";
-		print "<tr><td></td><td>" . $ad{'rfc2822'} .
-			sprintf(" (%02d:%02d %s)", $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'}) .
-			"</td></tr>\n";
+		git_print_authorship_rows(\%tag, 'author');
 	}
 	print "</table>\n\n" .
 	      "</div>\n";
-- 
1.6.3.rc1.192.gdbfcb

[PATCHv7 4/9] 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..ef24a1b 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

[PATCHv7 5/9] gitweb: (gr)avatar support

[Giuseppe Bilotta]

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

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.

No avatar provider is selected by default, except in the t9500 test.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.css                      |    4 ++
 gitweb/gitweb.perl                     |   81 ++++++++++++++++++++++++++++++-
 t/t9500-gitweb-standalone-no-errors.sh |    2 +
 3 files changed, 84 insertions(+), 3 deletions(-)

diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index ef24a1b..cd8066d 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 7183ad2..ad9ae31 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,23 @@ 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' => {
+		'sub' => \&feature_avatar,
+		'override' => 0,
+		'default' => ['']},
 );
 
 sub gitweb_get_feature {
@@ -433,6 +458,16 @@ sub feature_patches {
 	return ($_[0]);
 }
 
+sub feature_avatar {
+	my @val = (git_get_project_config('avatar'));
+
+	if (@val) {
+		return @val;
+	}
+
+	return @_;
+}
+
 # checking HEAD file with -e is fragile if the repository was
 # initialized long time ago (i.e. symlink HEAD) and was pack-ref'ed
 # and then pruned.
@@ -814,6 +849,17 @@ $git_dir = "$projectroot/$project" if $project;
 our @snapshot_fmts = gitweb_get_feature('snapshot');
 @snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);
 
+# check that the avatar feature is set to a known provider name,
+# and for each provider check if the dependencies are satisfied.
+# if the provider name is invalid or the dependencies are not met,
+# reset $git_avatar to the empty string.
+our ($git_avatar) = gitweb_get_feature('avatar');
+if ($git_avatar eq 'gravatar') {
+	$git_avatar = '' unless (eval { require Digest::MD5; 1; });
+} else {
+	$git_avatar = '';
+}
+
 # dispatch
 if (!defined $action) {
 	if (defined $hash) {
@@ -1469,6 +1515,29 @@ sub format_subject_html {
 	}
 }
 
+# Insert an avatar for the given $email at the given $size if the feature
+# is enabled.
+sub git_get_avatar {
+	my ($email, %opts) = @_;
+	my $pre_white  = ($opts{'pad_before'} ? "&nbsp;" : "");
+	my $post_white = ($opts{'pad_after'}  ? "&nbsp;" : "");
+	my $size = $avatar_size{$opts{'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 width=\"$size\" class=\"avatar\" src=\"$url\" />" . $post_white;
+	} else {
+		return "";
+	}
+}
+
 # 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).
@@ -1476,7 +1545,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>";
+	return "<$tag class=\"author\">" .
+	       git_get_avatar($co->{'author_email'}, 'pad_after' => 1) .
+	       $author . "</$tag>";
 }
 
 # format git diff header line, i.e. "diff --(git|combined|cc) ..."
@@ -3249,7 +3320,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 containing the full author or committer information,
@@ -3264,7 +3336,10 @@ sub git_print_authorship_rows {
 	@people = ('author', 'committer') unless @people;
 	foreach my $who (@people) {
 		my %ad = parse_date($co->{$who . '_epoch'}, $co->{$who . '_tz'});
-		print "<tr><td>$who</td><td>" . esc_html($co->{$who}) . "</td></tr>\n".
+		print "<tr><td>$who</td><td>" . esc_html($co->{$who}) . "</td>" .
+		      "<td rowspan=\"2\">" .
+		      git_get_avatar($co->{$who . '_email'}, 'size' => 'double') .
+		      "</td></tr>\n" .
 		      "<tr>" .
 		      "<td></td><td> $ad{'rfc2822'}";
 		if ($ad{'hour_local'} < 6) {
diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh
index d539619..6275181 100755
--- a/t/t9500-gitweb-standalone-no-errors.sh
+++ b/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 \
@@ -671,6 +672,7 @@ test_expect_success \
 	'config override: tree view, features disabled in repo config' \
 	'git config gitweb.blame no &&
 	 git config gitweb.snapshot none &&
+	 git config gitweb.avatar gravatar &&
 	 gitweb_run "p=.git;a=tree"'
 test_debug 'cat gitweb.log'
 
-- 
1.6.3.rc1.192.gdbfcb

[PATCHv7 6/9] 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 |   30 +++++++++++++++++++++++++++---
 1 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index ad9ae31..de4cc63 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1515,6 +1515,27 @@ sub format_subject_html {
 	}
 }
 
+# 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 {
@@ -1524,15 +1545,18 @@ sub git_get_avatar {
 	my $size = $avatar_size{$opts{'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
 	# as needed. If no variant puts something in $url, we assume avatars
 	# are completely disabled/unavailable.
 	if ($url) {
-		return $pre_white . "<img width=\"$size\" class=\"avatar\" src=\"$url\" />" . $post_white;
+		return $pre_white .
+		       "<img width=\"$size\" " .
+		            "class=\"avatar\" " .
+		            "src=\"$url\" " .
+		       "/>" . $post_white;
 	} else {
 		return "";
 	}
-- 
1.6.3.rc1.192.gdbfcb

[PATCHv7 7/9] gitweb: picon avatar provider

[Giuseppe Bilotta]

Simple implementation of picon that only relies on the indiana.edu
database.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index de4cc63..ae73d45 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -378,14 +378,17 @@ our %feature = (
 	# 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.
+	# Currently available providers are gravatar and picon.
+
+	# Gravatar depends on Digest::MD5.
+	# Picon currently relies on the indiana.edu database.
 
 	# To enable system wide have in $GITWEB_CONFIG
-	# $feature{'avatar'}{'default'} = ['gravatar'];
+	# $feature{'avatar'}{'default'} = ['provider'];
+	# where provider is either gravatar or picon.
 	# To have project specific config enable override in $GITWEB_CONFIG
 	# $feature{'avatar'}{'override'} = 1;
-	# and in project config gitweb.avatar = gravatar;
+	# and in project config gitweb.avatar = provider;
 	'avatar' => {
 		'sub' => \&feature_avatar,
 		'override' => 0,
@@ -856,6 +859,8 @@ our @snapshot_fmts = gitweb_get_feature('snapshot');
 our ($git_avatar) = gitweb_get_feature('avatar');
 if ($git_avatar eq 'gravatar') {
 	$git_avatar = '' unless (eval { require Digest::MD5; 1; });
+} elsif ($git_avatar eq 'picon') {
+	# no dependencies
 } else {
 	$git_avatar = '';
 }
@@ -1523,6 +1528,17 @@ sub format_subject_html {
 # given page, there's no risk for cache conflicts.
 our %avatar_cache = ();
 
+# Compute the picon url for a given email, by using the picon search service over at
+# http://www.cs.indiana.edu/picons/search.html
+sub picon_url {
+	my $email = lc shift;
+	if (!$avatar_cache{$email}) {
+		my ($user, $domain) = split('@', $email);
+		$avatar_cache{$email} = "http://www.cs.indiana.edu/cgi-pub/kinzler/piconsearch.cgi/$domain/$user/users+domains+unknown/up/single";
+	}
+	return $avatar_cache{$email};
+}
+
 # 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
@@ -1546,6 +1562,8 @@ sub git_get_avatar {
 	my $url = "";
 	if ($git_avatar eq 'gravatar') {
 		$url = gravatar_url($email, $size);
+	} elsif ($git_avatar eq 'picon') {
+		$url = picon_url($email);
 	}
 	# 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

[PATCHv7 8/9] gitweb: use picon for gravatar fallback

[Giuseppe Bilotta]

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index ae73d45..e2638cb 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1546,9 +1546,13 @@ sub picon_url {
 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=";
+	if (!$avatar_cache{$email}) {
+		my $picon = CGI::escape(picon_url($email));
+		$avatar_cache{$email} = "http://www.gravatar.com/avatar.php?gravatar_id=" .
+			Digest::MD5::md5_hex($email) .
+			"&amp;default=$picon" .
+			"&amp;size=";
+	}
 	return $avatar_cache{$email} . $size;
 }
 
-- 
1.6.3.rc1.192.gdbfcb

[PATCHv7 9/9] gitweb: add alt text to avatar img

[Giuseppe Bilotta]

The alt text, set to the avatar provider (i.e. image type), is more
friendly than the img url in some text-only browsers.
---
 gitweb/gitweb.perl |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index e2638cb..dc6049a 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1578,6 +1578,7 @@ sub git_get_avatar {
 		       "<img width=\"$size\" " .
 		            "class=\"avatar\" " .
 		            "src=\"$url\" " .
+			    "alt=\"$git_avatar\" " .
 		       "/>" . $post_white;
 	} else {
 		return "";
-- 
1.6.3.rc1.192.gdbfcb

Re: [PATCHv7 1/9] gitweb: refactor author name insertion

[Jakub Narebski]

On Sat, 27 June 2009, Giuseppe Bilotta wrote:

> Collect all author display code in appropriate functions, making it
> easier to extend these functions on the CGI side.
> 
> We also move some of the presentation code from hard-coded HTML to CSS,
> for easier customization.

Very nicely written, compact yet comprehensive commit message.

It _might_ be worth adding that accidentally commit date got "atnight"
warning too.  I consider that improvement, even though for local commit
(committer == author) it duplicates information.  And it removes
(some of) code duplication.

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

If not for the fact that due to typo you lost localtime info from
'commitdiff' view, I would ACK it.

> ---
>  gitweb/gitweb.css  |    5 ++-
>  gitweb/gitweb.perl |   96 +++++++++++++++++++++++++++++++--------------------
>  2 files changed, 62 insertions(+), 39 deletions(-)

[...]
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 1e7e2d8..9be723c 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>";
> +}

In the future we might want to change it so instead of

  <$tag class="author"><span title="Joe Random Hacker">Joe Random... </span></$tag>

we would use 'title' attribute of $tag element

  <$tag class="author" title="Joe Random Hacker">Joe Random... </$tag>

But I guess this is not worth complicating code required to do this.

> +
>  # format git diff header line, i.e. "diff --(git|combined|cc) ..."
>  sub format_git_diff_header_line {
>  	my $line = shift;
> @@ -3214,21 +3224,53 @@ 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 %opts = @_;
> +	my $tag = $opts{-tag} || 'div';

BTW. gitweb is a bit inconsistent here.  Sometimes we use $opts{'-tag'}
form, and sometimes $opts{-tag} form like you use here.

(Actually we use '-' prefixing i.e. '-key' not 'key' to make it safe
to write  $opts{-key}  and  -key => 1).

>  
>  	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 ($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'});
> +	if ($opts{-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'});
> +		}
> +	}

By the way, this "atnight" warning is duplicated in subroutine below
git_print_authorship_rows(), but IMHO it is not something very important.
It can be left for later.

> +	print "]</$tag>\n";
> +}
> +
> +# Outputs table rows containing the full author or committer information,
> +# in the format expected for 'commit' view (& similia).
> +# Parameters are a commit hash reference, followed by the list of people
> +# to output information for. If the list is empty it defalts to both
> +# author and committer.
> +sub git_print_authorship_rows {
> +	my $co = shift;
> +	# too bad we can't use @people = @_ || ('author', 'committer')
> +	my @people = @_;
> +	@people = ('author', 'committer') unless @people;
> +	foreach my $who (@people) {
> +		my %ad = parse_date($co->{$who . '_epoch'}, $co->{$who . '_tz'});

Wouldn't

  $co->{"${who}_epoch"}

be simpler than

  $co->{$who . '_epoch'}

Also it is now %wd (%who_date) or just %date rather than 
%ad (%author_date) vs %cd (%committer_date).


Both of those issues are very, very minor.

> +		print "<tr><td>$who</td><td>" . esc_html($co->{$who}) . "</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'});
> +		}

This is a bit of code duplication with git_print_authorship(),
but I don't think it is something seriously to worry about.

> +		print "</td>" .
> +		      "</tr>\n";
>  	}

> -	print "]</div>\n";

Heh.  Strange quirks of diff algorithm... :-)

>  }
>  
>  sub git_print_page_path {
> @@ -4142,11 +4184,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.

> @@ -4193,11 +4233,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.  Good, well aligned comment.

> @@ -4350,9 +4388,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... although this set of diff chunks made me wonder why we
use "10" for 'shortlog', "15, 3" for 'history, and "15, 5" for 
'search' (grep).

> @@ -5094,9 +5131,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, -tag => 'span');
> +		      print "<br/>\n</div>\n";
>  
>  		print "<div class=\"log_body\">\n";
>  		git_print_log($co{'comment'}, -final_empty_line=> 1);

This preserves layout.  Good.

I see you choose to correct mentioned issue this way.

> @@ -5115,8 +5152,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 +5218,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_authorship_rows(\%co);
>  	print "<tr><td>commit</td><td class=\"sha1\">$co{'id'}</td></tr>\n";
>  	print "<tr>" .
>  	      "<td>tree</td>" .

Nice simplification.  Even without making 'commitdiff' use the same
authorship info layout like 'commit', and 'tag' reusing this subroutine
it might have been worth it.

> @@ -5579,7 +5599,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";

ATTENTION !!!

You should have

  -		git_print_authorship(\%co);
  +		git_print_authorship(\%co, '-localtime' => 1);

here ('-localtime' instead of 'localtime'), otherwise you will loose
localtime information (and "atnight" warning) from 'commitdiff' view.


Thanks again for your diligent work on this patch series!

-- 
Jakub Narebski
Poland

Re: [PATCHv7 1/9] gitweb: refactor author name insertion

[Giuseppe Bilotta]

2009/6/27 Jakub Narebski <jnareb@gmail.com>:
> By the way, this "atnight" warning is duplicated in subroutine below
> git_print_authorship_rows(), but IMHO it is not something very important.
> It can be left for later.

I'll resend this patch, with this thing refactored too, and with the
localtime fix.

>> +     foreach my $who (@people) {
>> +             my %ad = parse_date($co->{$who . '_epoch'}, $co->{$who . '_tz'});
>
> Wouldn't
>
>  $co->{"${who}_epoch"}
>
> be simpler than
>
>  $co->{$who . '_epoch'}
>
> Also it is now %wd (%who_date) or just %date rather than
> %ad (%author_date) vs %cd (%committer_date).
>
>
> Both of those issues are very, very minor.

But I can tune them out 8-)

> Nice... although this set of diff chunks made me wonder why we
> use "10" for 'shortlog', "15, 3" for 'history, and "15, 5" for
> 'search' (grep).

I don't know about that. It might be worth considering unifying them.

> Thanks again for your diligent work on this patch series!

Thank you very much for your comments. I could clearly see the code
improving significantly at each iteration. It's a great sastisfaction,
even though it's quite some work.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv7 1/9] gitweb: refactor author name insertion

[Giuseppe Bilotta]

On Sat, Jun 27, 2009 at 5:26 PM, Giuseppe
Bilotta<giuseppe.bilotta@gmail.com> wrote:
> 2009/6/27 Jakub Narebski <jnareb@gmail.com>:
>> By the way, this "atnight" warning is duplicated in subroutine below
>> git_print_authorship_rows(), but IMHO it is not something very important.
>> It can be left for later.
>
> I'll resend this patch, with this thing refactored too, and with the
> localtime fix.

Eh, the refactoring caused a chain of changes in some of the following
patches, so I'll wait a bit before resending. In the mean time, the
new patchset can be found here:
http://git.oblomov.eu/git/shortlog/next..refs/tags/gitweb/avatar7bis

[BTW, for some reason
http://git.oblomov.eu/git/shortlog/next..gitweb/avatar7bis gives a 404
instead]

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv7 2/9] gitweb: uniform author info for commit and commitdiff

[Jakub Narebski]

On Sat, 27 June 2009, Giuseppe Bilotta wrote:
> Switch from
> 
> A U Thor <email@example.com> [date time]
> 
> to
> 
> author	A U Thor <email@example.com>
> 	date time
> committer	C O Mitter <other.email@example.com>
> 	committer date time

I would use:

  Switch from form similar to the one used by 'log' view

  	A U Thor <email@example.com> [date time]

  to the form used in 'commit' view

	author       A U Thor <email@example.com>
  	 	     date time
  	committer    C O Mitter <other.email@example.com>
  	             date time

(i.e. use spaces and not tabs to align).  But this is minor
issue, not worth worrying about IMVHO.

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

I am still not sure about this change.  On one hand side it is unifying
of 'commit' and 'commitdiff' view; most other web interfaces have single
view equivalent to git-show, which displays both commit info, and the
diff.  (And 'commitdiff' with 'hp' parameter i.e. between two commits
has to be redesigned anyway, so this issue doesn't enter this 
consideration).

On the other hand side IIRC 'commitdiff' uses short (one-line) 
authorship info because the main point is the diff, and multi-line
author and commit info like the one used in 'commit' view takes
a bit of vertical space.  Also one can use similarity between
'log' and 'commitdiff' views (git-log and git-show) as a counter
for argument that 'commitdiff' has to look like 'commit'.

But otherwise I quite like this patch.

> ---
>  gitweb/gitweb.perl |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 9be723c..0d8005d 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -5599,7 +5599,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_authorship_rows(\%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 earlier (re)factoring.

BTW. after this change the -localtime part of git_print_authorship()
subroutine is unused... just saying ;-)  Not something terribly 
important.

-- 
Jakub Narebski
Poland

Re: [PATCHv7 3/9] gitweb: use git_print_authorship_rows in 'tag' view too

[Jakub Narebski]

On Sat, 27 June 2009, Giuseppe Bilotta wrote:

> parse_tag has to be adapted to use the hash keys expected by
> git_print_authorship_rows, which is not a problem since git_tag
> is the only user of this sub.

Nitpick: I think that with s/has/had/ (past tense) and 
s/, which/. This/ (split paragraph into two sentences)
it would read better.

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

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

> ---
>  gitweb/gitweb.perl |   16 +++++++++-------
>  1 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 0d8005d..7183ad2 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2409,8 +2409,14 @@ sub parse_tag {
>  			$tag{'name'} = $1;
>  		} elsif ($line =~ m/^tagger (.*) ([0-9]+) (.*)$/) {
>  			$tag{'author'} = $1;
> -			$tag{'epoch'} = $2;
> -			$tag{'tz'} = $3;
> +			$tag{'author_epoch'} = $2;
> +			$tag{'author_tz'} = $3;
> +			if ($tag{'author'} =~ m/^([^<]+) <([^>]*)>/) {
> +				$tag{'author_name'}  = $1;
> +				$tag{'author_email'} = $2;
> +			} else {
> +				$tag{'author_name'} = $tag{'author'};
> +			}
>  		} elsif ($line =~ m/--BEGIN/) {
>  			push @comment, $line;
>  			last;

Sidenote: I wonder if it would be worth doing to factor out code dealing
with extracting data from (parsing) author/committer/tagger object headers.
Probably not worth it...

> @@ -4623,11 +4629,7 @@ sub git_tag {
>  	                                      $tag{'type'}) . "</td>\n" .
>  	      "</tr>\n";
>  	if (defined($tag{'author'})) {
> -		my %ad = parse_date($tag{'epoch'}, $tag{'tz'});
> -		print "<tr><td>author</td><td>" . esc_html($tag{'author'}) . "</td></tr>\n";
> -		print "<tr><td></td><td>" . $ad{'rfc2822'} .
> -			sprintf(" (%02d:%02d %s)", $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'}) .
> -			"</td></tr>\n";
> +		git_print_authorship_rows(\%tag, 'author');
>  	}
>  	print "</table>\n\n" .
>  	      "</div>\n";

I wonder why we used and use 'author' instead of 'tagger'...

-- 
Jakub Narebski
Poland

Re: [PATCHv7 4/9] gitweb: right-align date cell in shortlog

[Jakub Narebski]

On Sat, 27 June 2009, Giuseppe Bilotta wrote:

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

I don't like it.  In my opinion it is less readable that way, especially
if word wrapping gets involved:

  5 hours |
      ago |

  5 hours |
      ago |

   5 days |
      ago |

It is IMVHO a bit ugly.


That aside this is *not complete*.  Take a look at 'summary' view.  
You have there 'date' column in 'shortlog' section aligned to the right,
while in 'heads' and 'tags' section it is aligned to the left.  Add to
that "Last Change" column in projects list view (which should probably
be aligned to the left, even with this patch completed).

NAK from me.  (Alternate stylesheet?  Just kidding...)

> ---
>  gitweb/gitweb.css |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
> index 68b22ff..ef24a1b 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;
>  }

On the other hand: it is short and simple.

-- 
Jakub Narebski
Poland

Re: [PATCHv7 2/9] gitweb: uniform author info for commit and commitdiff

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> I would use:
>
>   Switch from form similar to the one used by 'log' view
>
>   	A U Thor <email@example.com> [date time]
>
>   to the form used in 'commit' view
>
> 	author       A U Thor <email@example.com>
>   	 	     date time
>   	committer    C O Mitter <other.email@example.com>
>   	             date time
>
> (i.e. use spaces and not tabs to align).  But this is minor
> issue, not worth worrying about IMVHO.

These extra blank lines make things much easier to read, and explicit
mention of switching from WHAT to WHAT ELSE is very much appreciated.

> On the other hand side IIRC 'commitdiff' uses short (one-line) 
> authorship info because the main point is the diff, and multi-line
> author and commit info like the one used in 'commit' view takes
> a bit of vertical space.

In general, we might want to make the committer information less prominent
than it currently is.

When looking at a repository that is used like CVS, author and committer
are always the same.  When looking at a repository that is owned by a
single integrator, the committer is a single person.  The only time
committer information for every commit would help is when viewing a
repository of higher level integrator in a project that has subintegrators
(e.g. Linus pulls from David who commits patches from others).

So at some point, it may not be a bad idea to introduce a per-repository
option/feature to hide committer information from certain views to
allocate more space for other information.

But certainly that shouldn't be a part of this topic.

Re: [PATCHv7 5/9] gitweb: (gr)avatar support

[Jakub Narebski]

On Sat, 27 June 2009, Giuseppe Bilotta wrote:

> Introduce avatar support: the feature adds the appropriate img tag next
> to author and committer in commit(diff), history, shortlog, log and tag
> views. Multiple avatar providers are possible, but only gravatar is
> implemented at the moment.
> 
> 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.
> 
> No avatar provider is selected by default, except in the t9500 test.

This is well written commit message.  Good work!

> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.css                      |    4 ++
>  gitweb/gitweb.perl                     |   81 ++++++++++++++++++++++++++++++-
>  t/t9500-gitweb-standalone-no-errors.sh |    2 +

Please, now that you added this to t9500 to be able to test gravatar
and avatar code, run t9500-gitweb-standalone-no-errors.sh script...

> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 7183ad2..ad9ae31 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.  Good and complete explanation.

[...]
> +	# 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' => {
> +		'sub' => \&feature_avatar,
> +		'override' => 0,
> +		'default' => ['']},
>  );

So you have chosen [''] and not [] as "no avatar" value, to simplify
later code.  All right.

>  
>  sub gitweb_get_feature {
> @@ -433,6 +458,16 @@ sub feature_patches {
>  	return ($_[0]);
>  }
>  
> +sub feature_avatar {
> +	my @val = (git_get_project_config('avatar'));
> +
> +	if (@val) {
> +		return @val;
> +	}
> +
> +	return @_;
> +}

Hmmm... why not simply

  +	return @val ? @val : @_;


By the way, we might want to accept 'none' instead of empty value
or no value to turn off avatar support for specific project (if
avatars are turned on globally, and project specific override is on).
We use this technique for 'snapshot' feature.

But this isn't terribly important.

[...]
> @@ -814,6 +849,17 @@ $git_dir = "$projectroot/$project" if $project;
>  our @snapshot_fmts = gitweb_get_feature('snapshot');
>  @snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);
>  
> +# check that the avatar feature is set to a known provider name,
> +# and for each provider check if the dependencies are satisfied.
> +# if the provider name is invalid or the dependencies are not met,
> +# reset $git_avatar to the empty string.
> +our ($git_avatar) = gitweb_get_feature('avatar');
> +if ($git_avatar eq 'gravatar') {
> +	$git_avatar = '' unless (eval { require Digest::MD5; 1; });
> +} else {
> +	$git_avatar = '';
> +}

Thoughts for the future: this can lead to not very pretty if-elsif
chain.  We have replaces such chain for selecting action (for dispatch)
by using %actions hash of subroutine refs (as a kind of 'switch'/'given'
statement).  We could do the same for avatar provider initialization
and validation subroutines.

But for now it is clear enough.  Don't worry about this issue now.

> @@ -1469,6 +1515,29 @@ sub format_subject_html {
>  	}
>  }
>  
> +# Insert an avatar for the given $email at the given $size if the feature
> +# is enabled.
> +sub git_get_avatar {
> +	my ($email, %opts) = @_;
> +	my $pre_white  = ($opts{'pad_before'} ? "&nbsp;" : "");
> +	my $post_white = ($opts{'pad_after'}  ? "&nbsp;" : "");
> +	my $size = $avatar_size{$opts{'size'}} || $avatar_size{'default'};

Running t9500-gitweb-standalone-no-errors.sh with --debug option shows
failing of 15 among 87 tests, with error:

   gitweb.perl: Use of uninitialized value in hash element at gitweb/gitweb.perl line 1524.

which is the above line.  This line should read:

  +	my $size = exists $opts{'size'}
  +		? $avatar_size{$opts{'size'}} || $avatar_size{'default'}
  +		: $avatar_size{'default'};

or something like that, e.g.:

  +	my $size = $avatar_size{ defined $opts{'size'} ? $opts{'size'} : 'default' }
  +		|| $avatar_size{'default'};


BTW. didn't we agree on '-size' and '-pad_before' convention? 

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

Why not use the new API[1][2]?:

  +		$url = "http://www.gravatar.com/avatar/" .
  +			Digest::MD5::md5_hex(lc $email) . "?size=$size";

[1] http://www.gravatar.com/site/implement/url
[2] http://search.cpan.org/perldoc/Gravatar::URL
    http://p3rl.org/Gravatar::URL

> +	}
> +	# 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 width=\"$size\" class=\"avatar\" src=\"$url\" />" . $post_white;
> +	} else {
> +		return "";
> +	}
> +}

Good idea, and nice description.

[...]
> @@ -1476,7 +1545,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>";
> +	return "<$tag class=\"author\">" .
> +	       git_get_avatar($co->{'author_email'}, 'pad_after' => 1) .
> +	       $author . "</$tag>";
>  }
>  
>  # format git diff header line, i.e. "diff --(git|combined|cc) ..."
> @@ -3249,7 +3320,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 containing the full author or committer information,
> @@ -3264,7 +3336,10 @@ sub git_print_authorship_rows {
>  	@people = ('author', 'committer') unless @people;
>  	foreach my $who (@people) {
>  		my %ad = parse_date($co->{$who . '_epoch'}, $co->{$who . '_tz'});
> -		print "<tr><td>$who</td><td>" . esc_html($co->{$who}) . "</td></tr>\n".
> +		print "<tr><td>$who</td><td>" . esc_html($co->{$who}) . "</td>" .
> +		      "<td rowspan=\"2\">" .
> +		      git_get_avatar($co->{$who . '_email'}, 'size' => 'double') .

Nitpick: you can use  "${who}_email", but I am not sure if it is more
readable than  $who . '_email' form.

> +		      "</td></tr>\n" .
>  		      "<tr>" .
>  		      "<td></td><td> $ad{'rfc2822'}";
>  		if ($ad{'hour_local'} < 6) {

Short and nice, thanks to refactoring done in earlier patches in this
series.  Very good.

> diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh
> index d539619..6275181 100755
> --- a/t/t9500-gitweb-standalone-no-errors.sh
> +++ b/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 \
> @@ -671,6 +672,7 @@ test_expect_success \
>  	'config override: tree view, features disabled in repo config' \
>  	'git config gitweb.blame no &&
>  	 git config gitweb.snapshot none &&
> +	 git config gitweb.avatar gravatar &&
>  	 gitweb_run "p=.git;a=tree"'
>  test_debug 'cat gitweb.log'
>  

Please run it too :-)


Apart from this issue of Perl warning (which would get logged, but 
otherwise is not an error), and using old Gravatar API, this patch
shapes very nicely, thanks to all the work done earlier on refactoring.
I like it very much.

-- 
Jakub Narebski
Poland

Re: [PATCHv7 6/9] gitweb: gravatar url cache

[Jakub Narebski]

On Sat, 27 June 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.
> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

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

> ---

Here there are some very simple benchmarks of the effect of gravatar 
URL cache.  I'm not sure if they should be put in commit message.

When running gitweb as a script
   $ time gitweb-run.sh "p=git.git;a=shortlog" >/dev/null
I got the following results

             | before          | after 
 ------------+-----------------|------------------
 summary     | 0.978s (0.964s) | 0.961s (0.940s)
 shortlog    | 1.001s (0.980s) | 1.033s (1.008s)

which means the same results within the margin of error, (as seen when
repeating benchmark the differences are within the range of fluctuations).
Time is real (wallclock) time, in parentheses there is user + sys time.


When using ApacheBench 2.0.41-dev, with gitweb run as CGI script
(not as legacy, i.e. ModPerl::Registry, mod_perl script)
  $ ab <opt> "http://localhost/cgi-bin/gitweb/gitweb.cgi/git.git/shortlog"
I got the folowing results

             | before          | after 
 ------------+-----------------|------------------
 -n 10       | 1094.050 [ms]   |  989.136 [ms]
 -n 10 -c 2  | 1147.965 [ms]   | 1041.324 [ms]

which means around 10% improvement.  Standard deviation is around 10 ms
for no concurrency, and around 200 ms (more than third of difference)
for concurrent connections.  Shown here is time per request (mean, 
across all concurrent requests).

>  gitweb/gitweb.perl |   30 +++++++++++++++++++++++++++---
>  1 files changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index ad9ae31..de4cc63 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1515,6 +1515,27 @@ sub format_subject_html {
>  	}
>  }
>  
> +# 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 = ();

Good comment.

> +
> +# 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=";

The same comment as for previous patch: why not use modern API?

  +		"http://www.gravatar.com/avatar/" .
  +			Digest::MD5::md5_hex($email) . "?size=";


> +	return $avatar_cache{$email} . $size;
> +}

Nice.

> +
>  # Insert an avatar for the given $email at the given $size if the feature
>  # is enabled.
>  sub git_get_avatar {
> @@ -1524,15 +1545,18 @@ sub git_get_avatar {
>  	my $size = $avatar_size{$opts{'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);
>  	}

Great encapsulation.

>  	# 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 width=\"$size\" class=\"avatar\" src=\"$url\" />" . $post_white;
> +		return $pre_white .
> +		       "<img width=\"$size\" " .
> +		            "class=\"avatar\" " .
> +		            "src=\"$url\" " .
> +		       "/>" . $post_white;

This is independent change.  It shouldn't be here; if you prefer such 
solution, you should squash it with previous patch.  Please drop this
chunk.

>  	} else {
>  		return "";
>  	}
> -- 

Thanks again for great work on this series!

-- 
Jakub Narebski
Poland

Re: [PATCHv7 3/9] gitweb: use git_print_authorship_rows in 'tag' view too

[Giuseppe Bilotta]

2009/6/27 Jakub Narebski <jnareb@gmail.com>:
> On Sat, 27 June 2009, Giuseppe Bilotta wrote:
>
>> parse_tag has to be adapted to use the hash keys expected by
>> git_print_authorship_rows, which is not a problem since git_tag
>> is the only user of this sub.
>
> Nitpick: I think that with s/has/had/ (past tense) and
> s/, which/. This/ (split paragraph into two sentences)
> it would read better.

I can do that.

> Sidenote: I wonder if it would be worth doing to factor out code dealing
> with extracting data from (parsing) author/committer/tagger object headers.
> Probably not worth it...

It might be worth it, but I'd say we can leave it to some other time ;-)

> I wonder why we used and use 'author' instead of 'tagger'...

The upside of using 'author' is that the tag can then be dealt with
just like any other commit by the rest of the code, I think. I don't
know if this benefit is actually used anywhere in the code.


-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv7 4/9] gitweb: right-align date cell in shortlog

[Giuseppe Bilotta]

2009/6/27 Jakub Narebski <jnareb@gmail.com>:
> On Sat, 27 June 2009, Giuseppe Bilotta wrote:
>
>> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
>
> I don't like it.  In my opinion it is less readable that way, especially
> if word wrapping gets involved:
>
>  5 hours |
>      ago |
>
>  5 hours |
>      ago |
>
>   5 days |
>      ago |
>
> It is IMVHO a bit ugly.

You might have a point there.

> That aside this is *not complete*.  Take a look at 'summary' view.
> You have there 'date' column in 'shortlog' section aligned to the right,
> while in 'heads' and 'tags' section it is aligned to the left.  Add to
> that "Last Change" column in projects list view (which should probably
> be aligned to the left, even with this patch completed).
>
> NAK from me.  (Alternate stylesheet?  Just kidding...)

[...]
>
> On the other hand: it is short and simple.

And totally independent from the rest of the patchset. I'll move it to
the top of the stack, so that the rest of the next iteration can be
applied in batch as soon as it's ready.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv7 2/9] gitweb: uniform author info for commit and commitdiff

[Giuseppe Bilotta]

On Sat, Jun 27, 2009 at 8:38 PM, Junio C Hamano<gitster@pobox.com> wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>
>> I would use:
>>
>>   Switch from form similar to the one used by 'log' view
>>
>>       A U Thor <email@example.com> [date time]
>>
>>   to the form used in 'commit' view
>>
>>       author       A U Thor <email@example.com>
>>                    date time
>>       committer    C O Mitter <other.email@example.com>
>>                    date time
>>
>> (i.e. use spaces and not tabs to align).  But this is minor
>> issue, not worth worrying about IMVHO.
>
> These extra blank lines make things much easier to read, and explicit
> mention of switching from WHAT to WHAT ELSE is very much appreciated.

I updated the patch accordingly.

>> On the other hand side IIRC 'commitdiff' uses short (one-line)
>> authorship info because the main point is the diff, and multi-line
>> author and commit info like the one used in 'commit' view takes
>> a bit of vertical space.
>
> In general, we might want to make the committer information less prominent
> than it currently is.

I've been thinking about this. A possible approach I considered could
be to only show the commit _date_ by default (which could be
interesting to see), and use some DHTML to show the committer on
request. But as you mention, this is not the patchset to do this kind
of work on 8-)

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv7 5/9] gitweb: (gr)avatar support

[Giuseppe Bilotta]

2009/6/27 Jakub Narebski <jnareb@gmail.com>:
>
> This is well written commit message.  Good work!

Thanks 8-)

>> +     # 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' => {
>> +             'sub' => \&feature_avatar,
>> +             'override' => 0,
>> +             'default' => ['']},
>>  );
>
> So you have chosen [''] and not [] as "no avatar" value, to simplify
> later code.  All right.
>
>>
>>  sub gitweb_get_feature {
>> @@ -433,6 +458,16 @@ sub feature_patches {
>>       return ($_[0]);
>>  }
>>
>> +sub feature_avatar {
>> +     my @val = (git_get_project_config('avatar'));
>> +
>> +     if (@val) {
>> +             return @val;
>> +     }
>> +
>> +     return @_;
>> +}
>
> Hmmm... why not simply
>
>  +     return @val ? @val : @_;

Right. Why not?

> By the way, we might want to accept 'none' instead of empty value
> or no value to turn off avatar support for specific project (if
> avatars are turned on globally, and project specific override is on).
> We use this technique for 'snapshot' feature.
>
> But this isn't terribly important.

Well, since 'none' is not a known provider, that trick should work
correctly anyway.

>> +our ($git_avatar) = gitweb_get_feature('avatar');
>> +if ($git_avatar eq 'gravatar') {
>> +     $git_avatar = '' unless (eval { require Digest::MD5; 1; });
>> +} else {
>> +     $git_avatar = '';
>> +}
>
> Thoughts for the future: this can lead to not very pretty if-elsif
> chain.  We have replaces such chain for selecting action (for dispatch)
> by using %actions hash of subroutine refs (as a kind of 'switch'/'given'
> statement).  We could do the same for avatar provider initialization
> and validation subroutines.
>
> But for now it is clear enough.  Don't worry about this issue now.

I'm also not terribly sure about how to implement _this_ through hash
calls. But I did consider the issue (how I wish Perl had an actual
switch statement ...)

> Running t9500-gitweb-standalone-no-errors.sh with --debug option shows
> failing of 15 among 87 tests, with error:
>
>   gitweb.perl: Use of uninitialized value in hash element at gitweb/gitweb.perl line 1524.
>
> which is the above line.  This line should read:
>
>  +     my $size = exists $opts{'size'}
>  +             ? $avatar_size{$opts{'size'}} || $avatar_size{'default'}
>  +             : $avatar_size{'default'};
>
> or something like that, e.g.:
>
>  +     my $size = $avatar_size{ defined $opts{'size'} ? $opts{'size'} : 'default' }
>  +             || $avatar_size{'default'};

I did $opts{-size} ||= 'default'. Or is it bad form to modify the
passed option hash?

> BTW. didn't we agree on '-size' and '-pad_before' convention?

Yes we did. I don't know why it went back to 'size' etc. Must have
messed up something with the patchsets.

>> +     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";
>
> Why not use the new API[1][2]?:

Updated.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv7 5/9] gitweb: (gr)avatar support

[Jakub Narebski]

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

>>> +     # 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.
[...]

>> By the way, we might want to accept 'none' instead of empty value
>> or no value to turn off avatar support for specific project (if
>> avatars are turned on globally, and project specific override is on).
>> We use this technique for 'snapshot' feature.
>>
>> But this isn't terribly important.
> 
> Well, since 'none' is not a known provider, that trick should work
> correctly anyway.

Ah, I have forgot about this.  Perhaps it should be stated more 
explicitly that unknown provider (type) = no avatar image?  Or do I
simply not see it?

>>> +our ($git_avatar) = gitweb_get_feature('avatar');
>>> +if ($git_avatar eq 'gravatar') {
>>> +     $git_avatar = '' unless (eval { require Digest::MD5; 1; });
>>> +} else {
>>> +     $git_avatar = '';
>>> +}
>>
>> Thoughts for the future: this can lead to not very pretty if-elsif
>> chain.  We have replaces such chain for selecting action (for dispatch)
>> by using %actions hash of subroutine refs (as a kind of 'switch'/'given'
>> statement).  We could do the same for avatar provider initialization
>> and validation subroutines.
>>
>> But for now it is clear enough.  Don't worry about this issue now.
> 
> I'm also not terribly sure about how to implement _this_ through hash
> calls. But I did consider the issue (how I wish Perl had an actual
> switch statement ...)

Perl 5.10, Perl 6, Switch.pm... but it isn't something that we can use
in gitweb.  Switch statement (spelled 'given / when') in Perl6 / Perl 5.10
is very, very powerfull (from what I have read).

But what I meant here is something like the following:

  our %avatar_providers = (
  	'gravatar' => sub { eval { require Digest::MD5; 1; } },
  	'picon'    => sub { 1; },
  );
  unless (defined $avatar_providers{$git_avatar} &&
          $avatar_providers{$git_avatar}->()) {
  	$git_avatar = '';
  }

Or something like that (we can use 'undef' for providers without need
for initialization).  But that can be improved later, "in tree".


On the other hand side this solution wouldn't I think be able to deal
with specifying fallbacks using e.g. 'gravatar+picon' (gravatar using
picon URL as fallback i.e. "default" argument), or two images side by
side like GMane uses (if there are two), e.g. 'gravatar picon' or 
'gravatar, picon'.

Food for thought.  Not something to act on now.

>> Running t9500-gitweb-standalone-no-errors.sh with --debug option shows
>> failing of 15 among 87 tests, with error:
>>
>>   gitweb.perl: Use of uninitialized value in hash element at gitweb/gitweb.perl line 1524.
>>
>> which is the above line.  This line should read:
>>
>>  +     my $size = exists $opts{'size'}
>>  +             ? $avatar_size{$opts{'size'}} || $avatar_size{'default'}
>>  +             : $avatar_size{'default'};
>>
>> or something like that, e.g.:
>>
>>  +     my $size = $avatar_size{ defined $opts{'size'} ? $opts{'size'} : 'default' }
>>  +             || $avatar_size{'default'};
> 
> I did $opts{-size} ||= 'default'. Or is it bad form to modify the
> passed option hash?

No, it is O.K.  We do similar thing in href(), deleting keys from hash.

>>> +     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";
>>
>> Why not use the new API[1][2]?:
> 
> Updated.

This affects also the next patch in series.

-- 
Jakub Narebski
Poland

Re: [PATCHv7 7/9] gitweb: picon avatar provider

[Jakub Narebski]

On Sat, 27 June 2009, Giuseppe Bilotta wrote:

> Simple implementation of picon that only relies on the indiana.edu
> database.

I'd like to see where you got information about picons.  But it is
not necessary to have in commit message.

... Ah, I'm sorry, it is stated in comment.  Nevermind then.

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

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

> ---
>  gitweb/gitweb.perl |   26 ++++++++++++++++++++++----
>  1 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index de4cc63..ae73d45 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -378,14 +378,17 @@ our %feature = (
>  	# 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.
> +	# Currently available providers are gravatar and picon.
> +
> +	# Gravatar depends on Digest::MD5.
> +	# Picon currently relies on the indiana.edu database.
>  
>  	# To enable system wide have in $GITWEB_CONFIG
> -	# $feature{'avatar'}{'default'} = ['gravatar'];
> +	# $feature{'avatar'}{'default'} = ['provider'];
> +	# where provider is either gravatar or picon.

I wonder if it wouldn't be better to use "<provider>" (or "'<provider>'")
in place of "'provider'", as it is not about literal 'provider', but 
about one of 'gravatar' or 'picon'.

>  	# To have project specific config enable override in $GITWEB_CONFIG
>  	# $feature{'avatar'}{'override'} = 1;
> -	# and in project config gitweb.avatar = gravatar;
> +	# and in project config gitweb.avatar = provider;

Same as above.

>  	'avatar' => {
>  		'sub' => \&feature_avatar,
>  		'override' => 0,
> @@ -856,6 +859,8 @@ our @snapshot_fmts = gitweb_get_feature('snapshot');
>  our ($git_avatar) = gitweb_get_feature('avatar');
>  if ($git_avatar eq 'gravatar') {
>  	$git_avatar = '' unless (eval { require Digest::MD5; 1; });
> +} elsif ($git_avatar eq 'picon') {
> +	# no dependencies
>  } else {
>  	$git_avatar = '';
>  }

Nice.

> @@ -1523,6 +1528,17 @@ sub format_subject_html {
>  # given page, there's no risk for cache conflicts.
>  our %avatar_cache = ();
>  
> +# Compute the picon url for a given email, by using the picon search service over at
> +# http://www.cs.indiana.edu/picons/search.html
> +sub picon_url {
> +	my $email = lc shift;
> +	if (!$avatar_cache{$email}) {
> +		my ($user, $domain) = split('@', $email);
> +		$avatar_cache{$email} = "http://www.cs.indiana.edu/cgi-pub/kinzler/piconsearch.cgi/$domain/$user/users+domains+unknown/up/single";

Hmmm... perhaps it would be better to break this very long URL (link)
in lines...

> +	}
> +	return $avatar_cache{$email};
> +}

I wonder if it is worth caching picons, at least in this form.  It isn't
as if splitting on '@' is expensive.  But perhaps to not break pattern
(that avatar URLs are cached) it is a good thing.

Thoughts for the possible future enhancements: find final URL of an image
via http://www.cs.indiana.edu/cgi-pub/kinzler/piconsearch.cgi/$domain/$user/users+domains+unknown/up/off/1/order
by scrapping (parsing) it for .gif link, and store this URL in cache.
But that most probably isn't worth it.  Just feel like mentioning it.

> +
>  # 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
> @@ -1546,6 +1562,8 @@ sub git_get_avatar {
>  	my $url = "";
>  	if ($git_avatar eq 'gravatar') {
>  		$url = gravatar_url($email, $size);
> +	} elsif ($git_avatar eq 'picon') {
> +		$url = picon_url($email);
>  	}
>  	# 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 pattern, nice use of infrastructure.

I think that adding 'width' attribute to img tag of avatar should
perhaps be put here rather than in previous patch, which is about
adding gravatar URL cache.  This chunk:

         # 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 width=\"$size\" class=\"avatar\" src=\"$url\" />" . $post_white;
 +               return $pre_white .
 +                      "<img width=\"$size\" " .
 +                           "class=\"avatar\" " .
 +                           "src=\"$url\" " .
 +                      "/>" . $post_white;
         } else {
                 return "";
         }

should be, I think, in this patch and not in previous one (or at least
in some other patch than the previous one).

BTW. you should have updated the comment here.


Should it be stated that <img width="$size" ...> is here because not 
all kinds of avatars (not all avatar providers) support selecting size
of avatar, somewhere in this comment?


Very nice, well thought infrastructure, which makes adding new avatar
providers easy.  Good work!

-- 
Jakub Narebski
Poland

Re: [PATCHv7 8/9] gitweb: use picon for gravatar fallback

[Jakub Narebski]

On Sat, 27 June 2009, Giuseppe Bilotta wrote:

I think this issue is complicated enough to do it well and configurable
that it is out of scope of current patch series, and it would be better
to be left for next series.

.......................................................................

There are few issues connected with multiple avatar providers:

1. Avatar provider side fallback, i.e. what to do if given person
(usually given email address) is not registered with used avatar 
provider. 

Gravatar provider by default falls back to [G] gravatar default image,
and you can specify fallback using 'd'/'default' parameter, either to
one of defined providers: special values "identicon", "monsterid" and
"wavatar", or to specified image using URI-encoded URL as value of 
'default' parameter.  From the point of view of gitweb, you either
pass provider or URL literally (encoding), or you resolve fallback
provider avatar and pass encoded URI.

Picon provider has built-in many level fallback; current gitweb 
implementation searches in 'users' database, then in 'domains'
database (there e.g. gmail.com domain addresses receive GMail
avatar), and last in 'unknown' database which would always provide
some default avatar for "person".  But in general one can use 
cs.indiana.edu database as fallback, and try personal / local site
database first.

2. Gitweb side fallback; for some kinds of avatars one can think about,
for example 'local' avatars (example URL: "avatar/user.gif"), or 'inrepo'
avatar provider (example URL: "?p=$project;a=blob_plain;f=avatar/user.gif"),
one can easily detect in gitweb if avatar for given email (given person)
exists or not (is available or not).

3. Multiple avatars.  We can use here something like what Gmane does,
which shows gravatar if it is available (and nothing if it is not), 
picon if it is available (and nothing if it is not), and both picon
and gravatar if both are available.  See example in:
  http://permalink.gmane.org/gmane.comp.version-control.git/122394
  http://permalink.gmane.org/gmane.comp.version-control.git/118058


This patch tries to address issue 1.) for gravatar provider (which allow
for specifying default avatar via its URL API).  I think however that
this issue should be configurable; unconditional using picon provider
with very long URI (making gravatar-with-picon-fallback provider URI
very, very long) is not a good idea.

By configurable I mean that user should be able to specify e.g. gravatar
with wavatar fallback, gravatar with static image fallback, and gravatar
with arbitrary provider (resolved) fallback.

Unless you wanted to provide this patch as an example of having multiple
providers / providing avatar fallback (default value).  But then it would
be better to have it marked as RFC explicitly...

> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index ae73d45..e2638cb 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1546,9 +1546,13 @@ sub picon_url {
>  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) . "&size=";
> +	if (!$avatar_cache{$email}) {
> +		my $picon = CGI::escape(picon_url($email));

Sidenote: if we end requiring 'escape', we can simply import it.

> +		$avatar_cache{$email} = "http://www.gravatar.com/avatar.php?gravatar_id=" .
> +			Digest::MD5::md5_hex($email) .
> +			"&default=$picon" .
> +			"&size=";
> +	}
>  	return $avatar_cache{$email} . $size;
>  }

Nevertheless this is nice example that infrastructure created in previous
patches leads to adding such feature quite simple.

-- 
Jakub Narebski
Poland

Re: [PATCHv7 9/9] gitweb: add alt text to avatar img

[Jakub Narebski]

On Sat, 27 June 2009, Giuseppe Bilotta wrote:

> The alt text, set to the avatar provider (i.e. image type), is more
> friendly than the img url in some text-only browsers.

Signoff?

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

> ---
>  gitweb/gitweb.perl |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index e2638cb..dc6049a 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1578,6 +1578,7 @@ sub git_get_avatar {
>  		       "<img width=\"$size\" " .
>  		            "class=\"avatar\" " .
>  		            "src=\"$url\" " .
> +			    "alt=\"$git_avatar\" " .
>  		       "/>" . $post_white;
>  	} else {
>  		return "";
> -- 

Bit of bikeshedding: "alt=\"\" " (empty alt text) would also be
a good solution.  This way text browser which do not display images
wouldn't take limited horizontal space for alt text of extraneous
avatar image.

-- 
Jakub Narebski
Poland

Re: [PATCHv7 7/9] gitweb: picon avatar provider

[Giuseppe Bilotta]

2009/6/28 Jakub Narebski <jnareb@gmail.com>:
> Thoughts for the possible future enhancements: find final URL of an image
> via http://www.cs.indiana.edu/cgi-pub/kinzler/piconsearch.cgi/$domain/$user/users+domains+unknown/up/off/1/order
> by scrapping (parsing) it for .gif link, and store this URL in cache.
> But that most probably isn't worth it.  Just feel like mentioning it.

That was my first thought, but since piconsearch offers a link
directly, I decided it was much better to use that rather than html
scraping.

> Should it be stated that <img width="$size" ...> is here because not
> all kinds of avatars (not all avatar providers) support selecting size
> of avatar, somewhere in this comment?

It is generally good form to have the size specified in advance in
HTML anyway, which is why on later iterations I put the width spec in
the first avatar service commit.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv7 9/9] gitweb: add alt text to avatar img

[Giuseppe Bilotta]

2009/6/28 Jakub Narebski <jnareb@gmail.com>:
> Signoff?

Duh.

> Bit of bikeshedding: "alt=\"\" " (empty alt text) would also be
> a good solution.  This way text browser which do not display images
> wouldn't take limited horizontal space for alt text of extraneous
> avatar image.

Oh, interesting. Let me try that.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv7 0/9] gitweb: avatar support

[Jakub Narebski]

On Sat, 27 Jun 2009, Giuseppe Bilotta wrote:
> One more attempt at adding avatar support to gitweb.
> 
> The most significant change since the previous iteration is the addition
> of a picon provider, and its use as default from gravatar.
> 
> Most other patches were changed to follow Jakub's suggestions. In
> particular, the two-line author/committer information was refactored
> to reduce code duplication, with the benefit that it could also be used
> in 'tag' view.
> 
> Giuseppe Bilotta (9):
>   gitweb: refactor author name insertion
>   gitweb: uniform author info for commit and commitdiff
>   gitweb: use git_print_authorship_rows in 'tag' view too
>   gitweb: right-align date cell in shortlog
>   gitweb: (gr)avatar support
>   gitweb: gravatar url cache
>   gitweb: picon avatar provider
>   gitweb: use picon for gravatar fallback
>   gitweb: add alt text to avatar img

I think this patch series shapes very nicely.  A bit of refactoring 
upfront, so that the following patches are not very large, and don't
need to repeat the same code in many places.  Separate issues such 
as right-align date cell, or making 'commitdiff' view use authorship
info layout from 'commit' view are put in separate patches in such
way that they can be accepted or rejected individually.

Very good work!

-- 
Jakub Narebski
Poland

Re: [PATCHv7 0/9] gitweb: avatar support

[Giuseppe Bilotta]

2009/6/29 Jakub Narebski <jnareb@gmail.com>:
>> Giuseppe Bilotta (9):
>>   gitweb: refactor author name insertion
>>   gitweb: uniform author info for commit and commitdiff
>>   gitweb: use git_print_authorship_rows in 'tag' view too
>>   gitweb: right-align date cell in shortlog
>>   gitweb: (gr)avatar support
>>   gitweb: gravatar url cache
>>   gitweb: picon avatar provider
>>   gitweb: use picon for gravatar fallback
>>   gitweb: add alt text to avatar img
>
> I think this patch series shapes very nicely.  A bit of refactoring
> upfront, so that the following patches are not very large, and don't
> need to repeat the same code in many places.  Separate issues such
> as right-align date cell, or making 'commitdiff' view use authorship
> info layout from 'commit' view are put in separate patches in such
> way that they can be accepted or rejected individually.

Thanks. I'll send a cleaned up version of everything we agreed on.

-- 
Giuseppe "Oblomov" Bilotta