Re: [PATCH 6/7] Switch path canonicalization to use the SVN API.
On 2012.8.2 2:51 PM, Eric Wong wrote: > svn_path_canonicalize() may be accessible in some versions of SVN, > but it'll return undef. Yuck! Good catch! > I've tested the following on an old CentOS 5.2 chroot with SVN 1.4.2: Looks good to me. -- Alligator sandwich, and make it snappy! -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/7] Switch path canonicalization to use the SVN API.
Eric Wong wrote:
> Michael G Schwern wrote:
> > 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.
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.
> > > 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/7] Switch path canonicalization to use the SVN API.
Michael G Schwern wrote:
> 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.
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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/7] Switch path canonicalization to use the SVN API.
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"
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/7] Switch path canonicalization to use the SVN API.
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? Would it be safe to make it
return an error message, or even to do something like the following?
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);
}
Thanks,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
