From 64d3c308e872d784139a9c39932d8c73fe01a8f5 Mon Sep 17 00:00:00 2001 From: Benjamin Tan Date: Mon, 29 Apr 2024 15:45:33 +0800 Subject: [PATCH] rebase: do not print out commit summaries of skipped commits --- cli/src/commands/rebase.rs | 79 +++++++++++++++----------------- cli/tests/test_rebase_command.rs | 46 ++++++------------- 2 files changed, 50 insertions(+), 75 deletions(-) diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs index 6eb9e61d0c..6765197836 100644 --- a/cli/src/commands/rebase.rs +++ b/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,14 +395,12 @@ fn rebase_descendants_transaction( let (skipped_commits, old_commits) = old_commits .iter() .partition::, _>(|commit| commit.parents() == new_parents); - if !skipped_commits.is_empty() { - if let Some(mut fmt) = ui.status_formatter() { - log_skipped_rebase_commits_message( - fmt.as_mut(), - workspace_command, - skipped_commits.into_iter(), - )?; - } + let num_skipped_rebases = skipped_commits.len(); + if num_skipped_rebases > 0 { + writeln!( + ui.status(), + "Skipped rebase of {num_skipped_rebases} commits that were already in place" + )?; } 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() { - log_skipped_rebase_commits_message( - fmt.as_mut(), - tx.base_workspace_helper(), - skipped_commits.iter(), + if num_skipped_rebases > 0 { + writeln!( + fmt, + "Skipped rebase of {num_skipped_rebases} commits that were already in place" )?; } 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), CommandError> { +) -> Result { 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 = 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, -) -> 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(()) -} diff --git a/cli/tests/test_rebase_command.rs b/cli/tests/test_rebase_command.rs index f463ff3544..bb6835198c 100644 --- a/cli/tests/test_rebase_command.rs +++ b/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: - kmkuslsw cd86b3e4 c | c - lylxulpl 7d0512e5 d | d - nkmrtpmo 858693f7 e | e - xznxytkn e4a00798 f | f + Skipped rebase of 4 commits that were already in place 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: - kmkuslsw cd86b3e4 c | c - lylxulpl 7d0512e5 d | d - nkmrtpmo 858693f7 e | e - xznxytkn e4a00798 f | f + Skipped rebase of 4 commits that were already in place 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: - kmkuslsw cd86b3e4 c | c - lylxulpl 7d0512e5 d | d - nkmrtpmo 858693f7 e | e - xznxytkn e4a00798 f | f + Skipped rebase of 4 commits that were already in place 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: - kmkuslsw cd86b3e4 c | c - lylxulpl 7d0512e5 d | d - nkmrtpmo 858693f7 e | e - xznxytkn e4a00798 f | f + Skipped rebase of 4 commits that were already in place 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: - royxmykx 903ab0d6 b2 | b2 - zsuskuln 072d5ae1 b1 | b1 + Skipped rebase of 2 commits that were already in place "###); 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