[PATCH] t1800: correct test to handle Cygwin

[PATCH] t1800: correct test to handle Cygwin

[Adam Dinwoodie]

On Cygwin, when failing to spawn a process using start_command, Git
outputs the same error as on Linux systems, rather than using the
GIT_WINDOWS_NATIVE-specific error output.  The WINDOWS test prerequisite
is set in both Cygwin and native Windows environments, which means it's
not appropriate to use to anticipate the error output from
start_command.  Instead, use the MINGW test prerequisite, which is only
set for Git in native Windows environments, and not for Cygwin.

Signed-off-by: Adam Dinwoodie <adam@dinwoodie.org>
---

The job of setting Cygwin up to get Git CI builds, either as part of the
main CI builds or as something using separate automation, is rapidly
rising up my when-I-have-the-time to-do list...

 t/t1800-hook.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
index 64096adac7..fae8b2faf9 100755
--- a/t/t1800-hook.sh
+++ b/t/t1800-hook.sh
@@ -159,7 +159,7 @@ test_expect_success 'git hook run a hook with a bad shebang' '
 	# TODO: We should emit the same (or at least a more similar)
 	# error on Windows and !Windows. See the OS-specific code in
 	# start_command()
-	if test_have_prereq !WINDOWS
+	if test_have_prereq !MINGW
 	then
 		cat >expect <<-\EOF
 		fatal: cannot run bad-hooks/test-hook: ...
-- 
2.37.2

Re: [PATCH] t1800: correct test to handle Cygwin

[Junio C Hamano]

Adam Dinwoodie <adam@dinwoodie.org> writes:

> On Cygwin, when failing to spawn a process using start_command, Git
> outputs the same error as on Linux systems, rather than using the
> GIT_WINDOWS_NATIVE-specific error output.  The WINDOWS test prerequisite
> is set in both Cygwin and native Windows environments, which means it's
> not appropriate to use to anticipate the error output from
> start_command.  Instead, use the MINGW test prerequisite, which is only
> set for Git in native Windows environments, and not for Cygwin.
>
> Signed-off-by: Adam Dinwoodie <adam@dinwoodie.org>
> ---
>
> The job of setting Cygwin up to get Git CI builds, either as part of the
> main CI builds or as something using separate automation, is rapidly
> rising up my when-I-have-the-time to-do list...

I added a few who probably are more familiar with Windows situation
than those already CC'ed.

A quick grep in the test directory

    $ git grep '\<WINDOWS\>" t

hits this one and t7450-bad-git-dotfiles.sh and nothing else, but
we do have quite a many hits of "test_have_prereq !MINGW,!CYGWIN".

I guess it depends on how common various "glitches" MINGW and CYGWIN
prerequistes are trying to cover, but I am wondering if we are
helped by having three (i.e. MINGW, CYGWIN, and WINDOWS)
prerequisites, or does WINDOWS as a separate prerequisite
contributes more to the confusion (and if so, if it is a good idea
to remove and use only MINGW and CYGWIN prerequistes).

One possible roadblock is that it is not easy to express "MINGW or
CYGWIN" in the prerequisite syntax and WINDOWS prerequisite is an
easy way to fill that gap.

In any case, thanks for a fix.  Will queue.

>  t/t1800-hook.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
> index 64096adac7..fae8b2faf9 100755
> --- a/t/t1800-hook.sh
> +++ b/t/t1800-hook.sh
> @@ -159,7 +159,7 @@ test_expect_success 'git hook run a hook with a bad shebang' '
>  	# TODO: We should emit the same (or at least a more similar)
>  	# error on Windows and !Windows. See the OS-specific code in
>  	# start_command()
> -	if test_have_prereq !WINDOWS
> +	if test_have_prereq !MINGW
>  	then
>  		cat >expect <<-\EOF
>  		fatal: cannot run bad-hooks/test-hook: ...

Re: [PATCH] t1800: correct test to handle Cygwin

[Đoàn Trần Công Danh]

On 2022-09-01 11:32:12-0700, Junio C Hamano <gitster@pobox.com> wrote:
> In any case, thanks for a fix.  Will queue.
> 
> >  t/t1800-hook.sh | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
> > index 64096adac7..fae8b2faf9 100755
> > --- a/t/t1800-hook.sh
> > +++ b/t/t1800-hook.sh
> > @@ -159,7 +159,7 @@ test_expect_success 'git hook run a hook with a bad shebang' '
> >  	# TODO: We should emit the same (or at least a more similar)
> >  	# error on Windows and !Windows. See the OS-specific code in

