From: Junio C Hamano <junkio@cox.net>
Date: Thu, 17 Aug 2006 01:59:15 GMT
Subject: Re: [PATCH] gitweb: continue consolidation of URL generation.
Message-ID: <7voduk15sc.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <11557673213372-git-send-email-tali@admingilde.org>

Martin Waitz <tali@admingilde.org> writes:

> Further use href() instead of URL generation by string concatenation.
> Almost all functions are converted now.
>
> Signed-off-by: Martin Waitz <tali@admingilde.org>
> ---
>  gitweb/gitweb.perl |   68 +++++++++++++++++++++++++++-------------------------
>  1 files changed, 35 insertions(+), 33 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 37a6284..72e687e 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -204,7 +204,9 @@ sub href(%) {
>  
>  	my $href = "$my_uri?";
>  	$href .= esc_param( join(";",
> -		map { "$mapping{$_}=$params{$_}" } keys %params
> +		map {
> +			"$mapping{$_}=$params{$_}" if $params{$_}
> +		} keys %params
>  	) );
>  
>  	return $href;

This is not about "further use href()".

I think "if $params{$_}" is unsafe here.  Can you guarantee that
we will never want to pass 0 or an empty string as an parameter
value both in the current and the future code?

I think it needs to be "if defined $params{$_}", at least.

> @@ -1252,7 +1254,7 @@ sub git_difftree_body {
>  			      "<td class=\"link\">" .
>  				$cgi->a({-href => href(action=>"blob", hash=>$to_id, hash_base=>$hash, file_name=>$file)}, "blob");
>  			if ($to_id ne $from_id) { # modified
> -				print $cgi->a({-href => href(action=>"blobdiff", hash=>$to_id, hash_parent=>$from_id, hash_base=>$hash, file_name=>$file)}, "diff");
> +				print " | " . $cgi->a({-href => href(action=>"blobdiff", hash=>$to_id, hash_parent=>$from_id, hash_base=>$hash, file_name=>$file)}, "diff");
>  			}
>  			print " | " . $cgi->a({-href => href(action=>"history", hash_base=>$hash, file_name=>$file)}, "history") . "\n";
>  			print "</td>\n";

Well spotted.