mrs: a bare #N in a commit no longer suppresses the description's close !108

merged cmc wants to merge krz/gitbay:mr-close-dedup into main

3 files changed, +32 −6

internal/control/commitrefs.go +14 −5

		@@ -60,17 +60,20 @@ func ProcessCommitMessages(st *store.Store, dir string, repo store.Repo, actorID
60	60	// they are what lands — but the intent is written in the merge request
61	61	// just as often, and a "Closes #N" there used to close nothing.
62	62	//
63		// The dedup key is the merged sha, shared with ProcessCommitMessages, so
64		// an issue named in both a commit and the description is acted on once.
65		func ProcessMRDescription(st *store.Store, repo store.Repo, mr store.MR, actorID int64, sha string) {
	63	// Acting once is guaranteed by the state check, not by the dedup key: a
	64	// commit that closed the issue leaves it closed, and this skips it. The
	65	// key is per merge request rather than the merged sha, because sharing
	66	// the sha let a bare "#N" in a commit message claim it first and silently
	67	// suppress the close.
	68	func ProcessMRDescription(st *store.Store, repo store.Repo, mr store.MR, actorID int64) {
66	69	for _, n := range closingRefs(mr.Title + "\n" + mr.Body) {
67	70	issue, err := st.IssueByNumber(repo.ID, n)
68	71	if err != nil \|\| issue.State != "open" {
69	72	continue // no such issue, or a commit already closed it
70	73	}
71		fresh, err := st.TryRecordCommitRef(issue.ID, sha)
	74	fresh, err := st.TryRecordCommitRef(issue.ID, mrRefKey(mr.Number))
72	75	if err != nil \|\| !fresh {
73		continue
	76	continue // this merge request already acted on this issue
74	77	}
75	78	if err := st.SetIssueState(issue.ID, "closed"); err != nil {
76	79	slog.Error("mr refs: closing issue", "issue", n, "err", err)
		@@ -84,6 +87,12 @@ func ProcessMRDescription(st *store.Store, repo store.Repo, mr store.MR, actorID
84	87	}
85	88	}
86	89
	90	// mrRefKey namespaces a merge request's dedup record so it cannot
	91	// collide with a commit sha.
	92	func mrRefKey(number int64) string {
	93	return fmt.Sprintf("mr-%d", number)
	94	}
	95
87	96	// closingRefs returns the issue numbers a text closes, in no order.
88	97	func closingRefs(text string) []int64 {
89	98	seen := map[int64]bool{}

internal/control/control_test.go +17

		@@ -69,3 +69,20 @@ func TestBuildJobsIsAReadCommand(t *testing.T) {
69	69	t.Error("build jobs must not be SSHOnly; the web and the app need it")
70	70	}
71	71	}
	72
	73	// A merge request's dedup key must not collide with a commit sha. It did:
	74	// a bare "#N" in a commit message recorded (issue, sha) first, and the
	75	// description's "Closes #N" then found the key taken and silently gave
	76	// up. That is how krz/gitbay-ios#8 stayed open after its own MR merged.
	77	func TestMRDedupKeyCannotCollideWithASHA(t *testing.T) {
	78	key := mrRefKey(24)
	79	if key == "51b6a14eab49ab08e890597653fcf02f8f38f3d6" \|\| len(key) == 40 {
	80	t.Errorf("mrRefKey(24) = %q, which is shaped like a sha", key)
	81	}
	82	if key != "mr-24" {
	83	t.Errorf("mrRefKey(24) = %q, want \"mr-24\"", key)
	84	}
	85	if mrRefKey(24) == mrRefKey(25) {
	86	t.Error("different merge requests share a dedup key")
	87	}
	88	}

internal/control/mr.go +1 −1

		@@ -894,7 +894,7 @@ func runMRMerge(c *Ctx, args []string) int {
894	894	// when both name the same issue.
895	895	if mr.TargetRef == repo.DefaultBranch {
896	896	ProcessCommitMessages(c.Store, dir, repo, c.User.ID, targetSHA, newSHA)
897		ProcessMRDescription(c.Store, repo, mr, c.User.ID, newSHA)
	897	ProcessMRDescription(c.Store, repo, mr, c.User.ID)
898	898	RecordLandedCommits(c.Store, dir, repo, targetSHA, newSHA)
899	899	}
900	900	c.Store.MarkMirrorsDirty(repo.ID, "push")