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

VfsPath::join(): treat '..' at root as valid #41

Conversation

Property404
Copy link
Contributor

In unix systems, trying to access the parent of root will return root. This commit implements that behavior.

@Property404 Property404 changed the title Path::join(): treat '..' at root as valid VfsPath::join(): treat '..' at root as valid Nov 26, 2022
@manuel-woelker
Copy link
Owner

Thanks for the PR, although I am not quite sure this would be the expected behaviour. AFAICT the parent of the root being the root is a result of making the implementation of paths simpler, by enforcing every path having a parent.

This change would cause internal inconsistencies since currently root.parent() == None.

Navigating to ".." from root is probably an error or at least not intended. I'd rather have a proper Error when resolving such paths. To better understand the use case, do you have any concrete examples where this behaviour might be useful?

@Property404
Copy link
Contributor Author

My use case is that I use VFS to emulate a unix-like system, and the current behavior causes unexpected results

Quote from Stackoverflow:
"While it may seem seem more logical to throw an error, this behavior allows all directories to be treated the same rather than having to treat the root directory as a special case."

And I think that makes sense, at least for what I'm doing. But I'm not sure what other people are using this crate for

@Property404
Copy link
Contributor Author

Added a fixup! for the VfsPath::parent() issue

Copy link
Owner

@manuel-woelker manuel-woelker left a comment

Choose a reason for hiding this comment

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

Thanks for the fixup! LGTM, just a small doc change request.

src/path.rs Show resolved Hide resolved
@Property404 Property404 force-pushed the Property404/parent-of-root-is-root branch 2 times, most recently from db17e64 to 88e41f4 Compare December 1, 2022 19:37
@Property404
Copy link
Contributor Author

Fixed doc comment and squashed/rebased

In unix systems, trying to access the parent of root will return root.
This commit implements that behavior.
@Property404 Property404 force-pushed the Property404/parent-of-root-is-root branch from 88e41f4 to 8cdf9e9 Compare December 2, 2022 03:25
@manuel-woelker manuel-woelker merged commit faa3e24 into manuel-woelker:master Dec 4, 2022
@Property404 Property404 deleted the Property404/parent-of-root-is-root branch December 18, 2022 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants