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

[Alphare (Raphaël Gomès)]

Alphare added a comment.


  In D7058#103954 <https://phab.mercurial-scm.org/D7058#103954>, @yuja wrote:
  
  > 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?
  
  Indeed, it was for a future patch, removed.
  
  > I think `path: impl AsRef<HgPath>` syntax is easier to follow unless we need
  > a type variable.
  
  I changed to `impl Trait` in all relevant places in this patch.
  
  >> +    // 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?
  
  Same as above, removed.
  
  >> +    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`.
  
  Thanks. I also caught a typo in the name of the non-unix impl. It's now split it in two impls.
  
  >> +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`.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7058/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7058

To: Alphare, #hg-reviewers
Cc: yuja, martinvonz, durin42, kevincox, mercurial-devel