{"thread":{"id":"46996","subject":"[PATCH] use filetest pragma to work with ACL","startedAt":"2017-10-18T19:55:37Z","lastAt":"2017-10-24T05:12:38Z","messageCount":6,"participants":["Guillaume Castagnino","Stefan Beller","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"330607","messageId":"0102015f310e24b9-b96378f3-a029-4110-80dd-e454522e2cb7-000000@eu-west-1.amazonses.com","threadId":"46996","inReplyTo":null,"subject":"[PATCH] use filetest pragma to work with ACL","fromName":"Guillaume Castagnino","fromEmail":"casta+github@xwing.info","sentAt":"2017-10-18T19:55:31Z","receivedAt":"2017-10-18T19:55:37Z","isPatch":true,"sender":{"key":"casta+github@xwing.info","avatar":null},"body":"From: Guillaume Castagnino <casta@xwing.info>\n\nas 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.\nthis commit handle ACL too\n---\n gitweb/gitweb.perl | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 9208f42ed1753..0ee7f304ce2b1 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -3072,6 +3072,7 @@ sub git_get_projects_list {\n \t\t\t\t# only directories can be git repositories\n \t\t\t\treturn unless (-d $_);\n \t\t\t\t# need search permission\n+\t\t\t\tuse filetest 'access';\n \t\t\t\treturn unless (-x $_);\n \t\t\t\t# don't traverse too deep (Find is super slow on os x)\n \t\t\t\t# $project_maxdepth excludes depth of $projectroot\n\n--\nhttps://github.com/git/git/pull/416\n"},{"id":"330610","messageId":"CAGZ79kZG=nk+U815mWhSMDHqa=UHSwCc_PbQcaTRBA8DBKoWrw@mail.gmail.com","threadId":"46996","inReplyTo":"0102015f310e24b9-b96378f3-a029-4110-80dd-e454522e2cb7-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH] use filetest pragma to work with ACL","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-18T20:05:08Z","receivedAt":"2017-10-18T20:05:14Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Oct 18, 2017 at 12:55 PM, Guillaume Castagnino\n<casta+github@xwing.info> wrote:\n> From: Guillaume Castagnino <casta@xwing.info>\n>\n> 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.\n> this commit handle ACL too\n\nThanks for contributing to Git!\n\nPlease see Documentation/SubmittingPatches for details,\ntl;dr:\n* If you can legally agree with\n   https://developercertificate.org/\n   add a line \"Signed-off-by: NAME <email>\"\n* Please give a more descriptive commit message.\n  Usually we phrase the commit subject as\n  \"area: do thing\", you have the \"do thing\" part,\n  but the area is unclear. Maybe:\n      gitweb: use filetest to allow ACLs\n\n* Keep the message text roughly at 70 characters per line,\n  as that is easier to read in e.g. git-show.\n\n* Instead of linking to github, we usually only refer to the commit, e.g.\n\n   In commit 46a1385 (gitweb: skip unreadable subdirectories, 2017-07-18)\n   we forgot to handle non-unix ACLs as well. Fix this.\n\n* Do we need a test/documentation for this?\n\nThanks,\nStefan\n\n> ---\n>  gitweb/gitweb.perl | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 9208f42ed1753..0ee7f304ce2b1 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -3072,6 +3072,7 @@ sub git_get_projects_list {\n>                                 # only directories can be git repositories\n>                                 return unless (-d $_);\n>                                 # need search permission\n> +                               use filetest 'access';\n>                                 return unless (-x $_);\n>                                 # don't traverse too deep (Find is super slow on os x)\n>                                 # $project_maxdepth excludes depth of $projectroot\n>\n> --\n> https://github.com/git/git/pull/416\n"},{"id":"330612","messageId":"20171018212451.goqxu4qq6aqe4tpl@sigill.intra.peff.net","threadId":"46996","inReplyTo":"0102015f310e24b9-b96378f3-a029-4110-80dd-e454522e2cb7-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH] use filetest pragma to work with ACL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-18T21:24:51Z","receivedAt":"2017-10-18T21:24:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 18, 2017 at 07:55:31PM +0000, Guillaume Castagnino wrote:\n\n> From: Guillaume Castagnino <casta@xwing.info>\n> [...]\n\nStefan raised a few meta issues, all of which I agree with. But I had\nsome questions about the patch itself:\n\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 9208f42ed1753..0ee7f304ce2b1 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -3072,6 +3072,7 @@ sub git_get_projects_list {\n>  \t\t\t\t# only directories can be git repositories\n>  \t\t\t\treturn unless (-d $_);\n>  \t\t\t\t# need search permission\n> +\t\t\t\tuse filetest 'access';\n>  \t\t\t\treturn unless (-x $_);\n\nThis \"use\" will unconditionally at compile-time (such as \"compile\" is\nfor perl, anyway). Which raises a few questions:\n\n  - would we want to use \"require\" instead to avoid loading when we\n    don't enter this function?\n\n  - If the answer to the above is \"no\" (e.g., because we basically\n    always need it; I didn't check), should it go at the top of the\n    script with the other \"use\" directives?\n\n    I think this is a scoped pragma, so what you have here affects only\n    this particular \"-x\". But wouldn't other uses of \"-x\" potentially\n    want the same benefit?\n\n  - Do all relevant versions of perl ship with filetest? According to\n    Module::Corelist, it first shipped with perl 5.6. In general I think\n    we treat that as a minimum for our perl scripts, though I do notice\n    that the gitweb script says \"use 5.008\". I'm not sure how realistic\n    that is.\n\n    If we can't count on it everywhere, then we probably need to wrap it\n    in an eval, and fall back to the existing \"-x\".\n\n-Peff\n"},{"id":"330629","messageId":"1508399608.4529.10.camel@xwing.info","threadId":"46996","inReplyTo":"20171018212451.goqxu4qq6aqe4tpl@sigill.intra.peff.net","subject":"Re: [PATCH] use filetest pragma to work with ACL","fromName":"Guillaume Castagnino","fromEmail":"casta@xwing.info","sentAt":"2017-10-19T07:53:28Z","receivedAt":"2017-10-19T08:01:49Z","isPatch":true,"sender":{"key":"casta@xwing.info","avatar":null},"body":"Hi,\n\nLe mercredi 18 octobre 2017 à 17:24 -0400, Jeff King a écrit :\n> On Wed, Oct 18, 2017 at 07:55:31PM +0000, Guillaume Castagnino wrote:\n> \n> > From: Guillaume Castagnino <casta@xwing.info>\n> > [...]\n> \n> Stefan raised a few meta issues, all of which I agree with. But I had\n> some questions about the patch itself:\n> \n> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> > index 9208f42ed1753..0ee7f304ce2b1 100755\n> > --- a/gitweb/gitweb.perl\n> > +++ b/gitweb/gitweb.perl\n> > @@ -3072,6 +3072,7 @@ sub git_get_projects_list {\n> >  \t\t\t\t# only directories can be git\n> > repositories\n> >  \t\t\t\treturn unless (-d $_);\n> >  \t\t\t\t# need search permission\n> > +\t\t\t\tuse filetest 'access';\n> >  \t\t\t\treturn unless (-x $_);\n> \n> This \"use\" will unconditionally at compile-time (such as \"compile\" is\n> for perl, anyway). Which raises a few questions:\n> \n>   - would we want to use \"require\" instead to avoid loading when we\n>     don't enter this function?\n\nI was under the impression that as this is a pragma affecting perl\n“compiler”, you have to always use “use”, not require, as the pragma\ninitialisation has to be done before code is run.\n\n>   - If the answer to the above is \"no\" (e.g., because we basically\n>     always need it; I didn't check), should it go at the top of the\n>     script with the other \"use\" directives?\n> \n>     I think this is a scoped pragma, so what you have here affects\n> only\n>     this particular \"-x\". But wouldn't other uses of \"-x\" potentially\n>     want the same benefit?\n\nI quickly grepped the code, I did not see other calls that could\nbenefits from the pragma, but it could be indeed nice to move it at the\ntop to avoid future issues with such tests and POSIX ACL.\n\n>   - Do all relevant versions of perl ship with filetest? According to\n>     Module::Corelist, it first shipped with perl 5.6. In general I\n> think\n>     we treat that as a minimum for our perl scripts, though I do\n> notice\n>     that the gitweb script says \"use 5.008\". I'm not sure how\n> realistic\n>     that is.\n\nSo the CGI already requires perl 5.8, so it’s fine I think.\n\nAttached a cleanup proposal and moving the use at the top.\n\nThanks\nGuillaume\n\n-- \nGuillaume Castagnino\n    casta@xwing.info / guillaume@castagnino.org\n\nFrom 4d5a082970063b34d3dbdf5b9a577624310057d6 Mon Sep 17 00:00:00 2001\nFrom: Guillaume Castagnino <casta@xwing.info>\nDate: Thu, 19 Oct 2017 09:32:46 +0200\nSubject: [PATCH] gitweb: use filetest to allow ACLs\n\nIn commit 46a1385 (gitweb: skip unreadable subdirectories, 2017-07-18)\nwe forgot to handle non-unix ACLs as well. Fix this.\n\nSigned-off-by: Guillaume Castagnino <casta@xwing.info>\n---\n gitweb/gitweb.perl | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 9208f42ed..6ac49eaf3 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -10,6 +10,8 @@\n use 5.008;\n use strict;\n use warnings;\n+# handle ACL in file access tests\n+use filetest 'access';\n use CGI qw(:standard :escapeHTML -nosticky);\n use CGI::Util qw(unescape);\n use CGI::Carp qw(fatalsToBrowser set_message);\n-- \n2.14.2\n\n"},{"id":"330647","messageId":"20171019161344.zmwgb2a2rziorffm@sigill.intra.peff.net","threadId":"46996","inReplyTo":"1508399608.4529.10.camel@xwing.info","subject":"Re: [PATCH] use filetest pragma to work with ACL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-19T16:13:44Z","receivedAt":"2017-10-19T16:13:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 19, 2017 at 09:53:28AM +0200, Guillaume Castagnino wrote:\n\n> > This \"use\" will unconditionally at compile-time (such as \"compile\" is\n> > for perl, anyway). Which raises a few questions:\n> > \n> >   - would we want to use \"require\" instead to avoid loading when we\n> >     don't enter this function?\n> \n> I was under the impression that as this is a pragma affecting perl\n> “compiler”, you have to always use “use”, not require, as the pragma\n> initialisation has to be done before code is run.\n\nYeah, I think you're right.\n\n> I quickly grepped the code, I did not see other calls that could\n> benefits from the pragma, but it could be indeed nice to move it at the\n> top to avoid future issues with such tests and POSIX ACL.\n\nMakes sense.\n\n> >   - Do all relevant versions of perl ship with filetest? According to\n> >     Module::Corelist, it first shipped with perl 5.6. In general I\n> > think\n> >     we treat that as a minimum for our perl scripts, though I do\n> > notice\n> >     that the gitweb script says \"use 5.008\". I'm not sure how\n> > realistic\n> >     that is.\n> \n> So the CGI already requires perl 5.8, so it’s fine I think.\n\nRight, I totally forgot about perl's funny version-numbering scheme.\n\n> Attached a cleanup proposal and moving the use at the top.\n\nThanks, it looks good to me.\n\nFor future reference, we usually prefer patches inline, separated by\n\"-- >8 --\" scissors if there's other material (like your reply).\n\n-Peff\n"},{"id":"330894","messageId":"xmqqtvypkujv.fsf@gitster.mtv.corp.google.com","threadId":"46996","inReplyTo":"20171019161344.zmwgb2a2rziorffm@sigill.intra.peff.net","subject":"Re: [PATCH] use filetest pragma to work with ACL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-24T05:02:12Z","receivedAt":"2017-10-24T05:12:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> Attached a cleanup proposal and moving the use at the top.\n>\n> Thanks, it looks good to me.\n\nI somehow missed this exchange; sorry for being late to pick it up.\n"}]}