On Wed, Mar 16, 2016 at 07:34:07PM +0000, Eric Wong wrote: > Adam Dinwoodie <adam@xxxxxxxxxxxxx> wrote: > > According to the documentation, full URLs can be specified in the `-T` > > argument to `git svn init`. However, the canonicalization of such > > arguments squashes together consecutive "/"s, which unsurprisingly > > breaks http://, svn://, etc URLs. Add a failing test case to provide > > evidence of that. > > > > On systems where Subversion provides svn_path_canonicalize but not > > svn_dirent_canonicalize (Subversion 1.6 and earlier?), this test passes, > > as svn_path_canonicalize doesn't mangle the consecutive "/"s. > > > > Signed-off-by: Adam Dinwoodie <adam@xxxxxxxxxxxxx> > > Thanks, I was just working on a patch to fix this problem > when I got this patch :) Awesome, thank you! > Signed-off-by: Eric Wong <normalperson@xxxxxxxx> > > > --- > > > > I think the bug here is in using perl/Git/SVN/Utils.pm's > > `canonicalize_path` on the `-T` argument. If it's available, that > > function calls Subversion's `svn_dirent_canonicalize`. The Subversion > > code[0] makes it clear that this function is fine for relative and > > absolute local paths, and for UNC paths on Windows, but it isn't > > suitable for use on URLs. > > > > [0]: https://svn.apache.org/repos/asf/subversion/trunk/subversion/include/svn_dirent_uri.h > > Yep, we should be using canonicalize_url for URLs. I was able > to reproduce this on Debian jessie (GNU/Linux), too. Good to know. I didn't get as far as digging through to see what a code fix would look like or how much existing code there was that could be used. I'd initially thought it was a Cygwin-specific problem on the grounds that my Linux box wasn't reproducing the problem, but that's evidently actually my Linux box having Subversion 1.6 installed, while Cygwin has 1.9. > > It occurs to me that the correct "fix" here may simply be to stop > > claiming support for specifying URLs as arguments to -T, and mandate > > users use the `git svn init $url -T $dirent` syntax instead, > > Nope, we should never stop supporting existing behavior without > very good reason and adequate deprecation warnings. Fair enough. I was thinking here that the existing behaviour seems little-used, given the bug has existed for some time, although I guess there may still be a significant subset of users who do have it working thanks to using an old Subversion release. > > --- a/t/t9117-git-svn-init-clone.sh > > +++ b/t/t9117-git-svn-init-clone.sh > > @@ -119,4 +119,10 @@ test_expect_success 'clone with -s/-T/-b/-t and --prefix "" still works' ' > > rm -f warning > > ' > > For future reference, your mail editor is expanding tabs to > spaces and munging your patches. mutt won't do that itself > (at least not with my config), so I guess it's your editor. > > Manually fixing up the whitespaces damage on my end. Mea culpa. I copy-pasted the patch into Vim out of laziness, and completely forgot that'd mangle the tabs. I'll try to remember for next time :) -- 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