Skip to content

cmp: accept unit suffixes in --bytes #309

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

Merged
sylvestre merged 2 commits into from
Oct 24, 2026
Merged

cmp: accept unit suffixes in --bytes #309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e736b44
cmp: accept unit suffixes in --bytes
jayhemnani9910 Oct 19, 2026
f89e83b
cmp: call parse_size directly and test -n 1K
jayhemnani9910 Oct 23, 2026
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
60 changes: 36 additions & 24 deletions src/cmp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@ pub fn parse_params<I: Iterator<Item = OsString>>(mut opts: Peekable<I>) -> Resu
let executable = opts.next().ok_or("Usage: <exe> <from> <to>".to_string())?;
let executable_str = executable.to_string_lossy().to_string();

let parse_skip = |param: &str, skip_desc: &str| -> Result<SkipU64, String> {
// Sizes for --ignore-initial and --bytes, with the same unit suffixes.
let parse_size = |param: &str, option: &str, desc: &str| -> Result<SkipU64, String> {
let suffix_start = param
.find(|b: char| !b.is_ascii_digit())
.unwrap_or(param.len());
Expand All @@ -83,7 +84,7 @@ pub fn parse_params<I: Iterator<Item = OsString>>(mut opts: Peekable<I>) -> Resu
Err(e) if *e.kind() == std::num::IntErrorKind::PosOverflow => SkipU64::MAX,
Err(_) => {
return Err(format!(
"{executable_str}: invalid --ignore-initial value '{skip_desc}'"
"{executable_str}: invalid --{option} value '{desc}'"
))
}
};
Expand Down Expand Up @@ -111,7 +112,7 @@ pub fn parse_params<I: Iterator<Item = OsString>>(mut opts: Peekable<I>) -> Resu
"Y" => SkipU64::MAX, // 1_208_925_819_614_629_174_706_176,
_ => {
return Err(format!(
"{executable_str}: invalid --ignore-initial value '{skip_desc}'"
"{executable_str}: invalid --{option} value '{desc}'"
));
}
};
Expand Down Expand Up @@ -172,17 +173,7 @@ pub fn parse_params<I: Iterator<Item = OsString>>(mut opts: Peekable<I>) -> Resu
let (_, arg) = param_str.split_once('=').unwrap();
arg.to_string()
};
let max_bytes = match max_bytes.parse::<BytesLimitU64>() {
Ok(num) => num,
// TODO limit to MAX is dangerous, this should become an error like in GNU cmp.
Err(e) if *e.kind() == std::num::IntErrorKind::PosOverflow => BytesLimitU64::MAX,
Err(_) => {
return Err(format!(
"{executable_str}: invalid --bytes value '{max_bytes}'"
))
}
};
params.max_bytes = Some(max_bytes);
params.max_bytes = Some(parse_size(&max_bytes, "bytes", &max_bytes)?);
continue;
}
if param == "-i" || param_str.starts_with("--ignore-initial=") {
Expand All @@ -197,11 +188,11 @@ pub fn parse_params<I: Iterator<Item = OsString>>(mut opts: Peekable<I>) -> Resu
};
let (skip_a, skip_b) = if let Some((skip_a, skip_b)) = skip_desc.split_once(':') {
(
parse_skip(skip_a, &skip_desc)?,
parse_skip(skip_b, &skip_desc)?,
parse_size(skip_a, "ignore-initial", &skip_desc)?,
parse_size(skip_b, "ignore-initial", &skip_desc)?,
)
} else {
let skip = parse_skip(&skip_desc, &skip_desc)?;
let skip = parse_size(&skip_desc, "ignore-initial", &skip_desc)?;
(skip, skip)
};
params.skip_a = Some(skip_a);
Expand All @@ -224,9 +215,9 @@ pub fn parse_params<I: Iterator<Item = OsString>>(mut opts: Peekable<I>) -> Resu
} else if to.is_none() {
to = Some(param);
} else if skip_pos1.is_none() {
skip_pos1 = Some(parse_skip(&param_str, &param_str)?);
skip_pos1 = Some(parse_size(&param_str, "ignore-initial", &param_str)?);
} else if skip_pos2.is_none() {
skip_pos2 = Some(parse_skip(&param_str, &param_str)?);
skip_pos2 = Some(parse_size(&param_str, "ignore-initial", &param_str)?);
} else {
return Err(usage_string(&executable_str));
}
Expand Down Expand Up @@ -268,15 +259,15 @@ pub fn parse_params<I: Iterator<Item = OsString>>(mut opts: Peekable<I>) -> Resu
params.skip_a = skip_pos1;
} else if let Some(param) = opts.next() {
let param_str = param.to_string_lossy().to_string();
params.skip_a = Some(parse_skip(&param_str, &param_str)?);
params.skip_a = Some(parse_size(&param_str, "ignore-initial", &param_str)?);
}
};
if params.skip_b.is_none() {
if skip_pos2.is_some() {
params.skip_b = skip_pos2;
} else if let Some(param) = opts.next() {
let param_str = param.to_string_lossy().to_string();
params.skip_b = Some(parse_skip(&param_str, &param_str)?);
params.skip_b = Some(parse_size(&param_str, "ignore-initial", &param_str)?);
}
}

Expand Down Expand Up @@ -985,12 +976,33 @@ mod tests {
)
);

// Same unit suffixes as --ignore-initial

@sylvestre sylvestre Oct 23, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please also add a -n 1K case in cmp_max_bytes in tests/integration.rs

@jayhemnani9910 jayhemnani9910 Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in f89e83b: -n 1K in cmp_max_bytes. It fails on main with invalid --bytes value '1K'. cargo test passes and clippy is clean.

for (args, bytes) in [
(&["-n", "1K"][..], 1_024),
(&["--bytes=1kB"][..], 1_000),
(&["--bytes=2M"][..], 2_097_152),
] {
let mut opts = vec![os("cmp")];
opts.extend(args.iter().map(|arg| os(arg)));
opts.extend([os("foo"), os("bar")]);
assert_eq!(
Ok(Params {
executable: os("cmp"),
from: os("foo"),
to: os("bar"),
max_bytes: Some(bytes),
..Default::default()
}),
parse_params(opts.into_iter().peekable()),
"{args:?}"
);
}

// Failure case
// TODO This is actually fine in GNU cmp. --bytes does not have a unit parser yet.
assert_eq!(
Err("cmp: invalid --bytes value '1K'".to_string()),
Err("cmp: invalid --bytes value '1x'".to_string()),
parse_params(
[os("cmp"), os("--bytes=1K"), os("foo"), os("bar")]
[os("cmp"), os("--bytes=1x"), os("foo"), os("bar")]
.iter()
.cloned()
.peekable()
Expand Down
13 changes: 13 additions & 0 deletions tests/integration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -669,6 +669,19 @@ mod cmp {
cmd.arg("-n");
cmd.arg("13");
cmd.arg(&a_path).arg(&b_path);
cmd.assert()
.code(predicate::eq(1))
.failure()
.stderr(predicate::str::is_empty())
.stdout(predicate::eq(" 4 40 144 d\n 8 40 150 h\n"));

let mut cmd = cargo_bin_cmd!("diffutils");
cmd.arg("cmp");
cmd.arg("-l");
cmd.arg("-b");
cmd.arg("-n");
cmd.arg("1K");
cmd.arg(&a_path).arg(&b_path);
cmd.assert()
.code(predicate::eq(1))
.failure()
Expand Down
Loading