{"thread":{"id":"16338","subject":"[PATCH] gitweb: fixes to gitweb feature check code","startedAt":"2008-11-15T14:26:05Z","lastAt":"2008-11-17T10:48:03Z","messageCount":9,"participants":["Giuseppe Bilotta","Junio C Hamano","Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"95882","messageId":"1226759165-6894-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"16338","inReplyTo":null,"subject":"[PATCH] gitweb: fixes to gitweb feature check code","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-11-15T14:26:05Z","receivedAt":"2008-11-15T14:26:05Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"The gitweb_check_feature routine was being used for two different\npurposes: retrieving the actual feature value (such as the list of\nsnapshot formats or the list of additional actions), and to check if a\nfeature was enabled.\n\nFor the latter use, since all features return an array, it led to either\nclumsy code or subtle bugs, with disabled features appearing enabled\nbecause (0) evaluates to 1.\n\nWe fix these bugs, and simplify the code, by separating feature (list)\nvalue retrieval (gitweb_get_feature) from boolean feature check (i.e.\nretrieving the first/only item in the feature value list). Usage of\ngitweb_check_feature across gitweb is replaced by the appropriate call\nwherever needed.\n---\n gitweb/gitweb.perl | 52 +++++++++++++++++++++++++++++++++++-----------------\n 1 files changed, 35 insertions(+), 17 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 933e137..b0d00ea 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -190,7 +190,9 @@ our %feature = (\n \t# if there is no 'sub' key (no feature-sub), then feature cannot be\n \t# overriden\n \t#\n-\t# use gitweb_check_feature() to check if is enabled\n+\t# use gitweb_get_feature() to retrieve the value\n+\t# (an array) or gitweb_check_feature() to check if \n+\t# is enabled\n \n \t# Enable the 'blame' blob view, showing the last commit that modified\n \t# each line in the file. This can be very CPU-intensive.\n@@ -329,7 +331,8 @@ our %feature = (\n \t\t'default' => [0]},\n );\n \n-sub gitweb_check_feature {\n+# retrieve the value of a given feature, as an array\n+sub gitweb_get_feature {\n \tmy ($name) = @_;\n \treturn unless exists $feature{$name};\n \tmy ($sub, $override, @defaults) = (\n@@ -344,6 +347,21 @@ sub gitweb_check_feature {\n \treturn $sub->(@defaults);\n }\n \n+# check if a given feature is enabled or not, returning the first (and only)\n+# value of the feature. Comfort code, allowing the use of\n+# my $bool_feat = gitweb_check_feature('bool_feat');\n+# or\n+# gitweb_check_feature('bool_feat') or somecode;\n+# instead of\n+# my ($bool_feat) = gitweb_git_feature('bool_feat');\n+# or\n+# (gitweb_check_feature('bool_feat'))[0] or somecode;\n+# respectively\n+sub gitweb_check_feature {\n+\treturn (gitweb_get_feature(@_))[0];\n+}\n+\n+\n sub feature_blame {\n \tmy ($val) = git_get_project_config('blame', '--bool');\n \n@@ -767,7 +785,7 @@ our $git_dir;\n $git_dir = \"$projectroot/$project\" if $project;\n \n # list of supported snapshot formats\n-our @snapshot_fmts = gitweb_check_feature('snapshot');\n+our @snapshot_fmts = gitweb_get_feature('snapshot');\n @snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);\n \n # dispatch\n@@ -810,7 +828,7 @@ sub href (%) {\n \t\t}\n \t}\n \n-\tmy ($use_pathinfo) = gitweb_check_feature('pathinfo');\n+\tmy $use_pathinfo = gitweb_check_feature('pathinfo');\n \tif ($use_pathinfo) {\n \t\t# try to put as many parameters as possible in PATH_INFO:\n \t\t# - project name\n@@ -2101,7 +2119,7 @@ sub git_get_projects_list {\n \t$filter ||= '';\n \t$filter =~ s/\\.git$//;\n \n-\tmy ($check_forks) = gitweb_check_feature('forks');\n+\tmy $check_forks = gitweb_check_feature('forks');\n \n \tif (-d $projects_list) {\n \t\t# search in directory\n@@ -2947,7 +2965,7 @@ EOF\n \t}\n \tprint \"\\n\";\n \n-\tmy ($have_search) = gitweb_check_feature('search');\n+\tmy $have_search = gitweb_check_feature('search');\n \tif (defined $project && $have_search) {\n \t\tif (!defined $searchtext) {\n \t\t\t$searchtext = \"\";\n@@ -2961,7 +2979,7 @@ EOF\n \t\t\t$search_hash = \"HEAD\";\n \t\t}\n \t\tmy $action = $my_uri;\n-\t\tmy ($use_pathinfo) = gitweb_check_feature('pathinfo');\n+\t\tmy $use_pathinfo = gitweb_check_feature('pathinfo');\n \t\tif ($use_pathinfo) {\n \t\t\t$action .= \"/\".esc_url($project);\n \t\t}\n@@ -3084,7 +3102,7 @@ sub git_print_page_nav {\n \t$arg{'tree'}{'hash'} = $treehead if defined $treehead;\n \t$arg{'tree'}{'hash_base'} = $treebase if defined $treebase;\n \n-\tmy @actions = gitweb_check_feature('actions');\n+\tmy @actions = gitweb_get_feature('actions');\n \tmy %repl = (\n \t\t'%' => '%',\n \t\t'n' => $project, # project name\n@@ -3454,7 +3472,7 @@ sub is_patch_split {\n sub git_difftree_body {\n \tmy ($difftree, $hash, @parents) = @_;\n \tmy ($parent) = $parents[0];\n-\tmy ($have_blame) = gitweb_check_feature('blame');\n+\tmy $have_blame = gitweb_check_feature('blame');\n \tprint \"
\\n\";\n \tif ($#{$difftree} > 10) {\n \t\tprint(($#{$difftree} + 1) . \" files changed:\\n\");\n@@ -3968,7 +3986,7 @@ sub git_project_list_body {\n \t# actually uses global variable $project\n \tmy ($projlist, $order, $from, $to, $extra, $no_header) = @_;\n \n-\tmy ($check_forks) = gitweb_check_feature('forks');\n+\tmy $check_forks = gitweb_check_feature('forks');\n \tmy @projects = fill_project_list_info($projlist, $check_forks);\n \n \t$order ||= $default_projects_order;\n@@ -4428,7 +4446,7 @@ sub git_summary {\n \tmy @taglist = git_get_tags_list(16);\n \tmy @headlist = git_get_heads_list(16);\n \tmy @forklist;\n-\tmy ($check_forks) = gitweb_check_feature('forks');\n+\tmy $check_forks = gitweb_check_feature('forks');\n \n \tif ($check_forks) {\n \t\t@forklist = git_get_projects_list($project);\n@@ -4457,7 +4475,7 @@ sub git_summary {\n \t}\n \n \t# Tag cloud\n-\tmy $show_ctags = (gitweb_check_feature('ctags'))[0];\n+\tmy $show_ctags = gitweb_check_feature('ctags');\n \tif ($show_ctags) {\n \t\tmy $ctags = git_get_project_ctags($project);\n \t\tmy $cloud = git_populate_project_tagcloud($ctags);\n@@ -4747,7 +4765,7 @@ sub git_blob {\n \t\t$expires = \"+1d\";\n \t}\n \n-\tmy ($have_blame) = gitweb_check_feature('blame');\n+\tmy $have_blame = gitweb_check_feature('blame');\n \topen my $fd, \"-|\", git_cmd(), \"cat-file\", \"blob\", $hash\n \t\tor die_error(500, \"Couldn't cat $file_name, $hash\");\n \tmy $mimetype = blob_mimetype($fd, $file_name);\n@@ -4840,7 +4858,7 @@ sub git_tree {\n \tmy $ref = format_ref_marker($refs, $hash_base);\n \tgit_header_html();\n \tmy $basedir = '';\n-\tmy ($have_blame) = gitweb_check_feature('blame');\n+\tmy $have_blame = gitweb_check_feature('blame');\n \tif (defined $hash_base && (my %co = parse_commit($hash_base))) {\n \t\tmy @views_nav = ();\n \t\tif (defined $file_name) {\n@@ -5838,7 +5856,7 @@ insensitive).

\n
commit
\n
The commit messages and authorship information will be scanned for the given pattern.
\n EOT\n-\tmy ($have_grep) = gitweb_check_feature('grep');\n+\tmy $have_grep = gitweb_check_feature('grep');\n \tif ($have_grep) {\n \t\tprint <grep\n@@ -5855,7 +5873,7 @@ EOT\n
committer
\n
Name and e-mail of the committer and date of commit will be scanned for the given pattern.
\n EOT\n-\tmy ($have_pickaxe) = gitweb_check_feature('pickaxe');\n+\tmy $have_pickaxe = gitweb_check_feature('pickaxe');\n \tif ($have_pickaxe) {\n \t\tprint <pickaxe\n@@ -5907,7 +5925,7 @@ sub git_shortlog {\n \n sub git_feed {\n \tmy $format = shift || 'atom';\n-\tmy ($have_blame) = gitweb_check_feature('blame');\n+\tmy $have_blame = gitweb_check_feature('blame');\n \n \t# Atom: http://www.atomenabled.org/developers/syndication/\n \t# RSS: http://www.notestips.com/80256B3A007F2692/1/NAMO5P9UPQ\n-- \n1.5.6.5\n"},{"id":"95949","messageId":"cb7bb73a0811160730g31eaf673n528359b8c799b2ef@mail.gmail.com","threadId":"16338","inReplyTo":"1226759165-6894-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCH] gitweb: fixes to gitweb feature check code","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-11-16T15:30:33Z","receivedAt":"2008-11-16T15:30:33Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Forgot the sign-off line\n\nOn Sat, Nov 15, 2008 at 3:26 PM, Giuseppe Bilotta\n wrote:\n> The gitweb_check_feature routine was being used for two different\n> purposes: retrieving the actual feature value (such as the list of\n> snapshot formats or the list of additional actions), and to check if a\n> feature was enabled.\n>\n> For the latter use, since all features return an array, it led to either\n> clumsy code or subtle bugs, with disabled features appearing enabled\n> because (0) evaluates to 1.\n>\n> We fix these bugs, and simplify the code, by separating feature (list)\n> value retrieval (gitweb_get_feature) from boolean feature check (i.e.\n> retrieving the first/only item in the feature value list). Usage of\n> gitweb_check_feature across gitweb is replaced by the appropriate call\n> wherever needed.\n\nSigned-off-by: Giuseppe Bilotta \n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"95973","messageId":"7vwsf31ima.fsf@gitster.siamese.dyndns.org","threadId":"16338","inReplyTo":"1226759165-6894-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCH] gitweb: fixes to gitweb feature check code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-16T21:11:25Z","receivedAt":"2008-11-16T21:11:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta writes:\n\n> The gitweb_check_feature routine was being used for two different\n> purposes: retrieving the actual feature value (such as the list of\n> snapshot formats or the list of additional actions), and to check if a\n> feature was enabled.\n\n> +# check if a given feature is enabled or not, returning the first (and only)\n> +# value of the feature. Comfort code, allowing the use of\n> +# my $bool_feat = gitweb_check_feature('bool_feat');\n> +# or\n> +# gitweb_check_feature('bool_feat') or somecode;\n> +# instead of\n> +# my ($bool_feat) = gitweb_git_feature('bool_feat');\n> +# or\n> +# (gitweb_check_feature('bool_feat'))[0] or somecode;\n> +# respectively\n\nWhat's \"Comfort code\"?\n\nI'd agree that introduction of gitweb_get_feature() may help avoiding\nmistakes at the call sites for Perl illiterates like myself.\n\n> @@ -767,7 +785,7 @@ our $git_dir;\n> $git_dir = \"$projectroot/$project\" if $project;\n> \n> # list of supported snapshot formats\n> -our @snapshot_fmts = gitweb_check_feature('snapshot');\n> +our @snapshot_fmts = gitweb_get_feature('snapshot');\n> @snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);\n\nAnd this may be a good change from that point of view, but...\n\n> @@ -810,7 +828,7 @@ sub href (%) {\n> \t\t}\n> \t}\n> \n> -\tmy ($use_pathinfo) = gitweb_check_feature('pathinfo');\n> +\tmy $use_pathinfo = gitweb_check_feature('pathinfo');\n\n... I do not think changes like these are warranted. They have been using\nthe function _correctly_ by calling it in the list context and I think\nthey will continue to work with your patch.\n"},{"id":"95978","messageId":"cb7bb73a0811161357m28c3110flcc6b3d6b0c0cfef7@mail.gmail.com","threadId":"16338","inReplyTo":"7vwsf31ima.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] gitweb: fixes to gitweb feature check code","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-11-16T21:57:56Z","receivedAt":"2008-11-16T21:57:56Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Sun, Nov 16, 2008 at 10:11 PM, Junio C Hamano wrote:\n> Giuseppe Bilotta writes:\n>\n>> The gitweb_check_feature routine was being used for two different\n>> purposes: retrieving the actual feature value (such as the list of\n>> snapshot formats or the list of additional actions), and to check if a\n>> feature was enabled.\n>\n>> +# check if a given feature is enabled or not, returning the first (and only)\n>> +# value of the feature. Comfort code, allowing the use of\n>> +# my $bool_feat = gitweb_check_feature('bool_feat');\n>> +# or\n>> +# gitweb_check_feature('bool_feat') or somecode;\n>> +# instead of\n>> +# my ($bool_feat) = gitweb_git_feature('bool_feat');\n>> +# or\n>> +# (gitweb_check_feature('bool_feat'))[0] or somecode;\n>> +# respectively\n>\n> What's \"Comfort code\"?\n\nCode that provides comfort? 8-D\n\n> I'd agree that introduction of gitweb_get_feature() may help avoiding\n> mistakes at the call sites for Perl illiterates like myself.\n\n>> - my ($use_pathinfo) = gitweb_check_feature('pathinfo');\n>> + my $use_pathinfo = gitweb_check_feature('pathinfo');\n>\n> ... I do not think changes like these are warranted. They have been using\n> the function _correctly_ by calling it in the list context and I think\n> they will continue to work with your patch.\n\nBecause the returned scalar value gets properly promoted to array?\nMaybe, but I would say that keeping the () is confusing and\ninconsistent. After all, the purpose of the patch is to _eliminate_\n(the need for) such constructs.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"95999","messageId":"200811170202.27893.jnareb@gmail.com","threadId":"16338","inReplyTo":"1226759165-6894-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCH] gitweb: fixes to gitweb feature check code","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-11-17T01:02:26Z","receivedAt":"2008-11-17T01:02:26Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sat, 15 Nov 2008, Giuseppe Bilotta wrote:\n\n> The gitweb_check_feature routine was being used for two different\n> purposes: retrieving the actual feature value (such as the list of\n> snapshot formats or the list of additional actions), and to check if a\n> feature was enabled.\n> \n> For the latter use, since all features return an array, it led to either\n> clumsy code or subtle bugs, with disabled features appearing enabled\n> because (0) evaluates to 1.\n> \n> We fix these bugs, and simplify the code, by separating feature (list)\n> value retrieval (gitweb_get_feature) from boolean feature check (i.e.\n> retrieving the first/only item in the feature value list). Usage of\n> gitweb_check_feature across gitweb is replaced by the appropriate call\n> wherever needed.\n> ---\n\nFirst, you forgot the signoff, but you have addressed that already.\n\n\nSecond, I thought at first that it would be good for the patch to also\nsimplify %feature hash, using \"'default' => 1\" instead of current bit\nconvoluted \"'default' => [1]\", at the cost of bit more code for\ndefensive programming. But now I think that if it is to be done,\nit should be put as separate patch.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"96007","messageId":"cb7bb73a0811162210iadb7511rc3474272c8e60c59@mail.gmail.com","threadId":"16338","inReplyTo":"200811170202.27893.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: fixes to gitweb feature check code","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-11-17T06:10:49Z","receivedAt":"2008-11-17T06:10:49Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Mon, Nov 17, 2008 at 2:02 AM, Jakub Narebski wrote:\n> On Sat, 15 Nov 2008, Giuseppe Bilotta wrote:\n>\n>> The gitweb_check_feature routine was being used for two different\n>> purposes: retrieving the actual feature value (such as the list of\n>> snapshot formats or the list of additional actions), and to check if a\n>> feature was enabled.\n>>\n>> For the latter use, since all features return an array, it led to either\n>> clumsy code or subtle bugs, with disabled features appearing enabled\n>> because (0) evaluates to 1.\n>>\n>> We fix these bugs, and simplify the code, by separating feature (list)\n>> value retrieval (gitweb_get_feature) from boolean feature check (i.e.\n>> retrieving the first/only item in the feature value list). Usage of\n>> gitweb_check_feature across gitweb is replaced by the appropriate call\n>> wherever needed.\n>> ---\n>\n> First, you forgot the signoff, but you have addressed that already.\n>\n>\n> Second, I thought at first that it would be good for the patch to also\n> simplify %feature hash, using \"'default' => 1\" instead of current bit\n> convoluted \"'default' => [1]\", at the cost of bit more code for\n> defensive programming. But now I think that if it is to be done,\n> it should be put as separate patch.\n\nIs this an ACK? 8-D\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"96015","messageId":"200811171028.19807.jnareb@gmail.com","threadId":"16338","inReplyTo":"cb7bb73a0811162210iadb7511rc3474272c8e60c59@mail.gmail.com","subject":"Re: [PATCH] gitweb: fixes to gitweb feature check code","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-11-17T09:28:18Z","receivedAt":"2008-11-17T09:28:18Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Dnia poniedziałek 17. listopada 2008 07:10, Giuseppe Bilotta napisał:\n> On Mon, Nov 17, 2008 at 2:02 AM, Jakub Narebski wrote:\n\n[...]\n> > First, you forgot the signoff, but you have addressed that already.\n> >\n> >\n> > Second, I thought at first that it would be good for the patch to also\n> > simplify %feature hash, using \"'default' => 1\" instead of current bit\n> > convoluted \"'default' => [1]\", at the cost of bit more code for\n> > defensive programming. But now I think that if it is to be done,\n> > it should be put as separate patch.\n> \n> Is this an ACK? 8-D\n\nI'm sorry. Yes, it is.\n\nAcked-by: Jakub Narebski \n\n-- \nJakub Narebski\nPoland\n"},{"id":"96017","messageId":"7vskpqzms5.fsf@gitster.siamese.dyndns.org","threadId":"16338","inReplyTo":"200811171028.19807.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: fixes to gitweb feature check code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-17T10:09:46Z","receivedAt":"2008-11-17T10:09:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> Dnia poniedziałek 17. listopada 2008 07:10, Giuseppe Bilotta napisał:\n>> On Mon, Nov 17, 2008 at 2:02 AM, Jakub Narebski wrote:\n>\n> [...]\n>> > First, you forgot the signoff, but you have addressed that already.\n>> >\n>> >\n>> > Second, I thought at first that it would be good for the patch to also\n>> > simplify %feature hash, using \"'default' => 1\" instead of current bit\n>> > convoluted \"'default' => [1]\", at the cost of bit more code for\n>> > defensive programming. But now I think that if it is to be done,\n>> > it should be put as separate patch.\n>> \n>> Is this an ACK? 8-D\n>\n> I'm sorry. Yes, it is.\n\nAre you sure, even with those unnecessary changes from list context\nassignments to scalar ones?\n"},{"id":"96027","messageId":"200811171148.03649.jnareb@gmail.com","threadId":"16338","inReplyTo":"7vskpqzms5.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] gitweb: fixes to gitweb feature check code","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-11-17T10:48:03Z","receivedAt":"2008-11-17T10:48:03Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, 17 Nov 2008, Junio C Hamano wrote:\n> Jakub Narebski writes:\n>> Dnia poniedziałek 17. listopada 2008 07:10, Giuseppe Bilotta napisał:\n>>> On Mon, Nov 17, 2008 at 2:02 AM, Jakub Narebski wrote:\n>>\n>> [...]\n>>>> First, you forgot the signoff, but you have addressed that already.\n>>>>\n>>>>\n>>>> Second, I thought at first that it would be good for the patch to also\n>>>> simplify %feature hash, using \"'default' => 1\" instead of current bit\n>>>> convoluted \"'default' => [1]\", at the cost of bit more code for\n>>>> defensive programming. But now I think that if it is to be done,\n>>>> it should be put as separate patch.\n>>> \n>>> Is this an ACK? 8-D\n>>\n>> I'm sorry. Yes, it is.\n> \n> Are you sure, even with those unnecessary changes from list context\n> assignments to scalar ones?\n\nWell, on one hand this change is not _necessary_, as it would work\nwithout it. On the other hand it feels like cleanup (like e.g. using\ntabs to indent but spaces to align, or word-wrapping too long lines).\n\nSo I think I'll pass the ball to you... your call ;-)\n-- \nJakub Narebski\nPoland\n"}]}