diff mbox series

Fix overwriting files with fs::copy_file on windows

Message ID 20240324213401.47870-1-gcc@hazardy.de
State New
Headers show
Series Fix overwriting files with fs::copy_file on windows | expand

Commit Message

Björn Schäpers March 24, 2024, 9:34 p.m. UTC
From: Björn Schäpers <bjoern@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 <bjoern@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(-)

Comments

Björn Schäpers April 25, 2024, 8:16 p.m. UTC | #1
Am 24.03.2024 um 22:34 schrieb Björn Schäpers:
> From: Björn Schäpers <bjoern@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 <bjoern@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
>   	  {
>   	    ec = std::make_error_code(std::errc::file_exists);
>   	    return false;

A friendly ping.
One of your IPs tried to hack me May 16, 2024, 6:49 p.m. UTC | #2
Am 25.04.2024 um 22:16 schrieb Björn Schäpers:
> Am 24.03.2024 um 22:34 schrieb Björn Schäpers:
>> From: Björn Schäpers <bjoern@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 <bjoern@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
>>         {
>>           ec = std::make_error_code(std::errc::file_exists);
>>           return false;
> 
> A friendly ping.

Ping.
Jonathan Wakely May 17, 2024, 2:34 p.m. UTC | #3
On Sun, 24 Mar 2024 at 21:34, Björn Schäpers <gcc@hazardy.de> wrote:
>
> From: Björn Schäpers <bjoern@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 <bjoern@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);

I don't think we want this declared in the public header, it should be
internal to the library.

Can it just be declared in ops-common.h instead?


> +} // 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);

This creates a new filesystem::path, just to call c_str() on it
immediately. The auto_handle ctor should be changed to just take const
wchar_t* so we don't need to allocate and parse new path objects.

> +  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
>           {
>             ec = std::make_error_code(std::errc::file_exists);
>             return false;
> --
> 2.44.0
>
Jonathan Wakely July 30, 2024, 9:13 a.m. UTC | #4
On Sun, 24 Mar 2024 at 21:34, Björn Schäpers <gcc@hazardy.de> wrote:
>
> From: Björn Schäpers <bjoern@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 <bjoern@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
>
Björn Schäpers July 31, 2024, 2:42 p.m. UTC | #5
Am 30.07.2024 um 11:13 schrieb Jonathan Wakely:
> On Sun, 24 Mar 2024 at 21:34, Björn Schäpers <gcc@hazardy.de> wrote:
>>
>> From: Björn Schäpers <bjoern@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 <bjoern@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.

But I see you fixed it yourself, thanks for that.
Jonathan Wakely July 31, 2024, 3:31 p.m. UTC | #6
On Wed, 31 Jul 2024 at 15:42, Björn Schäpers <gcc@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 <gcc@hazardy.de> wrote:
> >>
> >> From: Björn Schäpers <bjoern@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 <bjoern@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!
diff mbox series

Patch

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
 	  {
 	    ec = std::make_error_code(std::errc::file_exists);
 	    return false;