fix(issues): serialize issue link reindexing, keep truncated references whole and refresh links after merges
- commit
- b58599a1cdb6738071e9fefc2aae445e05443ee7
- author
- brice · · 2026-10-20 15:29 UTC
- parent
- 29c90bb
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
10 files · +252 -34
M docs/issues.md +1 -1
| @@ -44,7 +44,7 @@ A merge request and the issues of its repository are linked by `#N` in its text: | |||
| 44 | 44 | - The links are recomputed whenever the merge request changes (a note added, edited or deleted, a push to its branch, a merge), so removing a mention removes the link. | |
| 45 | 45 | - Words glued to the number (`abc#3`), `'` and `##3` are not references. Code spans are not skipped. | |
| 46 | 46 | ||
| 47 | - | Limits: the first 100 commits of the merge request, 16 KiB of each text, 100 merge requests listed per issue. Only numbers of existing issues are linked: a mention of a number that does not exist yet is not filled in later. With issues `off`, nothing is closed. | |
| 47 | + | Limits: the 100 most recent commits of the merge request, 16 KiB of each text, 100 merge requests listed per issue. Only numbers of existing issues are linked: a mention of a number that does not exist yet is not filled in later. With issues `off`, nothing is closed. | |
| 48 | 48 | ||
| 49 | 49 | ## Inbox | |
| 50 | 50 | ||
| @@ -44,7 +44,7 @@ A merge request and the issues of its repository are linked by `#N` in its text: | |||||
| 44 | - The links are recomputed whenever the merge request changes (a note added, edited or deleted, a push to its branch, a merge), so removing a mention removes the link. | 44 | - The links are recomputed whenever the merge request changes (a note added, edited or deleted, a push to its branch, a merge), so removing a mention removes the link. | ||
| 45 | - Words glued to the number (`abc#3`), `'` and `##3` are not references. Code spans are not skipped. | 45 | - Words glued to the number (`abc#3`), `'` and `##3` are not references. Code spans are not skipped. | ||
| 46 | 46 | ||||
| 47 | - | Limits: the first 100 commits of the merge request, 16 KiB of each text, 100 merge requests listed per issue. Only numbers of existing issues are linked: a mention of a number that does not exist yet is not filled in later. With issues `off`, nothing is closed. | 47 | + | Limits: the 100 most recent commits of the merge request, 16 KiB of each text, 100 merge requests listed per issue. Only numbers of existing issues are linked: a mention of a number that does not exist yet is not filled in later. With issues `off`, nothing is closed. |
| 48 | 48 | ||||
| 49 | ## Inbox | 49 | ## Inbox | ||
| 50 | 50 | ||||
M internal/adapters/kvstore/issue_links.go +18 -6
| @@ -57,8 +57,10 @@ func (l *IssueLinks) ReplaceLinks(ctx context.Context, repo domain.RepoName, mr | |||
| 57 | 57 | return nil, nil, domain.ErrInvalid | |
| 58 | 58 | } | |
| 59 | 59 | } | |
| 60 | + | issueSet, closeSet := toSet(issues), toSet(closes) | |
| 61 | + | oldIssues, oldCloses := toSet(old.GetIssues()), toSet(old.GetCloses()) | |
| 60 | 62 | for _, n := range closes { | |
| 61 | - | if !slices.Contains(issues, n) { | |
| 63 | + | if _, ok := issueSet[n]; !ok { | |
| 62 | 64 | return nil, nil, domain.ErrInvalid | |
| 63 | 65 | } | |
| 64 | 66 | } | |
| @@ -72,18 +74,20 @@ func (l *IssueLinks) ReplaceLinks(ctx context.Context, repo domain.RepoName, mr | |||
| 72 | 74 | b.RequireAbsent(key) | |
| 73 | 75 | } | |
| 74 | 76 | for _, n := range old.GetIssues() { | |
| 75 | - | wasClosing := slices.Contains(old.GetCloses(), n) | |
| 76 | - | if !slices.Contains(issues, n) { | |
| 77 | + | _, wasClosing := oldCloses[n] | |
| 78 | + | if _, ok := issueSet[n]; !ok { | |
| 77 | 79 | b.Delete(issueRefKey(repo, n, mr)) | |
| 78 | 80 | removed = append(removed, domain.IssueLink{Issue: n, MergeRequest: mr, Closes: wasClosing}) | |
| 79 | - | } else if wasClosing && !slices.Contains(closes, n) { | |
| 81 | + | } else if _, still := closeSet[n]; wasClosing && !still { | |
| 80 | 82 | removed = append(removed, domain.IssueLink{Issue: n, MergeRequest: mr, Closes: true}) | |
| 81 | 83 | } | |
| 82 | 84 | } | |
| 83 | 85 | for _, n := range issues { | |
| 84 | - | closing := slices.Contains(closes, n) | |
| 86 | + | _, closing := closeSet[n] | |
| 85 | 87 | b.Put(issueRefKey(repo, n, mr), mustMarshal(&storagepb.IssueLink{Closes: closing})) | |
| 86 | - | if !slices.Contains(old.GetIssues(), n) || closing && !slices.Contains(old.GetCloses(), n) { | |
| 88 | + | _, was := oldIssues[n] | |
| 89 | + | _, wasClosing := oldCloses[n] | |
| 90 | + | if !was || closing && !wasClosing { | |
| 87 | 91 | added = append(added, domain.IssueLink{Issue: n, MergeRequest: mr, Closes: closing}) | |
| 88 | 92 | } | |
| 89 | 93 | } | |
| @@ -137,3 +141,11 @@ func (l *IssueLinks) LinksOfMergeRequest(ctx context.Context, repo domain.RepoNa | |||
| 137 | 141 | } | |
| 138 | 142 | return result, nil | |
| 139 | 143 | } | |
| 144 | + | ||
| 145 | + | func toSet(numbers []int64) map[int64]struct{} { | |
| 146 | + | set := make(map[int64]struct{}, len(numbers)) | |
| 147 | + | for _, n := range numbers { | |
| 148 | + | set[n] = struct{}{} | |
| 149 | + | } | |
| 150 | + | return set | |
| 151 | + | } | |
| @@ -57,8 +57,10 @@ func (l *IssueLinks) ReplaceLinks(ctx context.Context, repo domain.RepoName, mr | |||||
| 57 | return nil, nil, domain.ErrInvalid | 57 | return nil, nil, domain.ErrInvalid | ||
| 58 | } | 58 | } | ||
| 59 | } | 59 | } | ||
| 60 | + | issueSet, closeSet := toSet(issues), toSet(closes) | |||
| 61 | + | oldIssues, oldCloses := toSet(old.GetIssues()), toSet(old.GetCloses()) | |||
| 60 | for _, n := range closes { | 62 | for _, n := range closes { | ||
| 61 | - | if !slices.Contains(issues, n) { | 63 | + | if _, ok := issueSet[n]; !ok { |
| 62 | return nil, nil, domain.ErrInvalid | 64 | return nil, nil, domain.ErrInvalid | ||
| 63 | } | 65 | } | ||
| 64 | } | 66 | } | ||
| @@ -72,18 +74,20 @@ func (l *IssueLinks) ReplaceLinks(ctx context.Context, repo domain.RepoName, mr | |||||
| 72 | b.RequireAbsent(key) | 74 | b.RequireAbsent(key) | ||
| 73 | } | 75 | } | ||
| 74 | for _, n := range old.GetIssues() { | 76 | for _, n := range old.GetIssues() { | ||
| 75 | - | wasClosing := slices.Contains(old.GetCloses(), n) | 77 | + | _, wasClosing := oldCloses[n] |
| 76 | - | if !slices.Contains(issues, n) { | 78 | + | if _, ok := issueSet[n]; !ok { |
| 77 | b.Delete(issueRefKey(repo, n, mr)) | 79 | b.Delete(issueRefKey(repo, n, mr)) | ||
| 78 | removed = append(removed, domain.IssueLink{Issue: n, MergeRequest: mr, Closes: wasClosing}) | 80 | removed = append(removed, domain.IssueLink{Issue: n, MergeRequest: mr, Closes: wasClosing}) | ||
| 79 | - | } else if wasClosing && !slices.Contains(closes, n) { | 81 | + | } else if _, still := closeSet[n]; wasClosing && !still { |
| 80 | removed = append(removed, domain.IssueLink{Issue: n, MergeRequest: mr, Closes: true}) | 82 | removed = append(removed, domain.IssueLink{Issue: n, MergeRequest: mr, Closes: true}) | ||
| 81 | } | 83 | } | ||
| 82 | } | 84 | } | ||
| 83 | for _, n := range issues { | 85 | for _, n := range issues { | ||
| 84 | - | closing := slices.Contains(closes, n) | 86 | + | _, closing := closeSet[n] |
| 85 | b.Put(issueRefKey(repo, n, mr), mustMarshal(&storagepb.IssueLink{Closes: closing})) | 87 | b.Put(issueRefKey(repo, n, mr), mustMarshal(&storagepb.IssueLink{Closes: closing})) | ||
| 86 | - | if !slices.Contains(old.GetIssues(), n) || closing && !slices.Contains(old.GetCloses(), n) { | 88 | + | _, was := oldIssues[n] |
| 89 | + | _, wasClosing := oldCloses[n] | |||
| 90 | + | if !was || closing && !wasClosing { | |||
| 87 | added = append(added, domain.IssueLink{Issue: n, MergeRequest: mr, Closes: closing}) | 91 | added = append(added, domain.IssueLink{Issue: n, MergeRequest: mr, Closes: closing}) | ||
| 88 | } | 92 | } | ||
| 89 | } | 93 | } | ||
| @@ -137,3 +141,11 @@ func (l *IssueLinks) LinksOfMergeRequest(ctx context.Context, repo domain.RepoNa | |||||
| 137 | } | 141 | } | ||
| 138 | return result, nil | 142 | return result, nil | ||
| 139 | } | 143 | } | ||
| 144 | + | ||||
| 145 | + | func toSet(numbers []int64) map[int64]struct{} { | |||
| 146 | + | set := make(map[int64]struct{}, len(numbers)) | |||
| 147 | + | for _, n := range numbers { | |||
| 148 | + | set[n] = struct{}{} | |||
| 149 | + | } | |||
| 150 | + | return set | |||
| 151 | + | } | |||
M internal/adapters/web/issues.go +8 -7
| @@ -101,7 +101,7 @@ const maxNumberLookups = 50 | |||
| 101 | 101 | // numberResolver resolves #N to the page of an issue or a merge request of | |
| 102 | 102 | // the repository. It is meant for one render: answers are memoized and at most | |
| 103 | 103 | // maxNumberLookups numbers are asked, the others do not resolve. | |
| 104 | - | func (s *Server) numberResolver(ctx context.Context, repo string) func(int64) (string, bool) { | |
| 104 | + | func (s *Server) numberResolver(ctx context.Context, repo string) func(int64) (string, domain.NumberKind, bool) { | |
| 105 | 105 | if s.cfg.IssueNumbers == nil { | |
| 106 | 106 | return nil | |
| 107 | 107 | } | |
| @@ -111,28 +111,29 @@ func (s *Server) numberResolver(ctx context.Context, repo string) func(int64) (s | |||
| 111 | 111 | } | |
| 112 | 112 | type answer struct { | |
| 113 | 113 | href string | |
| 114 | + | kind domain.NumberKind | |
| 114 | 115 | ok bool | |
| 115 | 116 | } | |
| 116 | 117 | seen := map[int64]answer{} | |
| 117 | - | return func(n int64) (string, bool) { | |
| 118 | + | return func(n int64) (string, domain.NumberKind, bool) { | |
| 118 | 119 | if a, ok := seen[n]; ok { | |
| 119 | - | return a.href, a.ok | |
| 120 | + | return a.href, a.kind, a.ok | |
| 120 | 121 | } | |
| 121 | 122 | if len(seen) >= maxNumberLookups { | |
| 122 | - | return "", false | |
| 123 | + | return "", domain.NumberNone, false | |
| 123 | 124 | } | |
| 124 | 125 | var a answer | |
| 125 | 126 | kind, err := s.cfg.IssueNumbers.Kind(ctx, principal(ctx), name, n) | |
| 126 | 127 | if err == nil { | |
| 127 | 128 | switch kind { | |
| 128 | 129 | case domain.NumberIssue: | |
| 129 | - | a = answer{issueHref(repo, n), true} | |
| 130 | + | a = answer{issueHref(repo, n), kind, true} | |
| 130 | 131 | case domain.NumberMergeRequest: | |
| 131 | - | a = answer{mrHref(repo, n), true} | |
| 132 | + | a = answer{mrHref(repo, n), kind, true} | |
| 132 | 133 | } | |
| 133 | 134 | } | |
| 134 | 135 | seen[n] = a | |
| 135 | - | return a.href, a.ok | |
| 136 | + | return a.href, a.kind, a.ok | |
| 136 | 137 | } | |
| 137 | 138 | } | |
| 138 | 139 | ||
| @@ -101,7 +101,7 @@ const maxNumberLookups = 50 | |||||
| 101 | // numberResolver resolves #N to the page of an issue or a merge request of | 101 | // numberResolver resolves #N to the page of an issue or a merge request of | ||
| 102 | // the repository. It is meant for one render: answers are memoized and at most | 102 | // the repository. It is meant for one render: answers are memoized and at most | ||
| 103 | // maxNumberLookups numbers are asked, the others do not resolve. | 103 | // maxNumberLookups numbers are asked, the others do not resolve. | ||
| 104 | - | func (s *Server) numberResolver(ctx context.Context, repo string) func(int64) (string, bool) { | 104 | + | func (s *Server) numberResolver(ctx context.Context, repo string) func(int64) (string, domain.NumberKind, bool) { |
| 105 | if s.cfg.IssueNumbers == nil { | 105 | if s.cfg.IssueNumbers == nil { | ||
| 106 | return nil | 106 | return nil | ||
| 107 | } | 107 | } | ||
| @@ -111,28 +111,29 @@ func (s *Server) numberResolver(ctx context.Context, repo string) func(int64) (s | |||||
| 111 | } | 111 | } | ||
| 112 | type answer struct { | 112 | type answer struct { | ||
| 113 | href string | 113 | href string | ||
| 114 | + | kind domain.NumberKind | |||
| 114 | ok bool | 115 | ok bool | ||
| 115 | } | 116 | } | ||
| 116 | seen := map[int64]answer{} | 117 | seen := map[int64]answer{} | ||
| 117 | - | return func(n int64) (string, bool) { | 118 | + | return func(n int64) (string, domain.NumberKind, bool) { |
| 118 | if a, ok := seen[n]; ok { | 119 | if a, ok := seen[n]; ok { | ||
| 119 | - | return a.href, a.ok | 120 | + | return a.href, a.kind, a.ok |
| 120 | } | 121 | } | ||
| 121 | if len(seen) >= maxNumberLookups { | 122 | if len(seen) >= maxNumberLookups { | ||
| 122 | - | return "", false | 123 | + | return "", domain.NumberNone, false |
| 123 | } | 124 | } | ||
| 124 | var a answer | 125 | var a answer | ||
| 125 | kind, err := s.cfg.IssueNumbers.Kind(ctx, principal(ctx), name, n) | 126 | kind, err := s.cfg.IssueNumbers.Kind(ctx, principal(ctx), name, n) | ||
| 126 | if err == nil { | 127 | if err == nil { | ||
| 127 | switch kind { | 128 | switch kind { | ||
| 128 | case domain.NumberIssue: | 129 | case domain.NumberIssue: | ||
| 129 | - | a = answer{issueHref(repo, n), true} | 130 | + | a = answer{issueHref(repo, n), kind, true} |
| 130 | case domain.NumberMergeRequest: | 131 | case domain.NumberMergeRequest: | ||
| 131 | - | a = answer{mrHref(repo, n), true} | 132 | + | a = answer{mrHref(repo, n), kind, true} |
| 132 | } | 133 | } | ||
| 133 | } | 134 | } | ||
| 134 | seen[n] = a | 135 | seen[n] = a | ||
| 135 | - | return a.href, a.ok | 136 | + | return a.href, a.kind, a.ok |
| 136 | } | 137 | } | ||
| 137 | } | 138 | } | ||
| 138 | 139 | ||||
M internal/adapters/web/markdown.go +6 -5
| @@ -11,6 +11,7 @@ import ( | |||
| 11 | 11 | "strconv" | |
| 12 | 12 | "strings" | |
| 13 | 13 | ||
| 14 | + | "bnema.dev/ermite/internal/domain" | |
| 14 | 15 | "github.com/yuin/goldmark" | |
| 15 | 16 | "github.com/yuin/goldmark/ast" | |
| 16 | 17 | "github.com/yuin/goldmark/extension" | |
| @@ -76,7 +77,7 @@ func (m *markdown) renderNote(source []byte, repo, ref, idPrefix string, refs re | |||
| 76 | 77 | ||
| 77 | 78 | // renderIssue renders the trusted body of an issue: the policy of render, plus | |
| 78 | 79 | // links to issues and merge requests. | |
| 79 | - | func (m *markdown) renderIssue(source []byte, repo, ref, idPrefix string, numbers func(int64) (string, bool)) (template.HTML, error) { | |
| 80 | + | func (m *markdown) renderIssue(source []byte, repo, ref, idPrefix string, numbers func(int64) (string, domain.NumberKind, bool)) (template.HTML, error) { | |
| 80 | 81 | h, _, err := m.renderPolicy(source, linkPolicy{repo: repo, ref: ref, idPrefix: idPrefix}, refResolver{numbers: numbers}) | |
| 81 | 82 | return h, err | |
| 82 | 83 | } | |
| @@ -87,7 +88,7 @@ type refResolver struct { | |||
| 87 | 88 | notes func(int64) bool | |
| 88 | 89 | // numbers gives the page of the issue or merge request #N, when it exists | |
| 89 | 90 | // (nil: no #N links). The href is built by the server, never from the text. | |
| 90 | - | numbers func(int64) (href string, ok bool) | |
| 91 | + | numbers func(int64) (href string, kind domain.NumberKind, ok bool) | |
| 91 | 92 | } | |
| 92 | 93 | ||
| 93 | 94 | // renderUntrusted renders Markdown written by an anonymous visitor (an issue | |
| @@ -97,7 +98,7 @@ type refResolver struct { | |||
| 97 | 98 | // rel="nofollow noopener noreferrer ugc"; relative links, #fragments and | |
| 98 | 99 | // every other scheme lose their link and keep the text; | |
| 99 | 100 | // - nothing points into the repository. | |
| 100 | - | func (m *markdown) renderUntrusted(source []byte, idPrefix string, numbers func(int64) (string, bool)) (template.HTML, error) { | |
| 101 | + | func (m *markdown) renderUntrusted(source []byte, idPrefix string, numbers func(int64) (string, domain.NumberKind, bool)) (template.HTML, error) { | |
| 101 | 102 | h, _, err := m.renderPolicy(source, linkPolicy{idPrefix: idPrefix, untrusted: true}, refResolver{numbers: numbers}) | |
| 102 | 103 | return h, err | |
| 103 | 104 | } | |
| @@ -457,13 +458,13 @@ func references(t *ast.Text, source []byte, res refResolver) { | |||
| 457 | 458 | link.Destination = []byte("#c" + id) | |
| 458 | 459 | link.Title = []byte("note " + id) | |
| 459 | 460 | } else { | |
| 460 | - | href, ok := res.numbers(n) | |
| 461 | + | href, kind, ok := res.numbers(n) | |
| 461 | 462 | if !ok { | |
| 462 | 463 | continue | |
| 463 | 464 | } | |
| 464 | 465 | link.Destination = []byte(href) | |
| 465 | 466 | title := "issue " | |
| 466 | - | if strings.Contains(href, "/merge-requests/") { | |
| 467 | + | if kind == domain.NumberMergeRequest { | |
| 467 | 468 | title = "merge request " | |
| 468 | 469 | } | |
| 469 | 470 | link.Title = []byte(title + id) | |
| @@ -11,6 +11,7 @@ import ( | |||||
| 11 | "strconv" | 11 | "strconv" | ||
| 12 | "strings" | 12 | "strings" | ||
| 13 | 13 | ||||
| 14 | + | "bnema.dev/ermite/internal/domain" | |||
| 14 | "github.com/yuin/goldmark" | 15 | "github.com/yuin/goldmark" | ||
| 15 | "github.com/yuin/goldmark/ast" | 16 | "github.com/yuin/goldmark/ast" | ||
| 16 | "github.com/yuin/goldmark/extension" | 17 | "github.com/yuin/goldmark/extension" | ||
| @@ -76,7 +77,7 @@ func (m *markdown) renderNote(source []byte, repo, ref, idPrefix string, refs re | |||||
| 76 | 77 | ||||
| 77 | // renderIssue renders the trusted body of an issue: the policy of render, plus | 78 | // renderIssue renders the trusted body of an issue: the policy of render, plus | ||
| 78 | // links to issues and merge requests. | 79 | // links to issues and merge requests. | ||
| 79 | - | func (m *markdown) renderIssue(source []byte, repo, ref, idPrefix string, numbers func(int64) (string, bool)) (template.HTML, error) { | 80 | + | func (m *markdown) renderIssue(source []byte, repo, ref, idPrefix string, numbers func(int64) (string, domain.NumberKind, bool)) (template.HTML, error) { |
| 80 | h, _, err := m.renderPolicy(source, linkPolicy{repo: repo, ref: ref, idPrefix: idPrefix}, refResolver{numbers: numbers}) | 81 | h, _, err := m.renderPolicy(source, linkPolicy{repo: repo, ref: ref, idPrefix: idPrefix}, refResolver{numbers: numbers}) | ||
| 81 | return h, err | 82 | return h, err | ||
| 82 | } | 83 | } | ||
| @@ -87,7 +88,7 @@ type refResolver struct { | |||||
| 87 | notes func(int64) bool | 88 | notes func(int64) bool | ||
| 88 | // numbers gives the page of the issue or merge request #N, when it exists | 89 | // numbers gives the page of the issue or merge request #N, when it exists | ||
| 89 | // (nil: no #N links). The href is built by the server, never from the text. | 90 | // (nil: no #N links). The href is built by the server, never from the text. | ||
| 90 | - | numbers func(int64) (href string, ok bool) | 91 | + | numbers func(int64) (href string, kind domain.NumberKind, ok bool) |
| 91 | } | 92 | } | ||
| 92 | 93 | ||||
| 93 | // renderUntrusted renders Markdown written by an anonymous visitor (an issue | 94 | // renderUntrusted renders Markdown written by an anonymous visitor (an issue | ||
| @@ -97,7 +98,7 @@ type refResolver struct { | |||||
| 97 | // rel="nofollow noopener noreferrer ugc"; relative links, #fragments and | 98 | // rel="nofollow noopener noreferrer ugc"; relative links, #fragments and | ||
| 98 | // every other scheme lose their link and keep the text; | 99 | // every other scheme lose their link and keep the text; | ||
| 99 | // - nothing points into the repository. | 100 | // - nothing points into the repository. | ||
| 100 | - | func (m *markdown) renderUntrusted(source []byte, idPrefix string, numbers func(int64) (string, bool)) (template.HTML, error) { | 101 | + | func (m *markdown) renderUntrusted(source []byte, idPrefix string, numbers func(int64) (string, domain.NumberKind, bool)) (template.HTML, error) { |
| 101 | h, _, err := m.renderPolicy(source, linkPolicy{idPrefix: idPrefix, untrusted: true}, refResolver{numbers: numbers}) | 102 | h, _, err := m.renderPolicy(source, linkPolicy{idPrefix: idPrefix, untrusted: true}, refResolver{numbers: numbers}) | ||
| 102 | return h, err | 103 | return h, err | ||
| 103 | } | 104 | } | ||
| @@ -457,13 +458,13 @@ func references(t *ast.Text, source []byte, res refResolver) { | |||||
| 457 | link.Destination = []byte("#c" + id) | 458 | link.Destination = []byte("#c" + id) | ||
| 458 | link.Title = []byte("note " + id) | 459 | link.Title = []byte("note " + id) | ||
| 459 | } else { | 460 | } else { | ||
| 460 | - | href, ok := res.numbers(n) | 461 | + | href, kind, ok := res.numbers(n) |
| 461 | if !ok { | 462 | if !ok { | ||
| 462 | continue | 463 | continue | ||
| 463 | } | 464 | } | ||
| 464 | link.Destination = []byte(href) | 465 | link.Destination = []byte(href) | ||
| 465 | title := "issue " | 466 | title := "issue " | ||
| 466 | - | if strings.Contains(href, "/merge-requests/") { | 467 | + | if kind == domain.NumberMergeRequest { |
| 467 | title = "merge request " | 468 | title = "merge request " | ||
| 468 | } | 469 | } | ||
| 469 | link.Title = []byte(title + id) | 470 | link.Title = []byte(title + id) | ||
M internal/adapters/web/markdown_test.go +18 -7
| @@ -238,14 +238,14 @@ func TestNoteMarkdown(t *testing.T) { | |||
| 238 | 238 | ||
| 239 | 239 | func TestNumberReferences(t *testing.T) { | |
| 240 | 240 | t.Parallel() | |
| 241 | - | numbers := func(n int64) (string, bool) { | |
| 241 | + | numbers := func(n int64) (string, domain.NumberKind, bool) { | |
| 242 | 242 | switch n { | |
| 243 | 243 | case 3: | |
| 244 | - | return "/demo/issues/3", true | |
| 244 | + | return "/demo/issues/3", domain.NumberIssue, true | |
| 245 | 245 | case 4: | |
| 246 | - | return "/demo/merge-requests/4", true | |
| 246 | + | return "/demo/merge-requests/4", domain.NumberMergeRequest, true | |
| 247 | 247 | } | |
| 248 | - | return "", false | |
| 248 | + | return "", domain.NumberNone, false | |
| 249 | 249 | } | |
| 250 | 250 | tests := []struct { | |
| 251 | 251 | source string | |
| @@ -286,7 +286,7 @@ func TestNumberAndNoteReferencesTogether(t *testing.T) { | |||
| 286 | 286 | t.Parallel() | |
| 287 | 287 | h, err := newMarkdown().renderNote([]byte("see ^2 and #3."), "demo", "main", "c1-", refResolver{ | |
| 288 | 288 | notes: func(n int64) bool { return n == 2 }, | |
| 289 | - | numbers: func(n int64) (string, bool) { return "/demo/issues/3", n == 3 }, | |
| 289 | + | numbers: func(n int64) (string, domain.NumberKind, bool) { return "/demo/issues/3", domain.NumberIssue, n == 3 }, | |
| 290 | 290 | }) | |
| 291 | 291 | require.NoError(t, err) | |
| 292 | 292 | assert.Contains(t, string(h), `href="#c2"`) | |
| @@ -302,9 +302,20 @@ func TestNumberResolverCapsLookups(t *testing.T) { | |||
| 302 | 302 | resolve := srv.numberResolver(t.Context(), "demo") | |
| 303 | 303 | require.NotNil(t, resolve) | |
| 304 | 304 | for n := int64(1); n <= maxNumberLookups+10; n++ { | |
| 305 | - | _, ok := resolve(n) | |
| 305 | + | _, _, ok := resolve(n) | |
| 306 | 306 | assert.Equal(t, n <= maxNumberLookups, ok, n) | |
| 307 | 307 | } | |
| 308 | - | _, ok := resolve(1) // memoized | |
| 308 | + | _, _, ok := resolve(1) // memoized | |
| 309 | 309 | assert.True(t, ok) | |
| 310 | 310 | } | |
| 311 | + | ||
| 312 | + | func TestNumberReferenceTitleFromKindNotHref(t *testing.T) { | |
| 313 | + | t.Parallel() | |
| 314 | + | numbers := func(n int64) (string, domain.NumberKind, bool) { | |
| 315 | + | return "/owner/merge-requests/issues/3", domain.NumberIssue, true | |
| 316 | + | } | |
| 317 | + | h, err := newMarkdown().renderIssue([]byte("see #3"), "merge-requests", "main", "i-", numbers) | |
| 318 | + | require.NoError(t, err) | |
| 319 | + | assert.Contains(t, string(h), `title="issue 3"`) | |
| 320 | + | assert.NotContains(t, string(h), "merge request 3") | |
| 321 | + | } | |
| @@ -238,14 +238,14 @@ func TestNoteMarkdown(t *testing.T) { | |||||
| 238 | 238 | ||||
| 239 | func TestNumberReferences(t *testing.T) { | 239 | func TestNumberReferences(t *testing.T) { | ||
| 240 | t.Parallel() | 240 | t.Parallel() | ||
| 241 | - | numbers := func(n int64) (string, bool) { | 241 | + | numbers := func(n int64) (string, domain.NumberKind, bool) { |
| 242 | switch n { | 242 | switch n { | ||
| 243 | case 3: | 243 | case 3: | ||
| 244 | - | return "/demo/issues/3", true | 244 | + | return "/demo/issues/3", domain.NumberIssue, true |
| 245 | case 4: | 245 | case 4: | ||
| 246 | - | return "/demo/merge-requests/4", true | 246 | + | return "/demo/merge-requests/4", domain.NumberMergeRequest, true |
| 247 | } | 247 | } | ||
| 248 | - | return "", false | 248 | + | return "", domain.NumberNone, false |
| 249 | } | 249 | } | ||
| 250 | tests := []struct { | 250 | tests := []struct { | ||
| 251 | source string | 251 | source string | ||
| @@ -286,7 +286,7 @@ func TestNumberAndNoteReferencesTogether(t *testing.T) { | |||||
| 286 | t.Parallel() | 286 | t.Parallel() | ||
| 287 | h, err := newMarkdown().renderNote([]byte("see ^2 and #3."), "demo", "main", "c1-", refResolver{ | 287 | h, err := newMarkdown().renderNote([]byte("see ^2 and #3."), "demo", "main", "c1-", refResolver{ | ||
| 288 | notes: func(n int64) bool { return n == 2 }, | 288 | notes: func(n int64) bool { return n == 2 }, | ||
| 289 | - | numbers: func(n int64) (string, bool) { return "/demo/issues/3", n == 3 }, | 289 | + | numbers: func(n int64) (string, domain.NumberKind, bool) { return "/demo/issues/3", domain.NumberIssue, n == 3 }, |
| 290 | }) | 290 | }) | ||
| 291 | require.NoError(t, err) | 291 | require.NoError(t, err) | ||
| 292 | assert.Contains(t, string(h), `href="#c2"`) | 292 | assert.Contains(t, string(h), `href="#c2"`) | ||
| @@ -302,9 +302,20 @@ func TestNumberResolverCapsLookups(t *testing.T) { | |||||
| 302 | resolve := srv.numberResolver(t.Context(), "demo") | 302 | resolve := srv.numberResolver(t.Context(), "demo") | ||
| 303 | require.NotNil(t, resolve) | 303 | require.NotNil(t, resolve) | ||
| 304 | for n := int64(1); n <= maxNumberLookups+10; n++ { | 304 | for n := int64(1); n <= maxNumberLookups+10; n++ { | ||
| 305 | - | _, ok := resolve(n) | 305 | + | _, _, ok := resolve(n) |
| 306 | assert.Equal(t, n <= maxNumberLookups, ok, n) | 306 | assert.Equal(t, n <= maxNumberLookups, ok, n) | ||
| 307 | } | 307 | } | ||
| 308 | - | _, ok := resolve(1) // memoized | 308 | + | _, _, ok := resolve(1) // memoized |
| 309 | assert.True(t, ok) | 309 | assert.True(t, ok) | ||
| 310 | } | 310 | } | ||
| 311 | + | ||||
| 312 | + | func TestNumberReferenceTitleFromKindNotHref(t *testing.T) { | |||
| 313 | + | t.Parallel() | |||
| 314 | + | numbers := func(n int64) (string, domain.NumberKind, bool) { | |||
| 315 | + | return "/owner/merge-requests/issues/3", domain.NumberIssue, true | |||
| 316 | + | } | |||
| 317 | + | h, err := newMarkdown().renderIssue([]byte("see #3"), "merge-requests", "main", "i-", numbers) | |||
| 318 | + | require.NoError(t, err) | |||
| 319 | + | assert.Contains(t, string(h), `title="issue 3"`) | |||
| 320 | + | assert.NotContains(t, string(h), "merge request 3") | |||
| 321 | + | } | |||
M internal/domain/reference.go +14 -4
| @@ -6,8 +6,6 @@ import ( | |||
| 6 | 6 | "strconv" | |
| 7 | 7 | ) | |
| 8 | 8 | ||
| 9 | - | const maxReferenceDigits = 10 | |
| 10 | - | ||
| 11 | 9 | // MaxReferenceTextBytes bounds each title, description, note and commit message scanned. | |
| 12 | 10 | const MaxReferenceTextBytes = 16 << 10 | |
| 13 | 11 | ||
| @@ -73,13 +71,25 @@ type MergeRequestReferences struct { | |||
| 73 | 71 | ||
| 74 | 72 | // ScanMergeRequest scans titles, descriptions, notes and commit messages for | |
| 75 | 73 | // references. Notes can mention issues but cannot close them. Each individual | |
| 76 | - | // text is truncated to MaxReferenceTextBytes before scanning; no Markdown or | |
| 74 | + | // text is truncated to MaxReferenceTextBytes, without a trailing partial token, before scanning; no Markdown or | |
| 77 | 75 | // cross-repository interpretation is performed. | |
| 78 | 76 | func ScanMergeRequest(title, description string, notes, commitMessages []string) MergeRequestReferences { | |
| 79 | 77 | mentions, closes := make(map[int64]struct{}), make(map[int64]struct{}) | |
| 80 | 78 | scan := func(text string, closing bool) { | |
| 81 | 79 | if len(text) > MaxReferenceTextBytes { | |
| 82 | - | text = text[:MaxReferenceTextBytes] | |
| 80 | + | // Drop the trailing partial token: back off to the last ASCII | |
| 81 | + | // whitespace at or before the limit. | |
| 82 | + | cut := -1 | |
| 83 | + | for i := MaxReferenceTextBytes; i >= 0; i-- { | |
| 84 | + | if i < len(text) && (text[i] == ' ' || text[i] == '\t' || text[i] == '\n' || text[i] == '\r') { | |
| 85 | + | cut = i | |
| 86 | + | break | |
| 87 | + | } | |
| 88 | + | } | |
| 89 | + | if cut < 0 { | |
| 90 | + | return | |
| 91 | + | } | |
| 92 | + | text = text[:cut] | |
| 83 | 93 | } | |
| 84 | 94 | for _, n := range FindReferences(text) { | |
| 85 | 95 | mentions[n] = struct{}{} | |
| @@ -6,8 +6,6 @@ import ( | |||||
| 6 | "strconv" | 6 | "strconv" | ||
| 7 | ) | 7 | ) | ||
| 8 | 8 | ||||
| 9 | - | const maxReferenceDigits = 10 | |||
| 10 | - | ||||
| 11 | // MaxReferenceTextBytes bounds each title, description, note and commit message scanned. | 9 | // MaxReferenceTextBytes bounds each title, description, note and commit message scanned. | ||
| 12 | const MaxReferenceTextBytes = 16 << 10 | 10 | const MaxReferenceTextBytes = 16 << 10 | ||
| 13 | 11 | ||||
| @@ -73,13 +71,25 @@ type MergeRequestReferences struct { | |||||
| 73 | 71 | ||||
| 74 | // ScanMergeRequest scans titles, descriptions, notes and commit messages for | 72 | // ScanMergeRequest scans titles, descriptions, notes and commit messages for | ||
| 75 | // references. Notes can mention issues but cannot close them. Each individual | 73 | // references. Notes can mention issues but cannot close them. Each individual | ||
| 76 | - | // text is truncated to MaxReferenceTextBytes before scanning; no Markdown or | 74 | + | // text is truncated to MaxReferenceTextBytes, without a trailing partial token, before scanning; no Markdown or |
| 77 | // cross-repository interpretation is performed. | 75 | // cross-repository interpretation is performed. | ||
| 78 | func ScanMergeRequest(title, description string, notes, commitMessages []string) MergeRequestReferences { | 76 | func ScanMergeRequest(title, description string, notes, commitMessages []string) MergeRequestReferences { | ||
| 79 | mentions, closes := make(map[int64]struct{}), make(map[int64]struct{}) | 77 | mentions, closes := make(map[int64]struct{}), make(map[int64]struct{}) | ||
| 80 | scan := func(text string, closing bool) { | 78 | scan := func(text string, closing bool) { | ||
| 81 | if len(text) > MaxReferenceTextBytes { | 79 | if len(text) > MaxReferenceTextBytes { | ||
| 82 | - | text = text[:MaxReferenceTextBytes] | 80 | + | // Drop the trailing partial token: back off to the last ASCII |
| 81 | + | // whitespace at or before the limit. | |||
| 82 | + | cut := -1 | |||
| 83 | + | for i := MaxReferenceTextBytes; i >= 0; i-- { | |||
| 84 | + | if i < len(text) && (text[i] == ' ' || text[i] == '\t' || text[i] == '\n' || text[i] == '\r') { | |||
| 85 | + | cut = i | |||
| 86 | + | break | |||
| 87 | + | } | |||
| 88 | + | } | |||
| 89 | + | if cut < 0 { | |||
| 90 | + | return | |||
| 91 | + | } | |||
| 92 | + | text = text[:cut] | |||
| 83 | } | 93 | } | ||
| 84 | for _, n := range FindReferences(text) { | 94 | for _, n := range FindReferences(text) { | ||
| 85 | mentions[n] = struct{}{} | 95 | mentions[n] = struct{}{} | ||
M internal/domain/reference_test.go +19 -0
| @@ -57,3 +57,22 @@ func TestScanMergeRequest(t *testing.T) { | |||
| 57 | 57 | } | |
| 58 | 58 | } | |
| 59 | 59 | } | |
| 60 | + | ||
| 61 | + | func TestScanMergeRequestTruncationDropsPartialToken(t *testing.T) { | |
| 62 | + | prefix := strings.Repeat(" ", MaxReferenceTextBytes-len("fixes #123")) | |
| 63 | + | got := ScanMergeRequest("", "", nil, []string{prefix + "fixes #12345"}) | |
| 64 | + | if len(got.Mentions) != 0 || len(got.Closes) != 0 { | |
| 65 | + | t.Errorf("split number reported: %+v", got) | |
| 66 | + | } | |
| 67 | + | // Fully before the limit, followed by a split token. | |
| 68 | + | full := "fixes #7 " + strings.Repeat("a", MaxReferenceTextBytes) | |
| 69 | + | got = ScanMergeRequest("", "", nil, []string{full}) | |
| 70 | + | if !reflect.DeepEqual(got, MergeRequestReferences{Mentions: []int64{7}, Closes: []int64{7}}) { | |
| 71 | + | t.Errorf("got %+v", got) | |
| 72 | + | } | |
| 73 | + | // No whitespace at all: nothing scanned. | |
| 74 | + | got = ScanMergeRequest("", "", nil, []string{strings.Repeat("a", MaxReferenceTextBytes-3) + " #1" + "2"}) | |
| 75 | + | if len(got.Mentions) != 0 { | |
| 76 | + | t.Errorf("got %+v", got) | |
| 77 | + | } | |
| 78 | + | } | |
| @@ -57,3 +57,22 @@ func TestScanMergeRequest(t *testing.T) { | |||||
| 57 | } | 57 | } | ||
| 58 | } | 58 | } | ||
| 59 | } | 59 | } | ||
| 60 | + | ||||
| 61 | + | func TestScanMergeRequestTruncationDropsPartialToken(t *testing.T) { | |||
| 62 | + | prefix := strings.Repeat(" ", MaxReferenceTextBytes-len("fixes #123")) | |||
| 63 | + | got := ScanMergeRequest("", "", nil, []string{prefix + "fixes #12345"}) | |||
| 64 | + | if len(got.Mentions) != 0 || len(got.Closes) != 0 { | |||
| 65 | + | t.Errorf("split number reported: %+v", got) | |||
| 66 | + | } | |||
| 67 | + | // Fully before the limit, followed by a split token. | |||
| 68 | + | full := "fixes #7 " + strings.Repeat("a", MaxReferenceTextBytes) | |||
| 69 | + | got = ScanMergeRequest("", "", nil, []string{full}) | |||
| 70 | + | if !reflect.DeepEqual(got, MergeRequestReferences{Mentions: []int64{7}, Closes: []int64{7}}) { | |||
| 71 | + | t.Errorf("got %+v", got) | |||
| 72 | + | } | |||
| 73 | + | // No whitespace at all: nothing scanned. | |||
| 74 | + | got = ScanMergeRequest("", "", nil, []string{strings.Repeat("a", MaxReferenceTextBytes-3) + " #1" + "2"}) | |||
| 75 | + | if len(got.Mentions) != 0 { | |||
| 76 | + | t.Errorf("got %+v", got) | |||
| 77 | + | } | |||
| 78 | + | } | |||
M internal/usecase/issue_links.go +61 -4
| @@ -7,6 +7,7 @@ import ( | |||
| 7 | 7 | "errors" | |
| 8 | 8 | "slices" | |
| 9 | 9 | "strings" | |
| 10 | + | "sync" | |
| 10 | 11 | "time" | |
| 11 | 12 | ||
| 12 | 13 | "bnema.dev/ermite/internal/domain" | |
| @@ -29,11 +30,55 @@ type IssueLinksConfig struct { | |||
| 29 | 30 | } | |
| 30 | 31 | ||
| 31 | 32 | // IssueLinks maintains the derived index and applies closing references on merge. | |
| 32 | - | type IssueLinks struct{ cfg IssueLinksConfig } | |
| 33 | + | type IssueLinks struct { | |
| 34 | + | cfg IssueLinksConfig | |
| 35 | + | mu sync.Mutex | |
| 36 | + | locks map[linkKey]*linkLock | |
| 37 | + | } | |
| 38 | + | ||
| 39 | + | type linkKey struct { | |
| 40 | + | repo domain.RepoName | |
| 41 | + | number int64 | |
| 42 | + | } | |
| 43 | + | ||
| 44 | + | type linkLock struct { | |
| 45 | + | ch chan struct{} | |
| 46 | + | refs int | |
| 47 | + | } | |
| 48 | + | ||
| 49 | + | // lockMR serializes reindexes of one merge request, waiting until ctx ends. | |
| 50 | + | // Entries are removed when no one holds or waits for them. | |
| 51 | + | func (l *IssueLinks) lockMR(ctx context.Context, repo domain.RepoName, number int64) (func(), error) { | |
| 52 | + | key := linkKey{repo, number} | |
| 53 | + | l.mu.Lock() | |
| 54 | + | e, ok := l.locks[key] | |
| 55 | + | if !ok { | |
| 56 | + | e = &linkLock{ch: make(chan struct{}, 1)} | |
| 57 | + | l.locks[key] = e | |
| 58 | + | } | |
| 59 | + | e.refs++ | |
| 60 | + | l.mu.Unlock() | |
| 61 | + | release := func() { | |
| 62 | + | l.mu.Lock() | |
| 63 | + | if e.refs--; e.refs == 0 { | |
| 64 | + | delete(l.locks, key) | |
| 65 | + | } | |
| 66 | + | l.mu.Unlock() | |
| 67 | + | } | |
| 68 | + | select { | |
| 69 | + | case e.ch <- struct{}{}: | |
| 70 | + | return func() { <-e.ch; release() }, nil | |
| 71 | + | case <-ctx.Done(): | |
| 72 | + | release() | |
| 73 | + | return nil, ctx.Err() | |
| 74 | + | } | |
| 75 | + | } | |
| 33 | 76 | ||
| 34 | 77 | var _ ports.IssueLinkTrigger = (*IssueLinks)(nil) | |
| 35 | 78 | ||
| 36 | - | func NewIssueLinks(cfg IssueLinksConfig) *IssueLinks { return &IssueLinks{cfg: cfg} } | |
| 79 | + | func NewIssueLinks(cfg IssueLinksConfig) *IssueLinks { | |
| 80 | + | return &IssueLinks{cfg: cfg, locks: map[linkKey]*linkLock{}} | |
| 81 | + | } | |
| 37 | 82 | ||
| 38 | 83 | // Reindex replaces links from the current texts and bounded commit range. | |
| 39 | 84 | func (l *IssueLinks) Reindex(ctx context.Context, repo domain.Repo, number int64) error { | |
| @@ -41,6 +86,11 @@ func (l *IssueLinks) Reindex(ctx context.Context, repo domain.Repo, number int64 | |||
| 41 | 86 | return err | |
| 42 | 87 | } | |
| 43 | 88 | func (l *IssueLinks) reindex(ctx context.Context, repo domain.Repo, number int64) ([]int64, error) { | |
| 89 | + | unlock, err := l.lockMR(ctx, repo.Name, number) | |
| 90 | + | if err != nil { | |
| 91 | + | return nil, err | |
| 92 | + | } | |
| 93 | + | defer unlock() | |
| 44 | 94 | mr, err := l.cfg.MRs.Get(ctx, repo.Name, number) | |
| 45 | 95 | if err != nil { | |
| 46 | 96 | return nil, err | |
| @@ -103,14 +153,21 @@ func (l *IssueLinks) reindex(ctx context.Context, repo domain.Repo, number int64 | |||
| 103 | 153 | } | |
| 104 | 154 | } | |
| 105 | 155 | for range maxWriteRetries { | |
| 106 | - | added, _, err := l.cfg.Links.ReplaceLinks(ctx, repo.Name, number, mentions, closes) | |
| 156 | + | added, removed, err := l.cfg.Links.ReplaceLinks(ctx, repo.Name, number, mentions, closes) | |
| 107 | 157 | if errors.Is(err, domain.ErrConflict) { | |
| 108 | 158 | continue | |
| 109 | 159 | } | |
| 110 | 160 | if err != nil { | |
| 111 | 161 | return nil, err | |
| 112 | 162 | } | |
| 113 | - | for _, link := range added { | |
| 163 | + | type pair struct{ issue, mr int64 } | |
| 164 | + | sent := map[pair]struct{}{} | |
| 165 | + | for _, link := range slices.Concat(added, removed) { | |
| 166 | + | p := pair{link.Issue, link.MergeRequest} | |
| 167 | + | if _, ok := sent[p]; ok { | |
| 168 | + | continue | |
| 169 | + | } | |
| 170 | + | sent[p] = struct{}{} | |
| 114 | 171 | l.notify(ctx, repo, link) | |
| 115 | 172 | } | |
| 116 | 173 | return closes, nil | |
| @@ -7,6 +7,7 @@ import ( | |||||
| 7 | "errors" | 7 | "errors" | ||
| 8 | "slices" | 8 | "slices" | ||
| 9 | "strings" | 9 | "strings" | ||
| 10 | + | "sync" | |||
| 10 | "time" | 11 | "time" | ||
| 11 | 12 | ||||
| 12 | "bnema.dev/ermite/internal/domain" | 13 | "bnema.dev/ermite/internal/domain" | ||
| @@ -29,11 +30,55 @@ type IssueLinksConfig struct { | |||||
| 29 | } | 30 | } | ||
| 30 | 31 | ||||
| 31 | // IssueLinks maintains the derived index and applies closing references on merge. | 32 | // IssueLinks maintains the derived index and applies closing references on merge. | ||
| 32 | - | type IssueLinks struct{ cfg IssueLinksConfig } | 33 | + | type IssueLinks struct { |
| 34 | + | cfg IssueLinksConfig | |||
| 35 | + | mu sync.Mutex | |||
| 36 | + | locks map[linkKey]*linkLock | |||
| 37 | + | } | |||
| 38 | + | ||||
| 39 | + | type linkKey struct { | |||
| 40 | + | repo domain.RepoName | |||
| 41 | + | number int64 | |||
| 42 | + | } | |||
| 43 | + | ||||
| 44 | + | type linkLock struct { | |||
| 45 | + | ch chan struct{} | |||
| 46 | + | refs int | |||
| 47 | + | } | |||
| 48 | + | ||||
| 49 | + | // lockMR serializes reindexes of one merge request, waiting until ctx ends. | |||
| 50 | + | // Entries are removed when no one holds or waits for them. | |||
| 51 | + | func (l *IssueLinks) lockMR(ctx context.Context, repo domain.RepoName, number int64) (func(), error) { | |||
| 52 | + | key := linkKey{repo, number} | |||
| 53 | + | l.mu.Lock() | |||
| 54 | + | e, ok := l.locks[key] | |||
| 55 | + | if !ok { | |||
| 56 | + | e = &linkLock{ch: make(chan struct{}, 1)} | |||
| 57 | + | l.locks[key] = e | |||
| 58 | + | } | |||
| 59 | + | e.refs++ | |||
| 60 | + | l.mu.Unlock() | |||
| 61 | + | release := func() { | |||
| 62 | + | l.mu.Lock() | |||
| 63 | + | if e.refs--; e.refs == 0 { | |||
| 64 | + | delete(l.locks, key) | |||
| 65 | + | } | |||
| 66 | + | l.mu.Unlock() | |||
| 67 | + | } | |||
| 68 | + | select { | |||
| 69 | + | case e.ch <- struct{}{}: | |||
| 70 | + | return func() { <-e.ch; release() }, nil | |||
| 71 | + | case <-ctx.Done(): | |||
| 72 | + | release() | |||
| 73 | + | return nil, ctx.Err() | |||
| 74 | + | } | |||
| 75 | + | } | |||
| 33 | 76 | ||||
| 34 | var _ ports.IssueLinkTrigger = (*IssueLinks)(nil) | 77 | var _ ports.IssueLinkTrigger = (*IssueLinks)(nil) | ||
| 35 | 78 | ||||
| 36 | - | func NewIssueLinks(cfg IssueLinksConfig) *IssueLinks { return &IssueLinks{cfg: cfg} } | 79 | + | func NewIssueLinks(cfg IssueLinksConfig) *IssueLinks { |
| 80 | + | return &IssueLinks{cfg: cfg, locks: map[linkKey]*linkLock{}} | |||
| 81 | + | } | |||
| 37 | 82 | ||||
| 38 | // Reindex replaces links from the current texts and bounded commit range. | 83 | // Reindex replaces links from the current texts and bounded commit range. | ||
| 39 | func (l *IssueLinks) Reindex(ctx context.Context, repo domain.Repo, number int64) error { | 84 | func (l *IssueLinks) Reindex(ctx context.Context, repo domain.Repo, number int64) error { | ||
| @@ -41,6 +86,11 @@ func (l *IssueLinks) Reindex(ctx context.Context, repo domain.Repo, number int64 | |||||
| 41 | return err | 86 | return err | ||
| 42 | } | 87 | } | ||
| 43 | func (l *IssueLinks) reindex(ctx context.Context, repo domain.Repo, number int64) ([]int64, error) { | 88 | func (l *IssueLinks) reindex(ctx context.Context, repo domain.Repo, number int64) ([]int64, error) { | ||
| 89 | + | unlock, err := l.lockMR(ctx, repo.Name, number) | |||
| 90 | + | if err != nil { | |||
| 91 | + | return nil, err | |||
| 92 | + | } | |||
| 93 | + | defer unlock() | |||
| 44 | mr, err := l.cfg.MRs.Get(ctx, repo.Name, number) | 94 | mr, err := l.cfg.MRs.Get(ctx, repo.Name, number) | ||
| 45 | if err != nil { | 95 | if err != nil { | ||
| 46 | return nil, err | 96 | return nil, err | ||
| @@ -103,14 +153,21 @@ func (l *IssueLinks) reindex(ctx context.Context, repo domain.Repo, number int64 | |||||
| 103 | } | 153 | } | ||
| 104 | } | 154 | } | ||
| 105 | for range maxWriteRetries { | 155 | for range maxWriteRetries { | ||
| 106 | - | added, _, err := l.cfg.Links.ReplaceLinks(ctx, repo.Name, number, mentions, closes) | 156 | + | added, removed, err := l.cfg.Links.ReplaceLinks(ctx, repo.Name, number, mentions, closes) |
| 107 | if errors.Is(err, domain.ErrConflict) { | 157 | if errors.Is(err, domain.ErrConflict) { | ||
| 108 | continue | 158 | continue | ||
| 109 | } | 159 | } | ||
| 110 | if err != nil { | 160 | if err != nil { | ||
| 111 | return nil, err | 161 | return nil, err | ||
| 112 | } | 162 | } | ||
| 113 | - | for _, link := range added { | 163 | + | type pair struct{ issue, mr int64 } |
| 164 | + | sent := map[pair]struct{}{} | |||
| 165 | + | for _, link := range slices.Concat(added, removed) { | |||
| 166 | + | p := pair{link.Issue, link.MergeRequest} | |||
| 167 | + | if _, ok := sent[p]; ok { | |||
| 168 | + | continue | |||
| 169 | + | } | |||
| 170 | + | sent[p] = struct{}{} | |||
| 114 | l.notify(ctx, repo, link) | 171 | l.notify(ctx, repo, link) | ||
| 115 | } | 172 | } | ||
| 116 | return closes, nil | 173 | return closes, nil | ||
M internal/usecase/issue_links_test.go +104 -0
| @@ -3,8 +3,11 @@ | |||
| 3 | 3 | package usecase_test | |
| 4 | 4 | ||
| 5 | 5 | import ( | |
| 6 | + | "context" | |
| 6 | 7 | "fmt" | |
| 8 | + | "sync/atomic" | |
| 7 | 9 | "testing" | |
| 10 | + | "time" | |
| 8 | 11 | ||
| 9 | 12 | "bnema.dev/ermite/internal/domain" | |
| 10 | 13 | "bnema.dev/ermite/internal/ports" | |
| @@ -129,3 +132,104 @@ func TestIssueLinksMergeClosing(t *testing.T) { | |||
| 129 | 132 | }) | |
| 130 | 133 | } | |
| 131 | 134 | } | |
| 135 | + | ||
| 136 | + | type gatedBranchGit struct { | |
| 137 | + | ports.MergeGit | |
| 138 | + | entered atomic.Int32 | |
| 139 | + | started chan struct{} | |
| 140 | + | gate chan struct{} | |
| 141 | + | } | |
| 142 | + | ||
| 143 | + | func (g *gatedBranchGit) BranchHead(ctx context.Context, name domain.RepoName, branch string) (domain.ObjectID, error) { | |
| 144 | + | if g.entered.Add(1) == 1 { | |
| 145 | + | close(g.started) | |
| 146 | + | <-g.gate | |
| 147 | + | } | |
| 148 | + | return g.MergeGit.BranchHead(ctx, name, branch) | |
| 149 | + | } | |
| 150 | + | ||
| 151 | + | func TestIssueLinksReindexSerializedPerMergeRequest(t *testing.T) { | |
| 152 | + | r, _, issues := linkedRig(t) | |
| 153 | + | issue, err := issues.Create(r.ctx, root, r.repo, ports.CreateIssueParams{Title: "bug"}) | |
| 154 | + | require.NoError(t, err) | |
| 155 | + | r.feature("feat") | |
| 156 | + | mr := r.open("feat") | |
| 157 | + | g := &gatedBranchGit{MergeGit: r.git, started: make(chan struct{}), gate: make(chan struct{})} | |
| 158 | + | links := usecase.NewIssueLinks(usecase.IssueLinksConfig{Meta: r.store, Issues: r.store.Issues(), MRs: r.store.MergeRequests(), Links: r.store.IssueLinks(), Git: g, Bus: r.bus, Clock: fixedClock{}}) | |
| 159 | + | repo, err := r.store.Get(r.ctx, r.repo) | |
| 160 | + | require.NoError(t, err) | |
| 161 | + | done := make(chan error, 2) | |
| 162 | + | go func() { done <- links.Reindex(r.ctx, repo, mr.Number) }() | |
| 163 | + | <-g.started | |
| 164 | + | // The first reindex read the old texts; a note is added afterwards. | |
| 165 | + | _, err = r.mrs.AddComment(r.ctx, root, r.repo, mr.Number, ports.AddCommentParams{Text: fmt.Sprintf("see #%d", issue.Number)}) | |
| 166 | + | require.NoError(t, err) | |
| 167 | + | go func() { done <- links.Reindex(r.ctx, repo, mr.Number) }() | |
| 168 | + | time.Sleep(100 * time.Millisecond) | |
| 169 | + | assert.EqualValues(t, 1, g.entered.Load(), "the second reindex waits for the first") | |
| 170 | + | close(g.gate) | |
| 171 | + | require.NoError(t, <-done) | |
| 172 | + | require.NoError(t, <-done) | |
| 173 | + | got, err := r.store.IssueLinks().LinksOfMergeRequest(r.ctx, r.repo, mr.Number) | |
| 174 | + | require.NoError(t, err) | |
| 175 | + | require.Len(t, got, 1, "the final index reflects the latest content") | |
| 176 | + | } | |
| 177 | + | ||
| 178 | + | func TestIssueLinksNotifiesRemovalsAndClosingFlips(t *testing.T) { | |
| 179 | + | r, links, issues := linkedRig(t) | |
| 180 | + | issue, err := issues.Create(r.ctx, root, r.repo, ports.CreateIssueParams{Title: "bug"}) | |
| 181 | + | require.NoError(t, err) | |
| 182 | + | r.feature("feat") | |
| 183 | + | mr := r.open("feat") | |
| 184 | + | referenced := func() (n int) { | |
| 185 | + | for _, ev := range r.drain() { | |
| 186 | + | if e, ok := ev.(domain.IssueReferenced); ok && e.Issue == issue.Number { | |
| 187 | + | n++ | |
| 188 | + | } | |
| 189 | + | } | |
| 190 | + | return n | |
| 191 | + | } | |
| 192 | + | note, err := r.mrs.AddComment(r.ctx, root, r.repo, mr.Number, ports.AddCommentParams{Text: fmt.Sprintf("see #%d", issue.Number)}) | |
| 193 | + | require.NoError(t, err) | |
| 194 | + | assert.Equal(t, 1, referenced()) | |
| 195 | + | _, err = r.mrs.EditComment(r.ctx, root, r.repo, mr.Number, note.ID, "nothing") | |
| 196 | + | require.NoError(t, err) | |
| 197 | + | assert.Equal(t, 1, referenced(), "removing the last mention notifies") | |
| 198 | + | ||
| 199 | + | // closes true -> false emits one event: a commit message fixes the issue, | |
| 200 | + | // then is rewritten to a plain mention. | |
| 201 | + | head := r.commit(author, "fix.txt", "fix", fmt.Sprintf("Fixes #%d", issue.Number)) | |
| 202 | + | r.push("feat") | |
| 203 | + | links.OnBranchesMoved(r.ctx, r.repo, []domain.RefUpdate{{Ref: "refs/heads/feat", New: head.String()}}) | |
| 204 | + | referenced() | |
| 205 | + | r.sh("commit", "-q", "--amend", "-m", fmt.Sprintf("See #%d", issue.Number)) | |
| 206 | + | r.push("feat") | |
| 207 | + | links.OnBranchesMoved(r.ctx, r.repo, []domain.RefUpdate{{Ref: "refs/heads/feat", New: r.branchHead("feat").String()}}) | |
| 208 | + | assert.Equal(t, 1, referenced(), "a closing flip notifies once") | |
| 209 | + | } | |
| 210 | + | ||
| 211 | + | func TestIssueLinksMergeReindexesRequestsWhoseSourceMoved(t *testing.T) { | |
| 212 | + | r, _, issues := linkedRig(t) | |
| 213 | + | issue, err := issues.Create(r.ctx, root, r.repo, ports.CreateIssueParams{Title: "bug"}) | |
| 214 | + | require.NoError(t, err) | |
| 215 | + | r.sh("checkout", "-q", "-B", "x", "main") | |
| 216 | + | r.commit(author, "x.txt", "x", "work on x") | |
| 217 | + | r.push("x") | |
| 218 | + | r.sh("checkout", "-q", "-B", "feat", "x") | |
| 219 | + | head := r.commit(author, "fix.txt", "fix", fmt.Sprintf("Fixes #%d", issue.Number)) | |
| 220 | + | r.push("feat") | |
| 221 | + | b, err := r.mrs.Create(r.ctx, root, r.repo, ports.CreateMergeRequestParams{Title: "B", Source: "x"}) | |
| 222 | + | require.NoError(t, err) | |
| 223 | + | a, err := r.mrs.Create(r.ctx, root, r.repo, ports.CreateMergeRequestParams{Title: "A", Source: "feat", Target: "x"}) | |
| 224 | + | require.NoError(t, err) | |
| 225 | + | before, err := r.store.IssueLinks().LinksOfMergeRequest(r.ctx, r.repo, b.Number) | |
| 226 | + | require.NoError(t, err) | |
| 227 | + | require.Empty(t, before) | |
| 228 | + | _, err = r.mrs.Merge(r.ctx, root, r.repo, a.Number, ports.MergeParams{HeadID: head, Mode: domain.MergeModeFastForward}) | |
| 229 | + | require.NoError(t, err) | |
| 230 | + | r.mrs.Wait(r.ctx) | |
| 231 | + | after, err := r.store.IssueLinks().LinksOfMergeRequest(r.ctx, r.repo, b.Number) | |
| 232 | + | require.NoError(t, err) | |
| 233 | + | require.Len(t, after, 1) | |
| 234 | + | assert.Equal(t, issue.Number, after[0].Issue) | |
| 235 | + | } | |
| @@ -3,8 +3,11 @@ | |||||
| 3 | package usecase_test | 3 | package usecase_test | ||
| 4 | 4 | ||||
| 5 | import ( | 5 | import ( | ||
| 6 | + | "context" | |||
| 6 | "fmt" | 7 | "fmt" | ||
| 8 | + | "sync/atomic" | |||
| 7 | "testing" | 9 | "testing" | ||
| 10 | + | "time" | |||
| 8 | 11 | ||||
| 9 | "bnema.dev/ermite/internal/domain" | 12 | "bnema.dev/ermite/internal/domain" | ||
| 10 | "bnema.dev/ermite/internal/ports" | 13 | "bnema.dev/ermite/internal/ports" | ||
| @@ -129,3 +132,104 @@ func TestIssueLinksMergeClosing(t *testing.T) { | |||||
| 129 | }) | 132 | }) | ||
| 130 | } | 133 | } | ||
| 131 | } | 134 | } | ||
| 135 | + | ||||
| 136 | + | type gatedBranchGit struct { | |||
| 137 | + | ports.MergeGit | |||
| 138 | + | entered atomic.Int32 | |||
| 139 | + | started chan struct{} | |||
| 140 | + | gate chan struct{} | |||
| 141 | + | } | |||
| 142 | + | ||||
| 143 | + | func (g *gatedBranchGit) BranchHead(ctx context.Context, name domain.RepoName, branch string) (domain.ObjectID, error) { | |||
| 144 | + | if g.entered.Add(1) == 1 { | |||
| 145 | + | close(g.started) | |||
| 146 | + | <-g.gate | |||
| 147 | + | } | |||
| 148 | + | return g.MergeGit.BranchHead(ctx, name, branch) | |||
| 149 | + | } | |||
| 150 | + | ||||
| 151 | + | func TestIssueLinksReindexSerializedPerMergeRequest(t *testing.T) { | |||
| 152 | + | r, _, issues := linkedRig(t) | |||
| 153 | + | issue, err := issues.Create(r.ctx, root, r.repo, ports.CreateIssueParams{Title: "bug"}) | |||
| 154 | + | require.NoError(t, err) | |||
| 155 | + | r.feature("feat") | |||
| 156 | + | mr := r.open("feat") | |||
| 157 | + | g := &gatedBranchGit{MergeGit: r.git, started: make(chan struct{}), gate: make(chan struct{})} | |||
| 158 | + | links := usecase.NewIssueLinks(usecase.IssueLinksConfig{Meta: r.store, Issues: r.store.Issues(), MRs: r.store.MergeRequests(), Links: r.store.IssueLinks(), Git: g, Bus: r.bus, Clock: fixedClock{}}) | |||
| 159 | + | repo, err := r.store.Get(r.ctx, r.repo) | |||
| 160 | + | require.NoError(t, err) | |||
| 161 | + | done := make(chan error, 2) | |||
| 162 | + | go func() { done <- links.Reindex(r.ctx, repo, mr.Number) }() | |||
| 163 | + | <-g.started | |||
| 164 | + | // The first reindex read the old texts; a note is added afterwards. | |||
| 165 | + | _, err = r.mrs.AddComment(r.ctx, root, r.repo, mr.Number, ports.AddCommentParams{Text: fmt.Sprintf("see #%d", issue.Number)}) | |||
| 166 | + | require.NoError(t, err) | |||
| 167 | + | go func() { done <- links.Reindex(r.ctx, repo, mr.Number) }() | |||
| 168 | + | time.Sleep(100 * time.Millisecond) | |||
| 169 | + | assert.EqualValues(t, 1, g.entered.Load(), "the second reindex waits for the first") | |||
| 170 | + | close(g.gate) | |||
| 171 | + | require.NoError(t, <-done) | |||
| 172 | + | require.NoError(t, <-done) | |||
| 173 | + | got, err := r.store.IssueLinks().LinksOfMergeRequest(r.ctx, r.repo, mr.Number) | |||
| 174 | + | require.NoError(t, err) | |||
| 175 | + | require.Len(t, got, 1, "the final index reflects the latest content") | |||
| 176 | + | } | |||
| 177 | + | ||||
| 178 | + | func TestIssueLinksNotifiesRemovalsAndClosingFlips(t *testing.T) { | |||
| 179 | + | r, links, issues := linkedRig(t) | |||
| 180 | + | issue, err := issues.Create(r.ctx, root, r.repo, ports.CreateIssueParams{Title: "bug"}) | |||
| 181 | + | require.NoError(t, err) | |||
| 182 | + | r.feature("feat") | |||
| 183 | + | mr := r.open("feat") | |||
| 184 | + | referenced := func() (n int) { | |||
| 185 | + | for _, ev := range r.drain() { | |||
| 186 | + | if e, ok := ev.(domain.IssueReferenced); ok && e.Issue == issue.Number { | |||
| 187 | + | n++ | |||
| 188 | + | } | |||
| 189 | + | } | |||
| 190 | + | return n | |||
| 191 | + | } | |||
| 192 | + | note, err := r.mrs.AddComment(r.ctx, root, r.repo, mr.Number, ports.AddCommentParams{Text: fmt.Sprintf("see #%d", issue.Number)}) | |||
| 193 | + | require.NoError(t, err) | |||
| 194 | + | assert.Equal(t, 1, referenced()) | |||
| 195 | + | _, err = r.mrs.EditComment(r.ctx, root, r.repo, mr.Number, note.ID, "nothing") | |||
| 196 | + | require.NoError(t, err) | |||
| 197 | + | assert.Equal(t, 1, referenced(), "removing the last mention notifies") | |||
| 198 | + | ||||
| 199 | + | // closes true -> false emits one event: a commit message fixes the issue, | |||
| 200 | + | // then is rewritten to a plain mention. | |||
| 201 | + | head := r.commit(author, "fix.txt", "fix", fmt.Sprintf("Fixes #%d", issue.Number)) | |||
| 202 | + | r.push("feat") | |||
| 203 | + | links.OnBranchesMoved(r.ctx, r.repo, []domain.RefUpdate{{Ref: "refs/heads/feat", New: head.String()}}) | |||
| 204 | + | referenced() | |||
| 205 | + | r.sh("commit", "-q", "--amend", "-m", fmt.Sprintf("See #%d", issue.Number)) | |||
| 206 | + | r.push("feat") | |||
| 207 | + | links.OnBranchesMoved(r.ctx, r.repo, []domain.RefUpdate{{Ref: "refs/heads/feat", New: r.branchHead("feat").String()}}) | |||
| 208 | + | assert.Equal(t, 1, referenced(), "a closing flip notifies once") | |||
| 209 | + | } | |||
| 210 | + | ||||
| 211 | + | func TestIssueLinksMergeReindexesRequestsWhoseSourceMoved(t *testing.T) { | |||
| 212 | + | r, _, issues := linkedRig(t) | |||
| 213 | + | issue, err := issues.Create(r.ctx, root, r.repo, ports.CreateIssueParams{Title: "bug"}) | |||
| 214 | + | require.NoError(t, err) | |||
| 215 | + | r.sh("checkout", "-q", "-B", "x", "main") | |||
| 216 | + | r.commit(author, "x.txt", "x", "work on x") | |||
| 217 | + | r.push("x") | |||
| 218 | + | r.sh("checkout", "-q", "-B", "feat", "x") | |||
| 219 | + | head := r.commit(author, "fix.txt", "fix", fmt.Sprintf("Fixes #%d", issue.Number)) | |||
| 220 | + | r.push("feat") | |||
| 221 | + | b, err := r.mrs.Create(r.ctx, root, r.repo, ports.CreateMergeRequestParams{Title: "B", Source: "x"}) | |||
| 222 | + | require.NoError(t, err) | |||
| 223 | + | a, err := r.mrs.Create(r.ctx, root, r.repo, ports.CreateMergeRequestParams{Title: "A", Source: "feat", Target: "x"}) | |||
| 224 | + | require.NoError(t, err) | |||
| 225 | + | before, err := r.store.IssueLinks().LinksOfMergeRequest(r.ctx, r.repo, b.Number) | |||
| 226 | + | require.NoError(t, err) | |||
| 227 | + | require.Empty(t, before) | |||
| 228 | + | _, err = r.mrs.Merge(r.ctx, root, r.repo, a.Number, ports.MergeParams{HeadID: head, Mode: domain.MergeModeFastForward}) | |||
| 229 | + | require.NoError(t, err) | |||
| 230 | + | r.mrs.Wait(r.ctx) | |||
| 231 | + | after, err := r.store.IssueLinks().LinksOfMergeRequest(r.ctx, r.repo, b.Number) | |||
| 232 | + | require.NoError(t, err) | |||
| 233 | + | require.Len(t, after, 1) | |||
| 234 | + | assert.Equal(t, issue.Number, after[0].Issue) | |||
| 235 | + | } | |||
M internal/usecase/mergerequests.go +3 -0
| @@ -1217,6 +1217,9 @@ func (m *MergeRequests) afterMerge(ctx context.Context, p domain.Principal, repo | |||
| 1217 | 1217 | if m.mergeab != nil { | |
| 1218 | 1218 | m.mergeab.OnBranchesMoved(ctx, name, updates) // the other requests of the moved target | |
| 1219 | 1219 | } | |
| 1220 | + | if m.links != nil { | |
| 1221 | + | m.links.OnBranchesMoved(ctx, name, updates) // open requests whose source moved | |
| 1222 | + | } | |
| 1220 | 1223 | } | |
| 1221 | 1224 | ||
| 1222 | 1225 | // deleteSource deletes the source branch of a merged request if it is still | |
| @@ -1217,6 +1217,9 @@ func (m *MergeRequests) afterMerge(ctx context.Context, p domain.Principal, repo | |||||
| 1217 | if m.mergeab != nil { | 1217 | if m.mergeab != nil { | ||
| 1218 | m.mergeab.OnBranchesMoved(ctx, name, updates) // the other requests of the moved target | 1218 | m.mergeab.OnBranchesMoved(ctx, name, updates) // the other requests of the moved target | ||
| 1219 | } | 1219 | } | ||
| 1220 | + | if m.links != nil { | |||
| 1221 | + | m.links.OnBranchesMoved(ctx, name, updates) // open requests whose source moved | |||
| 1222 | + | } | |||
| 1220 | } | 1223 | } | ||
| 1221 | 1224 | ||||
| 1222 | // deleteSource deletes the source branch of a merged request if it is still | 1225 | // deleteSource deletes the source branch of a merged request if it is still | ||