GeezerVison

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

GeezerVison is showing github.com — diff: return the difference status in side-by-side mode by SichenLiang · Pull Request #291 · uutils/diffutils

Conversation

SichenLiang commented Sep 28, 2026 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

The side-by-side renderer wrote directly to stdout but did not return a comparison result. Its caller therefore treated the comparison as identical and exited 0 even when the rendered output showed differences.

side_diff::diff now returns whether it saw a changed line, and the caller uses that value for the exit status. With -y -q, the renderer still runs but writes to io::sink(), leaving only the brief message on stdout.

Tests cover the exit status, -y -q, -y -s, and a missing file.

Fixes #290

Sorry, something went wrong.

Uh oh!

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

All reactions

SichenLiang mentioned this pull request

Oct 4, 2026

diff: clustered short options such as -ut and -qst are rejected #298

Open

sylvestre reviewed

Oct 23, 2026

View reviewed changes

Comment thread

src/diff.rs Outdated

		}

		// Side-by-side writes its output directly, so its result cannot indicate differences.
		let side_by_side_differs = params.format == Format::SideBySide && from_content != to_content;

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.

could side_diff::diff return whether it found differences instead of vec![]?
then we don't need this special case (or the comment)

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. side_diff::diff now returns whether it saw a changed line

Sorry, something went wrong.

Uh oh!

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

All reactions

sylvestre reviewed

Oct 23, 2026

View reviewed changes

Comment thread

src/diff.rs Outdated

		eprintln!("{error}");
		exit(2);
		}),
		Format::SideBySide if params.brief => Vec::new(),

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.

if side_diff returns the status, do we still need this arm? a sink writer would do

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.

I removed that branch. With -q, the renderer still runs but writes to io::sink().

Sorry, something went wrong.

Uh oh!

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

All reactions

sylvestre reviewed

Oct 23, 2026

View reviewed changes

Comment thread

tests/integration.rs Outdated

		}

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

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 test does too many things, could you please split it (exit status, -q, -s, missing file)?

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. I split it into separate tests for the exit status, -q, -s, and a missing file.

Sorry, something went wrong.

Uh oh!

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

All reactions

sylvestre reviewed

Oct 23, 2026

View reviewed changes

Comment thread

tests/integration.rs Outdated

		}

		std::fs::write(&left, b"alpha\n")?;
		for (right_content, identical) in [(b"alpha\n".as_slice(), true), (b"bravo\n", false)] {

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.

-y -s is already run in the loop above, could this be merged there?

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.

Sure -y -s is now another row in the table.

Sorry, something went wrong.

Uh oh!

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

All reactions

sylvestre reviewed

Oct 23, 2026

View reviewed changes

Comment thread

tests/integration.rs Outdated

		.code(predicate::eq(2))
		.failure()
		.stdout(predicate::str::is_empty())
		.stderr(predicate::str::contains("right"));

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.

please check the full error message, "right" is weak

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 test now checks the complete error message.

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


          diff: return the difference status in side-by-side mode

0fb54ee


          diff: let side_diff::diff report whether the files differ

6fc1ac5

Return whether `side_diff::diff` saw a changed line and use that value
for the exit status instead of comparing the inputs again. With
`-y -q`, keep running the renderer but send its output to `io::sink()`,
so only the brief message is printed.

SichenLiang force-pushed the side-by-side-exit-status branch from b99b21e to 6fc1ac5 Compare

October 24, 2026 18:07

codspeed Bot commented Oct 28, 2026

Copy link

Copy Markdown

Contributor

Merging this PR will degrade performance by 7.93%

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

⚡ 3 improved benchmarks
❌ 16 regressed benchmarks
✅ 7 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.6 µs	-13.36%
❌	`cmd_diff_gnu_equal[25000]`	22.7 µs	25.9 µs	-12.66%
❌	`cmd_diff_gnu_equal[10000]`	22.7 µs	25.9 µs	-12.27%
❌	`cmd_cmp_gnu_equal[25000]`	22.7 µs	25.6 µs	-11.65%
❌	`cmd_cmp_gnu_equal[10000]`	22.7 µs	25.6 µs	-11.44%
❌	`cmd_cmp_release_equal[25000]`	24.2 µs	27.3 µs	-11.38%
❌	`cmd_diff_gnu_equal[100]`	22.3 µs	25.1 µs	-11.25%
❌	`cmd_diff_release_equal[1000]`	24.3 µs	27.4 µs	-11.19%
❌	`cmd_diff_release_equal[10000]`	23.9 µs	26.8 µs	-10.94%
❌	`cmd_diff_gnu_equal[1000]`	22.4 µs	25.1 µs	-10.82%
❌	`cmd_cmp_release_equal[1000]`	24.4 µs	27.3 µs	-10.78%
❌	`cmd_diff_release_equal[100]`	24.2 µs	27 µs	-10.18%
❌	`cmd_cmp_release_equal[10000]`	24.5 µs	27.3 µs	-10.16%
❌	`cmd_cmp_gnu_equal[100]`	22.5 µs	24.4 µs	-8%
❌	`cmd_cmp_release_equal[100]`	25.1 µs	27.2 µs	-7.67%
❌	`cmd_cmp_gnu_equal[1000]`	22.5 µs	24.4 µs	-7.59%
⚡	`cmp_parser`	12.4 µs	10.7 µs	+15.71%
⚡	`diff_parser`	6.1 ms	5.7 ms	+6.85%
⚡	`cmp_compare_files_different[100]`	79.1 µs	76.6 µs	+3.3%

Tip

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

_{Comparing SichenLiang:side-by-side-exit-status (6fc1ac5) 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

sorry but it needs to be rebased, and 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.

diff: return the difference status with -y

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 Sep 28, 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!

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!

codspeed Bot commented Oct 28, 2026

Merging this PR will degrade performance by 7.93%

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 Sep 28, 2026 •

edited

Loading

Uh oh!

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