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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jul 28, 2012, 13:50 UTC
Message-ID: <20120728135018.GB9715@burratino>
In-Reply-To: <1343468312-72024-3-git-send-email-schwern@pobox.com>

Hi,

Michael G. Schwern wrote:

> --- a/perl/Git/SVN/Utils.pm
> +++ b/perl/Git/SVN/Utils.pm

[...]

Show 20 quoted lines

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