Re: Bug: t5813 failing on Cygwin

From: Ramsay Jones <ramsay@ramsayjones.plus.com>
Date: Nov 8, 2015, 01:46 UTC
Message-ID: <563EA972.9000209@ramsayjones.plus.com>
In-Reply-To: <1446939168.16957.11.camel@kaarsemaker.net>

On 07/11/15 23:32, Dennis Kaarsemaker wrote:

Show 135 quoted lines

> 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