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

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<File>,
+    output_file: Option<Rc<File>>,
 }
 
 impl Ls {
-    pub fn new(output_file: Option<File>) -> Self {
+    pub fn new(output_file: Option<Rc<File>>) -> 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,

src/find/matchers/mod.rs

@@ -66,6 +66,7 @@ use std::{
     error::Error,
     fs::{File, Metadata},
     path::{Path, PathBuf},
+    rc::Rc,
     str::FromStr,
     time::SystemTime,
 };
@@ -440,10 +441,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<File, Box<dyn Error>> {
-    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<Rc<File>, Box<dyn Error>> {
+    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 +496,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 +508,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 +519,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 +529,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 +1041,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 +1922,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());
+    }
 }

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<File>,
+    output_file: Option<Rc<File>>,
 }
 
 impl Printer {
-    pub fn new(delimiter: PrintDelimiter, output_file: Option<File>) -> Self {
+    pub fn new(delimiter: PrintDelimiter, output_file: Option<Rc<File>>) -> 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,
@@ -128,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()));

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<File>, PathBuf)>,
 }
 
 impl Printf {
-    pub fn new(format: &str, output_file: Option<(File, PathBuf)>) -> Result<Self, Box<dyn Error>> {
+    pub fn new(
+        format: &str,
+        output_file: Option<(Rc<File>, PathBuf)>,
+    ) -> Result<Self, Box<dyn Error>> {
         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())
         };

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<String, Rc<File>>,
 }
 
 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(),
         }
     }
 }