cygwin v2.55.0 test failures

cygwin v2.55.0 test failures

[Ramsay Jones]

Sorry for being a bit tardy, but I was somewhat unwell and, as a result,
AFK for some weeks during the last part of the v2.55.0 cycle. So, I'm
still catching up. As part of that, I found that the cygwin test-suite
fails for v2.55.0, like so:

  $ tail -13 test-out-2-55-rel
  Test Summary Report
  -------------------
  unit-tests/bin/unit-tests.exe                    (Wstat: 256 (exited 1) Tests: 249 Failed: 1)
    Failed test:  242
    Non-zero exit status: 1
  t9904-url-parse.sh                               (Wstat: 256 (exited 1) Tests: 53 Failed: 4)
    Failed tests:  39, 42-43, 47
    Non-zero exit status: 1
  Files=1047, Tests=32848, 4584 wallclock secs (29.05 usr 85.59 sys + 9679.08 cusr 14883.94 csys = 24677.67 CPU)
  Result: FAIL
  make[1]: *** [Makefile:78: prove] Error 1
  make[1]: Leaving directory '/home/ramsay/git/t'
  make: *** [Makefile:3381: test] Error 2
  $ 

Unsurprisingly, the -rc0, -rc1 and -rc2 builds show the same failures.

After a quick squint, the reason for the failure looked familiar ... ;)
Indeed, I have been aware of the reason for this failure since v2.23.1
at the end of 2019. For those who don't instantly recognize it, this was
one of a series of security releases, which mainly affected GfW. From
the release Notes:

  Git v2.23.1 Release Notes
  =========================
  
  This release merges up the fixes that appear in v2.14.6, v2.15.4,
  v2.17.3, v2.20.2 and in v2.21.1, addressing the security issues
  CVE-2019-1348, CVE-2019-1349, CVE-2019-1350, CVE-2019-1351,
  CVE-2019-1352, CVE-2019-1353, CVE-2019-1354, CVE-2019-1387, and
  CVE-2019-19604; see the release notes for those versions for details.

For example, the test-suite for v2.25.0-rc0 looks like:

  $ tail -17 test-out/test-out-2-25-rc0
  Test Summary Report
  -------------------
  t5500-fetch-pack.sh                              (Wstat: 256 Tests: 369 Failed: 12)
    Failed tests:  142, 145, 161-162, 239, 242, 258-259, 336
                  339, 355-356
    Non-zero exit status: 1
  t5580-clone-push-unc.sh                          (Wstat: 256 Tests: 6 Failed: 1)
    Failed test:  4
    Non-zero exit status: 1
  t5601-clone.sh                                   (Wstat: 256 Tests: 104 Failed: 4)
    Failed tests:  62-64, 66
    Non-zero exit status: 1
  Files=890, Tests=21098, 16304 wallclock secs ( 0.48 usr  0.73 sys + 2374.33 cusr 7438.69 csys = 9814.24 CPU)
  Result: FAIL
  make[1]: *** [Makefile:52: prove] Error 1
  make[1]: Leaving directory '/home/ramsay/git/t'
  make: *** [Makefile:2754: test] Error 2
  $ 

[t5580-clone-push-unc.sh was later renamed to t5580-unc-paths.sh]

From about that time, I had hacked up a 'fix' for cygwin, but hadn't decided
how to proceed with an actual patch. :)

So, I cherry-picked the 'fix' onto 'master' (then on v2.55.0 release). I was
a little surprised it went without problem, given how much 'git-compat-util.h'
has changed, but it just required a 'slide' from line 203 back to line 153.
This patch is equivalent to the 'git-compat-util.h' only part of the patch
given below.

This fixed up the current test failures (unit-tests and t9904-url-parse.sh).

