Convert all content types to string

[ibhagwan]

fzf_exec now converts all content types (tables/funcs) to shell command strings this greatly simplifies the code as all picker inputs are now shell commands which can be used in fzf's reload, execute-silent, etc.

This will also enable us to combine pickers easily with semicolon or logical and, e.g. cmd1; cmd2 or cmd1 && cmd2.

Fixes #2058 and prepares the ground for #1879, #2058.

[ibhagwan]

@phanen, still a lot more work , but starting to look like something...

TODO:

• Merge core.mt_cmd_wrapper into shell.stringify and remove all calls to core.mt_cmd_wrapper
• Replace shell.raw_action and shell.raw_preview_action_cmd with the new shell.stringify (should be able to pipe any type of content into fzf)
• Automatic convert fn_transform to strings with string.dump when possible (no upvalue refs)
• Add fzf_exec/fzf_live API testing for all use cases (string, multiprocess, table, function, etc)
• Deprecate fn_{pre|post}_fzf
• Add opts._resume_reload which will be used by the hide profile to determine if we need to reload on unhide (e.g. buffer lines changes, etc).

[phanen]

:FzfLua live_grep rg_glob=true multiprocess=false glob don't work after 624ae3b.

[ibhagwan]

:FzfLua live_grep rg_glob=true multiprocess=false glob don't work after 624ae3b.

Ty @phanen, I’ll take a look, still a lot more work but I’m happy with the code cleanup.

[ibhagwan]

live_grep_st use a function rather a pre-built string:

Correct, but I have to find a generic solution for this that isn’t a hack or maybe a condition in live_grep which changes fn_reload based on opts.multiprocess?

mutliprocess=false is just for testing/debugging issues anyways.

[ibhagwan]

I remember I've been using multiprocess=false because I don't need file_icon/git_icon and want glob feature while live_grep_native don't support glob.

You’re always at a performance disadvantage this way, why not use multiprocess=true with git_icons=false and file_icons=false?

[phanen]

I don't know... FzfLua live_grep rg_glob=true multiprocess=false is significantly faster than FzfLua live_grep rg_glob=true multiprocess=true

[ibhagwan]

I don't know... FzfLua live_grep rg_glob=true multiprocess=false is significantly faster than FzfLua live_grep rg_glob=true multiprocess=true

Weird, it shouldn’t be, multiprocess should always be faster as it’s an external independent command.

[ibhagwan]

live workspace symbol also broken.

Ty for the diff fixes, I’ll add these later today.

[phanen]

Damn, I forgot to disable temp-throttle.service. So my words may be wrong, I will test it again.

[ibhagwan]

Pushed your fixes in 497ab31, ty @phanen!

[ibhagwan]

@phanen, fixed some more stuff, still have an issue with multiprocess=false and file_icons=mini which I’m able to solve with vim.schedule by uncommenting this line:

fzf-lua/lua/fzf-lua/libuv.lua

Lines 215 to 216 in 01ef2bb

	-- Avoid "attempt to yield across C-call boundary"
	-- if vim.in_fast_event() then vim.schedule(read) else read() end

But that presents another issue, it seems that when I use vim.scheudule there the fzf pipe closes before all the reads are complete (maybe fzf closes it with process exit?), if you run :FzfLua live_grep multiprocess=false and type quickly you will sometimes encounter partial/empty results, or if you run :FzfLua files on a large folder the last read will get cut resulting in less number of files.

Had to go out so I left it commented for now but once I solve that it would also remove the need for the ft_match mock as calls to fn_transform will be executed outside of the “fast event”.

Once I solve this we can “retire” both of these functions (I think):

fzf-lua/lua/fzf-lua/path.lua

Line 612 in 01ef2bb

function M.ft_match(args)

fzf-lua/lua/fzf-lua/path.lua

Line 639 in 01ef2bb

function M.ft_match_fast_event(args)

[ibhagwan]

Seems the issue is the pipe gets closed before finalizing all writes so this line is hit:

fzf-lua/lua/fzf-lua/shell.lua

Line 328 in 01ef2bb

if not pipe then return end

Probably some race condition causing this to trigger:

fzf-lua/lua/fzf-lua/shell.lua

Lines 315 to 321 in 01ef2bb

	if write_cb_count == 0 then
	-- only close if all our uv.write calls are completed
	uv.close(pipe)
	pipe = nil
	-- Run the postprocess function
	if type(fn_postprocess) == "function" then fn_postprocess(opts) end
	end

[ibhagwan]

Fixed in 2cd1528, finish would be called before any read were scheduled so write_cb_count would be 0, needed to add a read_cb_count.

[ibhagwan]

@phanen, how do you feel about merging this?

[ibhagwan]

Lol, review by Grok4, a bit repetitive but mostly on point:

