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): make find_upwards() search root #506

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shreve
Copy link

@shreve shreve commented Jul 4, 2023

The prior implementation had several implementation issues which caused an infinite loop if the search turned up empty.

This new implementation corrects the bounds check to stop searching once we've searched everywhere we can, and properly searches the root directory.

@shreve shreve changed the title Fix Path:find_upwards() fix: Path:find_upwards() Jul 4, 2023
lua/plenary/path.lua Outdated Show resolved Hide resolved
@zapling
Copy link

zapling commented Aug 30, 2023

@Conni2461 do you have any additional input on this? Would love to see this issue resolved.

@shreve shreve changed the title fix: Path:find_upwards() fix(path): find_upwards() failure case is an infinite loop Nov 30, 2023
The previous implementation of `Path:find_upwards()` used the loop
condition `folder ~= root` to bound the search which meant the root
directory would never be searched. This commit instead breaks the `while
true` loop after searching the root directory.

The function now also returns `nil` rather than a blank string to
indicate that the search has failed.

Finally, this commit adds specs for the function.
@shreve shreve closed this Nov 18, 2024
@shreve shreve reopened this Nov 18, 2024
@shreve shreve changed the title fix(path): find_upwards() failure case is an infinite loop fix(path): make find_upwards() search root Nov 18, 2024
@shreve
Copy link
Author

shreve commented Nov 18, 2024

@Conni2461 I got scooped by #562, but their implementation was still broken in a different way.

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