From e825a4fa1080bde5c4ff729cbe30d77ca962d1c9 Mon Sep 17 00:00:00 2001 From: William Escande Date: Wed, 6 Apr 2022 22:16:04 +0200 Subject: [PATCH] Fix warning highlight for trailing whitespace Fix #137 --- src/edits.rs | 81 ++++++++--- src/paint.rs | 35 ++--- src/style.rs | 10 ++ src/tests/ansi_test_utils.rs | 73 +++++++++- src/tests/test_example_diffs.rs | 247 ++++++++++++++++++++++++++++++++ 5 files changed, 401 insertions(+), 45 deletions(-) diff --git a/src/edits.rs b/src/edits.rs index eb6a310a0..1231e95ae 100644 --- a/src/edits.rs +++ b/src/edits.rs @@ -90,7 +90,16 @@ where } // Emit any remaining plus lines for plus_line in &plus_lines[plus_index..] { - annotated_plus_lines.push(vec![(noop_insertions[plus_index], plus_line)]); + // Split the line if it contains extra whitespace at the end preceded with non whitespace + let line = plus_line.trim_end_matches(&[' ', '\t', '\n']); + if !line.is_empty() && line != plus_line.trim_end_matches('\n') { + annotated_plus_lines.push(vec![ + (noop_insertions[plus_index], line), + (noop_insertions[plus_index], &plus_line[line.len()..]), + ]); + } else { + annotated_plus_lines.push(vec![(noop_insertions[plus_index], plus_line)]); + } line_alignment.push((None, Some(plus_index))); plus_index += 1; } @@ -228,14 +237,19 @@ where }, minus_section, )); - annotated_plus_line.push(( - if coalesce_space_with_previous { - plus_op_prev - } else { - noop_insertion - }, - plus_section(n, &mut y_offset), - )); + let op = if coalesce_space_with_previous { + plus_op_prev + } else { + noop_insertion + }; + let plus_section = plus_section(n, &mut y_offset); + let line = plus_section.trim_end_matches(&[' ', '\t', '\n']); + if !line.is_empty() && line != plus_section.trim_end_matches('\n') { + annotated_plus_line.push((op, line)); + annotated_plus_line.push((plus_op_prev, &plus_section[line.len()..])); + } else { + annotated_plus_line.push((op, plus_section)); + } minus_op_prev = noop_deletion; plus_op_prev = noop_insertion; } @@ -553,7 +567,8 @@ mod tests { ], ], vec![vec![ - (PlusNoop, "á á b á á "), + (PlusNoop, "á á b á á"), + (PlusNoop, " "), (Insertion, "c"), (PlusNoop, " á á b"), ]], @@ -574,9 +589,19 @@ mod tests { vec![(MinusNoop, "cccc "), (Deletion, "c"), (MinusNoop, " ccc")], ], vec![ - vec![(PlusNoop, "bbbb "), (Insertion, "!"), (PlusNoop, " bbb")], + vec![ + (PlusNoop, "bbbb"), + (PlusNoop, " "), + (Insertion, "!"), + (PlusNoop, " bbb"), + ], vec![(PlusNoop, "dddd d ddd")], - vec![(PlusNoop, "cccc "), (Insertion, "!"), (PlusNoop, " ccc")], + vec![ + (PlusNoop, "cccc"), + (PlusNoop, " "), + (Insertion, "!"), + (PlusNoop, " ccc"), + ], ], ), 0.66, @@ -615,7 +640,8 @@ mod tests { (MinusNoop, "EditOperation>("), ]], vec![vec![ - (PlusNoop, "fn coalesce_edits<'a, "), + (PlusNoop, "fn coalesce_edits<'a,"), + (PlusNoop, " "), (Insertion, "'b, "), (PlusNoop, "EditOperation>("), ]], @@ -636,7 +662,8 @@ mod tests { (MinusNoop, ":"), ]], vec![vec![ - (PlusNoop, "for _ in range(0, "), + (PlusNoop, "for _ in range(0,"), + (PlusNoop, " "), (Insertion, "int("), (PlusNoop, "options[\"count\"])"), (Insertion, ")"), @@ -654,7 +681,12 @@ mod tests { vec!["a b a"], ( vec![vec![(MinusNoop, "a "), (MinusNoop, "a")]], - vec![vec![(PlusNoop, "a "), (Insertion, "b "), (PlusNoop, "a")]], + vec![vec![ + (PlusNoop, "a"), + (PlusNoop, " "), + (Insertion, "b "), + (PlusNoop, "a"), + ]], ), 1.0, ); @@ -663,7 +695,12 @@ mod tests { vec!["a b b a"], ( vec![vec![(MinusNoop, "a "), (MinusNoop, "a")]], - vec![vec![(PlusNoop, "a "), (Insertion, "b b "), (PlusNoop, "a")]], + vec![vec![ + (PlusNoop, "a"), + (PlusNoop, " "), + (Insertion, "b b "), + (PlusNoop, "a"), + ]], ), 1.0, ); @@ -684,7 +721,8 @@ mod tests { (MinusNoop, " from any one of them."), ]], vec![vec![ - (PlusNoop, "so it is safe to read "), + (PlusNoop, "so it is safe to read"), + (PlusNoop, " "), (Insertion, "build"), (Insertion, " "), (Insertion, "info"), @@ -761,7 +799,7 @@ mod tests { #[test] fn test_infer_edits_14() { - assert_paired_edits( + assert_edits( vec!["a b c d ", "p "], vec!["x y c z ", "q r"], ( @@ -783,7 +821,8 @@ mod tests { (Insertion, "x"), (Insertion, " "), (Insertion, "y"), - (PlusNoop, " c "), + (PlusNoop, " c"), + (Insertion, " "), (Insertion, "z"), (PlusNoop, " "), ], @@ -795,6 +834,7 @@ mod tests { ], ], ), + 1.0, ); } @@ -834,7 +874,8 @@ mod tests { vec![vec![ (PlusNoop, ""), (Insertion, "c "), - (PlusNoop, "a a a a a a "), + (PlusNoop, "a a a a a a"), + (Insertion, " "), (Insertion, "c"), (Insertion, " "), (Insertion, "c"), diff --git a/src/paint.rs b/src/paint.rs index 67bd58c79..ca35aa548 100644 --- a/src/paint.rs +++ b/src/paint.rs @@ -522,9 +522,15 @@ impl<'p> Painter<'p> { let line_has_emph_and_non_emph_sections = style_sections_contain_more_than_one_style(style_sections); let should_update_non_emph_styles = non_emph_style.is_some() && *line_has_homolog; - let is_whitespace_error = - whitespace_error_style.is_some() && is_whitespace_error(style_sections); - for (style, _) in style_sections.iter_mut() { + + // TODO: Git recognizes blank lines at end of file (blank-at-eof) + // as a whitespace error but delta does not yet. + // https://git-scm.com/docs/git-config#Documentation/git-config.txt-corewhitespace + let mut is_whitespace_error = whitespace_error_style.is_some(); + for (style, s) in style_sections.iter_mut().rev() { + if is_whitespace_error && !s.trim().is_empty() { + is_whitespace_error = false; + } // If the line as a whole constitutes a whitespace error then highlight this // section if either (a) it is an emph section, or (b) the line lacks any // emph/non-emph distinction. @@ -537,6 +543,9 @@ impl<'p> Painter<'p> { // Otherwise, update the style if this is a non-emph section that needs updating. else if should_update_non_emph_styles && !style.is_emph { *style = non_emph_style.unwrap(); + if is_whitespace_error { + *style = whitespace_error_style.unwrap(); + } } } } @@ -856,26 +865,6 @@ fn style_sections_contain_more_than_one_style(sections: &[(Style, &str)]) -> boo } } -/// True iff the line represented by `sections` constitutes a whitespace error. -// A line is a whitespace error iff it is non-empty and contains only whitespace -// characters. -// TODO: Git recognizes blank lines at end of file (blank-at-eof) -// as a whitespace error but delta does not yet. -// https://git-scm.com/docs/git-config#Documentation/git-config.txt-corewhitespace -fn is_whitespace_error(sections: &[(Style, &str)]) -> bool { - let mut any_chars = false; - for c in sections.iter().flat_map(|(_, s)| s.chars()) { - if c == '\n' { - return any_chars; - } else if c != ' ' && c != '\t' { - return false; - } else { - any_chars = true; - } - } - false -} - mod superimpose_style_sections { use syntect::highlighting::Style as SyntectStyle; diff --git a/src/style.rs b/src/style.rs index aaf427308..c52400c8a 100644 --- a/src/style.rs +++ b/src/style.rs @@ -125,6 +125,16 @@ impl Style { } } + #[cfg(test)] + pub fn get_matching_substring<'a>(&self, s: &'a str) -> Option<&'a str> { + for (parsed_style, parsed_str) in ansi::parse_style_sections(s) { + if ansi_term_style_equality(parsed_style, self.ansi_term_style) { + return Some(parsed_str); + } + } + None + } + pub fn to_painted_string(self) -> ansi_term::ANSIGenericString<'static, str> { self.paint(self.to_string()) } diff --git a/src/tests/ansi_test_utils.rs b/src/tests/ansi_test_utils.rs index 083cf58b6..37a7b69c7 100644 --- a/src/tests/ansi_test_utils.rs +++ b/src/tests/ansi_test_utils.rs @@ -8,6 +8,8 @@ pub mod ansi_test_utils { use crate::paint; use crate::style::Style; + // Check if `output[line_number]` start with `expected_prefix` + // Then check if the first style in the line is the `expected_style` pub fn assert_line_has_style( output: &str, line_number: usize, @@ -25,6 +27,50 @@ pub mod ansi_test_utils { )); } + // Check if `output[line_number]` start with `expected_prefix` + // Then check if it contains the `expected_substring` with the corresponding `expected_style` + // If the line contains multiples times the `expected_style`, will only compare with the first + // item found + pub fn assert_line_contain_substring_style( + output: &str, + line_number: usize, + expected_prefix: &str, + expected_substring: &str, + expected_style: &str, + config: &Config, + ) { + assert_eq!( + expected_substring, + _line_get_substring_matching_style( + output, + line_number, + expected_prefix, + expected_style, + config, + ) + .unwrap() + ); + } + + // Check if `output[line_number]` start with `expected_prefix` + // Then check if the line does not contains the `expected_style` + pub fn assert_line_does_not_contain_substring_style( + output: &str, + line_number: usize, + expected_prefix: &str, + expected_style: &str, + config: &Config, + ) { + assert!(_line_get_substring_matching_style( + output, + line_number, + expected_prefix, + expected_style, + config, + ) + .is_none()); + } + pub fn assert_line_does_not_have_style( output: &str, line_number: usize, @@ -150,6 +196,30 @@ pub mod ansi_test_utils { output_buffer } + fn _line_extract<'a>(output: &'a str, line_number: usize, expected_prefix: &str) -> &'a str { + let line = output.lines().nth(line_number).unwrap(); + assert!(ansi::strip_ansi_codes(line).starts_with(expected_prefix)); + line + } + + fn _line_get_substring_matching_style<'a>( + output: &'a str, + line_number: usize, + expected_prefix: &str, + expected_style: &str, + config: &Config, + ) -> Option<&'a str> { + let line = _line_extract(output, line_number, expected_prefix); + let style = Style::from_str( + expected_style, + None, + None, + config.true_color, + config.git_config.as_ref(), + ); + style.get_matching_substring(line) + } + fn _line_has_style( output: &str, line_number: usize, @@ -158,8 +228,7 @@ pub mod ansi_test_utils { config: &Config, _4_bit_color: bool, ) -> bool { - let line = output.lines().nth(line_number).unwrap(); - assert!(ansi::strip_ansi_codes(line).starts_with(expected_prefix)); + let line = _line_extract(output, line_number, expected_prefix); let mut style = Style::from_str( expected_style, None, diff --git a/src/tests/test_example_diffs.rs b/src/tests/test_example_diffs.rs index 2fca095cc..71203f721 100644 --- a/src/tests/test_example_diffs.rs +++ b/src/tests/test_example_diffs.rs @@ -1462,6 +1462,182 @@ src/align.rs:71: impl<'a> Alignment<'a> { │ ); } + #[test] + fn test_whitespace_unrelated_edit_text_error() { + let whitespace_error_style = "bold yellow red ul"; + let config = integration_test_utils::make_config_from_args(&[ + "--whitespace-error-style", + whitespace_error_style, + ]); + let output = + integration_test_utils::run_delta(DIFF_WITH_WHITESPACE_UNRELATED_EDIT_ERROR, &config); + ansi_test_utils::assert_line_contain_substring_style( + &output, + 9, + "some new", + " ", + whitespace_error_style, + &config, + ); + } + + #[test] + fn test_whitespace_edit_text_error() { + let whitespace_error_style = "bold yellow red ul"; + let config = integration_test_utils::make_config_from_args(&[ + "--whitespace-error-style", + whitespace_error_style, + ]); + let output = integration_test_utils::run_delta(DIFF_WITH_WHITESPACE_EDIT_ERROR, &config); + ansi_test_utils::assert_line_contain_substring_style( + &output, + 9, + "same ", + " ", + whitespace_error_style, + &config, + ); + } + + #[test] + fn test_whitespace_added_empty_line_error() { + let whitespace_error_style = "bold yellow red ul"; + let config = integration_test_utils::make_config_from_args(&[ + "--whitespace-error-style", + whitespace_error_style, + ]); + let output = + integration_test_utils::run_delta(DIFF_WITH_ADDED_WHITESPACE_EMPTY_LINE_ERROR, &config); + // TODO is this the first style ? + ansi_test_utils::assert_line_has_style(&output, 9, " ", whitespace_error_style, &config); + ansi_test_utils::assert_line_contain_substring_style( + &output, + 9, + "", + " ", + whitespace_error_style, + &config, + ); + } + + #[test] + fn test_whitespace_after_text_error() { + let whitespace_error_style = "bold yellow red ul"; + let config = integration_test_utils::make_config_from_args(&[ + "--whitespace-error-style", + whitespace_error_style, + ]); + let output = + integration_test_utils::run_delta(DIFF_WITH_WHITESPACE_AFTER_TEXT_ERROR, &config); + ansi_test_utils::assert_line_contain_substring_style( + &output, + 8, + "foo bar", + " ", + whitespace_error_style, + &config, + ); + let output = integration_test_utils::run_delta( + DIFF_WITH_REMOVED_WHITESPACE_AFTER_TEXT_ERROR, + &config, + ); + ansi_test_utils::assert_line_does_not_contain_substring_style( + &output, + 8, + "foo bar", + whitespace_error_style, + &config, + ); + } + + #[test] + fn test_whitespace_complex_error() { + let whitespace_error_style = "bold yellow red ul"; + let config = integration_test_utils::make_config_from_args(&[ + "--whitespace-error-style", + whitespace_error_style, + ]); + let output = integration_test_utils::run_delta(DIFF_WITH_WHITESPACE_COMPLEX_ERROR, &config); + // `minus` line should not display whitespace error + ansi_test_utils::assert_line_does_not_have_style( + &output, + 8, + " ", + whitespace_error_style, + &config, + ); + ansi_test_utils::assert_line_does_not_have_style( + &output, + 9, + " ", + whitespace_error_style, + &config, + ); + ansi_test_utils::assert_line_does_not_contain_substring_style( + &output, + 10, + " foo0 ", + whitespace_error_style, + &config, + ); + ansi_test_utils::assert_line_does_not_contain_substring_style( + &output, + 11, + " foo1 ", + whitespace_error_style, + &config, + ); + ansi_test_utils::assert_line_does_not_contain_substring_style( + &output, + 12, + " bar ", + whitespace_error_style, + &config, + ); + + // `plus` line should display whitespace error + ansi_test_utils::assert_line_contain_substring_style( + &output, + 13, + " ", + " ", + whitespace_error_style, + &config, + ); + ansi_test_utils::assert_line_contain_substring_style( + &output, + 14, + " ", + " ", + whitespace_error_style, + &config, + ); + ansi_test_utils::assert_line_contain_substring_style( + &output, + 15, + " foo0", + " ", + whitespace_error_style, + &config, + ); + ansi_test_utils::assert_line_contain_substring_style( + &output, + 16, + " foo1", + " ", + whitespace_error_style, + &config, + ); + ansi_test_utils::assert_line_contain_substring_style( + &output, + 17, + " bAr", + " ", + whitespace_error_style, + &config, + ); + } + #[test] fn test_added_empty_line_is_not_whitespace_error() { let plus_style = "bold yellow red ul"; @@ -2268,6 +2444,77 @@ index 8d1c8b6..8b13789 100644 @@ -1 +1 @@ - + +"; + + const DIFF_WITH_WHITESPACE_UNRELATED_EDIT_ERROR: &str = r" +diff --git a/foo b/foo +index 8d1c8b6..8b13789 100644 +--- a/foo ++++ b/foo +@@ -1 +1 @@ +-some line with trailing spaces ++some new line with trailing spaces +"; + + const DIFF_WITH_WHITESPACE_EDIT_ERROR: &str = r" +diff --git a/foo b/foo +index 8d1c8b6..8b13789 100644 +--- a/foo ++++ b/foo +@@ -1 +1 @@ +-same line with different number of trailing spaces ++same line with different number of trailing spaces +"; + + const DIFF_WITH_WHITESPACE_AFTER_TEXT_ERROR: &str = r" +diff --git c/a i/a +new file mode 100644 +index 0000000..8d1c8b6 +--- /dev/null ++++ i/a +@@ -0,0 +1 @@ ++foo bar +"; + + const DIFF_WITH_REMOVED_WHITESPACE_AFTER_TEXT_ERROR: &str = r" +diff --git i/a w/a +index 8d1c8b6..8b13789 100644 +--- i/a ++++ w/a +@@ -1 +0,0 @@ +-foo bar +"; + const DIFF_WITH_ADDED_WHITESPACE_EMPTY_LINE_ERROR: &str = r" +diff --git a/a b/a +index 0ec702f..8c75341 100644 +--- a/a ++++ b/a +@@ -1,0 +1,0 @@ +- ++ +"; + + // Delta handling is different for each of theses cases: + // * Only space in the line is added or partially removed + // * Space after text added or partially removed + // * Space in a unmodified part of the line + // This test regroup theses 5 cases. + const DIFF_WITH_WHITESPACE_COMPLEX_ERROR: &str = r" +diff --git a/a b/a +index 0ec702f..8c75341 100644 +--- a/a ++++ b/a +@@ -1,5 +1,5 @@ +- +- +- foo0 +- foo1 +- bar ++ ++ ++ foo0 ++ foo1 ++ bAr "; const DIFF_WITH_TWO_ADDED_LINES: &str = r#"