On Wed, 7 Sept 2022 at 21:49, Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Johannes Schindelin <Johannes.Schindelin@xxxxxx> 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…
