GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fs: filehandle.read(buffer) can't read file when options are omitted by pulkit-30 · Pull Request #51087 · nodejs/node

Conversation

@pulkit-30

@pulkit-30 pulkit-30 commented Dec 7, 2023

Copy link
Copy Markdown
Contributor

fix: #47183

  • make length arg in fh.read(buffer, offset, position, length) default to length ?? buffer.byteLength - offset

Code

import fs from "fs/promises";

async function test() {
  let path = './ex.txt';

  let buf = Buffer.alloc(4);
  let handle;
  try {
    handle = await fs.open(path);
    {
      let buf = Buffer.alloc(4);
      let { bytesRead, buffer } = await handle.read(buf);
      console.log(bytesRead);
      console.log(buffer);
      console.log(buf);
    }
  } finally {
    await handle?.close();
  }
}
test();

before

0
<Buffer 00 00 00 00>
<Buffer 00 00 00 00>

now

4
<Buffer 48 65 6c 6c>
<Buffer 48 65 6c 6c>

That's my second attempt at this issue 😅

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss and H1Gdev reacted with thumbs up emoji bricss reacted with eyes emoji
All reactions
  • 👍 2 reactions
  • 👀 1 reaction
pulkit-30 added 2 commits December 7, 2023 22:16
@pulkit-30
fs: make offset, position & length args in fh.read() optional
f2f8e63
@pulkit-30
fix test
35ee64e
@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to file-system APIs and the fs module. needs-ci PRs that need a full CI run. labels Dec 7, 2023
bricss
bricss reviewed Dec 7, 2023
View reviewed changes
Comment thread lib/internal/fs/promises.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

@pulkit-30
fix suggestions
1de21b8
@pulkit-30
pulkit-30 force-pushed the fix-47183 branch from ca3883a to 1de21b8 Compare December 7, 2023 19:08
@pulkit-30
pulkit-30 requested a review from bricss December 8, 2023 18:27
benjamingr
benjamingr approved these changes Dec 10, 2023
View reviewed changes
H4ad
H4ad approved these changes Dec 12, 2023
View reviewed changes
@H4ad H4ad added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Dec 12, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 12, 2023
@nodejs-github-bot

nodejs-github-bot commented Dec 12, 2023

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

BridgeAR
BridgeAR approved these changes Dec 12, 2023
View reviewed changes

@BridgeAR BridgeAR left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM (while we could remove the length assignment in line 651 and 662)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
lpinca
lpinca approved these changes Dec 13, 2023
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented Dec 13, 2023

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bricss bricss mentioned this pull request Dec 17, 2023
v21.5.0 proposal #51166
Merged
@nodejs-github-bot

nodejs-github-bot commented Dec 19, 2023

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jasnell
jasnell approved these changes Dec 22, 2023
View reviewed changes
@jasnell jasnell added the commit-queue PRs queued for automated landing through the Commit Queue. label Dec 22, 2023
@nodejs-github-bot nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Dec 22, 2023
@nodejs-github-bot

nodejs-github-bot commented Dec 22, 2023

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/51087
✔  Done loading data for nodejs/node/pull/51087
----------------------------------- PR info ------------------------------------
Title      fs: filehandle.read(buffer) can't read file when options are omitted (#51087)
Author     Pulkit Gupta  (@pulkit-30)
Branch     pulkit-30:fix-47183 -> nodejs:main
Labels     fs, author ready, needs-ci
Commits    3
 - fs: make offset, position & length args in fh.read() optional
 - fix test
 - fix suggestions
Committers 1
 - pulkit-30 
PR-URL: https://github.com/nodejs/node/pull/51087
Fixes: https://github.com/nodejs/node/issues/47183
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Vinícius Lourenço Claro Cardoso 
Reviewed-By: Ruben Bridgewater 
Reviewed-By: Luigi Pinca 
Reviewed-By: James M Snell 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/51087
Fixes: https://github.com/nodejs/node/issues/47183
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Vinícius Lourenço Claro Cardoso 
Reviewed-By: Ruben Bridgewater 
Reviewed-By: Luigi Pinca 
Reviewed-By: James M Snell 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Thu, 07 Dec 2023 16:55:28 GMT
   ✔  Approvals: 5
   ✔  - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/51087#pullrequestreview-1774088173
   ✔  - Vinícius Lourenço Claro Cardoso (@H4ad): https://github.com/nodejs/node/pull/51087#pullrequestreview-1777399313
   ✔  - Ruben Bridgewater (@BridgeAR) (TSC): https://github.com/nodejs/node/pull/51087#pullrequestreview-1777519295
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/51087#pullrequestreview-1780448292
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/51087#pullrequestreview-1793960251
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-12-19T17:04:36Z: https://ci.nodejs.org/job/node-test-pull-request/56391/
- Querying data for job/node-test-pull-request/56391/
   ✔  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 51087
