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

From: Jakub Narebski <jnareb@gmail.com>
Date: Nov 30, 2008, 00:31 UTC
Message-ID: <200811300131.16787.jnareb@gmail.com>
In-Reply-To: <1227904793-1821-2-git-send-email-giuseppe.bilotta@gmail.com>

On Fri, 28 Nov 2008, Giuseppe Bilotta wrote:

Show 16 quoted lines

> 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 <giuseppe.bilotta@gmail.com>

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