# [PATCH] use filetest pragma to work with ACL 6 messages from 2017-10-18 to 2017-10-24. Participants: Guillaume Castagnino, Stefan Beller, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/46996 ## Guillaume Castagnino, 2017-10-18 19:55 Subject: [PATCH] use filetest pragma to work with ACL Message-ID: <0102015f310e24b9-b96378f3-a029-4110-80dd-e454522e2cb7-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/0102015f310e24b9-b96378f3-a029-4110-80dd-e454522e2cb7-000000%40eu-west-1.amazonses.com ``` From: Guillaume Castagnino 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 ``` ## Stefan Beller, 2017-10-18 20:05 Subject: Re: [PATCH] use filetest pragma to work with ACL Message-ID: URL: https://gitlist.dev/e/CAGZ79kZG%3Dnk%2BU815mWhSMDHqa%3DUHSwCc_PbQcaTRBA8DBKoWrw%40mail.gmail.com In-Reply-To: <0102015f310e24b9-b96378f3-a029-4110-80dd-e454522e2cb7-000000@eu-west-1.amazonses.com> ``` On Wed, Oct 18, 2017 at 12:55 PM, Guillaume Castagnino wrote: > From: Guillaume Castagnino > > 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 " * 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 ``` ## Jeff King, 2017-10-18 21:24 Subject: Re: [PATCH] use filetest pragma to work with ACL Message-ID: <20171018212451.goqxu4qq6aqe4tpl@sigill.intra.peff.net> URL: https://gitlist.dev/e/20171018212451.goqxu4qq6aqe4tpl%40sigill.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 > [...] 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 ``` ## Guillaume Castagnino, 2017-10-19 07:53 Subject: Re: [PATCH] use filetest pragma to work with ACL Message-ID: <1508399608.4529.10.camel@xwing.info> URL: https://gitlist.dev/e/1508399608.4529.10.camel%40xwing.info In-Reply-To: <20171018212451.goqxu4qq6aqe4tpl@sigill.intra.peff.net> ``` 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 > > [...] > > 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 From 4d5a082970063b34d3dbdf5b9a577624310057d6 Mon Sep 17 00:00:00 2001 From: Guillaume Castagnino 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 --- 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 ``` ## Jeff King, 2017-10-19 16:13 Subject: Re: [PATCH] use filetest pragma to work with ACL Message-ID: <20171019161344.zmwgb2a2rziorffm@sigill.intra.peff.net> URL: https://gitlist.dev/e/20171019161344.zmwgb2a2rziorffm%40sigill.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: > > 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 ``` ## Junio C Hamano, 2017-10-24 05:02 Subject: Re: [PATCH] use filetest pragma to work with ACL Message-ID: URL: https://gitlist.dev/e/xmqqtvypkujv.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20171019161344.zmwgb2a2rziorffm@sigill.intra.peff.net> ``` Jeff King 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. ```