Skip to content

Commit

Permalink
git: on fetch, pin visible untracked remote refs
Browse files Browse the repository at this point in the history
This implements the other workaround described in 57167ce "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
  • Loading branch information
yuja committed Apr 14, 2024
1 parent 82c85ba commit aaa2025
Show file tree
Hide file tree
Showing 2 changed files with 173 additions and 11 deletions.
39 changes: 28 additions & 11 deletions lib/src/git.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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<Item = &CommitId> {
fn pinned_commit_ids(view: &View) -> Vec<CommitId> {
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<CommitId> {
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.
Expand Down
145 changes: 145 additions & 0 deletions lib/tests/test_git.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down

0 comments on commit aaa2025

Please sign in to comment.