[PATCH] t9117: test specifying full url to git svn init -T

[PATCH] t9117: test specifying full url to git svn init -T

[Adam Dinwoodie]

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

Re: [PATCH] t9117: test specifying full url to git svn init -T

[Eric Wong]

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.

[PATCH 2/1] git-svn: fix URL canonicalization during init w/ SVN 1.7+

[Eric Wong]

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

Re: [PATCH] t9117: test specifying full url to git svn init -T

[Adam Dinwoodie]

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 :)