mirror of
https://codeberg.org/forgejo/forgejo.git
synced 2026-05-12 22:10:25 +00:00
feat(ui): improve rendering commit links for PR commits, external repos and diffs (#9146)
- fix links to PR commits, e.g. `!7979 (commit4d968c08e0)` - 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...e2278e5a38 (options/locale/locale_fi-FI.ini)` Closes: #7980 Closes: #9130 Closes: #8023 Ref: #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--> Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9146 Reviewed-by: Robert Wolff <mahlzahn@posteo.de> Co-authored-by: Lucas Schwiderski <lucas@lschwiderski.de> Co-committed-by: Lucas Schwiderski <lucas@lschwiderski.de>
This commit is contained in:
Lucas Schwiderski
Earl Warren
parent
6d1818ffdb
commit
7530e8a941
3 changed files with 349 additions and 42 deletions
|
|
@ -55,10 +55,13 @@ var (
|
|||
shortLinkPattern = regexp.MustCompile(`\[\[(.*?)\]\](\w*)`)
|
||||
|
||||
// anyHashPattern splits url containing SHA into parts
|
||||
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]+)?`)
|
||||
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]+)?`)
|
||||
|
||||
// comparePattern matches "http://domain/org/repo/compare/COMMIT1...COMMIT2#hash"
|
||||
comparePattern = regexp.MustCompile(`https?://(?:\S+/){4,5}([0-9a-f]{7,64})(\.\.\.?)([0-9a-f]{7,64})?(#[-+~_%.a-zA-Z0-9]+)?`)
|
||||
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]+)?`)
|
||||
|
||||
// pullReviewCommitPattern matches "https://domain.tld/<subpath...>/<owner>/<repo>/pulls/<id>/commits/<sha>"
|
||||
pullReviewCommitPattern = regexp.MustCompile(`https?://[^\s/]+/(?:\S+/)?([^\s/]+/[^\s/]+)/pulls/(\d+)/commits/([0-9a-f]{7,64})(#[-+~_%.a-zA-Z0-9]+)?`)
|
||||
|
||||
validLinksPattern = regexp.MustCompile(`^[a-z][\w-]+://`)
|
||||
|
||||
|
|
@ -147,6 +150,7 @@ func (p *postProcessError) Error() string {
|
|||
type processor func(ctx *RenderContext, node *html.Node)
|
||||
|
||||
var defaultProcessors = []processor{
|
||||
pullReviewCommitPatternProcessor,
|
||||
fullIssuePatternProcessor,
|
||||
comparePatternProcessor,
|
||||
filePreviewPatternProcessor,
|
||||
|
|
@ -177,6 +181,7 @@ func PostProcess(
|
|||
}
|
||||
|
||||
var commitMessageProcessors = []processor{
|
||||
pullReviewCommitPatternProcessor,
|
||||
fullIssuePatternProcessor,
|
||||
comparePatternProcessor,
|
||||
fullHashPatternProcessor,
|
||||
|
|
@ -209,6 +214,7 @@ func RenderCommitMessage(
|
|||
}
|
||||
|
||||
var commitMessageSubjectProcessors = []processor{
|
||||
pullReviewCommitPatternProcessor,
|
||||
fullIssuePatternProcessor,
|
||||
comparePatternProcessor,
|
||||
fullHashPatternProcessor,
|
||||
|
|
@ -796,6 +802,64 @@ func shortLinkProcessor(ctx *RenderContext, node *html.Node) {
|
|||
}
|
||||
}
|
||||
|
||||
// pullReviewCommitPatternProcessor creates links to pull review commits.
|
||||
func pullReviewCommitPatternProcessor(ctx *RenderContext, node *html.Node) {
|
||||
next := node.NextSibling
|
||||
for node != nil && node != next {
|
||||
m := pullReviewCommitPattern.FindStringSubmatchIndex(node.Data)
|
||||
if m == nil {
|
||||
return
|
||||
}
|
||||
|
||||
urlFull := node.Data[m[0]:m[1]]
|
||||
repoSlug := node.Data[m[2]:m[3]]
|
||||
id := node.Data[m[4]:m[5]]
|
||||
sha := base.ShortSha(node.Data[m[6]:m[7]])
|
||||
|
||||
// Create an `<a>` node with a text of
|
||||
// `!123 (commit <code>abcdef1234</code>)`
|
||||
aNode := &html.Node{
|
||||
Type: html.ElementNode,
|
||||
Data: atom.A.String(),
|
||||
Attr: []html.Attribute{{Key: "href", Val: urlFull}, {Key: "class", Val: "commit"}},
|
||||
}
|
||||
|
||||
text := "!" + id + " (commit "
|
||||
|
||||
baseURLEnd := strings.Index(urlFull, repoSlug) + len(repoSlug)
|
||||
if len(ctx.Links.Base) > 0 && !strings.HasPrefix(ctx.Links.Base, urlFull[:baseURLEnd]) {
|
||||
text = repoSlug + "@" + text
|
||||
}
|
||||
|
||||
aNode.AppendChild(&html.Node{
|
||||
Type: html.TextNode,
|
||||
Data: text,
|
||||
})
|
||||
|
||||
textNode := &html.Node{
|
||||
Type: html.TextNode,
|
||||
Data: sha,
|
||||
}
|
||||
|
||||
codeNode := &html.Node{
|
||||
Type: html.ElementNode,
|
||||
Data: atom.Code.String(),
|
||||
Attr: []html.Attribute{{Key: "class", Val: "nohighlight"}},
|
||||
}
|
||||
|
||||
codeNode.AppendChild(textNode)
|
||||
aNode.AppendChild(codeNode)
|
||||
|
||||
aNode.AppendChild(&html.Node{
|
||||
Type: html.TextNode,
|
||||
Data: ")",
|
||||
})
|
||||
|
||||
replaceContent(node, m[0], m[1], aNode)
|
||||
node = node.NextSibling.NextSibling
|
||||
}
|
||||
}
|
||||
|
||||
func fullIssuePatternProcessor(ctx *RenderContext, node *html.Node) {
|
||||
if ctx.Metas == nil {
|
||||
return
|
||||
|
|
@ -952,7 +1016,7 @@ func commitCrossReferencePatternProcessor(ctx *RenderContext, node *html.Node) {
|
|||
}
|
||||
}
|
||||
|
||||
// fullHashPatternProcessor renders SHA containing URLs
|
||||
// fullHashPatternProcessor renders URLs that contain a SHA
|
||||
func fullHashPatternProcessor(ctx *RenderContext, node *html.Node) {
|
||||
if ctx.Metas == nil {
|
||||
return
|
||||
|
|
@ -966,37 +1030,103 @@ func fullHashPatternProcessor(ctx *RenderContext, node *html.Node) {
|
|||
}
|
||||
|
||||
urlFull := node.Data[m[0]:m[1]]
|
||||
text := base.ShortSha(node.Data[m[2]:m[3]])
|
||||
|
||||
// 3rd capture group matches a optional path
|
||||
subpath := ""
|
||||
if m[5] > 0 {
|
||||
subpath = node.Data[m[4]:m[5]]
|
||||
// In most cases, the URL will look like this:
|
||||
// `https://domain.tld/<owner>/<repo>/<path>/<sha>`.
|
||||
// The amount of components in `<path>` is variable, but that alone is doable with regexp.
|
||||
//
|
||||
// However, Forgejo also allows being hosted on a sub path, i.e.
|
||||
// `https://domain.tld/<sub>/<owner>/<repo>/<path>/<sha>`.
|
||||
// And this sub path can also have any amount of components. But fishing out a section
|
||||
// between two variable length matches is not something regular grammars are capable of.
|
||||
//
|
||||
// Instead, the regexp extracts the entire path section before the SHA
|
||||
// (i.e. `<sub>/<owner>/<repo>/<path>`), and we find the components we need by counting.
|
||||
// `<sub>` is unknown, but the possible values for `<path>` are defined by us
|
||||
// (see `router/web/web.go`). So we count from the back.
|
||||
subPath := node.Data[m[2]:m[3]]
|
||||
|
||||
components := strings.Split(subPath, "/")
|
||||
componentCount := len(components)
|
||||
|
||||
// In most cases, the `<owner>` component is right at the start of the path.
|
||||
ownerIndex := 0
|
||||
|
||||
// But if there are more than three components, this could be `<sub>` or an app route
|
||||
// with two components. Or both.
|
||||
if componentCount > 3 {
|
||||
// As mentioned, we count from the back. We decrement for the `<repo>` component, and the one
|
||||
// component from the app route that's guaranteed to be there.
|
||||
// We also adjust this to be an array index, so we subtract one more.
|
||||
ownerIndex = componentCount - 3
|
||||
|
||||
// We then check for known app routes that use two components.
|
||||
// Currently, this checks for:
|
||||
// - `src/commit`
|
||||
// - `commits/commit`
|
||||
//
|
||||
// This does have one scenario where we cannot figure things out reliably:
|
||||
// If there is a sub path, and the repository is named like one of the known app routes
|
||||
// (e.g. `src`), we cannot distinguish between the repo and the app route.
|
||||
// We assume that naming a repository like that is uncommon, and prioritize the case where its
|
||||
// part of the app route.
|
||||
if components[componentCount-1] == "commit" &&
|
||||
(components[componentCount-2] == "src" || components[componentCount-2] == "commits") {
|
||||
ownerIndex--
|
||||
}
|
||||
}
|
||||
|
||||
repoSlug := components[ownerIndex] + "/" + components[ownerIndex+1]
|
||||
|
||||
text := base.ShortSha(node.Data[m[4]:m[5]])
|
||||
|
||||
// We need to figure out the base of the provided URL, which is up to and including the
|
||||
// `<owner>/<repo>` slug.
|
||||
// With that we can determine if it matches the current repo, or if the slug should be shown.
|
||||
baseURLEnd := strings.Index(urlFull, repoSlug) + len(repoSlug)
|
||||
if len(ctx.Links.Base) > 0 && !strings.HasPrefix(ctx.Links.Base, urlFull[:baseURLEnd]) {
|
||||
text = repoSlug + "@" + text
|
||||
}
|
||||
|
||||
// 3rd capture group matches an optional file path after the SHA
|
||||
filePath := ""
|
||||
if m[7] > 0 {
|
||||
filePath = node.Data[m[6]:m[7]]
|
||||
}
|
||||
|
||||
// 5th capture group matches a optional url hash
|
||||
hash := ""
|
||||
if m[9] > 0 {
|
||||
hash = node.Data[m[8]:m[9]][1:]
|
||||
if m[11] > 0 {
|
||||
hash = node.Data[m[10]:m[11]][1:]
|
||||
|
||||
// Truncate long diff IDs
|
||||
if len(hash) > 15 && strings.HasPrefix(hash, "diff-") {
|
||||
hash = hash[:15]
|
||||
}
|
||||
}
|
||||
|
||||
start := m[0]
|
||||
end := m[1]
|
||||
|
||||
// If url ends in '.', it's very likely that it is not part of the
|
||||
// actual url but used to finish a sentence.
|
||||
// If the URL ends in '.', it's very likely that it is not part of the
|
||||
// actual URL but used to finish a sentence.
|
||||
if strings.HasSuffix(urlFull, ".") {
|
||||
end--
|
||||
urlFull = urlFull[:len(urlFull)-1]
|
||||
if hash != "" {
|
||||
hash = hash[:len(hash)-1]
|
||||
} else if subpath != "" {
|
||||
subpath = subpath[:len(subpath)-1]
|
||||
} else if filePath != "" {
|
||||
filePath = filePath[:len(filePath)-1]
|
||||
}
|
||||
}
|
||||
|
||||
if subpath != "" {
|
||||
text += subpath
|
||||
if filePath != "" {
|
||||
decoded, err := url.QueryUnescape(filePath)
|
||||
if err != nil {
|
||||
text += decoded
|
||||
} else {
|
||||
text += filePath
|
||||
}
|
||||
}
|
||||
|
||||
if hash != "" {
|
||||
|
|
@ -1019,41 +1149,71 @@ func comparePatternProcessor(ctx *RenderContext, node *html.Node) {
|
|||
return
|
||||
}
|
||||
|
||||
// Ensure that every group (m[0]...m[7]) has a match
|
||||
for i := 0; i < 8; i++ {
|
||||
// Ensure that every group (m[0]...m[9]) has a match
|
||||
for i := 0; i < 10; i++ {
|
||||
if m[i] == -1 {
|
||||
return
|
||||
}
|
||||
}
|
||||
|
||||
urlFull := node.Data[m[0]:m[1]]
|
||||
text1 := base.ShortSha(node.Data[m[2]:m[3]])
|
||||
textDots := base.ShortSha(node.Data[m[4]:m[5]])
|
||||
text2 := base.ShortSha(node.Data[m[6]:m[7]])
|
||||
repoSlug := node.Data[m[2]:m[3]]
|
||||
text1 := base.ShortSha(node.Data[m[4]:m[5]])
|
||||
textDots := base.ShortSha(node.Data[m[6]:m[7]])
|
||||
text2 := base.ShortSha(node.Data[m[8]:m[9]])
|
||||
|
||||
query := ""
|
||||
if m[11] > 0 {
|
||||
query = node.Data[m[10]:m[11]][1:]
|
||||
}
|
||||
|
||||
hash := ""
|
||||
if m[9] > 0 {
|
||||
hash = node.Data[m[8]:m[9]][1:]
|
||||
if m[13] > 0 {
|
||||
hash = node.Data[m[12]:m[13]][1:]
|
||||
}
|
||||
|
||||
start := m[0]
|
||||
end := m[1]
|
||||
|
||||
// If url ends in '.', it's very likely that it is not part of the
|
||||
// actual url but used to finish a sentence.
|
||||
// If the URL ends in '.', it's very likely that it is not part of the
|
||||
// actual URL but used to finish a sentence.
|
||||
if strings.HasSuffix(urlFull, ".") {
|
||||
end--
|
||||
urlFull = urlFull[:len(urlFull)-1]
|
||||
if hash != "" {
|
||||
hash = hash[:len(hash)-1]
|
||||
} else if query != "" {
|
||||
query = query[:len(query)-1]
|
||||
} else if text2 != "" {
|
||||
text2 = text2[:len(text2)-1]
|
||||
}
|
||||
}
|
||||
|
||||
text := text1 + textDots + text2
|
||||
|
||||
baseURLEnd := strings.Index(urlFull, repoSlug) + len(repoSlug)
|
||||
if len(ctx.Links.Base) > 0 && !strings.HasPrefix(ctx.Links.Base, urlFull[:baseURLEnd]) {
|
||||
text = repoSlug + "@" + text
|
||||
}
|
||||
|
||||
extra := ""
|
||||
if query != "" {
|
||||
query, err := url.ParseQuery(query)
|
||||
if err == nil && query.Has("files") {
|
||||
extra = query.Get("files")
|
||||
}
|
||||
}
|
||||
|
||||
if hash != "" {
|
||||
text += " (" + hash + ")"
|
||||
if extra != "" {
|
||||
extra += "#"
|
||||
}
|
||||
|
||||
extra += hash
|
||||
}
|
||||
|
||||
if extra != "" {
|
||||
text += " (" + extra + ")"
|
||||
}
|
||||
replaceContent(node, start, end, createCodeLink(urlFull, text, "compare"))
|
||||
node = node.NextSibling.NextSibling
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue