Skip to content

Commit

Permalink
tests: compare git refs loaded from disk, not in-memory cache values
Browse files Browse the repository at this point in the history
This addresses the test instability. The underlying problem still exists, but
it's unlikely to trigger user-facing issues because of that. A repo instance
won't be reused after gc() call.

Fixes #3537
  • Loading branch information
yuja committed Apr 22, 2024
1 parent 527713a commit 9f4a731
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 13 deletions.
6 changes: 6 additions & 0 deletions lib/src/git_backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1223,6 +1223,12 @@ impl Backend for GitBackend {
// TODO: remove unreachable extras table segments
// TODO: pass in keep_newer to "git gc" command
run_git_gc(self.git_repo_path()).map_err(|err| BackendError::Other(err.into()))
// TODO: Since "git gc" will move loose refs into packed refs, in-memory
// packed-refs cache should be invalidated here. If mtime accuracy is
// high or "git gc" is slow enough, gix::RefStore can notice the change,
// but it's not always the case. Upgrade gix to 0.63.0 and call
// force_refresh_packed_buffer().
// https://github.com/Byron/gitoxide/issues/1348
}
}

Expand Down
27 changes: 14 additions & 13 deletions lib/tests/test_git_backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
// limitations under the License.

use std::collections::HashSet;
use std::path::Path;
use std::process::Command;
use std::sync::Arc;
use std::time::{Duration, SystemTime};
Expand All @@ -30,11 +31,11 @@ fn get_git_backend(repo: &Arc<ReadonlyRepo>) -> &GitBackend {
.unwrap()
}

fn get_git_repo(repo: &Arc<ReadonlyRepo>) -> gix::Repository {
get_git_backend(repo).git_repo()
}

fn collect_no_gc_refs(git_repo: &gix::Repository) -> HashSet<CommitId> {
fn collect_no_gc_refs(git_repo_path: &Path) -> HashSet<CommitId> {
// Load fresh git repo to isolate from false caching issue. Here we want to
// ensure that the underlying data is correct. We could test the in-memory
// data as well, but we don't have any special handling in our code.
let git_repo = gix::open(git_repo_path).unwrap();
let git_refs = git_repo.references().unwrap();
let no_gc_refs_iter = git_refs.prefixed("refs/jj/keep/").unwrap();
no_gc_refs_iter
Expand All @@ -53,7 +54,7 @@ fn test_gc() {
let settings = testutils::user_settings();
let test_repo = TestRepo::init_with_backend(TestRepoBackend::Git);
let repo = test_repo.repo;
let git_repo = get_git_repo(&repo);
let git_repo_path = get_git_backend(&repo).git_repo_path();
let base_index = repo.readonly_index();

// Set up commits:
Expand Down Expand Up @@ -94,7 +95,7 @@ fn test_gc() {

// At first, all commits have no-gc refs
assert_eq!(
collect_no_gc_refs(&git_repo),
collect_no_gc_refs(git_repo_path),
hashset! {
commit_a.id().clone(),
commit_b.id().clone(),
Expand All @@ -113,7 +114,7 @@ fn test_gc() {
.gc(base_index.as_index(), SystemTime::UNIX_EPOCH)
.unwrap();
assert_eq!(
collect_no_gc_refs(&git_repo),
collect_no_gc_refs(git_repo_path),
hashset! {
commit_a.id().clone(),
commit_b.id().clone(),
Expand All @@ -133,7 +134,7 @@ fn test_gc() {
// All reachable: redundant no-gc refs will be removed
repo.store().gc(repo.index(), now()).unwrap();
assert_eq!(
collect_no_gc_refs(&git_repo),
collect_no_gc_refs(git_repo_path),
hashset! {
commit_d.id().clone(),
commit_g.id().clone(),
Expand All @@ -152,7 +153,7 @@ fn test_gc() {
mut_index.add_commit(&commit_h);
repo.store().gc(mut_index.as_index(), now()).unwrap();
assert_eq!(
collect_no_gc_refs(&git_repo),
collect_no_gc_refs(git_repo_path),
hashset! {
commit_d.id().clone(),
commit_e.id().clone(),
Expand All @@ -168,7 +169,7 @@ fn test_gc() {
mut_index.add_commit(&commit_f);
repo.store().gc(mut_index.as_index(), now()).unwrap();
assert_eq!(
collect_no_gc_refs(&git_repo),
collect_no_gc_refs(git_repo_path),
hashset! {
commit_c.id().clone(),
commit_f.id().clone(),
Expand All @@ -180,13 +181,13 @@ fn test_gc() {
mut_index.add_commit(&commit_a);
repo.store().gc(mut_index.as_index(), now()).unwrap();
assert_eq!(
collect_no_gc_refs(&git_repo),
collect_no_gc_refs(git_repo_path),
hashset! {
commit_a.id().clone(),
},
);

// All unreachable
repo.store().gc(base_index.as_index(), now()).unwrap();
assert_eq!(collect_no_gc_refs(&git_repo), hashset! {});
assert_eq!(collect_no_gc_refs(git_repo_path), hashset! {});
}

0 comments on commit 9f4a731

Please sign in to comment.