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

Code before:
            symlinks: false,
            executables_only: false,
        }
    }
}

enum SizeLimitType {
    Max,
    Min,
}

pub struct SizeFilter {
    size: u64,
    limit_type: SizeLimitType,
}

impl SizeFilter {
    pub fn is_within(&self, size: u64) -> bool {
        match self.limit_type {
            SizeLimitType::Max => size <= self.size,
            SizeLimitType::Min => size >= self.size,
        }
    }
}

const KILO: u64 = 1024;
const MEGA: u64 = KILO * 1024;
const GIGA: u64 = MEGA * 1024;
const TERA: u64 = GIGA * 1024;

impl<'a> From<&'a str> for SizeFilter {
    /// Create the `SizeFilter` from the given `&str`.
    /// It is imperative that the incoming value has been validated for
    /// proper format.
    fn from(s: &str) -> Self {
        let captures = SIZE_CAPTURES.captures(s).unwrap();
        let limit = match captures.get(1).map_or("+", |m| m.as_str()) {
            "+" => SizeLimitType::Min,
            _ => SizeLimitType::Max,
        };

        let quantity = captures.get(2).unwrap().as_str().parse::<u64>().unwrap();

        let multiplier = match &captures.get(3).map_or("m", |m| m.as_str()).to_lowercase()[..] {
            "k" => KILO,
            "m" => MEGA,
            "g" => GIGA,
            "t" => TERA,
            _ => 1, // Any we don't understand we'll just say the number of bytes
        };


Review comment (suggestion):
Concerning units, I would like to follow the [official SI and EIC standards](https://en.wikipedia.org/wiki/Kibibyte). The "k", "M", "G", "T"... prefixes are for powers of 10 while the "Ki", "Mi", "Gi", "Ti"... prefixes are for powers of 2. For example:

- 1 MB = 10^6 byte = 1000² byte
- 1 MiB = 2^20 byte = 1024² byte

We can add the "Ki", "Mi".. prefixes later, but these numbers here should be `1000`, not `1024`.

Code after:
            symlinks: false,
            executables_only: false,
        }
    }
}

#[derive(Clone, Copy, Debug, PartialEq)]
enum SizeLimitType {
    Max,
    Min,
}

#[derive(Clone, Copy, Debug, PartialEq)]
pub struct SizeFilter {
    size: u64,
    limit_type: SizeLimitType,
}

// SI prefixes (powers of 10)
const KILO: u64 = 1000;
const MEGA: u64 = KILO * 1000;
const GIGA: u64 = MEGA * 1000;
const TERA: u64 = GIGA * 1000;

// Binary prefixes (powers of 2)
const KIBI: u64 = 1024;
const MEBI: u64 = KIBI * 1024;
const GIBI: u64 = MEBI * 1024;
const TEBI: u64 = GIBI * 1024;

impl SizeFilter {
    pub fn from_string<'a>(s: &str) -> Option<Self> {
        if !SIZE_CAPTURES.is_match(s) {
            return None;
        }

        let captures = match SIZE_CAPTURES.captures(s) {
            Some(cap) => cap,
            None => return None,
        };

        let limit = match captures.get(1).map_or("+", |m| m.as_str()) {
            "+" => SizeLimitType::Min,
            _ => SizeLimitType::Max,
        };

