diff --git a/CHANGELOG.md b/CHANGELOG.md index ab40682716..559f1fa5bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -168,6 +168,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [#697](https://github.com/martinvonz/jj/issues/697), [#1608](https://github.com/martinvonz/jj/issues/1608) +* In colocated repos, a bug causing conflicts when undoing branch moves (#922) + has been fixed. Some surprising behaviors related to undoing `jj git push` or + `jj git fetch` remain. + ## [0.7.0] - 2023-02-16 ### Breaking changes diff --git a/src/commands/operation.rs b/src/commands/operation.rs index 254cf8491e..fd0162417c 100644 --- a/src/commands/operation.rs +++ b/src/commands/operation.rs @@ -206,15 +206,40 @@ fn view_with_desired_portions_restored( new_view } -fn process_what_arg(what_arg: &[UndoWhatToRestore]) -> BTreeSet { +fn process_what_arg( + what_arg: &[UndoWhatToRestore], + colocated: bool, +) -> BTreeSet { if !what_arg.is_empty() { what_arg.iter().cloned().collect() } else { - btreeset!( - UndoWhatToRestore::Repo, - UndoWhatToRestore::RemoteTracking, - UndoWhatToRestore::GitTracking - ) + let mut default_what = + btreeset!(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.insert(UndoWhatToRestore::GitTracking); + } + default_what } } @@ -225,6 +250,7 @@ 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")); @@ -242,7 +268,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), + process_what_arg(&args.what, repo_is_colocated), ); tx.mut_repo().set_view(new_view); tx.finish(ui)?; @@ -257,12 +283,13 @@ 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), + process_what_arg(&args.what, repo_is_colocated), ); tx.mut_repo().set_view(new_view); tx.finish(ui)?; diff --git a/tests/test_git_colocated.rs b/tests/test_git_colocated.rs index c481dc6f8c..16405ff990 100644 --- a/tests/test_git_colocated.rs +++ b/tests/test_git_colocated.rs @@ -374,10 +374,8 @@ fn test_git_colocated_squash_undo() { // TODO: There should be no divergence here; 2f376ea1478c should be hidden // (#922) insta::assert_snapshot!(get_log_output_divergence(&test_env, &repo_path), @r###" - ◉ qpvuntsmwlqt 2f376ea1478c A master !divergence! - │ @ rlvkpnrzqnoo 8f71e3b6a3be - │ ◉ qpvuntsmwlqt a86754f975f9 A HEAD@git !divergence! - ├─╯ + @ rlvkpnrzqnoo 8f71e3b6a3be + ◉ qpvuntsmwlqt a86754f975f9 A master HEAD@git ◉ zzzzzzzzzzzz 000000000000 "###); } diff --git a/tests/test_undo.rs b/tests/test_undo.rs index 027cfa2e2e..00d1834ab0 100644 --- a/tests/test_undo.rs +++ b/tests/test_undo.rs @@ -199,27 +199,32 @@ fn test_git_push_undo_colocated() { // Undo the push test_env.jj_cmd_success(&repo_path, &["op", "restore", &pre_push_opid]); - // === Before auto-import ==== + // === Before auto-export ==== // | jj refs | jj's | git // | | git | repo // | |tracking| // ------------------------------------------ // local `main` | BB | BB | BB - // remote-tracking | AA | AA | BB - // === After automatic `jj git import` ==== + // remote-tracking | AA | BB | BB + // === After automatic `jj git export` ==== // | jj refs | jj's | git // | | git | repo // | |tracking| // ------------------------------------------ // local `main` | BB | BB | BB - // remote-tracking | BB | BB | BB + // remote-tracking | AA | AA | AA test_env.advance_test_rng_seed_to_multiple_of(100_000); test_env.jj_cmd_success(&repo_path, &["describe", "-m", "CC"]); test_env.jj_cmd_success(&repo_path, &["git", "fetch"]); - // This currently gives an identical result to `test_git_push_undo_import` + // We have the same conflict as `test_git_push_undo`. TODO: why did we get the + // same result in a seemingly different way? insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###" - main: 0a3e99f08a48 CC - @origin (ahead by 1 commits, behind by 1 commits): 8c05de152218 BB + main (conflicted): + - 0cffb6146141 AA + + 0a3e99f08a48 CC + + 8c05de152218 BB + @git (behind by 1 commits): 0a3e99f08a48 CC + @origin (behind by 1 commits): 8c05de152218 BB "###); }