Commit de43af4650
de43af4650beeb1f1e230dcd24ab5372d4ebd190
parent: e7b249b38d
Verified · cmc ci/build: success
cmc <hello@cleberg.net> · 2026-08-28T19:26:23Z
mrs: a bare #N in a commit no longer suppresses the description's close
ProcessMRDescription shared its dedup key with ProcessCommitMessages —
the merged sha. A commit mentioning "#N" in passing recorded that key
first, so the description's "Closes #N" found it taken and gave up
without closing anything. krz/gitbay-ios#8 stayed open after the merge
request that fixed it, saying "Closes #8", landed.
The key is now per merge request. Acting once was never the key's job:
a commit that closes the issue leaves it closed, and the state check
already skips it.
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") |