diff --git a/models/issues/comment.go b/models/issues/comment.go index b42cd8aa40..e0ea1e111d 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -798,7 +798,7 @@ func (c *Comment) ResolveCurrentLine(ctx context.Context, repo *repo_model.Repos // instead, return a blame where CommitID != headCommitID, which will be an indicator to callers (for // both resolution methods) that the line of code is outdated in the diff. reverseBlame = &git.ReverseLineBlame{ - CommitID: c.CommitSHA, // not currentHead + CommitID: "", // not currentHead LineNumber: c.UnsignedLine(), FilePath: c.TreePath, } diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index e478f93655..e28c1edafc 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -337,6 +337,7 @@ func CreatePullReviewComment(ctx *context.APIContext) { pr.Issue, opts.Body, opts.Path, + pr.MergeBase, review.CommitID, line, review.ID, @@ -509,6 +510,7 @@ func CreatePullReview(ctx *context.APIContext) { c.Path, true, // pending review 0, // no reply + pr.MergeBase, opts.CommitID, nil, ); err != nil { diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 4a8217c719..2c5c7f94f3 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1027,7 +1027,11 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi } endCommitID = commitID - startCommitID = prInfo.MergeBase + if prevCommit != nil { + startCommitID = prevCommit.ID.String() + } else { + startCommitID = prInfo.MergeBase + } ctx.Data["IsShowingAllCommits"] = false } else if willShowSpecifiedCommitRange { if len(specifiedEndCommit) > 0 { diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index c2e30770d3..401f100d84 100644 --- a/routers/web/repo/pull_review.go +++ b/routers/web/repo/pull_review.go @@ -53,6 +53,15 @@ func RenderNewCodeCommentForm(ctx *context.Context) { } } ctx.Data["AfterCommitID"] = afterCommitID + beforeCommitID := ctx.FormString("before_commit_id") + if beforeCommitID == "" { + if err := issue.LoadPullRequest(ctx); err != nil { + ctx.ServerError("LoadPullRequest", err) + return + } + beforeCommitID = issue.PullRequest.MergeBase + } + ctx.Data["BeforeCommitID"] = beforeCommitID ctx.Data["IsAttachmentEnabled"] = setting.Attachment.Enabled upload.AddUploadContext(ctx, "comment") ctx.HTML(http.StatusOK, tplNewComment) @@ -112,6 +121,7 @@ func CreateCodeComment(ctx *context.Context) { form.TreePath, pendingReview, form.Reply, + form.BeforeCommitID, form.LatestCommitID, attachments, ) diff --git a/routers/web/repo/pull_review_test.go b/routers/web/repo/pull_review_test.go index dd0fa7df29..c068f575dd 100644 --- a/routers/web/repo/pull_review_test.go +++ b/routers/web/repo/pull_review_test.go @@ -51,7 +51,7 @@ func TestRenderConversation(t *testing.T) { var preparedComment *issues_model.Comment run("prepare", func(t *testing.T, ctx *context.Context, resp *httptest.ResponseRecorder) { comment, err := pull.CreateCodeComment(ctx, pr.Issue.Poster, ctx.Repo.GitRepo, pr.Issue, - 1, "content", "", false, 0, + 1, "content", "", false, 0, pr.MergeBase, prHeadCommitID, nil) require.NoError(t, err) diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go index f9443b8b6c..d1a5b521f8 100644 --- a/services/forms/repo_form.go +++ b/services/forms/repo_form.go @@ -472,6 +472,7 @@ type CodeCommentForm struct { TreePath string `form:"path" binding:"Required"` SingleReview bool `form:"single_review"` Reply int64 `form:"reply"` + BeforeCommitID string LatestCommitID string Files []string } diff --git a/services/mailer/incoming/incoming_handler.go b/services/mailer/incoming/incoming_handler.go index e2a2852b07..6fdf00ca59 100644 --- a/services/mailer/incoming/incoming_handler.go +++ b/services/mailer/incoming/incoming_handler.go @@ -152,6 +152,7 @@ func (h *ReplyHandler) Handle(ctx context.Context, content *MailContent, doer *u comment.TreePath, false, // not pending review but a single review comment.ReviewID, + issue.PullRequest.MergeBase, afterCommitID, attachmentIDs, ) diff --git a/services/pull/review.go b/services/pull/review.go index 9fc9862823..d8e04675ea 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -78,7 +78,10 @@ func InvalidateCodeComments(ctx context.Context, prs issues_model.PullRequestLis } // CreateCodeComment creates a comment on the code line -func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git.Repository, issue *issues_model.Issue, line int64, content, treePath string, pendingReview bool, replyReviewID int64, latestCommitID string, attachments []string) (*issues_model.Comment, error) { +func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git.Repository, + issue *issues_model.Issue, line int64, content, treePath string, pendingReview bool, + replyReviewID int64, beforeCommitID, latestCommitID string, attachments []string, +) (*issues_model.Comment, error) { var ( existsReview bool err error @@ -109,6 +112,7 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. issue, content, treePath, + beforeCommitID, latestCommitID, line, replyReviewID, @@ -151,6 +155,7 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. issue, content, treePath, + beforeCommitID, latestCommitID, line, review.ID, @@ -173,7 +178,10 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. } // CreateCodeCommentKnownReviewID creates a plain code comment at the specified line / path -func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, issue *issues_model.Issue, content, treePath, afterCommitID string, line, reviewID int64, attachments []string) (*issues_model.Comment, error) { +func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, + issue *issues_model.Issue, content, treePath, beforeCommitID, afterCommitID string, + line, reviewID int64, attachments []string, +) (*issues_model.Comment, error) { var commitID, blamedCommitID, patch string blamedLine := line if err := issue.LoadPullRequest(ctx); err != nil { @@ -237,9 +245,9 @@ func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User, // Commenting on a line that was removed. In this case, what we want to track in the comment is which line of // code was this, in the last commit that the line of code actually existed in. We'll use a reverse git blame to // identify this, from the PR base -> commit being viewed. - blame, err := gitRepo.ReverseLineBlame(pr.MergeBase, treePath, uint64(-1*line), afterCommitID) + blame, err := gitRepo.ReverseLineBlame(beforeCommitID, treePath, uint64(-1*line), afterCommitID) if err != nil { - return nil, fmt.Errorf("ReverseLineBlame[%s, %s, %d, %s]: %w", pr.MergeBase, treePath, -1*line, afterCommitID, err) + return nil, fmt.Errorf("ReverseLineBlame[%s, %s, %d, %s]: %w", beforeCommitID, treePath, -1*line, afterCommitID, err) } else if blame.CommitID == afterCommitID { // Although this is a comment on the "previous" side of the diff, the reverse blame indicates that the line // of code still exists in the commit being viewed (eg. it was a comment on a white line in the left-side of @@ -277,8 +285,8 @@ func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User, _ = writer.Close() }() go func() { - 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)) + if err := git.GetRepoRawDiffForFile(gitRepo, beforeCommitID, afterCommitID, git.RawDiffNormal, treePath, writer); err != nil { + _ = writer.CloseWithError(fmt.Errorf("GetRawDiffForLine[%s, %s, %s, %s]: %w", gitRepo.Path, pr.MergeBase, afterCommitID, treePath, err)) return } _ = writer.Close() diff --git a/templates/repo/diff/box.tmpl b/templates/repo/diff/box.tmpl index 5bd9a50b3b..7577ff0921 100644 --- a/templates/repo/diff/box.tmpl +++ b/templates/repo/diff/box.tmpl @@ -201,7 +201,7 @@ {{end}} {{else}} - +
{{if $.IsSplitStyle}} {{template "repo/diff/section_split" dict "file" . "root" $}} {{else}} diff --git a/templates/repo/diff/comment_form.tmpl b/templates/repo/diff/comment_form.tmpl index 922f3f3d73..7b64be71a0 100644 --- a/templates/repo/diff/comment_form.tmpl +++ b/templates/repo/diff/comment_form.tmpl @@ -1,6 +1,7 @@ {{if and $.root.SignedUserID (not $.Repository.IsArchived)}} + diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index b91fdab6d4..8c7b302c68 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -1730,6 +1730,67 @@ func TestPullRequestCommentPlacement(t *testing.T) { assert.True(t, commentReloaded.Invalidated) }, 1*time.Second, 50*time.Millisecond) }) + + t.Run("comment on removed line in specific commit adjusts to correct location", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + tester := newPullRequestCommentPlacementTester(t) + + // 3 commits: modify line 50, remove line 50, remove some earlier lines + content := tester.fileContent + content = strings.Replace(content, "Line 50\n", "Line 50--modified\n", 1) + commit1 := tester.changeFile("file1.md", content) + t.Logf("commit1 = %q", commit1) + content = strings.Replace(content, "Line 50--modified\n", "", 1) + commit2 := tester.changeFile("file1.md", content) + t.Logf("commit2 = %q", commit2) + content = strings.Replace(content, "Line 1\nLine 2\nLine 3\nLine 4\nLine 5\nLine 6\nLine 7\nLine 8\nLine 9\nLine 10\n", "", 1) + commit3 := tester.changeFile("file1.md", content) + t.Logf("commit3 = %q", commit3) + tester.createPR() + + comment := tester.commentOnPreviousFromSpecificCommit(commit2, "file1.md", 50) + assert.Equal(t, `diff --git a/file1.md b/file1.md +--- a/file1.md ++++ b/file1.md +@@ -47,7 +47,6 @@ Line 46 + Line 47 + Line 48 + Line 49 +-Line 50--modified`, comment.PatchQuoted) + assert.Equal(t, "previous", comment.DiffSide()) + assert.EqualValues(t, -50, comment.Line) + assert.Equal(t, commit1, comment.CommitSHA) + + diff1 := []diffTableRow{ + {rowType: RowHasCode, code: "Line 49"}, + {rowType: RowDelCode, code: "Line 50"}, + {rowType: RowAddCode, code: "Line 50--modified"}, + {rowType: RowHasCode, code: "Line 51"}, + } + tester.assertCommitDiff(commit1, diff1, "checking commit1 contents in single-commit diff") + + diff2 := []diffTableRow{ + {rowType: RowHasCode, code: "Line 49"}, + {rowType: RowDelCode, code: "Line 50--modified"}, + {rowType: RowComment, commentID: comment.ID}, + {rowType: RowHasCode, code: "Line 51"}, + } + tester.assertCommitDiff(commit2, diff2, "checking commit2 contents in single-commit diff") + + diff3 := []diffTableRow{ + {rowType: RowHasCode, code: "Line 49"}, + {rowType: RowDelCode, code: "Line 50"}, + // This is a small bug -- the comment was placed on the code "Line 50--modified" in commit1, which was + // later amended by commit2. This comment should be marked out-of-date and not appear here. But the + // comment's `ResolveCurrentLine` doesn't quite detect this case correctly -- as the comment's CommitSHA + // is commit1, it performs a diff commit1..PR-HEAD, not mergebase..PR-HEAD, and it believes that this + // line of code still exists because it exists in that diff range. It's a rare edge case that is defered + // for future repair. + {rowType: RowComment, commentID: comment.ID}, + {rowType: RowHasCode, code: "Line 51"}, + } + tester.assertFilesChangedDiff(diff3, "checking overall contents in full PR diff") + }) }) } @@ -1862,16 +1923,29 @@ func (tester *PullRequestCommentPlacementTester) commentOnPreviousFromFilesChang return tester.commentFromNewCommentForm(resp, filename, line, "previous") } +func (tester *PullRequestCommentPlacementTester) getCommitParent(commitID string) string { + repo, err := gitrepo.OpenRepository(tester.t.Context(), tester.repo) + require.NoError(tester.t, err) + defer repo.Close() + commit, err := repo.GetCommit(commitID) + require.NoError(tester.t, err) + require.Len(tester.t, commit.Parents, 1) + return commit.Parents[0].String() +} + func (tester *PullRequestCommentPlacementTester) commentFromSpecificCommit(commitID, filename string, line int) *issues_model.Comment { + beforeCommitID := tester.getCommitParent(commitID) req := NewRequest(tester.t, "GET", - fmt.Sprintf("/%s/%s/pulls/%d/files/reviews/new_comment?after_commit_id=%s", tester.repo.OwnerName, tester.repo.Name, tester.pr.Index, commitID)) + fmt.Sprintf("/%s/%s/pulls/%d/files/reviews/new_comment?before_commit_id=%s&after_commit_id=%s", tester.repo.OwnerName, tester.repo.Name, tester.pr.Index, beforeCommitID, commitID)) resp := tester.session.MakeRequest(tester.t, req, http.StatusOK) return tester.commentFromNewCommentForm(resp, filename, line, "proposed") } func (tester *PullRequestCommentPlacementTester) commentOnPreviousFromSpecificCommit(commitID, filename string, line int) *issues_model.Comment { + beforeCommitID := tester.getCommitParent(commitID) + tester.t.Logf("beforeCommitID(%q) = %q", commitID, beforeCommitID) req := NewRequest(tester.t, "GET", - fmt.Sprintf("/%s/%s/pulls/%d/files/reviews/new_comment?after_commit_id=%s", tester.repo.OwnerName, tester.repo.Name, tester.pr.Index, commitID)) + fmt.Sprintf("/%s/%s/pulls/%d/files/reviews/new_comment?before_commit_id=%s&after_commit_id=%s", tester.repo.OwnerName, tester.repo.Name, tester.pr.Index, beforeCommitID, commitID)) resp := tester.session.MakeRequest(tester.t, req, http.StatusOK) return tester.commentFromNewCommentForm(resp, filename, line, "previous") } @@ -1879,10 +1953,13 @@ func (tester *PullRequestCommentPlacementTester) commentOnPreviousFromSpecificCo func (tester *PullRequestCommentPlacementTester) commentFromNewCommentForm(resp *httptest.ResponseRecorder, filename string, line int, side string) *issues_model.Comment { commentContent := uuid.New().String() doc := NewHTMLParser(tester.t, resp.Body) + tester.t.Logf("doc.before = %q", doc.GetInputValueByName("before_commit_id")) + tester.t.Logf("doc.latest = %q", doc.GetInputValueByName("latest_commit_id")) req := NewRequestWithValues(tester.t, "POST", fmt.Sprintf("/%s/%s/pulls/%d/files/reviews/comments", tester.repo.OwnerName, tester.repo.Name, tester.pr.Index), map[string]string{ "origin": doc.GetInputValueByName("origin"), + "before_commit_id": doc.GetInputValueByName("before_commit_id"), "latest_commit_id": doc.GetInputValueByName("latest_commit_id"), "side": side, // "proposed" (RHS) or "previous" (LHS) "line": strconv.Itoa(line),