[PATCHv4 0/2] gitweb: gravatar support

[PATCHv4 0/2] gitweb: gravatar support

[Giuseppe Bilotta]

As suggested by Junio, the author display refactoring now comes before
the actual gravatar feature implementation. The refactoring also goes a
little bit deeper, changing the 'commitdiff' view to match 'commit' for
the author and committer layout.

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

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

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

[PATCHv4 2/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 |   56 ++++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 54 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..531d589 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) {
@@ -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_gravatar($co->{'author_email'}, 'space_after' => 1) . $author . "</$tag>\n";
 }
 
 # format git diff header line, i.e. "diff --(git|combined|cc) ..."
@@ -3222,6 +3249,21 @@ 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
 sub git_print_authorship {
 	my $co = shift;
@@ -3238,7 +3280,8 @@ sub git_print_authorship {
 		printf(" (%02d:%02d %s)",
 		       $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
 	}
-	print "]</$tag>\n";
+	print "]" . git_get_gravatar($co->{'author_email'}, 'space_before' => 1)
+		  . "</$tag>\n";
 }
 
 # Outputs the author and commiter name and date in long form
@@ -3246,9 +3289,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_gravatar($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 +3301,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_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

Re: [PATCHv4 2/2] gitweb: gravatar support

[Nanako Shiraishi]

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

> diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
> index 68b22ff..ddf982b 100644
> ........
> +img.avatar {
> +	vertical-align:middle;
> +}
> +
> .........
> +# 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
> +) ;
> ........

Early parts of the patch talk about "avatars"; compared with "icons" Junio
suggested, I think that is a better generic word to use for this purpose.

> +	# Gravatar support. When this feature is enabled, views such as
> ........
> +	'gravatar' => {
> +		'sub' => sub { feature_bool('gravatar', @_) },
> +		'override' => 0,
> +		'default' => [0]},
>  );

And this "feature" is about getting such avatars from "gravatar" service;
it is good to use such a specific word here.

> +# check if gravatars are enabled and dependencies are satisfied
> +our $git_gravatar_enabled = gitweb_check_feature('gravatar') &&
> +	(eval { require Digest::MD5; 1; });

The same.

> @@ -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_gravatar($co->{'author_email'}, 'space_after' => 1) . $author . "</$tag>\n";
>  }

But the function that returns a string suitable for embedding in the HTML
page, given an e-mail address, is called get_gravatar(), not get_avatar()? 

I expected from an earlier review message by Junio that get_avatar() will
look like this:

sub git_get_avatar {
	my $url = undef;
	if ($git_gravatar_enabled) {
		my $md5 = .......;
		$url = "http://www.gravatar.com/avatar.php?gravatar_id=$md5";
	} else if ($git_picons_enabled) {
		my $userpath = .......;
		$url = "http://www.cs.indiana.edu/picons/db/users/$userpath/face.xpm";
	}
	return "" unless (defined $url);
	return $pre_white . "<img ... src=\"$url\" size=$size />" . $post_white;
}

(without "picons" part, of course).

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Re: [PATCHv4 2/2] gitweb: gravatar support

[Giuseppe Bilotta]

On Wed, Jun 24, 2009 at 3:44 PM, Nanako Shiraishi<nanako3@lavabit.com> wrote:
> Quoting Giuseppe Bilotta <giuseppe.bilotta@gmail.com>:
>
>> diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
>> index 68b22ff..ddf982b 100644
>> ........
>> +img.avatar {
>> +     vertical-align:middle;
>> +}
>> +
>> .........
>> +# 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
>> +) ;
>> ........
>
> Early parts of the patch talk about "avatars"; compared with "icons" Junio
> suggested, I think that is a better generic word to use for this purpose.

Well, I would argue that 'avatar' is pretty commonly understood to be
the name of the picture identifying an individual online. OTOH, I've
been thinking that there are other kind of icons that might be used
across gitweb in the future, and they are likely to use the same
sizes, so I'll go for icon_size here.

