Skip to content

Commit 5528a0a

Browse files
phstphst
committed
Use FileName class in CopyTree function
1 parent 5644125 commit 5528a0a

File tree

3 files changed

+73
-87
lines changed

3 files changed

+73
-87
lines changed

‎elisp/private/tools/system.cc‎

Lines changed: 57 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -284,12 +284,14 @@ static NativeChar* absl_nonnull Pointer(
284284
return string.data();
285285
}
286286

287+
#ifdef _WIN32
287288
static const NativeChar* absl_nonnull Pointer(
288289
const NativeString& string ABSL_ATTRIBUTE_LIFETIME_BOUND) {
289290
CHECK(!ContainsNull(string))
290291
<< absl::StrFormat("%s contains null character", string);
291292
return string.data();
292293
}
294+
#endif
293295

294296
#ifndef _WIN32
295297
static std::vector<char* absl_nullable> Pointers(
@@ -622,12 +624,11 @@ absl::StatusOr<std::vector<FileName>> ListDirectory(const FileName& dir) {
622624
#endif
623625

624626
#ifdef _WIN32
625-
static absl::Status CopyTreeWindows(const std::wstring& from,
626-
const std::wstring& to) {
627-
if (!::CreateDirectoryExW(Pointer(from), Pointer(to), nullptr)) {
627+
static absl::Status CopyTreeWindows(const FileName& from, const FileName& to) {
628+
if (!::CreateDirectoryExW(from.pointer(), to.pointer(), nullptr)) {
628629
return WindowsStatus("CreateDirectoryExW", from, to, nullptr);
629630
}
630-
const std::wstring pattern = from + L"\\*";
631+
const std::wstring pattern = from.string() + L"\\*";
631632
WIN32_FIND_DATAW data;
632633
const HANDLE handle = ::FindFirstFileW(Pointer(pattern), &data);
633634
if (handle == INVALID_HANDLE_VALUE) {
@@ -639,18 +640,20 @@ static absl::Status CopyTreeWindows(const std::wstring& from,
639640
if (!::FindClose(handle)) LOG(ERROR) << WindowsStatus("FindClose");
640641
};
641642
do {
642-
const std::wstring name = data.cFileName;
643+
const std::wstring_view name = data.cFileName;
643644
if (name == L"." || name == L"..") continue;
644-
const std::wstring from_entry = from + L'\\' + name;
645-
const std::wstring to_entry = to + L'\\' + name;
645+
const absl::StatusOr<FileName> from_entry = from.Child(name);
646+
if (!from_entry.ok()) return from_entry.status();
647+
const absl::StatusOr<FileName> to_entry = to.Child(name);
648+
if (!to_entry.ok()) return to_entry.status();
646649
if (data.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) {
647-
const absl::Status status = CopyTreeWindows(from_entry, to_entry);
650+
const absl::Status status = CopyTreeWindows(*from_entry, *to_entry);
648651
if (!status.ok()) return status;
649652
} else {
650653
constexpr DWORD flags = COPY_FILE_FAIL_IF_EXISTS | COPY_FILE_COPY_SYMLINK;
651-
if (!::CopyFileExW(Pointer(from_entry), Pointer(to_entry), nullptr,
654+
if (!::CopyFileExW(from_entry->pointer(), to_entry->pointer(), nullptr,
652655
nullptr, nullptr, flags)) {
653-
return WindowsStatus("CopyFileExW", from_entry, to_entry, nullptr,
656+
return WindowsStatus("CopyFileExW", *from_entry, *to_entry, nullptr,
654657
nullptr, nullptr, absl::Hex(flags));
655658
}
656659
}
@@ -664,16 +667,16 @@ static absl::Status CopyTreeWindows(const std::wstring& from,
664667

665668
#ifdef RULES_ELISP_COPY_TREE_POSIX
666669
static absl::Status CopyFilePosix(const int from_parent, const int to_parent,
667-
const std::string& name,
670+
const FileName& name,
668671
const struct stat& from_stat) {
669-
CHECK(!name.empty());
670-
CHECK_NE(name, ".");
671-
CHECK_NE(name, "..");
672-
CHECK_EQ(name.find('/'), name.npos) << "invalid name " << name;
672+
CHECK_NE(name.string(), ".");
673+
CHECK_NE(name.string(), "..");
674+
CHECK_EQ(name.string().find('/'), name.string().npos)
675+
<< "invalid name " << name;
673676
CHECK(S_ISREG(from_stat.st_mode)) << "file " << name << " is not regular";
674677

675678
constexpr int from_flags = O_RDONLY | O_CLOEXEC | O_NOCTTY | O_NOFOLLOW;
676-
const int from_fd = openat(from_parent, Pointer(name), from_flags);
679+
const int from_fd = openat(from_parent, name.pointer(), from_flags);
677680
if (from_fd < 0) {
678681
return ErrnoStatus("openat", from_parent, name, absl::Hex(from_flags));
679682
}
@@ -684,7 +687,7 @@ static absl::Status CopyFilePosix(const int from_parent, const int to_parent,
684687
const mode_t mode = from_stat.st_mode;
685688
constexpr int to_flags =
686689
O_WRONLY | O_CREAT | O_EXCL | O_CLOEXEC | O_NOCTTY | O_NOFOLLOW;
687-
const int to_fd = openat(to_parent, Pointer(name), to_flags, mode);
690+
const int to_fd = openat(to_parent, name.pointer(), to_flags, mode);
688691
if (to_fd < 0) {
689692
return ErrnoStatus("openat", to_parent, name, to_flags, Oct(mode));
690693
}
@@ -710,7 +713,7 @@ static absl::Status CopyFilePosix(const int from_parent, const int to_parent,
710713
if (written == 0) {
711714
// Prevent infinite loop.
712715
return absl::DataLossError(absl::StrFormat(
713-
"Cannot write %d bytes to %s", remaining.size(), name));
716+
"Cannot write %d bytes to %v", remaining.size(), name));
714717
}
715718
remaining.remove_prefix(
716719
CastNumber<std::string_view::size_type>(written).value());
@@ -727,25 +730,25 @@ static absl::Status CopyFilePosix(const int from_parent, const int to_parent,
727730
}
728731

729732
static absl::Status CopyTreePosix(const int from_parent,
730-
const std::string& from_name,
733+
const FileName& from_name,
731734
std::optional<struct stat> from_stat,
732735
const int to_parent,
733-
const std::string& to_name) {
734-
CHECK(!from_name.empty());
735-
CHECK_NE(from_name, ".");
736-
CHECK_NE(from_name, "..");
737-
CHECK(from_parent == AT_FDCWD || from_name.find('/') == from_name.npos)
736+
const FileName& to_name) {
737+
CHECK_NE(from_name.string(), ".");
738+
CHECK_NE(from_name.string(), "..");
739+
CHECK(from_parent == AT_FDCWD ||
740+
from_name.string().find('/') == from_name.string().npos)
738741
<< "invalid source name " << from_name;
739742

740-
CHECK(!to_name.empty());
741-
CHECK_NE(to_name, ".");
742-
CHECK_NE(to_name, "..");
743-
CHECK(to_parent == AT_FDCWD || to_name.find('/') == to_name.npos)
743+
CHECK_NE(to_name.string(), ".");
744+
CHECK_NE(to_name.string(), "..");
745+
CHECK(to_parent == AT_FDCWD ||
746+
to_name.string().find('/') == to_name.string().npos)
744747
<< "invalid target name " << to_name;
745748

746749
const int from_flags =
747750
O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC | O_NOCTTY;
748-
const int from_fd = openat(from_parent, Pointer(from_name), from_flags);
751+
const int from_fd = openat(from_parent, from_name.pointer(), from_flags);
749752
if (from_fd < 0) {
750753
return ErrnoStatus("openat", from_parent, from_name, absl::Hex(from_flags));
751754
}
@@ -760,7 +763,7 @@ static absl::Status CopyTreePosix(const int from_parent,
760763
if (fstat(from_fd, &buffer) != 0) return ErrnoStatus("fstat", from_fd);
761764
if (!S_ISDIR(buffer.st_mode)) {
762765
return absl::FailedPreconditionError(
763-
absl::StrFormat("Source %s is not a directory", from_name));
766+
absl::StrFormat("Source %v is not a directory", from_name));
764767
}
765768
from_stat = buffer;
766769
}
@@ -775,12 +778,12 @@ static absl::Status CopyTreePosix(const int from_parent,
775778
};
776779

777780
const mode_t mode = from_stat->st_mode | S_IWUSR;
778-
const int result = mkdirat(to_parent, Pointer(to_name), mode);
781+
const int result = mkdirat(to_parent, to_name.pointer(), mode);
779782
if (result != 0) return ErrnoStatus("mkdirat", to_parent, to_name, Oct(mode));
780783

781784
constexpr int to_flags =
782785
O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC | O_NOCTTY;
783-
const int to_fd = openat(to_parent, Pointer(to_name), to_flags);
786+
const int to_fd = openat(to_parent, to_name.pointer(), to_flags);
784787
if (to_fd < 0) {
785788
return ErrnoStatus("openat", to_parent, to_name, absl::Hex(to_flags));
786789
}
@@ -795,8 +798,10 @@ static absl::Status CopyTreePosix(const int from_parent,
795798
if (errno != 0) return ErrnoStatus("readdir");
796799
break;
797800
}
798-
const std::string name = entry->d_name;
801+
const std::string_view name = entry->d_name;
799802
if (name == "." || name == "..") continue;
803+
const absl::StatusOr<FileName> file = FileName::FromString(name);
804+
if (!file.ok()) return file.status();
800805

801806
struct stat buffer;
802807
constexpr int stat_flags = AT_SYMLINK_NOFOLLOW;
@@ -807,10 +812,10 @@ static absl::Status CopyTreePosix(const int from_parent,
807812
const mode_t mode = buffer.st_mode;
808813
if (S_ISDIR(mode)) {
809814
const absl::Status status =
810-
CopyTreePosix(from_fd, name, buffer, to_fd, name);
815+
CopyTreePosix(from_fd, *file, buffer, to_fd, *file);
811816
if (!status.ok()) return status;
812817
} else if (S_ISREG(mode)) {
813-
const absl::Status status = CopyFilePosix(from_fd, to_fd, name, buffer);
818+
const absl::Status status = CopyFilePosix(from_fd, to_fd, *file, buffer);
814819
if (!status.ok()) return status;
815820
} else {
816821
return absl::FailedPreconditionError(absl::StrFormat(
@@ -826,78 +831,56 @@ static absl::Status CopyTreePosix(const int from_parent,
826831
}
827832
#endif
828833

829-
absl::Status CopyTree(NativeStringView from, NativeStringView to) {
830-
if (from.empty()) {
831-
return absl::InvalidArgumentError("Empty source directory name");
832-
}
833-
if (ContainsNull(from)) {
834-
return absl::InvalidArgumentError(absl::StrFormat(
835-
"Source directory name %s contains null character", from));
836-
}
837-
if (to.empty()) {
838-
return absl::InvalidArgumentError("Empty destination directory name");
839-
}
840-
if (ContainsNull(to)) {
841-
return absl::InvalidArgumentError(absl::StrFormat(
842-
"Destination directory name %s contains null character", from));
843-
}
844-
834+
absl::Status CopyTree(const FileName& from, const FileName& to) {
845835
#if defined _WIN32
846-
const absl::StatusOr<FileName> from_name = FileName::FromString(from);
847-
if (!from_name.ok()) return from_name.status();
848-
const absl::StatusOr<FileName> from_abs = from_name->MakeAbsolute();
836+
const absl::StatusOr<FileName> from_abs = from.MakeAbsolute();
849837
if (!from_abs.ok()) return from_abs.status();
850838
if (from_abs->string().length() < 4) {
851839
return absl::InvalidArgumentError(
852840
absl::StrFormat("Cannot copy drive %v", *from_abs));
853841
}
854-
const absl::StatusOr<FileName> to_name = FileName::FromString(to);
855-
if (!to_name.ok()) return to_name.status();
856-
const absl::StatusOr<FileName> to_abs = to_name->MakeAbsolute();
842+
const absl::StatusOr<FileName> to_abs = to.MakeAbsolute();
857843
if (!to_abs.ok()) return to_abs.status();
858844
if (to_abs->string().length() < 4) {
859845
return absl::AlreadyExistsError(
860846
absl::StrFormat("Cannot copy over drive %v", *from_abs));
861847
}
862-
return CopyTreeWindows(from_abs->string(), to_abs->string());
848+
return CopyTreeWindows(*from_abs, *to_abs);
863849
#elif defined __APPLE__
850+
std::string from_str = from.string();
864851
// The behavior of copyfile changes if the source directory name ends in a
865852
// slash, see the man page copyfile(3). We need to remove trailing slashes to
866853
// get the expected behavior.
867-
const std::string_view::size_type i = from.find_last_not_of('/');
868-
from.remove_suffix(from.size() - (i == from.npos ? 0 : i + 1));
869-
if (from.empty()) {
854+
while (from_str.back() == '/') from_str.pop_back();
855+
if (from_str.empty()) {
870856
// If the source directory name is empty, it has consisted only of slashes.
871857
return absl::InvalidArgumentError("Cannot copy from root directory");
872858
}
873-
const std::string from_str(from);
874-
const std::string to_str(to);
875859
// The behavior of copyfile changes if the destination directory already
876860
// exists, see the man page copyfile(3).
877861
struct stat buffer;
878-
if (lstat(Pointer(to_str), &buffer) == 0) {
862+
if (lstat(to.pointer(), &buffer) == 0) {
879863
return absl::AlreadyExistsError(
880-
absl::StrFormat("Destination directory %s already exists", to_str));
864+
absl::StrFormat("Destination directory %v already exists", to));
881865
}
882-
if (errno != ENOENT) return ErrnoStatus("lstat", to_str);
866+
if (errno != ENOENT) return ErrnoStatus("lstat", to);
883867
constexpr copyfile_flags_t flags = COPYFILE_ALL | COPYFILE_RECURSIVE |
884868
COPYFILE_EXCL | COPYFILE_NOFOLLOW |
885869
COPYFILE_CLONE | COPYFILE_DATA_SPARSE;
886-
if (copyfile(Pointer(from_str), Pointer(to_str), nullptr, flags) != 0) {
887-
return ErrnoStatus("copyfile", from_str, to_str, nullptr, absl::Hex(flags));
870+
if (copyfile(Pointer(from_str), to.pointer(), nullptr, flags) != 0) {
871+
return ErrnoStatus("copyfile", from_str, to, nullptr, absl::Hex(flags));
888872
}
889873
return absl::OkStatus();
890874
#elif defined RULES_ELISP_COPY_TREE_POSIX
891-
if (from.find_first_not_of('/') == from.npos) {
875+
if (from.string().find_first_not_of('/') == from.string().npos) {
892876
return absl::InvalidArgumentError(
893-
absl::StrFormat("Cannot copy from root directory %s", from));
877+
absl::StrFormat("Cannot copy from root directory %v", from));
894878
}
895-
if (to.find_first_not_of('/') == to.npos) {
879+
if (to.string().find_first_not_of('/') == to.string().npos) {
896880
return absl::AlreadyExistsError(
897-
absl::StrFormat("Cannot overwrite root directory %s", to));
881+
absl::StrFormat("Cannot overwrite root directory %v", to));
898882
}
899-
return CopyTreePosix(AT_FDCWD, std::string(from), std::nullopt, AT_FDCWD,
900-
std::string(to));
883+
return CopyTreePosix(AT_FDCWD, from, std::nullopt, AT_FDCWD, to);
901884
#else
902885
# error CopyTree not implemented on this system
903886
#endif

‎elisp/private/tools/system.h‎

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

244244
class Environment final {
245245
private:

‎tests/tools/system_test.cc‎

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

522-
EXPECT_THAT(CopyTree(temp->string(), temp->string()),
522+
EXPECT_THAT(CopyTree(*temp, *temp),
523523
StatusIs(AnyOf(absl::StatusCode::kAlreadyExists,
524524
absl::StatusCode::kNotFound)));
525525
}
@@ -528,12 +528,13 @@ TEST(CopyTreeTest, RejectsCopyFromRootDirectory) {
528528
const absl::StatusOr<FileName> temp = TempDir();
529529
ASSERT_THAT(temp, IsOk());
530530

531+
const FileName from =
532+
FileName::FromString(kWindows ? RULES_ELISP_NATIVE_LITERAL("C:\\")
533+
: RULES_ELISP_NATIVE_LITERAL("/"))
534+
.value();
531535
const FileName to = temp->Child(RULES_ELISP_NATIVE_LITERAL("to")).value();
532536

533-
EXPECT_THAT(CopyTree(kWindows ? RULES_ELISP_NATIVE_LITERAL("C:\\")
534-
: RULES_ELISP_NATIVE_LITERAL("/"),
535-
to.string()),
536-
StatusIs(absl::StatusCode::kInvalidArgument));
537+
EXPECT_THAT(CopyTree(from, to), StatusIs(absl::StatusCode::kInvalidArgument));
537538

538539
EXPECT_THAT(RemoveDirectory(to), StatusIs(absl::StatusCode::kNotFound));
539540
}
@@ -542,10 +543,12 @@ TEST(CopyTreeTest, RejectsCopyToRootDirectory) {
542543
const absl::StatusOr<FileName> temp = TempDir();
543544
ASSERT_THAT(temp, IsOk());
544545

545-
EXPECT_THAT(
546-
CopyTree(temp->string(), kWindows ? RULES_ELISP_NATIVE_LITERAL("C:\\")
547-
: RULES_ELISP_NATIVE_LITERAL("/")),
548-
StatusIs(absl::StatusCode::kAlreadyExists));
546+
const FileName to =
547+
FileName::FromString(kWindows ? RULES_ELISP_NATIVE_LITERAL("C:\\")
548+
: RULES_ELISP_NATIVE_LITERAL("/"))
549+
.value();
550+
551+
EXPECT_THAT(CopyTree(*temp, to), StatusIs(absl::StatusCode::kAlreadyExists));
549552
}
550553

551554
TEST(CopyTreeTest, RejectsCopyToExistingDirectory) {
@@ -570,7 +573,7 @@ TEST(CopyTreeTest, RejectsCopyToExistingDirectory) {
570573
EXPECT_THAT(CreateDirectory(to_dir), IsOk());
571574
EXPECT_THAT(WriteFile(from_file, "contents"), IsOk());
572575

573-
EXPECT_THAT(CopyTree(from_dir.string(), to_dir.string()),
576+
EXPECT_THAT(CopyTree(from_dir, to_dir),
574577
StatusIs(absl::StatusCode::kAlreadyExists));
575578
}
576579

@@ -596,7 +599,7 @@ TEST(CopyTreeTest, CopiesToNewDirectory) {
596599
EXPECT_THAT(CreateDirectory(from_dir), IsOk());
597600
EXPECT_THAT(WriteFile(from_file, "contents"), IsOk());
598601

599-
EXPECT_THAT(CopyTree(from_dir.string(), to_dir.string()), IsOk());
602+
EXPECT_THAT(CopyTree(from_dir, to_dir), IsOk());
600603

601604
EXPECT_THAT(ReadFile(to_file), IsOkAndHolds("contents"));
602605
}
@@ -624,7 +627,7 @@ TEST(CopyTreeTest, IgnoresTrailingSlash) {
624627
EXPECT_THAT(CreateDirectory(from_dir), IsOk());
625628
EXPECT_THAT(WriteFile(from_file, "contents"), IsOk());
626629

627-
EXPECT_THAT(CopyTree(from_dir.string(), to_dir.string()), IsOk());
630+
EXPECT_THAT(CopyTree(from_dir, to_dir), IsOk());
628631

629632
EXPECT_THAT(ReadFile(to_file), IsOkAndHolds("contents"));
630633
}

0 commit comments

Comments
 (0)