        let quantity = match captures.get(2) {
            None => return None,
            Some(v) => match v.as_str().parse::<u64>() {
                Ok(val) => val,
                _ => return None,
Repository: sharkdp/fd
File: src/internal.rs (Rust)

Code before:
// Copyright (c) 2017 fd developers
// Licensed under the Apache License, Version 2.0
// <LICENSE-APACHE or http://www.apache.org/licenses/LICENSE-2.0>
// or the MIT license <LICENSE-MIT or http://opensource.org/licenses/MIT>,
// at your option. All files in the project carrying such
// notice may not be copied, modified, or distributed except
// according to those terms.

use std::ffi::OsString;
use std::io::Write;
use std::path::PathBuf;
use std::process;
use std::time;

use exec::CommandTemplate;
use lscolors::LsColors;
use regex::{Regex, RegexSet};
use regex_syntax::hir::Hir;
use regex_syntax::Parser;

lazy_static! {
    static ref SIZE_CAPTURES: Regex = { Regex::new(r"(?i)^([+-])(\d+)([bkmgt]i?)b?$").unwrap() };
}

/// Whether or not to show
pub struct FileTypes {
    pub files: bool,
    pub directories: bool,
    pub symlinks: bool,
    pub executables_only: bool,
}

impl Default for FileTypes {
    fn default() -> FileTypes {
        FileTypes {
            files: false,
            directories: false,
            symlinks: false,
            executables_only: false,
        }
    }
}

#[derive(Clone, Copy, Debug, PartialEq)]
enum SizeLimitType {
    Max,
    Min,

Review comment (suggestion):
This still allows for `bb` and `bib`, right? The `b` in the first character clause should be removed, I think. In order to make the whole unit optional, just add `?` after the parens:

`([kmgt]i?)?b?`

Code after:
// Copyright (c) 2017 fd developers
// Licensed under the Apache License, Version 2.0
// <LICENSE-APACHE or http://www.apache.org/licenses/LICENSE-2.0>
// or the MIT license <LICENSE-MIT or http://opensource.org/licenses/MIT>,
// at your option. All files in the project carrying such
// notice may not be copied, modified, or distributed except
// according to those terms.

use std::ffi::OsString;
use std::io::Write;
use std::path::PathBuf;
use std::process;
use std::time;

use exec::CommandTemplate;
use lscolors::LsColors;
use regex::{Regex, RegexSet};
use regex_syntax::hir::Hir;
use regex_syntax::Parser;

lazy_static! {
    static ref SIZE_CAPTURES: Regex = { Regex::new(r"(?i)^([+-])(\d+)(b|[kmgt]i?b?)$").unwrap() };
}

/// Whether or not to show
pub struct FileTypes {
    pub files: bool,
    pub directories: bool,
    pub symlinks: bool,
    pub executables_only: bool,
}

impl Default for FileTypes {
    fn default() -> FileTypes {
        FileTypes {
            files: false,
            directories: false,
            symlinks: false,
            executables_only: false,
        }
    }
}

#[derive(Clone, Copy, Debug, PartialEq)]
enum SizeLimitType {
    Max,
    Min,
Repository: sharkdp/fd
File: src/internal.rs (Rust)

Code before:
    fn is_within_less_than() {
        let f = SizeFilter::from_string("-1k").unwrap();
        assert!(f.is_within(999));
    }

    #[test]
    fn is_within_less_than_equal() {
        let f = SizeFilter::from_string("-1k").unwrap();
        assert!(f.is_within(1000));
    }

    #[test]
    fn is_within_greater_than() {
        let f = SizeFilter::from_string("+1k").unwrap();
        assert!(f.is_within(1001));
    }

    #[test]
    fn is_within_greater_than_equal() {
        let f = SizeFilter::from_string("+1K").unwrap();
        assert!(f.is_within(1000));
    }

    #[test]
    fn is_time_within() {
        let now = SystemTime::now();
        assert!(TimeFilter::after("1min").unwrap().is_within(&now));
        assert!(!TimeFilter::before("1min").unwrap().is_within(&now));

        let t1m_ago = SystemTime::now() - time::Duration::from_secs(60);
        assert!(!TimeFilter::after("30sec").unwrap().is_within(&t1m_ago));
        assert!(TimeFilter::after("2min").unwrap().is_within(&t1m_ago));

        assert!(TimeFilter::before("30sec").unwrap().is_within(&t1m_ago));
        assert!(!TimeFilter::before("2min").unwrap().is_within(&t1m_ago));
    }
}


Review comment (bug):
Even if this is rather unlikely to fail, it would be great if all tests could be completely deterministic by using specific (random) points in time instead of `SystemTime::now()`.

Code after:
    fn is_within_less_than() {
        let f = SizeFilter::from_string("-1k").unwrap();
        assert!(f.is_within(999));
    }

    #[test]
    fn is_within_less_than_equal() {
        let f = SizeFilter::from_string("-1k").unwrap();
        assert!(f.is_within(1000));
    }

    #[test]
    fn is_within_greater_than() {
        let f = SizeFilter::from_string("+1k").unwrap();
        assert!(f.is_within(1001));
    }

    #[test]
    fn is_within_greater_than_equal() {
        let f = SizeFilter::from_string("+1K").unwrap();
        assert!(f.is_within(1000));
    }

    #[test]
    fn is_time_filter_applicable() {
        use humantime;

        let ref_time = humantime::parse_rfc3339("2010-10-10T10:10:10Z").unwrap();
        assert!(
            TimeFilter::after(&ref_time, "1min")
                .unwrap()
                .applies_to(&ref_time)
        );
        assert!(
            !TimeFilter::before(&ref_time, "1min")
                .unwrap()
                .applies_to(&ref_time)
        );

        let t1m_ago = ref_time - time::Duration::from_secs(60);
        assert!(
            !TimeFilter::after(&ref_time, "30sec")
                .unwrap()
                .applies_to(&t1m_ago)
        );
        assert!(
            TimeFilter::after(&ref_time, "2min")
                .unwrap()
                .applies_to(&t1m_ago)
        );
Repository: sharkdp/fd
File: src/internal/opts.rs (Rust)

Code before:

    /// The type of file to search for. If set to `None`, all file types are displayed. If
    /// set to `Some(..)`, only the types that are specified are shown.
    pub file_types: Option<FileTypes>,

    /// The extension to search for. Only entries matching the extension will be included.
    ///
    /// The value (if present) will be a lowercase string without leading dots.
    pub extensions: Option<RegexSet>,

    /// If a value is supplied, each item found will be used to generate and execute commands.
    pub command: Option<CommandTemplate>,

    /// A list of glob patterns that should be excluded from the search.
    pub exclude_patterns: Vec<String>,

    /// A list of custom ignore files.
    pub ignore_files: Vec<PathBuf>,

    /// The given constraints on the size of returned files
    pub size_constraints: Vec<SizeFilter>,

    /// Constraints on last modification time of files
    pub time_constraints: Vec<TimeFilter>,

    /// Whether to print errors on stdout
    pub verbose: bool,
}


Review comment (suggestion):
This does not disable all errors. So the docstring should be something like: "Whether or not to display filesystem errors"

Code after:

    /// The type of file to search for. If set to `None`, all file types are displayed. If
    /// set to `Some(..)`, only the types that are specified are shown.
    pub file_types: Option<FileTypes>,

    /// The extension to search for. Only entries matching the extension will be included.
    ///
    /// The value (if present) will be a lowercase string without leading dots.
    pub extensions: Option<RegexSet>,

    /// If a value is supplied, each item found will be used to generate and execute commands.
    pub command: Option<CommandTemplate>,

    /// A list of glob patterns that should be excluded from the search.
    pub exclude_patterns: Vec<String>,

    /// A list of custom ignore files.
    pub ignore_files: Vec<PathBuf>,

    /// The given constraints on the size of returned files
    pub size_constraints: Vec<SizeFilter>,

    /// Constraints on last modification time of files
    pub time_constraints: Vec<TimeFilter>,

    /// Whether or not to display filesystem errors
    pub show_filesystem_errors: bool,
}
Repository: sharkdp/fd
File: src/internal/opts.rs (Rust)

Code before:
    /// The type of file to search for. If set to `None`, all file types are displayed. If
    /// set to `Some(..)`, only the types that are specified are shown.
    pub file_types: Option<FileTypes>,

    /// The extension to search for. Only entries matching the extension will be included.
    ///
    /// The value (if present) will be a lowercase string without leading dots.
    pub extensions: Option<RegexSet>,

    /// If a value is supplied, each item found will be used to generate and execute commands.
    pub command: Option<CommandTemplate>,

    /// A list of glob patterns that should be excluded from the search.
    pub exclude_patterns: Vec<String>,

    /// A list of custom ignore files.
    pub ignore_files: Vec<PathBuf>,

    /// The given constraints on the size of returned files
    pub size_constraints: Vec<SizeFilter>,

    /// Constraints on last modification time of files
    pub time_constraints: Vec<TimeFilter>,

    /// Whether to print errors on stdout
    pub verbose: bool,
}


Review comment (suggestion):
I suggest that we rename all variables from `verbose` to `show_filesystem_errors`.

Code after:
    /// The type of file to search for. If set to `None`, all file types are displayed. If
    /// set to `Some(..)`, only the types that are specified are shown.
    pub file_types: Option<FileTypes>,

    /// The extension to search for. Only entries matching the extension will be included.
    ///
    /// The value (if present) will be a lowercase string without leading dots.
    pub extensions: Option<RegexSet>,

    /// If a value is supplied, each item found will be used to generate and execute commands.
    pub command: Option<CommandTemplate>,

    /// A list of glob patterns that should be excluded from the search.
    pub exclude_patterns: Vec<String>,

    /// A list of custom ignore files.
    pub ignore_files: Vec<PathBuf>,

    /// The given constraints on the size of returned files
    pub size_constraints: Vec<SizeFilter>,

    /// Constraints on last modification time of files
    pub time_constraints: Vec<TimeFilter>,

    /// Whether or not to display filesystem errors
    pub show_filesystem_errors: bool,
}
Repository: sharkdp/fd
File: tests/tests.rs (Rust)

Code before:
            c
            d
            directory_foo",
        );

        te.assert_output(
            &["foo", exec_style, "echo", "{//}"],
            ".
            one
            one/two
            one/two
            one/two/three
            one/two/three",
        );

        te.assert_output(&["e1", exec_style, "printf", "%s.%s\n"], "e1 e2.");
    }
}

