From 010867308714cf659b6bedd692e8ffde37112848 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sun, 18 Sep 2022 21:59:49 -0700 Subject: [PATCH] backend: let each backend handle root commit on write This moves the logic for handling the root commit when writing commits from `CommitBuilder` into the individual backends. It always bothered me a bit that the `commit::Commit` wrapper had a different idea of the number of parents than the wrapped `backend::Commit` had. With this change, the `LocalBackend` will now write the root commit in the list of parents if it's there in the argument to `write_commit()`. Note that root commit itself won't be written. The main argument for not writing it is that we can then keep the fake all-zeros hash for it. One argument for writing it, if we were to do so, is that it would make the set of written objects consistent, so any future processing of them (such as GC) doesn't have to know to ignore the root commit in the list of parents. We still treat the two backends the same, so the user won't be allowed to create merges including the root commit even when using the `LocalBackend`. --- lib/src/commit.rs | 9 +-------- lib/src/commit_builder.rs | 9 +++------ lib/src/git_backend.rs | 21 ++++++++++++++++----- lib/src/store.rs | 1 + lib/src/testutils.rs | 8 ++++++-- lib/tests/test_commit_builder.rs | 10 +++++++--- lib/tests/test_init.rs | 5 ++++- lib/tests/test_merge_trees.rs | 8 ++++++-- lib/tests/test_revset.rs | 23 +++++++++++++++-------- lib/tests/test_rewrite.rs | 8 ++++++-- 10 files changed, 65 insertions(+), 37 deletions(-) diff --git a/lib/src/commit.rs b/lib/src/commit.rs index 6d8341499..a165671e5 100644 --- a/lib/src/commit.rs +++ b/lib/src/commit.rs @@ -76,11 +76,7 @@ impl Commit { } pub fn parent_ids(&self) -> Vec { - if self.data.parents.is_empty() && &self.id != self.store.root_commit_id() { - vec![self.store.root_commit_id().clone()] - } else { - self.data.parents.clone() - } + self.data.parents.clone() } pub fn parents(&self) -> Vec { @@ -88,9 +84,6 @@ impl Commit { for parent in &self.data.parents { parents.push(self.store.get_commit(parent).unwrap()); } - if parents.is_empty() && &self.id != self.store.root_commit_id() { - parents.push(self.store.root_commit()) - } parents } diff --git a/lib/src/commit_builder.rs b/lib/src/commit_builder.rs index 46fc80da5..147c0684b 100644 --- a/lib/src/commit_builder.rs +++ b/lib/src/commit_builder.rs @@ -37,6 +37,7 @@ impl CommitBuilder { tree_id: TreeId, ) -> CommitBuilder { let signature = settings.signature(); + assert!(!parents.is_empty()); let commit = backend::Commit { parents, predecessors: vec![], @@ -94,6 +95,7 @@ impl CommitBuilder { } pub fn set_parents(mut self, parents: Vec) -> Self { + assert!(!parents.is_empty()); self.commit.parents = parents; self } @@ -138,12 +140,7 @@ impl CommitBuilder { self } - pub fn write_to_repo(mut self, repo: &mut MutableRepo) -> Commit { - let parents = &mut self.commit.parents; - if parents.contains(repo.store().root_commit_id()) { - assert_eq!(parents.len(), 1); - parents.clear(); - } + pub fn write_to_repo(self, repo: &mut MutableRepo) -> Commit { let mut rewrite_source_id = None; if let Some(rewrite_source) = self.rewrite_source { if *rewrite_source.change_id() == self.commit.change_id { diff --git a/lib/src/git_backend.rs b/lib/src/git_backend.rs index 2a49edaef..cb4a0744c 100644 --- a/lib/src/git_backend.rs +++ b/lib/src/git_backend.rs @@ -366,10 +366,13 @@ impl Backend for GitBackend { .map(|b| b.reverse_bits()) .collect(), ); - let parents = commit + let mut parents = commit .parent_ids() .map(|oid| CommitId::from_bytes(oid.as_bytes())) .collect_vec(); + if parents.is_empty() { + parents.push(self.root_commit_id.clone()); + }; let tree_id = TreeId::from_bytes(commit.tree_id().as_bytes()); let description = commit.message().unwrap_or("").to_owned(); let author = signature_from_git(commit.author()); @@ -406,9 +409,17 @@ impl Backend for GitBackend { let mut parents = vec![]; for parent_id in &contents.parents { - let parent_git_commit = - locked_repo.find_commit(Oid::from_bytes(parent_id.as_bytes())?)?; - parents.push(parent_git_commit); + if *parent_id == self.root_commit_id { + // Git doesn't have a root commit, so if the parent is the root commit, we don't + // add it to the list of parents to write in the Git commit. We also check that + // there are no other parents since Git cannot represent a merge between a root + // commit and another commit. + assert_eq!(contents.parents.len(), 1); + } else { + let parent_git_commit = + locked_repo.find_commit(Oid::from_bytes(parent_id.as_bytes())?)?; + parents.push(parent_git_commit); + } } let parent_refs = parents.iter().collect_vec(); let git_id = locked_repo.commit( @@ -573,7 +584,7 @@ mod tests { let store = GitBackend::init_external(store_path, git_repo_path); let commit = store.read_commit(&commit_id).unwrap(); assert_eq!(&commit.change_id, &change_id); - assert_eq!(commit.parents, vec![]); + assert_eq!(commit.parents, vec![CommitId::from_bytes(&[0; 20])]); assert_eq!(commit.predecessors, vec![]); assert_eq!(commit.root_tree.as_bytes(), root_tree_id.as_bytes()); assert!(!commit.is_open); diff --git a/lib/src/store.rs b/lib/src/store.rs index 944bf1a0c..ba8197f54 100644 --- a/lib/src/store.rs +++ b/lib/src/store.rs @@ -83,6 +83,7 @@ impl Store { } pub fn write_commit(self: &Arc, commit: backend::Commit) -> Commit { + assert!(!commit.parents.is_empty()); let commit_id = self.backend.write_commit(&commit).unwrap(); let data = Arc::new(commit); { diff --git a/lib/src/testutils.rs b/lib/src/testutils.rs index 1eeae0e1d..1b55fcff6 100644 --- a/lib/src/testutils.rs +++ b/lib/src/testutils.rs @@ -189,8 +189,12 @@ pub fn create_random_tree(repo: &ReadonlyRepo) -> TreeId { pub fn create_random_commit(settings: &UserSettings, repo: &ReadonlyRepo) -> CommitBuilder { let tree_id = create_random_tree(repo); let number = rand::random::(); - CommitBuilder::for_new_commit(settings, vec![], tree_id) - .set_description(format!("random commit {}", number)) + CommitBuilder::for_new_commit( + settings, + vec![repo.store().root_commit_id().clone()], + tree_id, + ) + .set_description(format!("random commit {}", number)) } pub fn write_working_copy_file(workspace_root: &Path, path: &RepoPath, contents: &str) { diff --git a/lib/tests/test_commit_builder.rs b/lib/tests/test_commit_builder.rs index 3288d8602..7d8b38634 100644 --- a/lib/tests/test_commit_builder.rs +++ b/lib/tests/test_commit_builder.rs @@ -170,7 +170,7 @@ fn test_rewrite_update_missing_user(use_git: bool) { let mut tx = repo.start_transaction("test"); let initial_commit = CommitBuilder::for_new_commit( &missing_user_settings, - vec![], + vec![repo.store().root_commit_id().clone()], repo.store().empty_tree_id().clone(), ) .write_to_repo(tx.mut_repo()); @@ -219,8 +219,12 @@ fn test_commit_builder_descendants(use_git: bool) { // Test with for_new_commit() let mut tx = repo.start_transaction("test"); - CommitBuilder::for_new_commit(&settings, vec![], store.empty_tree_id().clone()) - .write_to_repo(tx.mut_repo()); + CommitBuilder::for_new_commit( + &settings, + vec![store.root_commit_id().clone()], + store.empty_tree_id().clone(), + ) + .write_to_repo(tx.mut_repo()); let mut rebaser = tx.mut_repo().create_descendant_rebaser(&settings); assert!(rebaser.rebase_next().unwrap().is_none()); diff --git a/lib/tests/test_init.rs b/lib/tests/test_init.rs index 97fd17c6a..3e3d0eb3d 100644 --- a/lib/tests/test_init.rs +++ b/lib/tests/test_init.rs @@ -119,7 +119,10 @@ fn test_init_checkout(use_git: bool) { .unwrap(); let wc_commit = repo.store().get_commit(wc_commit_id).unwrap(); assert_eq!(wc_commit.tree_id(), repo.store().empty_tree_id()); - assert_eq!(wc_commit.store_commit().parents, vec![]); + assert_eq!( + wc_commit.store_commit().parents, + vec![repo.store().root_commit_id().clone()] + ); assert_eq!(wc_commit.predecessors(), vec![]); assert_eq!(wc_commit.description(), ""); assert!(wc_commit.is_open()); diff --git a/lib/tests/test_merge_trees.rs b/lib/tests/test_merge_trees.rs index 0d53746cd..2d3340c6a 100644 --- a/lib/tests/test_merge_trees.rs +++ b/lib/tests/test_merge_trees.rs @@ -573,8 +573,12 @@ fn test_simplify_conflict_after_resolving_parent(use_git: bool) { let path = RepoPath::from_internal_string("dir/file"); let mut tx = repo.start_transaction("test"); let tree_a = testutils::create_tree(repo, &[(&path, "abc\ndef\nghi\n")]); - let commit_a = CommitBuilder::for_new_commit(&settings, vec![], tree_a.id().clone()) - .write_to_repo(tx.mut_repo()); + let commit_a = CommitBuilder::for_new_commit( + &settings, + vec![repo.store().root_commit_id().clone()], + tree_a.id().clone(), + ) + .write_to_repo(tx.mut_repo()); let tree_b = testutils::create_tree(repo, &[(&path, "Abc\ndef\nghi\n")]); let commit_b = CommitBuilder::for_new_commit(&settings, vec![commit_a.id().clone()], tree_b.id().clone()) diff --git a/lib/tests/test_revset.rs b/lib/tests/test_revset.rs index c1864d833..1cc76a63f 100644 --- a/lib/tests/test_revset.rs +++ b/lib/tests/test_revset.rs @@ -55,12 +55,15 @@ fn test_resolve_symbol_commit_id() { let mut commits = vec![]; for i in &[1, 167, 895] { - let commit = - CommitBuilder::for_new_commit(&settings, vec![], repo.store().empty_tree_id().clone()) - .set_description(format!("test {}", i)) - .set_author(signature.clone()) - .set_committer(signature.clone()) - .write_to_repo(mut_repo); + let commit = CommitBuilder::for_new_commit( + &settings, + vec![repo.store().root_commit_id().clone()], + repo.store().empty_tree_id().clone(), + ) + .set_description(format!("test {}", i)) + .set_author(signature.clone()) + .set_committer(signature.clone()) + .write_to_repo(mut_repo); commits.push(commit); } let repo = tx.commit(); @@ -1755,8 +1758,12 @@ fn test_filter_by_diff(use_git: bool) { // added_modified_removed, ], ); - let commit1 = CommitBuilder::for_new_commit(&settings, vec![], tree1.id().clone()) - .write_to_repo(mut_repo); + let commit1 = CommitBuilder::for_new_commit( + &settings, + vec![repo.store().root_commit_id().clone()], + tree1.id().clone(), + ) + .write_to_repo(mut_repo); let commit2 = CommitBuilder::for_new_commit(&settings, vec![commit1.id().clone()], tree2.id().clone()) .write_to_repo(mut_repo); diff --git a/lib/tests/test_rewrite.rs b/lib/tests/test_rewrite.rs index e08f7be84..ae7426850 100644 --- a/lib/tests/test_rewrite.rs +++ b/lib/tests/test_rewrite.rs @@ -849,8 +849,12 @@ fn test_rebase_descendants_contents(use_git: bool) { let mut tx = repo.start_transaction("test"); let path1 = RepoPath::from_internal_string("file1"); let tree1 = testutils::create_tree(repo, &[(&path1, "content")]); - let commit_a = CommitBuilder::for_new_commit(&settings, vec![], tree1.id().clone()) - .write_to_repo(tx.mut_repo()); + let commit_a = CommitBuilder::for_new_commit( + &settings, + vec![repo.store().root_commit_id().clone()], + tree1.id().clone(), + ) + .write_to_repo(tx.mut_repo()); let path2 = RepoPath::from_internal_string("file2"); let tree2 = testutils::create_tree(repo, &[(&path2, "content")]); let commit_b =