Re: [PATCHv2 1/2] gitweb: fixes to gitweb feature check code

On Fri, 28 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 is replaced by gitweb_get_feature where
appropriate.

Signed-off-by: Giuseppe Bilotta <redacted>

I like the idea of futureproofing gitweb code to avoid further bugs
with checking if feature is enabled. So from me this patch gets Ack.

I have thought at first that perhaps this patch could also remove
requirement that 'default' field is array (reference), replacing a bit
cumbersome
  $feature{'blame'}{'default'} = [1];
with simpler
  $feature{'blame'}{'default'} = 1;

But now I think that is separate issue, and if we think it is worth
having (in spite of its disadvantages in form of more complicated code
dealing among others with legacy configuration), it should be put in
separate patch.

-- 
Jakub Narebski
Poland