[PATCH 0/3] MR4168: cmd: Handle caret / blocks with whitespace only lines better
This series of patches is heavily inspired by https://gitlab.winehq.org/wine/wine/-/merge_requests/277 and solves problems related to - whitespace only lines in blocks - EOF in the middle of the blocks - lines with caret followed by EOF -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4168
From: Alexander Merkle <alexander.merkle(a)lauterbach.com> Heavily inspired by https://gitlab.winehq.org/wine/wine/-/merge_requests/277 https://gitlab.winehq.org/bernhardu/wine/-/tree/51599_cmd_brackets by Bernhard Übelacker <bernhardu(a)mailbox.org> . Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51599 Signed-off-by: Alexander Merkle <alexander.merkle(a)lauterbach.com> --- programs/cmd/tests/test_builtins.cmd | 14 ++++++++++++++ programs/cmd/tests/test_builtins.cmd.exp | 3 +++ programs/cmd/wcmdmain.c | 18 +++++++++++++++++- 3 files changed, 34 insertions(+), 1 deletion(-) diff --git a/programs/cmd/tests/test_builtins.cmd b/programs/cmd/tests/test_builtins.cmd index 510a1ba5931..35caf136e4c 100644 --- a/programs/cmd/tests/test_builtins.cmd +++ b/programs/cmd/tests/test_builtins.cmd @@ -1029,6 +1029,20 @@ if not exist %windir% ( ) else ( echo windir does exist ) +if 1 == 0 ( + echo 1 == 0 should not be true +(a)space@@tab@ +) else echo block containing a line with just spaces seems to work +if 1 == 0 ( +(a)space@@tab@ + echo 1 == 0 should not be true +) else echo block containing a line with just spaces seems to work +if 1 == 0 ( +(a)space@@tab@ + echo 1 == 0 should not be true +(a)tab@ +(a)space@ +) else echo block containing a line with just spaces seems to work echo --- case sensitivity with and without /i option if bar==BAR echo if does not default to case sensitivity if not bar==BAR echo if seems to default to case sensitivity diff --git a/programs/cmd/tests/test_builtins.cmd.exp b/programs/cmd/tests/test_builtins.cmd.exp index 0f48a823109..379225f38e5 100644 --- a/programs/cmd/tests/test_builtins.cmd.exp +++ b/programs/cmd/tests/test_builtins.cmd.exp @@ -707,6 +707,9 @@ comparison operators surrounded by brackets seem to work comparison operators surrounded by brackets seem to work windir is defined windir does exist +block containing a line with just spaces seems to work +block containing a line with just spaces seems to work +block containing a line with just spaces seems to work --- case sensitivity with and without /i option if seems to default to case sensitivity if /i seems to work diff --git a/programs/cmd/wcmdmain.c b/programs/cmd/wcmdmain.c index 843fef8ea50..3759b1605da 100644 --- a/programs/cmd/wcmdmain.c +++ b/programs/cmd/wcmdmain.c @@ -1804,6 +1804,22 @@ static BOOL WCMD_IsEndQuote(const WCHAR *quote, int quoteIndex) return FALSE; } +/*************************************************************************** + * WCMD_isEmptyOrJustWhiteSpace + * + * Returns TRUE if str is empty or contains just whitespace characters, + * otherwise returns FALSE. + */ +static BOOL WCMD_isEmptyOrJustWhiteSpace(const WCHAR *str) +{ + while (*str != '\0') { + if (*str != ' ' && *str != '\t') + return FALSE; + str++; + } + return TRUE; +} + /*************************************************************************** * WCMD_ReadAndParseLine * @@ -2326,7 +2342,7 @@ WCHAR *WCMD_ReadAndParseLine(const WCHAR *optionalcmd, CMD_LIST **output, HANDLE } else break; } - } while (*extraData == 0x00); + } while (WCMD_isEmptyOrJustWhiteSpace(extraData)); curPos = extraSpace; /* Skip preceding whitespace */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4168
From: Alexander Merkle <alexander.merkle(a)lauterbach.com> as discussed in https://gitlab.winehq.org/wine/wine/-/merge_requests/277 Signed-off-by: Alexander Merkle <alexander.merkle(a)lauterbach.com> --- programs/cmd/tests/rsrc.rc | 12 ++++++++++++ programs/cmd/tests/test_block_if_eof1.cmd | 5 +++++ programs/cmd/tests/test_block_if_eof1.cmd.exp | 1 + programs/cmd/tests/test_block_if_eof2.cmd | 5 +++++ programs/cmd/tests/test_block_if_eof2.cmd.exp | 1 + programs/cmd/wcmdmain.c | 6 ++++-- 6 files changed, 28 insertions(+), 2 deletions(-) create mode 100644 programs/cmd/tests/test_block_if_eof1.cmd create mode 100644 programs/cmd/tests/test_block_if_eof1.cmd.exp create mode 100644 programs/cmd/tests/test_block_if_eof2.cmd create mode 100644 programs/cmd/tests/test_block_if_eof2.cmd.exp diff --git a/programs/cmd/tests/rsrc.rc b/programs/cmd/tests/rsrc.rc index affe04c2bdd..ca45543cfa4 100644 --- a/programs/cmd/tests/rsrc.rc +++ b/programs/cmd/tests/rsrc.rc @@ -22,6 +22,18 @@ test_builtins.cmd TESTCMD "test_builtins.cmd" /* @makedep: test_builtins.cmd.exp */ test_builtins.cmd.exp TESTOUT "test_builtins.cmd.exp" +/* @makedep: test_block_if_eof1.cmd */ +test_block_if_eof1.cmd TESTCMD "test_block_if_eof1.cmd" + +/* @makedep: test_block_if_eof1.cmd.exp */ +test_block_if_eof1.cmd.exp TESTOUT "test_block_if_eof1.cmd.exp" + +/* @makedep: test_block_if_eof2.cmd */ +test_block_if_eof2.cmd TESTCMD "test_block_if_eof2.cmd" + +/* @makedep: test_block_if_eof2.cmd.exp */ +test_block_if_eof2.cmd.exp TESTOUT "test_block_if_eof2.cmd.exp" + /* @makedep: test_cmdline.cmd */ test_cmdline.cmd TESTCMD "test_cmdline.cmd" diff --git a/programs/cmd/tests/test_block_if_eof1.cmd b/programs/cmd/tests/test_block_if_eof1.cmd new file mode 100644 index 00000000000..9b2ee2e97f3 --- /dev/null +++ b/programs/cmd/tests/test_block_if_eof1.cmd @@ -0,0 +1,5 @@ +(a)echo off +echo Test if with unbalanced parentheses + +if 1 == 1 ( + echo should not be executed diff --git a/programs/cmd/tests/test_block_if_eof1.cmd.exp b/programs/cmd/tests/test_block_if_eof1.cmd.exp new file mode 100644 index 00000000000..419b130e442 --- /dev/null +++ b/programs/cmd/tests/test_block_if_eof1.cmd.exp @@ -0,0 +1 @@ +Test if with unbalanced parentheses diff --git a/programs/cmd/tests/test_block_if_eof2.cmd b/programs/cmd/tests/test_block_if_eof2.cmd new file mode 100644 index 00000000000..2c34647a732 --- /dev/null +++ b/programs/cmd/tests/test_block_if_eof2.cmd @@ -0,0 +1,5 @@ +(a)echo off +echo Test if with unbalanced parentheses + +if 1 == 0 ( + echo should not be executed diff --git a/programs/cmd/tests/test_block_if_eof2.cmd.exp b/programs/cmd/tests/test_block_if_eof2.cmd.exp new file mode 100644 index 00000000000..419b130e442 --- /dev/null +++ b/programs/cmd/tests/test_block_if_eof2.cmd.exp @@ -0,0 +1 @@ +Test if with unbalanced parentheses diff --git a/programs/cmd/wcmdmain.c b/programs/cmd/wcmdmain.c index 3759b1605da..453da83aa2a 100644 --- a/programs/cmd/wcmdmain.c +++ b/programs/cmd/wcmdmain.c @@ -2328,8 +2328,10 @@ WCHAR *WCMD_ReadAndParseLine(const WCHAR *optionalcmd, CMD_LIST **output, HANDLE do { WINE_TRACE("Read more input\n"); if (!context) WCMD_output_asis( WCMD_LoadMessage(WCMD_MOREPROMPT)); - if (!WCMD_fgets(extraData, MAXSTRING, readFrom)) - break; + if (!WCMD_fgets(extraData, MAXSTRING, readFrom)) { + /* EOF in parentheses - abort don't execute block */ + return NULL; + } /* Edge case for carets - a completely blank line (i.e. was just CRLF) is oddly added as an LF but then more data is received (but -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4168
From: Alexander Merkle <alexander.merkle(a)lauterbach.com> --- programs/cmd/tests/rsrc.rc | 6 ++++++ programs/cmd/tests/test_caret_eof.cmd | 3 +++ programs/cmd/tests/test_caret_eof.cmd.exp | 3 +++ programs/cmd/wcmdmain.c | 6 ++++++ 4 files changed, 18 insertions(+) create mode 100644 programs/cmd/tests/test_caret_eof.cmd create mode 100644 programs/cmd/tests/test_caret_eof.cmd.exp diff --git a/programs/cmd/tests/rsrc.rc b/programs/cmd/tests/rsrc.rc index ca45543cfa4..382e297c468 100644 --- a/programs/cmd/tests/rsrc.rc +++ b/programs/cmd/tests/rsrc.rc @@ -34,6 +34,12 @@ test_block_if_eof2.cmd TESTCMD "test_block_if_eof2.cmd" /* @makedep: test_block_if_eof2.cmd.exp */ test_block_if_eof2.cmd.exp TESTOUT "test_block_if_eof2.cmd.exp" +/* @makedep: test_caret_eof.cmd */ +test_caret_eof.cmd TESTCMD "test_caret_eof.cmd" + +/* @makedep: test_caret_eof.cmd.exp */ +test_caret_eof.cmd.exp TESTOUT "test_caret_eof.cmd.exp" + /* @makedep: test_cmdline.cmd */ test_cmdline.cmd TESTCMD "test_cmdline.cmd" diff --git a/programs/cmd/tests/test_caret_eof.cmd b/programs/cmd/tests/test_caret_eof.cmd new file mode 100644 index 00000000000..d0157171925 --- /dev/null +++ b/programs/cmd/tests/test_caret_eof.cmd @@ -0,0 +1,3 @@ +(a)echo off +echo Test caret followed by EOF +echo foo^ diff --git a/programs/cmd/tests/test_caret_eof.cmd.exp b/programs/cmd/tests/test_caret_eof.cmd.exp new file mode 100644 index 00000000000..9c99c033e6b --- /dev/null +++ b/programs/cmd/tests/test_caret_eof.cmd.exp @@ -0,0 +1,3 @@ +Test caret followed by EOF +foo + diff --git a/programs/cmd/wcmdmain.c b/programs/cmd/wcmdmain.c index 453da83aa2a..2a12014fcde 100644 --- a/programs/cmd/wcmdmain.c +++ b/programs/cmd/wcmdmain.c @@ -2330,6 +2330,12 @@ WCHAR *WCMD_ReadAndParseLine(const WCHAR *optionalcmd, CMD_LIST **output, HANDLE if (!context) WCMD_output_asis( WCMD_LoadMessage(WCMD_MOREPROMPT)); if (!WCMD_fgets(extraData, MAXSTRING, readFrom)) { /* EOF in parentheses - abort don't execute block */ + /* caret followed by EOF - execute line */ + if (lastWasCaret && (curDepth == 0) && (*extraSpace == 0x00)) { + *extraData++ = '\r'; + *extraData = 0x00; + break; + } return NULL; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4168
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 tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=139024 Your paranoid android. === w7u_2qxl (32 bit report) === cmd.exe: batch.c:347: Test failed: unexpected end of output in line 2, missing === w7u_adm (32 bit report) === cmd.exe: batch.c:347: Test failed: unexpected end of output in line 2, missing === w7u_el (32 bit report) === cmd.exe: batch.c:347: Test failed: unexpected end of output in line 2, missing === w8 (32 bit report) === cmd.exe: batch.c:347: Test failed: unexpected end of output in line 2, missing === w8adm (32 bit report) === cmd.exe: batch.c:347: Test failed: unexpected end of output in line 2, missing === w864 (32 bit report) === cmd.exe: batch.c:347: Test failed: unexpected end of output in line 2, missing === w1064v1507 (32 bit report) === cmd.exe: batch.c:347: Test failed: unexpected end of output in line 2, missing === w1064v1809 (32 bit report) === cmd.exe: batch.c:347: Test failed: unexpected end of output in line 2, missing === w1064_tsign (32 bit report) === cmd.exe: batch.c:347: Test failed: unexpected end of output in line 2, missing === w10pro64 (32 bit report) === cmd.exe: batch.c:347: Test failed: unexpected end of output in line 2, missing === w10pro64_en_AE_u8 (32 bit report) === cmd.exe: batch.c:347: Test failed: unexpected end of output in line 2, missing === w11pro64 (32 bit report) === cmd.exe: batch.c:347: Test failed: unexpected end of output in line 2, missing === w7pro64 (64 bit report) === cmd.exe: batch.c:89: Test failed: CreateFile failed batch.c:89: Test failed: CreateFile failed batch.c:89: Test failed: CreateFile failed batch.c:89: Test failed: CreateFile failed === w864 (64 bit report) === cmd.exe: batch.c:347: Test failed: unexpected end of output in line 2, missing === w1064v1507 (64 bit report) === cmd.exe: batch.c:347: Test failed: unexpected end of output in line 2, missing === w1064v1809 (64 bit report) === cmd.exe: batch.c:347: Test failed: unexpected end of output in line 2, missing === w1064_2qxl (64 bit report) === cmd.exe: batch.c:347: Test failed: unexpected end of output in line 2, missing === w1064_adm (64 bit report) === cmd.exe: batch.c:347: Test failed: unexpected end of output in line 2, missing === w1064_tsign (64 bit report) === cmd.exe: batch.c:347: Test failed: unexpected end of output in line 2, missing === w10pro64 (64 bit report) === cmd.exe: batch.c:347: Test failed: unexpected end of output in line 2, missing === w10pro64_ar (64 bit report) === cmd.exe: batch.c:347: Test failed: unexpected end of output in line 2, missing === w10pro64_ja (64 bit report) === cmd.exe: batch.c:347: Test failed: unexpected end of output in line 2, missing === w10pro64_zh_CN (64 bit report) === cmd.exe: batch.c:347: Test failed: unexpected end of output in line 2, missing === w11pro64_amd (64 bit report) === cmd.exe: batch.c:347: Test failed: unexpected end of output in line 2, missing
eric pouech (@epo) commented about programs/cmd/tests/test_builtins.cmd.exp:
comparison operators surrounded by brackets seem to work windir is defined windir does exist +block containing a line with just spaces seems to work +block containing a line with just spaces seems to work +block containing a line with just spaces seems to work --- case sensitivity with and without /i option there's something wrong here the the "echo --- case..." command gets lots in the output
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4168#note_49639
eric pouech (@epo) commented about programs/cmd/wcmdmain.c:
} else break; }
- } while (*extraData == 0x00);
what about using WCMD_skip_leading_spaces() instead? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4168#note_49640
eric pouech (@epo) commented about programs/cmd/tests/rsrc.rc:
/* @makedep: test_builtins.cmd.exp */ test_builtins.cmd.exp TESTOUT "test_builtins.cmd.exp"
+/* @makedep: test_block_if_eof1.cmd */ adding separated test cases looks a bit overkill could you do create the test files from withing the existing .cmd file (search for tmp.cmd for examples)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4168#note_49641
eric pouech (@epo) commented about programs/cmd/wcmdmain.c:
if (!context) WCMD_output_asis( WCMD_LoadMessage(WCMD_MOREPROMPT)); if (!WCMD_fgets(extraData, MAXSTRING, readFrom)) { /* EOF in parentheses - abort don't execute block */ + /* caret followed by EOF - execute line */
I wonder if this part is actually covered by your tests (not sure it should as it requires to have the exact right length before) so I wonder if native cmd has the same behavior -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4168#note_49642
On Tue Oct 24 17:51:17 2023 +0000, eric pouech wrote:
what about using WCMD_skip_leading_spaces() instead? Doable, would result in
} while (!*WCMD_skip_leading_spaces(extraData));
I will make this part of the next push, I can integrate(fixup) this in the final force-push. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4168#note_49728
On Tue Oct 24 17:51:15 2023 +0000, eric pouech wrote:
there's something wrong here the the "echo --- case..." command gets lots in the output Sorry, I didn't get that. Is there a fail in the CI / tests that I missed?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4168#note_49729
On Tue Oct 24 17:51:19 2023 +0000, eric pouech wrote:
I wonder if this part is actually covered by your tests (not sure it should as it requires to have the exact right length before) so I wonder if native cmd has the same behavior That was the purpose of `test_caret_eof` - I think this is a very rare cornercase - I just added is for completeness. Or the meaning of `caret` something different than the line continuation marker `^` here?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4168#note_49730
participants (4)
-
Alexander Merkle -
eric pouech (＠epo)
-
Marvin -
userid0x0 (＠userid0x0)