D7058: rust-dirstate-status: add first Rust implementation of `dirstate.status`

Yuya Nishihara yuya at tcha.org
Mon Oct 14 11:11:03 EDT 2019


Just quickly scanned. Not reviewed the core logic.

> +/// Get name in the case stored in the filesystem
> +/// The name should be relative to root, and be normcase-ed for efficiency.
> +///
> +/// Note that this function is unnecessary, and should not be
> +//  called, for case-sensitive filesystems (simply because it's expensive).
> +/// The root should be normcase-ed, too.
> +pub fn filesystem_path<P: AsRef<HgPath>>(root: &HgPath, path: P) -> HgPathBuf {

Unused?

I think `path: impl AsRef<HgPath>` syntax is easier to follow unless we need
a type variable.

> +    // TODO path for case-insensitive filesystems, for now this is transparent
> +    root.to_owned().join(path.as_ref())
> +}

`filesystems_path()` sounds like returning a `std::path::PathBuf`, but
`HgPath` should be a repo-relative path.

> +/// Returns `true` if path refers to an existing path.
> +/// Returns `true` for broken symbolic links.
> +/// Equivalent to `exists()` on platforms lacking `lstat`.
> +pub fn lexists<P: AsRef<Path>>(path: P) -> bool {

Unused?

> +    if !path.as_ref().exists() {
> +        return read_link(path).is_ok();
> +    }
> +    true
> +}

Maybe you want `fs::symlink_metadata()`?
I don't know which is faster, but the behavior should be closer to `lstat`.

https://doc.rust-lang.org/std/fs/fn.symlink_metadata.html

> +impl HgMetadata {
> +    #[cfg(unix)]
> +    pub fn from_metadata(metadata: Metadata) -> Self {
> +        use std::os::linux::fs::MetadataExt;

`unix != linux`. Maybe you want `std::os::unix::fs::MetadataExt`.

> +pub fn status<P: AsRef<Path>>(
> +    mut dmap: &mut DirstateMap,
> +    root_dir: P,
> +    files: Vec<HgPathBuf>,
> +    list_clean: bool,
> +    last_normal_time: i64,
> +    check_exec: bool,
> +) -> std::io::Result<(Vec<HgPathBuf>, StatusResult)>
> +where
> +    P: Sync,

`P: AsRef<path> + Sync`.


More information about the Mercurial-devel mailing list