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

@@ -76,11 +76,7 @@ impl Commit {
     }
 
     pub fn parent_ids(&self) -> Vec<CommitId> {
-        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<Commit> {
@@ -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
     }
 

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<CommitId>) -> 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 {

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("<no message>").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);

lib/src/store.rs

@@ -83,6 +83,7 @@ impl Store {
     }
 
     pub fn write_commit(self: &Arc<Self>, commit: backend::Commit) -> Commit {
+        assert!(!commit.parents.is_empty());
         let commit_id = self.backend.write_commit(&commit).unwrap();
         let data = Arc::new(commit);
         {

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::<u32>();
-    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) {

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());
 

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());

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())

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);

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 =