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

@@ -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
+/// Returns the path for the history file for the given `session_id`
-/// loaded. If `suffix` is provided, append that suffix to the path; this is used for temporary files.
+/// if `session_id` is non-empty.
-fn history_filename(session_id: &wstr, suffix: &wstr) -> Option<WString> {
+/// 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<Option<WString>> {
     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<WString>;
@@ -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!("")) {
+        if let Ok(Some(history_path)) = history_filename(&self.name, L!("")) {
-            let Ok(mut file) = wopen_cloexec(&filename, OFlag::O_RDONLY, Mode::empty()) else {
+            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
             );
-
+            Err(err)
-            false
         } 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 {
+        let (mut tmp_file, tmp_name) = create_temporary_file(&tmp_name_template)?;
-            return;
-        };
         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;
         };