From 9de4825e5151b6fbe917800a20e29f2b675525ca Mon Sep 17 00:00:00 2001 From: VXNCXNX <93332837+VXNCXNX@users.noreply.github.com> Date: Fri, 21 Aug 2026 22:06:58 +0200 Subject: [PATCH] fix(matcher): an inverse query only checks the first --nth field (#1159) Co-authored-by: VXNCXNX --- src/engine/exact.rs | 35 +++++++++++------- src/engine/exact_tests.rs | 77 +++++++++++++++++++++++++++++++++++++++ tests/cli.rs | 22 +++++++++++ 3 files changed, 121 insertions(+), 13 deletions(-) diff --git a/src/engine/exact.rs b/src/engine/exact.rs index 0f2858b1..050c4e9f 100644 --- a/src/engine/exact.rs +++ b/src/engine/exact.rs @@ -79,24 +79,33 @@ impl MatchEngine for ExactEngine { let mut matched_result = None; let item_text = item.text(); let default_range = [(0, item_text.len())]; - for &(start, end) in item.get_matching_ranges().unwrap_or(&default_range) { - let start = min(start, item_text.len()); - let end = min(end, item_text.len()); - if self.query_regex.is_none() { - matched_result = Some((0, 0)); - break; + let ranges = item.get_matching_ranges().unwrap_or(&default_range); + + if ranges.is_empty() { + // Nothing to match against (e.g. every `--nth` index is out of range): the item + // stays unmatched, inverse or not. + } else if self.query_regex.is_none() { + matched_result = Some((0, 0)); + } else { + for &(start, end) in ranges { + let start = min(start, item_text.len()); + let end = min(end, item_text.len()); + + matched_result = + regex_match(&item_text[start..end], self.query_regex.as_ref()).map(|(s, e)| (s + start, e + start)); + + if matched_result.is_some() { + break; + } } - matched_result = - regex_match(&item_text[start..end], self.query_regex.as_ref()).map(|(s, e)| (s + start, e + start)); - + // An inverse query has to be evaluated over *all* the matching ranges: the item + // only matches when none of them contains the query. Inverting inside the loop + // would let the first non-matching field short-circuit the scan and wrongly + // accept an item whose later fields do contain the query. if self.inverse { matched_result = matched_result.xor(Some((0, 0))); } - - if matched_result.is_some() { - break; - } } let (begin, end) = matched_result?; diff --git a/src/engine/exact_tests.rs b/src/engine/exact_tests.rs index 39152e62..0bc863e7 100644 --- a/src/engine/exact_tests.rs +++ b/src/engine/exact_tests.rs @@ -83,6 +83,83 @@ fn inverse_match_excludes_query() { assert!(e.match_item(&"foo".to_string()).is_none()); } +/// An item exposing explicit matching ranges, as `--nth` produces. +struct RangedItem { + text: String, + ranges: Vec<(usize, usize)>, +} + +impl SkimItem for RangedItem { + fn text(&self) -> std::borrow::Cow<'_, str> { + std::borrow::Cow::Borrowed(&self.text) + } + + fn get_matching_ranges(&self) -> Option<&[(usize, usize)]> { + Some(&self.ranges) + } +} + +#[test] +fn inverse_match_checks_every_matching_range() { + // `--nth 1,2` over "foo bar" yields two ranges: "foo" and "bar". An inverse + // query `!foo` must reject the item because one of the ranges contains "foo", + // even though the *first* range scanned may not. + let e = engine( + "foo", + ExactMatchingParam { + inverse: true, + case: CaseMatching::Ignore, + ..Default::default() + }, + ); + + let foo_in_first_range = RangedItem { + text: "foo bar".to_string(), + ranges: vec![(0, 3), (4, 7)], + }; + assert!( + e.match_item(&foo_in_first_range).is_none(), + "item whose first field contains the query must not match an inverse query" + ); + + let foo_in_second_range = RangedItem { + text: "bar foo".to_string(), + ranges: vec![(0, 3), (4, 7)], + }; + assert!( + e.match_item(&foo_in_second_range).is_none(), + "item whose second field contains the query must not match an inverse query" + ); + + let no_foo = RangedItem { + text: "bar baz".to_string(), + ranges: vec![(0, 3), (4, 7)], + }; + assert!( + e.match_item(&no_foo).is_some(), + "item where no field contains the query must match an inverse query" + ); +} + +#[test] +fn inverse_match_with_no_matching_range_does_not_match() { + // Every `--nth` index out of range leaves the item with no range at all; + // there is nothing to match against, so the item stays unmatched. + let e = engine( + "foo", + ExactMatchingParam { + inverse: true, + case: CaseMatching::Ignore, + ..Default::default() + }, + ); + let item = RangedItem { + text: "bar baz".to_string(), + ranges: vec![], + }; + assert!(e.match_item(&item).is_none()); +} + #[test] fn empty_query_matches_everything() { let e = engine("", ExactMatchingParam::default()); diff --git a/tests/cli.rs b/tests/cli.rs index b7498769..599bebfe 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -110,6 +110,28 @@ fn filter_mode_with_print0() { assert!(stdout.contains('\0')); } +#[test] +fn filter_mode_inverse_query_checks_every_nth_field() { + // `--nth 1,2` gives each item two matching ranges. An inverse query must + // reject an item when ANY of them contains the term, not just the first one. + let (code, stdout, _) = run_sk_argv("foo bar\nqux bar\n", &["-f", "!foo", "--nth", "1,2"], &[]); + assert_eq!(code, Some(0)); + assert!( + !stdout.contains("foo bar"), + "!foo must exclude 'foo bar' (got {stdout:?})" + ); + assert!(stdout.contains("qux bar"), "!foo must keep 'qux bar' (got {stdout:?})"); + + // The term sitting in the second field must be caught too. + let (code, stdout, _) = run_sk_argv("bar foo\nbar qux\n", &["-f", "!foo", "--nth", "1,2"], &[]); + assert_eq!(code, Some(0)); + assert!( + !stdout.contains("bar foo"), + "!foo must exclude 'bar foo' (got {stdout:?})" + ); + assert!(stdout.contains("bar qux"), "!foo must keep 'bar qux' (got {stdout:?})"); +} + #[test] fn filter_mode_no_sort_preserves_input_order() { // Workers grab 4096-item chunks from a shared queue, so with enough items