rewrite: default to not simplifying ancestor merges

This means auto-rebase will no longer simplify ancestor merges.

CHANGELOG.md

@@ -66,6 +66,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 * On Windows, the pager will now be the built-in instead of disabled.
 
+* Auto-rebase now preserves the shape of history even for merge commits where
+  one parent is an ancestor of another.
+  [#2600](https://github.com/martinvonz/jj/issues/2600)
+
 ## [0.14.0] - 2024-02-07
 
 ### Deprecations

cli/tests/test_abandon_command.rs

@@ -152,11 +152,8 @@ fn test_basics() {
     "###);
 }
 
-// TODO(#2600): Make sure the results here become saner as #2600 is fixed. There
+// This behavior illustrates https://github.com/martinvonz/jj/issues/2600.
-// is an simpler demo of #2600 at https://github.com/martinvonz/jj/pull/2655.
+// See also the corresponding test in `test_rebase_command`
-// However, fixing #2600 will likely change how `abandon` works. This test
-// exists to track how that happens. See also the corresponding test in
-// `test_rebase_command`
 #[test]
 fn test_bug_2600() {
     let test_env = TestEnvironment::default();
@@ -191,22 +188,18 @@ fn test_bug_2600() {
     insta::assert_snapshot!(stderr, @r###"
     Abandoned commit zsuskuln 73c929fc base | base
     Rebased 3 descendant commits onto parents of abandoned commits
-    Working copy now at: znkkpsqq 510f8756 c | c
+    Working copy now at: znkkpsqq 86e31bec c | c
-    Parent commit      : vruxwmqv 7301d9ab b | b
+    Parent commit      : vruxwmqv fd6eb121 b | b
     Added 0 files, modified 0 files, removed 1 files
     "###);
-    // BUG. The user would expect
+    // Commits "a" and "b" should both have "nottherootcommit" as parent, and "b"
-    // @  c
+    // should keep "a" as second parent.
-    // ├─╮
-    // │ ◉  a
-    // ├─╯
-    // ◉  base nottherootcommit
-    // ◉
-    // This is likely caused by DescendantRebaser
     insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
     @  [znk] c
-    ◉  [vru] b
+    ◉    [vru] b
-    ◉  [roy] a
+    ├─╮
+    │ ◉  [roy] a
+    ├─╯
     ◉  [rlv] base nottherootcommit
     ◉  [zzz]
     "###);
@@ -221,7 +214,9 @@ fn test_bug_2600() {
     Parent commit      : vruxwmqv c10cb7b4 b | b
     Added 0 files, modified 0 files, removed 1 files
     "###);
-    // This is likely what the user will expect.
+    // Commit "b" should have "base" as parent. It should not have two parent
+    // pointers to that commit even though it was a merge commit before we abandoned
+    // "a".
     insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
     @  [znk] c
     ◉  [vru] b
@@ -236,22 +231,17 @@ fn test_bug_2600() {
     insta::assert_snapshot!(stderr, @r###"
     Abandoned commit vruxwmqv 8c0dced0 b | b
     Rebased 1 descendant commits onto parents of abandoned commits
-    Working copy now at: znkkpsqq 924bdd1c c | c
+    Working copy now at: znkkpsqq 33a94991 c | c
+    Parent commit      : zsuskuln 73c929fc b?? base | base
     Parent commit      : royxmykx 98f3b9ba a b?? | a
     Added 0 files, modified 0 files, removed 1 files
     "###);
-    // BUG. The user would expect
+    // Commit "c" should inherit the parents from the abndoned commit "b".
-    // @  c
-    // ├─╮
-    // │ ◉  a
-    // ├─╯
-    // ◉  base
-    // ◉  nottherootcommit
-    // ◉
-    // This is likely caused by logic in `cmd_abandon`, not DescendantRebaser
     insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
-    @  [znk] c
+    @    [znk] c
-    ◉  [roy] a b??
+    ├─╮
+    │ ◉  [roy] a b??
+    ├─╯
     ◉  [zsu] b?? base
     ◉  [rlv] nottherootcommit
     ◉  [zzz]
@@ -280,7 +270,8 @@ fn test_bug_2600() {
     Parent commit      : zsuskuln 73c929fc a b base | base
     Added 0 files, modified 0 files, removed 2 files
     "###);
-    // This is likely what the user would expect
+    // Commit "c" should have "base" as parent. As when we abandoned "a", it should
+    // not have two parent pointers to the same commit.
     insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
     @  [znk] c
     ◉  [zsu] a b base
@@ -318,24 +309,12 @@ fn test_bug_2600_rootcommit_special_case() {
     "###);
 
     // Now, the test
-    let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["abandon", "base"]);
+    let stderr = test_env.jj_cmd_internal_error(&repo_path, &["abandon", "base"]);
-    insta::assert_snapshot!(stdout, @"");
     insta::assert_snapshot!(stderr, @r###"
-    Abandoned commit rlvkpnrz 0c61db1b base | base
+    Internal error: Merge failed
-    Rebased 3 descendant commits onto parents of abandoned commits
+    Caused by:
-    Working copy now at: vruxwmqv 73e9185c c | c
+    1: Backend error
-    Parent commit      : royxmykx 80dd9cba b | b
+    2: The Git backend does not support creating merge commits with the root commit as one of the parents.
-    Added 0 files, modified 0 files, removed 1 files
-    "###);
-    // The current behavior is either correct or should be replaced with an error
-    // message. Even though the user would expect `b` to still be a descendant of
-    // `base`, it is impossible in the Git backend.
-    // See also https://github.com/martinvonz/jj/issues/2600#issuecomment-1835418824
-    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
-    @  [vru] c
-    ◉  [roy] b
-    ◉  [zsu] a
-    ◉  [zzz] base
     "###);
 }
 

lib/src/rewrite.rs

@@ -241,7 +241,7 @@ pub enum EmptyBehaviour {
 // change the RebaseOptions construction in the CLI, and changing the
 // rebase_commit function to actually use the flag, and ensure we don't need to
 // plumb it in.
-#[derive(Clone, PartialEq, Eq, Debug)]
+#[derive(Clone, Default, PartialEq, Eq, Debug)]
 pub struct RebaseOptions {
     pub empty: EmptyBehaviour,
     /// If a merge commit would end up with one parent being an ancestor of the
@@ -249,16 +249,6 @@ pub struct RebaseOptions {
     pub simplify_ancestor_merge: bool,
 }
 
-impl Default for RebaseOptions {
-    fn default() -> Self {
-        Self {
-            empty: Default::default(),
-            simplify_ancestor_merge: true,
-        }
-    }
-}
-
-/// Rebases descendants of a commit onto a new commit (or several).
 pub(crate) struct DescendantRebaser<'settings, 'repo> {
     settings: &'settings UserSettings,
     mut_repo: &'repo mut MutableRepo,