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

@@ -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

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

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
+	target := release.Target
-	if _, ok := countCache[release.Target]; !ok {
+	if target == "" {
-		// short-circuit for the default branch
+		target = repoCtx.Repository.DefaultBranch
-		if repoCtx.Repository.DefaultBranch == release.Target || repoCtx.GitRepo.IsBranchExist(release.Target) {
+	}
-			commit, err := repoCtx.GitRepo.GetBranchCommit(release.Target)
+	// Get count if not cached
-			if err != nil {
+	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[target], err = commit.CommitsCount()
-			countCache[release.Target] = release.NumCommits
+		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
 }
 

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)
+	}
+}

templates/repo/release/list.tmpl

@@ -56,7 +56,7 @@
 								{{end}}
 								|
 								{{end}}
-								<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>
+								<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>
 							</p>
 						{{else}}
 							<p class="text grey">
@@ -77,7 +77,7 @@
 									<span class="time">{{TimeSinceUnix .CreatedUnix $.locale}}</span>
 								{{end}}
 								{{if not .IsDraft}}
-									| <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>
+									| <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>
 								{{end}}
 							</p>
 						{{end}}

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)
+	links := make([]string, 0, 5)
-	commitsToMain := make([]string, 0, 3)
+	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",