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

[Search Directory] Use fd or fdfind unaliased to resolve apparent hang #282

Merged
merged 10 commits into from
Jan 15, 2023

Conversation

Susensio
Copy link
Contributor

@Susensio Susensio commented Dec 9, 2022

A cursory search through GitHub code for "alias fd fdfind" reveals hundreds of pieces of code that aliases fdfind to fd instead of using symlinks (in spite of fd's instructions to symlink instead). And that's just the code that's public! The problem with this is that fish functions buffer their output, so when fd is aliased, Search Directory does not pop open fzf until fd is done outputting files. If Search Directory is triggered in $HOME or some other large directory, then Search Directory appears to hang unresponsively.

We fix this by using the fd binary directly. Since presumably many of fzf.fish users on certain Linux distros have it installed as fdfind, we look for either binaries and use it. Resolves #281.

@@ -1,4 +1,7 @@
function _fzf_search_directory --description "Search the current directory. Replace the current token with the selected file paths."
# On Ubuntu and other Debian-based Linux distributions, fd binary is called fdfind.
Copy link
Owner

Choose a reason for hiding this comment

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

*fd binary is installed as

@PatrickF1
Copy link
Owner

The test "~/ is expanded to HOME" failed because the test works by capturing the fd arguments using a wrapper function, but now that we are using command, the wrapper function is bypassed. We'll need to rewrite the test. I'd test it now by creating a directory with an certain number of files in it, scope the search to that directory, select all, and test that the number of files outputted = number of files in that directory.

@PatrickF1 PatrickF1 changed the title [Search Directory] Look up fd binary or fdfind [Search Directory] Use fd or fdfind unaliased Jan 15, 2023
@PatrickF1 PatrickF1 merged commit 6f50cc1 into PatrickF1:main Jan 15, 2023
@PatrickF1 PatrickF1 changed the title [Search Directory] Use fd or fdfind unaliased [Search Directory] Use fd or fdfind unaliased to resolve apparent hang Jan 15, 2023
@zolrath
Copy link

zolrath commented Jan 21, 2023

On Ubuntu there is a default fd which is a different CLI utility entirely (File & Directory tool)

So when the fd_cmd searches for fd first, it finds it but it still results in a broken directory search

set fd_cmd (command -v fd || command -v fdfind || echo "fd")

Instead, if the first check is for fdfind

set fd_cmd (command -v fdfind || command -v fd || echo "fd")

this operates properly even though there is a preexisting, incorrect fd.

@PatrickF1
Copy link
Owner

PatrickF1 commented Jan 21, 2023

Thanks @zolrath for reporting this. My bad, should've been an easy catch if I had thought it through. This is why I need code reviews 😅️
Just curious, is fd (File & Directory tool) a default tool on Ubuntu? I hope I didn't just break this for ALL Ubuntu users.

PatrickF1 added a commit that referenced this pull request Jan 21, 2023
PatrickF1 added a commit that referenced this pull request Jan 21, 2023
Follow up to a bug and missing readme update in #282. #282 (comment)
@PatrickF1
Copy link
Owner

Fixed in 7b14fa7

@zolrath
Copy link

zolrath commented Jan 22, 2023

Thanks @zolrath for reporting this. My bad, should've been an easy catch if I had thought it through. This is why I need code reviews 😅️ Just curious, is fd (File & Directory tool) a default tool on Ubuntu? I hope I didn't just break this for ALL Ubuntu users.

Yep, it's at least installed by default in 22.04! Thanks for applying the fix! 🔥

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.

aliasing fd causes Search Directory to hang
3 participants