Re: [PATCHv2ter 2/2] gitweb: clean up gitweb_check_feature() calls

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

Here is what I think the commit message of your [1/2] should have read:

    Subject: [PATCH] gitweb: make gitweb_check_feature a boolean wrapper

    The gitweb_check_feature() function retrieves the configuration
    parameters for the feature (such as the list of snapshot formats or
    the list of additional actions), but it is very often used to see if
    feature is enabled (which is returned as the first element in the
    list).

    Because accepting the returned list in a scalar context by mistake
    yields the number of elements in the array, which is non-zero in all
    cases, such a mistake would result in a bug for the latter use, with
    disabled features appearing enabled.  All existing callers that call
    the function for this purpose assign the return value in the list
    context to retrieve the first element, but this is a bug waiting to
    happen for future callers that are not careful.

    This changes gitweb_check_feature() to a wrapper that can be called
    safely in the scalar context to see if a feature is enabled to reduce
    the risk of future bugs.  A new function gitweb_get_feature() is
    introduced for the callers that want the list of configuration
    parameters.  All existing callers of gitweb_check_feature() that want
    the configuration parameters are adjusted to call gitweb_get_feature().

Notice the difference in the tone from your [1/2]. The change is not about fixing (your proposed commit log message read "gitweb:fixes to gitweb feature check code") as nothing is broken. It is purely about futureproofing and I think we should mark it as such.

The splitting of two patches is not strictly necessary, but if you wanted to, I would do so slightly differently:

 * Make [1/2] purely the rename of s/check/get/, saying "the function is
   not just checking if it is enabled, but it is getting the configuration
   parameter list.  I'll introduce a new API to ask if it is enabled,
   which will be called 'check', hence this rename".

   Obviously, after applying this patch, there should be no caller of
   "check".

 * Make [2/2] introduce the "check" wrapper, and change the existing
   callers of "get" that want boolean to call "check" (and in the scalar
   context, as it does not matter anymore).  You can steal most of the
   above rewrite as your commit message for this (except for the last two
   sentences).

Also I would rewrite the mysterious "Comfort code" comment like this:

        # A wrapper to check if a given feature is enabled.
        # With this, you can say
        #
        #   my $bool_feat = gitweb_check_feature('bool_feat');
        #   gitweb_check_feature('bool_feat') or somecode;
        #
        # instead of
        #
        #   my ($bool_feat) = gitweb_get_feature('bool_feat');
        #   (gitweb_get_feature('bool_feat'))[0] or somecode;

(you also had a few typos in these four example calls).