Fix headers of modified binary files, closes #1621 (#1629)

4 files changed, 89 insertions, 21 deletions

diff --git a/src/handlers/diff_header.rs b/src/handlers/diff_header.rs
index 334ca76c..3c71a61a 100644
--- a/src/handlers/diff_header.rs
+++ b/src/handlers/diff_header.rs
@@ -357,7 +357,7 @@ fn parse_diff_header_line(line: &str, git_diff_name: bool) -> (String, FileEvent
 
 /// Given input like "diff --git a/src/my file.rs b/src/my file.rs"
 /// return Some("src/my file.rs")
-fn get_repeated_file_path_from_diff_line(line: &str) -> Option<String> {
+pub fn get_repeated_file_path_from_diff_line(line: &str) -> Option<String> {
     if let Some(line) = line.strip_prefix("diff --git ") {
         let line: Vec<&str> = line.graphemes(true).collect();
         let midpoint = line.len() / 2;
diff --git a/src/handlers/diff_header_diff.rs b/src/handlers/diff_header_diff.rs
index a79f6e1c..f8a007e2 100644
--- a/src/handlers/diff_header_diff.rs
+++ b/src/handlers/diff_header_diff.rs
@@ -1,4 +1,5 @@
 use crate::delta::{DiffType, InMergeConflict, MergeParents, State, StateMachine};
+use crate::handlers::diff_header::{get_repeated_file_path_from_diff_line, FileEvent};
 
 impl<'a> StateMachine<'a> {
     #[inline]
@@ -25,6 +26,19 @@ impl<'a> StateMachine<'a> {
         self.handle_pending_line_with_diff_name()?;
         self.handled_diff_header_header_line_file_pair = None;
         self.diff_line = self.line.clone();
+
+        // Pre-fill header fields from the diff line. For added, removed or renamed files
+        // these are updated precisely on actual header minus and header plus lines.
+        // But for modified binary files which are not added, removed or renamed, there
+        // are no minus and plus lines. Without the code below, in such cases the file names
+        // would remain unchanged from the previous diff, or empty for the very first diff.
+        let name = get_repeated_file_path_from_diff_line(&self.line).unwrap_or_default();
+        self.minus_file = name.clone();
+        self.plus_file = name.clone();
+        self.minus_file_event = FileEvent::Change;
+        self.plus_file_event = FileEvent::Change;
+        self.current_file_pair = Some((self.minus_file.clone(), self.plus_file.clone()));
+
         if !self.should_skip_line() {
             self.emit_line_unchanged()?;
         }
diff --git a/src/handlers/diff_header_misc.rs b/src/handlers/diff_header_misc.rs
index 141767ae..c5e4026b 100644
--- a/src/handlers/diff_header_misc.rs
+++ b/src/handlers/diff_header_misc.rs
@@ -12,25 +12,26 @@ impl<'a> StateMachine<'a> {
     }
 
     pub fn handle_diff_header_misc_line(&mut self) -> std::io::Result<bool> {
-        let is_binary: bool = self.test_diff_is_binary();
-        let file_missing: bool = self.test_diff_file_missing();
-
-        if !file_missing && !is_binary {
+        if !self.test_diff_file_missing() && !self.test_diff_is_binary() {
             return Ok(false);
         }
 
-        if is_binary {
-            match (self.minus_file.as_str(), self.plus_file.as_str()) {
-                ("", "") => {
-                    return self.handle_additional_cases(match self.state {
-                        State::DiffHeader(_) => self.state.clone(),
-                        _ => State::DiffHeader(DiffType::Unified),
-                    });
-                }
-                ("/dev/null", _) => self.plus_file.push_str(" (binary file)"),
-                (_, "/dev/null") => self.minus_file.push_str(" (binary file)"),
-                (_, _) => (),
-            };
+        if self.test_diff_is_binary() {
+            // Print the "Binary files" line verbatim, if there was no "diff" line, or it
+            // listed different files but was not followed by header minus and plus lines.
+            // This can happen in output of standalone diff or git diff --no-index.
+            if self.minus_file.is_empty() && self.plus_file.is_empty() {
+                self.emit_line_unchanged()?;
+                self.handled_diff_header_header_line_file_pair = self.current_file_pair.clone();
+                return Ok(true);
+            }
+
+            if self.minus_file != "/dev/null" {
+                self.minus_file.push_str(" (binary file)");
+            }
+            if self.plus_file != "/dev/null" {
+                self.plus_file.push_str(" (binary file)");
+            }
             return Ok(true);
         }
 
diff --git a/src/tests/test_example_diffs.rs b/src/tests/test_example_diffs.rs
index 1b710429..68a88658 100644
--- a/src/tests/test_example_diffs.rs
+++ b/src/tests/test_example_diffs.rs
@@ -237,10 +237,11 @@ mod tests {
 
     #[test]
     fn test_binary_files_differ() {
-        let config = integration_test_utils::make_config_from_args(&[]);
+        let config =
+            integration_test_utils::make_config_from_args(&["--file-modified-label", "modified:"]);
         let output = integration_test_utils::run_delta(BINARY_FILES_DIFFER, &config);
         let output = strip_ansi_codes(&output);
-        assert!(output.contains("Binary files a/foo and b/foo differ\n"));
+        assert!(output.contains("\nmodified: foo (binary file)\n"));
     }
 
     #[test]
@@ -248,7 +249,7 @@ mod tests {
         let config = integration_test_utils::make_config_from_args(&[]);
         let output = integration_test_utils::run_delta(BINARY_FILE_ADDED, &config);
         let output = strip_ansi_codes(&output);
-        assert!(output.contains("added: foo (binary file)\n"));
+        assert!(output.contains("\nadded: foo (binary file)\n"));
     }
 
     #[test]
@@ -256,7 +257,17 @@ mod tests {
         let config = integration_test_utils::make_config_from_args(&[]);
         let output = integration_test_utils::run_delta(BINARY_FILE_REMOVED, &config);
         let output = strip_ansi_codes(&output);
-        assert!(output.contains("removed: foo (binary file)\n"));
+        assert!(output.contains("\nremoved: foo (binary file)\n"));
+    }
+
+    #[test]
+    fn test_binary_files_differ_after_other() {
+        let config =
+            integration_test_utils::make_config_from_args(&["--file-modified-label", "modified:"]);
+        let output = integration_test_utils::run_delta(BINARY_FILES_DIFFER_AFTER_OTHER, &config);
+        let output = strip_ansi_codes(&output);
+        assert!(output.contains("\nrenamed: foo ⟶   bar\n"));
+        assert!(output.contains("\nmodified: qux (binary file)\n"));
     }
 
     #[test]
@@ -269,6 +280,32 @@ mod tests {
     }
 
     #[test]
+    fn test_standalone_diff_files_are_identical() {
+        let diff = "Files foo and bar are identical\n";
+        let config = integration_test_utils::make_config_from_args(&[]);
+        let output = integration_test_utils::run_delta(diff, &config);
+        assert_eq!(strip_ansi_codes(&output), diff);
+    }
+
+    #[test]
+    fn test_standalone_diff_binary_files_differ() {
+        let diff = "Binary files foo and bar differ\n";
+        let config = integration_test_utils::make_config_from_args(&[]);
+        let output = integration_test_utils::run_delta(diff, &config);
+        assert_eq!(strip_ansi_codes(&output), diff);
+    }
+
+    #[test]
+    fn test_diff_no_index_binary_files_differ() {
+        let config = integration_test_utils::make_config_from_args(&[]);
+        let output = integration_test_utils::run_delta(DIFF_NO_INDEX_BINARY_FILES_DIFFER, &config);
+        assert_eq!(
+            strip_ansi_codes(&output),
+            "Binary files foo bar and sub dir/foo bar baz differ\n"
+        );
+    }
+
+    #[test]
     fn test_commit_style_raw_no_decoration() {
         let config = integration_test_utils::make_config_from_args(&[
             "--commit-style",
@@ -2309,6 +2346,22 @@ index c9bbb35..5fc172d 100644
 Binary files a/foo and /dev/null differ
 ";
 
+    const BINARY_FILES_DIFFER_AFTER_OTHER: &str = "
+diff --git a/foo b/bar
+similarity index 100%
+rename from foo
+rename to bar
+diff --git a/qux b/qux
+index 00de669..d47cd84 100644
+Binary files a/qux and b/qux differ
+";
+
+    const DIFF_NO_INDEX_BINARY_FILES_DIFFER: &str = "\
+diff --git foo bar sub dir/foo bar baz
+index 329fbf5..481817c 100644
+Binary files foo bar and sub dir/foo bar baz differ
+";
+
     const GIT_DIFF_WITH_COPIED_FILE: &str = "
 commit f600ed5ced4d98295ffa97571ed240cd86c34ac6 (HEAD -> master)
 Author: Dan Davison <dandavison7@gmail.com>