fine tune

Author: wxiaoguang

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{}
 		}
 	}

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() {

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
 }

services/pull/commit_status.go

@@ -59,16 +59,15 @@ func MergeRequiredContextsCommitStatus(commitStatuses []*git_model.CommitStatus,
 	}
 
 	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 != 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 status.State == structs.CommitStatusSkipped {
+			return structs.CommitStatusSuccess // if all statuses are skipped, return success
 		}
-		return structs.CommitStatusSuccess
+		return status.State
 	}
 
 	return returnedStatus

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{