On Wed, 31 Jul 2024 at 15:42, Björn Schäpers <g...@hazardy.de> wrote: > > Am 30.07.2024 um 11:13 schrieb Jonathan Wakely: > > On Sun, 24 Mar 2024 at 21:34, Björn Schäpers <g...@hazardy.de> wrote: > >> > >> From: Björn Schäpers <bjo...@hazardy.de> > >> > >> This fixes i.e. https://github.com/msys2/MSYS2-packages/issues/1937 > >> I don't know if I picked the right way to do it. > >> > >> When acceptable I think the declaration should be moved into > >> ops-common.h, since then we could use stat_type and also use that in the > >> commonly used function. > >> > >> Manually tested on i686-w64-mingw32. > >> > >> -- >8 -- > >> libstdc++: Fix overwriting files on windows > >> > >> The inodes have no meaning on windows, thus all files have an inode of > >> 0. Use a differenz approach to identify equivalent files. As a result > >> std::filesystem::copy_file did not honor > >> copy_options::overwrite_existing. Factored the method out of > >> std::filesystem::equivalent. > >> > >> libstdc++-v3/Changelog: > >> > >> * include/bits/fs_ops.h: Add declaration of > >> __detail::equivalent_win32. > >> * src/c++17/fs_ops.cc (__detail::equivalent_win32): Implement it > >> (fs::equivalent): Use __detail::equivalent_win32, factored the > >> old test out. > >> * src/filesystem/ops-common.h (_GLIBCXX_FILESYSTEM_IS_WINDOWS): > >> Use the function. > >> > >> Signed-off-by: Björn Schäpers <bjo...@hazardy.de> > >> --- > >> libstdc++-v3/include/bits/fs_ops.h | 8 +++ > >> libstdc++-v3/src/c++17/fs_ops.cc | 79 +++++++++++++----------- > >> libstdc++-v3/src/filesystem/ops-common.h | 10 ++- > >> 3 files changed, 60 insertions(+), 37 deletions(-) > >> > >> diff --git a/libstdc++-v3/include/bits/fs_ops.h > >> b/libstdc++-v3/include/bits/fs_ops.h > >> index 90650c47b46..d10b78a4bdd 100644 > >> --- a/libstdc++-v3/include/bits/fs_ops.h > >> +++ b/libstdc++-v3/include/bits/fs_ops.h > >> @@ -40,6 +40,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > >> > >> namespace filesystem > >> { > >> +#ifdef _GLIBCXX_FILESYSTEM_IS_WINDOWS > >> +namespace __detail > >> +{ > >> + bool > >> + equivalent_win32(const wchar_t* p1, const wchar_t* p2, error_code& ec); > >> +} // namespace __detail > >> +#endif //_GLIBCXX_FILESYSTEM_IS_WINDOWS > >> + > >> /** @addtogroup filesystem > >> * @{ > >> */ > >> diff --git a/libstdc++-v3/src/c++17/fs_ops.cc > >> b/libstdc++-v3/src/c++17/fs_ops.cc > >> index 61df19753ef..3cc87d45237 100644 > >> --- a/libstdc++-v3/src/c++17/fs_ops.cc > >> +++ b/libstdc++-v3/src/c++17/fs_ops.cc > >> @@ -67,6 +67,49 @@ > >> namespace fs = std::filesystem; > >> namespace posix = std::filesystem::__gnu_posix; > >> > >> +#ifdef _GLIBCXX_FILESYSTEM_IS_WINDOWS > >> +bool > >> +fs::__detail::equivalent_win32(const wchar_t* p1, const wchar_t* p2, > >> + error_code& ec) > >> +{ > >> + struct auto_handle { > >> + explicit auto_handle(const path& p_) > >> + : handle(CreateFileW(p_.c_str(), 0, > >> + FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE, > >> + 0, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, 0)) > >> + { } > >> + > >> + ~auto_handle() > >> + { if (*this) CloseHandle(handle); } > >> + > >> + explicit operator bool() const > >> + { return handle != INVALID_HANDLE_VALUE; } > >> + > >> + bool get_info() > >> + { return GetFileInformationByHandle(handle, &info); } > >> + > >> + HANDLE handle; > >> + BY_HANDLE_FILE_INFORMATION info; > >> + }; > >> + auto_handle h1(p1); > >> + auto_handle h2(p2); > >> + if (!h1 || !h2) > >> + { > >> + if (!h1 && !h2) > >> + ec = __last_system_error(); > >> + return false; > >> + } > >> + if (!h1.get_info() || !h2.get_info()) > >> + { > >> + ec = __last_system_error(); > >> + return false; > >> + } > >> + return h1.info.dwVolumeSerialNumber == h2.info.dwVolumeSerialNumber > >> + && h1.info.nFileIndexHigh == h2.info.nFileIndexHigh > >> + && h1.info.nFileIndexLow == h2.info.nFileIndexLow; > >> +} > >> +#endif //_GLIBCXX_FILESYSTEM_IS_WINDOWS > >> + > >> fs::path > >> fs::absolute(const path& p) > >> { > >> @@ -858,41 +901,7 @@ fs::equivalent(const path& p1, const path& p2, > >> error_code& ec) noexcept > >> if (st1.st_mode != st2.st_mode || st1.st_dev != st2.st_dev) > >> return false; > >> > >> - struct auto_handle { > >> - explicit auto_handle(const path& p_) > >> - : handle(CreateFileW(p_.c_str(), 0, > >> - FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE, > >> - 0, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, 0)) > >> - { } > >> - > >> - ~auto_handle() > >> - { if (*this) CloseHandle(handle); } > >> - > >> - explicit operator bool() const > >> - { return handle != INVALID_HANDLE_VALUE; } > >> - > >> - bool get_info() > >> - { return GetFileInformationByHandle(handle, &info); } > >> - > >> - HANDLE handle; > >> - BY_HANDLE_FILE_INFORMATION info; > >> - }; > >> - auto_handle h1(p1); > >> - auto_handle h2(p2); > >> - if (!h1 || !h2) > >> - { > >> - if (!h1 && !h2) > >> - ec = __last_system_error(); > >> - return false; > >> - } > >> - if (!h1.get_info() || !h2.get_info()) > >> - { > >> - ec = __last_system_error(); > >> - return false; > >> - } > >> - return h1.info.dwVolumeSerialNumber == h2.info.dwVolumeSerialNumber > >> - && h1.info.nFileIndexHigh == h2.info.nFileIndexHigh > >> - && h1.info.nFileIndexLow == h2.info.nFileIndexLow; > >> + return __detail::equivalent_win32(p1.c_str(), p2.c_str(), ec); > >> #else > >> return st1.st_dev == st2.st_dev && st1.st_ino == st2.st_ino; > >> #endif > >> diff --git a/libstdc++-v3/src/filesystem/ops-common.h > >> b/libstdc++-v3/src/filesystem/ops-common.h > >> index d917fddbeb1..7e67286bd01 100644 > >> --- a/libstdc++-v3/src/filesystem/ops-common.h > >> +++ b/libstdc++-v3/src/filesystem/ops-common.h > >> @@ -489,8 +489,14 @@ _GLIBCXX_BEGIN_NAMESPACE_FILESYSTEM > >> return false; > >> } > >> > >> - if (to_st->st_dev == from_st->st_dev > >> - && to_st->st_ino == from_st->st_ino) > >> +#ifdef _GLIBCXX_FILESYSTEM_IS_WINDOWS > >> + // st_ino is not set, so can't be used to distinguish files > >> + std::error_code not_used; > >> + if (st1.st_mode != st2.st_mode || st1.st_dev != st2.st_dev || > >> + fs::__detail::equivalent_win32(from, to, not_used)) > >> +#else > >> + if (st1.st_dev != st2.st_dev || st1.st_ino != st2.st_ino) > >> +#endif > > > > What's this code doing? You seem to have inverted the logic, so that > > it now returns a file_exists error when the files are *not* the same. > > > > Is this just a copy & paste error from the similar code in > > fs::equivalent? Was this tested? > > > >> { > >> ec = std::make_error_code(std::errc::file_exists); > >> return false; > >> -- > >> 2.44.0 > >> > > > > It was tested on windows and no other platform. You already sent some > feedback, > but I've not yet found the time to work on that, since I already deployed a > work-around.
I think this logic would have caused a spurious error when copying a file over an existing file on a different device: > >> + if (st1.st_mode != st2.st_mode || st1.st_dev != st2.st_dev || > >> + fs::__detail::equivalent_win32(from, to, not_used)) Which probably wouldn't show up for most testing, and certainly isn't exercised in our testsuite. > But I see you fixed it yourself, thanks for that. Yup, it should be fixed now. If you notice any remaining issues, please let me know. Thanks for identifying where the problem was and how to fix it!
