{"thread":{"id":"41712","subject":"[PATCH] t9117: test specifying full url to git svn init -T","startedAt":"2016-03-16T19:09:54Z","lastAt":"2016-03-16T22:34:18Z","messageCount":4,"participants":["Adam Dinwoodie","Eric Wong"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"280946","messageId":"20160316190954.GR29016@dinwoodie.org","threadId":"41712","inReplyTo":null,"subject":"[PATCH] t9117: test specifying full url to git svn init -T","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2016-03-16T19:09:54Z","receivedAt":"2016-03-16T19:09:54Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"According to the documentation, full URLs can be specified in the `-T`\nargument to `git svn init`. However, the canonicalization of such\narguments squashes together consecutive \"/\"s, which unsurprisingly\nbreaks http://, svn://, etc URLs. Add a failing test case to provide\nevidence of that.\n\nOn systems where Subversion provides svn_path_canonicalize but not\nsvn_dirent_canonicalize (Subversion 1.6 and earlier?), this test passes,\nas svn_path_canonicalize doesn't mangle the consecutive \"/\"s.\n\nSigned-off-by: Adam Dinwoodie \n---\n\nI think the bug here is in using perl/Git/SVN/Utils.pm's\n`canonicalize_path` on the `-T` argument. If it's available, that\nfunction calls Subversion's `svn_dirent_canonicalize`. The Subversion\ncode[0] makes it clear that this function is fine for relative and\nabsolute local paths, and for UNC paths on Windows, but it isn't\nsuitable for use on URLs.\n\n[0]: https://svn.apache.org/repos/asf/subversion/trunk/subversion/include/svn_dirent_uri.h\n\nIt occurs to me that the correct \"fix\" here may simply be to stop\nclaiming support for specifying URLs as arguments to -T, and mandate\nusers use the `git svn init $url -T $dirent` syntax instead, but I\nfigured providing the failing testcase would be a good start to that\ndiscussion regardless.\n\n t/t9117-git-svn-init-clone.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh\nindex a66f43c..2ba003d 100755\n--- a/t/t9117-git-svn-init-clone.sh\n+++ b/t/t9117-git-svn-init-clone.sh\n@@ -119,4 +119,10 @@ test_expect_success 'clone with -s/-T/-b/-t and --prefix \"\" still works' '\n rm -f warning\n '\n\n+test_expect_failure 'init with -T as a full url works' '\n+ test ! -d project &&\n+ git svn init -T \"$svnrepo\"/project/trunk project &&\n+ rm -rf project\n+ '\n+\n test_done\n--\n2.7.0\n"},{"id":"280949","messageId":"20160316193407.GA3781@dcvr.yhbt.net","threadId":"41712","inReplyTo":"20160316190954.GR29016@dinwoodie.org","subject":"Re: [PATCH] t9117: test specifying full url to git svn init -T","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2016-03-16T19:34:07Z","receivedAt":"2016-03-16T19:34:07Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Adam Dinwoodie wrote:\n> According to the documentation, full URLs can be specified in the `-T`\n> argument to `git svn init`. However, the canonicalization of such\n> arguments squashes together consecutive \"/\"s, which unsurprisingly\n> breaks http://, svn://, etc URLs. Add a failing test case to provide\n> evidence of that.\n> \n> On systems where Subversion provides svn_path_canonicalize but not\n> svn_dirent_canonicalize (Subversion 1.6 and earlier?), this test passes,\n> as svn_path_canonicalize doesn't mangle the consecutive \"/\"s.\n> \n> Signed-off-by: Adam Dinwoodie \n\nThanks, I was just working on a patch to fix this problem\nwhen I got this patch :)\n\nSigned-off-by: Eric Wong \n\n> ---\n> \n> I think the bug here is in using perl/Git/SVN/Utils.pm's\n> `canonicalize_path` on the `-T` argument. If it's available, that\n> function calls Subversion's `svn_dirent_canonicalize`. The Subversion\n> code[0] makes it clear that this function is fine for relative and\n> absolute local paths, and for UNC paths on Windows, but it isn't\n> suitable for use on URLs.\n> \n> [0]: https://svn.apache.org/repos/asf/subversion/trunk/subversion/include/svn_dirent_uri.h\n\nYep, we should be using canonicalize_url for URLs. I was able\nto reproduce this on Debian jessie (GNU/Linux), too.\n\n> It occurs to me that the correct \"fix\" here may simply be to stop\n> claiming support for specifying URLs as arguments to -T, and mandate\n> users use the `git svn init $url -T $dirent` syntax instead,\n\nNope, we should never stop supporting existing behavior without\nvery good reason and adequate deprecation warnings.\n\n> --- a/t/t9117-git-svn-init-clone.sh\n> +++ b/t/t9117-git-svn-init-clone.sh\n> @@ -119,4 +119,10 @@ test_expect_success 'clone with -s/-T/-b/-t and --prefix \"\" still works' '\n> rm -f warning\n> '\n\nFor future reference, your mail editor is expanding tabs to\nspaces and munging your patches. mutt won't do that itself\n(at least not with my config), so I guess it's your editor.\n\nManually fixing up the whitespaces damage on my end.\n"},{"id":"280956","messageId":"20160316201408.GA22261@dcvr.yhbt.net","threadId":"41712","inReplyTo":"20160316190954.GR29016@dinwoodie.org","subject":"[PATCH 2/1] git-svn: fix URL canonicalization during init w/ SVN 1.7+","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2016-03-16T20:14:08Z","receivedAt":"2016-03-16T20:14:08Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"URL canonicalization when full URLs are passed became broken\nwhen using SVN::_Core::svn_dirent_canonicalize under SVN 1.7.\n\nEnsure we canonicalize paths and URLs with appropriate functions\nfor each type from now on as the path/URL-agnostic\nSVN::_Core::svn_path_canonicalize function is deprecated in SVN.\n\nTested with the following commands:\n\n git svn init -T svn://svn.code.sf.net/p/squirrelmail/code/trunk\n git svn init -b svn://svn.code.sf.net/p/squirrelmail/code/branches\n\nReported-by: Adam Dinwoodie \n http://mid.gmane.org/20160315162344.GM29016@dinwoodie.org\nSigned-off-by: Eric Wong \n---\n git-svn.perl | 14 ++++++++------\n t/t9117-git-svn-init-clone.sh | 2 +-\n 2 files changed, 9 insertions(+), 7 deletions(-)\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex fa5f253..05eced0 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -1745,11 +1745,12 @@ sub post_fetch_checkout {\n \n sub complete_svn_url {\n \tmy ($url, $path) = @_;\n-\t$path = canonicalize_path($path);\n \n-\t# If the path is not a URL...\n-\tif ($path !~ m#^[a-z\\+]+://#) {\n-\t\tif (!defined $url || $url !~ m#^[a-z\\+]+://#) {\n+\tif ($path =~ m#^[a-z\\+]+://#i) { # path is a URL\n+\t\t$path = canonicalize_url($path);\n+\t} else {\n+\t\t$path = canonicalize_path($path);\n+\t\tif (!defined $url || $url !~ m#^[a-z\\+]+://#i) {\n \t\t\tfatal(\"E: '$path' is not a complete URL \",\n \t\t\t \"and a separate URL is not specified\");\n \t\t}\n@@ -1764,11 +1765,12 @@ sub complete_url_ls_init {\n \t\tprint STDERR \"W: $switch not specified\\n\";\n \t\treturn;\n \t}\n-\t$repo_path = canonicalize_path($repo_path);\n-\tif ($repo_path =~ m#^[a-z\\+]+://#) {\n+\tif ($repo_path =~ m#^[a-z\\+]+://#i) {\n+\t\t$repo_path = canonicalize_url($repo_path);\n \t\t$ra = Git::SVN::Ra->new($repo_path);\n \t\t$repo_path = '';\n \t} else {\n+\t\t$repo_path = canonicalize_path($repo_path);\n \t\t$repo_path =~ s#^/+##;\n \t\tunless ($ra) {\n \t\t\tfatal(\"E: '$repo_path' is not a complete URL \",\ndiff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh\nindex 2ba003d..69a6750 100755\n--- a/t/t9117-git-svn-init-clone.sh\n+++ b/t/t9117-git-svn-init-clone.sh\n@@ -119,7 +119,7 @@ test_expect_success 'clone with -s/-T/-b/-t and --prefix \"\" still works' '\n \trm -f warning\n \t'\n \n-test_expect_failure 'init with -T as a full url works' '\n+test_expect_success 'init with -T as a full url works' '\n \ttest ! -d project &&\n \tgit svn init -T \"$svnrepo\"/project/trunk project &&\n \trm -rf project\n-- \nEW\n"},{"id":"280983","messageId":"20160316223418.GS29016@dinwoodie.org","threadId":"41712","inReplyTo":"20160316193407.GA3781@dcvr.yhbt.net","subject":"Re: [PATCH] t9117: test specifying full url to git svn init -T","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2016-03-16T22:34:18Z","receivedAt":"2016-03-16T22:34:18Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"On Wed, Mar 16, 2016 at 07:34:07PM +0000, Eric Wong wrote:\n> Adam Dinwoodie wrote:\n> > According to the documentation, full URLs can be specified in the `-T`\n> > argument to `git svn init`. However, the canonicalization of such\n> > arguments squashes together consecutive \"/\"s, which unsurprisingly\n> > breaks http://, svn://, etc URLs. Add a failing test case to provide\n> > evidence of that.\n> > \n> > On systems where Subversion provides svn_path_canonicalize but not\n> > svn_dirent_canonicalize (Subversion 1.6 and earlier?), this test passes,\n> > as svn_path_canonicalize doesn't mangle the consecutive \"/\"s.\n> > \n> > Signed-off-by: Adam Dinwoodie \n> \n> Thanks, I was just working on a patch to fix this problem\n> when I got this patch :)\n\nAwesome, thank you!\n\n> Signed-off-by: Eric Wong \n> \n> > ---\n> > \n> > I think the bug here is in using perl/Git/SVN/Utils.pm's\n> > `canonicalize_path` on the `-T` argument. If it's available, that\n> > function calls Subversion's `svn_dirent_canonicalize`. The Subversion\n> > code[0] makes it clear that this function is fine for relative and\n> > absolute local paths, and for UNC paths on Windows, but it isn't\n> > suitable for use on URLs.\n> > \n> > [0]: https://svn.apache.org/repos/asf/subversion/trunk/subversion/include/svn_dirent_uri.h\n> \n> Yep, we should be using canonicalize_url for URLs. I was able\n> to reproduce this on Debian jessie (GNU/Linux), too.\n\nGood to know. I didn't get as far as digging through to see what a code\nfix would look like or how much existing code there was that could be\nused.\n\nI'd initially thought it was a Cygwin-specific problem on the grounds\nthat my Linux box wasn't reproducing the problem, but that's evidently\nactually my Linux box having Subversion 1.6 installed, while Cygwin has\n1.9.\n\n> > It occurs to me that the correct \"fix\" here may simply be to stop\n> > claiming support for specifying URLs as arguments to -T, and mandate\n> > users use the `git svn init $url -T $dirent` syntax instead,\n> \n> Nope, we should never stop supporting existing behavior without\n> very good reason and adequate deprecation warnings.\n\nFair enough. I was thinking here that the existing behaviour seems\nlittle-used, given the bug has existed for some time, although I guess\nthere may still be a significant subset of users who do have it working\nthanks to using an old Subversion release.\n\n> > --- a/t/t9117-git-svn-init-clone.sh\n> > +++ b/t/t9117-git-svn-init-clone.sh\n> > @@ -119,4 +119,10 @@ test_expect_success 'clone with -s/-T/-b/-t and --prefix \"\" still works' '\n> > rm -f warning\n> > '\n> \n> For future reference, your mail editor is expanding tabs to\n> spaces and munging your patches. mutt won't do that itself\n> (at least not with my config), so I guess it's your editor.\n> \n> Manually fixing up the whitespaces damage on my end.\n\nMea culpa. I copy-pasted the patch into Vim out of laziness, and\ncompletely forgot that'd mangle the tabs. I'll try to remember for next\ntime :)\n"}]}