[PATCH 0/2] MR6885: cmd: Use the console output codepage to read batch files.

[Akihiro Sagawa (＠sgwaki)]

To prevent performance degradation, we will cache the result returned by GetConsoleOutputCP() after executing any external command.

[PATCH 1/2] cmd/tests: Add tests for batch files encoded in UTF-8.

[Akihiro Sagawa]

From: Akihiro Sagawa

---
 programs/cmd/tests/batch.c           | 26 +++++++++++++++++++++++++-
 programs/cmd/tests/rsrc.rc           |  6 ++++++
 programs/cmd/tests/test_utf8.cmd     |  4 ++++
 programs/cmd/tests/test_utf8.cmd.exp |  2 ++
 4 files changed, 37 insertions(+), 1 deletion(-)
 create mode 100644 programs/cmd/tests/test_utf8.cmd
 create mode 100644 programs/cmd/tests/test_utf8.cmd.exp

diff --git a/programs/cmd/tests/batch.c b/programs/cmd/tests/batch.c
index 6bc5e916ea9..c6016733709 100644
--- a/programs/cmd/tests/batch.c
+++ b/programs/cmd/tests/batch.c
@@ -500,6 +500,28 @@ void create_nul_test_file(void)
     CloseHandle(file);
 }

+static void test_utf8_cmd(void)
+{
+    UINT prev_cp = GetConsoleOutputCP();
+    BOOL ret;
+
+    if (!prev_cp) {
+        /* Detach Wine's shell-no-window kind of console.
+           See test_Console() in kernel32/tests/process.c for details. */
+        FreeConsole();
+        AllocConsole();
+        prev_cp = GetConsoleOutputCP();
+    }
+    ok(prev_cp, "Can't get the console codepage, err %lu\n", GetLastError());
+
+    ret = SetConsoleOutputCP(CP_UTF8);
+    ok(ret, "Can't update the console codepage, err %lu\n", GetLastError());
+
+    EnumResourceNamesA(NULL, "TESTCMD_UTF8", test_enum_proc, 0);
+
+    SetConsoleOutputCP(prev_cp);
+}
+
 START_TEST(batch)
 {
     int argc;
@@ -529,8 +551,10 @@ START_TEST(batch)
     argc = winetest_get_mainargs(&argv);
     if(argc > 2)
         run_from_file(argv[2]);
-    else
+    else {
         EnumResourceNamesA(NULL, "TESTCMD", test_enum_proc, 0);
+        test_utf8_cmd();
+    }

DeleteFileA("nul_test_file");
 }
diff --git a/programs/cmd/tests/rsrc.rc b/programs/cmd/tests/rsrc.rc
index 0336863963f..6626c968a68 100644
--- a/programs/cmd/tests/rsrc.rc
+++ b/programs/cmd/tests/rsrc.rc
@@ -33,3 +33,9 @@ test_cmdline.cmd TESTCMD "test_cmdline.cmd"

/* @makedep: test_cmdline.cmd.exp */
 test_cmdline.cmd.exp TESTOUT "test_cmdline.cmd.exp"
+
+/* @makedep: test_utf8.cmd */
+test_utf8.cmd TESTCMD_UTF8 "test_utf8.cmd"
+
+/* @makedep: test_utf8.cmd.exp */
+test_utf8.cmd.exp TESTOUT "test_utf8.cmd.exp"
diff --git a/programs/cmd/tests/test_utf8.cmd b/programs/cmd/tests/test_utf8.cmd
new file mode 100644
index 00000000000..c71e51b5ee6
--- /dev/null
+++ b/programs/cmd/tests/test_utf8.cmd
@@ -0,0 +1,4 @@
+@echo off
+echo --- Test 1
+set ALPHA=α
+if %ALPHA:~0,1% == α (echo alpha) else echo not alpha
diff --git a/programs/cmd/tests/test_utf8.cmd.exp b/programs/cmd/tests/test_utf8.cmd.exp
new file mode 100644
index 00000000000..0b767fed5d9
--- /dev/null
+++ b/programs/cmd/tests/test_utf8.cmd.exp
@@ -0,0 +1,2 @@
+--- Test 1
+@todo_wine@alpha

[PATCH 2/2] cmd: Use the console output codepage to read batch files.

[Akihiro Sagawa]

From: Akihiro Sagawa

To prevent performance degradation, we will cache the result returned
by GetConsoleOutputCP() after executing any external command.
---
 programs/cmd/batch.c                 | 20 ++++++++++++++++----
 programs/cmd/builtins.c              |  1 +
 programs/cmd/tests/test_utf8.cmd.exp |  2 +-
 programs/cmd/wcmd.h                  |  1 +
 programs/cmd/wcmdmain.c              |  6 ++++++
 5 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/programs/cmd/batch.c b/programs/cmd/batch.c
index 15c01de4b89..89789cd0223 100644
--- a/programs/cmd/batch.c
+++ b/programs/cmd/batch.c
@@ -24,6 +24,8 @@

