Merge pull request 'Find a rechecked pull request's group by its repository and number (issue 867, ADR 0382 §1)' (#350) from fix/867-recheck-finds-its-group into main

cmd/mesh-controller/check_siblings.go

@@ -57,19 +57,39 @@ func nameOf(repository string) string {
 // learned, empty when they could: the group's members carry no head or base — a mesh-delivery from before the rule.
 func siblingsFor(p link.PullUpdated, entries []inventory.Entry, beside map[string]link.CheckedOut,
 	lines []groupsLine) ([]link.CheckSibling, string) {
-	if p.Head == "" {
-		return nil, ""
-	}
+	own := strings.ToLower(p.Owner + "/" + p.Repo)
 	var group *groupsLine
-	for i := range lines {
-		if lines[i].ID == p.Head {
-			group = &lines[i]
+	if p.Head == "" {
+		// A recheck (mesh-delivery's `recheck` through `delivery check`) carries no head branch name: the group is the one
+		// whose members hold this very pull request, by repository and number.
+		learnable := false
+		for i := range lines {
+			for _, m := range lines[i].Details {
+				learnable = true
+				if p.Number != 0 && m.Number == p.Number && strings.ToLower(m.Repository) == own {
+					group = &lines[i]
+				}
+			}
+		}
+		if group == nil {
+			for _, l := range lines {
+				if len(l.Details) == 0 && len(l.Members) > 0 && !learnable {
+					return nil, "mesh-delivery named the groups' members without their pull requests (a mesh-delivery from before novox/hq ADR 0382): " +
+						"the group of a pull request announced without its head branch cannot be found"
+				}
+			}
+			return nil, "" // a lone pull request
+		}
+	} else {
+		for i := range lines {
+			if lines[i].ID == p.Head {
+				group = &lines[i]
+			}
+		}
+		if group == nil {
+			return nil, ""
 		}
 	}
-	if group == nil {
-		return nil, ""
-	}
-	own := strings.ToLower(p.Owner + "/" + p.Repo)
 	if len(group.Details) == 0 && len(group.Members) > 0 {
 		return nil, "mesh-delivery named the group's members without their heads and bases (a mesh-delivery from before novox/hq ADR 0382)"
 	}

cmd/mesh-controller/check_siblings_test.go

@@ -117,10 +117,25 @@ func TestASiblingIsTheGroupsOtherOpenPullRequestIntoABaseOfTheSameName(t *testin
 	if got, _ := siblingsFor(release, entries, besideTheCore(), lines); len(got) != 1 || got[0].Dir != "mesh-controller" {
 		t.Errorf("into a release branch: %+v", got)
 	}
-	// A pull request on no head branch is in no group.
+	// A recheck announces the pull request without its head branch: its group is the one holding it (repository, number).
 	pull.Head = ""
+	got, unknown := siblingsFor(pull, entries, besideTheCore(), lines)
+	if len(got) != 1 || got[0].Dir != "mesh-tools" || unknown != "" {
+		t.Errorf("a recheck of a group member: %+v %q", got, unknown)
+	}
+	// A pull request in no group, rechecked, is alone: no sibling, and nothing said.
+	pull.Number = 999
 	if got, unknown := siblingsFor(pull, entries, besideTheCore(), lines); got != nil || unknown != "" {
-		t.Errorf("no head branch: %+v %q", got, unknown)
+		t.Errorf("a lone pull request: %+v %q", got, unknown)
+	}
+	if got, unknown := siblingsFor(pull, entries, besideTheCore(), nil); got != nil || unknown != "" {
+		t.Errorf("no groups at all: %+v %q", got, unknown)
+	}
+	// A mesh-delivery that names members without their pull requests cannot find the group of a recheck: it says so.
+	older := []groupsLine{{ID: "feat/x", Members: []string{"novox/mesh-host@111111111111 (checked)"}}}
+	pull.Number = 114
+	if got, unknown := siblingsFor(pull, entries, besideTheCore(), older); got != nil || unknown == "" {
+		t.Errorf("an older mesh-delivery: %+v %q", got, unknown)
 	}
 }
 

cmd/mesh-controller/replay867_test.go

@@ -34,7 +34,13 @@ import (
 // It runs the whole path: the forge announces each pull request (following.PullUpdated), the controller asks
 // mesh-delivery which pull requests the group holds, the ask is queued for the build seat, and the build seat's check
 // (builder.Check, with a container runtime that runs each container's command on this machine) judges it.
-func TestReplay867(t *testing.T) {
+func TestReplay867(t *testing.T) { replay867(t, false) }
+
+// TestReplay867Recheck is the same pair re-checked through mesh-delivery (`delivery check` with one member): the forge's
+// announcement is not repeated, so the pull request reaches the controller without its head branch name.
+func TestReplay867Recheck(t *testing.T) { replay867(t, true) }
+
+func replay867(t *testing.T, recheck bool) {
 	if _, err := exec.LookPath("go"); err != nil {
 		t.Skip("no Go toolchain on this machine to build a judge with")
 	}
@@ -46,7 +52,7 @@ func TestReplay867(t *testing.T) {
 		module     string
 	}{{"the node-engine change", "mesh-host", 114, r.hostHead, "mesh-host"},
 		{"the controller change", "mesh-controller", 346, r.controllerHead, "mesh-controller"}} {
-		v := r.checked(t, c.repo, c.number, c.head)
+		v := r.checked(t, c.repo, c.number, c.head, recheck)
 		if v.Gate == nil || v.Gate.Verdict != "pass" || v.Repo == nil || v.Repo.Verdict != "pass" {
 			t.Errorf("%s (%s#%d) did not pass its own merge check beside its group: gate %+v, repository %+v\n%s",
 				c.what, c.repo, c.number, v.Gate, v.Repo, v.Report)
@@ -179,7 +185,7 @@ func theGroupOfIssue867(t *testing.T) *theIssue867 {
 }
 
 // checked is a pull request announced by the forge, asked of the build seat by the controller, and checked there.
-func (r *theIssue867) checked(t *testing.T, repo string, number int, head string) builderVerdict {
+func (r *theIssue867) checked(t *testing.T, repo string, number int, head string, recheck bool) builderVerdict {
 	t.Helper()
 	js, ask := aBuildQueue(t)
 	_ = ask
@@ -188,6 +194,12 @@ func (r *theIssue867) checked(t *testing.T, repo string, number int, head string
 	p := link.PullUpdated{Owner: "novox", Repo: repo, Number: number, Base: "main", Head: "feat/867", Commit: head,
 		Paths: []string{"lock.txt", "validate/more.go", "cmd/mesh-controller/main.go"}, ModuleDirsSaid: true,
 		MergeCheck: true, MergeCheckSaid: true}
+	if recheck {
+		// As `delivery check --members <one head>` builds it from mesh-delivery's member: no head branch name.
+		p = orderMember{ID: "novox/" + repo + "@" + head[:12], Repository: "novox/" + repo, Base: "main", Head: head,
+			Number: number, Paths: p.Paths, ModuleDirsSaid: true}.pull()
+		p.MergeCheck, p.MergeCheckSaid = true, true
+	}
 	if err := (following{open: r.open, oneShot: true}).PullUpdated(t.Context(), p); err != nil {
 		t.Fatal(err)
 	}

testdata/store-tests

@@ -80,3 +80,4 @@
 ./cmd/mesh-controller TestAPersonsRecordAndTheBusHoldEveryMove
 ./cmd/mesh-controller TestARecordedMoveThatCannotBeJudgedWaits
 ./cmd/mesh-controller TestReplay867
+./cmd/mesh-controller TestReplay867Recheck