git: look for predecessors also in locally reachable commits

When multiple bookmarked revisions exist in a stack, previously merged revisions
were not abandoned because they remained reachable. This patch fixes that by
rewriting merged revisions based on their change IDs and rebasing their
descendants onto the merged head.

The tricky part is that we shouldn't "rewrite" locally hidden revisions that
were remotely reachable (but no longer are). However, we still need to count
them as predecessors for a better evolution history.

There is a known issue where immutable descendants can accidentally be rebased
onto rewritten immutable parents. For example, suppose we have the history
A@origin <- B@origin and A@origin is rewritten remotely. If we fetch only
A'@origin, B@origin ends up being rebased onto A'@origin. Because remote
bookmarks do not move locally, this process creates a new, anonymous B revision.
One way to fix this would be to implement rebase_descendants() that leaves
immutable descendants untouched, while still allowing mutable descendants to be
rebased onto A'@origin.

cli/tests/test_git_fetch.rs

@@ -2234,6 +2234,91 @@ fn test_git_fetch_remotely_rewritten_no_synthetic_predecessors() {
     ");
 }
 
+#[test]
+fn test_git_fetch_remotely_rewritten_descendants() {
+    let test_env = TestEnvironment::default();
+
+    // Add bookmarked branches to the remote repo
+    test_env
+        .run_jj_in(".", ["git", "init", "remote", "--colocate"])
+        .success();
+    let remote_dir = test_env.work_dir("remote");
+    remote_dir.run_jj(["describe", "-moriginal"]).success();
+    remote_dir.run_jj(["new", "-mbookmarked 1"]).success();
+    remote_dir.run_jj(["bookmark", "set", "book1"]).success();
+    remote_dir.run_jj(["new", "@-", "-mbookmarked 2"]).success();
+    remote_dir.run_jj(["bookmark", "set", "book2"]).success();
+
+    // Check out the base revision
+    test_env
+        .run_jj_in(".", ["git", "clone", "remote", "local"])
+        .success();
+    let local_dir = test_env.work_dir("local");
+    local_dir
+        .run_jj(["new", "subject(original)", "-mlocal"])
+        .success();
+    insta::assert_snapshot!(get_log_output(&local_dir), @r#"
+    @  b4adc7786cf0 "local"
+    │ ◆  cce448c253e0 "bookmarked 2" book2@origin
+    ├─╯
+    │ ◆  2a6bbeb458de "bookmarked 1" book1@origin
+    ├─╯
+    ◆  97604bbedb48 "original"
+    ◆  000000000000 ""
+    [EOF]
+    "#);
+
+    // Rewrite the base revision and descendants remotely
+    remote_dir
+        .run_jj(["describe", "-r@-", "-mmodified"])
+        .success();
+
+    // Fetch one of the rewritten branches
+    let output = local_dir.run_jj(["git", "fetch", "--branch=book1"]);
+    insta::assert_snapshot!(output, @"
+    ------- stderr -------
+    bookmark: book1@origin [updated] untracked
+    Updated 2 rewritten commits.
+    Rebased 2 descendant commits
+    Working copy  (@) now at: vruxwmqv a1d01244 (empty) local
+    Parent commit (@-)      : qpvuntsm a843bfad (empty) modified
+    [EOF]
+    ");
+
+    // The working copy should be rebased onto the modified revision
+    // FIXME: the other remote branch shouldn't be rebased
+    insta::assert_snapshot!(get_log_output(&local_dir), @r#"
+    @  a1d01244a4ec "local"
+    │ ○  8e6c17fa9e2e "bookmarked 2"
+    ├─╯
+    │ ◆  ad5c5f3c59a7 "bookmarked 1" book1@origin
+    ├─╯
+    ◆  a843bfad2abb "modified"
+    ◆  000000000000 ""
+    [EOF]
+    "#);
+
+    // Evolution history should point to the "git fetch" operation
+    let output = local_dir.run_jj(["evolog", "-r..remote_bookmarks()"]);
+    insta::assert_snapshot!(output, @"
+    ◆  kkmpptxz test.user@example.com 2001-02-03 08:05:16 book1@origin ad5c5f3c
+    │  (empty) bookmarked 1
+    │  -- operation ac34b601b3a2 fetch from git remote(s) origin
+    ○  kkmpptxz/1 test.user@example.com 2001-02-03 08:05:09 2a6bbeb4 (hidden)
+       (empty) bookmarked 1
+    ◆  qpvuntsm test.user@example.com 2001-02-03 08:05:16 a843bfad
+    │  (empty) modified
+    │  -- operation ac34b601b3a2 fetch from git remote(s) origin
+    ◆  qpvuntsm/1 test.user@example.com 2001-02-03 08:05:08 97604bbe (hidden)
+       (empty) original
+    ◆  mzvwutvl/1 test.user@example.com 2001-02-03 08:05:11 book2@origin cce448c2 (hidden)
+       (empty) bookmarked 2
+    ◆  qpvuntsm/1 test.user@example.com 2001-02-03 08:05:08 97604bbe (hidden)
+       (empty) original
+    [EOF]
+    ");
+}
+
 #[test]
 fn test_git_fetch_tracked() {
     let test_env = TestEnvironment::default();

lib/src/git.rs

@@ -722,9 +722,11 @@ async fn import_refs_inner(
         record_synthetic_predecessors(
             mut_repo,
             &old_visible_heads,
-            &new_referenced_heads,
-            &abandoned_commits,
+            Diff::new(&old_referenced_heads, &new_referenced_heads),
             &imported_commits,
+            // TODO: Maybe enable rewriting unconditionally? This should be more
+            // reliable than reachability-based heuristic.
+            options.abandon_unreachable_commits,
         )
         .await?
     } else {
@@ -781,47 +783,49 @@ async fn abandon_unreachable_commits(
 async fn record_synthetic_predecessors(
     mut_repo: &mut MutableRepo,
     old_visible_heads: &Arc<ResolvedRevsetExpression>,
-    new_referenced_heads: &Arc<ResolvedRevsetExpression>,
-    abandoned_commits: &[Commit],
+    Diff {
+        before: old_referenced_heads,
+        after: new_referenced_heads,
+    }: Diff<&Arc<ResolvedRevsetExpression>>,
     imported_commits: &[Commit],
+    rewrite_commits: bool,
 ) -> Result<HashSet<CommitId>, GitImportError> {
-    let new_referenced_change_to_commit_ids = {
+    let build_change_to_commit_ids_map = async |expr: Arc<ResolvedRevsetExpression>| {
         let mut change_to_commit_ids: HashMap<ChangeId, Vec<CommitId>> = HashMap::new();
-        let mut stream = old_visible_heads
-            .range(new_referenced_heads)
-            .evaluate(mut_repo)?
-            .commit_change_ids();
+        let mut stream = expr.evaluate(mut_repo)?.commit_change_ids();
         while let Some((commit_id, change_id)) = stream.try_next().await? {
             let commit_ids = change_to_commit_ids.entry(change_id).or_default();
             commit_ids.push(commit_id);
         }
-        change_to_commit_ids
-    };
-    // TODO: Maybe we can use new_referenced_heads..old_referenced_heads as
-    // predecessor candidates. If abandon_unreachable_commits is set, we can
-    // mark them as "rewritten" in addition to the current abandoned commits.
-    let abandoned_change_to_commit_ids = {
-        let mut change_to_commit_ids: HashMap<&ChangeId, Vec<&CommitId>> = HashMap::new();
-        for commit in abandoned_commits {
-            let commit_ids = change_to_commit_ids.entry(commit.change_id()).or_default();
-            commit_ids.push(commit.id());
-        }
-        change_to_commit_ids
+        Ok::<_, GitImportError>(change_to_commit_ids)
     };
+    // TODO: Commits within new_referenced_heads..old_referenced_heads may have
+    // both mutable and immutable descendants. Rebasing the immutable
+    // descendants onto the new parents would be unexpected.
+    let old_referenced_change_to_commit_ids =
+        build_change_to_commit_ids_map(new_referenced_heads.range(old_referenced_heads)).await?;
+    let new_referenced_change_to_commit_ids =
+        build_change_to_commit_ids_map(old_visible_heads.range(new_referenced_heads)).await?;
     let imported_commit_ids: HashSet<_> = imported_commits.iter().map(Commit::id).collect();
-    debug_assert!(
-        new_referenced_change_to_commit_ids
-            .values()
-            .flatten()
-            .all(|new_id| abandoned_commits.iter().all(|old| old.id() != new_id)),
-        "new referenced commits should never be reachable from old refs"
-    );
+    let rewritable_commit_ids: HashSet<_> = if rewrite_commits {
+        // Similar to old_referenced_change_to_commit_ids, but doesn't include
+        // previously abandoned commits, which shouldn't be rewritten again.
+        new_referenced_heads
+            .range(old_referenced_heads)
+            .intersection(&old_visible_heads.ancestors())
+            .evaluate(mut_repo)?
+            .stream()
+            .try_collect()
+            .await?
+    } else {
+        HashSet::new()
+    };
 
     let mut rewritten_commit_ids = HashSet::new();
     for (change_id, new_commit_ids) in &new_referenced_change_to_commit_ids {
         let predecessor_id: Option<CommitId>;
-        let rewrite_source_ids: &[&CommitId];
-        if let Some(old_commit_ids) = abandoned_change_to_commit_ids.get(change_id) {
+        let rewrite_source_ids: &[CommitId];
+        if let Some(old_commit_ids) = old_referenced_change_to_commit_ids.get(change_id) {
             // Pick the latest one if previously diverged. Divergence isn't
             // usually resolved by "squashing" the commits.
             predecessor_id = Some(old_commit_ids[0].clone());
@@ -841,17 +845,21 @@ async fn record_synthetic_predecessors(
         {
             mut_repo.set_predecessors(new_commit_id.clone(), predecessor_id.as_slice().to_vec());
         }
+        let rewrite_source_ids = rewrite_source_ids
+            .iter()
+            .filter(|id| rewritable_commit_ids.contains(id));
         if let [new_commit_id] = &**new_commit_ids {
-            for &old_commit_id in rewrite_source_ids {
+            for old_commit_id in rewrite_source_ids {
                 mut_repo.set_rewritten_commit(old_commit_id.clone(), new_commit_id.clone());
+                rewritten_commit_ids.insert(old_commit_id.clone());
             }
         } else {
-            for &old_commit_id in rewrite_source_ids {
+            for old_commit_id in rewrite_source_ids {
                 mut_repo
                     .set_divergent_rewrite(old_commit_id.clone(), new_commit_ids.iter().cloned());
+                rewritten_commit_ids.insert(old_commit_id.clone());
             }
         }
-        rewritten_commit_ids.extend(rewrite_source_ids.iter().map(|&id| id.clone()));
     }
 
     Ok(rewritten_commit_ids)

lib/tests/test_git.rs

@@ -1664,6 +1664,287 @@ fn test_import_refs_synthetic_predecessors_multiple_descendants() -> TestResult
     Ok(())
 }
 
+#[test_case(true, false; "without synthetic predecessors")]
+#[test_case(false, true; "with synthetic predecessors but no rewriting")]
+#[test_case(true, true; "with synthetic predecessors")]
+fn test_import_refs_synthetic_predecessors_bookmarked_simple(
+    abandon_unreachable_commits: bool,
+    record_synthetic_predecessors: bool,
+) -> TestResult {
+    let synthetic_rewrite_commits = abandon_unreachable_commits && record_synthetic_predecessors;
+    let test_repo = TestRepo::init_with_backend(TestRepoBackend::Git);
+    let main_repo = &test_repo.repo;
+    let ext_repo = init_external_git_repo(&test_repo, "ext-repo".as_ref())?;
+    let ext_store = ext_repo.store().clone();
+    let import_options = GitImportOptions {
+        abandon_unreachable_commits,
+        record_synthetic_predecessors,
+        ..default_import_options()
+    };
+
+    // Main:
+    // 2A
+    //  |
+    // 1A*
+    let mut tx = main_repo.start_transaction();
+    let commit1a = write_random_commit(tx.repo_mut());
+    let commit2a = write_random_commit_with_parents(tx.repo_mut(), &[&commit1a]);
+    tx.repo_mut()
+        .set_local_bookmark_target("1A".as_ref(), RefTarget::normal(commit1a.id().clone()));
+    git::export_refs(tx.repo_mut())?;
+    let main_repo = tx.commit("test").block_on()?;
+
+    // Ext: Rewrite 1A* -> 1B*
+    let mut tx = ext_repo.start_transaction();
+    let stats = git::import_refs(tx.repo_mut(), &import_options).block_on()?;
+    assert_eq!(stats.changed_remote_bookmarks.len(), 1);
+    let commit1b = rewrite_commit(tx.repo_mut(), &ext_store.get_commit(commit1a.id())?, "1B");
+    let num_rebased = tx.repo_mut().rebase_descendants().block_on()?;
+    assert_eq!(num_rebased, 0);
+    git::export_refs(tx.repo_mut())?;
+
+    // Main: Set bookmark 2A* (without importing)
+    let mut tx = main_repo.start_transaction();
+    tx.repo_mut()
+        .set_local_bookmark_target("2A".as_ref(), RefTarget::normal(commit2a.id().clone()));
+    git::export_refs(tx.repo_mut())?;
+
+    // Main: Import changes
+    //
+    // (synthetic_rewrite_commits = true)
+    // 2C*
+    //  |
+    // 1B*
+    //
+    // (synthetic_rewrite_commits = false)
+    // 2A
+    //  |
+    // 1A* 1B*
+    let stats = git::import_refs(tx.repo_mut(), &import_options).block_on()?;
+    assert_eq!(stats.abandoned_commits.len(), 0);
+    assert_eq!(
+        stats.rewritten_commit_ids.len(),
+        if synthetic_rewrite_commits { 1 } else { 0 }
+    );
+    assert_eq!(stats.changed_remote_bookmarks.len(), 1);
+    let num_rebased = tx.repo_mut().rebase_descendants().block_on()?;
+    assert_eq!(num_rebased, if synthetic_rewrite_commits { 1 } else { 0 });
+    let main_repo = tx.commit("test").block_on()?;
+
+    if synthetic_rewrite_commits {
+        let commit2c = find_unique_successor(&main_repo, commit2a.id()).unwrap();
+
+        // Sanity check for the new graph
+        assert_eq!(
+            *main_repo.view().heads(),
+            HashSet::from([&commit2c].map(|c| c.id().clone()))
+        );
+
+        // Synthetic predecessors should be recorded
+        assert_eq!(
+            main_repo.operation().predecessors_for_commit(commit1b.id()),
+            Some(slice::from_ref(commit1a.id()))
+        );
+        // Descendants should be rebased onto 1B
+        assert_eq!(commit2c.parent_ids(), slice::from_ref(commit1b.id()));
+    } else if record_synthetic_predecessors {
+        assert!(!abandon_unreachable_commits);
+        // Sanity check for the new graph
+        assert_eq!(
+            *main_repo.view().heads(),
+            HashSet::from([&commit2a, &commit1b].map(|c| c.id().clone()))
+        );
+
+        // Synthetic predecessors should be recorded
+        assert_eq!(
+            main_repo.operation().predecessors_for_commit(commit1b.id()),
+            Some(slice::from_ref(commit1a.id()))
+        );
+    } else {
+        // Sanity check for the new graph
+        assert_eq!(
+            *main_repo.view().heads(),
+            HashSet::from([&commit2a, &commit1b].map(|c| c.id().clone()))
+        );
+
+        // Synthetic predecessors shouldn't be recorded
+        assert_eq!(
+            main_repo.operation().predecessors_for_commit(commit1b.id()),
+            None
+        );
+    }
+
+    Ok(())
+}
+
+#[test]
+fn test_import_refs_synthetic_predecessors_some_bookmarked_descendants() -> TestResult {
+    let test_repo = TestRepo::init_with_backend(TestRepoBackend::Git);
+    let main_repo = &test_repo.repo;
+    let ext_repo = init_external_git_repo(&test_repo, "ext-repo".as_ref())?;
+    let ext_store = ext_repo.store().clone();
+    let import_options = default_import_options();
+
+    // Main:
+    // 3A  5A
+    //  | /
+    // 2A* 4A
+    //  | /
+    // 1A
+    let mut tx = main_repo.start_transaction();
+    let commit1a = write_random_commit(tx.repo_mut());
+    let commit2a = write_random_commit_with_parents(tx.repo_mut(), &[&commit1a]);
+    let commit3a = write_random_commit_with_parents(tx.repo_mut(), &[&commit2a]);
+    let commit4a = write_random_commit_with_parents(tx.repo_mut(), &[&commit1a]);
+    let commit5a = write_random_commit_with_parents(tx.repo_mut(), &[&commit2a]);
+    tx.repo_mut()
+        .set_local_bookmark_target("2A".as_ref(), RefTarget::normal(commit2a.id().clone()));
+    git::export_refs(tx.repo_mut())?;
+    let main_repo = tx.commit("test").block_on()?;
+
+    // Ext: Rewrite 1A -> 1B, 2A* -> 2B*
+    let mut tx = ext_repo.start_transaction();
+    let stats = git::import_refs(tx.repo_mut(), &import_options).block_on()?;
+    assert_eq!(stats.changed_remote_bookmarks.len(), 1);
+    let commit1b = rewrite_commit(tx.repo_mut(), &ext_store.get_commit(commit1a.id())?, "1B");
+    let num_rebased = tx.repo_mut().rebase_descendants().block_on()?;
+    assert_eq!(num_rebased, 1);
+    git::export_refs(tx.repo_mut())?;
+    let ext_repo = tx.commit("test").block_on()?;
+    let commit2b = find_unique_successor(&ext_repo, commit2a.id()).unwrap();
+
+    // Main: Set bookmark 3A* (without importing)
+    let mut tx = main_repo.start_transaction();
+    tx.repo_mut()
+        .set_local_bookmark_target("3A".as_ref(), RefTarget::normal(commit3a.id().clone()));
+    git::export_refs(tx.repo_mut())?;
+
+    // Main: Import changes
+    // 3C* 5C
+    //  | /
+    // 2B* 4C
+    //  | /
+    // 1B
+    let stats = git::import_refs(tx.repo_mut(), &import_options).block_on()?;
+    assert_eq!(stats.abandoned_commits.len(), 0);
+    assert_eq!(stats.rewritten_commit_ids.len(), 2);
+    assert_eq!(stats.changed_remote_bookmarks.len(), 1);
+    let num_rebased = tx.repo_mut().rebase_descendants().block_on()?;
+    assert_eq!(num_rebased, 3);
+    let main_repo = tx.commit("test").block_on()?;
+    let commit3c = find_unique_successor(&main_repo, commit3a.id()).unwrap();
+    let commit4c = find_unique_successor(&main_repo, commit4a.id()).unwrap();
+    let commit5c = find_unique_successor(&main_repo, commit5a.id()).unwrap();
+
+    // Sanity check for the new graph
+    assert_eq!(
+        *main_repo.view().heads(),
+        HashSet::from([&commit3c, &commit4c, &commit5c].map(|c| c.id().clone()))
+    );
+
+    // Synthetic predecessors should be recorded
+    assert_eq!(
+        main_repo.operation().predecessors_for_commit(commit1b.id()),
+        Some(slice::from_ref(commit1a.id()))
+    );
+    assert_eq!(
+        main_repo.operation().predecessors_for_commit(commit2b.id()),
+        Some(slice::from_ref(commit2a.id()))
+    );
+    // Descendants should be rebased onto 2B
+    assert_eq!(commit3c.parent_ids(), slice::from_ref(commit2b.id()));
+    assert_eq!(commit4c.parent_ids(), slice::from_ref(commit1b.id()));
+    assert_eq!(commit5c.parent_ids(), slice::from_ref(commit2b.id()));
+
+    Ok(())
+}
+
+#[test]
+fn test_import_refs_synthetic_predecessors_rewritten_bookmarked_descendants() -> TestResult {
+    let test_repo = TestRepo::init_with_backend(TestRepoBackend::Git);
+    let main_repo = &test_repo.repo;
+    let ext_repo = init_external_git_repo(&test_repo, "ext-repo".as_ref())?;
+    let ext_store = ext_repo.store().clone();
+    let import_options = default_import_options();
+
+    // Main:
+    // 2A*
+    //  |
+    // 1A*
+    let mut tx = main_repo.start_transaction();
+    let commit1a = write_random_commit(tx.repo_mut());
+    let commit2a = write_random_commit_with_parents(tx.repo_mut(), &[&commit1a]);
+    tx.repo_mut()
+        .set_local_bookmark_target("1A".as_ref(), RefTarget::normal(commit1a.id().clone()));
+    tx.repo_mut()
+        .set_local_bookmark_target("2A".as_ref(), RefTarget::normal(commit2a.id().clone()));
+    git::export_refs(tx.repo_mut())?;
+    let main_repo = tx.commit("test").block_on()?;
+
+    // Ext: Rewrite 1A* -> 1B*, 2A* -> 2B*
+    let mut tx = ext_repo.start_transaction();
+    let stats = git::import_refs(tx.repo_mut(), &import_options).block_on()?;
+    assert_eq!(stats.changed_remote_bookmarks.len(), 2);
+    let commit1b = rewrite_commit(tx.repo_mut(), &ext_store.get_commit(commit1a.id())?, "1B");
+    let num_rebased = tx.repo_mut().rebase_descendants().block_on()?;
+    assert_eq!(num_rebased, 1);
+    git::export_refs(tx.repo_mut())?;
+    let ext_repo = tx.commit("test").block_on()?;
+    let commit2b = find_unique_successor(&ext_repo, commit2a.id()).unwrap();
+
+    // Main: Rewrite 2A* -> 2C*, Add 3C* (without importing)
+    // 2C* 3C*
+    //  | /
+    // 1A*
+    let mut tx = main_repo.start_transaction();
+    let commit2c = rewrite_commit(tx.repo_mut(), &commit2a, "2C");
+    let commit3c = write_random_commit_with_parents(tx.repo_mut(), &[&commit1a]);
+    tx.repo_mut()
+        .set_local_bookmark_target("2C".as_ref(), RefTarget::normal(commit2c.id().clone()));
+    tx.repo_mut()
+        .set_local_bookmark_target("3C".as_ref(), RefTarget::normal(commit3c.id().clone()));
+    let num_rebased = tx.repo_mut().rebase_descendants().block_on()?;
+    assert_eq!(num_rebased, 0);
+
+    // Main: Import changes
+    // 2D* 3D* 2B*
+    //    \ | /
+    //     1B*
+    let stats = git::import_refs(tx.repo_mut(), &import_options).block_on()?;
+    assert_eq!(stats.abandoned_commits.len(), 0);
+    assert_eq!(
+        stats.rewritten_commit_ids.len(),
+        1 // 2B/2C isn't included because it has already been rewritten locally
+    );
+    assert_eq!(stats.changed_remote_bookmarks.len(), 2);
+    let num_rebased = tx.repo_mut().rebase_descendants().block_on()?;
+    assert_eq!(num_rebased, 2);
+    let main_repo = tx.commit("test").block_on()?;
+    let commit2d = find_unique_successor(&main_repo, commit2c.id()).unwrap();
+    let commit3d = find_unique_successor(&main_repo, commit3c.id()).unwrap();
+
+    // Sanity check for the new graph
+    assert_eq!(
+        *main_repo.view().heads(),
+        HashSet::from([&commit2d, &commit3d, &commit2b].map(|c| c.id().clone()))
+    );
+
+    // Synthetic predecessors should be recorded
+    assert_eq!(
+        main_repo.operation().predecessors_for_commit(commit1b.id()),
+        Some(slice::from_ref(commit1a.id()))
+    );
+    assert_eq!(
+        main_repo.operation().predecessors_for_commit(commit2b.id()),
+        Some(slice::from_ref(commit2a.id()))
+    );
+    // Descendants should be rebased onto 1B
+    assert_eq!(commit2d.parent_ids(), slice::from_ref(commit1b.id()));
+    assert_eq!(commit3d.parent_ids(), slice::from_ref(commit1b.id()));
+
+    Ok(())
+}
+
 #[test]
 fn test_import_refs_synthetic_predecessors_old_divergent() -> TestResult {
     let test_repo = TestRepo::init_with_backend(TestRepoBackend::Git);