From 7bdb28f1fe6e4aebf9b0be9adfe6730d1f24c10a Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Tue, 23 Jul 2024 23:15:56 +0900 Subject: [PATCH] cli: make "op abandon" not fail with multiple op heads 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=" can't be used because ancestor operations would be preserved by the other head. --- cli/src/commands/operation/abandon.rs | 68 +++++++++++-------- cli/tests/test_operations.rs | 98 ++++++++++++++++++++++++--- lib/src/op_walk.rs | 2 +- 3 files changed, 132 insertions(+), 36 deletions(-) diff --git a/cli/src/commands/operation/abandon.rs b/cli/src/commands/operation/abandon.rs index 5334835cb..082c3e2ab 100644 --- a/cli/src/commands/operation/abandon.rs +++ b/cli/src/commands/operation/abandon.rs @@ -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 @@ -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(¤t_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, ¤t_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(); @@ -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()?; @@ -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(¤t_head_op), + &abandon_head_ops, + ¤t_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(¤t_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(()); } @@ -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(()) diff --git a/cli/tests/test_operations.rs b/cli/tests/test_operations.rs index 9d2d6e732..9d2b08c04 100644 --- a/cli/tests/test_operations.rs +++ b/cli/tests/test_operations.rs @@ -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` "###); @@ -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 test-username@host.example.com 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 test-username@host.example.com 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 test-username@host.example.com 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 test-username@host.example.com 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 test-username@host.example.com 2001-02-03 04:05:07.000 +07:00 - 2001-02-03 04:05:07.000 +07:00 + │ add workspace 'default' + ○ 9a7d829846af test-username@host.example.com 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(); @@ -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"]); @@ -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 test-username@host.example.com 2001-02-03 04:05:12.000 +07:00 - 2001-02-03 04:05:12.000 +07:00 │ describe commit 37bb762e5dc08073ec4323bdffc023a0f0cc901e @@ -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 test.user@example.com 2001-02-03 08:05:12 6d868f04 - │ (empty) 4 + ○ mzvwutvl?? test.user@example.com 2001-02-03 08:05:15 dc2c6d52 + │ (empty) 4.1 + │ @ mzvwutvl?? test.user@example.com 2001-02-03 08:05:12 6d868f04 + ├─╯ (empty) 4 ○ zsuskuln test.user@example.com 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] diff --git a/lib/src/op_walk.rs b/lib/src/op_walk.rs index a0cf1a65e..baeb57ccd 100644 --- a/lib/src/op_walk.rs +++ b/lib/src/op_walk.rs @@ -250,7 +250,7 @@ pub fn walk_ancestors(head_ops: &[Operation]) -> impl Iterator, /// The number of rewritten operations. pub rewritten_count: usize,