undo: always preserve git_refs by default

The result is equivalent to martinvonz#1541
but with many more tests

src/commands/operation.rs

@@ -208,16 +208,12 @@ fn view_with_desired_portions_restored(
 
 fn process_what_arg(
     what_arg: &[UndoWhatToRestore],
-    colocated: bool,
+    _colocated: bool,
 ) -> BTreeSet<UndoWhatToRestore> {
     if !what_arg.is_empty() {
         what_arg.iter().cloned().collect()
     } else {
-        let mut default_what =
-            btreeset!(UndoWhatToRestore::Repo, UndoWhatToRestore::RemoteTracking);
-        if !colocated {
-            default_what.insert(UndoWhatToRestore::GitTracking);
-        }
+        let default_what = btreeset!(UndoWhatToRestore::Repo, UndoWhatToRestore::RemoteTracking);
         default_what
     }
 }

tests/test_git_fetch.rs

@@ -668,12 +668,11 @@ fn test_git_fetch_undo() {
     "###);
     // Now try to fetch just one branch
     let stdout = test_env.jj_cmd_success(&target_jj_repo_path, &["git", "fetch", "--branch", "b"]);
-    insta::assert_snapshot!(stdout, @"");
+    insta::assert_snapshot!(stdout, @r###"
+    Nothing changed.
+    "###);
     insta::assert_snapshot!(get_log_output(&test_env, &target_jj_repo_path), @r###"
-    ◉  c7d4bdcbc215 descr_for_b b
-    ◉  ff36dc55760e descr_for_trunk1
-    │ @  230dd059e1b0
-    ├─╯
+    @  230dd059e1b0
     ◉  000000000000
     "###);
 }

tests/test_git_import_export.rs

@@ -104,6 +104,7 @@ fn test_git_export_undo() {
 
     // "git export" can't be undone.
     insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["op", "undo"]), @r###"
+    Nothing changed.
     "###);
     insta::assert_debug_snapshot!(get_git_repo_refs(&git_repo), @r###"
     [
@@ -118,7 +119,9 @@ fn test_git_export_undo() {
 
     // 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.
-    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["git", "export"]), @"");
+    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["git", "export"]), @r###"
+    Nothing changed.
+    "###);
 }
 
 #[test]
@@ -149,11 +152,17 @@ fn test_git_import_undo() {
     let stdout = test_env.jj_cmd_success(&repo_path, &["op", "restore", &base_operation_id]);
     insta::assert_snapshot!(stdout, @r###"
     "###);
-    insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @"");
+    insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###"
+    a (deleted)
+      @git: 230dd059e1b0 (no description set)
+    "###);
     // Try "git import" again, which should re-import the branch "a".
-    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["git", "import"]), @"");
+    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["git", "import"]), @r###"
+    Nothing changed.
+    "###);
     insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###"
-    a: 230dd059e1b0 (no description set)
+    a (deleted)
+      @git: 230dd059e1b0 (no description set)
     "###);
 
     // If we don't restore the git_refs, undoing the import removes the local branch
@@ -169,6 +178,7 @@ fn test_git_import_undo() {
         ],
     );
     insta::assert_snapshot!(stdout, @r###"
+    Nothing changed.
     "###);
     insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###"
     a (deleted)
@@ -244,7 +254,10 @@ fn test_git_import_move_export_with_default_undo() {
     Working copy now at: 230dd059e1b0 (no description set)
     Parent commit      : 000000000000 (no description set)
     "###);
-    insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @"");
+    insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###"
+    a (deleted)
+      @git: 096dc80da670 (no description set)
+    "###);
     insta::assert_debug_snapshot!(get_git_repo_refs(&git_repo), @r###"
     [
         (
@@ -258,9 +271,12 @@ fn test_git_import_move_export_with_default_undo() {
 
     // The last branch "a" state is imported from git. No idea what's the most
     // intuitive result here.
-    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["git", "import"]), @"");
+    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["git", "import"]), @r###"
+    Nothing changed.
+    "###);
     insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###"
-    a: 096dc80da670 (no description set)
+    a (deleted)
+      @git: 096dc80da670 (no description set)
     "###);
 }
 

tests/test_undo.rs

@@ -85,20 +85,14 @@ fn test_git_push_undo() {
     //                     |         |tracking|
     //   ------------------------------------------
     //    local  `main`    | BB      |   --   | --
-    //    remote-tracking  | AA      |   AA   | BB
+    //    remote-tracking  | AA      |   BB   | BB
     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"]);
-    // TODO: This should probably not be considered a conflict. It currently is
-    // because the undo made us forget that the remote was at v2, so the fetch
-    // made us think it updated from v1 to v2 (instead of the no-op it could
-    // have been).
+    // Conflict is gone, though the refs are in a strange state
     insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###"
-    main (conflicted):
-      - 0cffb6146141 AA
-      + 0a3e99f08a48 CC
-      + 8c05de152218 BB
-      @origin (behind by 1 commits): 8c05de152218 BB
+    main: 0a3e99f08a48 CC
+      @origin (ahead by 1 commits, behind by 1 commits): 0cffb6146141 AA
     "###);
 }
 
@@ -141,26 +135,34 @@ fn test_git_push_undo_with_import() {
     //                     |         |tracking|
     //   ------------------------------------------
     //    local  `main`    | BB      |   --   | --
-    //    remote-tracking  | AA      |   AA   | BB
+    //    remote-tracking  | AA      |   BB   | BB
 
-    // PROBLEM: inserting this import changes the outcome compared to previous test
-    // TODO: decide if this is the better behavior, and whether import of
-    // remote-tracking branches should happen on every operation.
+    // import becomes a no-op
     test_env.jj_cmd_success(&repo_path, &["git", "import"]);
     //                     | jj refs | jj's   | git
     //                     |         | git    | repo
     //                     |         |tracking|
     //   ------------------------------------------
     //    local  `main`    | BB      |   --   | --
-    //    remote-tracking  | BB      |   BB   | BB
+    //    remote-tracking  | AA      |   BB   | BB
+
+    // Export makes the git repo state more sane (though the test doesn't make it
+    // explicit)
+    test_env.jj_cmd_success(&repo_path, &["git", "export"]);
+    //                     | jj refs | jj's   | git
+    //                     |         | git    | repo
+    //                     |         |tracking|
+    //   ------------------------------------------
+    //    local  `main`    | 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"]);
     // There is not a conflict. This seems like a good outcome; undoing `git push`
     // was essentially a no-op.
     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
+      @origin (ahead by 1 commits, behind by 1 commits): 0cffb6146141 AA
     "###);
 }