issue-report: add transitively-blocks count alongside direct depended-on-by count
This commit is contained in:
parent
aef42b64eb
commit
361121c7f6
2 changed files with 101 additions and 8 deletions
|
|
@ -5,9 +5,9 @@
|
||||||
// swarm-controller's `GET /api/repos` (dropdown source, only repos with
|
// swarm-controller's `GET /api/repos` (dropdown source, only repos with
|
||||||
// an open issue), `GET /api/issue-report` (default — every repo
|
// an open issue), `GET /api/issue-report` (default — every repo
|
||||||
// combined) and `GET /api/repos/{org}/{repo}/issue-report` (once a
|
// combined) and `GET /api/repos/{org}/{repo}/issue-report` (once a
|
||||||
// specific repo is picked) — `blocked` and `depended_on_by_count` both
|
// specific repo is picked) — `blocked`, `depended_on_by_count` and
|
||||||
// arrive pre-resolved per row, so nothing here does its own
|
// `transitively_blocks_count` all arrive pre-resolved per row, so
|
||||||
// dependency-graph walk.
|
// nothing here does its own dependency-graph walk.
|
||||||
//
|
//
|
||||||
// Sorting and the label/hide-blocked filters are client-side over
|
// Sorting and the label/hide-blocked filters are client-side over
|
||||||
// whatever's currently loaded — only the repo selection re-fetches.
|
// whatever's currently loaded — only the repo selection re-fetches.
|
||||||
|
|
@ -42,9 +42,17 @@ interface IssueReportRow {
|
||||||
html_url: string | null;
|
html_url: string | null;
|
||||||
blocked: boolean;
|
blocked: boolean;
|
||||||
depended_on_by_count: number;
|
depended_on_by_count: number;
|
||||||
|
transitively_blocks_count: number;
|
||||||
}
|
}
|
||||||
|
|
||||||
type SortKey = 'repo' | 'number' | 'title' | 'assignees' | 'blocked' | 'depended_on_by_count';
|
type SortKey =
|
||||||
|
| 'repo'
|
||||||
|
| 'number'
|
||||||
|
| 'title'
|
||||||
|
| 'assignees'
|
||||||
|
| 'blocked'
|
||||||
|
| 'depended_on_by_count'
|
||||||
|
| 'transitively_blocks_count';
|
||||||
type SortDir = 'asc' | 'desc';
|
type SortDir = 'asc' | 'desc';
|
||||||
|
|
||||||
// Sentinel `<select>` value for "every repo" — `''` can't collide with a
|
// Sentinel `<select>` value for "every repo" — `''` can't collide with a
|
||||||
|
|
@ -71,6 +79,8 @@ function compareRows(a: IssueReportRow, b: IssueReportRow, key: SortKey): number
|
||||||
return Number(a.blocked) - Number(b.blocked);
|
return Number(a.blocked) - Number(b.blocked);
|
||||||
case 'depended_on_by_count':
|
case 'depended_on_by_count':
|
||||||
return a.depended_on_by_count - b.depended_on_by_count;
|
return a.depended_on_by_count - b.depended_on_by_count;
|
||||||
|
case 'transitively_blocks_count':
|
||||||
|
return a.transitively_blocks_count - b.transitively_blocks_count;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -269,6 +279,20 @@ export function IssueReportPage() {
|
||||||
ariaSort: ariaSortFor('depended_on_by_count'),
|
ariaSort: ariaSortFor('depended_on_by_count'),
|
||||||
render: (r) => r.depended_on_by_count,
|
render: (r) => r.depended_on_by_count,
|
||||||
},
|
},
|
||||||
|
{
|
||||||
|
key: 'transitively_blocks_count',
|
||||||
|
header: (
|
||||||
|
<SortHeader
|
||||||
|
label="transitively blocks"
|
||||||
|
sortKey="transitively_blocks_count"
|
||||||
|
activeKey={sortKey}
|
||||||
|
dir={sortDir}
|
||||||
|
onSort={onSort}
|
||||||
|
/>
|
||||||
|
),
|
||||||
|
ariaSort: ariaSortFor('transitively_blocks_count'),
|
||||||
|
render: (r) => r.transitively_blocks_count,
|
||||||
|
},
|
||||||
];
|
];
|
||||||
|
|
||||||
return (
|
return (
|
||||||
|
|
|
||||||
|
|
@ -69,6 +69,12 @@ pub struct IssueReportRow {
|
||||||
/// dependency — the reverse of `blocked`. Ranks "fix this one to
|
/// dependency — the reverse of `blocked`. Ranks "fix this one to
|
||||||
/// unblock the most other work" highest.
|
/// unblock the most other work" highest.
|
||||||
pub depended_on_by_count: u32,
|
pub depended_on_by_count: u32,
|
||||||
|
/// How many distinct OTHER issues this issue unblocks, following the
|
||||||
|
/// same reverse-dependency edges transitively (a depends on b depends
|
||||||
|
/// on c: fixing c eventually frees both a and b, so c's count includes
|
||||||
|
/// both). A superset of `depended_on_by_count`, which only counts the
|
||||||
|
/// direct edge. See [`Client::issue_report`] for how it's walked.
|
||||||
|
pub transitively_blocks_count: u32,
|
||||||
}
|
}
|
||||||
|
|
||||||
/// The `operators` team, whitelisted for the merge gate on every repo
|
/// The `operators` team, whitelisted for the merge gate on every repo
|
||||||
|
|
@ -631,6 +637,12 @@ impl Client {
|
||||||
/// outside this open-issue set, can never accrue a count here — it
|
/// outside this open-issue set, can never accrue a count here — it
|
||||||
/// isn't a row in the report either, so "how many rows depend on this
|
/// isn't a row in the report either, so "how many rows depend on this
|
||||||
/// row" would have nothing to point at.
|
/// row" would have nothing to point at.
|
||||||
|
///
|
||||||
|
/// `transitively_blocks_count` walks the same `reverse_adj` (blocker
|
||||||
|
/// number → issues naming it as a dependency) from each issue — see
|
||||||
|
/// [`transitive_reach`] for how the walk itself is made
|
||||||
|
/// diamond/cycle-safe. `depended_on_by_count` is `reverse_adj[n].len()`;
|
||||||
|
/// this is everything reachable from `n`, not just the direct edge.
|
||||||
pub async fn issue_report(&self, owner: &str, repo: &str) -> Result<Vec<IssueReportRow>> {
|
pub async fn issue_report(&self, owner: &str, repo: &str) -> Result<Vec<IssueReportRow>> {
|
||||||
const CONCURRENCY: usize = 8;
|
const CONCURRENCY: usize = 8;
|
||||||
|
|
||||||
|
|
@ -672,13 +684,13 @@ impl Client {
|
||||||
.await?;
|
.await?;
|
||||||
|
|
||||||
let open_numbers: HashSet<i64> = issues.iter().filter_map(|i| i.number).collect();
|
let open_numbers: HashSet<i64> = issues.iter().filter_map(|i| i.number).collect();
|
||||||
let mut dependent_counts: HashMap<i64, u32> = HashMap::new();
|
let mut reverse_adj: HashMap<i64, Vec<i64>> = HashMap::new();
|
||||||
for blockers in deps.values() {
|
for (&number, blockers) in &deps {
|
||||||
for blocker in blockers {
|
for blocker in blockers {
|
||||||
if let Some(n) = blocker.number
|
if let Some(n) = blocker.number
|
||||||
&& open_numbers.contains(&n)
|
&& open_numbers.contains(&n)
|
||||||
{
|
{
|
||||||
*dependent_counts.entry(n).or_insert(0) += 1;
|
reverse_adj.entry(n).or_default().push(number);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
@ -709,7 +721,15 @@ impl Client {
|
||||||
.collect(),
|
.collect(),
|
||||||
html_url: issue.html_url.map(|u| u.to_string()),
|
html_url: issue.html_url.map(|u| u.to_string()),
|
||||||
blocked,
|
blocked,
|
||||||
depended_on_by_count: dependent_counts.get(&number).copied().unwrap_or(0),
|
depended_on_by_count: u32::try_from(
|
||||||
|
reverse_adj.get(&number).map_or(0, Vec::len),
|
||||||
|
)
|
||||||
|
.unwrap_or(u32::MAX),
|
||||||
|
transitively_blocks_count: u32::try_from(transitive_reach(
|
||||||
|
number,
|
||||||
|
&reverse_adj,
|
||||||
|
))
|
||||||
|
.unwrap_or(u32::MAX),
|
||||||
})
|
})
|
||||||
})
|
})
|
||||||
.collect())
|
.collect())
|
||||||
|
|
@ -889,6 +909,29 @@ impl Client {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Every distinct node reachable from `start` by following `adj` edges
|
||||||
|
/// (`adj[n]` = the issues that directly depend on `n`), not counting
|
||||||
|
/// `start` itself. Plain DFS with a `visited` set doubling as the
|
||||||
|
/// cycle guard — see [`Client::issue_report`]'s doc comment for why a
|
||||||
|
/// cycle has to be tolerated rather than assumed impossible.
|
||||||
|
fn transitive_reach(start: i64, adj: &HashMap<i64, Vec<i64>>) -> usize {
|
||||||
|
// `start` goes into `visited` up front, pre-empting the one case a
|
||||||
|
// plain "insert on visit" walk gets wrong: a cycle that loops back to
|
||||||
|
// `start` would otherwise re-insert it and count it as its own
|
||||||
|
// descendant. Pre-seeding makes that re-visit a no-op instead, so the
|
||||||
|
// `- 1` below only ever backs out the seed, never a real node.
|
||||||
|
let mut visited: HashSet<i64> = HashSet::from([start]);
|
||||||
|
let mut stack: Vec<i64> = adj.get(&start).cloned().unwrap_or_default();
|
||||||
|
while let Some(node) = stack.pop() {
|
||||||
|
if visited.insert(node)
|
||||||
|
&& let Some(next) = adj.get(&node)
|
||||||
|
{
|
||||||
|
stack.extend(next);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
visited.len() - 1
|
||||||
|
}
|
||||||
|
|
||||||
/// Where a hook lives. The knowledge hook is repo-scoped and the config-PR
|
/// Where a hook lives. The knowledge hook is repo-scoped and the config-PR
|
||||||
/// hook is org-scoped, mirroring exactly where the per-hive registrars put
|
/// hook is org-scoped, mirroring exactly where the per-hive registrars put
|
||||||
/// theirs — a hook on the wrong scope would never fire, and forgejo would
|
/// theirs — a hook on the wrong scope would never fire, and forgejo would
|
||||||
|
|
@ -1041,4 +1084,30 @@ mod tests {
|
||||||
std::env::remove_var(URL_ENV);
|
std::env::remove_var(URL_ENV);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn transitive_reach_walks_a_chain_not_just_the_direct_edge() {
|
||||||
|
// 3 depends on 2 depends on 1: reverse_adj is "who depends on me",
|
||||||
|
// so 1 -> [2], 2 -> [3]. Fixing 1 eventually frees both 2 and 3.
|
||||||
|
let adj = HashMap::from([(1, vec![2]), (2, vec![3])]);
|
||||||
|
assert_eq!(transitive_reach(1, &adj), 2);
|
||||||
|
assert_eq!(transitive_reach(2, &adj), 1);
|
||||||
|
assert_eq!(transitive_reach(3, &adj), 0);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn transitive_reach_counts_a_diamond_descendant_once() {
|
||||||
|
// 1 -> [2, 3], both 2 and 3 -> [4]: 4 is reachable via two paths
|
||||||
|
// but must only be counted once.
|
||||||
|
let adj = HashMap::from([(1, vec![2, 3]), (2, vec![4]), (3, vec![4])]);
|
||||||
|
assert_eq!(transitive_reach(1, &adj), 3);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn transitive_reach_terminates_on_a_cycle() {
|
||||||
|
// 1 -> [2] -> [1]: nothing forge-side prevents a dependency cycle,
|
||||||
|
// so the walk has to survive one instead of looping forever.
|
||||||
|
let adj = HashMap::from([(1, vec![2]), (2, vec![1])]);
|
||||||
|
assert_eq!(transitive_reach(1, &adj), 1);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue