Skip to content

cmp: honor the byte limit in quiet size checks #296

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
SichenLiang wants to merge 1 commit into
base: main
Choose a base branch
from
+159 −40
Open

cmp: honor the byte limit in quiet size checks #296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 55 additions & 16 deletions src/cmp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use std::process::ExitCode;
use std::{cmp, fs, io};

#[cfg(unix)]
use std::os::fd::{AsRawFd, FromRawFd};
use std::os::fd::{AsFd, AsRawFd, FromRawFd};

#[cfg(unix)]
use std::os::unix::fs::MetadataExt;
Expand Down Expand Up @@ -311,9 +311,36 @@ pub enum Cmp {
Different,
}

fn is_directory(path: &OsString, params: &Params) -> Result<bool, String> {
if path != "-" {
return Ok(fs::metadata(path).is_ok_and(|metadata| metadata.is_dir()));
}
#[cfg(unix)]
{
io::stdin()
.as_fd()
.try_clone_to_owned()
.and_then(|fd| fs::File::from(fd).metadata())
.map(|metadata| metadata.is_dir())
.map_err(|e| format_failure_to_read_input_file(&params.executable, path, &e))
}
#[cfg(not(unix))]
{
let _ = params;
Ok(false)
}
}

pub fn cmp(params: &Params) -> Result<Cmp, String> {
// With -n 0 a directory operand must still be read, so that it is reported.
let reads_directory = match params.max_bytes {
Some(0) => is_directory(&params.from, params)
.and_then(|is_dir| Ok(is_dir || is_directory(&params.to, params)?)),
_ => Ok(false),
};
let mut from = prepare_reader(&params.from, &params.skip_a, params)?;
let mut to = prepare_reader(&params.to, &params.skip_b, params)?;
let stop_at_limit = !reads_directory?;

let mut offset_width = params.max_bytes.unwrap_or(BytesLimitU64::MAX);

Expand All @@ -326,8 +353,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;
Expand All @@ -345,6 +385,14 @@ pub fn cmp(params: &Params) -> Result<Cmp, String> {
let mut stdout = BufWriter::new(io::stdout().lock());
let mut compare = Cmp::Equal;
loop {
if stop_at_limit
&& params
.max_bytes
.is_some_and(|max_bytes| at_byte > max_bytes)
{
break;
}

// Fill up our buffers.
let from_buf = from
.fill_buf()
Expand Down Expand Up @@ -373,6 +421,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();

Expand All @@ -381,12 +432,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);

Expand All @@ -395,7 +440,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;

Expand Down Expand Up @@ -427,12 +472,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.
Expand Down
128 changes: 104 additions & 24 deletions tests/integration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};

// Integration tests for the diffutils command
Expand Down Expand Up @@ -642,6 +644,79 @@ mod cmp {
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]
fn cmp_skip_args_parsing() -> Result<(), Box<dyn std::error::Error>> {
let tmp_dir = tempdir()?;
Expand Down Expand Up @@ -829,39 +904,44 @@ mod cmp {
// 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")
}
match child.try_wait() {
Ok(Some(status)) => {
assert_eq!(status.code(), Some(1));
break;
}
Ok(None) => (),
Err(e) => panic!("{e:#?}"),
}
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
Expand Down