sort: report read failures with context - #14005
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves sort’s user-facing error reporting for input read failures by adding a dedicated localized message that includes context (“read failed”) and strips Rust’s trailing "(os error N)" suffix, along with an integration test that locks in the new output.
Changes:
- Wrap read errors in
chunks.rswith aread failedcontext message and format the underlyingio::Errorviastrip_errno. - Add a new Fluent localization key
sort-read-failedfor the new message. - Add a Linux-only regression test using
/proc/self/memto verify the exact stderr output.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/by-util/test_sort.rs | Adds a Linux-only regression test asserting the improved read-failure message formatting. |
| src/uu/sort/src/chunks.rs | Replaces raw io::Error::to_string() with a translated, contextualized error message using strip_errno. |
| src/uu/sort/locales/en-US.ftl | Introduces the new sort-read-failed localization string used by the new error path. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
A read error was reported as the bare io::Error string, e.g.
`sort: Input/output error (os error 5)` for `sort /proc/self/mem`.
That names neither the operation that failed nor anything the user
can act on, and the `(os error 5)` suffix is noise.
Wrap it with a "read failed" context and drop the errno suffix using
the existing strip_errno helper, matching the style of the other
SortError variants:
sort: read failed: Input/output error
Fixes uutils#13992
7c16a97 to
f9dbbd3
Compare
Merging this PR will degrade performance by 3.72%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | cksum_crc |
41.4 ms | 43.1 ms | -3.86% |
| ❌ | Simulation | numfmt_stream_to_si_precision |
346.4 ms | 359.2 ms | -3.59% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing arcusbuilds:fix/sort-read-error-message (f9dbbd3) with main (1a36740)
Footnotes
-
50 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. ↩
|
GNU testsuite comparison: |
Fixes #13992.
sort /proc/self/memreported the read failure as the bareio::Errorstring:That names neither the operation that failed nor anything the user can act on, and the
(os error 5)suffix is noise. GNU printssort: read failed: /proc/self/mem: Input/output errorfor the same input.chunks.rswas callinge.to_string()directly. This wraps it with aread failedcontext and drops the errno suffix using the existingstrip_errnohelper, matching how the otherSortErrorvariants are rendered:It deliberately stops short of naming the file.
read_to_buffertakes&mut T: Read, andext_sorthands it an iterator of already-opened readers, so the path is gone by the time a read fails. Carrying it through would mean changing that iterator's item type and threading aPathBufthroughchunks.rsandext_sort/{mod,threaded,wasi}.rs. That is worth doing, but it is a much wider diff and I did not want to bundle the two. I am happy to follow up with it, or to fold it into this PR if you would rather have the whole fix at once.The existing
sort-cannot-readstring is also wordedcannot read: {$path}: {$error}rather thanread failed:, so a path-carrying version would need either a reworded key or a new one. That is another reason to keep the change separate.Tests:
test_read_error_messageintests/by-util/test_sort.rs, Linux-only since it relies on/proc/self/memreturningEIO. It fails onmainwith the old message and passes here. The fulltest_sortsuite is green (187 passed), andcargo fmt --checkandcargo clippy -p uu_sort --all-targets -- -D warningsare both clean.