WINE_DEFAULT_DEBUG_CHANNEL(cmd);

+static UINT file_codepage; /* codepage for reading batch file */
+
 static RETURN_CODE WCMD_batch_main_loop(void)
 {
     RETURN_CODE return_code = NO_ERROR;
@@ -200,6 +202,18 @@ WCHAR *WCMD_parameter (WCHAR *s, int n, WCHAR **start, BOOL raw,
   return WCMD_parameter_with_delims (s, n, start, raw, wholecmdline, L" \t,=;");
 }

+/****************************************************************************
+ * WCMD_update_file_codepage
+ *
+ * Update a file codepage used in WCMD_fgets.
+ * Since GetConsoleOutputCP is slow, we cache the result in advance.
+ */
+void WCMD_update_file_codepage(void)
+{
+    UINT cp = GetConsoleOutputCP();
+    file_codepage = cp ? cp : CP_OEMCP;
+}
+
 /****************************************************************************
  * WCMD_fgets
  *
@@ -234,10 +248,8 @@ WCHAR *WCMD_fgets(WCHAR *buf, DWORD noChars, HANDLE h)
   else {
       LARGE_INTEGER filepos;
       char *bufA;
-      UINT cp;
       const char *p;

-      cp = GetOEMCP();
       bufA = xalloc(noChars);

/* Save current file position */
@@ -251,7 +263,7 @@ WCHAR *WCMD_fgets(WCHAR *buf, DWORD noChars, HANDLE h)
       }

/* Find first EOL */
-      for (p = bufA; p < (bufA + charsRead); p = CharNextExA(cp, p, 0)) {
+      for (p = bufA; p < (bufA + charsRead); p = CharNextExA(file_codepage, p, 0)) {
           if (*p == '\n' || *p == '\r')
               break;
       }
@@ -260,7 +272,7 @@ WCHAR *WCMD_fgets(WCHAR *buf, DWORD noChars, HANDLE h)
       filepos.QuadPart += p - bufA + 1 + (*p == '\r' ? 1 : 0);
       SetFilePointerEx(h, filepos, NULL, FILE_BEGIN);

-      i = MultiByteToWideChar(cp, 0, bufA, p - bufA, buf, noChars);
+      i = MultiByteToWideChar(file_codepage, 0, bufA, p - bufA, buf, noChars);
       free(bufA);
   }

diff --git a/programs/cmd/builtins.c b/programs/cmd/builtins.c
index d7c5090d17f..d1a646b98a2 100644
--- a/programs/cmd/builtins.c
+++ b/programs/cmd/builtins.c
@@ -3393,6 +3393,7 @@ RETURN_CODE WCMD_start(WCHAR *args)
         WaitForSingleObject( pi.hProcess, INFINITE );
         GetExitCodeProcess( pi.hProcess, &exit_code );
         errorlevel = (exit_code == STILL_ACTIVE) ? NO_ERROR : exit_code;
+        WCMD_update_file_codepage();
         CloseHandle(pi.hProcess);
         CloseHandle(pi.hThread);
     }
diff --git a/programs/cmd/tests/test_utf8.cmd.exp b/programs/cmd/tests/test_utf8.cmd.exp
index 0b767fed5d9..4ac22345138 100644
--- a/programs/cmd/tests/test_utf8.cmd.exp
+++ b/programs/cmd/tests/test_utf8.cmd.exp
@@ -1,2 +1,2 @@
 --- Test 1
-@todo_wine@alpha
+alpha
diff --git a/programs/cmd/wcmd.h b/programs/cmd/wcmd.h
index caacd44995d..031f08f7088 100644
--- a/programs/cmd/wcmd.h
+++ b/programs/cmd/wcmd.h
@@ -199,6 +199,7 @@ RETURN_CODE WCMD_volume(void);
 RETURN_CODE WCMD_mklink(WCHAR *args);
 RETURN_CODE WCMD_change_drive(WCHAR drive);

