From 8030614386b5d3fa02dc294446a344d274b04a26 Mon Sep 17 00:00:00 2001
From: oliverpool <3864879+oliverpool@users.noreply.github.com>
Date: Wed, 10 May 2023 05:43:55 +0200
Subject: [PATCH] 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.
---
models/fixtures/release.yml | 28 ++++++++++++++++++
models/repo/release.go | 1 +
routers/web/repo/release.go | 34 ++++++++++++++--------
routers/web/repo/release_test.go | 47 +++++++++++++++++++++++++++++++
templates/repo/release/list.tmpl | 4 +--
tests/integration/release_test.go | 10 +++++--
6 files changed, 107 insertions(+), 17 deletions(-)
diff --git a/models/fixtures/release.yml b/models/fixtures/release.yml
index 6d09401eb..4ed7df440 100644
--- a/models/fixtures/release.yml
+++ b/models/fixtures/release.yml
@@ -108,3 +108,31 @@
is_prerelease: false
is_tag: false
created_unix: 946684803
+
+- id: 9
+ repo_id: 57
+ publisher_id: 2
+ tag_name: "non-existing-target-branch"
+ lower_tag_name: "non-existing-target-branch"
+ target: "non-existing"
+ title: "non-existing-target-branch"
+ sha1: "cef06e48f2642cd0dc9597b4bea09f4b3f74aad6"
+ num_commits: 5
+ is_draft: false
+ is_prerelease: false
+ is_tag: false
+ created_unix: 946684803
+
+- id: 10
+ repo_id: 57
+ publisher_id: 2
+ tag_name: "empty-target-branch"
+ lower_tag_name: "empty-target-branch"
+ target: ""
+ title: "empty-target-branch"
+ sha1: "cef06e48f2642cd0dc9597b4bea09f4b3f74aad6"
+ num_commits: 5
+ is_draft: false
+ is_prerelease: false
+ is_tag: false
+ created_unix: 946684803
diff --git a/models/repo/release.go b/models/repo/release.go
index 75eb27f07..b77490584 100644
--- a/models/repo/release.go
+++ b/models/repo/release.go
@@ -72,6 +72,7 @@ type Release struct {
OriginalAuthorID int64 `xorm:"index"`
LowerTagName string
Target string
+ TargetBehind string `xorm:"-"` // to handle non-existing or empty target
Title string
Sha1 string `xorm:"VARCHAR(40)"`
NumCommits int64
diff --git a/routers/web/repo/release.go b/routers/web/repo/release.go
index 689994c14..afba1f18b 100644
--- a/routers/web/repo/release.go
+++ b/routers/web/repo/release.go
@@ -5,6 +5,7 @@
package repo
import (
+ "errors"
"fmt"
"net/http"
"strings"
@@ -16,6 +17,7 @@ import (
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/base"
"code.gitea.io/gitea/modules/context"
+ "code.gitea.io/gitea/modules/git"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/markup"
"code.gitea.io/gitea/modules/markup/markdown"
@@ -36,24 +38,32 @@ const (
// calReleaseNumCommitsBehind calculates given release has how many commits behind release target.
func calReleaseNumCommitsBehind(repoCtx *context.Repository, release *repo_model.Release, countCache map[string]int64) error {
- // Get count if not exists
- if _, ok := countCache[release.Target]; !ok {
- // short-circuit for the default branch
- if repoCtx.Repository.DefaultBranch == release.Target || repoCtx.GitRepo.IsBranchExist(release.Target) {
- commit, err := repoCtx.GitRepo.GetBranchCommit(release.Target)
- if err != nil {
+ target := release.Target
+ if target == "" {
+ target = repoCtx.Repository.DefaultBranch
+ }
+ // Get count if not cached
+ if _, ok := countCache[target]; !ok {
+ commit, err := repoCtx.GitRepo.GetBranchCommit(target)
+ if err != nil {
+ var errNotExist git.ErrNotExist
+ if target == repoCtx.Repository.DefaultBranch || !errors.As(err, &errNotExist) {
return fmt.Errorf("GetBranchCommit: %w", err)
}
- countCache[release.Target], err = commit.CommitsCount()
+ // fallback to default branch
+ target = repoCtx.Repository.DefaultBranch
+ commit, err = repoCtx.GitRepo.GetBranchCommit(target)
if err != nil {
- return fmt.Errorf("CommitsCount: %w", err)
+ return fmt.Errorf("GetBranchCommit(DefaultBranch): %w", err)
}
- } else {
- // Use NumCommits of the newest release on that target
- countCache[release.Target] = release.NumCommits
+ }
+ countCache[target], err = commit.CommitsCount()
+ if err != nil {
+ return fmt.Errorf("CommitsCount: %w", err)
}
}
- release.NumCommitsBehind = countCache[release.Target] - release.NumCommits
+ release.NumCommitsBehind = countCache[target] - release.NumCommits
+ release.TargetBehind = target
return nil
}
diff --git a/routers/web/repo/release_test.go b/routers/web/repo/release_test.go
index 81ae58178..9ec1b4d34 100644
--- a/routers/web/repo/release_test.go
+++ b/routers/web/repo/release_test.go
@@ -11,6 +11,8 @@ import (
"code.gitea.io/gitea/modules/test"
"code.gitea.io/gitea/modules/web"
"code.gitea.io/gitea/services/forms"
+
+ "github.com/stretchr/testify/assert"
)
func TestNewReleasePost(t *testing.T) {
@@ -62,3 +64,48 @@ func TestNewReleasePost(t *testing.T) {
ctx.Repo.GitRepo.Close()
}
}
+
+func TestNewReleasesList(t *testing.T) {
+ unittest.PrepareTestEnv(t)
+ ctx := test.MockContext(t, "user2/repo-release/releases")
+ test.LoadUser(t, ctx, 2)
+ test.LoadRepo(t, ctx, 57)
+ test.LoadGitRepo(t, ctx)
+ t.Cleanup(func() { ctx.Repo.GitRepo.Close() })
+
+ Releases(ctx)
+ releases := ctx.Data["Releases"].([]*repo_model.Release)
+ type computedFields struct {
+ NumCommitsBehind int64
+ TargetBehind string
+ }
+ expectedComputation := map[string]computedFields{
+ "v1.0": {
+ NumCommitsBehind: 3,
+ TargetBehind: "main",
+ },
+ "v1.1": {
+ NumCommitsBehind: 1,
+ TargetBehind: "main",
+ },
+ "v2.0": {
+ NumCommitsBehind: 0,
+ TargetBehind: "main",
+ },
+ "non-existing-target-branch": {
+ NumCommitsBehind: 1,
+ TargetBehind: "main",
+ },
+ "empty-target-branch": {
+ NumCommitsBehind: 1,
+ TargetBehind: "main",
+ },
+ }
+ for _, r := range releases {
+ actual := computedFields{
+ NumCommitsBehind: r.NumCommitsBehind,
+ TargetBehind: r.TargetBehind,
+ }
+ assert.Equal(t, expectedComputation[r.TagName], actual, "wrong computed fields for %s: %#v", r.TagName, r)
+ }
+}
diff --git a/templates/repo/release/list.tmpl b/templates/repo/release/list.tmpl
index be0976c1a..2dcb01241 100644
--- a/templates/repo/release/list.tmpl
+++ b/templates/repo/release/list.tmpl
@@ -56,7 +56,7 @@
{{end}}
|
{{end}}
- {{$.locale.Tr "repo.release.ahead.commits" .NumCommitsBehind | Str2html}} {{$.locale.Tr "repo.tag.ahead.target" $.DefaultBranch}}
+ {{$.locale.Tr "repo.release.ahead.commits" .NumCommitsBehind | Str2html}} {{$.locale.Tr "repo.tag.ahead.target" .TargetBehind}}
{{else}}
@@ -77,7 +77,7 @@
{{TimeSinceUnix .CreatedUnix $.locale}}
{{end}}
{{if not .IsDraft}}
- | {{$.locale.Tr "repo.release.ahead.commits" .NumCommitsBehind | Str2html}} {{$.locale.Tr "repo.release.ahead.target" .Target}}
+ | {{$.locale.Tr "repo.release.ahead.commits" .NumCommitsBehind | Str2html}} {{$.locale.Tr "repo.release.ahead.target" .TargetBehind}}
{{end}}
{{end}}
diff --git a/tests/integration/release_test.go b/tests/integration/release_test.go
index 8e3f96c93..8de761ea6 100644
--- a/tests/integration/release_test.go
+++ b/tests/integration/release_test.go
@@ -143,10 +143,10 @@ func TestViewReleaseListNoLogin(t *testing.T) {
htmlDoc := NewHTMLParser(t, rsp.Body)
releases := htmlDoc.Find("#release-list li.ui.grid")
- assert.Equal(t, 3, releases.Length())
+ assert.Equal(t, 5, releases.Length())
- links := make([]string, 0, 3)
- commitsToMain := make([]string, 0, 3)
+ links := make([]string, 0, 5)
+ commitsToMain := make([]string, 0, 5)
releases.Each(func(i int, s *goquery.Selection) {
link, exist := s.Find(".release-list-title a").Attr("href")
if !exist {
@@ -158,11 +158,15 @@ func TestViewReleaseListNoLogin(t *testing.T) {
})
assert.EqualValues(t, []string{
+ "/user2/repo-release/releases/tag/empty-target-branch",
+ "/user2/repo-release/releases/tag/non-existing-target-branch",
"/user2/repo-release/releases/tag/v2.0",
"/user2/repo-release/releases/tag/v1.1",
"/user2/repo-release/releases/tag/v1.0",
}, links)
assert.EqualValues(t, []string{
+ "1 commits", // like v1.1
+ "1 commits", // like v1.1
"0 commits",
"1 commits", // should be 3 commits ahead and 2 commits behind, but not implemented yet
"3 commits",