From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Fri, 09 Jan 2026 20:04:40 GMT
Subject: Re: [PATCH 11/18] mingw: support renaming symlinks
Message-ID: <99570ae3-d34d-c917-e218-ebbf3f9d7b6c@gmx.de>
In-Reply-To: <02fa15af-5a57-4557-b016-fd14b9107c6e@kdbg.org>

Hi Hannes,

On Thu, 18 Dec 2025, Johannes Sixt wrote:

> Am 17.12.25 um 15:08 schrieb Karsten Blees via GitGitGadget:
> > From: Karsten Blees <blees@dcon.de>
> > 
> > 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 <blees@dcon.de>
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  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.

I do agree that the logic is quite convoluted. Historically grown, like.
But as you suspect: This has been battle-hardened, and I am loathe to
introduce a regression by making it prettier at this point. This is tried
and tested code, and that counts for something.

Ciao,
Johannes