{"thread":{"id":"62655","subject":"[PATCH] mingw_rename: do support directory renames","startedAt":"2024-12-17T12:52:07Z","lastAt":"2024-12-18T06:00:56Z","messageCount":3,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"509219","messageId":"pull.1839.git.1734439924842.gitgitgadget@gmail.com","threadId":"62655","inReplyTo":null,"subject":"[PATCH] mingw_rename: do support directory renames","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-12-17T12:52:04Z","receivedAt":"2024-12-17T12:52:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 391bceae435 (compat/mingw: support POSIX semantics for atomic\nrenames, 2024-10-27), we taught the `mingw_rename()` function to respect\nPOSIX semantics, but we did so only as a fallback after `_wrename()`\nfails.\n\nThis hid a bug in the implementation that was not caught by Git's test\nsuite: The `CreateFileW()` function _can_ open handles to directories,\nbut not when asked to use the `FILE_ATTRIBUTE_NORMAL` flag, as that flag\nonly is allowed for files.\n\nLet's fix this by using the common `FILE_FLAG_BACKUP_SEMANTICS` flag\nthat can be used for opening handles to directories, too.\n\nSigned-off-by: Johannes Schindelin \n---\n mingw_rename: do support directory renames\n \n This is not quite a critical bug fix for Git because (unlike Git for\n Windows) it attempts _wrename() first. If that succeeds, we'll not\n bother with the POSIX semantics.\n \n However, Git for Windows knows how to deal with symbolic links, and\n _wrename() does not work for them. Therefore, that _wrename() call was\n patched out there and we rely on the native Win32 API call to\n SetFileInformationByHandle() to rename files and directories.\n \n It is that latter part that is at heart of this here bug fix: To be able\n to call SetFileInformationByHandle(), we need a handle, and\n CreateFileW() is what we use, for files, and crucially, also for\n directories.\n \n So while it is not critical for Git to take this patch, it still is\n important because that _wrename() call can fail, even when renaming\n directories, and then we want the fallback to fail not because we tried\n to obtain a handle using incorrect flags, but only because the actual\n rename operation failed.\n \n This patch is based on ps/mingw-rename.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1839%2Fdscho%2Fps%2Fmingw-rename-followup-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1839/dscho/ps/mingw-rename-followup-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1839\n\n compat/mingw.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex c4320769db6..e8f491d03a7 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -2273,7 +2273,7 @@ repeat:\n \n \t\told_handle = CreateFileW(wpold, DELETE,\n \t\t\t\t\t FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,\n-\t\t\t\t\t NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);\n+\t\t\t\t\t NULL, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, NULL);\n \t\tif (old_handle == INVALID_HANDLE_VALUE) {\n \t\t\terrno = err_win_to_posix(GetLastError());\n \t\t\treturn -1;\n\nbase-commit: 391bceae4350136a05d977573caeaa07059f2136\n-- \ngitgitgadget\n"},{"id":"509237","messageId":"xmqqy10eysuz.fsf@gitster.g","threadId":"62655","inReplyTo":"pull.1839.git.1734439924842.gitgitgadget@gmail.com","subject":"Re: [PATCH] mingw_rename: do support directory renames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-17T20:09:08Z","receivedAt":"2024-12-17T20:09:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> This is not quite a critical bug fix for Git because (unlike Git for\n> Windows) it attempts _wrename() first. If that succeeds, we'll not\n> bother with the POSIX semantics.\n> \n> However, Git for Windows knows how to deal with symbolic links, and\n> _wrename() does not work for them. Therefore, that _wrename() call was\n> patched out there and we rely on the native Win32 API call to\n> SetFileInformationByHandle() to rename files and directories.\n> \n> It is that latter part that is at heart of this here bug fix: To be able\n> to call SetFileInformationByHandle(), we need a handle, and\n> CreateFileW() is what we use, for files, and crucially, also for\n> directories.\n> \n> So while it is not critical for Git to take this patch, it still is\n> important because that _wrename() call can fail, even when renaming\n> directories, and then we want the fallback to fail not because we tried\n> to obtain a handle using incorrect flags, but only because the actual\n> rename operation failed.\n> \n> This patch is based on ps/mingw-rename.\n\nThanks.\n\nAs this will be part of GfW 2.48 anyway (I presume), let me take it\nand include it in my tree to keep the divergence between our trees\nsmall.\n"},{"id":"509278","messageId":"Z2JlBQ4DNf7tsNxV@pks.im","threadId":"62655","inReplyTo":"pull.1839.git.1734439924842.gitgitgadget@gmail.com","subject":"Re: [PATCH] mingw_rename: do support directory renames","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-18T06:00:37Z","receivedAt":"2024-12-18T06:00:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Dec 17, 2024 at 12:52:04PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index c4320769db6..e8f491d03a7 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -2273,7 +2273,7 @@ repeat:\n> \n> \t\told_handle = CreateFileW(wpold, DELETE,\n> \t\t\t\t\t FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,\n> -\t\t\t\t\t NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);\n> +\t\t\t\t\t NULL, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, NULL);\n> \t\tif (old_handle == INVALID_HANDLE_VALUE) {\n> \t\t\terrno = err_win_to_posix(GetLastError());\n> \t\t\treturn -1;\n\nOkay. I saw that FILE_FLAG_BACKUP_SEMANTICS was used in several other\nusers of `CreateFileW()` in this context, but I couldn't find good\nenough information around what it is actually doing anywhere. The\nexplanation makes sense to me, so thanks for the follow-up fix!\n\nPatrick\n"}]}