From d7e06e646252d70c8534c5a3d2ef69302920cab2 Mon Sep 17 00:00:00 2001 From: Benjamin Tan Date: Wed, 6 Nov 2024 07:38:28 +0800 Subject: [PATCH] rebase: allow `-r` with `--skip-emptied` --- cli/src/commands/rebase.rs | 24 ++---------- cli/tests/test_rebase_command.rs | 66 +++++++++++++++++++++++--------- 2 files changed, 51 insertions(+), 39 deletions(-) diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs index 78df064824..7ebfbb1075 100644 --- a/cli/src/commands/rebase.rs +++ b/cli/src/commands/rebase.rs @@ -175,7 +175,7 @@ pub(crate) struct RebaseArgs { /// abandoned. It will not be abandoned if it was already empty before the /// rebase. Will never skip merge commits with multiple non-empty /// parents. - #[arg(long, conflicts_with = "revisions")] + #[arg(long)] skip_emptied: bool, } @@ -231,21 +231,6 @@ pub(crate) fn cmd_rebase( }; let mut workspace_command = command.workspace_helper(ui)?; if !args.revisions.is_empty() { - assert_eq!( - // In principle, `-r --skip-empty` could mean to abandon the `-r` - // commit if it becomes empty. This seems internally consistent with - // the behavior of other commands, but is not very useful. - // - // It would become even more confusing once `-r --before` is - // implemented. If `rebase -r` behaves like `abandon`, the - // descendants of the `-r` commits should not be abandoned if - // emptied. But it would also make sense for the descendants of the - // `--before` commit to be abandoned if emptied. A commit can easily - // be in both categories. - rebase_options.empty, - EmptyBehaviour::Keep, - "clap should forbid `-r --skip-empty`" - ); rebase_revisions( ui, command.settings(), @@ -569,10 +554,6 @@ fn rebase_revisions_transaction( &MoveCommitsTarget::Commits(target_commits), rebase_options, )?; - // TODO(ilyagr): Consider making it possible for descendants of the target set - // to become emptied, like --skip-empty. This would require writing careful - // tests. - assert_eq!(num_abandoned, 0); if let Some(mut fmt) = ui.status_formatter() { if num_skipped_rebases > 0 { @@ -590,6 +571,9 @@ fn rebase_revisions_transaction( if num_rebased_descendants > 0 { writeln!(fmt, "Rebased {num_rebased_descendants} descendant commits")?; } + if num_abandoned > 0 { + writeln!(fmt, "Abandoned {num_abandoned} newly emptied commits")?; + } } tx.finish(ui, tx_description) diff --git a/cli/tests/test_rebase_command.rs b/cli/tests/test_rebase_command.rs index 181453770e..602486bb0d 100644 --- a/cli/tests/test_rebase_command.rs +++ b/cli/tests/test_rebase_command.rs @@ -78,19 +78,6 @@ fn test_rebase_invalid() { For more information, try '--help'. "###); - // Both -r and --skip-empty - let stderr = test_env.jj_cmd_cli_error( - &repo_path, - &["rebase", "-r", "a", "-d", "b", "--skip-empty"], - ); - insta::assert_snapshot!(stderr, @r###" - error: the argument '--revisions ' cannot be used with '--skip-empty' - - Usage: jj rebase --revisions <--destination |--insert-after |--insert-before > - - For more information, try '--help'. - "###); - // Both -d and --after let stderr = test_env.jj_cmd_cli_error( &repo_path, @@ -2456,6 +2443,7 @@ fn test_rebase_skip_emptied() { test_env.jj_cmd_ok(&repo_path, &["restore", "--from=b"]); test_env.jj_cmd_ok(&repo_path, &["new", "-m", "already empty"]); test_env.jj_cmd_ok(&repo_path, &["new", "-m", "also already empty"]); + let setup_opid = test_env.current_operation_id(&repo_path); // Test the setup insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["log", "-T", "description"]), @r###" @@ -2470,12 +2458,12 @@ fn test_rebase_skip_emptied() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-d=b", "--skip-emptied"]); insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(stderr, @r###" + insta::assert_snapshot!(stderr, @r#" Rebased 2 commits Abandoned 1 newly emptied commits - Working copy now at: yostqsxw 6b74c840 (empty) also already empty - Parent commit : vruxwmqv 48a31526 (empty) already empty - "###); + Working copy now at: yostqsxw bc4222f2 (empty) also already empty + Parent commit : vruxwmqv 6b41ecb2 (empty) already empty + "#); // The parent commit became empty and was dropped, but the already empty commits // were kept @@ -2486,6 +2474,47 @@ fn test_rebase_skip_emptied() { ○ a ◆ "###); + + test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]); + // Test the setup + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["log", "-T", "description"]), @r###" + @ also already empty + ○ already empty + ○ will become empty + │ ○ b + ├─╯ + ○ a + ◆ + "###); + + let (stdout, stderr) = test_env.jj_cmd_ok( + &repo_path, + &[ + "rebase", + "-r=description('will become empty')", + "-d=b", + "--skip-emptied", + ], + ); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r#" + Rebased 2 descendant commits + Abandoned 1 newly emptied commits + Working copy now at: yostqsxw 74149b9b (empty) also already empty + Parent commit : vruxwmqv 3bdb2801 (empty) already empty + Added 0 files, modified 0 files, removed 1 files + "#); + + // Rebasing a single commit which becomes empty abandons that commit, whilst its + // already empty descendants were kept + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["log", "-T", "description"]), @r#" + @ also already empty + ○ already empty + │ ○ b + ├─╯ + ○ a + ◆ + "#); } #[test] @@ -2513,10 +2542,9 @@ fn test_rebase_skip_emptied_descendants() { ◆ "#); - // TODO: Use `-r` instead of `-s` once `-r --skip-emptied` is supported. let (stdout, stderr) = test_env.jj_cmd_ok( &repo_path, - &["rebase", "-s", "b", "--before", "c", "--skip-emptied"], + &["rebase", "-r", "b", "--before", "c", "--skip-emptied"], ); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r#"