Skip to content

Commit d0b8200

Browse files
forgejo-backport-actionsclu1034
authored andcommitted
[v13.0/forgejo] feat(ui): improve rendering commit links for PR commits, external repos and diffs (go-gitea#9493)
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/9146 - fix links to PR commits, e.g. `!7979 (commit 4d968c08e0)` - show the repo slug for links other than the current repository, e.g. `forgejo/docs@498bd80133` - truncate long `diff-...` fragments, e.g. `600be26638 (diff-953bb4f01)` - show `files` query values for compare links, e.g. `8bbac4c679...e2278e5 (options/locale/locale_fi-FI.ini)` Closes: go-gitea#7980 Closes: go-gitea#9130 Closes: go-gitea#8023 Ref: go-gitea#5901 ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Tests - I added test coverage for Go changes... - [x] in their respective `*_test.go` for unit tests. - [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I added test coverage for JavaScript changes... - [ ] in `web_src/js/*.test.js` if it can be unit tested. - [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)). ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [ ] I do not want this change to show in the release notes. - [x] I want the title to show in the release notes with a link to this pull request. - [ ] I want the content of the `release-notes/<pull request number>.md` to be be used for the release notes instead of the title. <!--start release-notes-assistant--> ## Release notes <!--URL:https://codeberg.org/forgejo/forgejo--> - User Interface features - [PR](https://codeberg.org/forgejo/forgejo/pulls/9146): <!--number 9146 --><!--line 0 --><!--description ZmVhdCh1aSk6IGltcHJvdmUgcmVuZGVyaW5nIGNvbW1pdCBsaW5rcyBmb3IgUFIgY29tbWl0cywgZXh0ZXJuYWwgcmVwb3MgYW5kIGRpZmZz-->feat(ui): improve rendering commit links for PR commits, external repos and diffs<!--description--> <!--end release-notes-assistant--> Co-authored-by: Lucas Schwiderski <lucas@lschwiderski.de> Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9493 Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org> Co-authored-by: forgejo-backport-action <forgejo-backport-action@noreply.codeberg.org> Co-committed-by: forgejo-backport-action <forgejo-backport-action@noreply.codeberg.org>
1 parent d452207 commit d0b8200

3 files changed

Lines changed: 349 additions & 42 deletions

File tree

modules/markup/html.go

Lines changed: 186 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -55,10 +55,13 @@ var (
5555
shortLinkPattern = regexp.MustCompile(`\[\[(.*?)\]\](\w*)`)
5656

5757
// anyHashPattern splits url containing SHA into parts
58-
anyHashPattern = regexp.MustCompile(`https?://(?:(?:\S+/){3,4}(?:commit|tree|blob)/)([0-9a-f]{7,64})(/[-+~_%.a-zA-Z0-9/]+)?(\?[-+~_%\.a-zA-Z0-9=&]+)?(#[-+~_%.a-zA-Z0-9]+)?`)
58+
anyHashPattern = regexp.MustCompile(`https?://[^\s/]+/(\S+/(?:commit|tree|blob))/([0-9a-f]{7,64})(/[-+~_%.a-zA-Z0-9/]+)?(\?[-+~_%\.a-zA-Z0-9=&]+)?(#[-+~_%.a-zA-Z0-9]+)?`)
5959

6060
// comparePattern matches "http://domain/org/repo/compare/COMMIT1...COMMIT2#hash"
61-
comparePattern = regexp.MustCompile(`https?://(?:\S+/){4,5}([0-9a-f]{7,64})(\.\.\.?)([0-9a-f]{7,64})?(#[-+~_%.a-zA-Z0-9]+)?`)
61+
comparePattern = regexp.MustCompile(`https?://[^\s/]+/(?:\S+/)?([^\s/]+/[^\s/]+)/compare/([0-9a-f]{7,64})(\.\.\.?)([0-9a-f]{7,64})?(\?[-+~_%\.a-zA-Z0-9=&/]+)?(#[-+~_%.a-zA-Z0-9]+)?`)
62+
63+
// pullReviewCommitPattern matches "https://domain.tld/<subpath...>/<owner>/<repo>/pulls/<id>/commits/<sha>"
64+
pullReviewCommitPattern = regexp.MustCompile(`https?://[^\s/]+/(?:\S+/)?([^\s/]+/[^\s/]+)/pulls/(\d+)/commits/([0-9a-f]{7,64})(#[-+~_%.a-zA-Z0-9]+)?`)
6265

6366
validLinksPattern = regexp.MustCompile(`^[a-z][\w-]+://`)
6467

@@ -147,6 +150,7 @@ func (p *postProcessError) Error() string {
147150
type processor func(ctx *RenderContext, node *html.Node)
148151

149152
var defaultProcessors = []processor{
153+
pullReviewCommitPatternProcessor,
150154
fullIssuePatternProcessor,
151155
comparePatternProcessor,
152156
filePreviewPatternProcessor,
@@ -177,6 +181,7 @@ func PostProcess(
177181
}
178182

179183
var commitMessageProcessors = []processor{
184+
pullReviewCommitPatternProcessor,
180185
fullIssuePatternProcessor,
181186
comparePatternProcessor,
182187
fullHashPatternProcessor,
@@ -209,6 +214,7 @@ func RenderCommitMessage(
209214
}
210215

211216
var commitMessageSubjectProcessors = []processor{
217+
pullReviewCommitPatternProcessor,
212218
fullIssuePatternProcessor,
213219
comparePatternProcessor,
214220
fullHashPatternProcessor,
@@ -796,6 +802,64 @@ func shortLinkProcessor(ctx *RenderContext, node *html.Node) {
796802
}
797803
}
798804

805+
// pullReviewCommitPatternProcessor creates links to pull review commits.
806+
func pullReviewCommitPatternProcessor(ctx *RenderContext, node *html.Node) {
807+
next := node.NextSibling
808+
for node != nil && node != next {
809+
m := pullReviewCommitPattern.FindStringSubmatchIndex(node.Data)
810+
if m == nil {
811+
return
812+
}
813+
814+
urlFull := node.Data[m[0]:m[1]]
815+
repoSlug := node.Data[m[2]:m[3]]
816+
id := node.Data[m[4]:m[5]]
817+
sha := base.ShortSha(node.Data[m[6]:m[7]])
818+
819+
// Create an `<a>` node with a text of
820+
// `!123 (commit <code>abcdef1234</code>)`
821+
aNode := &html.Node{
822+
Type: html.ElementNode,
823+
Data: atom.A.String(),
824+
Attr: []html.Attribute{{Key: "href", Val: urlFull}, {Key: "class", Val: "commit"}},
825+
}
826+
827+
text := "!" + id + " (commit "
828+
829+
baseURLEnd := strings.Index(urlFull, repoSlug) + len(repoSlug)
830+
if len(ctx.Links.Base) > 0 && !strings.HasPrefix(ctx.Links.Base, urlFull[:baseURLEnd]) {
831+
text = repoSlug + "@" + text
832+
}
833+
834+
aNode.AppendChild(&html.Node{
835+
Type: html.TextNode,
836+
Data: text,
837+
})
838+
839+
textNode := &html.Node{
840+
Type: html.TextNode,
841+
Data: sha,
842+
}
843+
844+
codeNode := &html.Node{
845+
Type: html.ElementNode,
846+
Data: atom.Code.String(),
847+
Attr: []html.Attribute{{Key: "class", Val: "nohighlight"}},
848+
}
849+
850+
codeNode.AppendChild(textNode)
851+
aNode.AppendChild(codeNode)
852+
853+
aNode.AppendChild(&html.Node{
854+
Type: html.TextNode,
855+
Data: ")",
856+
})
857+
858+
replaceContent(node, m[0], m[1], aNode)
859+
node = node.NextSibling.NextSibling
860+
}
861+
}
862+
799863
func fullIssuePatternProcessor(ctx *RenderContext, node *html.Node) {
800864
if ctx.Metas == nil {
801865
return
@@ -952,7 +1016,7 @@ func commitCrossReferencePatternProcessor(ctx *RenderContext, node *html.Node) {
9521016
}
9531017
}
9541018

955-
// fullHashPatternProcessor renders SHA containing URLs
1019+
// fullHashPatternProcessor renders URLs that contain a SHA
9561020
func fullHashPatternProcessor(ctx *RenderContext, node *html.Node) {
9571021
if ctx.Metas == nil {
9581022
return
@@ -966,37 +1030,103 @@ func fullHashPatternProcessor(ctx *RenderContext, node *html.Node) {
9661030
}
9671031

9681032
urlFull := node.Data[m[0]:m[1]]
969-
text := base.ShortSha(node.Data[m[2]:m[3]])
9701033

971-
// 3rd capture group matches a optional path
972-
subpath := ""
973-
if m[5] > 0 {
974-
subpath = node.Data[m[4]:m[5]]
1034+
// In most cases, the URL will look like this:
1035+
// `https://domain.tld/<owner>/<repo>/<path>/<sha>`.
1036+
// The amount of components in `<path>` is variable, but that alone is doable with regexp.
1037+
//
1038+
// However, Forgejo also allows being hosted on a sub path, i.e.
1039+
// `https://domain.tld/<sub>/<owner>/<repo>/<path>/<sha>`.
1040+
// And this sub path can also have any amount of components. But fishing out a section
1041+
// between two variable length matches is not something regular grammars are capable of.
1042+
//
1043+
// Instead, the regexp extracts the entire path section before the SHA
1044+
// (i.e. `<sub>/<owner>/<repo>/<path>`), and we find the components we need by counting.
1045+
// `<sub>` is unknown, but the possible values for `<path>` are defined by us
1046+
// (see `router/web/web.go`). So we count from the back.
1047+
subPath := node.Data[m[2]:m[3]]
1048+
1049+
components := strings.Split(subPath, "/")
1050+
componentCount := len(components)
1051+
1052+
// In most cases, the `<owner>` component is right at the start of the path.
1053+
ownerIndex := 0
1054+
1055+
// But if there are more than three components, this could be `<sub>` or an app route
1056+
// with two components. Or both.
1057+
if componentCount > 3 {
1058+
// As mentioned, we count from the back. We decrement for the `<repo>` component, and the one
1059+
// component from the app route that's guaranteed to be there.
1060+
// We also adjust this to be an array index, so we subtract one more.
1061+
ownerIndex = componentCount - 3
1062+
1063+
// We then check for known app routes that use two components.
1064+
// Currently, this checks for:
1065+
// - `src/commit`
1066+
// - `commits/commit`
1067+
//
1068+
// This does have one scenario where we cannot figure things out reliably:
1069+
// If there is a sub path, and the repository is named like one of the known app routes
1070+
// (e.g. `src`), we cannot distinguish between the repo and the app route.
1071+
// We assume that naming a repository like that is uncommon, and prioritize the case where its
1072+
// part of the app route.
1073+
if components[componentCount-1] == "commit" &&
1074+
(components[componentCount-2] == "src" || components[componentCount-2] == "commits") {
1075+
ownerIndex--
1076+
}
1077+
}
1078+
1079+
repoSlug := components[ownerIndex] + "/" + components[ownerIndex+1]
1080+
1081+
text := base.ShortSha(node.Data[m[4]:m[5]])
1082+
1083+
// We need to figure out the base of the provided URL, which is up to and including the
1084+
// `<owner>/<repo>` slug.
1085+
// With that we can determine if it matches the current repo, or if the slug should be shown.
1086+
baseURLEnd := strings.Index(urlFull, repoSlug) + len(repoSlug)
1087+
if len(ctx.Links.Base) > 0 && !strings.HasPrefix(ctx.Links.Base, urlFull[:baseURLEnd]) {
1088+
text = repoSlug + "@" + text
1089+
}
1090+
1091+
// 3rd capture group matches an optional file path after the SHA
1092+
filePath := ""
1093+
if m[7] > 0 {
1094+
filePath = node.Data[m[6]:m[7]]
9751095
}
9761096

9771097
// 5th capture group matches a optional url hash
9781098
hash := ""
979-
if m[9] > 0 {
980-
hash = node.Data[m[8]:m[9]][1:]
1099+
if m[11] > 0 {
1100+
hash = node.Data[m[10]:m[11]][1:]
1101+
1102+
// Truncate long diff IDs
1103+
if len(hash) > 15 && strings.HasPrefix(hash, "diff-") {
1104+
hash = hash[:15]
1105+
}
9811106
}
9821107

9831108
start := m[0]
9841109
end := m[1]
9851110

986-
// If url ends in '.', it's very likely that it is not part of the
987-
// actual url but used to finish a sentence.
1111+
// If the URL ends in '.', it's very likely that it is not part of the
1112+
// actual URL but used to finish a sentence.
9881113
if strings.HasSuffix(urlFull, ".") {
9891114
end--
9901115
urlFull = urlFull[:len(urlFull)-1]
9911116
if hash != "" {
9921117
hash = hash[:len(hash)-1]
993-
} else if subpath != "" {
994-
subpath = subpath[:len(subpath)-1]
1118+
} else if filePath != "" {
1119+
filePath = filePath[:len(filePath)-1]
9951120
}
9961121
}
9971122

998-
if subpath != "" {
999-
text += subpath
1123+
if filePath != "" {
1124+
decoded, err := url.QueryUnescape(filePath)
1125+
if err != nil {
1126+
text += decoded
1127+
} else {
1128+
text += filePath
1129+
}
10001130
}
10011131

10021132
if hash != "" {
@@ -1019,41 +1149,71 @@ func comparePatternProcessor(ctx *RenderContext, node *html.Node) {
10191149
return
10201150
}
10211151

1022-
// Ensure that every group (m[0]...m[7]) has a match
1023-
for i := 0; i < 8; i++ {
1152+
// Ensure that every group (m[0]...m[9]) has a match
1153+
for i := 0; i < 10; i++ {
10241154
if m[i] == -1 {
10251155
return
10261156
}
10271157
}
10281158

10291159
urlFull := node.Data[m[0]:m[1]]
1030-
text1 := base.ShortSha(node.Data[m[2]:m[3]])
1031-
textDots := base.ShortSha(node.Data[m[4]:m[5]])
1032-
text2 := base.ShortSha(node.Data[m[6]:m[7]])
1160+
repoSlug := node.Data[m[2]:m[3]]
1161+
text1 := base.ShortSha(node.Data[m[4]:m[5]])
1162+
textDots := base.ShortSha(node.Data[m[6]:m[7]])
1163+
text2 := base.ShortSha(node.Data[m[8]:m[9]])
1164+
1165+
query := ""
1166+
if m[11] > 0 {
1167+
query = node.Data[m[10]:m[11]][1:]
1168+
}
10331169

10341170
hash := ""
1035-
if m[9] > 0 {
1036-
hash = node.Data[m[8]:m[9]][1:]
1171+
if m[13] > 0 {
1172+
hash = node.Data[m[12]:m[13]][1:]
10371173
}
10381174

10391175
start := m[0]
10401176
end := m[1]
10411177

1042-
// If url ends in '.', it's very likely that it is not part of the
1043-
// actual url but used to finish a sentence.
1178+
// If the URL ends in '.', it's very likely that it is not part of the
1179+
// actual URL but used to finish a sentence.
10441180
if strings.HasSuffix(urlFull, ".") {
10451181
end--
10461182
urlFull = urlFull[:len(urlFull)-1]
10471183
if hash != "" {
10481184
hash = hash[:len(hash)-1]
1185+
} else if query != "" {
1186+
query = query[:len(query)-1]
10491187
} else if text2 != "" {
10501188
text2 = text2[:len(text2)-1]
10511189
}
10521190
}
10531191

10541192
text := text1 + textDots + text2
1193+
1194+
baseURLEnd := strings.Index(urlFull, repoSlug) + len(repoSlug)
1195+
if len(ctx.Links.Base) > 0 && !strings.HasPrefix(ctx.Links.Base, urlFull[:baseURLEnd]) {
1196+
text = repoSlug + "@" + text
1197+
}
1198+
1199+
extra := ""
1200+
if query != "" {
1201+
query, err := url.ParseQuery(query)
1202+
if err == nil && query.Has("files") {
1203+
extra = query.Get("files")
1204+
}
1205+
}
1206+
10551207
if hash != "" {
1056-
text += " (" + hash + ")"
1208+
if extra != "" {
1209+
extra += "#"
1210+
}
1211+
1212+
extra += hash
1213+
}
1214+
1215+
if extra != "" {
1216+
text += " (" + extra + ")"
10571217
}
10581218
replaceContent(node, start, end, createCodeLink(urlFull, text, "compare"))
10591219
node = node.NextSibling.NextSibling

0 commit comments

Comments
 (0)