diff --git a/modules/mesh-delivery/cmd/mesh-delivery/holder.go b/modules/mesh-delivery/cmd/mesh-delivery/holder.go index 78069b17..adb84b6f 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/holder.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/holder.go @@ -329,6 +329,11 @@ func (h *Holder) PullUpdated(p PullEvent) (isNew bool) { } h.mu.Lock() defer h.mu.Unlock() + return h.pullUpdated(p) +} + +// pullUpdated is PullUpdated under the lock. +func (h *Holder) pullUpdated(p PullEvent) (isNew bool) { id := IDOf(p.repository(), p.HeadSHA) isNew = h.deliveries[id] == nil now := h.Now() @@ -356,6 +361,12 @@ func (h *Holder) PullUpdated(p PullEvent) (isNew bool) { d.AskedAgain = &now h.keep(d) } + // Another pull request at this head, announced while this one is live (issue 1114): kept, so this one's close + // starts the record over for it. + if d.Number != p.Number && !d.State.Final() && d.MergedAs == "" { + d.Others = withOther(d.Others, p) + h.keep(d) + } return } // A delivery stopped only because its pull request closed unmerged does not end its commit's delivery for @@ -405,12 +416,32 @@ func (h *Holder) PullUpdated(p PullEvent) (isNew bool) { // approved, so no Approve ended. An answered ask is left as it is. The record is never a gate (ADR 0421 ยง5). An edit with no // description, or of a pull request with no open delivery at that head, changes nothing. func (h *Holder) PullEdited(p PullEvent) { - if p.HeadSHA == "" || p.Number == 0 || p.Body == "" { + if p.HeadSHA == "" || p.Number == 0 { return } h.mu.Lock() defer h.mu.Unlock() d := h.deliveries[IDOf(p.repository(), p.HeadSHA)] + if d != nil && startedOverFor(d, p.Number) != "" { + // The record at this head is stopped under another, closed pull request (issue 1114: this one was announced while + // that one was live and never heard again): the edit takes it over, as an announcement would. The forge's edit + // event carries no paths; the record's, for the same head, are the ones. + if p.Paths == nil { + p.Paths, p.PathsTruncated, p.Removed = d.Paths, d.PathsTruncated, d.Removed + p.ModuleDirs, p.ModuleDirsSaid = d.ModuleDirs, d.ModuleDirsSaid + } + if p.CloneURL == "" { + p.CloneURL = d.CloneURL + } + if p.BaseSHA == "" { + p.BaseSHA = d.BaseSHA + } + h.pullUpdated(p) + return + } + if p.Body == "" { + return + } if d == nil || d.Number != p.Number || d.State.Final() || d.MergedAs != "" { return } @@ -431,6 +462,23 @@ func (h *Holder) PullEdited(p PullEvent) { } } +// withOther is others with p put in, replacing an earlier announcement of the same pull request. +func withOther(others []PullEvent, p PullEvent) []PullEvent { + out := withoutOther(others, p.Number) + return append(out, p) +} + +// withoutOther is others without the pull request number. +func withoutOther(others []PullEvent, number int) []PullEvent { + var out []PullEvent + for _, o := range others { + if o.Number != number { + out = append(out, o) + } + } + return out +} + // 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: 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 @@ -748,15 +796,38 @@ func verdictOfState(s string) string { func (h *Holder) PullClosed(p PullEvent) { h.mu.Lock() h.forgetUnchecked(p.Owner, p.Repo, p.Number) + var over *PullEvent for _, d := range h.deliveries { - if d.Repository == p.repository() && d.Number == p.Number && !d.State.Final() && d.MergedAs == "" { + if d.Repository != p.repository() { + continue + } + if d.Number != p.Number { + // The other pull request at this head closed first: it is no longer one to take the record over. + if len(d.Others) > 0 { + if rest := withoutOther(d.Others, p.Number); len(rest) != len(d.Others) { + d.Others = rest + h.keep(d) + } + } + continue + } + if !d.State.Final() && d.MergedAs == "" { d.ClosedUnmerged = true h.settleAndKeep(d) if d.Group != "" { h.leaveGroup(d) } + // Another open pull request carries this head (issue 1114): the record starts over for it at once, as a first + // announcement of it would, so it gets its check, its review record and its Approve ask. + if len(d.Others) > 0 { + next := d.Others[0] + over = &next + } } } + if over != nil { + h.pullUpdated(*over) + } h.mu.Unlock() if h.Controller == nil || p.Owner == "" || p.Repo == "" || p.Number <= 0 { return diff --git a/modules/mesh-delivery/cmd/mesh-delivery/issue1114_test.go b/modules/mesh-delivery/cmd/mesh-delivery/issue1114_test.go new file mode 100644 index 00000000..6da2cec4 --- /dev/null +++ b/modules/mesh-delivery/cmd/mesh-delivery/issue1114_test.go @@ -0,0 +1,90 @@ +package main + +import ( + "strings" + "testing" +) + +// novox/hq issue 1114: mesh-controller #379 was opened at the head of the still-open #378. Its announcement read as the +// same delivery announced again, so when #378 closed unmerged the shared record went Stopped and #379 had no delivery. +// The other open pull request at the head is kept on the record; a close of the record's own pull request starts the +// record over for it at once (as issue 1083's take-over does for a later announcement), and an edit of it does so too, +// so a case stuck before the fix frees itself. A merged record is never taken over. + +const head1114 = "0149a3d70149a3d70149a3d70149a3d70149a3d7" + +func TestClosingThePullRequestOfASharedHeadStartsTheOtherOnesDeliveryOver(t *testing.T) { + w := newWorld(t) + w.op.auto = false + id := IDOf(gCtl, head1114) + w.h.PullUpdated(pr(gCtl, 378, head1114, "feat/chain", "internal/a.go")) + b := pr(gCtl, 379, head1114, "feat/chain-2", "internal/a.go") + b.Body = "Reviewed-by: reviewer@shanks at " + head1114 + w.h.PullUpdated(b) // announced while #378 is live: the same delivery heard again + if d := w.delivery(id); d.Number != 378 { + t.Fatalf("setup: the record is #%d, wanted #378 still", d.Number) + } + w.h.PullClosed(pr(gCtl, 378, head1114, "feat/chain")) + d := w.delivery(id) + if d.Number != 379 || d.State.Final() || d.ClosedUnmerged { + t.Fatalf("after #378 closed the record is #%d %s (closed %v); wanted #379, proposed", d.Number, d.State, d.ClosedUnmerged) + } + if d.ReviewedBy == "" { + t.Fatalf("#379's review record was not read: %+v", d) + } + w.h.Checked(verdict(gCtl, 379, head1114, "pass")) + w.settleAll() + if open := w.op.open(); w.state(id) != Ready || len(open) != 1 || + !strings.Contains(w.op.ask(open[0]).Explanation, "reviewed by reviewer") { + t.Fatalf("#379's delivery is %s with asks %v; wanted ready and one Approve ask with its review record", w.state(id), w.op.open()) + } +} + +func TestClosingTheOtherPullRequestOfASharedHeadLeavesTheRecordAlone(t *testing.T) { + w := newWorld(t) + id := IDOf(gCtl, head1114) + w.h.PullUpdated(pr(gCtl, 378, head1114, "feat/chain", "internal/a.go")) + w.h.PullUpdated(pr(gCtl, 379, head1114, "feat/chain-2", "internal/a.go")) + w.h.PullClosed(pr(gCtl, 379, head1114, "feat/chain-2")) + w.h.PullClosed(pr(gCtl, 378, head1114, "feat/chain")) + if d := w.delivery(id); d.Number != 378 || !d.ClosedUnmerged || !d.State.Final() { + t.Fatalf("with #379 closed first, #378's close left %d %s; wanted it stopped under #378", d.Number, d.State) + } +} + +func TestAnEditOfAPullRequestWhoseSharedHeadsRecordIsStoppedUnderAnotherTakesItOver(t *testing.T) { + w := newWorld(t) + id := IDOf(gCtl, head1114) + w.h.PullUpdated(pr(gCtl, 378, head1114, "feat/chain", "internal/a.go")) + w.h.PullClosed(pr(gCtl, 378, head1114, "feat/chain")) // #379 was never heard: the stuck case + if d := w.delivery(id); !d.State.Final() { + t.Fatalf("setup: %s", d.State) + } + e := pr(gCtl, 379, head1114, "feat/chain-2") + e.Body = "Reviewed-by: reviewer@shanks at " + head1114 + w.h.PullEdited(e) + d := w.delivery(id) + if d.Number != 379 || d.State != Proposed || d.ReviewedBy == "" || len(d.Paths) == 0 { + t.Fatalf("after #379's edit the record is #%d %s reviewed %q paths %v; wanted #379 proposed, read, with the paths", d.Number, + d.State, d.ReviewedBy, d.Paths) + } + w.h.PullEdited(e) // an edit heard again changes nothing + if again := w.delivery(id); again.Number != 379 || again.State != Proposed { + t.Fatalf("a second edit moved it: #%d %s", again.Number, again.State) + } +} + +func TestAMergedRecordIsNeverTakenOverByTheOtherPullRequestOfItsHead(t *testing.T) { + w := newWorld(t) + id := IDOf(gCtl, head1114) + w.h.PullUpdated(pr(gCtl, 378, head1114, "feat/chain", "internal/a.go")) + w.h.PullUpdated(pr(gCtl, 379, head1114, "feat/chain-2", "internal/a.go")) + w.h.PullMerged(merged(gCtl, 378, head1114, "f378f378f378f378")) + w.h.PullClosed(pr(gCtl, 378, head1114, "feat/chain")) + e := pr(gCtl, 379, head1114, "feat/chain-2") + e.Body = "text" + w.h.PullEdited(e) + if d := w.delivery(id); d.Number != 378 || d.MergedAs == "" { + t.Fatalf("a merged record was taken over: #%d merged as %q", d.Number, d.MergedAs) + } +} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/model.go b/modules/mesh-delivery/cmd/mesh-delivery/model.go index 822c5a97..a81bc2bf 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/model.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/model.go @@ -69,11 +69,14 @@ type Delivery struct { // The facts the table's guards read. NewerHead is the head that superseded it; ClosedUnmerged the pull // request closed; MergedAs the commit it landed on the trunk as; HeldWhy what holds it for a person. - NewerHead string `json:"newer_head,omitempty"` - ClosedUnmerged bool `json:"closed_unmerged,omitempty"` - MergedAs string `json:"merged_as,omitempty"` - MergedAt time.Time `json:"merged_at"` - HeldWhy string `json:"held_why,omitempty"` + NewerHead string `json:"newer_head,omitempty"` + ClosedUnmerged bool `json:"closed_unmerged,omitempty"` + // Others are the other open pull requests announced at this head while this one was live (issue 1114), as they were + // announced: the record is theirs to take over when this one's pull request closes unmerged. + Others []PullEvent `json:"others,omitempty"` + MergedAs string `json:"merged_as,omitempty"` + MergedAt time.Time `json:"merged_at"` + HeldWhy string `json:"held_why,omitempty"` // BackupsAgainAfter is the revision of its walk when a person released it for a failed backup: a record at that // revision or before still holds the failure the controller was asked to clear, and does not hold it again // (novox/hq ADR 0404, issue 1068).