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.

INLINE COMMENTS

> 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

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