From 9551e3cf0a4b15411955e8aee83f8c33944eaac7 Mon Sep 17 00:00:00 2001 From: Waleed Khan Date: Tue, 16 Jan 2024 18:23:16 -0800 Subject: [PATCH 1/2] refactor(bisect): pass `Bounds` to `Strategy::midpoint` I forgot that we have a container type for the success+failure bounds, so use that here. --- scm-bisect/examples/guessing_game.rs | 8 ++-- scm-bisect/src/basic.rs | 7 ++- scm-bisect/src/search.rs | 68 +++++++++++++--------------- 3 files changed, 42 insertions(+), 41 deletions(-) diff --git a/scm-bisect/examples/guessing_game.rs b/scm-bisect/examples/guessing_game.rs index 3bfd7d5d5..59b0e7ab5 100644 --- a/scm-bisect/examples/guessing_game.rs +++ b/scm-bisect/examples/guessing_game.rs @@ -1,5 +1,4 @@ use std::cmp::Ordering; -use std::collections::HashSet; use std::convert::Infallible; use std::io; use std::ops::RangeInclusive; @@ -38,10 +37,13 @@ impl search::Strategy for Strategy { fn midpoint( &self, _graph: &Graph, - success_bounds: &HashSet, - failure_bounds: &HashSet, + bounds: &search::Bounds, _statuses: &IndexMap, ) -> Result, Self::Error> { + let search::Bounds { + success: success_bounds, + failure: failure_bounds, + } = bounds; let lower_bound = success_bounds .iter() .max() diff --git a/scm-bisect/src/basic.rs b/scm-bisect/src/basic.rs index 996a95756..aa9ab9c5d 100644 --- a/scm-bisect/src/basic.rs +++ b/scm-bisect/src/basic.rs @@ -198,10 +198,13 @@ impl search::Strategy for BasicStrategy { fn midpoint( &self, graph: &G, - success_bounds: &HashSet, - failure_bounds: &HashSet, + bounds: &search::Bounds, statuses: &IndexMap, ) -> Result, G::Error> { + let search::Bounds { + success: success_bounds, + failure: failure_bounds, + } = bounds; let mut nodes_to_search = { let implied_success_nodes = graph.ancestors_all(success_bounds.clone())?; let implied_failure_nodes = graph.descendants_all(failure_bounds.clone())?; diff --git a/scm-bisect/src/search.rs b/scm-bisect/src/search.rs index 61f0af792..19fb112c1 100644 --- a/scm-bisect/src/search.rs +++ b/scm-bisect/src/search.rs @@ -121,8 +121,7 @@ pub trait Strategy: Debug { fn midpoint( &self, graph: &G, - success_bounds: &HashSet, - failure_bounds: &HashSet, + bounds: &Bounds, statuses: &IndexMap, ) -> Result, Self::Error>; } @@ -274,8 +273,7 @@ impl Search { #[derive(Debug)] struct State { - success_bounds: HashSet, - failure_bounds: HashSet, + bounds: Bounds, statuses: IndexMap, } @@ -292,24 +290,16 @@ impl Search { fn next(&mut self) -> Option { while let Some(state) = self.states.pop_front() { debug!(?state, "Popped speculation state"); - let State { - success_bounds, - failure_bounds, - statuses, - } = state; - - let node = match self.strategy.midpoint( - self.graph, - &success_bounds, - &failure_bounds, - &statuses, - ) { + let State { bounds, statuses } = state; + + let node = match self.strategy.midpoint(self.graph, &bounds, &statuses) { Ok(Some(node)) => node, Ok(None) => continue, Err(err) => return Some(Err(SearchError::Strategy(err))), }; - for success_node in success_bounds.iter() { + let Bounds { success, failure } = bounds; + for success_node in success.iter() { match self.graph.is_ancestor(node.clone(), success_node.clone()) { Ok(true) => { return Some(Err(SearchError::AlreadySearchedMidpoint { @@ -321,7 +311,7 @@ impl Search { Err(err) => return Some(Err(SearchError::Graph(err))), } } - for failure_node in failure_bounds.iter() { + for failure_node in failure.iter() { match self.graph.is_ancestor(failure_node.clone(), node.clone()) { Ok(true) => { return Some(Err(SearchError::AlreadySearchedMidpoint { @@ -336,14 +326,16 @@ impl Search { // Speculate failure: self.states.push_back(State { - success_bounds: success_bounds.clone(), - failure_bounds: { - let mut failure_bounds = failure_bounds.clone(); - failure_bounds.insert(node.clone()); - match self.graph.simplify_failure_bounds(failure_bounds) { - Ok(bounds) => bounds, - Err(err) => return Some(Err(SearchError::Graph(err))), - } + bounds: Bounds { + success: success.clone(), + failure: { + let mut failure_bounds = failure.clone(); + failure_bounds.insert(node.clone()); + match self.graph.simplify_failure_bounds(failure_bounds) { + Ok(bounds) => bounds, + Err(err) => return Some(Err(SearchError::Graph(err))), + } + }, }, statuses: { let mut statuses = statuses.clone(); @@ -354,15 +346,17 @@ impl Search { // Speculate success: self.states.push_back(State { - success_bounds: { - let mut success_bounds = success_bounds.clone(); - success_bounds.insert(node.clone()); - match self.graph.simplify_success_bounds(success_bounds) { - Ok(bounds) => bounds, - Err(err) => return Some(Err(SearchError::Graph(err))), - } + bounds: Bounds { + success: { + let mut success_bounds = success.clone(); + success_bounds.insert(node.clone()); + match self.graph.simplify_success_bounds(success_bounds) { + Ok(bounds) => bounds, + Err(err) => return Some(Err(SearchError::Graph(err))), + } + }, + failure: failure.clone(), }, - failure_bounds: failure_bounds.clone(), statuses: { let mut statuses = statuses.clone(); statuses.insert(node.clone(), Status::Success); @@ -379,8 +373,10 @@ impl Search { } let initial_state = State { - success_bounds: success_bounds.clone(), - failure_bounds: failure_bounds.clone(), + bounds: Bounds { + success: success_bounds.clone(), + failure: failure_bounds.clone(), + }, statuses: self.nodes.clone(), }; let iter = Iter { From 2e288acf0e157d6bfa240b559aa83d6162685a4c Mon Sep 17 00:00:00 2001 From: Waleed Khan Date: Tue, 26 Dec 2023 10:38:21 -0600 Subject: [PATCH 2/2] refactor(dag): extract `query_stack_commits` to `Dag` So that we can use this logic in `git-branchless-submit`, which wants to be fundamentally aware of commit stacks as part of submitting. --- git-branchless-lib/src/core/dag.rs | 24 ++++++++++++++++++++++++ git-branchless-revset/src/builtins.rs | 25 +++---------------------- 2 files changed, 27 insertions(+), 22 deletions(-) diff --git a/git-branchless-lib/src/core/dag.rs b/git-branchless-lib/src/core/dag.rs index 2c952d650..12162897d 100644 --- a/git-branchless-lib/src/core/dag.rs +++ b/git-branchless-lib/src/core/dag.rs @@ -527,6 +527,30 @@ impl Dag { }) } + /// Determine the connected components among draft commits (commit "stacks") + /// that intersect with the provided set. + #[instrument] + pub fn query_stack_commits(&self, commit_set: CommitSet) -> eyre::Result { + let draft_commits = self.query_draft_commits()?; + let stack_roots = self.query_roots(draft_commits.clone())?; + let stack_ancestors = self.query_range(stack_roots, commit_set)?; + let stack = self + // Note that for a graph like + // + // ``` + // O + // | + // o A + // | \ + // | o B + // | + // @ C + // ``` + // this will return `{A, B, C}`, not just `{A, C}`. + .query_range(stack_ancestors, draft_commits.clone())?; + Ok(stack) + } + /// Wrapper around DAG method. #[instrument] pub fn query_all(&self) -> eyre::Result { diff --git a/git-branchless-revset/src/builtins.rs b/git-branchless-revset/src/builtins.rs index 8cdf3ef61..68717a33a 100644 --- a/git-branchless-revset/src/builtins.rs +++ b/git-branchless-revset/src/builtins.rs @@ -290,28 +290,9 @@ fn fn_draft(ctx: &mut Context, name: &str, args: &[Expr]) -> EvalResult { #[instrument] fn fn_stack(ctx: &mut Context, name: &str, args: &[Expr]) -> EvalResult { let arg = eval0_or_1(ctx, name, args)?.unwrap_or_else(|| ctx.dag.head_commit.clone()); - let draft_commits = ctx - .dag - .query_draft_commits() - .map_err(EvalError::OtherError)?; - let stack_roots = ctx.dag.query_roots(draft_commits.clone())?; - let stack_ancestors = ctx.dag.query_range(stack_roots, arg)?; - let stack = ctx - .dag - // Note that for a graph like - // - // ``` - // O - // | - // o A - // | \ - // | o B - // | - // @ C - // ``` - // this will return `{A, B, C}`, not just `{A, C}`. - .query_range(stack_ancestors, draft_commits.clone())?; - Ok(stack) + ctx.dag + .query_stack_commits(arg) + .map_err(EvalError::OtherError) } type MatcherFn = dyn Fn(&Repo, &Commit) -> Result + Sync + Send;