Skip to content

Commit

Permalink
cli rebase -r: add tests for weird ancestry, fix an assumption in a c…
Browse files Browse the repository at this point in the history 
…omment

In Git, a commit's direct parent is allowed to also be an indirect ancestor
at the same time. `jj` currently tries to prevent this situation, but does
allow it. The correctness of `rebase -r A -d descendant_of_A` currently depends
on this jj-specific behavior; we should change that.

Cc #2600
  • Loading branch information
@ilyagr
ilyagr committed Nov 28, 2023
1 parent 3e96bf5 commit 0afb4e5
Show file tree
Hide file tree
Showing 2 changed files with 205 additions and 5 deletions.
22 changes: 17 additions & 5 deletions cli/src/commands/rebase.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -382,14 +382,26 @@ 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 descendant of `old_commit` cannot also be a
// direct child of `old_commit`. This fact will likely change, see
// https://github.com/martinvonz/jj/issues/2600. So, the code needs to be
// updated before that happens. This would also affect
// `test_rebase_with_child_and_descendant_bug_2600`.
//
// The issue is that if a child and a descendant of `old_commit` were the
// same commit (call it `Q`), it would be rebased first by `rebase_commit`
// above, and then the result would be rebased again by
// `rebase_descendants_return_map`. Then, if we were trying to rebase
// `old_commit` onto `Q`, new_parents would only account for one of these.
let new_parents: Vec<_> = new_parents
.iter()
.map(|new_parent| {
Expand Down
188 changes: 188 additions & 0 deletions cli/tests/test_rebase_command.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -631,3 +631,191 @@ 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
"###);
// The user would expect unsimplified ancestry here.
// ◉ base
// │ @ c
// │ ◉ b
// │ ├─╮
// │ │ ◉ a
// │ ├─╯
// ├─╯
// ◉
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
◉ base
│ @ c
│ ◉ b
│ ◉ a
├─╯
◉
"###);

// This tests the algorithm for rebasing onto descendants. The result should be
// simplified if and only if it's simplified in the above case.
let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-r", "base", "-d", "b"]);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @"");
// Unsimlified ancestry would look like
// @ c
// │ ◉ base
// ├─╯
// ◉ b
// ├─╮
// │ ◉ a
// ├─╯
// ◉
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
◉ base
│ @ c
├─╯
◉ b
◉ a
◉
"###);

// This tests the algorithm for rebasing onto descendants. The result should be
// simplified if and only if it's simplified in the above case.
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]);
// ====== Reminder of 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", "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 fe3877ec c | c
Parent commit : royxmykx 0ac7ac49 b | b
Added 0 files, modified 0 files, removed 1 files
"###);
// In this case, it is unclear whether the user would always prefer unsimplified
// ancestry (whether `b` should also be a direct child of the root commit).
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 d2e22304 c | c
Parent commit : zsuskuln 2c5b7858 a | a
Added 0 files, modified 0 files, removed 1 files
"###);
// The user would expect unsimplified ancestry here.
// ◉ b
// │ @ c
// │ ├─╮
// │ │ ◉ a
// │ ├─╯
// │ ◉ base
// ├─╯
// ◉
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
◉ b
│ @ c
│ ◉ a
│ ◉ base
├─╯
◉
"###);

// This tests the algorithm for rebasing onto descendants. The result should be
// simplified if and only if it's simplified in the above case.
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 9138faf9 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 ea347cde 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
◉
"###);
}

0 comments on commit 0afb4e5

Please sign in to comment.