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 fs.rm support for loop symlinks #45439

Merged
merged 4 commits into from
Nov 18, 2022

Conversation

nathanael-ruf
Copy link
Contributor

@nathanael-ruf nathanael-ruf commented Nov 12, 2022

Fixes: #45404

@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 Nov 12, 2022
@nathanael-ruf
Copy link
Contributor Author

The tests have quite a bit of duplicated code (both existing and my new tests). I can add a new block at the bottom of the file that adds the new tests for the sync, async and promise version without so much duplicated code if you want.

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 12, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 12, 2022
@nodejs-github-bot

This comment was marked as outdated.

@Trott
Copy link
Member

Trott commented Nov 12, 2022

The tests have quite a bit of duplicated code (both existing and my new tests). I can add a new block at the bottom of the file that adds the new tests for the sync, async and promise version without so much duplicated code if you want.

For tests only, I generally prefer the repetition. (Ref: https://stackoverflow.com/a/11837973/436641)

@Trott Trott added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Nov 12, 2022
@aduh95 aduh95 changed the title fs: fix deleting invalid/loop symlinks. add tests. fs: fix fs.rm support for loop symlinks Nov 12, 2022
@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 12, 2022
test/parallel/test-fs-rm.js Outdated Show resolved Hide resolved
test/parallel/test-fs-rm.js Outdated Show resolved Hide resolved
@aduh95 aduh95 removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 12, 2022
@nathanael-ruf
Copy link
Contributor Author

nathanael-ruf commented Nov 13, 2022

Thanks for fixing the commit message, one question though: For something like addressing the review comments, should I always amend and force push or is another commit okay for this (I guess you can always squash at the end)?

Also I think the title of the PR is no longer 100% accurate, because deleting invalid links was also broken before (the stat call failed with ENOENT):

nathanael ~/tmp λ tree
.
└── b -> a

nathanael ~/tmp λ node -e 'fs.rmSync("b")'
node:internal/fs/utils:344
    throw err;
    ^

Error: ENOENT: no such file or directory, stat 'b'
    at Object.statSync (node:fs:1538:3)
    at __node_internal_ (node:internal/fs/utils:793:8)
    at Object.rmSync (node:fs:1213:13)
    at [eval]:1:4
    at Script.runInThisContext (node:vm:129:12)
    at Object.runInThisContext (node:vm:305:38)
    at node:internal/process/execution:76:19
    at [eval]-wrapper:6:22
    at evalScript (node:internal/process/execution:75:60)
    at node:internal/main/eval_string:27:3 {
  errno: -2,
  syscall: 'stat',
  code: 'ENOENT',
  path: 'b'
}

The loop thing was just the first issue I encountered.

@aduh95
Copy link
Contributor

aduh95 commented Nov 13, 2022 via email

@aduh95
Copy link
Contributor

aduh95 commented Nov 13, 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 Nov 13, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 13, 2022
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

Copy link
Contributor

@LiviaMedeiros LiviaMedeiros left a comment

Choose a reason for hiding this comment

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

LGTM with a suggestion.

test/parallel/test-fs-rm.js Show resolved Hide resolved
test/parallel/test-fs-rm.js Show resolved Hide resolved
test/parallel/test-fs-rm.js Show resolved Hide resolved
@nathanael-ruf
Copy link
Contributor Author

Looks like existsSync (and therefore access) returns false on an invalid symlink (see my last commit). Is this also a bug or intended behaviour?

@LiviaMedeiros
Copy link
Contributor

LiviaMedeiros commented Nov 14, 2022

Looks like existsSync (and therefore access) returns false on an invalid symlink (see my last commit). Is this also a bug or intended behaviour?

I think this is intended. In general, symlinks are supposed to be transparent, so accessing invalid symlink should be equal to accessing its destination (e.g. nonexistent file).

Testing if broken symlink still exists can be performed explicitly with lstat instead.
Alternatively, improvements to access (like optional argument { verbatimSymlinks: false }) might also be considered.

@LiviaMedeiros LiviaMedeiros added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Nov 14, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 14, 2022
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@LiviaMedeiros LiviaMedeiros added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 18, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 18, 2022
@nodejs-github-bot nodejs-github-bot merged commit 6f9175d into nodejs:main Nov 18, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 6f9175d

@LiviaMedeiros
Copy link
Contributor

Thanks for the contribution!

ruyadorno pushed a commit that referenced this pull request Nov 21, 2022
Fixes: #45404
PR-URL: #45439
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Nov 23, 2022
Fixes: nodejs#45404
PR-URL: nodejs#45439
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Nov 24, 2022
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
Fixes: #45404
PR-URL: #45439
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
Fixes: #45404
PR-URL: #45439
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
Fixes: #45404
PR-URL: #45439
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
Fixes: #45404
PR-URL: #45439
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
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.rm cannot delete a symlink which is part of a loop
7 participants