Skip to content

Commit

Permalink
cli: rebase -b: add support for --insert-after and `--insert-before…
Browse files Browse the repository at this point in the history 
…` options
  • Loading branch information
@bnjmnt4n
bnjmnt4n committed Nov 12, 2024
1 parent 1c1117f commit 9ea91dc
Show file tree
Hide file tree
Showing 4 changed files with 120 additions and 53 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

* New command `jj util exec` that can be used for arbitrary aliases.

* `jj rebase -b` can now be used with the `--insert-after` and `--insert-before`
options, like `jj rebase -r` and `jj rebase -s`.

* A preview of improved shell completions was added. Please refer to the
[documentation](https://martinvonz.github.io/jj/latest/install-and-setup/#command-line-completion)
to activate them.
Expand Down
58 changes: 25 additions & 33 deletions cli/src/commands/rebase.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ use std::rc::Rc;
use std::sync::Arc;

use clap::ArgGroup;
use indexmap::IndexSet;
use itertools::Itertools;
use jj_lib::backend::CommitId;
use jj_lib::commit::Commit;
Expand Down Expand Up @@ -188,8 +187,6 @@ pub struct RebaseDestinationArgs {
destination: Option<Vec<RevisionArg>>,
/// The revision(s) to insert after (can be repeated to create a merge
/// commit)
///
/// Only works with `-r` and `-s`.
#[arg(
long,
short = 'A',
Expand All @@ -199,8 +196,6 @@ pub struct RebaseDestinationArgs {
insert_after: Option<Vec<RevisionArg>>,
/// The revision(s) to insert before (can be repeated to create a merge
/// commit)
///
/// Only works with `-r` and `-s`.
#[arg(
long,
short = 'B',
Expand Down Expand Up @@ -249,26 +244,12 @@ pub(crate) fn cmd_rebase(
&rebase_options,
)?;
} else {
let Some(destination) = args.destination.destination.as_ref() else {
return Err(cli_error(
"The argument `--destination <DESTINATION>` is required to rebase a whole branch",
));
};
let new_parents = workspace_command
.resolve_some_revsets_default_single(ui, destination)?
.into_iter()
.collect_vec();
let branch_commits = if args.branch.is_empty() {
IndexSet::from([workspace_command.resolve_single_rev(ui, &RevisionArg::AT)?])
} else {
workspace_command.resolve_some_revsets_default_single(ui, &args.branch)?
};
rebase_branch(
ui,
command.settings(),
&mut workspace_command,
new_parents,
&branch_commits,
&args.branch,
&args.destination,
rebase_options,
)?;
}
Expand Down Expand Up @@ -349,16 +330,25 @@ fn rebase_branch(
ui: &mut Ui,
settings: &UserSettings,
workspace_command: &mut WorkspaceCommandHelper,
new_parents: Vec<Commit>,
branch_commits: &IndexSet<Commit>,
branch: &[RevisionArg],
rebase_destination: &RebaseDestinationArgs,
rebase_options: RebaseOptions,
) -> Result<(), CommandError> {
let parent_ids = new_parents.iter().ids().cloned().collect_vec();
let branch_commit_ids = branch_commits
.iter()
.map(|commit| commit.id().clone())
.collect_vec();
let roots_expression = RevsetExpression::commits(parent_ids.clone())
let branch_commits: Vec<_> = if branch.is_empty() {
vec![workspace_command.resolve_single_rev(ui, &RevisionArg::AT)?]
} else {
workspace_command
.resolve_some_revsets_default_single(ui, branch)?
.iter()
.cloned()
.collect_vec()
};

let (new_parents, new_children) =
compute_rebase_destination(ui, workspace_command, rebase_destination)?;
let new_parent_ids = new_parents.iter().ids().cloned().collect_vec();
let branch_commit_ids = branch_commits.iter().ids().cloned().collect_vec();
let roots_expression = RevsetExpression::commits(new_parent_ids.clone())
.range(&RevsetExpression::commits(branch_commit_ids))
.roots();
let root_commits: Vec<_> = roots_expression
Expand All @@ -368,16 +358,18 @@ fn rebase_branch(
.commits(workspace_command.repo().store())
.try_collect()?;
workspace_command.check_rewritable(root_commits.iter().ids())?;
for commit in &root_commits {
check_rebase_destinations(workspace_command.repo(), &new_parents, commit)?;
if rebase_destination.destination.is_some() && new_children.is_empty() {
for commit in &root_commits {
check_rebase_destinations(workspace_command.repo(), &new_parents, commit)?;
}
}

rebase_descendants_transaction(
ui,
settings,
workspace_command,
&parent_ids,
&[],
&new_parent_ids,
&new_children,
root_commits,
&rebase_options,
)
Expand Down
4 changes: 0 additions & 4 deletions cli/tests/cli-reference@.md.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1846,11 +1846,7 @@ commit. This is true in general; it is not specific to this command.
If none of `-b`, `-s`, or `-r` is provided, then the default is `-b @`.
* `-d`, `--destination <DESTINATION>` — The revision(s) to rebase onto (can be repeated to create a merge commit)
* `-A`, `--insert-after <INSERT_AFTER>` — The revision(s) to insert after (can be repeated to create a merge commit)
Only works with `-r` and `-s`.
* `-B`, `--insert-before <INSERT_BEFORE>` — The revision(s) to insert before (can be repeated to create a merge commit)
Only works with `-r` and `-s`.
* `--skip-emptied` — If true, when rebasing would produce an empty commit, the commit is abandoned. It will not be abandoned if it was already empty before the rebase. Will never skip merge commits with multiple non-empty parents
Expand Down
108 changes: 92 additions & 16 deletions cli/tests/test_rebase_command.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,6 @@ fn test_rebase_invalid() {
For more information, try '--help'.
"###);

// --after with implicit -b
let stderr = test_env.jj_cmd_cli_error(&repo_path, &["rebase", "--after", "a"]);
insta::assert_snapshot!(stderr, @"Error: The argument `--destination <DESTINATION>` is required to rebase a whole branch");

// --before with implicit -b
let stderr = test_env.jj_cmd_cli_error(&repo_path, &["rebase", "--before", "a"]);
insta::assert_snapshot!(stderr, @"Error: The argument `--destination <DESTINATION>` is required to rebase a whole branch");

// Both -r and -s
let stderr =
test_env.jj_cmd_cli_error(&repo_path, &["rebase", "-r", "a", "-s", "a", "-d", "b"]);
Expand Down Expand Up @@ -91,10 +83,6 @@ fn test_rebase_invalid() {
For more information, try '--help'.
"###);

// -b with --after
let stderr = test_env.jj_cmd_cli_error(&repo_path, &["rebase", "-b", "a", "--after", "b"]);
insta::assert_snapshot!(stderr, @"Error: The argument `--destination <DESTINATION>` is required to rebase a whole branch");

// Both -d and --before
let stderr = test_env.jj_cmd_cli_error(
&repo_path,
Expand All @@ -108,10 +96,6 @@ fn test_rebase_invalid() {
For more information, try '--help'.
"###);

// -b with --before
let stderr = test_env.jj_cmd_cli_error(&repo_path, &["rebase", "-b", "a", "--before", "b"]);
insta::assert_snapshot!(stderr, @"Error: The argument `--destination <DESTINATION>` is required to rebase a whole branch");

// Rebase onto self with -r
let stderr = test_env.jj_cmd_failure(&repo_path, &["rebase", "-r", "a", "-d", "a"]);
insta::assert_snapshot!(stderr, @r###"
Expand Down Expand Up @@ -1718,6 +1702,34 @@ fn test_rebase_after() {
"###);
test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]);

// `rebase -b` of commit "b3" after "b1" moves its descendants which are not
// already descendants of "b1" (just "b3" and "b4") in between "b1" and its
// child "b2".
let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-b", "b3", "--after", "b1"]);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r#"
Rebased 6 commits onto destination
Rebased 1 descendant commits
Working copy now at: xznxytkn b4078b57 f | f
Parent commit : nkmrtpmo 1b95558f e | e
Added 0 files, modified 0 files, removed 1 files
"#);
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r#"
○ b2: d f
├─╮
│ @ f: e
│ ○ e: c
○ │ d: c
├─╯
○ c: b4
○ b4: b3
○ b3: b1
○ b1: a
○ a
◆
"#);
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,
Expand Down Expand Up @@ -2225,6 +2237,36 @@ fn test_rebase_before() {
"###);
test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]);

// `rebase -b` of commit "b3" before "b2" moves its descendants which are not
// already descendants of its parent "b1" (just "b3" and "b4") in between "b1"
// and its child "b2".
let (stdout, stderr) =
test_env.jj_cmd_ok(&repo_path, &["rebase", "-b", "b3", "--before", "b1"]);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r#"
Skipped rebase of 2 commits that were already in place
Rebased 4 commits onto destination
Rebased 2 descendant commits
Working copy now at: xznxytkn 16422f85 f | f
Parent commit : nkmrtpmo ef9dea83 e | e
Added 0 files, modified 0 files, removed 2 files
"#);
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r#"
○ b2: b1
○ b1: d f
├─╮
│ @ f: e
│ ○ e: c
○ │ d: c
├─╯
○ c: b4
○ b4: b3
○ b3: a
○ a
◆
"#);
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,
Expand Down Expand Up @@ -2446,6 +2488,40 @@ fn test_rebase_after_before() {
"#);
test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]);

// `rebase -b` of a commit "y" to a destination after "a" will rebase all
// commits in "roots(a..y)" and their descendants, corresponding to "x", "y"
// and "z". They will be inserted in a new branch after "a" and before "c".
let (stdout, stderr) = test_env.jj_cmd_ok(
&repo_path,
&["rebase", "-b", "y", "--after", "a", "--before", "c"],
);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r#"
Rebased 3 commits onto destination
Rebased 4 descendant commits
Working copy now at: nmzmmopx 4496f88e f | f
Parent commit : xznxytkn a85404a6 e | e
Added 3 files, modified 0 files, removed 0 files
"#);
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r#"
@ f: e
○ e: c
│ ○ d: c
├─╯
○ c: b1 b2 z
├─┬─╮
│ │ ○ z: y
│ │ ○ y: x
│ │ ○ x: a
│ ○ │ b2: a
│ ├─╯
○ │ b1: a
├─╯
○ a
◆
"#);
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,
Expand Down

0 comments on commit 9ea91dc

Please sign in to comment.