chmod: report the real error when a target's metadata is inaccessible - #13637
chmod: report the real error when a target's metadata is inaccessible#136370xfandom wants to merge 2 commits into
Conversation
|
GNU testsuite comparison: |
|
a few jobs are failing |
chmod used Path::exists() to decide whether a target exists, but exists() returns false on any metadata error, so a file whose parent directory lacks search permission was reported as "No such file or directory" instead of "Permission denied". Use try_exists() to distinguish a genuine ENOENT from a permission error, matching GNU. Fixes uutils#9789.
44c1f3f to
7308279
Compare
Merging this PR will improve performance by 22.62%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | df_with_path |
700.8 µs | 571.5 µs | +22.62% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing 0xfandom:chmod-inaccessible-permission-denied (6dadcce) with main (a730551)
Footnotes
-
46 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. ↩
Satisfy clippy::unreadable_literal (0o100644 -> 0o100_644) so the Style and Lint job passes.
|
Thanks for the heads up. The failing job was clippy's unreadable_literal on the new test — I'd written the octal mode as 0o100644. Pushed a fixup that changes it to 0o100_644, and clippy is clean locally now. The CodSpeed report flags a ~3.5% change on du_wide_tree, but that's an unrelated du benchmark this PR doesn't touch, and the report itself notes it compared across different runtime environments. Should be noise. Let me know if you'd like anything else adjusted. |
Fixes #9789.
chmodusedPath::exists()to decide whether a target exists.Path::exists()returnsfalsefor any metadata access error, including permission errors, so a file whose parent directory lacks search permission was reported as:instead of GNU's:
This switches the non-recursive path to
Path::try_exists(), which returnsOk(false)only for a genuineENOENTandErr(..)for a permission error. A permission error is now reported via the existingChmodError::PermissionDeniedvariant, whose message already matches GNU. Any other unexpected metadata error falls through to the normal chmod attempt so it can surface a precise message.Added a regression test (
test_chmod_inaccessible_file_reports_permission_denied) for the non-recursive case; it fails on the oldPath::exists()code (prints "No such file or directory") and passes with the fix. The existing permission-denied tests only covered the recursive (-R) path.