{"thread":{"id":"15742","subject":"[PATCHv4] gitweb: refactor input parameters parse/validation","startedAt":"2008-10-02T00:10:28Z","lastAt":"2008-10-06T00:17:34Z","messageCount":34,"participants":["Giuseppe Bilotta","Jakub Narebski","Petr Baudis"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"92107","messageId":"1222906234-8182-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"15742","inReplyTo":null,"subject":"[PATCHv4] gitweb: PATH_INFO support improvements","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-10-02T00:10:28Z","receivedAt":"2008-10-02T00:10:28Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Fourth version of 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 URLs is kept, as long as $hash is not a refname whose\nname happens to match a git action.\n\nThe main implementation is provided by paired patches (#1#3, #5#6)\nthat implement parsing and generation of the new style URLs.\n\nPatch #2 deals with a refactoring of the input parameters parsing and\nvalidation, so that the rest of gitweb can be agnostic wrt to the\nparameters' origin (CGI vs PATH_INFO vs possible other future inputs\nsuch as CLI).\n\nPatch #4 is a minor improvement to the URL syntax that allows web\ndocuments to be properly browsable in raw mode.\n\nGiuseppe Bilotta (6):\n gitweb: parse project/action/hash_base:filename PATH_INFO\n gitweb: refactor input parameters parse/validation\n gitweb: generate project/action/hash URLs\n gitweb: use_pathinfo filenames start with /\n gitweb: parse parent..current syntax from pathinfo\n gitweb: generate parent..current URLs\n\n gitweb/gitweb.perl | 392 ++++++++++++++++++++++++++++++++++------------------\n 1 files changed, 254 insertions(+), 138 deletions(-)\n"},{"id":"92109","messageId":"1222906234-8182-2-git-send-email-giuseppe.bilotta@gmail.com","threadId":"15742","inReplyTo":"1222906234-8182-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv4] gitweb: parse project/action/hash_base:filename PATH_INFO","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-10-02T00:10:29Z","receivedAt":"2008-10-02T00:10:29Z","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,\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 | 90 +++++++++++++++++++++++++++++++---------------------\n 1 files changed, 54 insertions(+), 36 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex e7e4d6b..f088681 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -495,6 +495,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@@ -519,9 +550,19 @@ 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,^$action/*,,;\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+\t\t# we got \"project.git/action/branch:filename\" or \"project.git/action/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@@ -534,8 +575,9 @@ sub evaluate_path_info {\n \t\t$file_name ||= validate_pathname($pathname);\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@@ -544,37 +586,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@@ -631,8 +642,15 @@ 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\t# the parameter we want to recycle may be either part of the\n+\t\t\t\t# list of CGI parameter, or recovered from PATH_INFO\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-- \n1.5.6.5\n"},{"id":"92105","messageId":"1222906234-8182-3-git-send-email-giuseppe.bilotta@gmail.com","threadId":"15742","inReplyTo":"1222906234-8182-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv4] gitweb: refactor input parameters parse/validation","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-10-02T00:10:30Z","receivedAt":"2008-10-02T00:10:30Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Since input parameters can now be obtained both from CGI parameters and\nPATH_INFO, we would like most of the code to be agnostic about the way\nparameters were retrieved.\n\nWe thus collect all the parameters into the new %input_params hash,\nremoving the need for ad-hoc kludgy code in href(). As much of the\nparameters validation code is now shared between CGI and PATH_INFO,\nalthough this requires PATH_INFO parsing to be interleaved into the main\ncode instead of being factored out into its own routine.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 336 ++++++++++++++++++++++++++++------------------------\n 1 files changed, 183 insertions(+), 153 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex f088681..ec4326f 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -28,8 +28,10 @@ our $my_url = $cgi->url();\n our $my_uri = $cgi->url(-absolute => 1);\n \n # if we're called with PATH_INFO, we have to strip that\n-# from the URL to find our real URL\n-if (my $path_info = $ENV{\"PATH_INFO\"}) {\n+# from the URL to find our real URL. PATH_INFO is kept\n+# as it's used later on for parameter extraction\n+my $path_info = $ENV{\"PATH_INFO\"};\n+if ($path_info) {\n \t$my_url =~ s,\\Q$path_info\\E$,,;\n \t$my_uri =~ s,\\Q$path_info\\E$,,;\n }\n@@ -390,15 +392,111 @@ $projects_list ||= $projectroot;\n \n # ======================================================================\n # input validation and dispatch\n-our $action = $cgi->param('a');\n-if (defined $action) {\n-\tif ($action =~ m/[^0-9a-zA-Z\\.\\-_]/) {\n-\t\tdie_error(400, \"Invalid action parameter\");\n+\n+# input parameters can be collected from a variety of sources (presently, CGI\n+# and PATH_INFO), so we define an %input_params hash that collects them all\n+# together during validation: this allows subsequent uses (e.g. href()) to be\n+# agnostic of the parameter origin\n+\n+my %input_params = ();\n+\n+# input parameters are stored with the long parameter name as key, so we need\n+# the name -> CGI key mapping here. This will also be used in the href\n+# subroutine to convert parameters to their CGI equivalent.\n+#\n+# XXX: Warning: If you touch this, check the search form for updating,\n+# too.\n+\n+my @cgi_param_mapping = (\n+\tproject => \"p\",\n+\taction => \"a\",\n+\tfile_name => \"f\",\n+\tfile_parent => \"fp\",\n+\thash => \"h\",\n+\thash_parent => \"hp\",\n+\thash_base => \"hb\",\n+\thash_parent_base => \"hpb\",\n+\tpage => \"pg\",\n+\torder => \"o\",\n+\tsearchtext => \"s\",\n+\tsearchtype => \"st\",\n+\tsnapshot_format => \"sf\",\n+\textra_options => \"opt\",\n+\tsearch_use_regexp => \"sr\",\n+);\n+my %cgi_param_mapping = @cgi_param_mapping;\n+\n+# we will also need to know the possible actions, for validation\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+# fill %input_params with the CGI parameters. All values except for 'opt'\n+# should be single values, but opt can be an array. We should probably\n+# build an array of parameters that can be multi-valued, but since for the time\n+# being it's only this one, we just single it out\n+while (my ($name, $symbol) = each %cgi_param_mapping) {\n+\tif ($symbol eq 'opt') {\n+\t\t$input_params{$name} = [$cgi->param($symbol)];\n+\t} else {\n+\t\t$input_params{$name} = $cgi->param($symbol);\n \t}\n }\n \n-# parameters which are pathnames\n-our $project = $cgi->param('p');\n+# next, we want to parse PATH_INFO (which was already stored in $path_info at\n+# the beginning). This is a little hairy because PATH_INFO parsing needs some\n+# form of parameter validation, so we interleave parsing and validation.\n+#\n+# The accepted PATH_INFO syntax is $project/$action/$hash or\n+# $project/$action/$hash_base:$file_name, where $action may be missing (mostly for\n+# backwards compatibility), so we need to parse and validate the parameters in\n+# this same order.\n+\n+# clear $path_info of any leading /\n+$path_info =~ s,^/+,,;\n+\n+our $project = $input_params{'project'};\n+if ($path_info && !defined $project) {\n+\t# if $project was not defined by CGI, we try to extract it from\n+\t# $path_info\n+\t$project = $path_info;\n+\t$project =~ s,/+$,,;\n+\twhile ($project && !check_head_link(\"$projectroot/$project\")) {\n+\t\t$project =~ s,/*[^/]*$,,;\n+\t}\n+\t$input_params{'project'} = $project;\n+} else {\n+\t# otherwise, we suppress $path_info parsing altogether\n+\t$path_info = undef;\n+}\n+\n+# project name validation\n if (defined $project) {\n \tif (!validate_pathname($project) ||\n \t !(-d \"$projectroot/$project\") ||\n@@ -408,16 +506,66 @@ if (defined $project) {\n \t\tundef $project;\n \t\tdie_error(404, \"No such project\");\n \t}\n+\n+\t# we purge the $project name from the $path_info, preparing it for\n+\t# subsequent parameters extraction\n+\t$path_info =~ s,^\\Q$project\\E/*,, if defined $path_info;\n+} else {\n+\t# we also suppress $path_info parsing if no project was defined\n+\t$path_info = undef;\n+}\n+\n+# action parsing/validation\n+our $action = $input_params{'action'};\n+if ($path_info && !defined $action) {\n+\t# next comes the action\n+\t$action = $path_info;\n+\t$action =~ s,/.*$,,;\n+\tif (exists $actions{$action}) {\n+\t\t# remove the action from $path_info, and sync %input_params\n+\t\t$path_info =~ s,^$action/*,,;\n+\t\t$input_params{'action'} = $action;\n+\t} else {\n+\t\t$action = undef;\n+\t}\n+} elsif (defined $action && !exists $actions{$action}) {\n+\tdie_error(400, \"Invalid action parameter\");\n+}\n+\n+# we can now parse ref and pathnames in PATH_INFO\n+if ($path_info) {\n+\tmy ($refname, $pathname) = split(/:/, $path_info, 2);\n+\tif (defined $pathname) {\n+\t\t# we got \"project.git/action/branch:filename\" or \"project.git/action/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$pathname =~ s,/$,,;\n+\t\t} else {\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$input_params{'hash_base'} ||= $refname;\n+\t}\n }\n \n-our $file_name = $cgi->param('f');\n+# and now the rest of the validation\n+\n+# parameters which are pathnames\n+our $file_name = $input_params{'file_name'};\n if (defined $file_name) {\n \tif (!validate_pathname($file_name)) {\n \t\tdie_error(400, \"Invalid file parameter\");\n \t}\n }\n \n-our $file_parent = $cgi->param('fp');\n+our $file_parent = $input_params{'file_parent'};\n if (defined $file_parent) {\n \tif (!validate_pathname($file_parent)) {\n \t\tdie_error(400, \"Invalid file parent parameter\");\n@@ -425,21 +573,21 @@ if (defined $file_parent) {\n }\n \n # parameters which are refnames\n-our $hash = $cgi->param('h');\n+our $hash = $input_params{'hash'};\n if (defined $hash) {\n \tif (!validate_refname($hash)) {\n \t\tdie_error(400, \"Invalid hash parameter\");\n \t}\n }\n \n-our $hash_parent = $cgi->param('hp');\n+our $hash_parent = $input_params{'hash_parent'};\n if (defined $hash_parent) {\n \tif (!validate_refname($hash_parent)) {\n \t\tdie_error(400, \"Invalid hash parent parameter\");\n \t}\n }\n \n-our $hash_base = $cgi->param('hb');\n+our $hash_base = $input_params{'hash_base'};\n if (defined $hash_base) {\n \tif (!validate_refname($hash_base)) {\n \t\tdie_error(400, \"Invalid hash base parameter\");\n@@ -450,19 +598,19 @@ my %allowed_options = (\n \t\"--no-merges\" => [ qw(rss atom log shortlog history) ],\n );\n \n-our @extra_options = $cgi->param('opt');\n-if (defined @extra_options) {\n-\tforeach my $opt (@extra_options) {\n-\t\tif (not exists $allowed_options{$opt}) {\n-\t\t\tdie_error(400, \"Invalid option parameter\");\n-\t\t}\n-\t\tif (not grep(/^$action$/, @{$allowed_options{$opt}})) {\n-\t\t\tdie_error(400, \"Invalid option parameter for this action\");\n-\t\t}\n+our @extra_options = @{$input_params{'extra_options'}};\n+# @extra_options is always defined, since it can only be (currently) set from\n+# CGI, and $cgi->param() returns the empty array in array context\n+foreach my $opt (@extra_options) {\n+\tif (not exists $allowed_options{$opt}) {\n+\t\tdie_error(400, \"Invalid option parameter\");\n+\t}\n+\tif (not grep(/^$action$/, @{$allowed_options{$opt}})) {\n+\t\tdie_error(400, \"Invalid option parameter for this action\");\n \t}\n }\n \n-our $hash_parent_base = $cgi->param('hpb');\n+our $hash_parent_base = $input_params{'hash_parent_base'};\n if (defined $hash_parent_base) {\n \tif (!validate_refname($hash_parent_base)) {\n \t\tdie_error(400, \"Invalid hash parent base parameter\");\n@@ -470,23 +618,23 @@ if (defined $hash_parent_base) {\n }\n \n # other parameters\n-our $page = $cgi->param('pg');\n+our $page = $input_params{'page'};\n if (defined $page) {\n \tif ($page =~ m/[^0-9]/) {\n \t\tdie_error(400, \"Invalid page parameter\");\n \t}\n }\n \n-our $searchtype = $cgi->param('st');\n+our $searchtype = $input_params{'searchtype'};\n if (defined $searchtype) {\n \tif ($searchtype =~ m/[^a-z]/) {\n \t\tdie_error(400, \"Invalid searchtype parameter\");\n \t}\n }\n \n-our $search_use_regexp = $cgi->param('sr');\n+our $search_use_regexp = $input_params{'search_use_regexp'};\n \n-our $searchtext = $cgi->param('s');\n+our $searchtext = $input_params{'searchtext'};\n our $search_regexp;\n if (defined $searchtext) {\n \tif (length($searchtext) < 2) {\n@@ -495,93 +643,6 @@ 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-\tmy $path_info = $ENV{\"PATH_INFO\"};\n-\treturn if !$path_info;\n-\t$path_info =~ s,^/+,,;\n-\treturn if !$path_info;\n-\t# find which part of PATH_INFO is project\n-\t$project = $path_info;\n-\t$project =~ s,/+$,,;\n-\twhile ($project && !check_head_link(\"$projectroot/$project\")) {\n-\t\t$project =~ s,/*[^/]*$,,;\n-\t}\n-\t# validate project\n-\t$project = validate_pathname($project);\n-\tif (!$project ||\n-\t ($export_ok && !-e \"$projectroot/$project/$export_ok\") ||\n-\t ($strict_export && !project_in_list($project))) {\n-\t\tundef $project;\n-\t\treturn;\n-\t}\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,^$action/*,,;\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/action/branch:filename\" or \"project.git/action/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$action ||= \"tree\";\n-\t\t\t$pathname =~ s,/$,,;\n-\t\t} else {\n-\t\t\t$action ||= \"blob_plain\";\n-\t\t}\n-\t\t$hash_base ||= validate_refname($refname);\n-\t\t$file_name ||= validate_pathname($pathname);\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$hash_base ||= validate_refname($refname);\n-\t}\n-}\n-evaluate_path_info();\n-\n # path to the current git repository\n our $git_dir;\n $git_dir = \"$projectroot/$project\" if $project;\n@@ -615,43 +676,12 @@ sub href (%) {\n \t# default is to use -absolute url() i.e. $my_uri\n \tmy $href = $params{-full} ? $my_url : $my_uri;\n \n-\t# XXX: Warning: If you touch this, check the search form for updating,\n-\t# too.\n-\n-\tmy @mapping = (\n-\t\tproject => \"p\",\n-\t\taction => \"a\",\n-\t\tfile_name => \"f\",\n-\t\tfile_parent => \"fp\",\n-\t\thash => \"h\",\n-\t\thash_parent => \"hp\",\n-\t\thash_base => \"hb\",\n-\t\thash_parent_base => \"hpb\",\n-\t\tpage => \"pg\",\n-\t\torder => \"o\",\n-\t\tsearchtext => \"s\",\n-\t\tsearchtype => \"st\",\n-\t\tsnapshot_format => \"sf\",\n-\t\textra_options => \"opt\",\n-\t\tsearch_use_regexp => \"sr\",\n-\t);\n-\tmy %mapping = @mapping;\n-\n \t$params{'project'} = $project unless exists $params{'project'};\n \n \tif ($params{-replay}) {\n-\t\twhile (my ($name, $symbol) = each %mapping) {\n-\t\t\tif (!exists $params{$name}) {\n-\t\t\t\t# the parameter we want to recycle may be either part of the\n-\t\t\t\t# list of CGI parameter, or recovered from PATH_INFO\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\twhile (my ($name, $val) = each %input_params) {\n+\t\t\t$params{$name} = $input_params{$name}\n+\t\t\t\tunless (exists $params{$name});\n \t\t}\n \t}\n \n@@ -669,8 +699,8 @@ sub href (%) {\n \n \t# now encode the parameters explicitly\n \tmy @result = ();\n-\tfor (my $i = 0; $i < @mapping; $i += 2) {\n-\t\tmy ($name, $symbol) = ($mapping[$i], $mapping[$i+1]);\n+\tfor (my $i = 0; $i < @cgi_param_mapping; $i += 2) {\n+\t\tmy ($name, $symbol) = ($cgi_param_mapping[$i], $cgi_param_mapping[$i+1]);\n \t\tif (defined $params{$name}) {\n \t\t\tif (ref($params{$name}) eq \"ARRAY\") {\n \t\t\t\tforeach my $par (@{$params{$name}}) {\n@@ -4006,7 +4036,7 @@ sub git_search_grep_body {\n ## actions\n \n sub git_project_list {\n-\tmy $order = $cgi->param('o');\n+\tmy $order = $input_params{'order'};\n \tif (defined $order && $order !~ m/none|project|descr|owner|age/) {\n \t\tdie_error(400, \"Unknown order parameter\");\n \t}\n@@ -4029,7 +4059,7 @@ sub git_project_list {\n }\n \n sub git_forks {\n-\tmy $order = $cgi->param('o');\n+\tmy $order = $input_params{'order'};\n \tif (defined $order && $order !~ m/none|project|descr|owner|age/) {\n \t\tdie_error(400, \"Unknown order parameter\");\n \t}\n@@ -4562,7 +4592,7 @@ sub git_snapshot {\n \tmy @supported_fmts = gitweb_check_feature('snapshot');\n \t@supported_fmts = filter_snapshot_fmts(@supported_fmts);\n \n-\tmy $format = $cgi->param('sf');\n+\tmy $format = $input_params{'snapshot_format'};\n \tif (!@supported_fmts) {\n \t\tdie_error(403, \"Snapshots not allowed\");\n \t}\n-- \n1.5.6.5\n"},{"id":"92110","messageId":"1222906234-8182-4-git-send-email-giuseppe.bilotta@gmail.com","threadId":"15742","inReplyTo":"1222906234-8182-3-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv4] gitweb: generate project/action/hash URLs","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-10-02T00:10:31Z","receivedAt":"2008-10-02T00:10:31Z","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 | 32 +++++++++++++++++++++++++++++---\n 1 files changed, 29 insertions(+), 3 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex ec4326f..2c380ac 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -687,14 +687,40 @@ 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# Strip any trailing / from $href, or we might get double\n+\t\t# slashes when the script is the DirectoryIndex\n+\t\t#\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":"92106","messageId":"1222906234-8182-5-git-send-email-giuseppe.bilotta@gmail.com","threadId":"15742","inReplyTo":"1222906234-8182-4-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv4] gitweb: use_pathinfo filenames start with /","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-10-02T00:10:32Z","receivedAt":"2008-10-02T00:10:32Z","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.\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 2c380ac..3e5b2b7 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -690,7 +690,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# Strip any trailing / from $href, or we might get double\n \t\t# slashes when the script is the DirectoryIndex\n@@ -708,11 +708,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":"92108","messageId":"1222906234-8182-6-git-send-email-giuseppe.bilotta@gmail.com","threadId":"15742","inReplyTo":"1222906234-8182-5-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv4] gitweb: parse parent..current syntax from pathinfo","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-10-02T00:10:33Z","receivedAt":"2008-10-02T00:10:33Z","isPatch":false,"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 3e5b2b7..89e360f 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -534,7 +534,9 @@ if ($path_info && !defined $action) {\n \n # we can now parse ref and pathnames in PATH_INFO\n if ($path_info) {\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/action/branch:filename\" or \"project.git/action/branch:dir/\"\n \t\t# we could use git_get_type(branch:pathname), but it needs $git_dir\n@@ -543,7 +545,11 @@ if ($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\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@@ -553,6 +559,22 @@ if ($path_info) {\n \t\t$input_params{'hash'} ||= $refname;\n \t\t$input_params{'hash_base'} ||= $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$input_params{'hash_parent_base'} ||= $parentrefname;\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\tif (defined $input_params{'file_parent'}) {\n+\t\t\t$input_params{'hash_parent'} ||= git_get_hash_by_path($input_params{'hash_parent_base'}, $input_params{'file_parent'});\n+\t\t} else {\n+\t\t\t$input_params{'hash_parent'} ||= $parentrefname;\n+\t\t}\n+\t}\n }\n \n # and now the rest of the validation\n-- \n1.5.6.5\n"},{"id":"92111","messageId":"1222906234-8182-7-git-send-email-giuseppe.bilotta@gmail.com","threadId":"15742","inReplyTo":"1222906234-8182-6-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv4] gitweb: generate parent..current URLs","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-10-02T00:10:34Z","receivedAt":"2008-10-02T00:10:34Z","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 89e360f..d863ef7 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -712,7 +712,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# Strip any trailing / from $href, or we might get double\n \t\t# slashes when the script is the DirectoryIndex\n@@ -730,17 +731,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":"92115","messageId":"200810021019.27383.jnareb@gmail.com","threadId":"15742","inReplyTo":"1222906234-8182-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv4] gitweb: PATH_INFO support improvements","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-10-02T08:19:26Z","receivedAt":"2008-10-02T08:19:26Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Giuseppe Bilotta wrote:\n\n> Fourth version of my gitweb PATH_INFO patchset, whose purpose is to\n> reduce the use of CGI parameters by embedding as many parameters as\n> possible in the URL path itself, provided the pathinfo feature is\n> enabled.\n\nA nit: when sending longer patch series you should use numbered\nformat in the form of [PATCH m/n] or [PATCH m/n vX] prefix.\n\n> \n> The 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 URLs is kept, as long as $hash is not a refname whose\n> name happens to match a git action.\n\nMinor nit: there was also old-style $project/$hash_base:$file_name\npath_info format.\n\n>\n> The main implementation is provided by paired patches (#1#3, #5#6)\n> that implement parsing and generation of the new style URLs.\n> \n> Patch #2 deals with a refactoring of the input parameters parsing and\n> validation, so that the rest of gitweb can be agnostic wrt to the\n> parameters' origin (CGI vs PATH_INFO vs possible other future inputs\n> such as CLI).\n> \n> Patch #4 is a minor improvement to the URL syntax that allows web\n> documents to be properly browsable in raw mode.\n\nVery nice summary of patchset and patch coverage in this cover letter.\n\n>\n> Giuseppe Bilotta (6):\n> gitweb: parse project/action/hash_base:filename PATH_INFO\n> gitweb: refactor input parameters parse/validation\n> gitweb: generate project/action/hash URLs\n> gitweb: use_pathinfo filenames start with /\n> gitweb: parse parent..current syntax from pathinfo\n> gitweb: generate parent..current URLs\n\n-- \nJakub Narebski\nPoland\n"},{"id":"92118","messageId":"cb7bb73a0810020149l4722be08p52be3b4703be0e41@mail.gmail.com","threadId":"15742","inReplyTo":"200810021019.27383.jnareb@gmail.com","subject":"Re: [PATCHv4] gitweb: PATH_INFO support improvements","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-10-02T08:49:59Z","receivedAt":"2008-10-02T08:49:59Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Thu, Oct 2, 2008 at 10:19 AM, Jakub Narebski wrote:\n> Giuseppe Bilotta wrote:\n>\n>> Fourth version of my gitweb PATH_INFO patchset, whose purpose is to\n>> reduce the use of CGI parameters by embedding as many parameters as\n>> possible in the URL path itself, provided the pathinfo feature is\n>> enabled.\n>\n> A nit: when sending longer patch series you should use numbered\n> format in the form of [PATCH m/n] or [PATCH m/n vX] prefix.\n\nW00t, I still manage to get this wrong. Kudos to me 8-/\n\nI wonder why these options are not the default when there is more than\none patch, btw?\n\n(And yes, I tried looking into the builtin-log.c code but making it\nautomatic is somewhat less trivial than I can dedicate time to.)\n\n>> The 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 URLs is kept, as long as $hash is not a refname whose\n>> name happens to match a git action.\n>\n> Minor nit: there was also old-style $project/$hash_base:$file_name\n> path_info format.\n\nRight, forgot about that.\n\n>> The main implementation is provided by paired patches (#1#3, #5#6)\n>> that implement parsing and generation of the new style URLs.\n>>\n>> Patch #2 deals with a refactoring of the input parameters parsing and\n>> validation, so that the rest of gitweb can be agnostic wrt to the\n>> parameters' origin (CGI vs PATH_INFO vs possible other future inputs\n>> such as CLI).\n>>\n>> Patch #4 is a minor improvement to the URL syntax that allows web\n>> documents to be properly browsable in raw mode.\n>\n> Very nice summary of patchset and patch coverage in this cover letter.\n\nThanks. At least I'm learning from my past errors. I'll manage to send\nthe perfect patchset sooner or later ;)\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"92121","messageId":"200810021059.19708.jnareb@gmail.com","threadId":"15742","inReplyTo":"1222906234-8182-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv4] gitweb: parse project/action/hash_base:filename PATH_INFO","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-10-02T08:59:19Z","receivedAt":"2008-10-02T08:59:19Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"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> except when $project/$branch was used to access a branch whose name\n> matches a gitweb action.\n\nNice summary.\n\n> Signed-off-by: Giuseppe Bilotta \n> ---\n> gitweb/gitweb.perl | 90 +++++++++++++++++++++++++++++++---------------------\n> 1 files changed, 54 insertions(+), 36 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index e7e4d6b..f088681 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -495,6 +495,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[...]\n> +);\n\nI'm not sure if the '# dispatch' comment is correct here now that\n%actions hash is moved away from actual dispatch (selecting action\nto run)\n\n> @@ -519,9 +550,19 @@ 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 here pattern matching, but your code is also good and\ndoesn't need changing; just for completeness below there is alternate\nsolution:\n\n+\t$path_info =~ m,^(.*?)/,;\n+\t$action = $1;\n\n> +\tif (exists $actions{$action}) {\n> +\t\t$path_info =~ s,^$action/*,,;\n> +\t} else {\n> +\t\t$action = undef;\n> +\t}\n> +\n\n[...]\n> @@ -534,8 +575,9 @@ sub evaluate_path_info {\n> \t\t$file_name ||= validate_pathname($pathname);\n> \t} elsif (defined $refname) {\n> \t\t# we got \"project.git/branch\"\n\nYou meant here\n\n \t\t# we got \"project.git/branch\" or \"project.git/action/branch\"\n\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\nThis hunk is IMHO incorrect. First, $refname is _either_ $hash, or\n$hash_base; it cannot be both. Second, in most cases (like the case\nof 'shortlog' action, either explicit or implicit) it is simply $hash;\nI think it can be $hash_base when $file_name is not set only in\nsingular exception case of 'tree' view for the top tree (lack of\nfilename is not an error, but is equivalent to $file_name='/').\n\n> @@ -544,37 +586,6 @@ evaluate_path_info();\n> our $git_dir;\n> $git_dir = \"$projectroot/$project\" if $project;\n> \n> -# dispatch\n> -my %actions = (\n[...]\n> -);\n> -\n> if (!defined $action) {\n> \tif (defined $hash) {\n> \t\t$action = git_get_type($hash);\n\nI _think_ the '# dispatch' comment should be left here, and not moved\nwith the %actions hash.\n\n> @@ -631,8 +642,15 @@ 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\t# the parameter we want to recycle may be either part of the\n> +\t\t\t\t# list of CGI parameter, or recovered from PATH_INFO\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\nI would _perhaps_ add here comment that multivalued parameters can come\nonly from CGI query string, so there is no need for something like:\n\n+\t\t\t\t\t$params{$name} = (ref($$name) ? @$name : $$name) if $$name;\n\n> +\t\t\t\t}\n> \t\t\t}\n> \t\t}\n> \t}\n\nThis fragment is a bit of ugly code, hopefully corrected in later patch.\nI think it would be better to have 'refactor parsing/validation of input\nparameters' to be very fist patch in series; I am not sure but I suspect\nthat is a kind of bugfix for current \"$project/$hash\" ('shortlog' view)\nand \"$project/$hash_base:$file_name\" ('blob_plain' and 'tree' view)\npath_info.\n\nP.S. It is a bit of pity that Mechanize test from Lea Wiemann caching\ngitweb code is not in the 'master' or at least 'pu'. Using big, single,\nmonolithic patch instead of patch series of small, easy reviewable\ncommits strikes again... ;-(\n\n-- \nJakub Narebski\nPoland\n"},{"id":"92125","messageId":"cb7bb73a0810020243v37759f79xfde4c32c49e1a375@mail.gmail.com","threadId":"15742","inReplyTo":"200810021059.19708.jnareb@gmail.com","subject":"Re: [PATCHv4] gitweb: parse project/action/hash_base:filename PATH_INFO","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-10-02T09:43:04Z","receivedAt":"2008-10-02T09:43:04Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Thu, Oct 2, 2008 at 10:59 AM, Jakub Narebski wrote:\n> Giuseppe Bilotta wrote:\n>>\n>> +# dispatch\n>> +my %actions = (\n>> + \"blame\" => \\&git_blame,\n> [...]\n>> +);\n>\n> I'm not sure if the '# dispatch' comment is correct here now that\n> %actions hash is moved away from actual dispatch (selecting action\n> to run)\n\nBingo.\n\n>> @@ -519,9 +550,19 @@ 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 here pattern matching, but your code is also good and\n> doesn't need changing; just for completeness below there is alternate\n> solution:\n>\n> + $path_info =~ m,^(.*?)/,;\n> + $action = $1;\n\n\nYeah, I just followed the existing code style.\n\n\n>> @@ -534,8 +575,9 @@ sub evaluate_path_info {\n>> $file_name ||= validate_pathname($pathname);\n>> } elsif (defined $refname) {\n>> # we got \"project.git/branch\"\n>\n> You meant here\n>\n> # we got \"project.git/branch\" or \"project.git/action/branch\"\n\nYes I do.\n\n>> - $action ||= \"shortlog\";\n>> - $hash ||= validate_refname($refname);\n>> + $action ||= \"shortlog\";\n>> + $hash ||= validate_refname($refname);\n>> + $hash_base ||= validate_refname($refname);\n>> }\n>> }\n>\n> This hunk is IMHO incorrect. First, $refname is _either_ $hash, or\n> $hash_base; it cannot be both. Second, in most cases (like the case\n> of 'shortlog' action, either explicit or implicit) it is simply $hash;\n> I think it can be $hash_base when $file_name is not set only in\n> singular exception case of 'tree' view for the top tree (lack of\n> filename is not an error, but is equivalent to $file_name='/').\n\nOTOH, while setting both $hash and $hash_base has worked fine for me\nso far (because the right one is automatically used and apparently\nsetting the other doesn't hurt), choosing which one to set is a much\nhairier case. Do you have suggestions for a better way to always make\nit work?\n\n>> @@ -544,37 +586,6 @@ evaluate_path_info();\n>> our $git_dir;\n>> $git_dir = \"$projectroot/$project\" if $project;\n>>\n>> -# dispatch\n>> -my %actions = (\n> [...]\n>> -);\n>> -\n>> if (!defined $action) {\n>> if (defined $hash) {\n>> $action = git_get_type($hash);\n>\n> I _think_ the '# dispatch' comment should be left here, and not moved\n> with the %actions hash.\n\nI agree.\n\n>> @@ -631,8 +642,15 @@ 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>> + # the parameter we want to recycle may be either part of the\n>> + # list of CGI parameter, or recovered from PATH_INFO\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> I would _perhaps_ add here comment that multivalued parameters can come\n> only from CGI query string, so there is no need for something like:\n>\n> + $params{$name} = (ref($$name) ? @$name : $$name) if $$name;\n>\n>> + }\n>> }\n>> }\n>> }\n>\n> This fragment is a bit of ugly code, hopefully corrected in later patch.\n> I think it would be better to have 'refactor parsing/validation of input\n> parameters' to be very fist patch in series; I am not sure but I suspect\n> that is a kind of bugfix for current \"$project/$hash\" ('shortlog' view)\n> and \"$project/$hash_base:$file_name\" ('blob_plain' and 'tree' view)\n> path_info.\n\nBut implementing the path_info parsing first makes the input param\nrefactoring SO much nicer that I would rather put a comment here\nsaying \"this code sucks: we should rather collect all input\nparameters\" and then clean it up on the subsequent patch.\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"92133","messageId":"200810021216.10544.jnareb@gmail.com","threadId":"15742","inReplyTo":"cb7bb73a0810020149l4722be08p52be3b4703be0e41@mail.gmail.com","subject":"Re: [PATCHv4] gitweb: PATH_INFO support improvements","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-10-02T10:16:10Z","receivedAt":"2008-10-02T10:16:10Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Giuseppe Bilotta wrote:\n> On Thu, Oct 2, 2008 at 10:19 AM, Jakub Narebski wrote:\n\n> >\n> > A nit: when sending longer patch series you should use numbered\n> > format in the form of [PATCH m/n] or [PATCH m/n vX] prefix.\n> \n> W00t, I still manage to get this wrong. Kudos to me 8-/\n> \n> I wonder why these options are not the default when there is more than\n> one patch, btw?\n> \n> (And yes, I tried looking into the builtin-log.c code but making it\n> automatic is somewhat less trivial than I can dedicate time to.)\n\nHmmm... I thought that format.numbered config variable is 'auto' by\ndefault; I guess it isn't (just so you know where to look to change\nit ;-)).\n\nI have the following in the .git/config / ~/.gitconfig:\n\n [format]\n numbered = auto\n suffix = .txt\n\n-- \nJakub Narebski\nPoland\n"},{"id":"92182","messageId":"20081002153403.GQ10360@machine.or.cz","threadId":"15742","inReplyTo":"1222906234-8182-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv4] gitweb: parse project/action/hash_base:filename PATH_INFO","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-10-02T15:34:03Z","receivedAt":"2008-10-02T15:34:03Z","isPatch":false,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"On Thu, Oct 02, 2008 at 02:10:29AM +0200, Giuseppe Bilotta wrote:\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> except when $project/$branch was used to access a branch whose name\n> matches a gitweb action.\n> \n> Signed-off-by: Giuseppe Bilotta \n\nJust nits...\n\n> ---\n> gitweb/gitweb.perl | 90 +++++++++++++++++++++++++++++++---------------------\n> 1 files changed, 54 insertions(+), 36 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index e7e4d6b..f088681 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -519,9 +550,19 @@ 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,^$action/*,,;\n> +\t} else {\n> +\t\t$action = undef;\n\nExtra whitespace.\n\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 \"project.git/action/branch:filename\" or \"project.git/action/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\nBut the old variant is still possible, maybe the comments should\nindicate that the action/ part is optional.\n\n> @@ -534,8 +575,9 @@ sub evaluate_path_info {\n> \t\t$file_name ||= validate_pathname($pathname);\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\nWhat is this good for?\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nPeople who take cold baths never have rheumatism, but they have\ncold baths.\n"},{"id":"92190","messageId":"cb7bb73a0810021230u2ec512c0l577b3146cffccb3e@mail.gmail.com","threadId":"15742","inReplyTo":"20081002153403.GQ10360@machine.or.cz","subject":"Re: [PATCHv4] gitweb: parse project/action/hash_base:filename PATH_INFO","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-10-02T19:30:42Z","receivedAt":"2008-10-02T19:30:42Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Thu, Oct 2, 2008 at 5:34 PM, Petr Baudis wrote:\n\n> Just nits...\n>> + $action = undef;\n>\n> Extra whitespace.\n\nRight, fixed.\n\n>> + }\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>> + # we got \"project.git/action/branch:filename\" or \"project.git/action/branch:dir/\"\n>> # we could use git_get_type(branch:pathname), but it needs $git_dir\n>> $pathname =~ s,^/+,,;\n>> if (!$pathname || substr($pathname, -1) eq \"/\") {\n>\n> But the old variant is still possible, maybe the comments should\n> indicate that the action/ part is optional.\n\nI put the action/ part in square brackets. (e.g.\nproject.git/[action/]branch:filename). Is this good enough?\n\n>> @@ -534,8 +575,9 @@ sub evaluate_path_info {\n>> $file_name ||= validate_pathname($pathname);\n>> } elsif (defined $refname) {\n>> # we got \"project.git/branch\"\n>> - $action ||= \"shortlog\";\n>> - $hash ||= validate_refname($refname);\n>> + $action ||= \"shortlog\";\n>> + $hash ||= validate_refname($refname);\n>> + $hash_base ||= validate_refname($refname);\n>> }\n>> }\n>> evaluate_path_info();\n>\n> What is this good for?\n\nThe purpose of what? setting both $hash and $hash_base was something\nthat I found was needed in some extreme cases, as discussed with\nJakub. Proposals for recommended cleaner but equally fast way to\nhandle it. If you're referring to the whitespace, I was just lining up\nthe entries. Should I do it in a separate patch?\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"92197","messageId":"20081002205603.GW10360@machine.or.cz","threadId":"15742","inReplyTo":"cb7bb73a0810021230u2ec512c0l577b3146cffccb3e@mail.gmail.com","subject":"Re: [PATCHv4] gitweb: parse project/action/hash_base:filename PATH_INFO","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-10-02T20:56:03Z","receivedAt":"2008-10-02T20:56:03Z","isPatch":false,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"On Thu, Oct 02, 2008 at 09:30:42PM +0200, Giuseppe Bilotta wrote:\n> On Thu, Oct 2, 2008 at 5:34 PM, Petr Baudis wrote:\n> \n> > Just nits...\n> >> + }\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> >> + # we got \"project.git/action/branch:filename\" or \"project.git/action/branch:dir/\"\n> >> # we could use git_get_type(branch:pathname), but it needs $git_dir\n> >> $pathname =~ s,^/+,,;\n> >> if (!$pathname || substr($pathname, -1) eq \"/\") {\n> >\n> > But the old variant is still possible, maybe the comments should\n> > indicate that the action/ part is optional.\n> \n> I put the action/ part in square brackets. (e.g.\n> project.git/[action/]branch:filename). Is this good enough?\n\nThat's perfect.\n\n> >> @@ -534,8 +575,9 @@ sub evaluate_path_info {\n> >> $file_name ||= validate_pathname($pathname);\n> >> } elsif (defined $refname) {\n> >> # we got \"project.git/branch\"\n> >> - $action ||= \"shortlog\";\n> >> - $hash ||= validate_refname($refname);\n> >> + $action ||= \"shortlog\";\n> >> + $hash ||= validate_refname($refname);\n> >> + $hash_base ||= validate_refname($refname);\n> >> }\n> >> }\n> >> evaluate_path_info();\n> >\n> > What is this good for?\n> \n> The purpose of what? setting both $hash and $hash_base was something\n> that I found was needed in some extreme cases, as discussed with\n> Jakub. Proposals for recommended cleaner but equally fast way to\n> handle it. If you're referring to the whitespace, I was just lining up\n> the entries. Should I do it in a separate patch?\n\nI refer to the setting of $hash_base (I'm not huge fan of the whitespace\naligning, but I don't really care). What extreme cases are these?\nI think you should describe that in the code since it's not really\nobvious. Maybe I could find it in older threads but I should understand\nthe code just from reading it. :-)\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nPeople who take cold baths never have rheumatism, but they have\ncold baths.\n"},{"id":"92199","messageId":"cb7bb73a0810021405j68b0a164i9469e64afc543ebf@mail.gmail.com","threadId":"15742","inReplyTo":"20081002205603.GW10360@machine.or.cz","subject":"Re: [PATCHv4] gitweb: parse project/action/hash_base:filename PATH_INFO","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-10-02T21:05:18Z","receivedAt":"2008-10-02T21:05:18Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":">> >> @@ -534,8 +575,9 @@ sub evaluate_path_info {\n>> >> $file_name ||= validate_pathname($pathname);\n>> >> } elsif (defined $refname) {\n>> >> # we got \"project.git/branch\"\n>> >> - $action ||= \"shortlog\";\n>> >> - $hash ||= validate_refname($refname);\n>> >> + $action ||= \"shortlog\";\n>> >> + $hash ||= validate_refname($refname);\n>> >> + $hash_base ||= validate_refname($refname);\n>> >> }\n>> >> }\n>> >> evaluate_path_info();\n>> >\n>> > What is this good for?\n>>\n>> The purpose of what? setting both $hash and $hash_base was something\n>> that I found was needed in some extreme cases, as discussed with\n>> Jakub. Proposals for recommended cleaner but equally fast way to\n>> handle it. If you're referring to the whitespace, I was just lining up\n>> the entries. Should I do it in a separate patch?\n>\n> I refer to the setting of $hash_base (I'm not huge fan of the whitespace\n> aligning, but I don't really care). What extreme cases are these?\n> I think you should describe that in the code since it's not really\n> obvious. Maybe I could find it in older threads but I should understand\n> the code just from reading it. :-)\n\nIn preparing the new patchset, I've put a big comment block explaining\nwhy we need to set both $hash and $hash_base in this code-path:\n\n# we got \"project.git/[action/]branch\". in this case\n# we set both $hash and $hash_base because different actions\n# need one or the other to be set to behave correctly.\n#\n# For example, if $hash_base is not set then the blob and\n# history links on the page project.git/tree/somebranch will\n# assume a $hash_base of HEAD instead of the correct\n# somebranch.\n# Conversely, not setting $hash will make URLs such as\n# project.git/shortlog/somebranch display the wrong page.\n#\n# Since it also turns out that the unused one is properly\n# overwritten as needed, setting both is quite safe.\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"92202","messageId":"20081002220439.GX10360@machine.or.cz","threadId":"15742","inReplyTo":"cb7bb73a0810021405j68b0a164i9469e64afc543ebf@mail.gmail.com","subject":"Re: [PATCHv4] gitweb: parse project/action/hash_base:filename PATH_INFO","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-10-02T22:04:39Z","receivedAt":"2008-10-02T22:04:39Z","isPatch":false,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"On Thu, Oct 02, 2008 at 11:05:18PM +0200, Giuseppe Bilotta wrote:\n> In preparing the new patchset, I've put a big comment block explaining\n> why we need to set both $hash and $hash_base in this code-path:\n> \n> # we got \"project.git/[action/]branch\". in this case\n> # we set both $hash and $hash_base because different actions\n> # need one or the other to be set to behave correctly.\n> #\n> # For example, if $hash_base is not set then the blob and\n> # history links on the page project.git/tree/somebranch will\n> # assume a $hash_base of HEAD instead of the correct\n> # somebranch.\n> # Conversely, not setting $hash will make URLs such as\n> # project.git/shortlog/somebranch display the wrong page.\n> #\n> # Since it also turns out that the unused one is properly\n> # overwritten as needed, setting both is quite safe.\n\n Ok, but is this related to the pathinfo changes? Or is this fixing a\nseparate bug? (Not that I would want to bother you with splitting this,\nbut you should at least mention it in the commit message... and it's\nfairly isolated anyway.)\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nPeople who take cold baths never have rheumatism, but they have\ncold baths.\n"},{"id":"92203","messageId":"200810030041.54563.jnareb@gmail.com","threadId":"15742","inReplyTo":"20081002220439.GX10360@machine.or.cz","subject":"Re: [PATCHv4] gitweb: parse project/action/hash_base:filename PATH_INFO","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-10-02T22:41:52Z","receivedAt":"2008-10-02T22:41:52Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Petr Baudis wrote:\n> On Thu, Oct 02, 2008 at 11:05:18PM +0200, Giuseppe Bilotta wrote:\n> >\n> > In preparing the new patchset, I've put a big comment block explaining\n> > why we need to set both $hash and $hash_base in this code-path:\n> > \n> > # we got \"project.git/[action/]branch\". in this case\n> > # we set both $hash and $hash_base because different actions\n> > # need one or the other to be set to behave correctly.\n> > #\n> > # For example, if $hash_base is not set then the blob and\n\n'blob' without $file_name doesn't have sense, but 'tree' does.\nProbably should be s/blob/tree/ above.\n\n> > # history links on the page project.git/tree/somebranch will\n> > # assume a $hash_base of HEAD instead of the correct\n> > # somebranch.\n> > # Conversely, not setting $hash will make URLs such as\n> > # project.git/shortlog/somebranch display the wrong page.\n> > #\n> > # Since it also turns out that the unused one is properly\n> > # overwritten as needed, setting both is quite safe.\n> \n> Ok, but is this related to the pathinfo changes? Or is this fixing a\n> separate bug? (Not that I would want to bother you with splitting this,\n> but you should at least mention it in the commit message... and it's\n> fairly isolated anyway.)\n\nYes, it related to path_info changes. With current code the only way\nto get to no $file_name branch of evaluate_path_info() code was to have\nURL which looks like project/branch, for which 'shortlog' action was\nchosen (if not specified by CGI query parameters), and for 'shortlog'\naction the correct way was to use $hash. Now there can be\nproject/tree/branch to show top directory of given branch, and this\nas described required $hash_base set.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"92212","messageId":"200810030248.57144.jnareb@gmail.com","threadId":"15742","inReplyTo":"cb7bb73a0810020243v37759f79xfde4c32c49e1a375@mail.gmail.com","subject":"Re: [PATCHv4] gitweb: parse project/action/hash_base:filename PATH_INFO","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-10-03T00:48:56Z","receivedAt":"2008-10-03T00:48:56Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 2 Oct 2008, Giuseppe Bilotta wrote:\n> On Thu, Oct 2, 2008 at 10:59 AM, Jakub Narebski wrote:\n>> Giuseppe Bilotta wrote:\n\n>>> - $action ||= \"shortlog\";\n>>> - $hash ||= validate_refname($refname);\n>>> + $action ||= \"shortlog\";\n>>> + $hash ||= validate_refname($refname);\n>>> + $hash_base ||= validate_refname($refname);\n>>> }\n>>> }\n>>\n>> This hunk is IMHO incorrect. First, $refname is _either_ $hash, or\n>> $hash_base; it cannot be both. Second, in most cases (like the case\n>> of 'shortlog' action, either explicit or implicit) it is simply $hash;\n>> I think it can be $hash_base when $file_name is not set only in\n>> singular exception case of 'tree' view for the top tree (lack of\n>> filename is not an error, but is equivalent to $file_name='/').\n> \n> OTOH, while setting both $hash and $hash_base has worked fine for me\n> so far (because the right one is automatically used and apparently\n> setting the other doesn't hurt), choosing which one to set is a much\n> hairier case. Do you have suggestions for a better way to always make\n> it work?\n\nWell, it is either checking $action and setting either $hash or\n$hash_base, or setting both, with some comments on why and when it is\nneeded (as discussed on #git). IIUC $hash_base is needed only for\nfilename-taking tree actions which acts on top-tree, and therefore\ndon't need $file_name, like 'project/tree/branch' or related \n'project/history/branch' (the latter is practically almost equivalent\nto 'project/shortlog/branch' or 'project/branch').\n\nI'm not sure if it wouldn't be better to call validate_refname($refname)\nonce, either as:\n\n $hash_base ||= $hash ||= validate_refname($refname);\n \nbut that might be incorrect in the obscure case of setting $hash via 'h'\nCGI query parameter, and letting gitweb to set-up $hash_base via\npath_info, so perhaps ($refname is local to evaluate_path_info, IIRC)\n\n $refname = validate_refname($refname);\n $hash ||= $refname;\n $hash_base ||= $refname;\n\nBut that is just nitpicking this fragment of code to death. In short:\neither check which of $hash and $hash_base to set in this branch of\nconditional, or explain why setting both $hash and $hash_base is needed,\nand why it is acceptable, either as comments, or in commit message.\n\n>>> @@ -631,8 +642,15 @@ 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>>> + # the parameter we want to recycle may be either part of the\n>>> + # list of CGI parameter, or recovered from PATH_INFO\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>> I would _perhaps_ add here comment that multivalued parameters can come\n>> only from CGI query string, so there is no need for something like:\n>>\n>> + $params{$name} = (ref($$name) ? @$name : $$name) if $$name;\n>>\n>>> + }\n>>> }\n>>> }\n>>> }\n>>\n>> This fragment is a bit of ugly code, hopefully corrected in later patch.\n>> I think it would be better to have 'refactor parsing/validation of input\n>> parameters' to be very fist patch in series; I am not sure but I suspect\n>> that is a kind of bugfix for current \"$project/$hash\" ('shortlog' view)\n>> and \"$project/$hash_base:$file_name\" ('blob_plain' and 'tree' view)\n>> path_info.\n> \n> But implementing the path_info parsing first makes the input param\n> refactoring SO much nicer that I would rather put a comment here\n> saying \"this code sucks: we should rather collect all input\n> parameters\" and then clean it up on the subsequent patch.\n\nWhy not cleanup first?\n\nWhen implementing href(..., -replay=>1) I have forgot that some of\ngitweb parameters are implicitly passed ($project, because it is needed\nin most gitweb links), and some can be passed via path_info ($hash \nand/or $hash_base, $file_name). Your code adds $action to the mix, but\nit doesn't change the fact that 1.) even before your code -replay case\nwas incorrect for some path_info links (handcrafted, as gitweb generates\nonly $project via path_info); 2.) code you have added is a bit ugly.\n\nBesides using variables change a little meaning of -replay, namely\nin your code gitweb always sets action, even for non-path_name links\nwhen we started from \"default action\" (i.e. without action set) links.\nI guess this is mainly theoretical issue, as I don't think that default\nviews use many -replay links.\n\n\nP.S. with the idea of pushing parameters obtained not from CGI query\nstring to $cgi->param() via \"$cgi->param($name, $value);\" or in named\nparams form \"$cgi->(-name=>$name, -value=>$value);\" you would not need\nto change (a bit hacky, admittedly) href(...,-replay=>1) code.\n-- \nJakub Narebski\nPoland\n"},{"id":"92211","messageId":"200810030336.31469.jnareb@gmail.com","threadId":"15742","inReplyTo":"1222906234-8182-3-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv4] gitweb: refactor input parameters parse/validation","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-10-03T01:36:30Z","receivedAt":"2008-10-03T01:36:30Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 2 Oct 2008, Giuseppe Bilotta wrote:\n\n> Since input parameters can now be obtained both from CGI parameters and\n> PATH_INFO, we would like most of the code to be agnostic about the way\n> parameters were retrieved.\n> \n\nI'd prefer that this cleanup/refactoring patch was _first_ patch in\nthe series, as we were able to obtain parameters both from CGI query\nparameters and from PATH_INFO ($project, $hash or $hash_base+$file_name)\n_before_ first patch in this series. So it correct not only issue\nintroduced by first patch (and fixed somewhat there), but what was\noutstanding (but rare because gitweb did not generate such links)\nissue.\n\n\n> We thus collect all the parameters into the new %input_params hash,\n> removing the need for ad-hoc kludgy code in href(). \n\nAlternate solution would be push data from PATH_INFO in query params\ndata using (for example)\n\n $cgi->param('a', $action);\n\nor, naming parameters explicitely\n\n $cgi->param(-name=>'a', -value=>$action);\n\nThis avoids need for additional variable, reuses current code, and\nnicely sidesteps issue whether to use long names for keys ('action',\n'file_name') or short ones from CGI query ('a', 'f').\n\n\nIt would probably has to be at least partially written to check which\nof those two solutions (%input_params or $cgi->param('a', $a)))\nis better.\n\n> As much of the \n> parameters validation code is now shared between CGI and PATH_INFO,\n> although this requires PATH_INFO parsing to be interleaved into the main\n> code instead of being factored out into its own routine.\n\nI'm not sure if it is worth it then to unify parameters validation,\nwith such drawback.\n\n> Signed-off-by: Giuseppe Bilotta \n> ---\n> gitweb/gitweb.perl | 336 ++++++++++++++++++++++++++++------------------------\n> 1 files changed, 183 insertions(+), 153 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index f088681..ec4326f 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -28,8 +28,10 @@ our $my_url = $cgi->url();\n> our $my_uri = $cgi->url(-absolute => 1);\n> \n> # if we're called with PATH_INFO, we have to strip that\n> -# from the URL to find our real URL\n> -if (my $path_info = $ENV{\"PATH_INFO\"}) {\n> +# from the URL to find our real URL. PATH_INFO is kept\n> +# as it's used later on for parameter extraction\n> +my $path_info = $ENV{\"PATH_INFO\"};\n> +if ($path_info) {\n> \t$my_url =~ s,\\Q$path_info\\E$,,;\n> \t$my_uri =~ s,\\Q$path_info\\E$,,;\n> }\n\nThis might be separate patch, if you wanted to increase your commit\ncount ;-) More seriously I think it should be at least briefly\nmentioned in commit message (make $path_info global).\n\n> @@ -390,15 +392,111 @@ $projects_list ||= $projectroot;\n> \n> # ======================================================================\n> # input validation and dispatch\n> -our $action = $cgi->param('a');\n> -if (defined $action) {\n> -\tif ($action =~ m/[^0-9a-zA-Z\\.\\-_]/) {\n> -\t\tdie_error(400, \"Invalid action parameter\");\n> +\n> +# input parameters can be collected from a variety of sources (presently, CGI\n> +# and PATH_INFO), so we define an %input_params hash that collects them all\n> +# together during validation: this allows subsequent uses (e.g. href()) to be\n> +# agnostic of the parameter origin\n> +\n> +my %input_params = ();\n> +\n> +# input parameters are stored with the long parameter name as key, so we need\n> +# the name -> CGI key mapping here. This will also be used in the href\n> +# subroutine to convert parameters to their CGI equivalent.\n> +#\n> +# XXX: Warning: If you touch this, check the search form for updating,\n> +# too.\n> +\n> +my @cgi_param_mapping = (\n> +\tproject => \"p\",\n> +\taction => \"a\",\n> +\tfile_name => \"f\",\n> +\tfile_parent => \"fp\",\n> +\thash => \"h\",\n> +\thash_parent => \"hp\",\n> +\thash_base => \"hb\",\n> +\thash_parent_base => \"hpb\",\n> +\tpage => \"pg\",\n> +\torder => \"o\",\n> +\tsearchtext => \"s\",\n> +\tsearchtype => \"st\",\n> +\tsnapshot_format => \"sf\",\n> +\textra_options => \"opt\",\n> +\tsearch_use_regexp => \"sr\",\n> +);\n> +my %cgi_param_mapping = @cgi_param_mapping;\n> +\n> +# we will also need to know the possible actions, for validation\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\nNice, although I'm not sure if [%@]cgi_param_mapping has to global.\nIf we use long parameters names as keys, I think it has to, somewhat.\nSee also comment below.\n\n> +\n> +# fill %input_params with the CGI parameters. All values except for 'opt'\n> +# should be single values, but opt can be an array. We should probably\n> +# build an array of parameters that can be multi-valued, but since for the time\n> +# being it's only this one, we just single it out\n> +while (my ($name, $symbol) = each %cgi_param_mapping) {\n> +\tif ($symbol eq 'opt') {\n> +\t\t$input_params{$name} = [$cgi->param($symbol)];\n> +\t} else {\n> +\t\t$input_params{$name} = $cgi->param($symbol);\n> \t}\n> }\n\nIf it was chosen to use short (CGI query) parameter names, the above\nloop could be replaced by simple\n\n %input_params = $cgi->Vars;\n\nor to be more exact, if we want to have multi-valued parameters stored\nas array ref\n\n %input_params = map { [ split /\\0/ ] if /\\0/; } $cgi->Vars;\n\n\nSee CGI(3pm):\n\n When using this [Vars], the thing you must watch out for are multivalued CGI\n parameters. Because a hash cannot distinguish between scalar and list con-\n text, multivalued parameters will be returned as a packed string, separated\n by the \"\\0\" (null) character. You must split this packed string in order\n to get at the individual values.\n \n> -# parameters which are pathnames\n> -our $project = $cgi->param('p');\n> +# next, we want to parse PATH_INFO (which was already stored in $path_info at\n> +# the beginning). This is a little hairy because PATH_INFO parsing needs some\n> +# form of parameter validation, so we interleave parsing and validation.\n\nI don't think it is a good idea. In my opinion, for my taste, it would\nbe better to separate evaluating path_info from the rest.\n\nWe could instead introduce convention that if variable (like $project)\nis set, then it is assumed to be validated; if it is present only in\nthe %input_params hash, then it has to be validated.\n\n\nOn the other hand this ordering, first by parameter, then by method\nof extraction could be seem quite equally valid. Nevertheless I think\nthat previous flow with separate evaluate_path_info() and what should\nbe evaluate_CGI_query() has better encapsulation.\n\n> +#\n> +# The accepted PATH_INFO syntax is $project/$action/$hash or\n> +# $project/$action/$hash_base:$file_name, where $action may be missing (mostly for\n> +# backwards compatibility), so we need to parse and validate the parameters in\n> +# this same order.\n> +\n> +# clear $path_info of any leading /\n> +$path_info =~ s,^/+,,;\n> +\n> +our $project = $input_params{'project'};\n> +if ($path_info && !defined $project) {\n> +\t# if $project was not defined by CGI, we try to extract it from\n> +\t# $path_info\n> +\t$project = $path_info;\n> +\t$project =~ s,/+$,,;\n> +\twhile ($project && !check_head_link(\"$projectroot/$project\")) {\n> +\t\t$project =~ s,/*[^/]*$,,;\n> +\t}\n> +\t$input_params{'project'} = $project;\n> +} else {\n> +\t# otherwise, we suppress $path_info parsing altogether\n> +\t$path_info = undef;\n> +}\n> +\n> +# project name validation\n> if (defined $project) {\n> \tif (!validate_pathname($project) ||\n> \t !(-d \"$projectroot/$project\") ||\n\nNote that this code does less checking if $project is in path_info than\nfor the case where it is set by CGI query. Perhaps there should be base\nfast check in a loop, and more extensive test later.\n\n> @@ -408,16 +506,66 @@ if (defined $project) {\n> \t\tundef $project;\n> \t\tdie_error(404, \"No such project\");\n> \t}\n> +\n> +\t# we purge the $project name from the $path_info, preparing it for\n> +\t# subsequent parameters extraction\n> +\t$path_info =~ s,^\\Q$project\\E/*,, if defined $path_info;\n> +} else {\n> +\t# we also suppress $path_info parsing if no project was defined\n> +\t$path_info = undef;\n> +}\n\nIn evaluate_path_info it was simply 'return if...'; here with mentioned\ninterleaving it is harder and a bit hacky.\n\n[cut]\n\n> @@ -615,43 +676,12 @@ sub href (%) {\n> \t# default is to use -absolute url() i.e. $my_uri\n> \tmy $href = $params{-full} ? $my_url : $my_uri;\n\n[cut removed, or rather moved to beginning, @mapping]\n \n> \t$params{'project'} = $project unless exists $params{'project'};\n> \n> \tif ($params{-replay}) {\n> -\t\twhile (my ($name, $symbol) = each %mapping) {\n> -\t\t\tif (!exists $params{$name}) {\n> -\t\t\t\t# the parameter we want to recycle may be either part of the\n> -\t\t\t\t# list of CGI parameter, or recovered from PATH_INFO\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\twhile (my ($name, $val) = each %input_params) {\n> +\t\t\t$params{$name} = $input_params{$name}\n> +\t\t\t\tunless (exists $params{$name});\n\nVery nice, short code. Should be something like that from\nthe very beginning.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"92213","messageId":"200810030348.13575.jnareb@gmail.com","threadId":"15742","inReplyTo":"1222906234-8182-4-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv4] gitweb: generate project/action/hash URLs","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-10-03T01:48:13Z","receivedAt":"2008-10-03T01:48:13Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Tue, 2 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_Perhaps_ it should be noted that even though gitweb accepted\n'project/hash' and 'project/hash_base:file_name' path_info URLs, it\ngenerated links with only 'project/' in path_info.\n\n> \n> Signed-off-by: Giuseppe Bilotta \n> ---\n> gitweb/gitweb.perl | 32 +++++++++++++++++++++++++++++---\n> 1 files changed, 29 insertions(+), 3 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index ec4326f..2c380ac 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -687,14 +687,40 @@ 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# Strip any trailing / from $href, or we might get double\n> +\t\t# slashes when the script is the DirectoryIndex\n\nPerhaps example, like $href='gitweb.example.com/', could be put here.\n\n> +\t\t#\n\nI think that we can lose this empty line comment here.\n\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\nGood.\n\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\nHmmmm...\n\nShouldn't the code first check for $file_name, then add either \n\"$hash_base:$file_name\" (url-escaped), or \"$hash\" (not \"$hash_base\")?\n\n> \t}\n> \n> \t# now encode the parameters explicitly\n\nThank you very much for work on improving gitweb.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"92220","messageId":"cb7bb73a0810022254v7dd3af5cwc315a2b4a67f10ba@mail.gmail.com","threadId":"15742","inReplyTo":"200810030041.54563.jnareb@gmail.com","subject":"Re: [PATCHv4] gitweb: parse project/action/hash_base:filename PATH_INFO","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-10-03T05:54:58Z","receivedAt":"2008-10-03T05:54:58Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Fri, Oct 3, 2008 at 12:41 AM, Jakub Narebski wrote:\n> Petr Baudis wrote:\n>> On Thu, Oct 02, 2008 at 11:05:18PM +0200, Giuseppe Bilotta wrote:\n>> >\n>> > In preparing the new patchset, I've put a big comment block explaining\n>> > why we need to set both $hash and $hash_base in this code-path:\n>> >\n>> > # we got \"project.git/[action/]branch\". in this case\n>> > # we set both $hash and $hash_base because different actions\n>> > # need one or the other to be set to behave correctly.\n>> > #\n>> > # For example, if $hash_base is not set then the blob and\n>\n> 'blob' without $file_name doesn't have sense, but 'tree' does.\n> Probably should be s/blob/tree/ above.\n>\n>> > # history links on the page project.git/tree/somebranch will\n>> > # assume a $hash_base of HEAD instead of the correct\n>> > # somebranch.\n\nNote: the blob and history links _in a tree page_ are wrong. The page\nitself is correct.\nWhat this means is that if you go to project.git/tree/somebranch you\nget the correct list of files and directories for the project at\nsomebranch's HEAD, but all the links IN that page will point to the\nwrong version, because they will have no $hash_base set and will thus\ndefault to HEAD.\n\nYou can see this effect for example by going to\nhttp://repo.or.cz/w/git.git/v1.4.0?a=tree <- this hand-crafted path\nwould never be generated by the old pathinfo code, but it show exactly\nwhat would happen with my pahtinfo code on\nhttp://repo.or.cz/w/git.git/tree/v1.4.0 if $hash_base wasn't set.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"92222","messageId":"cb7bb73a0810022304r11d2ad87q7691213ff67f7e4c@mail.gmail.com","threadId":"15742","inReplyTo":"200810030248.57144.jnareb@gmail.com","subject":"Re: [PATCHv4] gitweb: parse project/action/hash_base:filename PATH_INFO","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-10-03T06:04:13Z","receivedAt":"2008-10-03T06:04:13Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Fri, Oct 3, 2008 at 2:48 AM, Jakub Narebski wrote:\n> On Thu, 2 Oct 2008, Giuseppe Bilotta wrote:\n>> OTOH, while setting both $hash and $hash_base has worked fine for me\n>> so far (because the right one is automatically used and apparently\n>> setting the other doesn't hurt), choosing which one to set is a much\n>> hairier case. Do you have suggestions for a better way to always make\n>> it work?\n>\n> Well, it is either checking $action and setting either $hash or\n> $hash_base, or setting both, with some comments on why and when it is\n> needed (as discussed on #git). IIUC $hash_base is needed only for\n> filename-taking tree actions which acts on top-tree, and therefore\n> don't need $file_name, like 'project/tree/branch' or related\n> 'project/history/branch' (the latter is practically almost equivalent\n> to 'project/shortlog/branch' or 'project/branch').\n>\n> I'm not sure if it wouldn't be better to call validate_refname($refname)\n> once, either as:\n>\n> $hash_base ||= $hash ||= validate_refname($refname);\n>\n> but that might be incorrect in the obscure case of setting $hash via 'h'\n> CGI query parameter, and letting gitweb to set-up $hash_base via\n> path_info, so perhaps ($refname is local to evaluate_path_info, IIRC)\n>\n> $refname = validate_refname($refname);\n> $hash ||= $refname;\n> $hash_base ||= $refname;\n\nI'll go with this.\n\n> But that is just nitpicking this fragment of code to death. In short:\n> either check which of $hash and $hash_base to set in this branch of\n> conditional, or explain why setting both $hash and $hash_base is needed,\n> and why it is acceptable, either as comments, or in commit message.\n\nComment is probably better, as long as I remember to move it with the\ncode it belongs to ;)\n\n>>>> @@ -631,8 +642,15 @@ 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>>>> + # the parameter we want to recycle may be either part of the\n>>>> + # list of CGI parameter, or recovered from PATH_INFO\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>>> I would _perhaps_ add here comment that multivalued parameters can come\n>>> only from CGI query string, so there is no need for something like:\n>>>\n>>> + $params{$name} = (ref($$name) ? @$name : $$name) if $$name;\n>>>\n>>>> + }\n>>>> }\n>>>> }\n>>>> }\n>>>\n>>> This fragment is a bit of ugly code, hopefully corrected in later patch.\n>>> I think it would be better to have 'refactor parsing/validation of input\n>>> parameters' to be very fist patch in series; I am not sure but I suspect\n>>> that is a kind of bugfix for current \"$project/$hash\" ('shortlog' view)\n>>> and \"$project/$hash_base:$file_name\" ('blob_plain' and 'tree' view)\n>>> path_info.\n>>\n>> But implementing the path_info parsing first makes the input param\n>> refactoring SO much nicer that I would rather put a comment here\n>> saying \"this code sucks: we should rather collect all input\n>> parameters\" and then clean it up on the subsequent patch.\n>\n> Why not cleanup first?\n\nBecause cleaning it up depends on the refactoring, and the refactoring\nis much cleaner when path_info already handles $action too.\n\n> When implementing href(..., -replay=>1) I have forgot that some of\n> gitweb parameters are implicitly passed ($project, because it is needed\n> in most gitweb links), and some can be passed via path_info ($hash\n> and/or $hash_base, $file_name). Your code adds $action to the mix, but\n> it doesn't change the fact that 1.) even before your code -replay case\n> was incorrect for some path_info links (handcrafted, as gitweb generates\n> only $project via path_info); 2.) code you have added is a bit ugly.\n>\n> Besides using variables change a little meaning of -replay, namely\n> in your code gitweb always sets action, even for non-path_name links\n> when we started from \"default action\" (i.e. without action set) links.\n> I guess this is mainly theoretical issue, as I don't think that default\n> views use many -replay links.\n\nAh the issue of the default action is something I hadn't taken into\nconsideration, actually. Now the question is, should replay keep\ndefault -> default, or should it go with default -> last incantation?\n\n> P.S. with the idea of pushing parameters obtained not from CGI query\n> string to $cgi->param() via \"$cgi->param($name, $value);\" or in named\n> params form \"$cgi->(-name=>$name, -value=>$value);\" you would not need\n> to change (a bit hacky, admittedly) href(...,-replay=>1) code.\n\nYes, but it would muddy the waters about 'where did this parameter\ncome from' in case we ever need to know that.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"92224","messageId":"cb7bb73a0810030024t663940dbqa23f2e1bb75e23bc@mail.gmail.com","threadId":"15742","inReplyTo":"200810030336.31469.jnareb@gmail.com","subject":"Re: [PATCHv4] gitweb: refactor input parameters parse/validation","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-10-03T07:24:35Z","receivedAt":"2008-10-03T07:24:35Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Fri, Oct 3, 2008 at 3:36 AM, Jakub Narebski wrote:\n> On Thu, 2 Oct 2008, Giuseppe Bilotta wrote:\n>\n>> Since input parameters can now be obtained both from CGI parameters and\n>> PATH_INFO, we would like most of the code to be agnostic about the way\n>> parameters were retrieved.\n>\n> I'd prefer that this cleanup/refactoring patch was _first_ patch in\n> the series, as we were able to obtain parameters both from CGI query\n> parameters and from PATH_INFO ($project, $hash or $hash_base+$file_name)\n> _before_ first patch in this series. So it correct not only issue\n> introduced by first patch (and fixed somewhat there), but what was\n> outstanding (but rare because gitweb did not generate such links)\n> issue.\n\nIt's true that the patch makes sense regardless of the rest of the\npath_info patch, indeed.\n\n>> We thus collect all the parameters into the new %input_params hash,\n>> removing the need for ad-hoc kludgy code in href().\n>\n> Alternate solution would be push data from PATH_INFO in query params\n> data using (for example)\n>\n> $cgi->param('a', $action);\n>\n> or, naming parameters explicitely\n>\n> $cgi->param(-name=>'a', -value=>$action);\n>\n> This avoids need for additional variable, reuses current code, and\n> nicely sidesteps issue whether to use long names for keys ('action',\n> 'file_name') or short ones from CGI query ('a', 'f').\n>\n>\n> It would probably has to be at least partially written to check which\n> of those two solutions (%input_params or $cgi->param('a', $a)))\n> is better.\n\nIf we really don't want to care where the parameters came from *at\nall*, writing on $cgi->param is likely to be the cleanest solution.\n\n>> As much of the\n>> parameters validation code is now shared between CGI and PATH_INFO,\n>> although this requires PATH_INFO parsing to be interleaved into the main\n>> code instead of being factored out into its own routine.\n>\n> I'm not sure if it is worth it then to unify parameters validation,\n> with such drawback.\n\nYeah, I don't like it very much either. But it does spare a little on\nthe validation. OTOH, the amount we spare is not extraordinary, so\nit's probably not worth the spaghettization of the code ...\n\n>> # if we're called with PATH_INFO, we have to strip that\n>> -# from the URL to find our real URL\n>> -if (my $path_info = $ENV{\"PATH_INFO\"}) {\n>> +# from the URL to find our real URL. PATH_INFO is kept\n>> +# as it's used later on for parameter extraction\n>> +my $path_info = $ENV{\"PATH_INFO\"};\n>> +if ($path_info) {\n>> $my_url =~ s,\\Q$path_info\\E$,,;\n>> $my_uri =~ s,\\Q$path_info\\E$,,;\n>> }\n>\n> This might be separate patch, if you wanted to increase your commit\n> count ;-) More seriously I think it should be at least briefly\n> mentioned in commit message (make $path_info global).\n\nNote however that the path_info evaluation is _destructive_, so after\nevaluation is complete we don't have much of it left.\n\n[snip]\n\n> Nice, although I'm not sure if [%@]cgi_param_mapping has to global.\n> If we use long parameters names as keys, I think it has to, somewhat.\n> See also comment below.\n\n>> +# fill %input_params with the CGI parameters. All values except for 'opt'\n>> +# should be single values, but opt can be an array. We should probably\n>> +# build an array of parameters that can be multi-valued, but since for the time\n>> +# being it's only this one, we just single it out\n>> +while (my ($name, $symbol) = each %cgi_param_mapping) {\n>> + if ($symbol eq 'opt') {\n>> + $input_params{$name} = [$cgi->param($symbol)];\n>> + } else {\n>> + $input_params{$name} = $cgi->param($symbol);\n>> }\n>> }\n>\n> If it was chosen to use short (CGI query) parameter names, the above\n> loop could be replaced by simple\n>\n> %input_params = $cgi->Vars;\n>\n> or to be more exact, if we want to have multi-valued parameters stored\n> as array ref\n>\n> %input_params = map { [ split /\\0/ ] if /\\0/; } $cgi->Vars;\n>\n>\n> See CGI(3pm):\n>\n> When using this [Vars], the thing you must watch out for are multivalued CGI\n> parameters. Because a hash cannot distinguish between scalar and list con-\n> text, multivalued parameters will be returned as a packed string, separated\n> by the \"\\0\" (null) character. You must split this packed string in order\n> to get at the individual values.\n\nAh, an interesting alternative. This would make parameter copying a\none-liner, almost as good as just using $cgi->param for everything :)\n\n>> -# parameters which are pathnames\n>> -our $project = $cgi->param('p');\n>> +# next, we want to parse PATH_INFO (which was already stored in $path_info at\n>> +# the beginning). This is a little hairy because PATH_INFO parsing needs some\n>> +# form of parameter validation, so we interleave parsing and validation.\n>\n> I don't think it is a good idea. In my opinion, for my taste, it would\n> be better to separate evaluating path_info from the rest.\n>\n> We could instead introduce convention that if variable (like $project)\n> is set, then it is assumed to be validated; if it is present only in\n> the %input_params hash, then it has to be validated.\n>\n>\n> On the other hand this ordering, first by parameter, then by method\n> of extraction could be seem quite equally valid. Nevertheless I think\n> that previous flow with separate evaluate_path_info() and what should\n> be evaluate_CGI_query() has better encapsulation.\n>\n>> +#\n>> +# The accepted PATH_INFO syntax is $project/$action/$hash or\n>> +# $project/$action/$hash_base:$file_name, where $action may be missing (mostly for\n>> +# backwards compatibility), so we need to parse and validate the parameters in\n>> +# this same order.\n>> +\n>> +# clear $path_info of any leading /\n>> +$path_info =~ s,^/+,,;\n>> +\n>> +our $project = $input_params{'project'};\n>> +if ($path_info && !defined $project) {\n>> + # if $project was not defined by CGI, we try to extract it from\n>> + # $path_info\n>> + $project = $path_info;\n>> + $project =~ s,/+$,,;\n>> + while ($project && !check_head_link(\"$projectroot/$project\")) {\n>> + $project =~ s,/*[^/]*$,,;\n>> + }\n>> + $input_params{'project'} = $project;\n>> +} else {\n>> + # otherwise, we suppress $path_info parsing altogether\n>> + $path_info = undef;\n>> +}\n>> +\n>> +# project name validation\n>> if (defined $project) {\n>> if (!validate_pathname($project) ||\n>> !(-d \"$projectroot/$project\") ||\n>\n> Note that this code does less checking if $project is in path_info than\n> for the case where it is set by CGI query. Perhaps there should be base\n> fast check in a loop, and more extensive test later.\n\nUh ... isn't this exactly what's happening? In the loop we just gobble\nuntil we find a git dir. Validation is then done, and it's the _same_\nvalidation for both cases. Why do you say that path_info $project is\nless checked?\n\n>> @@ -408,16 +506,66 @@ if (defined $project) {\n>> undef $project;\n>> die_error(404, \"No such project\");\n>> }\n>> +\n>> + # we purge the $project name from the $path_info, preparing it for\n>> + # subsequent parameters extraction\n>> + $path_info =~ s,^\\Q$project\\E/*,, if defined $path_info;\n>> +} else {\n>> + # we also suppress $path_info parsing if no project was defined\n>> + $path_info = undef;\n>> +}\n>\n> In evaluate_path_info it was simply 'return if...'; here with mentioned\n> interleaving it is harder and a bit hacky.\n\nI know.\n\n>> $params{'project'} = $project unless exists $params{'project'};\n>>\n>> if ($params{-replay}) {\n>> - while (my ($name, $symbol) = each %mapping) {\n>> - if (!exists $params{$name}) {\n>> - # the parameter we want to recycle may be either part of the\n>> - # list of CGI parameter, or recovered from PATH_INFO\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>> + while (my ($name, $val) = each %input_params) {\n>> + $params{$name} = $input_params{$name}\n>> + unless (exists $params{$name});\n>\n> Very nice, short code. Should be something like that from\n> the very beginning.\n\nOk, I'll try working up a patch for params merging before any\npath_info extensions.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"92237","messageId":"200810031231.40146.jnareb@gmail.com","threadId":"15742","inReplyTo":"cb7bb73a0810022304r11d2ad87q7691213ff67f7e4c@mail.gmail.com","subject":"Re: [PATCHv4] gitweb: parse project/action/hash_base:filename PATH_INFO","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-10-03T10:31:38Z","receivedAt":"2008-10-03T10:31:38Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 3 Oct 2008, Giuseppe Bilotta wrote:\n> On Fri, Oct 3, 2008 at 2:48 AM, Jakub Narebski wrote:\n>> On Thu, 2 Oct 2008, Giuseppe Bilotta wrote:\n\n>>> But implementing the path_info parsing first makes the input param\n>>> refactoring SO much nicer that I would rather put a comment here\n>>> saying \"this code sucks: we should rather collect all input\n>>> parameters\" and then clean it up on the subsequent patch.\n>>\n>> Why not cleanup first?\n> \n> Because cleaning it up depends on the refactoring, and the refactoring\n> is much cleaner when path_info already handles $action too.\n\nHmmm... it looks like after refactoring you don't have to change href()\nat all when adding $action to parameters gitweb can get from path_info;\nall that having refactoring (cleanup) upfront changes in/for this patch\nis a bit different saving action (also in %input_params) and lack of\nthis ugly and I think subtly wrong added code for href(...,-replay=>1).\n \n>> When implementing href(..., -replay=>1) I have forgot that some of\n>> gitweb parameters are implicitly passed ($project, because it is needed\n>> in most gitweb links), and some can be passed via path_info ($hash\n>> and/or $hash_base, $file_name). Your code adds $action to the mix, but\n>> it doesn't change the fact that 1.) even before your code -replay case\n>> was incorrect for some path_info links (handcrafted, as gitweb generates\n>> only $project via path_info); 2.) code you have added is a bit ugly.\n>>\n>> Besides using variables change a little meaning of -replay, namely\n>> in your code gitweb always sets action, even for non-path_name links\n>> when we started from \"default action\" (i.e. without action set) links.\n>> I guess this is mainly theoretical issue, as I don't think that default\n>> views use many -replay links.\n> \n> Ah the issue of the default action is something I hadn't taken into\n> consideration, actually. Now the question is, should replay keep\n> default -> default, or should it go with default -> last incantation?\n\nI think we should use default -> default.\n\nBesides I think there can also be an issue (\"can\" because I am not sure\nif in practice it is a problem) the fact that gitweb sometimes expand\nparameters to sha-1, for example setting $head to git_get_head_hash()\nif it is not set (default param value), and not to 'HEAD'. This perhaps\nshould be changed, but to be on safer side better not to use 'action\nvariables' because some code treats them as temporary variables.\n\n>> P.S. with the idea of pushing parameters obtained not from CGI query\n>> string to $cgi->param() via \"$cgi->param($name, $value);\" or in named\n>> params form \"$cgi->(-name=>$name, -value=>$value);\" you would not need\n>> to change (a bit hacky, admittedly) href(...,-replay=>1) code.\n> \n> Yes, but it would muddy the waters about 'where did this parameter\n> come from' in case we ever need to know that.\n\nTrue. Like for example implementing -faithful_replay where if parameter\nwas passed through path_info it is replayed through path_info, and if\nit was passed through query string it is replayed as CGI query param.\n\nAfter thinking a bit about that I think that %input_params idea is\nsuperior to both $cgi->params(-name=>..., -value=>...) and to have\neither no strict refs $$name or name to action variable ref hash.\nSee also my comment for the refactoring patch.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"92236","messageId":"200810031320.28843.jnareb@gmail.com","threadId":"15742","inReplyTo":"cb7bb73a0810030024t663940dbqa23f2e1bb75e23bc@mail.gmail.com","subject":"Re: [PATCHv4] gitweb: refactor input parameters parse/validation","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-10-03T11:20:28Z","receivedAt":"2008-10-03T11:20:28Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 3 Oct 2008, Giuseppe Bilotta wrote:\n> On Fri, Oct 3, 2008 at 3:36 AM, Jakub Narebski wrote:\n>> On Thu, 2 Oct 2008, Giuseppe Bilotta wrote:\n\n>>> We thus collect all the parameters into the new %input_params hash,\n>>> removing the need for ad-hoc kludgy code in href().\n>>\n>> Alternate solution would be push data from PATH_INFO in query params\n>> data using (for example)\n>>\n>> $cgi->param('a', $action);\n>>\n>> or, naming parameters explicitely\n>>\n>> $cgi->param(-name=>'a', -value=>$action);\n>>\n>> This avoids need for additional variable, reuses current code, and\n>> nicely sidesteps issue whether to use long names for keys ('action',\n>> 'file_name') or short ones from CGI query ('a', 'f').\n>>\n>>\n>> It would probably has to be at least partially written to check which\n>> of those two solutions (%input_params or $cgi->param('a', $a)))\n>> is better.\n> \n> If we really don't want to care where the parameters came from *at\n> all*, writing on $cgi->param is likely to be the cleanest solution.\n\nI now think that %input_params is a better solution, even if we won't\nbe implementing -true_replay / -faithful_replay option, where options\ngotten in path_info would be passed in path_info, and options passed\nin query string would be passed as CGI params (even if those params\ncould be encoded in path_info).\n \nOne of the advantages is that having @cgi_param_mapping near the top\nprovides opportunity to describe short CGI query options.\n\nAnother advantage is that with %input_params it is easy to find if\nparameter is multivalued, or is meant to be multivalued: just check\nref($input_params{long_name}); well, perhaps not _that_ easy, but...\n\n>>> As much of the\n>>> parameters validation code is now shared between CGI and PATH_INFO,\n>>> although this requires PATH_INFO parsing to be interleaved into the main\n>>> code instead of being factored out into its own routine.\n>>\n>> I'm not sure if it is worth it then to unify parameters validation,\n>> with such drawback.\n> \n> Yeah, I don't like it very much either. But it does spare a little on\n> the validation. OTOH, the amount we spare is not extraordinary, so\n> it's probably not worth the spaghettization of the code ...\n\nAvoiding repetition can be done in different ways, for example we can\nhave gitweb assume that if value is already in params variable\n($project, $action, $hash,...) it is already validated, and if it is\nonly in %input_params it is not. Add to that for example putting\ncommon part of project path checks into validate_project and you have\nall the advantages (no code repetition, spared validation) without\ndrawback of harder to maintain spaghetti-like code.\n\n>>> +# fill %input_params with the CGI parameters. All values except for 'opt'\n>>> +# should be single values, but opt can be an array. We should probably\n>>> +# build an array of parameters that can be multi-valued, but since for the time\n>>> +# being it's only this one, we just single it out\n>>> +while (my ($name, $symbol) = each %cgi_param_mapping) {\n>>> + if ($symbol eq 'opt') {\n>>> + $input_params{$name} = [$cgi->param($symbol)];\n>>> + } else {\n>>> + $input_params{$name} = $cgi->param($symbol);\n>>> }\n>>> }\n>>\n>> If it was chosen to use short (CGI query) parameter names, the above\n>> loop could be replaced by simple\n>>\n>> %input_params = $cgi->Vars;\n>>\n>> or to be more exact, if we want to have multi-valued parameters stored\n>> as array ref\n>>\n>> %input_params = map { [ split /\\0/ ] if /\\0/; } $cgi->Vars;\n>>\n>>\n>> See CGI(3pm):\n>>\n>> When using this [Vars], the thing you must watch out for are multivalued CGI\n>> parameters. Because a hash cannot distinguish between scalar and list con-\n>> text, multivalued parameters will be returned as a packed string, separated\n>> by the \"\\0\" (null) character. You must split this packed string in order\n>> to get at the individual values.\n> \n> Ah, an interesting alternative. This would make parameter copying a\n> one-liner, almost as good as just using $cgi->param for everything :)\n\nHmmmm...\n \n\n[cut]\n>> Note that this code does less checking if $project is in path_info than\n>> for the case where it is set by CGI query. Perhaps there should be base\n>> fast check in a loop, and more extensive test later.\n> \n> Uh ... isn't this exactly what's happening? In the loop we just gobble\n> until we find a git dir. Validation is then done, and it's the _same_\n> validation for both cases. Why do you say that path_info $project is\n> less checked?\n\nFor project from query string we now have:\n\n !validate_pathname($project) ||\n !(-d \"$projectroot/$project\") ||\n !check_head_link(\"$projectroot/$project\") ||\n ($export_ok && !(-e \"$projectroot/$project/$export_ok\")) ||\n ($strict_export && !project_in_list($project))\n\nFor project from path_info we now have:\n\n while ($project && !check_head_link(\"$projectroot/$project\")) {\n\t$project =~ s,/*[^/]*$,,;\n }\n ...\n !validate_pathname($project) ||\n ($export_ok && !-e \"$projectroot/$project/$export_ok\") ||\n ($strict_export && !project_in_list($project))\n \nIt is almost the same; I have thought that they differ more. Perhaps\nsome of above code could be refactored into validate_project(), or\nproject_ok() subroutine.\n\n>>> $params{'project'} = $project unless exists $params{'project'};\n>>>\n>>> if ($params{-replay}) {\n>>> - while (my ($name, $symbol) = each %mapping) {\n>>> - if (!exists $params{$name}) {\n>>> - # the parameter we want to recycle may be either part of the\n>>> - # list of CGI parameter, or recovered from PATH_INFO\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>>> + while (my ($name, $val) = each %input_params) {\n>>> + $params{$name} = $input_params{$name}\n>>> + unless (exists $params{$name});\n>>\n>> Very nice, short code. Should be something like that from\n>> the very beginning.\n> \n> Ok, I'll try working up a patch for params merging before any\n> path_info extensions.\n\nPerhaps also put query string handling into evaluate_CGI_query(), or\nevaluate_query_string(), similar to evaluate_path_info()?\n\n-- \nJakub Narebski\nPoland\n"},{"id":"92239","messageId":"200810031324.53811.jnareb@gmail.com","threadId":"15742","inReplyTo":"cb7bb73a0810022330l498bdb20h703dec7833a443e@mail.gmail.com","subject":"Re: [PATCHv4] gitweb: generate project/action/hash URLs","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-10-03T11:24:53Z","receivedAt":"2008-10-03T11:24:53Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 3 October 2008, Giuseppe Bilotta wrote:\n> On Fri, Oct 3, 2008 at 3:48 AM, Jakub Narebski wrote:\n>> On Tue, 2 Oct 2008, Giuseppe Bilotta wrote:\n\n>>> + # Finally, 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{'file_name'};\n>>> + }\n>>> + delete $params{'hash'};\n>>> + delete $params{'hash_base'};\n>>> + } elsif (defined $params{'hash'}) {\n>>> + $href .= \"/\".esc_url($params{'hash'});\n>>> + delete $params{'hash'};\n>>> + }\n>>\n>> Hmmmm...\n>>\n>> Shouldn't the code first check for $file_name, then add either\n>> \"$hash_base:$file_name\" (url-escaped), or \"$hash\" (not \"$hash_base\")?\n> \n> Hm, your idea is probably better indeed, if we can ensure that\n> $file_name is always set for generated links that need $hash_base (I\n> mean, as opposed to the root tree case we were discussing for the\n> first patch).\n\nHmm... I'm just worrying here about diluting meaning of params passed\nvia path_info. We had either project/hash, or project/hash_base:file_name;\nnow the hashy parameter in path_info can be hash or hash_base and we\ndon't know which. But perhaps I am worrying over nothing...\n\nA short comment though would be nice (that we can have $hash or\n$hash_base for case without $file_name)\n-- \nJakub Narebski\nPoland\n"},{"id":"92238","messageId":"200810031328.33956.jnareb@gmail.com","threadId":"15742","inReplyTo":"1222906234-8182-5-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv4] gitweb: use_pathinfo filenames start with /","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-10-03T11:28:33Z","receivedAt":"2008-10-03T11:28:33Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 2 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.\n\n...in 'raw' mode/'blob_plain' view.\n\nThis allows to for example view web site as it was at some revision,\nfollowing relative links and checking if they are broken.\n\n> \n> Signed-off-by: Giuseppe Bilotta \n\nAcked-by: Jakub Narebski \n\n(Note that it depends on previous patch, and without it doesn't make\nsense, so this Ack doesn't matter much now!)\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 2c380ac..3e5b2b7 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -690,7 +690,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# Strip any trailing / from $href, or we might get double\n> \t\t# slashes when the script is the DirectoryIndex\n> @@ -708,11 +708,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":"92285","messageId":"200810040315.37662.jnareb@gmail.com","threadId":"15742","inReplyTo":"1222906234-8182-4-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv4] gitweb: generate project/action/hash URLs","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-10-04T01:15:36Z","receivedAt":"2008-10-04T01:15:36Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 2 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\nI think this is good.\n\n>---\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\nThat I'm not sure about, both the layout of conditional (shouldn't\nwe check $file_name first), and the fact that we remove parameter\nwhich is not passed, and can be even not recoverable (for example\nboth 'hash' and 'hash_base' set, 'hash' != 'hash_base', and\n'file_name' not set). So the code above probably has some strange\ncorner cases, but I guess it wouldn't be triggered by links generated\nby gitweb.\n\nBut I guess that is \"good enough\", especially that 'tree' and 'history'\naction links can have 'file_name' unset if they refer to top tree, and\nthey still need 'hash_base'.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"92284","messageId":"200810040331.27605.jnareb@gmail.com","threadId":"15742","inReplyTo":"1222906234-8182-6-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv4] gitweb: parse parent..current syntax from pathinfo","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-10-04T01:31:27Z","receivedAt":"2008-10-04T01:31:27Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 2 Oct 2008, Giuseppe Bilotta wrote:\n\n> This 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\nIn short, it allows to have link to '*diff' views using path_info URL,\nor in general to pass $hash_[parent_]base and $file_parent using\npath_info.\n \n> Signed-off-by: Giuseppe Bilotta \n> ---\n> gitweb/gitweb.perl | 26 ++++++++++++++++++++++++--\n> 1 files changed, 24 insertions(+), 2 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 3e5b2b7..89e360f 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -534,7 +534,9 @@ if ($path_info && !defined $action) {\n> \n> # we can now parse ref and pathnames in PATH_INFO\n> if ($path_info) {\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\nStyle: I would use (but that is perhaps matter of taste)\n\n+\tmy ($parentrefname, $parentpathname, $refname, $pathname) =\n+\t\t($2, $4, $5, $7);\n\nAlso it would be I think simpler to use instead non-catching grouping,\ni.e. (?: xxx ) extended pattern (see perlre(1)), and use \n($1, $2, $3, $4), or even simpler 'list = (string =~ regexp)' form. \n\n\nI also think that the situation is more complicated than that, if we\nwant to be more correct. \n\nThe following path_info layouts with '..' make sense:\n\n hpb:fp..hb:f\n hpb..hb:f == hpb:f..hb:f\n hp..h\n\nAnd the layout below can be though to make sense, but it is just\nplain weird.\n\n hpb:fp..f == hpb:fp..HEAD:f\n \n\n> \tif (defined $pathname) {\n> \t\t# we got \"project.git/action/branch:filename\" or \"project.git/action/branch:dir/\"\n> \t\t# we could use git_get_type(branch:pathname), but it needs $git_dir\n> @@ -543,7 +545,11 @@ if ($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\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\nGood catch.\n\n> \t\t}\n> \t\t$input_params{'hash_base'} ||= $refname;\n> \t\t$input_params{'file_name'} ||= $pathname;\n> @@ -553,6 +559,22 @@ if ($path_info) {\n> \t\t$input_params{'hash'} ||= $refname;\n> \t\t$input_params{'hash_base'} ||= $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$input_params{'hash_parent_base'} ||= $parentrefname;\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\tif (defined $input_params{'file_parent'}) {\n> +\t\t\t$input_params{'hash_parent'} ||= git_get_hash_by_path($input_params{'hash_parent_base'}, $input_params{'file_parent'});\n\nThis line is bit long, and I think it should be wrapped..\n\n> +\t\t} else {\n> +\t\t\t$input_params{'hash_parent'} ||= $parentrefname;\n> +\t\t}\n> +\t}\n> }\n\n-- \nJakub Narebski\nPoland\n"},{"id":"92290","messageId":"cb7bb73a0810040024q4dfad117uf719f0aaa17ae95b@mail.gmail.com","threadId":"15742","inReplyTo":"200810040331.27605.jnareb@gmail.com","subject":"Re: [PATCHv4] gitweb: parse parent..current syntax from pathinfo","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2008-10-04T07:24:12Z","receivedAt":"2008-10-04T07:24:12Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Sat, Oct 4, 2008 at 3:31 AM, Jakub Narebski wrote:\n> On Thu, 2 Oct 2008, Giuseppe Bilotta wrote:\n>\n>> This 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>\n> In short, it allows to have link to '*diff' views using path_info URL,\n> or in general to pass $hash_[parent_]base and $file_parent using\n> path_info.\n\nYes, that's probably a better form for the commit message.\n\n>> Signed-off-by: Giuseppe Bilotta \n>> ---\n>> gitweb/gitweb.perl | 26 ++++++++++++++++++++++++--\n>> 1 files changed, 24 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>> index 3e5b2b7..89e360f 100755\n>> --- a/gitweb/gitweb.perl\n>> +++ b/gitweb/gitweb.perl\n>> @@ -534,7 +534,9 @@ if ($path_info && !defined $action) {\n>>\n>> # we can now parse ref and pathnames in PATH_INFO\n>> if ($path_info) {\n>> - my ($refname, $pathname) = split(/:/, $path_info, 2);\n>> + $path_info =~ /^((.+?)(:(.+))?\\.\\.)?(.+?)(:(.+))?$/;\n>> + my ($parentrefname, $parentpathname, $refname, $pathname) = (\n>> + $2, $4, $5, $7);\n>\n> Style: I would use (but that is perhaps matter of taste)\n>\n> + my ($parentrefname, $parentpathname, $refname, $pathname) =\n> + ($2, $4, $5, $7);\n\nRight, I'm not sure why I put the ( on the previous line.\n\n> Also it would be I think simpler to use instead non-catching grouping,\n> i.e. (?: xxx ) extended pattern (see perlre(1)), and use\n> ($1, $2, $3, $4), or even simpler 'list = (string =~ regexp)' form.\n\nGood idea, I'll rework it in that sense.\n\n> I also think that the situation is more complicated than that, if we\n> want to be more correct.\n>\n> The following path_info layouts with '..' make sense:\n>\n> hpb:fp..hb:f\n> hpb..hb:f == hpb:f..hb:f\n> hp..h\n\nAnd these are matched by the above regexp\n\n> And the layout below can be though to make sense, but it is just\n> plain weird.\n>\n> hpb:fp..f == hpb:fp..HEAD:f\n\nI'm afraid I'm not going to support that, although it's probably easy\nto support hpb:fp..:f (i.e. accept a missing refname but on condition\nof having a : in front of the file spec).\n\n>> + if (defined $input_params{'file_parent'}) {\n>> + $input_params{'hash_parent'} ||= git_get_hash_by_path($input_params{'hash_parent_base'}, $input_params{'file_parent'});\n>\n> This line is bit long, and I think it should be wrapped..\n\nBy the way, on the first revision of the path_info patchset, you had me discard\n\n$hash ||= git_get_hash_by_path($hash_base, $file_name);\n\nin the simple case on the basis that it was an extra call to external git.\n\nI actually forgot to remove it from this part of the patchset too at\nthe time, so this gets me wondering about this: should I put it back\nin place in the simple case, or remove it from here too?\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"92291","messageId":"200810040948.15511.jnareb@gmail.com","threadId":"15742","inReplyTo":"cb7bb73a0810040024q4dfad117uf719f0aaa17ae95b@mail.gmail.com","subject":"Re: [PATCHv4] gitweb: parse parent..current syntax from pathinfo","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-10-04T07:48:14Z","receivedAt":"2008-10-04T07:48:14Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Giuseppe Bilotta wrote:\n> On Sat, Oct 4, 2008 at 3:31 AM, Jakub Narebski wrote:\n>> On Thu, 2 Oct 2008, Giuseppe Bilotta wrote:\n>>\n>>> This 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>>\n>> In short, it allows to have link to '*diff' views using path_info URL,\n>> or in general to pass $hash_[parent_]base and $file_parent using\n>> path_info.\n> \n> Yes, that's probably a better form for the commit message.\n\nI have thought about this rather as supplement (addition) to the\ncurrent commit message (which states explicitly new form of supported\npath_info URL), than replacing it.\n \n>> The following path_info layouts with '..' make sense:\n>>\n>> hpb:fp..hb:f\n>> hpb..hb:f == hpb:f..hb:f\n>> hp..h\n> \n> And these are matched by the above regexp\n> \n>> And the layout below can be though to make sense, but it is just\n>> plain weird.\n>>\n>> hpb:fp..f == hpb:fp..HEAD:f\n> \n> I'm afraid I'm not going to support that, although it's probably easy\n> to support hpb:fp..:f (i.e. accept a missing refname but on condition\n> of having a : in front of the file spec).\n\nNo, not supporting this form is just fine.\n \n>>> + if (defined $input_params{'file_parent'}) {\n>>> + $input_params{'hash_parent'} ||= git_get_hash_by_path($input_params{'hash_parent_base'}, $input_params{'file_parent'});\n>>\n>> This line is bit long, and I think it should be wrapped..\n> \n> By the way, on the first revision of the path_info patchset, you had me discard\n> \n> $hash ||= git_get_hash_by_path($hash_base, $file_name);\n> \n> in the simple case on the basis that it was an extra call to external git.\n> \n> I actually forgot to remove it from this part of the patchset too at\n> the time, so this gets me wondering about this: should I put it back\n> in place in the simple case, or remove it from here too?\n\nI think you should remove it here too. IMHO if needed, it should be\ndealt with (and I think is dealt with) in appropriate action subroutine.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"92323","messageId":"200810051019.39702.jnareb@gmail.com","threadId":"15742","inReplyTo":"200810040331.27605.jnareb@gmail.com","subject":"Re: [PATCHv4] gitweb: parse parent..current syntax from pathinfo","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-10-05T08:19:39Z","receivedAt":"2008-10-05T08:19:39Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sun, 4 Oct 2008, Jakub Narebski wrote:\n> On Thu, 2 Oct 2008, Giuseppe Bilotta wrote:\n \n> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> > index 3e5b2b7..89e360f 100755\n> > --- a/gitweb/gitweb.perl\n> > +++ b/gitweb/gitweb.perl\n> > @@ -534,7 +534,9 @@ if ($path_info && !defined $action) {\n> >  \n> >  # we can now parse ref and pathnames in PATH_INFO\n> >  if ($path_info) {\n> > -     my ($refname, $pathname) = split(/:/, $path_info, 2);\n> > +     $path_info =~ /^((.+?)(:(.+))?\\.\\.)?(.+?)(:(.+))?$/;\n> > +     my ($parentrefname, $parentpathname, $refname, $pathname) = (\n> > +             $2, $4, $5, $7);\n> \n> Style: I would use (but that is perhaps matter of taste)\n> \n> +       my ($parentrefname, $parentpathname, $refname, $pathname) =\n> +               ($2, $4, $5, $7);\n> \n> Also it would be I think simpler to use instead non-catching grouping,\n> i.e. (?: xxx ) extended pattern (see perlre(1)), and use \n> ($1, $2, $3, $4), or even simpler  'list = (string =~ regexp)'  form.  \n\nAlternate solution would be to use split(..., ..., 2), but I think you\ngot the regular expression right. Just mentioning...\n\n-- \nJakub Narebski\nPoland\n"},{"id":"92379","messageId":"200810060217.36233.jnareb@gmail.com","threadId":"15742","inReplyTo":"1222906234-8182-7-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv4] gitweb: generate parent..current URLs","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-10-06T00:17:34Z","receivedAt":"2008-10-06T00:17:34Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 2 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[cut]\n\n>From the first glance, it looks good. I just worry a bit about\ncomplicated issue of hash_parent vs hash_parent_base etc.\n\n-- \nJakub Narebski\nPoland\n"}]}