ronantakizawa/codereview-bench huggingface dataset:
Repository: sharkdp/bat
File: src/terminal.rs (Rust)

Code before:

pub fn to_ansi_color(color: highlighting::Color, true_color: bool) -> ansi_term::Colour {
    if true_color {
        RGB(color.r, color.g, color.b)
    } else {
        Fixed(ansi_colours::ansi256_from_rgb((color.r, color.g, color.b)))
    }
}

pub fn as_terminal_escaped(
    style: highlighting::Style,
    text: &str,
    true_color: bool,
    colored: bool,
    italics: bool,
) -> String {
    let style = if !colored {
        Style::default()
    } else {
        let color = to_ansi_color(style.foreground, true_color);

        if style.font_style.contains(FontStyle::BOLD) {
            color.bold()
        } else if style.font_style.contains(FontStyle::UNDERLINE) {
            color.underline()
        } else if style.font_style.contains(FontStyle::ITALIC) && italics {
            color.italic()
        } else {
            color.normal()
        }
    };

    style.paint(text).to_string()
}


Review comment (performance):
Since this is potentially performance-relevant, I'd suggest we use `else if italics && ...` here in order to profit from short-circuiting.

Code after:

pub fn to_ansi_color(color: highlighting::Color, true_color: bool) -> ansi_term::Colour {
    if true_color {
        RGB(color.r, color.g, color.b)
    } else {
        Fixed(ansi_colours::ansi256_from_rgb((color.r, color.g, color.b)))
    }
}

pub fn as_terminal_escaped(
    style: highlighting::Style,
    text: &str,
    true_color: bool,
    colored: bool,
    italics: bool,
) -> String {
    let style = if !colored {
        Style::default()
    } else {
        let color = to_ansi_color(style.foreground, true_color);

        if style.font_style.contains(FontStyle::BOLD) {
            color.bold()
        } else if style.font_style.contains(FontStyle::UNDERLINE) {
            color.underline()
        } else if italics && style.font_style.contains(FontStyle::ITALIC) {
            color.italic()
        } else {
            color.normal()
        }
    };

    style.paint(text).to_string()
}
Repository: sharkdp/bat
File: src/output.rs (Rust)

Code before:
            None => Ok(OutputType::stdout()),
        }
    }

    fn stdout() -> Self {
        OutputType::Stdout(io::stdout())
    }

    pub fn handle(&mut self) -> Result<&mut Write> {
        Ok(match *self {
            OutputType::Pager(ref mut command) => command
                .stdin
                .as_mut()
                .chain_err(|| "Could not open stdin for pager")?,
            OutputType::Stdout(ref mut handle) => handle,
        })
    }
}

// Since someone could set PAGER to less without arg -R that bat needs,
// ignore flags for less and change flags to -FRX.
// If a user wants an env variable with flags unchanged, he/she should use
// BAT_PAGER.
fn add_default_flags_to_less(pager: String) -> String {
    if let Ok(flags) = shell_words::split(&pager) {
        if flags.first() == Some(&String::from("less")) {
            return "less -FRX".to_string();
        }
    }

    pager.to_string()
}

impl Drop for OutputType {
    fn drop(&mut self) {
        if let OutputType::Pager(ref mut command) = *self {
            let _ = command.wait();
        }
    }
}


Review comment (suggestion):
This does not work if the first argument is `/usr/bin/less`, for example. See the `is_less` variable example in the same file.

Code after:
                        p.args(args);
                    }
                    p.env("LESSCHARSET", "UTF-8");
                    p
                } else {
                    let mut p = Command::new(&pager_path);
                    p.args(args);
                    p
                };

                Ok(process
                    .stdin(Stdio::piped())
                    .spawn()
                    .map(OutputType::Pager)
                    .unwrap_or_else(|_| OutputType::stdout()))
            }
            None => Ok(OutputType::stdout()),
        }
    }

    fn stdout() -> Self {
        OutputType::Stdout(io::stdout())
    }

    pub fn handle(&mut self) -> Result<&mut Write> {
        Ok(match *self {
            OutputType::Pager(ref mut command) => command
                .stdin
                .as_mut()
                .chain_err(|| "Could not open stdin for pager")?,
            OutputType::Stdout(ref mut handle) => handle,
        })
    }
}