Note that Patrick wanted to have a clean test-suite run on cygwin, so in
commit 5f8af25ff9 ("t5500, t5601: skip tests which exercise paths with '[::1]'
on Cygwin", 2024-10-16), he suppressed the test failures in t5500 and t5601.
(that was about the time of the v2.48.0 release).

The changes to tests t5500 and t5601, in the patch given below, essentially
reverts Patrick's commit 5f8af25ff9. This fixes all of the tests in t5601 and
ten of the twelve failures in t5500. (I don't recall what happened to t5580,
the single failure - the push test - was fixed somewhere between v2.43.0 and
v2.44.0-rc0).

As luck would have it, I left a note to myself about the remaining two
failure cases. This leads to the remaining hunk, to connect.c, in the patch
below; ie. the removal of a conditional (which should only fire for GfW and
cygwin). The '#ifdef DUMMY/#endif' should probably be replaced with an
'#ifdef GIT_WINDOWS_NATIVE/#endif' so that GfW is not affected. (Having said
that, I suspect that even GfW should drop it ['somebody was smoking something
exotic'], but I have no way to test it, so ...).

With this final hunk, this patch results in a clean test-suite run. :)

So, what does this mean? Well, I should probably not leave it another five
years, or thereabouts, before actually sending a real patch (series) to fix
this up properly! ;)

But what is properly? The 'fix' works, but the problem was principally caused
by cygwin being a bit schizophrenic about the 'pathname utilities' and its
support for POSIX only paths, WIN32 only paths or both.

For example, f82a97eb91 ("mingw: handle `subst`-ed "DOS drives"", 2019-09-06)
notes in the commit message:

    Note: `[::1]:repo` is a valid URL, but not a valid path on Windows.
    As `[` is now considered a valid drive letter, we need to be very
    careful to avoid misinterpreting such a string as valid local path in
    `url_is_local_not_ssh()`. To do that, we use the just-introduced
    function `is_valid_path()` (which will label the string as invalid file
    name because of the colon characters).
    
    This fixes CVE-2019-1351.

Note that part of the fix involves a cygwin specific 'is_valid_path()' which
would otherwise be defined, like Linux, as simply '1' in git-compat-util.h.
I was a bit surprised that an IPv6 address was not parsed at a higher level
and take priority over an (possible) '[' drive letter, rather than being a
side-effect of calling an 'is_valid_path()' helper function. ;)

The 'is_valid_path()' was also part of the security fixes to check for some
'illegal' win32 paths which included those ending in spaces or periods or
for names such as AUX, COM1, LPTn, NUL, PRN, CONIN$, etc,. I know that some
of these paths are valid in cygwin (eg. those ending in spaces or periods)
so the win32 version of that function cannot be used without change. (this
could lead to one part of git allowing you to use said paths and other
parts not)! Note that some of the CVEs may actually still apply to cygwin.
I didn't check.

Also, note that I replaced the win32 version of the 'offset_1st_component()'
function. I can't quite remember why I did that, but I think the fact that
the win32 version doesn't ensure that the 'path' consists of both the <server>
and <share> component before returning the offset. (ie make sure that the
'path' consists of "//<server>/<share>/" at the very least and return one,
for '/', otherwise).

Personally, I would be quite happy to rip out all win32 path handling and
only support POSIX paths (I have been using cygwin since about 1996 and
have only ever used win32 paths when testing git ... that is the whole
point of cygwin! :) ), but I already know that that is a no-go. (there is
always somebody that complains when you suggest it).

So, for now anyway, it seems that I need to tidy up the patch and move in
the opposite direction to e.g. commit 1cadad6f65 ("git clone <url> 
C:\cygwin\home\USER\repo' is working (again)", 2018-12-15).

Part of the reason for vacillating on the correct way forward with this
patch, was because I have often thought that I should use the cygwin API
to cater to both POSIX and win32 paths. For example, we could possibly use
the 'cygwin_conv_path()' function to do the path conversion (somewhat
similar to the macos pre-composed-utf8 stuff, minus the directory reading).
However, I think that would open a different can of worms, including some
potential memory leaks. So, not exactly a slam dunk.

[I also had a note-to-self about 'mixed / and \ urls' in the config file
which is exposed by these same tests. So, another patch may be needed?]

Anyway, something to think about. Hmm, I suspect it would be best to just
tidy up this patch first. ;)

Just FYI. Thanks!

ATB,
Ramsay Jones


----- >8 -----
Subject: [PATCH] cygwin: fix up IPv6 scp urls

---
 connect.c             |  2 ++
 git-compat-util.h     | 39 +++++++++++++++++++++++++++++++++++++++
 t/t5500-fetch-pack.sh | 14 ++++----------
 t/t5601-clone.sh      | 11 ++---------
 4 files changed, 47 insertions(+), 19 deletions(-)

