From 0e04670b689876e4a632240ae527a3e2002ee2c9 Mon Sep 17 00:00:00 2001 From: Nils Lehnen <30603423+iderex@users.noreply.github.com> Date: Fri, 4 Sep 2026 20:25:11 +0200 Subject: [PATCH] Name a set of issues that waits on itself [#234] The blockers run resolves what each blocked issue names and asks whether those issues are still open. Neither question asks whether the issue an issue waits for is waiting back, so a set in which every member waits on another member of the same set passes both untouched: every member reads as ordinarily blocked, every member is subtracted from the count of available work by its label, and the last line of the run calls the board clean. Nothing closing outside such a set ends any of it, so no later run does better on its own and no reader is asked to look. Eight open issues on this board are in one today, and only six of the pairs are visible by eye. #85 reaches the set over four edges with no two of the five bodies on the path naming each other, which is why the walk follows the waits as far as they go instead of comparing bodies two at a time. Only an issue carrying the label is a member, because an issue that does not carry it is claiming to wait for nothing and a number in its body is a mention rather than an edge. That is what keeps a popular open issue out of a set it is merely named by, and it is proved in both directions: an unlabelled issue and a closed one. The set is reported and refuses nothing, which is the one place this package prints a state in capitals without failing on it, and the code says so where a reader meets it. Refusing would put a red gate on a condition whose only repair is a reading of what those issues are for, which the run cannot take and the author of a single issue cannot take alone. Whether it should refuse anyway is the open half of #234 and is not decided here. A row proves the report is printed beside a failure rather than instead of one, so the states that do refuse still reach the verdict. Each row was shown to bite by returning no set from the walk and watching four of them go red. Signed-off-by: Nils Lehnen <30603423+iderex@users.noreply.github.com> --- internal/blockers/blockers.go | 116 ++++++++++++++++++++++++- internal/blockers/blockers_test.go | 133 ++++++++++++++++++++++++++++- 2 files changed, 244 insertions(+), 5 deletions(-) diff --git a/internal/blockers/blockers.go b/internal/blockers/blockers.go index 235d740..a723bb1 100644 --- a/internal/blockers/blockers.go +++ b/internal/blockers/blockers.go @@ -25,6 +25,24 @@ // hold is unresolved, which is a failure and never a pass. A run that resolved // nothing and reported agreement is what this exists to prevent. // +// There is a third reading, and it is the one an issue passes through both +// questions untouched. Neither of them asks whether the issue an issue waits +// for is waiting back. A set in which every member waits on another member of +// the same set is a set nothing outside it can end, and every member of it +// reads as ordinarily blocked, is subtracted from the count of available work +// by its label, and is walked past by every reader for as long as nobody +// notices. Eight of this board's own issues were in one such set when this was +// measured, and the run's last line called them clean. +// +// That reading is REPORTED and refuses nothing, which is the one place this +// package prints a state in capitals without failing on it. The repair is an +// edit to an issue somebody has to decide on, the same repair the two +// questions above ask for, but unlike them it cannot be made by the issue's own +// author alone: which member of the set gives way is a reading of what those +// issues are for. Refusing here would put a red gate on a condition that stands +// until that reading is taken. Whether it should refuse anyway is the open half +// of #234 and is not decided here. +// // The reading is the whole tracker rather than one query per issue. A body // names a number that has closed more often than one that has not, and a // closed issue is absent from the listing an open-issue query answers with, so @@ -211,8 +229,13 @@ func Run(read Reader, out io.Writer) error { } } - fmt.Fprintf(out, "%d issue(s) read, %d naming no issue, %d no longer blocked, %d unresolved.\n", - len(blocked), nameless, cleared, unresolved) + sets := reciprocal(blocked) + for _, set := range sets { + fmt.Fprintf(out, " WAITING ON ITSELF: %s each wait on another issue in this set, so nothing closing outside it makes any of them available\n", numbers(set)) + } + + fmt.Fprintf(out, "%d issue(s) read, %d naming no issue, %d no longer blocked, %d unresolved, %d set(s) waiting on themselves.\n", + len(blocked), nameless, cleared, unresolved, len(sets)) var wrong []string if nameless > 0 { @@ -230,6 +253,95 @@ func Run(read Reader, out io.Writer) error { return nil } +// reciprocal returns every set of the issues given in which each member waits +// on another member of the same set, each set sorted and the sets ordered by +// their lowest member. +// +// Only an issue carrying the label is a member. An issue that does not carry +// it is claiming to wait for nothing, so a reference into it is not a wait and +// no path runs through it, however many blocked issues name it. That is what +// keeps a popular open issue out of a set it is merely mentioned by. +// +// The edges are the references the two questions above already resolve, so +// this reading inherits their bound: a body naming a number in passing is read +// as a wait. What makes the inherited over-reading survivable here is that a +// set needs the reference to run in both directions, so both bodies have to be +// describing something other than a wait, in opposite directions, at once. A +// one-sided mention produces no set. +// +// The walk is a reachability closure per member rather than a linear +// component algorithm. The population is the issues one board has under one +// label, which is tens rather than thousands, and the cost of the closure is +// paid once per run against a reading that took a paged fetch of the whole +// tracker to produce. +func reciprocal(blocked []Issue) [][]int { + member := map[int]bool{} + for _, i := range blocked { + member[i.Number] = true + } + + waits := map[int][]int{} + for _, i := range blocked { + for _, n := range References(i.Body) { + if n == i.Number || !member[n] { + continue + } + waits[i.Number] = append(waits[i.Number], n) + } + } + + // reach[n] is every member a wait from n arrives at, following waits as + // far as they go. n is in its own set when it arrives back at itself, + // which is a path of at least one edge and never the empty one. + reach := map[int]map[int]bool{} + for _, i := range blocked { + seen := map[int]bool{} + stack := append([]int(nil), waits[i.Number]...) + for len(stack) > 0 { + n := stack[len(stack)-1] + stack = stack[:len(stack)-1] + if seen[n] { + continue + } + seen[n] = true + stack = append(stack, waits[n]...) + } + reach[i.Number] = seen + } + + taken := map[int]bool{} + var sets [][]int + for _, i := range blocked { + n := i.Number + if taken[n] || !reach[n][n] { + continue + } + set := []int{n} + taken[n] = true + for _, j := range blocked { + m := j.Number + if taken[m] || !reach[n][m] || !reach[m][n] { + continue + } + set = append(set, m) + taken[m] = true + } + sort.Ints(set) + sets = append(sets, set) + } + return sets +} + +// numbers writes a set the way the rest of this run writes a reference, so a +// reader can take a number out of either line and put it into the same query. +func numbers(set []int) string { + out := make([]string, 0, len(set)) + for _, n := range set { + out = append(out, "#"+strconv.Itoa(n)) + } + return strings.Join(out, ", ") +} + // describe names a reference the way a reader has to see it, because a body // naming a merged pull request and a body naming a closed issue are the same // four characters and are not the same statement. diff --git a/internal/blockers/blockers_test.go b/internal/blockers/blockers_test.go index 7a13366..9bad675 100644 --- a/internal/blockers/blockers_test.go +++ b/internal/blockers/blockers_test.go @@ -85,7 +85,7 @@ func TestRunPassesWhenEveryBlockedIssueNamesSomethingStillOpen(t *testing.T) { if !strings.Contains(out, "#36: blocked, waiting on #35 (open issue)") { t.Fatalf("the run should say what #36 waits on, got:\n%s", out) } - if !strings.Contains(out, "1 issue(s) read, 0 naming no issue, 0 no longer blocked, 0 unresolved.") { + if !strings.Contains(out, "1 issue(s) read, 0 naming no issue, 0 no longer blocked, 0 unresolved, 0 set(s) waiting on themselves.") { t.Fatalf("the run should count what it read, got:\n%s", out) } } @@ -228,7 +228,7 @@ func TestRunPassesABoardOnWhichNothingIsBlocked(t *testing.T) { if err != nil { t.Fatalf("a board where nothing carries the label is a real zero, got %v\n%s", err, out) } - if !strings.Contains(out, "0 issue(s) read, 0 naming no issue, 0 no longer blocked, 0 unresolved.") { + if !strings.Contains(out, "0 issue(s) read, 0 naming no issue, 0 no longer blocked, 0 unresolved, 0 set(s) waiting on themselves.") { t.Fatalf("the run should say it read nothing, got:\n%s", out) } } @@ -275,7 +275,7 @@ func TestRunCountsEveryStateSeparatelyOnOneBoard(t *testing.T) { if err == nil { t.Fatalf("three of these four are wrong and the run should be red, got:\n%s", out) } - if !strings.Contains(out, "4 issue(s) read, 1 naming no issue, 1 no longer blocked, 1 unresolved.") { + if !strings.Contains(out, "4 issue(s) read, 1 naming no issue, 1 no longer blocked, 1 unresolved, 0 set(s) waiting on themselves.") { t.Fatalf("the run should count the three states apart, got:\n%s", out) } for _, want := range []string{"1 name no issue by number", "1 are no longer blocked and still say they are", "1 name a number this board does not hold"} { @@ -284,3 +284,130 @@ func TestRunCountsEveryStateSeparatelyOnOneBoard(t *testing.T) { } } } + +func TestRunNamesTwoIssuesThatWaitOnEachOther(t *testing.T) { + // Neither of these is reported by any of the states above: both name a + // number, both name one that is open, and both resolve. What no reading + // before this one asked is whether the issue each waits for is waiting + // back, and nothing closing anywhere else ends either of them. + out, err := run(t, board( + blockedIssue(35, "Depends on #63, which pins the browser this leg loads the built output in."), + blockedIssue(63, "The second condition needs one browser-backed leg in the same change, which is #35."), + blockedIssue(58, "Depends on #53, the release workflow."), + openIssue(53), + )) + if err != nil { + t.Fatalf("a reciprocal wait is reported and refuses nothing, got %v\n%s", err, out) + } + if !strings.Contains(out, "WAITING ON ITSELF: #35, #63 each wait on another issue in this set") { + t.Fatalf("the run should name the set, got:\n%s", out) + } + if strings.Contains(out, "#58,") || strings.Contains(out, ", #58") { + t.Fatalf("#58 waits on something outside any set and does not belong in one, got:\n%s", out) + } + if !strings.Contains(out, "3 issue(s) read, 0 naming no issue, 0 no longer blocked, 0 unresolved, 1 set(s) waiting on themselves.") { + t.Fatalf("the run should count the set, got:\n%s", out) + } +} + +func TestRunNamesASetLongerThanAPair(t *testing.T) { + // The near miss for the row above, and the reason the walk follows waits + // as far as they go rather than comparing two bodies. No two of these + // three name each other, so a reading that only compared pairs would + // report nothing here and the set would stand. + out, err := run(t, board( + blockedIssue(35, "Depends on #63."), + blockedIssue(63, "Depends on #71."), + blockedIssue(71, "Depends on #35."), + )) + if err != nil { + t.Fatalf("a reciprocal wait is reported and refuses nothing, got %v\n%s", err, out) + } + if !strings.Contains(out, "WAITING ON ITSELF: #35, #63, #71 each wait on another issue in this set") { + t.Fatalf("the run should name all three, got:\n%s", out) + } +} + +func TestRunNamesEachSetSeparatelyRatherThanMergingThem(t *testing.T) { + // Two sets with no path between them are two repairs, and one line + // carrying five numbers would send a reader looking for a wait that is + // not there. + out, err := run(t, board( + blockedIssue(35, "Depends on #63."), + blockedIssue(63, "Depends on #35."), + blockedIssue(71, "Depends on #87."), + blockedIssue(87, "Depends on #71."), + )) + if err != nil { + t.Fatalf("a reciprocal wait is reported and refuses nothing, got %v\n%s", err, out) + } + for _, want := range []string{"WAITING ON ITSELF: #35, #63 each", "WAITING ON ITSELF: #71, #87 each"} { + if !strings.Contains(out, want) { + t.Fatalf("the run should carry %q, got:\n%s", want, out) + } + } + if !strings.Contains(out, "0 unresolved, 2 set(s) waiting on themselves.") { + t.Fatalf("the run should count two sets, got:\n%s", out) + } +} + +func TestRunReadsNoWaitBackFromAnIssueThatIsNotBlocked(t *testing.T) { + // An issue carrying no label is claiming to wait for nothing, so a number + // in its body is a mention and not an edge. Reading it as one turns every + // blocked issue named by the thing it depends on into a set, which is the + // false positive this row exists against. + out, err := run(t, board( + blockedIssue(35, "Depends on #63, which pins the browser."), + Issue{Number: 63, Title: "the browser pin", State: "open", Body: "The legs that need this are #35 and #36."}, + )) + if err != nil { + t.Fatalf("one blocked issue and one open one is an ordinary board, got %v\n%s", err, out) + } + if strings.Contains(out, "WAITING ON ITSELF") { + t.Fatalf("#63 carries no label and waits for nothing, so there is no set, got:\n%s", out) + } + if !strings.Contains(out, "#35: blocked, waiting on #63 (open issue)") { + t.Fatalf("#35 is still ordinarily blocked, got:\n%s", out) + } +} + +func TestRunReadsNoWaitBackFromAClosedIssue(t *testing.T) { + // The other half of the row above. A closed issue is not judged by this + // run at all, so its body's numbers are not edges either, and a set built + // through one would be a set nobody is waiting in. + out, err := run(t, board( + blockedIssue(35, "Depends on #63 and on #90, which is still open."), + Issue{Number: 63, Title: "done", State: "closed", Body: "Waits on #35.", Labels: []string{Label}}, + openIssue(90), + )) + if err != nil { + t.Fatalf("one open dependency keeps #35 blocked, got %v\n%s", err, out) + } + if strings.Contains(out, "WAITING ON ITSELF") { + t.Fatalf("#63 has closed and waits for nothing, so there is no set, got:\n%s", out) + } +} + +func TestRunReportsASetWithoutMovingTheVerdictItWouldOtherwiseReach(t *testing.T) { + // The bound this reading is delivered under, in the direction that would + // hide something: a set is printed beside a failure rather than instead of + // one, and the failure still carries every state that refuses. + out, err := run(t, board( + blockedIssue(35, "Depends on #63."), + blockedIssue(63, "Depends on #35."), + blockedIssue(50, "Depends on #48."), + closedIssue(48), + )) + if err == nil { + t.Fatalf("#50 is no longer blocked and should still red the run, got:\n%s", out) + } + if !strings.Contains(err.Error(), "1 are no longer blocked and still say they are") { + t.Fatalf("the error should carry the state that refuses, got %v", err) + } + if strings.Contains(err.Error(), "waiting on themselves") { + t.Fatalf("a set refuses nothing and does not belong in the error, got %v", err) + } + if !strings.Contains(out, "WAITING ON ITSELF: #35, #63 each") { + t.Fatalf("the set should be printed beside the failure, got:\n%s", out) + } +}