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

Reply via email to