threads / discuss / 19888

[PATCHv3 0/2] gitweb: gravatar support

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

## tl;dr

4 messages between Jun 21, 2009 and Jun 21, 2009.

replies: 3people: 2as markdown or json

Giuseppe Bilotta· Jun 21, 2009, 17:57 UTC · lore

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(-)
Giuseppe Bilotta· Jun 21, 2009, 17:57 UTC · re: Giuseppe Bilotta · lore

[PATCHv3 1/2] gitweb: gravatar support

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
Giuseppe Bilotta· Jun 21, 2009, 17:57 UTC · re: Giuseppe Bilotta · lore

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

---
 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'} ? "&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 "";
+	}
+}
+
+# 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
Junio C Hamano· Jun 21, 2009, 22:34 UTC · re: Giuseppe Bilotta · lore

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

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.

← back to recent threads