[PATCHv2] gitweb: gravatar support

[PATCHv2] 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, which
is available in most Perl installations.

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

Changes from the previous version include gravatar use in history view,
CSS use and the possibility to override the feature on a per-project basis.

diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index a01eac8..ca716e6 100644
--- a/gitweb/gitweb.css
+++ b/gitweb/gitweb.css
@@ -28,6 +28,10 @@ img.logo {
 	border-width: 0px;
 }
 
+img.gravatar {
+	vertical-align:middle;
+}
+
 div.page_header {
 	height: 25px;
 	padding: 8px;
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 1e7e2d8..c06356b 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -365,6 +365,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 +829,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 { use Digest::MD5 qw(md5_hex); 1; });
+
 # dispatch
 if (!defined $action) {
 	if (defined $hash) {
@@ -3214,11 +3233,26 @@ 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, $size, $whitespace) = @_;
+		$whitespace = 0 unless defined($whitespace);
+		$size = 32 if (!defined($size) || $size <= 0);
+		return "<img class=\"gravatar\" src=\"http://www.gravatar.com/avatar.php?gravatar_id=" .
+		       md5_hex(lc $email) . "&amp;size=$size\" />" . ($whitespace ? "&nbsp;" : "");
+	} else {
+		return "";
+	}
+}
+
 sub git_print_authorship {
 	my $co = shift;
 
 	my %ad = parse_date($co->{'author_epoch'}, $co->{'author_tz'});
 	print "<div class=\"author_date\">" .
+	      git_get_gravatar($co->{'author_email'}, 16, 1) .
 	      esc_html($co->{'author_name'}) .
 	      " [$ad{'rfc2822'}";
 	if ($ad{'hour_local'} < 6) {
@@ -4145,7 +4179,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, 1) . "<i>" . $author . "</i></td>\n" .
 		      "<td>";
 		print format_subject_html($co{'title'}, $co{'title_short'},
 		                          href(action=>"commit", hash=>$commit), $ref);
@@ -4196,7 +4230,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'}, 16, 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 +4386,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'}, 16, 1) . "<i>" . $author . "</i></td>\n" .
 		      "<td>" .
 		      $cgi->a({-href => href(action=>"commit", hash=>$co{'id'}),
 		               -class => "list subject"},
@@ -5095,8 +5129,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 +5218,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'}) . "</td></tr>\n" .
 	      "<tr>" .
 	      "<td></td><td> $ad{'rfc2822'}";
 	if ($ad{'hour_local'} < 6) {
@@ -5195,7 +5231,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'}) . "</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: [PATCHv2] gitweb: gravatar support

[Johannes Schindelin]

Hi,

On Fri, 19 Jun 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, which
> is available in most Perl installations.
> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

Something important is missing in this message, which was the reason why I 
had to go through the hassle of reading the source code to find out!

	This feature is optional and can be enabled by ...

Hth,
Dscho

Re: [PATCHv2] gitweb: gravatar support

[Johannes Schindelin]

Hi,

On Fri, 19 Jun 2009, Johannes Schindelin wrote:

> On Fri, 19 Jun 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, which 
> > is available in most Perl installations.
> > 
> > Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> 
> Something important is missing in this message, which was the reason why 
> I had to go through the hassle of reading the source code to find out!
> 
> 	This feature is optional and can be enabled by ...

Oops.  Please disregard my response.

Ciao,
Dscho

Re: [PATCHv2] gitweb: gravatar support

[Junio C Hamano]

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

> 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, which
> is available in most Perl installations.
>
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.css  |    4 ++++
>  gitweb/gitweb.perl |   51 ++++++++++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 48 insertions(+), 7 deletions(-)
>
> Changes from the previous version include gravatar use in history view,
> CSS use and the possibility to override the feature on a per-project basis.

I see these repeated patterns in your patch.

> @@ -4145,7 +4179,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, 1) . "<i>" . $author . "</i></td>\n" .
>...
> -		      "<td><i>" . $author . "</i></td>\n" .
> +		      "<td>" . git_get_gravatar($co{'author_email'}, 16, 1) . "<i>" . $author . "</i></td>\n" .
>...
> -		      "<td><i>" . $author . "</i></td>\n" .
> +		      "<td>" . git_get_gravatar($co{'author_email'}, 16, 1) . "<i>" . $author . "</i></td>\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'}) . "</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'}) . "</td></tr>\n";
>...

Doesn't it strike you as needing a bit more refactoring?  The first three
are identical, and you can refactor them to

-		      "<td><i>" . $author . "</i></td>\n" .
+		      "<td>" . oneline_person($author) . "</td>\n" .

where oneline_person is

	sub oneline_person {
        	my ($me) = @_;
                return ($me->{'smallicon'} .
			"<i>" . $me->{'name_escaped'} . "</i>");
        }

And instead of doing "my $author = chop_and_escape_str()", you will
set up richer my $author upfront, like so:

	my $author = +{
		name_escaped => chop_and_escape_str($co{'author_name'}, 10),
                smallicon => git_get_gravatar($co{'author_email'}, 16, 1),
        };

and pass it to the helper function.

That way, people who do not like italics, or people who want to have the
icon at the end instead of at the beginning, can touch only one place
(i.e. "sub oneline_person").

By the way, in the above example, I named the field 'smallicon', as use of
gravatar is merely an implementation detail.  It is plausible other people
may want to use picons instead.

I do not know about the following hunk (why does it have the icon at the
end, unlike the other ones?), but I think you got the idea.

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

In the medium term, we may want to go one step further and do

    package person;

and make sets of methods like "oneline_person".

Re: [PATCHv2] gitweb: gravatar support

[Giuseppe Bilotta]

On Fri, Jun 19, 2009 at 10:28 PM, Junio C Hamano<gitster@pobox.com> wrote:
>
> I see these repeated patterns in your patch.
>
>> @@ -4145,7 +4179,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, 1) . "<i>" . $author . "</i></td>\n" .
>>...
>> -                   "<td><i>" . $author . "</i></td>\n" .
>> +                   "<td>" . git_get_gravatar($co{'author_email'}, 16, 1) . "<i>" . $author . "</i></td>\n" .
>>...
>> -                   "<td><i>" . $author . "</i></td>\n" .
>> +                   "<td>" . git_get_gravatar($co{'author_email'}, 16, 1) . "<i>" . $author . "</i></td>\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'}) . "</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'}) . "</td></tr>\n";
>>...
>
> Doesn't it strike you as needing a bit more refactoring?

