fix: stop find_nearest_bookmarked_ancestors re-walking merge ancestors combinatorially (RIG-3585)

src/bookmark.rs

@@ -685,6 +685,7 @@ impl<'a> BookmarkGraph<'a> {
                 bookmark.change(),
                 skip_untracked_local_bookmarks,
                 &pending_bookmarks,
+                &mut HashSet::new(),
             )?;
 
             adjacency_list
@@ -861,11 +862,16 @@ impl<'a> BookmarkGraph<'a> {
     }
 
     /// Find the nearest bookmarked ancestors starting from a given commit.
+    ///
+    /// Each unbookmarked commit is expanded once per starting bookmark. Its
+    /// first expansion collects all reachable boundary bookmarks, so later
+    /// paths to the same commit need not expand it again.
     fn find_nearest_bookmarked_ancestors(
         jj: &Jujutsu,
         from: &Change,
         skip_untracked_local_bookmarks: bool,
         pending_bookmarks: &HashSet<String>,
+        visited: &mut HashSet<String>,
     ) -> Result<Vec<Change>> {
         let mut ancestors = Vec::new();
 
@@ -878,15 +884,16 @@ impl<'a> BookmarkGraph<'a> {
                 .filter(|bookmark| !skip_untracked_local_bookmarks || bookmark.is_tracked())
                 .collect();
 
-            if bookmarks.is_empty() && !pending_bookmarks.contains(&parent.change_id) {
+            if !bookmarks.is_empty() || pending_bookmarks.contains(&parent.change_id) {
+                ancestors.push(parent);
+            } else if visited.insert(parent.commit_id.clone()) {
                 ancestors.extend(Self::find_nearest_bookmarked_ancestors(
                     jj,
                     &parent,
                     skip_untracked_local_bookmarks,
                     pending_bookmarks,
+                    visited,
                 )?);
-            } else {
-                ancestors.push(parent);
             }
         }
 

src/jj.rs

@@ -441,6 +441,11 @@ pub struct Jujutsu {
 
     /// The default branch name.
     default_branch: OnceCell<Result<String, Error>>,
+
+    /// Count of `jj` subprocesses spawned, used by tests to assert the number
+    /// of invocations stays linear (see the merge-graph re-walk regression).
+    #[cfg(test)]
+    exec_count: core::sync::atomic::AtomicUsize,
 }
 
 /// Assemble the full argv for a bookmark push: the resolved push command
@@ -505,6 +510,8 @@ impl Jujutsu {
             cwd: cwd.into(),
             config_override: None,
             default_branch: OnceCell::new(),
+            #[cfg(test)]
+            exec_count: core::sync::atomic::AtomicUsize::new(0),
         })
     }
 
@@ -554,6 +561,10 @@ impl Jujutsu {
         let args_string = args.iter().map(|s| s.as_ref().to_string_lossy()).join(" ");
         trace!("Running jj command: jj {args_string}",);
 
+        #[cfg(test)]
+        self.exec_count
+            .fetch_add(1, core::sync::atomic::Ordering::Relaxed);
+
         let jj_bin = Self::which()?;
         let mut cmd = Command::new(&jj_bin);
         cmd.current_dir(&self.cwd).args(args);
@@ -582,6 +593,13 @@ impl Jujutsu {
         })
     }
 
+    /// Number of `jj` subprocesses spawned so far. Test-only.
+    #[cfg(test)]
+    #[must_use]
+    pub fn exec_count(&self) -> usize {
+        self.exec_count.load(core::sync::atomic::Ordering::Relaxed)
+    }
+
     /// Run an arbitrary command given as a full argv (`argv[0]` is the binary,
     /// the rest are its arguments), from the repo working directory. Used for
     /// the configurable push command (`jj-vine.push`), which may reach a

src/tests/edge_cases.rs

@@ -251,6 +251,102 @@ fn find_changes_to_submit_excludes_foreign_authored_ancestry_companion() -> Resu
     Ok(())
 }
 
