git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCHv8 0/7] gitweb: avatar support
@ 2009-06-29 22:00 Giuseppe Bilotta
  2009-06-29 22:00 ` [PATCHv8 1/7] gitweb: refactor author name insertion Giuseppe Bilotta
  0 siblings, 1 reply; 22+ messages in thread
From: Giuseppe Bilotta @ 2009-06-29 22:00 UTC (permalink / raw
  To: git; +Cc: Jakub Narebski, Junio C Hamano, Petr Baudis, Giuseppe Bilotta

8th iteration of my attempt to implement avatar support for gitweb. Only
minor changes from the previous version: the removal of a couple of
patches that need to be discussed some more, and some tune-up suggested
by Jakub.

Giuseppe Bilotta (7):
  gitweb: refactor author name insertion
  gitweb: uniform author info for commit and commitdiff
  gitweb: use git_print_authorship_rows in 'tag' view too
  gitweb: (gr)avatar support
  gitweb: gravatar url cache
  gitweb: picon avatar provider
  gitweb: add empty alt text to avatar img

 gitweb/gitweb.css                      |    9 +-
 gitweb/gitweb.perl                     |  231 +++++++++++++++++++++++++------
 t/t9500-gitweb-standalone-no-errors.sh |    2 +
 3 files changed, 196 insertions(+), 46 deletions(-)

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCHv8 1/7] gitweb: refactor author name insertion
  2009-06-29 22:00 [PATCHv8 0/7] gitweb: avatar support Giuseppe Bilotta
@ 2009-06-29 22:00 ` Giuseppe Bilotta
  2009-06-29 22:00   ` [PATCHv8 2/7] gitweb: uniform author info for commit and commitdiff Giuseppe Bilotta
  2009-06-30 20:04   ` [PATCHv8 1/7] gitweb: refactor author name insertion Jakub Narebski
  0 siblings, 2 replies; 22+ messages in thread
