Merge pull request #473 from dandavison/move-line-number

Move line number into hunk header

9 files changed, 191 insertions, 132 deletions

diff --git a/src/cli.rs b/src/cli.rs
index 22e3de2b..cd1555df 100644
--- a/src/cli.rs
+++ b/src/cli.rs
@@ -363,10 +363,13 @@ pub struct Opt {
     #[structopt(long = "hyperlinks-file-link-format", default_value = "file://{path}")]
     pub hyperlinks_file_link_format: String,
 
-    #[structopt(long = "hunk-header-style", default_value = "syntax")]
-    /// Style (foreground, background, attributes) for the hunk-header. See STYLES section. The
-    /// special attribute 'file' can be used to include the file path in the hunk header. The style
-    /// 'omit' can be used to remove the hunk header section from the output.
+    #[structopt(long = "hunk-header-style", default_value = "line-number syntax")]
+    /// Style (foreground, background, attributes) for the hunk-header. See STYLES section. Special
+    /// attributes 'file' and 'line-number' can be used to include the file path, and number of
+    /// first hunk line, in the hunk header. If included in the hunk header, 'file' and
+    /// 'line-number' are styled according to 'file-style' and 'hunk-header-decoration-style'
+    /// respectively. The style 'omit' can be used to remove the hunk header section from the
+    /// output.
     pub hunk_header_style: String,
 
     #[structopt(long = "hunk-header-decoration-style", default_value = "blue box")]
@@ -578,7 +581,6 @@ pub struct ComputedValues {
     pub decorations_width: Width,
     pub inspect_raw_lines: InspectRawLines,
     pub is_light_mode: bool,
-    pub line_numbers_mode: LineNumbersMode,
     pub paging_mode: PagingMode,
     pub syntax_dummy_theme: SyntaxTheme,
     pub syntax_set: SyntaxSet,
@@ -610,19 +612,6 @@ impl Default for InspectRawLines {
     }
 }
 
-#[derive(Clone, Debug, PartialEq)]
-pub enum LineNumbersMode {
-    None,
-    First,
-    Full,
-}
-
-impl Default for LineNumbersMode {
-    fn default() -> Self {
-        LineNumbersMode::First
-    }
-}
-
 impl Default for PagingMode {
     fn default() -> Self {
         PagingMode::Never
diff --git a/src/config.rs b/src/config.rs
index 6d47ac59..7331a12c 100644
--- a/src/config.rs
+++ b/src/config.rs
@@ -33,6 +33,7 @@ pub struct Config {
     pub git_config_entries: HashMap<String, GitConfigEntry>,
     pub hunk_header_style: Style,
     pub hunk_header_style_include_file_path: bool,
+    pub hunk_header_style_include_line_number: bool,
     pub hyperlinks: bool,
     pub hyperlinks_file_link_format: String,
     pub inspect_raw_lines: cli::InspectRawLines,
@@ -44,7 +45,6 @@ pub struct Config {
     pub line_numbers_plus_style: Style,
     pub line_numbers_right_format: String,
     pub line_numbers_right_style: Style,
-    pub line_numbers_show_first_line_number: bool,
     pub line_numbers_zero_style: Style,
     pub line_buffer_size: usize,
     pub max_line_distance: f64,
@@ -168,19 +168,21 @@ impl From<cli::Opt> for Config {
                 .hunk_header_style
                 .split(' ')
                 .any(|s| s == "file"),
+            hunk_header_style_include_line_number: opt
+                .hunk_header_style
+                .split(' ')
+                .any(|s| s == "line-number"),
             hyperlinks: opt.hyperlinks,
             hyperlinks_file_link_format: opt.hyperlinks_file_link_format,
             inspect_raw_lines: opt.computed.inspect_raw_lines,
             keep_plus_minus_markers: opt.keep_plus_minus_markers,
-            line_numbers: (opt.computed.line_numbers_mode == cli::LineNumbersMode::Full),
+            line_numbers: opt.line_numbers,
             line_numbers_left_format: opt.line_numbers_left_format,
             line_numbers_left_style,
             line_numbers_minus_style,
             line_numbers_plus_style,
             line_numbers_right_format: opt.line_numbers_right_format,
             line_numbers_right_style,
-            line_numbers_show_first_line_number: (opt.computed.line_numbers_mode
-                == cli::LineNumbersMode::First),
             line_numbers_zero_style,
             line_buffer_size: opt.line_buffer_size,
             max_line_distance: opt.max_line_distance,
diff --git a/src/features/diff_so_fancy.rs b/src/features/diff_so_fancy.rs
index cdc0c359..3898dac1 100644
--- a/src/features/diff_so_fancy.rs
+++ b/src/features/diff_so_fancy.rs
@@ -38,7 +38,7 @@ pub fn make_feature() -> Vec<(String, OptionValueFunction)> {
             "hunk-header-style",
             String,
             Some("color.diff.frag"),
-            _opt => "bold syntax"
+            _opt => "file line-number bold syntax"
         ),
         (
             "hunk-header-decoration-style",
@@ -70,7 +70,7 @@ pub mod tests {
         assert_eq!(opt.file_style, "11");
         assert_eq!(opt.file_decoration_style, "bold yellow ul ol");
 
-        assert_eq!(opt.hunk_header_style, "bold syntax");
+        assert_eq!(opt.hunk_header_style, "file line-number bold syntax");
         assert_eq!(opt.hunk_header_decoration_style, "magenta box");
     }
 
diff --git a/src/hunk_header.rs b/src/hunk_header.rs
index abf042ca..2f54dd79 100644
--- a/src/hunk_header.rs
+++ b/src/hunk_header.rs
@@ -31,18 +31,15 @@ pub fn handle_hunk_header_line(
     } else if config.hunk_header_style.is_omitted {
         writeln!(painter.writer)?;
     } else {
-        _write_hunk_header(&raw_code_fragment, painter, line, plus_file, config)?;
+        _write_hunk_header(
+            &raw_code_fragment,
+            &line_numbers,
+            painter,
+            line,
+            plus_file,
+            config,
+        )?;
     };
-
-    // Do not emit a line number in color-only mode, since the extra line would break the
-    // requirement for output lines to be in one-to-one correspondence with input lines.
-    if !config.line_numbers
-        && config.line_numbers_show_first_line_number
-        && !config.hunk_header_style.is_raw
-        && !config.color_only
-    {
-        _write_line_number(&line_numbers, painter, plus_file, config)?;
-    }
     Ok(())
 }
 
@@ -69,6 +66,7 @@ fn _write_hunk_header_raw(
 
 fn _write_hunk_header(
     raw_code_fragment: &str,
+    line_numbers: &Vec<(usize, usize)>,
     painter: &mut Painter,
     line: &str,
     plus_file: &str,
@@ -93,13 +91,26 @@ fn _write_hunk_header(
     if config.hunk_header_style_include_file_path {
         let _ = write!(
             &mut painter.output_buffer,
-            "{}{} ",
+            "{}",
             config.file_style.paint(plus_file),
-            if line.is_empty() { "" } else { ":" },
         );
         have_hunk_header = true;
     };
+    if !config.line_numbers
+        && config.hunk_header_style_include_line_number
+        && !config.hunk_header_style.is_raw
+        && !config.color_only
+    {
+        if have_hunk_header {
+            let _ = write!(&mut painter.output_buffer, ":");
+        }
+        _write_line_number(&line_numbers, painter, plus_file, config)?;
+        have_hunk_header = true;
+    }
     if !line.is_empty() {
+        if have_hunk_header {
+            let _ = write!(&mut painter.output_buffer, ": ");
+        }
         let lines = vec![(line, delta::State::HunkHeader)];
         let syntax_style_sections = Painter::get_syntax_style_sections_for_lines(
             &lines,
@@ -120,6 +131,8 @@ fn _write_hunk_header(
         );
         painter.output_buffer.pop(); // trim newline
         have_hunk_header = true;
+    } else if have_hunk_header {
+        let _ = write!(&mut painter.output_buffer, " ");
     }
     if have_hunk_header {
         draw_fn(
@@ -196,12 +209,16 @@ fn _write_line_number(
         Cow::from(format!("{}", plus_line_number))
     };
     match config.hunk_header_style.decoration_ansi_term_style() {
-        Some(style) => writeln!(
-            painter.writer,
-            "{}",
-            style.paint(formatted_plus_line_number)
-        )?,
-        None => writeln!(painter.writer, "{}", formatted_plus_line_number)?,
+        Some(style) => {
+            let _ = write!(
+                &mut painter.output_buffer,
+                "{}",
+                style.paint(formatted_plus_line_number)
+            );
+        }
+        None => {
+            let _ = write!(&mut painter.output_buffer, "{}", formatted_plus_line_number);
+        }
     }
     Ok(())
 }
diff --git a/src/options/option_value.rs b/src/options/option_value.rs
index d569f676..cbe5b383 100644
--- a/src/options/option_value.rs
+++ b/src/options/option_value.rs
@@ -55,13 +55,6 @@ impl From<OptionValue> for Option<String> {
     fn from(value: OptionValue) -> Self {
         match value {
             OptionValue::OptionString(value) => value,
-            // HACK: See the comment in options::set::compute_line_numbers_mode(). That function
-            // deliberately reads what is normally a boolean value ('line-numbers') as a string.
-            // However options::get::get_option_value() can fall through to obtaining the value
-            // from builtin_features, in which case an OptionValue::Boolean will be encountered.
-            // See the comment in options::set::compute_line_numbers_mode() and docstring of
-            // options::get::get_option_value().
-            OptionValue::Boolean(_) => None,
             _ => delta_unreachable("Error converting OptionValue to Option<String>."),
         }
     }
diff --git a/src/options/set.rs b/src/options/set.rs
index 5b68ffec..cebdd2f7 100644
--- a/src/options/set.rs
+++ b/src/options/set.rs
@@ -16,7 +16,7 @@ use crate::features;
 use crate::git_config;
 use crate::git_config_entry::{self, GitConfigEntry};
 use crate::options::option_value::{OptionValue, ProvenancedOptionValue};
-use crate::options::{self, theme};
+use crate::options::theme;
 
 macro_rules! set_options {
     ([$( $field_ident:ident ),* ],
@@ -186,8 +186,6 @@ pub fn set_options(
 
     opt.computed.inspect_raw_lines =
         cli::InspectRawLines::from_str(&opt.inspect_raw_lines).unwrap();
-    opt.computed.line_numbers_mode =
-        compute_line_numbers_mode(opt, &builtin_features, git_config, &option_names);
     opt.computed.paging_mode = parse_paging_mode(&opt.paging_mode);
 
     // --color-only is used for interactive.diffFilter (git add -p). side-by-side, and
@@ -201,31 +199,6 @@ pub fn set_options(
     }
 }
 
-fn compute_line_numbers_mode(
-    opt: &cli::Opt,
-    builtin_features: &HashMap<String, features::BuiltinFeature>,
-    git_config: &mut Option<git_config::GitConfig>,
-    option_names: &HashMap<&str, &str>,
-) -> cli::LineNumbersMode {
-    // line-numbers is in general treated as a boolean value. We read it as a string here in order
-    // to interpret an explicit "false" (as opposed to merely absence) as meaning "Do not show any
-    // line numbers; not even the first line number of the hunk".
-    let line_numbers_string_value: Option<Option<String>> = options::get::get_option_value(
-        option_names["line-numbers"],
-        builtin_features,
-        opt,
-        git_config,
-    );
-    match (
-        line_numbers_string_value.as_ref().map(|val| val.as_deref()),
-        opt.line_numbers,
-    ) {
-        (Some(Some("false")), _) => cli::LineNumbersMode::None,
-        (_, true) => cli::LineNumbersMode::Full,
-        (_, false) => cli::LineNumbersMode::First,
-    }
-}
-
 #[allow(non_snake_case)]
 fn set__light__dark__syntax_theme__options(
     opt: &mut cli::Opt,
diff --git a/src/parse_style.rs b/src/parse_style.rs
index 1fa4fb03..6b7179a9 100644
--- a/src/parse_style.rs
+++ b/src/parse_style.rs
@@ -241,8 +241,8 @@ fn parse_ansi_term_style(
             style.is_strikethrough = true;
         } else if word == "ul" || word == "underline" {
             style.is_underline = true;
-        } else if word == "file" {
-            // Allow: this is meaningful in hunk-header-style.
+        } else if word == "line-number" || word == "file" {
+            // Allow: these are meaningful in hunk-header-style.
         } else if !seen_foreground {
             if word == "syntax" {
                 is_syntax_highlighted = true;
diff --git a/src/tests/ansi_test_utils.rs b/src/tests/ansi_test_utils.rs
index d0456d92..dd160569 100644
--- a/src/tests/ansi_test_utils.rs
+++ b/src/tests/ansi_test_utils.rs
@@ -66,21 +66,26 @@ pub mod ansi_test_utils {
         assert_eq!(line, stripped_line);
     }
 
-    /// Assert that the specified line number of output (a) matches
-    /// `expected_prefix` and (b) for the length of expected_prefix is
-    /// syntax-highlighted according to `language_extension`.
-    pub fn assert_line_is_syntax_highlighted(
+    /// Assert that the specified line number of output (a) has, after stripping ANSI codes, a
+    /// substring starting at `substring_begin` equal to `expected_substring` and (b) in its raw
+    /// form contains a version of that substring syntax-highlighted according to
+    /// `language_extension`.
+    pub fn assert_line_has_syntax_highlighted_substring(
         output: &str,
         line_number: usize,
-        expected_prefix: &str,
+        substring_begin: usize,
+        expected_substring: &str,
         language_extension: &str,
         state: State,
         config: &Config,
     ) {
         let line = output.lines().nth(line_number).unwrap();
-        let painted_line = paint_line(expected_prefix, language_extension, state, config);
+        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);
         // remove trailing newline appended by paint::paint_lines.
-        assert!(line.starts_with(painted_line.trim_end()));
+        assert!(line.contains(painted_substring.trim_end()));
     }
 
     pub fn assert_has_color_other_than_plus_color(string: &str, config: &Config) {
diff --git a/src/tests/test_example_diffs.rs b/src/tests/test_example_diffs.rs
index 94b18f11..402a88a5 100644
--- a/src/tests/test_example_diffs.rs
+++ b/src/tests/test_example_diffs.rs
@@ -74,7 +74,7 @@ mod tests {
         let config = integration_test_utils::make_config_from_args(&[]);
         let output = integration_test_utils::get_line_of_code_from_delta(
             &ADDED_FILE_INPUT,
-            12,
+            14,
             "class X:",
             &config,
         );
@@ -88,7 +88,7 @@ mod tests {
         let config = integration_test_utils::make_config_from_args(&[]);
         let input = ADDED_FILE_INPUT.replace("a.py", "a");
         let output =
-            integration_test_utils::get_line_of_code_from_delta(&input, 12, "class X:", &config);
+            integration_test_utils::get_line_of_code_from_delta(&input, 14, "class X:", &config);
         ansi_test_utils::assert_has_color_other_than_plus_color(&output, &config);
     }
 
@@ -104,7 +104,7 @@ mod tests {
         ]);
         let input = ADDED_FILE_INPUT.replace("a.py", "a");
         let output =
-            integration_test_utils::get_line_of_code_from_delta(&input, 12, "class X:", &config);
+            integration_test_utils::get_line_of_code_from_delta(&input, 14, "class X:", &config);
         ansi_test_utils::assert_has_plus_color_only(&output, &config);
     }
 
@@ -117,14 +117,10 @@ mod tests {
 
         // Header
         assert_eq!(lines.nth(1).unwrap(), "comparing: one.rs ⟶   src/two.rs");
-        // Line
-        assert_eq!(lines.nth(2).unwrap(), "5");
         // Change
-        assert_eq!(lines.nth(2).unwrap(), "println!(\"Hello ruster\");");
-        // Next chunk
-        assert_eq!(lines.nth(2).unwrap(), "43");
+        assert_eq!(lines.nth(7).unwrap(), "println!(\"Hello ruster\");");
         // Unchanged in second chunk
-        assert_eq!(lines.nth(2).unwrap(), "Unchanged");
+        assert_eq!(lines.nth(7).unwrap(), "Unchanged");
     }
 
     #[test]
@@ -139,10 +135,8 @@ mod tests {
             lines.nth(1).unwrap(),
             "comparing: a/different ⟶   b/different"
         );
-        // Line number
-        assert_eq!(lines.nth(2).unwrap(), "1");
         // Change
-        assert_eq!(lines.nth(2).unwrap(), "This is different from b");
+        assert_eq!(lines.nth(7).unwrap(), "This is different from b");
         // File uniqueness
         assert_eq!(lines.nth(2).unwrap(), "Only in a/: just_a");
         // FileMeta divider
@@ -196,7 +190,7 @@ mod tests {
         let output = integration_test_utils::run_delta(DIFF_WITH_MERGE_CONFLICT, &config);
         // TODO: The + in the first column is being removed.
         assert!(strip_ansi_codes(&output).contains("+>>>>>>> Stashed changes"));
-        assert_eq!(output.lines().count(), 46);
+        assert_eq!(output.lines().count(), 45);
     }
 
     #[test]
@@ -1014,7 +1008,7 @@ src/align.rs
             "--file-style",
             "yellow",
             "--hunk-header-style",
-            "file red",
+            "file line-number red",
             "--hunk-header-decoration-style",
             "box",
         ]);
@@ -1023,16 +1017,16 @@ src/align.rs
         ansi_test_utils::assert_line_has_style(
             &output,
             11,
-            "src/align.rs: impl<'a> Alignment<'a> {",
+            "src/align.rs:71: impl<'a> Alignment<'a> {",
             "yellow",
             &config,
         );
         let output = strip_ansi_codes(&output);
         assert!(output.contains(
             "
-───────────────────────────────────────┐
-src/align.rs: impl<'a> Alignment<'a> { │
-───────────────────────────────────────┘
+──────────────────────────────────────────┐
+src/align.rs:71: impl<'a> Alignment<'a> { │
+──────────────────────────────────────────┘
 "
         ));
     }
@@ -1043,19 +1037,19 @@ src/align.rs: impl<'a> Alignment<'a> { │
             "--file-style",
             "yellow",
             "--hunk-header-style",
-            "file red",
+            "file line-number red",
             "--hunk-header-decoration-style",
             "box",
         ]);
         let output = integration_test_utils::run_delta(GIT_DIFF_SINGLE_HUNK_NO_FRAG, &config);
 
-        ansi_test_utils::assert_line_has_style(&output, 5, "src/delta.rs", "yellow", &config);
+        ansi_test_utils::assert_line_has_style(&output, 5, "src/delta.rs:1 ", "yellow", &config);
         let output = strip_ansi_codes(&output);
         assert!(output.contains(
             "
-─────────────┐
-src/delta.rs │
-─────────────┘
+───────────────┐
+src/delta.rs:1 │
+───────────────┘
 "
         ));
     }
@@ -1084,7 +1078,7 @@ src/delta.rs │
     fn test_hunk_header_style_colored_input_color_is_stripped_under_normal() {
         let config = integration_test_utils::make_config_from_args(&[
             "--hunk-header-style",
-            "normal",
+            "line-number normal",
             "--hunk-header-decoration-style",
             "omit",
         ]);
@@ -1094,7 +1088,7 @@ src/delta.rs │
         );
         // An additional newline is inserted under anything other than `style=raw,
         // decoration-style=omit`, to better separate the hunks. Hence 9 + 1.
-        ansi_test_utils::assert_line_has_no_color(&output, 9 + 1, "impl<'a> Alignment<'a> {");
+        ansi_test_utils::assert_line_has_no_color(&output, 9 + 1, "71: impl<'a> Alignment<'a> {");
     }
 
     #[test]
@@ -1178,6 +1172,36 @@ impl<'a> Alignment<'a> {
     }
 
     #[test]
+    fn test_hunk_header_style_box_line_number() {
+        _do_test_hunk_header_style_box(&[
+            "--hunk-header-style",
+            "line-number",
+            "--hunk-header-decoration-style",
+            "white box",
+        ]);
+    }
+
+    #[test]
+    fn test_hunk_header_style_box_file_line_number() {
+        _do_test_hunk_header_style_box_file_line_number(&[
+            "--hunk-header-style",
+            "file line-number",
+            "--hunk-header-decoration-style",
+            "white box",
+        ]);
+    }
+
+    #[test]
+    fn test_hunk_header_style_box_file() {
+        _do_test_hunk_header_style_box_file(&[
+            "--hunk-header-style",
+            "file",
+            "--hunk-header-decoration-style",
+            "white box",
+        ]);
+    }
+
+    #[test]
     fn test_hunk_header_style_box_deprecated_options() {
         _do_test_hunk_header_style_box(&["--hunk-color", "white", "--hunk-style", "box"]);
     }
@@ -1188,23 +1212,77 @@ impl<'a> Alignment<'a> {
         ansi_test_utils::assert_line_has_style(
             &output,
             10,
-            "─────────────────────────┐",
+            "─────────────────────────────┐",
+            "white",
+            &config,
+        );
+        ansi_test_utils::assert_line_has_style(
+            &output,
+            12,
+            "─────────────────────────────┘",
+            "white",
+            &config,
+        );
+        let output = strip_ansi_codes(&output);
+        assert!(output.contains(
+            "