rust: make `Revision` a newtype

This change is the one we've been building towards during this series.
The aim is to make `Revision` mean more than a simple integer, holding
the information that it is valid for a given revlog index.
While this still allows for programmer error, since creating a revision
directly and querying a different index with a "checked" revision are
still possible, the friction created by the newtype will hopefully make
us think twice about which type to use.

Enough of the Rust ecosystem relies on the newtype pattern to be
efficiently optimized away (even compiler in codegen tests¹), so I'm not
worried about this being a fundamental problem.

[1] https://github.com/rust-lang/rust/blob/7a70647f195f6b0a0f1ebd72b1542ba91a32f43a/tests/codegen/vec-in-place.rs#L47

rust/hg-core/examples/nodemap/index.rs

@@ -29,7 +29,7 b' pub const INDEX_ENTRY_SIZE: usize = 64;'
 
 impl IndexEntry {
     fn parents(&self) -> [Revision; 2] {
-        [Revision::from_be(self.p1), Revision::from_be(self.p1)]
+        [self.p1, self.p2]
     }
 }
 
@@ -42,23 +42,18 b' impl RevlogIndex for Index {'
         if rev == NULL_REVISION {
             return None;
         }
-        let i = rev as usize;
-        if i >= self.len() {
-            None
-        } else {
-            Some(&self.data[i].node)
-        }
+        Some(&self.data[rev.0 as usize].node)
     }
 }
 
 impl Graph for &Index {
     fn parents(&self, rev: Revision) -> Result<[Revision; 2], GraphError> {
-        let [p1, p2] = (*self).data[rev as usize].parents();
+        let [p1, p2] = self.data[rev.0 as usize].parents();
         let len = (*self).len();
         if p1 < NULL_REVISION
             || p2 < NULL_REVISION
-            || p1 as usize >= len
-            || p2 as usize >= len
+            || p1.0 as usize >= len
+            || p2.0 as usize >= len
         {
             return Err(GraphError::ParentOutOfRange(rev));
         }

rust/hg-core/examples/nodemap/main.rs

@@ -36,7 +36,7 b' fn create(index: &Index, path: &Path) ->'
     let start = Instant::now();
     let mut nm = NodeTree::default();
     for rev in 0..index.len() {
-        let rev = rev as Revision;
+        let rev = Revision(rev as BaseRevision);
         nm.insert(index, index.node(rev).unwrap(), rev).unwrap();
     }
     eprintln!("Nodemap constructed in RAM in {:?}", start.elapsed());
@@ -55,7 +55,11 b' fn bench(index: &Index, nm: &NodeTree, q'
     let len = index.len() as u32;
     let mut rng = rand::thread_rng();
     let nodes: Vec<Node> = (0..queries)
-        .map(|_| *index.node((rng.gen::<u32>() % len) as Revision).unwrap())
+        .map(|_| {
+            *index
+                .node(Revision((rng.gen::<u32>() % len) as BaseRevision))
+                .unwrap()
+        })
         .collect();
     if queries < 10 {
         let nodes_hex: Vec<String> =

rust/hg-core/src/ancestors.rs

@@ -247,7 +247,9 b' impl<G: Graph> MissingAncestors<G> {'
                 revs.remove(&curr);
                 self.add_parents(curr)?;
             }
-            curr -= 1;
+            // We know this revision is safe because we've checked the bounds
+            // before.
+            curr = Revision(curr.0 - 1);
         }
         Ok(())
     }
@@ -297,14 +299,14 b' impl<G: Graph> MissingAncestors<G> {'
 
         // TODO heuristics for with_capacity()?
         let mut missing: Vec<Revision> = Vec::new();
