microsoft / STL

MSVC's implementation of the C++ Standard Library.
Other
10.23k stars 1.51k forks source link

`<filesystem>`: `rename` is not POSIX compliant #2053

Open SibiSiddharthan opened 3 years ago

SibiSiddharthan commented 3 years ago

According to this https://en.cppreference.com/w/cpp/filesystem/rename filesystem::rename does not handle 2 cases.

Case 1: Renaming files which are hard-links of each other should be a no-op

Command-line test case

C:\Temp> echo "hello" > f1
C:\Temp> mklink /H f1 f2

C:\Temp>type c1.cpp
#include <filesystem>

int main() {
   std::filesystem::rename("f1","f2");
}

C:\Temp>cl /EHsc /W4 /std:c++17 .\c1.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.23.28019.1 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

c1.cpp
Microsoft (R) Incremental Linker Version 14.23.28019.1
Copyright (C) Microsoft Corporation.  All rights reserved.

/out:c1.exe
c1.obj

C:\Temp>.\c1.exe

Expected behavior

C:\Temp> dir /B
f1
f2

Observed behavior

C:\Temp> dir /B
f2

Case 2: In case of renaming directories where the new path points to an empty directory, the operation should not fail

Command-line test case

C:\Temp> mkdir mydir
C:\Temp> echo "hello" > mydir\f1
C:\Temp> mkdir emptydir

C:\Temp>type c2.cpp
#include <filesystem>

int main() {
   std::filesystem::rename("mydir","emptydir");
}

C:\Temp>cl /EHsc /W4 /std:c++17 .\c2.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.23.28019.1 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

c2.cpp
Microsoft (R) Incremental Linker Version 14.23.28019.1
Copyright (C) Microsoft Corporation.  All rights reserved.

/out:c2.exe
c2.obj

C:\Temp>.\c2.exe

Expected behavior

C:\Temp> dir /B /S
C:\Temp\emptydir
C:\Temp\emptydir\f1

Observed behavior

C:\Temp> dir /B /S
C:\Temp\emptydir
C:\Temp\mydir
C:\Temp\mydir\f1

STL version

https://github.com/microsoft/STL/commit/e745bad3b1d05b5b19ec652d68abb37865ffa454
SibiSiddharthan commented 3 years ago

Hey there, I have been working on this for a while now. While doing so I found a new bug with rename. This has to do with the read-only bit again. I have fixed the above 2 issues. Looks like fixing the new one below will require similar handling to that of #1559. Shall I raise a PR for the fix for the above 2 issues alone or should I fix this as well?

Details

C:\Temp> echo "hello" > f1
C:\Temp> echo "world" > f2

C:\Temp>type c1.cpp
#include <filesystem>
using namespace std::filesystem;
int main() {
   permissions("f2", perms::owner_write | perms::group_write | perms::others_write, perm_options::remove);
   rename("f1","f2");
}

C:\Temp>cl /EHsc /W4 /std:c++17 .\c1.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.23.28019.1 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

c1.cpp
Microsoft (R) Incremental Linker Version 14.23.28019.1
Copyright (C) Microsoft Corporation.  All rights reserved.

/out:c1.exe
c1.obj

C:\Temp>.\c1.exe

Expected behavior

C:\Temp> dir /B
f2

Observed behavior

C:\Temp> dir /B
f1
f2
StephanTLavavej commented 3 years ago

Shall I raise a PR for the fix for the above 2 issues alone or should I fix this as well?

As this affects the same function rename, I think it would be simplest to have a single PR fixing all issues, so that we can review all of the changes at once (instead of having to reason about an intermediate state between two PRs). If different functions were affected, I think that different PRs would be simpler.

Thanks for asking - we really appreciate it :smile_cat:

SibiSiddharthan commented 3 years ago

I have raised #2062. Please take a look.