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.