A new pull request at a commit whose delivery was stopped and unmerged takes that delivery over (issue 1083)

modules/mesh-delivery/cmd/mesh-delivery/holder.go

@@ -359,6 +359,12 @@ func (h *Holder) PullUpdated(p PullEvent) (isNew bool) {
 		// another pull request with the same head (novox/hq issue 783): started over below, exactly as for a first
 		// announcement, under the same id — never a transition out of Stopped.
 	}
+	if startedOver != "" {
+		// The record the new pull request takes over leaves the group it was in (issue 1083): the new one joins its own.
+		if old := h.deliveries[id]; old != nil && old.Group != "" {
+			h.leaveGroup(old)
+		}
+	}
 	replaced := ""
 	for _, old := range h.deliveries {
 		if old.Repository == p.repository() && old.Number == p.Number && !sameCommit(old.Commit, p.HeadSHA) &&
@@ -391,14 +397,28 @@ func (h *Holder) PullUpdated(p PullEvent) (isNew bool) {
 }
 
 // startedOverFor is why a delivery already kept at a head's id is started over for a pull request's number rather
-// than taken as announced again: only when it was stopped solely by an earlier, closed duplicate pull request with
-// the same head — still Stopped, ClosedUnmerged, never merged, and under another number (novox/hq issue 783).
-// Empty for the ordinary 'announced again' path, or for any other stop (a person's, a group's, its own close).
+// than taken as announced again: it is Stopped, never merged, and under another number. Stopped by an earlier, closed
+// duplicate pull request with the same head (novox/hq issue 783), or by a person or a group before its pull request
+// closed (issue 1083: the new pull request at that commit takes the record over, the key staying repository@commit).
+// Empty for the ordinary 'announced again' path, or for the same pull request's own stop.
 func startedOverFor(d *Delivery, number int) string {
-	if d != nil && d.State == Stopped && d.ClosedUnmerged && d.MergedAs == "" && d.Number != number {
+	if d == nil || d.State != Stopped || d.MergedAs != "" || d.Number == number {
+		return ""
+	}
+	if d.ClosedUnmerged {
 		return fmt.Sprintf("#%d with the same head closed unmerged", d.Number)
 	}
-	return ""
+	said := fmt.Sprintf("#%d with the same head was stopped", d.Number)
+	if n := len(d.Transitions); n > 0 && d.Transitions[n-1].To == Stopped {
+		last := d.Transitions[n-1]
+		if last.By != "" {
+			said += " by " + last.By
+		}
+		if last.Why != "" {
+			said += ": " + last.Why
+		}
+	}
+	return said
 }
 
 // freshAnnouncement makes a delivery for p's head exactly as a first announcement would: its own id, proposed.
@@ -553,6 +573,9 @@ func (h *Holder) PullMerged(p PullEvent) {
 		// merge in every field but its id: FromForge, and its check the forge's word on the head, so a long-past
 		// merge replayed from the bus's history still falls under the history rule (issue 313) and is not held for
 		// a person (review round 1).
+		if d.Group != "" {
+			h.leaveGroup(d) // issue 1083: the record taken over leaves its group
+		}
 		fresh, t, err := h.freshAnnouncement(p, now, over)
 		if err != nil {
 			h.refuse(err)

modules/mesh-delivery/cmd/mesh-delivery/issue1083_test.go

@@ -0,0 +1,89 @@
+package main
+
+import (
+	"strings"
+	"testing"
+)
+
+// novox/hq issue 1083: mesh-controller #375's delivery was stopped by a person, then #375 was closed and the same commit
+// opened as #376 from another branch. The delivery is named repository@commit; it was Stopped before its pull request
+// closed, so it was not ClosedUnmerged, and #376 got no delivery of its own: its Approve was never asked. A new pull
+// request at a commit whose delivery is Stopped and unmerged, under another number, takes that record over (the key stays
+// repository@commit); the take-over is said in its transitions, with who stopped it and why.
+
+const (
+	head1083   = "e375e375e375e375"
+	branch1083 = "feat/backup-before-send"
+)
+
+func TestAPullRequestAtACommitWhoseDeliveryAPersonStoppedTakesItOver(t *testing.T) {
+	w := newWorld(t)
+	id := IDOf(catalog783, head1083)
+
+	w.h.PullUpdated(pr(catalog783, 375, head1083, branch1083, "src/a.go"))
+	w.h.Checked(verdict(catalog783, 375, head1083, "pass"))
+	if w.state(id) != Ready {
+		t.Fatalf("its passing verdict left it %s", w.state(id))
+	}
+	if _, err := w.h.Stop(id, "to break the loop", "jochen"); err != nil {
+		t.Fatal(err)
+	}
+	// The person stopped it first; the pull request closed afterwards: a final state is not moved by the close.
+	w.h.PullClosed(pr(catalog783, 375, head1083, branch1083))
+	if d := w.delivery(id); d.State != Stopped || d.ClosedUnmerged {
+		t.Fatalf("setup: %s, closed unmerged %v; wanted stopped by the person alone", d.State, d.ClosedUnmerged)
+	}
+
+	w.h.PullUpdated(pr(catalog783, 376, head1083, branch1083+"-2", "src/a.go"))
+	d := w.delivery(id)
+	if d.State != Proposed || d.Number != 376 {
+		t.Fatalf("#376 at the same commit left it %s, Number %d; wanted proposed under #376", d.State, d.Number)
+	}
+	why := d.Transitions[len(d.Transitions)-1].Why
+	if !strings.Contains(why, "#375 with the same head was stopped by jochen: to break the loop") {
+		t.Fatalf("the take-over is not said, with who stopped it and why: %q", why)
+	}
+	w.h.Checked(verdict(catalog783, 376, head1083, "pass"))
+	if w.state(id) != Ready {
+		t.Fatalf("a passing verdict for #376 left it %s", w.state(id))
+	}
+}
+
+// A stopped record that was merged is not taken over, and the same pull request announced again stays stopped.
+func TestAStoppedRecordThatWasMergedIsNotTakenOverByAnotherPullRequest(t *testing.T) {
+	w := newWorld(t)
+	id := IDOf(catalog783, head783c+"1")
+	w.h.PullUpdated(pr(catalog783, 400, head783c+"1", branch1083, "src/a.go"))
+	w.h.PullMerged(merged(catalog783, 400, head783c+"1", "f400f400f400f400"))
+	w.h.PullUpdated(pr(catalog783, 401, head783c+"1", branch1083+"-2", "src/a.go"))
+	if d := w.delivery(id); d.Number != 400 || d.MergedAs == "" {
+		t.Fatalf("a merged record was taken over: Number %d, merged as %q", d.Number, d.MergedAs)
+	}
+}
+
+// The take-over leaves the stopped record's group: the new record joins its own group by its branch.
+func TestATakenOverRecordLeavesTheGroupItWasStoppedIn(t *testing.T) {
+	w := newWorld(t)
+	w.h.PullUpdated(pr(gCtl, 1, gHc, "feat/g", "internal/a.go"))
+	w.h.PullUpdated(pr(gCat, 2, gHk, "feat/g", "modules/app/module.json"))
+	id := IDOf(gCtl, gHc)
+	if _, err := w.h.Stop(id, "no", "jochen"); err != nil {
+		t.Fatal(err)
+	}
+	w.h.PullClosed(pr(gCtl, 1, gHc, "feat/g"))
+	w.h.PullUpdated(pr(gCtl, 9, gHc, "feat/other", "internal/a.go"))
+	w.h.mu.Lock()
+	defer w.h.mu.Unlock()
+	for _, g := range w.h.groups {
+		if g.ID == "feat/g" {
+			for _, m := range g.Members {
+				if m == id {
+					t.Fatalf("group feat/g still lists %s, now #9 on feat/other: %v", id, g.Members)
+				}
+			}
+		}
+	}
+	if d := w.h.deliveries[id]; d.Number != 9 || d.State != Proposed || d.Group == "feat/g" {
+		t.Fatalf("taken over: Number %d, %s, group %q", d.Number, d.State, d.Group)
+	}
+}