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

From: Jeff King <peff@peff.net>
Date: Oct 19, 2017, 16:13 UTC
Message-ID: <20171019161344.zmwgb2a2rziorffm@sigill.intra.peff.net>
In-Reply-To: <1508399608.4529.10.camel@xwing.info>

On Thu, Oct 19, 2017 at 09:53:28AM +0200, Guillaume Castagnino wrote:

Show 9 quoted lines

> > 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?
> 
> I was under the impression that as this is a pragma affecting perl
> “compiler”, you have to always use “use”, not require, as the pragma
> initialisation has to be done before code is run.

Yeah, I think you're right.

> I quickly grepped the code, I did not see other calls that could
> benefits from the pragma, but it could be indeed nice to move it at the
> top to avoid future issues with such tests and POSIX ACL.

Makes sense.

Show 10 quoted lines

> >   - 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.
> 
> So the CGI already requires perl 5.8, so it’s fine I think.

Right, I totally forgot about perl's funny version-numbering scheme.

> Attached a cleanup proposal and moving the use at the top.

Thanks, it looks good to me.

For future reference, we usually prefer patches inline, separated by "-- >8 --" scissors if there's other material (like your reply).

-Peff