rewrite: don't rebase immutable descendants of rewritten revisions

Commands like "jj git fetch"/"import" can update immutable commits to reflect
the remote changes. When this happens, we should avoid rebasing their immutable
descendants.

I also updated the doc comment for transform_commits(), which already supports
disconnected ranges. Because find_descendants_for_rebase() excludes commits
within parent_mappings, the graph isn't always connected (without this patch).

The new changelog entry also covers the changes from 44146561 "git: look for
predecessors also in locally reachable commits".

CHANGELOG.md

@@ -44,6 +44,11 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 * `jj gerrit upload` now supports the `-o` (`--option`) flag, which works like
   `git push -o` (`--push-option`).
 
+* `jj git fetch` now rebases the descendants of revisions that were rewritten
+  based on their change IDs. Previously, when multiple bookmarked revisions
+  existed in a stack, those rewritten revisions and their descendants wouldn't
+  always be rebased. Note that immutable descendants will not be rebased.
+
 ### Fixed bugs
 
 * `jj` now creates a new working-copy revision during snapshotting if the

cli/src/cli_util.rs

@@ -120,6 +120,7 @@ use jj_lib::revset::RevsetStreamExt as _;
 use jj_lib::revset::RevsetWorkspaceContext;
 use jj_lib::revset::SymbolResolverExtension;
 use jj_lib::revset::UserRevsetExpression;
+use jj_lib::rewrite::RebaseOptions;
 use jj_lib::rewrite::restore_tree;
 use jj_lib::settings::HumanByteSize;
 use jj_lib::settings::UserSettings;
@@ -1378,7 +1379,7 @@ impl WorkspaceCommandHelper {
         }
 
         let mut tx = tx.into_inner();
