From 345ae04837eb6bf01c33348ec5a80139484e9ff9 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 13 Jul 2025 16:07:46 -0700 Subject: [PATCH 01/35] Allow code review comments display cross commits even if the head branch has a force-push --- models/issues/comment.go | 4 + models/issues/comment_code.go | 51 ++- models/issues/comment_test.go | 6 +- models/issues/review.go | 2 +- models/issues/review_test.go | 2 +- modules/git/diff.go | 57 ++++ modules/git/diff_test.go | 6 + routers/api/v1/repo/pull_review.go | 13 +- routers/web/repo/issue_view.go | 33 +- routers/web/repo/pull.go | 142 ++++----- routers/web/repo/pull_review.go | 56 ++-- routers/web/repo/pull_review_test.go | 14 +- services/convert/pull_review.go | 46 ++- services/feed/notifier.go | 28 +- services/forms/repo_form.go | 3 +- services/gitdiff/gitdiff.go | 28 -- services/gitdiff/gitdiff_test.go | 42 --- services/mailer/incoming/incoming_handler.go | 3 +- services/mailer/mail_issue_common.go | 7 +- services/pull/review.go | 297 +++++++++++++----- services/pull/review_test.go | 89 ++++++ templates/repo/diff/box.tmpl | 2 +- templates/repo/diff/comment_form.tmpl | 3 +- .../repo/issue/view_content/comments.tmpl | 6 +- 24 files changed, 559 insertions(+), 381 deletions(-) diff --git a/models/issues/comment.go b/models/issues/comment.go index 4fdb0c1808fb4..082ede1e199f7 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -1007,6 +1007,7 @@ type FindCommentsOptions struct { RepoID int64 IssueID int64 ReviewID int64 + CommitSHA string Since int64 Before int64 Line int64 @@ -1052,6 +1053,9 @@ func (opts FindCommentsOptions) ToConds() builder.Cond { if opts.IsPull.Has() { cond = cond.And(builder.Eq{"issue.is_pull": opts.IsPull.Value()}) } + if opts.CommitSHA != "" { + cond = cond.And(builder.Eq{"comment.commit_sha": opts.CommitSHA}) + } return cond } diff --git a/models/issues/comment_code.go b/models/issues/comment_code.go index 55e67a1243b70..2cd2614ff0243 100644 --- a/models/issues/comment_code.go +++ b/models/issues/comment_code.go @@ -9,6 +9,7 @@ import ( "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/renderhelper" + repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/markup/markdown" @@ -16,39 +17,44 @@ import ( ) // CodeComments represents comments on code by using this structure: FILENAME -> LINE (+ == proposed; - == previous) -> COMMENTS -type CodeComments map[string]map[int64][]*Comment +type CodeComments map[string][]*Comment + +func (cc CodeComments) AllComments() []*Comment { + var allComments []*Comment + for _, comments := range cc { + allComments = append(allComments, comments...) + } + return allComments +} // FetchCodeComments will return a 2d-map: ["Path"]["Line"] = Comments at line -func FetchCodeComments(ctx context.Context, issue *Issue, currentUser *user_model.User, showOutdatedComments bool) (CodeComments, error) { - return fetchCodeCommentsByReview(ctx, issue, currentUser, nil, showOutdatedComments) +func FetchCodeComments(ctx context.Context, repo *repo_model.Repository, issueID int64, currentUser *user_model.User, showOutdatedComments bool) (CodeComments, error) { + return fetchCodeCommentsByReview(ctx, repo, issueID, currentUser, nil, showOutdatedComments) } -func fetchCodeCommentsByReview(ctx context.Context, issue *Issue, currentUser *user_model.User, review *Review, showOutdatedComments bool) (CodeComments, error) { - pathToLineToComment := make(CodeComments) +func fetchCodeCommentsByReview(ctx context.Context, repo *repo_model.Repository, issueID int64, currentUser *user_model.User, review *Review, showOutdatedComments bool) (CodeComments, error) { + codeCommentsPathMap := make(CodeComments) if review == nil { review = &Review{ID: 0} } opts := FindCommentsOptions{ Type: CommentTypeCode, - IssueID: issue.ID, + IssueID: issueID, ReviewID: review.ID, } - comments, err := findCodeComments(ctx, opts, issue, currentUser, review, showOutdatedComments) + comments, err := FindCodeComments(ctx, opts, repo, currentUser, review, showOutdatedComments) if err != nil { return nil, err } for _, comment := range comments { - if pathToLineToComment[comment.TreePath] == nil { - pathToLineToComment[comment.TreePath] = make(map[int64][]*Comment) - } - pathToLineToComment[comment.TreePath][comment.Line] = append(pathToLineToComment[comment.TreePath][comment.Line], comment) + codeCommentsPathMap[comment.TreePath] = append(codeCommentsPathMap[comment.TreePath], comment) } - return pathToLineToComment, nil + return codeCommentsPathMap, nil } -func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issue, currentUser *user_model.User, review *Review, showOutdatedComments bool) ([]*Comment, error) { +func FindCodeComments(ctx context.Context, opts FindCommentsOptions, repo *repo_model.Repository, currentUser *user_model.User, review *Review, showOutdatedComments bool) ([]*Comment, error) { var comments CommentList if review == nil { review = &Review{ID: 0} @@ -67,10 +73,6 @@ func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issu return nil, err } - if err := issue.LoadRepo(ctx); err != nil { - return nil, err - } - if err := comments.LoadPosters(ctx); err != nil { return nil, err } @@ -110,12 +112,12 @@ func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issu return nil, err } - if err := comment.LoadReactions(ctx, issue.Repo); err != nil { + if err := comment.LoadReactions(ctx, repo); err != nil { return nil, err } var err error - rctx := renderhelper.NewRenderContextRepoComment(ctx, issue.Repo, renderhelper.RepoCommentOptions{ + rctx := renderhelper.NewRenderContextRepoComment(ctx, repo, renderhelper.RepoCommentOptions{ FootnoteContextID: strconv.FormatInt(comment.ID, 10), }) if comment.RenderedContent, err = markdown.RenderString(rctx, comment.Content); err != nil { @@ -124,14 +126,3 @@ func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issu } return comments[:n], nil } - -// FetchCodeCommentsByLine fetches the code comments for a given treePath and line number -func FetchCodeCommentsByLine(ctx context.Context, issue *Issue, currentUser *user_model.User, treePath string, line int64, showOutdatedComments bool) (CommentList, error) { - opts := FindCommentsOptions{ - Type: CommentTypeCode, - IssueID: issue.ID, - TreePath: treePath, - Line: line, - } - return findCodeComments(ctx, opts, issue, currentUser, nil, showOutdatedComments) -} diff --git a/models/issues/comment_test.go b/models/issues/comment_test.go index c08e3b970d3b2..627cc188a6625 100644 --- a/models/issues/comment_test.go +++ b/models/issues/comment_test.go @@ -68,15 +68,15 @@ func TestFetchCodeComments(t *testing.T) { issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2}) user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) - res, err := issues_model.FetchCodeComments(db.DefaultContext, issue, user, false) + res, err := issues_model.FetchCodeComments(db.DefaultContext, issue.Repo, issue.ID, user, false) assert.NoError(t, err) assert.Contains(t, res, "README.md") assert.Contains(t, res["README.md"], int64(4)) assert.Len(t, res["README.md"][4], 1) - assert.Equal(t, int64(4), res["README.md"][4][0].ID) + assert.Equal(t, int64(4), res["README.md"][0].ID) user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) - res, err = issues_model.FetchCodeComments(db.DefaultContext, issue, user2, false) + res, err = issues_model.FetchCodeComments(db.DefaultContext, issue.Repo, issue.ID, user2, false) assert.NoError(t, err) assert.Len(t, res, 1) } diff --git a/models/issues/review.go b/models/issues/review.go index 71fdb7456f185..2102da28453bd 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -159,7 +159,7 @@ func (r *Review) LoadCodeComments(ctx context.Context) (err error) { if err = r.LoadIssue(ctx); err != nil { return err } - r.CodeComments, err = fetchCodeCommentsByReview(ctx, r.Issue, nil, r, false) + r.CodeComments, err = fetchCodeCommentsByReview(ctx, r.Issue.Repo, r.Issue.ID, nil, r, false) return err } diff --git a/models/issues/review_test.go b/models/issues/review_test.go index 2588b8ba41b05..182efb76dceba 100644 --- a/models/issues/review_test.go +++ b/models/issues/review_test.go @@ -48,7 +48,7 @@ func TestReview_LoadCodeComments(t *testing.T) { assert.NoError(t, review.LoadAttributes(db.DefaultContext)) assert.NoError(t, review.LoadCodeComments(db.DefaultContext)) assert.Len(t, review.CodeComments, 1) - assert.Equal(t, int64(4), review.CodeComments["README.md"][int64(4)][0].Line) + assert.Equal(t, int64(4), review.CodeComments["README.md"][0].Line) } func TestReviewType_Icon(t *testing.T) { diff --git a/modules/git/diff.go b/modules/git/diff.go index 35d115be0e5da..7f3d57ddbc834 100644 --- a/modules/git/diff.go +++ b/modules/git/diff.go @@ -15,6 +15,7 @@ import ( "strings" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/util" ) // RawDiffType type of a raw diff. @@ -107,12 +108,16 @@ func ParseDiffHunkString(diffHunk string) (leftLine, leftHunk, rightLine, rightH leftLine, _ = strconv.Atoi(leftRange[0][1:]) if len(leftRange) > 1 { leftHunk, _ = strconv.Atoi(leftRange[1]) + } else { + leftHunk = util.Iif(leftLine > 0, leftLine, -leftLine) } if len(ranges) > 1 { rightRange := strings.Split(ranges[1], ",") rightLine, _ = strconv.Atoi(rightRange[0]) if len(rightRange) > 1 { rightHunk, _ = strconv.Atoi(rightRange[1]) + } else { + rightHunk = rightLine } } else { log.Debug("Parse line number failed: %v", diffHunk) @@ -342,3 +347,55 @@ func GetAffectedFiles(repo *Repository, branchName, oldCommitID, newCommitID str return affectedFiles, err } + +type HunkInfo struct { + LeftLine int64 // Line number in the old file + LeftHunk int64 // Number of lines in the old file + RightLine int64 // Line number in the new file + RightHunk int64 // Number of lines in the new file +} + +// GetAffectedHunksForTwoCommitsSpecialFile returns the affected hunks between two commits for a special file +// git diff --unified=0 abc123 def456 -- src/main.go +func GetAffectedHunksForTwoCommitsSpecialFile(ctx context.Context, repoPath, oldCommitID, newCommitID, filePath string) ([]*HunkInfo, error) { + reader, writer := io.Pipe() + defer func() { + _ = reader.Close() + _ = writer.Close() + }() + go func() { + if err := NewCommand("diff", "--unified=0", "--no-color"). + AddDynamicArguments(oldCommitID, newCommitID). + AddDashesAndList(filePath). + Run(ctx, &RunOpts{ + Dir: repoPath, + Stdout: writer, + }); err != nil { + _ = writer.CloseWithError(fmt.Errorf("GetAffectedHunksForTwoCommitsSpecialFile[%s, %s, %s, %s]: %w", repoPath, oldCommitID, newCommitID, filePath, err)) + return + } + _ = writer.Close() + }() + + scanner := bufio.NewScanner(reader) + hunks := make([]*HunkInfo, 0, 32) + for scanner.Scan() { + lof := scanner.Text() + if !strings.HasPrefix(lof, "@@") { + continue + } + // Parse the hunk header + leftLine, leftHunk, rightLine, rightHunk := ParseDiffHunkString(lof) + hunks = append([]*HunkInfo{}, &HunkInfo{ + LeftLine: int64(leftLine), + LeftHunk: int64(leftHunk), + RightLine: int64(rightLine), + RightHunk: int64(rightHunk), + }) + } + if scanner.Err() != nil { + return nil, fmt.Errorf("GetAffectedHunksForTwoCommitsSpecialFile[%s, %s, %s, %s]: %w", repoPath, oldCommitID, newCommitID, filePath, scanner.Err()) + } + + return hunks, nil +} diff --git a/modules/git/diff_test.go b/modules/git/diff_test.go index 7671fffcc1683..11bdcd35fb1e8 100644 --- a/modules/git/diff_test.go +++ b/modules/git/diff_test.go @@ -181,4 +181,10 @@ func TestParseDiffHunkString(t *testing.T) { assert.Equal(t, 3, leftHunk) assert.Equal(t, 19, rightLine) assert.Equal(t, 5, rightHunk) + + leftLine, leftHunk, rightLine, rightHunk = ParseDiffHunkString("@@ -1 +0,0 @@") + assert.Equal(t, 1, leftLine) + assert.Equal(t, 1, leftHunk) + assert.Equal(t, 0, rightLine) + assert.Equal(t, 0, rightHunk) } diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index 3c00193fac1d9..082326333b7a7 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -13,7 +13,6 @@ import ( "code.gitea.io/gitea/models/organization" access_model "code.gitea.io/gitea/models/perm/access" user_model "code.gitea.io/gitea/models/user" - "code.gitea.io/gitea/modules/gitrepo" api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/web" "code.gitea.io/gitea/routers/api/v1/utils" @@ -329,14 +328,7 @@ func CreatePullReview(ctx *context.APIContext) { // if CommitID is empty, set it as lastCommitID if opts.CommitID == "" { - gitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, pr.Issue.Repo) - if err != nil { - ctx.APIErrorInternal(err) - return - } - defer closer.Close() - - headCommitID, err := gitRepo.GetRefCommitID(pr.GetGitHeadRefName()) + headCommitID, err := ctx.Repo.GitRepo.GetRefCommitID(pr.GetGitHeadRefName()) if err != nil { ctx.APIErrorInternal(err) return @@ -357,11 +349,12 @@ func CreatePullReview(ctx *context.APIContext) { ctx.Repo.GitRepo, pr.Issue, line, + pr.MergeBase, + opts.CommitID, c.Body, c.Path, true, // pending review 0, // no reply - opts.CommitID, nil, ); err != nil { ctx.APIErrorInternal(err) diff --git a/routers/web/repo/issue_view.go b/routers/web/repo/issue_view.go index d0064e763ef82..30e0043ee1ce4 100644 --- a/routers/web/repo/issue_view.go +++ b/routers/web/repo/issue_view.go @@ -730,28 +730,23 @@ func prepareIssueViewCommentsAndSidebarParticipants(ctx *context.Context, issue } comment.Review.Reviewer = user_model.NewGhostUser() } - if err = comment.Review.LoadCodeComments(ctx); err != nil { - ctx.ServerError("Review.LoadCodeComments", err) - return - } + for _, codeComments := range comment.Review.CodeComments { - for _, lineComments := range codeComments { - for _, c := range lineComments { - // Check tag. - role, ok = marked[c.PosterID] - if ok { - c.ShowRole = role - continue - } + for _, c := range codeComments { + // Check tag. + role, ok = marked[c.PosterID] + if ok { + c.ShowRole = role + continue + } - c.ShowRole, err = roleDescriptor(ctx, issue.Repo, c.Poster, permCache, issue, c.HasOriginalAuthor()) - if err != nil { - ctx.ServerError("roleDescriptor", err) - return - } - marked[c.PosterID] = c.ShowRole - participants = addParticipant(c.Poster, participants) + c.ShowRole, err = roleDescriptor(ctx, issue.Repo, c.Poster, permCache, issue, c.HasOriginalAuthor()) + if err != nil { + ctx.ServerError("roleDescriptor", err) + return } + marked[c.PosterID] = c.ShowRole + participants = addParticipant(c.Poster, participants) } } if err = comment.LoadResolveDoer(ctx); err != nil { diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index c5302dd50f58d..64d2ad330c9ac 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -643,8 +643,17 @@ func ViewPullCommits(ctx *context.Context) { ctx.HTML(http.StatusOK, tplPullCommits) } +func indexCommit(commits []*git.Commit, commitID string) *git.Commit { + for i := range commits { + if commits[i].ID.String() == commitID { + return commits[i] + } + } + return nil +} + // ViewPullFiles render pull request changed files list page -func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommit string, willShowSpecifiedCommitRange, willShowSpecifiedCommit bool) { +func viewPullFiles(ctx *context.Context, beforeCommitID, afterCommitID string) { ctx.Data["PageIsPullList"] = true ctx.Data["PageIsPullFiles"] = true @@ -653,12 +662,7 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi return } pull := issue.PullRequest - - var ( - startCommitID string - endCommitID string - gitRepo = ctx.Repo.GitRepo - ) + gitRepo := ctx.Repo.GitRepo prInfo := preparePullViewPullInfo(ctx, issue) if ctx.Written() { @@ -668,77 +672,65 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi return } - // Validate the given commit sha to show (if any passed) - if willShowSpecifiedCommit || willShowSpecifiedCommitRange { - foundStartCommit := len(specifiedStartCommit) == 0 - foundEndCommit := len(specifiedEndCommit) == 0 - - if !(foundStartCommit && foundEndCommit) { - for _, commit := range prInfo.Commits { - if commit.ID.String() == specifiedStartCommit { - foundStartCommit = true - } - if commit.ID.String() == specifiedEndCommit { - foundEndCommit = true - } - - if foundStartCommit && foundEndCommit { - break - } - } - } - - if !(foundStartCommit && foundEndCommit) { - ctx.NotFound(nil) - return - } - } - - if ctx.Written() { - return - } - headCommitID, err := gitRepo.GetRefCommitID(pull.GetGitHeadRefName()) if err != nil { ctx.ServerError("GetRefCommitID", err) return } - ctx.Data["IsShowingOnlySingleCommit"] = willShowSpecifiedCommit + ctx.Data["IsShowingOnlySingleCommit"] = beforeCommitID != "" && beforeCommitID == afterCommitID + isShowAllCommits := (beforeCommitID == "" || beforeCommitID == prInfo.MergeBase) && (afterCommitID == "" || afterCommitID == headCommitID) + ctx.Data["IsShowingAllCommits"] = isShowAllCommits - if willShowSpecifiedCommit || willShowSpecifiedCommitRange { - if len(specifiedEndCommit) > 0 { - endCommitID = specifiedEndCommit - } else { - endCommitID = headCommitID + if beforeCommitID == "" { + beforeCommitID = prInfo.MergeBase + } + if afterCommitID == "" { + afterCommitID = headCommitID + } + + var beforeCommit, afterCommit *git.Commit + if beforeCommitID != prInfo.MergeBase { + beforeCommit = indexCommit(prInfo.Commits, beforeCommitID) + if beforeCommit == nil { + ctx.NotFound(errors.New("before commit not found in PR commits")) + return } - if len(specifiedStartCommit) > 0 { - startCommitID = specifiedStartCommit - } else { - startCommitID = prInfo.MergeBase + beforeCommit, err = beforeCommit.Parent(0) + if err != nil { + ctx.ServerError("GetParentCommit", err) + return } - ctx.Data["IsShowingAllCommits"] = false - } else { - endCommitID = headCommitID - startCommitID = prInfo.MergeBase - ctx.Data["IsShowingAllCommits"] = true + beforeCommitID = beforeCommit.ID.String() + } else { // mergebase commit is not in the list of the pull request commits + beforeCommit, err = gitRepo.GetCommit(beforeCommitID) + if err != nil { + ctx.ServerError("GetCommit", err) + return + } + } + + afterCommit = indexCommit(prInfo.Commits, afterCommitID) + if afterCommit == nil { + ctx.NotFound(errors.New("after commit not found in PR commits")) + return } ctx.Data["Username"] = ctx.Repo.Owner.Name ctx.Data["Reponame"] = ctx.Repo.Repository.Name - ctx.Data["AfterCommitID"] = endCommitID - ctx.Data["BeforeCommitID"] = startCommitID - - fileOnly := ctx.FormBool("file-only") + ctx.Data["AfterCommitID"] = afterCommitID + ctx.Data["BeforeCommitID"] = beforeCommitID maxLines, maxFiles := setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffFiles files := ctx.FormStrings("files") + fileOnly := ctx.FormBool("file-only") if fileOnly && (len(files) == 2 || len(files) == 1) { maxLines, maxFiles = -1, -1 } diffOptions := &gitdiff.DiffOptions{ - AfterCommitID: endCommitID, + BeforeCommitID: beforeCommitID, + AfterCommitID: afterCommitID, SkipTo: ctx.FormString("skip-to"), MaxLines: maxLines, MaxLineCharacters: setting.Git.MaxGitDiffLineCharacters, @@ -746,10 +738,6 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi WhitespaceBehavior: gitdiff.GetWhitespaceFlag(ctx.Data["WhitespaceBehavior"].(string)), } - if !willShowSpecifiedCommit { - diffOptions.BeforeCommitID = startCommitID - } - diff, err := gitdiff.GetDiffForRender(ctx, ctx.Repo.RepoLink, gitRepo, diffOptions, files...) if err != nil { ctx.ServerError("GetDiff", err) @@ -761,7 +749,7 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi // as the viewed information is designed to be loaded only on latest PR // diff and if you're signed in. var reviewState *pull_model.ReviewState - if ctx.IsSigned && !willShowSpecifiedCommit && !willShowSpecifiedCommitRange { + if ctx.IsSigned && isShowAllCommits { reviewState, err = gitdiff.SyncUserSpecificDiff(ctx, ctx.Doer.ID, pull, gitRepo, diff, diffOptions) if err != nil { ctx.ServerError("SyncUserSpecificDiff", err) @@ -769,7 +757,7 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi } } - diffShortStat, err := gitdiff.GetDiffShortStat(ctx.Repo.GitRepo, startCommitID, endCommitID) + diffShortStat, err := gitdiff.GetDiffShortStat(ctx.Repo.GitRepo, beforeCommitID, afterCommitID) if err != nil { ctx.ServerError("GetDiffShortStat", err) return @@ -781,7 +769,8 @@ 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 = pull_service.LoadCodeComments(ctx, ctx.Repo.GitRepo, ctx.Repo.Repository, + diff, issue.ID, ctx.Doer, beforeCommit, afterCommit, ctx.Data["ShowOutdatedComments"].(bool)); err != nil { ctx.ServerError("LoadComments", err) return } @@ -816,7 +805,7 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi if !fileOnly { // note: use mergeBase is set to false because we already have the merge base from the pull request info - diffTree, err := gitdiff.GetDiffTree(ctx, gitRepo, false, startCommitID, endCommitID) + diffTree, err := gitdiff.GetDiffTree(ctx, gitRepo, false, beforeCommitID, afterCommitID) if err != nil { ctx.ServerError("GetDiffTree", err) return @@ -836,17 +825,6 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi ctx.Data["Diff"] = diff ctx.Data["DiffNotAvailable"] = diffShortStat.NumFiles == 0 - baseCommit, err := ctx.Repo.GitRepo.GetCommit(startCommitID) - if err != nil { - ctx.ServerError("GetCommit", err) - return - } - commit, err := gitRepo.GetCommit(endCommitID) - if err != nil { - ctx.ServerError("GetCommit", err) - return - } - if ctx.IsSigned && ctx.Doer != nil { if ctx.Data["CanMarkConversation"], err = issues_model.CanMarkConversation(ctx, issue, ctx.Doer); err != nil { ctx.ServerError("CanMarkConversation", err) @@ -854,7 +832,7 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi } } - setCompareContext(ctx, baseCommit, commit, ctx.Repo.Owner.Name, ctx.Repo.Repository.Name) + setCompareContext(ctx, beforeCommit, afterCommit, ctx.Repo.Owner.Name, ctx.Repo.Repository.Name) assigneeUsers, err := repo_model.GetRepoAssignees(ctx, ctx.Repo.Repository) if err != nil { @@ -901,7 +879,7 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi ctx.Data["CanBlockUser"] = func(blocker, blockee *user_model.User) bool { return user_service.CanBlockUser(ctx, ctx.Doer, blocker, blockee) } - if !willShowSpecifiedCommit && !willShowSpecifiedCommitRange && pull.Flow == issues_model.PullRequestFlowGithub { + if !isShowAllCommits && pull.Flow == issues_model.PullRequestFlowGithub { if err := pull.LoadHeadRepo(ctx); err != nil { ctx.ServerError("LoadHeadRepo", err) return @@ -930,19 +908,19 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi } func ViewPullFilesForSingleCommit(ctx *context.Context) { - viewPullFiles(ctx, "", ctx.PathParam("sha"), true, true) + viewPullFiles(ctx, ctx.PathParam("sha"), ctx.PathParam("sha")) } func ViewPullFilesForRange(ctx *context.Context) { - viewPullFiles(ctx, ctx.PathParam("shaFrom"), ctx.PathParam("shaTo"), true, false) + viewPullFiles(ctx, ctx.PathParam("shaFrom"), ctx.PathParam("shaTo")) } func ViewPullFilesStartingFromCommit(ctx *context.Context) { - viewPullFiles(ctx, "", ctx.PathParam("sha"), true, false) + viewPullFiles(ctx, ctx.PathParam("sha"), "") } func ViewPullFilesForAllCommitsOfPr(ctx *context.Context) { - viewPullFiles(ctx, "", "", false, false) + viewPullFiles(ctx, "", "") } // UpdatePullRequest merge PR's baseBranch into headBranch diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index 18e14e9b224c4..a8fdd613a7b3c 100644 --- a/routers/web/repo/pull_review.go +++ b/routers/web/repo/pull_review.go @@ -16,6 +16,7 @@ import ( "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/templates" + "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/modules/web" "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/services/context/upload" @@ -49,12 +50,8 @@ func RenderNewCodeCommentForm(ctx *context.Context) { ctx.Data["PageIsPullFiles"] = true ctx.Data["Issue"] = issue ctx.Data["CurrentReview"] = currentReview - pullHeadCommitID, err := ctx.Repo.GitRepo.GetRefCommitID(issue.PullRequest.GetGitHeadRefName()) - if err != nil { - ctx.ServerError("GetRefCommitID", err) - return - } - ctx.Data["AfterCommitID"] = pullHeadCommitID + ctx.Data["BeforeCommitID"] = ctx.FormString("before_commit_id") + ctx.Data["AfterCommitID"] = ctx.FormString("after_commit_id") ctx.Data["IsAttachmentEnabled"] = setting.Attachment.Enabled upload.AddUploadContext(ctx, "comment") ctx.HTML(http.StatusOK, tplNewComment) @@ -77,10 +74,7 @@ func CreateCodeComment(ctx *context.Context) { return } - signedLine := form.Line - if form.Side == "previous" { - signedLine *= -1 - } + signedLine := util.Iif(form.Side == "previous", -form.Line, form.Line) var attachments []string if setting.Attachment.Enabled { @@ -92,11 +86,12 @@ func CreateCodeComment(ctx *context.Context) { ctx.Repo.GitRepo, issue, signedLine, + form.BeforeCommitID, + form.AfterCommitID, form.Content, form.TreePath, !form.SingleReview, form.Reply, - form.LatestCommitID, attachments, ) if err != nil { @@ -112,7 +107,7 @@ func CreateCodeComment(ctx *context.Context) { log.Trace("Comment created: %-v #%d[%d] Comment[%d]", ctx.Repo.Repository, issue.Index, issue.ID, comment.ID) - renderConversation(ctx, comment, form.Origin) + renderConversation(ctx, comment, form.Origin, form.BeforeCommitID, form.AfterCommitID) } // UpdateResolveConversation add or remove an Conversation resolved mark @@ -163,14 +158,33 @@ func UpdateResolveConversation(ctx *context.Context) { return } - renderConversation(ctx, comment, origin) + beforeCommitID, afterCommitID := ctx.FormString("before_commit_id"), ctx.FormString("after_commit_id") + + renderConversation(ctx, comment, origin, beforeCommitID, afterCommitID) } -func renderConversation(ctx *context.Context, comment *issues_model.Comment, origin string) { +func renderConversation(ctx *context.Context, comment *issues_model.Comment, origin, beforeCommitID, afterCommitID string) { ctx.Data["PageIsPullFiles"] = origin == "diff" + if err := comment.Issue.LoadPullRequest(ctx); err != nil { + ctx.ServerError("comment.Issue.LoadPullRequest", err) + return + } + if beforeCommitID == "" { + beforeCommitID = comment.Issue.PullRequest.MergeBase + } + if afterCommitID == "" { + var err error + afterCommitID, err = ctx.Repo.GitRepo.GetRefCommitID(comment.Issue.PullRequest.GetGitHeadRefName()) + if err != nil { + ctx.ServerError("GetRefCommitID", err) + return + } + } + showOutdatedComments := origin == "timeline" || ctx.Data["ShowOutdatedComments"].(bool) - comments, err := issues_model.FetchCodeCommentsByLine(ctx, comment.Issue, ctx.Doer, comment.TreePath, comment.Line, showOutdatedComments) + comments, err := pull_service.FetchCodeCommentsByLine(ctx, ctx.Repo.Repository, comment.IssueID, + ctx.Doer, beforeCommitID, afterCommitID, comment.TreePath, comment.Line, showOutdatedComments) if err != nil { ctx.ServerError("FetchCodeCommentsByLine", err) return @@ -195,16 +209,8 @@ func renderConversation(ctx *context.Context, comment *issues_model.Comment, ori return } ctx.Data["Issue"] = comment.Issue - if err = comment.Issue.LoadPullRequest(ctx); err != nil { - ctx.ServerError("comment.Issue.LoadPullRequest", err) - return - } - pullHeadCommitID, err := ctx.Repo.GitRepo.GetRefCommitID(comment.Issue.PullRequest.GetGitHeadRefName()) - if err != nil { - ctx.ServerError("GetRefCommitID", err) - return - } - ctx.Data["AfterCommitID"] = pullHeadCommitID + ctx.Data["BeforeCommitID"] = beforeCommitID + ctx.Data["AfterCommitID"] = afterCommitID ctx.Data["CanBlockUser"] = func(blocker, blockee *user_model.User) bool { return user_service.CanBlockUser(ctx, ctx.Doer, blocker, blockee) } diff --git a/routers/web/repo/pull_review_test.go b/routers/web/repo/pull_review_test.go index 3d0997ab4d844..7ebf93c780701 100644 --- a/routers/web/repo/pull_review_test.go +++ b/routers/web/repo/pull_review_test.go @@ -41,7 +41,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, pr.HeadCommitID, nil) + comment, err := pull.CreateCodeComment(ctx, pr.Issue.Poster, ctx.Repo.GitRepo, pr.Issue, 1, "", "", "content", "", false, 0, nil) require.NoError(t, err) comment.Invalidated = true @@ -54,29 +54,29 @@ func TestRenderConversation(t *testing.T) { run("diff with outdated", func(t *testing.T, ctx *context.Context, resp *httptest.ResponseRecorder) { ctx.Data["ShowOutdatedComments"] = true - renderConversation(ctx, preparedComment, "diff") + renderConversation(ctx, preparedComment, "diff", "", "") assert.Contains(t, resp.Body.String(), `
0 { - if reviewID != 0 { - first, err := issues_model.FindComments(ctx, &issues_model.FindCommentsOptions{ - ReviewID: reviewID, - Line: line, - TreePath: treePath, - Type: issues_model.CommentTypeCode, - ListOptions: db.ListOptions{ - PageSize: 1, - Page: 1, - }, - }) - if err == nil && len(first) > 0 { - commitID = first[0].CommitSHA - invalidated = first[0].Invalidated - patch = first[0].Patch - } else if err != nil && !issues_model.IsErrCommentNotExist(err) { - return nil, fmt.Errorf("Find first comment for %d line %d path %s. Error: %w", reviewID, line, treePath, err) - } else { - review, err := issues_model.GetReviewByID(ctx, reviewID) - if err == nil && len(review.CommitID) > 0 { - head = review.CommitID - } else if err != nil && !issues_model.IsErrReviewNotExist(err) { - return nil, fmt.Errorf("GetReviewByID %d. Error: %w", reviewID, err) - } - } - } - - if len(commitID) == 0 { - // FIXME validate treePath - // Get latest commit referencing the commented line - // No need for get commit for base branch changes - commit, err := gitRepo.LineBlame(head, gitRepo.Path, treePath, uint(line)) - if err == nil { - commitID = commit.ID.String() - } else if !(strings.Contains(err.Error(), "exit status 128 - fatal: no such path") || notEnoughLines.MatchString(err.Error())) { - return nil, fmt.Errorf("LineBlame[%s, %s, %s, %d]: %w", pr.GetGitHeadRefName(), gitRepo.Path, treePath, line, err) - } - } - } - // Only fetch diff if comment is review comment - if len(patch) == 0 && reviewID != 0 { - headCommitID, err := gitRepo.GetRefCommitID(pr.GetGitHeadRefName()) - if err != nil { - return nil, fmt.Errorf("GetRefCommitID[%s]: %w", pr.GetGitHeadRefName(), err) - } - if len(commitID) == 0 { - commitID = headCommitID - } + patch, err := cache.GetString(patchCacheKey(issue.ID, beforeCommitID, afterCommitID, treePath, line), func() (string, error) { reader, writer := io.Pipe() defer func() { _ = reader.Close() _ = 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, beforeCommitID, afterCommitID, git.RawDiffNormal, treePath, writer); err != nil { + _ = writer.CloseWithError(fmt.Errorf("GetRawDiffForLine[%s, %s, %s, %s]: %w", gitRepo.Path, beforeCommitID, afterCommitID, treePath, err)) return } _ = writer.Close() }() - patch, err = git.CutDiffAroundLine(reader, int64((&issues_model.Comment{Line: line}).UnsignedLine()), line < 0, setting.UI.CodeCommentLines) - if err != nil { - log.Error("Error whilst generating patch: %v", err) - return nil, err - } + return git.CutDiffAroundLine(reader, int64((&issues_model.Comment{Line: line}).UnsignedLine()), line < 0, setting.UI.CodeCommentLines) + }) + if err != nil { + return nil, fmt.Errorf("GetPatch failed: %w", err) } + + lineCommitID := util.Iif(line < 0, beforeCommitID, afterCommitID) + // TODO: the commit ID Must be referenced in the git repository, because the branch maybe rebased or force-pushed. + + // If the commit ID is not referenced, it cannot be calculated the position dynamically. return issues_model.CreateComment(ctx, &issues_model.CreateCommentOptions{ Type: issues_model.CommentTypeCode, Doer: doer, @@ -278,10 +277,10 @@ func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_mo Content: content, LineNum: line, TreePath: treePath, - CommitSHA: commitID, + CommitSHA: lineCommitID, ReviewID: reviewID, Patch: patch, - Invalidated: invalidated, + Invalidated: false, Attachments: attachments, }) } @@ -328,15 +327,13 @@ func SubmitReview(ctx context.Context, doer *user_model.User, gitRepo *git.Repos notify_service.PullRequestReview(ctx, pr, review, comm, mentions) - for _, lines := range review.CodeComments { - for _, comments := range lines { - for _, codeComment := range comments { - mentions, err := issues_model.FindAndUpdateIssueMentions(ctx, issue, doer, codeComment.Content) - if err != nil { - return nil, nil, err - } - notify_service.PullRequestCodeComment(ctx, pr, codeComment, mentions) + for _, fileComments := range review.CodeComments { + for _, codeComment := range fileComments { + mentions, err := issues_model.FindAndUpdateIssueMentions(ctx, issue, doer, codeComment.Content) + if err != nil { + return nil, nil, err } + notify_service.PullRequestCodeComment(ctx, pr, codeComment, mentions) } } @@ -471,3 +468,143 @@ func DismissReview(ctx context.Context, reviewID, repoID int64, message string, return comment, nil } + +// ReCalculateLineNumber recalculates the line number based on the hunks of the diff. +// If the returned line number is zero, it should not be displayed. +func ReCalculateLineNumber(hunks []*git.HunkInfo, leftLine int64) int64 { + if len(hunks) == 0 || leftLine == 0 { + return leftLine + } + + isLeft := leftLine < 0 + absLine := leftLine + if isLeft { + absLine = -leftLine + } + newLine := absLine + + for _, hunk := range hunks { + if hunk.LeftLine+hunk.LeftHunk <= absLine { + newLine += hunk.RightHunk - hunk.LeftHunk + } else if hunk.LeftLine <= absLine && absLine < hunk.LeftLine+hunk.LeftHunk { + // The line has been removed, so it should not be displayed + return 0 + } else if absLine < hunk.LeftLine { + // The line is before the hunk, so we can ignore it + continue + } + } + return util.Iif(isLeft, -newLine, newLine) +} + +// FetchCodeCommentsByLine fetches the code comments for a given commit, treePath and line number of a pull request. +func FetchCodeCommentsByLine(ctx context.Context, repo *repo_model.Repository, issueID int64, currentUser *user_model.User, startCommitID, endCommitID, treePath string, line int64, showOutdatedComments bool) (issues_model.CommentList, error) { + opts := issues_model.FindCommentsOptions{ + Type: issues_model.CommentTypeCode, + IssueID: issueID, + TreePath: treePath, + } + // load all the comments on this file and then filter them by line number + // we cannot use the line number in the options because some comments's line number may have changed + comments, err := issues_model.FindCodeComments(ctx, opts, repo, currentUser, nil, showOutdatedComments) + if err != nil { + return nil, fmt.Errorf("FindCodeComments: %w", err) + } + if len(comments) == 0 { + return nil, nil + } + n := 0 + hunksCache := make(map[string][]*git.HunkInfo) + for _, comment := range comments { + if comment.CommitSHA == endCommitID { + if comment.Line == line { + comments[n] = comment + n++ + } + continue + } + + // If the comment is not for the current commit, we need to recalculate the line number + hunks, ok := hunksCache[comment.CommitSHA] + if !ok { + hunks, err = git.GetAffectedHunksForTwoCommitsSpecialFile(ctx, repo.RepoPath(), comment.CommitSHA, endCommitID, treePath) + if err != nil { + return nil, fmt.Errorf("GetAffectedHunksForTwoCommitsSpecialFile[%s, %s, %s]: %w", repo.FullName(), comment.CommitSHA, endCommitID, err) + } + hunksCache[comment.CommitSHA] = hunks + } + + comment.Line = ReCalculateLineNumber(hunks, comment.Line) + comments[n] = comment + n++ + } + return comments[:n], nil +} + +// LoadCodeComments loads comments into each line, so that the comments can be displayed in the diff view. +// the comments' line number is recalculated based on the hunks of the diff. +func LoadCodeComments(ctx context.Context, gitRepo *git.Repository, repo *repo_model.Repository, diff *gitdiff.Diff, issueID int64, currentUser *user_model.User, startCommit, endCommit *git.Commit, showOutdatedComments bool) error { + if startCommit == nil || endCommit == nil { + return errors.New("startCommit and endCommit cannot be nil") + } + + allComments, err := issues_model.FetchCodeComments(ctx, repo, issueID, currentUser, showOutdatedComments) + if err != nil { + return err + } + + for _, file := range diff.Files { + if fileComments, ok := allComments[file.Name]; ok { + lineComments := make(map[int64][]*issues_model.Comment) + hunksCache := make(map[string][]*git.HunkInfo) + // filecomments should be sorted by created time, so that the latest comments are at the end + for _, comment := range fileComments { + dstCommitID := startCommit.ID.String() + if comment.Line > 0 { + dstCommitID = endCommit.ID.String() + } + + if comment.CommitSHA == dstCommitID { + lineComments[comment.Line] = append(lineComments[comment.Line], comment) + continue + } + + // If the comment is not for the current commit, we need to recalculate the line number + hunks, ok := hunksCache[comment.CommitSHA+".."+dstCommitID] + if !ok { + hunks, err = git.GetAffectedHunksForTwoCommitsSpecialFile(ctx, repo.RepoPath(), comment.CommitSHA, dstCommitID, file.Name) + if err != nil { + return fmt.Errorf("GetAffectedHunksForTwoCommitsSpecialFile[%s, %s, %s]: %w", repo.FullName(), dstCommitID, comment.CommitSHA, err) + } + hunksCache[comment.CommitSHA+".."+dstCommitID] = hunks + } + comment.Line = ReCalculateLineNumber(hunks, comment.Line) + if comment.Line != 0 { + dstCommit, err := gitRepo.GetCommit(dstCommitID) + if err != nil { + return fmt.Errorf("GetCommit[%s]: %w", dstCommitID, err) + } + // If the comment is not the first one or the comment created before the current commit + if len(lineComments[comment.Line]) > 0 || comment.CreatedUnix.AsTime().Before(dstCommit.Committer.When) { + lineComments[comment.Line] = append(lineComments[comment.Line], comment) + } + } + } + + for _, section := range file.Sections { + for _, line := range section.Lines { + if comments, ok := lineComments[int64(line.LeftIdx*-1)]; ok { + line.Comments = append(line.Comments, comments...) + } + if comments, ok := lineComments[int64(line.RightIdx)]; ok { + line.Comments = append(line.Comments, comments...) + } + sort.SliceStable(line.Comments, func(i, j int) bool { + return line.Comments[i].CreatedUnix < line.Comments[j].CreatedUnix + }) + } + } + } + } + return nil +} diff --git a/services/pull/review_test.go b/services/pull/review_test.go index 3bce1e523d7bc..80cc78bcbe646 100644 --- a/services/pull/review_test.go +++ b/services/pull/review_test.go @@ -10,6 +10,9 @@ import ( issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/gitrepo" + "code.gitea.io/gitea/services/gitdiff" pull_service "code.gitea.io/gitea/services/pull" "github.com/stretchr/testify/assert" @@ -46,3 +49,89 @@ func TestDismissReview(t *testing.T) { assert.Error(t, err) assert.True(t, pull_service.IsErrDismissRequestOnClosedPR(err)) } + +func setupDefaultDiff() *gitdiff.Diff { + return &gitdiff.Diff{ + Files: []*gitdiff.DiffFile{ + { + Name: "README.md", + Sections: []*gitdiff.DiffSection{ + { + Lines: []*gitdiff.DiffLine{ + { + LeftIdx: 4, + RightIdx: 4, + }, + }, + }, + }, + }, + }, + } +} + +func TestDiff_LoadCommentsNoOutdated(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2}) + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + diff := setupDefaultDiff() + assert.NoError(t, issue.LoadRepo(t.Context())) + assert.NoError(t, issue.LoadPullRequest(t.Context())) + + gitRepo, err := gitrepo.OpenRepository(t.Context(), issue.Repo) + assert.NoError(t, err) + defer gitRepo.Close() + startCommit, err := gitRepo.GetCommit(issue.PullRequest.MergeBase) + assert.NoError(t, err) + endCommit, err := gitRepo.GetCommit(issue.PullRequest.GetGitHeadRefName()) + assert.NoError(t, err) + + assert.NoError(t, pull_service.LoadCodeComments(db.DefaultContext, gitRepo, issue.Repo, diff, issue.ID, user, startCommit, endCommit, false)) + assert.Len(t, diff.Files[0].Sections[0].Lines[0].Comments, 2) +} + +func TestDiff_LoadCommentsWithOutdated(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2}) + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + assert.NoError(t, issue.LoadRepo(t.Context())) + assert.NoError(t, issue.LoadPullRequest(t.Context())) + + diff := setupDefaultDiff() + gitRepo, err := gitrepo.OpenRepository(t.Context(), issue.Repo) + assert.NoError(t, err) + defer gitRepo.Close() + startCommit, err := gitRepo.GetCommit(issue.PullRequest.MergeBase) + assert.NoError(t, err) + endCommit, err := gitRepo.GetCommit(issue.PullRequest.GetGitHeadRefName()) + assert.NoError(t, err) + + assert.NoError(t, pull_service.LoadCodeComments(db.DefaultContext, gitRepo, issue.Repo, diff, issue.ID, user, startCommit, endCommit, true)) + assert.Len(t, diff.Files[0].Sections[0].Lines[0].Comments, 3) +} + +func Test_reCalculateLineNumber(t *testing.T) { + hunks := []*git.HunkInfo{ + { + LeftLine: 0, + LeftHunk: 0, + RightLine: 1, + RightHunk: 3, + }, + } + assert.EqualValues(t, 6, pull_service.ReCalculateLineNumber(hunks, 3)) + + hunks = []*git.HunkInfo{ + { + LeftLine: 1, + LeftHunk: 4, + RightLine: 1, + RightHunk: 4, + }, + } + assert.EqualValues(t, 0, pull_service.ReCalculateLineNumber(hunks, 4)) + assert.EqualValues(t, 5, pull_service.ReCalculateLineNumber(hunks, 5)) + assert.EqualValues(t, 0, pull_service.ReCalculateLineNumber(hunks, -1)) +} diff --git a/templates/repo/diff/box.tmpl b/templates/repo/diff/box.tmpl index 22abf9a2193cc..ade1da655a0df 100644 --- a/templates/repo/diff/box.tmpl +++ b/templates/repo/diff/box.tmpl @@ -184,7 +184,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 58b675467c035..f701c700fc9f5 100644 --- a/templates/repo/diff/comment_form.tmpl +++ b/templates/repo/diff/comment_form.tmpl @@ -2,7 +2,8 @@ {{$.root.CsrfTokenHtml}} - + + diff --git a/templates/repo/issue/view_content/comments.tmpl b/templates/repo/issue/view_content/comments.tmpl index 089cdf2ccdd77..ea616b355b06d 100644 --- a/templates/repo/issue/view_content/comments.tmpl +++ b/templates/repo/issue/view_content/comments.tmpl @@ -448,10 +448,8 @@ {{if and .Review .Review.CodeComments}}
- {{range $filename, $lines := .Review.CodeComments}} - {{range $line, $comms := $lines}} - {{template "repo/issue/view_content/conversation" dict "." $ "comments" $comms}} - {{end}} + {{range $filename, $comms := .Review.CodeComments}} + {{template "repo/issue/view_content/conversation" dict "." $ "comments" $comms}} {{end}}
{{end}} From f8cea67265544ece8d1c9eca37a96eb1d969b2db Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 14 Jul 2025 09:58:28 -0700 Subject: [PATCH 02/35] improvements --- modules/git/commit.go | 6 +---- modules/git/diff.go | 5 ++-- modules/git/diff_test.go | 8 +++++- modules/git/repo_commit.go | 52 ++++++++++++++++++++++++++++---------- services/issue/comments.go | 2 ++ services/pull/review.go | 23 +++++++++++++---- 6 files changed, 68 insertions(+), 28 deletions(-) diff --git a/modules/git/commit.go b/modules/git/commit.go index aae40c575bcdb..7382018a3475c 100644 --- a/modules/git/commit.go +++ b/modules/git/commit.go @@ -258,11 +258,7 @@ func (c *Commit) CommitsBeforeLimit(num int) ([]*Commit, error) { // CommitsBeforeUntil returns the commits between commitID to current revision func (c *Commit) CommitsBeforeUntil(commitID string) ([]*Commit, error) { - endCommit, err := c.repo.GetCommit(commitID) - if err != nil { - return nil, err - } - return c.repo.CommitsBetween(c, endCommit) + return c.repo.CommitsBetween(c.ID.String(), commitID) } // SearchCommitsOptions specify the parameters for SearchCommits diff --git a/modules/git/diff.go b/modules/git/diff.go index 7f3d57ddbc834..34307f7768eee 100644 --- a/modules/git/diff.go +++ b/modules/git/diff.go @@ -15,7 +15,6 @@ import ( "strings" "code.gitea.io/gitea/modules/log" - "code.gitea.io/gitea/modules/util" ) // RawDiffType type of a raw diff. @@ -109,7 +108,7 @@ func ParseDiffHunkString(diffHunk string) (leftLine, leftHunk, rightLine, rightH if len(leftRange) > 1 { leftHunk, _ = strconv.Atoi(leftRange[1]) } else { - leftHunk = util.Iif(leftLine > 0, leftLine, -leftLine) + leftHunk = 1 } if len(ranges) > 1 { rightRange := strings.Split(ranges[1], ",") @@ -117,7 +116,7 @@ func ParseDiffHunkString(diffHunk string) (leftLine, leftHunk, rightLine, rightH if len(rightRange) > 1 { rightHunk, _ = strconv.Atoi(rightRange[1]) } else { - rightHunk = rightLine + rightHunk = 1 } } else { log.Debug("Parse line number failed: %v", diffHunk) diff --git a/modules/git/diff_test.go b/modules/git/diff_test.go index 11bdcd35fb1e8..8e618d354ad0d 100644 --- a/modules/git/diff_test.go +++ b/modules/git/diff_test.go @@ -185,6 +185,12 @@ func TestParseDiffHunkString(t *testing.T) { leftLine, leftHunk, rightLine, rightHunk = ParseDiffHunkString("@@ -1 +0,0 @@") assert.Equal(t, 1, leftLine) assert.Equal(t, 1, leftHunk) - assert.Equal(t, 0, rightLine) + assert.Equal(t, 1, rightLine) assert.Equal(t, 0, rightHunk) + + leftLine, leftHunk, rightLine, rightHunk = ParseDiffHunkString("@@ -2 +2 @@") + assert.Equal(t, 2, leftLine) + assert.Equal(t, 1, leftHunk) + assert.Equal(t, 2, rightLine) + assert.Equal(t, 1, rightHunk) } diff --git a/modules/git/repo_commit.go b/modules/git/repo_commit.go index 4066a1ca7ba1f..9ccb13b5a4e0c 100644 --- a/modules/git/repo_commit.go +++ b/modules/git/repo_commit.go @@ -6,6 +6,7 @@ package git import ( "bytes" + "context" "io" "os" "strconv" @@ -304,23 +305,47 @@ func (repo *Repository) FilesCountBetween(startCommitID, endCommitID string) (in // CommitsBetween returns a list that contains commits between [before, last). // If before is detached (removed by reset + push) it is not included. -func (repo *Repository) CommitsBetween(last, before *Commit) ([]*Commit, error) { +func (repo *Repository) CommitsBetween(lastCommitID, beforeCommitID string) ([]*Commit, error) { + commitIDs, err := CommitIDsBetween(repo.Ctx, repo.Path, beforeCommitID, lastCommitID) + if err != nil { + return nil, err + } + + commits := make([]*Commit, 0, len(commitIDs)) + for _, commitID := range commitIDs { + commit, err := repo.GetCommit(commitID) + if err != nil { + return nil, err + } + commits = append(commits, commit) + } + return commits, nil +} + +func CommitIDsBetween(ctx context.Context, repoPath, beforeCommitID, afterCommitID string) ([]string, error) { var stdout []byte var err error - if before == nil { - stdout, _, err = NewCommand("rev-list").AddDynamicArguments(last.ID.String()).RunStdBytes(repo.Ctx, &RunOpts{Dir: repo.Path}) + if beforeCommitID == "" { + stdout, _, err = NewCommand("rev-list").AddDynamicArguments(afterCommitID).RunStdBytes(ctx, &RunOpts{Dir: repoPath}) } else { - stdout, _, err = NewCommand("rev-list").AddDynamicArguments(before.ID.String()+".."+last.ID.String()).RunStdBytes(repo.Ctx, &RunOpts{Dir: repo.Path}) + stdout, _, err = NewCommand("rev-list").AddDynamicArguments(beforeCommitID+".."+afterCommitID).RunStdBytes(ctx, &RunOpts{Dir: repoPath}) if err != nil && strings.Contains(err.Error(), "no merge base") { // future versions of git >= 2.28 are likely to return an error if before and last have become unrelated. // previously it would return the results of git rev-list before last so let's try that... - stdout, _, err = NewCommand("rev-list").AddDynamicArguments(before.ID.String(), last.ID.String()).RunStdBytes(repo.Ctx, &RunOpts{Dir: repo.Path}) + stdout, _, err = NewCommand("rev-list").AddDynamicArguments(beforeCommitID, afterCommitID).RunStdBytes(ctx, &RunOpts{Dir: repoPath}) } } if err != nil { return nil, err } - return repo.parsePrettyFormatLogToList(bytes.TrimSpace(stdout)) + + commitIDs := make([]string, 0, 10) + for commitID := range bytes.SplitSeq(stdout, []byte{'\n'}) { + if len(commitID) > 0 { + commitIDs = append(commitIDs, string(commitID)) + } + } + return commitIDs, nil } // CommitsBetweenLimit returns a list that contains at most limit commits skipping the first skip commits between [before, last) @@ -375,18 +400,17 @@ func (repo *Repository) CommitsBetweenNotBase(last, before *Commit, baseBranch s // CommitsBetweenIDs return commits between twoe commits func (repo *Repository) CommitsBetweenIDs(last, before string) ([]*Commit, error) { - lastCommit, err := repo.GetCommit(last) + _, err := repo.GetCommit(last) if err != nil { return nil, err } - if before == "" { - return repo.CommitsBetween(lastCommit, nil) - } - beforeCommit, err := repo.GetCommit(before) - if err != nil { - return nil, err + if before != "" { + _, err := repo.GetCommit(before) + if err != nil { + return nil, err + } } - return repo.CommitsBetween(lastCommit, beforeCommit) + return repo.CommitsBetween(last, before) } // CommitsCountBetween return numbers of commits between two commits diff --git a/services/issue/comments.go b/services/issue/comments.go index 10c81198d57e2..63dd3171c9dbf 100644 --- a/services/issue/comments.go +++ b/services/issue/comments.go @@ -134,6 +134,8 @@ func UpdateComment(ctx context.Context, c *issues_model.Comment, contentVersion func DeleteComment(ctx context.Context, doer *user_model.User, comment *issues_model.Comment) error { err := db.WithTx(ctx, func(ctx context.Context) error { return issues_model.DeleteComment(ctx, comment) + // TODO: delete review if the comment is the last comment of the review + // TODO: delete comment attachments }) if err != nil { return err diff --git a/services/pull/review.go b/services/pull/review.go index 4e4fb31870e22..2586b81b535e4 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -10,6 +10,7 @@ import ( "fmt" "io" "regexp" + "slices" "sort" "strings" @@ -111,14 +112,23 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. defer closer.Close() } - if beforeCommitID == "" { + prCommitIDs, err := git.CommitIDsBetween(ctx, gitRepo.Path, beforeCommitID, afterCommitID) + if err != nil { + return nil, fmt.Errorf("CommitIDsBetween[%s, %s]: %w", beforeCommitID, afterCommitID, err) + } + + if beforeCommitID == "" || beforeCommitID == issue.PullRequest.MergeBase { beforeCommitID = issue.PullRequest.MergeBase } else { - beforeCommit, err := gitRepo.GetCommit(beforeCommitID) // Ensure beforeCommitID is valid + // beforeCommitID must be one of the pull request commits + if !slices.Contains(prCommitIDs, beforeCommitID) { + return nil, fmt.Errorf("beforeCommitID[%s] is not a valid pull request commit", beforeCommitID) + } + + beforeCommit, err := gitRepo.GetCommit(beforeCommitID) if err != nil { return nil, fmt.Errorf("GetCommit[%s]: %w", beforeCommitID, err) } - // TODO: beforeCommitID must be one of the pull request commits beforeCommit, err = beforeCommit.Parent(0) if err != nil { @@ -131,8 +141,11 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. if err != nil { return nil, fmt.Errorf("GetRefCommitID[%s]: %w", issue.PullRequest.GetGitHeadRefName(), err) } - } else { //nolint - // TODO: afterCommitID must be one of the pull request commits + } else { + // afterCommitID must be one of the pull request commits + if !slices.Contains(prCommitIDs, afterCommitID) { + return nil, fmt.Errorf("afterCommitID[%s] is not a valid pull request commit", afterCommitID) + } } // CreateCodeComment() is used for: From eddd21bdea4086d65e10713ee9c6916260bd3213 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 14 Jul 2025 12:07:12 -0700 Subject: [PATCH 03/35] Refactor calc mergebase --- models/issues/comment.go | 13 +++++ modules/git/ref.go | 12 +++++ modules/git/repo_commit_nogogit.go | 12 ----- modules/git/repo_compare_test.go | 4 +- routers/web/repo/pull.go | 82 ++++++++++++++++-------------- services/issue/comments.go | 47 +++++++++++++++-- services/issue/issue.go | 4 +- services/pull/pull.go | 4 +- services/pull/review.go | 49 ++++++++++-------- services/user/delete.go | 1 + 10 files changed, 149 insertions(+), 79 deletions(-) diff --git a/models/issues/comment.go b/models/issues/comment.go index 082ede1e199f7..a036b89d87cd5 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -764,6 +764,10 @@ func (c *Comment) CodeCommentLink(ctx context.Context) string { return fmt.Sprintf("%s/files#%s", c.Issue.Link(), c.HashTag()) } +func GetCodeCommentRef(prIndex, commentID int64) string { + return fmt.Sprintf("refs/pull/%d/code-comment-%d", prIndex, commentID) +} + // CreateComment creates comment with context func CreateComment(ctx context.Context, opts *CreateCommentOptions) (_ *Comment, err error) { ctx, committer, err := db.TxContext(ctx) @@ -1151,6 +1155,15 @@ func DeleteComment(ctx context.Context, comment *Comment) error { return err } + // delete review if the comment is the last comment of the review + if comment.ReviewID > 0 { + if _, err := db.GetEngine(ctx).ID(comment.ReviewID). + Where("(SELECT count(id) FROM comment WHERE review_id = ?) == 0", comment.ReviewID). + Delete(new(Review)); err != nil { + return err + } + } + if err := comment.neuterCrossReferences(ctx); err != nil { return err } diff --git a/modules/git/ref.go b/modules/git/ref.go index 56b2db858ad63..232c473828745 100644 --- a/modules/git/ref.go +++ b/modules/git/ref.go @@ -4,6 +4,7 @@ package git import ( + "context" "regexp" "strings" @@ -220,3 +221,14 @@ func (ref RefName) RefWebLinkPath() string { } return string(refType) + "/" + util.PathEscapeSegments(ref.ShortName()) } + +func UpdateRef(ctx context.Context, repoPath, refName, newCommitID string) error { + _, _, err := NewCommand("update-ref").AddDynamicArguments(refName, newCommitID).RunStdString(ctx, &RunOpts{Dir: repoPath}) + return err +} + +func RemoveRef(ctx context.Context, repoPath, refName string) error { + _, _, err := NewCommand("update-ref", "--no-deref", "-d"). + AddDynamicArguments(refName).RunStdString(ctx, &RunOpts{Dir: repoPath}) + return err +} diff --git a/modules/git/repo_commit_nogogit.go b/modules/git/repo_commit_nogogit.go index 3ead3e22165f4..29650179f554e 100644 --- a/modules/git/repo_commit_nogogit.go +++ b/modules/git/repo_commit_nogogit.go @@ -50,18 +50,6 @@ func (repo *Repository) GetRefCommitID(name string) (string, error) { return string(shaBs), nil } -// SetReference sets the commit ID string of given reference (e.g. branch or tag). -func (repo *Repository) SetReference(name, commitID string) error { - _, _, err := NewCommand("update-ref").AddDynamicArguments(name, commitID).RunStdString(repo.Ctx, &RunOpts{Dir: repo.Path}) - return err -} - -// RemoveReference removes the given reference (e.g. branch or tag). -func (repo *Repository) RemoveReference(name string) error { - _, _, err := NewCommand("update-ref", "--no-deref", "-d").AddDynamicArguments(name).RunStdString(repo.Ctx, &RunOpts{Dir: repo.Path}) - return err -} - // IsCommitExist returns true if given commit exists in current repository. func (repo *Repository) IsCommitExist(name string) bool { if err := ensureValidGitRepository(repo.Ctx, repo.Path); err != nil { diff --git a/modules/git/repo_compare_test.go b/modules/git/repo_compare_test.go index 25ee4c5198568..7bd6eab85d326 100644 --- a/modules/git/repo_compare_test.go +++ b/modules/git/repo_compare_test.go @@ -99,7 +99,7 @@ func TestReadWritePullHead(t *testing.T) { // Write a fake sha1 with only 40 zeros newCommit := "feaf4ba6bc635fec442f46ddd4512416ec43c2c2" - err = repo.SetReference(PullPrefix+"1/head", newCommit) + err = UpdateRef(t.Context(), repo.Path, PullPrefix+"1/head", newCommit) if err != nil { assert.NoError(t, err) return @@ -116,7 +116,7 @@ func TestReadWritePullHead(t *testing.T) { assert.Equal(t, headContents, newCommit) // Remove file after the test - err = repo.RemoveReference(PullPrefix + "1/head") + err = RemoveRef(t.Context(), repo.Path, PullPrefix+"1/head") assert.NoError(t, err) } diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 64d2ad330c9ac..97c0c15302dc6 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -190,7 +190,7 @@ func GetPullDiffStats(ctx *context.Context) { } pull := issue.PullRequest - mergeBaseCommitID := GetMergedBaseCommitID(ctx, issue) + mergeBaseCommitID := GetMergedBaseCommitID(ctx, pull) if mergeBaseCommitID == "" { return // no merge base, do nothing, do not stop the route handler, see below } @@ -210,48 +210,46 @@ func GetPullDiffStats(ctx *context.Context) { ctx.Data["DiffShortStat"] = diffShortStat } -func GetMergedBaseCommitID(ctx *context.Context, issue *issues_model.Issue) string { - pull := issue.PullRequest - - var baseCommit string - // Some migrated PR won't have any Base SHA and lose history, try to get one - if pull.MergeBase == "" { - var commitSHA, parentCommit string - // If there is a head or a patch file, and it is readable, grab info - commitSHA, err := ctx.Repo.GitRepo.GetRefCommitID(pull.GetGitHeadRefName()) - if err != nil { - // Head File does not exist, try the patch - commitSHA, err = ctx.Repo.GitRepo.ReadPatchCommit(pull.Index) - if err == nil { - // Recreate pull head in files for next time - if err := ctx.Repo.GitRepo.SetReference(pull.GetGitHeadRefName(), commitSHA); err != nil { - log.Error("Could not write head file", err) - } - } else { - // There is no history available - log.Trace("No history file available for PR %d", pull.Index) +func calcMergeBase(ctx *context.Context, pull *issues_model.PullRequest) string { + var commitSHA, parentCommit string + // If there is a head or a patch file, and it is readable, grab info + commitSHA, err := ctx.Repo.GitRepo.GetRefCommitID(pull.GetGitHeadRefName()) + if err != nil { + // Head File does not exist, try the patch + // FIXME: it seems this patch file is not used in the code, but it is still read + commitSHA, err = ctx.Repo.GitRepo.ReadPatchCommit(pull.Index) + if err == nil { + // Recreate pull head in files for next time + if err := git.UpdateRef(ctx, ctx.Repo.GitRepo.Path, pull.GetGitHeadRefName(), commitSHA); err != nil { + log.Error("Could not write head file", err) } + } else { + // There is no history available + log.Trace("No history file available for PR %d", pull.Index) } - if commitSHA != "" { - // Get immediate parent of the first commit in the patch, grab history back - parentCommit, _, err = git.NewCommand("rev-list", "-1", "--skip=1").AddDynamicArguments(commitSHA).RunStdString(ctx, &git.RunOpts{Dir: ctx.Repo.GitRepo.Path}) - if err == nil { - parentCommit = strings.TrimSpace(parentCommit) - } - // Special case on Git < 2.25 that doesn't fail on immediate empty history - if err != nil || parentCommit == "" { - log.Info("No known parent commit for PR %d, error: %v", pull.Index, err) - // bring at least partial history if it can work - parentCommit = commitSHA - } + } + if commitSHA != "" { + // Get immediate parent of the first commit in the patch, grab history back + parentCommit, _, err = git.NewCommand("rev-list", "-1", "--skip=1").AddDynamicArguments(commitSHA).RunStdString(ctx, &git.RunOpts{Dir: ctx.Repo.GitRepo.Path}) + if err == nil { + parentCommit = strings.TrimSpace(parentCommit) } - baseCommit = parentCommit - } else { - // Keep an empty history or original commit - baseCommit = pull.MergeBase + // Special case on Git < 2.25 that doesn't fail on immediate empty history + if err != nil || parentCommit == "" { + log.Info("No known parent commit for PR %d, error: %v", pull.Index, err) + // bring at least partial history if it can work + parentCommit = commitSHA + } + } + return parentCommit +} + +func GetMergedBaseCommitID(ctx *context.Context, pull *issues_model.PullRequest) string { + if pull.MergeBase != "" { + return pull.MergeBase } - return baseCommit + return calcMergeBase(ctx, pull) } func preparePullViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *git.CompareInfo { @@ -271,7 +269,13 @@ func prepareMergedViewPullInfo(ctx *context.Context, issue *issues_model.Issue) setMergeTarget(ctx, pull) ctx.Data["HasMerged"] = true - baseCommit := GetMergedBaseCommitID(ctx, issue) + baseCommit := GetMergedBaseCommitID(ctx, pull) + if baseCommit == "" { + ctx.Data["BaseTarget"] = pull.BaseBranch + ctx.Data["NumCommits"] = 0 + ctx.Data["NumFiles"] = 0 + return nil // no merge base, do nothing + } compareInfo, err := ctx.Repo.GitRepo.GetCompareInfo(ctx.Repo.Repository.RepoPath(), baseCommit, pull.GetGitHeadRefName(), false, false) diff --git a/services/issue/comments.go b/services/issue/comments.go index 63dd3171c9dbf..ba18099db7b8a 100644 --- a/services/issue/comments.go +++ b/services/issue/comments.go @@ -13,8 +13,11 @@ import ( access_model "code.gitea.io/gitea/models/perm/access" repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/json" + "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/storage" "code.gitea.io/gitea/modules/timeutil" git_service "code.gitea.io/gitea/services/git" notify_service "code.gitea.io/gitea/services/notify" @@ -133,9 +136,47 @@ func UpdateComment(ctx context.Context, c *issues_model.Comment, contentVersion // DeleteComment deletes the comment func DeleteComment(ctx context.Context, doer *user_model.User, comment *issues_model.Comment) error { err := db.WithTx(ctx, func(ctx context.Context) error { - return issues_model.DeleteComment(ctx, comment) - // TODO: delete review if the comment is the last comment of the review - // TODO: delete comment attachments + if err := comment.LoadAttachments(ctx); err != nil { + return err + } + + if err := issues_model.DeleteComment(ctx, comment); err != nil { + return err + } + + // delete comment attachments + if _, err := repo_model.DeleteAttachments(ctx, comment.Attachments, true); err != nil { + return fmt.Errorf("delete attachments: %w", err) + } + + if comment.ReviewID > 0 { + if err := comment.LoadIssue(ctx); err != nil { + return err + } + if err := comment.Issue.LoadRepo(ctx); err != nil { + return err + } + if err := comment.Issue.LoadPullRequest(ctx); err != nil { + return err + } + if err := git.RemoveRef(ctx, comment.Issue.Repo.RepoPath(), issues_model.GetCodeCommentRef(comment.Issue.PullRequest.Index, comment.ID)); err != nil { + log.Error("Unable to remove ref in base repository for PR[%d] Error: %v", comment.Issue.PullRequest.ID, err) + // We should not return error here, because the comment has been removed from database. + // users have to delete this ref manually or we should have a synchronize between + // database comment table and git refs. + } + } + + for _, attachment := range comment.Attachments { + if err := storage.Attachments.Delete(repo_model.AttachmentRelativePath(attachment.UUID)); err != nil { + // Even delete files failed, but the attachments has been removed from database, so we + // should not return error but only record the error on logs. + // users have to delete this attachments manually or we should have a + // synchronize between database attachment table and attachment storage + log.Error("delete attachment[uuid: %s] failed: %v", attachment.UUID, err) + } + } + return nil }) if err != nil { return err diff --git a/services/issue/issue.go b/services/issue/issue.go index f03be3e18f6c7..9114cd27d3318 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -200,7 +200,7 @@ func DeleteIssue(ctx context.Context, doer *user_model.User, gitRepo *git.Reposi // delete pull request related git data if issue.IsPull && gitRepo != nil { - if err := gitRepo.RemoveReference(issue.PullRequest.GetGitHeadRefName()); err != nil { + if err := git.RemoveRef(ctx, gitRepo.Path, issue.PullRequest.GetGitHeadRefName()); err != nil { return err } } @@ -301,6 +301,8 @@ func deleteIssue(ctx context.Context, issue *issues_model.Issue) ([]string, erro attachmentPaths = append(attachmentPaths, issue.Attachments[i].RelativePath()) } + // TODO: deference all review comments + // delete all database data still assigned to this issue if err := db.DeleteBeans(ctx, &issues_model.ContentHistory{IssueID: issue.ID}, diff --git a/services/pull/pull.go b/services/pull/pull.go index 2829e15441081..76d5bdcb05d98 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -184,7 +184,7 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { return nil }); err != nil { // cleanup: this will only remove the reference, the real commit will be clean up when next GC - if err1 := baseGitRepo.RemoveReference(pr.GetGitHeadRefName()); err1 != nil { + if err1 := git.RemoveRef(ctx, baseGitRepo.Path, pr.GetGitHeadRefName()); err1 != nil { log.Error("RemoveReference: %v", err1) } return err @@ -648,7 +648,7 @@ func UpdateRef(ctx context.Context, pr *issues_model.PullRequest) (err error) { return err } - _, _, err = git.NewCommand("update-ref").AddDynamicArguments(pr.GetGitHeadRefName(), pr.HeadCommitID).RunStdString(ctx, &git.RunOpts{Dir: pr.BaseRepo.RepoPath()}) + err = git.UpdateRef(ctx, pr.BaseRepo.RepoPath(), pr.GetGitHeadRefName(), pr.HeadCommitID) if err != nil { log.Error("Unable to update ref in base repository for PR[%d] Error: %v", pr.ID, err) } diff --git a/services/pull/review.go b/services/pull/review.go index 2586b81b535e4..9f03f60c376fe 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -21,6 +21,7 @@ import ( "code.gitea.io/gitea/modules/cache" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/gitrepo" + "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/optional" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/util" @@ -141,11 +142,8 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. if err != nil { return nil, fmt.Errorf("GetRefCommitID[%s]: %w", issue.PullRequest.GetGitHeadRefName(), err) } - } else { - // afterCommitID must be one of the pull request commits - if !slices.Contains(prCommitIDs, afterCommitID) { - return nil, fmt.Errorf("afterCommitID[%s] is not a valid pull request commit", afterCommitID) - } + } else if !slices.Contains(prCommitIDs, afterCommitID) { // afterCommitID must be one of the pull request commits + return nil, fmt.Errorf("afterCommitID[%s] is not a valid pull request commit", afterCommitID) } // CreateCodeComment() is used for: @@ -279,22 +277,33 @@ func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_mo } lineCommitID := util.Iif(line < 0, beforeCommitID, afterCommitID) - // TODO: the commit ID Must be referenced in the git repository, because the branch maybe rebased or force-pushed. + return db.WithTx2(ctx, func(ctx context.Context) (*issues_model.Comment, error) { + comment, err := issues_model.CreateComment(ctx, &issues_model.CreateCommentOptions{ + Type: issues_model.CommentTypeCode, + Doer: doer, + Repo: repo, + Issue: issue, + Content: content, + LineNum: line, + TreePath: treePath, + CommitSHA: lineCommitID, + ReviewID: reviewID, + Patch: patch, + Invalidated: false, + Attachments: attachments, + }) + if err != nil { + return nil, err + } - // If the commit ID is not referenced, it cannot be calculated the position dynamically. - return issues_model.CreateComment(ctx, &issues_model.CreateCommentOptions{ - Type: issues_model.CommentTypeCode, - Doer: doer, - Repo: repo, - Issue: issue, - Content: content, - LineNum: line, - TreePath: treePath, - CommitSHA: lineCommitID, - ReviewID: reviewID, - Patch: patch, - Invalidated: false, - Attachments: attachments, + // The line commit ID Must be referenced in the git repository, because the branch maybe rebased or force-pushed. + // If the review commit is GC, the position will not be calculated dynamically. + if err := git.UpdateRef(ctx, pr.BaseRepo.RepoPath(), issues_model.GetCodeCommentRef(pr.Index, comment.ID), lineCommitID); err != nil { + log.Error("Unable to update ref in base repository for PR[%d] Error: %v", pr.ID, err) + return nil, err + } + + return comment, nil }) } diff --git a/services/user/delete.go b/services/user/delete.go index 39c6ef052dca7..b8e51c550ff70 100644 --- a/services/user/delete.go +++ b/services/user/delete.go @@ -120,6 +120,7 @@ func deleteUser(ctx context.Context, u *user_model.User, purge bool) (err error) if err = issues_model.DeleteComment(ctx, comment); err != nil { return err } + // TODO: delete attachments } } From 5ba2216a8cde2bde26aacb57e35c42b94ef48b21 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 14 Jul 2025 13:23:21 -0700 Subject: [PATCH 04/35] improvements --- modules/git/repo_compare.go | 28 ---------------------- modules/git/repo_compare_test.go | 30 ----------------------- routers/web/repo/pull.go | 41 +++++--------------------------- services/pull/merge_merge.go | 27 +++++++++++++++++++++ 4 files changed, 33 insertions(+), 93 deletions(-) diff --git a/modules/git/repo_compare.go b/modules/git/repo_compare.go index ff44506e13c2d..d32e5c9e1e0d5 100644 --- a/modules/git/repo_compare.go +++ b/modules/git/repo_compare.go @@ -5,14 +5,10 @@ package git import ( - "bufio" "bytes" "context" - "errors" "fmt" "io" - "os" - "path/filepath" "regexp" "strconv" "strings" @@ -193,8 +189,6 @@ func GetDiffShortStatByCmdArgs(ctx context.Context, repoPath string, trustedArgs var shortStatFormat = regexp.MustCompile( `\s*(\d+) files? changed(?:, (\d+) insertions?\(\+\))?(?:, (\d+) deletions?\(-\))?`) -var patchCommits = regexp.MustCompile(`^From\s(\w+)\s`) - func parseDiffStat(stdout string) (numFiles, totalAdditions, totalDeletions int, err error) { if len(stdout) == 0 || stdout == "\n" { return 0, 0, 0, nil @@ -282,25 +276,3 @@ func (repo *Repository) GetFilesChangedBetween(base, head string) ([]string, err return split, err } - -// ReadPatchCommit will check if a diff patch exists and return stats -func (repo *Repository) ReadPatchCommit(prID int64) (commitSHA string, err error) { - // Migrated repositories download patches to "pulls" location - patchFile := fmt.Sprintf("pulls/%d.patch", prID) - loadPatch, err := os.Open(filepath.Join(repo.Path, patchFile)) - if err != nil { - return "", err - } - defer loadPatch.Close() - // Read only the first line of the patch - usually it contains the first commit made in patch - scanner := bufio.NewScanner(loadPatch) - scanner.Scan() - // Parse the Patch stats, sometimes Migration returns a 404 for the patch file - commitSHAGroups := patchCommits.FindStringSubmatch(scanner.Text()) - if len(commitSHAGroups) != 0 { - commitSHA = commitSHAGroups[1] - } else { - return "", errors.New("patch file doesn't contain valid commit ID") - } - return commitSHA, nil -} diff --git a/modules/git/repo_compare_test.go b/modules/git/repo_compare_test.go index 7bd6eab85d326..c1ebdd6a69252 100644 --- a/modules/git/repo_compare_test.go +++ b/modules/git/repo_compare_test.go @@ -45,36 +45,6 @@ func TestGetFormatPatch(t *testing.T) { assert.Contains(t, patch, "Subject: [PATCH] Add file2.txt") } -func TestReadPatch(t *testing.T) { - // Ensure we can read the patch files - bareRepo1Path := filepath.Join(testReposDir, "repo1_bare") - repo, err := openRepositoryWithDefaultContext(bareRepo1Path) - if err != nil { - assert.NoError(t, err) - return - } - defer repo.Close() - // This patch doesn't exist - noFile, err := repo.ReadPatchCommit(0) - assert.Error(t, err) - - // This patch is an empty one (sometimes it's a 404) - noCommit, err := repo.ReadPatchCommit(1) - assert.Error(t, err) - - // This patch is legit and should return a commit - oldCommit, err := repo.ReadPatchCommit(2) - if err != nil { - assert.NoError(t, err) - return - } - - assert.Empty(t, noFile) - assert.Empty(t, noCommit) - assert.Len(t, oldCommit, 40) - assert.Equal(t, "6e8e2a6f9efd71dbe6917816343ed8415ad696c3", oldCommit) -} - func TestReadWritePullHead(t *testing.T) { // Ensure we can write SHA1 head corresponding to PR and open them bareRepo1Path := filepath.Join(testReposDir, "repo1_bare") diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 97c0c15302dc6..581d97bd4f6eb 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -210,46 +210,17 @@ func GetPullDiffStats(ctx *context.Context) { ctx.Data["DiffShortStat"] = diffShortStat } -func calcMergeBase(ctx *context.Context, pull *issues_model.PullRequest) string { - var commitSHA, parentCommit string - // If there is a head or a patch file, and it is readable, grab info - commitSHA, err := ctx.Repo.GitRepo.GetRefCommitID(pull.GetGitHeadRefName()) - if err != nil { - // Head File does not exist, try the patch - // FIXME: it seems this patch file is not used in the code, but it is still read - commitSHA, err = ctx.Repo.GitRepo.ReadPatchCommit(pull.Index) - if err == nil { - // Recreate pull head in files for next time - if err := git.UpdateRef(ctx, ctx.Repo.GitRepo.Path, pull.GetGitHeadRefName(), commitSHA); err != nil { - log.Error("Could not write head file", err) - } - } else { - // There is no history available - log.Trace("No history file available for PR %d", pull.Index) - } - } - if commitSHA != "" { - // Get immediate parent of the first commit in the patch, grab history back - parentCommit, _, err = git.NewCommand("rev-list", "-1", "--skip=1").AddDynamicArguments(commitSHA).RunStdString(ctx, &git.RunOpts{Dir: ctx.Repo.GitRepo.Path}) - if err == nil { - parentCommit = strings.TrimSpace(parentCommit) - } - // Special case on Git < 2.25 that doesn't fail on immediate empty history - if err != nil || parentCommit == "" { - log.Info("No known parent commit for PR %d, error: %v", pull.Index, err) - // bring at least partial history if it can work - parentCommit = commitSHA - } - } - return parentCommit -} - func GetMergedBaseCommitID(ctx *context.Context, pull *issues_model.PullRequest) string { if pull.MergeBase != "" { return pull.MergeBase } - return calcMergeBase(ctx, pull) + var err error + pull.MergeBase, err = pull_service.CalcMergeBase(ctx, pull) + if err != nil { + log.Error("CalcMergeBase: %v", err) + } + return pull.MergeBase } func preparePullViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *git.CompareInfo { diff --git a/services/pull/merge_merge.go b/services/pull/merge_merge.go index 118d21c7cd9bc..d7f059d6acd38 100644 --- a/services/pull/merge_merge.go +++ b/services/pull/merge_merge.go @@ -4,6 +4,10 @@ package pull import ( + "context" + "strings" + + issues_model "code.gitea.io/gitea/models/issues" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" @@ -23,3 +27,26 @@ func doMergeStyleMerge(ctx *mergeContext, message string) error { } return nil } + +// CalcMergeBase calculates the merge base for a pull request. +func CalcMergeBase(ctx context.Context, pr *issues_model.PullRequest) (string, error) { + repoPath := pr.BaseRepo.RepoPath() + if pr.HasMerged { + mergeBase, _, err := git.NewCommand("merge-base").AddDashesAndList(pr.MergedCommitID+"^", pr.GetGitHeadRefName()). + RunStdString(ctx, &git.RunOpts{Dir: repoPath}) + return strings.TrimSpace(mergeBase), err + } + + mergeBase, _, err := git.NewCommand("merge-base").AddDashesAndList(pr.BaseBranch, pr.GetGitHeadRefName()). + RunStdString(ctx, &git.RunOpts{Dir: repoPath}) + if err != nil { + var err2 error + mergeBase, _, err2 = git.NewCommand("rev-parse").AddDynamicArguments(git.BranchPrefix+pr.BaseBranch). + RunStdString(ctx, &git.RunOpts{Dir: repoPath}) + if err2 != nil { + log.Error("Unable to get merge base for PR ID %d, Index %d in %s/%s. Error: %v & %v", pr.ID, pr.Index, pr.BaseRepo.OwnerName, pr.BaseRepo.Name, err, err2) + return "", err2 + } + } + return strings.TrimSpace(mergeBase), nil +} From aaa53641aed8c01556462067e44dda05a416c298 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 14 Jul 2025 15:13:40 -0700 Subject: [PATCH 05/35] Add migrations for wrong negative line number in the review code comment --- models/migrations/migrations.go | 4 ++++ models/migrations/v1_25/v321.go | 14 ++++++++++++++ 2 files changed, 18 insertions(+) create mode 100644 models/migrations/v1_25/v321.go diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 176372486e8f6..e1a6a6f8c7903 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -24,6 +24,7 @@ import ( "code.gitea.io/gitea/models/migrations/v1_22" "code.gitea.io/gitea/models/migrations/v1_23" "code.gitea.io/gitea/models/migrations/v1_24" + "code.gitea.io/gitea/models/migrations/v1_25" "code.gitea.io/gitea/models/migrations/v1_6" "code.gitea.io/gitea/models/migrations/v1_7" "code.gitea.io/gitea/models/migrations/v1_8" @@ -382,6 +383,9 @@ func prepareMigrationTasks() []*migration { newMigration(318, "Add anonymous_access_mode for repo_unit", v1_24.AddRepoUnitAnonymousAccessMode), newMigration(319, "Add ExclusiveOrder to Label table", v1_24.AddExclusiveOrderColumnToLabelTable), newMigration(320, "Migrate two_factor_policy to login_source table", v1_24.MigrateSkipTwoFactor), + + // Gitea 1.24.0 ends at database version 321 + newMigration(321, "Migrate commit id of pull requests code review comment", v1_25.MigrateCommitIDOfPullRequestCodeReviewComment), } return preparedMigrations } diff --git a/models/migrations/v1_25/v321.go b/models/migrations/v1_25/v321.go new file mode 100644 index 0000000000000..bfed6127db257 --- /dev/null +++ b/models/migrations/v1_25/v321.go @@ -0,0 +1,14 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package v1_25 + +import ( + "xorm.io/xorm" +) + +// MigrateCommitIDOfPullRequestCodeReviewComment this will be almost right before comment on the special commit of the pull request +func MigrateCommitIDOfPullRequestCodeReviewComment(x *xorm.Engine) error { + _, err := x.Exec("UPDATE comment SET commit_sha = (select merge_base from pull_request WHERE issue_id = comment.issue_id) WHERE line < 0") + return err +} From 2256fa7e9ce2c7eff045d50b68b3ef79c0a19359 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 14 Jul 2025 16:54:06 -0700 Subject: [PATCH 06/35] Fix bug --- models/issues/issue.go | 4 ++-- services/issue/issue.go | 41 +++++++++++++++++++++++++++++++++++++++-- 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/models/issues/issue.go b/models/issues/issue.go index a86d50ca9da3c..238f4bf5cb137 100644 --- a/models/issues/issue.go +++ b/models/issues/issue.go @@ -223,7 +223,7 @@ func (issue *Issue) LoadPullRequest(ctx context.Context) (err error) { return nil } -func (issue *Issue) loadComments(ctx context.Context) (err error) { +func (issue *Issue) LoadComments(ctx context.Context) (err error) { return issue.loadCommentsByType(ctx, CommentTypeUndefined) } @@ -344,7 +344,7 @@ func (issue *Issue) LoadAttributes(ctx context.Context) (err error) { return err } - if err = issue.loadComments(ctx); err != nil { + if err = issue.LoadComments(ctx); err != nil { return err } diff --git a/services/issue/issue.go b/services/issue/issue.go index 9114cd27d3318..e40f6453ad6e6 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -301,12 +301,14 @@ func deleteIssue(ctx context.Context, issue *issues_model.Issue) ([]string, erro attachmentPaths = append(attachmentPaths, issue.Attachments[i].RelativePath()) } - // TODO: deference all review comments + // deference all review comments + if err := issue.LoadComments(ctx); err != nil { + return nil, err + } // delete all database data still assigned to this issue if err := db.DeleteBeans(ctx, &issues_model.ContentHistory{IssueID: issue.ID}, - &issues_model.Comment{IssueID: issue.ID}, &issues_model.IssueLabel{IssueID: issue.ID}, &issues_model.IssueDependency{IssueID: issue.ID}, &issues_model.IssueAssignees{IssueID: issue.ID}, @@ -327,6 +329,41 @@ func deleteIssue(ctx context.Context, issue *issues_model.Issue) ([]string, erro return nil, err } + for _, comment := range issue.Comments { + if err := issues_model.DeleteComment(ctx, comment); err != nil { + return nil, err + } + + if comment.ReviewID > 0 { + if err := comment.LoadIssue(ctx); err != nil { + return nil, err + } + if err := comment.Issue.LoadRepo(ctx); err != nil { + return nil, err + } + if err := comment.Issue.LoadPullRequest(ctx); err != nil { + return nil, err + } + if err := git.RemoveRef(ctx, comment.Issue.Repo.RepoPath(), issues_model.GetCodeCommentRef(comment.Issue.PullRequest.Index, comment.ID)); err != nil { + log.Error("Unable to remove ref in base repository for PR[%d] Error: %v", comment.Issue.PullRequest.ID, err) + // We should not return error here, because the comment has been removed from database. + // users have to delete this ref manually or we should have a synchronize between + // database comment table and git refs. + } + } + + // delete all attachments related to this comment + for _, attachment := range comment.Attachments { + if err := storage.Attachments.Delete(repo_model.AttachmentRelativePath(attachment.UUID)); err != nil { + // Even delete files failed, but the attachments has been removed from database, so we + // should not return error but only record the error on logs. + // users have to delete this attachments manually or we should have a + // synchronize between database attachment table and attachment storage + log.Error("delete attachment[uuid: %s] failed: %v", attachment.UUID, err) + } + } + } + if err := committer.Commit(); err != nil { return nil, err } From 65a01c2c47d069333a08a5b02029b04ac3644b3b Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 14 Jul 2025 19:14:51 -0700 Subject: [PATCH 07/35] Fix bug --- models/issues/pull.go | 21 --------------- services/issue/issue.go | 48 ++++++++++++++++++++++++----------- services/repository/delete.go | 14 ++++------ 3 files changed, 38 insertions(+), 45 deletions(-) diff --git a/models/issues/pull.go b/models/issues/pull.go index 4c25b6f0c8494..6d737076e5fa0 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -15,7 +15,6 @@ import ( "code.gitea.io/gitea/models/db" git_model "code.gitea.io/gitea/models/git" org_model "code.gitea.io/gitea/models/organization" - pull_model "code.gitea.io/gitea/models/pull" repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/git" @@ -156,26 +155,6 @@ func init() { db.RegisterModel(new(PullRequest)) } -// DeletePullsByBaseRepoID deletes all pull requests by the base repository ID -func DeletePullsByBaseRepoID(ctx context.Context, repoID int64) error { - deleteCond := builder.Select("id").From("pull_request").Where(builder.Eq{"pull_request.base_repo_id": repoID}) - - // Delete scheduled auto merges - if _, err := db.GetEngine(ctx).In("pull_id", deleteCond). - Delete(&pull_model.AutoMerge{}); err != nil { - return err - } - - // Delete review states - if _, err := db.GetEngine(ctx).In("pull_id", deleteCond). - Delete(&pull_model.ReviewState{}); err != nil { - return err - } - - _, err := db.DeleteByBean(ctx, &PullRequest{BaseRepoID: repoID}) - return err -} - func (pr *PullRequest) String() string { if pr == nil { return "" diff --git a/services/issue/issue.go b/services/issue/issue.go index e40f6453ad6e6..70a395eb12cee 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -12,6 +12,7 @@ import ( issues_model "code.gitea.io/gitea/models/issues" access_model "code.gitea.io/gitea/models/perm/access" project_model "code.gitea.io/gitea/models/project" + pull_model "code.gitea.io/gitea/models/pull" repo_model "code.gitea.io/gitea/models/repo" system_model "code.gitea.io/gitea/models/system" user_model "code.gitea.io/gitea/models/user" @@ -267,10 +268,6 @@ func deleteIssue(ctx context.Context, issue *issues_model.Issue) ([]string, erro } defer committer.Close() - if _, err := db.GetEngine(ctx).ID(issue.ID).NoAutoCondition().Delete(issue); err != nil { - return nil, err - } - // update the total issue numbers if err := repo_model.UpdateRepoIssueNumbers(ctx, issue.RepoID, issue.IsPull, false); err != nil { return nil, err @@ -302,6 +299,13 @@ func deleteIssue(ctx context.Context, issue *issues_model.Issue) ([]string, erro } // deference all review comments + if err := issue.LoadRepo(ctx); err != nil { + return nil, err + } + if err := issue.LoadPullRequest(ctx); err != nil { + return nil, err + } + if err := issue.LoadComments(ctx); err != nil { return nil, err } @@ -335,17 +339,8 @@ func deleteIssue(ctx context.Context, issue *issues_model.Issue) ([]string, erro } if comment.ReviewID > 0 { - if err := comment.LoadIssue(ctx); err != nil { - return nil, err - } - if err := comment.Issue.LoadRepo(ctx); err != nil { - return nil, err - } - if err := comment.Issue.LoadPullRequest(ctx); err != nil { - return nil, err - } - if err := git.RemoveRef(ctx, comment.Issue.Repo.RepoPath(), issues_model.GetCodeCommentRef(comment.Issue.PullRequest.Index, comment.ID)); err != nil { - log.Error("Unable to remove ref in base repository for PR[%d] Error: %v", comment.Issue.PullRequest.ID, err) + if err := git.RemoveRef(ctx, issue.Repo.RepoPath(), issues_model.GetCodeCommentRef(issue.PullRequest.Index, comment.ID)); err != nil { + log.Error("Unable to remove ref in base repository for PR[%d] Error: %v", issue.PullRequest.ID, err) // We should not return error here, because the comment has been removed from database. // users have to delete this ref manually or we should have a synchronize between // database comment table and git refs. @@ -364,6 +359,29 @@ func deleteIssue(ctx context.Context, issue *issues_model.Issue) ([]string, erro } } + // delete all pull request records + if issue.IsPull { + // Delete scheduled auto merges + if _, err := db.GetEngine(ctx).Where("pull_id=?", issue.PullRequest.ID). + Delete(&pull_model.AutoMerge{}); err != nil { + return nil, err + } + + // Delete review states + if _, err := db.GetEngine(ctx).Where("pull_id=?", issue.PullRequest.ID). + Delete(&pull_model.ReviewState{}); err != nil { + return nil, err + } + + if _, err := db.GetEngine(ctx).ID(issue.PullRequest.ID).Delete(&issues_model.PullRequest{}); err != nil { + return nil, err + } + } + + if _, err := db.GetEngine(ctx).ID(issue.ID).NoAutoCondition().Delete(issue); err != nil { + return nil, err + } + if err := committer.Commit(); err != nil { return nil, err } diff --git a/services/repository/delete.go b/services/repository/delete.go index c48d6e1d56e94..a42c77c331f3c 100644 --- a/services/repository/delete.go +++ b/services/repository/delete.go @@ -97,10 +97,6 @@ func DeleteRepositoryDirectly(ctx context.Context, repoID int64, ignoreOrgTeams } needRewriteKeysFile := deleted > 0 - if err := deleteDBRepository(ctx, repoID); err != nil { - return err - } - if org != nil && org.IsOrganization() { teams, err := organization.FindOrgTeams(ctx, org.ID) if err != nil { @@ -187,11 +183,6 @@ func DeleteRepositoryDirectly(ctx context.Context, repoID int64, ignoreOrgTeams return err } - // Delete Pulls and related objects - if err := issues_model.DeletePullsByBaseRepoID(ctx, repoID); err != nil { - return err - } - // Delete Issues and related objects var attachmentPaths []string if attachmentPaths, err = issue_service.DeleteIssuesByRepoID(ctx, repoID); err != nil { @@ -291,6 +282,11 @@ func DeleteRepositoryDirectly(ctx context.Context, repoID int64, ignoreOrgTeams return err } + // delete all related database records first before deleting the repository record + if err := deleteDBRepository(ctx, repoID); err != nil { + return err + } + if err = committer.Commit(); err != nil { return err } From 968b2c5dc12bc7327f7b6135cf484b5cc1e28dc3 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 14 Jul 2025 20:54:16 -0700 Subject: [PATCH 08/35] Fix bug --- models/issues/comment_test.go | 11 +++-- models/issues/pull.go | 7 ++- routers/web/repo/pull_review.go | 46 ++++++++++++++++--- services/automergequeue/automergequeue.go | 4 +- services/convert/pull_review.go | 4 ++ services/issue/issue.go | 5 +++ services/issue/pull.go | 26 +---------- services/pull/merge.go | 6 +++ services/pull/review.go | 54 +++++++++++++++++------ 9 files changed, 112 insertions(+), 51 deletions(-) diff --git a/models/issues/comment_test.go b/models/issues/comment_test.go index 627cc188a6625..b5849c6e90513 100644 --- a/models/issues/comment_test.go +++ b/models/issues/comment_test.go @@ -71,9 +71,14 @@ func TestFetchCodeComments(t *testing.T) { res, err := issues_model.FetchCodeComments(db.DefaultContext, issue.Repo, issue.ID, user, false) assert.NoError(t, err) assert.Contains(t, res, "README.md") - assert.Contains(t, res["README.md"], int64(4)) - assert.Len(t, res["README.md"][4], 1) - assert.Equal(t, int64(4), res["README.md"][0].ID) + fourthLineComments := []*issues_model.Comment{} + for _, comment := range res["README.md"] { + if comment.Line == 4 { + fourthLineComments = append(fourthLineComments, comment) + } + } + assert.Len(t, fourthLineComments, 1) + assert.Equal(t, int64(4), fourthLineComments[0].ID) user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) res, err = issues_model.FetchCodeComments(db.DefaultContext, issue.Repo, issue.ID, user2, false) diff --git a/models/issues/pull.go b/models/issues/pull.go index 6d737076e5fa0..77af38e0afa6b 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -392,11 +392,16 @@ func (pr *PullRequest) getReviewedByLines(ctx context.Context, writer io.Writer) return committer.Commit() } -// GetGitHeadRefName returns git ref for hidden pull request branch +// GetGitHeadRefName returns git head commit id ref for the pull request's branch func (pr *PullRequest) GetGitHeadRefName() string { return fmt.Sprintf("%s%d/head", git.PullPrefix, pr.Index) } +// GetGitMergeRefName returns git merged commit id ref for the pull request +func (pr *PullRequest) GetGitMergeRefName() string { + return fmt.Sprintf("%s%d/merge", git.PullPrefix, pr.Index) +} + func (pr *PullRequest) GetGitHeadBranchRefName() string { return fmt.Sprintf("%s%s", git.BranchPrefix, pr.HeadBranch) } diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index a8fdd613a7b3c..29b436e9fc33d 100644 --- a/routers/web/repo/pull_review.go +++ b/routers/web/repo/pull_review.go @@ -7,11 +7,13 @@ import ( "errors" "fmt" "net/http" + "slices" issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/organization" pull_model "code.gitea.io/gitea/models/pull" user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/json" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" @@ -170,20 +172,50 @@ func renderConversation(ctx *context.Context, comment *issues_model.Comment, ori ctx.ServerError("comment.Issue.LoadPullRequest", err) return } - if beforeCommitID == "" { - beforeCommitID = comment.Issue.PullRequest.MergeBase + + headCommitID, err := ctx.Repo.GitRepo.GetRefCommitID(comment.Issue.PullRequest.GetGitHeadRefName()) + if err != nil { + ctx.ServerError("GetRefCommitID", err) + return } - if afterCommitID == "" { - var err error - afterCommitID, err = ctx.Repo.GitRepo.GetRefCommitID(comment.Issue.PullRequest.GetGitHeadRefName()) + + prCommitIDs, err := git.CommitIDsBetween(ctx, ctx.Repo.GitRepo.Path, comment.Issue.PullRequest.MergeBase, headCommitID) + if err != nil { + ctx.ServerError("CommitIDsBetween", err) + return + } + + if beforeCommitID == "" || beforeCommitID == comment.Issue.PullRequest.MergeBase { + beforeCommitID = comment.Issue.PullRequest.MergeBase + } else { + // beforeCommitID must be one of the pull request commits + if !slices.Contains(prCommitIDs, beforeCommitID) { + ctx.HTTPError(http.StatusBadRequest, fmt.Sprintf("beforeCommitID[%s] is not a valid pull request commit", beforeCommitID)) + return + } + + beforeCommit, err := ctx.Repo.GitRepo.GetCommit(beforeCommitID) if err != nil { - ctx.ServerError("GetRefCommitID", err) + ctx.ServerError("GetCommit", err) return } + + beforeCommit, err = beforeCommit.Parent(0) + if err != nil { + ctx.ServerError("GetParent", err) + return + } + beforeCommitID = beforeCommit.ID.String() + } + if afterCommitID == "" { + afterCommitID = headCommitID + } else if !slices.Contains(prCommitIDs, afterCommitID) { // afterCommitID must be one of the pull request commits + ctx.HTTPError(http.StatusBadRequest, fmt.Sprintf("afterCommitID[%s] is not a valid pull request commit", afterCommitID)) + return } showOutdatedComments := origin == "timeline" || ctx.Data["ShowOutdatedComments"].(bool) - comments, err := pull_service.FetchCodeCommentsByLine(ctx, ctx.Repo.Repository, comment.IssueID, + comments, err := pull_service.FetchCodeCommentsByLine(ctx, ctx.Repo.GitRepo, ctx.Repo.Repository, comment.IssueID, ctx.Doer, beforeCommitID, afterCommitID, comment.TreePath, comment.Line, showOutdatedComments) if err != nil { ctx.ServerError("FetchCodeCommentsByLine", err) diff --git a/services/automergequeue/automergequeue.go b/services/automergequeue/automergequeue.go index fa9c04da87492..aed8cded44102 100644 --- a/services/automergequeue/automergequeue.go +++ b/services/automergequeue/automergequeue.go @@ -39,11 +39,11 @@ func StartPRCheckAndAutoMerge(ctx context.Context, pull *issues_model.PullReques return } defer gitRepo.Close() - commitID, err := gitRepo.GetRefCommitID(pull.GetGitHeadRefName()) + headCommitID, err := gitRepo.GetRefCommitID(pull.GetGitHeadRefName()) if err != nil { log.Error("GetRefCommitID: %v", err) return } - AddToQueue(pull, commitID) + AddToQueue(pull, headCommitID) } diff --git a/services/convert/pull_review.go b/services/convert/pull_review.go index 3dcf4dea74505..755e416d7d50d 100644 --- a/services/convert/pull_review.go +++ b/services/convert/pull_review.go @@ -21,6 +21,10 @@ func ToPullReview(ctx context.Context, r *issues_model.Review, doer *user_model. r.Reviewer = user_model.NewGhostUser() } + if err := r.Issue.LoadRepo(ctx); err != nil { + return nil, err + } + result := &api.PullReview{ ID: r.ID, Reviewer: ToUser(ctx, r.Reviewer, doer), diff --git a/services/issue/issue.go b/services/issue/issue.go index 70a395eb12cee..332eb5c674ab9 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -204,6 +204,11 @@ func DeleteIssue(ctx context.Context, doer *user_model.User, gitRepo *git.Reposi if err := git.RemoveRef(ctx, gitRepo.Path, issue.PullRequest.GetGitHeadRefName()); err != nil { return err } + if issue.PullRequest.HasMerged { + if err := git.RemoveRef(ctx, gitRepo.Path, issue.PullRequest.GetGitMergeRefName()); err != nil { + return err + } + } } notify_service.DeleteIssue(ctx, doer, issue) diff --git a/services/issue/pull.go b/services/issue/pull.go index 512fdf78e84be..9dd818be39de6 100644 --- a/services/issue/pull.go +++ b/services/issue/pull.go @@ -7,33 +7,15 @@ import ( "context" "fmt" "slices" - "time" issues_model "code.gitea.io/gitea/models/issues" org_model "code.gitea.io/gitea/models/organization" user_model "code.gitea.io/gitea/models/user" - "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" ) -func getMergeBase(repo *git.Repository, pr *issues_model.PullRequest, baseBranch, headBranch string) (string, error) { - // Add a temporary remote - tmpRemote := fmt.Sprintf("mergebase-%d-%d", pr.ID, time.Now().UnixNano()) - if err := repo.AddRemote(tmpRemote, repo.Path, false); err != nil { - return "", fmt.Errorf("AddRemote: %w", err) - } - defer func() { - if err := repo.RemoveRemote(tmpRemote); err != nil { - log.Error("getMergeBase: RemoveRemote: %v", err) - } - }() - - mergeBase, _, err := repo.GetMergeBase(tmpRemote, baseBranch, headBranch) - return mergeBase, err -} - type ReviewRequestNotifier struct { Comment *issues_model.Comment IsAdd bool @@ -96,15 +78,9 @@ func PullRequestCodeOwnersReview(ctx context.Context, pr *issues_model.PullReque return nil, nil } - // get the mergebase - mergeBase, err := getMergeBase(repo, pr, git.BranchPrefix+pr.BaseBranch, pr.GetGitHeadRefName()) - if err != nil { - return nil, err - } - // https://github.com/go-gitea/gitea/issues/29763, we need to get the files changed // between the merge base and the head commit but not the base branch and the head commit - changedFiles, err := repo.GetFilesChangedBetween(mergeBase, pr.GetGitHeadRefName()) + changedFiles, err := repo.GetFilesChangedBetween(pr.MergeBase, pr.GetGitHeadRefName()) if err != nil { return nil, err } diff --git a/services/pull/merge.go b/services/pull/merge.go index cd9aeb2ad1ed5..1bb1b56de49b9 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -726,6 +726,12 @@ func SetMerged(ctx context.Context, pr *issues_model.PullRequest, mergedCommitID return false, issues_model.ErrIssueAlreadyChanged } + // update merge ref, this is necessary to ensure pr.MergedCommitID can be used to do diff operations even + // if the repository rebased/force-pushed and the pull request's merge commit is no longer in the history + if err := git.UpdateRef(ctx, pr.BaseRepo.RepoPath(), pr.GetGitMergeRefName(), pr.MergedCommitID); err != nil { + return false, fmt.Errorf("UpdateRef: %w", err) + } + if err := committer.Commit(); err != nil { return false, err } diff --git a/services/pull/review.go b/services/pull/review.go index 9f03f60c376fe..fc3fc5b917958 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -113,7 +113,12 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. defer closer.Close() } - prCommitIDs, err := git.CommitIDsBetween(ctx, gitRepo.Path, beforeCommitID, afterCommitID) + headCommitID, err := gitRepo.GetRefCommitID(issue.PullRequest.GetGitHeadRefName()) + if err != nil { + return nil, fmt.Errorf("GetRefCommitID[%s]: %w", issue.PullRequest.GetGitHeadRefName(), err) + } + + prCommitIDs, err := git.CommitIDsBetween(ctx, gitRepo.Path, issue.PullRequest.MergeBase, headCommitID) if err != nil { return nil, fmt.Errorf("CommitIDsBetween[%s, %s]: %w", beforeCommitID, afterCommitID, err) } @@ -138,10 +143,7 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. beforeCommitID = beforeCommit.ID.String() } if afterCommitID == "" { - afterCommitID, err = gitRepo.GetRefCommitID(issue.PullRequest.GetGitHeadRefName()) - if err != nil { - return nil, fmt.Errorf("GetRefCommitID[%s]: %w", issue.PullRequest.GetGitHeadRefName(), err) - } + afterCommitID = headCommitID } else if !slices.Contains(prCommitIDs, afterCommitID) { // afterCommitID must be one of the pull request commits return nil, fmt.Errorf("afterCommitID[%s] is not a valid pull request commit", afterCommitID) } @@ -520,7 +522,7 @@ func ReCalculateLineNumber(hunks []*git.HunkInfo, leftLine int64) int64 { } // FetchCodeCommentsByLine fetches the code comments for a given commit, treePath and line number of a pull request. -func FetchCodeCommentsByLine(ctx context.Context, repo *repo_model.Repository, issueID int64, currentUser *user_model.User, startCommitID, endCommitID, treePath string, line int64, showOutdatedComments bool) (issues_model.CommentList, error) { +func FetchCodeCommentsByLine(ctx context.Context, gitRepo *git.Repository, repo *repo_model.Repository, issueID int64, currentUser *user_model.User, startCommitID, endCommitID, treePath string, line int64, showOutdatedComments bool) (issues_model.CommentList, error) { opts := issues_model.FindCommentsOptions{ Type: issues_model.CommentTypeCode, IssueID: issueID, @@ -538,7 +540,24 @@ func FetchCodeCommentsByLine(ctx context.Context, repo *repo_model.Repository, i n := 0 hunksCache := make(map[string][]*git.HunkInfo) for _, comment := range comments { - if comment.CommitSHA == endCommitID { + // Code comment should always have a commit SHA, if not, we need to set it based on the line number + if comment.CommitSHA == "" { + if comment.Line > 0 { + comment.CommitSHA = endCommitID + } else if comment.Line < 0 { + comment.CommitSHA = startCommitID + } else { + // If the comment has no line number, we cannot display it in the diff view + continue + } + } + + dstCommitID := startCommitID + if comment.Line > 0 { + dstCommitID = endCommitID + } + + if comment.CommitSHA == dstCommitID { if comment.Line == line { comments[n] = comment n++ @@ -547,18 +566,27 @@ func FetchCodeCommentsByLine(ctx context.Context, repo *repo_model.Repository, i } // If the comment is not for the current commit, we need to recalculate the line number - hunks, ok := hunksCache[comment.CommitSHA] + hunks, ok := hunksCache[comment.CommitSHA+".."+dstCommitID] if !ok { - hunks, err = git.GetAffectedHunksForTwoCommitsSpecialFile(ctx, repo.RepoPath(), comment.CommitSHA, endCommitID, treePath) + hunks, err = git.GetAffectedHunksForTwoCommitsSpecialFile(ctx, repo.RepoPath(), comment.CommitSHA, dstCommitID, treePath) if err != nil { - return nil, fmt.Errorf("GetAffectedHunksForTwoCommitsSpecialFile[%s, %s, %s]: %w", repo.FullName(), comment.CommitSHA, endCommitID, err) + return nil, fmt.Errorf("GetAffectedHunksForTwoCommitsSpecialFile[%s, %s, %s]: %w", repo.FullName(), comment.CommitSHA, dstCommitID, err) } - hunksCache[comment.CommitSHA] = hunks + hunksCache[comment.CommitSHA+".."+dstCommitID] = hunks } comment.Line = ReCalculateLineNumber(hunks, comment.Line) - comments[n] = comment - n++ + if comment.Line != 0 { + dstCommit, err := gitRepo.GetCommit(dstCommitID) + if err != nil { + return nil, fmt.Errorf("GetCommit[%s]: %w", dstCommitID, err) + } + // If the comment is not the first one or the comment created before the current commit + if n > 0 || comment.CreatedUnix.AsTime().Before(dstCommit.Committer.When) { + comments[n] = comment + n++ + } + } } return comments[:n], nil } From e895298abaa20710539e5127f41168aacc3807b6 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 14 Jul 2025 21:19:35 -0700 Subject: [PATCH 09/35] Fix bug --- services/pull/merge.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/pull/merge.go b/services/pull/merge.go index 1bb1b56de49b9..d8c8d19416e51 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -728,7 +728,7 @@ func SetMerged(ctx context.Context, pr *issues_model.PullRequest, mergedCommitID // update merge ref, this is necessary to ensure pr.MergedCommitID can be used to do diff operations even // if the repository rebased/force-pushed and the pull request's merge commit is no longer in the history - if err := git.UpdateRef(ctx, pr.BaseRepo.RepoPath(), pr.GetGitMergeRefName(), pr.MergedCommitID); err != nil { + if err := git.UpdateRef(ctx, pr.Issue.Repo.RepoPath(), pr.GetGitMergeRefName(), pr.MergedCommitID); err != nil { return false, fmt.Errorf("UpdateRef: %w", err) } From c68718d1145e6f62ca4fb125f0e54f70eb287db3 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 14 Jul 2025 21:55:33 -0700 Subject: [PATCH 10/35] Fix test --- routers/private/hook_post_receive_test.go | 5 +++-- services/issue/issue_test.go | 5 +---- services/pull/review.go | 22 ++++++++++++++++++---- services/pull/review_test.go | 6 +++--- 4 files changed, 25 insertions(+), 13 deletions(-) diff --git a/routers/private/hook_post_receive_test.go b/routers/private/hook_post_receive_test.go index ca721b16d1b42..3d70552a3d12e 100644 --- a/routers/private/hook_post_receive_test.go +++ b/routers/private/hook_post_receive_test.go @@ -37,13 +37,14 @@ func TestHandlePullRequestMerging(t *testing.T) { PullRequestID: pr.ID, UserID: 2, }, pr.BaseRepo.OwnerName, pr.BaseRepo.Name, []*repo_module.PushUpdateOptions{ - {NewCommitID: "01234567"}, + // assume the first commit is merged from this pull request but it's not a real world scenario + {NewCommitID: "65f1bf27bc3bf70f64657658635e66094edbcb4d"}, }) assert.Empty(t, resp.Body.String()) pr, err = issues_model.GetPullRequestByID(db.DefaultContext, pr.ID) assert.NoError(t, err) assert.True(t, pr.HasMerged) - assert.Equal(t, "01234567", pr.MergedCommitID) + assert.Equal(t, "65f1bf27bc3bf70f64657658635e66094edbcb4d", pr.MergedCommitID) unittest.AssertNotExistsBean(t, &pull_model.AutoMerge{ID: autoMerge.ID}) } diff --git a/services/issue/issue_test.go b/services/issue/issue_test.go index bad0d65d1ed8f..0527192f9161b 100644 --- a/services/issue/issue_test.go +++ b/services/issue/issue_test.go @@ -39,10 +39,7 @@ func TestIssue_DeleteIssue(t *testing.T) { assert.NoError(t, err) assert.Len(t, issueIDs, 5) - issue := &issues_model.Issue{ - RepoID: 1, - ID: issueIDs[2], - } + issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: issueIDs[2]}) _, err = deleteIssue(db.DefaultContext, issue) assert.NoError(t, err) diff --git a/services/pull/review.go b/services/pull/review.go index fc3fc5b917958..a66317c2581d0 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -593,8 +593,11 @@ func FetchCodeCommentsByLine(ctx context.Context, gitRepo *git.Repository, repo // LoadCodeComments loads comments into each line, so that the comments can be displayed in the diff view. // the comments' line number is recalculated based on the hunks of the diff. -func LoadCodeComments(ctx context.Context, gitRepo *git.Repository, repo *repo_model.Repository, diff *gitdiff.Diff, issueID int64, currentUser *user_model.User, startCommit, endCommit *git.Commit, showOutdatedComments bool) error { - if startCommit == nil || endCommit == nil { +func LoadCodeComments(ctx context.Context, gitRepo *git.Repository, repo *repo_model.Repository, + diff *gitdiff.Diff, issueID int64, currentUser *user_model.User, + beforeCommit, afterCommit *git.Commit, showOutdatedComments bool, +) error { + if beforeCommit == nil || afterCommit == nil { return errors.New("startCommit and endCommit cannot be nil") } @@ -609,9 +612,20 @@ func LoadCodeComments(ctx context.Context, gitRepo *git.Repository, repo *repo_m hunksCache := make(map[string][]*git.HunkInfo) // filecomments should be sorted by created time, so that the latest comments are at the end for _, comment := range fileComments { - dstCommitID := startCommit.ID.String() + if comment.CommitSHA == "" { + if comment.Line > 0 { + comment.CommitSHA = afterCommit.ID.String() + } else if comment.Line < 0 { + comment.CommitSHA = beforeCommit.ID.String() + } else { + // If the comment has no line number, we cannot display it in the diff view + continue + } + } + + dstCommitID := beforeCommit.ID.String() if comment.Line > 0 { - dstCommitID = endCommit.ID.String() + dstCommitID = afterCommit.ID.String() } if comment.CommitSHA == dstCommitID { diff --git a/services/pull/review_test.go b/services/pull/review_test.go index 80cc78bcbe646..65abc0a277fab 100644 --- a/services/pull/review_test.go +++ b/services/pull/review_test.go @@ -82,12 +82,12 @@ func TestDiff_LoadCommentsNoOutdated(t *testing.T) { gitRepo, err := gitrepo.OpenRepository(t.Context(), issue.Repo) assert.NoError(t, err) defer gitRepo.Close() - startCommit, err := gitRepo.GetCommit(issue.PullRequest.MergeBase) + beforeCommit, err := gitRepo.GetCommit(issue.PullRequest.MergeBase) assert.NoError(t, err) - endCommit, err := gitRepo.GetCommit(issue.PullRequest.GetGitHeadRefName()) + afterCommit, err := gitRepo.GetCommit(issue.PullRequest.GetGitHeadRefName()) assert.NoError(t, err) - assert.NoError(t, pull_service.LoadCodeComments(db.DefaultContext, gitRepo, issue.Repo, diff, issue.ID, user, startCommit, endCommit, false)) + assert.NoError(t, pull_service.LoadCodeComments(db.DefaultContext, gitRepo, issue.Repo, diff, issue.ID, user, beforeCommit, afterCommit, false)) assert.Len(t, diff.Files[0].Sections[0].Lines[0].Comments, 2) } From 01476dbffd2c9bf71e2dcfcb9eafbf3f1de1b1d4 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 15 Jul 2025 12:49:07 -0700 Subject: [PATCH 11/35] Fix test --- models/issues/review.go | 4 ++++ services/convert/pull_review.go | 4 ++++ services/pull/review.go | 4 ++++ 3 files changed, 12 insertions(+) diff --git a/models/issues/review.go b/models/issues/review.go index 2102da28453bd..e388981fe63e3 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -432,6 +432,10 @@ func SubmitReview(ctx context.Context, doer *user_model.User, issue *Issue, revi defer committer.Close() sess := db.GetEngine(ctx) + if err := issue.LoadRepo(ctx); err != nil { + return nil, nil, fmt.Errorf("LoadRepo: %w", err) + } + official := false review, err := GetCurrentReview(ctx, doer, issue) diff --git a/services/convert/pull_review.go b/services/convert/pull_review.go index 755e416d7d50d..7fd343d2f0c3b 100644 --- a/services/convert/pull_review.go +++ b/services/convert/pull_review.go @@ -91,6 +91,10 @@ func ToPullReviewCommentList(ctx context.Context, review *issues_model.Review, d review.Reviewer = user_model.NewGhostUser() } + if err := review.Issue.LoadRepo(ctx); err != nil { + return nil, err + } + apiComments := make([]*api.PullReviewComment, 0, len(review.CodeComments)) for _, comments := range review.CodeComments { diff --git a/services/pull/review.go b/services/pull/review.go index a66317c2581d0..ee840cdd9c24f 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -113,6 +113,10 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. defer closer.Close() } + if err := issue.LoadPullRequest(ctx); err != nil { + return nil, fmt.Errorf("LoadPullRequest: %w", err) + } + headCommitID, err := gitRepo.GetRefCommitID(issue.PullRequest.GetGitHeadRefName()) if err != nil { return nil, fmt.Errorf("GetRefCommitID[%s]: %w", issue.PullRequest.GetGitHeadRefName(), err) From c58cc46382fc78bfb7d810dedc544b6907e78284 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 15 Jul 2025 16:35:58 -0700 Subject: [PATCH 12/35] Fix bug --- routers/web/repo/pull.go | 57 +++++++++++--------- routers/web/repo/pull_review.go | 26 ++------- services/pull/review.go | 31 ++--------- templates/repo/diff/box.tmpl | 2 +- web_src/js/components/DiffCommitSelector.vue | 29 +++++----- 5 files changed, 58 insertions(+), 87 deletions(-) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 581d97bd4f6eb..9c11a8453e5f4 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -653,46 +653,51 @@ func viewPullFiles(ctx *context.Context, beforeCommitID, afterCommitID string) { return } - ctx.Data["IsShowingOnlySingleCommit"] = beforeCommitID != "" && beforeCommitID == afterCommitID + isSingleCommit := beforeCommitID == "" && afterCommitID != "" + ctx.Data["IsShowingOnlySingleCommit"] = isSingleCommit isShowAllCommits := (beforeCommitID == "" || beforeCommitID == prInfo.MergeBase) && (afterCommitID == "" || afterCommitID == headCommitID) ctx.Data["IsShowingAllCommits"] = isShowAllCommits - if beforeCommitID == "" { - beforeCommitID = prInfo.MergeBase + var beforeCommit, afterCommit *git.Commit + if !isSingleCommit { + if beforeCommitID == "" || beforeCommitID == prInfo.MergeBase { + beforeCommitID = prInfo.MergeBase + // mergebase commit is not in the list of the pull request commits + beforeCommit, err = gitRepo.GetCommit(beforeCommitID) + if err != nil { + ctx.ServerError("GetCommit", err) + return + } + } else { + beforeCommit = indexCommit(prInfo.Commits, beforeCommitID) + if beforeCommit == nil { + ctx.HTTPError(http.StatusBadRequest, "before commit not found in PR commits") + return + } + } } - if afterCommitID == "" { + + if afterCommitID == "" || afterCommitID == headCommitID { afterCommitID = headCommitID } + afterCommit = indexCommit(prInfo.Commits, afterCommitID) + if afterCommit == nil { + ctx.HTTPError(http.StatusBadRequest, "after commit not found in PR commits") + return + } - var beforeCommit, afterCommit *git.Commit - if beforeCommitID != prInfo.MergeBase { - beforeCommit = indexCommit(prInfo.Commits, beforeCommitID) - if beforeCommit == nil { - ctx.NotFound(errors.New("before commit not found in PR commits")) - return - } - beforeCommit, err = beforeCommit.Parent(0) + if isSingleCommit { + beforeCommit, err = afterCommit.Parent(0) if err != nil { ctx.ServerError("GetParentCommit", err) return } beforeCommitID = beforeCommit.ID.String() - } else { // mergebase commit is not in the list of the pull request commits - beforeCommit, err = gitRepo.GetCommit(beforeCommitID) - if err != nil { - ctx.ServerError("GetCommit", err) - return - } - } - - afterCommit = indexCommit(prInfo.Commits, afterCommitID) - if afterCommit == nil { - ctx.NotFound(errors.New("after commit not found in PR commits")) - return } ctx.Data["Username"] = ctx.Repo.Owner.Name ctx.Data["Reponame"] = ctx.Repo.Repository.Name + ctx.Data["MergeBase"] = prInfo.MergeBase ctx.Data["AfterCommitID"] = afterCommitID ctx.Data["BeforeCommitID"] = beforeCommitID @@ -883,7 +888,9 @@ func viewPullFiles(ctx *context.Context, beforeCommitID, afterCommitID string) { } func ViewPullFilesForSingleCommit(ctx *context.Context) { - viewPullFiles(ctx, ctx.PathParam("sha"), ctx.PathParam("sha")) + // it doesn't support showing files from mergebase to the special commit + // otherwise it will be ambiguous + viewPullFiles(ctx, "", ctx.PathParam("sha")) } func ViewPullFilesForRange(ctx *context.Context) { diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index 29b436e9fc33d..fcb4b88776c45 100644 --- a/routers/web/repo/pull_review.go +++ b/routers/web/repo/pull_review.go @@ -178,7 +178,6 @@ func renderConversation(ctx *context.Context, comment *issues_model.Comment, ori ctx.ServerError("GetRefCommitID", err) return } - prCommitIDs, err := git.CommitIDsBetween(ctx, ctx.Repo.GitRepo.Path, comment.Issue.PullRequest.MergeBase, headCommitID) if err != nil { ctx.ServerError("CommitIDsBetween", err) @@ -187,27 +186,12 @@ func renderConversation(ctx *context.Context, comment *issues_model.Comment, ori if beforeCommitID == "" || beforeCommitID == comment.Issue.PullRequest.MergeBase { beforeCommitID = comment.Issue.PullRequest.MergeBase - } else { - // beforeCommitID must be one of the pull request commits - if !slices.Contains(prCommitIDs, beforeCommitID) { - ctx.HTTPError(http.StatusBadRequest, fmt.Sprintf("beforeCommitID[%s] is not a valid pull request commit", beforeCommitID)) - return - } - - beforeCommit, err := ctx.Repo.GitRepo.GetCommit(beforeCommitID) - if err != nil { - ctx.ServerError("GetCommit", err) - return - } - - beforeCommit, err = beforeCommit.Parent(0) - if err != nil { - ctx.ServerError("GetParent", err) - return - } - beforeCommitID = beforeCommit.ID.String() + } else if !slices.Contains(prCommitIDs, beforeCommitID) { // beforeCommitID must be one of the pull request commits + ctx.HTTPError(http.StatusBadRequest, fmt.Sprintf("beforeCommitID[%s] is not a valid pull request commit", beforeCommitID)) + return } - if afterCommitID == "" { + + if afterCommitID == "" || afterCommitID == headCommitID { afterCommitID = headCommitID } else if !slices.Contains(prCommitIDs, afterCommitID) { // afterCommitID must be one of the pull request commits ctx.HTTPError(http.StatusBadRequest, fmt.Sprintf("afterCommitID[%s] is not a valid pull request commit", afterCommitID)) diff --git a/services/pull/review.go b/services/pull/review.go index ee840cdd9c24f..d1b5ef86062fc 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -121,7 +121,6 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. if err != nil { return nil, fmt.Errorf("GetRefCommitID[%s]: %w", issue.PullRequest.GetGitHeadRefName(), err) } - prCommitIDs, err := git.CommitIDsBetween(ctx, gitRepo.Path, issue.PullRequest.MergeBase, headCommitID) if err != nil { return nil, fmt.Errorf("CommitIDsBetween[%s, %s]: %w", beforeCommitID, afterCommitID, err) @@ -129,24 +128,11 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. if beforeCommitID == "" || beforeCommitID == issue.PullRequest.MergeBase { beforeCommitID = issue.PullRequest.MergeBase - } else { - // beforeCommitID must be one of the pull request commits - if !slices.Contains(prCommitIDs, beforeCommitID) { - return nil, fmt.Errorf("beforeCommitID[%s] is not a valid pull request commit", beforeCommitID) - } - - beforeCommit, err := gitRepo.GetCommit(beforeCommitID) - if err != nil { - return nil, fmt.Errorf("GetCommit[%s]: %w", beforeCommitID, err) - } - - beforeCommit, err = beforeCommit.Parent(0) - if err != nil { - return nil, fmt.Errorf("GetParent[%s]: %w", beforeCommitID, err) - } - beforeCommitID = beforeCommit.ID.String() + } else if !slices.Contains(prCommitIDs, beforeCommitID) { // beforeCommitID must be one of the pull request commits + return nil, fmt.Errorf("beforeCommitID[%s] is not a valid pull request commit", beforeCommitID) } - if afterCommitID == "" { + + if afterCommitID == "" || afterCommitID == headCommitID { afterCommitID = headCommitID } else if !slices.Contains(prCommitIDs, afterCommitID) { // afterCommitID must be one of the pull request commits return nil, fmt.Errorf("afterCommitID[%s] is not a valid pull request commit", afterCommitID) @@ -648,14 +634,7 @@ func LoadCodeComments(ctx context.Context, gitRepo *git.Repository, repo *repo_m } comment.Line = ReCalculateLineNumber(hunks, comment.Line) if comment.Line != 0 { - dstCommit, err := gitRepo.GetCommit(dstCommitID) - if err != nil { - return fmt.Errorf("GetCommit[%s]: %w", dstCommitID, err) - } - // If the comment is not the first one or the comment created before the current commit - if len(lineComments[comment.Line]) > 0 || comment.CreatedUnix.AsTime().Before(dstCommit.Committer.When) { - lineComments[comment.Line] = append(lineComments[comment.Line], comment) - } + lineComments[comment.Line] = append(lineComments[comment.Line], comment) } } diff --git a/templates/repo/diff/box.tmpl b/templates/repo/diff/box.tmpl index ade1da655a0df..53e6053602b1f 100644 --- a/templates/repo/diff/box.tmpl +++ b/templates/repo/diff/box.tmpl @@ -35,7 +35,7 @@ {{template "repo/diff/whitespace_dropdown" .}} {{template "repo/diff/options_dropdown" .}} {{if .PageIsPullFiles}} -
+
{{/* the following will be replaced by vue component, but this avoids any loading artifacts till the vue component is initialized */}}
{{svg "octicon-git-commit"}} diff --git a/web_src/js/components/DiffCommitSelector.vue b/web_src/js/components/DiffCommitSelector.vue index 4b18694bd2dff..03d55e896b969 100644 --- a/web_src/js/components/DiffCommitSelector.vue +++ b/web_src/js/components/DiffCommitSelector.vue @@ -32,6 +32,7 @@ export default defineComponent({ locale: { filter_changes_by_commit: el.getAttribute('data-filter_changes_by_commit'), } as Record, + merge_base: el.getAttribute('data-merge-base'), commits: [] as Array, hoverActivated: false, lastReviewCommitSha: '', @@ -179,9 +180,6 @@ export default defineComponent({ * When a commit is clicked with shift this enables the range * selection. Second click (with shift) defines the end of the * range. This opens the diff of this range - * Exception: first commit is the first commit of this PR. Then - * the diff from beginning of PR up to the second clicked commit is - * opened */ commitClickedShift(commit: Commit) { this.hoverActivated = !this.hoverActivated; @@ -189,18 +187,21 @@ export default defineComponent({ // Second click -> determine our range and open links accordingly if (!this.hoverActivated) { // find all selected commits and generate a link - if (this.commits[0].selected) { - // first commit is selected - generate a short url with only target sha - const lastCommitIdx = this.commits.findLastIndex((x) => x.selected); - if (lastCommitIdx === this.commits.length - 1) { - // user selected all commits - just show the normal diff page - window.location.assign(`${this.issueLink}/files${this.queryParams}`); - } else { - window.location.assign(`${this.issueLink}/files/${this.commits[lastCommitIdx].id}${this.queryParams}`); - } + const firstSelected = this.commits.findIndex((x) => x.selected); + let start: string; + if (firstSelected === 0) { + start = this.merge_base; + } else { + start = this.commits[firstSelected - 1].id; + } + const end = this.commits.findLast((x) => x.selected).id; + if (start === end) { + // if the start and end are the same, we show this single commit + window.location.assign(`${this.issueLink}/commits/${start}${this.queryParams}`); + } else if (firstSelected === 0 && end === this.commits.at(-1).id) { + // if the first commit is selected and the last commit is selected, we show all commits + window.location.assign(`${this.issueLink}/files${this.queryParams}`); } else { - const start = this.commits[this.commits.findIndex((x) => x.selected) - 1].id; - const end = this.commits.findLast((x) => x.selected).id; window.location.assign(`${this.issueLink}/files/${start}..${end}${this.queryParams}`); } } From cc62a937729ab3d3d8b45db632476a8ef08f3037 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 15 Jul 2025 17:07:12 -0700 Subject: [PATCH 13/35] Remove unused router and test --- routers/web/repo/pull.go | 4 ---- routers/web/web.go | 1 - tests/integration/pull_diff_test.go | 4 ---- 3 files changed, 9 deletions(-) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 9c11a8453e5f4..5e7a67ac0c544 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -897,10 +897,6 @@ func ViewPullFilesForRange(ctx *context.Context) { viewPullFiles(ctx, ctx.PathParam("shaFrom"), ctx.PathParam("shaTo")) } -func ViewPullFilesStartingFromCommit(ctx *context.Context) { - viewPullFiles(ctx, ctx.PathParam("sha"), "") -} - func ViewPullFilesForAllCommitsOfPr(ctx *context.Context) { viewPullFiles(ctx, "", "") } diff --git a/routers/web/web.go b/routers/web/web.go index f8612db504dc5..5850b0fcaf56b 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -1540,7 +1540,6 @@ func registerWebRoutes(m *web.Router) { m.Post("/cleanup", context.RepoMustNotBeArchived(), repo.CleanUpPullRequest) m.Group("/files", func() { m.Get("", repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesForAllCommitsOfPr) - m.Get("/{sha:[a-f0-9]{7,40}}", repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesStartingFromCommit) m.Get("/{shaFrom:[a-f0-9]{7,40}}..{shaTo:[a-f0-9]{7,40}}", repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesForRange) m.Group("/reviews", func() { m.Get("/new_comment", repo.RenderNewCodeCommentForm) diff --git a/tests/integration/pull_diff_test.go b/tests/integration/pull_diff_test.go index 5411250935889..0b286fd2b2911 100644 --- a/tests/integration/pull_diff_test.go +++ b/tests/integration/pull_diff_test.go @@ -25,10 +25,6 @@ func TestPullDiff_CommitRangePRDiff(t *testing.T) { doTestPRDiff(t, "/user2/commitsonpr/pulls/1/files/4ca8bcaf27e28504df7bf996819665986b01c847..23576dd018294e476c06e569b6b0f170d0558705", true, []string{"test2.txt", "test3.txt", "test4.txt"}) } -func TestPullDiff_StartingFromBaseToCommitPRDiff(t *testing.T) { - doTestPRDiff(t, "/user2/commitsonpr/pulls/1/files/c5626fc9eff57eb1bb7b796b01d4d0f2f3f792a2", true, []string{"test1.txt", "test2.txt", "test3.txt"}) -} - func doTestPRDiff(t *testing.T, prDiffURL string, reviewBtnDisabled bool, expectedFilenames []string) { defer tests.PrepareTestEnv(t)() From c4f7e319d019a11409915a4823dfc7f1efbe8178 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 15 Jul 2025 21:59:27 -0700 Subject: [PATCH 14/35] Fix bug --- routers/web/repo/pull.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 5e7a67ac0c544..4050f75fc9b92 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -859,7 +859,7 @@ func viewPullFiles(ctx *context.Context, beforeCommitID, afterCommitID string) { ctx.Data["CanBlockUser"] = func(blocker, blockee *user_model.User) bool { return user_service.CanBlockUser(ctx, ctx.Doer, blocker, blockee) } - if !isShowAllCommits && pull.Flow == issues_model.PullRequestFlowGithub { + if isShowAllCommits && pull.Flow == issues_model.PullRequestFlowGithub { if err := pull.LoadHeadRepo(ctx); err != nil { ctx.ServerError("LoadHeadRepo", err) return From 64dec560dcc3b2c24bdc598c2d1d2fa62495a1b2 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 15 Jul 2025 22:36:42 -0700 Subject: [PATCH 15/35] Fix bug --- models/issues/comment.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/issues/comment.go b/models/issues/comment.go index a036b89d87cd5..72546b893717c 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -1158,7 +1158,7 @@ func DeleteComment(ctx context.Context, comment *Comment) error { // delete review if the comment is the last comment of the review if comment.ReviewID > 0 { if _, err := db.GetEngine(ctx).ID(comment.ReviewID). - Where("(SELECT count(id) FROM comment WHERE review_id = ?) == 0", comment.ReviewID). + Where("NOT EXISTS (SELECT 1 FROM comment WHERE review_id = ?)", comment.ReviewID). Delete(new(Review)); err != nil { return err } From e279f0e904197d6ad7278cfa6dcea505742b2c48 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 16 Jul 2025 11:21:44 -0700 Subject: [PATCH 16/35] Rename --- models/issues/comment.go | 2 +- services/issue/comments.go | 2 +- services/issue/issue.go | 2 +- services/pull/review.go | 4 ++-- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/models/issues/comment.go b/models/issues/comment.go index 72546b893717c..3d70a8c1bd2b1 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -764,7 +764,7 @@ func (c *Comment) CodeCommentLink(ctx context.Context) string { return fmt.Sprintf("%s/files#%s", c.Issue.Link(), c.HashTag()) } -func GetCodeCommentRef(prIndex, commentID int64) string { +func GetCodeCommentRefName(prIndex, commentID int64) string { return fmt.Sprintf("refs/pull/%d/code-comment-%d", prIndex, commentID) } diff --git a/services/issue/comments.go b/services/issue/comments.go index ba18099db7b8a..54d6a429cb01c 100644 --- a/services/issue/comments.go +++ b/services/issue/comments.go @@ -159,7 +159,7 @@ func DeleteComment(ctx context.Context, doer *user_model.User, comment *issues_m if err := comment.Issue.LoadPullRequest(ctx); err != nil { return err } - if err := git.RemoveRef(ctx, comment.Issue.Repo.RepoPath(), issues_model.GetCodeCommentRef(comment.Issue.PullRequest.Index, comment.ID)); err != nil { + if err := git.RemoveRef(ctx, comment.Issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(comment.Issue.PullRequest.Index, comment.ID)); err != nil { log.Error("Unable to remove ref in base repository for PR[%d] Error: %v", comment.Issue.PullRequest.ID, err) // We should not return error here, because the comment has been removed from database. // users have to delete this ref manually or we should have a synchronize between diff --git a/services/issue/issue.go b/services/issue/issue.go index 332eb5c674ab9..8df9bdc93076a 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -344,7 +344,7 @@ func deleteIssue(ctx context.Context, issue *issues_model.Issue) ([]string, erro } if comment.ReviewID > 0 { - if err := git.RemoveRef(ctx, issue.Repo.RepoPath(), issues_model.GetCodeCommentRef(issue.PullRequest.Index, comment.ID)); err != nil { + if err := git.RemoveRef(ctx, issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID)); err != nil { log.Error("Unable to remove ref in base repository for PR[%d] Error: %v", issue.PullRequest.ID, err) // We should not return error here, because the comment has been removed from database. // users have to delete this ref manually or we should have a synchronize between diff --git a/services/pull/review.go b/services/pull/review.go index d1b5ef86062fc..2fc9c917ac0d7 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -289,8 +289,8 @@ func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_mo } // The line commit ID Must be referenced in the git repository, because the branch maybe rebased or force-pushed. - // If the review commit is GC, the position will not be calculated dynamically. - if err := git.UpdateRef(ctx, pr.BaseRepo.RepoPath(), issues_model.GetCodeCommentRef(pr.Index, comment.ID), lineCommitID); err != nil { + // If the review commit is GC, the position can not be calculated dynamically. + if err := git.UpdateRef(ctx, pr.BaseRepo.RepoPath(), issues_model.GetCodeCommentRefName(pr.Index, comment.ID), lineCommitID); err != nil { log.Error("Unable to update ref in base repository for PR[%d] Error: %v", pr.ID, err) return nil, err } From aaed5c9332745562e3b0fdfa8397c0c934240e25 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 16 Jul 2025 13:59:27 -0700 Subject: [PATCH 17/35] remove the code extract to other pull requests --- models/issues/comment.go | 9 --------- services/issue/comments.go | 19 ------------------- 2 files changed, 28 deletions(-) diff --git a/models/issues/comment.go b/models/issues/comment.go index 3d70a8c1bd2b1..a3a3858a4d5d8 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -1155,15 +1155,6 @@ func DeleteComment(ctx context.Context, comment *Comment) error { return err } - // delete review if the comment is the last comment of the review - if comment.ReviewID > 0 { - if _, err := db.GetEngine(ctx).ID(comment.ReviewID). - Where("NOT EXISTS (SELECT 1 FROM comment WHERE review_id = ?)", comment.ReviewID). - Delete(new(Review)); err != nil { - return err - } - } - if err := comment.neuterCrossReferences(ctx); err != nil { return err } diff --git a/services/issue/comments.go b/services/issue/comments.go index 54d6a429cb01c..15e22af52aed9 100644 --- a/services/issue/comments.go +++ b/services/issue/comments.go @@ -17,7 +17,6 @@ import ( "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/json" "code.gitea.io/gitea/modules/log" - "code.gitea.io/gitea/modules/storage" "code.gitea.io/gitea/modules/timeutil" git_service "code.gitea.io/gitea/services/git" notify_service "code.gitea.io/gitea/services/notify" @@ -136,19 +135,10 @@ func UpdateComment(ctx context.Context, c *issues_model.Comment, contentVersion // DeleteComment deletes the comment func DeleteComment(ctx context.Context, doer *user_model.User, comment *issues_model.Comment) error { err := db.WithTx(ctx, func(ctx context.Context) error { - if err := comment.LoadAttachments(ctx); err != nil { - return err - } - if err := issues_model.DeleteComment(ctx, comment); err != nil { return err } - // delete comment attachments - if _, err := repo_model.DeleteAttachments(ctx, comment.Attachments, true); err != nil { - return fmt.Errorf("delete attachments: %w", err) - } - if comment.ReviewID > 0 { if err := comment.LoadIssue(ctx); err != nil { return err @@ -167,15 +157,6 @@ func DeleteComment(ctx context.Context, doer *user_model.User, comment *issues_m } } - for _, attachment := range comment.Attachments { - if err := storage.Attachments.Delete(repo_model.AttachmentRelativePath(attachment.UUID)); err != nil { - // Even delete files failed, but the attachments has been removed from database, so we - // should not return error but only record the error on logs. - // users have to delete this attachments manually or we should have a - // synchronize between database attachment table and attachment storage - log.Error("delete attachment[uuid: %s] failed: %v", attachment.UUID, err) - } - } return nil }) if err != nil { From 45931b3df3d14df38aa8b9f6b2aa5008d43d529b Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 18 Jul 2025 18:36:41 -0700 Subject: [PATCH 18/35] revert unnecessary change --- services/user/delete.go | 1 - 1 file changed, 1 deletion(-) diff --git a/services/user/delete.go b/services/user/delete.go index b8e51c550ff70..39c6ef052dca7 100644 --- a/services/user/delete.go +++ b/services/user/delete.go @@ -120,7 +120,6 @@ func deleteUser(ctx context.Context, u *user_model.User, purge bool) (err error) if err = issues_model.DeleteComment(ctx, comment); err != nil { return err } - // TODO: delete attachments } } From 1ecac1aaddb04c413c3608bb2208dda636ca9fff Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 18 Jul 2025 18:52:58 -0700 Subject: [PATCH 19/35] Don't use migrations but use a doctor to fix old negative line number of code comments --- models/migrations/migrations.go | 4 --- models/migrations/v1_25/v321.go | 14 ---------- services/doctor/review.go | 46 +++++++++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 18 deletions(-) delete mode 100644 models/migrations/v1_25/v321.go create mode 100644 services/doctor/review.go diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index e1a6a6f8c7903..176372486e8f6 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -24,7 +24,6 @@ import ( "code.gitea.io/gitea/models/migrations/v1_22" "code.gitea.io/gitea/models/migrations/v1_23" "code.gitea.io/gitea/models/migrations/v1_24" - "code.gitea.io/gitea/models/migrations/v1_25" "code.gitea.io/gitea/models/migrations/v1_6" "code.gitea.io/gitea/models/migrations/v1_7" "code.gitea.io/gitea/models/migrations/v1_8" @@ -383,9 +382,6 @@ func prepareMigrationTasks() []*migration { newMigration(318, "Add anonymous_access_mode for repo_unit", v1_24.AddRepoUnitAnonymousAccessMode), newMigration(319, "Add ExclusiveOrder to Label table", v1_24.AddExclusiveOrderColumnToLabelTable), newMigration(320, "Migrate two_factor_policy to login_source table", v1_24.MigrateSkipTwoFactor), - - // Gitea 1.24.0 ends at database version 321 - newMigration(321, "Migrate commit id of pull requests code review comment", v1_25.MigrateCommitIDOfPullRequestCodeReviewComment), } return preparedMigrations } diff --git a/models/migrations/v1_25/v321.go b/models/migrations/v1_25/v321.go deleted file mode 100644 index bfed6127db257..0000000000000 --- a/models/migrations/v1_25/v321.go +++ /dev/null @@ -1,14 +0,0 @@ -// Copyright 2025 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -package v1_25 - -import ( - "xorm.io/xorm" -) - -// MigrateCommitIDOfPullRequestCodeReviewComment this will be almost right before comment on the special commit of the pull request -func MigrateCommitIDOfPullRequestCodeReviewComment(x *xorm.Engine) error { - _, err := x.Exec("UPDATE comment SET commit_sha = (select merge_base from pull_request WHERE issue_id = comment.issue_id) WHERE line < 0") - return err -} diff --git a/services/doctor/review.go b/services/doctor/review.go new file mode 100644 index 0000000000000..790c0a475e060 --- /dev/null +++ b/services/doctor/review.go @@ -0,0 +1,46 @@ +// Copyright 2021 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package doctor + +import ( + "context" + + "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/modules/log" +) + +// checkCommitSHAOfPullRequestCodeReviewComment will check if the commit SHA of pull request code review comments +// For a comment with negative line number, it should be the merge base of the pull request if the comment is on the files page +// if it's on a special commit page or a range commit page, it should be the previous commit when reviewing that commit/commit range +// so that this may be broken for those comments submitted in a special commit(non the first one) page or a range commit page +// NOTICE: the fix can only be done once, so it should be run twice or more +func checkCommitSHAOfPullRequestCodeReviewComment(ctx context.Context, logger log.Logger, autofix bool) error { + count, err := db.GetEngine(ctx).SQL("SELECT 1 FROM comment where line < 0 AND commit_sha != (select merge_base from pull_request WHERE issue_id = comment.issue_id)").Count() + if err != nil { + logger.Critical("Error: %v whilst counting wrong comment commit sha", err) + return err + } + if count > 0 { + if autofix { + total, err := db.GetEngine(ctx).Exec("UPDATE comment SET commit_sha = (select merge_base from pull_request WHERE issue_id = comment.issue_id) WHERE line < 0") + if err != nil { + return err + } + logger.Info("%d comments with wrong commit sha fixed\nWARNING: This doctor can only fix this once, so it should NOT be run twice or more", total) + } else { + logger.Warn("%d comments with wrong commit sha exist", count) + } + } + return nil +} + +func init() { + Register(&Check{ + Title: "Check if comment with negative line number has wrong commit sha", + Name: "check-commitsha-review-comment", + IsDefault: true, + Run: checkCommitSHAOfPullRequestCodeReviewComment, + Priority: 3, + }) +} From 99b56344942ae36062d4567d1191d97828139c91 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 19 Jul 2025 16:06:10 -0700 Subject: [PATCH 20/35] Fix --- services/doctor/review.go | 2 +- services/issue/comments.go | 42 ++++--- services/issue/issue.go | 210 +++++++++++++++++------------------ services/issue/issue_test.go | 6 +- 4 files changed, 128 insertions(+), 132 deletions(-) diff --git a/services/doctor/review.go b/services/doctor/review.go index 790c0a475e060..0f7365b0e3ffd 100644 --- a/services/doctor/review.go +++ b/services/doctor/review.go @@ -1,4 +1,4 @@ -// Copyright 2021 The Gitea Authors. All rights reserved. +// Copyright 2025 The Gitea Authors. All rights reserved. // SPDX-License-Identifier: MIT package doctor diff --git a/services/issue/comments.go b/services/issue/comments.go index 15e22af52aed9..33934f05d7732 100644 --- a/services/issue/comments.go +++ b/services/issue/comments.go @@ -134,35 +134,33 @@ func UpdateComment(ctx context.Context, c *issues_model.Comment, contentVersion // DeleteComment deletes the comment func DeleteComment(ctx context.Context, doer *user_model.User, comment *issues_model.Comment) error { - err := db.WithTx(ctx, func(ctx context.Context) error { - if err := issues_model.DeleteComment(ctx, comment); err != nil { + if comment.ReviewID > 0 { + if err := comment.LoadIssue(ctx); err != nil { return err } - - if comment.ReviewID > 0 { - if err := comment.LoadIssue(ctx); err != nil { - return err - } - if err := comment.Issue.LoadRepo(ctx); err != nil { - return err - } - if err := comment.Issue.LoadPullRequest(ctx); err != nil { - return err - } - if err := git.RemoveRef(ctx, comment.Issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(comment.Issue.PullRequest.Index, comment.ID)); err != nil { - log.Error("Unable to remove ref in base repository for PR[%d] Error: %v", comment.Issue.PullRequest.ID, err) - // We should not return error here, because the comment has been removed from database. - // users have to delete this ref manually or we should have a synchronize between - // database comment table and git refs. - } + if err := comment.Issue.LoadRepo(ctx); err != nil { + return err } + if err := comment.Issue.LoadPullRequest(ctx); err != nil { + return err + } + } - return nil - }) - if err != nil { + if err := db.WithTx(ctx, func(ctx context.Context) error { + return issues_model.DeleteComment(ctx, comment) + }); err != nil { return err } + if comment.ReviewID > 0 { + if err := git.RemoveRef(ctx, comment.Issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(comment.Issue.PullRequest.Index, comment.ID)); err != nil { + log.Error("Unable to remove ref in base repository for PR[%d] Error: %v", comment.Issue.PullRequest.ID, err) + // We should not return error here, because the comment has been removed from database. + // users have to delete this ref manually or we should have a synchronize between + // database comment table and git refs. + } + } + notify_service.DeleteComment(ctx, doer, comment) return nil diff --git a/services/issue/issue.go b/services/issue/issue.go index 8df9bdc93076a..c1b9a213e285d 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -191,7 +191,7 @@ func DeleteIssue(ctx context.Context, doer *user_model.User, gitRepo *git.Reposi } // delete entries in database - attachmentPaths, err := deleteIssue(ctx, issue) + attachmentPaths, comments, err := deleteIssue(ctx, issue) if err != nil { return err } @@ -211,6 +211,17 @@ func DeleteIssue(ctx context.Context, doer *user_model.User, gitRepo *git.Reposi } } + for _, comment := range comments { + if comment.ReviewID > 0 { + if err := git.RemoveRef(ctx, issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID)); err != nil { + log.Error("Unable to remove ref in base repository for PR[%d] Error: %v", issue.PullRequest.ID, err) + // We should not return error here, because the comment has been removed from database. + // users have to delete this ref manually or we should have a synchronize between + // database comment table and git refs. + } + } + } + notify_service.DeleteIssue(ctx, doer, issue) return nil @@ -266,131 +277,107 @@ func GetRefEndNamesAndURLs(issues []*issues_model.Issue, repoLink string) (map[i } // deleteIssue deletes the issue -func deleteIssue(ctx context.Context, issue *issues_model.Issue) ([]string, error) { - ctx, committer, err := db.TxContext(ctx) - if err != nil { - return nil, err - } - defer committer.Close() - - // update the total issue numbers - if err := repo_model.UpdateRepoIssueNumbers(ctx, issue.RepoID, issue.IsPull, false); err != nil { - return nil, err - } - // if the issue is closed, update the closed issue numbers - if issue.IsClosed { - if err := repo_model.UpdateRepoIssueNumbers(ctx, issue.RepoID, issue.IsPull, true); err != nil { - return nil, err +func deleteIssue(ctx context.Context, issue *issues_model.Issue) ([]string, []*issues_model.Comment, error) { + var attachmentPaths []string + if err := db.WithTx(ctx, func(ctx context.Context) error { + // update the total issue numbers + if err := repo_model.UpdateRepoIssueNumbers(ctx, issue.RepoID, issue.IsPull, false); err != nil { + return err + } + // if the issue is closed, update the closed issue numbers + if issue.IsClosed { + if err := repo_model.UpdateRepoIssueNumbers(ctx, issue.RepoID, issue.IsPull, true); err != nil { + return err + } } - } - - if err := issues_model.UpdateMilestoneCounters(ctx, issue.MilestoneID); err != nil { - return nil, fmt.Errorf("error updating counters for milestone id %d: %w", - issue.MilestoneID, err) - } - if err := activities_model.DeleteIssueActions(ctx, issue.RepoID, issue.ID, issue.Index); err != nil { - return nil, err - } + if err := issues_model.UpdateMilestoneCounters(ctx, issue.MilestoneID); err != nil { + return fmt.Errorf("error updating counters for milestone id %d: %w", + issue.MilestoneID, err) + } - // find attachments related to this issue and remove them - if err := issue.LoadAttachments(ctx); err != nil { - return nil, err - } + if err := activities_model.DeleteIssueActions(ctx, issue.RepoID, issue.ID, issue.Index); err != nil { + return err + } - var attachmentPaths []string - for i := range issue.Attachments { - attachmentPaths = append(attachmentPaths, issue.Attachments[i].RelativePath()) - } + // find attachments related to this issue and remove them + if err := issue.LoadAttachments(ctx); err != nil { + return err + } - // deference all review comments - if err := issue.LoadRepo(ctx); err != nil { - return nil, err - } - if err := issue.LoadPullRequest(ctx); err != nil { - return nil, err - } + for i := range issue.Attachments { + attachmentPaths = append(attachmentPaths, issue.Attachments[i].RelativePath()) + } - if err := issue.LoadComments(ctx); err != nil { - return nil, err - } + // deference all review comments + if err := issue.LoadRepo(ctx); err != nil { + return err + } + if err := issue.LoadPullRequest(ctx); err != nil { + return err + } - // delete all database data still assigned to this issue - if err := db.DeleteBeans(ctx, - &issues_model.ContentHistory{IssueID: issue.ID}, - &issues_model.IssueLabel{IssueID: issue.ID}, - &issues_model.IssueDependency{IssueID: issue.ID}, - &issues_model.IssueAssignees{IssueID: issue.ID}, - &issues_model.IssueUser{IssueID: issue.ID}, - &activities_model.Notification{IssueID: issue.ID}, - &issues_model.Reaction{IssueID: issue.ID}, - &issues_model.IssueWatch{IssueID: issue.ID}, - &issues_model.Stopwatch{IssueID: issue.ID}, - &issues_model.TrackedTime{IssueID: issue.ID}, - &project_model.ProjectIssue{IssueID: issue.ID}, - &repo_model.Attachment{IssueID: issue.ID}, - &issues_model.PullRequest{IssueID: issue.ID}, - &issues_model.Comment{RefIssueID: issue.ID}, - &issues_model.IssueDependency{DependencyID: issue.ID}, - &issues_model.Comment{DependentIssueID: issue.ID}, - &issues_model.IssuePin{IssueID: issue.ID}, - ); err != nil { - return nil, err - } + if err := issue.LoadComments(ctx); err != nil { + return err + } - for _, comment := range issue.Comments { - if err := issues_model.DeleteComment(ctx, comment); err != nil { - return nil, err + // delete all database data still assigned to this issue + if err := db.DeleteBeans(ctx, + &issues_model.ContentHistory{IssueID: issue.ID}, + &issues_model.IssueLabel{IssueID: issue.ID}, + &issues_model.IssueDependency{IssueID: issue.ID}, + &issues_model.IssueAssignees{IssueID: issue.ID}, + &issues_model.IssueUser{IssueID: issue.ID}, + &activities_model.Notification{IssueID: issue.ID}, + &issues_model.Reaction{IssueID: issue.ID}, + &issues_model.IssueWatch{IssueID: issue.ID}, + &issues_model.Stopwatch{IssueID: issue.ID}, + &issues_model.TrackedTime{IssueID: issue.ID}, + &project_model.ProjectIssue{IssueID: issue.ID}, + &repo_model.Attachment{IssueID: issue.ID}, + &issues_model.PullRequest{IssueID: issue.ID}, + &issues_model.Comment{RefIssueID: issue.ID}, + &issues_model.IssueDependency{DependencyID: issue.ID}, + &issues_model.Comment{DependentIssueID: issue.ID}, + &issues_model.IssuePin{IssueID: issue.ID}, + ); err != nil { + return err } - if comment.ReviewID > 0 { - if err := git.RemoveRef(ctx, issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID)); err != nil { - log.Error("Unable to remove ref in base repository for PR[%d] Error: %v", issue.PullRequest.ID, err) - // We should not return error here, because the comment has been removed from database. - // users have to delete this ref manually or we should have a synchronize between - // database comment table and git refs. + for _, comment := range issue.Comments { + if err := issues_model.DeleteComment(ctx, comment); err != nil { + return err } } - // delete all attachments related to this comment - for _, attachment := range comment.Attachments { - if err := storage.Attachments.Delete(repo_model.AttachmentRelativePath(attachment.UUID)); err != nil { - // Even delete files failed, but the attachments has been removed from database, so we - // should not return error but only record the error on logs. - // users have to delete this attachments manually or we should have a - // synchronize between database attachment table and attachment storage - log.Error("delete attachment[uuid: %s] failed: %v", attachment.UUID, err) + // delete all pull request records + if issue.IsPull { + // Delete scheduled auto merges + if _, err := db.GetEngine(ctx).Where("pull_id=?", issue.PullRequest.ID). + Delete(&pull_model.AutoMerge{}); err != nil { + return err } - } - } - // delete all pull request records - if issue.IsPull { - // Delete scheduled auto merges - if _, err := db.GetEngine(ctx).Where("pull_id=?", issue.PullRequest.ID). - Delete(&pull_model.AutoMerge{}); err != nil { - return nil, err - } + // Delete review states + if _, err := db.GetEngine(ctx).Where("pull_id=?", issue.PullRequest.ID). + Delete(&pull_model.ReviewState{}); err != nil { + return err + } - // Delete review states - if _, err := db.GetEngine(ctx).Where("pull_id=?", issue.PullRequest.ID). - Delete(&pull_model.ReviewState{}); err != nil { - return nil, err + if _, err := db.GetEngine(ctx).ID(issue.PullRequest.ID).Delete(&issues_model.PullRequest{}); err != nil { + return err + } } - if _, err := db.GetEngine(ctx).ID(issue.PullRequest.ID).Delete(&issues_model.PullRequest{}); err != nil { - return nil, err + if _, err := db.GetEngine(ctx).ID(issue.ID).NoAutoCondition().Delete(issue); err != nil { + return err } + return nil + }); err != nil { + return nil, nil, err } - if _, err := db.GetEngine(ctx).ID(issue.ID).NoAutoCondition().Delete(issue); err != nil { - return nil, err - } - - if err := committer.Commit(); err != nil { - return nil, err - } - return attachmentPaths, nil + return attachmentPaths, issue.Comments, nil } // DeleteOrphanedIssues delete issues without a repo @@ -438,11 +425,22 @@ func DeleteIssuesByRepoID(ctx context.Context, repoID int64) (attachmentPaths [] } for _, issue := range issues { - issueAttachPaths, err := deleteIssue(ctx, issue) + issueAttachPaths, comments, err := deleteIssue(ctx, issue) if err != nil { return nil, fmt.Errorf("deleteIssue [issue_id: %d]: %w", issue.ID, err) } + for _, comment := range comments { + if comment.ReviewID > 0 { + if err := git.RemoveRef(ctx, issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID)); err != nil { + log.Error("Unable to remove ref in base repository for PR[%d] Error: %v", issue.PullRequest.ID, err) + // We should not return error here, because the comment has been removed from database. + // users have to delete this ref manually or we should have a synchronize between + // database comment table and git refs. + } + } + } + attachmentPaths = append(attachmentPaths, issueAttachPaths...) } } diff --git a/services/issue/issue_test.go b/services/issue/issue_test.go index 0527192f9161b..920a8e5ace7f0 100644 --- a/services/issue/issue_test.go +++ b/services/issue/issue_test.go @@ -41,7 +41,7 @@ func TestIssue_DeleteIssue(t *testing.T) { issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: issueIDs[2]}) - _, err = deleteIssue(db.DefaultContext, issue) + _, _, err = deleteIssue(db.DefaultContext, issue) assert.NoError(t, err) issueIDs, err = issues_model.GetIssueIDsByRepoID(db.DefaultContext, 1) assert.NoError(t, err) @@ -52,7 +52,7 @@ func TestIssue_DeleteIssue(t *testing.T) { assert.NoError(t, err) issue, err = issues_model.GetIssueByID(db.DefaultContext, 4) assert.NoError(t, err) - _, err = deleteIssue(db.DefaultContext, issue) + _, _, err = deleteIssue(db.DefaultContext, issue) assert.NoError(t, err) assert.Len(t, attachments, 2) for i := range attachments { @@ -75,7 +75,7 @@ func TestIssue_DeleteIssue(t *testing.T) { assert.NoError(t, err) assert.False(t, left) - _, err = deleteIssue(db.DefaultContext, issue2) + _, _, err = deleteIssue(db.DefaultContext, issue2) assert.NoError(t, err) left, err = issues_model.IssueNoDependenciesLeft(db.DefaultContext, issue1) assert.NoError(t, err) From be18643bfd88b599ae0a966ad10ba1629d796c8b Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 19 Jul 2025 17:55:54 -0700 Subject: [PATCH 21/35] Add test and comment --- modules/git/diff_test.go | 12 ++++++++++++ modules/git/repo_commit.go | 3 +++ 2 files changed, 15 insertions(+) diff --git a/modules/git/diff_test.go b/modules/git/diff_test.go index 8e618d354ad0d..4f124648af0a0 100644 --- a/modules/git/diff_test.go +++ b/modules/git/diff_test.go @@ -194,3 +194,15 @@ func TestParseDiffHunkString(t *testing.T) { assert.Equal(t, 2, rightLine) assert.Equal(t, 1, rightHunk) } + +func Test_GetAffectedHunksForTwoCommitsSpecialFile(t *testing.T) { + repoPath := "./tests/repos/repo4_commitsbetween" + hunks, err := GetAffectedHunksForTwoCommitsSpecialFile(t.Context(), repoPath, "fdc1b615bdcff0f0658b216df0c9209e5ecb7c78", "a78e5638b66ccfe7e1b4689d3d5684e42c97d7ca", "test.txt") + assert.NoError(t, err) + assert.Len(t, hunks, 1) + // @@ -1 +1 @@ + assert.Equal(t, int64(1), hunks[0].LeftLine) + assert.Equal(t, int64(1), hunks[0].LeftHunk) + assert.Equal(t, int64(1), hunks[0].RightLine) + assert.Equal(t, int64(1), hunks[0].RightHunk) +} diff --git a/modules/git/repo_commit.go b/modules/git/repo_commit.go index 9ccb13b5a4e0c..5c74d077fde97 100644 --- a/modules/git/repo_commit.go +++ b/modules/git/repo_commit.go @@ -322,6 +322,9 @@ func (repo *Repository) CommitsBetween(lastCommitID, beforeCommitID string) ([]* return commits, nil } +// CommitIDsBetween returns a list that contains commit IDs between (beforeCommitID, afterCommitID]. +// If beforeCommitID is empty, it will return all commits before afterCommitID. +// If beforeCommitID is given, it will not be included in the result. func CommitIDsBetween(ctx context.Context, repoPath, beforeCommitID, afterCommitID string) ([]string, error) { var stdout []byte var err error From 68e5b28e7df1b608d93cfc19c8a7233a3d60da37 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 19 Jul 2025 18:02:28 -0700 Subject: [PATCH 22/35] add test --- modules/git/diff_test.go | 3 ++- modules/git/repo_commit_test.go | 23 +++++++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/modules/git/diff_test.go b/modules/git/diff_test.go index 4f124648af0a0..4ff5763bc0025 100644 --- a/modules/git/diff_test.go +++ b/modules/git/diff_test.go @@ -4,6 +4,7 @@ package git import ( + "path/filepath" "strings" "testing" @@ -196,7 +197,7 @@ func TestParseDiffHunkString(t *testing.T) { } func Test_GetAffectedHunksForTwoCommitsSpecialFile(t *testing.T) { - repoPath := "./tests/repos/repo4_commitsbetween" + repoPath := filepath.Join(testReposDir, "repo4_commitsbetween") hunks, err := GetAffectedHunksForTwoCommitsSpecialFile(t.Context(), repoPath, "fdc1b615bdcff0f0658b216df0c9209e5ecb7c78", "a78e5638b66ccfe7e1b4689d3d5684e42c97d7ca", "test.txt") assert.NoError(t, err) assert.Len(t, hunks, 1) diff --git a/modules/git/repo_commit_test.go b/modules/git/repo_commit_test.go index e9f469accdf0c..cf964e66d9b2c 100644 --- a/modules/git/repo_commit_test.go +++ b/modules/git/repo_commit_test.go @@ -148,3 +148,26 @@ func TestCommitsByFileAndRange(t *testing.T) { require.NoError(t, err) assert.Len(t, commits, 1) } + +func Test_CommitIDsBetween(t *testing.T) { + defer test.MockVariableValue(&setting.Git.CommitsRangeSize, 2)() + + bareRepo1Path := filepath.Join(testReposDir, "repo1_bare") + bareRepo1, err := openRepositoryWithDefaultContext(bareRepo1Path) + require.NoError(t, err) + defer bareRepo1.Close() + + // Test with empty beforeCommitID + commitIDs, err := CommitIDsBetween(bareRepo1.Ctx, bareRepo1.Path, "", "master") + require.NoError(t, err) + assert.Len(t, commitIDs, 7) + assert.Equal(t, "ce064814f4a0d337b333e646ece456cd39fab612", commitIDs[0]) + assert.Equal(t, "95bb4d39648ee7e325106df01a621c530863a653", commitIDs[6]) + + // Test with a specific beforeCommitID + commitIDs, err = CommitIDsBetween(bareRepo1.Ctx, bareRepo1.Path, "37991dec2c8e592043f47155ce4808d4580f9123", "master") + require.NoError(t, err) + assert.Len(t, commitIDs, 2) + assert.Equal(t, "ce064814f4a0d337b333e646ece456cd39fab612", commitIDs[0]) + assert.Equal(t, "feaf4ba6bc635fec442f46ddd4512416ec43c2c2", commitIDs[1]) +} From cbd73a59cce795b2654bd6a5eadb1a2ee0fa16be Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 20 Jul 2025 10:54:39 -0700 Subject: [PATCH 23/35] add test --- tests/integration/pull_review_test.go | 252 ++++++++++++++++++++++++++ 1 file changed, 252 insertions(+) diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 1121751c88bc7..6fe6f4b47c404 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -4,6 +4,7 @@ package integration import ( + "fmt" "net/http" "net/http/httptest" "net/url" @@ -17,6 +18,7 @@ import ( "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/test" issue_service "code.gitea.io/gitea/services/issue" repo_service "code.gitea.io/gitea/services/repository" @@ -276,3 +278,253 @@ func testIssueClose(t *testing.T, session *TestSession, owner, repo, issueNumber req = NewRequestWithValues(t, "POST", closeURL, options) return session.MakeRequest(t, req, http.StatusOK) } + +func Test_ReviewCodeComment(t *testing.T) { + onGiteaRun(t, func(t *testing.T, u *url.URL) { + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + + // Create the repo. + repo, err := repo_service.CreateRepositoryDirectly(db.DefaultContext, user2, user2, repo_service.CreateRepoOptions{ + Name: "test_codecomment", + Readme: "Default", + AutoInit: true, + ObjectFormatName: git.Sha1ObjectFormat.Name(), + DefaultBranch: "master", + }, true) + assert.NoError(t, err) + + // add README.md to default branch + _, err = files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{ + OldBranch: repo.DefaultBranch, + Files: []*files_service.ChangeRepoFile{ + { + Operation: "update", + TreePath: "README.md", + ContentReader: strings.NewReader("# 111\n# 222\n"), + }, + }, + }) + assert.NoError(t, err) + + var pr *issues_model.PullRequest + t.Run("Create Pull Request", func(t *testing.T) { + // create a new branch to prepare for pull request + _, err = files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{ + NewBranch: "branch_codecomment1", + Files: []*files_service.ChangeRepoFile{ + { + Operation: "update", + TreePath: "README.md", + // add 5 lines to the file + ContentReader: strings.NewReader("# 111\n# 222\n# 333\n# 444\n# 555\n# 666\n# 777\n"), + }, + }, + }) + assert.NoError(t, err) + + // Create a pull request. + session := loginUser(t, "user2") + testPullCreate(t, session, "user2", "test_codecomment", false, repo.DefaultBranch, "branch_codecomment1", "Test Pull Request1") + + pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "branch_codecomment1"}) + }) + + t.Run("Create Code Comment", func(t *testing.T) { + session := loginUser(t, "user2") + + // Grab the CSRF token. + req := NewRequest(t, "GET", path.Join("user2", "test_codecomment", "pulls", "1")) + resp := session.MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + + // Create a code comment on the pull request. + commentURL := fmt.Sprintf("/user2/test_codecomment/pulls/%d/files/reviews/comments", pr.Index) + options := map[string]string{ + "_csrf": htmlDoc.GetCSRF(), + "origin": "diff", + "content": "code comment on right line 4", + "side": "proposed", + "line": "4", + "path": "README.md", + "single_review": "true", + "reply": "0", + "before_commit_id": "", + "after_commit_id": "", + } + req = NewRequestWithValues(t, "POST", commentURL, options) + session.MakeRequest(t, req, http.StatusOK) + + // Check if the comment was created. + comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ + Type: issues_model.CommentTypeCode, + IssueID: pr.IssueID, + }) + assert.Equal(t, "code comment on right line 4", comment.Content) + assert.Equal(t, "README.md", comment.TreePath) + assert.Equal(t, int64(4), comment.Line) + gitRepo, err := gitrepo.OpenRepository(t.Context(), repo) + assert.NoError(t, err) + defer gitRepo.Close() + commitID, err := gitRepo.GetRefCommitID(pr.GetGitHeadRefName()) + assert.NoError(t, err) + assert.Equal(t, commitID, comment.CommitSHA) + + // load the files page and confirm the comment is there + filesPageURL := fmt.Sprintf("/user2/test_codecomment/pulls/%d/files", pr.Index) + req = NewRequest(t, "GET", filesPageURL) + resp = session.MakeRequest(t, req, http.StatusOK) + htmlDoc = NewHTMLParser(t, resp.Body) + commentHTML := htmlDoc.Find(fmt.Sprintf("#issuecomment-%d", comment.ID)) + assert.NotNil(t, commentHTML) + assert.Equal(t, "code comment on right line 4", strings.TrimSpace(commentHTML.Find(".comment-body .render-content").Text())) + + // the last line of this comment line number is 4 + parentTr := commentHTML.ParentsFiltered("tr").First() + assert.NotNil(t, parentTr) + previousTr := parentTr.PrevAllFiltered("tr").First() + val, _ := previousTr.Attr("data-line-type") + assert.Equal(t, "add", val) + td := previousTr.Find("td.lines-num-new") + val, _ = td.Attr("data-line-num") + assert.Equal(t, "4", val) + }) + + t.Run("Pushing new commit to the pull request to add lines", func(t *testing.T) { + // create a new branch to prepare for pull request + _, err = files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{ + OldBranch: "branch_codecomment1", + Files: []*files_service.ChangeRepoFile{ + { + Operation: "update", + TreePath: "README.md", + // add 1 line before the code comment line 4 + ContentReader: strings.NewReader("# 111\n# 222\n# 333\n# 334\n# 444\n# 555\n# 666\n# 777\n"), + }, + }, + }) + assert.NoError(t, err) + + session := loginUser(t, "user2") + comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ + Type: issues_model.CommentTypeCode, + IssueID: pr.IssueID, + }) + + // load the files page and confirm the comment's line number is dynamically adjusted + filesPageURL := fmt.Sprintf("/user2/test_codecomment/pulls/%d/files", pr.Index) + req := NewRequest(t, "GET", filesPageURL) + resp := session.MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + commentHTML := htmlDoc.Find(fmt.Sprintf("#issuecomment-%d", comment.ID)) + assert.NotNil(t, commentHTML) + assert.Equal(t, "code comment on right line 4", strings.TrimSpace(commentHTML.Find(".comment-body .render-content").Text())) + + // the last line of this comment line number is 4 + parentTr := commentHTML.ParentsFiltered("tr").First() + assert.NotNil(t, parentTr) + previousTr := parentTr.PrevAllFiltered("tr").First() + val, _ := previousTr.Attr("data-line-type") + assert.Equal(t, "add", val) + td := previousTr.Find("td.lines-num-new") + val, _ = td.Attr("data-line-num") + assert.Equal(t, "5", val) // one line have inserted in this commit, so the line number should be 5 now + }) + + t.Run("Pushing new commit to the pull request to delete lines", func(t *testing.T) { + // create a new branch to prepare for pull request + _, err := files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{ + OldBranch: "branch_codecomment1", + Files: []*files_service.ChangeRepoFile{ + { + Operation: "update", + TreePath: "README.md", + // delete the second line before the code comment line 4 + ContentReader: strings.NewReader("# 111\n# 333\n# 334\n# 444\n# 555\n# 666\n# 777\n"), + }, + }, + }) + assert.NoError(t, err) + + diffContent, _, err := git.NewCommand("diff").AddDynamicArguments(pr.MergeBase, pr.GetGitHeadRefName()).RunStdString( + t.Context(), &git.RunOpts{ + Dir: repo.RepoPath(), + }, + ) + assert.NoError(t, err) + fmt.Println("=======") + fmt.Println(diffContent) + fmt.Println("=======") + + session := loginUser(t, "user2") + comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ + Type: issues_model.CommentTypeCode, + IssueID: pr.IssueID, + }) + + // load the files page and confirm the comment's line number is dynamically adjusted + filesPageURL := fmt.Sprintf("/user2/test_codecomment/pulls/%d/files", pr.Index) + req := NewRequest(t, "GET", filesPageURL) + resp := session.MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + commentHTML := htmlDoc.Find(fmt.Sprintf("#issuecomment-%d", comment.ID)) + assert.NotNil(t, commentHTML) + assert.Equal(t, "code comment on right line 4", strings.TrimSpace(commentHTML.Find(".comment-body .render-content").Text())) + + // the last line of this comment line number is 4 + parentTr := commentHTML.ParentsFiltered("tr").First() + assert.NotNil(t, parentTr) + previousTr := parentTr.PrevAllFiltered("tr").First() + val, _ := previousTr.Attr("data-line-type") + assert.Equal(t, "add", val) + td := previousTr.Find("td.lines-num-new") + val, _ = td.Attr("data-line-num") + assert.Equal(t, "4", val) // one line have inserted and one line deleted before this line in this commit, so the line number should be 4 now + + // add a new comment on the deleted line + commentURL := fmt.Sprintf("/user2/test_codecomment/pulls/%d/files/reviews/comments", pr.Index) + options := map[string]string{ + "_csrf": htmlDoc.GetCSRF(), + "origin": "diff", + "content": "code comment on left line 2", + "side": "previous", + "line": "2", + "path": "README.md", + "single_review": "true", + "reply": "0", + "before_commit_id": "", + "after_commit_id": "", + } + req = NewRequestWithValues(t, "POST", commentURL, options) + session.MakeRequest(t, req, http.StatusOK) + // Check if the comment was created. + commentLast := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ + Type: issues_model.CommentTypeCode, + IssueID: pr.IssueID, + Content: "code comment on left line 2", + }) + assert.Equal(t, "code comment on left line 2", commentLast.Content) + assert.Equal(t, "README.md", commentLast.TreePath) + assert.Equal(t, int64(-2), commentLast.Line) + assert.Equal(t, pr.MergeBase, commentLast.CommitSHA) + + // load the files page and confirm the comment's line number is dynamically adjusted + filesPageURL = fmt.Sprintf("/user2/test_codecomment/pulls/%d/files", pr.Index) + req = NewRequest(t, "GET", filesPageURL) + resp = session.MakeRequest(t, req, http.StatusOK) + htmlDoc = NewHTMLParser(t, resp.Body) + commentHTML = htmlDoc.Find(fmt.Sprintf("#issuecomment-%d", commentLast.ID)) + assert.NotNil(t, commentHTML) + assert.Equal(t, "code comment on left line 2", strings.TrimSpace(commentHTML.Find(".comment-body .render-content").Text())) + + // the last line of this comment line number is 4 + parentTr = commentHTML.ParentsFiltered("tr").First() + assert.NotNil(t, parentTr) + previousTr = parentTr.PrevAllFiltered("tr").First() + val, _ = previousTr.Attr("data-line-type") + assert.Equal(t, "del", val) + td = previousTr.Find("td.lines-num-old") + val, _ = td.Attr("data-line-num") + assert.Equal(t, "2", val) + }) + }) +} From 155691146889cd0fb5be52f93463b36f4c2f5058 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 21 Jul 2025 17:02:39 -0700 Subject: [PATCH 24/35] Fix test --- modules/git/diff.go | 2 +- tests/integration/pull_review_test.go | 10 ---------- 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/modules/git/diff.go b/modules/git/diff.go index 34307f7768eee..505a3e8ff538d 100644 --- a/modules/git/diff.go +++ b/modules/git/diff.go @@ -385,7 +385,7 @@ func GetAffectedHunksForTwoCommitsSpecialFile(ctx context.Context, repoPath, old } // Parse the hunk header leftLine, leftHunk, rightLine, rightHunk := ParseDiffHunkString(lof) - hunks = append([]*HunkInfo{}, &HunkInfo{ + hunks = append(hunks, &HunkInfo{ LeftLine: int64(leftLine), LeftHunk: int64(leftHunk), RightLine: int64(rightLine), diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 6fe6f4b47c404..2f01bce1f7fe6 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -445,16 +445,6 @@ func Test_ReviewCodeComment(t *testing.T) { }) assert.NoError(t, err) - diffContent, _, err := git.NewCommand("diff").AddDynamicArguments(pr.MergeBase, pr.GetGitHeadRefName()).RunStdString( - t.Context(), &git.RunOpts{ - Dir: repo.RepoPath(), - }, - ) - assert.NoError(t, err) - fmt.Println("=======") - fmt.Println(diffContent) - fmt.Println("=======") - session := loginUser(t, "user2") comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ Type: issues_model.CommentTypeCode, From 8b4e077bdd68e0b4479eacafb5a9c4fbafe95bf8 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 22 Jul 2025 19:13:13 -0700 Subject: [PATCH 25/35] Fix bug --- models/issues/comment.go | 10 ++- models/migrations/migrations.go | 4 + services/automergequeue/automergequeue.go | 4 +- services/doctor/review.go | 46 ----------- services/issue/comments.go | 8 +- services/issue/issue.go | 28 +++---- services/migrations/gitea_uploader.go | 19 ++--- services/pull/review.go | 82 ++++++++++++-------- web_src/js/components/DiffCommitSelector.vue | 4 +- 9 files changed, 94 insertions(+), 111 deletions(-) delete mode 100644 services/doctor/review.go diff --git a/models/issues/comment.go b/models/issues/comment.go index a3a3858a4d5d8..7f1c2696feb80 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -238,6 +238,7 @@ type CommentMetaData struct { ProjectColumnID int64 `json:"project_column_id,omitempty"` ProjectColumnTitle string `json:"project_column_title,omitempty"` ProjectTitle string `json:"project_title,omitempty"` + BeforeCommitID string `json:"before_commit_id,omitempty"` // commit id before this comment } // Comment represents a comment in commit and issue page. @@ -293,7 +294,8 @@ type Comment struct { UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` // Reference issue in commit message - CommitSHA string `xorm:"VARCHAR(64)"` + BeforeCommitID string `xorm:"VARCHAR(64)"` + CommitSHA string `xorm:"VARCHAR(64)"` Attachments []*repo_model.Attachment `xorm:"-"` Reactions ReactionList `xorm:"-"` @@ -764,7 +766,7 @@ func (c *Comment) CodeCommentLink(ctx context.Context) string { return fmt.Sprintf("%s/files#%s", c.Issue.Link(), c.HashTag()) } -func GetCodeCommentRefName(prIndex, commentID int64) string { +func GetCodeCommentRefName(prIndex, commentID int64, suffix string) string { return fmt.Sprintf("refs/pull/%d/code-comment-%d", prIndex, commentID) } @@ -806,6 +808,7 @@ func CreateComment(ctx context.Context, opts *CreateCommentOptions) (_ *Comment, AssigneeID: opts.AssigneeID, AssigneeTeamID: opts.AssigneeTeamID, CommitID: opts.CommitID, + BeforeCommitID: opts.BeforeCommitID, CommitSHA: opts.CommitSHA, Line: opts.LineNum, Content: opts.Content, @@ -977,7 +980,8 @@ type CreateCommentOptions struct { OldRef string NewRef string CommitID int64 - CommitSHA string + BeforeCommitID string // before commit id when creating this code comment + CommitSHA string // after commit id when creating this code comment, ref commit id for other comment Patch string LineNum int64 TreePath string diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 176372486e8f6..e1e35b331c360 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -24,6 +24,7 @@ import ( "code.gitea.io/gitea/models/migrations/v1_22" "code.gitea.io/gitea/models/migrations/v1_23" "code.gitea.io/gitea/models/migrations/v1_24" + "code.gitea.io/gitea/models/migrations/v1_25" "code.gitea.io/gitea/models/migrations/v1_6" "code.gitea.io/gitea/models/migrations/v1_7" "code.gitea.io/gitea/models/migrations/v1_8" @@ -382,6 +383,9 @@ func prepareMigrationTasks() []*migration { newMigration(318, "Add anonymous_access_mode for repo_unit", v1_24.AddRepoUnitAnonymousAccessMode), newMigration(319, "Add ExclusiveOrder to Label table", v1_24.AddExclusiveOrderColumnToLabelTable), newMigration(320, "Migrate two_factor_policy to login_source table", v1_24.MigrateSkipTwoFactor), + + // Gitea 1.24.0-rc0 ends at migration ID number 320 (database version 321) + newMigration(321, "Add BeforeCommitID to Comment table", v1_25.AddBeforeCommitIDForComment), } return preparedMigrations } diff --git a/services/automergequeue/automergequeue.go b/services/automergequeue/automergequeue.go index aed8cded44102..fa9c04da87492 100644 --- a/services/automergequeue/automergequeue.go +++ b/services/automergequeue/automergequeue.go @@ -39,11 +39,11 @@ func StartPRCheckAndAutoMerge(ctx context.Context, pull *issues_model.PullReques return } defer gitRepo.Close() - headCommitID, err := gitRepo.GetRefCommitID(pull.GetGitHeadRefName()) + commitID, err := gitRepo.GetRefCommitID(pull.GetGitHeadRefName()) if err != nil { log.Error("GetRefCommitID: %v", err) return } - AddToQueue(pull, headCommitID) + AddToQueue(pull, commitID) } diff --git a/services/doctor/review.go b/services/doctor/review.go deleted file mode 100644 index 0f7365b0e3ffd..0000000000000 --- a/services/doctor/review.go +++ /dev/null @@ -1,46 +0,0 @@ -// Copyright 2025 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -package doctor - -import ( - "context" - - "code.gitea.io/gitea/models/db" - "code.gitea.io/gitea/modules/log" -) - -// checkCommitSHAOfPullRequestCodeReviewComment will check if the commit SHA of pull request code review comments -// For a comment with negative line number, it should be the merge base of the pull request if the comment is on the files page -// if it's on a special commit page or a range commit page, it should be the previous commit when reviewing that commit/commit range -// so that this may be broken for those comments submitted in a special commit(non the first one) page or a range commit page -// NOTICE: the fix can only be done once, so it should be run twice or more -func checkCommitSHAOfPullRequestCodeReviewComment(ctx context.Context, logger log.Logger, autofix bool) error { - count, err := db.GetEngine(ctx).SQL("SELECT 1 FROM comment where line < 0 AND commit_sha != (select merge_base from pull_request WHERE issue_id = comment.issue_id)").Count() - if err != nil { - logger.Critical("Error: %v whilst counting wrong comment commit sha", err) - return err - } - if count > 0 { - if autofix { - total, err := db.GetEngine(ctx).Exec("UPDATE comment SET commit_sha = (select merge_base from pull_request WHERE issue_id = comment.issue_id) WHERE line < 0") - if err != nil { - return err - } - logger.Info("%d comments with wrong commit sha fixed\nWARNING: This doctor can only fix this once, so it should NOT be run twice or more", total) - } else { - logger.Warn("%d comments with wrong commit sha exist", count) - } - } - return nil -} - -func init() { - Register(&Check{ - Title: "Check if comment with negative line number has wrong commit sha", - Name: "check-commitsha-review-comment", - IsDefault: true, - Run: checkCommitSHAOfPullRequestCodeReviewComment, - Priority: 3, - }) -} diff --git a/services/issue/comments.go b/services/issue/comments.go index 33934f05d7732..1334ff266ce63 100644 --- a/services/issue/comments.go +++ b/services/issue/comments.go @@ -153,7 +153,13 @@ func DeleteComment(ctx context.Context, doer *user_model.User, comment *issues_m } if comment.ReviewID > 0 { - if err := git.RemoveRef(ctx, comment.Issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(comment.Issue.PullRequest.Index, comment.ID)); err != nil { + if err := git.RemoveRef(ctx, comment.Issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(comment.Issue.PullRequest.Index, comment.ID, "before")); err != nil { + log.Error("Unable to remove ref in base repository for PR[%d] Error: %v", comment.Issue.PullRequest.ID, err) + // We should not return error here, because the comment has been removed from database. + // users have to delete this ref manually or we should have a synchronize between + // database comment table and git refs. + } + if err := git.RemoveRef(ctx, comment.Issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(comment.Issue.PullRequest.Index, comment.ID, "after")); err != nil { log.Error("Unable to remove ref in base repository for PR[%d] Error: %v", comment.Issue.PullRequest.ID, err) // We should not return error here, because the comment has been removed from database. // users have to delete this ref manually or we should have a synchronize between diff --git a/services/issue/issue.go b/services/issue/issue.go index c1b9a213e285d..b1cbcc7398995 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -202,22 +202,22 @@ func DeleteIssue(ctx context.Context, doer *user_model.User, gitRepo *git.Reposi // delete pull request related git data if issue.IsPull && gitRepo != nil { if err := git.RemoveRef(ctx, gitRepo.Path, issue.PullRequest.GetGitHeadRefName()); err != nil { - return err + log.Error("Unable to remove ref %s in base repository for PR[%d] Error: %v", issue.PullRequest.GetGitHeadRefName(), issue.PullRequest.ID, err) } if issue.PullRequest.HasMerged { if err := git.RemoveRef(ctx, gitRepo.Path, issue.PullRequest.GetGitMergeRefName()); err != nil { - return err + log.Error("Unable to remove ref %s in base repository for PR[%d] Error: %v", issue.PullRequest.GetGitMergeRefName(), issue.PullRequest.ID, err) } } } for _, comment := range comments { - if comment.ReviewID > 0 { - if err := git.RemoveRef(ctx, issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID)); err != nil { - log.Error("Unable to remove ref in base repository for PR[%d] Error: %v", issue.PullRequest.ID, err) - // We should not return error here, because the comment has been removed from database. - // users have to delete this ref manually or we should have a synchronize between - // database comment table and git refs. + if comment.Type == issues_model.CommentTypeCode { + if err := git.RemoveRef(ctx, issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID, "before")); err != nil { + log.Error("Unable to remove ref %s in base repository for PR[%d] Error: %v", issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID, "before"), issue.PullRequest.ID, err) + } + if err := git.RemoveRef(ctx, issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID, "after")); err != nil { + log.Error("Unable to remove ref %s in base repository for PR[%d] Error: %v", issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID, "after"), issue.PullRequest.ID, err) } } } @@ -431,12 +431,12 @@ func DeleteIssuesByRepoID(ctx context.Context, repoID int64) (attachmentPaths [] } for _, comment := range comments { - if comment.ReviewID > 0 { - if err := git.RemoveRef(ctx, issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID)); err != nil { - log.Error("Unable to remove ref in base repository for PR[%d] Error: %v", issue.PullRequest.ID, err) - // We should not return error here, because the comment has been removed from database. - // users have to delete this ref manually or we should have a synchronize between - // database comment table and git refs. + if comment.Type == issues_model.CommentTypeCode { + if err := git.RemoveRef(ctx, issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID, "before")); err != nil { + log.Error("Unable to remove ref %s in base repository for PR[%d] Error: %v", issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID, "before"), issue.PullRequest.ID, err) + } + if err := git.RemoveRef(ctx, issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID, "after")); err != nil { + log.Error("Unable to remove ref %s in base repository for PR[%d] Error: %v", issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID, "after"), issue.PullRequest.ID, err) } } } diff --git a/services/migrations/gitea_uploader.go b/services/migrations/gitea_uploader.go index 75eb06d01fa3f..1bf59bb2ab2b8 100644 --- a/services/migrations/gitea_uploader.go +++ b/services/migrations/gitea_uploader.go @@ -927,15 +927,16 @@ func (g *GiteaLocalUploader) CreateReviews(ctx context.Context, reviews ...*base } c := issues_model.Comment{ - Type: issues_model.CommentTypeCode, - IssueID: issue.ID, - Content: comment.Content, - Line: int64(line + comment.Position - 1), - TreePath: comment.TreePath, - CommitSHA: comment.CommitID, - Patch: patch, - CreatedUnix: timeutil.TimeStamp(comment.CreatedAt.Unix()), - UpdatedUnix: timeutil.TimeStamp(comment.UpdatedAt.Unix()), + Type: issues_model.CommentTypeCode, + IssueID: issue.ID, + Content: comment.Content, + Line: int64(line + comment.Position - 1), + TreePath: comment.TreePath, + BeforeCommitID: pr.MergeBase, + CommitSHA: comment.CommitID, + Patch: patch, + CreatedUnix: timeutil.TimeStamp(comment.CreatedAt.Unix()), + UpdatedUnix: timeutil.TimeStamp(comment.UpdatedAt.Unix()), } if err := g.remapUser(ctx, review, &c); err != nil { diff --git a/services/pull/review.go b/services/pull/review.go index 2fc9c917ac0d7..401f59fcf9e3f 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -268,21 +268,21 @@ func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_mo return nil, fmt.Errorf("GetPatch failed: %w", err) } - lineCommitID := util.Iif(line < 0, beforeCommitID, afterCommitID) return db.WithTx2(ctx, func(ctx context.Context) (*issues_model.Comment, error) { comment, err := issues_model.CreateComment(ctx, &issues_model.CreateCommentOptions{ - Type: issues_model.CommentTypeCode, - Doer: doer, - Repo: repo, - Issue: issue, - Content: content, - LineNum: line, - TreePath: treePath, - CommitSHA: lineCommitID, - ReviewID: reviewID, - Patch: patch, - Invalidated: false, - Attachments: attachments, + Type: issues_model.CommentTypeCode, + Doer: doer, + Repo: repo, + Issue: issue, + Content: content, + LineNum: line, + TreePath: treePath, + BeforeCommitID: beforeCommitID, + CommitSHA: afterCommitID, + ReviewID: reviewID, + Patch: patch, + Invalidated: false, + Attachments: attachments, }) if err != nil { return nil, err @@ -290,8 +290,12 @@ func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_mo // The line commit ID Must be referenced in the git repository, because the branch maybe rebased or force-pushed. // If the review commit is GC, the position can not be calculated dynamically. - if err := git.UpdateRef(ctx, pr.BaseRepo.RepoPath(), issues_model.GetCodeCommentRefName(pr.Index, comment.ID), lineCommitID); err != nil { - log.Error("Unable to update ref in base repository for PR[%d] Error: %v", pr.ID, err) + if err := git.UpdateRef(ctx, pr.BaseRepo.RepoPath(), issues_model.GetCodeCommentRefName(pr.Index, comment.ID, "before"), beforeCommitID); err != nil { + log.Error("Unable to update ref before_commitid in base repository for PR[%d] Error: %v", pr.ID, err) + return nil, err + } + if err := git.UpdateRef(ctx, pr.BaseRepo.RepoPath(), issues_model.GetCodeCommentRefName(pr.Index, comment.ID, "after"), afterCommitID); err != nil { + log.Error("Unable to update ref after_commitid in base repository for PR[%d] Error: %v", pr.ID, err) return nil, err } @@ -484,6 +488,7 @@ func DismissReview(ctx context.Context, reviewID, repoID int64, message string, } // ReCalculateLineNumber recalculates the line number based on the hunks of the diff. +// left side is the commit the comment was created on, right side is the commit the comment is displayed on. // If the returned line number is zero, it should not be displayed. func ReCalculateLineNumber(hunks []*git.HunkInfo, leftLine int64) int64 { if len(hunks) == 0 || leftLine == 0 { @@ -498,15 +503,16 @@ func ReCalculateLineNumber(hunks []*git.HunkInfo, leftLine int64) int64 { newLine := absLine for _, hunk := range hunks { - if hunk.LeftLine+hunk.LeftHunk <= absLine { - newLine += hunk.RightHunk - hunk.LeftHunk - } else if hunk.LeftLine <= absLine && absLine < hunk.LeftLine+hunk.LeftHunk { - // The line has been removed, so it should not be displayed - return 0 - } else if absLine < hunk.LeftLine { + if absLine < hunk.LeftLine { // The line is before the hunk, so we can ignore it continue + } else if hunk.LeftLine <= absLine && absLine < hunk.LeftLine+hunk.LeftHunk { + // The line is within the hunk, that means the line is deleted from the current commit + // So that we don't need to display this line + return 0 } + // The line is after the hunk, so we can add the right hunk size + newLine += hunk.RightHunk - hunk.LeftHunk } return util.Iif(isLeft, -newLine, newLine) } @@ -614,27 +620,35 @@ func LoadCodeComments(ctx context.Context, gitRepo *git.Repository, repo *repo_m } dstCommitID := beforeCommit.ID.String() + commentCommitID := comment.BeforeCommitID if comment.Line > 0 { dstCommitID = afterCommit.ID.String() + commentCommitID = comment.CommitSHA } - if comment.CommitSHA == dstCommitID { - lineComments[comment.Line] = append(lineComments[comment.Line], comment) - continue + if commentCommitID != dstCommitID { + // If the comment is not for the current commit, we need to recalculate the line number + hunks, ok := hunksCache[commentCommitID+".."+dstCommitID] + if !ok { + hunks, err = git.GetAffectedHunksForTwoCommitsSpecialFile(ctx, repo.RepoPath(), commentCommitID, dstCommitID, file.Name) + if err != nil { + return fmt.Errorf("GetAffectedHunksForTwoCommitsSpecialFile[%s, %s, %s]: %w", repo.FullName(), commentCommitID, dstCommitID, err) + } + hunksCache[commentCommitID+".."+dstCommitID] = hunks + } + comment.Line = ReCalculateLineNumber(hunks, comment.Line) } - // If the comment is not for the current commit, we need to recalculate the line number - hunks, ok := hunksCache[comment.CommitSHA+".."+dstCommitID] - if !ok { - hunks, err = git.GetAffectedHunksForTwoCommitsSpecialFile(ctx, repo.RepoPath(), comment.CommitSHA, dstCommitID, file.Name) + if comment.Line != 0 { + commentAfterCommit, err := gitRepo.GetCommit(comment.CommitSHA) if err != nil { - return fmt.Errorf("GetAffectedHunksForTwoCommitsSpecialFile[%s, %s, %s]: %w", repo.FullName(), dstCommitID, comment.CommitSHA, err) + return fmt.Errorf("GetCommit[%s]: %w", comment.CommitSHA, err) + } + // If the comment is not the first one or the comment created before the current commit + if lineComments[comment.Line] != nil || comment.CommitSHA == afterCommit.ID.String() || + commentAfterCommit.Committer.When.Before(afterCommit.Committer.When) { + lineComments[comment.Line] = append(lineComments[comment.Line], comment) } - hunksCache[comment.CommitSHA+".."+dstCommitID] = hunks - } - comment.Line = ReCalculateLineNumber(hunks, comment.Line) - if comment.Line != 0 { - lineComments[comment.Line] = append(lineComments[comment.Line], comment) } } diff --git a/web_src/js/components/DiffCommitSelector.vue b/web_src/js/components/DiffCommitSelector.vue index 03d55e896b969..71fcb23d2cb90 100644 --- a/web_src/js/components/DiffCommitSelector.vue +++ b/web_src/js/components/DiffCommitSelector.vue @@ -195,10 +195,10 @@ export default defineComponent({ start = this.commits[firstSelected - 1].id; } const end = this.commits.findLast((x) => x.selected).id; - if (start === end) { + if (firstSelected === end) { // if the start and end are the same, we show this single commit window.location.assign(`${this.issueLink}/commits/${start}${this.queryParams}`); - } else if (firstSelected === 0 && end === this.commits.at(-1).id) { + } else if (start === this.merge_base && end === this.commits.at(-1).id) { // if the first commit is selected and the last commit is selected, we show all commits window.location.assign(`${this.issueLink}/files${this.queryParams}`); } else { From 0806da79c76a8195f4af3f8184116ff70801fd64 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 22 Jul 2025 21:01:46 -0700 Subject: [PATCH 26/35] add missing file --- models/migrations/v1_25/main_test.go | 14 ++++++ models/migrations/v1_25/v321.go | 28 +++++++++++ models/migrations/v1_25/v321_test.go | 75 ++++++++++++++++++++++++++++ 3 files changed, 117 insertions(+) create mode 100644 models/migrations/v1_25/main_test.go create mode 100644 models/migrations/v1_25/v321.go create mode 100644 models/migrations/v1_25/v321_test.go diff --git a/models/migrations/v1_25/main_test.go b/models/migrations/v1_25/main_test.go new file mode 100644 index 0000000000000..d2c4a4105d3a8 --- /dev/null +++ b/models/migrations/v1_25/main_test.go @@ -0,0 +1,14 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package v1_25 + +import ( + "testing" + + "code.gitea.io/gitea/models/migrations/base" +) + +func TestMain(m *testing.M) { + base.MainTest(m) +} diff --git a/models/migrations/v1_25/v321.go b/models/migrations/v1_25/v321.go new file mode 100644 index 0000000000000..308d7a9eba610 --- /dev/null +++ b/models/migrations/v1_25/v321.go @@ -0,0 +1,28 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package v1_25 + +import ( + "xorm.io/xorm" +) + +type comment struct { + BeforeCommitID string `xorm:"VARCHAR(64)"` +} + +// TableName return database table name for xorm +func (comment) TableName() string { + return "comment" +} + +func AddBeforeCommitIDForComment(x *xorm.Engine) error { + if _, err := x.SyncWithOptions(xorm.SyncOptions{ + IgnoreConstrains: true, + IgnoreIndices: true, + }, new(comment)); err != nil { + return err + } + x.Exec("UPDATE comment SET before_commit_id = (SELECT merge_base FROM pull_request WHERE pull_request.issue_id = comment.issue_id) WHERE before_commit_id IS NULL") + return nil +} diff --git a/models/migrations/v1_25/v321_test.go b/models/migrations/v1_25/v321_test.go new file mode 100644 index 0000000000000..90e2a9cf81f60 --- /dev/null +++ b/models/migrations/v1_25/v321_test.go @@ -0,0 +1,75 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package v1_25 + +import ( + "testing" + + "code.gitea.io/gitea/models/migrations/base" + "code.gitea.io/gitea/modules/references" + "code.gitea.io/gitea/modules/timeutil" + + "github.com/stretchr/testify/assert" +) + +func Test_AddCombinedIndexToIssueUser(t *testing.T) { + type Comment struct { // old struct + ID int64 `xorm:"pk autoincr"` + Type int `xorm:"INDEX"` + PosterID int64 `xorm:"INDEX"` + OriginalAuthor string + OriginalAuthorID int64 + IssueID int64 `xorm:"INDEX"` + LabelID int64 + OldProjectID int64 + ProjectID int64 + OldMilestoneID int64 + MilestoneID int64 + TimeID int64 + AssigneeID int64 + RemovedAssignee bool + AssigneeTeamID int64 `xorm:"NOT NULL DEFAULT 0"` + ResolveDoerID int64 + OldTitle string + NewTitle string + OldRef string + NewRef string + DependentIssueID int64 `xorm:"index"` // This is used by issue_service.deleteIssue + + CommitID int64 + Line int64 // - previous line / + proposed line + TreePath string + Content string `xorm:"LONGTEXT"` + ContentVersion int `xorm:"NOT NULL DEFAULT 0"` + + // Path represents the 4 lines of code cemented by this comment + Patch string `xorm:"-"` + PatchQuoted string `xorm:"LONGTEXT patch"` + + CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"` + UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` + + // Reference issue in commit message + CommitSHA string `xorm:"VARCHAR(64)"` + + ReviewID int64 `xorm:"index"` + Invalidated bool + + // Reference an issue or pull from another comment, issue or PR + // All information is about the origin of the reference + RefRepoID int64 `xorm:"index"` // Repo where the referencing + RefIssueID int64 `xorm:"index"` + RefCommentID int64 `xorm:"index"` // 0 if origin is Issue title or content (or PR's) + RefAction references.XRefAction `xorm:"SMALLINT"` // What happens if RefIssueID resolves + RefIsPull bool + + CommentMetaData string `xorm:"JSON TEXT"` // put all non-index metadata in a single field + } + + // Prepare and load the testing database + x, deferable := base.PrepareTestEnv(t, 0, new(Comment)) + defer deferable() + + assert.NoError(t, AddBeforeCommitIDForComment(x)) +} From 5bb19fd8ea2a509041143a6237a31550db9ae42b Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 22 Jul 2025 22:28:24 -0700 Subject: [PATCH 27/35] Remove unnecessary code --- models/issues/comment.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/models/issues/comment.go b/models/issues/comment.go index 493ddf2fcf3af..ab9fafa9f0230 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -1007,7 +1007,6 @@ type FindCommentsOptions struct { RepoID int64 IssueID int64 ReviewID int64 - CommitSHA string Since int64 Before int64 Line int64 @@ -1053,9 +1052,6 @@ func (opts FindCommentsOptions) ToConds() builder.Cond { if opts.IsPull.Has() { cond = cond.And(builder.Eq{"issue.is_pull": opts.IsPull.Value()}) } - if opts.CommitSHA != "" { - cond = cond.And(builder.Eq{"comment.commit_sha": opts.CommitSHA}) - } return cond } From 664da3ad7167bff992bff10a49a0a399b0dfe04e Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 22 Jul 2025 22:30:48 -0700 Subject: [PATCH 28/35] Remove unnecessary code --- models/issues/comment.go | 1 - 1 file changed, 1 deletion(-) diff --git a/models/issues/comment.go b/models/issues/comment.go index ab9fafa9f0230..3926842d22941 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -238,7 +238,6 @@ type CommentMetaData struct { ProjectColumnID int64 `json:"project_column_id,omitempty"` ProjectColumnTitle string `json:"project_column_title,omitempty"` ProjectTitle string `json:"project_title,omitempty"` - BeforeCommitID string `json:"before_commit_id,omitempty"` // commit id before this comment } // Comment represents a comment in commit and issue page. From 105179ad617cc7e5f2253e0c42d5e5f064529e00 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 22 Jul 2025 22:32:22 -0700 Subject: [PATCH 29/35] Fix lint --- models/migrations/v1_25/v321.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/models/migrations/v1_25/v321.go b/models/migrations/v1_25/v321.go index 308d7a9eba610..887540d070b3f 100644 --- a/models/migrations/v1_25/v321.go +++ b/models/migrations/v1_25/v321.go @@ -23,6 +23,6 @@ func AddBeforeCommitIDForComment(x *xorm.Engine) error { }, new(comment)); err != nil { return err } - x.Exec("UPDATE comment SET before_commit_id = (SELECT merge_base FROM pull_request WHERE pull_request.issue_id = comment.issue_id) WHERE before_commit_id IS NULL") - return nil + _, err := x.Exec("UPDATE comment SET before_commit_id = (SELECT merge_base FROM pull_request WHERE pull_request.issue_id = comment.issue_id) WHERE before_commit_id IS NULL") + return err } From 1fb7b013f3c6cdf490723ae58072bfd974a3125e Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 22 Jul 2025 23:10:19 -0700 Subject: [PATCH 30/35] improvements --- models/migrations/v1_25/v321.go | 2 +- services/issue/comments.go | 13 ++++----- services/issue/issue.go | 50 +++++++++++++++------------------ services/issue/issue_test.go | 6 ++-- services/pull/review.go | 50 +++++++++++++++++---------------- 5 files changed, 58 insertions(+), 63 deletions(-) diff --git a/models/migrations/v1_25/v321.go b/models/migrations/v1_25/v321.go index 887540d070b3f..ea5704b5b7a2e 100644 --- a/models/migrations/v1_25/v321.go +++ b/models/migrations/v1_25/v321.go @@ -23,6 +23,6 @@ func AddBeforeCommitIDForComment(x *xorm.Engine) error { }, new(comment)); err != nil { return err } - _, err := x.Exec("UPDATE comment SET before_commit_id = (SELECT merge_base FROM pull_request WHERE pull_request.issue_id = comment.issue_id) WHERE before_commit_id IS NULL") + _, err := x.Exec("UPDATE comment SET before_commit_id = (SELECT merge_base FROM pull_request WHERE pull_request.issue_id = comment.issue_id) WHERE `type`=21 AND before_commit_id IS NULL") return err } diff --git a/services/issue/comments.go b/services/issue/comments.go index 1334ff266ce63..83b6d0b1f47fb 100644 --- a/services/issue/comments.go +++ b/services/issue/comments.go @@ -134,7 +134,7 @@ func UpdateComment(ctx context.Context, c *issues_model.Comment, contentVersion // DeleteComment deletes the comment func DeleteComment(ctx context.Context, doer *user_model.User, comment *issues_model.Comment) error { - if comment.ReviewID > 0 { + if comment.Type == issues_model.CommentTypeCode { if err := comment.LoadIssue(ctx); err != nil { return err } @@ -152,18 +152,15 @@ func DeleteComment(ctx context.Context, doer *user_model.User, comment *issues_m return err } - if comment.ReviewID > 0 { + if comment.Type == issues_model.CommentTypeCode { + // We should not return error here, because the comment has been removed from database. + // users have to delete this ref manually or we should have a synchronize between + // database comment table and git refs. if err := git.RemoveRef(ctx, comment.Issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(comment.Issue.PullRequest.Index, comment.ID, "before")); err != nil { log.Error("Unable to remove ref in base repository for PR[%d] Error: %v", comment.Issue.PullRequest.ID, err) - // We should not return error here, because the comment has been removed from database. - // users have to delete this ref manually or we should have a synchronize between - // database comment table and git refs. } if err := git.RemoveRef(ctx, comment.Issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(comment.Issue.PullRequest.Index, comment.ID, "after")); err != nil { log.Error("Unable to remove ref in base repository for PR[%d] Error: %v", comment.Issue.PullRequest.ID, err) - // We should not return error here, because the comment has been removed from database. - // users have to delete this ref manually or we should have a synchronize between - // database comment table and git refs. } } diff --git a/services/issue/issue.go b/services/issue/issue.go index b1cbcc7398995..2158f5ce86197 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -191,7 +191,7 @@ func DeleteIssue(ctx context.Context, doer *user_model.User, gitRepo *git.Reposi } // delete entries in database - attachmentPaths, comments, err := deleteIssue(ctx, issue) + attachmentPaths, err := deleteIssue(ctx, issue) if err != nil { return err } @@ -211,7 +211,7 @@ func DeleteIssue(ctx context.Context, doer *user_model.User, gitRepo *git.Reposi } } - for _, comment := range comments { + for _, comment := range issue.Comments { if comment.Type == issues_model.CommentTypeCode { if err := git.RemoveRef(ctx, issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID, "before")); err != nil { log.Error("Unable to remove ref %s in base repository for PR[%d] Error: %v", issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID, "before"), issue.PullRequest.ID, err) @@ -277,48 +277,48 @@ func GetRefEndNamesAndURLs(issues []*issues_model.Issue, repoLink string) (map[i } // deleteIssue deletes the issue -func deleteIssue(ctx context.Context, issue *issues_model.Issue) ([]string, []*issues_model.Comment, error) { - var attachmentPaths []string - if err := db.WithTx(ctx, func(ctx context.Context) error { +func deleteIssue(ctx context.Context, issue *issues_model.Issue) ([]string, error) { + return db.WithTx2(ctx, func(ctx context.Context) ([]string, error) { // update the total issue numbers if err := repo_model.UpdateRepoIssueNumbers(ctx, issue.RepoID, issue.IsPull, false); err != nil { - return err + return nil, err } // if the issue is closed, update the closed issue numbers if issue.IsClosed { if err := repo_model.UpdateRepoIssueNumbers(ctx, issue.RepoID, issue.IsPull, true); err != nil { - return err + return nil, err } } if err := issues_model.UpdateMilestoneCounters(ctx, issue.MilestoneID); err != nil { - return fmt.Errorf("error updating counters for milestone id %d: %w", + return nil, fmt.Errorf("error updating counters for milestone id %d: %w", issue.MilestoneID, err) } if err := activities_model.DeleteIssueActions(ctx, issue.RepoID, issue.ID, issue.Index); err != nil { - return err + return nil, err } // find attachments related to this issue and remove them if err := issue.LoadAttachments(ctx); err != nil { - return err + return nil, err } + var attachmentPaths []string for i := range issue.Attachments { attachmentPaths = append(attachmentPaths, issue.Attachments[i].RelativePath()) } // deference all review comments if err := issue.LoadRepo(ctx); err != nil { - return err + return nil, err } if err := issue.LoadPullRequest(ctx); err != nil { - return err + return nil, err } if err := issue.LoadComments(ctx); err != nil { - return err + return nil, err } // delete all database data still assigned to this issue @@ -341,12 +341,12 @@ func deleteIssue(ctx context.Context, issue *issues_model.Issue) ([]string, []*i &issues_model.Comment{DependentIssueID: issue.ID}, &issues_model.IssuePin{IssueID: issue.ID}, ); err != nil { - return err + return nil, err } for _, comment := range issue.Comments { if err := issues_model.DeleteComment(ctx, comment); err != nil { - return err + return nil, err } } @@ -355,29 +355,25 @@ func deleteIssue(ctx context.Context, issue *issues_model.Issue) ([]string, []*i // Delete scheduled auto merges if _, err := db.GetEngine(ctx).Where("pull_id=?", issue.PullRequest.ID). Delete(&pull_model.AutoMerge{}); err != nil { - return err + return nil, err } // Delete review states if _, err := db.GetEngine(ctx).Where("pull_id=?", issue.PullRequest.ID). Delete(&pull_model.ReviewState{}); err != nil { - return err + return nil, err } if _, err := db.GetEngine(ctx).ID(issue.PullRequest.ID).Delete(&issues_model.PullRequest{}); err != nil { - return err + return nil, err } } if _, err := db.GetEngine(ctx).ID(issue.ID).NoAutoCondition().Delete(issue); err != nil { - return err + return nil, err } - return nil - }); err != nil { - return nil, nil, err - } - - return attachmentPaths, issue.Comments, nil + return attachmentPaths, nil + }) } // DeleteOrphanedIssues delete issues without a repo @@ -425,12 +421,12 @@ func DeleteIssuesByRepoID(ctx context.Context, repoID int64) (attachmentPaths [] } for _, issue := range issues { - issueAttachPaths, comments, err := deleteIssue(ctx, issue) + issueAttachPaths, err := deleteIssue(ctx, issue) if err != nil { return nil, fmt.Errorf("deleteIssue [issue_id: %d]: %w", issue.ID, err) } - for _, comment := range comments { + for _, comment := range issue.Comments { if comment.Type == issues_model.CommentTypeCode { if err := git.RemoveRef(ctx, issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID, "before")); err != nil { log.Error("Unable to remove ref %s in base repository for PR[%d] Error: %v", issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID, "before"), issue.PullRequest.ID, err) diff --git a/services/issue/issue_test.go b/services/issue/issue_test.go index 920a8e5ace7f0..0527192f9161b 100644 --- a/services/issue/issue_test.go +++ b/services/issue/issue_test.go @@ -41,7 +41,7 @@ func TestIssue_DeleteIssue(t *testing.T) { issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: issueIDs[2]}) - _, _, err = deleteIssue(db.DefaultContext, issue) + _, err = deleteIssue(db.DefaultContext, issue) assert.NoError(t, err) issueIDs, err = issues_model.GetIssueIDsByRepoID(db.DefaultContext, 1) assert.NoError(t, err) @@ -52,7 +52,7 @@ func TestIssue_DeleteIssue(t *testing.T) { assert.NoError(t, err) issue, err = issues_model.GetIssueByID(db.DefaultContext, 4) assert.NoError(t, err) - _, _, err = deleteIssue(db.DefaultContext, issue) + _, err = deleteIssue(db.DefaultContext, issue) assert.NoError(t, err) assert.Len(t, attachments, 2) for i := range attachments { @@ -75,7 +75,7 @@ func TestIssue_DeleteIssue(t *testing.T) { assert.NoError(t, err) assert.False(t, left) - _, _, err = deleteIssue(db.DefaultContext, issue2) + _, err = deleteIssue(db.DefaultContext, issue2) assert.NoError(t, err) left, err = issues_model.IssueNoDependenciesLeft(db.DefaultContext, issue1) assert.NoError(t, err) diff --git a/services/pull/review.go b/services/pull/review.go index 401f59fcf9e3f..f1a3550b80600 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -518,7 +518,7 @@ func ReCalculateLineNumber(hunks []*git.HunkInfo, leftLine int64) int64 { } // FetchCodeCommentsByLine fetches the code comments for a given commit, treePath and line number of a pull request. -func FetchCodeCommentsByLine(ctx context.Context, gitRepo *git.Repository, repo *repo_model.Repository, issueID int64, currentUser *user_model.User, startCommitID, endCommitID, treePath string, line int64, showOutdatedComments bool) (issues_model.CommentList, error) { +func FetchCodeCommentsByLine(ctx context.Context, gitRepo *git.Repository, repo *repo_model.Repository, issueID int64, currentUser *user_model.User, beforeCommitID, afterCommitID, treePath string, line int64, showOutdatedComments bool) (issues_model.CommentList, error) { opts := issues_model.FindCommentsOptions{ Type: issues_model.CommentTypeCode, IssueID: issueID, @@ -533,52 +533,54 @@ func FetchCodeCommentsByLine(ctx context.Context, gitRepo *git.Repository, repo if len(comments) == 0 { return nil, nil } + afterCommit, err := gitRepo.GetCommit(afterCommitID) + if err != nil { + return nil, fmt.Errorf("GetCommit[%s]: %w", afterCommitID, err) + } n := 0 hunksCache := make(map[string][]*git.HunkInfo) for _, comment := range comments { // Code comment should always have a commit SHA, if not, we need to set it based on the line number if comment.CommitSHA == "" { if comment.Line > 0 { - comment.CommitSHA = endCommitID + comment.CommitSHA = afterCommitID } else if comment.Line < 0 { - comment.CommitSHA = startCommitID + comment.CommitSHA = beforeCommitID } else { // If the comment has no line number, we cannot display it in the diff view continue } } - dstCommitID := startCommitID + dstCommitID := beforeCommitID + commentCommitID := comment.BeforeCommitID if comment.Line > 0 { - dstCommitID = endCommitID + dstCommitID = afterCommitID + commentCommitID = comment.CommitSHA } - if comment.CommitSHA == dstCommitID { - if comment.Line == line { - comments[n] = comment - n++ + if commentCommitID != dstCommitID { + // If the comment is not for the current commit, we need to recalculate the line number + hunks, ok := hunksCache[commentCommitID+".."+dstCommitID] + if !ok { + hunks, err = git.GetAffectedHunksForTwoCommitsSpecialFile(ctx, repo.RepoPath(), commentCommitID, dstCommitID, treePath) + if err != nil { + return nil, fmt.Errorf("GetAffectedHunksForTwoCommitsSpecialFile[%s, %s, %s]: %w", repo.FullName(), commentCommitID, dstCommitID, err) + } + hunksCache[commentCommitID+".."+dstCommitID] = hunks } - continue + comment.Line = ReCalculateLineNumber(hunks, comment.Line) } - // If the comment is not for the current commit, we need to recalculate the line number - hunks, ok := hunksCache[comment.CommitSHA+".."+dstCommitID] - if !ok { - hunks, err = git.GetAffectedHunksForTwoCommitsSpecialFile(ctx, repo.RepoPath(), comment.CommitSHA, dstCommitID, treePath) + if comment.Line == line { + commentAfterCommit, err := gitRepo.GetCommit(comment.CommitSHA) if err != nil { - return nil, fmt.Errorf("GetAffectedHunksForTwoCommitsSpecialFile[%s, %s, %s]: %w", repo.FullName(), comment.CommitSHA, dstCommitID, err) + return nil, fmt.Errorf("GetCommit[%s]: %w", comment.CommitSHA, err) } - hunksCache[comment.CommitSHA+".."+dstCommitID] = hunks - } - comment.Line = ReCalculateLineNumber(hunks, comment.Line) - if comment.Line != 0 { - dstCommit, err := gitRepo.GetCommit(dstCommitID) - if err != nil { - return nil, fmt.Errorf("GetCommit[%s]: %w", dstCommitID, err) - } // If the comment is not the first one or the comment created before the current commit - if n > 0 || comment.CreatedUnix.AsTime().Before(dstCommit.Committer.When) { + if n > 0 || comment.CommitSHA == afterCommitID || + commentAfterCommit.Committer.When.Before(afterCommit.Committer.When) { comments[n] = comment n++ } From 8fbc1e0d7cefedfb4143d1b351be08b6ade7549f Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 23 Jul 2025 12:16:33 -0700 Subject: [PATCH 31/35] Fix test --- models/migrations/v1_25/v321_test.go | 32 ++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/models/migrations/v1_25/v321_test.go b/models/migrations/v1_25/v321_test.go index 90e2a9cf81f60..456b53f93cba2 100644 --- a/models/migrations/v1_25/v321_test.go +++ b/models/migrations/v1_25/v321_test.go @@ -13,7 +13,7 @@ import ( "github.com/stretchr/testify/assert" ) -func Test_AddCombinedIndexToIssueUser(t *testing.T) { +func Test_AddBeforeCommitIDForComment(t *testing.T) { type Comment struct { // old struct ID int64 `xorm:"pk autoincr"` Type int `xorm:"INDEX"` @@ -67,8 +67,36 @@ func Test_AddCombinedIndexToIssueUser(t *testing.T) { CommentMetaData string `xorm:"JSON TEXT"` // put all non-index metadata in a single field } + type PullRequest struct { + ID int64 `xorm:"pk autoincr"` + Type int + Status int + ConflictedFiles []string `xorm:"TEXT JSON"` + CommitsAhead int + CommitsBehind int + + ChangedProtectedFiles []string `xorm:"TEXT JSON"` + + IssueID int64 `xorm:"INDEX"` + Index int64 + + HeadRepoID int64 `xorm:"INDEX"` + BaseRepoID int64 `xorm:"INDEX"` + HeadBranch string + BaseBranch string + MergeBase string `xorm:"VARCHAR(64)"` + AllowMaintainerEdit bool `xorm:"NOT NULL DEFAULT false"` + + HasMerged bool `xorm:"INDEX"` + MergedCommitID string `xorm:"VARCHAR(64)"` + MergerID int64 `xorm:"INDEX"` + MergedUnix timeutil.TimeStamp `xorm:"updated INDEX"` + + Flow int `xorm:"NOT NULL DEFAULT 0"` + } + // Prepare and load the testing database - x, deferable := base.PrepareTestEnv(t, 0, new(Comment)) + x, deferable := base.PrepareTestEnv(t, 0, new(Comment), new(PullRequest)) defer deferable() assert.NoError(t, AddBeforeCommitIDForComment(x)) From 648c479159d489e43c5e83bb758db4031520d1bf Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 23 Jul 2025 12:35:37 -0700 Subject: [PATCH 32/35] Fix bug --- services/pull/review.go | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/services/pull/review.go b/services/pull/review.go index f1a3550b80600..c7cf8b9cfe162 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -541,15 +541,11 @@ func FetchCodeCommentsByLine(ctx context.Context, gitRepo *git.Repository, repo hunksCache := make(map[string][]*git.HunkInfo) for _, comment := range comments { // Code comment should always have a commit SHA, if not, we need to set it based on the line number + if comment.BeforeCommitID == "" { + comment.BeforeCommitID = beforeCommitID + } if comment.CommitSHA == "" { - if comment.Line > 0 { - comment.CommitSHA = afterCommitID - } else if comment.Line < 0 { - comment.CommitSHA = beforeCommitID - } else { - // If the comment has no line number, we cannot display it in the diff view - continue - } + comment.CommitSHA = afterCommitID } dstCommitID := beforeCommitID From 73b1d11a04bfbfda4a06ff3b62e3d990598d1a60 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 23 Jul 2025 14:50:09 -0700 Subject: [PATCH 33/35] Fix bug --- web_src/js/components/DiffCommitSelector.vue | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web_src/js/components/DiffCommitSelector.vue b/web_src/js/components/DiffCommitSelector.vue index 71fcb23d2cb90..63f4ce04acca5 100644 --- a/web_src/js/components/DiffCommitSelector.vue +++ b/web_src/js/components/DiffCommitSelector.vue @@ -195,7 +195,7 @@ export default defineComponent({ start = this.commits[firstSelected - 1].id; } const end = this.commits.findLast((x) => x.selected).id; - if (firstSelected === end) { + if (start === end) { // if the start and end are the same, we show this single commit window.location.assign(`${this.issueLink}/commits/${start}${this.queryParams}`); } else if (start === this.merge_base && end === this.commits.at(-1).id) { From c9ce1cc640d2d37c55475de4dcb55fd9d96e2c4f Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 23 Jul 2025 15:54:49 -0700 Subject: [PATCH 34/35] Fix bug --- services/pull/review.go | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/services/pull/review.go b/services/pull/review.go index c7cf8b9cfe162..9b5b2e72ccb28 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -606,15 +606,11 @@ func LoadCodeComments(ctx context.Context, gitRepo *git.Repository, repo *repo_m hunksCache := make(map[string][]*git.HunkInfo) // filecomments should be sorted by created time, so that the latest comments are at the end for _, comment := range fileComments { + if comment.BeforeCommitID == "" { + comment.BeforeCommitID = beforeCommit.ID.String() + } if comment.CommitSHA == "" { - if comment.Line > 0 { - comment.CommitSHA = afterCommit.ID.String() - } else if comment.Line < 0 { - comment.CommitSHA = beforeCommit.ID.String() - } else { - // If the comment has no line number, we cannot display it in the diff view - continue - } + comment.CommitSHA = afterCommit.ID.String() } dstCommitID := beforeCommit.ID.String() From 6f6d95674bcf346a449481d7dffff57cad15b821 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 23 Jul 2025 22:28:57 -0700 Subject: [PATCH 35/35] Fix test --- tests/integration/pull_review_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 2f01bce1f7fe6..eb6e57f2c09d5 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -495,7 +495,7 @@ func Test_ReviewCodeComment(t *testing.T) { assert.Equal(t, "code comment on left line 2", commentLast.Content) assert.Equal(t, "README.md", commentLast.TreePath) assert.Equal(t, int64(-2), commentLast.Line) - assert.Equal(t, pr.MergeBase, commentLast.CommitSHA) + assert.Equal(t, pr.MergeBase, commentLast.BeforeCommitID) // load the files page and confirm the comment's line number is dynamically adjusted filesPageURL = fmt.Sprintf("/user2/test_codecomment/pulls/%d/files", pr.Index)