-
Notifications
You must be signed in to change notification settings - Fork 2.6k
compat/mingw: rename the symlink, not the target #5437
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
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 b30404d [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]: git-for-windows@183ea3e [2]: git-for-windows@b30404d [3]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew [4]: git-for-windows#5436 Signed-off-by: Eliah Kagan <eliah.kagan@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, what an excellent Pull Request! Thank you so much!
I verified manually that this fixes it by running: cd t && MSYS=winsymlinks:nativestrict sh t7001-mv.sh -iVx With the version from current
With this patch, it succeeds:
|
This fixes yet another fall-out from the `mingw_rename()` change that was introduced in v2.48.0.
This fixes yet another fall-out from the `mingw_rename()` change that was introduced in v2.48.0.
This fixes yet another fall-out from the `mingw_rename()` change that was introduced in v2.48.0.
/add relnote bug A change in upstream Git v2.48.0 broke renaming symlinks, which was fixed. The workflow run was started |
A change in upstream Git v2.48.0 broke renaming symlinks, which [was fixed](git-for-windows/git#5437). Signed-off-by: gitforwindowshelper[bot] <gitforwindowshelper-bot@users.noreply.github.com>
This fixes yet another fall-out from the `mingw_rename()` change that was introduced in v2.48.0.
This fixes yet another fall-out from the `mingw_rename()` change that was introduced in v2.48.0.
This fixes yet another fall-out from the `mingw_rename()` change that was introduced in v2.48.0.
This fixes yet another fall-out from the `mingw_rename()` change that was introduced in v2.48.0.
This fixes yet another fall-out from the `mingw_rename()` change that was introduced in v2.48.0.
This fixes yet another fall-out from the `mingw_rename()` change that was introduced in v2.48.0.
This fixes yet another fall-out from the `mingw_rename()` change that was introduced in v2.48.0.
This fixes yet another fall-out from the `mingw_rename()` change that was introduced in v2.48.0.
This fixes yet another fall-out from the `mingw_rename()` change that was introduced in v2.48.0.
This fixes #5436. I've tried to write the commit message (6a994b2) in such a way that it would be suitable both here and upstream, but I do not know if I succeeded at that. As noted in the commit message, and in the issue, existing tests are able to detect the bug this is fixing, so at least for now I have not written any tests.
The code this modifies is also present upstream, but my understanding of the contribution guidelines is that I should open this PR here rather than submitting a patch to the Git mailing list, because the effect of this change is entirely Windows-specific. If this instead needs to be submitted to the mailing list, then I could try and do that. The change here is also rather simple, and I would certainly have no objection to it being integrated in some other way that doesn't involve merging this in any form, if that is more convenient.