Skip to content

Share the metadata(path, follow) leaf #14344

Description

@meghuizen

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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions