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

From: Adam Dinwoodie <adam@dinwoodie.org>
Date: Mar 16, 2016, 22:34 UTC
Message-ID: <20160316223418.GS29016@dinwoodie.org>
In-Reply-To: <20160316193407.GA3781@dcvr.yhbt.net>

On Wed, Mar 16, 2016 at 07:34:07PM +0000, Eric Wong wrote:

Show 15 quoted lines

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

Show 15 quoted lines

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

Show 6 quoted lines

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

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

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