diff --git a/models/issues/comment.go b/models/issues/comment.go index bfad3935fb..61d49cf7b6 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -19,7 +19,9 @@ import ( project_model "forgejo.org/models/project" repo_model "forgejo.org/models/repo" user_model "forgejo.org/models/user" + "forgejo.org/modules/cache" "forgejo.org/modules/container" + "forgejo.org/modules/git" "forgejo.org/modules/gitrepo" "forgejo.org/modules/json" "forgejo.org/modules/log" @@ -321,6 +323,8 @@ type Comment struct { CommitsNum int64 `xorm:"-"` IsForcePush bool `xorm:"-"` + reverseLineBlame *git.ReverseLineBlame `xorm:"-"` + // If you add new fields that might be used to store abusive content (mainly string fields), // please also add them in the CommentData struct and the corresponding constructor. } @@ -744,6 +748,55 @@ func (c *Comment) UnsignedLine() uint64 { return uint64(c.Line) } +func (c *Comment) ResolveCurrentLine(ctx context.Context, repo *repo_model.Repository, currentHead string) (*git.ReverseLineBlame, error) { + if c.reverseLineBlame != nil { + return c.reverseLineBlame, nil + } + + // When a PR is viewed, the requirement to perform `git blame --reverse...` on every comment is a bit of a + // performance risk. To minimize this risk, cache the results relative to the requested head, so it only needs to be + // recalculated when head changes (or on cache eviction). + // + // Some performance testing was done which showed that a hot cache is much faster than the blame reverse + // operation -- 500-1000x runtime difference: + // + // - cache miss (Forgejo repo) took 7,690,574 ns + // - cache miss (~1000 commit repo) took 1,671,223 ns + // - cache hit (in-memory adapter) took 3,710 ns + // - cache hit (redis adapter) took 77,311 ns + resolveJSON, err := cache.GetString(fmt.Sprintf("comment.Resolve;ID=%d;HEAD=%s", c.ID, currentHead), func() (string, error) { + gitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, repo) + if err != nil { + return "", fmt.Errorf("failed to open repo: %w", err) + } + defer closer.Close() + + reverseBlame, err := gitRepo.ReverseLineBlame(c.CommitSHA, c.TreePath, c.UnsignedLine(), currentHead) + if err != nil { + return "", fmt.Errorf("failed to perform `git blame --reverse` to resolve current line for comment (id=%d): %w", c.ID, err) + } + + data, err := json.Marshal(reverseBlame) + if err != nil { + return "", err + } + + return string(data), nil + }) + if err != nil { + return nil, err + } + + var reverseBlame *git.ReverseLineBlame + err = json.Unmarshal([]byte(resolveJSON), &reverseBlame) + if err != nil { + return nil, err + } + + c.reverseLineBlame = reverseBlame + return c.reverseLineBlame, nil +} + // CodeCommentLink returns the url to a comment in code func (c *Comment) CodeCommentLink(ctx context.Context) string { err := c.LoadIssue(ctx) diff --git a/models/issues/comment_code.go b/models/issues/comment_code.go index 800d1e830e..cc46541743 100644 --- a/models/issues/comment_code.go +++ b/models/issues/comment_code.go @@ -7,7 +7,10 @@ import ( "context" "forgejo.org/models/db" + repo_model "forgejo.org/models/repo" user_model "forgejo.org/models/user" + "forgejo.org/modules/git" + "forgejo.org/modules/log" "forgejo.org/modules/markup" "forgejo.org/modules/markup/markdown" @@ -23,33 +26,62 @@ type CodeConversationsAtLine map[int64][]CodeConversation // CodeConversationsAtLineAndTreePath contains the conversations for a given TreePath and line type CodeConversationsAtLineAndTreePath map[string]CodeConversationsAtLine -func newCodeConversationsAtLineAndTreePath(comments []*Comment) CodeConversationsAtLineAndTreePath { +func newCodeConversationsAtLineAndTreePath(ctx context.Context, comments []*Comment, repo *repo_model.Repository, headCommitID string) (CodeConversationsAtLineAndTreePath, error) { tree := make(CodeConversationsAtLineAndTreePath) for _, comment := range comments { - tree.insertComment(comment) + blame, err := comment.ResolveCurrentLine(ctx, repo, headCommitID) + if err != nil { + // ResolveCurrentLine can fail in at least one known situation -- where a comment is left on a line in a + // file that is being deleted. The blame would be for the commit that deleted the file, and a reverse git + // blame won't work because the file is missing in the target sha. + log.Warn("ResolveCurrentLine failed: %s", err.Error()) + // handle gracefully -- insertComment will use the original values which may be usable + blame = nil + } else if blame.CommitID != headCommitID { + // Commit was made on a line that can't be reverse-blamed to the currently viewing head. This can happen + // because: + // - line of code was removed between the commit it was tagged on, and the head commit + // - force push on the repo caused there to be no git relationship between blame.CommitID->headCommitID + // We won't insert this comment into the comment tree because we don't know where to place it; it may appear + // when the user views a different commit in the PR, and it will always appear on the "Conversations" tab. + continue + } + tree.insertComment(comment, blame) } - return tree + return tree, nil } -func (tree CodeConversationsAtLineAndTreePath) insertComment(comment *Comment) { +func (tree CodeConversationsAtLineAndTreePath) insertComment(comment *Comment, blame *git.ReverseLineBlame) { + treePath := comment.TreePath + line := comment.Line + if blame != nil { + treePath = blame.FilePath + line = int64(blame.LineNumber) + if comment.Line < 0 { + line *= -1 + } + } + // attempt to append comment to existing conversations (i.e. list of comments belonging to the same review) - for i, conversation := range tree[comment.TreePath][comment.Line] { + for i, conversation := range tree[treePath][line] { if conversation[0].ReviewID == comment.ReviewID { - tree[comment.TreePath][comment.Line][i] = append(conversation, comment) + tree[treePath][line][i] = append(conversation, comment) return } } // no previous conversation was found at this line, create it - if tree[comment.TreePath] == nil { - tree[comment.TreePath] = make(map[int64][]CodeConversation) + if tree[treePath] == nil { + tree[treePath] = make(map[int64][]CodeConversation) } - tree[comment.TreePath][comment.Line] = append(tree[comment.TreePath][comment.Line], CodeConversation{comment}) + tree[treePath][line] = append(tree[treePath][line], CodeConversation{comment}) } -// FetchCodeConversations will return a 2d-map: ["Path"]["Line"] = List of CodeConversation (one per review) for this line -func FetchCodeConversations(ctx context.Context, issue *Issue, doer *user_model.User, showOutdatedComments bool) (CodeConversationsAtLineAndTreePath, error) { +// FetchCodeConversations will return a 2d-map: ["Path"]["Line"] = List of CodeConversation (one per review) for this +// line. headCommitID will be used to reverse-blame the comment into the correct path & line for the current context +// that is being viewed. +func FetchCodeConversations(ctx context.Context, issue *Issue, doer *user_model.User, showOutdatedComments bool, headCommitID string) (CodeConversationsAtLineAndTreePath, error) { opts := FindCommentsOptions{ Type: CommentTypeCode, IssueID: issue.ID, @@ -59,7 +91,7 @@ func FetchCodeConversations(ctx context.Context, issue *Issue, doer *user_model. return nil, err } - return newCodeConversationsAtLineAndTreePath(comments), nil + return newCodeConversationsAtLineAndTreePath(ctx, comments, issue.Repo, headCommitID) } // CodeComments represents comments on code by using this structure: FILENAME -> LINE (+ == proposed; - == previous) -> COMMENTS diff --git a/models/issues/comment_test.go b/models/issues/comment_test.go index da87b8ec2f..ea59d4f215 100644 --- a/models/issues/comment_test.go +++ b/models/issues/comment_test.go @@ -63,7 +63,7 @@ func TestFetchCodeConversations(t *testing.T) { unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 4}), user, true)) - res, err := issues_model.FetchCodeConversations(db.DefaultContext, issue, user, false) + res, err := issues_model.FetchCodeConversations(db.DefaultContext, issue, user, false, "") require.NoError(t, err) require.Contains(t, res, "README.md") require.Contains(t, res["README.md"], int64(4)) @@ -77,7 +77,7 @@ func TestFetchCodeConversations(t *testing.T) { assert.NotNil(t, r.User) user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) - res, err = issues_model.FetchCodeConversations(db.DefaultContext, issue, user2, false) + res, err = issues_model.FetchCodeConversations(db.DefaultContext, issue, user2, false, "") require.NoError(t, err) assert.Len(t, res, 1) } diff --git a/modules/git/repo_blame.go b/modules/git/repo_blame.go index 50f4c572d5..d760898ac6 100644 --- a/modules/git/repo_blame.go +++ b/modules/git/repo_blame.go @@ -65,3 +65,97 @@ func (repo *Repository) LineBlame(revision, file string, line uint64) (*Commit, return commit, originalLineNo, nil } + +type ReverseLineBlame struct { + CommitID string + LineNumber uint64 + FilePath string +} + +// Reverses the effect of [LineBlame]. If a file was modified at originalLine number in originalRevision, +// ReverseLineBlame will identify the last commit up-to-and-including currentRevision where that line exists, including +// its new path and line number. If the returned commit is not the same as currentRevision, then it indicates that +// content can no longer be located in currentRevision, and the returned commit is the last commit that had it. +func (repo *Repository) ReverseLineBlame(originalRevision, file string, originalLine uint64, currentRevision string) (*ReverseLineBlame, error) { + if originalRevision == currentRevision { + // Would cause an error to run the reverse, "fatal: More than one commit to dig up from, (N) and (N)" + return &ReverseLineBlame{ + CommitID: originalRevision, + LineNumber: originalLine, + FilePath: file, + }, nil + } + + res, _, gitErr := NewCommand(repo.Ctx, "blame"). + AddOptionValues("--reverse"). + AddDynamicArguments(fmt.Sprintf("%s..%s", originalRevision, currentRevision)). + AddOptionFormat("-L %d,%d", originalLine, originalLine). + AddOptionValues("-p"). + AddDashesAndList(file).RunStdString(&RunOpts{Dir: repo.Path}) + if gitErr != nil { + return nil, gitErr + } + + // Example output: + // + // 74be0e8aa338d1374ab7ca0a25a4f594955a69c2 16 9 1 + // author FirstName LastName + // author-mail + // author-time 1775492007 + // author-tz -0600 + // committer FirstName LastName + // committer-mail + // committer-time 1775492007 + // committer-tz -0600 + // summary restore file-in-base to orig, now not present in diff + // filename README.md + // + // Header (https://git-scm.com/docs/git-blame#_the_porcelain_format): + // - 40-byte SHA-1 of the commit the line is attributed to; + // - the line number of the line in the original file; [note: opposite in reverse] + // - the line number of the line in the final file; [note: opposite in reverse] + // - on a line that starts a group of lines from a different commit than the previous one, the number of lines in + // this group. On subsequent lines this field is absent. + + lines := strings.Split(res, "\n") + + header := lines[0] + headerValues := strings.Split(header, " ") + if len(headerValues) < 2 { + return nil, fmt.Errorf("failed to parse blame --reverse header: %q", header) + } + + objectFormat, err := repo.GetObjectFormat() + if err != nil { + return nil, err + } + objectIDLen := objectFormat.FullLength() + objectID := headerValues[0] + if len(objectID) != objectIDLen { + return nil, fmt.Errorf("output of blame is invalid, cannot contain commit ID: %s", objectID) + } + commit, err := repo.GetCommit(objectID) + if err != nil { + return nil, fmt.Errorf("GetCommit: %w", err) + } + + currentLineStr := headerValues[1] + currentLineNo, err := strconv.ParseUint(currentLineStr, 10, 64) + if err != nil { + return nil, fmt.Errorf("strconv.ParseUint: %w", err) + } + + var filename string + for _, otherLine := range lines { + if strings.HasPrefix(otherLine, "filename ") { + filename = otherLine[len("filename "):] + break + } + } + + return &ReverseLineBlame{ + CommitID: commit.ID.String(), + LineNumber: currentLineNo, + FilePath: filename, + }, nil +} diff --git a/modules/git/repo_blame_test.go b/modules/git/repo_blame_test.go index 4ddd5d9c3f..5803d082f0 100644 --- a/modules/git/repo_blame_test.go +++ b/modules/git/repo_blame_test.go @@ -89,11 +89,23 @@ func TestLineBlame(t *testing.T) { assert.Equal(t, firstCommit, commit.ID.String()) assert.EqualValues(t, 1, lineno) + rev, err := gitRepo.ReverseLineBlame(commit.ID.String(), "ANSWER", lineno, secondCommit) + require.NoError(t, err) + assert.Equal(t, secondCommit, rev.CommitID) + assert.Equal(t, "ANSWER", rev.FilePath) + assert.EqualValues(t, 10, rev.LineNumber) + for i := range uint64(9) { commit, lineno, err = gitRepo.LineBlame("HEAD", "ANSWER", i+1) require.NoError(t, err) assert.Equal(t, secondCommit, commit.ID.String()) assert.Equal(t, i+1, lineno) + + rev, err := gitRepo.ReverseLineBlame(commit.ID.String(), "ANSWER", lineno, secondCommit) + require.NoError(t, err) + assert.Equal(t, secondCommit, rev.CommitID) + assert.Equal(t, "ANSWER", rev.FilePath) + assert.Equal(t, i+1, rev.LineNumber) } } @@ -108,3 +120,66 @@ func TestLineBlame(t *testing.T) { }) }) } + +func TestReverseLineBlame(t *testing.T) { + t.Run("single commit", func(t *testing.T) { + tmpDir := t.TempDir() + require.NoError(t, InitRepository(t.Context(), tmpDir, false, Sha1ObjectFormat.Name())) + + gitRepo, err := OpenRepository(t.Context(), tmpDir) + require.NoError(t, err) + defer gitRepo.Close() + + require.NoError(t, os.WriteFile(path.Join(tmpDir, "file1.md"), []byte("abba\n"), 0o666)) + require.NoError(t, AddChanges(tmpDir, true)) + require.NoError(t, CommitChanges(tmpDir, CommitChangesOptions{Message: "abba spelt backwards"})) + + commit, err := gitRepo.GetRefCommitID("HEAD") + require.NoError(t, err) + + blameCommit, lineno, err := gitRepo.LineBlame("HEAD", "file1.md", 1) + require.NoError(t, err) + assert.Equal(t, commit, blameCommit.ID.String()) + assert.EqualValues(t, 1, lineno) + + rev, err := gitRepo.ReverseLineBlame(commit, "file1.md", lineno, commit) + require.NoError(t, err) + assert.Equal(t, commit, rev.CommitID) + assert.Equal(t, "file1.md", rev.FilePath) + assert.EqualValues(t, 1, rev.LineNumber) + }) + + t.Run("move file", func(t *testing.T) { + tmpDir := t.TempDir() + require.NoError(t, InitRepository(t.Context(), tmpDir, false, Sha1ObjectFormat.Name())) + + gitRepo, err := OpenRepository(t.Context(), tmpDir) + require.NoError(t, err) + defer gitRepo.Close() + + require.NoError(t, os.WriteFile(path.Join(tmpDir, "file1.md"), []byte("abba\n"), 0o666)) + require.NoError(t, AddChanges(tmpDir, true)) + require.NoError(t, CommitChanges(tmpDir, CommitChangesOptions{Message: "abba spelt backwards"})) + + firstCommit, err := gitRepo.GetRefCommitID("HEAD") + require.NoError(t, err) + + require.NoError(t, os.Rename(path.Join(tmpDir, "file1.md"), path.Join(tmpDir, "file2.md"))) + require.NoError(t, AddChanges(tmpDir, true)) + require.NoError(t, CommitChanges(tmpDir, CommitChangesOptions{Message: "move file"})) + + secondCommit, err := gitRepo.GetRefCommitID("HEAD") + require.NoError(t, err) + + blameCommit, lineno, err := gitRepo.LineBlame("HEAD", "file2.md", 1) + require.NoError(t, err) + assert.Equal(t, firstCommit, blameCommit.ID.String()) + assert.EqualValues(t, 1, lineno) + + rev, err := gitRepo.ReverseLineBlame(firstCommit, "file1.md", lineno, secondCommit) + require.NoError(t, err) + assert.Equal(t, secondCommit, rev.CommitID) + assert.Equal(t, "file2.md", rev.FilePath) + assert.EqualValues(t, 1, rev.LineNumber) + }) +} diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index dc36d2de68..4a8217c719 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1099,7 +1099,7 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi "numberOfViewedFiles": diff.NumViewedFiles, } - if err = diff.LoadComments(ctx, issue, ctx.Doer, ctx.Data["ShowOutdatedComments"].(bool)); err != nil { + if err = diff.LoadComments(ctx, issue, ctx.Doer, ctx.Data["ShowOutdatedComments"].(bool), endCommitID); err != nil { ctx.ServerError("LoadComments", err) return } diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index c1d0bc0107..fa3c26f149 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -487,8 +487,8 @@ type Diff struct { } // LoadComments loads comments into each line -func (diff *Diff) LoadComments(ctx context.Context, issue *issues_model.Issue, currentUser *user_model.User, showOutdatedComments bool) error { - allConversations, err := issues_model.FetchCodeConversations(ctx, issue, currentUser, showOutdatedComments) +func (diff *Diff) LoadComments(ctx context.Context, issue *issues_model.Issue, currentUser *user_model.User, showOutdatedComments bool, headCommitID string) error { + allConversations, err := issues_model.FetchCodeConversations(ctx, issue, currentUser, showOutdatedComments, headCommitID) if err != nil { return err } diff --git a/services/gitdiff/gitdiff_test.go b/services/gitdiff/gitdiff_test.go index 7ba439be35..9748bf3828 100644 --- a/services/gitdiff/gitdiff_test.go +++ b/services/gitdiff/gitdiff_test.go @@ -600,7 +600,7 @@ func TestDiff_LoadCommentsNoOutdated(t *testing.T) { issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2}) user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) diff := setupDefaultDiff() - require.NoError(t, diff.LoadComments(db.DefaultContext, issue, user, false)) + require.NoError(t, diff.LoadComments(db.DefaultContext, issue, user, false, "")) assert.Len(t, diff.Files[0].Sections[0].Lines[0].Conversations, 2) } @@ -610,7 +610,7 @@ func TestDiff_LoadCommentsWithOutdated(t *testing.T) { issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2}) user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) diff := setupDefaultDiff() - require.NoError(t, diff.LoadComments(db.DefaultContext, issue, user, true)) + require.NoError(t, diff.LoadComments(db.DefaultContext, issue, user, true, "")) assert.Len(t, diff.Files[0].Sections[0].Lines[0].Conversations, 2) assert.Len(t, diff.Files[0].Sections[0].Lines[0].Conversations[0], 2) assert.Len(t, diff.Files[0].Sections[0].Lines[0].Conversations[1], 1) diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 69628e08b5..1a60c1ea11 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -6,6 +6,7 @@ package integration import ( "bytes" + "encoding/base64" "fmt" "io" "net/http" @@ -13,11 +14,13 @@ import ( "net/url" "os" "path" + "slices" "strconv" "strings" "testing" "time" + auth_model "forgejo.org/models/auth" "forgejo.org/models/db" issues_model "forgejo.org/models/issues" repo_model "forgejo.org/models/repo" @@ -26,7 +29,9 @@ import ( user_model "forgejo.org/models/user" "forgejo.org/modules/git" "forgejo.org/modules/gitrepo" + "forgejo.org/modules/optional" repo_module "forgejo.org/modules/repository" + api "forgejo.org/modules/structs" "forgejo.org/modules/test" issue_service "forgejo.org/services/issue" "forgejo.org/services/mailer" @@ -35,8 +40,10 @@ import ( "forgejo.org/tests" "github.com/PuerkitoBio/goquery" + "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "golang.org/x/net/html" ) func TestPullView_ReviewerMissed(t *testing.T) { @@ -1048,3 +1055,659 @@ func TestPullRequestStaleReview(t *testing.T) { }) }) } + +func TestPullRequestCommentPlacement(t *testing.T) { + onApplicationRun(t, func(t *testing.T, u *url.URL) { + t.Run("comment directly on change in PR", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + tester := newPullRequestCommentPlacementTester(t) + + commitSHA := tester.changeFile("file1.md", + strings.Replace(tester.fileContent, "Line 50\n", "Line 50--modified\n", 1)) + tester.createPR() + + comment := tester.commentFromFilesChanged("file1.md", 50) + assert.Equal(t, `diff --git a/file1.md b/file1.md +--- a/file1.md ++++ b/file1.md +@@ -48,3 +48,3 @@ + Line 48 + Line 49 +-Line 50 ++Line 50--modified`, comment.PatchQuoted) + assert.Equal(t, "proposed", comment.DiffSide()) + assert.EqualValues(t, 50, comment.Line) + assert.Equal(t, commitSHA, comment.CommitSHA) + + diff := []diffTableRow{ + {rowType: RowHasCode, code: "Line 49"}, + {rowType: RowDelCode, code: "Line 50"}, + {rowType: RowAddCode, code: "Line 50--modified"}, + {rowType: RowComment, commentID: comment.ID}, + {rowType: RowHasCode, code: "Line 51"}, + } + tester.assertFilesChangedDiff(diff) + tester.assertCommitDiff(commitSHA, diff) + }) + + t.Run("comment lands on blame from commit within PR", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + tester := newPullRequestCommentPlacementTester(t) + + // Modify an earlier part of the file in one commit, and a later part of the file in a second commit. + content := tester.fileContent + content = strings.Replace(content, "Line 25\n", "Line 25--modified\n", 1) + commit1 := tester.changeFile("file1.md", content) + content = strings.Replace(content, "Line 75\n", "Line 75--modified\n", 1) + commit2 := tester.changeFile("file1.md", content) + tester.createPR() + + // Comment on the earlier change, from the "Files changed" view; this should "git blame" and be asociated + // with the first commit where that change was made, therefore appearing on the commit-specific diff later: + comment := tester.commentFromFilesChanged("file1.md", 25) + assert.Equal(t, `diff --git a/file1.md b/file1.md +--- a/file1.md ++++ b/file1.md +@@ -23,3 +23,3 @@ + Line 23 + Line 24 +-Line 25 ++Line 25--modified`, comment.PatchQuoted) + assert.Equal(t, "proposed", comment.DiffSide()) + assert.EqualValues(t, 25, comment.Line) + assert.Equal(t, commit1, comment.CommitSHA) + + diff25 := []diffTableRow{ + {rowType: RowHasCode, code: "Line 24"}, + {rowType: RowDelCode, code: "Line 25"}, + {rowType: RowAddCode, code: "Line 25--modified"}, + {rowType: RowComment, commentID: comment.ID}, + {rowType: RowHasCode, code: "Line 26"}, + } + tester.assertFilesChangedDiff(diff25) + tester.assertCommitDiff(commit1, diff25) + + diff75 := []diffTableRow{ + {rowType: RowHasCode, code: "Line 74"}, + {rowType: RowDelCode, code: "Line 75"}, + {rowType: RowAddCode, code: "Line 75--modified"}, + {rowType: RowHasCode, code: "Line 76"}, + } + tester.assertFilesChangedDiff(diff75) + tester.assertCommitDiff(commit2, diff75) + }) + + t.Run("comment lands on blame commit from before PR", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + tester := newPullRequestCommentPlacementTester(t) + + // Modify line 50... + commitSHA := tester.changeFile("file1.md", + strings.Replace(tester.fileContent, "Line 50\n", "Line 50--modified\n", 1)) + tester.createPR() + + // But while viewing line 50's diff, place a comment on line 49. This will "git blame" to a commit outside + // of this PR, but that's fine... + comment := tester.commentFromFilesChanged("file1.md", 49) + assert.Equal(t, `diff --git a/file1.md b/file1.md +--- a/file1.md ++++ b/file1.md +@@ -47,7 +47,7 @@ Line 46 + Line 47 + Line 48 + Line 49`, comment.PatchQuoted) + assert.Equal(t, "proposed", comment.DiffSide()) + assert.EqualValues(t, 49, comment.Line) + assert.NotEqual(t, commitSHA, comment.CommitSHA) + + diff := []diffTableRow{ + {rowType: RowHasCode, code: "Line 48"}, + {rowType: RowHasCode, code: "Line 49"}, + {rowType: RowComment, commentID: comment.ID}, + {rowType: RowDelCode, code: "Line 50"}, + {rowType: RowAddCode, code: "Line 50--modified"}, + {rowType: RowHasCode, code: "Line 51"}, + } + tester.assertFilesChangedDiff(diff) + tester.assertCommitDiff(commitSHA, diff) + }) + + t.Run("comment on line moves due to a following commit", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + tester := newPullRequestCommentPlacementTester(t) + + // Modify line 50 + content := tester.fileContent + content = strings.Replace(content, "Line 50\n", "Line 50--modified\n", 1) + commit1 := tester.changeFile("file1.md", content) + tester.createPR() + + // Place a comment on "Line 50--modified" + comment := tester.commentFromFilesChanged("file1.md", 50) + assert.Equal(t, `diff --git a/file1.md b/file1.md +--- a/file1.md ++++ b/file1.md +@@ -48,3 +48,3 @@ + Line 48 + Line 49 +-Line 50 ++Line 50--modified`, comment.PatchQuoted) + assert.Equal(t, "proposed", comment.DiffSide()) + assert.EqualValues(t, 50, comment.Line) + assert.Equal(t, commit1, comment.CommitSHA) + + // Add a second commit to the PR which removes "Line 1" - "Line 10". + 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) + commit2 := tester.changeFile("file1.md", content) + + diff2 := []diffTableRow{ + {rowType: RowDelCode, code: "Line 9"}, + {rowType: RowDelCode, code: "Line 10"}, + {rowType: RowHasCode, code: "Line 11"}, + } + tester.assertFilesChangedDiff(diff2, "checking commit2 contents in full PR diff") + tester.assertCommitDiff(commit2, diff2, "checking commit2 contents in single-commit diff") + + diff1 := []diffTableRow{ + {rowType: RowHasCode, code: "Line 49"}, + {rowType: RowDelCode, code: "Line 50"}, + {rowType: RowAddCode, code: "Line 50--modified"}, + {rowType: RowComment, commentID: comment.ID}, + {rowType: RowHasCode, code: "Line 51"}, + } + tester.assertFilesChangedDiff(diff1, "checking commit1 contents in full PR diff") + tester.assertCommitDiff(commit1, diff1, "checking commit1 contents in single-commit diff") + }) + + t.Run("comment on line moves due to a following commit, following commit is rewritten and force-push'd", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + tester := newPullRequestCommentPlacementTester(t) + + // Modify line 50 + content := tester.fileContent + content = strings.Replace(content, "Line 50\n", "Line 50--modified\n", 1) + commit1 := tester.changeFile("file1.md", content) + tester.createPR() + + // Place a comment on "Line 50--modified" + comment := tester.commentFromFilesChanged("file1.md", 50) + assert.Equal(t, `diff --git a/file1.md b/file1.md +--- a/file1.md ++++ b/file1.md +@@ -48,3 +48,3 @@ + Line 48 + Line 49 +-Line 50 ++Line 50--modified`, comment.PatchQuoted) + assert.Equal(t, "proposed", comment.DiffSide()) + assert.EqualValues(t, 50, comment.Line) + assert.Equal(t, commit1, comment.CommitSHA) + + // Add a second commit to the PR which removes "Line 1" - "Line 10". + 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) + tester.changeFile("file1.md", content) + + // Now amend commit2v1 with an additional change, causing a force push of the branch + tester.withBranchCheckout(func(repoPath string) { + content = strings.Replace(content, "Line 11\n", "", 1) // Remove Line 11 as well + require.NoError(t, os.WriteFile(path.Join(repoPath, "file1.md"), []byte(content), 0o644)) + require.NoError(t, git.NewCommand(t.Context(), "commit", "-a", "--amend", "--no-edit").Run(&git.RunOpts{Dir: repoPath})) + require.NoError(t, git.NewCommand(t.Context(), "push", "--force").Run(&git.RunOpts{Dir: repoPath})) + }) + + diff2 := []diffTableRow{ + {rowType: RowDelCode, code: "Line 10"}, + {rowType: RowDelCode, code: "Line 11"}, + {rowType: RowHasCode, code: "Line 12"}, + } + tester.assertFilesChangedDiff(diff2, "checking commit2 (force push) contents in full PR diff") + + diff1 := []diffTableRow{ + {rowType: RowHasCode, code: "Line 49"}, + {rowType: RowDelCode, code: "Line 50"}, + {rowType: RowAddCode, code: "Line 50--modified"}, + {rowType: RowComment, commentID: comment.ID}, + {rowType: RowHasCode, code: "Line 51"}, + } + tester.assertFilesChangedDiff(diff1, "checking commit1 contents in full PR diff") + tester.assertCommitDiff(commit1, diff1, "checking commit1 contents in single-commit diff") + }) + + t.Run("comment on line commit is rewritten and force-push'd", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + tester := newPullRequestCommentPlacementTester(t) + + // Modify line 50 + content := tester.fileContent + content = strings.Replace(content, "Line 50\n", "Line 50--modified\n", 1) + commit1 := tester.changeFile("file1.md", content) + tester.createPR() + + // Place a comment on "Line 50--modified" + comment := tester.commentFromFilesChanged("file1.md", 50) + assert.Equal(t, `diff --git a/file1.md b/file1.md +--- a/file1.md ++++ b/file1.md +@@ -48,3 +48,3 @@ + Line 48 + Line 49 +-Line 50 ++Line 50--modified`, comment.PatchQuoted) + assert.Equal(t, "proposed", comment.DiffSide()) + assert.EqualValues(t, 50, comment.Line) + assert.Equal(t, commit1, comment.CommitSHA) + + // Add a second commit to the PR which removes "Line 1" - "Line 10". + 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) + commit2 := tester.changeFile("file1.md", content) + + // Now, reorganize these commits, so that it's main->commit2->commit1 on the branch, rather than + // main->commit1->commit2. Then force push the branch. + tester.withBranchCheckout(func(repoPath string) { + // move commit2 onto main, off commit1 + require.NoError(t, + git.NewCommand(t.Context(), "rebase"). + AddArguments("--onto").AddDynamicArguments(tester.initialSHA). + AddDynamicArguments(commit1). + AddDynamicArguments(commit2). + Run(&git.RunOpts{Dir: repoPath})) + // move commit1 onto HEAD, off main + require.NoError(t, + git.NewCommand(t.Context(), "rebase"). + AddArguments("--onto").AddDynamicArguments("HEAD"). + AddDynamicArguments(tester.initialSHA). + AddDynamicArguments(commit1). + Run(&git.RunOpts{Dir: repoPath})) + + // delete branch for the PR + has, branch := tester.branch.Get() + require.True(t, has) + require.NoError(t, + git.NewCommand(t.Context(), "branch"). + AddArguments("-D").AddDynamicArguments(branch). + Run(&git.RunOpts{Dir: repoPath})) + // call HEAD as the branch + require.NoError(t, + git.NewCommand(t.Context(), "branch"). + AddDynamicArguments(branch). + Run(&git.RunOpts{Dir: repoPath})) + + // force push the rebuilt branch + require.NoError(t, git.NewCommand(t.Context(), "push", "--force", "origin").AddDynamicArguments(branch).Run(&git.RunOpts{Dir: repoPath})) + }) + + diff2 := []diffTableRow{ + {rowType: RowDelCode, code: "Line 10"}, + {rowType: RowHasCode, code: "Line 11"}, + } + tester.assertFilesChangedDiff(diff2, "checking commit2 (force push) contents in full PR diff") + + diff1 := []diffTableRow{ + {rowType: RowHasCode, code: "Line 49"}, + {rowType: RowDelCode, code: "Line 50"}, + {rowType: RowAddCode, code: "Line 50--modified"}, + // no comment visible anymore; force push has lost its place at this time + {rowType: RowHasCode, code: "Line 51"}, + } + tester.assertFilesChangedDiff(diff1, "checking commit1 contents in full PR diff") + }) + + t.Run("comment lands on blame with original line number varying from current", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + tester := newPullRequestCommentPlacementTester(t) + + // Remove "Line 1" - "Line 10", on the base branch. If you "git blame" Line 50 at that point, it will have + // an original line number 50, but actually be appearing at line number index 40, causing wrong outputs. + content := tester.fileContent + 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) + tester.changeFileOnBase("file1.md", content) + + // Now modify "Line 51" in a PR: + commitSHA := tester.changeFile("file1.md", strings.Replace(content, "Line 51\n", "Line 51--modified\n", 1)) + tester.createPR() + + // Place a comment on "Line 50", which would "git blame" to the original commit and line number 50, even + // though it's now actually at line number 40. + comment := tester.commentFromFilesChanged("file1.md", lineNumber(content, "Line 50")) + assert.Equal(t, `diff --git a/file1.md b/file1.md +--- a/file1.md ++++ b/file1.md +@@ -38,7 +38,7 @@ Line 47 + Line 48 + Line 49 + Line 50`, comment.PatchQuoted) + assert.Equal(t, "proposed", comment.DiffSide()) + assert.EqualValues(t, 50, comment.Line) + assert.Equal(t, tester.initialSHA, comment.CommitSHA) + + diff := []diffTableRow{ + {rowType: RowHasCode, code: "Line 49"}, + {rowType: RowHasCode, code: "Line 50"}, + {rowType: RowComment, commentID: comment.ID}, + {rowType: RowDelCode, code: "Line 51"}, + {rowType: RowAddCode, code: "Line 51--modified"}, + {rowType: RowHasCode, code: "Line 52"}, + } + tester.assertFilesChangedDiff(diff) + tester.assertCommitDiff(commitSHA, diff) + }) + }) +} + +type PullRequestCommentPlacementTester struct { + t *testing.T + user *user_model.User + session *TestSession + apiToken string + fileContent string + repo *repo_model.Repository + initialSHA string + branch optional.Option[string] + pr *api.PullRequest +} + +func newPullRequestCommentPlacementTester(t *testing.T) *PullRequestCommentPlacementTester { + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + token := getUserToken(t, "user2", auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteIssue) + session := loginUser(t, user2.Name) + + var content strings.Builder + for i := range 100 { + content.WriteString(fmt.Sprintf("Line %d\n", i+1)) // +1 -> make "Line N" appear on the Nth line and avoid off-by-one confusions + } + + repo, initialSHA, reset := tests.CreateDeclarativeRepoWithOptions(t, user2, tests.DeclarativeRepoOptions{ + Files: optional.Some([]*files_service.ChangeRepoFile{ + { + Operation: "create", + TreePath: "file1.md", + ContentReader: strings.NewReader(content.String()), + }, + { + Operation: "create", + TreePath: "file2.md", + ContentReader: strings.NewReader(content.String()), + }, + }), + }) + t.Cleanup(reset) + + return &PullRequestCommentPlacementTester{ + t: t, + user: user2, + session: session, + apiToken: token, + fileContent: content.String(), + repo: repo, + initialSHA: initialSHA, + } +} + +func (tester *PullRequestCommentPlacementTester) changeFileOnBranch(sourceBranch, targetBranch string, targetBranchIsNew bool, filename, newContent string) string { + req := NewRequest(tester.t, + "GET", + fmt.Sprintf("/api/v1/repos/%s/%s/contents/%s?ref=%s", tester.repo.OwnerName, tester.repo.Name, filename, sourceBranch)). + AddTokenAuth(tester.apiToken) + resp := MakeRequest(tester.t, req, http.StatusOK) + var existingFile api.ContentsResponse + DecodeJSON(tester.t, resp, &existingFile) + + opts := api.UpdateFileOptions{ + DeleteFileOptions: api.DeleteFileOptions{ + SHA: existingFile.SHA, + }, + ContentBase64: base64.StdEncoding.EncodeToString([]byte(newContent)), + } + if targetBranchIsNew { + opts.DeleteFileOptions.FileOptions.NewBranchName = targetBranch + } else { + opts.DeleteFileOptions.FileOptions.BranchName = targetBranch + } + req = NewRequestWithJSON(tester.t, + "PUT", + fmt.Sprintf("/api/v1/repos/%s/%s/contents/%s", tester.repo.OwnerName, tester.repo.Name, filename), + opts).AddTokenAuth(tester.apiToken) + resp = MakeRequest(tester.t, req, http.StatusOK) + var updateFileResponse api.FileResponse + DecodeJSON(tester.t, resp, &updateFileResponse) + + return updateFileResponse.Commit.SHA +} + +func (tester *PullRequestCommentPlacementTester) changeFileOnBase(filename, newContent string) string { + return tester.changeFileOnBranch(tester.repo.DefaultBranch, tester.repo.DefaultBranch, false, filename, newContent) +} + +func (tester *PullRequestCommentPlacementTester) changeFile(filename, newContent string) string { + var sourceBranch string // where to get the file's last SHA from + branchExists, branch := tester.branch.Get() + if !branchExists { + branch = fmt.Sprintf("branch-%s", uuid.New().String()) + tester.branch = optional.Some(branch) + sourceBranch = tester.repo.DefaultBranch + } else { + sourceBranch = branch + } + return tester.changeFileOnBranch(sourceBranch, branch, !branchExists, filename, newContent) +} + +func (tester *PullRequestCommentPlacementTester) createPR() { + branchExists, branch := tester.branch.Get() + require.True(tester.t, branchExists) + req := NewRequestWithJSON(tester.t, "POST", + fmt.Sprintf("/api/v1/repos/%s/%s/pulls", tester.repo.OwnerName, tester.repo.Name), + &api.CreatePullRequestOption{ + Head: branch, + Base: tester.repo.DefaultBranch, + Title: fmt.Sprintf("PR from branch %s", tester.branch), + }).AddTokenAuth(tester.apiToken) + resp := MakeRequest(tester.t, req, http.StatusCreated) + var pr api.PullRequest + DecodeJSON(tester.t, resp, &pr) + tester.pr = &pr +} + +func (tester *PullRequestCommentPlacementTester) commentFromFilesChanged(filename string, line int) *issues_model.Comment { + commentContent := uuid.New().String() + + req := NewRequest(tester.t, "GET", + fmt.Sprintf("/%s/%s/pulls/%d/files/reviews/new_comment", tester.repo.OwnerName, tester.repo.Name, tester.pr.Index)) + resp := tester.session.MakeRequest(tester.t, req, http.StatusOK) + + doc := NewHTMLParser(tester.t, resp.Body) + 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"), + "latest_commit_id": doc.GetInputValueByName("latest_commit_id"), + "side": "proposed", // "proposed" (RHS) or "previous" (LHS) + "line": strconv.Itoa(line), + "path": filename, + "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": commentContent, + "single_review": "true", + }) + tester.session.MakeRequest(tester.t, req, http.StatusOK) + + comment := unittest.AssertExistsAndLoadBean(tester.t, &issues_model.Comment{Content: commentContent}) + return comment +} + +func (tester *PullRequestCommentPlacementTester) withBranchCheckout(action func(string)) { + dstPath := tester.t.TempDir() + cloneURL, _ := url.Parse(tester.repo.CloneLink().HTTPS) + cloneURL.User = url.UserPassword(tester.user.LoginName, userPassword) + require.NoError(tester.t, git.CloneWithArgs(tester.t.Context(), nil, cloneURL.String(), dstPath, git.CloneRepoOptions{})) + doGitSetRemoteURL(dstPath, "origin", cloneURL)(tester.t) + + branchExists, branch := tester.branch.Get() + require.True(tester.t, branchExists) + require.NoError(tester.t, git.NewCommand(tester.t.Context(), "checkout").AddDynamicArguments(branch).Run(&git.RunOpts{Dir: dstPath})) + + action(dstPath) +} + +func (tester *PullRequestCommentPlacementTester) assertFilesChangedDiff(rowAssertions []diffTableRow, note ...string) { + req := NewRequest(tester.t, "GET", + fmt.Sprintf("/%s/%s/pulls/%d/files?show-outdated=true", tester.repo.OwnerName, tester.repo.Name, tester.pr.Index)) + resp := tester.session.MakeRequest(tester.t, req, http.StatusOK) + doc := NewHTMLParser(tester.t, resp.Body) + var testNote string + if len(note) == 0 { + testNote = "contents in single-commit diff" + } else { + testNote = note[0] + } + assertDiffTable(tester.t, doc, rowAssertions, testNote) +} + +func (tester *PullRequestCommentPlacementTester) assertCommitDiff(commitSHA string, rowAssertions []diffTableRow, note ...string) { + req := NewRequest(tester.t, "GET", + fmt.Sprintf("/%s/%s/pulls/%d/commits/%s?show-outdated=true", tester.repo.OwnerName, tester.repo.Name, tester.pr.Index, commitSHA)) + resp := tester.session.MakeRequest(tester.t, req, http.StatusOK) + doc := NewHTMLParser(tester.t, resp.Body) + var testNote string + if len(note) == 0 { + testNote = "contents in full PR diff" + } else { + testNote = note[0] + } + assertDiffTable(tester.t, doc, rowAssertions, testNote) +} + +func lineNumber(content, line string) int { + return slices.Index(strings.Split(content, "\n"), line) + 1 +} + +type diffTableRowType int + +const ( + RowHasCode diffTableRowType = iota + RowAddCode + RowDelCode + RowComment +) + +type diffTableRow struct { + rowType diffTableRowType + // RowHasCode, RowAddCode, RowDelCode + code string + // RowComment + commentID int64 +} + +func nodeText(node *html.Node) string { + if node.Type == html.TextNode { + return node.Data + } + var builder strings.Builder + for child := range node.ChildNodes() { + childText := strings.TrimSpace(nodeText(child)) + builder.WriteString(childText) + } + return builder.String() +} + +func nodeAttr(node *html.Node, key string) string { + for _, attr := range node.Attr { + if attr.Key == key { + return attr.Val + } + } + return "" +} + +func checkDiffTableRow(t *testing.T, tableRow *html.Node, rowAssertion diffTableRow) string { + switch rowAssertion.rowType { + case RowHasCode: + text := nodeText(tableRow) + if text != rowAssertion.code { + return fmt.Sprintf("wanted diff %q, but found diff %q", rowAssertion.code, text) + } + case RowDelCode: + dataLineType := nodeAttr(tableRow, "data-line-type") + if dataLineType != "del" { + return fmt.Sprintf("wanted delete code in diff, but found data-line-type=%q", dataLineType) + } + text := nodeText(tableRow) + if text != rowAssertion.code { + return fmt.Sprintf("wanted delete code with line %q, but found diff %q", rowAssertion.code, text) + } + case RowAddCode: + dataLineType := nodeAttr(tableRow, "data-line-type") + if dataLineType != "add" { + return fmt.Sprintf("wanted add code in diff, but found data-line-type=%q", dataLineType) + } + text := nodeText(tableRow) + if text != rowAssertion.code { + return fmt.Sprintf("wanted add code with line %q, but found diff %q", rowAssertion.code, text) + } + case RowComment: + class := nodeAttr(tableRow, "class") + if class != "add-comment" { + return fmt.Sprintf("wanted comment in diff, but found class=%q", class) + } + found := false + for desc := range tableRow.Descendants() { + descID := nodeAttr(desc, "id") + if descID == fmt.Sprintf("code-comments-%d", rowAssertion.commentID) { + found = true + break + } + } + if !found { + return fmt.Sprintf("wanted comment with ID %d, but could not be identified", rowAssertion.commentID) + } + } + return "" +} + +func assertDiffTable(t *testing.T, doc *HTMLDoc, rowAssertions []diffTableRow, note string) { + require.NotEmpty(t, rowAssertions) + + diffTable := doc.Find("table.chroma") + require.Equal(t, 1, diffTable.Length()) + + rows := diffTable.Find("tbody > tr[data-line-type]") // [data-line-type] is used to avoid matching tables within comment boxes + + // Find the first row to match rowAssertions[0], and then we'll iterate from there matching each row exactly. + tableFirstRowIndex := 0 + foundFirst := false + firstRowMismatches := []string{} + for ; tableFirstRowIndex < rows.Length(); tableFirstRowIndex++ { + mismatch := checkDiffTableRow(t, rows.Get(tableFirstRowIndex), rowAssertions[0]) + if mismatch == "" { + foundFirst = true + break + } + firstRowMismatches = append(firstRowMismatches, mismatch) + } + if !foundFirst { + // We're going to fail because we couldn't find the first row in rowAssertions -- this can be tricky to debug so + // help out by outputting all the rows we looked at that didn't match: + t.Log("first row mismatches:") + for _, mm := range firstRowMismatches { + t.Logf("\t%s", mm) + } + require.Failf(t, "unable to find first row", "test %s: failed to find first row assertion", note) + } + + for idx, assertion := range rowAssertions { + if idx == 0 { // skip first row assertion, already checked to find tableFirstRowIndex + continue + } + + tableIdx := tableFirstRowIndex + idx + if tableIdx >= rows.Length() { + require.Failf(t, "ran out of table rows", "test %s: row assertion at index %d couldn't be satisfied", note, idx) + } + + tableRow := rows.Get(tableIdx) + check := checkDiffTableRow(t, tableRow, assertion) + if check != "" { + assert.Failf(t, check, "test %s: row assertion at index %d couldn't be satisfied", note, idx) + } + } +}