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

Move line number into hunk header

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

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,

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");
     }
 

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(())
 }

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>."),
         }
     }

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,

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;

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) {