fix: don't fail importing unreadable history lines (reubeno#710)

Don't treat unparseable lines in history files as fatal.

Author: reubeno

brush-core/src/history.rs

@@ -22,7 +22,9 @@ pub struct History {
 
 impl History {
     /// Constructs a new `History` instance, with its contents initialized from the given readable
-    /// stream.
+    /// stream. If errors are encountered reading lines from the stream, unreadable lines will
+    /// be skipped but the call will still return successfully, with a warning logged. An error
+    /// result will be returned only if an internal error occurs updating the history.
     ///
     /// # Arguments
     ///
@@ -33,9 +35,18 @@ impl History {
         let buf_reader = std::io::BufReader::new(reader);
 
         let mut next_timestamp = None;
-        for line in buf_reader.lines() {
-            let line = line?;
+        for line_result in buf_reader.lines() {
+            // If we couldn't decode the line (perhaps it wasn't valid UTF8?), skip it and make
+            // a best-effort attempt to proceed on. We'll later warn the user.
+            let line = match line_result {
+                Ok(line) => line,
+                Err(err) => {
+                    tracing::warn!("unreadable history line; {err}");
+                    continue;
+                }
+            };
 
+            // Look for timestamp comments; ignore other comment lines.
             if let Some(comment) = line.strip_prefix("#") {
                 if let Ok(seconds_since_epoch) = comment.trim().parse() {
                     next_timestamp =

brush-shell/tests/cases/builtins/history.yaml

@@ -185,3 +185,9 @@ cases:
       : cmd1
       : cmd2
       history
+
+  - name: "history file with unreadable lines"
+    ignore_stderr: true # Ignore brush's warnings about unreadable lines
+    stdin: |
+      bash -c 'printf "\xff\xfe\xfd" >history-file.txt'
+      HISTFILE=history-file.txt $0 -o history -c 'echo hi'

brush-shell/tests/compat_tests.rs

@@ -10,7 +10,6 @@ use clap::Parser;
 use colored::Colorize;
 use descape::UnescapeExt;
 use serde::{Deserialize, Serialize};
-use std::fs;
 #[cfg(unix)]
 use std::os::unix::{fs::PermissionsExt, process::ExitStatusExt};
 use std::{
@@ -19,6 +18,7 @@ use std::{
     path::{Path, PathBuf},
     process::ExitStatus,
 };
+use std::{fs, io::Read};
 
 #[derive(Clone)]
 struct ShellConfig {
@@ -1375,15 +1375,28 @@ fn diff_dirs(oracle_path: &Path, test_path: &Path) -> Result<DirComparison> {
         let test_file_path = test_path.join(filename);
 
         if file_type.is_file() {
-            let oracle_contents = fs::read_to_string(&oracle_file_path)?;
-            let test_contents = fs::read_to_string(&test_file_path)?;
+            let mut oracle_file = std::fs::OpenOptions::new()
+                .read(true)
+                .open(&oracle_file_path)?;
+            let mut oracle_bytes = vec![];
+            oracle_file.read_to_end(&mut oracle_bytes)?;
+
+            let mut test_file = std::fs::OpenOptions::new()
+                .read(true)
+                .open(&test_file_path)?;
+            let mut test_bytes = vec![];
+            test_file.read_to_end(&mut test_bytes)?;
+
+            if oracle_bytes != test_bytes {
+                // Convert using lossy conversion to avoid issues with invalid UTF-8.
+                let oracle_display_text = String::from_utf8_lossy(&oracle_bytes);
+                let test_display_text = String::from_utf8_lossy(&test_bytes);
 
-            if oracle_contents != test_contents {
                 entries.push(DirComparisonEntry::Different(
                     oracle_file_path,
-                    oracle_contents,
+                    oracle_display_text.to_string(),
                     test_file_path,
-                    test_contents,
+                    test_display_text.to_string(),
                 ));
             }
         } else if file_type.is_dir() {