Share the `metadata(path, follow)` leaf
Nobody has claimed this yet.
- Dominant language
- Rust
- Stars
- 24.1k
- Forks
- 2k
- Avg merge
- 1d 5h
- Merged PRs (30d)
- 365
Description
The duplication
uucore::perms::get_metadata (perms.rs:279) and ls's private
get_metadata_with_deref_opt (ls.rs:1559) are the same function, character
for character:
if follow { path.metadata() } else { path.symlink_metadata() }
Seven more inline copies of that body exist at test.rs:405, test.rs:443,
touch.rs:479-482, touch.rs:723-732, cp.rs:2698-2700, du.rs:150-157 and
stat.rs:1377.
The change
Move the function to uucore::fs, which 43 crates already depend on.
perms is unix-only and used by 8, so it is the narrower home. Keep
perms::get_metadata as a re-export so chown, chgrp and chmod do not
change.
// src/uucore/src/lib/features/fs.rs
/// Metadata for `path`, following a final symlink only when `follow` is true.
///
/// `follow: true` is `Path::metadata` (stat), which describes the file a
/// symlink points at. `follow: false` is `Path::symlink_metadata` (lstat),
/// which describes the symlink itself.
pub fn get_metadata(path: impl AsRef<Path>, follow: bool) -> IOResult<fs::Metadata> {
let path = path.as_ref();
if follow { path.metadata() } else { path.symlink_metadata() }
}
// src/uucore/src/lib/features/perms.rs
pub use crate::features::fs::get_metadata;
Value
This removes duplication. It fixes no bug. The benefit is that the meaning of
follow is written down once instead of eight times. Eight separate copies is
eight chances to get the direction backwards.
What is found further
Seven of the nine sites converted, plus a tenth the census missed in
test/platform/wasi.rs. Two sites are not this function:
du.rs:150-157is a three-way choice. Between the follow and no-follow arms
there is a Windows fast path that reads metadata from aDirEntry.touch.rs:723-732falls back tosymlink_metadatawhenmetadatafails
with anything other thanNotFound, so a link that is broken in some other
way still yields times.
The perms feature now declares fs in Cargo.toml. That is a correction:
perms.rs already imported FileInformation from fs, so the dependency
existed and was undeclared.
What to leave alone
The dereference option enums. du::Deref needs Args(Vec<PathBuf>) for -D.
ls::Dereference needs DirArgs. perms::TraverseSymlinks keeps recursion
semantics separate from operand semantics. A single enum covering all of them
would have six variants where each utility uses three, and every match would
need arms that cannot happen. Each utility keeps its own enum and reduces it to
a bool at the call site, which du.rs:141-147 and cp.rs:1289 already do.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with src/uucore/src/lib/features/fs.rs and perms.rs, then inspect the listed metadata copies in ls.rs, test.rs, touch.rs, cp.rs, du.rs, stat.rs, and test/platform/wasi.rs. Run the relevant existing tests in test.rs and test/platform/wasi.rs; done means the applicable sites use the shared helper, perms remains a re-export, and the du.rs and touch.rs special cases remain unchanged.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- cli
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100