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

Eric Wong [off-list ref] wrote:

Michael G Schwern [off-list ref] wrote:

quoted

On 2012.7.28 6:55 AM, Jonathan Nieder wrote:

quoted

Michael G. Schwern wrote:

quoted

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

svn_path_canonicalize() may be accessible in some versions of SVN,
but it'll return undef.

I'm squashing the change below to have it fall back to
_canonicalize_path_ourselves in the case svn_path_canonicalize()
is present but unusable.

quoted

quoted

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.

I agree there's no reason to break something on older SVN.

git-svn should work with whatever SVN is in CentOS 5.x and similar
distros (SVN 1.4.2).  As long as an active "long-term" distro supports
a version of SVN, I think we should support that if it's not too
difficult.

I've tested the following on an old CentOS 5.2 chroot with SVN 1.4.2:

diff --git a/perl/Git/SVN/Utils.pm b/perl/Git/SVN/Utils.pm
index b7727db..4bb4dde 100644
--- a/perl/Git/SVN/Utils.pm
+++ b/perl/Git/SVN/Utils.pm

@@ -88,22 +88,25 @@ sub _collapse_dotdot {
 
 sub canonicalize_path {
 	my $path = shift;
+	my $rv;
 
 	# 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);
+		$rv = SVN::_Core::svn_dirent_canonicalize($path);
 	}
 	# The 1.6 way to do it
+	# This can return undef on subversion-perl-1.4.2-2.el5 (CentOS 5.2)
 	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 {
-		$path = _canonicalize_path_ourselves($path);
-		return $path;
+		$rv = SVN::_Core::svn_path_canonicalize($path);
 	}
+
+	return $rv if defined $rv;
+
+	# No SVN API canonicalization is available, or the SVN API
+	# didn't return a successful result, do it ourselves
+	return _canonicalize_path_ourselves($path);
 }
 
 

-- 
Eric Wong