git_backend: ensure change id generated from git commit id never reas…

…signed Fixes jj-vcs#924
yuja · May 19, 2023 · ed1fdca · ed1fdca
1 parent 6cddecf
commit ed1fdca
Show file tree

Hide file tree

Showing 2 changed files with 14 additions and 8 deletions.
diff --git a/lib/src/git_backend.rs b/lib/src/git_backend.rs
@@ -491,6 +491,18 @@ impl Backend for GitBackend {
         let table = self.cached_extra_metadata_table()?;
         if let Some(extras) = table.get_value(id.as_bytes()) {
             deserialize_extras(&mut commit, extras);
+        } else {
+            let (table, table_lock) = self.read_extra_metadata_table_locked()?;
+            if let Some(extras) = table.get_value(id.as_bytes()) {
+                // Concurrent write_commit() would update extras before taking a lock.
+                deserialize_extras(&mut commit, extras);
+                *self.cached_extra_metadata.lock().unwrap() = Some(table);
+            } else {
+                // Make our change id persist (otherwise future write_commit() could reassign
+                // new change id.)
+                let extras = serialize_extras(&commit);
+                self.write_extra_metadata_entry(&table, &table_lock, id, extras)?;
+            }
         }
 
         Ok(commit)

diff --git a/lib/tests/test_git.rs b/lib/tests/test_git.rs
@@ -1906,7 +1906,7 @@ fn test_rewrite_imported_commit() {
 
     // Try to create identical commit with different change id.
     let mut tx = repo.start_transaction(&settings, "test");
-    let _authored_commit = tx
+    let authored_commit = tx
         .mut_repo()
         .new_commit(
             &settings,
@@ -1918,20 +1918,17 @@ fn test_rewrite_imported_commit() {
         .set_description(imported_commit.description())
         .write()
         .unwrap();
-    let _repo = tx.commit();
+    let repo = tx.commit();
 
     // Imported commit shouldn't be reused, and the timestamp of the authored
     // commit should be adjusted to create new commit.
-    /* TODO
     assert_ne!(imported_commit.id(), authored_commit.id());
     assert_ne!(
         imported_commit.committer().timestamp,
         authored_commit.committer().timestamp,
     );
-    */
 
     // The index should be consistent with the store.
-    /* TODO
     assert_eq!(
         repo.resolve_change_id(imported_commit.change_id()),
         Some(vec![imported_commit.id().clone()]),
@@ -1940,7 +1937,6 @@ fn test_rewrite_imported_commit() {
         repo.resolve_change_id(authored_commit.change_id()),
         Some(vec![authored_commit.id().clone()]),
     );
-    */
 }
 
 #[test]
@@ -2094,13 +2090,11 @@ fn test_concurrent_read_write_commit() {
     let repo = repo.reload_at_head(settings).unwrap();
     for commit_id in &commit_ids {
         assert!(repo.index().has_id(commit_id));
-        /* TODO
         let commit = repo.store().get_commit(commit_id).unwrap();
         assert_eq!(
             repo.resolve_change_id(commit.change_id()),
             Some(vec![commit_id.clone()]),
         );
-        */
     }
 }