From b7eb551cf78bbd0d8295c12c927387ed40ba78eb Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Fri, 12 Jan 2024 00:06:47 +0900 Subject: [PATCH] index: fix reindexing to scan all referenced commits such as hidden remote refs Since hidden commits can be looked up by remote_branches() revset for example, reindexing should traverse ancestors from all named refs in addition to the visible heads. --- lib/src/default_index/store.rs | 5 ++-- lib/src/view.rs | 44 ++++++++++++++++++++++++++++++ lib/tests/test_index.rs | 49 ++++++++++++++++++++++++++++++++++ 3 files changed, 96 insertions(+), 2 deletions(-) diff --git a/lib/src/default_index/store.rs b/lib/src/default_index/store.rs index a228d4028..dea80f188 100644 --- a/lib/src/default_index/store.rs +++ b/lib/src/default_index/store.rs @@ -147,7 +147,8 @@ impl DefaultIndexStore { let operations_dir = self.dir.join("operations"); let commit_id_length = store.commit_id_length(); let change_id_length = store.change_id_length(); - let mut visited_heads: HashSet = view.heads().clone(); + let mut visited_heads: HashSet = + view.all_referenced_commit_ids().cloned().collect(); let mut historical_heads: Vec<(CommitId, OperationId)> = visited_heads .iter() .map(|commit_id| (commit_id.clone(), operation.id().clone())) @@ -167,7 +168,7 @@ impl DefaultIndexStore { parent_op_id = Some(op.id().clone()); } // TODO: no need to walk ancestors of the parent_op_id operation - for commit_id in op.view()?.heads() { + for commit_id in op.view()?.all_referenced_commit_ids() { if visited_heads.insert(commit_id.clone()) { historical_heads.push((commit_id.clone(), op.id().clone())); } diff --git a/lib/src/view.rs b/lib/src/view.rs index b97bef485..42ec8cd64 100644 --- a/lib/src/view.rs +++ b/lib/src/view.rs @@ -311,6 +311,50 @@ impl View { self.data.git_head = target; } + /// Iterates all commit ids referenced by this view. + /// + /// This can include hidden commits referenced by remote branches, previous + /// positions of conflicted branches, etc. The ancestors and predecessors of + /// the returned commits should be considered reachable from the view. Use + /// this to build commit index from scratch. + /// + /// The iteration order is unspecified, and may include duplicated entries. + pub fn all_referenced_commit_ids(&self) -> impl Iterator { + // Include both added/removed ids since ancestry information of old + // references will be needed while merging views. + fn ref_target_ids(target: &RefTarget) -> impl Iterator { + target.as_merge().iter().flatten() + } + + // Some of the fields (e.g. wc_commit_ids) would be redundant, but let's + // not be smart here. Callers will build a larger set of commits anyway. + let op_store::View { + head_ids, + public_head_ids, + local_branches, + tags, + remote_views, + git_refs, + git_head, + wc_commit_ids, + } = &self.data; + itertools::chain!( + head_ids, + public_head_ids, + local_branches.values().flat_map(ref_target_ids), + tags.values().flat_map(ref_target_ids), + remote_views.values().flat_map(|remote_view| { + let op_store::RemoteView { branches } = remote_view; + branches + .values() + .flat_map(|remote_ref| ref_target_ids(&remote_ref.target)) + }), + git_refs.values().flat_map(ref_target_ids), + ref_target_ids(git_head), + wc_commit_ids.values() + ) + } + pub fn set_view(&mut self, data: op_store::View) { self.data = data; } diff --git a/lib/tests/test_index.rs b/lib/tests/test_index.rs index 4094c3a9e..3abc321ab 100644 --- a/lib/tests/test_index.rs +++ b/lib/tests/test_index.rs @@ -25,6 +25,7 @@ use jj_lib::default_index::{ }; use jj_lib::index::Index as _; use jj_lib::object_id::{HexPrefix, ObjectId as _, PrefixResolution}; +use jj_lib::op_store::{RefTarget, RemoteRef}; use jj_lib::repo::{MutableRepo, ReadonlyRepo, Repo}; use jj_lib::settings::UserSettings; use testutils::test_backend::TestBackend; @@ -313,6 +314,54 @@ fn test_index_commits_previous_operations() { assert_eq!(generation_number(index, commit_c.id()), 3); } +#[test] +fn test_index_commits_hidden_but_referenced() { + // Test that hidden-but-referenced commits are indexed. + let settings = testutils::user_settings(); + let test_repo = TestRepo::init(); + let repo = &test_repo.repo; + + // Remote branches are usually visible at a certain point in operation + // history, but that's not guaranteed if old operations have been discarded. + // This can also happen if imported remote branches get immediately + // abandoned because the other branch has moved. + let mut tx = repo.start_transaction(&settings); + let commit_a = write_random_commit(tx.mut_repo(), &settings); + let commit_b = write_random_commit(tx.mut_repo(), &settings); + let commit_c = write_random_commit(tx.mut_repo(), &settings); + tx.mut_repo().remove_head(commit_a.id()); + tx.mut_repo().remove_head(commit_b.id()); + tx.mut_repo().remove_head(commit_c.id()); + tx.mut_repo().set_remote_branch( + "branch", + "origin", + RemoteRef { + target: RefTarget::from_legacy_form( + [commit_a.id().clone()], + [commit_b.id().clone(), commit_c.id().clone()], + ), + state: jj_lib::op_store::RemoteRefState::New, + }, + ); + let repo = tx.commit("test"); + + // All commits should be indexed + assert!(repo.index().has_id(commit_a.id())); + assert!(repo.index().has_id(commit_b.id())); + assert!(repo.index().has_id(commit_c.id())); + + // Delete index from disk + let default_index_store: &DefaultIndexStore = + repo.index_store().as_any().downcast_ref().unwrap(); + default_index_store.reinit().unwrap(); + + let repo = load_repo_at_head(&settings, repo.repo_path()); + // All commits should be reindexed + assert!(repo.index().has_id(commit_a.id())); + assert!(repo.index().has_id(commit_b.id())); + assert!(repo.index().has_id(commit_c.id())); +} + #[test] fn test_index_commits_incremental() { let settings = testutils::user_settings();