Skip to content

cat: distinguish read and write errors - #14721

Merged
cakebaker merged 1 commit into
uutils:mainfrom
wtcpython:cat-distinct-err
Oct 15, 2026
Merged

cakebaker merged 1 commit into
uutils:mainfrom
wtcpython:cat-distinct-err

Conversation

@wtcpython

Copy link
Copy Markdown
Contributor

Should make test cat/cat-distinct-err.sh pass

Comment thread src/uucore/src/lib/features/pipes.rs
Comment thread src/uucore/src/lib/features/pipes.rs
PipeError::Read(error) | PipeError::Write(error) => error,
}
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is currently unwrapping Read,Write for other utils. But we should distinct at all utils and remove this impl in the future. This is why we should https://github.com/uutils/coreutils/pull/14721/changes#r4053088325 .

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.

This is fine for now.

@github-actions

github-actions Bot commented Sep 19, 2026 •

Copy link
Copy Markdown

GNU testsuite comparison:

Congrats! The gnu test tests/cat/cat-distinct-err is no longer failing!

@wtcpython
wtcpython marked this pull request as ready for review September 19, 2026 14:31
Copilot AI lite review requested due to automatic review settings September 19, 2026 14:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI review requested due to automatic review settings September 19, 2026 18:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI review requested due to automatic review settings September 21, 2026 00:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Comment thread src/uu/cat/src/cat.rs
#[error("{}", strip_errno(.0))]
Io(#[from] io::Error),
#[error("{}: {}", translate!("common-write-error"), strip_errno(.0))]
Write(io::Error),

@oech3 oech3 Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we directly put uucore::io::DistinguishedIOError::Write at here?

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.

uucore::io::DistinguishedIOError does not yet exist, let's fix the test and refactor later.

@codspeed

codspeed Bot commented Sep 21, 2026 •

Copy link
Copy Markdown

Merging this PR will regress 1 benchmark

⚠️ 117 benchmarks spent significant time in system calls

System calls cannot be consistently instrumented, so they are not included in the measure, which understates the real cost. Please switch to the Walltime instrument to accurately measure system calls.

Measurement and system calls

ℹ️ 1 benchmark has no flame graph

The flamegraph could not be generated because too much time was spent in runtime internals, making the visualization not useful for performance analysis.

⚡ 1 improved benchmark
❌ 1 regressed benchmark
✅ 399 untouched benchmarks
⏩ 54 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
❌ Simulation three_39_bit_primes 591.3 ms 686 ms -13.8%
⚡ Simulation five_38_bit_primes 2.1 s 1.5 s +34.85%

Tip

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


Comparing wtcpython:cat-distinct-err (e5c769e) with main (01a8fc0)

Open in CodSpeed

Footnotes

  1. 54 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

Copilot AI review requested due to automatic review settings September 24, 2026 07:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI review requested due to automatic review settings September 25, 2026 00:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI review requested due to automatic review settings September 25, 2026 09:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI review requested due to automatic review settings September 27, 2026 00:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI review requested due to automatic review settings September 29, 2026 06:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI lite review requested due to automatic review settings September 30, 2026 03:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI lite review requested due to automatic review settings October 1, 2026 11:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

Review effort: Lite
Findings: 2 Medium severity · 1 Low severity

Open (3)

Read(std::io::Error),
Write(std::io::Error),
}

Comment on lines +109 to +112
if drain_pipe(pipe_rd, dest, n)
.map_err(PipeError::Write)?
.is_err()
{
Comment thread tests/by-util/test_cat.rs
.ignore_stdin_write_error()
.fails()
.stderr_contains("No space left on device");
.stderr_is("cat: write error: No space left on device\n");
Copilot AI lite review requested due to automatic review settings October 2, 2026 10:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI lite review requested due to automatic review settings October 4, 2026 01:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Comment thread src/uu/cat/src/cat.rs Outdated
if let Err(err) = cat_path(path, options, &mut state) {
error_messages.push(format!("{}: {err}", path.maybe_quote()));
let is_write_error = matches!(&err, CatError::Write(_));
error_messages.push(match &err {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

could be simpler, no? if let CatError::Write(_) = err { error_messages.push(err.to_string()); break; } and keep the old push otherwise

Comment thread src/uu/cat/src/cat.rs
.inspect_err(handle_broken_pipe)?;
.inspect_err(handle_broken_pipe)
.map_err(CatError::Write)?;
// cannot use rustix::io on Windows

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what about print_lines? cat -n - /dev/zero > /dev/full still goes through Io, so it gets the -: prefix and doesn't stop
please handle it too and add a test with -n

}

type PipeRes = std::io::Result<Result<(), ()>>;
type SpliceRes = Result<Result<(), ()>, PipeError>;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the doc explaining the Ok(Ok)/Ok(Err)/Err meaning was removed, could you please keep it on SpliceRes?

Copilot AI lite review requested due to automatic review settings October 9, 2026 00:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI lite review requested due to automatic review settings October 9, 2026 00:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI lite review requested due to automatic review settings October 10, 2026 15:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@cakebaker
cakebaker merged commit 59cd0b5 into uutils:main Oct 15, 2026
190 of 193 checks passed
@cakebaker

Copy link
Copy Markdown
Contributor

Congrats! The gnu test tests/cat/cat-distinct-err is no longer failing!

Great, thanks!

@wtcpython
wtcpython deleted the cat-distinct-err branch October 15, 2026 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants