diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs index 08d13ffe494..e51ad78ed5a 100644 --- a/cli/src/commands/rebase.rs +++ b/cli/src/commands/rebase.rs @@ -382,14 +382,19 @@ fn rebase_revision( let num_rebased_descendants = rebased_commit_ids.len(); // We now update `new_parents` to account for the rebase of all of - // `old_commit`'s descendants. Even if some of the original `new_parents` were - // descendants of `old_commit`, this will no longer be the case after the - // update. + // `old_commit`'s descendants. Even if some of the original `new_parents` + // were descendants of `old_commit`, this will no longer be the case after + // the update. // // To make the update simpler, we assume that each commit was rewritten only // once; we don't have a situation where both `(A,B)` and `(B,C)` are in - // `rebased_commit_ids`. This assumption relies on the fact that a descendant of - // a child of `old_commit` cannot also be a direct child of `old_commit`. + // `rebased_commit_ids`. + // TODO(ilyagr): This assumption relies on the fact that, after + // `rebase_descendants`, a child of `old_commit` cannot also be a direct + // child of `old_commit`. This will likely change, see + // https://github.com/martinvonz/jj/issues/2600. So, the code needs to be + // updated before that happens. This will also affect + // `test_rebase_with_child_and_descendant_bug_2600`. let new_parents: Vec<_> = new_parents .iter() .map(|new_parent| { diff --git a/cli/tests/test_rebase_command.rs b/cli/tests/test_rebase_command.rs index 07b19c8dc3e..3e29cf94f07 100644 --- a/cli/tests/test_rebase_command.rs +++ b/cli/tests/test_rebase_command.rs @@ -631,3 +631,145 @@ fn test_rebase_with_descendants() { fn get_log_output(test_env: &TestEnvironment, repo_path: &Path) -> String { test_env.jj_cmd_success(repo_path, &["log", "-T", "branches"]) } + +// This behavior illustrates https://github.com/martinvonz/jj/issues/2600 +// The behavior of `rebase -r A -d descendant_of_A` can also be affected or +// broken depending on how #2600 is fixed, so we test that as well. +#[test] +fn test_rebase_with_child_and_descendant_bug_2600() { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]); + let repo_path = test_env.env_root().join("repo"); + + create_commit(&test_env, &repo_path, "base", &[]); + create_commit(&test_env, &repo_path, "a", &["base"]); + create_commit(&test_env, &repo_path, "b", &["base", "a"]); + create_commit(&test_env, &repo_path, "c", &["b"]); + let setup_opid = test_env.current_operation_id(&repo_path); + + // Test the setup + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ c + ◉ b + ├─╮ + │ ◉ a + ├─╯ + ◉ base + ◉ + "###); + + let (stdout, stderr) = + test_env.jj_cmd_ok(&repo_path, &["rebase", "-r", "base", "-d", "root()"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Also rebased 4 descendant commits onto parent of rebased commit + Working copy now at: vruxwmqv 1fdab507 c | c + Parent commit : royxmykx 4d413a39 b | b + Added 0 files, modified 0 files, removed 1 files + "###); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + ◉ base + │ @ c + │ ◉ b + │ ◉ a + ├─╯ + ◉ + "###); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-r", "base", "-d", "b"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @""); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + ◉ base + │ @ c + ├─╯ + ◉ b + ◉ a + ◉ + "###); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-r", "base", "-d", "a"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @""); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + ◉ base + │ @ c + │ ◉ b + ├─╯ + ◉ a + ◉ + "###); + + test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]); + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-r", "a", "-d", "root()"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Also rebased 2 descendant commits onto parent of rebased commit + Working copy now at: vruxwmqv 8d3a8426 c | c + Parent commit : royxmykx 4ea418a3 b | b + Added 0 files, modified 0 files, removed 1 files + "###); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + ◉ a + │ @ c + │ ◉ b + │ ◉ base + ├─╯ + ◉ + "###); + + test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]); + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-r", "b", "-d", "root()"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Also rebased 1 descendant commits onto parent of rebased commit + Working copy now at: vruxwmqv 62ca8ca6 c | c + Parent commit : zsuskuln 2c5b7858 a | a + Added 0 files, modified 0 files, removed 1 files + "###); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + ◉ b + │ @ c + │ ◉ a + │ ◉ base + ├─╯ + ◉ + "###); + + test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]); + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-r", "b", "-d", "c"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Also rebased 1 descendant commits onto parent of rebased commit + Working copy now at: vruxwmqv 28f17d8e c | c + Parent commit : zsuskuln 2c5b7858 a | a + Added 0 files, modified 0 files, removed 1 files + "###); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + ◉ b + @ c + ◉ a + ◉ base + ◉ + "###); + + // In this test, the commit with weird ancestry is not rebased (neither directly + // nor indirectly). + test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]); + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-r", "c", "-d", "a"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Working copy now at: vruxwmqv ee203f6d c | c + Parent commit : zsuskuln 2c5b7858 a | a + Added 0 files, modified 0 files, removed 1 files + "###); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ c + │ ◉ b + ╭─┤ + ◉ │ a + ├─╯ + ◉ base + ◉ + "###); +}