Let an owed act carry a status target, a commit without a pull request and a numbered comment (issue 1322)

go/delivery/gate.go

@@ -130,28 +130,44 @@ func checkOwedAct(act string) (kind, context string, err error) {
 			return "", "", err
 		}
 		return kind, context, nil
-	case OwedView, OwedNote, OwedComment:
+	case OwedView, OwedNote:
 		if hasContext {
 			return "", "", fmt.Errorf("delivery: the owed act %q takes no context", act)
 		}
 		return kind, "", nil
 	}
-	return "", "", fmt.Errorf("delivery: the owed act %q is not status=<context>, view, note or comment", act)
+	if kind, seq, ok := strings.Cut(act, "."); ok && kind == OwedComment {
+		if n, err := strconv.Atoi(seq); err != nil || n < 1 || strconv.Itoa(n) != seq {
+			return "", "", fmt.Errorf("delivery: the owed act %q is a comment without its sequence, comment.<seq> with seq of 1 or more", act)
+		}
+		return OwedComment, seq, nil
+	}
+	return "", "", fmt.Errorf("delivery: the owed act %q is not status=<context>, view, note or comment.<seq>", act)
 }
 
-// ParseOwedAct splits an owed act into its kind and, for a status, its context.
+// OwedCommentAct is the act of the seq-th comment on a head: `comment.<seq>`, seq of 1 or more, so a second comment
+// has a key of its own and does not overwrite the first.
+func OwedCommentAct(seq int) (string, error) {
+	if seq < 1 {
+		return "", fmt.Errorf("delivery: a comment's sequence is 1 or more, not %d", seq)
+	}
+	return OwedComment + "." + strconv.Itoa(seq), nil
+}
+
+// ParseOwedAct splits an owed act into its kind and, for a status, its context, for a comment its sequence as
+// digits.
 func ParseOwedAct(act string) (kind, context string, err error) { return checkOwedAct(act) }
 
 // OwedKey is the key, in OwedBucket, of an act the gate owes the forge on a pull request at a head:
 // `<repository>.<number>.<head>.<act>`, the act being `status=<context>` (mesh/merge-gate), `view`, `note` or
