Junio C Hamano wrote:
> Jakub Narebski <jnareb@xxxxxxxxx> writes:
>
>> In first version of href() function we had
>> (commit 06a9d86b49b826562e2b12b5c7e831e20b8f7dce)
>>
>> my $href = "$my_uri?";
>> $href .= esc_param( join(";",
>> map {
>> "$mapping{$_}=$params{$_}"
>> } keys %params
>> ) );
>>
>> First, there was a question what happend if someone would enter
>> parameter name incorrectly, and some key of %params is not found in
>> %mapping hash. The above code would generate warnings (which web admins
>> frown upon), and empty (because undef) parameters corresponding to e.g.
>> mistyped parameter name.
>
> The one in "next" seems to do this (the diff is between "master"
> and "next"):
>
> @@ -204,7 +277,9 @@ sub href(%) {
>
> my $href = "$my_uri?";
> $href .= esc_param( join(";",
> - map { "$mapping{$_}=$params{$_}" } keys %params
> + map {
> + "$mapping{$_}=$params{$_}" if defined $params{$_}
> + } keys %params
> ) );
>
> return $href;
>
This doesn't work as expected. Map works on _every_ element of list; if
expression doesn't return anything it just puts undef I think. For example
while
print join(";",
map {
"a/$_" if ($_ eq lc($_))
} ("a", "b", "C", "d")),
"\n";'
returns "a/a;a/b;;a/d" (notice the doubled ';'), the correct way would be to
use grep instead:
print join(";",
map {
"a/$_"
} grep {
($_ eq lc($_))
} ("a", "b", "C", "d")),
"\n";
returns correct "a/a;a/b;a/d". So the fragment should read what I wrote:
my $href = "$my_uri?";
$href .= esc_param( join(";",
map {
"$mapping{$_}=$params{$_}"
} grep { exists $mapping{$_} } keys %params
) );
>> Second problem is that using href() function, although it consolidates
>> to generate URL for CGI, it changes the order of CGI parameters. It
>> used to be that 'p' (project) parameter was first, then 'a' (action)
>> parameter, then hashes ('h', 'hp', 'hb'), last 'f' (filename) or
>> 'p' (page) or 's' (searchtext). The simplest and fastest solution would
>> be to create array with all keys of %mapping in appropriate order and
>> do something like this:
>>
>> my @mapping_sorted = ('project', 'action', 'hash',
>> 'hash_parent', 'hash_base', 'file_name', 'searchtext');
>>
>> my $href = "$my_uri?";
>> $href .= esc_param( join(";",
>> map {
>> "$mapping{$_}=$params{$_}"
>> } grep { exists $params{$_}} @mapping_sorted;
>> ) );
>>
>> The problem is of course updating both %mappings and @mapping_sorted.
>>
>> Is this really a problem, should this (ordering of CGI parameters)
>> addressed?
>
> Keeping the generated URL stable would be a very desirable
> feature. Perhaps something like this?
>
> sub href(%) {
> my @mapping = ( project => "p",
> action => "a",
> hash => "h",
> hash_parent => "hp",
> hash_base => "hb",
> file_name => "f",
> file_parent => "fp",
> page => "pg",
> searchtext => "s",
> );
> my %mapping;
> for (my $i = 0; $i < @mapping; $i += 2) {
> my ($k, $v) = ($mapping[$i], $mapping[$i+1]);
> $mapping{$k} = [$i, $v];
> }
> my %params = @_;
> $params{"project"} ||= $project;
>
> my $href = "$my_uri?";
> $href .= esc_param( join(";",
> map { $_->[1] }
> sort { $a->[0] <=> $b->[0] }
> map {
> (defined $params{$_} && exists $mapping{$_})
> ? [ $mapping{$_}[0], "$mapping{$_}[1]=$params{$_}" ]
> : ();
> } keys %params
> ) );
>
> return $href;
> }
What about my proposed solution? Don't sort, use sorted keys instead, i.e.
something like (after unmangling whitespace)
sub href(%) {
my @mapping = ( project => "p",
action => "a",
hash => "h",
hash_parent => "hp",
hash_base => "hb",
file_name => "f",
file_parent => "fp",
page => "pg",
searchtext => "s",
);
my %mapping;
my @mapping_keys;
for (my $i = 0; $i < @mapping; $i += 2) {
my ($k, $v) = ($mapping[$i], $mapping[$i+1]);
$mapping{$k} = $v;
push @mapping_keys, $k;
}
my %params = @_;
$params{"project"} ||= $project;
my $href = "$my_uri?";
$href .= esc_param( join(";",
map { "$mapping{$_}=$params{$_}" }
grep { exists $params{$_} }
@mapping_keys
) );
return $href;
}
This has the advantage and disadvantage of constant cost, linear with number
of %mapping keys, instead of N log N cost where N is number of parameters.
--
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html