git: on fetch, pin visible untracked remote refs

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

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<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.

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.