Skip to content

Commit

Permalink
gitweb: make gitweb_check_feature a boolean wrapper
Browse files Browse the repository at this point in the history 
The gitweb_get_feature() function retrieves the configuration parameters
for the feature (such as the list of snapshot formats or the list of
additional actions), but it is very often used to see if feature is
enabled (which is returned as the first element in the list).

Because accepting the returned list in the scalar context by mistake
yields the number of elements in the array, which is non-zero in all
cases, such a mistake would result in a bug for the latter use, with
disabled features appearing enabled.  All existing callers that call the
function for this purpose assign the return value in the list context to
retrieve the first element, but that is only because we fixed careless
callers recently.

This adds gitweb_check_feature() as a wrapper to gitweb_get_feature() that
can be called safely in the scalar context to see if a feature is enabled
to reduce the risk of future bugs.  Callers of "get" that use the call
only to see if the feature is enabled are updated to call this wrapper.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
  • Loading branch information
Giuseppe Bilotta authored and Junio C Hamano committed Dec 1, 2008
1 parent a7c5a28 commit 25b2790
Showing 1 changed file with 36 additions and 20 deletions.
56 changes: 36 additions & 20 deletions gitweb/gitweb.perl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ BEGIN
# if there is no 'sub' key (no feature-sub), then feature cannot be
# overriden
#
#
# use gitweb_check_feature(<feature>) to check if <feature> is enabled

# Enable the 'blame' blob view, showing the last commit that modified
# each line in the file. This can be very CPU-intensive.
Expand Down Expand Up @@ -344,6 +344,22 @@ sub gitweb_get_feature {
return $sub->(@defaults);
}

# A wrapper to check if a given feature is enabled.
# With this, you can say
#
# my $bool_feat = gitweb_check_feature('bool_feat');
# gitweb_check_feature('bool_feat') or somecode;
#
# instead of
#
# my ($bool_feat) = gitweb_get_feature('bool_feat');
# (gitweb_get_feature('bool_feat'))[0] or somecode;
#
sub gitweb_check_feature {
return (gitweb_get_feature(@_))[0];
}


sub feature_blame {
my ($val) = git_get_project_config('blame', '--bool');

Expand Down Expand Up @@ -810,7 +826,7 @@ (%)
}
}

