Alphare created this revision. Herald added subscribers: mercurial-devel, kevincox, durin42. Herald added a reviewer: hg-reviewers.
REVISION SUMMARY In 20a3bf5e71d6 <https://phab.mercurial-scm.org/rHG20a3bf5e71d669799f42122d7afb5d09c37b6835> I introduced a temporary bugfix to align Rust code with a new behavior from C/Python and forgot about a few `Result`s (cargo's compiler cache does not re-emit warnings on cached modules). This fixes it. For the record, I am still unsure that this behavior change is a good idea. Note: I was already quite unhappy with the setters and getters for the `DirstateMap` and, indirectly, `Dirs`, and this only further reinforces my feelings. I hope we can one day fix that situation at the type level; Georges Racinet and I were just talking about devising a POC for using the builder pattern in the context of FFI with Python, we'll see what comes out of it. REPOSITORY rHG Mercurial BRANCH default REVISION DETAIL https://phab.mercurial-scm.org/D7522 AFFECTED FILES rust/hg-core/src/dirstate/dirs_multiset.rs rust/hg-core/src/dirstate/dirstate_map.rs rust/hg-cpython/src/dirstate/dirs_multiset.rs rust/hg-cpython/src/dirstate/dirstate_map.rs CHANGE DETAILS 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 @@ -200,6 +200,9 @@ let d = d.extract::<PyBytes>(py)?; Ok(self.inner_shared(py).borrow_mut()? .has_tracked_dir(HgPath::new(d.data(py))) + .map_err(|e| { + PyErr::new::<exc::ValueError, _>(py, e.to_string()) + })? .to_py_object(py)) } @@ -207,6 +210,9 @@ let d = d.extract::<PyBytes>(py)?; Ok(self.inner_shared(py).borrow_mut()? .has_dir(HgPath::new(d.data(py))) + .map_err(|e| { + PyErr::new::<exc::ValueError, _>(py, e.to_string()) + })? .to_py_object(py)) } @@ -330,24 +336,35 @@ def getdirs(&self) -> PyResult<Dirs> { // TODO don't copy, share the reference - self.inner_shared(py).borrow_mut()?.set_dirs(); + self.inner_shared(py).borrow_mut()?.set_dirs() + .map_err(|e| { + PyErr::new::<exc::ValueError, _>(py, e.to_string()) + })?; Dirs::from_inner( py, DirsMultiset::from_dirstate( &self.inner_shared(py).borrow(), Some(EntryState::Removed), - ), + ) + .map_err(|e| { + PyErr::new::<exc::ValueError, _>(py, e.to_string()) + })?, ) } def getalldirs(&self) -> PyResult<Dirs> { // TODO don't copy, share the reference - self.inner_shared(py).borrow_mut()?.set_all_dirs(); + self.inner_shared(py).borrow_mut()?.set_all_dirs() + .map_err(|e| { + PyErr::new::<exc::ValueError, _>(py, e.to_string()) + })?; Dirs::from_inner( py, DirsMultiset::from_dirstate( &self.inner_shared(py).borrow(), None, - ), + ).map_err(|e| { + PyErr::new::<exc::ValueError, _>(py, e.to_string()) + })?, ) } 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 @@ -47,6 +47,9 @@ let inner = if let Ok(map) = map.cast_as::<PyDict>(py) { let dirstate = extract_dirstate(py, &map)?; DirsMultiset::from_dirstate(&dirstate, skip_state) + .map_err(|e| { + PyErr::new::<exc::ValueError, _>(py, e.to_string()) + })? } else { let map: Result<Vec<HgPathBuf>, PyErr> = map .iter(py)? @@ -57,6 +60,9 @@ }) .collect(); DirsMultiset::from_manifest(&map?) + .map_err(|e| { + PyErr::new::<exc::ValueError, _>(py, e.to_string()) + })? }; Self::create_instance( 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 @@ -126,7 +126,7 @@ } 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)?; } } @@ -227,30 +227,32 @@ /// emulate a Python lazy property, but it is ugly and unidiomatic. /// TODO One day, rewriting this struct using the typestate might be a /// good idea. - pub fn set_all_dirs(&mut self) { + pub fn set_all_dirs(&mut self) -> Result<(), DirstateMapError> { if self.all_dirs.is_none() { self.all_dirs = - Some(DirsMultiset::from_dirstate(&self.state_map, None)); + Some(DirsMultiset::from_dirstate(&self.state_map, None)?); } + Ok(()) } - pub fn set_dirs(&mut self) { + pub fn set_dirs(&mut self) -> Result<(), DirstateMapError> { if self.dirs.is_none() { self.dirs = Some(DirsMultiset::from_dirstate( &self.state_map, Some(EntryState::Removed), - )); + )?); } + Ok(()) } - pub fn has_tracked_dir(&mut self, directory: &HgPath) -> bool { - self.set_dirs(); - self.dirs.as_ref().unwrap().contains(directory) + pub fn has_tracked_dir(&mut self, directory: &HgPath) -> Result<bool, DirstateMapError> { + self.set_dirs()?; + Ok(self.dirs.as_ref().unwrap().contains(directory)) } - pub fn has_dir(&mut self, directory: &HgPath) -> bool { - self.set_all_dirs(); - self.all_dirs.as_ref().unwrap().contains(directory) + pub fn has_dir(&mut self, directory: &HgPath) -> Result<bool, DirstateMapError> { + self.set_all_dirs()?; + Ok(self.all_dirs.as_ref().unwrap().contains(directory)) } pub fn parents( 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 @@ -30,7 +30,7 @@ pub fn from_dirstate( vec: &HashMap<HgPathBuf, DirstateEntry>, skip_state: Option<EntryState>, - ) -> Self { + ) -> Result<Self, DirstateMapError> { let mut multiset = DirsMultiset { inner: HashMap::new(), }; @@ -39,27 +39,27 @@ // This `if` is optimized out of the loop if let Some(skip) = skip_state { if skip != *state { - multiset.add_path(filename); + multiset.add_path(filename)?; } } else { - multiset.add_path(filename); + multiset.add_path(filename)?; } } - multiset + Ok(multiset) } /// Initializes the multiset from a manifest. - pub fn from_manifest(vec: &Vec<HgPathBuf>) -> Self { + pub fn from_manifest(vec: &Vec<HgPathBuf>) -> Result<Self, DirstateMapError> { let mut multiset = DirsMultiset { inner: HashMap::new(), }; for filename in vec { - multiset.add_path(filename); + multiset.add_path(filename)?; } - multiset + Ok(multiset) } /// Increases the count of deepest directory contained in the path. To: Alphare, #hg-reviewers Cc: durin42, kevincox, mercurial-devel _______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
