From cb7ba8969db3b6750221fa955e4e8b2968a7eaed Mon Sep 17 00:00:00 2001 From: oliverpool <3864879+oliverpool@users.noreply.github.com> Date: Fri, 12 May 2023 20:14:41 +0200 Subject: [PATCH] fix: release page for empty or non-existing target (#24659) Backport #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. _contributed in the context of @forgejo_ --- 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 abf91bc4b..73311da87 100644 --- a/models/repo/release.go +++ b/models/repo/release.go @@ -71,6 +71,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 15b00c33e..f94f14166 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 ca4e3b378..e14f9c698 100644 --- a/templates/repo/release/list.tmpl +++ b/templates/repo/release/list.tmpl @@ -47,7 +47,7 @@ {{end}} | {{end}} - {{$.locale.Tr "repo.release.ahead.commits" .NumCommitsBehind | Str2html}} {{$.locale.Tr "repo.release.ahead.target" $.DefaultBranch}} + {{$.locale.Tr "repo.release.ahead.commits" .NumCommitsBehind | Str2html}} {{$.locale.Tr "repo.release.ahead.target" .TargetBehind}}

{{if $.Permission.CanRead $.UnitTypeCode}} @@ -96,7 +96,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}}

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",