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:
> 
>> 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
> 
> ... say this, than using extra grep {}.
> 
>         print join(";", 
>                 map { 
>                         ($_ eq lc($_)) ? "a/$_" : ()
>                 } ("a", "b", "C", "d")),
>                 "\n";

That's better. I didn't know about this trick. Patch will follow.

BTW. I have encountered this error in true situation, while doing history
pagination because I forgot in parse_difftree_raw_line that the
map(function, list) works only for prototyped functions, and had undef fed
to 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;
>>         }
> 
> Now this loop got an expensive way to say %mapping = @mapping
> plus assigning @mapping_keys, so I would do only the push part
> in the loop for readability while changing the name of the
> variable a bit more meaningful.
> 
>       my %mapping = @mapping;
>         my @valid_params;
>       for (my $i = 0; $i < @mapping; $i += 2) {
>               push @valid_params, $mapping[$i];
>       }
> 
> But aside from that, I very much prefer your version that loops
> over what _we_ define (i.e. @valid_params), rather than what the
> user happens to throw at us (i.e. keys %params).

So perhaps

        my %mapping = @mapping;
        my @params_keys_sorted;
        for (my $i = 0; $i < @mapping; $i += 2) {
                if (exists $params{$mapping[$i]}) {
                        push @params_keys_sorted, $mapping[$i];
                }
        }

This way we have only one loop over all valid parameter names,
and we wouldn't need grep nor conditional expression in map,
and map would loop over all (valid) params keys only.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git