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

From: Junio C Hamano <junkio@cox.net>
Date: Aug 22, 2006, 08:55 UTC
Message-ID: <7vodudno88.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <eceekl$b0l$2@sea.gmane.org>

Jakub Narebski <jnareb@gmail.com> writes:

Show 13 quoted lines

> 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.

At that point I suspect that loop can do everything your map {} did, and would look simpler (albeit perhaps a little bit less Perlish) like this:

sub href(%) {
	my %params = @_;
	$params{"project"} ||= $project;
        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 @result = (); 
        for (my $i = 0; $i < @mapping; $i += 2) {
        	my ($name, $symbol) = ($mapping[$i], $mapping[$i+1]);
		if (defined $params{$name}) {
			push @result, "$symbol=$params{$name}";
		}
	}
        return "$my_uri?" . esc_param(join(';', @result));
}

By the way, is it really what we want to do to call esc_param() on joined result?

Maybe I am not reading this code correctly, but this feels quite counter-intuitive. Actually, it feels downright wrong.

Semicolons and equals are used as separators between key-value pairs (i.e. syntactic elements) so if we have a value $params{$name} that happens to contain a ';' or '=' character I suspect we would want to quote that but not the one we use before the value or between tuples. Otherwise, how is a search text that is "a = b;" encoded in the resulting href?

So the last part of the above should perhaps read:

	my @result = (); 
        for (my $i = 0; $i < @mapping; $i += 2) {
        	my ($name, $symbol) = ($mapping[$i], $mapping[$i+1]);
		if (defined $params{$name}) {
			push @result, join('=', esc_param($symbol),
						esc_param($params{$name}));
		}
	}
        return "$my_uri?" . join(';', @result);

We would also need to fix esc_param to quote ';' and '=' as well, which does not seem to quote them.