From 6809a88d42900ab971a6932932e228bbbb3cb40d Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Wed, 19 May 2021 13:39:03 -0700 Subject: [PATCH] cleanup: use ReadonlyRepo returned from Transaction::commit() I thought I had looked for this case and cleaned up all the places when I made `Transaction::commit()` return a new `ReadonlyRepo`. I must have forgotten to do that, because there we tons of places to clean up left. --- lib/tests/test_git.rs | 12 +++--- lib/tests/test_index.rs | 21 ++++------ lib/tests/test_load_repo.rs | 7 ++-- lib/tests/test_mut_repo.rs | 77 ++++++++++++------------------------- src/commands.rs | 13 +++---- 5 files changed, 47 insertions(+), 83 deletions(-) diff --git a/lib/tests/test_git.rs b/lib/tests/test_git.rs index 6a7599228..a64fe784a 100644 --- a/lib/tests/test_git.rs +++ b/lib/tests/test_git.rs @@ -104,7 +104,7 @@ fn test_import_refs() { #[test] fn test_import_refs_reimport() { let settings = testutils::user_settings(); - let (_temp_dir, mut repo) = testutils::init_repo(&settings, true); + let (_temp_dir, repo) = testutils::init_repo(&settings, true); let git_repo = repo.store().git_repo().unwrap(); let commit1 = empty_git_commit(&git_repo, "refs/heads/main", &[]); @@ -119,14 +119,13 @@ fn test_import_refs_reimport() { let heads_before = repo.view().heads().clone(); let mut tx = repo.start_transaction("test"); jujutsu_lib::git::import_refs(tx.mut_repo(), &git_repo).unwrap_or_default(); - tx.commit(); + let repo = tx.commit(); // Delete feature1 and rewrite feature2 delete_git_ref(&git_repo, "refs/heads/feature1"); delete_git_ref(&git_repo, "refs/heads/feature2"); let commit5 = empty_git_commit(&git_repo, "refs/heads/feature2", &[&commit2]); - repo = repo.reload().unwrap(); let mut tx = repo.start_transaction("test"); let mut_repo = tx.mut_repo(); jujutsu_lib::git::import_refs(mut_repo, &git_repo).unwrap_or_default(); @@ -165,7 +164,7 @@ fn delete_git_ref(git_repo: &git2::Repository, name: &str) { #[test] fn test_import_refs_merge() { let settings = testutils::user_settings(); - let (_temp_dir, mut repo) = testutils::init_repo(&settings, true); + let (_temp_dir, repo) = testutils::init_repo(&settings, true); let git_repo = repo.store().git_repo().unwrap(); // Set up the following refs and update them as follows: @@ -202,8 +201,7 @@ fn test_import_refs_merge() { git_ref(&git_repo, "refs/heads/remove-forward", commit1.id()); let mut tx = repo.start_transaction("initial import"); jujutsu_lib::git::import_refs(tx.mut_repo(), &git_repo).unwrap_or_default(); - tx.commit(); - repo = repo.reload().unwrap(); + let repo = tx.commit(); // One of the concurrent operations: git_ref(&git_repo, "refs/heads/sideways-unchanged", commit4.id()); @@ -230,7 +228,7 @@ fn test_import_refs_merge() { tx2.commit(); // Reload the repo, causing the operations to be merged. - repo = repo.reload().unwrap(); + let repo = repo.reload().unwrap(); let view = repo.view(); let git_refs = view.git_refs(); diff --git a/lib/tests/test_index.rs b/lib/tests/test_index.rs index cabcc3578..1d2c9e87b 100644 --- a/lib/tests/test_index.rs +++ b/lib/tests/test_index.rs @@ -66,7 +66,7 @@ fn test_index_commits_empty_repo(use_git: bool) { #[test_case(true ; "git store")] fn test_index_commits_standard_cases(use_git: bool) { let settings = testutils::user_settings(); - let (_temp_dir, mut repo) = testutils::init_repo(&settings, use_git); + let (_temp_dir, repo) = testutils::init_repo(&settings, use_git); // o H // o | G @@ -94,8 +94,7 @@ fn test_index_commits_standard_cases(use_git: bool) { let commit_f = graph_builder.commit_with_parents(&[&commit_b, &commit_e]); let commit_g = graph_builder.commit_with_parents(&[&commit_f]); let commit_h = graph_builder.commit_with_parents(&[&commit_e]); - tx.commit(); - repo = repo.reload().unwrap(); + let repo = tx.commit(); let index = repo.index(); // There should be the root commit and the working copy commit, plus @@ -137,7 +136,7 @@ fn test_index_commits_standard_cases(use_git: bool) { #[test_case(true ; "git store")] fn test_index_commits_criss_cross(use_git: bool) { let settings = testutils::user_settings(); - let (_temp_dir, mut repo) = testutils::init_repo(&settings, use_git); + let (_temp_dir, repo) = testutils::init_repo(&settings, use_git); let num_generations = 50; @@ -156,8 +155,7 @@ fn test_index_commits_criss_cross(use_git: bool) { left_commits.push(new_left); right_commits.push(new_right); } - tx.commit(); - repo = repo.reload().unwrap(); + let repo = tx.commit(); let index = repo.index(); // There should the root commit and the working copy commit, plus 2 for each @@ -238,7 +236,7 @@ fn test_index_commits_criss_cross(use_git: bool) { fn test_index_commits_previous_operations(use_git: bool) { // Test that commits visible only in previous operations are indexed. let settings = testutils::user_settings(); - let (_temp_dir, mut repo) = testutils::init_repo(&settings, use_git); + let (_temp_dir, repo) = testutils::init_repo(&settings, use_git); // Remove commit B and C in one operation and make sure they're still // visible in the index after that operation. @@ -254,13 +252,11 @@ fn test_index_commits_previous_operations(use_git: bool) { let commit_a = graph_builder.initial_commit(); let commit_b = graph_builder.commit_with_parents(&[&commit_a]); let commit_c = graph_builder.commit_with_parents(&[&commit_b]); - tx.commit(); - repo = repo.reload().unwrap(); + let repo = tx.commit(); let mut tx = repo.start_transaction("test"); tx.mut_repo().remove_head(&commit_c); - tx.commit(); - repo = repo.reload().unwrap(); + let repo = tx.commit(); // Delete index from disk let index_operations_dir = repo @@ -418,8 +414,7 @@ fn create_n_commits( for _ in 0..num_commits { create_random_commit(settings, repo).write_to_repo(tx.mut_repo()); } - tx.commit(); - repo.reload().unwrap() + tx.commit() } fn commits_by_level(repo: &ReadonlyRepo) -> Vec { diff --git a/lib/tests/test_load_repo.rs b/lib/tests/test_load_repo.rs index 832b5837f..4f55f5752 100644 --- a/lib/tests/test_load_repo.rs +++ b/lib/tests/test_load_repo.rs @@ -45,12 +45,11 @@ fn test_load_from_subdir(use_git: bool) { #[test_case(true ; "git store")] fn test_load_at_operation(use_git: bool) { let settings = testutils::user_settings(); - let (_temp_dir, mut repo) = testutils::init_repo(&settings, use_git); + let (_temp_dir, repo) = testutils::init_repo(&settings, use_git); let mut tx = repo.start_transaction("add commit"); let commit = testutils::create_random_commit(&settings, &repo).write_to_repo(tx.mut_repo()); - let op = tx.commit().operation().clone(); - repo = repo.reload().unwrap(); + let repo = tx.commit(); let mut tx = repo.start_transaction("remove commit"); tx.mut_repo().remove_head(&commit); @@ -65,6 +64,6 @@ fn test_load_at_operation(use_git: bool) { // If we load the repo at the previous operation, we should see the commit since // it has not been removed yet let loader = RepoLoader::init(&settings, repo.working_copy_path().clone()).unwrap(); - let old_repo = loader.load_at(&op).unwrap(); + let old_repo = loader.load_at(&repo.operation()).unwrap(); assert!(old_repo.view().heads().contains(commit.id())); } diff --git a/lib/tests/test_mut_repo.rs b/lib/tests/test_mut_repo.rs index 7d264e882..6dcfa0633 100644 --- a/lib/tests/test_mut_repo.rs +++ b/lib/tests/test_mut_repo.rs @@ -36,14 +36,12 @@ fn test_checkout_open(use_git: bool) { let requested_checkout = testutils::create_random_commit(&settings, &repo) .set_open(true) .write_to_repo(tx.mut_repo()); - tx.commit(); - let repo = repo.reload().unwrap(); + let repo = tx.commit(); let mut tx = repo.start_transaction("test"); let actual_checkout = tx.mut_repo().check_out(&settings, &requested_checkout); assert_eq!(actual_checkout.id(), requested_checkout.id()); - tx.commit(); - let repo = repo.reload().unwrap(); + let repo = tx.commit(); assert_eq!(repo.view().checkout(), actual_checkout.id()); } @@ -59,16 +57,14 @@ fn test_checkout_closed(use_git: bool) { let requested_checkout = testutils::create_random_commit(&settings, &repo) .set_open(false) .write_to_repo(tx.mut_repo()); - tx.commit(); - let repo = repo.reload().unwrap(); + let repo = tx.commit(); let mut tx = repo.start_transaction("test"); let actual_checkout = tx.mut_repo().check_out(&settings, &requested_checkout); assert_eq!(actual_checkout.tree().id(), requested_checkout.tree().id()); assert_eq!(actual_checkout.parents().len(), 1); assert_eq!(actual_checkout.parents()[0].id(), requested_checkout.id()); - tx.commit(); - let repo = repo.reload().unwrap(); + let repo = tx.commit(); assert_eq!(repo.view().checkout(), actual_checkout.id()); } @@ -93,8 +89,7 @@ fn test_checkout_open_with_conflict(use_git: bool) { let requested_checkout = CommitBuilder::for_new_commit(&settings, store, tree_id) .set_open(true) .write_to_repo(tx.mut_repo()); - tx.commit(); - let repo = repo.reload().unwrap(); + let repo = tx.commit(); let mut tx = repo.start_transaction("test"); let actual_checkout = tx.mut_repo().check_out(&settings, &requested_checkout); @@ -108,8 +103,7 @@ fn test_checkout_open_with_conflict(use_git: bool) { } assert_eq!(actual_checkout.parents().len(), 1); assert_eq!(actual_checkout.parents()[0].id(), requested_checkout.id()); - tx.commit(); - let repo = repo.reload().unwrap(); + let repo = tx.commit(); assert_eq!(repo.view().checkout(), actual_checkout.id()); } @@ -134,8 +128,7 @@ fn test_checkout_closed_with_conflict(use_git: bool) { let requested_checkout = CommitBuilder::for_new_commit(&settings, store, tree_id) .set_open(false) .write_to_repo(tx.mut_repo()); - tx.commit(); - let repo = repo.reload().unwrap(); + let repo = tx.commit(); let mut tx = repo.start_transaction("test"); let actual_checkout = tx.mut_repo().check_out(&settings, &requested_checkout); @@ -149,8 +142,7 @@ fn test_checkout_closed_with_conflict(use_git: bool) { } assert_eq!(actual_checkout.parents().len(), 1); assert_eq!(actual_checkout.parents()[0].id(), requested_checkout.id()); - tx.commit(); - let repo = repo.reload().unwrap(); + let repo = tx.commit(); assert_eq!(repo.view().checkout(), actual_checkout.id()); } @@ -197,8 +189,7 @@ fn test_checkout_previous_not_empty(use_git: bool) { .set_open(true) .write_to_repo(mut_repo); mut_repo.check_out(&settings, &old_checkout); - tx.commit(); - let repo = repo.reload().unwrap(); + let repo = tx.commit(); let mut tx = repo.start_transaction("test"); let mut_repo = tx.mut_repo(); @@ -228,8 +219,7 @@ fn test_checkout_previous_empty(use_git: bool) { ) .write_to_repo(mut_repo); mut_repo.check_out(&settings, &old_checkout); - tx.commit(); - let repo = repo.reload().unwrap(); + let repo = tx.commit(); let mut tx = repo.start_transaction("test"); let mut_repo = tx.mut_repo(); @@ -261,8 +251,7 @@ fn test_checkout_previous_empty_and_obsolete(use_git: bool) { let successor = CommitBuilder::for_rewrite_from(&settings, repo.store(), &old_checkout) .write_to_repo(mut_repo); mut_repo.check_out(&settings, &old_checkout); - tx.commit(); - let repo = repo.reload().unwrap(); + let repo = tx.commit(); let mut tx = repo.start_transaction("test"); let mut_repo = tx.mut_repo(); @@ -291,8 +280,7 @@ fn test_checkout_previous_empty_and_pruned(use_git: bool) { .set_pruned(true) .write_to_repo(mut_repo); mut_repo.check_out(&settings, &old_checkout); - tx.commit(); - let repo = repo.reload().unwrap(); + let repo = tx.commit(); let mut tx = repo.start_transaction("test"); let mut_repo = tx.mut_repo(); @@ -332,8 +320,7 @@ fn test_add_head_success(use_git: bool) { mut_repo.add_head(&new_commit); assert!(mut_repo.view().heads().contains(new_commit.id())); assert!(mut_repo.index().has_id(new_commit.id())); - tx.commit(); - let repo = repo.reload().unwrap(); + let repo = tx.commit(); assert!(repo.view().heads().contains(new_commit.id())); assert!(repo.index().has_id(new_commit.id())); let index_stats = repo.index().stats(); @@ -355,8 +342,7 @@ fn test_add_head_ancestor(use_git: bool) { let commit1 = graph_builder.initial_commit(); let commit2 = graph_builder.commit_with_parents(&[&commit1]); let _commit3 = graph_builder.commit_with_parents(&[&commit2]); - tx.commit(); - let repo = repo.reload().unwrap(); + let repo = tx.commit(); let index_stats = repo.index().stats(); assert_eq!(index_stats.num_heads, 2); @@ -383,8 +369,7 @@ fn test_add_head_not_immediate_child(use_git: bool) { let mut tx = repo.start_transaction("test"); let initial = testutils::create_random_commit(&settings, &repo).write_to_repo(tx.mut_repo()); - tx.commit(); - let repo = repo.reload().unwrap(); + let repo = tx.commit(); // Create some commit outside of the repo by using a temporary transaction. Then // add one of them as a head. @@ -437,8 +422,7 @@ fn test_remove_head(use_git: bool) { let commit1 = graph_builder.initial_commit(); let commit2 = graph_builder.commit_with_parents(&[&commit1]); let commit3 = graph_builder.commit_with_parents(&[&commit2]); - tx.commit(); - let repo = repo.reload().unwrap(); + let repo = tx.commit(); let mut tx = repo.start_transaction("test"); let mut_repo = tx.mut_repo(); @@ -451,9 +435,7 @@ fn test_remove_head(use_git: bool) { assert!(mut_repo.index().has_id(commit1.id())); assert!(mut_repo.index().has_id(commit2.id())); assert!(mut_repo.index().has_id(commit3.id())); - tx.commit(); - - let repo = repo.reload().unwrap(); + let repo = tx.commit(); let heads = repo.view().heads().clone(); assert!(!heads.contains(commit3.id())); assert!(!heads.contains(commit2.id())); @@ -478,8 +460,7 @@ fn test_remove_head_ancestor_git_ref(use_git: bool) { let commit3 = graph_builder.commit_with_parents(&[&commit2]); tx.mut_repo() .insert_git_ref("refs/heads/main".to_string(), commit1.id().clone()); - tx.commit(); - let repo = repo.reload().unwrap(); + let repo = tx.commit(); let mut tx = repo.start_transaction("test"); let mut_repo = tx.mut_repo(); @@ -490,9 +471,7 @@ fn test_remove_head_ancestor_git_ref(use_git: bool) { assert!(!heads.contains(commit3.id())); assert!(!heads.contains(commit2.id())); assert!(heads.contains(commit1.id())); - tx.commit(); - - let repo = repo.reload().unwrap(); + let repo = tx.commit(); let heads = repo.view().heads().clone(); assert!(!heads.contains(commit3.id())); assert!(!heads.contains(commit2.id())); @@ -509,16 +488,14 @@ fn test_add_public_head(use_git: bool) { let mut tx = repo.start_transaction("test"); let commit1 = testutils::create_random_commit(&settings, &repo).write_to_repo(tx.mut_repo()); - tx.commit(); - let repo = repo.reload().unwrap(); + let repo = tx.commit(); let mut tx = repo.start_transaction("test"); let mut_repo = tx.mut_repo(); assert!(!mut_repo.view().public_heads().contains(commit1.id())); mut_repo.add_public_head(&commit1); assert!(mut_repo.view().public_heads().contains(commit1.id())); - tx.commit(); - let repo = repo.reload().unwrap(); + let repo = tx.commit(); assert!(repo.view().public_heads().contains(commit1.id())); } @@ -535,16 +512,14 @@ fn test_add_public_head_ancestor(use_git: bool) { let commit1 = graph_builder.initial_commit(); let commit2 = graph_builder.commit_with_parents(&[&commit1]); tx.mut_repo().add_public_head(&commit2); - tx.commit(); - let repo = repo.reload().unwrap(); + let repo = tx.commit(); let mut tx = repo.start_transaction("test"); let mut_repo = tx.mut_repo(); assert!(!mut_repo.view().public_heads().contains(commit1.id())); mut_repo.add_public_head(&commit1); assert!(!mut_repo.view().public_heads().contains(commit1.id())); - tx.commit(); - let repo = repo.reload().unwrap(); + let repo = tx.commit(); assert!(!repo.view().public_heads().contains(commit1.id())); } @@ -560,15 +535,13 @@ fn test_remove_public_head(use_git: bool) { let mut_repo = tx.mut_repo(); let commit1 = testutils::create_random_commit(&settings, &repo).write_to_repo(mut_repo); mut_repo.add_public_head(&commit1); - tx.commit(); - let repo = repo.reload().unwrap(); + let repo = tx.commit(); let mut tx = repo.start_transaction("test"); let mut_repo = tx.mut_repo(); assert!(mut_repo.view().public_heads().contains(commit1.id())); mut_repo.remove_public_head(&commit1); assert!(!mut_repo.view().public_heads().contains(commit1.id())); - tx.commit(); - let repo = repo.reload().unwrap(); + let repo = tx.commit(); assert!(!repo.view().public_heads().contains(commit1.id())); } diff --git a/src/commands.rs b/src/commands.rs index 99bc7c86d..d06e3147a 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -771,21 +771,20 @@ fn cmd_init( fs::create_dir(&wc_path).unwrap(); } - let repo; - if let Some(git_store_str) = sub_matches.value_of("git-store") { + let repo = if let Some(git_store_str) = sub_matches.value_of("git-store") { let git_store_path = ui.cwd().join(git_store_str); - repo = ReadonlyRepo::init_external_git(ui.settings(), wc_path, git_store_path); + let repo = ReadonlyRepo::init_external_git(ui.settings(), wc_path, git_store_path); let git_repo = repo.store().git_repo().unwrap(); let mut tx = repo.start_transaction("import git refs"); git::import_refs(tx.mut_repo(), &git_repo).unwrap(); // TODO: Check out a recent commit. Maybe one with the highest generation // number. - tx.commit(); + tx.commit() } else if sub_matches.is_present("git") { - repo = ReadonlyRepo::init_internal_git(ui.settings(), wc_path); + ReadonlyRepo::init_internal_git(ui.settings(), wc_path) } else { - repo = ReadonlyRepo::init_local(ui.settings(), wc_path); - } + ReadonlyRepo::init_local(ui.settings(), wc_path) + }; writeln!( ui, "Initialized repo in \"{}\"",