From 8de1e3a8878b7971a4eb9f8d503f57d178fd97db Mon Sep 17 00:00:00 2001 From: Pablo Garcia Date: Tue, 11 Aug 2026 15:32:29 +0200 Subject: [PATCH 1/3] find: share one file handle per output path 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, so repeated references to the same path reuse a single file offset. Fixes #439 --- src/find/matchers/ls.rs | 7 ++-- src/find/matchers/mod.rs | 78 +++++++++++++++++++++++++++++++----- src/find/matchers/printer.rs | 7 ++-- src/find/matchers/printf.rs | 10 +++-- src/find/mod.rs | 8 ++++ 5 files changed, 90 insertions(+), 20 deletions(-) diff --git a/src/find/matchers/ls.rs b/src/find/matchers/ls.rs index 9b05f82d..771c9880 100644 --- a/src/find/matchers/ls.rs +++ b/src/find/matchers/ls.rs @@ -7,6 +7,7 @@ use chrono::DateTime; use std::{ fs::File, io::{stderr, Write}, + rc::Rc, }; use super::{Matcher, MatcherIO, WalkEntry}; @@ -110,11 +111,11 @@ fn format_permissions(file_attributes: u32) -> String { } pub struct Ls { - output_file: Option, + output_file: Option>, } impl Ls { - pub fn new(output_file: Option) -> Self { + pub fn new(output_file: Option>) -> Self { Self { output_file } } @@ -270,7 +271,7 @@ impl Ls { impl Matcher for Ls { fn matches(&self, file_info: &WalkEntry, matcher_io: &mut MatcherIO) -> bool { if let Some(file) = &self.output_file { - self.print(file_info, matcher_io, file, true); + self.print(file_info, matcher_io, &**file, true); } else { self.print( file_info, diff --git a/src/find/matchers/mod.rs b/src/find/matchers/mod.rs index 1a2cc68a..0ce5a747 100644 --- a/src/find/matchers/mod.rs +++ b/src/find/matchers/mod.rs @@ -65,7 +65,9 @@ use ls::Ls; use std::{ error::Error, fs::{File, Metadata}, + io::Read, path::{Path, PathBuf}, + rc::Rc, str::FromStr, time::SystemTime, }; @@ -440,10 +442,21 @@ fn parse_str_to_newer_args(input: &str) -> Option<(String, String)> { } } -/// Creates a file if it doesn't exist. -/// If it does exist, it will be overwritten. -fn get_or_create_file(path: &str) -> Result> { - let file = File::create(path)?; +/// Returns the output file for `path`, creating (and truncating) it the first +/// time it is requested. +/// +/// Later requests for the same path reuse the handle opened earlier, so that +/// several output predicates writing to one file share a single file offset +/// instead of overwriting each other. +fn get_or_create_file(config: &mut Config, path: &str) -> Result, Box> { + if let Some(file) = config.output_files.get(path) { + return Ok(Rc::clone(file)); + } + + let file = Rc::new(File::create(path)?); + config + .output_files + .insert(path.to_string(), Rc::clone(&file)); Ok(file) } @@ -484,7 +497,7 @@ fn build_matcher_tree( } i += 1; - let file = get_or_create_file(args[i])?; + let file = get_or_create_file(config, args[i])?; Some(Printer::new(PrintDelimiter::Newline, Some(file)).into_box()) } "-fprintf" => { @@ -496,7 +509,7 @@ fn build_matcher_tree( // Args + 1: output file path // Args + 2: format string i += 1; - let file = get_or_create_file(args[i])?; + let file = get_or_create_file(config, args[i])?; let output_path = PathBuf::from(args[i]); i += 1; Some(Printf::new(args[i], Some((file, output_path)))?.into_box()) @@ -507,7 +520,7 @@ fn build_matcher_tree( } i += 1; - let file = get_or_create_file(args[i])?; + let file = get_or_create_file(config, args[i])?; Some(Printer::new(PrintDelimiter::Null, Some(file)).into_box()) } "-ls" => Some(Ls::new(None).into_box()), @@ -517,7 +530,7 @@ fn build_matcher_tree( } i += 1; - let file = get_or_create_file(args[i])?; + let file = get_or_create_file(config, args[i])?; Some(Ls::new(Some(file)).into_box()) } "-true" => Some(TrueMatcher.into_box()), @@ -1029,6 +1042,8 @@ mod tests { use super::*; use crate::find::tests::fix_up_slashes; use crate::find::tests::FakeDependencies; + use std::io::Write; + use tempfile::Builder; /// Helper function for tests to get a [WalkEntry] object. root should /// probably be a string starting with `test_data/` (cargo's tests run with @@ -1908,25 +1923,66 @@ mod tests { fn get_or_create_file_test() { use std::fs; + let mut config = Config::default(); + // remove file if hard link file exist. // But you can't delete a file that doesn't exist, // so ignore the error returned here. let _ = fs::remove_file("test_data/get_or_create_file_test"); // test create file - let file = get_or_create_file("test_data/get_or_create_file_test"); + let file = get_or_create_file(&mut config, "test_data/get_or_create_file_test"); assert!(file.is_ok()); - let file = get_or_create_file("test_data/get_or_create_file_test"); + let file = get_or_create_file(&mut config, "test_data/get_or_create_file_test"); assert!(file.is_ok()); // test error when file no permission #[cfg(unix)] { - let result = get_or_create_file("/etc/shadow"); + let result = get_or_create_file(&mut config, "/etc/shadow"); assert!(result.is_err()); } let _ = fs::remove_file("test_data/get_or_create_file_test"); } + + #[test] + fn get_or_create_file_reuses_handle_for_same_path() { + use std::fs; + + let temp_dir = Builder::new().prefix("example").tempdir().unwrap(); + let path = temp_dir.path().join("out"); + let path = path.to_string_lossy().to_string(); + let mut config = Config::default(); + + let first = get_or_create_file(&mut config, &path).unwrap(); + let second = get_or_create_file(&mut config, &path).unwrap(); + assert!(Rc::ptr_eq(&first, &second)); + + // Writes through both handles share one file offset, so neither + // overwrites the other. + writeln!(&*first, "one").unwrap(); + writeln!(&*second, "two").unwrap(); + assert_eq!("one\ntwo\n", fs::read_to_string(&path).unwrap()); + } + + #[test] + fn two_fprints_to_the_same_file_do_not_overwrite_each_other() { + use std::fs; + + let temp_dir = Builder::new().prefix("example").tempdir().unwrap(); + let path = temp_dir.path().join("out"); + let path = path.to_string_lossy().to_string(); + let mut config = Config::default(); + + let matcher = + build_top_level_matcher(&["-fprint", &path, "-fprint", &path], &mut config).unwrap(); + let deps = FakeDependencies::new(); + let abbbc = get_dir_entry_for("test_data/simple", "abbbc"); + matcher.matches(&abbbc, &mut deps.new_matcher_io()); + + let expected = format!("{0}\n{0}\n", abbbc.path().to_string_lossy()); + assert_eq!(expected, fs::read_to_string(&path).unwrap()); + } } diff --git a/src/find/matchers/printer.rs b/src/find/matchers/printer.rs index c26287a1..06ec492b 100644 --- a/src/find/matchers/printer.rs +++ b/src/find/matchers/printer.rs @@ -6,6 +6,7 @@ use std::fs::File; use std::io::{stderr, Write}; +use std::rc::Rc; use super::{Matcher, MatcherIO, WalkEntry}; @@ -26,11 +27,11 @@ impl std::fmt::Display for PrintDelimiter { /// This matcher just prints the name of the file to stdout. pub struct Printer { delimiter: PrintDelimiter, - output_file: Option, + output_file: Option>, } impl Printer { - pub fn new(delimiter: PrintDelimiter, output_file: Option) -> Self { + pub fn new(delimiter: PrintDelimiter, output_file: Option>) -> Self { Self { delimiter, output_file, @@ -72,7 +73,7 @@ impl Printer { impl Matcher for Printer { fn matches(&self, file_info: &WalkEntry, matcher_io: &mut MatcherIO) -> bool { if let Some(file) = &self.output_file { - self.print(file_info, matcher_io, file, true); + self.print(file_info, matcher_io, &**file, true); } else { self.print( file_info, diff --git a/src/find/matchers/printf.rs b/src/find/matchers/printf.rs index 8339ab1a..8eda93a3 100644 --- a/src/find/matchers/printf.rs +++ b/src/find/matchers/printf.rs @@ -9,6 +9,7 @@ use std::error::Error; use std::fs::{self, File}; use std::io::{stderr, Write}; use std::path::{Path, PathBuf}; +use std::rc::Rc; use std::time::SystemTime; use chrono::{format::StrftimeItems, DateTime, Local}; @@ -593,11 +594,14 @@ fn format_directive<'entry>( /// find's printf syntax. pub struct Printf { format: FormatString, - output_file: Option<(File, PathBuf)>, + output_file: Option<(Rc, PathBuf)>, } impl Printf { - pub fn new(format: &str, output_file: Option<(File, PathBuf)>) -> Result> { + pub fn new( + format: &str, + output_file: Option<(Rc, PathBuf)>, + ) -> Result> { Ok(Self { format: FormatString::parse(format)?, output_file, @@ -661,7 +665,7 @@ impl Printf { impl Matcher for Printf { fn matches(&self, file_info: &WalkEntry, matcher_io: &mut MatcherIO) -> bool { let result = if let Some((file, _)) = &self.output_file { - self.print(file_info, file) + self.print(file_info, &**file) } else { self.print(file_info, &mut *matcher_io.deps.get_output().borrow_mut()) }; diff --git a/src/find/mod.rs b/src/find/mod.rs index ff047c82..d4717074 100644 --- a/src/find/mod.rs +++ b/src/find/mod.rs @@ -8,7 +8,9 @@ pub mod matchers; use matchers::{Follow, WalkEntry}; use std::cell::RefCell; +use std::collections::HashMap; use std::error::Error; +use std::fs::File; #[cfg(unix)] use std::io::IsTerminal; use std::io::{self, stderr, stdout, BufRead, BufReader, Write}; @@ -32,6 +34,11 @@ pub struct Config { /// Whether the expression uses -ok or -okdir, which prompt on stderr and /// read the answer from stdin when there is no terminal. interactive_exec: bool, + /// Files opened by output predicates (-fprint, -fprintf, -fprint0, -fls), + /// keyed by the path given on the command line. Reusing one handle per + /// path means specifying the same output file more than once appends + /// rather than overwriting (see issue #439). + output_files: HashMap>, } impl Default for Config { @@ -52,6 +59,7 @@ impl Default for Config { follow: Follow::Never, files0_argument: None, // This option exclusively for -files0-from argument. interactive_exec: false, + output_files: HashMap::new(), } } } From 2289f6675b71274cffc4d531527fae4bbd2f7378 Mon Sep 17 00:00:00 2001 From: Pablo Garcia Date: Wed, 19 Aug 2026 14:10:12 +0200 Subject: [PATCH 2/3] find: drop unused io::Read import after rebase --- src/find/matchers/mod.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/find/matchers/mod.rs b/src/find/matchers/mod.rs index 0ce5a747..4f6a2b18 100644 --- a/src/find/matchers/mod.rs +++ b/src/find/matchers/mod.rs @@ -65,7 +65,6 @@ use ls::Ls; use std::{ error::Error, fs::{File, Metadata}, - io::Read, path::{Path, PathBuf}, rc::Rc, str::FromStr, From 2b5c84f9fb9a6b685270ce32c9aa43fa59c9b7a5 Mon Sep 17 00:00:00 2001 From: MsfPablo Date: Wed, 19 Aug 2026 16:51:00 +0200 Subject: [PATCH 3/3] fix: wrap /dev/full handle in Rc for Printer::new test Printer::new takes Option> 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. --- src/find/matchers/printer.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/find/matchers/printer.rs b/src/find/matchers/printer.rs index 06ec492b..24e6392a 100644 --- a/src/find/matchers/printer.rs +++ b/src/find/matchers/printer.rs @@ -129,7 +129,7 @@ mod tests { let dev_full = File::open("/dev/full").unwrap(); let abbbc = get_dir_entry_for("./test_data/simple", "abbbc"); - let matcher = Printer::new(PrintDelimiter::Newline, Some(dev_full)); + let matcher = Printer::new(PrintDelimiter::Newline, Some(Rc::new(dev_full))); let deps = FakeDependencies::new(); assert!(matcher.matches(&abbbc, &mut deps.new_matcher_io()));