{"thread":{"id":"15930","subject":"[PATCHv6 0/5] gitweb: PATH_INFO enhancement","startedAt":"2008-10-16T20:27:06Z","lastAt":"2008-11-02T08:50:22Z","messageCount":26,"participants":["Giuseppe Bilotta","Jakub Narebski","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"93230","messageId":"1224188831-17767-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"15930","inReplyTo":null,"subject":"[PATCHv6 0/5] gitweb: PATH_INFO enhancement","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-10-16T20:27:06Z","receivedAt":"2008-10-16T20:27:06Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Sixth attempt for my gitweb PATH_INFO patchset, whose purpose is to\nreduce the use of CGI parameters by embedding as many parameters as\npossible in the URL path itself, provided the pathinfo feature is\nenabled.\n\nThe new typical gitweb URL is therefore in the form\n\n$project/$action/$parent:$file..$hash:$file\n\n(with useless parts stripped). Backwards compatibility for old-style\n$project/$hash[:$file] URLs is kept, as long as $hash is not a refname\nwhose name happens to match a git action.\n\nThe main implementation is provided by paired patches (#1#2, #4#5)\nthat implement parsing and generation of the new style URLs.\n\nPatch #3 is a minor improvement to the URL syntax that allows web\nsites to be properly browsable in raw mode.\n\nThe patchset depends on my previous input parameter handling patch\ncurrently waiting in 'next'. This resend fixes a couple of problems\nI found in v5.\n\nGiuseppe Bilotta (5):\n gitweb: parse project/action/hash_base:filename PATH_INFO\n gitweb: generate project/action/hash URLs\n gitweb: use_pathinfo filenames start with /\n gitweb: parse parent..current syntax from PATH_INFO\n gitweb: generate parent..current URLs\n\n gitweb/gitweb.perl | 124 +++++++++++++++++++++++++++++++++++++++++++++++----\n 1 files changed, 114 insertions(+), 10 deletions(-)\n"},{"id":"93231","messageId":"1224188831-17767-2-git-send-email-giuseppe.bilotta@gmail.com","threadId":"15930","inReplyTo":"1224188831-17767-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv6 1/5] gitweb: parse project/action/hash_base:filename PATH_INFO","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-10-16T20:27:07Z","receivedAt":"2008-10-16T20:27:07Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"This patch enables gitweb to parse URLs with more information embedded\nin PATH_INFO, reducing the need for CGI parameters. The typical gitweb\npath is now $project/$action/$hash_base:$file_name or\n$project/$action/$hash\n\nThis is mostly backwards compatible with the old-style gitweb paths,\n$project/$branch[:$filename], except when it was used to access a branch\nwhose name matches a gitweb action.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 37 +++++++++++++++++++++++++++++++------\n 1 files changed, 31 insertions(+), 6 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex c5254af..6d0dc26 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -534,23 +534,48 @@ sub evaluate_path_info {\n \treturn if $input_params{'action'};\n \t$path_info =~ s,^\\Q$project\\E/*,,;\n \n+\t# next, check if we have an action\n+\tmy $action = $path_info;\n+\t$action =~ s,/.*$,,;\n+\tif (exists $actions{$action}) {\n+\t\t$path_info =~ s,^$action/*,,;\n+\t\t$input_params{'action'} = $action;\n+\t}\n+\n+\t# list of actions that want hash_base instead of hash\n+\tmy @wants_base = (\n+\t\t'tree',\n+\t\t'history',\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+\t\t# we got \"branch:filename\" or \"branch:dir/\"\n \t\t# we could use git_get_type(branch:pathname), but it needs $git_dir\n \t\t$pathname =~ s,^/+,,;\n \t\tif (!$pathname || substr($pathname, -1) eq \"/\") {\n-\t\t\t$input_params{'action'} = \"tree\";\n+\t\t\t$input_params{'action'} ||= \"tree\";\n \t\t\t$pathname =~ s,/$,,;\n \t\t} else {\n-\t\t\t$input_params{'action'} = \"blob_plain\";\n+\t\t\t$input_params{'action'} ||= \"blob_plain\";\n \t\t}\n \t\t$input_params{'hash_base'} ||= $refname;\n \t\t$input_params{'file_name'} ||= $pathname;\n \t} elsif (defined $refname) {\n-\t\t# we got \"project.git/branch\"\n-\t\t$input_params{'action'} = \"shortlog\";\n-\t\t$input_params{'hash'} ||= $refname;\n+\t\t# we got \"branch\". in this case we have to choose if we have to\n+\t\t# set hash or hash_base.\n+\t\t#\n+\t\t# Most of the actions without a pathname only want hash to be\n+\t\t# set, except for the ones specified in @wants_base that want\n+\t\t# hash_base instead. It should also be noted that hand-crafted\n+\t\t# links having 'history' as an action and no pathname or hash\n+\t\t# set will fail, but that happens regardless of PATH_INFO.\n+\t\t$input_params{'action'} ||= \"shortlog\";\n+\t\tif (grep {$input_params{'action'} eq $_} @wants_base) {\n+\t\t\t$input_params{'hash_base'} ||= $refname;\n+\t\t} else {\n+\t\t\t$input_params{'hash'} ||= $refname;\n+\t\t}\n \t}\n }\n evaluate_path_info();\n-- \n1.5.6.5\n"},{"id":"93235","messageId":"1224188831-17767-3-git-send-email-giuseppe.bilotta@gmail.com","threadId":"15930","inReplyTo":"1224188831-17767-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv6 2/5] gitweb: generate project/action/hash URLs","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-10-16T20:27:08Z","receivedAt":"2008-10-16T20:27:08Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"When generating path info URLs, reduce the number of CGI parameters by\nembedding action and hash_parent:filename or hash in the path.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 33 ++++++++++++++++++++++++++++++---\n 1 files changed, 30 insertions(+), 3 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 6d0dc26..5337d40 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -724,14 +724,41 @@ sub href (%) {\n \n \tmy ($use_pathinfo) = gitweb_check_feature('pathinfo');\n \tif ($use_pathinfo) {\n-\t\t# use PATH_INFO for project name\n+\t\t# try to put as many parameters as possible in PATH_INFO:\n+\t\t# - project name\n+\t\t# - action\n+\t\t# - hash or hash_base:filename\n+\n+\t\t# When the script is the root DirectoryIndex for the domain,\n+\t\t# $href here would be something like http://gitweb.example.com/\n+\t\t# Thus, we strip any trailing / from $href, to spare us double\n+\t\t# slashes in the final URL\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\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 is\n+\t\t# added to 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# Finally, 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{'file_name'};\n+\t\t\t}\n+\t\t\tdelete $params{'hash'};\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":"93233","messageId":"1224188831-17767-4-git-send-email-giuseppe.bilotta@gmail.com","threadId":"15930","inReplyTo":"1224188831-17767-3-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv6 3/5] gitweb: use_pathinfo filenames start with /","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-10-16T20:27:09Z","receivedAt":"2008-10-16T20:27:09Z","isPatch":false,"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 in raw\n('blob_plain') mode.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 5337d40..49730f3 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -727,7 +727,7 @@ sub href (%) {\n \t\t# try to put as many parameters as possible in PATH_INFO:\n \t\t# - project name\n \t\t# - action\n-\t\t# - hash or hash_base:filename\n+\t\t# - hash or hash_base:/filename\n \n \t\t# When the script is the root DirectoryIndex for the domain,\n \t\t# $href here would be something like http://gitweb.example.com/\n@@ -746,11 +746,11 @@ sub href (%) {\n \t\t\tdelete $params{'action'};\n \t\t}\n \n-\t\t# Finally, we put either hash_base:file_name or hash\n+\t\t# Finally, 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\t$href .= \":/\".esc_url($params{'file_name'});\n \t\t\t\tdelete $params{'file_name'};\n \t\t\t}\n \t\t\tdelete $params{'hash'};\n-- \n1.5.6.5\n"},{"id":"93232","messageId":"1224188831-17767-5-git-send-email-giuseppe.bilotta@gmail.com","threadId":"15930","inReplyTo":"1224188831-17767-4-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv6 4/5] gitweb: parse parent..current syntax from PATH_INFO","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-10-16T20:27:10Z","receivedAt":"2008-10-16T20:27:10Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"This patch 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\nAll '*diff' actions and in general actions that use $hash_parent[_base]\nand $file_parent can now get all of their parameters from PATH_INFO\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 36 ++++++++++++++++++++++++++++++++++--\n 1 files changed, 34 insertions(+), 2 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 49730f3..1a7b0b9 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -548,7 +548,12 @@ sub evaluate_path_info {\n \t\t'history',\n \t);\n \n-\tmy ($refname, $pathname) = split(/:/, $path_info, 2);\n+\t# horrible regexp to catch\n+\t# [$hash_parent_base[:$file_parent]..]$hash_parent[:$file_name]\n+\tmy ($parentrefname, $parentpathname, $refname, $pathname) =\n+\t\t($path_info =~ /^(?:(.+?)(?::(.+))?\\.\\.)?(.+?)(?::(.+))?$/);\n+\n+\t# first, analyze the 'current' part\n \tif (defined $pathname) {\n \t\t# we got \"branch:filename\" or \"branch:dir/\"\n \t\t# we could use git_get_type(branch:pathname), but it needs $git_dir\n@@ -557,7 +562,13 @@ sub evaluate_path_info {\n \t\t\t$input_params{'action'} ||= \"tree\";\n \t\t\t$pathname =~ s,/$,,;\n \t\t} else {\n-\t\t\t$input_params{'action'} ||= \"blob_plain\";\n+\t\t\t# the default action depends on whether we had parent info\n+\t\t\t# or not\n+\t\t\tif ($parentrefname) {\n+\t\t\t\t$input_params{'action'} ||= \"blobdiff_plain\";\n+\t\t\t} else {\n+\t\t\t\t$input_params{'action'} ||= \"blob_plain\";\n+\t\t\t}\n \t\t}\n \t\t$input_params{'hash_base'} ||= $refname;\n \t\t$input_params{'file_name'} ||= $pathname;\n@@ -577,6 +588,27 @@ sub evaluate_path_info {\n \t\t\t$input_params{'hash'} ||= $refname;\n \t\t}\n \t}\n+\n+\t# next, handle the 'parent' part, if present\n+\tif (defined $parentrefname) {\n+\t\t# a missing pathspec defaults to the 'current' filename, allowing e.g.\n+\t\t# someproject/blobdiff/oldrev..newrev:/filename\n+\t\tif ($parentpathname) {\n+\t\t\t$parentpathname =~ s,^/+,,;\n+\t\t\t$parentpathname =~ s,/$,,;\n+\t\t\t$input_params{'file_parent'} ||= $parentpathname;\n+\t\t} else {\n+\t\t\t$input_params{'file_parent'} ||= $input_params{'file_name'};\n+\t\t}\n+\t\t# we assume that hash_parent_base is wanted if a path was specified,\n+\t\t# or if the action wants hash_base instead of hash\n+\t\tif (defined $input_params{'file_parent'} ||\n+\t\t\tgrep {$input_params{'action'} eq $_} @wants_base) {\n+\t\t\t$input_params{'hash_parent_base'} ||= $parentrefname;\n+\t\t} else {\n+\t\t\t$input_params{'hash_parent'} ||= $parentrefname;\n+\t\t}\n+\t}\n }\n evaluate_path_info();\n \n-- \n1.5.6.5\n"},{"id":"93234","messageId":"1224188831-17767-6-git-send-email-giuseppe.bilotta@gmail.com","threadId":"15930","inReplyTo":"1224188831-17767-5-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv6 5/5] gitweb: generate parent..current URLs","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-10-16T20:27:11Z","receivedAt":"2008-10-16T20:27:11Z","isPatch":false,"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.\n\nIf any of the filename contains two consecutive dots, it's kept as a CGI\nparameter since the resulting path would otherwise be ambiguous.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 30 +++++++++++++++++++++++++-----\n 1 files changed, 25 insertions(+), 5 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 1a7b0b9..f4642e7 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -759,7 +759,8 @@ sub href (%) {\n \t\t# try to put as many parameters as possible in PATH_INFO:\n \t\t# - project name\n \t\t# - action\n-\t\t# - hash or hash_base:/filename\n+\t\t# - hash_parent or hash_parent_base:/file_parent\n+\t\t# - hash or hash_base:/file_name\n \n \t\t# When the script is the root DirectoryIndex for the domain,\n \t\t# $href here would be something like http://gitweb.example.com/\n@@ -778,17 +779,36 @@ sub href (%) {\n \t\t\tdelete $params{'action'};\n \t\t}\n \n-\t\t# Finally, we put either hash_base:/file_name or hash\n+\t\t# Next, we put hash_parent_base:/file_parent..hash_base:/file_name,\n+\t\t# stripping nonexistent or useless pieces\n+\t\t$href .= \"/\" if ($params{'hash_base'} || $params{'hash_parent_base'}\n+\t\t\t|| $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{'file_name'}) {\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\t# skip the file_parent if it's the same as the file_name\n+\t\t\t\tdelete $params{'file_parent'} if $params{'file_parent'} eq $params{'file_name'};\n+\t\t\t\tif (defined $params{'file_parent'} && $params{'file_parent'} !~ /\\.\\./) {\n+\t\t\t\t\t$href .= \":/\".esc_url($params{'file_parent'});\n+\t\t\t\t\tdelete $params{'file_parent'};\n+\t\t\t\t}\n+\t\t\t\t$href .= \"..\";\n+\t\t\t\tdelete $params{'hash_parent'};\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'} && $params{'file_name'} !~ /\\.\\./) {\n \t\t\t\t$href .= \":/\".esc_url($params{'file_name'});\n \t\t\t\tdelete $params{'file_name'};\n \t\t\t}\n \t\t\tdelete $params{'hash'};\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":"93398","messageId":"200810190041.44711.jnareb@gmail.com","threadId":"15930","inReplyTo":"1224188831-17767-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv6 1/5] gitweb: parse project/action/hash_base:filename PATH_INFO","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-10-18T22:41:44Z","receivedAt":"2008-10-18T22:41:44Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 16 Oct 2008, Giuseppe Bilotta wrote:\n\n> This patch enables gitweb to parse URLs with more information embedded\n> in PATH_INFO, reducing the need for CGI parameters. The typical gitweb\n> path is now $project/$action/$hash_base:$file_name or\n> $project/$action/$hash\n> \n> This is mostly backwards compatible with the old-style gitweb paths,\n> $project/$branch[:$filename], except when it was used to access a branch\n> whose name matches a gitweb action.\n\n...in which case old code would access branch, and new code would treat\nname as action. This shouldn't matter in practice, unless there are\nbranches named exactly as gitweb actions.\n\n[But I think current commit description is enough. Just in case...]\n\n> \n> Signed-off-by: Giuseppe Bilotta \n\nI like it. For what it is worth:\n\nAcked-by: Jakub Narebski \n\n> ---\n> gitweb/gitweb.perl | 37 +++++++++++++++++++++++++++++++------\n> 1 files changed, 31 insertions(+), 6 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index c5254af..6d0dc26 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -534,23 +534,48 @@ sub evaluate_path_info {\n> \treturn if $input_params{'action'};\n> \t$path_info =~ s,^\\Q$project\\E/*,,;\n> \n> +\t# next, check if we have an action\n> +\tmy $action = $path_info;\n> +\t$action =~ s,/.*$,,;\n> +\tif (exists $actions{$action}) {\n> +\t\t$path_info =~ s,^$action/*,,;\n> +\t\t$input_params{'action'} = $action;\n> +\t}\n> +\n> +\t# list of actions that want hash_base instead of hash\n\n...and can have no file_name ('f') parameter.\n\n> +\tmy @wants_base = (\n> +\t\t'tree',\n> +\t\t'history',\n> +\t);\n\nI like this solution. It makes path_info URL unambiguous: it is \n/log/ (where is usually refname), and\n/tree/ which is just shortcut for /tree/:/\n\nIt means that the @wants_base list contains actions which require / use\n$hash_base and $file_name, but $file_name might be not set, which denotes\ntop directory (root tree).\n\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> +\t\t# we got \"branch:filename\" or \"branch:dir/\"\n> \t\t# we could use git_get_type(branch:pathname), but it needs $git_dir\n\nWell, this comment was there... but actually I think we avoid \ngit_get_type(\"branch:pathname\") because currently it is additional fork.\nAnd with convention that filename part of path_info ends in '/' for\ndirectories (quite sensible I think) it is not needed. Moreso after\nnext patch in series, when we state action explicitly, and the only\ndifference _might_ be for 'history' view (which is for directories\nand for files both).\n\n> \t\t$pathname =~ s,^/+,,;\n> \t\tif (!$pathname || substr($pathname, -1) eq \"/\") {\n> -\t\t\t$input_params{'action'} = \"tree\";\n> +\t\t\t$input_params{'action'} ||= \"tree\";\n> \t\t\t$pathname =~ s,/$,,;\n> \t\t} else {\n> -\t\t\t$input_params{'action'} = \"blob_plain\";\n> +\t\t\t$input_params{'action'} ||= \"blob_plain\";\n> \t\t}\n> \t\t$input_params{'hash_base'} ||= $refname;\n> \t\t$input_params{'file_name'} ||= $pathname;\n> \t} elsif (defined $refname) {\n> -\t\t# we got \"project.git/branch\"\n> -\t\t$input_params{'action'} = \"shortlog\";\n> -\t\t$input_params{'hash'} ||= $refname;\n> +\t\t# we got \"branch\". in this case we have to choose if we have to\n ^ ^\n |-|-- ??? (typo?)\n\n> +\t\t# set hash or hash_base.\n> +\t\t#\n> +\t\t# Most of the actions without a pathname only want hash to be\n> +\t\t# set, except for the ones specified in @wants_base that want\n> +\t\t# hash_base instead. It should also be noted that hand-crafted\n> +\t\t# links having 'history' as an action and no pathname or hash\n> +\t\t# set will fail, but that happens regardless of PATH_INFO.\n\nAh, I see that the comment about what makes action to be put into\n@wants_base is here.\n\n> +\t\t$input_params{'action'} ||= \"shortlog\";\n> +\t\tif (grep {$input_params{'action'} eq $_} @wants_base) {\n\nStyle: I would use here (but perhaps it is just me) for better \nreadibility\n\n+\t\tif (grep { $input_params{'action'} eq $_ } @wants_base) {\n\n> +\t\t\t$input_params{'hash_base'} ||= $refname;\n> +\t\t} else {\n> +\t\t\t$input_params{'hash'} ||= $refname;\n> +\t\t}\n> \t}\n> }\n> evaluate_path_info();\n> -- \n> 1.5.6.5\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"93399","messageId":"200810190114.26280.jnareb@gmail.com","threadId":"15930","inReplyTo":"1224188831-17767-3-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv6 2/5] gitweb: generate project/action/hash URLs","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-10-18T23:14:25Z","receivedAt":"2008-10-18T23:14:25Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 16 Oct 2008, Giuseppe Bilotta wrote:\n\n> When generating path info URLs, reduce the number of CGI parameters by\n> embedding action and hash_parent:filename or hash in the path.\n> \n> Signed-off-by: Giuseppe Bilotta \n\nNice, well commented, and clean.\n\nAcked-by: Jakub Narebski \n\n> ---\n> gitweb/gitweb.perl | 33 ++++++++++++++++++++++++++++++---\n> 1 files changed, 30 insertions(+), 3 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 6d0dc26..5337d40 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -724,14 +724,41 @@ sub href (%) {\n> \n> \tmy ($use_pathinfo) = gitweb_check_feature('pathinfo');\n> \tif ($use_pathinfo) {\n> -\t\t# use PATH_INFO for project name\n> +\t\t# try to put as many parameters as possible in PATH_INFO:\n> +\t\t# - project name\n> +\t\t# - action\n> +\t\t# - hash or hash_base:filename\n> +\n> +\t\t# When the script is the root DirectoryIndex for the domain,\n> +\t\t# $href here would be something like http://gitweb.example.com/\n> +\t\t# Thus, we strip any trailing / from $href, to spare us double\n> +\t\t# slashes in the final URL\n> +\t\t$href =~ s,/$,,;\n\nI like having this comment here.\n\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\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 is\n> +\t\t# added to the URL\n\nWhich meant that we prefer http://base_url/project over http://base_url/project/summary\n\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# Finally, 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{'file_name'};\n> +\t\t\t}\n> +\t\t\tdelete $params{'hash'};\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> -- \n> 1.5.6.5\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"93400","messageId":"200810190126.06697.jnareb@gmail.com","threadId":"15930","inReplyTo":"1224188831-17767-4-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv6 3/5] gitweb: use_pathinfo filenames start with /","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-10-18T23:26:06Z","receivedAt":"2008-10-18T23:26:06Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 16 Oct 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 in raw\n> ('blob_plain') mode.\n\nThis means generating project/action/hash_base:/filename rather than\nearlier project/action/hash_base:filename, isn't it?\n\nFor example for http://repo.or.cz/w/git.git/html:/git.html links leads\nto correct HTML pages, while for http://repo.or.cz/w/git.git/html:git.html\nthey lead to empty gitweb page (no errors, so link checker would be\nfooled).\n\n> \n> Signed-off-by: Giuseppe Bilotta \n\nAcked-by: Jakub Narebski \n\n> ---\n> gitweb/gitweb.perl | 6 +++---\n> 1 files changed, 3 insertions(+), 3 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 5337d40..49730f3 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -727,7 +727,7 @@ sub href (%) {\n> \t\t# try to put as many parameters as possible in PATH_INFO:\n> \t\t# - project name\n> \t\t# - action\n> -\t\t# - hash or hash_base:filename\n> +\t\t# - hash or hash_base:/filename\n> \n> \t\t# When the script is the root DirectoryIndex for the domain,\n> \t\t# $href here would be something like http://gitweb.example.com/\n> @@ -746,11 +746,11 @@ sub href (%) {\n> \t\t\tdelete $params{'action'};\n> \t\t}\n> \n> -\t\t# Finally, we put either hash_base:file_name or hash\n> +\t\t# Finally, 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\t$href .= \":/\".esc_url($params{'file_name'});\n> \t\t\t\tdelete $params{'file_name'};\n> \t\t\t}\n> \t\t\tdelete $params{'hash'};\n> -- \n> 1.5.6.5\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"93401","messageId":"cb7bb73a0810181657i67d6b9f8o66db9f57bcf01dd3@mail.gmail.com","threadId":"15930","inReplyTo":"200810190126.06697.jnareb@gmail.com","subject":"Re: [PATCHv6 3/5] gitweb: use_pathinfo filenames start with /","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-10-18T23:57:08Z","receivedAt":"2008-10-18T23:57:08Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Sun, Oct 19, 2008 at 1:26 AM, Jakub Narebski wrote:\n> On Thu, 16 Oct 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 in raw\n>> ('blob_plain') mode.\n>\n> This means generating project/action/hash_base:/filename rather than\n> earlier project/action/hash_base:filename, isn't it?\n\nExactly. We _accept_ paths with or without the /, but we always\ngenerate paths with it.\n\n> For example for http://repo.or.cz/w/git.git/html:/git.html links leads\n> to correct HTML pages, while for http://repo.or.cz/w/git.git/html:git.html\n> they lead to empty gitweb page (no errors, so link checker would be\n> fooled).\n\nAn idea that could be taken into consideration, if the ability to\nnavigate web documents is deemed of primary importance, would be a\nredirect from the no-slash URL (a hand-coded one, given that with this\npatch we only generate slashed URLs) to the slashed URL. Not sure it's\nworth the effort (and reparsing) though: it would obviously be MUCH\nnicer if we could change the URL without having to actually reload the\ndocument ...\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"93417","messageId":"200810191043.16395.jnareb@gmail.com","threadId":"15930","inReplyTo":"cb7bb73a0810181657i67d6b9f8o66db9f57bcf01dd3@mail.gmail.com","subject":"Re: [PATCHv6 3/5] gitweb: use_pathinfo filenames start with /","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-10-19T08:43:14Z","receivedAt":"2008-10-19T08:43:14Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sun, 19 Oct 2008, Giuseppe Bilotta wrote:\n> On Sun, Oct 19, 2008 at 1:26 AM, Jakub Narebski wrote:\n>> On Thu, 16 Oct 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 in raw\n>>> ('blob_plain') mode.\n[...]\n\n>> For example for http://repo.or.cz/w/git.git/html:/git.html links leads\n>> to correct HTML pages, while for http://repo.or.cz/w/git.git/html:git.html\n>> they lead to empty gitweb page (no errors, so link checker would be\n>> fooled).\n> \n> An idea that could be taken into consideration, if the ability to\n> navigate web documents is deemed of primary importance, would be a\n> redirect from the no-slash URL (a hand-coded one, given that with this\n> patch we only generate slashed URLs) to the slashed URL. Not sure it's\n> worth the effort (and reparsing) though: it would obviously be MUCH\n> nicer if we could change the URL without having to actually reload the\n> document ...\n\nI think that changing URL without reloading is impossible because of\nsecurity reasons. For example if you change document.location in\nJavaScript changing the URL (you can add links to non-existing anchors\nwithout reloading) then web browser reloads the page from new URL.\n\nWhat you can do is early redirect using 301 Moved Permanently (or\nsimilar) and Location: redirect, using $cgi->redirect() like in\ngit_object().\n\nAlso not sure if it is worth the effort...\n-- \nJakub Narebski\nPoland\n"},{"id":"93433","messageId":"1224418315-26164-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"15930","inReplyTo":"1224188831-17767-6-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCH 6/6] gitweb: retrieve snapshot format from PATH_INFO","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-10-19T12:11:55Z","receivedAt":"2008-10-19T12:11:55Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"We parse requests for $project/snapshot/$head.$sfx as equivalent to\n$project/snapshot/$head?sf=$sfx, where $sfx is any of the known\n(although not necessarily supported) snapshot formats (or its default\nsuffix).\n\nThe filename for the resulting package preserves the requested\nextensions (so asking for a .tgz gives a .tgz, and asking for a .tar.gz\ngives a .tar.gz), although for obvious reasons it doesn't preserve the\nbasename (git/snapshot/next.tgz returns a file names git-next.tgz).\n\nSigned-off-by: Giuseppe Bilotta \n---\n\nOk, I lied, there's a 6th patch ready. Its purpose is to allow embedding of\nthe snapshot format parameter in the URL, in the form of an extension\nappended to the refname.\n\nI have not prepared the corresponding URL generation code yet, because of\npotential ambiguity that might arise: even though at URL generation time\nthe project does not have a head named $head.$sfx, it is possible (although\nextremely unlikely) that such head is created later on, and the PATH_INFO\ngeneration code cannot therefore guarantee the permalink nature of the\nembedded snapshot format URL.\n\n gitweb/gitweb.perl | 35 +++++++++++++++++++++++++++++++++++\n 1 files changed, 35 insertions(+), 0 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 99c8c20..7ffd10b 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -609,6 +609,41 @@ sub evaluate_path_info {\n \t\t\t$input_params{'hash_parent'} ||= $parentrefname;\n \t\t}\n \t}\n+\n+\t# for the snapshot action, we allow URLs in the form\n+\t# $project/snapshot/$hash.ext\n+\t# where .ext determines the snapshot and gets removed from the\n+\t# passed $refname to provide the $hash.\n+\t#\n+\t# To be able to tell that $refname includes the format extension, we\n+\t# require some conditions to be satisfied:\n+\t# - the hash input parameter MUST have been set from the $refname part\n+\t# of the URL (i.e. they must be equal)\n+\t# - the snapshot format MUST NOT have been defined already\n+\t# - the $refname itself MUST NOT be a valid refname\n+\tif ($input_params{'action'} eq 'snapshot' && defined $refname &&\n+\t\t$refname eq $input_params{'hash'} &&\n+\t\t!defined $input_params{'snapshot_format'} &&\n+\t\t!parse_commit($refname)) {\n+\t\t# We loop over the known snapshot formats, checking for\n+\t\t# extensions. Allowed extensions are both the defined suffix\n+\t\t# (which includes the initial dot already) and the snapshot\n+\t\t# format key itself, with a prepended dot\n+\t\twhile (my ($fmt, %opt) = each %known_snapshot_formats) {\n+\t\t\tmy $hash = $refname;\n+\t\t\tmy $sfx;\n+\t\t\t$hash =~ s/(\\Q$opt{'suffix'}\\E|\\Q.$fmt\\E)$//;\n+\t\t\tnext unless $sfx = $1;\n+\t\t\t# a valid suffix was found, so set the snapshot format\n+\t\t\t# and reset the hash parameter\n+\t\t\t$input_params{'snapshot_format'} = $fmt;\n+\t\t\t$input_params{'hash'} = $hash;\n+\t\t\t# we also set the format suffix to the one requested\n+\t\t\t# in the URL: this way a request for e.g. .tgz returns\n+\t\t\t# a .tgz instead of a .tar.gz\n+\t\t\t$known_snapshot_formats{$fmt}{'suffix'} = $sfx;\n+\t\t}\n+\t}\n }\n evaluate_path_info();\n \n-- \n1.5.6.5\n"},{"id":"93424","messageId":"1224426270-27755-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"15930","inReplyTo":"1224188831-17767-6-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATHv2 6/8] gitweb: retrieve snapshot format from PATH_INFO","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-10-19T14:24:28Z","receivedAt":"2008-10-19T14:24:28Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"We parse requests for $project/snapshot/$head.$sfx as equivalent to\n$project/snapshot/$head?sf=$sfx, where $sfx is any of the known\n(although not necessarily supported) snapshot formats (or its default\nsuffix).\n\nThe filename for the resulting package preserves the requested\nextensions (so asking for a .tgz gives a .tgz, and asking for a .tar.gz\ngives a .tar.gz), although for obvious reasons it doesn't preserve the\nbasename (git/snapshot/next.tgz returns a file names git-next.tgz).\n\nThis introduces a potential case for ambiguity if a project has a head\nthat ends with a snapshot-like suffix (.zip, .tgz, .tar.gz, etc) and the\nsf CGI parameter is not present; however, gitweb only produces URLs with\nthe sf parameter, so this is only a potential issue for hand-coded URLs\nfor extremely unusual project.\n\nSigned-off-by: Giuseppe Bilotta \n---\n\nI had second thoughts on this. Now we always look for the snapshot extension if\nthe sf CGI parameter is missing, even if the project has a head that matches\nthe full pseudo-refname $head.$sfx.\n\nThe reason for this is that (1) there is no ambiguity for gitweb-generated\nURLs (2) the only URLs that could fail are hand-made URLs for extremely\nunusual projects and (3) it allows us to set gitweb up to generate\n(unambiguous) URLs without the sf CGI parameter.\n\nThis also means that I can add 3 patches to the series, instead of just one:\n* patch #6 that parses the new format\n* patch #7 that generates the new URLs\n* patch #8 for some code refactoring\n\n gitweb/gitweb.perl | 34 ++++++++++++++++++++++++++++++++++\n 1 files changed, 34 insertions(+), 0 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 99c8c20..e9e9e60 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -609,6 +609,40 @@ sub evaluate_path_info {\n \t\t\t$input_params{'hash_parent'} ||= $parentrefname;\n \t\t}\n \t}\n+\n+\t# for the snapshot action, we allow URLs in the form\n+\t# $project/snapshot/$hash.ext\n+\t# where .ext determines the snapshot and gets removed from the\n+\t# passed $refname to provide the $hash.\n+\t#\n+\t# To be able to tell that $refname includes the format extension, we\n+\t# require the following two conditions to be satisfied:\n+\t# - the hash input parameter MUST have been set from the $refname part\n+\t# of the URL (i.e. they must be equal)\n+\t# - the snapshot format MUST NOT have been defined already\n+\tif ($input_params{'action'} eq 'snapshot' && defined $refname &&\n+\t\t$refname eq $input_params{'hash'} &&\n+\t\t!defined $input_params{'snapshot_format'}) {\n+\t\t# We loop over the known snapshot formats, checking for\n+\t\t# extensions. Allowed extensions are both the defined suffix\n+\t\t# (which includes the initial dot already) and the snapshot\n+\t\t# format key itself, with a prepended dot\n+\t\twhile (my ($fmt, %opt) = each %known_snapshot_formats) {\n+\t\t\tmy $hash = $refname;\n+\t\t\tmy $sfx;\n+\t\t\t$hash =~ s/(\\Q$opt{'suffix'}\\E|\\Q.$fmt\\E)$//;\n+\t\t\tnext unless $sfx = $1;\n+\t\t\t# a valid suffix was found, so set the snapshot format\n+\t\t\t# and reset the hash parameter\n+\t\t\t$input_params{'snapshot_format'} = $fmt;\n+\t\t\t$input_params{'hash'} = $hash;\n+\t\t\t# we also set the format suffix to the one requested\n+\t\t\t# in the URL: this way a request for e.g. .tgz returns\n+\t\t\t# a .tgz instead of a .tar.gz\n+\t\t\t$known_snapshot_formats{$fmt}{'suffix'} = $sfx;\n+\t\t\tlast;\n+\t\t}\n+\t}\n }\n evaluate_path_info();\n \n-- \n1.5.6.5\n"},{"id":"93425","messageId":"1224426270-27755-2-git-send-email-giuseppe.bilotta@gmail.com","threadId":"15930","inReplyTo":"1224426270-27755-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATHv2 7/8] gitweb: embed snapshot format parameter in PATH_INFO","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-10-19T14:24:29Z","receivedAt":"2008-10-19T14:24:29Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"When PATH_INFO is active, get rid of the sf CGI parameter by embedding\nthe snapshot format information in the PATH_INFO URL, in the form of an\nappropriate extension.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 21 +++++++++++++++++++++\n 1 files changed, 21 insertions(+), 0 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex e9e9e60..5fd5a1f 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -795,6 +795,7 @@ sub href (%) {\n \t\t# - action\n \t\t# - hash_parent or hash_parent_base:/file_parent\n \t\t# - hash or hash_base:/file_name\n+\t\t# - the snapshot_format as an appropriate suffix\n \n \t\t# When the script is the root DirectoryIndex for the domain,\n \t\t# $href here would be something like http://gitweb.example.com/\n@@ -806,6 +807,10 @@ sub href (%) {\n \t\t$href .= \"/\".esc_url($params{'project'}) if defined $params{'project'};\n \t\tdelete $params{'project'};\n \n+\t\t# since we destructively absorb parameters, we keep this\n+\t\t# boolean that remembers if we're handling a snapshot\n+\t\tmy $is_snapshot = $params{'action'} eq 'snapshot';\n+\n \t\t# Summary just uses the project path URL, any other action is\n \t\t# added to the URL\n \t\tif (defined $params{'action'}) {\n@@ -845,6 +850,22 @@ sub href (%) {\n \t\t\t$href .= esc_url($params{'hash'});\n \t\t\tdelete $params{'hash'};\n \t\t}\n+\n+\t\t# If the action was a snapshot, we can absorb the\n+\t\t# snapshot_format parameter too\n+\t\tif ($is_snapshot) {\n+\t\t\tmy $fmt = $params{'snapshot_format'};\n+\t\t\t# snapshot_format should always be defined when href()\n+\t\t\t# is called, but just in case some code forgets, we\n+\t\t\t# fall back to the default\n+\t\t\tif (!$fmt) {\n+\t\t\t\tmy @snapshot_fmts = gitweb_check_feature('snapshot');\n+\t\t\t\t@snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);\n+\t\t\t\t$fmt = $snapshot_fmts[0];\n+\t\t\t}\n+\t\t\t$href .= $known_snapshot_formats{$fmt}{'suffix'};\n+\t\t\tdelete $params{'snapshot_format'};\n+\t\t}\n \t}\n \n \t# now encode the parameters explicitly\n-- \n1.5.6.5\n"},{"id":"93423","messageId":"1224426270-27755-3-git-send-email-giuseppe.bilotta@gmail.com","threadId":"15930","inReplyTo":"1224426270-27755-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATHv2 8/8] gitweb: make the supported snapshot formats array global","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-10-19T14:24:30Z","receivedAt":"2008-10-19T14:24:30Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"The @snapshot_fmts array, containing the list of the supported snapshot\nformats, was recreated locally in three different routines (with the\ndifferent name @supported_fmts in one of them).\n\nIts local generation is particularly expensive because two of the\ncallers, href() and format_snapshot_links(), are often called many times\nin a single page.\n\nSimplify code and speed up page generation by making the array global.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 21 ++++++++-------------\n 1 files changed, 8 insertions(+), 13 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 5fd5a1f..d1475b7 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -748,6 +748,10 @@ if (defined $searchtext) {\n our $git_dir;\n $git_dir = \"$projectroot/$project\" if $project;\n \n+# list of supported snapshot formats\n+our @snapshot_fmts = gitweb_check_feature('snapshot');\n+@snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);\n+\n # dispatch\n if (!defined $action) {\n \tif (defined $hash) {\n@@ -858,11 +862,7 @@ sub href (%) {\n \t\t\t# snapshot_format should always be defined when href()\n \t\t\t# is called, but just in case some code forgets, we\n \t\t\t# fall back to the default\n-\t\t\tif (!$fmt) {\n-\t\t\t\tmy @snapshot_fmts = gitweb_check_feature('snapshot');\n-\t\t\t\t@snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);\n-\t\t\t\t$fmt = $snapshot_fmts[0];\n-\t\t\t}\n+\t\t\t$fmt ||= $snapshot_fmts[0];\n \t\t\t$href .= $known_snapshot_formats{$fmt}{'suffix'};\n \t\t\tdelete $params{'snapshot_format'};\n \t\t}\n@@ -1695,8 +1695,6 @@ sub format_diff_line {\n # linked. Pass the hash of the tree/commit to snapshot.\n sub format_snapshot_links {\n \tmy ($hash) = @_;\n-\tmy @snapshot_fmts = gitweb_check_feature('snapshot');\n-\t@snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);\n \tmy $num_fmts = @snapshot_fmts;\n \tif ($num_fmts > 1) {\n \t\t# A parenthesized list of links bearing format names.\n@@ -4898,20 +4896,17 @@ sub git_tree {\n }\n \n sub git_snapshot {\n-\tmy @supported_fmts = gitweb_check_feature('snapshot');\n-\t@supported_fmts = filter_snapshot_fmts(@supported_fmts);\n-\n \tmy $format = $input_params{'snapshot_format'};\n-\tif (!@supported_fmts) {\n+\tif (!@snapshot_fmts) {\n \t\tdie_error(403, \"Snapshots not allowed\");\n \t}\n \t# default to first supported snapshot format\n-\t$format ||= $supported_fmts[0];\n+\t$format ||= $snapshot_fmts[0];\n \tif ($format !~ m/^[a-z0-9]+$/) {\n \t\tdie_error(400, \"Invalid snapshot format parameter\");\n \t} elsif (!exists($known_snapshot_formats{$format})) {\n \t\tdie_error(400, \"Unknown snapshot format\");\n-\t} elsif (!grep($_ eq $format, @supported_fmts)) {\n+\t} elsif (!grep($_ eq $format, @snapshot_fmts)) {\n \t\tdie_error(403, \"Unsupported snapshot format\");\n \t}\n \n-- \n1.5.6.5\n"},{"id":"93491","messageId":"200810201118.44654.jnareb@gmail.com","threadId":"15930","inReplyTo":"1224188831-17767-5-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv6 4/5] gitweb: parse parent..current syntax from PATH_INFO","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-10-20T09:18:43Z","receivedAt":"2008-10-20T09:18:43Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 16 Oct 2008, Giuseppe Bilotta wrote:\n\n> This patch makes it possible to use an URL such as\n> $project/somebranch..otherbranch:/filename to get a diff between\n> different version of a file. Paths like\n> $project/$action/somebranch:/somefile..otherbranch:/otherfile are parsed\n> as well.\n\nJust a nitpick: why '$project' and '$action', but 'somebranch',\n'otherbranch' and 'filename'?\n\n> \n> All '*diff' actions and in general actions that use $hash_parent[_base]\n> and $file_parent can now get all of their parameters from PATH_INFO\n\nWhich currently mean 'shortlog', and I guess in the future would mean\nalso all other log-like views: 'log', 'history', 'search' (the commit\nmessage kind, not the pickaxe kind), and perhaps also 'rss'/'atom'.\n\n> \n> Signed-off-by: Giuseppe Bilotta \n\nFor what is worth:\n\nAcked-by: Jakub Narebski \n\n> ---\n> gitweb/gitweb.perl | 36 ++++++++++++++++++++++++++++++++++--\n> 1 files changed, 34 insertions(+), 2 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 49730f3..1a7b0b9 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -548,7 +548,12 @@ sub evaluate_path_info {\n> \t\t'history',\n> \t);\n> \n> -\tmy ($refname, $pathname) = split(/:/, $path_info, 2);\n> +\t# horrible regexp to catch\n> +\t# [$hash_parent_base[:$file_parent]..]$hash_parent[:$file_name]\n\nThis comment is really nice here, to explain what we try to catch.\nIs it really \"horrible\"... let others decide.\n\nNote: this (as also previous code) makes use of the fact that refname\ncannot contain ':' as per git-check-ref-format(1), and the fact that\ngitweb limits $hash* parameters to simple revision syntax, allowing\nonly SHA1 and refnames, and not allowing (see validate_refname() used\nin $hash* validation) extended SHA1 syntax like :\nfor $hash ('h') param for 'blob' view: gitweb uses $hash_base and\n$file_name for that.\n\n(But I guess it is too long explanation to put it in comment)\n\n\nSide note: the regexp below allow for $parentpathname to contain\n'..', but we don't want to rely on such minute detail of implementation\ndetail (because it depends on whether we use greedy or non-greedy\nmatching there).\n\n> +\tmy ($parentrefname, $parentpathname, $refname, $pathname) =\n> +\t\t($path_info =~ /^(?:(.+?)(?::(.+))?\\.\\.)?(.+?)(?::(.+))?$/);\n> +\n> +\t# first, analyze the 'current' part\n> \tif (defined $pathname) {\n> \t\t# we got \"branch:filename\" or \"branch:dir/\"\n> \t\t# we could use git_get_type(branch:pathname), but it needs $git_dir\n> @@ -557,7 +562,13 @@ sub evaluate_path_info {\n> \t\t\t$input_params{'action'} ||= \"tree\";\n> \t\t\t$pathname =~ s,/$,,;\n> \t\t} else {\n> -\t\t\t$input_params{'action'} ||= \"blob_plain\";\n> +\t\t\t# the default action depends on whether we had parent info\n> +\t\t\t# or not\n> +\t\t\tif ($parentrefname) {\n> +\t\t\t\t$input_params{'action'} ||= \"blobdiff_plain\";\n> +\t\t\t} else {\n> +\t\t\t\t$input_params{'action'} ||= \"blob_plain\";\n> +\t\t\t}\n\nNice.\n\nI was wondering about 'project/hash_parent..hash' syntax, but then I have\nrealized that it doesn't change action (as in 'blob_plain' -> 'blobdiff_plain'),\nbut is always 'shortlog'.\n\nBy the way, I wonder if it should be 'blobdiff_plain' or just 'blobdiff'.\nthe 'blob_plain' was here to use gitweb as a kind of versioned web\nserver; there is no such equivalent for 'p/hbp..hb:f' syntax. On the\nother hand it is consistent behavior, always using *_plain...\n\n> \t\t}\n> \t\t$input_params{'hash_base'} ||= $refname;\n> \t\t$input_params{'file_name'} ||= $pathname;\n> @@ -577,6 +588,27 @@ sub evaluate_path_info {\n> \t\t\t$input_params{'hash'} ||= $refname;\n> \t\t}\n> \t}\n> +\n> +\t# next, handle the 'parent' part, if present\n> +\tif (defined $parentrefname) {\n> +\t\t# a missing pathspec defaults to the 'current' filename, allowing e.g.\n> +\t\t# someproject/blobdiff/oldrev..newrev:/filename\n> +\t\tif ($parentpathname) {\n> +\t\t\t$parentpathname =~ s,^/+,,;\n> +\t\t\t$parentpathname =~ s,/$,,;\n\nHmmm... should we strip trailing '/' here?\n\n> +\t\t\t$input_params{'file_parent'} ||= $parentpathname;\n> +\t\t} else {\n> +\t\t\t$input_params{'file_parent'} ||= $input_params{'file_name'};\n> +\t\t}\n> +\t\t# we assume that hash_parent_base is wanted if a path was specified,\n> +\t\t# or if the action wants hash_base instead of hash\n\nNice comment.\n\n> +\t\tif (defined $input_params{'file_parent'} ||\n> +\t\t\tgrep {$input_params{'action'} eq $_} @wants_base) {\n\nMy preference of style would be to use here:\n\n+\t\t grep { $input_params{'action'} eq $_ } @wants_base) {\n\n> +\t\t\t$input_params{'hash_parent_base'} ||= $parentrefname;\n> +\t\t} else {\n> +\t\t\t$input_params{'hash_parent'} ||= $parentrefname;\n> +\t\t}\n> +\t}\n> }\n> evaluate_path_info();\n> \n> -- \n> 1.5.6.5\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"93494","messageId":"200810201249.14426.jnareb@gmail.com","threadId":"15930","inReplyTo":"1224188831-17767-6-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv6 5/5] gitweb: generate parent..current URLs","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-10-20T10:49:13Z","receivedAt":"2008-10-20T10:49:13Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 16 Oct 2008, Giuseppe Bilotta wrote:\n\n> If use_pathinfo is enabled, href now creates links that contain paths in\n> the form $project/$action/oldhash:/oldname..newhash:/newname for actions\n> that use hash_parent etc.\n> \n> If any of the filename contains two consecutive dots, it's kept as a CGI\n> parameter since the resulting path would otherwise be ambiguous.\n\nNote that this part is _required_ after previous patch, because\notherwise gitweb could generate path_info links which point to\ndifferent view than intended, when filename contains '..' in it,\nlike for example some test vectors in git test suite, for example\nfiles in t/t5515\n\n> \n> Signed-off-by: Giuseppe Bilotta \n\nFor what is worth:\n\nAcked-by: Jakub Narebski \n\n\nBTW. I wanted to test this using gitweb-Mechanize test from Lea Wiemann\nGSoC 2008 work on gitweb caching, but I couldn't make it run as single\nindividual test (cd t && ./t9503-gitweb-Mechanize.sh); it requires to\nrun as \"make test\" and I didn't want that. I could have resurrected my\ninitial version of Test::WWW::Mechanize::CGI test...\n\n> ---\n> gitweb/gitweb.perl | 30 +++++++++++++++++++++++++-----\n> 1 files changed, 25 insertions(+), 5 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 1a7b0b9..f4642e7 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -759,7 +759,8 @@ sub href (%) {\n> \t\t# try to put as many parameters as possible in PATH_INFO:\n> \t\t# - project name\n> \t\t# - action\n> -\t\t# - hash or hash_base:/filename\n> +\t\t# - hash_parent or hash_parent_base:/file_parent\n> +\t\t# - hash or hash_base:/file_name\n\nMinor nit: this contain independent change 'filename' -> 'file_name',\nbut I think it is not worth separating...\n\n> \n> \t\t# When the script is the root DirectoryIndex for the domain,\n> \t\t# $href here would be something like http://gitweb.example.com/\n> @@ -778,17 +779,36 @@ sub href (%) {\n> \t\t\tdelete $params{'action'};\n> \t\t}\n> \n> -\t\t# Finally, we put either hash_base:/file_name or hash\n> +\t\t# Next, we put hash_parent_base:/file_parent..hash_base:/file_name,\n> +\t\t# stripping nonexistent or useless pieces\n> +\t\t$href .= \"/\" if ($params{'hash_base'} || $params{'hash_parent_base'}\n> +\t\t\t|| $params{'hash_parent'} || $params{'hash'});\n\nNice trick (and required change).\n\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\tif (defined $params{'hash_parent_base'}) {\n> +\t\t\t\t$href .= esc_url($params{'hash_parent_base'});\n> +\t\t\t\t# skip the file_parent if it's the same as the file_name\n> +\t\t\t\tdelete $params{'file_parent'} if $params{'file_parent'} eq $params{'file_name'};\n> +\t\t\t\tif (defined $params{'file_parent'} && $params{'file_parent'} !~ /\\.\\./) {\n> +\t\t\t\t\t$href .= \":/\".esc_url($params{'file_parent'});\n> +\t\t\t\t\tdelete $params{'file_parent'};\n> +\t\t\t\t}\n\nSide note: I wonder if we should use esc_url or esc_param here...\n\n> +\t\t\t\t$href .= \"..\";\n> +\t\t\t\tdelete $params{'hash_parent'};\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'} && $params{'file_name'} !~ /\\.\\./) {\n> \t\t\t\t$href .= \":/\".esc_url($params{'file_name'});\n> \t\t\t\tdelete $params{'file_name'};\n> \t\t\t}\n> \t\t\tdelete $params{'hash'};\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> -- \n> 1.5.6.5\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"93493","messageId":"cb7bb73a0810200752l70e1952dmb8e14a419b02f61b@mail.gmail.com","threadId":"15930","inReplyTo":"200810201118.44654.jnareb@gmail.com","subject":"Re: [PATCHv6 4/5] gitweb: parse parent..current syntax from PATH_INFO","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-10-20T14:52:39Z","receivedAt":"2008-10-20T14:52:39Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Mon, Oct 20, 2008 at 11:18 AM, Jakub Narebski wrote:\n> On Thu, 16 Oct 2008, Giuseppe Bilotta wrote:\n>\n>> This patch makes it possible to use an URL such as\n>> $project/somebranch..otherbranch:/filename to get a diff between\n>> different version of a file. Paths like\n>> $project/$action/somebranch:/somefile..otherbranch:/otherfile are parsed\n>> as well.\n>\n> Just a nitpick: why '$project' and '$action', but 'somebranch',\n> 'otherbranch' and 'filename'?\n\nGood question ... I think I got distracted along the way.\n\n>> All '*diff' actions and in general actions that use $hash_parent[_base]\n>> and $file_parent can now get all of their parameters from PATH_INFO\n>\n> Which currently mean 'shortlog', and I guess in the future would mean\n> also all other log-like views: 'log', 'history', 'search' (the commit\n> message kind, not the pickaxe kind), and perhaps also 'rss'/'atom'.\n\nI'm not sure rss/atom makes sense, but the others were already in my\ntodo list after the shortlog patch, so I'll try to get them ready as\nsoon as I have the time to refactor them as we discussed on IRC.\n\n> Side note: the regexp below allow for $parentpathname to contain\n> '..', but we don't want to rely on such minute detail of implementation\n> detail (because it depends on whether we use greedy or non-greedy\n> matching there).\n>\n>> + my ($parentrefname, $parentpathname, $refname, $pathname) =\n>> + ($path_info =~ /^(?:(.+?)(?::(.+))?\\.\\.)?(.+?)(?::(.+))?$/);\n\nHm, actually it might be a better idea to make the first pathname\nmatch non-greedy.\n\n>> - $input_params{'action'} ||= \"blob_plain\";\n>> + # the default action depends on whether we had parent info\n>> + # or not\n>> + if ($parentrefname) {\n>> + $input_params{'action'} ||= \"blobdiff_plain\";\n>> + } else {\n>> + $input_params{'action'} ||= \"blob_plain\";\n>> + }\n>\n> Nice.\n>\n> I was wondering about 'project/hash_parent..hash' syntax, but then I have\n> realized that it doesn't change action (as in 'blob_plain' -> 'blobdiff_plain'),\n> but is always 'shortlog'.\n>\n> By the way, I wonder if it should be 'blobdiff_plain' or just 'blobdiff'.\n> the 'blob_plain' was here to use gitweb as a kind of versioned web\n> server; there is no such equivalent for 'p/hbp..hb:f' syntax. On the\n> other hand it is consistent behavior, always using *_plain...\n\nMoreover, it allows sending shorter URLs for patches, which are the\nones you usually write by hand.\n\n>> +\n>> + # next, handle the 'parent' part, if present\n>> + if (defined $parentrefname) {\n>> + # a missing pathspec defaults to the 'current' filename, allowing e.g.\n>> + # someproject/blobdiff/oldrev..newrev:/filename\n>> + if ($parentpathname) {\n>> + $parentpathname =~ s,^/+,,;\n>> + $parentpathname =~ s,/$,,;\n>\n> Hmmm... should we strip trailing '/' here?\n\nI must confess I don't remember why I decided that was needed.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"93504","messageId":"cb7bb73a0810200757h540a699qd9d3ea236fc7b5ad@mail.gmail.com","threadId":"15930","inReplyTo":"200810201249.14426.jnareb@gmail.com","subject":"Re: [PATCHv6 5/5] gitweb: generate parent..current URLs","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-10-20T14:57:44Z","receivedAt":"2008-10-20T14:57:44Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Mon, Oct 20, 2008 at 12:49 PM, Jakub Narebski wrote:\n>> - # - hash or hash_base:/filename\n>> + # - hash_parent or hash_parent_base:/file_parent\n>> + # - hash or hash_base:/file_name\n>\n> Minor nit: this contain independent change 'filename' -> 'file_name',\n> but I think it is not worth separating...\n\nOopsie. Oh well, I was getting so used to all those _ that it felt\nstrange without\n\n>> # When the script is the root DirectoryIndex for the domain,\n>> # $href here would be something like http://gitweb.example.com/\n>> @@ -778,17 +779,36 @@ sub href (%) {\n>> delete $params{'action'};\n>> }\n>>\n>> - # Finally, we put either hash_base:/file_name or hash\n>> + # Next, we put hash_parent_base:/file_parent..hash_base:/file_name,\n>> + # stripping nonexistent or useless pieces\n>> + $href .= \"/\" if ($params{'hash_base'} || $params{'hash_parent_base'}\n>> + || $params{'hash_parent'} || $params{'hash'});\n>\n> Nice trick (and required change).\n>\n>> if (defined $params{'hash_base'}) {\n>> - $href .= \"/\".esc_url($params{'hash_base'});\n>> - if (defined $params{'file_name'}) {\n>> + if (defined $params{'hash_parent_base'}) {\n>> + $href .= esc_url($params{'hash_parent_base'});\n>> + # skip the file_parent if it's the same as the file_name\n>> + delete $params{'file_parent'} if $params{'file_parent'} eq $params{'file_name'};\n>> + if (defined $params{'file_parent'} && $params{'file_parent'} !~ /\\.\\./) {\n>> + $href .= \":/\".esc_url($params{'file_parent'});\n>> + delete $params{'file_parent'};\n>> + }\n>\n> Side note: I wonder if we should use esc_url or esc_param here...\n\nesc_url, I would say, allowing us to build RFC-compliant URLs. Isn't\nesc_param for CGI?\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"93613","messageId":"200810211844.35714.jnareb@gmail.com","threadId":"15930","inReplyTo":"1224426270-27755-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATHv2 6/8] gitweb: retrieve snapshot format from PATH_INFO","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-10-21T16:44:34Z","receivedAt":"2008-10-21T16:44:34Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"I like the idea behind this patch, to enable to use path_info for as\nmuch gitweb parameters as possible. After this patch series the only\nparameters which wouldn't be possible to represent in path_info would\nbe: \n * @extra_options ('opt') multi-valued parameter, used to pass\n thinks like '--no-merges', which cannot be fit in the \"simplified\"\n list-like (as opposed to hash-like query string) path_info URL.\n * $searchtype ('st') and $searchtext ('s') etc. parameters, which\n are generated by HTML form, and are naturally generated in query\n string format.\n * $page ('pg') parameter, which could theoretically be added as last\n part of path_info URL, for example $project/next/2/... if not for\n pesky $project/history/next:/Documentation/2/ where you cannot be\n sure that having // at the end is rare.\n * $order ('o') parameter, which would be hard to fit in path_info,\n with its limitation of parameters being specified by position.\n Or even next to impossible.\n * 'by_tag'...\n\nBut I'd rather have this patch series to be in separate thread...\n\n\nOn Sun, 19 Oct 2008, Giuseppe Bilotta wrote:\n\n> We parse requests for $project/snapshot/$head.$sfx as equivalent to\n> $project/snapshot/$head?sf=$sfx, where $sfx is any of the known\n> (although not necessarily supported) snapshot formats (or its default\n> suffix).\n> \n> The filename for the resulting package preserves the requested\n> extensions (so asking for a .tgz gives a .tgz, and asking for a .tar.gz\n> gives a .tar.gz), although for obvious reasons it doesn't preserve the\n> basename (git/snapshot/next.tgz returns a file names git-next.tgz).\n\nThat is a bit of difference from sf= in CGI query string, where\n is always a name of a format (for example 'tgz' or 'tbz2'), \nand actual suffix is defined in %known_snapshot_formats (for example\n'.tar.gz' and '.tar.bz2' respectively). Now you can specify snapshot\nformat either either by its name, for example 'tgz' (which is simple\nlookup in hash) which result in proposed filename with '.tgz' suffix,\nor you can specify suffix, for example 'tar.gz' (which requires\nsearching through all hash) which result in proposed filename with\n'.tar.gz' suffix.\n\nThis is a bit of inconsistency; to be consistent with how we handle\n'sf' CGI parameter we would translate 'tgz' $sfx into 'tar.gz' in\nsnapshot filename. This would also cover currently purely theoretical\ncase when different snapshot formats (for example 'tgz' and 'tgz9')\nwould use the same snapshot suffix (extension), but differ for example\nin parameters passed to compressor (for example '-9' or '--best' in\nthe 'tgz9' case).\n\nOn the other hand one would expect that when URL which looks like\nURL to snapshot ends with '.$sfx', then filename for snapshot would\nalso end with '.$sfx'.\n\nThis certainly requires some further thoughts.\n\n> \n> This introduces a potential case for ambiguity if a project has a head\n> that ends with a snapshot-like suffix (.zip, .tgz, .tar.gz, etc) and the\n> sf CGI parameter is not present; however, gitweb only produces URLs with\n> the sf parameter, so this is only a potential issue for hand-coded URLs\n> for extremely unusual project.\n\nI think you wanted to say here \"_currently_ produces URLs with the 'sf'\nparameter\" as the next patch in series changes this.\n\n> \n> Signed-off-by: Giuseppe Bilotta \n> ---\n> \n> I had second thoughts on this. Now we always look for the snapshot extension if\n> the sf CGI parameter is missing, even if the project has a head that matches\n> the full pseudo-refname $head.$sfx.\n> \n> The reason for this is that (1) there is no ambiguity for gitweb-generated\n> URLs (2) the only URLs that could fail are hand-made URLs for extremely\n> unusual projects and (3) it allows us to set gitweb up to generate\n> (unambiguous) URLs without the sf CGI parameter.\n\nThis is also simpler and cheaper solution.\n\n> \n> This also means that I can add 3 patches to the series, instead of just one:\n> * patch #6 that parses the new format\n> * patch #7 that generates the new URLs\n> * patch #8 for some code refactoring\n\nNow, I haven't yet read the last patch in series, so I don't know if\nit is independent refactoring, making sense even before patches named\n#6 and #7 here, or is it connected with searching for snapshot format\nby suffix it uses. If the former, it should be done upfront, as it\nshouldn't need discussion, and being easier to be accepted into git.git.\nIf the latter, then it should probably be folded (squashed) into #6,\nfirst patch in the series.\n\n> \n> gitweb/gitweb.perl | 34 ++++++++++++++++++++++++++++++++++\n> 1 files changed, 34 insertions(+), 0 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 99c8c20..e9e9e60 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -609,6 +609,40 @@ sub evaluate_path_info {\n> \t\t\t$input_params{'hash_parent'} ||= $parentrefname;\n> \t\t}\n> \t}\n> +\n> +\t# for the snapshot action, we allow URLs in the form\n> +\t# $project/snapshot/$hash.ext\n> +\t# where .ext determines the snapshot and gets removed from the\n> +\t# passed $refname to provide the $hash.\n> +\t#\n> +\t# To be able to tell that $refname includes the format extension, we\n> +\t# require the following two conditions to be satisfied:\n> +\t# - the hash input parameter MUST have been set from the $refname part\n> +\t# of the URL (i.e. they must be equal)\n\nThis means no \"$project/.tgz?h=next\", isn't it?\n\n> +\t# - the snapshot format MUST NOT have been defined already\n\nI would add \"which means that 'sf' parameter is not set in URL\", or\nsomething like that as the last line of above comment.\n\nI like that the code is so well commented, by the way.\n\n> +\tif ($input_params{'action'} eq 'snapshot' && defined $refname &&\n> +\t\t$refname eq $input_params{'hash'} &&\n\nMinor nit.\n\nI would use here (the question of style / better readability):\n\n+\tif ($input_params{'action'} eq 'snapshot' &&\n+\t\t defined $refname && $refname eq $input_params{'hash'} &&\n\nto have both conditions about $refname in the same line.\n\n> +\t\t!defined $input_params{'snapshot_format'}) {\n> +\t\t# We loop over the known snapshot formats, checking for\n> +\t\t# extensions. Allowed extensions are both the defined suffix\n> +\t\t# (which includes the initial dot already) and the snapshot\n> +\t\t# format key itself, with a prepended dot\n> +\t\twhile (my ($fmt, %opt) = each %known_snapshot_formats) {\n> +\t\t\tmy $hash = $refname;\n> +\t\t\tmy $sfx;\n> +\t\t\t$hash =~ s/(\\Q$opt{'suffix'}\\E|\\Q.$fmt\\E)$//;\n> +\t\t\tnext unless $sfx = $1;\n> +\t\t\t# a valid suffix was found, so set the snapshot format\n> +\t\t\t# and reset the hash parameter\n> +\t\t\t$input_params{'snapshot_format'} = $fmt;\n> +\t\t\t$input_params{'hash'} = $hash;\n> +\t\t\t# we also set the format suffix to the one requested\n> +\t\t\t# in the URL: this way a request for e.g. .tgz returns\n> +\t\t\t# a .tgz instead of a .tar.gz\n> +\t\t\t$known_snapshot_formats{$fmt}{'suffix'} = $sfx;\n> +\t\t\tlast;\n> +\t\t}\n\nI'm not sure if it worth (see comment at the beginning of this mail)\nadding this code, or just allow $sfx to be snapshot _name_ (key in\n%known_snapshot_formats hash).\n\nOtherwise it would be as simple as checking if $known_snapshot_formats{$sfx}\nexists (assuming that snapshot format names does not contain '.').\n\nIf we decide to go more complicated route, then refactoring it in such\na way that suffixes are also keys to %known_snapshot_formats would be\npreferred... err, sorry, not so simple. But refactoring this check\ninto separate subroutine (as I think last patch in series does) would\nbe good idea.\n\n\nAlso, I'd rather you checked if the $refname part contains '.' for it\nto even consider that it can be suffix.\n\n> +\t}\n> }\n> evaluate_path_info();\n> \n> -- \n> 1.5.6.5\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"93621","messageId":"cb7bb73a0810211136n452ac8bdp7814ff09749b3142@mail.gmail.com","threadId":"15930","inReplyTo":"200810211844.35714.jnareb@gmail.com","subject":"Re: [PATHv2 6/8] gitweb: retrieve snapshot format from PATH_INFO","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-10-21T18:36:41Z","receivedAt":"2008-10-21T18:36:41Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Tue, Oct 21, 2008 at 6:44 PM, Jakub Narebski wrote:\n> I like the idea behind this patch, to enable to use path_info for as\n> much gitweb parameters as possible. After this patch series the only\n> parameters which wouldn't be possible to represent in path_info would\n> be:\n> * @extra_options ('opt') multi-valued parameter, used to pass\n> thinks like '--no-merges', which cannot be fit in the \"simplified\"\n> list-like (as opposed to hash-like query string) path_info URL.\n> * $searchtype ('st') and $searchtext ('s') etc. parameters, which\n> are generated by HTML form, and are naturally generated in query\n> string format.\n> * $page ('pg') parameter, which could theoretically be added as last\n> part of path_info URL, for example $project/next/2/... if not for\n> pesky $project/history/next:/Documentation/2/ where you cannot be\n> sure that having // at the end is rare.\n> * $order ('o') parameter, which would be hard to fit in path_info,\n> with its limitation of parameters being specified by position.\n> Or even next to impossible.\n> * 'by_tag'...\n>\n> But I'd rather have this patch series to be in separate thread...\n\nYes, a posteriori I think it's better too. I'll resend the 5 path_info\npatches with the minor stylistic corrections you suggested, and send\nthese 3 separately.\n\n\n> On Sun, 19 Oct 2008, Giuseppe Bilotta wrote:\n>\n>> We parse requests for $project/snapshot/$head.$sfx as equivalent to\n>> $project/snapshot/$head?sf=$sfx, where $sfx is any of the known\n>> (although not necessarily supported) snapshot formats (or its default\n>> suffix).\n>>\n>> The filename for the resulting package preserves the requested\n>> extensions (so asking for a .tgz gives a .tgz, and asking for a .tar.gz\n>> gives a .tar.gz), although for obvious reasons it doesn't preserve the\n>> basename (git/snapshot/next.tgz returns a file names git-next.tgz).\n>\n> That is a bit of difference from sf= in CGI query string, where\n> is always a name of a format (for example 'tgz' or 'tbz2'),\n> and actual suffix is defined in %known_snapshot_formats (for example\n> '.tar.gz' and '.tar.bz2' respectively). Now you can specify snapshot\n> format either either by its name, for example 'tgz' (which is simple\n> lookup in hash) which result in proposed filename with '.tgz' suffix,\n> or you can specify suffix, for example 'tar.gz' (which requires\n> searching through all hash) which result in proposed filename with\n> '.tar.gz' suffix.\n>\n> This is a bit of inconsistency; to be consistent with how we handle\n> 'sf' CGI parameter we would translate 'tgz' $sfx into 'tar.gz' in\n> snapshot filename. This would also cover currently purely theoretical\n> case when different snapshot formats (for example 'tgz' and 'tgz9')\n> would use the same snapshot suffix (extension), but differ for example\n> in parameters passed to compressor (for example '-9' or '--best' in\n> the 'tgz9' case).\n>\n> On the other hand one would expect that when URL which looks like\n> URL to snapshot ends with '.$sfx', then filename for snapshot would\n> also end with '.$sfx'.\n>\n> This certainly requires some further thoughts.\n\nWhat I decided was to set gitweb to always produce links with the\nsuffix (.e.g .tar.gz), but I saw no particular reason not to accept\nthe shorter version which is (1) commonly used as a suffix as well and\n(2) happens to be the actual format key used by gitweb.\n\nA different, possibly cleaner approach, but a more extensive change,\nwould be to have each format describe a list of suffixes, defaulting\nto the first one on creation by identifying all of them. This is more\ninvasive because all of the uses of {'suffix'} have to be replaced\nwith {'suffix'}[0], or something like that (maybe we could add a\nseparate key 'other_suffixes' instead?)\n\n\n>> This introduces a potential case for ambiguity if a project has a head\n>> that ends with a snapshot-like suffix (.zip, .tgz, .tar.gz, etc) and the\n>> sf CGI parameter is not present; however, gitweb only produces URLs with\n>> the sf parameter, so this is only a potential issue for hand-coded URLs\n>> for extremely unusual project.\n>\n> I think you wanted to say here \"_currently_ produces URLs with the 'sf'\n> parameter\" as the next patch in series changes this.\n\nAh yes, good point.\n\n>> Signed-off-by: Giuseppe Bilotta \n>> ---\n>>\n>> I had second thoughts on this. Now we always look for the snapshot extension if\n>> the sf CGI parameter is missing, even if the project has a head that matches\n>> the full pseudo-refname $head.$sfx.\n>>\n>> The reason for this is that (1) there is no ambiguity for gitweb-generated\n>> URLs (2) the only URLs that could fail are hand-made URLs for extremely\n>> unusual projects and (3) it allows us to set gitweb up to generate\n>> (unambiguous) URLs without the sf CGI parameter.\n>\n> This is also simpler and cheaper solution.\n\nThat, too 8-)\n\n>> This also means that I can add 3 patches to the series, instead of just one:\n>> * patch #6 that parses the new format\n>> * patch #7 that generates the new URLs\n>> * patch #8 for some code refactoring\n>\n> Now, I haven't yet read the last patch in series, so I don't know if\n> it is independent refactoring, making sense even before patches named\n> #6 and #7 here, or is it connected with searching for snapshot format\n> by suffix it uses. If the former, it should be done upfront, as it\n> shouldn't need discussion, and being easier to be accepted into git.git.\n> If the latter, then it should probably be folded (squashed) into #6,\n> first patch in the series.\n\nIn fact, patch #8 can be written independently of the other too, and\nwould provide a significant speed benefit for generation of pages with\nlots of 'snapshot' links: what it does is just to make the 'supported\nformats' array global, preparing it only once instead of re-preparing\nit every time a snapshot link is created.\n\n>> gitweb/gitweb.perl | 34 ++++++++++++++++++++++++++++++++++\n>> 1 files changed, 34 insertions(+), 0 deletions(-)\n>>\n>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>> index 99c8c20..e9e9e60 100755\n>> --- a/gitweb/gitweb.perl\n>> +++ b/gitweb/gitweb.perl\n>> @@ -609,6 +609,40 @@ sub evaluate_path_info {\n>> $input_params{'hash_parent'} ||= $parentrefname;\n>> }\n>> }\n>> +\n>> + # for the snapshot action, we allow URLs in the form\n>> + # $project/snapshot/$hash.ext\n>> + # where .ext determines the snapshot and gets removed from the\n>> + # passed $refname to provide the $hash.\n>> + #\n>> + # To be able to tell that $refname includes the format extension, we\n>> + # require the following two conditions to be satisfied:\n>> + # - the hash input parameter MUST have been set from the $refname part\n>> + # of the URL (i.e. they must be equal)\n>\n> This means no \"$project/.tgz?h=next\", isn't it?\n\nRight.\n\n>> + # - the snapshot format MUST NOT have been defined already\n>\n> I would add \"which means that 'sf' parameter is not set in URL\", or\n> something like that as the last line of above comment.\n\nGood idea. I'll make it an 'e.g.' to keep the comment valid for future\nadditional parameter evaluation such as command-line input.\n\n> I like that the code is so well commented, by the way.\n\nThanks.\n\n>> + if ($input_params{'action'} eq 'snapshot' && defined $refname &&\n>> + $refname eq $input_params{'hash'} &&\n>\n> Minor nit.\n>\n> I would use here (the question of style / better readability):\n>\n> + if ($input_params{'action'} eq 'snapshot' &&\n> + defined $refname && $refname eq $input_params{'hash'} &&\n>\n> to have both conditions about $refname in the same line.\n\nYes, it'd look much better.\n\n>> + !defined $input_params{'snapshot_format'}) {\n>> + # We loop over the known snapshot formats, checking for\n>> + # extensions. Allowed extensions are both the defined suffix\n>> + # (which includes the initial dot already) and the snapshot\n>> + # format key itself, with a prepended dot\n>> + while (my ($fmt, %opt) = each %known_snapshot_formats) {\n>> + my $hash = $refname;\n>> + my $sfx;\n>> + $hash =~ s/(\\Q$opt{'suffix'}\\E|\\Q.$fmt\\E)$//;\n>> + next unless $sfx = $1;\n>> + # a valid suffix was found, so set the snapshot format\n>> + # and reset the hash parameter\n>> + $input_params{'snapshot_format'} = $fmt;\n>> + $input_params{'hash'} = $hash;\n>> + # we also set the format suffix to the one requested\n>> + # in the URL: this way a request for e.g. .tgz returns\n>> + # a .tgz instead of a .tar.gz\n>> + $known_snapshot_formats{$fmt}{'suffix'} = $sfx;\n>> + last;\n>> + }\n>\n> I'm not sure if it worth (see comment at the beginning of this mail)\n> adding this code, or just allow $sfx to be snapshot _name_ (key in\n> %known_snapshot_formats hash).\n>\n> Otherwise it would be as simple as checking if $known_snapshot_formats{$sfx}\n> exists (assuming that snapshot format names does not contain '.').\n>\n> If we decide to go more complicated route, then refactoring it in such\n> a way that suffixes are also keys to %known_snapshot_formats would be\n> preferred... err, sorry, not so simple. But refactoring this check\n> into separate subroutine (as I think last patch in series does) would\n> be good idea.\n\nSee comments above.\n\n> Also, I'd rather you checked if the $refname part contains '.' for it\n> to even consider that it can be suffix.\n\nAh, good idea.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"93623","messageId":"7vtzb5ydd1.fsf@gitster.siamese.dyndns.org","threadId":"15930","inReplyTo":"cb7bb73a0810211136n452ac8bdp7814ff09749b3142@mail.gmail.com","subject":"Re: [PATHv2 6/8] gitweb: retrieve snapshot format from PATH_INFO","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-21T19:09:14Z","receivedAt":"2008-10-21T19:09:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Giuseppe Bilotta\" writes:\n\n>> But I'd rather have this patch series to be in separate thread...\n>\n> Yes, a posteriori I think it's better too. I'll resend the 5 path_info\n> patches with the minor stylistic corrections you suggested, and send\n> these 3 separately.\n\nI've only been watching from the sidelines, but the discussion seemed\nsensible. With the resend with Acks I think they are already 'next'\nmaterial.\n\nThanks, both of you. \n"},{"id":"94505","messageId":"200811010118.14191.jnareb@gmail.com","threadId":"15930","inReplyTo":"1224426270-27755-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATHv2 7/8] gitweb: embed snapshot format parameter in PATH_INFO","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-11-01T00:18:13Z","receivedAt":"2008-11-01T00:18:13Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sun, 19 Oct 2008, Giuseppe Bilotta wrote:\n\nI'm sorry for the delay.\n\n> When PATH_INFO is active, get rid of the sf CGI parameter by embedding\n> the snapshot format information in the PATH_INFO URL, in the form of an\n> appropriate extension.\n\nThe question is: should we use format suffix (e.g. 'tar.gz'),\nor format name (e.g. 'tgz')? The latter is later easier to parse,\nsee comments to first patch in better snapshot support for path_info.\n\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index e9e9e60..5fd5a1f 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -795,6 +795,7 @@ sub href (%) {\n> \t\t# - action\n> \t\t# - hash_parent or hash_parent_base:/file_parent\n> \t\t# - hash or hash_base:/file_name\n> +\t\t# - the snapshot_format as an appropriate suffix\n> \n> \t\t# When the script is the root DirectoryIndex for the domain,\n> \t\t# $href here would be something like http://gitweb.example.com/\n\nGood.\n\n> @@ -806,6 +807,10 @@ sub href (%) {\n> \t\t$href .= \"/\".esc_url($params{'project'}) if defined $params{'project'};\n> \t\tdelete $params{'project'};\n> \n> +\t\t# since we destructively absorb parameters, we keep this\n> +\t\t# boolean that remembers if we're handling a snapshot\n> +\t\tmy $is_snapshot = $params{'action'} eq 'snapshot';\n> +\n\nSide note: we destructively absorb parameters, because parameters\nwhich are not absorbed are then used to generate query string part\nof URL.\n\nDeleting parameter but remembering the fact that it was used is one\n(but not only) solution.\n\n> \t\t# Summary just uses the project path URL, any other action is\n> \t\t# added to the URL\n> \t\tif (defined $params{'action'}) {\n> @@ -845,6 +850,22 @@ sub href (%) {\n> \t\t\t$href .= esc_url($params{'hash'});\n> \t\t\tdelete $params{'hash'};\n> \t\t}\n> +\n> +\t\t# If the action was a snapshot, we can absorb the\n> +\t\t# snapshot_format parameter too\n> +\t\tif ($is_snapshot) {\n> +\t\t\tmy $fmt = $params{'snapshot_format'};\n> +\t\t\t# snapshot_format should always be defined when href()\n> +\t\t\t# is called, but just in case some code forgets, we\n> +\t\t\t# fall back to the default\n\n> +\t\t\tif (!$fmt) {\n> +\t\t\t\tmy @snapshot_fmts = gitweb_check_feature('snapshot');\n> +\t\t\t\t@snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);\n> +\t\t\t\t$fmt = $snapshot_fmts[0];\n> +\t\t\t}\n\nI anderstand that the above code is improved with new patch?\n\n> +\t\t\t$href .= $known_snapshot_formats{$fmt}{'suffix'};\n\nAgain: should we use snapshot prefix, or snapshot name, which means here\ndo we use $known_snapshot_formats{$fmt}{'suffix'}; or just $fmt; ?\n\n> +\t\t\tdelete $params{'snapshot_format'};\n> +\t\t}\n> \t}\n> \n> \t# now encode the parameters explicitly\n> -- \n> 1.5.6.5\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"94555","messageId":"cb7bb73a0811010557u31ce5f69o9b4b0d53a102498a@mail.gmail.com","threadId":"15930","inReplyTo":"200811010118.14191.jnareb@gmail.com","subject":"Re: [PATHv2 7/8] gitweb: embed snapshot format parameter in PATH_INFO","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-11-01T12:57:52Z","receivedAt":"2008-11-01T12:57:52Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Sat, Nov 1, 2008 at 1:18 AM, Jakub Narebski wrote:\n> On Sun, 19 Oct 2008, Giuseppe Bilotta wrote:\n>\n> I'm sorry for the delay.\n\nNo problem.\n\n>> When PATH_INFO is active, get rid of the sf CGI parameter by embedding\n>> the snapshot format information in the PATH_INFO URL, in the form of an\n>> appropriate extension.\n>\n> The question is: should we use format suffix (e.g. 'tar.gz'),\n> or format name (e.g. 'tgz')? The latter is later easier to parse,\n> see comments to first patch in better snapshot support for path_info.\n\nI think it's essentially a matter of deciding which extension to use\nby default on output, and accepting either extension on input.\n\n\n>> + # since we destructively absorb parameters, we keep this\n>> + # boolean that remembers if we're handling a snapshot\n>> + my $is_snapshot = $params{'action'} eq 'snapshot';\n>> +\n>\n> Side note: we destructively absorb parameters, because parameters\n> which are not absorbed are then used to generate query string part\n> of URL.\n>\n> Deleting parameter but remembering the fact that it was used is one\n> (but not only) solution.\n\nWell, in the specific case of the 'action' parameter we could just\nkeep it around and only discard it at the end.\n\n>> + if (!$fmt) {\n>> + my @snapshot_fmts = gitweb_check_feature('snapshot');\n>> + @snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);\n>> + $fmt = $snapshot_fmts[0];\n>> + }\n>\n> I anderstand that the above code is improved with new patch?\n\nYup, I followed your suggestion of putting the refactoring earlier.\n\n>> + $href .= $known_snapshot_formats{$fmt}{'suffix'};\n>\n> Again: should we use snapshot prefix, or snapshot name, which means here\n> do we use $known_snapshot_formats{$fmt}{'suffix'}; or just $fmt; ?\n\nI really don't know, either is fine for me.\n\nMaybe using $fmt has the benefit that it copes better with some buggy\nversions of some browser (like Opera) that tend to strip the .gz from\n.tar.gz files ...\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"94607","messageId":"200811020255.01895.jnareb@gmail.com","threadId":"15930","inReplyTo":"1224426270-27755-3-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATHv2 8/8] gitweb: make the supported snapshot formats array global","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-11-02T01:54:59Z","receivedAt":"2008-11-02T01:54:59Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sun, 19 Oct 2008, Giuseppe Bilotta wrote:\n\n> The @snapshot_fmts array, containing the list of the supported snapshot\n> formats, was recreated locally in three different routines (with the\n> different name @supported_fmts in one of them).\n> \n> Its local generation is particularly expensive because two of the\n> callers, href() and format_snapshot_links(), are often called many times\n> in a single page.\n> \n> Simplify code and speed up page generation by making the array global.\n> \n> Signed-off-by: Giuseppe Bilotta \n\nVery good idea, although I'd prefer for this patch to come first;\nit is not controversial contrary to other patches in this (sub)series\nwhich IMHO needs some thought first.\n\nAcked-by: Jakub Narebski \n\n> ---\n> gitweb/gitweb.perl | 21 ++++++++-------------\n> 1 files changed, 8 insertions(+), 13 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 5fd5a1f..d1475b7 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -748,6 +748,10 @@ if (defined $searchtext) {\n> our $git_dir;\n> $git_dir = \"$projectroot/$project\" if $project;\n> \n> +# list of supported snapshot formats\n> +our @snapshot_fmts = gitweb_check_feature('snapshot');\n> +@snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);\n> +\n> # dispatch\n> if (!defined $action) {\n> \tif (defined $hash) {\n> @@ -858,11 +862,7 @@ sub href (%) {\n> \t\t\t# snapshot_format should always be defined when href()\n> \t\t\t# is called, but just in case some code forgets, we\n> \t\t\t# fall back to the default\n> -\t\t\tif (!$fmt) {\n> -\t\t\t\tmy @snapshot_fmts = gitweb_check_feature('snapshot');\n> -\t\t\t\t@snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);\n> -\t\t\t\t$fmt = $snapshot_fmts[0];\n> -\t\t\t}\n> +\t\t\t$fmt ||= $snapshot_fmts[0];\n> \t\t\t$href .= $known_snapshot_formats{$fmt}{'suffix'};\n> \t\t\tdelete $params{'snapshot_format'};\n> \t\t}\n> @@ -1695,8 +1695,6 @@ sub format_diff_line {\n> # linked. Pass the hash of the tree/commit to snapshot.\n> sub format_snapshot_links {\n> \tmy ($hash) = @_;\n> -\tmy @snapshot_fmts = gitweb_check_feature('snapshot');\n> -\t@snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);\n> \tmy $num_fmts = @snapshot_fmts;\n> \tif ($num_fmts > 1) {\n> \t\t# A parenthesized list of links bearing format names.\n> @@ -4898,20 +4896,17 @@ sub git_tree {\n> }\n> \n> sub git_snapshot {\n> -\tmy @supported_fmts = gitweb_check_feature('snapshot');\n> -\t@supported_fmts = filter_snapshot_fmts(@supported_fmts);\n> -\n> \tmy $format = $input_params{'snapshot_format'};\n> -\tif (!@supported_fmts) {\n> +\tif (!@snapshot_fmts) {\n> \t\tdie_error(403, \"Snapshots not allowed\");\n> \t}\n> \t# default to first supported snapshot format\n> -\t$format ||= $supported_fmts[0];\n> +\t$format ||= $snapshot_fmts[0];\n> \tif ($format !~ m/^[a-z0-9]+$/) {\n> \t\tdie_error(400, \"Invalid snapshot format parameter\");\n> \t} elsif (!exists($known_snapshot_formats{$format})) {\n> \t\tdie_error(400, \"Unknown snapshot format\");\n> -\t} elsif (!grep($_ eq $format, @supported_fmts)) {\n> +\t} elsif (!grep($_ eq $format, @snapshot_fmts)) {\n> \t\tdie_error(403, \"Unsupported snapshot format\");\n> \t}\n> \n> -- \n> 1.5.6.5\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"94638","messageId":"7vbpwy1pgx.fsf@gitster.siamese.dyndns.org","threadId":"15930","inReplyTo":"200811020255.01895.jnareb@gmail.com","subject":"Re: [PATHv2 8/8] gitweb: make the supported snapshot formats array global","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-02T08:50:22Z","receivedAt":"2008-11-02T08:50:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> Very good idea, although I'd prefer for this patch to come first;\n> it is not controversial contrary to other patches in this (sub)series\n> which IMHO needs some thought first.\n>\n> Acked-by: Jakub Narebski \n\nI've dismissed the three patches 6-8 (without prejudice, as I lost track\nof them); please resubmit and re-ack.\n"}]}