>> @@ -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_gravatar($co->{'author_email'}, 'space_after' => 1) . $author . "</$tag>\n";
>>  }
>
> But the function that returns a string suitable for embedding in the HTML
> page, given an e-mail address, is called get_gravatar(), not get_avatar()?
>
> I expected from an earlier review message by Junio that get_avatar() will
> look like this:
>
> sub git_get_avatar {
>        my $url = undef;
>        if ($git_gravatar_enabled) {
>                my $md5 = .......;
>                $url = "http://www.gravatar.com/avatar.php?gravatar_id=$md5";
>        } else if ($git_picons_enabled) {
>                my $userpath = .......;
>                $url = "http://www.cs.indiana.edu/picons/db/users/$userpath/face.xpm";
>        }
>        return "" unless (defined $url);
>        return $pre_white . "<img ... src=\"$url\" size=$size />" . $post_white;
> }
>
> (without "picons" part, of course).

Right. I was wondering if it would make sense to factor each avatar
block separately (i.e. have a git_get_avatar that calls
git_get_whatever) but it probably doesn't make sense.

I'll wait for Jakub's (and whoever else wants to chip in) comments,
and then I'll send an update to this patch following your
recommendations.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv4 2/2] gitweb: gravatar support

[Jakub Narebski]

On Tue, 23 June 2009, Giuseppe Bilotta wrote:

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

That reminds me that I have refactoring all log-like views in my TODO
file for gitweb for quite long time.

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

Good description.

> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.css  |    4 +++
>  gitweb/gitweb.perl |   56 ++++++++++++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 54 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;
> +}
> +

Nitpick: "vertical-align: middle;" (with space separating key from
value).

>  div.page_header {
>  	height: 25px;
>  	padding: 8px;
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 223648f..531d589 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
> +) ;

Good idea, good description.  I wonder if it would be worth adding
to gitweb_conf.perl description in gitweb/README and gitweb/INSTALL...

Nitpick: ");"

> +
>  # 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]},
>  );

Good.

>  
>  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; });
> +

I think this is correct.

>  # 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_gravatar($co->{'author_email'}, 'space_after' => 1) . $author . "</$tag>\n";
>  }

Line too long, please break it.

>  
>  # format git diff header line, i.e. "diff --(git|combined|cc) ..."
> @@ -3222,6 +3249,21 @@ 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;" : "");

This name of parameter is a bit too low level for my taste.  It doesn't
matter whether we add '&nbsp;' nonbreakable space before or after img,
but whethere gravatar image has _padding_ on the left/on the right hand
side of gravatar image.  So 'pad_left' and 'pad_right', or 'pad_before'
and 'pad_after' rather than 'space_before' and 'space_after'.

But this is a matter of taste...

> +		my $size = $avatar_size{$params{'size'}} || $avatar_size{'default'};

Nice trick.

