Bug: t5813 failing on Cygwin

Bug: t5813 failing on Cygwin

[Adam Dinwoodie]

In the process of pulling up the Cygwin Git release from v2.5.3, I've 
discovered t5813 is failing, and appears to have been failing since it 
was first introduced in a5adace.

I've not yet done any significant digging into the problem myself; I'm 
reporting here now in case in the hope someone else will have a better 
idea what's going on.

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.

Adam

Re: Bug: t5813 failing on Cygwin

[Jeff King]

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.

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.

-Peff

Re: Bug: t5813 failing on Cygwin

[Adam Dinwoodie]

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.

Adam

Re: Bug: t5813 failing on Cygwin

[Dennis Kaarsemaker]

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


-- 
Dennis Kaarsemaker
www.kaarsemaker.net

Re: Bug: t5813 failing on Cygwin

[Ramsay Jones]

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

ATB,
Ramsay Jones

Re: Bug: t5813 failing on Cygwin

[Ramsay Jones]

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

ATB,
Ramsay Jones

Re: Bug: t5813 failing on Cygwin

[Adam Dinwoodie]

On Sat, Nov 07, 2015 at 10:02:45PM +0100, 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"
>  
>  test_done
>  

Confirmed this patch has the test running successfully for me.

Adam

Re: Bug: t5813 failing on Cygwin

[Dennis Kaarsemaker]

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

Re: Bug: t5813 failing on Cygwin

[Ramsay Jones]

On 07/11/15 23:32, Dennis Kaarsemaker wrote:
> 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?

