Improve blame file type detection (#1829)

Move blame to CallingProcess

Signed-off-by: dvermd <315743+dvermd@users.noreply.github.com>

2 files changed, 203 insertions, 164 deletions

diff --git a/src/handlers/blame.rs b/src/handlers/blame.rs
index fc3d6d1..6c61810 100644
--- a/src/handlers/blame.rs
+++ b/src/handlers/blame.rs
@@ -14,7 +14,7 @@ use crate::format::{self, FormatStringSimple, Placeholder};
 use crate::format::{make_placeholder_regex, parse_line_number_format};
 use crate::paint::{self, BgShouldFill, StyleSectionSpecifier};
 use crate::style::Style;
-use crate::utils;
+use crate::utils::process;
 
 #[derive(Clone, Debug)]
 pub enum BlameLineNumbers {
@@ -76,9 +76,8 @@ impl<'a> StateMachine<'a> {
                 )?;
 
                 // Emit syntax-highlighted code
-                if matches!(self.state, State::Unknown) {
-                    self.painter
-                        .set_syntax(utils::process::git_blame_filename().as_deref());
+                if self.state == State::Unknown {
+                    self.painter.set_syntax(self.get_filename().as_deref());
                     self.painter.set_highlighter();
                 }
                 self.state = State::Blame(key);
@@ -94,6 +93,13 @@ impl<'a> StateMachine<'a> {
         Ok(handled_line)
     }
 
+    fn get_filename(&self) -> Option<String> {
+        match &*process::calling_process() {
+            process::CallingProcess::GitBlame(command_line) => command_line.last_arg.clone(),
+            _ => None,
+        }
+    }
+
     fn blame_metadata_style(
         &mut self,
         key: &str,
diff --git a/src/utils/process.rs b/src/utils/process.rs
index 6fa5ea9..f1e3c66 100644
--- a/src/utils/process.rs
+++ b/src/utils/process.rs
@@ -13,6 +13,7 @@ pub enum CallingProcess {
     GitShow(CommandLine, Option<String>), // element 2 is filename
     GitLog(CommandLine),
     GitReflog(CommandLine),
+    GitBlame(CommandLine),
     GitGrep(CommandLine),
     OtherGrep, // rg, grep, ag, ack, etc
     None,      // no matching process could be found
@@ -26,7 +27,9 @@ impl CallingProcess {
             CallingProcess::GitDiff(cmd) if cmd.long_options.contains("--relative") => true,
             CallingProcess::GitShow(cmd, _) if cmd.long_options.contains("--relative") => true,
             CallingProcess::GitLog(cmd) if cmd.long_options.contains("--relative") => true,
-            CallingProcess::GitGrep(_) | CallingProcess::OtherGrep => true,
+            CallingProcess::GitBlame(_)
+            | CallingProcess::GitGrep(_)
+            | CallingProcess::OtherGrep => true,
             _ => false,
         }
     }
@@ -36,7 +39,7 @@ impl CallingProcess {
 pub struct CommandLine {
     pub long_options: HashSet<String>,
     pub short_options: HashSet<String>,
-    last_arg: Option<String>,
+    pub last_arg: Option<String>,
 }
 
 lazy_static! {
@@ -110,36 +113,6 @@ pub enum ProcessArgs<T> {
     OtherProcess,
 }
 
-pub fn git_blame_filename() -> Option<String> {
-    calling_process_cmdline(ProcInfo::new(), guess_git_blame_filename)
-}
-
-pub fn guess_git_blame_filename(args: &[String]) -> ProcessArgs<String> {
-    let all_args = args.iter().map(|s| s.as_str());
-
-    // See git(1) and git-blame(1). Some arguments separate their parameter with space or '=', e.g.
-    // --date 2015 or --date=2015.
-    let git_blame_options_with_parameter =
-        "-C -c -L --since --ignore-rev --ignore-revs-file --contents --reverse --date";
-
-    let selected_args =
-        skip_uninteresting_args(all_args, git_blame_options_with_parameter.split(' '));
-
-    match selected_args.as_slice() {
-        [git, "blame", .., last_arg] if is_git_binary(git) => {
-            match Path::new(last_arg)
-                .file_name()
-                .map(|filename| filename.to_string_lossy().to_string())
-            {
-                Some(filename) => ProcessArgs::Args(filename),
-                None => ProcessArgs::ArgError,
-            }
-        }
-        [git, "blame"] if is_git_binary(git) => ProcessArgs::ArgError,
-        _ => ProcessArgs::OtherProcess,
-    }
-}
-
 pub fn describe_calling_process(args: &[String]) -> ProcessArgs<CallingProcess> {
     let mut args = args.iter().map(|s| s.as_str());
 
@@ -155,7 +128,12 @@ pub fn describe_calling_process(args: &[String]) -> ProcessArgs<CallingProcess>
         Some(command) => match Path::new(command).file_stem() {
             Some(s) if s.to_str().map(is_git_binary).unwrap_or(false) => {
                 let mut args = args.skip_while(|s| {
-                    *s != "diff" && *s != "show" && *s != "log" && *s != "reflog" && *s != "grep"
+                    *s != "diff"
+                        && *s != "show"
+                        && *s != "log"
+                        && *s != "reflog"
+                        && *s != "grep"
+                        && *s != "blame"
                 });
                 match args.next() {
                     Some("diff") => {
@@ -184,6 +162,9 @@ pub fn describe_calling_process(args: &[String]) -> ProcessArgs<CallingProcess>
                     Some("grep") => {
                         ProcessArgs::Args(CallingProcess::GitGrep(parse_command_line(args)))
                     }
+                    Some("blame") => {
+                        ProcessArgs::Args(CallingProcess::GitBlame(parse_command_line(args)))
+                    }
                     _ => {
                         // It's git, but not a subcommand that we parse. Don't
                         // look at any more processes.
@@ -223,49 +204,19 @@ fn is_git_binary(git: &str) -> bool {
         .unwrap_or(false)
 }
 
-// Skip all arguments starting with '-' from `args_it`. Also skip all arguments listed in
-// `skip_this_plus_parameter` plus their respective next argument.
-// Keep all arguments once a '--' is encountered.
-// (Note that some arguments work with and without '=': '--foo' 'bar' / '--foo=bar')
-fn skip_uninteresting_args<'a, 'b, ArgsI, SkipI>(
-    mut args_it: ArgsI,
-    skip_this_plus_parameter: SkipI,
-) -> Vec<&'a str>
-where
-    ArgsI: Iterator<Item = &'a str>,
-    SkipI: Iterator<Item = &'b str>,
-{
-    let arg_follows_space: HashSet<&'b str> = skip_this_plus_parameter.into_iter().collect();
-
-    let mut result = Vec::new();
-    loop {
-        match args_it.next() {
-            None => break result,
-            Some("--") => {
-                result.extend(args_it);
-                break result;
-            }
-            Some(arg) if arg_follows_space.contains(arg) => {
-                let _skip_parameter = args_it.next();
-            }
-            Some(arg) if !arg.starts_with('-') => {
-                result.push(arg);
-            }
-            Some(_) => { /* skip: --these -and --also=this */ }
-        }
-    }
-}
-
 // Given `--aa val -bc -d val e f -- ...` return
 // ({"--aa"}, {"-b", "-c", "-d"})
 fn parse_command_line<'a>(args: impl Iterator<Item = &'a str>) -> CommandLine {
     let mut long_options = HashSet::new();
     let mut short_options = HashSet::new();
     let mut last_arg = None;
+    let mut after_double_dash = false;
 
     for s in args {
-        if s == "--" {
-            break;
+        if after_double_dash {
+            last_arg = Some(s);
+        } else if s == "--" {
+            after_double_dash = true;
         } else if s.starts_with("--") {
             long_options.insert(s.split('=').next().unwrap().to_owned());
         } else if let Some(suffix) = s.strip_prefix('-') {
@@ -702,53 +653,6 @@ pub mod tests {
         }
     }
 
-    #[test]
-    fn test_guess_git_blame_filename() {
-        use ProcessArgs::Args;
-
-        fn make_string_vec(args: &[&str]) -> Vec<String> {
-            args.iter().map(|&x| x.to_owned()).collect::<Vec<String>>()
-        }
-        let args = make_string_vec(&["git", "blame", "hello", "world.txt"]);
-        assert_eq!(guess_git_blame_filename(&args), Args("world.txt".into()));
-
-        let args = make_string_vec(&[
-            "git",
-            "blame",
-            "-s",
-            "-f",
-            "hello.txt",
-            "--date=2015",
-            "--date",
-            "now",
-        ]);
-        assert_eq!(guess_git_blame_filename(&args), Args("hello.txt".into()));
-
-        let args = make_string_vec(&["git", "blame", "-s", "-f", "--", "hello.txt"]);
-        assert_eq!(guess_git_blame_filename(&args), Args("hello.txt".into()));
-
-        let args = make_string_vec(&["git", "blame", "--", "--not.an.argument"]);
-        assert_eq!(
-            guess_git_blame_filename(&args),
-            Args("--not.an.argument".into())
-        );
-
-        let args = make_string_vec(&["foo", "bar", "-a", "--123", "not.git"]);
-        assert_eq!(guess_git_blame_filename(&args), ProcessArgs::OtherProcess);
-
-        let args = make_string_vec(&["git", "blame", "--help.txt"]);
-        assert_eq!(guess_git_blame_filename(&args), ProcessArgs::ArgError);
-
-        let args = make_string_vec(&["git", "-c", "a=b", "blame", "main.rs"]);
-        assert_eq!(guess_git_blame_filename(&args), Args("main.rs".into()));
-
-        let args = make_string_vec(&["git", "blame", "README"]);
-        assert_eq!(guess_git_blame_filename(&args), Args("README".into()));
-
-        let args = make_string_vec(&["git", "blame", ""]);
-        assert_eq!(guess_git_blame_filename(&args), ProcessArgs::ArgError);
-    }
-
     #[derive(Debug, Default)]
     struct FakeProc {
         #[allow(dead_code)]
@@ -833,22 +737,34 @@ pub mod tests {
         {
             let _args = FakeParentArgs::once("git blame hello");
             assert_eq!(
-                calling_process_cmdline(ProcInfo::new(), guess_git_blame_filename),
-                Some("hello".into())
+                calling_process_cmdline(ProcInfo::new(), describe_calling_process),
+                Some(CallingProcess::GitBlame(CommandLine {
+                    long_options: [].into(),
+                    short_options: [].into(),
+                    last_arg: Some("hello".into())
+                }))
             );
         }
         {
             let _args = FakeParentArgs::once("git blame world.txt");
             assert_eq!(
-                calling_process_cmdline(ProcInfo::new(), guess_git_blame_filename),
-                Some("world.txt".into())
+                calling_process_cmdline(ProcInfo::new(), describe_calling_process),
+                Some(CallingProcess::GitBlame(CommandLine {
+                    long_options: [].into(),
+                    short_options: [].into(),
+                    last_arg: Some("world.txt".into())
+                }))
             );
         }
         {
             let _args = FakeParentArgs::for_scope("git blame hello world.txt");
             assert_eq!(
-                calling_process_cmdline(ProcInfo::new(), guess_git_blame_filename),
-                Some("world.txt".into())
+                calling_process_cmdline(ProcInfo::new(), describe_calling_process),
+                Some(CallingProcess::GitBlame(CommandLine {
+                    long_options: [].into(),
+                    short_options: [].into(),
+                    last_arg: Some("world.txt".into())
+                }))
             );
         }
     }
@@ -858,11 +774,14 @@ pub mod tests {
     fn test_process_testing_assert() {
         let _args = FakeParentArgs::once("git blame do.not.panic");
         assert_eq!(
-            calling_process_cmdline(ProcInfo::new(), guess_git_blame_filename),
-            Some("do.not.panic".into())
+            calling_process_cmdline(ProcInfo::new(), describe_calling_process),
+            Some(CallingProcess::GitBlame(CommandLine {
+                long_options: [].into(),
+                short_options: [].into(),
+                last_arg: Some("do.not.panic".into())
+            }))
         );
-
-        calling_process_cmdline(ProcInfo::new(), guess_git_blame_filename);
+        calling_process_cmdline(ProcInfo::new(), describe_calling_process);
     }
 
     #[test]
@@ -891,16 +810,24 @@ pub mod tests {
     }
 
     #[test]
-    fn test_process_testing_n_times_panic() {
+    fn test_process_testing_n_times() {
         let _args = FakeParentArgs::with(&["git blame once", "git blame twice"]);
         assert_eq!(
-            calling_process_cmdline(ProcInfo::new(), guess_git_blame_filename),
-            Some("once".into())
+            calling_process_cmdline(ProcInfo::new(), describe_calling_process),
+            Some(CallingProcess::GitBlame(CommandLine {
+                long_options: [].into(),
+                short_options: [].into(),
+                last_arg: Some("once".into())
+            }))
         );
 
         assert_eq!(
-            calling_process_cmdline(ProcInfo::new(), guess_git_blame_filename),
-            Some("twice".into())
+            calling_process_cmdline(ProcInfo::new(), describe_calling_process),
+            Some(CallingProcess::GitBlame(CommandLine {
+                long_options: [].into(),
+                short_options: [].into(),
+                last_arg: Some("twice".into())
+            }))
         );
     }
 
@@ -915,8 +842,12 @@ pub mod tests {
     fn test_process_testing_n_times_underused() {
         let _args = FakeParentArgs::with(&["git blame once", "git blame twice"]);
         assert_eq!(
-            calling_process_cmdline(ProcInfo::new(), guess_git_blame_filename),
-            Some("once".into())
+            calling_process_cmdline(ProcInfo::new(), describe_calling_process),
+            Some(CallingProcess::GitBlame(CommandLine {
+                long_options: [].into(),
+                short_options: [].into(),
+                last_arg: Some("once".into())
+            }))
         );
     }
 
@@ -925,14 +856,24 @@ pub mod tests {
     fn test_process_testing_n_times_overused() {
         let _args = FakeParentArgs::with(&["git blame once"]);
         assert_eq!(
-            calling_process_cmdline(ProcInfo::new(), guess_git_blame_filename),
-            Some("once".into())
+            calling_process_cmdline(ProcInfo::new(), describe_calling_process),
+            Some(CallingProcess::GitBlame(CommandLine {
+                long_options: [].into(),
+                short_options: [].into(),
+                last_arg: Some("once".into())
+            }))
         );
-        calling_process_cmdline(ProcInfo::new(), guess_git_blame_filename);
+        calling_process_cmdline(ProcInfo::new(), describe_calling_process);
     }
 
     #[test]
-    fn test_process_blame_no_parent_found() {
+    fn test_describe_calling_process_blame() {
+        let no_processes = MockProcInfo::with(&[]);
+        assert_eq!(
+            calling_process_cmdline(no_processes, describe_calling_process),
+            None
+        );
+
         let two_trees = MockProcInfo::with(&[
             (2, 100, "-shell", None),
             (3, 100, "git blame src/main.rs", Some(2)),
@@ -940,43 +881,119 @@ pub mod tests {
             (5, 100, "delta", Some(4)),
         ]);
         assert_eq!(
-            calling_process_cmdline(two_trees, guess_git_blame_filename),
+            calling_process_cmdline(two_trees, describe_calling_process),
             None
         );
-    }
 
-    #[test]
-    fn test_process_blame_info_with_parent() {
-        let no_processes = MockProcInfo::with(&[]);
+        let no_options_command_line = CommandLine {
+            long_options: [].into(),
+            short_options: [].into(),
+            last_arg: Some("hello.txt".to_string()),
+        };
+        let parent = MockProcInfo::with(&[
+            (2, 100, "-shell", None),
+            (3, 100, "git blame hello.txt", Some(2)),
+            (4, 100, "delta", Some(3)),
+        ]);
         assert_eq!(
-            calling_process_cmdline(no_processes, guess_git_blame_filename),
-            None
+            calling_process_cmdline(parent, describe_calling_process),
+            Some(CallingProcess::GitBlame(no_options_command_line.clone()))
         );
 
         let parent = MockProcInfo::with(&[
             (2, 100, "-shell", None),
-            (3, 100, "git blame hello.txt", Some(2)),
+            (3, 100, "git blame -- hello.txt", Some(2)),
+            (4, 100, "delta", Some(3)),
+        ]);
+        assert_eq!(
+            calling_process_cmdline(parent, describe_calling_process),
+            Some(CallingProcess::GitBlame(no_options_command_line.clone()))
+        );
+
+        let parent = MockProcInfo::with(&[
+            (2, 100, "-shell", None),
+            (3, 100, "git blame -- --not.an.argument", Some(2)),
+            (4, 100, "delta", Some(3)),
+        ]);
+        assert_eq!(
+            calling_process_cmdline(parent, describe_calling_process),
+            Some(CallingProcess::GitBlame(CommandLine {
+                long_options: [].into(),
+                short_options: [].into(),
+                last_arg: Some("--not.an.argument".to_string()),
+            }))
+        );
+
+        let parent = MockProcInfo::with(&[
+            (2, 100, "-shell", None),
+            (3, 100, "git blame --help.txt", Some(2)),
             (4, 100, "delta", Some(3)),
         ]);
         assert_eq!(
-            calling_process_cmdline(parent, guess_git_blame_filename),
-            Some("hello.txt".into())
+            calling_process_cmdline(parent, describe_calling_process),
+            Some(CallingProcess::GitBlame(CommandLine {
+                long_options: ["--help.txt".into()].into(),
+                short_options: [].into(),
+                last_arg: None,
+            }))
+        );
+
+        let parent = MockProcInfo::with(&[
+            (2, 100, "-shell", None),
+            (3, 100, "git blame --", Some(2)),
+            (4, 100, "delta", Some(3)),
+        ]);
+        assert_eq!(
+            calling_process_cmdline(parent, describe_calling_process),
+            Some(CallingProcess::GitBlame(CommandLine {
+                long_options: [].into(),
+                short_options: [].into(),
+                last_arg: None,
+            }))
+        );
+
+        let parent = MockProcInfo::with(&[
+            (2, 100, "-shell", None),
+            (3, 100, "Git.exe blame hello.txt", Some(2)),
+            (4, 100, "delta", Some(3)),
+        ]);
+        assert_eq!(
+            calling_process_cmdline(parent, describe_calling_process),
+            Some(CallingProcess::GitBlame(no_options_command_line.clone()))
+        );
+
+        let git_blame_command =
+            "git -c a=b blame -fnb --incremental -t --color-by-age -M --since=3.weeks --contents annotation.txt -C -C2 hello.txt";
+
+        // here -C2 is parsed as -C and -2. It doesn't really matters because we only use last_arg from options
+        // to determine the file type.
+        let expected_result = Some(CallingProcess::GitBlame(CommandLine {
+            long_options: set(&["--incremental", "--color-by-age", "--since", "--contents"]),
+            short_options: set(&["-f", "-n", "-b", "-t", "-M", "-C", "-2"]),
+            last_arg: Some("hello.txt".to_string()),
+        }));
+
+        let parent = MockProcInfo::with(&[
+            (2, 100, "-shell", None),
+            (3, 100, git_blame_command, Some(2)),
+            (4, 100, "delta", Some(3)),
+        ]);
+        assert_eq!(
+            calling_process_cmdline(parent, describe_calling_process),
+            expected_result
         );
 
         let grandparent = MockProcInfo::with(&[
             (2, 100, "-shell", None),
-            (3, 100, "git blame src/main.rs", Some(2)),
+            (3, 100, git_blame_command, Some(2)),
             (4, 100, "call_delta.sh", Some(3)),
             (5, 100, "delta", Some(4)),
         ]);
         assert_eq!(
-            calling_process_cmdline(grandparent, guess_git_blame_filename),
-            Some("main.rs".into())
+            calling_process_cmdline(grandparent, describe_calling_process),
+            expected_result
         );
-    }
 
-    #[test]
-    fn test_process_blame_info_with_sibling() {
         let sibling = MockProcInfo::with(&[
             (2, 100, "-xterm", None),
             (3, 100, "-shell", Some(2)),
@@ -984,8 +1001,12 @@ pub mod tests {
             (5, 100, "delta", Some(3)),
         ]);
         assert_eq!(
-            calling_process_cmdline(sibling, guess_git_blame_filename),
-            Some("main.rs".into())
+            calling_process_cmdline(sibling, describe_calling_process),
+            Some(CallingProcess::GitBlame(CommandLine {
+                long_options: [].into(),
+                short_options: [].into(),
+                last_arg: Some("src/main.rs".into())
+            }))
         );
 
         let indirect_sibling = MockProcInfo::with(&[
@@ -1002,8 +1023,12 @@ pub mod tests {
             (20, 100, "delta", Some(5)),
         ]);
         assert_eq!(
-            calling_process_cmdline(indirect_sibling, guess_git_blame_filename),
-            Some("main.abc".into())
+            calling_process_cmdline(indirect_sibling, describe_calling_process),
+            Some(CallingProcess::GitBlame(CommandLine {
+                long_options: set(&["--correct"]),
+                short_options: [].into(),
+                last_arg: Some("src/main.abc".into())
+            }))
         );
 
         let indirect_sibling2 = MockProcInfo::with(&[
@@ -1015,8 +1040,12 @@ pub mod tests {
             (20, 100, "delta", Some(5)),
         ]);
         assert_eq!(
-            calling_process_cmdline(indirect_sibling2, guess_git_blame_filename),
-            Some("main.def".into())
+            calling_process_cmdline(indirect_sibling2, describe_calling_process),
+            Some(CallingProcess::GitBlame(CommandLine {
+                long_options: [].into(),
+                short_options: [].into(),
+                last_arg: Some("src/main.def".into())
+            }))
         );
 
         // 3 blame processes, 2 with matching start times, pick the one with lower
@@ -1034,8 +1063,12 @@ pub mod tests {
             (20, 100, "delta", Some(5)),
         ]);
         assert_eq!(
-            calling_process_cmdline(indirect_sibling_start_times, guess_git_blame_filename),
-            Some("main.this".into())
+            calling_process_cmdline(indirect_sibling_start_times, describe_calling_process),
+            Some(CallingProcess::GitBlame(CommandLine {
+                long_options: [].into(),
+                short_options: [].into(),
+                last_arg: Some("src/main.this".into())
+            }))
         );
     }