ronantakizawa/codereview-bench huggingface dataset:
Repository: nushell/nushell
File: crates/nu-command/src/filters/drop/nth.rs (Rust)

Code before:

        rows_to_drop.sort_unstable();
        rows_to_drop.dedup();

        let input = if let Some(cutoff) = min_unbounded_start {
            input
                .into_iter()
                .take(cutoff)
                .into_pipeline_data_with_metadata(
                    head,
                    engine_state.signals().clone(),
                    metadata.clone(),
                )
        } else {
            input
        };

        Ok(DropNthIterator {
            input: input.into_iter(),
            rows: rows_to_drop,
            current: 0,
        }
        .into_pipeline_data_with_metadata(
            head,
            engine_state.signals().clone(),
            metadata.clone(),
        ))
    }
}

struct DropNthIterator {
    input: PipelineIterator,
    rows: Vec<usize>,
    current: usize,
}

impl Iterator for DropNthIterator {
    type Item = Value;

    fn next(&mut self) -> Option<Self::Item> {
        loop {
            if let Some(row) = self.rows.first() {
                if self.current == *row {
                    self.rows.remove(0);
                    self.current += 1;
                    let _ = self.input.next();
                    continue;
                } else {
                    self.current += 1;
                    return self.input.next();
                }

Review comment (suggestion):
this clone isn't necessary
```suggestion
            metadata,
```

Code after:
                    });
                }
            }
        } else {
            return Err(ShellError::TypeMismatch {
                err_message: "Expected int or range".into(),
                span: value.span(),
            });
        }
    }

    rows_to_drop.sort_unstable();
    rows_to_drop.dedup();

    Ok((VecDeque::from(rows_to_drop), min_unbounded_start))
}

struct DropNthIterator {
    input: PipelineIterator,
    rows: VecDeque<usize>,
    current: usize,
}

impl Iterator for DropNthIterator {
    type Item = Value;

    fn next(&mut self) -> Option<Self::Item> {
        loop {
            if let Some(row) = self.rows.front() {
                if self.current == *row {
                    self.rows.pop_front();
                    self.current += 1;
                    let _ = self.input.next();
                    continue;
                } else {
                    self.current += 1;
                    return self.input.next();
                }
            } else {
                return self.input.next();
            }
        }
    }
}

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

    #[test]
    fn test_examples() {
Repository: nushell/nushell
File: crates/nu-command/tests/commands/debug/metadata_set.rs (Rust)

Code before:
#[test]
fn errors_on_conflicting_metadata_flags() {
    let actual = nu!(
        cwd: ".", pipeline(
        r#"
        echo "foo" | metadata set --datasource-filepath foo.txt --datasource-ls
        "#
    ));

    assert!(
        actual
            .err
            .contains("Cannot use both --datasource-filepath and --datasource-ls")
    );
}

#[test]
fn works_with_datasource_filepath() {
    let actual = nu!(
        cwd: ".", pipeline(
        r#"
        echo "foo" | metadata set --datasource-filepath foo.txt
        "#
    ));

    assert!(actual.out.contains("foo"));
}


Review comment (suggestion):
If you go through the motions to add a test named `works...` the test should probably also check that it actually works.

e.g. with `metadata`

Code after:
#[test]
fn errors_on_conflicting_metadata_flags() {
    let actual = nu!(
        cwd: ".", pipeline(
        r#"
        echo "foo" | metadata set --datasource-filepath foo.txt --datasource-ls
        "#
    ));

    assert!(actual.err.contains("cannot use `--datasource-filepath`"));
    assert!(actual.err.contains("with `--datasource-ls`"));
}

#[test]
fn works_with_datasource_filepath() {
    let actual = nu!(
        cwd: ".", pipeline(
        r#"
        echo "foo"
        | metadata set --datasource-filepath foo.txt
        | metadata
        "#
    ));

    assert!(actual.out.contains("foo.txt"));
}

#[test]
fn works_with_datasource_ls() {
    let actual = nu!(
        cwd: ".", pipeline(
        r#"
        echo "foo"
        | metadata set --datasource-ls
        | metadata
        "#
    ));

    assert!(actual.out.contains("ls"));
}
Repository: nushell/nushell
File: crates/nu-command/tests/commands/mut_.rs (Rust)

Code before:
}