impl Drop for OutputType {
    fn drop(&mut self) {
        if let OutputType::Pager(ref mut command) = *self {
            let _ = command.wait();
        }
    }
}
Repository: sharkdp/bat
File: assets/create.sh (Shell)

Code before:
#!/bin/bash
set -euo pipefail

ASSET_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"

# Ensure submodules are initialized.
git submodule init
git submodule update

# Always remove the local cache to avoid any confusion
bat cache --clear

# TODO: Remove this (and the reverse part below) when
# https://github.com/trishume/syntect/issues/222 has been fixed
JAVADOC_FILE="${ASSET_DIR}/syntaxes/Packages/Java/JavaDoc.sublime-syntax"
JAVADOC_PATCH="${ASSET_DIR}/JavaDoc.sublime-syntax.patch"
patch "$JAVADOC_FILE" "$JAVADOC_PATCH"

bat cache --build --blank --source="$ASSET_DIR" --target="$ASSET_DIR"

patch -R "$JAVADOC_FILE" "$JAVADOC_PATCH"


Review comment (suggestion):
To make this a little bit more robust, you could use `git -C "$ASSET_DIR" …`. Otherwise, it only works when the CWD is within the `bat` repository (and not within one of the submodules).

Code after:
#!/bin/bash
set -euo pipefail

ASSET_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
REPO_DIR="$ASSET_DIR/.."

# Ensure submodules are initialized.
function update_submodules() {
	local submodule
	local submodule_prompt=unspecified
	local submodule_path

	{
		while { read -r submodule && read -r submodule_path; } <&3; do
			if ! [[ -d "${REPO_DIR}/.git/modules/${submodule}" ]] && [[ -d "${REPO_DIR}/${submodule_path}" ]]; then
				if [[ "$submodule_prompt" = "unspecified" ]]; then
					echo "One or more submodules were found to be uninitialized."
					printf "Initialize and update them? [Y/n] "
					read -r submodule_prompt
				fi

				case "$submodule_prompt" in
					y|yes|'') {
						git -C "$REPO_DIR" submodule update --init "$submodule_path"
					};;
					n|no) {
						return
					};;
					*) {
						echo "Unknown answer. Not updating submodules."
					};;
				esac
Repository: sharkdp/bat
File: src/dirs.rs (Rust)

Code before:
    fn new() -> Option<BatProjectDirs> {
        let cache_dir = BatProjectDirs::get_cache_dir()?;

        #[cfg(target_os = "macos")]
        let config_dir_op = env::var_os("XDG_CONFIG_HOME")
            .map(PathBuf::from)
            .filter(|p| p.is_absolute())
            .or_else(|| dirs_rs::home_dir().map(|d| d.join(".config")));

        #[cfg(not(target_os = "macos"))]
        let config_dir_op = dirs_rs::config_dir();

        let config_dir = config_dir_op.map(|d| d.join("bat"))?;

        Some(BatProjectDirs {
            cache_dir,
            config_dir,
        })
    }

    pub fn get_cache_dir() -> Option<PathBuf> {
        // on all OS prefer BAT_CACHE_PATH if set
        let cache_dir_op = env::var_os("BAT_CACHE_PATH")
            .map(PathBuf::from)
            .filter(|p| p.is_absolute());
        if !cache_dir_op.is_none() {
            return cache_dir_op;
        }

        #[cfg(target_os = "macos")]
        let cache_dir_op = env::var_os("XDG_CACHE_HOME")
            .map(PathBuf::from)
            .filter(|p| p.is_absolute())
            .or_else(|| dirs_rs::home_dir().map(|d| d.join(".cache")));

        #[cfg(not(target_os = "macos"))]
        let cache_dir_op = dirs_rs::cache_dir();

        Some(cache_dir_op.map(|d| d.join("bat")))?
    }

    pub fn cache_dir(&self) -> &Path {
        &self.cache_dir
    }

    pub fn config_dir(&self) -> &Path {
        &self.config_dir
    }
}

