cmp: honor the byte limit in quiet size checks

src/cmp.rs

@@ -317,8 +317,21 @@ pub fn cmp(params: &Params) -> Result<Cmp, String> {
 
         // 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 {
-            return Ok(Cmp::Different);
+        if params.quiet
+            && params.from != "-"
+            && params.to != "-"
+            && a_meta.is_file()
+            && b_meta.is_file()
+        {
+            let remaining = |size: u64, skip: Option<SkipU64>| {
+                let remaining = size.saturating_sub(skip.unwrap_or(0));
+                params
+                    .max_bytes
+                    .map_or(remaining, |max_bytes| remaining.min(max_bytes))
+            };
+            if remaining(a_size, params.skip_a) != remaining(b_size, params.skip_b) {
+                return Ok(Cmp::Different);
+            }
         }
 
         let smaller = cmp::min(a_size, b_size) as BytesLimitU64;
@@ -336,6 +349,15 @@ pub fn cmp(params: &Params) -> Result<Cmp, String> {
     let mut stdout = BufWriter::new(io::stdout().lock());
     let mut compare = Cmp::Equal;
     loop {
+        // Once the limit is reached, don't wait for more input.
+        if at_byte > 1
+            && params
+                .max_bytes
+                .is_some_and(|max_bytes| at_byte > max_bytes)
+        {
+            break;
+        }
+
         // Fill up our buffers.
         let from_buf = from
             .fill_buf()
@@ -345,6 +367,13 @@ pub fn cmp(params: &Params) -> Result<Cmp, String> {
             .fill_buf()
             .map_err(|e| format_failure_to_read_input_file(&params.executable, &params.to, &e))?;
 
+        if params
+            .max_bytes
+            .is_some_and(|max_bytes| at_byte > max_bytes)
+        {
+            break;
+        }
+
         // Check for EOF conditions.
         if from_buf.is_empty() && to_buf.is_empty() {
             break;
@@ -364,6 +393,9 @@ pub fn cmp(params: &Params) -> Result<Cmp, String> {
         // Fast path - for long files in which almost all bytes are the same we
         // can do a direct comparison to let the compiler optimize.
         let consumed = std::cmp::min(from_buf.len(), to_buf.len());
+        let consumed = params.max_bytes.map_or(consumed, |max_bytes| {
+            consumed.min(usize::try_from(max_bytes - (at_byte - 1)).unwrap_or(usize::MAX))
+        });
         if from_buf[..consumed] == to_buf[..consumed] {
             let last = from_buf[..consumed].last().unwrap();
 
@@ -372,12 +404,6 @@ pub fn cmp(params: &Params) -> Result<Cmp, String> {
 
             start_of_line = *last == b'\n';
 
-            if let Some(max_bytes) = params.max_bytes {
-                if at_byte > max_bytes {
-                    break;
-                }
-            }
-
             from.consume(consumed);
             to.consume(consumed);
 
@@ -386,7 +412,7 @@ pub fn cmp(params: &Params) -> Result<Cmp, String> {
 
         // Iterate over the buffers, the zip iterator will stop us as soon as the
         // first one runs out.
-        for (&from_byte, &to_byte) in from_buf.iter().zip(to_buf.iter()) {
+        for (&from_byte, &to_byte) in from_buf[..consumed].iter().zip(&to_buf[..consumed]) {
             if from_byte != to_byte {
                 compare = Cmp::Different;
 
@@ -418,12 +444,6 @@ pub fn cmp(params: &Params) -> Result<Cmp, String> {
             }
 
             at_byte += 1;
-
-            if let Some(max_bytes) = params.max_bytes {
-                if at_byte > max_bytes {
-                    break;
-                }
-            }
         }
 
         // Notify our readers about the bytes we went over.

tests/by-util/test_cmp.rs

@@ -3,12 +3,14 @@
 // For the full copyright and license information, please view the LICENSE-*
 // files that was distributed with this source code.
 
+use assert_cmd::assert::OutputAssertExt;
 use assert_cmd::cargo::cargo_bin_cmd;
 use predicates::prelude::*;
 use std::fs::File;
 #[cfg(not(windows))]
 use std::fs::OpenOptions;
 use std::io::Write;
+use std::process::Stdio;
 use tempfile::{tempdir, NamedTempFile};
 
 // A file whose metadata length is 0 but which still yields bytes (/dev/zero)
@@ -318,6 +320,108 @@ fn cmp_max_bytes() -> Result<(), Box<dyn std::error::Error>> {
     Ok(())
 }
 
+#[track_caller]
+fn assert_cmp(dir: &std::path::Path, args: &[&str], stdin: Stdio, code: i32, stderr: &str) {
+    std::process::Command::new(assert_cmd::cargo::cargo_bin!("diffutils"))
+        .env("LC_ALL", "C")
+        .current_dir(dir)
+        .arg("cmp")
+        .args(args)
+        .stdin(stdin)
+        .output()
+        .unwrap()
+        .assert()
+        .append_context("args", format!("{args:?}"))
+        .code(predicate::eq(code))
+        .stdout(predicate::str::is_empty())
+        .stderr(predicate::eq(stderr));
+}
+
+#[test]
+fn cmp_quiet_bytes_limit() -> Result<(), Box<dyn std::error::Error>> {
+    let tmp_dir = tempdir()?;
+    let dir = tmp_dir.path();
+    for name in ["a", "ab", "b", "abc", "xabc"] {
+        std::fs::write(dir.join(name), name)?;
+    }
+    for (args, code) in [
+        (&["-s", "-n", "1", "a", "ab"][..], 0),
+        (&["-s", "-n", "2", "a", "ab"][..], 1),
+        (&["-s", "-n", "0", "a", "b"][..], 0),
+        (&["-s", "-i", "1:0", "xabc", "abc"][..], 0),
+        (&["-s", "-i", "1:0", "-n", "2", "xabc", "ab"][..], 0),
+    ] {
+        assert_cmp(dir, args, Stdio::null(), code, "");
+    }
+    std::fs::write(dir.join("-"), "zz")?;
+    for (args, stdin) in [
+        (&["-s", "-", "abc"][..], "abc"),
+        (&["-s", "-i", "0:1", "abc", "-"], "xabc"),
+    ] {
+        assert_cmp(dir, args, File::open(dir.join(stdin))?.into(), 0, "");
+    }
+    Ok(())
+}
+
+#[test]
+#[cfg(unix)]
+fn cmp_directory_and_device_operands() -> Result<(), Box<dyn std::error::Error>> {
+    let tmp_dir = tempdir()?;
+    let dir = tmp_dir.path();
+    std::fs::write(dir.join("file"), "a")?;
+    std::fs::write(dir.join("nul"), "\0")?;
+    std::fs::create_dir(dir.join("dir"))?;
+    for (args, code, stderr) in [
+        (&["-s", "file", "dir"][..], 2, ""),
+        (
+            &["-n", "0", "file", "dir"][..],
+            2,
+            "cmp: dir: Is a directory\n",
+        ),
+        (&["-s", "-n", "1", "/dev/zero", "nul"][..], 0, ""),
+    ] {
+        assert_cmp(dir, args, Stdio::null(), code, stderr);
+    }
+    let stdin = File::open(dir)?.into();
+    assert_cmp(
+        dir,
+        &["-n", "0", "file", "-"],
+        stdin,
+        2,
+        "cmp: -: Is a directory\n",
+    );
+    Ok(())
+}
+
+#[test]
+#[cfg(unix)]
+fn cmp_bytes_limit_with_open_pipe() -> Result<(), Box<dyn std::error::Error>> {
+    let tmp_dir = tempdir()?;
+    let file = tmp_dir.path().join("file");
+    std::fs::write(&file, "abcde")?;
+
+    let mut child = std::process::Command::new(assert_cmd::cargo::cargo_bin!("diffutils"))
+        .args(["cmp", "-n", "5", "-"])
+        .arg(&file)
+        .stdin(Stdio::piped())
+        .stdout(Stdio::null())
+        .spawn()?;
+    // The write end stays open, so another read would block.
+    let mut stdin = child.stdin.take().unwrap();
+    stdin.write_all(b"abcde")?;
+
+    for _ in 0..500 {
+        if let Some(status) = child.try_wait()? {
+            assert_eq!(status.code(), Some(0));
+            return Ok(());
+        }
+        std::thread::sleep(std::time::Duration::from_millis(10));
+    }
+    child.kill()?;
+    child.wait()?;
+    panic!("cmp -n 5 kept reading after the limit");
+}
+
 #[test]
 fn cmp_skip_args_parsing() -> Result<(), Box<dyn std::error::Error>> {
     let tmp_dir = tempdir()?;
@@ -505,39 +609,44 @@ fn cmp_fast_paths() -> Result<(), Box<dyn std::error::Error>> {
     // validating the /dev/null optimization.
     let a_path = tmp_dir.path().join("a");
     let a = File::create(&a_path).unwrap();
-    a.set_len(14 * 1024 * 1024 * 1024 * 1024).unwrap();
+    let a_len = 14 * 1024 * 1024 * 1024 * 1024;
+    a.set_len(a_len).unwrap();
 
     let b_path = tmp_dir.path().join("b");
     let b = File::create(&b_path).unwrap();
     b.set_len(15 * 1024 * 1024 * 1024 * 1024).unwrap();
 
     let dev_null = OpenOptions::new().write(true).open("/dev/null").unwrap();
 
-    let mut child = std::process::Command::new(assert_cmd::cargo::cargo_bin!("diffutils"))
-        .arg("cmp")
-        .arg(&a_path)
-        .arg(&b_path)
-        .stdout(dev_null)
-        .spawn()
-        .unwrap();
-
-    // Bound the runtime to a very short time that still allows for some resource
-    // constraint to slow it down while also allowing very fast systems to exit as
-    // early as possible.
-    const MAX_TRIES: u8 = 50;
-    for tries in 0..=MAX_TRIES {
-        if tries == MAX_TRIES {
-            panic!("cmp took too long to run, /dev/null optimization probably not working")
-        }
-        match child.try_wait() {
-            Ok(Some(status)) => {
-                assert_eq!(status.code(), Some(1));
-                break;
+    let limit = (a_len + 1).to_string();
+    for args in [&[][..], &["-i", "0"], &["-i", "1"], &["-n", &limit]] {
+        let mut child = std::process::Command::new(assert_cmd::cargo::cargo_bin!("diffutils"))
+            .arg("cmp")
+            .args(args)
+            .arg(&a_path)
+            .arg(&b_path)
+            .stdout(dev_null.try_clone().unwrap())
+            .spawn()
+            .unwrap();
+
+        // Bound the runtime to a very short time that still allows for some resource
+        // constraint to slow it down while also allowing very fast systems to exit as
+        // early as possible.
+        const MAX_TRIES: u8 = 50;
+        for tries in 0..=MAX_TRIES {
+            if tries == MAX_TRIES {
+                panic!("cmp {args:?} took too long to run, /dev/null optimization probably not working")
             }
-            Ok(None) => (),
-            Err(e) => panic!("{e:#?}"),
+            match child.try_wait() {
+                Ok(Some(status)) => {
+                    assert_eq!(status.code(), Some(1));
+                    break;
+                }
+                Ok(None) => (),
+                Err(e) => panic!("{e:#?}"),
+            }
+            std::thread::sleep(std::time::Duration::from_millis(10));
         }
-        std::thread::sleep(std::time::Duration::from_millis(10));
     }
 
     // Two stdins should be equal