Re: [PATCH 2/7] Change canonicalize_url() to use the SVN 1.7 API when available.

On 2012.7.28 12:30 PM, Jonathan Nieder wrote:

quoted

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

svn_path_canonicalize() has been usable for this kind of thing since
SVN 1.1, possibly earlier.

Great!  Then _canonicalize_url_ourselves() can probably be replaced with that.
 Just take my advice and do it after 1.7 is working and the code is ready for
canonicalization.

quoted

                                     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.

Since this part of the series is not tested with SVN 1.7, this is
basically adding dead code, right?  That could be avoided by
reordering the changes to keep "canonicalize_url" as-is until later in
the series when the switchover is safe.

I would suggest that worrying whether a few lines of code are introduced now
or 10 patches later in the same branch which is all going to be merged in one
go (and retesting the patches after it) is not the most important thing.  The
code needs humans looking over it and deciding if canonicalizations were
missed or applied inappropriately.  Or hey, work on that path and url object
idea that makes a lot of real code mess go away.


-- 
ROCKS FALL! EVERYONE DIES!
	http://www.somethingpositive.net/sp05032002.shtml