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

From: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
Date: Jun 27, 2009, 22:45 UTC
Message-ID: <cb7bb73a0906271545h8033f63jf0198a2f9e187428@mail.gmail.com>
In-Reply-To: <200906272145.46506.jnareb@gmail.com>

2009/6/27 Jakub Narebski <jnareb@gmail.com>:

>
> This is well written commit message.  Good work!

Thanks 8-)

Show 40 quoted lines

>> +     # 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.
>
>>
>>  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?

Show 6 quoted lines

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

Show 14 quoted lines

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

Show 15 quoted lines

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

Show 6 quoted lines

>> +     my $url = "";
>> +     if ($git_avatar eq 'gravatar') {
>> +             $url = "http://www.gravatar.com/avatar.php?gravatar_id=" .
>> +                     Digest::MD5::md5_hex(lc $email) . "&amp;size=$size";
>
> Why not use the new API[1][2]?:

Updated.

-- 
Giuseppe "Oblomov" Bilotta