+void WCMD_update_file_codepage (void);
 WCHAR *WCMD_fgets (WCHAR *buf, DWORD n, HANDLE stream);
 WCHAR *WCMD_parameter (WCHAR *s, int n, WCHAR **start, BOOL raw, BOOL wholecmdline);
 WCHAR *WCMD_parameter_with_delims (WCHAR *s, int n, WCHAR **start, BOOL raw,
diff --git a/programs/cmd/wcmdmain.c b/programs/cmd/wcmdmain.c
index 1018fd56624..42c6d2d33ef 100644
--- a/programs/cmd/wcmdmain.c
+++ b/programs/cmd/wcmdmain.c
@@ -1373,7 +1373,10 @@ static RETURN_CODE run_full_path(const WCHAR *file, WCHAR *full_cmdline, BOOL ca
     }

if (!interactive || (console && !HIWORD(console)))
+    {
         WaitForSingleObject(handle, INFINITE);
+        WCMD_update_file_codepage();
+    }
     GetExitCodeProcess(handle, &exit_code);
     errorlevel = (exit_code == STILL_ACTIVE) ? NO_ERROR : exit_code;

@@ -3890,6 +3893,9 @@ int __cdecl wmain (int argc, WCHAR *argvW[])
   forloopcontext = NULL;
   WCMD_save_for_loop_context(TRUE);

+  /* init batch file codepage */
+  WCMD_update_file_codepage();
+
   /* Can't use argc/argv as it will have stripped quotes from parameters
    * meaning cmd.exe /C echo "quoted string" is impossible
    */

[PATCH 2/2] cmd: Use the console output codepage to read batch files.

[Marvin]

Hi,

It looks like your patch introduced the new failures shown below.
Please investigate and fix them before resubmitting your patch.
If they are not new, fixing them anyway would help a lot. Otherwise
please ask for the known failures list to be updated.

The full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=149880

Your paranoid android.

=== build (build log) ===

error: patch failed: programs/cmd/tests/batch.c:500
error: patch failed: programs/cmd/tests/rsrc.rc:33
error: patch failed: programs/cmd/tests/test_utf8.cmd.exp:1
Task: Patch failed to apply

=== debian11 (build log) ===

error: patch failed: programs/cmd/tests/batch.c:500
error: patch failed: programs/cmd/tests/rsrc.rc:33
error: patch failed: programs/cmd/tests/test_utf8.cmd.exp:1
Task: Patch failed to apply

=== debian11b (build log) ===

error: patch failed: programs/cmd/tests/batch.c:500
error: patch failed: programs/cmd/tests/rsrc.rc:33
error: patch failed: programs/cmd/tests/test_utf8.cmd.exp:1
Task: Patch failed to apply

[eric pouech (＠epo)]

I'm puzzled by a couple of items:

* use of (Get|Set)ConsoleOutputCP(), while the changes are mostly on input, not output
* changing SetConsoleOutputCP() into SetConsoleCP() let the test also pass
* I would have preferred toying with the CP inside the .cmd files themselves, but 'chcp 65001' as a command doesn't help here (as I hoped it would; and yet does it set the console cp to utf8)

so I feel like we're missing something

[Akihiro Sagawa (＠sgwaki)]

On Sun Nov 24 12:41:00 2024 +0000, eric pouech wrote:
> I'm puzzled by a couple of items:
> - use of (Get|Set)ConsoleOutputCP(), while the changes are mostly on
>
> input, not output
> - changing SetConsoleOutputCP() into SetConsoleCP() let the test also pass
> - I would have preferred toying with the CP inside the .cmd files
>
> themselves, but 'chcp 65001' as a command doesn't help here (as I hoped
> it would; and yet does it set the console cp to utf8)
> so I feel like we're missing something

Thank you for reviewing.
> - use of (Get|Set)ConsoleOutputCP(), while the changes are mostly on input, not output

Yes, this is counter intuitive. So why not think of (Get|Set)ConsoleCP() as controlling keyboard input and (Get|Set)ConsoleOutputCP() as controlling the rest?
> - changing SetConsoleOutputCP() into SetConsoleCP() let the test also pass

That surprises me. I have tried as well and see some failures --- see https://testbot.winehq.org/JobDetails.pl?Key=149930 .
> - I would have preferred toying with the CP inside the .cmd files themselves, but 'chcp 65001' as a command doesn't help here (as I hoped it would; and yet does it set the console cp to utf8)

I was similarly perplexed. Though the original code that showed symptoms called chcp, I prefer to call SetConsoleOuputCP() explicitly to avoid confusion between ConsoleCP and ConsoleOutputCP. Don't you think so?

[eric pouech (＠epo)]

> That surprises me. I have tried as well and see some failures --- see https://testbot.winehq.org/JobDetails.pl?Key=149930 .

sorry, looks like I messed up with my tests.
> I was similarly perplexed. Though the original code that showed symptoms called chcp, I prefer to call SetConsoleOuputCP() explicitly to avoid confusion between ConsoleCP and ConsoleOutputCP. Don't you think so?

as your original test case comes from using chcp (which sets both cp), and all the testing done shows that cmd.exe relies on output cp, I don't think it matters too much

one the previous failure I was seeing comes from the fact that native cmd in cp65001 interprets CR (alone) wrongly and fails parsing (so be sure to always use CRLF <g>)

I've tried to generate the test files inside the current tests, and made some interesting progress:

https://testbot.winehq.org/JobDetails.pl?Key=149935

yes it's very convoluted to say the least, but we need to:

* run the test in another cmd instance so that we don't temper current cmd's instance codepage
* ensure that the lines that generate the test.cmd (which contain multibyte characters for the 65001 cp) are not wrongly interpreted in current cp

even if it's convoluted, I'd rather go that way as we control all bits from withing the tests_*.cmd and don't rely on external parameters