undo: preserve git-tracking refs in colocated repos by default

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

src/commands/operation.rs

@@ -207,15 +207,36 @@ fn view_with_desired_portions_restored(
     new_view
 }
 
-fn process_what_arg(what_arg: &[UndoWhatToRestore]) -> Vec<UndoWhatToRestore> {
+fn process_what_arg(what_arg: &[UndoWhatToRestore], colocated: bool) -> Vec<UndoWhatToRestore> {
     if !what_arg.is_empty() {
         what_arg.to_vec()
     } else {
-        vec![
+        let mut default_what = vec![UndoWhatToRestore::Repo, UndoWhatToRestore::RemoteTracking];
-            UndoWhatToRestore::Repo,
+        if !colocated {
-            UndoWhatToRestore::RemoteTracking,
+            // In a colocated repo, restoring the git-tracking refs is harmful
-            UndoWhatToRestore::GitTracking,
+            // (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
     }
 }
 
@@ -226,6 +247,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"));
@@ -243,7 +265,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)?;
@@ -258,12 +280,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)?;

tests/test_git_colocated.rs

@@ -371,10 +371,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
-    │ @  rlvkpnrzqnoo 8f71e3b6a3be
+    ◉  qpvuntsmwlqt a86754f975f9 A master HEAD@git
-    │ ◉  qpvuntsmwlqt a86754f975f9 A HEAD@git !divergence!
-    ├─╯
     ◉  zzzzzzzzzzzz 000000000000
     "###);
 }

tests/test_undo.rs

@@ -235,30 +235,36 @@ 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
+    //    remote-tracking  | AA      |   BB   | BB
-    //       === After automatic `jj git import` ====
+    //       === 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
     insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###"
     main: 8c05de152218 BB
+      @origin (ahead by 1 commits, behind by 1 commits): 0cffb6146141 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
+    main (conflicted):
-      @origin (ahead by 1 commits, behind by 1 commits): 8c05de152218 BB
+      - 0cffb6146141 AA
+      + 0a3e99f08a48 CC
+      + 8c05de152218 BB
+      @git (behind by 1 commits): 0a3e99f08a48 CC
+      @origin (behind by 1 commits): 8c05de152218 BB
     "###);
 }