my ($use_pathinfo) = gitweb_get_feature('pathinfo');
my $use_pathinfo = gitweb_check_feature('pathinfo');
if ($use_pathinfo) {
# try to put as many parameters as possible in PATH_INFO:
# - project name
Expand Down Expand Up @@ -2101,7 +2117,7 @@ sub git_get_projects_list {
$filter ||= '';
$filter =~ s/\.git$//;

my ($check_forks) = gitweb_get_feature('forks');
my $check_forks = gitweb_check_feature('forks');

if (-d $projects_list) {
# search in directory
Expand Down Expand Up @@ -2947,7 +2963,7 @@ sub git_header_html {
}
print "</div>\n";

my ($have_search) = gitweb_get_feature('search');
my $have_search = gitweb_check_feature('search');
if (defined $project && $have_search) {
if (!defined $searchtext) {
$searchtext = "";
Expand All @@ -2961,7 +2977,7 @@ sub git_header_html {
$search_hash = "HEAD";
}
my $action = $my_uri;
my ($use_pathinfo) = gitweb_get_feature('pathinfo');
my $use_pathinfo = gitweb_check_feature('pathinfo');
if ($use_pathinfo) {
$action .= "/".esc_url($project);
}
Expand Down Expand Up @@ -3454,7 +3470,7 @@ sub is_patch_split {
sub git_difftree_body {
my ($difftree, $hash, @parents) = @_;
my ($parent) = $parents[0];
my ($have_blame) = gitweb_get_feature('blame');
my $have_blame = gitweb_check_feature('blame');
print "<div class=\"list_head\">\n";
if ($#{$difftree} > 10) {
print(($#{$difftree} + 1) . " files changed:\n");
Expand Down Expand Up @@ -3914,7 +3930,7 @@ sub fill_project_list_info {
my ($projlist, $check_forks) = @_;
my @projects;

my ($show_ctags) = gitweb_get_feature('ctags');
my $show_ctags = gitweb_check_feature('ctags');
PROJECT:
foreach my $pr (@$projlist) {
my (@activity) = git_get_last_activity($pr->{'path'});
Expand Down Expand Up @@ -3968,7 +3984,7 @@ sub git_project_list_body {
# actually uses global variable $project
my ($projlist, $order, $from, $to, $extra, $no_header) = @_;

my ($check_forks) = gitweb_get_feature('forks');
my $check_forks = gitweb_check_feature('forks');
my @projects = fill_project_list_info($projlist, $check_forks);

$order ||= $default_projects_order;
Expand All @@ -3988,7 +4004,7 @@ sub git_project_list_body {
@projects = sort {$a->{$oi->{'key'}} <=> $b->{$oi->{'key'}}} @projects;
}

my ($show_ctags) = gitweb_get_feature('ctags');
my $show_ctags = gitweb_check_feature('ctags');
if ($show_ctags) {
my %ctags;
foreach my $p (@projects) {
Expand Down Expand Up @@ -4428,7 +4444,7 @@ sub git_summary {
my @taglist = git_get_tags_list(16);
my @headlist = git_get_heads_list(16);
my @forklist;
my ($check_forks) = gitweb_get_feature('forks');
my $check_forks = gitweb_check_feature('forks');

if ($check_forks) {
@forklist = git_get_projects_list($project);
Expand Down Expand Up @@ -4457,7 +4473,7 @@ sub git_summary {
}

# Tag cloud
my ($show_ctags) = gitweb_get_feature('ctags');
my $show_ctags = gitweb_check_feature('ctags');
if ($show_ctags) {
my $ctags = git_get_project_ctags($project);
my $cloud = git_populate_project_tagcloud($ctags);
Expand Down Expand Up @@ -4559,7 +4575,7 @@ sub git_blame {
my $fd;
my $ftype;

gitweb_get_feature('blame')[0]
gitweb_check_feature('blame')
or die_error(403, "Blame view not allowed");

die_error(400, "No file name given") unless $file_name;
Expand Down Expand Up @@ -4747,7 +4763,7 @@ sub git_blob {
$expires = "+1d";
}

my ($have_blame) = gitweb_get_feature('blame');
my $have_blame = gitweb_check_feature('blame');
open my $fd, "-|", git_cmd(), "cat-file", "blob", $hash
or die_error(500, "Couldn't cat $file_name, $hash");
my $mimetype = blob_mimetype($fd, $file_name);
Expand Down Expand Up @@ -4840,7 +4856,7 @@ sub git_tree {
my $ref = format_ref_marker($refs, $hash_base);
git_header_html();
my $basedir = '';
my ($have_blame) = gitweb_get_feature('blame');
my $have_blame = gitweb_check_feature('blame');
if (defined $hash_base && (my %co = parse_commit($hash_base))) {
my @views_nav = ();
if (defined $file_name) {
Expand Down Expand Up @@ -5610,7 +5626,7 @@ sub git_history {
}

sub git_search {
gitweb_get_feature('search')[0] or die_error(403, "Search is disabled");
gitweb_check_feature('search') or die_error(403, "Search is disabled");
if (!defined $searchtext) {
die_error(400, "Text field is empty");
}
Expand All @@ -5629,11 +5645,11 @@ sub git_search {
if ($searchtype eq 'pickaxe') {
# pickaxe may take all resources of your box and run for several minutes
# with every query - so decide by yourself how public you make this feature
gitweb_get_feature('pickaxe')[0]
gitweb_check_feature('pickaxe')
or die_error(403, "Pickaxe is disabled");
}
if ($searchtype eq 'grep') {
gitweb_get_feature('grep')[0]
gitweb_check_feature('grep')
or die_error(403, "Grep is disabled");
}

Expand Down Expand Up @@ -5838,7 +5854,7 @@ sub git_search_help {
<dt><b>commit</b></dt>
<dd>The commit messages and authorship information will be scanned for the given pattern.</dd>
EOT
my ($have_grep) = gitweb_get_feature('grep');
my $have_grep = gitweb_check_feature('grep');
if ($have_grep) {
print <<EOT;
<dt><b>grep</b></dt>
Expand All @@ -5855,7 +5871,7 @@ sub git_search_help {
<dt><b>committer</b></dt>
<dd>Name and e-mail of the committer and date of commit will be scanned for the given pattern.</dd>
EOT
my ($have_pickaxe) = gitweb_get_feature('pickaxe');
my $have_pickaxe = gitweb_check_feature('pickaxe');
if ($have_pickaxe) {
print <<EOT;
<dt><b>pickaxe</b></dt>
Expand Down Expand Up @@ -5907,7 +5923,7 @@ sub git_shortlog {

sub git_feed {
my $format = shift || 'atom';
my ($have_blame) = gitweb_get_feature('blame');
my $have_blame = gitweb_check_feature('blame');

# Atom: http://www.atomenabled.org/developers/syndication/
# RSS: http://www.notestips.com/80256B3A007F2692/1/NAMO5P9UPQ
Expand Down

0 comments on commit 25b2790

Please sign in to comment.