-        for curr in (0..=start).rev() {
+        for curr in (0..=start.0).rev() {
             if revs_visit.is_empty() {
                 break;
             }
-            if both_visit.remove(&curr) {
+            if both_visit.remove(&Revision(curr)) {
                 // curr's parents might have made it into revs_visit through
                 // another path
-                for p in self.graph.parents(curr)?.iter().cloned() {
+                for p in self.graph.parents(Revision(curr))?.iter().cloned() {
                     if p == NULL_REVISION {
                         continue;
                     }
@@ -312,9 +314,9 b' impl<G: Graph> MissingAncestors<G> {'
                     bases_visit.insert(p);
                     both_visit.insert(p);
                 }
-            } else if revs_visit.remove(&curr) {
-                missing.push(curr);
-                for p in self.graph.parents(curr)?.iter().cloned() {
+            } else if revs_visit.remove(&Revision(curr)) {
+                missing.push(Revision(curr));
+                for p in self.graph.parents(Revision(curr))?.iter().cloned() {
                     if p == NULL_REVISION {
                         continue;
                     }
@@ -331,8 +333,8 b' impl<G: Graph> MissingAncestors<G> {'
                         revs_visit.insert(p);
                     }
                 }
-            } else if bases_visit.contains(&curr) {
-                for p in self.graph.parents(curr)?.iter().cloned() {
+            } else if bases_visit.contains(&Revision(curr)) {
+                for p in self.graph.parents(Revision(curr))?.iter().cloned() {
                     if p == NULL_REVISION {
                         continue;
                     }
@@ -356,7 +358,41 b' impl<G: Graph> MissingAncestors<G> {'
 mod tests {
 
     use super::*;
-    use crate::testing::{SampleGraph, VecGraph};
+    use crate::{
+        testing::{SampleGraph, VecGraph},
+        BaseRevision,
+    };
+
+    impl From<BaseRevision> for Revision {
+        fn from(value: BaseRevision) -> Self {
+            if !cfg!(test) {
+                panic!("should only be used in tests")
+            }
+            Revision(value)
+        }
+    }
+
+    impl PartialEq<BaseRevision> for Revision {
+        fn eq(&self, other: &BaseRevision) -> bool {
+            if !cfg!(test) {
+                panic!("should only be used in tests")
+            }
+            self.0.eq(other)
+        }
+    }
+
+    impl PartialEq<u32> for Revision {
+        fn eq(&self, other: &u32) -> bool {
+            if !cfg!(test) {
+                panic!("should only be used in tests")
+            }
+            let check: Result<u32, _> = self.0.try_into();
+            match check {
+                Ok(value) => value.eq(other),
+                Err(_) => false,
+            }
+        }
+    }
 
     fn list_ancestors<G: Graph>(
         graph: G,
@@ -374,37 +410,80 b' mod tests {'
     /// Same tests as test-ancestor.py, without membership
     /// (see also test-ancestor.py.out)
     fn test_list_ancestor() {
-        assert_eq!(list_ancestors(SampleGraph, vec![], 0, false), vec![]);
+        assert_eq!(
+            list_ancestors(SampleGraph, vec![], 0.into(), false),
+            Vec::<Revision>::new()
+        );
         assert_eq!(
-            list_ancestors(SampleGraph, vec![11, 13], 0, false),
+            list_ancestors(
+                SampleGraph,
+                vec![11.into(), 13.into()],
+                0.into(),
+                false
+            ),
             vec![8, 7, 4, 3, 2, 1, 0]
         );
         assert_eq!(
-            list_ancestors(SampleGraph, vec![1, 3], 0, false),
+            list_ancestors(
+                SampleGraph,
+                vec![1.into(), 3.into()],
+                0.into(),
+                false
+            ),
             vec![1, 0]
         );
         assert_eq!(
-            list_ancestors(SampleGraph, vec![11, 13], 0, true),
+            list_ancestors(
+                SampleGraph,
+                vec![11.into(), 13.into()],
+                0.into(),
+                true
+            ),
             vec![13, 11, 8, 7, 4, 3, 2, 1, 0]
         );
         assert_eq!(
-            list_ancestors(SampleGraph, vec![11, 13], 6, false),
+            list_ancestors(
+                SampleGraph,
+                vec![11.into(), 13.into()],
+                6.into(),
+                false
+            ),
             vec![8, 7]
         );
         assert_eq!(
-            list_ancestors(SampleGraph, vec![11, 13], 6, true),
+            list_ancestors(
+                SampleGraph,
+                vec![11.into(), 13.into()],
+                6.into(),
+                true
+            ),
             vec![13, 11, 8, 7]
         );
         assert_eq!(
-            list_ancestors(SampleGraph, vec![11, 13], 11, true),
+            list_ancestors(
+                SampleGraph,
+                vec![11.into(), 13.into()],
+                11.into(),
+                true
+            ),
             vec![13, 11]
         );
         assert_eq!(
-            list_ancestors(SampleGraph, vec![11, 13], 12, true),
+            list_ancestors(
+                SampleGraph,
+                vec![11.into(), 13.into()],
+                12.into(),
+                true
+            ),
             vec![13]
         );
         assert_eq!(
-            list_ancestors(SampleGraph, vec![10, 1], 0, true),
+            list_ancestors(
+                SampleGraph,
+                vec![10.into(), 1.into()],
+                0.into(),
+                true
+            ),
             vec![10, 5, 4, 2, 1, 0]
         );
     }
@@ -415,33 +494,53 b' mod tests {'
     /// suite.
     /// For instance, run tests/test-obsolete-checkheads.t
     fn test_nullrev_input() {
-        let mut iter =
-            AncestorsIterator::new(SampleGraph, vec![-1], 0, false).unwrap();
+        let mut iter = AncestorsIterator::new(
+            SampleGraph,
+            vec![Revision(-1)],
+            0.into(),
+            false,
+        )
+        .unwrap();
         assert_eq!(iter.next(), None)
     }
 
     #[test]
     fn test_contains() {
-        let mut lazy =
-            AncestorsIterator::new(SampleGraph, vec![10, 1], 0, true).unwrap();
-        assert!(lazy.contains(1).unwrap());
-        assert!(!lazy.contains(3).unwrap());
+        let mut lazy = AncestorsIterator::new(
+            SampleGraph,
+            vec![10.into(), 1.into()],
+            0.into(),
+            true,
+        )
+        .unwrap();
+        assert!(lazy.contains(1.into()).unwrap());
+        assert!(!lazy.contains(3.into()).unwrap());
 
-        let mut lazy =
-            AncestorsIterator::new(SampleGraph, vec![0], 0, false).unwrap();
+        let mut lazy = AncestorsIterator::new(
+            SampleGraph,
+            vec![0.into()],
+            0.into(),
+            false,
+        )
+        .unwrap();
         assert!(!lazy.contains(NULL_REVISION).unwrap());
     }
 
     #[test]
     fn test_peek() {
-        let mut iter =
-            AncestorsIterator::new(SampleGraph, vec![10], 0, true).unwrap();
+        let mut iter = AncestorsIterator::new(
+            SampleGraph,
+            vec![10.into()],
+            0.into(),
+            true,
+        )
+        .unwrap();
         // peek() gives us the next value
-        assert_eq!(iter.peek(), Some(10));
+        assert_eq!(iter.peek(), Some(10.into()));
         // but it's not been consumed
-        assert_eq!(iter.next(), Some(Ok(10)));
+        assert_eq!(iter.next(), Some(Ok(10.into())));
         // and iteration resumes normally
-        assert_eq!(iter.next(), Some(Ok(5)));
+        assert_eq!(iter.next(), Some(Ok(5.into())));
 
         // let's drain the iterator to test peek() at the end
         while iter.next().is_some() {}
@@ -450,19 +549,29 b' mod tests {'
 
     #[test]
     fn test_empty() {
-        let mut iter =
-            AncestorsIterator::new(SampleGraph, vec![10], 0, true).unwrap();
+        let mut iter = AncestorsIterator::new(
+            SampleGraph,
+            vec![10.into()],
+            0.into(),
+            true,
+        )
+        .unwrap();
         assert!(!iter.is_empty());
         while iter.next().is_some() {}
         assert!(!iter.is_empty());
 
-        let iter =
-            AncestorsIterator::new(SampleGraph, vec![], 0, true).unwrap();
+        let iter = AncestorsIterator::new(SampleGraph, vec![], 0.into(), true)
+            .unwrap();
         assert!(iter.is_empty());
 
         // case where iter.seen == {NULL_REVISION}
-        let iter =
-            AncestorsIterator::new(SampleGraph, vec![0], 0, false).unwrap();
+        let iter = AncestorsIterator::new(
+            SampleGraph,
+            vec![0.into()],
+            0.into(),
+            false,
+        )
+        .unwrap();
         assert!(iter.is_empty());
     }
 
@@ -471,9 +580,11 b' mod tests {'
     struct Corrupted;
 
     impl Graph for Corrupted {
+        // FIXME what to do about this? Are we just not supposed to get them
+        // anymore?
         fn parents(&self, rev: Revision) -> Result<[Revision; 2], GraphError> {
             match rev {
-                1 => Ok([0, -1]),
+                Revision(1) => Ok([0.into(), (-1).into()]),
                 r => Err(GraphError::ParentOutOfRange(r)),
             }
         }
@@ -482,9 +593,14 b' mod tests {'
     #[test]
     fn test_initrev_out_of_range() {
         // inclusive=false looks up initrev's parents right away
-        match AncestorsIterator::new(SampleGraph, vec![25], 0, false) {
+        match AncestorsIterator::new(
+            SampleGraph,
+            vec![25.into()],
+            0.into(),
+            false,
+        ) {
             Ok(_) => panic!("Should have been ParentOutOfRange"),
-            Err(e) => assert_eq!(e, GraphError::ParentOutOfRange(25)),
+            Err(e) => assert_eq!(e, GraphError::ParentOutOfRange(25.into())),
         }
     }
 
@@ -492,22 +608,29 b' mod tests {'
     fn test_next_out_of_range() {
         // inclusive=false looks up initrev's parents right away
         let mut iter =
-            AncestorsIterator::new(Corrupted, vec![1], 0, false).unwrap();
-        assert_eq!(iter.next(), Some(Err(GraphError::ParentOutOfRange(0))));
+            AncestorsIterator::new(Corrupted, vec![1.into()], 0.into(), false)
+                .unwrap();
+        assert_eq!(
+            iter.next(),
+            Some(Err(GraphError::ParentOutOfRange(0.into())))
+        );
     }
 
     #[test]
     /// Test constructor, add/get bases and heads
     fn test_missing_bases() -> Result<(), GraphError> {
-        let mut missing_ancestors =
-            MissingAncestors::new(SampleGraph, [5, 3, 1, 3].iter().cloned());
+        let mut missing_ancestors = MissingAncestors::new(
+            SampleGraph,
+            [5.into(), 3.into(), 1.into(), 3.into()].iter().cloned(),
+        );
         let mut as_vec: Vec<Revision> =
             missing_ancestors.get_bases().iter().cloned().collect();
         as_vec.sort_unstable();
         assert_eq!(as_vec, [1, 3, 5]);
         assert_eq!(missing_ancestors.max_base, 5);
 
-        missing_ancestors.add_bases([3, 7, 8].iter().cloned());
+        missing_ancestors
+            .add_bases([3.into(), 7.into(), 8.into()].iter().cloned());
         as_vec = missing_ancestors.get_bases().iter().cloned().collect();
         as_vec.sort_unstable();
         assert_eq!(as_vec, [1, 3, 5, 7, 8]);
@@ -520,13 +643,16 b' mod tests {'
     }
 
     fn assert_missing_remove(
-        bases: &[Revision],
-        revs: &[Revision],
-        expected: &[Revision],
+        bases: &[BaseRevision],
+        revs: &[BaseRevision],
+        expected: &[BaseRevision],
     ) {
-        let mut missing_ancestors =
-            MissingAncestors::new(SampleGraph, bases.iter().cloned());
-        let mut revset: HashSet<Revision> = revs.iter().cloned().collect();
+        let mut missing_ancestors = MissingAncestors::new(
+            SampleGraph,
+            bases.iter().map(|r| Revision(*r)),
+        );
+        let mut revset: HashSet<Revision> =
+            revs.iter().map(|r| Revision(*r)).collect();
         missing_ancestors
             .remove_ancestors_from(&mut revset)
             .unwrap();
@@ -547,14 +673,16 b' mod tests {'
     }
 
     fn assert_missing_ancestors(
-        bases: &[Revision],
-        revs: &[Revision],
-        expected: &[Revision],
+        bases: &[BaseRevision],
+        revs: &[BaseRevision],
+        expected: &[BaseRevision],
     ) {
-        let mut missing_ancestors =
-            MissingAncestors::new(SampleGraph, bases.iter().cloned());
+        let mut missing_ancestors = MissingAncestors::new(
+            SampleGraph,
+            bases.iter().map(|r| Revision(*r)),
+        );
         let missing = missing_ancestors
-            .missing_ancestors(revs.iter().cloned())
+            .missing_ancestors(revs.iter().map(|r| Revision(*r)))
             .unwrap();
         assert_eq!(missing.as_slice(), expected);
     }
@@ -575,110 +703,115 b' mod tests {'
     #[allow(clippy::unnecessary_cast)]
     #[test]
     fn test_remove_ancestors_from_case1() {
+        const FAKE_NULL_REVISION: BaseRevision = -1;
+        assert_eq!(FAKE_NULL_REVISION, NULL_REVISION.0);
         let graph: VecGraph = vec![
-            [NULL_REVISION, NULL_REVISION],
-            [0, NULL_REVISION],
+            [FAKE_NULL_REVISION, FAKE_NULL_REVISION],
+            [0, FAKE_NULL_REVISION],
             [1, 0],
             [2, 1],
-            [3, NULL_REVISION],
-            [4, NULL_REVISION],
+            [3, FAKE_NULL_REVISION],
+            [4, FAKE_NULL_REVISION],
             [5, 1],
-            [2, NULL_REVISION],
-            [7, NULL_REVISION],
-            [8, NULL_REVISION],
-            [9, NULL_REVISION],
+            [2, FAKE_NULL_REVISION],
+            [7, FAKE_NULL_REVISION],
+            [8, FAKE_NULL_REVISION],
+            [9, FAKE_NULL_REVISION],
             [10, 1],
-            [3, NULL_REVISION],
-            [12, NULL_REVISION],
-            [13, NULL_REVISION],
-            [14, NULL_REVISION],
-            [4, NULL_REVISION],
-            [16, NULL_REVISION],
-            [17, NULL_REVISION],
-            [18, NULL_REVISION],
+            [3, FAKE_NULL_REVISION],
+            [12, FAKE_NULL_REVISION],
+            [13, FAKE_NULL_REVISION],
+            [14, FAKE_NULL_REVISION],
+            [4, FAKE_NULL_REVISION],
+            [16, FAKE_NULL_REVISION],
+            [17, FAKE_NULL_REVISION],
+            [18, FAKE_NULL_REVISION],
             [19, 11],
-            [20, NULL_REVISION],
-            [21, NULL_REVISION],
-            [22, NULL_REVISION],
-            [23, NULL_REVISION],
-            [2, NULL_REVISION],
-            [3, NULL_REVISION],
+            [20, FAKE_NULL_REVISION],
+            [21, FAKE_NULL_REVISION],
+            [22, FAKE_NULL_REVISION],
+            [23, FAKE_NULL_REVISION],
+            [2, FAKE_NULL_REVISION],
+            [3, FAKE_NULL_REVISION],
             [26, 24],
-            [27, NULL_REVISION],
-            [28, NULL_REVISION],
-            [12, NULL_REVISION],
-            [1, NULL_REVISION],
+            [27, FAKE_NULL_REVISION],
+            [28, FAKE_NULL_REVISION],
+            [12, FAKE_NULL_REVISION],
+            [1, FAKE_NULL_REVISION],
             [1, 9],
-            [32, NULL_REVISION],
-            [33, NULL_REVISION],
+            [32, FAKE_NULL_REVISION],
+            [33, FAKE_NULL_REVISION],
             [34, 31],
-            [35, NULL_REVISION],
+            [35, FAKE_NULL_REVISION],
             [36, 26],
-            [37, NULL_REVISION],
-            [38, NULL_REVISION],
-            [39, NULL_REVISION],
-            [40, NULL_REVISION],
-            [41, NULL_REVISION],
+            [37, FAKE_NULL_REVISION],
+            [38, FAKE_NULL_REVISION],
+            [39, FAKE_NULL_REVISION],
+            [40, FAKE_NULL_REVISION],
+            [41, FAKE_NULL_REVISION],
             [42, 26],
-            [0, NULL_REVISION],
-            [44, NULL_REVISION],
+            [0, FAKE_NULL_REVISION],
+            [44, FAKE_NULL_REVISION],
             [45, 4],
-            [40, NULL_REVISION],
-            [47, NULL_REVISION],
+            [40, FAKE_NULL_REVISION],
+            [47, FAKE_NULL_REVISION],
             [36, 0],
-            [49, NULL_REVISION],
-            [NULL_REVISION, NULL_REVISION],
-            [51, NULL_REVISION],
-            [52, NULL_REVISION],
-            [53, NULL_REVISION],
-            [14, NULL_REVISION],
-            [55, NULL_REVISION],
-            [15, NULL_REVISION],
-            [23, NULL_REVISION],
-            [58, NULL_REVISION],
-            [59, NULL_REVISION],
-            [2, NULL_REVISION],
+            [49, FAKE_NULL_REVISION],
+            [FAKE_NULL_REVISION, FAKE_NULL_REVISION],
+            [51, FAKE_NULL_REVISION],
+            [52, FAKE_NULL_REVISION],
+            [53, FAKE_NULL_REVISION],
+            [14, FAKE_NULL_REVISION],
+            [55, FAKE_NULL_REVISION],
+            [15, FAKE_NULL_REVISION],
+            [23, FAKE_NULL_REVISION],
+            [58, FAKE_NULL_REVISION],
+            [59, FAKE_NULL_REVISION],
+            [2, FAKE_NULL_REVISION],
             [61, 59],
-            [62, NULL_REVISION],
-            [63, NULL_REVISION],
-            [NULL_REVISION, NULL_REVISION],
-            [65, NULL_REVISION],
-            [66, NULL_REVISION],
-            [67, NULL_REVISION],
-            [68, NULL_REVISION],
+            [62, FAKE_NULL_REVISION],
+            [63, FAKE_NULL_REVISION],
+            [FAKE_NULL_REVISION, FAKE_NULL_REVISION],
+            [65, FAKE_NULL_REVISION],
+            [66, FAKE_NULL_REVISION],
+            [67, FAKE_NULL_REVISION],
+            [68, FAKE_NULL_REVISION],
             [37, 28],
             [69, 25],
-            [71, NULL_REVISION],
-            [72, NULL_REVISION],
+            [71, FAKE_NULL_REVISION],
+            [72, FAKE_NULL_REVISION],
             [50, 2],
-            [74, NULL_REVISION],
-            [12, NULL_REVISION],
-            [18, NULL_REVISION],
-            [77, NULL_REVISION],
-            [78, NULL_REVISION],
-            [79, NULL_REVISION],
+            [74, FAKE_NULL_REVISION],
+            [12, FAKE_NULL_REVISION],
+            [18, FAKE_NULL_REVISION],
+            [77, FAKE_NULL_REVISION],
+            [78, FAKE_NULL_REVISION],
+            [79, FAKE_NULL_REVISION],
             [43, 33],
-            [81, NULL_REVISION],
-            [82, NULL_REVISION],
-            [83, NULL_REVISION],
+            [81, FAKE_NULL_REVISION],
+            [82, FAKE_NULL_REVISION],
+            [83, FAKE_NULL_REVISION],
             [84, 45],
-            [85, NULL_REVISION],
-            [86, NULL_REVISION],
-            [NULL_REVISION, NULL_REVISION],
-            [88, NULL_REVISION],
-            [NULL_REVISION, NULL_REVISION],
+            [85, FAKE_NULL_REVISION],
+            [86, FAKE_NULL_REVISION],
+            [FAKE_NULL_REVISION, FAKE_NULL_REVISION],
+            [88, FAKE_NULL_REVISION],
+            [FAKE_NULL_REVISION, FAKE_NULL_REVISION],
             [76, 83],
-            [44, NULL_REVISION],
-            [92, NULL_REVISION],
-            [93, NULL_REVISION],
-            [9, NULL_REVISION],
+            [44, FAKE_NULL_REVISION],
+            [92, FAKE_NULL_REVISION],
+            [93, FAKE_NULL_REVISION],
+            [9, FAKE_NULL_REVISION],
             [95, 67],
-            [96, NULL_REVISION],
-            [97, NULL_REVISION],
-            [NULL_REVISION, NULL_REVISION],
-        ];
-        let problem_rev = 28 as Revision;
-        let problem_base = 70 as Revision;
+            [96, FAKE_NULL_REVISION],
+            [97, FAKE_NULL_REVISION],
+            [FAKE_NULL_REVISION, FAKE_NULL_REVISION],
+        ]
+        .into_iter()
+        .map(|[a, b]| [Revision(a), Revision(b)])
+        .collect();
+        let problem_rev = 28.into();
+        let problem_base = 70.into();
         // making the problem obvious: problem_rev is a parent of problem_base
         assert_eq!(graph.parents(problem_base).unwrap()[1], problem_rev);
 
@@ -687,14 +820,14 b' mod tests {'
                 graph,
                 [60, 26, 70, 3, 96, 19, 98, 49, 97, 47, 1, 6]
                     .iter()
-                    .cloned(),
+                    .map(|r| Revision(*r)),
             );
         assert!(missing_ancestors.bases.contains(&problem_base));
 
         let mut revs: HashSet<Revision> =
             [4, 12, 41, 28, 68, 38, 1, 30, 56, 44]
                 .iter()
-                .cloned()
+                .map(|r| Revision(*r))
                 .collect();
         missing_ancestors.remove_ancestors_from(&mut revs).unwrap();
         assert!(!revs.contains(&problem_rev));

rust/hg-core/src/copy_tracing/tests.rs

@@ -1,5 +1,12 b''
 use super::*;
 
+/// Shorthand to reduce boilerplate when creating [`Revision`] for testing
+macro_rules! R {
+    ($revision:literal) => {
+        Revision($revision)
+    };
+}
+
 /// Unit tests for:
 ///
 /// ```ignore
@@ -27,7 +34,12 b' fn test_compare_value() {'
     use MergePick::*;
 
     assert_eq!(
-        compare_value!(1, Normal, (1, None, { 1 }), (1, None, { 1 })),
+        compare_value!(
+            R!(1),
+            Normal,
+            (R!(1), None, { R!(1) }),
+            (R!(1), None, { R!(1) })
+        ),
         (Any, false)
     );
 }
@@ -70,12 +82,12 b' fn test_merge_copies_dict() {'
 
     assert_eq!(
         merge_copies_dict!(
-            1,
-            {"foo" => (1, None, {})},
+            R!(1),
+            {"foo" => (R!(1), None, {})},
             {},
             {"foo" => Merged}
         ),
-        internal_path_copies!("foo" => (1, None, {}))
+        internal_path_copies!("foo" => (R!(1), None, {}))
     );
 }
 
@@ -124,17 +136,29 b' fn test_combine_changeset_copies() {'
 
     assert_eq!(
         combine_changeset_copies!(
-            { 1 => 1, 2 => 1 },
+            { R!(1) => 1, R!(2) => 1 },
             [
-                { rev: 1, p1: NULL, p2: NULL, actions: [], merge_cases: {}, },
-                { rev: 2, p1: NULL, p2: NULL, actions: [], merge_cases: {}, },
+                {
+                    rev: R!(1),
+                    p1: NULL,
+                    p2: NULL,
+                    actions: [],
+                    merge_cases: {},
+                },
                 {
-                    rev: 3, p1: 1, p2: 2,
+                    rev: R!(2),
+                    p1: NULL,
+                    p2: NULL,
+                    actions: [],
+                    merge_cases: {},
+                },
+                {
+                    rev: R!(3), p1: R!(1), p2: R!(2),
                     actions: [CopiedFromP1("destination.txt", "source.txt")],
                     merge_cases: {"destination.txt" => Merged},
                 },
             ],
-            3,
+            R!(3),
         ),
         path_copies!("destination.txt" => "source.txt")
     );

rust/hg-core/src/dagops.rs

@@ -171,14 +171,15 b' pub fn range('
 mod tests {
 
     use super::*;
-    use crate::testing::SampleGraph;
+    use crate::{testing::SampleGraph, BaseRevision};
 
     /// Apply `retain_heads()` to the given slice and return as a sorted `Vec`
     fn retain_heads_sorted(
         graph: &impl Graph,
-        revs: &[Revision],
+        revs: &[BaseRevision],
     ) -> Result<Vec<Revision>, GraphError> {
-        let mut revs: HashSet<Revision> = revs.iter().cloned().collect();
+        let mut revs: HashSet<Revision> =
+            revs.iter().cloned().map(Revision).collect();
         retain_heads(graph, &mut revs)?;
         let mut as_vec: Vec<Revision> = revs.iter().cloned().collect();
         as_vec.sort_unstable();
@@ -202,9 +203,11 b' mod tests {'
     /// Apply `heads()` to the given slice and return as a sorted `Vec`
     fn heads_sorted(
         graph: &impl Graph,
-        revs: &[Revision],
+        revs: &[BaseRevision],
     ) -> Result<Vec<Revision>, GraphError> {
-        let heads = heads(graph, revs.iter())?;
+        let iter_revs: Vec<_> =
+            revs.into_iter().cloned().map(Revision).collect();
+        let heads = heads(graph, iter_revs.iter())?;
         let mut as_vec: Vec<Revision> = heads.iter().cloned().collect();
         as_vec.sort_unstable();
         Ok(as_vec)
@@ -227,9 +230,9 b' mod tests {'
     /// Apply `roots()` and sort the result for easier comparison
     fn roots_sorted(
         graph: &impl Graph,
-        revs: &[Revision],
+        revs: &[BaseRevision],
     ) -> Result<Vec<Revision>, GraphError> {
-        let set: HashSet<_> = revs.iter().cloned().collect();
+        let set: HashSet<_> = revs.iter().cloned().map(Revision).collect();
         let mut as_vec = roots(graph, &set)?;
         as_vec.sort_unstable();
         Ok(as_vec)
@@ -252,17 +255,24 b' mod tests {'
     /// Apply `range()` and convert the result into a Vec for easier comparison
     fn range_vec(
         graph: impl Graph + Clone,
-        roots: &[Revision],
-        heads: &[Revision],
+        roots: &[BaseRevision],
+        heads: &[BaseRevision],
     ) -> Result<Vec<Revision>, GraphError> {
-        range(&graph, roots.iter().cloned(), heads.iter().cloned())
-            .map(|bs| bs.into_iter().collect())
+        range(
+            &graph,
+            roots.iter().cloned().map(Revision),
+            heads.iter().cloned().map(Revision),
+        )
+        .map(|bs| bs.into_iter().collect())
     }
 
     #[test]
     fn test_range() -> Result<(), GraphError> {
         assert_eq!(range_vec(SampleGraph, &[0], &[4])?, vec![0, 1, 2, 4]);
-        assert_eq!(range_vec(SampleGraph, &[0], &[8])?, vec![]);
+        assert_eq!(
+            range_vec(SampleGraph, &[0], &[8])?,
+            Vec::<Revision>::new()
+        );
         assert_eq!(
             range_vec(SampleGraph, &[5, 6], &[10, 11, 13])?,
             vec![5, 10]

rust/hg-core/src/discovery.rs

@@ -481,6 +481,13 b' mod tests {'
     use super::*;
     use crate::testing::SampleGraph;
 
+    /// Shorthand to reduce boilerplate when creating [`Revision`] for testing
+    macro_rules! R {
+        ($revision:literal) => {
+            Revision($revision)
+        };
+    }
+
     /// A PartialDiscovery as for pushing all the heads of `SampleGraph`
     ///
     /// To avoid actual randomness in these tests, we give it a fixed
@@ -488,7 +495,7 b' mod tests {'
     fn full_disco() -> PartialDiscovery<SampleGraph> {
         PartialDiscovery::new_with_seed(
             SampleGraph,
-            vec![10, 11, 12, 13],
+            vec![R!(10), R!(11), R!(12), R!(13)],
             [0; 16],
             true,
             true,
@@ -501,7 +508,7 b' mod tests {'
     fn disco12() -> PartialDiscovery<SampleGraph> {
         PartialDiscovery::new_with_seed(
             SampleGraph,
-            vec![12],
+            vec![R!(12)],
             [0; 16],
             true,
             true,
@@ -540,7 +547,7 b' mod tests {'
         assert!(!disco.has_info());
         assert_eq!(disco.stats().undecided, None);
 
-        disco.add_common_revisions(vec![11, 12])?;
+        disco.add_common_revisions(vec![R!(11), R!(12)])?;
         assert!(disco.has_info());
         assert!(!disco.is_complete());
         assert!(disco.missing.is_empty());
@@ -559,14 +566,14 b' mod tests {'
     #[test]
     fn test_discovery() -> Result<(), GraphError> {
         let mut disco = full_disco();
-        disco.add_common_revisions(vec![11, 12])?;
-        disco.add_missing_revisions(vec![8, 10])?;
+        disco.add_common_revisions(vec![R!(11), R!(12)])?;
+        disco.add_missing_revisions(vec![R!(8), R!(10)])?;
         assert_eq!(sorted_undecided(&disco), vec![5]);
         assert_eq!(sorted_missing(&disco), vec![8, 10, 13]);
         assert!(!disco.is_complete());
 
-        disco.add_common_revisions(vec![5])?;
-        assert_eq!(sorted_undecided(&disco), vec![]);
+        disco.add_common_revisions(vec![R!(5)])?;
+        assert_eq!(sorted_undecided(&disco), Vec::<Revision>::new());
         assert_eq!(sorted_missing(&disco), vec![8, 10, 13]);
         assert!(disco.is_complete());
         assert_eq!(sorted_common_heads(&disco)?, vec![5, 11, 12]);
@@ -577,12 +584,12 b' mod tests {'
     fn test_add_missing_early_continue() -> Result<(), GraphError> {
         eprintln!("test_add_missing_early_stop");
         let mut disco = full_disco();
-        disco.add_common_revisions(vec![13, 3, 4])?;
+        disco.add_common_revisions(vec![R!(13), R!(3), R!(4)])?;
         disco.ensure_children_cache()?;
         // 12 is grand-child of 6 through 9
         // passing them in this order maximizes the chances of the
         // early continue to do the wrong thing
-        disco.add_missing_revisions(vec![6, 9, 12])?;
+        disco.add_missing_revisions(vec![R!(6), R!(9), R!(12)])?;
         assert_eq!(sorted_undecided(&disco), vec![5, 7, 10, 11]);
         assert_eq!(sorted_missing(&disco), vec![6, 9, 12]);
         assert!(!disco.is_complete());
@@ -591,18 +598,24 b' mod tests {'
 
     #[test]
     fn test_limit_sample_no_need_to() {
-        let sample = vec![1, 2, 3, 4];
+        let sample = vec![R!(1), R!(2), R!(3), R!(4)];
         assert_eq!(full_disco().limit_sample(sample, 10), vec![1, 2, 3, 4]);
     }
 
     #[test]
     fn test_limit_sample_less_than_half() {
-        assert_eq!(full_disco().limit_sample((1..6).collect(), 2), vec![2, 5]);
+        assert_eq!(
+            full_disco().limit_sample((1..6).map(Revision).collect(), 2),
+            vec![2, 5]
+        );
     }
 
     #[test]
     fn test_limit_sample_more_than_half() {
-        assert_eq!(full_disco().limit_sample((1..4).collect(), 2), vec![1, 2]);
+        assert_eq!(
+            full_disco().limit_sample((1..4).map(Revision).collect(), 2),
+            vec![1, 2]
+        );
     }
 
     #[test]
@@ -610,7 +623,10 b' mod tests {'
         let mut disco = full_disco();
         disco.randomize = false;
         assert_eq!(
-            disco.limit_sample(vec![1, 8, 13, 5, 7, 3], 4),
+            disco.limit_sample(
+                vec![R!(1), R!(8), R!(13), R!(5), R!(7), R!(3)],
+                4
+            ),
             vec![1, 3, 5, 7]
         );
     }
@@ -618,7 +634,7 b' mod tests {'
     #[test]
     fn test_quick_sample_enough_undecided_heads() -> Result<(), GraphError> {
         let mut disco = full_disco();
-        disco.undecided = Some((1..=13).collect());
+        disco.undecided = Some((1..=13).map(Revision).collect());
 
         let mut sample_vec = disco.take_quick_sample(vec![], 4)?;
         sample_vec.sort_unstable();
@@ -631,7 +647,7 b' mod tests {'
         let mut disco = disco12();
         disco.ensure_undecided()?;
 
-        let mut sample_vec = disco.take_quick_sample(vec![12], 4)?;
+        let mut sample_vec = disco.take_quick_sample(vec![R!(12)], 4)?;
         sample_vec.sort_unstable();
         // r12's only parent is r9, whose unique grand-parent through the
         // diamond shape is r4. This ends there because the distance from r4
@@ -646,16 +662,16 b' mod tests {'
         disco.ensure_children_cache()?;
 
         let cache = disco.children_cache.unwrap();
-        assert_eq!(cache.get(&2).cloned(), Some(vec![4]));
-        assert_eq!(cache.get(&10).cloned(), None);
+        assert_eq!(cache.get(&R!(2)).cloned(), Some(vec![R!(4)]));
+        assert_eq!(cache.get(&R!(10)).cloned(), None);
 
-        let mut children_4 = cache.get(&4).cloned().unwrap();
+        let mut children_4 = cache.get(&R!(4)).cloned().unwrap();
         children_4.sort_unstable();
-        assert_eq!(children_4, vec![5, 6, 7]);
+        assert_eq!(children_4, vec![R!(5), R!(6), R!(7)]);
 
-        let mut children_7 = cache.get(&7).cloned().unwrap();
+        let mut children_7 = cache.get(&R!(7)).cloned().unwrap();
         children_7.sort_unstable();
-        assert_eq!(children_7, vec![9, 11]);
+        assert_eq!(children_7, vec![R!(9), R!(11)]);
 
         Ok(())
     }
@@ -664,14 +680,14 b' mod tests {'
     fn test_complete_sample() {
         let mut disco = full_disco();
         let undecided: HashSet<Revision> =
-            [4, 7, 9, 2, 3].iter().cloned().collect();
+            [4, 7, 9, 2, 3].iter().cloned().map(Revision).collect();
         disco.undecided = Some(undecided);
 
-        let mut sample = vec![0];
+        let mut sample = vec![R!(0)];
         disco.random_complete_sample(&mut sample, 3);
         assert_eq!(sample.len(), 3);
 
-        let mut sample = vec![2, 4, 7];
+        let mut sample = vec![R!(2), R!(4), R!(7)];
         disco.random_complete_sample(&mut sample, 1);
         assert_eq!(sample.len(), 3);
     }
@@ -679,7 +695,7 b' mod tests {'
     #[test]
     fn test_bidirectional_sample() -> Result<(), GraphError> {
         let mut disco = full_disco();
-        disco.undecided = Some((0..=13).into_iter().collect());
+        disco.undecided = Some((0..=13).into_iter().map(Revision).collect());
 
         let (sample_set, size) = disco.bidirectional_sample(7)?;
         assert_eq!(size, 7);

rust/hg-core/src/revlog/index.rs

@@ -215,7 +215,7 b' impl Index {'
         rev: Revision,
         offsets: &[usize],
     ) -> IndexEntry {
-        let start = offsets[rev as usize];
+        let start = offsets[rev.0 as usize];
         let end = start + INDEX_ENTRY_SIZE;
         let bytes = &self.bytes[start..end];
 
@@ -229,13 +229,13 b' impl Index {'
     }
 
     fn get_entry_separated(&self, rev: Revision) -> IndexEntry {
-        let start = rev as usize * INDEX_ENTRY_SIZE;
+        let start = rev.0 as usize * INDEX_ENTRY_SIZE;
         let end = start + INDEX_ENTRY_SIZE;
         let bytes = &self.bytes[start..end];
 
         // Override the offset of the first revision as its bytes are used
         // for the index's metadata (saving space because it is always 0)
-        let offset_override = if rev == 0 { Some(0) } else { None };
+        let offset_override = if rev == Revision(0) { Some(0) } else { None };
 
         IndexEntry {
             bytes,
@@ -359,8 +359,8 b' mod tests {'
                 offset: 0,
                 compressed_len: 0,
                 uncompressed_len: 0,
-                base_revision_or_base_of_delta_chain: 0,
-                link_revision: 0,
+                base_revision_or_base_of_delta_chain: Revision(0),
+                link_revision: Revision(0),
                 p1: NULL_REVISION,
                 p2: NULL_REVISION,
                 node: NULL_NODE,
@@ -450,11 +450,11 b' mod tests {'
             bytes.extend(&(self.compressed_len as u32).to_be_bytes());
             bytes.extend(&(self.uncompressed_len as u32).to_be_bytes());
             bytes.extend(
-                &self.base_revision_or_base_of_delta_chain.to_be_bytes(),
+                &self.base_revision_or_base_of_delta_chain.0.to_be_bytes(),
             );
-            bytes.extend(&self.link_revision.to_be_bytes());
-            bytes.extend(&self.p1.to_be_bytes());
-            bytes.extend(&self.p2.to_be_bytes());
+            bytes.extend(&self.link_revision.0.to_be_bytes());
+            bytes.extend(&self.p1.0.to_be_bytes());
+            bytes.extend(&self.p2.0.to_be_bytes());
             bytes.extend(self.node.as_bytes());
             bytes.extend(vec![0u8; 12]);
             bytes
@@ -564,7 +564,7 b' mod tests {'
     #[test]
     fn test_base_revision_or_base_of_delta_chain() {
         let bytes = IndexEntryBuilder::new()
-            .with_base_revision_or_base_of_delta_chain(1)
+            .with_base_revision_or_base_of_delta_chain(Revision(1))
             .build();
         let entry = IndexEntry {
             bytes: &bytes,
@@ -576,7 +576,9 b' mod tests {'
 
     #[test]
     fn link_revision_test() {
-        let bytes = IndexEntryBuilder::new().with_link_revision(123).build();
+        let bytes = IndexEntryBuilder::new()
+            .with_link_revision(Revision(123))
+            .build();
 
         let entry = IndexEntry {
             bytes: &bytes,
@@ -588,7 +590,7 b' mod tests {'
 
     #[test]
     fn p1_test() {
-        let bytes = IndexEntryBuilder::new().with_p1(123).build();
+        let bytes = IndexEntryBuilder::new().with_p1(Revision(123)).build();
 
         let entry = IndexEntry {
             bytes: &bytes,
@@ -600,7 +602,7 b' mod tests {'
 
     #[test]
     fn p2_test() {
-        let bytes = IndexEntryBuilder::new().with_p2(123).build();
+        let bytes = IndexEntryBuilder::new().with_p2(Revision(123)).build();
 
         let entry = IndexEntry {
             bytes: &bytes,

rust/hg-core/src/revlog/mod.rs

@@ -33,20 +33,14 b' use super::nodemap::{NodeMap, NodeMapErr'
 use crate::errors::HgError;
 use crate::vfs::Vfs;
 
-/// Mercurial revision numbers
-///
 /// As noted in revlog.c, revision numbers are actually encoded in
 /// 4 bytes, and are liberally converted to ints, whence the i32
-pub type Revision = i32;
+pub type BaseRevision = i32;
 
-/// Unchecked Mercurial revision numbers.
-///
-/// Values of this type have no guarantee of being a valid revision number
-/// in any context. Use method `check_revision` to get a valid revision within
-/// the appropriate index object.
-///
-/// As noted in revlog.c, revision numbers are actually encoded in
-/// 4 bytes, and are liberally converted to ints, whence the i32
+/// Mercurial revision numbers
+/// In contrast to the more general [`UncheckedRevision`], these are "checked"
+/// in the sense that they should only be used for revisions that are
+/// valid for a given index (i.e. in bounds).
 #[derive(
     Debug,
     derive_more::Display,
@@ -58,10 +52,52 b' pub type Revision = i32;'
     PartialOrd,
     Ord,
 )]
-pub struct UncheckedRevision(i32);
+pub struct Revision(pub BaseRevision);
+
+impl format_bytes::DisplayBytes for Revision {
+    fn display_bytes(
+        &self,
+        output: &mut dyn std::io::Write,
+    ) -> std::io::Result<()> {
+        self.0.display_bytes(output)
+    }
+}
+
+/// Unchecked Mercurial revision numbers.
+///
+/// Values of this type have no guarantee of being a valid revision number
+/// in any context. Use method `check_revision` to get a valid revision within
+/// the appropriate index object.
+#[derive(
+    Debug,
+    derive_more::Display,
+    Clone,
+    Copy,
+    Hash,
+    PartialEq,
+    Eq,
+    PartialOrd,
+    Ord,
+)]
+pub struct UncheckedRevision(pub BaseRevision);
+
+impl format_bytes::DisplayBytes for UncheckedRevision {
+    fn display_bytes(
+        &self,
+        output: &mut dyn std::io::Write,
+    ) -> std::io::Result<()> {
+        self.0.display_bytes(output)
+    }
+}
 
 impl From<Revision> for UncheckedRevision {
     fn from(value: Revision) -> Self {
+        Self(value.0)
+    }
+}
+
+impl From<BaseRevision> for UncheckedRevision {
+    fn from(value: BaseRevision) -> Self {
         Self(value)
     }
 }
@@ -70,7 +106,7 b' impl From<Revision> for UncheckedRevisio'
 ///
 /// Independently of the actual representation, `NULL_REVISION` is guaranteed
 /// to be smaller than all existing revisions.
-pub const NULL_REVISION: Revision = -1;
+pub const NULL_REVISION: Revision = Revision(-1);
 
 /// Same as `mercurial.node.wdirrev`
 ///
@@ -116,8 +152,9 b' pub trait RevlogIndex {'
     fn check_revision(&self, rev: UncheckedRevision) -> Option<Revision> {
         let rev = rev.0;
 
-        if rev == NULL_REVISION || (rev >= 0 && (rev as usize) < self.len()) {
-            Some(rev)
+        if rev == NULL_REVISION.0 || (rev >= 0 && (rev as usize) < self.len())
+        {
+            Some(Revision(rev))
         } else {
             None
         }
@@ -301,7 +338,8 b' impl Revlog {'
         // TODO: consider building a non-persistent nodemap in memory to
         // optimize these cases.
         let mut found_by_prefix = None;
-        for rev in (0..self.len() as Revision).rev() {
+        for rev in (0..self.len()).rev() {
+            let rev = Revision(rev as BaseRevision);
             let index_entry = self.index.get_entry(rev).ok_or_else(|| {
                 HgError::corrupted(
                     "revlog references a revision not in the index",
@@ -600,7 +638,7 b" impl<'revlog> RevlogEntry<'revlog> {"
                 delta_chain.push(entry);
                 self.revlog.get_entry_for_checked_rev(base_rev)?
             } else {
-                let base_rev = UncheckedRevision(entry.rev - 1);
+                let base_rev = UncheckedRevision(entry.rev.0 - 1);
                 delta_chain.push(entry);
                 self.revlog.get_entry(base_rev)?
             };
@@ -800,8 +838,8 b' mod tests {'
             .build();
         let entry2_bytes = IndexEntryBuilder::new()
             .with_offset(INDEX_ENTRY_SIZE)
-            .with_p1(0)
-            .with_p2(1)
+            .with_p1(Revision(0))
+            .with_p2(Revision(1))
             .with_node(node2)
             .build();
         let contents = vec![entry0_bytes, entry1_bytes, entry2_bytes]
@@ -812,7 +850,7 b' mod tests {'
         let revlog = Revlog::open(&vfs, "foo.i", None, false).unwrap();
 
         let entry0 = revlog.get_entry(0.into()).ok().unwrap();
-        assert_eq!(entry0.revision(), 0);
+        assert_eq!(entry0.revision(), Revision(0));
         assert_eq!(*entry0.node(), node0);
         assert!(!entry0.has_p1());
         assert_eq!(entry0.p1(), None);
@@ -823,7 +861,7 b' mod tests {'
         assert!(p2_entry.is_none());
 
         let entry1 = revlog.get_entry(1.into()).ok().unwrap();
-        assert_eq!(entry1.revision(), 1);
+        assert_eq!(entry1.revision(), Revision(1));
         assert_eq!(*entry1.node(), node1);
         assert!(!entry1.has_p1());
         assert_eq!(entry1.p1(), None);
@@ -834,17 +872,17 b' mod tests {'
         assert!(p2_entry.is_none());
 
         let entry2 = revlog.get_entry(2.into()).ok().unwrap();
-        assert_eq!(entry2.revision(), 2);
+        assert_eq!(entry2.revision(), Revision(2));
         assert_eq!(*entry2.node(), node2);
         assert!(entry2.has_p1());
-        assert_eq!(entry2.p1(), Some(0));
-        assert_eq!(entry2.p2(), Some(1));
+        assert_eq!(entry2.p1(), Some(Revision(0)));
+        assert_eq!(entry2.p2(), Some(Revision(1)));
         let p1_entry = entry2.p1_entry().unwrap();
         assert!(p1_entry.is_some());
-        assert_eq!(p1_entry.unwrap().revision(), 0);
+        assert_eq!(p1_entry.unwrap().revision(), Revision(0));
         let p2_entry = entry2.p2_entry().unwrap();
         assert!(p2_entry.is_some());
-        assert_eq!(p2_entry.unwrap().revision(), 1);
+        assert_eq!(p2_entry.unwrap().revision(), Revision(1));
     }
 
     #[test]
@@ -880,20 +918,23 b' mod tests {'
         // accessing the data shows the corruption
         revlog.get_entry(0.into()).unwrap().data().unwrap_err();
 
-        assert_eq!(revlog.rev_from_node(NULL_NODE.into()).unwrap(), -1);
-        assert_eq!(revlog.rev_from_node(node0.into()).unwrap(), 0);
-        assert_eq!(revlog.rev_from_node(node1.into()).unwrap(), 1);
+        assert_eq!(
+            revlog.rev_from_node(NULL_NODE.into()).unwrap(),
+            Revision(-1)
+        );
+        assert_eq!(revlog.rev_from_node(node0.into()).unwrap(), Revision(0));
+        assert_eq!(revlog.rev_from_node(node1.into()).unwrap(), Revision(1));
         assert_eq!(
             revlog
                 .rev_from_node(NodePrefix::from_hex("000").unwrap())
                 .unwrap(),
-            -1
+            Revision(-1)
         );
         assert_eq!(
             revlog
                 .rev_from_node(NodePrefix::from_hex("b00").unwrap())
                 .unwrap(),
-            1
+            Revision(1)
         );
         // RevlogError does not implement PartialEq
         // (ultimately because io::Error does not)

rust/hg-core/src/revlog/nodemap.rs

@@ -474,9 +474,12 b' impl NodeTree {'
             self.mutable_block(deepest.block_idx);
 
         if let Element::Rev(old_rev) = deepest.element {
-            let old_node = index.node(old_rev).ok_or_else(|| {
-                NodeMapError::RevisionNotInIndex(old_rev.into())
-            })?;
+            let old_node = index
+                .check_revision(old_rev.into())
+                .and_then(|rev| index.node(rev))
+                .ok_or_else(|| {
+                    NodeMapError::RevisionNotInIndex(old_rev.into())
+                })?;
             if old_node == node {
                 return Ok(()); // avoid creating lots of useless blocks
             }
@@ -500,14 +503,14 b' impl NodeTree {'
                 } else {
                     let mut new_block = Block::new();
                     new_block.set(old_nybble, Element::Rev(old_rev));
-                    new_block.set(new_nybble, Element::Rev(rev));
+                    new_block.set(new_nybble, Element::Rev(rev.0));
                     self.growable.push(new_block);
                     break;
                 }
             }
         } else {
             // Free slot in the deepest block: no splitting has to be done
-            block.set(deepest.nybble, Element::Rev(rev));
+            block.set(deepest.nybble, Element::Rev(rev.0));
         }
 
         // Backtrack over visit steps to update references
@@ -707,6 +710,13 b' mod tests {'
         )
     }
 
+    /// Shorthand to reduce boilerplate when creating [`Revision`] for testing
+    macro_rules! R {
+        ($revision:literal) => {
+            Revision($revision)
+        };
+    }
+
     #[test]
     fn test_block_debug() {
         let mut block = Block::new();
@@ -755,7 +765,7 b' mod tests {'
         }
 
         fn check_revision(&self, rev: UncheckedRevision) -> Option<Revision> {
-            self.get(&rev).map(|_| rev.0)
+            self.get(&rev).map(|_| Revision(rev.0))
         }
     }
 
@@ -800,17 +810,20 b' mod tests {'
     #[test]
     fn test_immutable_find_simplest() -> Result<(), NodeMapError> {
         let mut idx: TestIndex = HashMap::new();
-        pad_insert(&mut idx, 1, "1234deadcafe");
+        pad_insert(&mut idx, R!(1), "1234deadcafe");
 
         let nt = NodeTree::from(vec![block! {1: Rev(1)}]);
-        assert_eq!(nt.find_bin(&idx, hex("1"))?, Some(1));
-        assert_eq!(nt.find_bin(&idx, hex("12"))?, Some(1));
-        assert_eq!(nt.find_bin(&idx, hex("1234de"))?, Some(1));
+        assert_eq!(nt.find_bin(&idx, hex("1"))?, Some(R!(1)));
+        assert_eq!(nt.find_bin(&idx, hex("12"))?, Some(R!(1)));
+        assert_eq!(nt.find_bin(&idx, hex("1234de"))?, Some(R!(1)));
         assert_eq!(nt.find_bin(&idx, hex("1a"))?, None);
         assert_eq!(nt.find_bin(&idx, hex("ab"))?, None);
 
         // and with full binary Nodes
-        assert_eq!(nt.find_node(&idx, idx.get(&1.into()).unwrap())?, Some(1));
+        assert_eq!(
+            nt.find_node(&idx, idx.get(&1.into()).unwrap())?,
+            Some(R!(1))
+        );
         let unknown = Node::from_hex(&hex_pad_right("3d")).unwrap();
         assert_eq!(nt.find_node(&idx, &unknown)?, None);
         Ok(())
@@ -819,15 +832,15 b' mod tests {'
     #[test]
     fn test_immutable_find_one_jump() {
         let mut idx = TestIndex::new();
-        pad_insert(&mut idx, 9, "012");
-        pad_insert(&mut idx, 0, "00a");
+        pad_insert(&mut idx, R!(9), "012");
+        pad_insert(&mut idx, R!(0), "00a");
 
         let nt = sample_nodetree();
 
         assert_eq!(nt.find_bin(&idx, hex("0")), Err(MultipleResults));
-        assert_eq!(nt.find_bin(&idx, hex("01")), Ok(Some(9)));
+        assert_eq!(nt.find_bin(&idx, hex("01")), Ok(Some(R!(9))));
         assert_eq!(nt.find_bin(&idx, hex("00")), Err(MultipleResults));
-        assert_eq!(nt.find_bin(&idx, hex("00a")), Ok(Some(0)));
+        assert_eq!(nt.find_bin(&idx, hex("00a")), Ok(Some(R!(0))));
         assert_eq!(nt.unique_prefix_len_bin(&idx, hex("00a")), Ok(Some(3)));
         assert_eq!(nt.find_bin(&idx, hex("000")), Ok(Some(NULL_REVISION)));
     }
@@ -835,11 +848,11 b' mod tests {'
     #[test]
     fn test_mutated_find() -> Result<(), NodeMapError> {
         let mut idx = TestIndex::new();
-        pad_insert(&mut idx, 9, "012");
-        pad_insert(&mut idx, 0, "00a");
-        pad_insert(&mut idx, 2, "cafe");
-        pad_insert(&mut idx, 3, "15");
-        pad_insert(&mut idx, 1, "10");
+        pad_insert(&mut idx, R!(9), "012");
+        pad_insert(&mut idx, R!(0), "00a");
+        pad_insert(&mut idx, R!(2), "cafe");
+        pad_insert(&mut idx, R!(3), "15");
+        pad_insert(&mut idx, R!(1), "10");
 
         let nt = NodeTree {
             readonly: sample_nodetree().readonly,
@@ -847,13 +860,13 b' mod tests {'
             root: block![0: Block(1), 1:Block(3), 12: Rev(2)],
             masked_inner_blocks: 1,
         };
-        assert_eq!(nt.find_bin(&idx, hex("10"))?, Some(1));
-        assert_eq!(nt.find_bin(&idx, hex("c"))?, Some(2));
+        assert_eq!(nt.find_bin(&idx, hex("10"))?, Some(R!(1)));
+        assert_eq!(nt.find_bin(&idx, hex("c"))?, Some(R!(2)));
         assert_eq!(nt.unique_prefix_len_bin(&idx, hex("c"))?, Some(1));
         assert_eq!(nt.find_bin(&idx, hex("00")), Err(MultipleResults));
         assert_eq!(nt.find_bin(&idx, hex("000"))?, Some(NULL_REVISION));
         assert_eq!(nt.unique_prefix_len_bin(&idx, hex("000"))?, Some(3));
-        assert_eq!(nt.find_bin(&idx, hex("01"))?, Some(9));
+        assert_eq!(nt.find_bin(&idx, hex("01"))?, Some(R!(9)));
         assert_eq!(nt.masked_readonly_blocks(), 2);
         Ok(())
     }
@@ -915,34 +928,34 b' mod tests {'
     fn test_insert_full_mutable() -> Result<(), NodeMapError> {
         let mut idx = TestNtIndex::new();
         idx.insert(0, "1234")?;
-        assert_eq!(idx.find_hex("1")?, Some(0));
-        assert_eq!(idx.find_hex("12")?, Some(0));
+        assert_eq!(idx.find_hex("1")?, Some(R!(0)));
+        assert_eq!(idx.find_hex("12")?, Some(R!(0)));
 
         // let's trigger a simple split
         idx.insert(1, "1a34")?;
         assert_eq!(idx.nt.growable.len(), 1);
-        assert_eq!(idx.find_hex("12")?, Some(0));
-        assert_eq!(idx.find_hex("1a")?, Some(1));
+        assert_eq!(idx.find_hex("12")?, Some(R!(0)));
+        assert_eq!(idx.find_hex("1a")?, Some(R!(1)));
 
         // reinserting is a no_op
         idx.insert(1, "1a34")?;
         assert_eq!(idx.nt.growable.len(), 1);
-        assert_eq!(idx.find_hex("12")?, Some(0));
-        assert_eq!(idx.find_hex("1a")?, Some(1));
+        assert_eq!(idx.find_hex("12")?, Some(R!(0)));
+        assert_eq!(idx.find_hex("1a")?, Some(R!(1)));
 
         idx.insert(2, "1a01")?;
         assert_eq!(idx.nt.growable.len(), 2);
         assert_eq!(idx.find_hex("1a"), Err(NodeMapError::MultipleResults));
-        assert_eq!(idx.find_hex("12")?, Some(0));
-        assert_eq!(idx.find_hex("1a3")?, Some(1));
-        assert_eq!(idx.find_hex("1a0")?, Some(2));
+        assert_eq!(idx.find_hex("12")?, Some(R!(0)));
+        assert_eq!(idx.find_hex("1a3")?, Some(R!(1)));
+        assert_eq!(idx.find_hex("1a0")?, Some(R!(2)));
         assert_eq!(idx.find_hex("1a12")?, None);
 
         // now let's make it split and create more than one additional block
         idx.insert(3, "1a345")?;
         assert_eq!(idx.nt.growable.len(), 4);
-        assert_eq!(idx.find_hex("1a340")?, Some(1));
-        assert_eq!(idx.find_hex("1a345")?, Some(3));
+        assert_eq!(idx.find_hex("1a340")?, Some(R!(1)));
+        assert_eq!(idx.find_hex("1a345")?, Some(R!(3)));
         assert_eq!(idx.find_hex("1a341")?, None);
 
         // there's no readonly block to mask
@@ -987,12 +1000,12 b' mod tests {'
         let node1 = Node::from_hex(&node1_hex).unwrap();
 
         idx.insert(0.into(), node0);
-        nt.insert(idx, &node0, 0)?;
+        nt.insert(idx, &node0, R!(0))?;
         idx.insert(1.into(), node1);
-        nt.insert(idx, &node1, 1)?;
+        nt.insert(idx, &node1, R!(1))?;
 
-        assert_eq!(nt.find_bin(idx, (&node0).into())?, Some(0));
-        assert_eq!(nt.find_bin(idx, (&node1).into())?, Some(1));
+        assert_eq!(nt.find_bin(idx, (&node0).into())?, Some(R!(0)));
+        assert_eq!(nt.find_bin(idx, (&node1).into())?, Some(R!(1)));
         Ok(())
     }
 
@@ -1004,28 +1017,28 b' mod tests {'
         idx.insert(2, "131")?;
         idx.insert(3, "cafe")?;
         let mut idx = idx.commit();
-        assert_eq!(idx.find_hex("1234")?, Some(0));
-        assert_eq!(idx.find_hex("1235")?, Some(1));
-        assert_eq!(idx.find_hex("131")?, Some(2));
-        assert_eq!(idx.find_hex("cafe")?, Some(3));
+        assert_eq!(idx.find_hex("1234")?, Some(R!(0)));
+        assert_eq!(idx.find_hex("1235")?, Some(R!(1)));
+        assert_eq!(idx.find_hex("131")?, Some(R!(2)));
+        assert_eq!(idx.find_hex("cafe")?, Some(R!(3)));
         // we did not add anything since init from readonly
         assert_eq!(idx.nt.masked_readonly_blocks(), 0);
 
         idx.insert(4, "123A")?;
-        assert_eq!(idx.find_hex("1234")?, Some(0));
-        assert_eq!(idx.find_hex("1235")?, Some(1));
-        assert_eq!(idx.find_hex("131")?, Some(2));
-        assert_eq!(idx.find_hex("cafe")?, Some(3));
-        assert_eq!(idx.find_hex("123A")?, Some(4));
+        assert_eq!(idx.find_hex("1234")?, Some(R!(0)));
+        assert_eq!(idx.find_hex("1235")?, Some(R!(1)));
+        assert_eq!(idx.find_hex("131")?, Some(R!(2)));
+        assert_eq!(idx.find_hex("cafe")?, Some(R!(3)));
+        assert_eq!(idx.find_hex("123A")?, Some(R!(4)));
         // we masked blocks for all prefixes of "123", including the root
         assert_eq!(idx.nt.masked_readonly_blocks(), 4);
 
         eprintln!("{:?}", idx.nt);
         idx.insert(5, "c0")?;
-        assert_eq!(idx.find_hex("cafe")?, Some(3));
-        assert_eq!(idx.find_hex("c0")?, Some(5));
+        assert_eq!(idx.find_hex("cafe")?, Some(R!(3)));
+        assert_eq!(idx.find_hex("c0")?, Some(R!(5)));
         assert_eq!(idx.find_hex("c1")?, None);
-        assert_eq!(idx.find_hex("1234")?, Some(0));
+        assert_eq!(idx.find_hex("1234")?, Some(R!(0)));
         // inserting "c0" is just splitting the 'c' slot of the mutable root,
         // it doesn't mask anything
         assert_eq!(idx.nt.masked_readonly_blocks(), 4);

rust/hg-core/src/revset.rs

@@ -55,7 +55,9 b' pub fn resolve_rev_number_or_hex_prefix('
             && integer >= 0
             && revlog.has_rev(integer.into())
         {
-            return Ok(integer);
+            // This is fine because we've just checked that the revision is
+            // valid for the given revlog.
+            return Ok(Revision(integer));
         }
     }
     if let Ok(prefix) = NodePrefix::from_hex(input) {

rust/hg-core/src/testing.rs

@@ -41,22 +41,27 b' pub struct SampleGraph;'
 
 impl Graph for SampleGraph {
     fn parents(&self, rev: Revision) -> Result<[Revision; 2], GraphError> {
-        match rev {
-            0 => Ok([NULL_REVISION, NULL_REVISION]),
-            1 => Ok([0, NULL_REVISION]),
-            2 => Ok([1, NULL_REVISION]),
-            3 => Ok([1, NULL_REVISION]),
-            4 => Ok([2, NULL_REVISION]),
-            5 => Ok([4, NULL_REVISION]),
-            6 => Ok([4, NULL_REVISION]),
-            7 => Ok([4, NULL_REVISION]),
-            8 => Ok([NULL_REVISION, NULL_REVISION]),
+        let null_rev = NULL_REVISION.0;
+        let res = match rev.0 {
+            0 => Ok([null_rev, null_rev]),
+            1 => Ok([0, null_rev]),
+            2 => Ok([1, null_rev]),
+            3 => Ok([1, null_rev]),
+            4 => Ok([2, null_rev]),
+            5 => Ok([4, null_rev]),
+            6 => Ok([4, null_rev]),
+            7 => Ok([4, null_rev]),
+            8 => Ok([null_rev, null_rev]),
             9 => Ok([6, 7]),
-            10 => Ok([5, NULL_REVISION]),
+            10 => Ok([5, null_rev]),
             11 => Ok([3, 7]),
-            12 => Ok([9, NULL_REVISION]),
-            13 => Ok([8, NULL_REVISION]),
-            r => Err(GraphError::ParentOutOfRange(r)),
+            12 => Ok([9, null_rev]),
+            13 => Ok([8, null_rev]),
+            r => Err(GraphError::ParentOutOfRange(Revision(r))),
+        };
+        match res {
+            Ok([a, b]) => Ok([Revision(a), Revision(b)]),
+            Err(e) => Err(e),
         }
     }
 }
@@ -67,6 +72,6 b' pub type VecGraph = Vec<[Revision; 2]>;'
 
 impl Graph for VecGraph {
     fn parents(&self, rev: Revision) -> Result<[Revision; 2], GraphError> {
-        Ok(self[rev as usize])
+        Ok(self[rev.0 as usize])
     }
 }

rust/hg-core/tests/test_missing_ancestors.rs

@@ -26,25 +26,28 b' fn build_random_graph('
         if i == 0 || rng.gen_bool(rootprob) {
             vg.push([NULL_REVISION, NULL_REVISION])
         } else if i == 1 {
-            vg.push([0, NULL_REVISION])
+            vg.push([Revision(0), NULL_REVISION])
         } else if rng.gen_bool(mergeprob) {
             let p1 = {
                 if i == 2 || rng.gen_bool(prevprob) {
-                    (i - 1) as Revision
+                    Revision((i - 1) as BaseRevision)
                 } else {
-                    rng.gen_range(0..i - 1) as Revision
+                    Revision(rng.gen_range(0..i - 1) as BaseRevision)
                 }
             };
             // p2 is a random revision lower than i and different from p1
-            let mut p2 = rng.gen_range(0..i - 1) as Revision;
+            let mut p2 = Revision(rng.gen_range(0..i - 1) as BaseRevision);
             if p2 >= p1 {
-                p2 += 1;
+                p2.0 += 1;
             }
             vg.push([p1, p2]);
         } else if rng.gen_bool(prevprob) {
-            vg.push([(i - 1) as Revision, NULL_REVISION])
+            vg.push([Revision((i - 1) as BaseRevision), NULL_REVISION])
         } else {
-            vg.push([rng.gen_range(0..i - 1) as Revision, NULL_REVISION])
+            vg.push([
+                Revision(rng.gen_range(0..i - 1) as BaseRevision),
+                NULL_REVISION,
+            ])
         }
     }
     vg
@@ -55,10 +58,10 b' fn ancestors_sets(vg: &VecGraph) -> Vec<'
     let mut ancs: Vec<HashSet<Revision>> = Vec::new();
     (0..vg.len()).for_each(|i| {
         let mut ancs_i = HashSet::new();
-        ancs_i.insert(i as Revision);
+        ancs_i.insert(Revision(i as BaseRevision));
         for p in vg[i].iter().cloned() {
             if p != NULL_REVISION {
-                ancs_i.extend(&ancs[p as usize]);
+                ancs_i.extend(&ancs[p.0 as usize]);
             }
         }
         ancs.push(ancs_i);
@@ -115,7 +118,7 b" impl<'a> NaiveMissingAncestors<'a> {"
             .push(MissingAncestorsAction::RemoveAncestorsFrom(revs.clone()));
         for base in self.bases.iter().cloned() {
             if base != NULL_REVISION {
-                for rev in &self.ancestors_sets[base as usize] {
+                for rev in &self.ancestors_sets[base.0 as usize] {
                     revs.remove(rev);
                 }
             }
@@ -131,7 +134,7 b" impl<'a> NaiveMissingAncestors<'a> {"
         let mut missing: HashSet<Revision> = HashSet::new();
         for rev in revs_as_set.iter().cloned() {
             if rev != NULL_REVISION {
-                missing.extend(&self.ancestors_sets[rev as usize])
+                missing.extend(&self.ancestors_sets[rev.0 as usize])
             }
         }
         self.history
@@ -139,7 +142,7 b" impl<'a> NaiveMissingAncestors<'a> {"
 
         for base in self.bases.iter().cloned() {
             if base != NULL_REVISION {
-                for rev in &self.ancestors_sets[base as usize] {
+                for rev in &self.ancestors_sets[base.0 as usize] {
                     missing.remove(rev);
                 }
             }
@@ -193,10 +196,10 b' fn sample_revs<R: RngCore>('
     let sigma = sigma_opt.unwrap_or(0.8);
 
     let log_normal = LogNormal::new(mu, sigma).unwrap();
-    let nb = min(maxrev as usize, log_normal.sample(rng).floor() as usize);
+    let nb = min(maxrev.0 as usize, log_normal.sample(rng).floor() as usize);
 
-    let dist = Uniform::from(NULL_REVISION..maxrev);
-    rng.sample_iter(&dist).take(nb).collect()
+    let dist = Uniform::from(NULL_REVISION.0..maxrev.0);
+    rng.sample_iter(&dist).take(nb).map(Revision).collect()
 }
 
 /// Produces the hexadecimal representation of a slice of bytes
@@ -294,7 +297,7 b' fn test_missing_ancestors_compare_naive('
             eprintln!("Tested with {} graphs", g);
         }
         let graph = build_random_graph(None, None, None, None);
-        let graph_len = graph.len() as Revision;
+        let graph_len = Revision(graph.len() as BaseRevision);
         let ancestors_sets = ancestors_sets(&graph);
         for _testno in 0..testcount {
             let bases: HashSet<Revision> =

rust/hg-cpython/src/ancestors.rs

@@ -35,6 +35,7 b''
 //! [`MissingAncestors`]: struct.MissingAncestors.html
 //! [`AncestorsIterator`]: struct.AncestorsIterator.html
 use crate::revlog::pyindex_to_graph;
+use crate::PyRevision;
 use crate::{
     cindex::Index, conversion::rev_pyiter_collect, exceptions::GraphError,
 };
@@ -54,16 +55,16 b' use vcsgraph::lazy_ancestors::{'
 py_class!(pub class AncestorsIterator |py| {
     data inner: RefCell<Box<VCGAncestorsIterator<Index>>>;
 
-    def __next__(&self) -> PyResult<Option<Revision>> {
+    def __next__(&self) -> PyResult<Option<PyRevision>> {
         match self.inner(py).borrow_mut().next() {
             Some(Err(e)) => Err(GraphError::pynew_from_vcsgraph(py, e)),
             None => Ok(None),
-            Some(Ok(r)) => Ok(Some(r)),
+            Some(Ok(r)) => Ok(Some(PyRevision(r))),
         }
     }
 
-    def __contains__(&self, rev: Revision) -> PyResult<bool> {
-        self.inner(py).borrow_mut().contains(rev)
+    def __contains__(&self, rev: PyRevision) -> PyResult<bool> {
+        self.inner(py).borrow_mut().contains(rev.0)
             .map_err(|e| GraphError::pynew_from_vcsgraph(py, e))
     }
 
@@ -71,13 +72,19 b' py_class!(pub class AncestorsIterator |p'
         Ok(self.clone_ref(py))
     }
 
-    def __new__(_cls, index: PyObject, initrevs: PyObject, stoprev: Revision,
-                inclusive: bool) -> PyResult<AncestorsIterator> {
-        let initvec: Vec<Revision> = rev_pyiter_collect(py, &initrevs)?;
+    def __new__(
+        _cls,
+        index: PyObject,
+        initrevs: PyObject,
+        stoprev: PyRevision,
+        inclusive: bool
+    ) -> PyResult<AncestorsIterator> {
+        let index = pyindex_to_graph(py, index)?;
+        let initvec: Vec<_> = rev_pyiter_collect(py, &initrevs, &index)?;
         let ait = VCGAncestorsIterator::new(
-            pyindex_to_graph(py, index)?,
-            initvec,
-            stoprev,
+            index,
+            initvec.into_iter().map(|r| r.0),
+            stoprev.0,
             inclusive,
         )
         .map_err(|e| GraphError::pynew_from_vcsgraph(py, e))?;
@@ -98,10 +105,10 b' impl AncestorsIterator {'
 py_class!(pub class LazyAncestors |py| {
     data inner: RefCell<Box<VCGLazyAncestors<Index>>>;
 
-    def __contains__(&self, rev: Revision) -> PyResult<bool> {
+    def __contains__(&self, rev: PyRevision) -> PyResult<bool> {
         self.inner(py)
             .borrow_mut()
-            .contains(rev)
+            .contains(rev.0)
             .map_err(|e| GraphError::pynew_from_vcsgraph(py, e))
     }
 
@@ -113,14 +120,24 b' py_class!(pub class LazyAncestors |py| {'
         Ok(!self.inner(py).borrow().is_empty())
     }
 
-    def __new__(_cls, index: PyObject, initrevs: PyObject, stoprev: Revision,
-                inclusive: bool) -> PyResult<Self> {
-        let initvec: Vec<Revision> = rev_pyiter_collect(py, &initrevs)?;
+    def __new__(
+        _cls,
+        index: PyObject,
+        initrevs: PyObject,
+        stoprev: PyRevision,
+        inclusive: bool
+    ) -> PyResult<Self> {
+        let index = pyindex_to_graph(py, index)?;
+        let initvec: Vec<_> = rev_pyiter_collect(py, &initrevs, &index)?;
 
         let lazy =
-            VCGLazyAncestors::new(pyindex_to_graph(py, index)?,
-                          initvec, stoprev, inclusive)
-                .map_err(|e| GraphError::pynew_from_vcsgraph(py, e))?;
+            VCGLazyAncestors::new(
+                index,
+                initvec.into_iter().map(|r| r.0),
+                stoprev.0,
+                inclusive
+            )
+            .map_err(|e| GraphError::pynew_from_vcsgraph(py, e))?;
 
         Self::create_instance(py, RefCell::new(Box::new(lazy)))
         }
@@ -129,6 +146,7 b' py_class!(pub class LazyAncestors |py| {'
 
 py_class!(pub class MissingAncestors |py| {
     data inner: RefCell<Box<CoreMissing<Index>>>;
+    data index: RefCell<Index>;
 
     def __new__(
         _cls,
@@ -136,9 +154,15 b' py_class!(pub class MissingAncestors |py'
         bases: PyObject
     )
     -> PyResult<MissingAncestors> {
-        let bases_vec: Vec<Revision> = rev_pyiter_collect(py, &bases)?;
-        let inner = CoreMissing::new(pyindex_to_graph(py, index)?, bases_vec);
-        MissingAncestors::create_instance(py, RefCell::new(Box::new(inner)))
+        let index = pyindex_to_graph(py, index)?;
+        let bases_vec: Vec<_> = rev_pyiter_collect(py, &bases, &index)?;
+
+        let inner = CoreMissing::new(index.clone_ref(py), bases_vec);
+        MissingAncestors::create_instance(
+            py,
+            RefCell::new(Box::new(inner)),
+            RefCell::new(index)
+        )
     }
 
     def hasbases(&self) -> PyResult<bool> {
@@ -146,8 +170,9 b' py_class!(pub class MissingAncestors |py'
     }
 
     def addbases(&self, bases: PyObject) -> PyResult<PyObject> {
+        let index = self.index(py).borrow();
+        let bases_vec: Vec<_> = rev_pyiter_collect(py, &bases, &*index)?;
         let mut inner = self.inner(py).borrow_mut();
-        let bases_vec: Vec<Revision> = rev_pyiter_collect(py, &bases)?;
         inner.add_bases(bases_vec);
         // cpython doc has examples with PyResult<()> but this gives me
         //   the trait `cpython::ToPyObject` is not implemented for `()`
@@ -155,17 +180,31 b' py_class!(pub class MissingAncestors |py'
         Ok(py.None())
     }
 
-    def bases(&self) -> PyResult<HashSet<Revision>> {
-        Ok(self.inner(py).borrow().get_bases().clone())
+    def bases(&self) -> PyResult<HashSet<PyRevision>> {
+        Ok(
+            self.inner(py)
+                .borrow()
+                .get_bases()
+                .iter()
+                .map(|r| PyRevision(r.0))
+                .collect()
+        )
     }
 
-    def basesheads(&self) -> PyResult<HashSet<Revision>> {
+    def basesheads(&self) -> PyResult<HashSet<PyRevision>> {
         let inner = self.inner(py).borrow();
-        inner.bases_heads().map_err(|e| GraphError::pynew(py, e))
+        Ok(
+            inner
+                .bases_heads()
+                .map_err(|e| GraphError::pynew(py, e))?
+                .into_iter()
+                .map(|r| PyRevision(r.0))
+                .collect()
+        )
     }
 
     def removeancestorsfrom(&self, revs: PyObject) -> PyResult<PyObject> {
-        let mut inner = self.inner(py).borrow_mut();
+        let index = self.index(py).borrow();
         // this is very lame: we convert to a Rust set, update it in place
         // and then convert back to Python, only to have Python remove the
         // excess (thankfully, Python is happy with a list or even an iterator)
@@ -174,7 +213,10 b' py_class!(pub class MissingAncestors |py'
         //    discard
         //  - define a trait for sets of revisions in the core and implement
         //    it for a Python set rewrapped with the GIL marker
-        let mut revs_pyset: HashSet<Revision> = rev_pyiter_collect(py, &revs)?;
+        let mut revs_pyset: HashSet<Revision> = rev_pyiter_collect(
+            py, &revs, &*index
+        )?;
+        let mut inner = self.inner(py).borrow_mut();
         inner.remove_ancestors_from(&mut revs_pyset)
             .map_err(|e| GraphError::pynew(py, e))?;
 
@@ -182,15 +224,19 b' py_class!(pub class MissingAncestors |py'
         let mut remaining_pyint_vec: Vec<PyObject> = Vec::with_capacity(
             revs_pyset.len());
         for rev in revs_pyset {
-            remaining_pyint_vec.push(rev.to_py_object(py).into_object());
+            remaining_pyint_vec.push(
+                PyRevision(rev.0).to_py_object(py).into_object()
+            );
         }
         let remaining_pylist = PyList::new(py, remaining_pyint_vec.as_slice());
         revs.call_method(py, "intersection_update", (remaining_pylist, ), None)
     }
 
     def missingancestors(&self, revs: PyObject) -> PyResult<PyList> {
+        let index = self.index(py).borrow();
+        let revs_vec: Vec<Revision> = rev_pyiter_collect(py, &revs, &*index)?;
+
         let mut inner = self.inner(py).borrow_mut();
-        let revs_vec: Vec<Revision> = rev_pyiter_collect(py, &revs)?;
         let missing_vec = match inner.missing_ancestors(revs_vec) {
             Ok(missing) => missing,
             Err(e) => {
@@ -201,7 +247,9 b' py_class!(pub class MissingAncestors |py'
         let mut missing_pyint_vec: Vec<PyObject> = Vec::with_capacity(
             missing_vec.len());
         for rev in missing_vec {
-            missing_pyint_vec.push(rev.to_py_object(py).into_object());
+            missing_pyint_vec.push(
+                PyRevision(rev.0).to_py_object(py).into_object()
+            );
         }
         Ok(PyList::new(py, missing_pyint_vec.as_slice()))
     }

rust/hg-cpython/src/cindex.rs

@@ -15,7 +15,7 b' use cpython::{'
     PyObject, PyResult, PyTuple, Python, PythonObject,
 };
 use hg::revlog::{Node, RevlogIndex};
-use hg::{Graph, GraphError, Revision};
+use hg::{BaseRevision, Graph, GraphError, Revision};
 use libc::{c_int, ssize_t};
 
 const REVLOG_CABI_VERSION: c_int = 3;
@@ -145,12 +145,12 b' impl Graph for Index {'
         let code = unsafe {
             (self.capi.index_parents)(
                 self.index.as_ptr(),
-                rev as c_int,
+                rev.0 as c_int,
                 &mut res as *mut [c_int; 2],
             )
         };
         match code {
-            0 => Ok(res),
+            0 => Ok([Revision(res[0]), Revision(res[1])]),
             _ => Err(GraphError::ParentOutOfRange(rev)),
         }
     }
@@ -159,13 +159,17 b' impl Graph for Index {'
 impl vcsgraph::graph::Graph for Index {
     fn parents(
         &self,
-        rev: Revision,
+        rev: BaseRevision,
     ) -> Result<vcsgraph::graph::Parents, vcsgraph::graph::GraphReadError>
     {
-        match Graph::parents(self, rev) {
-            Ok(parents) => Ok(vcsgraph::graph::Parents(parents)),
+        // FIXME This trait should be reworked to decide between Revision
+        // and UncheckedRevision, get better errors names, etc.
+        match Graph::parents(self, Revision(rev)) {
+            Ok(parents) => {
+                Ok(vcsgraph::graph::Parents([parents[0].0, parents[1].0]))
+            }
             Err(GraphError::ParentOutOfRange(rev)) => {
-                Err(vcsgraph::graph::GraphReadError::KeyedInvalidKey(rev))
+                Err(vcsgraph::graph::GraphReadError::KeyedInvalidKey(rev.0))
             }
         }
     }
@@ -174,7 +178,7 b' impl vcsgraph::graph::Graph for Index {'
 impl vcsgraph::graph::RankedGraph for Index {
     fn rank(
         &self,
-        rev: Revision,
+        rev: BaseRevision,
     ) -> Result<vcsgraph::graph::Rank, vcsgraph::graph::GraphReadError> {
         match unsafe {
             (self.capi.fast_rank)(self.index.as_ptr(), rev as ssize_t)
@@ -194,7 +198,7 b' impl RevlogIndex for Index {'
 
     fn node(&self, rev: Revision) -> Option<&Node> {
         let raw = unsafe {
-            (self.capi.index_node)(self.index.as_ptr(), rev as ssize_t)
+            (self.capi.index_node)(self.index.as_ptr(), rev.0 as ssize_t)
         };
         if raw.is_null() {
             None

rust/hg-cpython/src/conversion.rs

@@ -8,8 +8,10 b''
 //! Bindings for the hg::ancestors module provided by the
 //! `hg-core` crate. From Python, this will be seen as `rustext.ancestor`
 
-use cpython::{ObjectProtocol, PyObject, PyResult, Python};
-use hg::Revision;
+use cpython::{ObjectProtocol, PyErr, PyObject, PyResult, Python};
+use hg::{Revision, RevlogIndex, UncheckedRevision};
+
+use crate::{exceptions::GraphError, PyRevision};
 
 /// Utility function to convert a Python iterable into various collections
 ///
@@ -17,11 +19,28 b' use hg::Revision;'
 /// with `impl IntoIterator<Item=Revision>` arguments, because
 /// a `PyErr` can arise at each step of iteration, whereas these methods
 /// expect iterables over `Revision`, not over some `Result<Revision, PyErr>`
-pub fn rev_pyiter_collect<C>(py: Python, revs: &PyObject) -> PyResult<C>
+pub fn rev_pyiter_collect<C, I>(
+    py: Python,
+    revs: &PyObject,
+    index: &I,
+) -> PyResult<C>
 where
     C: FromIterator<Revision>,
+    I: RevlogIndex,
 {
     revs.iter(py)?
-        .map(|r| r.and_then(|o| o.extract::<Revision>(py)))
+        .map(|r| {
+            r.and_then(|o| match o.extract::<PyRevision>(py) {
+                Ok(r) => index
+                    .check_revision(UncheckedRevision(r.0))
+                    .ok_or_else(|| {
+                        PyErr::new::<GraphError, _>(
+                            py,
+                            ("InvalidRevision", r.0),
+                        )
+                    }),
+                Err(e) => Err(e),
+            })
+        })
         .collect()
 }

rust/hg-cpython/src/copy_tracing.rs

@@ -14,6 +14,7 b' use hg::copy_tracing::CombineChangesetCo'
 use hg::Revision;
 
 use crate::pybytes_deref::PyBytesDeref;
+use crate::PyRevision;
 
 /// Combines copies information contained into revision `revs` to build a copy
 /// map.
@@ -23,14 +24,17 b' pub fn combine_changeset_copies_wrapper('
     py: Python,
     revs: PyList,
     children_count: PyDict,
-    target_rev: Revision,
+    target_rev: PyRevision,
     rev_info: PyObject,
     multi_thread: bool,
 ) -> PyResult<PyDict> {
+    let target_rev = Revision(target_rev.0);
     let children_count = children_count
         .items(py)
         .iter()
-        .map(|(k, v)| Ok((k.extract(py)?, v.extract(py)?)))
+        .map(|(k, v)| {
+            Ok((Revision(k.extract::<PyRevision>(py)?.0), v.extract(py)?))
+        })
         .collect::<PyResult<_>>()?;
 
     /// (Revision number, parent 1, parent 2, copy data for this revision)
@@ -38,11 +42,13 b' pub fn combine_changeset_copies_wrapper('
 
     let revs_info =
         revs.iter(py).map(|rev_py| -> PyResult<RevInfo<PyBytes>> {
-            let rev = rev_py.extract(py)?;
+            let rev = Revision(rev_py.extract::<PyRevision>(py)?.0);
             let tuple: PyTuple =
                 rev_info.call(py, (rev_py,), None)?.cast_into(py)?;
-            let p1 = tuple.get_item(py, 0).extract(py)?;
-            let p2 = tuple.get_item(py, 1).extract(py)?;
+            let p1 =
+                Revision(tuple.get_item(py, 0).extract::<PyRevision>(py)?.0);
+            let p2 =
+                Revision(tuple.get_item(py, 1).extract::<PyRevision>(py)?.0);
             let opt_bytes = tuple.get_item(py, 2).extract(py)?;
             Ok((rev, p1, p2, opt_bytes))
         });
@@ -179,7 +185,7 b' pub fn init_module(py: Python, package: '
             combine_changeset_copies_wrapper(
                 revs: PyList,
                 children: PyDict,
-                target_rev: Revision,
+                target_rev: PyRevision,
                 rev_info: PyObject,
                 multi_thread: bool
             )

rust/hg-cpython/src/dagops.rs

@@ -9,6 +9,7 b''
 //! `hg-core` package.
 //!
 //! From Python, this will be seen as `mercurial.rustext.dagop`
+use crate::PyRevision;
 use crate::{conversion::rev_pyiter_collect, exceptions::GraphError};
 use cpython::{PyDict, PyModule, PyObject, PyResult, Python};
 use hg::dagops;
@@ -26,11 +27,12 b' pub fn headrevs('
     py: Python,
     index: PyObject,
     revs: PyObject,
-) -> PyResult<HashSet<Revision>> {
-    let mut as_set: HashSet<Revision> = rev_pyiter_collect(py, &revs)?;
-    dagops::retain_heads(&pyindex_to_graph(py, index)?, &mut as_set)
+) -> PyResult<HashSet<PyRevision>> {
+    let index = pyindex_to_graph(py, index)?;
+    let mut as_set: HashSet<Revision> = rev_pyiter_collect(py, &revs, &index)?;
+    dagops::retain_heads(&index, &mut as_set)
         .map_err(|e| GraphError::pynew(py, e))?;
-    Ok(as_set)
+    Ok(as_set.into_iter().map(Into::into).collect())
 }
 
 /// Computes the rank, i.e. the number of ancestors including itself,
@@ -38,10 +40,10 b' pub fn headrevs('
 pub fn rank(
     py: Python,
     index: PyObject,
-    p1r: Revision,
-    p2r: Revision,
+    p1r: PyRevision,
+    p2r: PyRevision,
 ) -> PyResult<Rank> {
-    node_rank(&pyindex_to_graph(py, index)?, &Parents([p1r, p2r]))
+    node_rank(&pyindex_to_graph(py, index)?, &Parents([p1r.0, p2r.0]))
         .map_err(|e| GraphError::pynew_from_vcsgraph(py, e))
 }
 
@@ -59,7 +61,7 b' pub fn init_module(py: Python, package: '
     m.add(
         py,
         "rank",
-        py_fn!(py, rank(index: PyObject, p1r: Revision, p2r: Revision)),
+        py_fn!(py, rank(index: PyObject, p1r: PyRevision, p2r: PyRevision)),
     )?;
 
     let sys = PyModule::import(py, "sys")?;

rust/hg-cpython/src/discovery.rs

@@ -12,12 +12,13 b''
 //! - [`PartialDiscover`] is the Rust implementation of
 //!   `mercurial.setdiscovery.partialdiscovery`.
 
+use crate::PyRevision;
 use crate::{
     cindex::Index, conversion::rev_pyiter_collect, exceptions::GraphError,
 };
 use cpython::{
-    ObjectProtocol, PyDict, PyModule, PyObject, PyResult, PyTuple, Python,
-    PythonObject, ToPyObject,
+    ObjectProtocol, PyClone, PyDict, PyModule, PyObject, PyResult, PyTuple,
+    Python, PythonObject, ToPyObject,
 };
 use hg::discovery::PartialDiscovery as CorePartialDiscovery;
 use hg::Revision;
@@ -29,6 +30,7 b' use crate::revlog::pyindex_to_graph;'
 
 py_class!(pub class PartialDiscovery |py| {
     data inner: RefCell<Box<CorePartialDiscovery<Index>>>;
+    data index: RefCell<Index>;
 
     // `_respectsize` is currently only here to replicate the Python API and
     // will be used in future patches inside methods that are yet to be
@@ -41,28 +43,33 b' py_class!(pub class PartialDiscovery |py'
         randomize: bool = true
     ) -> PyResult<PartialDiscovery> {
         let index = repo.getattr(py, "changelog")?.getattr(py, "index")?;
+        let index = pyindex_to_graph(py, index)?;
+        let target_heads = rev_pyiter_collect(py, &targetheads, &index)?;
         Self::create_instance(
             py,
             RefCell::new(Box::new(CorePartialDiscovery::new(
-                pyindex_to_graph(py, index)?,
-                rev_pyiter_collect(py, &targetheads)?,
+                index.clone_ref(py),
+                target_heads,
                 respectsize,
                 randomize,
-            )))
+            ))),
+            RefCell::new(index),
         )
     }
 
     def addcommons(&self, commons: PyObject) -> PyResult<PyObject> {
+        let index = self.index(py).borrow();
+        let commons_vec: Vec<_> = rev_pyiter_collect(py, &commons, &*index)?;
         let mut inner = self.inner(py).borrow_mut();
-        let commons_vec: Vec<Revision> = rev_pyiter_collect(py, &commons)?;
         inner.add_common_revisions(commons_vec)
-            .map_err(|e| GraphError::pynew(py, e))?;
-        Ok(py.None())
-    }
+        .map_err(|e| GraphError::pynew(py, e))?;
+    Ok(py.None())
+}
 
     def addmissings(&self, missings: PyObject) -> PyResult<PyObject> {
+        let index = self.index(py).borrow();
+        let missings_vec: Vec<_> = rev_pyiter_collect(py, &missings, &*index)?;
         let mut inner = self.inner(py).borrow_mut();
-        let missings_vec: Vec<Revision> = rev_pyiter_collect(py, &missings)?;
         inner.add_missing_revisions(missings_vec)
             .map_err(|e| GraphError::pynew(py, e))?;
         Ok(py.None())
@@ -73,7 +80,10 b' py_class!(pub class PartialDiscovery |py'
         let mut common: Vec<Revision> = Vec::new();
         for info in sample.iter(py)? { // info is a pair (Revision, bool)
             let mut revknown = info?.iter(py)?;
-            let rev: Revision = revknown.next().unwrap()?.extract(py)?;
+            let rev: PyRevision = revknown.next().unwrap()?.extract(py)?;
+            // This is fine since we're just using revisions as integers
+            // for the purposes of discovery
+            let rev = Revision(rev.0);
             let known: bool = revknown.next().unwrap()?.extract(py)?;
             if known {
                 common.push(rev);
@@ -107,9 +117,10 b' py_class!(pub class PartialDiscovery |py'
         Ok(as_dict)
     }
 
-    def commonheads(&self) -> PyResult<HashSet<Revision>> {
-        self.inner(py).borrow().common_heads()
-            .map_err(|e| GraphError::pynew(py, e))
+    def commonheads(&self) -> PyResult<HashSet<PyRevision>> {
+        let res = self.inner(py).borrow().common_heads()
+                    .map_err(|e| GraphError::pynew(py, e))?;
+        Ok(res.into_iter().map(Into::into).collect())
     }
 
     def takefullsample(&self, _headrevs: PyObject,
@@ -119,20 +130,21 b' py_class!(pub class PartialDiscovery |py'
             .map_err(|e| GraphError::pynew(py, e))?;
         let as_vec: Vec<PyObject> = sample
             .iter()
-            .map(|rev| rev.to_py_object(py).into_object())
+            .map(|rev| PyRevision(rev.0).to_py_object(py).into_object())
             .collect();
         Ok(PyTuple::new(py, as_vec.as_slice()).into_object())
     }
 
     def takequicksample(&self, headrevs: PyObject,
                         size: usize) -> PyResult<PyObject> {
+        let index = self.index(py).borrow();
         let mut inner = self.inner(py).borrow_mut();
-        let revsvec: Vec<Revision> = rev_pyiter_collect(py, &headrevs)?;
+        let revsvec: Vec<_> = rev_pyiter_collect(py, &headrevs, &*index)?;
         let sample = inner.take_quick_sample(revsvec, size)
             .map_err(|e| GraphError::pynew(py, e))?;
         let as_vec: Vec<PyObject> = sample
             .iter()
-            .map(|rev| rev.to_py_object(py).into_object())
+            .map(|rev| PyRevision(rev.0).to_py_object(py).into_object())
             .collect();
         Ok(PyTuple::new(py, as_vec.as_slice()).into_object())
     }

rust/hg-cpython/src/exceptions.rs

@@ -18,13 +18,15 b' use cpython::{'
 };
 use hg;
 
+use crate::PyRevision;
+
 py_exception!(rustext, GraphError, ValueError);
 
 impl GraphError {
     pub fn pynew(py: Python, inner: hg::GraphError) -> PyErr {
         match inner {
             hg::GraphError::ParentOutOfRange(r) => {
-                GraphError::new(py, ("ParentOutOfRange", r))
+                GraphError::new(py, ("ParentOutOfRange", PyRevision(r.0)))
             }
         }
     }

rust/hg-cpython/src/lib.rs

@@ -24,6 +24,9 b''
 #![allow(clippy::manual_strip)] // rust-cpython macros
 #![allow(clippy::type_complexity)] // rust-cpython macros
 
+use cpython::{FromPyObject, PyInt, Python, ToPyObject};
+use hg::{BaseRevision, Revision};
+
 /// This crate uses nested private macros, `extern crate` is still needed in
 /// 2018 edition.
 #[macro_use]
@@ -44,6 +47,40 b' mod pybytes_deref;'
 pub mod revlog;
 pub mod utils;
 
+/// Revision as exposed to/from the Python layer.
+///
+/// We need this indirection because of the orphan rule, meaning we can't
+/// implement a foreign trait (like [`cpython::ToPyObject`])
+/// for a foreign type (like [`hg::UncheckedRevision`]).
+///
+/// This also acts as a deterrent against blindly trusting Python to send
+/// us valid revision numbers.
+#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub struct PyRevision(BaseRevision);
+
+impl From<Revision> for PyRevision {
+    fn from(r: Revision) -> Self {
+        PyRevision(r.0)
+    }
+}
+
+impl<'s> FromPyObject<'s> for PyRevision {
+    fn extract(
+        py: Python,
+        obj: &'s cpython::PyObject,
+    ) -> cpython::PyResult<Self> {
+        Ok(Self(obj.extract::<BaseRevision>(py)?))
+    }
+}
+
+impl ToPyObject for PyRevision {
+    type ObjectType = PyInt;
+
+    fn to_py_object(&self, py: Python) -> Self::ObjectType {
+        self.0.to_py_object(py)
+    }
+}
+
 py_module_initializer!(rustext, initrustext, PyInit_rustext, |py, m| {
     m.add(
         py,

rust/hg-cpython/src/revlog.rs

@@ -8,6 +8,7 b''
 use crate::{
     cindex,
     utils::{node_from_py_bytes, node_from_py_object},
+    PyRevision,
 };
 use cpython::{
     buffer::{Element, PyBuffer},
@@ -18,7 +19,7 b' use cpython::{'
 use hg::{
     nodemap::{Block, NodeMapError, NodeTree},
     revlog::{nodemap::NodeMap, NodePrefix, RevlogIndex},
-    Revision, UncheckedRevision,
+    BaseRevision, Revision, UncheckedRevision,
 };
 use std::cell::RefCell;
 
@@ -59,12 +60,13 b' py_class!(pub class MixedIndex |py| {'
 
     /// Return Revision if found, raises a bare `error.RevlogError`
     /// in case of ambiguity, same as C version does
-    def get_rev(&self, node: PyBytes) -> PyResult<Option<Revision>> {
+    def get_rev(&self, node: PyBytes) -> PyResult<Option<PyRevision>> {
         let opt = self.get_nodetree(py)?.borrow();
         let nt = opt.as_ref().unwrap();
         let idx = &*self.cindex(py).borrow();
         let node = node_from_py_bytes(py, &node)?;
-        nt.find_bin(idx, node.into()).map_err(|e| nodemap_error(py, e))
+        let res = nt.find_bin(idx, node.into());
+        Ok(res.map_err(|e| nodemap_error(py, e))?.map(Into::into))
     }
 
     /// same as `get_rev()` but raises a bare `error.RevlogError` if node
@@ -72,7 +74,7 b' py_class!(pub class MixedIndex |py| {'
     ///
     /// No need to repeat `node` in the exception, `mercurial/revlog.py`
     /// will catch and rewrap with it
-    def rev(&self, node: PyBytes) -> PyResult<Revision> {
+    def rev(&self, node: PyBytes) -> PyResult<PyRevision> {
         self.get_rev(py, node)?.ok_or_else(|| revlog_error(py))
     }
 
@@ -131,9 +133,11 b' py_class!(pub class MixedIndex |py| {'
         let node = node_from_py_object(py, &node_bytes)?;
 
         let mut idx = self.cindex(py).borrow_mut();
-        let rev = idx.len() as Revision;
 
+        // This is ok since we will just add the revision to the index
+        let rev = Revision(idx.len() as BaseRevision);
         idx.append(py, tup)?;
+
         self.get_nodetree(py)?.borrow_mut().as_mut().unwrap()
             .insert(&*idx, &node, rev)
             .map_err(|e| nodemap_error(py, e))?;
@@ -270,7 +274,7 b' py_class!(pub class MixedIndex |py| {'
         let cindex = self.cindex(py).borrow();
         match item.extract::<i32>(py) {
             Ok(rev) => {
-                Ok(rev >= -1 && rev < cindex.inner().len(py)? as Revision)
+                Ok(rev >= -1 && rev < cindex.inner().len(py)? as BaseRevision)
             }
             Err(_) => {
                 cindex.inner().call_method(
@@ -331,7 +335,7 b' impl MixedIndex {'
     ) -> PyResult<PyObject> {
         let index = self.cindex(py).borrow();
         for r in 0..index.len() {
-            let rev = r as Revision;
+            let rev = Revision(r as BaseRevision);
             // in this case node() won't ever return None
             nt.insert(&*index, index.node(rev).unwrap(), rev)
                 .map_err(|e| nodemap_error(py, e))?
@@ -447,8 +451,10 b' impl MixedIndex {'
 
         let mut nt = NodeTree::load_bytes(Box::new(bytes), len);
 
-        let data_tip =
-            docket.getattr(py, "tip_rev")?.extract::<i32>(py)?.into();
+        let data_tip = docket
+            .getattr(py, "tip_rev")?
+            .extract::<BaseRevision>(py)?
+            .into();
         self.docket(py).borrow_mut().replace(docket.clone_ref(py));
         let idx = self.cindex(py).borrow();
         let data_tip = idx.check_revision(data_tip).ok_or_else(|| {
@@ -456,8 +462,8 b' impl MixedIndex {'
         })?;
         let current_tip = idx.len();
 
-        for r in (data_tip + 1)..current_tip as Revision {
-            let rev = r as Revision;
+        for r in (data_tip.0 + 1)..current_tip as BaseRevision {
+            let rev = Revision(r);
             // in this case node() won't ever return None
             nt.insert(&*idx, idx.node(rev).unwrap(), rev)
                 .map_err(|e| nodemap_error(py, e))?

tests/test-rust-ancestor.py

@@ -2,7 +2,6 b' import sys'
 import unittest
 
 from mercurial.node import wdirrev
-from mercurial import error
 
 from mercurial.testing import revlog as revlogtesting
 
@@ -144,11 +143,15 b' class rustancestorstest(revlogtesting.Re'
 
     def testwdirunsupported(self):
         # trying to access ancestors of the working directory raises
-        # WdirUnsupported directly
         idx = self.parseindex()
-        with self.assertRaises(error.WdirUnsupported):
+        with self.assertRaises(rustext.GraphError) as arc:
             list(AncestorsIterator(idx, [wdirrev], -1, False))
 
+        exc = arc.exception
+        self.assertIsInstance(exc, ValueError)
+        # rust-cpython issues appropriate str instances for Python 2 and 3
+        self.assertEqual(exc.args, ('InvalidRevision', wdirrev))
+
     def testheadrevs(self):
         idx = self.parseindex()
         self.assertEqual(dagop.headrevs(idx, [1, 2, 3]), {3})