I was having the same thoughts while writing v2 of the patch, indeed.
However, I wasn't sure if it was appropriate to the refactoring in the
same patch. A couple of places could be changed to use the existing
git_print_authorship, others would need their own function, as you
point out.

> By the way, in the above example, I named the field 'smallicon', as use of
> gravatar is merely an implementation detail.  It is plausible other people
> may want to use picons instead.

I had to google for picons but yes, it makes sense.

> I do not know about the following hunk (why does it have the icon at the
> end, unlike the other ones?), but I think you got the idea.
>
>> -                   "<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";

I had the impression that in this case it made more aesthetical sense
to have the icon on the other side. I'm not exactly a good designer
though, so we might want to prefer consistency and keep it all the
same way. This section would probably use git_print_authorship or at
least share some code with that.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv2] 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.

You have added gravatar support also to 'history' view, which you
should mention here.  By the way, I have noticed that there is no
gravatar support in 'commitdiff' view... which probably means that,
as Junio said, this would require some refactoring and factoring out
common code.

> 
> The feature is disabled by default, and depends on Digest::MD5, which
> is available in most Perl installations.

I wonder if there should be mentioned that if Digest::MD5 is not 
installed, the feature will not be used...

> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.css  |    4 ++++
>  gitweb/gitweb.perl |   51 ++++++++++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 48 insertions(+), 7 deletions(-)
> 
> Changes from the previous version include gravatar use in history view,
> CSS use and the possibility to override the feature on a per-project basis.

There are a few other 'code' changes...

> 
> diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
> index a01eac8..ca716e6 100644
> --- a/gitweb/gitweb.css
> +++ b/gitweb/gitweb.css
> @@ -28,6 +28,10 @@ img.logo {
>  	border-width: 0px;
>  }
>  
> +img.gravatar {
> +	vertical-align:middle;
> +}
> +