#[test]
fn mut_path_upsert() {
    let actual = nu!("mut a = {b:[{c:1}]}; $a.b.0.d = 11; $a.b.0.d");

    assert_eq!(actual.out, "11");
}

#[test]
fn mut_path_upsert_list() {
    let actual = nu!("mut a = [[[3] 2] 1]; $a.0.0.1 = 0; $a.0.2 = 0; $a.2 = 0; $a | to nuon");

    assert_eq!(actual.out, "[[[3, 0], 2, 0], 1, 0]");
}

#[test]
fn mut_path_operator_assign() {
    let actual =
        nu!("mut a: record<b: number> = {b:1}; $a.b += 3; $a.b -= 2; $a.b *= 10; $a.b /= 4; $a.b");

    assert_eq!(actual.out, "5.0");
}

#[test]
fn mut_path_operator_assign_should_error() {
    let actual = nu!("mut a = {b:1}; $a.b += 3; $a.b -= 2; $a.b *= 10; $a.b /= 4; $a.b");

    assert!(actual.err.contains("nu::shell::cant_convert"));
}

#[test]
fn mut_records_update_properly() {
    let actual = nu!("mut a = {}; $a.b.c = 100; $a.b.c");
    assert_eq!(actual.out, "100");
}

#[test]
fn mut_value_with_if() {
    let actual = nu!("mut a = 3; $a = if 3 == 3 { 10 }; echo $a");
    assert_eq!(actual.out, "10");
}

#[test]
fn mut_value_with_match() {
    let actual = nu!("mut a = 'maybe?'; $a = match 3 { 1 => { 'yes!' }, _ => { 'no!' } }; echo $a");
    assert_eq!(actual.out, "no!");
}

#[test]
fn mut_glob_type() {

Review comment (suggestion):
This seems to be a breaking change, I'm not sure this is the "correct" semantics of mutable records.

Code after:
}

#[test]
fn mut_path_upsert() {
    let actual = nu!("mut a = {b:[{c:1}]}; $a.b.0.d = 11; $a.b.0.d");

    assert_eq!(actual.out, "11");
}

#[test]
fn mut_path_upsert_list() {
    let actual = nu!("mut a = [[[3] 2] 1]; $a.0.0.1 = 0; $a.0.2 = 0; $a.2 = 0; $a | to nuon");

    assert_eq!(actual.out, "[[[3, 0], 2, 0], 1, 0]");
}

#[test]
fn mut_path_operator_assign_should_error_enforce_runtime() {
    // should error on the division
    let actual = nu!(
        experimental: vec!["enforce-runtime-annotations".to_string()],
        "mut a: record<b: int> = {b:1}; $a.b += 3; $a.b -= 2; $a.b *= 10; $a.b /= 4; $a.b",
    );

    assert!(actual.err.contains("nu::shell::cant_convert"));
}

#[test]
fn mut_records_update_properly() {
    let actual = nu!("mut a = {}; $a.b.c = 100; $a.b.c");
    assert_eq!(actual.out, "100");
}

#[test]
fn mut_value_with_if() {
    let actual = nu!("mut a = 3; $a = if 3 == 3 { 10 }; echo $a");
    assert_eq!(actual.out, "10");
}

#[test]
fn mut_value_with_match() {
    let actual = nu!("mut a = 'maybe?'; $a = match 3 { 1 => { 'yes!' }, _ => { 'no!' } }; echo $a");
    assert_eq!(actual.out, "no!");
}

#[test]
fn mut_glob_type() {
    let actual = nu!("mut x: glob = 'aa'; $x | describe");
    assert_eq!(actual.out, "glob");
}
Repository: nushell/nushell
File: crates/nu-command/tests/commands/mut_.rs (Rust)

