cleanup: commit transactions in tests when it's simpler

There were some tests that discarded a transaction only because it
used to be easier to do that than to commit and reload the repo. We
get the new repo back when we commit the transaction these days, so
now it's often easier to commit the transaction instead.
This commit is contained in:
Martin von Zweigbergk 2021-07-30 14:35:08 -07:00
parent 6b1ccd4512
commit 38032b0132
6 changed files with 74 additions and 98 deletions

View file

@ -12,7 +12,6 @@
// See the License for the specific language governing permissions and
// limitations under the License.
use std::collections::HashSet;
use std::fmt::{Debug, Formatter};
use std::fs;
use std::fs::File;
@ -648,10 +647,6 @@ impl MutableRepo {
open_commit
}
pub fn heads(&self) -> &HashSet<CommitId> {
self.view.heads()
}
fn enforce_view_invariants(&mut self) {
let view = self.view.store_view_mut();
// TODO: This is surely terribly slow on large repos, at least in its current

View file

@ -68,10 +68,10 @@ fn test_import_refs() {
let git_repo = repo.store().git_repo().unwrap();
let mut tx = repo.start_transaction("test");
let mut_repo = tx.mut_repo();
let heads_before: HashSet<_> = repo.view().heads().clone();
jujutsu_lib::git::import_refs(mut_repo, &git_repo).unwrap_or_default();
let view = mut_repo.view();
jujutsu_lib::git::import_refs(tx.mut_repo(), &git_repo).unwrap();
let repo = tx.commit();
let view = repo.view();
let expected_heads: HashSet<_> = heads_before
.union(&hashset!(
commit_id(&commit3),
@ -99,7 +99,6 @@ fn test_import_refs() {
view.git_refs().get("refs/remotes/origin/main"),
Some(RefTarget::Normal(commit_id(&commit5))).as_ref()
);
tx.discard();
}
#[test]
@ -119,7 +118,7 @@ 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();
jujutsu_lib::git::import_refs(tx.mut_repo(), &git_repo).unwrap();
let repo = tx.commit();
// Delete feature1 and rewrite feature2
@ -128,10 +127,10 @@ fn test_import_refs_reimport() {
let commit5 = empty_git_commit(&git_repo, "refs/heads/feature2", &[&commit2]);
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();
jujutsu_lib::git::import_refs(tx.mut_repo(), &git_repo).unwrap();
let repo = tx.commit();
let view = mut_repo.view();
let view = repo.view();
// TODO: commit3 and commit4 should probably be removed
let expected_heads: HashSet<_> = heads_before
.union(&hashset!(
@ -151,7 +150,6 @@ fn test_import_refs_reimport() {
view.git_refs().get("refs/heads/feature2"),
Some(RefTarget::Normal(commit_id(&commit5))).as_ref()
);
tx.discard();
}
fn git_ref(git_repo: &git2::Repository, name: &str, target: Oid) {
@ -220,11 +218,10 @@ fn test_import_refs_empty_git_repo() {
let repo = ReadonlyRepo::init_external_git(&settings, jj_repo_dir, git_repo_dir).unwrap();
let heads_before = repo.view().heads().clone();
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();
assert_eq!(*mut_repo.view().heads(), heads_before);
assert_eq!(mut_repo.view().git_refs().len(), 0);
tx.discard();
jujutsu_lib::git::import_refs(tx.mut_repo(), &git_repo).unwrap();
let repo = tx.commit();
assert_eq!(*repo.view().heads(), heads_before);
assert_eq!(repo.view().git_refs().len(), 0);
}
#[test]
@ -266,11 +263,9 @@ fn test_fetch_success() {
// The new commit is visible after git::fetch().
let mut tx = jj_repo.start_transaction("test");
let mut_repo = tx.mut_repo();
git::fetch(mut_repo, &clone_git_repo, "origin").unwrap();
assert!(mut_repo.heads().contains(&commit_id(&new_git_commit)));
tx.discard();
git::fetch(tx.mut_repo(), &clone_git_repo, "origin").unwrap();
let repo = tx.commit();
assert!(repo.view().heads().contains(&commit_id(&new_git_commit)));
}
#[test]
@ -286,7 +281,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();
}

View file

@ -27,7 +27,7 @@ fn test_init_local() {
assert_eq!(repo.working_copy_path(), &wc_path);
assert_eq!(repo.repo_path(), &wc_path.join(".jj"));
// Just test that we write a commit to the store
// 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();
@ -43,7 +43,7 @@ fn test_init_internal_git() {
assert_eq!(repo.working_copy_path(), &wc_path);
assert_eq!(repo.repo_path(), &wc_path.join(".jj"));
// Just test that we write a commit to the store
// 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();
@ -62,7 +62,7 @@ fn test_init_external_git() {
assert_eq!(repo.working_copy_path(), &wc_path);
assert_eq!(repo.repo_path(), &wc_path.join(".jj"));
// Just test that we write a commit to the store
// 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();

View file

@ -63,6 +63,7 @@ fn test_resolve_symbol_commit_id() {
.write_to_repo(mut_repo);
commits.push(commit);
}
let repo = tx.commit();
// Test the test setup
assert_eq!(
@ -79,7 +80,7 @@ fn test_resolve_symbol_commit_id() {
);
// Test lookup by full commit id
let repo_ref = mut_repo.as_repo_ref();
let repo_ref = repo.as_repo_ref();
assert_eq!(
resolve_symbol(repo_ref, "0454de3cae04c46cda37ba2e8873b4c17ff51dcb"),
Ok(vec![commits[0].id().clone()])
@ -116,8 +117,6 @@ fn test_resolve_symbol_commit_id() {
resolve_symbol(repo_ref, "foo"),
Err(RevsetError::NoSuchRevision("foo".to_string()))
);
tx.discard();
}
#[test]

View file

@ -25,8 +25,6 @@ use test_case::test_case;
fn test_graph_iterator_linearized(skip_transitive_edges: bool) {
let settings = testutils::user_settings();
let (_temp_dir, repo) = testutils::init_repo(&settings, true);
let mut tx = repo.start_transaction("test");
let mut_repo = tx.mut_repo();
// Tests that a fork and a merge becomes a single edge:
// D
@ -36,31 +34,31 @@ fn test_graph_iterator_linearized(skip_transitive_edges: bool) {
// A ~
// |
// root
let mut graph_builder = CommitGraphBuilder::new(&settings, mut_repo);
let mut tx = repo.start_transaction("test");
let mut graph_builder = CommitGraphBuilder::new(&settings, tx.mut_repo());
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_a]);
let commit_d = graph_builder.commit_with_parents(&[&commit_b, &commit_c]);
let pos_root = mut_repo
let repo = tx.commit();
let pos_root = repo
.index()
.commit_id_to_pos(repo.store().root_commit_id())
.unwrap();
let pos_a = mut_repo.index().commit_id_to_pos(commit_a.id()).unwrap();
let pos_a = repo.index().commit_id_to_pos(commit_a.id()).unwrap();
let revset = revset_for_commits(mut_repo.as_repo_ref(), &[&commit_a, &commit_d]);
let revset = revset_for_commits(repo.as_repo_ref(), &[&commit_a, &commit_d]);
let commits = revset
.iter()
.graph()
.set_skip_transitive_edges(skip_transitive_edges)
.collect_vec();
drop(revset);
assert_eq!(commits.len(), 2);
assert_eq!(commits[0].0.commit_id(), *commit_d.id());
assert_eq!(commits[1].0.commit_id(), *commit_a.id());
assert_eq!(commits[0].1, hashset![RevsetGraphEdge::indirect(pos_a)]);
assert_eq!(commits[1].1, hashset![RevsetGraphEdge::missing(pos_root)]);
tx.discard();
}
#[test_case(false ; "keep transitive edges")]
@ -68,8 +66,6 @@ fn test_graph_iterator_linearized(skip_transitive_edges: bool) {
fn test_graph_iterator_virtual_octopus(skip_transitive_edges: bool) {
let settings = testutils::user_settings();
let (_temp_dir, repo) = testutils::init_repo(&settings, true);
let mut tx = repo.start_transaction("test");
let mut_repo = tx.mut_repo();
// Tests that merges outside the set can result in more parent edges than there
// was in the input: F
@ -79,23 +75,26 @@ fn test_graph_iterator_virtual_octopus(skip_transitive_edges: bool) {
// A B C A B C
// \|/ ~ ~ ~
// root
let mut graph_builder = CommitGraphBuilder::new(&settings, mut_repo);
let mut tx = repo.start_transaction("test");
let mut graph_builder = CommitGraphBuilder::new(&settings, tx.mut_repo());
let commit_a = graph_builder.initial_commit();
let commit_b = graph_builder.initial_commit();
let commit_c = graph_builder.initial_commit();
let commit_d = graph_builder.commit_with_parents(&[&commit_a, &commit_b]);
let commit_e = graph_builder.commit_with_parents(&[&commit_b, &commit_c]);
let commit_f = graph_builder.commit_with_parents(&[&commit_d, &commit_e]);
let pos_root = mut_repo
let repo = tx.commit();
let pos_root = repo
.index()
.commit_id_to_pos(repo.store().root_commit_id())
.unwrap();
let pos_a = mut_repo.index().commit_id_to_pos(commit_a.id()).unwrap();
let pos_b = mut_repo.index().commit_id_to_pos(commit_b.id()).unwrap();
let pos_c = mut_repo.index().commit_id_to_pos(commit_c.id()).unwrap();
let pos_a = repo.index().commit_id_to_pos(commit_a.id()).unwrap();
let pos_b = repo.index().commit_id_to_pos(commit_b.id()).unwrap();
let pos_c = repo.index().commit_id_to_pos(commit_c.id()).unwrap();
let revset = revset_for_commits(
mut_repo.as_repo_ref(),
repo.as_repo_ref(),
&[&commit_a, &commit_b, &commit_c, &commit_f],
);
let commits = revset
@ -103,7 +102,6 @@ fn test_graph_iterator_virtual_octopus(skip_transitive_edges: bool) {
.graph()
.set_skip_transitive_edges(skip_transitive_edges)
.collect_vec();
drop(revset);
assert_eq!(commits.len(), 4);
assert_eq!(commits[0].0.commit_id(), *commit_f.id());
assert_eq!(commits[1].0.commit_id(), *commit_c.id());
@ -120,8 +118,6 @@ fn test_graph_iterator_virtual_octopus(skip_transitive_edges: bool) {
assert_eq!(commits[1].1, hashset![RevsetGraphEdge::missing(pos_root)]);
assert_eq!(commits[2].1, hashset![RevsetGraphEdge::missing(pos_root)]);
assert_eq!(commits[3].1, hashset![RevsetGraphEdge::missing(pos_root)]);
tx.discard();
}
#[test_case(false ; "keep transitive edges")]
@ -129,8 +125,6 @@ fn test_graph_iterator_virtual_octopus(skip_transitive_edges: bool) {
fn test_graph_iterator_simple_fork(skip_transitive_edges: bool) {
let settings = testutils::user_settings();
let (_temp_dir, repo) = testutils::init_repo(&settings, true);
let mut tx = repo.start_transaction("test");
let mut_repo = tx.mut_repo();
// Tests that the branch with "C" gets emitted correctly:
// E
@ -143,25 +137,27 @@ fn test_graph_iterator_simple_fork(skip_transitive_edges: bool) {
// A
// |
// root
let mut graph_builder = CommitGraphBuilder::new(&settings, mut_repo);
let mut tx = repo.start_transaction("test");
let mut graph_builder = CommitGraphBuilder::new(&settings, tx.mut_repo());
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]);
let commit_d = graph_builder.commit_with_parents(&[&commit_b]);
let commit_e = graph_builder.commit_with_parents(&[&commit_d]);
let pos_root = mut_repo
let repo = tx.commit();
let pos_root = repo
.index()
.commit_id_to_pos(repo.store().root_commit_id())
.unwrap();
let pos_a = mut_repo.index().commit_id_to_pos(commit_a.id()).unwrap();
let pos_a = repo.index().commit_id_to_pos(commit_a.id()).unwrap();
let revset = revset_for_commits(mut_repo.as_repo_ref(), &[&commit_a, &commit_c, &commit_e]);
let revset = revset_for_commits(repo.as_repo_ref(), &[&commit_a, &commit_c, &commit_e]);
let commits = revset
.iter()
.graph()
.set_skip_transitive_edges(skip_transitive_edges)
.collect_vec();
drop(revset);
assert_eq!(commits.len(), 3);
assert_eq!(commits[0].0.commit_id(), *commit_e.id());
assert_eq!(commits[1].0.commit_id(), *commit_c.id());
@ -169,8 +165,6 @@ fn test_graph_iterator_simple_fork(skip_transitive_edges: bool) {
assert_eq!(commits[0].1, hashset![RevsetGraphEdge::indirect(pos_a)]);
assert_eq!(commits[1].1, hashset![RevsetGraphEdge::indirect(pos_a)]);
assert_eq!(commits[2].1, hashset![RevsetGraphEdge::missing(pos_root)]);
tx.discard();
}
#[test_case(false ; "keep transitive edges")]
@ -178,8 +172,6 @@ fn test_graph_iterator_simple_fork(skip_transitive_edges: bool) {
fn test_graph_iterator_multiple_missing(skip_transitive_edges: bool) {
let settings = testutils::user_settings();
let (_temp_dir, repo) = testutils::init_repo(&settings, true);
let mut tx = repo.start_transaction("test");
let mut_repo = tx.mut_repo();
// Tests that we get missing edges to "a" and "c" and not just one missing edge
// to the root.
@ -190,28 +182,30 @@ fn test_graph_iterator_multiple_missing(skip_transitive_edges: bool) {
// a B c ~
// \|/
// root
let mut graph_builder = CommitGraphBuilder::new(&settings, mut_repo);
let mut tx = repo.start_transaction("test");
let mut graph_builder = CommitGraphBuilder::new(&settings, tx.mut_repo());
let commit_a = graph_builder.initial_commit();
let commit_b = graph_builder.initial_commit();
let commit_c = graph_builder.initial_commit();
let commit_d = graph_builder.commit_with_parents(&[&commit_a, &commit_b]);
let commit_e = graph_builder.commit_with_parents(&[&commit_b, &commit_c]);
let commit_f = graph_builder.commit_with_parents(&[&commit_d, &commit_e]);
let pos_root = mut_repo
let repo = tx.commit();
let pos_root = repo
.index()
.commit_id_to_pos(repo.store().root_commit_id())
.unwrap();
let pos_a = mut_repo.index().commit_id_to_pos(commit_a.id()).unwrap();
let pos_b = mut_repo.index().commit_id_to_pos(commit_b.id()).unwrap();
let pos_c = mut_repo.index().commit_id_to_pos(commit_c.id()).unwrap();
let pos_a = repo.index().commit_id_to_pos(commit_a.id()).unwrap();
let pos_b = repo.index().commit_id_to_pos(commit_b.id()).unwrap();
let pos_c = repo.index().commit_id_to_pos(commit_c.id()).unwrap();
let revset = revset_for_commits(mut_repo.as_repo_ref(), &[&commit_b, &commit_f]);
let revset = revset_for_commits(repo.as_repo_ref(), &[&commit_b, &commit_f]);
let commits = revset
.iter()
.graph()
.set_skip_transitive_edges(skip_transitive_edges)
.collect_vec();
drop(revset);
assert_eq!(commits.len(), 2);
assert_eq!(commits[0].0.commit_id(), *commit_f.id());
assert_eq!(commits[1].0.commit_id(), *commit_b.id());
@ -224,8 +218,6 @@ fn test_graph_iterator_multiple_missing(skip_transitive_edges: bool) {
]
);
assert_eq!(commits[1].1, hashset![RevsetGraphEdge::missing(pos_root)]);
tx.discard();
}
#[test_case(false ; "keep transitive edges")]
@ -233,8 +225,6 @@ fn test_graph_iterator_multiple_missing(skip_transitive_edges: bool) {
fn test_graph_iterator_edge_to_ancestor(skip_transitive_edges: bool) {
let settings = testutils::user_settings();
let (_temp_dir, repo) = testutils::init_repo(&settings, true);
let mut tx = repo.start_transaction("test");
let mut_repo = tx.mut_repo();
// Tests that we get both an edge from F to D and to D's ancestor C if we keep
// transitive edges and only the edge from F to D if we skip transitive
@ -248,25 +238,27 @@ fn test_graph_iterator_edge_to_ancestor(skip_transitive_edges: bool) {
// a
// |
// root
let mut graph_builder = CommitGraphBuilder::new(&settings, mut_repo);
let mut tx = repo.start_transaction("test");
let mut graph_builder = CommitGraphBuilder::new(&settings, tx.mut_repo());
let commit_a = graph_builder.initial_commit();
let commit_b = graph_builder.initial_commit();
let commit_c = graph_builder.commit_with_parents(&[&commit_a]);
let commit_d = graph_builder.commit_with_parents(&[&commit_b, &commit_c]);
let commit_e = graph_builder.commit_with_parents(&[&commit_c]);
let commit_f = graph_builder.commit_with_parents(&[&commit_d, &commit_e]);
let pos_a = mut_repo.index().commit_id_to_pos(commit_a.id()).unwrap();
let pos_b = mut_repo.index().commit_id_to_pos(commit_b.id()).unwrap();
let pos_c = mut_repo.index().commit_id_to_pos(commit_c.id()).unwrap();
let pos_d = mut_repo.index().commit_id_to_pos(commit_d.id()).unwrap();
let repo = tx.commit();
let revset = revset_for_commits(mut_repo.as_repo_ref(), &[&commit_c, &commit_d, &commit_f]);
let pos_a = repo.index().commit_id_to_pos(commit_a.id()).unwrap();
let pos_b = repo.index().commit_id_to_pos(commit_b.id()).unwrap();
let pos_c = repo.index().commit_id_to_pos(commit_c.id()).unwrap();
let pos_d = repo.index().commit_id_to_pos(commit_d.id()).unwrap();
let revset = revset_for_commits(repo.as_repo_ref(), &[&commit_c, &commit_d, &commit_f]);
let commits = revset
.iter()
.graph()
.set_skip_transitive_edges(skip_transitive_edges)
.collect_vec();
drop(revset);
assert_eq!(commits.len(), 3);
assert_eq!(commits[0].0.commit_id(), *commit_f.id());
assert_eq!(commits[1].0.commit_id(), *commit_d.id());
@ -290,8 +282,6 @@ fn test_graph_iterator_edge_to_ancestor(skip_transitive_edges: bool) {
]
);
assert_eq!(commits[2].1, hashset![RevsetGraphEdge::missing(pos_a)]);
tx.discard();
}
#[test_case(false ; "keep transitive edges")]
@ -299,8 +289,6 @@ fn test_graph_iterator_edge_to_ancestor(skip_transitive_edges: bool) {
fn test_graph_iterator_edge_escapes_from_(skip_transitive_edges: bool) {
let settings = testutils::user_settings();
let (_temp_dir, repo) = testutils::init_repo(&settings, true);
let mut tx = repo.start_transaction("test");
let mut_repo = tx.mut_repo();
// Tests a more complex case for skipping transitive edges.
// J
@ -318,7 +306,8 @@ fn test_graph_iterator_edge_escapes_from_(skip_transitive_edges: bool) {
// A
// |
// root
let mut graph_builder = CommitGraphBuilder::new(&settings, mut_repo);
let mut tx = repo.start_transaction("test");
let mut graph_builder = CommitGraphBuilder::new(&settings, tx.mut_repo());
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_a]);
@ -329,17 +318,19 @@ fn test_graph_iterator_edge_escapes_from_(skip_transitive_edges: bool) {
let commit_h = graph_builder.commit_with_parents(&[&commit_f]);
let commit_i = graph_builder.commit_with_parents(&[&commit_e, &commit_h]);
let commit_j = graph_builder.commit_with_parents(&[&commit_g, &commit_i]);
let pos_root = mut_repo
let repo = tx.commit();
let pos_root = repo
.index()
.commit_id_to_pos(repo.store().root_commit_id())
.unwrap();
let pos_a = mut_repo.index().commit_id_to_pos(commit_a.id()).unwrap();
let pos_d = mut_repo.index().commit_id_to_pos(commit_d.id()).unwrap();
let pos_g = mut_repo.index().commit_id_to_pos(commit_g.id()).unwrap();
let pos_h = mut_repo.index().commit_id_to_pos(commit_h.id()).unwrap();
let pos_a = repo.index().commit_id_to_pos(commit_a.id()).unwrap();
let pos_d = repo.index().commit_id_to_pos(commit_d.id()).unwrap();
let pos_g = repo.index().commit_id_to_pos(commit_g.id()).unwrap();
let pos_h = repo.index().commit_id_to_pos(commit_h.id()).unwrap();
let revset = revset_for_commits(
mut_repo.as_repo_ref(),
repo.as_repo_ref(),
&[&commit_a, &commit_d, &commit_g, &commit_h, &commit_j],
);
let commits = revset
@ -347,7 +338,6 @@ fn test_graph_iterator_edge_escapes_from_(skip_transitive_edges: bool) {
.graph()
.set_skip_transitive_edges(skip_transitive_edges)
.collect_vec();
drop(revset);
assert_eq!(commits.len(), 5);
assert_eq!(commits[0].0.commit_id(), *commit_j.id());
assert_eq!(commits[1].0.commit_id(), *commit_h.id());
@ -383,6 +373,4 @@ fn test_graph_iterator_edge_escapes_from_(skip_transitive_edges: bool) {
assert_eq!(commits[2].1, hashset![RevsetGraphEdge::indirect(pos_a)]);
assert_eq!(commits[3].1, hashset![RevsetGraphEdge::indirect(pos_a)]);
assert_eq!(commits[4].1, hashset![RevsetGraphEdge::missing(pos_root)]);
tx.discard();
}

View file

@ -42,17 +42,17 @@ fn test_heads_fork(use_git: bool) {
let initial = graph_builder.initial_commit();
let child1 = graph_builder.commit_with_parents(&[&initial]);
let child2 = graph_builder.commit_with_parents(&[&initial]);
let repo = tx.commit();
let wc = repo.working_copy_locked();
assert_eq!(
*tx.mut_repo().view().heads(),
*repo.view().heads(),
hashset! {
wc.current_commit_id(),
child1.id().clone(),
child2.id().clone(),
}
);
tx.discard();
}
#[test_case(false ; "local store")]
@ -67,11 +67,11 @@ fn test_heads_merge(use_git: bool) {
let child1 = graph_builder.commit_with_parents(&[&initial]);
let child2 = graph_builder.commit_with_parents(&[&initial]);
let merge = graph_builder.commit_with_parents(&[&child1, &child2]);
let repo = tx.commit();
let wc = repo.working_copy_locked();
assert_eq!(
*tx.mut_repo().view().heads(),
*repo.view().heads(),
hashset! {wc.current_commit_id(), merge.id().clone()}
);
tx.discard();
}