Skip to content

Commit 0d97ef7

Browse files
jviottijviotti
authored
Revise atomic_directory_replace as atomic_directory_swap (#2279)
Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
1 parent d2bc5b7 commit 0d97ef7

4 files changed

Lines changed: 56 additions & 37 deletions

File tree

‎src/lang/io/include/sourcemeta/core/io.h‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -118,20 +118,21 @@ auto hardlink_directory(const std::filesystem::path &source,
118118

119119
/// @ingroup io
120120
///
121-
/// Replace one directory with another, guaranteeing an atomic swap when
122-
/// possible. Both directories must reside on the same filesystem and the
123-
/// original path must not be a bare filename (it must have a parent
124-
/// component). If the original does not exist, the replacement is simply
125-
/// renamed into place.
121+
/// Atomically swap two directories. Both directories must reside on the same
122+
/// filesystem and the original path must not be a bare filename (it must have
123+
/// a parent component). After the call, the original path holds the contents
124+
/// of the replacement and the replacement path holds the former contents of
125+
/// the original. If the original does not exist, the replacement is simply
126+
/// renamed into place and the replacement path will no longer exist.
126127
///
127128
/// ```cpp
128129
/// #include <sourcemeta/core/io.h>
129130
///
130-
/// sourcemeta::core::atomic_directory_replace("/output", "/staging");
131+
/// sourcemeta::core::atomic_directory_swap("/output", "/staging");
131132
/// ```
132133
SOURCEMETA_CORE_IO_EXPORT
133-
auto atomic_directory_replace(const std::filesystem::path &original,
134-
const std::filesystem::path &replacement) -> void;
134+
auto atomic_directory_swap(const std::filesystem::path &original,
135+
const std::filesystem::path &replacement) -> void;
135136

136137
/// @ingroup io
137138
///

‎src/lang/io/io.cc‎

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -72,9 +72,8 @@ auto hardlink_directory(const std::filesystem::path &source,
7272
}
7373
}
7474

75-
auto atomic_directory_replace(const std::filesystem::path &original,
76-
const std::filesystem::path &replacement)
77-
-> void {
75+
auto atomic_directory_swap(const std::filesystem::path &original,
76+
const std::filesystem::path &replacement) -> void {
7877
assert(std::filesystem::is_directory(replacement));
7978
assert(!std::filesystem::exists(original) ||
8079
std::filesystem::is_directory(original));
@@ -94,7 +93,6 @@ auto atomic_directory_replace(const std::filesystem::path &original,
9493
std::error_code{errno, std::generic_category()}};
9594
}
9695

97-
std::filesystem::remove_all(replacement);
9896
// Atomic swap via renameatx_np with RENAME_SWAP
9997
#elif defined(__APPLE__)
10098
if (renameatx_np(AT_FDCWD, replacement.c_str(), AT_FDCWD, original.c_str(),
@@ -104,21 +102,22 @@ auto atomic_directory_replace(const std::filesystem::path &original,
104102
std::error_code{errno, std::generic_category()}};
105103
}
106104

107-
std::filesystem::remove_all(replacement);
108105
#else
109-
// Non-atomic fallback: two-rename approach with rollback.
110-
106+
// Non-atomic fallback: two-rename approach with rollback
107+
//
111108
// Note we cannot safely use the temporary directory of the system as it
112109
// might be in another volume
113-
TemporaryDirectory backup{original.parent_path(), ".backup-"};
114-
std::filesystem::remove(backup.path());
115-
std::filesystem::rename(original, backup.path());
110+
TemporaryDirectory temporary{original.parent_path(), ".swap-"};
111+
std::filesystem::remove(temporary.path());
112+
std::filesystem::rename(original, temporary.path());
116113
try {
117114
std::filesystem::rename(replacement, original);
118115
} catch (...) {
119-
std::filesystem::rename(backup.path(), original);
116+
std::filesystem::rename(temporary.path(), original);
120117
throw;
121118
}
119+
120+
std::filesystem::rename(temporary.path(), replacement);
122121
#endif
123122
}
124123

‎test/io/CMakeLists.txt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ sourcemeta_googletest(NAMESPACE sourcemeta PROJECT core NAME io
88
io_fileview_test.cc
99
io_temporary_test.cc
1010
io_hardlink_directory_test.cc
11-
io_atomic_directory_replace_test.cc)
11+
io_atomic_directory_swap_test.cc)
1212

1313
target_link_libraries(sourcemeta_core_io_unit
1414
PRIVATE sourcemeta::core::io)

test/io/io_atomic_directory_replace_test.cc renamed to test/io/io_atomic_directory_swap_test.cc

Lines changed: 36 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
#include <fstream> // std::ofstream, std::ifstream
77
#include <string> // std::string
88

9-
TEST(IO_atomic_directory_replace, creates_when_original_absent) {
9+
TEST(IO_atomic_directory_swap, creates_when_original_absent) {
1010
const sourcemeta::core::TemporaryDirectory workspace{
1111
std::filesystem::path{BUILD_DIRECTORY}, ".test-atomic-"};
1212
const auto original{workspace.path() / "original"};
@@ -17,7 +17,7 @@ TEST(IO_atomic_directory_replace, creates_when_original_absent) {
1717

1818
EXPECT_FALSE(std::filesystem::exists(original));
1919

20-
sourcemeta::core::atomic_directory_replace(original, replacement);
20+
sourcemeta::core::atomic_directory_swap(original, replacement);
2121

2222
EXPECT_TRUE(std::filesystem::exists(original));
2323
EXPECT_TRUE(std::filesystem::is_directory(original));
@@ -29,7 +29,7 @@ TEST(IO_atomic_directory_replace, creates_when_original_absent) {
2929
EXPECT_EQ(content, "hello");
3030
}
3131

32-
TEST(IO_atomic_directory_replace, replaces_existing_directory) {
32+
TEST(IO_atomic_directory_swap, swaps_existing_directories) {
3333
const sourcemeta::core::TemporaryDirectory workspace{
3434
std::filesystem::path{BUILD_DIRECTORY}, ".test-atomic-"};
3535
const auto original{workspace.path() / "original"};
@@ -41,47 +41,63 @@ TEST(IO_atomic_directory_replace, replaces_existing_directory) {
4141
std::filesystem::create_directory(replacement);
4242
std::ofstream{replacement / "new.txt"} << "new";
4343

44-
sourcemeta::core::atomic_directory_replace(original, replacement);
44+
sourcemeta::core::atomic_directory_swap(original, replacement);
4545

4646
EXPECT_TRUE(std::filesystem::exists(original));
4747
EXPECT_FALSE(std::filesystem::exists(original / "old.txt"));
4848
EXPECT_TRUE(std::filesystem::exists(original / "new.txt"));
4949

50-
std::ifstream stream{original / "new.txt"};
51-
std::string content;
52-
std::getline(stream, content);
53-
EXPECT_EQ(content, "new");
50+
std::ifstream new_stream{original / "new.txt"};
51+
std::string new_content;
52+
std::getline(new_stream, new_content);
53+
EXPECT_EQ(new_content, "new");
54+
55+
EXPECT_TRUE(std::filesystem::exists(replacement));
56+
EXPECT_FALSE(std::filesystem::exists(replacement / "new.txt"));
57+
EXPECT_TRUE(std::filesystem::exists(replacement / "old.txt"));
58+
59+
std::ifstream old_stream{replacement / "old.txt"};
60+
std::string old_content;
61+
std::getline(old_stream, old_content);
62+
EXPECT_EQ(old_content, "old");
5463
}
5564

56-
TEST(IO_atomic_directory_replace, replacement_is_consumed) {
65+
TEST(IO_atomic_directory_swap, old_directory_preserved_at_replacement_path) {
5766
const sourcemeta::core::TemporaryDirectory workspace{
5867
std::filesystem::path{BUILD_DIRECTORY}, ".test-atomic-"};
5968
const auto original{workspace.path() / "original"};
6069
const auto replacement{workspace.path() / "replacement"};
6170

6271
std::filesystem::create_directory(original);
72+
std::ofstream{original / "a.txt"} << "aaa";
73+
std::ofstream{original / "b.txt"} << "bbb";
74+
6375
std::filesystem::create_directory(replacement);
6476
std::ofstream{replacement / "file.txt"} << "data";
6577

66-
sourcemeta::core::atomic_directory_replace(original, replacement);
78+
sourcemeta::core::atomic_directory_swap(original, replacement);
6779

68-
EXPECT_FALSE(std::filesystem::exists(replacement));
80+
EXPECT_TRUE(std::filesystem::is_directory(replacement));
81+
EXPECT_TRUE(std::filesystem::exists(replacement / "a.txt"));
82+
EXPECT_TRUE(std::filesystem::exists(replacement / "b.txt"));
83+
EXPECT_FALSE(std::filesystem::exists(replacement / "file.txt"));
6984
}
7085

71-
TEST(IO_atomic_directory_replace, preserves_nested_structure) {
86+
TEST(IO_atomic_directory_swap, preserves_nested_structure) {
7287
const sourcemeta::core::TemporaryDirectory workspace{
7388
std::filesystem::path{BUILD_DIRECTORY}, ".test-atomic-"};
7489
const auto original{workspace.path() / "original"};
7590
const auto replacement{workspace.path() / "replacement"};
7691

7792
std::filesystem::create_directory(original);
93+
std::ofstream{original / "orig.txt"} << "orig";
7894

7995
std::filesystem::create_directories(replacement / "a" / "b");
8096
std::ofstream{replacement / "root.txt"} << "root";
8197
std::ofstream{replacement / "a" / "mid.txt"} << "mid";
8298
std::ofstream{replacement / "a" / "b" / "deep.txt"} << "deep";
8399

84-
sourcemeta::core::atomic_directory_replace(original, replacement);
100+
sourcemeta::core::atomic_directory_swap(original, replacement);
85101

86102
EXPECT_TRUE(std::filesystem::exists(original / "root.txt"));
87103
EXPECT_TRUE(std::filesystem::exists(original / "a" / "mid.txt"));
@@ -91,9 +107,12 @@ TEST(IO_atomic_directory_replace, preserves_nested_structure) {
91107
std::string content;
92108
std::getline(stream, content);
93109
EXPECT_EQ(content, "deep");
110+
111+
EXPECT_TRUE(std::filesystem::exists(replacement / "orig.txt"));
112+
EXPECT_FALSE(std::filesystem::exists(replacement / "root.txt"));
94113
}
95114

96-
TEST(IO_atomic_directory_replace, no_leftover_backup) {
115+
TEST(IO_atomic_directory_swap, no_leftover_temporary) {
97116
const sourcemeta::core::TemporaryDirectory workspace{
98117
std::filesystem::path{BUILD_DIRECTORY}, ".test-atomic-"};
99118
const auto original{workspace.path() / "original"};
@@ -105,12 +124,12 @@ TEST(IO_atomic_directory_replace, no_leftover_backup) {
105124
std::filesystem::create_directory(replacement);
106125
std::ofstream{replacement / "new.txt"} << "new";
107126

108-
sourcemeta::core::atomic_directory_replace(original, replacement);
127+
sourcemeta::core::atomic_directory_swap(original, replacement);
109128

110129
for (const auto &entry :
111130
std::filesystem::directory_iterator{workspace.path()}) {
112131
const auto filename{entry.path().filename().string()};
113-
EXPECT_FALSE(filename.starts_with(".backup-"))
114-
<< "leftover backup directory found: " << filename;
132+
EXPECT_FALSE(filename.starts_with(".swap-"))
133+
<< "leftover temporary directory found: " << filename;
115134
}
116135
}

0 commit comments

Comments
 (0)