patchgitweb: fixes to gitweb feature check code

> 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.

> +# 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

> @@ -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);

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

> 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"?

>> -     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.

> 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.
> ---

> 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.

> > 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

> 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.

> 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?