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

Jump to bottom

fs: fix length option being ignored during read() #40906

Merged
merged 3 commits into from Dec 10, 2021
Merged

fs: fix length option being ignored during read() #40906

merged 3 commits into from Dec 10, 2021

Conversation

fracsinus
Copy link
Contributor

Currently, calling read() with an options object always uses buffer.byteLength, ignoring length in the object.

First time opening a PR here, please let me know if anything doesn't conform to the guidelines.

@fracsinus
fs: fix length option being ignored during read()
4948c56 
Currently, `length` in an options object is ignored.
@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 21, 2021
ronag
ronag reviewed Nov 21, 2021
View reviewed changes
lib/internal/fs/promises.js Outdated Show resolved Hide resolved
ronag
ronag requested changes Nov 21, 2021
View reviewed changes
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

Needs a test

@aduh95
Copy link
Contributor

aduh95 commented Nov 21, 2021

Thanks for sending this PR, this looks indeed like an oversight.

Can you please add test cases in test/parallel/test-fs-promises-file-handle-read.js where the length argument is different from buffer.byteLength, and verify that the number of read bytes is as expected?

@fracsinus @ronag
fs: use nullish coalescing to respect zero-length reads
8fdb45b 
Co-authored-by: Robert Nagy <ronagy@icloud.com>
@ronag ronag 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. and removed needs-ci PRs that need a full CI run. labels Nov 22, 2021
@ronag
Copy link
Member

ronag commented Nov 22, 2021

@nodejs/fs

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 22, 2021
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@ronag ronag requested a review from aduh95 November 22, 2021 10:22
aduh95
aduh95 reviewed Nov 22, 2021
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Can you please add a test case where length is 0?

test/parallel/test-fs-promises-file-handle-read.js Outdated Show resolved Hide resolved
test/parallel/test-fs-promises-file-handle-read.js Outdated Show resolved Hide resolved
@aduh95 aduh95 added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Nov 22, 2021
@github-actions github-actions bot mentioned this pull request Nov 23, 2021
Open
45 tasks
@fracsinus @aduh95
fs: add test for zero-length read
3d49687 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@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 23, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 23, 2021
@nodejs-github-bot

This comment has been minimized.

Sign in to view
This was referenced Nov 24, 2021
Open
Open
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Nov 26, 2021
Open
33 tasks
@github-actions github-actions bot mentioned this pull request Nov 27, 2021
Open
27 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 10, 2021
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 10, 2021
@nodejs-github-bot nodejs-github-bot merged commit 10493b4 into nodejs:master Dec 10, 2021
@nodejs-github-bot
Copy link
Collaborator

Landed in 10493b4

danielleadams pushed a commit that referenced this pull request Dec 13, 2021
@fracsinus @danielleadams
fs: fix length option being ignored during read()
7a8890a 
Currently, `length` in an options object is ignored.

PR-URL: #40906
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this pull request Dec 14, 2021
@fracsinus @danielleadams
fs: fix length option being ignored during read()
e25671c 
Currently, `length` in an options object is ignored.

PR-URL: #40906
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@danielleadams danielleadams mentioned this pull request Dec 14, 2021
Merged
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
@fracsinus @danielleadams
fs: fix length option being ignored during read()
6a98aa3 
Currently, `length` in an options object is ignored.

PR-URL: #40906
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
@fracsinus @danielleadams
fs: fix length option being ignored during read()
8a790a9 
Currently, `length` in an options object is ignored.

PR-URL: #40906
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
@fracsinus @Linkgoron
fs: fix length option being ignored during read()
059a0c5 
Currently, `length` in an options object is ignored.

PR-URL: nodejs#40906
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
@fracsinus @danielleadams
fs: fix length option being ignored during read()
67c6170 
Currently, `length` in an options object is ignored.

PR-URL: #40906
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@ronag ronag ronag 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. 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@fracsinus @aduh95 @ronag @nodejs-github-bot @lpinca @targos