From 8d799c236c854c8bb132b38c97385beaf53d9327 Mon Sep 17 00:00:00 2001 From: badhezi Date: Tue, 15 Apr 2025 19:56:19 +0300 Subject: [PATCH 01/15] use the correct context data for PR link template in issue card --- templates/repo/issue/card.tmpl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/templates/repo/issue/card.tmpl b/templates/repo/issue/card.tmpl index c7bbe91885012..41fe6cea8fbae 100644 --- a/templates/repo/issue/card.tmpl +++ b/templates/repo/issue/card.tmpl @@ -45,7 +45,7 @@ {{if $.Page.LinkedPRs}} {{range index $.Page.LinkedPRs .ID}}
- + {{svg "octicon-git-merge" 16 "tw-mr-1 tw-align-middle"}} {{.Title}} #{{.Index}} From 8adf0280ddfda1be999ed860200b9d0817146bb4 Mon Sep 17 00:00:00 2001 From: badhezi Date: Tue, 20 May 2025 19:49:00 +0300 Subject: [PATCH 02/15] add skipped commit status and icon indicator --- modules/structs/commit_status.go | 2 ++ services/actions/commit_status.go | 4 +++- templates/repo/commit_status.tmpl | 3 +++ web_src/js/components/DashboardRepoList.vue | 1 + 4 files changed, 9 insertions(+), 1 deletion(-) diff --git a/modules/structs/commit_status.go b/modules/structs/commit_status.go index dc880ef5eb98d..e57eb03adcd1a 100644 --- a/modules/structs/commit_status.go +++ b/modules/structs/commit_status.go @@ -18,6 +18,8 @@ const ( CommitStatusFailure CommitStatusState = "failure" // CommitStatusWarning is for when the CommitStatus is Warning CommitStatusWarning CommitStatusState = "warning" + // CommitStatusSkipped is for when CommitStatus is Skipped + CommitStatusSkipped CommitStatusState = "skipped" ) var commitStatusPriorities = map[CommitStatusState]int{ diff --git a/services/actions/commit_status.go b/services/actions/commit_status.go index eb15d16061ea2..5d7515df3a52f 100644 --- a/services/actions/commit_status.go +++ b/services/actions/commit_status.go @@ -149,12 +149,14 @@ func createCommitStatus(ctx context.Context, job *actions_model.ActionRunJob) er func toCommitStatus(status actions_model.Status) api.CommitStatusState { switch status { - case actions_model.StatusSuccess, actions_model.StatusSkipped: + case actions_model.StatusSuccess: return api.CommitStatusSuccess case actions_model.StatusFailure, actions_model.StatusCancelled: return api.CommitStatusFailure case actions_model.StatusWaiting, actions_model.StatusBlocked, actions_model.StatusRunning: return api.CommitStatusPending + case actions_model.StatusSkipped: + return api.CommitStatusSkipped default: return api.CommitStatusError } diff --git a/templates/repo/commit_status.tmpl b/templates/repo/commit_status.tmpl index eb700ab2bb63e..7184f5f8eb6de 100644 --- a/templates/repo/commit_status.tmpl +++ b/templates/repo/commit_status.tmpl @@ -14,3 +14,6 @@ {{if eq .State "warning"}} {{svg "gitea-exclamation" 18 "commit-status icon text yellow"}} {{end}} +{{if eq .State "skipped"}} + {{svg "octicon-skip" 18 "commit-status icon text grey"}} +{{end}} diff --git a/web_src/js/components/DashboardRepoList.vue b/web_src/js/components/DashboardRepoList.vue index fc6a7bd281f11..2457b86b2a7cd 100644 --- a/web_src/js/components/DashboardRepoList.vue +++ b/web_src/js/components/DashboardRepoList.vue @@ -22,6 +22,7 @@ const commitStatus: CommitStatusMap = { error: {name: 'gitea-exclamation', color: 'red'}, failure: {name: 'octicon-x', color: 'red'}, warning: {name: 'gitea-exclamation', color: 'yellow'}, + skipped: {name: 'octicon-skip', color: 'grey'}, }; export default defineComponent({ From d352cfb83086aa2a5af3f2d02123c2ab30195e52 Mon Sep 17 00:00:00 2001 From: badhezi Date: Tue, 20 May 2025 20:16:30 +0300 Subject: [PATCH 03/15] update js CommitStatus type --- web_src/js/components/DashboardRepoList.vue | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web_src/js/components/DashboardRepoList.vue b/web_src/js/components/DashboardRepoList.vue index 2457b86b2a7cd..0528ceaf9044a 100644 --- a/web_src/js/components/DashboardRepoList.vue +++ b/web_src/js/components/DashboardRepoList.vue @@ -6,7 +6,7 @@ import {fomanticQuery} from '../modules/fomantic/base.ts'; const {appSubUrl, assetUrlPrefix, pageData} = window.config; -type CommitStatus = 'pending' | 'success' | 'error' | 'failure' | 'warning'; +type CommitStatus = 'pending' | 'success' | 'error' | 'failure' | 'warning' | 'skipped'; type CommitStatusMap = { [status in CommitStatus]: { From 4479858f4283a8b44cc6f1a9120338a5ccd91587 Mon Sep 17 00:00:00 2001 From: badhezi Date: Fri, 23 May 2025 20:17:15 +0300 Subject: [PATCH 04/15] add commitStatusPriorities to skipped status --- modules/structs/commit_status.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/modules/structs/commit_status.go b/modules/structs/commit_status.go index e57eb03adcd1a..fee0da214c38c 100644 --- a/modules/structs/commit_status.go +++ b/modules/structs/commit_status.go @@ -28,6 +28,7 @@ var commitStatusPriorities = map[CommitStatusState]int{ CommitStatusWarning: 2, CommitStatusPending: 3, CommitStatusSuccess: 4, + CommitStatusSkipped: 5, } func (css CommitStatusState) String() string { @@ -37,7 +38,7 @@ func (css CommitStatusState) String() string { // NoBetterThan returns true if this State is no better than the given State // This function only handles the states defined in CommitStatusPriorities func (css CommitStatusState) NoBetterThan(css2 CommitStatusState) bool { - // NoBetterThan only handles the 5 states above + // NoBetterThan only handles the 6 states above if _, exist := commitStatusPriorities[css]; !exist { return false } From a2d05212b75ed8e8740034f95539f9f67565a980 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 27 May 2025 20:02:50 +0800 Subject: [PATCH 05/15] Update modules/structs/commit_status.go --- modules/structs/commit_status.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/structs/commit_status.go b/modules/structs/commit_status.go index fee0da214c38c..8880f5a085f83 100644 --- a/modules/structs/commit_status.go +++ b/modules/structs/commit_status.go @@ -38,7 +38,7 @@ func (css CommitStatusState) String() string { // NoBetterThan returns true if this State is no better than the given State // This function only handles the states defined in CommitStatusPriorities func (css CommitStatusState) NoBetterThan(css2 CommitStatusState) bool { - // NoBetterThan only handles the 6 states above + // only handle the states with defined priorities above, it always returns false for undefined priorities if _, exist := commitStatusPriorities[css]; !exist { return false } From 8d78184596889b827d5ff11273e451f53c9637f4 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 28 May 2025 03:41:18 +0800 Subject: [PATCH 06/15] clean up --- models/git/commit_status.go | 2 +- modules/structs/commit_status.go | 17 +-- modules/structs/commit_status_test.go | 168 ++------------------------ services/convert/status.go | 3 +- services/pull/commit_status.go | 4 +- 5 files changed, 21 insertions(+), 173 deletions(-) diff --git a/models/git/commit_status.go b/models/git/commit_status.go index 0e4e8215b0e0e..fab64c1fb4f3a 100644 --- a/models/git/commit_status.go +++ b/models/git/commit_status.go @@ -233,7 +233,7 @@ func CalcCommitStatus(statuses []*CommitStatus) *CommitStatus { var lastStatus *CommitStatus state := api.CommitStatusSuccess for _, status := range statuses { - if status.State.NoBetterThan(state) { + if status.State.HasHigherPriorityThan(state) { state = status.State lastStatus = status } diff --git a/modules/structs/commit_status.go b/modules/structs/commit_status.go index 8880f5a085f83..398001974d4c4 100644 --- a/modules/structs/commit_status.go +++ b/modules/structs/commit_status.go @@ -35,19 +35,10 @@ func (css CommitStatusState) String() string { return string(css) } -// NoBetterThan returns true if this State is no better than the given State -// This function only handles the states defined in CommitStatusPriorities -func (css CommitStatusState) NoBetterThan(css2 CommitStatusState) bool { - // only handle the states with defined priorities above, it always returns false for undefined priorities - if _, exist := commitStatusPriorities[css]; !exist { - return false - } - - if _, exist := commitStatusPriorities[css2]; !exist { - return false - } - - return commitStatusPriorities[css] <= commitStatusPriorities[css2] +// HasHigherPriorityThan returns true if this state has higher priority than the other +// Undefined states are considered to have the highest priority like CommitStatusError(0) +func (css CommitStatusState) HasHigherPriorityThan(other CommitStatusState) bool { + return commitStatusPriorities[css] < commitStatusPriorities[other] } // IsPending represents if commit status state is pending diff --git a/modules/structs/commit_status_test.go b/modules/structs/commit_status_test.go index 88e09aadc1596..cc383824298ea 100644 --- a/modules/structs/commit_status_test.go +++ b/modules/structs/commit_status_test.go @@ -10,165 +10,21 @@ import ( ) func TestNoBetterThan(t *testing.T) { - type args struct { - css CommitStatusState - css2 CommitStatusState - } - var unExpectedState CommitStatusState tests := []struct { - name string - args args - want bool + s1, s2 CommitStatusState + higher bool }{ - { - name: "success is no better than success", - args: args{ - css: CommitStatusSuccess, - css2: CommitStatusSuccess, - }, - want: true, - }, - { - name: "success is no better than pending", - args: args{ - css: CommitStatusSuccess, - css2: CommitStatusPending, - }, - want: false, - }, - { - name: "success is no better than failure", - args: args{ - css: CommitStatusSuccess, - css2: CommitStatusFailure, - }, - want: false, - }, - { - name: "success is no better than error", - args: args{ - css: CommitStatusSuccess, - css2: CommitStatusError, - }, - want: false, - }, - { - name: "pending is no better than success", - args: args{ - css: CommitStatusPending, - css2: CommitStatusSuccess, - }, - want: true, - }, - { - name: "pending is no better than pending", - args: args{ - css: CommitStatusPending, - css2: CommitStatusPending, - }, - want: true, - }, - { - name: "pending is no better than failure", - args: args{ - css: CommitStatusPending, - css2: CommitStatusFailure, - }, - want: false, - }, - { - name: "pending is no better than error", - args: args{ - css: CommitStatusPending, - css2: CommitStatusError, - }, - want: false, - }, - { - name: "failure is no better than success", - args: args{ - css: CommitStatusFailure, - css2: CommitStatusSuccess, - }, - want: true, - }, - { - name: "failure is no better than pending", - args: args{ - css: CommitStatusFailure, - css2: CommitStatusPending, - }, - want: true, - }, - { - name: "failure is no better than failure", - args: args{ - css: CommitStatusFailure, - css2: CommitStatusFailure, - }, - want: true, - }, - { - name: "failure is no better than error", - args: args{ - css: CommitStatusFailure, - css2: CommitStatusError, - }, - want: false, - }, - { - name: "error is no better than success", - args: args{ - css: CommitStatusError, - css2: CommitStatusSuccess, - }, - want: true, - }, - { - name: "error is no better than pending", - args: args{ - css: CommitStatusError, - css2: CommitStatusPending, - }, - want: true, - }, - { - name: "error is no better than failure", - args: args{ - css: CommitStatusError, - css2: CommitStatusFailure, - }, - want: true, - }, - { - name: "error is no better than error", - args: args{ - css: CommitStatusError, - css2: CommitStatusError, - }, - want: true, - }, - { - name: "unExpectedState is no better than success", - args: args{ - css: unExpectedState, - css2: CommitStatusSuccess, - }, - want: false, - }, - { - name: "unExpectedState is no better than unExpectedState", - args: args{ - css: unExpectedState, - css2: unExpectedState, - }, - want: false, - }, + {CommitStatusError, CommitStatusFailure, true}, + {CommitStatusFailure, CommitStatusWarning, true}, + {CommitStatusWarning, CommitStatusPending, true}, + {CommitStatusPending, CommitStatusSuccess, true}, + {CommitStatusSuccess, CommitStatusSkipped, true}, + + {CommitStatusError, "unknown-xxx", false}, + {"unknown-xxx", CommitStatusFailure, true}, } for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := tt.args.css.NoBetterThan(tt.args.css2) - assert.Equal(t, tt.want, result) - }) + assert.Equal(t, tt.higher, tt.s1.HasHigherPriorityThan(tt.s2), "s1=%s, s2=%s, expected=%v", tt.s1, tt.s2, tt.higher) } + assert.Equal(t, false, CommitStatusError.HasHigherPriorityThan(CommitStatusError)) } diff --git a/services/convert/status.go b/services/convert/status.go index 6cef63c1cdadf..cfad684a02c6c 100644 --- a/services/convert/status.go +++ b/services/convert/status.go @@ -43,12 +43,13 @@ func ToCombinedStatus(ctx context.Context, statuses []*git_model.CommitStatus, r TotalCount: len(statuses), Repository: repo, URL: "", + State: api.CommitStatusSuccess, } retStatus.Statuses = make([]*api.CommitStatus, 0, len(statuses)) for _, status := range statuses { retStatus.Statuses = append(retStatus.Statuses, ToCommitStatus(ctx, status)) - if retStatus.State == "" || status.State.NoBetterThan(retStatus.State) { + if status.State.HasHigherPriorityThan(retStatus.State) { retStatus.State = status.State } } diff --git a/services/pull/commit_status.go b/services/pull/commit_status.go index 553496713fe2b..a407d8711308c 100644 --- a/services/pull/commit_status.go +++ b/services/pull/commit_status.go @@ -46,13 +46,13 @@ func MergeRequiredContextsCommitStatus(commitStatuses []*git_model.CommitStatus, // If required rule not match any action, then it is pending if targetStatus == "" { - if structs.CommitStatusPending.NoBetterThan(returnedStatus) { + if structs.CommitStatusPending.HasHigherPriorityThan(returnedStatus) { returnedStatus = structs.CommitStatusPending } break } - if targetStatus.NoBetterThan(returnedStatus) { + if targetStatus.HasHigherPriorityThan(returnedStatus) { returnedStatus = targetStatus } } From e6dc1862ec58e65d68fe550eb01e529a2afec121 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 28 May 2025 04:00:12 +0800 Subject: [PATCH 07/15] add comment for CalcCommitStatus --- models/git/commit_status.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/models/git/commit_status.go b/models/git/commit_status.go index fab64c1fb4f3a..efe922ca381ef 100644 --- a/models/git/commit_status.go +++ b/models/git/commit_status.go @@ -230,6 +230,10 @@ func (status *CommitStatus) HideActionsURL(ctx context.Context) { // CalcCommitStatus returns commit status state via some status, the commit statues should order by id desc func CalcCommitStatus(statuses []*CommitStatus) *CommitStatus { + // This function is widely used, but it is not quite right. + // The bad case is: if all commits are "skipped", GitHub will return "success" as the combined status. + // But here Gitea just returns the first status, which is still "skipped" in this case. + // Ideally it should return something like "CommitStatusSummary" with proper aggregated state. var lastStatus *CommitStatus state := api.CommitStatusSuccess for _, status := range statuses { From ebeed316718cb126591bb79caa0c93e4db6e6f98 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 28 May 2025 04:11:17 +0800 Subject: [PATCH 08/15] fix status check in MergeRequiredContextsCommitStatus --- models/git/commit_status.go | 2 +- services/pull/commit_status.go | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/models/git/commit_status.go b/models/git/commit_status.go index efe922ca381ef..e8c080b1305cd 100644 --- a/models/git/commit_status.go +++ b/models/git/commit_status.go @@ -237,7 +237,7 @@ func CalcCommitStatus(statuses []*CommitStatus) *CommitStatus { var lastStatus *CommitStatus state := api.CommitStatusSuccess for _, status := range statuses { - if status.State.HasHigherPriorityThan(state) { + if state == status.State || status.State.HasHigherPriorityThan(state) { state = status.State lastStatus = status } diff --git a/services/pull/commit_status.go b/services/pull/commit_status.go index a407d8711308c..f18274cde5bde 100644 --- a/services/pull/commit_status.go +++ b/services/pull/commit_status.go @@ -61,6 +61,11 @@ 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 } return structs.CommitStatusSuccess From 49eef32f0442595977c4acb96d0ae3e94858a9bf Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 28 May 2025 04:22:02 +0800 Subject: [PATCH 09/15] improve tests --- services/pull/commit_status_test.go | 90 +++++++++++++++++------------ 1 file changed, 54 insertions(+), 36 deletions(-) diff --git a/services/pull/commit_status_test.go b/services/pull/commit_status_test.go index 592acdd55cd43..71abd08011fed 100644 --- a/services/pull/commit_status_test.go +++ b/services/pull/commit_status_test.go @@ -14,52 +14,70 @@ import ( ) func TestMergeRequiredContextsCommitStatus(t *testing.T) { - testCases := [][]*git_model.CommitStatus{ + cases := []struct { + commitStatuses []*git_model.CommitStatus + requiredContexts []string + expected structs.CommitStatusState + }{ { - {Context: "Build 1", State: structs.CommitStatusSuccess}, - {Context: "Build 2", State: structs.CommitStatusSuccess}, - {Context: "Build 3", State: structs.CommitStatusSuccess}, + commitStatuses: []*git_model.CommitStatus{}, + requiredContexts: []string{}, + expected: structs.CommitStatusSuccess, }, { - {Context: "Build 1", State: structs.CommitStatusSuccess}, - {Context: "Build 2", State: structs.CommitStatusSuccess}, - {Context: "Build 2t", State: structs.CommitStatusPending}, + commitStatuses: []*git_model.CommitStatus{ + {Context: "Build xxx", State: structs.CommitStatusSkipped}, + }, + requiredContexts: []string{"Build*"}, + expected: structs.CommitStatusSuccess, }, { - {Context: "Build 1", State: structs.CommitStatusSuccess}, - {Context: "Build 2", State: structs.CommitStatusSuccess}, - {Context: "Build 2t", State: structs.CommitStatusFailure}, + commitStatuses: []*git_model.CommitStatus{ + {Context: "Build 1", State: structs.CommitStatusSuccess}, + {Context: "Build 2", State: structs.CommitStatusSuccess}, + {Context: "Build 3", State: structs.CommitStatusSuccess}, + }, + requiredContexts: []string{"Build*"}, + expected: structs.CommitStatusSuccess, }, { - {Context: "Build 1", State: structs.CommitStatusSuccess}, - {Context: "Build 2", State: structs.CommitStatusSuccess}, - {Context: "Build 2t", State: structs.CommitStatusSuccess}, + commitStatuses: []*git_model.CommitStatus{ + {Context: "Build 1", State: structs.CommitStatusSuccess}, + {Context: "Build 2", State: structs.CommitStatusSuccess}, + {Context: "Build 2t", State: structs.CommitStatusPending}, + }, + requiredContexts: []string{"Build*", "Build 2t*"}, + expected: structs.CommitStatusPending, }, { - {Context: "Build 1", State: structs.CommitStatusSuccess}, - {Context: "Build 2", State: structs.CommitStatusSuccess}, - {Context: "Build 2t", State: structs.CommitStatusSuccess}, + commitStatuses: []*git_model.CommitStatus{ + {Context: "Build 1", State: structs.CommitStatusSuccess}, + {Context: "Build 2", State: structs.CommitStatusSuccess}, + {Context: "Build 2t", State: structs.CommitStatusFailure}, + }, + requiredContexts: []string{"Build*", "Build 2t*"}, + expected: structs.CommitStatusFailure, + }, + { + commitStatuses: []*git_model.CommitStatus{ + {Context: "Build 1", State: structs.CommitStatusSuccess}, + {Context: "Build 2", State: structs.CommitStatusSuccess}, + {Context: "Build 2t", State: structs.CommitStatusSuccess}, + }, + requiredContexts: []string{"Build*", "Build 2t*", "Build 3*"}, + expected: structs.CommitStatusPending, + }, + { + commitStatuses: []*git_model.CommitStatus{ + {Context: "Build 1", State: structs.CommitStatusSuccess}, + {Context: "Build 2", State: structs.CommitStatusSuccess}, + {Context: "Build 2t", State: structs.CommitStatusSuccess}, + }, + requiredContexts: []string{"Build*", "Build *", "Build 2t*", "Build 1*"}, + expected: structs.CommitStatusSuccess, }, } - testCasesRequiredContexts := [][]string{ - {"Build*"}, - {"Build*", "Build 2t*"}, - {"Build*", "Build 2t*"}, - {"Build*", "Build 2t*", "Build 3*"}, - {"Build*", "Build *", "Build 2t*", "Build 1*"}, - } - - testCasesExpected := []structs.CommitStatusState{ - structs.CommitStatusSuccess, - structs.CommitStatusPending, - structs.CommitStatusFailure, - structs.CommitStatusPending, - structs.CommitStatusSuccess, - } - - for i, commitStatuses := range testCases { - if MergeRequiredContextsCommitStatus(commitStatuses, testCasesRequiredContexts[i]) != testCasesExpected[i] { - assert.Fail(t, "Test case failed", "Test case %d failed", i+1) - } + for i, c := range cases { + assert.Equal(t, c.expected, MergeRequiredContextsCommitStatus(c.commitStatuses, c.requiredContexts), "case %d", i) } } From 708b158aaf71c62d6670c3928bf31b3f5235c109 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 28 May 2025 04:24:20 +0800 Subject: [PATCH 10/15] improve tests --- services/pull/commit_status_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/pull/commit_status_test.go b/services/pull/commit_status_test.go index 71abd08011fed..11804e143a247 100644 --- a/services/pull/commit_status_test.go +++ b/services/pull/commit_status_test.go @@ -33,7 +33,7 @@ func TestMergeRequiredContextsCommitStatus(t *testing.T) { }, { commitStatuses: []*git_model.CommitStatus{ - {Context: "Build 1", State: structs.CommitStatusSuccess}, + {Context: "Build 1", State: structs.CommitStatusSkipped}, {Context: "Build 2", State: structs.CommitStatusSuccess}, {Context: "Build 3", State: structs.CommitStatusSuccess}, }, From 8c21f017daa2c813faf0fa4a07948f022913a614 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 28 May 2025 04:30:14 +0800 Subject: [PATCH 11/15] improve comments --- models/git/commit_status.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/models/git/commit_status.go b/models/git/commit_status.go index e8c080b1305cd..6a74e3a3c61e7 100644 --- a/models/git/commit_status.go +++ b/models/git/commit_status.go @@ -231,8 +231,8 @@ func (status *CommitStatus) HideActionsURL(ctx context.Context) { // CalcCommitStatus returns commit status state via some status, the commit statues should order by id desc func CalcCommitStatus(statuses []*CommitStatus) *CommitStatus { // This function is widely used, but it is not quite right. - // The bad case is: if all commits are "skipped", GitHub will return "success" as the combined status. - // But here Gitea just returns the first status, which is still "skipped" in this case. + // If all commits are "skipped", GitHub will return "success" as the combined status. + // FIXME: But the bad case here is: Gitea just returns the first status, which is still "skipped". // Ideally it should return something like "CommitStatusSummary" with proper aggregated state. var lastStatus *CommitStatus state := api.CommitStatusSuccess @@ -244,8 +244,10 @@ func CalcCommitStatus(statuses []*CommitStatus) *CommitStatus { } if lastStatus == nil { if len(statuses) > 0 { + // the bad case mentioned above: only the first status is returned, its status is "skipped" lastStatus = statuses[0] } else { + // FIXME: this is another bad case, if the "statuses" slice is empty, the returned value is an invalid CommitStatus, all its fields are empty. lastStatus = &CommitStatus{} } } From ca6bcf0361dfe842e1594217dba929c28d610133 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 28 May 2025 04:34:14 +0800 Subject: [PATCH 12/15] improve comments --- models/git/commit_status.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/models/git/commit_status.go b/models/git/commit_status.go index 6a74e3a3c61e7..b631943a9caa0 100644 --- a/models/git/commit_status.go +++ b/models/git/commit_status.go @@ -231,9 +231,8 @@ func (status *CommitStatus) HideActionsURL(ctx context.Context) { // CalcCommitStatus returns commit status state via some status, the commit statues should order by id desc func CalcCommitStatus(statuses []*CommitStatus) *CommitStatus { // This function is widely used, but it is not quite right. - // If all commits are "skipped", GitHub will return "success" as the combined status. - // FIXME: But the bad case here is: Gitea just returns the first status, which is still "skipped". - // Ideally it should return something like "CommitStatusSummary" with proper aggregated state. + // Ideally it should return something like "CommitStatusSummary" with properly aggregated state. + // GitHub's behavior: if all statuses are "skipped", GitHub will return "success" as the combined status. var lastStatus *CommitStatus state := api.CommitStatusSuccess for _, status := range statuses { @@ -244,10 +243,10 @@ func CalcCommitStatus(statuses []*CommitStatus) *CommitStatus { } if lastStatus == nil { if len(statuses) > 0 { - // the bad case mentioned above: only the first status is returned, its status is "skipped" + // FIXME: a bad case: Gitea just returns the first commit status, its status is "skipped" in this case. lastStatus = statuses[0] } else { - // FIXME: this is another bad case, if the "statuses" slice is empty, the returned value is an invalid CommitStatus, all its fields are empty. + // FIXME: another bad case: if the "statuses" slice is empty, the returned value is an invalid CommitStatus, all its fields are empty. lastStatus = &CommitStatus{} } } From e843ac5711007577cc4bdc888ea1097a015bf1b2 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 28 May 2025 04:49:03 +0800 Subject: [PATCH 13/15] fix lint --- modules/structs/commit_status_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/structs/commit_status_test.go b/modules/structs/commit_status_test.go index cc383824298ea..c11daf248d1c3 100644 --- a/modules/structs/commit_status_test.go +++ b/modules/structs/commit_status_test.go @@ -26,5 +26,5 @@ func TestNoBetterThan(t *testing.T) { for _, tt := range tests { assert.Equal(t, tt.higher, tt.s1.HasHigherPriorityThan(tt.s2), "s1=%s, s2=%s, expected=%v", tt.s1, tt.s2, tt.higher) } - assert.Equal(t, false, CommitStatusError.HasHigherPriorityThan(CommitStatusError)) + assert.False(t, CommitStatusError.HasHigherPriorityThan(CommitStatusError)) } From 31e81156c84f8c8e0194d3ac544856f661e7b60f Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 28 May 2025 05:35:43 +0800 Subject: [PATCH 14/15] 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 | 15 +++++++-------- services/pull/commit_status_test.go | 2 +- 5 files changed, 22 insertions(+), 14 deletions(-) diff --git a/models/git/commit_status.go b/models/git/commit_status.go index b631943a9caa0..2e765391b899c 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 a3f440fd86d9e..774e49bb9859b 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 cfad684a02c6c..4fffbfbe5ee6f 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 f18274cde5bde..80d798734faf8 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 { + 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 diff --git a/services/pull/commit_status_test.go b/services/pull/commit_status_test.go index 11804e143a247..9cb20d6c5d892 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{ From ae60b24d9845e3a7fa5eed61cbf159891fd26930 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 28 May 2025 05:51:28 +0800 Subject: [PATCH 15/15] remove dead code --- services/pull/commit_status.go | 30 ------------------------------ 1 file changed, 30 deletions(-) diff --git a/services/pull/commit_status.go b/services/pull/commit_status.go index 80d798734faf8..58d26c5a00079 100644 --- a/services/pull/commit_status.go +++ b/services/pull/commit_status.go @@ -73,36 +73,6 @@ func MergeRequiredContextsCommitStatus(commitStatuses []*git_model.CommitStatus, return returnedStatus } -// IsCommitStatusContextSuccess returns true if all required status check contexts succeed. -func IsCommitStatusContextSuccess(commitStatuses []*git_model.CommitStatus, requiredContexts []string) bool { - // If no specific context is required, require that last commit status is a success - if len(requiredContexts) == 0 { - status := git_model.CalcCommitStatus(commitStatuses) - if status == nil || status.State != structs.CommitStatusSuccess { - return false - } - return true - } - - for _, ctx := range requiredContexts { - var found bool - for _, commitStatus := range commitStatuses { - if commitStatus.Context == ctx { - if commitStatus.State != structs.CommitStatusSuccess { - return false - } - - found = true - break - } - } - if !found { - return false - } - } - return true -} - // IsPullCommitStatusPass returns if all required status checks PASS func IsPullCommitStatusPass(ctx context.Context, pr *issues_model.PullRequest) (bool, error) { pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch)