util: use stronger-guarantee rename method #24308

1. 
   fanquake commented at 8:18 am on February 10, 2022: member

   Use std::filesystem::rename() instead of std::rename(). We rely on the destination to be overwritten if it exists, but std::rename()’s behavior is implementation-defined in this case.

   This is a rebase of #20435 by vasild.

2. util: use stronger-guarantee rename method

   Use std::filesystem::rename() instead of std::rename(). We rely on the
   destination to be overwritten if it exists, but std::rename()'s behavior
   is implementation-defined in this case.

   ee822d85d6
3. fanquake added the label Utils/log/libs on Feb 10, 2022
4. hebasto approved
5. 
   hebasto commented at 9:54 am on February 10, 2022: member

   Approach ACK ee822d85d6de7db85416190cf843ad74147519cf.

   We rely on the destination to be overwritten if it exists

   Add a test for this assumption?

6. vasild approved
7. 
   vasild commented at 1:24 pm on February 10, 2022: member

   ACK ee822d85d6de7db85416190cf843ad74147519cf

   Add a test for this assumption?

    0commit 23074edf054845b1b74c43618cf7a8cc58aa5518 (HEAD -> pull/24308_1644480965_ee822d85d6__20435_rebased)
    1Parent: ee822d85d6de7db85416190cf843ad74147519cf
    2Author:     Vasil Dimov <vd@FreeBSD.org>
    3AuthorDate: Thu Feb 10 14:11:35 2022 +0100
    4Commit:     Vasil Dimov <vd@FreeBSD.org>
    5CommitDate: Thu Feb 10 14:14:58 2022 +0100
    6
    7    test: add a unit test for RenameOver()
    8
    9diff --git a/src/test/fs_tests.cpp b/src/test/fs_tests.cpp
   10index 1256395849..8a5f025e5f 100644
   11--- a/src/test/fs_tests.cpp
   12+++ b/src/test/fs_tests.cpp
   13@@ -115,7 +115,40 @@ BOOST_AUTO_TEST_CASE(fsbridge_fstream)
   14         BOOST_CHECK(p1 != p2);
   15         BOOST_CHECK(p2 != p3);
   16         BOOST_CHECK(p1 != p3);
   17     }
   18 }
   19 
   20-BOOST_AUTO_TEST_SUITE_END()
   21\ No newline at end of file
   22+BOOST_AUTO_TEST_CASE(rename)
   23+{
   24+    const fs::path tmpfolder{m_args.GetDataDirBase()};
   25+
   26+    const fs::path path1{GetUniquePath(tmpfolder)};
   27+    const fs::path path2{GetUniquePath(tmpfolder)};
   28+
   29+    const std::string path1_contents{"1111"};
   30+    const std::string path2_contents{"2222"};
   31+
   32+    {
   33+        std::ofstream file{path1};
   34+        file << path1_contents;
   35+    }
   36+
   37+    {
   38+        std::ofstream file{path2};
   39+        file << path2_contents;
   40+    }
   41+
   42+    // Rename path1 -> path2.
   43+    BOOST_CHECK(RenameOver(path1, path2));
   44+
   45+    BOOST_CHECK(!fs::exists(path1));
   46+
   47+    {
   48+        std::ifstream file{path2};
   49+        std::string contents;
   50+        file >> contents;
   51+        BOOST_CHECK_EQUAL(contents, path1_contents);
   52+    }
   53+}
   54+
   55+BOOST_AUTO_TEST_SUITE_END()
   

8. 
   laanwj commented at 1:25 pm on February 10, 2022: member

   Concept ACK. Checked the relevant documentation that the behavior is what we want for RenameOver:

   Moves or renames the filesystem object identified by old_p to new_p as if by the POSIX rename

   - https://en.cppreference.com/w/cpp/filesystem/rename

   If the link named by the new argument exists, it shall be removed and old renamed to new.

   - https://pubs.opengroup.org/onlinepubs/9699919799/functions/rename.html

   Also, it is a nice cleanup, no more win32-specific code here.

9. 
   MarcoFalke commented at 3:39 pm on February 11, 2022: member

   review ACK ee822d85d6de7db85416190cf843ad74147519cf
10. MarcoFalke merged this on Feb 11, 2022
11. MarcoFalke closed this on Feb 11, 2022

    ---

12. 
    MarcoFalke commented at 4:04 pm on February 11, 2022: member

    (Also reviewed the unit test in #24308#pullrequestreview-878832906 but didn’t run it)
13. sidhujag referenced this in commit ed278f1df3 on Feb 11, 2022
14. 
    hebasto commented at 3:33 pm on February 12, 2022: member

    It seems to break Windows builds:

    

15. 
    hebasto commented at 3:06 pm on February 13, 2022: member

    Add a test for this assumption?

    Unit test for RenameOver()

    Btw, it fails for mingw build: https://cirrus-ci.com/task/6670169794674688

16. fanquake deleted the branch on Feb 13, 2022
17. laanwj referenced this in commit df0825046a on Feb 17, 2022
18. sidhujag referenced this in commit aa1d6e2c06 on Feb 18, 2022
19. DrahtBot locked this on Feb 13, 2023

Contributors
fanquake hebasto vasild laanwj MarcoFalke

Labels
Utils/log/libs