From a95188ddbc3c8d11e754d1874e6bd8c70b9c4056 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Thu, 11 May 2023 15:40:24 -0700 Subject: [PATCH] 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 | 2 +- lib/src/backend.rs | 8 +++++++- lib/src/git_backend.rs | 25 ++++++++++++------------- lib/src/local_backend.rs | 8 ++++---- lib/src/store.rs | 2 +- 5 files changed, 25 insertions(+), 20 deletions(-) diff --git a/examples/custom-backend/main.rs b/examples/custom-backend/main.rs index e7c8a8495..d4a2bed59 100644 --- a/examples/custom-backend/main.rs +++ b/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 { + fn write_commit(&self, contents: Commit) -> BackendResult<(CommitId, Commit)> { self.inner.write_commit(contents) } } diff --git a/lib/src/backend.rs b/lib/src/backend.rs index dc9ffc5ef..c4ba57f07 100644 --- a/lib/src/backend.rs +++ b/lib/src/backend.rs @@ -406,5 +406,11 @@ pub trait Backend: Send + Sync + Debug { fn read_commit(&self, id: &CommitId) -> BackendResult; - fn write_commit(&self, contents: &Commit) -> BackendResult; + /// 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)>; } diff --git a/lib/src/git_backend.rs b/lib/src/git_backend.rs index 78b0eb7da..64521218d 100644 --- a/lib/src/git_backend.rs +++ b/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 { + 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 { diff --git a/lib/src/local_backend.rs b/lib/src/local_backend.rs index 9a89a725b..92251ef1c 100644 --- a/lib/src/local_backend.rs +++ b/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 { + 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)) } } diff --git a/lib/src/store.rs b/lib/src/store.rs index ba0b2f1c6..982649464 100644 --- a/lib/src/store.rs +++ b/lib/src/store.rs @@ -93,7 +93,7 @@ impl Store { pub fn write_commit(self: &Arc, commit: backend::Commit) -> BackendResult { 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();