/// Shell script execution using -exec
#[test]
fn test_exec_batch() {
    assert_exec_batch_output("--exec-batch");
}

// Shell script execution using -x
#[test]
fn test_exec_batch_short_arg() {
    assert_exec_batch_output("-X");
}

#[cfg(test)]
fn assert_exec_batch_output(exec_style: &str) {
    let (te, abs_path) = get_test_env_with_abs_path(DEFAULT_DIRS, DEFAULT_FILES);
    let te = te.normalize_line(true);

    // TODO Windows tests: D:file.txt \file.txt \\server\share\file.txt ...
    if !cfg!(windows) {
        te.assert_output(
            &["--absolute-path", "foo", exec_style, "echo"],
            &format!(
                "{abs_path}/a.foo {abs_path}/one/b.foo {abs_path}/one/two/C.Foo2 {abs_path}/one/two/c.foo {abs_path}/one/two/three/d.foo {abs_path}/one/two/three/directory_foo",
                abs_path = &abs_path
            ),
        );

        te.assert_output(
            &["foo", exec_style, "echo", "{}"],
            "a.foo one/b.foo one/two/C.Foo2 one/two/c.foo one/two/three/d.foo one/two/three/directory_foo",
        );


Review comment (suggestion):
I think this is redundant. If you want to keep it, please modify the docstring to use `-X` instead of `-x`.

Code after:
            c
            d
            directory_foo",
        );

        te.assert_output(
            &["foo", exec_style, "echo", "{//}"],
            ".
            one
            one/two
            one/two
            one/two/three
            one/two/three",
        );

        te.assert_output(&["e1", exec_style, "printf", "%s.%s\n"], "e1 e2.");
    }
}