From https://github.com/nodejs/node
 * branch                  refs/pull/51087/merge -> FETCH_HEAD
✔  Fetched commits as 4f3261f10183..1de21b873667
--------------------------------------------------------------------------------
Auto-merging doc/api/fs.md
[main fd0e346463] fs: make offset, position & length args in fh.read() optional
 Author: pulkit-30 
 Date: Thu Dec 7 22:16:46 2023 +0530
 3 files changed, 23 insertions(+), 3 deletions(-)
[main a0f036d617] fix test
 Author: pulkit-30 
 Date: Thu Dec 7 22:17:44 2023 +0530
 1 file changed, 1 insertion(+), 1 deletion(-)
[main dfb1cf7171] fix suggestions
 Author: pulkit-30 
 Date: Fri Dec 8 00:38:07 2023 +0530
 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: make offset, position & length args in fh.read() optional

PR-URL: #51087
Fixes: #47183
Reviewed-By: Benjamin Gruenbaum [email protected]
Reviewed-By: Vinícius Lourenço Claro Cardoso [email protected]
Reviewed-By: Ruben Bridgewater [email protected]
Reviewed-By: Luigi Pinca [email protected]
Reviewed-By: James M Snell [email protected]

[detached HEAD 7932ec3bda] fs: make offset, position & length args in fh.read() optional
Author: pulkit-30 [email protected]
Date: Thu Dec 7 22:16:46 2023 +0530
3 files changed, 23 insertions(+), 3 deletions(-)
Rebasing (3/6)
Rebasing (4/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
fix test

PR-URL: #51087
Fixes: #47183
Reviewed-By: Benjamin Gruenbaum [email protected]
Reviewed-By: Vinícius Lourenço Claro Cardoso [email protected]
Reviewed-By: Ruben Bridgewater [email protected]
Reviewed-By: Luigi Pinca [email protected]
Reviewed-By: James M Snell [email protected]

[detached HEAD 224a463637] fix test
Author: pulkit-30 [email protected]
Date: Thu Dec 7 22:17:44 2023 +0530
1 file changed, 1 insertion(+), 1 deletion(-)
Rebasing (5/6)
Rebasing (6/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
fix suggestions

PR-URL: #51087
Fixes: #47183
Reviewed-By: Benjamin Gruenbaum [email protected]
Reviewed-By: Vinícius Lourenço Claro Cardoso [email protected]
Reviewed-By: Ruben Bridgewater [email protected]
Reviewed-By: Luigi Pinca [email protected]
Reviewed-By: James M Snell [email protected]

[detached HEAD c191ac4bbb] fix suggestions
Author: pulkit-30 [email protected]
Date: Fri Dec 8 00:38:07 2023 +0530
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/7296297391

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@H4ad H4ad added commit-queue PRs queued for automated landing through the Commit Queue. commit-queue-squash PRs the Commit Queue should land as one squashed commit. and removed commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. labels Dec 22, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Dec 22, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 48cdb88 into nodejs:main Dec 22, 2023
@nodejs-github-bot

nodejs-github-bot commented Dec 22, 2023

Copy link
Copy Markdown
Collaborator

Landed in 48cdb88

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

RafaelGSS pushed a commit that referenced this pull request Jan 2, 2024
@pulkit-30 @RafaelGSS
fs: make offset, position & length args in fh.read() optional
e5a8fa0
PR-URL: #51087
Fixes: #47183
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Jan 2, 2024
v21.6.0 proposal #51342
Merged
@markrian markrian mentioned this pull request Jan 2, 2024
filehandle.read(buffer) can't read file when options are omitted #47183
Closed
richardlau pushed a commit that referenced this pull request Mar 25, 2024
@pulkit-30 @richardlau
fs: make offset, position & length args in fh.read() optional
a8fd01a
PR-URL: #51087
Fixes: #47183
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@richardlau richardlau mentioned this pull request Mar 25, 2024
v20.12.0 proposal #52212
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@benjamingr benjamingr benjamingr approved these changes
@lpinca lpinca lpinca approved these changes
@BridgeAR BridgeAR BridgeAR approved these changes
@H4ad H4ad H4ad approved these changes
@bricss bricss Awaiting requested review from bricss

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue-squash PRs the Commit Queue should land as one squashed commit. fs Issues and PRs related to file-system APIs and the fs module. 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.

filehandle.read(buffer) can't read file when options are omitted

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@pulkit-30 @nodejs-github-bot @jasnell @benjamingr @lpinca @bricss @BridgeAR @H4ad
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.