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