[PATCH 0/2] MR8589: cmd/tests: Don't use if errorlevel 0
if errorlevel is a >= comparison; if errorlevel 0 always succeeds and is meaningless. Also fix the find command; the copy tests were actually failing, but failures were masked by the errorlevel thing. This commit is the same as in !8588, it's needed by both. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8589
From: Alfred Agrell <floating(a)muncher.se> Without that, find ignores all arguments, and instantly returns errorlevel 1 and no output. Other builtins and programs may also react to invalid stdin; I didn't investigate it. --- programs/cmd/tests/batch.c | 10 +++++++++- programs/cmd/tests/test_builtins.bat.exp | 16 ++++++++-------- programs/cmd/tests/test_builtins.cmd.exp | 16 ++++++++-------- 3 files changed, 25 insertions(+), 17 deletions(-) diff --git a/programs/cmd/tests/batch.c b/programs/cmd/tests/batch.c index 03a05f34215..79da6b011a0 100644 --- a/programs/cmd/tests/batch.c +++ b/programs/cmd/tests/batch.c @@ -112,7 +112,7 @@ static BOOL run_cmd(const char *res_name, const char *cmd_data, DWORD cmd_size) char *command; STARTUPINFOA si = {sizeof(si)}; PROCESS_INFORMATION pi; - HANDLE file,fileerr; + HANDLE file,fileerr,filenul; DWORD size; BOOL bres; @@ -143,9 +143,16 @@ static BOOL run_cmd(const char *res_name, const char *cmd_data, DWORD cmd_size) if(fileerr == INVALID_HANDLE_VALUE) return FALSE; + filenul = CreateFileA("NUL", GENERIC_READ, FILE_SHARE_WRITE|FILE_SHARE_READ, &sa, OPEN_EXISTING, + FILE_ATTRIBUTE_NORMAL, NULL); + ok(filenul != INVALID_HANDLE_VALUE, "CreateFile stdin failed\n"); + if(filenul == INVALID_HANDLE_VALUE) + return FALSE; + si.dwFlags = STARTF_USESTDHANDLES; si.hStdOutput = file; si.hStdError = fileerr; + si.hStdInput = filenul; bres = CreateProcessA(NULL, command, NULL, NULL, TRUE, 0, NULL, NULL, &si, &pi); ok(bres, "CreateProcess failed: %lu\n", GetLastError()); if(!bres) { @@ -158,6 +165,7 @@ static BOOL run_cmd(const char *res_name, const char *cmd_data, DWORD cmd_size) CloseHandle(pi.hProcess); CloseHandle(file); CloseHandle(fileerr); + CloseHandle(filenul); DeleteFileA(command); return TRUE; } diff --git a/programs/cmd/tests/test_builtins.bat.exp b/programs/cmd/tests/test_builtins.bat.exp index a6b583f78c1..36e6b527a17 100644 --- a/programs/cmd/tests/test_builtins.bat.exp +++ b/programs/cmd/tests/test_builtins.bat.exp @@ -74,7 +74,7 @@ FAILURE 1 --- success/failure for COPY command FAILURE 1 SUCCESS 0 -FAILURE 1 +(a)todo_wine@SUCCESS 0 FAILURE 1 FAILURE 1 --- success/failure for MOVE command @@ -174,7 +174,7 @@ SUCCESS 0 FAILURE 1 FAILURE 1 --- success/failure for LABEL command -FAILURE 1 +(a)todo_wine@SUCCESS 0 --- success/failure for PATH command SUCCESS 666 SUCCESS 666 @@ -218,16 +218,16 @@ FAILURE 1 SUCCESS 666 SUCCESS 666 --- success/failure for CHOICE command -FAILURE 1 -FAILURE 1 +(a)todo_wine@FAILURE 255 +(a)todo_wine@FAILURE 255 FAILURE 2 -FAILURE 1 +(a)todo_wine@FAILURE 255 --- success/failure for MORE command -SUCCESS 0 -SUCCESS 0 +(a)todo_wine@FAILURE 1 +(a)todo_wine@FAILURE 1 foo(a)space@ SUCCESS 0 --- success/failure for PAUSE command -FAILURE 1 +(a)todo_wine@SUCCESS 666 --- diff --git a/programs/cmd/tests/test_builtins.cmd.exp b/programs/cmd/tests/test_builtins.cmd.exp index 1d31b19e01c..de28e463e14 100644 --- a/programs/cmd/tests/test_builtins.cmd.exp +++ b/programs/cmd/tests/test_builtins.cmd.exp @@ -535,7 +535,7 @@ FAILURE 1 --- success/failure for COPY command FAILURE 1 SUCCESS 0 -FAILURE 1 +(a)todo_wine@SUCCESS 0 FAILURE 1 FAILURE 1 --- success/failure for MOVE command @@ -635,7 +635,7 @@ SUCCESS 0 FAILURE 1 FAILURE 1 --- success/failure for LABEL command -FAILURE 1 +(a)todo_wine@SUCCESS 0 --- success/failure for PATH command SUCCESS 0 SUCCESS 0 @@ -679,18 +679,18 @@ FAILURE 1 SUCCESS 666 SUCCESS 666 --- success/failure for CHOICE command -FAILURE 1 -FAILURE 1 +(a)todo_wine@FAILURE 255 +(a)todo_wine@FAILURE 255 FAILURE 2 -FAILURE 1 +(a)todo_wine@FAILURE 255 --- success/failure for MORE command -SUCCESS 0 -SUCCESS 0 +(a)todo_wine@FAILURE 1 +(a)todo_wine@FAILURE 1 foo(a)space@ SUCCESS 0 --- success/failure for PAUSE command -FAILURE 1 +(a)todo_wine@SUCCESS 666 --- ------------ Testing 'set' ------------ 1 -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8589
From: Alfred Agrell <floating(a)muncher.se> if errorlevel is a >= comparison, so if errorlevel 0 always succeeds, making the test meaningless. --- programs/cmd/tests/test_builtins.cmd | 24 ++----- programs/cmd/tests/test_builtins.cmd.exp | 88 ++++++++++++------------ 2 files changed, 48 insertions(+), 64 deletions(-) diff --git a/programs/cmd/tests/test_builtins.cmd b/programs/cmd/tests/test_builtins.cmd index 7db955f634d..39ce652341b 100644 --- a/programs/cmd/tests/test_builtins.cmd +++ b/programs/cmd/tests/test_builtins.cmd @@ -2598,11 +2598,7 @@ echo ------------ Testing del ------------ echo abc > file echo deleting 'file' del file -if errorlevel 0 ( - echo errorlevel is 0, good -) else ( - echo unexpected errorlevel, got %errorlevel% -) +echo %errorlevel% if not exist file ( echo successfully deleted 'file' ) else ( @@ -2610,11 +2606,7 @@ if not exist file ( ) echo attempting to delete 'file', even though it is not present del file -if errorlevel 0 ( - echo errorlevel is 0, good -) else ( - echo unexpected errorlevel, got %errorlevel% -) +echo %errorlevel% echo ------------ Testing del /a ------------ del /f/q *.test > nul @@ -3418,11 +3410,7 @@ goto :eof :CheckOutputExist find /i "%1" test1.txt >nul 2>&1 -if errorlevel 0 ( - echo Passed: Found expected %1 in COPY output -) else ( - echo Failed: Did not find expected %1 in COPY output -) +echo line %1 errorlevel %errorlevel% expected 0 shift if not "%1"=="" goto :CheckOutputExist del /q test1.txt >nul 2>&1 @@ -3430,11 +3418,7 @@ goto :eof :CheckOutputNotExist find /i "%1" test1.txt >nul 2>&1 -if errorlevel 1 ( - echo Passed: Did not find %1 in COPY output -) else ( - echo Failed: Unexpectedly found %1 in COPY output -) +echo line %1 errorlevel %errorlevel% expected 1 shift if not "%1"=="" goto :CheckOutputNotExist del /q test1.txt >nul 2>&1 diff --git a/programs/cmd/tests/test_builtins.cmd.exp b/programs/cmd/tests/test_builtins.cmd.exp index de28e463e14..8c33121f04f 100644 --- a/programs/cmd/tests/test_builtins.cmd.exp +++ b/programs/cmd/tests/test_builtins.cmd.exp @@ -1664,10 +1664,10 @@ a a b c ------------ Testing del ------------ deleting 'file' -errorlevel is 0, good +0 successfully deleted 'file' attempting to delete 'file', even though it is not present -errorlevel is 0, good +(a)todo_wine@0 ------------ Testing del /a ------------ not-r.test not found after delete, good r.test found before delete, good @@ -1936,33 +1936,33 @@ file correctly deleted --- a batch file can alter itself bar ---------- Testing copy -Passed: Did not find file1 in COPY output +line file1 errorlevel 1 expected 1 Passed: Found expected dummy.file -Passed: Did not find file1 in COPY output +line file1 errorlevel 1 expected 1 Passed: Found expected dir1\file1 -Passed: Did not find file1 in COPY output +line file1 errorlevel 1 expected 1 Passed: Found expected dir1\file1 -Passed: Did not find file1 in COPY output +line file1 errorlevel 1 expected 1 Passed: Found expected dir1\file99 -Passed: Did not find file1 in COPY output +line file1 errorlevel 1 expected 1 Passed: Found expected file1 Passed: Did not find dir2 Passed: Did not find dir2\file1 -Passed: Found expected file1 in COPY output -Passed: Found expected file2 in COPY output -Passed: Found expected file3 in COPY output +line file1 errorlevel 0 expected 0 +line file2 errorlevel 0 expected 0 +line file3 errorlevel 0 expected 0 Passed: Found expected dir1\file1 Passed: Found expected dir1\file2 Passed: Found expected dir1\file3 -Passed: Found expected file1 in COPY output -Passed: Found expected file2 in COPY output -Passed: Found expected file3 in COPY output +line file1 errorlevel 0 expected 0 +line file2 errorlevel 0 expected 0 +line file3 errorlevel 0 expected 0 Passed: Found expected dir1\file1 Passed: Found expected dir1\file2 Passed: Found expected dir1\file3 -Passed: Found expected file1 in COPY output -Passed: Found expected file2 in COPY output -Passed: Found expected file3 in COPY output +line file1 errorlevel 0 expected 0 +line file2 errorlevel 0 expected 0 +line file3 errorlevel 0 expected 0 Passed: Found expected file1 Passed: Found expected file2 Passed: Found expected file3 @@ -1970,42 +1970,42 @@ Passed: Did not find dir2 Passed: Did not find dir2\file1 Passed: Did not find dir2\file2 Passed: Did not find dir2\file3 -Passed: Did not find file2 in COPY output +line file2 errorlevel 1 expected 1 Passed: Found expected testfile -Passed: Found expected file1 in COPY output -Passed: Found expected file2 in COPY output +line file1 errorlevel 0 expected 0 +line file2 errorlevel 0 expected 0 Passed: Found expected testfile -Passed: Found expected file1 in COPY output -Passed: Found expected file2 in COPY output -Passed: Found expected file3 in COPY output +line file1 errorlevel 0 expected 0 +line file2 errorlevel 0 expected 0 +line file3 errorlevel 0 expected 0 Passed: Found expected testfile -Passed: Found expected file1 in COPY output -Passed: Found expected file2 in COPY output -Passed: Found expected file3 in COPY output -Passed: Found expected fred in COPY output +line file1 errorlevel 0 expected 0 +line file2 errorlevel 0 expected 0 +line file3 errorlevel 0 expected 0 +line fred errorlevel 0 expected 0 Passed: Found expected testfile -Passed: Found expected fred in COPY output -Passed: Found expected file1 in COPY output -Passed: Found expected file2 in COPY output -Passed: Found expected file3 in COPY output +line fred errorlevel 0 expected 0 +line file1 errorlevel 0 expected 0 +line file2 errorlevel 0 expected 0 +line file3 errorlevel 0 expected 0 Passed: Found expected testfile -Passed: Found expected fred in COPY output -Passed: Found expected file1 in COPY output -Passed: Found expected file2 in COPY output -Passed: Found expected file3 in COPY output +line fred errorlevel 0 expected 0 +line file1 errorlevel 0 expected 0 +line file2 errorlevel 0 expected 0 +line file3 errorlevel 0 expected 0 Passed: Found expected dir1\fred -Passed: Found expected fred in COPY output -Passed: Found expected file1 in COPY output -Passed: Found expected file2 in COPY output -Passed: Found expected file3 in COPY output +line fred errorlevel 0 expected 0 +line file1 errorlevel 0 expected 0 +line file2 errorlevel 0 expected 0 +line file3 errorlevel 0 expected 0 Passed: Found expected dir1\fred -Passed: Found expected fred in COPY output -Passed: Found expected file1 in COPY output -Passed: Found expected file2 in COPY output -Passed: Found expected file3 in COPY output +line fred errorlevel 0 expected 0 +line file1 errorlevel 0 expected 0 +line file2 errorlevel 0 expected 0 +line file3 errorlevel 0 expected 0 Passed: Found expected fred -Passed: Found expected fred in COPY output -Passed: Found expected file1 in COPY output +line fred errorlevel 0 expected 0 +line file1 errorlevel 0 expected 0 Passed: Found expected fred Passed: file size check on file1 [5]@or_broken(a)Skipping file size check on NT4 Passed: file size check on file2 [8]@or_broken(a)Skipping file size check on NT4 -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8589
the fixes for DEL are ok (still need to decide upon overlap and comments from MR8588) (side note: internally `errorlevel` is a signed integer, and can be set to a negative value. so, generally speaking, `if errorlevel 0` is still a valid and meaningful statement and is not always true; AFAICT DEL's doesn't set errorlevel to a negative value) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8589#note_110263
On Fri Jul 18 09:42:57 2025 +0000, eric pouech wrote:
the fixes for DEL are ok (still need to decide upon overlap and comments from MR8588) (side note: internally `errorlevel` is a signed integer, and can be set to a negative value. so, generally speaking, `if errorlevel 0` is still a valid and meaningful statement and is not always true; AFAICT DEL's doesn't set errorlevel to a negative value) Technically true, but accepting both zero and (some) nonzero errorlevels is quite implausible. If such behavior is desired, it should be rewritten to if %errorlevel% geq 0, or prominently commented.
In this case, they're clearly intended to be equality comparisons. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8589#note_110285
participants (3)
-
Alfred Agrell -
Alfred Agrell (＠Alcaro)
-
eric pouech (＠epo)