D6393: rust-dirstate: add "dirs" Rust implementation

kevincox (Kevin Cox) phabricator at mercurial-scm.org
Mon Jun 24 09:46:51 EDT 2019


kevincox added a comment.
kevincox accepted this revision.


  Sorry. I was busy. In general don't worry about blocking on me, I can't promise any sort of reasonable response time. Worst case I can review the changes after submission and changes can be made afterwards.

INLINE COMMENTS

> dirs_multiset.rs:43
> +                    if let Some(skip) = skip_state {
> +                        if skip != state {
> +                            multiset.add_path(filename);

You can replate the nested if with:

  if skip_state == Some(state) {

> dirs_multiset.rs:63
> +    /// without trailing slash, from right to left.
> +    fn find_dir(path: &[u8], mut pos: usize) -> Option<&[u8]> {
> +        loop {

I would remove the `pos` argument. IIUC following two are currently identical.

  find_dir(path, n);
  find_dir(path[..n], n);

If you remove the second argument then you can just always remove the last component of the path. This will also allow you to have a more clear doc comment. Right now I find it a little confusing.

REPOSITORY
  rHG Mercurial

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

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

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


More information about the Mercurial-devel mailing list