{"thread":{"id":"16719","subject":"[PATCH 1/2] gitweb: allow access to forks with strict_export","startedAt":"2008-12-13T21:16:54Z","lastAt":"2008-12-20T00:35:48Z","messageCount":7,"participants":["Matt McCutchen","Jakub Narebski","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"97815","messageId":"1229203014.31181.7.camel@mattlaptop2.local","threadId":"16719","inReplyTo":null,"subject":"[PATCH 1/2] gitweb: allow access to forks with strict_export","fromName":"Matt McCutchen","fromEmail":"matt@mattmccutchen.net","sentAt":"2008-12-13T21:16:54Z","receivedAt":"2008-12-13T21:16:54Z","isPatch":true,"sender":{"key":"matt@mattmccutchen.net","avatar":"https://avatars.githubusercontent.com/u/8885753?v=4"},"body":"git_get_projects_list excludes forks in order to unclutter the main\nproject list, but this caused the strict_export check, which also relies\non git_get_project_list, to incorrectly fail for forks.  This patch adds\nan argument so git_get_projects_list knows when it is being called for a\nstrict_export check (as opposed to a user-visible project list) and\ndoesn't exclude the forks.\n\nSigned-off-by: Matt McCutchen <matt@mattmccutchen.net>\n---\n gitweb/gitweb.perl |    7 ++++---\n 1 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 86511cf..5357bcc 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1144,7 +1144,8 @@ sub untabify {\n \n sub project_in_list {\n \tmy $project = shift;\n-\tmy @list = git_get_projects_list();\n+\t# Tell git_get_projects_list to include forks.\n+\tmy @list = git_get_projects_list(undef, 1);\n \treturn @list && scalar(grep { $_->{'path'} eq $project } @list);\n }\n \n@@ -2128,13 +2129,13 @@ sub git_get_project_url_list {\n }\n \n sub git_get_projects_list {\n-\tmy ($filter) = @_;\n+\tmy ($filter, $for_strict_export) = @_;\n \tmy @list;\n \n \t$filter ||= '';\n \t$filter =~ s/\\.git$//;\n \n-\tmy $check_forks = gitweb_check_feature('forks');\n+\tmy $check_forks = !$for_strict_export && gitweb_check_feature('forks');\n \n \tif (-d $projects_list) {\n \t\t# search in directory\n-- \n1.6.1.rc2.27.gc7114\n"},{"id":"97819","messageId":"m3prjvg2st.fsf@localhost.localdomain","threadId":"16719","inReplyTo":"1229203014.31181.7.camel@mattlaptop2.local","subject":"Re: [PATCH 1/2] gitweb: allow access to forks with strict_export","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-13T21:53:26Z","receivedAt":"2008-12-13T21:53:26Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Matt McCutchen <matt@mattmccutchen.net> writes:\n\nCC-ed Petr Baudis, author of forks support in gitweb.\n\n> git_get_projects_list excludes forks in order to unclutter the main\n> project list, but this caused the strict_export check, which also relies\n> on git_get_project_list, to incorrectly fail for forks.  This patch adds\n> an argument so git_get_projects_list knows when it is being called for a\n> strict_export check (as opposed to a user-visible project list) and\n> doesn't exclude the forks.\n> \n> Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>\n\nLooks good for me.\n\nAcked-by: Jakub Narebski <jnareb@gmail.com>\n\n> ---\n>  gitweb/gitweb.perl |    7 ++++---\n>  1 files changed, 4 insertions(+), 3 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 86511cf..5357bcc 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -1144,7 +1144,8 @@ sub untabify {\n>  \n>  sub project_in_list {\n>  \tmy $project = shift;\n> -\tmy @list = git_get_projects_list();\n> +\t# Tell git_get_projects_list to include forks.\n> +\tmy @list = git_get_projects_list(undef, 1);\n>  \treturn @list && scalar(grep { $_->{'path'} eq $project } @list);\n>  }\n>  \n> @@ -2128,13 +2129,13 @@ sub git_get_project_url_list {\n>  }\n>  \n>  sub git_get_projects_list {\n> -\tmy ($filter) = @_;\n> +\tmy ($filter, $for_strict_export) = @_;\n>  \tmy @list;\n>  \n>  \t$filter ||= '';\n>  \t$filter =~ s/\\.git$//;\n>  \n> -\tmy $check_forks = gitweb_check_feature('forks');\n> +\tmy $check_forks = !$for_strict_export && gitweb_check_feature('forks');\n>  \n>  \tif (-d $projects_list) {\n>  \t\t# search in directory\n> -- \n> 1.6.1.rc2.27.gc7114\n> \n> \n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"97825","messageId":"7vr64b4sib.fsf@gitster.siamese.dyndns.org","threadId":"16719","inReplyTo":"m3prjvg2st.fsf@localhost.localdomain","subject":"Re: [PATCH 1/2] gitweb: allow access to forks with strict_export","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-13T22:31:08Z","receivedAt":"2008-12-13T22:31:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> Matt McCutchen <matt@mattmccutchen.net> writes:\n>\n> CC-ed Petr Baudis, author of forks support in gitweb.\n>\n>> git_get_projects_list excludes forks in order to unclutter the main\n>> project list, but this caused the strict_export check, which also relies\n>> on git_get_project_list, to incorrectly fail for forks.  This patch adds\n>> an argument so git_get_projects_list knows when it is being called for a\n>> strict_export check (as opposed to a user-visible project list) and\n>> doesn't exclude the forks.\n>>\n>> Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>\n>\n> Looks good for me.\n\nThat sounds like a broken API to me.\n\nAt least, please have the decency to not call the extra parameter \"for\nstrict export\".  I would understand it if the extra parameter is called\n\"toplevel_only\" (or its negation, \"include_forks\").\n\nIOW, don't name a parameter after the name of one caller that happens to\nwant an unspecified special semantics, without saying what that special\nsemantics is.  Instead, name it after the special semantics that the\nargument triggers.\n"},{"id":"97827","messageId":"200812132351.37420.jnareb@gmail.com","threadId":"16719","inReplyTo":"7vr64b4sib.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] gitweb: allow access to forks with strict_export","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-13T22:51:33Z","receivedAt":"2008-12-13T22:51:33Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n> Jakub Narebski <jnareb@gmail.com> writes:\n> > Matt McCutchen <matt@mattmccutchen.net> writes:\n>>\n>> CC-ed Petr Baudis, author of forks support in gitweb.\n>>\n>>> git_get_projects_list excludes forks in order to unclutter the main\n>>> project list, but this caused the strict_export check, which also relies\n>>> on git_get_project_list, to incorrectly fail for forks.  This patch adds\n>>> an argument so git_get_projects_list knows when it is being called for a\n>>> strict_export check (as opposed to a user-visible project list) and\n>>> doesn't exclude the forks.\n>>>\n>>> Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>\n>>\n>> Looks good for me.\n> \n> That sounds like a broken API to me.\n> \n> At least, please have the decency to not call the extra parameter \"for\n> strict export\".  I would understand it if the extra parameter is called\n> \"toplevel_only\" (or its negation, \"include_forks\").\n> \n> IOW, don't name a parameter after the name of one caller that happens to\n> want an unspecified special semantics, without saying what that special\n> semantics is.  Instead, name it after the special semantics that the\n> argument triggers.\n \nAhhh... true. \n\n\"no_hide\" (currently \"include_forks\") allows us to _not_ passing this\nparameter in other places than project_in_list(); undef is falsy.\n\nBy the way, doesn't git_project_index and perhaps git_opml also need\nthis parameter passed to git_get_projects_list?\n\nThen patch subject would change...\n-- \nJakub Narebski\nPoland\n"},{"id":"97834","messageId":"1229219475.3360.51.camel@mattlaptop2.local","threadId":"16719","inReplyTo":"7vr64b4sib.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] gitweb: allow access to forks with strict_export","fromName":"Matt McCutchen","fromEmail":"matt@mattmccutchen.net","sentAt":"2008-12-14T01:51:15Z","receivedAt":"2008-12-14T01:51:15Z","isPatch":true,"sender":{"key":"matt@mattmccutchen.net","avatar":"https://avatars.githubusercontent.com/u/8885753?v=4"},"body":"On Sat, 2008-12-13 at 14:31 -0800, Junio C Hamano wrote:\n> Jakub Narebski <jnareb@gmail.com> writes:\n> \n> > Matt McCutchen <matt@mattmccutchen.net> writes:\n> >\n> > CC-ed Petr Baudis, author of forks support in gitweb.\n> >\n> >> git_get_projects_list excludes forks in order to unclutter the main\n> >> project list, but this caused the strict_export check, which also relies\n> >> on git_get_project_list, to incorrectly fail for forks.  This patch adds\n> >> an argument so git_get_projects_list knows when it is being called for a\n> >> strict_export check (as opposed to a user-visible project list) and\n> >> doesn't exclude the forks.\n> >>\n> >> Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>\n> >\n> > Looks good for me.\n> \n> That sounds like a broken API to me.\n> \n> At least, please have the decency to not call the extra parameter \"for\n> strict export\".  I would understand it if the extra parameter is called\n> \"toplevel_only\" (or its negation, \"include_forks\").\n> \n> IOW, don't name a parameter after the name of one caller that happens to\n> want an unspecified special semantics, without saying what that special\n> semantics is.  Instead, name it after the special semantics that the\n> argument triggers.\n\nI disagree.  The parameter is really \"include forks (if there is such a\nconcept under the current config)\", and with my second patch, it becomes\n\"include hidden projects\" too.  That's really unwieldy.\n\nIn my view, the parameter makes the distinction between generating a\nfiltered list for user consumption and a list of everything for a\nstrict_export check.  The particular semantics it activates may evolve\nas gitweb does (case in point: my second patch).  The current semantics\ncan be described in a comment on git_get_projects_list.\n\nGranted, there may be a better name for the parameter than\n$for_strict_export.  How about $include_all?\n\n-- \nMatt\n"},{"id":"97836","messageId":"1229220398.3360.66.camel@mattlaptop2.local","threadId":"16719","inReplyTo":"200812132351.37420.jnareb@gmail.com","subject":"Re: [PATCH 1/2] gitweb: allow access to forks with strict_export","fromName":"Matt McCutchen","fromEmail":"matt@mattmccutchen.net","sentAt":"2008-12-14T02:06:38Z","receivedAt":"2008-12-14T02:06:38Z","isPatch":true,"sender":{"key":"matt@mattmccutchen.net","avatar":"https://avatars.githubusercontent.com/u/8885753?v=4"},"body":"On Sat, 2008-12-13 at 23:51 +0100, Jakub Narebski wrote:\n> \"no_hide\" (currently \"include_forks\") allows us to _not_ passing this\n> parameter in other places than project_in_list(); undef is falsy.\n\nRight.  That's why I made the current parameter $for_strict_export (so\nonly project_in_list passes it) rather than the negation.\n\n> By the way, doesn't git_project_index and perhaps git_opml also need\n> this parameter passed to git_get_projects_list?\n\nYes, now that you mention it, I suppose they should show forks, though\nnot hidden repositories.  Then git_get_projects_list can be called in\nthree different modes: include everything (project_in_list), include\nforks but not hidden (git_get_project_index and git_opml), or include\nneither forks nor hidden (git_project_list).  Should we have two\nseparate parameters to git_get_projects_list or a single three-valued\none?\n\nThat raises another point.  I was going to change git_get_projects_list\nso that forks of a hidden project that are not themselves hidden appear\non the parent project's page but not in the main project list.  This\nway, users who know about the parent project can navigate to the fork,\nbut the fork does not give away the existence of the parent project by\nappearing in the main list.  Then I guess git_project_index and git_opml\nshould omit forks of hidden projects, meaning that some fork-checking\nstill has to take place with \"include forks\" on but \"include hidden\"\noff.  This will make git_get_projects_list somewhat more complex but not\nunmanageably so, and I do think it's the behavior we want.\n\nI will send an updated patch.\n\n-- \nMatt\n"},{"id":"98401","messageId":"200812200135.51151.jnareb@gmail.com","threadId":"16719","inReplyTo":"1229220398.3360.66.camel@mattlaptop2.local","subject":"Re: [PATCH 1/2] gitweb: allow access to forks with strict_export","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-20T00:35:48Z","receivedAt":"2008-12-20T00:35:48Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sun, 14 Dec 2008 03:06, Matt McCutchen napisał:\n> On Sat, 2008-12-13 at 23:51 +0100, Jakub Narebski wrote:\n> >\n> > \"no_hide\" (currently \"include_forks\") allows us to _not_ passing this\n> > parameter in other places than project_in_list(); undef is falsy.\n> \n> Right.  That's why I made the current parameter $for_strict_export (so\n> only project_in_list passes it) rather than the negation.\n\nStill I think $for_strict_export is singularly _bad_ name for\na parameter. $no_hide or $show_all would be much, much better.\n\n> > By the way, doesn't git_project_index and perhaps git_opml also need\n> > this parameter passed to git_get_projects_list?\n> \n> Yes, now that you mention it, I suppose they should show forks, though\n> not hidden repositories.  Then git_get_projects_list can be called in\n> three different modes: include everything (project_in_list), include\n> forks but not hidden (git_get_project_index and git_opml), or include\n> neither forks nor hidden (git_project_list).  Should we have two\n> separate parameters to git_get_projects_list or a single three-valued\n> one?\n\nBy the way, I am not sure if your idea of \"hidden projects\" and projects\nlist (projects index) file.\n\nWe have two way of specifying list of projects. One is scanning\nprojectroot directory for something that looks like git repository,\nthe other is having projects list file with project paths and project\nowners.\n\nIf you use projects list file, you can have projects which are in not\non project list, but are accessible in filesystem. Those projects\n(repositories) are hidden, i.e. they are not visible on projects_list\npage, but still you can access a project. If you want to have projects\nwhich are not on list to be not accessible at all, and not only hidden,\nyou have to use $strict_export... using which makes access to repo\nviews a tiny bit slower (and _only_ slows down gitweb for when scanning\ndirectories for projects).\n\nBut why would one _want_ projects which are not visible, but still\naccessible; \"hidden\" projects? Moreso what you want, the three class\nof projects inside $projectroot: visible, hidden, and not exported.\n\n\nWith newly added $export_auth_hook you can limit accessibility of\nprojects in other ways, for example using .htaccess control files of\nApache web server.  Check gitweb/README (or gitweb/INSTALL) for\nexample.\n\n> That raises another point.  I was going to change git_get_projects_list\n> so that forks of a hidden project that are not themselves hidden appear\n> on the parent project's page but not in the main project list.  This\n> way, users who know about the parent project can navigate to the fork,\n> but the fork does not give away the existence of the parent project by\n> appearing in the main list.  Then I guess git_project_index and git_opml\n> should omit forks of hidden projects, meaning that some fork-checking\n> still has to take place with \"include forks\" on but \"include hidden\"\n> off.  This will make git_get_projects_list somewhat more complex but not\n> unmanageably so, and I do think it's the behavior we want.\n> \n> I will send an updated patch.\n\nSo you see that having explicitly hidden files have yet another\ncomplication. I wonder if this is really worth it...\n\n-- \nJakub Narebski\nPoland\n"}]}