Re: [PATCH 2/7] Change canonicalize_url() to use the SVN 1.7 API when available.
From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:54:22
Hi, Michael G. Schwern wrote:
quoted hunk ↗ jump to hunk
--- a/perl/Git/SVN/Utils.pm +++ b/perl/Git/SVN/Utils.pm
[...]
quoted hunk ↗ jump to hunk
@@ -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