uutils / uutils/coreutils

Share the `metadata(path, follow)` leaf

Open
#14,344 0 comments 0 reactions 0 assignees View on GitHub

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-157 is a three-way choice. Between the follow and no-follow arms
    there is a Windows fast path that reads metadata from a DirEntry.
  • touch.rs:723-732 falls back to symlink_metadata when metadata fails
    with anything other than NotFound, 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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.