-        let num_rebased = tx.repo_mut().rebase_descendants().await?;
+        let num_rebased = rebase_mutable_descendants(&self.env, &mut tx).await?;
         if num_rebased > 0 {
             writeln!(
                 ui.status(),
@@ -2689,7 +2690,7 @@ impl WorkspaceCommandTransaction<'_> {
             writeln!(ui.status(), "Nothing changed.")?;
             return Ok(());
         }
-        let num_rebased = tx.repo_mut().rebase_descendants().await?;
+        let num_rebased = rebase_mutable_descendants(&helper.env, &mut tx).await?;
         if num_rebased > 0 {
             writeln!(ui.status(), "Rebased {num_rebased} descendant commits")?;
         }
@@ -2809,6 +2810,26 @@ pub fn start_repo_transaction(
     tx
 }
 
+async fn rebase_mutable_descendants(
+    env: &WorkspaceCommandEnvironment,
+    tx: &mut Transaction,
+) -> Result<usize, CommandError> {
+    // Commands like "jj git fetch" can update immutable commits to reflect the
+    // remote changes. Their immutable descendants shouldn't be rebased. We use
+    // tx.base_repo() here because we're interested in existing immutable
+    // commits that are still reachable.
+    let mut num_rebased = 0;
+    let immutable = env.resolve_immutable_expression(tx.base_repo().as_ref())?;
+    tx.repo_mut()
+        .rebase_descendants_with_options(
+            &immutable,
+            &RebaseOptions::default(),
+            |_old_commit, _rebased_commit| num_rebased += 1,
+        )
+        .await?;
+    Ok(num_rebased)
+}
+
 /// Check if the working copy is stale and reload the repo if the repo is ahead
 /// of the working copy.
 ///

cli/src/commands/abandon.rs

@@ -117,6 +117,7 @@ pub(crate) async fn cmd_abandon(
     tx.repo_mut()
         .transform_descendants_with_options(
             to_abandon.iter().cloned().collect(),
+            &RevsetExpression::none(),
             &HashMap::new(),
             &options,
             async |rewriter| {

cli/tests/test_git_clone.rs

@@ -1127,19 +1127,17 @@ fn test_git_clone_invalid_immutable_heads() {
     // Suppress lengthy warnings in commit summary template
     test_env.add_config("revsets.short-prefixes = ''");
 
-    // Even if there were an error about the invalid immutable_heads(), the
-    // error shouldn't be counted as an immutable working-copy commit.
+    // The error about the invalid immutable_heads() shouldn't be counted as an
+    // immutable working-copy commit.
     let output = root_dir.run_jj(["git", "clone", "source", "clone"]);
-    insta::assert_snapshot!(output, @r#"
+    insta::assert_snapshot!(output, @"
     ------- stderr -------
-    Fetching into new repo in "$TEST_ENV/clone"
-    bookmark: main@origin [new] tracked
-    Setting the revset alias `trunk()` to `main@origin`
-    Working copy  (@) now at: sqpuoqvx 1ca44815 (empty) (no description set)
-    Parent commit (@-)      : qomsplrm ebeb70d8 main | message
-    Added 1 files, modified 0 files, removed 0 files
+    Config error: Invalid `revset-aliases.immutable_heads()`
+    Caused by: Revision `unknown` doesn't exist
+    For help, see https://docs.jj-vcs.dev/latest/config/ or use `jj help -k config`.
     [EOF]
-    "#);
+    [exit status: 1]
+    ");
 }
 
 #[test]

cli/tests/test_git_fetch.rs

@@ -2279,17 +2279,41 @@ fn test_git_fetch_remotely_rewritten_descendants() {
     ------- stderr -------
     bookmark: book1@origin [updated] untracked
     Updated 2 rewritten commits.
-    Rebased 2 descendant commits
+    Rebased 1 descendant commits
     Working copy  (@) now at: vruxwmqv a1d01244 (empty) local
-    Parent commit (@-)      : qpvuntsm a843bfad (empty) modified
+    Parent commit (@-)      : qpvuntsm/0 a843bfad (divergent) (empty) modified
     [EOF]
     ");
 
-    // The working copy should be rebased onto the modified revision
-    // FIXME: the other remote branch shouldn't be rebased
+    // The working copy should be rebased onto the modified revision, but the
+    // other remote branch shouldn't
     insta::assert_snapshot!(get_log_output(&local_dir), @r#"
     @  a1d01244a4ec "local"
-    │ ○  8e6c17fa9e2e "bookmarked 2"
+    │ ◆  ad5c5f3c59a7 "bookmarked 1" book1@origin
+    ├─╯
+    ◆  a843bfad2abb "modified"
+    │ ◆  cce448c253e0 "bookmarked 2" book2@origin
+    │ ◆  97604bbedb48 "original"
+    ├─╯
+    ◆  000000000000 ""
+    [EOF]
+    "#);
+
+    // Fetch the other branch
+    let output = local_dir.run_jj(["git", "fetch"]);
+    insta::assert_snapshot!(output, @"
+    ------- stderr -------
+    bookmark: book2@origin [updated] untracked
+    Abandoned 1 commits that are no longer reachable:
+      qpvuntsm/1 97604bbe (divergent) (empty) original
+    Updated 1 rewritten commits.
+    [EOF]
+    ");
+
+    // Divergence should be resolved
+    insta::assert_snapshot!(get_log_output(&local_dir), @r#"
+    @  a1d01244a4ec "local"
+    │ ◆  3faff7724dd4 "bookmarked 2" book2@origin
     ├─╯
     │ ◆  ad5c5f3c59a7 "bookmarked 1" book1@origin
     ├─╯
@@ -2301,19 +2325,20 @@ fn test_git_fetch_remotely_rewritten_descendants() {
     // Evolution history should point to the "git fetch" operation
     let output = local_dir.run_jj(["evolog", "-r..remote_bookmarks()"]);
     insta::assert_snapshot!(output, @"
+    ◆  mzvwutvl test.user@example.com 2001-02-03 08:05:16 book2@origin 3faff772
+    │  (empty) bookmarked 2
+    │  -- operation 8285ffe9ef99 fetch from git remote(s) origin
+    ○  mzvwutvl/1 test.user@example.com 2001-02-03 08:05:11 cce448c2 (hidden)
+       (empty) bookmarked 2
     ◆  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
+    │  -- operation eed0c1c063f4 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)
+    │  -- operation eed0c1c063f4 fetch from git remote(s) origin
+    ○  qpvuntsm/1 test.user@example.com 2001-02-03 08:05:08 97604bbe (hidden)
        (empty) original
     [EOF]
     ");

lib/src/git.rs

@@ -799,9 +799,6 @@ async fn record_synthetic_predecessors(
         }
         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 =

lib/src/repo.rs

@@ -93,6 +93,7 @@ use crate::refs::diff_named_remote_refs;
 use crate::refs::merge_ref_targets;
 use crate::refs::merge_remote_refs;
 use crate::revset;
+use crate::revset::ResolvedRevsetExpression;
 use crate::revset::RevsetEvaluationError;
 use crate::revset::RevsetExpression;
 use crate::revset::RevsetStreamExt as _;
@@ -1260,13 +1261,15 @@ impl MutableRepo {
     }
 
     /// Find descendants of `root`, unless they've already been rewritten
-    /// (according to `parent_mapping`).
+    /// (according to `parent_mapping`) or are included in `immutable`.
     pub async fn find_descendants_for_rebase(
         &self,
         roots: Vec<CommitId>,
+        immutable: &Arc<ResolvedRevsetExpression>,
     ) -> BackendResult<Vec<Commit>> {
         let to_visit_revset = RevsetExpression::commits(roots)
             .descendants()
+            .minus(immutable)
             .minus(&RevsetExpression::commits(
                 self.parent_mapping.keys().cloned().collect(),
             ))
@@ -1343,13 +1346,20 @@ impl MutableRepo {
         roots: Vec<CommitId>,
         callback: impl AsyncFnMut(CommitRewriter) -> BackendResult<()>,
     ) -> BackendResult<()> {
-        let options = RewriteRefsOptions::default();
-        self.transform_descendants_with_options(roots, &HashMap::new(), &options, callback)
-            .await
+        self.transform_descendants_with_options(
+            roots,
+            &RevsetExpression::none(),
+            &HashMap::new(),
+            &RewriteRefsOptions::default(),
+            callback,
+        )
+        .await
     }
 
     /// Rewrite descendants of the given roots with options.
     ///
+    /// Commits within the `immutable` set are excluded.
+    ///
     /// If a commit is in the `new_parents_map` is provided, it will be rebased
     /// onto the new parents provided in the map instead of its original
     /// parents.
@@ -1358,19 +1368,18 @@ impl MutableRepo {
     pub async fn transform_descendants_with_options(
         &mut self,
         roots: Vec<CommitId>,
+        immutable: &Arc<ResolvedRevsetExpression>,
         new_parents_map: &HashMap<CommitId, Vec<CommitId>>,
         options: &RewriteRefsOptions,
         callback: impl AsyncFnMut(CommitRewriter) -> BackendResult<()>,
     ) -> BackendResult<()> {
-        let descendants = self.find_descendants_for_rebase(roots).await?;
+        let descendants = self.find_descendants_for_rebase(roots, immutable).await?;
         self.transform_commits(descendants, new_parents_map, options, callback)
             .await
     }
 
     /// Rewrite the given commits in reverse topological order.
     ///
-    /// `commits` should be a connected range.
-    ///
     /// This function is similar to
     /// [`Self::transform_descendants_with_options()`], but only rewrites the
     /// `commits` provided, and does not rewrite their descendants.
@@ -1408,7 +1417,9 @@ impl MutableRepo {
     /// Rebase descendants of the rewritten commits with options and callback.
     ///
     /// The descendants of the commits registered in `self.parent_mappings` will
-    /// be recursively rebased onto the new version of their parents.
+    /// be recursively rebased onto the new version of their parents. Commits
+    /// within the `immutable` set are left unchanged, which also prevents their
+    /// further descendants from being rebased.
     ///
     /// If `options.empty` is the default (`EmptyBehavior::Keep`), all rebased
     /// descendant commits will be preserved even if they were emptied following
@@ -1422,12 +1433,14 @@ impl MutableRepo {
     /// `(old_commit, rebased_commit)` as arguments.
     pub async fn rebase_descendants_with_options(
         &mut self,
+        immutable: &Arc<ResolvedRevsetExpression>,
         options: &RebaseOptions,
         mut progress: impl FnMut(Commit, RebasedCommit),
     ) -> BackendResult<()> {
         let roots = self.parent_mapping.keys().cloned().collect();
         self.transform_descendants_with_options(
             roots,
+            immutable,
             &HashMap::new(),
             &options.rewrite_refs,
             async |rewriter| {
@@ -1454,11 +1467,14 @@ impl MutableRepo {
     /// emptied following the rebase operation. To customize the rebase
     /// behavior, use [`MutableRepo::rebase_descendants_with_options`].
     pub async fn rebase_descendants(&mut self) -> BackendResult<usize> {
-        let options = RebaseOptions::default();
         let mut num_rebased = 0;
-        self.rebase_descendants_with_options(&options, |_old_commit, _rebased_commit| {
-            num_rebased += 1;
-        })
+        self.rebase_descendants_with_options(
+            &RevsetExpression::none(),
+            &RebaseOptions::default(),
+            |_old_commit, _rebased_commit| {
+                num_rebased += 1;
+            },
+        )
         .await?;
         Ok(num_rebased)
     }

lib/src/rewrite.rs

@@ -838,7 +838,9 @@ pub async fn compute_move_commits(
     let mut roots = target_roots.iter().cloned().collect_vec();
     roots.extend(new_children.iter().ids().cloned());
 
-    let descendants = repo.find_descendants_for_rebase(roots.clone()).await?;
+    let descendants = repo
+        .find_descendants_for_rebase(roots.clone(), &RevsetExpression::none())
+        .await?;
     let commit_new_parents_map = descendants
         .iter()
         .map(|commit| -> BackendResult<_> {
@@ -1384,8 +1386,9 @@ pub async fn squash_commits<'repo>(
         // rewritten sources. Otherwise it will likely already have the content
         // changes we're moving, so applying them will have no effect and the
         // changes will disappear.
+        let immutable = RevsetExpression::none();
         let options = RebaseOptions::default();
-        repo.rebase_descendants_with_options(&options, |old_commit, rebased_commit| {
+        repo.rebase_descendants_with_options(&immutable, &options, |old_commit, rebased_commit| {
             if old_commit.id() != destination.id() {
                 return;
             }

lib/tests/test_revset_optimized.rs

@@ -64,8 +64,10 @@ fn write_new_commit<'a>(
 }
 
 fn rebase_descendants(repo: &mut MutableRepo) -> Vec<Commit> {
+    let immutable = RevsetExpression::none();
+    let options = RebaseOptions::default();
     let mut commits = Vec::new();
-    repo.rebase_descendants_with_options(&RebaseOptions::default(), |_, rebased| match rebased {
+    repo.rebase_descendants_with_options(&immutable, &options, |_, rebased| match rebased {
         RebasedCommit::Rewritten(commit) => commits.push(commit),
         RebasedCommit::Abandoned { .. } => {}
     })

lib/tests/test_rewrite.rs

@@ -12,6 +12,8 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+use std::slice;
+
 use assert_matches::assert_matches;
 use itertools::Itertools as _;
 use jj_lib::backend::ChangeId;
@@ -29,11 +31,13 @@ use jj_lib::ref_name::RemoteRefSymbol;
 use jj_lib::ref_name::WorkspaceName;
 use jj_lib::ref_name::WorkspaceNameBuf;
 use jj_lib::repo::Repo as _;
+use jj_lib::revset::RevsetExpression;
 use jj_lib::rewrite::CommitRewriter;
 use jj_lib::rewrite::CommitWithSelection;
 use jj_lib::rewrite::EmptyBehavior;
 use jj_lib::rewrite::MoveCommitsTarget;
 use jj_lib::rewrite::RebaseOptions;
+use jj_lib::rewrite::RebasedCommit;
 use jj_lib::rewrite::RewriteRefsOptions;
 use jj_lib::rewrite::find_duplicate_divergent_commits;
 use jj_lib::rewrite::find_recursive_merge_commits;
@@ -1211,6 +1215,62 @@ fn test_rebase_descendants_hidden() -> TestResult {
     Ok(())
 }
 
+#[test]
+fn test_rebase_descendants_some_excluded() -> TestResult {
+    let test_repo = TestRepo::init();
+    let repo = &test_repo.repo;
+
+    // Commits A and B are immutable. A is rewritten explicitly. B shouldn't be
+    // rebased because it's immutable. C isn't rebased because of that. D should
+    // be rebased.
+    //
+    // C
+    // |
+    // B D
+    // |/
+    // A
+    let mut tx = repo.start_transaction();
+    let commit_a = write_random_commit(tx.repo_mut());
+    let commit_b = write_random_commit_with_parents(tx.repo_mut(), &[&commit_a]);
+    let commit_c = write_random_commit_with_parents(tx.repo_mut(), &[&commit_b]);
+    let commit_d = write_random_commit_with_parents(tx.repo_mut(), &[&commit_a]);
+    let repo = tx.commit("test").block_on()?;
+
+    let mut tx = repo.start_transaction();
+    let commit_a2 = tx
+        .repo_mut()
+        .rewrite_commit(&commit_a)
+        .set_description("a2")
+        .write()
+        .block_on()?;
+    let immutable = RevsetExpression::commit(commit_b.id().clone()).ancestors();
+    let options = RebaseOptions::default();
+    let mut rebased_commits = Vec::new();
+    tx.repo_mut()
+        .rebase_descendants_with_options(
+            &immutable,
+            &options,
+            |old_commit, rebased| match rebased {
+                RebasedCommit::Rewritten(commit) => rebased_commits.push((old_commit, commit)),
+                RebasedCommit::Abandoned { .. } => panic!("no commit should be abandoned"),
+            },
+        )
+        .block_on()?;
+    assert_eq!(rebased_commits.len(), 1);
+    assert_eq!(rebased_commits[0].0, commit_d);
+    let (_, commit_d2) = rebased_commits[0].clone();
+    assert_eq!(commit_d2.parent_ids(), slice::from_ref(commit_a2.id()));
+    assert_eq!(
+        *tx.repo().view().heads(),
+        hashset! {
+            commit_c.id().clone(),
+            commit_d2.id().clone(),
+        }
+    );
+
+    Ok(())
+}
+
 #[test]
 fn test_rebase_descendants_repeated() {
     let test_repo = TestRepo::init();

lib/tests/test_rewrite_transform.rs

@@ -16,6 +16,7 @@ use std::collections::HashMap;
 
 use jj_lib::commit::Commit;
 use jj_lib::repo::Repo as _;
+use jj_lib::revset::RevsetExpression;
 use jj_lib::rewrite::RewriteRefsOptions;
 use maplit::hashmap;
 use maplit::hashset;
@@ -158,6 +159,7 @@ fn test_transform_descendants_new_parents_map() -> TestResult {
     tx.repo_mut()
         .transform_descendants_with_options(
             vec![commit_b.id().clone()],
+            &RevsetExpression::none(),
             &hashmap! {
                 commit_b.id().clone() => vec![commit_c.id().clone()],
                 commit_c.id().clone() => vec![commit_a.id().clone()],

lib/testutils/src/lib.rs

@@ -60,6 +60,7 @@ use jj_lib::repo::StoreFactories;
 use jj_lib::repo_path::RepoPath;
 use jj_lib::repo_path::RepoPathBuf;
 use jj_lib::repo_path::RepoPathComponent;
+use jj_lib::revset::RevsetExpression;
 use jj_lib::rewrite::RebaseOptions;
 use jj_lib::rewrite::RebasedCommit;
 use jj_lib::secret_backend::SecretBackend;
@@ -778,8 +779,9 @@ pub fn rebase_descendants_with_options_return_map(
     repo: &mut MutableRepo,
     options: &RebaseOptions,
 ) -> HashMap<CommitId, CommitId> {
+    let immutable = RevsetExpression::none();
     let mut rebased: HashMap<CommitId, CommitId> = HashMap::new();
-    repo.rebase_descendants_with_options(options, |old_commit, rebased_commit| {
+    repo.rebase_descendants_with_options(&immutable, options, |old_commit, rebased_commit| {
         let old_commit_id = old_commit.id().clone();
         let new_commit_id = match rebased_commit {
             RebasedCommit::Rewritten(new_commit) => new_commit.id().clone(),