ok/jj
1
0
Fork 0
forked from mirrors/jj

git_backend: on read_commit(), fall back to importing extras as needed

One problematic scenario is that we have commits imported by old jj, and all
of their descendant commits are created by jj. Therefore import_head_commits()
wouldn't reach the old ancestor commits.

This change might bury a real bug, but I don't have a better alternative. Maybe
we can remove this hack after a couple of jj releases, and add a debug command
that imports all reachable Git commits from all historical heads.

Closes #2343
This commit is contained in:
Yuya Nishihara 2023-10-06 22:25:08 +09:00
parent b5722eb0a5
commit f0ad1f53ea

View file

@ -689,6 +689,7 @@ impl Backend for GitBackend {
Ok(ConflictId::from_bytes(oid.as_bytes())) Ok(ConflictId::from_bytes(oid.as_bytes()))
} }
#[tracing::instrument(skip(self))]
fn read_commit(&self, id: &CommitId) -> BackendResult<Commit> { fn read_commit(&self, id: &CommitId) -> BackendResult<Commit> {
if *id == self.root_commit_id { if *id == self.root_commit_id {
return Ok(make_root_commit( return Ok(make_root_commit(
@ -698,25 +699,32 @@ impl Backend for GitBackend {
} }
let git_commit_id = validate_git_object_id(id)?; let git_commit_id = validate_git_object_id(id)?;
let locked_repo = self.repo.lock().unwrap(); let mut commit = {
let commit = locked_repo let locked_repo = self.repo.lock().unwrap();
.find_commit(git_commit_id) let git_commit = locked_repo
.map_err(|err| map_not_found_err(err, id))?; .find_commit(git_commit_id)
let mut commit = commit_from_git_without_root_parent(&commit, false); .map_err(|err| map_not_found_err(err, id))?;
commit_from_git_without_root_parent(&git_commit, false)
};
if commit.parents.is_empty() { if commit.parents.is_empty() {
commit.parents.push(self.root_commit_id.clone()); commit.parents.push(self.root_commit_id.clone());
}; };
let table = self.cached_extra_metadata_table()?; let table = self.cached_extra_metadata_table()?;
let extras = if let Some(extras) = table.get_value(id.as_bytes()) {
table deserialize_extras(&mut commit, extras);
.get_value(id.as_bytes()) } else {
.ok_or_else(|| BackendError::ObjectNotFound { // TODO: Remove this hack and map to ObjectNotFound error if we're sure that
object_type: id.object_type(), // there are no reachable ancestor commits without extras metadata. Git commits
hash: id.hex(), // imported by jj < 0.8.0 might not have extras (#924).
source: "Not imported from Git".into(), // https://github.com/martinvonz/jj/issues/2343
})?; tracing::info!("unimported Git commit found");
deserialize_extras(&mut commit, extras); let uses_tree_conflict_format = false;
self.import_head_commits([id], uses_tree_conflict_format)?;
let table = self.cached_extra_metadata_table()?;
let extras = table.get_value(id.as_bytes()).unwrap();
deserialize_extras(&mut commit, extras);
}
Ok(commit) Ok(commit)
} }
@ -1079,6 +1087,44 @@ mod tests {
} }
} }
#[test]
fn read_git_commit_without_importing() {
let temp_dir = testutils::new_temp_dir();
let store_path = temp_dir.path();
let git_repo_path = temp_dir.path().join("git");
let git_repo = git2::Repository::init(&git_repo_path).unwrap();
let signature = git2::Signature::now("Someone", "someone@example.com").unwrap();
let empty_tree_id = Oid::from_str("4b825dc642cb6eb9a060e54bf8d69288fbee4904").unwrap();
let empty_tree = git_repo.find_tree(empty_tree_id).unwrap();
let git_commit_id = git_repo
.commit(
Some("refs/heads/main"),
&signature,
&signature,
"git commit message",
&empty_tree,
&[],
)
.unwrap();
let backend = GitBackend::init_external(store_path, &git_repo_path).unwrap();
// read_commit() without import_head_commits() works as of now. This might be
// changed later.
assert!(backend
.read_commit(&CommitId::from_bytes(git_commit_id.as_bytes()))
.is_ok());
assert!(
backend
.cached_extra_metadata_table()
.unwrap()
.get_value(git_commit_id.as_bytes())
.is_some(),
"extra metadata should have been be created"
);
}
#[test] #[test]
fn read_empty_string_placeholder() { fn read_empty_string_placeholder() {
let git_signature1 = git2::Signature::new( let git_signature1 = git2::Signature::new(