* [PATCH] t9117: test specifying full url to git svn init -T @ 2016-03-16 19:09 Adam Dinwoodie 2016-03-16 19:34 ` Eric Wong 2016-03-16 20:14 ` [PATCH 2/1] git-svn: fix URL canonicalization during init w/ SVN 1.7+ Eric Wong 0 siblings, 2 replies; 4+ messages in thread From: Adam Dinwoodie @ 2016-03-16 19:09 UTC (permalink / raw) To: git; +Cc: Eric Wong, Michael G. Schwern 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@dinwoodie.org> --- 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 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, but I figured providing the failing testcase would be a good start to that discussion regardless. t/t9117-git-svn-init-clone.sh | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh index a66f43c..2ba003d 100755 --- 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 ' +test_expect_failure 'init with -T as a full url works' ' + test ! -d project && + git svn init -T "$svnrepo"/project/trunk project && + rm -rf project + ' + test_done -- 2.7.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] t9117: test specifying full url to git svn init -T 2016-03-16 19:09 [PATCH] t9117: test specifying full url to git svn init -T Adam Dinwoodie @ 2016-03-16 19:34 ` Eric Wong 2016-03-16 22:34 ` Adam Dinwoodie 2016-03-16 20:14 ` [PATCH 2/1] git-svn: fix URL canonicalization during init w/ SVN 1.7+ Eric Wong 1 sibling, 1 reply; 4+ messages in thread From: Eric Wong @ 2016-03-16 19:34 UTC (permalink / raw) To: Adam Dinwoodie; +Cc: git, Michael G. Schwern Adam Dinwoodie <adam@dinwoodie.org> 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@dinwoodie.org> Thanks, I was just working on a patch to fix this problem when I got this patch :) Signed-off-by: Eric Wong <normalperson@yhbt.net> > --- > > 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. > 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. > --- 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] t9117: test specifying full url to git svn init -T 2016-03-16 19:34 ` Eric Wong @ 2016-03-16 22:34 ` Adam Dinwoodie 0 siblings, 0 replies; 4+ messages in thread From: Adam Dinwoodie @ 2016-03-16 22:34 UTC (permalink / raw) To: Eric Wong; +Cc: git, Michael G. Schwern On Wed, Mar 16, 2016 at 07:34:07PM +0000, Eric Wong wrote: > Adam Dinwoodie <adam@dinwoodie.org> 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@dinwoodie.org> > > 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@yhbt.net> > > > --- > > > > 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 :) ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 2/1] git-svn: fix URL canonicalization during init w/ SVN 1.7+ 2016-03-16 19:09 [PATCH] t9117: test specifying full url to git svn init -T Adam Dinwoodie 2016-03-16 19:34 ` Eric Wong @ 2016-03-16 20:14 ` Eric Wong 1 sibling, 0 replies; 4+ messages in thread From: Eric Wong @ 2016-03-16 20:14 UTC (permalink / raw) To: Adam Dinwoodie; +Cc: git, Michael G. Schwern URL canonicalization when full URLs are passed became broken when using SVN::_Core::svn_dirent_canonicalize under SVN 1.7. Ensure we canonicalize paths and URLs with appropriate functions for each type from now on as the path/URL-agnostic SVN::_Core::svn_path_canonicalize function is deprecated in SVN. Tested with the following commands: git svn init -T svn://svn.code.sf.net/p/squirrelmail/code/trunk git svn init -b svn://svn.code.sf.net/p/squirrelmail/code/branches Reported-by: Adam Dinwoodie <adam@dinwoodie.org> http://mid.gmane.org/20160315162344.GM29016@dinwoodie.org Signed-off-by: Eric Wong <normalperson@yhbt.net> --- git-svn.perl | 14 ++++++++------ t/t9117-git-svn-init-clone.sh | 2 +- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/git-svn.perl b/git-svn.perl index fa5f253..05eced0 100755 --- a/git-svn.perl +++ b/git-svn.perl @@ -1745,11 +1745,12 @@ sub post_fetch_checkout { sub complete_svn_url { my ($url, $path) = @_; - $path = canonicalize_path($path); - # If the path is not a URL... - if ($path !~ m#^[a-z\+]+://#) { - if (!defined $url || $url !~ m#^[a-z\+]+://#) { + if ($path =~ m#^[a-z\+]+://#i) { # path is a URL + $path = canonicalize_url($path); + } else { + $path = canonicalize_path($path); + if (!defined $url || $url !~ m#^[a-z\+]+://#i) { fatal("E: '$path' is not a complete URL ", "and a separate URL is not specified"); } @@ -1764,11 +1765,12 @@ sub complete_url_ls_init { print STDERR "W: $switch not specified\n"; return; } - $repo_path = canonicalize_path($repo_path); - if ($repo_path =~ m#^[a-z\+]+://#) { + if ($repo_path =~ m#^[a-z\+]+://#i) { + $repo_path = canonicalize_url($repo_path); $ra = Git::SVN::Ra->new($repo_path); $repo_path = ''; } else { + $repo_path = canonicalize_path($repo_path); $repo_path =~ s#^/+##; unless ($ra) { fatal("E: '$repo_path' is not a complete URL ", diff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh index 2ba003d..69a6750 100755 --- a/t/t9117-git-svn-init-clone.sh +++ b/t/t9117-git-svn-init-clone.sh @@ -119,7 +119,7 @@ test_expect_success 'clone with -s/-T/-b/-t and --prefix "" still works' ' rm -f warning ' -test_expect_failure 'init with -T as a full url works' ' +test_expect_success 'init with -T as a full url works' ' test ! -d project && git svn init -T "$svnrepo"/project/trunk project && rm -rf project -- EW ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-03-16 22:34 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-03-16 19:09 [PATCH] t9117: test specifying full url to git svn init -T Adam Dinwoodie 2016-03-16 19:34 ` Eric Wong 2016-03-16 22:34 ` Adam Dinwoodie 2016-03-16 20:14 ` [PATCH 2/1] git-svn: fix URL canonicalization during init w/ SVN 1.7+ Eric Wong
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.