Skip to content

Commit

Permalink
git: when importing refs, abandon commits that were abandoned in git
Browse files Browse the repository at this point in the history 
Now that I'm using GitHub PRs instead of pushing directly to the main
branch, it's quite annoying to have to abandon the old commits after
GitHub rebases them. This patch makes it so we compare the remote's
previous heads to the new heads and abandons any commits that were
removed on the remote. As usual, that means that descendants get
rebased onto the closest remaining commit.

This is half of #241. The other half is to detect rewritten branches
and rebase on top.
  • Loading branch information
@martinvonz
martinvonz committed Apr 28, 2022
1 parent e3254fa commit 57ba9a9
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 9 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
30 changes: 25 additions & 5 deletions lib/src/git.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand All @@ -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()));
Expand All @@ -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 {
Expand All @@ -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
Expand Down
17 changes: 13 additions & 4 deletions lib/tests/test_git.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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(),
};
Expand Down Expand Up @@ -245,13 +245,15 @@ 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));

// 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));
}

Expand Down Expand Up @@ -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);
Expand All @@ -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! {
Expand All @@ -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
Expand All @@ -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(()));
Expand All @@ -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(()));
Expand Down Expand Up @@ -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(()));
Expand Down Expand Up @@ -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(()));
Expand Down

0 comments on commit 57ba9a9

Please sign in to comment.