Skip to content

Add File already exists error doc to hard_link function #135415

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

Merged
merged 1 commit into from
Jan 25, 2025

Conversation

Harshit933
Copy link
Contributor

@Harshit933 Harshit933 commented Jan 12, 2025

Description

If the link path already exists, the error AlreadyExists is returned. This commit adds this error to the docs.

I tested it with the current rust master version, this error was returned when there is already a link for the file is present.
This was the error returned:

[harshit:../Desktop/rust_compiler_testing/hard_link (master|…5)] cargo +stage1 run
   Compiling hard_link v0.1.0 (/home/harshit/Desktop/rust_compiler_testing/hard_link)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.12s
     Running `target/debug/hard_link`
Err(Os { code: 17, kind: AlreadyExists, message: "File exists" })

This is my first PR on rust, any suggestions on which issue I can take next are most welcome 😄

Fixes #130117

@rustbot
Copy link
Collaborator

rustbot commented Jan 12, 2025

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ChrisDenton (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 12, 2025
@ChrisDenton
Copy link
Member

I do agree with this PR because I think this is always the intended behaviour and not documenting it was just an oversight. However, I would caution about documenting based on testing alone as that can miss edge cases or other platforms. cc @the8472 in case I'm missing something.

I think this would need a sign-off by someone on libs-api as it's technically a new API guarantee. But I think this should be an easy decision. cc @rust-lang/libs-api

@the8472
Copy link
Member

the8472 commented Jan 13, 2025

Yeah posix requires that it fails if the destination exists. So that only leaves WASI, which to my understanding does try to match what's portable and secure so it probably should have this property too but I couldn't find documentation on that. CC @sunfishcode

@ChrisDenton ChrisDenton added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 13, 2025
@Harshit933
Copy link
Contributor Author

However, I would caution about documenting based on testing alone as that can miss edge cases or other platforms

Hello ,I agree that behavior can vary across platforms and may sometimes differ from what is expected. Out of curiosity is there any way to know (other than testing on different OS) the anomaly of this function. I looked up the code it internally calls the system functions namely link and linkat based on the OS.

@ChrisDenton ChrisDenton added the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Jan 13, 2025
@ChrisDenton
Copy link
Member

ChrisDenton commented Jan 13, 2025

The documentation for hard_link says which functions it currently uses on each platform. You can look up each platform documentation for the corresponding function.

The reason I pinged our libs-api team is because documenting a guarantee is a bit of a stronger decision than just "this is what happens now". It's a guarantee that this is the behaviour we expect from all platforms, now and in the future. I personally think this is a good guarantee to have but it'll need libs-api to agree.

@Harshit933
Copy link
Contributor Author

Got it. Thank you so much for the explanation.

@Amanieu
Copy link
Member

Amanieu commented Jan 14, 2025

@rfcbot merge

@rfcbot
Copy link
Collaborator

rfcbot commented Jan 14, 2025

Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jan 14, 2025
@rfcbot
Copy link
Collaborator

rfcbot commented Jan 14, 2025

🔔 This is now entering its final comment period, as per the review above. 🔔

@tbu-
Copy link
Contributor

tbu- commented Jan 15, 2025

Has anyone tested this on Windows? I can't find anything concerning the second path already existing in the docs of CreateHardLinkW.

@sunfishcode
Copy link
Member

Yeah posix requires that it fails if the destination exists. So that only leaves WASI, which to my understanding does try to match what's portable and secure so it probably should have this property too but I couldn't find documentation on that. CC @sunfishcode

Indeed, I've now opened WebAssembly/wasi-filesystem#165 to add documentation for that.

@ChrisDenton
Copy link
Member

Has anyone tested this on Windows?

Yes. It'll return ERROR_ALREADY_EXISTS if the file already exists. Internally it uses NtSetInformationFile with FILE_LINK_INFORMATION and sets ReplaceIfExists to false.

I can't find anything concerning the second path already existing in the docs of CreateHardLinkW.

I mean, it's not as explicit as I'd like but it is meant to be implied by calling it a "new file" and not mentioning replacing the old file.

@Amanieu Amanieu removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Jan 21, 2025
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jan 24, 2025
@rfcbot
Copy link
Collaborator

rfcbot commented Jan 24, 2025

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

If the link path already exists, the error `AlreadyExists`
is returned. This commit adds this error to the docs.
@ChrisDenton
Copy link
Member

Thanks!

@bors r+

@bors
Copy link
Collaborator

bors commented Jan 24, 2025

📌 Commit ab27463 has been approved by ChrisDenton

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 24, 2025
@bors bors merged commit 0741cc0 into rust-lang:master Jan 25, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 25, 2025
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify behavior of std::fs::hard_link when second path already exists.