diff --git a/services/pull/review.go b/services/pull/review.go index b0ab700fa6..8efe7d6bcb 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -237,12 +237,11 @@ func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User, // Only fetch diff if comment is review comment if len(patch) == 0 && reviewID != 0 { - headCommitID, err := gitRepo.GetRefCommitID(pr.GetGitRefName()) - if err != nil { - return nil, fmt.Errorf("GetRefCommitID[%s]: %w", pr.GetGitRefName(), err) - } if len(commitID) == 0 { - commitID = headCommitID + commitID, err = gitRepo.GetRefCommitID(head) + if err != nil { + return nil, fmt.Errorf("GetRefCommitID[%s]: %w", head, err) + } } reader, writer := io.Pipe() defer func() { @@ -250,8 +249,8 @@ func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User, _ = writer.Close() }() go func() { - if err := git.GetRepoRawDiffForFile(gitRepo, pr.MergeBase, headCommitID, git.RawDiffNormal, treePath, writer); err != nil { - _ = writer.CloseWithError(fmt.Errorf("GetRawDiffForLine[%s, %s, %s, %s]: %w", gitRepo.Path, pr.MergeBase, headCommitID, treePath, err)) + if err := git.GetRepoRawDiffForFile(gitRepo, pr.MergeBase, commitID, git.RawDiffNormal, treePath, writer); err != nil { + _ = writer.CloseWithError(fmt.Errorf("GetRawDiffForLine[%s, %s, %s, %s]: %w", gitRepo.Path, pr.MergeBase, commitID, treePath, err)) return } _ = writer.Close() diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index b385916f96..131e8537c7 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -527,6 +527,60 @@ func TestPullView_GivenApproveOrRejectReviewOnClosedPR(t *testing.T) { }) } +func TestPullReview_OldLatestCommitId(t *testing.T) { + defer tests.PrepareTestEnv(t)() + session := loginUser(t, "user1") + + baseRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user2", Name: "repo1"}) + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: baseRepo.ID, Index: 3}) + + baseGitRepo, err := gitrepo.OpenRepository(db.DefaultContext, baseRepo) + require.NoError(t, err) + defer baseGitRepo.Close() + + headCommitSHA, err := baseGitRepo.GetRefCommitID(pr.GetGitRefName()) + require.NoError(t, err) + + headCommit, err := baseGitRepo.GetCommit(headCommitSHA) + require.NoError(t, err) + require.GreaterOrEqual(t, headCommit.ParentCount(), 1) + + parentCommit, err := headCommit.Parent(0) + require.NoError(t, err) + oldCommitSHA := parentCommit.ID.String() + require.NotEqual(t, headCommitSHA, oldCommitSHA) + + req := NewRequest(t, "GET", "/user2/repo1/pulls/3/files/reviews/new_comment") + resp := session.MakeRequest(t, req, http.StatusOK) + doc := NewHTMLParser(t, resp.Body) + + const content = "TestPullReview_OldLatestCommitId" + req = NewRequestWithValues(t, "POST", "/user2/repo1/pulls/3/files/reviews/comments", map[string]string{ + "_csrf": doc.GetInputValueByName("_csrf"), + "origin": doc.GetInputValueByName("origin"), + "latest_commit_id": oldCommitSHA, + "side": "proposed", + "line": "2", + "path": "iso-8859-1.txt", + "diff_start_cid": doc.GetInputValueByName("diff_start_cid"), + "diff_end_cid": doc.GetInputValueByName("diff_end_cid"), + "diff_base_cid": doc.GetInputValueByName("diff_base_cid"), + "content": content, + "single_review": "true", + }) + session.MakeRequest(t, req, http.StatusOK) + + comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{IssueID: pr.IssueID, Content: content}) + require.NotZero(t, comment.ReviewID) + assert.Equal(t, oldCommitSHA, comment.CommitSHA) + assert.NotEqual(t, headCommitSHA, comment.CommitSHA) + + review := unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: comment.ReviewID}) + assert.Equal(t, issues_model.ReviewTypeComment, review.Type) + assert.Equal(t, oldCommitSHA, review.CommitID) + assert.NotEqual(t, headCommitSHA, review.CommitID) +} + func TestPullReviewInArchivedRepo(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { session := loginUser(t, "user2")