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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html