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 <earl-warren@noreply.codeberg.org>
Co-authored-by: Gusted <postmaster@gusted.xyz>
Co-committed-by: Gusted <postmaster@gusted.xyz>

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

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,
+				})
+			}
 		}
 	}
 

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)
+		}
+	})
+}

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

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

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