h/fish-shell
1
Fork 0
mirror of https://github.com/fish-shell/fish-shell.git synced 2026-06-02 22:21:15 -03:00
Code Issues Packages Projects Releases Wiki Activity

Make control flow more explicit, log errors

Browse Source
This commit is contained in:
Daniel Rainer
2025-03-28 17:33:46 +01:00
committed by Fabian Boehm
parent 5c0fddae70
commit 9618a38215
1 changed files with 69 additions and 70 deletions
Download Patch File Download Diff File Expand all files Collapse all files

139
src/history.rs
View File

@@ -824,8 +824,7 @@ fn save_internal_via_rewrite(&mut self) {
}
/// Saves history by appending to the file.
// TODO: return error information
fn save_internal_via_appending(&mut self) -> bool {
fn save_internal_via_appending(&mut self) -> std::io::Result<()> {
FLOGF!(
history,
"Saving %lu items via appending",
@@ -834,31 +833,34 @@ fn save_internal_via_appending(&mut self) -> bool {
// No deleting allowed.
assert!(self.deleted_items.is_empty());
let mut ok = false;
// 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 {
// TODO: Why have a successful return here?
return true;
// 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.
// Limit our max tries so we don't do this forever.
let mut history_file = None;
for _i in 0..MAX_SAVE_TRIES {
let Ok(mut file) = wopen_cloexec(
let mut num_attempts = 0;
let mut history_file = loop {
if num_attempts == MAX_SAVE_TRIES {
return Err(std::io::Error::new(
std::io::ErrorKind::Other,
"Number of unsuccessful attempts to open history file exceeds maximum.",
));
}
// Return immediately if an error occurs here.
let mut file = wopen_cloexec(
&history_path,
OFlag::O_WRONLY | OFlag::O_APPEND,
Mode::empty(),
) else {
// can't open, we're hosed
break;
};
)?;
// Exclusive lock on the entire file. This is released when we close the file (below). This
// may fail on (e.g.) lockless NFS. If so, proceed as if it did not fail; the risk is that
@@ -875,72 +877,68 @@ fn save_internal_via_appending(&mut self) -> bool {
if file_id != self.history_file_id {
file_changed = true;
}
history_file = Some(file);
break;
break file;
}
}
num_attempts += 1;
};
if let Some(mut history_file) = history_file {
// We (hopefully successfully) took the exclusive lock. Append to the file.
// Note that this is sketchy for a few reasons:
// - Another shell may have appended its own items with a later timestamp, so our file may
// no longer be sorted by timestamp.
// - Another shell may have appended the same items, so our file may now contain
// duplicates.
//
// We cannot modify any previous parts of our file, because other instances may be reading
// those portions. We can only append.
//
// Originally we always rewrote the file on saving, which avoided both of these problems.
// However, appending allows us to save history after every command, which is nice!
//
// Periodically we "clean up" the file by rewriting it, so that most of the time it doesn't
// have duplicates, although we don't yet sort by timestamp (the timestamp isn't really used
// for much anyways).
// We (hopefully successfully) took the exclusive lock. Append to the file.
// Note that this is sketchy for a few reasons:
// - Another shell may have appended its own items with a later timestamp, so our file may
// no longer be sorted by timestamp.
// - Another shell may have appended the same items, so our file may now contain
// duplicates.
//
// We cannot modify any previous parts of our file, because other instances may be reading
// those portions. We can only append.
//
// Originally we always rewrote the file on saving, which avoided both of these problems.
// However, appending allows us to save history after every command, which is nice!
//
// Periodically we "clean up" the file by rewriting it, so that most of the time it doesn't
// have duplicates, although we don't yet sort by timestamp (the timestamp isn't really used
// for much anyways).
// 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.
// TODO: consider filling the buffer ahead of time, so we can just lock, splat, and unlock?
let mut res = Ok(());
// Use a small buffer size for appending, we usually only have 1 item
let mut buffer = Vec::new();
while self.first_unwritten_new_item_index < self.new_items.len() {
let item = &self.new_items[self.first_unwritten_new_item_index];
if item.should_write_to_disk() {
append_history_item_to_buffer(item, &mut buffer);
// Ensure that each item is written individually and makes it to storage.
// This is done to keep `self.first_unwritten_new_item_index` consistent with
// the file system.
// Flushing and syncing each iteration adds overhead,
// but hopefully there are not that many items to write when appending.
res = drain_buffer_into_file_and_flush(&mut buffer, &mut history_file);
if res.is_err() {
break;
}
// 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.
// TODO: consider filling the buffer ahead of time, so we can just lock, splat, and unlock?
let mut res = Ok(());
// Use a small buffer size for appending, we usually only have 1 item
let mut buffer = Vec::new();
while self.first_unwritten_new_item_index < self.new_items.len() {
let item = &self.new_items[self.first_unwritten_new_item_index];
if item.should_write_to_disk() {
append_history_item_to_buffer(item, &mut buffer);
// Ensure that each item is written individually and makes it to storage.
// This is done to keep `self.first_unwritten_new_item_index` consistent with
// the file system.
// Flushing and syncing each iteration adds overhead,
// but hopefully there are not that many items to write when appending.
res = drain_buffer_into_file_and_flush(&mut buffer, &mut history_file);
if res.is_err() {
break;
}
// We wrote or skipped this item, hooray.
self.first_unwritten_new_item_index += 1;
}
// Since we just modified the file, update our history_file_id to match its current state
// Otherwise we'll think the file has been changed by someone else the next time we go to
// write.
// We don't update the mapping since we only appended to the file, and everything we
// appended remains in our new_items
self.history_file_id = file_id_for_fd(history_file.as_fd());
ok = res.is_ok();
drop(history_file);
// We wrote or skipped this item, hooray.
self.first_unwritten_new_item_index += 1;
}
// Since we just modified the file, update our history_file_id to match its current state
// Otherwise we'll think the file has been changed by someone else the next time we go to
// write.
// We don't update the mapping since we only appended to the file, and everything we
// appended remains in our new_items
self.history_file_id = file_id_for_fd(history_file.as_fd());
drop(history_file);
// If someone has replaced the file, forget our file state.
if file_changed {
self.clear_file_state();
}
ok
res
}
/// Saves history.
@@ -967,9 +965,10 @@ fn save(&mut self, vacuum: bool) {
let mut ok = false;
if !vacuum && self.deleted_items.is_empty() {
// Try doing a fast append.
ok = self.save_internal_via_appending();
if !ok {
FLOG!(history, "Appending failed");
if let Err(e) = self.save_internal_via_appending() {
FLOG!(history, "Appending failed", e);
} else {
ok = true;
}
}
if !ok {