diff --git a/modules/mesh-delivery/cmd/mesh-delivery/holder.go b/modules/mesh-delivery/cmd/mesh-delivery/holder.go index 4e18c0a2..749fbabe 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/holder.go +++ b/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) diff --git a/modules/mesh-delivery/cmd/mesh-delivery/issue1083_test.go b/modules/mesh-delivery/cmd/mesh-delivery/issue1083_test.go new file mode 100644 index 00000000..194f7146 --- /dev/null +++ b/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) + } +}