+/// Regression: a chain of merges must not re-walk shared ancestors once per
+/// path. Each repeated expansion starts another `jj log` subprocess.
+#[test]
+fn merge_ladder_does_not_rewalk_combinatorially() -> Result<()> {
+    let repo = TestRepo::new();
+
+    // Each rung merges a child with its parent. That parent is reached twice:
+    // directly from the merge and through the child. Temporary rung bookmarks
+    // only navigate the setup; remove them before resolving leaf to base.
+    repo.create_change("base.txt", "base", "base")
+        .create_bookmark("base");
+
+    // Eight rungs exceed 500 invocations without deduplication, but stay below
+    // 200 with the visited set.
+    let rungs: u32 = 8;
+    for i in 0..rungs {
+        let parent = if i == 0 {
+            "base".to_owned()
+        } else {
+            format!("rung{}", i - 1)
+        };
+        repo.jj(["new", &parent])?
+            .create_change(&format!("r{i}.txt"), "r", "right");
+        let rung_name = format!("rung{i}");
+        repo.jj(["new", "@", "@-"])?
+            .create_change(&format!("m{i}.txt"), "m", "merge")
+            .create_bookmark(&rung_name);
+    }
+
+    repo.jj(["new", &format!("rung{}", rungs - 1)])?
+        .create_change("leaf.txt", "leaf", "leaf")
+        .create_bookmark("leaf");
+
+    // Drop the interior rung bookmarks so only `base` and `leaf` remain.
+    for i in 0..rungs {
+        repo.jj(["bookmark", "delete", &format!("rung{i}")])?;
+    }
+
+    let changes = repo.jj.log("base | leaf")?;
+    let bookmarks: Vec<_> = BookmarkOrPending::from_changes(&changes)
+        .into_iter()
+        .collect();
+
+    let before = repo.jj.exec_count();
+    let graph = BookmarkGraph::from_bookmarks(&repo.jj, bookmarks.iter().cloned(), false)?;
+    let spawned = repo.jj.exec_count() - before;
+
+    // The pre-fix walk spawns over 500 subprocesses at eight rungs.
+    // A generous linear ceiling separates the two regimes.
+    assert!(
+        spawned <= 200,
+        "resolving `leaf` over {rungs} merge rungs spawned {spawned} jj \
+         invocations; expected a linear count (<=200). A combinatorial blow-up \
+         means the visited-set dedup regressed."
+    );
+
+    // The fix preserves behavior: leaf resolves and its downstack reaches base
+    // through the ladder.
+    assert_some!(graph.find_bookmark_in_components("leaf"));
+    let downstack = graph.downstack_of("leaf")?;
+    assert_any!(downstack.iter(), |b: &BookmarkOrPending| b.name() == "base");
+
+    Ok(())
+}
+
+#[test]
+fn merge_frontier_preserves_both_bookmarked_parents() -> Result<()> {
+    let repo = TestRepo::new();
+    repo.create_change("base.txt", "base", "base")
+        .create_bookmark("base");
+    repo.jj(["new", "base"])?
+        .create_change("left.txt", "left", "left")
+        .create_bookmark("left");
+    repo.jj(["new", "base"])?
+        .create_change("right.txt", "right", "right")
+        .create_bookmark("right");
+    repo.jj(["new", "left", "right"])?
+        .create_change("merge.txt", "merge", "merge")
+        .jj(["new", "@"])?
+        .create_change("leaf.txt", "leaf", "leaf")
+        .create_bookmark("leaf");
+    repo.jj(["new", "@-"])?
+        .create_change("peer.txt", "peer", "peer")
+        .create_bookmark("peer");
+
+    let changes = repo.jj.log("base | left | right | leaf | peer")?;
+    let graph = BookmarkGraph::from_changes(&repo.jj, &changes, false)?;
+    for child in ["leaf", "peer"] {
+        let downstack = graph.downstack_of(child)?;
+        assert_any!(downstack.iter(), |b: &BookmarkOrPending| b.name() == "left");
+        assert_any!(downstack.iter(), |b: &BookmarkOrPending| b.name()
+            == "right");
+    }
+    Ok(())
+}
+
 #[cfg(not(feature = "no-e2e-tests"))]
 mod e2e {
     use assertables::assert_contains;