Conversation
There was a problem hiding this comment.
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: 1
Open (2)
What changed in this PR
This PR updates stat error reporting to preserve non-UTF-8 filenames by quoting the original OS string rather than a lossy UTF-8 conversion (fixing #14685).
Changes:
- Use
OsStr::quote()output instaterror paths to avoid U+FFFD replacing invalid bytes. - Add an integration test covering missing non-UTF-8 filenames across default,
-L, and-fmodes.
| File | Description |
|---|---|
| tests/by-util/test_stat.rs | Adds a regression test ensuring diagnostics keep non-UTF-8 bytes when a path is missing. |
| src/uu/stat/src/stat.rs | Switches relevant error messages to use quoted original OsStr instead of lossy UTF-8 strings. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
GNU testsuite comparison: |
| .args(&args) | ||
| .arg(name) | ||
| .fails_with_code(1) | ||
| .stderr_contains("$'missing-\\xFF'"); |
There was a problem hiding this comment.
I think the expected output is not correct. With GNU stat I get the following output:
$ stat $'missing-\xFF'
stat: cannot statx 'missing-'$'\377': No such file or directory
There was a problem hiding this comment.
You are right: preserving the byte with Quotable still gave a different format. b36f315 switches these errors to the existing locale-aware shell quoter and expects octal escapes. I also fixed its single-quote/invalid-byte edge case. Thirty full stderr and exit-code comparisons against GNU stat 9.11 match across C/C.UTF-8, default/-L/-f and five filename cases. The 42 stat tests, 14 quoting tests and 350 related utility tests pass (2 ignored).
| always_quote: bool, | ||
| dirname: bool, | ||
| size_hint: usize, | ||
| encoding: crate::i18n::UEncoding, |
There was a problem hiding this comment.
please import UEncoding instead of the full path 3 times
| encoding: crate::i18n::UEncoding, | ||
| ) -> Self { | ||
| let (mut quotes, must_quote) = initial_quoting(reference, dirname, always_quote, true); | ||
| let invalid = match encoding { |
There was a problem hiding this comment.
this rescans every name (ls calls this per entry). could you only do it when initial_quoting picked Quotes::Double?
| } | ||
|
|
||
| #[test] | ||
| fn test_invalid_bytes_with_single_quote() { |
There was a problem hiding this comment.
could use check_names_raw_both like the other tests here, no?
Preserve filenames in stat errors without lossy UTF-8 conversion, using the existing locale-aware shell escaping. The diagnostics now use GNU-compatible octal byte escapes. Fixes #14685.
Also fix the shared shell quoter when a filename contains both a single quote and invalid bytes: dollar-quoted escapes require single-quote segments. Add regression coverage and explicitly gate the byte-based stat test to Unix.
Validation: 42 stat tests, 14 quoting tests, and 350 related utility integration tests passed (2 ignored); clippy and rustfmt passed. Thirty stderr/exit-code comparisons against GNU stat 9.11 matched across C/C.UTF-8, default/-L/-f and five filename cases. The full selected-feature test command also passes with the host NO_COLOR setting removed for the color tests.
AI assistance: Codex helped implement, test, and respond to review under the account owner's direction.