Please also change the comment here too.
I'm not sure what should be put in the comment here, though.
Maybe on MINGW and other platforms?

> >  	# start_command()
> > -	if test_have_prereq !WINDOWS
> > +	if test_have_prereq !MINGW
> >  	then
> >  		cat >expect <<-\EOF
> >  		fatal: cannot run bad-hooks/test-hook: ...

-- 
Danh

Re: [PATCH] t1800: correct test to handle Cygwin

[Johannes Schindelin]

Hi Adam,

On Thu, 1 Sep 2022, Adam Dinwoodie wrote:

> On Cygwin, when failing to spawn a process using start_command, Git
> outputs the same error as on Linux systems, rather than using the
> GIT_WINDOWS_NATIVE-specific error output.

Given this explanation, I would have expected...

> The WINDOWS test prerequisite is set in both Cygwin and native Windows
> environments, which means it's not appropriate to use to anticipate the
> error output from start_command.  Instead, use the MINGW test
> prerequisite, which is only set for Git in native Windows environments,
> and not for Cygwin.
>
> Signed-off-by: Adam Dinwoodie <adam@dinwoodie.org>
> ---
>
> The job of setting Cygwin up to get Git CI builds, either as part of the
> main CI builds or as something using separate automation, is rapidly
> rising up my when-I-have-the-time to-do list...
>
>  t/t1800-hook.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
> index 64096adac7..fae8b2faf9 100755
> --- a/t/t1800-hook.sh
> +++ b/t/t1800-hook.sh
> @@ -159,7 +159,7 @@ test_expect_success 'git hook run a hook with a bad shebang' '
>  	# TODO: We should emit the same (or at least a more similar)
>  	# error on Windows and !Windows. See the OS-specific code in
>  	# start_command()
> -	if test_have_prereq !WINDOWS
> +	if test_have_prereq !MINGW

... this to be `test_have_prereq !WINDOWS || test_have_prereq CYGWIN`
instead.

At least it is my understanding that Cygwin is the special case of the
special case that is Windows.

That is, if most scenarios on Windows were behaving like Cygwin and the
mingw-w64 build was the odd duck, I would understand to restrict this only
to MINGW. But I think it is the other way around: with Cygwin, _despite_
being on Windows, we see this Linux-like behavior.

Ciao,
Dscho

>  	then
>  		cat >expect <<-\EOF
>  		fatal: cannot run bad-hooks/test-hook: ...
> --
> 2.37.2
>

Re: [PATCH] t1800: correct test to handle Cygwin

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> @@ -159,7 +159,7 @@ test_expect_success 'git hook run a hook with a bad shebang' '
>>  	# TODO: We should emit the same (or at least a more similar)
>>  	# error on Windows and !Windows. See the OS-specific code in
>>  	# start_command()
>> -	if test_have_prereq !WINDOWS
>> +	if test_have_prereq !MINGW
>
> ... this to be `test_have_prereq !WINDOWS || test_have_prereq CYGWIN`
> instead.

My understanding is that there are CYGWIN and MINGW prereqs that are
mutually exclusive (if you are on MINGW you cannot be on CYGWIN and
vice versa), and WINDOWS is a short-hand for "we are either on MINGW
or CYGWIN", and I am not sure if it clarifies anything to rewrite
"we do not want to be on MINGW to run these tests" to "anything non
Windows is fine, by the way, CYGWIN is also OK".

If we are planning to add another variant that falls into umbrella
WINDOWS that is not CYGWIN, and that new variant falls short of the
requirement of the tests in this script just like MINGW does, then
the suggested rewrite does make quite a lot of sense.  There would
be not just two but more variants in Windows family, and if Cygwin
is the odd man out, then the rewritten one would convey that more
clearly.  But with only two, as long as everybody understands what
MINGW is, I think "this part does not work on MINGW" is more concise
and clear.

As Đoàn Trần Công Danh pointed out, the TODO: comment may need to be
updated.  For future-proofing, perhaps "a more similar) error on all
platforms." would be appropriate.

Thanks.

RE: [PATCH] t1800: correct test to handle Cygwin

[rsbecker]