Code before:
fn mut_path_upsert() {
    let actual = nu!("mut a = {b:[{c:1}]}; $a.b.0.d = 11; $a.b.0.d");

    assert_eq!(actual.out, "11");
}

#[test]
fn mut_path_upsert_list() {
    let actual = nu!("mut a = [[[3] 2] 1]; $a.0.0.1 = 0; $a.0.2 = 0; $a.2 = 0; $a | to nuon");

    assert_eq!(actual.out, "[[[3, 0], 2, 0], 1, 0]");
}

#[test]
fn mut_path_operator_assign() {
    let actual =
        nu!("mut a: record<b: number> = {b:1}; $a.b += 3; $a.b -= 2; $a.b *= 10; $a.b /= 4; $a.b");

    assert_eq!(actual.out, "5.0");
}

#[test]
fn mut_path_operator_assign_should_error() {
    let actual = nu!("mut a = {b:1}; $a.b += 3; $a.b -= 2; $a.b *= 10; $a.b /= 4; $a.b");

    assert!(actual.err.contains("nu::shell::cant_convert"));
}

#[test]
fn mut_records_update_properly() {
    let actual = nu!("mut a = {}; $a.b.c = 100; $a.b.c");
    assert_eq!(actual.out, "100");
}

#[test]
fn mut_value_with_if() {
    let actual = nu!("mut a = 3; $a = if 3 == 3 { 10 }; echo $a");
    assert_eq!(actual.out, "10");
}

#[test]
fn mut_value_with_match() {
    let actual = nu!("mut a = 'maybe?'; $a = match 3 { 1 => { 'yes!' }, _ => { 'no!' } }; echo $a");
    assert_eq!(actual.out, "no!");
}

#[test]
fn mut_glob_type() {
    let actual = nu!("mut x: glob = 'aa'; $x | describe");
    assert_eq!(actual.out, "glob");
}

Review comment (suggestion):
This test doesn't actually check that every one of the `$a.b foo= bar` statements errors. Only one of them needs to error for the test to pass. I assume the intention is to make sure that all of them are not allowed

Code after:
fn mut_path_upsert() {
    let actual = nu!("mut a = {b:[{c:1}]}; $a.b.0.d = 11; $a.b.0.d");

    assert_eq!(actual.out, "11");
}

#[test]
fn mut_path_upsert_list() {
    let actual = nu!("mut a = [[[3] 2] 1]; $a.0.0.1 = 0; $a.0.2 = 0; $a.2 = 0; $a | to nuon");

    assert_eq!(actual.out, "[[[3, 0], 2, 0], 1, 0]");
}

#[test]
fn mut_path_operator_assign_should_error_enforce_runtime() {
    // should error on the division
    let actual = nu!(
        experimental: vec!["enforce-runtime-annotations".to_string()],
        "mut a: record<b: int> = {b:1}; $a.b += 3; $a.b -= 2; $a.b *= 10; $a.b /= 4; $a.b",
    );

    assert!(actual.err.contains("nu::shell::cant_convert"));
}

#[test]
fn mut_records_update_properly() {
    let actual = nu!("mut a = {}; $a.b.c = 100; $a.b.c");
    assert_eq!(actual.out, "100");
}

#[test]
fn mut_value_with_if() {
    let actual = nu!("mut a = 3; $a = if 3 == 3 { 10 }; echo $a");
    assert_eq!(actual.out, "10");
}

#[test]
fn mut_value_with_match() {
    let actual = nu!("mut a = 'maybe?'; $a = match 3 { 1 => { 'yes!' }, _ => { 'no!' } }; echo $a");
    assert_eq!(actual.out, "no!");
}

#[test]
fn mut_glob_type() {
    let actual = nu!("mut x: glob = 'aa'; $x | describe");
    assert_eq!(actual.out, "glob");
}

#[test]
fn mut_raw_string() {
    let actual = nu!(r#"mut x = r#'abcde""fghi"''''jkl'#; $x"#);
Repository: nushell/nushell
File: crates/nu-protocol/src/signature.rs (Rust)

Code before:
            if is_required {
                format!("<{}> ", arg.name)
            } else {
                format!("({}) ", arg.name)
            }
        }
    }
}

