From 4f2e72a140758745cfdb7f0c311c297b26aa6875 Mon Sep 17 00:00:00 2001
From: Benjamin Tan <benjamin@dev.ofcr.se>
Date: Wed, 13 Nov 2024 23:29:56 +0800
Subject: [PATCH] cli: simplify-parents: avoid multiple rebases of the same
 commits

The previous iteration of `jj simplify-parents` would only reparent the
commits in the target set in the `MutableRepo::transform_descendants`
callback. The subsequent `MutableRepo::rebase_descendants` call invoked
when the transaction is committed would then rebase all descendants of
the target set, which includes the commits in the target set again.

This commit updates the `MutableRepo::transform_descendants` callback to
perform rebasing of all descendants within the callback. All descendants
of the target set will only be rebased at most once.
---
 cli/src/commands/simplify_parents.rs       | 75 ++++++++--------------
 cli/tests/test_simplify_parents_command.rs |  8 +--
 2 files changed, 30 insertions(+), 53 deletions(-)

diff --git a/cli/src/commands/simplify_parents.rs b/cli/src/commands/simplify_parents.rs
index 78a105db52..8d68cfdf3f 100644
--- a/cli/src/commands/simplify_parents.rs
+++ b/cli/src/commands/simplify_parents.rs
@@ -2,10 +2,7 @@ use std::collections::HashSet;
 
 use clap_complete::ArgValueCandidates;
 use itertools::Itertools;
-use jj_lib::backend::BackendResult;
 use jj_lib::revset::RevsetExpression;
-use jj_lib::rewrite::CommitRewriter;
-use jj_lib::settings::UserSettings;
 
 use crate::cli_util::CommandHelper;
 use crate::cli_util::RevisionArg;
@@ -67,63 +64,47 @@ pub(crate) fn cmd_simplify_parents(
     let num_orig_commits = commit_ids.len();
 
     let mut tx = workspace_command.start_transaction();
-    let mut stats = SimplifyStats::default();
+    let mut simplified_commits = 0;
+    let mut edges = 0;
+    let mut reparented_descendants = 0;
+
     tx.repo_mut()
-        .transform_descendants(command.settings(), commit_ids, |rewriter| {
-            if commit_ids_set.contains(rewriter.old_commit().id()) {
-                simplify_commit_parents(command.settings(), rewriter, &mut stats)?;
+        .transform_descendants(command.settings(), commit_ids, |mut rewriter| {
+            let num_old_heads = rewriter.new_parents().len();
+            if commit_ids_set.contains(rewriter.old_commit().id()) && num_old_heads > 1 {
+                rewriter.simplify_ancestor_merge();
             }
+            let num_new_heads = rewriter.new_parents().len();
+
+            if rewriter.parents_changed() {
+                rewriter.reparent(command.settings())?.write()?;
 
+                if num_new_heads < num_old_heads {
+                    simplified_commits += 1;
+                    edges += num_old_heads - num_new_heads;
+                } else {
+                    reparented_descendants += 1;
+                }
+            }
             Ok(())
         })?;
 
     if let Some(mut formatter) = ui.status_formatter() {
-        if !stats.is_empty() {
+        if simplified_commits > 0 {
             writeln!(
                 formatter,
-                "Removed {} edges from {} out of {} commits.",
-                stats.edges, stats.commits, num_orig_commits
+                "Removed {edges} edges from {simplified_commits} out of {num_orig_commits} \
+                 commits.",
             )?;
+            if reparented_descendants > 0 {
+                writeln!(
+                    formatter,
+                    "Rebased {reparented_descendants} descendant commits",
+                )?;
+            }
         }
     }
     tx.finish(ui, format!("simplify {num_orig_commits} commits"))?;
 
     Ok(())
 }
-
-#[derive(Default)]
-struct SimplifyStats {
-    commits: usize,
-    edges: usize,
-}
-
-impl SimplifyStats {
-    fn is_empty(&self) -> bool {
-        self.commits == 0 && self.edges == 0
-    }
-}
-
-fn simplify_commit_parents(
-    settings: &UserSettings,
-    mut rewriter: CommitRewriter,
-    stats: &mut SimplifyStats,
-) -> BackendResult<()> {
-    if rewriter.old_commit().parent_ids().len() <= 1 {
-        return Ok(());
-    }
-
-    let num_old_heads = rewriter.new_parents().len();
-    rewriter.simplify_ancestor_merge();
-    let num_new_heads = rewriter.new_parents().len();
-
-    if rewriter.parents_changed() {
-        rewriter.reparent(settings)?.write()?;
-
-        if num_new_heads < num_old_heads {
-            stats.commits += 1;
-            stats.edges += num_old_heads - num_new_heads;
-        }
-    }
-
-    Ok(())
-}
diff --git a/cli/tests/test_simplify_parents_command.rs b/cli/tests/test_simplify_parents_command.rs
index 52995ca6df..aa360045d4 100644
--- a/cli/tests/test_simplify_parents_command.rs
+++ b/cli/tests/test_simplify_parents_command.rs
@@ -208,11 +208,9 @@ fn test_simplify_parents_multiple_redundant_parents() {
     let (stdout, stderr) =
         test_env.jj_cmd_ok(&repo_path, &["simplify-parents", "-r", "c", "-r", "f"]);
     insta::assert_snapshot!(stdout, @"");
-    // TODO: The output should indicate that edges were removed from 2 commits
-    // (c, f) and 2 descendant commits were rebased (d, e).
     insta::assert_snapshot!(stderr, @r#"
     Removed 2 edges from 2 out of 2 commits.
-    Rebased 3 descendant commits
+    Rebased 2 descendant commits
     Working copy now at: kmkuslsw 8cc01e1b f | f
     Parent commit      : znkkpsqq 040ae3a6 e | e
     "#);
@@ -232,11 +230,9 @@ fn test_simplify_parents_multiple_redundant_parents() {
     test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]);
     let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["simplify-parents", "-s", "c"]);
     insta::assert_snapshot!(stdout, @"");
-    // TODO: The output should indicate that edges were removed from 2 commits
-    // (c, f) and 2 descendant commits were rebased (d, e).
     insta::assert_snapshot!(stderr, @r#"
     Removed 2 edges from 2 out of 4 commits.
-    Rebased 3 descendant commits
+    Rebased 2 descendant commits
     Working copy now at: kmkuslsw 70a39dff f | f
     Parent commit      : znkkpsqq a021fee9 e | e
     "#);