[PATCH v4 0/3] MR6891: cmd: Use CD /D to change directory in POPD (same as PUSHD).
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=57137 -- v4: cmd: Use CD /D to change directory in POPD (same as PUSHD). cmd: Refactor CD command. https://gitlab.winehq.org/wine/wine/-/merge_requests/6891
From: huangqinjin <huangqinjin(a)gmail.com> --- programs/cmd/tests/test_builtins.bat | 1 + programs/cmd/tests/test_builtins.bat.exp | 1 + programs/cmd/tests/test_builtins.cmd | 1 + programs/cmd/tests/test_builtins.cmd.exp | 1 + 4 files changed, 4 insertions(+) diff --git a/programs/cmd/tests/test_builtins.bat b/programs/cmd/tests/test_builtins.bat index a95c94c5ad8..a1b7b46a7f6 100644 --- a/programs/cmd/tests/test_builtins.bat +++ b/programs/cmd/tests/test_builtins.bat @@ -155,6 +155,7 @@ call :setError 666 & (pushd abc &&echo SUCCESS !errorlevel!||echo FAILURE !error call :setError 666 & (popd abc &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) call :setError 666 & (popd &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) call :setError 666 & popd & echo ERRORLEVEL !errorlevel! +cd abc & pushd .. & rmdir abc & (popd &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) cd .. && rd /q /s foo echo --- success/failure for DIR command diff --git a/programs/cmd/tests/test_builtins.bat.exp b/programs/cmd/tests/test_builtins.bat.exp index a2cbe0cecd7..6b131085120 100644 --- a/programs/cmd/tests/test_builtins.bat.exp +++ b/programs/cmd/tests/test_builtins.bat.exp @@ -107,6 +107,7 @@ FAILURE 1 SUCCESS 666 FAILURE 1 ERRORLEVEL 666 +(a)todo_wine@FAILURE 3 --- success/failure for DIR command FAILURE 1 FAILURE 1 diff --git a/programs/cmd/tests/test_builtins.cmd b/programs/cmd/tests/test_builtins.cmd index 7a2953b735d..d7b04f503df 100644 --- a/programs/cmd/tests/test_builtins.cmd +++ b/programs/cmd/tests/test_builtins.cmd @@ -619,6 +619,7 @@ call :setError 666 & (pushd abc &&echo SUCCESS !errorlevel!||echo FAILURE !error call :setError 666 & (popd abc &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) call :setError 666 & (popd &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) call :setError 666 & popd & echo ERRORLEVEL !errorlevel! +cd abc & pushd .. & rmdir abc & (popd &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) cd .. && rd /q /s foo echo --- success/failure for DIR command diff --git a/programs/cmd/tests/test_builtins.cmd.exp b/programs/cmd/tests/test_builtins.cmd.exp index 3c516b79fe5..03e404a0366 100644 --- a/programs/cmd/tests/test_builtins.cmd.exp +++ b/programs/cmd/tests/test_builtins.cmd.exp @@ -558,6 +558,7 @@ FAILURE 1 SUCCESS 666 FAILURE 1 ERRORLEVEL 666 +(a)todo_wine@FAILURE 3 --- success/failure for DIR command FAILURE 1 FAILURE 1 -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6891
From: huangqinjin <huangqinjin(a)gmail.com> --- programs/cmd/builtins.c | 165 +++++++++++++++++++++++++--------------- 1 file changed, 102 insertions(+), 63 deletions(-) diff --git a/programs/cmd/builtins.c b/programs/cmd/builtins.c index d7c5090d17f..80d021c9473 100644 --- a/programs/cmd/builtins.c +++ b/programs/cmd/builtins.c @@ -1750,6 +1750,15 @@ RETURN_CODE WCMD_goto(void) return ERROR_INVALID_FUNCTION; } +/***************************************************************************** + * WCMD_cd_set_env + * + * Change current directory and set special environment variable =C:. + * Restore to cwd after cd if supplied. + */ + +static RETURN_CODE WCMD_cd_set_env(const WCHAR *args, const WCHAR *cwd); + /***************************************************************************** * WCMD_pushd * @@ -2307,12 +2316,7 @@ RETURN_CODE WCMD_endlocal(void) RETURN_CODE WCMD_setshow_default(const WCHAR *args) { RETURN_CODE return_code; - BOOL status; - WCHAR string[1024]; WCHAR cwd[1024]; - WCHAR *pos; - WIN32_FIND_DATAW fd; - HANDLE hff; WINE_TRACE("Request change to directory '%s'\n", wine_dbgstr_w(args)); @@ -2334,78 +2338,113 @@ RETURN_CODE WCMD_setshow_default(const WCHAR *args) WCMD_output_asis (cwd); } else { - /* Remove any double quotes, which may be in the - middle, eg. cd "C:\Program Files"\Microsoft is ok */ - pos = string; - while (*args) { - if (*args != '"') *pos++ = *args; - args++; + /* Restore old directory if drive letter would change, and + CD x:\directory /D (or pushd c:\directory) not supplied */ + if ((wcsstr(quals, L"/D") == NULL) && + (param1[1] == ':') && (towupper(param1[0]) != towupper(cwd[0]))) { + return_code = WCMD_cd_set_env(args, cwd); + } + else { + return_code = WCMD_cd_set_env(args, NULL); } - while (pos > string && (*(pos-1) == ' ' || *(pos-1) == '\t')) - pos--; - *pos = 0x00; + } - /* Search for appropriate directory */ - WINE_TRACE("Looking for directory '%s'\n", wine_dbgstr_w(string)); - hff = FindFirstFileW(string, &fd); - if (hff != INVALID_HANDLE_VALUE) { - do { - if (fd.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) { - WCHAR fpath[MAX_PATH]; - WCHAR drive[10]; - WCHAR dir[MAX_PATH]; - WCHAR fname[MAX_PATH]; - WCHAR ext[MAX_PATH]; + if (return_code == ERROR_PATH_NOT_FOUND) { + WCMD_print_error(); + } - /* Convert path into actual directory spec */ - if (!WCMD_get_fullpath(string, ARRAY_SIZE(fpath), fpath, NULL)) - return errorlevel = ERROR_INVALID_FUNCTION; + return return_code == NO_ERROR ? + (errorlevel = NO_ERROR) : (errorlevel = ERROR_INVALID_FUNCTION); +} - _wsplitpath(fpath, drive, dir, fname, ext); +/***************************************************************************** + * WCMD_cd_set_env + * + * Change current directory and set special environment variable =C:. + * Restore to cwd after cd if supplied. + */ - /* Rebuild path */ - wsprintfW(string, L"%s%s%s", drive, dir, fd.cFileName); - break; - } - } while (FindNextFileW(hff, &fd) != 0); - FindClose(hff); - } +static RETURN_CODE WCMD_cd_set_env(const WCHAR *args, const WCHAR *cwd) +{ + RETURN_CODE return_code; + BOOL status; + WCHAR string[1024]; + WCHAR *pos; + WIN32_FIND_DATAW fd; + HANDLE hff; - /* Change to that directory */ - WINE_TRACE("Really changing to directory '%s'\n", wine_dbgstr_w(string)); + return_code = NO_ERROR; - status = SetCurrentDirectoryW(string); - if (!status) { - WCMD_print_error (); - return_code = ERROR_INVALID_FUNCTION; - } else { + /* Remove any double quotes, which may be in the + middle, eg. cd "C:\Program Files"\Microsoft is ok */ + pos = string; + while (*args) { + if (*args != '"') *pos++ = *args; + args++; + } + while (pos > string && (*(pos-1) == ' ' || *(pos-1) == '\t')) + pos--; + *pos = 0x00; + + /* Search for appropriate directory */ + WINE_TRACE("Looking for directory '%s'\n", wine_dbgstr_w(string)); + hff = FindFirstFileW(string, &fd); + if (hff != INVALID_HANDLE_VALUE) { + do { + if (fd.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) { + WCHAR fpath[MAX_PATH]; + WCHAR drive[10]; + WCHAR dir[MAX_PATH]; + WCHAR fname[MAX_PATH]; + WCHAR ext[MAX_PATH]; - /* Save away the actual new directory, to store as current location */ - GetCurrentDirectoryW(ARRAY_SIZE(string), string); + /* Convert path into actual directory spec */ + if (!WCMD_get_fullpath(string, ARRAY_SIZE(fpath), fpath, NULL)) { + FindClose(hff); + return ERROR_INVALID_FUNCTION; + } + _wsplitpath(fpath, drive, dir, fname, ext); - /* Restore old directory if drive letter would change, and - CD x:\directory /D (or pushd c:\directory) not supplied */ - if ((wcsstr(quals, L"/D") == NULL) && - (param1[1] == ':') && (towupper(param1[0]) != towupper(cwd[0]))) { - SetCurrentDirectoryW(cwd); + /* Rebuild path */ + wsprintfW(string, L"%s%s%s", drive, dir, fd.cFileName); + break; } - } + } while (FindNextFileW(hff, &fd) != 0); + FindClose(hff); + } + + /* Change to that directory */ + WINE_TRACE("Really changing to directory '%s'\n", wine_dbgstr_w(string)); + + status = SetCurrentDirectoryW(string); + if (!status) { + return_code = ERROR_PATH_NOT_FOUND; + } else { + + /* Save away the actual new directory, to store as current location */ + GetCurrentDirectoryW(ARRAY_SIZE(string), string); - /* Set special =C: type environment variable, for drive letter of - change of directory, even if path was restored due to missing - /D (allows changing drive letter when not resident on that - drive */ - if ((string[1] == ':') && IsCharAlphaW(string[0])) { - WCHAR env[4]; - lstrcpyW(env, L"="); - memcpy(env+1, string, 2 * sizeof(WCHAR)); - env[3] = 0x00; - WINE_TRACE("Setting '%s' to '%s'\n", wine_dbgstr_w(env), wine_dbgstr_w(string)); - SetEnvironmentVariableW(env, string); + /* Restore old directory if drive letter would change, and + CD x:\directory /D (or pushd c:\directory) not supplied */ + if (cwd != NULL) { + SetCurrentDirectoryW(cwd); } + } + /* Set special =C: type environment variable, for drive letter of + change of directory, even if path was restored due to missing + /D (allows changing drive letter when not resident on that + drive */ + if ((string[1] == ':') && IsCharAlphaW(string[0])) { + WCHAR env[4]; + lstrcpyW(env, L"="); + memcpy(env+1, string, 2 * sizeof(WCHAR)); + env[3] = 0x00; + WINE_TRACE("Setting '%s' to '%s'\n", wine_dbgstr_w(env), wine_dbgstr_w(string)); + SetEnvironmentVariableW(env, string); } - return errorlevel = return_code; + + return return_code; } /**************************************************************************** -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6891
From: huangqinjin <huangqinjin(a)gmail.com> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=57137 --- programs/cmd/builtins.c | 6 ++++-- programs/cmd/tests/test_builtins.bat.exp | 2 +- programs/cmd/tests/test_builtins.cmd.exp | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/programs/cmd/builtins.c b/programs/cmd/builtins.c index 80d021c9473..04367da9648 100644 --- a/programs/cmd/builtins.c +++ b/programs/cmd/builtins.c @@ -1815,6 +1815,7 @@ RETURN_CODE WCMD_pushd(const WCHAR *args) RETURN_CODE WCMD_popd(void) { + RETURN_CODE return_code; struct env_stack *temp = pushd_directories; if (!pushd_directories) @@ -1822,10 +1823,11 @@ RETURN_CODE WCMD_popd(void) /* pop the old environment from the stack, and make it the current dir */ pushd_directories = temp->next; - SetCurrentDirectoryW(temp->strings); + return_code = WCMD_cd_set_env(temp->strings, NULL); free(temp->strings); free(temp); - return NO_ERROR; + return return_code == NO_ERROR ? + return_code : (errorlevel = return_code); } /**************************************************************************** diff --git a/programs/cmd/tests/test_builtins.bat.exp b/programs/cmd/tests/test_builtins.bat.exp index 6b131085120..1dd24b8a9e4 100644 --- a/programs/cmd/tests/test_builtins.bat.exp +++ b/programs/cmd/tests/test_builtins.bat.exp @@ -107,7 +107,7 @@ FAILURE 1 SUCCESS 666 FAILURE 1 ERRORLEVEL 666 -(a)todo_wine@FAILURE 3 +FAILURE 3 --- success/failure for DIR command FAILURE 1 FAILURE 1 diff --git a/programs/cmd/tests/test_builtins.cmd.exp b/programs/cmd/tests/test_builtins.cmd.exp index 03e404a0366..a052ee16699 100644 --- a/programs/cmd/tests/test_builtins.cmd.exp +++ b/programs/cmd/tests/test_builtins.cmd.exp @@ -558,7 +558,7 @@ FAILURE 1 SUCCESS 666 FAILURE 1 ERRORLEVEL 666 -(a)todo_wine@FAILURE 3 +FAILURE 3 --- success/failure for DIR command FAILURE 1 FAILURE 1 -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6891
eric pouech (@epo) commented about programs/cmd/builtins.c:
+ status = SetCurrentDirectoryW(string); + if (!status) { + return_code = ERROR_PATH_NOT_FOUND; + } else { + + /* Save away the actual new directory, to store as current location */ + GetCurrentDirectoryW(ARRAY_SIZE(string), string); + + /* Restore old directory if drive letter would change, and + CD x:\directory /D (or pushd c:\directory) not supplied */ + if (cwd != NULL) { + SetCurrentDirectoryW(cwd); } + }
+ /* Set special =C: type environment variable, for drive letter of it looks wrong to set the =C: env variable when SetCurrentDirectory fails (yes, it's what the existing code is doing, but it's a good opportunity to fix it)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6891#note_89016
eric pouech (@epo) commented about programs/cmd/tests/test_builtins.bat:
call :setError 666 & (popd abc &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) call :setError 666 & (popd &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) call :setError 666 & popd & echo ERRORLEVEL !errorlevel! +cd abc & pushd .. & rmdir abc & (popd &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) please keep the same form as the other tests here:
`cd abc & pushd .. & rmdir abc` `call :SetError 666 & (popd &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!)` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6891#note_89017
eric pouech (@epo) commented about programs/cmd/tests/test_builtins.cmd:
call :setError 666 & (popd abc &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) call :setError 666 & (popd &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) call :setError 666 & popd & echo ERRORLEVEL !errorlevel! +cd abc & pushd .. & rmdir abc & (popd &&echo SUCCESS !errorlevel!||echo FAILURE !errorlevel!) same comment here
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6891#note_89018
(assuming you're done with the update, we usually wait for the threads to be resolved) the title of the third patch should be updated Note: I'm fine with the logic of the MR as it is (ie accept it when points listed above are fixed). However, I think it could be further improved (up to you to move forward or not, in this MR or another one): * the logic in changing the directory is currently overkill. we just need two cases: change current directory (+set env var) (the `cd /d` case or `cd` case in the same current directory) , or just change env var (if dir exists) (so no need in that case to change twice the current directory) * the logic in case of `cd foo*` is wrong (current code looks up for first directory in foo\*, while native picks first element in foo\*, and fails if it's not a directory) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6891#note_89019
participants (3)
-
eric pouech (＠epo) -
huangqinjin
-
huangqinjin (＠huangqinjin)