Skip to content

Commit

Permalink
cli: make jj rebase not simplify ancestor merges
Browse files Browse the repository at this point in the history 
I think I prefer this behavior because it's less lossy. The user can
manually simplify the history with `jj rebase -s <merge commit> -d
<one of the parents>` afterwards. We can roll this change back later
if we find it annoying.
  • Loading branch information
@martinvonz
martinvonz committed Feb 17, 2024
1 parent 64aa747 commit b442093
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 42 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
the previous behavior is deprecated, but supported for now. it will be removed
in the future.

* `jj rebase` now preserves the shape of history even for merge commits where
one parent is an ancestor of another. You can follow the `jj rebase` by
`jj rebase -s <merge commit> -d <single parent>` if you want to linearize the
history.

### New features

* `jj util completion` now supports powershell and elvish.
Expand Down
2 changes: 1 addition & 1 deletion cli/src/commands/rebase.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ Please use `jj rebase -d 'all:x|y'` instead of `jj rebase --allow-large-revsets
true => EmptyBehaviour::AbandonAllEmpty,
false => EmptyBehaviour::Keep,
},
no_ancestor_merge: true,
no_ancestor_merge: false,
};
let mut workspace_command = command.workspace_helper(ui)?;
let new_parents = cli_util::resolve_all_revs(&workspace_command, &args.destination)?
Expand Down
103 changes: 62 additions & 41 deletions cli/tests/test_rebase_command.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -219,15 +219,18 @@ fn test_rebase_branch_with_merge() {
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Rebased 3 commits
Working copy now at: znkkpsqq 391c91a7 e | e
Working copy now at: znkkpsqq 5f8a3db2 e | e
Parent commit : rlvkpnrz 2443ea76 a | a
Parent commit : vruxwmqv 1677f795 d | d
Added 1 files, modified 0 files, removed 0 files
"###);
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
@ e
◉ d
◉ c
◉ b
@ e
├─╮
│ ◉ d
│ ◉ c
│ ◉ b
├─╯
◉ a
◉
"###);
Expand All @@ -237,15 +240,18 @@ fn test_rebase_branch_with_merge() {
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Rebased 3 commits
Working copy now at: znkkpsqq 040ae3a6 e | e
Working copy now at: znkkpsqq a331ac11 e | e
Parent commit : rlvkpnrz 2443ea76 a | a
Parent commit : vruxwmqv 3d0f3644 d | d
Added 1 files, modified 0 files, removed 0 files
"###);
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
@ e
◉ d
◉ c
◉ b
@ e
├─╮
│ ◉ d
│ ◉ c
│ ◉ b
├─╯
◉ a
◉
"###);
Expand Down Expand Up @@ -536,13 +542,15 @@ fn test_rebase_with_descendants() {
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Rebased 3 commits
Working copy now at: vruxwmqv 309336ff d | d
Parent commit : royxmykx 244fa794 c | c
Working copy now at: vruxwmqv 705832bd d | d
Parent commit : royxmykx 57c7246a c | c
"###);
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
@ d
◉ c
◉ b
◉ c
├─╮
│ ◉ b
├─╯
◉ a
◉
"###);
Expand Down Expand Up @@ -590,8 +598,10 @@ fn test_rebase_with_descendants() {
Added 0 files, modified 0 files, removed 2 files
"###);
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
◉ c
◉ b
◉ c
├─╮
│ ◉ b
├─╯
│ @ d
├─╯
◉ a
Expand All @@ -617,8 +627,10 @@ fn test_rebase_with_descendants() {
Added 0 files, modified 0 files, removed 2 files
"###);
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
◉ c
◉ b
◉ c
├─╮
│ ◉ b
├─╯
│ @ d
├─╯
◉ a
Expand All @@ -631,8 +643,6 @@ fn get_log_output(test_env: &TestEnvironment, repo_path: &Path) -> String {
}

// 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();
Expand Down Expand Up @@ -662,14 +672,16 @@ fn test_rebase_with_child_and_descendant_bug_2600() {
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Rebased 4 commits
Working copy now at: vruxwmqv bda47523 c | c
Parent commit : royxmykx caeef796 b | b
Working copy now at: vruxwmqv 13b23fa1 c | c
Parent commit : royxmykx 15092f8a b | b
"###);
// This should be a no-op, but isn't.
// This should be a no-op
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
@ c
◉ b
◉ a
◉ b
├─╮
│ ◉ a
├─╯
◉ base
◉
"###);
Expand All @@ -679,14 +691,16 @@ fn test_rebase_with_child_and_descendant_bug_2600() {
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Rebased 3 commits
Working copy now at: vruxwmqv 2ce41b33 c | c
Parent commit : royxmykx f16045cb b | b
Working copy now at: vruxwmqv 2d130d92 c | c
Parent commit : royxmykx f9e26e2a b | b
"###);
// This should be a no-op
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
@ c
◉ b
◉ a
◉ b
├─╮
│ ◉ a
├─╯
◉ base
◉
"###);
Expand All @@ -699,7 +713,8 @@ fn test_rebase_with_child_and_descendant_bug_2600() {
Working copy now at: vruxwmqv 2b10f149 c | c
Parent commit : royxmykx 3b233bd8 b | b
"###);
// This works as expected
// Commit "a" should be rebased onto the root commit. Commit "b" should have
// "base" and "a" as parents as before.
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
@ c
◉ b
Expand Down Expand Up @@ -727,14 +742,17 @@ fn test_rebase_with_child_and_descendant_bug_2600() {
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Rebased 3 commits
Working copy now at: vruxwmqv 4c7dc623 c | c
Parent commit : royxmykx 5ea34bfd b | b
Working copy now at: vruxwmqv fe9741ac c | c
Parent commit : royxmykx 9da147b4 b | b
"###);
// This should be a no-op
// The commits in roots(base..c), i.e. commit "a" should be rebased onto "base",
// which is a no-op
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
@ c
◉ b
◉ a
◉ b
├─╮
│ ◉ a
├─╯
◉ base
◉
"###);
Expand All @@ -747,7 +765,8 @@ fn test_rebase_with_child_and_descendant_bug_2600() {
Working copy now at: vruxwmqv 2fc4ef73 c | c
Parent commit : royxmykx 9912ef4b b | b
"###);
// I'm unsure what the user would expect here, probably a no-op
// The commits in roots(a..c), i.e. commit "b" should be rebased onto "a",
// which means "b" loses its "base" parent
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
@ c
◉ b
Expand All @@ -761,14 +780,16 @@ fn test_rebase_with_child_and_descendant_bug_2600() {
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Rebased 4 commits
Working copy now at: vruxwmqv 0a026b90 c | c
Parent commit : royxmykx d1b575a5 b | b
Working copy now at: vruxwmqv 631fc029 c | c
Parent commit : royxmykx 1cd34b4d b | b
"###);
// I'm unsure what the user would expect here, probably a no-op
// This should be a no-op
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
@ c
◉ b
◉ a
◉ b
├─╮
│ ◉ a
├─╯
◉ base
◉
"###);
Expand Down

0 comments on commit b442093

Please sign in to comment.