Giving this one final shot. If this is not deemed acceptable, I will abandon the effort and just move on to other things, no hard feelings.
This is IMO a cleaner, more conservative/less invasive change.
What is fixed: - Bug #56381, "TYPE c:\windows\winhelp.exe >foo", i.e. binary mode operation. I would probably consider this the main reason for this change. I'm trying to get the compiler mentioned in the bug report working. - Ctrl-Z termination of TYPE output to the console. - "TYPE con >foo", with Ctrl-Z handling, functionally equivalent to "COPY con foo".
-- v3: cmd: Fix some TYPE command behavior. cmd/tests: Add test to check for TYPE truncation in binary mode.
From: Joe Souza jsouza@yahoo.com
--- programs/cmd/tests/test_builtins.cmd | 11 +++++++++++ programs/cmd/tests/test_builtins.cmd.exp | 2 ++ 2 files changed, 13 insertions(+)
diff --git a/programs/cmd/tests/test_builtins.cmd b/programs/cmd/tests/test_builtins.cmd index 9f144bb4e0a..6842733f30e 100644 --- a/programs/cmd/tests/test_builtins.cmd +++ b/programs/cmd/tests/test_builtins.cmd @@ -1405,6 +1405,13 @@ echo ---6 type foobaw echo ---7 del foobaz foobay foobax foobaw +echo ---8 +rem generating sequence ab<ctrl-Z>c\n\r +echo 61621A630D0A>seq.hex +certutil -decodehex seq.hex seq.bin > nul +type seq.bin > foo0 +call :CompareFileSizes seq.bin foo0 +erase seq.hex seq.bin foo0
echo ------------ Testing NUL ------------ md foobar & cd foobar @@ -3513,6 +3520,10 @@ shift if not "%1"=="" goto :CheckFileSize goto :eof
+:CompareFileSizes +if "%~z1"=="%~z2" (echo passed) else (echo failed) +goto :eof + :testcopy
rem ----------------------- diff --git a/programs/cmd/tests/test_builtins.cmd.exp b/programs/cmd/tests/test_builtins.cmd.exp index 6ec30eba3a7..0282a5dc566 100644 --- a/programs/cmd/tests/test_builtins.cmd.exp +++ b/programs/cmd/tests/test_builtins.cmd.exp @@ -994,6 +994,8 @@ foobay
---7 +---8 +@todo_wine@passed ------------ Testing NUL ------------ bar bar
From: Joe Souza jsouza@yahoo.com
--- programs/cmd/builtins.c | 102 +++++++++++++++++++++++----------------- 1 file changed, 59 insertions(+), 43 deletions(-)
diff --git a/programs/cmd/builtins.c b/programs/cmd/builtins.c index dd9808aa9f2..ff835386560 100644 --- a/programs/cmd/builtins.c +++ b/programs/cmd/builtins.c @@ -540,6 +540,49 @@ end: return ret; }
+/**************************************************************************** + * WCMD_copy_loop + * + * Copies from a file + * optionally reading only until EOF (ascii copy) + * Returns TRUE on success + */ +static BOOL WCMD_copy_loop(HANDLE in, HANDLE out, BOOL ascii) +{ + BOOL ok; + DWORD bytesread, byteswritten; + char *eof = NULL; + + /* Loop copying data from source to destination until EOF read */ + do + { + char buffer[MAXSTRING]; + + ok = ReadFile(in, buffer, MAXSTRING, &bytesread, NULL); + if (ok) { + + /* Stop at first EOF */ + if (ascii) { + eof = (char *)memchr((void *)buffer, '\x1a', bytesread); + if (eof) bytesread = (eof - buffer); + } + + if (bytesread) { + ok = WriteFile(out, buffer, bytesread, &byteswritten, NULL); + if (!ok || byteswritten != bytesread) { + WINE_ERR("Unexpected failure writing, rc=%ld\n", + GetLastError()); + } + } + } else { + WINE_ERR("Unexpected failure reading, rc=%ld\n", + GetLastError()); + } + } while (ok && bytesread > 0 && !eof); + + return ok; +} + /**************************************************************************** * WCMD_ManualCopy * @@ -552,7 +595,6 @@ static BOOL WCMD_ManualCopy(WCHAR *srcname, WCHAR *dstname, BOOL ascii, BOOL app { HANDLE in,out; BOOL ok; - DWORD bytesread, byteswritten;
WINE_TRACE("Manual Copying %s to %s (ascii: %u) (append: %u)\n", wine_dbgstr_w(srcname), wine_dbgstr_w(dstname), ascii, append); @@ -578,32 +620,7 @@ static BOOL WCMD_ManualCopy(WCHAR *srcname, WCHAR *dstname, BOOL ascii, BOOL app SetFilePointer(out, 0, NULL, FILE_END); }
- /* Loop copying data from source to destination until EOF read */ - do - { - char buffer[MAXSTRING]; - - ok = ReadFile(in, buffer, MAXSTRING, &bytesread, NULL); - if (ok) { - - /* Stop at first EOF */ - if (ascii) { - char *ptr = (char *)memchr((void *)buffer, '\x1a', bytesread); - if (ptr) bytesread = (ptr - buffer); - } - - if (bytesread) { - ok = WriteFile(out, buffer, bytesread, &byteswritten, NULL); - if (!ok || byteswritten != bytesread) { - WINE_ERR("Unexpected failure writing to %s, rc=%ld\n", - wine_dbgstr_w(dstname), GetLastError()); - } - } - } else { - WINE_ERR("Unexpected failure reading from %s, rc=%ld\n", - wine_dbgstr_w(srcname), GetLastError()); - } - } while (ok && bytesread > 0); + ok = WCMD_copy_loop(in, out, ascii);
CloseHandle(out); CloseHandle(in); @@ -3416,31 +3433,30 @@ RETURN_CODE WCMD_type(WCHAR *args) while (argN) { WCHAR *thisArg = WCMD_parameter (args, argno++, &argN, FALSE, FALSE);
- HANDLE h; - WCHAR buffer[512]; - DWORD count; + HANDLE hIn, hOut; + DWORD console_mode;
if (!argN) break;
WINE_TRACE("type: Processing arg '%s'\n", wine_dbgstr_w(thisArg)); - h = CreateFileW(thisArg, GENERIC_READ, FILE_SHARE_READ|FILE_SHARE_WRITE|FILE_SHARE_DELETE, NULL, OPEN_EXISTING, - FILE_ATTRIBUTE_NORMAL, NULL); - if (h == INVALID_HANDLE_VALUE) { + hIn = CreateFileW(thisArg, GENERIC_READ, FILE_SHARE_READ|FILE_SHARE_WRITE|FILE_SHARE_DELETE, NULL, OPEN_EXISTING, + FILE_ATTRIBUTE_NORMAL, NULL); + if (hIn == INVALID_HANDLE_VALUE) { WCMD_print_error (); WCMD_output_stderr(WCMD_LoadMessage(WCMD_READFAIL), thisArg); return errorlevel = ERROR_INVALID_FUNCTION; - } else { - if (writeHeaders) { - WCMD_output_stderr(L"\n%1\n\n\n", thisArg); - } - while (WCMD_ReadFile(h, buffer, ARRAY_SIZE(buffer) - 1, &count)) { - if (count == 0) break; /* ReadFile reports success on EOF! */ - buffer[count] = 0; - WCMD_output_asis (buffer); - } - CloseHandle (h); } + hOut = GetStdHandle(STD_OUTPUT_HANDLE); + + if (writeHeaders) { + WCMD_output_stderr(L"\n%1\n\n\n", thisArg); + } + + WCMD_copy_loop(hIn, hOut, GetConsoleMode(hIn, &console_mode) || GetConsoleMode(hOut, &console_mode)); + + CloseHandle (hIn); } + return errorlevel = return_code; }
On Tue Sep 9 18:12:34 2025 +0000, eric pouech wrote:
sorry looks I forgot to post following bits alongside previous comments
(general note: you'd better update the previous MR rather than closing/creating a new one as we lose some context) for the tests:
- IMO adding a test dependency to winhelp.exe (even from system dir)
could cause trouble on some configurations (and it takes for granted that it always contains a ctrl-z character)
- it's already a pain to swing back and forth between the two files for
input / expected output, so I'd rather not add another test file creation into batch.c
- unfortunately, trying to create the test file inside test_builtin.cmd
using the @\x1b@ sequence doesn't work (it looks like native just erases ctrl-z from the input in that case)
- so I'd ended up with
[test-type.patch](/uploads/bc7b0de8295a24a1242c043c36043387/test-type.patch) instead
- (and we should also get rid at some point of create_test_nul_file() in
batch.c and move test file creation in test_builtins.cmd)
- and adding tests should be first patch in MR (so it's clearer in
subsequent patches what has been fixed by removing the todo_wine)
Thanks for the test patch. I replaced my tests with your and switched the order of the commits. However: - Code before my change truncated at NUL, not Ctrl-Z. winhelp.exe test was the specific case mentioned in the bug and I added it as a regression test. Your test does not have a case that checks for truncation at NUL. - I also had difficulty creating a file containing a Ctrl-Z character from the .cmd file, which is what prompted me to create the file in batch.c. My test file had an occurrence of each ASCII character, to ensure that there would be not truncation at any character. - I left the todo_wine in the test after adding it because the test is failing. Seems that certutil is not successful on wine, as I noticed this is the test output: '''0354:fixme:certutil:wmain stub: L"certutil" L"-decodehex" L"seq.hex" L"seq.bin"''' If I run the test manually, seq.bin does not exist.
thanks, that looks way better. Notes: - I submitted MR!8947 to support -decodehex in certutil, so I'd rather wait for this MR to be approved before approving this one so that we can make use of certutil, - (and you'll have to remove the todo_wine after certutil is changed) - last point: it would be better to split the second commit in two: 1) the introduction of WCMD_copy_loop (explaining that it does fix COPY /a in some cases), b) the fix for TYPE command making use of WCMD_copy_loop. This would ensure that in the future, if someone does a git bisect, each single commit does a single thing. - and the commit fixing the TYPE command shall have a Wine-Bug line referring to the fixed bugzilla entry.
TIA
-
eric pouech (＠epo) -
Joe Souza
-
Joe Souza (＠JoeS209)