Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix wrong commit status in web ui #26121

Merged
merged 6 commits into from
Jul 25, 2023

Conversation

yp05327
Copy link
Contributor

@yp05327 yp05327 commented Jul 25, 2023

Before:
image
After:
image

There's a bug in the recent logic, CalcCommitStatus will always return the first item of statuses or error status, because state is defined with default value which should be CommitStatusSuccess

Then

if status.State.NoBetterThan(state) {

this if will always return false unless status.State = CommitStatusError which makes no sense.
So lastStatus will always be nil or error status.
Then we will always return the first item of statuses here or only return error status, and this is why in the first picture the commit status is Success but not Failure.

if lastStatus == nil {
if len(statuses) > 0 {
lastStatus = statuses[0]
} else {
lastStatus = &CommitStatus{}
}
}
return lastStatus

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 25, 2023
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 25, 2023
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 25, 2023
@yp05327
Copy link
Contributor Author

yp05327 commented Jul 25, 2023

It seems that this logic problem comes from #25839
In old code, CalcCommitStatus will return true when the input is an empty string(default value).

default:
    return css2 != CommitStatusError && css2 != CommitStatusFailure && css2 != CommitStatusWarning && css2 != CommitStatusPending

@silverwind
Copy link
Member

silverwind commented Jul 25, 2023

Possible to add a test that verifies that "ok, fail" checks result in "fail"?

@yp05327
Copy link
Contributor Author

yp05327 commented Jul 25, 2023

Possible to add a test that verifies that "ok, fail" checks result in "fail"?

Maybe a unit test for CalcCommitStatus is enough?

Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be simplified to:

	var lastStatus *CommitStatus
	if lastStatus == nil || status.State.NoBetterThan(lastStatus.state) {
		lastStatus = status
	}

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jul 25, 2023
@CaiCandong
Copy link
Member

CaiCandong commented Jul 25, 2023

It seems that this logic problem comes from #25839

My fault, the status.State.NoBetterThan I wrote only handles CommitStatusError, CommitStatusFailure CommitStatusPending, CommitStatusSuccess.
A better way to work around this bug would be to make the NoBetterThan function more robust? but It's a bit strange that to set the empty string "" is best.
The function is used elsewhere, and there seems to be a logic error in this place as well:

retStatus := &api.CombinedStatus{
SHA: statuses[0].SHA,
TotalCount: len(statuses),
Repository: repo,
URL: "",
}
retStatus.Statuses = make([]*api.CommitStatus, 0, len(statuses))
for _, status := range statuses {
retStatus.Statuses = append(retStatus.Statuses, ToCommitStatus(ctx, status))
if status.State.NoBetterThan(retStatus.State) {
retStatus.State = status.State
}
}

This place looks right.
var targetStatus structs.CommitStatusState
for _, gp := range requiredContextsGlob {
if gp.Match(commitStatus.Context) {
targetStatus = commitStatus.State
matchedCount++
break
}
}
if targetStatus != "" && targetStatus.NoBetterThan(returnedStatus) {
returnedStatus = targetStatus
}

@lunny lunny added type/bug skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels Jul 25, 2023
@lunny lunny added this to the 1.21.0 milestone Jul 25, 2023
@lunny lunny enabled auto-merge (squash) July 25, 2023 09:17
@wolfogre wolfogre removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 25, 2023
@lunny lunny disabled auto-merge July 25, 2023 09:30
@wolfogre wolfogre added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 25, 2023
@wolfogre wolfogre enabled auto-merge (squash) July 25, 2023 09:41
@wolfogre
Copy link
Member

I believe this PR is good enough to merge, @CaiCandong please help to improve if you have any ideas.

@wolfogre wolfogre merged commit 7a687ca into go-gitea:main Jul 25, 2023
23 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 25, 2023
@yp05327 yp05327 deleted the fix-wrong-commit-status-in-web-ui branch July 25, 2023 23:45
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jul 26, 2023
* giteaofficial/main: (21 commits)
  Only show newly pushed branches message in the same repository (go-gitea#26148)
  Docusaurus-ify (go-gitea#26051)
  Display deprecated warning in admin panel pages as well as in the log file (go-gitea#26094)
  Remove "misc" scope check from public API endpoints (go-gitea#26134)
  Fix LFS object list style (go-gitea#26133)
  Drop the correct deleted branch table (go-gitea#26028)
  Fix CLI allowing creation of access tokens with existing name (go-gitea#26071)
  Fix incorrect router logger (go-gitea#26137)
  Increase table cell horizontal padding (go-gitea#26140)
  Update xorm version (go-gitea#26128)
  Fix UI for release tag page / wiki page / subscription page (go-gitea#25948)
  added ssh mirror workaround description (go-gitea#26096)
  Improve "gitea doctor" sub-command and fix "help" commands (go-gitea#26072)
  Fix wrong commit status in web ui (go-gitea#26121)
  remove IsWarning in  tmpl (go-gitea#26120)
  Fix minor capitalization error in string (go-gitea#26100)
  Improve commit graph alignment and truncating (go-gitea#26112)
  Fix wrong workflow status when rerun a job in an already finished workflow (go-gitea#26119)
  Allow Organisations to have a E-Mail (go-gitea#25082)
  doc sync authentication.md to zh-cn (go-gitea#26117)
  ...
lunny pushed a commit that referenced this pull request Jul 26, 2023
- The `NoBetterThan` function can only handle comparisons between
"pending," "success," "error," and "failure." For any other comparison,
we directly return false. This prevents logic errors like the one in
#26121.
- The callers of the `NoBetterThan` function should also avoid making
incomparable calls.

---------

Co-authored-by: yp05327 <576951401@qq.com>
Co-authored-by: puni9869 <80308335+puni9869@users.noreply.github.com>
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Oct 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants