Patchwork [6,of,6] rust: propagate error of index_get_parents() properly

login
register
mail settings
Submitter Yuya Nishihara
Date Dec. 5, 2018, 1:43 p.m.
Message ID <3afbe6d40d6859d313f4.1544017417@mimosa>
Download mbox | patch
Permalink /patch/36979/
State Accepted
Headers show

Comments

Yuya Nishihara - Dec. 5, 2018, 1:43 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1540817453 -32400
#      Mon Oct 29 21:50:53 2018 +0900
# Node ID 3afbe6d40d6859d313f45b66ad5ca3c0a48f2e06
# Parent  f5cdfa49994e3943ba7c4ce2d66708142f0c7058
rust: propagate error of index_get_parents() properly

Before, rustla_contains() would return 0 on error, and the exception would
be cleared or noticed somewhere else. We need to propagate the error from
AncestorsIterator up to the FFI surface.
via Mercurial-devel - Dec. 8, 2018, 8:10 a.m.
On Wed, Dec 5, 2018 at 5:53 AM Yuya Nishihara <yuya@tcha.org> wrote:

> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1540817453 -32400
> #      Mon Oct 29 21:50:53 2018 +0900
> # Node ID 3afbe6d40d6859d313f45b66ad5ca3c0a48f2e06
> # Parent  f5cdfa49994e3943ba7c4ce2d66708142f0c7058
> rust: propagate error of index_get_parents() properly


Queued, thanks.
Georges Racinet - Dec. 10, 2018, 6:10 p.m.
Thanks for taking on this !
I have only one minor remark below

On 12/5/18 2:43 PM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1540817453 -32400
> #      Mon Oct 29 21:50:53 2018 +0900
> # Node ID 3afbe6d40d6859d313f45b66ad5ca3c0a48f2e06
> # Parent  f5cdfa49994e3943ba7c4ce2d66708142f0c7058
> rust: propagate error of index_get_parents() properly
>
> Before, rustla_contains() would return 0 on error, and the exception would
> be cleared or noticed somewhere else. We need to propagate the error from
> AncestorsIterator up to the FFI surface.
>
> diff --git a/rust/hg-core/src/ancestors.rs b/rust/hg-core/src/ancestors.rs
> --- a/rust/hg-core/src/ancestors.rs
> +++ b/rust/hg-core/src/ancestors.rs
> @@ -77,19 +77,20 @@ impl<G: Graph> AncestorsIterator<G> {
>      /// is in the ancestors it emits.
>      /// This is meant for iterators actually dedicated to that kind of
>      /// purpose
> -    pub fn contains(&mut self, target: Revision) -> bool {
> +    pub fn contains(&mut self, target: Revision) -> Result<bool, GraphError> {
>          if self.seen.contains(&target) && target != NULL_REVISION {
> -            return true;
> +            return Ok(true);
>          }
> -        for rev in self {
> +        for item in self {
> +            let rev = item?;
>              if rev == target {
> -                return true;
> +                return Ok(true);
>              }
>              if rev < target {
> -                return false;
> +                return Ok(false);
>              }
>          }
> -        false
> +        Ok(false)
>      }
>  }
>  
> @@ -117,19 +118,19 @@ impl<G: Graph> AncestorsIterator<G> {
>  ///   concrete caller we target, so we shouldn't need a finer error treatment
>  ///   for the time being.
I think the comment right above about error treatment becomes obsolete
with this patch.
>  impl<G: Graph> Iterator for AncestorsIterator<G> {
> -    type Item = Revision;
> +    type Item = Result<Revision, GraphError>;
>  
> -    fn next(&mut self) -> Option<Revision> {
> +    fn next(&mut self) -> Option<Self::Item> {
>          let current = match self.visit.peek() {
>              None => {
>                  return None;
>              }
>              Some(c) => *c,
>          };
> -        let (p1, p2) = self
> -            .graph
> -            .parents(current)
> -            .unwrap_or((NULL_REVISION, NULL_REVISION));
> +        let (p1, p2) = match self.graph.parents(current) {
> +            Ok(ps) => ps,
> +            Err(e) => return Some(Err(e)),
> +        };
>          if p1 < self.stoprev || self.seen.contains(&p1) {
>              self.visit.pop();
>          } else {
> @@ -138,7 +139,7 @@ impl<G: Graph> Iterator for AncestorsIte
>          };
>  
>          self.conditionally_push_rev(p2);
> -        Some(current)
> +        Some(Ok(current))
>      }
>  }
>  
> diff --git a/rust/hg-direct-ffi/src/ancestors.rs b/rust/hg-direct-ffi/src/ancestors.rs
> --- a/rust/hg-direct-ffi/src/ancestors.rs
> +++ b/rust/hg-direct-ffi/src/ancestors.rs
> @@ -139,7 +139,11 @@ pub extern "C" fn rustlazyancestors_next
>  #[inline]
>  fn raw_next<G: Graph>(raw: *mut AncestorsIterator<G>) -> c_long {
>      let as_ref = unsafe { &mut *raw };
> -    as_ref.next().unwrap_or(NULL_REVISION) as c_long
> +    let rev = match as_ref.next() {
> +        Some(Ok(rev)) => rev,
> +        Some(Err(_)) | None => NULL_REVISION,
> +    };
> +    rev as c_long
>  }
>  
>  #[no_mangle]
> @@ -157,10 +161,10 @@ fn raw_contains<G: Graph>(
>      target: c_long,
>  ) -> c_int {
>      let as_ref = unsafe { &mut *raw };
> -    if as_ref.contains(target as Revision) {
> -        return 1;
> +    match as_ref.contains(target as Revision) {
> +        Ok(r) => r as c_int,
> +        Err(_) => -1,
>      }
> -    0
>  }
>  
>  #[cfg(test)]
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/rust/hg-core/src/ancestors.rs b/rust/hg-core/src/ancestors.rs
--- a/rust/hg-core/src/ancestors.rs
+++ b/rust/hg-core/src/ancestors.rs
@@ -77,19 +77,20 @@  impl<G: Graph> AncestorsIterator<G> {
     /// is in the ancestors it emits.
     /// This is meant for iterators actually dedicated to that kind of
     /// purpose
-    pub fn contains(&mut self, target: Revision) -> bool {
+    pub fn contains(&mut self, target: Revision) -> Result<bool, GraphError> {
         if self.seen.contains(&target) && target != NULL_REVISION {
-            return true;
+            return Ok(true);
         }
-        for rev in self {
+        for item in self {
+            let rev = item?;
             if rev == target {
-                return true;
+                return Ok(true);
             }
             if rev < target {
-                return false;
+                return Ok(false);
             }
         }
-        false
+        Ok(false)
     }
 }
 
@@ -117,19 +118,19 @@  impl<G: Graph> AncestorsIterator<G> {
 ///   concrete caller we target, so we shouldn't need a finer error treatment
 ///   for the time being.
 impl<G: Graph> Iterator for AncestorsIterator<G> {
-    type Item = Revision;
+    type Item = Result<Revision, GraphError>;
 
-    fn next(&mut self) -> Option<Revision> {
+    fn next(&mut self) -> Option<Self::Item> {
         let current = match self.visit.peek() {
             None => {
                 return None;
             }
             Some(c) => *c,
         };
-        let (p1, p2) = self
-            .graph
-            .parents(current)
-            .unwrap_or((NULL_REVISION, NULL_REVISION));
+        let (p1, p2) = match self.graph.parents(current) {
+            Ok(ps) => ps,
+            Err(e) => return Some(Err(e)),
+        };
         if p1 < self.stoprev || self.seen.contains(&p1) {
             self.visit.pop();
         } else {
@@ -138,7 +139,7 @@  impl<G: Graph> Iterator for AncestorsIte
         };
 
         self.conditionally_push_rev(p2);
-        Some(current)
+        Some(Ok(current))
     }
 }
 
diff --git a/rust/hg-direct-ffi/src/ancestors.rs b/rust/hg-direct-ffi/src/ancestors.rs
--- a/rust/hg-direct-ffi/src/ancestors.rs
+++ b/rust/hg-direct-ffi/src/ancestors.rs
@@ -139,7 +139,11 @@  pub extern "C" fn rustlazyancestors_next
 #[inline]
 fn raw_next<G: Graph>(raw: *mut AncestorsIterator<G>) -> c_long {
     let as_ref = unsafe { &mut *raw };
-    as_ref.next().unwrap_or(NULL_REVISION) as c_long
+    let rev = match as_ref.next() {
+        Some(Ok(rev)) => rev,
+        Some(Err(_)) | None => NULL_REVISION,
+    };
+    rev as c_long
 }
 
 #[no_mangle]
@@ -157,10 +161,10 @@  fn raw_contains<G: Graph>(
     target: c_long,
 ) -> c_int {
     let as_ref = unsafe { &mut *raw };
-    if as_ref.contains(target as Revision) {
-        return 1;
+    match as_ref.contains(target as Revision) {
+        Ok(r) => r as c_int,
+        Err(_) => -1,
     }
-    0
 }
 
 #[cfg(test)]