Re: [PATCHv7 5/9] gitweb: (gr)avatar support

2009/6/27 Jakub Narebski [off-list ref]:

This is well written commit message.  Good work!

Thanks 8-)

quoted

+     # Avatar support. When this feature is enabled, views such as
+     # shortlog or commit will display an avatar associated with
+     # the email of the committer(s) and/or author(s).
+
+     # Currently only the gravatar provider is available, and it
+     # depends on Digest::MD5.
+
+     # To enable system wide have in $GITWEB_CONFIG
+     # $feature{'avatar'}{'default'} = ['gravatar'];
+     # To have project specific config enable override in $GITWEB_CONFIG
+     # $feature{'avatar'}{'override'} = 1;
+     # and in project config gitweb.avatar = gravatar;
+     'avatar' => {
+             'sub' => \&feature_avatar,
+             'override' => 0,
+             'default' => ['']},
 );

So you have chosen [''] and not [] as "no avatar" value, to simplify
later code.  All right.

quoted

 sub gitweb_get_feature {

@@ -433,6 +458,16 @@ sub feature_patches {

      return ($_[0]);
 }

+sub feature_avatar {
+     my @val = (git_get_project_config('avatar'));
+
+     if (@val) {
+             return @val;
+     }
+
+     return @_;
+}

Hmmm... why not simply

 +     return @val ? @val : @_;

Right. Why not?

By the way, we might want to accept 'none' instead of empty value
or no value to turn off avatar support for specific project (if
avatars are turned on globally, and project specific override is on).
We use this technique for 'snapshot' feature.

But this isn't terribly important.

Well, since 'none' is not a known provider, that trick should work
correctly anyway.

quoted

+our ($git_avatar) = gitweb_get_feature('avatar');
+if ($git_avatar eq 'gravatar') {
+     $git_avatar = '' unless (eval { require Digest::MD5; 1; });
+} else {
+     $git_avatar = '';
+}

Thoughts for the future: this can lead to not very pretty if-elsif
chain.  We have replaces such chain for selecting action (for dispatch)
by using %actions hash of subroutine refs (as a kind of 'switch'/'given'
statement).  We could do the same for avatar provider initialization
and validation subroutines.

But for now it is clear enough.  Don't worry about this issue now.

I'm also not terribly sure about how to implement _this_ through hash
calls. But I did consider the issue (how I wish Perl had an actual
switch statement ...)

Running t9500-gitweb-standalone-no-errors.sh with --debug option shows
failing of 15 among 87 tests, with error:

  gitweb.perl: Use of uninitialized value in hash element at gitweb/gitweb.perl line 1524.

which is the above line.  This line should read:

 +     my $size = exists $opts{'size'}
 +             ? $avatar_size{$opts{'size'}} || $avatar_size{'default'}
 +             : $avatar_size{'default'};

or something like that, e.g.:

 +     my $size = $avatar_size{ defined $opts{'size'} ? $opts{'size'} : 'default' }
 +             || $avatar_size{'default'};

I did $opts{-size} ||= 'default'. Or is it bad form to modify the
passed option hash?

BTW. didn't we agree on '-size' and '-pad_before' convention?

Yes we did. I don't know why it went back to 'size' etc. Must have
messed up something with the patchsets.

quoted

+     my $url = "";
+     if ($git_avatar eq 'gravatar') {
+             $url = "http://www.gravatar.com/avatar.php?gravatar_id=" .
+                     Digest::MD5::md5_hex(lc $email) . "&size=$size";

Why not use the new API[1][2]?:

Updated.

-- 
Giuseppe "Oblomov" Bilotta