Re: [PATCH 2/7] Change canonicalize_url() to use the SVN 1.7 API when available.

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Michael G. Schwern wrote:

> --- a/perl/Git/SVN/Utils.pm
> +++ b/perl/Git/SVN/Utils.pm
[...]
> @@ -100,6 +102,20 @@ API as a URL.
>  =cut
>  
>  sub canonicalize_url {
> +	my $url = shift;
> +
> +	# The 1.7 way to do it
> +	if ( defined &SVN::_Core::svn_uri_canonicalize ) {
> +		return SVN::_Core::svn_uri_canonicalize($url);
> +	}
> +	# There wasn't a 1.6 way to do it, so we do it ourself.
> +	else {
> +		return _canonicalize_url_ourselves($url);
> +	}
> +}
> +
> +
> +sub _canonicalize_url_ourselves {
>  	my ($url) = @_;
>  	$url =~ s#^([^:]+://[^/]*/)(.*)$#$1 . canonicalize_path($2)#e;

Leaves me a bit nervous.

What effect should we expect this change to have?  Is our emulation
of svn_uri_canonicalize already perfect and this change just a little
futureproofing in case svn_uri_canonicalize gets even better, or is
this a trap waiting to happen when new callers of canonicalize_url
start relying on, e.g., %-encoding of special characters?

If I am reading Subversion r873487 correctly, in ancient times,
svn_path_canonicalize() did the appropriate tweaking for URIs.  Today
its implementation is comforting:

	const char *
	svn_path_canonicalize(const char *path, apr_pool_t *pool)
	{
	  if (svn_path_is_url(path))
	    return svn_uri_canonicalize(path, pool);
	  else
	    return svn_dirent_canonicalize(path, pool);
	}

It might be easier to rely on that on pre-1.7 systems.

Thanks,
Jonathan