Skip to content

Commit 8030614

Browse files
oliverpoololiverpool
authored
fix: release page for empty or non-existing target (#24470)
Fixes #24145 To solve the bug, I added a "computed" `TargetBehind` field to the `Release` model, which indicates the target branch of a release. This is particularly useful if the target branch was deleted in the meantime (or is empty). I also did a micro-optimization in `calReleaseNumCommitsBehind`. Instead of checking that a branch exists and then call `GetBranchCommit`, I immediately call `GetBranchCommit` and handle the `git.ErrNotExist` error. This optimization is covered by the added unit test.
1 parent 5930ab5 commit 8030614

File tree

6 files changed

+107
-17
lines changed

6 files changed

+107
-17
lines changed

‎models/fixtures/release.yml

+28
Original file line numberDiff line numberDiff line change
@@ -108,3 +108,31 @@
108108
is_prerelease: false
109109
is_tag: false
110110
created_unix: 946684803
111+
112+
- id: 9
113+
repo_id: 57
114+
publisher_id: 2
115+
tag_name: "non-existing-target-branch"
116+
lower_tag_name: "non-existing-target-branch"
117+
target: "non-existing"
118+
title: "non-existing-target-branch"
119+
sha1: "cef06e48f2642cd0dc9597b4bea09f4b3f74aad6"
120+
num_commits: 5
121+
is_draft: false
122+
is_prerelease: false
123+
is_tag: false
124+
created_unix: 946684803
125+
126+
- id: 10
127+
repo_id: 57
128+
publisher_id: 2
129+
tag_name: "empty-target-branch"
130+
lower_tag_name: "empty-target-branch"
131+
target: ""
132+
title: "empty-target-branch"
133+
sha1: "cef06e48f2642cd0dc9597b4bea09f4b3f74aad6"
134+
num_commits: 5
135+
is_draft: false
136+
is_prerelease: false
137+
is_tag: false
138+
created_unix: 946684803

‎models/repo/release.go

+1
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ type Release struct {
7272
OriginalAuthorID int64 `xorm:"index"`
7373
LowerTagName string
7474
Target string
75+
TargetBehind string `xorm:"-"` // to handle non-existing or empty target
7576
Title string
7677
Sha1 string `xorm:"VARCHAR(40)"`
7778
NumCommits int64

‎routers/web/repo/release.go

+22-12
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
package repo
66

77
import (
8+
"errors"
89
"fmt"
910
"net/http"
1011
"strings"
@@ -16,6 +17,7 @@ import (
1617
user_model "code.gitea.io/gitea/models/user"
1718
"code.gitea.io/gitea/modules/base"
1819
"code.gitea.io/gitea/modules/context"
20+
"code.gitea.io/gitea/modules/git"
1921
"code.gitea.io/gitea/modules/log"
2022
"code.gitea.io/gitea/modules/markup"
2123
"code.gitea.io/gitea/modules/markup/markdown"
@@ -36,24 +38,32 @@ const (
3638

3739
// calReleaseNumCommitsBehind calculates given release has how many commits behind release target.
3840
func calReleaseNumCommitsBehind(repoCtx *context.Repository, release *repo_model.Release, countCache map[string]int64) error {
39-
// Get count if not exists
40-
if _, ok := countCache[release.Target]; !ok {
41-
// short-circuit for the default branch
42-
if repoCtx.Repository.DefaultBranch == release.Target || repoCtx.GitRepo.IsBranchExist(release.Target) {
43-
commit, err := repoCtx.GitRepo.GetBranchCommit(release.Target)
44-
if err != nil {
41+
target := release.Target
42+
if target == "" {
43+
target = repoCtx.Repository.DefaultBranch
44+
}
45+
// Get count if not cached
46+
if _, ok := countCache[target]; !ok {
47+
commit, err := repoCtx.GitRepo.GetBranchCommit(target)
48+
if err != nil {
49+
var errNotExist git.ErrNotExist
50+
if target == repoCtx.Repository.DefaultBranch || !errors.As(err, &errNotExist) {
4551
return fmt.Errorf("GetBranchCommit: %w", err)
4652
}
47-
countCache[release.Target], err = commit.CommitsCount()
53+
// fallback to default branch
54+
target = repoCtx.Repository.DefaultBranch
55+
commit, err = repoCtx.GitRepo.GetBranchCommit(target)
4856
if err != nil {
49-
return fmt.Errorf("CommitsCount: %w", err)
57+
return fmt.Errorf("GetBranchCommit(DefaultBranch): %w", err)
5058
}
51-
} else {
52-
// Use NumCommits of the newest release on that target
53-
countCache[release.Target] = release.NumCommits
59+
}
60+
countCache[target], err = commit.CommitsCount()
61+
if err != nil {
62+
return fmt.Errorf("CommitsCount: %w", err)
5463
}
5564
}
56-
release.NumCommitsBehind = countCache[release.Target] - release.NumCommits
65+
release.NumCommitsBehind = countCache[target] - release.NumCommits
66+
release.TargetBehind = target
5767
return nil
5868
}
5969

‎routers/web/repo/release_test.go

+47
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@ import (
1111
"code.gitea.io/gitea/modules/test"
1212
"code.gitea.io/gitea/modules/web"
1313
"code.gitea.io/gitea/services/forms"
14+
15+
"github.com/stretchr/testify/assert"
1416
)
1517

1618
func TestNewReleasePost(t *testing.T) {
@@ -62,3 +64,48 @@ func TestNewReleasePost(t *testing.T) {
6264
ctx.Repo.GitRepo.Close()
6365
}
6466
}
67+
68+
func TestNewReleasesList(t *testing.T) {
69+
unittest.PrepareTestEnv(t)
70+
ctx := test.MockContext(t, "user2/repo-release/releases")
71+
test.LoadUser(t, ctx, 2)
72+
test.LoadRepo(t, ctx, 57)
73+
test.LoadGitRepo(t, ctx)
74+
t.Cleanup(func() { ctx.Repo.GitRepo.Close() })
75+
76+
Releases(ctx)
77+
releases := ctx.Data["Releases"].([]*repo_model.Release)
78+
type computedFields struct {
79+
NumCommitsBehind int64
80+
TargetBehind string
81+
}
82+
expectedComputation := map[string]computedFields{
83+
"v1.0": {
84+
NumCommitsBehind: 3,
85+
TargetBehind: "main",
86+
},
87+
"v1.1": {
88+
NumCommitsBehind: 1,
89+
TargetBehind: "main",
90+
},
91+
"v2.0": {
92+
NumCommitsBehind: 0,
93+
TargetBehind: "main",
94+
},
95+
"non-existing-target-branch": {
96+
NumCommitsBehind: 1,
97+
TargetBehind: "main",
98+
},
99+
"empty-target-branch": {
100+
NumCommitsBehind: 1,
101+
TargetBehind: "main",
102+
},
103+
}
104+
for _, r := range releases {
105+
actual := computedFields{
106+
NumCommitsBehind: r.NumCommitsBehind,
107+
TargetBehind: r.TargetBehind,
108+
}
109+
assert.Equal(t, expectedComputation[r.TagName], actual, "wrong computed fields for %s: %#v", r.TagName, r)
110+
}
111+
}

‎templates/repo/release/list.tmpl

+2-2
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@
5656
{{end}}
5757
|
5858
{{end}}
59-
<span class="ahead"><a href="{{$.RepoLink}}/compare/{{.TagName | PathEscapeSegments}}{{if .Target}}...{{.Target | PathEscapeSegments}}{{end}}">{{$.locale.Tr "repo.release.ahead.commits" .NumCommitsBehind | Str2html}}</a> {{$.locale.Tr "repo.tag.ahead.target" $.DefaultBranch}}</span>
59+
<span class="ahead"><a href="{{$.RepoLink}}/compare/{{.TagName | PathEscapeSegments}}...{{.TargetBehind | PathEscapeSegments}}">{{$.locale.Tr "repo.release.ahead.commits" .NumCommitsBehind | Str2html}}</a> {{$.locale.Tr "repo.tag.ahead.target" .TargetBehind}}</span>
6060
</p>
6161
{{else}}
6262
<p class="text grey">
@@ -77,7 +77,7 @@
7777
<span class="time">{{TimeSinceUnix .CreatedUnix $.locale}}</span>
7878
{{end}}
7979
{{if not .IsDraft}}
80-
| <span class="ahead"><a href="{{$.RepoLink}}/compare/{{.TagName | PathEscapeSegments}}...{{.Target | PathEscapeSegments}}">{{$.locale.Tr "repo.release.ahead.commits" .NumCommitsBehind | Str2html}}</a> {{$.locale.Tr "repo.release.ahead.target" .Target}}</span>
80+
| <span class="ahead"><a href="{{$.RepoLink}}/compare/{{.TagName | PathEscapeSegments}}...{{.TargetBehind | PathEscapeSegments}}">{{$.locale.Tr "repo.release.ahead.commits" .NumCommitsBehind | Str2html}}</a> {{$.locale.Tr "repo.release.ahead.target" .TargetBehind}}</span>
8181
{{end}}
8282
</p>
8383
{{end}}

‎tests/integration/release_test.go

+7-3
Original file line numberDiff line numberDiff line change
@@ -143,10 +143,10 @@ func TestViewReleaseListNoLogin(t *testing.T) {
143143

144144
htmlDoc := NewHTMLParser(t, rsp.Body)
145145
releases := htmlDoc.Find("#release-list li.ui.grid")
146-
assert.Equal(t, 3, releases.Length())
146+
assert.Equal(t, 5, releases.Length())
147147

148-
links := make([]string, 0, 3)
149-
commitsToMain := make([]string, 0, 3)
148+
links := make([]string, 0, 5)
149+
commitsToMain := make([]string, 0, 5)
150150
releases.Each(func(i int, s *goquery.Selection) {
151151
link, exist := s.Find(".release-list-title a").Attr("href")
152152
if !exist {
@@ -158,11 +158,15 @@ func TestViewReleaseListNoLogin(t *testing.T) {
158158
})
159159

160160
assert.EqualValues(t, []string{
161+
"/user2/repo-release/releases/tag/empty-target-branch",
162+
"/user2/repo-release/releases/tag/non-existing-target-branch",
161163
"/user2/repo-release/releases/tag/v2.0",
162164
"/user2/repo-release/releases/tag/v1.1",
163165
"/user2/repo-release/releases/tag/v1.0",
164166
}, links)
165167
assert.EqualValues(t, []string{
168+
"1 commits", // like v1.1
169+
"1 commits", // like v1.1
166170
"0 commits",
167171
"1 commits", // should be 3 commits ahead and 2 commits behind, but not implemented yet
168172
"3 commits",

0 commit comments

Comments
 (0)