From: Giuseppe Bilotta @ 2009-06-29 22:00 UTC (permalink / raw
  To: git; +Cc: Jakub Narebski, Junio C Hamano, Petr Baudis, Giuseppe Bilotta

Collect all author display code in appropriate functions, making it
easier to extend these functions on the CGI side.

We also move some of the presentation code from hard-coded HTML to CSS,
for easier customization.

A side effect of the refactoring is that now localtime is always
displayed with the 'at night' warning.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.css  |    5 ++-
 gitweb/gitweb.perl |   93 ++++++++++++++++++++++++++++++---------------------
 2 files changed, 59 insertions(+), 39 deletions(-)

diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index a01eac8..68b22ff 100644
--- a/gitweb/gitweb.css
+++ b/gitweb/gitweb.css
@@ -132,11 +132,14 @@ div.list_head {
 	font-style: italic;
 }
 
+.author_date, .author {
+	font-style: italic;
+}
+
 div.author_date {
 	padding: 8px;
 	border: solid #d9d8d1;
 	border-width: 0px 0px 1px 0px;
-	font-style: italic;
 }
 
 a.list {
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 1e7e2d8..7fd53f6 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1469,6 +1469,16 @@ sub format_subject_html {
 	}
 }
 
+# format the author name of the given commit with the given tag
+# the author name is chopped and escaped according to the other
+# optional parameters (see chop_str).
+sub format_author_html {
+	my $tag = shift;
+	my $co = shift;
+	my $author = chop_and_escape_str($co->{'author_name'}, @_);
+	return "<$tag class=\"author\">" . $author . "</$tag>";
+}
+
 # format git diff header line, i.e. "diff --(git|combined|cc) ..."
 sub format_git_diff_header_line {
 	my $line = shift;
@@ -3214,21 +3224,50 @@ sub git_print_header_div {
 	      "\n</div>\n";
 }
 
+sub print_local_time {
+	my %date = @_;
+	if ($date{'hour_local'} < 6) {
+		printf(" (<span class=\"atnight\">%02d:%02d</span> %s)",
+			$date{'hour_local'}, $date{'minute_local'}, $date{'tz_local'});
+	} else {
+		printf(" (%02d:%02d %s)",
+			$date{'hour_local'}, $date{'minute_local'}, $date{'tz_local'});
+	}
+}
+
+# Outputs the author name and date in long form
 sub git_print_authorship {
 	my $co = shift;
+	my %opts = @_;
+	my $tag = $opts{-tag} || 'div';
 
 	my %ad = parse_date($co->{'author_epoch'}, $co->{'author_tz'});
-	print "<div class=\"author_date\">" .
+	print "<$tag class=\"author_date\">" .
 	      esc_html($co->{'author_name'}) .
 	      " [$ad{'rfc2822'}";
-	if ($ad{'hour_local'} < 6) {
-		printf(" (<span class=\"atnight\">%02d:%02d</span> %s)",
-		       $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
-	} else {
-		printf(" (%02d:%02d %s)",
-		       $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
+	print_local_time(%ad) if ($opts{-localtime});
+	print "]</$tag>\n";
+}
+
+# Outputs table rows containing the full author or committer information,
+# in the format expected for 'commit' view (& similia).
+# Parameters are a commit hash reference, followed by the list of people
+# to output information for. If the list is empty it defalts to both
+# author and committer.
+sub git_print_authorship_rows {
+	my $co = shift;
+	# too bad we can't use @people = @_ || ('author', 'committer')
+	my @people = @_;
+	@people = ('author', 'committer') unless @people;
+	foreach my $who (@people) {
+		my %wd = parse_date($co->{"${who}_epoch"}, $co->{"${who}_tz"});
+		print "<tr><td>$who</td><td>" . esc_html($co->{$who}) . "</td></tr>\n".
+		      "<tr>" .
+		      "<td></td><td> $wd{'rfc2822'}";
+		print_local_time(%wd);
+		print "</td>" .
+		      "</tr>\n";
 	}
-	print "]</div>\n";
 }
 
 sub git_print_page_path {
@@ -4142,11 +4181,9 @@ sub git_shortlog_body {
 			print "<tr class=\"light\">\n";
 		}
 		$alternate ^= 1;
-		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>";
+		      format_author_html('td', \%co, 10) . "<td>";
 		print format_subject_html($co{'title'}, $co{'title_short'},
 		                          href(action=>"commit", hash=>$commit), $ref);
 		print "</td>\n" .
@@ -4193,11 +4230,9 @@ sub git_history_body {
 			print "<tr class=\"light\">\n";
 		}
 		$alternate ^= 1;
-	# 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>";
+	# shortlog:   format_author_html('td', \%co, 10)
+		      format_author_html('td', \%co, 15, 3) . "<td>";
 		# originally git_history used chop_str($co{'title'}, 50)
 		print format_subject_html($co{'title'}, $co{'title_short'},
 		                          href(action=>"commit", hash=>$commit), $ref);
@@ -4350,9 +4385,8 @@ sub git_search_grep_body {
 			print "<tr class=\"light\">\n";
 		}
 		$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" .
+		      format_author_html('td', \%co, 15, 5) .
 		      "<td>" .
 		      $cgi->a({-href => href(action=>"commit", hash=>$co{'id'}),
 		               -class => "list subject"},
@@ -5094,9 +5128,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";
+		      git_print_authorship(\%co, -tag => 'span');
+		      print "<br/>\n</div>\n";
 
 		print "<div class=\"log_body\">\n";
 		git_print_log($co{'comment'}, -final_empty_line=> 1);
@@ -5115,8 +5149,6 @@ sub git_commit {
 	$hash ||= $hash_base || "HEAD";
 	my %co = parse_commit($hash)
 	    or die_error(404, "Unknown commit object");
-	my %ad = parse_date($co{'author_epoch'}, $co{'author_tz'});
-	my %cd = parse_date($co{'committer_epoch'}, $co{'committer_tz'});
 
 	my $parent  = $co{'parent'};
 	my $parents = $co{'parents'}; # listref
@@ -5183,22 +5215,7 @@ 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".
-	      "<tr>" .
-	      "<td></td><td> $ad{'rfc2822'}";
-	if ($ad{'hour_local'} < 6) {
-		printf(" (<span class=\"atnight\">%02d:%02d</span> %s)",
-		       $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
-	} else {
-		printf(" (%02d:%02d %s)",
-		       $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
-	}
-	print "</td>" .
-	      "</tr>\n";
-	print "<tr><td>committer</td><td>" . esc_html($co{'committer'}) . "</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";
+	git_print_authorship_rows(\%co);
 	print "<tr><td>commit</td><td class=\"sha1\">$co{'id'}</td></tr>\n";
 	print "<tr>" .
 	      "<td>tree</td>" .
@@ -5579,7 +5596,7 @@ sub git_commitdiff {
 		git_header_html(undef, $expires);
 		git_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash, $formats_nav);
 		git_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash);
-		git_print_authorship(\%co);
+		git_print_authorship(\%co, -localtime => 1);
 		print "<div class=\"page_body\">\n";
 		if (@{$co{'comment'}} > 1) {
 			print "<div class=\"log\">\n";
-- 
1.6.3.rc1.192.gdbfcb

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCHv8 2/7] gitweb: uniform author info for commit and commitdiff
  2009-06-29 22:00 ` [PATCHv8 1/7] gitweb: refactor author name insertion Giuseppe Bilotta
@ 2009-06-29 22:00   ` Giuseppe Bilotta
  2009-06-29 22:00     ` [PATCHv8 3/7] gitweb: use git_print_authorship_rows in 'tag' view too Giuseppe Bilotta
  2009-06-30 20:04     ` [PATCHv8 2/7] gitweb: uniform author info for commit and commitdiff Jakub Narebski
  2009-06-30 20:04   ` [PATCHv8 1/7] gitweb: refactor author name insertion Jakub Narebski
  1 sibling, 2 replies; 22+ messages in thread
From: Giuseppe Bilotta @ 2009-06-29 22:00 UTC (permalink / raw
  To: git; +Cc: Jakub Narebski, Junio C Hamano, Petr Baudis, Giuseppe Bilotta

Switch from 'log'-like layout

    A U Thor <email@example.com> [date time]

to 'commit'-like layout

    author    A U Thor <email@example.com>
              date time
    committer C O Mitter <other.email@example.com>
              committer date time

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 7fd53f6..9a8d775 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5596,7 +5596,11 @@ sub git_commitdiff {
 		git_header_html(undef, $expires);
 		git_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash, $formats_nav);
 		git_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash);
-		git_print_authorship(\%co, -localtime => 1);
+		print "<div class=\"title_text\">\n" .
+		      "<table class=\"object_header\">\n";
+		git_print_authorship_rows(\%co);
+		print "</table>".
+		      "</div>\n";
 		print "<div class=\"page_body\">\n";
 		if (@{$co{'comment'}} > 1) {
 			print "<div class=\"log\">\n";
-- 
1.6.3.rc1.192.gdbfcb

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCHv8 3/7] gitweb: use git_print_authorship_rows in 'tag' view too
  2009-06-29 22:00   ` [PATCHv8 2/7] gitweb: uniform author info for commit and commitdiff Giuseppe Bilotta
@ 2009-06-29 22:00     ` Giuseppe Bilotta
  2009-06-29 22:00       ` [PATCHv8 4/7] gitweb: (gr)avatar support Giuseppe Bilotta
  2009-06-30 20:10       ` [PATCHv8 3/7] gitweb: use git_print_authorship_rows in 'tag' view too Jakub Narebski
  2009-06-30 20:04     ` [PATCHv8 2/7] gitweb: uniform author info for commit and commitdiff Jakub Narebski
  1 sibling, 2 replies; 22+ messages in thread
From: Giuseppe Bilotta @ 2009-06-29 22:00 UTC (permalink / raw
  To: git; +Cc: Jakub Narebski, Junio C Hamano, Petr Baudis, Giuseppe Bilotta

parse_tag must be adapted to use the hash keys expected by
git_print_authorship_rows. This is not a problem since git_tag is the
only user of this sub.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 9a8d775..a393ac6 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2409,8 +2409,14 @@ sub parse_tag {
 			$tag{'name'} = $1;
 		} elsif ($line =~ m/^tagger (.*) ([0-9]+) (.*)$/) {
 			$tag{'author'} = $1;
-			$tag{'epoch'} = $2;
-			$tag{'tz'} = $3;
+			$tag{'author_epoch'} = $2;
+			$tag{'author_tz'} = $3;
+			if ($tag{'author'} =~ m/^([^<]+) <([^>]*)>/) {
+				$tag{'author_name'}  = $1;
+				$tag{'author_email'} = $2;
+			} else {
+				$tag{'author_name'} = $tag{'author'};
+			}
 		} elsif ($line =~ m/--BEGIN/) {
 			push @comment, $line;
 			last;
@@ -4620,11 +4626,7 @@ sub git_tag {
 	                                      $tag{'type'}) . "</td>\n" .
 	      "</tr>\n";
 	if (defined($tag{'author'})) {
-		my %ad = parse_date($tag{'epoch'}, $tag{'tz'});
-		print "<tr><td>author</td><td>" . esc_html($tag{'author'}) . "</td></tr>\n";
-		print "<tr><td></td><td>" . $ad{'rfc2822'} .
-			sprintf(" (%02d:%02d %s)", $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'}) .
-			"</td></tr>\n";
+		git_print_authorship_rows(\%tag, 'author');
 	}
 	print "</table>\n\n" .
 	      "</div>\n";
-- 
1.6.3.rc1.192.gdbfcb

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCHv8 4/7] gitweb: (gr)avatar support
  2009-06-29 22:00     ` [PATCHv8 3/7] gitweb: use git_print_authorship_rows in 'tag' view too Giuseppe Bilotta
@ 2009-06-29 22:00       ` Giuseppe Bilotta
  2009-06-29 22:00         ` [PATCHv8 5/7] gitweb: gravatar url cache Giuseppe Bilotta
  2009-06-30 20:16         ` [PATCHv8 4/7] gitweb: (gr)avatar support Jakub Narebski
  2009-06-30 20:10       ` [PATCHv8 3/7] gitweb: use git_print_authorship_rows in 'tag' view too Jakub Narebski
  1 sibling, 2 replies; 22+ messages in thread
From: Giuseppe Bilotta @ 2009-06-29 22:00 UTC (permalink / raw
  To: git; +Cc: Jakub Narebski, Junio C Hamano, Petr Baudis, Giuseppe Bilotta

Introduce avatar support: the feature adds the appropriate img tag next
to author and committer in commit(diff), history, shortlog, log and tag
views. Multiple avatar providers are possible, but only gravatar is
implemented at the moment.

Gravatar support depends on Digest::MD5, which is a core package since
Perl 5.8. If gravatars are activated but Digest::MD5 cannot be found,
the feature will be automatically disabled.

No avatar provider is selected by default, except in the t9500 test.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.css                      |    4 ++
 gitweb/gitweb.perl                     |   83 ++++++++++++++++++++++++++++++-
 t/t9500-gitweb-standalone-no-errors.sh |    2 +
 3 files changed, 86 insertions(+), 3 deletions(-)

diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index 68b22ff..d05bc37 100644
--- a/gitweb/gitweb.css
+++ b/gitweb/gitweb.css
@@ -28,6 +28,10 @@ img.logo {
 	border-width: 0px;
 }
 
+img.avatar {
+	vertical-align: middle;
+}
+
 div.page_header {
 	height: 25px;
 	padding: 8px;
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index a393ac6..92695a3 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -195,6 +195,14 @@ our %known_snapshot_format_aliases = (
 	'x-zip' => undef, '' => undef,
 );
 
+# Pixel sizes for icons and avatars. If the default font sizes or lineheights
+# are changed, it may be appropriate to change these values too via
+# $GITWEB_CONFIG.
+our %avatar_size = (
+	'default' => 16,
+	'double'  => 32
+);
+
 # You define site-wide feature defaults here; override them with
 # $GITWEB_CONFIG as necessary.
 our %feature = (
@@ -365,6 +373,24 @@ our %feature = (
 		'sub' => \&feature_patches,
 		'override' => 0,
 		'default' => [16]},
+
+	# 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. If an unknown provider is specified,
+	# the feature is disabled.
+
+	# 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' => ['']},
 );
 
 sub gitweb_get_feature {
@@ -433,6 +459,12 @@ sub feature_patches {
 	return ($_[0]);
 }
 
+sub feature_avatar {
+	my @val = (git_get_project_config('avatar'));
+
+	return @val ? @val : @_;
+}
+
 # checking HEAD file with -e is fragile if the repository was
 # initialized long time ago (i.e. symlink HEAD) and was pack-ref'ed
 # and then pruned.
@@ -814,6 +846,17 @@ $git_dir = "$projectroot/$project" if $project;
 our @snapshot_fmts = gitweb_get_feature('snapshot');
 @snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);
 
+# check that the avatar feature is set to a known provider name,
+# and for each provider check if the dependencies are satisfied.
+# if the provider name is invalid or the dependencies are not met,
+# reset $git_avatar to the empty string.
+our ($git_avatar) = gitweb_get_feature('avatar');
+if ($git_avatar eq 'gravatar') {
+	$git_avatar = '' unless (eval { require Digest::MD5; 1; });
+} else {
+	$git_avatar = '';
+}
+
 # dispatch
 if (!defined $action) {
 	if (defined $hash) {
@@ -1469,6 +1512,34 @@ sub format_subject_html {
 	}
 }
 
+# Insert an avatar for the given $email at the given $size if the feature
+# is enabled.
+sub git_get_avatar {
+	my ($email, %opts) = @_;
+	my $pre_white  = ($opts{-pad_before} ? "&nbsp;" : "");
+	my $post_white = ($opts{-pad_after}  ? "&nbsp;" : "");
+	$opts{-size} ||= 'default';
+	my $size = $avatar_size{$opts{-size}} || $avatar_size{'default'};
+	my $url = "";
+	if ($git_avatar eq 'gravatar') {
+		$url = "http://www.gravatar.com/avatar/" .
+			Digest::MD5::md5_hex(lc $email) . "?s=$size";
+	}
+	# Currently only gravatars are supported, but other forms such as
+	# picons can be added by putting an else up here and defining $url
+	# as needed. If no variant puts something in $url, we assume avatars
+	# are completely disabled/unavailable.
+	if ($url) {
+		return $pre_white .
+		       "<img width=\"$size\" " .
+		            "class=\"avatar\" " .
+		            "src=\"$url\" " .
+		       "/>" . $post_white;
+	} else {
+		return "";
+	}
+}
+
 # format the author name of the given commit with the given tag
 # the author name is chopped and escaped according to the other
 # optional parameters (see chop_str).
@@ -1476,7 +1547,9 @@ sub format_author_html {
 	my $tag = shift;
 	my $co = shift;
 	my $author = chop_and_escape_str($co->{'author_name'}, @_);
-	return "<$tag class=\"author\">" . $author . "</$tag>";
+	return "<$tag class=\"author\">" .
+	       git_get_avatar($co->{'author_email'}, -pad_after => 1) .
+	       $author . "</$tag>";
 }
 
 # format git diff header line, i.e. "diff --(git|combined|cc) ..."
@@ -3252,7 +3325,8 @@ sub git_print_authorship {
 	      esc_html($co->{'author_name'}) .
 	      " [$ad{'rfc2822'}";
 	print_local_time(%ad) if ($opts{-localtime});
-	print "]</$tag>\n";
+	print "]" . git_get_avatar($co->{'author_email'}, -pad_before => 1)
+		  . "</$tag>\n";
 }
 
 # Outputs table rows containing the full author or committer information,
@@ -3267,7 +3341,10 @@ sub git_print_authorship_rows {
 	@people = ('author', 'committer') unless @people;
 	foreach my $who (@people) {
 		my %wd = parse_date($co->{"${who}_epoch"}, $co->{"${who}_tz"});
-		print "<tr><td>$who</td><td>" . esc_html($co->{$who}) . "</td></tr>\n".
+		print "<tr><td>$who</td><td>" . esc_html($co->{$who}) . "</td>" .
+		      "<td rowspan=\"2\">" .
+		      git_get_avatar($co->{"${who}_email"}, -size => 'double') .
+		      "</td></tr>\n" .
 		      "<tr>" .
 		      "<td></td><td> $wd{'rfc2822'}";
 		print_local_time(%wd);
diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh
index d539619..6275181 100755
--- a/t/t9500-gitweb-standalone-no-errors.sh
+++ b/t/t9500-gitweb-standalone-no-errors.sh
@@ -660,6 +660,7 @@ cat >>gitweb_config.perl <<EOF
 
 \$feature{'blame'}{'override'} = 1;
 \$feature{'snapshot'}{'override'} = 1;
+\$feature{'avatar'}{'override'} = 1;
 EOF
 
 test_expect_success \
@@ -671,6 +672,7 @@ test_expect_success \
 	'config override: tree view, features disabled in repo config' \
 	'git config gitweb.blame no &&
 	 git config gitweb.snapshot none &&
+	 git config gitweb.avatar gravatar &&
 	 gitweb_run "p=.git;a=tree"'
 test_debug 'cat gitweb.log'
 
-- 
1.6.3.rc1.192.gdbfcb

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCHv8 5/7] gitweb: gravatar url cache
  2009-06-29 22:00       ` [PATCHv8 4/7] gitweb: (gr)avatar support Giuseppe Bilotta
@ 2009-06-29 22:00         ` Giuseppe Bilotta
  2009-06-29 22:00           ` [PATCHv8 6/7] gitweb: picon avatar provider Giuseppe Bilotta
                             ` (2 more replies)
  2009-06-30 20:16         ` [PATCHv8 4/7] gitweb: (gr)avatar support Jakub Narebski
  1 sibling, 3 replies; 22+ messages in thread
From: Giuseppe Bilotta @ 2009-06-29 22:00 UTC (permalink / raw
  To: git; +Cc: Jakub Narebski, Junio C Hamano, Petr Baudis, Giuseppe Bilotta

Views which contain many occurrences of the same email address (e.g.
shortlog view) benefit from not having to recalculate the MD5 of the
email address every time.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |   24 ++++++++++++++++++++++--
 1 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 92695a3..4d7e4ff 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1512,6 +1512,27 @@ sub format_subject_html {
 	}
 }
 
+# Rather than recomputing the url for an email multiple times, we cache it
+# after the first hit. This gives a visible benefit in views where the avatar
+# for the same email is used repeatedly (e.g. shortlog).
+# The cache is shared by all avatar engines (currently gravatar only), which
+# are free to use it as preferred. Since only one avatar engine is used for any
+# given page, there's no risk for cache conflicts.
+our %avatar_cache = ();
+
+# Compute the gravatar url for a given email, if it's not in the cache already.
+# Gravatar stores only the part of the URL before the size, since that's the
+# one computationally more expensive. This also allows reuse of the cache for
+# different sizes (for this particular engine).
+sub gravatar_url {
+	my $email = lc shift;
+	my $size = shift;
+	$avatar_cache{$email} ||=
+		"http://www.gravatar.com/avatar/" .
+			Digest::MD5::md5_hex($email) . "?s=";
+	return $avatar_cache{$email} . $size;
+}
+
 # Insert an avatar for the given $email at the given $size if the feature
 # is enabled.
 sub git_get_avatar {
@@ -1522,8 +1543,7 @@ sub git_get_avatar {
 	my $size = $avatar_size{$opts{-size}} || $avatar_size{'default'};
 	my $url = "";
 	if ($git_avatar eq 'gravatar') {
-		$url = "http://www.gravatar.com/avatar/" .
-			Digest::MD5::md5_hex(lc $email) . "?s=$size";
+		$url = gravatar_url($email, $size);
 	}
 	# Currently only gravatars are supported, but other forms such as
 	# picons can be added by putting an else up here and defining $url
-- 
1.6.3.rc1.192.gdbfcb

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCHv8 6/7] gitweb: picon avatar provider
  2009-06-29 22:00         ` [PATCHv8 5/7] gitweb: gravatar url cache Giuseppe Bilotta
@ 2009-06-29 22:00           ` Giuseppe Bilotta
  2009-06-29 22:00             ` [PATCHv8 7/7] gitweb: add empty alt text to avatar img Giuseppe Bilotta
  2009-06-30 20:23             ` [PATCHv8 6/7] gitweb: picon avatar provider Jakub Narebski
  2009-06-30 20:18           ` [PATCHv8 5/7] gitweb: gravatar url cache Jakub Narebski
  2009-06-30 20:38           ` Junio C Hamano
  2 siblings, 2 replies; 22+ messages in thread
From: Giuseppe Bilotta @ 2009-06-29 22:00 UTC (permalink / raw
  To: git; +Cc: Jakub Narebski, Junio C Hamano, Petr Baudis, Giuseppe Bilotta

Simple implementation of picon that only relies on the indiana.edu
database.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |   34 +++++++++++++++++++++++++++-------
 1 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 4d7e4ff..862ea99 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -378,15 +378,18 @@ our %feature = (
 	# 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. If an unknown provider is specified,
-	# the feature is disabled.
+	# Currently available providers are gravatar and picon.
+	# If an unknown provider is specified, the feature is disabled.
+
+	# Gravatar depends on Digest::MD5.
+	# Picon currently relies on the indiana.edu database.
 
 	# To enable system wide have in $GITWEB_CONFIG
-	# $feature{'avatar'}{'default'} = ['gravatar'];
+	# $feature{'avatar'}{'default'} = ['<provider>'];
+	# where <provider> is either gravatar or picon.
 	# To have project specific config enable override in $GITWEB_CONFIG
 	# $feature{'avatar'}{'override'} = 1;
-	# and in project config gitweb.avatar = gravatar;
+	# and in project config gitweb.avatar = <provider>;
 	'avatar' => {
 		'sub' => \&feature_avatar,
 		'override' => 0,
@@ -853,6 +856,8 @@ our @snapshot_fmts = gitweb_get_feature('snapshot');
 our ($git_avatar) = gitweb_get_feature('avatar');
 if ($git_avatar eq 'gravatar') {
 	$git_avatar = '' unless (eval { require Digest::MD5; 1; });
+} elsif ($git_avatar eq 'picon') {
+	# no dependencies
 } else {
 	$git_avatar = '';
 }
@@ -1520,6 +1525,20 @@ sub format_subject_html {
 # given page, there's no risk for cache conflicts.
 our %avatar_cache = ();
 
+# Compute the picon url for a given email, by using the picon search service over at
+# http://www.cs.indiana.edu/picons/search.html
+sub picon_url {
+	my $email = lc shift;
+	if (!$avatar_cache{$email}) {
+		my ($user, $domain) = split('@', $email);
+		$avatar_cache{$email} =
+			"http://www.cs.indiana.edu/cgi-pub/kinzler/piconsearch.cgi/" .
+			"$domain/$user/" .
+			"users+domains+unknown/up/single";
+	}
+	return $avatar_cache{$email};
+}
+
 # Compute the gravatar url for a given email, if it's not in the cache already.
 # Gravatar stores only the part of the URL before the size, since that's the
 # one computationally more expensive. This also allows reuse of the cache for
@@ -1544,9 +1563,10 @@ sub git_get_avatar {
 	my $url = "";
 	if ($git_avatar eq 'gravatar') {
 		$url = gravatar_url($email, $size);
+	} elsif ($git_avatar eq 'picon') {
+		$url = picon_url($email);
 	}
-	# Currently only gravatars are supported, but other forms such as
-	# picons can be added by putting an else up here and defining $url
+	# Other providers can be added by extending the if chain, defining $url
 	# as needed. If no variant puts something in $url, we assume avatars
 	# are completely disabled/unavailable.
 	if ($url) {
-- 
1.6.3.rc1.192.gdbfcb

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCHv8 7/7] gitweb: add empty alt text to avatar img
  2009-06-29 22:00           ` [PATCHv8 6/7] gitweb: picon avatar provider Giuseppe Bilotta
@ 2009-06-29 22:00             ` Giuseppe Bilotta
  2009-06-30 20:29               ` Jakub Narebski
  2009-06-30 20:23             ` [PATCHv8 6/7] gitweb: picon avatar provider Jakub Narebski
  1 sibling, 1 reply; 22+ messages in thread
From: Giuseppe Bilotta @ 2009-06-29 22:00 UTC (permalink / raw
  To: git; +Cc: Jakub Narebski, Junio C Hamano, Petr Baudis, Giuseppe Bilotta

The empty alt text optimizes screen estate in text-only browsers.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 862ea99..6a1b5b5 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1574,6 +1574,7 @@ sub git_get_avatar {
 		       "<img width=\"$size\" " .
 		            "class=\"avatar\" " .
 		            "src=\"$url\" " .
+			    "alt=\"\" " .
 		       "/>" . $post_white;
 	} else {
 		return "";
-- 
1.6.3.rc1.192.gdbfcb

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCHv8 1/7] gitweb: refactor author name insertion
  2009-06-29 22:00 ` [PATCHv8 1/7] gitweb: refactor author name insertion Giuseppe Bilotta
  2009-06-29 22:00   ` [PATCHv8 2/7] gitweb: uniform author info for commit and commitdiff Giuseppe Bilotta
@ 2009-06-30 20:04   ` Jakub Narebski
  2009-06-30 20:23     ` Giuseppe Bilotta
  1 sibling, 1 reply; 22+ messages in thread
From: Jakub Narebski @ 2009-06-30 20:04 UTC (permalink / raw
  To: Giuseppe Bilotta; +Cc: git, Junio C Hamano, Petr Baudis

On Tue, 30 Jun 2009, Giuseppe Bilotta wrote:

> Collect all author display code in appropriate functions, making it
> easier to extend these functions on the CGI side.
> 
> We also move some of the presentation code from hard-coded HTML to CSS,
> for easier customization.
> 
> A side effect of the refactoring is that now localtime is always
> displayed with the 'at night' warning.

It is a very nice side effect, I think.

> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

Acked-by: Jakub Narebski <jnareb@gmail.com>

> ---
>  gitweb/gitweb.css  |    5 ++-
>  gitweb/gitweb.perl |   93 ++++++++++++++++++++++++++++++---------------------
>  2 files changed, 59 insertions(+), 39 deletions(-)

[...]  
> +sub print_local_time {
> +	my %date = @_;
> +	if ($date{'hour_local'} < 6) {
> +		printf(" (<span class=\"atnight\">%02d:%02d</span> %s)",
> +			$date{'hour_local'}, $date{'minute_local'}, $date{'tz_local'});
> +	} else {
> +		printf(" (%02d:%02d %s)",
> +			$date{'hour_local'}, $date{'minute_local'}, $date{'tz_local'});
> +	}
> +}

Very nice refactoring.  

It could do with a comment describing its output, but it is not
necessary, and IMHO not worth resend.  We can always add it "in tree".

-- 
Jakub Narebski
Poland

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCHv8 2/7] gitweb: uniform author info for commit and commitdiff
  2009-06-29 22:00   ` [PATCHv8 2/7] gitweb: uniform author info for commit and commitdiff Giuseppe Bilotta
  2009-06-29 22:00     ` [PATCHv8 3/7] gitweb: use git_print_authorship_rows in 'tag' view too Giuseppe Bilotta
@ 2009-06-30 20:04     ` Jakub Narebski
  1 sibling, 0 replies; 22+ messages in thread
From: Jakub Narebski @ 2009-06-30 20:04 UTC (permalink / raw
  To: Giuseppe Bilotta; +Cc: git, Junio C Hamano, Petr Baudis

On Tue, 30 Jun 2009, Giuseppe Bilotta wrote:

> Switch from 'log'-like layout
> 
>     A U Thor <email@example.com> [date time]
> 
> to 'commit'-like layout
> 
>     author    A U Thor <email@example.com>
>               date time
>     committer C O Mitter <other.email@example.com>
>               committer date time
> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

I am (as I have said earlier) a bit ambivalent about this issue.
But I like this patch from technical point of view.

I also like the fact that this feature is put in separate patch,
so it can be accepted or rejected (or reverted) independently.
Good work on refactoring!

Techically-Acked-By: Jakub Narebski <jnareb@gmail.com>

> ---
>  gitweb/gitweb.perl |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 7fd53f6..9a8d775 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -5596,7 +5596,11 @@ sub git_commitdiff {
>  		git_header_html(undef, $expires);
>  		git_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash, 
$formats_nav);
>  		git_print_header_div('commit', esc_html($co{'title'}) . $ref, 
$hash);
> -		git_print_authorship(\%co, -localtime => 1);
> +		print "<div class=\"title_text\">\n" .
> +		      "<table class=\"object_header\">\n";
> +		git_print_authorship_rows(\%co);
> +		print "</table>".
> +		      "</div>\n";
>  		print "<div class=\"page_body\">\n";
>  		if (@{$co{'comment'}} > 1) {
>  			print "<div class=\"log\">\n";
> -- 
> 1.6.3.rc1.192.gdbfcb
> 
> 

-- 
Jakub Narebski
Poland

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCHv8 3/7] gitweb: use git_print_authorship_rows in 'tag' view too
  2009-06-29 22:00     ` [PATCHv8 3/7] gitweb: use git_print_authorship_rows in 'tag' view too Giuseppe Bilotta
  2009-06-29 22:00       ` [PATCHv8 4/7] gitweb: (gr)avatar support Giuseppe Bilotta
@ 2009-06-30 20:10       ` Jakub Narebski
  1 sibling, 0 replies; 22+ messages in thread
From: Jakub Narebski @ 2009-06-30 20:10 UTC (permalink / raw
  To: Giuseppe Bilotta; +Cc: git, Junio C Hamano, Petr Baudis

On Tue, 30 Jun 2009, Giuseppe Bilotta wrote:

> parse_tag must be adapted to use the hash keys expected by
> git_print_authorship_rows. This is not a problem since git_tag is the
> only user of this sub.
> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

Very nice unification (of hash key names for parse_tag and
parse_commit_text), and nice refactoring of common code.

Acked-by: Jakub Narebski <jnareb@gmail.com>

[...]
> @@ -2409,8 +2409,14 @@ sub parse_tag {
>  			$tag{'name'} = $1;
>  		} elsif ($line =~ m/^tagger (.*) ([0-9]+) (.*)$/) {
>  			$tag{'author'} = $1;
> -			$tag{'epoch'} = $2;
> -			$tag{'tz'} = $3;
> +			$tag{'author_epoch'} = $2;
> +			$tag{'author_tz'} = $3;
> +			if ($tag{'author'} =~ m/^([^<]+) <([^>]*)>/) {
> +				$tag{'author_name'}  = $1;
> +				$tag{'author_email'} = $2;
> +			} else {
> +				$tag{'author_name'} = $tag{'author'};
> +			}

Unrelated sidenote: I wonder if it would work correctly on
malformed 'tagger' headers which sometimes happen... Probably
not.  But this is not an issue with this patch, but with gitweb
in general.

-- 
Jakub Narebski
Poland

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCHv8 4/7] gitweb: (gr)avatar support
  2009-06-29 22:00       ` [PATCHv8 4/7] gitweb: (gr)avatar support Giuseppe Bilotta
  2009-06-29 22:00         ` [PATCHv8 5/7] gitweb: gravatar url cache Giuseppe Bilotta
@ 2009-06-30 20:16         ` Jakub Narebski
  1 sibling, 0 replies; 22+ messages in thread
From: Jakub Narebski @ 2009-06-30 20:16 UTC (permalink / raw
  To: Giuseppe Bilotta; +Cc: git, Junio C Hamano, Petr Baudis

On Tue, 30 Jun 2009, Giuseppe Bilotta wrote:

> Introduce avatar support: the feature adds the appropriate img tag next
> to author and committer in commit(diff), history, shortlog, log and tag
> views. Multiple avatar providers are possible, but only gravatar is
> implemented at the moment.
> 
> Gravatar support depends on Digest::MD5, which is a core package since
> Perl 5.8. If gravatars are activated but Digest::MD5 cannot be found,
> the feature will be automatically disabled.
> 
> No avatar provider is selected by default, except in the t9500 test.
> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

I like both commit message and comprehensive comments.

Acked-by: Jakub Narebski <jnareb@gmail.com>

[...]
> +# Insert an avatar for the given $email at the given $size if the feature
> +# is enabled.
> +sub git_get_avatar {
> +	my ($email, %opts) = @_;
> +	my $pre_white  = ($opts{-pad_before} ? "&nbsp;" : "");
> +	my $post_white = ($opts{-pad_after}  ? "&nbsp;" : "");
> +	$opts{-size} ||= 'default';
> +	my $size = $avatar_size{$opts{-size}} || $avatar_size{'default'};
> +	my $url = "";
> +	if ($git_avatar eq 'gravatar') {
> +		$url = "http://www.gravatar.com/avatar/" .
> +			Digest::MD5::md5_hex(lc $email) . "?s=$size";
> +	}

I guess that you use short form 's' instead of 'size' to make URL (and
therefore gitweb pages) shorter, isn't it?

> +	# Currently only gravatars are supported, but other forms such as
> +	# picons can be added by putting an else up here and defining $url
> +	# as needed. If no variant puts something in $url, we assume avatars
> +	# are completely disabled/unavailable.
> +	if ($url) {
> +		return $pre_white .
> +		       "<img width=\"$size\" " .
> +		            "class=\"avatar\" " .
> +		            "src=\"$url\" " .
> +		       "/>" . $post_white;

Nitpicky sidenote: it might be more readable to use qq( ... ) quote-like
operator to avoid need to escape \" while providing variable 
interpolation.  But on the other hand it might not: this is more obscure
Perl operator.

Nevertheless it is not something to worry about (and certainly not 
something that would require resend).

> +	} else {
> +		return "";
> +	}
> +}
> +
>  # format the author name of the given commit with the given tag
>  # the author name is chopped and escaped according to the other
>  # optional parameters (see chop_str).
> @@ -1476,7 +1547,9 @@ sub format_author_html {
>  	my $tag = shift;
>  	my $co = shift;
>  	my $author = chop_and_escape_str($co->{'author_name'}, @_);
> -	return "<$tag class=\"author\">" . $author . "</$tag>";
> +	return "<$tag class=\"author\">" .
> +	       git_get_avatar($co->{'author_email'}, -pad_after => 1) .
> +	       $author . "</$tag>";
>  }
>  
>  # format git diff header line, i.e. "diff --(git|combined|cc) ..."
> @@ -3252,7 +3325,8 @@ sub git_print_authorship {
>  	      esc_html($co->{'author_name'}) .
>  	      " [$ad{'rfc2822'}";
>  	print_local_time(%ad) if ($opts{-localtime});
> -	print "]</$tag>\n";
> +	print "]" . git_get_avatar($co->{'author_email'}, -pad_before => 1)
> +		  . "</$tag>\n";
>  }
>  
>  # Outputs table rows containing the full author or committer information,
> @@ -3267,7 +3341,10 @@ sub git_print_authorship_rows {
>  	@people = ('author', 'committer') unless @people;
>  	foreach my $who (@people) {
>  		my %wd = parse_date($co->{"${who}_epoch"}, $co->{"${who}_tz"});
> -		print "<tr><td>$who</td><td>" . esc_html($co->{$who}) . "</td></tr>\n".
> +		print "<tr><td>$who</td><td>" . esc_html($co->{$who}) . "</td>" .
> +		      "<td rowspan=\"2\">" .
> +		      git_get_avatar($co->{"${who}_email"}, -size => 'double') .
> +		      "</td></tr>\n" .
>  		      "<tr>" .
>  		      "<td></td><td> $wd{'rfc2822'}";
>  		print_local_time(%wd);
> diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh
> index d539619..6275181 100755
> --- a/t/t9500-gitweb-standalone-no-errors.sh
> +++ b/t/t9500-gitweb-standalone-no-errors.sh
> @@ -660,6 +660,7 @@ cat >>gitweb_config.perl <<EOF
>  
>  \$feature{'blame'}{'override'} = 1;
>  \$feature{'snapshot'}{'override'} = 1;
> +\$feature{'avatar'}{'override'} = 1;
>  EOF
>  
>  test_expect_success \
> @@ -671,6 +672,7 @@ test_expect_success \
>  	'config override: tree view, features disabled in repo config' \
>  	'git config gitweb.blame no &&
>  	 git config gitweb.snapshot none &&
> +	 git config gitweb.avatar gravatar &&
>  	 gitweb_run "p=.git;a=tree"'
>  test_debug 'cat gitweb.log'
>  
> -- 
> 1.6.3.rc1.192.gdbfcb
> 
> 

-- 
Jakub Narebski
Poland

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCHv8 5/7] gitweb: gravatar url cache
  2009-06-29 22:00         ` [PATCHv8 5/7] gitweb: gravatar url cache Giuseppe Bilotta
  2009-06-29 22:00           ` [PATCHv8 6/7] gitweb: picon avatar provider Giuseppe Bilotta
@ 2009-06-30 20:18           ` Jakub Narebski
  2009-06-30 20:38           ` Junio C Hamano
  2 siblings, 0 replies; 22+ messages in thread
From: Jakub Narebski @ 2009-06-30 20:18 UTC (permalink / raw
  To: Giuseppe Bilotta; +Cc: git, Junio C Hamano, Petr Baudis

On Tue, 30 Jun 2009, Giuseppe Bilotta wrote:

> Views which contain many occurrences of the same email address (e.g.
> shortlog view) benefit from not having to recalculate the MD5 of the
> email address every time.
> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

Very nice, very well written.

Acked-by: Jakub Narebski <jnarebWgmail.com>

-- 
Jakub Narebski
Poland

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCHv8 6/7] gitweb: picon avatar provider
  2009-06-29 22:00           ` [PATCHv8 6/7] gitweb: picon avatar provider Giuseppe Bilotta
  2009-06-29 22:00             ` [PATCHv8 7/7] gitweb: add empty alt text to avatar img Giuseppe Bilotta
@ 2009-06-30 20:23             ` Jakub Narebski
  2009-06-30 20:50               ` Giuseppe Bilotta
  1 sibling, 1 reply; 22+ messages in thread
From: Jakub Narebski @ 2009-06-30 20:23 UTC (permalink / raw
  To: Giuseppe Bilotta; +Cc: git, Junio C Hamano, Petr Baudis

On Tue, 30 Jun 2009, Giuseppe Bilotta wrote:

> Simple implementation of picon that only relies on the indiana.edu
> database.
> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

Well written, and nice example how to add other avatar providers.

Acked-by: Jakub Narebski <jnareb@gmail.com>

[...]
> -	# Currently only the gravatar provider is available, and it
> -	# depends on Digest::MD5. If an unknown provider is specified,
> -	# the feature is disabled.
> +	# Currently available providers are gravatar and picon.
> +	# If an unknown provider is specified, the feature is disabled.
> +
> +	# Gravatar depends on Digest::MD5.
> +	# Picon currently relies on the indiana.edu database.

[...]
> -	# Currently only gravatars are supported, but other forms such as
> -	# picons can be added by putting an else up here and defining $url
> +	# Other providers can be added by extending the if chain, defining $url
>  	# as needed. If no variant puts something in $url, we assume avatars
>  	# are completely disabled/unavailable.

I see that you have updated comments too.

-- 
Jakub Narebski
Poland

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCHv8 1/7] gitweb: refactor author name insertion
  2009-06-30 20:04   ` [PATCHv8 1/7] gitweb: refactor author name insertion Jakub Narebski
@ 2009-06-30 20:23     ` Giuseppe Bilotta
  2009-06-30 21:08       ` Jakub Narebski
  0 siblings, 1 reply; 22+ messages in thread
From: Giuseppe Bilotta @ 2009-06-30 20:23 UTC (permalink / raw
  To: Jakub Narebski; +Cc: git, Junio C Hamano, Petr Baudis

2009/6/30 Jakub Narebski <jnareb@gmail.com>:
> On Tue, 30 Jun 2009, Giuseppe Bilotta wrote:
>> +sub print_local_time {
>> +     my %date = @_;
>> +     if ($date{'hour_local'} < 6) {
>> +             printf(" (<span class=\"atnight\">%02d:%02d</span> %s)",
>> +                     $date{'hour_local'}, $date{'minute_local'}, $date{'tz_local'});
>> +     } else {
>> +             printf(" (%02d:%02d %s)",
>> +                     $date{'hour_local'}, $date{'minute_local'}, $date{'tz_local'});
>> +     }
>> +}
>
> Very nice refactoring.
>
> It could do with a comment describing its output, but it is not
> necessary, and IMHO not worth resend.  We can always add it "in tree".

Damn! I was sure I'd forget something ...

-- 
Giuseppe "Oblomov" Bilotta

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCHv8 7/7] gitweb: add empty alt text to avatar img
  2009-06-29 22:00             ` [PATCHv8 7/7] gitweb: add empty alt text to avatar img Giuseppe Bilotta
@ 2009-06-30 20:29               ` Jakub Narebski
  0 siblings, 0 replies; 22+ messages in thread
From: Jakub Narebski @ 2009-06-30 20:29 UTC (permalink / raw
  To: Giuseppe Bilotta; +Cc: git, Junio C Hamano, Petr Baudis

On Tue, 30 Jun 2009, Giuseppe Bilotta wrote:

> The empty alt text optimizes screen estate in text-only browsers.
> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

I am not sure if such alt-text is a good idea from standards point
of view.  But the reasoning seems sound.

Acked-by: Jakub Narebski <jnareb@gmail.com>

> ---
>  gitweb/gitweb.perl |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 862ea99..6a1b5b5 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1574,6 +1574,7 @@ sub git_get_avatar {
>  		       "<img width=\"$size\" " .
>  		            "class=\"avatar\" " .
>  		            "src=\"$url\" " .
> +			    "alt=\"\" " .
>  		       "/>" . $post_white;
>  	} else {
>  		return "";
> -- 
> 1.6.3.rc1.192.gdbfcb
> 
> 

-- 
Jakub Narebski
Poland

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCHv8 5/7] gitweb: gravatar url cache
  2009-06-29 22:00         ` [PATCHv8 5/7] gitweb: gravatar url cache Giuseppe Bilotta
  2009-06-29 22:00           ` [PATCHv8 6/7] gitweb: picon avatar provider Giuseppe Bilotta
  2009-06-30 20:18           ` [PATCHv8 5/7] gitweb: gravatar url cache Jakub Narebski
@ 2009-06-30 20:38           ` Junio C Hamano
  2009-06-30 20:52             ` Giuseppe Bilotta
  2 siblings, 1 reply; 22+ messages in thread
From: Junio C Hamano @ 2009-06-30 20:38 UTC (permalink / raw
  To: Giuseppe Bilotta; +Cc: git, Jakub Narebski, Junio C Hamano, Petr Baudis

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

> +# Rather than recomputing the url for an email multiple times, we cache it
> +# after the first hit. This gives a visible benefit in views where the avatar
> +# for the same email is used repeatedly (e.g. shortlog).
> +# The cache is shared by all avatar engines (currently gravatar only), which
> +# are free to use it as preferred. Since only one avatar engine is used for any
> +# given page, there's no risk for cache conflicts.

Is it "any given page", or "any given e-mail"?

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCHv8 6/7] gitweb: picon avatar provider
  2009-06-30 20:23             ` [PATCHv8 6/7] gitweb: picon avatar provider Jakub Narebski
@ 2009-06-30 20:50               ` Giuseppe Bilotta
  0 siblings, 0 replies; 22+ messages in thread
From: Giuseppe Bilotta @ 2009-06-30 20:50 UTC (permalink / raw
  To: Jakub Narebski; +Cc: git, Junio C Hamano, Petr Baudis

2009/6/30 Jakub Narebski <jnareb@gmail.com>:
> On Tue, 30 Jun 2009, Giuseppe Bilotta wrote:
>> -     # Currently only gravatars are supported, but other forms such as
>> -     # picons can be added by putting an else up here and defining $url
>> +     # Other providers can be added by extending the if chain, defining $url
>>       # as needed. If no variant puts something in $url, we assume avatars
>>       # are completely disabled/unavailable.
>
> I see that you have updated comments too.

Well, I tried to address all the points that were raised during review 8-)

-- 
Giuseppe "Oblomov" Bilotta

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCHv8 5/7] gitweb: gravatar url cache
  2009-06-30 20:38           ` Junio C Hamano
@ 2009-06-30 20:52             ` Giuseppe Bilotta
  0 siblings, 0 replies; 22+ messages in thread
From: Giuseppe Bilotta @ 2009-06-30 20:52 UTC (permalink / raw
  To: Junio C Hamano; +Cc: git, Jakub Narebski, Petr Baudis

On Tue, Jun 30, 2009 at 10:38 PM, Junio C Hamano<gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> +# Rather than recomputing the url for an email multiple times, we cache it
>> +# after the first hit. This gives a visible benefit in views where the avatar
>> +# for the same email is used repeatedly (e.g. shortlog).
>> +# The cache is shared by all avatar engines (currently gravatar only), which
>> +# are free to use it as preferred. Since only one avatar engine is used for any
>> +# given page, there's no risk for cache conflicts.
>
> Is it "any given page", or "any given e-mail"?

It's 'page' as in HTML file served by the script. I couldn't find a
better way to express that.


-- 
Giuseppe "Oblomov" Bilotta

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCHv8 1/7] gitweb: refactor author name insertion
  2009-06-30 20:23     ` Giuseppe Bilotta
@ 2009-06-30 21:08       ` Jakub Narebski
  2009-06-30 23:15         ` Giuseppe Bilotta
  0 siblings, 1 reply; 22+ messages in thread
From: Jakub Narebski @ 2009-06-30 21:08 UTC (permalink / raw
  To: Giuseppe Bilotta; +Cc: git, Junio C Hamano, Petr Baudis

On Tue, 30 Jun 2009, Giuseppe Bilotta wrote:
> 2009/6/30 Jakub Narebski <jnareb@gmail.com>:
>> On Tue, 30 Jun 2009, Giuseppe Bilotta wrote:

# prints time in local timezone, from result of parse_date 
# in the following format: " (HH:MM +/-tz_offset)" or " (%H:%M %z)"
# with "atnight" warning when 0 <= hour_local < 6

or something like that.  Or just

# prints time in local timezone, from result of parse_date 

>>> +sub print_local_time {
>>> +     my %date = @_;
>>> +     if ($date{'hour_local'} < 6) {
>>> +             printf(" (<span class=\"atnight\">%02d:%02d</span> %s)",
>>> +                     $date{'hour_local'}, $date{'minute_local'}, $date{'tz_local'});
>>> +     } else {
>>> +             printf(" (%02d:%02d %s)",
>>> +                     $date{'hour_local'}, $date{'minute_local'}, $date{'tz_local'});
>>> +     }
>>> +}
>>
>> Very nice refactoring.
>>
>> It could do with a comment describing its output, but it is not
>> necessary, and IMHO not worth resend.  We can always add it "in tree".
> 
> Damn! I was sure I'd forget something ...

Don't worry. It can be always addd later, or Junio can just squash 
proposed comment (see above).

-- 
Jakub Narebski
Poland

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCHv8 1/7] gitweb: refactor author name insertion
  2009-06-30 21:08       ` Jakub Narebski
@ 2009-06-30 23:15         ` Giuseppe Bilotta
  2009-07-01  7:41           ` Jakub Narebski
  0 siblings, 1 reply; 22+ messages in thread
From: Giuseppe Bilotta @ 2009-06-30 23:15 UTC (permalink / raw
  To: Jakub Narebski; +Cc: git, Junio C Hamano, Petr Baudis

On Tue, Jun 30, 2009 at 11:08 PM, Jakub Narebski<jnareb@gmail.com> wrote:
> On Tue, 30 Jun 2009, Giuseppe Bilotta wrote:
>> 2009/6/30 Jakub Narebski <jnareb@gmail.com>:
>>> On Tue, 30 Jun 2009, Giuseppe Bilotta wrote:
>
> # prints time in local timezone, from result of parse_date
> # in the following format: " (HH:MM +/-tz_offset)" or " (%H:%M %z)"
> # with "atnight" warning when 0 <= hour_local < 6
>
> or something like that.  Or just
>
> # prints time in local timezone, from result of parse_date

How about

# prints time in local timezone, highlight night hours

>>>> +sub print_local_time {
>>>> +     my %date = @_;
>>>> +     if ($date{'hour_local'} < 6) {
>>>> +             printf(" (<span class=\"atnight\">%02d:%02d</span> %s)",
>>>> +                     $date{'hour_local'}, $date{'minute_local'}, $date{'tz_local'});
>>>> +     } else {
>>>> +             printf(" (%02d:%02d %s)",
>>>> +                     $date{'hour_local'}, $date{'minute_local'}, $date{'tz_local'});
>>>> +     }
>>>> +}

-- 
Giuseppe "Oblomov" Bilotta

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCHv8 1/7] gitweb: refactor author name insertion
  2009-06-30 23:15         ` Giuseppe Bilotta
@ 2009-07-01  7:41           ` Jakub Narebski
  0 siblings, 0 replies; 22+ messages in thread
From: Jakub Narebski @ 2009-07-01  7:41 UTC (permalink / raw
  To: Giuseppe Bilotta; +Cc: git, Junio C Hamano, Petr Baudis

On Wed, 1 July 2009, Giuseppe Bilotta wrote:
> On Tue, Jun 30, 2009 at 11:08 PM, Jakub Narebski<jnareb@gmail.com> wrote:

>> # prints time in local timezone, from result of parse_date
>> # in the following format: " (HH:MM +/-tz_offset)" or " (%H:%M %z)"
>> # with "atnight" warning when 0 <= hour_local < 6
>>
>> or something like that.  Or just
>>
>> # prints time in local timezone, from result of parse_date
> 
> How about
> 
> # prints time in local timezone, highlight night hours

Or

# prints local part of parse_date, highlight night hours

It important, I think, to state that print_local_time takes result of
parse_date as an argument.

Nevertheless that is just nitpicking and bikeshedding.
 
>>>>> +sub print_local_time {
>>>>> +     my %date = @_;
>>>>> +     if ($date{'hour_local'} < 6) {
>>>>> +             printf(" (<span class=\"atnight\">%02d:%02d</span> %s)",
>>>>> +                     $date{'hour_local'}, $date{'minute_local'}, $date{'tz_local'});
>>>>> +     } else {
>>>>> +             printf(" (%02d:%02d %s)",
>>>>> +                     $date{'hour_local'}, $date{'minute_local'}, $date{'tz_local'});
>>>>> +     }
>>>>> +}
> 
> -- 
> Giuseppe "Oblomov" Bilotta
> 

-- 
Jakub Narebski
Poland

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2009-07-01  7:43 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-06-29 22:00 [PATCHv8 0/7] gitweb: avatar support Giuseppe Bilotta
2009-06-29 22:00 ` [PATCHv8 1/7] gitweb: refactor author name insertion Giuseppe Bilotta
2009-06-29 22:00   ` [PATCHv8 2/7] gitweb: uniform author info for commit and commitdiff Giuseppe Bilotta
2009-06-29 22:00     ` [PATCHv8 3/7] gitweb: use git_print_authorship_rows in 'tag' view too Giuseppe Bilotta
2009-06-29 22:00       ` [PATCHv8 4/7] gitweb: (gr)avatar support Giuseppe Bilotta
2009-06-29 22:00         ` [PATCHv8 5/7] gitweb: gravatar url cache Giuseppe Bilotta
2009-06-29 22:00           ` [PATCHv8 6/7] gitweb: picon avatar provider Giuseppe Bilotta
2009-06-29 22:00             ` [PATCHv8 7/7] gitweb: add empty alt text to avatar img Giuseppe Bilotta
2009-06-30 20:29               ` Jakub Narebski
2009-06-30 20:23             ` [PATCHv8 6/7] gitweb: picon avatar provider Jakub Narebski
2009-06-30 20:50               ` Giuseppe Bilotta
2009-06-30 20:18           ` [PATCHv8 5/7] gitweb: gravatar url cache Jakub Narebski
2009-06-30 20:38           ` Junio C Hamano
2009-06-30 20:52             ` Giuseppe Bilotta
2009-06-30 20:16         ` [PATCHv8 4/7] gitweb: (gr)avatar support Jakub Narebski
2009-06-30 20:10       ` [PATCHv8 3/7] gitweb: use git_print_authorship_rows in 'tag' view too Jakub Narebski
2009-06-30 20:04     ` [PATCHv8 2/7] gitweb: uniform author info for commit and commitdiff Jakub Narebski
2009-06-30 20:04   ` [PATCHv8 1/7] gitweb: refactor author name insertion Jakub Narebski
2009-06-30 20:23     ` Giuseppe Bilotta
2009-06-30 21:08       ` Jakub Narebski
2009-06-30 23:15         ` Giuseppe Bilotta
2009-07-01  7:41           ` Jakub Narebski

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).