On September 7, 2022 4:49 PM, Junio C Hamano wrote:
>Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>>> @@ -159,7 +159,7 @@ test_expect_success 'git hook run a hook with a bad
>shebang' '
>>>  	# TODO: We should emit the same (or at least a more similar)
>>>  	# error on Windows and !Windows. See the OS-specific code in
>>>  	# start_command()
>>> -	if test_have_prereq !WINDOWS
>>> +	if test_have_prereq !MINGW
>>
>> ... this to be `test_have_prereq !WINDOWS || test_have_prereq CYGWIN`
>> instead.
>
>My understanding is that there are CYGWIN and MINGW prereqs that are
>mutually exclusive (if you are on MINGW you cannot be on CYGWIN and vice
>versa), and WINDOWS is a short-hand for "we are either on MINGW or CYGWIN",
>and I am not sure if it clarifies anything to rewrite "we do not want to be on
>MINGW to run these tests" to "anything non Windows is fine, by the way,
>CYGWIN is also OK".
>
>If we are planning to add another variant that falls into umbrella WINDOWS that is
>not CYGWIN, and that new variant falls short of the requirement of the tests in
>this script just like MINGW does, then the suggested rewrite does make quite a lot
>of sense.  There would be not just two but more variants in Windows family, and if
>Cygwin is the odd man out, then the rewritten one would convey that more
>clearly.  But with only two, as long as everybody understands what MINGW is, I
>think "this part does not work on MINGW" is more concise and clear.
>
>As Đoàn Trần Công Danh pointed out, the TODO: comment may need to be
>updated.  For future-proofing, perhaps "a more similar) error on all platforms."
>would be appropriate.

I think this is the same as I reported under [BUG] t1800: Fails for error text comparison, which somehow Outlook associated with a different thread. NonStop has the same issue with error messages not being consistent across platforms.

-Randall

Re: [PATCH] t1800: correct test to handle Cygwin

[Adam Dinwoodie]

On Wed, 7 Sept 2022 at 21:49, Junio C Hamano <gitster@pobox.com> wrote:
>
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> @@ -159,7 +159,7 @@ test_expect_success 'git hook run a hook with a bad shebang' '
> >>      # TODO: We should emit the same (or at least a more similar)
> >>      # error on Windows and !Windows. See the OS-specific code in
> >>      # start_command()
> >> -    if test_have_prereq !WINDOWS
> >> +    if test_have_prereq !MINGW
> >
> > ... this to be `test_have_prereq !WINDOWS || test_have_prereq CYGWIN`
> > instead.
>
> My understanding is that there are CYGWIN and MINGW prereqs that are
> mutually exclusive (if you are on MINGW you cannot be on CYGWIN and
> vice versa), and WINDOWS is a short-hand for "we are either on MINGW
> or CYGWIN", and I am not sure if it clarifies anything to rewrite
> "we do not want to be on MINGW to run these tests" to "anything non
> Windows is fine, by the way, CYGWIN is also OK".
>
> If we are planning to add another variant that falls into umbrella
> WINDOWS that is not CYGWIN, and that new variant falls short of the
> requirement of the tests in this script just like MINGW does, then
> the suggested rewrite does make quite a lot of sense.  There would
> be not just two but more variants in Windows family, and if Cygwin
> is the odd man out, then the rewritten one would convey that more
> clearly.  But with only two, as long as everybody understands what
> MINGW is, I think "this part does not work on MINGW" is more concise
> and clear.

This is definitely how I was thinking about things. I've no particular
objection to taking some other approach, but until/unless there are a
wider variety of Windows variants, it seems like doing so would cause
more confusion, not less.

The only caveat is the "as long as everybody understands what MINGW
is" part. This is well beyond the scope of this small patch, but I
think "MINGW" is probably needlessly opaque these days. It makes sense
given the history of Git for Windows, but it's not a term many people
are likely to be familiar with unless they've already been playing
around with the MinGW/Cygwin environments, and I'm not sure that's a
useful prerequisite for folk to be able to contribute to Git. I'm not
sure what a better solution here might be; maybe retire "MINGW" and
redefine "WINDOWS" to take its place, with current uses of "WINDOWS"
being replaced with "WINDOWS || CYGWIN"?

> As Đoàn Trần Công Danh pointed out, the TODO: comment may need to be
> updated.  For future-proofing, perhaps "a more similar) error on all
> platforms." would be appropriate.

I'll respin with some updated comments shortly…

Re: [PATCH] t1800: correct test to handle Cygwin

[Junio C Hamano]

Adam Dinwoodie <adam@dinwoodie.org> writes:

