This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7503

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

Authored by Alphare on Nov 22 2019, 4:46 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG1fe2e574616e: rust-dirs: address failing tests for `dirs` impl with a temporary fix
rHG20a3bf5e71d6: rust-dirs: address failing tests for `dirs` impl with a temporary fix
Summary

https://phab.mercurial-scm.org/D7252 (5d40317d42b7083b49467502549e25f144888cb3)
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.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Alphare created this revision.Nov 22 2019, 4:46 AM
marmoute added a subscriber: marmoute.Nov 22 2019, 12:09 PM
marmoute added inline comments.
rust/hg-core/src/lib.rs
117

Should we also have the path included in that error ?

Alphare added a commit: rHG20a3bf5e71d6: rust-dirs: address failing tests for `dirs` impl with a temporary fix.Nov 22 2019, 12:44 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG20a3bf5e71d6: rust-dirs: address failing tests for `dirs` impl with a temporary fix (authored by Alphare). · Explain Why
This revision was automatically updated to reflect the committed changes.
Alphare added inline comments.Nov 22 2019, 12:46 PM
rust/hg-core/src/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.

marmoute added inline comments.Nov 22 2019, 12:50 PM
rust/hg-core/src/lib.rs
117

This seems fine.

yuja added a subscriber: yuja.Nov 23 2019, 1:13 AM

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.

Alphare added a comment.Nov 25 2019, 5:25 AM
In 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?

yuja added a comment.Nov 25 2019, 8:47 AM
> 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.

Alphare added a comment.Nov 27 2019, 6:32 AM

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.

yuja added a comment.Nov 27 2019, 8:30 AM
> 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.

Alphare added a comment.Nov 27 2019, 8:35 AM

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 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) that @durin42 introduced.

yuja added a comment.Nov 28 2019, 8:14 AM
> 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.)

marmoute added a comment.Nov 29 2019, 6:00 AM
In 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

yuja added a comment.Nov 29 2019, 9:57 AM
>>   > 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.

marmoute added a comment.Nov 29 2019, 10:01 AM

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.

yuja added a comment.Nov 29 2019, 10:15 AM
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.

Revision Contents
Changeset List

Diff 18318

rust/hg-core/src/dirstate/dirs_multiset.rs

} }
multiset multiset
} }
/// Increases the count of deepest directory contained in the path. /// Increases the count of deepest directory contained in the path.
/// ///
/// If the directory is not yet in the map, adds its parents. /// 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) { 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) { if let Some(val) = self.inner.get_mut(subpath) {
*val += 1; *val += 1;
break; break;
} }
self.inner.insert(subpath.to_owned(), 1); self.inner.insert(subpath.to_owned(), 1);
} }
Ok(())
} }
/// Decreases the count of deepest directory contained in the path. /// Decreases the count of deepest directory contained in the path.
/// ///
/// If it is the only reference, decreases all parents until one is /// If it is the only reference, decreases all parents until one is
/// removed. /// removed.
/// If the directory is not in the map, something horrible has happened. /// If the directory is not in the map, something horrible has happened.
pub fn delete_path( pub fn delete_path(

rust/hg-core/src/dirstate/dirstate_map.rs

} }
/// Add a tracked file to the dirstate /// Add a tracked file to the dirstate
pub fn add_file( pub fn add_file(
&mut self, &mut self,
filename: &HgPath, filename: &HgPath,
old_state: EntryState, old_state: EntryState,
entry: DirstateEntry, entry: DirstateEntry,
) { ) -> Result<(), DirstateMapError> {
if old_state == EntryState::Unknown || old_state == EntryState::Removed if old_state == EntryState::Unknown || old_state == EntryState::Removed
{ {
if let Some(ref mut dirs) = self.dirs { if let Some(ref mut dirs) = self.dirs {
dirs.add_path(filename) dirs.add_path(filename)?;
} }
} }
if old_state == EntryState::Unknown { if old_state == EntryState::Unknown {
if let Some(ref mut all_dirs) = self.all_dirs { 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()); self.state_map.insert(filename.to_owned(), entry.to_owned());
if entry.state != EntryState::Normal || entry.mtime == MTIME_UNSET { if entry.state != EntryState::Normal || entry.mtime == MTIME_UNSET {
self.non_normal_set.insert(filename.to_owned()); self.non_normal_set.insert(filename.to_owned());
} }
if entry.size == SIZE_FROM_OTHER_PARENT { if entry.size == SIZE_FROM_OTHER_PARENT {
self.other_parent_set.insert(filename.to_owned()); self.other_parent_set.insert(filename.to_owned());
} }
Ok(())
} }
/// Mark a file as removed in the dirstate. /// Mark a file as removed in the dirstate.
/// ///
/// The `size` parameter is used to store sentinel values that indicate /// The `size` parameter is used to store sentinel values that indicate
/// the file's previous state. In the future, we should refactor this /// the file's previous state. In the future, we should refactor this
/// to be more explicit about what that state is. /// to be more explicit about what that state is.
pub fn remove_file( pub fn remove_file(

rust/hg-core/src/lib.rs

fn from(e: std::io::Error) -> Self { fn from(e: std::io::Error) -> Self {
DirstatePackError::CorruptedEntry(e.to_string()) DirstatePackError::CorruptedEntry(e.to_string())
} }
} }
#[derive(Debug, PartialEq)] #[derive(Debug, PartialEq)]
pub enum DirstateMapError { pub enum DirstateMapError {
PathNotFound(HgPathBuf), PathNotFound(HgPathBuf),
EmptyPath, 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()
}
}
}
marmouteUnsubmitted
Not Done

Should we also have the path included in that error ?

marmoute: Should we also have the path included in that error ?
AlphareAuthorUnsubmitted
Done

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.

Alphare: This patch is a minimal effort to stick to the C and Python implementations until a decision is…
marmouteUnsubmitted
Done

This seems fine.

marmoute: This seems fine.
} }
pub enum DirstateError { pub enum DirstateError {
Parse(DirstateParseError), Parse(DirstateParseError),
Pack(DirstatePackError), Pack(DirstatePackError),
Map(DirstateMapError), Map(DirstateMapError),
IO(std::io::Error), IO(std::io::Error),
} }

