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

[Michael G Schwern <schwern@pobox.com>]

On 2012.7.28 6:50 AM, Jonathan Nieder 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.

As it should, SVN dumped a mess on us.


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

This change is *just* about sliding in the SVN API call and seeing if git-svn
still works with SVN 1.6.  It should have no effect on SVN 1.6.  These patches
are a very slow and careful refactoring doing just one thing at a time.  Every
time I tried to do too many things at once, tests broke and I had to tease the
patch apart.

At this point in the patch series the code is not ready for canonicalization.
 Until 3/8 in the next patch series, canonicalize_url() basically does nothing
on SVN 1.6 so the code has never had to deal with the problem.  3/8 deals with
improving _canonicalize_url_ourselves() to work more like
svn_uri_canonicalize() and thus "turns on" canonicalization for SVN 1.6 and
deals with the breakage.


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

I didn't know about that.  I don't know what your SVN backwards compat
requirements are, but if that behavior goes back far enough in SVN to satisfy
you folks, then canonicalize_url() should fall back to
SVN::_Core::svn_path_canonicalize().  But try it at the end of the patch
series.  The code has to be prepared for canonicalization first.  Then how it
actually does it can be improved.


-- 
Defender of Lexical Encapsulation