[PATCH] compat/mingw: rename the symlink, not the target

Johannes Schindelin via GitGitGadget· Feb 21, 2025, 12:01 UTC · lore

From: Eliah Kagan <eliah.kagan@gmail.com>

Since 183ea3e [1], a new technique is used on Windows to rename files, where supported. The first step of this technique is to open the file with `CreateFileW`. At that time, `FILE_ATTRIBUTE_NORMAL` was passed as the value of the `dwFlagsAndAttributes` argument. In b30404df [2], this was improved by passing `FILE_FLAG_BACKUP_SEMANTICS`, to support directories as well as regular files.

However, neither value of `dwFlagsAndAttributes` is sufficient to open a symbolic link with the correct semantics to rename it. Symlinks on Windows are reparse points. Attempting to open a reparse point with `CreateFileW` dereferences the reparse point and opens the target instead, unless `FILE_FLAG_OPEN_REPARSE_POINT` is included in `dwFlagsAndAttributes`. This is documented for that flag and in the "Symbolic Link Behavior" section of the `CreateFileW` docs [3].

This produces a regression where attempting to rename a symlink on Windows renames its target to the intended new name and location of the symlink. For example, if `symlink` points to `file`, then running

    git mv symlink symlink-renamed

leaves `symlink` in place and unchanged, but renames `file` to `symlink-renamed` [4].

This regression is detectable by existing tests in `t7001-mv.sh`, but the tests must be run by a Windows user with the ability to create symlinks, and the `ln -s` command used to create the initial symlink must also be able to create a real symlink (such as by setting the `MSYS` environment variable to `winsymlinks:nativestrict`). Then these two tests fail if the regression is present, and pass otherwise:

    38 - git mv should overwrite file with a symlink
    39 - check moved symlink

Let's fix this, so that renaming a symlink again renames the symlink itself and leaves the target unchanged, by passing

    FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OPEN_REPARSE_POINT

as the `dwFlagsAndAttributes` argument. This is sufficient (and safe) because including `FILE_FLAG_OPEN_REPARSE_POINT` causes no harm even when used to open a file or directory that is not a reparse point. In that case, as noted in [3], this flag is simply ignored.

[1]: https://github.com/git-for-windows/git/commit/183ea3eabf81822506d2cd3aa1dc0727099ebccd [2]: https://github.com/git-for-windows/git/commit/b30404dfc04a4b087b630aea4ab88a51cd3a7459 [3]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew [4]: https://github.com/git-for-windows/git/issues/5436

Signed-off-by: Eliah Kagan <eliah.kagan@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    compat/mingw: rename the symlink, not the target
    
    This contribution just came in as a Git for Windows Pull Request.
    
    Granted, I have not yet managed to find time to upstream support for
    symbolic links (it is in the pipeline:
    https://github.com/dscho/git/tree/support-symlinks-on-windows), but this
    patch still should be in upstream Git because there are other ways to
    create symbolic links than by using Git.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1864%2Fdscho%2Ffix-renaming-symlinks-on-windows-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1864/dscho/fix-renaming-symlinks-on-windows-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1864

 compat/mingw.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Show changes to compat/mingw.c +3 −1

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

base-commit: b838bf1938926a7a900166136d995d86f8a00e24

-- 
gitgitgadget

Junio C Hamano· Feb 21, 2025, 18:26 UTC · re: Johannes Schindelin via GitGitGadget · lore

Re: [PATCH] compat/mingw: rename the symlink, not the target

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 7 quoted lines

>     This contribution just came in as a Git for Windows Pull Request.
>     
>     Granted, I have not yet managed to find time to upstream support for
>     symbolic links (it is in the pipeline:
>     https://github.com/dscho/git/tree/support-symlinks-on-windows), but this
>     patch still should be in upstream Git because there are other ways to
>     create symbolic links than by using Git.

Thanks, let me mark it for 'next' immediately.

Patrick Steinhardt· Feb 24, 2025, 11:12 UTC · re: Junio C Hamano · lore

Re: [PATCH] compat/mingw: rename the symlink, not the target

On Fri, Feb 21, 2025 at 10:26:15AM -0800, Junio C Hamano wrote:

Show 12 quoted lines

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> >     This contribution just came in as a Git for Windows Pull Request.
> >     
> >     Granted, I have not yet managed to find time to upstream support for
> >     symbolic links (it is in the pipeline:
> >     https://github.com/dscho/git/tree/support-symlinks-on-windows), but this
> >     patch still should be in upstream Git because there are other ways to
> >     create symbolic links than by using Git.
> 
> Thanks, let me mark it for 'next' immediately.

The Windows API continues to be quite esoteric to me, so thanks for plugging the gaps in my knowledge and fixing this edge case.

Patrick

patchcompat/mingw: rename the symlink, not the target