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

Rename FilenameTooLong to InvalidFilename and also use it for Windows' ERROR_INVALID_NAME #90955

Merged
merged 5 commits into from
Feb 12, 2022
Merged

Rename FilenameTooLong to InvalidFilename and also use it for Windows' ERROR_INVALID_NAME #90955

merged 5 commits into from
Feb 12, 2022

Conversation

JohnTitor
Copy link
Member

Address #90940 (comment)
ERROR_INVALID_NAME (i.e. "The filename, directory name, or volume label syntax is incorrect") happens if we pass an invalid filename, directory name, or label syntax, so mapping as InvalidInput is reasonable to me.

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 16, 2021
@JohnTitor JohnTitor mentioned this pull request Nov 16, 2021
Closed
@m-ou-se m-ou-se added O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Nov 17, 2021
@m-ou-se m-ou-se assigned m-ou-se and unassigned kennytm Nov 17, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Nov 17, 2021

Seems reasonable to me. Since the ErrorKind mapping is something we keep stable, let's quickly get some more eyes on it before merging it:

@rfcbot merge

@kennytm

This comment has been minimized.

Sign in to view
@rfcbot
Copy link

rfcbot commented Nov 17, 2021 •
edited
Loading

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

Concerns:

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. labels Nov 17, 2021
@ChrisDenton
Copy link
Member

One issue I've found while testing symlinks is that a symlink to an invalid file name can return ERROR_INVALID_NAME. This is still technically an invalid input to the function that receives the resolved file name, but from the user's perspective their input might be perfectly valid.

So now I'm uncertain whether InvalidInput is the right mapping or not.

@m-ou-se
Copy link
Member

m-ou-se commented Nov 24, 2021

@rfcbot concern broken symlinks

See #90955 (comment)

@joshtriplett
Copy link
Member

Might it make sense to change FilenameTooLong (currently nightly only) to FilenameInvalid and map this to that?

@JohnTitor
Copy link
Member Author

Might it make sense to change FilenameTooLong (currently nightly only) to FilenameInvalid and map this to that?

Sounds good, I'm happy to switch to it if y'all agree with the nightly change.

@joshtriplett
Copy link
Member

@rfcbot concern should-map-to-FilenameInvalid-renamed-from-FilenameTooLong

@ChrisDenton
Copy link
Member

Might it make sense to change FilenameTooLong (currently nightly only) to FilenameInvalid and map this to that?

Yes, I think this is the best idea.

@joshtriplett
Copy link
Member

@JohnTitor Could you please update the PR to do that? (Let us know if you don't have the bandwidth to do so.)

@JohnTitor JohnTitor force-pushed the os-error-123-as-invalid-input branch from 5bf5ce9 to 1bc408d Compare January 31, 2022 08:27
@JohnTitor
Copy link
Member Author

Sure! Updated as suggested in #90955 (comment).

@joshtriplett
Copy link
Member

@rfcbot resolved should-map-to-FilenameInvalid-renamed-from-FilenameTooLong

@joshtriplett
Copy link
Member

@m-ou-se I think this change should resolve your concern as well.

@m-ou-se m-ou-se changed the title windows: Map ERROR_INVALID_NAME as InvalidInput Rename FilenameTooLong to FilenameInvalid and also use it for Windows' ERROR_INVALID_NAME Jan 31, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Jan 31, 2022

@rfcbot resolve broken symlinks

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Jan 31, 2022
@rfcbot
Copy link

rfcbot commented Jan 31, 2022

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

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jan 31, 2022
@rfcbot
Copy link

rfcbot commented Feb 10, 2022

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.

@JohnTitor JohnTitor changed the title Rename FilenameTooLong to FilenameInvalid and also use it for Windows' ERROR_INVALID_NAME Rename FilenameTooLong to InvalidFilename and also use it for Windows' ERROR_INVALID_NAME Feb 10, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Feb 10, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Feb 11, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Feb 11, 2022

@m-ou-se: 🔑 Insufficient privileges: Not in reviewers

@m-ou-se
Copy link
Member

m-ou-se commented Feb 11, 2022

Uh. @rust-lang/infra ^

@m-ou-se
Copy link
Member

m-ou-se commented Feb 11, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Feb 11, 2022

📌 Commit a898b31 has been approved by m-ou-se

@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 Feb 11, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 11, 2022
@matthiaskrgr
Rollup merge of rust-lang#90955 - JohnTitor:os-error-123-as-invalid-i…
5ccee0e 
…nput, r=m-ou-se

Rename `FilenameTooLong` to `InvalidFilename` and also use it for Windows' `ERROR_INVALID_NAME`

Address rust-lang#90940 (comment)
`ERROR_INVALID_NAME` (i.e. "The filename, directory name, or volume label syntax is incorrect") happens if we pass an invalid filename, directory name, or label syntax, so mapping as `InvalidInput` is reasonable to me.
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 11, 2022
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 11, 2022
@matthiaskrgr
Rollup merge of rust-lang#90955 - JohnTitor:os-error-123-as-invalid-i…
8a9ccbf 
…nput, r=m-ou-se

Rename `FilenameTooLong` to `InvalidFilename` and also use it for Windows' `ERROR_INVALID_NAME`

Address rust-lang#90940 (comment)
`ERROR_INVALID_NAME` (i.e. "The filename, directory name, or volume label syntax is incorrect") happens if we pass an invalid filename, directory name, or label syntax, so mapping as `InvalidInput` is reasonable to me.
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 11, 2022
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 11, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 11, 2022
@bors
Auto merge of rust-lang#93921 - matthiaskrgr:rollup-wn3jlxj, r=matthi…
f198510 
…askrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#90955 (Rename `FilenameTooLong` to `InvalidFilename` and also use it for Windows' `ERROR_INVALID_NAME`)
 - rust-lang#91607 (Make `span_extend_to_prev_str()` more robust)
 - rust-lang#92895 (Remove some unused functionality)
 - rust-lang#93635 (Add missing platform-specific information on current_dir and set_current_dir)
 - rust-lang#93660 (rustdoc-json: Add some tests for typealias item)
 - rust-lang#93782 (Split `pauth` target feature)
 - rust-lang#93868 (Fix incorrect register conflict detection in asm!)
 - rust-lang#93888 (Implement `AsFd` for `&T` and `&mut T`.)
 - rust-lang#93909 (Fix typo: explicitely -> explicitly)
 - rust-lang#93910 (fix mention of moved function in `rustc_hir` docs)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ce4df92 into rust-lang:master Feb 12, 2022
@rustbot rustbot added this to the 1.60.0 milestone Feb 12, 2022
@JohnTitor JohnTitor deleted the os-error-123-as-invalid-input branch February 12, 2022 07:45
@tavianator tavianator mentioned this pull request Mar 9, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m-ou-se m-ou-se m-ou-se left review comments

Assignees

@m-ou-se m-ou-se

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. O-windows Operating system: Windows 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
Milestone
1.60.0
Development

Successfully merging this pull request may close these issues.