Skip to content

find: share one file handle per output path (fixes #439) - #828

Open
MsfPablo wants to merge 3 commits into
uutils:mainfrom
MsfPablo:fix-439-dedup-output-files
Open

find: share one file handle per output path (fixes #439)#828
MsfPablo wants to merge 3 commits into
uutils:mainfrom
MsfPablo:fix-439-dedup-output-files

Conversation

@MsfPablo

Copy link
Copy Markdown
Contributor

Fixes #439.

Problem

GNU find de-duplicates the files opened by its output predicates, so
find . -fprint foo -fprint foo writes each path to foo twice. uutils
opened a separate File for every occurrence of -fprint/-fprintf/
-fprint0/-fls, so each handle had its own file offset and the writes
overwrote one another — only one line per entry survived.

Fix

get_or_create_file now caches opened output files on Config, keyed by the
path as written on the command line (matching GNU's literal-filename
de-duplication), and hands out a shared Rc<File>. Repeated references to the
same path therefore share a single file descriptor and a single file offset.
Printer, Printf and Ls take Option<Rc<File>> instead of Option<File>.
The first reference still truncates, as before.

Tests

Two unit tests in src/find/matchers/mod.rs:

  • get_or_create_file_reuses_handle_for_same_path — the same path yields the
    same Rc, and writes through both handles append rather than overwrite.
  • two_fprints_to_the_same_file_do_not_overwrite_each_other — end-to-end
    through build_top_level_matcher with -fprint out -fprint out, asserting
    the entry appears twice.

Verified the regression test fails without the fix (reverting just the cache
lookup gives left: "…abbbc\n…abbbc\n" / right: "…abbbc\n").

$ cargo test
test result: ok. 228 passed; 0 failed  (lib)
... all other suites ok, 0 failed
$ cargo clippy --all-targets --all-features -- -D warnings
    Finished `dev` profile
$ cargo fmt --all --check
(clean)

AI disclosure

This change was written with the assistance of Claude (Anthropic). I have
reviewed the diff, and ran the build, the full test suite, clippy and rustfmt
locally as shown above.

@github-actions

Copy link
Copy Markdown

Commit 2081a60 has test result changes:

bfs testsuite:

Test results comparison:
  Current:   TOTAL: 313 / PASSED: 266 / FAILED: 41 / SKIPPED: 6
  Reference: TOTAL: 313 / PASSED: 267 / FAILED: 40 / SKIPPED: 6

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

New test failures (1):
  - gnu/okdir_path_relative

@sylvestre

Copy link
Copy Markdown
Contributor

many jobs are failing

@sylvestre

Copy link
Copy Markdown
Contributor

could you please fix the conflict? thanks

Pablo Garcia added 2 commits August 19, 2026 14:09
GNU find de-duplicates the files opened by -fprint, -fprintf, -fprint0
and -fls, so `find -fprint foo -fprint foo` writes each line twice.
We opened a separate File per predicate, so the two handles had
independent file offsets and their writes overwrote each other.

Cache opened output files by the path given on the command line and
hand out a shared Rc<File>, so repeated references to the same path
reuse a single file offset.

Fixes uutils#439
@MsfPablo
MsfPablo force-pushed the fix-439-dedup-output-files branch from 2081a60 to 2289f66 Compare August 19, 2026 12:10
@github-actions

Copy link
Copy Markdown

Commit 2289f66 has test result changes:

bfs testsuite:

Test results comparison:
  Current:   TOTAL: 317 / PASSED: 274 / FAILED: 37 / SKIPPED: 6
  Reference: TOTAL: 314 / PASSED: 266 / FAILED: 42 / SKIPPED: 6

Changes from main branch:
  TOTAL: +3
  PASSED: +8
  FAILED: -5

Test improvements (5):
  + gnu/execdir_path_dot
  + gnu/execdir_path_empty
  + gnu/execdir_path_relative
  + gnu/files0_from_ok
  + gnu/okdir_path_empty

Printer::new takes Option<Rc<File>> since the output-file dedup
change, but the prints_error_message test still passed a bare File.
Wrap it with Rc::new to satisfy the signature and clippy -D warnings.
@codspeed-hq

codspeed-hq Bot commented Aug 19, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 20 untouched benchmarks


Comparing MsfPablo:fix-439-dedup-output-files (2b5c84f) with main (1acf40a)

Open in CodSpeed

@github-actions

Copy link
Copy Markdown

Commit 2b5c84f has test result changes:

GNU findutils testsuite:

Test results comparison:
  Current:   TOTAL: 495 / PASSED: 421 / FAILED: 73 / SKIPPED: 1
  Reference: TOTAL: 495 / PASSED: 420 / FAILED: 74 / SKIPPED: 1

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

Test improvements (1):
  + size-invalid.new-O3

@codecov

codecov Bot commented Aug 19, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.15385% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.26%. Comparing base (1acf40a) to head (2b5c84f).

Files with missing lines Patch % Lines
src/find/matchers/ls.rs 50.00% 1 Missing ⚠️
src/find/matchers/mod.rs 97.56% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #828      +/-   ##
==========================================
+ Coverage   92.22%   92.26%   +0.03%     
==========================================
  Files          35       35              
  Lines        7435     7471      +36     
  Branches      386      387       +1     
==========================================
+ Hits         6857     6893      +36     
  Misses        437      437              
  Partials      141      141              

☔ 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.

Specifying the same file for output multiple times in find will overwrite the file by default.

2 participants