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

@@ -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) {

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();
 

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()));
+}

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