#[derive(Clone, DeriveFromValue)]
pub struct CustomExample {
    pub example: String,
    pub description: String,
    pub result: Option<Value>,
}

impl CustomExample {
    pub fn to_example(&self) -> Example<'_> {
        Example {
            example: self.example.as_str(),
            description: self.description.as_str(),
            result: self.result.clone(),
        }
    }
}

#[derive(Clone)]
struct BlockCommand {
    signature: Signature,
    block_id: BlockId,
    attributes: Vec<(String, Value)>,
    examples: Vec<CustomExample>,
}

impl Command for BlockCommand {
    fn name(&self) -> &str {
        &self.signature.name
    }

    fn signature(&self) -> Signature {
        self.signature.clone()
    }

    fn description(&self) -> &str {
        &self.signature.description
    }

    fn extra_description(&self) -> &str {
        &self.signature.extra_description
    }


Review comment (suggestion):
I guess switching these `to_`/`from_` methods to `From` implementations would be more idiomatic. Not really important, just noticed that during the review.

Code after:
            if is_required {
                format!("<{}> ", arg.name)
            } else {
                format!("({}) ", arg.name)
            }
        }
    }
}

#[derive(Clone, DeriveFromValue)]
pub struct CustomExample {
    pub example: String,
    pub description: String,
    pub result: Option<Value>,
}

impl CustomExample {
    pub fn to_example(&self) -> Example<'_> {
        Example {
            example: self.example.as_str(),
            description: self.description.as_str(),
            result: self.result.clone(),
        }
    }

    pub fn from_example(example: Example<'_>) -> Self {
        Self {
            example: example.example.to_string(),
            description: example.description.to_string(),
            result: example.result,
        }
    }
}

#[derive(Clone)]
struct BlockCommand {
    signature: Signature,
    block_id: BlockId,
    attributes: Vec<(String, Value)>,
    examples: Vec<CustomExample>,
}

impl Command for BlockCommand {
    fn name(&self) -> &str {
        &self.signature.name
    }

    fn signature(&self) -> Signature {
        self.signature.clone()
    }
Repository: nushell/nushell
File: crates/nu-cli/src/commands/commandline/edit.rs (Rust)

Code before:

    fn run(
        &self,
        engine_state: &EngineState,
        stack: &mut Stack,
        call: &Call,
        _input: PipelineData,
    ) -> Result<PipelineData, ShellError> {
        let str: String = call.req(engine_state, stack, 0)?;
        let mut repl = engine_state.repl_state.lock().expect("repl state mutex");
        if call.has_flag(engine_state, stack, "append")? {
            repl.buffer.push_str(&str);
        } else if call.has_flag(engine_state, stack, "insert")? {
            let cursor_pos = repl.cursor_pos;
            repl.buffer.insert_str(cursor_pos, &str);
            repl.cursor_pos += str.len();
        } else {
            repl.buffer = str;
            repl.cursor_pos = repl.buffer.len();
        }

        if call.has_flag(engine_state, stack, "accept")? {
            engine_state
                .immediately_accept
                .store(true, Ordering::Relaxed);
        }

        Ok(Value::nothing(call.head).into_pipeline_data())
    }
}


Review comment (suggestion):
Not sure how much it matters in practice, but shouldn't this be `Ordering::Release` which would imply a happens-before relationship for the writes in the critical section of `engine_state.repl_state`?

In `nu-cli/repl.rs` the `compare_exchange` which you set to `Acquire` is ahead of the read of the `repl_state` mutex which happens right before the call into `Reedline::read_line`.

I guess for singular uses of `commandline --accept` the mutex should ensure ordering in itself, as the `MutexGuard::drop` happens after the store.

