2009/6/27 Jakub Narebski <jnareb@xxxxxxxxx>:
>
> This is well written commit message. Good work!
Thanks 8-)
>> + # 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?
> 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.
>> +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.
>> + 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html