{"thread":{"id":"19948","subject":"[PATCHv7 0/9] gitweb: avatar support","startedAt":"2009-06-27T12:04:56Z","lastAt":"2009-06-29T21:55:38Z","messageCount":31,"participants":["Giuseppe Bilotta","Jakub Narebski","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"117075","messageId":"1246104305-15191-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"19948","inReplyTo":null,"subject":"[PATCHv7 0/9] gitweb: avatar support","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-27T12:04:56Z","receivedAt":"2009-06-27T12:04:56Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"One more attempt at adding avatar support to gitweb.\n\nThe most significant change since the previous iteration is the addition\nof a picon provider, and its use as default from gravatar.\n\nMost other patches were changed to follow Jakub's suggestions. In\nparticular, the two-line author/committer information was refactored\nto reduce code duplication, with the benefit that it could also be used\nin 'tag' view.\n\nGiuseppe Bilotta (9):\n gitweb: refactor author name insertion\n gitweb: uniform author info for commit and commitdiff\n gitweb: use git_print_authorship_rows in 'tag' view too\n gitweb: right-align date cell in shortlog\n gitweb: (gr)avatar support\n gitweb: gravatar url cache\n gitweb: picon avatar provider\n gitweb: use picon for gravatar fallback\n gitweb: add alt text to avatar img\n\n gitweb/gitweb.css | 13 ++-\n gitweb/gitweb.perl | 238 ++++++++++++++++++++++++++------\n t/t9500-gitweb-standalone-no-errors.sh | 2 +\n 3 files changed, 207 insertions(+), 46 deletions(-)\n"},{"id":"117076","messageId":"1246104305-15191-2-git-send-email-giuseppe.bilotta@gmail.com","threadId":"19948","inReplyTo":"1246104305-15191-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv7 1/9] gitweb: refactor author name insertion","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-27T12:04:57Z","receivedAt":"2009-06-27T12:04:57Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Collect all author display code in appropriate functions, making it\neasier to extend these functions on the CGI side.\n\nWe also move some of the presentation code from hard-coded HTML to CSS,\nfor easier customization.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.css | 5 ++-\n gitweb/gitweb.perl | 96 +++++++++++++++++++++++++++++++--------------------\n 2 files changed, 62 insertions(+), 39 deletions(-)\n\ndiff --git a/gitweb/gitweb.css b/gitweb/gitweb.css\nindex a01eac8..68b22ff 100644\n--- a/gitweb/gitweb.css\n+++ b/gitweb/gitweb.css\n@@ -132,11 +132,14 @@ div.list_head {\n \tfont-style: italic;\n }\n \n+.author_date, .author {\n+\tfont-style: italic;\n+}\n+\n div.author_date {\n \tpadding: 8px;\n \tborder: solid #d9d8d1;\n \tborder-width: 0px 0px 1px 0px;\n-\tfont-style: italic;\n }\n \n a.list {\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 1e7e2d8..9be723c 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1469,6 +1469,16 @@ sub format_subject_html {\n \t}\n }\n \n+# format the author name of the given commit with the given tag\n+# the author name is chopped and escaped according to the other\n+# optional parameters (see chop_str).\n+sub format_author_html {\n+\tmy $tag = shift;\n+\tmy $co = shift;\n+\tmy $author = chop_and_escape_str($co->{'author_name'}, @_);\n+\treturn \"<$tag class=\\\"author\\\">\" . $author . \"\";\n+}\n+\n # format git diff header line, i.e. \"diff --(git|combined|cc) ...\"\n sub format_git_diff_header_line {\n \tmy $line = shift;\n@@ -3214,21 +3224,53 @@ sub git_print_header_div {\n \t \"\\n\\n\";\n }\n \n+# Outputs the author name and date in long form\n sub git_print_authorship {\n \tmy $co = shift;\n+\tmy %opts = @_;\n+\tmy $tag = $opts{-tag} || 'div';\n \n \tmy %ad = parse_date($co->{'author_epoch'}, $co->{'author_tz'});\n-\tprint \"
\" .\n+\tprint \"<$tag class=\\\"author_date\\\">\" .\n \t esc_html($co->{'author_name'}) .\n \t \" [$ad{'rfc2822'}\";\n-\tif ($ad{'hour_local'} < 6) {\n-\t\tprintf(\" (%02d:%02d %s)\",\n-\t\t $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});\n-\t} else {\n-\t\tprintf(\" (%02d:%02d %s)\",\n-\t\t $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});\n+\tif ($opts{-localtime}) {\n+\t\tif ($ad{'hour_local'} < 6) {\n+\t\t\tprintf(\" (%02d:%02d %s)\",\n+\t\t\t $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});\n+\t\t} else {\n+\t\t\tprintf(\" (%02d:%02d %s)\",\n+\t\t\t $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});\n+\t\t}\n+\t}\n+\tprint \"]\\n\";\n+}\n+\n+# Outputs table rows containing the full author or committer information,\n+# in the format expected for 'commit' view (& similia).\n+# Parameters are a commit hash reference, followed by the list of people\n+# to output information for. If the list is empty it defalts to both\n+# author and committer.\n+sub git_print_authorship_rows {\n+\tmy $co = shift;\n+\t# too bad we can't use @people = @_ || ('author', 'committer')\n+\tmy @people = @_;\n+\t@people = ('author', 'committer') unless @people;\n+\tforeach my $who (@people) {\n+\t\tmy %ad = parse_date($co->{$who . '_epoch'}, $co->{$who . '_tz'});\n+\t\tprint \"$who\" . esc_html($co->{$who}) . \"\\n\".\n+\t\t \"\" .\n+\t\t \" $ad{'rfc2822'}\";\n+\t\tif ($ad{'hour_local'} < 6) {\n+\t\t\tprintf(\" (%02d:%02d %s)\",\n+\t\t\t $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});\n+\t\t} else {\n+\t\t\tprintf(\" (%02d:%02d %s)\",\n+\t\t\t $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});\n+\t\t}\n+\t\tprint \"\" .\n+\t\t \"\\n\";\n \t}\n-\tprint \"]
\\n\";\n }\n \n sub git_print_page_path {\n@@ -4142,11 +4184,9 @@ sub git_shortlog_body {\n \t\t\tprint \"\\n\";\n \t\t}\n \t\t$alternate ^= 1;\n-\t\tmy $author = chop_and_escape_str($co{'author_name'}, 10);\n \t\t# git_summary() used print \"$co{'age_string'}\\n\" .\n \t\tprint \"$co{'age_string_date'}\\n\" .\n-\t\t \"\" . $author . \"\\n\" .\n-\t\t \"\";\n+\t\t format_author_html('td', \\%co, 10) . \"\";\n \t\tprint format_subject_html($co{'title'}, $co{'title_short'},\n \t\t href(action=>\"commit\", hash=>$commit), $ref);\n \t\tprint \"\\n\" .\n@@ -4193,11 +4233,9 @@ sub git_history_body {\n \t\t\tprint \"\\n\";\n \t\t}\n \t\t$alternate ^= 1;\n-\t# shortlog uses chop_str($co{'author_name'}, 10)\n-\t\tmy $author = chop_and_escape_str($co{'author_name'}, 15, 3);\n \t\tprint \"$co{'age_string_date'}\\n\" .\n-\t\t \"\" . $author . \"\\n\" .\n-\t\t \"\";\n+\t# shortlog: format_author_html('td', \\%co, 10)\n+\t\t format_author_html('td', \\%co, 15, 3) . \"\";\n \t\t# originally git_history used chop_str($co{'title'}, 50)\n \t\tprint format_subject_html($co{'title'}, $co{'title_short'},\n \t\t href(action=>\"commit\", hash=>$commit), $ref);\n@@ -4350,9 +4388,8 @@ sub git_search_grep_body {\n \t\t\tprint \"\\n\";\n \t\t}\n \t\t$alternate ^= 1;\n-\t\tmy $author = chop_and_escape_str($co{'author_name'}, 15, 5);\n \t\tprint \"$co{'age_string_date'}\\n\" .\n-\t\t \"\" . $author . \"\\n\" .\n+\t\t format_author_html('td', \\%co, 15, 5) .\n \t\t \"\" .\n \t\t $cgi->a({-href => href(action=>\"commit\", hash=>$co{'id'}),\n \t\t -class => \"list subject\"},\n@@ -5094,9 +5131,9 @@ sub git_log {\n \t\t \" | \" .\n \t\t $cgi->a({-href => href(action=>\"tree\", hash=>$commit, hash_base=>$commit)}, \"tree\") .\n \t\t \"
\\n\" .\n-\t\t \"\\n\" .\n-\t\t \"\" . esc_html($co{'author_name'}) . \" [$ad{'rfc2822'}]
\\n\" .\n \t\t \"\\n\";\n+\t\t git_print_authorship(\\%co, -tag => 'span');\n+\t\t print \"
\\n\\n\";\n \n \t\tprint \"
\\n\";\n \t\tgit_print_log($co{'comment'}, -final_empty_line=> 1);\n@@ -5115,8 +5152,6 @@ sub git_commit {\n \t$hash ||= $hash_base || \"HEAD\";\n \tmy %co = parse_commit($hash)\n \t or die_error(404, \"Unknown commit object\");\n-\tmy %ad = parse_date($co{'author_epoch'}, $co{'author_tz'});\n-\tmy %cd = parse_date($co{'committer_epoch'}, $co{'committer_tz'});\n \n \tmy $parent = $co{'parent'};\n \tmy $parents = $co{'parents'}; # listref\n@@ -5183,22 +5218,7 @@ sub git_commit {\n \t}\n \tprint \"
\\n\" .\n \t \"\\n\";\n-\tprint \"\\n\".\n-\t \"\" .\n-\t \"\" .\n-\t \"\\n\";\n-\tprint \"\\n\";\n-\tprint \"\\n\";\n+\tgit_print_authorship_rows(\\%co);\n \tprint \"\\n\";\n \tprint \"\" .\n \t \"\" .\n@@ -5579,7 +5599,7 @@ sub git_commitdiff {\n \t\tgit_header_html(undef, $expires);\n \t\tgit_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash, $formats_nav);\n \t\tgit_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash);\n-\t\tgit_print_authorship(\\%co);\n+\t\tgit_print_authorship(\\%co, 'localtime' => 1);\n \t\tprint \"
\\n\";\n \t\tif (@{$co{'comment'}} > 1) {\n \t\t\tprint \"
\\n\";\n-- \n1.6.3.rc1.192.gdbfcb\n"},{"id":"117078","messageId":"1246104305-15191-3-git-send-email-giuseppe.bilotta@gmail.com","threadId":"19948","inReplyTo":"1246104305-15191-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv7 2/9] gitweb: uniform author info for commit and commitdiff","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-27T12:04:58Z","receivedAt":"2009-06-27T12:04:58Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Switch from\n\nA U Thor [date time]\n\nto\n\nauthor\tA U Thor \n\tdate time\ncommitter\tC O Mitter \n\tcommitter date time\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 6 +++++-\n 1 files changed, 5 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 9be723c..0d8005d 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5599,7 +5599,11 @@ sub git_commitdiff {\n \t\tgit_header_html(undef, $expires);\n \t\tgit_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash, $formats_nav);\n \t\tgit_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash);\n-\t\tgit_print_authorship(\\%co, 'localtime' => 1);\n+\t\tprint \"
\\n\" .\n+\t\t \"
author\" . esc_html($co{'author'}) . \"
$ad{'rfc2822'}\";\n-\tif ($ad{'hour_local'} < 6) {\n-\t\tprintf(\" (%02d:%02d %s)\",\n-\t\t $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});\n-\t} else {\n-\t\tprintf(\" (%02d:%02d %s)\",\n-\t\t $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});\n-\t}\n-\tprint \"
committer\" . esc_html($co{'committer'}) . \"
$cd{'rfc2822'}\" .\n-\t sprintf(\" (%02d:%02d %s)\", $cd{'hour_local'}, $cd{'minute_local'}, $cd{'tz_local'}) .\n-\t \"
commit$co{'id'}
tree
\\n\";\n+\t\tgit_print_authorship_rows(\\%co);\n+\t\tprint \"
\".\n+\t\t \"
\\n\";\n \t\tprint \"
\\n\";\n \t\tif (@{$co{'comment'}} > 1) {\n \t\t\tprint \"
\\n\";\n-- \n1.6.3.rc1.192.gdbfcb\n"},{"id":"117077","messageId":"1246104305-15191-4-git-send-email-giuseppe.bilotta@gmail.com","threadId":"19948","inReplyTo":"1246104305-15191-3-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv7 3/9] gitweb: use git_print_authorship_rows in 'tag' view too","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-27T12:04:59Z","receivedAt":"2009-06-27T12:04:59Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"parse_tag has to be adapted to use the hash keys expected by\ngit_print_authorship_rows, which is not a problem since git_tag\nis the only user of this sub.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 16 +++++++++-------\n 1 files changed, 9 insertions(+), 7 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 0d8005d..7183ad2 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -2409,8 +2409,14 @@ sub parse_tag {\n \t\t\t$tag{'name'} = $1;\n \t\t} elsif ($line =~ m/^tagger (.*) ([0-9]+) (.*)$/) {\n \t\t\t$tag{'author'} = $1;\n-\t\t\t$tag{'epoch'} = $2;\n-\t\t\t$tag{'tz'} = $3;\n+\t\t\t$tag{'author_epoch'} = $2;\n+\t\t\t$tag{'author_tz'} = $3;\n+\t\t\tif ($tag{'author'} =~ m/^([^<]+) <([^>]*)>/) {\n+\t\t\t\t$tag{'author_name'} = $1;\n+\t\t\t\t$tag{'author_email'} = $2;\n+\t\t\t} else {\n+\t\t\t\t$tag{'author_name'} = $tag{'author'};\n+\t\t\t}\n \t\t} elsif ($line =~ m/--BEGIN/) {\n \t\t\tpush @comment, $line;\n \t\t\tlast;\n@@ -4623,11 +4629,7 @@ sub git_tag {\n \t $tag{'type'}) . \"\\n\" .\n \t \"\\n\";\n \tif (defined($tag{'author'})) {\n-\t\tmy %ad = parse_date($tag{'epoch'}, $tag{'tz'});\n-\t\tprint \"author\" . esc_html($tag{'author'}) . \"\\n\";\n-\t\tprint \"\" . $ad{'rfc2822'} .\n-\t\t\tsprintf(\" (%02d:%02d %s)\", $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'}) .\n-\t\t\t\"\\n\";\n+\t\tgit_print_authorship_rows(\\%tag, 'author');\n \t}\n \tprint \"\\n\\n\" .\n \t \"
\\n\";\n-- \n1.6.3.rc1.192.gdbfcb\n"},{"id":"117079","messageId":"1246104305-15191-5-git-send-email-giuseppe.bilotta@gmail.com","threadId":"19948","inReplyTo":"1246104305-15191-4-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv7 4/9] gitweb: right-align date cell in shortlog","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-27T12:05:00Z","receivedAt":"2009-06-27T12:05:00Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Signed-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.css | 4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/gitweb/gitweb.css b/gitweb/gitweb.css\nindex 68b22ff..ef24a1b 100644\n--- a/gitweb/gitweb.css\n+++ b/gitweb/gitweb.css\n@@ -180,6 +180,10 @@ table {\n \tborder-spacing: 0;\n }\n \n+table.shortlog td:first-child {\n+\ttext-align: right;\n+}\n+\n table.diff_tree {\n \tfont-family: monospace;\n }\n-- \n1.6.3.rc1.192.gdbfcb\n"},{"id":"117081","messageId":"1246104305-15191-6-git-send-email-giuseppe.bilotta@gmail.com","threadId":"19948","inReplyTo":"1246104305-15191-5-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv7 5/9] gitweb: (gr)avatar support","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-27T12:05:01Z","receivedAt":"2009-06-27T12:05:01Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Introduce avatar support: the feature adds the appropriate img tag next\nto author and committer in commit(diff), history, shortlog, log and tag\nviews. Multiple avatar providers are possible, but only gravatar is\nimplemented at the moment.\n\nGravatar support depends on Digest::MD5, which is a core package since\nPerl 5.8. If gravatars are activated but Digest::MD5 cannot be found,\nthe feature will be automatically disabled.\n\nNo avatar provider is selected by default, except in the t9500 test.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.css | 4 ++\n gitweb/gitweb.perl | 81 ++++++++++++++++++++++++++++++-\n t/t9500-gitweb-standalone-no-errors.sh | 2 +\n 3 files changed, 84 insertions(+), 3 deletions(-)\n\ndiff --git a/gitweb/gitweb.css b/gitweb/gitweb.css\nindex ef24a1b..cd8066d 100644\n--- a/gitweb/gitweb.css\n+++ b/gitweb/gitweb.css\n@@ -28,6 +28,10 @@ img.logo {\n \tborder-width: 0px;\n }\n \n+img.avatar {\n+\tvertical-align: middle;\n+}\n+\n div.page_header {\n \theight: 25px;\n \tpadding: 8px;\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 7183ad2..ad9ae31 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -195,6 +195,14 @@ our %known_snapshot_format_aliases = (\n \t'x-zip' => undef, '' => undef,\n );\n \n+# Pixel sizes for icons and avatars. If the default font sizes or lineheights\n+# are changed, it may be appropriate to change these values too via\n+# $GITWEB_CONFIG.\n+our %avatar_size = (\n+\t'default' => 16,\n+\t'double' => 32\n+);\n+\n # You define site-wide feature defaults here; override them with\n # $GITWEB_CONFIG as necessary.\n our %feature = (\n@@ -365,6 +373,23 @@ our %feature = (\n \t\t'sub' => \\&feature_patches,\n \t\t'override' => 0,\n \t\t'default' => [16]},\n+\n+\t# Avatar support. When this feature is enabled, views such as\n+\t# shortlog or commit will display an avatar associated with\n+\t# the email of the committer(s) and/or author(s).\n+\n+\t# Currently only the gravatar provider is available, and it\n+\t# depends on Digest::MD5.\n+\n+\t# To enable system wide have in $GITWEB_CONFIG\n+\t# $feature{'avatar'}{'default'} = ['gravatar'];\n+\t# To have project specific config enable override in $GITWEB_CONFIG\n+\t# $feature{'avatar'}{'override'} = 1;\n+\t# and in project config gitweb.avatar = gravatar;\n+\t'avatar' => {\n+\t\t'sub' => \\&feature_avatar,\n+\t\t'override' => 0,\n+\t\t'default' => ['']},\n );\n \n sub gitweb_get_feature {\n@@ -433,6 +458,16 @@ sub feature_patches {\n \treturn ($_[0]);\n }\n \n+sub feature_avatar {\n+\tmy @val = (git_get_project_config('avatar'));\n+\n+\tif (@val) {\n+\t\treturn @val;\n+\t}\n+\n+\treturn @_;\n+}\n+\n # checking HEAD file with -e is fragile if the repository was\n # initialized long time ago (i.e. symlink HEAD) and was pack-ref'ed\n # and then pruned.\n@@ -814,6 +849,17 @@ $git_dir = \"$projectroot/$project\" if $project;\n our @snapshot_fmts = gitweb_get_feature('snapshot');\n @snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);\n \n+# check that the avatar feature is set to a known provider name,\n+# and for each provider check if the dependencies are satisfied.\n+# if the provider name is invalid or the dependencies are not met,\n+# reset $git_avatar to the empty string.\n+our ($git_avatar) = gitweb_get_feature('avatar');\n+if ($git_avatar eq 'gravatar') {\n+\t$git_avatar = '' unless (eval { require Digest::MD5; 1; });\n+} else {\n+\t$git_avatar = '';\n+}\n+\n # dispatch\n if (!defined $action) {\n \tif (defined $hash) {\n@@ -1469,6 +1515,29 @@ sub format_subject_html {\n \t}\n }\n \n+# Insert an avatar for the given $email at the given $size if the feature\n+# is enabled.\n+sub git_get_avatar {\n+\tmy ($email, %opts) = @_;\n+\tmy $pre_white = ($opts{'pad_before'} ? \" \" : \"\");\n+\tmy $post_white = ($opts{'pad_after'} ? \" \" : \"\");\n+\tmy $size = $avatar_size{$opts{'size'}} || $avatar_size{'default'};\n+\tmy $url = \"\";\n+\tif ($git_avatar eq 'gravatar') {\n+\t\t$url = \"http://www.gravatar.com/avatar.php?gravatar_id=\" .\n+\t\t\tDigest::MD5::md5_hex(lc $email) . \"&size=$size\";\n+\t}\n+\t# Currently only gravatars are supported, but other forms such as\n+\t# picons can be added by putting an else up here and defining $url\n+\t# as needed. If no variant puts something in $url, we assume avatars\n+\t# are completely disabled/unavailable.\n+\tif ($url) {\n+\t\treturn $pre_white . \"\" . $post_white;\n+\t} else {\n+\t\treturn \"\";\n+\t}\n+}\n+\n # format the author name of the given commit with the given tag\n # the author name is chopped and escaped according to the other\n # optional parameters (see chop_str).\n@@ -1476,7 +1545,9 @@ sub format_author_html {\n \tmy $tag = shift;\n \tmy $co = shift;\n \tmy $author = chop_and_escape_str($co->{'author_name'}, @_);\n-\treturn \"<$tag class=\\\"author\\\">\" . $author . \"\";\n+\treturn \"<$tag class=\\\"author\\\">\" .\n+\t git_get_avatar($co->{'author_email'}, 'pad_after' => 1) .\n+\t $author . \"\";\n }\n \n # format git diff header line, i.e. \"diff --(git|combined|cc) ...\"\n@@ -3249,7 +3320,8 @@ sub git_print_authorship {\n \t\t\t $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});\n \t\t}\n \t}\n-\tprint \"]\\n\";\n+\tprint \"]\" . git_get_avatar($co->{'author_email'}, 'pad_before' => 1)\n+\t\t . \"\\n\";\n }\n \n # Outputs table rows containing the full author or committer information,\n@@ -3264,7 +3336,10 @@ sub git_print_authorship_rows {\n \t@people = ('author', 'committer') unless @people;\n \tforeach my $who (@people) {\n \t\tmy %ad = parse_date($co->{$who . '_epoch'}, $co->{$who . '_tz'});\n-\t\tprint \"$who\" . esc_html($co->{$who}) . \"\\n\".\n+\t\tprint \"$who\" . esc_html($co->{$who}) . \"\" .\n+\t\t \"\" .\n+\t\t git_get_avatar($co->{$who . '_email'}, 'size' => 'double') .\n+\t\t \"\\n\" .\n \t\t \"\" .\n \t\t \" $ad{'rfc2822'}\";\n \t\tif ($ad{'hour_local'} < 6) {\ndiff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh\nindex d539619..6275181 100755\n--- a/t/t9500-gitweb-standalone-no-errors.sh\n+++ b/t/t9500-gitweb-standalone-no-errors.sh\n@@ -660,6 +660,7 @@ cat >>gitweb_config.perl <\n---\n gitweb/gitweb.perl | 30 +++++++++++++++++++++++++++---\n 1 files changed, 27 insertions(+), 3 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex ad9ae31..de4cc63 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1515,6 +1515,27 @@ sub format_subject_html {\n \t}\n }\n \n+# Rather than recomputing the url for an email multiple times, we cache it\n+# after the first hit. This gives a visible benefit in views where the avatar\n+# for the same email is used repeatedly (e.g. shortlog).\n+# The cache is shared by all avatar engines (currently gravatar only), which\n+# are free to use it as preferred. Since only one avatar engine is used for any\n+# given page, there's no risk for cache conflicts.\n+our %avatar_cache = ();\n+\n+# Compute the gravatar url for a given email, if it's not in the cache already.\n+# Gravatar stores only the part of the URL before the size, since that's the\n+# one computationally more expensive. This also allows reuse of the cache for\n+# different sizes (for this particular engine).\n+sub gravatar_url {\n+\tmy $email = lc shift;\n+\tmy $size = shift;\n+\t$avatar_cache{$email} ||=\n+\t\t\"http://www.gravatar.com/avatar.php?gravatar_id=\" .\n+\t\t\tDigest::MD5::md5_hex($email) . \"&size=\";\n+\treturn $avatar_cache{$email} . $size;\n+}\n+\n # Insert an avatar for the given $email at the given $size if the feature\n # is enabled.\n sub git_get_avatar {\n@@ -1524,15 +1545,18 @@ sub git_get_avatar {\n \tmy $size = $avatar_size{$opts{'size'}} || $avatar_size{'default'};\n \tmy $url = \"\";\n \tif ($git_avatar eq 'gravatar') {\n-\t\t$url = \"http://www.gravatar.com/avatar.php?gravatar_id=\" .\n-\t\t\tDigest::MD5::md5_hex(lc $email) . \"&size=$size\";\n+\t\t$url = gravatar_url($email, $size);\n \t}\n \t# Currently only gravatars are supported, but other forms such as\n \t# picons can be added by putting an else up here and defining $url\n \t# as needed. If no variant puts something in $url, we assume avatars\n \t# are completely disabled/unavailable.\n \tif ($url) {\n-\t\treturn $pre_white . \"\" . $post_white;\n+\t\treturn $pre_white .\n+\t\t \"\" . $post_white;\n \t} else {\n \t\treturn \"\";\n \t}\n-- \n1.6.3.rc1.192.gdbfcb\n"},{"id":"117084","messageId":"1246104305-15191-8-git-send-email-giuseppe.bilotta@gmail.com","threadId":"19948","inReplyTo":"1246104305-15191-7-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv7 7/9] gitweb: picon avatar provider","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-27T12:05:03Z","receivedAt":"2009-06-27T12:05:03Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Simple implementation of picon that only relies on the indiana.edu\ndatabase.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 26 ++++++++++++++++++++++----\n 1 files changed, 22 insertions(+), 4 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex de4cc63..ae73d45 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -378,14 +378,17 @@ our %feature = (\n \t# shortlog or commit will display an avatar associated with\n \t# the email of the committer(s) and/or author(s).\n \n-\t# Currently only the gravatar provider is available, and it\n-\t# depends on Digest::MD5.\n+\t# Currently available providers are gravatar and picon.\n+\n+\t# Gravatar depends on Digest::MD5.\n+\t# Picon currently relies on the indiana.edu database.\n \n \t# To enable system wide have in $GITWEB_CONFIG\n-\t# $feature{'avatar'}{'default'} = ['gravatar'];\n+\t# $feature{'avatar'}{'default'} = ['provider'];\n+\t# where provider is either gravatar or picon.\n \t# To have project specific config enable override in $GITWEB_CONFIG\n \t# $feature{'avatar'}{'override'} = 1;\n-\t# and in project config gitweb.avatar = gravatar;\n+\t# and in project config gitweb.avatar = provider;\n \t'avatar' => {\n \t\t'sub' => \\&feature_avatar,\n \t\t'override' => 0,\n@@ -856,6 +859,8 @@ our @snapshot_fmts = gitweb_get_feature('snapshot');\n our ($git_avatar) = gitweb_get_feature('avatar');\n if ($git_avatar eq 'gravatar') {\n \t$git_avatar = '' unless (eval { require Digest::MD5; 1; });\n+} elsif ($git_avatar eq 'picon') {\n+\t# no dependencies\n } else {\n \t$git_avatar = '';\n }\n@@ -1523,6 +1528,17 @@ sub format_subject_html {\n # given page, there's no risk for cache conflicts.\n our %avatar_cache = ();\n \n+# Compute the picon url for a given email, by using the picon search service over at\n+# http://www.cs.indiana.edu/picons/search.html\n+sub picon_url {\n+\tmy $email = lc shift;\n+\tif (!$avatar_cache{$email}) {\n+\t\tmy ($user, $domain) = split('@', $email);\n+\t\t$avatar_cache{$email} = \"http://www.cs.indiana.edu/cgi-pub/kinzler/piconsearch.cgi/$domain/$user/users+domains+unknown/up/single\";\n+\t}\n+\treturn $avatar_cache{$email};\n+}\n+\n # Compute the gravatar url for a given email, if it's not in the cache already.\n # Gravatar stores only the part of the URL before the size, since that's the\n # one computationally more expensive. This also allows reuse of the cache for\n@@ -1546,6 +1562,8 @@ sub git_get_avatar {\n \tmy $url = \"\";\n \tif ($git_avatar eq 'gravatar') {\n \t\t$url = gravatar_url($email, $size);\n+\t} elsif ($git_avatar eq 'picon') {\n+\t\t$url = picon_url($email);\n \t}\n \t# Currently only gravatars are supported, but other forms such as\n \t# picons can be added by putting an else up here and defining $url\n-- \n1.6.3.rc1.192.gdbfcb\n"},{"id":"117082","messageId":"1246104305-15191-9-git-send-email-giuseppe.bilotta@gmail.com","threadId":"19948","inReplyTo":"1246104305-15191-8-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv7 8/9] gitweb: use picon for gravatar fallback","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-27T12:05:04Z","receivedAt":"2009-06-27T12:05:04Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Signed-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 10 +++++++---\n 1 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex ae73d45..e2638cb 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1546,9 +1546,13 @@ sub picon_url {\n sub gravatar_url {\n \tmy $email = lc shift;\n \tmy $size = shift;\n-\t$avatar_cache{$email} ||=\n-\t\t\"http://www.gravatar.com/avatar.php?gravatar_id=\" .\n-\t\t\tDigest::MD5::md5_hex($email) . \"&size=\";\n+\tif (!$avatar_cache{$email}) {\n+\t\tmy $picon = CGI::escape(picon_url($email));\n+\t\t$avatar_cache{$email} = \"http://www.gravatar.com/avatar.php?gravatar_id=\" .\n+\t\t\tDigest::MD5::md5_hex($email) .\n+\t\t\t\"&default=$picon\" .\n+\t\t\t\"&size=\";\n+\t}\n \treturn $avatar_cache{$email} . $size;\n }\n \n-- \n1.6.3.rc1.192.gdbfcb\n"},{"id":"117083","messageId":"1246104305-15191-10-git-send-email-giuseppe.bilotta@gmail.com","threadId":"19948","inReplyTo":"1246104305-15191-9-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv7 9/9] gitweb: add alt text to avatar img","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-27T12:05:05Z","receivedAt":"2009-06-27T12:05:05Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"The alt text, set to the avatar provider (i.e. image type), is more\nfriendly than the img url in some text-only browsers.\n---\n gitweb/gitweb.perl | 1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex e2638cb..dc6049a 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1578,6 +1578,7 @@ sub git_get_avatar {\n \t\t \"\" . $post_white;\n \t} else {\n \t\treturn \"\";\n-- \n1.6.3.rc1.192.gdbfcb\n"},{"id":"117087","messageId":"200906271624.52372.jnareb@gmail.com","threadId":"19948","inReplyTo":"1246104305-15191-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv7 1/9] gitweb: refactor author name insertion","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-27T14:24:51Z","receivedAt":"2009-06-27T14:24:51Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sat, 27 June 2009, Giuseppe Bilotta wrote:\n\n> Collect all author display code in appropriate functions, making it\n> easier to extend these functions on the CGI side.\n> \n> We also move some of the presentation code from hard-coded HTML to CSS,\n> for easier customization.\n\nVery nicely written, compact yet comprehensive commit message.\n\nIt _might_ be worth adding that accidentally commit date got \"atnight\"\nwarning too. I consider that improvement, even though for local commit\n(committer == author) it duplicates information. And it removes\n(some of) code duplication.\n\n> \n> Signed-off-by: Giuseppe Bilotta \n\nIf not for the fact that due to typo you lost localtime info from\n'commitdiff' view, I would ACK it.\n\n> ---\n> gitweb/gitweb.css | 5 ++-\n> gitweb/gitweb.perl | 96 +++++++++++++++++++++++++++++++--------------------\n> 2 files changed, 62 insertions(+), 39 deletions(-)\n\n[...]\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 1e7e2d8..9be723c 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -1469,6 +1469,16 @@ sub format_subject_html {\n> \t}\n> }\n> \n> +# format the author name of the given commit with the given tag\n> +# the author name is chopped and escaped according to the other\n> +# optional parameters (see chop_str).\n> +sub format_author_html {\n> +\tmy $tag = shift;\n> +\tmy $co = shift;\n> +\tmy $author = chop_and_escape_str($co->{'author_name'}, @_);\n> +\treturn \"<$tag class=\\\"author\\\">\" . $author . \"\";\n> +}\n\nIn the future we might want to change it so instead of\n\n <$tag class=\"author\">Joe Random... \n\nwe would use 'title' attribute of $tag element\n\n <$tag class=\"author\" title=\"Joe Random Hacker\">Joe Random... \n\nBut I guess this is not worth complicating code required to do this.\n\n> +\n> # format git diff header line, i.e. \"diff --(git|combined|cc) ...\"\n> sub format_git_diff_header_line {\n> \tmy $line = shift;\n> @@ -3214,21 +3224,53 @@ sub git_print_header_div {\n> \t \"\\n
\\n\";\n> }\n> \n> +# Outputs the author name and date in long form\n> sub git_print_authorship {\n> \tmy $co = shift;\n> +\tmy %opts = @_;\n> +\tmy $tag = $opts{-tag} || 'div';\n\nBTW. gitweb is a bit inconsistent here. Sometimes we use $opts{'-tag'}\nform, and sometimes $opts{-tag} form like you use here.\n\n(Actually we use '-' prefixing i.e. '-key' not 'key' to make it safe\nto write $opts{-key} and -key => 1).\n\n> \n> \tmy %ad = parse_date($co->{'author_epoch'}, $co->{'author_tz'});\n> -\tprint \"
\" .\n> +\tprint \"<$tag class=\\\"author_date\\\">\" .\n> \t esc_html($co->{'author_name'}) .\n> \t \" [$ad{'rfc2822'}\";\n> -\tif ($ad{'hour_local'} < 6) {\n> -\t\tprintf(\" (%02d:%02d %s)\",\n> -\t\t $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});\n> -\t} else {\n> -\t\tprintf(\" (%02d:%02d %s)\",\n> -\t\t $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});\n> +\tif ($opts{-localtime}) {\n> +\t\tif ($ad{'hour_local'} < 6) {\n> +\t\t\tprintf(\" (%02d:%02d %s)\",\n> +\t\t\t $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});\n> +\t\t} else {\n> +\t\t\tprintf(\" (%02d:%02d %s)\",\n> +\t\t\t $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});\n> +\t\t}\n> +\t}\n\nBy the way, this \"atnight\" warning is duplicated in subroutine below\ngit_print_authorship_rows(), but IMHO it is not something very important.\nIt can be left for later.\n\n> +\tprint \"]\\n\";\n> +}\n> +\n> +# Outputs table rows containing the full author or committer information,\n> +# in the format expected for 'commit' view (& similia).\n> +# Parameters are a commit hash reference, followed by the list of people\n> +# to output information for. If the list is empty it defalts to both\n> +# author and committer.\n> +sub git_print_authorship_rows {\n> +\tmy $co = shift;\n> +\t# too bad we can't use @people = @_ || ('author', 'committer')\n> +\tmy @people = @_;\n> +\t@people = ('author', 'committer') unless @people;\n> +\tforeach my $who (@people) {\n> +\t\tmy %ad = parse_date($co->{$who . '_epoch'}, $co->{$who . '_tz'});\n\nWouldn't\n\n $co->{\"${who}_epoch\"}\n\nbe simpler than\n\n $co->{$who . '_epoch'}\n\nAlso it is now %wd (%who_date) or just %date rather than \n%ad (%author_date) vs %cd (%committer_date).\n\n\nBoth of those issues are very, very minor.\n\n> +\t\tprint \"$who\" . esc_html($co->{$who}) . \"\\n\".\n> +\t\t \"\" .\n> +\t\t \" $ad{'rfc2822'}\";\n\n> +\t\tif ($ad{'hour_local'} < 6) {\n> +\t\t\tprintf(\" (%02d:%02d %s)\",\n> +\t\t\t $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});\n> +\t\t} else {\n> +\t\t\tprintf(\" (%02d:%02d %s)\",\n> +\t\t\t $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});\n> +\t\t}\n\nThis is a bit of code duplication with git_print_authorship(),\nbut I don't think it is something seriously to worry about.\n\n> +\t\tprint \"\" .\n> +\t\t \"\\n\";\n> \t}\n\n> -\tprint \"]
\\n\";\n\nHeh. Strange quirks of diff algorithm... :-)\n\n> }\n> \n> sub git_print_page_path {\n> @@ -4142,11 +4184,9 @@ sub git_shortlog_body {\n> \t\t\tprint \"\\n\";\n> \t\t}\n> \t\t$alternate ^= 1;\n> -\t\tmy $author = chop_and_escape_str($co{'author_name'}, 10);\n> \t\t# git_summary() used print \"$co{'age_string'}\\n\" .\n> \t\tprint \"$co{'age_string_date'}\\n\" .\n> -\t\t \"\" . $author . \"\\n\" .\n> -\t\t \"\";\n> +\t\t format_author_html('td', \\%co, 10) . \"\";\n> \t\tprint format_subject_html($co{'title'}, $co{'title_short'},\n> \t\t href(action=>\"commit\", hash=>$commit), $ref);\n> \t\tprint \"\\n\" .\n\nNice.\n\n> @@ -4193,11 +4233,9 @@ sub git_history_body {\n> \t\t\tprint \"\\n\";\n> \t\t}\n> \t\t$alternate ^= 1;\n> -\t# shortlog uses chop_str($co{'author_name'}, 10)\n> -\t\tmy $author = chop_and_escape_str($co{'author_name'}, 15, 3);\n> \t\tprint \"$co{'age_string_date'}\\n\" .\n> -\t\t \"\" . $author . \"\\n\" .\n> -\t\t \"\";\n> +\t# shortlog: format_author_html('td', \\%co, 10)\n> +\t\t format_author_html('td', \\%co, 15, 3) . \"\";\n> \t\t# originally git_history used chop_str($co{'title'}, 50)\n> \t\tprint format_subject_html($co{'title'}, $co{'title_short'},\n> \t\t href(action=>\"commit\", hash=>$commit), $ref);\n\nNice. Good, well aligned comment.\n\n> @@ -4350,9 +4388,8 @@ sub git_search_grep_body {\n> \t\t\tprint \"\\n\";\n> \t\t}\n> \t\t$alternate ^= 1;\n> -\t\tmy $author = chop_and_escape_str($co{'author_name'}, 15, 5);\n> \t\tprint \"$co{'age_string_date'}\\n\" .\n> -\t\t \"\" . $author . \"\\n\" .\n> +\t\t format_author_html('td', \\%co, 15, 5) .\n> \t\t \"\" .\n> \t\t $cgi->a({-href => href(action=>\"commit\", hash=>$co{'id'}),\n> \t\t -class => \"list subject\"},\n\nNice... although this set of diff chunks made me wonder why we\nuse \"10\" for 'shortlog', \"15, 3\" for 'history, and \"15, 5\" for \n'search' (grep).\n\n> @@ -5094,9 +5131,9 @@ sub git_log {\n> \t\t \" | \" .\n> \t\t $cgi->a({-href => href(action=>\"tree\", hash=>$commit, hash_base=>$commit)}, \"tree\") .\n> \t\t \"
\\n\" .\n> -\t\t \"
\\n\" .\n> -\t\t \"\" . esc_html($co{'author_name'}) . \" [$ad{'rfc2822'}]
\\n\" .\n> \t\t \"\\n\";\n> +\t\t git_print_authorship(\\%co, -tag => 'span');\n> +\t\t print \"
\\n\\n\";\n> \n> \t\tprint \"
\\n\";\n> \t\tgit_print_log($co{'comment'}, -final_empty_line=> 1);\n\nThis preserves layout. Good.\n\nI see you choose to correct mentioned issue this way.\n\n> @@ -5115,8 +5152,6 @@ sub git_commit {\n> \t$hash ||= $hash_base || \"HEAD\";\n> \tmy %co = parse_commit($hash)\n> \t or die_error(404, \"Unknown commit object\");\n> -\tmy %ad = parse_date($co{'author_epoch'}, $co{'author_tz'});\n> -\tmy %cd = parse_date($co{'committer_epoch'}, $co{'committer_tz'});\n> \n> \tmy $parent = $co{'parent'};\n> \tmy $parents = $co{'parents'}; # listref\n> @@ -5183,22 +5218,7 @@ sub git_commit {\n> \t}\n> \tprint \"
\\n\" .\n> \t \"\\n\";\n> -\tprint \"\\n\".\n> -\t \"\" .\n> -\t \"\" .\n> -\t \"\\n\";\n> -\tprint \"\\n\";\n> -\tprint \"\\n\";\n> +\tgit_print_authorship_rows(\\%co);\n> \tprint \"\\n\";\n> \tprint \"\" .\n> \t \"\" .\n\nNice simplification. Even without making 'commitdiff' use the same\nauthorship info layout like 'commit', and 'tag' reusing this subroutine\nit might have been worth it.\n\n> @@ -5579,7 +5599,7 @@ sub git_commitdiff {\n> \t\tgit_header_html(undef, $expires);\n> \t\tgit_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash, $formats_nav);\n> \t\tgit_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash);\n> -\t\tgit_print_authorship(\\%co);\n> +\t\tgit_print_authorship(\\%co, 'localtime' => 1);\n> \t\tprint \"
\\n\";\n> \t\tif (@{$co{'comment'}} > 1) {\n> \t\t\tprint \"
\\n\";\n\nATTENTION !!!\n\nYou should have\n\n -\t\tgit_print_authorship(\\%co);\n +\t\tgit_print_authorship(\\%co, '-localtime' => 1);\n\nhere ('-localtime' instead of 'localtime'), otherwise you will loose\nlocaltime information (and \"atnight\" warning) from 'commitdiff' view.\n\n\nThanks again for your diligent work on this patch series!\n\n-- \nJakub Narebski\nPoland\n"},{"id":"117089","messageId":"cb7bb73a0906270826y45f3f9f1lec720aa4dad2a237@mail.gmail.com","threadId":"19948","inReplyTo":"200906271624.52372.jnareb@gmail.com","subject":"Re: [PATCHv7 1/9] gitweb: refactor author name insertion","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-27T15:26:29Z","receivedAt":"2009-06-27T15:26:29Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2009/6/27 Jakub Narebski :\n> By the way, this \"atnight\" warning is duplicated in subroutine below\n> git_print_authorship_rows(), but IMHO it is not something very important.\n> It can be left for later.\n\nI'll resend this patch, with this thing refactored too, and with the\nlocaltime fix.\n\n>> +     foreach my $who (@people) {\n>> +             my %ad = parse_date($co->{$who . '_epoch'}, $co->{$who . '_tz'});\n>\n> Wouldn't\n>\n>  $co->{\"${who}_epoch\"}\n>\n> be simpler than\n>\n>  $co->{$who . '_epoch'}\n>\n> Also it is now %wd (%who_date) or just %date rather than\n> %ad (%author_date) vs %cd (%committer_date).\n>\n>\n> Both of those issues are very, very minor.\n\nBut I can tune them out 8-)\n\n> Nice... although this set of diff chunks made me wonder why we\n> use \"10\" for 'shortlog', \"15, 3\" for 'history, and \"15, 5\" for\n> 'search' (grep).\n\nI don't know about that. It might be worth considering unifying them.\n\n> Thanks again for your diligent work on this patch series!\n\nThank you very much for your comments. I could clearly see the code\nimproving significantly at each iteration. It's a great sastisfaction,\neven though it's quite some work.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"117090","messageId":"cb7bb73a0906270848q64c76bb3ye24a2d5388cf3c6c@mail.gmail.com","threadId":"19948","inReplyTo":"cb7bb73a0906270826y45f3f9f1lec720aa4dad2a237@mail.gmail.com","subject":"Re: [PATCHv7 1/9] gitweb: refactor author name insertion","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-27T15:48:33Z","receivedAt":"2009-06-27T15:48:33Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Sat, Jun 27, 2009 at 5:26 PM, Giuseppe\nBilotta wrote:\n> 2009/6/27 Jakub Narebski :\n>> By the way, this \"atnight\" warning is duplicated in subroutine below\n>> git_print_authorship_rows(), but IMHO it is not something very important.\n>> It can be left for later.\n>\n> I'll resend this patch, with this thing refactored too, and with the\n> localtime fix.\n\nEh, the refactoring caused a chain of changes in some of the following\npatches, so I'll wait a bit before resending. In the mean time, the\nnew patchset can be found here:\nhttp://git.oblomov.eu/git/shortlog/next..refs/tags/gitweb/avatar7bis\n\n[BTW, for some reason\nhttp://git.oblomov.eu/git/shortlog/next..gitweb/avatar7bis gives a 404\ninstead]\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"117096","messageId":"200906271810.23119.jnareb@gmail.com","threadId":"19948","inReplyTo":"1246104305-15191-3-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv7 2/9] gitweb: uniform author info for commit and commitdiff","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-27T16:10:22Z","receivedAt":"2009-06-27T16:10:22Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sat, 27 June 2009, Giuseppe Bilotta wrote:\n> Switch from\n> \n> A U Thor [date time]\n> \n> to\n> \n> author\tA U Thor \n> \tdate time\n> committer\tC O Mitter \n> \tcommitter date time\n\nI would use:\n\n Switch from form similar to the one used by 'log' view\n\n \tA U Thor [date time]\n\n to the form used in 'commit' view\n\n\tauthor A U Thor \n \t \t date time\n \tcommitter C O Mitter \n \t date time\n\n(i.e. use spaces and not tabs to align). But this is minor\nissue, not worth worrying about IMVHO.\n\n> \n> Signed-off-by: Giuseppe Bilotta \n\nI am still not sure about this change. On one hand side it is unifying\nof 'commit' and 'commitdiff' view; most other web interfaces have single\nview equivalent to git-show, which displays both commit info, and the\ndiff. (And 'commitdiff' with 'hp' parameter i.e. between two commits\nhas to be redesigned anyway, so this issue doesn't enter this \nconsideration).\n\nOn the other hand side IIRC 'commitdiff' uses short (one-line) \nauthorship info because the main point is the diff, and multi-line\nauthor and commit info like the one used in 'commit' view takes\na bit of vertical space. Also one can use similarity between\n'log' and 'commitdiff' views (git-log and git-show) as a counter\nfor argument that 'commitdiff' has to look like 'commit'.\n\nBut otherwise I quite like this patch.\n\n> ---\n> gitweb/gitweb.perl | 6 +++++-\n> 1 files changed, 5 insertions(+), 1 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 9be723c..0d8005d 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -5599,7 +5599,11 @@ sub git_commitdiff {\n> \t\tgit_header_html(undef, $expires);\n> \t\tgit_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash, $formats_nav);\n> \t\tgit_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash);\n> -\t\tgit_print_authorship(\\%co, 'localtime' => 1);\n> +\t\tprint \"
\\n\" .\n> +\t\t \"
author\" . esc_html($co{'author'}) . \"
$ad{'rfc2822'}\";\n> -\tif ($ad{'hour_local'} < 6) {\n> -\t\tprintf(\" (%02d:%02d %s)\",\n> -\t\t $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});\n> -\t} else {\n> -\t\tprintf(\" (%02d:%02d %s)\",\n> -\t\t $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});\n> -\t}\n> -\tprint \"
committer\" . esc_html($co{'committer'}) . \"
$cd{'rfc2822'}\" .\n> -\t sprintf(\" (%02d:%02d %s)\", $cd{'hour_local'}, $cd{'minute_local'}, $cd{'tz_local'}) .\n> -\t \"
commit$co{'id'}
tree
\\n\";\n> +\t\tgit_print_authorship_rows(\\%co);\n> +\t\tprint \"
\".\n> +\t\t \"
\\n\";\n> \t\tprint \"
\\n\";\n> \t\tif (@{$co{'comment'}} > 1) {\n> \t\t\tprint \"
\\n\";\n\nNice and short, thanks to earlier (re)factoring.\n\nBTW. after this change the -localtime part of git_print_authorship()\nsubroutine is unused... just saying ;-) Not something terribly \nimportant.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"117098","messageId":"200906272010.30754.jnareb@gmail.com","threadId":"19948","inReplyTo":"1246104305-15191-4-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv7 3/9] gitweb: use git_print_authorship_rows in 'tag' view too","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-27T18:10:29Z","receivedAt":"2009-06-27T18:10:29Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sat, 27 June 2009, Giuseppe Bilotta wrote:\n\n> parse_tag has to be adapted to use the hash keys expected by\n> git_print_authorship_rows, which is not a problem since git_tag\n> is the only user of this sub.\n\nNitpick: I think that with s/has/had/ (past tense) and \ns/, which/. This/ (split paragraph into two sentences)\nit would read better.\n\n> \n> Signed-off-by: Giuseppe Bilotta \n\nAcked-by: Jakub Narebski \n\n> ---\n> gitweb/gitweb.perl | 16 +++++++++-------\n> 1 files changed, 9 insertions(+), 7 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 0d8005d..7183ad2 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -2409,8 +2409,14 @@ sub parse_tag {\n> \t\t\t$tag{'name'} = $1;\n> \t\t} elsif ($line =~ m/^tagger (.*) ([0-9]+) (.*)$/) {\n> \t\t\t$tag{'author'} = $1;\n> -\t\t\t$tag{'epoch'} = $2;\n> -\t\t\t$tag{'tz'} = $3;\n> +\t\t\t$tag{'author_epoch'} = $2;\n> +\t\t\t$tag{'author_tz'} = $3;\n> +\t\t\tif ($tag{'author'} =~ m/^([^<]+) <([^>]*)>/) {\n> +\t\t\t\t$tag{'author_name'} = $1;\n> +\t\t\t\t$tag{'author_email'} = $2;\n> +\t\t\t} else {\n> +\t\t\t\t$tag{'author_name'} = $tag{'author'};\n> +\t\t\t}\n> \t\t} elsif ($line =~ m/--BEGIN/) {\n> \t\t\tpush @comment, $line;\n> \t\t\tlast;\n\nSidenote: I wonder if it would be worth doing to factor out code dealing\nwith extracting data from (parsing) author/committer/tagger object headers.\nProbably not worth it...\n\n> @@ -4623,11 +4629,7 @@ sub git_tag {\n> \t $tag{'type'}) . \"\\n\" .\n> \t \"\\n\";\n> \tif (defined($tag{'author'})) {\n> -\t\tmy %ad = parse_date($tag{'epoch'}, $tag{'tz'});\n> -\t\tprint \"author\" . esc_html($tag{'author'}) . \"\\n\";\n> -\t\tprint \"\" . $ad{'rfc2822'} .\n> -\t\t\tsprintf(\" (%02d:%02d %s)\", $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'}) .\n> -\t\t\t\"\\n\";\n> +\t\tgit_print_authorship_rows(\\%tag, 'author');\n> \t}\n> \tprint \"\\n\\n\" .\n> \t \"
\\n\";\n\nI wonder why we used and use 'author' instead of 'tagger'...\n\n-- \nJakub Narebski\nPoland\n"},{"id":"117099","messageId":"200906272028.05239.jnareb@gmail.com","threadId":"19948","inReplyTo":"1246104305-15191-5-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv7 4/9] gitweb: right-align date cell in shortlog","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-27T18:28:04Z","receivedAt":"2009-06-27T18:28:04Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sat, 27 June 2009, Giuseppe Bilotta wrote:\n\n> Signed-off-by: Giuseppe Bilotta \n\nI don't like it. In my opinion it is less readable that way, especially\nif word wrapping gets involved:\n\n 5 hours |\n ago |\n\n 5 hours |\n ago |\n\n 5 days |\n ago |\n\nIt is IMVHO a bit ugly.\n\n\nThat aside this is *not complete*. Take a look at 'summary' view. \nYou have there 'date' column in 'shortlog' section aligned to the right,\nwhile in 'heads' and 'tags' section it is aligned to the left. Add to\nthat \"Last Change\" column in projects list view (which should probably\nbe aligned to the left, even with this patch completed).\n\nNAK from me. (Alternate stylesheet? Just kidding...)\n\n> ---\n> gitweb/gitweb.css | 4 ++++\n> 1 files changed, 4 insertions(+), 0 deletions(-)\n> \n> diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css\n> index 68b22ff..ef24a1b 100644\n> --- a/gitweb/gitweb.css\n> +++ b/gitweb/gitweb.css\n> @@ -180,6 +180,10 @@ table {\n> \tborder-spacing: 0;\n> }\n> \n> +table.shortlog td:first-child {\n> +\ttext-align: right;\n> +}\n> +\n> table.diff_tree {\n> \tfont-family: monospace;\n> }\n\nOn the other hand: it is short and simple.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"117100","messageId":"7vskhlikt7.fsf@alter.siamese.dyndns.org","threadId":"19948","inReplyTo":"200906271810.23119.jnareb@gmail.com","subject":"Re: [PATCHv7 2/9] gitweb: uniform author info for commit and commitdiff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-06-27T18:38:28Z","receivedAt":"2009-06-27T18:38:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> I would use:\n>\n> Switch from form similar to the one used by 'log' view\n>\n> \tA U Thor [date time]\n>\n> to the form used in 'commit' view\n>\n> \tauthor A U Thor \n> \t \t date time\n> \tcommitter C O Mitter \n> \t date time\n>\n> (i.e. use spaces and not tabs to align). But this is minor\n> issue, not worth worrying about IMVHO.\n\nThese extra blank lines make things much easier to read, and explicit\nmention of switching from WHAT to WHAT ELSE is very much appreciated.\n\n> On the other hand side IIRC 'commitdiff' uses short (one-line) \n> authorship info because the main point is the diff, and multi-line\n> author and commit info like the one used in 'commit' view takes\n> a bit of vertical space.\n\nIn general, we might want to make the committer information less prominent\nthan it currently is.\n\nWhen looking at a repository that is used like CVS, author and committer\nare always the same. When looking at a repository that is owned by a\nsingle integrator, the committer is a single person. The only time\ncommitter information for every commit would help is when viewing a\nrepository of higher level integrator in a project that has subintegrators\n(e.g. Linus pulls from David who commits patches from others).\n\nSo at some point, it may not be a bad idea to introduce a per-repository\noption/feature to hide committer information from certain views to\nallocate more space for other information.\n\nBut certainly that shouldn't be a part of this topic.\n"},{"id":"117104","messageId":"200906272145.46506.jnareb@gmail.com","threadId":"19948","inReplyTo":"1246104305-15191-6-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv7 5/9] gitweb: (gr)avatar support","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-27T19:45:46Z","receivedAt":"2009-06-27T19:45:46Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sat, 27 June 2009, Giuseppe Bilotta wrote:\n\n> Introduce avatar support: the feature adds the appropriate img tag next\n> to author and committer in commit(diff), history, shortlog, log and tag\n> views. Multiple avatar providers are possible, but only gravatar is\n> implemented at the moment.\n> \n> Gravatar support depends on Digest::MD5, which is a core package since\n> Perl 5.8. If gravatars are activated but Digest::MD5 cannot be found,\n> the feature will be automatically disabled.\n> \n> No avatar provider is selected by default, except in the t9500 test.\n\nThis is well written commit message. Good work!\n\n> \n> Signed-off-by: Giuseppe Bilotta \n> ---\n> gitweb/gitweb.css | 4 ++\n> gitweb/gitweb.perl | 81 ++++++++++++++++++++++++++++++-\n> t/t9500-gitweb-standalone-no-errors.sh | 2 +\n\nPlease, now that you added this to t9500 to be able to test gravatar\nand avatar code, run t9500-gitweb-standalone-no-errors.sh script...\n\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 7183ad2..ad9ae31 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -195,6 +195,14 @@ our %known_snapshot_format_aliases = (\n> \t'x-zip' => undef, '' => undef,\n> );\n> \n> +# Pixel sizes for icons and avatars. If the default font sizes or lineheights\n> +# are changed, it may be appropriate to change these values too via\n> +# $GITWEB_CONFIG.\n> +our %avatar_size = (\n> +\t'default' => 16,\n> +\t'double' => 32\n> +);\n\nNice. Good and complete explanation.\n\n[...]\n> +\t# Avatar support. When this feature is enabled, views such as\n> +\t# shortlog or commit will display an avatar associated with\n> +\t# the email of the committer(s) and/or author(s).\n> +\n> +\t# Currently only the gravatar provider is available, and it\n> +\t# depends on Digest::MD5.\n> +\n> +\t# To enable system wide have in $GITWEB_CONFIG\n> +\t# $feature{'avatar'}{'default'} = ['gravatar'];\n> +\t# To have project specific config enable override in $GITWEB_CONFIG\n> +\t# $feature{'avatar'}{'override'} = 1;\n> +\t# and in project config gitweb.avatar = gravatar;\n> +\t'avatar' => {\n> +\t\t'sub' => \\&feature_avatar,\n> +\t\t'override' => 0,\n> +\t\t'default' => ['']},\n> );\n\nSo you have chosen [''] and not [] as \"no avatar\" value, to simplify\nlater code. All right.\n\n> \n> sub gitweb_get_feature {\n> @@ -433,6 +458,16 @@ sub feature_patches {\n> \treturn ($_[0]);\n> }\n> \n> +sub feature_avatar {\n> +\tmy @val = (git_get_project_config('avatar'));\n> +\n> +\tif (@val) {\n> +\t\treturn @val;\n> +\t}\n> +\n> +\treturn @_;\n> +}\n\nHmmm... why not simply\n\n +\treturn @val ? @val : @_;\n\n\nBy the way, we might want to accept 'none' instead of empty value\nor no value to turn off avatar support for specific project (if\navatars are turned on globally, and project specific override is on).\nWe use this technique for 'snapshot' feature.\n\nBut this isn't terribly important.\n\n[...]\n> @@ -814,6 +849,17 @@ $git_dir = \"$projectroot/$project\" if $project;\n> our @snapshot_fmts = gitweb_get_feature('snapshot');\n> @snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);\n> \n> +# check that the avatar feature is set to a known provider name,\n> +# and for each provider check if the dependencies are satisfied.\n> +# if the provider name is invalid or the dependencies are not met,\n> +# reset $git_avatar to the empty string.\n> +our ($git_avatar) = gitweb_get_feature('avatar');\n> +if ($git_avatar eq 'gravatar') {\n> +\t$git_avatar = '' unless (eval { require Digest::MD5; 1; });\n> +} else {\n> +\t$git_avatar = '';\n> +}\n\nThoughts for the future: this can lead to not very pretty if-elsif\nchain. We have replaces such chain for selecting action (for dispatch)\nby using %actions hash of subroutine refs (as a kind of 'switch'/'given'\nstatement). We could do the same for avatar provider initialization\nand validation subroutines.\n\nBut for now it is clear enough. Don't worry about this issue now.\n\n> @@ -1469,6 +1515,29 @@ sub format_subject_html {\n> \t}\n> }\n> \n> +# Insert an avatar for the given $email at the given $size if the feature\n> +# is enabled.\n> +sub git_get_avatar {\n> +\tmy ($email, %opts) = @_;\n> +\tmy $pre_white = ($opts{'pad_before'} ? \" \" : \"\");\n> +\tmy $post_white = ($opts{'pad_after'} ? \" \" : \"\");\n> +\tmy $size = $avatar_size{$opts{'size'}} || $avatar_size{'default'};\n\nRunning t9500-gitweb-standalone-no-errors.sh with --debug option shows\nfailing of 15 among 87 tests, with error:\n\n gitweb.perl: Use of uninitialized value in hash element at gitweb/gitweb.perl line 1524.\n\nwhich is the above line. This line should read:\n\n +\tmy $size = exists $opts{'size'}\n +\t\t? $avatar_size{$opts{'size'}} || $avatar_size{'default'}\n +\t\t: $avatar_size{'default'};\n\nor something like that, e.g.:\n\n +\tmy $size = $avatar_size{ defined $opts{'size'} ? $opts{'size'} : 'default' }\n +\t\t|| $avatar_size{'default'};\n\n\nBTW. didn't we agree on '-size' and '-pad_before' convention? \n\n> +\tmy $url = \"\";\n> +\tif ($git_avatar eq 'gravatar') {\n> +\t\t$url = \"http://www.gravatar.com/avatar.php?gravatar_id=\" .\n> +\t\t\tDigest::MD5::md5_hex(lc $email) . \"&size=$size\";\n\nWhy not use the new API[1][2]?:\n\n +\t\t$url = \"http://www.gravatar.com/avatar/\" .\n +\t\t\tDigest::MD5::md5_hex(lc $email) . \"?size=$size\";\n\n[1] http://www.gravatar.com/site/implement/url\n[2] http://search.cpan.org/perldoc/Gravatar::URL\n http://p3rl.org/Gravatar::URL\n\n> +\t}\n> +\t# Currently only gravatars are supported, but other forms such as\n> +\t# picons can be added by putting an else up here and defining $url\n> +\t# as needed. If no variant puts something in $url, we assume avatars\n> +\t# are completely disabled/unavailable.\n> +\tif ($url) {\n> +\t\treturn $pre_white . \"\" . $post_white;\n> +\t} else {\n> +\t\treturn \"\";\n> +\t}\n> +}\n\nGood idea, and nice description.\n\n[...]\n> @@ -1476,7 +1545,9 @@ sub format_author_html {\n> \tmy $tag = shift;\n> \tmy $co = shift;\n> \tmy $author = chop_and_escape_str($co->{'author_name'}, @_);\n> -\treturn \"<$tag class=\\\"author\\\">\" . $author . \"\";\n> +\treturn \"<$tag class=\\\"author\\\">\" .\n> +\t git_get_avatar($co->{'author_email'}, 'pad_after' => 1) .\n> +\t $author . \"\";\n> }\n> \n> # format git diff header line, i.e. \"diff --(git|combined|cc) ...\"\n> @@ -3249,7 +3320,8 @@ sub git_print_authorship {\n> \t\t\t $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});\n> \t\t}\n> \t}\n> -\tprint \"]\\n\";\n> +\tprint \"]\" . git_get_avatar($co->{'author_email'}, 'pad_before' => 1)\n> +\t\t . \"\\n\";\n> }\n> \n> # Outputs table rows containing the full author or committer information,\n> @@ -3264,7 +3336,10 @@ sub git_print_authorship_rows {\n> \t@people = ('author', 'committer') unless @people;\n> \tforeach my $who (@people) {\n> \t\tmy %ad = parse_date($co->{$who . '_epoch'}, $co->{$who . '_tz'});\n> -\t\tprint \"$who\" . esc_html($co->{$who}) . \"\\n\".\n> +\t\tprint \"$who\" . esc_html($co->{$who}) . \"\" .\n> +\t\t \"\" .\n> +\t\t git_get_avatar($co->{$who . '_email'}, 'size' => 'double') .\n\nNitpick: you can use \"${who}_email\", but I am not sure if it is more\nreadable than $who . '_email' form.\n\n> +\t\t \"\\n\" .\n> \t\t \"\" .\n> \t\t \" $ad{'rfc2822'}\";\n> \t\tif ($ad{'hour_local'} < 6) {\n\nShort and nice, thanks to refactoring done in earlier patches in this\nseries. Very good.\n\n> diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh\n> index d539619..6275181 100755\n> --- a/t/t9500-gitweb-standalone-no-errors.sh\n> +++ b/t/t9500-gitweb-standalone-no-errors.sh\n> @@ -660,6 +660,7 @@ cat >>gitweb_config.perl < \n> \\$feature{'blame'}{'override'} = 1;\n> \\$feature{'snapshot'}{'override'} = 1;\n> +\\$feature{'avatar'}{'override'} = 1;\n> EOF\n> \n> test_expect_success \\\n> @@ -671,6 +672,7 @@ test_expect_success \\\n> \t'config override: tree view, features disabled in repo config' \\\n> \t'git config gitweb.blame no &&\n> \t git config gitweb.snapshot none &&\n> +\t git config gitweb.avatar gravatar &&\n> \t gitweb_run \"p=.git;a=tree\"'\n> test_debug 'cat gitweb.log'\n> \n\nPlease run it too :-)\n\n\nApart from this issue of Perl warning (which would get logged, but \notherwise is not an error), and using old Gravatar API, this patch\nshapes very nicely, thanks to all the work done earlier on refactoring.\nI like it very much.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"117107","messageId":"200906280015.31639.jnareb@gmail.com","threadId":"19948","inReplyTo":"1246104305-15191-7-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv7 6/9] gitweb: gravatar url cache","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-27T22:15:30Z","receivedAt":"2009-06-27T22:15:30Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sat, 27 June 2009, Giuseppe Bilotta wrote:\n\n> Views which contain many occurrences of the same email address (e.g.\n> shortlog view) benefit from not having to recalculate the MD5 of the\n> email address every time.\n> \n> Signed-off-by: Giuseppe Bilotta \n\nAcked-by: Jakub Narebski \n\n> ---\n\nHere there are some very simple benchmarks of the effect of gravatar \nURL cache. I'm not sure if they should be put in commit message.\n\nWhen running gitweb as a script\n $ time gitweb-run.sh \"p=git.git;a=shortlog\" >/dev/null\nI got the following results\n\n | before | after \n ------------+-----------------|------------------\n summary | 0.978s (0.964s) | 0.961s (0.940s)\n shortlog | 1.001s (0.980s) | 1.033s (1.008s)\n\nwhich means the same results within the margin of error, (as seen when\nrepeating benchmark the differences are within the range of fluctuations).\nTime is real (wallclock) time, in parentheses there is user + sys time.\n\n\nWhen using ApacheBench 2.0.41-dev, with gitweb run as CGI script\n(not as legacy, i.e. ModPerl::Registry, mod_perl script)\n $ ab \"http://localhost/cgi-bin/gitweb/gitweb.cgi/git.git/shortlog\"\nI got the folowing results\n\n | before | after \n ------------+-----------------|------------------\n -n 10 | 1094.050 [ms] | 989.136 [ms]\n -n 10 -c 2 | 1147.965 [ms] | 1041.324 [ms]\n\nwhich means around 10% improvement. Standard deviation is around 10 ms\nfor no concurrency, and around 200 ms (more than third of difference)\nfor concurrent connections. Shown here is time per request (mean, \nacross all concurrent requests).\n\n> gitweb/gitweb.perl | 30 +++++++++++++++++++++++++++---\n> 1 files changed, 27 insertions(+), 3 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index ad9ae31..de4cc63 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -1515,6 +1515,27 @@ sub format_subject_html {\n> \t}\n> }\n> \n> +# Rather than recomputing the url for an email multiple times, we cache it\n> +# after the first hit. This gives a visible benefit in views where the avatar\n> +# for the same email is used repeatedly (e.g. shortlog).\n> +# The cache is shared by all avatar engines (currently gravatar only), which\n> +# are free to use it as preferred. Since only one avatar engine is used for any\n> +# given page, there's no risk for cache conflicts.\n> +our %avatar_cache = ();\n\nGood comment.\n\n> +\n> +# Compute the gravatar url for a given email, if it's not in the cache already.\n> +# Gravatar stores only the part of the URL before the size, since that's the\n> +# one computationally more expensive. This also allows reuse of the cache for\n> +# different sizes (for this particular engine).\n> +sub gravatar_url {\n> +\tmy $email = lc shift;\n> +\tmy $size = shift;\n> +\t$avatar_cache{$email} ||=\n> +\t\t\"http://www.gravatar.com/avatar.php?gravatar_id=\" .\n> +\t\t\tDigest::MD5::md5_hex($email) . \"&size=\";\n\nThe same comment as for previous patch: why not use modern API?\n\n +\t\t\"http://www.gravatar.com/avatar/\" .\n +\t\t\tDigest::MD5::md5_hex($email) . \"?size=\";\n\n\n> +\treturn $avatar_cache{$email} . $size;\n> +}\n\nNice.\n\n> +\n> # Insert an avatar for the given $email at the given $size if the feature\n> # is enabled.\n> sub git_get_avatar {\n> @@ -1524,15 +1545,18 @@ sub git_get_avatar {\n> \tmy $size = $avatar_size{$opts{'size'}} || $avatar_size{'default'};\n> \tmy $url = \"\";\n> \tif ($git_avatar eq 'gravatar') {\n> -\t\t$url = \"http://www.gravatar.com/avatar.php?gravatar_id=\" .\n> -\t\t\tDigest::MD5::md5_hex(lc $email) . \"&size=$size\";\n> +\t\t$url = gravatar_url($email, $size);\n> \t}\n\nGreat encapsulation.\n\n> \t# Currently only gravatars are supported, but other forms such as\n> \t# picons can be added by putting an else up here and defining $url\n> \t# as needed. If no variant puts something in $url, we assume avatars\n> \t# are completely disabled/unavailable.\n> \tif ($url) {\n> -\t\treturn $pre_white . \"\" . $post_white;\n> +\t\treturn $pre_white .\n> +\t\t \" +\t\t \"class=\\\"avatar\\\" \" .\n> +\t\t \"src=\\\"$url\\\" \" .\n> +\t\t \"/>\" . $post_white;\n\nThis is independent change. It shouldn't be here; if you prefer such \nsolution, you should squash it with previous patch. Please drop this\nchunk.\n\n> \t} else {\n> \t\treturn \"\";\n> \t}\n> -- \n\nThanks again for great work on this series!\n\n-- \nJakub Narebski\nPoland\n"},{"id":"117108","messageId":"cb7bb73a0906271524k5681ce93pf79680eb728c516e@mail.gmail.com","threadId":"19948","inReplyTo":"200906272010.30754.jnareb@gmail.com","subject":"Re: [PATCHv7 3/9] gitweb: use git_print_authorship_rows in 'tag' view too","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-27T22:24:44Z","receivedAt":"2009-06-27T22:24:44Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2009/6/27 Jakub Narebski :\n> On Sat, 27 June 2009, Giuseppe Bilotta wrote:\n>\n>> parse_tag has to be adapted to use the hash keys expected by\n>> git_print_authorship_rows, which is not a problem since git_tag\n>> is the only user of this sub.\n>\n> Nitpick: I think that with s/has/had/ (past tense) and\n> s/, which/. This/ (split paragraph into two sentences)\n> it would read better.\n\nI can do that.\n\n> Sidenote: I wonder if it would be worth doing to factor out code dealing\n> with extracting data from (parsing) author/committer/tagger object headers.\n> Probably not worth it...\n\nIt might be worth it, but I'd say we can leave it to some other time ;-)\n\n> I wonder why we used and use 'author' instead of 'tagger'...\n\nThe upside of using 'author' is that the tag can then be dealt with\njust like any other commit by the rest of the code, I think. I don't\nknow if this benefit is actually used anywhere in the code.\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"117109","messageId":"cb7bb73a0906271527g60a92daqd20e3c20e2d87842@mail.gmail.com","threadId":"19948","inReplyTo":"200906272028.05239.jnareb@gmail.com","subject":"Re: [PATCHv7 4/9] gitweb: right-align date cell in shortlog","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-27T22:27:50Z","receivedAt":"2009-06-27T22:27:50Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2009/6/27 Jakub Narebski :\n> On Sat, 27 June 2009, Giuseppe Bilotta wrote:\n>\n>> Signed-off-by: Giuseppe Bilotta \n>\n> I don't like it.  In my opinion it is less readable that way, especially\n> if word wrapping gets involved:\n>\n>  5 hours |\n>      ago |\n>\n>  5 hours |\n>      ago |\n>\n>   5 days |\n>      ago |\n>\n> It is IMVHO a bit ugly.\n\nYou might have a point there.\n\n> That aside this is *not complete*.  Take a look at 'summary' view.\n> You have there 'date' column in 'shortlog' section aligned to the right,\n> while in 'heads' and 'tags' section it is aligned to the left.  Add to\n> that \"Last Change\" column in projects list view (which should probably\n> be aligned to the left, even with this patch completed).\n>\n> NAK from me.  (Alternate stylesheet?  Just kidding...)\n\n[...]\n>\n> On the other hand: it is short and simple.\n\nAnd totally independent from the rest of the patchset. I'll move it to\nthe top of the stack, so that the rest of the next iteration can be\napplied in batch as soon as it's ready.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"117110","messageId":"cb7bb73a0906271533g308fd42w82d11cd2658b1e99@mail.gmail.com","threadId":"19948","inReplyTo":"7vskhlikt7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv7 2/9] gitweb: uniform author info for commit and commitdiff","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-27T22:33:53Z","receivedAt":"2009-06-27T22:33:53Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Sat, Jun 27, 2009 at 8:38 PM, Junio C Hamano wrote:\n> Jakub Narebski writes:\n>\n>> I would use:\n>>\n>>   Switch from form similar to the one used by 'log' view\n>>\n>>       A U Thor [date time]\n>>\n>>   to the form used in 'commit' view\n>>\n>>       author       A U Thor \n>>                    date time\n>>       committer    C O Mitter \n>>                    date time\n>>\n>> (i.e. use spaces and not tabs to align).  But this is minor\n>> issue, not worth worrying about IMVHO.\n>\n> These extra blank lines make things much easier to read, and explicit\n> mention of switching from WHAT to WHAT ELSE is very much appreciated.\n\nI updated the patch accordingly.\n\n>> On the other hand side IIRC 'commitdiff' uses short (one-line)\n>> authorship info because the main point is the diff, and multi-line\n>> author and commit info like the one used in 'commit' view takes\n>> a bit of vertical space.\n>\n> In general, we might want to make the committer information less prominent\n> than it currently is.\n\nI've been thinking about this. A possible approach I considered could\nbe to only show the commit _date_ by default (which could be\ninteresting to see), and use some DHTML to show the committer on\nrequest. But as you mention, this is not the patchset to do this kind\nof work on 8-)\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"117111","messageId":"cb7bb73a0906271545h8033f63jf0198a2f9e187428@mail.gmail.com","threadId":"19948","inReplyTo":"200906272145.46506.jnareb@gmail.com","subject":"Re: [PATCHv7 5/9] gitweb: (gr)avatar support","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-27T22:45:00Z","receivedAt":"2009-06-27T22:45:00Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2009/6/27 Jakub Narebski :\n>\n> This is well written commit message.  Good work!\n\nThanks 8-)\n\n>> +     # Avatar support. When this feature is enabled, views such as\n>> +     # shortlog or commit will display an avatar associated with\n>> +     # the email of the committer(s) and/or author(s).\n>> +\n>> +     # Currently only the gravatar provider is available, and it\n>> +     # depends on Digest::MD5.\n>> +\n>> +     # To enable system wide have in $GITWEB_CONFIG\n>> +     # $feature{'avatar'}{'default'} = ['gravatar'];\n>> +     # To have project specific config enable override in $GITWEB_CONFIG\n>> +     # $feature{'avatar'}{'override'} = 1;\n>> +     # and in project config gitweb.avatar = gravatar;\n>> +     'avatar' => {\n>> +             'sub' => \\&feature_avatar,\n>> +             'override' => 0,\n>> +             'default' => ['']},\n>>  );\n>\n> So you have chosen [''] and not [] as \"no avatar\" value, to simplify\n> later code.  All right.\n>\n>>\n>>  sub gitweb_get_feature {\n>> @@ -433,6 +458,16 @@ sub feature_patches {\n>>       return ($_[0]);\n>>  }\n>>\n>> +sub feature_avatar {\n>> +     my @val = (git_get_project_config('avatar'));\n>> +\n>> +     if (@val) {\n>> +             return @val;\n>> +     }\n>> +\n>> +     return @_;\n>> +}\n>\n> Hmmm... why not simply\n>\n>  +     return @val ? @val : @_;\n\nRight. Why not?\n\n> By the way, we might want to accept 'none' instead of empty value\n> or no value to turn off avatar support for specific project (if\n> avatars are turned on globally, and project specific override is on).\n> We use this technique for 'snapshot' feature.\n>\n> But this isn't terribly important.\n\nWell, since 'none' is not a known provider, that trick should work\ncorrectly anyway.\n\n>> +our ($git_avatar) = gitweb_get_feature('avatar');\n>> +if ($git_avatar eq 'gravatar') {\n>> +     $git_avatar = '' unless (eval { require Digest::MD5; 1; });\n>> +} else {\n>> +     $git_avatar = '';\n>> +}\n>\n> Thoughts for the future: this can lead to not very pretty if-elsif\n> chain.  We have replaces such chain for selecting action (for dispatch)\n> by using %actions hash of subroutine refs (as a kind of 'switch'/'given'\n> statement).  We could do the same for avatar provider initialization\n> and validation subroutines.\n>\n> But for now it is clear enough.  Don't worry about this issue now.\n\nI'm also not terribly sure about how to implement _this_ through hash\ncalls. But I did consider the issue (how I wish Perl had an actual\nswitch statement ...)\n\n> Running t9500-gitweb-standalone-no-errors.sh with --debug option shows\n> failing of 15 among 87 tests, with error:\n>\n>   gitweb.perl: Use of uninitialized value in hash element at gitweb/gitweb.perl line 1524.\n>\n> which is the above line.  This line should read:\n>\n>  +     my $size = exists $opts{'size'}\n>  +             ? $avatar_size{$opts{'size'}} || $avatar_size{'default'}\n>  +             : $avatar_size{'default'};\n>\n> or something like that, e.g.:\n>\n>  +     my $size = $avatar_size{ defined $opts{'size'} ? $opts{'size'} : 'default' }\n>  +             || $avatar_size{'default'};\n\nI did $opts{-size} ||= 'default'. Or is it bad form to modify the\npassed option hash?\n\n> BTW. didn't we agree on '-size' and '-pad_before' convention?\n\nYes we did. I don't know why it went back to 'size' etc. Must have\nmessed up something with the patchsets.\n\n>> +     my $url = \"\";\n>> +     if ($git_avatar eq 'gravatar') {\n>> +             $url = \"http://www.gravatar.com/avatar.php?gravatar_id=\" .\n>> +                     Digest::MD5::md5_hex(lc $email) . \"&size=$size\";\n>\n> Why not use the new API[1][2]?:\n\nUpdated.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"117112","messageId":"200906280120.02475.jnareb@gmail.com","threadId":"19948","inReplyTo":"cb7bb73a0906271545h8033f63jf0198a2f9e187428@mail.gmail.com","subject":"Re: [PATCHv7 5/9] gitweb: (gr)avatar support","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-27T23:20:02Z","receivedAt":"2009-06-27T23:20:02Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sun, 28 Jun 2009, Giuseppe Bilotta wrote:\n> 2009/6/27 Jakub Narebski :\n\n>>> +     # Avatar support. When this feature is enabled, views such as\n>>> +     # shortlog or commit will display an avatar associated with\n>>> +     # the email of the committer(s) and/or author(s).\n>>> +\n>>> +     # Currently only the gravatar provider is available, and it\n>>> +     # depends on Digest::MD5.\n[...]\n\n>> By the way, we might want to accept 'none' instead of empty value\n>> or no value to turn off avatar support for specific project (if\n>> avatars are turned on globally, and project specific override is on).\n>> We use this technique for 'snapshot' feature.\n>>\n>> But this isn't terribly important.\n> \n> Well, since 'none' is not a known provider, that trick should work\n> correctly anyway.\n\nAh, I have forgot about this. Perhaps it should be stated more \nexplicitly that unknown provider (type) = no avatar image? Or do I\nsimply not see it?\n\n>>> +our ($git_avatar) = gitweb_get_feature('avatar');\n>>> +if ($git_avatar eq 'gravatar') {\n>>> +     $git_avatar = '' unless (eval { require Digest::MD5; 1; });\n>>> +} else {\n>>> +     $git_avatar = '';\n>>> +}\n>>\n>> Thoughts for the future: this can lead to not very pretty if-elsif\n>> chain.  We have replaces such chain for selecting action (for dispatch)\n>> by using %actions hash of subroutine refs (as a kind of 'switch'/'given'\n>> statement).  We could do the same for avatar provider initialization\n>> and validation subroutines.\n>>\n>> But for now it is clear enough.  Don't worry about this issue now.\n> \n> I'm also not terribly sure about how to implement _this_ through hash\n> calls. But I did consider the issue (how I wish Perl had an actual\n> switch statement ...)\n\nPerl 5.10, Perl 6, Switch.pm... but it isn't something that we can use\nin gitweb. Switch statement (spelled 'given / when') in Perl6 / Perl 5.10\nis very, very powerfull (from what I have read).\n\nBut what I meant here is something like the following:\n\n our %avatar_providers = (\n \t'gravatar' => sub { eval { require Digest::MD5; 1; } },\n \t'picon' => sub { 1; },\n );\n unless (defined $avatar_providers{$git_avatar} &&\n $avatar_providers{$git_avatar}->()) {\n \t$git_avatar = '';\n }\n\nOr something like that (we can use 'undef' for providers without need\nfor initialization). But that can be improved later, \"in tree\".\n\n\nOn the other hand side this solution wouldn't I think be able to deal\nwith specifying fallbacks using e.g. 'gravatar+picon' (gravatar using\npicon URL as fallback i.e. \"default\" argument), or two images side by\nside like GMane uses (if there are two), e.g. 'gravatar picon' or \n'gravatar, picon'.\n\nFood for thought. Not something to act on now.\n\n>> Running t9500-gitweb-standalone-no-errors.sh with --debug option shows\n>> failing of 15 among 87 tests, with error:\n>>\n>>   gitweb.perl: Use of uninitialized value in hash element at gitweb/gitweb.perl line 1524.\n>>\n>> which is the above line.  This line should read:\n>>\n>>  +     my $size = exists $opts{'size'}\n>>  +             ? $avatar_size{$opts{'size'}} || $avatar_size{'default'}\n>>  +             : $avatar_size{'default'};\n>>\n>> or something like that, e.g.:\n>>\n>>  +     my $size = $avatar_size{ defined $opts{'size'} ? $opts{'size'} : 'default' }\n>>  +             || $avatar_size{'default'};\n> \n> I did $opts{-size} ||= 'default'. Or is it bad form to modify the\n> passed option hash?\n\nNo, it is O.K. We do similar thing in href(), deleting keys from hash.\n\n>>> +     my $url = \"\";\n>>> +     if ($git_avatar eq 'gravatar') {\n>>> +             $url = \"http://www.gravatar.com/avatar.php?gravatar_id=\" .\n>>> +                     Digest::MD5::md5_hex(lc $email) . \"&size=$size\";\n>>\n>> Why not use the new API[1][2]?:\n> \n> Updated.\n\nThis affects also the next patch in series.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"117119","messageId":"200906281335.40312.jnareb@gmail.com","threadId":"19948","inReplyTo":"1246104305-15191-8-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv7 7/9] gitweb: picon avatar provider","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-28T11:35:38Z","receivedAt":"2009-06-28T11:35:38Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sat, 27 June 2009, Giuseppe Bilotta wrote:\n\n> Simple implementation of picon that only relies on the indiana.edu\n> database.\n\nI'd like to see where you got information about picons. But it is\nnot necessary to have in commit message.\n\n... Ah, I'm sorry, it is stated in comment. Nevermind then.\n\n> \n> Signed-off-by: Giuseppe Bilotta \n\nAcked-by: Jakub Narebski \n\n> ---\n> gitweb/gitweb.perl | 26 ++++++++++++++++++++++----\n> 1 files changed, 22 insertions(+), 4 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index de4cc63..ae73d45 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -378,14 +378,17 @@ our %feature = (\n> \t# shortlog or commit will display an avatar associated with\n> \t# the email of the committer(s) and/or author(s).\n> \n> -\t# Currently only the gravatar provider is available, and it\n> -\t# depends on Digest::MD5.\n> +\t# Currently available providers are gravatar and picon.\n> +\n> +\t# Gravatar depends on Digest::MD5.\n> +\t# Picon currently relies on the indiana.edu database.\n> \n> \t# To enable system wide have in $GITWEB_CONFIG\n> -\t# $feature{'avatar'}{'default'} = ['gravatar'];\n> +\t# $feature{'avatar'}{'default'} = ['provider'];\n> +\t# where provider is either gravatar or picon.\n\nI wonder if it wouldn't be better to use \"\" (or \"''\")\nin place of \"'provider'\", as it is not about literal 'provider', but \nabout one of 'gravatar' or 'picon'.\n\n> \t# To have project specific config enable override in $GITWEB_CONFIG\n> \t# $feature{'avatar'}{'override'} = 1;\n> -\t# and in project config gitweb.avatar = gravatar;\n> +\t# and in project config gitweb.avatar = provider;\n\nSame as above.\n\n> \t'avatar' => {\n> \t\t'sub' => \\&feature_avatar,\n> \t\t'override' => 0,\n> @@ -856,6 +859,8 @@ our @snapshot_fmts = gitweb_get_feature('snapshot');\n> our ($git_avatar) = gitweb_get_feature('avatar');\n> if ($git_avatar eq 'gravatar') {\n> \t$git_avatar = '' unless (eval { require Digest::MD5; 1; });\n> +} elsif ($git_avatar eq 'picon') {\n> +\t# no dependencies\n> } else {\n> \t$git_avatar = '';\n> }\n\nNice.\n\n> @@ -1523,6 +1528,17 @@ sub format_subject_html {\n> # given page, there's no risk for cache conflicts.\n> our %avatar_cache = ();\n> \n> +# Compute the picon url for a given email, by using the picon search service over at\n> +# http://www.cs.indiana.edu/picons/search.html\n> +sub picon_url {\n> +\tmy $email = lc shift;\n> +\tif (!$avatar_cache{$email}) {\n> +\t\tmy ($user, $domain) = split('@', $email);\n> +\t\t$avatar_cache{$email} = \"http://www.cs.indiana.edu/cgi-pub/kinzler/piconsearch.cgi/$domain/$user/users+domains+unknown/up/single\";\n\nHmmm... perhaps it would be better to break this very long URL (link)\nin lines...\n\n> +\t}\n> +\treturn $avatar_cache{$email};\n> +}\n\nI wonder if it is worth caching picons, at least in this form. It isn't\nas if splitting on '@' is expensive. But perhaps to not break pattern\n(that avatar URLs are cached) it is a good thing.\n\nThoughts for the possible future enhancements: find final URL of an image\nvia http://www.cs.indiana.edu/cgi-pub/kinzler/piconsearch.cgi/$domain/$user/users+domains+unknown/up/off/1/order\nby scrapping (parsing) it for .gif link, and store this URL in cache.\nBut that most probably isn't worth it. Just feel like mentioning it.\n\n> +\n> # Compute the gravatar url for a given email, if it's not in the cache already.\n> # Gravatar stores only the part of the URL before the size, since that's the\n> # one computationally more expensive. This also allows reuse of the cache for\n> @@ -1546,6 +1562,8 @@ sub git_get_avatar {\n> \tmy $url = \"\";\n> \tif ($git_avatar eq 'gravatar') {\n> \t\t$url = gravatar_url($email, $size);\n> +\t} elsif ($git_avatar eq 'picon') {\n> +\t\t$url = picon_url($email);\n> \t}\n> \t# Currently only gravatars are supported, but other forms such as\n> \t# picons can be added by putting an else up here and defining $url\n\nVery nice pattern, nice use of infrastructure.\n\nI think that adding 'width' attribute to img tag of avatar should\nperhaps be put here rather than in previous patch, which is about\nadding gravatar URL cache. This chunk:\n\n # Currently only gravatars are supported, but other forms such as\n # picons can be added by putting an else up here and defining $url\n # as needed. If no variant puts something in $url, we assume avatars\n # are completely disabled/unavailable.\n if ($url) {\n - return $pre_white . \"\" . $post_white;\n + return $pre_white .\n + \"\" . $post_white;\n } else {\n return \"\";\n }\n\nshould be, I think, in this patch and not in previous one (or at least\nin some other patch than the previous one).\n\nBTW. you should have updated the comment here.\n\n\nShould it be stated that is here because not \nall kinds of avatars (not all avatar providers) support selecting size\nof avatar, somewhere in this comment?\n\n\nVery nice, well thought infrastructure, which makes adding new avatar\nproviders easy. Good work!\n\n-- \nJakub Narebski\nPoland\n"},{"id":"117128","messageId":"200906281642.36410.jnareb@gmail.com","threadId":"19948","inReplyTo":"1246104305-15191-9-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv7 8/9] gitweb: use picon for gravatar fallback","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-28T14:42:36Z","receivedAt":"2009-06-28T14:42:36Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sat, 27 June 2009, Giuseppe Bilotta wrote:\n\nI think this issue is complicated enough to do it well and configurable\nthat it is out of scope of current patch series, and it would be better\nto be left for next series.\n\n.......................................................................\n\nThere are few issues connected with multiple avatar providers:\n\n1. Avatar provider side fallback, i.e. what to do if given person\n(usually given email address) is not registered with used avatar \nprovider. \n\nGravatar provider by default falls back to [G] gravatar default image,\nand you can specify fallback using 'd'/'default' parameter, either to\none of defined providers: special values \"identicon\", \"monsterid\" and\n\"wavatar\", or to specified image using URI-encoded URL as value of \n'default' parameter. From the point of view of gitweb, you either\npass provider or URL literally (encoding), or you resolve fallback\nprovider avatar and pass encoded URI.\n\nPicon provider has built-in many level fallback; current gitweb \nimplementation searches in 'users' database, then in 'domains'\ndatabase (there e.g. gmail.com domain addresses receive GMail\navatar), and last in 'unknown' database which would always provide\nsome default avatar for \"person\". But in general one can use \ncs.indiana.edu database as fallback, and try personal / local site\ndatabase first.\n\n2. Gitweb side fallback; for some kinds of avatars one can think about,\nfor example 'local' avatars (example URL: \"avatar/user.gif\"), or 'inrepo'\navatar provider (example URL: \"?p=$project;a=blob_plain;f=avatar/user.gif\"),\none can easily detect in gitweb if avatar for given email (given person)\nexists or not (is available or not).\n\n3. Multiple avatars. We can use here something like what Gmane does,\nwhich shows gravatar if it is available (and nothing if it is not), \npicon if it is available (and nothing if it is not), and both picon\nand gravatar if both are available. See example in:\n http://permalink.gmane.org/gmane.comp.version-control.git/122394\n http://permalink.gmane.org/gmane.comp.version-control.git/118058\n\n\nThis patch tries to address issue 1.) for gravatar provider (which allow\nfor specifying default avatar via its URL API). I think however that\nthis issue should be configurable; unconditional using picon provider\nwith very long URI (making gravatar-with-picon-fallback provider URI\nvery, very long) is not a good idea.\n\nBy configurable I mean that user should be able to specify e.g. gravatar\nwith wavatar fallback, gravatar with static image fallback, and gravatar\nwith arbitrary provider (resolved) fallback.\n\nUnless you wanted to provide this patch as an example of having multiple\nproviders / providing avatar fallback (default value). But then it would\nbe better to have it marked as RFC explicitly...\n\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index ae73d45..e2638cb 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -1546,9 +1546,13 @@ sub picon_url {\n> sub gravatar_url {\n> \tmy $email = lc shift;\n> \tmy $size = shift;\n> -\t$avatar_cache{$email} ||=\n> -\t\t\"http://www.gravatar.com/avatar.php?gravatar_id=\" .\n> -\t\t\tDigest::MD5::md5_hex($email) . \"&size=\";\n> +\tif (!$avatar_cache{$email}) {\n> +\t\tmy $picon = CGI::escape(picon_url($email));\n\nSidenote: if we end requiring 'escape', we can simply import it.\n\n> +\t\t$avatar_cache{$email} = \"http://www.gravatar.com/avatar.php?gravatar_id=\" .\n> +\t\t\tDigest::MD5::md5_hex($email) .\n> +\t\t\t\"&default=$picon\" .\n> +\t\t\t\"&size=\";\n> +\t}\n> \treturn $avatar_cache{$email} . $size;\n> }\n\nNevertheless this is nice example that infrastructure created in previous\npatches leads to adding such feature quite simple.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"117130","messageId":"200906281743.55871.jnareb@gmail.com","threadId":"19948","inReplyTo":"1246104305-15191-10-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv7 9/9] gitweb: add alt text to avatar img","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-28T15:43:52Z","receivedAt":"2009-06-28T15:43:52Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sat, 27 June 2009, Giuseppe Bilotta wrote:\n\n> The alt text, set to the avatar provider (i.e. image type), is more\n> friendly than the img url in some text-only browsers.\n\nSignoff?\n\nAcked-by: Jakub Narebski \n\n> ---\n> gitweb/gitweb.perl | 1 +\n> 1 files changed, 1 insertions(+), 0 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index e2638cb..dc6049a 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -1578,6 +1578,7 @@ sub git_get_avatar {\n> \t\t \" \t\t \"class=\\\"avatar\\\" \" .\n> \t\t \"src=\\\"$url\\\" \" .\n> +\t\t\t \"alt=\\\"$git_avatar\\\" \" .\n> \t\t \"/>\" . $post_white;\n> \t} else {\n> \t\treturn \"\";\n> -- \n\nBit of bikeshedding: \"alt=\\\"\\\" \" (empty alt text) would also be\na good solution. This way text browser which do not display images\nwouldn't take limited horizontal space for alt text of extraneous\navatar image.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"117132","messageId":"cb7bb73a0906280903y3175712bx6923b179a9789a1d@mail.gmail.com","threadId":"19948","inReplyTo":"200906281335.40312.jnareb@gmail.com","subject":"Re: [PATCHv7 7/9] gitweb: picon avatar provider","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-28T16:03:58Z","receivedAt":"2009-06-28T16:03:58Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2009/6/28 Jakub Narebski :\n> Thoughts for the possible future enhancements: find final URL of an image\n> via http://www.cs.indiana.edu/cgi-pub/kinzler/piconsearch.cgi/$domain/$user/users+domains+unknown/up/off/1/order\n> by scrapping (parsing) it for .gif link, and store this URL in cache.\n> But that most probably isn't worth it.  Just feel like mentioning it.\n\nThat was my first thought, but since piconsearch offers a link\ndirectly, I decided it was much better to use that rather than html\nscraping.\n\n> Should it be stated that is here because not\n> all kinds of avatars (not all avatar providers) support selecting size\n> of avatar, somewhere in this comment?\n\nIt is generally good form to have the size specified in advance in\nHTML anyway, which is why on later iterations I put the width spec in\nthe first avatar service commit.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"117134","messageId":"cb7bb73a0906280910n22c5907av910b1de28a6c2f2b@mail.gmail.com","threadId":"19948","inReplyTo":"200906281743.55871.jnareb@gmail.com","subject":"Re: [PATCHv7 9/9] gitweb: add alt text to avatar img","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-28T16:10:25Z","receivedAt":"2009-06-28T16:10:25Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2009/6/28 Jakub Narebski :\n> Signoff?\n\nDuh.\n\n> Bit of bikeshedding: \"alt=\\\"\\\" \" (empty alt text) would also be\n> a good solution.  This way text browser which do not display images\n> wouldn't take limited horizontal space for alt text of extraneous\n> avatar image.\n\nOh, interesting. Let me try that.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"117184","messageId":"200906292337.18636.jnareb@gmail.com","threadId":"19948","inReplyTo":"1246104305-15191-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv7 0/9] gitweb: avatar support","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-29T21:37:17Z","receivedAt":"2009-06-29T21:37:17Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sat, 27 Jun 2009, Giuseppe Bilotta wrote:\n> One more attempt at adding avatar support to gitweb.\n> \n> The most significant change since the previous iteration is the addition\n> of a picon provider, and its use as default from gravatar.\n> \n> Most other patches were changed to follow Jakub's suggestions. In\n> particular, the two-line author/committer information was refactored\n> to reduce code duplication, with the benefit that it could also be used\n> in 'tag' view.\n> \n> Giuseppe Bilotta (9):\n> gitweb: refactor author name insertion\n> gitweb: uniform author info for commit and commitdiff\n> gitweb: use git_print_authorship_rows in 'tag' view too\n> gitweb: right-align date cell in shortlog\n> gitweb: (gr)avatar support\n> gitweb: gravatar url cache\n> gitweb: picon avatar provider\n> gitweb: use picon for gravatar fallback\n> gitweb: add alt text to avatar img\n\nI think this patch series shapes very nicely. A bit of refactoring \nupfront, so that the following patches are not very large, and don't\nneed to repeat the same code in many places. Separate issues such \nas right-align date cell, or making 'commitdiff' view use authorship\ninfo layout from 'commit' view are put in separate patches in such\nway that they can be accepted or rejected individually.\n\nVery good work!\n\n-- \nJakub Narebski\nPoland\n"},{"id":"117187","messageId":"cb7bb73a0906291455i6a5e8d91ha535a3a918875ac5@mail.gmail.com","threadId":"19948","inReplyTo":"200906292337.18636.jnareb@gmail.com","subject":"Re: [PATCHv7 0/9] gitweb: avatar support","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-29T21:55:38Z","receivedAt":"2009-06-29T21:55:38Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2009/6/29 Jakub Narebski :\n>> Giuseppe Bilotta (9):\n>>   gitweb: refactor author name insertion\n>>   gitweb: uniform author info for commit and commitdiff\n>>   gitweb: use git_print_authorship_rows in 'tag' view too\n>>   gitweb: right-align date cell in shortlog\n>>   gitweb: (gr)avatar support\n>>   gitweb: gravatar url cache\n>>   gitweb: picon avatar provider\n>>   gitweb: use picon for gravatar fallback\n>>   gitweb: add alt text to avatar img\n>\n> I think this patch series shapes very nicely.  A bit of refactoring\n> upfront, so that the following patches are not very large, and don't\n> need to repeat the same code in many places.  Separate issues such\n> as right-align date cell, or making 'commitdiff' view use authorship\n> info layout from 'commit' view are put in separate patches in such\n> way that they can be accepted or rejected individually.\n\nThanks. I'll send a cleaned up version of everything we agreed on.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"}]}