jj rebase -r A -d descendant_of_A can panic if the graph has weird ancestry.

[ilyagr]

See #2644 for the panicking test (jj_cli_panic). See also #2644 (comment).

This is loosely related to #2600. It happens with or without #2646 .

[ilyagr]

Some debug info is below. As far as rebase is concerned, there are no cycles. Perhaps one of the children of the rebased commit wasn't counted because of the graph simplification.

https://github.com/martinvonz/jj/blob/6ce7bd533881bc6c1a0929e5d978e9fc1f0ea718/cli/src/commands/rebase.rs#L356-L363

Debug info:

diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs
index fd172e1a3..5862c265b 100644
--- a/cli/src/commands/rebase.rs
+++ b/cli/src/commands/rebase.rs
@@ -409,7 +409,7 @@ fn rebase_revision(
     let new_parents: Vec<_> = new_parents
         .iter()
         .map(|new_parent| {
-            rebased_commit_ids
+            dbg!(&rebased_commit_ids)
                 .get(new_parent.id())
                 .map_or(Ok(new_parent.clone()), |rebased_new_parent_id| {
                     tx.repo().store().get_commit(rebased_new_parent_id)
@@ -420,7 +420,12 @@ fn rebase_revision(
     // Finally, it's safe to rebase `old_commit`. At this point, it should no longer
     // have any children; they have all been rebased and the originals have been
     // abandoned.
-    rebase_commit(settings, tx.mut_repo(), &old_commit, &new_parents)?;
+    rebase_commit(
+        settings,
+        tx.mut_repo(),
+        dbg!(&old_commit),
+        dbg!(&new_parents),
+    )?;
     debug_assert_eq!(tx.mut_repo().rebase_descendants(settings)?, 0);
 
     if num_rebased_descendants > 0 {
diff --git a/cli/tests/test_rebase_command.rs b/cli/tests/test_rebase_command.rs
index fda43ead9..32ff48032 100644
--- a/cli/tests/test_rebase_command.rs
+++ b/cli/tests/test_rebase_command.rs
@@ -821,6 +855,40 @@ fn test_rebase_with_child_and_descendant_bug_2600() {
     test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]);
     let stderr = test_env.jj_cmd_panic(&repo_path, &["rebase", "-r", "base", "-d", "b"]);
     insta::assert_snapshot!(stderr, @r###"
+    [cli/src/commands/rebase.rs:412] &rebased_commit_ids = {
+        CommitId(
+            "9039f68d4c4f85c9db529db4a810eaf78960d3e1",
+        ): CommitId(
+            "a8fb66ca18baac5a3edc8af31f75eb7930b93ac5",
+        ),
+        CommitId(
+            "5140ced3454deb9cc91a4d7554ef4f72e8522265",
+        ): CommitId(
+            "a72f0141a0ce3f9e6ec83c2d8b61acdf43a173d5",
+        ),
+        CommitId(
+            "2c5b785856e8bc78a64528fdbeb9f0b31b07e26f",
+        ): CommitId(
+            "d34be1afc4177370f11be8b5be3a07ca536f42e0",
+        ),
+        CommitId(
+            "a8fb66ca18baac5a3edc8af31f75eb7930b93ac5",
+        ): CommitId(
+            "7033e7756b576cf7717829f496c48d6327321837",
+        ),
+    }
+    [cli/src/commands/rebase.rs:426] &old_commit = Commit {
+        id: CommitId(
+            "0c61db1be8c827d8b4f0c6b99fb1d20d179f833e",
+        ),
+    }
+    [cli/src/commands/rebase.rs:427] &new_parents = [
+        Commit {
+            id: CommitId(
+                "a8fb66ca18baac5a3edc8af31f75eb7930b93ac5",
+            ),
+        },
+    ]
     thread 'main' panicked at lib/src/dag_walk.rs:113:13:
     graph has cycle
     stack backtrace:
@@ -845,7 +913,7 @@ fn test_rebase_with_child_and_descendant_bug_2600() {
        9: jj_lib::repo::MutableRepo::rebase_descendants
                  at /usr/local/google/home/ilyagr/dev/jj/lib/src/repo.rs:878:9
       10: jj_cli::commands::rebase::rebase_revision
-                 at /usr/local/google/home/ilyagr/dev/jj/cli/src/commands/rebase.rs:424:22
+                 at /usr/local/google/home/ilyagr/dev/jj/cli/src/commands/rebase.rs:429:22
       11: jj_cli::commands::rebase::cmd_rebase
                  at /usr/local/google/home/ilyagr/dev/jj/cli/src/commands/rebase.rs:197:9
       12: jj_cli::commands::run_command

[ilyagr]

Indeed, it seems like removing the code and using #2646 fixes this bug (at the moment, you can take a look at the last commit of main...ilyagr:jj:rebasersimpler). However, there are still issues with the root commit, see #2600 (comment). If we keep simplifying the graph for the root commit, I'm not sure if some shade of this bug will remain.

[ilyagr]

I'm not actively working on this ATM, it's likely to be affected as we refactor the DescendantRebaser. This could also become a part of that refactor.

[bnjmnt4n]

I think this has been fixed by migrating to the new transform_descendants API in #3551. In particular, the panicking test case no longer panics.

[ilyagr]

Thank you so much for working on this! I haven't been able to look at the details yet, but it sounds very nice.