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 promises reads with pos > 4GB #21148

Merged
merged 1 commit into from
Jun 7, 2018
Merged

fs: fix promises reads with pos > 4GB #21148

merged 1 commit into from
Jun 7, 2018

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jun 5, 2018

This commit fixes fs.promises based reads with a position greater than 4GB. This ports the changes from #21003 to the fs.promises module.

Refs: #21003
Fixes: #21121

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Jun 5, 2018
@joyeecheung
Copy link
Member

@trivikr trivikr added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 6, 2018
PR-URL: nodejs#21148
Fixes: nodejs#21121
Refs: nodejs#21003
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
@cjihrig cjihrig merged commit 5012587 into nodejs:master Jun 7, 2018
@cjihrig cjihrig deleted the fs branch June 7, 2018 13:26
@ryzokuken
Copy link
Contributor

@cjihrig neat! Is this an interface change in GitHub or did you use the interface for landing this?

@ryzokuken
Copy link
Contributor

For consistency: Landed in 5012587 🎉

@cjihrig
Copy link
Contributor Author

cjihrig commented Jun 7, 2018

@ryzokuken I'm not exactly sure what you mean. I landed it from the command line though.

targos pushed a commit that referenced this pull request Jun 7, 2018
PR-URL: #21148
Fixes: #21121
Refs: #21003
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
@ryzokuken
Copy link
Contributor

@cjihrig that's what I wanted to ask, sorry. If you landed this in the standard NCU way, then it's a welcome interface change by GitHub that you didn't have to close this manually and place a comment, it identified that and tagged it as "merged".

@targos
Copy link
Member

targos commented Jun 7, 2018

@ryzokuken There is one more step required for this to work. Using ncu, it would look like this:

...
git node land --final
git push --force origin HEAD:pr-branch
git push upstream master

@ChALkeR
Copy link
Member

ChALkeR commented Jun 7, 2018

@ryzokuken You can do that manually, GitHub marks commits as merged when the top commit of the PR gets included in the target branch. If you rebase, then push first to the PR branch and then to master — GitHub labels PR as merged.

@joyeecheung
Copy link
Member

If we track the pr branch in ncu then ncu can prompt the command as well (a suggestion with a placeholder for PR branch name also works although).

@ryzokuken
Copy link
Contributor

Cool, I never knew. Thanks for sharing.

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

Successfully merging this pull request may close these issues.

fsPromise - read files from positions >4GB doesn't work
9 participants