lazy_static! {

Review comment (performance):
you can use:
```rust
if cache_dir_op.is_some() {
  // …
```

Code after:
    fn new() -> Option<BatProjectDirs> {
        let cache_dir = BatProjectDirs::get_cache_dir()?;

        #[cfg(target_os = "macos")]
        let config_dir_op = env::var_os("XDG_CONFIG_HOME")
            .map(PathBuf::from)
            .filter(|p| p.is_absolute())
            .or_else(|| dirs_rs::home_dir().map(|d| d.join(".config")));

        #[cfg(not(target_os = "macos"))]
        let config_dir_op = dirs_rs::config_dir();

        let config_dir = config_dir_op.map(|d| d.join("bat"))?;

        Some(BatProjectDirs {
            cache_dir,
            config_dir,
        })
    }

    pub fn get_cache_dir() -> Option<PathBuf> {
        // on all OS prefer BAT_CACHE_PATH if set
        let cache_dir_op = env::var_os("BAT_CACHE_PATH").map(PathBuf::from);
        if cache_dir_op.is_some() {
            return cache_dir_op;
        }

        #[cfg(target_os = "macos")]
        let cache_dir_op = env::var_os("XDG_CACHE_HOME")
            .map(PathBuf::from)
            .filter(|p| p.is_absolute())
            .or_else(|| dirs_rs::home_dir().map(|d| d.join(".cache")));

        #[cfg(not(target_os = "macos"))]
        let cache_dir_op = dirs_rs::cache_dir();

        cache_dir_op.map(|d| d.join("bat"))
    }

    pub fn cache_dir(&self) -> &Path {
        &self.cache_dir
    }

    pub fn config_dir(&self) -> &Path {
        &self.config_dir
    }
}

lazy_static! {
    pub static ref PROJECT_DIRS: BatProjectDirs =
        BatProjectDirs::new().expect("Could not get home directory");
Repository: sharkdp/bat
File: src/dirs.rs (Rust)

Code before:

        let config_dir = config_dir_op.map(|d| d.join("bat"))?;

        Some(BatProjectDirs {
            cache_dir,
            config_dir,
        })
    }

    pub fn get_cache_dir() -> Option<PathBuf> {
        // on all OS prefer BAT_CACHE_PATH if set
        let cache_dir_op = env::var_os("BAT_CACHE_PATH").map(PathBuf::from);
        if cache_dir_op.is_some() {
            return cache_dir_op;
        }

        #[cfg(target_os = "macos")]
        let cache_dir_op = env::var_os("XDG_CACHE_HOME")
            .map(PathBuf::from)
            .filter(|p| p.is_absolute())
            .or_else(|| dirs_rs::home_dir().map(|d| d.join(".cache")));

        #[cfg(not(target_os = "macos"))]
        let cache_dir_op = dirs_rs::cache_dir();

        Some(cache_dir_op.map(|d| d.join("bat")))?
    }

    pub fn cache_dir(&self) -> &Path {
        &self.cache_dir
    }

    pub fn config_dir(&self) -> &Path {
        &self.config_dir
    }
}

lazy_static! {
    pub static ref PROJECT_DIRS: BatProjectDirs =
        BatProjectDirs::new().expect("Could not get home directory");
}


Review comment (performance):
Couldn't this just be:
```rust
cache_dir_op.map(|d| d.join("bat"))
```

Using `Some(xx)?` is the same as `xx`, right?

Code after:

        let config_dir = config_dir_op.map(|d| d.join("bat"))?;

        Some(BatProjectDirs {
            cache_dir,
            config_dir,
        })
    }

    pub fn get_cache_dir() -> Option<PathBuf> {
        // on all OS prefer BAT_CACHE_PATH if set
        let cache_dir_op = env::var_os("BAT_CACHE_PATH").map(PathBuf::from);
        if cache_dir_op.is_some() {
            return cache_dir_op;
        }

        #[cfg(target_os = "macos")]
        let cache_dir_op = env::var_os("XDG_CACHE_HOME")
            .map(PathBuf::from)
            .filter(|p| p.is_absolute())
            .or_else(|| dirs_rs::home_dir().map(|d| d.join(".cache")));

        #[cfg(not(target_os = "macos"))]
        let cache_dir_op = dirs_rs::cache_dir();

        cache_dir_op.map(|d| d.join("bat"))
    }

    pub fn cache_dir(&self) -> &Path {
        &self.cache_dir
    }

    pub fn config_dir(&self) -> &Path {
        &self.config_dir
    }
}

