Keeping consistent between UI and API about combined commit status state and fix some bugs (#34562)

Extract from #34531 

## Move Commit status state to a standalone package

Move the state from `structs` to `commitstatus` package. It also
introduce `CommitStatusStates` so that the combine function could be
used from UI and API logic.

## Combined commit status Changed

This PR will follow Github's combined commit status. Before this PR,
every commit status could be a combined one.
According to
https://docs.github.com/en/rest/commits/statuses?apiVersion=2022-11-28#get-the-combined-status-for-a-specific-reference
> Additionally, a combined state is returned. The state is one of:
> failure if any of the contexts report as error or failure
> pending if there are no statuses or a context is pending
> success if the latest status for all contexts is success

This PR will follow that rule and remove the `NoBetterThan` logic. This
also fixes the inconsistent between UI and API. In the API convert
package, it has implemented this which is different from the UI. It also
fixed the missing `URL` and `CommitURL` in the API.

## `CalcCommitStatus` return nil if there is no commit statuses

The behavior of `CalcCommitStatus` is changed. If the parameter commit
statuses is empty, it will return nil. The reference places should check
the returned value themselves.

services/actions/commit_status.go

@@ -14,9 +14,9 @@ import (
 	git_model "code.gitea.io/gitea/models/git"
 	user_model "code.gitea.io/gitea/models/user"
 	actions_module "code.gitea.io/gitea/modules/actions"
+	"code.gitea.io/gitea/modules/commitstatus"
 	git "code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/log"
-	api "code.gitea.io/gitea/modules/structs"
 	webhook_module "code.gitea.io/gitea/modules/webhook"
 	commitstatus_service "code.gitea.io/gitea/services/repository/commitstatus"
 
@@ -147,18 +147,18 @@ func createCommitStatus(ctx context.Context, job *actions_model.ActionRunJob) er
 	return commitstatus_service.CreateCommitStatus(ctx, repo, creator, commitID.String(), &status)
 }
 
-func toCommitStatus(status actions_model.Status) api.CommitStatusState {
+func toCommitStatus(status actions_model.Status) commitstatus.CommitStatusState {
 	switch status {
 	case actions_model.StatusSuccess:
-		return api.CommitStatusSuccess
+		return commitstatus.CommitStatusSuccess
 	case actions_model.StatusFailure, actions_model.StatusCancelled:
-		return api.CommitStatusFailure
+		return commitstatus.CommitStatusFailure
 	case actions_model.StatusWaiting, actions_model.StatusBlocked, actions_model.StatusRunning:
-		return api.CommitStatusPending
+		return commitstatus.CommitStatusPending
 	case actions_model.StatusSkipped:
-		return api.CommitStatusSkipped
+		return commitstatus.CommitStatusSkipped
 	default:
-		return api.CommitStatusError
+		return commitstatus.CommitStatusError
 	}
 }
 

services/convert/status.go

@@ -5,6 +5,7 @@ package convert
 
 import (
 	"context"
+	"net/url"
 
 	git_model "code.gitea.io/gitea/models/git"
 	user_model "code.gitea.io/gitea/models/user"
@@ -32,39 +33,29 @@ func ToCommitStatus(ctx context.Context, status *git_model.CommitStatus) *api.Co
 	return apiStatus
 }
 
+func ToCommitStatuses(ctx context.Context, statuses []*git_model.CommitStatus) []*api.CommitStatus {
+	apiStatuses := make([]*api.CommitStatus, len(statuses))
+	for i, status := range statuses {
+		apiStatuses[i] = ToCommitStatus(ctx, status)
+	}
+	return apiStatuses
+}
+
 // ToCombinedStatus converts List of CommitStatus to a CombinedStatus
 func ToCombinedStatus(ctx context.Context, statuses []*git_model.CommitStatus, repo *api.Repository) *api.CombinedStatus {
 	if len(statuses) == 0 {
 		return nil
 	}
 
-	retStatus := &api.CombinedStatus{
-		SHA:        statuses[0].SHA,
+	combinedStatus := git_model.CalcCommitStatus(statuses)
+
+	return &api.CombinedStatus{
+		State:      combinedStatus.State,
+		Statuses:   ToCommitStatuses(ctx, statuses),
+		SHA:        combinedStatus.SHA,
 		TotalCount: len(statuses),
 		Repository: repo,
-		URL:        "", // never set or used?
-		State:      api.CommitStatusSuccess,
+		CommitURL:  repo.URL + "/commits/" + url.PathEscape(combinedStatus.SHA),
+		URL:        repo.URL + "/commits/" + url.PathEscape(combinedStatus.SHA) + "/status",
 	}
-
-	retStatus.Statuses = make([]*api.CommitStatus, 0, len(statuses))
-	for _, status := range statuses {
-		retStatus.Statuses = append(retStatus.Statuses, ToCommitStatus(ctx, status))
-		if status.State.HasHigherPriorityThan(retStatus.State) {
-			retStatus.State = status.State
-		}
-	}
-	// According to https://docs.github.com/en/rest/commits/statuses?apiVersion=2022-11-28#get-the-combined-status-for-a-specific-reference
-	// > Additionally, a combined state is returned. The state is one of:
-	// > failure if any of the contexts report as error or failure
-	// > pending if there are no statuses or a context is pending
-	// > success if the latest status for all contexts is success
-	switch retStatus.State {
-	case api.CommitStatusSkipped:
-		retStatus.State = api.CommitStatusSuccess // all skipped means success
-	case api.CommitStatusPending, api.CommitStatusSuccess:
-		// use the current state for pending or success
-	default:
-		retStatus.State = api.CommitStatusFailure // otherwise, it is a failure
-	}
-	return retStatus
 }

services/pull/commit_status.go

@@ -10,67 +10,56 @@ import (
 	"code.gitea.io/gitea/models/db"
 	git_model "code.gitea.io/gitea/models/git"
 	issues_model "code.gitea.io/gitea/models/issues"
+	"code.gitea.io/gitea/modules/commitstatus"
 	"code.gitea.io/gitea/modules/gitrepo"
 	"code.gitea.io/gitea/modules/log"
-	"code.gitea.io/gitea/modules/structs"
 
 	"github.com/gobwas/glob"
 	"github.com/pkg/errors"
 )
 
 // MergeRequiredContextsCommitStatus returns a commit status state for given required contexts
-func MergeRequiredContextsCommitStatus(commitStatuses []*git_model.CommitStatus, requiredContexts []string) structs.CommitStatusState {
-	// matchedCount is the number of `CommitStatus.Context` that match any context of `requiredContexts`
-	matchedCount := 0
-	returnedStatus := structs.CommitStatusSuccess
+func MergeRequiredContextsCommitStatus(commitStatuses []*git_model.CommitStatus, requiredContexts []string) commitstatus.CommitStatusState {
+	if len(commitStatuses) == 0 {
+		return commitstatus.CommitStatusPending
+	}
 
-	if len(requiredContexts) > 0 {
-		requiredContextsGlob := make(map[string]glob.Glob, len(requiredContexts))
-		for _, ctx := range requiredContexts {
-			if gp, err := glob.Compile(ctx); err != nil {
-				log.Error("glob.Compile %s failed. Error: %v", ctx, err)
-			} else {
-				requiredContextsGlob[ctx] = gp
-			}
+	if len(requiredContexts) == 0 {
+		return git_model.CalcCommitStatus(commitStatuses).State
+	}
+
+	requiredContextsGlob := make(map[string]glob.Glob, len(requiredContexts))
+	for _, ctx := range requiredContexts {
+		if gp, err := glob.Compile(ctx); err != nil {
+			log.Error("glob.Compile %s failed. Error: %v", ctx, err)
+		} else {
+			requiredContextsGlob[ctx] = gp
 		}
+	}
 
-		for _, gp := range requiredContextsGlob {
-			var targetStatus structs.CommitStatusState
-			for _, commitStatus := range commitStatuses {
-				if gp.Match(commitStatus.Context) {
-					targetStatus = commitStatus.State
-					matchedCount++
-					break
-				}
-			}
-
-			// If required rule not match any action, then it is pending
-			if targetStatus == "" {
-				if structs.CommitStatusPending.HasHigherPriorityThan(returnedStatus) {
-					returnedStatus = structs.CommitStatusPending
-				}
+	requiredCommitStatuses := make([]*git_model.CommitStatus, 0, len(commitStatuses))
+	for _, gp := range requiredContextsGlob {
+		for _, commitStatus := range commitStatuses {
+			if gp.Match(commitStatus.Context) {
+				requiredCommitStatuses = append(requiredCommitStatuses, commitStatus)
 				break
 			}
-
-			if targetStatus.HasHigherPriorityThan(returnedStatus) {
-				returnedStatus = targetStatus
-			}
 		}
 	}
-
-	if matchedCount == 0 && returnedStatus == structs.CommitStatusSuccess {
-		if len(commitStatuses) == 0 {
-			// "no statuses" should mean "pending"
-			return structs.CommitStatusPending
-		}
-		status := git_model.CalcCommitStatus(commitStatuses)
-		if status.State == structs.CommitStatusSkipped {
-			return structs.CommitStatusSuccess // if all statuses are skipped, return success
-		}
-		return status.State
+	if len(requiredCommitStatuses) == 0 {
+		return commitstatus.CommitStatusPending
 	}
 
-	return returnedStatus
+	returnedStatus := git_model.CalcCommitStatus(requiredCommitStatuses).State
+	if len(requiredCommitStatuses) == len(requiredContexts) {
+		return returnedStatus
+	}
+
+	if returnedStatus == commitstatus.CommitStatusFailure {
+		return commitstatus.CommitStatusFailure
+	}
+	// even if part of success, return pending
+	return commitstatus.CommitStatusPending
 }
 
 // IsPullCommitStatusPass returns if all required status checks PASS
@@ -91,7 +80,7 @@ func IsPullCommitStatusPass(ctx context.Context, pr *issues_model.PullRequest) (
 }
 
 // GetPullRequestCommitStatusState returns pull request merged commit status state
-func GetPullRequestCommitStatusState(ctx context.Context, pr *issues_model.PullRequest) (structs.CommitStatusState, error) {
+func GetPullRequestCommitStatusState(ctx context.Context, pr *issues_model.PullRequest) (commitstatus.CommitStatusState, error) {
 	// Ensure HeadRepo is loaded
 	if err := pr.LoadHeadRepo(ctx); err != nil {
 		return "", errors.Wrap(err, "LoadHeadRepo")

services/pull/commit_status_test.go

@@ -8,7 +8,7 @@ import (
 	"testing"
 
 	git_model "code.gitea.io/gitea/models/git"
-	"code.gitea.io/gitea/modules/structs"
+	"code.gitea.io/gitea/modules/commitstatus"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -17,64 +17,64 @@ func TestMergeRequiredContextsCommitStatus(t *testing.T) {
 	cases := []struct {
 		commitStatuses   []*git_model.CommitStatus
 		requiredContexts []string
-		expected         structs.CommitStatusState
+		expected         commitstatus.CommitStatusState
 	}{
 		{
 			commitStatuses:   []*git_model.CommitStatus{},
 			requiredContexts: []string{},
-			expected:         structs.CommitStatusPending,
+			expected:         commitstatus.CommitStatusPending,
 		},
 		{
 			commitStatuses: []*git_model.CommitStatus{
-				{Context: "Build xxx", State: structs.CommitStatusSkipped},
+				{Context: "Build xxx", State: commitstatus.CommitStatusSkipped},
 			},
 			requiredContexts: []string{"Build*"},
-			expected:         structs.CommitStatusSuccess,
+			expected:         commitstatus.CommitStatusSuccess,
 		},
 		{
 			commitStatuses: []*git_model.CommitStatus{
-				{Context: "Build 1", State: structs.CommitStatusSkipped},
-				{Context: "Build 2", State: structs.CommitStatusSuccess},
-				{Context: "Build 3", State: structs.CommitStatusSuccess},
+				{Context: "Build 1", State: commitstatus.CommitStatusSkipped},
+				{Context: "Build 2", State: commitstatus.CommitStatusSuccess},
+				{Context: "Build 3", State: commitstatus.CommitStatusSuccess},
 			},
 			requiredContexts: []string{"Build*"},
-			expected:         structs.CommitStatusSuccess,
+			expected:         commitstatus.CommitStatusSuccess,
 		},
 		{
 			commitStatuses: []*git_model.CommitStatus{
-				{Context: "Build 1", State: structs.CommitStatusSuccess},
-				{Context: "Build 2", State: structs.CommitStatusSuccess},
-				{Context: "Build 2t", State: structs.CommitStatusPending},
+				{Context: "Build 1", State: commitstatus.CommitStatusSuccess},
+				{Context: "Build 2", State: commitstatus.CommitStatusSuccess},
+				{Context: "Build 2t", State: commitstatus.CommitStatusPending},
 			},
 			requiredContexts: []string{"Build*", "Build 2t*"},
-			expected:         structs.CommitStatusPending,
+			expected:         commitstatus.CommitStatusPending,
 		},
 		{
 			commitStatuses: []*git_model.CommitStatus{
-				{Context: "Build 1", State: structs.CommitStatusSuccess},
-				{Context: "Build 2", State: structs.CommitStatusSuccess},
-				{Context: "Build 2t", State: structs.CommitStatusFailure},
+				{Context: "Build 1", State: commitstatus.CommitStatusSuccess},
+				{Context: "Build 2", State: commitstatus.CommitStatusSuccess},
+				{Context: "Build 2t", State: commitstatus.CommitStatusFailure},
 			},
 			requiredContexts: []string{"Build*", "Build 2t*"},
-			expected:         structs.CommitStatusFailure,
+			expected:         commitstatus.CommitStatusFailure,
 		},
 		{
 			commitStatuses: []*git_model.CommitStatus{
-				{Context: "Build 1", State: structs.CommitStatusSuccess},
-				{Context: "Build 2", State: structs.CommitStatusSuccess},
-				{Context: "Build 2t", State: structs.CommitStatusSuccess},
+				{Context: "Build 1", State: commitstatus.CommitStatusSuccess},
+				{Context: "Build 2", State: commitstatus.CommitStatusSuccess},
+				{Context: "Build 2t", State: commitstatus.CommitStatusSuccess},
 			},
 			requiredContexts: []string{"Build*", "Build 2t*", "Build 3*"},
-			expected:         structs.CommitStatusPending,
+			expected:         commitstatus.CommitStatusPending,
 		},
 		{
 			commitStatuses: []*git_model.CommitStatus{
-				{Context: "Build 1", State: structs.CommitStatusSuccess},
-				{Context: "Build 2", State: structs.CommitStatusSuccess},
-				{Context: "Build 2t", State: structs.CommitStatusSuccess},
+				{Context: "Build 1", State: commitstatus.CommitStatusSuccess},
+				{Context: "Build 2", State: commitstatus.CommitStatusSuccess},
+				{Context: "Build 2t", State: commitstatus.CommitStatusSuccess},
 			},
 			requiredContexts: []string{"Build*", "Build *", "Build 2t*", "Build 1*"},
-			expected:         structs.CommitStatusSuccess,
+			expected:         commitstatus.CommitStatusSuccess,
 		},
 	}
 	for i, c := range cases {

services/repository/commitstatus/commitstatus.go

@@ -14,12 +14,12 @@ import (
 	repo_model "code.gitea.io/gitea/models/repo"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/cache"
+	"code.gitea.io/gitea/modules/commitstatus"
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/gitrepo"
 	"code.gitea.io/gitea/modules/json"
 	"code.gitea.io/gitea/modules/log"
 	repo_module "code.gitea.io/gitea/modules/repository"
-	api "code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/services/notify"
 )
 
@@ -47,7 +47,7 @@ func getCommitStatusCache(repoID int64, branchName string) *commitStatusCacheVal
 	return nil
 }
 
-func updateCommitStatusCache(repoID int64, branchName string, state api.CommitStatusState, targetURL string) error {
+func updateCommitStatusCache(repoID int64, branchName string, state commitstatus.CommitStatusState, targetURL string) error {
 	c := cache.GetCache()
 	bs, err := json.Marshal(commitStatusCacheValue{
 		State:     state.String(),
@@ -127,7 +127,7 @@ func FindReposLastestCommitStatuses(ctx context.Context, repos []*repo_model.Rep
 	for i, repo := range repos {
 		if cv := getCommitStatusCache(repo.ID, repo.DefaultBranch); cv != nil {
 			results[i] = &git_model.CommitStatus{
-				State:     api.CommitStatusState(cv.State),
+				State:     commitstatus.CommitStatusState(cv.State),
 				TargetURL: cv.TargetURL,
 			}
 		} else {