Patchwork D7522: rust-dirs: handle forgotten `Result`s

login
register
mail settings
Submitter phabricator
Date Nov. 27, 2019, 1:30 p.m.
Message ID <differential-rev-PHID-DREV-zy6znhemx4zvm6l5psea-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/43526/
State Superseded
Headers show

Comments

phabricator - Nov. 27, 2019, 1:30 p.m.
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




To: Alphare, #hg-reviewers
Cc: durin42, kevincox, mercurial-devel
phabricator - Dec. 11, 2019, 5:41 p.m.
durin42 added a comment.
durin42 resigned from this revision.


  This needs a rebase that I'm not sure I can accomplish correctly. Could you take a look?

REPOSITORY
  rHG Mercurial

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

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

To: Alphare, #hg-reviewers, kevincox
Cc: durin42, kevincox, mercurial-devel
phabricator - Dec. 11, 2019, 6:57 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> dirstate_map.rs:355
>  
>          assert_eq!(false, map.has_dir(HgPath::new(b"nope")));
>          assert!(map.all_dirs.is_some());

Have to update at this test and probably others

REPOSITORY
  rHG Mercurial

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

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

To: Alphare, #hg-reviewers, kevincox
Cc: martinvonz, durin42, kevincox, mercurial-devel
phabricator - Dec. 12, 2019, 8:59 a.m.
Alphare added a comment.


  @martinvonz I was scratching my head trying to find this changeset to update/rebase it, here's `hg obslog -r 5a1ff3d75f35 --hidden`:
  
    x  5a1ff3d75f35 (46379) rust-dirs: handle forgotten `Result`s
    |    rewritten as 80c3e3d4ddea by Martin von Zweigbergk <martinvonz@google.com> (Wed Dec 11 10:51:38 2019 -0800)
    |
    x  2c62ed1a015a (46378) rust-dirs: handle forgotten `Result`s
         rewritten(description) as 5a1ff3d75f35 using phabsend by Raphaël Gomès <rgomes@octobus.net> (Wed Nov 27 14:30:10 2019 +0100)
  
  I don't have `80c3e3d4ddea` in my local repository. I could just revive `2c62ed1a015a` and edit from here, but I'm worried that might complicate things further.

REPOSITORY
  rHG Mercurial

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

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

To: Alphare, #hg-reviewers, kevincox
Cc: martinvonz, durin42, kevincox, mercurial-devel
phabricator - Dec. 12, 2019, 2:25 p.m.
martinvonz added a comment.


  In D7522#111970 <https://phab.mercurial-scm.org/D7522#111970>, @Alphare wrote:
  
  > @martinvonz I was scratching my head trying to find this changeset to update/rebase it, here's `hg obslog -r 5a1ff3d75f35 --hidden`:
  >
  >   x  5a1ff3d75f35 (46379) rust-dirs: handle forgotten `Result`s
  >   |    rewritten as 80c3e3d4ddea by Martin von Zweigbergk <martinvonz@google.com> (Wed Dec 11 10:51:38 2019 -0800)
  >   |
  >   x  2c62ed1a015a (46378) rust-dirs: handle forgotten `Result`s
  >        rewritten(description) as 5a1ff3d75f35 using phabsend by Raphaël Gomès <rgomes@octobus.net> (Wed Nov 27 14:30:10 2019 +0100)
  >
  > I don't have `80c3e3d4ddea` in my local repository. I could just revive `2c62ed1a015a` and edit from here, but I'm worried that might complicate things further.
  
  I think I had just pruned my local version of it. I assume you got the prune marker too, so I'm surprised that doesn't show up in obslog. Anyway, it should be the right thing to just revive your version. Sorry about the trouble. We need to fix this workflow some day.

REPOSITORY
  rHG Mercurial

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

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

To: Alphare, #hg-reviewers, kevincox
Cc: martinvonz, durin42, kevincox, mercurial-devel
phabricator - Dec. 12, 2019, 3 p.m.
Alphare added a comment.
Alphare abandoned this revision.


  Abandoned in favor of D7609 <https://phab.mercurial-scm.org/D7609>

REPOSITORY
  rHG Mercurial

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

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

To: Alphare, #hg-reviewers, kevincox
Cc: martinvonz, 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
@@ -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.