#[test]
fn test_exec_batch() {
    assert_exec_batch_output("--exec-batch");
}

#[test]
fn test_exec_batch_short_arg() {
    assert_exec_batch_output("-X");
}

#[cfg(test)]
fn assert_exec_batch_output(exec_style: &str) {
    let (te, abs_path) = get_test_env_with_abs_path(DEFAULT_DIRS, DEFAULT_FILES);
    let te = te.normalize_line(true);

    // TODO Test for windows
    if !cfg!(windows) {
        te.assert_output(
            &["--absolute-path", "foo", exec_style, "echo"],
            &format!(
                "{abs_path}/a.foo {abs_path}/one/b.foo {abs_path}/one/two/C.Foo2 {abs_path}/one/two/c.foo {abs_path}/one/two/three/d.foo {abs_path}/one/two/three/directory_foo",
                abs_path = &abs_path
            ),
        );

        te.assert_output(
            &["foo", exec_style, "echo", "{}"],
            "a.foo one/b.foo one/two/C.Foo2 one/two/c.foo one/two/three/d.foo one/two/three/directory_foo",
        );

        te.assert_output(
            &["foo", exec_style, "echo", "{/}"],
Repository: sharkdp/fd
File: tests/tests.rs (Rust)

Code before:

/// Maximum depth (--max-depth)
#[test]
fn test_max_depth() {
    let te = TestEnv::new(DEFAULT_DIRS, DEFAULT_FILES);

    te.assert_output(
        &["--max-depth", "3"],
        "a.foo
        e1 e2
        one
        one/b.foo
        one/two
        one/two/c.foo
        one/two/C.Foo2
        one/two/three
        symlink",
    );

    te.assert_output(
        &["--max-depth", "2"],
        "a.foo
        e1 e2
        one
        one/b.foo
        one/two
        symlink",
    );

    te.assert_output(
        &["--max-depth", "1"],
        "a.foo
        e1 e2
        one
        symlink",
    );
}

/// Absolute paths (--absolute-path)
#[test]
fn test_absolute_path() {
    let (te, abs_path) = get_test_env_with_abs_path(DEFAULT_DIRS, DEFAULT_FILES);

    te.assert_output(
        &["--absolute-path"],
        &format!(
            "{abs_path}/a.foo
            {abs_path}/e1 e2
            {abs_path}/one
            {abs_path}/one/b.foo
            {abs_path}/one/two

Review comment (bug):
I think this test actually shows that something is not yet correct in the implementation.

I would *not* expect `--type file` to show a broken symlink. A broken symlink should only show up for `--type symlink` (or if no `--type` is specified).

`--follow` shouldn't have any influence on the result.

I guess I would expect this behavior:

``` rust
te.assert_output(&["symlink"], "broken_symlink");
te.assert_output(&["--type", "link", "symlink"], "broken_symlink");

te.assert_output(&["--type", "file", "symlink"], "");

te.assert_output(&["--follow", "--type", "link", "symlink"], "broken_symlink");
te.assert_output(&["--follow", "--type", "file", "symlink"], "");
```

Code after:
    }

    te.assert_output(
        &["--full-path", "--max-depth", "2", "^/dev/null$", "/"],
        "/dev/null",
    );
    te.assert_output(
        &[
            "--one-file-system",
            "--full-path",
            "--max-depth",
            "2",
            "^/dev/null$",
            "/",
        ],
        "",
    );
}

#[test]
fn test_follow_broken_symlink() {
    let mut te = TestEnv::new(DEFAULT_DIRS, DEFAULT_FILES);
    te.create_broken_symlink("broken_symlink")
        .expect("Failed to create broken symlink.");

    te.assert_output(
        &["symlink"],
        "broken_symlink
        symlink",
    );
    te.assert_output(
        &["--type", "symlink", "symlink"],
        "broken_symlink
        symlink",
    );

    te.assert_output(&["--type", "file", "symlink"], "");

    te.assert_output(
        &["--follow", "--type", "symlink", "symlink"],
        "broken_symlink",
    );
    te.assert_output(&["--follow", "--type", "file", "symlink"], "");
}

/// Null separator (--print0)
#[test]
fn test_print0() {
    let te = TestEnv::new(DEFAULT_DIRS, DEFAULT_FILES);

    te.assert_output(
Repository: sharkdp/fd
File: src/exit_codes.rs (Rust)

Code before:
pub enum ExitCode {
    Success,
    GeneralError,
    KilledBySigint,
}

impl Into<i32> for ExitCode {
    fn into(self) -> i32 {
        match self {
            Self::Success => 0,
            Self::GeneralError => 1,
            Self::KilledBySigint => 130,
        }
    }
}

impl ExitCode {
    pub fn error_if_any_error(results: Vec<Self>) -> Self {
        if results.iter().any(|s| match s {
            Self::GeneralError => true,
            _ => false,
        }) {
            return Self::GeneralError;
        }
        Self::Success
    }
}


Review comment (refactor):
Maybe this `match` should be extracted into another function: `ExitCode::is_error`?

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

impl Into<i32> for ExitCode {
    fn into(self) -> i32 {
        match self {
            ExitCode::Success => 0,
            ExitCode::GeneralError => 1,
            ExitCode::KilledBySigint => 130,
        }
    }
}

impl ExitCode {
    fn is_error(&self) -> bool {
        match self {
            ExitCode::GeneralError | ExitCode::KilledBySigint => true,
            _ => false,
        }
    }
}

pub fn merge_exitcodes(results: Vec<ExitCode>) -> ExitCode {
    if results.iter().any(ExitCode::is_error) {
        return ExitCode::GeneralError;
    }
    ExitCode::Success
}

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

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

    #[test]
    fn general_error_with_at_least_a_matching_error() {
Repository: sharkdp/fd
File: src/filter/owner.rs (Rust)

Code before:
use anyhow::{anyhow, Result};
use std::fs;

#[derive(Clone, Copy, Debug, PartialEq)]
pub struct OwnerFilter {
    uid: Check<u32>,
    gid: Check<u32>,
}

#[derive(Clone, Copy, Debug, PartialEq)]
enum Check<T> {
    Equal(T),
    NotEq(T),
    Ignore,
}

impl OwnerFilter {
    pub fn from_string(input: &str) -> Result<Self> {
        let mut it = input.split(':');
        let (fst, snd) = (it.next(), it.next());

        let uid = Check::parse(fst, |s| {
            s.parse()
                .ok()
                .or_else(|| users::get_user_by_name(s).map(|user| user.uid()))
                .ok_or_else(|| anyhow!("'{}' is not a recognized user name", s))
        })?;
        let gid = Check::parse(snd, |s| {
            s.parse()
                .ok()
                .or_else(|| users::get_group_by_name(s).map(|group| group.gid()))
                .ok_or_else(|| anyhow!("'{}' is not a recognized group name", s))
        })?;

        if let (Check::Ignore, Check::Ignore) = (uid, gid) {
            Err(anyhow!(
                "'{}' is not a valid user/group specifier. See 'fd --help'.",
                input
            ))
        } else {
            Ok(OwnerFilter { uid, gid })
        }
    }

    pub fn matches(&self, md: &fs::Metadata) -> bool {
        use std::os::unix::fs::MetadataExt;

        self.uid.check(md.uid()) && self.gid.check(md.gid())

Review comment (suggestion):
Could we add a check here to make sure that `it.next()` is `None` after calling it twice? To disallow usage of two or more colons like `--owner shark:lxd:whatever` which would likely be a mistake.

Code after:
use anyhow::{anyhow, Result};
use std::fs;

#[derive(Clone, Copy, Debug, PartialEq)]
pub struct OwnerFilter {
    uid: Check<u32>,
    gid: Check<u32>,
}

#[derive(Clone, Copy, Debug, PartialEq)]
enum Check<T> {
    Equal(T),
    NotEq(T),
    Ignore,
}

impl OwnerFilter {
    /// Parses an owner constraint
    /// Returns an error if the string is invalid
    /// Returns Ok(None) when string is acceptable but a noop (such as "" or ":")
    pub fn from_string(input: &str) -> Result<Option<Self>> {
        let mut it = input.split(':');
        let (fst, snd) = (it.next(), it.next());

        if it.next().is_some() {
            return Err(anyhow!(
                "more than one ':' present in owner string '{}'. See 'fd --help'.",
                input
            ));
        }

        let uid = Check::parse(fst, |s| {
            s.parse()
                .ok()
                .or_else(|| users::get_user_by_name(s).map(|user| user.uid()))
                .ok_or_else(|| anyhow!("'{}' is not a recognized user name", s))
        })?;
        let gid = Check::parse(snd, |s| {
            s.parse()
                .ok()
                .or_else(|| users::get_group_by_name(s).map(|group| group.gid()))
                .ok_or_else(|| anyhow!("'{}' is not a recognized group name", s))
        })?;

        if let (Check::Ignore, Check::Ignore) = (uid, gid) {
            Ok(None)
        } else {
            Ok(Some(OwnerFilter { uid, gid }))