From 2081a601432d2baa1fbef476a975e0781922c8ae Mon Sep 17 00:00:00 2001 From: Pablo Garcia Date: Tue, 11 Aug 2026 15:32:29 +0200 Subject: [PATCH] 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 | 77 ++++++++++++++++++++++++++++++------ src/find/matchers/printer.rs | 7 ++-- src/find/matchers/printf.rs | 7 ++-- src/find/mod.rs | 6 +++ 5 files changed, 84 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 e35ebbbb..e2a1361a 100644 --- a/src/find/matchers/mod.rs +++ b/src/find/matchers/mod.rs @@ -67,6 +67,7 @@ use std::{ fs::{File, Metadata}, io::Read, path::Path, + rc::Rc, str::FromStr, time::SystemTime, }; @@ -437,10 +438,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) } @@ -481,7 +493,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" => { @@ -493,7 +505,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])?; i += 1; Some(Printf::new(args[i], Some(file))?.into_box()) } @@ -503,7 +515,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()), @@ -513,7 +525,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()), @@ -1059,6 +1071,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 @@ -1938,25 +1952,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 31151db2..b223c83e 100644 --- a/src/find/matchers/printf.rs +++ b/src/find/matchers/printf.rs @@ -7,6 +7,7 @@ use std::error::Error; use std::fs::{self, File}; use std::path::Path; +use std::rc::Rc; use std::time::SystemTime; use std::{borrow::Cow, io::Write}; @@ -586,11 +587,11 @@ fn format_directive<'entry>( /// find's printf syntax. pub struct Printf { format: FormatString, - output_file: Option, + output_file: Option>, } impl Printf { - pub fn new(format: &str, output_file: Option) -> Result> { + pub fn new(format: &str, output_file: Option>) -> Result> { Ok(Self { format: FormatString::parse(format)?, output_file, @@ -638,7 +639,7 @@ impl Printf { impl Matcher for Printf { fn matches(&self, file_info: &WalkEntry, matcher_io: &mut MatcherIO) -> bool { 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 f6b80751..a611252e 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}; @@ -30,6 +32,9 @@ pub struct Config { follow: Follow, new_paths: Option>, files0_argument: Option, + /// Files opened by output predicates (-fprint, -fprintf, -fprint0, -fls), + /// keyed by the path given on the command line. + output_files: HashMap>, } impl Default for Config { @@ -50,6 +55,7 @@ impl Default for Config { follow: Follow::Never, new_paths: None, // This option exclusively for -files0-from argument. files0_argument: None, //This option also is used for file0-from + output_files: HashMap::new(), } } }