From 1f27a78957375b1c9c76642e547be1913804f70b Mon Sep 17 00:00:00 2001
From: Martin von Zweigbergk <martinvonz@google.com>
Date: Thu, 14 Jan 2021 23:54:11 -0800
Subject: [PATCH] view: make remove_head() not add parents as heads

I think it's better to let the caller decide if the parents should be
added. One use case for removing a head is when fetching from a Git
remote where a branch has been rewritten. In that case, it's probably
the best user experience to remove the old head. With the current
semantics of `View::remove_head()`, we would need to walk up the graph
to find a commit that's an ancestor and for each commit we remove as
head, its parents get temporarily added as heads. It's much easier for
callers that want to add the parents as heads to do that.
---
 lib/src/view.rs               |  4 ----
 lib/tests/test_index.rs       |  1 -
 lib/tests/test_transaction.rs | 40 +++++++++++++++++++++++++++++++++--
 src/commands.rs               |  3 +++
 4 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/lib/src/view.rs b/lib/src/view.rs
index 3d8fd51ba..690d933ac 100644
--- a/lib/src/view.rs
+++ b/lib/src/view.rs
@@ -434,10 +434,6 @@ impl MutableView {
 
     pub fn remove_head(&mut self, head: &Commit) {
         self.data.head_ids.remove(head.id());
-        for parent in head.parents() {
-            self.data.head_ids.insert(parent.id().clone());
-        }
-        self.remove_non_heads();
     }
 
     pub fn insert_git_ref(&mut self, name: String, commit_id: CommitId) {
diff --git a/lib/tests/test_index.rs b/lib/tests/test_index.rs
index 2a7eea411..7e59b25fb 100644
--- a/lib/tests/test_index.rs
+++ b/lib/tests/test_index.rs
@@ -261,7 +261,6 @@ fn test_index_commits_previous_operations(use_git: bool) {
 
     let mut tx = repo.start_transaction("test");
     tx.remove_head(&commit_c);
-    tx.remove_head(&commit_b);
     tx.commit();
     Arc::get_mut(&mut repo).unwrap().reload();
 
diff --git a/lib/tests/test_transaction.rs b/lib/tests/test_transaction.rs
index 06c976419..5f625f294 100644
--- a/lib/tests/test_transaction.rs
+++ b/lib/tests/test_transaction.rs
@@ -343,8 +343,6 @@ fn test_add_head_ancestor(use_git: bool) {
     let (_temp_dir, mut repo) = testutils::init_repo(&settings, use_git);
     let store = repo.store();
 
-    // Create a commit outside of the repo by using a temporary transaction. Then
-    // add that as a head.
     let mut tx = repo.start_transaction("test");
     let commit1 = CommitBuilder::for_new_commit(&settings, store, store.empty_tree_id().clone())
         .write_to_transaction(&mut tx);
@@ -363,3 +361,41 @@ fn test_add_head_ancestor(use_git: bool) {
     assert!(!heads.contains(commit1.id()));
     tx.discard();
 }
+
+#[test_case(false ; "local store")]
+// #[test_case(true ; "git store")]
+fn test_remove_head(use_git: bool) {
+    // Test that Transaction::remove_head() removes the head, and that it's still
+    // removed after commit.
+    let settings = testutils::user_settings();
+    let (_temp_dir, mut repo) = testutils::init_repo(&settings, use_git);
+    let store = repo.store();
+
+    let mut tx = repo.start_transaction("test");
+    let commit1 = CommitBuilder::for_new_commit(&settings, store, store.empty_tree_id().clone())
+        .write_to_transaction(&mut tx);
+    let commit2 = CommitBuilder::for_new_commit(&settings, store, store.empty_tree_id().clone())
+        .set_parents(vec![commit1.id().clone()])
+        .write_to_transaction(&mut tx);
+    let commit3 = CommitBuilder::for_new_commit(&settings, store, store.empty_tree_id().clone())
+        .set_parents(vec![commit2.id().clone()])
+        .write_to_transaction(&mut tx);
+    tx.commit();
+    Arc::get_mut(&mut repo).unwrap().reload();
+
+    let mut tx = repo.start_transaction("test");
+    let heads: HashSet<_> = tx.as_repo().view().heads().cloned().collect();
+    assert!(heads.contains(commit3.id()));
+    tx.remove_head(&commit3);
+    let heads: HashSet<_> = tx.as_repo().view().heads().cloned().collect();
+    assert!(!heads.contains(commit3.id()));
+    assert!(!heads.contains(commit2.id()));
+    assert!(!heads.contains(commit1.id()));
+    tx.commit();
+
+    Arc::get_mut(&mut repo).unwrap().reload();
+    let heads: HashSet<_> = repo.view().heads().cloned().collect();
+    assert!(!heads.contains(commit3.id()));
+    assert!(!heads.contains(commit2.id()));
+    assert!(!heads.contains(commit1.id()));
+}
diff --git a/src/commands.rs b/src/commands.rs
index c80b52ffc..4b83013e8 100644
--- a/src/commands.rs
+++ b/src/commands.rs
@@ -1384,6 +1384,9 @@ fn cmd_discard(
     let commit = resolve_revision_arg(ui, mut_repo, sub_matches)?;
     let mut tx = repo.start_transaction(&format!("discard commit {}", commit.id().hex()));
     tx.remove_head(&commit);
+    for parent in commit.parents() {
+        tx.add_head(&parent);
+    }
     // TODO: also remove descendants
     tx.commit();
     // TODO: check out parent/ancestor if the current commit got hidden