diff mbox series

mingw_rename: do support directory renames

Message ID pull.1839.git.1734439924842.gitgitgadget@gmail.com (mailing list archive)
State Accepted
Commit b30404dfc04a4b087b630aea4ab88a51cd3a7459
Headers show
Series mingw_rename: do support directory renames | expand

Commit Message

Johannes Schindelin Dec. 17, 2024, 12:52 p.m. UTC 
From: Johannes Schindelin <johannes.schindelin@gmx.de>

In 391bceae435 (compat/mingw: support POSIX semantics for atomic
renames, 2024-10-27), we taught the `mingw_rename()` function to respect
POSIX semantics, but we did so only as a fallback after `_wrename()`
fails.

This hid a bug in the implementation that was not caught by Git's test
suite: The `CreateFileW()` function _can_ open handles to directories,
but not when asked to use the `FILE_ATTRIBUTE_NORMAL` flag, as that flag
only is allowed for files.

Let's fix this by using the common `FILE_FLAG_BACKUP_SEMANTICS` flag
that can be used for opening handles to directories, too.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    mingw_rename: do support directory renames
    
    This is not quite a critical bug fix for Git because (unlike Git for
    Windows) it attempts _wrename() first. If that succeeds, we'll not
    bother with the POSIX semantics.
    
    However, Git for Windows knows how to deal with symbolic links, and
    _wrename() does not work for them. Therefore, that _wrename() call was
    patched out there and we rely on the native Win32 API call to
    SetFileInformationByHandle() to rename files and directories.
    
    It is that latter part that is at heart of this here bug fix: To be able
    to call SetFileInformationByHandle(), we need a handle, and
    CreateFileW() is what we use, for files, and crucially, also for
    directories.
    
    So while it is not critical for Git to take this patch, it still is
    important because that _wrename() call can fail, even when renaming
    directories, and then we want the fallback to fail not because we tried
    to obtain a handle using incorrect flags, but only because the actual
    rename operation failed.
    
    This patch is based on ps/mingw-rename.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1839%2Fdscho%2Fps%2Fmingw-rename-followup-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1839/dscho/ps/mingw-rename-followup-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1839

 compat/mingw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


base-commit: 391bceae4350136a05d977573caeaa07059f2136

Comments

Junio C Hamano Dec. 17, 2024, 8:09 p.m. UTC | #1 
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

>     This is not quite a critical bug fix for Git because (unlike Git for
>     Windows) it attempts _wrename() first. If that succeeds, we'll not
>     bother with the POSIX semantics.
>     
>     However, Git for Windows knows how to deal with symbolic links, and
>     _wrename() does not work for them. Therefore, that _wrename() call was
>     patched out there and we rely on the native Win32 API call to
>     SetFileInformationByHandle() to rename files and directories.
>     
>     It is that latter part that is at heart of this here bug fix: To be able
>     to call SetFileInformationByHandle(), we need a handle, and
>     CreateFileW() is what we use, for files, and crucially, also for
>     directories.
>     
>     So while it is not critical for Git to take this patch, it still is
>     important because that _wrename() call can fail, even when renaming
>     directories, and then we want the fallback to fail not because we tried
>     to obtain a handle using incorrect flags, but only because the actual
>     rename operation failed.
>     
>     This patch is based on ps/mingw-rename.

Thanks.

As this will be part of GfW 2.48 anyway (I presume), let me take it
and include it in my tree to keep the divergence between our trees
small.
Patrick Steinhardt Dec. 18, 2024, 6 a.m. UTC | #2 
On Tue, Dec 17, 2024 at 12:52:04PM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/compat/mingw.c b/compat/mingw.c
> index c4320769db6..e8f491d03a7 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -2273,7 +2273,7 @@ repeat:
>  
>  		old_handle = CreateFileW(wpold, DELETE,
>  					 FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,
> -					 NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
> +					 NULL, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, NULL);
>  		if (old_handle == INVALID_HANDLE_VALUE) {
>  			errno = err_win_to_posix(GetLastError());
>  			return -1;

Okay. I saw that FILE_FLAG_BACKUP_SEMANTICS was used in several other
users of `CreateFileW()` in this context, but I couldn't find good
enough information around what it is actually doing anywhere. The
explanation makes sense to me, so thanks for the follow-up fix!

Patrick
diff mbox series

Patch

diff --git a/compat/mingw.c b/compat/mingw.c
index c4320769db6..e8f491d03a7 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2273,7 +2273,7 @@  repeat:
 
 		old_handle = CreateFileW(wpold, DELETE,
 					 FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,
-					 NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
+					 NULL, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, NULL);
 		if (old_handle == INVALID_HANDLE_VALUE) {
 			errno = err_win_to_posix(GetLastError());
 			return -1;