{"thread":{"id":"5342","subject":"[RFD] gitweb: href() function to generate URLs for CGI","startedAt":"2006-08-21T15:39:32Z","lastAt":"2006-08-22T22:39:25Z","messageCount":17,"participants":["Jakub Narebski","Junio C Hamano","Randal L. Schwartz"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"25690","messageId":"200608211739.32993.jnareb@gmail.com","threadId":"5342","inReplyTo":null,"subject":"[RFD] gitweb: href() function to generate URLs for CGI","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-08-21T15:39:32Z","receivedAt":"2006-08-21T15:39:32Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"In first version of href() function we had\n(commit 06a9d86b49b826562e2b12b5c7e831e20b8f7dce)\n\n\tmy $href = \"$my_uri?\";\n\t$href .= esc_param( join(\";\",\n\t\tmap {\n\t\t\t\"$mapping{$_}=$params{$_}\"\n\t\t} keys %params\n\t) );\n\nFirst, there was a question what happend if someone would enter \nparameter name incorrectly, and some key of %params is not found in \n%mapping hash. The above code would generate warnings (which web admins \nfrown upon), and empty (because undef) parameters corresponding to e.g. \nmistyped parameter name. \n\nOne solution (sweeping under the carpet) would be to use parameter key \nas CGI parameter name if it is not found in the %mapping, i.e.\n\n\tmy $href = \"$my_uri?\";\n\t$href .= esc_param( join(\";\",\n\t\tmap {\n\t\t\tif (exists $mapping{$_}) {\n\t\t\t\t\"$mapping{$_}=$params{$_}\"\n\t\t\t} else {\n\t\t\t\t\"$_=$params{$_}\"\n\t\t\t}\n\t\t} keys %params\n\t) );\n\nAnother solution would be to skip parameters which are not found in\n%mapping. Correct way to do this is:\n\n\tmy $href = \"$my_uri?\";\n\t$href .= esc_param( join(\";\",\n\t\tmap {\n\t\t\t\"$mapping{$_}=$params{$_}\"\n\t\t} grep { exists $mapping{$_} } keys %params\n\t) );\n\n(we cannot put condition in map BLOCK, because map does not filter, only \nact on elements, so the result would be empty parameter (e.g. \";;\" in \ngenerated URL), I guess without warnings).\n\nWhich solutions should be chosen? If the one is chosen, I can send the\npatch.\n\n\nSecond problem is that using href() function, although it consolidates \nto generate URL for CGI, it changes the order of CGI parameters. It \nused to be that 'p' (project) parameter was first, then 'a' (action) \nparameter, then hashes ('h', 'hp', 'hb'), last 'f' (filename) or \n'p' (page) or 's' (searchtext). The simplest and fastest solution would \nbe to create array with all keys of %mapping in appropriate order and \ndo something like this:\n\n\tmy @mapping_sorted = ('project', 'action', 'hash',\n\t\t'hash_parent', 'hash_base', 'file_name', 'searchtext');\n\n\tmy $href = \"$my_uri?\";\n\t$href .= esc_param( join(\";\",\n\t\tmap {\n\t\t\t\"$mapping{$_}=$params{$_}\"\n\t\t} grep { exists $params{$_}} @mapping_sorted;\n\t) );\n\nThe problem is of course updating both %mappings and @mapping_sorted.\n\nIs this really a problem, should this (ordering of CGI parameters)\naddressed?\n\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"25694","messageId":"eccksa$2eh$1@sea.gmane.org","threadId":"5342","inReplyTo":"200608211739.32993.jnareb@gmail.com","subject":"Re: [RFD] gitweb: href() function to generate URLs for CGI","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-08-21T15:52:59Z","receivedAt":"2006-08-21T15:52:59Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Ooops. Sorry for duplicated post.\n\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"25701","messageId":"7v1wrauex2.fsf@assigned-by-dhcp.cox.net","threadId":"5342","inReplyTo":"200608211739.32993.jnareb@gmail.com","subject":"Re: [RFD] gitweb: href() function to generate URLs for CGI","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-08-21T18:22:49Z","receivedAt":"2006-08-21T18:22:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> In first version of href() function we had\n> (commit 06a9d86b49b826562e2b12b5c7e831e20b8f7dce)\n>\n> \tmy $href = \"$my_uri?\";\n> \t$href .= esc_param( join(\";\",\n> \t\tmap {\n> \t\t\t\"$mapping{$_}=$params{$_}\"\n> \t\t} keys %params\n> \t) );\n>\n> First, there was a question what happend if someone would enter \n> parameter name incorrectly, and some key of %params is not found in \n> %mapping hash. The above code would generate warnings (which web admins \n> frown upon), and empty (because undef) parameters corresponding to e.g. \n> mistyped parameter name. \n\nThe one in \"next\" seems to do this (the diff is between \"master\"\nand \"next\"):\n\n@@ -204,7 +277,9 @@ sub href(%) {\n \n \tmy $href = \"$my_uri?\";\n \t$href .= esc_param( join(\";\",\n-\t\tmap { \"$mapping{$_}=$params{$_}\" } keys %params\n+\t\tmap {\n+\t\t\t\"$mapping{$_}=$params{$_}\" if defined $params{$_}\n+\t\t} keys %params\n \t) );\n \n \treturn $href;\n\nSo we perhaps would want to say:\n\n\tif (defined $params{$_} && exists $mapping{$_})\n\ninstead there?\n\n> Second problem is that using href() function, although it consolidates \n> to generate URL for CGI, it changes the order of CGI parameters. It \n> used to be that 'p' (project) parameter was first, then 'a' (action) \n> parameter, then hashes ('h', 'hp', 'hb'), last 'f' (filename) or \n> 'p' (page) or 's' (searchtext). The simplest and fastest solution would \n> be to create array with all keys of %mapping in appropriate order and \n> do something like this:\n>\n> \tmy @mapping_sorted = ('project', 'action', 'hash',\n> \t\t'hash_parent', 'hash_base', 'file_name', 'searchtext');\n>\n> \tmy $href = \"$my_uri?\";\n> \t$href .= esc_param( join(\";\",\n> \t\tmap {\n> \t\t\t\"$mapping{$_}=$params{$_}\"\n> \t\t} grep { exists $params{$_}} @mapping_sorted;\n> \t) );\n>\n> The problem is of course updating both %mappings and @mapping_sorted.\n>\n> Is this really a problem, should this (ordering of CGI parameters)\n> addressed?\n\nKeeping the generated URL stable would be a very desirable\nfeature.  Perhaps something like this?\n\nsub href(%) {\n        my @mapping = ( project => \"p\",\n                        action => \"a\",\n                        hash => \"h\",\n                        hash_parent => \"hp\",\n                        hash_base => \"hb\",\n                        file_name => \"f\",\n                        file_parent => \"fp\",\n                        page => \"pg\",\n                        searchtext => \"s\",\n                        );\n\tmy %mapping;                        \n        for (my $i = 0; $i < @mapping; $i += 2) {\n        \tmy ($k, $v) = ($mapping[$i], $mapping[$i+1]);\n                $mapping{$k} = [$i, $v];\n        }\n\tmy %params = @_;\n\t$params{\"project\"} ||= $project;\n\n\tmy $href = \"$my_uri?\";\n\t$href .= esc_param( join(\";\",\n\t\tmap { $_->[1] }\n\t\tsort { $a->[0] <=> $b->[0] }\n\t\tmap {\n                \t(defined $params{$_} && exists $mapping{$_})\n\t\t\t? [ $mapping{$_}[0], \"$mapping{$_}[1]=$params{$_}\" ]\n                        : ();\n\t\t} keys %params\n\t) );\n\n\treturn $href;\n}\n"},{"id":"25703","messageId":"eccujr$90h$1@sea.gmane.org","threadId":"5342","inReplyTo":"7v1wrauex2.fsf@assigned-by-dhcp.cox.net","subject":"Re: [RFD] gitweb: href() function to generate URLs for CGI","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-08-21T18:38:45Z","receivedAt":"2006-08-21T18:38:45Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n\n> Jakub Narebski <jnareb@gmail.com> writes:\n> \n>> In first version of href() function we had\n>> (commit 06a9d86b49b826562e2b12b5c7e831e20b8f7dce)\n>>\n>>      my $href = \"$my_uri?\";\n>>      $href .= esc_param( join(\";\",\n>>              map {\n>>                      \"$mapping{$_}=$params{$_}\"\n>>              } keys %params\n>>      ) );\n>>\n>> First, there was a question what happend if someone would enter \n>> parameter name incorrectly, and some key of %params is not found in \n>> %mapping hash. The above code would generate warnings (which web admins \n>> frown upon), and empty (because undef) parameters corresponding to e.g. \n>> mistyped parameter name. \n> \n> The one in \"next\" seems to do this (the diff is between \"master\"\n> and \"next\"):\n> \n> @@ -204,7 +277,9 @@ sub href(%) {\n>  \n>       my $href = \"$my_uri?\";\n>       $href .= esc_param( join(\";\",\n> -             map { \"$mapping{$_}=$params{$_}\" } keys %params\n> +             map {\n> +                     \"$mapping{$_}=$params{$_}\" if defined $params{$_}\n> +             } keys %params\n>       ) );\n>  \n>       return $href;\n> \n\nThis doesn't work as expected. Map works on _every_ element of list; if\nexpression doesn't return anything it just puts undef I think. For example\nwhile\n\n        print join(\";\", \n                map { \n                        \"a/$_\" if ($_ eq lc($_)) \n                } (\"a\", \"b\", \"C\", \"d\")),\n                \"\\n\";'\n\nreturns \"a/a;a/b;;a/d\" (notice the doubled ';'), the correct way would be to\nuse grep instead:\n\n        print join(\";\",\n                map {\n                        \"a/$_\"\n                } grep { \n                        ($_ eq lc($_)) \n                } (\"a\", \"b\", \"C\", \"d\")),\n                \"\\n\";\n\nreturns correct \"a/a;a/b;a/d\". So the fragment should read what I wrote:\n\n        my $href = \"$my_uri?\";\n        $href .= esc_param( join(\";\",\n                map {\n                        \"$mapping{$_}=$params{$_}\"\n                } grep { exists $mapping{$_} } keys %params\n        ) );\n\n>> Second problem is that using href() function, although it consolidates \n>> to generate URL for CGI, it changes the order of CGI parameters. It \n>> used to be that 'p' (project) parameter was first, then 'a' (action) \n>> parameter, then hashes ('h', 'hp', 'hb'), last 'f' (filename) or \n>> 'p' (page) or 's' (searchtext). The simplest and fastest solution would \n>> be to create array with all keys of %mapping in appropriate order and \n>> do something like this:\n>>\n>>      my @mapping_sorted = ('project', 'action', 'hash',\n>>              'hash_parent', 'hash_base', 'file_name', 'searchtext');\n>>\n>>      my $href = \"$my_uri?\";\n>>      $href .= esc_param( join(\";\",\n>>              map {\n>>                      \"$mapping{$_}=$params{$_}\"\n>>              } grep { exists $params{$_}} @mapping_sorted;\n>>      ) );\n>>\n>> The problem is of course updating both %mappings and @mapping_sorted.\n>>\n>> Is this really a problem, should this (ordering of CGI parameters)\n>> addressed?\n> \n> Keeping the generated URL stable would be a very desirable\n> feature.  Perhaps something like this?\n> \n> sub href(%) {\n>         my @mapping = ( project => \"p\",\n>                         action => \"a\",\n>                         hash => \"h\",\n>                         hash_parent => \"hp\",\n>                         hash_base => \"hb\",\n>                         file_name => \"f\",\n>                         file_parent => \"fp\",\n>                         page => \"pg\",\n>                         searchtext => \"s\",\n>                         );\n>       my %mapping;                        \n>         for (my $i = 0; $i < @mapping; $i += 2) {\n>               my ($k, $v) = ($mapping[$i], $mapping[$i+1]);\n>                 $mapping{$k} = [$i, $v];\n>         }\n>       my %params = @_;\n>       $params{\"project\"} ||= $project;\n> \n>       my $href = \"$my_uri?\";\n>       $href .= esc_param( join(\";\",\n>               map { $_->[1] }\n>               sort { $a->[0] <=> $b->[0] }\n>               map {\n>                       (defined $params{$_} && exists $mapping{$_})\n>                       ? [ $mapping{$_}[0], \"$mapping{$_}[1]=$params{$_}\" ]\n>                         : ();\n>               } keys %params\n>       ) );\n> \n>       return $href;\n> }\n\nWhat about my proposed solution? Don't sort, use sorted keys instead, i.e.\nsomething like (after unmangling whitespace)\n\nsub href(%) {\n        my @mapping = ( project => \"p\",\n                        action => \"a\",\n                        hash => \"h\",\n                        hash_parent => \"hp\",\n                        hash_base => \"hb\",\n                        file_name => \"f\",\n                        file_parent => \"fp\",\n                        page => \"pg\",\n                        searchtext => \"s\",\n                        );\n        my %mapping;\n        my @mapping_keys;                        \n        for (my $i = 0; $i < @mapping; $i += 2) {\n                my ($k, $v) = ($mapping[$i], $mapping[$i+1]);\n                $mapping{$k} = $v;\n                push @mapping_keys, $k;\n        }\n        my %params = @_;\n        $params{\"project\"} ||= $project;\n\n        my $href = \"$my_uri?\";\n        $href .= esc_param( join(\";\",\n                map { \"$mapping{$_}=$params{$_}\" }\n                grep { exists $params{$_} } \n                @mapping_keys\n        ) );\n\n        return $href;\n}\n\nThis has the advantage and disadvantage of constant cost, linear with number\nof %mapping keys, instead of N log N cost where N is number of parameters.\n\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"25728","messageId":"7vlkphqmac.fsf@assigned-by-dhcp.cox.net","threadId":"5342","inReplyTo":"eccujr$90h$1@sea.gmane.org","subject":"Re: [RFD] gitweb: href() function to generate URLs for CGI","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-08-22T07:09:31Z","receivedAt":"2006-08-22T07:09:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> This doesn't work as expected. Map works on _every_ element of list; if\n> expression doesn't return anything it just puts undef I think. For example\n> while\n>\n>         print join(\";\", \n>                 map { \n>                         \"a/$_\" if ($_ eq lc($_)) \n>                 } (\"a\", \"b\", \"C\", \"d\")),\n>                 \"\\n\";'\n>\n> returns \"a/a;a/b;;a/d\" (notice the doubled ';'), the correct way would be to\n\n... say this, than using extra grep {}.\n\n        print join(\";\", \n                map { \n                        ($_ eq lc($_)) ? \"a/$_\" : ()\n                } (\"a\", \"b\", \"C\", \"d\")),\n                \"\\n\";\n\n> What about my proposed solution? Don't sort, use sorted keys instead, i.e.\n> something like (after unmangling whitespace)\n>\n> sub href(%) {\n>         my @mapping = ( project => \"p\",\n>                         action => \"a\",\n>                         hash => \"h\",\n>                         hash_parent => \"hp\",\n>                         hash_base => \"hb\",\n>                         file_name => \"f\",\n>                         file_parent => \"fp\",\n>                         page => \"pg\",\n>                         searchtext => \"s\",\n>                         );\n>         my %mapping;\n>         my @mapping_keys;                        \n>         for (my $i = 0; $i < @mapping; $i += 2) {\n>                 my ($k, $v) = ($mapping[$i], $mapping[$i+1]);\n>                 $mapping{$k} = $v;\n>                 push @mapping_keys, $k;\n>         }\n\nNow this loop got an expensive way to say %mapping = @mapping\nplus assigning @mapping_keys, so I would do only the push part\nin the loop for readability while changing the name of the\nvariable a bit more meaningful.\n\n\tmy %mapping = @mapping;\n        my @valid_params;\n\tfor (my $i = 0; $i < @mapping; $i += 2) {\n\t        push @valid_params, $mapping[$i];\n\t}\n\nBut aside from that, I very much prefer your version that loops\nover what _we_ define (i.e. @valid_params), rather than what the\nuser happens to throw at us (i.e. keys %params).\n"},{"id":"25730","messageId":"eceekl$b0l$2@sea.gmane.org","threadId":"5342","inReplyTo":"7vlkphqmac.fsf@assigned-by-dhcp.cox.net","subject":"Re: [RFD] gitweb: href() function to generate URLs for CGI","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-08-22T08:18:29Z","receivedAt":"2006-08-22T08:18:29Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n\n> Jakub Narebski <jnareb@gmail.com> writes:\n> \n>> This doesn't work as expected. Map works on _every_ element of list; if\n>> expression doesn't return anything it just puts undef I think. For example\n>> while\n>>\n>>         print join(\";\", \n>>                 map { \n>>                         \"a/$_\" if ($_ eq lc($_)) \n>>                 } (\"a\", \"b\", \"C\", \"d\")),\n>>                 \"\\n\";'\n>>\n>> returns \"a/a;a/b;;a/d\" (notice the doubled ';'), the correct way would be to\n> \n> ... say this, than using extra grep {}.\n> \n>         print join(\";\", \n>                 map { \n>                         ($_ eq lc($_)) ? \"a/$_\" : ()\n>                 } (\"a\", \"b\", \"C\", \"d\")),\n>                 \"\\n\";\n\nThat's better. I didn't know about this trick. Patch will follow.\n\nBTW. I have encountered this error in true situation, while doing history\npagination because I forgot in parse_difftree_raw_line that the\nmap(function, list) works only for prototyped functions, and had undef fed\nto href.\n\n>> What about my proposed solution? Don't sort, use sorted keys instead, i.e.\n>> something like (after unmangling whitespace)\n>>\n>> sub href(%) {\n>>         my @mapping = ( project => \"p\",\n>>                         action => \"a\",\n>>                         hash => \"h\",\n>>                         hash_parent => \"hp\",\n>>                         hash_base => \"hb\",\n>>                         file_name => \"f\",\n>>                         file_parent => \"fp\",\n>>                         page => \"pg\",\n>>                         searchtext => \"s\",\n>>                         );\n>>         my %mapping;\n>>         my @mapping_keys;                        \n>>         for (my $i = 0; $i < @mapping; $i += 2) {\n>>                 my ($k, $v) = ($mapping[$i], $mapping[$i+1]);\n>>                 $mapping{$k} = $v;\n>>                 push @mapping_keys, $k;\n>>         }\n> \n> Now this loop got an expensive way to say %mapping = @mapping\n> plus assigning @mapping_keys, so I would do only the push part\n> in the loop for readability while changing the name of the\n> variable a bit more meaningful.\n> \n>       my %mapping = @mapping;\n>         my @valid_params;\n>       for (my $i = 0; $i < @mapping; $i += 2) {\n>               push @valid_params, $mapping[$i];\n>       }\n> \n> But aside from that, I very much prefer your version that loops\n> over what _we_ define (i.e. @valid_params), rather than what the\n> user happens to throw at us (i.e. keys %params).\n\nSo perhaps\n\n        my %mapping = @mapping;\n        my @params_keys_sorted;\n        for (my $i = 0; $i < @mapping; $i += 2) {\n                if (exists $params{$mapping[$i]}) {\n                        push @params_keys_sorted, $mapping[$i];\n                }\n        }\n\nThis way we have only one loop over all valid parameter names,\nand we wouldn't need grep nor conditional expression in map,\nand map would loop over all (valid) params keys only.\n\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"25734","messageId":"7vodudno88.fsf@assigned-by-dhcp.cox.net","threadId":"5342","inReplyTo":"eceekl$b0l$2@sea.gmane.org","subject":"Re: [RFD] gitweb: href() function to generate URLs for CGI","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-08-22T08:55:51Z","receivedAt":"2006-08-22T08:55:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> So perhaps\n>\n>         my %mapping = @mapping;\n>         my @params_keys_sorted;\n>         for (my $i = 0; $i < @mapping; $i += 2) {\n>                 if (exists $params{$mapping[$i]}) {\n>                         push @params_keys_sorted, $mapping[$i];\n>                 }\n>         }\n>\n> This way we have only one loop over all valid parameter names,\n> and we wouldn't need grep nor conditional expression in map,\n> and map would loop over all (valid) params keys only.\n\nAt that point I suspect that loop can do everything your map {}\ndid, and would look simpler (albeit perhaps a little bit less\nPerlish) like this:\n\nsub href(%) {\n\tmy %params = @_;\n\t$params{\"project\"} ||= $project;\n        my @mapping = ( project => \"p\",\n                        action => \"a\",\n                        hash => \"h\",\n                        hash_parent => \"hp\",\n                        hash_base => \"hb\",\n                        file_name => \"f\",\n                        file_parent => \"fp\",\n                        page => \"pg\",\n                        searchtext => \"s\",\n                        );\n\tmy @result = (); \n        for (my $i = 0; $i < @mapping; $i += 2) {\n        \tmy ($name, $symbol) = ($mapping[$i], $mapping[$i+1]);\n\t\tif (defined $params{$name}) {\n\t\t\tpush @result, \"$symbol=$params{$name}\";\n\t\t}\n\t}\n        return \"$my_uri?\" . esc_param(join(';', @result));\n}\n\nBy the way, is it really what we want to do to call esc_param()\non joined result?\n\nMaybe I am not reading this code correctly, but this feels quite\ncounter-intuitive.  Actually, it feels downright wrong.\n\nSemicolons and equals are used as separators between key-value\npairs (i.e. syntactic elements) so if we have a value\n$params{$name} that happens to contain a ';' or '=' character I\nsuspect we would want to quote that but not the one we use\nbefore the value or between tuples.  Otherwise, how is a search\ntext that is \"a = b;\" encoded in the resulting href?\n\nSo the last part of the above should perhaps read:\n\n\tmy @result = (); \n        for (my $i = 0; $i < @mapping; $i += 2) {\n        \tmy ($name, $symbol) = ($mapping[$i], $mapping[$i+1]);\n\t\tif (defined $params{$name}) {\n\t\t\tpush @result, join('=', esc_param($symbol),\n\t\t\t\t\t\tesc_param($params{$name}));\n\t\t}\n\t}\n        return \"$my_uri?\" . join(';', @result);\n\nWe would also need to fix esc_param to quote ';' and '=' as\nwell, which does not seem to quote them.\n"},{"id":"25739","messageId":"ecej3m$rv2$3@sea.gmane.org","threadId":"5342","inReplyTo":"7vodudno88.fsf@assigned-by-dhcp.cox.net","subject":"Re: [RFD] gitweb: href() function to generate URLs for CGI","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-08-22T09:34:46Z","receivedAt":"2006-08-22T09:34:46Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n\n> So the last part of the above should perhaps read:\n> \n>         my @result = (); \n>         for (my $i = 0; $i < @mapping; $i += 2) {\n>                 my ($name, $symbol) = ($mapping[$i], $mapping[$i+1]);\n>                 if (defined $params{$name}) {\n>                         push @result, join('=', esc_param($symbol),\n>                                                 esc_param($params{$name}));\n>                 }\n>         }\n\nThere is really no need to escape $symbol; we control this,\nand it is CGI param safe.\n\nBTW. why use join? The following\n\n        push @result, \"$symbol=\" . esc_param($params{$name}); \n\nwould be simplier... we could even use\n\n        $href .= ($i > 0? \";\" : \"\") . \"$symbol=\" . esc_param($params{$name});\n\nalthough probably this isn't best idea (worse readibility).\n\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"25742","messageId":"ecenc1$b5o$1@sea.gmane.org","threadId":"5342","inReplyTo":"7vodudno88.fsf@assigned-by-dhcp.cox.net","subject":"Re: [RFD] gitweb: href() function to generate URLs for CGI","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-08-22T10:47:28Z","receivedAt":"2006-08-22T10:47:28Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n\n> Semicolons and equals are used as separators between key-value\n> pairs (i.e. syntactic elements) so if we have a value\n> $params{$name} that happens to contain a ';' or '=' character I\n> suspect we would want to quote that but not the one we use\n> before the value or between tuples.  Otherwise, how is a search\n> text that is \"a = b;\" encoded in the resulting href?\n\nURL for search is created and encoded by browser I think, so search\nfor \"a = b\" generates\n\n        gitweb.cgi?p=git.git&a=search&h=HEAD&s=a+%3D+b\n\nURI (notice it uses '&' as parameter separator, not ';'), \nbut because '=' is not on the list of valid $searchtext characters, \ndue to \n\n        if ($searchtext =~ m/[^a-zA-Z0-9_\\.\\/\\-\\+\\:\\@ ]/)\n\ngitweb returns \"403 Forbidden - Invalid search parameter\".\n\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"25753","messageId":"11562663253739-git-send-email-jnareb@gmail.com","threadId":"5342","inReplyTo":"200608211739.32993.jnareb@gmail.com","subject":"[PATCH 1/2] gitweb: Drop the href() params which keys are not in %mapping","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-08-22T17:05:24Z","receivedAt":"2006-08-22T17:05:24Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"If someone would enter parameter name incorrectly, and some key of\n%params is not found in %mapping hash, the parameter is now\nignored. Change introduced by Martin Waitz in commit\n  756d2f064b2419fcdf9cd9c851f352e2a4f75103\ntried to do that, but it left empty value and there was doubled \";;\"\nin returned string.\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n---\n gitweb/gitweb.perl |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 063735d..89ef5e7 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -278,7 +278,7 @@ sub href(%) {\n \tmy $href = \"$my_uri?\";\n \t$href .= esc_param( join(\";\",\n \t\tmap {\n-\t\t\t\"$mapping{$_}=$params{$_}\" if defined $params{$_}\n+\t\t\tdefined $params{$_} ? \"$mapping{$_}=$params{$_}\" : ()\n \t\t} keys %params\n \t) );\n \n-- \n1.4.1.1\n"},{"id":"25752","messageId":"11562663261084-git-send-email-jnareb@gmail.com","threadId":"5342","inReplyTo":"200608211739.32993.jnareb@gmail.com","subject":"[PATCH 2/2] gitweb: Sort CGI parameters returned by href()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-08-22T17:05:25Z","receivedAt":"2006-08-22T17:05:25Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Restore pre-1c2a4f5addce479c619057c6cdc841802139982f\nordering of CGI parameters.\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n---\n gitweb/gitweb.perl |   22 ++++++++++++----------\n 1 files changed, 12 insertions(+), 10 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 89ef5e7..da0630e 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -260,7 +260,9 @@ ## =====================================\n ## action links\n \n sub href(%) {\n-\tmy %mapping = (\n+\tmy %params = @_;\n+\n+\tmy @mapping = (\n \t\taction => \"a\",\n \t\tproject => \"p\",\n \t\tfile_name => \"f\",\n@@ -271,18 +273,18 @@ sub href(%) {\n \t\tpage => \"pg\",\n \t\tsearchtext => \"s\",\n \t);\n+\tmy %mapping = @mapping;\n \n-\tmy %params = @_;\n \t$params{\"project\"} ||= $project;\n \n-\tmy $href = \"$my_uri?\";\n-\t$href .= esc_param( join(\";\",\n-\t\tmap {\n-\t\t\tdefined $params{$_} ? \"$mapping{$_}=$params{$_}\" : ()\n-\t\t} keys %params\n-\t) );\n-\n-\treturn $href;\n+\tmy @result = ();\n+\tfor (my $i = 0; $i < @mapping; $i += 2) {\n+\t\tmy ($name, $symbol) = ($mapping[$i], $mapping[$i+1]);\n+\t\tif (defined $params{$name}) {\n+\t\t\tpush @result, $symbol . \"=\" . esc_param($params{$name});\n+\t\t}\n+\t}\n+\treturn \"$my_uri?\" . join(';', @result);\n }\n \n \n-- \n1.4.1.1\n"},{"id":"25757","messageId":"86fyfohb4v.fsf@blue.stonehenge.com","threadId":"5342","inReplyTo":"7vodudno88.fsf@assigned-by-dhcp.cox.net","subject":"Re: [RFD] gitweb: href() function to generate URLs for CGI","fromName":"Randal L. Schwartz","fromEmail":"merlyn@stonehenge.com","sentAt":"2006-08-22T18:35:12Z","receivedAt":"2006-08-22T18:35:12Z","isPatch":false,"sender":{"key":"merlyn@stonehenge.com","avatar":"https://gravatar.com/avatar/dc528d210743ff0333e6213f9ee7b33b23f1b7bc1f3c5a8c2d819074ecd7ab19?d=mp&s=160"},"body":">>>>> \"Junio\" == Junio C Hamano <junkio@cox.net> writes:\n\nJunio> \tmy @result = (); \nJunio>         for (my $i = 0; $i < @mapping; $i += 2) {\nJunio>         \tmy ($name, $symbol) = ($mapping[$i], $mapping[$i+1]);\nJunio> \t\tif (defined $params{$name}) {\nJunio> \t\t\tpush @result, \"$symbol=$params{$name}\";\nJunio> \t\t}\nJunio> \t}\nJunio>         return \"$my_uri?\" . esc_param(join(';', @result));\nJunio> }\n\nIf you already depend on the LWP package, then the \"URI\" module\ndoes precisely what you're reinventing.\n\nmy $uri = URI->new(\"http://host/base/path\")\n$uri->query_form(\\%params);\nmy $result = $uri->as_string;\n\nAnd I'd rely on Gisle Aas's experience about constructing these things\nfar more than the thread I've just witnessed here. :)\n\n-- \nRandal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095\n<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>\nPerl/Unix/security consulting, Technical writing, Comedy, etc. etc.\nSee PerlTraining.Stonehenge.com for onsite and open-enrollment Perl training!\n"},{"id":"25759","messageId":"ecfnf0$4bg$1@sea.gmane.org","threadId":"5342","inReplyTo":"86fyfohb4v.fsf@blue.stonehenge.com","subject":"Re: [RFD] gitweb: href() function to generate URLs for CGI","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-08-22T19:55:13Z","receivedAt":"2006-08-22T19:55:13Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Randal L. Schwartz wrote:\n\n>>>>>> \"Junio\" == Junio C Hamano <junkio@cox.net> writes:\n> \n> Junio>        my @result = (); \n> Junio>         for (my $i = 0; $i < @mapping; $i += 2) {\n> Junio>                my ($name, $symbol) = ($mapping[$i], $mapping[$i+1]);\n> Junio>                if (defined $params{$name}) {\n> Junio>                        push @result, \"$symbol=$params{$name}\";\n> Junio>                }\n> Junio>        }\n> Junio>         return \"$my_uri?\" . esc_param(join(';', @result));\n> Junio> }\n> \n> If you already depend on the LWP package, then the \"URI\" module\n> does precisely what you're reinventing.\n> \n> my $uri = URI->new(\"http://host/base/path\")\n> $uri->query_form(\\%params);\n> my $result = $uri->as_string;\n> \n> And I'd rely on Gisle Aas's experience about constructing these things\n> far more than the thread I've just witnessed here. :)\n\nFirst, I'd rather not introduce new dependency to git (and I think Junio\nwould agree). Second, more important, we do _parameters processing_,\nit means renaming parameters (e.g. 'file_name' in params is 'p' in CGI URI),\nin the future perhaps passing project via PATH_INFO not in query string,\nand sorting the CGI parameters.\n\nSo it wouldn't be as easy as writing \n        $uri->query_form(\\%params); \n        return $uri->as_string;\nor as \n        return \"$my_uri?\" . esc_param(join(\";\", \"$_=$params{$_}\") keys %params)'.\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"25760","messageId":"ecfnqj$5m9$1@sea.gmane.org","threadId":"5342","inReplyTo":"ecfnf0$4bg$1@sea.gmane.org","subject":"Re: [RFD] gitweb: href() function to generate URLs for CGI","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-08-22T20:01:25Z","receivedAt":"2006-08-22T20:01:25Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"<opublikowany i wysłany>\n\nJakub Narebski wrote:\n\n> Randal L. Schwartz wrote:\n> \n>>>>>>> \"Junio\" == Junio C Hamano <junkio@cox.net> writes:\n>> \n>> Junio>        my @result = (); \n>> Junio>         for (my $i = 0; $i < @mapping; $i += 2) {\n>> Junio>                my ($name, $symbol) = ($mapping[$i], $mapping[$i+1]);\n>> Junio>                if (defined $params{$name}) {\n>> Junio>                        push @result, \"$symbol=$params{$name}\";\n>> Junio>                }\n>> Junio>        }\n>> Junio>         return \"$my_uri?\" . esc_param(join(';', @result));\n>> Junio> }\n>> \n>> If you already depend on the LWP package, then the \"URI\" module\n>> does precisely what you're reinventing.\n>> \n>> my $uri = URI->new(\"http://host/base/path\")\n>> $uri->query_form(\\%params);\n>> my $result = $uri->as_string;\n>> \n>> And I'd rely on Gisle Aas's experience about constructing these things\n>> far more than the thread I've just witnessed here. :)\n> \n> First, I'd rather not introduce new dependency to git (and I think Junio\n> would agree). Second, more important, we do _parameters processing_,\n> it means renaming parameters (e.g. 'file_name' in params is 'p' in CGI URI),\n> in the future perhaps passing project via PATH_INFO not in query string,\n> and sorting the CGI parameters.\n> \n> So it wouldn't be as easy as writing \n>         $uri->query_form(\\%params); \n>         return $uri->as_string;\n> or as \n>         return \"$my_uri?\" . esc_param(join(\";\", \"$_=$params{$_}\") keys %params)'.\n\nThird, read comment to esc_param:\n\n        # quote unsafe chars, but keep the slash, even when it's not\n        # correct, but quoted slashes look too horrible in bookmarks\n\nI assume that URI module methods do not keep slash...\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"25767","messageId":"7vzmdwmmy7.fsf@assigned-by-dhcp.cox.net","threadId":"5342","inReplyTo":"ecfnqj$5m9$1@sea.gmane.org","subject":"Re: [RFD] gitweb: href() function to generate URLs for CGI","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-08-22T22:21:04Z","receivedAt":"2006-08-22T22:21:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> Third, read comment to esc_param:\n>\n>         # quote unsafe chars, but keep the slash, even when it's not\n>         # correct, but quoted slashes look too horrible in bookmarks\n>\n> I assume that URI module methods do not keep slash...\n\nI do not think so either but I do not necessarily agree with\nthat comment ;-).\n"},{"id":"25768","messageId":"7vveokmmpr.fsf@assigned-by-dhcp.cox.net","threadId":"5342","inReplyTo":"ecenc1$b5o$1@sea.gmane.org","subject":"Re: [RFD] gitweb: href() function to generate URLs for CGI","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-08-22T22:26:08Z","receivedAt":"2006-08-22T22:26:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> URL for search is created and encoded by browser I think, so search\n> for \"a = b\" generates\n>\n>         gitweb.cgi?p=git.git&a=search&h=HEAD&s=a+%3D+b\n\nwhich means href() should generate something similar when it\nwants to reproduce what the user queried, doesn't it?  I do not\nthink quoting after joining does, and that is why I asked that\nquestion.\n"},{"id":"25770","messageId":"ecg12q$3uj$1@sea.gmane.org","threadId":"5342","inReplyTo":"7vveokmmpr.fsf@assigned-by-dhcp.cox.net","subject":"Re: [RFD] gitweb: href() function to generate URLs for CGI","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-08-22T22:39:25Z","receivedAt":"2006-08-22T22:39:25Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n\n> Jakub Narebski <jnareb@gmail.com> writes:\n> \n>> URL for search is created and encoded by browser I think, so search\n>> for \"a = b\" generates\n>>\n>>         gitweb.cgi?p=git.git&a=search&h=HEAD&s=a+%3D+b\n> \n> which means href() should generate something similar when it\n> wants to reproduce what the user queried, doesn't it?  I do not\n> think quoting after joining does, and that is why I asked that\n> question.\n\nYes, ot should... but first we would have to convert all the\ncases of generating URI via esc_param(...) to using href().\nThat means to extend href() somewhat (-absolute=>1 as special\nvalue in href() parameters, or changing href(%) to href(\\%;$)?)\nto be able to generate absolute URI (for example for RSS/OPML).\n\nAnd the case is not that inconceivable: $project can be anything,\nand can have ';' and '=' inside. I'm not sure about valid symbolic refs...\n\nUsage of '&' as query parameters separator instead of ';' probably\ndepend on the browser...\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"}]}