[PATCH] gitweb: gravatar support

[PATCH] gitweb: gravatar support

[Giuseppe Bilotta]

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

The feature is disabled by default, and depends on Digest::MD5.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 1e7e2d8..9f40070 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -365,6 +365,18 @@ 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];
+	# Project specific override is not supported.
+	'gravatar' => {
+		'override' => 0,
+		'default' => [0]},
 );
 
 sub gitweb_get_feature {
@@ -814,6 +826,9 @@ $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 { use Digest::MD5 qw(md5_hex); 1; });
+
 # dispatch
 if (!defined $action) {
 	if (defined $hash) {
@@ -4123,6 +4138,19 @@ sub git_project_list_body {
 	print "</table>\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, $size) = @_;
+		$size = 32 if $size <= 0;
+		return "<img src=\"http://www.gravatar.com/avatar.php?gravatar_id=" .
+		       md5_hex($email) . "&amp;size=$size\" style=\"vertical-align:middle\" />";
+	} else {
+		return "";
+	}
+}
+
 sub git_shortlog_body {
 	# uses global variable $project
 	my ($commitlist, $from, $to, $refs, $extra) = @_;
@@ -4145,7 +4173,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'}, 16) . "&nbsp;<i>" . $author . "</i></td>\n" .
 		      "<td>";
 		print format_subject_html($co{'title'}, $co{'title_short'},
 		                          href(action=>"commit", hash=>$commit), $ref);
@@ -5095,8 +5123,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'}, 16) .
+		      "<br/>\n</div>\n";
 
 		print "<div class=\"log_body\">\n";
 		git_print_log($co{'comment'}, -final_empty_line=> 1);
@@ -5183,7 +5212,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(lc $co{'author_email'}) . "</td></tr>\n" .
 	      "<tr>" .
 	      "<td></td><td> $ad{'rfc2822'}";
 	if ($ad{'hour_local'} < 6) {
@@ -5195,7 +5225,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(lc $co{'committer_email'}) . "</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.1.282.g9f93

Re: [PATCH] gitweb: gravatar support

[Jakub Narebski]

On Fri, 19 June 2009, Giuseppe Bilotta wrote:

> Introduce gravatar support by adding the appropriate img tag next to
> author and committer in commit, shortlog and log view.
> 
> The feature is disabled by default, and depends on Digest::MD5.

Digest::MD5 is core Perl module and (in, I guess, most cases)
installed together with Perl.

You might want to mention it in the commit message.

> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.perl |   41 ++++++++++++++++++++++++++++++++++++-----
>  1 files changed, 36 insertions(+), 5 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 1e7e2d8..9f40070 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -365,6 +365,18 @@ 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];
> +	# Project specific override is not supported.
> +	'gravatar' => {
> +		'override' => 0,
> +		'default' => [0]},

Yet another global feature without project specific override.  Hmmm...
I wonder if project specific and global (non-overridable) features 
should be separated.  But it is question for a separate commit.

Question: why it is not overridable (why project specific override
is not supported for this feature)?  Some projects may use Gravatars,
some might not, although I guess that usually it is deployment specific
feature.

>  );
>  
>  sub gitweb_get_feature {
> @@ -814,6 +826,9 @@ $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 { use Digest::MD5 qw(md5_hex); 1; });

I would use line break here:

  +our $git_gravatar_enabled = gitweb_check_feature('gravatar') && 
  +    (eval { use Digest::MD5 qw(md5_hex); 1; });

or

  +our $git_gravatar_enabled = gitweb_check_feature('gravatar')
  +    && (eval { use Digest::MD5 qw(md5_hex); 1; });


I really like the fact that we do not include Digest::MD5 if it is not
required (if feature is not tuned on).

> +
>  # dispatch
>  if (!defined $action) {
>  	if (defined $hash) {
> @@ -4123,6 +4138,19 @@ sub git_project_list_body {
>  	print "</table>\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, $size) = @_;
> +		$size = 32 if $size <= 0;

You would probably want to protect against $size being undefined:

  +		$size = 32 if (!defined($size) || $size <= 0);

Because currently when you are not passing size parameter to use 
default size you would get the following warning:

  Use of uninitialized value in numeric le (<=) ...

Did you run the t9500, adding test enabling gravatars?  

Hmmm... perhaps we should add test with all possible features turned
on (unless they need extra configuration... oh, so it isn't as easy
as I initially thought to add this...).

> +		return "<img src=\"http://www.gravatar.com/avatar.php?gravatar_id=" .
> +		       md5_hex($email) . "&amp;size=$size\" style=\"vertical-align:middle\" />";

Hmmm... 'style="vertical-align:middle"' or 'class="gravatar"' (plus 
addition to CSS)?

> +	} else {
> +		return "";
> +	}
> +}
> +


[...]
> @@ -4145,7 +4173,7 @@ sub git_shortlog_body {

> +		      "<td>" . git_get_gravatar($co{'author_email'}, 16) . "&nbsp;<i>" . $author . "</i></td>\n" .

> @@ -5095,8 +5123,9 @@ sub git_log {

> +		      "<i>" . esc_html($co{'author_name'}) .  " [$ad{'rfc2822'}]</i>&nbsp;" .
> +		      git_get_gravatar($co{'author_email'}, 16) .
> +		      "<br/>\n</div>\n";

> @@ -5183,7 +5212,8 @@ sub git_commit {

> +	print "<tr><td>author</td><td>" . esc_html($co{'author'}) . "</td>".
> +	      "<td rowspan=\"2\">" .git_get_gravatar(lc $co{'author_email'}) . "</td></tr>\n" .

> +	print "<tr><td>committer</td><td>" . esc_html($co{'committer'}) . "</td>".
> +	      "<td rowspan=\"2\">" .git_get_gravatar(lc $co{'committer_email'}) . "</td></tr>\n";

Hmmm... why use 'lc' here and not in other places?  Also gravatars are
not shown in 'history' view, but I guess that could wait for proper
refactoring of all log-like views/actions to use common infrastructure.

Those all look nice with the *default* font sizes.  But as the size of
gravatar is used when constructing gravatar URL, to pass to gravatar.com
I don't see how this problem can be resolved...  Beside making it 
configurable, I guess...

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: gravatar support

[Giuseppe Bilotta]

Hello,

thanks for the comments, I'll send a new patch taking them into
consideration soon.

A few additional replies:

>> +     # 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];
>> +     # Project specific override is not supported.
>> +     'gravatar' => {
>> +             'override' => 0,
>> +             'default' => [0]},
>
> Yet another global feature without project specific override.  Hmmm...
> I wonder if project specific and global (non-overridable) features
> should be separated.  But it is question for a separate commit.
>
> Question: why it is not overridable (why project specific override
> is not supported for this feature)?  Some projects may use Gravatars,
> some might not, although I guess that usually it is deployment specific
> feature.

I see it as a deployment feature, and considering that it adds an
(admittedly small) extra load on the server, I thought it was sensible
to make it non-overridable. OTOH, since the load is small, it might be
possible to make it per-project without big issues.

> You would probably want to protect against $size being undefined:
>
>  +             $size = 32 if (!defined($size) || $size <= 0);
>
> Because currently when you are not passing size parameter to use
> default size you would get the following warning:
>
>  Use of uninitialized value in numeric le (<=) ...

Oh right.

> Did you run the t9500, adding test enabling gravatars?

Ehrm, no 8-P

> Hmmm... perhaps we should add test with all possible features turned
> on (unless they need extra configuration... oh, so it isn't as easy
> as I initially thought to add this...).

If gravatars can be added easily, I can try doing it.

>> +     print "<tr><td>author</td><td>" . esc_html($co{'author'}) . "</td>".
>> +           "<td rowspan=\"2\">" .git_get_gravatar(lc $co{'author_email'}) . "</td></tr>\n" .
>
>> +     print "<tr><td>committer</td><td>" . esc_html($co{'committer'}) . "</td>".
>> +           "<td rowspan=\"2\">" .git_get_gravatar(lc $co{'committer_email'}) . "</td></tr>\n";
>
> Hmmm... why use 'lc' here and not in other places?

That's a good question. I'm actually not sure if the gravatar service
is case sensitive or not ... the reference implementation uses lc. I
should probably move the lowercasing to git_get_gravar itself.

> Also gravatars are
> not shown in 'history' view, but I guess that could wait for proper
> refactoring of all log-like views/actions to use common infrastructure.

That's part of the reason, the other being that I couldn't find a
satisfactory way to do it 8-P

> Those all look nice with the *default* font sizes.  But as the size of
> gravatar is used when constructing gravatar URL, to pass to gravatar.com
> I don't see how this problem can be resolved...  Beside making it
> configurable, I guess...

That's something I hadn't thought about, honestly. The problem of
course is that the font sizes get customized via CSS, but the gravatar
size would get customized at the cgi level ... so unless we parse the
CSS from the cgi it cannot be done automatically.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH] gitweb: gravatar support

[Jakub Narebski]

On Fri, 19 June 2009, Giuseppe Bilotta wrote:

> thanks for the comments, I'll send a new patch taking them into
> consideration soon.

Errr... I had a few additional comments to this reply, but I was a bit
late I think with replying.

> 
> A few additional replies:
> 
>>> +     # 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];
>>> +     # Project specific override is not supported.
>>> +     'gravatar' => {
>>> +             'override' => 0,
>>> +             'default' => [0]},
>>
>> Yet another global feature without project specific override.  Hmmm...
>> I wonder if project specific and global (non-overridable) features
>> should be separated.  But it is question for a separate commit.
>>
>> Question: why it is not overridable (why project specific override
>> is not supported for this feature)?  Some projects may use Gravatars,
>> some might not, although I guess that usually it is deployment specific
>> feature.
> 
> I see it as a deployment feature, and considering that it adds an
> (admittedly small) extra load on the server, I thought it was sensible
> to make it non-overridable. OTOH, since the load is small, it might be
> possible to make it per-project without big issues.

Well, I was not asking you to change it; I was asking about 
justification behind making it non-overridable.

> 
>> You would probably want to protect against $size being undefined:
>>
>>  +             $size = 32 if (!defined($size) || $size <= 0);
>>
>> Because currently when you are not passing size parameter to use
>> default size you would get the following warning:
>>
>>  Use of uninitialized value in numeric le (<=) ...
> 
> Oh right.

Well, as you use 'undef' (do not pass parameter) for default value
and you _do not_ use negative (or zero) value, then

+             $size = 32 if !defined($size);

would be enough (in Perl 6 / Perl 5.10 it would be "$size //= 32" ;-)

The question is: does gravatar.com accepts any size?  What does it do
if it gets negative size passed?  I have not used gravatar as 
developer...

> 
>> Did you run the t9500, adding test enabling gravatars?
> 
> Ehrm, no 8-P

Well, it is always nice to have test for new feature.  Unfortunately
due to the smart way it is done currently unmodified (not extended)
t9500 wouldn't catch above issue.

[...]

>> Also gravatars are
>> not shown in 'history' view, but I guess that could wait for proper
>> refactoring of all log-like views/actions to use common infrastructure.
> 
> That's part of the reason, the other being that I couldn't find a
> satisfactory way to do it 8-P

What is the problem?

> 
>> Those all look nice with the *default* font sizes.  But as the size of
>> gravatar is used when constructing gravatar URL, to pass to gravatar.com
>> I don't see how this problem can be resolved...  Beside making it
>> configurable, I guess...
> 
> That's something I hadn't thought about, honestly. The problem of
> course is that the font sizes get customized via CSS, but the gravatar
> size would get customized at the cgi level ... so unless we parse the
> CSS from the cgi it cannot be done automatically.

What I though here was to use %gravatar_size hash, with keys such as
'default' and 'double' (for "double line height").  If you change or
add CSS, changing configuration, you can change also this in config.

More comments to follow in v2 and replies.
-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: gravatar support

[Giuseppe Bilotta]

On Sat, Jun 20, 2009 at 12:31 AM, Jakub Narebski<jnareb@gmail.com> wrote:
>>> Question: why it is not overridable (why project specific override
>>> is not supported for this feature)?  Some projects may use Gravatars,
>>> some might not, although I guess that usually it is deployment specific
>>> feature.
>>
>> I see it as a deployment feature, and considering that it adds an
>> (admittedly small) extra load on the server, I thought it was sensible
>> to make it non-overridable. OTOH, since the load is small, it might be
>> possible to make it per-project without big issues.
>
> Well, I was not asking you to change it; I was asking about
> justification behind making it non-overridable.

I decided to go for the override because it was little effort and it
whether or not the override can take place can still be decided at a
central place.

>>> You would probably want to protect against $size being undefined:
>>>
>>>  +             $size = 32 if (!defined($size) || $size <= 0);
>>>
>>> Because currently when you are not passing size parameter to use
>>> default size you would get the following warning:
>>>
>>>  Use of uninitialized value in numeric le (<=) ...
>>
>> Oh right.
>
> Well, as you use 'undef' (do not pass parameter) for default value
> and you _do not_ use negative (or zero) value, then
>
> +             $size = 32 if !defined($size);
>
> would be enough (in Perl 6 / Perl 5.10 it would be "$size //= 32" ;-)
>
> The question is: does gravatar.com accepts any size?  What does it do
> if it gets negative size passed?  I have not used gravatar as
> developer...

If you pass a negative size, the image comes up blank. Using undef for
the default value makes sense. The question rather becomes whether it
makes sense or not to provide a safety fallback for when the size
comes up negative.

>>> Also gravatars are
>>> not shown in 'history' view, but I guess that could wait for proper
>>> refactoring of all log-like views/actions to use common infrastructure.
>>
>> That's part of the reason, the other being that I couldn't find a
>> satisfactory way to do it 8-P
>
> What is the problem?

I really don't remember, I created the patch a long time ago. As you
can probably seen from v2, I just decided to go with the same code
everywhere (which asks for a refactoring).

>>> Those all look nice with the *default* font sizes.  But as the size of
>>> gravatar is used when constructing gravatar URL, to pass to gravatar.com
>>> I don't see how this problem can be resolved...  Beside making it
>>> configurable, I guess...
>>
>> That's something I hadn't thought about, honestly. The problem of
>> course is that the font sizes get customized via CSS, but the gravatar
>> size would get customized at the cgi level ... so unless we parse the
>> CSS from the cgi it cannot be done automatically.
>
> What I though here was to use %gravatar_size hash, with keys such as
> 'default' and 'double' (for "double line height").  If you change or
> add CSS, changing configuration, you can change also this in config.

And since the sices could be used for other things than gravatar, we
could call it just %avatar_size_hash or whatever, and future support
for picons or whatever else can use it too.


-- 
Giuseppe "Oblomov" Bilotta