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

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

[Jakub Narebski]

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?

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

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

[Jakub Narebski]

Ooops. Sorry for duplicated post.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

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

[Junio C Hamano]

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;

So we perhaps would want to say:

	if (defined $params{$_} && exists $mapping{$_})

instead there?

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

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

[Jakub Narebski]

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

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

[Junio C Hamano]

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

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

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

[Jakub Narebski]

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

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

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

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

[Jakub Narebski]

Junio C Hamano wrote:

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

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

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

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

[Jakub Narebski]

Junio C Hamano wrote:

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

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
Warsaw, Poland
ShadeHawk on #git

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> 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

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.

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

[Jakub Narebski]

Junio C Hamano wrote:

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

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...
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

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

[Randal L. Schwartz]

>>>>> "Junio" == Junio C Hamano <junkio@cox.net> writes:

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 - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Perl/Unix/security consulting, Technical writing, Comedy, etc. etc.
See PerlTraining.Stonehenge.com for onsite and open-enrollment Perl training!

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

[Jakub Narebski]

Randal L. Schwartz wrote:

>>>>>> "Junio" == Junio C Hamano <junkio@cox.net> writes:
> 
> 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. :)

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)'.
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

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

[Jakub Narebski]

<opublikowany i wysłany>

Jakub Narebski wrote:

> Randal L. Schwartz wrote:
> 
>>>>>>> "Junio" == Junio C Hamano <junkio@cox.net> writes:
>> 
>> 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. :)
> 
> 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)'.

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
Warsaw, Poland
ShadeHawk on #git

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

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

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

[PATCH 1/2] gitweb: Drop the href() params which keys are not in %mapping

[Jakub Narebski]

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 063735d..89ef5e7 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -278,7 +278,7 @@ sub href(%) {
 	my $href = "$my_uri?";
 	$href .= esc_param( join(";",
 		map {
-			"$mapping{$_}=$params{$_}" if defined $params{$_}
+			defined $params{$_} ? "$mapping{$_}=$params{$_}" : ()
 		} keys %params
 	) );
 
-- 
1.4.1.1

[PATCH 2/2] gitweb: Sort CGI parameters returned by href()

[Jakub Narebski]

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 89ef5e7..da0630e 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -260,7 +260,9 @@ ## =====================================
 ## action links
 
 sub href(%) {
-	my %mapping = (
+	my %params = @_;
+
+	my @mapping = (
 		action => "a",
 		project => "p",
 		file_name => "f",
@@ -271,18 +273,18 @@ sub href(%) {
 		page => "pg",
 		searchtext => "s",
 	);
+	my %mapping = @mapping;
 
-	my %params = @_;
 	$params{"project"} ||= $project;
 
-	my $href = "$my_uri?";
-	$href .= esc_param( join(";",
-		map {
-			defined $params{$_} ? "$mapping{$_}=$params{$_}" : ()
-		} keys %params
-	) );
-
-	return $href;
+	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 . "=" . esc_param($params{$name});
+		}
+	}
+	return "$my_uri?" . join(';', @result);
 }
 
 
-- 
1.4.1.1

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

[Jakub Narebski]

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?

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git