That is good enough CSS class name for now, but in the future we might
want to use more generic name: avatar, personal_avatar, personal_icon;
or something like that.

>  div.page_header {
>  	height: 25px;
>  	padding: 8px;
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 1e7e2d8..c06356b 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -365,6 +365,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]},

Nice.  I think it can be on per-project basis decision whether to
use avatars or not.  For example if project uses fake emails, etc.

[...]
> +# 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, $whitespace) = @_;
> +		$whitespace = 0 unless defined($whitespace);
> +		$size = 32 if (!defined($size) || $size <= 0);
> +		return "<img class=\"gravatar\" src=\"http://www.gravatar.com/avatar.php?gravatar_id=" .
> +		       md5_hex(lc $email) . "&amp;size=$size\" />" . ($whitespace ? "&nbsp;" : "");
> +	} else {
> +		return "";
> +	}
> +}

I don't like this change.  I don't like it at all.

First, there is an issue of having two _independently_ optional
parameters.  Using 'undef' / not passing parameter to use default value
is IMVHO an anti-pattern; it is a bit unwieldy / ugly to call it with
$whitespace set while using default value for $size.

Second, I think it is better to avoid such boolean parameters like
$whitespace, because you don't know from call site what it means
to pass 1 there (in C we would use constant instead of 1 here).

Both of above are solved in current gitweb code (at least in some
places) by passing hash of (named) optional parameters.  So the call
(if not for last note for this issue) would look like the following:

  git_get_gravatar($co->{'author_email'}, -size => 16, -whitespace => 1);

Third, rather than formatting with '&nbsp;' (ugh, just like formatting
with tab and enter in word processors) I'd rather use (if possible)
CSS style for that; something like margin-right, or padding-right...
if it would work, of course.  But see also note below.


Last but not least, but after thinking about it a bit more I think now
that those optional parameters are entirely too low level, too close to
implementation.  What matters is not the size (which you would then need
to change in every and each callsite) and adding &nbsp; after image.
What matters is whether to use double size, and whether gravatar image
is on the left side or on the right side of text it is referring to.

You would be then able to easily chose small/large size from hash of
possible sizes (mentioned %avatar_size), and just add appropriate class
differentiator for left/right hand side gravatar.

So the call would look like the following:

  git_get_gravatar($co->{'author_email'}, -large => 0, -position => 'left');

or

  git_get_gravatar($co->{'author_email'}, -small => 1, -padding => 'right');

or something like that[1] (-large, -small, -double_size, -inline, ...;
-position, -padding, -justify, -pad, -space, -separate, ...).

[1] Naming is unfortunately hard.  Fortunately we can change it later.

