From 7861968f648839e8a5b598b237c6cb9109057dde Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Mon, 12 Apr 2021 22:47:21 -0700 Subject: [PATCH] index: make IndexRef::entry_by_id() etc return entry with repo's lifetime It's useful to be able to know that given a `repo: RepoRef<'a>`, the the lifetime of `repo.index().entry_by_id()` will also be `'a`. --- lib/src/index.rs | 30 ++++++++++++++++--------- lib/src/repo.rs | 15 ++++++++----- lib/tests/test_index.rs | 49 ++++++++++++++++++++++------------------- 3 files changed, 55 insertions(+), 39 deletions(-) diff --git a/lib/src/index.rs b/lib/src/index.rs index f100661d0..3f7bf5488 100644 --- a/lib/src/index.rs +++ b/lib/src/index.rs @@ -33,12 +33,12 @@ use crate::store::{ChangeId, CommitId}; #[derive(Clone)] pub enum IndexRef<'a> { - Readonly(Arc), + Readonly(&'a ReadonlyIndex), Mutable(&'a MutableIndex), } -impl From> for IndexRef<'_> { - fn from(index: Arc) -> Self { +impl<'a> From<&'a ReadonlyIndex> for IndexRef<'a> { + fn from(index: &'a ReadonlyIndex) -> Self { IndexRef::Readonly(index) } } @@ -78,14 +78,14 @@ impl<'a> IndexRef<'a> { } } - pub fn entry_by_id(&self, commit_id: &CommitId) -> Option { + pub fn entry_by_id(&self, commit_id: &CommitId) -> Option> { match self { IndexRef::Readonly(index) => index.entry_by_id(commit_id), IndexRef::Mutable(index) => index.entry_by_id(commit_id), } } - pub fn entry_by_pos(&self, pos: u32) -> IndexEntry { + pub fn entry_by_pos(&self, pos: u32) -> IndexEntry<'a> { match self { IndexRef::Readonly(index) => index.entry_by_pos(pos), IndexRef::Mutable(index) => index.entry_by_pos(pos), @@ -113,7 +113,7 @@ impl<'a> IndexRef<'a> { } } - pub fn walk_revs(&self, wanted: &[CommitId], unwanted: &[CommitId]) -> RevWalk { + pub fn walk_revs(&self, wanted: &[CommitId], unwanted: &[CommitId]) -> RevWalk<'a> { match self { IndexRef::Readonly(index) => index.walk_revs(wanted, unwanted), IndexRef::Mutable(index) => index.walk_revs(wanted, unwanted), @@ -772,7 +772,7 @@ impl<'a> CompositeIndex<'a> { self.0 .segment_parent_file() .as_ref() - .and_then(|file| IndexRef::Readonly(file.clone()).commit_id_to_pos(commit_id)) + .and_then(|file| IndexRef::Readonly(file).commit_id_to_pos(commit_id)) }) } @@ -1564,8 +1564,10 @@ mod tests { fn index_empty(on_disk: bool) { let temp_dir = tempfile::tempdir().unwrap(); let index = MutableIndex::full(3); + let mut _saved_index = None; let index = if on_disk { - IndexRef::Readonly(index.save_in(temp_dir.path().to_owned()).unwrap()) + _saved_index = Some(index.save_in(temp_dir.path().to_owned()).unwrap()); + IndexRef::Readonly(_saved_index.as_ref().unwrap()) } else { IndexRef::Mutable(&index) }; @@ -1593,8 +1595,10 @@ mod tests { let id_0 = CommitId::from_hex("000000"); let change_id0 = new_change_id(); index.add_commit_data(id_0.clone(), change_id0.clone(), false, vec![], vec![]); + let mut _saved_index = None; let index = if on_disk { - IndexRef::Readonly(index.save_in(temp_dir.path().to_owned()).unwrap()) + _saved_index = Some(index.save_in(temp_dir.path().to_owned()).unwrap()); + IndexRef::Readonly(_saved_index.as_ref().unwrap()) } else { IndexRef::Mutable(&index) }; @@ -1715,8 +1719,10 @@ mod tests { vec![id_4.clone(), id_2.clone()], vec![], ); + let mut _saved_index = None; let index = if on_disk { - IndexRef::Readonly(index.save_in(temp_dir.path().to_owned()).unwrap()) + _saved_index = Some(index.save_in(temp_dir.path().to_owned()).unwrap()); + IndexRef::Readonly(_saved_index.as_ref().unwrap()) } else { IndexRef::Mutable(&index) }; @@ -1839,8 +1845,10 @@ mod tests { vec![id_1, id_2, id_3, id_4, id_5], vec![], ); + let mut _saved_index = None; let index = if on_disk { - IndexRef::Readonly(index.save_in(temp_dir.path().to_owned()).unwrap()) + _saved_index = Some(index.save_in(temp_dir.path().to_owned()).unwrap()); + IndexRef::Readonly(_saved_index.as_ref().unwrap()) } else { IndexRef::Mutable(&index) }; diff --git a/lib/src/repo.rs b/lib/src/repo.rs index d3a01439d..800b8e22a 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -84,7 +84,7 @@ impl<'a> RepoRef<'a> { } } - pub fn index(&self) -> IndexRef { + pub fn index(&self) -> IndexRef<'a> { match self { RepoRef::Readonly(repo) => IndexRef::Readonly(repo.index()), RepoRef::Mutable(repo) => IndexRef::Mutable(repo.index()), @@ -298,7 +298,7 @@ impl ReadonlyRepo { locked_evolution.as_ref().unwrap().clone() } - pub fn index(&self) -> Arc { + pub fn index(&self) -> &Arc { let mut locked_index = self.index.lock().unwrap(); if locked_index.is_none() { let op_id = self.op_id.clone(); @@ -306,10 +306,15 @@ impl ReadonlyRepo { let op = Operation::new(self.op_store.clone(), op_id, op); locked_index.replace(self.index_store.get_index_at_op(&op, self.store.as_ref())); } - locked_index.as_ref().unwrap().clone() + let index: &Arc = locked_index.as_ref().unwrap(); + // Extend lifetime from that of mutex lock to that of self. Safe since we never + // change value once it's been set (except in `reindex()` but that + // requires a mutable reference). + let index: &Arc = unsafe { std::mem::transmute(index) }; + index } - pub fn reindex(&mut self) -> Arc { + pub fn reindex(&mut self) -> &Arc { self.index_store.reinit(); { let mut locked_index = self.index.lock().unwrap(); @@ -350,7 +355,7 @@ impl ReadonlyRepo { let locked_evolution = self.evolution.lock().unwrap(); let mut_repo = MutableRepo::new( self.clone(), - self.index(), + self.index().clone(), &self.view, locked_evolution.as_ref(), ); diff --git a/lib/tests/test_index.rs b/lib/tests/test_index.rs index 6d87eff2e..3de1680dd 100644 --- a/lib/tests/test_index.rs +++ b/lib/tests/test_index.rs @@ -50,11 +50,14 @@ fn test_index_commits_empty_repo(use_git: bool) { // Check the generation numbers of the root and the working copy assert_eq!( - generation_number(index.clone(), repo.store().root_commit_id()), + generation_number(index.as_ref(), repo.store().root_commit_id()), 0 ); assert_eq!( - generation_number(index, &repo.working_copy_locked().current_commit_id()), + generation_number( + index.as_ref(), + &repo.working_copy_locked().current_commit_id() + ), 1 ); } @@ -105,16 +108,16 @@ fn test_index_commits_standard_cases(use_git: bool) { assert_eq!(stats.num_merges, 1); assert_eq!(stats.max_generation_number, 6); - assert_eq!(generation_number(index.clone(), root_commit.id()), 0); - assert_eq!(generation_number(index.clone(), wc_commit.id()), 1); - assert_eq!(generation_number(index.clone(), commit_a.id()), 1); - assert_eq!(generation_number(index.clone(), commit_b.id()), 2); - assert_eq!(generation_number(index.clone(), commit_c.id()), 2); - assert_eq!(generation_number(index.clone(), commit_d.id()), 3); - assert_eq!(generation_number(index.clone(), commit_e.id()), 4); - assert_eq!(generation_number(index.clone(), commit_f.id()), 5); - assert_eq!(generation_number(index.clone(), commit_g.id()), 6); - assert_eq!(generation_number(index.clone(), commit_h.id()), 5); + assert_eq!(generation_number(index.as_ref(), root_commit.id()), 0); + assert_eq!(generation_number(index.as_ref(), wc_commit.id()), 1); + assert_eq!(generation_number(index.as_ref(), commit_a.id()), 1); + assert_eq!(generation_number(index.as_ref(), commit_b.id()), 2); + assert_eq!(generation_number(index.as_ref(), commit_c.id()), 2); + assert_eq!(generation_number(index.as_ref(), commit_d.id()), 3); + assert_eq!(generation_number(index.as_ref(), commit_e.id()), 4); + assert_eq!(generation_number(index.as_ref(), commit_f.id()), 5); + assert_eq!(generation_number(index.as_ref(), commit_g.id()), 6); + assert_eq!(generation_number(index.as_ref(), commit_h.id()), 5); assert!(index.is_ancestor(root_commit.id(), commit_a.id())); assert!(!index.is_ancestor(commit_a.id(), root_commit.id())); @@ -181,11 +184,11 @@ fn test_index_commits_criss_cross(use_git: bool) { // Check generation numbers for gen in 0..num_generations { assert_eq!( - generation_number(index.clone(), left_commits[gen].id()), + generation_number(index.as_ref(), left_commits[gen].id()), (gen as u32) + 1 ); assert_eq!( - generation_number(index.clone(), right_commits[gen].id()), + generation_number(index.as_ref(), right_commits[gen].id()), (gen as u32) + 1 ); } @@ -291,9 +294,9 @@ fn test_index_commits_previous_operations(use_git: bool) { assert_eq!(stats.num_merges, 0); assert_eq!(stats.max_generation_number, 3); - assert_eq!(generation_number(index.clone(), commit_a.id()), 1); - assert_eq!(generation_number(index.clone(), commit_b.id()), 2); - assert_eq!(generation_number(index, commit_c.id()), 3); + assert_eq!(generation_number(index.as_ref(), commit_a.id()), 1); + assert_eq!(generation_number(index.as_ref(), commit_b.id()), 2); + assert_eq!(generation_number(index.as_ref(), commit_c.id()), 3); } #[test_case(false ; "local store")] @@ -339,10 +342,10 @@ fn test_index_commits_incremental(use_git: bool) { assert_eq!(stats.levels.len(), 1); assert_eq!(stats.levels[0].num_commits, 5); - assert_eq!(generation_number(index.clone(), root_commit.id()), 0); - assert_eq!(generation_number(index.clone(), commit_a.id()), 1); - assert_eq!(generation_number(index.clone(), commit_b.id()), 2); - assert_eq!(generation_number(index, commit_c.id()), 3); + assert_eq!(generation_number(index.as_ref(), root_commit.id()), 0); + assert_eq!(generation_number(index.as_ref(), commit_a.id()), 1); + assert_eq!(generation_number(index.as_ref(), commit_b.id()), 2); + assert_eq!(generation_number(index.as_ref(), commit_c.id()), 3); } #[test_case(false ; "local store")] @@ -385,8 +388,8 @@ fn test_index_commits_incremental_empty_transaction(use_git: bool) { assert_eq!(stats.levels[1].num_commits, 1); assert_ne!(stats.levels[1].name, stats.levels[0].name); - assert_eq!(generation_number(index.clone(), root_commit.id()), 0); - assert_eq!(generation_number(index, commit_a.id()), 1); + assert_eq!(generation_number(index.as_ref(), root_commit.id()), 0); + assert_eq!(generation_number(index.as_ref(), commit_a.id()), 1); } #[test_case(false ; "local store")]