diff --git a/src/sed/named_writer.rs b/src/sed/named_writer.rs index 18cf1e5c..39b3cc34 100644 --- a/src/sed/named_writer.rs +++ b/src/sed/named_writer.rs @@ -11,17 +11,18 @@ use crate::sed::error_handling::{ScriptLocation, runtime_error}; use std::cell::RefCell; -use std::fs::{File, OpenOptions}; +use std::collections::HashMap; +use std::fs::{self, File, OpenOptions}; use std::io::{BufWriter, Write}; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::rc::Rc; use uucore::display::Quotable; use uucore::error::UResult; thread_local! { - /// Global list of all writers that should be flushed at shutdown - static FLUSH_LIST: RefCell>>> = const { RefCell::new(Vec::new()) }; + /// Writers indexed by canonical output path, used to share duplicate writes. + static WRITERS: RefCell>>> = RefCell::new(HashMap::new()); } #[derive(Debug)] @@ -35,6 +36,13 @@ pub struct NamedWriter { impl NamedWriter { /// Create a new writer, truncate the file, and register it for flushing. pub fn new(path: PathBuf, location: ScriptLocation) -> UResult>> { + let canonical_path = canonicalize_output_path(&path, &location)?; + + if let Some(writer) = WRITERS.with(|writers| writers.borrow().get(&canonical_path).cloned()) + { + return Ok(writer); + } + let file = OpenOptions::new() .create(true) .write(true) @@ -51,10 +59,19 @@ impl NamedWriter { location, })); - FLUSH_LIST.with(|list| list.borrow_mut().push(Rc::clone(&writer))); + WRITERS.with(|writers| { + writers + .borrow_mut() + .insert(canonical_path, Rc::clone(&writer)); + }); Ok(writer) } + /// Return the path used when this writer was first opened. + pub fn original_path(&self) -> &Path { + &self.path + } + /// Write String to the file, possibly with a newline, returning errors. pub fn write_line(&mut self, line: &str, newline: bool) -> UResult<()> { self.write_line_bytes(line.as_bytes(), newline) @@ -92,11 +109,26 @@ impl NamedWriter { } } -/// Flush buffered content to the file, returning descriptive errors. +fn canonicalize_output_path(path: &Path, location: &ScriptLocation) -> UResult { + let parent = path + .parent() + .filter(|parent| !parent.as_os_str().is_empty()) + .unwrap_or_else(|| Path::new(".")); + let canonical_parent = fs::canonicalize(parent).map_err(|e| { + runtime_error::<()>(location, format!("creating file {}: {}", path.quote(), e)).unwrap_err() + })?; + + Ok(match path.file_name() { + Some(file_name) => canonical_parent.join(file_name), + None => canonical_parent, + }) +} + +/// Flush buffered content to all open files, returning descriptive errors. pub fn flush_all() -> UResult<()> { - FLUSH_LIST.with(|cell| { - for handle in cell.borrow().iter() { - handle.borrow_mut().flush()?; + WRITERS.with(|writers| { + for writer in writers.borrow().values() { + writer.borrow_mut().flush()?; } Ok(()) @@ -107,7 +139,7 @@ pub fn flush_all() -> UResult<()> { mod tests { use super::*; use std::fs; - use tempfile::NamedTempFile; + use tempfile::{NamedTempFile, tempdir}; #[test] fn test_write_line_bytes_appends_newline() { @@ -138,4 +170,36 @@ mod tests { assert_eq!(fs::read(path).unwrap(), b"a\xE9"); } + + #[test] + fn test_new_reuses_writer_for_same_path() { + let file = NamedTempFile::new().unwrap(); + let path = file.path().to_path_buf(); + let first = NamedWriter::new(path.clone(), ScriptLocation::default()).unwrap(); + let second = NamedWriter::new(path.clone(), ScriptLocation::default()).unwrap(); + + assert!(Rc::ptr_eq(&first, &second)); + assert_eq!(first.borrow().original_path(), path.as_path()); + + first.borrow_mut().write_line("first", true).unwrap(); + second.borrow_mut().write_line("second", true).unwrap(); + first.borrow_mut().flush().unwrap(); + + assert_eq!(fs::read_to_string(path).unwrap(), "first\nsecond\n"); + } + + #[test] + fn test_new_reuses_writer_for_canonical_duplicate_path() { + let dir = tempdir().unwrap(); + let subdir = dir.path().join("subdir"); + fs::create_dir(&subdir).unwrap(); + + let path = subdir.join("output"); + let duplicate_path = subdir.join(".").join("output"); + let first = NamedWriter::new(path.clone(), ScriptLocation::default()).unwrap(); + let second = NamedWriter::new(duplicate_path, ScriptLocation::default()).unwrap(); + + assert!(Rc::ptr_eq(&first, &second)); + assert_eq!(first.borrow().original_path(), path.as_path()); + } } diff --git a/tests/by-util/test_sed.rs b/tests/by-util/test_sed.rs index aab7142e..7f006ea7 100644 --- a/tests/by-util/test_sed.rs +++ b/tests/by-util/test_sed.rs @@ -1734,6 +1734,27 @@ fn write_single_file() -> std::io::Result<()> { Ok(()) } +#[test] +fn write_single_file_twice() -> std::io::Result<()> { + let temp = NamedTempFile::new()?; + // Write odd and even lines to the same file through different commands. + // Unless the writers are unified, this fails on Windows. + let cmd1 = format!("3~2w {}", temp.path().display()); + let cmd2 = format!("4~2w {}", temp.path().display()); + + new_ucmd!() + .args(&["-e", &cmd1, "-e", &cmd2, "-e", "12q", LINES1]) + .succeeds(); + + let mut actual = String::new(); + temp.reopen()?.read_to_string(&mut actual)?; + + let expected = fs::read_to_string("tests/fixtures/sed/output/write_single_file")?; + assert_eq!(actual, expected, "Output did not match fixture"); + + Ok(()) +} + #[test] fn write_single_file_no_newline() -> std::io::Result<()> { let temp = NamedTempFile::new()?;