From patchwork Thu Dec 12 14:59:52 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: D7609: rust-dirs: handle forgotten `Result`s From: phabricator X-Patchwork-Id: 43737 Message-Id: To: Phabricator Cc: mercurial-devel@mercurial-scm.org Date: Thu, 12 Dec 2019 14:59:52 +0000 Alphare created this revision. Herald added subscribers: mercurial-devel, kevincox, durin42. Herald added a reviewer: hg-reviewers. REVISION SUMMARY In 1fe2e574616e 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/D7609 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 To: Alphare, #hg-reviewers Cc: durin42, kevincox, mercurial-devel 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 @@ -199,6 +199,9 @@ let d = d.extract::(py)?; Ok(self.inner_shared(py).borrow_mut()? .has_tracked_dir(HgPath::new(d.data(py))) + .map_err(|e| { + PyErr::new::(py, e.to_string()) + })? .to_py_object(py)) } @@ -206,6 +209,9 @@ let d = d.extract::(py)?; Ok(self.inner_shared(py).borrow_mut()? .has_dir(HgPath::new(d.data(py))) + .map_err(|e| { + PyErr::new::(py, e.to_string()) + })? .to_py_object(py)) } @@ -329,24 +335,35 @@ def getdirs(&self) -> PyResult { // 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::(py, e.to_string()) + })?; Dirs::from_inner( py, DirsMultiset::from_dirstate( &self.inner_shared(py).borrow(), Some(EntryState::Removed), - ), + ) + .map_err(|e| { + PyErr::new::(py, e.to_string()) + })?, ) } def getalldirs(&self) -> PyResult { // 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::(py, e.to_string()) + })?; Dirs::from_inner( py, DirsMultiset::from_dirstate( &self.inner_shared(py).borrow(), None, - ), + ).map_err(|e| { + PyErr::new::(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::(py) { let dirstate = extract_dirstate(py, &map)?; DirsMultiset::from_dirstate(&dirstate, skip_state) + .map_err(|e| { + PyErr::new::(py, e.to_string()) + })? } else { let map: Result, PyErr> = map .iter(py)? @@ -57,6 +60,9 @@ }) .collect(); DirsMultiset::from_manifest(&map?) + .map_err(|e| { + PyErr::new::(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 @@ -125,7 +125,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)?; } } @@ -226,30 +226,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 { + 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 { + 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, skip_state: Option, - ) -> Self { + ) -> Result { 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) -> Self { + pub fn from_manifest(vec: &Vec) -> Result { 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.