Add fs.path.ComponentIterator and use it in Dir.makePath
#16570
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
matu3ba
reviewed
Jul 26, 2023
| break :posix root_end_index; | ||
| }, | ||
| .windows => windows: { | ||
| // Namespaces other than the Win32 file namespace are tricky |
There was a problem hiding this comment.
I think a shorter counter-example would be more readable, but I dont have too much of an opinion on this.
Before this commit, there were three issues with the makePath implementation: 1. The component iteration did not 'collapse' consecutive path separators; instead, it would treat `a/b//c` as `a/b//c`, `a/b/`, `a/b`, and `a`. 2. Trailing path separators led to redundant `makeDir` calls, e.g. with the path `a/b/` (if `a` doesn't exist), it would try to create `a/b/`, then try `a/b`, then try `a`, then try `a/b`, and finally try `a/b/` again. 3. The iteration did not treat the root of a path specially, so on Windows it could attempt to make a directory with a path like `X:` for something like `X:\a\b\c` if the `X:\` drive doesn't exist. This didn't lead to any problems that I could find, but there's no reason to try to make a drive letter as a directory (or any other root path). This commit fixes all three issues by introducing a ComponentIterator that is root-aware and handles both sequential path separators and trailing path separators and uses it in `Dir.makePath`. This reduces the number of `makeDir` calls for paths where (1) the root of the path doesn't exist, (2) there are consecutive path separators, or (3) there are trailing path separators As an example, here are the makeDir calls that would have been made before this commit when calling `makePath` for a relative path like `a/b//c//` (where the full path needs to be created): a/b//c// a/b//c/ a/b//c a/b/ a/b a a/b a/b/ a/b//c a/b//c/ a/b//c// And after this commit: a/b//c a/b a a/b a/b//c
squeek502
force-pushed
the
path-component-iterator
branch
from
3812828
to
72c7524
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before this commit, there were three issues with the
makePathimplementation:a/b//casa/b//c,a/b/,a/b, anda.makeDircalls, e.g. with the patha/b/(ifadoesn't exist), it would try to createa/b/, then trya/b, then trya, then trya/b, and finally trya/b/again.X:for something likeX:\a\b\cif theX:\drive doesn't exist. This didn't lead to any problems that I could find, but there's no reason to try to make a drive letter as a directory (or any other root path).This commit fixes all three issues by introducing a ComponentIterator that is root-aware and handles both sequential path separators and trailing path separators and uses it in
Dir.makePath. This reduces the number ofmakeDircalls for paths where (1) the root of the path doesn't exist, (2) there are consecutive path separators, or (3) there are trailing path separatorsAs an example, here are the makeDir calls that would have been made before this commit when calling
makePathfor a relative path likea/b//c//(where the full path needs to be created):a/b//c//
a/b//c/
a/b//c
a/b/
a/b
a
a/b
a/b/
a/b//c
a/b//c/
a/b//c//
And after this commit:
a/b//c
a/b
a
a/b
a/b//c
Tangentially related to #14833, but only in the sense that
ComponentIteratorcould be used there as well.