Patchwork D7503: rust-dirs: address failing tests for `dirs` impl with a temporary fix

login
register
mail settings
Submitter phabricator
Date Nov. 22, 2019, 9:47 a.m.
Message ID <differential-rev-PHID-DREV-4zkdmvok4vro5zejnvyr-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/43438/
State Superseded
Headers show

Comments

phabricator - Nov. 22, 2019, 9:47 a.m.
Alphare created this revision.
Herald added subscribers: mercurial-devel, kevincox, durin42.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  https://phab.mercurial-scm.org/D7252 (5d40317d42b7083b49467502549e25f144888cb3 <https://phab.mercurial-scm.org/rHG5d40317d42b7083b49467502549e25f144888cb3>)
  introduced a regression in Rust tests.
  
  This is a temporary fix that replicates the behavior of the C and Python impl,
  pending the resolution of the discussion (in the phabricator link) about how
  we actually want to solve this problem.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

AFFECTED FILES
  rust/hg-core/src/dirstate/dirs_multiset.rs
  rust/hg-core/src/dirstate/dirstate_map.rs
  rust/hg-core/src/lib.rs
  rust/hg-cpython/src/dirstate/dirs_multiset.rs
  rust/hg-cpython/src/dirstate/dirstate_map.rs

CHANGE DETAILS




To: Alphare, #hg-reviewers
Cc: durin42, kevincox, mercurial-devel
phabricator - Nov. 22, 2019, 5:09 p.m.
marmoute added inline comments.

INLINE COMMENTS

> lib.rs:117
> +        }
> +    }
>  }

Should we also have the path included in that error ?

REPOSITORY
  rHG Mercurial

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

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

To: Alphare, #hg-reviewers
Cc: marmoute, durin42, kevincox, mercurial-devel
phabricator - Nov. 22, 2019, 5:46 p.m.
Alphare added inline comments.

INLINE COMMENTS

> marmoute wrote in lib.rs:117
> Should we also have the path included in that error ?

This patch is a minimal effort to stick to the C and Python implementations until a decision is reached. Should we decide to keep this code, I would be for having the path included in the error.

REPOSITORY
  rHG Mercurial

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

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

To: Alphare, #hg-reviewers
Cc: marmoute, durin42, kevincox, mercurial-devel
phabricator - Nov. 22, 2019, 5:50 p.m.
marmoute added inline comments.

INLINE COMMENTS

> Alphare wrote in lib.rs:117
> This patch is a minimal effort to stick to the C and Python implementations until a decision is reached. Should we decide to keep this code, I would be for having the path included in the error.

This seems fine.

REPOSITORY
  rHG Mercurial

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

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

To: Alphare, #hg-reviewers
Cc: marmoute, durin42, kevincox, mercurial-devel
Yuya Nishihara - Nov. 23, 2019, 6:11 a.m.
Many unhandled results:

```
warning: unused `std::result::Result` that must be used
  --> hg-core/src/dirstate/dirs_multiset.rs:42:21
   |
42 |                     multiset.add_path(filename);
   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: #[warn(unused_must_use)] on by default
   = note: this `Result` may be an `Err` variant, which should be handled

warning: unused `std::result::Result` that must be used
  --> hg-core/src/dirstate/dirs_multiset.rs:45:17
   |
45 |                 multiset.add_path(filename);
   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: this `Result` may be an `Err` variant, which should be handled

warning: unused `std::result::Result` that must be used
  --> hg-core/src/dirstate/dirs_multiset.rs:59:13
   |
59 |             multiset.add_path(filename);
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: this `Result` may be an `Err` variant, which should be handled

warning: unused `std::result::Result` that must be used
   --> hg-core/src/dirstate/dirstate_map.rs:129:17
    |
129 |                 all_dirs.add_path(filename);
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: this `Result` may be an `Err` variant, which should be handled
```

