fs.Path: add a parent function that returns null when the root is reached #6746
Conversation
| /// Asserts that the path has a directory component. | ||
| pub fn parent(path: []const u8) ?[]const u8 { | ||
| const new_path = dirname(path) orelse unreachable; | ||
| if (new_path.len == path.len) return null; |
There was a problem hiding this comment.
There are a few edge cases that should be handled, such as dirname('//') = '/' that the length check doesn't detect.
There was a problem hiding this comment.
The length check does exactly check for this case to happen:
When passing in / or C:\, it has no parent component, so the path returned has the exact same length as the one passed in, in all other cases, the length will shorten and thus, a parent directory is found
There was a problem hiding this comment.
"//".len != "/".len, the check fails yet the two strings point to the same path and they're both root dirs.
There was a problem hiding this comment.
Yeah, I also think we shouldn't handle this edge case in here. Lets say someone is calling parent with //// then the first time parent returns / and then the next time it will find the root (because of dirname returning / for /). Which seems fine in my opinion.
There was a problem hiding this comment.
So you're telling me that the parent of the root is the root itself?
then the next time it will find the root
You're assuming this is repeatedly applied, nobody is stopping me from calling it just once and get the wrong result.
Rust shares my view and returns None when applied to "//".
There was a problem hiding this comment.
Hmm I see. Okay, let me see how to implement this.
There was a problem hiding this comment.
// is not equivalent to /, infact in posix, // is allowed to point to an entirely different root/tree.
However, three or more slashes (///) can indeed be simplified to /
There was a problem hiding this comment.
Alright, I stand corrected. Posix is so full of edge cases... AFAICS the double slash has no meaning on Linux/BSD but under Cygwin it's used to refer to network shares.
If we go full-posix then dirname // should return // no?
There was a problem hiding this comment.
If we go full-posix then
dirname //should return//no?
correct. and dirname /// could return /
lib/std/fs/path.zig
Outdated
| /// Returns the parant path of a directory path, or null when the root is reached. | ||
| /// Asserts that the path has a directory component. | ||
| pub fn parent(path: []const u8) ?[]const u8 { | ||
| const new_path = dirname(path) orelse unreachable; |
There was a problem hiding this comment.
Strictly speaking this should be orelse return ".". When dirname returns null you either have a file name or an empty string so the parent is indeed . (or "").
Implementing this opens the door to another class of infinite loops, returning null is fine.
|
Tests please. A simple (terminating!) loop that chops a Windows/unix path is enough. |
kubkon
added
the
standard library
|
Thanks for all the review comments! However I was just interested in fixing the infinite loop issues in the compiler and coding a perfect |
|
I think |
…ched When calling dirname in a loop, it's pretty easy to forget that you have to check for a root directory. So this adds an API that forces the check.
FireFox317
force-pushed
the
fix-infinite-loops
branch
from
f2edd59
to
7944dec
Compare
|
PR is updated regarding review. I decided to follow @LemonBoy in regards to this comment:
I also decided that for me it is out of scope to handle all the edge cases, so i added a TODO comment linking to this PR. |
This intentionally diverges from the unix dirname command, as well as Python and Node.js standard libraries, which all have this edge case return the input path, unmodified. This is a footgun, and nobody should have ever done it this way. Even the man page contradicts the behavior. It says: "strip last component from file name". Now consider, if you remove the last item from an array of length 1, then you have now an array of length 0. After you strip the last component, there should be no components remaining. Clearly, returning the input parameter unmodified in this case does not match the documented behavior. This is my justification for taking a stand on this API design. closes #6746 closes #6727 closes #6584 closes #6592 closes #6602
When calling dirname in a loop, it's pretty easy to forget that you have
to check for a root directory. So this adds an API that forces the check.
Closes #6727, #6584, #6592, #6602.