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

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.

One solution (sweeping under the carpet) would be to use parameter key as CGI parameter name if it is not found in the %mapping, i.e.

	my $href = "$my_uri?";
	$href .= esc_param( join(";",
		map {
			if (exists $mapping{$_}) {
				"$mapping{$_}=$params{$_}"
			} else {
				"$_=$params{$_}"
			}
		} keys %params
	) );

Another solution would be to skip parameters which are not found in %mapping. Correct way to do this is:

	my $href = "$my_uri?";
	$href .= esc_param( join(";",
		map {
			"$mapping{$_}=$params{$_}"
		} grep { exists $mapping{$_} } keys %params
	) );

(we cannot put condition in map BLOCK, because map does not filter, only act on elements, so the result would be empty parameter (e.g. ";;" in generated URL), I guess without warnings).

Which solutions should be chosen? If the one is chosen, I can send the patch.

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?

Ooops. Sorry for duplicated post.

Jakub Narebski <jnareb@gmail.com> writes:

The one in "next" seems to do this (the diff is between "master" and "next"):

Junio C Hamano wrote:

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
        ) );

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 <jnareb@gmail.com> writes:

... say this, than using extra grep {}.

        print join(";", 
                map { 
                        ($_ eq lc($_)) ? "a/$_" : ()
                } ("a", "b", "C", "d")),
                "\n";

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

Junio C Hamano wrote:

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.

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 <jnareb@gmail.com> writes:

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.

Junio C Hamano wrote:

There is really no need to escape $symbol; we control this, and it is CGI param safe.

BTW. why use join? The following

        push @result, "$symbol=" . esc_param($params{$name}); 

would be simplier... we could even use

        $href .= ($i > 0? ";" : "") . "$symbol=" . esc_param($params{$name});

although probably this isn't best idea (worse readibility).

Junio C Hamano wrote:

URL for search is created and encoded by browser I think, so search for "a = b" generates

        gitweb.cgi?p=git.git&a=search&h=HEAD&s=a+%3D+b

URI (notice it uses '&' as parameter separator, not ';'), but because '=' is not on the list of valid $searchtext characters, due to

        if ($searchtext =~ m/[^a-zA-Z0-9_\.\/\-\+\:\@ ]/)

gitweb returns "403 Forbidden - Invalid search parameter".

Jakub Narebski <jnareb@gmail.com> writes:

which means href() should generate something similar when it wants to reproduce what the user queried, doesn't it? I do not think quoting after joining does, and that is why I asked that question.

Junio C Hamano wrote:

Yes, ot should... but first we would have to convert all the cases of generating URI via esc_param(...) to using href(). That means to extend href() somewhat (-absolute=>1 as special value in href() parameters, or changing href(%) to href(\%;$)?) to be able to generate absolute URI (for example for RSS/OPML).

And the case is not that inconceivable: $project can be anything, and can have ';' and '=' inside. I'm not sure about valid symbolic refs...

Usage of '&' as query parameters separator instead of ';' probably depend on the browser...

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

If you already depend on the LWP package, then the "URI" module does precisely what you're reinventing.

my $uri = URI->new("http://host/base/path") $uri->query_form(\%params); my $result = $uri->as_string;

And I'd rely on Gisle Aas's experience about constructing these things far more than the thread I've just witnessed here. :)

Randal L. Schwartz wrote:

First, I'd rather not introduce new dependency to git (and I think Junio would agree). Second, more important, we do _parameters processing_, it means renaming parameters (e.g. 'file_name' in params is 'p' in CGI URI), in the future perhaps passing project via PATH_INFO not in query string, and sorting the CGI parameters.

So it wouldn't be as easy as writing 
        $uri->query_form(\%params); 
        return $uri->as_string;
or as 
        return "$my_uri?" . esc_param(join(";", "$_=$params{$_}") keys %params)'.

<opublikowany i wysłany>

Jakub Narebski wrote:

Third, read comment to esc_param:

        # quote unsafe chars, but keep the slash, even when it's not
        # correct, but quoted slashes look too horrible in bookmarks

I assume that URI module methods do not keep slash...

Jakub Narebski <jnareb@gmail.com> writes:

I do not think so either but I do not necessarily agree with that comment ;-).

If someone would enter parameter name incorrectly, and some key of
%params is not found in %mapping hash, the parameter is now
ignored. Change introduced by Martin Waitz in commit
  756d2f064b2419fcdf9cd9c851f352e2a4f75103
tried to do that, but it left empty value and there was doubled ";;"
in returned string.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 gitweb/gitweb.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Restore pre-1c2a4f5addce479c619057c6cdc841802139982f ordering of CGI parameters.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 gitweb/gitweb.perl |   22 ++++++++++++----------
 1 files changed, 12 insertions(+), 10 deletions(-)