Merge pull request 'Judge the one protection rule the forge applies, and keep a recorded repository id (#174 follow-up)' (#177) from fix/the-forges-first-rule-and-a-kept-identity into main

cmd/mesh-controller/build_source.go

@@ -175,33 +175,54 @@ var askTheForge = func(ctx context.Context, owner, repo, branch string) (forgeFa
 		return forgeFacts{}, fmt.Errorf("the forge named no id for %s/%s", owner, repo)
 	}
 	var rules struct {
-		Rules []struct {
+		Rules []protectionRule `json:"rules"`
-			Rule             string   `json:"rule"`
-			Push             bool     `json:"push"`
-			RequiredStatuses []string `json:"required_statuses"`
-			AdminMayOverride bool     `json:"admin_may_override"`
-		} `json:"rules"`
 	}
 	if err := ask("gitea_branch_protection_get", map[string]any{"owner": owner, "repo": repo}, &rules); err != nil {
 		return forgeFacts{}, err
 	}
-	facts := forgeFacts{ID: found.Result.ID, Why: fmt.Sprintf("no protection rule covers %s", branch)}
+	facts := judgedRule(rules.Rules, branch)
-	for _, r := range rules.Rules {
+	facts.ID = found.Result.ID
-		if !ruleCovers(r.Rule, branch) {
+	return facts, nil
-			continue
+}
-		}
+
-		switch {
+// protectionRule is a branch protection rule as the forge's tool summarises it.
-		case r.Push:
+type protectionRule struct {
-			facts.Why = fmt.Sprintf("the rule %s lets a person push to %s directly", r.Rule, branch)
+	Rule             string   `json:"rule"`
-		case len(r.RequiredStatuses) == 0:
+	Push             bool     `json:"push"`
-			facts.Why = fmt.Sprintf("the rule %s requires no status before a merge into %s", r.Rule, branch)
+	RequiredStatuses []string `json:"required_statuses"`
-		case r.AdminMayOverride:
+	AdminMayOverride bool     `json:"admin_may_override"`
-			facts.Why = fmt.Sprintf("the rule %s lets an administrator merge into %s past a status", r.Rule, branch)
+}
-		default:
+
-			return forgeFacts{ID: facts.ID, Guarded: true}, nil
+// judgedRule judges the one rule the forge applies to a branch, as the forge picks it: the rule named for the
+// branch, else the first glob rule, in the forge's order, that covers it (gitea's first matching rule). Never a
+// later rule that happens to be stronger: the forge does not read it.
+func judgedRule(rules []protectionRule, branch string) forgeFacts {
+	var applied *protectionRule
+	for i := range rules {
+		if rules[i].Rule == branch {
+			applied = &rules[i]
+			break
 		}
 	}
-	return facts, nil
+	if applied == nil {
+		for i := range rules {
+			if ruleCovers(rules[i].Rule, branch) {
+				applied = &rules[i]
+				break
+			}
+		}
+	}
+	switch r := applied; {
+	case r == nil:
+		return forgeFacts{Why: fmt.Sprintf("no protection rule covers %s", branch)}
+	case r.Push:
+		return forgeFacts{Why: fmt.Sprintf("the rule %s lets a person push to %s directly", r.Rule, branch)}
+	case len(r.RequiredStatuses) == 0:
+		return forgeFacts{Why: fmt.Sprintf("the rule %s requires no status before a merge into %s", r.Rule, branch)}
+	case r.AdminMayOverride:
+		return forgeFacts{Why: fmt.Sprintf("the rule %s lets an administrator merge into %s past a status", r.Rule, branch)}
+	}
+	return forgeFacts{Guarded: true}
 }
 
 // ruleCovers says a protection rule's name — a branch, or a glob of them — covers a branch, as the forge reads it.

cmd/mesh-controller/build_source_test.go

@@ -343,3 +343,38 @@ func TestTheServingControllerIsNeverTheTerminal(t *testing.T) {
 		t.Fatal("what the serving controller starts does not carry its mark")
 	}
 }
+
+// The forge applies one rule to a branch: the rule named for it, else the first glob that covers it. A stronger
+// rule later in the list is not what guards the branch, and is not read as if it were (the confirmation review).
+func TestTheRuleJudgedIsTheOneTheForgeApplies(t *testing.T) {
+	guarded := protectionRule{Rule: "*", RequiredStatuses: []string{"mesh/merge-gate"}}
+	open := protectionRule{Rule: "main", Push: true, RequiredStatuses: []string{"mesh/merge-gate"}}
+	if f := judgedRule([]protectionRule{guarded, open}, "main"); f.Guarded {
+		t.Error("an exact rule letting pushes was passed over for a glob that guards")
+	}
+	if f := judgedRule([]protectionRule{{Rule: "ma*", RequiredStatuses: nil}, {Rule: "*", RequiredStatuses: []string{"x"}}},
+		"main"); f.Guarded || !strings.Contains(f.Why, "ma*") {
+		t.Errorf("the first glob covering the branch was not the one judged: %+v", f)
+	}
+	if f := judgedRule([]protectionRule{{Rule: "release/*"}, {Rule: "main", RequiredStatuses: []string{"x"}}}, "main"); !f.Guarded {
+		t.Errorf("the rule named for the branch was not judged: %+v", f)
+	}
+	if f := judgedRule(nil, "main"); f.Guarded {
+		t.Error("no rule is no protection")
+	}
+}
+
+// An id the forge could not give keeps the id already recorded.
+func TestAnUnknownIdentityKeepsTheRecordedOne(t *testing.T) {
+	open := theCatalogue(t)
+	ctx := t.Context()
+	if err := open.inventory.SetSourceIdentity(ctx, "sudo", 100); err != nil {
+		t.Fatal(err)
+	}
+	if err := open.inventory.SetSourceIdentity(ctx, "sudo", 0); err != nil {
+		t.Fatal(err)
+	}
+	if id, _ := open.inventory.SourceIdentity(ctx, "sudo"); id != 100 {
+		t.Fatalf("the recorded id became %d", id)
+	}
+}

internal/inventory/catalogue.go

@@ -182,9 +182,11 @@ func (i *Inventory) SourceIdentity(ctx context.Context, module string) (int64, e
 	return *id, nil
 }
 
-// SetSourceIdentity records the forge's id of the repository a module is registered from; 0 records none.
+// SetSourceIdentity records the forge's id of the repository a module is registered from. 0 — the forge could not
+// say, at the terminal — keeps the id already recorded: an unknown is never a reason to forget what was known.
 func (i *Inventory) SetSourceIdentity(ctx context.Context, module string, id int64) error {
-	_, err := i.store.Pool().Exec(ctx, `update module set source_repo_id = nullif($2::bigint, 0) where name = $1`, module, id)
+	_, err := i.store.Pool().Exec(ctx,
+		`update module set source_repo_id = coalesce(nullif($2::bigint, 0), source_repo_id) where name = $1`, module, id)
 	return err
 }