Re: [PATCHv2] gitweb: gravatar support

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 19, 2009, 20:28 UTC
Message-ID: <7v63esklxh.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1245435670-5688-1-git-send-email-giuseppe.bilotta@gmail.com>

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

Show 14 quoted lines

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

Show 21 quoted lines

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

Show 5 quoted lines

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