rewrite: make simplification of ancestor merges optional

I think the conclusion from #2600 is that at least auto-rebasing
should not simplify merge commits that merge a commit with its
ancestor. Let's start by adding an option for that in the library.

cli/src/commands/rebase.rs

@@ -184,6 +184,7 @@ Please use `jj rebase -d 'all:x|y'` instead of `jj rebase --allow-large-revsets
             true => EmptyBehaviour::AbandonAllEmpty,
             false => EmptyBehaviour::Keep,
         },
+        simplify_ancestor_merge: true,
     };
     let mut workspace_command = command.workspace_helper(ui)?;
     let new_parents = cli_util::resolve_all_revs(&workspace_command, &args.destination)?

lib/src/rewrite.rs

@@ -241,9 +241,21 @@ 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, Default, PartialEq, Eq, Debug)]
+#[derive(Clone, PartialEq, Eq, Debug)]
 pub struct RebaseOptions {
     pub empty: EmptyBehaviour,
+    /// If a merge commit would end up with one parent being an ancestor of the
+    /// other, then filter out the ancestor.
+    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).
@@ -541,7 +553,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
             return Ok(());
         }
         let old_parent_ids = old_commit.parent_ids();
-        let new_parent_ids = self.new_parents(old_parent_ids);
+        let mut new_parent_ids = self.new_parents(old_parent_ids);
         if self.abandoned.contains(&old_commit_id) {
             // Update the `new_parents` map so descendants are rebased correctly.
             self.parent_mapping
@@ -553,16 +565,18 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
             return Ok(());
         }
 
-        // Don't create commit where one parent is an ancestor of another.
-        let head_set: HashSet<_> = self
-            .mut_repo
-            .index()
-            .heads(&mut new_parent_ids.iter())
-            .into_iter()
-            .collect();
+        // If specified, don't create commit where one parent is an ancestor of another.
+        if self.options.simplify_ancestor_merge {
+            let head_set: HashSet<_> = self
+                .mut_repo
+                .index()
+                .heads(&mut new_parent_ids.iter())
+                .into_iter()
+                .collect();
+            new_parent_ids.retain(|new_parent| head_set.contains(new_parent));
+        }
         let new_parents: Vec<_> = new_parent_ids
             .iter()
-            .filter(|new_parent| head_set.contains(new_parent))
             .map(|new_parent_id| self.mut_repo.store().get_commit(new_parent_id))
             .try_collect()?;
         let new_commit = rebase_commit_with_options(

lib/tests/test_rewrite.rs

@@ -506,9 +506,8 @@ fn test_rebase_descendants_abandon_and_replace() {
     );
 }
 
-// TODO(#2600): This behavior may need to change
 #[test]
-fn test_rebase_descendants_abandon_degenerate_merge() {
+fn test_rebase_descendants_abandon_degenerate_merge_simplify() {
     let settings = testutils::user_settings();
     let test_repo = TestRepo::init();
     let repo = &test_repo.repo;
@@ -531,7 +530,13 @@ fn test_rebase_descendants_abandon_degenerate_merge() {
     tx.mut_repo().record_abandoned_commit(commit_b.id().clone());
     let rebase_map = tx
         .mut_repo()
-        .rebase_descendants_return_map(&settings)
+        .rebase_descendants_with_options_return_map(
+            &settings,
+            RebaseOptions {
+                simplify_ancestor_merge: true,
+                ..Default::default()
+            },
+        )
         .unwrap();
     let new_commit_d = assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_d, &[commit_c.id()]);
     assert_eq!(rebase_map.len(), 1);
@@ -542,6 +547,52 @@ fn test_rebase_descendants_abandon_degenerate_merge() {
     );
 }
 
+#[test]
+fn test_rebase_descendants_abandon_degenerate_merge_preserve() {
+    let settings = testutils::user_settings();
+    let test_repo = TestRepo::init();
+    let repo = &test_repo.repo;
+
+    // Commit B was abandoned. Commit D should get rebased to have A and C as
+    // parents.
+    //
+    // D
+    // |\
+    // B C
+    // |/
+    // A
+    let mut tx = repo.start_transaction(&settings);
+    let mut graph_builder = CommitGraphBuilder::new(&settings, tx.mut_repo());
+    let commit_a = graph_builder.initial_commit();
+    let commit_b = graph_builder.commit_with_parents(&[&commit_a]);
+    let commit_c = graph_builder.commit_with_parents(&[&commit_a]);
+    let commit_d = graph_builder.commit_with_parents(&[&commit_b, &commit_c]);
+
+    tx.mut_repo().record_abandoned_commit(commit_b.id().clone());
+    let rebase_map = tx
+        .mut_repo()
+        .rebase_descendants_with_options_return_map(
+            &settings,
+            RebaseOptions {
+                simplify_ancestor_merge: false,
+                ..Default::default()
+            },
+        )
+        .unwrap();
+    let new_commit_d = assert_rebased_onto(
+        tx.mut_repo(),
+        &rebase_map,
+        &commit_d,
+        &[&commit_a.id(), &commit_c.id()],
+    );
+    assert_eq!(rebase_map.len(), 1);
+
+    assert_eq!(
+        *tx.mut_repo().view().heads(),
+        hashset! {new_commit_d.id().clone()}
+    );
+}
+
 #[test]
 fn test_rebase_descendants_abandon_widen_merge() {
     let settings = testutils::user_settings();
@@ -1532,6 +1583,7 @@ fn test_empty_commit_option(empty_behavior: EmptyBehaviour) {
             &settings,
             RebaseOptions {
                 empty: empty_behavior.clone(),
+                simplify_ancestor_merge: true,
             },
         )
         .unwrap();
@@ -1672,6 +1724,7 @@ fn test_rebase_abandoning_empty() {
 
     let rebase_options = RebaseOptions {
         empty: EmptyBehaviour::AbandonAllEmpty,
+        simplify_ancestor_merge: true,
     };
     rebase_commit_with_options(
         &settings,