diff --git a/cli/src/commands/operation.rs b/cli/src/commands/operation.rs index 450cb3ae48..6e61e766b2 100644 --- a/cli/src/commands/operation.rs +++ b/cli/src/commands/operation.rs @@ -54,13 +54,8 @@ pub struct OperationRestoreArgs { /// What portions of the local state to restore (can be repeated) /// - /// Defaults to everything for non-colocated repos. - /// - /// Defaults to `repo` and `remote-tracking` for colocated repos. This - /// ensures that the automatic `jj git export` succeeds. - /// /// This option is EXPERIMENTAL. - #[arg(long)] + #[arg(long, value_enum, default_values_t = DEFAULT_UNDO_WHAT)] what: Vec, } @@ -78,13 +73,8 @@ pub struct OperationUndoArgs { /// What portions of the local state to restore (can be repeated) /// - /// Defaults to everything for non-colocated repos. - /// - /// Defaults to `repo` and `remote-tracking` for colocated repos. This - /// ensures that the automatic `jj git export` succeeds. - /// /// This option is EXPERIMENTAL. - #[arg(long)] + #[arg(long, value_enum, default_values_t = DEFAULT_UNDO_WHAT)] what: Vec, } @@ -95,10 +85,11 @@ enum UndoWhatToRestore { /// The remote-tracking branches. Do not restore these if you'd like to push /// after the undo RemoteTracking, - /// Remembered git repo state from the last `jj git import` - GitTracking, } +const DEFAULT_UNDO_WHAT: [UndoWhatToRestore; 2] = + [UndoWhatToRestore::Repo, UndoWhatToRestore::RemoteTracking]; + fn cmd_op_log( ui: &mut Ui, command: &CommandHelper, @@ -179,13 +170,8 @@ fn view_with_desired_portions_restored( current_view.clone() }; - let git_source_view = if what.contains(&UndoWhatToRestore::GitTracking) { - view_being_restored - } else { - current_view - }; - new_view.git_refs = git_source_view.git_refs.clone(); - new_view.git_head = git_source_view.git_head.clone(); + new_view.git_refs = current_view.git_refs.clone(); + new_view.git_head = current_view.git_head.clone(); if what.contains(&UndoWhatToRestore::RemoteTracking) == what.contains(&UndoWhatToRestore::Repo) { @@ -229,39 +215,6 @@ fn view_with_desired_portions_restored( new_view } -fn process_what_arg(what_arg: &[UndoWhatToRestore], colocated: bool) -> Vec { - if !what_arg.is_empty() { - what_arg.to_vec() - } else { - let mut default_what = vec![UndoWhatToRestore::Repo, UndoWhatToRestore::RemoteTracking]; - if !colocated { - // In a colocated repo, restoring the git-tracking refs is harmful - // (https://github.com/martinvonz/jj/issues/922). - // - // The issue is that `jj undo` does not directly change the local - // git repo's branches. Keeping those up to date the job of the - // automatic `jj git import` and `jj git export`, and they rely on the - // git-tracking refs matching the git repo's branches. - // - // Consider, for example, undoing a `jj branch set` command. If the - // git-tracking refs were restored by `undo`, they would no longer - // match the actual positions of branches in the git repo. So, the - // automatic `jj git export` would fail and the automatic `jj git - // import` would create a conflict, as demonstrated by the bug - // linked above. - // - // So, we have `undo` *not* move the git-tracking branches. After - // the undo, git-tracking refs will still match the actual positions - // of the git repo's branches (in the normal case where they matched - // before the undo). The automatic `jj git export` that happens - // immediately after the undo will successfully export whatever - // changes to branches `undo` caused. - default_what.push(UndoWhatToRestore::GitTracking); - } - default_what - } -} - pub fn cmd_op_undo( ui: &mut Ui, command: &CommandHelper, @@ -269,7 +222,6 @@ pub fn cmd_op_undo( ) -> Result<(), CommandError> { let mut workspace_command = command.workspace_helper(ui)?; let bad_op = workspace_command.resolve_single_op(&args.operation)?; - let repo_is_colocated = workspace_command.working_copy_shared_with_git(); let parent_ops = bad_op.parents(); if parent_ops.len() > 1 { return Err(user_error("Cannot undo a merge operation")); @@ -287,7 +239,7 @@ pub fn cmd_op_undo( let new_view = view_with_desired_portions_restored( tx.repo().view().store_view(), tx.base_repo().view().store_view(), - &process_what_arg(&args.what, repo_is_colocated), + &args.what, ); tx.mut_repo().set_view(new_view); tx.finish(ui)?; @@ -302,13 +254,12 @@ fn cmd_op_restore( ) -> Result<(), CommandError> { let mut workspace_command = command.workspace_helper(ui)?; let target_op = workspace_command.resolve_single_op(&args.operation)?; - let repo_is_colocated = workspace_command.working_copy_shared_with_git(); let mut tx = workspace_command .start_transaction(&format!("restore to operation {}", target_op.id().hex())); let new_view = view_with_desired_portions_restored( target_op.view()?.store_view(), tx.base_repo().view().store_view(), - &process_what_arg(&args.what, repo_is_colocated), + &args.what, ); tx.mut_repo().set_view(new_view); tx.finish(ui)?; diff --git a/cli/tests/test_git_import_export.rs b/cli/tests/test_git_import_export.rs index 99956f8dcd..2c5a441449 100644 --- a/cli/tests/test_git_import_export.rs +++ b/cli/tests/test_git_import_export.rs @@ -88,8 +88,14 @@ fn test_git_export_undo() { a: qpvuntsm 230dd059 (empty) (no description set) "###); insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["git", "export"]), @""); + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["log", "-ra@git"]), @r###" + @ qpvuntsm test.user@example.com 2001-02-03 04:05:07.000 +07:00 a 230dd059 + │ (empty) (no description set) + ~ + "###); - // "git export" can't be undone. + // Exported refs won't be removed by undoing the export, but the git-tracking + // branch is. This is the same as remote-tracking branches. insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["op", "undo"]), @r###" "###); insta::assert_debug_snapshot!(get_git_repo_refs(&git_repo), @r###" @@ -102,10 +108,18 @@ fn test_git_export_undo() { ), ] "###); + insta::assert_snapshot!(test_env.jj_cmd_failure(&repo_path, &["log", "-ra@git"]), @r###" + Error: Revision "a@git" doesn't exist + Hint: Did you mean "a"? + "###); - // This would re-export branch "a" as the internal state has been rolled back. - // It might be better to preserve the state, and say "Nothing changed" here. + // This would re-export branch "a" and create git-tracking branch. insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["git", "export"]), @""); + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["log", "-ra@git"]), @r###" + @ qpvuntsm test.user@example.com 2001-02-03 04:05:07.000 +07:00 a 230dd059 + │ (empty) (no description set) + ~ + "###); } #[test] @@ -132,7 +146,7 @@ fn test_git_import_undo() { a: qpvuntsm 230dd059 (empty) (no description set) "###); - // "git import" can be undone by default in non-colocated repositories. + // "git import" can be undone by default. let stdout = test_env.jj_cmd_success(&repo_path, &["op", "restore", &base_operation_id]); insta::assert_snapshot!(stdout, @r###" "###); @@ -142,27 +156,6 @@ fn test_git_import_undo() { insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###" a: qpvuntsm 230dd059 (empty) (no description set) "###); - - // Even if we don't restore the git_refs, "git import" can re-import the - // local branch "a". - // TODO: This will be the default "op restore" mode. - let stdout = test_env.jj_cmd_success( - &repo_path, - &[ - "op", - "restore", - &base_operation_id, - "--what=repo", - "--what=remote-tracking", - ], - ); - insta::assert_snapshot!(stdout, @r###" - "###); - insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @""); - insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["git", "import"]), @""); - insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###" - a: qpvuntsm 230dd059 (empty) (no description set) - "###); } #[test]