Code after:
    fn run(
        &self,
        engine_state: &EngineState,
        stack: &mut Stack,
        call: &Call,
        _input: PipelineData,
    ) -> Result<PipelineData, ShellError> {
        let str: String = call.req(engine_state, stack, 0)?;
        let mut repl = engine_state.repl_state.lock().expect("repl state mutex");
        if call.has_flag(engine_state, stack, "append")? {
            repl.buffer.push_str(&str);
        } else if call.has_flag(engine_state, stack, "insert")? {
            let cursor_pos = repl.cursor_pos;
            repl.buffer.insert_str(cursor_pos, &str);
            repl.cursor_pos += str.len();
        } else {
            repl.buffer = str;
            repl.cursor_pos = repl.buffer.len();
        }

        repl.accept = call.has_flag(engine_state, stack, "accept")?;

        Ok(Value::nothing(call.head).into_pipeline_data())
    }
}
Repository: nushell/nushell
File: src/signals.rs (Rust)

Code before:
use nu_protocol::{Handlers, SignalAction, Signals, engine::EngineState};
use std::sync::{
    Arc,
    atomic::{AtomicBool, Ordering},
};

pub(crate) fn ctrlc_protection(engine_state: &mut EngineState) {
    let interrupt = Arc::new(AtomicBool::new(false));
    engine_state.set_signals(Signals::new(interrupt.clone()));

    let signal_handlers = Handlers::new();

    // Register a handler to kill all background jobs on interrupt.
    signal_handlers
        .register_unguarded({
            let jobs = engine_state.jobs.clone();
            Box::new(move |action| {
                if action == SignalAction::Interrupt {
                    if let Ok(mut jobs) = jobs.lock() {
                        jobs.kill_all().ok();
                    }
                }
            })
        })
        .expect("Failed to register interrupt signal handler");

    engine_state.signal_handlers = Some(signal_handlers.clone());

    ctrlc::set_handler(move || {
        interrupt.store(true, Ordering::Relaxed);
        signal_handlers.run(SignalAction::Interrupt);
    })
    .expect("Error setting Ctrl-C handler");
}


Review comment (suggestion):
also let's make this a bit more explicit
```suggestion
                        let _ = jobs.kill_all();
```

Code after:
use nu_protocol::{Handlers, SignalAction, Signals, engine::EngineState};
use std::sync::{
    Arc,
    atomic::{AtomicBool, Ordering},
};

pub(crate) fn ctrlc_protection(engine_state: &mut EngineState) {
    let interrupt = Arc::new(AtomicBool::new(false));
    engine_state.set_signals(Signals::new(interrupt.clone()));

    let signal_handlers = Handlers::new();

    // Register a handler to kill all background jobs on interrupt.
    signal_handlers
        .register_unguarded({
            let jobs = engine_state.jobs.clone();
            Box::new(move |action| {
                if action == SignalAction::Interrupt {
                    if let Ok(mut jobs) = jobs.lock() {
                        let _ = jobs.kill_all();
                    }
                }
            })
        })
        .expect("Failed to register interrupt signal handler");

    engine_state.signal_handlers = Some(signal_handlers.clone());

    ctrlc::set_handler(move || {
        interrupt.store(true, Ordering::Relaxed);
        signal_handlers.run(SignalAction::Interrupt);
    })
    .expect("Error setting Ctrl-C handler");
}
Repository: nushell/nushell
File: crates/nu-command/tests/commands/let_.rs (Rust)

Code before:

#[test]
fn let_glob_type() {
    let actual = nu!("let x: glob = 'aa'; $x | describe");
    assert_eq!(actual.out, "glob");
}

