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 <giuseppe.bilotta@xxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
