Skip to content

find, locate: replace onig with fancy-regex - #864

Open
wtcpython wants to merge 1 commit into
uutils:mainfrom
wtcpython:replace-onig-with-fancy-regex
Open

wtcpython wants to merge 1 commit into
uutils:mainfrom
wtcpython:replace-onig-with-fancy-regex

Conversation

@wtcpython

Copy link
Copy Markdown
Contributor

Closes #212

@wtcpython
wtcpython force-pushed the replace-onig-with-fancy-regex branch from 4becade to 571d3a2 Compare September 18, 2026 11:39
@codspeed

codspeed Bot commented Sep 18, 2026 •

Copy link
Copy Markdown

Merging this PR will degrade performance by 29.38%

⚡ 1 improved benchmark
❌ 5 regressed benchmarks
✅ 14 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
❌ regex_path 49.8 ms 112.9 ms -55.94%
❌ build_pruned 33.7 ms 56.6 ms -40.49%
❌ combined_expr 52.3 ms 82.1 ms -36.22%
❌ iname_glob 36.3 ms 54.9 ms -33.93%
❌ name_glob 36.7 ms 52.6 ms -30.23%
⚡ prune 13.6 ms 8.5 ms +60.9%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing wtcpython:replace-onig-with-fancy-regex (6e93bd7) with main (42143b5)

Open in CodSpeed

@oech3

oech3 commented Sep 19, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will degrade performance by 36.61%

Oh no...

Comment thread Cargo.toml Outdated
fancy-regex = { version = "0.19.2", default-features = false, features = ["std", "unicode"] }
regex = "1.12"
uucore = { version = "0.10.0", features = ["entries", "fs", "fsext", "mode"] }
uucore = { git = "https://github.com/wtcpython/coreutils.git", branch = "uucore-regex", features = ["entries", "fs", "fsext", "mode", "regex"] }

This comment was marked as outdated.

This comment was marked as outdated.

@wtcpython
wtcpython force-pushed the replace-onig-with-fancy-regex branch 2 times, most recently from 70d7f30 to 671b7ca Compare September 23, 2026 01:01
@github-actions

Copy link
Copy Markdown

Commit 70d7f30 has test result changes:

bfs testsuite:

Test results comparison:
  Current:   TOTAL: 316 / PASSED: 273 / FAILED: 37 / SKIPPED: 6
  Reference: TOTAL: 317 / PASSED: 274 / FAILED: 37 / SKIPPED: 6

Changes from main branch:
  TOTAL: -1
  PASSED: -1
  FAILED: +0

No result in this run (1) - hung, crashed, or renamed:
  ? posix/depth (was PASS)

@wtcpython
wtcpython marked this pull request as ready for review September 23, 2026 01:09
Copilot AI lite review requested due to automatic review settings September 23, 2026 01:09

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@sylvestre

Copy link
Copy Markdown
Contributor

the ci is red :)

Comment thread src/find/matchers/regex.rs Outdated
RegexType::PosixBasic => Syntax::posix_basic(),
RegexType::PosixExtended => Syntax::posix_extended(),
// GNU find's -regex does full-path matching, so anchor the pattern.
let anchored = if pattern.starts_with('^') && pattern.ends_with('$') {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

anchoring by concatenation breaks alternation: a|b becomes ^a|b$, which means ^a or b$. please wrap it instead, something like ^(?:{pattern})$

Comment thread src/find/matchers/regex.rs Outdated
} else if pattern.ends_with('$') {
format!("^{pattern}")
} else {
format!("^{pattern}$")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this changes -regex/-name matching semantics, please add tests in tests/test_find.rs (alternation, ^/$ in the middle, -iregex, each -regextype)

Comment thread src/find/matchers/regex.rs Outdated
fn matches(&self, file_info: &WalkEntry, _: &mut MatcherIO) -> bool {
self.regex
.is_match(file_info.path().to_string_lossy().as_ref())
.unwrap_or(false)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

unwrap_or(false) silently turns a backtrack-limit error into "no match", is that what we want?

Comment thread src/find/matchers/regex_transpile.rs Outdated
}

match chars.next() {
Some(c) if "(){}|+?".contains(c) => output.push(c),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

in emacs syntax a bare | is a literal and only \| is alternation, so | needs to be escaped in the other branch. the doc comment above says the opposite

Comment thread src/find/matchers/glob.rs Outdated
// so anchor the regex to prevent partial matches.
let anchored = format!("^{r}$");
regex_transpile::compile(&anchored, super::regex::RegexType::PosixBasic, caseless)
.unwrap()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

unwrap() on a user-supplied pattern, could you please propagate the error? the "should never fail" comment was true for parse_bre, not for the new transpiler

@wtcpython
wtcpython force-pushed the replace-onig-with-fancy-regex branch from 671b7ca to 097a4d1 Compare September 23, 2026 14:02
Copilot AI review requested due to automatic review settings September 23, 2026 14:02

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@wtcpython
wtcpython force-pushed the replace-onig-with-fancy-regex branch from 097a4d1 to 6e93bd7 Compare September 25, 2026 13:27
Copilot AI review requested due to automatic review settings September 25, 2026 13:27

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@codecov

codecov Bot commented Sep 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.63670% with 50 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.16%. Comparing base (42143b5) to head (6e93bd7).

Files with missing lines Patch % Lines
src/find/matchers/bre_to_ere.rs 87.07% 23 Missing and 23 partials ⚠️
src/find/matchers/regex_transpile.rs 97.97% 1 Missing and 1 partial ⚠️
src/find/matchers/glob.rs 95.00% 1 Missing ⚠️
src/find/matchers/mod.rs 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #864      +/-   ##
==========================================
- Coverage   92.28%   92.16%   -0.12%     
==========================================
  Files          35       37       +2     
  Lines        7504     7964     +460     
  Branches      390      439      +49     
==========================================
+ Hits         6925     7340     +415     
- Misses        437      459      +22     
- Partials      142      165      +23     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

consider making onig optional

4 participants