rebase: do not print out commit summaries of skipped commits

cli/src/commands/rebase.rs

@@ -36,7 +36,6 @@ use crate::cli_util::{
     WorkspaceCommandTransaction,
 };
 use crate::command_error::{user_error, CommandError};
-use crate::formatter::Formatter;
 use crate::ui::Ui;
 
 /// Move revisions to different parent(s)
@@ -396,15 +395,13 @@ fn rebase_descendants_transaction(
     let (skipped_commits, old_commits) = old_commits
         .iter()
         .partition::<Vec<_>, _>(|commit| commit.parents() == new_parents);
-    if !skipped_commits.is_empty() {
+    let num_skipped_rebases = skipped_commits.len();
-        if let Some(mut fmt) = ui.status_formatter() {
+    if num_skipped_rebases > 0 {
-            log_skipped_rebase_commits_message(
+        writeln!(
-                fmt.as_mut(),
+            ui.status(),
-                workspace_command,
+            "Skipped rebase of {num_skipped_rebases} commits that were already in place"
-                skipped_commits.into_iter(),
         )?;
     }
-    }
     if old_commits.is_empty() {
         return Ok(());
     }
@@ -593,7 +590,11 @@ fn move_commits_transaction(
         )
     };
 
-    let (num_rebased_targets, num_rebased_descendants, skipped_commits) = move_commits(
+    let MoveCommitsStats {
+        num_rebased_targets,
+        num_rebased_descendants,
+        num_skipped_rebases,
+    } = move_commits(
         settings,
         tx.mut_repo(),
         new_parent_ids,
@@ -602,11 +603,10 @@ fn move_commits_transaction(
     )?;
 
     if let Some(mut fmt) = ui.status_formatter() {
-        if !skipped_commits.is_empty() {
+        if num_skipped_rebases > 0 {
-            log_skipped_rebase_commits_message(
+            writeln!(
-                fmt.as_mut(),
+                fmt,
-                tx.base_workspace_helper(),
+                "Skipped rebase of {num_skipped_rebases} commits that were already in place"
-                skipped_commits.iter(),
             )?;
         }
         if num_rebased_targets > 0 {
@@ -623,6 +623,16 @@ fn move_commits_transaction(
     tx.finish(ui, tx_description)
 }
 
+struct MoveCommitsStats {
+    /// The number of commits in the target set which were rebased.
+    num_rebased_targets: u32,
+    /// The number of descendant commits which were rebased.
+    num_rebased_descendants: u32,
+    /// The number of commits for which rebase was skipped, due to the commit
+    /// already being in place.
+    num_skipped_rebases: u32,
+}
+
 /// Moves `target_commits` from their current location to a new location in the
 /// graph, given by the set of `new_parent_ids` and `new_children`.
 /// The roots of `target_commits` are rebased onto the new parents, while the
@@ -636,9 +646,13 @@ fn move_commits(
     new_parent_ids: &[CommitId],
     new_children: &[Commit],
     target_commits: &[Commit],
-) -> Result<(usize, usize, Vec<Commit>), CommandError> {
+) -> Result<MoveCommitsStats, CommandError> {
     if target_commits.is_empty() {
-        return Ok((0, 0, vec![]));
+        return Ok(MoveCommitsStats {
+            num_rebased_targets: 0,
+            num_rebased_descendants: 0,
+            num_skipped_rebases: 0,
+        });
     }
 
     let target_commit_ids: HashSet<_> = target_commits.iter().ids().cloned().collect();
@@ -917,9 +931,9 @@ fn move_commits(
         },
     );
 
-    let mut skipped_commits: Vec<Commit> = vec![];
     let mut num_rebased_targets = 0;
     let mut num_rebased_descendants = 0;
+    let mut num_skipped_rebases = 0;
 
     // Rebase each commit onto its new parents in the reverse topological order
     // computed above.
@@ -942,16 +956,16 @@ fn move_commits(
                 num_rebased_descendants += 1;
             }
         } else {
-            skipped_commits.push(old_commit.clone());
+            num_skipped_rebases += 1;
         }
     }
     mut_repo.update_rewritten_references(settings)?;
 
-    Ok((
+    Ok(MoveCommitsStats {
         num_rebased_targets,
         num_rebased_descendants,
-        skipped_commits,
+        num_skipped_rebases,
-    ))
+    })
 }
 
 /// Ensure that there is no possible cycle between the potential children and
@@ -992,24 +1006,3 @@ fn check_rebase_destinations(
     }
     Ok(())
 }
-
-fn log_skipped_rebase_commits_message<'a>(
-    fmt: &mut dyn Formatter,
-    workspace_command: &WorkspaceCommandHelper,
-    commits: impl ExactSizeIterator<Item = &'a Commit>,
-) -> Result<(), CommandError> {
-    let template = workspace_command.commit_summary_template();
-    if commits.len() == 1 {
-        write!(fmt, "Skipping rebase of commit ")?;
-        template.format(commits.into_iter().next().unwrap(), fmt)?;
-        writeln!(fmt)?;
-    } else {
-        writeln!(fmt, "Skipping rebase of commits:")?;
-        for commit in commits {
-            write!(fmt, "  ")?;
-            template.format(commit, fmt)?;
-            writeln!(fmt)?;
-        }
-    }
-    Ok(())
-}

cli/tests/test_rebase_command.rs

@@ -211,7 +211,7 @@ fn test_rebase_branch() {
     let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-b=e", "-b=d", "-d=b"]);
     insta::assert_snapshot!(stdout, @"");
     insta::assert_snapshot!(stderr, @r###"
-    Skipping rebase of commit vruxwmqv 514fa6b2 d | d
+    Skipped rebase of 1 commits that were already in place
     Rebased 1 commits
     Working copy now at: znkkpsqq 9ca2a154 e | e
     Parent commit      : zsuskuln 1394f625 b | b
@@ -241,7 +241,7 @@ fn test_rebase_branch() {
     let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-b=all:e|d", "-d=b"]);
     insta::assert_snapshot!(stdout, @"");
     insta::assert_snapshot!(stderr, @r###"
-    Skipping rebase of commit vruxwmqv 514fa6b2 d | d
+    Skipped rebase of 1 commits that were already in place
     Rebased 1 commits
     Working copy now at: znkkpsqq 817e3fb0 e | e
     Parent commit      : zsuskuln 1394f625 b | b
@@ -984,7 +984,7 @@ fn test_rebase_with_child_and_descendant_bug_2600() {
     insta::assert_snapshot!(stdout, @"");
     // This should be a no-op
     insta::assert_snapshot!(stderr, @r###"
-    Skipping rebase of commit zsuskuln 0a7fb8f6 base | base
+    Skipped rebase of 1 commits that were already in place
     "###);
     insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
     @  c
@@ -1002,7 +1002,7 @@ fn test_rebase_with_child_and_descendant_bug_2600() {
     insta::assert_snapshot!(stdout, @"");
     // This should be a no-op
     insta::assert_snapshot!(stderr, @r###"
-    Skipping rebase of commit royxmykx 86a06598 a | a
+    Skipped rebase of 1 commits that were already in place
     "###);
     insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
     @  c
@@ -1055,7 +1055,7 @@ fn test_rebase_with_child_and_descendant_bug_2600() {
     // The commits in roots(base..c), i.e. commit "a" should be rebased onto "base",
     // which is a no-op
     insta::assert_snapshot!(stderr, @r###"
-    Skipping rebase of commit royxmykx 86a06598 a | a
+    Skipped rebase of 1 commits that were already in place
     "###);
     insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
     @  c
@@ -1092,7 +1092,7 @@ fn test_rebase_with_child_and_descendant_bug_2600() {
     insta::assert_snapshot!(stdout, @"");
     // This should be a no-op
     insta::assert_snapshot!(stderr, @r###"
-    Skipping rebase of commit rlvkpnrz 39f28e63 notroot | notroot
+    Skipped rebase of 1 commits that were already in place
     "###);
     insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
     @  c
@@ -1335,11 +1335,7 @@ fn test_rebase_revisions_after() {
     );
     insta::assert_snapshot!(stdout, @"");
     insta::assert_snapshot!(stderr, @r###"
-    Skipping rebase of commits:
+    Skipped rebase of 4 commits that were already in place
-      kmkuslsw cd86b3e4 c | c
-      lylxulpl 7d0512e5 d | d
-      nkmrtpmo 858693f7 e | e
-      xznxytkn e4a00798 f | f
     Nothing changed.
     "###);
     insta::assert_snapshot!(get_long_log_output(&test_env, &repo_path), @r###"
@@ -1362,11 +1358,7 @@ fn test_rebase_revisions_after() {
     let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-r", "c", "--after", "c"]);
     insta::assert_snapshot!(stdout, @"");
     insta::assert_snapshot!(stderr, @r###"
-    Skipping rebase of commits:
+    Skipped rebase of 4 commits that were already in place
-      kmkuslsw cd86b3e4 c | c
-      lylxulpl 7d0512e5 d | d
-      nkmrtpmo 858693f7 e | e
-      xznxytkn e4a00798 f | f
     Nothing changed.
     "###);
     insta::assert_snapshot!(get_long_log_output(&test_env, &repo_path), @r###"
@@ -1725,11 +1717,7 @@ fn test_rebase_revisions_before() {
     );
     insta::assert_snapshot!(stdout, @"");
     insta::assert_snapshot!(stderr, @r###"
-    Skipping rebase of commits:
+    Skipped rebase of 4 commits that were already in place
-      kmkuslsw cd86b3e4 c | c
-      lylxulpl 7d0512e5 d | d
-      nkmrtpmo 858693f7 e | e
-      xznxytkn e4a00798 f | f
     Nothing changed.
     "###);
     insta::assert_snapshot!(get_long_log_output(&test_env, &repo_path), @r###"
@@ -1752,11 +1740,7 @@ fn test_rebase_revisions_before() {
     let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-r", "c", "--before", "c"]);
     insta::assert_snapshot!(stdout, @"");
     insta::assert_snapshot!(stderr, @r###"
-    Skipping rebase of commits:
+    Skipped rebase of 4 commits that were already in place
-      kmkuslsw cd86b3e4 c | c
-      lylxulpl 7d0512e5 d | d
-      nkmrtpmo 858693f7 e | e
-      xznxytkn e4a00798 f | f
     Nothing changed.
     "###);
     insta::assert_snapshot!(get_long_log_output(&test_env, &repo_path), @r###"
@@ -2326,9 +2310,7 @@ fn test_rebase_skip_if_on_destination() {
     insta::assert_snapshot!(stdout, @"");
     // Skip rebase with -b
     insta::assert_snapshot!(stderr, @r###"
-    Skipping rebase of commits:
+    Skipped rebase of 2 commits that were already in place
-      royxmykx 903ab0d6 b2 | b2
-      zsuskuln 072d5ae1 b1 | b1
     "###);
     insta::assert_snapshot!(get_long_log_output(&test_env, &repo_path), @r###"
     @  f  lylxulpl  88f778c5
@@ -2349,7 +2331,7 @@ fn test_rebase_skip_if_on_destination() {
     insta::assert_snapshot!(stdout, @"");
     // Skip rebase with -s
     insta::assert_snapshot!(stderr, @r###"
-    Skipping rebase of commit vruxwmqv c41e416e c | c
+    Skipped rebase of 1 commits that were already in place
     "###);
     insta::assert_snapshot!(get_long_log_output(&test_env, &repo_path), @r###"
     @  f  lylxulpl  88f778c5
@@ -2369,7 +2351,7 @@ fn test_rebase_skip_if_on_destination() {
     insta::assert_snapshot!(stdout, @"");
     // Skip rebase with -r since commit has no children
     insta::assert_snapshot!(stderr, @r###"
-    Skipping rebase of commit znkkpsqq 92438fc9 d | d
+    Skipped rebase of 1 commits that were already in place
     Nothing changed.
     "###);
     insta::assert_snapshot!(get_long_log_output(&test_env, &repo_path), @r###"
@@ -2390,7 +2372,7 @@ fn test_rebase_skip_if_on_destination() {
     insta::assert_snapshot!(stdout, @"");
     // Skip rebase of commit, but rebases children onto destination with -r
     insta::assert_snapshot!(stderr, @r###"
-    Skipping rebase of commit kmkuslsw 48dd9e3f e | e
+    Skipped rebase of 1 commits that were already in place
     Rebased 1 descendant commits
     Working copy now at: lylxulpl 77cb229f f | f
     Parent commit      : vruxwmqv c41e416e c | c