Mercurial > hg
changeset 50979:4c5f6e95df84
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
| author | Raphaël Gomès <rgomes@octobus.net> |
|---|---|
| date | Fri, 18 Aug 2023 14:34:29 +0200 |
| parents | 27e773aa607d |
| children | d7b2701f17fa |
| files | rust/hg-core/examples/nodemap/index.rs rust/hg-core/examples/nodemap/main.rs rust/hg-core/src/ancestors.rs rust/hg-core/src/copy_tracing/tests.rs rust/hg-core/src/dagops.rs rust/hg-core/src/discovery.rs rust/hg-core/src/revlog/index.rs rust/hg-core/src/revlog/mod.rs rust/hg-core/src/revlog/nodemap.rs rust/hg-core/src/revset.rs rust/hg-core/src/testing.rs rust/hg-core/tests/test_missing_ancestors.rs rust/hg-cpython/src/ancestors.rs rust/hg-cpython/src/cindex.rs rust/hg-cpython/src/conversion.rs rust/hg-cpython/src/copy_tracing.rs rust/hg-cpython/src/dagops.rs rust/hg-cpython/src/discovery.rs rust/hg-cpython/src/exceptions.rs rust/hg-cpython/src/lib.rs rust/hg-cpython/src/revlog.rs tests/test-rust-ancestor.py |
| diffstat | 22 files changed, 804 insertions(+), 417 deletions(-) [+] |
line wrap: on
line diff
--- a/rust/hg-core/examples/nodemap/index.rs Thu Aug 10 11:01:07 2023 +0200 +++ b/rust/hg-core/examples/nodemap/index.rs Fri Aug 18 14:34:29 2023 +0200 @@ -29,7 +29,7 @@ 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 @@ 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)); }
--- a/rust/hg-core/examples/nodemap/main.rs Thu Aug 10 11:01:07 2023 +0200 +++ b/rust/hg-core/examples/nodemap/main.rs Fri Aug 18 14:34:29 2023 +0200 @@ -36,7 +36,7 @@ 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 @@ 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> =
--- a/rust/hg-core/src/ancestors.rs Thu Aug 10 11:01:07 2023 +0200 +++ b/rust/hg-core/src/ancestors.rs Fri Aug 18 14:34:29 2023 +0200 @@ -247,7 +247,9 @@ 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 @@ // 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 @@ 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 @@ 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 @@ 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 @@ /// 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 @@ /// 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 @@ #[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 @@ 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 @@ #[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 @@ 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 @@ } 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 @@ } 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 @@ #[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 @@ 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));
--- a/rust/hg-core/src/copy_tracing/tests.rs Thu Aug 10 11:01:07 2023 +0200 +++ b/rust/hg-core/src/copy_tracing/tests.rs Fri Aug 18 14:34:29 2023 +0200 @@ -1,5 +1,12 @@ 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 @@ 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 @@ 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 @@ 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") );
--- a/rust/hg-core/src/dagops.rs Thu Aug 10 11:01:07 2023 +0200 +++ b/rust/hg-core/src/dagops.rs Fri Aug 18 14:34:29 2023 +0200 @@ -171,14 +171,15 @@ 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 @@ /// 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 @@ /// 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 @@ /// 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]
--- a/rust/hg-core/src/discovery.rs Thu Aug 10 11:01:07 2023 +0200 +++ b/rust/hg-core/src/discovery.rs Fri Aug 18 14:34:29 2023 +0200 @@ -481,6 +481,13 @@ 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 @@ 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 @@ fn disco12() -> PartialDiscovery<SampleGraph> { PartialDiscovery::new_with_seed( SampleGraph, - vec![12], + vec![R!(12)], [0; 16], true, true, @@ -540,7 +547,7 @@ 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 @@ #[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 @@ 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 @@ #[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 @@ 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 @@ #[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 @@ 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 @@ 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 @@ 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 @@ #[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);
--- a/rust/hg-core/src/revlog/index.rs Thu Aug 10 11:01:07 2023 +0200 +++ b/rust/hg-core/src/revlog/index.rs Fri Aug 18 14:34:29 2023 +0200 @@ -215,7 +215,7 @@ 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 @@ } 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 @@ 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 @@ 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 @@ #[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 @@ #[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 @@ #[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 @@ #[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,
--- a/rust/hg-core/src/revlog/mod.rs Thu Aug 10 11:01:07 2023 +0200 +++ b/rust/hg-core/src/revlog/mod.rs Fri Aug 18 14:34:29 2023 +0200 @@ -33,20 +33,14 @@ 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 @@ 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 @@ /// /// 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 @@ 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 @@ // 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 @@ 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 @@ .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 @@ 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 @@ 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 @@ 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 @@ // 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)
--- a/rust/hg-core/src/revlog/nodemap.rs Thu Aug 10 11:01:07 2023 +0200 +++ b/rust/hg-core/src/revlog/nodemap.rs Fri Aug 18 14:34:29 2023 +0200 @@ -474,9 +474,12 @@ 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 @@ } 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 @@ ) } + /// 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 @@ } 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 @@ #[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 @@ #[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 @@ #[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 @@ 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 @@ 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 @@ 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 @@ 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);
--- a/rust/hg-core/src/revset.rs Thu Aug 10 11:01:07 2023 +0200 +++ b/rust/hg-core/src/revset.rs Fri Aug 18 14:34:29 2023 +0200 @@ -55,7 +55,9 @@ && 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) {
--- a/rust/hg-core/src/testing.rs Thu Aug 10 11:01:07 2023 +0200 +++ b/rust/hg-core/src/testing.rs Fri Aug 18 14:34:29 2023 +0200 @@ -41,22 +41,27 @@ 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 @@ impl Graph for VecGraph { fn parents(&self, rev: Revision) -> Result<[Revision; 2], GraphError> { - Ok(self[rev as usize]) + Ok(self[rev.0 as usize]) } }
--- a/rust/hg-core/tests/test_missing_ancestors.rs Thu Aug 10 11:01:07 2023 +0200 +++ b/rust/hg-core/tests/test_missing_ancestors.rs Fri Aug 18 14:34:29 2023 +0200 @@ -26,25 +26,28 @@ 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 @@ 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 @@ .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 @@ 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 @@ 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 @@ 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 @@ 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> =
--- a/rust/hg-cpython/src/ancestors.rs Thu Aug 10 11:01:07 2023 +0200 +++ b/rust/hg-cpython/src/ancestors.rs Fri Aug 18 14:34:29 2023 +0200 @@ -35,6 +35,7 @@ //! [`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 @@ 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 @@ 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 @@ 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 @@ 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 @@ py_class!(pub class MissingAncestors |py| { data inner: RefCell<Box<CoreMissing<Index>>>; + data index: RefCell<Index>; def __new__( _cls, @@ -136,9 +154,15 @@ 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 @@ } 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 @@ 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 @@ // 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 @@ 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 @@ 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())) }
--- a/rust/hg-cpython/src/cindex.rs Thu Aug 10 11:01:07 2023 +0200 +++ b/rust/hg-cpython/src/cindex.rs Fri Aug 18 14:34:29 2023 +0200 @@ -15,7 +15,7 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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
--- a/rust/hg-cpython/src/conversion.rs Thu Aug 10 11:01:07 2023 +0200 +++ b/rust/hg-cpython/src/conversion.rs Fri Aug 18 14:34:29 2023 +0200 @@ -8,8 +8,10 @@ //! 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 @@ /// 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() }
--- a/rust/hg-cpython/src/copy_tracing.rs Thu Aug 10 11:01:07 2023 +0200 +++ b/rust/hg-cpython/src/copy_tracing.rs Fri Aug 18 14:34:29 2023 +0200 @@ -14,6 +14,7 @@ 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 @@ 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 @@ 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 @@ combine_changeset_copies_wrapper( revs: PyList, children: PyDict, - target_rev: Revision, + target_rev: PyRevision, rev_info: PyObject, multi_thread: bool )
--- a/rust/hg-cpython/src/dagops.rs Thu Aug 10 11:01:07 2023 +0200 +++ b/rust/hg-cpython/src/dagops.rs Fri Aug 18 14:34:29 2023 +0200 @@ -9,6 +9,7 @@ //! `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 @@ 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 @@ 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 @@ 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")?;
--- a/rust/hg-cpython/src/discovery.rs Thu Aug 10 11:01:07 2023 +0200 +++ b/rust/hg-cpython/src/discovery.rs Fri Aug 18 14:34:29 2023 +0200 @@ -12,12 +12,13 @@ //! - [`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 @@ 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 @@ 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 @@ 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 @@ 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 @@ .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()) }
--- a/rust/hg-cpython/src/exceptions.rs Thu Aug 10 11:01:07 2023 +0200 +++ b/rust/hg-cpython/src/exceptions.rs Fri Aug 18 14:34:29 2023 +0200 @@ -18,13 +18,15 @@ }; 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))) } } }
--- a/rust/hg-cpython/src/lib.rs Thu Aug 10 11:01:07 2023 +0200 +++ b/rust/hg-cpython/src/lib.rs Fri Aug 18 14:34:29 2023 +0200 @@ -24,6 +24,9 @@ #![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 @@ 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,
--- a/rust/hg-cpython/src/revlog.rs Thu Aug 10 11:01:07 2023 +0200 +++ b/rust/hg-cpython/src/revlog.rs Fri Aug 18 14:34:29 2023 +0200 @@ -8,6 +8,7 @@ use crate::{ cindex, utils::{node_from_py_bytes, node_from_py_object}, + PyRevision, }; use cpython::{ buffer::{Element, PyBuffer}, @@ -18,7 +19,7 @@ use hg::{ nodemap::{Block, NodeMapError, NodeTree}, revlog::{nodemap::NodeMap, NodePrefix, RevlogIndex}, - Revision, UncheckedRevision, + BaseRevision, Revision, UncheckedRevision, }; use std::cell::RefCell; @@ -59,12 +60,13 @@ /// 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 @@ /// /// 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 @@ 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 @@ 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 @@ ) -> 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 @@ 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 @@ })?; 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))?
--- a/tests/test-rust-ancestor.py Thu Aug 10 11:01:07 2023 +0200 +++ b/tests/test-rust-ancestor.py Fri Aug 18 14:34:29 2023 +0200 @@ -2,7 +2,6 @@ import unittest from mercurial.node import wdirrev -from mercurial import error from mercurial.testing import revlog as revlogtesting @@ -144,11 +143,15 @@ 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})