ronantakizawa/codereview-bench huggingface dataset:
Repository: sharkdp/fd
File: src/exec/command.rs (Rust)

Code before:
use std::io;
use std::io::Write;
use std::process::Command;
use std::sync::Mutex;

use crate::error::print_error;
use crate::exit_codes::ExitCode;

/// Executes a command.
pub fn execute_command(mut cmd: Command, out_perm: &Mutex<()>, is_multithread: bool) -> ExitCode {
    let output;
    // Spawn the supplied command.
    if is_multithread {
        output = cmd.output();
    } else {
        // If running on only one thread, don't buffer output
        // Allows for viewing and interacting with intermediate command output
        let child = cmd.spawn().expect("Failed to execute command");
        output = child.wait_with_output();
    }
    
    // Then wait for the command to exit, if it was spawned.
    match output {
        Ok(output) => {
            // While this lock is active, this thread will be the only thread allowed
            // to write its outputs.
            let _lock = out_perm.lock().unwrap();

            let stdout = io::stdout();
            let stderr = io::stderr();

            let _ = stdout.lock().write_all(&output.stdout);
            let _ = stderr.lock().write_all(&output.stderr);

            if output.status.code() == Some(0) {
                ExitCode::Success
            } else {
                ExitCode::GeneralError
            }
        }
        Err(ref why) if why.kind() == io::ErrorKind::NotFound => {
            print_error(format!("Command not found: {:?}", cmd));
            ExitCode::GeneralError
        }
        Err(why) => {

Review comment (suggestion):
You could rewrite this to:
``` rust
let output = if … {
  cmd.output()
} else {
  …
  child.wait_with_output()
};
```

Code after:
use std::io;
use std::io::Write;
use std::process::Command;
use std::sync::Mutex;

use crate::error::print_error;
use crate::exit_codes::ExitCode;

/// Executes a command.
pub fn execute_command(
    mut cmd: Command,
    out_perm: &Mutex<()>,
    enable_output_buffering: bool,
) -> ExitCode {
    // Spawn the supplied command.
    let output = if enable_output_buffering {
        cmd.output()
    } else {
        // If running on only one thread, don't buffer output
        // Allows for viewing and interacting with intermediate command output
        cmd.spawn().and_then(|c| c.wait_with_output())
    };

    // Then wait for the command to exit, if it was spawned.
    match output {
        Ok(output) => {
            // While this lock is active, this thread will be the only thread allowed
            // to write its outputs.
            let _lock = out_perm.lock().unwrap();

            let stdout = io::stdout();
            let stderr = io::stderr();

            let _ = stdout.lock().write_all(&output.stdout);
            let _ = stderr.lock().write_all(&output.stderr);

            if output.status.code() == Some(0) {
                ExitCode::Success
            } else {
                ExitCode::GeneralError
            }
        }
        Err(ref why) if why.kind() == io::ErrorKind::NotFound => {
            print_error(format!("Command not found: {:?}", cmd));
            ExitCode::GeneralError
Repository: sharkdp/fd
File: src/exec/command.rs (Rust)

Code before:
use std::io;
use std::io::Write;
use std::process::Command;
use std::sync::Mutex;

use crate::error::print_error;
use crate::exit_codes::ExitCode;

/// Executes a command.
pub fn execute_command(mut cmd: Command, out_perm: &Mutex<()>, is_multithread: bool) -> ExitCode {
    let output;
    // Spawn the supplied command.
    if is_multithread {
        output = cmd.output();
    } else {
        // If running on only one thread, don't buffer output
        // Allows for viewing and interacting with intermediate command output
        let child = cmd.spawn().expect("Failed to execute command");
        output = child.wait_with_output();
    }
    
    // Then wait for the command to exit, if it was spawned.
    match output {
        Ok(output) => {
            // While this lock is active, this thread will be the only thread allowed
            // to write its outputs.
            let _lock = out_perm.lock().unwrap();

            let stdout = io::stdout();
            let stderr = io::stderr();

            let _ = stdout.lock().write_all(&output.stdout);
            let _ = stderr.lock().write_all(&output.stderr);

            if output.status.code() == Some(0) {
                ExitCode::Success
            } else {
                ExitCode::GeneralError
            }
        }
        Err(ref why) if why.kind() == io::ErrorKind::NotFound => {
            print_error(format!("Command not found: {:?}", cmd));
            ExitCode::GeneralError

Review comment (suggestion):
This looks like it breaks our error handling. What happens for a non-existing command if you run it with `--threads=2` and `--threads=1`? The output should be the same.

Code after:
use std::io;
use std::io::Write;
use std::process::Command;
use std::sync::Mutex;

use crate::error::print_error;
use crate::exit_codes::ExitCode;

/// Executes a command.
pub fn execute_command(
    mut cmd: Command,
    out_perm: &Mutex<()>,
    enable_output_buffering: bool,
) -> ExitCode {
    // Spawn the supplied command.
    let output = if enable_output_buffering {
        cmd.output()
    } else {
        // If running on only one thread, don't buffer output
        // Allows for viewing and interacting with intermediate command output
        cmd.spawn().and_then(|c| c.wait_with_output())
    };

    // Then wait for the command to exit, if it was spawned.
    match output {
        Ok(output) => {
            // While this lock is active, this thread will be the only thread allowed
            // to write its outputs.
            let _lock = out_perm.lock().unwrap();

            let stdout = io::stdout();
            let stderr = io::stderr();

            let _ = stdout.lock().write_all(&output.stdout);
            let _ = stderr.lock().write_all(&output.stderr);

            if output.status.code() == Some(0) {
                ExitCode::Success
            } else {
                ExitCode::GeneralError
            }
        }
        Err(ref why) if why.kind() == io::ErrorKind::NotFound => {
Repository: sharkdp/fd
File: src/app.rs (Rust)

Code before:
use clap::{crate_version, App, AppSettings, Arg};

pub fn build_app() -> App<'static, 'static> {
    let clap_color_setting =
        if atty::is(atty::Stream::Stdout) && std::env::var_os("NO_COLOR").is_none() {
            AppSettings::ColoredHelp
        } else {
            AppSettings::ColorNever
        };

    let mut app = App::new("fd")
        .version(crate_version!())
        .usage("fd [FLAGS/OPTIONS] [<pattern>] [<path>...]")
        .setting(clap_color_setting)
        .setting(AppSettings::DeriveDisplayOrder)
        .after_help(
            "Note: `fd -h` prints a short and concise overview while `fd --help` gives all \
                 details.",
        )
        .arg(
            Arg::with_name("hidden")
                .long("hidden")
                .short("H")
                .overrides_with("hidden")
                .help("Search hidden files and directories")
                .long_help(
                    "Include hidden directories and files in the search results (default: \
                         hidden files and directories are skipped). Files and directories are \
                         considered to be hidden if their name starts with a `.` sign (dot).",
                ),

Review comment (suggestion):
I think `clap` does an `isatty` check internally, because `fd --help | cat` is not colored at the moment. So I guess we could remove the `atty::is(…)` part here (as well as the build dependency)

Code after:
use clap::{crate_version, App, AppSettings, Arg};

pub fn build_app() -> App<'static, 'static> {
    let clap_color_setting =
        if std::env::var_os("NO_COLOR").is_none() {
            AppSettings::ColoredHelp
        } else {
            AppSettings::ColorNever
        };

    let mut app = App::new("fd")
        .version(crate_version!())
        .usage("fd [FLAGS/OPTIONS] [<pattern>] [<path>...]")
        .setting(clap_color_setting)
        .setting(AppSettings::DeriveDisplayOrder)
        .after_help(
            "Note: `fd -h` prints a short and concise overview while `fd --help` gives all \
                 details.",
        )
        .arg(
            Arg::with_name("hidden")
                .long("hidden")
                .short("H")
                .overrides_with("hidden")
                .help("Search hidden files and directories")
                .long_help(
                    "Include hidden directories and files in the search results (default: \
                         hidden files and directories are skipped). Files and directories are \
                         considered to be hidden if their name starts with a `.` sign (dot).",
                ),
Repository: sharkdp/fd
File: src/exit_codes.rs (Rust)

Code before:
#[derive(Debug, Clone, Copy, PartialEq)]
pub enum ExitCode {
    Success,
    HasMatch(bool),
    GeneralError,
    KilledBySigint,
}

impl From<ExitCode> for i32 {
    fn from(code: ExitCode) -> Self {
        match code {
            ExitCode::Success => 0,
            ExitCode::HasMatch(has_match) => !has_match as i32,
            ExitCode::GeneralError => 1,
            ExitCode::KilledBySigint => 130,
        }
    }
}

impl ExitCode {
    fn is_error(self) -> bool {
        match self {
            ExitCode::Success => false,
            ExitCode::HasMatch(has_match) => !has_match,
            _ => true,
        }
    }
}

pub fn merge_exitcodes(results: &[ExitCode]) -> ExitCode {
    if results.iter().any(|&c| ExitCode::is_error(c)) {
        return ExitCode::GeneralError;
    }
    ExitCode::Success
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn success_when_no_results() {
        assert_eq!(merge_exitcodes(&[]), ExitCode::Success);
    }

    #[test]
    fn success_when_has_match() {
        assert_eq!(
            merge_exitcodes(&[ExitCode::HasMatch(true)]),
            ExitCode::Success
        );

Review comment (performance):
Hm. Maybe it makes sense to implement `is_error` in terms of `from(code: ExitCode)`? This way, we don't have to duplicate the logic. And this is not a function that is performance-critical.

Code after:
#[derive(Debug, Clone, Copy, PartialEq)]
pub enum ExitCode {
    Success,
    HasResults(bool),
    GeneralError,
    KilledBySigint,
}

impl From<ExitCode> for i32 {
    fn from(code: ExitCode) -> Self {
        match code {
            ExitCode::Success => 0,
            ExitCode::HasResults(has_results) => !has_results as i32,
            ExitCode::GeneralError => 1,
            ExitCode::KilledBySigint => 130,
        }
    }
}

impl ExitCode {
    fn is_error(self) -> bool {
        i32::from(self) != 0
    }
}

pub fn merge_exitcodes(results: &[ExitCode]) -> ExitCode {
    if results.iter().any(|&c| ExitCode::is_error(c)) {
        return ExitCode::GeneralError;
    }
    ExitCode::Success
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn success_when_no_results() {
        assert_eq!(merge_exitcodes(&[]), ExitCode::Success);
    }

    #[test]
    fn general_error_if_at_least_one_error() {
        assert_eq!(
            merge_exitcodes(&[ExitCode::GeneralError]),
            ExitCode::GeneralError
        );
        assert_eq!(
            merge_exitcodes(&[ExitCode::KilledBySigint]),
            ExitCode::GeneralError
        );
Repository: sharkdp/fd
File: src/exit_codes.rs (Rust)

Code before:
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn success_when_no_results() {
        assert_eq!(merge_exitcodes(&[]), ExitCode::Success);
    }

    #[test]
    fn success_when_has_match() {
        assert_eq!(
            merge_exitcodes(&[ExitCode::HasMatch(true)]),
            ExitCode::Success
        );
    }

    #[test]
    fn general_error_if_has_no_match() {
        assert_eq!(
            merge_exitcodes(&[ExitCode::HasMatch(false)]),
            ExitCode::GeneralError
        );
    }

    #[test]
    fn general_error_if_at_least_one_error() {
        assert_eq!(
            merge_exitcodes(&[ExitCode::GeneralError]),
            ExitCode::GeneralError
        );
        assert_eq!(
            merge_exitcodes(&[ExitCode::KilledBySigint]),
            ExitCode::GeneralError
        );
        assert_eq!(
            merge_exitcodes(&[ExitCode::KilledBySigint, ExitCode::Success]),
            ExitCode::GeneralError
        );
        assert_eq!(
            merge_exitcodes(&[ExitCode::Success, ExitCode::GeneralError]),
            ExitCode::GeneralError
        );
        assert_eq!(
            merge_exitcodes(&[ExitCode::GeneralError, ExitCode::KilledBySigint]),
            ExitCode::GeneralError
        );
    }


Review comment (suggestion):
Thank you for adding tests, but `merge_exitcodes` is really only relevant for job execution. `HasMatch(_)` is not an output that could result from executing a job.

I think we can remove these tests, what do you think?

Maybe this is a sign that we are merging two functionalities here which are attached to a single datatype (`ExitCode`). Not sure if there is a cleaner strategy for implementing this though.

Code after:
    use super::*;

    #[test]
    fn success_when_no_results() {
        assert_eq!(merge_exitcodes(&[]), ExitCode::Success);
    }

    #[test]
    fn general_error_if_at_least_one_error() {
        assert_eq!(
            merge_exitcodes(&[ExitCode::GeneralError]),
            ExitCode::GeneralError
        );
        assert_eq!(
            merge_exitcodes(&[ExitCode::KilledBySigint]),
            ExitCode::GeneralError
        );
        assert_eq!(
            merge_exitcodes(&[ExitCode::KilledBySigint, ExitCode::Success]),
            ExitCode::GeneralError
        );
        assert_eq!(
            merge_exitcodes(&[ExitCode::Success, ExitCode::GeneralError]),
            ExitCode::GeneralError
        );
        assert_eq!(
            merge_exitcodes(&[ExitCode::GeneralError, ExitCode::KilledBySigint]),
            ExitCode::GeneralError
        );
    }

    #[test]
    fn success_if_no_error() {
        assert_eq!(merge_exitcodes(&[ExitCode::Success]), ExitCode::Success);
        assert_eq!(
            merge_exitcodes(&[ExitCode::Success, ExitCode::Success]),
            ExitCode::Success
        );
    }
}
Repository: sharkdp/fd
File: src/exec/command.rs (Rust)

Code before:

        // Then wait for the command to exit, if it was spawned.
        match output {
            Ok(output) => {
                if enable_output_buffering {
                    output_buffer.push(output.stdout, output.stderr);
                }
                if output.status.code() != Some(0) {
                    output_buffer.write();
                    return ExitCode::GeneralError;
                }
            }
            Err(why) => {
                output_buffer.write();
                return handle_cmd_error(Some(&cmd), why);
            }
        }
    }
    output_buffer.write();
    ExitCode::Success
}

pub fn handle_cmd_error(cmd: Option<&Command>, err: io::Error) -> ExitCode {
    match (cmd, err) {
        (Some(cmd), err) if err.kind() == io::ErrorKind::NotFound => {
            print_error(format!("Command not found: {:?}", cmd.get_program()));
            ExitCode::GeneralError
        }
        (_, err) => {
            print_error(format!("Problem while executing command: {}", err));
            ExitCode::GeneralError
        }
    }
}


Review comment (bug):
Minor: by using `{:?}`, we further rely on Debug outputs which are typically not guaranteed to be stable. Furthermore, `.get_program()` returns an `OsStr` and I'm not sure what it would do in case of invalid UTF-8 program names (which, agreed, is hopefully a non-existent corner case). But still, taking these two things together, I think I would prefer something more explicit like (untested):
```suggestion
            print_error(format!("Command not found: '{}'", cmd.get_program().to_string_lossy()));
```

Code after:

        // Then wait for the command to exit, if it was spawned.
        match output {
            Ok(output) => {
                if enable_output_buffering {
                    output_buffer.push(output.stdout, output.stderr);
                }
                if output.status.code() != Some(0) {
                    output_buffer.write();
                    return ExitCode::GeneralError;
                }
            }
            Err(why) => {
                output_buffer.write();
                return handle_cmd_error(Some(&cmd), why);
            }
        }
    }
    output_buffer.write();
    ExitCode::Success
}

pub fn handle_cmd_error(cmd: Option<&Command>, err: io::Error) -> ExitCode {
    match (cmd, err) {
        (Some(cmd), err) if err.kind() == io::ErrorKind::NotFound => {
            print_error(format!(
                "Command not found: {}",
                cmd.get_program().to_string_lossy()
            ));
            ExitCode::GeneralError
        }
        (_, err) => {
            print_error(format!("Problem while executing command: {}", err));
            ExitCode::GeneralError
        }
    }
}
Repository: sharkdp/fd
File: src/main.rs (Rust)

Code before:
    }
}

fn run() -> Result<ExitCode> {
    let opts = Opts::parse();

    #[cfg(feature = "completions")]
    if let Some(shell) = opts.gen_completions()? {
        return print_completions(shell);
    }

    set_working_dir(&opts)?;
    let search_paths = opts.search_paths()?;
    if search_paths.is_empty() {
        bail!("No valid search paths given.");
    }

    ensure_search_pattern_is_not_a_path(&opts)?;
    let pattern = &opts.pattern;
    let exprs = &opts.exprs;
    let empty = Vec::new();
    let patterns = exprs
        .as_ref()
        .unwrap_or(&empty)
        .into_iter()
        .chain(vec![pattern])
        .collect::<Vec<&String>>();
    let pattern_regexps = patterns
        .iter()
        .map(|pat| {
            let re = build_pattern_regex(pat, &opts);
            match re {
                Ok(r) => r,
                Err(e) => panic!("Bad pattern: {}", e),
            }
        })
        .collect::<Vec<String>>();

    let config = construct_config(opts, &pattern_regexps)?;
    ensure_use_hidden_option_for_leading_dot_pattern(&config, &pattern_regexps)?;
    let regexps = pattern_regexps
        .iter()
        .map(|pat| {
            let regex = build_regex(pat.to_string(), &config);
            match regex {
                Ok(r) => r,
                Err(e) => panic!("Bad regex: {}", e),
            }
        })
        .collect();
    walk::scan(&search_paths, Arc::new(regexps), Arc::new(config))

Review comment (suggestion):
I think this could probably be:

```suggestion
        .chain([pattern])
```

Code after:
    }
}

fn run() -> Result<ExitCode> {
    let opts = Opts::parse();

    #[cfg(feature = "completions")]
    if let Some(shell) = opts.gen_completions()? {
        return print_completions(shell);
    }

    set_working_dir(&opts)?;
    let search_paths = opts.search_paths()?;
    if search_paths.is_empty() {
        bail!("No valid search paths given.");
    }

    ensure_search_pattern_is_not_a_path(&opts)?;
    let pattern = &opts.pattern;
    let exprs = &opts.exprs;
    let empty = Vec::new();

    let pattern_regexps = exprs
        .as_ref()
        .unwrap_or(&empty)
        .iter()
        .chain([pattern])
        .map(|pat| build_pattern_regex(pat, &opts))
        .collect::<Result<Vec<String>>>()?;

    let config = construct_config(opts, &pattern_regexps)?;

    ensure_use_hidden_option_for_leading_dot_pattern(&config, &pattern_regexps)?;

    let regexps = pattern_regexps
        .into_iter()
        .map(|pat| build_regex(pat, &config))
        .collect::<Result<Vec<Regex>>>()?;

    walk::scan(&search_paths, Arc::new(regexps), Arc::new(config))
}

#[cfg(feature = "completions")]
#[cold]
fn print_completions(shell: clap_complete::Shell) -> Result<ExitCode> {
    // The program name is the first argument.
    let first_arg = env::args().next();
    let program_name = first_arg
        .as_ref()
        .map(Path::new)
        .and_then(|path| path.file_stem())
Repository: sharkdp/fd
File: src/dir_entry.rs (Rust)

Code before:

impl Eq for DirEntry {}

impl PartialOrd for DirEntry {
    #[inline]
    fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
        self.path().partial_cmp(other.path())
    }
}

impl Ord for DirEntry {
    #[inline]
    fn cmp(&self, other: &Self) -> std::cmp::Ordering {
        self.path().cmp(other.path())
    }
}

impl Colorable for DirEntry {
    fn path(&self) -> PathBuf {
        self.path().to_owned()
    }

    fn file_name(&self) -> OsString {
        let name = match &self.inner {
            DirEntryInner::Normal(e) => e.file_name(),
            DirEntryInner::BrokenSymlink(_) => todo!(),
        };
        name.to_owned()
    }

    fn file_type(&self) -> Option<FileType> {
        self.file_type()
    }

    fn metadata(&self) -> Option<Metadata> {
        self.metadata().cloned()
    }
}


Review comment (bug):
This todo should probably be done before merging. 

I imagine it could probably be something like:


```suggestion
            DirEntryInner::BrokenSymlink(p) => p.file_name().map_or_else(OsString::new, OsStr::to_os_string),
```

well, I guess then you might need to move the `to_owned` up to line 123 as well.

Code after:
            .as_ref()
    }
}

impl PartialEq for DirEntry {
    #[inline]
    fn eq(&self, other: &Self) -> bool {
        self.path() == other.path()
    }
}

impl Eq for DirEntry {}

impl PartialOrd for DirEntry {
    #[inline]
    fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
        self.path().partial_cmp(other.path())
    }
}

impl Ord for DirEntry {
    #[inline]
    fn cmp(&self, other: &Self) -> std::cmp::Ordering {
        self.path().cmp(other.path())
    }
}

impl Colorable for DirEntry {
    fn path(&self) -> PathBuf {
        self.path().to_owned()
    }

    fn file_name(&self) -> OsString {
        let name = match &self.inner {
            DirEntryInner::Normal(e) => e.file_name(),
            DirEntryInner::BrokenSymlink(path) => {
                // Path::file_name() only works if the last component is Normal,
                // but we want it for all component types, so we open code it.
                // Copied from LsColors::style_for_path_with_metadata().
                path.components()
                    .last()
                    .map(|c| c.as_os_str())
                    .unwrap_or_else(|| path.as_os_str())
            }
        };
        name.to_owned()
    }

    fn file_type(&self) -> Option<FileType> {
        self.file_type()
    }