diff --git a/CHANGELOG.md b/CHANGELOG.md index 0cf0a8ec8..6ad5b81c2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,6 +54,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `$SSH_AUTH_SOCK` is set). This should help at least macOS users where ssh-agent is launched by default and only `$SSH_AUTH_SOCK` is set. +* When importing from a git, any commits that are no longer referenced on the + git side will now be abandoned on the jj side as well. That means that + `jj git fetch` will now abandon unreferenced commits and rebase any local + changes you had on top. + ### Fixed bugs * When rebasing a conflict where one side modified a file and the other side diff --git a/lib/src/git.rs b/lib/src/git.rs index 0403dc7a6..1118856b2 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -57,9 +57,14 @@ pub fn import_refs( git_repo: &git2::Repository, ) -> Result<(), GitImportError> { let store = mut_repo.store().clone(); - let git_refs = git_repo.references()?; let mut existing_git_refs = mut_repo.view().git_refs().clone(); + let old_git_heads: HashSet<_> = existing_git_refs + .values() + .flat_map(|old_target| old_target.adds()) + .collect(); + let mut new_git_heads = HashSet::new(); let mut changed_git_refs = BTreeMap::new(); + let git_refs = git_repo.references()?; for git_ref in git_refs { let git_ref = git_ref?; if !(git_ref.is_tag() || git_ref.is_branch() || git_ref.is_remote()) @@ -83,6 +88,7 @@ pub fn import_refs( } }; let id = CommitId::from_bytes(git_commit.id().as_bytes()); + new_git_heads.insert(id.clone()); // TODO: Make it configurable which remotes are publishing and update public // heads here. mut_repo.set_git_ref(full_name.clone(), RefTarget::Normal(id.clone())); @@ -96,10 +102,6 @@ pub fn import_refs( } for (full_name, target) in existing_git_refs { mut_repo.remove_git_ref(&full_name); - // TODO: We should probably also remove heads pointing to the same - // commits and commits no longer reachable from other refs. - // If the underlying git repo has a branch that gets rewritten, we - // should probably not keep the commits it used to point to. changed_git_refs.insert(full_name, (Some(target), None)); } for (full_name, (old_git_target, new_git_target)) in changed_git_refs { @@ -117,6 +119,24 @@ pub fn import_refs( } } } + + // Find commits that are no longer referenced in the git repo and abandon them + // in jj as well. + let old_git_heads = old_git_heads.into_iter().collect_vec(); + let new_git_heads = new_git_heads.into_iter().collect_vec(); + // We could use mut_repo.record_rewrites() here but we know we only need to care + // about abandoned commits for now. We may want to change this if we ever + // add a way of preserving change IDs across rewrites by `git` (e.g. by + // putting them in the commit message). + let abandoned_commits = mut_repo + .index() + .walk_revs(&old_git_heads, &new_git_heads) + .map(|entry| entry.commit_id()) + .collect_vec(); + for abandoned_commit in abandoned_commits { + mut_repo.record_abandoned_commit(abandoned_commit); + } + // TODO: Should this be a separate function? We may not always want to import // the Git HEAD (and add it to our set of heads). if let Ok(head_git_commit) = git_repo diff --git a/lib/tests/test_git.rs b/lib/tests/test_git.rs index beeadf20c..992633698 100644 --- a/lib/tests/test_git.rs +++ b/lib/tests/test_git.rs @@ -74,6 +74,7 @@ fn test_import_refs() { let git_repo = repo.store().git_repo().unwrap(); let mut tx = repo.start_transaction("test"); git::import_refs(tx.mut_repo(), &git_repo).unwrap(); + tx.mut_repo().rebase_descendants(&settings); let repo = tx.commit(); let view = repo.view(); @@ -150,7 +151,7 @@ fn test_import_refs_reimport() { let commit1 = empty_git_commit(&git_repo, "refs/heads/main", &[]); git_ref(&git_repo, "refs/remotes/origin/main", commit1.id()); let commit2 = empty_git_commit(&git_repo, "refs/heads/main", &[&commit1]); - let commit3 = empty_git_commit(&git_repo, "refs/heads/feature1", &[&commit2]); + let _commit3 = empty_git_commit(&git_repo, "refs/heads/feature1", &[&commit2]); let commit4 = empty_git_commit(&git_repo, "refs/heads/feature2", &[&commit2]); let pgp_key_oid = git_repo.blob(b"my PGP key").unwrap(); git_repo @@ -159,6 +160,7 @@ fn test_import_refs_reimport() { let mut tx = repo.start_transaction("test"); git::import_refs(tx.mut_repo(), &git_repo).unwrap(); + tx.mut_repo().rebase_descendants(&settings); let repo = tx.commit(); // Delete feature1 and rewrite feature2 @@ -179,13 +181,11 @@ fn test_import_refs_reimport() { let mut tx = repo.start_transaction("test"); git::import_refs(tx.mut_repo(), &git_repo).unwrap(); + tx.mut_repo().rebase_descendants(&settings); let repo = tx.commit(); let view = repo.view(); - // TODO: commit3 and commit4 should probably be removed let expected_heads = hashset! { - commit_id(&commit3), - commit_id(&commit4), commit_id(&commit5), commit6.id().clone(), }; @@ -245,6 +245,7 @@ fn test_import_refs_reimport_head_removed() { let commit = empty_git_commit(&git_repo, "refs/heads/main", &[]); let mut tx = repo.start_transaction("test"); git::import_refs(tx.mut_repo(), &git_repo).unwrap(); + tx.mut_repo().rebase_descendants(&settings); let commit_id = CommitId::from_bytes(commit.id().as_bytes()); // Test the setup assert!(tx.mut_repo().view().heads().contains(&commit_id)); @@ -252,6 +253,7 @@ fn test_import_refs_reimport_head_removed() { // Remove the head and re-import tx.mut_repo().remove_head(&commit_id); git::import_refs(tx.mut_repo(), &git_repo).unwrap(); + tx.mut_repo().rebase_descendants(&settings); assert!(!tx.mut_repo().view().heads().contains(&commit_id)); } @@ -299,6 +301,7 @@ fn test_import_refs_empty_git_repo() { let heads_before = test_data.repo.view().heads().clone(); let mut tx = test_data.repo.start_transaction("test"); git::import_refs(tx.mut_repo(), &test_data.git_repo).unwrap(); + tx.mut_repo().rebase_descendants(&test_data.settings); let repo = tx.commit(); assert_eq!(*repo.view().heads(), heads_before); assert_eq!(repo.view().branches().len(), 0); @@ -323,6 +326,7 @@ fn test_import_refs_detached_head() { let mut tx = test_data.repo.start_transaction("test"); git::import_refs(tx.mut_repo(), &test_data.git_repo).unwrap(); + tx.mut_repo().rebase_descendants(&test_data.settings); let repo = tx.commit(); let expected_heads = hashset! { @@ -345,6 +349,7 @@ fn test_export_refs_initial() { git_repo.set_head("refs/heads/main").unwrap(); let mut tx = test_data.repo.start_transaction("test"); git::import_refs(tx.mut_repo(), &git_repo).unwrap(); + tx.mut_repo().rebase_descendants(&test_data.settings); test_data.repo = tx.commit(); // The first export shouldn't do anything @@ -366,6 +371,7 @@ fn test_export_refs_no_op() { let mut tx = test_data.repo.start_transaction("test"); git::import_refs(tx.mut_repo(), &git_repo).unwrap(); + tx.mut_repo().rebase_descendants(&test_data.settings); test_data.repo = tx.commit(); assert_eq!(git::export_refs(&test_data.repo, &git_repo), Ok(())); @@ -392,6 +398,7 @@ fn test_export_refs_branch_changed() { let mut tx = test_data.repo.start_transaction("test"); git::import_refs(tx.mut_repo(), &git_repo).unwrap(); + tx.mut_repo().rebase_descendants(&test_data.settings); test_data.repo = tx.commit(); assert_eq!(git::export_refs(&test_data.repo, &git_repo), Ok(())); @@ -427,6 +434,7 @@ fn test_export_refs_current_branch_changed() { git_repo.set_head("refs/heads/main").unwrap(); let mut tx = test_data.repo.start_transaction("test"); git::import_refs(tx.mut_repo(), &git_repo).unwrap(); + tx.mut_repo().rebase_descendants(&test_data.settings); test_data.repo = tx.commit(); assert_eq!(git::export_refs(&test_data.repo, &git_repo), Ok(())); @@ -460,6 +468,7 @@ fn test_export_refs_unborn_git_branch() { git_repo.set_head("refs/heads/main").unwrap(); let mut tx = test_data.repo.start_transaction("test"); git::import_refs(tx.mut_repo(), &git_repo).unwrap(); + tx.mut_repo().rebase_descendants(&test_data.settings); test_data.repo = tx.commit(); assert_eq!(git::export_refs(&test_data.repo, &git_repo), Ok(()));