{"thread":{"id":"16516","subject":"[PATCHv2 0/2] fixes to gitweb feature check code","startedAt":"2008-11-28T20:39:51Z","lastAt":"2008-12-03T02:18:12Z","messageCount":26,"participants":["Giuseppe Bilotta","Jakub Narebski","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"96692","messageId":"1227904793-1821-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"16516","inReplyTo":null,"subject":"[PATCHv2 0/2] fixes to gitweb feature check code","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-11-28T20:39:51Z","receivedAt":"2008-11-28T20:39:51Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"This is v2 of the gitweb feature check fix patch, which has now been\nsplit into into a code patch and cleanup patch.\n\nThe first patch introduces git_get_feature() to clearly distinguish the\nfeature retrieval from the boolean feature check (which is kept at\ngit_check_feature()). The new function is used where appropriate.\n\nThe second patch cleans up use of git_check_feature(): since the\nfunction now returns a boolean instead of an array, the often-used\nconstruct\n my ($somevar) = git_check_feature('somefeat');\nalthough still valid, becomes a rather clumsy stylistic choice, as it\nintroduces an unnecessary ambiguity. Make it clear that we're now\ndealing with scalars by using scalar assignment.\n\nGiuseppe Bilotta (2):\n gitweb: fixes to gitweb feature check code\n gitweb: clean up git_check_feature() calls\n\n gitweb/gitweb.perl | 52 +++++++++++++++++++++++++++++++++++-----------------\n 1 files changed, 35 insertions(+), 17 deletions(-)\n"},{"id":"96693","messageId":"1227904793-1821-2-git-send-email-giuseppe.bilotta@gmail.com","threadId":"16516","inReplyTo":"1227904793-1821-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv2 1/2] gitweb: fixes to gitweb feature check code","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-11-28T20:39:52Z","receivedAt":"2008-11-28T20:39:52Z","isPatch":false,"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 is replaced by gitweb_get_feature where\nappropriate.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 26 ++++++++++++++++++++++----\n 1 files changed, 22 insertions(+), 4 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 933e137..128d7ad 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@@ -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-- \n1.5.6.5\n"},{"id":"96694","messageId":"1227904793-1821-3-git-send-email-giuseppe.bilotta@gmail.com","threadId":"16516","inReplyTo":"1227904793-1821-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv2 2/2] gitweb: clean up git_check_feature() calls","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-11-28T20:39:53Z","receivedAt":"2008-11-28T20:39:53Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Since git_check_feature now returns a scalar, the parentheses around the\nvariables initialized from git_check_feature() are not needed.\n\nWe remove them for stylistic consistency and to prevent unnecessary\nambiguity in the code.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 26 +++++++++++++-------------\n 1 files changed, 13 insertions(+), 13 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 128d7ad..b0d00ea 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -828,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@@ -2119,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@@ -2965,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@@ -2979,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@@ -3472,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@@ -3986,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@@ -4446,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@@ -4475,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@@ -4765,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@@ -4858,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@@ -5856,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@@ -5873,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@@ -5925,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":"96713","messageId":"200811291148.15182.jnareb@gmail.com","threadId":"16516","inReplyTo":"1227904793-1821-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv2 0/2] fixes to gitweb feature check code","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-11-29T10:48:13Z","receivedAt":"2008-11-29T10:48:13Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Dnia piątek 28. listopada 2008 21:39, Giuseppe Bilotta napisał:\n> This is v2 of the gitweb feature check fix patch, which has now been\n> split into into a code patch and cleanup patch.\n> \n> The first patch introduces git_get_feature() to clearly distinguish the\n> feature retrieval from the boolean feature check (which is kept at\n> git_check_feature()). The new function is used where appropriate.\n> \n> The second patch cleans up use of git_check_feature(): since the\n> function now returns a boolean instead of an array, the often-used\n> construct\n> my ($somevar) = git_check_feature('somefeat');\n> although still valid, becomes a rather clumsy stylistic choice, as it\n> introduces an unnecessary ambiguity. Make it clear that we're now\n> dealing with scalars by using scalar assignment.\n\nIt is gitweb_get_feature() (for retrieval) and gitweb_check_feature()\n(for checking), not git_get_feature() and git_check_feature().\n\nI really like this series; making coding easier by removing need to\nuse special construct, and making it easier to avoid mistakes is always\na good thing.\n\n> Giuseppe Bilotta (2):\n> gitweb: fixes to gitweb feature check code\n> gitweb: clean up git_check_feature() calls\n> \n> gitweb/gitweb.perl | 52 +++++++++++++++++++++++++++++++++++-----------------\n> 1 files changed, 35 insertions(+), 17 deletions(-)\n\nI also like the split, as it separates improvement (much easier to use\nand less error prone gitweb_check_feature()) from style cleanup.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"96714","messageId":"cb7bb73a0811290313v735ba182hc13a9ae6f39b7e27@mail.gmail.com","threadId":"16516","inReplyTo":"200811291148.15182.jnareb@gmail.com","subject":"Re: [PATCHv2 0/2] fixes to gitweb feature check code","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-11-29T11:13:26Z","receivedAt":"2008-11-29T11:13:26Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2008/11/29 Jakub Narebski :\n> Dnia piątek 28. listopada 2008 21:39, Giuseppe Bilotta napisał:\n\n>> The first patch introduces git_get_feature() to clearly distinguish the\n>> feature retrieval from the boolean feature check (which is kept at\n>> git_check_feature()). The new function is used where appropriate.\n\n> It is gitweb_get_feature() (for retrieval) and gitweb_check_feature()\n> (for checking), not git_get_feature() and git_check_feature().\n\nAhem. Right. I'll resend the second patch with a fixed subject and\ncommit message 8-P\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"96715","messageId":"1227957301-7417-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"16516","inReplyTo":"1227904793-1821-3-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv2 2/2] gitweb: clean up gitweb_check_feature() calls","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-11-29T11:15:01Z","receivedAt":"2008-11-29T11:15:01Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Since git_check_feature now returns a scalar, the parentheses around the\nvariables initialized from gitweb_check_feature() are not needed.\n\nWe remove them for stylistic consistency and to prevent unnecessary\nambiguity in the code.\n\nSigned-off-by: Giuseppe Bilotta \n---\n\nFixed the commit message, use this instead of the previous one.\n\n gitweb/gitweb.perl | 26 +++++++++++++-------------\n 1 files changed, 13 insertions(+), 13 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 128d7ad..b0d00ea 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -828,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@@ -2119,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@@ -2965,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@@ -2979,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@@ -3472,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@@ -3986,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@@ -4446,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@@ -4475,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@@ -4765,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@@ -4858,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@@ -5856,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@@ -5873,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@@ -5925,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":"96716","messageId":"200811291218.22775.jnareb@gmail.com","threadId":"16516","inReplyTo":"1227957301-7417-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv2 2/2] gitweb: clean up gitweb_check_feature() calls","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-11-29T11:18:22Z","receivedAt":"2008-11-29T11:18:22Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Giuseppe Bilotta wrote:\n\n> Since git_check_feature now returns a scalar, the parentheses around the\n ^^^\n \\---- missed one.\n\n> variables initialized from gitweb_check_feature() are not needed.\n> \n> We remove them for stylistic consistency and to prevent unnecessary\n> ambiguity in the code.\n> \n> Signed-off-by: Giuseppe Bilotta \n> ---\n> \n> Fixed the commit message, use this instead of the previous one.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"96717","messageId":"1227959616-8056-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"16516","inReplyTo":"1227904793-1821-3-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv2ter 2/2] gitweb: clean up gitweb_check_feature() calls","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-11-29T11:53:36Z","receivedAt":"2008-11-29T11:53:36Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Since gitweb_check_feature now returns a scalar, the parentheses around\nthe variables initialized from gitweb_check_feature() are not needed.\n\nWe remove them for stylistic consistency and to prevent unnecessary\nambiguity in the code.\n\nSigned-off-by: Giuseppe Bilotta \n---\n\nThird attempt, hopefully I got the commit message right, finally 8-P\n\n gitweb/gitweb.perl | 26 +++++++++++++-------------\n 1 files changed, 13 insertions(+), 13 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 128d7ad..b0d00ea 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -828,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@@ -2119,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@@ -2965,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@@ -2979,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@@ -3472,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@@ -3986,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@@ -4446,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@@ -4475,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@@ -4765,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@@ -4858,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@@ -5856,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@@ -5873,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@@ -5925,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":"96735","messageId":"7vej0u1d2c.fsf@gitster.siamese.dyndns.org","threadId":"16516","inReplyTo":"1227959616-8056-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv2ter 2/2] gitweb: clean up gitweb_check_feature() calls","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-29T20:39:55Z","receivedAt":"2008-11-29T20:39:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta writes:\n\n> Since gitweb_check_feature now returns a scalar, the parentheses around\n> the variables initialized from gitweb_check_feature() are not needed.\n\nHere is what I think the commit message of your [1/2] should have read:\n\n Subject: [PATCH] gitweb: make gitweb_check_feature a boolean wrapper\n\n The gitweb_check_feature() function retrieves the configuration\n parameters for the feature (such as the list of snapshot formats or\n the list of additional actions), but it is very often used to see if\n feature is enabled (which is returned as the first element in the\n list).\n\n Because accepting the returned list in a scalar context by mistake\n yields the number of elements in the array, which is non-zero in all\n cases, such a mistake would result in a bug for the latter use, with\n disabled features appearing enabled. All existing callers that call\n the function for this purpose assign the return value in the list\n context to retrieve the first element, but this is a bug waiting to\n happen for future callers that are not careful.\n\n This changes gitweb_check_feature() to a wrapper that can be called\n safely in the scalar context to see if a feature is enabled to reduce\n the risk of future bugs. A new function gitweb_get_feature() is\n introduced for the callers that want the list of configuration\n parameters. All existing callers of gitweb_check_feature() that want\n the configuration parameters are adjusted to call gitweb_get_feature().\n\nNotice the difference in the tone from your [1/2]. The change is not\nabout fixing (your proposed commit log message read \"gitweb:fixes to\ngitweb feature check code\") as nothing is broken. It is purely about\nfutureproofing and I think we should mark it as such.\n\nThe splitting of two patches is not strictly necessary, but if you wanted\nto, I would do so slightly differently:\n\n * Make [1/2] purely the rename of s/check/get/, saying \"the function is\n not just checking if it is enabled, but it is getting the configuration\n parameter list. I'll introduce a new API to ask if it is enabled,\n which will be called 'check', hence this rename\".\n\n Obviously, after applying this patch, there should be no caller of\n \"check\".\n\n * Make [2/2] introduce the \"check\" wrapper, and change the existing\n callers of \"get\" that want boolean to call \"check\" (and in the scalar\n context, as it does not matter anymore). You can steal most of the\n above rewrite as your commit message for this (except for the last two\n sentences).\n\nAlso I would rewrite the mysterious \"Comfort code\" comment like this:\n\n # A wrapper to check if a given feature is enabled.\n # With this, you can say\n #\n # my $bool_feat = gitweb_check_feature('bool_feat');\n # gitweb_check_feature('bool_feat') or somecode;\n #\n # instead of\n #\n # my ($bool_feat) = gitweb_get_feature('bool_feat');\n # (gitweb_get_feature('bool_feat'))[0] or somecode;\n\n(you also had a few typos in these four example calls).\n"},{"id":"96736","messageId":"7vzljiz1qn.fsf@gitster.siamese.dyndns.org","threadId":"16516","inReplyTo":"7vej0u1d2c.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCHv2ter 2/2] gitweb: clean up gitweb_check_feature() calls","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-29T21:00:32Z","receivedAt":"2008-11-29T21:00:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> ... The change is not\n> about fixing (your proposed commit log message read \"gitweb:fixes to\n> gitweb feature check code\") as nothing is broken. It is purely about\n> futureproofing and I think we should mark it as such.\n\nActually, there is a handful clueless/careless callers. Before applying\nany of the check => get patch, let's do this as a fix.\n\n-- >8 --\nSubject: [PATCH] gitweb: fix 'ctags' feature check and others\n\ngitweb_check_feature() function is to retrieve the configuration parameter\nlist and calling it in a scalar context does not give its first element\nthat tells if the feature is enabled. This fixes all the existing callers\nto call the function correctly in list context.\n\nSigned-off-by: Junio C Hamano \n---\n gitweb/gitweb.perl | 14 +++++++-------\n 1 files changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 933e137..400f5c8 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -3914,7 +3914,7 @@ sub fill_project_list_info {\n \tmy ($projlist, $check_forks) = @_;\n \tmy @projects;\n \n-\tmy $show_ctags = gitweb_check_feature('ctags');\n+\tmy ($show_ctags) = gitweb_check_feature('ctags');\n PROJECT:\n \tforeach my $pr (@$projlist) {\n \t\tmy (@activity) = git_get_last_activity($pr->{'path'});\n@@ -3988,7 +3988,7 @@ sub git_project_list_body {\n \t\t@projects = sort {$a->{$oi->{'key'}} <=> $b->{$oi->{'key'}}} @projects;\n \t}\n \n-\tmy $show_ctags = gitweb_check_feature('ctags');\n+\tmy ($show_ctags) = gitweb_check_feature('ctags');\n \tif ($show_ctags) {\n \t\tmy %ctags;\n \t\tforeach my $p (@projects) {\n@@ -4457,7 +4457,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@@ -4559,7 +4559,7 @@ sub git_blame {\n \tmy $fd;\n \tmy $ftype;\n \n-\tgitweb_check_feature('blame')\n+\tgitweb_check_feature('blame')[0]\n \t or die_error(403, \"Blame view not allowed\");\n \n \tdie_error(400, \"No file name given\") unless $file_name;\n@@ -5610,7 +5610,7 @@ sub git_history {\n }\n \n sub git_search {\n-\tgitweb_check_feature('search') or die_error(403, \"Search is disabled\");\n+\tgitweb_check_feature('search')[0] or die_error(403, \"Search is disabled\");\n \tif (!defined $searchtext) {\n \t\tdie_error(400, \"Text field is empty\");\n \t}\n@@ -5629,11 +5629,11 @@ sub git_search {\n \tif ($searchtype eq 'pickaxe') {\n \t\t# pickaxe may take all resources of your box and run for several minutes\n \t\t# with every query - so decide by yourself how public you make this feature\n-\t\tgitweb_check_feature('pickaxe')\n+\t\tgitweb_check_feature('pickaxe')[0]\n \t\t or die_error(403, \"Pickaxe is disabled\");\n \t}\n \tif ($searchtype eq 'grep') {\n-\t\tgitweb_check_feature('grep')\n+\t\tgitweb_check_feature('grep')[0]\n \t\t or die_error(403, \"Grep is disabled\");\n \t}\n \n-- \n1.6.0.4.850.g6bd829\n"},{"id":"96737","messageId":"7vskpaz16x.fsf_-_@gitster.siamese.dyndns.org","threadId":"16516","inReplyTo":"7vzljiz1qn.fsf@gitster.siamese.dyndns.org","subject":"[PATCH 2/3] gitweb: rename gitweb_check_feature to gitweb_get_feature","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-29T21:12:22Z","receivedAt":"2008-11-29T21:12:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The function is about retrieving the configuration parameter list for the\nfeature. A more robust way to check if a feature is enabled is introduced\nin the next patch, and the function will be called gitweb_check_feature.\n\nSigned-off-by: Junio C Hamano \n---\n * The earlier \"fix ctags and friends\" is the 1/3; this shows how I would\n have liked to see your patches as a reviewable series. The end result\n is almost identical.\n\n gitweb/gitweb.perl | 46 +++++++++++++++++++++++-----------------------\n 1 files changed, 23 insertions(+), 23 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 400f5c8..7ab16cd 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -190,7 +190,7 @@ 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 check if 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 +329,7 @@ our %feature = (\n \t\t'default' => [0]},\n );\n \n-sub gitweb_check_feature {\n+sub gitweb_get_feature {\n \tmy ($name) = @_;\n \treturn unless exists $feature{$name};\n \tmy ($sub, $override, @defaults) = (\n@@ -767,7 +767,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 +810,7 @@ sub href (%) {\n \t\t}\n \t}\n \n-\tmy ($use_pathinfo) = gitweb_check_feature('pathinfo');\n+\tmy ($use_pathinfo) = gitweb_get_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 +2101,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_get_feature('forks');\n \n \tif (-d $projects_list) {\n \t\t# search in directory\n@@ -2947,7 +2947,7 @@ EOF\n \t}\n \tprint \"
\\n\";\n \n-\tmy ($have_search) = gitweb_check_feature('search');\n+\tmy ($have_search) = gitweb_get_feature('search');\n \tif (defined $project && $have_search) {\n \t\tif (!defined $searchtext) {\n \t\t\t$searchtext = \"\";\n@@ -2961,7 +2961,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_get_feature('pathinfo');\n \t\tif ($use_pathinfo) {\n \t\t\t$action .= \"/\".esc_url($project);\n \t\t}\n@@ -3084,7 +3084,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 +3454,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_get_feature('blame');\n \tprint \"
\\n\";\n \tif ($#{$difftree} > 10) {\n \t\tprint(($#{$difftree} + 1) . \" files changed:\\n\");\n@@ -3914,7 +3914,7 @@ sub fill_project_list_info {\n \tmy ($projlist, $check_forks) = @_;\n \tmy @projects;\n \n-\tmy ($show_ctags) = gitweb_check_feature('ctags');\n+\tmy ($show_ctags) = gitweb_get_feature('ctags');\n PROJECT:\n \tforeach my $pr (@$projlist) {\n \t\tmy (@activity) = git_get_last_activity($pr->{'path'});\n@@ -3968,7 +3968,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_get_feature('forks');\n \tmy @projects = fill_project_list_info($projlist, $check_forks);\n \n \t$order ||= $default_projects_order;\n@@ -3988,7 +3988,7 @@ sub git_project_list_body {\n \t\t@projects = sort {$a->{$oi->{'key'}} <=> $b->{$oi->{'key'}}} @projects;\n \t}\n \n-\tmy ($show_ctags) = gitweb_check_feature('ctags');\n+\tmy ($show_ctags) = gitweb_get_feature('ctags');\n \tif ($show_ctags) {\n \t\tmy %ctags;\n \t\tforeach my $p (@projects) {\n@@ -4428,7 +4428,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_get_feature('forks');\n \n \tif ($check_forks) {\n \t\t@forklist = git_get_projects_list($project);\n@@ -4457,7 +4457,7 @@ sub git_summary {\n \t}\n \n \t# Tag cloud\n-\tmy ($show_ctags) = gitweb_check_feature('ctags');\n+\tmy ($show_ctags) = gitweb_get_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@@ -4559,7 +4559,7 @@ sub git_blame {\n \tmy $fd;\n \tmy $ftype;\n \n-\tgitweb_check_feature('blame')[0]\n+\tgitweb_get_feature('blame')[0]\n \t or die_error(403, \"Blame view not allowed\");\n \n \tdie_error(400, \"No file name given\") unless $file_name;\n@@ -4747,7 +4747,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_get_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 +4840,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_get_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@@ -5610,7 +5610,7 @@ sub git_history {\n }\n \n sub git_search {\n-\tgitweb_check_feature('search')[0] or die_error(403, \"Search is disabled\");\n+\tgitweb_get_feature('search')[0] or die_error(403, \"Search is disabled\");\n \tif (!defined $searchtext) {\n \t\tdie_error(400, \"Text field is empty\");\n \t}\n@@ -5629,11 +5629,11 @@ sub git_search {\n \tif ($searchtype eq 'pickaxe') {\n \t\t# pickaxe may take all resources of your box and run for several minutes\n \t\t# with every query - so decide by yourself how public you make this feature\n-\t\tgitweb_check_feature('pickaxe')[0]\n+\t\tgitweb_get_feature('pickaxe')[0]\n \t\t or die_error(403, \"Pickaxe is disabled\");\n \t}\n \tif ($searchtype eq 'grep') {\n-\t\tgitweb_check_feature('grep')[0]\n+\t\tgitweb_get_feature('grep')[0]\n \t\t or die_error(403, \"Grep is disabled\");\n \t}\n \n@@ -5838,7 +5838,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_get_feature('grep');\n \tif ($have_grep) {\n \t\tprint <grep\n@@ -5855,7 +5855,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_get_feature('pickaxe');\n \tif ($have_pickaxe) {\n \t\tprint <pickaxe\n@@ -5907,7 +5907,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_get_feature('blame');\n \n \t# Atom: http://www.atomenabled.org/developers/syndication/\n \t# RSS: http://www.notestips.com/80256B3A007F2692/1/NAMO5P9UPQ\n-- \n1.6.0.4.850.g6bd829\n"},{"id":"96738","messageId":"7vmyfiz0zf.fsf_-_@gitster.siamese.dyndns.org","threadId":"16516","inReplyTo":"7vzljiz1qn.fsf@gitster.siamese.dyndns.org","subject":"[PATCH 3/3] gitweb: make gitweb_check_feature a boolean wrapper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-29T21:16:52Z","receivedAt":"2008-11-29T21:16:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The gitweb_get_feature() function retrieves the configuration parameters\nfor the feature (such as the list of snapshot formats or the list of\nadditional actions), but it is very often used to see if feature is\nenabled (which is returned as the first element in the list).\n\nBecause accepting the returned list in a scalar context by mistake yields\nthe number of elements in the array, which is non-zero in all cases, such\na mistake would result in a bug for the latter use, with disabled features\nappearing enabled. All existing callers that call the function for this\npurpose assign the return value in the list context to retrieve the first\nelement, but that is only because we fixed careless callers recently.\n\nThis add gitweb_check_feature() as a wrapper to gitweb_get_feature() that\ncan be called safely in the scalar context to see if a feature is enabled\nto reduce the risk of future bugs.\n\nSigned-off-by: Junio C Hamano \n---\n * Again, this is to demonstrate how I would have liked to see your\n patches as a reviewable series. Notice how this naturally justifies\n the dropping of parentheses in many lines that begin with \"my\", and\n makes it easier to review? You can review the patch easily by knowing\n that callers that have s/get/check/ are now safe to use scalar context.\n\n gitweb/gitweb.perl | 56 +++++++++++++++++++++++++++++++++------------------\n 1 files changed, 36 insertions(+), 20 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 7ab16cd..acc4cfd 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -190,7 +190,7 @@ 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_get_feature() to check if is enabled\n+\t# use gitweb_check_feature() to check if 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@@ -344,6 +344,22 @@ sub gitweb_get_feature {\n \treturn $sub->(@defaults);\n }\n \n+# A wrapper to check if a given feature is enabled.\n+# With this, you can say\n+#\n+# my $bool_feat = gitweb_check_feature('bool_feat');\n+# gitweb_check_feature('bool_feat') or somecode;\n+#\n+# instead of\n+#\n+# my ($bool_feat) = gitweb_get_feature('bool_feat');\n+# (gitweb_get_feature('bool_feat'))[0] or somecode;\n+#\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@@ -810,7 +826,7 @@ sub href (%) {\n \t\t}\n \t}\n \n-\tmy ($use_pathinfo) = gitweb_get_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 +2117,7 @@ sub git_get_projects_list {\n \t$filter ||= '';\n \t$filter =~ s/\\.git$//;\n \n-\tmy ($check_forks) = gitweb_get_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 +2963,7 @@ EOF\n \t}\n \tprint \"
\\n\";\n \n-\tmy ($have_search) = gitweb_get_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 +2977,7 @@ EOF\n \t\t\t$search_hash = \"HEAD\";\n \t\t}\n \t\tmy $action = $my_uri;\n-\t\tmy ($use_pathinfo) = gitweb_get_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@@ -3454,7 +3470,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_get_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@@ -3914,7 +3930,7 @@ sub fill_project_list_info {\n \tmy ($projlist, $check_forks) = @_;\n \tmy @projects;\n \n-\tmy ($show_ctags) = gitweb_get_feature('ctags');\n+\tmy $show_ctags = gitweb_check_feature('ctags');\n PROJECT:\n \tforeach my $pr (@$projlist) {\n \t\tmy (@activity) = git_get_last_activity($pr->{'path'});\n@@ -3968,7 +3984,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_get_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@@ -3988,7 +4004,7 @@ sub git_project_list_body {\n \t\t@projects = sort {$a->{$oi->{'key'}} <=> $b->{$oi->{'key'}}} @projects;\n \t}\n \n-\tmy ($show_ctags) = gitweb_get_feature('ctags');\n+\tmy $show_ctags = gitweb_check_feature('ctags');\n \tif ($show_ctags) {\n \t\tmy %ctags;\n \t\tforeach my $p (@projects) {\n@@ -4428,7 +4444,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_get_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 +4473,7 @@ sub git_summary {\n \t}\n \n \t# Tag cloud\n-\tmy ($show_ctags) = gitweb_get_feature('ctags');\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@@ -4559,7 +4575,7 @@ sub git_blame {\n \tmy $fd;\n \tmy $ftype;\n \n-\tgitweb_get_feature('blame')[0]\n+\tgitweb_check_feature('blame')\n \t or die_error(403, \"Blame view not allowed\");\n \n \tdie_error(400, \"No file name given\") unless $file_name;\n@@ -4747,7 +4763,7 @@ sub git_blob {\n \t\t$expires = \"+1d\";\n \t}\n \n-\tmy ($have_blame) = gitweb_get_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 +4856,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_get_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@@ -5610,7 +5626,7 @@ sub git_history {\n }\n \n sub git_search {\n-\tgitweb_get_feature('search')[0] or die_error(403, \"Search is disabled\");\n+\tgitweb_check_feature('search') or die_error(403, \"Search is disabled\");\n \tif (!defined $searchtext) {\n \t\tdie_error(400, \"Text field is empty\");\n \t}\n@@ -5629,11 +5645,11 @@ sub git_search {\n \tif ($searchtype eq 'pickaxe') {\n \t\t# pickaxe may take all resources of your box and run for several minutes\n \t\t# with every query - so decide by yourself how public you make this feature\n-\t\tgitweb_get_feature('pickaxe')[0]\n+\t\tgitweb_check_feature('pickaxe')\n \t\t or die_error(403, \"Pickaxe is disabled\");\n \t}\n \tif ($searchtype eq 'grep') {\n-\t\tgitweb_get_feature('grep')[0]\n+\t\tgitweb_check_feature('grep')\n \t\t or die_error(403, \"Grep is disabled\");\n \t}\n \n@@ -5838,7 +5854,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_get_feature('grep');\n+\tmy $have_grep = gitweb_check_feature('grep');\n \tif ($have_grep) {\n \t\tprint <grep\n@@ -5855,7 +5871,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_get_feature('pickaxe');\n+\tmy $have_pickaxe = gitweb_check_feature('pickaxe');\n \tif ($have_pickaxe) {\n \t\tprint <pickaxe\n@@ -5907,7 +5923,7 @@ sub git_shortlog {\n \n sub git_feed {\n \tmy $format = shift || 'atom';\n-\tmy ($have_blame) = gitweb_get_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.6.0.4.850.g6bd829\n"},{"id":"96739","messageId":"cb7bb73a0811291416h4c227411u61bfe7033f0d2bae@mail.gmail.com","threadId":"16516","inReplyTo":"7vzljiz1qn.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCHv2ter 2/2] gitweb: clean up gitweb_check_feature() calls","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-11-29T22:16:29Z","receivedAt":"2008-11-29T22:16:29Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Sat, Nov 29, 2008 at 10:00 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> ... The change is not\n>> about fixing (your proposed commit log message read \"gitweb:fixes to\n>> gitweb feature check code\") as nothing is broken. It is purely about\n>> futureproofing and I think we should mark it as such.\n>\n> Actually, there is a handful clueless/careless callers. Before applying\n> any of the check => get patch, let's do this as a fix.\n\nAnd this is precisely the reason why the first time I sent the patch I\ndid the restyling in the same go: by not touching the\nclueless/careless callers and instead bringing gitweb_check_feature to\nact in scalar context, it would automatically fix those broken usages,\nand it made sense to convert all array-to-get-a-scalar assignmets as\nwell.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"96740","messageId":"7viqq6yxqu.fsf@gitster.siamese.dyndns.org","threadId":"16516","inReplyTo":"cb7bb73a0811291416h4c227411u61bfe7033f0d2bae@mail.gmail.com","subject":"Re: [PATCHv2ter 2/2] gitweb: clean up gitweb_check_feature() calls","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-29T22:26:49Z","receivedAt":"2008-11-29T22:26:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Giuseppe Bilotta\" writes:\n\n> And this is precisely the reason why the first time I sent the patch I\n> did the restyling in the same go: by not touching the\n> clueless/careless callers and instead bringing gitweb_check_feature to\n> act in scalar context, it would automatically fix those broken usages,\n\n... which is very bad for reviewability purposes.\n\nBy explicitly fixing them before doing the \"this will sweep all the\npotential bugs under the rug\", you can demonstrate a lot more clearly why\nthese changes are necessary.\n"},{"id":"96741","messageId":"cb7bb73a0811291427n7922ab1fq50caedafe7163f9e@mail.gmail.com","threadId":"16516","inReplyTo":"7vmyfiz0zf.fsf_-_@gitster.siamese.dyndns.org","subject":"Re: [PATCH 3/3] gitweb: make gitweb_check_feature a boolean wrapper","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-11-29T22:27:40Z","receivedAt":"2008-11-29T22:27:40Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Sat, Nov 29, 2008 at 10:16 PM, Junio C Hamano wrote:\n> * Again, this is to demonstrate how I would have liked to see your\n> patches as a reviewable series. Notice how this naturally justifies\n> the dropping of parentheses in many lines that begin with \"my\", and\n> makes it easier to review? You can review the patch easily by knowing\n> that callers that have s/get/check/ are now safe to use scalar context.\n\nYes, I get the point: do less things under the hood. I also still\nthink that three patches with code going ping-pong are way too much,\nthough 8-P\n\nFor what it's worth, you get my Ack: to your patchset 8-)\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"96742","messageId":"200811292337.00380.jnareb@gmail.com","threadId":"16516","inReplyTo":"7viqq6yxqu.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCHv2ter 2/2] gitweb: clean up gitweb_check_feature() calls","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-11-29T22:36:58Z","receivedAt":"2008-11-29T22:36:58Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n> \"Giuseppe Bilotta\" writes:\n> \n> > And this is precisely the reason why the first time I sent the patch I\n> > did the restyling in the same go: by not touching the\n> > clueless/careless callers and instead bringing gitweb_check_feature to\n> > act in scalar context, it would automatically fix those broken usages,\n> \n> ... which is very bad for reviewability purposes.\n> \n> By explicitly fixing them before doing the \"this will sweep all the\n> potential bugs under the rug\", you can demonstrate a lot more clearly why\n> these changes are necessary.\n\nWell, I think now that it would be best to split this series into\n_two_ patches: first Junio's patch fixing (!) gitweb_check_feature()\ncalls, second original v1 Guiseppe's renaming gitweb_check_feature()\nto gitweb_get_feature(), and adding gitweb_check_feature(), and using\ngitweb_get_feature() only where needed; optionally fixing \"style\".\n\nPure rename IMVHO doesn't look that nice...\n-- \nJakub Narebski\nPoland\n"},{"id":"96743","messageId":"200811292338.28156.jnareb@gmail.com","threadId":"16516","inReplyTo":"200811292337.00380.jnareb@gmail.com","subject":"Re: [PATCHv2ter 2/2] gitweb: clean up gitweb_check_feature() calls","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-11-29T22:38:27Z","receivedAt":"2008-11-29T22:38:27Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Jakub Narebski wrote:\n\n> \n> Well, I think now that it would be best to split this series into\n> _two_ patches: first Junio's patch fixing (!) gitweb_check_feature()\n> calls, second original v1 Guiseppe's renaming gitweb_check_feature()\n> to gitweb_get_feature(), and adding gitweb_check_feature(), and using\n> gitweb_get_feature() only where needed; optionally fixing \"style\".\n\nIt means: first fixup patch, then futureproof patch.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"96748","messageId":"7vej0uysbs.fsf@gitster.siamese.dyndns.org","threadId":"16516","inReplyTo":"cb7bb73a0811291427n7922ab1fq50caedafe7163f9e@mail.gmail.com","subject":"Re: [PATCH 3/3] gitweb: make gitweb_check_feature a boolean wrapper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-30T00:23:51Z","receivedAt":"2008-11-30T00:23:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Giuseppe Bilotta\" writes:\n\n> On Sat, Nov 29, 2008 at 10:16 PM, Junio C Hamano wrote:\n>> * Again, this is to demonstrate how I would have liked to see your\n>> patches as a reviewable series. Notice how this naturally justifies\n>> the dropping of parentheses in many lines that begin with \"my\", and\n>> makes it easier to review? You can review the patch easily by knowing\n>> that callers that have s/get/check/ are now safe to use scalar context.\n>\n> Yes, I get the point: do less things under the hood. I also still\n> think that three patches with code going ping-pong are way too much,\n> though 8-P\n>\n> For what it's worth, you get my Ack: to your patchset 8-)\n\nWell, I am not interested in taking credits for this series. I am very\nmuch interested in reducing my future workload by showing people how an\neasily reviewable series should look like.\n\nSo if you still think it is \"way too much\", I did not succeed what I tried\nto do X-<.\n"},{"id":"96750","messageId":"200811300131.16787.jnareb@gmail.com","threadId":"16516","inReplyTo":"1227904793-1821-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv2 1/2] gitweb: fixes to gitweb feature check code","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-11-30T00:31:12Z","receivedAt":"2008-11-30T00:31:12Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 28 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 is replaced by gitweb_get_feature where\n> appropriate.\n> \n> Signed-off-by: Giuseppe Bilotta \n\nI like the idea of futureproofing gitweb code to avoid further bugs\nwith checking if feature is enabled. So from me this patch gets Ack.\n\nI have thought at first that perhaps this patch could also remove\nrequirement that 'default' field is array (reference), replacing a bit\ncumbersome\n $feature{'blame'}{'default'} = [1];\nwith simpler\n $feature{'blame'}{'default'} = 1;\n\nBut now I think that is separate issue, and if we think it is worth\nhaving (in spite of its disadvantages in form of more complicated code\ndealing among others with legacy configuration), it should be put in\nseparate patch.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"96754","messageId":"cb7bb73a0811291731g7f8770f7p89e924c00d2ab004@mail.gmail.com","threadId":"16516","inReplyTo":"7vej0uysbs.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 3/3] gitweb: make gitweb_check_feature a boolean wrapper","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-11-30T01:31:12Z","receivedAt":"2008-11-30T01:31:12Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Sun, Nov 30, 2008 at 1:23 AM, Junio C Hamano wrote:\n> \"Giuseppe Bilotta\" writes:\n>\n>> On Sat, Nov 29, 2008 at 10:16 PM, Junio C Hamano wrote:\n>>> * Again, this is to demonstrate how I would have liked to see your\n>>> patches as a reviewable series. Notice how this naturally justifies\n>>> the dropping of parentheses in many lines that begin with \"my\", and\n>>> makes it easier to review? You can review the patch easily by knowing\n>>> that callers that have s/get/check/ are now safe to use scalar context.\n>>\n>> Yes, I get the point: do less things under the hood. I also still\n>> think that three patches with code going ping-pong are way too much,\n>> though 8-P\n>>\n>> For what it's worth, you get my Ack: to your patchset 8-)\n>\n> Well, I am not interested in taking credits for this series. I am very\n> much interested in reducing my future workload by showing people how an\n> easily reviewable series should look like.\n>\n> So if you still think it is \"way too much\", I did not succeed what I tried\n> to do X-<.\n\nAt least as far as this patch goes, from my point of view, the\n'clueless/careless' usages of gitweb_check_feature are the\nconceptually (although not code-wise) correct ones, so they shouldn't\nbe the ones touched by the patch, considering that in addition to the\nfutureproofing, (implicit) fixing those usages is one of the main\npoints of the patch itself.\n\nOTOH, I _do_ get the point about ease of review. But for example (and\nfor future reference for myself): would you say that it would have\nbeen enough to have a cleaner commit message explicitly mentioning the\nfact that formerly incorrect usages of the gitweb_check_feature() were\nleft intact because they were now correct?\n\nSomething like this http://git.oblomov.eu/git/patch/gitweb/check-feature-v3\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"96755","messageId":"1228008844-12506-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"16516","inReplyTo":"cb7bb73a0811291731g7f8770f7p89e924c00d2ab004@mail.gmail.com","subject":"[PATCH] gitweb: fixes to gitweb feature check code","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-11-30T01:34:04Z","receivedAt":"2008-11-30T01:34:04Z","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 checking if a\nfeature was enabled.\n\nThis led to subtle bugs in feature checking code: gitweb_check_feature\nwould return (0) for disabled features, so its use in scalar context\nwould return true instead of false.\n\nTo fix this issue and future-proof the code, we split feature value\nretrieval into its own gitweb_get_feature()function , and ensure that\nthe boolean feature check function gitweb_check_feature() always returns\na scalar (precisely, the first/only item in the feature value list).\n\nUsage of gitweb_check_feature() across gitweb is replaced with\ngitweb_get_feature() where appropriate, and list evaluation of\ngitweb_check_feature() is demoted to scalar evaluation to prevent\nambiguity. The few previously incorrect uses of gitweb_check_feature()\nin scalar context are left untouched because they are now correct.\n\nSigned-off-by: Giuseppe Bilotta \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..4246819 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. This allows us to write\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_get_feature('bool_feat');\n+# or\n+# (gitweb_get_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":"96904","messageId":"200812020253.09430.jnareb@gmail.com","threadId":"16516","inReplyTo":"1228008844-12506-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-12-02T01:53:06Z","receivedAt":"2008-12-02T01:53:06Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sun, 30 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 checking if a\n> feature was enabled.\n> \n> This led to subtle bugs in feature checking code: gitweb_check_feature\n> would return (0) for disabled features, so its use in scalar context\n> would return true instead of false.\n> \n> To fix this issue and future-proof the code, we split feature value\n> retrieval into its own gitweb_get_feature()function , and ensure that\n\n retrieval into its own gitweb_get_feature() function, and ensure that\n\n> the boolean feature check function gitweb_check_feature() always returns\n> a scalar (precisely, the first/only item in the feature value list).\n> \n> Usage of gitweb_check_feature() across gitweb is replaced with\n> gitweb_get_feature() where appropriate, and list evaluation of\n> gitweb_check_feature() is demoted to scalar evaluation to prevent\n> ambiguity. The few previously incorrect uses of gitweb_check_feature()\n> in scalar context are left untouched because they are now correct.\n> \n> Signed-off-by: Giuseppe Bilotta \n\nAcked-by: Jakub Narebski \n\n\nWhat I like about having all this, i.e. fix, futureproof and style\ncorrection in one single patch is the fact that fix doesn't introduce\nstrange looking (gitweb_check_feature('bool_feat'))[0]... well, except\nencapsulated in a subroutine.\n\n>From all possible splits of this feature into series of up to three\npatches I think I like the one with pure subroutine rename from *check*\nto *get* least...\n-- \nJakub Narebski\nPoland\n"},{"id":"96936","messageId":"1228214598-26557-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"16516","inReplyTo":"200812020253.09430.jnareb@gmail.com","subject":"[PATCHv3bis] gitweb: fixes to gitweb feature check code","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-12-02T10:43:18Z","receivedAt":"2008-12-02T10:43:18Z","isPatch":false,"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 checking if a\nfeature was enabled.\n\nThis led to subtle bugs in feature checking code: gitweb_check_feature\nwould return (0) for disabled features, so its use in scalar context\nwould return true instead of false.\n\nTo fix this issue and future-proof the code, we split feature value\nretrieval into its own gitweb_get_feature()function, and ensure that\nthe boolean feature check function gitweb_check_feature() always returns\na scalar (precisely, the first/only item in the feature value list).\n\nUsage of gitweb_check_feature() across gitweb is replaced with\ngitweb_get_feature() where appropriate, and list evaluation of\ngitweb_check_feature() is demoted to scalar evaluation to prevent\nambiguity. The few previously incorrect uses of gitweb_check_feature()\nin scalar context are left untouched because they are now correct.\n\nSigned-off-by: Giuseppe Bilotta \n---\n\nFixed typo in commit message.\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..4246819 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. This allows us to write\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_get_feature('bool_feat');\n+# or\n+# (gitweb_get_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":"96989","messageId":"7v4p1mp7hx.fsf@gitster.siamese.dyndns.org","threadId":"16516","inReplyTo":"200812020253.09430.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: fixes to gitweb feature check code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-02T21:55:22Z","receivedAt":"2008-12-02T21:55:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> What I like about having all this, i.e. fix, futureproof and style\n> correction in one single patch is the fact that fix doesn't introduce\n> strange looking (gitweb_check_feature('bool_feat'))[0]... well, except\n> encapsulated in a subroutine.\n>\n> From all possible splits of this feature into series of up to three\n> patches I think I like the one with pure subroutine rename from *check*\n> to *get* least...\n\nWell, I have to say that you have a strange taste, sense of priorities,\nand perhaps aversion to logical progression. Let's explain one more\ntime.\n\nThe case we had at hand was that a callee has a less-than-ideal calling\nconvention that has caused a few bugs by callers because they did not\nunderstand the calling convention. You can argue it is not entirely\ncaller's fault that they failed to follow the calling convention, but the\nfact remains that there are bugs taken as a whole.\n\nFirst we fix the callers, because existing bugs get highest priority.\nThis is a pure bugfix patch that could even go to maintenance \"bugfix\nonly\" branch.\n\nThen we fix the calling convention because we all know that the calling\nconvention was less-than-ideal. A large part of the reason the calling\nconvention was confusing was because the wording \"check\" implied it was a\nboolean function. Logically, s/check/get/ would be a major part of fixing\nthat.\n\nAfter calling convention is enhanced by a new function that lets callers\n\"check\" via a boolean function, we can have them use that, which makes\nthem easier to read.\n\nBut remember that it is the order I wanted the patches to be presented for\nreview. After people involved in review agree that the result is good, I\ndo not have any problem in squashing the three steps into a single patch\nfor things like this after the end result is verified to be good (which we\ndid).\n"},{"id":"97003","messageId":"200812030221.31180.jnareb@gmail.com","threadId":"16516","inReplyTo":"7v4p1mp7hx.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] gitweb: fixes to gitweb feature check code","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-03T01:21:30Z","receivedAt":"2008-12-03T01:21:30Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Dnia wtorek 2. grudnia 2008 22:55, Junio C Hamano napisał:\n> Jakub Narebski writes:\n> \n> > What I like about having all this, i.e. fix, futureproof and style\n> > correction in one single patch is the fact that fix doesn't introduce\n> > strange looking (gitweb_check_feature('bool_feat'))[0]... well, except\n> > encapsulated in a subroutine.\n> >\n> > From all possible splits of this feature into series of up to three\n> > patches I think I like the one with pure subroutine rename from *check*\n> > to *get* least...\n> \n> Well, I have to say that you have a strange taste, sense of priorities,\n> and perhaps aversion to logical progression. Let's explain one more\n> time.\n> \n> The case we had at hand was that a callee has a less-than-ideal calling\n> convention that has caused a few bugs by callers because they did not\n> understand the calling convention. You can argue it is not entirely\n> caller's fault that they failed to follow the calling convention, but the\n> fact remains that there are bugs taken as a whole.\n> \n> First we fix the callers, because existing bugs get highest priority.\n> This is a pure bugfix patch that could even go to maintenance \"bugfix\n> only\" branch.\n\nI agree here.\n\n> Then we fix the calling convention because we all know that the calling\n> convention was less-than-ideal. A large part of the reason the calling\n> convention was confusing was because the wording \"check\" implied it was a\n> boolean function. Logically, s/check/get/ would be a major part of fixing\n> that.\n\nAnd here I slightly disagree. The lone change s/check/get/ doesn't\nactually do anything. Splitting wrongly named 'check' into checking\nif feature is enabled and actual getting config does. And it is IMVHO\nquote obvious which calls are to be to new semantic of 'check':\n my ($var) = gitweb_check_feature('feature');\nand\n (gitweb_check_feature('feature'))[0]\n\nand which are to be renamed to 'get':\n my ($a, $b) = gitweb_check_feature('feature');\nand\n my @a = gitweb_check_feature('feature');\n\nPerhaps it is more clear from the point of view of reviewing individual\npieces...\n\n-- \nJakub Narebski\nPoland\n"},{"id":"97008","messageId":"7vy6yyhuhn.fsf@gitster.siamese.dyndns.org","threadId":"16516","inReplyTo":"1228214598-26557-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv3bis] gitweb: fixes to gitweb feature check code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-03T02:18:12Z","receivedAt":"2008-12-03T02:18:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks, but the ones based on the previous round were already queued in\n'next', so I took the change to the check/get comment (the first hunk in\nthis squashed patch) not to lose the remaining improvements from this\npatch.\n"}]}