Giuseppe Bilotta <giuseppe.bilotta@xxxxxxxxx> writes:
> Since gitweb_check_feature now returns a scalar, the parentheses around
> the variables initialized from gitweb_check_feature() are not needed.
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).
--
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
