transaction: remove Drop implementation

I can't remember when the `Drop` implementation last helped me find a
bug, so let's just remove it.

lib/src/transaction.rs

@@ -29,7 +29,6 @@ pub struct Transaction {
     description: String,
     start_time: Timestamp,
     tags: HashMap<String, String>,
-    closed: bool,
 }
 
 impl Transaction {
@@ -41,7 +40,6 @@ impl Transaction {
             description: description.to_owned(),
             start_time: Timestamp::now(),
             tags: Default::default(),
-            closed: false,
         }
     }
 
@@ -94,21 +92,8 @@ impl Transaction {
             .index_store()
             .associate_file_with_operation(&index, operation.id())
             .unwrap();
-        self.closed = true;
         UnpublishedOperation::new(base_repo.loader(), operation, view, index)
     }
-
-    pub fn discard(mut self) {
-        self.closed = true;
-    }
-}
-
-impl Drop for Transaction {
-    fn drop(&mut self) {
-        if !std::thread::panicking() {
-            debug_assert!(self.closed, "Transaction was dropped without being closed.");
-        }
-    }
 }
 
 struct NewRepoData {

lib/tests/test_commit_builder.rs

@@ -171,7 +171,6 @@ fn test_commit_builder_descendants(use_git: bool) {
         .write_to_repo(tx.mut_repo());
     let mut rebaser = tx.mut_repo().create_descendant_rebaser(&settings);
     assert!(rebaser.rebase_next().is_none());
-    tx.discard();
 
     // Test with for_open_commit()
     let mut tx = repo.start_transaction("test");
@@ -184,7 +183,6 @@ fn test_commit_builder_descendants(use_git: bool) {
     .write_to_repo(tx.mut_repo());
     let mut rebaser = tx.mut_repo().create_descendant_rebaser(&settings);
     assert!(rebaser.rebase_next().is_none());
-    tx.discard();
 
     // Test with for_rewrite_from()
     let mut tx = repo.start_transaction("test");
@@ -193,7 +191,6 @@ fn test_commit_builder_descendants(use_git: bool) {
     let mut rebaser = tx.mut_repo().create_descendant_rebaser(&settings);
     assert_rebased(rebaser.rebase_next(), &commit3, &[&commit4]);
     assert!(rebaser.rebase_next().is_none());
-    tx.discard();
 
     // Test with for_rewrite_from() but new change id
     let mut tx = repo.start_transaction("test");
@@ -202,5 +199,4 @@ fn test_commit_builder_descendants(use_git: bool) {
         .write_to_repo(tx.mut_repo());
     let mut rebaser = tx.mut_repo().create_descendant_rebaser(&settings);
     assert!(rebaser.rebase_next().is_none());
-    tx.discard();
 }

lib/tests/test_git.rs

@@ -352,8 +352,6 @@ fn test_fetch_prune_deleted_ref() {
         .unwrap();
     git::fetch(tx.mut_repo(), &clone_git_repo, "origin").unwrap();
     assert!(tx.mut_repo().get_branch("main").is_none());
-
-    tx.discard();
 }
 
 #[test]
@@ -382,7 +380,6 @@ fn test_fetch_no_default_branch() {
     let default_branch = git::fetch(tx.mut_repo(), &clone_git_repo, "origin").unwrap();
     // There is no default branch
     assert_eq!(default_branch, None);
-    tx.discard();
 }
 
 #[test]
@@ -398,7 +395,6 @@ fn test_fetch_no_such_remote() {
     let mut tx = jj_repo.start_transaction("test");
     let result = git::fetch(tx.mut_repo(), &git_repo, "invalid-remote");
     assert!(matches!(result, Err(GitFetchError::NoSuchRemote(_))));
-    tx.discard();
 }
 
 struct PushTestSetup {

lib/tests/test_index.rs

@@ -396,7 +396,6 @@ fn test_index_commits_incremental_already_indexed(use_git: bool) {
     let mut_repo = tx.mut_repo();
     mut_repo.add_head(&commit_a);
     assert_eq!(mut_repo.index().num_commits(), 2 + 1);
-    tx.discard();
 }
 
 #[must_use]

lib/tests/test_init.rs

@@ -30,7 +30,6 @@ fn test_init_local() {
     // Just test that we can write a commit to the store
     let mut tx = repo.start_transaction("test");
     testutils::create_random_commit(&settings, &repo).write_to_repo(tx.mut_repo());
-    tx.discard();
 }
 
 #[test]
@@ -46,7 +45,6 @@ fn test_init_internal_git() {
     // Just test that we ca write a commit to the store
     let mut tx = repo.start_transaction("test");
     testutils::create_random_commit(&settings, &repo).write_to_repo(tx.mut_repo());
-    tx.discard();
 }
 
 #[test]
@@ -66,7 +64,6 @@ fn test_init_external_git() {
     // Just test that we can write a commit to the store
     let mut tx = repo.start_transaction("test");
     testutils::create_random_commit(&settings, &repo).write_to_repo(tx.mut_repo());
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]

lib/tests/test_mut_repo.rs

@@ -90,7 +90,6 @@ fn test_checkout_previous_not_empty(use_git: bool) {
     mut_repo.check_out(&settings, &new_checkout);
     mut_repo.create_descendant_rebaser(&settings).rebase_all();
     assert!(mut_repo.view().heads().contains(old_checkout.id()));
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -122,7 +121,6 @@ fn test_checkout_previous_empty(use_git: bool) {
     mut_repo.check_out(&settings, &new_checkout);
     mut_repo.create_descendant_rebaser(&settings).rebase_all();
     assert!(!mut_repo.view().heads().contains(old_checkout.id()));
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -138,7 +136,7 @@ fn test_add_head_success(use_git: bool) {
     // add that as a head.
     let mut tx = repo.start_transaction("test");
     let new_commit = testutils::create_random_commit(&settings, repo).write_to_repo(tx.mut_repo());
-    tx.discard();
+    drop(tx);
 
     let index_stats = repo.index().stats();
     assert_eq!(index_stats.num_heads, 1);
@@ -188,7 +186,6 @@ fn test_add_head_ancestor(use_git: bool) {
     assert_eq!(index_stats.num_heads, 2);
     assert_eq!(index_stats.num_commits, 5);
     assert_eq!(index_stats.max_generation_number, 3);
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -214,7 +211,7 @@ fn test_add_head_not_immediate_child(use_git: bool) {
     let child = testutils::create_random_commit(&settings, &repo)
         .set_parents(vec![rewritten.id().clone()])
         .write_to_repo(tx.mut_repo());
-    tx.discard();
+    drop(tx);
 
     let index_stats = repo.index().stats();
     assert_eq!(index_stats.num_heads, 2);
@@ -233,7 +230,6 @@ fn test_add_head_not_immediate_child(use_git: bool) {
     assert_eq!(index_stats.num_heads, 3);
     assert_eq!(index_stats.num_commits, 5);
     assert_eq!(index_stats.max_generation_number, 2);
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -385,7 +381,6 @@ fn test_rebase_descendants_simple(use_git: bool) {
         .create_descendant_rebaser(&settings)
         .rebase_next()
         .is_none());
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -420,5 +415,4 @@ fn test_rebase_descendants_conflicting_rewrite(use_git: bool) {
         .create_descendant_rebaser(&settings)
         .rebase_next()
         .is_none());
-    tx.discard();
 }

lib/tests/test_revset.rs

@@ -258,8 +258,6 @@ fn test_resolve_symbol_checkout(use_git: bool) {
         resolve_symbol(mut_repo.as_repo_ref(), "@"),
         Ok(vec![commit2.id().clone()])
     );
-
-    tx.discard();
 }
 
 #[test]
@@ -382,8 +380,6 @@ fn test_resolve_symbol_git_refs() {
         resolve_symbol(mut_repo.as_repo_ref(), "refs/heads/conflicted"),
         Ok(vec![commit1.id().clone(), commit3.id().clone()])
     );
-
-    tx.discard();
 }
 
 fn resolve_commit_ids(repo: RepoRef, revset_str: &str) -> Vec<CommitId> {
@@ -421,8 +417,6 @@ fn test_evaluate_expression_root_and_checkout(use_git: bool) {
         resolve_commit_ids(mut_repo.as_repo_ref(), "@"),
         vec![commit1.id().clone()]
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -485,8 +479,6 @@ fn test_evaluate_expression_heads_of(use_git: bool) {
         resolve_commit_ids(mut_repo.as_repo_ref(), "heads(all())"),
         resolve_commit_ids(mut_repo.as_repo_ref(), "heads()")
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -548,8 +540,6 @@ fn test_evaluate_expression_parents(use_git: bool) {
         ),
         vec![commit3.id().clone(), commit2.id().clone()]
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -605,8 +595,6 @@ fn test_evaluate_expression_children(use_git: bool) {
         ),
         vec![commit5.id().clone()]
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -643,8 +631,6 @@ fn test_evaluate_expression_ancestors(use_git: bool) {
             root_commit.id().clone(),
         ]
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -708,8 +694,6 @@ fn test_evaluate_expression_range(use_git: bool) {
         ),
         vec![commit3.id().clone()]
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -780,8 +764,6 @@ fn test_evaluate_expression_dag_range(use_git: bool) {
             commit2.id().clone(),
         ]
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -833,8 +815,6 @@ fn test_evaluate_expression_descendants(use_git: bool) {
             commit2.id().clone(),
         ]
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -876,8 +856,6 @@ fn test_evaluate_expression_all(use_git: bool) {
             root_commit_id,
         ]
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -898,8 +876,6 @@ fn test_evaluate_expression_heads(use_git: bool) {
         resolve_commit_ids(mut_repo.as_repo_ref(), "heads()"),
         vec![commit2.id().clone(), checkout_id]
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -933,8 +909,6 @@ fn test_evaluate_expression_public_heads(use_git: bool) {
         resolve_commit_ids(mut_repo.as_repo_ref(), "public_heads()"),
         vec![commit2.id().clone(), commit1.id().clone()]
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -1004,8 +978,6 @@ fn test_evaluate_expression_git_refs(use_git: bool) {
             commit2.id().clone()
         ]
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -1075,8 +1047,6 @@ fn test_evaluate_expression_branches(use_git: bool) {
             commit2.id().clone()
         ]
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -1157,8 +1127,6 @@ fn test_evaluate_expression_remote_branches(use_git: bool) {
             commit2.id().clone()
         ]
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -1190,8 +1158,6 @@ fn test_evaluate_expression_merges(use_git: bool) {
         ),
         vec![commit5.id().clone()]
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -1235,8 +1201,6 @@ fn test_evaluate_expression_description(use_git: bool) {
         resolve_commit_ids(mut_repo.as_repo_ref(), "description(\"commit 2\",heads())"),
         vec![]
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -1310,8 +1274,6 @@ fn test_evaluate_expression_union(use_git: bool) {
             commit3.id().clone()
         ]
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -1352,8 +1314,6 @@ fn test_evaluate_expression_intersection(use_git: bool) {
         ),
         vec![]
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -1428,6 +1388,4 @@ fn test_evaluate_expression_difference(use_git: bool) {
         ),
         vec![commit4.id().clone()]
     );
-
-    tx.discard();
 }

lib/tests/test_rewrite.rs

@@ -68,8 +68,6 @@ fn test_rebase_descendants_sideways(use_git: bool) {
             new_commit_e.id().clone()
         }
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -123,8 +121,6 @@ fn test_rebase_descendants_forward(use_git: bool) {
             new_commit_e.id().clone()
         }
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -163,8 +159,6 @@ fn test_rebase_descendants_backward(use_git: bool) {
         *tx.mut_repo().view().heads(),
         hashset! {repo.view().checkout().clone(), new_commit_d.id().clone()}
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -215,8 +209,6 @@ fn test_rebase_descendants_internal_merge(use_git: bool) {
         *tx.mut_repo().view().heads(),
         hashset! { repo.view().checkout().clone(), new_commit_e.id().clone() }
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -262,8 +254,6 @@ fn test_rebase_descendants_external_merge(use_git: bool) {
         *tx.mut_repo().view().heads(),
         hashset! {repo.view().checkout().clone(), new_commit_e.id().clone()}
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -311,8 +301,6 @@ fn test_rebase_descendants_abandon(use_git: bool) {
             new_commit_f.id().clone()
         }
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -349,8 +337,6 @@ fn test_rebase_descendants_abandon_no_descendants(use_git: bool) {
             commit_a.id().clone(),
         }
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -390,8 +376,6 @@ fn test_rebase_descendants_abandon_and_replace(use_git: bool) {
         *tx.mut_repo().view().heads(),
         hashset! {repo.view().checkout().clone(), new_commit_d.id().clone()}
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -430,8 +414,6 @@ fn test_rebase_descendants_abandon_degenerate_merge(use_git: bool) {
         *tx.mut_repo().view().heads(),
         hashset! {repo.view().checkout().clone(), new_commit_d.id().clone()}
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -478,8 +460,6 @@ fn test_rebase_descendants_abandon_widen_merge(use_git: bool) {
         *tx.mut_repo().view().heads(),
         hashset! {repo.view().checkout().clone(), new_commit_f.id().clone()}
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -528,8 +508,6 @@ fn test_rebase_descendants_multiple_sideways(use_git: bool) {
             new_commit_e.id().clone()
         }
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -576,8 +554,6 @@ fn test_rebase_descendants_multiple_swap(use_git: bool) {
             new_commit_e.id().clone()
         }
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -618,8 +594,6 @@ fn test_rebase_descendants_multiple_no_descendants(use_git: bool) {
             commit_c.id().clone()
         }
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -677,8 +651,6 @@ fn test_rebase_descendants_multiple_forward_and_backward(use_git: bool) {
             new_commit_h.id().clone()
         }
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -748,8 +720,6 @@ fn test_rebase_descendants_divergent_rewrite(use_git: bool) {
             new_commit_g.id().clone(),
         }
     );
-
-    tx.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -816,8 +786,6 @@ fn test_rebase_descendants_contents(use_git: bool) {
         new_commit_c.tree().path_value(&path2),
         commit_b.tree().path_value(&path2)
     );
-
-    tx.discard();
 }
 
 #[test]
@@ -855,8 +823,6 @@ fn test_rebase_descendants_basic_branch_update() {
         *tx.mut_repo().view().heads(),
         hashset! {repo.view().checkout().clone(), commit_b2.id().clone()}
     );
-
-    tx.discard();
 }
 
 #[test]
@@ -914,8 +880,6 @@ fn test_rebase_descendants_basic_branch_update_with_non_local_branch() {
         *tx.mut_repo().view().heads(),
         hashset! {repo.view().checkout().clone(), commit_b2.id().clone()}
     );
-
-    tx.discard();
 }
 
 #[test]
@@ -952,8 +916,6 @@ fn test_rebase_descendants_update_branch_after_abandon() {
         *tx.mut_repo().view().heads(),
         hashset! {repo.view().checkout().clone(), commit_a.id().clone()}
     );
-
-    tx.discard();
 }
 
 #[test]
@@ -1013,8 +975,6 @@ fn test_rebase_descendants_update_branches_after_divergent_rewrite() {
             commit_b4.id().clone(),
         }
     );
-
-    tx.discard();
 }
 
 #[test]
@@ -1084,8 +1044,6 @@ fn test_rebase_descendants_rewrite_updates_branch_conflict() {
             commit_c.id().clone(),
         }
     );
-
-    tx.discard();
 }
 
 #[test]
@@ -1131,8 +1089,6 @@ fn test_rebase_descendants_rewrite_resolves_branch_conflict() {
         *tx.mut_repo().view().heads(),
         hashset! {repo.view().checkout().clone(), commit_b2.id().clone()}
     );
-
-    tx.discard();
 }
 
 // TODO: Add a test for the following case, which can't happen with our current

src/commands.rs

@@ -451,7 +451,6 @@ impl WorkspaceCommandHelper {
     fn finish_transaction(&mut self, ui: &mut Ui, mut tx: Transaction) -> Result<(), CommandError> {
         let mut_repo = tx.mut_repo();
         if !mut_repo.has_changes() {
-            tx.discard();
             writeln!(ui, "Nothing changed.")?;
             return Ok(());
         }
@@ -1406,8 +1405,6 @@ fn cmd_init(ui: &mut Ui, command: &CommandHelper, args: &ArgMatches) -> Result<(
         // number.
         if tx.mut_repo().has_changes() {
             workspace_command.finish_transaction(ui, tx)?;
-        } else {
-            tx.discard();
         }
     } else if args.is_present("git") {
         Workspace::init_internal_git(ui.settings(), wc_path.clone())?;