D7058: rust-dirstate-status: add first Rust implementation of `dirstate.status`
Alphare (Raphaël Gomès)
phabricator at mercurial-scm.org
Tue Oct 15 10:43:59 EDT 2019
Alphare added inline comments.
Alphare marked an inline comment as done.
> kevincox wrote in status.rs:23
> 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]`.
Done with `&[impl AsRef<HgPath> + Sync]` which is more generic.
> kevincox wrote in status.rs:168
> 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`?
I could indeed probably simplify it. I'll update this part later today (Paris time) .
> kevincox wrote in status.rs:240
> 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.
I had to add a `Copy` trait bound for it to work, otherwise it would be moved by `walk_explicit` before usage in `stat_dmap_entries`
> kevincox wrote in files.rs:112
> Isn't it better to avoid implementing this so that we get a compile error rather than a runtime error?
It is indeed a much better idea. I'll also send a patch for the other uses of `unimplemented!()` in the rest of the codebase
CHANGES SINCE LAST ACTION
To: Alphare, #hg-reviewers, kevincox
Cc: yuja, martinvonz, durin42, kevincox, mercurial-devel
More information about the Mercurial-devel