Repository: dandavison/delta
Pull request #865 (2021-12-21 01:28:18)

Fix grep parse bugs


Patch:
diff --git a/src/config.rs b/src/config.rs
index be944cf..1eb8778 100644
--- a/src/config.rs
+++ b/src/config.rs
@@ -57,6 +57,7 @@ fn adapt_wrap_max_lines_argument(arg: String) -> usize {
     }
 }
 
+#[cfg_attr(test, derive(Clone))]
 pub struct Config {
     pub available_terminal_width: usize,
     pub background_color_extends_to_terminal_width: bool,
diff --git a/src/features/side_by_side.rs b/src/features/side_by_side.rs
index fb0a819..ed55b3b 100644
--- a/src/features/side_by_side.rs
+++ b/src/features/side_by_side.rs
@@ -36,7 +36,7 @@ pub use MinusPlusIndex::Plus as Right;
 
 use super::line_numbers::LineNumbersData;
 
-#[derive(Debug)]
+#[derive(Debug, Clone)]
 pub struct Panel {
     pub width: usize,
 }
diff --git a/src/git_config/mod.rs b/src/git_config/mod.rs
index 9b9f11f..2151725 100644
--- a/src/git_config/mod.rs
+++ b/src/git_config/mod.rs
@@ -8,8 +8,6 @@ use std::env;
 #[cfg(test)]
 use std::path::Path;
 
-use crate::fatal;
-
 use lazy_static::lazy_static;
 
 pub struct GitConfig {
@@ -17,10 +15,31 @@ pub struct GitConfig {
     config_from_env_var: HashMap<String, String>,
     pub enabled: bool,
     pub repo: Option<git2::Repository>,
+    // To make GitConfig cloneable when testing (in turn to make Config cloneable):
+    #[cfg(test)]
+    path: std::path::PathBuf,
+}
+
+#[cfg(test)]
+impl Clone for GitConfig {
+    fn clone(&self) -> Self {
+        assert!(self.repo.is_none());
+        GitConfig {
+            // Assumes no test modifies the file pointed to by `path`
+            config: git2::Config::open(&self.path).unwrap(),
+            config_from_env_var: self.config_from_env_var.clone(),
+            enabled: self.enabled,
+            repo: None,
+            path: self.path.clone(),
+        }
+    }
 }
 
 impl GitConfig {
+    #[cfg(not(test))]
     pub fn try_create() -> Option<Self> {
+        use crate::fatal;
+
         let repo = match std::env::current_dir() {
             Ok(dir) => git2::Repository::discover(dir).ok(),
             _ => None,
@@ -45,6 +64,11 @@ impl GitConfig {
         }
     }
 
+    #[cfg(test)]
+    pub fn try_create() -> Option<Self> {
+        unreachable!("GitConfig::try_create() is not available when testing");
+    }
+
     #[cfg(test)]
     pub fn from_path(path: &Path, honor_env_var: bool) -> Self {
         Self {
@@ -56,6 +80,7 @@ impl GitConfig {
             },
             repo: None,
             enabled: true,
+            path: path.into(),
         }
     }
 
diff --git a/src/handlers/grep.rs b/src/handlers/grep.rs
index c3d6da6..3bff9c4 100644
--- a/src/handlers/grep.rs
+++ b/src/handlers/grep.rs
@@ -288,18 +288,30 @@ fn make_output_config() -> GrepOutputConfig {
 }
 
 enum GrepLineRegex {
-    FilePathWithFileExtension,
-    FilePathWithoutSeparatorCharacters,
+    WithFileExtensionAndLineNumber,
+    WithFileExtension,
+    WithFileExtensionNoSpaces,
+    WithoutSeparatorCharacters,
+}
+
+lazy_static! {
+    static ref GREP_LINE_REGEX_ASSUMING_FILE_EXTENSION_AND_LINE_NUMBER: Regex =
+        make_grep_line_regex(GrepLineRegex::WithFileExtensionAndLineNumber);
+}
+
+lazy_static! {
+    static ref GREP_LINE_REGEX_ASSUMING_FILE_EXTENSION_NO_SPACES: Regex =
+        make_grep_line_regex(GrepLineRegex::WithFileExtensionNoSpaces);
 }
 
 lazy_static! {
     static ref GREP_LINE_REGEX_ASSUMING_FILE_EXTENSION: Regex =
-        make_grep_line_regex(GrepLineRegex::FilePathWithFileExtension);
+        make_grep_line_regex(GrepLineRegex::WithFileExtension);
 }
 
 lazy_static! {
     static ref GREP_LINE_REGEX_ASSUMING_NO_INTERNAL_SEPARATOR_CHARS: Regex =
-        make_grep_line_regex(GrepLineRegex::FilePathWithoutSeparatorCharacters);
+        make_grep_line_regex(GrepLineRegex::WithoutSeparatorCharacters);
 }
 
 // See tests for example grep lines
@@ -328,16 +340,24 @@ fn make_grep_line_regex(regex_variant: GrepLineRegex) -> Regex {
     // Make-7-file-7-xxx
 
     let file_path = match regex_variant {
-        GrepLineRegex::FilePathWithFileExtension => {
+        GrepLineRegex::WithFileExtensionAndLineNumber | GrepLineRegex::WithFileExtension => {
             r"
         (                        # 1. file name (colons not allowed)
             [^:|\ ]                 # try to be strict about what a file path can start with
             [^:]*                   # anything
-            \.[^.\ :=-]{1,6}        # extension
+            [^\ ]\.[^.\ :=-]{1,6}   # extension
         )    
         "
         }
-        GrepLineRegex::FilePathWithoutSeparatorCharacters => {
+        GrepLineRegex::WithFileExtensionNoSpaces => {
+            r"
+        (                        # 1. file name (colons not allowed)
+            [^:|\ ]+                # try to be strict about what a file path can start with
+            [^\ ]\.[^.\ :=-]{1,6}   # extension
+        )    
+        "
+        }
+        GrepLineRegex::WithoutSeparatorCharacters => {
             r"
         (                        # 1. file name (colons not allowed)
             [^:|\ =-]               # try to be strict about what a file path can start with
@@ -348,30 +368,59 @@ fn make_grep_line_regex(regex_variant: GrepLineRegex) -> Regex {
         }
     };
 
+    let separator = match regex_variant {
+        GrepLineRegex::WithFileExtensionAndLineNumber => {
+            r#"
+    (?:
+        (
+            :                # 2. match marker
+            ([0-9]+):        # 3. line number followed by second match marker
+        )
+        |
+        (
+            -                # 4. nomatch marker
+            ([0-9]+)-        # 5. line number followed by second nomatch marker
+        )
+        |
+        (
+            =                # 6. match marker
+            ([0-9]+)=        # 7. line number followed by second header marker
+        )
+    )
+            "#
+        }
+        _ => {
+            r#"
+    (?:
+        (                    
+            :                # 2. match marker
+            (?:([0-9]+):)?   # 3. optional: line number followed by second match marker
+        )
+        |
+        (
+            -                # 4. nomatch marker
+            (?:([0-9]+)-)?   # 5. optional: line number followed by second nomatch marker
+        )
+        |
+        (
+            =                # 6. match marker
+            (?:([0-9]+)=)?   # 7. optional: line number followed by second header marker
+        )
+    )
+        "#
+        }
+    };
+
     Regex::new(&format!(
         "(?x)
 ^
 {file_path}
-(?:
-    (                    
-        :                # 2. match marker
-        (?:([0-9]+):)?   # 3. optional: line number followed by second match marker
-    )
-    |
-    (
-        -                # 4. nomatch marker
-        (?:([0-9]+)-)?   # 5. optional: line number followed by second nomatch marker
-    )
-    |
-    (
-        =                # 6. match marker
-        (?:([0-9]+)=)?   # 7. optional: line number followed by second header marker
-    )
-)
+{separator}
 (.*)                     # 8. code (i.e. line contents)
 $
 ",
-        file_path = file_path
+        file_path = file_path,
+        separator = separator,
     ))
     .unwrap()
 }
@@ -382,6 +431,8 @@ pub fn parse_grep_line(line: &str) -> Option<GrepLine> {
     } else {
         match &*process::calling_process() {
             process::CallingProcess::GitGrep(_) | process::CallingProcess::OtherGrep => [
+                &*GREP_LINE_REGEX_ASSUMING_FILE_EXTENSION_AND_LINE_NUMBER,
+                &*GREP_LINE_REGEX_ASSUMING_FILE_EXTENSION_NO_SPACES,
                 &*GREP_LINE_REGEX_ASSUMING_FILE_EXTENSION,
                 &*GREP_LINE_REGEX_ASSUMING_NO_INTERNAL_SEPARATOR_CHARS,
             ]
@@ -654,6 +705,28 @@ mod tests {
                 submatches: None,
             })
         );
+
+        assert_eq!(
+            parse_grep_line(r#"aaa/bbb.scala-              s"xxx.yyy.zzz: $ccc ddd""#),
+            Some(GrepLine {
+                path: "aaa/bbb.scala".into(),
+                line_number: None,
+                line_type: LineType::Context,
+                code: r#"              s"xxx.yyy.zzz: $ccc ddd""#.into(),
+                submatches: None,
+            })
+        );
+
+        assert_eq!(
+            parse_grep_line(r#"aaa/bbb.scala-  val atRegex = Regex.compile("(@.*)|(-shdw@.*)""#),
+            Some(GrepLine {
+                path: "aaa/bbb.scala".into(),
+                line_number: None,
+                line_type: LineType::Context,
+                code: r#"  val atRegex = Regex.compile("(@.*)|(-shdw@.*)""#.into(),
+                submatches: None,
+            })
+        );
     }
 
     #[test]
@@ -684,6 +757,28 @@ mod tests {
                 submatches: None,
             })
         );
+
+        assert_eq!(
+            parse_grep_line(r#"foo.rs-12-  .x-"#),
+            Some(GrepLine {
+                path: "foo.rs".into(),
+                line_number: Some(12),
+                line_type: LineType::Context,
+                code: r#"  .x-"#.into(),
+                submatches: None,
+            })
+        );
+
+        assert_eq!(
+            parse_grep_line(r#"foo.rs-12-.x-"#),
+            Some(GrepLine {
+                path: "foo.rs".into(),
+                line_number: Some(12),
+                line_type: LineType::Context,
+                code: r#".x-"#.into(),
+                submatches: None,
+            })
+        );
     }
 
     #[test]
diff --git a/src/minusplus.rs b/src/minusplus.rs
index e37c801..1e2e66c 100644
--- a/src/minusplus.rs
+++ b/src/minusplus.rs
@@ -2,7 +2,7 @@ use std::ops::{Index, IndexMut};
 
 /// Represent data related to removed/minus and added/plus lines which
 /// can be indexed with [`MinusPlusIndex::{Plus`](MinusPlusIndex::Plus)`,`[`Minus}`](MinusPlusIndex::Minus).
-#[derive(Debug, PartialEq, Eq)]
+#[derive(Debug, Clone, PartialEq, Eq)]
 pub struct MinusPlus<T> {
     pub minus: T,
     pub plus: T,
diff --git a/src/style.rs b/src/style.rs
index fe0da88..925d7e7 100644
--- a/src/style.rs
+++ b/src/style.rs
@@ -215,6 +215,7 @@ pub fn ansi_term_style_equality(a: ansi_term::Style, b: ansi_term::Style) -> boo
 // TODO: The equality methods were implemented first, and the equality_key
 // methods later. The former should be re-implemented in terms of the latter.
 // But why did the former not address equality of ansi_term::Color::RGB values?
+#[derive(Clone)]
 pub struct AnsiTermStyleEqualityKey {
     attrs_key: (bool, bool, bool, bool, bool, bool, bool, bool),
     foreground_key: Option<(u8, u8, u8, u8)>,
diff --git a/src/wrapping.rs b/src/wrapping.rs
index f1078de..37f7a6f 100644
--- a/src/wrapping.rs
+++ b/src/wrapping.rs
@@ -988,11 +988,11 @@ index 223ca50..e69de29 100644
 
     #[test]
     fn test_alignment_2_lines_vs_3_lines() {
-        let config_1 =
-            || make_config_from_args(&default_wrap_cfg_plus(&["--side-by-side", "--width", "55"]));
+        let config =
+            make_config_from_args(&default_wrap_cfg_plus(&["--side-by-side", "--width", "55"]));
 
         {
-            DeltaTest::with_config(config_1())
+            DeltaTest::with_config(&config)
                 .with_input(&format!(
                     "{}-{}+{}",
                     HUNK_ALIGN_DIFF_HEADER, HUNK_ALIGN_DIFF_SHORT, HUNK_ALIGN_DIFF_LONG
@@ -1007,7 +1007,7 @@ index 223ca50..e69de29 100644
         }
 
         {
-            DeltaTest::with_config(config_1())
+            DeltaTest::with_config(&config)
                 .with_input(&format!(
                     "{}-{}+{}",
                     HUNK_ALIGN_DIFF_HEADER, HUNK_ALIGN_DIFF_LONG, HUNK_ALIGN_DIFF_SHORT
@@ -1023,18 +1023,16 @@ index 223ca50..e69de29 100644
 
     #[test]
     fn test_alignment_1_line_vs_3_lines() {
-        let config_2 = || {
-            make_config_from_args(&default_wrap_cfg_plus(&[
-                "--side-by-side",
-                "--width",
-                "61",
-                "--line-fill-method",
-                "spaces",
-            ]))
-        };
+        let config = make_config_from_args(&default_wrap_cfg_plus(&[
+            "--side-by-side",
+            "--width",
+            "61",
+            "--line-fill-method",
+            "spaces",
+        ]));
 
         {
-            DeltaTest::with_config(config_2())
+            DeltaTest::with_config(&config)
                 .with_input(&format!(
                     "{}-{}+{}",
                     HUNK_ALIGN_DIFF_HEADER, HUNK_ALIGN_DIFF_SHORT, HUNK_ALIGN_DIFF_LONG
@@ -1048,7 +1046,7 @@ index 223ca50..e69de29 100644
         }
 
         {
-            DeltaTest::with_config(config_2())
+            DeltaTest::with_config(&config)
                 .with_input(&format!(
                     "{}-{}+{}",
                     HUNK_ALIGN_DIFF_HEADER, HUNK_ALIGN_DIFF_LONG, HUNK_ALIGN_DIFF_SHORT
@@ -1065,22 +1063,19 @@ index 223ca50..e69de29 100644
     #[test]
     fn test_wrap_max_lines_2() {
         // TODO overriding is not possible, need to change config directly
-        let config_3 = || {
-            let mut config = make_config_from_args(&default_wrap_cfg_plus(&[
-                // "--wrap-max-lines",
-                // "2",
-                "--side-by-side",
-                "--width",
-                "72",
-                "--line-fill-method",
-                "spaces",
-            ]));
-            config.truncation_symbol = ">".into();
-            config
-        };
+        let mut config = make_config_from_args(&default_wrap_cfg_plus(&[
+            // "--wrap-max-lines",
+            // "2",
+            "--side-by-side",
+            "--width",
+            "72",
+            "--line-fill-method",
+            "spaces",
+        ]));
+        config.truncation_symbol = ">".into();
 
         {
-            DeltaTest::with_config(config_3())
+            DeltaTest::with_config(&config)
                 .with_input(&format!(
                     "{}-{}+{}",
                     HUNK_ALIGN_DIFF_HEADER, HUNK_ALIGN_DIFF_SHORT, HUNK_ALIGN_DIFF_LONG
@@ -1094,9 +1089,8 @@ index 223ca50..e69de29 100644
         }
 
         {
-            let mut config = config_3();
             config.wrap_config.max_lines = 2;
-            DeltaTest::with_config(config)
+            DeltaTest::with_config(&config)
                 .with_input(&format!(
                     "{}-{}+{}",
                     HUNK_ALIGN_DIFF_HEADER, HUNK_ALIGN_DIFF_SHORT, HUNK_ALIGN_DIFF_LONG


Test patch:
diff --git a/src/tests/integration_test_utils.rs b/src/tests/integration_test_utils.rs
index 2e7e24c..6c75efd 100644
--- a/src/tests/integration_test_utils.rs
+++ b/src/tests/integration_test_utils.rs
@@ -1,5 +1,6 @@
 #![cfg(test)]
 
+use std::borrow::Cow;
 use std::fs::File;
 use std::io::{BufReader, Write};
 use std::path::Path;
@@ -147,24 +148,24 @@ pub fn delineated_string(txt: &str) -> String {
     top + &nl + txt + &nl + &btm
 }
 
-pub struct DeltaTest {
-    config: config::Config,
+pub struct DeltaTest<'a> {
+    config: Cow<'a, config::Config>,
     calling_process: Option<String>,
     explain_ansi_: bool,
 }
 
-impl DeltaTest {
+impl<'a> DeltaTest<'a> {
     pub fn with_args(args: &[&str]) -> Self {
         Self {
-            config: make_config_from_args(args),
+            config: Cow::Owned(make_config_from_args(args)),
             calling_process: None,
             explain_ansi_: false,
         }
     }
 
-    pub fn with_config(config: config::Config) -> Self {
+    pub fn with_config(config: &'a config::Config) -> Self {
         Self {
-            config: config,
+            config: Cow::Borrowed(config),
             calling_process: None,
             explain_ansi_: false,
         }
@@ -174,7 +175,9 @@ impl DeltaTest {
     where
         F: Fn(&mut config::Config),
     {
-        f(&mut self.config);
+        let mut owned_config = self.config.into_owned();
+        f(&mut owned_config);
+        self.config = Cow::Owned(owned_config);
         self
     }