Name a set of issues that waits on itself [#234]

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.

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)
+	}
+}