{"thread":{"id":"19867","subject":"[PATCHv2] gitweb: gravatar support","startedAt":"2009-06-19T18:21:10Z","lastAt":"2009-06-20T19:24:31Z","messageCount":9,"participants":["Giuseppe Bilotta","Johannes Schindelin","Junio C Hamano","Jakub Narebski","Aaron Crane"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"116631","messageId":"1245435670-5688-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"19867","inReplyTo":null,"subject":"[PATCHv2] gitweb: gravatar support","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-19T18:21:10Z","receivedAt":"2009-06-19T18:21:10Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Introduce gravatar support by adding the appropriate img tag next to\nauthor and committer in commit, shortlog and log view.\n\nThe feature is disabled by default, and depends on Digest::MD5, which\nis available in most Perl installations.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.css | 4 ++++\n gitweb/gitweb.perl | 51 ++++++++++++++++++++++++++++++++++++++++++++-------\n 2 files changed, 48 insertions(+), 7 deletions(-)\n\nChanges from the previous version include gravatar use in history view,\nCSS use and the possibility to override the feature on a per-project basis.\n\ndiff --git a/gitweb/gitweb.css b/gitweb/gitweb.css\nindex a01eac8..ca716e6 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.gravatar {\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 1e7e2d8..c06356b 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -365,6 +365,21 @@ our %feature = (\n \t\t'sub' => \\&feature_patches,\n \t\t'override' => 0,\n \t\t'default' => [16]},\n+\n+\t# Gravatar support. When this feature is enabled, views such as\n+\t# shortlog or commit will display the gravatar associated with\n+\t# the email of the committer(s) and/or author(s). Please note that\n+\t# the feature depends on Digest::MD5.\n+\n+\t# To enable system wide have in $GITWEB_CONFIG\n+\t# $feature{'gravatar'}{'default'} = [1];\n+\t# To have project specific config enable override in $GITWEB_CONFIG\n+\t# $feature{'gravatar'}{'override'} = 1;\n+\t# and in project config gitweb.gravatar = 0|1;\n+\t'gravatar' => {\n+\t\t'sub' => sub { feature_bool('gravatar', @_) },\n+\t\t'override' => 0,\n+\t\t'default' => [0]},\n );\n \n sub gitweb_get_feature {\n@@ -814,6 +829,10 @@ $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 gravatars are enabled and dependencies are satisfied\n+our $git_gravatar_enabled = gitweb_check_feature('gravatar') &&\n+\t(eval { use Digest::MD5 qw(md5_hex); 1; });\n+\n # dispatch\n if (!defined $action) {\n \tif (defined $hash) {\n@@ -3214,11 +3233,26 @@ sub git_print_header_div {\n \t \"\\n\\n\";\n }\n \n+# insert a gravatar for the given $email at the given $size if the feature\n+# is enabled\n+sub git_get_gravatar {\n+\tif ($git_gravatar_enabled) {\n+\t\tmy ($email, $size, $whitespace) = @_;\n+\t\t$whitespace = 0 unless defined($whitespace);\n+\t\t$size = 32 if (!defined($size) || $size <= 0);\n+\t\treturn \"\" . ($whitespace ? \" \" : \"\");\n+\t} else {\n+\t\treturn \"\";\n+\t}\n+}\n+\n sub git_print_authorship {\n \tmy $co = shift;\n \n \tmy %ad = parse_date($co->{'author_epoch'}, $co->{'author_tz'});\n \tprint \"
\" .\n+\t git_get_gravatar($co->{'author_email'}, 16, 1) .\n \t esc_html($co->{'author_name'}) .\n \t \" [$ad{'rfc2822'}\";\n \tif ($ad{'hour_local'} < 6) {\n@@ -4145,7 +4179,7 @@ sub git_shortlog_body {\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 \"\" . git_get_gravatar($co{'author_email'}, 16, 1) . \"\" . $author . \"\\n\" .\n \t\t \"\";\n \t\tprint format_subject_html($co{'title'}, $co{'title_short'},\n \t\t href(action=>\"commit\", hash=>$commit), $ref);\n@@ -4196,7 +4230,7 @@ sub git_history_body {\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 \"\" . git_get_gravatar($co{'author_email'}, 16, 1) . \"\" . $author . \"\\n\" .\n \t\t \"\";\n \t\t# originally git_history used chop_str($co{'title'}, 50)\n \t\tprint format_subject_html($co{'title'}, $co{'title_short'},\n@@ -4352,7 +4386,7 @@ sub git_search_grep_body {\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 \"\" . git_get_gravatar($co{'author_email'}, 16, 1) . \"\" . $author . \"\\n\" .\n \t\t \"\" .\n \t\t $cgi->a({-href => href(action=>\"commit\", hash=>$co{'id'}),\n \t\t -class => \"list subject\"},\n@@ -5095,8 +5129,9 @@ sub git_log {\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 \"\" . esc_html($co{'author_name'}) . \" [$ad{'rfc2822'}] \" .\n+\t\t git_get_gravatar($co{'author_email'}, 16) .\n+\t\t \"
\\n\\n\";\n \n \t\tprint \"
\\n\";\n \t\tgit_print_log($co{'comment'}, -final_empty_line=> 1);\n@@ -5183,7 +5218,8 @@ sub git_commit {\n \t}\n \tprint \"
\\n\" .\n \t \"\\n\";\n-\tprint \"\\n\".\n+\tprint \"\".\n+\t \"\\n\" .\n \t \"\" .\n \t \"\" .\n \t \"\\n\";\n-\tprint \"\\n\";\n+\tprint \"\".\n+\t \"\\n\";\n \tprint \"\\n\";\n-- \n1.6.3.rc1.192.gdbfcb\n"},{"id":"116632","messageId":"alpine.DEB.1.00.0906192025000.4265@intel-tinevez-2-302","threadId":"19867","inReplyTo":"1245435670-5688-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv2] gitweb: gravatar support","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-06-19T18:26:18Z","receivedAt":"2009-06-19T18:26:18Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 19 Jun 2009, Giuseppe Bilotta wrote:\n\n> Introduce gravatar support by adding the appropriate img tag next to\n> author and committer in commit, shortlog and log view.\n> \n> The feature is disabled by default, and depends on Digest::MD5, which\n> is available in most Perl installations.\n> \n> Signed-off-by: Giuseppe Bilotta \n\nSomething important is missing in this message, which was the reason why I \nhad to go through the hassle of reading the source code to find out!\n\n\tThis feature is optional and can be enabled by ...\n\nHth,\nDscho\n"},{"id":"116633","messageId":"alpine.DEB.1.00.0906192035590.4265@intel-tinevez-2-302","threadId":"19867","inReplyTo":"alpine.DEB.1.00.0906192025000.4265@intel-tinevez-2-302","subject":"Re: [PATCHv2] gitweb: gravatar support","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-06-19T18:36:34Z","receivedAt":"2009-06-19T18:36:34Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 19 Jun 2009, Johannes Schindelin wrote:\n\n> On Fri, 19 Jun 2009, Giuseppe Bilotta wrote:\n> \n> > Introduce gravatar support by adding the appropriate img tag next to \n> > author and committer in commit, shortlog and log view.\n> > \n> > The feature is disabled by default, and depends on Digest::MD5, which \n> > is available in most Perl installations.\n> > \n> > Signed-off-by: Giuseppe Bilotta \n> \n> Something important is missing in this message, which was the reason why \n> I had to go through the hassle of reading the source code to find out!\n> \n> \tThis feature is optional and can be enabled by ...\n\nOops. Please disregard my response.\n\nCiao,\nDscho\n"},{"id":"116637","messageId":"7v63esklxh.fsf@alter.siamese.dyndns.org","threadId":"19867","inReplyTo":"1245435670-5688-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv2] gitweb: gravatar support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-06-19T20:28:42Z","receivedAt":"2009-06-19T20:28:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta writes:\n\n> Introduce gravatar support by adding the appropriate img tag next to\n> author and committer in commit, shortlog and log view.\n>\n> The feature is disabled by default, and depends on Digest::MD5, which\n> is available in most Perl installations.\n>\n> Signed-off-by: Giuseppe Bilotta \n> ---\n> gitweb/gitweb.css | 4 ++++\n> gitweb/gitweb.perl | 51 ++++++++++++++++++++++++++++++++++++++++++++-------\n> 2 files changed, 48 insertions(+), 7 deletions(-)\n>\n> Changes from the previous version include gravatar use in history view,\n> CSS use and the possibility to override the feature on a per-project basis.\n\nI see these repeated patterns in your patch.\n\n> @@ -4145,7 +4179,7 @@ sub git_shortlog_body {\n> \t\tmy $author = chop_and_escape_str($co{'author_name'}, 10);\n> \t\t# git_summary() used print \"\\n\" .\n> \t\tprint \"\\n\" .\n> -\t\t \"\\n\" .\n> +\t\t \"\\n\" .\n>...\n> -\t\t \"\\n\" .\n> +\t\t \"\\n\" .\n>...\n> -\t\t \"\\n\" .\n> +\t\t \"\\n\" .\n>...\n> -\tprint \"\\n\".\n> +\tprint \"\".\n> +\t \"\\n\" .\n>...\n> -\tprint \"\\n\";\n> +\tprint \"\".\n> +\t \"\\n\";\n>...\n\nDoesn't it strike you as needing a bit more refactoring? The first three\nare identical, and you can refactor them to\n\n-\t\t \"\\n\" .\n+\t\t \"\\n\" .\n\nwhere oneline_person is\n\n\tsub oneline_person {\n \tmy ($me) = @_;\n return ($me->{'smallicon'} .\n\t\t\t\"\" . $me->{'name_escaped'} . \"\");\n }\n\nAnd instead of doing \"my $author = chop_and_escape_str()\", you will\nset up richer my $author upfront, like so:\n\n\tmy $author = +{\n\t\tname_escaped => chop_and_escape_str($co{'author_name'}, 10),\n smallicon => git_get_gravatar($co{'author_email'}, 16, 1),\n };\n\nand pass it to the helper function.\n\nThat way, people who do not like italics, or people who want to have the\nicon at the end instead of at the beginning, can touch only one place\n(i.e. \"sub oneline_person\").\n\nBy the way, in the above example, I named the field 'smallicon', as use of\ngravatar is merely an implementation detail. It is plausible other people\nmay want to use picons instead.\n\nI do not know about the following hunk (why does it have the icon at the\nend, unlike the other ones?), but I think you got the idea.\n\n> -\t\t \"\" . esc_html($co{'author_name'}) . \" [$ad{'rfc2822'}]
\\n\" .\n> -\t\t \"\\n\";\n> +\t\t \"\" . esc_html($co{'author_name'}) . \" [$ad{'rfc2822'}] \" .\n> +\t\t git_get_gravatar($co{'author_email'}, 16) .\n> +\t\t \"
\\n\\n\";\n\nIn the medium term, we may want to go one step further and do\n\n package person;\n\nand make sets of methods like \"oneline_person\".\n"},{"id":"116647","messageId":"cb7bb73a0906191543l75722b35xa1af532f4d8e1b76@mail.gmail.com","threadId":"19867","inReplyTo":"7v63esklxh.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2] gitweb: gravatar support","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-19T22:43:46Z","receivedAt":"2009-06-19T22:43:46Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Fri, Jun 19, 2009 at 10:28 PM, Junio C Hamano wrote:\n>\n> I see these repeated patterns in your patch.\n>\n>> @@ -4145,7 +4179,7 @@ sub git_shortlog_body {\n>>               my $author = chop_and_escape_str($co{'author_name'}, 10);\n>>               # git_summary() used print \"\\n\" .\n>>               print \"\\n\" .\n>> -                   \"\\n\" .\n>> +                   \"\\n\" .\n>>...\n>> -                   \"\\n\" .\n>> +                   \"\\n\" .\n>>...\n>> -                   \"\\n\" .\n>> +                   \"\\n\" .\n>>...\n>> -     print \"\\n\".\n>> +     print \"\".\n>> +           \"\\n\" .\n>>...\n>> -     print \"\\n\";\n>> +     print \"\".\n>> +           \"\\n\";\n>>...\n>\n> Doesn't it strike you as needing a bit more refactoring?\n\nI was having the same thoughts while writing v2 of the patch, indeed.\nHowever, I wasn't sure if it was appropriate to the refactoring in the\nsame patch. A couple of places could be changed to use the existing\ngit_print_authorship, others would need their own function, as you\npoint out.\n\n> By the way, in the above example, I named the field 'smallicon', as use of\n> gravatar is merely an implementation detail.  It is plausible other people\n> may want to use picons instead.\n\nI had to google for picons but yes, it makes sense.\n\n> I do not know about the following hunk (why does it have the icon at the\n> end, unlike the other ones?), but I think you got the idea.\n>\n>> -                   \"\" . esc_html($co{'author_name'}) .  \" [$ad{'rfc2822'}]
\\n\" .\n>> -                   \"\\n\";\n>> +                   \"\" . esc_html($co{'author_name'}) .  \" [$ad{'rfc2822'}] \" .\n>> +                   git_get_gravatar($co{'author_email'}, 16) .\n>> +                   \"
\\n\\n\";\n\nI had the impression that in this case it made more aesthetical sense\nto have the icon on the other side. I'm not exactly a good designer\nthough, so we might want to prefer consistency and keep it all the\nsame way. This section would probably use git_print_authorship or at\nleast share some code with that.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"116668","messageId":"200906201257.44486.jnareb@gmail.com","threadId":"19867","inReplyTo":"1245435670-5688-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv2] gitweb: gravatar support","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-20T10:57:43Z","receivedAt":"2009-06-20T10:57:43Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 19 June 2009, Giuseppe Bilotta wrote:\n\n> Introduce gravatar support by adding the appropriate img tag next to\n> author and committer in commit, shortlog and log view.\n\nYou have added gravatar support also to 'history' view, which you\nshould mention here. By the way, I have noticed that there is no\ngravatar support in 'commitdiff' view... which probably means that,\nas Junio said, this would require some refactoring and factoring out\ncommon code.\n\n> \n> The feature is disabled by default, and depends on Digest::MD5, which\n> is available in most Perl installations.\n\nI wonder if there should be mentioned that if Digest::MD5 is not \ninstalled, the feature will not be used...\n\n> \n> Signed-off-by: Giuseppe Bilotta \n> ---\n> gitweb/gitweb.css | 4 ++++\n> gitweb/gitweb.perl | 51 ++++++++++++++++++++++++++++++++++++++++++++-------\n> 2 files changed, 48 insertions(+), 7 deletions(-)\n> \n> Changes from the previous version include gravatar use in history view,\n> CSS use and the possibility to override the feature on a per-project basis.\n\nThere are a few other 'code' changes...\n\n> \n> diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css\n> index a01eac8..ca716e6 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.gravatar {\n> +\tvertical-align:middle;\n> +}\n> +\n\nThat is good enough CSS class name for now, but in the future we might\nwant to use more generic name: avatar, personal_avatar, personal_icon;\nor something like that.\n\n> div.page_header {\n> \theight: 25px;\n> \tpadding: 8px;\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 1e7e2d8..c06356b 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -365,6 +365,21 @@ our %feature = (\n> \t\t'sub' => \\&feature_patches,\n> \t\t'override' => 0,\n> \t\t'default' => [16]},\n> +\n> +\t# Gravatar support. When this feature is enabled, views such as\n> +\t# shortlog or commit will display the gravatar associated with\n> +\t# the email of the committer(s) and/or author(s). Please note that\n> +\t# the feature depends on Digest::MD5.\n> +\n> +\t# To enable system wide have in $GITWEB_CONFIG\n> +\t# $feature{'gravatar'}{'default'} = [1];\n> +\t# To have project specific config enable override in $GITWEB_CONFIG\n> +\t# $feature{'gravatar'}{'override'} = 1;\n> +\t# and in project config gitweb.gravatar = 0|1;\n> +\t'gravatar' => {\n> +\t\t'sub' => sub { feature_bool('gravatar', @_) },\n> +\t\t'override' => 0,\n> +\t\t'default' => [0]},\n\nNice. I think it can be on per-project basis decision whether to\nuse avatars or not. For example if project uses fake emails, etc.\n\n[...]\n> +# insert a gravatar for the given $email at the given $size if the feature\n> +# is enabled\n> +sub git_get_gravatar {\n> +\tif ($git_gravatar_enabled) {\n> +\t\tmy ($email, $size, $whitespace) = @_;\n> +\t\t$whitespace = 0 unless defined($whitespace);\n> +\t\t$size = 32 if (!defined($size) || $size <= 0);\n> +\t\treturn \" +\t\t md5_hex(lc $email) . \"&size=$size\\\" />\" . ($whitespace ? \" \" : \"\");\n> +\t} else {\n> +\t\treturn \"\";\n> +\t}\n> +}\n\nI don't like this change. I don't like it at all.\n\nFirst, there is an issue of having two _independently_ optional\nparameters. Using 'undef' / not passing parameter to use default value\nis IMVHO an anti-pattern; it is a bit unwieldy / ugly to call it with\n$whitespace set while using default value for $size.\n\nSecond, I think it is better to avoid such boolean parameters like\n$whitespace, because you don't know from call site what it means\nto pass 1 there (in C we would use constant instead of 1 here).\n\nBoth of above are solved in current gitweb code (at least in some\nplaces) by passing hash of (named) optional parameters. So the call\n(if not for last note for this issue) would look like the following:\n\n git_get_gravatar($co->{'author_email'}, -size => 16, -whitespace => 1);\n\nThird, rather than formatting with ' ' (ugh, just like formatting\nwith tab and enter in word processors) I'd rather use (if possible)\nCSS style for that; something like margin-right, or padding-right...\nif it would work, of course. But see also note below.\n\n\nLast but not least, but after thinking about it a bit more I think now\nthat those optional parameters are entirely too low level, too close to\nimplementation. What matters is not the size (which you would then need\nto change in every and each callsite) and adding   after image.\nWhat matters is whether to use double size, and whether gravatar image\nis on the left side or on the right side of text it is referring to.\n\nYou would be then able to easily chose small/large size from hash of\npossible sizes (mentioned %avatar_size), and just add appropriate class\ndifferentiator for left/right hand side gravatar.\n\nSo the call would look like the following:\n\n git_get_gravatar($co->{'author_email'}, -large => 0, -position => 'left');\n\nor\n\n git_get_gravatar($co->{'author_email'}, -small => 1, -padding => 'right');\n\nor something like that[1] (-large, -small, -double_size, -inline, ...;\n-position, -padding, -justify, -pad, -space, -separate, ...).\n\n[1] Naming is unfortunately hard. Fortunately we can change it later.\n\n> +\n> sub git_print_authorship {\n> \tmy $co = shift;\n> \n> \tmy %ad = parse_date($co->{'author_epoch'}, $co->{'author_tz'});\n> \tprint \"
\" .\n> +\t git_get_gravatar($co->{'author_email'}, 16, 1) .\n> \t esc_html($co->{'author_name'}) .\n> \t \" [$ad{'rfc2822'}\";\n> \tif ($ad{'hour_local'} < 6) {\n\nThat is a very good idea to add it to git_print_authorship. This should\ntake care of many other places, and many views, I think. But you should\ncheck if it wouldn't make gravatar appear in unwanted places.\n\n> @@ -4145,7 +4179,7 @@ sub git_shortlog_body {\n> @@ -4196,7 +4230,7 @@ sub git_history_body {\n> @@ -4352,7 +4386,7 @@ sub git_search_grep_body {\n> @@ -5095,8 +5129,9 @@ sub git_log {\n\nThis would really, really be easier with log-like view refactoring. This\nis on my TODO list for gitweb for a quite long time...\n\n> @@ -5183,7 +5218,8 @@ sub git_commit {\n\n-- \nJakub Narebski\nPoland\n"},{"id":"116672","messageId":"20090620141626.GK7675@aaroncrane.co.uk","threadId":"19867","inReplyTo":"1245435670-5688-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv2] gitweb: gravatar support","fromName":"Aaron Crane","fromEmail":"git@aaroncrane.co.uk","sentAt":"2009-06-20T14:16:27Z","receivedAt":"2009-06-20T14:16:27Z","isPatch":false,"sender":{"key":"git@aaroncrane.co.uk","avatar":null},"body":"Giuseppe Bilotta writes:\n> +# check if gravatars are enabled and dependencies are satisfied\n> +our $git_gravatar_enabled = gitweb_check_feature('gravatar') &&\n> +\t(eval { use Digest::MD5 qw(md5_hex); 1; });\n\nThis test for the availability of Digest::MD5 is broken: `use`\nstatements are executed at compile time, so the whole program will\nfail if Digest::MD5 can't be loaded.\n\nA possible fix would be to move the compile-time actions to run time:\n\n our $git_gravatar_enabled = gitweb_check_feature('gravatar') &&\n (eval { require Digest::MD5; Digest::MD5->import('md5_hex'); 1 });\n\nHowever, I don't recommend doing that. Digest::MD5 is a core module\nin Perl 5.8.0 and later, so an installation of Perl 5.8 that doesn't\nhave it is broken. Since gitweb.perl already needs 5.8 (because of\nthe `binmode STDOUT, ':utf8'` at the top, if nothing else) I see no\nvalue in jumping through hoops to make this work in the essentially\nimpossible situation where Digest::MD5 is unavailable.\n\n-- \nAaron Crane ** http://aaroncrane.co.uk/\n"},{"id":"116674","messageId":"200906201858.57346.jnareb@gmail.com","threadId":"19867","inReplyTo":"20090620141626.GK7675@aaroncrane.co.uk","subject":"Re: [PATCHv2] gitweb: gravatar support","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-20T16:58:56Z","receivedAt":"2009-06-20T16:58:56Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sat, 20 June 2009, Aaron Crane wrote:\n> Giuseppe Bilotta writes:\n\n> > +# check if gravatars are enabled and dependencies are satisfied\n> > +our $git_gravatar_enabled = gitweb_check_feature('gravatar') &&\n> > +\t(eval { use Digest::MD5 qw(md5_hex); 1; });\n> \n> This test for the availability of Digest::MD5 is broken: `use`\n> statements are executed at compile time, so the whole program will\n> fail if Digest::MD5 can't be loaded.\n> \n> A possible fix would be to move the compile-time actions to run time:\n> \n> our $git_gravatar_enabled = gitweb_check_feature('gravatar') &&\n> (eval { require Digest::MD5; Digest::MD5->import('md5_hex'); 1 });\n>\n\nGood catch!!! But we don't need import if we are to use slightly longer\nform: Digest::MD5::md5_hex.\n \n> However, I don't recommend doing that. Digest::MD5 is a core module\n> in Perl 5.8.0 and later, so an installation of Perl 5.8 that doesn't\n> have it is broken. Since gitweb.perl already needs 5.8 (because of\n> the `binmode STDOUT, ':utf8'` at the top, if nothing else) I see no\n> value in jumping through hoops to make this work in the essentially\n> impossible situation where Digest::MD5 is unavailable.\n\nIn this case the problem is not with Digest::MD5 not being installed.\nThe problem is with not loading this module in a CGI script if it is\nnot required (i.e. problem is not dependencies but performance).\n\nBTW. we sometimes unnecessary calculate MD5 hash repeatedly for the\nsame author / committer in log-like views... but I don't see how to\nsolve it in an easy way.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"116679","messageId":"200906202124.33278.jnareb@gmail.com","threadId":"19867","inReplyTo":"7v63esklxh.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2] gitweb: gravatar support","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-20T19:24:31Z","receivedAt":"2009-06-20T19:24:31Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 19 June 2009, Junio C Hamano wrote:\n> Giuseppe Bilotta writes:\n\n> I see these repeated patterns in your patch.\n> \n> > @@ -4145,7 +4179,7 @@ sub git_shortlog_body {\n> > \t\tmy $author = chop_and_escape_str($co{'author_name'}, 10);\n> > \t\t# git_summary() used print \"
\\n\" .\n> > \t\tprint \"\\n\" .\n> > -\t\t \"\\n\" .\n> > +\t\t \"\\n\" .\n> >...\n> > -\t\t \"\\n\" .\n> > +\t\t \"\\n\" .\n> >...\n> > -\t\t \"\\n\" .\n> > +\t\t \"\\n\" .\n> >...\n> > -\tprint \"\\n\".\n> > +\tprint \"\".\n> > +\t \"\\n\" .\n> >...\n> > -\tprint \"\\n\";\n> > +\tprint \"\".\n> > +\t \"\\n\";\n> >...\n> \n> Doesn't it strike you as needing a bit more refactoring? The first three\n> are identical, and you can refactor them to\n> \n> -\t\t \"\\n\" .\n> +\t\t \"\\n\" .\n> \n> where oneline_person is\n> \n> \tsub oneline_person {\n> \tmy ($me) = @_;\n> return ($me->{'smallicon'} .\n> \t\t\t\"\" . $me->{'name_escaped'} . \"\");\n> }\n[...]\n\nWell, gitweb certainly needs some love^W refactoring. That is only one\nof areas. But should we postpone introducing new features till gitweb\ngets cleaned up? Although in this case I guess we can do cleanup during\nintroduction of new feature (which makes stronger case for factorizing\ncommon elements, as they just get larger, those common elements, now with\nthis feature).\n\n> That way, people who do not like italics, or people who want to have the\n> icon at the end instead of at the beginning, can touch only one place\n> (i.e. \"sub oneline_person\").\n\nAbout using presentational HTML: I think one reason behind keeping it\nis to have gitweb work also in text browsers like lynx, w3m or lynx, \nwhich can not understand CSS.\n\n[...]\n> In the medium term, we may want to go one step further and do\n> \n> package person;\n> \n> and make sets of methods like \"oneline_person\".\n\nThat would be first in gitweb.\n\nBTW. Should we split gitweb into packages? Should we split gitweb into\nseparate files (one package per file)?\n-- \nJakub Narebski\nPoland\n"}]}
author\" . esc_html($co{'author'}) . \"
author\" . esc_html($co{'author'}) . \"\" .git_get_gravatar($co{'author_email'}) . \"
$ad{'rfc2822'}\";\n \tif ($ad{'hour_local'} < 6) {\n@@ -5195,7 +5231,8 @@ sub git_commit {\n \t}\n \tprint \"
committer\" . esc_html($co{'committer'}) . \"
committer\" . esc_html($co{'committer'}) . \"\" .git_get_gravatar($co{'committer_email'}) . \"
$cd{'rfc2822'}\" .\n \t sprintf(\" (%02d:%02d %s)\", $cd{'hour_local'}, $cd{'minute_local'}, $cd{'tz_local'}) .\n \t \"
$co{'age_string'}$co{'age_string_date'}\" . $author . \"\" . git_get_gravatar($co{'author_email'}, 16, 1) . \"\" . $author . \"\" . $author . \"\" . git_get_gravatar($co{'author_email'}, 16, 1) . \"\" . $author . \"\" . $author . \"\" . git_get_gravatar($co{'author_email'}, 16, 1) . \"\" . $author . \"
author\" . esc_html($co{'author'}) . \"
author\" . esc_html($co{'author'}) . \"\" .git_get_gravatar($co{'author_email'}) . \"
committer\" . esc_html($co{'committer'}) . \"
committer\" . esc_html($co{'committer'}) . \"\" .git_get_gravatar($co{'committer_email'}) . \"
\" . $author . \"\" . oneline_person($author) . \"$co{'age_string'}$co{'age_string_date'}\" . $author . \"\" . git_get_gravatar($co{'author_email'}, 16, 1) . \"\" . $author . \"\" . $author . \"\" . git_get_gravatar($co{'author_email'}, 16, 1) . \"\" . $author . \"\" . $author . \"\" . git_get_gravatar($co{'author_email'}, 16, 1) . \"\" . $author . \"
author\" . esc_html($co{'author'}) . \"
author\" . esc_html($co{'author'}) . \"\" .git_get_gravatar($co{'author_email'}) . \"
committer\" . esc_html($co{'committer'}) . \"
committer\" . esc_html($co{'committer'}) . \"\" .git_get_gravatar($co{'committer_email'}) . \"
$co{'age_string'}$co{'age_string_date'}\" . $author . \"\" . git_get_gravatar($co{'author_email'}, 16, 1) . \"\" . $author . \"\" . $author . \"\" . git_get_gravatar($co{'author_email'}, 16, 1) . \"\" . $author . \"\" . $author . \"\" . git_get_gravatar($co{'author_email'}, 16, 1) . \"\" . $author . \"
author\" . esc_html($co{'author'}) . \"
author\" . esc_html($co{'author'}) . \"\" .git_get_gravatar($co{'author_email'}) . \"
committer\" . esc_html($co{'committer'}) . \"
committer\" . esc_html($co{'committer'}) . \"\" .git_get_gravatar($co{'committer_email'}) . \"
\" . $author . \"\" . oneline_person($author) . \"