> +
>  sub git_print_authorship {
>  	my $co = shift;
>  
>  	my %ad = parse_date($co->{'author_epoch'}, $co->{'author_tz'});
>  	print "<div class=\"author_date\">" .
> +	      git_get_gravatar($co->{'author_email'}, 16, 1) .
>  	      esc_html($co->{'author_name'}) .
>  	      " [$ad{'rfc2822'}";
>  	if ($ad{'hour_local'} < 6) {

That is a very good idea to add it to git_print_authorship.  This should
take care of many other places, and many views, I think.  But you should
check if it wouldn't make gravatar appear in unwanted places.

> @@ -4145,7 +4179,7 @@ sub git_shortlog_body {
> @@ -4196,7 +4230,7 @@ sub git_history_body {
> @@ -4352,7 +4386,7 @@ sub git_search_grep_body {
> @@ -5095,8 +5129,9 @@ sub git_log {

This would really, really be easier with log-like view refactoring. This
is on my TODO list for gitweb for a quite long time...

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

-- 
Jakub Narebski
Poland

Re: [PATCHv2] gitweb: gravatar support

[Aaron Crane]

Giuseppe Bilotta writes:
> +# 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; });

This test for the availability of Digest::MD5 is broken: `use`
statements are executed at compile time, so the whole program will
fail if Digest::MD5 can't be loaded.

A possible fix would be to move the compile-time actions to run time:

    our $git_gravatar_enabled = gitweb_check_feature('gravatar') &&
         (eval { require Digest::MD5; Digest::MD5->import('md5_hex'); 1 });

However, I don't recommend doing that.  Digest::MD5 is a core module
in Perl 5.8.0 and later, so an installation of Perl 5.8 that doesn't
have it is broken.  Since gitweb.perl already needs 5.8 (because of
the `binmode STDOUT, ':utf8'` at the top, if nothing else) I see no
value in jumping through hoops to make this work in the essentially
impossible situation where Digest::MD5 is unavailable.

-- 
Aaron Crane ** http://aaroncrane.co.uk/

Re: [PATCHv2] gitweb: gravatar support

[Jakub Narebski]

On Sat, 20 June 2009, Aaron Crane wrote:
> Giuseppe Bilotta writes:

> > +# 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; });
> 
> This test for the availability of Digest::MD5 is broken: `use`
> statements are executed at compile time, so the whole program will
> fail if Digest::MD5 can't be loaded.
> 
> A possible fix would be to move the compile-time actions to run time:
> 
>     our $git_gravatar_enabled = gitweb_check_feature('gravatar') &&
>          (eval { require Digest::MD5; Digest::MD5->import('md5_hex'); 1 });
>

Good catch!!!  But we don't need import if we are to use slightly longer
form: Digest::MD5::md5_hex.
 
> However, I don't recommend doing that.  Digest::MD5 is a core module
> in Perl 5.8.0 and later, so an installation of Perl 5.8 that doesn't
> have it is broken.  Since gitweb.perl already needs 5.8 (because of
> the `binmode STDOUT, ':utf8'` at the top, if nothing else) I see no
> value in jumping through hoops to make this work in the essentially
> impossible situation where Digest::MD5 is unavailable.

In this case the problem is not with Digest::MD5 not being installed.
The problem is with not loading this module in a CGI script if it is
not required (i.e. problem is not dependencies but performance).

BTW. we sometimes unnecessary calculate MD5 hash repeatedly for the
same author / committer in log-like views... but I don't see how to
solve it in an easy way.

-- 
Jakub Narebski
Poland

Re: [PATCHv2] gitweb: gravatar support

[Jakub Narebski]

On Fri, 19 June 2009, Junio C Hamano wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

> I see these repeated patterns in your patch.
> 
> > @@ -4145,7 +4179,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, 1) . "<i>" . $author . "</i></td>\n" .
> >...
> > -		      "<td><i>" . $author . "</i></td>\n" .
> > +		      "<td>" . git_get_gravatar($co{'author_email'}, 16, 1) . "<i>" . $author . "</i></td>\n" .
> >...
> > -		      "<td><i>" . $author . "</i></td>\n" .
> > +		      "<td>" . git_get_gravatar($co{'author_email'}, 16, 1) . "<i>" . $author . "</i></td>\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'}) . "</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'}) . "</td></tr>\n";
> >...
> 
> Doesn't it strike you as needing a bit more refactoring?  The first three
> are identical, and you can refactor them to
> 
> -		      "<td><i>" . $author . "</i></td>\n" .
> +		      "<td>" . oneline_person($author) . "</td>\n" .
> 
> where oneline_person is
> 
> 	sub oneline_person {
>         	my ($me) = @_;
>                 return ($me->{'smallicon'} .
> 			"<i>" . $me->{'name_escaped'} . "</i>");
>         }
[...]

Well, gitweb certainly needs some love^W refactoring.  That is only one
of areas.  But should we postpone introducing new features till gitweb
gets cleaned up?  Although in this case I guess we can do cleanup during
introduction of new feature (which makes stronger case for factorizing
common elements, as they just get larger, those common elements, now with
this feature).

> That way, people who do not like italics, or people who want to have the
> icon at the end instead of at the beginning, can touch only one place
> (i.e. "sub oneline_person").

About using presentational HTML: I think one reason behind keeping it
is to have gitweb work also in text browsers like lynx, w3m or lynx, 
which can not understand CSS.

[...]
> In the medium term, we may want to go one step further and do
> 
>     package person;
> 
> and make sets of methods like "oneline_person".

That would be first in gitweb.

BTW. Should we split gitweb into packages? Should we split gitweb into
separate files (one package per file)?
-- 
Jakub Narebski
Poland