Re: [PATCH 6/7] Switch path canonicalization to use the SVN API.

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

On 2012.7.28 6:55 AM, Jonathan Nieder wrote:
> Michael G. Schwern wrote:
>> --- a/perl/Git/SVN/Utils.pm
>> +++ b/perl/Git/SVN/Utils.pm
>> @@ -86,6 +86,27 @@ sub _collapse_dotdot {
>>  
>>  
>>  sub canonicalize_path {
>> +	my $path = shift;
>> +
>> +	# The 1.7 way to do it
>> +	if ( defined &SVN::_Core::svn_dirent_canonicalize ) {
>> +		$path = _collapse_dotdot($path);
>> +		return SVN::_Core::svn_dirent_canonicalize($path);
>> +	}
>> +	# The 1.6 way to do it
>> +	elsif ( defined &SVN::_Core::svn_path_canonicalize ) {
>> +		$path = _collapse_dotdot($path);
>> +		return SVN::_Core::svn_path_canonicalize($path);
>> +	}
>> +	# No SVN API canonicalization is available, do it ourselves
>> +	else {
> 
> When would this "else" case trip?

When svn_path_canonicalize() does not exist in the SVN API, presumably because
their SVN is too old.


> Would it be safe to make it
> return an error message, or even to do something like the following?

I don't know what your SVN backwards compat requirements are, or when
svn_path_canonicalize() appears in the API, so I left it as is.  git-svn's
home rolled path canonicalization worked and its no work to leave it working.
 No reason to break it IMO.


> 	sub canonicalize_path {
> 		my $path = shift;
> 		$path = _collapse_dotdot($path);
> 
> 		# Subversion 1.7 split svn_path_canonicalize() into
> 		# svn_dirent_canonicalize() and svn_uri_canonicalize().
> 		if (!defined &SVN::_Core::svn_dirent_canonicalize) {
> 			return SVN::_Core::svn_path_canonicalize($path);
> 		}
> 
> 		return SVN::_Core::svn_dirent_canonicalize($path);
> 	}

As a side note...
"If they don't have Mars bar, get me a Twix.  Else get me a Mars bar."
"If they have a Mars bar, get me one.  Else get me a Twix."


-- 
Look at me talking when there's science to do.
When I look out there it makes me glad I'm not you.
I've experiments to be run.
There is research to be done
On the people who are still alive.
    -- Jonathan Coulton, "Still Alive"