[PATCH] use filetest pragma to work with ACL

[PATCH] use filetest pragma to work with ACL

[Guillaume Castagnino]

From: Guillaume Castagnino <casta@xwing.info>

as stated in comment in https://github.com/git/git/commit/46a13857fc036b54ac2ddd0a218e5cc171aa7bd9#diff-00703a794a540acf45e225abd6aeda3b the referenced commit is broken when using ACL and not basic UNIX rights.
this commit handle ACL too
---
 gitweb/gitweb.perl | 1 +
 1 file changed, 1 insertion(+)

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 $_);
 				# don't traverse too deep (Find is super slow on os x)
 				# $project_maxdepth excludes depth of $projectroot

--
https://github.com/git/git/pull/416

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

[Stefan Beller]

On Wed, Oct 18, 2017 at 12:55 PM, Guillaume Castagnino
<casta+github@xwing.info> wrote:
> From: Guillaume Castagnino <casta@xwing.info>
>
> as stated in comment in https://github.com/git/git/commit/46a13857fc036b54ac2ddd0a218e5cc171aa7bd9#diff-00703a794a540acf45e225abd6aeda3b the referenced commit is broken when using ACL and not basic UNIX rights.
> this commit handle ACL too

Thanks for contributing to Git!

Please see Documentation/SubmittingPatches for details,
tl;dr:
* If you can legally agree with
   https://developercertificate.org/
   add a line "Signed-off-by: NAME <email>"
* Please give a more descriptive commit message.
  Usually we phrase the commit subject as
  "area: do thing", you have the "do thing" part,
  but the area is unclear. Maybe:
      gitweb: use filetest to allow ACLs

* Keep the message text roughly at 70 characters per line,
  as that is easier to read in e.g. git-show.

* Instead of linking to github, we usually only refer to the commit, e.g.

   In commit 46a1385 (gitweb: skip unreadable subdirectories, 2017-07-18)
   we forgot to handle non-unix ACLs as well. Fix this.

* Do we need a test/documentation for this?

Thanks,
Stefan

> ---
>  gitweb/gitweb.perl | 1 +
>  1 file changed, 1 insertion(+)
>
> 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 $_);
>                                 # don't traverse too deep (Find is super slow on os x)
>                                 # $project_maxdepth excludes depth of $projectroot
>
> --
> https://github.com/git/git/pull/416

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

[Jeff King]

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:

> 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

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

[Guillaume Castagnino]

[-- Attachment #1: Type: text/plain, Size: 2245 bytes --]

Hi,

Le mercredi 18 octobre 2017 à 17:24 -0400, Jeff King a écrit :
> 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:
> 
> > 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?

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.

>   - 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?

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.

>   - 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.

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

Thanks
Guillaume

-- 
Guillaume Castagnino
    casta@xwing.info / guillaume@castagnino.org

[-- Attachment #2: 0001-gitweb-use-filetest-to-allow-ACLs.patch --]
[-- Type: text/x-patch, Size: 842 bytes --]

From 4d5a082970063b34d3dbdf5b9a577624310057d6 Mon Sep 17 00:00:00 2001
From: Guillaume Castagnino <casta@xwing.info>
Date: Thu, 19 Oct 2017 09:32:46 +0200
Subject: [PATCH] gitweb: use filetest to allow ACLs

In commit 46a1385 (gitweb: skip unreadable subdirectories, 2017-07-18)
we forgot to handle non-unix ACLs as well. Fix this.

Signed-off-by: Guillaume Castagnino <casta@xwing.info>
---
 gitweb/gitweb.perl | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 9208f42ed..6ac49eaf3 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -10,6 +10,8 @@
 use 5.008;
 use strict;
 use warnings;
+# handle ACL in file access tests
+use filetest 'access';
 use CGI qw(:standard :escapeHTML -nosticky);
 use CGI::Util qw(unescape);
 use CGI::Carp qw(fatalsToBrowser set_message);
-- 
2.14.2

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

[Jeff King]

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

> > 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.

> >   - 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> Attached a cleanup proposal and moving the use at the top.
>
> Thanks, it looks good to me.

I somehow missed this exchange; sorry for being late to pick it up.