{"thread":{"id":"16716","subject":"[PATCH] gitweb: Add option to put a trailing slash on pathinfo-style project URLs","startedAt":"2008-12-13T19:10:21Z","lastAt":"2008-12-14T23:58:34Z","messageCount":11,"participants":["Matt McCutchen","Jakub Narebski","Giuseppe Bilotta","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"97806","messageId":"1229195421.3943.8.camel@mattlaptop2.local","threadId":"16716","inReplyTo":null,"subject":"[PATCH] gitweb: Add option to put a trailing slash on pathinfo-style project URLs","fromName":"Matt McCutchen","fromEmail":"matt@mattmccutchen.net","sentAt":"2008-12-13T19:10:21Z","receivedAt":"2008-12-13T19:10:21Z","isPatch":true,"sender":{"key":"matt@mattmccutchen.net","avatar":"https://avatars.githubusercontent.com/u/8885753?v=4"},"body":"My Web site uses pathinfo mode and some rewrite magic to show the gitweb\ninterface at the URL of the real repository directory (which users also\npull from).  In this case, it's desirable to end generated links to the\nproject in a trailing slash so the Web server doesn't have to redirect\nthe client to add the slash.  This patch adds a second element to the\n\"pathinfo\" feature configuration to control the trailing slash.\n---\n\nWhat do you think of this?  I've been using it on my Web site for a\nwhile now.\n\nMatt\n\n gitweb/gitweb.perl |   28 ++++++++++++++++++++++------\n 1 files changed, 22 insertions(+), 6 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 6eb370d..86511cf 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -270,6 +270,11 @@ our %feature = (\n \t# $feature{'pathinfo'}{'default'} = [1];\n \t# Project specific override is not supported.\n \n+\t# If you want a trailing slash on the project path (because, for\n+\t# example, you have a real directory at that URL and are using\n+\t# some rewrite magic to invoke gitweb), then set:\n+\t# $feature{'pathinfo'}{'default'} = [1, 1];\n+\n \t# Note that you will need to change the default location of CSS,\n \t# favicon, logo and possibly other files to an absolute URL. Also,\n \t# if gitweb.cgi serves as your indexfile, you will need to force\n@@ -829,8 +834,8 @@ sub href (%) {\n \t\t}\n \t}\n \n-\tmy $use_pathinfo = gitweb_check_feature('pathinfo');\n-\tif ($use_pathinfo) {\n+\tmy @use_pathinfo = gitweb_get_feature('pathinfo');\n+\tif ($use_pathinfo[0]) {\n \t\t# try to put as many parameters as possible in PATH_INFO:\n \t\t#   - project name\n \t\t#   - action\n@@ -845,7 +850,12 @@ sub href (%) {\n \t\t$href =~ s,/$,,;\n \n \t\t# Then add the project name, if present\n-\t\t$href .= \"/\".esc_url($params{'project'}) if defined $params{'project'};\n+\t\tmy $proj_href = undef;\n+\t\tif (defined $params{'project'}) {\n+\t\t\t$href .= \"/\".esc_url($params{'project'});\n+\t\t\t# Save for trailing-slash check below.\n+\t\t\t$proj_href = $href;\n+\t\t}\n \t\tdelete $params{'project'};\n \n \t\t# since we destructively absorb parameters, we keep this\n@@ -903,6 +913,10 @@ sub href (%) {\n \t\t\t$href .= $known_snapshot_formats{$fmt}{'suffix'};\n \t\t\tdelete $params{'snapshot_format'};\n \t\t}\n+\n+\t\t# If requested in the configuration, add a trailing slash to a URL that\n+\t\t# has nothing appended after the project path.\n+\t\t$href .= '/' if ($use_pathinfo[1] && defined $proj_href && $href eq $proj_href);\n \t}\n \n \t# now encode the parameters explicitly\n@@ -2987,13 +3001,15 @@ EOF\n \t\t\t$search_hash = \"HEAD\";\n \t\t}\n \t\tmy $action = $my_uri;\n-\t\tmy $use_pathinfo = gitweb_check_feature('pathinfo');\n-\t\tif ($use_pathinfo) {\n+\t\tmy @use_pathinfo = gitweb_get_feature('pathinfo');\n+\t\tif ($use_pathinfo[0]) {\n \t\t\t$action .= \"/\".esc_url($project);\n+\t\t\t# Add a trailing slash if requested in the configuration.\n+\t\t\t$action .= '/' if ($use_pathinfo[1]);\n \t\t}\n \t\tprint $cgi->startform(-method => \"get\", -action => $action) .\n \t\t      \"<div class=\\\"search\\\">\\n\" .\n-\t\t      (!$use_pathinfo &&\n+\t\t      (!$use_pathinfo[0] &&\n \t\t      $cgi->input({-name=>\"p\", -value=>$project, -type=>\"hidden\"}) . \"\\n\") .\n \t\t      $cgi->input({-name=>\"a\", -value=>\"search\", -type=>\"hidden\"}) . \"\\n\" .\n \t\t      $cgi->input({-name=>\"h\", -value=>$search_hash, -type=>\"hidden\"}) . \"\\n\" .\n-- \n1.6.1.rc2.24.gf17b3c.dirty\n"},{"id":"97813","messageId":"1229202689.31181.1.camel@mattlaptop2.local","threadId":"16716","inReplyTo":"1229195421.3943.8.camel@mattlaptop2.local","subject":"[PATCH try 2] gitweb: Add option to put a trailing slash on pathinfo-style project URLs","fromName":"Matt McCutchen","fromEmail":"matt@mattmccutchen.net","sentAt":"2008-12-13T21:11:29Z","receivedAt":"2008-12-13T21:11:29Z","isPatch":true,"sender":{"key":"matt@mattmccutchen.net","avatar":"https://avatars.githubusercontent.com/u/8885753?v=4"},"body":"My Web site uses pathinfo mode and some rewrite magic to show the gitweb\ninterface at the URL of the real repository directory (which users also\npull from).  In this case, it's desirable to end generated links to the\nproject in a trailing slash so the Web server doesn't have to redirect\nthe client to add the slash.  This patch adds a second element to the\n\"pathinfo\" feature configuration to control the trailing slash.\n\nSigned-off-by: Matt McCutchen <matt@mattmccutchen.net>\n---\nResending with a sign-off.\n\n gitweb/gitweb.perl |   28 ++++++++++++++++++++++------\n 1 files changed, 22 insertions(+), 6 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 6eb370d..86511cf 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -270,6 +270,11 @@ our %feature = (\n \t# $feature{'pathinfo'}{'default'} = [1];\n \t# Project specific override is not supported.\n \n+\t# If you want a trailing slash on the project path (because, for\n+\t# example, you have a real directory at that URL and are using\n+\t# some rewrite magic to invoke gitweb), then set:\n+\t# $feature{'pathinfo'}{'default'} = [1, 1];\n+\n \t# Note that you will need to change the default location of CSS,\n \t# favicon, logo and possibly other files to an absolute URL. Also,\n \t# if gitweb.cgi serves as your indexfile, you will need to force\n@@ -829,8 +834,8 @@ sub href (%) {\n \t\t}\n \t}\n \n-\tmy $use_pathinfo = gitweb_check_feature('pathinfo');\n-\tif ($use_pathinfo) {\n+\tmy @use_pathinfo = gitweb_get_feature('pathinfo');\n+\tif ($use_pathinfo[0]) {\n \t\t# try to put as many parameters as possible in PATH_INFO:\n \t\t#   - project name\n \t\t#   - action\n@@ -845,7 +850,12 @@ sub href (%) {\n \t\t$href =~ s,/$,,;\n \n \t\t# Then add the project name, if present\n-\t\t$href .= \"/\".esc_url($params{'project'}) if defined $params{'project'};\n+\t\tmy $proj_href = undef;\n+\t\tif (defined $params{'project'}) {\n+\t\t\t$href .= \"/\".esc_url($params{'project'});\n+\t\t\t# Save for trailing-slash check below.\n+\t\t\t$proj_href = $href;\n+\t\t}\n \t\tdelete $params{'project'};\n \n \t\t# since we destructively absorb parameters, we keep this\n@@ -903,6 +913,10 @@ sub href (%) {\n \t\t\t$href .= $known_snapshot_formats{$fmt}{'suffix'};\n \t\t\tdelete $params{'snapshot_format'};\n \t\t}\n+\n+\t\t# If requested in the configuration, add a trailing slash to a URL that\n+\t\t# has nothing appended after the project path.\n+\t\t$href .= '/' if ($use_pathinfo[1] && defined $proj_href && $href eq $proj_href);\n \t}\n \n \t# now encode the parameters explicitly\n@@ -2987,13 +3001,15 @@ EOF\n \t\t\t$search_hash = \"HEAD\";\n \t\t}\n \t\tmy $action = $my_uri;\n-\t\tmy $use_pathinfo = gitweb_check_feature('pathinfo');\n-\t\tif ($use_pathinfo) {\n+\t\tmy @use_pathinfo = gitweb_get_feature('pathinfo');\n+\t\tif ($use_pathinfo[0]) {\n \t\t\t$action .= \"/\".esc_url($project);\n+\t\t\t# Add a trailing slash if requested in the configuration.\n+\t\t\t$action .= '/' if ($use_pathinfo[1]);\n \t\t}\n \t\tprint $cgi->startform(-method => \"get\", -action => $action) .\n \t\t      \"<div class=\\\"search\\\">\\n\" .\n-\t\t      (!$use_pathinfo &&\n+\t\t      (!$use_pathinfo[0] &&\n \t\t      $cgi->input({-name=>\"p\", -value=>$project, -type=>\"hidden\"}) . \"\\n\") .\n \t\t      $cgi->input({-name=>\"a\", -value=>\"search\", -type=>\"hidden\"}) . \"\\n\" .\n \t\t      $cgi->input({-name=>\"h\", -value=>$search_hash, -type=>\"hidden\"}) . \"\\n\" .\n-- \n1.6.1.rc2.27.gc7114\n"},{"id":"97817","messageId":"m3tz97g329.fsf@localhost.localdomain","threadId":"16716","inReplyTo":"1229202689.31181.1.camel@mattlaptop2.local","subject":"Re: [PATCH try 2] gitweb: Add option to put a trailing slash on pathinfo-style project URLs","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-13T21:47:46Z","receivedAt":"2008-12-13T21:47:46Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Matt McCutchen <matt@mattmccutchen.net> writes:\n\n> My Web site uses pathinfo mode and some rewrite magic to show the gitweb\n> interface at the URL of the real repository directory (which users also\n> pull from).  In this case, it's desirable to end generated links to the\n> project in a trailing slash so the Web server doesn't have to redirect\n> the client to add the slash.  This patch adds a second element to the\n> \"pathinfo\" feature configuration to control the trailing slash.\n> \n> Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>\n\nDid you check that it does not confuse gitweb if filename parameter is\npassed using pathinfo?  Gitweb used to rely on final '/' to\ndistinguish directory pathnames from ordinary pathnames, but I think\ncurrently thanks to the fact that gitweb now embeds action in pathinfo\nURL, and does not need to guess type, it is not an issue.\n\nOr only project URLs (i.e. only with project parameter, i.e. only\n\"http://git.example.com/project.git/\" but not other path_info links)\nhave trailing slash added?\n\nErrr... I see that it adds trailing slash only for project-only\npath_info links, but the commit message was not entirely clear for me.\n\n(CC-ed author of path_info enhancements, Giuseppe Bilotta)\n\n> ---\n> Resending with a sign-off.\n\nThanks.\n\n>  gitweb/gitweb.perl |   28 ++++++++++++++++++++++------\n>  1 files changed, 22 insertions(+), 6 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 6eb370d..86511cf 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -270,6 +270,11 @@ our %feature = (\n>  \t# $feature{'pathinfo'}{'default'} = [1];\n>  \t# Project specific override is not supported.\n>  \n> +\t# If you want a trailing slash on the project path (because, for\n> +\t# example, you have a real directory at that URL and are using\n> +\t# some rewrite magic to invoke gitweb), then set:\n> +\t# $feature{'pathinfo'}{'default'} = [1, 1];\n> +\n\nAre any disadvantages to having it always enabled?\n\nBTW. encoding data in position in array feels a bit hacky to me, but\nI guess that is the limitation of current %feature design, with\n'default' having to be array (reference).\n\n>  \t# Note that you will need to change the default location of CSS,\n>  \t# favicon, logo and possibly other files to an absolute URL. Also,\n>  \t# if gitweb.cgi serves as your indexfile, you will need to force\n> @@ -829,8 +834,8 @@ sub href (%) {\n>  \t\t}\n>  \t}\n>  \n> -\tmy $use_pathinfo = gitweb_check_feature('pathinfo');\n> -\tif ($use_pathinfo) {\n> +\tmy @use_pathinfo = gitweb_get_feature('pathinfo');\n\nWhy not name those variables for better readability?\n\n+       my ($use_pathinfo, $trailing_slash) = gitweb_get_feature('pathinfo');\n\n> +\tif ($use_pathinfo[0]) {\n>  \t\t# try to put as many parameters as possible in PATH_INFO:\n>  \t\t#   - project name\n>  \t\t#   - action\n> @@ -845,7 +850,12 @@ sub href (%) {\n>  \t\t$href =~ s,/$,,;\n>  \n>  \t\t# Then add the project name, if present\n> -\t\t$href .= \"/\".esc_url($params{'project'}) if defined $params{'project'};\n> +\t\tmy $proj_href = undef;\n> +\t\tif (defined $params{'project'}) {\n> +\t\t\t$href .= \"/\".esc_url($params{'project'});\n> +\t\t\t# Save for trailing-slash check below.\n> +\t\t\t$proj_href = $href;\n> +\t\t}\n>  \t\tdelete $params{'project'};\n>  \n>  \t\t# since we destructively absorb parameters, we keep this\n> @@ -903,6 +913,10 @@ sub href (%) {\n>  \t\t\t$href .= $known_snapshot_formats{$fmt}{'suffix'};\n>  \t\t\tdelete $params{'snapshot_format'};\n>  \t\t}\n> +\n> +\t\t# If requested in the configuration, add a trailing slash to a URL that\n> +\t\t# has nothing appended after the project path.\n> +\t\t$href .= '/' if ($use_pathinfo[1] && defined $proj_href && $href eq $proj_href);\n>  \t}\n\nThe check _feels_ inefficient.  I think (but feel free to disagree) that\nit would be better to use something like $project_pathinfo, set it\nwhen adding project as pathinfo, and unset if we add anything else as\npathinfo.\n\n>  \n>  \t# now encode the parameters explicitly\n> @@ -2987,13 +3001,15 @@ EOF\n>  \t\t\t$search_hash = \"HEAD\";\n>  \t\t}\n>  \t\tmy $action = $my_uri;\n> -\t\tmy $use_pathinfo = gitweb_check_feature('pathinfo');\n> -\t\tif ($use_pathinfo) {\n> +\t\tmy @use_pathinfo = gitweb_get_feature('pathinfo');\n\nSame comment as above: better named variable instead of relying on\nposition in array.\n\n> +\t\tif ($use_pathinfo[0]) {\n>  \t\t\t$action .= \"/\".esc_url($project);\n> +\t\t\t# Add a trailing slash if requested in the configuration.\n> +\t\t\t$action .= '/' if ($use_pathinfo[1]);\n\nHmmm... let me check something... you rely on the fact that $project\ndoesn't end with slash, while I think (but please check it) that it\ncan end with slash if it is provided by CGI query.\n\n>  \t\t}\n>  \t\tprint $cgi->startform(-method => \"get\", -action => $action) .\n>  \t\t      \"<div class=\\\"search\\\">\\n\" .\n> -\t\t      (!$use_pathinfo &&\n> +\t\t      (!$use_pathinfo[0] &&\n>  \t\t      $cgi->input({-name=>\"p\", -value=>$project, -type=>\"hidden\"}) . \"\\n\") .\n>  \t\t      $cgi->input({-name=>\"a\", -value=>\"search\", -type=>\"hidden\"}) . \"\\n\" .\n>  \t\t      $cgi->input({-name=>\"h\", -value=>$search_hash, -type=>\"hidden\"}) . \"\\n\" .\n> -- \n> 1.6.1.rc2.27.gc7114\n> \n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"97824","messageId":"cb7bb73a0812131423h1f629ec1n9e8eacd657a4901@mail.gmail.com","threadId":"16716","inReplyTo":"m3tz97g329.fsf@localhost.localdomain","subject":"Re: [PATCH try 2] gitweb: Add option to put a trailing slash on pathinfo-style project URLs","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-12-13T22:23:09Z","receivedAt":"2008-12-13T22:23:09Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Sat, Dec 13, 2008 at 10:47 PM, Jakub Narebski <jnareb@gmail.com> wrote:\n> Matt McCutchen <matt@mattmccutchen.net> writes:\n>\n>> My Web site uses pathinfo mode and some rewrite magic to show the gitweb\n>> interface at the URL of the real repository directory (which users also\n>> pull from).  In this case, it's desirable to end generated links to the\n>> project in a trailing slash so the Web server doesn't have to redirect\n>> the client to add the slash.  This patch adds a second element to the\n>> \"pathinfo\" feature configuration to control the trailing slash.\n>>\n>> Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>\n>\n> Did you check that it does not confuse gitweb if filename parameter is\n> passed using pathinfo?  Gitweb used to rely on final '/' to\n> distinguish directory pathnames from ordinary pathnames, but I think\n> currently thanks to the fact that gitweb now embeds action in pathinfo\n> URL, and does not need to guess type, it is not an issue.\n>\n> Or only project URLs (i.e. only with project parameter, i.e. only\n> \"http://git.example.com/project.git/\" but not other path_info links)\n> have trailing slash added?\n>\n> Errr... I see that it adds trailing slash only for project-only\n> path_info links, but the commit message was not entirely clear for me.\n\nIf indeed the additional / is only asked for in summary view, I think\nthere's no need for a feature toggle, we can always put it there. If\nnot, I'm really curious about seeing the rewrite rules (they might\nalso be worth adding to the gitweb documentation as examples of 'power\nusage').\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"97826","messageId":"7vmyez4s86.fsf@gitster.siamese.dyndns.org","threadId":"16716","inReplyTo":"m3tz97g329.fsf@localhost.localdomain","subject":"Re: [PATCH try 2] gitweb: Add option to put a trailing slash on pathinfo-style project URLs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-13T22:37:13Z","receivedAt":"2008-12-13T22:37:13Z","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>> +\t# If you want a trailing slash on the project path (because, for\n>> +\t# example, you have a real directory at that URL and are using\n>> +\t# some rewrite magic to invoke gitweb), then set:\n>> +\t# $feature{'pathinfo'}{'default'} = [1, 1];\n>> +\n>\n> Are any disadvantages to having it always enabled?\n\nGood question.\n\n>> +\tmy @use_pathinfo = gitweb_get_feature('pathinfo');\n>\n> Why not name those variables for better readability?\n>\n> +       my ($use_pathinfo, $trailing_slash) = gitweb_get_feature('pathinfo');\n\nGood suggestion.\n"},{"id":"97831","messageId":"1229217235.3360.13.camel@mattlaptop2.local","threadId":"16716","inReplyTo":"cb7bb73a0812131423h1f629ec1n9e8eacd657a4901@mail.gmail.com","subject":"Re: [PATCH try 2] gitweb: Add option to put a trailing slash on pathinfo-style project URLs","fromName":"Matt McCutchen","fromEmail":"matt@mattmccutchen.net","sentAt":"2008-12-14T01:13:55Z","receivedAt":"2008-12-14T01:13:55Z","isPatch":true,"sender":{"key":"matt@mattmccutchen.net","avatar":"https://avatars.githubusercontent.com/u/8885753?v=4"},"body":"On Sat, 2008-12-13 at 23:23 +0100, Giuseppe Bilotta wrote:\n> On Sat, Dec 13, 2008 at 10:47 PM, Jakub Narebski <jnareb@gmail.com> wrote:\n> > Matt McCutchen <matt@mattmccutchen.net> writes:\n> >\n> >> My Web site uses pathinfo mode and some rewrite magic to show the gitweb\n> >> interface at the URL of the real repository directory (which users also\n> >> pull from).  In this case, it's desirable to end generated links to the\n> >> project in a trailing slash so the Web server doesn't have to redirect\n> >> the client to add the slash.  This patch adds a second element to the\n> >> \"pathinfo\" feature configuration to control the trailing slash.\n> >>\n> >> Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>\n> >\n> > Did you check that it does not confuse gitweb if filename parameter is\n> > passed using pathinfo?  Gitweb used to rely on final '/' to\n> > distinguish directory pathnames from ordinary pathnames, but I think\n> > currently thanks to the fact that gitweb now embeds action in pathinfo\n> > URL, and does not need to guess type, it is not an issue.\n> >\n> > Or only project URLs (i.e. only with project parameter, i.e. only\n> > \"http://git.example.com/project.git/\" but not other path_info links)\n> > have trailing slash added?\n> >\n> > Errr... I see that it adds trailing slash only for project-only\n> > path_info links, but the commit message was not entirely clear for me.\n> \n> If indeed the additional / is only asked for in summary view, I think\n> there's no need for a feature toggle, we can always put it there. If\n> not, I'm really curious about seeing the rewrite rules (they might\n> also be worth adding to the gitweb documentation as examples of 'power\n> usage').\n\nThe trailing slash is used only when the URL refers to a project with no\nappended parameters (i.e., summary view), because the URL refers to the\nreal git dir on disk (hence, pulling from the same URL) and it plays\nnicer with the Web server configuration to have the trailing slash.\n\nI was wary about changing the default behavior, but if you and Jakub\nboth think it's OK, that's great.\n\nI was thinking of proposing the addition of some info about my setup,\nincluding the rewrite rules, to the documentation.  Maybe we could do\nthat after dealing with the patches.\n\n-- \nMatt\n"},{"id":"97832","messageId":"1229219030.3360.44.camel@mattlaptop2.local","threadId":"16716","inReplyTo":"m3tz97g329.fsf@localhost.localdomain","subject":"Re: [PATCH try 2] gitweb: Add option to put a trailing slash on pathinfo-style project URLs","fromName":"Matt McCutchen","fromEmail":"matt@mattmccutchen.net","sentAt":"2008-12-14T01:43:50Z","receivedAt":"2008-12-14T01:43:50Z","isPatch":true,"sender":{"key":"matt@mattmccutchen.net","avatar":"https://avatars.githubusercontent.com/u/8885753?v=4"},"body":"On Sat, 2008-12-13 at 13:47 -0800, Jakub Narebski wrote:\n> Errr... I see that it adds trailing slash only for project-only\n> path_info links, but the commit message was not entirely clear for me.\n\nI will clarify the message.\n\n> BTW. encoding data in position in array feels a bit hacky to me, but\n> I guess that is the limitation of current %feature design, with\n> 'default' having to be array (reference).\n> \n> >  \t# Note that you will need to change the default location of CSS,\n> >  \t# favicon, logo and possibly other files to an absolute URL. Also,\n> >  \t# if gitweb.cgi serves as your indexfile, you will need to force\n> > @@ -829,8 +834,8 @@ sub href (%) {\n> >  \t\t}\n> >  \t}\n> >  \n> > -\tmy $use_pathinfo = gitweb_check_feature('pathinfo');\n> > -\tif ($use_pathinfo) {\n> > +\tmy @use_pathinfo = gitweb_get_feature('pathinfo');\n> \n> Why not name those variables for better readability?\n> \n> +       my ($use_pathinfo, $trailing_slash) = gitweb_get_feature('pathinfo');\n\nI'll do that.\n\n> > +\tif ($use_pathinfo[0]) {\n> >  \t\t# try to put as many parameters as possible in PATH_INFO:\n> >  \t\t#   - project name\n> >  \t\t#   - action\n> > @@ -845,7 +850,12 @@ sub href (%) {\n> >  \t\t$href =~ s,/$,,;\n> >  \n> >  \t\t# Then add the project name, if present\n> > -\t\t$href .= \"/\".esc_url($params{'project'}) if defined $params{'project'};\n> > +\t\tmy $proj_href = undef;\n> > +\t\tif (defined $params{'project'}) {\n> > +\t\t\t$href .= \"/\".esc_url($params{'project'});\n> > +\t\t\t# Save for trailing-slash check below.\n> > +\t\t\t$proj_href = $href;\n> > +\t\t}\n> >  \t\tdelete $params{'project'};\n> >  \n> >  \t\t# since we destructively absorb parameters, we keep this\n> > @@ -903,6 +913,10 @@ sub href (%) {\n> >  \t\t\t$href .= $known_snapshot_formats{$fmt}{'suffix'};\n> >  \t\t\tdelete $params{'snapshot_format'};\n> >  \t\t}\n> > +\n> > +\t\t# If requested in the configuration, add a trailing slash to a URL that\n> > +\t\t# has nothing appended after the project path.\n> > +\t\t$href .= '/' if ($use_pathinfo[1] && defined $proj_href && $href eq $proj_href);\n> >  \t}\n> \n> The check _feels_ inefficient.  I think (but feel free to disagree) that\n> it would be better to use something like $project_pathinfo, set it\n> when adding project as pathinfo, and unset if we add anything else as\n> pathinfo.\n\nI considered doing that, but I decided that not having to litter the\npreceding code with manipulation of $project_pathinfo outweighed\nwhatever negligible performance difference there might be.\n\n> > +\t\tif ($use_pathinfo[0]) {\n> >  \t\t\t$action .= \"/\".esc_url($project);\n> > +\t\t\t# Add a trailing slash if requested in the configuration.\n> > +\t\t\t$action .= '/' if ($use_pathinfo[1]);\n> \n> Hmmm... let me check something... you rely on the fact that $project\n> doesn't end with slash, while I think (but please check it) that it\n> can end with slash if it is provided by CGI query.\n\nYou are right; in fact, this is already a problem for the strict_export\ncheck.  Gitweb should probably strip trailing slashes when it reads the\n\"p\" parameter.  I will submit a separate patch for that.\n\n-- \nMatt\n"},{"id":"97899","messageId":"200812150020.53370.jnareb@gmail.com","threadId":"16716","inReplyTo":"1229219030.3360.44.camel@mattlaptop2.local","subject":"Re: [PATCH try 2] gitweb: Add option to put a trailing slash on pathinfo-style project URLs","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-14T23:20:48Z","receivedAt":"2008-12-14T23:20:48Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sun, 14 Dec 2008, Matt McCutchen wrote:\n> On Sat, 2008-12-13 at 13:47 -0800, Jakub Narebski wrote:\n> >\n> > Errr... I see that it adds trailing slash only for project-only\n> > path_info links, but the commit message was not entirely clear for me.\n> \n> I will clarify the message.\n\nIt would be nice (e.g. to have example URL with trailing slash ensured).\n\n[...]\n> > > @@ -829,8 +834,8 @@ sub href (%) {\n> > >  \t\t}\n> > >  \t}\n> > >  \n> > > -\tmy $use_pathinfo = gitweb_check_feature('pathinfo');\n> > > -\tif ($use_pathinfo) {\n> > > +\tmy @use_pathinfo = gitweb_get_feature('pathinfo');\n> > \n> > Why not name those variables for better readability?\n> > \n> > +       my ($use_pathinfo, $trailing_slash) = gitweb_get_feature('pathinfo');\n> \n> I'll do that.\n\nNote that you wouldn't need that if you decide that it makes sense\n(and doesn't have disadvantages) to *always* add trailing slash to\nthe end of path_info for some kinds of gitweb links.\n\n> > > +\tif ($use_pathinfo[0]) {\n> > >  \t\t# try to put as many parameters as possible in PATH_INFO:\n> > >  \t\t#   - project name\n> > >  \t\t#   - action\n> > > @@ -845,7 +850,12 @@ sub href (%) {\n> > >  \t\t$href =~ s,/$,,;\n> > >  \n> > >  \t\t# Then add the project name, if present\n> > > -\t\t$href .= \"/\".esc_url($params{'project'}) if defined $params{'project'};\n> > > +\t\tmy $proj_href = undef;\n> > > +\t\tif (defined $params{'project'}) {\n> > > +\t\t\t$href .= \"/\".esc_url($params{'project'});\n> > > +\t\t\t# Save for trailing-slash check below.\n> > > +\t\t\t$proj_href = $href;\n> > > +\t\t}\n> > >  \t\tdelete $params{'project'};\n> > >  \n> > >  \t\t# since we destructively absorb parameters, we keep this\n> > > @@ -903,6 +913,10 @@ sub href (%) {\n> > >  \t\t\t$href .= $known_snapshot_formats{$fmt}{'suffix'};\n> > >  \t\t\tdelete $params{'snapshot_format'};\n> > >  \t\t}\n> > > +\n> > > +\t\t# If requested in the configuration, add a trailing slash to a URL that\n> > > +\t\t# has nothing appended after the project path.\n> > > +\t\t$href .= '/' if ($use_pathinfo[1] && defined $proj_href && $href eq $proj_href);\n> > >  \t}\n> > \n> > The check _feels_ inefficient.  I think (but feel free to disagree) that\n> > it would be better to use something like $project_pathinfo, set it\n> > when adding project as pathinfo, and unset if we add anything else as\n> > pathinfo.\n> \n> I considered doing that, but I decided that not having to litter the\n> preceding code with manipulation of $project_pathinfo outweighed\n> whatever negligible performance difference there might be.\n\nOn the other hand, with having boolean variable named for example\n$trailing_slash or $add_trailing_slash, you can set it to appropriate\nvalue by default (should project list URL: http://example.com/ have\ntrailing slash), and at [almost] each 'delete $params{<param>}' either\nset it to true, or set it to false. This way it would be easy to\nextend to have trailing slash also for example for OPML link\nhttp://example.com/opml/ or not have it and use http://example.com/opml\n\nI think it is not only more efficient, but is also more flexible.\nAdmittedly it is also more complicated...\n \n> > > +\t\tif ($use_pathinfo[0]) {\n> > >  \t\t\t$action .= \"/\".esc_url($project);\n> > > +\t\t\t# Add a trailing slash if requested in the configuration.\n> > > +\t\t\t$action .= '/' if ($use_pathinfo[1]);\n> > \n> > Hmmm... let me check something... you rely on the fact that $project\n> > doesn't end with slash, while I think (but please check it) that it\n> > can end with slash if it is provided by CGI query.\n> \n> You are right; in fact, this is already a problem for the strict_export\n> check.  Gitweb should probably strip trailing slashes when it reads the\n> \"p\" parameter.  I will submit a separate patch for that.\n\nThat would be nice. TIA.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"97901","messageId":"200812150039.58797.jnareb@gmail.com","threadId":"16716","inReplyTo":"1229217235.3360.13.camel@mattlaptop2.local","subject":"Re: [PATCH try 2] gitweb: Add option to put a trailing slash on pathinfo-style project URLs","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-14T23:39:57Z","receivedAt":"2008-12-14T23:39:57Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sun, 14 Dec 2008, Matt McCutchen wrote:\n> On Sat, 2008-12-13 at 23:23 +0100, Giuseppe Bilotta wrote:\n>> On Sat, Dec 13, 2008 at 10:47 PM, Jakub Narebski <jnareb@gmail.com> wrote:\n>>> Matt McCutchen <matt@mattmccutchen.net> writes:\n>>>\n>>>> My Web site uses pathinfo mode and some rewrite magic to show the gitweb\n>>>> interface at the URL of the real repository directory (which users also\n>>>> pull from).  In this case, it's desirable to end generated links to the\n>>>> project in a trailing slash so the Web server doesn't have to redirect\n>>>> the client to add the slash.  This patch adds a second element to the\n>>>> \"pathinfo\" feature configuration to control the trailing slash.\n>>>>\n>>>> Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>\n[...]\n>>>\n>>> Errr... I see that it adds trailing slash only for project-only\n>>> path_info links, but the commit message was not entirely clear for me.\n>> \n>> If indeed the additional / is only asked for in summary view, I think\n>> there's no need for a feature toggle, we can always put it there. If\n>> not, I'm really curious about seeing the rewrite rules (they might\n>> also be worth adding to the gitweb documentation as examples of 'power\n>> usage').\n> \n> The trailing slash is used only when the URL refers to a project with no\n> appended parameters (i.e., summary view), because the URL refers to the\n> real git dir on disk (hence, pulling from the same URL) and it plays\n> nicer with the Web server configuration to have the trailing slash.\n\nIt would be nice to have in commit message that we want to have\ntrailing slash in the cases where URL can correspond to filesystem\npath.\n\nBut there are two cases: \n * http://example.com/ corresponding to $projectroot on filesystem,\n   and giving projects_list in gitweb\n * http://example.com/project.git/ corresponding to project.git dir\n   on filesystem, and giving project summary view in gitweb.\n\n> I was wary about changing the default behavior, but if you and Jakub\n> both think it's OK, that's great.\n\nNow I'm not so sure... but I guess the performance cost would be\nnegligible, and I'm not sure if it would be worth slight complication\nin the code (and configuration).\n\n> I was thinking of proposing the addition of some info about my setup,\n> including the rewrite rules, to the documentation.  Maybe we could do\n> that after dealing with the patches.\n\nDo you plan updating \"Webserver configuration\" section in \ngitweb/README? \n\nBTW. could you please check if the $my_uri and $my_link need to be set\nin gitweb config for your configuration, or did some of Giuseppe's\npath_info improvements took care of that, and it is no longer needed?\n\n-- \nJakub Narebski\nPoland\n"},{"id":"97904","messageId":"200812150055.40463.jnareb@gmail.com","threadId":"16716","inReplyTo":"200812150039.58797.jnareb@gmail.com","subject":"Re: [PATCH try 2] gitweb: Add option to put a trailing slash on pathinfo-style project URLs","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-14T23:55:38Z","receivedAt":"2008-12-14T23:55:38Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Jakub Narebski wrote:\n> On Sun, 14 Dec 2008, Matt McCutchen wrote:\n\n> > I was thinking of proposing the addition of some info about my setup,\n> > including the rewrite rules, to the documentation.  Maybe we could do\n> > that after dealing with the patches.\n> \n> Do you plan updating \"Webserver configuration\" section in \n> gitweb/README? \n> \n> BTW. could you please check if the $my_uri and $my_link need to be set\n> in gitweb config for your configuration, or did some of Giuseppe's\n> path_info improvements took care of that, and it is no longer needed?\n\nIt would be I think good idea to describe there _and_ in the commit\nmessage why would you prefer for gitweb path_info links to be generated\nwith trailing slash; how it looks resolution for URL with and without\ntrailing slash. \n\n-- \nJakub Narebski\nPoland\n"},{"id":"97905","messageId":"200812150058.36038.jnareb@gmail.com","threadId":"16716","inReplyTo":"200812150020.53370.jnareb@gmail.com","subject":"Re: [PATCH try 2] gitweb: Add option to put a trailing slash on pathinfo-style project URLs","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-14T23:58:34Z","receivedAt":"2008-12-14T23:58:34Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, 15 Dec 2008, Jakub Narebski wrote:\n> On Sun, 14 Dec 2008, Matt McCutchen wrote:\n>> On Sat, 2008-12-13 at 13:47 -0800, Jakub Narebski wrote:\n\n>>>> @@ -845,7 +850,12 @@ sub href (%) {\n>>>>  \t\t$href =~ s,/$,,;\n>>>>  \n>>>>  \t\t# Then add the project name, if present\n>>>> -\t\t$href .= \"/\".esc_url($params{'project'}) if defined $params{'project'};\n>>>> +\t\tmy $proj_href = undef;\n>>>> +\t\tif (defined $params{'project'}) {\n>>>> +\t\t\t$href .= \"/\".esc_url($params{'project'});\n>>>> +\t\t\t# Save for trailing-slash check below.\n>>>> +\t\t\t$proj_href = $href;\n>>>> +\t\t}\n>>>>  \t\tdelete $params{'project'};\n>>>>  \n>>>>  \t\t# since we destructively absorb parameters, we keep this\n>>>> @@ -903,6 +913,10 @@ sub href (%) {\n>>>>  \t\t\t$href .= $known_snapshot_formats{$fmt}{'suffix'};\n>>>>  \t\t\tdelete $params{'snapshot_format'};\n>>>>  \t\t}\n>>>> +\n>>>> +\t\t# If requested in the configuration, add a trailing slash to a URL that\n>>>> +\t\t# has nothing appended after the project path.\n>>>> +\t\t$href .= '/' if ($use_pathinfo[1] && defined $proj_href && $href eq $proj_href);\n>>>>  \t}\n>>> \n>>> The check _feels_ inefficient.  I think (but feel free to disagree) that\n>>> it would be better to use something like $project_pathinfo, set it\n>>> when adding project as pathinfo, and unset if we add anything else as\n>>> pathinfo.\n>> \n>> I considered doing that, but I decided that not having to litter the\n>> preceding code with manipulation of $project_pathinfo outweighed\n>> whatever negligible performance difference there might be.\n> \n> On the other hand, with having boolean variable named for example\n> $trailing_slash or $add_trailing_slash, you can set it to appropriate\n> value by default (should project list URL: http://example.com/ have\n> trailing slash), and at [almost] each 'delete $params{<param>}' either\n> set it to true, or set it to false. This way it would be easy to\n> extend to have trailing slash also for example for OPML link\n> http://example.com/opml/ or not have it and use http://example.com/opml\n> \n> I think it is not only more efficient, but is also more flexible.\n> Admittedly it is also more complicated...\n\nOn the other hand you don't need such flexibility, so perhaps simpler\ncode (or at least less changes) outweights this issue...\n\n-- \nJakub Narebski\nPoland\n"}]}