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 modify commit IDs if commit is unchanged after rebase #3367

Merged
merged 1 commit into from
Mar 29, 2024
Merged

rebase: do not modify commit IDs if commit is unchanged after rebase #3367

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
78 changes: 64 additions & 14 deletions cli/src/commands/rebase.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,13 +290,22 @@ fn rebase_descendants(
rebase_options: RebaseOptions,
) -> Result<(), CommandError> {
workspace_command.check_rewritable(old_commits)?;
let (skipped_commits, old_commits) = old_commits
.iter()
.partition::<Vec<_>, _>(|commit| commit.parents() == new_parents);
if !skipped_commits.is_empty() {
log_skipped_rebase_commits_message(ui, workspace_command, &skipped_commits)?;
}
if old_commits.is_empty() {
return Ok(());
bnjmnt4n marked this conversation as resolved.
Show resolved Hide resolved
}
for old_commit in old_commits.iter() {
check_rebase_destinations(workspace_command.repo(), new_parents, old_commit)?;
}
let mut tx = workspace_command.start_transaction();
// `rebase_descendants` takes care of sorting in reverse topological order, so
// no need to do it here.
for old_commit in old_commits {
for old_commit in old_commits.iter() {
rebase_commit_with_options(
settings,
tx.mut_repo(),
Expand Down Expand Up @@ -344,7 +353,7 @@ fn rebase_revision(
.iter()
.commits(workspace_command.repo().store())
.try_collect()?;
// Currently, immutable commits are defied so that a child of a rewriteable
// Currently, immutable commits are defined so that a child of a rewriteable
// commit is always rewriteable.
debug_assert!(workspace_command.check_rewritable(&child_commits).is_ok());

Expand Down Expand Up @@ -432,21 +441,40 @@ fn rebase_revision(
})
.try_collect()?;

// Finally, it's safe to rebase `old_commit`. At this point, it should no longer
// have any children; they have all been rebased and the originals have been
// abandoned.
rebase_commit(settings, tx.mut_repo(), &old_commit, &new_parents)?;
debug_assert_eq!(tx.mut_repo().rebase_descendants(settings)?, 0);
// Finally, it's safe to rebase `old_commit`. We can skip rebasing if it is
// already a child of `new_parents`. Otherwise, at this point, it should no
// longer have any children; they have all been rebased and the originals
// have been abandoned.
let skipped_commit_rebase = if old_commit.parents() == new_parents {
write!(ui.stderr(), "Skipping rebase of commit ")?;
tx.write_commit_summary(ui.stderr_formatter().as_mut(), &old_commit)?;
writeln!(ui.stderr())?;
true
} else {
rebase_commit(settings, tx.mut_repo(), &old_commit, &new_parents)?;
debug_assert_eq!(tx.mut_repo().rebase_descendants(settings)?, 0);
false
};

if num_rebased_descendants > 0 {
writeln!(
ui.stderr(),
"Also rebased {num_rebased_descendants} descendant commits onto parent of rebased \
commit"
)?;
if skipped_commit_rebase {
writeln!(
ui.stderr(),
"Rebased {num_rebased_descendants} descendant commits onto parent of commit"
)?;
} else {
writeln!(
ui.stderr(),
"Also rebased {num_rebased_descendants} descendant commits onto parent of rebased \
commit"
)?;
}
}
if tx.mut_repo().has_changes() {
tx.finish(ui, format!("rebase commit {}", old_commit.id().hex()))
} else {
Ok(()) // Do not print "Nothing changed."
}
tx.finish(ui, format!("rebase commit {}", old_commit.id().hex()))?;
Ok(())
}

fn check_rebase_destinations(
Expand All @@ -465,3 +493,25 @@ fn check_rebase_destinations(
}
Ok(())
}

fn log_skipped_rebase_commits_message(
ui: &Ui,
workspace_command: &WorkspaceCommandHelper,
commits: &[&Commit],
) -> Result<(), CommandError> {
let mut fmt = ui.stderr_formatter();
let template = workspace_command.commit_summary_template();
if commits.len() == 1 {
write!(fmt, "Skipping rebase of commit ")?;
template.format(commits[0], fmt.as_mut())?;
writeln!(fmt)?;
} else {
writeln!(fmt, "Skipping rebase of commits:")?;
for commit in commits {
write!(fmt, " ")?;
template.format(commit, fmt.as_mut())?;
writeln!(fmt)?;
}
}
Ok(())
}
36 changes: 21 additions & 15 deletions cli/tests/test_duplicate_command.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,49 +293,55 @@ fn test_rebase_duplicates() {

create_commit(&test_env, &repo_path, "a", &[]);
create_commit(&test_env, &repo_path, "b", &["a"]);
create_commit(&test_env, &repo_path, "c", &["b"]);
// Test the setup
insta::assert_snapshot!(get_log_output_with_ts(&test_env, &repo_path), @r###"
@ 1394f625cbbd b @ 2001-02-03 04:05:11.000 +07:00
@ 7e4fbf4f2759 c @ 2001-02-03 04:05:13.000 +07:00
◉ 1394f625cbbd b @ 2001-02-03 04:05:11.000 +07:00
◉ 2443ea76b0b1 a @ 2001-02-03 04:05:09.000 +07:00
◉ 000000000000 @ 1970-01-01 00:00:00.000 +00:00
"###);

let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["duplicate", "b"]);
let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["duplicate", "c"]);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Duplicated 1394f625cbbd as yqosqzyt fdaaf395 b
Duplicated 7e4fbf4f2759 as yostqsxw 0ac2063b c
"###);
let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["duplicate", "b"]);
let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["duplicate", "c"]);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Duplicated 1394f625cbbd as vruxwmqv 870cf438 b
Duplicated 7e4fbf4f2759 as znkkpsqq ce5f4eeb c
"###);
insta::assert_snapshot!(get_log_output_with_ts(&test_env, &repo_path), @r###"
◉ 870cf438ccbb b @ 2001-02-03 04:05:14.000 +07:00
│ ◉ fdaaf3950f07 b @ 2001-02-03 04:05:13.000 +07:00
◉ ce5f4eeb69d1 c @ 2001-02-03 04:05:16.000 +07:00
│ ◉ 0ac2063b1bee c @ 2001-02-03 04:05:15.000 +07:00
├─╯
│ @ 1394f625cbbd b @ 2001-02-03 04:05:11.000 +07:00
│ @ 7e4fbf4f2759 c @ 2001-02-03 04:05:13.000 +07:00
├─╯
◉ 1394f625cbbd b @ 2001-02-03 04:05:11.000 +07:00
◉ 2443ea76b0b1 a @ 2001-02-03 04:05:09.000 +07:00
◉ 000000000000 @ 1970-01-01 00:00:00.000 +00:00
"###);

let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-s", "a", "-d", "a-"]);
let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-s", "b", "-d", "root()"]);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Rebased 4 commits
Working copy now at: zsuskuln 29bd36b6 b | b
Parent commit : rlvkpnrz 2f6dc5a1 a | a
Working copy now at: royxmykx ed671a3c c | c
Parent commit : zsuskuln 4c6f1569 b | b
Added 0 files, modified 0 files, removed 1 files
"###);
// Some of the duplicate commits' timestamps were changed a little to make them
// have distinct commit ids.
insta::assert_snapshot!(get_log_output_with_ts(&test_env, &repo_path), @r###"
◉ b43fe7354758 b @ 2001-02-03 04:05:14.000 +07:00
│ ◉ 08beb14c3ead b @ 2001-02-03 04:05:15.000 +07:00
◉ b86e9f27d085 c @ 2001-02-03 04:05:16.000 +07:00
│ ◉ 8033590fe04d c @ 2001-02-03 04:05:17.000 +07:00
├─╯
│ @ 29bd36b60e60 b @ 2001-02-03 04:05:16.000 +07:00
│ @ ed671a3cbf35 c @ 2001-02-03 04:05:18.000 +07:00
├─╯
◉ 4c6f1569e2a9 b @ 2001-02-03 04:05:18.000 +07:00
│ ◉ 2443ea76b0b1 a @ 2001-02-03 04:05:09.000 +07:00
├─╯
◉ 2f6dc5a1ffc2 a @ 2001-02-03 04:05:16.000 +07:00
◉ 000000000000 @ 1970-01-01 00:00:00.000 +00:00
"###);
}
Expand Down
Loading