{"thread":{"id":"15618","subject":"[PATCHv2 0/6] gitweb pathinfo improvements","startedAt":"2008-09-21T20:57:37Z","lastAt":"2008-09-30T23:49:48Z","messageCount":25,"participants":["Giuseppe Bilotta","Jakub Narebski"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"91262","messageId":"1222030663-22540-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"15618","inReplyTo":null,"subject":"[PATCHv2 0/6] gitweb pathinfo improvements","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-09-21T20:57:37Z","receivedAt":"2008-09-21T20:57:37Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"This is a resend, with some improvements and a proper cover\nletter, of my patchset to extend PATH_INFO support in gitweb.\nHopefully I'm doing it the right way this time :)\n\nThe basic idea is to have gitweb support paths in the form of\nproject/action/parent:/filename..hash:/filename\n(modulo missing parameters) and to generate them when use_pathinfo\nis enabled.\n\nFor backwards compatibility, old-style urls $project/$hash are\nstill supported (unless $hash is a named ref that happens to\ncoincide with a gitweb action). Also, CGI parameters are still\nused when the path_info form would be ambiguous (e.g. filenames\nwith two consecutive dots in their name).\n\nGiuseppe Bilotta (6):\n gitweb: action in path with use_pathinfo\n gitweb: use_pathinfo filenames start with /\n gitweb: parse parent..current syntax from pathinfo\n gitweb: use_pathinfo creates parent..current paths\n gitweb: remove PATH_INFO from $my_url and $my_uri\n gitweb: prevent double slashes in PATH_INFO hrefs\n\n gitweb/gitweb.perl | 161 +++++++++++++++++++++++++++++++++++++++-------------\n 1 files changed, 122 insertions(+), 39 deletions(-)\n"},{"id":"91263","messageId":"1222030663-22540-2-git-send-email-giuseppe.bilotta@gmail.com","threadId":"15618","inReplyTo":"1222030663-22540-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCH 1/6] gitweb: action in path with use_pathinfo","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-09-21T20:57:38Z","receivedAt":"2008-09-21T20:57:38Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"When using path info, reduce the number of CGI parameters by embedding\nthe action in the path. The typicial cgiweb path is thus\n$project/$action/$hash_base:$file_name or $project/$action/$hash\n\nThis is mostly backwards compatible with the old-style gitweb paths,\nexcept when $project/$branch was used to access a branch whose name\nmatches a gitweb action.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 109 ++++++++++++++++++++++++++++++++++------------------\n 1 files changed, 72 insertions(+), 37 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex da474d0..e783d12 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -488,6 +488,37 @@ if (defined $searchtext) {\n \t$search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext;\n }\n \n+# dispatch\n+my %actions = (\n+\t\"blame\" => \\&git_blame,\n+\t\"blobdiff\" => \\&git_blobdiff,\n+\t\"blobdiff_plain\" => \\&git_blobdiff_plain,\n+\t\"blob\" => \\&git_blob,\n+\t\"blob_plain\" => \\&git_blob_plain,\n+\t\"commitdiff\" => \\&git_commitdiff,\n+\t\"commitdiff_plain\" => \\&git_commitdiff_plain,\n+\t\"commit\" => \\&git_commit,\n+\t\"forks\" => \\&git_forks,\n+\t\"heads\" => \\&git_heads,\n+\t\"history\" => \\&git_history,\n+\t\"log\" => \\&git_log,\n+\t\"rss\" => \\&git_rss,\n+\t\"atom\" => \\&git_atom,\n+\t\"search\" => \\&git_search,\n+\t\"search_help\" => \\&git_search_help,\n+\t\"shortlog\" => \\&git_shortlog,\n+\t\"summary\" => \\&git_summary,\n+\t\"tag\" => \\&git_tag,\n+\t\"tags\" => \\&git_tags,\n+\t\"tree\" => \\&git_tree,\n+\t\"snapshot\" => \\&git_snapshot,\n+\t\"object\" => \\&git_object,\n+\t# those below don't need $project\n+\t\"opml\" => \\&git_opml,\n+\t\"project_list\" => \\&git_project_list,\n+\t\"project_index\" => \\&git_project_index,\n+);\n+\n # now read PATH_INFO and use it as alternative to parameters\n sub evaluate_path_info {\n \treturn if defined $project;\n@@ -512,6 +543,16 @@ sub evaluate_path_info {\n \t# do not change any parameters if an action is given using the query string\n \treturn if $action;\n \t$path_info =~ s,^\\Q$project\\E/*,,;\n+\n+\t# next comes the action\n+\t$action = $path_info;\n+\t$action =~ s,/.*$,,;\n+\tif (exists $actions{$action}) {\n+\t\t$path_info =~ s,^\\Q$action\\E/*,,;\n+\t} else {\n+\t\t$action = undef;\n+\t}\n+\n \tmy ($refname, $pathname) = split(/:/, $path_info, 2);\n \tif (defined $pathname) {\n \t\t# we got \"project.git/branch:filename\" or \"project.git/branch:dir/\"\n@@ -525,10 +566,12 @@ sub evaluate_path_info {\n \t\t}\n \t\t$hash_base ||= validate_refname($refname);\n \t\t$file_name ||= validate_pathname($pathname);\n+\t\t$hash ||= git_get_hash_by_path($hash_base, $file_name);\n \t} elsif (defined $refname) {\n \t\t# we got \"project.git/branch\"\n-\t\t$action ||= \"shortlog\";\n-\t\t$hash ||= validate_refname($refname);\n+\t\t$action ||= \"shortlog\";\n+\t\t$hash ||= validate_refname($refname);\n+\t\t$hash_base ||= validate_refname($refname);\n \t}\n }\n evaluate_path_info();\n@@ -537,37 +580,6 @@ evaluate_path_info();\n our $git_dir;\n $git_dir = \"$projectroot/$project\" if $project;\n \n-# dispatch\n-my %actions = (\n-\t\"blame\" => \\&git_blame,\n-\t\"blobdiff\" => \\&git_blobdiff,\n-\t\"blobdiff_plain\" => \\&git_blobdiff_plain,\n-\t\"blob\" => \\&git_blob,\n-\t\"blob_plain\" => \\&git_blob_plain,\n-\t\"commitdiff\" => \\&git_commitdiff,\n-\t\"commitdiff_plain\" => \\&git_commitdiff_plain,\n-\t\"commit\" => \\&git_commit,\n-\t\"forks\" => \\&git_forks,\n-\t\"heads\" => \\&git_heads,\n-\t\"history\" => \\&git_history,\n-\t\"log\" => \\&git_log,\n-\t\"rss\" => \\&git_rss,\n-\t\"atom\" => \\&git_atom,\n-\t\"search\" => \\&git_search,\n-\t\"search_help\" => \\&git_search_help,\n-\t\"shortlog\" => \\&git_shortlog,\n-\t\"summary\" => \\&git_summary,\n-\t\"tag\" => \\&git_tag,\n-\t\"tags\" => \\&git_tags,\n-\t\"tree\" => \\&git_tree,\n-\t\"snapshot\" => \\&git_snapshot,\n-\t\"object\" => \\&git_object,\n-\t# those below don't need $project\n-\t\"opml\" => \\&git_opml,\n-\t\"project_list\" => \\&git_project_list,\n-\t\"project_index\" => \\&git_project_index,\n-);\n-\n if (!defined $action) {\n \tif (defined $hash) {\n \t\t$action = git_get_type($hash);\n@@ -624,8 +636,13 @@ sub href (%) {\n \tif ($params{-replay}) {\n \t\twhile (my ($name, $symbol) = each %mapping) {\n \t\t\tif (!exists $params{$name}) {\n-\t\t\t\t# to allow for multivalued params we use arrayref form\n-\t\t\t\t$params{$name} = [ $cgi->param($symbol) ];\n+\t\t\t\tif ($cgi->param($symbol)) {\n+\t\t\t\t\t# to allow for multivalued params we use arrayref form\n+\t\t\t\t\t$params{$name} = [ $cgi->param($symbol) ];\n+\t\t\t\t} else {\n+\t\t\t\t\tno strict 'refs';\n+\t\t\t\t\t$params{$name} = $$name if $$name;\n+\t\t\t\t}\n \t\t\t}\n \t\t}\n \t}\n@@ -636,10 +653,28 @@ sub href (%) {\n \t\t$href .= \"/\".esc_url($params{'project'}) if defined $params{'project'};\n \t\tdelete $params{'project'};\n \n-\t\t# Summary just uses the project path URL\n-\t\tif (defined $params{'action'} && $params{'action'} eq 'summary') {\n+\t\t# Summary just uses the project path URL, any other action come next\n+\t\t# in the URL\n+\t\tif (defined $params{'action'}) {\n+\t\t\t$href .= \"/\".esc_url($params{'action'}) unless $params{'action'} eq 'summary';\n \t\t\tdelete $params{'action'};\n \t\t}\n+\n+\t\t# next, we put either hash_base:file_name or hash\n+\t\tif (defined $params{'hash_base'}) {\n+\t\t\t$href .= \"/\".esc_url($params{'hash_base'});\n+\t\t\tif (defined $params{'file_name'}) {\n+\t\t\t\t$href .= \":\".esc_url($params{'file_name'});\n+\t\t\t\tdelete $params{'hash'} if $params{'hash'} eq git_get_hash_by_path($params{'hash_base'},$params{'file_name'});\n+\t\t\t\tdelete $params{'file_name'};\n+\t\t\t} else {\n+\t\t\t\tdelete $params{'hash'} if $params{'hash'} eq $params{'hash_base'};\n+\t\t\t}\n+\t\t\tdelete $params{'hash_base'};\n+\t\t} elsif (defined $params{'hash'}) {\n+\t\t\t$href .= \"/\".esc_url($params{'hash'});\n+\t\t\tdelete $params{'hash'};\n+\t\t}\n \t}\n \n \t# now encode the parameters explicitly\n-- \n1.5.6.5\n"},{"id":"91266","messageId":"1222030663-22540-3-git-send-email-giuseppe.bilotta@gmail.com","threadId":"15618","inReplyTo":"1222030663-22540-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCH 2/6] gitweb: use_pathinfo filenames start with /","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-09-21T20:57:39Z","receivedAt":"2008-09-21T20:57:39Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"When using path info, make filenames start with a / (right after the :\nthat separates them from the hash base). This minimal change allows\nrelative navigation to work properly when viewing HTML files.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex e783d12..18da484 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -664,7 +664,7 @@ sub href (%) {\n \t\tif (defined $params{'hash_base'}) {\n \t\t\t$href .= \"/\".esc_url($params{'hash_base'});\n \t\t\tif (defined $params{'file_name'}) {\n-\t\t\t\t$href .= \":\".esc_url($params{'file_name'});\n+\t\t\t\t$href .= \":/\".esc_url($params{'file_name'});\n \t\t\t\tdelete $params{'hash'} if $params{'hash'} eq git_get_hash_by_path($params{'hash_base'},$params{'file_name'});\n \t\t\t\tdelete $params{'file_name'};\n \t\t\t} else {\n-- \n1.5.6.5\n"},{"id":"91264","messageId":"1222030663-22540-4-git-send-email-giuseppe.bilotta@gmail.com","threadId":"15618","inReplyTo":"1222030663-22540-3-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCH 3/6] gitweb: parse parent..current syntax from pathinfo","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-09-21T20:57:40Z","receivedAt":"2008-09-21T20:57:40Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"This makes it possible to use an URL such as\n$project/somebranch..otherbranch:/filename to get a diff between\ndifferent version of a file. Paths like\n$project/$action/somebranch:/somefile..otherbranch:/otherfile are parsed\nas well.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 26 ++++++++++++++++++++++++--\n 1 files changed, 24 insertions(+), 2 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 18da484..9868bf4 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -553,7 +553,9 @@ sub evaluate_path_info {\n \t\t$action = undef;\n \t}\n \n-\tmy ($refname, $pathname) = split(/:/, $path_info, 2);\n+\t$path_info =~ /^((.+?)(:(.+))?\\.\\.)?(.+?)(:(.+))?$/;\n+\tmy ($parentrefname, $parentpathname, $refname, $pathname) = (\n+\t\t$2, $4, $5, $7);\n \tif (defined $pathname) {\n \t\t# we got \"project.git/branch:filename\" or \"project.git/branch:dir/\"\n \t\t# we could use git_get_type(branch:pathname), but it needs $git_dir\n@@ -562,7 +564,11 @@ sub evaluate_path_info {\n \t\t\t$action ||= \"tree\";\n \t\t\t$pathname =~ s,/$,,;\n \t\t} else {\n-\t\t\t$action ||= \"blob_plain\";\n+\t\t\tif ($parentrefname) {\n+\t\t\t\t$action ||= \"blobdiff_plain\";\n+\t\t\t} else {\n+\t\t\t\t$action ||= \"blob_plain\";\n+\t\t\t}\n \t\t}\n \t\t$hash_base ||= validate_refname($refname);\n \t\t$file_name ||= validate_pathname($pathname);\n@@ -573,6 +579,22 @@ sub evaluate_path_info {\n \t\t$hash ||= validate_refname($refname);\n \t\t$hash_base ||= validate_refname($refname);\n \t}\n+\t# the parent part might be missing the pathname, in which case we use the $file_name, if present\n+\tif (defined $parentrefname) {\n+\t\t$hash_parent_base ||= validate_refname($parentrefname);\n+\t\tif ($parentpathname) {\n+\t\t\t$parentpathname =~ s,^/+,,;\n+\t\t\t$parentpathname =~ s,/$,,;\n+\t\t\t$file_parent ||= validate_pathname($parentpathname);\n+\t\t} else {\n+\t\t\t$file_parent ||= $file_name\n+\t\t}\n+\t\tif (defined $file_parent) {\n+\t\t\t$hash_parent ||= git_get_hash_by_path($hash_parent_base, $file_parent);\n+\t\t} else {\n+\t\t\t$hash_parent ||= validate_refname($parentrefname);\n+\t\t}\n+\t}\n }\n evaluate_path_info();\n \n-- \n1.5.6.5\n"},{"id":"91267","messageId":"1222030663-22540-5-git-send-email-giuseppe.bilotta@gmail.com","threadId":"15618","inReplyTo":"1222030663-22540-4-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCH 4/6] gitweb: use_pathinfo creates parent..current paths","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-09-21T20:57:41Z","receivedAt":"2008-09-21T20:57:41Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"If use_pathinfo is enabled, href now creates links that contain paths in\nthe form $project/$action/oldhash:/oldname..newhash:/newname for actions\nthat use hash_parent etc, unless oldname contains two consecutive dots, since\nthis would cause ambiguity when parsing the resulting path.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 26 +++++++++++++++++++++++---\n 1 files changed, 23 insertions(+), 3 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 9868bf4..0dd2526 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -682,9 +682,29 @@ sub href (%) {\n \t\t\tdelete $params{'action'};\n \t\t}\n \n-\t\t# next, we put either hash_base:file_name or hash\n+\t\t# next, we put hash_parent_base:/file_parent..hash_base:/file_name, stripping nonexistent or useless pieces\n+\t\t$href .= \"/\" if ($params{'hash_base'} || $params{'hash_parent_base'} || $params{'hash_parent'} || $params{'hash'});\n \t\tif (defined $params{'hash_base'}) {\n-\t\t\t$href .= \"/\".esc_url($params{'hash_base'});\n+\t\t\tif (defined $params{'hash_parent_base'}) {\n+\t\t\t\t$href .= esc_url($params{'hash_parent_base'});\n+\t\t\t\tif (defined $params{'file_parent'} && $params{'file_parent'} !~ /\\.\\./) {\n+\t\t\t\t\t$href .= \":/\".esc_url($params{'file_parent'}) unless $params{'file_parent'} eq $params{'file_name'};\n+\t\t\t\t\tdelete $params{'hash_parent'} if $params{'hash_parent'} eq git_get_hash_by_path($params{'hash_parent_base'},$params{'file_parent'});\n+\t\t\t\t\tdelete $params{'file_parent'};\n+\t\t\t\t} else {\n+\t\t\t\t\tdelete $params{'hash_parent'} if $params{'hash_parent'} eq $params{'hash_parent_base'};\n+\t\t\t\t\tif ($params{'file_name'}) {\n+\t\t\t\t\t\tdelete $params{'hash_parent'} if $params{'hash_parent'} eq git_get_hash_by_path($params{'hash_parent_base'},$params{'file_name'});\n+\t\t\t\t\t}\n+\t\t\t\t}\n+\t\t\t\t$href .= \"..\";\n+\t\t\t\tdelete $params{'hash_parent_base'};\n+\t\t\t} elsif (defined $params{'hash_parent'}) {\n+\t\t\t\t$href .= esc_url($params{'hash_parent'}). \"..\";\n+\t\t\t\tdelete $params{'hash_parent'};\n+\t\t\t}\n+\n+\t\t\t$href .= esc_url($params{'hash_base'});\n-\t\t\tif (defined $params{'file_name'}) {\n+\t\t\tif (defined $params{'file_name'} && $params{'file_name'} !~ /\\.\\./) {\n \t\t\t\t$href .= \":/\".esc_url($params{'file_name'});\n \t\t\t\tdelete $params{'hash'} if $params{'hash'} eq git_get_hash_by_path($params{'hash_base'},$params{'file_name'});\n@@ -694,7 +714,7 @@ sub href (%) {\n \t\t\t}\n \t\t\tdelete $params{'hash_base'};\n \t\t} elsif (defined $params{'hash'}) {\n-\t\t\t$href .= \"/\".esc_url($params{'hash'});\n+\t\t\t$href .= esc_url($params{'hash'});\n \t\t\tdelete $params{'hash'};\n \t\t}\n \t}\n-- \n1.5.6.5\n"},{"id":"91265","messageId":"1222030663-22540-6-git-send-email-giuseppe.bilotta@gmail.com","threadId":"15618","inReplyTo":"1222030663-22540-5-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCH 5/6] gitweb: remove PATH_INFO from $my_url and $my_uri","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-09-21T20:57:42Z","receivedAt":"2008-09-21T20:57:42Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"This patch (combined with appropriate server configuration) allows usage\nof the gitweb CGI script as DirectoryIndex for the server root even when\nthe pathinfo feature is enabled.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 0dd2526..4a91d07 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -26,6 +26,10 @@ our $cgi = new CGI;\n our $version = \"++GIT_VERSION++\";\n our $my_url = $cgi->url();\n our $my_uri = $cgi->url(-absolute => 1);\n+if (my $path_info = $ENV{\"PATH_INFO\"}) {\n+\t$my_url =~ s,$path_info$,,;\n+\t$my_uri =~ s,$path_info$,,;\n+}\n \n # core git executable to use\n # this can just be \"git\" if your webserver has a sensible PATH\n-- \n1.5.6.5\n"},{"id":"91268","messageId":"1222030663-22540-7-git-send-email-giuseppe.bilotta@gmail.com","threadId":"15618","inReplyTo":"1222030663-22540-6-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCH 6/6] gitweb: prevent double slashes in PATH_INFO hrefs","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-09-21T20:57:43Z","receivedAt":"2008-09-21T20:57:43Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"When using PATH_INFO in combination with a rewrite rule that hides the\ncgi script name, links to projects and/or actions without projects might\nbe generated with a double slash.\n\nFix by removing the trailing slash (if present) from $href before\nappending PATH_INFO data.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 4a91d07..ebab86b 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -675,6 +675,8 @@ sub href (%) {\n \n \tmy ($use_pathinfo) = gitweb_check_feature('pathinfo');\n \tif ($use_pathinfo) {\n+\t\t$href =~ s,/$,,;\n+\n \t\t# use PATH_INFO for project name\n \t\t$href .= \"/\".esc_url($params{'project'}) if defined $params{'project'};\n \t\tdelete $params{'project'};\n-- \n1.5.6.5\n"},{"id":"91819","messageId":"200809290303.21290.jnareb@gmail.com","threadId":"15618","inReplyTo":"1222030663-22540-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCH 1/6] gitweb: action in path with use_pathinfo","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-09-29T01:03:20Z","receivedAt":"2008-09-29T01:03:20Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"I'm sorry for the delay reviewing those patches.\n\nOn Sun, 21 Sep 2008, Giuseppe Bilotta wrote:\n\n> When using path info, reduce the number of CGI parameters by embedding\n> the action in the path. The typicial cgiweb path is thus\n> $project/$action/$hash_base:$file_name or $project/$action/$hash\n\ncgiweb?\n\n> \n> This is mostly backwards compatible with the old-style gitweb paths,\n> except when $project/$branch was used to access a branch whose name\n> matches a gitweb action.\n\nI would also state that gitweb _generates_ such pathinfo links if\nconfigured (or if coming from pahinfo URL), and that this change\nallow to represent wider number of gitweb links (gitweb URLs) in\npathinfo form.\n\nAlso, from what I understand, generated pathinfo links now always\nuse action, so they are a tiny little bit longer.\n\n> Signed-off-by: Giuseppe Bilotta \n> ---\n> gitweb/gitweb.perl | 109 ++++++++++++++++++++++++++++++++++------------------\n> 1 files changed, 72 insertions(+), 37 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index da474d0..e783d12 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -488,6 +488,37 @@ if (defined $searchtext) {\n> \t$search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext;\n> }\n> \n> +# dispatch\n> +my %actions = (\n> +\t\"blame\" => \\&git_blame,\n> +\t\"blobdiff\" => \\&git_blobdiff,\n> +\t\"blobdiff_plain\" => \\&git_blobdiff_plain,\n> +\t\"blob\" => \\&git_blob,\n> +\t\"blob_plain\" => \\&git_blob_plain,\n> +\t\"commitdiff\" => \\&git_commitdiff,\n> +\t\"commitdiff_plain\" => \\&git_commitdiff_plain,\n> +\t\"commit\" => \\&git_commit,\n> +\t\"forks\" => \\&git_forks,\n> +\t\"heads\" => \\&git_heads,\n> +\t\"history\" => \\&git_history,\n> +\t\"log\" => \\&git_log,\n> +\t\"rss\" => \\&git_rss,\n> +\t\"atom\" => \\&git_atom,\n> +\t\"search\" => \\&git_search,\n> +\t\"search_help\" => \\&git_search_help,\n> +\t\"shortlog\" => \\&git_shortlog,\n> +\t\"summary\" => \\&git_summary,\n> +\t\"tag\" => \\&git_tag,\n> +\t\"tags\" => \\&git_tags,\n> +\t\"tree\" => \\&git_tree,\n> +\t\"snapshot\" => \\&git_snapshot,\n> +\t\"object\" => \\&git_object,\n> +\t# those below don't need $project\n> +\t\"opml\" => \\&git_opml,\n> +\t\"project_list\" => \\&git_project_list,\n> +\t\"project_index\" => \\&git_project_index,\n> +);\n> +\n\nThis is as I understand simply moving %actions hash around?\nWell, you could instead split hash declaration from defining it,\nin the form of\n\n my %actions = ();\n ...\n %actions = (\n ...\n );\n\nbut I guess moving declaration earlier is good solution.\n\n> # now read PATH_INFO and use it as alternative to parameters\n> sub evaluate_path_info {\n> \treturn if defined $project;\n> @@ -512,6 +543,16 @@ sub evaluate_path_info {\n> \t# do not change any parameters if an action is given using the query string\n> \treturn if $action;\n> \t$path_info =~ s,^\\Q$project\\E/*,,;\n> +\n> +\t# next comes the action\n> +\t$action = $path_info;\n> +\t$action =~ s,/.*$,,;\n\nI would use perhaps \"$action = ($path_info =~ m!^([^/]+)/!;\"\nBut that is Perl, so TIMTOWDI.\n\n> +\tif (exists $actions{$action}) {\n> +\t\t$path_info =~ s,^\\Q$action\\E/*,,;\n> +\t} else {\n> +\t\t$action = undef;\n> +\t}\n> +\n\nI don't think you need quoting pattern metacharacters; valid actions\nshould not contain regexp metacharacters. Defensive programming?\n\n> \tmy ($refname, $pathname) = split(/:/, $path_info, 2);\n> \tif (defined $pathname) {\n> \t\t# we got \"project.git/branch:filename\" or \"project.git/branch:dir/\"\n> @@ -525,10 +566,12 @@ sub evaluate_path_info {\n> \t\t}\n> \t\t$hash_base ||= validate_refname($refname);\n> \t\t$file_name ||= validate_pathname($pathname);\n> +\t\t$hash ||= git_get_hash_by_path($hash_base, $file_name);\n\nI don't understand why you feel that you need to do this (this is\nadditional git command fork, as git_get_hash_by_path calls Git, to\nbe more exact it calls git-ls-tree (it could call git-rev-parse\ninstead). Moreover, I don't understand why you need to do this _here_,\ninstead of just before where you would have to have $hash variable set.\n\n> \t} elsif (defined $refname) {\n> \t\t# we got \"project.git/branch\"\n> -\t\t$action ||= \"shortlog\";\n> -\t\t$hash ||= validate_refname($refname);\n> +\t\t$action ||= \"shortlog\";\n> +\t\t$hash ||= validate_refname($refname);\n> +\t\t$hash_base ||= validate_refname($refname);\n> \t}\n> }\n> evaluate_path_info();\n> @@ -624,8 +636,13 @@ sub href (%) {\n> \tif ($params{-replay}) {\n> \t\twhile (my ($name, $symbol) = each %mapping) {\n> \t\t\tif (!exists $params{$name}) {\n> -\t\t\t\t# to allow for multivalued params we use arrayref form\n> -\t\t\t\t$params{$name} = [ $cgi->param($symbol) ];\n> +\t\t\t\tif ($cgi->param($symbol)) {\n> +\t\t\t\t\t# to allow for multivalued params we use arrayref form\n> +\t\t\t\t\t$params{$name} = [ $cgi->param($symbol) ];\n> +\t\t\t\t} else {\n> +\t\t\t\t\tno strict 'refs';\n> +\t\t\t\t\t$params{$name} = $$name if $$name;\n> +\t\t\t\t}\n> \t\t\t}\n> \t\t}\n> \t}\n\nWhat this change is about? And why this change is _here_, in this\ncommit? It is I think unrelated, and wrong change. \n\nhref(..., -replay=>1) is all about reusing current URL, perhaps with\na few parameters changed, like for example pagination links differ only\nin page number param change. For example if we had only hb= and f=\nparameters, -replay=>1 links should use only those, and not add h=\nparameter because somewhere we felt that we need $hash to be calculated.\n\n> @@ -636,10 +653,28 @@ sub href (%) {\n\nThis hunk is about generating pathinfo URL, isn't it?\n\nYou probably would want to change comment at the top of this part\nof href() subroutine, namely\n\n \tif ($use_pathinfo) {\n\t\t# use PATH_INFO for project name\n\nas you now try to use PATH_INFO for more than project name. Please do\nnot leave comments to get out of sync with the code.\n\n> \t\t$href .= \"/\".esc_url($params{'project'}) if defined $params{'project'};\n> \t\tdelete $params{'project'};\n> \n> -\t\t# Summary just uses the project path URL\n> -\t\tif (defined $params{'action'} && $params{'action'} eq 'summary') {\n> +\t\t# Summary just uses the project path URL, any other action come next\n> +\t\t# in the URL\n> +\t\tif (defined $params{'action'}) {\n> +\t\t\t$href .= \"/\".esc_url($params{'action'}) unless $params{'action'} eq 'summary';\n> \t\t\tdelete $params{'action'};\n> \t\t}\n> +\n> +\t\t# next, we put either hash_base:file_name or hash\n> +\t\tif (defined $params{'hash_base'}) {\n> +\t\t\t$href .= \"/\".esc_url($params{'hash_base'});\n> +\t\t\tif (defined $params{'file_name'}) {\n> +\t\t\t\t$href .= \":\".esc_url($params{'file_name'});\n> +\t\t\t\tdelete $params{'hash'} if $params{'hash'} eq git_get_hash_by_path($params{'hash_base'},$params{'file_name'});\n\nFirst, this page has around 140 characters. That is too long, much too long.\nPlease try to wrap code around 80-characters.\n\nSecond, following Petr 'Pasky' Baudis suggestion of reducing complexity\nand shortening gitweb URLs, we could unconditionally remove redundant\n'hash' parameter if we have both 'hash_base' and 'file_name'\nparameters. This would also simplify and speed up (lack of extra fork)\ngitweb code.\n\n> +\t\t\t\tdelete $params{'file_name'};\n> +\t\t\t} else {\n> +\t\t\t\tdelete $params{'hash'} if $params{'hash'} eq $params{'hash_base'};\n> +\t\t\t}\n> +\t\t\tdelete $params{'hash_base'};\n> +\t\t} elsif (defined $params{'hash'}) {\n> +\t\t\t$href .= \"/\".esc_url($params{'hash'});\n> +\t\t\tdelete $params{'hash'};\n> +\t\t}\n\nO.K.... I think.\n\nDid you test this, preferably also using Mechanize test, with gitweb\nconfiguration turning path_info on by default.?\n\n> \t}\n> \n> \t# now encode the parameters explicitly\n\nHmmm... now I am not so sure if it wouldn't be better to split this\npatch in pathinfo parsing and pathinfo generation. The first part\nwould be obvious, the second part would be smaller and easier to review.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"91820","messageId":"200809290308.09312.jnareb@gmail.com","threadId":"15618","inReplyTo":"1222030663-22540-3-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCH 2/6] gitweb: use_pathinfo filenames start with /","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-09-29T01:08:08Z","receivedAt":"2008-09-29T01:08:08Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sun, 21 Sep 2008, Giuseppe Bilotta wrote:\n\n> When using path info, make filenames start with a / (right after the :\n> that separates them from the hash base). This minimal change allows\n> relative navigation to work properly when viewing HTML files.\n\nGood idea. Nitpick: instead of \"using path info\", perhaps \"generating\npath info URL\"; this change is about gitweb link generation...\n\nDid you check if gitweb strips leading '/' from filename?\n\n> Signed-off-by: Giuseppe Bilotta \n> ---\n> gitweb/gitweb.perl | 2 +-\n> 1 files changed, 1 insertions(+), 1 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index e783d12..18da484 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -664,7 +664,7 @@ sub href (%) {\n> \t\tif (defined $params{'hash_base'}) {\n> \t\t\t$href .= \"/\".esc_url($params{'hash_base'});\n> \t\t\tif (defined $params{'file_name'}) {\n> -\t\t\t\t$href .= \":\".esc_url($params{'file_name'});\n> +\t\t\t\t$href .= \":/\".esc_url($params{'file_name'});\n> \t\t\t\tdelete $params{'hash'} if $params{'hash'} eq git_get_hash_by_path($params{'hash_base'},$params{'file_name'});\n> \t\t\t\tdelete $params{'file_name'};\n> \t\t\t} else {\n> -- \n> 1.5.6.5\n\nIs there reason why this change is separate (not squashed) from\nprevious commit?\n\n-- \nJakub Narebski\nPoland\n"},{"id":"91829","messageId":"200809291033.34588.jnareb@gmail.com","threadId":"15618","inReplyTo":"1222030663-22540-6-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCH 5/6] gitweb: remove PATH_INFO from $my_url and $my_uri","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-09-29T08:33:33Z","receivedAt":"2008-09-29T08:33:33Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sun, 21 Sep 2008, Giuseppe Bilotta wrote:\n\n> This patch (combined with appropriate server configuration) allows usage\n> of the gitweb CGI script as DirectoryIndex for the server root even when\n> the pathinfo feature is enabled.\n>\n\nThis is IMHO a bugfix for a path_info handling bug, for which there\nwas an ugly workaround of specifying base URL ($my_url and $my_uri)\nexplicitly in gitweb configuration (GITWEB_CONFIG).\n\nTherefore I think that this patch should have been sent outside of\nthe rest of \"new path_info features\" series, as a separate single\npatch, and now that it is there it really should be applied, perhaps\neven to the 'maint' branch.\n\n> Signed-off-by: Giuseppe Bilotta \n\nAcked-by: Jakub Narebski \n(for what is worth)\n\n> ---\n> gitweb/gitweb.perl | 4 ++++\n> 1 files changed, 4 insertions(+), 0 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 0dd2526..4a91d07 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -26,6 +26,10 @@ our $cgi = new CGI;\n> our $version = \"++GIT_VERSION++\";\n> our $my_url = $cgi->url();\n> our $my_uri = $cgi->url(-absolute => 1);\n\nPerhaps some comment here?\n\n> +if (my $path_info = $ENV{\"PATH_INFO\"}) {\n> +\t$my_url =~ s,$path_info$,,;\n> +\t$my_uri =~ s,$path_info$,,;\n\n+\t$my_url =~ s,\\Q$path_info\\E$,,;\n+\t$my_uri =~ s,\\Q$path_info\\E$,,;\n\nJust in case.\n\n> +}\n> \n> # core git executable to use\n> # this can just be \"git\" if your webserver has a sensible PATH\n\nI was wondering if $path_info should be global variable, but then\nI checked that $path_info is local to evaluate_path_info() subroutine.\nSo it is good as it is now, but with quoting regular expression\nmetacharacters.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"91836","messageId":"cb7bb73a0809290605o106a34bbxc3e5e48f4b1a45f4@mail.gmail.com","threadId":"15618","inReplyTo":"200809291033.34588.jnareb@gmail.com","subject":"Re: [PATCH 5/6] gitweb: remove PATH_INFO from $my_url and $my_uri","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-09-29T13:05:16Z","receivedAt":"2008-09-29T13:05:16Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Mon, Sep 29, 2008 at 10:33 AM, Jakub Narebski wrote:\n> On Sun, 21 Sep 2008, Giuseppe Bilotta wrote:\n>\n>> This patch (combined with appropriate server configuration) allows usage\n>> of the gitweb CGI script as DirectoryIndex for the server root even when\n>> the pathinfo feature is enabled.\n>>\n>\n> This is IMHO a bugfix for a path_info handling bug, for which there\n> was an ugly workaround of specifying base URL ($my_url and $my_uri)\n> explicitly in gitweb configuration (GITWEB_CONFIG).\n>\n> Therefore I think that this patch should have been sent outside of\n> the rest of \"new path_info features\" series, as a separate single\n> patch, and now that it is there it really should be applied, perhaps\n> even to the 'maint' branch.\n\nOk, I'll resend it separately, with the suggested fix.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"91839","messageId":"cb7bb73a0809290712g324ec015r70fd868b91673645@mail.gmail.com","threadId":"15618","inReplyTo":"200809290308.09312.jnareb@gmail.com","subject":"Re: [PATCH 2/6] gitweb: use_pathinfo filenames start with /","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-09-29T14:12:28Z","receivedAt":"2008-09-29T14:12:28Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Mon, Sep 29, 2008 at 3:08 AM, Jakub Narebski wrote:\n> On Sun, 21 Sep 2008, Giuseppe Bilotta wrote:\n>\n>> When using path info, make filenames start with a / (right after the :\n>> that separates them from the hash base). This minimal change allows\n>> relative navigation to work properly when viewing HTML files.\n>\n> Good idea. Nitpick: instead of \"using path info\", perhaps \"generating\n> path info URL\"; this change is about gitweb link generation...\n\nRight.\n\n> Did you check if gitweb strips leading '/' from filename?\n\nYes.\n\n>> Signed-off-by: Giuseppe Bilotta \n>> ---\n>> gitweb/gitweb.perl | 2 +-\n>> 1 files changed, 1 insertions(+), 1 deletions(-)\n>>\n>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>> index e783d12..18da484 100755\n>> --- a/gitweb/gitweb.perl\n>> +++ b/gitweb/gitweb.perl\n>> @@ -664,7 +664,7 @@ sub href (%) {\n>> if (defined $params{'hash_base'}) {\n>> $href .= \"/\".esc_url($params{'hash_base'});\n>> if (defined $params{'file_name'}) {\n>> - $href .= \":\".esc_url($params{'file_name'});\n>> + $href .= \":/\".esc_url($params{'file_name'});\n>> delete $params{'hash'} if $params{'hash'} eq git_get_hash_by_path($params{'hash_base'},$params{'file_name'});\n>> delete $params{'file_name'};\n>> } else {\n>> --\n>> 1.5.6.5\n>\n> Is there reason why this change is separate (not squashed) from\n> previous commit?\n\nHistorical reason (i.e. I came up with the idea later on). I'll squash it.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"91841","messageId":"cb7bb73a0809290722w5ed92171v98d6b83a7dae8f8b@mail.gmail.com","threadId":"15618","inReplyTo":"200809290303.21290.jnareb@gmail.com","subject":"Re: [PATCH 1/6] gitweb: action in path with use_pathinfo","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-09-29T14:22:40Z","receivedAt":"2008-09-29T14:22:40Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Mon, Sep 29, 2008 at 3:03 AM, Jakub Narebski wrote:\n> I'm sorry for the delay reviewing those patches.\n>\n> On Sun, 21 Sep 2008, Giuseppe Bilotta wrote:\n>\n>> When using path info, reduce the number of CGI parameters by embedding\n>> the action in the path. The typicial cgiweb path is thus\n>> $project/$action/$hash_base:$file_name or $project/$action/$hash\n>\n> cgiweb?\n\nGood question. I have no idea why I wrote that instad of gitweb.\n\n>> This is mostly backwards compatible with the old-style gitweb paths,\n>> except when $project/$branch was used to access a branch whose name\n>> matches a gitweb action.\n>\n> I would also state that gitweb _generates_ such pathinfo links if\n> configured (or if coming from pahinfo URL), and that this change\n> allow to represent wider number of gitweb links (gitweb URLs) in\n> pathinfo form.\n\nCan do.\n\n> Also, from what I understand, generated pathinfo links now always\n> use action, so they are a tiny little bit longer.\n\nIs that a problem, by the way? I've had half-thoughts about making the\naction implicit when possible, but I'm afraid that's prone to make the\ncode way more complex and the path info handling much less robust.\n\n>> Signed-off-by: Giuseppe Bilotta \n>> ---\n>> gitweb/gitweb.perl | 109 ++++++++++++++++++++++++++++++++++------------------\n>> 1 files changed, 72 insertions(+), 37 deletions(-)\n>>\n>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>> index da474d0..e783d12 100755\n>> --- a/gitweb/gitweb.perl\n>> +++ b/gitweb/gitweb.perl\n>> @@ -488,6 +488,37 @@ if (defined $searchtext) {\n>> $search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext;\n>> }\n>>\n>> +# dispatch\n>> +my %actions = (\n>> + \"blame\" => \\&git_blame,\n>> + \"blobdiff\" => \\&git_blobdiff,\n>> + \"blobdiff_plain\" => \\&git_blobdiff_plain,\n>> + \"blob\" => \\&git_blob,\n>> + \"blob_plain\" => \\&git_blob_plain,\n>> + \"commitdiff\" => \\&git_commitdiff,\n>> + \"commitdiff_plain\" => \\&git_commitdiff_plain,\n>> + \"commit\" => \\&git_commit,\n>> + \"forks\" => \\&git_forks,\n>> + \"heads\" => \\&git_heads,\n>> + \"history\" => \\&git_history,\n>> + \"log\" => \\&git_log,\n>> + \"rss\" => \\&git_rss,\n>> + \"atom\" => \\&git_atom,\n>> + \"search\" => \\&git_search,\n>> + \"search_help\" => \\&git_search_help,\n>> + \"shortlog\" => \\&git_shortlog,\n>> + \"summary\" => \\&git_summary,\n>> + \"tag\" => \\&git_tag,\n>> + \"tags\" => \\&git_tags,\n>> + \"tree\" => \\&git_tree,\n>> + \"snapshot\" => \\&git_snapshot,\n>> + \"object\" => \\&git_object,\n>> + # those below don't need $project\n>> + \"opml\" => \\&git_opml,\n>> + \"project_list\" => \\&git_project_list,\n>> + \"project_index\" => \\&git_project_index,\n>> +);\n>> +\n>\n> This is as I understand simply moving %actions hash around?\n\nYes.\n\n> Well, you could instead split hash declaration from defining it,\n> in the form of\n>\n> my %actions = ();\n> ...\n> %actions = (\n> ...\n> );\n>\n> but I guess moving declaration earlier is good solution.\n\nIs there some coding style recommendation wrt this situations, or is\nit just a matter of making the patch smaller?\n\n>> # now read PATH_INFO and use it as alternative to parameters\n>> sub evaluate_path_info {\n>> return if defined $project;\n>> @@ -512,6 +543,16 @@ sub evaluate_path_info {\n>> # do not change any parameters if an action is given using the query string\n>> return if $action;\n>> $path_info =~ s,^\\Q$project\\E/*,,;\n>> +\n>> + # next comes the action\n>> + $action = $path_info;\n>> + $action =~ s,/.*$,,;\n>\n> I would use perhaps \"$action = ($path_info =~ m!^([^/]+)/!;\"\n> But that is Perl, so TIMTOWDI.\n\nWell, Perl is not my native language so I tend to stay away from\ncomplex expressions if possible ;-)\n\n>> + if (exists $actions{$action}) {\n>> + $path_info =~ s,^\\Q$action\\E/*,,;\n>> + } else {\n>> + $action = undef;\n>> + }\n>> +\n>\n> I don't think you need quoting pattern metacharacters; valid actions\n> should not contain regexp metacharacters. Defensive programming?\n\nYou're right, in this case it's probably excessive. I'll strip it out.\n\n>> my ($refname, $pathname) = split(/:/, $path_info, 2);\n>> if (defined $pathname) {\n>> # we got \"project.git/branch:filename\" or \"project.git/branch:dir/\"\n>> @@ -525,10 +566,12 @@ sub evaluate_path_info {\n>> }\n>> $hash_base ||= validate_refname($refname);\n>> $file_name ||= validate_pathname($pathname);\n>> + $hash ||= git_get_hash_by_path($hash_base, $file_name);\n>\n> I don't understand why you feel that you need to do this (this is\n> additional git command fork, as git_get_hash_by_path calls Git, to\n> be more exact it calls git-ls-tree (it could call git-rev-parse\n> instead). Moreover, I don't understand why you need to do this _here_,\n> instead of just before where you would have to have $hash variable set.\n\nHm. I must confess that I honestly don't remember. The same holds for\nthe other chunks you have perplexities on. When I started writing\nthese patches I came across a few situations where $hash wouldn't\ncarry over properly, but now I can't seem to recreate those issues\nanymore, which leads me to suspect it was a problem with hand-crafted\nlinks (i.e. before I coded the link generation part too). I'll resend\nwithout these chunks.\n\n\n> > @@ -624,8 +636,13 @@ sub href (%) {\n> > if ($params{-replay}) {\n> > while (my ($name, $symbol) = each %mapping) {\n> > if (!exists $params{$name}) {\n> > - # to allow for multivalued params we use arrayref form\n> > - $params{$name} = [ $cgi->param($symbol) ];\n> > + if ($cgi->param($symbol)) {\n> > + # to allow for multivalued params we use arrayref form\n> > + $params{$name} = [ $cgi->param($symbol) ];\n> > + } else {\n> > + no strict 'refs';\n> > + $params{$name} = $$name if $$name;\n> > + }\n> > }\n> > }\n> > }\n>\n> What this change is about? And why this change is _here_, in this\n> commit? It is I think unrelated, and wrong change.\n\nThis is about being able to recycle CGI parameters that came through\nas part of path_info instead of the CGI parameter list. It might not\nbe the best way to recover it, though. I *did* have a few thoughts\nabout an alternative way that consisted of build a parameter list\nmerging CGI and path-info parameter, but since this approach seemed to\nwork, I went with it.\n\n> href(..., -replay=>1) is all about reusing current URL, perhaps with\n> a few parameters changed, like for example pagination links differ only\n> in page number param change. For example if we had only hb= and f=\n> parameters, -replay=>1 links should use only those, and not add h=\n> parameter because somewhere we felt that we need $hash to be calculated.\n\nAssume for example that you are to an url such as\n\nhttp://git.oblomov.eu/git/tree/refs/remotes/origin/master:gitweb\n\nWithout this patch, the 'history' link on the second header line links\nto ARRAY(0xblah)ARRAY(0xblah). With this patch, it shows the proper\nlink. So either replay is being abused somewhere in the link\ngeneration code, or this CGI+path_info parameter retrieval is\nnecessary, one way or the other.\n\n>> @@ -636,10 +653,28 @@ sub href (%) {\n>\n> This hunk is about generating pathinfo URL, isn't it?\n\nYes.\n\n> You probably would want to change comment at the top of this part\n> of href() subroutine, namely\n>\n> if ($use_pathinfo) {\n> # use PATH_INFO for project name\n>\n> as you now try to use PATH_INFO for more than project name. Please do\n> not leave comments to get out of sync with the code.\n\nRight.\n\n>> $href .= \"/\".esc_url($params{'project'}) if defined $params{'project'};\n>> delete $params{'project'};\n>>\n>> - # Summary just uses the project path URL\n>> - if (defined $params{'action'} && $params{'action'} eq 'summary') {\n>> + # Summary just uses the project path URL, any other action come next\n>> + # in the URL\n>> + if (defined $params{'action'}) {\n>> + $href .= \"/\".esc_url($params{'action'}) unless $params{'action'} eq 'summary';\n>> delete $params{'action'};\n>> }\n>> +\n>> + # next, we put either hash_base:file_name or hash\n>> + if (defined $params{'hash_base'}) {\n>> + $href .= \"/\".esc_url($params{'hash_base'});\n>> + if (defined $params{'file_name'}) {\n>> + $href .= \":\".esc_url($params{'file_name'});\n>> + delete $params{'hash'} if $params{'hash'} eq git_get_hash_by_path($params{'hash_base'},$params{'file_name'});\n>\n> First, this page has around 140 characters. That is too long, much too long.\n> Please try to wrap code around 80-characters.\n>\n> Second, following Petr 'Pasky' Baudis suggestion of reducing complexity\n> and shortening gitweb URLs, we could unconditionally remove redundant\n> 'hash' parameter if we have both 'hash_base' and 'file_name'\n> parameters. This would also simplify and speed up (lack of extra fork)\n> gitweb code.\n\nIf it's indeed guaranteed that hash is not needed in these cases, it's\nsurely the best course of action. I changed the code to that effect.\n\n>> + delete $params{'file_name'};\n>> + } else {\n>> + delete $params{'hash'} if $params{'hash'} eq $params{'hash_base'};\n>> + }\n>> + delete $params{'hash_base'};\n>> + } elsif (defined $params{'hash'}) {\n>> + $href .= \"/\".esc_url($params{'hash'});\n>> + delete $params{'hash'};\n>> + }\n>\n> O.K.... I think.\n>\n> Did you test this, preferably also using Mechanize test, with gitweb\n> configuration turning path_info on by default.?\n\nThe whole patchset is running both on my machine on\nhttp://git.oblomov.eu and on the rbot repository\nhttp://ruby-rbot.org/gitweb (older version there, though). I have no\nidea how to run automated Mechanize tests though.\n\n>> }\n>>\n>> # now encode the parameters explicitly\n>\n> Hmmm... now I am not so sure if it wouldn't be better to split this\n> patch in pathinfo parsing and pathinfo generation. The first part\n> would be obvious, the second part would be smaller and easier to review.\n\nOk, I'll split parsing from generation. Since it's what I did for\nsubsequent extensions (such as the parent..current thing) it also fits\nnicely with the patchflow.\n\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"91866","messageId":"200809292012.09460.jnareb@gmail.com","threadId":"15618","inReplyTo":"1222030663-22540-7-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCH 6/6] gitweb: prevent double slashes in PATH_INFO hrefs","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-09-29T18:12:08Z","receivedAt":"2008-09-29T18:12:08Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sun, 21 Sep 2008, Giuseppe Bilotta wrote:\n\n> When using PATH_INFO in combination with a rewrite rule that hides the\n> cgi script name, links to projects and/or actions without projects might\n> be generated with a double slash.\n> \n\nYou mean here that base URL ends with '/'?\n\n> Fix by removing the trailing slash (if present) from $href before\n> appending PATH_INFO data.\n> \n> Signed-off-by: Giuseppe Bilotta \n\nAcked-by: Jakub Narebski \n\nThis is a good change, and worth applying even before the rest of\nseries (which probably would go through a few rounds of review).\nI'm not sure if it applies cleanly, but conceptually it does not\ndepend on the rest of patches in this series.\n\n> ---\n> gitweb/gitweb.perl | 2 ++\n> 1 files changed, 2 insertions(+), 0 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 4a91d07..ebab86b 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -675,6 +675,8 @@ sub href (%) {\n> \n> \tmy ($use_pathinfo) = gitweb_check_feature('pathinfo');\n> \tif ($use_pathinfo) {\n> +\t\t$href =~ s,/$,,;\n> +\n> \t\t# use PATH_INFO for project name\n> \t\t$href .= \"/\".esc_url($params{'project'}) if defined $params{'project'};\n> \t\tdelete $params{'project'};\n> -- \n> 1.5.6.5\n> \n> \n\nShould not go wrong...\n\n-- \nJakub Narebski\nPoland\n"},{"id":"91885","messageId":"200809300120.02492.jnareb@gmail.com","threadId":"15618","inReplyTo":"cb7bb73a0809290712g324ec015r70fd868b91673645@mail.gmail.com","subject":"Re: [PATCH 2/6] gitweb: use_pathinfo filenames start with /","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-09-29T23:20:01Z","receivedAt":"2008-09-29T23:20:01Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, 29 Sep 2008, Giuseppe Bilotta wrote:\n> On Mon, Sep 29, 2008 at 3:08 AM, Jakub Narebski wrote:\n>> On Sun, 21 Sep 2008, Giuseppe Bilotta wrote:\n\n>>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>>> index e783d12..18da484 100755\n>>> --- a/gitweb/gitweb.perl\n>>> +++ b/gitweb/gitweb.perl\n>>> @@ -664,7 +664,7 @@ sub href (%) {\n>>> if (defined $params{'hash_base'}) {\n>>> $href .= \"/\".esc_url($params{'hash_base'});\n>>> if (defined $params{'file_name'}) {\n>>> - $href .= \":\".esc_url($params{'file_name'});\n>>> + $href .= \":/\".esc_url($params{'file_name'});\n>>> delete $params{'hash'} if $params{'hash'} eq git_get_hash_by_path($params{'hash_base'},$params{'file_name'});\n>>> delete $params{'file_name'};\n>>> } else {\n>>> --\n>>> 1.5.6.5\n>>\n>> Is there reason why this change is separate (not squashed) from\n>> previous commit?\n> \n> Historical reason (i.e. I came up with the idea later on). I'll squash it.\n\nHn. Now I am not sure if it should be squashed, or should be separate.\n\n\nGood change.\n\nPlease don't forget to describe this decision, i.e. why gitweb is\nusing \"branch:/filename\" when creating path_info links instead of\nsimple \"branch:name\" for relative URLs in HTML in 'raw' ('blob_plain')\nview, in the commit message. Thanks in advance.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"91889","messageId":"200809300221.25094.jnareb@gmail.com","threadId":"15618","inReplyTo":"cb7bb73a0809290722w5ed92171v98d6b83a7dae8f8b@mail.gmail.com","subject":"Re: [PATCH 1/6] gitweb: action in path with use_pathinfo","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-09-30T00:21:24Z","receivedAt":"2008-09-30T00:21:24Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, 29 Sep 2008, Giuseppe Bilotta wrote:\n> On Mon, Sep 29, 2008 at 3:03 AM, Jakub Narebski wrote:\n>> On Sun, 21 Sep 2008, Giuseppe Bilotta wrote:\n\n>> Also, from what I understand, generated pathinfo links now always\n>> use action, so they are a tiny little bit longer.\n> \n> Is that a problem, by the way? I've had half-thoughts about making the\n> action implicit when possible, but I'm afraid that's prone to make the\n> code way more complex and the path info handling much less robust.\n\nNo, I don't think there is a problem; if I remember correctly action\nis omitted for default actions with and without project, i.e. for\nprojects list, and for 'summary' view for a project, which is default\nview in absence of other parameters.\n\nI would explain difference between then and now in the patch adding\nsupport for _creating_ wider range of path_info links (I don't know,\nperhaps you did that in new version):\n * path_info URL were always without action, and were possible only\n for the case of default action (projects list, summary), and in the\n case of implicit action ('tree' for trees i.e. filename ending in\n '/'; 'blob_plain' for ordinary files i.e. filename but no '/' at end;\n 'shortlog' for bare ref, assuming branch).\n * now that pathinfo can contain action, wider range of URL can be done\n as purely path_info links; in other way more of parameters can be put\n in path_info part.\n\nPerhaps not in so large and detailed form... I guess explanation of\nusing ':/' as separator should be put there as well, if you plan to\nsquash those patches.\n \n>>> ---\n>>> gitweb/gitweb.perl | 109 ++++++++++++++++++++++++++++++++++------------------\n>>> 1 files changed, 72 insertions(+), 37 deletions(-)\n>>>\n>>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>>> index da474d0..e783d12 100755\n>>> --- a/gitweb/gitweb.perl\n>>> +++ b/gitweb/gitweb.perl\n\n>> Well, you could instead split hash declaration from defining it,\n>> in the form of\n>>\n>> my %actions = ();\n>> ...\n>> %actions = (\n>> ...\n>> );\n>>\n>> but I guess moving declaration earlier is good solution.\n> \n> Is there some coding style recommendation wrt this situations, or is\n> it just a matter of making the patch smaller?\n\nI think that moving %actions earlier is a better solution.\n \n>>> # now read PATH_INFO and use it as alternative to parameters\n>>> sub evaluate_path_info {\n>>> return if defined $project;\n>>> @@ -512,6 +543,16 @@ sub evaluate_path_info {\n>>> # do not change any parameters if an action is given using the query string\n>>> return if $action;\n>>> $path_info =~ s,^\\Q$project\\E/*,,;\n>>> +\n>>> + # next comes the action\n>>> + $action = $path_info;\n>>> + $action =~ s,/.*$,,;\n>>\n>> I would use perhaps \"($action) = ($path_info =~ m!^([^/]+)!);\"\n>> But that is Perl, so TIMTOWDI.\n> \n> Well, Perl is not my native language so I tend to stay away from\n> complex expressions if possible ;-)\n\nWhat I meant is instead of \"copy and strip\" use \"find match\".\nI tried to use one-liner, but it could be written instead as:\n\n+ # next comes the action\n+ if ($path_info =~ m!^([^/]+)!) {;\n+ \t $action = $1;\n+ }\n\nBut I guess your approach is equally valid. I don't think of myself\nbeing a Perl expert, either.\n\n>>> @@ -525,10 +566,12 @@ sub evaluate_path_info {\n>>> }\n>>> $hash_base ||= validate_refname($refname);\n>>> $file_name ||= validate_pathname($pathname);\n>>> + $hash ||= git_get_hash_by_path($hash_base, $file_name);\n>>\n>> I don't understand why you feel that you need to do this (this is\n>> additional git command fork, as git_get_hash_by_path calls Git, to\n>> be more exact it calls git-ls-tree (it could call git-rev-parse\n>> instead). Moreover, I don't understand why you need to do this _here_,\n>> instead of just before where you would have to have $hash variable set.\n> \n> Hm. I must confess that I honestly don't remember. The same holds for\n> the other chunks you have perplexities on. When I started writing\n> these patches I came across a few situations where $hash wouldn't\n> carry over properly, but now I can't seem to recreate those issues\n> anymore, which leads me to suspect it was a problem with hand-crafted\n> links (i.e. before I coded the link generation part too). I'll resend\n> without these chunks.\n\nThat is the problem, but not as large a problem as having similar code\ncalling git_get_hash_by_path() during link generation, in href(...)\nsubroutine (at least without Lea's gitweb caching, the part that reuses\n\"git cat-file --batch\"). This is called once per view (page); that\nwas called once per generated gitweb link...\n \n>>> @@ -624,8 +636,13 @@ sub href (%) {\n>>> if ($params{-replay}) {\n>>> while (my ($name, $symbol) = each %mapping) {\n>>> if (!exists $params{$name}) {\n>>> - # to allow for multivalued params we use arrayref form\n>>> - $params{$name} = [ $cgi->param($symbol) ];\n>>> + if ($cgi->param($symbol)) {\n>>> + # to allow for multivalued params we use arrayref form\n>>> + $params{$name} = [ $cgi->param($symbol) ];\n>>> + } else {\n>>> + no strict 'refs';\n>>> + $params{$name} = $$name if $$name;\n>>> + }\n>>> }\n>>> }\n>>> }\n>>\n>> What this change is about? And why this change is _here_, in this\n>> commit? It is I think unrelated, and wrong change.\n> \n> This is about being able to recycle CGI parameters that came through\n> as part of path_info instead of the CGI parameter list. It might not\n> be the best way to recover it, though. I *did* have a few thoughts\n> about an alternative way that consisted of build a parameter list\n> merging CGI and path-info parameter, but since this approach seemed to\n> work, I went with it.\n\nFact, I have totally forgot about this.\n\n>> href(..., -replay=>1) is all about reusing current URL, perhaps with\n>> a few parameters changed, like for example pagination links differ only\n>> in page number param change. For example if we had only hb= and f=\n>> parameters, -replay=>1 links should use only those, and not add h=\n>> parameter because somewhere we felt that we need $hash to be calculated.\n> \n> Assume for example that you are to an url such as\n> \n> http://git.oblomov.eu/git/tree/refs/remotes/origin/master:gitweb\n> \n> Without this patch, the 'history' link on the second header line links\n> to ARRAY(0xblah)ARRAY(0xblah). With this patch, it shows the proper\n> link. So either replay is being abused somewhere in the link\n> generation code, or this CGI+path_info parameter retrieval is\n> necessary, one way or the other.\n\nAh. Now I understand.\n\nWhen creating code for href(..., -replay=>1), which by the way I thought\nwould be more useful than actually is, I have forgot that parameters to\ngitweb could be passed in other way that through CGI parameters\n(CGI query)[1].\n\nUsing\n\n\t$params{$name} = [ $cgi->param($symbol) ];\n\nis a cute hack, but it doesn't work for arguments passed via path_info\n(was: project, hash_base and file_name; while now it is project, action,\nhash_base (in full) and file_name).\n\n\nThe solution I thought about and abandoned in favor of this cute hack\nwas to have additional hash (in addition to %mapping), which would map\naction names to references to variables holding the value for parameter.\n\nThis has the same problem as your proposed solution of putting some\nparameters which didn't come from URL but were filled from other info.\n$hash parameter is most likely to be culprit here.\n\nOn the other hand it is more generic and doesn't rely on knowledge that\nthere is no multi-valued parameter which can be expressed in path_info\n(currently only 'opt' parameter can be multi-valued, and requires\narrayref, but also 'opt' parameter is and cannot be put in path_info).\n\nI am talking there about the following solution:\n\n\tmy %action_vars = (\n\t\tproject => \\$project,\n\t\taction => \\$action,\n # ...\n\t\textra_options => \\@extra_options,\n\t);\n # ...\n while (my ($name, $symbol) = each %mapping) {\n if (!exists $params{$name}) {\n $params{$name} = ${$action_vars{$name}};\n }\n }\n\n\nThis avoids cure hack of (from your code)\n\n } else {\n no strict 'refs';\n $params{$name} = $$name if $$name;\n }\n\nI think that gitweb should use single source, not CGI query parameters\nor variable saving [sanitized] value.\n\n\n[*1*] Currently parameters can be passed either as CGI query parameters\n (which I remember about), but also (with some restrictions) in the\n path_info part of gitweb URLs. If we implement command line\n switches (for example to generate directory listing in format\n expected by gitweb for file $projects_list, or for off-line\n generation of RSS feed), there could be yet another source.\n\n>>> @@ -636,10 +653,28 @@ sub href (%) {\n\n>>> + # next, we put either hash_base:file_name or hash\n>>> + if (defined $params{'hash_base'}) {\n>>> + $href .= \"/\".esc_url($params{'hash_base'});\n>>> + if (defined $params{'file_name'}) {\n>>> + $href .= \":\".esc_url($params{'file_name'});\n>>> + delete $params{'hash'} if $params{'hash'} eq git_get_hash_by_path($params{'hash_base'},$params{'file_name'});\n>>\n>> First, this page has around 140 characters. That is too long, much too long.\n>> Please try to wrap code around 80-characters.\n>>\n>> Second, following Petr 'Pasky' Baudis suggestion of reducing complexity\n>> and shortening gitweb URLs, we could unconditionally remove redundant\n>> 'hash' parameter if we have both 'hash_base' and 'file_name'\n>> parameters. This would also simplify and speed up (lack of extra fork)\n>> gitweb code.\n> \n> If it's indeed guaranteed that hash is not needed in these cases, it's\n> surely the best course of action. I changed the code to that effect.\n\nHash is not needed for hash_base:file_name, unless you messed up\nsomething (by hand-crafting URL). If they do not much, you have more\nproblems than that...\n \n>> Hmmm... now I am not so sure if it wouldn't be better to split this\n>> patch in pathinfo parsing and pathinfo generation. The first part\n>> would be obvious, the second part would be smaller and easier to review.\n> \n> Ok, I'll split parsing from generation. Since it's what I did for\n> subsequent extensions (such as the parent..current thing) it also fits\n> nicely with the patchflow.\n\nThanks.\n-- \nJakub Narebski\nPoland\n"},{"id":"91901","messageId":"cb7bb73a0809300048j7da35623m44ec9bfe7780fedb@mail.gmail.com","threadId":"15618","inReplyTo":"200809300120.02492.jnareb@gmail.com","subject":"Re: [PATCH 2/6] gitweb: use_pathinfo filenames start with /","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-09-30T07:48:22Z","receivedAt":"2008-09-30T07:48:22Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Tue, Sep 30, 2008 at 1:20 AM, Jakub Narebski wrote:\n>\n> Hn. Now I am not sure if it should be squashed, or should be separate.\n\nYeah. The fact that it's *specifically* to allow web docs to be used\nin raw view makes it count towarda a feature in itself, even if the\npatch by itself is trivial ...\n\nSo. Squashed or separate? :)\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"91906","messageId":"cb7bb73a0809300105s24706d79hb40e147739ec6f05@mail.gmail.com","threadId":"15618","inReplyTo":"200809300221.25094.jnareb@gmail.com","subject":"Re: [PATCH 1/6] gitweb: action in path with use_pathinfo","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-09-30T08:05:59Z","receivedAt":"2008-09-30T08:05:59Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Tue, Sep 30, 2008 at 2:21 AM, Jakub Narebski wrote:\n> Perhaps not in so large and detailed form... I guess explanation of\n> using ':/' as separator should be put there as well, if you plan to\n> squash those patches.\n\nI will add the explanation for the :/ separator in the patch for URL\ngeneration (forgot it in v3)\n\n>>>> @@ -624,8 +636,13 @@ sub href (%) {\n>>>> if ($params{-replay}) {\n>>>> while (my ($name, $symbol) = each %mapping) {\n>>>> if (!exists $params{$name}) {\n>>>> - # to allow for multivalued params we use arrayref form\n>>>> - $params{$name} = [ $cgi->param($symbol) ];\n>>>> + if ($cgi->param($symbol)) {\n>>>> + # to allow for multivalued params we use arrayref form\n>>>> + $params{$name} = [ $cgi->param($symbol) ];\n>>>> + } else {\n>>>> + no strict 'refs';\n>>>> + $params{$name} = $$name if $$name;\n>>>> + }\n>>>> }\n>>>> }\n>>>> }\n>>>\n>>> What this change is about? And why this change is _here_, in this\n>>> commit? It is I think unrelated, and wrong change.\n>>\n>> This is about being able to recycle CGI parameters that came through\n>> as part of path_info instead of the CGI parameter list. It might not\n>> be the best way to recover it, though. I *did* have a few thoughts\n>> about an alternative way that consisted of build a parameter list\n>> merging CGI and path-info parameter, but since this approach seemed to\n>> work, I went with it.\n>\n> Fact, I have totally forgot about this.\n\nMe too actually (the first version of this patch is almost a year old,\nand since I forgot to write down why I did a few things ... taught me\nthat I should start putting more comments, though 8-D)\n\n>>> href(..., -replay=>1) is all about reusing current URL, perhaps with\n>>> a few parameters changed, like for example pagination links differ only\n>>> in page number param change. For example if we had only hb= and f=\n>>> parameters, -replay=>1 links should use only those, and not add h=\n>>> parameter because somewhere we felt that we need $hash to be calculated.\n>>\n>> Assume for example that you are to an url such as\n>>\n>> http://git.oblomov.eu/git/tree/refs/remotes/origin/master:gitweb\n>>\n>> Without this patch, the 'history' link on the second header line links\n>> to ARRAY(0xblah)ARRAY(0xblah). With this patch, it shows the proper\n>> link. So either replay is being abused somewhere in the link\n>> generation code, or this CGI+path_info parameter retrieval is\n>> necessary, one way or the other.\n>\n> Ah. Now I understand.\n>\n> When creating code for href(..., -replay=>1), which by the way I thought\n> would be more useful than actually is, I have forgot that parameters to\n> gitweb could be passed in other way that through CGI parameters\n> (CGI query)[1].\n>\n> Using\n>\n> $params{$name} = [ $cgi->param($symbol) ];\n>\n> is a cute hack, but it doesn't work for arguments passed via path_info\n> (was: project, hash_base and file_name; while now it is project, action,\n> hash_base (in full) and file_name).\n\nExactly.\n\n> The solution I thought about and abandoned in favor of this cute hack\n> was to have additional hash (in addition to %mapping), which would map\n> action names to references to variables holding the value for parameter.\n>\n> This has the same problem as your proposed solution of putting some\n> parameters which didn't come from URL but were filled from other info.\n> $hash parameter is most likely to be culprit here.\n>\n> On the other hand it is more generic and doesn't rely on knowledge that\n> there is no multi-valued parameter which can be expressed in path_info\n> (currently only 'opt' parameter can be multi-valued, and requires\n> arrayref, but also 'opt' parameter is and cannot be put in path_info).\n>\n> I am talking there about the following solution:\n>\n> my %action_vars = (\n> project => \\$project,\n> action => \\$action,\n> # ...\n> extra_options => \\@extra_options,\n> );\n> # ...\n> while (my ($name, $symbol) = each %mapping) {\n> if (!exists $params{$name}) {\n> $params{$name} = ${$action_vars{$name}};\n> }\n> }\n>\n>\n> This avoids cure hack of (from your code)\n>\n> } else {\n> no strict 'refs';\n> $params{$name} = $$name if $$name;\n> }\n>\n> I think that gitweb should use single source, not CGI query parameters\n> or variable saving [sanitized] value.\n\nThe alternative I've been thinking about would be to have an\n%input_parameters hash that holds all input parameters regardless of\nhash; thus CGI query parameters and data extracted from PATH_INFO,\npresently, but also command line options in the future, or whatever\nelse.\n\nThis is somewhat different from your %action_vars alternative, in the\nsense that it isolates _input_ data, whereas if I understand correctly\nthe approach you suggest would isolate _output_ data (in the sense of\ndata to be used during link creation and whatnot).\n\nPresently, the gitweb code defines some $variables from the input\nparameters, and then overwrites them for output. Keeping the input\nstuff clearly separate from the output stuff would mean that any\nroutine can retrieve the input data regardless of the subsequent\nmangling and without any need to make ad-hoc backups or other tricks.\n\nSo my proposal is that I implement this %input_params stuff as the\nfirst patch for the pathinfo series, and use %input_params all around\nwhere cgi parameters are used currently (of course, %input_params is\ninitialized with the CGI parameters at first). The next patch would be\nthe extraction of parameters from PATH_INFO. And thirdly the PATH_INFO\nURL generation (with or without the /-before-filename thing, at your\npreference)\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"91911","messageId":"200809301048.40046.jnareb@gmail.com","threadId":"15618","inReplyTo":"cb7bb73a0809300105s24706d79hb40e147739ec6f05@mail.gmail.com","subject":"Re: [PATCH 1/6] gitweb: action in path with use_pathinfo","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-09-30T08:48:38Z","receivedAt":"2008-09-30T08:48:38Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Tue, 30 Sep 2008, Giuseppe Bilotta wrote:\n> On Tue, Sep 30, 2008 at 2:21 AM, Jakub Narebski wrote:\n\n[...]\n>> Ah. Now I understand.\n>>\n>> When creating code for href(..., -replay=>1), which by the way I thought\n>> would be more useful than actually is, I have forgot that parameters to\n>> gitweb could be passed in other way that through CGI parameters\n>> (CGI query)[1].\n>>\n>> Using\n>>\n>> $params{$name} = [ $cgi->param($symbol) ];\n>>\n>> is a cute hack, but it doesn't work for arguments passed via path_info\n>> (was: project, hash_base and file_name; while now it is project, action,\n>> hash_base (in full) and file_name).\n[...]\n\n>> The solution I thought about and abandoned in favor of this cute hack\n>> was to have additional hash (in addition to %mapping), which would map\n>> action names to references to variables holding the value for parameter.\n[...]\n\n>> I am talking there about the following solution:\n>>\n>> my %action_vars = (\n>> project => \\$project,\n>> action => \\$action,\n>> # ...\n>> extra_options => \\@extra_options,\n>> );\n>> # ...\n>> while (my ($name, $symbol) = each %mapping) {\n>> if (!exists $params{$name}) {\n>> $params{$name} = ${$action_vars{$name}};\n>> }\n>> }\n>>\n>>\n>> This avoids cure hack of (from your code)\n>>\n>> } else {\n>> no strict 'refs';\n>> $params{$name} = $$name if $$name;\n>> }\n>>\n>> I think that gitweb should use single source, not CGI query parameters\n>> or variable saving [sanitized] value.\n> \n> The alternative I've been thinking about would be to have an\n> %input_parameters hash that holds all input parameters regardless of\n> hash; thus CGI query parameters and data extracted from PATH_INFO,\n> presently, but also command line options in the future, or whatever\n> else.\n> \n> This is somewhat different from your %action_vars alternative, in the\n> sense that it isolates _input_ data, whereas if I understand correctly\n> the approach you suggest would isolate _output_ data (in the sense of\n> data to be used during link creation and whatnot).\n> \n> Presently, the gitweb code defines some $variables from the input\n> parameters, and then overwrites them for output. Keeping the input\n> stuff clearly separate from the output stuff would mean that any\n> routine can retrieve the input data regardless of the subsequent\n> mangling and without any need to make ad-hoc backups or other tricks.\n> \n> So my proposal is that I implement this %input_params stuff as the\n> first patch for the pathinfo series, and use %input_params all around\n> where cgi parameters are used currently (of course, %input_params is\n> initialized with the CGI parameters at first). The next patch would be\n> the extraction of parameters from PATH_INFO. And thirdly the PATH_INFO\n> URL generation (with or without the /-before-filename thing, at your\n> preference)\n\nI presume that you would want to replace for example $hash_base\neverywhere by %input_params{'hash_base'}?\n\n\nI can think of yet another solution, namely to abstract getting\nparameters from CGI query string, from path_info, and possibly in the\nfuture also from command line options, and use this mechanism in\nthe getting parameters and validation part.\n\nThe %params hash would be filled from CGI parameters by using simply\n\"%params = $cgi->Vars;\", then added to in evaluate_path_info instead\nof directly modifying global parameters variables. The input validation\nand dispatch part would be modified to use %params (taking care of\nmultivalued parameters as described in CGI(3pm)), like below:\n\n our $action = $params{'a'} || $params{'action'};\n if (defined $action) {\n \tif ($action =~ m/[^0-9a-zA-Z\\.\\-_]/) {\n \t\tdie_error(400, \"Invalid action parameter\");\n \t}\n }\n\nThat is just for consideration: each approach has its advantages and\ndisadvantages. Your proposal, as I understand it, is similar to the\nway described in \"Storing options in a hash\" subsection of \nGetopt::Long(3pm) manpage.\n\n\nOr we could just scrap and revert adding href(..., -replay=>1).\nThere is much trouble with getting it right and performing well,\nand it is less useful than I thought (at least now).\n\n-- \nJakub Narebski\nPoland\n"},{"id":"91920","messageId":"cb7bb73a0809300340t79a497fey4ededd960223fcdd@mail.gmail.com","threadId":"15618","inReplyTo":"200809301048.40046.jnareb@gmail.com","subject":"Re: [PATCH 1/6] gitweb: action in path with use_pathinfo","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-09-30T10:40:36Z","receivedAt":"2008-09-30T10:40:36Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Tue, Sep 30, 2008 at 10:48 AM, Jakub Narebski wrote:\n> On Tue, 30 Sep 2008, Giuseppe Bilotta wrote:\n>> On Tue, Sep 30, 2008 at 2:21 AM, Jakub Narebski wrote:\n\n>>> I think that gitweb should use single source, not CGI query parameters\n>>> or variable saving [sanitized] value.\n>>\n>> The alternative I've been thinking about would be to have an\n>> %input_parameters hash that holds all input parameters regardless of\n>> hash; thus CGI query parameters and data extracted from PATH_INFO,\n>> presently, but also command line options in the future, or whatever\n>> else.\n>>\n>> This is somewhat different from your %action_vars alternative, in the\n>> sense that it isolates _input_ data, whereas if I understand correctly\n>> the approach you suggest would isolate _output_ data (in the sense of\n>> data to be used during link creation and whatnot).\n>>\n>> Presently, the gitweb code defines some $variables from the input\n>> parameters, and then overwrites them for output. Keeping the input\n>> stuff clearly separate from the output stuff would mean that any\n>> routine can retrieve the input data regardless of the subsequent\n>> mangling and without any need to make ad-hoc backups or other tricks.\n>>\n>> So my proposal is that I implement this %input_params stuff as the\n>> first patch for the pathinfo series, and use %input_params all around\n>> where cgi parameters are used currently (of course, %input_params is\n>> initialized with the CGI parameters at first). The next patch would be\n>> the extraction of parameters from PATH_INFO. And thirdly the PATH_INFO\n>> URL generation (with or without the /-before-filename thing, at your\n>> preference)\n>\n> I presume that you would want to replace for example $hash_base\n> everywhere by %input_params{'hash_base'}?\n\nNo. %input_params{'hash_base'} would only be the _input_ hash base.\n$hash_base would be kept if it's supposed to indicate the value of\nhash base that is being manipulated.\n\n> I can think of yet another solution, namely to abstract getting\n> parameters from CGI query string, from path_info, and possibly in the\n> future also from command line options, and use this mechanism in\n> the getting parameters and validation part.\n>\n> The %params hash would be filled from CGI parameters by using simply\n> \"%params = $cgi->Vars;\", then added to in evaluate_path_info instead\n> of directly modifying global parameters variables.\n\nSo far I agree.\n\n> The input validation\n> and dispatch part would be modified to use %params (taking care of\n> multivalued parameters as described in CGI(3pm)), like below:\n>\n> our $action = $params{'a'} || $params{'action'};\n\nNot too sure about that. The path_info (or whatever)-derived params\nshould be converted to use the same name as the CGI params. Or\nconversely, CGI params should be mapped to the corresponding\nfull-form.\n\n> That is just for consideration: each approach has its advantages and\n> disadvantages. Your proposal, as I understand it, is similar to the\n> way described in \"Storing options in a hash\" subsection of\n> Getopt::Long(3pm) manpage.\n\nI'll read that, although it probably is.\n\n> Or we could just scrap and revert adding href(..., -replay=>1).\n> There is much trouble with getting it right and performing well,\n> and it is less useful than I thought (at least now).\n\nDunno, the idea in itself is not bad. We just have to get it right ;)\n\nIn a way, I actually think that -replay=>1 should be the default, I\nsuspect it makes sense in most cases.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"91930","messageId":"200809301322.58280.jnareb@gmail.com","threadId":"15618","inReplyTo":"cb7bb73a0809300340t79a497fey4ededd960223fcdd@mail.gmail.com","subject":"Re: [PATCH 1/6] gitweb: action in path with use_pathinfo","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-09-30T11:22:56Z","receivedAt":"2008-09-30T11:22:56Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Tue, 30 Sep 2008, Giuseppe \"Oblomov\" Bilotta wrote:\n> On Tue, Sep 30, 2008 at 10:48 AM, Jakub Narebski wrote:\n\n> > Or we could just scrap and revert adding href(..., -replay=>1).\n> > There is much trouble with getting it right and performing well,\n> > and it is less useful than I thought (at least now).\n> \n> Dunno, the idea in itself is not bad. We just have to get it right ;)\n\nIt is not easy to get it right, especially that there are multivalued\nparameters like @extra_options, see e.g. commit 7863c612\n \n> In a way, I actually think that -replay=>1 should be the default, I\n> suspect it makes sense in most cases.\n\nWell, -replay=>1 was meant to be used for \"alternate view\" links, like\nfor example 'next page' link, or 'raw view' link, or 'sorted by' link;\nit would be stretch and feature abuse to use it for \"item\" links, like\nentries in 'tree' view, or commits in log-like views, or changed files\nlinks in 'commitdiff' and 'blobdiff' views.\n\nI guess it would be half the cases, not most cases.\n-- \nJakub Narebski\nPoland\n"},{"id":"91936","messageId":"cb7bb73a0809300553o7496f4c1me14ddf55b31fe4a6@mail.gmail.com","threadId":"15618","inReplyTo":"200809301322.58280.jnareb@gmail.com","subject":"Re: [PATCH 1/6] gitweb: action in path with use_pathinfo","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-09-30T12:53:08Z","receivedAt":"2008-09-30T12:53:08Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Tue, Sep 30, 2008 at 1:22 PM, Jakub Narebski wrote:\n> On Tue, 30 Sep 2008, Giuseppe \"Oblomov\" Bilotta wrote:\n>> On Tue, Sep 30, 2008 at 10:48 AM, Jakub Narebski wrote:\n>\n>> > Or we could just scrap and revert adding href(..., -replay=>1).\n>> > There is much trouble with getting it right and performing well,\n>> > and it is less useful than I thought (at least now).\n>>\n>> Dunno, the idea in itself is not bad. We just have to get it right ;)\n>\n> It is not easy to get it right, especially that there are multivalued\n> parameters like @extra_options, see e.g. commit 7863c612\n\nSo we just let values be arrays. Or isn't this enough?\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"91989","messageId":"200809302300.46786.jnareb@gmail.com","threadId":"15618","inReplyTo":"cb7bb73a0809300553o7496f4c1me14ddf55b31fe4a6@mail.gmail.com","subject":"Re: [PATCH 1/6] gitweb: action in path with use_pathinfo","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-09-30T21:00:45Z","receivedAt":"2008-09-30T21:00:45Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Tue, 30 September 2008, Giuseppe Bilotta wrote:\n> On Tue, Sep 30, 2008 at 1:22 PM, Jakub Narebski wrote:\n>> On Tue, 30 Sep 2008, Giuseppe \"Oblomov\" Bilotta wrote:\n>>> On Tue, Sep 30, 2008 at 10:48 AM, Jakub Narebski wrote:\n\n>>>> Or we could just scrap and revert adding href(..., -replay=>1).\n>>>> There is much trouble with getting it right and performing well,\n>>>> and it is less useful than I thought (at least now).\n>>>\n>>> Dunno, the idea in itself is not bad. We just have to get it right ;)\n>>\n>> It is not easy to get it right, especially that there are multivalued\n>> parameters like @extra_options, see e.g. commit 7863c612\n> \n> So we just let values be arrays. Or isn't this enough?\n\nI don't think it would be that simple.\n\n\nLet me elaborate a bit about complications and difficulties\nwithin href() code.\n\nFirst there are two names of parameters: the short name like 'a',\nor 'h', or 'f' which is used in links to make them short (and\nwhich is a bit historical legacy), and the long names like 'action',\n'hash' or 'file_name' which are used for readability; then there are\nalso variables which hold values of parameters, like $action,\n$hash and $file_name (which were source of long names for parameters).\nhref() has to provide translation between those two (well, three)\nnames; long names are used as names of \"named parameters\" to href(),\nwhile short names are used when generating URL; %mapping provides\nmapping between those two.\n\nSecond, href() must distinguish between gitweb options/parameters\nlike 'file_name'=>$file_name and extra options like '-full'=>1\nor '-replay'=>1; additionally we want to have options output in\nsome definite order, with more important options first. This\nis provided by %mapping (to filter out) and @mapping (to sort).\n\nThird, there are various sources of values of parameters, and\nparameters used. There are parameters specified explicitly in\nhref() call, and there are also implicit parameters: 'project'\nparameter is implicitly added as it is needed in almost all\ngitweb links (you can override it and generate projectless link by\nusing 'project'=>undef), and for '-replay'=>1 all parameters from\ncurrent URL are added. Parameters specified explicitly have preference\nover implicit or '-replay' ones. \n\nNow for '-replay' parameters might come from CGI query string, from\npath_info part of URL, and perhaps in the future also from command\nline. To avoid duplicated code and other problems we should either\nget parameters from variables (like in your code, but which doesn't\ncover @extra_options well, but it could; or using \"long name\" to\nvariable ref mapping), or have some place where we save parameters\nas we extract it from CGI query string, from path_info part of URL,\nand in the future probably also from command line options/parameters.\n\nFourth, there is a little matter of _some_ parameters be multivalued;\ncurrently it is only @extra_options / 'opt', but this may change in\nthe future, while _most_ are ordinary scalar values. Printing arrayref\nisn't something we want to do...\n\n\nThat is third and fourth which caused problems in the past with\nhref(..., -replay=>1)...\n\n-- \nJakub Narebski\nPoland\n"},{"id":"91996","messageId":"200810010124.41455.jnareb@gmail.com","threadId":"15618","inReplyTo":"cb7bb73a0809300340t79a497fey4ededd960223fcdd@mail.gmail.com","subject":"Re: [PATCH 1/6] gitweb: action in path with use_pathinfo","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-09-30T23:24:40Z","receivedAt":"2008-09-30T23:24:40Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Tue, 30 Sep 2008, Giuseppe Bilotta wrote:\n> On Tue, Sep 30, 2008 at 10:48 AM, Jakub Narebski wrote:\n\n>> I presume that you would want to replace for example $hash_base\n>> everywhere by %input_params{'hash_base'}?\n> \n> No. %input_params{'hash_base'} would only be the _input_ hash base.\n> $hash_base would be kept if it's supposed to indicate the value of\n> hash base that is being manipulated.\n\nAh, sorry, I misunderstood. Then your idea is the same as one of mine\n(except perhaps some details).\n \n>> I can think of yet another solution, namely to abstract getting\n>> parameters from CGI query string, from path_info, and possibly in the\n>> future also from command line options, and use this mechanism in\n>> the getting parameters and validation part.\n>>\n>> The %params hash would be filled from CGI parameters by using simply\n>> \"%params = $cgi->Vars;\", then added to in evaluate_path_info instead\n>> of directly modifying global parameters variables.\n> \n> So far I agree.\n\nUsing \"%input_params = %cgi->Vars;\" has consequence of using short\nparameter names for keys (and also a bit strange syntax for multivalue\noptions, see CGI(3pm)).\n\n>> The input validation\n>> and dispatch part would be modified to use %params (taking care of\n>> multivalued parameters as described in CGI(3pm)), like below:\n\nThis has the additional advantage of doing gitweb parameter validation\n_once_, and not like it is now done for example first in the \"input\nvalidation and dispatch\" section, and then in evaluate_path_info()\nsubroutine.\n\nOn the other hand $project is checked _already_ in evaluate_path_info(),\nbecause it has to to find where project name ends, so this part would\nget duplicated, unless something smart is done.\n\n>>\n>> our $action = $params{'a'} || $params{'action'};\n>\n> Not too sure about that. The path_info (or whatever)-derived params\n> should be converted to use the same name as the CGI params. Or\n> conversely, CGI params should be mapped to the corresponding\n> full-form.\n\nAfter thinking about it a little, I agree with above paragraph.\n\n>> That is just for consideration: each approach has its advantages and\n>> disadvantages. Your proposal, as I understand it, is similar to the\n>> way described in \"Storing options in a hash\" subsection of\n>> Getopt::Long(3pm) manpage.\n> \n> I'll read that, although it probably is.\n\nPerhaps gitweb should have implement something like GetOptions?\n\n>> Or we could just scrap and revert adding href(..., -replay=>1).\n>> There is much trouble with getting it right and performing well,\n>> and it is less useful than I thought (at least now).\n> \n> Dunno, the idea in itself is not bad. We just have to get it right ;)\n> \n> In a way, I actually think that -replay=>1 should be the default, I\n> suspect it makes sense in most cases.\n\nPITA but useful. Hmmm....\n\n-- \nJakub Narebski\nPoland\n"},{"id":"91999","messageId":"200810010149.48988.jnareb@gmail.com","threadId":"15618","inReplyTo":"cb7bb73a0809300048j7da35623m44ec9bfe7780fedb@mail.gmail.com","subject":"Re: [PATCH 2/6] gitweb: use_pathinfo filenames start with /","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-09-30T23:49:48Z","receivedAt":"2008-09-30T23:49:48Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Tue, 30 Sep 2008, Giuseppe Bilotta wrote:\n> On Tue, Sep 30, 2008 at 1:20 AM, Jakub Narebski wrote:\n> >\n> > Hn. Now I am not sure if it should be squashed, or should be separate.\n> \n> Yeah. The fact that it's *specifically* to allow web docs to be used\n> in raw view makes it count towarda a feature in itself, even if the\n> patch by itself is trivial ...\n> \n> So. Squashed or separate? :)\n\nI'm slighty for separate, if only to avoid overly long commit message.\n: is understandable and expected because it is\nwhat one uses for git-show; making :/ the default\nbecause of relative links in 'blob_plain' view of HTML file requires\nsome further explanation.\n\nBut is is IMVHO finally your call here.\n-- \nJakub Narebski\nPoland\n"}]}