From fbb2fcdb068f82dedee77cd3d0dcda4c5d7cfc19 Mon Sep 17 00:00:00 2001 From: Daniel Rainer Date: Mon, 26 May 2025 00:25:55 +0200 Subject: [PATCH] Improve error handling in `src/history.rs` This allows for more informative error handling. In some cases, the `?` operator can now be applied sensibly instead of more verbose local error handling. --- src/history.rs | 94 ++++++++++++++++++++++++++++---------------------- 1 file changed, 53 insertions(+), 41 deletions(-) diff --git a/src/history.rs b/src/history.rs index 85eb4a9a4..50ec6c76a 100644 --- a/src/history.rs +++ b/src/history.rs @@ -215,20 +215,27 @@ fn add_item(&mut self, item: HistoryItem) { } } -/// Returns the path for the history file for the given `session_id`, or `None` if it could not be -/// loaded. If `suffix` is provided, append that suffix to the path; this is used for temporary files. -fn history_filename(session_id: &wstr, suffix: &wstr) -> Option { +/// Returns the path for the history file for the given `session_id` +/// if `session_id` is non-empty. +/// If it is empty, `Ok(None)` will be returned. +/// An error is returned if obtaining the data directory failed. +/// Because the `path_get_data` function does not return error information, +/// we cannot provide more detail about the reason for the failure here. +/// If `suffix` is provided, append that suffix to the path; this is used for temporary files. +fn history_filename(session_id: &wstr, suffix: &wstr) -> std::io::Result> { if session_id.is_empty() { - return None; + return Ok(None); } - let mut result = path_get_data()?; + let Some(mut result) = path_get_data() else { + return Err(std::io::Error::new(std::io::ErrorKind::NotFound, "Error obtaining data directory. This is a manually constructed error which does not indicate why this happened.")); + }; result.push('/'); result.push_utfstr(session_id); result.push_utfstr(L!("_history")); result.push_utfstr(suffix); - Some(result) + Ok(Some(result)) } pub type PathList = Vec; @@ -495,8 +502,8 @@ fn load_old_if_needed(&mut self) { self.loaded_old = true; let _profiler = TimeProfiler::new("load_old"); - if let Some(filename) = history_filename(&self.name, L!("")) { - let Ok(mut file) = wopen_cloexec(&filename, OFlag::O_RDONLY, Mode::empty()) else { + if let Ok(Some(history_path)) = history_filename(&self.name, L!("")) { + let Ok(mut file) = wopen_cloexec(&history_path, OFlag::O_RDONLY, Mode::empty()) else { return; }; @@ -572,7 +579,11 @@ fn remove_ephemeral_items(&mut self) { /// Given an existing history file, write a new history file to `dst`. /// Returns false on error, true on success - fn rewrite_to_temporary_file(&self, existing_file: Option<&mut File>, dst: &mut File) -> bool { + fn rewrite_to_temporary_file( + &self, + existing_file: Option<&mut File>, + dst: &mut File, + ) -> std::io::Result<()> { // We are reading FROM existing_file and writing TO dst // Make an LRU cache to save only the last N elements. @@ -650,39 +661,34 @@ fn rewrite_to_temporary_file(&self, existing_file: Option<&mut File>, dst: &mut "Error writing to temporary history file:", err ); - - false + Err(err) } else { - true + Ok(()) } } /// Saves history by rewriting the file. - fn save_internal_via_rewrite(&mut self) { + fn save_internal_via_rewrite(&mut self) -> std::io::Result<()> { + // We want to rewrite the file, while holding the lock for as briefly as possible + // To do this, we speculatively write a file, and then lock and see if our original file changed + // Repeat until we succeed or give up + let Some(possibly_indirect_target_name) = history_filename(&self.name, L!(""))? else { + return Ok(()); + }; + let tmp_name_template = history_filename(&self.name, L!(".XXXXXX"))?.unwrap(); + FLOGF!( history, "Saving %lu items via rewrite", self.new_items.len() - self.first_unwritten_new_item_index ); - // We want to rewrite the file, while holding the lock for as briefly as possible - // To do this, we speculatively write a file, and then lock and see if our original file changed - // Repeat until we succeed or give up - let Some(possibly_indirect_target_name) = history_filename(&self.name, L!("")) else { - return; - }; - let Some(tmp_name_template) = history_filename(&self.name, L!(".XXXXXX")) else { - return; - }; - // If the history file is a symlink, we want to rewrite the real file so long as we can find it. let target_name = wrealpath(&possibly_indirect_target_name).unwrap_or(possibly_indirect_target_name); // Make our temporary file - let Ok((mut tmp_file, tmp_name)) = create_temporary_file(&tmp_name_template) else { - return; - }; + let (mut tmp_file, tmp_name) = create_temporary_file(&tmp_name_template)?; let mut done = false; for _i in 0..MAX_SAVE_TRIES { if done { @@ -704,8 +710,11 @@ fn save_internal_via_rewrite(&mut self) { .unwrap_or(INVALID_FILE_ID); // Open any target file, but do not lock it right away - if !self.rewrite_to_temporary_file(target_file_before.ok().as_mut(), &mut tmp_file) { + if let Err(err) = + self.rewrite_to_temporary_file(target_file_before.ok().as_mut(), &mut tmp_file) + { // Failed to write, no good + FLOG!(history_file, "Error writing to temporary file:", err); break; } @@ -808,10 +817,17 @@ fn save_internal_via_rewrite(&mut self) { // file. self.clear_file_state(); } + Ok(()) } /// Saves history by appending to the file. fn save_internal_via_appending(&mut self) -> std::io::Result<()> { + // Get the path to the real history file. + let Some(history_path) = history_filename(&self.name, L!(""))? else { + return Ok(()); + }; + let history_path = wrealpath(&history_path).unwrap_or(history_path); + FLOGF!( history, "Saving %lu items via appending", @@ -823,13 +839,6 @@ fn save_internal_via_appending(&mut self) -> std::io::Result<()> { // If the file is different (someone vacuumed it) then we need to update our mmap. let mut file_changed = false; - // Get the path to the real history file. - let Some(history_path) = history_filename(&self.name, L!("")) else { - // No history should be saved if fish_history is set to the empty string, - // so nothing needs to be done here. - return Ok(()); - }; - // We are going to open the file, lock it, append to it, and then close it // After locking it, we need to stat the file at the path; if there is a new file there, it // means the file was replaced and we have to try again. @@ -937,7 +946,7 @@ fn save(&mut self, vacuum: bool) { return; } - if history_filename(&self.name, L!("")).is_none() { + if self.name.is_empty() { // We're in the "incognito" mode. Pretend we've saved the history. self.first_unwritten_new_item_index = self.new_items.len(); self.deleted_items.clear(); @@ -953,14 +962,16 @@ fn save(&mut self, vacuum: bool) { if !vacuum && self.deleted_items.is_empty() { // Try doing a fast append. if let Err(e) = self.save_internal_via_appending() { - FLOG!(history, "Appending failed", e); + FLOG!(history, "Appending to history failed", e); } else { ok = true; } } if !ok { // We did not or could not append; rewrite the file ("vacuum" it). - self.save_internal_via_rewrite(); + if let Err(e) = self.save_internal_via_rewrite() { + FLOG!(history, "Rewriting history failed:", e) + } } } @@ -971,7 +982,7 @@ fn save_unless_disabled(&mut self) { return; } - // We may or may not vacuum. We try to vacuum every kVacuumFrequency items, but start the + // We may or may not vacuum. We try to vacuum every `VACUUM_FREQUENCY` items, but start the // countdown at a random number so that even if the user never runs more than 25 commands, we'll // eventually vacuum. If countdown_to_vacuum is None, it means we haven't yet picked a value for // the counter. @@ -1036,7 +1047,8 @@ fn is_empty(&mut self) -> bool { } else { // If we have not loaded old items, don't actually load them (which may be expensive); just // stat the file and see if it exists and is nonempty. - let Some(where_) = history_filename(&self.name, L!("")) else { + + let Ok(Some(where_)) = history_filename(&self.name, L!("")) else { return true; }; @@ -1092,7 +1104,7 @@ fn clear(&mut self) { self.deleted_items.clear(); self.first_unwritten_new_item_index = 0; self.old_item_offsets.clear(); - if let Some(filename) = history_filename(&self.name, L!("")) { + if let Ok(Some(filename)) = history_filename(&self.name, L!("")) { wunlink(&filename); } self.clear_file_state(); @@ -1113,7 +1125,7 @@ fn clear_session(&mut self) { /// file to the new history file. /// The new contents will automatically be re-mapped later. fn populate_from_config_path(&mut self) { - let Some(new_file) = history_filename(&self.name, L!("")) else { + let Ok(Some(new_file)) = history_filename(&self.name, L!("")) else { return; };