changeset 43863:bc7d8f45c3b6

rust-dirs: handle forgotten `Result`s

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.

Differential Revision: https://phab.mercurial-scm.org/D7609

--- a/rust/hg-core/src/dirstate/dirs_multiset.rs	Fri Dec 13 09:43:43 2019 -0800
+++ b/rust/hg-core/src/dirstate/dirs_multiset.rs	Thu Dec 12 15:55:25 2019 +0100
@@ -30,7 +30,7 @@
     pub fn from_dirstate(
         dirstate: &FastHashMap<HgPathBuf, DirstateEntry>,
         skip_state: Option<EntryState>,
-    ) -> Self {
+    ) -> Result<Self, DirstateMapError> {
         let mut multiset = DirsMultiset {
             inner: FastHashMap::default(),
         };
@@ -39,27 +39,29 @@
             // 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(manifest: &[impl AsRef<HgPath>]) -> Self {
+    pub fn from_manifest(
+        manifest: &[impl AsRef<HgPath>],
+    ) -> Result<Self, DirstateMapError> {
         let mut multiset = DirsMultiset {
             inner: FastHashMap::default(),
         };
 
         for filename in manifest {
-            multiset.add_path(filename.as_ref());
+            multiset.add_path(filename.as_ref())?;
         }
 
-        multiset
+        Ok(multiset)
     }
 
     /// Increases the count of deepest directory contained in the path.
@@ -134,7 +136,7 @@
     #[test]
     fn test_delete_path_path_not_found() {
         let manifest: Vec<HgPathBuf> = vec![];
-        let mut map = DirsMultiset::from_manifest(&manifest);
+        let mut map = DirsMultiset::from_manifest(&manifest).unwrap();
         let path = HgPathBuf::from_bytes(b"doesnotexist/");
         assert_eq!(
             Err(DirstateMapError::PathNotFound(path.to_owned())),
@@ -144,7 +146,8 @@
 
     #[test]
     fn test_delete_path_empty_path() {
-        let mut map = DirsMultiset::from_manifest(&vec![HgPathBuf::new()]);
+        let mut map =
+            DirsMultiset::from_manifest(&vec![HgPathBuf::new()]).unwrap();
         let path = HgPath::new(b"");
         assert_eq!(Ok(()), map.delete_path(path));
         assert_eq!(
@@ -191,7 +194,7 @@
     #[test]
     fn test_add_path_empty_path() {
         let manifest: Vec<HgPathBuf> = vec![];
-        let mut map = DirsMultiset::from_manifest(&manifest);
+        let mut map = DirsMultiset::from_manifest(&manifest).unwrap();
         let path = HgPath::new(b"");
         map.add_path(path);
 
@@ -201,7 +204,7 @@
     #[test]
     fn test_add_path_successful() {
         let manifest: Vec<HgPathBuf> = vec![];
-        let mut map = DirsMultiset::from_manifest(&manifest);
+        let mut map = DirsMultiset::from_manifest(&manifest).unwrap();
 
         map.add_path(HgPath::new(b"a/"));
         assert_eq!(1, *map.inner.get(HgPath::new(b"a")).unwrap());
@@ -247,13 +250,14 @@
     #[test]
     fn test_dirsmultiset_new_empty() {
         let manifest: Vec<HgPathBuf> = vec![];
-        let new = DirsMultiset::from_manifest(&manifest);
+        let new = DirsMultiset::from_manifest(&manifest).unwrap();
         let expected = DirsMultiset {
             inner: FastHashMap::default(),
         };
         assert_eq!(expected, new);
 
-        let new = DirsMultiset::from_dirstate(&FastHashMap::default(), None);
+        let new = DirsMultiset::from_dirstate(&FastHashMap::default(), None)
+            .unwrap();
         let expected = DirsMultiset {
             inner: FastHashMap::default(),
         };
@@ -271,7 +275,7 @@
             .map(|(k, v)| (HgPathBuf::from_bytes(k.as_bytes()), *v))
             .collect();
 
-        let new = DirsMultiset::from_manifest(&input_vec);
+        let new = DirsMultiset::from_manifest(&input_vec).unwrap();
         let expected = DirsMultiset {
             inner: expected_inner,
         };
@@ -296,7 +300,7 @@
             .map(|(k, v)| (HgPathBuf::from_bytes(k.as_bytes()), *v))
             .collect();
 
-        let new = DirsMultiset::from_dirstate(&input_map, None);
+        let new = DirsMultiset::from_dirstate(&input_map, None).unwrap();
         let expected = DirsMultiset {
             inner: expected_inner,
         };
@@ -332,7 +336,8 @@
             .collect();
 
         let new =
-            DirsMultiset::from_dirstate(&input_map, Some(EntryState::Normal));
+            DirsMultiset::from_dirstate(&input_map, Some(EntryState::Normal))
+                .unwrap();
         let expected = DirsMultiset {
             inner: expected_inner,
         };

--- a/rust/hg-core/src/dirstate/dirstate_map.rs	Fri Dec 13 09:43:43 2019 -0800
+++ b/rust/hg-core/src/dirstate/dirstate_map.rs	Thu Dec 12 15:55:25 2019 +0100
@@ -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,38 @@
     /// 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(
@@ -350,11 +358,11 @@
         assert!(map.dirs.is_none());
         assert!(map.all_dirs.is_none());
 
-        assert_eq!(false, map.has_dir(HgPath::new(b"nope")));
+        assert_eq!(map.has_dir(HgPath::new(b"nope")).unwrap(), false);
         assert!(map.all_dirs.is_some());
         assert!(map.dirs.is_none());
 
-        assert_eq!(false, map.has_tracked_dir(HgPath::new(b"nope")));
+        assert_eq!(map.has_tracked_dir(HgPath::new(b"nope")).unwrap(), false);
         assert!(map.dirs.is_some());
     }
 

--- a/rust/hg-core/src/matchers.rs	Fri Dec 13 09:43:43 2019 -0800
+++ b/rust/hg-core/src/matchers.rs	Thu Dec 12 15:55:25 2019 +0100
@@ -7,7 +7,7 @@
 
 //! Structs and types for matching files and directories.
 
-use crate::utils::hg_path::{HgPath, HgPathBuf};
+use crate::utils::hg_path::HgPath;
 use std::collections::HashSet;
 
 pub enum VisitChildrenSet<'a> {

--- a/rust/hg-cpython/src/dirstate/dirs_multiset.rs	Fri Dec 13 09:43:43 2019 -0800
+++ b/rust/hg-cpython/src/dirstate/dirs_multiset.rs	Thu Dec 12 15:55:25 2019 +0100
@@ -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(

--- a/rust/hg-cpython/src/dirstate/dirstate_map.rs	Fri Dec 13 09:43:43 2019 -0800
+++ b/rust/hg-cpython/src/dirstate/dirstate_map.rs	Thu Dec 12 15:55:25 2019 +0100
@@ -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())
+            })?,
         )
     }