backend: take commit to write by value and return new value

The internal backend at Google doesn't let you write any value you
want for in the committer field. The `Store` type still caches the
value it attempted to write, which gets a little weird when the
written value is not what we tried to write. We should use the value
the backend actually wrote. However, we don't know if the backend
changed anything without reading the value back, which is often
wasteful. This commit changes the API to return the written value.

I only changed the signature of `write_commit()` for now. Maybe we
should make a similar change to `write_tree()`.

examples/custom-backend/main.rs

@@ -153,7 +153,7 @@ impl Backend for JitBackend {
         self.inner.read_commit(id)
     }
 
-    fn write_commit(&self, contents: &Commit) -> BackendResult<CommitId> {
+    fn write_commit(&self, contents: Commit) -> BackendResult<(CommitId, Commit)> {
         self.inner.write_commit(contents)
     }
 }

lib/src/backend.rs

@@ -406,5 +406,11 @@ pub trait Backend: Send + Sync + Debug {
 
     fn read_commit(&self, id: &CommitId) -> BackendResult<Commit>;
 
-    fn write_commit(&self, contents: &Commit) -> BackendResult<CommitId>;
+    /// Writes a commit and returns its ID and the commit itself. The commit
+    /// should contain the data that was actually written, which may differ
+    /// from the data passed in. For example, the backend may change the
+    /// committer name to an authenticated user's name, or the backend's
+    /// timestamps may have less precision than the millisecond precision in
+    /// `Commit`.
+    fn write_commit(&self, contents: Commit) -> BackendResult<(CommitId, Commit)>;
 }

lib/src/git_backend.rs

@@ -119,7 +119,7 @@ fn signature_from_git(signature: git2::Signature) -> Signature {
     }
 }
 
-fn signature_to_git(signature: &Signature) -> git2::Signature {
+fn signature_to_git(signature: &Signature) -> git2::Signature<'static> {
     let name = &signature.name;
     let email = &signature.email;
     let time = git2::Time::new(
@@ -468,7 +468,7 @@ impl Backend for GitBackend {
         Ok(commit)
     }
 
-    fn write_commit(&self, contents: &Commit) -> BackendResult<CommitId> {
+    fn write_commit(&self, contents: Commit) -> BackendResult<(CommitId, Commit)> {
         let locked_repo = self.repo.lock().unwrap();
         let git_tree_id = validate_git_object_id(&contents.root_tree)?;
         let git_tree = locked_repo
@@ -505,7 +505,7 @@ impl Backend for GitBackend {
             }
         }
         let parent_refs = parents.iter().collect_vec();
-        let extras = serialize_extras(contents);
+        let extras = serialize_extras(&contents);
         let mut mut_table = self
             .extra_metadata_store
             .get_head()
@@ -553,7 +553,7 @@ impl Backend for GitBackend {
                 BackendError::Other(format!("Failed to write non-git metadata: {err}"))
             })?;
         *self.cached_extra_metadata.lock().unwrap() = None;
-        Ok(id)
+        Ok((id, contents))
     }
 }
 
@@ -767,13 +767,13 @@ mod tests {
         // No parents
         commit.parents = vec![];
         assert_matches!(
-            backend.write_commit(&commit),
+            backend.write_commit(commit.clone()),
             Err(BackendError::Other(message)) if message.contains("no parents")
         );
 
         // Only root commit as parent
         commit.parents = vec![backend.root_commit_id().clone()];
-        let first_id = backend.write_commit(&commit).unwrap();
+        let first_id = backend.write_commit(commit.clone()).unwrap().0;
         let first_commit = backend.read_commit(&first_id).unwrap();
         assert_eq!(first_commit, commit);
         let first_git_commit = git_repo.find_commit(git_id(&first_id)).unwrap();
@@ -781,7 +781,7 @@ mod tests {
 
         // Only non-root commit as parent
         commit.parents = vec![first_id.clone()];
-        let second_id = backend.write_commit(&commit).unwrap();
+        let second_id = backend.write_commit(commit.clone()).unwrap().0;
         let second_commit = backend.read_commit(&second_id).unwrap();
         assert_eq!(second_commit, commit);
         let second_git_commit = git_repo.find_commit(git_id(&second_id)).unwrap();
@@ -792,7 +792,7 @@ mod tests {
 
         // Merge commit
         commit.parents = vec![first_id.clone(), second_id.clone()];
-        let merge_id = backend.write_commit(&commit).unwrap();
+        let merge_id = backend.write_commit(commit.clone()).unwrap().0;
         let merge_commit = backend.read_commit(&merge_id).unwrap();
         assert_eq!(merge_commit, commit);
         let merge_git_commit = git_repo.find_commit(git_id(&merge_id)).unwrap();
@@ -804,7 +804,7 @@ mod tests {
         // Merge commit with root as one parent
         commit.parents = vec![first_id, backend.root_commit_id().clone()];
         assert_matches!(
-            backend.write_commit(&commit),
+            backend.write_commit(commit),
             Err(BackendError::Other(message)) if message.contains("root commit")
         );
     }
@@ -830,7 +830,7 @@ mod tests {
             author: signature.clone(),
             committer: signature,
         };
-        let commit_id = store.write_commit(&commit).unwrap();
+        let commit_id = store.write_commit(commit).unwrap().0;
         let git_refs = store
             .git_repo()
             .references_glob("refs/jj/keep/*")
@@ -853,12 +853,11 @@ mod tests {
             author: create_signature(),
             committer: create_signature(),
         };
-        let commit_id1 = store.write_commit(&commit1).unwrap();
-        let mut commit2 = commit1;
+        let (commit_id1, mut commit2) = store.write_commit(commit1).unwrap();
         commit2.predecessors.push(commit_id1.clone());
         // `write_commit` should prevent the ids from being the same by changing the
         // committer timestamp of the commit it actually writes.
-        assert_ne!(store.write_commit(&commit2).unwrap(), commit_id1);
+        assert_ne!(store.write_commit(commit2).unwrap().0, commit_id1);
     }
 
     fn git_id(commit_id: &CommitId) -> Oid {

lib/src/local_backend.rs

@@ -259,16 +259,16 @@ impl Backend for LocalBackend {
         Ok(commit_from_proto(proto))
     }
 
-    fn write_commit(&self, commit: &Commit) -> BackendResult<CommitId> {
+    fn write_commit(&self, commit: Commit) -> BackendResult<(CommitId, Commit)> {
         let temp_file = NamedTempFile::new_in(&self.path)?;
 
-        let proto = commit_to_proto(commit);
+        let proto = commit_to_proto(&commit);
         temp_file.as_file().write_all(&proto.encode_to_vec())?;
 
-        let id = CommitId::new(blake2b_hash(commit).to_vec());
+        let id = CommitId::new(blake2b_hash(&commit).to_vec());
 
         persist_content_addressed_temp_file(temp_file, self.commit_path(&id))?;
-        Ok(id)
+        Ok((id, commit))
     }
 }
 

lib/src/store.rs

@@ -93,7 +93,7 @@ impl Store {
 
     pub fn write_commit(self: &Arc<Self>, commit: backend::Commit) -> BackendResult<Commit> {
         assert!(!commit.parents.is_empty());
-        let commit_id = self.backend.write_commit(&commit)?;
+        let (commit_id, commit) = self.backend.write_commit(commit)?;
         let data = Arc::new(commit);
         {
             let mut write_locked_cache = self.commit_cache.write().unwrap();