From 3fc35288c04e54c451afeb0019fc94a389da752a Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Tue, 2 Mar 2021 23:03:49 -0800 Subject: [PATCH] index: remove dir field from ReadonlyIndex and MutableIndex --- lib/src/index.rs | 66 +++++++++++++++--------------------------- lib/src/index_store.rs | 8 +++-- lib/src/transaction.rs | 9 +++++- 3 files changed, 38 insertions(+), 45 deletions(-) diff --git a/lib/src/index.rs b/lib/src/index.rs index 2497ae5b0..461dd04d4 100644 --- a/lib/src/index.rs +++ b/lib/src/index.rs @@ -230,7 +230,6 @@ impl CommitLookupEntry<'_> { // ids // TODO: add a fanout table like git's commit graph has? pub struct ReadonlyIndex { - dir: PathBuf, parent_file: Option>, num_parent_commits: u32, name: String, @@ -318,7 +317,6 @@ struct MutableGraphEntry { } pub struct MutableIndex { - dir: PathBuf, parent_file: Option>, num_parent_commits: u32, hash_length: usize, @@ -327,9 +325,8 @@ pub struct MutableIndex { } impl MutableIndex { - pub(crate) fn full(dir: PathBuf, hash_length: usize) -> Self { + pub(crate) fn full(hash_length: usize) -> Self { Self { - dir, parent_file: None, num_parent_commits: 0, hash_length, @@ -342,7 +339,6 @@ impl MutableIndex { let num_parent_commits = parent_file.num_parent_commits + parent_file.num_local_commits; let hash_length = parent_file.hash_length; Self { - dir: parent_file.dir.clone(), parent_file: Some(parent_file), num_parent_commits, hash_length, @@ -517,7 +513,7 @@ impl MutableIndex { break; } if parent_file.parent_file.is_none() { - squashed = MutableIndex::full(self.dir.clone(), self.hash_length); + squashed = MutableIndex::full(self.hash_length); break; } num_new_commits += parent_file.segment_num_commits(); @@ -549,13 +545,12 @@ impl MutableIndex { squashed } - pub fn save(self) -> io::Result> { + pub fn save_in(self, dir: PathBuf) -> io::Result> { if self.segment_num_commits() == 0 && self.parent_file.is_some() { return Ok(self.parent_file.unwrap()); } let hash_length = self.hash_length; - let dir = self.dir.clone(); let buf = self.maybe_squash_with_ancestors().serialize(); let mut hasher = Blake2b::new(); @@ -1296,12 +1291,8 @@ impl ReadonlyIndex { let parent_filename = String::from_utf8(parent_filename_bytes).unwrap(); let parent_file_path = dir.join(&parent_filename); let mut index_file = File::open(&parent_file_path).unwrap(); - let parent_file = ReadonlyIndex::load_from( - &mut index_file, - dir.clone(), - parent_filename, - hash_length, - )?; + let parent_file = + ReadonlyIndex::load_from(&mut index_file, dir, parent_filename, hash_length)?; num_parent_commits = parent_file.num_parent_commits + parent_file.num_local_commits; maybe_parent_file = Some(parent_file); } else { @@ -1327,7 +1318,6 @@ impl ReadonlyIndex { let lookup = data.split_off(graph_size); let graph = data; Ok(Arc::new(ReadonlyIndex { - dir, parent_file: maybe_parent_file, num_parent_commits, name, @@ -1461,9 +1451,9 @@ mod tests { #[test_case(true; "file")] fn index_empty(on_disk: bool) { let temp_dir = tempfile::tempdir().unwrap(); - let index = MutableIndex::full(temp_dir.path().to_owned(), 3); + let index = MutableIndex::full(3); let index = if on_disk { - IndexRef::Readonly(index.save().unwrap()) + IndexRef::Readonly(index.save_in(temp_dir.path().to_owned()).unwrap()) } else { IndexRef::Mutable(&index) }; @@ -1487,12 +1477,12 @@ mod tests { #[test_case(true; "file")] fn index_root_commit(on_disk: bool) { let temp_dir = tempfile::tempdir().unwrap(); - let mut index = MutableIndex::full(temp_dir.path().to_owned(), 3); + let mut index = MutableIndex::full(3); 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 index = if on_disk { - IndexRef::Readonly(index.save().unwrap()) + IndexRef::Readonly(index.save_in(temp_dir.path().to_owned()).unwrap()) } else { IndexRef::Mutable(&index) }; @@ -1526,8 +1516,7 @@ mod tests { #[test] #[should_panic(expected = "parent commit is not indexed")] fn index_missing_parent_commit() { - let temp_dir = tempfile::tempdir().unwrap(); - let mut index = MutableIndex::full(temp_dir.path().to_owned(), 3); + let mut index = MutableIndex::full(3); let id_0 = CommitId::from_hex("000000"); let id_1 = CommitId::from_hex("111111"); index.add_commit_data(id_1, new_change_id(), false, vec![id_0], vec![]); @@ -1536,8 +1525,7 @@ mod tests { #[test] #[should_panic(expected = "predecessor commit is not indexed")] fn index_missing_predecessor_commit() { - let temp_dir = tempfile::tempdir().unwrap(); - let mut index = MutableIndex::full(temp_dir.path().to_owned(), 3); + let mut index = MutableIndex::full(3); let id_0 = CommitId::from_hex("000000"); let id_1 = CommitId::from_hex("111111"); index.add_commit_data(id_1, new_change_id(), false, vec![], vec![id_0]); @@ -1549,7 +1537,7 @@ mod tests { #[test_case(true, true; "incremental on disk")] fn index_multiple_commits(incremental: bool, on_disk: bool) { let temp_dir = tempfile::tempdir().unwrap(); - let mut index = MutableIndex::full(temp_dir.path().to_owned(), 3); + let mut index = MutableIndex::full(3); // 5 // |\ // 4 | 3 @@ -1582,7 +1570,7 @@ mod tests { // If testing incremental indexing, write the first three commits to one file // now and build the remainder as another segment on top. if incremental { - let initial_file = index.save().unwrap(); + let initial_file = index.save_in(temp_dir.path().to_owned()).unwrap(); index = MutableIndex::incremental(initial_file); } @@ -1614,7 +1602,7 @@ mod tests { vec![], ); let index = if on_disk { - IndexRef::Readonly(index.save().unwrap()) + IndexRef::Readonly(index.save_in(temp_dir.path().to_owned()).unwrap()) } else { IndexRef::Mutable(&index) }; @@ -1675,7 +1663,7 @@ mod tests { #[test_case(true; "on disk")] fn index_many_parents(on_disk: bool) { let temp_dir = tempfile::tempdir().unwrap(); - let mut index = MutableIndex::full(temp_dir.path().to_owned(), 3); + let mut index = MutableIndex::full(3); // 6 // /|\ // / | \ @@ -1730,7 +1718,7 @@ mod tests { vec![], ); let index = if on_disk { - IndexRef::Readonly(index.save().unwrap()) + IndexRef::Readonly(index.save_in(temp_dir.path().to_owned()).unwrap()) } else { IndexRef::Mutable(&index) }; @@ -1753,7 +1741,7 @@ mod tests { #[test] fn resolve_prefix() { let temp_dir = tempfile::tempdir().unwrap(); - let mut index = MutableIndex::full(temp_dir.path().to_owned(), 3); + let mut index = MutableIndex::full(3); // Create some commits with different various common prefixes. let id_0 = CommitId::from_hex("000000"); @@ -1764,7 +1752,7 @@ mod tests { index.add_commit_data(id_2.clone(), new_change_id(), false, vec![], vec![]); // Write the first three commits to one file and build the remainder on top. - let initial_file = index.save().unwrap(); + let initial_file = index.save_in(temp_dir.path().to_owned()).unwrap(); index = MutableIndex::incremental(initial_file); let id_3 = CommitId::from_hex("055444"); @@ -1819,8 +1807,7 @@ mod tests { } #[test] fn test_is_ancestor() { - let temp_dir = tempfile::tempdir().unwrap(); - let mut index = MutableIndex::full(temp_dir.path().to_owned(), 3); + let mut index = MutableIndex::full(3); // 5 // |\ // 4 | 3 @@ -1886,8 +1873,7 @@ mod tests { #[test] fn test_common_ancestors() { - let temp_dir = tempfile::tempdir().unwrap(); - let mut index = MutableIndex::full(temp_dir.path().to_owned(), 3); + let mut index = MutableIndex::full(3); // 5 // |\ // 4 | @@ -1998,8 +1984,7 @@ mod tests { #[test] fn test_common_ancestors_criss_cross() { - let temp_dir = tempfile::tempdir().unwrap(); - let mut index = MutableIndex::full(temp_dir.path().to_owned(), 3); + let mut index = MutableIndex::full(3); // 3 4 // |X| // 1 2 @@ -2041,8 +2026,7 @@ mod tests { #[test] fn test_common_ancestors_merge_with_ancestor() { - let temp_dir = tempfile::tempdir().unwrap(); - let mut index = MutableIndex::full(temp_dir.path().to_owned(), 3); + let mut index = MutableIndex::full(3); // 4 5 // |\ /| // 1 2 3 @@ -2086,8 +2070,7 @@ mod tests { #[test] fn test_walk_revs() { - let temp_dir = tempfile::tempdir().unwrap(); - let mut index = MutableIndex::full(temp_dir.path().to_owned(), 3); + let mut index = MutableIndex::full(3); // 5 // |\ // 4 | 3 @@ -2195,8 +2178,7 @@ mod tests { #[test] fn test_heads() { - let temp_dir = tempfile::tempdir().unwrap(); - let mut index = MutableIndex::full(temp_dir.path().to_owned(), 3); + let mut index = MutableIndex::full(3); // 5 // |\ // 4 | 3 diff --git a/lib/src/index_store.rs b/lib/src/index_store.rs index c31ee8853..dcb013192 100644 --- a/lib/src/index_store.rs +++ b/lib/src/index_store.rs @@ -60,6 +60,10 @@ impl IndexStore { } } + pub fn write_index(&self, index: MutableIndex) -> io::Result> { + index.save_in(self.dir.clone()) + } + fn load_index_at_operation( &self, hash_length: usize, @@ -112,7 +116,7 @@ impl IndexStore { match parent_op_id { None => { maybe_parent_file = None; - data = MutableIndex::full(self.dir.clone(), hash_length); + data = MutableIndex::full(hash_length); } Some(parent_op_id) => { let parent_file = self @@ -131,7 +135,7 @@ impl IndexStore { data.add_commit(&commit); } - let index_file = data.save()?; + let index_file = data.save_in(self.dir.clone())?; self.associate_file_with_operation(&index_file, operation.id())?; diff --git a/lib/src/transaction.rs b/lib/src/transaction.rs index 38dcb4ecb..b1dad1aa0 100644 --- a/lib/src/transaction.rs +++ b/lib/src/transaction.rs @@ -204,7 +204,14 @@ impl<'r> Transaction<'r> { let mut_repo = Arc::try_unwrap(self.repo.take().unwrap()).ok().unwrap(); let index_store = mut_repo.base_repo().index_store(); let (mut_index, mut_view) = mut_repo.consume(); - let index = mut_index.save().unwrap(); + // TODO: There is a race here: We add the operation to the list of head + // operations (in mut_view.save()) before we associate the operation with the + // index. That means that there's a small risk that another process finds the + // new operation and does redundant work to calculate the. We should + // probably make mut_view.save() write the operation without recording + // the new head (and without removing the old head), so we can associate the + // operation id with the index before we mark the operation as a head. + let index = index_store.write_index(mut_index).unwrap(); let operation = mut_view.save(self.description.clone(), self.start_time.clone()); index_store .associate_file_with_operation(&index, operation.id())