Handle non-UTF8 encodings better when preparing unified diffs for the frontend (#10475)

Author: Byron

Cargo.lock

@@ -28,6 +28,7 @@ gitbutler-serde.workspace = true
 gitbutler-error.workspace = true
 uuid.workspace = true
 toml.workspace = true
+chardetng = "0.1.17"
 
 [dev-dependencies]
 but-testsupport.workspace = true

crates/but-core/Cargo.toml

@@ -30,6 +30,9 @@ pub struct DiffHunk {
     ///
     /// The line separator is the one used in the original file and may be `LF` or `CRLF`.
     /// Note that the file-portion of the header isn't used here.
+    ///
+    /// Also note that this has possibly been decoded lossily, assuming UTF8 if the encoding couldn't be determined,
+    /// replacing invalid codepoints with markers.
     #[serde(serialize_with = "gitbutler_serde::bstring_lossy::serialize")]
     pub diff: BString,
 }
@@ -181,7 +184,7 @@ impl UnifiedDiff {
                                 let mut buf = Vec::with_capacity(header_str.len() + hunk.len());
                                 buf.extend_from_slice(header_str.as_bytes());
                                 buf.extend_from_slice(hunk);
-                                buf.into()
+                                detect_and_convert_to_utf8(buf.into())
                             },
                         });
                         Ok(())
@@ -234,6 +237,22 @@ impl UnifiedDiff {
     }
 }
 
+/// Detect the encoding of the given byte content and convert it to UTF-8, after attempting to guess its encoding.
+/// Even if decoding failed, we always return the original `content` in the wirst case.
+fn detect_and_convert_to_utf8(content: BString) -> BString {
+    // Use chardet to detect the encoding
+    let mut detect = chardetng::EncodingDetector::new();
+    detect.feed(&content, true);
+    let (encoding, high_confidence) = detect.guess_assess(None, true);
+    if encoding.name() == "UTF-8" || !high_confidence {
+        return content;
+    }
+
+    // Convert to UTF-8
+    let (decoded, _actual_encoding, _used_replacement_chars) = encoding.decode(&content);
+    decoded.into_owned().into()
+}
+
 fn compute_line_changes(hunks: &Vec<DiffHunk>) -> (u32, u32) {
     let mut lines_added = 0;
     let mut lines_removed = 0;

crates/but-core/src/unified_diff.rs

@@ -967,6 +967,45 @@ fn added_modified_in_worktree() -> Result<()> {
     Ok(())
 }
 
+#[test]
+fn non_utf8_decoding() -> Result<()> {
+    let repo = repo("non-utf8-encodings")?;
+    let actual = diff::worktree_changes(&repo)?;
+    // Let's have one sample file per codepage with reasonable amounts of text for inference.
+    insta::assert_debug_snapshot!(actual, @r#"
+    WorktreeChanges {
+        changes: [
+            TreeChange {
+                path: "windows1252",
+                status: Addition {
+                    state: ChangeState {
+                        id: Sha1(0000000000000000000000000000000000000000),
+                        kind: Blob,
+                    },
+                    is_untracked: true,
+                },
+            },
+        ],
+        ignored_changes: [],
+    }
+    "#);
+    insta::assert_debug_snapshot!(unified_diffs(actual, &repo)?, @r#"
+    [
+        Patch {
+            hunks: [
+                DiffHunk("@@ -1,0 +1,1 @@
+                +€ÄÀ
+                "),
+            ],
+            is_result_of_binary_to_text_conversion: false,
+            lines_added: 1,
+            lines_removed: 0,
+        },
+    ]
+    "#);
+    Ok(())
+}
+
 #[test]
 fn modified_in_index() -> Result<()> {
     let repo = repo("modified-in-index")?;

crates/but-core/tests/core/diff/worktree_changes.rs

@@ -310,3 +310,8 @@ cat <<EOF >>.git/config
 	textconv = "shift; echo ho"
 EOF
 )
+
+git init non-utf8-encodings
+(cd non-utf8-encodings
+  printf '\x80\xc4\xc0' > windows1252
+)

crates/but-core/tests/fixtures/worktree-changes.sh

@@ -281,7 +281,6 @@ pub async fn handle_stop(nightly: bool) -> anyhow::Result<CursorHookOutput> {
         }
     }
 
-    // dbg!(input);
     Ok(CursorHookOutput::default())
 }