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

kevincox (Kevin Cox) phabricator at mercurial-scm.org
Tue Oct 15 09:12:06 EDT 2019


This revision now requires changes to proceed.
kevincox added inline comments.
kevincox requested changes to this revision.

INLINE COMMENTS

> Alphare wrote in status.rs:83
> Collecting into a `Result` prevents us from doing a `filter_map` during `collect`. Implementing a `ParallelIterator` adapter that would do a `filter_map` with `Result` handling is possible, but would require more work.
> 
> In this specific case, as I eluded to above, I just removed the sentinel value so the question is not relevant anymore.

I don't follow. can't this `.filter_map(...)` be replaced with a `.filter(...).map(...)`? I think this would make the code much more clear.

> status.rs:23
> +fn walk_explicit(
> +    files: Vec<HgPathBuf>,
> +    dmap: &mut DirstateMap,

This doesn't appear to be consumed by the function so you should take a `&[HgPathBuf]`. Or since I don't think you use the path either probably a `&[HgPath]`.

> status.rs:134
> +    results: HashMap<HgPathBuf, Option<HgMetadata>>,
> +) -> ((Vec<HgPathBuf>, StatusResult)) {
> +    let mut lookup = vec![];

Why are there two pairs of parens?

> status.rs:168
> +                _ => {}
> +            },
> +            Some(HgMetadata {

This seems like a very complicated way to write this. Why not separate the `match state` and do it first. Or put it in an `else` block? Furthermore why do you have two separate `match state`? Why not handle them all in one `match`?

> status.rs:240
> +    let mut results =
> +        walk_explicit(files, &mut dmap, root_dir.as_ref().to_owned())?;
> +

Do you need to do this to `root_dir`? The `walk_explicit` function seems to only need `AsRef<Path>` so you should be able to just pass it. You might need to update the argument to be a reference.

> files.rs:112
> +        // TODO support other platforms
> +        unimplemented!()
> +    }

Isn't it better to avoid implementing this so that we get a compile error rather than a runtime error?

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, kevincox
Cc: yuja, martinvonz, durin42, kevincox, mercurial-devel


More information about the Mercurial-devel mailing list