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.

Sign up for GitHub

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

Jump to bottom

fs: allow passing negative zero fd #37123

Merged
merged 1 commit into from
Feb 6, 2021
Merged

fs: allow passing negative zero fd #37123

merged 1 commit into from
Feb 6, 2021

Conversation

RaisinTen
Copy link
Contributor

Fixes: #37122

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Jan 29, 2021
@nodejs-github-bot
Copy link
Collaborator

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

aduh95
aduh95 approved these changes Jan 29, 2021
View reviewed changes
test/parallel/test-fs-stat.js Outdated Show resolved Hide resolved
@RaisinTen
Copy link
Contributor Author

@aduh95 do you know why the mac os run got cancelled here: eac8f2f?

@aduh95
Copy link
Contributor

aduh95 commented Jan 29, 2021

@aduh95 do you know why the mac os run got cancelled here: eac8f2f?

I canceled it – when a lot of changes are being pushed at the same time to the repo, we are hitting GH Actions limitations and sometimes it takes hours for all the CI to complete. So I try to cancel jobs on commits which has more recents commits on the same PR as we don't need those anyway.

@nodejs-github-bot
Copy link
Collaborator

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

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 29, 2021
mscdex
mscdex reviewed Jan 29, 2021
View reviewed changes
lib/internal/fs/utils.js Outdated Show resolved Hide resolved
@aduh95 aduh95 mentioned this pull request Jan 29, 2021
Closed
juanarbol
juanarbol previously requested changes Jan 30, 2021
View reviewed changes
lib/internal/fs/utils.js Outdated Show resolved Hide resolved
@aduh95 aduh95 removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 30, 2021
@nodejs-github-bot
Copy link
Collaborator

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

@aduh95 aduh95 dismissed juanarbol’s stale review February 1, 2021 22:10

He confirmed his suggestion is not a blocker in a comment above.

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 1, 2021
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@RaisinTen
Copy link
Contributor Author

Rebased to resolve the conflicts. Restarting the CI.

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@RaisinTen @Trott
fs: allow passing negative zero fd
6b8b165 
Fixes: nodejs#37122

PR-URL: nodejs#37123
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@Trott Trott force-pushed the fs/fix-negative-zero-for-fd branch from 016550c to 6b8b165 Compare February 6, 2021 13:10
@Trott Trott merged commit 6b8b165 into nodejs:master Feb 6, 2021
@Trott
Copy link
Member

Trott commented Feb 6, 2021

Landed in 6b8b165

@RaisinTen RaisinTen deleted the fs/fix-negative-zero-for-fd branch February 6, 2021 13:37
danielleadams pushed a commit that referenced this pull request Feb 16, 2021
@RaisinTen @danielleadams
fs: allow passing negative zero fd
c302450 
Fixes: #37122

PR-URL: #37123
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
This was referenced Feb 16, 2021
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juanarbol juanarbol juanarbol left review comments

@aduh95 aduh95 aduh95 approved these changes

@mscdex mscdex mscdex left review comments

@Lxxyx Lxxyx Lxxyx approved these changes

@Trott Trott Trott 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. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

fs: error when fd = -0
8 participants
@RaisinTen @nodejs-github-bot @aduh95 @Trott @mscdex @Lxxyx @juanarbol @targos