<filesystem>: Fix filesystem::equivalent() (and filesystem::copy() with overwrite_existing) on FAT32 etc.#6187
Open
StephanTLavavej wants to merge 3 commits intomicrosoft:mainfrom
Open
Conversation
…calls". This reverts only the alternatives to GetFileInformationByHandleEx() with FileIdInfo. All other changes, including the unconditional use of GetFileInformationByHandleEx() with FileStandardInfo to get NumberOfLinks in legacy filesys.cpp _Hard_links(), are being allowed to stand as correct. Note that that case matches what modern filesystem.cpp __std_fs_get_stats() has always done.
Previously, the legacy implementation had NO fallback behavior. For `_CRT_APP`, 128-bit file IDs were the only codepath. That might have been okay, depending on what filesystems were available, but it might have been a mistake. For non-App, 64-bit file indexes were the only codepath, so we weren't handling modern filesystems in an ideal way. I'm bringing the logic closer to the modern implementation. It isn't an exact match (we don't look at the error codes, so if neither file exists, we try the fallback only to discover again that neither file exists). However, it should be good enough for the legacy implementation, and a strict improvement over the status quo.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6180, a regression that was introduced by #5434 in the MSVC Build Tools 14.50.
Commits
(1) Partial revert
Partially revert #5434. This reverts only the alternatives to
GetFileInformationByHandleEx()withFileIdInfo. All other changes, including the unconditional use ofGetFileInformationByHandleEx()withFileStandardInfoto getNumberOfLinksin legacyfilesys.cpp_Hard_links(), are being allowed to stand as correct. Note that that case matches what modernfilesystem.cpp__std_fs_get_stats()has always done:STL/stl/src/filesys.cpp
Lines 294 to 296 in bed5dd5
STL/stl/src/filesystem.cpp
Lines 960 to 969 in bed5dd5
(2) Fix modern
filesystem.cppfilesystem.cpp: Always build the fallback, improve comment.We don't need
_CRT_APPguards anymore. @amyw-msft explained:(3) Fix legacy
filesys.cppfilesys.cpp: Add fallback behavior, improve comments.Previously, the legacy implementation had no fallback behavior.
For
_CRT_APP, 128-bit file IDs were the only codepath. That might have been okay, depending on what filesystems were available, but it might have been a mistake.For non-App, 64-bit file indexes were the only codepath, so we weren't handling modern filesystems in an ideal way.
I'm bringing the logic closer to the modern implementation. It isn't an exact match (we don't look at the error codes, so if neither file exists, we try the fallback only to discover again that neither file exists). However, it should be good enough for the legacy implementation, and a strict improvement over the status quo. This is already a fairly major change, and I don't want it to become even more invasive.
Manual testing
For obvious reasons, adding automated coverage for this would be far more difficult than it's worth. I've manually verified both the legacy and modern implementations. With the new comments, the risk of regression in the future should be low.
Click to expand before-and-after testing on NTFS and FAT32:
NTFS is unaffected:
FAT32 is fixed: