Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: fix opts.filter issue in cp async #44922

Merged
merged 3 commits into from Oct 10, 2022

Conversation

thoqbk
Copy link
Contributor

@thoqbk thoqbk commented Oct 8, 2022

Fixes: #44720

issues:

  • copy API doesn't handle opts.filter properly.
    As a result, the path validation logic still gets triggered
    even though the file or folder is filtered out
  • no central place to handle opts.filter
    e.g. need to check and call handleFilter before all copies

changes:

  • use checkPaths as a central place to validate the paths
    (with consideration of opts.filter) before copying
  • cleanup: removing handleFilter, startCopy

related PR: #44786

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Oct 8, 2022
@thoqbk thoqbk force-pushed the fix-opts-filter-in-cp-async branch from c5c3fa6 to 37639ea Compare Oct 8, 2022
@thoqbk thoqbk force-pushed the fix-opts-filter-in-cp-async branch 2 times, most recently from 8c5a267 to bf9c3bb Compare Oct 8, 2022
@thoqbk thoqbk force-pushed the fix-opts-filter-in-cp-async branch from bf9c3bb to 8bc9f40 Compare Oct 8, 2022
lib/internal/fs/cp/cp.js Outdated Show resolved Hide resolved
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@thoqbk thoqbk requested a review from aduh95 Oct 9, 2022
lib/internal/fs/cp/cp.js Outdated Show resolved Hide resolved
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
aduh95
aduh95 approved these changes Oct 9, 2022
@aduh95
Copy link
Contributor

aduh95 commented Oct 9, 2022

/cc @nodejs/fs

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 9, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 9, 2022
@nodejs-github-bot
Copy link
Contributor

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 10, 2022
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 10, 2022
@nodejs-github-bot
Copy link
Contributor

Commit Queue failed
- Loading data for nodejs/node/pull/44922
✔  Done loading data for nodejs/node/pull/44922
----------------------------------- PR info ------------------------------------
Title      fs: fix opts.filter issue in cp async (#44922)
Author     Tho  (@thoqbk, first-time contributor)
Branch     thoqbk:fix-opts-filter-in-cp-async -> nodejs:main
Labels     fs, author ready, needs-ci
Commits    3
 - fs: fix opts.filter issue in cp async
 - fs: use if instead of ternary
 - Update lib/internal/fs/cp/cp.js
Committers 2
 - Tho 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/44922
Fixes: https://github.com/nodejs/node/issues/44720
Reviewed-By: Antoine du Hamel 
Reviewed-By: Minwoo Jung 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/44922
Fixes: https://github.com/nodejs/node/issues/44720
Reviewed-By: Antoine du Hamel 
Reviewed-By: Minwoo Jung 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sat, 08 Oct 2022 06:25:08 GMT
   ✔  Approvals: 2
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/44922#pullrequestreview-1135337026
   ✔  - Minwoo Jung (@JungMinu): https://github.com/nodejs/node/pull/44922#pullrequestreview-1135524737
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-10-09T11:43:51Z: https://ci.nodejs.org/job/node-test-pull-request/47137/
- Querying data for job/node-test-pull-request/47137/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 44922
From https://github.com/nodejs/node
 * branch                  refs/pull/44922/merge -> FETCH_HEAD
✔  Fetched commits as 78d280a76821..3fe69b82b4f2
--------------------------------------------------------------------------------
[main c5804042ee] fs: fix opts.filter issue in cp async
 Author: Tho 
 Date: Sat Oct 8 14:06:59 2022 +0800
 2 files changed, 45 insertions(+), 19 deletions(-)
[main 74ddfbb006] fs: use if instead of ternary
 Author: Tho 
 Date: Sun Oct 9 13:04:56 2022 +0800
 1 file changed, 1 insertion(+), 1 deletion(-)
[main a58683d117] Update lib/internal/fs/cp/cp.js
 Author: Tho 
 Date: Sun Oct 9 18:28:16 2022 +0800
 1 file changed, 1 insertion(+), 1 deletion(-)
   ✔  Patches applied
There are 3 commits in the PR. Attempting autorebase.
Rebasing (2/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
fs: fix opts.filter issue in cp async

PR-URL: #44922
Fixes: #44720
Reviewed-By: Antoine du Hamel duhamelantoine1995@gmail.com
Reviewed-By: Minwoo Jung nodecorelab@gmail.com

[detached HEAD 6ef84e8550] fs: fix opts.filter issue in cp async
Author: Tho thoqbk@gmail.com
Date: Sat Oct 8 14:06:59 2022 +0800
2 files changed, 45 insertions(+), 19 deletions(-)
Rebasing (3/6)
Rebasing (4/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
fs: use if instead of ternary

Co-authored-by: Antoine du Hamel duhamelantoine1995@gmail.com
PR-URL: #44922
Fixes: #44720
Reviewed-By: Antoine du Hamel duhamelantoine1995@gmail.com
Reviewed-By: Minwoo Jung nodecorelab@gmail.com

[detached HEAD 7530d14200] fs: use if instead of ternary
Author: Tho thoqbk@gmail.com
Date: Sun Oct 9 13:04:56 2022 +0800
1 file changed, 1 insertion(+), 1 deletion(-)
Rebasing (5/6)
Rebasing (6/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
Update lib/internal/fs/cp/cp.js

Co-authored-by: Antoine du Hamel duhamelantoine1995@gmail.com
PR-URL: #44922
Fixes: #44720
Reviewed-By: Antoine du Hamel duhamelantoine1995@gmail.com
Reviewed-By: Minwoo Jung nodecorelab@gmail.com

[detached HEAD a5e05135b8] Update lib/internal/fs/cp/cp.js
Author: Tho thoqbk@gmail.com
Date: Sun Oct 9 18:28:16 2022 +0800
1 file changed, 1 insertion(+), 1 deletion(-)

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/3220375855

@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Oct 10, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 10, 2022
@nodejs-github-bot nodejs-github-bot merged commit 5c9ea8a into nodejs:main Oct 10, 2022
58 checks passed
@nodejs-github-bot
Copy link
Contributor

Landed in 5c9ea8a

danielleadams pushed a commit that referenced this pull request Oct 11, 2022
PR-URL: #44922
Fixes: #44720
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[fs.cp] fails with EPERM despite filter
4 participants