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

From: Junio C Hamano <[email-masked]>
Date: Wed, 07 Sep 2022 13:49:08 -0700

Johannes Schindelin <[email-masked]> 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.