From 72b5526e142a41dd4c3dc203ecec4808470e362e Mon Sep 17 00:00:00 2001 From: Dario Piotrowicz Date: Sat, 7 Jun 2025 21:11:34 +0100 Subject: [PATCH 01/11] src: move cpSync dir copy logic completely to C++ prior to these changes cpSync would copy directories using C++ logic only if the user didn't provide a filtering function to the cpSync call, the changes here make it so that instead C++ is used to copy directories even when a filtering function is provided --- lib/internal/fs/cp/cp-sync.js | 65 ++++------------------------------- src/node_file.cc | 40 ++++++++++++++++++--- 2 files changed, 43 insertions(+), 62 deletions(-) diff --git a/lib/internal/fs/cp/cp-sync.js b/lib/internal/fs/cp/cp-sync.js index 03fcae9b7cdbda..c26335ee4bc353 100644 --- a/lib/internal/fs/cp/cp-sync.js +++ b/lib/internal/fs/cp/cp-sync.js @@ -22,8 +22,6 @@ const { chmodSync, copyFileSync, lstatSync, - mkdirSync, - opendirSync, readlinkSync, statSync, symlinkSync, @@ -33,7 +31,6 @@ const { const { dirname, isAbsolute, - join, resolve, } = require('path'); const { isPromise } = require('util/types'); @@ -65,7 +62,13 @@ function getStats(src, dest, opts) { const destStat = statSyncFn(dest, { bigint: true, throwIfNoEntry: false }); if (srcStat.isDirectory() && opts.recursive) { - return onDir(srcStat, destStat, src, dest, opts); + return fsBinding.cpSyncCopyDir(src, dest, + opts.force, + opts.dereference, + opts.errorOnExist, + opts.verbatimSymlinks, + opts.preserveTimestamps, + opts.filter); } else if (srcStat.isFile() || srcStat.isCharacterDevice() || srcStat.isBlockDevice()) { @@ -131,60 +134,6 @@ function setDestTimestamps(src, dest) { return utimesSync(dest, updatedSrcStat.atime, updatedSrcStat.mtime); } -// TODO(@anonrig): Move this function to C++. -function onDir(srcStat, destStat, src, dest, opts) { - if (!destStat) return copyDir(src, dest, opts, true, srcStat.mode); - return copyDir(src, dest, opts); -} - -function copyDir(src, dest, opts, mkDir, srcMode) { - if (!opts.filter) { - // The caller didn't provide a js filter function, in this case - // we can run the whole function faster in C++ - // TODO(dario-piotrowicz): look into making cpSyncCopyDir also accept the potential filter function - return fsBinding.cpSyncCopyDir(src, dest, - opts.force, - opts.dereference, - opts.errorOnExist, - opts.verbatimSymlinks, - opts.preserveTimestamps); - } - - if (mkDir) { - mkdirSync(dest); - } - - const dir = opendirSync(src); - - try { - let dirent; - - while ((dirent = dir.readSync()) !== null) { - const { name } = dirent; - const srcItem = join(src, name); - const destItem = join(dest, name); - let shouldCopy = true; - - if (opts.filter) { - shouldCopy = opts.filter(srcItem, destItem); - if (isPromise(shouldCopy)) { - throw new ERR_INVALID_RETURN_VALUE('boolean', 'filter', shouldCopy); - } - } - - if (shouldCopy) { - getStats(srcItem, destItem, opts); - } - } - } finally { - dir.closeSync(); - - if (srcMode !== undefined) { - setDestMode(dest, srcMode); - } - } -} - // TODO(@anonrig): Move this function to C++. function onLink(destStat, src, dest, verbatimSymlinks) { let resolvedSrc = readlinkSync(src); diff --git a/src/node_file.cc b/src/node_file.cc index 3d8ce43e88d961..edbae9a1919c09 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -3428,8 +3428,9 @@ bool isInsideDir(const std::filesystem::path& src, } static void CpSyncCopyDir(const FunctionCallbackInfo& args) { - CHECK_EQ(args.Length(), 7); // src, dest, force, dereference, errorOnExist, - // verbatimSymlinks, preserveTimestamps + CHECK_EQ(args.Length(), + 8); // src, dest, force, dereference, errorOnExist, + // verbatimSymlinks, preserveTimestamps, filterFunction Environment* env = Environment::GetCurrent(args); Isolate* isolate = env->isolate(); @@ -3448,6 +3449,27 @@ static void CpSyncCopyDir(const FunctionCallbackInfo& args) { bool verbatim_symlinks = args[5]->IsTrue(); bool preserve_timestamps = args[6]->IsTrue(); + std::function filter_fn = nullptr; + + if (args[7]->IsFunction()) { + Local args_filter_fn = args[7].As(); + + filter_fn = [env, args_filter_fn](std::string src, + std::string dest) -> bool { + Local argv[] = { + String::NewFromUtf8( + env->isolate(), src.c_str(), v8::NewStringType::kNormal) + .ToLocalChecked(), + String::NewFromUtf8( + env->isolate(), dest.c_str(), v8::NewStringType::kNormal) + .ToLocalChecked()}; + auto result = + args_filter_fn->Call(env->context(), Null(env->isolate()), 2, argv) + .ToLocalChecked(); + return result->BooleanValue(env->isolate()); + }; + } + std::error_code error; std::filesystem::create_directories(*dest, error); if (error) { @@ -3473,11 +3495,21 @@ static void CpSyncCopyDir(const FunctionCallbackInfo& args) { force, error_on_exist, dereference, - &isolate](std::filesystem::path src, - std::filesystem::path dest) { + &isolate, + &filter_fn](std::filesystem::path src, + std::filesystem::path dest) { std::error_code error; for (auto dir_entry : std::filesystem::directory_iterator(src)) { auto dest_file_path = dest / dir_entry.path().filename(); + + if (filter_fn) { + auto shouldSkip = + !filter_fn(dir_entry.path().c_str(), dest_file_path.c_str()); + if (shouldSkip) { + continue; + } + } + auto dest_str = PathToString(dest); if (dir_entry.is_symlink()) { From fb14768da43a0964b7187e6120be23309852143c Mon Sep 17 00:00:00 2001 From: Dario Piotrowicz Date: Sun, 8 Jun 2025 00:34:49 +0100 Subject: [PATCH 02/11] fixup! src: move cpSync dir copy logic completely to C++ use std::optional --- src/node_file.cc | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index edbae9a1919c09..a0f96dcee548a1 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -3449,7 +3449,7 @@ static void CpSyncCopyDir(const FunctionCallbackInfo& args) { bool verbatim_symlinks = args[5]->IsTrue(); bool preserve_timestamps = args[6]->IsTrue(); - std::function filter_fn = nullptr; + std::optional> filter_fn; if (args[7]->IsFunction()) { Local args_filter_fn = args[7].As(); @@ -3502,12 +3502,9 @@ static void CpSyncCopyDir(const FunctionCallbackInfo& args) { for (auto dir_entry : std::filesystem::directory_iterator(src)) { auto dest_file_path = dest / dir_entry.path().filename(); - if (filter_fn) { - auto shouldSkip = - !filter_fn(dir_entry.path().c_str(), dest_file_path.c_str()); - if (shouldSkip) { - continue; - } + if (filter_fn.has_value() && !filter_fn.value()(dir_entry.path().c_str(), + dest_file_path.c_str())) { + continue; } auto dest_str = PathToString(dest); From 8f6e69a842623f9ee873a90cd20af85dcb7b30bf Mon Sep 17 00:00:00 2001 From: Dario Piotrowicz Date: Sun, 8 Jun 2025 00:42:05 +0100 Subject: [PATCH 03/11] fixup! src: move cpSync dir copy logic completely to C++ use std:string_view --- src/node_file.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index a0f96dcee548a1..3f7f7243348799 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -3454,14 +3454,14 @@ static void CpSyncCopyDir(const FunctionCallbackInfo& args) { if (args[7]->IsFunction()) { Local args_filter_fn = args[7].As(); - filter_fn = [env, args_filter_fn](std::string src, - std::string dest) -> bool { + filter_fn = [env, args_filter_fn](std::string_view src, + std::string_view dest) -> bool { Local argv[] = { String::NewFromUtf8( - env->isolate(), src.c_str(), v8::NewStringType::kNormal) + env->isolate(), src.data(), v8::NewStringType::kNormal) .ToLocalChecked(), String::NewFromUtf8( - env->isolate(), dest.c_str(), v8::NewStringType::kNormal) + env->isolate(), dest.data(), v8::NewStringType::kNormal) .ToLocalChecked()}; auto result = args_filter_fn->Call(env->context(), Null(env->isolate()), 2, argv) From 6437abb0c07394b666a867a17e15565c829f959b Mon Sep 17 00:00:00 2001 From: Dario Piotrowicz Date: Sun, 8 Jun 2025 01:06:41 +0100 Subject: [PATCH 04/11] Update src/node_file.cc Co-authored-by: Yagiz Nizipli --- src/node_file.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_file.cc b/src/node_file.cc index 3f7f7243348799..e79a4df359ce25 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -3502,7 +3502,7 @@ static void CpSyncCopyDir(const FunctionCallbackInfo& args) { for (auto dir_entry : std::filesystem::directory_iterator(src)) { auto dest_file_path = dest / dir_entry.path().filename(); - if (filter_fn.has_value() && !filter_fn.value()(dir_entry.path().c_str(), + if (filter_fn && !filter_fn->(dir_entry.path().c_str(), dest_file_path.c_str())) { continue; } From 39d2447f88b7db5799831d61281d9d06533d28b4 Mon Sep 17 00:00:00 2001 From: Dario Piotrowicz Date: Sun, 8 Jun 2025 01:09:36 +0100 Subject: [PATCH 05/11] fix cpp formatting --- src/node_file.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index e79a4df359ce25..30d23d5be60b82 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -3502,8 +3502,8 @@ static void CpSyncCopyDir(const FunctionCallbackInfo& args) { for (auto dir_entry : std::filesystem::directory_iterator(src)) { auto dest_file_path = dest / dir_entry.path().filename(); - if (filter_fn && !filter_fn->(dir_entry.path().c_str(), - dest_file_path.c_str())) { + if (filter_fn && + !filter_fn->(dir_entry.path().c_str(), dest_file_path.c_str())) { continue; } From 431bcd13c3d1bef75b3c3cfec02a7009f397d317 Mon Sep 17 00:00:00 2001 From: Dario Piotrowicz Date: Sun, 8 Jun 2025 13:51:44 +0100 Subject: [PATCH 06/11] fix unqualified-id error --- src/node_file.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_file.cc b/src/node_file.cc index 30d23d5be60b82..67c30d350d5d14 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -3503,7 +3503,7 @@ static void CpSyncCopyDir(const FunctionCallbackInfo& args) { auto dest_file_path = dest / dir_entry.path().filename(); if (filter_fn && - !filter_fn->(dir_entry.path().c_str(), dest_file_path.c_str())) { + !(*filter_fn)(dir_entry.path().c_str(), dest_file_path.c_str())) { continue; } From ed9a4806ea9d65a77aedb0df990c17e75d9ad99c Mon Sep 17 00:00:00 2001 From: Dario Piotrowicz Date: Sun, 8 Jun 2025 14:11:11 +0100 Subject: [PATCH 07/11] avoid ToLocalChecked calls --- src/node_file.cc | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index 67c30d350d5d14..64dbdd6f05b4ce 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -3456,16 +3456,28 @@ static void CpSyncCopyDir(const FunctionCallbackInfo& args) { filter_fn = [env, args_filter_fn](std::string_view src, std::string_view dest) -> bool { - Local argv[] = { - String::NewFromUtf8( - env->isolate(), src.data(), v8::NewStringType::kNormal) - .ToLocalChecked(), - String::NewFromUtf8( - env->isolate(), dest.data(), v8::NewStringType::kNormal) - .ToLocalChecked()}; - auto result = - args_filter_fn->Call(env->context(), Null(env->isolate()), 2, argv) - .ToLocalChecked(); + Local src_arg; + Local dest_arg; + + if (!String::NewFromUtf8( + env->isolate(), src.data(), v8::NewStringType::kNormal) + .ToLocal(&src_arg) || + !String::NewFromUtf8( + env->isolate(), dest.data(), v8::NewStringType::kNormal) + .ToLocal(&dest_arg)) { + // if for some reason we fail to load the src or dest strings + // just skip the filtering function and allow the copy + return true; + } + + Local argv[] = {src_arg, dest_arg}; + + Local result; + if (!args_filter_fn->Call(env->context(), Null(env->isolate()), 2, argv) + .ToLocal(&result)) { + // if the call failed for whatever reason allow the copy + return true; + } return result->BooleanValue(env->isolate()); }; } From 8563b730aa740c80dc62b1742f2c8ce2e2a52d3c Mon Sep 17 00:00:00 2001 From: Dario Piotrowicz Date: Sun, 8 Jun 2025 23:11:36 +0100 Subject: [PATCH 08/11] (try to) fix windows paths issue --- src/node_file.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index 64dbdd6f05b4ce..2270e1c3c5af2f 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -3512,10 +3512,12 @@ static void CpSyncCopyDir(const FunctionCallbackInfo& args) { std::filesystem::path dest) { std::error_code error; for (auto dir_entry : std::filesystem::directory_iterator(src)) { + auto dir_entry_path_str = PathToString(dir_entry.path()); auto dest_file_path = dest / dir_entry.path().filename(); + auto dest_file_path_str = PathToString(dest_file_path); if (filter_fn && - !(*filter_fn)(dir_entry.path().c_str(), dest_file_path.c_str())) { + !(*filter_fn)(dir_entry_path_str.c_str(), dest_file_path_str.c_str())) { continue; } @@ -3582,7 +3584,6 @@ static void CpSyncCopyDir(const FunctionCallbackInfo& args) { } } else if (std::filesystem::is_regular_file(dest_file_path)) { if (!dereference || (!force && error_on_exist)) { - auto dest_file_path_str = PathToString(dest_file_path); env->ThrowStdErrException( std::make_error_code(std::errc::file_exists), "cp", From 4d9cd5141315bd93073e59f168a8641faca3bacf Mon Sep 17 00:00:00 2001 From: Dario Piotrowicz Date: Sun, 8 Jun 2025 23:14:16 +0100 Subject: [PATCH 09/11] fix cpp linting error --- src/node_file.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/node_file.cc b/src/node_file.cc index 2270e1c3c5af2f..e72d932f311ef0 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -3517,7 +3517,8 @@ static void CpSyncCopyDir(const FunctionCallbackInfo& args) { auto dest_file_path_str = PathToString(dest_file_path); if (filter_fn && - !(*filter_fn)(dir_entry_path_str.c_str(), dest_file_path_str.c_str())) { + !(*filter_fn)(dir_entry_path_str.c_str(), + dest_file_path_str.c_str())) { continue; } From d95e9896c4754ff4234e45d083b4d19dfa8b3b74 Mon Sep 17 00:00:00 2001 From: Dario Piotrowicz Date: Sun, 8 Jun 2025 23:31:38 +0100 Subject: [PATCH 10/11] actually fix cpp formatting --- src/node_file.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index e72d932f311ef0..7f3247c8a1a3d1 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -3516,9 +3516,8 @@ static void CpSyncCopyDir(const FunctionCallbackInfo& args) { auto dest_file_path = dest / dir_entry.path().filename(); auto dest_file_path_str = PathToString(dest_file_path); - if (filter_fn && - !(*filter_fn)(dir_entry_path_str.c_str(), - dest_file_path_str.c_str())) { + if (filter_fn && !(*filter_fn)(dir_entry_path_str.c_str(), + dest_file_path_str.c_str())) { continue; } From be0da1494d11a0b88cfeb849757fe1a9835f4635 Mon Sep 17 00:00:00 2001 From: Dario Piotrowicz Date: Thu, 12 Jun 2025 21:32:16 +0100 Subject: [PATCH 11/11] Apply suggestions from code review Co-authored-by: Anna Henningsen --- src/node_file.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index 7f3247c8a1a3d1..ccce9907ce02b8 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -3449,7 +3449,8 @@ static void CpSyncCopyDir(const FunctionCallbackInfo& args) { bool verbatim_symlinks = args[5]->IsTrue(); bool preserve_timestamps = args[6]->IsTrue(); - std::optional> filter_fn; + std::optional> + filter_fn; if (args[7]->IsFunction()) { Local args_filter_fn = args[7].As(); @@ -3516,8 +3517,7 @@ static void CpSyncCopyDir(const FunctionCallbackInfo& args) { auto dest_file_path = dest / dir_entry.path().filename(); auto dest_file_path_str = PathToString(dest_file_path); - if (filter_fn && !(*filter_fn)(dir_entry_path_str.c_str(), - dest_file_path_str.c_str())) { + if (filter_fn && !(*filter_fn)(dir_entry_path_str, dest_file_path_str)) { continue; }