Skip to content

Fix incorrect fzf minimum-version comparison#1030

Open
shaked-shlomo wants to merge 1 commit into
denisidoro:masterfrom
shaked-shlomo:fix/fzf-version-comparison
Open

Fix incorrect fzf minimum-version comparison#1030
shaked-shlomo wants to merge 1 commit into
denisidoro:masterfrom
shaked-shlomo:fix/fzf-version-comparison

Conversation

@shaked-shlomo

Copy link
Copy Markdown

Problem

The minimum supported fzf version is 0.23.1 (MIN_FZF_VERSION_*), but the check in src/finder/mod.rs ANDs the three components independently:

if major == MIN_FZF_VERSION_MAJOR   // == 0
    && minor < MIN_FZF_VERSION_MINOR // < 23
    && patch < MIN_FZF_VERSION_PATCH // < 1  (i.e. patch == 0)
{

patch < 1 is only true when patch == 0, so the warning fires for the wrong set of versions:

fzf version older than 0.23.1? current check fires?
0.22.0 yes ✅ yes
0.22.5 yes no (5 < 1 is false)
0.23.0 yes no (23 < 23 is false)
0.23.1 no ✅ no
1.0.0 no ✅ no

So users on common too-old versions like 0.22.x (x≥1) or 0.23.0 silently skip the warning, while only *.*.0 versions are caught.

Fix

Compare the version tuples lexicographically (Rust tuples already implement Ord this way), which correctly flags any version below the minimum:

if (major, minor, patch)
    < (MIN_FZF_VERSION_MAJOR, MIN_FZF_VERSION_MINOR, MIN_FZF_VERSION_PATCH)
{

No behavior change for versions ≥ 0.23.1; it only fixes the missed older versions.

The minimum supported fzf version is 0.23.1, but the guard required
major == 0 && minor < 23 && patch < 1 (all with &&). Because patch < 1
is only true for patch == 0, versions such as 0.22.5 and 0.23.0 — both
older than 0.23.1 — were not detected as too old, while only x.y.0
versions were. Compare the (major, minor, patch) tuples directly so any
version below the minimum is correctly flagged.
@shaked-shlomo
shaked-shlomo requested a review from denisidoro as a code owner June 1, 2026 22:15
@welcome

welcome Bot commented Jun 1, 2026

Copy link
Copy Markdown

Thanks for opening this pull request!

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.

1 participant