> ... It makes sense
> given the history of Git for Windows, but it's not a term many people
> are likely to be familiar with unless they've already been playing
> around with the MinGW/Cygwin environments, and I'm not sure that's a
> useful prerequisite for folk to be able to contribute to Git. I'm not
> sure what a better solution here might be; maybe retire "MINGW" and
> redefine "WINDOWS" to take its place, with current uses of "WINDOWS"
> being replaced with "WINDOWS || CYGWIN"?

It sounds more like "the world would've could've been a better place
if we did X", which is too late.  As a non-Windows person, I think
the current approach is OK that assumes that those who write and
read to understand our tests can answer the question: "You're on
Windows?  There are MINGW or CYGWIN, which one do you have?"

>> As Đoàn Trần Công Danh pointed out, the TODO: comment may need to be
>> updated.  For future-proofing, perhaps "a more similar) error on all
>> platforms." would be appropriate.
>
> I'll respin with some updated comments shortly…

Thanks.

[PATCH v2] t1800: correct test to handle Cygwin

[Adam Dinwoodie]

On Cygwin, when failing to spawn a process using start_command, Git
outputs the same error as on Linux systems, rather than using the
GIT_WINDOWS_NATIVE-specific error output.  The WINDOWS test prerequisite
is set in both Cygwin and native Windows environments, which means it's
not appropriate to use to anticipate the error output from
start_command.  Instead, use the MINGW test prerequisite, which is only
set for Git in native Windows environments, and not for Cygwin.

Signed-off-by: Adam Dinwoodie <adam@dinwoodie.org>
Helped-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---

With apologies to folk receiving this multiple times; it looks like my
SMTP setup was causing problems for some servers, and while the mails
seemed to be arriving with me, they didn't seem to be appearing at
https://public-inbox.org/git/?q=t1800

 t/t1800-hook.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
index 64096adac7..43fcb7c0bf 100755
--- a/t/t1800-hook.sh
+++ b/t/t1800-hook.sh
@@ -157,9 +157,9 @@ test_expect_success 'git hook run a hook with a bad shebang' '
 	write_script bad-hooks/test-hook "/bad/path/no/spaces" </dev/null &&
 
 	# TODO: We should emit the same (or at least a more similar)
-	# error on Windows and !Windows. See the OS-specific code in
-	# start_command()
-	if test_have_prereq !WINDOWS
+	# error on MINGW (essentially Git for Windows) and all other
+	# platforms.. See the OS-specific code in start_command()
+	if test_have_prereq !MINGW
 	then
 		cat >expect <<-\EOF
 		fatal: cannot run bad-hooks/test-hook: ...
-- 
2.34.1

RE: [PATCH v2] t1800: correct test to handle Cygwin

[rsbecker]

On September 15, 2022 3:57 AM, Adam Dinwoodie wrote:
>On Cygwin, when failing to spawn a process using start_command, Git outputs the
>same error as on Linux systems, rather than using the GIT_WINDOWS_NATIVE-
>specific error output.  The WINDOWS test prerequisite is set in both Cygwin and
>native Windows environments, which means it's not appropriate to use to
>anticipate the error output from start_command.  Instead, use the MINGW test
>prerequisite, which is only set for Git in native Windows environments, and not for
>Cygwin.
>
>Signed-off-by: Adam Dinwoodie <adam@dinwoodie.org>
>Helped-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
>---
>
>With apologies to folk receiving this multiple times; it looks like my SMTP setup
>was causing problems for some servers, and while the mails seemed to be arriving
>with me, they didn't seem to be appearing at
>https://public-inbox.org/git/?q=t1800
>
> t/t1800-hook.sh | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index 64096adac7..43fcb7c0bf
>100755
>--- a/t/t1800-hook.sh
>+++ b/t/t1800-hook.sh
>@@ -157,9 +157,9 @@ test_expect_success 'git hook run a hook with a bad
>shebang' '
> 	write_script bad-hooks/test-hook "/bad/path/no/spaces" </dev/null &&
>
> 	# TODO: We should emit the same (or at least a more similar)
>-	# error on Windows and !Windows. See the OS-specific code in
>-	# start_command()
>-	if test_have_prereq !WINDOWS
>+	# error on MINGW (essentially Git for Windows) and all other
>+	# platforms.. See the OS-specific code in start_command()
>+	if test_have_prereq !MINGW
> 	then
> 		cat >expect <<-\EOF
> 		fatal: cannot run bad-hooks/test-hook: ...
>--
>2.34.1

I am not convinced this will solve the problem on NonStop where EPERM is reported instead of ENOENT. Apparently EPERM is allowed by POSIX, so text comparison of error messages are not going to work anyway.