commit_builder: error out if newly-created commit was already known to repo

This can happen if we remove predecessors from the commit object, and if
committer timestamp wasn't updated (like in some jj-lib tests.) It's not wrong
for the store to "overwrite" existing objects, but the repo expects
new/rewritten commits have unique ids. If they didn't, cycle would be created in
mut_repo.commit_predecessors/parent_mapping.

Author: yuja

lib/src/commit_builder.rs

@@ -19,6 +19,7 @@ use std::sync::Arc;
 use pollster::FutureExt as _;
 
 use crate::backend;
+use crate::backend::BackendError;
 use crate::backend::BackendResult;
 use crate::backend::ChangeId;
 use crate::backend::CommitId;
@@ -351,6 +352,15 @@ impl DetachedCommitBuilder {
     pub fn write(self, mut_repo: &mut MutableRepo) -> BackendResult<Commit> {
         let predecessors = self.commit.predecessors.clone();
         let commit = write_to_store(&self.store, self.commit, &self.sign_settings)?;
+        // FIXME: Google's index.has_id() always returns true.
+        if mut_repo.is_backed_by_default_index() && mut_repo.index().has_id(commit.id()) {
+            // Recording existing commit as new would create cycle in
+            // predecessors/parent mappings within the current transaction, and
+            // in predecessors graph globally.
+            return Err(BackendError::Other(
+                format!("Newly-created commit {id} already exists", id = commit.id()).into(),
+            ));
+        }
         mut_repo.add_head(&commit)?;
         mut_repo.set_predecessors(commit.id().clone(), predecessors);
         if let Some(rewrite_source) = self.rewrite_source {

lib/src/repo.rs

@@ -900,6 +900,10 @@ impl MutableRepo {
         self.index.as_ref()
     }
 
+    pub(crate) fn is_backed_by_default_index(&self) -> bool {
+        self.index.as_any().is::<DefaultMutableIndex>()
+    }
+
     pub fn has_changes(&self) -> bool {
         self.view.ensure_clean(|v| self.enforce_view_invariants(v));
         !(self.commit_predecessors.is_empty()
@@ -1781,7 +1785,7 @@ impl MutableRepo {
         // feasible to walk all added and removed commits.
         // TODO: Fix this somehow. Maybe a method on `Index` to find rewritten commits
         // given `base_heads`, `own_heads` and `other_heads`?
-        if self.index.as_any().is::<DefaultMutableIndex>() {
+        if self.is_backed_by_default_index() {
             self.record_rewrites(&base_heads, &own_heads)?;
             self.record_rewrites(&base_heads, &other_heads)?;
             // No need to remove heads removed by `other` because we already

lib/tests/test_commit_builder.rs

@@ -16,6 +16,7 @@ use assert_matches::assert_matches;
 use futures::StreamExt as _;
 use indoc::indoc;
 use itertools::Itertools as _;
+use jj_lib::backend::BackendError;
 use jj_lib::backend::ChangeId;
 use jj_lib::backend::MillisSinceEpoch;
 use jj_lib::backend::Signature;
@@ -367,6 +368,57 @@ fn test_rewrite_resets_author_timestamp(backend: TestRepoBackend) {
     assert_eq!(rewritten_commit_2.committer().timestamp, new_timestamp_2);
 }
 
+#[test_case(TestRepoBackend::Simple ; "simple backend")]
+#[test_case(TestRepoBackend::Git ; "git backend")]
+fn test_rewrite_to_identical_commit(backend: TestRepoBackend) {
+    let timestamp = "2001-02-03T04:05:06+07:00";
+    let settings = UserSettings::from_config(config_with_commit_timestamp(timestamp)).unwrap();
+    let test_repo = TestRepo::init_with_backend_and_settings(backend, &settings);
+    let repo = test_repo.repo;
+    let store = repo.store();
+
+    let mut tx = repo.start_transaction();
+    let commit1 = tx
+        .repo_mut()
+        .new_commit(
+            vec![store.root_commit_id().clone()],
+            store.empty_merged_tree_id(),
+        )
+        .write()
+        .unwrap();
+    let repo = tx.commit("test").unwrap();
+
+    // Create commit identical to the original
+    let mut tx = repo.start_transaction();
+    let mut builder = tx.repo_mut().rewrite_commit(&commit1).detach();
+    builder.set_predecessors(vec![]);
+    // Writing to the store should work
+    let commit2 = builder.write_hidden().unwrap();
+    assert_eq!(commit1, commit2);
+    // Writing to the repo shouldn't work, which would create cycle in
+    // predecessors/parent mappings
+    let result = builder.write(tx.repo_mut());
+    assert_matches!(result, Err(BackendError::Other(_)));
+    tx.repo_mut().rebase_descendants().unwrap();
+    tx.commit("test").unwrap();
+
+    // Create two rewritten commits of the same content and metadata
+    let mut tx = repo.start_transaction();
+    tx.repo_mut()
+        .rewrite_commit(&commit1)
+        .set_description("rewritten")
+        .write()
+        .unwrap();
+    let result = tx
+        .repo_mut()
+        .rewrite_commit(&commit1)
+        .set_description("rewritten")
+        .write();
+    assert_matches!(result, Err(BackendError::Other(_)));
+    tx.repo_mut().rebase_descendants().unwrap();
+    tx.commit("test").unwrap();
+}
+
 #[test_case(TestRepoBackend::Simple ; "simple backend")]
 // #[test_case(TestRepoBackend::Git ; "git backend")]
 fn test_commit_builder_descendants(backend: TestRepoBackend) {