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.
From: huangqinjin huangqinjin@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 +@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 +@todo_wine@FAILURE 3 --- success/failure for DIR command FAILURE 1 FAILURE 1
From: huangqinjin huangqinjin@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; }
/****************************************************************************
From: huangqinjin huangqinjin@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 -@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 -@todo_wine@FAILURE 3 +FAILURE 3 --- success/failure for DIR command FAILURE 1 FAILURE 1
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)
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!)`
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
(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)
-
eric pouech (＠epo) -
huangqinjin
-
huangqinjin (＠huangqinjin)