Repository: carbon-language/carbon-lang
File: common/filesystem_test.cpp (C++)

Code before:
  ASSERT_THAT(dir_.ReadFileToString(std::filesystem::path("subdir2") / "file1"),
              IsSuccess(Eq("content1")));
  EXPECT_THAT(dir_.Rename("subdir2", dir_, "subdir1"), IsSuccess(_));
  EXPECT_THAT(dir_.ReadFileToString(std::filesystem::path("subdir1") / "file1"),
              IsSuccess(Eq("content1")));

  // The open directory should survive the rename and point at the same
  // directory.
  EXPECT_THAT(d2.ReadFileToString("file1"), IsSuccess(Eq("content1")));
  EXPECT_THAT(d2.WriteFileFromString("file2", "content2"), IsSuccess(_));
  EXPECT_THAT(dir_.ReadFileToString(std::filesystem::path("subdir1") / "file2"),
              IsSuccess(Eq("content2")));

  // Rename over an existing file.
  EXPECT_THAT(d2.Rename("file2", d2, "file1"), IsSuccess(_));
  EXPECT_THAT(d2.ReadFileToString("file1"), IsSuccess(Eq("content2")));

  // Test error calls as well.
  auto result = dir_.Rename("missing1", dir_, "missing2");
  EXPECT_TRUE(result.error().no_entity()) << result.error();
  result = d2.Rename("file1", dir_,
                     std::filesystem::path("missing_subdir") / "file2");
  EXPECT_TRUE(result.error().no_entity()) << result.error();
  result = dir_.Rename("subdir1", d2, "infinite_subdirs");
  EXPECT_THAT(result.error().unix_errnum(), EINVAL) << result.error();
}

}  // namespace
}  // namespace Carbon::Filesystem


Review comment (refactor):
This check is a little hard to follow, because it relies on `subdir2` having been renamed to `subdir1` in the "Rename directories" portion of the test, while still being referred to as `d2`. (It took me a moment to see why this would result in infinite subdirectories.)

Can you split this up so this check is a bit more isolated from the prior logic? Or if it's intentional that we're testing that `d2` tracks the same directory across the directory itself being renamed while open, a comment here would help.

Code after:
  ASSERT_THAT(dir_.ReadFileToString(std::filesystem::path("subdir2") / "file1"),
              IsSuccess(Eq("content1")));
  EXPECT_THAT(dir_.Rename("subdir2", dir_, "subdir1"), IsSuccess(_));
  EXPECT_THAT(dir_.ReadFileToString(std::filesystem::path("subdir1") / "file1"),
              IsSuccess(Eq("content1")));

  // The open directory `d2` should survive the rename and point at the same
  // directory.
  EXPECT_THAT(d2.ReadFileToString("file1"), IsSuccess(Eq("content1")));
  EXPECT_THAT(d2.WriteFileFromString("file2", "content2"), IsSuccess(_));
  EXPECT_THAT(dir_.ReadFileToString(std::filesystem::path("subdir1") / "file2"),
              IsSuccess(Eq("content2")));

  // Rename over an existing file.
  EXPECT_THAT(d2.Rename("file2", d2, "file1"), IsSuccess(_));
  EXPECT_THAT(d2.ReadFileToString("file1"), IsSuccess(Eq("content2")));

  // Test error calls as well.
  auto result = dir_.Rename("missing1", dir_, "missing2");
  EXPECT_TRUE(result.error().no_entity()) << result.error();
  result = d2.Rename("file1", dir_,
                     std::filesystem::path("missing_subdir") / "file2");
  EXPECT_TRUE(result.error().no_entity()) << result.error();
  // Note that `d2` was renamed `subdir1` above, which is why this creates
  // infinite subdirectories.
  result = dir_.Rename("subdir1", d2, "infinite_subdirs");
  EXPECT_THAT(result.error().unix_errnum(), EINVAL) << result.error();
}

}  // namespace
}  // namespace Carbon::Filesystem