From b816bf923232d5504c63f2efbec2b174ad990661 Mon Sep 17 00:00:00 2001 From: Gusted Date: Fri, 12 Sep 2025 07:27:15 +0200 Subject: [PATCH] fix: ignore existence of commits for force pushes (#9262) - Because we wish to show the status of the old and new commit of a force push, ignore that the commit doesn't exist and return a commit with only its ID filled. This is enough to still show the CI status of this commit although the commit itself is no longer reachable. - Add unit test. - Add integration test. - Resolves forgejo/forgejo#9250 Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9262 Reviewed-by: Earl Warren Co-authored-by: Gusted Co-committed-by: Gusted --- models/issues/comment.go | 2 +- modules/git/repo_commit.go | 18 ++++++++- modules/git/repo_commit_test.go | 37 +++++++++++++++++++ .../integration/actions_commit_status_test.go | 10 ++++- .../TestForcePushCommitStatus/comment.yml | 7 ++++ .../commit_status.yml | 12 ++++++ 6 files changed, 81 insertions(+), 5 deletions(-) create mode 100644 tests/integration/fixtures/TestForcePushCommitStatus/comment.yml diff --git a/models/issues/comment.go b/models/issues/comment.go index a84bbffd08..6afd1623f3 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -802,7 +802,7 @@ func (c *Comment) LoadPushCommits(ctx context.Context) (err error) { return err } defer closer.Close() - c.Commits = git_model.ParseCommitsWithStatus(ctx, gitRepo.GetCommitsFromIDs(data.CommitIDs), c.Issue.Repo) + c.Commits = git_model.ParseCommitsWithStatus(ctx, gitRepo.GetCommitsFromIDs(data.CommitIDs, c.IsForcePush), c.Issue.Repo) c.CommitsNum = int64(len(c.Commits)) return err diff --git a/modules/git/repo_commit.go b/modules/git/repo_commit.go index 41ca0e39b1..a650e597d2 100644 --- a/modules/git/repo_commit.go +++ b/modules/git/repo_commit.go @@ -458,14 +458,28 @@ func (repo *Repository) getBranches(commit *Commit, limit int) ([]string, error) return branches, nil } -// GetCommitsFromIDs get commits from commit IDs -func (repo *Repository) GetCommitsFromIDs(commitIDs []string) []*Commit { +// GetCommitsFromIDs get commits from commit IDs. If ignoreExistence is +// specified, then commits that no longer exists are still returned but +// without any information except the ID. +func (repo *Repository) GetCommitsFromIDs(commitIDs []string, ignoreExistence bool) []*Commit { commits := make([]*Commit, 0, len(commitIDs)) for _, commitID := range commitIDs { commit, err := repo.GetCommit(commitID) if err == nil && commit != nil { commits = append(commits, commit) + } else if ignoreExistence && IsErrNotExist(err) { + // It's entirely possible the commit no longer exists, we only care + // about the status and verification. Verification is no longer possible, + // but getting the status is still possible with just the ID. We do have + // to assumme the commitID is not shortened, we cannot recover the full + // commitID. + id, err := NewIDFromString(commitID) + if err == nil { + commits = append(commits, &Commit{ + ID: id, + }) + } } } diff --git a/modules/git/repo_commit_test.go b/modules/git/repo_commit_test.go index 53760b208e..5932f31e3c 100644 --- a/modules/git/repo_commit_test.go +++ b/modules/git/repo_commit_test.go @@ -199,3 +199,40 @@ func TestCommitsByFileAndRange(t *testing.T) { assert.Len(t, commits, testCase.ExpectedCommitCount, "file: '%s', page: %d", testCase.File, testCase.Page) } } + +func TestGetCommitsFromIDs(t *testing.T) { + bareRepo1, err := openRepositoryWithDefaultContext(filepath.Join(testReposDir, "repo1_bare")) + require.NoError(t, err) + + commitIDs := []string{"2839944139e0de9737a044f78b0e4b40d989a9e3", "2839944139e0de9737a044f78b0e4b40d989a9e4"} + + t.Run("Normal", func(t *testing.T) { + commits := bareRepo1.GetCommitsFromIDs(commitIDs, false) + if assert.Len(t, commits, 1) { + assert.Equal(t, "2839944139e0de9737a044f78b0e4b40d989a9e3", commits[0].ID.String()) + assert.Equal(t, "Example User", commits[0].Author.Name) + } + }) + + t.Run("Ignore existence", func(t *testing.T) { + commits := bareRepo1.GetCommitsFromIDs(commitIDs, true) + if assert.Len(t, commits, 2) { + assert.Equal(t, "2839944139e0de9737a044f78b0e4b40d989a9e3", commits[0].ID.String()) + assert.Equal(t, "Example User", commits[0].Author.Name) + + assert.Equal(t, "2839944139e0de9737a044f78b0e4b40d989a9e4", commits[1].ID.String()) + assert.Nil(t, commits[1].Author) + } + }) + + t.Run("Not full commit ID", func(t *testing.T) { + commits := bareRepo1.GetCommitsFromIDs(append(commitIDs, "abba"), true) + if assert.Len(t, commits, 2) { + assert.Equal(t, "2839944139e0de9737a044f78b0e4b40d989a9e3", commits[0].ID.String()) + assert.Equal(t, "Example User", commits[0].Author.Name) + + assert.Equal(t, "2839944139e0de9737a044f78b0e4b40d989a9e4", commits[1].ID.String()) + assert.Nil(t, commits[1].Author) + } + }) +} diff --git a/tests/integration/actions_commit_status_test.go b/tests/integration/actions_commit_status_test.go index 7c61904bc6..5667bdadd5 100644 --- a/tests/integration/actions_commit_status_test.go +++ b/tests/integration/actions_commit_status_test.go @@ -58,6 +58,12 @@ func TestActionsForcePushCommitStatus(t *testing.T) { req := NewRequest(t, "GET", "/user2/commitsonpr/pulls/1") resp := MakeRequest(t, req, http.StatusOK) htmlDoc := NewHTMLParser(t, resp.Body) - htmlDoc.AssertElement(t, ".forced-push [data-tippy='commit-statuses']:nth-of-type(3) svg.commit-status.octicon-dot-fill", true) - htmlDoc.AssertElement(t, ".forced-push [data-tippy='commit-statuses']:nth-of-type(5) svg.commit-status.octicon-check", true) + + htmlDoc.AssertElement(t, ".error-code", false) + + htmlDoc.AssertElement(t, "#issuecomment-17 .forced-push [data-tippy='commit-statuses']:nth-of-type(3) svg.commit-status.octicon-dot-fill", true) + htmlDoc.AssertElement(t, "#issuecomment-17 .forced-push [data-tippy='commit-statuses']:nth-of-type(5) svg.commit-status.octicon-check", true) + + htmlDoc.AssertElement(t, "#issuecomment-1001 .forced-push [data-tippy='commit-statuses']:nth-of-type(3) svg.commit-status.octicon-check", true) + htmlDoc.AssertElement(t, "#issuecomment-1001 .forced-push [data-tippy='commit-statuses']:nth-of-type(5) svg.commit-status.octicon-check", true) } diff --git a/tests/integration/fixtures/TestForcePushCommitStatus/comment.yml b/tests/integration/fixtures/TestForcePushCommitStatus/comment.yml new file mode 100644 index 0000000000..11f331bb53 --- /dev/null +++ b/tests/integration/fixtures/TestForcePushCommitStatus/comment.yml @@ -0,0 +1,7 @@ +- + id: 1001 + type: 29 # push + poster_id: 2 + issue_id: 19 # in repo_id 58 + content: '{"is_force_push":true,"commit_ids":["9b93963cf6de4dc33f915bb67f192d099c301f43", "ffffffffffffffffffffffffffffffffffffffff"]}' + created_unix: 1757622411 \ No newline at end of file diff --git a/tests/integration/fixtures/TestForcePushCommitStatus/commit_status.yml b/tests/integration/fixtures/TestForcePushCommitStatus/commit_status.yml index 9673410671..2764aa23bb 100644 --- a/tests/integration/fixtures/TestForcePushCommitStatus/commit_status.yml +++ b/tests/integration/fixtures/TestForcePushCommitStatus/commit_status.yml @@ -19,4 +19,16 @@ description: My awesome CI-service context: ci/awesomeness context_hash: c65f4d64a3b14a3eced0c9b36799e66e1bd5ced7 + creator_id: 2 + +- + id: 1002 + index: 1 + repo_id: 58 + state: "success" + sha: "ffffffffffffffffffffffffffffffffffffffff" + target_url: https://example.com/builds/ + description: My awesome CI-service + context: ci/awesomeness + context_hash: c65f4d64a3b14a3eced0c9b36799e66e1bd5ced7 creator_id: 2 \ No newline at end of file