Skip to content

Commit

Permalink
cli: make "op abandon" not fail with multiple op heads
Browse files Browse the repository at this point in the history 
Since "op abandon" just rewrites DAG, it works no matter if the heads are
merged or not. This change will help crash recovery. "op abandon
--at-op=<one-of-the-heads>" can't be used because ancestor operations would be
preserved by the other head.
  • Loading branch information
@yuja
yuja committed Aug 7, 2024
1 parent 399110b commit 7bdb28f
Show file tree
Hide file tree
Showing 3 changed files with 132 additions and 36 deletions.
68 changes: 41 additions & 27 deletions cli/src/commands/operation/abandon.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,14 @@
// limitations under the License.

use std::io::Write as _;
use std::slice;
use std::{iter, slice};

use itertools::Itertools as _;
use jj_lib::op_store::OperationId;
use jj_lib::op_walk;
use jj_lib::operation::Operation;

use crate::cli_util::{short_operation_hash, CommandHelper};
use crate::command_error::{cli_error, user_error, user_error_with_hint, CommandError};
use crate::command_error::{cli_error, user_error, CommandError};
use crate::ui::Ui;

/// Abandon operation history
Expand Down Expand Up @@ -51,15 +50,15 @@ pub fn cmd_op_abandon(
let mut workspace = command.load_workspace()?;
let repo_loader = workspace.repo_loader();
let op_store = repo_loader.op_store();
let op_heads_store = repo_loader.op_heads_store();
// It doesn't make sense to create concurrent operations that will be merged
// with the current head.
if command.global_args().at_operation.is_some() {
return Err(cli_error("--at-op is not respected"));
}
let current_head_op = op_walk::resolve_op_for_load(repo_loader, "@")?;
let resolve_op =
|op_str| op_walk::resolve_op_at(op_store, slice::from_ref(&current_head_op), op_str);
let (abandon_root_op, abandon_head_op) =
let current_head_ops = op_walk::get_current_head_ops(op_store, op_heads_store.as_ref())?;
let resolve_op = |op_str| op_walk::resolve_op_at(op_store, &current_head_ops, op_str);
let (abandon_root_op, abandon_head_ops) =
if let Some((root_op_str, head_op_str)) = args.operation.split_once("..") {
let root_op = if root_op_str.is_empty() {
let id = op_store.root_operation_id();
Expand All @@ -68,12 +67,12 @@ pub fn cmd_op_abandon(
} else {
resolve_op(root_op_str)?
};
let head_op = if head_op_str.is_empty() {
current_head_op.clone()
let head_ops = if head_op_str.is_empty() {
current_head_ops.clone()
} else {
resolve_op(head_op_str)?
vec![resolve_op(head_op_str)?]
};
(root_op, head_op)
(root_op, head_ops)
} else {
let op = resolve_op(&args.operation)?;
let parent_ops: Vec<_> = op.parents().try_collect()?;
Expand All @@ -82,25 +81,37 @@ pub fn cmd_op_abandon(
1 => parent_ops.into_iter().next().unwrap(),
_ => return Err(user_error("Cannot abandon a merge operation")),
};
(parent_op, op)
(parent_op, vec![op])
};

if abandon_head_op == current_head_op {
return Err(user_error_with_hint(
"Cannot abandon the current operation",
"Run `jj undo` to revert the current operation, then use `jj op abandon`",
if let Some(op) = abandon_head_ops
.iter()
.find(|op| current_head_ops.contains(op))
{
let mut err = user_error(format!(
"Cannot abandon the current operation {}",
short_operation_hash(op.id())
));
if current_head_ops.len() == 1 {
err.add_hint("Run `jj undo` to revert the current operation, then use `jj op abandon`");
}
return Err(err);
}

// Reparent descendants, count the number of abandoned operations.
let stats = op_walk::reparent_range(
op_store.as_ref(),
slice::from_ref(&abandon_head_op),
slice::from_ref(&current_head_op),
&abandon_head_ops,
&current_head_ops,
&abandon_root_op,
)?;
let [new_head_id]: [OperationId; 1] = stats.new_head_ids.try_into().unwrap();
if current_head_op.id() == &new_head_id {
assert_eq!(
current_head_ops.len(),
stats.new_head_ids.len(),
"all current_head_ops should be reparented as they aren't included in abandon_head_ops"
);
let reparented_head_ops = || iter::zip(&current_head_ops, &stats.new_head_ids);
if reparented_head_ops().all(|(old, new_id)| old.id() == new_id) {
writeln!(ui.status(), "Nothing changed.")?;
return Ok(());
}
Expand All @@ -110,23 +121,26 @@ pub fn cmd_op_abandon(
stats.unreachable_count,
stats.rewritten_count,
)?;
repo_loader
.op_heads_store()
.update_op_heads(slice::from_ref(current_head_op.id()), &new_head_id);
for (old, new_id) in reparented_head_ops().filter(|&(old, new_id)| old.id() != new_id) {
op_heads_store.update_op_heads(slice::from_ref(old.id()), new_id);
}
// Remap the operation id of the current workspace. If there were any
// concurrent operations, user will need to re-abandon their ancestors.
if !command.global_args().ignore_working_copy {
let mut locked_ws = workspace.start_working_copy_mutation()?;
let old_op_id = locked_ws.locked_wc().old_operation_id();
if old_op_id != current_head_op.id() {
if let Some((_, new_id)) = reparented_head_ops().find(|(old, _)| old.id() == old_op_id) {
locked_ws.finish(new_id.clone())?
} else {
writeln!(
ui.warning_default(),
"The working copy operation {} is not updated because it differs from the repo {}.",
short_operation_hash(old_op_id),
short_operation_hash(current_head_op.id()),
current_head_ops
.iter()
.map(|op| short_operation_hash(op.id()))
.join(", "),
)?;
} else {
locked_ws.finish(new_head_id)?
}
}
Ok(())
Expand Down
98 changes: 90 additions & 8 deletions cli/tests/test_operations.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ fn test_op_abandon_ancestors() {
// Can't abandon the current operation.
let stderr = test_env.jj_cmd_failure(&repo_path, &["op", "abandon", "..@"]);
insta::assert_snapshot!(stderr, @r###"
Error: Cannot abandon the current operation
Error: Cannot abandon the current operation d92d0753399f
Hint: Run `jj undo` to revert the current operation, then use `jj op abandon`
"###);

Expand Down Expand Up @@ -508,6 +508,82 @@ fn test_op_abandon_without_updating_working_copy() {
"###);
}

#[test]
fn test_op_abandon_multiple_heads() {
let test_env = TestEnvironment::default();
test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]);
let repo_path = test_env.env_root().join("repo");

// Create 1 base operation + 2 operations to be diverged.
test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "commit 1"]);
test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "commit 2"]);
test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "commit 3"]);
let stdout = test_env.jj_cmd_success(
&repo_path,
&["op", "log", "--no-graph", r#"-Tid.short() ++ "\n""#],
);
let (head_op_id, prev_op_id) = stdout.lines().next_tuple().unwrap();
insta::assert_snapshot!(head_op_id, @"cd2b4690faf2");
insta::assert_snapshot!(prev_op_id, @"c2878c428b1c");

// Create 1 other concurrent operation.
test_env.jj_cmd_ok(&repo_path, &["commit", "--at-op=@--", "-m", "commit 4"]);

// Can't resolve operation relative to @.
let stderr = test_env.jj_cmd_failure(&repo_path, &["op", "abandon", "@-"]);
insta::assert_snapshot!(stderr, @r###"
Error: The "@" expression resolved to more than one operation
Hint: Try specifying one of the operations by ID: cd2b4690faf2, 603773cdd351
"###);
let (_, other_head_op_id) = stderr.trim_end().rsplit_once(", ").unwrap();
insta::assert_snapshot!(other_head_op_id, @"603773cdd351");
assert_ne!(head_op_id, other_head_op_id);

// Can't abandon one of the head operations.
let stderr = test_env.jj_cmd_failure(&repo_path, &["op", "abandon", head_op_id]);
insta::assert_snapshot!(stderr, @r###"
Error: Cannot abandon the current operation cd2b4690faf2
"###);

// Can't abandon the other head operation.
let stderr = test_env.jj_cmd_failure(&repo_path, &["op", "abandon", other_head_op_id]);
insta::assert_snapshot!(stderr, @r###"
Error: Cannot abandon the current operation 603773cdd351
"###);

// Can abandon the operation which is not an ancestor of the other head.
// This would crash if we attempted to remap the unchanged op in the op
// heads store.
let (_stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["op", "abandon", prev_op_id]);
insta::assert_snapshot!(stderr, @r###"
Abandoned 1 operations and reparented 2 descendant operations.
"###);

let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["op", "log"]);
insta::assert_snapshot!(stdout, @r###"
@ a232d055d331 [email protected] 2001-02-03 04:05:17.000 +07:00 - 2001-02-03 04:05:17.000 +07:00
├─╮ resolve concurrent operations
│ │ args: jj op log
○ │ 467d42715f00 [email protected] 2001-02-03 04:05:10.000 +07:00 - 2001-02-03 04:05:10.000 +07:00
│ │ commit 220cb0b1b5d1c03cc0d351139d824598bb3c1967
│ │ args: jj commit -m 'commit 3'
│ ○ 603773cdd351 [email protected] 2001-02-03 04:05:12.000 +07:00 - 2001-02-03 04:05:12.000 +07:00
├─╯ commit 81a4ef3dd421f3184289df1c58bd3a16ea1e3d8e
│ args: jj commit '--at-op=@--' -m 'commit 4'
○ 5d0ab09ab0fa [email protected] 2001-02-03 04:05:08.000 +07:00 - 2001-02-03 04:05:08.000 +07:00
│ commit 230dd059e1b059aefc0da06a2e5a7dbf22362f22
│ args: jj commit -m 'commit 1'
○ b51416386f26 [email protected] 2001-02-03 04:05:07.000 +07:00 - 2001-02-03 04:05:07.000 +07:00
│ add workspace 'default'
○ 9a7d829846af [email protected] 2001-02-03 04:05:07.000 +07:00 - 2001-02-03 04:05:07.000 +07:00
│ initialize repo
○ 000000000000 root()
"###);
insta::assert_snapshot!(stderr, @r###"
Concurrent modification detected, resolving automatically.
"###);
}

#[test]
fn test_op_recover_from_bad_gc() {
let test_env = TestEnvironment::default();
Expand Down Expand Up @@ -551,8 +627,7 @@ fn test_op_recover_from_bad_gc() {

// Do concurrent modification to make the situation even worse. At this
// point, the index can be loaded, so this command succeeds.
// TODO: test_env.jj_cmd_ok(&repo_path, &["--at-op=@-", "describe", "-m4.1"]);
// TODO: "op abandon" doesn't work if there are multiple op heads.
test_env.jj_cmd_ok(&repo_path, &["--at-op=@-", "describe", "-m4.1"]);

let stderr =
test_env.jj_cmd_internal_error(&repo_path, &["--at-op", head_op_id, "debug", "reindex"]);
Expand All @@ -563,7 +638,10 @@ fn test_op_recover_from_bad_gc() {
"###);

// "op log" should still be usable.
let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["op", "log", "--ignore-working-copy"]);
let (stdout, stderr) = test_env.jj_cmd_ok(
&repo_path,
&["op", "log", "--ignore-working-copy", "--at-op", head_op_id],
);
insta::assert_snapshot!(stdout, @r###"
@ 43d51d9b0c0c [email protected] 2001-02-03 04:05:12.000 +07:00 - 2001-02-03 04:05:12.000 +07:00
│ describe commit 37bb762e5dc08073ec4323bdffc023a0f0cc901e
Expand Down Expand Up @@ -592,19 +670,23 @@ fn test_op_recover_from_bad_gc() {
let (_stdout, stderr) =
test_env.jj_cmd_ok(&repo_path, &["op", "abandon", &format!("..{bad_op_id}")]);
insta::assert_snapshot!(stderr, @r###"
Abandoned 4 operations and reparented 3 descendant operations.
Abandoned 4 operations and reparented 4 descendant operations.
"###);

// The repo should no longer be corrupt.
let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["log"]);
insta::assert_snapshot!(stdout, @r###"
@ mzvwutvl [email protected] 2001-02-03 08:05:12 6d868f04
│ (empty) 4
○ mzvwutvl?? [email protected] 2001-02-03 08:05:15 dc2c6d52
│ (empty) 4.1
│ @ mzvwutvl?? [email protected] 2001-02-03 08:05:12 6d868f04
├─╯ (empty) 4
○ zsuskuln [email protected] 2001-02-03 08:05:10 HEAD@git f652c321
│ (empty) (no description set)
◆ zzzzzzzz root() 00000000
"###);
insta::assert_snapshot!(stderr, @"");
insta::assert_snapshot!(stderr, @r###"
Concurrent modification detected, resolving automatically.
"###);
}

#[test]
Expand Down
2 changes: 1 addition & 1 deletion lib/src/op_walk.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ pub fn walk_ancestors(head_ops: &[Operation]) -> impl Iterator<Item = OpStoreRes
/// Stats about `reparent_range()`.
#[derive(Clone, Debug, Eq, PartialEq)]
pub struct ReparentStats {
/// New head operation ids.
/// New head operation ids in order of the old `head_ops`.
pub new_head_ids: Vec<OperationId>,
/// The number of rewritten operations.
pub rewritten_count: usize,
Expand Down

0 comments on commit 7bdb28f

Please sign in to comment.