Re: [PATCH 7/8] gitweb: hard-depend on the Digest::MD5 5.8 module
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Feb 14, 2018, 23:52 UTC
- Message-ID
- <20180214235256.GG136185@aiede.svl.corp.google.com>
- In-Reply-To
- <20180214222146.10655-8-avarab@gmail.com>
Ævar Arnfjörð Bjarmason wrote:
Show 5 quoted lines
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@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
[...]
Show 11 quoted lines
> @@ -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.
[...]
Show 5 quoted lines
> @@ -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