Re: Bug: t5813 failing on Cygwin

[Dennis Kaarsemaker <dennis@kaarsemaker.net>]

On za, 2015-11-07 at 23:05 +0000, Ramsay Jones wrote:
> 
> On 07/11/15 21:21, Ramsay Jones wrote:
> > 
> > 
> > On 07/11/15 21:02, Dennis Kaarsemaker wrote:
> > > On za, 2015-11-07 at 19:20 +0000, Adam Dinwoodie wrote:
> > > > On Sat, Nov 07, 2015 at 01:45:27PM -0500, Jeff King wrote:
> > > > > On Sat, Nov 07, 2015 at 12:11:29PM +0000, Adam Dinwoodie
> > > > > wrote:
> > > > > 
> > > > > > Specifically, I'm seeing t5813 subtests 9-13 and 15-19
> > > > > > failing.
> > > > > > This happens
> > > > > > with a clean build straight from the Git source tree (git
> > > > > > clean 
> > > > > > -dfx && make
> > > > > > configure && ./configure && make && cd t && ./t5813-proto
> > > > > > -disable
> > > > > > -ssh.sh) as
> > > > > > well as builds using the Cygwin packaging paraphernalia.
> > > > > 
> > > > > What does the output of "./t5813-proto-disable-ssh.sh -v -i"
> > > > > show?
> > > > > 
> > > > > It seems strange that it would fail only on Cygwin; this code
> > > > > doesn't
> > > > > really use any platform-dependent features. It's also weird
> > > > > that it
> > > > > fails _only_ for ssh, and _only_ on the tests that are using
> > > > > "ssh://"
> > > > > URLs are not "host:path" syntax.
> > > > 
> > > > Ah!  I thought I'd checked that already, but looking at the
> > > > output
> > > > now I
> > > > can see what's going wrong.  Cutting down to the relevant
> > > > error:
> > > > 
> > > >     ssh: remote git-upload-pack '//home/Adam/vcs/Cygwin-Git/git
> > > > -2.6.2
> > > > -1.x86_64/build/t/trash directory.t5813-proto-disable
> > > > -ssh/remote/repo.git' fatal: '//home/Adam/vcs/Cygwin-Git/git
> > > > -2.6.2
> > > > -1.x86_64/build/t/trash directory.t5813-proto-disable
> > > > -ssh/remote/repo.git' does not appear to be a git repository
> > > > 
> > > > Note the '//' at the start of the path -- on most *nix systems
> > > > '//'
> > > > is
> > > > effectively identical to '/'.  On Cygwin, however, '//' is used
> > > > to
> > > > access Windows UNC paths: what Windows calls "\\server\share",
> > > > Cygwin
> > > > calls "//server/share".  If you replace the '//' with '/' you
> > > > get the
> > > > locatoin of the repository; but here Cygwin is looking for the
> > > > repository in a share called "Adam" on a network server called
> > > > "home"...
> > > > 
> > > > I suspect the correct fix here is to fix whatever's causing Git
> > > > to
> > > > generate a path with that '//'.  If nobody else gets to it soon
> > > > (probably on the order of a week before I'll get the chance),
> > > > I'll go
> > > > code diving and submit a patch.
> > > > 
> > > > > I tried building on Linux with the Cygwin build knobs found
> > > > > in
> > > > > config.mak.uname, but I couldn't get it to fail. I also
> > > > > wondered if
> > > > > the
> > > > > test was doing something with the shell that might not be
> > > > > portable,
> > > > > but
> > > > > I don't see anything interesting.
> > > > 
> > > > If I recall correctly, the correct interpretation of '//' isn't
> > > > defined
> > > > in POSIX, so whatever's causing that path to be generated is
> > > > the bit
> > > > that's not fully portable.  It looks as though t5813 throwing
> > > > this up
> > > > is
> > > > just a coincidence rather than it being particularly related to
> > > > the
> > > > function those tests are actually testing.
> > > 
> > > Looks like lib-proto-disable.sh's fake SSH doesn't strip double
> > > leading
> > > /'es from the path. Try this patch:
> > > 
> > > diff --git a/t/t5813-proto-disable-ssh.sh b/t/t5813-proto-disable
> > > -ssh.sh
> > > index ad877d7..a954ead 100755
> > > --- a/t/t5813-proto-disable-ssh.sh
> > > +++ b/t/t5813-proto-disable-ssh.sh
> > > @@ -14,7 +14,7 @@ test_expect_success 'setup repository to clone'
> > > '
> > >  '
> > >  
> > >  test_proto "host:path" ssh "remote:repo.git"
> > > -test_proto "ssh://" ssh "ssh://remote/$PWD/remote/repo.git"
> > > -test_proto "git+ssh://" ssh
> > > "git+ssh://remote/$PWD/remote/repo.git"
> > > +test_proto "ssh://" ssh "ssh://remote$PWD/remote/repo.git"
> > > +test_proto "git+ssh://" ssh
> > > "git+ssh://remote$PWD/remote/repo.git"
> > 
> > Heh, this looks familiar ... see, for example, commit 3a81f33c5. ;
> > -)
> 
> An alternative patch may look like this:
> 
> diff --git a/connect.c b/connect.c
> index 108f5ab..fc73cf9 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -636,6 +636,8 @@ static enum protocol parse_connect_url(const char
> *url_orig, char **ret_host,
>  	end = path; /* Need to \0 terminate host here */
>  	if (separator == ':')
>  		path++; /* path starts after ':' */
> +	if (starts_with(path, "//"))
> +		path++;
>  	if (protocol == PROTO_GIT || protocol == PROTO_SSH) {
>  		if (path[1] == '~')
>  			path++;
> 
> It seems to work, but I haven't thought about it too deeply ...
> so I don't know if there are any problems lurking. :)
> 
> I have to go now, so if somebody wants to take this up ...

Won't that break file:////server/share urls on cygwin?

-- 
Dennis Kaarsemaker
www.kaarsemaker.net