Hmm, yes ... do they work? (I'm not in a position to test them)
ie. does 'git clone [file://]//myserver/myshare/myrepo.git' work now
on cygwin? How about git for windows?

I suppose we could restrict the stripping to git and ssh protocols
like so:

diff --git a/connect.c b/connect.c
index 108f5ab..62e89aa 100644
--- a/connect.c
+++ b/connect.c
@@ -637,6 +637,8 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host,
 	if (separator == ':')
 		path++; /* path starts after ':' */
 	if (protocol == PROTO_GIT || protocol == PROTO_SSH) {
+		while (starts_with(path, "//"))
+			path++;
 		if (path[1] == '~')
 			path++;
 	}

... where I've added a loop to skip multiple /'s (may not be
necessary).

Again, it seems to work ... :-D

ATB,
Ramsay Jones

Re: Bug: t5813 failing on Cygwin

[Jeff King]

On Sat, Nov 07, 2015 at 10:02:45PM +0100, Dennis Kaarsemaker wrote:

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

Good catch. It's a shame that the resulting URL becomes a little less
readable, but given that the damage is limited to this one test, I think
we can live with it. Working around it in the fake ssh script would
probably end up unnecessarily complex.

We may want to have git itself remove extra slashes from URLs, as Ramsay
suggested elsewhere, but I would worry about regressions. Let's solve
this test portability problem by itself, and we can consider the other
as a separate topic if anybody wants to pursue it.

Dennis, can you roll up a full patch with a commit message?

-Peff

Re: Bug: t5813 failing on Cygwin

[Torsten Bögershausen]

>>
>> 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?
> 
The official way to specify UNC names looks like this:
file://<host>/<path>
(Or file://server/share/morepath to follow your example)
#http://www.faqs.org/rfcs/rfc1738.html

But unfortunately Git doesn't support it.
I can put it on the to-do list...

But using "file:////" this is not an official syntax,
and should (in theory) not be used by any user.
Does anybody use it?

Or do people use simply "git clone //server/share/" ?
 

[PATCH] t5813: avoid creating urls that break on cygwin

[Dennis Kaarsemaker]

The fake ssh used by this test simply strips ssh://host from the url,
leaving paths behind that start with //, which cygwin interprets as UNC
paths, causing the test to fail.

We may want to actually fix this in git itself, making it remove extra
slashes from urls before feeding them to transports or helpers, but
that's for another topic as it could cause regressions.

Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
---
 t/t5813-proto-disable-ssh.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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"
 
 test_done
-- 
2.6.3-495-gf0a7f49


-- 
Dennis Kaarsemaker <dennis@kaarsemaker.net>
http://twitter.com/seveas

Re: Bug: t5813 failing on Cygwin

[Ramsay Jones]

On 08/11/15 05:10, Jeff King wrote:
> On Sat, Nov 07, 2015 at 10:02:45PM +0100, Dennis Kaarsemaker wrote:
> 
>> 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"
>>  
>>  test_done
> 
> Good catch. It's a shame that the resulting URL becomes a little less
> readable, but given that the damage is limited to this one test, I think
> we can live with it. Working around it in the fake ssh script would
> probably end up unnecessarily complex.
> 
> We may want to have git itself remove extra slashes from URLs, as Ramsay
> suggested elsewhere, but I would worry about regressions. Let's solve
> this test portability problem by itself, and we can consider the other
> as a separate topic if anybody wants to pursue it.

Yep, this is essentially what I decided to do in a similar situation
back in 2011 (see commit 3a81f33c5).

Until I looked into that bug, I hadn't given any thought to whether
cygwin (let alone git) supported the UNC notation. At that time, I did
spend a little time trying to determine if it worked, but for some
reason I just couldn't get a network share configured properly.
It kept on giving me 'permission denied' errors - even using windows
only commands. :(

Since I have no need for network shares on cygwin, I didn't try too
hard ... I will leave it to people more motivated than me! ;-)

ATB,
Ramsay Jones

Re: [PATCH] t5813: avoid creating urls that break on cygwin

[Jeff King]

On Sun, Nov 08, 2015 at 10:54:04AM +0100, Dennis Kaarsemaker wrote:

> The fake ssh used by this test simply strips ssh://host from the url,
> leaving paths behind that start with //, which cygwin interprets as UNC
> paths, causing the test to fail.

I found the first sentence a little misleading. It is git itself that
strips the URL, isn't it? The problem is that we are feeding a URL with
a bogus path, which the fake ssh then tries to access (but in a way that
happens to work on Unix systems).

-Peff

[PATCH] t5813: avoid creating urls that break on cygwin

[Dennis Kaarsemaker]

When passed an ssh:// url, git strips ssh://host from the url but does
not remove leading slashes from the path. So when this test used
ssh://remote//path/to/pwd, the path accessed by our fake SSH is
//path/to/pwd, which cygwin interprets as a UNC path, causing the test
to fail.

We may want to actually fix this in git itself, making it remove extra
slashes from urls before feeding them to transports or helpers, but
that's for another topic as it could cause regressions.

Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
---

You're right of course. Somehow I remembered that the fake ssh was doing the
stripping, but didn't check that when writing the commit message. How about
this version?

 t/t5813-proto-disable-ssh.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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"
 
 test_done
-- 
2.6.3-495-gf0a7f49


-- 
Dennis Kaarsemaker <dennis@kaarsemaker.net>
http://twitter.com/seveas

Re: [PATCH] t5813: avoid creating urls that break on cygwin

[Jeff King]

On Mon, Nov 09, 2015 at 06:49:35PM +0100, Dennis Kaarsemaker wrote:

> When passed an ssh:// url, git strips ssh://host from the url but does
> not remove leading slashes from the path. So when this test used
> ssh://remote//path/to/pwd, the path accessed by our fake SSH is
> //path/to/pwd, which cygwin interprets as a UNC path, causing the test
> to fail.
> 
> We may want to actually fix this in git itself, making it remove extra
> slashes from urls before feeding them to transports or helpers, but
> that's for another topic as it could cause regressions.
> 
> Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
> ---
> 
> You're right of course. Somehow I remembered that the fake ssh was doing the
> stripping, but didn't check that when writing the commit message. How about
> this version?

Looks good. Thanks.

-Peff