#[test]
fn let_raw_string() {
    let actual = nu!(r#"let x = r#'abcde""fghi"''''jkl'#; $x"#);
    assert_eq!(actual.out, r#"abcde""fghi"''''jkl"#);

    let actual = nu!(r#"let x = r##'abcde""fghi"''''#jkl'##; $x"#);
    assert_eq!(actual.out, r#"abcde""fghi"''''#jkl"#);

    let actual = nu!(r#"let x = r###'abcde""fghi"'''##'#jkl'###; $x"#);
    assert_eq!(actual.out, r#"abcde""fghi"'''##'#jkl"#);

    let actual = nu!(r#"let x = r#'abc'#; $x"#);
    assert_eq!(actual.out, "abc");
}

#[test]
fn let_malformed_type() {
    let actual = nu!("let foo: )a");
    assert!(actual.err.contains("unbalanced ( and )"));

    let actual = nu!("let foo: }a");
    assert!(actual.err.contains("unbalanced { and }"));
}


Review comment (suggestion):
Gotta admit I can't wrap my head around how the change on the `}` side fixes the `)` panic :D

Code after:

#[test]
fn let_glob_type() {
    let actual = nu!("let x: glob = 'aa'; $x | describe");
    assert_eq!(actual.out, "glob");
}

#[test]
fn let_raw_string() {
    let actual = nu!(r#"let x = r#'abcde""fghi"''''jkl'#; $x"#);
    assert_eq!(actual.out, r#"abcde""fghi"''''jkl"#);

    let actual = nu!(r#"let x = r##'abcde""fghi"''''#jkl'##; $x"#);
    assert_eq!(actual.out, r#"abcde""fghi"''''#jkl"#);

    let actual = nu!(r#"let x = r###'abcde""fghi"'''##'#jkl'###; $x"#);
    assert_eq!(actual.out, r#"abcde""fghi"'''##'#jkl"#);

    let actual = nu!(r#"let x = r#'abc'#; $x"#);
    assert_eq!(actual.out, "abc");
}

#[test]
fn let_malformed_type() {
    let actual = nu!("let foo: )a");
    assert!(actual.err.contains("unbalanced ( and )"));

    let actual = nu!("let foo: }a");
    assert!(actual.err.contains("unbalanced { and }"));

    let actual = nu!("mut : , a");
    assert!(actual.err.contains("unknown type"));
}
Repository: nushell/nushell
File: crates/nu-command/src/path/relative_to.rs (Rust)

Code before:
    match lhs.strip_prefix(&rhs) {
        Ok(p) => Value::string(p.to_string_lossy(), span),
        Err(e) => Value::error(
            ShellError::CantConvert {
                to_type: e.to_string(),
                from_type: "string".into(),
                span,
                help: None,
            },
            span,
        ),
    }
}

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

    #[test]
    fn test_examples() {
        use crate::test_examples;

        test_examples(PathRelativeTo {})
    }
}


Review comment (security):
I'm not sure if hardcoding this based on the platform is a good idea. You can access NTFS or APFS from Linux, isn't there a better way to check the path belonging to a case insensitive fs?

Code after:

    match lhs.strip_prefix(&rhs) {
        Ok(p) => Value::string(p.to_string_lossy(), span),
        Err(e) => {
            // On case-insensitive filesystems, try case-insensitive comparison
            if is_case_insensitive_filesystem() {
                if let Some(relative_path) = try_case_insensitive_strip_prefix(&lhs, &rhs) {
                    return Value::string(relative_path.to_string_lossy(), span);
                }
            }

            Value::error(
                ShellError::CantConvert {
                    to_type: e.to_string(),
                    from_type: "string".into(),
                    span,
                    help: None,
                },
                span,
            )
        }
    }
}

/// Check if the current filesystem is typically case-insensitive
fn is_case_insensitive_filesystem() -> bool {
    // Windows and macOS typically have case-insensitive filesystems
    cfg!(any(target_os = "windows", target_os = "macos"))
}

/// Try to strip prefix in a case-insensitive manner
fn try_case_insensitive_strip_prefix(lhs: &Path, rhs: &Path) -> Option<std::path::PathBuf> {
    let mut lhs_components = lhs.components();
    let mut rhs_components = rhs.components();

    // Compare components case-insensitively
    loop {
        match (lhs_components.next(), rhs_components.next()) {
            (Some(lhs_comp), Some(rhs_comp)) => {
                match (lhs_comp, rhs_comp) {
                    (
                        std::path::Component::Normal(lhs_name),
                        std::path::Component::Normal(rhs_name),
                    ) => {
                        if lhs_name.to_string_lossy().to_lowercase()
                            != rhs_name.to_string_lossy().to_lowercase()
                        {
                            return None;
                        }
                    }
                    // Non-Normal components must match exactly