diff --git a/connect.c b/connect.c
index 47e39d2a73..6f5715e938 100644
--- a/connect.c
+++ b/connect.c
@@ -1088,10 +1088,12 @@ static enum url_scheme parse_connect_url(const char *url_orig, char **ret_host,
 
 	if (scheme == URL_SCHEME_LOCAL)
 		path = end;
+#ifdef DUMMY
 	else if (scheme == URL_SCHEME_FILE && *host != '/' &&
 		 !has_dos_drive_prefix(host) &&
 		 offset_1st_component(host - 2) > 1)
 		path = host - 2; /* include the leading "//" */
+#endif
 	else if (scheme == URL_SCHEME_FILE && has_dos_drive_prefix(end))
 		path = end; /* "file://$(pwd)" may be "file://C:/projects/repo" */
 	else
diff --git a/git-compat-util.h b/git-compat-util.h
index 8809776407..645ff96048 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -153,6 +153,45 @@ static inline int is_xplatform_dir_sep(int c)
 
 #if defined(__CYGWIN__)
 #include "compat/win32/path-utils.h"
+
+static inline int is_valid_cygwin_path(const char *path)
+{
+	if (path && strlen(path) > 2 &&
+	    path[0] == '[' && path[1] == ':' &&
+	    strchr(&path[2], ':'))
+		return 0;
+	return 1;
+}
+
+#define is_valid_path(path) is_valid_cygwin_path(path)
+
+static inline int cygwin_offset_1st_component(const char *path)
+{
+	int ret = 0;
+
+	/* C: prefix */
+	if ((ret = has_dos_drive_prefix(path)))
+		return ret;
+
+	/* //server/share/ prefix */
+	if (is_dir_sep(path[0]) && is_dir_sep(path[1])) {
+		char *pos = strpbrk(path + 2, "\\/");
+		if (pos && *(pos + 1) && !is_dir_sep(*(pos + 1))) {
+			pos = strpbrk(pos + 1, "\\/");
+			if (pos && *(pos + 1))
+				return pos + 1 - path;
+		}
+	}
+
+	/* / prefix */
+	if (is_dir_sep(path[0]))
+		return 1;
+
+	return 0;
+}
+
+#undef offset_1st_component
+#define offset_1st_component cygwin_offset_1st_component
 #endif
 #if defined(__MINGW32__)
 /* pull in Windows compatibility stuff */
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 649a615ec9..2c6c0a04be 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -774,7 +774,7 @@ do
 	# file with scheme
 	for p in file
 	do
-		test_expect_success !WINDOWS "fetch-pack --diag-url $p://$h/$r" '
+		test_expect_success !MINGW "fetch-pack --diag-url $p://$h/$r" '
 			check_prot_path $p://$h/$r $p "/$r"
 		'
 		test_expect_success MINGW "fetch-pack --diag-url $p://$h/$r" '
@@ -784,7 +784,7 @@ do
 			check_prot_path $p:///$r $p "/$r"
 		'
 		# No "/~" -> "~" conversion for file
-		test_expect_success !WINDOWS "fetch-pack --diag-url $p://$h/~$r" '
+		test_expect_success !MINGW "fetch-pack --diag-url $p://$h/~$r" '
 			check_prot_path $p://$h/~$r $p "/~$r"
 		'
 		test_expect_success MINGW "fetch-pack --diag-url $p://$h/~$r" '
@@ -806,17 +806,11 @@ do
 	p=ssh
 	for h in host [::1]
 	do
-		expectation="success"
-		if test_have_prereq CYGWIN && test "$h" = "[::1]"
-		then
-			expectation="failure"
-		fi
-
-		test_expect_$expectation "fetch-pack --diag-url $h:$r" '
+		test_expect_success "fetch-pack --diag-url $h:$r" '
 			check_prot_host_port_path $h:$r $p "$h" NONE "$r"
 		'
 		# Do "/~" -> "~" conversion
-		test_expect_$expectation "fetch-pack --diag-url $h:/~$r" '
+		test_expect_success "fetch-pack --diag-url $h:/~$r" '
 			check_prot_host_port_path $h:/~$r $p "$h" NONE "~$r"
 		'
 	done
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 3dd229c186..35e9f3eb0d 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -530,17 +530,10 @@ do
 	'
 done
 
-# Parsing of paths that look like IPv6 addresses is broken on Cygwin.
-expectation_for_ipv6_tests=success
-if test_have_prereq CYGWIN
-then
-	expectation_for_ipv6_tests=failure
-fi
-
 #ipv6
 for repo in rep rep/home/project 123
 do
-	test_expect_$expectation_for_ipv6_tests "clone [::1]:$repo" '
+	test_expect_success "clone [::1]:$repo" '
 		test_clone_url [::1]:$repo ::1 "$repo"
 	'
 done
@@ -553,7 +546,7 @@ test_expect_success !SANITIZE_LEAK "clone host:/~repo" '
 	test_clone_url host:/~repo host "~repo"
 '
 
-test_expect_$expectation_for_ipv6_tests !SANITIZE_LEAK "clone [::1]:/~repo" '
+test_expect_success !SANITIZE_LEAK "clone [::1]:/~repo" '
 	test_clone_url [::1]:/~repo ::1 "~repo"
 '
 
-- 
2.55.0

Re: cygwin v2.55.0 test failures

[Torsten Bögershausen]

On Fri, Jul 10, 2026 at 07:32:23PM +0100, Ramsay Jones wrote:

[snip]
Hej Ramsay,
 Thanks for picking this up - I have some smaller comments inline,
 trying to be helpful.

> As luck would have it, I left a note to myself about the remaining two
> failure cases. This leads to the remaining hunk, to connect.c, in the patch
> below; ie. the removal of a conditional (which should only fire for GfW and
> cygwin). The '#ifdef DUMMY/#endif' should probably be replaced with an
> '#ifdef GIT_WINDOWS_NATIVE/#endif' so that GfW is not affected. (Having said
> that, I suspect that even GfW should drop it ['somebody was smoking something
> exotic'], but I have no way to test it, so ...).

> Personally, I would be quite happy to rip out all win32 path handling and
> only support POSIX paths (I have been using cygwin since about 1996 and
> have only ever used win32 paths when testing git ... that is the whole
> point of cygwin! :) ), but I already know that that is a no-go. (there is
> always somebody that complains when you suggest it).

As cygwin supports/allows win32 paths: we do support them in Git as well.
(and nobody is forced to use them)

> 
> So, for now anyway, it seems that I need to tidy up the patch and move in
> the opposite direction to e.g. commit 1cadad6f65 ("git clone <url> 
> C:\cygwin\home\USER\repo' is working (again)", 2018-12-15).
> 
> Part of the reason for vacillating on the correct way forward with this
> patch, was because I have often thought that I should use the cygwin API
> to cater to both POSIX and win32 paths. For example, we could possibly use
> the 'cygwin_conv_path()' function to do the path conversion (somewhat
> similar to the macos pre-composed-utf8 stuff, minus the directory reading).
> However, I think that would open a different can of worms, including some
> potential memory leaks. So, not exactly a slam dunk.
> 
> [I also had a note-to-self about 'mixed / and \ urls' in the config file
> which is exposed by these same tests. So, another patch may be needed?]
Not sure if I follow. cygwin allows mixed / and \ . What should be patched ? 
> 
> Anyway, something to think about. Hmm, I suspect it would be best to just
> tidy up this patch first. ;)
> 
> Just FYI. Thanks!
> 
> ATB,
> Ramsay Jones
> diff --git a/connect.c b/connect.c
> index 47e39d2a73..6f5715e938 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -1088,10 +1088,12 @@ static enum url_scheme parse_connect_url(const char *url_orig, char **ret_host,
>  
>  	if (scheme == URL_SCHEME_LOCAL)
>  		path = end;
> +#ifdef DUMMY
>  	else if (scheme == URL_SCHEME_FILE && *host != '/' &&
>  		 !has_dos_drive_prefix(host) &&
>  		 offset_1st_component(host - 2) > 1)
>  		path = host - 2; /* include the leading "//" */
> +#endif

This very lines come from

commit ebb8d2c90fb0840a0803935804e37e2205505f23
  mingw: support UNC in git clone file://server/share/repo

...and I can not see a reason to remove it.

Re: cygwin v2.55.0 test failures

[Patrick Steinhardt]

On Fri, Jul 10, 2026 at 07:32:23PM +0100, Ramsay Jones wrote:
[snip]
> Note that Patrick wanted to have a clean test-suite run on cygwin, so in
> commit 5f8af25ff9 ("t5500, t5601: skip tests which exercise paths with '[::1]'
> on Cygwin", 2024-10-16), he suppressed the test failures in t5500 and t5601.
> (that was about the time of the v2.48.0 release).
> 
> The changes to tests t5500 and t5601, in the patch given below, essentially
> reverts Patrick's commit 5f8af25ff9. This fixes all of the tests in t5601 and
> ten of the twelve failures in t5500. (I don't recall what happened to t5580,
> the single failure - the push test - was fixed somewhere between v2.43.0 and
> v2.44.0-rc0).

Yeah, this was merely papering over issues. I'd very much welcome a
revert and proper fix for this.

> As luck would have it, I left a note to myself about the remaining two
> failure cases. This leads to the remaining hunk, to connect.c, in the patch
> below; ie. the removal of a conditional (which should only fire for GfW and
> cygwin). The '#ifdef DUMMY/#endif' should probably be replaced with an
> '#ifdef GIT_WINDOWS_NATIVE/#endif' so that GfW is not affected. (Having said
> that, I suspect that even GfW should drop it ['somebody was smoking something
> exotic'], but I have no way to test it, so ...).
> 
> With this final hunk, this patch results in a clean test-suite run. :)

Nice :)

By the way: I was pondering multiple times over whether or not we should
add Cygwin to our CI matrix. It seems to be sufficiently different from
both MSYS2 and native Win32 to have its own set of compatibility issues,
so that could be worth it?

>  connect.c             |  2 ++
>  git-compat-util.h     | 39 +++++++++++++++++++++++++++++++++++++++
>  t/t5500-fetch-pack.sh | 14 ++++----------
>  t/t5601-clone.sh      | 11 ++---------
>  4 files changed, 47 insertions(+), 19 deletions(-)

For the record: I don't really have much of an opinion on this given
that I tend to not use Windows, except when I (once again) break some
tests there. Especially the path handling si something that tends to
cause lots of confusion on my side.

Patrick

Re: cygwin v2.55.0 test failures

[Ramsay Jones]

On 12/07/2026 9:04 pm, Torsten Bögershausen wrote:
> On Fri, Jul 10, 2026 at 07:32:23PM +0100, Ramsay Jones wrote:
[snip]
>> [I also had a note-to-self about 'mixed / and \ urls' in the config file
>> which is exposed by these same tests. So, another patch may be needed?]
> Not sure if I follow. cygwin allows mixed / and \ . What should be patched ? 

Yes, maybe nothing needs patching - it was a note-to-self to check that the
mixed urls don't cause any issues and, maybe, normalize the urls before
writing them to the config.

>>
>> Anyway, something to think about. Hmm, I suspect it would be best to just
>> tidy up this patch first. ;)
>>
>> Just FYI. Thanks!
>>
>> ATB,
>> Ramsay Jones
>> diff --git a/connect.c b/connect.c
>> index 47e39d2a73..6f5715e938 100644
>> --- a/connect.c
>> +++ b/connect.c
>> @@ -1088,10 +1088,12 @@ static enum url_scheme parse_connect_url(const char *url_orig, char **ret_host,
>>  
>>  	if (scheme == URL_SCHEME_LOCAL)
>>  		path = end;
>> +#ifdef DUMMY
>>  	else if (scheme == URL_SCHEME_FILE && *host != '/' &&
>>  		 !has_dos_drive_prefix(host) &&
>>  		 offset_1st_component(host - 2) > 1)
>>  		path = host - 2; /* include the leading "//" */
>> +#endif
> 
> This very lines come from
> 
> commit ebb8d2c90fb0840a0803935804e37e2205505f23
>   mingw: support UNC in git clone file://server/share/repo
> 
> ...and I can not see a reason to remove it.

Heh, I just read a few references [1][2][3] about file URIs to refresh my
memory (I read the RFCs many many moons ago ... and they seem to have
changed in the meantime? At least I don't remember it said that! :) ).

I seem to have misremembered the 'number of slashes' after the 'file:'
prefix as three or four, not two (specifically with a windows UNC or
absolute path). However, I was clearly wrong!

[The 'non-standard' rules on win32 are wild - git clearly doesn't support
all the edge cases].

OK, so I probably need to look at the two failing tests again - maybe I
need to mark them with !CYGWIN.

Anyway, more work to do! ;)

ATB,
Ramsay Jones

[1] https://en.wikipedia.org/wiki/File_URI_scheme
[2] https://datatracker.ietf.org/doc/rfc8089/
[3] https://learn.microsoft.com/en-us/archive/blogs/ie/file-uris-in-windows

Re: cygwin v2.55.0 test failures

[Ramsay Jones]

On 13/07/2026 12:06 pm, Patrick Steinhardt wrote:
> On Fri, Jul 10, 2026 at 07:32:23PM +0100, Ramsay Jones wrote:
> [snip]
[snip]
> 
> By the way: I was pondering multiple times over whether or not we should
> add Cygwin to our CI matrix. It seems to be sufficiently different from
> both MSYS2 and native Win32 to have its own set of compatibility issues,
> so that could be worth it?

Hmm, I don't know. It is a distinct platform with its own set of peculiar
issues. So, it may be worth it. However, I wouldn't want to expend CI
resources on a platform which has an unknown user-base. How many cygwin
users are there? (it can sometimes feel like there are very many, sometimes
maybe just a handful!). ;)


> For the record: I don't really have much of an opinion on this given
> that I tend to not use Windows, except when I (once again) break some
> tests there. Especially the path handling si something that tends to
> cause lots of confusion on my side.

You are not alone!

ATB,
Ramsay Jones