diff --git a/src/bookmark.rs b/src/bookmark.rs index bf55eb7..c587df5 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,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, + visited: &mut HashSet, ) -> Result> { 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); } } 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..8c73d12 100644 --- a/src/tests/edge_cases.rs +++ b/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;