Might be better to do `path.check_state()` in cpython layer, and insert
`debug_assert` to hg-core.
phabricator - Nov. 23, 2019, 6:13 a.m.
yuja added a comment.


  Many unhandled results:
  
    warning: unused `std::result::Result` that must be used
      --> hg-core/src/dirstate/dirs_multiset.rs:42:21
       |
    42 |                     multiset.add_path(filename);
       |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
       |
       = note: #[warn(unused_must_use)] on by default
       = note: this `Result` may be an `Err` variant, which should be handled
    
    warning: unused `std::result::Result` that must be used
      --> hg-core/src/dirstate/dirs_multiset.rs:45:17
       |
    45 |                 multiset.add_path(filename);
       |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
       |
       = note: this `Result` may be an `Err` variant, which should be handled
    
    warning: unused `std::result::Result` that must be used
      --> hg-core/src/dirstate/dirs_multiset.rs:59:13
       |
    59 |             multiset.add_path(filename);
       |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
       |
       = note: this `Result` may be an `Err` variant, which should be handled
    
    warning: unused `std::result::Result` that must be used
       --> hg-core/src/dirstate/dirstate_map.rs:129:17
        |
    129 |                 all_dirs.add_path(filename);
        |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        |
        = note: this `Result` may be an `Err` variant, which should be handled
  
  Might be better to do `path.check_state()` in cpython layer, and insert
  `debug_assert` to hg-core.

REPOSITORY
  rHG Mercurial

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

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

To: Alphare, #hg-reviewers
Cc: yuja, marmoute, durin42, kevincox, mercurial-devel
phabricator - Nov. 25, 2019, 10:25 a.m.
Alphare added a comment.


  In D7503#110345 <https://phab.mercurial-scm.org/D7503#110345>, @yuja wrote:
  
  > Many unhandled results:
  
  Ah sorry, the cargo compilation cache hid those warnings from me, I usually catch them.
  
  > Might be better to do `path.check_state()` in cpython layer, and insert
  > `debug_assert` to hg-core.
  
  That would be cleaner for the current purposes, but using `debug_assert` in `hg-core` indicates to me that we want the Rust code to not worry about checking for consecutive slashes in `dirs`, because we would have the `pathauditor`. Am I correct?

REPOSITORY
  rHG Mercurial

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

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

To: Alphare, #hg-reviewers
Cc: yuja, marmoute, durin42, kevincox, mercurial-devel
Yuya Nishihara - Nov. 25, 2019, 1:46 p.m.
>   > Might be better to do `path.check_state()` in cpython layer, and insert
>   > `debug_assert` to hg-core.
>   
>   That would be cleaner for the current purposes, but using `debug_assert` in `hg-core` indicates to me that we want the Rust code to not worry about checking for consecutive slashes in `dirs`, because we would have the `pathauditor`. Am I correct?

I generally prefer adding safety checks at ABI boundary. If malicious input
makes Rust code crash or exhaust CPU/memory resource, I would add sanity
check to rust-cpython layer.
phabricator - Nov. 25, 2019, 1:47 p.m.
yuja added a comment.


  >   > Might be better to do `path.check_state()` in cpython layer, and insert
  >   > `debug_assert` to hg-core.
  >   That would be cleaner for the current purposes, but using `debug_assert` in `hg-core` indicates to me that we want the Rust code to not worry about checking for consecutive slashes in `dirs`, because we would have the `pathauditor`. Am I correct?
  
  I generally prefer adding safety checks at ABI boundary. If malicious input
  makes Rust code crash or exhaust CPU/memory resource, I would add sanity
  check to rust-cpython layer.

REPOSITORY
  rHG Mercurial

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

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

