Skip to content

fs: check subdir correctly in cpSync#55033

Merged
nodejs-github-bot merged 1 commit intonodejs:mainfrom
jazelly:fix-54285
Sep 28, 2024
Merged

fs: check subdir correctly in cpSync#55033
nodejs-github-bot merged 1 commit intonodejs:mainfrom
jazelly:fix-54285

Conversation

@jazelly
Copy link
Member

@jazelly jazelly commented Sep 20, 2024

Fixes: #54285

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Sep 20, 2024
@jazelly jazelly changed the title fs: check subdir correct in cpSync fs: check subdir correctly in cpSync Sep 20, 2024
avivkeller
avivkeller reviewed Sep 20, 2024
View reviewed changes
@jazelly jazelly marked this pull request as ready for review September 20, 2024 21:43
@jazelly
Copy link
Member Author

jazelly commented Sep 20, 2024 •
edited
Loading

This probably will have merge conflicts with #54653. I'd like the other one to land first as this one is easier to rebase, re-review and re-approve, if that sounds good : )

@jazelly jazelly marked this pull request as draft September 20, 2024 21:57
@avivkeller avivkeller added the blocked PRs that are blocked by other issues or PRs. label Sep 20, 2024
@jazelly jazelly marked this pull request as ready for review September 20, 2024 22:05
@avivkeller
Copy link
Member

avivkeller commented Sep 20, 2024

Per the comment above, I've added blocked, however, feel free to adjust.

@codecov
Copy link

codecov bot commented Sep 20, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.23%. Comparing base (cfe58cf) to head (9358d9e).
Report is 422 commits behind head on main.

Files with missing lines Patch % Lines
src/node_file.cc 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #55033   +/-   ##
=======================================
  Coverage   88.23%   88.23%           
=======================================
  Files         652      652           
  Lines      183912   183916    +4     
  Branches    35865    35862    -3     
=======================================
+ Hits       162272   162283   +11     
+ Misses      14915    14910    -5     
+ Partials     6725     6723    -2
Files with missing lines Coverage Δ
src/node_file.cc 77.06% <80.00%> (-0.01%) ⬇️

... and 33 files with indirect coverage changes

@jakecastelli
Copy link
Member

jakecastelli commented Sep 21, 2024

cc. @nodejs/fs @anonrig @aduh95 (surprised that both of you aren't in the fs team 😄)

jakecastelli
jakecastelli reviewed Sep 22, 2024
View reviewed changes
Copy link
Member

@jakecastelli jakecastelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other PR has landed and indeed there are merge conflicts now.

@jakecastelli jakecastelli removed the blocked PRs that are blocked by other issues or PRs. label Sep 22, 2024
@jazelly
Copy link
Member Author

jazelly commented Sep 22, 2024

Conflicts resolved ^ PTAL

@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 Sep 23, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2024
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 23, 2024

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

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 24, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 24, 2024
@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 24, 2024
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 24, 2024

Commit Queue failed
- Loading data for nodejs/node/pull/55033
✔  Done loading data for nodejs/node/pull/55033
----------------------------------- PR info ------------------------------------
Title      fs: check subdir correctly in cpSync (#55033)
Author     Jason Zhang <xzha4350@gmail.com> (@jazelly)
Branch     jazelly:fix-54285 -> nodejs:main
Labels     c++, fs, author ready, needs-ci
Commits    1
 - fs: check subdir correctly in cpSync
Committers 1
 - Jason Zhang <xzha4350@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/55033
Fixes: https://github.com/nodejs/node/issues/54285
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/55033
Fixes: https://github.com/nodejs/node/issues/54285
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 20 Sep 2024 21:03:08 GMT
   ✔  Approvals: 1
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/55033#pullrequestreview-2321643883
   ✘  This PR needs to wait 71 more hours to land (or 0 hours if there is one more approval)
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-09-23T09:40:40Z: https://ci.nodejs.org/job/node-test-pull-request/62685/
- Querying data for job/node-test-pull-request/62685/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/11021901134

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 27, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 27, 2024
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 27, 2024

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 28, 2024

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

@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Sep 28, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 28, 2024
@nodejs-github-bot nodejs-github-bot merged commit 7fe5bcd into nodejs:main Sep 28, 2024
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 28, 2024

Landed in 7fe5bcd

targos pushed a commit that referenced this pull request Oct 4, 2024
@jazelly @targos
fs: check subdir correctly in cpSync
a43ce87 
PR-URL: #55033
Fixes: #54285
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
targos pushed a commit that referenced this pull request Oct 4, 2024
@jazelly @targos
fs: check subdir correctly in cpSync
1c94725 
PR-URL: #55033
Fixes: #54285
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@aduh95 aduh95 mentioned this pull request Oct 9, 2024
Merged
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
@jazelly @louwers
fs: check subdir correctly in cpSync
8279f0c 
PR-URL: nodejs#55033
Fixes: nodejs#54285
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@marco-ippolito marco-ippolito added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Nov 16, 2024
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
@jazelly @tpoisseau
fs: check subdir correctly in cpSync
af617b0 
PR-URL: nodejs#55033
Fixes: nodejs#54285
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@jazelly jazelly deleted the fix-54285 branch November 25, 2024 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@avivkeller avivkeller avivkeller left review comments

@jakecastelli jakecastelli jakecastelli left review comments

@joyeecheung joyeecheung joyeecheung approved these changes

@aduh95 aduh95 aduh95 approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. 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.cpSync fails when copying to a directory with a name starting with the source directory name

7 participants

@jazelly @avivkeller @jakecastelli @nodejs-github-bot @joyeecheung @aduh95 @marco-ippolito