lazy_static! {
    pub static ref PROJECT_DIRS: BatProjectDirs =
        BatProjectDirs::new().expect("Could not get home directory");
}
Repository: sharkdp/bat
File: src/bin/bat/config.rs (Rust)

Code before:
use std::env;
use std::ffi::OsString;
use std::fs;
use std::io::{self, Write};
use std::path::PathBuf;

use shell_words;

use crate::directories::PROJECT_DIRS;

pub fn config_file() -> PathBuf {
    env::var("BAT_CONFIG_PATH")
        .ok()
        .map(PathBuf::from)
        .filter(|config_path| config_path.is_file())
        .unwrap_or_else(|| PROJECT_DIRS.config_dir().join("config"))
}

pub fn generate_config_file() {
    let config_file = config_file();
    if config_file.exists() {
        println!("A config file already exists at: {}", config_file.to_string_lossy());

        print!("Overwrite? (y/n): ");
        let _ = io::stdout().flush();
        let mut decision = String::new();
        io::stdin().read_line(&mut decision).expect("Failed to read input");

        if !decision.trim().eq_ignore_ascii_case("Y") {
            return;
        }
    } else {
        let config_dir = config_file.parent().unwrap();
        if !config_dir.exists() {
            fs::create_dir(config_dir).expect("Unable to create config directory");
        }
    }

    let default_config = "# Specify desired theme (e.g. \"TwoDark\")
#--theme=\"TwoDark\"

# Enable this to use italic text on the terminal (not supported on all terminals):
#--italic-text=always


Review comment (suggestion):
In order to properly handle errors, this function should return a `bat::errors::Result<()>`. In `main.rs`, you would then simply call it with
```rust
generate_config_file()?;
```

Code after:
use std::env;
use std::ffi::OsString;
use std::fs;
use std::io::{self, Write};
use std::path::PathBuf;

use shell_words;

use crate::directories::PROJECT_DIRS;

pub fn config_file() -> PathBuf {
    env::var("BAT_CONFIG_PATH")
        .ok()
        .map(PathBuf::from)
        .filter(|config_path| config_path.is_file())
        .unwrap_or_else(|| PROJECT_DIRS.config_dir().join("config"))
}

pub fn generate_config_file() -> bat::errors::Result<()> {
    let config_file = config_file();
    if config_file.exists() {
        println!("A config file already exists at: {}", config_file.to_string_lossy());

        print!("Overwrite? (y/N): ");
        io::stdout().flush()?;
        let mut decision = String::new();
        io::stdin().read_line(&mut decision)?;

        if !decision.trim().eq_ignore_ascii_case("Y") {
            return Ok(());
        }
    } else {
        let config_dir = config_file.parent();
        match config_dir {
            Some(path) => fs::create_dir_all(path)?,
            None => return Ok(Err(format!("Unable to write config file to: {}", config_file.to_string_lossy()))?),
        }
    }

    let default_config = r#"# bat config

# Specify desired theme (e.g. "TwoDark").  Issue `bat --list-themes` for a list of all available themes
#--theme="TwoDark"
Repository: sharkdp/bat
File: src/bin/bat/config.rs (Rust)

Code before:
use std::env;
use std::ffi::OsString;
use std::fs;
use std::io::{self, Write};
use std::path::PathBuf;

use shell_words;

use crate::directories::PROJECT_DIRS;

pub fn config_file() -> PathBuf {
    env::var("BAT_CONFIG_PATH")
        .ok()
        .map(PathBuf::from)
        .filter(|config_path| config_path.is_file())
        .unwrap_or_else(|| PROJECT_DIRS.config_dir().join("config"))
}

pub fn generate_config_file() {
    let config_file = config_file();
    if config_file.exists() {
        println!("A config file already exists at: {}", config_file.to_string_lossy());

        print!("Overwrite? (y/n): ");
        let _ = io::stdout().flush();
        let mut decision = String::new();
        io::stdin().read_line(&mut decision).expect("Failed to read input");

        if !decision.trim().eq_ignore_ascii_case("Y") {
            return;
        }
    } else {
        let config_dir = config_file.parent().unwrap();
        if !config_dir.exists() {
            fs::create_dir(config_dir).expect("Unable to create config directory");
        }
    }

    let default_config = "# Specify desired theme (e.g. \"TwoDark\")
#--theme=\"TwoDark\"

# Enable this to use italic text on the terminal (not supported on all terminals):
#--italic-text=always

# Uncomment the following line to disable automatic paging:
#--paging=never

# Use C++ syntax for .ino files
#--map-syntax \"*.ino:C++\"

Review comment (suggestion):
Maybe make it clear that "no" is the default here:
```
Overwrite? (y/N):
```

Code after:
use std::env;
use std::ffi::OsString;
use std::fs;
use std::io::{self, Write};
use std::path::PathBuf;

use shell_words;

use crate::directories::PROJECT_DIRS;

pub fn config_file() -> PathBuf {
    env::var("BAT_CONFIG_PATH")
        .ok()
        .map(PathBuf::from)
        .filter(|config_path| config_path.is_file())
        .unwrap_or_else(|| PROJECT_DIRS.config_dir().join("config"))
}

pub fn generate_config_file() -> bat::errors::Result<()> {
    let config_file = config_file();
    if config_file.exists() {
        println!("A config file already exists at: {}", config_file.to_string_lossy());

        print!("Overwrite? (y/N): ");
        io::stdout().flush()?;
        let mut decision = String::new();
        io::stdin().read_line(&mut decision)?;

        if !decision.trim().eq_ignore_ascii_case("Y") {
            return Ok(());
        }
    } else {
        let config_dir = config_file.parent();
        match config_dir {
            Some(path) => fs::create_dir_all(path)?,
            None => return Ok(Err(format!("Unable to write config file to: {}", config_file.to_string_lossy()))?),
        }
    }

    let default_config = r#"# bat config

# Specify desired theme (e.g. "TwoDark").  Issue `bat --list-themes` for a list of all available themes
#--theme="TwoDark"

# Enable this to use italic text on the terminal (not supported on all terminals):
#--italic-text=always

# Uncomment the following line to disable automatic paging:
#--paging=never
Repository: sharkdp/bat
File: src/bin/bat/config.rs (Rust)

Code before:
use std::env;
use std::ffi::OsString;
use std::fs;
use std::io::{self, Write};
use std::path::PathBuf;

use shell_words;

use crate::directories::PROJECT_DIRS;

pub fn config_file() -> PathBuf {
    env::var("BAT_CONFIG_PATH")
        .ok()
        .map(PathBuf::from)
        .filter(|config_path| config_path.is_file())
        .unwrap_or_else(|| PROJECT_DIRS.config_dir().join("config"))
}

pub fn generate_config_file() {
    let config_file = config_file();
    if config_file.exists() {
        println!("A config file already exists at: {}", config_file.to_string_lossy());

        print!("Overwrite? (y/n): ");
        let _ = io::stdout().flush();
        let mut decision = String::new();
        io::stdin().read_line(&mut decision).expect("Failed to read input");

        if !decision.trim().eq_ignore_ascii_case("Y") {
            return;
        }
    } else {
        let config_dir = config_file.parent().unwrap();
        if !config_dir.exists() {
            fs::create_dir(config_dir).expect("Unable to create config directory");
        }
    }

    let default_config = "# Specify desired theme (e.g. \"TwoDark\")
#--theme=\"TwoDark\"

# Enable this to use italic text on the terminal (not supported on all terminals):
#--italic-text=always

# Uncomment the following line to disable automatic paging:
#--paging=never

# Use C++ syntax for .ino files
#--map-syntax \"*.ino:C++\"


Review comment (suggestion):
If you change this function to return `Result<()>`, you can use
```rust
io::stdout().flush()?;
```
here. Another alternative would be `io::stdout().flush().ok()`.

Code after:
use std::env;
use std::ffi::OsString;
use std::fs;
use std::io::{self, Write};
use std::path::PathBuf;

use shell_words;

use crate::directories::PROJECT_DIRS;

pub fn config_file() -> PathBuf {
    env::var("BAT_CONFIG_PATH")
        .ok()
        .map(PathBuf::from)
        .filter(|config_path| config_path.is_file())
        .unwrap_or_else(|| PROJECT_DIRS.config_dir().join("config"))
}

pub fn generate_config_file() -> bat::errors::Result<()> {
    let config_file = config_file();
    if config_file.exists() {
        println!("A config file already exists at: {}", config_file.to_string_lossy());

        print!("Overwrite? (y/N): ");
        io::stdout().flush()?;
        let mut decision = String::new();
        io::stdin().read_line(&mut decision)?;

        if !decision.trim().eq_ignore_ascii_case("Y") {
            return Ok(());
        }
    } else {
        let config_dir = config_file.parent();
        match config_dir {
            Some(path) => fs::create_dir_all(path)?,
            None => return Ok(Err(format!("Unable to write config file to: {}", config_file.to_string_lossy()))?),
        }
    }

    let default_config = r#"# bat config

# Specify desired theme (e.g. "TwoDark").  Issue `bat --list-themes` for a list of all available themes
#--theme="TwoDark"

# Enable this to use italic text on the terminal (not supported on all terminals):
#--italic-text=always

# Uncomment the following line to disable automatic paging:
#--paging=never
Repository: sharkdp/bat
File: tests/integration_tests.rs (Rust)

Code before:
        "issue_914.rb",
        "issue_915.vue",
    ] {
        bat()
            .arg("--color=always")
            .arg(&format!("regression_tests/{}", filename))
            .assert()
            .success();
    }
}

#[test]
fn do_not_detect_different_syntax_for_stdin_and_files() {
    let file = "regression_tests/issue_985.js";

    let output_for_file = bat()
        .arg("--color=always")
        .arg("--map-syntax=*.js:Markdown")
        .arg(&format!("--file-name={}", file))
        .arg("--style=plain")
        .arg(file)
        .assert()
        .success()
        .get_output()
        .stdout
        .clone();

    let output_for_stdin = bat()
        .arg("--color=always")
        .arg("--map-syntax=*.js:Markdown")
        .arg("--style=plain")
        .arg(&format!("--file-name={}", file))
        .pipe_stdin(Path::new(EXAMPLES_DIR).join(file))
        .unwrap()
        .assert()
        .success()
        .get_output()
        .stdout
        .clone();

    assert_eq!(
        from_utf8(&output_for_file).unwrap(),
        from_utf8(&output_for_stdin).unwrap()
    );
}


Review comment (suggestion):
not that it matters too much here, but you can avoid the `.clone` by using:
``` rust
    let cmd_for_file = bat()
        .arg("--color=always")
        .arg("--map-syntax=*.js:Markdown")
        .arg(&format!("--file-name={}", file))
        .arg("--style=plain")
        .arg(file)
        .assert()
        .success();
    let output_for_file = &cmd_for_file.get_output().stdout;
```
similar below.

Code after:
        .arg(file_path.as_os_str())
        .assert()
        .success()
        .stdout("dummy content\n");
}

#[test]
fn do_not_panic_regression_tests() {
    for filename in &[
        "issue_28.md",
        "issue_190.md",
        "issue_314.hs",
        "issue_914.rb",
        "issue_915.vue",
    ] {
        bat()
            .arg("--color=always")
            .arg(&format!("regression_tests/{}", filename))
            .assert()
            .success();
    }
}

#[test]
fn do_not_detect_different_syntax_for_stdin_and_files() {
    let file = "regression_tests/issue_985.js";

    let cmd_for_file = bat()
        .arg("--color=always")
        .arg("--map-syntax=*.js:Markdown")
        .arg(&format!("--file-name={}", file))
        .arg("--style=plain")
        .arg(file)
        .assert()
        .success();

    let cmd_for_stdin = bat()
        .arg("--color=always")
        .arg("--map-syntax=*.js:Markdown")
        .arg("--style=plain")
        .arg(&format!("--file-name={}", file))
        .pipe_stdin(Path::new(EXAMPLES_DIR).join(file))
        .unwrap()
        .assert()
        .success();

    assert_eq!(
        from_utf8(&cmd_for_file.get_output().stdout).expect("output is valid utf-8"),
        from_utf8(&cmd_for_stdin.get_output().stdout).expect("output is valid utf-8")
    );
}