diff --git a/src/history/file.rs b/src/history/file.rs index b0c2ffcf8..2ffc004c2 100644 --- a/src/history/file.rs +++ b/src/history/file.rs @@ -2,7 +2,7 @@ use std::{ fs::File, - io::{Read, Write}, + io::Read, ops::{Deref, DerefMut}, os::fd::AsRawFd, time::{SystemTime, UNIX_EPOCH}, @@ -283,27 +283,30 @@ fn try_from(region: MmapRegion) -> std::io::Result { } } -/// Append a history item to a buffer, in preparation for outputting it to the history file. -pub fn append_history_item_to_buffer(item: &HistoryItem, buffer: &mut Vec) { - assert!(item.should_write_to_disk(), "Item should not be persisted"); +impl HistoryItem { + /// Write this history item to some writer. + pub fn write_to(&self, writer: &mut impl std::io::Write) -> std::io::Result<()> { + assert!(self.should_write_to_disk(), "Item should not be persisted"); - let mut cmd = wcs2bytes(item.str()); - escape_yaml_fish_2_0(&mut cmd); - buffer.extend(b"- cmd: "); - buffer.extend(&cmd); - buffer.push(b'\n'); - writeln!(buffer, " when: {}", time_to_seconds(item.timestamp())).unwrap(); + let mut cmd = wcs2bytes(self.str()); + escape_yaml_fish_2_0(&mut cmd); + writer.write_all(b"- cmd: ")?; + writer.write_all(&cmd)?; + writer.write_all(b"\n")?; + writeln!(writer, " when: {}", time_to_seconds(self.timestamp()))?; - let paths = item.get_required_paths(); - if !paths.is_empty() { - writeln!(buffer, " paths:").unwrap(); - for path in paths { - let mut path = wcs2bytes(path); - escape_yaml_fish_2_0(&mut path); - buffer.extend(b" - "); - buffer.extend(&path); - buffer.push(b'\n'); + let paths = self.get_required_paths(); + if !paths.is_empty() { + writeln!(writer, " paths:")?; + for path in paths { + let mut path = wcs2bytes(path); + escape_yaml_fish_2_0(&mut path); + writer.write_all(b" - ")?; + writer.write_all(&path)?; + writer.write_all(b"\n")?; + } } + Ok(()) } } diff --git a/src/history/history.rs b/src/history/history.rs index fee3f3ac9..8565b9692 100644 --- a/src/history/history.rs +++ b/src/history/history.rs @@ -29,7 +29,7 @@ collections::{BTreeMap, HashMap, HashSet}, ffi::CString, fs::File, - io::{BufRead, Read, Write}, + io::{BufRead, BufWriter, Read, Write}, mem::MaybeUninit, num::NonZeroUsize, ops::ControlFlow, @@ -50,7 +50,7 @@ fds::wopen_cloexec, flog::{flog, flogf}, fs::fsync, - history::file::{HistoryFile, RawHistoryFile, append_history_item_to_buffer}, + history::file::{HistoryFile, RawHistoryFile}, io::IoStreams, localization::wgettext_fmt, operation_context::{EXPANSION_LIMIT_BACKGROUND, OperationContext}, @@ -109,16 +109,6 @@ pub enum SearchDirection { pub const VACUUM_FREQUENCY: usize = 25; -/// Output the contents `buffer` to `file` and clear the `buffer`. -fn flush_to_file(buffer: &mut Vec, file: &mut File, min_size: usize) -> std::io::Result<()> { - if buffer.is_empty() || buffer.len() < min_size { - return Ok(()); - } - file.write_all(buffer)?; - buffer.clear(); - Ok(()) -} - struct TimeProfiler { what: &'static str, start: SystemTime, @@ -551,30 +541,21 @@ fn rewrite_to_temporary_file( /// Default buffer size for flushing to the history file. const HISTORY_OUTPUT_BUFFER_SIZE: usize = 64 * 1024; // Write them out. - let mut err = None; - let mut buffer = Vec::with_capacity(HISTORY_OUTPUT_BUFFER_SIZE + 128); - for item in items { - append_history_item_to_buffer(&item, &mut buffer); - if let Err(e) = flush_to_file(&mut buffer, dst, HISTORY_OUTPUT_BUFFER_SIZE) { - err = Some(e); - break; + let result: Result<(), std::io::Error> = { + let mut buffer = BufWriter::with_capacity(HISTORY_OUTPUT_BUFFER_SIZE + 128, dst); + for item in items { + item.write_to(&mut buffer)?; } - } - if err.is_none() { - if let Err(e) = flush_to_file(&mut buffer, dst, 0) { - err = Some(e); - } - } - if let Some(err) = err { + buffer.flush() + }; + if let Err(err) = &result { flog!( history_file, "Error writing to temporary history file:", err ); - Err(err) - } else { - Ok(()) } + result } /// Saves history by rewriting the file. @@ -646,19 +627,20 @@ fn save_internal_via_appending(&mut self, history_path: &wstr) -> std::io::Resul // So far so good. Write all items at or after first_unwritten_new_item_index. Note that we // write even a pending item - pending items are ignored by history within the command // itself, but should still be written to the file. - // Use a small buffer size for appending, we usually only have 1 item + // Use a small buffer size for appending, as we usually only have 1 item. + // Buffer everything and then write it all at once to avoid tearing writes (O_APPEND). let mut buffer = Vec::new(); let mut new_first_index = self.first_unwritten_new_item_index; while new_first_index < self.new_items.len() { let item = &self.new_items[new_first_index]; if item.should_write_to_disk() { - append_history_item_to_buffer(item, &mut buffer); + // Can't error writing to a buffer. + item.write_to(&mut buffer).unwrap(); } // We wrote or skipped this item, hooray. new_first_index += 1; } - - flush_to_file(&mut buffer, locked_history_file.get_mut(), 0)?; + locked_history_file.get_mut().write_all(&buffer)?; fsync(locked_history_file.get())?; self.first_unwritten_new_item_index = new_first_index;