| Message ID | 20220901173942.abolcr4aa5gixncm@lucy.dinwoodie.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | t1800: correct test to handle Cygwin | expand |
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: ...
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: ...
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 >
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.
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
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…
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.
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: ...
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(-)