GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — cmp: honor the byte limit in quiet size checks by SichenLiang · Pull Request #296 · uutils/diffutils

Conversation

SichenLiang commented Oct 3, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

In quiet mode, when both operands are regular files and neither is -, subtract the skipped bytes from each size, stopping at zero, and then apply the -n limit. If the resulting lengths differ, report a difference without reading; otherwise compare the requested data.

Do not use the shortcut for -, because metadata for that path would describe a file literally named -, not standard input.

This fixes byte-limited comparisons and gives GNU-compatible status in the tested directory and /dev/zero cases. One difference remains: cmp -s a dir now returns 2, but quiet mode still omits GNU's Is a directory diagnostic.

New tests cover byte limits, skipped bytes, devices, directories, and -i 0.

Fixes #295

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

sylvestre reviewed

Oct 3, 2026

View reviewed changes

Comment thread

src/cmp.rs Outdated

		if params.quiet
		&& a_meta.is_file()
		&& b_meta.is_file()
		&& params.skip_a.unwrap_or(0) == 0

sylvestre Oct 3, 2026

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

why not subtract the skips from the sizes instead of disabling the shortcut?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

sylvestre reviewed

Oct 3, 2026

View reviewed changes

Comment thread

src/cmp.rs Outdated

		#[cfg(unix)]
		{
		let stdin = io::stdin().lock();
		// SAFETY: stdin's fd 0 must stay open and valid while locked.

sylvestre Oct 3, 2026

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

do we really need unsafe here?
io::stdin().as_fd().try_clone_to_owned() would be safe, and failing on an fd limit of 4 seems acceptable to me

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

sylvestre reviewed

Oct 3, 2026

View reviewed changes

Comment thread

tests/integration.rs Outdated


		#[test]
		#[cfg(unix)]
		fn cmp_zero_bytes_stdin_directory() -> Result<(), Box<dyn std::error::Error>> {

sylvestre Oct 3, 2026

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

this is mostly the same as cmp_zero_bytes_stdin_directory_fd_limit and cmp_zero_bytes_directory_operand.
could you please factor the cmd + assert part into a helper? 250 lines of tests is a lot for this

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

sylvestre reviewed

Oct 3, 2026

View reviewed changes

Comment thread

tests/integration.rs Outdated

		let mut cmd = std::process::Command::new("sh");
		cmd.env("LC_ALL", "C")
		.current_dir(tmp_dir.path())
		.args(["-c", "ulimit -n 4 \|\| exit; exec \"$@\"", "sh"])

sylvestre Oct 3, 2026

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

the ulimit -n 4 test seems overkill, could we drop it?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

sylvestre reviewed

Oct 3, 2026

View reviewed changes

Comment thread

tests/integration.rs Outdated


		#[test]
		#[cfg(not(windows))]
		fn cmp_quiet_size_shortcut_variants() -> Result<(), Box<dyn std::error::Error>> {

sylvestre Oct 3, 2026

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

almost the same as cmp_fast_paths, could this be merged into it?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

SichenLiang force-pushed the cmp-quiet-byte-limit branch from 3b39012 to 906e801 Compare

October 4, 2026 02:03

SichenLiang commented Oct 4, 2026

Copy link

Copy Markdown

Author

Thanks for the review. I’ve addressed all five comments.

The size shortcut now subtracts the skips and applies the -n limit before comparing the remaining lengths. The added -i 1 sparse-file case in cmp_fast_paths exercises this path.
I removed the unsafe block and now use io::stdin().as_fd().try_clone_to_owned(). A failure is reported as an error with status 2.
The command setup and assertions now live in an assert_cmp helper, and the cases have been reduced to two table-driven tests.
The ulimit -n 4 test is gone.
The separate size-shortcut test has been folded into cmp_fast_paths, which now runs the four relevant argument sets.

I also excluded - operands from the metadata shortcut. Otherwise, fs::metadata("-") describes a file literally named -, not stdin. This fixes the case where ./- contains zz and cmp -s - abc < abc incorrectly exits 1 on main instead of 0. If cloning stdin’s descriptor fails, the error is now reported after both operands are opened, preserving the existing error order.
One pre-existing parsing difference is now visible in quiet mode: when -i is present, positional skip operands are ignored. As a result, cmp -s -i 1:0 xabc abc 0 1 exits 0 while GNU exits 1; main already returns 0 for the same case without -s. I left the option parsing unchanged and can address it separately.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sylvestre reviewed

Oct 23, 2026

View reviewed changes

Comment thread

src/cmp.rs Outdated


		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 {

sylvestre Oct 23, 2026

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

this adds a lot of code just for -n 0 with a directory.
could we check the limit right after the two fill_buf calls (before the EOF checks) instead?
both operands would still be read once, so the directory error comes for free, and is_directory / stop_at_limit could go away

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

SichenLiang Oct 24, 2026

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Done. The limit is now checked after the two fill_buf() calls, and is_directory / stop_at_limit are gone still kept one small check at the top of the loop: once N bytes have been compared, waiting for another fill_buf() can block if the pipe is still open. For example, { printf abcde; sleep 3; } | cmp -n 5 - five waited three seconds without it

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

SichenLiang added 2 commits

October 24, 2026 16:48


          cmp: honor the byte limit in quiet size checks

b196be9

In quiet mode, when both operands are regular files and neither is `-`,
subtract the skipped bytes from each size, stopping at zero, and then
apply the `-n` limit. If the resulting lengths differ, report a
difference without reading; otherwise compare the requested data.

Do not use the shortcut for `-`, because metadata for that path would
describe a file literally named `-`, not standard input.

This fixes byte-limited comparisons and gives GNU-compatible status in
the tested directory and /dev/zero cases. One difference remains:
`cmp -s a dir` now returns 2, but quiet mode still omits GNU's
"Is a directory" diagnostic.

New tests cover byte limits, skipped bytes, devices, directories, and
`-i 0`.


          cmp: check the byte limit right after reading

eac3123

Check the limit after both operands have been read and before the EOF
checks. This keeps -n 0 reporting a directory operand while allowing
is_directory and stop_at_limit to be removed.

Keep a check at the top of the loop after at least one byte has been
compared. Without it, reaching the limit can trigger another read and
block on a pipe that is still open. Add a regression test for that
case.

SichenLiang force-pushed the cmp-quiet-byte-limit branch from 906e801 to eac3123 Compare

October 24, 2026 18:26

jayhemnani9910 mentioned this pull request

Oct 26, 2026

cmp: size the -l offset column from the bytes left after the skips #330

Merged

codspeed Bot commented Oct 28, 2026

Copy link

Copy Markdown

Contributor

Merging this PR will degrade performance by 9.16%

2 warnings

⚠️ 24 benchmarks are dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

⚠️ Base and head ran on different runner types

Some benchmarks with significant performance changes were compared across different runner types, so the changes might not come from the code. Run base and head on the same runner type, or use CodSpeed Macro Runners for more stable results.

Learn more about Macro Runners

Open the report in CodSpeed to investigate

⚡ 2 improved benchmarks
❌ 16 regressed benchmarks
✅ 8 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Benchmark	`BASE`	`HEAD`	Efficiency
❌	`cmd_diff_release_equal[25000]`	24 µs	27.9 µs	-14.05%
❌	`cmd_diff_gnu_equal[25000]`	22.7 µs	26.2 µs	-13.58%
❌	`cmd_diff_gnu_equal[10000]`	22.7 µs	26.2 µs	-13.2%
❌	`cmd_cmp_gnu_equal[25000]`	22.7 µs	25.9 µs	-12.41%
❌	`cmd_diff_gnu_equal[100]`	22.3 µs	25.4 µs	-12.22%
❌	`cmd_cmp_gnu_equal[10000]`	22.7 µs	25.9 µs	-12.2%
❌	`cmd_cmp_release_equal[25000]`	24.2 µs	27.5 µs	-11.92%
❌	`cmd_diff_release_equal[1000]`	24.3 µs	27.6 µs	-11.9%
❌	`cmd_diff_gnu_equal[1000]`	22.4 µs	25.4 µs	-11.8%
❌	`cmd_diff_release_equal[10000]`	23.9 µs	27.1 µs	-11.67%
❌	`cmd_cmp_release_equal[1000]`	24.4 µs	27.5 µs	-11.32%
❌	`cmd_diff_release_equal[100]`	24.2 µs	27.2 µs	-10.91%
❌	`cmd_cmp_release_equal[10000]`	24.5 µs	27.4 µs	-10.71%
❌	`cmd_cmp_gnu_equal[100]`	22.5 µs	24.7 µs	-8.83%
❌	`cmd_cmp_gnu_equal[1000]`	22.5 µs	24.6 µs	-8.42%
❌	`cmd_cmp_release_equal[100]`	25.1 µs	27.3 µs	-8.24%
⚡	`cmp_parser`	12.4 µs	10.6 µs	+16.91%
⚡	`diff_parser`	6.1 ms	5.7 ms	+6.81%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.

_{Comparing SichenLiang:cmp-quiet-byte-limit (eac3123) with main (6b3d629)}

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sylvestre commented Nov 10, 2026

Copy link

Copy Markdown

Collaborator

a bunch of jobs are failing

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Reviewers

sylvestre

sylvestre left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

cmp: quiet mode compares full file sizes before applying -n

Uh oh!

There was an error while loading. Please reload this page.

2 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

SichenLiang commented Oct 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SichenLiang commented Oct 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codspeed Bot commented Oct 28, 2026

Merging this PR will degrade performance by 9.16%

Performance Changes

Uh oh!

sylvestre commented Nov 10, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SichenLiang commented Oct 3, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.