From 31e81156c84f8c8e0194d3ac544856f661e7b60f Mon Sep 17 00:00:00 2001
From: wxiaoguang <wxiaoguang@gmail.com>
Date: Wed, 28 May 2025 05:35:43 +0800
Subject: [PATCH] fine tune

---
 models/git/commit_status.go         |  1 +
 models/git/commit_status_summary.go |  6 +++++-
 services/convert/status.go          | 12 ++++++++----
 services/pull/commit_status.go      | 17 ++++++++---------
 services/pull/commit_status_test.go |  2 +-
 5 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/models/git/commit_status.go b/models/git/commit_status.go
index b631943a9c..2e765391b8 100644
--- a/models/git/commit_status.go
+++ b/models/git/commit_status.go
@@ -247,6 +247,7 @@ func CalcCommitStatus(statuses []*CommitStatus) *CommitStatus {
 			lastStatus = statuses[0]
 		} else {
 			// FIXME: another bad case: if the "statuses" slice is empty, the returned value is an invalid CommitStatus, all its fields are empty.
+			// Frontend code (tmpl&vue) sometimes depend on the empty fields to skip rendering commit status elements (need to double check in the future)
 			lastStatus = &CommitStatus{}
 		}
 	}
diff --git a/models/git/commit_status_summary.go b/models/git/commit_status_summary.go
index a3f440fd86..774e49bb98 100644
--- a/models/git/commit_status_summary.go
+++ b/models/git/commit_status_summary.go
@@ -59,7 +59,11 @@ func UpdateCommitStatusSummary(ctx context.Context, repoID int64, sha string) er
 	if err != nil {
 		return err
 	}
-	state := CalcCommitStatus(commitStatuses)
+	// it guarantees that commitStatuses is not empty because this function is always called after a commit status is created
+	if len(commitStatuses) == 0 {
+		setting.PanicInDevOrTesting("no commit statuses found for repo %d and sha %s", repoID, sha)
+	}
+	state := CalcCommitStatus(commitStatuses) // non-empty commitStatuses is guaranteed
 	// mysql will return 0 when update a record which state hasn't been changed which behaviour is different from other database,
 	// so we need to use insert in on duplicate
 	if setting.Database.Type.IsMySQL() {
diff --git a/services/convert/status.go b/services/convert/status.go
index cfad684a02..4fffbfbe5e 100644
--- a/services/convert/status.go
+++ b/services/convert/status.go
@@ -42,7 +42,7 @@ func ToCombinedStatus(ctx context.Context, statuses []*git_model.CommitStatus, r
 		SHA:        statuses[0].SHA,
 		TotalCount: len(statuses),
 		Repository: repo,
-		URL:        "",
+		URL:        "", // never set or used?
 		State:      api.CommitStatusSuccess,
 	}
 
@@ -58,9 +58,13 @@ func ToCombinedStatus(ctx context.Context, statuses []*git_model.CommitStatus, r
 	// > 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
-	if retStatus.State.IsError() {
-		retStatus.State = api.CommitStatusFailure
+	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
 }
diff --git a/services/pull/commit_status.go b/services/pull/commit_status.go
index f18274cde5..80d798734f 100644
--- a/services/pull/commit_status.go
+++ b/services/pull/commit_status.go
@@ -59,16 +59,15 @@ func MergeRequiredContextsCommitStatus(commitStatuses []*git_model.CommitStatus,
 	}
 
 	if matchedCount == 0 && returnedStatus == structs.CommitStatusSuccess {
-		status := git_model.CalcCommitStatus(commitStatuses)
-		if status != nil {
-			// FIXME: this check is not right, "status" can never be nil, but its fields can be empty if commitStatuses is empty
-			// here is just a quick patch to make it overall right.
-			if status.State == "" || status.State == structs.CommitStatusSkipped {
-				return structs.CommitStatusSuccess
-			}
-			return status.State
+		if len(commitStatuses) == 0 {
+			// "no statuses" should mean "pending"
+			return structs.CommitStatusPending
 		}
-		return structs.CommitStatusSuccess
+		status := git_model.CalcCommitStatus(commitStatuses)
+		if status.State == structs.CommitStatusSkipped {
+			return structs.CommitStatusSuccess // if all statuses are skipped, return success
+		}
+		return status.State
 	}
 
 	return returnedStatus
diff --git a/services/pull/commit_status_test.go b/services/pull/commit_status_test.go
index 11804e143a..9cb20d6c5d 100644
--- a/services/pull/commit_status_test.go
+++ b/services/pull/commit_status_test.go
@@ -22,7 +22,7 @@ func TestMergeRequiredContextsCommitStatus(t *testing.T) {
 		{
 			commitStatuses:   []*git_model.CommitStatus{},
 			requiredContexts: []string{},
-			expected:         structs.CommitStatusSuccess,
+			expected:         structs.CommitStatusPending,
 		},
 		{
 			commitStatuses: []*git_model.CommitStatus{