[PATCHv5 0/3] gitweb: gravatar support

[PATCHv5 0/3] gitweb: gravatar support

[Giuseppe Bilotta]

Fifth iteration of the implementation of gravatar support in gitweb.

Hopefully, this time I didn't forget any of the recommended suggestions.
It surely looks much more flexible and extensible to my eyes 8-). I also
found the time to implement the url cache for gravatar; I didn't benchmark
its effects, but it does feel faster when loading shortlog pages.

Giuseppe Bilotta (3):
  gitweb: refactor author name insertion
  gitweb: gravatar support
  gitweb: gravatar url cache

 gitweb/gitweb.css  |    9 +++-
 gitweb/gitweb.perl |  141 ++++++++++++++++++++++++++++++++++++++++------------
 2 files changed, 117 insertions(+), 33 deletions(-)

[PATCHv5 1/3] gitweb: refactor author name insertion

[Giuseppe Bilotta]

The refactoring allows easier customization of the output by means of
CSS and improves extensibility on the CGI side too.

Layout is preserved for all views except for 'commitdiff', which now
uses the same format as 'commit'.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.css  |    5 ++-
 gitweb/gitweb.perl |   79 +++++++++++++++++++++++++++++++---------------------
 2 files changed, 51 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..223648f 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1469,6 +1469,14 @@ sub format_subject_html {
 	}
 }
 
+# format the author name with the given tag and optionally shortening it
+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,11 +3222,13 @@ 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 $tag = shift || '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) {
@@ -3228,7 +3238,30 @@ sub git_print_authorship {
 		printf(" (%02d:%02d %s)",
 		       $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
 	}
-	print "]</div>\n";
+	print "]</$tag>\n";
+}
+
+# Outputs the author and commiter name and date in long form
+sub git_print_who {
+	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'});
+	} 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";
 }
 
 sub git_print_page_path {
@@ -4142,11 +4175,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 +4224,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 uses      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 +4379,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 +5122,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 +5143,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 +5209,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_who(\%co);
 	print "<tr><td>commit</td><td class=\"sha1\">$co{'id'}</td></tr>\n";
 	print "<tr>" .
 	      "<td>tree</td>" .
@@ -5579,7 +5590,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);
+		print "<div class=\"title_text\">\n" .
+		      "<table class=\"object_header\">\n";
+		git_print_who(\%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

[PATCHv5 2/3] gitweb: gravatar support

[Giuseppe Bilotta]

Introduce gravatar support by adding the appropriate img tag next to
author and committer in commit(diff), history, shortlog and log view.

The feature is disabled by default, and depends on Digest::MD5, which
is a core package since Perl 5.8. If Digest::MD5 cannot be found,
enabling this feature results in a no-op.

The avatar insertion code is made generic enough to allow other avatar
sources to be added easily.

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

diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index 68b22ff..ddf982b 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 223648f..6e807fe 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,21 @@ our %feature = (
 		'sub' => \&feature_patches,
 		'override' => 0,
 		'default' => [16]},
+
+	# Gravatar support. When this feature is enabled, views such as
+	# shortlog or commit will display the gravatar associated with
+	# the email of the committer(s) and/or author(s). Please note that
+	# the feature depends on Digest::MD5.
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'gravatar'}{'default'} = [1];
+	# To have project specific config enable override in $GITWEB_CONFIG
+	# $feature{'gravatar'}{'override'} = 1;
+	# and in project config gitweb.gravatar = 0|1;
+	'gravatar' => {
+		'sub' => sub { feature_bool('gravatar', @_) },
+		'override' => 0,
+		'default' => [0]},
 );
 
 sub gitweb_get_feature {
@@ -814,6 +837,10 @@ $git_dir = "$projectroot/$project" if $project;
 our @snapshot_fmts = gitweb_get_feature('snapshot');
 @snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);
 
