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

Make jj prev and jj next work when the working copy is a merge commit #3461

Merged
merged 2 commits into from
Apr 8, 2024
Merged

Make jj prev and jj next work when the working copy is a merge commit #3461

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
158e6ef
Make `jj prev` work when the working copy is a merge commit
emesterhazy Apr 2, 2024
1222c78
Make `jj next` work when the working copy is a merge commit
emesterhazy Apr 8, 2024
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* File path arguments now support [file pattern
syntax](docs/filesets.md#file-patterns).

* `jj prev` and `jj next` now work when the working copy revision is a merge.

* Operation objects in templates now have a `snapshot() -> Boolean` method that
evaluates to true if the operation was a snapshot created by a non-mutating
command (e.g. `jj log`).
Expand Down
36 changes: 16 additions & 20 deletions cli/src/commands/next.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,6 @@ pub(crate) fn cmd_next(
args: &NextArgs,
) -> Result<(), CommandError> {
let mut workspace_command = command.workspace_helper(ui)?;
let offset = args.offset;
let current_wc_id = workspace_command
.get_wc_commit_id()
.ok_or_else(|| user_error("This command requires a working copy"))?;
Expand All @@ -115,24 +114,20 @@ pub(crate) fn cmd_next(
.view()
.heads()
.contains(current_wc_id);
let current_wc = workspace_command.repo().store().get_commit(current_wc_id)?;
// If we're editing, start at the working-copy commit.
// Otherwise start from our direct parent.
let start_id = if edit {
current_wc_id
let wc_revset = RevsetExpression::commit(current_wc_id.clone());
// If we're editing, start at the working-copy commit. Otherwise, start from
// its direct parent(s).
let target_revset = if edit {
wc_revset.descendants_at(args.offset)
} else {
match current_wc.parent_ids() {
[parent_id] => parent_id,
_ => return Err(user_error("Cannot run `jj next` on a merge commit")),
}
};
let descendant_expression = RevsetExpression::commit(start_id.clone()).descendants_at(offset);
let target_expression = if edit {
descendant_expression
} else {
descendant_expression.minus(&RevsetExpression::commit(current_wc_id.clone()).descendants())
wc_revset
.parents()
.descendants_at(args.offset)
// In previous versions we subtracted `wc_revset.descendants()`. That's
// unnecessary now that --edit is implied if `@` has descendants.
.minus(&wc_revset)
emesterhazy marked this conversation as resolved.
Show resolved Hide resolved
};
let targets: Vec<Commit> = target_expression
let targets: Vec<Commit> = target_revset
.evaluate_programmatic(workspace_command.repo().as_ref())?
.iter()
.commits(workspace_command.repo().store())
Expand All @@ -142,13 +137,14 @@ pub(crate) fn cmd_next(
[] => {
// We found no descendant.
return Err(user_error(format!(
"No descendant found {offset} commit{} forward",
if offset > 1 { "s" } else { "" }
"No descendant found {} commit{} forward",
args.offset,
if args.offset > 1 { "s" } else { "" }
)));
}
commits => choose_commit(ui, &workspace_command, "next", commits)?,
};
let current_short = short_commit_hash(current_wc.id());
let current_short = short_commit_hash(current_wc_id);
let target_short = short_commit_hash(target.id());
// We're editing, just move to the target commit.
if edit {
Expand Down
33 changes: 11 additions & 22 deletions cli/src/commands/prev.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ use crate::ui::Ui;
/// ```
/// If the working copy revision already has visible children, then `--edit` is
/// implied.
// TODO(#2126): Handle multiple parents, e.g merges.
#[derive(clap::Args, Clone, Debug)]
#[command(verbatim_doc_comment)]
pub(crate) struct PrevArgs {
Expand All @@ -68,7 +67,6 @@ pub(crate) fn cmd_prev(
args: &PrevArgs,
) -> Result<(), CommandError> {
let mut workspace_command = command.workspace_helper(ui)?;
let offset = args.offset;
let current_wc_id = workspace_command
.get_wc_commit_id()
.ok_or_else(|| user_error("This command requires a working copy"))?;
Expand All @@ -78,25 +76,14 @@ pub(crate) fn cmd_prev(
.view()
.heads()
.contains(current_wc_id);
let current_wc = workspace_command.repo().store().get_commit(current_wc_id)?;
let start_id = if edit {
current_wc_id
} else {
match current_wc.parent_ids() {
[parent_id] => parent_id,
_ => return Err(user_error("Cannot run `jj prev` on a merge commit")),
}
};
let ancestor_expression = RevsetExpression::commit(start_id.clone()).ancestors_at(offset);
// If we're editing, start at the working-copy commit. Otherwise, start from
// its direct parent(s).
let target_revset = if edit {
ancestor_expression
RevsetExpression::commit(current_wc_id.clone()).ancestors_at(args.offset)
emesterhazy marked this conversation as resolved.
Show resolved Hide resolved
} else {
// Jujutsu will always create a new commit for prev, even where Mercurial cannot
emesterhazy marked this conversation as resolved.
Show resolved Hide resolved
// and fails. The decision and all discussion around it are available
// here: https://github.com/martinvonz/jj/pull/1200#discussion_r1298623933
//
// If users ever request erroring out, add `.ancestors()` to the revset below.
ancestor_expression.minus(&RevsetExpression::commit(current_wc_id.clone()))
emesterhazy marked this conversation as resolved.
Show resolved Hide resolved
RevsetExpression::commit(current_wc_id.clone())
.parents()
.ancestors_at(args.offset)
};
let targets: Vec<_> = target_revset
.evaluate_programmatic(workspace_command.repo().as_ref())?
Expand All @@ -107,14 +94,16 @@ pub(crate) fn cmd_prev(
[target] => target,
[] => {
return Err(user_error(format!(
"No ancestor found {offset} commit{} back",
if offset > 1 { "s" } else { "" }
"No ancestor found {} commit{} back",
args.offset,
emesterhazy marked this conversation as resolved.
Show resolved Hide resolved
if args.offset > 1 { "s" } else { "" }
)))
}
commits => choose_commit(ui, &workspace_command, "prev", commits)?,
};

// Generate a short commit hash, to make it readable in the op log.
let current_short = short_commit_hash(current_wc.id());
let current_short = short_commit_hash(current_wc_id);
let target_short = short_commit_hash(target.id());
// If we're editing, just move to the revision directly.
if edit {
Expand Down
211 changes: 197 additions & 14 deletions cli/tests/test_next_prev_commands.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,21 +132,128 @@ fn test_next_exceeding_history() {
"###);
}

// The working copy commit is a child of a "fork" with two children on each
// branch.
#[test]
fn test_next_fails_on_merge_commit() {
fn test_next_parent_has_multiple_descendants() {
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");
test_env.jj_cmd_ok(&repo_path, &["branch", "c", "left"]);
test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "first"]);
test_env.jj_cmd_ok(&repo_path, &["new", "@--"]);
test_env.jj_cmd_ok(&repo_path, &["branch", "c", "right"]);
test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "second"]);
test_env.jj_cmd_ok(&repo_path, &["new", "left", "right"]);
// Try to advance the working copy commit.
let stderr = test_env.jj_cmd_failure(&repo_path, &["next"]);
// Setup.
test_env.jj_cmd_ok(&repo_path, &["desc", "-m", "1"]);
test_env.jj_cmd_ok(&repo_path, &["new", "-m", "2"]);
test_env.jj_cmd_ok(&repo_path, &["new", "root()", "-m", "3"]);
test_env.jj_cmd_ok(&repo_path, &["new", "-m", "4"]);
test_env.jj_cmd_ok(&repo_path, &["edit", "description(3)"]);
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
◉ mzvwutvlkqwt 4
@ zsuskulnrvyr 3
│ ◉ kkmpptxzrspx 2
│ ◉ qpvuntsmwlqt 1
├─╯
◉ zzzzzzzzzzzz
"###);

// --edit is implied since the working copy isn't a leaf commit.
let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["next"]);
insta::assert_snapshot!(stdout,@r###""###);
insta::assert_snapshot!(stderr,@r###"
Error: Cannot run `jj next` on a merge commit
Working copy now at: mzvwutvl 1b8531ce (empty) 4
Parent commit : zsuskuln b1394455 (empty) 3
"###);
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
@ mzvwutvlkqwt 4
◉ zsuskulnrvyr 3
│ ◉ kkmpptxzrspx 2
│ ◉ qpvuntsmwlqt 1
├─╯
◉ zzzzzzzzzzzz
"###);
}

#[test]
fn test_next_with_merge_commit_parent() {
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");
// Setup.
test_env.jj_cmd_ok(&repo_path, &["desc", "-m", "1"]);
test_env.jj_cmd_ok(&repo_path, &["new", "root()", "-m", "2"]);
test_env.jj_cmd_ok(
&repo_path,
&["new", "description(1)", "description(2)", "-m", "3"],
);
test_env.jj_cmd_ok(&repo_path, &["new", "-m", "4"]);
test_env.jj_cmd_ok(&repo_path, &["prev", "0"]);
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
@ royxmykxtrkr
│ ◉ mzvwutvlkqwt 4
├─╯
◉ zsuskulnrvyr 3
├─╮
│ ◉ kkmpptxzrspx 2
◉ │ qpvuntsmwlqt 1
├─╯
◉ zzzzzzzzzzzz
"###);

let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["next"]);
insta::assert_snapshot!(stdout,@r###""###);
insta::assert_snapshot!(stderr,@r###"
Working copy now at: vruxwmqv 718bbcd9 (empty) (no description set)
Parent commit : mzvwutvl cb5881ec (empty) 4
"###);
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
@ vruxwmqvtpmx
◉ mzvwutvlkqwt 4
◉ zsuskulnrvyr 3
├─╮
│ ◉ kkmpptxzrspx 2
◉ │ qpvuntsmwlqt 1
├─╯
◉ zzzzzzzzzzzz
"###);
}

#[test]
fn test_next_on_merge_commit() {
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");
// Setup.
test_env.jj_cmd_ok(&repo_path, &["desc", "-m", "1"]);
test_env.jj_cmd_ok(&repo_path, &["new", "root()", "-m", "2"]);
test_env.jj_cmd_ok(
&repo_path,
&["new", "description(1)", "description(2)", "-m", "3"],
);
test_env.jj_cmd_ok(&repo_path, &["new", "-m", "4"]);
test_env.jj_cmd_ok(&repo_path, &["edit", "description(3)"]);
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
◉ mzvwutvlkqwt 4
@ zsuskulnrvyr 3
├─╮
│ ◉ kkmpptxzrspx 2
◉ │ qpvuntsmwlqt 1
├─╯
◉ zzzzzzzzzzzz
"###);

// --edit is implied since the working copy is not a leaf commit.
let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["next"]);
insta::assert_snapshot!(stdout,@r###""###);
insta::assert_snapshot!(stderr,@r###"
Working copy now at: mzvwutvl cb5881ec (empty) 4
Parent commit : zsuskuln 038acb86 (empty) 3
"###);
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
@ mzvwutvlkqwt 4
◉ zsuskulnrvyr 3
├─╮
│ ◉ kkmpptxzrspx 2
◉ │ qpvuntsmwlqt 1
├─╯
◉ zzzzzzzzzzzz
"###);
}

Expand Down Expand Up @@ -228,7 +335,7 @@ fn test_next_choose_branching_child() {
}

#[test]
fn test_prev_fails_on_merge_commit() {
fn test_prev_on_merge_commit() {
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");
Expand All @@ -248,10 +355,86 @@ fn test_prev_fails_on_merge_commit() {
◉ zzzzzzzzzzzz
"###);

// Try to advance the working copy commit.
let stderr = test_env.jj_cmd_failure(&repo_path, &["prev"]);
let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["prev"]);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr,@r###"
Working copy now at: vruxwmqv 41658cf4 (empty) (no description set)
Parent commit : zzzzzzzz 00000000 (empty) (no description set)
"###);

test_env.jj_cmd_ok(&repo_path, &["undo"]);
let (stdout, stderr) = test_env.jj_cmd_stdin_ok(&repo_path, &["prev", "--edit"], "2\n");
insta::assert_snapshot!(stdout, @r###"
ambiguous prev commit, choose one to target:
1: zsuskuln b0d21db3 right | (empty) second
2: qpvuntsm 69542c19 left | (empty) first
q: quit the prompt
enter the index of the commit you want to target:
"###);
insta::assert_snapshot!(stderr,@r###"
Working copy now at: qpvuntsm 69542c19 left | (empty) first
Parent commit : zzzzzzzz 00000000 (empty) (no description set)
"###);
}

#[test]
fn test_prev_on_merge_commit_with_parent_merge() {
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");
test_env.jj_cmd_ok(&repo_path, &["desc", "-m", "x"]);
test_env.jj_cmd_ok(&repo_path, &["new", "root()", "-m", "y"]);
test_env.jj_cmd_ok(
&repo_path,
&["new", "description(x)", "description(y)", "-m", "z"],
);
test_env.jj_cmd_ok(&repo_path, &["new", "root()", "-m", "1"]);
test_env.jj_cmd_ok(
&repo_path,
&["new", "description(z)", "description(1)", "-m", "M"],
);

// Check that the graph looks the way we expect.
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
@ royxmykxtrkr M
├─╮
│ ◉ mzvwutvlkqwt 1
◉ │ zsuskulnrvyr z
├───╮
│ │ ◉ kkmpptxzrspx y
│ ├─╯
◉ │ qpvuntsmwlqt x
├─╯
◉ zzzzzzzzzzzz
"###);

let (stdout, stderr) = test_env.jj_cmd_stdin_ok(&repo_path, &["prev"], "2\n");
insta::assert_snapshot!(stdout, @r###"
ambiguous prev commit, choose one to target:
1: kkmpptxz 146d5c67 (empty) y
2: qpvuntsm c56e5035 (empty) x
3: zzzzzzzz 00000000 (empty) (no description set)
q: quit the prompt
enter the index of the commit you want to target:
"###);
insta::assert_snapshot!(stderr,@r###"
Working copy now at: vruxwmqv e8ff4fa0 (empty) (no description set)
Parent commit : qpvuntsm c56e5035 (empty) x
"###);

test_env.jj_cmd_ok(&repo_path, &["undo"]);
let (stdout, stderr) = test_env.jj_cmd_stdin_ok(&repo_path, &["prev", "--edit"], "2\n");
insta::assert_snapshot!(stdout, @r###"
ambiguous prev commit, choose one to target:
1: mzvwutvl 89b8a355 (empty) 1
2: zsuskuln 1ef71474 (empty) z
q: quit the prompt
enter the index of the commit you want to target:
"###);
insta::assert_snapshot!(stderr,@r###"
Error: Cannot run `jj prev` on a merge commit
Working copy now at: zsuskuln 1ef71474 (empty) z
Parent commit : qpvuntsm c56e5035 (empty) x
Parent commit : kkmpptxz 146d5c67 (empty) y
"###);
}

Expand Down