[PATCHv3 0/2] gitweb: gravatar support

[PATCHv3 0/2] gitweb: gravatar support

[Giuseppe Bilotta]

I've done the refactoring in a separate patch, to have a better (IMO)
view of what I was doing. I can squash them together if needed. There
have also been a couple of other changes such as the use of 'require'
instead of 'use' to access Digest::MD5, and a better call pattern for
git_get_gravatar.

One thing that is missing but could be done is an email -> MD5 hash
mapping to cache multiple occurences of the same email (not unusual
in (short)log view).

Giuseppe Bilotta (2):
  gitweb: gravatar support
  gitweb: refactor author+gravatar insertion

 gitweb/gitweb.css  |    9 +++++-
 gitweb/gitweb.perl |   83 ++++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 76 insertions(+), 16 deletions(-)

[PATCHv3 1/2] 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.

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

diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index a01eac8..eaf74c3 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 1e7e2d8..f5654d7 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 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) {
@@ -3214,12 +3241,28 @@ sub git_print_header_div {
 	      "\n</div>\n";
 }
 
+# insert a gravatar for the given $email at the given $size if the feature
+# is enabled
+sub git_get_gravatar {
+	if ($git_gravatar_enabled) {
+		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'};
+		return $pre_white . "<img class=\"avatar\" src=\"http://www.gravatar.com/avatar.php?gravatar_id=" .
+		       Digest::MD5::md5_hex(lc $email) . "&amp;size=$size\" />" . $post_white;
+	} else {
+		return "";
+	}
+}
+
 sub git_print_authorship {
 	my $co = shift;
 
 	my %ad = parse_date($co->{'author_epoch'}, $co->{'author_tz'});
 	print "<div class=\"author_date\">" .
 	      esc_html($co->{'author_name'}) .
+	      git_get_gravatar($co->{'author_email'}, 'space_before' => 1) .
 	      " [$ad{'rfc2822'}";
 	if ($ad{'hour_local'} < 6) {
 		printf(" (<span class=\"atnight\">%02d:%02d</span> %s)",
@@ -4145,7 +4188,7 @@ sub git_shortlog_body {
 		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>" . git_get_gravatar($co{'author_email'}, 'space_after' => 1) . "<i>" . $author . "</i></td>\n" .
 		      "<td>";
 		print format_subject_html($co{'title'}, $co{'title_short'},
 		                          href(action=>"commit", hash=>$commit), $ref);
@@ -4196,7 +4239,7 @@ sub git_history_body {
 	# 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>" . git_get_gravatar($co{'author_email'}, 'space_after' => 1) . "<i>" . $author . "</i></td>\n" .
 		      "<td>";
 		# originally git_history used chop_str($co{'title'}, 50)
 		print format_subject_html($co{'title'}, $co{'title_short'},
@@ -4352,7 +4395,7 @@ sub git_search_grep_body {
 		$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" .
+		      "<td>" . git_get_gravatar($co{'author_email'}, 'space_after' => 1) . "<i>" . $author . "</i></td>\n" .
 		      "<td>" .
 		      $cgi->a({-href => href(action=>"commit", hash=>$co{'id'}),
 		               -class => "list subject"},
@@ -5095,8 +5138,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";
+		      "<i>" . esc_html($co{'author_name'}) .  " [$ad{'rfc2822'}]</i>&nbsp;" .
+		      git_get_gravatar($co{'author_email'}) .
+		      "<br/>\n</div>\n";
 
 		print "<div class=\"log_body\">\n";
 		git_print_log($co{'comment'}, -final_empty_line=> 1);
@@ -5183,7 +5227,8 @@ 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".
+	print "<tr><td>author</td><td>" . esc_html($co{'author'}) . "</td>".
+	      "<td rowspan=\"2\">" .git_get_gravatar($co{'author_email'}, 'size' => 'double') . "</td></tr>\n" .
 	      "<tr>" .
 	      "<td></td><td> $ad{'rfc2822'}";
 	if ($ad{'hour_local'} < 6) {
@@ -5195,7 +5240,8 @@ sub git_commit {
 	}
 	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_gravatar($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

[PATCHv3 2/2] gitweb: refactor author+gravatar insertion

[Giuseppe Bilotta]

---
 gitweb/gitweb.css  |    5 +++-
 gitweb/gitweb.perl |   69 ++++++++++++++++++++++++++++-----------------------
 2 files changed, 42 insertions(+), 32 deletions(-)

diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index eaf74c3..ddf982b 100644
--- a/gitweb/gitweb.css
+++ b/gitweb/gitweb.css
@@ -136,11 +136,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 f5654d7..cb4ecc3 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1496,6 +1496,31 @@ sub format_subject_html {
 	}
 }
 
+# insert a gravatar for the given $email at the given $size if the feature
+# is enabled
+sub git_get_gravatar {
+	if ($git_gravatar_enabled) {
+		my ($email, %params) = @_;
+		my $pre_white = ($params{'space_before'} ? " " : "");
+		my $post_white = ($params{'space_after'} ? " " : "");
+		my $size = $avatar_size{$params{'size'}} || $avatar_size{'default'};
+		return $pre_white . "<img class=\"avatar\" src=\"http://www.gravatar.com/avatar.php?gravatar_id=" .
+		       Digest::MD5::md5_hex(lc $email) . "&amp;size=$size\" />" . $post_white;
+	} else {
+		return "";
+	}
+}
+
+# format, perhaps shortened and preceded by the gravatar, the author name
+sub format_author_html {
+	my $tag = shift;
+	my $co = shift;
+	my $author = chop_and_escape_str($co->{'author_name'}, @_);
+	return "<$tag class=\"author\">" .
+		git_get_gravatar($co->{'author_email'}, 'space_after' => 1) .
+		$author . "</$tag>\n";
+}
+
 # format git diff header line, i.e. "diff --(git|combined|cc) ..."
 sub format_git_diff_header_line {
 	my $line = shift;
@@ -3241,28 +3266,14 @@ sub git_print_header_div {
 	      "\n</div>\n";
 }
 
-# insert a gravatar for the given $email at the given $size if the feature
-# is enabled
-sub git_get_gravatar {
-	if ($git_gravatar_enabled) {
-		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'};
-		return $pre_white . "<img class=\"avatar\" src=\"http://www.gravatar.com/avatar.php?gravatar_id=" .
-		       Digest::MD5::md5_hex(lc $email) . "&amp;size=$size\" />" . $post_white;
-	} else {
-		return "";
-	}
-}
-
+# Outputs the author name and date in long form, followed by the gravatar
 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'}) .
-	      git_get_gravatar($co->{'author_email'}, 'space_before' => 1) .
 	      " [$ad{'rfc2822'}";
 	if ($ad{'hour_local'} < 6) {
 		printf(" (<span class=\"atnight\">%02d:%02d</span> %s)",
@@ -3271,7 +3282,9 @@ sub git_print_authorship {
 		printf(" (%02d:%02d %s)",
 		       $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
 	}
-	print "]</div>\n";
+	print "]" .
+	git_get_gravatar($co->{'author_email'}, 'space_before' => 1) .
+	"</$tag>\n";
 }
 
 sub git_print_page_path {
@@ -4185,11 +4198,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>" . git_get_gravatar($co{'author_email'}, 'space_after' => 1) . "<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" .
@@ -4236,11 +4247,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>" . git_get_gravatar($co{'author_email'}, 'space_after' => 1) . "<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);
@@ -4393,9 +4402,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>" . git_get_gravatar($co{'author_email'}, 'space_after' => 1) . "<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"},
@@ -5137,10 +5145,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>&nbsp;" .
-		      git_get_gravatar($co{'author_email'}) .
-		      "<br/>\n</div>\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);
-- 
1.6.3.rc1.192.gdbfcb

Re: [PATCHv3 0/2] gitweb: gravatar support

[Junio C Hamano]

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

> I've done the refactoring in a separate patch, to have a better (IMO)
> view of what I was doing. I can squash them together if needed.

I personally think squashing won't add much value in this case, but I
think swapping would.

The recommended order is that refactor and clean-up come first and then a
new feature is built on the base that is made more solid, thanks to the
refactoring.  That allows the reviewers to see and judge the extent of
damage to the codebase caused by the new feature more fairly.

For example, adding a new feature to an unclean codebase may have to touch
ten different places to add the same code, which would make the feature
look expensive and intrusive.

But if you first refactor the codebase cleanly, these ten places will be
calling a single helper function, and you may be able to add the new
feature by touching only one place, the helper function.  It would be
clear that the change is not intrusive if presented in such an order.

This is a tangent, but the key to a better design is to try to resist the
templation to think that yours will be the last cool feature.  If you
think "with addition of gravatar, the authorship display is complete",
then there is no reason to name the subroutine that is called by
"git_print_authorship" anything but "get_gravatar".  If you resist the
temptation, however, you would realize that other people may want to add
support for different sources of person-label sources, and try to come up
with a more generic name to call it [*1*].

I would not change the name of variable $git_gravatar_enabled, though.
That way, people who would want other kinds of personal-icons can add an
else-if to the sub, without changing other things.


[Footnote]

*1* To put it differently, you can tell where one's imagination ends by
looking at the way that person refactors a piece of code and gives names.
A concrete name (e.g. "gravatar") shows where the boundary of abstraction
lies in that person's imagination.  The patch shows that it was done by
somebody who never thought of the possibility that git_print_authorship
might be able to use service other than gravatar to show small icons
associated with an email if other people build on top of his work.