Skip to content

Commit

Permalink
Tweak the behavior of jj prev when @ is a non-discardable tip
Browse files Browse the repository at this point in the history
Before this change, `jj prev` does this:
```
jj log -T 'separate(" ",change_id.short(), description)'
@  umspkqwnpqvx add file2
◉  nprzlkrnkpuu add file1
◉  zzzzzzzzzzzz
jj prev
Working copy now at: otzuvlqz 21a57e68 (empty) (no description set)
Parent commit      : zzzzzzzz 00000000 (empty) (no description set)
Added 0 files, modified 0 files, removed 2 files
jj log -T 'separate(" ",change_id.short(),description)'
@  otzuvlqzsrxt
│ ◉  umspkqwnpqvx add file2
│ ◉  nprzlkrnkpuu add file1
├─╯
◉  zzzzzzzzzzzz
```

After, it does this:
```
jj log -T 'separate(" ",change_id.short(), description)'
@  okxltnqrlpsr
│ ◉  umspkqwnpqvx add file2
├─╯
◉  nprzlkrnkpuu add file1
◉  zzzzzzzzzzzz
```

There is more discussion about this change in
#3426.

This commit also allows `jj prev` to run when `@` is a merge commit. Previously
it refused to do this.
  • Loading branch information
emesterhazy committed Apr 4, 2024
1 parent 005f803 commit 4b64c39
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 31 deletions.
33 changes: 11 additions & 22 deletions cli/src/commands/prev.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,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 +77,17 @@ pub(crate) fn cmd_prev(
.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
let offset = if current_wc.is_discardable() && !args.edit {
// If the current working copy is discardable and the user didn't
// specify --edit, then we move one commit further back. This is so that
// the new empty working commit is created on the parent of the current
// on (or further back depending on the value of args.amount). If we
// didn't do this, then we wouldn't move far enough.
args.offset + 1
} 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);
let target_revset = if edit {
ancestor_expression
} else {
// Jujutsu will always create a new commit for prev, even where Mercurial cannot
// 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()))
args.offset
};
let target_revset = RevsetExpression::commit(current_wc_id.clone()).ancestors_at(offset);
let targets: Vec<_> = target_revset
.evaluate_programmatic(workspace_command.repo().as_ref())?
.iter()
Expand All @@ -105,13 +96,11 @@ pub(crate) fn cmd_prev(
let target = match targets.as_slice() {
[target] => target,
[] => {
return Err(user_error(format!(
"No ancestor found {offset} commit{} back",
if offset > 1 { "s" } else { "" }
)))
return Err(user_error("No ancestor found at requested offset."));
}
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 target_short = short_commit_hash(target.id());
Expand Down
47 changes: 38 additions & 9 deletions cli/tests/test_next_prev_commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,14 +112,25 @@ fn test_prev_non_discardable_working_copy() {
let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["prev"]);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Working copy now at: royxmykx f04d8595 (empty) (no description set)
Parent commit : qpvuntsm 69542c19 (empty) first
Working copy now at: royxmykx 5647d685 (empty) (no description set)
Parent commit : rlvkpnrz 5c52832c (empty) second
"###);

// Since the user didn't specify --edit, @ is now sitting at a new empty
// child of second.
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
@ royxmykxtrkr
│ ◉ kkmpptxzrspx third
├─╯
◉ rlvkpnrzqnoo second
◉ qpvuntsmwlqt first
◉ zzzzzzzzzzzz
"###);
}

#[test]
fn test_prev_multiple_without_root() {
// Move from fourth => first.
// Move from fourth => second.
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 @@ -139,8 +150,18 @@ fn test_prev_multiple_without_root() {
let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["prev", "2"]);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Working copy now at: vruxwmqv bcea672c (empty) (no description set)
Parent commit : qpvuntsm 69542c19 (empty) first
Working copy now at: vruxwmqv 6c3e8d2a (empty) (no description set)
Parent commit : rlvkpnrz 5c52832c (empty) second
"###);

insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
@ vruxwmqvtpmx
│ ◉ zsuskulnrvyr fourth
│ ◉ kkmpptxzrspx third
├─╯
◉ rlvkpnrzqnoo second
◉ qpvuntsmwlqt first
◉ zzzzzzzzzzzz
"###);
}

Expand Down Expand Up @@ -257,7 +278,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 @@ -279,9 +300,17 @@ fn test_prev_fails_on_merge_commit() {
"###);

// Try to advance the working copy commit.
let stderr = test_env.jj_cmd_failure(&repo_path, &["prev"]);
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: zsuskuln edad76e9 right | (empty) second
2: qpvuntsm 5ae1a6a5 left | (empty) first
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: yostqsxw a9de0711 (empty) (no description set)
Parent commit : qpvuntsm 5ae1a6a5 left | (empty) first
"###);
}

Expand Down Expand Up @@ -348,7 +377,7 @@ fn test_prev_onto_root_fails() {
// The root commit is before "first", which is 5 commits back.
let stderr = test_env.jj_cmd_failure(&repo_path, &["prev", "5"]);
insta::assert_snapshot!(stderr,@r###"
Error: No ancestor found 5 commits back
Error: No ancestor found at requested offset.
"###);
}

Expand Down

0 comments on commit 4b64c39

Please sign in to comment.