Conversation
When stat() failed on an entry coming from read_dir(), PathData::metadata() discarded the error: `ls -l` printed the unknown-metadata row, wrote nothing to stderr and exited 0. GNU prints a diagnostic and exits 1. Route the cached DirEntry metadata through the same error handling as the non-cached path, so the failure is reported and the exit status is set. While at it: - do not print a second diagnostic when readlink() fails for an entry whose metadata could not be read either; GNU prints only one. - stat() failures are reported as "cannot access X: Permission denied", the GNU wording, instead of "cannot open file X: Permission denied" (that message was only ever reachable from stat()/readlink() failures). Reported by Hongkai Chen of SEFCOM Lab at Arizona State University.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved metadata handling and WASI test-compatibility issues remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates ls to report directory-entry metadata failures, suppress duplicate symlink diagnostics, and use GNU-compatible wording.
Changes:
- Propagates cached metadata errors and exit status.
- Suppresses duplicate diagnostics.
- Adds regression coverage and updates translations.
File summaries
| File | Summary |
|---|---|
tests/by-util/test_ls.rs |
Adds permission-failure coverage. Moderate (1 vote): add a WASI guard for chmod-dependent testing. Nit (2 votes): assert that each diagnostic appears exactly once. |
src/uu/ls/src/ls.rs |
Handles cached metadata errors and wording. Moderate (2 votes): honor !must_dereference by using symlink metadata handling. |
src/uu/ls/src/display.rs |
Suppresses duplicate symlink diagnostics. |
src/uu/ls/locales/fr-FR.ftl |
Updates French diagnostic text. |
src/uu/ls/locales/en-US.ftl |
Updates English diagnostic text. |
Review details
Suppressed comments (1)
tests/by-util/test_ls.rs:7942
- This test changes directory permissions with
set_mode, but the#[cfg(unix)]test will also be built for the WASI runner where chmod is unavailable. Existing chmod-dependent tests add acfg_attr(wasi_runner, ignore = ...)(for example,tests/by-util/test_cp.rs:9110-9113); add the same guard here.
#[cfg(unix)]
#[test]
fn test_ls_long_stat_failure_is_reported() {
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| { | ||
| return dir_entry.metadata().ok(); | ||
| } | ||
| dir_entry.metadata() |
Comment on lines
+7963
to
+7964
| .stderr_contains("cannot access 'dir/file': Permission denied") | ||
| .stderr_contains("cannot access 'dir/link': Permission denied"); |
|
GNU testsuite comparison: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When stat() failed on an entry coming from read_dir(), PathData::metadata() discarded the error:
ls -lprinted the unknown-metadata row, wrote nothing to stderr and exited 0. GNU prints a diagnostic and exits 1.Route the cached DirEntry metadata through the same error handling as the non-cached path, so the failure is reported and the exit status is set.
While at it:
Reported by Hongkai Chen of SEFCOM Lab at Arizona State University.