Re: [RFD] gitweb: href() function to generate URLs for CGI

[Jakub Narebski <jnareb@gmail.com>]

Junio C Hamano wrote:

> Jakub Narebski <jnareb@gmail.com> 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