From aaa2025dfc86129b9edba8277e364ea637e8af6e Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Fri, 12 Apr 2024 21:16:08 +0900 Subject: [PATCH] git: on fetch, pin visible untracked remote refs This implements the other workaround described in 57167cefda93 "git: on import_refs(), don't abandon ancestors of newly fetched refs": > I think there are two ways to fix the problem: > a. pin non-tracking remote branches just like local refs > b. pin newly fetched refs in addition to local refs > This patch implements (b) because it's simpler and more obvious that the > fetched commits would never be abandoned immediately. The idea of (a) is that untracked remote branches are independent read-only refs, and read-only branches shouldn't be rewritten implicitly. Once the branch gets rewritten or abandoned by user, these remote refs will be hidden, and won't be pinned anymore. Since (a) effectively supersedes (b), this patch also removes the original workaround. Fixes #3495 --- lib/src/git.rs | 39 ++++++++---- lib/tests/test_git.rs | 145 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 173 insertions(+), 11 deletions(-) diff --git a/lib/src/git.rs b/lib/src/git.rs index ee3748c18..9a082d276 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -350,17 +350,17 @@ fn abandon_unreachable_commits( if hidable_git_heads.is_empty() { return vec![]; } - let pinned_heads = itertools::chain!( - changed_remote_refs - .values() - .flat_map(|(_, new_target)| new_target.added_ids()), - pinned_commit_ids(mut_repo.view()), - iter::once(mut_repo.store().root_commit_id()), - ) - .cloned() - .collect_vec(); - let abandoned_expression = RevsetExpression::commits(pinned_heads) + let pinned_expression = RevsetExpression::union_all(&[ + // Local refs are usually visible, no need to filter out hidden + RevsetExpression::commits(pinned_commit_ids(mut_repo.view())), + RevsetExpression::commits(remotely_pinned_commit_ids(mut_repo.view())) + // Hidden remote branches should not contribute to pinning + .intersection(&RevsetExpression::visible_heads().ancestors()), + RevsetExpression::root(), + ]); + let abandoned_expression = pinned_expression .range(&RevsetExpression::commits(hidable_git_heads)) + // Don't include already-abandoned commits in GitImportStats .intersection(&RevsetExpression::visible_heads().ancestors()); let abandoned_commits = abandoned_expression .evaluate_programmatic(mut_repo) @@ -500,13 +500,30 @@ fn default_remote_ref_state_for(ref_name: &RefName, git_settings: &GitSettings) /// On `import_refs()`, this is similar to collecting commits referenced by /// `view.git_refs()`. Main difference is that local branches can be moved by /// tracking remotes, and such mutation isn't applied to `view.git_refs()` yet. -fn pinned_commit_ids(view: &View) -> impl Iterator { +fn pinned_commit_ids(view: &View) -> Vec { itertools::chain!( view.local_branches().map(|(_, target)| target), view.tags().values(), iter::once(view.git_head()), ) .flat_map(|target| target.added_ids()) + .cloned() + .collect() +} + +/// Commits referenced by untracked remote branches including hidden ones. +/// +/// Tracked remote branches aren't included because they should have been merged +/// into the local counterparts, and the changes pulled from one remote should +/// propagate to the other remotes on later push. OTOH, untracked remote +/// branches are considered independent refs. +fn remotely_pinned_commit_ids(view: &View) -> Vec { + view.all_remote_branches() + .filter(|(_, remote_ref)| !remote_ref.is_tracking()) + .map(|(_, remote_ref)| &remote_ref.target) + .flat_map(|target| target.added_ids()) + .cloned() + .collect() } /// Imports `HEAD@git` from the underlying Git repo. diff --git a/lib/tests/test_git.rs b/lib/tests/test_git.rs index 5bdf2c097..546b751a2 100644 --- a/lib/tests/test_git.rs +++ b/lib/tests/test_git.rs @@ -776,6 +776,151 @@ fn test_import_refs_reimport_with_moved_untracked_remote_ref() { ); } +#[test] +fn test_import_refs_reimport_with_deleted_untracked_intermediate_remote_ref() { + let settings = testutils::user_settings(); + let git_settings = GitSettings { + auto_local_branch: false, + ..Default::default() + }; + let test_workspace = TestRepo::init_with_backend(TestRepoBackend::Git); + let repo = &test_workspace.repo; + let git_repo = get_git_repo(repo); + + // Set up linear graph: + // o feature-b@origin + // o feature-a@origin + let remote_ref_name_a = "refs/remotes/origin/feature-a"; + let remote_ref_name_b = "refs/remotes/origin/feature-b"; + let commit_remote_a = empty_git_commit(&git_repo, remote_ref_name_a, &[]); + let commit_remote_b = empty_git_commit(&git_repo, remote_ref_name_b, &[&commit_remote_a]); + let mut tx = repo.start_transaction(&settings); + git::import_refs(tx.mut_repo(), &git_settings).unwrap(); + tx.mut_repo().rebase_descendants(&settings).unwrap(); + let repo = tx.commit("test"); + let view = repo.view(); + + assert_eq!(*view.heads(), hashset! { jj_id(&commit_remote_b) }); + assert_eq!(view.local_branches().count(), 0); + assert_eq!(view.all_remote_branches().count(), 2); + assert_eq!( + view.get_remote_branch("feature-a", "origin"), + &RemoteRef { + target: RefTarget::normal(jj_id(&commit_remote_a)), + state: RemoteRefState::New, + }, + ); + assert_eq!( + view.get_remote_branch("feature-b", "origin"), + &RemoteRef { + target: RefTarget::normal(jj_id(&commit_remote_b)), + state: RemoteRefState::New, + }, + ); + + // Delete feature-a remotely and fetch the changes. + delete_git_ref(&git_repo, remote_ref_name_a); + let mut tx = repo.start_transaction(&settings); + git::import_refs(tx.mut_repo(), &git_settings).unwrap(); + tx.mut_repo().rebase_descendants(&settings).unwrap(); + let repo = tx.commit("test"); + let view = repo.view(); + + // No commits should be abandoned because feature-a is pinned by feature-b. + // Otherwise, feature-b would have to be rebased locally even though the + // user haven't made any modifications to these commits yet. + assert_eq!(*view.heads(), hashset! { jj_id(&commit_remote_b) }); + assert_eq!(view.local_branches().count(), 0); + assert_eq!(view.all_remote_branches().count(), 1); + assert_eq!( + view.get_remote_branch("feature-b", "origin"), + &RemoteRef { + target: RefTarget::normal(jj_id(&commit_remote_b)), + state: RemoteRefState::New, + }, + ); +} + +#[test] +fn test_import_refs_reimport_with_deleted_abandoned_untracked_remote_ref() { + let settings = testutils::user_settings(); + let git_settings = GitSettings { + auto_local_branch: false, + ..Default::default() + }; + let test_workspace = TestRepo::init_with_backend(TestRepoBackend::Git); + let repo = &test_workspace.repo; + let git_repo = get_git_repo(repo); + + // Set up linear graph: + // o feature-b@origin + // o feature-a@origin + let remote_ref_name_a = "refs/remotes/origin/feature-a"; + let remote_ref_name_b = "refs/remotes/origin/feature-b"; + let commit_remote_a = empty_git_commit(&git_repo, remote_ref_name_a, &[]); + let commit_remote_b = empty_git_commit(&git_repo, remote_ref_name_b, &[&commit_remote_a]); + let mut tx = repo.start_transaction(&settings); + git::import_refs(tx.mut_repo(), &git_settings).unwrap(); + tx.mut_repo().rebase_descendants(&settings).unwrap(); + let repo = tx.commit("test"); + let view = repo.view(); + + assert_eq!(*view.heads(), hashset! { jj_id(&commit_remote_b) }); + assert_eq!(view.local_branches().count(), 0); + assert_eq!(view.all_remote_branches().count(), 2); + assert_eq!( + view.get_remote_branch("feature-a", "origin"), + &RemoteRef { + target: RefTarget::normal(jj_id(&commit_remote_a)), + state: RemoteRefState::New, + }, + ); + assert_eq!( + view.get_remote_branch("feature-b", "origin"), + &RemoteRef { + target: RefTarget::normal(jj_id(&commit_remote_b)), + state: RemoteRefState::New, + }, + ); + + // Abandon feature-b locally: + // x feature-b@origin (hidden) + // o feature-a@origin + let mut tx = repo.start_transaction(&settings); + tx.mut_repo() + .record_abandoned_commit(jj_id(&commit_remote_b)); + tx.mut_repo().rebase_descendants(&settings).unwrap(); + let repo = tx.commit("test"); + let view = repo.view(); + assert_eq!(*view.heads(), hashset! { jj_id(&commit_remote_a) }); + assert_eq!(view.local_branches().count(), 0); + assert_eq!(view.all_remote_branches().count(), 2); + + // Delete feature-a remotely and fetch the changes. + delete_git_ref(&git_repo, remote_ref_name_a); + let mut tx = repo.start_transaction(&settings); + git::import_refs(tx.mut_repo(), &git_settings).unwrap(); + tx.mut_repo().rebase_descendants(&settings).unwrap(); + let repo = tx.commit("test"); + let view = repo.view(); + + // The feature-a commit should be abandoned. Since feature-b has already + // been abandoned, there are no descendant commits to be rebased. + assert_eq!( + *view.heads(), + hashset! { repo.store().root_commit_id().clone() } + ); + assert_eq!(view.local_branches().count(), 0); + assert_eq!(view.all_remote_branches().count(), 1); + assert_eq!( + view.get_remote_branch("feature-b", "origin"), + &RemoteRef { + target: RefTarget::normal(jj_id(&commit_remote_b)), + state: RemoteRefState::New, + }, + ); +} + #[test] fn test_import_refs_reimport_git_head_with_fixed_ref() { // Simulate external `git checkout` in colocated repo, from named branch.