> +		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
>  sub git_print_authorship {
>  	my $co = shift;
> @@ -3238,7 +3280,8 @@ sub git_print_authorship {
>  		printf(" (%02d:%02d %s)",
>  		       $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
>  	}
> -	print "]</$tag>\n";
> +	print "]" . git_get_gravatar($co->{'author_email'}, 'space_before' => 1)
> +		  . "</$tag>\n";
>  }
>  

Good.

>  # Outputs the author and commiter name and date in long form
> @@ -3246,9 +3289,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_gravatar($co->{'author_email'}, 'size' => 'double') . "</td></tr>\n" .

                                    . git_get_gravatar($co->{'author_email'}, 'size' => 'double') .

> +	      "<tr><td></td><td> $ad{'rfc2822'}";

If you put <tr> on separate line, as was before, it would be more 
obvious in this diff that you are only adding single line.

>  	if ($ad{'hour_local'} < 6) {
>  		printf(" (<span class=\"atnight\">%02d:%02d</span> %s)",
>  		       $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
> @@ -3258,7 +3301,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_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";

Sidenote: you add here additional column.  Which is present only in 
fragment of this table.  Doesn't it screw layout of the rest of headers?

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

-- 
Jakub Narebski
Poland

Re: [PATCHv4 1/2] gitweb: refactor author name insertion

[Jakub Narebski]

On Tue, 23 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.

How?  The above description is slightly lacking in details.  Does it
mean that we replaced remains of presentational HTML (<i> and <b> tags)
by CSS styling using classes?  What about support for text browsers 
such as lynx, links/elinks, w3m?  (I'm not saying here that this should
block moving from presentational HTML to CSS, just that it should be
considered).

How it improves extensibility on the CGI side?  Does it mean that it
would be easier to add new features or otherwise extend gitweb, because
common parts are factored out?

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

Currently (i.e. without this patch) gitweb uses the following forms of
showing authorship, from shortest to longest:

 * "Giuseppe Bilotta"
  
   This is form used in 'shortlog' and 'history' views.

 * "Giuseppe Bilotta [Mon, 22 Jun 2009 22:49:58 +0000]"

   This is form used by 'log' view.

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

   This is form used by 'commitdiff' view.

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

   This is form used by 'commit' view.

I can understand factoring out code dealing with authorship in 'commit'
view, because it is a bit different from other headers; on the other
hand it is slightly similar to dealing other headers.

I think that changing 'commitdiff' view to use more detailed author
information might be a good idea.  But I do not think that having
it bundled together with this refactoring patch is a good idea.  It
really should be a separate patch, and refactoring shouldn't change
output, if possible (and I think here is very much possible, see below).

If you wanted to reduce number of places where you need to add later
gravatar support, it would be better to factor out formatting authorship
information in 'log' and 'commitdiff' view, either by having 'log' view
use 'commitdiff' authorship subroutine (and not 'commitdiff' use new
'commit' authorship subroutine), or make this subroutine produce two
slightly different outputs.

> 
> 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;
>  }

Good.

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

I would like to see calling convention for this subroutine described, 
because it is not entirely obvious; see passing of (misnamed)
chop_and_escape_str subroutine parameters.

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

Good.  You could have add option to print or not print localtime
(or use version with localtime in 'log' view).

> +
> +# Outputs the author and commiter name and date in long form
> +sub git_print_who {

Hmmm... perhaps git_print_authorship_long?  I am not sure if git_print_who
is a good name to have.  (Yes, I know, naming is hard).

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

While it might be a good idea of factoring out this part into separate
subroutine, I don't think that 'commitdiff' should use it... well, at
least not in this commit.

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

+		      format_author_html('td', \%co, 10) . "<td>";

Tabs are for indent, spaces are for align, at least for gitweb.perl
(this unfortunately requires either handcrafting, or using smart-tabs,
but it has the advantage of presenting the same layour regardless of
the tab size).

>  		print format_subject_html($co{'title'}, $co{'title_short'},
>  		                          href(action=>"commit", hash=>$commit), $ref);
>  		print "</td>\n" .

Very good.

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

Tabs are for indent, spaces are for align.
And you don't need to try to align in the comment now.

>  		# 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"},

Good. 

This one has correct whitespace mixture (tabs for indent, spaces for
align).

> @@ -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);

Good.

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

This makes git_commit subroutine less complex, so it might be good
idea anyway.

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

But here we can use git_print_authorship('div', \%co, undef, undef, -localtime=>1)
or something like that; no need to change 'commitdiff' view.

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

-- 
Jakub Narebski
Poland

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

[Junio C Hamano]

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

> As suggested by Junio, the author display refactoring now comes before
> the actual gravatar feature implementation. The refactoring also goes a
> little bit deeper, changing the 'commitdiff' view to match 'commit' for
> the author and committer layout.
>
> Giuseppe Bilotta (2):
>   gitweb: refactor author name insertion
>   gitweb: gravatar support
>
>  gitweb/gitweb.css  |    9 +++-
>  gitweb/gitweb.perl |  123 ++++++++++++++++++++++++++++++++++++++-------------
>  2 files changed, 99 insertions(+), 33 deletions(-)

Thanks.  It looks much better for reviewing, now the order of the patches
are sane.

Does anybody have comments on the patches?

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

[Jakub Narebski]

Junio C Hamano wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
> 
> > As suggested by Junio, the author display refactoring now comes before
> > the actual gravatar feature implementation. The refactoring also goes a
> > little bit deeper, changing the 'commitdiff' view to match 'commit' for
> > the author and committer layout.
> >
> > Giuseppe Bilotta (2):
> >   gitweb: refactor author name insertion
> >   gitweb: gravatar support
> >
> >  gitweb/gitweb.css  |    9 +++-
> >  gitweb/gitweb.perl |  123 ++++++++++++++++++++++++++++++++++++++-------------
> >  2 files changed, 99 insertions(+), 33 deletions(-)
> 
> Thanks.  It looks much better for reviewing, now the order of the patches
> are sane.
> 
> Does anybody have comments on the patches?

I'll send my comments today.

-- 
Jakub Narebski
Poland

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

[Nanako Shiraishi]

Quoting Junio C Hamano <gitster@pobox.com>:

> Thanks.  It looks much better for reviewing, now the order of the patches
> are sane.
>
> Does anybody have comments on the patches?

I'll send my comments later today.

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/