From e7edafc92497480ee71c065619f8b59774b39731 Mon Sep 17 00:00:00 2001 From: Benjamin Tan Date: Mon, 25 Mar 2024 19:46:10 +0800 Subject: [PATCH] rebase: do not modify commit IDs if commit is unchanged after rebase --- cli/src/commands/rebase.rs | 78 ++++++++++--- cli/tests/test_duplicate_command.rs | 36 +++--- cli/tests/test_rebase_command.rs | 173 +++++++++++++++++++++++----- 3 files changed, 230 insertions(+), 57 deletions(-) diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs index 0e9534e1e9..cdf13fbc2e 100644 --- a/cli/src/commands/rebase.rs +++ b/cli/src/commands/rebase.rs @@ -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::, _>(|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(()); + } 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(), @@ -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()); @@ -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( @@ -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(()) +} diff --git a/cli/tests/test_duplicate_command.rs b/cli/tests/test_duplicate_command.rs index c267cc925a..a1a92844b1 100644 --- a/cli/tests/test_duplicate_command.rs +++ b/cli/tests/test_duplicate_command.rs @@ -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 "###); } diff --git a/cli/tests/test_rebase_command.rs b/cli/tests/test_rebase_command.rs index ff0389faa7..4f2d3b3078 100644 --- a/cli/tests/test_rebase_command.rs +++ b/cli/tests/test_rebase_command.rs @@ -145,14 +145,15 @@ 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###" - Rebased 2 commits + Skipping rebase of commit vruxwmqv 514fa6b2 d | d + Rebased 1 commits Working copy now at: znkkpsqq 9ca2a154 e | e Parent commit : zsuskuln 1394f625 b | b Added 1 files, modified 0 files, removed 0 files "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - ◉ d - │ @ e + @ e + │ ◉ d ├─╯ │ ◉ c ├─╯ @@ -174,14 +175,15 @@ 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###" - Rebased 2 commits + Skipping rebase of commit vruxwmqv 514fa6b2 d | d + Rebased 1 commits Working copy now at: znkkpsqq 817e3fb0 e | e Parent commit : zsuskuln 1394f625 b | b Added 1 files, modified 0 files, removed 0 files "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - ◉ d - │ @ e + @ e + │ ◉ d ├─╯ │ ◉ c ├─╯ @@ -691,12 +693,10 @@ fn test_rebase_with_child_and_descendant_bug_2600() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-s", "base", "-d", "root()"]); insta::assert_snapshot!(stdout, @""); + // This should be a no-op insta::assert_snapshot!(stderr, @r###" - Rebased 4 commits - Working copy now at: vruxwmqv 13b23fa1 c | c - Parent commit : royxmykx 15092f8a b | b + Skipping rebase of commit rlvkpnrz 0c61db1b base | base "###); - // This should be a no-op insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ c ◉ b @@ -710,12 +710,10 @@ fn test_rebase_with_child_and_descendant_bug_2600() { test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]); let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-s", "a", "-d", "base"]); insta::assert_snapshot!(stdout, @""); + // This should be a no-op insta::assert_snapshot!(stderr, @r###" - Rebased 3 commits - Working copy now at: vruxwmqv 2d130d92 c | c - Parent commit : royxmykx f9e26e2a b | b + Skipping rebase of commit zsuskuln 2c5b7858 a | a "###); - // This should be a no-op insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ c ◉ b @@ -761,13 +759,11 @@ fn test_rebase_with_child_and_descendant_bug_2600() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-b", "c", "-d", "base"]); insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(stderr, @r###" - Rebased 3 commits - Working copy now at: vruxwmqv fe9741ac c | c - Parent commit : royxmykx 9da147b4 b | b - "###); // 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 zsuskuln 2c5b7858 a | a + "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ c ◉ b @@ -799,12 +795,10 @@ fn test_rebase_with_child_and_descendant_bug_2600() { test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]); let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-b", "a", "-d", "root()"]); insta::assert_snapshot!(stdout, @""); + // This should be a no-op insta::assert_snapshot!(stderr, @r###" - Rebased 4 commits - Working copy now at: vruxwmqv 631fc029 c | c - Parent commit : royxmykx 1cd34b4d b | b + Skipping rebase of commit rlvkpnrz 0c61db1b base | base "###); - // This should be a no-op insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ c ◉ b @@ -832,7 +826,8 @@ fn test_rebase_with_child_and_descendant_bug_2600() { test_env.jj_cmd_ok(&repo_path, &["rebase", "-r", "base", "-d", "root()"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" - Also rebased 4 descendant commits onto parent of rebased commit + Skipping rebase of commit rlvkpnrz 0c61db1b base | base + Rebased 4 descendant commits onto parent of commit Working copy now at: vruxwmqv 57aaa944 c | c Parent commit : royxmykx c8495a71 b | b Added 0 files, modified 0 files, removed 1 files @@ -847,10 +842,10 @@ fn test_rebase_with_child_and_descendant_bug_2600() { // ├─╯ // ◉ insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - ◉ base - │ @ c - │ ◉ b - │ ◉ a + @ c + ◉ b + ◉ a + │ ◉ base ├─╯ ◉ "###); @@ -1102,3 +1097,125 @@ fn test_rebase_skip_empty() { ◉ "###); } + +#[test] +fn test_rebase_skip_if_on_destination() { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]); + let repo_path = test_env.env_root().join("repo"); + + create_commit(&test_env, &repo_path, "a", &[]); + create_commit(&test_env, &repo_path, "b1", &["a"]); + create_commit(&test_env, &repo_path, "b2", &["a"]); + create_commit(&test_env, &repo_path, "c", &["b1", "b2"]); + create_commit(&test_env, &repo_path, "d", &["c"]); + create_commit(&test_env, &repo_path, "e", &["c"]); + create_commit(&test_env, &repo_path, "f", &["e"]); + // Test the setup + insta::assert_snapshot!(get_long_log_output(&test_env, &repo_path), @r###" + @ f lylxulpl 88f778c5 + ◉ e kmkuslsw 48dd9e3f + │ ◉ d znkkpsqq 92438fc9 + ├─╯ + ◉ c vruxwmqv c41e416e + ├─╮ + │ ◉ b2 royxmykx 903ab0d6 + ◉ │ b1 zsuskuln 072d5ae1 + ├─╯ + ◉ a rlvkpnrz 2443ea76 + ◉ zzzzzzzz 00000000 + "###); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-b", "d", "-d", "a"]); + 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 + "###); + insta::assert_snapshot!(get_long_log_output(&test_env, &repo_path), @r###" + @ f lylxulpl 88f778c5 + ◉ e kmkuslsw 48dd9e3f + │ ◉ d znkkpsqq 92438fc9 + ├─╯ + ◉ c vruxwmqv c41e416e + ├─╮ + │ ◉ b2 royxmykx 903ab0d6 + ◉ │ b1 zsuskuln 072d5ae1 + ├─╯ + ◉ a rlvkpnrz 2443ea76 + ◉ zzzzzzzz 00000000 + "###); + + let (stdout, stderr) = + test_env.jj_cmd_ok(&repo_path, &["rebase", "-s", "c", "-d", "b1", "-d", "b2"]); + insta::assert_snapshot!(stdout, @""); + // Skip rebase with -s + insta::assert_snapshot!(stderr, @r###" + Skipping rebase of commit vruxwmqv c41e416e c | c + "###); + insta::assert_snapshot!(get_long_log_output(&test_env, &repo_path), @r###" + @ f lylxulpl 88f778c5 + ◉ e kmkuslsw 48dd9e3f + │ ◉ d znkkpsqq 92438fc9 + ├─╯ + ◉ c vruxwmqv c41e416e + ├─╮ + │ ◉ b2 royxmykx 903ab0d6 + ◉ │ b1 zsuskuln 072d5ae1 + ├─╯ + ◉ a rlvkpnrz 2443ea76 + ◉ zzzzzzzz 00000000 + "###); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-r", "d", "-d", "c"]); + 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 + "###); + insta::assert_snapshot!(get_long_log_output(&test_env, &repo_path), @r###" + @ f lylxulpl 88f778c5 + ◉ e kmkuslsw 48dd9e3f + │ ◉ d znkkpsqq 92438fc9 + ├─╯ + ◉ c vruxwmqv c41e416e + ├─╮ + │ ◉ b2 royxmykx 903ab0d6 + ◉ │ b1 zsuskuln 072d5ae1 + ├─╯ + ◉ a rlvkpnrz 2443ea76 + ◉ zzzzzzzz 00000000 + "###); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-r", "e", "-d", "c"]); + 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 + Rebased 1 descendant commits onto parent of commit + Working copy now at: lylxulpl 77cb229f f | f + Parent commit : vruxwmqv c41e416e c | c + Added 0 files, modified 0 files, removed 1 files + "###); + insta::assert_snapshot!(get_long_log_output(&test_env, &repo_path), @r###" + @ f lylxulpl 77cb229f + │ ◉ e kmkuslsw 48dd9e3f + ├─╯ + │ ◉ d znkkpsqq 92438fc9 + ├─╯ + ◉ c vruxwmqv c41e416e + ├─╮ + │ ◉ b2 royxmykx 903ab0d6 + ◉ │ b1 zsuskuln 072d5ae1 + ├─╯ + ◉ a rlvkpnrz 2443ea76 + ◉ zzzzzzzz 00000000 + "###); +} + +fn get_long_log_output(test_env: &TestEnvironment, repo_path: &Path) -> String { + let template = r#"description.first_line() ++ " " ++ change_id.shortest(8) ++ " " ++ commit_id.shortest(8)"#; + test_env.jj_cmd_success(repo_path, &["log", "-T", template]) +}