{"thread":{"id":"19864","subject":"[PATCH] gitweb: gravatar support","startedAt":"2009-06-19T09:58:15Z","lastAt":"2009-06-20T06:30:58Z","messageCount":5,"participants":["Giuseppe Bilotta","Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"116620","messageId":"1245405495-7712-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"19864","inReplyTo":null,"subject":"[PATCH] gitweb: gravatar support","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-19T09:58:15Z","receivedAt":"2009-06-19T09:58:15Z","isPatch":true,"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.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 41 ++++++++++++++++++++++++++++++++++++-----\n 1 files changed, 36 insertions(+), 5 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 1e7e2d8..9f40070 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -365,6 +365,18 @@ 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# Project specific override is not supported.\n+\t'gravatar' => {\n+\t\t'override' => 0,\n+\t\t'default' => [0]},\n );\n \n sub gitweb_get_feature {\n@@ -814,6 +826,9 @@ $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') && (eval { use Digest::MD5 qw(md5_hex); 1; });\n+\n # dispatch\n if (!defined $action) {\n \tif (defined $hash) {\n@@ -4123,6 +4138,19 @@ sub git_project_list_body {\n \tprint \"\\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) = @_;\n+\t\t$size = 32 if $size <= 0;\n+\t\treturn \"\";\n+\t} else {\n+\t\treturn \"\";\n+\t}\n+}\n+\n sub git_shortlog_body {\n \t# uses global variable $project\n \tmy ($commitlist, $from, $to, $refs, $extra) = @_;\n@@ -4145,7 +4173,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) . \" \" . $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@@ -5095,8 +5123,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 +5212,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.1.282.g9f93\n"},{"id":"116624","messageId":"200906191325.00248.jnareb@gmail.com","threadId":"19864","inReplyTo":"1245405495-7712-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCH] gitweb: gravatar support","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-19T11:24:58Z","receivedAt":"2009-06-19T11:24:58Z","isPatch":true,"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> \n> The feature is disabled by default, and depends on Digest::MD5.\n\nDigest::MD5 is core Perl module and (in, I guess, most cases)\ninstalled together with Perl.\n\nYou might want to mention it in the commit message.\n\n> \n> Signed-off-by: Giuseppe Bilotta \n> ---\n> gitweb/gitweb.perl | 41 ++++++++++++++++++++++++++++++++++++-----\n> 1 files changed, 36 insertions(+), 5 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 1e7e2d8..9f40070 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -365,6 +365,18 @@ 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# Project specific override is not supported.\n> +\t'gravatar' => {\n> +\t\t'override' => 0,\n> +\t\t'default' => [0]},\n\nYet another global feature without project specific override. Hmmm...\nI wonder if project specific and global (non-overridable) features \nshould be separated. But it is question for a separate commit.\n\nQuestion: why it is not overridable (why project specific override\nis not supported for this feature)? Some projects may use Gravatars,\nsome might not, although I guess that usually it is deployment specific\nfeature.\n\n> );\n> \n> sub gitweb_get_feature {\n> @@ -814,6 +826,9 @@ $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') && (eval { use Digest::MD5 qw(md5_hex); 1; });\n\nI would use line break here:\n\n +our $git_gravatar_enabled = gitweb_check_feature('gravatar') && \n + (eval { use Digest::MD5 qw(md5_hex); 1; });\n\nor\n\n +our $git_gravatar_enabled = gitweb_check_feature('gravatar')\n + && (eval { use Digest::MD5 qw(md5_hex); 1; });\n\n\nI really like the fact that we do not include Digest::MD5 if it is not\nrequired (if feature is not tuned on).\n\n> +\n> # dispatch\n> if (!defined $action) {\n> \tif (defined $hash) {\n> @@ -4123,6 +4138,19 @@ sub git_project_list_body {\n> \tprint \"
author\" . esc_html($co{'author'}) . \"
author\" . esc_html($co{'author'}) . \"\" .git_get_gravatar(lc $co{'author_email'}) . \"
$ad{'rfc2822'}\";\n \tif ($ad{'hour_local'} < 6) {\n@@ -5195,7 +5225,8 @@ sub git_commit {\n \t}\n \tprint \"
committer\" . esc_html($co{'committer'}) . \"
committer\" . esc_html($co{'committer'}) . \"\" .git_get_gravatar(lc $co{'committer_email'}) . \"
$cd{'rfc2822'}\" .\n \t sprintf(\" (%02d:%02d %s)\", $cd{'hour_local'}, $cd{'minute_local'}, $cd{'tz_local'}) .\n \t \"
\\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) = @_;\n> +\t\t$size = 32 if $size <= 0;\n\nYou would probably want to protect against $size being undefined:\n\n +\t\t$size = 32 if (!defined($size) || $size <= 0);\n\nBecause currently when you are not passing size parameter to use \ndefault size you would get the following warning:\n\n Use of uninitialized value in numeric le (<=) ...\n\nDid you run the t9500, adding test enabling gravatars? \n\nHmmm... perhaps we should add test with all possible features turned\non (unless they need extra configuration... oh, so it isn't as easy\nas I initially thought to add this...).\n\n> +\t\treturn \" +\t\t md5_hex($email) . \"&size=$size\\\" style=\\\"vertical-align:middle\\\" />\";\n\nHmmm... 'style=\"vertical-align:middle\"' or 'class=\"gravatar\"' (plus \naddition to CSS)?\n\n> +\t} else {\n> +\t\treturn \"\";\n> +\t}\n> +}\n> +\n\n\n[...]\n> @@ -4145,7 +4173,7 @@ sub git_shortlog_body {\n\n> +\t\t \"\" . git_get_gravatar($co{'author_email'}, 16) . \" \" . $author . \"\\n\" .\n\n> @@ -5095,8 +5123,9 @@ sub git_log {\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> @@ -5183,7 +5212,8 @@ sub git_commit {\n\n> +\tprint \"author\" . esc_html($co{'author'}) . \"\".\n> +\t \"\" .git_get_gravatar(lc $co{'author_email'}) . \"\\n\" .\n\n> +\tprint \"committer\" . esc_html($co{'committer'}) . \"\".\n> +\t \"\" .git_get_gravatar(lc $co{'committer_email'}) . \"\\n\";\n\nHmmm... why use 'lc' here and not in other places? Also gravatars are\nnot shown in 'history' view, but I guess that could wait for proper\nrefactoring of all log-like views/actions to use common infrastructure.\n\nThose all look nice with the *default* font sizes. But as the size of\ngravatar is used when constructing gravatar URL, to pass to gravatar.com\nI don't see how this problem can be resolved... Beside making it \nconfigurable, I guess...\n\n-- \nJakub Narebski\nPoland\n"},{"id":"116629","messageId":"cb7bb73a0906190757o4d083735t1a3f37396af76c2f@mail.gmail.com","threadId":"19864","inReplyTo":"200906191325.00248.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: gravatar support","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-19T14:57:23Z","receivedAt":"2009-06-19T14:57:23Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Hello,\n\nthanks for the comments, I'll send a new patch taking them into\nconsideration soon.\n\nA few additional replies:\n\n>> +     # Gravatar support. When this feature is enabled, views such as\n>> +     # shortlog or commit will display the gravatar associated with\n>> +     # the email of the committer(s) and/or author(s). Please note that\n>> +     # the feature depends on Digest::MD5.\n>> +\n>> +     # To enable system wide have in $GITWEB_CONFIG\n>> +     # $feature{'gravatar'}{'default'} = [1];\n>> +     # Project specific override is not supported.\n>> +     'gravatar' => {\n>> +             'override' => 0,\n>> +             'default' => [0]},\n>\n> Yet another global feature without project specific override.  Hmmm...\n> I wonder if project specific and global (non-overridable) features\n> should be separated.  But it is question for a separate commit.\n>\n> Question: why it is not overridable (why project specific override\n> is not supported for this feature)?  Some projects may use Gravatars,\n> some might not, although I guess that usually it is deployment specific\n> feature.\n\nI see it as a deployment feature, and considering that it adds an\n(admittedly small) extra load on the server, I thought it was sensible\nto make it non-overridable. OTOH, since the load is small, it might be\npossible to make it per-project without big issues.\n\n> You would probably want to protect against $size being undefined:\n>\n>  +             $size = 32 if (!defined($size) || $size <= 0);\n>\n> Because currently when you are not passing size parameter to use\n> default size you would get the following warning:\n>\n>  Use of uninitialized value in numeric le (<=) ...\n\nOh right.\n\n> Did you run the t9500, adding test enabling gravatars?\n\nEhrm, no 8-P\n\n> Hmmm... perhaps we should add test with all possible features turned\n> on (unless they need extra configuration... oh, so it isn't as easy\n> as I initially thought to add this...).\n\nIf gravatars can be added easily, I can try doing it.\n\n>> +     print \"author\" . esc_html($co{'author'}) . \"\".\n>> +           \"\" .git_get_gravatar(lc $co{'author_email'}) . \"\\n\" .\n>\n>> +     print \"committer\" . esc_html($co{'committer'}) . \"\".\n>> +           \"\" .git_get_gravatar(lc $co{'committer_email'}) . \"\\n\";\n>\n> Hmmm... why use 'lc' here and not in other places?\n\nThat's a good question. I'm actually not sure if the gravatar service\nis case sensitive or not ... the reference implementation uses lc. I\nshould probably move the lowercasing to git_get_gravar itself.\n\n> Also gravatars are\n> not shown in 'history' view, but I guess that could wait for proper\n> refactoring of all log-like views/actions to use common infrastructure.\n\nThat's part of the reason, the other being that I couldn't find a\nsatisfactory way to do it 8-P\n\n> Those all look nice with the *default* font sizes.  But as the size of\n> gravatar is used when constructing gravatar URL, to pass to gravatar.com\n> I don't see how this problem can be resolved...  Beside making it\n> configurable, I guess...\n\nThat's something I hadn't thought about, honestly. The problem of\ncourse is that the font sizes get customized via CSS, but the gravatar\nsize would get customized at the cgi level ... so unless we parse the\nCSS from the cgi it cannot be done automatically.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"116646","messageId":"200906200031.55576.jnareb@gmail.com","threadId":"19864","inReplyTo":"cb7bb73a0906190757o4d083735t1a3f37396af76c2f@mail.gmail.com","subject":"Re: [PATCH] gitweb: gravatar support","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-19T22:31:53Z","receivedAt":"2009-06-19T22:31:53Z","isPatch":true,"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> thanks for the comments, I'll send a new patch taking them into\n> consideration soon.\n\nErrr... I had a few additional comments to this reply, but I was a bit\nlate I think with replying.\n\n> \n> A few additional replies:\n> \n>>> +     # Gravatar support. When this feature is enabled, views such as\n>>> +     # shortlog or commit will display the gravatar associated with\n>>> +     # the email of the committer(s) and/or author(s). Please note that\n>>> +     # the feature depends on Digest::MD5.\n>>> +\n>>> +     # To enable system wide have in $GITWEB_CONFIG\n>>> +     # $feature{'gravatar'}{'default'} = [1];\n>>> +     # Project specific override is not supported.\n>>> +     'gravatar' => {\n>>> +             'override' => 0,\n>>> +             'default' => [0]},\n>>\n>> Yet another global feature without project specific override.  Hmmm...\n>> I wonder if project specific and global (non-overridable) features\n>> should be separated.  But it is question for a separate commit.\n>>\n>> Question: why it is not overridable (why project specific override\n>> is not supported for this feature)?  Some projects may use Gravatars,\n>> some might not, although I guess that usually it is deployment specific\n>> feature.\n> \n> I see it as a deployment feature, and considering that it adds an\n> (admittedly small) extra load on the server, I thought it was sensible\n> to make it non-overridable. OTOH, since the load is small, it might be\n> possible to make it per-project without big issues.\n\nWell, I was not asking you to change it; I was asking about \njustification behind making it non-overridable.\n\n> \n>> You would probably want to protect against $size being undefined:\n>>\n>>  +             $size = 32 if (!defined($size) || $size <= 0);\n>>\n>> Because currently when you are not passing size parameter to use\n>> default size you would get the following warning:\n>>\n>>  Use of uninitialized value in numeric le (<=) ...\n> \n> Oh right.\n\nWell, as you use 'undef' (do not pass parameter) for default value\nand you _do not_ use negative (or zero) value, then\n\n+ $size = 32 if !defined($size);\n\nwould be enough (in Perl 6 / Perl 5.10 it would be \"$size //= 32\" ;-)\n\nThe question is: does gravatar.com accepts any size? What does it do\nif it gets negative size passed? I have not used gravatar as \ndeveloper...\n\n> \n>> Did you run the t9500, adding test enabling gravatars?\n> \n> Ehrm, no 8-P\n\nWell, it is always nice to have test for new feature. Unfortunately\ndue to the smart way it is done currently unmodified (not extended)\nt9500 wouldn't catch above issue.\n\n[...]\n\n>> Also gravatars are\n>> not shown in 'history' view, but I guess that could wait for proper\n>> refactoring of all log-like views/actions to use common infrastructure.\n> \n> That's part of the reason, the other being that I couldn't find a\n> satisfactory way to do it 8-P\n\nWhat is the problem?\n\n> \n>> Those all look nice with the *default* font sizes.  But as the size of\n>> gravatar is used when constructing gravatar URL, to pass to gravatar.com\n>> I don't see how this problem can be resolved...  Beside making it\n>> configurable, I guess...\n> \n> That's something I hadn't thought about, honestly. The problem of\n> course is that the font sizes get customized via CSS, but the gravatar\n> size would get customized at the cgi level ... so unless we parse the\n> CSS from the cgi it cannot be done automatically.\n\nWhat I though here was to use %gravatar_size hash, with keys such as\n'default' and 'double' (for \"double line height\"). If you change or\nadd CSS, changing configuration, you can change also this in config.\n\nMore comments to follow in v2 and replies.\n-- \nJakub Narebski\nPoland\n"},{"id":"116660","messageId":"cb7bb73a0906192330j7c7a8ed3o39f0eb65d29e334c@mail.gmail.com","threadId":"19864","inReplyTo":"200906200031.55576.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: gravatar support","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-06-20T06:30:58Z","receivedAt":"2009-06-20T06:30:58Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Sat, Jun 20, 2009 at 12:31 AM, Jakub Narebski wrote:\n>>> Question: why it is not overridable (why project specific override\n>>> is not supported for this feature)?  Some projects may use Gravatars,\n>>> some might not, although I guess that usually it is deployment specific\n>>> feature.\n>>\n>> I see it as a deployment feature, and considering that it adds an\n>> (admittedly small) extra load on the server, I thought it was sensible\n>> to make it non-overridable. OTOH, since the load is small, it might be\n>> possible to make it per-project without big issues.\n>\n> Well, I was not asking you to change it; I was asking about\n> justification behind making it non-overridable.\n\nI decided to go for the override because it was little effort and it\nwhether or not the override can take place can still be decided at a\ncentral place.\n\n>>> You would probably want to protect against $size being undefined:\n>>>\n>>>  +             $size = 32 if (!defined($size) || $size <= 0);\n>>>\n>>> Because currently when you are not passing size parameter to use\n>>> default size you would get the following warning:\n>>>\n>>>  Use of uninitialized value in numeric le (<=) ...\n>>\n>> Oh right.\n>\n> Well, as you use 'undef' (do not pass parameter) for default value\n> and you _do not_ use negative (or zero) value, then\n>\n> +             $size = 32 if !defined($size);\n>\n> would be enough (in Perl 6 / Perl 5.10 it would be \"$size //= 32\" ;-)\n>\n> The question is: does gravatar.com accepts any size?  What does it do\n> if it gets negative size passed?  I have not used gravatar as\n> developer...\n\nIf you pass a negative size, the image comes up blank. Using undef for\nthe default value makes sense. The question rather becomes whether it\nmakes sense or not to provide a safety fallback for when the size\ncomes up negative.\n\n>>> Also gravatars are\n>>> not shown in 'history' view, but I guess that could wait for proper\n>>> refactoring of all log-like views/actions to use common infrastructure.\n>>\n>> That's part of the reason, the other being that I couldn't find a\n>> satisfactory way to do it 8-P\n>\n> What is the problem?\n\nI really don't remember, I created the patch a long time ago. As you\ncan probably seen from v2, I just decided to go with the same code\neverywhere (which asks for a refactoring).\n\n>>> Those all look nice with the *default* font sizes.  But as the size of\n>>> gravatar is used when constructing gravatar URL, to pass to gravatar.com\n>>> I don't see how this problem can be resolved...  Beside making it\n>>> configurable, I guess...\n>>\n>> That's something I hadn't thought about, honestly. The problem of\n>> course is that the font sizes get customized via CSS, but the gravatar\n>> size would get customized at the cgi level ... so unless we parse the\n>> CSS from the cgi it cannot be done automatically.\n>\n> What I though here was to use %gravatar_size hash, with keys such as\n> 'default' and 'double' (for \"double line height\").  If you change or\n> add CSS, changing configuration, you can change also this in config.\n\nAnd since the sices could be used for other things than gravatar, we\ncould call it just %avatar_size_hash or whatever, and future support\nfor picons or whatever else can use it too.\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"}]}