Skip to content

Commit

Permalink
rebase: allow both --insert-after and --insert-before options to …
Browse files Browse the repository at this point in the history 
…be used simultaneously
  • Loading branch information
@bnjmnt4n
bnjmnt4n committed Apr 29, 2024
1 parent f75461e commit 0e2e09a
Show file tree
Hide file tree
Showing 2 changed files with 216 additions and 2 deletions.
52 changes: 50 additions & 2 deletions cli/src/commands/rebase.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ use crate::ui::Ui;
#[derive(clap::Args, Clone, Debug)]
#[command(verbatim_doc_comment)]
#[command(group(ArgGroup::new("to_rebase").args(&["branch", "source", "revisions"])))]
#[command(group(ArgGroup::new("target").args(&["destination", "insert_after", "insert_before"]).required(true)))]
#[command(group(ArgGroup::new("target").args(&["destination", "insert_after", "insert_before"]).multiple(true).required(true)))]
pub(crate) struct RebaseArgs {
/// Rebase the whole branch relative to destination's ancestors (can be
/// repeated)
Expand Down Expand Up @@ -242,7 +242,20 @@ Please use `jj rebase -d 'all:x|y'` instead of `jj rebase --allow-large-revsets
.parse_union_revsets(&args.revisions)?
.evaluate_to_commits()?
.try_collect()?; // in reverse topological order
if !args.insert_after.is_empty() {
if !args.insert_after.is_empty() && !args.insert_before.is_empty() {
let after_commits =
workspace_command.resolve_some_revsets_default_single(&args.insert_after)?;
let before_commits =
workspace_command.resolve_some_revsets_default_single(&args.insert_before)?;
rebase_revisions_after_before(
ui,
command.settings(),
&mut workspace_command,
&after_commits,
&before_commits,
&target_commits,
)?;
} else if !args.insert_after.is_empty() {
let after_commits =
workspace_command.resolve_some_revsets_default_single(&args.insert_after)?;
rebase_revisions_after(
Expand Down Expand Up @@ -521,6 +534,41 @@ fn rebase_revisions_before(
)
}

fn rebase_revisions_after_before(
ui: &mut Ui,
settings: &UserSettings,
workspace_command: &mut WorkspaceCommandHelper,
after_commits: &IndexSet<Commit>,
before_commits: &IndexSet<Commit>,
target_commits: &[Commit],
) -> Result<(), CommandError> {
workspace_command.check_rewritable(target_commits.iter().ids())?;
let before_commit_ids = before_commits.iter().ids().cloned().collect_vec();
workspace_command.check_rewritable(&before_commit_ids)?;

let after_commit_ids = after_commits.iter().ids().cloned().collect_vec();
let new_children_expression = RevsetExpression::commits(before_commit_ids);
let new_parents_expression = RevsetExpression::commits(after_commit_ids.clone());

ensure_no_commit_loop(
workspace_command.repo().as_ref(),
&new_children_expression,
&new_parents_expression,
)?;

let new_parent_ids = after_commit_ids;
let new_children = before_commits.iter().cloned().collect_vec();

move_commits_transaction(
ui,
settings,
workspace_command,
&new_parent_ids,
&new_children,
target_commits,
)
}

/// Wraps `move_commits` in a transaction.
fn move_commits_transaction(
ui: &mut Ui,
Expand Down
166 changes: 166 additions & 0 deletions cli/tests/test_rebase_command.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2085,6 +2085,172 @@ fn test_rebase_revisions_before() {
"###);
}

#[test]
fn test_rebase_revisions_after_before() {
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 setup_opid = test_env.current_operation_id(&repo_path);

// Rebase a commit after another commit and before that commit's child to
// insert directly between the two commits.
let (stdout, stderr) = test_env.jj_cmd_ok(
&repo_path,
&["rebase", "-r", "d", "--after", "e", "--before", "f"],
);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Rebased 1 commits onto destination
Rebased 1 descendant commits
Working copy now at: lylxulpl fe3d8c30 f | f
Parent commit : znkkpsqq cca70ee1 d | d
Added 1 files, modified 0 files, removed 0 files
"###);
insta::assert_snapshot!(get_long_log_output(&test_env, &repo_path), @r###"
@ f lylxulpl fe3d8c30
◉ d znkkpsqq cca70ee1
◉ e kmkuslsw 48dd9e3f
◉ c vruxwmqv c41e416e
├─╮
│ ◉ b2 royxmykx 903ab0d6
◉ │ b1 zsuskuln 072d5ae1
├─╯
◉ a rlvkpnrz 2443ea76
◉ zzzzzzzz 00000000
"###);
test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]);

// Rebase a commit after another commit and before that commit's descendant to
// create a new merge commit.
let (stdout, stderr) = test_env.jj_cmd_ok(
&repo_path,
&["rebase", "-r", "d", "--after", "a", "--before", "f"],
);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Rebased 1 commits onto destination
Rebased 1 descendant commits
Working copy now at: lylxulpl 22f0323c f | f
Parent commit : kmkuslsw 48dd9e3f e | e
Parent commit : znkkpsqq 61388bb6 d | d
Added 1 files, modified 0 files, removed 0 files
"###);
insta::assert_snapshot!(get_long_log_output(&test_env, &repo_path), @r###"
@ f lylxulpl 22f0323c
├─╮
│ ◉ d znkkpsqq 61388bb6
◉ │ e kmkuslsw 48dd9e3f
◉ │ c vruxwmqv c41e416e
├───╮
│ │ ◉ b2 royxmykx 903ab0d6
│ ├─╯
◉ │ b1 zsuskuln 072d5ae1
├─╯
◉ a rlvkpnrz 2443ea76
◉ zzzzzzzz 00000000
"###);
test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]);

// "c" has parents "b1" and "b2", so when it is rebased, its children "d" and
// "e" should have "b1" and "b2" as parents as well. "c" is then inserted in
// between "d" and "e", making "e" a merge commit with 3 parents "b1", "b2",
// and "c".
let (stdout, stderr) = test_env.jj_cmd_ok(
&repo_path,
&["rebase", "-r", "c", "--after", "d", "--before", "e"],
);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Rebased 1 commits onto destination
Rebased 3 descendant commits
Working copy now at: lylxulpl e37682c5 f | f
Parent commit : kmkuslsw 9bbc9e53 e | e
Added 1 files, modified 0 files, removed 0 files
"###);
insta::assert_snapshot!(get_long_log_output(&test_env, &repo_path), @r###"
@ f lylxulpl e37682c5
◉ e kmkuslsw 9bbc9e53
├─┬─╮
│ │ ◉ c vruxwmqv e11c7c95
│ │ ◉ d znkkpsqq 37869bd5
╭─┬─╯
│ ◉ b2 royxmykx 903ab0d6
◉ │ b1 zsuskuln 072d5ae1
├─╯
◉ a rlvkpnrz 2443ea76
◉ zzzzzzzz 00000000
"###);
test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]);

// Rebase multiple commits and preserve their ancestry. Apart from the heads of
// the target commits ("d" and "e"), "f" also has commits "b1" and "b2" as
// parents since its parents "d" and "e" were in the target set and were
// replaced by their closest ancestors outside the target set.
let (stdout, stderr) = test_env.jj_cmd_ok(
&repo_path,
&[
"rebase", "-r", "c", "-r", "d", "-r", "e", "--after", "a", "--before", "f",
],
);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Rebased 3 commits onto destination
Rebased 1 descendant commits
Working copy now at: lylxulpl 868f6c61 f | f
Parent commit : zsuskuln 072d5ae1 b1 | b1
Parent commit : royxmykx 903ab0d6 b2 | b2
Parent commit : znkkpsqq ae6181e6 d | d
Parent commit : kmkuslsw a55a6779 e | e
Added 1 files, modified 0 files, removed 0 files
"###);
insta::assert_snapshot!(get_long_log_output(&test_env, &repo_path), @r###"
@ f lylxulpl 868f6c61
├─┬─┬─╮
│ │ │ ◉ e kmkuslsw a55a6779
│ │ ◉ │ d znkkpsqq ae6181e6
│ │ ├─╯
│ │ ◉ c vruxwmqv 22540859
│ ◉ │ b2 royxmykx 903ab0d6
│ ├─╯
◉ │ b1 zsuskuln 072d5ae1
├─╯
◉ a rlvkpnrz 2443ea76
◉ zzzzzzzz 00000000
"###);
test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]);

// Should error if a loop will be created.
let stderr = test_env.jj_cmd_failure(
&repo_path,
&["rebase", "-r", "e", "--after", "c", "--before", "a"],
);
insta::assert_snapshot!(stderr, @r###"
Error: Refusing to create a loop: commit c41e416ee4cf would be both an ancestor and a descendant of the rebased commits
"###);
}

#[test]
fn test_rebase_skip_empty() {
let test_env = TestEnvironment::default();
Expand Down

0 comments on commit 0e2e09a

Please sign in to comment.