Handle a mode change on a renamed file. (#875)

* Handle a mode change on a renamed file.

I changed the diff parsing to cache the mode info from the old/new mode
lines until the parsing bumps into the start of the actual related diff,
finds the diff line for an unrelated diff, or runs out of input.  This
allows the mode info to be output in conjunction with a file event
instead of as a separate heading (that used to have an empty name when
the mode change was for a rename).

The mode info is passed down into the draw routines as a separate
"addendum" string that the draw code can use as it likes. Currently that
means that it appends the addendum string to the non-raw string in
parens. I imagine that future draw routines could decide to put it in a
separate box or some other per-routine method. There is currently a
single function in src/handlers/draw.rs that joins the strings in the
same way for everyone.

A couple examples of how the new code looks:

Δ foo.rs (mode +x)
───────────────────────────────────────────────────

renamed: old-longer-name → shorter-name (mode +x)
───────────────────────────────────────────────────

Would it look better on its own line?

Δ foo.rs
• mode +x
───────────────────────────────────────────────────

renamed: old-longer-name → shorter-name
• mode +x
───────────────────────────────────────────────────

Would it look better appended after a "•" character?

Δ foo.rs • mode +x
───────────────────────────────────────────────────

renamed: old-longer-name → shorter-name • mode +x
───────────────────────────────────────────────────

Should it be a user option? If so, we can do that later.

9 files changed, 156 insertions, 71 deletions

diff --git a/src/delta.rs b/src/delta.rs
index 35ecfae3..7ba29973 100644
--- a/src/delta.rs
+++ b/src/delta.rs
@@ -97,6 +97,7 @@ pub struct StateMachine<'a> {
     pub minus_file_event: handlers::diff_header::FileEvent,
     pub plus_file_event: handlers::diff_header::FileEvent,
     pub diff_line: String,
+    pub mode_info: String,
     pub painter: Painter<'a>,
     pub config: &'a Config,
 
@@ -129,6 +130,7 @@ impl<'a> StateMachine<'a> {
             minus_file_event: handlers::diff_header::FileEvent::NoEvent,
             plus_file_event: handlers::diff_header::FileEvent::NoEvent,
             diff_line: "".to_string(),
+            mode_info: "".to_string(),
             current_file_pair: None,
             handled_diff_header_header_line_file_pair: None,
             painter: Painter::new(writer, config),
@@ -158,6 +160,7 @@ impl<'a> StateMachine<'a> {
                 || self.handle_diff_header_minus_line()?
                 || self.handle_diff_header_plus_line()?
                 || self.handle_hunk_header_line()?
+                || self.handle_diff_header_mode_line()?
                 || self.handle_diff_header_misc_line()?
                 || self.handle_submodule_log_line()?
                 || self.handle_submodule_short_line()?
@@ -170,6 +173,7 @@ impl<'a> StateMachine<'a> {
                 || self.emit_line_unchanged()?;
         }
 
+        self.handle_pending_mode_line_with_diff_name()?;
         self.painter.paint_buffered_minus_and_plus_lines();
         self.painter.emit()?;
         Ok(())
diff --git a/src/handlers/commit_meta.rs b/src/handlers/commit_meta.rs
index 2ad59bf1..98bdfad7 100644
--- a/src/handlers/commit_meta.rs
+++ b/src/handlers/commit_meta.rs
@@ -50,6 +50,7 @@ impl<'a> StateMachine<'a> {
             self.painter.writer,
             &format!("{}{}", formatted_line, if pad { " " } else { "" }),
             &format!("{}{}", formatted_raw_line, if pad { " " } else { "" }),
+            "",
             &self.config.decorations_width,
             self.config.commit_style,
             decoration_ansi_term_style,
diff --git a/src/handlers/diff_header.rs b/src/handlers/diff_header.rs
index f90c6538..e3b29a34 100644
--- a/src/handlers/diff_header.rs
+++ b/src/handlers/diff_header.rs
@@ -17,20 +17,47 @@ pub enum FileEvent {
     Change,
     Copy,
     Rename,
-    ModeChange(String),
     NoEvent,
 }
 
 impl<'a> StateMachine<'a> {
+    /// Check for the old mode|new mode lines and cache their info for later use.
+    pub fn handle_diff_header_mode_line(&mut self) -> std::io::Result<bool> {
+        let mut handled_line = false;
+        if let Some(line_suf) = self.line.strip_prefix("old mode ") {
+            self.state = State::DiffHeader(DiffType::Unified);
+            if self.should_handle() && !self.config.color_only {
+                self.mode_info = line_suf.to_string();
+                handled_line = true;
+            }
+        } else if let Some(line_suf) = self.line.strip_prefix("new mode ") {
+            self.state = State::DiffHeader(DiffType::Unified);
+            if self.should_handle() && !self.config.color_only && !self.mode_info.is_empty() {
+                self.mode_info = match (self.mode_info.as_str(), line_suf) {
+                    // 100755 for executable and 100644 for non-executable are the only file modes Git records.
+                    // https://medium.com/@tahteche/how-git-treats-changes-in-file-permissions-f71874ca239d
+                    ("100644", "100755") => "mode +x".to_string(),
+                    ("100755", "100644") => "mode -x".to_string(),
+                    _ => format!(
+                        "mode {} {} {}",
+                        self.mode_info, self.config.right_arrow, line_suf
+                    ),
+                };
+                handled_line = true;
+            }
+        }
+        Ok(handled_line)
+    }
+
     #[inline]
     fn test_diff_header_minus_line(&self) -> bool {
         (matches!(self.state, State::DiffHeader(_)) || self.source == Source::DiffUnified)
             && (self.line.starts_with("--- ")
                 || self.line.starts_with("rename from ")
-                || self.line.starts_with("copy from ")
-                || self.line.starts_with("old mode "))
+                || self.line.starts_with("copy from "))
     }
 
+    /// Check for and handle the "--- filename ..." line.
     pub fn handle_diff_header_minus_line(&mut self) -> std::io::Result<bool> {
         if !self.test_diff_header_minus_line() {
             return Ok(false);
@@ -46,14 +73,7 @@ impl<'a> StateMachine<'a> {
                 None
             },
         );
-        // In the case of ModeChange only, the file path is taken from the diff
-        // --git line (since that is the only place the file path occurs);
-        // otherwise it is taken from the --- / +++ line.
-        self.minus_file = if let FileEvent::ModeChange(_) = &file_event {
-            get_repeated_file_path_from_diff_line(&self.diff_line).unwrap_or(path_or_mode)
-        } else {
-            path_or_mode
-        };
+        self.minus_file = path_or_mode;
         self.minus_file_event = file_event;
 
         if self.source == Source::DiffUnified {
@@ -76,6 +96,7 @@ impl<'a> StateMachine<'a> {
                 &self.line,
                 &self.raw_line,
                 &mut self.painter,
+                &mut self.mode_info,
                 self.config,
             )?;
             handled_line = true;
@@ -88,10 +109,10 @@ impl<'a> StateMachine<'a> {
         (matches!(self.state, State::DiffHeader(_)) || self.source == Source::DiffUnified)
             && (self.line.starts_with("+++ ")
                 || self.line.starts_with("rename to ")
-                || self.line.starts_with("copy to ")
-                || self.line.starts_with("new mode "))
+                || self.line.starts_with("copy to "))
     }
 
+    /// Check for and handle the "+++ filename ..." line.
     pub fn handle_diff_header_plus_line(&mut self) -> std::io::Result<bool> {
         if !self.test_diff_header_plus_line() {
             return Ok(false);
@@ -106,14 +127,7 @@ impl<'a> StateMachine<'a> {
                 None
             },
         );
-        // In the case of ModeChange only, the file path is taken from the diff
-        // --git line (since that is the only place the file path occurs);
-        // otherwise it is taken from the --- / +++ line.
-        self.plus_file = if let FileEvent::ModeChange(_) = &file_event {
-            get_repeated_file_path_from_diff_line(&self.diff_line).unwrap_or(path_or_mode)
-        } else {
-            path_or_mode
-        };
+        self.plus_file = path_or_mode;
         self.plus_file_event = file_event;
         self.painter
             .set_syntax(get_file_extension_from_diff_header_line_file_path(
@@ -130,6 +144,7 @@ impl<'a> StateMachine<'a> {
                 &self.line,
                 &self.raw_line,
                 &mut self.painter,
+                &mut self.mode_info,
                 self.config,
             )?;
             handled_line = true
@@ -154,7 +169,45 @@ impl<'a> StateMachine<'a> {
             self.config,
         );
         // FIXME: no support for 'raw'
-        write_generic_diff_header_header_line(&line, &line, &mut self.painter, self.config)
+        write_generic_diff_header_header_line(
+            &line,
+            &line,
+            &mut self.painter,
+            &mut self.mode_info,
+            self.config,
+        )
+    }
+
+    pub fn handle_pending_mode_line_with_diff_name(&mut self) -> std::io::Result<()> {
+        if !self.mode_info.is_empty() {
+            let format_label = |label: &str| {
+                if !label.is_empty() {
+                    format!("{} ", label)
+                } else {
+                    "".to_string()
+                }
+            };
+            let format_file = |file| {
+                if self.config.hyperlinks {
+                    features::hyperlinks::format_osc8_file_hyperlink(file, None, file, self.config)
+                } else {
+                    Cow::from(file)
+                }
+            };
+            let label = format_label(&self.config.file_modified_label);
+            let name = get_repeated_file_path_from_diff_line(&self.diff_line)
+                .unwrap_or_else(|| "".to_string());
+            let line = format!("{}{}", label, format_file(&name));
+            write_generic_diff_header_header_line(
+                &line,
+                &line,
+                &mut self.painter,
+                &mut self.mode_info,
+                self.config,
+            )
+        } else {
+            Ok(())
+        }
     }
 }
 
@@ -163,6 +216,7 @@ pub fn write_generic_diff_header_header_line(
     line: &str,
     raw_line: &str,
     painter: &mut Painter,
+    mode_info: &mut String,
     config: &Config,
 ) -> std::io::Result<()> {
     // If file_style is "omit", we'll skip the process and print nothing.
@@ -181,10 +235,14 @@ pub fn write_generic_diff_header_header_line(
         painter.writer,
         &format!("{}{}", line, if pad { " " } else { "" }),
         &format!("{}{}", raw_line, if pad { " " } else { "" }),
+        mode_info,
         &config.decorations_width,
         config.file_style,
         decoration_ansi_term_style,
     )?;
+    if !mode_info.is_empty() {
+        mode_info.truncate(0);
+    }
     Ok(())
 }
 
@@ -239,18 +297,11 @@ fn parse_diff_header_line(
         line if line.starts_with("copy to ") => {
             (line[8..].to_string(), FileEvent::Copy) // "copy to ".len()
         }
-        line if line.starts_with("old mode ") => {
-            ("".to_string(), FileEvent::ModeChange(line[9..].to_string())) // "old mode ".len()
-        }
-        line if line.starts_with("new mode ") => {
-            ("".to_string(), FileEvent::ModeChange(line[9..].to_string())) // "new mode ".len()
-        }
         _ => ("".to_string(), FileEvent::NoEvent),
     };
 
     if let Some(base) = relative_path_base {
-        if let FileEvent::ModeChange(_) = file_event {
-        } else if let Some(relative_path) = pathdiff::diff_paths(&path_or_mode, base) {
+        if let Some(relative_path) = pathdiff::diff_paths(&path_or_mode, base) {
             if let Some(relative_path) = relative_path.to_str() {
                 path_or_mode = relative_path.to_owned();
             }
@@ -326,33 +377,6 @@ pub fn get_file_change_description_from_file_paths(
             }
         };
         match (minus_file, plus_file, minus_file_event, plus_file_event) {
-            (
-                minus_file,
-                plus_file,
-                FileEvent::ModeChange(old_mode),
-                FileEvent::ModeChange(new_mode),
-            ) if minus_file == plus_file => match (old_mode.as_str(), new_mode.as_str()) {
-                // 100755 for executable and 100644 for non-executable are the only file modes Git records.
-                // https://medium.com/@tahteche/how-git-treats-changes-in-file-permissions-f71874ca239d
-                ("100644", "100755") => format!(
-                    "{}{}: mode +x",
-                    format_label(&config.file_modified_label),
-                    format_file(plus_file)
-                ),
-                ("100755", "100644") => format!(
-                    "{}{}: mode -x",
-                    format_label(&config.file_modified_label),
-                    format_file(plus_file)
-                ),
-                _ => format!(
-                    "{}{}: {} {} {}",
-                    format_label(&config.file_modified_label),
-                    format_file(plus_file),
-                    old_mode,
-                    config.right_arrow,
-                    new_mode
-                ),
-            },
             (minus_file, plus_file, _, _) if minus_file == plus_file => format!(
                 "{}{}",
                 format_label(&config.file_modified_label),
@@ -368,8 +392,7 @@ pub fn get_file_change_description_from_file_paths(
                 format_label(&config.file_added_label),
                 format_file(plus_file)
             ),
-            // minus_file_event == plus_file_event, except in the ModeChange
-            // case above.
+            // minus_file_event == plus_file_event
             (minus_file, plus_file, file_event, _) => format!(
                 "{}{} {} {}",
                 format_label(match file_event {
@@ -549,8 +572,9 @@ mod tests {
             None
         );
         assert_eq!(
-        get_repeated_file_path_from_diff_line("diff --git a/.config/Code - Insiders/User/settings.json b/.config/Code - Insiders/User/settings.json"),
-        Some(".config/Code - Insiders/User/settings.json".to_string())
-    );
+            get_repeated_file_path_from_diff_line(
+                "diff --git a/.config/Code - Insiders/User/settings.json b/.config/Code - Insiders/User/settings.json"),
+            Some(".config/Code - Insiders/User/settings.json".to_string())
+        );
     }
 }
diff --git a/src/handlers/diff_header_diff.rs b/src/handlers/diff_header_diff.rs
index 71367166..fdd2d39a 100644
--- a/src/handlers/diff_header_diff.rs
+++ b/src/handlers/diff_header_diff.rs
@@ -22,6 +22,7 @@ impl<'a> StateMachine<'a> {
             } else {
                 State::DiffHeader(DiffType::Unified)
             };
+        self.handle_pending_mode_line_with_diff_name()?;
         self.handled_diff_header_header_line_file_pair = None;
         self.diff_line = self.line.clone();
         if !self.should_skip_line() {
diff --git a/src/handlers/draw.rs b/src/handlers/draw.rs
index 7f700b80..c1eb2236 100644
--- a/src/handlers/draw.rs
+++ b/src/handlers/draw.rs
@@ -5,8 +5,25 @@ use crate::ansi;
 use crate::cli::Width;
 use crate::style::{DecorationStyle, Style};
 
-pub type DrawFunction =
-    dyn FnMut(&mut dyn Write, &str, &str, &Width, Style, ansi_term::Style) -> std::io::Result<()>;
+fn paint_text(text_style: Style, text: &str, addendum: &str) -> String {
+    if addendum.is_empty() {
+        text_style.paint(text).to_string()
+    } else {
+        text_style
+            .paint(text.to_string() + " (" + addendum + ")")
+            .to_string()
+    }
+}
+
+pub type DrawFunction = dyn FnMut(
+    &mut dyn Write,
+    &str,
+    &str,
+    &str,
+    &Width,
+    Style,
+    ansi_term::Style,
+) -> std::io::Result<()>;
 
 pub fn get_draw_function(
     decoration_style: DecorationStyle,
@@ -39,6 +56,7 @@ fn write_no_decoration(
     writer: &mut dyn Write,
     text: &str,
     raw_text: &str,
+    addendum: &str,
     _line_width: &Width, // ignored
     text_style: Style,
     _decoration_style: ansi_term::Style,
@@ -46,7 +64,7 @@ fn write_no_decoration(
     if text_style.is_raw {
         writeln!(writer, "{}", raw_text)?;
     } else {
-        writeln!(writer, "{}", text_style.paint(text))?;
+        writeln!(writer, "{}", paint_text(text_style, text, addendum))?;
     }
     Ok(())
 }
@@ -57,6 +75,7 @@ pub fn write_boxed(
     writer: &mut dyn Write,
     text: &str,
     raw_text: &str,
+    addendum: &str,
     _line_width: &Width, // ignored
     text_style: Style,
     decoration_style: ansi_term::Style,
@@ -71,6 +90,7 @@ pub fn write_boxed(
         writer,
         text,
         raw_text,
+        addendum,
         box_width,
         text_style,
         decoration_style,
@@ -85,6 +105,7 @@ fn write_boxed_with_underline(
     writer: &mut dyn Write,
     text: &str,
     raw_text: &str,
+    addendum: &str,
     line_width: &Width,
     text_style: Style,
     decoration_style: ansi_term::Style,
@@ -94,6 +115,7 @@ fn write_boxed_with_underline(
         writer,
         text,
         raw_text,
+        addendum,
         box_width,
         text_style,
         decoration_style,
@@ -126,6 +148,7 @@ fn write_underlined(
     writer: &mut dyn Write,
     text: &str,
     raw_text: &str,
+    addendum: &str,
     line_width: &Width,
     text_style: Style,
     decoration_style: ansi_term::Style,
@@ -135,6 +158,7 @@ fn write_underlined(
         writer,
         text,
         raw_text,
+        addendum,
         line_width,
         text_style,
         decoration_style,
@@ -145,6 +169,7 @@ fn write_overlined(
     writer: &mut dyn Write,
     text: &str,
     raw_text: &str,
+    addendum: &str,
     line_width: &Width,
     text_style: Style,
     decoration_style: ansi_term::Style,
@@ -154,6 +179,7 @@ fn write_overlined(
         writer,
         text,
         raw_text,
+        addendum,
         line_width,
         text_style,
         decoration_style,
@@ -164,6 +190,7 @@ fn write_underoverlined(
     writer: &mut dyn Write,
     text: &str,
     raw_text: &str,
+    addendum: &str,
     line_width: &Width,
     text_style: Style,
     decoration_style: ansi_term::Style,
@@ -173,17 +200,20 @@ fn write_underoverlined(
         writer,
         text,
         raw_text,
+        addendum,
         line_width,
         text_style,
         decoration_style,
     )
 }
 
+#[allow(clippy::too_many_arguments)]
 fn _write_under_or_over_lined(
     underoverline: UnderOverline,
     writer: &mut dyn Write,
     text: &str,
     raw_text: &str,
+    addendum: &str,
     line_width: &Width,
     text_style: Style,
     decoration_style: ansi_term::Style,
@@ -206,7 +236,7 @@ fn _write_under_or_over_lined(
     if text_style.is_raw {
         writeln!(writer, "{}", raw_text)?;
     } else {
-        writeln!(writer, "{}", text_style.paint(text))?;
+        writeln!(writer, "{}", paint_text(text_style, text, addendum))?;
     }
     match underoverline {
         UnderOverline::Over => {}
@@ -237,6 +267,7 @@ fn write_boxed_with_horizontal_whisker(
     writer: &mut dyn Write,
     text: &str,
     raw_text: &str,
+    addendum: &str,
     box_width: usize,
     text_style: Style,
     decoration_style: ansi_term::Style,
@@ -250,6 +281,7 @@ fn write_boxed_with_horizontal_whisker(
         writer,
         text,
         raw_text,
+        addendum,
         box_width,
         text_style,
         decoration_style,
@@ -262,6 +294,7 @@ fn write_boxed_partial(
     writer: &mut dyn Write,
     text: &str,
     raw_text: &str,
+    addendum: &str,
     box_width: usize,
     text_style: Style,
     decoration_style: ansi_term::Style,
@@ -289,7 +322,7 @@ fn write_boxed_partial(
     if text_style.is_raw {
         write!(writer, "{}", raw_text)?;
     } else {
-        write!(writer, "{}", text_style.paint(text))?;
+        write!(writer, "{}", paint_text(text_style, text, addendum))?;
     }
     write!(
         writer,
diff --git a/src/handlers/hunk_header.rs b/src/handlers/hunk_header.rs
index 833bfc81..2900008f 100644
--- a/src/handlers/hunk_header.rs
+++ b/src/handlers/hunk_header.rs
@@ -194,6 +194,7 @@ fn write_hunk_header_raw(
         painter.writer,
         &format!("{}{}", line, if pad { " " } else { "" }),
         &format!("{}{}", raw_line, if pad { " " } else { "" }),
+        "",
         &config.decorations_width,
         config.hunk_header_style,
         decoration_ansi_term_style,
@@ -229,6 +230,7 @@ pub fn write_hunk_header(
             painter.writer,
             &painter.output_buffer,
             &painter.output_buffer,
+            "",
             &config.decorations_width,
             config.null_style,
             decoration_ansi_term_style,
diff --git a/src/handlers/merge_conflict.rs b/src/handlers/merge_conflict.rs
index 926d7a3c..31cb2bda 100644
--- a/src/handlers/merge_conflict.rs
+++ b/src/handlers/merge_conflict.rs
@@ -200,6 +200,7 @@ fn write_diff_header(
         painter.writer,
         &text,
         &text,
+        "",
         &config.decorations_width,
         style,
         decoration_ansi_term_style,
diff --git a/src/handlers/mod.rs b/src/handlers/mod.rs
index 1b719f8b..048da99d 100644
--- a/src/handlers/mod.rs
+++ b/src/handlers/mod.rs
@@ -42,6 +42,7 @@ impl<'a> StateMachine<'a> {
                 &self.line,
                 &self.raw_line,
                 &mut self.painter,
+                &mut self.mode_info,
                 self.config,
             )?;
             handled_line = true;
diff --git a/src/tests/test_example_diffs.rs b/src/tests/test_example_diffs.rs
index 6e0c542c..3ef649b8 100644
--- a/src/tests/test_example_diffs.rs
+++ b/src/tests/test_example_diffs.rs
@@ -1572,31 +1572,40 @@ src/align.rs:71: impl<'a> Alignment<'a> { │
     }
 
     #[test]
+    fn test_file_mode_change_with_rename() {
+        let config = integration_test_utils::make_config_from_args(&["--right-arrow=->"]);
+        let output =
+            integration_test_utils::run_delta(GIT_DIFF_FILE_MODE_CHANGE_WITH_RENAME, &config);
+        let output = strip_ansi_codes(&output);
+        assert!(output.contains("renamed: old-longer-name -> shorter-name (mode +x)"));
+    }
+
+    #[test]
     fn test_file_mode_change_gain_executable_bit() {
         DeltaTest::with_args(&[])
             .with_input(GIT_DIFF_FILE_MODE_CHANGE_GAIN_EXECUTABLE_BIT)
-            .expect_contains(r"src/delta.rs: mode +x");
+            .expect_contains("src/delta.rs (mode +x)");
     }
 
     #[test]
     fn test_file_mode_change_lose_executable_bit() {
         DeltaTest::with_args(&[])
             .with_input(GIT_DIFF_FILE_MODE_CHANGE_LOSE_EXECUTABLE_BIT)
-            .expect_contains(r"src/delta.rs: mode -x");
+            .expect_contains("src/delta.rs (mode -x)");
     }
 
     #[test]
     fn test_file_mode_change_unexpected_bits() {
         DeltaTest::with_args(&["--navigate", "--right-arrow=->"])
             .with_input(GIT_DIFF_FILE_MODE_CHANGE_UNEXPECTED_BITS)
-            .expect_contains(r"Δ src/delta.rs: 100700 -> 100644");
+            .expect_contains("Δ src/delta.rs (mode 100700 -> 100644)");
     }
 
     #[test]
     fn test_file_mode_change_with_diff() {
         DeltaTest::with_args(&["--navigate", "--keep-plus-minus-markers"])
             .with_input(GIT_DIFF_FILE_MODE_CHANGE_WITH_DIFF)
-            .expect_contains("Δ src/script: mode +x")
+            .expect_contains("Δ src/script (mode +x)")
             .expect_after_skip(
                 5,
                 "
@@ -2308,6 +2317,15 @@ Date:   Sun Nov 1 15:28:53 2020 -0500
 +]
 "#;
 
+    const GIT_DIFF_FILE_MODE_CHANGE_WITH_RENAME: &str = "
+diff --git a/old-longer-name b/shorter-name
+old mode 100644
+new mode 100755
+similarity index 100%
+rename from old-longer-name
+rename to shorter-name
+";
+
     const GIT_DIFF_FILE_MODE_CHANGE_GAIN_EXECUTABLE_BIT: &str = "
 diff --git a/src/delta.rs b/src/delta.rs
 old mode 100644