diff --git a/modules/mesh-delivery/cmd/mesh-delivery/holder.go b/modules/mesh-delivery/cmd/mesh-delivery/holder.go index 7b448e2b..6a69eb1d 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/holder.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/holder.go @@ -542,6 +542,9 @@ func (h *Holder) PullMerged(p PullEvent) { } } } + if d != nil && d.MergedAs != "" && sameCommit(d.MergedAs, p.MergeCommit) { + return // this merge is already taken: heard again, past the bus's de-duplication window, it changes nothing + } now := h.Now() var ts []Transition if over := startedOverFor(d, p.Number); over != "" { diff --git a/modules/mesh-delivery/cmd/mesh-delivery/pullkey.go b/modules/mesh-delivery/cmd/mesh-delivery/pullkey.go index 2447b91b..9086de56 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/pullkey.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/pullkey.go @@ -56,7 +56,7 @@ func (h *Holder) KeepPull(p PullEvent) { // KeepVerdict writes a verdict into the key of its pull request, when the key holds that head and trunk commit. func (h *Holder) KeepVerdict(c CheckedEvent) { - if h.Pulls == nil || c.Group != "" || c.Number == 0 || c.Commit == "" { + if h.Pulls == nil || c.Group != "" || c.Number == 0 || c.Commit == "" || c.Trunk == "" { return } verdict := c.Verdict @@ -64,7 +64,7 @@ func (h *Holder) KeepVerdict(c CheckedEvent) { return } err := h.Pulls.Change(pullKeyName(c.Owner, c.Repo, c.Number), func(k PullKey) (PullKey, bool) { - if !sameCommit(k.Head, c.Commit) || (c.Trunk != "" && !sameCommit(k.Trunk, c.Trunk)) || k.Verdict == verdict { + if !sameCommit(k.Head, c.Commit) || !sameCommit(k.Trunk, c.Trunk) || k.Verdict == verdict { return k, false } k.Verdict = verdict diff --git a/modules/mesh-delivery/cmd/mesh-delivery/pullkey_test.go b/modules/mesh-delivery/cmd/mesh-delivery/pullkey_test.go index 936c4e4f..37e951df 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/pullkey_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/pullkey_test.go @@ -138,3 +138,34 @@ func TestAPullRequestMarkedReadyHasItsCheckAskedOnTheReadyStream(t *testing.T) { t.Fatalf("the check of an unmarked pull request was asked with ready %v", w2.ctl.checkReady) } } + +func TestATrunkLessVerdictWritesNothingToTheKey(t *testing.T) { + w := newWorld(t) + pulls := newFakePulls() + w.h.Pulls = pulls + w.h.KeepPull(announcedAt(readyHead, "t1t1t1t1t1t1")) + w.h.KeepVerdict(judgedAt("novox/app", 7, readyHead, "pass", "")) + if got := pulls.get(appKey); got.Verdict != "" { + t.Fatalf("a verdict naming no trunk commit was written: %+v", got) + } +} + +// The forge's catch-up after a restart can announce a merge again past the bus's de-duplication window (issue 975): +// heard again, it changes nothing, and moves no trunk back. +func TestTheSameMergeHeardAgainAfterALaterMergeMovesNoTrunkBack(t *testing.T) { + w := newWorld(t) + a, b := "aaaaaaaaaaaaaaaa", "bbbbbbbbbbbbbbbb" + w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) + w.h.PullUpdated(pr("novox/app", 8, a, "feat/y", "src/a.go")) + w.h.PullUpdated(pr("novox/app", 9, b, "feat/z", "src/a.go")) + w.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "m8m8m8m8m8m8")) + w.h.PullMerged(merged("novox/app", 8, a, "m8m8m8m8m8m8")) + w.h.PullMerged(merged("novox/app", 9, b, "m9m9m9m9m9m9")) + id := IDOf("novox/app", readyHead) + before := w.delivery(id) + w.h.PullMerged(merged("novox/app", 8, a, "m8m8m8m8m8m8")) // announced again, past the window + after := w.delivery(id) + if after.TrunkNow != before.TrunkNow || after.TrunkNow != "m9m9m9m9m9m9" { + t.Fatalf("the trunk moved from %q to %q on a merge heard again; wanted it to stay m9m9m9m9m9m9", before.TrunkNow, after.TrunkNow) + } +}