oss/forgejo
oss/forgejo
1
0
Fork 0
mirror of https://codeberg.org/forgejo/forgejo.git synced 2025-09-30 19:22:08 +00:00
Code Wiki Activity

fix: use correct commit when fetching patch view for review comments (#9264)

Browse source 
This is effectively a clean revert of e7a77d32cc, which for some reason broke this logic.

Yes, always having the latest code shown in the diff is nice, but doing this mismatches the commitID and the line number the comment is being posted on, which very easily and frequently leads to reviews that show a completely nonsensical patch above them, cause the code it's referring too shifted by several lines since the time the review was started and the time it was posted.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9264
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Co-authored-by: BtbN <btbn@btbn.de>
Co-committed-by: BtbN <btbn@btbn.de>
This commit is contained in:
BtbN 2025-09-20 22:47:52 +02:00 • committed by Gusted
parent aa345c9e0c
commit c08bdaacdb
2 changed files with 60 additions and 7 deletions
Download patch file Download diff file Expand all files Collapse all files

13
services/pull/review.go
View file

@ -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()

54
tests/integration/pull_review_test.go
View file

@ -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")