{"thread":{"id":"19926","subject":"[PATCHv6 0/8] gitweb: gravatar support","startedAt":"2009-06-25T10:42:59Z","lastAt":"2009-06-27T12:49:52Z","messageCount":46,"participants":["Giuseppe Bilotta","Jakub Narebski","Junio C Hamano","Thomas Adam"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"116917","messageId":"1245926587-25074-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"19926","inReplyTo":null,"subject":"[PATCHv6 0/8] gitweb: gravatar support","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-25T10:42:59Z","receivedAt":"2009-06-25T10:42:59Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"A new iteration for my gitweb gravatar patch series. This one includes a\nbunch of new patches, some of which make sense regardless of the\ngravatar support (particularly patches #3 and #7).\n\nSignificant changes from the previous iteration are:\n\n* the feature has been renamed to 'avatar', and 'gravatar' is a possible\n value for it (currently the only sensible value, other than '');\n* an 'alt' attribute is added to img tags, to be more friendly towards\n text-only browser; no special text is put there, only the avatar\n provider name (i.e. 'gravatar', currently);\n* the last patch adds avatars to signoff lines.\n\nI also trimmed the Cc: list for the series, because it was getting very\nlong and I started feeling like I was spamming around by including\neverybody ever commented on the series. Please don't feel left out ;-)\n\nGiuseppe Bilotta (8):\n gitweb: refactor author name insertion\n gitweb: uniform author info for commit and commitdiff\n gitweb: right-align date cell in shortlog\n gitweb: (gr)avatar support\n gitweb: gravatar url cache\n gitweb: add 'alt' to avatar images\n gitweb: recognize 'trivial' acks\n gitweb: add avatar in signoff lines\n\n gitweb/gitweb.css | 13 ++++-\n gitweb/gitweb.perl | 170 +++++++++++++++++++++++++++++++++++++++++----------\n 2 files changed, 148 insertions(+), 35 deletions(-)\n"},{"id":"116918","messageId":"1245926587-25074-2-git-send-email-giuseppe.bilotta@gmail.com","threadId":"19926","inReplyTo":"1245926587-25074-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv6 1/8] gitweb: refactor author name insertion","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-25T10:43:00Z","receivedAt":"2009-06-25T10:43:00Z","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\neasiser to extend them on the CGI side, and rely on CSS rather than\nhard-coded HTML formatting for easier customization.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.css | 5 ++-\n gitweb/gitweb.perl | 80 +++++++++++++++++++++++++++++++---------------------\n 2 files changed, 52 insertions(+), 33 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..9b60418 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+\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,13 +3224,36 @@ 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 %params = @_;\n+\tmy $tag = $params{'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 ($params{'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 containign the full author and commiter information.\n+sub git_print_full_authorship {\n+\tmy $co = shift;\n+\tmy %ad = parse_date($co->{'author_epoch'}, $co->{'author_tz'});\n+\tmy %cd = parse_date($co->{'committer_epoch'}, $co->{'committer_tz'});\n+\tprint \"author\" . esc_html($co->{'author'}) . \"\\n\".\n+\t \"\" .\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@@ -3228,7 +3261,12 @@ sub git_print_authorship {\n \t\tprintf(\" (%02d:%02d %s)\",\n \t\t $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});\n \t}\n-\tprint \"]
\\n\";\n+\tprint \"\" .\n+\t \"\\n\";\n+\tprint \"committer\" . esc_html($co->{'committer'}) . \"\\n\";\n+\tprint \" $cd{'rfc2822'}\" .\n+\t sprintf(\" (%02d:%02d %s)\", $cd{'hour_local'}, $cd{'minute_local'}, $cd{'tz_local'}) .\n+\t \"\\n\";\n }\n \n sub git_print_page_path {\n@@ -4142,11 +4180,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 +4229,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 +4384,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 +5127,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);\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 +5148,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 +5214,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_full_authorship(\\%co);\n \tprint \"\\n\";\n \tprint \"\" .\n \t \"\" .\n@@ -5579,7 +5595,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":"116921","messageId":"1245926587-25074-3-git-send-email-giuseppe.bilotta@gmail.com","threadId":"19926","inReplyTo":"1245926587-25074-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv6 2/8] gitweb: uniform author info for commit and commitdiff","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-25T10:43:01Z","receivedAt":"2009-06-25T10:43:01Z","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 | 6 +++++-\n 1 files changed, 5 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 9b60418..cdfd1d5 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5595,7 +5595,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_full_authorship(\\%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":"116922","messageId":"1245926587-25074-4-git-send-email-giuseppe.bilotta@gmail.com","threadId":"19926","inReplyTo":"1245926587-25074-3-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv6 3/8] gitweb: right-align date cell in shortlog","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-25T10:43:02Z","receivedAt":"2009-06-25T10:43:02Z","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..7240ed7 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":"116923","messageId":"1245926587-25074-5-git-send-email-giuseppe.bilotta@gmail.com","threadId":"19926","inReplyTo":"1245926587-25074-4-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv6 4/8] gitweb: (gr)avatar support","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-25T10:43:03Z","receivedAt":"2009-06-25T10:43:03Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Introduce avatar support: the featuer adds the appropriate img tag next\nto author and committer in commit(diff), history, shortlog and log view.\nMultiple avatar providers are possible, but only gravatar is implemented\nat the moment (and not enabled by default).\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\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.css | 4 +++\n gitweb/gitweb.perl | 67 ++++++++++++++++++++++++++++++++++++++++++++++++---\n 2 files changed, 67 insertions(+), 4 deletions(-)\n\ndiff --git a/gitweb/gitweb.css b/gitweb/gitweb.css\nindex 7240ed7..ad82f86 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 cdfd1d5..f2e0cfe 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,22 @@ 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'override' => 0,\n+\t\t'default' => ['']},\n );\n \n sub gitweb_get_feature {\n@@ -814,6 +838,13 @@ $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 if avatars are enabled and dependencies are satisfied\n+our ($git_avatar) = gitweb_get_feature('avatar');\n+if (($git_avatar eq 'gravatar') &&\n+ !(eval { require Digest::MD5; 1; })) {\n+\t$git_avatar = '';\n+}\n+\n # dispatch\n if (!defined $action) {\n \tif (defined $hash) {\n@@ -1476,7 +1507,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\";\n+\treturn \"<$tag class=\\\"author\\\">\" .\n+\t git_get_avatar($co->{'author_email'}, 'pad_after' => 1) .\n+\t $author . \"\\n\";\n }\n \n # format git diff header line, i.e. \"diff --(git|combined|cc) ...\"\n@@ -3224,6 +3257,29 @@ sub git_print_header_div {\n \t \"\\n
\\n\";\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, %params) = @_;\n+\tmy $pre_white = ($params{'pad_before'} ? \" \" : \"\");\n+\tmy $post_white = ($params{'pad_after'} ? \" \" : \"\");\n+\tmy $size = $avatar_size{$params{'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 # Outputs the author name and date in long form\n sub git_print_authorship {\n \tmy $co = shift;\n@@ -3243,7 +3299,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 containign the full author and commiter information.\n@@ -3251,7 +3308,8 @@ sub git_print_full_authorship {\n \tmy $co = shift;\n \tmy %ad = parse_date($co->{'author_epoch'}, $co->{'author_tz'});\n \tmy %cd = parse_date($co->{'committer_epoch'}, $co->{'committer_tz'});\n-\tprint \"author\" . esc_html($co->{'author'}) . \"\\n\".\n+\tprint \"author\" . esc_html($co->{'author'}) . \"\".\n+\t \"\" .git_get_avatar($co->{'author_email'}, 'size' => 'double') . \"\\n\" .\n \t \"\" .\n \t \" $ad{'rfc2822'}\";\n \tif ($ad{'hour_local'} < 6) {\n@@ -3263,7 +3321,8 @@ sub git_print_full_authorship {\n \t}\n \tprint \"\" .\n \t \"\\n\";\n-\tprint \"committer\" . esc_html($co->{'committer'}) . \"\\n\";\n+\tprint \"committer\" . esc_html($co->{'committer'}) . \"\".\n+\t \"\" .git_get_avatar($co->{'committer_email'}, 'size' => 'double') . \"\\n\";\n \tprint \" $cd{'rfc2822'}\" .\n \t sprintf(\" (%02d:%02d %s)\", $cd{'hour_local'}, $cd{'minute_local'}, $cd{'tz_local'}) .\n \t \"\\n\";\n-- \n1.6.3.rc1.192.gdbfcb\n"},{"id":"116919","messageId":"1245926587-25074-6-git-send-email-giuseppe.bilotta@gmail.com","threadId":"19926","inReplyTo":"1245926587-25074-5-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv6 5/8] gitweb: gravatar url cache","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-25T10:43:04Z","receivedAt":"2009-06-25T10:43:04Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Views which contain many occurrences of the same email address (e.g.\nshortlog view) benefit from not having to recalculate the MD5 of the\nemail address every time.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 24 ++++++++++++++++++++++--\n 1 files changed, 22 insertions(+), 2 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex f2e0cfe..d3bc849 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -3257,6 +3257,27 @@ sub git_print_header_div {\n \t \"\\n
\\n\";\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@@ -3266,8 +3287,7 @@ sub git_get_avatar {\n \tmy $size = $avatar_size{$params{'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-- \n1.6.3.rc1.192.gdbfcb\n"},{"id":"116925","messageId":"1245926587-25074-7-git-send-email-giuseppe.bilotta@gmail.com","threadId":"19926","inReplyTo":"1245926587-25074-6-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv6 6/8] gitweb: add 'alt' to avatar images","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-25T10:43:05Z","receivedAt":"2009-06-25T10:43:05Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Without it, text-only browsers display the URL of the avatar, which can\nbe long and not very informative. We use the avatar provider name for\nthe alt text.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex d3bc849..3e6786b 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -3294,7 +3294,7 @@ sub git_get_avatar {\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 . \"\\\"$git_avatar\\\"\" . $post_white;\n \t} else {\n \t\treturn \"\";\n \t}\n-- \n1.6.3.rc1.192.gdbfcb\n"},{"id":"116920","messageId":"1245926587-25074-8-git-send-email-giuseppe.bilotta@gmail.com","threadId":"19926","inReplyTo":"1245926587-25074-7-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv6 7/8] gitweb: recognize 'trivial' acks","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-25T10:43:06Z","receivedAt":"2009-06-25T10:43:06Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Sometimes patches are trivially acked rather than just acked, so\nextend the signoff regexp to include this case.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 3e6786b..7ca115f 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -3403,7 +3403,7 @@ sub git_print_log {\n \tmy $signoff = 0;\n \tmy $empty = 0;\n \tforeach my $line (@$log) {\n-\t\tif ($line =~ m/^ *(signed[ \\-]off[ \\-]by[ :]|acked[ \\-]by[ :]|cc[ :])/i) {\n+\t\tif ($line =~ m/^ *(signed[ \\-]off[ \\-]by[ :]|(?:trivially[ \\-])?acked[ \\-]by[ :]|cc[ :])/i) {\n \t\t\t$signoff = 1;\n \t\t\t$empty = 0;\n \t\t\tif (! $opts{'-remove_signoff'}) {\n-- \n1.6.3.rc1.192.gdbfcb\n"},{"id":"116924","messageId":"1245926587-25074-9-git-send-email-giuseppe.bilotta@gmail.com","threadId":"19926","inReplyTo":"1245926587-25074-8-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv6 8/8] gitweb: add avatar in signoff lines","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-25T10:43:07Z","receivedAt":"2009-06-25T10:43:07Z","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\nI can't say I'm really satisfied with the layout given by this patch.\nA significant improvement could be obtained by turning the signoff\nline block into a table with three/four columns (signoff, name,\nemail/avatar). But we cannot guarantee that signoff lines come all\ntogether in a block, so this would be more complex to implement.\n\n gitweb/gitweb.perl | 5 ++++-\n 1 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 7ca115f..d385f55 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -3407,7 +3407,10 @@ sub git_print_log {\n \t\t\t$signoff = 1;\n \t\t\t$empty = 0;\n \t\t\tif (! $opts{'-remove_signoff'}) {\n-\t\t\t\tprint \"\" . esc_html($line) . \"
\\n\";\n+\t\t\t\tmy ($email) = $line =~ /<(\\S+@\\S+)>/;\n+\t\t\t\tprint \"\" . esc_html($line) . \"\";\n+\t\t\t\tprint git_get_avatar($email, 'pad_before' => 1) if $email;\n+\t\t\t\tprint \"
\\n\";\n \t\t\t\tnext;\n \t\t\t} else {\n \t\t\t\t# remove signoff lines\n-- \n1.6.3.rc1.192.gdbfcb\n"},{"id":"116927","messageId":"200906251455.32953.jnareb@gmail.com","threadId":"19926","inReplyTo":"1245926587-25074-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv6 0/8] gitweb: gravatar support","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-25T12:55:32Z","receivedAt":"2009-06-25T12:55:32Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Giuseppe Bilotta wrote:\n\n> Significant changes from the previous iteration are:\n> \n> * the feature has been renamed to 'avatar', and 'gravatar' is a possible\n> value for it (currently the only sensible value, other than '');\n\nBy the way, I think it might be better solution to provide picon URL\nas 'default' attribute for gravatar URL, so it is used if there is no\ngravatar for given email.\n\n> * the last patch adds avatars to signoff lines.\n\nPerhaps it would be better to add gravatars at beginning of line?\n\n\nI'll try to post my comments today (i.e. within 24 hours)... but it\nlooks good.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"116931","messageId":"cb7bb73a0906250615i2ed880eci2d3716aa1ca43e4d@mail.gmail.com","threadId":"19926","inReplyTo":"200906251455.32953.jnareb@gmail.com","subject":"Re: [PATCHv6 0/8] gitweb: gravatar support","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-25T13:15:00Z","receivedAt":"2009-06-25T13:15:00Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2009/6/25 Jakub Narebski :\n> Giuseppe Bilotta wrote:\n>\n>> Significant changes from the previous iteration are:\n>>\n>> * the feature has been renamed to 'avatar', and 'gravatar' is a possible\n>>   value for it (currently the only sensible value, other than '');\n>\n> By the way, I think it might be better solution to provide picon URL\n> as 'default' attribute for gravatar URL, so it is used if there is no\n> gravatar for given email.\n\nI was thinking about some form of fallback like that too, but I\nhaven't the slightest idea how picons work, so I'm afraid I'll leave\nthat enhancement to some later time.\n\n>> * the last patch adds avatars to signoff lines.\n>\n> Perhaps it would be better to add gravatars at beginning of line?\n\nI'm not sure. As I mention in the email for that commit, I'm totally\nnot satisfied with the layout. I'm lookint into turning signoff blocks\ninto tables.\n\n> I'll try to post my comments today (i.e. within 24 hours)... but it\n> looks good.\n\nThanks a lot.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"116932","messageId":"1245936097-29538-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"19926","inReplyTo":"1245926587-25074-9-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv6 9/8] gitweb: put signoff lines in a table","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-25T13:21:37Z","receivedAt":"2009-06-25T13:21:37Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"This allows us to give better alignments to the components.\n\nSigned-off-by: Giuseppe Bilotta \n---\n\nBetter, but still far from perfect.\n\n gitweb/gitweb.css | 6 +++++-\n gitweb/gitweb.perl | 47 +++++++++++++++++++++++++++++++++++++++++------\n 2 files changed, 46 insertions(+), 7 deletions(-)\n\ndiff --git a/gitweb/gitweb.css b/gitweb/gitweb.css\nindex ad82f86..21c24fa 100644\n--- a/gitweb/gitweb.css\n+++ b/gitweb/gitweb.css\n@@ -115,10 +115,14 @@ span.age {\n \tfont-style: italic;\n }\n \n-span.signoff {\n+.signoff {\n \tcolor: #888888;\n }\n \n+table.signoff td:first-child {\n+\ttext-align: right;\n+}\n+\n div.log_link {\n \tpadding: 0px 8px;\n \tfont-size: 70%;\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex d385f55..53b8817 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -3402,15 +3402,31 @@ sub git_print_log {\n \t# print log\n \tmy $signoff = 0;\n \tmy $empty = 0;\n+\tmy $signoff_table = 0;\n \tforeach my $line (@$log) {\n-\t\tif ($line =~ m/^ *(signed[ \\-]off[ \\-]by[ :]|(?:trivially[ \\-])?acked[ \\-]by[ :]|cc[ :])/i) {\n-\t\t\t$signoff = 1;\n+\t\tif ($line =~ s/^ *(signed[ \\-]off[ \\-]by|(?:trivially[ \\-])?acked[ \\-]by|cc|looks[ \\-]right[ \\-]to[ \\-]me[ \\-]by)[ :]//i) {\n+\t\t\t$signoff = $1;\n \t\t\t$empty = 0;\n \t\t\tif (! $opts{'-remove_signoff'}) {\n-\t\t\t\tmy ($email) = $line =~ /<(\\S+@\\S+)>/;\n-\t\t\t\tprint \"\" . esc_html($line) . \"\";\n-\t\t\t\tprint git_get_avatar($email, 'pad_before' => 1) if $email;\n-\t\t\t\tprint \"
\\n\";\n+\t\t\t\tif (!$signoff_table) {\n+\t\t\t\t\tprint \"\\n\";\n+\t\t\t\t\t$signoff_table = 1;\n+\t\t\t\t}\n+\t\t\t\tmy $email;\n+\t\t\t\tif ($line =~ s/\\s*<(\\S+@\\S+)>//) {\n+\t\t\t\t\t$email = $1;\n+\t\t\t\t}\n+\t\t\t\tprint \"\";\n+\t\t\t\tprint \"\";\n+\t\t\t\tprint \"\";\n+\t\t\t\tif ($email && $git_avatar) {\n+\t\t\t\t\tprint \"\";\n+\t\t\t\t} else {\n+\t\t\t\t\tprint \"\";\n+\t\t\t\t}\n+\t\t\t\tprint \"\\n\";\n \t\t\t\tnext;\n \t\t\t} else {\n \t\t\t\t# remove signoff lines\n@@ -3429,7 +3445,26 @@ sub git_print_log {\n \t\t\t$empty = 0;\n \t\t}\n \n+\t\t# if we're in a signoff block, empty lines\n+\t\t# are empty rows, other lines terminate\n+\t\t# the block\n+\t\tif ($signoff_table) {\n+\t\t\tif ($empty) {\n+\t\t\t\tprint \"\\n\";\n+\t\t\t\tnext;\n+\t\t\t}\n+\t\t\tprint \"
$signoff\" . esc_html($line) . \"\";\n+\t\t\t\t\tprint git_get_avatar($email);\n+\t\t\t\t\tprint \"\" . esc_html(\"<$email>\") . \"
\\n\";\n+\t\t\t$signoff_table = 0;\n+\t\t}\n+\n \t\tprint format_log_line_html($line) . \"
\\n\";\n+\n+\t}\n+\n+\t# close the signoff table if it's still open\n+\tif ($signoff_table) {\n+\t\tprint \"\\n\";\n+\t\t$signoff_table = 0;\n \t}\n \n \tif ($opts{'-final_empty_line'}) {\n-- \n1.6.3.rc1.192.gdbfcb\n"},{"id":"116946","messageId":"7v8wjggs2c.fsf@alter.siamese.dyndns.org","threadId":"19926","inReplyTo":"cb7bb73a0906250615i2ed880eci2d3716aa1ca43e4d@mail.gmail.com","subject":"Re: [PATCHv6 0/8] gitweb: gravatar support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-06-25T17:07:55Z","receivedAt":"2009-06-25T17:07:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta writes:\n\n> I was thinking about some form of fallback like that too, but I\n> haven't the slightest idea how picons work, so I'm afraid I'll leave\n> that enhancement to some later time.\n\nYeah, let's not go overboard with the initial series.\n\nIn order to lay a sound groundwork so that later patches can build on more\ncleanly, it is good to talk about possibilities of adding support for\nother services _later_ and assess how easy/hard it would be with the\nproposed code structure. Once it's done, and everybody is happy with the\nresult that it will be easy to work with, then we should leave later later\nand move forward.\n"},{"id":"116955","messageId":"cb7bb73a0906251146s785fac1by6847e1d0350f195b@mail.gmail.com","threadId":"19926","inReplyTo":"7v8wjggs2c.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv6 0/8] gitweb: gravatar support","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-25T18:46:01Z","receivedAt":"2009-06-25T18:46:01Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Thu, Jun 25, 2009 at 7:07 PM, Junio C Hamano wrote:\n> Giuseppe Bilotta writes:\n>\n>> I was thinking about some form of fallback like that too, but I\n>> haven't the slightest idea how picons work, so I'm afraid I'll leave\n>> that enhancement to some later time.\n>\n> Yeah, let's not go overboard with the initial series.\n\nWell, I'll confess that I've been on a coding frenzy all day, so\nexpect a new release with preliminary picon support as soon as the\nreview for the last patchset is done 8-D\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"116957","messageId":"7veit8b0rp.fsf@alter.siamese.dyndns.org","threadId":"19926","inReplyTo":"cb7bb73a0906251146s785fac1by6847e1d0350f195b@mail.gmail.com","subject":"Re: [PATCHv6 0/8] gitweb: gravatar support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-06-25T18:56:26Z","receivedAt":"2009-06-25T18:56:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta writes:\n\n> On Thu, Jun 25, 2009 at 7:07 PM, Junio C Hamano wrote:\n>> Giuseppe Bilotta writes:\n>>\n>>> I was thinking about some form of fallback like that too, but I\n>>> haven't the slightest idea how picons work, so I'm afraid I'll leave\n>>> that enhancement to some later time.\n>>\n>> Yeah, let's not go overboard with the initial series.\n>\n> Well, I'll confess that I've been on a coding frenzy all day, so\n> expect a new release with preliminary picon support as soon as the\n> review for the last patchset is done 8-D\n\nOk, I saved v6 in my to-queue box planning to queue it in 'pu', but I'll\ndiscard them and wait until the dust settles in v$n ($n >= 7).\n\nThanks.\n"},{"id":"116978","messageId":"200906260055.11347.jnareb@gmail.com","threadId":"19926","inReplyTo":"1245926587-25074-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv6 1/8] gitweb: refactor author name insertion","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-25T22:55:10Z","receivedAt":"2009-06-25T22:55:10Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 25 June 2009, Giuseppe Bilotta wrote:\n\n> Collect all author display code in appropriate functions, making it\n> easiser to extend them on the CGI side, and rely on CSS rather than\n> hard-coded HTML formatting for easier customization.\n\nTypo: s/easiser/easier/ \n\nThis paragraph could have been written better, perhaps by dividing it\nin two sentences: one talking about collecting/factoring common code,\none talking about moving from presentational HTML (, tags) to\nstyling using CSS and class attribute.\n\nDo I understand it correctly that this was meant as pure refactoring,\ni.e. that none of gitweb output should have changed? Because you made\na mistake, and 'log' view is broken (and it doesn't look like it did\nbefore). See comments below for cause and (simple) solution.\n\n> \n> Signed-off-by: Giuseppe Bilotta \n> ---\n\nWhat are the changes as compared to previous (or just earlier) version?\n\n> gitweb/gitweb.css | 5 ++-\n> gitweb/gitweb.perl | 80 +++++++++++++++++++++++++++++++---------------------\n> 2 files changed, 52 insertions(+), 33 deletions(-)\n> \n> diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css\n> index 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\nNice.\n\n> \n> a.list {\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 1e7e2d8..9b60418 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\nGood... although I wonder if we should not get rid of chop_and_escape_str\naltogether, and for example add title attribute (if needed due to having\nto do shortening) directly to $tag, and not to inner element.\n\nShould \"\\n\" be in returned string? Just asking.\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,13 +3224,36 @@ 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 %params = @_;\n> +\tmy $tag = $params{'tag'} || 'div';\n\nNice using of hash to have named optional params.\n\nUsually though we use %opts and not %params for the name of this\nhash, and we use CGI-like keys prefixed by '-', for example\n'-z' in parse_ls_tree_line(), '-nbsp' in esc_html, '-nohtml' in \nquot_cec(), '-remove_title', '-remove_signoff' and '-final_empty_line'\nin git_print_log(). git_commitdiff() uses %params, but it doesn't\nhave non-optional parameters (still, I guess we should use %opts\nfor consistency), and it uses '-format' and '-single' as names.\n\nhref() subroutine uses %params... but those are not extra named\noptional parameters to subroutine; they are CGI query parameters.\n\n\nNOTE: Default tag (as used in git_log) should be not generic block \nelement tag: 'div', but rather generic inline element: 'span'. The\npresentational HTML element '' which we are replacing is inline\n(layout) element.\n\nIt is because it is '
' (and probably because some \"random\" CSS\nrules in gitweb.css match it) it breaks layout or 'log' view. There\ntwo horizontal lines: one for
element, and one for unnecessary\nnow
element. As div.author_date it has extra 8px padding, from\nwhich the top padding is visible as extra unnecessary vertical space.\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 ($params{'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\nGaah, git has chosen to show this diff a bit strangely...\n\n> +\n> +# Outputs table rows containign the full author and commiter information.\n\nTypo: s/containign/containing/\n\nYou could say that it uses format used to show author and comitter fields\n(headers) in 'commit' view.\n\n> +sub git_print_full_authorship {\n> +\tmy $co = shift;\n> +\tmy %ad = parse_date($co->{'author_epoch'}, $co->{'author_tz'});\n> +\tmy %cd = parse_date($co->{'committer_epoch'}, $co->{'committer_tz'});\n> +\tprint \"author\" . esc_html($co->{'author'}) . \"\\n\".\n> +\t \"\" .\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> @@ -3228,7 +3261,12 @@ sub git_print_authorship {\n> \t\tprintf(\" (%02d:%02d %s)\",\n> \t\t $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});\n> \t}\n> -\tprint \"]
\\n\";\n> +\tprint \"\" .\n> +\t \"\\n\";\n> +\tprint \"committer\" . esc_html($co->{'committer'}) . \"\\n\";\n> +\tprint \" $cd{'rfc2822'}\" .\n> +\t sprintf(\" (%02d:%02d %s)\", $cd{'hour_local'}, $cd{'minute_local'}, $cd{'tz_local'}) .\n> +\t \"\\n\";\n> }\n\nBy the way, what about author / tagger info used in 'tag' view?\nWouldn't it be better to factor out generating table rows for single\nauthor / committer / tagger header (field) info?\n\n> \n> sub git_print_page_path {\n> @@ -4142,11 +4180,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 refactoring and simplification.\n\n> @@ -4193,11 +4229,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 refactoring and simplification.\nGood comment.\n\n> @@ -4350,9 +4384,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 refactoring and simplification.\n\n> @@ -5094,9 +5127,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);\n> +\t\t print \"
\\n\\n\";\n> \n> \t\tprint \"
\\n\";\n> \t\tgit_print_log($co{'comment'}, -final_empty_line=> 1);\n\nHere you replace inline element with
_block_ element, \ndestroying layout. Replacing default of 'div' by default of\ninline element restores old layout.\n\n> @@ -5115,8 +5148,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 +5214,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_full_authorship(\\%co);\n> \tprint \"\\n\";\n> \tprint \"\" .\n> \t \"\" .\n\nI'd rather use here (as mentioned in comment about git_print_full_authorship\nsubroutine) something like the following:\n\n+\tgit_print_authorship_header(\\%co, 'author');\n+\tgit_print_authorship_header(\\%co, 'committer');\n\nOr something like that. But this might be a matter of taste.\n\n> @@ -5579,7 +5595,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\nNice. Although... 'localtime' / '-localtime'? Perhaps '-show_localtime'?\nBut that is just nitpicking (naming is hard), and I am not sure if \nproposed name is any better.\n\n> -- \n> 1.6.3.rc1.192.gdbfcb\n> \n> \n\nThank you for diligent work on this patch series!\n\n-- \nJakub Narebski\nPoland\n"},{"id":"116979","messageId":"200906260101.28054.jnareb@gmail.com","threadId":"19926","inReplyTo":"200906260055.11347.jnareb@gmail.com","subject":"Re: [PATCHv6 1/8] gitweb: refactor author name insertion","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-25T23:01:27Z","receivedAt":"2009-06-25T23:01:27Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 26 June 2009, Jakub Narebski wrote:\n> On Thu, 25 June 2009, Giuseppe Bilotta wrote:\n\n> > @@ -5094,9 +5127,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);\n> > +\t\t print \"
\\n\\n\";\n> > \n> > \t\tprint \"
\\n\";\n> > \t\tgit_print_log($co{'comment'}, -final_empty_line=> 1);\n> \n> Here you replace inline element with
_block_ element, \n> destroying layout. Replacing default of 'div' by default of\n> inline element restores old layout.\n\nNote: here we want .\n\n> > @@ -5579,7 +5595,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\nNote: here we want
.\n\nWhether default would be 'span' or 'div', at least one of those has to\npass explicit 'tag' / '-tag' parameter. I guess it would be easier to\npass 'tag' => 'span' in git_log().\n\n-- \nJakub Narebski\nPoland\n"},{"id":"116980","messageId":"200906260114.44345.jnareb@gmail.com","threadId":"19926","inReplyTo":"1245926587-25074-3-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv6 2/8] gitweb: uniform author info for commit and commitdiff","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-25T23:14:43Z","receivedAt":"2009-06-25T23:14:43Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 25 June 2009, Giuseppe Bilotta wrote:\n\nHere I would write that 'commitdiff' view moves from\n\n Giuseppe Bilotta [Mon, 22 Jun 2009 22:49:58 +0000 (00:49 +0200)]\n\nto\n\n author Giuseppe Bilotta \n Mon, 22 Jun 2009 22:49:58 +0000 (00:49 +0200)\n committer Jakub Narebski \n Tue, 23 Jun 2009 18:02:21 +0000 (20:02 +0200)\"\n\n(perhaps with A U Thor and C O Mitter as example names).\n\n> Signed-off-by: Giuseppe Bilotta \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 9b60418..cdfd1d5 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -5595,7 +5595,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_full_authorship(\\%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 refactoring you have done in previous patch.\n\n\nVery good that you put this in separate patch, so it can be evaluated\nindependently, and decided independently whether it is worth having more\ndetailed authorship information in 'commitdiff', making it more like\n'commit' view, or be more like 'log' view with similar, but slightly\nextended authorship information.\n\nI personally am a bit ambivalent about this issue...\n\n> -- \n> 1.6.3.rc1.192.gdbfcb\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"116982","messageId":"200906260117.22925.jnareb@gmail.com","threadId":"19926","inReplyTo":"200906251455.32953.jnareb@gmail.com","subject":"Re: [PATCHv6 0/8] gitweb: gravatar support","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-25T23:17:22Z","receivedAt":"2009-06-25T23:17:22Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 25 June 2009 14:55, Jakub Narebski wrote:\n\n> I'll try to post my comments today (i.e. within 24 hours)... but it\n> looks good.\n\nI'm sorry, I will post comments for the rest of patches in series \ntomorrow (but I'll try to be within those 24 hours).\n\n-- \nJakub Narebski\nPoland\n"},{"id":"116983","messageId":"cb7bb73a0906251641g71250105ob608b557cce7c454@mail.gmail.com","threadId":"19926","inReplyTo":"200906260055.11347.jnareb@gmail.com","subject":"Re: [PATCHv6 1/8] gitweb: refactor author name insertion","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-25T23:41:28Z","receivedAt":"2009-06-25T23:41:28Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2009/6/26 Jakub Narebski :\n> Do I understand it correctly that this was meant as pure refactoring,\n> i.e. that none of gitweb output should have changed?  Because you made\n> a mistake, and 'log' view is broken (and it doesn't look like it did\n> before).  See comments below for cause and (simple) solution.\n\nThanks for noticing. And yes, the problem is indeed that I forgot to\nspecify tag => span in git log view (I'm keeping div the default\nbecause that's what was there in the first place in\ngit_print_authorship).\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>> +     my $tag = shift;\n>> +     my $co = shift;\n>> +     my $author = chop_and_escape_str($co->{'author_name'}, @_);\n>> +     return \"<$tag class=\\\"author\\\">\" . $author . \"\\n\";\n>> +}\n>\n> Good... although I wonder if we should not get rid of chop_and_escape_str\n> altogether, and for example add title attribute (if needed due to having\n> to do shortening) directly to $tag, and not to inner element.\n\nThis would require some additional refactoring, and I don't have a\nclear idea on how to best implement it right now. I'm afraid it'll\nhave to wait for another time.\n\n> Should \"\\n\" be in returned string? Just asking.\n\nYou're right, we probably don't want to force the newline there. The\nreal question is, do we want the callers to put a newline there? I'm\nthinking no, because it's mostly used in table cells and a newline is\nbetter done at the row level, but I'm not sure either way. I'll just\nremove it for the time being, it should only have effects at the\nsourcecode level, not on the layout.\n\n> Usually though we use %opts and not %params for the name of this\n> hash, and we use CGI-like keys prefixed by '-', for example\n> '-z' in parse_ls_tree_line(), '-nbsp' in esc_html, '-nohtml' in\n> quot_cec(),  '-remove_title', '-remove_signoff' and '-final_empty_line'\n> in git_print_log().  git_commitdiff() uses %params, but it doesn't\n> have non-optional parameters (still, I guess we should use %opts\n> for consistency), and it uses '-format' and '-single' as names.\n>\n> href() subroutine uses %params... but those are not extra named\n> optional parameters to subroutine; they are CGI query parameters.\n\nI'll adjust the code accordingly. BTW the %params in git_commitdiff is\nmy fault too, IIRC.\n\n>>       my %ad = parse_date($co->{'author_epoch'}, $co->{'author_tz'});\n>> -     print \"
\" .\n>> +     print \"<$tag class=\\\"author_date\\\">\" .\n>>             esc_html($co->{'author_name'}) .\n>>             \" [$ad{'rfc2822'}\";\n>> +     if ($params{'localtime'}) {\n>> +             if ($ad{'hour_local'} < 6) {\n>> +                     printf(\" (%02d:%02d %s)\",\n>> +                            $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});\n>> +             } else {\n>> +                     printf(\" (%02d:%02d %s)\",\n>> +                            $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});\n>> +             }\n>> +     }\n>> +     print \"]\\n\";\n>> +}\n>\n> Gaah, git has chosen to show this diff a bit strangely...\n\nOh, very funny indeed. I hadn't realized it went that way. Wonder if\nthe patience diff would have helped here.\n\n> By the way, what about author / tagger info used in 'tag' view?\n\nI totally forgot about that.\n\n> Wouldn't it be better to factor out generating table rows for single\n> author / committer / tagger header (field) info?\n\nGood idea. I'm not sure the tagger field has all the relevant data. I'll check.\n\n> I'd rather use here (as mentioned in comment about git_print_full_authorship\n> subroutine) something like the following:\n>\n> +       git_print_authorship_header(\\%co, 'author');\n> +       git_print_authorship_header(\\%co, 'committer');\n>\n> Or something like that.  But this might be a matter of taste.\n\nI renamed the sub to git_print_authorship_rows, and I'm making it\naccept a list of people to print info for. I'll make it default to\nboth author and committer though.\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"117007","messageId":"200906261133.47326.jnareb@gmail.com","threadId":"19926","inReplyTo":"1245926587-25074-4-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv6 3/8] gitweb: right-align date cell in shortlog","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-26T09:33:46Z","receivedAt":"2009-06-26T09:33:46Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 25 June 2009, Giuseppe Bilotta wrote:\n\n> diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css\n> index 68b22ff..7240ed7 100644\n> --- a/gitweb/gitweb.css\n> +++ b/gitweb/gitweb.css\n> @@ -180,6 +180,10 @@ table {\n\n> +table.shortlog td:first-child{\n> +\ttext-align: right;\n> +}\n\nFirst, there is no space between ':first-child' pseudo-class selector\nand opening '{'. It should be \"td:first-child {\".\n\nSecond, I'd rather avoid more advanced CSS constructs; not all web\nbrowsers support ':first-child' selector. On the other hand adding\nclass attribute to handle this would make page slightly larger.\n\nLast, and most important: I don't agree with this change. In my\nopinion it does not improve layout (and you didn't provide support\nfor this change). Right-align justification should be sparingly,\nas it is not natural in left-to-right languages.\n\nNAK from me for this change.\n-- \nJakub Narebski\nPoland\n"},{"id":"117034","messageId":"cb7bb73a0906261052i4315075ag3f8e2ce220b8534e@mail.gmail.com","threadId":"19926","inReplyTo":"200906260114.44345.jnareb@gmail.com","subject":"Re: [PATCHv6 2/8] gitweb: uniform author info for commit and commitdiff","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-26T17:52:27Z","receivedAt":"2009-06-26T17:52:27Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2009/6/26 Jakub Narebski :\n> On Thu, 25 June 2009, Giuseppe Bilotta wrote:\n>\n> Here I would write that 'commitdiff' view moves from\n>\n>  Giuseppe Bilotta [Mon, 22 Jun 2009 22:49:58 +0000 (00:49 +0200)]\n>\n> to\n>\n>  author      Giuseppe Bilotta \n>              Mon, 22 Jun 2009 22:49:58 +0000 (00:49 +0200)\n>  committer   Jakub Narebski \n>              Tue, 23 Jun 2009 18:02:21 +0000 (20:02 +0200)\"\n>\n> (perhaps with A U Thor and C O Mitter as example names).\n\nDone.\n\n> Very good that you put this in separate patch, so it can be evaluated\n> independently, and decided independently whether it is worth having more\n> detailed authorship information in 'commitdiff', making it more like\n> 'commit' view, or be more like 'log' view with similar, but slightly\n> extended authorship information.\n>\n> I personally am a bit ambivalent about this issue...\n\nI really don't understand why commit and commitdiff are so different,\nbut this is something we went over already. One thing that should be\nkept in mind is that commitdiff can span multiple commits, but in this\ncase it only display the information about the first one (but this is\nnot something that is changed by my patch). commitdiff view probably\nneeds a complete rethinking.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"117035","messageId":"cb7bb73a0906261106n5e12948dydd02bd8d1b19a5e6@mail.gmail.com","threadId":"19926","inReplyTo":"200906261133.47326.jnareb@gmail.com","subject":"Re: [PATCHv6 3/8] gitweb: right-align date cell in shortlog","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-26T18:06:57Z","receivedAt":"2009-06-26T18:06:57Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2009/6/26 Jakub Narebski :\n> On Thu, 25 June 2009, Giuseppe Bilotta wrote:\n>\n>> diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css\n>> index 68b22ff..7240ed7 100644\n>> --- a/gitweb/gitweb.css\n>> +++ b/gitweb/gitweb.css\n>> @@ -180,6 +180,10 @@ table {\n>\n>> +table.shortlog td:first-child{\n>> +     text-align: right;\n>> +}\n>\n> First, there is no space between ':first-child' pseudo-class selector\n> and opening '{'.  It should be \"td:first-child {\".\n\nRight.\n\n> Second, I'd rather avoid more advanced CSS constructs; not all web\n> browsers support ':first-child' selector.  On the other hand adding\n> class attribute to handle this would make page slightly larger.\n\nIIRC :first-child is supported from IE7 onwards. There are hacks to\nmake it work on IE6, but I think they are definitely not worth it.\n\n> Last, and most important: I don't agree with this change.  In my\n> opinion it does not improve layout (and you didn't provide support\n> for this change).  Right-align justification should be sparingly,\n> as it is not natural in left-to-right languages.\n\nOf course, in my opinion it does improve layout.\n\nThe effect is to right-laign the first column of shortlog view, i.e.\nthe one holding the date. For dates that are presented as yyyy-mm-dd\nit makes not difference, but when the phrasing is 'X days ago' it\nprovides the benefit of aligning the 'days ago' part instead of having\nit ragged. See it live at\n\nhttp://git.oblomov.eu/git/shortlog\n\nand judge for yourselves.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"117039","messageId":"200906262142.28845.jnareb@gmail.com","threadId":"19926","inReplyTo":"1245926587-25074-5-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv6 4/8] gitweb: (gr)avatar support","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-26T19:42:28Z","receivedAt":"2009-06-26T19:42:28Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 25 June 2009, Giuseppe Bilotta wrote:\n\n> Introduce avatar support: the featuer adds the appropriate img tag next\n> to author and committer in commit(diff), history, shortlog and log view.\n\nYou forgot about 'tag' view (but I guess it would be done in next \nversion of this patch series).\n\nThere is also 'feed' action (Atom and RSS formats), but that is certainly\nseparate issue, for a separate patch.\n\n> Multiple avatar providers are possible, but only gravatar is implemented\n> at the moment (and not enabled by default).\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> Signed-off-by: Giuseppe Bilotta \n\nAlmost-Acked-by: Jakub Narebski \n\n> ---\n> gitweb/gitweb.css | 4 +++\n> gitweb/gitweb.perl | 67 ++++++++++++++++++++++++++++++++++++++++++++++++---\n> 2 files changed, 67 insertions(+), 4 deletions(-)\n\nYou would _probably_ want to squash the following, just in case:\n\ndiff --git i/t/t9500-gitweb-standalone-no-errors.sh w/t/t9500-gitweb-standalone-no-errors.sh\nindex d539619..e8b57c5 100755\n--- i/t/t9500-gitweb-standalone-no-errors.sh\n+++ w/t/t9500-gitweb-standalone-no-errors.sh\n@@ -660,6 +660,7 @@ cat >>gitweb_config.perl < \n> diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css\n> index 7240ed7..ad82f86 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\nGood. All concerns were addressed.\n\n> div.page_header {\n> \theight: 25px;\n> \tpadding: 8px;\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index cdfd1d5..f2e0cfe 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\nNice. \n\n> # You define site-wide feature defaults here; override them with\n> # $GITWEB_CONFIG as necessary.\n> our %feature = (\n> @@ -365,6 +373,22 @@ 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\nSidenote: Gravatar API description[1] mentions 'identicon', 'monsterid',\n'wavatar'. There are 'picons' (personal icons)[2]. Also avatars doesn't\nneed to be global: they can be some local static image somewhere in web\nserver which serves gitweb script, or they can be stored somewhere in\nrepository following some convention.\n\nCurrent implementation is flexible enough to leave place for extending\nthis feature, but also doesn't try to plan too far in advance. YAGNI\n(You Ain't Gonna Need It).\n\n[1] http://www.gravatar.com/site/implement/url\n[2] http://www.cs.indiana.edu/picons/ftp/faq.html\n\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'override' => 0,\n> +\t\t'default' => ['']},\n\nNote that to disable feature with non-boolean 'default' we use empty\nlist [] (which means 'undef' when parsing, which is false); see \ndescription of features 'snapshot', 'actions'; 'ctags' what is strange\nuses [0] here... Using [''] is a bit strange; and does not protect\nyou, I think.\n\n> );\n> \n> sub gitweb_get_feature {\n> @@ -814,6 +838,13 @@ $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 if avatars are enabled and dependencies are satisfied\n> +our ($git_avatar) = gitweb_get_feature('avatar');\n\nIMPORTANT!!!\n\nBecause you now allow possibility that there can be other avatars\nthan those provided by Gravatar, you should explain in comment\nwhat this check below does (e.g. something like \"load Digest::MD5,\nrequired for gravatar support, and disable it if it does not exist\"),\nso people adding (in the future) support for other kind of avatars\nwould know that they should put similar test there, if needed.\n\n> +if (($git_avatar eq 'gravatar') &&\n> + !(eval { require Digest::MD5; 1; })) {\n> +\t$git_avatar = '';\n> +}\n\nHere you would have to protect against $git_avatar being undefined...\nbut you should do it anyway, as gitweb_get_feature() can return \nundef / empty list.\n\nSo either\n\n +our ($git_avatar) = gitweb_get_feature('avatar') || '';\n\nor\n\n +if (defined $git_avatar && $git_avatar eq 'gravatar' &&\n + !(eval { require Digest::MD5; 1; })) {\n +\t$git_avatar = '';\n +}\n\n\n> +\n> # dispatch\n> if (!defined $action) {\n> \tif (defined $hash) {\n> @@ -1476,7 +1507,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\";\n> +\treturn \"<$tag class=\\\"author\\\">\" .\n> +\t git_get_avatar($co->{'author_email'}, 'pad_after' => 1) .\n> +\t $author . \"\\n\";\n> }\n\nAh, it uses the fact that format_author_html is used in 'history' and\n'shortlog' views, to format only author name for 'Author' column. \n\nBTW. wouldn't it be better if git_get_avatar was defined _before_ this\nuse?\n\n\nThis might be good enough starting point, but I wonder if it wouldn't\nbe a better solution to provide additional column with avatar image\nwhen avatar support is enabled. You would get a better layout in\na very rare case[3] when 'Author' column is too narrow and author is\ninfo is wrapped:\n\n [#] Jonathan\n H. Random\n\nversus in separate columns case:\n\n [#] | Jonathan\n | H. Random\n\nBut this is a very minor problem, which can be left for separate patch.\n\n[3] unless you use netbook or phone to browse...\n\n> \n> # format git diff header line, i.e. \"diff --(git|combined|cc) ...\"\n> @@ -3224,6 +3257,29 @@ sub git_print_header_div {\n> \t \"\\n
\\n\";\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, %params) = @_;\n> +\tmy $pre_white = ($params{'pad_before'} ? \" \" : \"\");\n> +\tmy $post_white = ($params{'pad_after'} ? \" \" : \"\");\n> +\tmy $size = $avatar_size{$params{'size'}} || $avatar_size{'default'};\n\nThe same comment as for first patch in series: gitweb uses \n%opts not %params, and '-size' not 'size' (CGI-like).\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> +\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\nVery nice solution to provide support for future alternate avatar\nsources, like picons or local images (for a gitweb installation).\n\n> +\tif ($url) {\n> +\t\treturn $pre_white . \"\" . $post_white;\n> +\t} else {\n> +\t\treturn \"\";\n> +\t}\n> +}\n\nNice idea.\n\n> +\n> # Outputs the author name and date in long form\n> sub git_print_authorship {\n> \tmy $co = shift;\n> @@ -3243,7 +3299,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\nNice solution for 'log' and perhaps 'commitdiff' views.\n\n> \n> # Outputs table rows containign the full author and commiter information.\n> @@ -3251,7 +3308,8 @@ sub git_print_full_authorship {\n> \tmy $co = shift;\n> \tmy %ad = parse_date($co->{'author_epoch'}, $co->{'author_tz'});\n> \tmy %cd = parse_date($co->{'committer_epoch'}, $co->{'committer_tz'});\n> -\tprint \"author\" . esc_html($co->{'author'}) . \"\\n\".\n> +\tprint \"author\" . esc_html($co->{'author'}) . \"\".\n> +\t \"\" .git_get_avatar($co->{'author_email'}, 'size' => 'double') . \"\\n\" .\n\nNitpick: perhaps it would be better to put git_get_avatar invocation\nin separate line, to limit line length.\n\nMinor nitpick: use \". git_get_avatar(...\", i.e. space between '.' string\nconcatenation operator and 'git_get_avatar'.\n\n> \t \"\" .\n> \t \" $ad{'rfc2822'}\";\n> \tif ($ad{'hour_local'} < 6) {\n> @@ -3263,7 +3321,8 @@ sub git_print_full_authorship {\n> \t}\n> \tprint \"\" .\n> \t \"\\n\";\n> -\tprint \"committer\" . esc_html($co->{'committer'}) . \"\\n\";\n> +\tprint \"committer\" . esc_html($co->{'committer'}) . \"\".\n> +\t \"\" .git_get_avatar($co->{'committer_email'}, 'size' => 'double') . \"\\n\";\n> \tprint \" $cd{'rfc2822'}\" .\n> \t sprintf(\" (%02d:%02d %s)\", $cd{'hour_local'}, $cd{'minute_local'}, $cd{'tz_local'}) .\n> \t \"\\n\";\n\nSame as above.\n\nBTW. if you refactor printing _single_ authorship info, either into\nseparate subroutine or as code in (short) loop, you wouldn't have\nthis code repetition. OTOH there would be also 'committing by night'\nwarning, like current 'coding by night' (localtime)...\n\n\nNice solution for 'commit' and with 2nd patch in series also \n'commitdiff' view.\n\n> -- \n> 1.6.3.rc1.192.gdbfcb\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"117050","messageId":"cb7bb73a0906261508s47e8834fuc9b3313bd9f127ce@mail.gmail.com","threadId":"19926","inReplyTo":"200906262142.28845.jnareb@gmail.com","subject":"Re: [PATCHv6 4/8] gitweb: (gr)avatar support","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-26T22:08:02Z","receivedAt":"2009-06-26T22:08:02Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2009/6/26 Jakub Narebski :\n> On Thu, 25 June 2009, Giuseppe Bilotta wrote:\n>\n>> Introduce avatar support: the featuer adds the appropriate img tag next\n>> to author and committer in commit(diff), history, shortlog and log view.\n>\n> You forgot about 'tag' view (but I guess it would be done in next\n> version of this patch series).\n\nIndeed, it's automatically addressed because patch view follows the refactoring.\n\n> There is also 'feed' action (Atom and RSS formats), but that is certainly\n> separate issue, for a separate patch.\n\nI'm not entirely sure we want avatars there.\n\n> You would _probably_ want to squash the following, just in case:\n\n[snip]\n\n> But even if you don't squash it, please run t9500 with this patch\n> applied, to catch Perl errors and warnings.\n\nI'll squash it in.\n\n> Sidenote: Gravatar API description[1] mentions 'identicon', 'monsterid',\n> 'wavatar'.  There are 'picons' (personal icons)[2].  Also avatars doesn't\n> need to be global: they can be some local static image somewhere in web\n> server which serves gitweb script, or they can be stored somewhere in\n> repository following some convention.\n>\n> Current implementation is flexible enough to leave place for extending\n> this feature, but also doesn't try to plan too far in advance.  YAGNI\n> (You Ain't Gonna Need It).\n>\n> [1] http://www.gravatar.com/site/implement/url\n> [2] http://www.cs.indiana.edu/picons/ftp/faq.html\n\nThe forthcoming series has picons provider and gravatar fallback;\nhowever, we might want to have some way to make the gravatar fallback\nconfigurable.\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>> +             'override' => 0,\n>> +             'default' => ['']},\n>\n> Note that to disable feature with non-boolean 'default' we use empty\n> list [] (which means 'undef' when parsing, which is false); see\n> description of features 'snapshot', 'actions'; 'ctags' what is strange\n> uses [0] here...  Using [''] is a bit strange; and does not protect\n> you, I think.\n\nUsing an empty string (or 0 like ctags do) is nice because it spares\nthe undef check you mention later on, and since empty strings and 0\nevaluate to false in Perl, it's a good way to handle it. Moreover, any\nstring which is not an actual provider would result in no avatars.\nMore about this later.\n\n>> +# check if avatars are enabled and dependencies are satisfied\n>> +our ($git_avatar) = gitweb_get_feature('avatar');\n>\n> IMPORTANT!!!\n>\n> Because you now allow possibility that there can be other avatars\n> than those provided by Gravatar, you should explain in comment\n> what this check below does (e.g. something like \"load Digest::MD5,\n> required for gravatar support, and disable it if it does not exist\"),\n> so people adding (in the future) support for other kind of avatars\n> would know that they should put similar test there, if needed.\n\nI'll make the check and subsequent switch a little cleaner.\n\n>> +if (($git_avatar eq 'gravatar') &&\n>> +   !(eval { require Digest::MD5; 1; })) {\n>> +     $git_avatar = '';\n>> +}\n>\n> Here you would have to protect against $git_avatar being undefined...\n> but you should do it anyway, as gitweb_get_feature() can return\n> undef / empty list.\n\nUsing '' as defalt instead of [] shields me from this problem, and\nworks properly for boolean checks.\n\n> This might be good enough starting point, but I wonder if it wouldn't\n> be a better solution to provide additional column with avatar image\n> when avatar support is enabled.  You would get a better layout in\n> a very rare case[3] when 'Author' column is too narrow and author is\n> info is wrapped:\n>\n>  [#] Jonathan\n>  H. Random\n>\n> versus in separate columns case:\n>\n>  [#] | Jonathan\n>      | H. Random\n>\n> But this is a very minor problem, which can be left for separate patch.\n>\n> [3] unless you use netbook or phone to browse...\n\nI had considered going this way, but it made the code somewhat more\ncomplex so I went for the simpler solution. I'll look into putting it\nin separate cells further on.\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"117051","messageId":"7vmy7ur5dz.fsf@alter.siamese.dyndns.org","threadId":"19926","inReplyTo":"cb7bb73a0906261106n5e12948dydd02bd8d1b19a5e6@mail.gmail.com","subject":"Re: [PATCHv6 3/8] gitweb: right-align date cell in shortlog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-06-26T22:34:32Z","receivedAt":"2009-06-26T22:34:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta writes:\n\n> See it live at\n>\n> http://git.oblomov.eu/git/shortlog\n>\n> and judge for yourselves.\n\nYay, very nice to see these faces! Thanks ;-)\n\nRegardless of other points that may or may not have to be addressed, I\nhad to say this.\n"},{"id":"117052","messageId":"cb7bb73a0906261557q52f2536nb778b2882e979b2b@mail.gmail.com","threadId":"19926","inReplyTo":"7vmy7ur5dz.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv6 3/8] gitweb: right-align date cell in shortlog","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-26T22:57:56Z","receivedAt":"2009-06-26T22:57:56Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Sat, Jun 27, 2009 at 12:34 AM, Junio C Hamano wrote:\n> Giuseppe Bilotta writes:\n>\n>> See it live at\n>>\n>> http://git.oblomov.eu/git/shortlog\n>>\n>> and judge for yourselves.\n>\n> Yay, very nice to see these faces!  Thanks ;-)\n>\n> Regardless of other points that may or may not have to be addressed, I\n> had to say this.\n\nI'm glad you like the faces 8-)\n\nWhat's your opinion on the alignment for the date column in the shortlog?\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"117053","messageId":"200906270058.16686.jnareb@gmail.com","threadId":"19926","inReplyTo":"cb7bb73a0906261508s47e8834fuc9b3313bd9f127ce@mail.gmail.com","subject":"Re: [PATCHv6 4/8] gitweb: (gr)avatar support","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-26T22:58:15Z","receivedAt":"2009-06-26T22:58:15Z","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> 2009/6/26 Jakub Narebski :\n>> On Thu, 25 June 2009, Giuseppe Bilotta wrote:\n>>\n>>> Introduce avatar support: the featuer adds the appropriate img tag next\n>>> to author and committer in commit(diff), history, shortlog and log view.\n\n[...]\n>> There is also 'feed' action (Atom and RSS formats), but that is certainly\n>> separate issue, for a separate patch.\n> \n> I'm not entirely sure we want avatars there.\n\nI think you are right. I have thought that Atom/RSS have place for icons\nfor each entry in feed, but it is not the case. Also feed reader can\nuse gravatars by itself. Sorry for the noise, then.\n\n>> Sidenote: Gravatar API description[1] mentions 'identicon', 'monsterid',\n>> 'wavatar'.  There are 'picons' (personal icons)[2].  Also avatars doesn't\n>> need to be global: they can be some local static image somewhere in web\n>> server which serves gitweb script, or they can be stored somewhere in\n>> repository following some convention.\n>>\n>> Current implementation is flexible enough to leave place for extending\n>> this feature, but also doesn't try to plan too far in advance.  YAGNI\n>> (You Ain't Gonna Need It).\n>>\n>> [1] http://www.gravatar.com/site/implement/url\n>> [2] http://www.cs.indiana.edu/picons/ftp/faq.html\n> \n> The forthcoming series has picons provider and gravatar fallback;\n> however, we might want to have some way to make the gravatar fallback\n> configurable.\n\nDo I understand this correctly that there is additional patch planned\nin new release of this series providing support for gitweb.avatar = picon?\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>>> +             'override' => 0,\n>>> +             'default' => ['']},\n\nNOTE, NOTE, NOTE!\n\nOne thing I forgot about (and would be discovered when running t9500\nwith provided patch... among other errors) is that you need to provide\n'sub' subroutine for feature to be overridable.\n\n...And that subroutine would be responsible for returning '' (empty\nstring) when feature is overridable. See other feature_* subroutines.\n\n>>\n>> Note that to disable feature with non-boolean 'default' we use empty\n>> list [] (which means 'undef' when parsing, which is false); see\n>> description of features 'snapshot', 'actions'; 'ctags' what is strange\n>> uses [0] here...  Using [''] is a bit strange; and does not protect\n>> you, I think.\n> \n> Using an empty string (or 0 like ctags do) is nice because it spares\n> the undef check you mention later on, and since empty strings and 0\n> evaluate to false in Perl, it's a good way to handle it. Moreover, any\n> string which is not an actual provider would result in no avatars.\n> More about this later.\n\n[...]\n>>> +if (($git_avatar eq 'gravatar') &&\n>>> +   !(eval { require Digest::MD5; 1; })) {\n>>> +     $git_avatar = '';\n>>> +}\n>>\n>> Here you would have to protect against $git_avatar being undefined...\n>> but you should do it anyway, as gitweb_get_feature() can return\n>> undef / empty list.\n> \n> Using '' as defalt instead of [] shields me from this problem, and\n> works properly for boolean checks.\n\nWell, I can understand that. I was wrong: it is up to (currently not\ndefined) 'sub' to ensure that gitweb_get_feature would return ''\n('default').\n\n\nStill,\n\n +our ($git_avatar) = gitweb_get_feature('avatar') || '';\n\nis quite simple... and can be in future subtly wrong (unless it would\nbe gitweb_check_feature, which always return scalar / single element).\n\n\n>> This might be good enough starting point, but I wonder if it wouldn't\n>> be a better solution to provide additional column with avatar image\n>> when avatar support is enabled.  You would get a better layout in\n>> a very rare case[3] when 'Author' column is too narrow and author is\n>> info is wrapped:\n>>\n>>  [#] Jonathan\n>>  H. Random\n>>\n>> versus in separate columns case:\n>>\n>>  [#] | Jonathan\n>>      | H. Random\n>>\n>> But this is a very minor problem, which can be left for separate patch.\n>>\n>> [3] unless you use netbook or phone to browse...\n> \n> I had considered going this way, but it made the code somewhat more\n> complex so I went for the simpler solution. I'll look into putting it\n> in separate cells further on.\n\nWell, by \"left for later\" here I thought about later as in after this\npatch series about gravatars get accepted :-)\n\n-- \nJakub Narebski\nPoland\n"},{"id":"117054","messageId":"200906270111.26640.jnareb@gmail.com","threadId":"19926","inReplyTo":"1245926587-25074-6-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv6 5/8] gitweb: gravatar url cache","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-26T23:11:26Z","receivedAt":"2009-06-26T23:11:26Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 25 Jun 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\nIt would be nice to have some benchmarks comparing performance before\nand after this patch.\n\n> \n> Signed-off-by: Giuseppe Bilotta \n\nAcked-by: Jakub Narebski \n\n> ---\n> gitweb/gitweb.perl | 24 ++++++++++++++++++++++--\n> 1 files changed, 22 insertions(+), 2 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index f2e0cfe..d3bc849 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -3257,6 +3257,27 @@ sub git_print_header_div {\n> \t \"\\n
\\n\";\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\nNice explanation.\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> +\treturn $avatar_cache{$email} . $size;\n> +}\n\nNice solution. Very good.\n\nI guess it is not worth it to _not_ use cache for few avatars views \nsuch as 'commit', 'commitdiff', in the future also 'tag' view, isn't it?\n\nBTW. http://www.gravatar.com/site/implement/url recommends\nhttp://www.gravatar.com/avatar/3b3be63a4c2a439b013787725dfce802 rather than\nhttp://www.gravatar.com/avatar.php?gravatar_id=3b3be63a4c2a439b013787725dfce802\nyou use, following http://www.gravatar.com/site/implement/perl\nHmmm...\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> @@ -3266,8 +3287,7 @@ sub git_get_avatar {\n> \tmy $size = $avatar_size{$params{'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\nVery nice.\n\n> -- \n> 1.6.3.rc1.192.gdbfcb\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"117055","messageId":"cb7bb73a0906261614x3a5dab02h1f29d68b6f5005b1@mail.gmail.com","threadId":"19926","inReplyTo":"200906270058.16686.jnareb@gmail.com","subject":"Re: [PATCHv6 4/8] gitweb: (gr)avatar support","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-26T23:14:52Z","receivedAt":"2009-06-26T23:14:52Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Sat, Jun 27, 2009 at 12:58 AM, Jakub Narebski wrote:\n> Do I understand this correctly that there is additional patch planned\n> in new release of this series providing support for gitweb.avatar = picon?\n\nYes.\n\nAnd a separate patch makes the picon the fallback for gravatar too,\nbut I'm thinking about having something like a gitweb.gravatar_default\n(or something like that) to make that configurable.\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>>>> +             'override' => 0,\n>>>> +             'default' => ['']},\n>\n> NOTE, NOTE, NOTE!\n>\n> One thing I forgot about (and would be discovered when running t9500\n> with provided patch... among other errors) is that you need to provide\n> 'sub' subroutine for feature to be overridable.\n\nDuh I had forgotten aout that. But I ran the test and didn't see that\nerror ... maybe I'm reading the log incorrectly.\n\n> ...And that subroutine would be responsible for returning '' (empty\n> string) when feature is overridable.  See other feature_* subroutines.\n\nI've tried an implementation. Hopefully it's correct 8-/\n\n>> I had considered going this way, but it made the code somewhat more\n>> complex so I went for the simpler solution. I'll look into putting it\n>> in separate cells further on.\n>\n> Well, by \"left for later\" here I thought about later as in after this\n> patch series about gravatars get accepted :-)\n\nMe too 8-D\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"117056","messageId":"200906270125.25048.jnareb@gmail.com","threadId":"19926","inReplyTo":"cb7bb73a0906261614x3a5dab02h1f29d68b6f5005b1@mail.gmail.com","subject":"Re: [PATCHv6 4/8] gitweb: (gr)avatar support","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-26T23:25:24Z","receivedAt":"2009-06-26T23:25:24Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Dnia sobota 27. czerwca 2009 01:14, Giuseppe Bilotta napisał:\n> On Sat, Jun 27, 2009 at 12:58 AM, Jakub Narebski wrote:\n\n> > Do I understand this correctly that there is additional patch planned\n> > in new release of this series providing support for gitweb.avatar = picon?\n> \n> Yes.\n> \n> And a separate patch makes the picon the fallback for gravatar too,\n> but I'm thinking about having something like a gitweb.gravatar_default\n> (or something like that) to make that configurable.\n\nI'm not sure if having picons as 'default' for gravatar is a good idea,\nbecause the rules for resolving picons are complicated[1]... which \nI didn't realize when writing this (sketch of) idea.\n\n[1] http://www.cs.indiana.edu/picons/ftp/faq.html\n-- \nJakub Narebski\nPoland\n"},{"id":"117057","messageId":"cb7bb73a0906261627i1a32bef1h3833d1c12a12e759@mail.gmail.com","threadId":"19926","inReplyTo":"200906270111.26640.jnareb@gmail.com","subject":"Re: [PATCHv6 5/8] gitweb: gravatar url cache","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-26T23:27:50Z","receivedAt":"2009-06-26T23: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 Thu, 25 Jun 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> It would be nice to have some benchmarks comparing performance before\n> and after this patch.\n\nIndeed it would.\n\nOh, you mean I should provide them? 8-D\n\n(I'll see if I can get around it this weekend)\n\n> I guess it is not worth it to _not_ use cache for few avatars views\n> such as 'commit', 'commitdiff', in the future also 'tag' view, isn't it?\n\nI would say not.\n\n> BTW. http://www.gravatar.com/site/implement/url recommends\n> http://www.gravatar.com/avatar/3b3be63a4c2a439b013787725dfce802 rather than\n> http://www.gravatar.com/avatar.php?gravatar_id=3b3be63a4c2a439b013787725dfce802\n> you use, following http://www.gravatar.com/site/implement/perl\n\nI think the perl code there is just obsolete (the /avarar/ thing is\nmore recent). I'll update to the new one because it's more compact.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"117058","messageId":"200906270139.55006.jnareb@gmail.com","threadId":"19926","inReplyTo":"1245926587-25074-7-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv6 6/8] gitweb: add 'alt' to avatar images","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-26T23:39:54Z","receivedAt":"2009-06-26T23:39:54Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 25 June 2009, Giuseppe Bilotta wrote:\n\n> Without it, text-only browsers display the URL of the avatar, which can\n> be long and not very informative. \n\nActually different text-only web browsers behave differently (with\ndefault settings).\n\nBefore this patch:\n * lynx show basename of the URL of the avatar, which ain't pretty\n [avatar.php?gravatar_id=955680802bc3d50476786bb3ca9cfc52&size=16]\n * elinks doesn't show by default images at all, which means that only\n   used for padding will be visible (don't use  ?)\n * w3m show basename of avatar URL without extension nor query string:\n [avatar]\n\nAfter this patch:\n * lynx show alt text, i.e. 'gravatar' (no [] to mark as image)\n * elinks show alt text, i.e. 'gravatar' (no [] to mark as image)\n * w3m show alt text, i.e. gravatar, in separate color (also no [])\n\nLynx Version 2.8.5rel.1\nELinks 0.10.3\nw3m version w3m/0.5.1+cvs-1.946\n\n> We use the avatar provider name for the alt text.\n\nI am not sure if 'type of avatar' wouldn't be easier to understand than\n'avatar provider name'. OTOH the latter is more correct. Perhaps\n\"We use the avatar provider (type of avatar) for the alt text\"?\nBut that is just nitpicking...\n\n> \n> Signed-off-by: Giuseppe Bilotta \n> ---\n> gitweb/gitweb.perl | 2 +-\n> 1 files changed, 1 insertions(+), 1 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index d3bc849..3e6786b 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -3294,7 +3294,7 @@ sub git_get_avatar {\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 . \"\\\"$git_avatar\\\"\" . $post_white;\n> \t} else {\n> \t\treturn \"\";\n> \t}\n\nNice, simple improvement.\n\n> -- \n> 1.6.3.rc1.192.gdbfcb\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"117059","messageId":"200906270153.52512.jnareb@gmail.com","threadId":"19926","inReplyTo":"cb7bb73a0906261627i1a32bef1h3833d1c12a12e759@mail.gmail.com","subject":"Re: [PATCHv6 5/8] gitweb: gravatar url cache","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-26T23:53:51Z","receivedAt":"2009-06-26T23:53:51Z","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> 2009/6/27 Jakub Narebski :\n\n> > BTW. http://www.gravatar.com/site/implement/url recommends\n> > http://www.gravatar.com/avatar/3b3be63a4c2a439b013787725dfce802 rather than\n> > http://www.gravatar.com/avatar.php?gravatar_id=3b3be63a4c2a439b013787725dfce802\n> > you use, following http://www.gravatar.com/site/implement/perl\n> \n> I think the perl code there is just obsolete (the /avatar/ thing is\n> more recent). I'll update to the new one because it's more compact.\n\nI think that Gravatar::URL module on CPAN[1] (which we cannot use in\ngitweb because it is extra nonstandard non-core not needed dependency)\nalso uses newer path_info form.\n\n[1] http://p3rl.org/Gravatar::URL\n\n-- \nJakub Narebski\nPoland\n"},{"id":"117060","messageId":"7vvdmipmze.fsf@alter.siamese.dyndns.org","threadId":"19926","inReplyTo":"cb7bb73a0906261557q52f2536nb778b2882e979b2b@mail.gmail.com","subject":"Re: [PATCHv6 3/8] gitweb: right-align date cell in shortlog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-06-26T23:57:25Z","receivedAt":"2009-06-26T23:57:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta writes:\n\n> What's your opinion on the alignment for the date column in the shortlog?\n\nEven though I am very used to seeing the current output style that aligns\nto the left edge, your output didn't bother me. \n\nBecause visual preference typically is influenced very heavily by inertia,\nI take this as a sign that (1) left or right it does not matter very much,\nand/or (2) aligning to the right edge is slightly better.\n\nBut that is just my impression.\n"},{"id":"117061","messageId":"18071eea0906261708k840737vaee1b1428cf70af2@mail.gmail.com","threadId":"19926","inReplyTo":"200906270139.55006.jnareb@gmail.com","subject":"Re: [PATCHv6 6/8] gitweb: add 'alt' to avatar images","fromName":"Thomas Adam","fromEmail":"thomas.adam22@gmail.com","sentAt":"2009-06-27T00:08:07Z","receivedAt":"2009-06-27T00:08:07Z","isPatch":false,"sender":{"key":"thomas.adam22@gmail.com","avatar":"https://gravatar.com/avatar/137f9858bc6bfd5b2f743aefd988c81ce0cbd306248889df80e269519cfc8741?d=mp&s=160"},"body":"2009/6/27 Jakub Narebski :\n> After this patch:\n>  * lynx   show alt text, i.e. 'gravatar' (no [] to mark as image)\n>  * elinks show alt text, i.e. 'gravatar' (no [] to mark as image)\n>  * w3m show alt text, i.e. gravatar, in separate color (also no [])\n>\n> Lynx Version 2.8.5rel.1\n> ELinks 0.10.3\n\nThat's an old version of ELinks. ELinks 0.12 I think changed this\nbehaviour to at least try and display ALT tags by default.\n\n-- Thomas Adam\n"},{"id":"117062","messageId":"200906270219.46266.jnareb@gmail.com","threadId":"19926","inReplyTo":"1245926587-25074-8-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv6 7/8] gitweb: recognize 'trivial' acks","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-27T00:19:45Z","receivedAt":"2009-06-27T00:19:45Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 25 Jun 2009, Giuseppe Bilotta wrote:\n> Sometimes patches are trivially acked rather than just acked, so\n> extend the signoff regexp to include this case.\n> \n> Signed-off-by: Giuseppe Bilotta \n\nThis is straghtforward enough.\n\nAcked-by: Jakub Narebski \n\n> ---\n> gitweb/gitweb.perl | 2 +-\n> 1 files changed, 1 insertions(+), 1 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 3e6786b..7ca115f 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -3403,7 +3403,7 @@ sub git_print_log {\n> \tmy $signoff = 0;\n> \tmy $empty = 0;\n> \tforeach my $line (@$log) {\n> -\t\tif ($line =~ m/^ *(signed[ \\-]off[ \\-]by[ :]|acked[ \\-]by[ :]|cc[ :])/i) {\n> +\t\tif ($line =~ m/^ *(signed[ \\-]off[ \\-]by[ :]|(?:trivially[ \\-])?acked[ \\-]by[ :]|cc[ :])/i) {\n> \t\t\t$signoff = 1;\n> \t\t\t$empty = 0;\n> \t\t\tif (! $opts{'-remove_signoff'}) {\n> -- \n> 1.6.3.rc1.192.gdbfcb\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"117065","messageId":"7vhby2o6xy.fsf@alter.siamese.dyndns.org","threadId":"19926","inReplyTo":"200906270125.25048.jnareb@gmail.com","subject":"Re: [PATCHv6 4/8] gitweb: (gr)avatar support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-06-27T00:29:13Z","receivedAt":"2009-06-27T00:29:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> I'm not sure if having picons as 'default' for gravatar is a good idea,\n> because the rules for resolving picons are complicated[1]... which \n> I didn't realize when writing this (sketch of) idea.\n>\n> [1] http://www.cs.indiana.edu/picons/ftp/faq.html\n\nAlso picons these days look somewhat antiquated with its rather strict\nlimitation to the colormaps (IIRC, you practically have to go grayscale if\nyou want to use a real photo), and it would make sense to try gravatar\nfirst and then fall back to picons.\n"},{"id":"117066","messageId":"cb7bb73a0906261732k57b78052ud8b96a550de52cd5@mail.gmail.com","threadId":"19926","inReplyTo":"7vhby2o6xy.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv6 4/8] gitweb: (gr)avatar support","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-27T00:32:09Z","receivedAt":"2009-06-27T00:32:09Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Sat, Jun 27, 2009 at 2:29 AM, Junio C Hamano wrote:\n> Jakub Narebski writes:\n>\n>> I'm not sure if having picons as 'default' for gravatar is a good idea,\n>> because the rules for resolving picons are complicated[1]... which\n>> I didn't realize when writing this (sketch of) idea.\n>>\n>> [1] http://www.cs.indiana.edu/picons/ftp/faq.html\n>\n> Also picons these days look somewhat antiquated with its rather strict\n> limitation to the colormaps (IIRC, you practically have to go grayscale if\n> you want to use a real photo), and it would make sense to try gravatar\n> first and then fall back to picons.\n\nThat's what gravatar's default is for: if the image is not set, use\nwhatever is provided as default (with this patch, the picon).\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"117067","messageId":"7vr5x6mqt2.fsf@alter.siamese.dyndns.org","threadId":"19926","inReplyTo":"1245926587-25074-8-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv6 7/8] gitweb: recognize 'trivial' acks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-06-27T01:03:05Z","receivedAt":"2009-06-27T01:03:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta writes:\n\n> \tforeach my $line (@$log) {\n> -\t\tif ($line =~ m/^ *(signed[ \\-]off[ \\-]by[ :]|acked[ \\-]by[ :]|cc[ :])/i) {\n> +\t\tif ($line =~ m/^ *(signed[ \\-]off[ \\-]by[ :]|(?:trivially[ \\-])?acked[ \\-]by[ :]|cc[ :])/i) {\n\nWouldn't it make more sense to do something like:\n\n\tif ($line =~ m/^[-a-z]+: (.*)$/i && looks_like_a_name_and_email($1)) {\n \t... do the avatar thing ...\n\t}\n\nand limit this processing only to the last run of no-blank lines?\n"},{"id":"117071","messageId":"cb7bb73a0906270204x6c28b8f2g79e0e9e70451cd00@mail.gmail.com","threadId":"19926","inReplyTo":"7vr5x6mqt2.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv6 7/8] gitweb: recognize 'trivial' acks","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-27T09:04:14Z","receivedAt":"2009-06-27T09:04:14Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Sat, Jun 27, 2009 at 3:03 AM, Junio C Hamano wrote:\n> Giuseppe Bilotta writes:\n>\n>>       foreach my $line (@$log) {\n>> -             if ($line =~ m/^ *(signed[ \\-]off[ \\-]by[ :]|acked[ \\-]by[ :]|cc[ :])/i) {\n>> +             if ($line =~ m/^ *(signed[ \\-]off[ \\-]by[ :]|(?:trivially[ \\-])?acked[ \\-]by[ :]|cc[ :])/i) {\n>\n> Wouldn't it make more sense to do something like:\n>\n>        if ($line =~ m/^[-a-z]+: (.*)$/i && looks_like_a_name_and_email($1)) {\n>                ... do the avatar thing ...\n>        }\n>\n> and limit this processing only to the last run of no-blank lines?\n\nI've been thinking about doing something like this, especially since\nthe next iteration this patch also adds 'Looks-fine-to-me-by' (spearce\nreally _loves_ customizing its signoffs lines during reviews 8-D).\n\nThe main difference between the two sections of the code is that the\noriginal code allows whitespace instead of dashes in the signoff\nlead-in, and the lead-in itself is OPTIONALLY terminated by a colon.\n\nIf we can lose this, then ^\\s*[-a-z]+: followed by what looks like a\nname and email can be a detector. An alternative would be to keep the\ncurrent signoff detector as-is and ADD the generic one, possibly under\nthe condition that we are already parsing signoffs.\n\nThe idea of accepting those lines as signoffs only at the end of the\ncommit log is also a good idea, but has the problem of requiring some\nform of look-ahead. Two possible logics:\n\n(1) when something that looks like a signoff is met, stash it away,\nand keep collecting signoff-lookalikes and empty lines until either a\nnon-signoff-lookalike (in which case the previous lines are NOT\nsignoff) or the log end is met (in which case we print them as signoff\nlines).\n\n(2) same as before, but two or more consecutive signoff-lookalikes are\na signoff block regardless of what follows them.\n\nWhich one should I go with?\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"117072","messageId":"200906271126.38757.jnareb@gmail.com","threadId":"19926","inReplyTo":"1245926587-25074-9-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv6 8/8] gitweb: add avatar in signoff lines","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-27T09:26:37Z","receivedAt":"2009-06-27T09:26:37Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 25 June 2009, Giuseppe Bilotta wrote:\n\n> Signed-off-by: Giuseppe Bilotta \n\nBTW. if it is an RFC, it should be marked as RFC in subject\n(\"[RFC PATCHv6 8/8] gitweb: add avatar in signoff lines\").\n\nAnd I guess this issue should be left for later.\n\n> ---\n> \n> I can't say I'm really satisfied with the layout given by this patch.\n> A significant improvement could be obtained by turning the signoff\n> line block into a table with three/four columns (signoff, name,\n> email/avatar). \n\nFirst, I think adding (gr)avatars to signoff lines might be not worth\nit. Neither GitHub nor Gitorious show gravatars for signoff lines\n(note that they use larger images, and therefore easier to view).\n\nSecond, it is not the only possible layout. Let's use one of existing\ncommits (e1d3793) in git repository as example:\n\n completion: add --thread=deep/shallow to format-patch\n\n [1] Signed-off-by: Stephen Boyd [2] [3] [4]|\n [1] Trivially-Acked-By: Shawn O. Pearce [2] [3] [4]|\n [1] Signed-off-by: Junio C Hamano [2] [3] [4]|\n\nEven without changing layout of signoff lines (so they look exactly\nlike they look in git-show or git-log output, modulo highlighting\nand (gr)avatars), there are more possibilities:\n\n 1. On the left side of signoff lines\n 2. Current version: on the right side of signoff lines, just after\n 3. On the right hand side, aligned; would probably need table\n 4. On the right hand side, flushed (floated) right\n\nThere is also more complicated solution of having (gr)avatars appear\nonly on mouseover, either all avatars on hover over signoff block,\nor single (and perhaps larger size) avatar on hover over signoff line.\nThis can be done using pure CSS, without JavaScript[1]\n\n[1] http://meyerweb.com/eric/css/edge/popups/demo2.html\n\n> But we cannot guarantee that signoff lines come all\n> together in a block, so this would be more complex to implement.\n\nActually I think we can assume that signoff lines come all together\nin single block at the very end of commit message; we should take\ninto account possibility of spurious empty lines between signoff\nlines, though (it did happen, see e.g. 8dfb17e1).\n\n> \n> gitweb/gitweb.perl | 5 ++++-\n> 1 files changed, 4 insertions(+), 1 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 7ca115f..d385f55 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -3407,7 +3407,10 @@ sub git_print_log {\n> \t\t\t$signoff = 1;\n> \t\t\t$empty = 0;\n> \t\t\tif (! $opts{'-remove_signoff'}) {\n> -\t\t\t\tprint \"\" . esc_html($line) . \"
\\n\";\n> +\t\t\t\tmy ($email) = $line =~ /<(\\S+@\\S+)>/;\n> +\t\t\t\tprint \"\" . esc_html($line) . \"\";\n> +\t\t\t\tprint git_get_avatar($email, 'pad_before' => 1) if $email;\n> +\t\t\t\tprint \"
\\n\";\n> \t\t\t\tnext;\n> \t\t\t} else {\n> \t\t\t\t# remove signoff lines\n> -- \n\nAnd here is version with (gr)avatar on the left side of signoff lines\n(take a look if it is not better layout):\n\ndiff --git c/gitweb/gitweb.perl w/gitweb/gitweb.perl\nindex 301bdd8..7701bac 100755\n--- c/gitweb/gitweb.perl\n+++ w/gitweb/gitweb.perl\n@@ -3407,6 +3407,8 @@ sub git_print_log {\n \t\t\t$signoff = 1;\n \t\t\t$empty = 0;\n \t\t\tif (! $opts{'-remove_signoff'}) {\n+\t\t\t\tmy ($email) = $line =~ /<(\\S+@\\S+)>/;\n+\t\t\t\tprint git_get_avatar($email, 'pad_after' => 1) if $email;\n \t\t\t\tprint \"\" . esc_html($line) . \"
\\n\";\n \t\t\t\tnext;\n \t\t\t} else {\n\n\n-- \nJakub Narebski\nPoland\n"},{"id":"117073","messageId":"200906271155.04602.jnareb@gmail.com","threadId":"19926","inReplyTo":"1245936097-29538-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv6 9/8] gitweb: put signoff lines in a table","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-27T09:55:04Z","receivedAt":"2009-06-27T09:55:04Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 25 June 2009, Giuseppe Bilotta wrote:\n\n> This allows us to give better alignments to the components.\n> \n> Signed-off-by: Giuseppe Bilotta \n> ---\n> \n> Better, but still far from perfect.\n\nI don't like it. NAK from me (for this experimental patch).\n\nFirst it breaks correspondence between gitweb's 'commit'/'commitdiff'\nview and git-show, and between gitweb's 'log' view and git-log.\nI'd rather we kept that gitweb output is similar to CLI output, so\nsomebody familiar with one of them would have it easy understanding\nthe other. Consistency in output.\n\nSecond, I have checked how it looks like in few examples:\ne1d37937 (different types of signoff) and 8dfb17e1 (empty line in \nsignoff block) and I have the following complaints:\n * There is extra vertical whitespace between signoff lines\n * The ':' character terminating signoffs is lost\n * Empty line vanished (which might be considered good thing).\n\n> \n> gitweb/gitweb.css | 6 +++++-\n> gitweb/gitweb.perl | 47 +++++++++++++++++++++++++++++++++++++++++------\n> 2 files changed, 46 insertions(+), 7 deletions(-)\n> \n> diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css\n> index ad82f86..21c24fa 100644\n> --- a/gitweb/gitweb.css\n> +++ b/gitweb/gitweb.css\n> @@ -115,10 +115,14 @@ span.age {\n> \tfont-style: italic;\n> }\n> \n> -span.signoff {\n> +.signoff {\n> \tcolor: #888888;\n> }\n\nThis change might be good to have nevertheless, for future extendability.\n\n> \n> +table.signoff td:first-child {\n> +\ttext-align: right;\n> +}\n\nAdvanced CSS selector. Not all web browsers support it (although \nnowadays I suppose most do support ':first-child' pseudo-class).\n\n> +\n> div.log_link {\n> \tpadding: 0px 8px;\n> \tfont-size: 70%;\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index d385f55..53b8817 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -3402,15 +3402,31 @@ sub git_print_log {\n> \t# print log\n> \tmy $signoff = 0;\n> \tmy $empty = 0;\n> +\tmy $signoff_table = 0;\n> \tforeach my $line (@$log) {\n> -\t\tif ($line =~ m/^ *(signed[ \\-]off[ \\-]by[ :]|(?:trivially[ \\-])?acked[ \\-]by[ :]|cc[ :])/i) {\n> -\t\t\t$signoff = 1;\n> +\t\tif ($line =~ s/^ *(signed[ \\-]off[ \\-]by|(?:trivially[ \\-])?acked[ \\-]by|cc|looks[ \\-]right[ \\-]to[ \\-]me[ \\-]by)[ :]//i) {\n> +\t\t\t$signoff = $1;\n\nExtending regexp for signoff matching is _independent_ change, and IMHO\nshould be put in separate commit (perhaps squashed in 7/8). We really\nneed to do something about it, as this regexp starts to be unwieldingly\nlong... but this issue is already discussed in subthread for patch 7/8\nin this series.\n\nYou changed \"$signoff = 1;\" to \"$signoff = $1;\" and later catch $email...\nwhy not do it in the same line, using single (more complicated) regexp?\n\nAlso you don't catch terminating ':' in $signoff (see complain above).\n\n> \t\t\t$empty = 0;\n> \t\t\tif (! $opts{'-remove_signoff'}) {\n> -\t\t\t\tmy ($email) = $line =~ /<(\\S+@\\S+)>/;\n> -\t\t\t\tprint \"\" . esc_html($line) . \"\";\n> -\t\t\t\tprint git_get_avatar($email, 'pad_before' => 1) if $email;\n> -\t\t\t\tprint \"
\\n\";\n> +\t\t\t\tif (!$signoff_table) {\n> +\t\t\t\t\tprint \"\\n\";\n> +\t\t\t\t\t$signoff_table = 1;\n> +\t\t\t\t}\n> +\t\t\t\tmy $email;\n> +\t\t\t\tif ($line =~ s/\\s*<(\\S+@\\S+)>//) {\n> +\t\t\t\t\t$email = $1;\n> +\t\t\t\t}\n> +\t\t\t\tprint \"\";\n> +\t\t\t\tprint \"\";\n> +\t\t\t\tprint \"\";\n> +\t\t\t\tif ($email && $git_avatar) {\n> +\t\t\t\t\tprint \"\";\n> +\t\t\t\t} else {\n> +\t\t\t\t\tprint \"\";\n> +\t\t\t\t}\n> +\t\t\t\tprint \"\\n\";\n> \t\t\t\tnext;\n> \t\t\t} else {\n> \t\t\t\t# remove signoff lines\n> @@ -3429,7 +3445,26 @@ sub git_print_log {\n> \t\t\t$empty = 0;\n> \t\t}\n> \n> +\t\t# if we're in a signoff block, empty lines\n> +\t\t# are empty rows, other lines terminate\n> +\t\t# the block\n> +\t\tif ($signoff_table) {\n> +\t\t\tif ($empty) {\n> +\t\t\t\tprint \"\\n\";\n> +\t\t\t\tnext;\n> +\t\t\t}\n\nI'd rather use \"\\n\" here instead.\n\n> +\t\t\tprint \"
$signoff\" . esc_html($line) . \"\";\n> +\t\t\t\t\tprint git_get_avatar($email);\n> +\t\t\t\t\tprint \"\" . esc_html(\"<$email>\") . \"
\\n\";\n> +\t\t\t$signoff_table = 0;\n> +\t\t}\n> +\n> \t\tprint format_log_line_html($line) . \"
\\n\";\n> +\n> +\t}\n> +\n> +\t# close the signoff table if it's still open\n> +\tif ($signoff_table) {\n> +\t\tprint \"\\n\";\n> +\t\t$signoff_table = 0;\n> \t}\n> \n> \tif ($opts{'-final_empty_line'}) {\n> -- \n\nMuch more complicated code, not much gain IMHO. It is not worth it\n(even if you think that the layout is better; I don't think that).\n\n-- \nJakub Narebski\nPoland\n"},{"id":"117074","messageId":"cb7bb73a0906270321m6154e6a2l240723ffbd7b6e8f@mail.gmail.com","threadId":"19926","inReplyTo":"200906271126.38757.jnareb@gmail.com","subject":"Re: [PATCHv6 8/8] gitweb: add avatar in signoff lines","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-27T10:21:29Z","receivedAt":"2009-06-27T10:21: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> On Thu, 25 June 2009, Giuseppe Bilotta wrote:\n>\n>> Signed-off-by: Giuseppe Bilotta \n>\n> BTW. if it is an RFC, it should be marked as RFC in subject\n> (\"[RFC PATCHv6 8/8] gitweb: add avatar in signoff lines\").\n>\n> And I guess this issue should be left for later.\n\n\nYou're right. My next patchset will not include this part (I'll send\nit separately).\n\n>> I can't say I'm really satisfied with the layout given by this patch.\n>> A significant improvement could be obtained by turning the signoff\n>> line block into a table with three/four columns (signoff, name,\n>> email/avatar).\n>\n> First, I think adding (gr)avatars to signoff lines might be not worth\n> it.  Neither GitHub nor Gitorious show gravatars for signoff lines\n> (note that they use larger images, and therefore easier to view).\n\nWell, just because the others don't do it, it doesn't mean we\nshouldn't either ;-)\n\nBut yes, I have to confess I love toying around like this.\n\n> Second, it is not the only possible layout.  Let's use one of existing\n> commits (e1d3793) in git repository as example:\n>\n>  completion: add --thread=deep/shallow to format-patch\n>\n>  [1] Signed-off-by: Stephen Boyd [2]          [3]            [4]|\n>  [1] Trivially-Acked-By: Shawn O. Pearce [2] [3]            [4]|\n>  [1] Signed-off-by: Junio C Hamano [2]         [3]            [4]|\n>\n> Even without changing layout of signoff lines (so they look exactly\n> like they look in git-show or git-log output, modulo highlighting\n> and (gr)avatars), there are more possibilities:\n>\n>  1. On the left side of signoff lines\n>  2. Current version: on the right side of signoff lines, just after\n>  3. On the right hand side, aligned; would probably need table\n>  4. On the right hand side, flushed (floated) right\n\nI have a table implementation running @ http://git.oblomov.eu/git/commit/e1d3793\n\n> There is also more complicated solution of having (gr)avatars appear\n> only on mouseover, either all avatars on hover over signoff block,\n> or single (and perhaps larger size) avatar on hover over signoff line.\n> This can be done using pure CSS, without JavaScript[1]\n>\n> [1] http://meyerweb.com/eric/css/edge/popups/demo2.html\n\nOh, that'd be an interesting variant. Straightforward to implement in\nthe table case, too.\n\n> And here is version with (gr)avatar on the left side of signoff lines\n> (take a look if it is not better layout):\n>\n> diff --git c/gitweb/gitweb.perl w/gitweb/gitweb.perl\n> index 301bdd8..7701bac 100755\n> --- c/gitweb/gitweb.perl\n> +++ w/gitweb/gitweb.perl\n> @@ -3407,6 +3407,8 @@ sub git_print_log {\n>                        $signoff = 1;\n>                        $empty = 0;\n>                        if (! $opts{'-remove_signoff'}) {\n> +                               my ($email) = $line =~ /<(\\S+@\\S+)>/;\n> +                               print git_get_avatar($email, 'pad_after' => 1) if $email;\n>                                print \"\" . esc_html($line) . \"
\\n\";\n>                                next;\n>                        } else {\n\nI think that putting them aligned on the right is somewhat better\nbecause both in commit(diff) and in log view the author/committer\navatar is already on the right.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"117085","messageId":"200906271414.53717.jnareb@gmail.com","threadId":"19926","inReplyTo":"cb7bb73a0906261106n5e12948dydd02bd8d1b19a5e6@mail.gmail.com","subject":"Re: [PATCHv6 3/8] gitweb: right-align date cell in shortlog","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-27T12:14:52Z","receivedAt":"2009-06-27T12:14:52Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 26 June 2009, Giuseppe Bilotta wrote:\n> 2009/6/26 Jakub Narebski :\n>> On Thu, 25 June 2009, Giuseppe Bilotta wrote:\n>>\n>>> diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css\n>>> index 68b22ff..7240ed7 100644\n>>> --- a/gitweb/gitweb.css\n>>> +++ b/gitweb/gitweb.css\n>>> @@ -180,6 +180,10 @@ table {\n>>\n>>> +table.shortlog td:first-child{\n>>> +     text-align: right;\n>>> +}\n\n>> Second, I'd rather avoid more advanced CSS constructs; not all web\n>> browsers support ':first-child' selector.  On the other hand adding\n>> class attribute to handle this would make page slightly larger.\n> \n> IIRC :first-child is supported from IE7 onwards. There are hacks to\n> make it work on IE6, but I think they are definitely not worth it.\n\nI was thinking here about more exotic web browsers, like Lynx, ELinks,\nw3m (and w3m in Emacs), Konqueror (KHTML). I think that both Opera\nand Safari (and other browsers based on WebKit engine) support \n':first-child' pseudo-class selector. Also I'd rather not start trend\nto use more advanced parts of CSS...\n\nOn the other hand if we introduce 'age coloring' (as used in projects\nlist view), by using classes age0..age2, then we would be able to use\nclass selector instead of :first-child pseudo-class selector for that.\n\n>> Last, and most important: I don't agree with this change.  In my\n>> opinion it does not improve layout (and you didn't provide support\n>> for this change).  Right-align justification should be sparingly,\n>> as it is not natural in left-to-right languages.\n> \n> Of course, in my opinion it does improve layout.\n> \n> The effect is to right-align the first column of shortlog view, i.e.\n> the one holding the date. For dates that are presented as yyyy-mm-dd\n> it makes not difference, but when the phrasing is 'X days ago' it\n> provides the benefit of aligning the 'days ago' part instead of having\n> it ragged. See it live at\n> \n> http://git.oblomov.eu/git/shortlog\n> \n> and judge for yourselves.\n\nFirst, it would be nice to have\n\n See it live at http://git.oblomov.eu/git/shortlog\n\nin the patch comment (between \"---\\n\" line and diffstat).\n\nAnd I took a look how it looks like, with:\n $ \n $ \n $ git am -3 \n $ stg uncommit -n \n $ stg pop -a\n $ stg push\n $ gitweb-update.sh\n $ \n $ ...\n\n\nSecond, even disregarding using ':first-child' pseudo-class selector\n(it is not that important issue that it is required to have this\nsupported in all browsers), there is problem that this change is\n_incomplete_. Take a look at 'summary' view (probably most used\nproject specific action): in 'shortlog' you have date right-aligned,\nwhile date column in 'heads' and 'tags' parts below you have date\nleft-aligned.\n\nThird, in my opinion it does not improve layout. You align on least\nimportant part of relative date specification: on the word \"ago\".\nUnit specifiers in relative date specification are of different length\nso you don't have align (or rather have align in sort subsequences).\n\nCompare:\n\n 15 min ago\n 6 hours ago\n 10 hours ago\n 2 days ago\n 2 weeks ago\n 6 months ago\n 2009-06-12\n\nwith\n\n 15 min ago\n 6 hours ago\n 10 hours ago\n 2 days ago\n 2 weeks ago\n 6 months ago\n 2009-06-12\n\nWhat you probably want to have (and which I am not sure if it is worth\ncomplication) is to align on first space/whitespace (align=\"char\" char=\" \")\n\n 15 min ago\n 6 hours ago\n 10 hours ago\n 2 days ago\n 2 weeks ago\n 6 months ago\n 2009-06-12\n\nor even\n\n 15 min ago\n 6 hours ago\n 10 hours ago\n 2 days ago\n 2 weeks ago\n 6 months ago\n 2009-06-12\n\n-- \nJakub Narebski\nPoland\n"},{"id":"117086","messageId":"200906271449.52832.jnareb@gmail.com","threadId":"19926","inReplyTo":"200906271414.53717.jnareb@gmail.com","subject":"Re: [PATCHv6 3/8] gitweb: right-align date cell in shortlog","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-27T12:49:52Z","receivedAt":"2009-06-27T12:49:52Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sat, 27 Jun 2009, Jakub Narebski wrote:\n> On Fri, 26 June 2009, Giuseppe Bilotta wrote:\n>> 2009/6/26 Jakub Narebski :\n\n>>> Last, and most important: I don't agree with this change.  In my\n>>> opinion it does not improve layout (and you didn't provide support\n>>> for this change).  Right-align justification should be sparingly,\n>>> as it is not natural in left-to-right languages.\n>> \n>> Of course, in my opinion it does improve layout.\n\n[..]\n> Compare:\n> \n> 15 min ago\n> 6 hours ago\n> 10 hours ago\n> 2 days ago\n> 2 weeks ago\n> 6 months ago\n> 2009-06-12\n> \n> with\n> \n> 15 min ago\n> 6 hours ago\n> 10 hours ago\n> 2 days ago\n> 2 weeks ago\n> 6 months ago\n> 2009-06-12\n\nIt looks IMHO even worse if, because of limited screen width, 'date'\ncolumn needs to be wrapped. See\n\n 6 months\n ago\n\nversus\n\n 6 months\n ago\n\n-- \nJakub Narebski\nPoland\n"}]}