mesh-delivery: an empty-head ready mark is cleared only after a minute, and not right after mark-ready; clearer wording (issue 913, issue 873)

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

@@ -4,6 +4,7 @@ import (
 	"errors"
 	"strings"
 	"testing"
+	"time"
 )
 
 // **A reviewer's word marks the pull request ready in the order the forge counts** (novox/hq issue 913): the label,
@@ -91,18 +92,45 @@ func TestMarkReadyWritesNoCommentWhenTheLabelFails(t *testing.T) {
 	}
 }
 
-func TestClearMarkWithAnEmptyHeadSaysWhatWasWrong(t *testing.T) {
+func TestClearMarkWithAnEmptyHeadSaysWhatWasWrongOnceItStayedEmpty(t *testing.T) {
 	w := newWorld(t)
 	w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go"))
 	w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, "")}
 	w.h.SeeOpenPulls()
 	w.settleAll()
-	if len(w.forge.comments) != 1 || !strings.Contains(w.forge.comments[0], "no \"ready at <head>\" comment by a listed account") ||
+	if len(w.forge.comments) != 0 || len(w.forge.unlabels) != 0 {
+		t.Fatalf("an empty-head mark seen once was cleared: %v", w.forge.comments)
+	}
+	w.later(readyTryEvery + time.Second)
+	w.h.SeeOpenPulls()
+	w.settleAll()
+	if len(w.forge.comments) != 1 || !strings.Contains(w.forge.comments[0], "no \"ready at\" comment by a listed account") ||
+		!strings.Contains(w.forge.comments[0], "the head is now "+readyHead) ||
 		!strings.Contains(w.forge.comments[0], "mesh-delivery.mark-ready") || strings.Contains(w.forge.comments[0], "pushed after it") {
 		t.Fatalf("comments: %v", w.forge.comments)
 	}
 }
 
+func TestAMarkReadyJustMadeIsNotClearedByAReadThatPredatesItsComment(t *testing.T) {
+	w := newWorld(t)
+	id := readyDelivery(t, w)
+	if _, err := w.h.MarkReady(id, "a@b", "ok"); err != nil {
+		t.Fatal(err)
+	}
+	w.later(readyTryEvery + time.Second) // the label alone was read long enough ago to count as stale...
+	w.forge.open = []OpenPull{markedPull("novox/app", 7, fullHead, "")}
+	w.h.SeeOpenPulls()
+	w.h.mu.Lock()
+	w.h.deliveries[id].Ready.Seen = w.now // ...but the mark was made just now
+	w.h.marks["novox/app#7"].firstEmpty = w.now.Add(-2 * readyTryEvery)
+	w.h.mu.Unlock()
+	before := len(w.forge.comments)
+	w.settleAll()
+	if len(w.forge.unlabels) != 0 || len(w.forge.comments) != before {
+		t.Fatalf("a mark just made was cleared: %v %v", w.forge.unlabels, w.forge.comments)
+	}
+}
+
 func TestClearMarkWithAnOlderHeadKeepsTheOldWording(t *testing.T) {
 	w := newWorld(t)
 	w.h.PullUpdated(pr("novox/app", 7, pushed, "feat/x", "src/a.go"))

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

@@ -88,6 +88,9 @@ type marked struct {
 	pull OpenPull
 	// cleared is when the mark was cleared, the label taken off: not cleared again within readyTryEvery.
 	cleared time.Time
+	// firstEmpty is when a mark with no head (a label no "ready at" comment follows yet) was first read so: it is a
+	// mark not yet complete, and is cleared only once it has stayed so for readyTryEvery (novox/hq issue 873).
+	firstEmpty time.Time
 }
 
 // pullKey is a pull request as a key: owner/repo#number.
@@ -109,6 +112,13 @@ func (h *Holder) seeMarks(open []OpenPull) {
 			was.pull.HeadSHA == p.HeadSHA {
 			m.cleared = was.cleared
 		}
+		if p.Ready.Head == "" {
+			m.firstEmpty = h.Now()
+			if was := h.marks[k]; was != nil && was.pull.Ready != nil && was.pull.Ready.Head == "" &&
+				was.pull.HeadSHA == p.HeadSHA && !was.firstEmpty.IsZero() {
+				m.firstEmpty = was.firstEmpty
+			}
+		}
 		seen[k] = m
 	}
 	h.marks = seen
@@ -129,6 +139,13 @@ func (h *Holder) readyAsks(now time.Time) []func() {
 		p, mark := m.pull, *m.pull.Ready
 		repository := strings.ToLower(p.Owner + "/" + p.Repo)
 		if !sameFull(mark.Head, p.HeadSHA) {
+			if mark.Head == "" && now.Sub(m.firstEmpty) < readyTryEvery {
+				continue // the label first, its comment not yet: a mark not complete, not an old one
+			}
+			if d := h.deliveries[IDOf(repository, p.HeadSHA)]; d != nil && d.Ready != nil &&
+				sameFull(d.Ready.Head, p.HeadSHA) && now.Sub(d.Ready.Seen) < readyTryEvery {
+				continue // marked through mark-ready a moment ago: this read may predate its comment
+			}
 			if !m.cleared.IsZero() && now.Sub(m.cleared) < readyTryEvery {
 				continue
 			}
@@ -297,8 +314,9 @@ func (h *Holder) clearMark(k string, p OpenPull, mark ReadyMark) func() {
 // comment of a listed account followed (novox/hq issue 873); one with a head names an older head than the pull's.
 func clearedSaying(mark ReadyMark, p OpenPull) string {
 	if mark.Head == "" {
-		return "mesh-delivery: the label " + ReadyLabel + " is taken off: no \"ready at <head>\" comment by a listed " +
-			"account was written after it. Mark it with `mesh-delivery.mark-ready`, which writes both in the order the forge counts."
+		return "mesh-delivery: the label " + ReadyLabel + " is taken off: no \"ready at\" comment by a listed account " +
+			"follows both the label's last addition and the last push; the head is now " + p.HeadSHA + ". Review it and " +
+			"mark that head with `mesh-delivery.mark-ready`, which writes label and comment in the order the forge counts."
 	}
 	return fmt.Sprintf("mesh-delivery: the ready mark at %s is cleared: "+
 		"the head is now %s, pushed after it. Mark the new head ready once it is reviewed.", mark.Head, p.HeadSHA)