FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fs: fix opts.filter issue in cp async by thoqbk · Pull Request #44922 · nodejs/node · GitHub

/ node Public

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

Merged
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
thoqbk:fix-opts-filter-in-cp-async
Oct 10, 2022
ConversationCommits (3)ChecksFiles changed
Merged

fs: fix opts.filter issue in cp async#44922
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
thoqbk:fix-opts-filter-in-cp-async

Conversation

thoqbk commented Oct 8, 2022 •
edited by aduh95
Loading

Copy link
Copy Markdown
Contributor

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 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 force-pushed the fix-opts-filter-in-cp-async branch from c5c3fa6 to 37639ea Compare October 8, 2022 06:33
thoqbk commented Oct 8, 2022
View reviewed changes
Comment thread test/parallel/test-fs-cp.mjs Outdated
thoqbk commented Oct 8, 2022
View reviewed changes
Comment thread test/parallel/test-fs-cp.mjs Outdated
thoqbk force-pushed the fix-opts-filter-in-cp-async branch 2 times, most recently from 8c5a267 to bf9c3bb Compare October 8, 2022 11:40
thoqbk force-pushed the fix-opts-filter-in-cp-async branch from bf9c3bb to 8bc9f40 Compare October 8, 2022 14:12
aduh95 reviewed Oct 9, 2022
View reviewed changes
Comment thread lib/internal/fs/cp/cp.js Outdated
fs: use if instead of ternary
b65df06 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
thoqbk requested a review from aduh95 October 9, 2022 05:08
aduh95 reviewed Oct 9, 2022
View reviewed changes
Comment thread lib/internal/fs/cp/cp.js Outdated
Update lib/internal/fs/cp/cp.js
3fe69b8 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

aduh95 commented Oct 9, 2022

Copy link
Copy Markdown
Contributor

/cc @nodejs/fs

aduh95 added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 9, 2022
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 commented Oct 9, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/47137/

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 10, 2022
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 commented Oct 10, 2022

Copy link
Copy Markdown
Collaborator
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 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 removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 10, 2022
nodejs-github-bot merged commit 5c9ea8a into nodejs:main Oct 10, 2022

nodejs-github-bot commented Oct 10, 2022

Copy link
Copy Markdown
Collaborator

Landed in 5c9ea8a

danielleadams pushed a commit that referenced this pull request Oct 11, 2022
fs: fix opts.filter issue in cp async
bc00f3b 
PR-URL: #44922
Fixes: #44720
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
danielleadams mentioned this pull request Oct 11, 2022
Merged
This was referenced Oct 20, 2022
Closed
Merged
thoqbk mentioned this pull request Oct 24, 2022
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

JungMinu JungMinu approved these changes
aduh95 aduh95 approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[fs.cp] fails with EPERM despite filter

4 participants

Back | FazBrowse Home | New Git URL