Ævar Arnfjörð Bjarmason wrote:
> Signed-off-by: Ævar Arnfjörð Bjarmason <ava...@gmail.com>
> ---
> gitweb/INSTALL | 3 +--
> gitweb/gitweb.perl | 17 +++++------------
> 2 files changed, 6 insertions(+), 14 deletions(-)
Makes sense, and I like the diffstat.
[...]
> +++ b/gitweb/gitweb.perl
[...]
> @@ -1166,18 +1165,11 @@ sub configure_gitweb_features {
> our @snapshot_fmts = gitweb_get_feature('snapshot');
> @snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);
>
> - # check that the avatar feature is set to a known provider name,
> - # and for each provider check if the dependencies are satisfied.
> - # if the provider name is invalid or the dependencies are not met,
> - # reset $git_avatar to the empty string.
> + # check that the avatar feature is set to a known provider
> + # name, if the provider name is invalid, reset $git_avatar to
> + # the empty string.
Comma splice. Formatting as sentences would make this easier to read,
anyway:
# Check that the avatar feature is set to a known provider name.
# If the provider name is invalid, reset $git_avatar to the empty
# string.
Even better would be to remove the comment altogether. The code is
clear enough on its own and the comment should not be needed now that
it is a one-liner.
[...]
> @@ -2165,6 +2157,7 @@ sub picon_url {
> sub gravatar_url {
> my $email = lc shift;
> my $size = shift;
> + require Digest::MD5;
Same question as the previous patch: how do we decide whether to 'use'
or to 'require' in cases like this?
Thanks,
Jonathan
