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

Fix Path.Finder use case when a hidden file and a regular subdirectory are in the same level #90

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

natanrolnik
Copy link

@natanrolnik natanrolnik commented Sep 5, 2023

When using Path.swift to build a tool, I noticed it had a bug related to its implementation around FileManager.DirectoryEnumerator. Here is a sample use case, which I added a test case to cover.

Imagine you have the following file tree:

.
├── .a
│   └── foo.txt
├── .foo.txt
└── b
    ├── .bar.txt
    ├── c
    │   └── bar.txt
    └── foo.txt

When running find() on root, including hidden files, one would expect to get all files and directories - and this is working fine. However, when setting .hidden(false), only the directory b is returned - and not b/c, b/c/bar.txt, nor b/foo.txt.

This happens because of these lines in the next() method in Path.Finder:

if !hidden, path.basename().hasPrefix(".") {
    enumerator.skipDescendents()
    continue
}

When finding the first hidden file or directory, skipDescendants() is called. And according to the docs, this is what it does:

Causes the receiver to skip recursion into the most recently obtained subdirectory.

This means that, when the first hidden file is found, and skipDescendants() is called, all files and directories residing alongside the hidden file are skipped.

This PR adds a check, and only calls skipDescendants() if the path in question is a directory - if it's just a file, then nothing should be skipped, as other sibling files or directories might be "waiting" to be enumerated.

Additionally, I changed the FileManager.DirectoryEnumerator initialization to use a more modern API, that can receive a list of URLResourceKeys, and passes .isDirectoryKey - this way, there's no need to do an extra disk read for each isDirectory check, as they're fetched on the enumeration stage and only read via a URL's URLResourceValues.

…y are in the same level

Also update `isDirectory` usage to be more efficient, by passing `.isDirectoryKey` to `FileManager.DirectoryEnumerator` initialization.
@mxcl
Copy link
Owner

mxcl commented Sep 9, 2023

an astounding PR. You deserve more kudos than I can give. Thank you.

Sources/Path+ls.swift Outdated Show resolved Hide resolved
@natanrolnik
Copy link
Author

@mxcl glad you liked it and learned a new API - I had no idea resourceValues were a thing on URLs either. I only knew about FileManager's contentsOfDirectory... method before, and met FileManager.DirectoryEnumerator thanks to this repo 😄

Once CI passes - may I merge it? Do you think you could publish a new release?

@mxcl
Copy link
Owner

mxcl commented Sep 12, 2023

We have to continue to support the same set of Swifts or we need to bump the major. Swift 5.0 is not that old. But I dunno, what do you think?

@mxcl
Copy link
Owner

mxcl commented Sep 12, 2023

ugh, CI needs updates because github retires images bah

@natanrolnik
Copy link
Author

@mxcl is there anything I can do to help here?

@mxcl
Copy link
Owner

mxcl commented Oct 11, 2023

CI/CD needs to pass. If you look at my contribution graph you can already see I basically only work on open source 7 days a week. CI/CD shit is hours and hours of tedious busywork.

We need it to pass or we need to bump the major version.

@natanrolnik
Copy link
Author

I know I know, I can deal with it. For some reason I forgot that CI could be changed in this PR itself.
What Swift version could be set as the minimum? I would suggest 5.5 + bumping the package major version.

@mxcl
Copy link
Owner

mxcl commented Oct 11, 2023

I know I know, I can deal with it.

like I don't feel that is fair either. But I really have no time to mess with this :(

What Swift version could be set as the minimum? I would suggest 5.5 + bumping the package major version.

Ideally we don’t bump, we promise to work with the version matrix we test against. However if it cannot be helped I can make a judgement call. I get that very few people use these old Swifts anymore, but that is the promise we make with semantic versioning.

Like my new project pkgx should be able to provide swift versions all the way back. I don't think we do yet tho.

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