Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rebase: do not print out commit summaries of skipped commits #3603

Merged
merged 1 commit into from
Apr 30, 2024
Merged

rebase: do not print out commit summaries of skipped commits #3603

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 36 additions & 43 deletions cli/src/commands/rebase.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -396,14 +395,12 @@ fn rebase_descendants_transaction(
let (skipped_commits, old_commits) = old_commits
.iter()
.partition::<Vec<_>, _>(|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(());
Expand Down Expand Up @@ -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,
Expand All @@ -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 {
Expand All @@ -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
Expand All @@ -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();
Expand Down Expand Up @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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(())
}
46 changes: 14 additions & 32 deletions cli/tests/test_rebase_command.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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###"
Expand All @@ -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###"
Expand Down Expand Up @@ -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###"
Expand All @@ -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###"
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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###"
Expand All @@ -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
Expand Down