+# check if gravatars are enabled and dependencies are satisfied
+our $git_gravatar_enabled = gitweb_check_feature('gravatar') &&
+	(eval { require Digest::MD5; 1; });
+
 # dispatch
 if (!defined $action) {
 	if (defined $hash) {
@@ -1474,7 +1501,7 @@ 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'}, 'space_after' => 1) . $author . "</$tag>\n";
 }
 
 # format git diff header line, i.e. "diff --(git|combined|cc) ..."
@@ -3222,6 +3249,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{'space_before'} ? "&nbsp;" : "");
+	my $post_white = ($params{'space_after'} ? "&nbsp;" : "");
+	my $size = $avatar_size{$params{'size'}} || $avatar_size{'default'};
+	my $url = "";
+	if ($git_gravatar_enabled) {
+		$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;
@@ -3238,7 +3288,8 @@ sub git_print_authorship {
 		printf(" (%02d:%02d %s)",
 		       $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
 	}
-	print "]</$tag>\n";
+	print "]" . git_get_avatar($co->{'author_email'}, 'space_before' => 1)
+		  . "</$tag>\n";
 }
 
 # Outputs the author and commiter name and date in long form
@@ -3246,9 +3297,9 @@ sub git_print_who {
 	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'}";
+	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) {
 		printf(" (<span class=\"atnight\">%02d:%02d</span> %s)",
 		       $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
@@ -3258,7 +3309,8 @@ sub git_print_who {
 	}
 	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

[PATCHv5 3/3] 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.
---
 gitweb/gitweb.perl |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 6e807fe..10ca0fe 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3249,6 +3249,17 @@ sub git_print_header_div {
 	      "\n</div>\n";
 }
 
+our %gravatar_url_cache = ();
+
+sub gravatar_url {
+	my $email = lc shift;
+	my $size = shift;
+	$gravatar_url_cache{$email} ||=
+		"http://www.gravatar.com/avatar.php?gravatar_id=" .
+			Digest::MD5::md5_hex($email) . "&amp;size=";
+	return $gravatar_url_cache{$email} . $size;
+}
+
 # Insert an avatar for the given $email at the given $size if the feature
 # is enabled.
 sub git_get_avatar {
@@ -3258,8 +3269,7 @@ sub git_get_avatar {
 	my $size = $avatar_size{$params{'size'}} || $avatar_size{'default'};
 	my $url = "";
 	if ($git_gravatar_enabled) {
-		$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

Re: [PATCHv5 3/3] gitweb: gravatar url cache

[Junio C Hamano]

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

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

Sign-off?

I think the cache is placed at the wrong level (it doesn't have to be a
GRavatar_url_cache, but can be a general avatar_url_cache).

IOW,

	our %avatar_url_cache = ();
        sub git_get_avatar {
        	...
                my $url;
                if (!exists $avatar_url_cache{$email}) {
                	if ($git_gravatar_enabled) {
                        	$url = ... gravatar stuff ...;
			} else if ($git_whatever_enabled) {
                        	$url = ... other stuff ...;
			}
                        $avatar_url_cache{$email} = $url;
		}
                $url = $avatar_url_cache{$email};
                if (defined $url) {
                	return ... prefix then $url then suffix ...;
                } else {
                	return "";
                }
	}

But the basic idea is sound, I think.

Re: [PATCHv5 3/3] gitweb: gravatar url cache

[Giuseppe Bilotta]

On Thu, Jun 25, 2009 at 12:02 AM, Junio C Hamano<gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> 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.
>> ---
>
> Sign-off?

Duh.

> I think the cache is placed at the wrong level (it doesn't have to be a
> GRavatar_url_cache, but can be a general avatar_url_cache).

I'm not sure about it. The URL depends on email and size (can you use
arrays as hash keys in Perl?) , and the email part might be the same
but the size part might differ across separate calls (in theory; in
practice avatars in a view are presently all the same size; but for
example if we were to autodetect email addresses in commit messages,
we might have both single- and double- sided avatars in the same
page). By hashing on email+size only we would lose the benefit of
cache when using the same avatar at separate sizes.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv5 3/3] gitweb: gravatar url cache

[Jacob Helwig]

On Wed, Jun 24, 2009 at 15:46, Giuseppe
Bilotta<giuseppe.bilotta@gmail.com> wrote:
> On Thu, Jun 25, 2009 at 12:02 AM, Junio C Hamano<gitster@pobox.com> wrote:
>
>> I think the cache is placed at the wrong level (it doesn't have to be a
>> GRavatar_url_cache, but can be a general avatar_url_cache).
>
> I'm not sure about it. The URL depends on email and size (can you use
> arrays as hash keys in Perl?) , and the email part might be the same
> but the size part might differ across separate calls (in theory; in
> practice avatars in a view are presently all the same size; but for
> example if we were to autodetect email addresses in commit messages,
> we might have both single- and double- sided avatars in the same
> page). By hashing on email+size only we would lose the benefit of
> cache when using the same avatar at separate sizes.
>

You could have a hash key of "$email_$size", or something similar to
fake an array hash key.

Re: [PATCHv5 3/3] gitweb: gravatar url cache

[Giuseppe Bilotta]

On Thu, Jun 25, 2009 at 1:06 AM, Jacob Helwig<jacob.helwig@gmail.com> wrote:
> On Wed, Jun 24, 2009 at 15:46, Giuseppe
> Bilotta<giuseppe.bilotta@gmail.com> wrote:
>> On Thu, Jun 25, 2009 at 12:02 AM, Junio C Hamano<gitster@pobox.com> wrote:
>>
>>> I think the cache is placed at the wrong level (it doesn't have to be a
>>> GRavatar_url_cache, but can be a general avatar_url_cache).
>>
>> I'm not sure about it. The URL depends on email and size (can you use
>> arrays as hash keys in Perl?) , and the email part might be the same
>> but the size part might differ across separate calls (in theory; in
>> practice avatars in a view are presently all the same size; but for
>> example if we were to autodetect email addresses in commit messages,
>> we might have both single- and double- sided avatars in the same
>> page). By hashing on email+size only we would lose the benefit of
>> cache when using the same avatar at separate sizes.
>
> You could have a hash key of "$email_$size", or something similar to
> fake an array hash key.

The point is not so much the form to give to the key but rather the
fact that hashing on both means the URL has to be recomputed when the
same email appears with both sizes. Considering that (at least for
gravatars) the computational intensive part comes from the MD5 of the
email, this means a waste of cycles.

By letting the cache be per-avatar, each avatar kind can choose to
hash on whatever it needs to. But I got an idea on how to improve on
this.

-- 
Giuseppe "Oblomov" Bilotta

[PATCHv5bis 3/3] 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 6e807fe..6771a9d 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3249,6 +3249,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 {
@@ -3258,8 +3279,7 @@ sub git_get_avatar {
 	my $size = $avatar_size{$params{'size'}} || $avatar_size{'default'};
 	my $url = "";
 	if ($git_gravatar_enabled) {
-		$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

Re: [PATCHv5 3/3] gitweb: gravatar url cache

[Junio C Hamano]

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

> On Thu, Jun 25, 2009 at 12:02 AM, Junio C Hamano<gitster@pobox.com> wrote:
>> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>>
>>> 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.
>>> ---
>>
>> Sign-off?
>
> Duh.
>
>> I think the cache is placed at the wrong level (it doesn't have to be a
>> GRavatar_url_cache, but can be a general avatar_url_cache).
>
> I'm not sure about it. The URL depends on email and size (can you use
> arrays as hash keys in Perl?) , and the email part might be the same
> but the size part might differ across separate calls (in theory; in
> practice avatars in a view are presently all the same size; but for
> example if we were to autodetect email addresses in commit messages,
> we might have both single- and double- sided avatars in the same
> page). By hashing on email+size only we would lose the benefit of
> cache when using the same avatar at separate sizes.

You can use nested hash, like:

	$avatar_cache{$email}{$size}

or

	$avatar_cache{$size}{$email}

but that is completely orthogonal to the issue of using a single cache
shared across services, isn't it?

Re: [PATCHv5 1/3] gitweb: refactor author name insertion

[Jakub Narebski]

On Wed, 24 June 2009, Giuseppe Bilotta wrote:

> The refactoring allows easier customization of the output by means of
> CSS and improves extensibility on the CGI side too.
> 
> Layout is preserved for all views except for 'commitdiff', which now
> uses the same format as 'commit'.
> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---

Could you please state here, in the comments area, what are the 
differences between v4 (previous) and v5 (current) version of patch,
and if there are any?  It helps with patch review...

>  gitweb/gitweb.css  |    5 ++-
>  gitweb/gitweb.perl |   79 +++++++++++++++++++++++++++++++---------------------
>  2 files changed, 51 insertions(+), 33 deletions(-)


-- 
Jakub Narebski
Poland

Re: [PATCHv5 1/3] gitweb: refactor author name insertion

[Giuseppe Bilotta]

2009/6/25 Jakub Narebski <jnareb@gmail.com>:
>
> Could you please state here, in the comments area, what are the
> differences between v4 (previous) and v5 (current) version of patch,
> and if there are any?  It helps with patch review...

Sorry. This patch was untouched because I only received your comments
on it this morning. I'm working on it right now so expect and new
patch series soon.

-- 
Giuseppe "Oblomov" Bilotta