Repair --default-language, and highlight using full filename (#1549)

* Fix clippy warnings

* Repair --default-language, and highlight using full filename

Fixed that the "txt" fallback was used instead of --default-language

And when looking for syntax highlighting, keep the filename around
as long as possible.

Using simple custom logic (filename > 4) a Makefile can be highlighted
with make syntax, but a 'cc' or 'ini' file does not get treated like
a *.cc or *.ini file.

Currently the underlying highlighting lib syntect and the sublime syntax
definitions can not make this distinction
(<http://www.sublimetext.com/docs/syntax.html>).

src/cli.rs

@@ -287,12 +287,12 @@ pub struct Opt {
     /// For more control, see the style options and --syntax-theme.
     pub dark: bool,
 
-    #[arg(long = "default-language", value_name = "LANG")]
+    #[arg(long = "default-language", value_name = "LANG", default_value = "txt")]
     /// Default language used for syntax highlighting.
     ///
-    /// Used when the language cannot be inferred from a filename. It will typically make sense to
-    /// set this in per-repository git config (.git/config)
-    pub default_language: Option<String>,
+    /// Used as a fallback when the language cannot be inferred from a filename. It will
+    /// typically make sense to set this in the per-repository config file '.git/config'.
+    pub default_language: String,
 
     /// Detect whether or not the terminal is dark or light by querying for its colors.
     ///

src/config.rs

@@ -31,6 +31,9 @@ use crate::wrapping::WrapConfig;
 
 pub const INLINE_SYMBOL_WIDTH_1: usize = 1;
 
+// Used if an invalid default-language was specified.
+pub const SYNTAX_FALLBACK_LANG: &str = "txt";
+
 #[cfg_attr(test, derive(Clone))]
 pub struct Config {
     pub available_terminal_width: usize,
@@ -49,7 +52,7 @@ pub struct Config {
     pub cwd_of_user_shell_process: Option<PathBuf>,
     pub cwd_relative_to_repo_root: Option<String>,
     pub decorations_width: cli::Width,
-    pub default_language: Option<String>,
+    pub default_language: String,
     pub diff_stat_align_width: usize,
     pub error_exit_code: i32,
     pub file_added_label: String,

src/handlers/blame.rs

@@ -77,13 +77,9 @@ impl<'a> StateMachine<'a> {
 
                 // Emit syntax-highlighted code
                 if matches!(self.state, State::Unknown) {
-                    if let Some(lang) = utils::process::git_blame_filename_extension()
-                        .as_ref()
-                        .or(self.config.default_language.as_ref())
-                    {
-                        self.painter.set_syntax(Some(lang));
-                        self.painter.set_highlighter();
-                    }
+                    self.painter
+                        .set_syntax(utils::process::git_blame_filename().as_deref());
+                    self.painter.set_highlighter();
                 }
                 self.state = State::Blame(key);
                 self.painter.syntax_highlight_and_paint_line(

src/handlers/diff_header.rs

@@ -95,10 +95,10 @@ impl<'a> StateMachine<'a> {
         if self.source == Source::DiffUnified {
             self.state = State::DiffHeader(DiffType::Unified);
             self.painter
-                .set_syntax(get_file_extension_from_marker_line(&self.line));
+                .set_syntax(get_filename_from_marker_line(&self.line));
         } else {
             self.painter
-                .set_syntax(get_file_extension_from_diff_header_line_file_path(
+                .set_syntax(get_filename_from_diff_header_line_file_path(
                     &self.minus_file,
                 ));
         }
@@ -129,7 +129,7 @@ impl<'a> StateMachine<'a> {
             .unwrap_or(path_or_mode);
         self.plus_file_event = file_event;
         self.painter
-            .set_syntax(get_file_extension_from_diff_header_line_file_path(
+            .set_syntax(get_filename_from_diff_header_line_file_path(
                 &self.plus_file,
             ));
         self.current_file_pair = Some((self.minus_file.clone(), self.plus_file.clone()));
@@ -300,30 +300,23 @@ pub fn write_generic_diff_header_header_line(
 
 #[allow(clippy::tabs_in_doc_comments)]
 /// Given input like
-/// "--- one.rs	2019-11-20 06:16:08.000000000 +0100"
-/// Return "rs"
-fn get_file_extension_from_marker_line(line: &str) -> Option<&str> {
+/// "--- a/zero/one.rs	2019-11-20 06:16:08.000000000 +0100"
+/// Return "one.rs"
+fn get_filename_from_marker_line(line: &str) -> Option<&str> {
     line.split('\t')
         .next()
         .and_then(|column| column.split(' ').nth(1))
-        .and_then(|file| file.split('.').last())
+        .and_then(get_filename_from_diff_header_line_file_path)
 }
 
-fn get_file_extension_from_diff_header_line_file_path(path: &str) -> Option<&str> {
-    if path.is_empty() || path == "/dev/null" {
-        None
-    } else {
-        get_extension(path).map(|ex| ex.trim())
-    }
-}
-
-/// Attempt to parse input as a file path and return extension as a &str.
-pub fn get_extension(s: &str) -> Option<&str> {
-    let path = Path::new(s);
-    path.extension()
-        .and_then(|e| e.to_str())
-        // E.g. 'Makefile' is the file name and also the extension
-        .or_else(|| path.file_name().and_then(|s| s.to_str()))
+fn get_filename_from_diff_header_line_file_path(path: &str) -> Option<&str> {
+    Path::new(path).file_name().and_then(|filename| {
+        if path != "/dev/null" {
+            filename.to_str()
+        } else {
+            None
+        }
+    })
 }
 
 fn parse_diff_header_line(line: &str, git_diff_name: bool) -> (String, FileEvent) {
@@ -477,51 +470,49 @@ mod tests {
     use super::*;
 
     #[test]
-    fn test_get_file_extension_from_marker_line() {
+    fn test_get_filename_from_marker_line() {
         assert_eq!(
-            get_file_extension_from_marker_line(
-                "--- src/one.rs	2019-11-20 06:47:56.000000000 +0100"
-            ),
-            Some("rs")
+            get_filename_from_marker_line("--- src/one.rs	2019-11-20 06:47:56.000000000 +0100"),
+            Some("one.rs")
         );
     }
 
     #[test]
-    fn test_get_file_extension_from_diff_header_line() {
+    fn test_get_filename_from_diff_header_line() {
         assert_eq!(
-            get_file_extension_from_diff_header_line_file_path("a/src/parse.rs"),
-            Some("rs")
+            get_filename_from_diff_header_line_file_path("a/src/parse.rs"),
+            Some("parse.rs")
         );
         assert_eq!(
-            get_file_extension_from_diff_header_line_file_path("b/src/pa rse.rs"),
-            Some("rs")
+            get_filename_from_diff_header_line_file_path("b/src/pa rse.rs"),
+            Some("pa rse.rs")
         );
         assert_eq!(
-            get_file_extension_from_diff_header_line_file_path("src/pa rse.rs"),
-            Some("rs")
+            get_filename_from_diff_header_line_file_path("src/pa rse.rs"),
+            Some("pa rse.rs")
         );
         assert_eq!(
-            get_file_extension_from_diff_header_line_file_path("wat hello.rs"),
-            Some("rs")
+            get_filename_from_diff_header_line_file_path("wat hello.rs"),
+            Some("wat hello.rs")
         );
         assert_eq!(
-            get_file_extension_from_diff_header_line_file_path("/dev/null"),
+            get_filename_from_diff_header_line_file_path("/dev/null"),
             None
         );
         assert_eq!(
-            get_file_extension_from_diff_header_line_file_path("Dockerfile"),
+            get_filename_from_diff_header_line_file_path("Dockerfile"),
             Some("Dockerfile")
         );
         assert_eq!(
-            get_file_extension_from_diff_header_line_file_path("Makefile"),
+            get_filename_from_diff_header_line_file_path("Makefile"),
             Some("Makefile")
         );
         assert_eq!(
-            get_file_extension_from_diff_header_line_file_path("a/src/Makefile"),
+            get_filename_from_diff_header_line_file_path("a/src/Makefile"),
             Some("Makefile")
         );
         assert_eq!(
-            get_file_extension_from_diff_header_line_file_path("src/Makefile"),
+            get_filename_from_diff_header_line_file_path("src/Makefile"),
             Some("Makefile")
         );
     }

src/handlers/git_show_file.rs

@@ -9,11 +9,11 @@ impl<'a> StateMachine<'a> {
         self.painter.emit()?;
         let mut handled_line = false;
         if matches!(self.state, State::Unknown) {
-            if let process::CallingProcess::GitShow(_, Some(extension)) =
+            if let process::CallingProcess::GitShow(_, Some(filename)) =
                 &*process::calling_process()
             {
                 self.state = State::GitShowFile;
-                self.painter.set_syntax(Some(extension));
+                self.painter.set_syntax(Some(filename));
             } else {
                 return Ok(handled_line);
             }

src/handlers/grep.rs

@@ -132,12 +132,8 @@ impl<'a> StateMachine<'a> {
             self.painter.set_highlighter()
         }
         if new_path {
-            if let Some(lang) = handlers::diff_header::get_extension(&grep_line.path)
-                .or(self.config.default_language.as_deref())
-            {
-                self.painter.set_syntax(Some(lang));
-                self.painter.set_highlighter();
-            }
+            self.painter.set_syntax(Some(grep_line.path.as_ref()));
+            self.painter.set_highlighter();
         }
         match &self.state {
             State::Grep(GrepType::Ripgrep, _, _, _) => {

src/options/get.rs

@@ -38,7 +38,7 @@ where
     T::get_option_value(option_name, builtin_features, opt, git_config)
 }
 
-static GIT_CONFIG_THEME_REGEX: &str = r#"^delta\.(.+)\.(light|dark)$"#;
+static GIT_CONFIG_THEME_REGEX: &str = r"^delta\.(.+)\.(light|dark)$";
 
 pub fn get_themes(git_config: Option<git_config::GitConfig>) -> Vec<String> {
     let mut themes: Vec<String> = Vec::new();

src/options/set.rs

@@ -735,7 +735,7 @@ pub mod tests {
         assert_eq!(opt.commit_decoration_style, "black black");
         assert_eq!(opt.commit_style, "black black");
         assert!(!opt.dark);
-        assert_eq!(opt.default_language, Some("rs".to_owned()));
+        assert_eq!(opt.default_language, "rs".to_owned());
         // TODO: should set_options not be called on any feature flags?
         // assert_eq!(opt.diff_highlight, true);
         // assert_eq!(opt.diff_so_fancy, true);

src/paint.rs

@@ -70,9 +70,7 @@ pub enum StyleSectionSpecifier<'l> {
 
 impl<'p> Painter<'p> {
     pub fn new(writer: &'p mut dyn Write, config: &'p config::Config) -> Self {
-        let default_syntax =
-            Self::get_syntax(&config.syntax_set, None, config.default_language.as_deref());
-
+        let default_syntax = Self::get_syntax(&config.syntax_set, None, &config.default_language);
         let panel_width_fix = ansifill::UseFullPanelWidth::new(config);
 
         let line_numbers_data = if config.line_numbers {
@@ -104,27 +102,50 @@ impl<'p> Painter<'p> {
         }
     }
 
-    pub fn set_syntax(&mut self, extension: Option<&str>) {
+    pub fn set_syntax(&mut self, filename: Option<&str>) {
         self.syntax = Painter::get_syntax(
             &self.config.syntax_set,
-            extension,
-            self.config.default_language.as_deref(),
+            filename,
+            &self.config.default_language,
         );
     }
 
     fn get_syntax<'a>(
         syntax_set: &'a SyntaxSet,
-        extension: Option<&str>,
-        fallback_extension: Option<&str>,
+        filename: Option<&str>,
+        fallback: &str,
     ) -> &'a SyntaxReference {
-        for extension in [extension, fallback_extension].iter().flatten() {
-            if let Some(syntax) = syntax_set.find_syntax_by_extension(extension) {
-                return syntax;
+        if let Some(filename) = filename {
+            let path = std::path::Path::new(filename);
+            let file_name = path.file_name().and_then(|n| n.to_str()).unwrap_or("");
+            let extension = path.extension().and_then(|x| x.to_str()).unwrap_or("");
+
+            // Like syntect's `find_syntax_for_file`, without inspecting the file content, plus:
+            // If the file has NO extension then look up the whole filename as a
+            // syntax definition (if it is longer than 4 bytes).
+            // This means file formats like Makefile/Dockerfile/Rakefile etc. will get highlighted,
+            // but 1-4 short filenames will not -- even if they, as a whole, match an extension:
+            // 'rs' will not get highlighted, while 'x.rs' will.
+            if !extension.is_empty() || file_name.len() > 4 {
+                if let Some(syntax) = syntax_set
+                    .find_syntax_by_extension(file_name)
+                    .or_else(|| syntax_set.find_syntax_by_extension(extension))
+                {
+                    return syntax;
+                }
             }
         }
-        syntax_set
-            .find_syntax_by_extension("txt")
-            .unwrap_or_else(|| delta_unreachable("Failed to find any language syntax definitions."))
+
+        // Nothing found, try the user provided fallback, or the internal fallback.
+        if let Some(syntax) = syntax_set.find_syntax_for_file(fallback).unwrap_or(None) {
+            syntax
+        } else {
+            syntax_set
+                .find_syntax_by_extension(config::SYNTAX_FALLBACK_LANG)
+                .unwrap_or_else(|| {
+                    delta_unreachable("Failed to find any language syntax definitions.")
+                })
+        }
     }
 
     pub fn set_highlighter(&mut self) {

src/subcommands/show_colors.rs

@@ -28,7 +28,7 @@ pub fn show_colors() -> std::io::Result<()> {
     let writer = output_type.handle().unwrap();
 
     let mut painter = paint::Painter::new(writer, &config);
-    painter.set_syntax(Some("ts"));
+    painter.set_syntax(Some("a.ts"));
     painter.set_highlighter();
 
     let title_style = ansi_term::Style::new().bold();

src/tests/ansi_test_utils.rs

@@ -122,15 +122,19 @@ pub mod ansi_test_utils {
         line_number: usize,
         substring_begin: usize,
         expected_substring: &str,
-        language_extension: &str,
+        filename_for_highlighting: &str,
         state: State,
         config: &Config,
     ) {
+        assert!(
+            filename_for_highlighting.contains("."),
+            "expecting filename, not just a file extension"
+        );
         let line = output.lines().nth(line_number).unwrap();
         let substring_end = substring_begin + expected_substring.len();
         let substring = &ansi::strip_ansi_codes(line)[substring_begin..substring_end];
         assert_eq!(substring, expected_substring);
-        let painted_substring = paint_line(substring, language_extension, state, config);
+        let painted_substring = paint_line(substring, filename_for_highlighting, state, config);
         // remove trailing newline appended by paint::paint_lines.
         assert!(line.contains(painted_substring.trim_end()));
     }
@@ -163,7 +167,7 @@ pub mod ansi_test_utils {
 
     pub fn paint_line(
         line: &str,
-        language_extension: &str,
+        filename_for_highlighting: &str,
         state: State,
         config: &Config,
     ) -> String {
@@ -174,7 +178,7 @@ pub mod ansi_test_utils {
             is_syntax_highlighted: true,
             ..Style::new()
         };
-        painter.set_syntax(Some(language_extension));
+        painter.set_syntax(Some(filename_for_highlighting));
         painter.set_highlighter();
         let lines = vec![(line.to_string(), state)];
         let syntax_style_sections = paint::get_syntax_style_sections_for_lines(