Re: [PATCH] use filetest pragma to work with ACL

From: Jeff King <peff@peff.net>
Date: Oct 18, 2017, 21:24 UTC
Message-ID: <20171018212451.goqxu4qq6aqe4tpl@sigill.intra.peff.net>
In-Reply-To: <0102015f310e24b9-b96378f3-a029-4110-80dd-e454522e2cb7-000000@eu-west-1.amazonses.com>

On Wed, Oct 18, 2017 at 07:55:31PM +0000, Guillaume Castagnino wrote:

> From: Guillaume Castagnino <casta@xwing.info>
> [...]

Stefan raised a few meta issues, all of which I agree with. But I had some questions about the patch itself:

Show 10 quoted lines

> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 9208f42ed1753..0ee7f304ce2b1 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3072,6 +3072,7 @@ sub git_get_projects_list {
>  				# only directories can be git repositories
>  				return unless (-d $_);
>  				# need search permission
> +				use filetest 'access';
>  				return unless (-x $_);

This "use" will unconditionally at compile-time (such as "compile" is for perl, anyway). Which raises a few questions:

  - would we want to use "require" instead to avoid loading when we
    don't enter this function?

  - If the answer to the above is "no" (e.g., because we basically
    always need it; I didn't check), should it go at the top of the
    script with the other "use" directives?

    I think this is a scoped pragma, so what you have here affects only
    this particular "-x". But wouldn't other uses of "-x" potentially
    want the same benefit?

  - Do all relevant versions of perl ship with filetest? According to
    Module::Corelist, it first shipped with perl 5.6. In general I think
    we treat that as a minimum for our perl scripts, though I do notice
    that the gitweb script says "use 5.008". I'm not sure how realistic
    that is.

    If we can't count on it everywhere, then we probably need to wrap it
    in an eval, and fall back to the existing "-x".

-Peff