Repository: dandavison/delta
Pull request #333 (2020-09-23 23:59:06)

Refactor handle_hunk_header_line and should_handle
Related to https://github.com/dandavison/delta/pull/331, and related to simplifying handle_hunk_header_line.

~~It used to handle whitespaces of box from handler in delta.rs, not from write_boxed in draw.rs.~~

~~This causes a whitespaces into hunk-header when `line_numbers=true and hunk-header-style=raw`, and while I'm simplifying the code, it makes assert_eq tests failed. So I decided to refactor this part. In any cases you draw a box, you always insert a whitespace after the last character. Therefore, we can move this whitespace handle into draw.rs.~~

~~This could prevent us from adding whitespaces or using pad flag at every time we want to write_box.~~

↑
This refactoring whitespaces was wrong approach.
Now I just refactored handle_hunk_header_line and should_handle.

dandavison: Hi @ryuta69, is this PR intended to change the behavior of delta or is it just a refactor?

Looking at git blame takes me to this commit: https://github.com/dandavison/delta/commit/470fb46a0a23052ea40ddc0906175d9ca54a18cb "Bug fix: hunk header background color was not reaching box line" and it looks like this PR does add blank space at the end of the hunk header, e.g. compare

```
git show | delta --hunk-header-style='blue "#dddddd"'
```

before and after this commit.