-// `comment`.
+// `comment.<seq>`. The number is 0 for a commit no pull request names, and never negative.
 func OwedKey(repository string, number int, head, act string) (string, error) {
 	repo, err := escapeRepository(repository)
 	if err != nil {
 		return "", err
 	}
-	if number <= 0 {
+	if number < 0 {
-		return "", fmt.Errorf("delivery: an owed key needs the pull request number, not %d", number)
+		return "", fmt.Errorf("delivery: an owed key's pull request number is 0 (none) or more, not %d", number)
 	}
 	if err := checkName("head", head); err != nil {
 		return "", err
@@ -164,7 +180,7 @@ func OwedKey(repository string, number int, head, act string) (string, error) {
 
 // ParseOwedKey is the inverse of OwedKey; act is whole, as `status=<context>` for a status.
 func ParseOwedKey(key string) (repository string, number int, head, act string, err error) {
-	parts := strings.Split(key, ".")
+	parts := strings.SplitN(key, ".", 4) // a comment's act holds a dot
 	if len(parts) != 4 {
 		return "", 0, "", "", fmt.Errorf("delivery: %q is not an owed key, <repository>.<number>.<head>.<act>", key)
 	}
@@ -291,12 +307,14 @@ func (a Answer) check() error {
 	return nil
 }
 
-// Owed is the value of an owed key: what the forge bridge performs. A status needs Context and State, a view and a
+// Owed is the value of an owed key: what the forge bridge performs. A status needs Context, State and Target (the
-// comment need Text, a note needs Text and Ref (the notes ref, mesh-plan).
+// address the status points at), a view and a comment need Text, a note needs Text and Ref (the notes ref,
+// mesh-plan).
 type Owed struct {
 	V       int    `json:"v"`
 	Act     string `json:"act"`
 	Context string `json:"context,omitempty"`
+	Target  string `json:"target,omitempty"`
 	State   string `json:"state,omitempty"`
 	Text    string `json:"text,omitempty"`
 	Ref     string `json:"ref,omitempty"`
@@ -308,6 +326,9 @@ func (o Owed) check() error {
 		if o.Context == "" {
 			return fmt.Errorf("delivery: an owed status has no context")
 		}
+		if o.Target == "" {
+			return fmt.Errorf("delivery: an owed status has no target")
+		}
 		for _, s := range statusStates {
 			if o.State == s {
 				return nil

go/delivery/gate_test.go

@@ -80,7 +80,7 @@ func TestAskID(t *testing.T) { // 1201/AC2
 }
 
 func TestOwedKey(t *testing.T) { // 1201/AC3
-	for _, act := range []string{"status=mesh/merge-gate", "view", "note", "comment"} {
+	for _, act := range []string{"status=mesh/merge-gate", "view", "note", "comment.1", "comment.12"} {
 		key, err := OwedKey("novox/mesh-sdk", 36, "aaaa", act)
 		if err != nil || key != "novox/mesh-sdk.36.aaaa."+act {
 			t.Fatalf("owed key for %s = %q, %v", act, key, err)
@@ -94,7 +94,7 @@ func TestOwedKey(t *testing.T) { // 1201/AC3
 	if err != nil || kind != OwedStatus || context != "mesh/merge-gate" {
 		t.Errorf("owed act = %q %q %v", kind, context, err)
 	}
-	for _, bad := range []string{"", "merge", "status", "status=", "view=x", "status=a.b", "statuses=x"} {
+	for _, bad := range []string{"", "merge", "status", "status=", "view=x", "status=a.b", "statuses=x", "comment", "comment.", "comment.0", "comment.-1", "comment.01", "comment.x", "comment=1"} {
 		if _, err := OwedKey("novox/mesh-sdk", 36, "aaaa", bad); err == nil {
 			t.Errorf("owed act %q accepted", bad)
 		}
@@ -102,13 +102,13 @@ func TestOwedKey(t *testing.T) { // 1201/AC3
 	if _, err := OwedKey("nope", 36, "aaaa", "view"); err == nil {
 		t.Error("a bad repository accepted")
 	}
-	if _, err := OwedKey("novox/mesh-sdk", 0, "aaaa", "view"); err == nil {
+	if _, err := OwedKey("novox/mesh-sdk", -1, "aaaa", "view"); err == nil {
-		t.Error("pull request number 0 accepted")
+		t.Error("a negative pull request number accepted")
 	}
 	if _, err := OwedKey("novox/mesh-sdk", 36, "", "view"); err == nil {
 		t.Error("an empty head accepted")
 	}
-	for _, bad := range []string{"", "novox/mesh-sdk.36.aaaa", "novox/mesh-sdk.x.aaaa.view", "novox/mesh-sdk.36.aaaa.merge", "a.b.36.aaaa.view"} {
+	for _, bad := range []string{"", "novox/mesh-sdk.36.aaaa", "novox/mesh-sdk.x.aaaa.view", "novox/mesh-sdk.-1.aaaa.view", "novox/mesh-sdk.36.aaaa.comment", "novox/mesh-sdk.36.aaaa.merge", "a.b.36.aaaa.view"} {
 		if _, _, _, _, err := ParseOwedKey(bad); err == nil {
 			t.Errorf("parse owed key %q accepted", bad)
 		}
@@ -134,7 +134,7 @@ func sampleAsk() Ask {
 
 func sampleOwed() []Owed {
 	return []Owed{
-		{Act: OwedStatus, Context: "mesh/merge-gate", State: "success"},
+		{Act: OwedStatus, Context: "mesh/merge-gate", State: "success", Target: "https://mesh.example/delivery/36"},
 		{Act: OwedView, Text: "the delivery reaches ace"},
 		{Act: OwedNote, Text: "plan: mesh-mail to ace", Ref: "mesh-plan"},
 		{Act: OwedComment, Text: "merged"},
@@ -252,8 +252,9 @@ func TestGateValuesRefused(t *testing.T) { // 1201/AC5
 	}
 
 	for name, o := range map[string]Owed{
-		"context": {Act: OwedStatus, State: "success"},
+		"context": {Act: OwedStatus, State: "success", Target: "t"},
-		"state":   {Act: OwedStatus, Context: "mesh/merge-gate", State: "green"},
+		"state":   {Act: OwedStatus, Context: "mesh/merge-gate", State: "green", Target: "t"},
+		"target":  {Act: OwedStatus, Context: "mesh/merge-gate", State: "success"},
 		"text":    {Act: OwedView},
 		"ref":     {Act: OwedNote, Text: "line"},
 		"comment": {Act: OwedComment},
@@ -448,3 +449,62 @@ func TestMergeWithoutApprovedHeadsDecodes(t *testing.T) { // 1321/AC3
 		t.Errorf("a merge without approved heads wrote them: %s %v", data, err)
 	}
 }
+
+func TestOwedStatusCarriesItsTarget(t *testing.T) { // 1322/AC1
+	o := Owed{Act: OwedStatus, Context: "mesh/delivery", State: "success", Target: "https://mesh.example/delivery/36"}
+	data, err := EncodeOwed(o)
+	if err != nil {
+		t.Fatal(err)
+	}
+	back, err := DecodeOwed(data)
+	o.V = Version
+	if err != nil || !SameOwed(back, o) || back.Target != o.Target {
+		t.Errorf("decoded %+v, %v; want %+v", back, err, o)
+	}
+	if _, err := EncodeOwed(Owed{Act: OwedStatus, Context: "mesh/delivery", State: "success"}); err == nil || !strings.Contains(err.Error(), "target") {
+		t.Errorf("a status without a target: %v", err)
+	}
+}
+
+func TestOwedKeyNumberZero(t *testing.T) { // 1322/AC2
+	key, err := OwedKey("novox/mesh-sdk", 0, "aaaa", "status=mesh/delivery")
+	if err != nil || key != "novox/mesh-sdk.0.aaaa.status=mesh/delivery" {
+		t.Fatalf("owed key for number 0 = %q, %v", key, err)
+	}
+	repo, number, head, act, err := ParseOwedKey(key)
+	if err != nil || repo != "novox/mesh-sdk" || number != 0 || head != "aaaa" || act != "status=mesh/delivery" {
+		t.Errorf("parse = %q %d %q %q %v", repo, number, head, act, err)
+	}
+	if _, err := OwedKey("novox/mesh-sdk", -1, "aaaa", "view"); err == nil {
+		t.Error("a negative number accepted by OwedKey")
+	}
+	if _, _, _, _, err := ParseOwedKey("novox/mesh-sdk.-1.aaaa.view"); err == nil {
+		t.Error("a negative number accepted by ParseOwedKey")
+	}
+}
+
+func TestOwedCommentsHaveASequence(t *testing.T) { // 1322/AC3
+	a1, err := OwedCommentAct(1)
+	a2, err2 := OwedCommentAct(2)
+	if err != nil || err2 != nil || a1 != "comment.1" || a2 != "comment.2" {
+		t.Fatalf("acts = %q %q %v %v", a1, a2, err, err2)
+	}
+	if _, err := OwedCommentAct(0); err == nil {
+		t.Error("comment sequence 0 accepted")
+	}
+	k1, err := OwedKey("novox/mesh-sdk", 36, "aaaa", a1)
+	k2, err2 := OwedKey("novox/mesh-sdk", 36, "aaaa", a2)
+	if err != nil || err2 != nil || k1 == k2 {
+		t.Fatalf("keys = %q %q %v %v", k1, k2, err, err2)
+	}
+	for key, seq := range map[string]string{k1: "1", k2: "2"} {
+		_, _, _, act, err := ParseOwedKey(key)
+		if err != nil {
+			t.Fatal(err)
+		}
+		kind, got, err := ParseOwedAct(act)
+		if err != nil || kind != OwedComment || got != seq {
+			t.Errorf("parse %q = %q %q %v; want comment %s", key, kind, got, err, seq)
+		}
+	}
+}