Overview of the Pull Request
This PR (#2152) in the ibhagwan/fzf-lua repository is a significant refactoring effort aimed at simplifying the codebase for fzf-lua, a Neovim plugin that integrates the fzf fuzzy finder. The core change is to convert all content types (e.g., tables, functions) passed to fzf_exec into shell command strings. This allows all picker inputs to be treated uniformly as shell commands, making it easier to use fzf features like reload and execute-silent. It also enables new functionality, such as combining multiple pickers using semicolons (;) or logical AND (&&), e.g., :FzfLua combine pickers=buffers,files.
The PR includes 24 commits (with multiple force-pushes), resulting in +2,565 lines added and -1,710 removed. It fixes issue #2058 (related to global search behavior) and lays groundwork for #1879 (VS Code-inspired search features like @, #, $). It bumps the minimum fzf version requirement from >0.25 to >0.36 (released in Jan 2023), deprecating support for older versions. The changes touch configuration, core logic, providers, utilities, and tests, with a focus on performance, async handling, and code cleanup.
Based on the PR description, comments, and code diffs, the changes appear well-motivated and progressively refined through iterative fixes. However, I’ll highlight potential flaws, bugs, or issues below after summarizing the key changes.
Key Changes and Refactoring
The diffs span multiple files, primarily in the lua/fzf-lua/ directory. Here’s a consolidated summary from the extracted diffs:
• CI and Config Files:
◦ .github/workflows/ci.yaml: Updated Neovim to v0.11.2 and fzf to v0.64.0 in CI setup for better compatibility testing.
◦ .luarc.jsonc: Added testing globals (describe, it) to diagnostics for improved Lua LSP support.
• Documentation:
◦ README.md: Updated fzf minimum version and fixed a config example (only_valid → valid_only in quickfix previewer).
◦ doc/fzf-lua.txt: Minor formatting updates (e.g., file end markers), no substantive changes.
• Actions and Completion Logic (lua/fzf-lua/actions.lua, lua/fzf-lua/complete.lua):
◦ New function list_del added to delete entries from quickfix/location lists based on selections (using buffer numbers).
◦ Updated grep_lgrep to use multiprocess (typo fixed from multiprcess) for handling globs in grep commands.
◦ Removed core.mt_cmd_wrapper calls in completion functions (path, file), directly passing opts.cmd to core.fzf_exec for simplification.
• Configuration Normalization (lua/fzf-lua/config.lua):
◦ Extensive refactoring in normalize_opts: Consolidated option handling, added support for new features like picker combining.
◦ Deprecated old keys (e.g., fn_pre_fzf, fn_post_fzf) and removed related hooks.
◦ Added defaults for new options like _resume_reload for hide/unhide behavior.
• Core Logic (lua/fzf-lua/core.lua):
◦ Major refactoring: fzf_exec now stringifies all inputs (tables/functions to shell cmds). Removed wrappers like mt_cmd_wrapper.
◦ Added stringify for content conversion, enabling piping of any content type into fzf.
◦ Introduced fzf_live for live searching, with better handling of multiprocess modes.
◦ Deprecated fn_pre/post_fzf and related functions; replaced with string-based transformations.
◦ Bug fixes for async reading/writing, including counters for read/write callbacks to prevent pipe closure races.
• Providers and Pickers (e.g., lua/fzf-lua/providers/buffers.lua, lua/fzf-lua/providers/files.lua, lua/fzf-lua/providers/grep.lua):
◦ Unified input handling to strings across pickers (e.g., buffers, files, grep, lsp).
◦ Added support for combining pickers; first picker’s opts determine settings (window, previewer).
◦ Fixes for glob patterns in live_grep (e.g., when rg_glob=true multiprocess=false).
◦ Performance tweaks: Better handling of icons (file/git) in non-multiprocess modes, using vim.schedule to avoid yielding across C boundaries.
◦ Workspace symbol fixes for live updates.
• Shell and Utilities (lua/fzf-lua/shell.lua, lua/fzf-lua/utils.lua):
◦ New stringify function to convert functions/tables to dumpable strings (using string.dump where possible, no upvalues).
◦ Removed raw_action and raw_preview_action_cmd; replaced with string-based piping.
◦ Added resume/reload logic for dynamic content (e.g., buffer changes on unhide).
• Window and UI (lua/fzf-lua/win.lua):
◦ Updated keymap handling (e.g., toggle_help with format_bind fixes).
◦ Added _resume_reload for reload on unhide in hide profiles.
• Tests (Various test files under tests/):
◦ Adjustments for API testing across content types (string, multiprocess, table, function).
◦ Fixes for test failures, e.g., in win_spec.lua (concatenation errors with function values).
• Other Minor Changes:
◦ Replaced vim.v.argv with _G.arg for consistency.
◦ Inline functions like setup_fzf_interactive_native.
◦ Workarounds for title flags in non-multiprocess modes.
Overall, the refactoring reduces code duplication by treating all inputs as strings, improves modularity (e.g., easier picker combining), and enhances performance in async scenarios.
Potential Major Flaws, Bugs, or Issues
The PR seems stable based on the iterative fixes in commits and comments (e.g., bugs reported by @phanen were addressed promptly). However, here are some potential concerns, drawn from code diffs, TODOs, and discussions:
1 Incomplete or Unfinished Work (TODOs):
◦ The initial comment lists several TODOs: Merge core.mt_cmd_wrapper into shell.stringify (partially done via removals); auto-convert fn_transform to strings with string.dump (implemented, but warns on upvalues); add comprehensive API testing for all input types (adjustments made, but not exhaustive); deprecate fn_pre/post_fzf (done in commits); add opts._resume_reload for hide/unhide reloads (added).
◦ While many are addressed in later commits, the PR was described as “still a lot more work” early on. If not fully completed, this could leave edge cases unhandled, especially for dynamic content like buffer changes.
2 Race Conditions and Async Issues:
◦ Fixed a race where pipes closed before all reads/writes completed in non-multiprocess modes (added read_cb_count and write_cb_count). Also used vim.schedule to avoid “attempt to yield across C-call boundary” errors.
◦ Potential flaw: These fixes rely on callback counting, which could be error-prone in high-load scenarios (e.g., large folders or rapid typing in live_grep). If not thoroughly stress-tested, partial/empty results might reoccur, as noted in comments.
3 Performance Regressions:
◦ Discussions highlight inconsistencies: multiprocess=false was reported as faster than true in some cases (e.g., live_grep with globs), but shouldn’t be since multiprocess offloads to external commands. This was attributed to temp-throttle services, but could indicate unoptimized string conversion or icon handling overhead.
◦ New string dumping for functions might add latency if functions have upvalues (code handles this by falling back, but could degrade in complex setups).
4 Breaking Changes and Deprecations:
◦ Bumping fzf to >0.36 deprecates older versions, potentially breaking setups for users on outdated fzf (though it’s 2+ years old).
◦ Removal of fn_pre/post_fzf, raw_action, etc., could break custom configurations or plugins relying on them. No migration guide in diffs, though deprecations are noted.
◦ Picker combining is new but warns about mismatching previewers (e.g., combining incompatible types might crash or error).
5 Bug Fixes Indicate Prior Issues:
◦ Fixed glob not working in live_grep after specific commits; live workspace symbols broken and repaired; partial results in large file listings.
◦ A test fix in win_spec.lua addressed concatenation of function values, suggesting type-handling bugs were introduced mid-PR.
◦ Multiprocess=false (debug mode) had icon-related issues, fixed but highlights that debug paths might still be fragile.
6 Testing and Coverage:
◦ API test adjustments were made, but the TODO emphasizes testing all use cases (string, multiprocess, table, function). If incomplete, regressions in less-common scenarios (e.g., function-based inputs with upvalues) could slip through.
◦ No new integration tests for picker combining or resume reloads visible in diffs.
7 General Code Quality:
◦ Error-prone areas: String conversion assumes dumpable functions; if upvalues reference non-dumpable state, it falls back but might lead to silent failures.
◦ No major security issues (e.g., shell command injection), as inputs are plugin-controlled, but user-provided cmds in custom pickers could be risky if not escaped properly (existing utils handle this).
◦ The force-pushes (e.g., 8+ times) suggest rapid iteration, which is fine but increases risk of overlooked changes.
In summary, this PR introduces valuable simplifications and features without obvious critical bugs in the final state, but its WIP nature early on and remaining TODOs suggest it might benefit from more testing before merging. If you’re using fzf-lua, test in a staging environment, especially for multiprocess=false or custom functions. For the latest status, check the PR directly, as it’s open as of July 16, 2025. 0 1

[ibhagwan]

With 34f7ef5 and some improvements to combine and global this PR should close #1879, #2058 and #2063.

[phanen]

how do you feel about merging this?

I think it's fine. The only break change for public api is fzf_live: https://github.com/search?q=fzf_live%20lang%3Alua%20NOT%20is%3Afork&type=code

[ibhagwan]

how do you feel about merging this?

I think it's fine. The only break change for public api is fzf_live: https://github.com/search?q=fzf_live%20lang%3Alua%20NOT%20is%3Afork&type=code

Alright, I'll merge it tomorrow, see if you can think of any other issues.

Also, the new global picker has an issue with resume (that isn't an "unhide"), probably has more issues that I can't think about ATM but it's a new picker, I can fix post merge.

[ibhagwan]

@phanen, updated to 0.11.3, added retires on win/Mac due to their flakey nature with the wait callbacks, let’s hope this ends the manual retries I have to do :)

d1bfe75

[ibhagwan]

I’m not super happy with the hacky opts propagation in the new global picker, I’ll merge this anyway and work on this later, need to find a better solution that propagates consistent opts to both actions and convert actions (for reloading and hide profile).