[PATCH] gitweb: fixes to gitweb feature check code

[PATCH] gitweb: fixes to gitweb feature check code

[Giuseppe Bilotta]

The gitweb_check_feature routine was being used for two different
purposes: retrieving the actual feature value (such as the list of
snapshot formats or the list of additional actions), and to check if a
feature was enabled.

For the latter use, since all features return an array, it led to either
clumsy code or subtle bugs, with disabled features appearing enabled
because (0) evaluates to 1.

We fix these bugs, and simplify the code, by separating feature (list)
value retrieval (gitweb_get_feature) from boolean feature check (i.e.
retrieving the first/only item in the feature value list). Usage of
gitweb_check_feature across gitweb is replaced by the appropriate call
wherever needed.
---
 gitweb/gitweb.perl |   52 +++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 35 insertions(+), 17 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 933e137..b0d00ea 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -190,7 +190,9 @@ our %feature = (
 	# 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
+	# use gitweb_get_feature(<feature>) to retrieve the <feature> value
+	# (an array) or 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.
@@ -329,7 +331,8 @@ our %feature = (
 		'default' => [0]},
 );
 
-sub gitweb_check_feature {
+# retrieve the value of a given feature, as an array
+sub gitweb_get_feature {
 	my ($name) = @_;
 	return unless exists $feature{$name};
 	my ($sub, $override, @defaults) = (
@@ -344,6 +347,21 @@ sub gitweb_check_feature {
 	return $sub->(@defaults);
 }
 
+# check if a given feature is enabled or not, returning the first (and only)
+# value of the feature. Comfort code, allowing the use of
+#   my $bool_feat = gitweb_check_feature('bool_feat');
+# or
+#   gitweb_check_feature('bool_feat') or somecode;
+# instead of
+#   my ($bool_feat) = gitweb_git_feature('bool_feat');
+# or
+#   (gitweb_check_feature('bool_feat'))[0] or somecode;
+# respectively
+sub gitweb_check_feature {
+	return (gitweb_get_feature(@_))[0];
+}
+
+
 sub feature_blame {
 	my ($val) = git_get_project_config('blame', '--bool');
 
@@ -767,7 +785,7 @@ our $git_dir;
 $git_dir = "$projectroot/$project" if $project;
 
 # list of supported snapshot formats
-our @snapshot_fmts = gitweb_check_feature('snapshot');
+our @snapshot_fmts = gitweb_get_feature('snapshot');
 @snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);
 
 # dispatch
@@ -810,7 +828,7 @@ sub href (%) {
 		}
 	}
 
-	my ($use_pathinfo) = gitweb_check_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
@@ -2101,7 +2119,7 @@ sub git_get_projects_list {
 	$filter ||= '';
 	$filter =~ s/\.git$//;
 
-	my ($check_forks) = gitweb_check_feature('forks');
+	my $check_forks = gitweb_check_feature('forks');
 
 	if (-d $projects_list) {
 		# search in directory
@@ -2947,7 +2965,7 @@ EOF
 	}
 	print "</div>\n";
 
-	my ($have_search) = gitweb_check_feature('search');
+	my $have_search = gitweb_check_feature('search');
 	if (defined $project && $have_search) {
 		if (!defined $searchtext) {
 			$searchtext = "";
@@ -2961,7 +2979,7 @@ EOF
 			$search_hash = "HEAD";
 		}
 		my $action = $my_uri;
-		my ($use_pathinfo) = gitweb_check_feature('pathinfo');
+		my $use_pathinfo = gitweb_check_feature('pathinfo');
 		if ($use_pathinfo) {
 			$action .= "/".esc_url($project);
 		}
@@ -3084,7 +3102,7 @@ sub git_print_page_nav {
 	$arg{'tree'}{'hash'} = $treehead if defined $treehead;
 	$arg{'tree'}{'hash_base'} = $treebase if defined $treebase;
 
-	my @actions = gitweb_check_feature('actions');
+	my @actions = gitweb_get_feature('actions');
 	my %repl = (
 		'%' => '%',
 		'n' => $project,         # project name
@@ -3454,7 +3472,7 @@ sub is_patch_split {
 sub git_difftree_body {
 	my ($difftree, $hash, @parents) = @_;
 	my ($parent) = $parents[0];
-	my ($have_blame) = gitweb_check_feature('blame');
+	my $have_blame = gitweb_check_feature('blame');
 	print "<div class=\"list_head\">\n";
 	if ($#{$difftree} > 10) {
 		print(($#{$difftree} + 1) . " files changed:\n");
@@ -3968,7 +3986,7 @@ sub git_project_list_body {
 	# actually uses global variable $project
 	my ($projlist, $order, $from, $to, $extra, $no_header) = @_;
 
-	my ($check_forks) = gitweb_check_feature('forks');
+	my $check_forks = gitweb_check_feature('forks');
 	my @projects = fill_project_list_info($projlist, $check_forks);
 
 	$order ||= $default_projects_order;
@@ -4428,7 +4446,7 @@ sub git_summary {
 	my @taglist  = git_get_tags_list(16);
 	my @headlist = git_get_heads_list(16);
 	my @forklist;
-	my ($check_forks) = gitweb_check_feature('forks');
+	my $check_forks = gitweb_check_feature('forks');
 
 	if ($check_forks) {
 		@forklist = git_get_projects_list($project);
@@ -4457,7 +4475,7 @@ sub git_summary {
 	}
 
 	# Tag cloud
-	my $show_ctags = (gitweb_check_feature('ctags'))[0];
+	my $show_ctags = gitweb_check_feature('ctags');
 	if ($show_ctags) {
 		my $ctags = git_get_project_ctags($project);
 		my $cloud = git_populate_project_tagcloud($ctags);
@@ -4747,7 +4765,7 @@ sub git_blob {
 		$expires = "+1d";
 	}
 
-	my ($have_blame) = gitweb_check_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);
@@ -4840,7 +4858,7 @@ sub git_tree {
 	my $ref = format_ref_marker($refs, $hash_base);
 	git_header_html();
 	my $basedir = '';
-	my ($have_blame) = gitweb_check_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) {
@@ -5838,7 +5856,7 @@ insensitive).</p>
 <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_check_feature('grep');
+	my $have_grep = gitweb_check_feature('grep');
 	if ($have_grep) {
 		print <<EOT;
 <dt><b>grep</b></dt>
@@ -5855,7 +5873,7 @@ EOT
 <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_check_feature('pickaxe');
+	my $have_pickaxe = gitweb_check_feature('pickaxe');
 	if ($have_pickaxe) {
 		print <<EOT;
 <dt><b>pickaxe</b></dt>
@@ -5907,7 +5925,7 @@ sub git_shortlog {
 
 sub git_feed {
 	my $format = shift || 'atom';
-	my ($have_blame) = gitweb_check_feature('blame');
+	my $have_blame = gitweb_check_feature('blame');
 
 	# Atom: http://www.atomenabled.org/developers/syndication/
 	# RSS:  http://www.notestips.com/80256B3A007F2692/1/NAMO5P9UPQ
-- 
1.5.6.5

Re: [PATCH] gitweb: fixes to gitweb feature check code

[Giuseppe Bilotta]

Forgot the sign-off line

On Sat, Nov 15, 2008 at 3:26 PM, Giuseppe Bilotta
<giuseppe.bilotta@gmail.com> wrote:
> The gitweb_check_feature routine was being used for two different
> purposes: retrieving the actual feature value (such as the list of
> snapshot formats or the list of additional actions), and to check if a
> feature was enabled.
>
> For the latter use, since all features return an array, it led to either
> clumsy code or subtle bugs, with disabled features appearing enabled
> because (0) evaluates to 1.
>
> We fix these bugs, and simplify the code, by separating feature (list)
> value retrieval (gitweb_get_feature) from boolean feature check (i.e.
> retrieving the first/only item in the feature value list). Usage of
> gitweb_check_feature across gitweb is replaced by the appropriate call
> wherever needed.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH] gitweb: fixes to gitweb feature check code

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

> The gitweb_check_feature routine was being used for two different
> purposes: retrieving the actual feature value (such as the list of
> snapshot formats or the list of additional actions), and to check if a
> feature was enabled.

> +# check if a given feature is enabled or not, returning the first (and only)
> +# value of the feature. Comfort code, allowing the use of
> +#   my $bool_feat = gitweb_check_feature('bool_feat');
> +# or
> +#   gitweb_check_feature('bool_feat') or somecode;
> +# instead of
> +#   my ($bool_feat) = gitweb_git_feature('bool_feat');
> +# or
> +#   (gitweb_check_feature('bool_feat'))[0] or somecode;
> +# respectively

What's "Comfort code"?

I'd agree that introduction of gitweb_get_feature() may help avoiding
mistakes at the call sites for Perl illiterates like myself.

> @@ -767,7 +785,7 @@ our $git_dir;
>  $git_dir = "$projectroot/$project" if $project;
>  
>  # list of supported snapshot formats
> -our @snapshot_fmts = gitweb_check_feature('snapshot');
> +our @snapshot_fmts = gitweb_get_feature('snapshot');
>  @snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);

And this may be a good change from that point of view, but...

> @@ -810,7 +828,7 @@ sub href (%) {
>  		}
>  	}
>  
> -	my ($use_pathinfo) = gitweb_check_feature('pathinfo');
> +	my $use_pathinfo = gitweb_check_feature('pathinfo');

... I do not think changes like these are warranted.  They have been using
the function _correctly_ by calling it in the list context and I think
they will continue to work with your patch.

Re: [PATCH] gitweb: fixes to gitweb feature check code

[Giuseppe Bilotta]

On Sun, Nov 16, 2008 at 10:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> The gitweb_check_feature routine was being used for two different
>> purposes: retrieving the actual feature value (such as the list of
>> snapshot formats or the list of additional actions), and to check if a
>> feature was enabled.
>
>> +# check if a given feature is enabled or not, returning the first (and only)
>> +# value of the feature. Comfort code, allowing the use of
>> +#   my $bool_feat = gitweb_check_feature('bool_feat');
>> +# or
>> +#   gitweb_check_feature('bool_feat') or somecode;
>> +# instead of
>> +#   my ($bool_feat) = gitweb_git_feature('bool_feat');
>> +# or
>> +#   (gitweb_check_feature('bool_feat'))[0] or somecode;
>> +# respectively
>
> What's "Comfort code"?

Code that provides comfort? 8-D

> I'd agree that introduction of gitweb_get_feature() may help avoiding
> mistakes at the call sites for Perl illiterates like myself.

>> -     my ($use_pathinfo) = gitweb_check_feature('pathinfo');
>> +     my $use_pathinfo = gitweb_check_feature('pathinfo');
>
> ... I do not think changes like these are warranted.  They have been using
> the function _correctly_ by calling it in the list context and I think
> they will continue to work with your patch.

Because the returned scalar value gets properly promoted to array?
Maybe, but I would say that keeping the () is confusing and
inconsistent. After all, the purpose of the patch is to _eliminate_
(the need for) such constructs.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH] gitweb: fixes to gitweb feature check code

[Jakub Narebski]

On Sat, 15 Nov 2008, Giuseppe Bilotta wrote:

> The gitweb_check_feature routine was being used for two different
> purposes: retrieving the actual feature value (such as the list of
> snapshot formats or the list of additional actions), and to check if a
> feature was enabled.
> 
> For the latter use, since all features return an array, it led to either
> clumsy code or subtle bugs, with disabled features appearing enabled
> because (0) evaluates to 1.
> 
> We fix these bugs, and simplify the code, by separating feature (list)
> value retrieval (gitweb_get_feature) from boolean feature check (i.e.
> retrieving the first/only item in the feature value list). Usage of
> gitweb_check_feature across gitweb is replaced by the appropriate call
> wherever needed.
> ---

First, you forgot the signoff, but you have addressed that already.


Second, I thought at first that it would be good for the patch to also
simplify %feature hash, using "'default' => 1" instead of current bit
convoluted "'default' => [1]", at the cost of bit more code for
defensive programming.  But now I think that if it is to be done,
it should be put as separate patch.

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: fixes to gitweb feature check code

[Giuseppe Bilotta]

On Mon, Nov 17, 2008 at 2:02 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Sat, 15 Nov 2008, Giuseppe Bilotta wrote:
>
>> The gitweb_check_feature routine was being used for two different
>> purposes: retrieving the actual feature value (such as the list of
>> snapshot formats or the list of additional actions), and to check if a
>> feature was enabled.
>>
>> For the latter use, since all features return an array, it led to either
>> clumsy code or subtle bugs, with disabled features appearing enabled
>> because (0) evaluates to 1.
>>
>> We fix these bugs, and simplify the code, by separating feature (list)
>> value retrieval (gitweb_get_feature) from boolean feature check (i.e.
>> retrieving the first/only item in the feature value list). Usage of
>> gitweb_check_feature across gitweb is replaced by the appropriate call
>> wherever needed.
>> ---
>
> First, you forgot the signoff, but you have addressed that already.
>
>
> Second, I thought at first that it would be good for the patch to also
> simplify %feature hash, using "'default' => 1" instead of current bit
> convoluted "'default' => [1]", at the cost of bit more code for
> defensive programming.  But now I think that if it is to be done,
> it should be put as separate patch.

Is this an ACK? 8-D

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH] gitweb: fixes to gitweb feature check code

[Jakub Narebski]

Dnia poniedziałek 17. listopada 2008 07:10, Giuseppe Bilotta napisał:
> On Mon, Nov 17, 2008 at 2:02 AM, Jakub Narebski <jnareb@gmail.com> wrote:

[...]
> > First, you forgot the signoff, but you have addressed that already.
> >
> >
> > Second, I thought at first that it would be good for the patch to also
> > simplify %feature hash, using "'default' => 1" instead of current bit
> > convoluted "'default' => [1]", at the cost of bit more code for
> > defensive programming.  But now I think that if it is to be done,
> > it should be put as separate patch.
> 
> Is this an ACK? 8-D

I'm sorry. Yes, it is.

Acked-by: Jakub Narebski <jnareb@gmail.com>

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: fixes to gitweb feature check code

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Dnia poniedziałek 17. listopada 2008 07:10, Giuseppe Bilotta napisał:
>> On Mon, Nov 17, 2008 at 2:02 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>
> [...]
>> > First, you forgot the signoff, but you have addressed that already.
>> >
>> >
>> > Second, I thought at first that it would be good for the patch to also
>> > simplify %feature hash, using "'default' => 1" instead of current bit
>> > convoluted "'default' => [1]", at the cost of bit more code for
>> > defensive programming.  But now I think that if it is to be done,
>> > it should be put as separate patch.
>> 
>> Is this an ACK? 8-D
>
> I'm sorry. Yes, it is.

Are you sure, even with those unnecessary changes from list context
assignments to scalar ones?

Re: [PATCH] gitweb: fixes to gitweb feature check code

[Jakub Narebski]

On Mon, 17 Nov 2008, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>> Dnia poniedziałek 17. listopada 2008 07:10, Giuseppe Bilotta napisał:
>>> On Mon, Nov 17, 2008 at 2:02 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>>
>> [...]
>>>> First, you forgot the signoff, but you have addressed that already.
>>>>
>>>>
>>>> Second, I thought at first that it would be good for the patch to also
>>>> simplify %feature hash, using "'default' => 1" instead of current bit
>>>> convoluted "'default' => [1]", at the cost of bit more code for
>>>> defensive programming.  But now I think that if it is to be done,
>>>> it should be put as separate patch.
>>> 
>>> Is this an ACK? 8-D
>>
>> I'm sorry. Yes, it is.
> 
> Are you sure, even with those unnecessary changes from list context
> assignments to scalar ones?

Well, on one hand this change is not _necessary_, as it would work
without it. On the other hand it feels like cleanup (like e.g. using
tabs to indent but spaces to align, or word-wrapping too long lines).

So I think I'll pass the ball to you... your call ;-)
-- 
Jakub Narebski
Poland

[PATCH] gitweb: fixes to gitweb feature check code

[Giuseppe Bilotta]

The gitweb_check_feature routine was being used for two different
purposes: retrieving the actual feature value (such as the list of
snapshot formats or the list of additional actions), and checking if a
feature was enabled.

This led to subtle bugs in feature checking code: gitweb_check_feature
would return (0) for disabled features, so its use in scalar context
would return true instead of false.

To fix this issue and future-proof the code, we split feature value
retrieval into its own gitweb_get_feature()function , and ensure that
the boolean feature check function gitweb_check_feature() always returns
a scalar (precisely, the first/only item in the feature value list).

Usage of gitweb_check_feature() across gitweb is replaced with
gitweb_get_feature() where appropriate, and list evaluation of
gitweb_check_feature() is demoted to scalar evaluation to prevent
ambiguity. The few previously incorrect uses of gitweb_check_feature()
in scalar context are left untouched because they are now correct.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |   52 +++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 35 insertions(+), 17 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 933e137..4246819 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -190,7 +190,9 @@ our %feature = (
 	# 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
+	# use gitweb_get_feature(<feature>) to retrieve the <feature> value
+	# (an array) or 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.
@@ -329,7 +331,8 @@ our %feature = (
 		'default' => [0]},
 );
 
-sub gitweb_check_feature {
+# retrieve the value of a given feature, as an array
+sub gitweb_get_feature {
 	my ($name) = @_;
 	return unless exists $feature{$name};
 	my ($sub, $override, @defaults) = (
@@ -344,6 +347,21 @@ sub gitweb_check_feature {
 	return $sub->(@defaults);
 }
 
+# check if a given feature is enabled or not, returning the first (and only)
+# value of the feature. This allows us to write
+#   my $bool_feat = gitweb_check_feature('bool_feat');
+# or
+#   gitweb_check_feature('bool_feat') or somecode;
+# instead of
+#   my ($bool_feat) = gitweb_get_feature('bool_feat');
+# or
+#   (gitweb_get_feature('bool_feat'))[0] or somecode;
+# respectively
+sub gitweb_check_feature {
+	return (gitweb_get_feature(@_))[0];
+}
+
+
 sub feature_blame {
 	my ($val) = git_get_project_config('blame', '--bool');
 
@@ -767,7 +785,7 @@ our $git_dir;
 $git_dir = "$projectroot/$project" if $project;
 
 # list of supported snapshot formats
-our @snapshot_fmts = gitweb_check_feature('snapshot');
+our @snapshot_fmts = gitweb_get_feature('snapshot');
 @snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);
 
 # dispatch
@@ -810,7 +828,7 @@ sub href (%) {
 		}
 	}
 
-	my ($use_pathinfo) = gitweb_check_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
@@ -2101,7 +2119,7 @@ sub git_get_projects_list {
 	$filter ||= '';
 	$filter =~ s/\.git$//;
 
-	my ($check_forks) = gitweb_check_feature('forks');
+	my $check_forks = gitweb_check_feature('forks');
 
 	if (-d $projects_list) {
 		# search in directory
@@ -2947,7 +2965,7 @@ EOF
 	}
 	print "</div>\n";
 
-	my ($have_search) = gitweb_check_feature('search');
+	my $have_search = gitweb_check_feature('search');
 	if (defined $project && $have_search) {
 		if (!defined $searchtext) {
 			$searchtext = "";
@@ -2961,7 +2979,7 @@ EOF
 			$search_hash = "HEAD";
 		}
 		my $action = $my_uri;
-		my ($use_pathinfo) = gitweb_check_feature('pathinfo');
+		my $use_pathinfo = gitweb_check_feature('pathinfo');
 		if ($use_pathinfo) {
 			$action .= "/".esc_url($project);
 		}
@@ -3084,7 +3102,7 @@ sub git_print_page_nav {
 	$arg{'tree'}{'hash'} = $treehead if defined $treehead;
 	$arg{'tree'}{'hash_base'} = $treebase if defined $treebase;
 
-	my @actions = gitweb_check_feature('actions');
+	my @actions = gitweb_get_feature('actions');
 	my %repl = (
 		'%' => '%',
 		'n' => $project,         # project name
@@ -3454,7 +3472,7 @@ sub is_patch_split {
 sub git_difftree_body {
 	my ($difftree, $hash, @parents) = @_;
 	my ($parent) = $parents[0];
-	my ($have_blame) = gitweb_check_feature('blame');
+	my $have_blame = gitweb_check_feature('blame');
 	print "<div class=\"list_head\">\n";
 	if ($#{$difftree} > 10) {
 		print(($#{$difftree} + 1) . " files changed:\n");
@@ -3968,7 +3986,7 @@ sub git_project_list_body {
 	# actually uses global variable $project
 	my ($projlist, $order, $from, $to, $extra, $no_header) = @_;
 
-	my ($check_forks) = gitweb_check_feature('forks');
+	my $check_forks = gitweb_check_feature('forks');
 	my @projects = fill_project_list_info($projlist, $check_forks);
 
 	$order ||= $default_projects_order;
@@ -4428,7 +4446,7 @@ sub git_summary {
 	my @taglist  = git_get_tags_list(16);
 	my @headlist = git_get_heads_list(16);
 	my @forklist;
-	my ($check_forks) = gitweb_check_feature('forks');
+	my $check_forks = gitweb_check_feature('forks');
 
 	if ($check_forks) {
 		@forklist = git_get_projects_list($project);
@@ -4457,7 +4475,7 @@ sub git_summary {
 	}
 
 	# Tag cloud
-	my $show_ctags = (gitweb_check_feature('ctags'))[0];
+	my $show_ctags = gitweb_check_feature('ctags');
 	if ($show_ctags) {
 		my $ctags = git_get_project_ctags($project);
 		my $cloud = git_populate_project_tagcloud($ctags);
@@ -4747,7 +4765,7 @@ sub git_blob {
 		$expires = "+1d";
 	}
 
-	my ($have_blame) = gitweb_check_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);
@@ -4840,7 +4858,7 @@ sub git_tree {
 	my $ref = format_ref_marker($refs, $hash_base);
 	git_header_html();
 	my $basedir = '';
-	my ($have_blame) = gitweb_check_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) {
@@ -5838,7 +5856,7 @@ insensitive).</p>
 <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_check_feature('grep');
+	my $have_grep = gitweb_check_feature('grep');
 	if ($have_grep) {
 		print <<EOT;
 <dt><b>grep</b></dt>
@@ -5855,7 +5873,7 @@ EOT
 <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_check_feature('pickaxe');
+	my $have_pickaxe = gitweb_check_feature('pickaxe');
 	if ($have_pickaxe) {
 		print <<EOT;
 <dt><b>pickaxe</b></dt>
@@ -5907,7 +5925,7 @@ sub git_shortlog {
 
 sub git_feed {
 	my $format = shift || 'atom';
-	my ($have_blame) = gitweb_check_feature('blame');
+	my $have_blame = gitweb_check_feature('blame');
 
 	# Atom: http://www.atomenabled.org/developers/syndication/
 	# RSS:  http://www.notestips.com/80256B3A007F2692/1/NAMO5P9UPQ
-- 
1.5.6.5

Re: [PATCH] gitweb: fixes to gitweb feature check code

[Jakub Narebski]

On Sun, 30 Nov 2008, Giuseppe Bilotta wrote:

> The gitweb_check_feature routine was being used for two different
> purposes: retrieving the actual feature value (such as the list of
> snapshot formats or the list of additional actions), and checking if a
> feature was enabled.
> 
> This led to subtle bugs in feature checking code: gitweb_check_feature
> would return (0) for disabled features, so its use in scalar context
> would return true instead of false.
> 
> To fix this issue and future-proof the code, we split feature value
> retrieval into its own gitweb_get_feature()function , and ensure that

  retrieval into its own gitweb_get_feature() function, and ensure that

> the boolean feature check function gitweb_check_feature() always returns
> a scalar (precisely, the first/only item in the feature value list).
> 
> Usage of gitweb_check_feature() across gitweb is replaced with
> gitweb_get_feature() where appropriate, and list evaluation of
> gitweb_check_feature() is demoted to scalar evaluation to prevent
> ambiguity. The few previously incorrect uses of gitweb_check_feature()
> in scalar context are left untouched because they are now correct.
> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

Acked-by: Jakub Narebski <jnareb@gmail.com>


What I like about having all this, i.e. fix, futureproof and style
correction in one single patch is the fact that fix doesn't introduce
strange looking (gitweb_check_feature('bool_feat'))[0]... well, except
encapsulated in a subroutine.

>From all possible splits of this feature into series of up to three
patches I think I like the one with pure subroutine rename from *check*
to *get* least...
-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: fixes to gitweb feature check code

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> What I like about having all this, i.e. fix, futureproof and style
> correction in one single patch is the fact that fix doesn't introduce
> strange looking (gitweb_check_feature('bool_feat'))[0]... well, except
> encapsulated in a subroutine.
>
> From all possible splits of this feature into series of up to three
> patches I think I like the one with pure subroutine rename from *check*
> to *get* least...

Well, I have to say that you have a strange taste, sense of priorities,
and perhaps aversion to logical progression.  Let's explain one more
time.

The case we had at hand was that a callee has a less-than-ideal calling
convention that has caused a few bugs by callers because they did not
understand the calling convention.  You can argue it is not entirely
caller's fault that they failed to follow the calling convention, but the
fact remains that there are bugs taken as a whole.

First we fix the callers, because existing bugs get highest priority.
This is a pure bugfix patch that could even go to maintenance "bugfix
only" branch.

Then we fix the calling convention because we all know that the calling
convention was less-than-ideal.  A large part of the reason the calling
convention was confusing was because the wording "check" implied it was a
boolean function.  Logically, s/check/get/ would be a major part of fixing
that.

After calling convention is enhanced by a new function that lets callers
"check" via a boolean function, we can have them use that, which makes
them easier to read.

But remember that it is the order I wanted the patches to be presented for
review.  After people involved in review agree that the result is good, I
do not have any problem in squashing the three steps into a single patch
for things like this after the end result is verified to be good (which we
did).

Re: [PATCH] gitweb: fixes to gitweb feature check code

[Jakub Narebski]

Dnia wtorek 2. grudnia 2008 22:55, Junio C Hamano napisał:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > What I like about having all this, i.e. fix, futureproof and style
> > correction in one single patch is the fact that fix doesn't introduce
> > strange looking (gitweb_check_feature('bool_feat'))[0]... well, except
> > encapsulated in a subroutine.
> >
> > From all possible splits of this feature into series of up to three
> > patches I think I like the one with pure subroutine rename from *check*
> > to *get* least...
> 
> Well, I have to say that you have a strange taste, sense of priorities,
> and perhaps aversion to logical progression.  Let's explain one more
> time.
> 
> The case we had at hand was that a callee has a less-than-ideal calling
> convention that has caused a few bugs by callers because they did not
> understand the calling convention.  You can argue it is not entirely
> caller's fault that they failed to follow the calling convention, but the
> fact remains that there are bugs taken as a whole.
> 
> First we fix the callers, because existing bugs get highest priority.
> This is a pure bugfix patch that could even go to maintenance "bugfix
> only" branch.

I agree here.

> Then we fix the calling convention because we all know that the calling
> convention was less-than-ideal.  A large part of the reason the calling
> convention was confusing was because the wording "check" implied it was a
> boolean function.  Logically, s/check/get/ would be a major part of fixing
> that.

And here I slightly disagree. The lone change s/check/get/ doesn't
actually do anything. Splitting wrongly named 'check' into checking
if feature is enabled and actual getting config does. And it is IMVHO
quote obvious which calls are to be to new semantic of 'check':
  my ($var) = gitweb_check_feature('feature');
and
  (gitweb_check_feature('feature'))[0]

and which are to be renamed to 'get':
  my ($a, $b) = gitweb_check_feature('feature');
and
  my @a  = gitweb_check_feature('feature');

Perhaps it is more clear from the point of view of reviewing individual
pieces...

-- 
Jakub Narebski
Poland