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

From: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
Date: Nov 16, 2008, 21:57 UTC
Message-ID: <cb7bb73a0811161357m28c3110flcc6b3d6b0c0cfef7@mail.gmail.com>
In-Reply-To: <7vwsf31ima.fsf@gitster.siamese.dyndns.org>

On Sun, Nov 16, 2008 at 10:11 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 19 quoted lines

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

Show 6 quoted lines

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