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

From: Eric Wong <normalperson@yhbt.net>
Date: Mar 16, 2016, 19:34 UTC
Message-ID: <20160316193407.GA3781@dcvr.yhbt.net>
In-Reply-To: <20160316190954.GR29016@dinwoodie.org>

Adam Dinwoodie <adam@dinwoodie.org> wrote:

Show 11 quoted lines

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

Show 10 quoted lines

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

Show 5 quoted lines

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