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

Implement copy_file_range() #1069

Merged
merged 1 commit into from
Jun 16, 2019
Merged

Implement copy_file_range() #1069

merged 1 commit into from
Jun 16, 2019

Conversation

ArniDagur
Copy link

This should fix the problems with #971 and #1008.

@ArniDagur ArniDagur mentioned this pull request May 26, 2019
Closed
asomers
asomers requested changes May 26, 2019
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
src/fcntl.rs Outdated Show resolved Hide resolved
src/fcntl.rs Outdated Show resolved Hide resolved
src/fcntl.rs Outdated Show resolved Hide resolved
src/fcntl.rs Outdated Show resolved Hide resolved
src/fcntl.rs Outdated Show resolved Hide resolved
src/fcntl.rs Outdated Show resolved Hide resolved
test/test_fcntl.rs Outdated Show resolved Hide resolved
@asomers
Copy link
Member

asomers commented Jun 7, 2019

You'll need to rebase and fix the CHANGELOG entry. Also, please wrap everything to 80 columns.

asomers
asomers requested changes Jun 11, 2019
View reviewed changes
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

When I said "wrap everything to 80 columns" I meant to only wrap new code, not existing code. Now you have a big change that mixes style with content. That's not good. Please undo it. If you want to wrap the rest of the file, that should be done as a separate commit.

CHANGELOG.md Outdated Show resolved Hide resolved
@ArniDagur
Copy link
Author

This should be fine, right?

asomers
asomers reviewed Jun 11, 2019
View reviewed changes
test/test_fcntl.rs Outdated Show resolved Hide resolved
asomers
asomers approved these changes Jun 12, 2019
View reviewed changes
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

It all LGTM now, but you need to rebase to fix the CHANGELOG.

@ArniDagur
Copy link
Author

I have rebased and fixed the changelog.

asomers
asomers approved these changes Jun 16, 2019
View reviewed changes
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

bors r+

bors bot added a commit that referenced this pull request Jun 16, 2019
@bors
Merge #1069
582846e 
1069: Implement copy_file_range() r=asomers a=ArniDagur

This should fix the problems with #971 and #1008.

Co-authored-by: Árni Dagur <arnidg@protonmail.ch>
@bors
Copy link
Contributor

bors bot commented Jun 16, 2019

Build succeeded

@bors bors bot merged commit 0f19012 into nix-rust:master Jun 16, 2019
@asomers asomers mentioned this pull request Jun 23, 2019
Closed
@ArniDagur
Copy link
Author

@asomers It seems that I made a mistake in this PR: The function should be in unistd.h, not fcntl.h. Perhaps this should be fixed in the next major version?

@asomers
Copy link
Member

asomers commented Jan 11, 2020

Honestly I wouldn't bother. The user disruption that would come by renaming the symbol isn't worth the gain to consistency.

@dubiousjim
Copy link

Why not export it in the correct place, and re-export it in nix::fcntl with a comment in the docs to say this export will eventually go away.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asomers asomers asomers approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ArniDagur @asomers @dubiousjim