From 277adebabdaa661540fc5f63e153761609bb2c0c Mon Sep 17 00:00:00 2001 From: binhvq Date: Wed, 11 Nov 2026 14:34:36 +0700 Subject: [PATCH 1/2] cmp: keep error messages when stdout is /dev/null The /dev/null optimisation turned on -s, which also hides error messages, so 'cmp a missing > /dev/null' exited 2 without saying why and 'cmp -l -s > /dev/null' no longer rejected the two options. Track the redirect separately and only skip the difference and EOF reports for it. --- src/cmp.rs | 31 ++++++++++++------- tests/by-util/test_cmp.rs | 63 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 11 deletions(-) diff --git a/src/cmp.rs b/src/cmp.rs index cdd0b5b9..01a5efa3 100644 --- a/src/cmp.rs +++ b/src/cmp.rs @@ -36,6 +36,15 @@ pub struct Params { max_bytes: Option, verbose: bool, quiet: bool, + stdout_is_dev_null: bool, +} + +impl Params { + /// Whether the difference and EOF reports are skipped. Error messages + /// are only hidden by `quiet`. + fn no_report(&self) -> bool { + self.quiet || self.stdout_is_dev_null + } } #[inline] @@ -223,21 +232,21 @@ pub fn parse_params>(mut opts: Peekable) -> Resu } } - // Do as GNU cmp, and completely disable printing if we are + if params.quiet && params.verbose { + return Err(format!( + "{executable_str}: options -l and -s are incompatible" + )); + } + + // Do as GNU cmp, and disable reporting the differences if we are // outputting to /dev/null. #[cfg(not(target_os = "windows"))] if is_stdout_dev_null() { - params.quiet = true; + params.stdout_is_dev_null = true; params.verbose = false; params.print_bytes = false; } - if params.quiet && params.verbose { - return Err(format!( - "{executable_str}: options -l and -s are incompatible" - )); - } - params.from = if let Some(from) = from { from } else if let Some(param) = opts.next() { @@ -317,7 +326,7 @@ pub fn cmp(params: &Params) -> Result { // If the files have different sizes, we already know they are not identical. If we have not // been asked to show even the first difference, we can quit early. - if params.quiet && a_size != b_size { + if params.no_report() && a_size != b_size { return Ok(Cmp::Different); } @@ -631,7 +640,7 @@ fn report_eof( eof_on: &str, params: &Params, ) { - if params.quiet { + if params.no_report() { return; } @@ -675,7 +684,7 @@ fn report_difference( at_line: u64, params: &Params, ) { - if params.quiet { + if params.no_report() { return; } diff --git a/tests/by-util/test_cmp.rs b/tests/by-util/test_cmp.rs index 020a941f..e39cf451 100644 --- a/tests/by-util/test_cmp.rs +++ b/tests/by-util/test_cmp.rs @@ -72,6 +72,69 @@ fn cmp_incompatible_params() -> Result<(), Box> { Ok(()) } +// Runs cmp with stdout on /dev/null, which turns the difference output off. +#[cfg(unix)] +fn cmp_to_dev_null(args: I) -> std::io::Result +where + I: IntoIterator, + S: AsRef, +{ + std::process::Command::new(assert_cmd::cargo::cargo_bin!("diffutils")) + .arg("cmp") + .args(args) + .stdout(OpenOptions::new().write(true).open("/dev/null")?) + .output() +} + +// Unlike -s, sending stdout to /dev/null must not hide error messages. +#[test] +#[cfg(unix)] +fn cmp_dev_null_stdout_still_reports_errors() -> Result<(), Box> { + let tmp_dir = tempdir()?; + let a_path = tmp_dir.path().join("a"); + std::fs::write(&a_path, b"abc")?; + + let output = cmp_to_dev_null([&a_path, &tmp_dir.path().join("missing")])?; + assert_eq!(output.status.code(), Some(2)); + assert!(String::from_utf8_lossy(&output.stderr).contains("No such file or directory")); + + let output = cmp_to_dev_null(["-l".as_ref(), "-s".as_ref(), a_path.as_os_str()])?; + assert_eq!(output.status.code(), Some(2)); + assert!( + String::from_utf8_lossy(&output.stderr).ends_with(": options -l and -s are incompatible\n") + ); + + Ok(()) +} + +// The difference and EOF reports stay off when stdout is /dev/null. +#[test] +#[cfg(unix)] +fn cmp_dev_null_stdout_reports_no_difference() -> Result<(), Box> { + let tmp_dir = tempdir()?; + let a_path = tmp_dir.path().join("a"); + std::fs::write(&a_path, b"abcdef")?; + let shorter_path = tmp_dir.path().join("shorter"); + std::fs::write(&shorter_path, b"abc")?; + let changed_path = tmp_dir.path().join("changed"); + std::fs::write(&changed_path, b"abcdeX")?; + + for other in [&shorter_path, &changed_path] { + for option in [None, Some("-l"), Some("-b")] { + let output = cmp_to_dev_null( + option + .iter() + .map(std::ffi::OsStr::new) + .chain([a_path.as_os_str(), other.as_os_str()]), + )?; + assert_eq!(output.status.code(), Some(1)); + assert!(output.stderr.is_empty()); + } + } + + Ok(()) +} + #[test] fn cmp_stdin() -> Result<(), Box> { let tmp_dir = tempdir()?; From a0a081108e405448f681df0d7488c2934ce18599 Mon Sep 17 00:00:00 2001 From: binhvq Date: Thu, 12 Nov 2026 04:06:52 +0700 Subject: [PATCH 2/2] tests: port the cmp /dev/null tests to uutests --- tests/by-util/test_cmp.rs | 79 +++++++++++++++------------------------ 1 file changed, 31 insertions(+), 48 deletions(-) diff --git a/tests/by-util/test_cmp.rs b/tests/by-util/test_cmp.rs index 4531fa4a..f4c07923 100644 --- a/tests/by-util/test_cmp.rs +++ b/tests/by-util/test_cmp.rs @@ -61,67 +61,50 @@ fn cmp_stdin(#[case] input: &str, #[case] code: i32, #[case] expected: &str) { .stdout_only(expected); } -// Runs cmp with stdout on /dev/null, which turns the difference output off. +// cmp turns the difference output off when its stdout is /dev/null. #[cfg(unix)] -fn cmp_to_dev_null(args: I) -> std::io::Result -where - I: IntoIterator, - S: AsRef, -{ - std::process::Command::new(assert_cmd::cargo::cargo_bin!("diffutils")) - .arg("cmp") - .args(args) - .stdout(OpenOptions::new().write(true).open("/dev/null")?) - .output() +fn dev_null() -> std::fs::File { + OpenOptions::new().write(true).open("/dev/null").unwrap() } // Unlike -s, sending stdout to /dev/null must not hide error messages. #[test] #[cfg(unix)] -fn cmp_dev_null_stdout_still_reports_errors() -> Result<(), Box> { - let tmp_dir = tempdir()?; - let a_path = tmp_dir.path().join("a"); - std::fs::write(&a_path, b"abc")?; - - let output = cmp_to_dev_null([&a_path, &tmp_dir.path().join("missing")])?; - assert_eq!(output.status.code(), Some(2)); - assert!(String::from_utf8_lossy(&output.stderr).contains("No such file or directory")); - - let output = cmp_to_dev_null(["-l".as_ref(), "-s".as_ref(), a_path.as_os_str()])?; - assert_eq!(output.status.code(), Some(2)); - assert!( - String::from_utf8_lossy(&output.stderr).ends_with(": options -l and -s are incompatible\n") - ); - - Ok(()) +fn cmp_dev_null_stdout_still_reports_errors() { + let (at, ts) = at_and_ts!(); + at.write("a", "abc"); + + ts.ucmd() + .args(&["a", "missing"]) + .set_stdout(dev_null()) + .fails_with_code(2) + .stderr_contains("No such file or directory"); + ts.ucmd() + .args(&["-l", "-s", "a"]) + .set_stdout(dev_null()) + .fails_with_code(2) + .stderr_str_check(|s| s.ends_with(": options -l and -s are incompatible\n")); } // The difference and EOF reports stay off when stdout is /dev/null. #[test] #[cfg(unix)] -fn cmp_dev_null_stdout_reports_no_difference() -> Result<(), Box> { - let tmp_dir = tempdir()?; - let a_path = tmp_dir.path().join("a"); - std::fs::write(&a_path, b"abcdef")?; - let shorter_path = tmp_dir.path().join("shorter"); - std::fs::write(&shorter_path, b"abc")?; - let changed_path = tmp_dir.path().join("changed"); - std::fs::write(&changed_path, b"abcdeX")?; - - for other in [&shorter_path, &changed_path] { - for option in [None, Some("-l"), Some("-b")] { - let output = cmp_to_dev_null( - option - .iter() - .map(std::ffi::OsStr::new) - .chain([a_path.as_os_str(), other.as_os_str()]), - )?; - assert_eq!(output.status.code(), Some(1)); - assert!(output.stderr.is_empty()); +fn cmp_dev_null_stdout_reports_no_difference() { + let (at, ts) = at_and_ts!(); + at.write("a", "abcdef"); + at.write("shorter", "abc"); + at.write("changed", "abcdeX"); + + for other in ["shorter", "changed"] { + for option in [&[][..], &["-l"], &["-b"]] { + ts.ucmd() + .args(option) + .args(&["a", other]) + .set_stdout(dev_null()) + .fails_with_code(1) + .no_stderr(); } } - - Ok(()) } #[test]