From 090ae9f6fcce0f015b36d31dde0fa3e1ae08db18 Mon Sep 17 00:00:00 2001 From: mintaka Date: Fri, 11 Sep 2026 16:51:29 -0400 Subject: [PATCH 1/2] fix: stop find_nearest_bookmarked_ancestors re-walking merge ancestors combinatorially (RIG-3585) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `find_nearest_bookmarked_ancestors` recursed with no visited set, re-expanding a shared ancestor once per path that reaches it — one `jj log` subprocess per visit. On a history with merge commits outside `trunk()` this is exponential in the number of merges: a 12-rung merge ladder spawns 8194 `jj` invocations and a real 136-commit / 26-merge repo did not terminate in 200s. Thread a `visited` set (keyed on commit id) through the recursion so each commit is expanded at most once. The set of boundary ancestors is unchanged — a boundary found on the first visit already bubbles to the root and the caller deduplicates via `BTreeSet` — so the walk becomes linear with identical output. A test-only `jj` subprocess counter on `Jujutsu` backs a regression test asserting the invocation count stays linear over a 12-rung merge ladder (8194 without the fix, well under 200 with it). Spec-impact: none. Refs RIG-3585 Co-authored-by: Matt Wilkinson --- src/bookmark.rs | 18 ++++++++-- src/jj.rs | 18 ++++++++++ src/tests/edge_cases.rs | 80 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 113 insertions(+), 3 deletions(-) diff --git a/src/bookmark.rs b/src/bookmark.rs index bf55eb7..d723975 100644 --- a/src/bookmark.rs +++ b/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,21 @@ impl<'a> BookmarkGraph<'a> { } /// Find the nearest bookmarked ancestors starting from a given commit. + /// + /// `visited` records the `commit_id` of every commit whose ancestry has + /// already been expanded. Without it, a history with merge commits re-walks + /// shared ancestors once per path that reaches them — one `jj log` + /// subprocess each — which is exponential in the number of merges and does + /// not terminate on a moderately branchy repo. Deduplicating the expansion + /// makes the walk linear; the resulting set of boundary ancestors is + /// unchanged, because a boundary found on the first visit already bubbles + /// up to the root and the caller deduplicates. fn find_nearest_bookmarked_ancestors( jj: &Jujutsu, from: &Change, skip_untracked_local_bookmarks: bool, pending_bookmarks: &HashSet, + visited: &mut HashSet, ) -> Result> { let mut ancestors = Vec::new(); @@ -878,15 +889,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); } } diff --git a/src/jj.rs b/src/jj.rs index 9490753..6340109 100644 --- a/src/jj.rs +++ b/src/jj.rs @@ -441,6 +441,11 @@ pub struct Jujutsu { /// The default branch name. default_branch: OnceCell>, + + /// 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 diff --git a/src/tests/edge_cases.rs b/src/tests/edge_cases.rs index 28a97c4..d91d9ac 100644 --- a/src/tests/edge_cases.rs +++ b/src/tests/edge_cases.rs @@ -251,6 +251,86 @@ fn find_changes_to_submit_excludes_foreign_authored_ancestry_companion() -> Resu Ok(()) } +/// Regression: a history with chained merge commits must not re-walk shared +/// ancestors combinatorially. The old `find_nearest_bookmarked_ancestors` +/// recursed with no visited set and spawned one `jj log` per visit, so a ladder +/// of merges produced an exponential number of `jj` invocations and did not +/// terminate on a moderately branchy repo. With the visited set the walk is +/// linear: the invocation count stays a small multiple of the commit count. +#[test] +fn merge_ladder_does_not_rewalk_combinatorially() -> Result<()> { + let repo = TestRepo::new(); + + // Build a ladder of diamonds on top of a single bookmarked `base`. Each + // rung merges two children of the previous rung, so the previous rung is a + // shared ancestor reachable by two paths. Under the old recursion each rung + // doubled how many times the lower rungs were re-expanded (2^depth); the + // visited set collapses that to one visit each. + // + // The interior rungs must carry no bookmark, or the walk stops at the first + // one. We use temporary bookmarks only to navigate while building, then + // delete them, leaving `base` and `leaf` as the only bookmarks — so + // resolving `leaf` descends the whole ladder to `base`. + repo.create_change("base.txt", "base", "base") + .create_bookmark("base"); + + // 8 rungs: without the fix this spawns 514 jj invocations (2.5x the 200 + // ceiling), so it separates the two regimes decisively at a third less build + // cost than a deeper ladder. + 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!("l{i}.txt"), "l", "left") + .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; + + // Linear bound: without the visited set this is exponential in `rungs` + // (~514 invocations at 8 rungs) and the test would hang at a deeper ladder. + // A generous linear ceiling still separates the two regimes cleanly. + 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(()) +} + #[cfg(not(feature = "no-e2e-tests"))] mod e2e { use assertables::assert_contains; From 5c886dab018a7c41ebc747961f2c87cd7df60af3 Mon Sep 17 00:00:00 2001 From: mintaka Date: Sat, 3 Oct 2026 20:13:29 -0400 Subject: [PATCH 2/2] test(bookmark): cover merge frontier and correct ladder (RIG-3585) Assert both bookmarked parents survive a merge frontier for two children. Remove the unreachable left branch from the complexity fixture, align its comment with the graph, and state the visited-set invariant precisely. Spec-impact: none. Refs RIG-3585 Co-authored-by: Matt Wilkinson --- src/bookmark.rs | 11 ++----- src/tests/edge_cases.rs | 64 +++++++++++++++++++++++++---------------- 2 files changed, 43 insertions(+), 32 deletions(-) diff --git a/src/bookmark.rs b/src/bookmark.rs index d723975..c587df5 100644 --- a/src/bookmark.rs +++ b/src/bookmark.rs @@ -863,14 +863,9 @@ impl<'a> BookmarkGraph<'a> { /// Find the nearest bookmarked ancestors starting from a given commit. /// - /// `visited` records the `commit_id` of every commit whose ancestry has - /// already been expanded. Without it, a history with merge commits re-walks - /// shared ancestors once per path that reaches them — one `jj log` - /// subprocess each — which is exponential in the number of merges and does - /// not terminate on a moderately branchy repo. Deduplicating the expansion - /// makes the walk linear; the resulting set of boundary ancestors is - /// unchanged, because a boundary found on the first visit already bubbles - /// up to the root and the caller deduplicates. + /// 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, diff --git a/src/tests/edge_cases.rs b/src/tests/edge_cases.rs index d91d9ac..8c73d12 100644 --- a/src/tests/edge_cases.rs +++ b/src/tests/edge_cases.rs @@ -251,32 +251,20 @@ fn find_changes_to_submit_excludes_foreign_authored_ancestry_companion() -> Resu Ok(()) } -/// Regression: a history with chained merge commits must not re-walk shared -/// ancestors combinatorially. The old `find_nearest_bookmarked_ancestors` -/// recursed with no visited set and spawned one `jj log` per visit, so a ladder -/// of merges produced an exponential number of `jj` invocations and did not -/// terminate on a moderately branchy repo. With the visited set the walk is -/// linear: the invocation count stays a small multiple of the commit count. +/// 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(); - // Build a ladder of diamonds on top of a single bookmarked `base`. Each - // rung merges two children of the previous rung, so the previous rung is a - // shared ancestor reachable by two paths. Under the old recursion each rung - // doubled how many times the lower rungs were re-expanded (2^depth); the - // visited set collapses that to one visit each. - // - // The interior rungs must carry no bookmark, or the walk stops at the first - // one. We use temporary bookmarks only to navigate while building, then - // delete them, leaving `base` and `leaf` as the only bookmarks — so - // resolving `leaf` descends the whole ladder to `base`. + // 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"); - // 8 rungs: without the fix this spawns 514 jj invocations (2.5x the 200 - // ceiling), so it separates the two regimes decisively at a third less build - // cost than a deeper ladder. + // 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 { @@ -285,8 +273,6 @@ fn merge_ladder_does_not_rewalk_combinatorially() -> Result<()> { format!("rung{}", i - 1) }; repo.jj(["new", &parent])? - .create_change(&format!("l{i}.txt"), "l", "left") - .jj(["new", &parent])? .create_change(&format!("r{i}.txt"), "r", "right"); let rung_name = format!("rung{i}"); repo.jj(["new", "@", "@-"])? @@ -312,9 +298,8 @@ fn merge_ladder_does_not_rewalk_combinatorially() -> Result<()> { let graph = BookmarkGraph::from_bookmarks(&repo.jj, bookmarks.iter().cloned(), false)?; let spawned = repo.jj.exec_count() - before; - // Linear bound: without the visited set this is exponential in `rungs` - // (~514 invocations at 8 rungs) and the test would hang at a deeper ladder. - // A generous linear ceiling still separates the two regimes cleanly. + // 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 \ @@ -331,6 +316,37 @@ fn merge_ladder_does_not_rewalk_combinatorially() -> Result<()> { 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;