From b954bab0caf5c42fde1ed6cd5afd185eebc773b5 Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Sat, 23 Dec 2023 11:22:30 +0900
Subject: [PATCH] index: fix partial reindexing to not lose commits only
 reachable from one side

Spotted while adding error propagation there. This wouldn't likely be a real
problem because "jj debug reindex" removes all of the operation links.

The "} else {" condition is removed because it doesn't make sense to exclude
only the exact parent_op_id operation. This can be optimized to not walk
ancestors of the parent_op_id operation, but I don't see a motivation to add
tests covering such scenarios. It's pretty rare that an intermediate operation
link is missing.
---
 lib/src/default_index/store.rs | 18 ++++++-----
 lib/tests/test_index.rs        | 55 ++++++++++++++++++++++++++++++++--
 2 files changed, 63 insertions(+), 10 deletions(-)

diff --git a/lib/src/default_index/store.rs b/lib/src/default_index/store.rs
index 0fb02fb64..3978f0c24 100644
--- a/lib/src/default_index/store.rs
+++ b/lib/src/default_index/store.rs
@@ -121,14 +121,16 @@ impl DefaultIndexStore {
             |op: &Operation| op.parents().collect_vec(),
         ) {
             let op = op?;
-            if operations_dir.join(op.id().hex()).is_file() {
-                if parent_op_id.is_none() {
-                    parent_op_id = Some(op.id().clone())
-                }
-            } else {
-                for head in op.view()?.heads() {
-                    new_heads.insert(head.clone());
-                }
+            // Pick the latest existing ancestor operation as the parent
+            // segment. Perhaps, breadth-first search is more appropriate here,
+            // but that wouldn't matter in practice as the operation log is
+            // mostly linear.
+            if parent_op_id.is_none() && operations_dir.join(op.id().hex()).is_file() {
+                parent_op_id = Some(op.id().clone());
+            }
+            // TODO: no need to walk ancestors of the parent_op_id operation
+            for head in op.view()?.heads() {
+                new_heads.insert(head.clone());
             }
         }
         let maybe_parent_file;
diff --git a/lib/tests/test_index.rs b/lib/tests/test_index.rs
index f88a9794b..9c6bffccc 100644
--- a/lib/tests/test_index.rs
+++ b/lib/tests/test_index.rs
@@ -12,9 +12,10 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+use std::fs;
 use std::sync::Arc;
 
-use jj_lib::backend::CommitId;
+use jj_lib::backend::{CommitId, ObjectId as _};
 use jj_lib::commit::Commit;
 use jj_lib::commit_builder::CommitBuilder;
 use jj_lib::default_index::{
@@ -24,7 +25,8 @@ use jj_lib::index::Index as _;
 use jj_lib::repo::{MutableRepo, ReadonlyRepo, Repo};
 use jj_lib::settings::UserSettings;
 use testutils::{
-    create_random_commit, load_repo_at_head, write_random_commit, CommitGraphBuilder, TestRepo,
+    commit_transactions, create_random_commit, load_repo_at_head, write_random_commit,
+    CommitGraphBuilder, TestRepo,
 };
 
 fn child_commit<'repo>(
@@ -531,6 +533,55 @@ fn test_index_commits_incremental_squashed() {
     assert_eq!(commits_by_level(&repo), vec![71, 20]);
 }
 
+#[test]
+fn test_reindex_from_merged_operation() {
+    let settings = testutils::user_settings();
+    let test_repo = TestRepo::init();
+    let repo = &test_repo.repo;
+
+    // The following operation log:
+    // x (add head, index will be missing)
+    // x (add head, index will be missing)
+    // |\
+    // o o (remove head)
+    // o o (add head)
+    // |/
+    // o
+    let mut txs = Vec::new();
+    for _ in 0..2 {
+        let mut tx = repo.start_transaction(&settings);
+        let commit = write_random_commit(tx.mut_repo(), &settings);
+        let repo = tx.commit("test");
+        let mut tx = repo.start_transaction(&settings);
+        tx.mut_repo().remove_head(commit.id());
+        txs.push(tx);
+    }
+    let repo = commit_transactions(&settings, txs);
+    let mut op_ids_to_delete = Vec::new();
+    op_ids_to_delete.push(repo.op_id());
+    let mut tx = repo.start_transaction(&settings);
+    write_random_commit(tx.mut_repo(), &settings);
+    let repo = tx.commit("test");
+    op_ids_to_delete.push(repo.op_id());
+    let operation_to_reload = repo.operation();
+
+    // Sanity check before corrupting the index store
+    let index = as_readonly_composite(&repo);
+    assert_eq!(index.num_commits(), 4);
+
+    let index_operations_dir = repo.repo_path().join("index").join("operations");
+    for &op_id in &op_ids_to_delete {
+        fs::remove_file(index_operations_dir.join(op_id.hex())).unwrap();
+    }
+
+    // When re-indexing, one of the merge parent operations will be selected as
+    // the parent index segment. The commits in the other side should still be
+    // reachable.
+    let repo = repo.reload_at(operation_to_reload).unwrap();
+    let index = as_readonly_composite(&repo);
+    assert_eq!(index.num_commits(), 4);
+}
+
 /// Test that .jj/repo/index/type is created when the repo is created, and that
 /// it is created when an old repo is loaded.
 #[test]