To: Alphare, #hg-reviewers
Cc: yuja, marmoute, durin42, kevincox, mercurial-devel
phabricator - Nov. 27, 2019, 11:32 a.m.
Alphare added a comment.


  > I generally prefer adding safety checks at ABI boundary. If malicious input
  > makes Rust code crash or exhaust CPU/memory resource, I would add sanity
  > check to rust-cpython layer.
  
  Sure, that makes sense in our configuration, but we need to consider `hg-core` as its own standalone library when making decisions like this. Either the `Dirs` / `dirs` API has changed to this new behavior (which I'm not super happy about), either we revert the changes proposed by Augie with a change in the fuzzer instead.
  
  Sorry for the warnings, I'll send a follow-up, I've been caught up in another project.

REPOSITORY
  rHG Mercurial

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

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

To: Alphare, #hg-reviewers
Cc: yuja, marmoute, durin42, kevincox, mercurial-devel
Yuya Nishihara - Nov. 27, 2019, 1:29 p.m.
>   > I generally prefer adding safety checks at ABI boundary. If malicious input
>   > makes Rust code crash or exhaust CPU/memory resource, I would add sanity
>   > check to rust-cpython layer.
>   
>   Sure, that makes sense in our configuration, but we need to consider `hg-core` as its own standalone library when making decisions like this.

Okay, then using non-debug `assert!()` seems more appropriate. If we prefer
being stricter, "checked" HgPath type can be introduced.

> Either the `Dirs` / `dirs` API has changed to this new behavior (which I'm not super happy about), either we revert the changes proposed by Augie with a change in the fuzzer instead.
>
>   
>   Sorry for the warnings, I'll send a follow-up, I've been caught up in another project.

Actually I tried to suppress these warnings by propagating Result upwards,
and I got a feeling that we're doing wrong.
phabricator - Nov. 27, 2019, 1:30 p.m.
yuja added a comment.


  >   > I generally prefer adding safety checks at ABI boundary. If malicious input
  >   > makes Rust code crash or exhaust CPU/memory resource, I would add sanity
  >   > check to rust-cpython layer.
  >   Sure, that makes sense in our configuration, but we need to consider `hg-core` as its own standalone library when making decisions like this.
  
  Okay, then using non-debug `assert!()` seems more appropriate. If we prefer
  being stricter, "checked" HgPath type can be introduced.
  
  > Either the `Dirs` / `dirs` API has changed to this new behavior (which I'm not super happy about), either we revert the changes proposed by Augie with a change in the fuzzer instead.
  >
  >   Sorry for the warnings, I'll send a follow-up, I've been caught up in another project.
  
  Actually I tried to suppress these warnings by propagating Result upwards,
  and I got a feeling that we're doing wrong.

REPOSITORY
  rHG Mercurial

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

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

To: Alphare, #hg-reviewers
Cc: yuja, marmoute, durin42, kevincox, mercurial-devel
phabricator - Nov. 27, 2019, 1:35 p.m.
Alphare added a comment.


  > Okay, then using non-debug `assert!()` seems more appropriate. If we prefer
  > being stricter, "checked" HgPath type can be introduced.
  > ...
  > Actually I tried to suppress these warnings by propagating Result upwards,
  > and I got a feeling that we're doing wrong.
  
  I sent a followup as D7522 <https://phab.mercurial-scm.org/D7522> because that gets rid of the warnings. As explained in the commit message, I am unhappy about this change, as you seem to be.
  
  We're doing it wrong, indeed, because the checks are happening at the wrong level, causing abstraction leakage all over the place. The Rust type-system makes it much more obvious than in C or Python.
  
  I am of the opinion of backing out of the original patch (5d40317d42b7083b49467502549e25f144888cb3 <https://phab.mercurial-scm.org/rHG5d40317d42b7083b49467502549e25f144888cb3>) that @durin42 introduced.

REPOSITORY
  rHG Mercurial

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

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

To: Alphare, #hg-reviewers
Cc: yuja, marmoute, durin42, kevincox, mercurial-devel
Yuya Nishihara - Nov. 28, 2019, 1:12 p.m.
>   > Okay, then using non-debug `assert!()` seems more appropriate. If we prefer
>   > being stricter, "checked" HgPath type can be introduced.
>   > ...
>   > Actually I tried to suppress these warnings by propagating Result upwards,
>   > and I got a feeling that we're doing wrong.
>   
>   I sent a followup as D7522 <https://phab.mercurial-scm.org/D7522> because that gets rid of the warnings. As explained in the commit message, I am unhappy about this change, as you seem to be.

Thanks. Given Rust impl is still unstable, I think it's better to back out
the changes in Rust and leave the test failure until we find a right solution.

>   We're doing it wrong, indeed, because the checks are happening at the wrong level, causing abstraction leakage all over the place. The Rust type-system makes it much more obvious than in C or Python.
>
>   I am of the opinion of backing out of the original patch (5d40317d42b7083b49467502549e25f144888cb3 <https://phab.mercurial-scm.org/rHG5d40317d42b7083b49467502549e25f144888cb3>) that @durin42 introduced.

I don't agree with that since raising Python exception is the only way to
safely error out from C extension layer. It should be better than blocking
the entire process (or exhausting resources.)
phabricator - Nov. 28, 2019, 1:14 p.m.
yuja added a comment.


  >   > Okay, then using non-debug `assert!()` seems more appropriate. If we prefer
  >   > being stricter, "checked" HgPath type can be introduced.
  >   > ...
  >   > Actually I tried to suppress these warnings by propagating Result upwards,
  >   > and I got a feeling that we're doing wrong.
  >   I sent a followup as D7522 <https://phab.mercurial-scm.org/D7522> because that gets rid of the warnings. As explained in the commit message, I am unhappy about this change, as you seem to be.
  
  Thanks. Given Rust impl is still unstable, I think it's better to back out
  the changes in Rust and leave the test failure until we find a right solution.
  
  >   We're doing it wrong, indeed, because the checks are happening at the wrong level, causing abstraction leakage all over the place. The Rust type-system makes it much more obvious than in C or Python.
  >   I am of the opinion of backing out of the original patch (5d40317d42b7083b49467502549e25f144888cb3 <https://phab.mercurial-scm.org/rHG5d40317d42b7083b49467502549e25f144888cb3>) that @durin42 introduced.
  
  I don't agree with that since raising Python exception is the only way to
  safely error out from C extension layer. It should be better than blocking
  the entire process (or exhausting resources.)

REPOSITORY
  rHG Mercurial

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

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

To: Alphare, #hg-reviewers
Cc: yuja, marmoute, durin42, kevincox, mercurial-devel
phabricator - Nov. 29, 2019, 11 a.m.
marmoute added a comment.


  In D7503#110631 <https://phab.mercurial-scm.org/D7503#110631>, @yuja wrote:
  
  >>   > Okay, then using non-debug `assert!()` seems more appropriate. If we prefer
  >>   > being stricter, "checked" HgPath type can be introduced.
  >>   > ...
  >>   > Actually I tried to suppress these warnings by propagating Result upwards,
  >>   > and I got a feeling that we're doing wrong.
  >>   I sent a followup as D7522 <https://phab.mercurial-scm.org/D7522> because that gets rid of the warnings. As explained in the commit message, I am unhappy about this change, as you seem to be.
  >
  > Thanks. Given Rust impl is still unstable, I think it's better to back out
  > the changes in Rust and leave the test failure until we find a right solution.
  
  I am -1 for this. Having the test broken is a big overhead for people actually using Rust and testing it. The rust support is experimental but not "unstable". We use it in production in a couple of places. Keeping the test suite

REPOSITORY
  rHG Mercurial

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

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

To: Alphare, #hg-reviewers
Cc: yuja, marmoute, durin42, kevincox, mercurial-devel
Yuya Nishihara - Nov. 29, 2019, 2:55 p.m.
>   >>   > Okay, then using non-debug `assert!()` seems more appropriate. If we prefer
>   >>   > being stricter, "checked" HgPath type can be introduced.
>   >>   > ...
>   >>   > Actually I tried to suppress these warnings by propagating Result upwards,
>   >>   > and I got a feeling that we're doing wrong.
>   >>   I sent a followup as D7522 <https://phab.mercurial-scm.org/D7522> because that gets rid of the warnings. As explained in the commit message, I am unhappy about this change, as you seem to be.
>   >
>   > Thanks. Given Rust impl is still unstable, I think it's better to back out
>   > the changes in Rust and leave the test failure until we find a right solution.
>   
>   I am -1 for this. Having the test broken is a big overhead for people actually using Rust and testing it. The rust support is experimental but not "unstable". We use it in production in a couple of places. Keeping the test suite

Well, IMHO, it's as unstable as Python 3. Core features should work, but I
think it's okay to opt out some "known to be broken" tests so long as the
test failure makes sense.

I felt the changes were too much to make Rust implementation behaves exactly
in the same way as C one. If not, I would send the fix last weekend instead
of getting around the phabricator email.
phabricator - Nov. 29, 2019, 2:57 p.m.
yuja added a comment.


  >   >>   > Okay, then using non-debug `assert!()` seems more appropriate. If we prefer
  >   >>   > being stricter, "checked" HgPath type can be introduced.
  >   >>   > ...
  >   >>   > Actually I tried to suppress these warnings by propagating Result upwards,
  >   >>   > and I got a feeling that we're doing wrong.
  >   >>   I sent a followup as D7522 <https://phab.mercurial-scm.org/D7522> because that gets rid of the warnings. As explained in the commit message, I am unhappy about this change, as you seem to be.
  >   >
  >   > Thanks. Given Rust impl is still unstable, I think it's better to back out
  >   > the changes in Rust and leave the test failure until we find a right solution.
  >   I am -1 for this. Having the test broken is a big overhead for people actually using Rust and testing it. The rust support is experimental but not "unstable". We use it in production in a couple of places. Keeping the test suite
  
  Well, IMHO, it's as unstable as Python 3. Core features should work, but I
  think it's okay to opt out some "known to be broken" tests so long as the
  test failure makes sense.
  
  I felt the changes were too much to make Rust implementation behaves exactly
  in the same way as C one. If not, I would send the fix last weekend instead
  of getting around the phabricator email.

REPOSITORY
  rHG Mercurial

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

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

To: Alphare, #hg-reviewers
Cc: yuja, marmoute, durin42, kevincox, mercurial-devel
phabricator - Nov. 29, 2019, 3:01 p.m.
marmoute added a comment.


  I am fine with not having all test passing from the start. However here we have a test that used to pass that is now failing, so we regressed here. I would rather not regress in test coverage here. Without this, all mercurial test pass with the Rust code, I would like to keep it that way.

REPOSITORY
  rHG Mercurial

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

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

To: Alphare, #hg-reviewers
Cc: yuja, marmoute, durin42, kevincox, mercurial-devel
Yuya Nishihara - Nov. 29, 2019, 3:14 p.m.
>   I am fine with not having all test passing from the start. However here we have a test that used to pass that is now failing, so we regressed here. I would rather not regress in test coverage here. Without this, all mercurial test pass with the Rust code, I would like to keep it that way.

Isn't it a new test added at 5d40317d42b7, right?

As I said, I'm not against the change at 5d40317d42b7, but it seems not
fit well into Rust.
phabricator - Nov. 29, 2019, 3:15 p.m.
yuja added a comment.


  >   I am fine with not having all test passing from the start. However here we have a test that used to pass that is now failing, so we regressed here. I would rather not regress in test coverage here. Without this, all mercurial test pass with the Rust code, I would like to keep it that way.
  
  Isn't it a new test added at 5d40317d42b7 <https://phab.mercurial-scm.org/rHG5d40317d42b7083b49467502549e25f144888cb3>, right?
  
  As I said, I'm not against the change at 5d40317d42b7 <https://phab.mercurial-scm.org/rHG5d40317d42b7083b49467502549e25f144888cb3>, but it seems not
  fit well into Rust.

REPOSITORY
  rHG Mercurial

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

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

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

Patch

diff --git a/rust/hg-cpython/src/dirstate/dirstate_map.rs b/rust/hg-cpython/src/dirstate/dirstate_map.rs
--- a/rust/hg-cpython/src/dirstate/dirstate_map.rs
+++ b/rust/hg-cpython/src/dirstate/dirstate_map.rs
@@ -25,8 +25,8 @@ 
 use hg::{
     utils::hg_path::{HgPath, HgPathBuf},
     DirsMultiset, DirstateEntry, DirstateMap as RustDirstateMap,
-    DirstateParents, DirstateParseError, EntryState, StateMapIter,
-    PARENT_SIZE,
+    DirstateMapError, DirstateParents, DirstateParseError, EntryState,
+    StateMapIter, PARENT_SIZE,
 };
 
 // TODO
@@ -97,8 +97,9 @@ 
                 size: size.extract(py)?,
                 mtime: mtime.extract(py)?,
             },
-        );
-        Ok(py.None())
+        ).and(Ok(py.None())).or_else(|e: DirstateMapError| {
+            Err(PyErr::new::<exc::ValueError, _>(py, e.to_string()))
+        })
     }
 
     def removefile(
diff --git a/rust/hg-cpython/src/dirstate/dirs_multiset.rs b/rust/hg-cpython/src/dirstate/dirs_multiset.rs
--- a/rust/hg-cpython/src/dirstate/dirs_multiset.rs
+++ b/rust/hg-cpython/src/dirstate/dirs_multiset.rs
@@ -68,8 +68,19 @@ 
     def addpath(&self, path: PyObject) -> PyResult<PyObject> {
         self.inner_shared(py).borrow_mut()?.add_path(
             HgPath::new(path.extract::<PyBytes>(py)?.data(py)),
-        );
-        Ok(py.None())
+        ).and(Ok(py.None())).or_else(|e| {
+            match e {
+                DirstateMapError::EmptyPath => {
+                    Ok(py.None())
+                },
+                e => {
+                    Err(PyErr::new::<exc::ValueError, _>(
+                        py,
+                        e.to_string(),
+                    ))
+                }
+            }
+        })
     }
 
     def delpath(&self, path: PyObject) -> PyResult<PyObject> {
@@ -79,15 +90,15 @@ 
             .and(Ok(py.None()))
             .or_else(|e| {
                 match e {
-                    DirstateMapError::PathNotFound(_p) => {
+                    DirstateMapError::EmptyPath => {
+                        Ok(py.None())
+                    },
+                    e => {
                         Err(PyErr::new::<exc::ValueError, _>(
                             py,
-                            "expected a value, found none".to_string(),
+                            e.to_string(),
                         ))
                     }
-                    DirstateMapError::EmptyPath => {
-                        Ok(py.None())
-                    }
                 }
             })
     }
diff --git a/rust/hg-core/src/lib.rs b/rust/hg-core/src/lib.rs
--- a/rust/hg-core/src/lib.rs
+++ b/rust/hg-core/src/lib.rs
@@ -101,6 +101,20 @@ 
 pub enum DirstateMapError {
     PathNotFound(HgPathBuf),
     EmptyPath,
+    ConsecutiveSlashes,
+}
+
+impl ToString for DirstateMapError {
+    fn to_string(&self) -> String {
+        use crate::DirstateMapError::*;
+        match self {
+            PathNotFound(_) => "expected a value, found none".to_string(),
+            EmptyPath => "Overflow in dirstate.".to_string(),
+            ConsecutiveSlashes => {
+                "found invalid consecutive slashes in path".to_string()
+            }
+        }
+    }
 }
 
 pub enum DirstateError {
diff --git a/rust/hg-core/src/dirstate/dirstate_map.rs b/rust/hg-core/src/dirstate/dirstate_map.rs
--- a/rust/hg-core/src/dirstate/dirstate_map.rs
+++ b/rust/hg-core/src/dirstate/dirstate_map.rs
@@ -83,16 +83,16 @@ 
         filename: &HgPath,
         old_state: EntryState,
         entry: DirstateEntry,
-    ) {
+    ) -> Result<(), DirstateMapError> {
         if old_state == EntryState::Unknown || old_state == EntryState::Removed
         {
             if let Some(ref mut dirs) = self.dirs {
-                dirs.add_path(filename)
+                dirs.add_path(filename)?;
             }
         }
         if old_state == EntryState::Unknown {
             if let Some(ref mut all_dirs) = self.all_dirs {
-                all_dirs.add_path(filename)
+                all_dirs.add_path(filename)?;
             }
         }
         self.state_map.insert(filename.to_owned(), entry.to_owned());
@@ -104,6 +104,7 @@ 
         if entry.size == SIZE_FROM_OTHER_PARENT {
             self.other_parent_set.insert(filename.to_owned());
         }
+        Ok(())
     }
 
     /// Mark a file as removed in the dirstate.
diff --git a/rust/hg-core/src/dirstate/dirs_multiset.rs b/rust/hg-core/src/dirstate/dirs_multiset.rs
--- a/rust/hg-core/src/dirstate/dirs_multiset.rs
+++ b/rust/hg-core/src/dirstate/dirs_multiset.rs
@@ -65,14 +65,20 @@ 
     /// Increases the count of deepest directory contained in the path.
     ///
     /// If the directory is not yet in the map, adds its parents.
-    pub fn add_path(&mut self, path: &HgPath) {
+    pub fn add_path(&mut self, path: &HgPath) -> Result<(), DirstateMapError> {
         for subpath in files::find_dirs(path) {
+            if subpath.as_bytes().last() == Some(&b'/') {
+                // TODO Remove this once PathAuditor is certified
+                // as the only entrypoint for path data
+                return Err(DirstateMapError::ConsecutiveSlashes);
+            }
             if let Some(val) = self.inner.get_mut(subpath) {
                 *val += 1;
                 break;
             }
             self.inner.insert(subpath.to_owned(), 1);
         }
+        Ok(())
     }
 
     /// Decreases the count of deepest directory contained in the path.