rust/hg-cpython/src/dirstate/dirs_multiset.rs

py, py,
PySharedRefCell::new(inner), PySharedRefCell::new(inner),
) )
} }
def addpath(&self, path: PyObject) -> PyResult<PyObject> { def addpath(&self, path: PyObject) -> PyResult<PyObject> {
self.inner_shared(py).borrow_mut()?.add_path( self.inner_shared(py).borrow_mut()?.add_path(
HgPath::new(path.extract::<PyBytes>(py)?.data(py)), HgPath::new(path.extract::<PyBytes>(py)?.data(py)),
); ).and(Ok(py.None())).or_else(|e| {
match e {
DirstateMapError::EmptyPath => {
Ok(py.None()) Ok(py.None())
},
e => {
Err(PyErr::new::<exc::ValueError, _>(
py,
e.to_string(),
))
}
}
})
} }
def delpath(&self, path: PyObject) -> PyResult<PyObject> { def delpath(&self, path: PyObject) -> PyResult<PyObject> {
self.inner_shared(py).borrow_mut()?.delete_path( self.inner_shared(py).borrow_mut()?.delete_path(
HgPath::new(path.extract::<PyBytes>(py)?.data(py)), HgPath::new(path.extract::<PyBytes>(py)?.data(py)),
) )
.and(Ok(py.None())) .and(Ok(py.None()))
.or_else(|e| { .or_else(|e| {
match e { match e {
DirstateMapError::PathNotFound(_p) => { DirstateMapError::EmptyPath => {
Ok(py.None())
},
e => {
Err(PyErr::new::<exc::ValueError, _>( Err(PyErr::new::<exc::ValueError, _>(
py, py,
"expected a value, found none".to_string(), e.to_string(),
)) ))
} }
DirstateMapError::EmptyPath => {
Ok(py.None())
}
} }
}) })
} }
def __iter__(&self) -> PyResult<DirsMultisetKeysIterator> { def __iter__(&self) -> PyResult<DirsMultisetKeysIterator> {
let leaked_ref = self.inner_shared(py).leak_immutable(); let leaked_ref = self.inner_shared(py).leak_immutable();
DirsMultisetKeysIterator::from_inner( DirsMultisetKeysIterator::from_inner(
py, py,
unsafe { leaked_ref.map(py, |o| o.iter()) }, unsafe { leaked_ref.map(py, |o| o.iter()) },

rust/hg-cpython/src/dirstate/dirstate_map.rs

use crate::{ use crate::{
dirstate::copymap::{CopyMap, CopyMapItemsIterator, CopyMapKeysIterator}, dirstate::copymap::{CopyMap, CopyMapItemsIterator, CopyMapKeysIterator},
dirstate::{dirs_multiset::Dirs, make_dirstate_tuple}, dirstate::{dirs_multiset::Dirs, make_dirstate_tuple},
ref_sharing::{PyLeaked, PySharedRefCell}, ref_sharing::{PyLeaked, PySharedRefCell},
}; };
use hg::{ use hg::{
utils::hg_path::{HgPath, HgPathBuf}, utils::hg_path::{HgPath, HgPathBuf},
DirsMultiset, DirstateEntry, DirstateMap as RustDirstateMap, DirsMultiset, DirstateEntry, DirstateMap as RustDirstateMap,
DirstateParents, DirstateParseError, EntryState, StateMapIter, DirstateMapError, DirstateParents, DirstateParseError, EntryState,
PARENT_SIZE, StateMapIter, PARENT_SIZE,
}; };
// TODO // TODO
// This object needs to share references to multiple members of its Rust // This object needs to share references to multiple members of its Rust
// inner struct, namely `copy_map`, `dirs` and `all_dirs`. // inner struct, namely `copy_map`, `dirs` and `all_dirs`.
// Right now `CopyMap` is done, but it needs to have an explicit reference // Right now `CopyMap` is done, but it needs to have an explicit reference
// to `RustDirstateMap` which itself needs to have an encapsulation for // to `RustDirstateMap` which itself needs to have an encapsulation for
// every method in `CopyMap` (copymapcopy, etc.). // every method in `CopyMap` (copymapcopy, etc.).
.try_into() .try_into()
.map_err(|e: DirstateParseError| { .map_err(|e: DirstateParseError| {
PyErr::new::<exc::ValueError, _>(py, e.to_string()) PyErr::new::<exc::ValueError, _>(py, e.to_string())
})?, })?,
mode: mode.extract(py)?, mode: mode.extract(py)?,
size: size.extract(py)?, size: size.extract(py)?,
mtime: mtime.extract(py)?, mtime: mtime.extract(py)?,
}, },
); ).and(Ok(py.None())).or_else(|e: DirstateMapError| {
Ok(py.None()) Err(PyErr::new::<exc::ValueError, _>(py, e.to_string()))
})
} }
def removefile( def removefile(
&self, &self,
f: PyObject, f: PyObject,
oldstate: PyObject, oldstate: PyObject,
size: PyObject size: PyObject
) -> PyResult<PyObject> { ) -> PyResult<PyObject> {