before
![image](https://user-images.githubusercontent.com/52205/94302088-a8e4b100-ff39-11ea-8b61-8895d968b124.png)

after:
![image](https://user-images.githubusercontent.com/52205/94302104-b0a45580-ff39-11ea-8c72-a3fee22b4e25.png)

I like the code simplification but can you clarify whether you're intending to make this change to behavior?

ryuta69: @dandavison 
Thank you for reviewing, actually I didn't intend that changing behavior.

I actually didn't notice that commit, but looked https://github.com/dandavison/delta/commit/0d593c2d one. 

So this PR shouldn't get merged, but I want you to see my purpose, and decide which way is better.

Please look for this https://github.com/ryuta69/delta/pull/2. I wrote the detail about my purpose and the two plans of simplifying.

1. Simplify the `should_handle` and `pad`, but causes changing behavior https://github.com/dandavison/delta/pull/333#issuecomment-699079852 .
2. Simplify the `should_handle` and not causing changing behavior, but needs to add `pad` in handle-hunk-header-line.

Right now I think 2nd one makes sense because it should have had a `pad` anyway.

I'll change this PR to the plan you choose.

Patch:
diff --git a/src/bat/output.rs b/src/bat/output.rs
index 3f53ca9..b8a3865 100644
--- a/src/bat/output.rs
+++ b/src/bat/output.rs
@@ -50,15 +50,18 @@ impl OutputType {
     ) -> Result<Self> {
         let mut replace_arguments_to_less = false;
 
-        let pager_from_env = match (env::get_env_var("BAT_PAGER"), env::get_env_var("PAGER")) {
-            (Some(bat_pager), _) => Some(bat_pager),
-            (_, Some(pager)) => {
-                // less needs to be called with the '-R' option in order to properly interpret the
-                // ANSI color sequences printed by bat. If someone has set PAGER="less -F", we
-                // therefore need to overwrite the arguments and add '-R'.
-                //
-                // We only do this for PAGER (as it is not specific to 'bat'), not for BAT_PAGER
-                // or bats '--pager' command line option.
+        let pager_from_env = match (
+            env::get_env_var("DELTA_PAGER"),
+            env::get_env_var("BAT_PAGER"),
+            env::get_env_var("PAGER"),
+        ) {
+            (Some(delta_pager), _, _) => Some(delta_pager),
+            (None, Some(bat_pager), _) => Some(bat_pager),
+            (None, None, Some(pager)) => {
+                // less needs to be called with the '-R' option in order to properly interpret ANSI
+                // color sequences. If someone has set PAGER="less -F", we therefore need to
+                // overwrite the arguments and add '-R'.
+                // We only do this for PAGER, since it is used in other contexts.
                 replace_arguments_to_less = true;
                 Some(pager)
             }
diff --git a/src/cli.rs b/src/cli.rs
index 6c29f9d..863b2af 100644
--- a/src/cli.rs
+++ b/src/cli.rs
@@ -477,8 +477,8 @@ pub struct Opt {
     pub inspect_raw_lines: String,
 
     /// Whether to use a pager when displaying output. Options are: auto, always, and never. The
-    /// default pager is `less`: this can be altered by setting the environment variables BAT_PAGER
-    /// or PAGER (BAT_PAGER has priority).
+    /// default pager is `less`: this can be altered by setting the environment variables
+    /// DELTA_PAGER, BAT_PAGER, or PAGER (and that is their order of priority).
     #[structopt(long = "paging", default_value = "auto")]
     pub paging_mode: String,
 
diff --git a/src/delta.rs b/src/delta.rs
index 0a8f151..c0ad1ed 100644
--- a/src/delta.rs
+++ b/src/delta.rs
@@ -136,11 +136,9 @@ where
             painter.paint_buffered_minus_and_plus_lines();
             state = State::HunkHeader;
             painter.set_highlighter();
-            if should_handle(&state, config) {
-                painter.emit()?;
-                handle_hunk_header_line(&mut painter, &line, &raw_line, &plus_file, config)?;
-                continue;
-            }
+            painter.emit()?;
+            handle_hunk_header_line(&mut painter, &line, &raw_line, &plus_file, config)?;
+            continue;
         } else if source == Source::DiffUnified && line.starts_with("Only in ")
             || line.starts_with("Submodule ")
             || line.starts_with("Binary files ")
@@ -193,9 +191,6 @@ where
 
 /// Should a handle_* function be called on this element?
 fn should_handle(state: &State, config: &Config) -> bool {
-    if *state == State::HunkHeader && config.line_numbers {
-        return true;
-    }
     let style = config.get_style(state);
     !(style.is_raw && style.decoration_style == DecorationStyle::NoDecoration)
 }
@@ -381,20 +376,25 @@ fn handle_hunk_header_line(
     config: &Config,
 ) -> std::io::Result<()> {
     let decoration_ansi_term_style;
+    let mut pad = false;
     let draw_fn = match config.hunk_header_style.decoration_style {
         DecorationStyle::Box(style) => {
+            pad = true;
             decoration_ansi_term_style = style;
             draw::write_boxed
         }
         DecorationStyle::BoxWithUnderline(style) => {
+            pad = true;
             decoration_ansi_term_style = style;
             draw::write_boxed_with_underline
         }
         DecorationStyle::BoxWithOverline(style) => {
+            pad = true;
             decoration_ansi_term_style = style;
             draw::write_boxed // TODO: not implemented
         }
         DecorationStyle::BoxWithUnderOverline(style) => {
+            pad = true;
             decoration_ansi_term_style = style;
             draw::write_boxed // TODO: not implemented
         }
@@ -423,8 +423,8 @@ fn handle_hunk_header_line(
         }
         draw_fn(
             painter.writer,
-            &format!("{} ", line),
-            &format!("{} ", raw_line),
+            &format!("{}{}", line, if pad { " " } else { "" }),
+            &format!("{}{}", raw_line, if pad { " " } else { "" }),
             &config.decorations_width,
             config.hunk_header_style,
             decoration_ansi_term_style,
diff --git a/src/main.rs b/src/main.rs
index d293785..a691c33 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -102,19 +102,24 @@ fn diff(
         eprintln!("Usage: delta minus_file plus_file");
         process::exit(1);
     };
-    let diff_process = process::Command::new(PathBuf::from("diff"))
+    let command = "diff";
+    let diff_process = process::Command::new(PathBuf::from(command))
         .arg("-u")
         .args(&[
             minus_file.unwrap_or_else(die),
             plus_file.unwrap_or_else(die),
         ])
         .stdout(process::Stdio::piped())
-        .spawn();
+        .spawn()
+        .unwrap_or_else(|err| {
+            eprintln!("Failed to execute the command '{}': {}", command, err);
+            process::exit(1);
+        });
 
     let mut output_type = OutputType::from_mode(config.paging_mode, None, &config).unwrap();
     let mut writer = output_type.handle().unwrap();
     if let Err(error) = delta(
-        BufReader::new(diff_process.unwrap().stdout.unwrap()).byte_lines(),
+        BufReader::new(diff_process.stdout.unwrap()).byte_lines(),
         &mut writer,
         &config,
     ) {
diff --git a/src/options/theme.rs b/src/options/theme.rs
index 8dc235e..909b29f 100644
--- a/src/options/theme.rs
+++ b/src/options/theme.rs
@@ -65,7 +65,7 @@ fn is_no_syntax_highlighting_syntax_theme_name(theme_name: &str) -> bool {
 ///
 /// Basically:
 /// 1. The theme is specified by the `--theme` option. If this isn't supplied then it is specified
-///    by the `BAT_PAGER` environment variable.
+///    by the `BAT_THEME` environment variable.
 /// 2. Light vs dark mode is specified by the `--light` or `--dark` options. If these aren't
 ///    supplied then it is inferred from the chosen theme.
 ///


Test patch:
diff --git a/src/tests/test_example_diffs.rs b/src/tests/test_example_diffs.rs
index 9332b21..94a100b 100644
--- a/src/tests/test_example_diffs.rs
+++ b/src/tests/test_example_diffs.rs
@@ -891,9 +891,7 @@ src/align.rs
             } else {
                 output_lines[n]
             };
-            // TODO: this trim() can be removed by simplifing width_boxed of draw_fn.
-            assert_eq!(input_line.trim(), output_line.trim());
-            // assert_eq!(input_line, output_line);
+            assert_eq!(input_line, output_line);
         }
     }