[PATCH 0/1] MR7131: DIR command improvements: Make /o by itself behave equal to how it does in Windows
"DIR /o" (no order specifier after /o) hasn't functioned the same as it does on Windows. This change makes the functionality of /o by itself equivalent to behavior on Windows. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7131
From: Joe Souza <jsouza(a)yahoo.com> --- programs/cmd/directory.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/programs/cmd/directory.c b/programs/cmd/directory.c index 3f4f7853710..c6cb27efb0c 100644 --- a/programs/cmd/directory.c +++ b/programs/cmd/directory.c @@ -49,7 +49,7 @@ static int file_total, dir_total, max_width; static ULONGLONG byte_total; static DISPLAYTIME dirTime; static DISPLAYORDER dirOrder; -static BOOL orderReverse, orderGroupDirs, orderGroupDirsReverse, orderByCol; +static BOOL orderReverse, orderGroupDirs, orderGroupDirsReverse, orderByCol, orderUnspecified; static BOOL paged_mode, recurse, wide, bare, lower, shortname, usernames, separator; static ULONG showattrs, attrsbits; @@ -99,7 +99,12 @@ static int __cdecl WCMD_dir_sort (const void *a, const void *b) (fileb->dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY))) { BOOL aDir = filea->dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY; - if (aDir) result = -1; + BOOL bDir = fileb->dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY; + if (aDir && bDir && orderUnspecified) { + result = lstrcmpiW(filea->cFileName, fileb->cFileName); + } else if (aDir) { + result = -1; + } else result = 1; if (orderGroupDirsReverse) result = -result; return result; @@ -678,6 +683,7 @@ RETURN_CODE WCMD_directory(WCHAR *args) orderReverse = FALSE; orderGroupDirs = FALSE; orderGroupDirsReverse = FALSE; + orderUnspecified = FALSE; showattrs = 0; attrsbits = FILE_ATTRIBUTE_HIDDEN | FILE_ATTRIBUTE_SYSTEM; @@ -742,6 +748,11 @@ RETURN_CODE WCMD_directory(WCHAR *args) break; case 'O': p = p + 1; if (*p==':') p++; /* Skip optional : */ + if (!*p) { /* /O by itself */ + orderUnspecified = TRUE; + dirOrder = Name; + orderGroupDirs = TRUE; + } while (*p && *p != '/') { WINE_TRACE("Processing subparm '%c' (in %s)\n", *p, wine_dbgstr_w(quals)); switch (*p) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7131
albeit the changes and the resulting output look functionally correct, there's actually a bigger issue: current implementation only supports one option after the /O switch I suspect /O to fallback to /O:GNE in native using multiple criteria in WCMD_dir_sort would be a cleaner way to fix this (also getting rid of a bunch of obnoxious global variables) since we're in code freeze right now, I'm not sure the multicriteria change could make its way in (have to wait after Wine 10.0 is released). Do you a real application that depends on this MR's output? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7131#note_91809
Thank you for the comments. First I would like to say that I agree with everything that you say here. My philosophy in general when working on other people's code is to 1) maintain the existing style, and 2) make the minimum changes needed in order to accomplish what you are trying to do. That said, this code was bad before I touched it. The use of these multiple global variables, and especially not initializing them when they are declared was a bit alarming to me. But, I figured, perhaps incorrectly, that this is stable, long-standing code, and who would I be to come in with a hatchet and rewrite it? Rewriting code often introduces new bugs, and if the existing code is stable, I would not want to do that. But your points are correct, so I will perhaps take a stab at cleaning up this code in the coming weeks, when I have time to do so. The fact that you are currently in code freeze will give me some time, it seems. In answer to your final question, no, there is no app that I am aware of that is relying on this functionality. It's just been a personal annoyance to me for quite some time. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7131#note_91815
This merge request was closed by Joe Souza. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7131
I checked behavior on Windows of DIR /O vs /O:GNE and your supposition appears to be correct. The output appears to be the same in both cases. Note, however, that this is not the case in Wine. The directory names are not sorted in the /O:GNE case. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7131#note_91818
I fully agree that deciding to rewrite a chunk of code for a single bug may not always the first choice to come up with note: last year, the parser/execution engine of cmd.exe has been rewritten - as it was missing a couple of needed features - and the same interrogation arose: patch the existing spaghetti, or rewrite; we went for second option. very good news that you can spend some time on extending the changes to cover multi criteria search a couple of side notes: * usually it's good practice to have tests showing the intended behaviour. but writing tests for cmd.exe is a pain (I've been here \<g\>), let me know if you need some help/advice here * you could keep this Merge Request open and update your patch(es) inside this MR when they are ready (so that we keep all discussions in one place) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7131#note_91824
Hi again Eric. I spent a bit more time this morning examining behavior on Windows again and it seems like the fallback behavior for /O is actually /O:GN. The E would be redundant and essentially overridden by N. I'm not quite sure how I would write a test for this case. Probably need to do something like create a known directory structure, pipe the DIR output to a file and then read/analyze the contents. Would need to ignore lines containing the volume name and serial number, etc. I had already closed the merge request over the weekend but it looks like I might be able to reopen again when changes are ready. Hopefully it will be intuitive to associate the later changes with this request. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7131#note_91917
since tests for cmd.exe are a bit painful to write, please find attached a first version of them the tests are in the .cmd file, and the expected output is in .cmd.exp the hard part is to keep them in sync, hence the various lines starting with '---' which are synchronisation points it looks I've been too optimistic with the multicriteria sort :-( some additional testings show /O:NG doesn't take into account the G, but /O:GGGG-N does take into account the -N part... (or /o:n-n sorts with N not -N, and /o:G-G-N doesn't take the -G into account but does take the -N into account!!!) so the option regexp (not suggesting to use regexp in the patch, for clarification only) looks more (-?g)\*(-?\[nesd\])\*, and only the first one of g and \[nesd\] is used so likely explains your remark about /O:GN vs /O:GNE the idea is to have a first patch in the MR based on the attached file: * tests shall pass on Windows * tests that fail on Wine shall be prefixed with @todo_wine@ * (feel free to adapt the attached patch to what you may find appropriate) and a second patch in the MR that fixes the Wine code (and you have to remove the @todo_wine@ to mark the tests that now pass) [diro.patch](/uploads/4bf00fbb81ddb692228f9d6ab61cee82/diro.patch) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7131#note_91990
participants (3)
-
eric pouech (＠epo) -
Joe Souza
-
Joe Souza (＠JoeS209)