From 4478055e1dad135a012f33a72963e8a6b22c9210 Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Fri, 24 May 2024 15:39:09 +0900
Subject: [PATCH] cli: don't abandon non-discardable old wc commit by
 import_git_head()

Perhaps, the original intent was to abandon non-empty working-copy commit
assuming it was rewritten by git (therefore it should be superseded by the
current working-copy content.) However, there are other reasons that could
make the HEAD out of sync (including concurrent jj operations), and abandoning
non-empty commit can be a disaster. This patch turns it to safer side, and let
user abandon non-empty commit manually.

Fixes #3747
---
 cli/src/cli_util.rs             | 11 +++-----
 cli/tests/test_git_colocated.rs | 46 ++++++++++++++++++++++++++-------
 2 files changed, 41 insertions(+), 16 deletions(-)

diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs
index ab98de44d9..567b0e44fc 100644
--- a/cli/src/cli_util.rs
+++ b/cli/src/cli_util.rs
@@ -571,9 +571,10 @@ impl WorkspaceCommandHelper {
 
     /// Imports new HEAD from the colocated Git repo.
     ///
-    /// If the Git HEAD has changed, this function abandons our old checkout and
-    /// checks out the new Git HEAD. The working-copy state will be reset to
-    /// point to the new Git HEAD. The working-copy contents won't be updated.
+    /// If the Git HEAD has changed, this function checks out the new Git HEAD.
+    /// The old working-copy commit will be abandoned if it's discardable. The
+    /// working-copy state will be reset to point to the new Git HEAD. The
+    /// working-copy contents won't be updated.
     #[instrument(skip_all)]
     fn import_git_head(&mut self, ui: &mut Ui) -> Result<(), CommandError> {
         assert!(self.may_update_working_copy);
@@ -598,10 +599,6 @@ impl WorkspaceCommandHelper {
         let new_git_head = tx.mut_repo().view().git_head().clone();
         if let Some(new_git_head_id) = new_git_head.as_normal() {
             let workspace_id = self.workspace_id().to_owned();
-            if let Some(old_wc_commit_id) = self.repo().view().get_wc_commit_id(&workspace_id) {
-                tx.mut_repo()
-                    .record_abandoned_commit(old_wc_commit_id.clone());
-            }
             let new_git_head_commit = tx.mut_repo().store().get_commit(new_git_head_id)?;
             tx.mut_repo()
                 .check_out(workspace_id, &self.settings, &new_git_head_commit)?;
diff --git a/cli/tests/test_git_colocated.rs b/cli/tests/test_git_colocated.rs
index 1496d5bbd2..d09aa1734b 100644
--- a/cli/tests/test_git_colocated.rs
+++ b/cli/tests/test_git_colocated.rs
@@ -573,6 +573,12 @@ fn test_git_colocated_external_checkout() {
     let test_env = TestEnvironment::default();
     let repo_path = test_env.env_root().join("repo");
     let git_repo = git2::Repository::init(&repo_path).unwrap();
+    let git_check_out_ref = |name| {
+        git_repo
+            .set_head_detached(git_repo.find_reference(name).unwrap().target().unwrap())
+            .unwrap()
+    };
+
     test_env.jj_cmd_ok(&repo_path, &["git", "init", "--git-repo=."]);
     test_env.jj_cmd_ok(&repo_path, &["ci", "-m=A"]);
     test_env.jj_cmd_ok(&repo_path, &["branch", "create", "-r@-", "master"]);
@@ -589,15 +595,7 @@ fn test_git_colocated_external_checkout() {
     "###);
 
     // Check out another branch by external command
-    git_repo
-        .set_head_detached(
-            git_repo
-                .find_reference("refs/heads/master")
-                .unwrap()
-                .target()
-                .unwrap(),
-        )
-        .unwrap();
+    git_check_out_ref("refs/heads/master");
 
     // The old working-copy commit gets abandoned, but the whole branch should not
     // be abandoned. (#1042)
@@ -612,6 +610,36 @@ fn test_git_colocated_external_checkout() {
     insta::assert_snapshot!(stderr, @r###"
     Reset the working copy parent to the new Git HEAD.
     "###);
+
+    // Edit non-head commit
+    test_env.jj_cmd_ok(&repo_path, &["new", "description(B)"]);
+    test_env.jj_cmd_ok(&repo_path, &["new", "-m=C", "--no-edit"]);
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    ◉  99a813753d6db988d8fc436b0d6b30a54d6b2707 C
+    @  81e086b7f9b1dd7fde252e28bdcf4ba4abd86ce5
+    ◉  eccedddfa5152d99fc8ddd1081b375387a8a382a HEAD@git B
+    │ ◉  a86754f975f953fa25da4265764adc0c62e9ce6b master A
+    ├─╯
+    ◉  0000000000000000000000000000000000000000
+    "###);
+
+    // Check out another branch by external command
+    git_check_out_ref("refs/heads/master");
+
+    // The old working-copy commit shouldn't be abandoned. (#3747)
+    let (stdout, stderr) = get_log_output_with_stderr(&test_env, &repo_path);
+    insta::assert_snapshot!(stdout, @r###"
+    @  f9f6929eae811496820623f44199a9e04ee402e8
+    ◉  a86754f975f953fa25da4265764adc0c62e9ce6b master HEAD@git A
+    │ ◉  99a813753d6db988d8fc436b0d6b30a54d6b2707 C
+    │ ◉  81e086b7f9b1dd7fde252e28bdcf4ba4abd86ce5
+    │ ◉  eccedddfa5152d99fc8ddd1081b375387a8a382a B
+    ├─╯
+    ◉  0000000000000000000000000000000000000000
+    "###);
+    insta::assert_snapshot!(stderr, @r###"
+    Reset the working copy parent to the new Git HEAD.
+    "###);
 }
 
 #[test]