Skip to content

Commit a7d9de2

Browse files
phstphst
committed
Use FileName class in CopyTree function
1 parent aa09848 commit a7d9de2

File tree

3 files changed

+71
-87
lines changed

3 files changed

+71
-87
lines changed

‎elisp/private/tools/system.cc‎

Lines changed: 55 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -576,12 +576,11 @@ absl::StatusOr<std::vector<FileName>> ListDirectory(const FileName& dir) {
576576
#endif
577577

578578
#ifdef _WIN32
579-
static absl::Status CopyTreeWindows(const std::wstring& from,
580-
const std::wstring& to) {
581-
if (!::CreateDirectoryExW(Pointer(from), Pointer(to), nullptr)) {
579+
static absl::Status CopyTreeWindows(const FileName& from, const FileName& to) {
580+
if (!::CreateDirectoryExW(from.pointer(), to.pointer(), nullptr)) {
582581
return WindowsStatus("CreateDirectoryExW", from, to, nullptr);
583582
}
584-
const std::wstring pattern = from + L"\\*";
583+
const std::wstring pattern = from.string() + L"\\*";
585584
WIN32_FIND_DATAW data;
586585
const HANDLE handle = ::FindFirstFileW(Pointer(pattern), &data);
587586
if (handle == INVALID_HANDLE_VALUE) {
@@ -593,18 +592,20 @@ static absl::Status CopyTreeWindows(const std::wstring& from,
593592
if (!::FindClose(handle)) LOG(ERROR) << WindowsStatus("FindClose");
594593
};
595594
do {
596-
const std::wstring name = data.cFileName;
595+
const std::wstring_view name = data.cFileName;
597596
if (name == L"." || name == L"..") continue;
598-
const std::wstring from_entry = from + L'\\' + name;
599-
const std::wstring to_entry = to + L'\\' + name;
597+
const absl::StatusOr<FileName> from_entry = from.Child(name);
598+
if (!from_entry.ok()) return from_entry.status();
599+
const absl::StatusOr<FileName> to_entry = to.Child(name);
600+
if (!to_entry.ok()) return to_entry.status();
600601
if (data.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) {
601-
const absl::Status status = CopyTreeWindows(from_entry, to_entry);
602+
const absl::Status status = CopyTreeWindows(*from_entry, *to_entry);
602603
if (!status.ok()) return status;
603604
} else {
604605
constexpr DWORD flags = COPY_FILE_FAIL_IF_EXISTS | COPY_FILE_COPY_SYMLINK;
605-
if (!::CopyFileExW(Pointer(from_entry), Pointer(to_entry), nullptr,
606+
if (!::CopyFileExW(from_entry->pointer(), to_entry->pointer(), nullptr,
606607
nullptr, nullptr, flags)) {
607-
return WindowsStatus("CopyFileExW", from_entry, to_entry, nullptr,
608+
return WindowsStatus("CopyFileExW", *from_entry, *to_entry, nullptr,
608609
nullptr, nullptr, absl::Hex(flags));
609610
}
610611
}
@@ -618,16 +619,16 @@ static absl::Status CopyTreeWindows(const std::wstring& from,
618619

619620
#ifdef RULES_ELISP_COPY_TREE_POSIX
620621
static absl::Status CopyFilePosix(const int from_parent, const int to_parent,
621-
const std::string& name,
622+
const FileName& name,
622623
const struct stat& from_stat) {
623-
CHECK(!name.empty());
624-
CHECK_NE(name, ".");
625-
CHECK_NE(name, "..");
626-
CHECK_EQ(name.find('/'), name.npos) << "invalid name " << name;
624+
CHECK_NE(name.string(), ".");
625+
CHECK_NE(name.string(), "..");
626+
CHECK_EQ(name.string().find('/'), name.string().npos)
627+
<< "invalid name " << name;
627628
CHECK(S_ISREG(from_stat.st_mode)) << "file " << name << " is not regular";
628629

629630
constexpr int from_flags = O_RDONLY | O_CLOEXEC | O_NOCTTY | O_NOFOLLOW;
630-
const int from_fd = openat(from_parent, Pointer(name), from_flags);
631+
const int from_fd = openat(from_parent, name.pointer(), from_flags);
631632
if (from_fd < 0) {
632633
return ErrnoStatus("openat", from_parent, name, absl::Hex(from_flags));
633634
}
@@ -638,7 +639,7 @@ static absl::Status CopyFilePosix(const int from_parent, const int to_parent,
638639
const mode_t mode = from_stat.st_mode;
639640
constexpr int to_flags =
640641
O_WRONLY | O_CREAT | O_EXCL | O_CLOEXEC | O_NOCTTY | O_NOFOLLOW;
641-
const int to_fd = openat(to_parent, Pointer(name), to_flags, mode);
642+
const int to_fd = openat(to_parent, name.pointer(), to_flags, mode);
642643
if (to_fd < 0) {
643644
return ErrnoStatus("openat", to_parent, name, to_flags, Oct(mode));
644645
}
@@ -664,7 +665,7 @@ static absl::Status CopyFilePosix(const int from_parent, const int to_parent,
664665
if (written == 0) {
665666
// Prevent infinite loop.
666667
return absl::DataLossError(absl::StrFormat(
667-
"Cannot write %d bytes to %s", remaining.size(), name));
668+
"Cannot write %d bytes to %v", remaining.size(), name));
668669
}
669670
remaining.remove_prefix(
670671
CastNumber<std::string_view::size_type>(written).value());
@@ -681,25 +682,25 @@ static absl::Status CopyFilePosix(const int from_parent, const int to_parent,
681682
}
682683

683684
static absl::Status CopyTreePosix(const int from_parent,
684-
const std::string& from_name,
685+
const FileName& from_name,
685686
std::optional<struct stat> from_stat,
686687
const int to_parent,
687-
const std::string& to_name) {
688-
CHECK(!from_name.empty());
689-
CHECK_NE(from_name, ".");
690-
CHECK_NE(from_name, "..");
691-
CHECK(from_parent == AT_FDCWD || from_name.find('/') == from_name.npos)
688+
const FileName& to_name) {
689+
CHECK_NE(from_name.string(), ".");
690+
CHECK_NE(from_name.string(), "..");
691+
CHECK(from_parent == AT_FDCWD ||
692+
from_name.string().find('/') == from_name.string().npos)
692693
<< "invalid source name " << from_name;
693694

694-
CHECK(!to_name.empty());
695-
CHECK_NE(to_name, ".");
696-
CHECK_NE(to_name, "..");
697-
CHECK(to_parent == AT_FDCWD || to_name.find('/') == to_name.npos)
695+
CHECK_NE(to_name.string(), ".");
696+
CHECK_NE(to_name.string(), "..");
697+
CHECK(to_parent == AT_FDCWD ||
698+
to_name.string().find('/') == to_name.string().npos)
698699
<< "invalid target name " << to_name;
699700

700701
const int from_flags =
701702
O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC | O_NOCTTY;
702-
const int from_fd = openat(from_parent, Pointer(from_name), from_flags);
703+
const int from_fd = openat(from_parent, from_name.pointer(), from_flags);
703704
if (from_fd < 0) {
704705
return ErrnoStatus("openat", from_parent, from_name, absl::Hex(from_flags));
705706
}
@@ -714,7 +715,7 @@ static absl::Status CopyTreePosix(const int from_parent,
714715
if (fstat(from_fd, &buffer) != 0) return ErrnoStatus("fstat", from_fd);
715716
if (!S_ISDIR(buffer.st_mode)) {
716717
return absl::FailedPreconditionError(
717-
absl::StrFormat("Source %s is not a directory", from_name));
718+
absl::StrFormat("Source %v is not a directory", from_name));
718719
}
719720
from_stat = buffer;
720721
}
@@ -729,12 +730,12 @@ static absl::Status CopyTreePosix(const int from_parent,
729730
};
730731

731732
const mode_t mode = from_stat->st_mode | S_IWUSR;
732-
const int result = mkdirat(to_parent, Pointer(to_name), mode);
733+
const int result = mkdirat(to_parent, to_name.pointer(), mode);
733734
if (result != 0) return ErrnoStatus("mkdirat", to_parent, to_name, Oct(mode));
734735

735736
constexpr int to_flags =
736737
O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC | O_NOCTTY;
737-
const int to_fd = openat(to_parent, Pointer(to_name), to_flags);
738+
const int to_fd = openat(to_parent, to_name.pointer(), to_flags);
738739
if (to_fd < 0) {
739740
return ErrnoStatus("openat", to_parent, to_name, absl::Hex(to_flags));
740741
}
@@ -749,8 +750,10 @@ static absl::Status CopyTreePosix(const int from_parent,
749750
if (errno != 0) return ErrnoStatus("readdir");
750751
break;
751752
}
752-
const std::string name = entry->d_name;
753+
const std::string_view name = entry->d_name;
753754
if (name == "." || name == "..") continue;
755+
const absl::StatusOr<FileName> file = FileName::FromString(name);
756+
if (!file.ok()) return file.status();
754757

755758
struct stat buffer;
756759
constexpr int stat_flags = AT_SYMLINK_NOFOLLOW;
@@ -761,10 +764,10 @@ static absl::Status CopyTreePosix(const int from_parent,
761764
const mode_t mode = buffer.st_mode;
762765
if (S_ISDIR(mode)) {
763766
const absl::Status status =
764-
CopyTreePosix(from_fd, name, buffer, to_fd, name);
767+
CopyTreePosix(from_fd, *file, buffer, to_fd, *file);
765768
if (!status.ok()) return status;
766769
} else if (S_ISREG(mode)) {
767-
const absl::Status status = CopyFilePosix(from_fd, to_fd, name, buffer);
770+
const absl::Status status = CopyFilePosix(from_fd, to_fd, *file, buffer);
768771
if (!status.ok()) return status;
769772
} else {
770773
return absl::FailedPreconditionError(absl::StrFormat(
@@ -780,78 +783,56 @@ static absl::Status CopyTreePosix(const int from_parent,
780783
}
781784
#endif
782785

783-
absl::Status CopyTree(NativeStringView from, NativeStringView to) {
784-
if (from.empty()) {
785-
return absl::InvalidArgumentError("Empty source directory name");
786-
}
787-
if (ContainsNull(from)) {
788-
return absl::InvalidArgumentError(absl::StrFormat(
789-
"Source directory name %s contains null character", from));
790-
}
791-
if (to.empty()) {
792-
return absl::InvalidArgumentError("Empty destination directory name");
793-
}
794-
if (ContainsNull(to)) {
795-
return absl::InvalidArgumentError(absl::StrFormat(
796-
"Destination directory name %s contains null character", from));
797-
}
798-
786+
absl::Status CopyTree(const FileName& from, const FileName& to) {
799787
#if defined _WIN32
800-
const absl::StatusOr<FileName> from_name = FileName::FromString(from);
801-
if (!from_name.ok()) return from_name.status();
802-
const absl::StatusOr<FileName> from_abs = from_name->MakeAbsolute();
788+
const absl::StatusOr<FileName> from_abs = from.MakeAbsolute();
803789
if (!from_abs.ok()) return from_abs.status();
804790
if (from_abs->string().length() < 4) {
805791
return absl::InvalidArgumentError(
806792
absl::StrFormat("Cannot copy drive %v", *from_abs));
807793
}
808-
const absl::StatusOr<FileName> to_name = FileName::FromString(to);
809-
if (!to_name.ok()) return to_name.status();
810-
const absl::StatusOr<FileName> to_abs = to_name->MakeAbsolute();
794+
const absl::StatusOr<FileName> to_abs = to.MakeAbsolute();
811795
if (!to_abs.ok()) return to_abs.status();
812796
if (to_abs->string().length() < 4) {
813797
return absl::AlreadyExistsError(
814798
absl::StrFormat("Cannot copy over drive %v", *from_abs));
815799
}
816-
return CopyTreeWindows(from_abs->string(), to_abs->string());
800+
return CopyTreeWindows(*from_abs, *to_abs);
817801
#elif defined __APPLE__
802+
std::string from_str = from.string();
818803
// The behavior of copyfile changes if the source directory name ends in a
819804
// slash, see the man page copyfile(3). We need to remove trailing slashes to
820805
// get the expected behavior.
821-
const std::string_view::size_type i = from.find_last_not_of('/');
822-
from.remove_suffix(from.size() - (i == from.npos ? 0 : i + 1));
823-
if (from.empty()) {
806+
while (from_str.back() == '/') from_str.pop_back();
807+
if (from_str.empty()) {
824808
// If the source directory name is empty, it has consisted only of slashes.
825809
return absl::InvalidArgumentError("Cannot copy from root directory");
826810
}
827-
const std::string from_str(from);
828-
const std::string to_str(to);
829811
// The behavior of copyfile changes if the destination directory already
830812
// exists, see the man page copyfile(3).
831813
struct stat buffer;
832-
if (lstat(Pointer(to_str), &buffer) == 0) {
814+
if (lstat(to.pointer(), &buffer) == 0) {
833815
return absl::AlreadyExistsError(
834-
absl::StrFormat("Destination directory %s already exists", to_str));
816+
absl::StrFormat("Destination directory %v already exists", to));
835817
}
836-
if (errno != ENOENT) return ErrnoStatus("lstat", to_str);
818+
if (errno != ENOENT) return ErrnoStatus("lstat", to);
837819
constexpr copyfile_flags_t flags = COPYFILE_ALL | COPYFILE_RECURSIVE |
838820
COPYFILE_EXCL | COPYFILE_NOFOLLOW |
839821
COPYFILE_CLONE | COPYFILE_DATA_SPARSE;
840-
if (copyfile(Pointer(from_str), Pointer(to_str), nullptr, flags) != 0) {
841-
return ErrnoStatus("copyfile", from_str, to_str, nullptr, absl::Hex(flags));
822+
if (copyfile(Pointer(from_str), to.pointer(), nullptr, flags) != 0) {
823+
return ErrnoStatus("copyfile", from_str, to, nullptr, absl::Hex(flags));
842824
}
843825
return absl::OkStatus();
844826
#elif defined RULES_ELISP_COPY_TREE_POSIX
845-
if (from.find_first_not_of('/') == from.npos) {
827+
if (from.string().find_first_not_of('/') == from.string().npos) {
846828
return absl::InvalidArgumentError(
847-
absl::StrFormat("Cannot copy from root directory %s", from));
829+
absl::StrFormat("Cannot copy from root directory %v", from));
848830
}
849-
if (to.find_first_not_of('/') == to.npos) {
831+
if (to.string().find_first_not_of('/') == to.string().npos) {
850832
return absl::AlreadyExistsError(
851-
absl::StrFormat("Cannot overwrite root directory %s", to));
833+
absl::StrFormat("Cannot overwrite root directory %v", to));
852834
}
853-
return CopyTreePosix(AT_FDCWD, std::string(from), std::nullopt, AT_FDCWD,
854-
std::string(to));
835+
return CopyTreePosix(AT_FDCWD, from, std::nullopt, AT_FDCWD, to);
855836
#else
856837
# error CopyTree not implemented on this system
857838
#endif

‎elisp/private/tools/system.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -237,7 +237,7 @@ absl::Status Unlink(const FileName& file);
237237
absl::Status CreateDirectory(const FileName& name);
238238
absl::Status RemoveDirectory(const FileName& name);
239239
absl::StatusOr<std::vector<FileName>> ListDirectory(const FileName& dir);
240-
absl::Status CopyTree(NativeStringView from, NativeStringView to);
240+
absl::Status CopyTree(const FileName& from, const FileName& to);
241241

242242
class Environment final {
243243
private:

‎tests/tools/system_test.cc‎

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -571,7 +571,7 @@ TEST(CopyTreeTest, RejectsSelfCopy) {
571571
const absl::StatusOr<FileName> temp = TempDir();
572572
ASSERT_THAT(temp, IsOk());
573573

574-
EXPECT_THAT(CopyTree(temp->string(), temp->string()),
574+
EXPECT_THAT(CopyTree(*temp, *temp),
575575
StatusIs(AnyOf(absl::StatusCode::kAlreadyExists,
576576
absl::StatusCode::kNotFound)));
577577
}
@@ -580,12 +580,13 @@ TEST(CopyTreeTest, RejectsCopyFromRootDirectory) {
580580
const absl::StatusOr<FileName> temp = TempDir();
581581
ASSERT_THAT(temp, IsOk());
582582

583+
const FileName from =
584+
FileName::FromString(kWindows ? RULES_ELISP_NATIVE_LITERAL("C:\\")
585+
: RULES_ELISP_NATIVE_LITERAL("/"))
586+
.value();
583587
const FileName to = temp->Child(RULES_ELISP_NATIVE_LITERAL("to")).value();
584588

585-
EXPECT_THAT(CopyTree(kWindows ? RULES_ELISP_NATIVE_LITERAL("C:\\")
586-
: RULES_ELISP_NATIVE_LITERAL("/"),
587-
to.string()),
588-
StatusIs(absl::StatusCode::kInvalidArgument));
589+
EXPECT_THAT(CopyTree(from, to), StatusIs(absl::StatusCode::kInvalidArgument));
589590

590591
EXPECT_THAT(RemoveDirectory(to), StatusIs(absl::StatusCode::kNotFound));
591592
}
@@ -594,10 +595,12 @@ TEST(CopyTreeTest, RejectsCopyToRootDirectory) {
594595
const absl::StatusOr<FileName> temp = TempDir();
595596
ASSERT_THAT(temp, IsOk());
596597

597-
EXPECT_THAT(
598-
CopyTree(temp->string(), kWindows ? RULES_ELISP_NATIVE_LITERAL("C:\\")
599-
: RULES_ELISP_NATIVE_LITERAL("/")),
600-
StatusIs(absl::StatusCode::kAlreadyExists));
598+
const FileName to =
599+
FileName::FromString(kWindows ? RULES_ELISP_NATIVE_LITERAL("C:\\")
600+
: RULES_ELISP_NATIVE_LITERAL("/"))
601+
.value();
602+
603+
EXPECT_THAT(CopyTree(*temp, to), StatusIs(absl::StatusCode::kAlreadyExists));
601604
}
602605

603606
TEST(CopyTreeTest, RejectsCopyToExistingDirectory) {
@@ -622,7 +625,7 @@ TEST(CopyTreeTest, RejectsCopyToExistingDirectory) {
622625
EXPECT_THAT(CreateDirectory(to_dir), IsOk());
623626
EXPECT_THAT(WriteFile(from_file, "contents"), IsOk());
624627

625-
EXPECT_THAT(CopyTree(from_dir.string(), to_dir.string()),
628+
EXPECT_THAT(CopyTree(from_dir, to_dir),
626629
StatusIs(absl::StatusCode::kAlreadyExists));
627630
}
628631

@@ -648,7 +651,7 @@ TEST(CopyTreeTest, CopiesToNewDirectory) {
648651
EXPECT_THAT(CreateDirectory(from_dir), IsOk());
649652
EXPECT_THAT(WriteFile(from_file, "contents"), IsOk());
650653

651-
EXPECT_THAT(CopyTree(from_dir.string(), to_dir.string()), IsOk());
654+
EXPECT_THAT(CopyTree(from_dir, to_dir), IsOk());
652655

653656
EXPECT_THAT(ReadFile(to_file), IsOkAndHolds("contents"));
654657
}
@@ -676,7 +679,7 @@ TEST(CopyTreeTest, IgnoresTrailingSlash) {
676679
EXPECT_THAT(CreateDirectory(from_dir), IsOk());
677680
EXPECT_THAT(WriteFile(from_file, "contents"), IsOk());
678681

679-
EXPECT_THAT(CopyTree(from_dir.string(), to_dir.string()), IsOk());
682+
EXPECT_THAT(CopyTree(from_dir, to_dir), IsOk());
680683

681684
EXPECT_THAT(ReadFile(to_file), IsOkAndHolds("contents"));
682685
}

0 commit comments

Comments
 (0)