[PATCH v5 0/2] MR11554: cmd: Canonicalize the echo of redirections and separators.
When echo:ing a command, match native behavior by only showing the last redirection on a given stream, and by adding spaces around redirections and separators. When a command has several redirections on the same stream, native only echoes the last one. Pls tell me if anything else needs to be added/removed from the tests, tried to be as minimal as possible wrt space addition/removal Fixes bug [21227](https://bugs.winehq.org/show_bug.cgi?id=21227) Signed-off-by: Lokesh Poovaragan lokesh.poovaragan@gmail.com -- v5: cmd: Move redirection deduplication to parse time. cmd/tests: Add tests for echo of redirections and separators. https://gitlab.winehq.org/wine/wine/-/merge_requests/11554
From: Lokesh Poovaragan <lokesh.poovaragan@gmail.com> When a command has several redirections on the same stream, native only echoes the last one, and separators get extra spaces around them. These are currently untested cases. Signed-off-by: Lokesh Poovaragan <lokesh.poovaragan@gmail.com> --- programs/cmd/tests/test_builtins.cmd | 2 ++ programs/cmd/tests/test_builtins.cmd.exp | 5 +++++ 2 files changed, 7 insertions(+) diff --git a/programs/cmd/tests/test_builtins.cmd b/programs/cmd/tests/test_builtins.cmd index 4ca987a8b05..416aa168948 100644 --- a/programs/cmd/tests/test_builtins.cmd +++ b/programs/cmd/tests/test_builtins.cmd @@ -32,6 +32,7 @@ echo @tab@word echo@tab@@tab@word echo @tab@ on @space@
nul echo a +> x > y echo a if@tab@1 == 2 then @echo a @rem native stores the keyword (and preserve the case) :-( IF@tab@1 == 2 ThEn @EchO a @@ -50,6 +51,7 @@ echo hidden @set V=@ %V%echo foo1 nul echo a && @echo foo2 +echo a | echo b @echo --- @echo off echo off@tab@@space@ diff --git a/programs/cmd/tests/test_builtins.cmd.exp b/programs/cmd/tests/test_builtins.cmd.exp index c7015c61b7a..2534b8e0000 100644 --- a/programs/cmd/tests/test_builtins.cmd.exp +++ b/programs/cmd/tests/test_builtins.cmd.exp @@ -91,6 +91,8 @@ word
@todo_wine@@pwd@>echo a 1>nul@space@ +@todo_wine@@pwd@>echo a 1>y@space@ + @pwd@>if 1 == 2 then @echo a@space@ @todo_wine@@pwd@>IF 1 == 2 ThEn @EchO a@space@ @@ -113,6 +115,9 @@ foo1 @todo_wine@@pwd@>echo a 1>nul &&@space@ foo2 + +@todo_wine@@pwd@>echo a | echo b@space@ +b --- noecho1 noecho2 -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11554
From: Lokesh Poovaragan <lokesh.poovaragan@gmail.com> When multiple redirections target the same stream, mark earlier ones as overridden at parse time in redirection_list_append, rather than scanning ahead in the execution and echo paths. This replaces the O(n^2) dedup loops in push_std_redirections and rebuild_append_all_redirections with a simple flag check. Also match native echo behavior by not inserting extra spaces inside block parentheses. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=21227 Signed-off-by: Lokesh Poovaragan <lokesh.poovaragan@gmail.com> --- programs/cmd/wcmd.h | 1 + programs/cmd/wcmdmain.c | 34 +++++++++++++++++++++++----------- 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/programs/cmd/wcmd.h b/programs/cmd/wcmd.h index c8d0d03e9e6..104f1dcfa51 100644 --- a/programs/cmd/wcmd.h +++ b/programs/cmd/wcmd.h @@ -39,6 +39,7 @@ typedef struct _CMD_REDIRECTION { enum CMD_REDIRECTION_KIND {REDIR_READ_FROM, REDIR_WRITE_TO, REDIR_WRITE_APPEND, REDIR_WRITE_CLONE} kind; unsigned short fd; + BOOL overridden; struct _CMD_REDIRECTION *next; union { diff --git a/programs/cmd/wcmdmain.c b/programs/cmd/wcmdmain.c index d2250ce1bbd..21fd0f52832 100644 --- a/programs/cmd/wcmdmain.c +++ b/programs/cmd/wcmdmain.c @@ -1347,6 +1347,7 @@ static CMD_REDIRECTION *redirection_create_file(enum CMD_REDIRECTION_KIND kind, redir->kind = kind; redir->fd = fd; + redir->overridden = FALSE; memcpy(redir->file, file, len * sizeof(WCHAR)); redir->next = NULL; @@ -1359,6 +1360,7 @@ static CMD_REDIRECTION *redirection_create_clone(unsigned fd, unsigned fd_clone) redir->kind = REDIR_WRITE_CLONE; redir->fd = fd; + redir->overridden = FALSE; redir->clone = fd_clone; redir->next = NULL; @@ -2159,12 +2161,7 @@ static BOOL push_std_redirections(CMD_REDIRECTION *redir, HANDLE saved[3]) saved[i] = GetStdHandle(std_index[i]); for (; redir; redir = redir->next) { - CMD_REDIRECTION *next; - - /* if we have several elements changing same std stream, only use last one */ - for (next = redir->next; next; next = next->next) - if (redir->fd == next->fd) break; - if (next) continue; + if (redir->overridden) continue; switch (redir->kind) { case REDIR_READ_FROM: @@ -2806,11 +2803,21 @@ static enum builder_token node_builder_top(const struct node_builder *builder, u static void redirection_list_append(CMD_REDIRECTION **redir, CMD_REDIRECTION *last) { - if (last) + CMD_REDIRECTION *iter, *head = last; + + if (!last) return; + + /* mark any existing redirection on the same fd as overridden */ + for (; last; last = last->next) { - for ( ; *redir; redir = &(*redir)->next) {} - *redir = last; + for (iter = *redir; iter; iter = iter->next) + { + if (iter->fd == last->fd) iter->overridden = TRUE; + } } + /* walk to end and append */ + for ( ; *redir; redir = &(*redir)->next) {} + *redir = head; } static BOOL node_builder_parse(struct node_builder *builder, unsigned precedence, CMD_NODE **result) @@ -3347,6 +3354,7 @@ static BOOL rebuild_append_all_redirections(struct command_rebuild *rb, const CM for (redir = node->redirects; ret && redir != NULL; redir = redir->next) { + if (redir->overridden) continue; if (rb->pos && !iswspace(rb->buffer[rb->pos - 1])) ret = ret && rebuild_append(rb, L" "); ret = ret && rebuild_append_redirection(rb, redir, expand); @@ -3566,9 +3574,13 @@ static BOOL rebuild_append_command(struct command_rebuild *rb, const CMD_NODE *n case CMD_BLOCK: { struct rebuild_flags new_rbflags = {.depth = rbflags.depth = 1, .in_echo = rbflags.in_echo}; - ret = rebuild_append(rb, L"( ") && + const WCHAR *open = rbflags.in_echo ? L"(" : L"( "; + const WCHAR *close = rbflags.in_echo ? L")" : L" ) "; + ret = rebuild_append(rb, open) && rebuild_append_command(rb, node->block, new_rbflags) && - rebuild_append(rb, L" ) "); + rebuild_append(rb, close); + if (rbflags.in_echo) + ret = ret && rebuild_append(rb, L" "); } break; default: -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11554
On Mon Aug 3 13:44:31 2026 +0000, eric pouech wrote:
doesn't look as bad as previous attempt... anyway I would split second patch into: 1. dedupe redirection, without adding extra space parameter; side note: since code is similar to what is done in push_std_redirections I wonder if this could be simplified either by removing overriden CMD_REDIRECTION - don't exactly remember why I haven't done it at once, could be a reason, or otherwise adding a field in CMD_REDIRECTION to mark it as overriden in redirection_list_append and just skip it in execution loops. Otherside note: unless mistaken, the bug reports seems to infer that a space is added for an overriden redirection, that doesn't seem to be inserted with your code 2. add the chunk that just changes the prefix/suffix for block (looks ok) 3. the rest (IMO that's the part that looks not so ok, or should be refactored differently): I'm more concerned about these... scattering append(L" "); makes me feel we miss a higher logic Thank you for the detailed feedback it is really helpful for learning.
I've reworked the second patch along the lines you suggested: \- Dedup via data structure, added an overridden flag to `CMD_REDIRECTION`, set in `redirection_list_append` when a same-fd redirect is appended. This replaces the scan-ahead loops in both `push_std_redirections` and `rebuild_append_all_redirections` with a simple flag check, and lets `rebuild_append_all_redirections` drop the `in_echo` parameter. \- Block parentheses, kept the ternary approach for the prefix/suffix strings. \- Spacing, dropped the `append(L" ")` calls for the operator/redirection spacing. The todo_wine markers have been added back for this. I agree this part needs a cleaner pattern; I considered always appending a trailing space after a redirection, and always putting spaces around operators, but those too seem like they wouldn't fit cases that might be encountered in the wild. I am uncertain if this could be pattern matched to fit native in a way that doesn't seem hacky (I think maybe this section is fighting against windows's own test cases for how the whitespace behaviour is tested) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11554#note_147682
looks fine; I would just split the CMD_BLOCK changes for rebuild_append_command in a third commit as it has nothing to do with redirections -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11554#note_147823
participants (3)
-
eric pouech (＠epo) -
Lokesh Poovaragan
-
Lokesh Poovaragan (＠lokeshpoovaragan)