From: Johannes Sixt Date: Thu, 18 Dec 2025 17:44:37 GMT Subject: Re: [PATCH 11/18] mingw: support renaming symlinks Message-ID: <02fa15af-5a57-4557-b016-fd14b9107c6e@kdbg.org> In-Reply-To: <5cb3b10500339fec403b84e30721e4f6a68e42f6.1765980535.git.gitgitgadget@gmail.com> Am 17.12.25 um 15:08 schrieb Karsten Blees via GitGitGadget: > From: Karsten Blees > > Older MSVCRT's `_wrename()` function cannot rename symlinks over > existing files: it returns success without doing anything. Newer > MSVCR*.dll versions probably do not share this problem: according to CRT > sources, they just call `MoveFileEx()` with the `MOVEFILE_COPY_ALLOWED` > flag. > > Avoid the `_wrename()` call, and go with directly calling > `MoveFileEx()`, with proper error handling of course. > > Signed-off-by: Karsten Blees > Signed-off-by: Johannes Schindelin > --- > compat/mingw.c | 38 ++++++++++++++++---------------------- > 1 file changed, 16 insertions(+), 22 deletions(-) > > diff --git a/compat/mingw.c b/compat/mingw.c > index b1cc30d0f1..55f0bb478e 100644 > --- a/compat/mingw.c > +++ b/compat/mingw.c > @@ -2275,7 +2275,7 @@ int mingw_accept(int sockfd1, struct sockaddr *sa, socklen_t *sz) > int mingw_rename(const char *pold, const char *pnew) > { > static int supports_file_rename_info_ex = 1; > - DWORD attrs, gle; > + DWORD attrs = INVALID_FILE_ATTRIBUTES, gle; > int tries = 0; > wchar_t wpold[MAX_PATH], wpnew[MAX_PATH]; > int wpnew_len; > @@ -2286,15 +2286,6 @@ int mingw_rename(const char *pold, const char *pnew) > if (wpnew_len < 0) > return -1; > > - /* > - * Try native rename() first to get errno right. > - * It is based on MoveFile(), which cannot overwrite existing files. > - */ > - if (!_wrename(wpold, wpnew)) > - return 0; > - if (errno != EEXIST) > - return -1; > - > repeat: > if (supports_file_rename_info_ex) { > /* > @@ -2370,13 +2361,22 @@ repeat: > * to retry. > */ > } else { > - if (MoveFileExW(wpold, wpnew, MOVEFILE_REPLACE_EXISTING)) > + if (MoveFileExW(wpold, wpnew, > + MOVEFILE_REPLACE_EXISTING | MOVEFILE_COPY_ALLOWED)) > return 0; > gle = GetLastError(); > } > > - /* TODO: translate more errors */ > - if (gle == ERROR_ACCESS_DENIED && > + /* revert file attributes on failure */ > + if (attrs != INVALID_FILE_ATTRIBUTES) > + SetFileAttributesW(wpnew, attrs); > + > + if (!is_file_in_use_error(gle)) { > + errno = err_win_to_posix(gle); > + return -1; > + } > + > + if (attrs == INVALID_FILE_ATTRIBUTES && > (attrs = GetFileAttributesW(wpnew)) != INVALID_FILE_ATTRIBUTES) { > if (attrs & FILE_ATTRIBUTE_DIRECTORY) { > DWORD attrsold = GetFileAttributesW(wpold); > @@ -2388,16 +2388,10 @@ repeat: > return -1; > } > if ((attrs & FILE_ATTRIBUTE_READONLY) && > - SetFileAttributesW(wpnew, attrs & ~FILE_ATTRIBUTE_READONLY)) { > - if (MoveFileExW(wpold, wpnew, MOVEFILE_REPLACE_EXISTING)) > - return 0; > - gle = GetLastError(); > - /* revert file attributes on failure */ > - SetFileAttributesW(wpnew, attrs); > - } > + SetFileAttributesW(wpnew, attrs & ~FILE_ATTRIBUTE_READONLY)) > + goto repeat; > } > - if (gle == ERROR_ACCESS_DENIED && > - retry_ask_yes_no(&tries, "Rename from '%s' to '%s' failed. " > + if (retry_ask_yes_no(&tries, "Rename from '%s' to '%s' failed. " > "Should I try again?", pold, pnew)) > goto repeat; > The logic in this function is incredibly convoluted. It does look somewhat reasonable, at least on the non-error path, but whether the variable attr is changed and reset as needed after 'goto repeat' and the various failure modes, I cannot tell. I give up and trust that this code has been battle-tested during the past decade and works as desired. -- Hannes