From 17d62fadfed200f8b15dd3ba143d9abc52b79146 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Dachary?= Date: Mon, 20 Feb 2023 19:59:45 +0100 Subject: [PATCH] Revert "some refactor about code comments(#20821) (#22707)" This reverts commit 1d191f9b5a262a9933c6024e051161dc90aa7f22. Refs: https://codeberg.org/forgejo/forgejo/issues/395 --- models/db/list_options.go | 50 +--------- models/issues/comment.go | 179 +++++++++++++++++++++++++++++----- models/issues/comment_code.go | 129 ------------------------ models/issues/pull_list.go | 27 ++++- models/issues/review.go | 8 +- services/pull/pull.go | 2 +- services/pull/review.go | 49 +--------- 7 files changed, 186 insertions(+), 258 deletions(-) delete mode 100644 models/issues/comment_code.go diff --git a/models/db/list_options.go b/models/db/list_options.go index 90048af179..54f6d945c8 100644 --- a/models/db/list_options.go +++ b/models/db/list_options.go @@ -5,11 +5,8 @@ package db import ( - "context" - "code.gitea.io/gitea/modules/setting" - "xorm.io/builder" "xorm.io/xorm" ) @@ -22,7 +19,6 @@ const ( type Paginator interface { GetSkipTake() (skip, take int) GetStartEnd() (start, end int) - IsListAll() bool } // GetPaginatedSession creates a paginated database session @@ -49,12 +45,9 @@ func SetEnginePagination(e Engine, p Paginator) Engine { // ListOptions options to paginate results type ListOptions struct { PageSize int - Page int // start from 1 - ListAll bool // if true, then PageSize and Page will not be taken + Page int // start from 1 } -var _ Paginator = &ListOptions{} - // GetSkipTake returns the skip and take values func (opts *ListOptions) GetSkipTake() (skip, take int) { opts.SetDefaultValues() @@ -68,11 +61,6 @@ func (opts *ListOptions) GetStartEnd() (start, end int) { return start, end } -// IsListAll indicates PageSize and Page will be ignored -func (opts *ListOptions) IsListAll() bool { - return opts.ListAll -} - // SetDefaultValues sets default values func (opts *ListOptions) SetDefaultValues() { if opts.PageSize <= 0 { @@ -92,8 +80,6 @@ type AbsoluteListOptions struct { take int } -var _ Paginator = &AbsoluteListOptions{} - // NewAbsoluteListOptions creates a list option with applied limits func NewAbsoluteListOptions(skip, take int) *AbsoluteListOptions { if skip < 0 { @@ -108,11 +94,6 @@ func NewAbsoluteListOptions(skip, take int) *AbsoluteListOptions { return &AbsoluteListOptions{skip, take} } -// IsListAll will always return false -func (opts *AbsoluteListOptions) IsListAll() bool { - return false -} - // GetSkipTake returns the skip and take values func (opts *AbsoluteListOptions) GetSkipTake() (skip, take int) { return opts.skip, opts.take @@ -122,32 +103,3 @@ func (opts *AbsoluteListOptions) GetSkipTake() (skip, take int) { func (opts *AbsoluteListOptions) GetStartEnd() (start, end int) { return opts.skip, opts.skip + opts.take } - -// FindOptions represents a find options -type FindOptions interface { - Paginator - ToConds() builder.Cond -} - -// Find represents a common find function which accept an options interface -func Find[T any](ctx context.Context, opts FindOptions, objects *[]T) error { - sess := GetEngine(ctx).Where(opts.ToConds()) - if !opts.IsListAll() { - sess.Limit(opts.GetSkipTake()) - } - return sess.Find(&objects) -} - -// Count represents a common count function which accept an options interface -func Count[T any](ctx context.Context, opts FindOptions, object T) (int64, error) { - return GetEngine(ctx).Where(opts.ToConds()).Count(object) -} - -// FindAndCount represents a common findandcount function which accept an options interface -func FindAndCount[T any](ctx context.Context, opts FindOptions, objects *[]T) (int64, error) { - sess := GetEngine(ctx).Where(opts.ToConds()) - if !opts.IsListAll() { - sess.Limit(opts.GetSkipTake()) - } - return sess.FindAndCount(&objects) -} diff --git a/models/issues/comment.go b/models/issues/comment.go index ee023272e1..291714799b 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -9,7 +9,9 @@ package issues import ( "context" "fmt" + "regexp" "strconv" + "strings" "unicode/utf8" "code.gitea.io/gitea/models/db" @@ -21,6 +23,8 @@ import ( "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/json" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/markup" + "code.gitea.io/gitea/modules/markup/markdown" "code.gitea.io/gitea/modules/references" "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/timeutil" @@ -693,6 +697,31 @@ func (c *Comment) LoadReview() error { return c.loadReview(db.DefaultContext) } +var notEnoughLines = regexp.MustCompile(`fatal: file .* has only \d+ lines?`) + +func (c *Comment) checkInvalidation(doer *user_model.User, repo *git.Repository, branch string) error { + // FIXME differentiate between previous and proposed line + commit, err := repo.LineBlame(branch, repo.Path, c.TreePath, uint(c.UnsignedLine())) + if err != nil && (strings.Contains(err.Error(), "fatal: no such path") || notEnoughLines.MatchString(err.Error())) { + c.Invalidated = true + return UpdateComment(c, doer) + } + if err != nil { + return err + } + if c.CommitSHA != "" && c.CommitSHA != commit.ID.String() { + c.Invalidated = true + return UpdateComment(c, doer) + } + return nil +} + +// CheckInvalidation checks if the line of code comment got changed by another commit. +// If the line got changed the comment is going to be invalidated. +func (c *Comment) CheckInvalidation(repo *git.Repository, doer *user_model.User, branch string) error { + return c.checkInvalidation(doer, repo, branch) +} + // DiffSide returns "previous" if Comment.Line is a LOC of the previous changes and "proposed" if it is a LOC of the proposed changes. func (c *Comment) DiffSide() string { if c.Line < 0 { @@ -1036,28 +1065,23 @@ func GetCommentByID(ctx context.Context, id int64) (*Comment, error) { // FindCommentsOptions describes the conditions to Find comments type FindCommentsOptions struct { db.ListOptions - RepoID int64 - IssueID int64 - ReviewID int64 - Since int64 - Before int64 - Line int64 - TreePath string - Type CommentType - IssueIDs []int64 - Invalidated util.OptionalBool + RepoID int64 + IssueID int64 + ReviewID int64 + Since int64 + Before int64 + Line int64 + TreePath string + Type CommentType } -// ToConds implements FindOptions interface -func (opts *FindCommentsOptions) ToConds() builder.Cond { +func (opts *FindCommentsOptions) toConds() builder.Cond { cond := builder.NewCond() if opts.RepoID > 0 { cond = cond.And(builder.Eq{"issue.repo_id": opts.RepoID}) } if opts.IssueID > 0 { cond = cond.And(builder.Eq{"comment.issue_id": opts.IssueID}) - } else if len(opts.IssueIDs) > 0 { - cond = cond.And(builder.In("comment.issue_id", opts.IssueIDs)) } if opts.ReviewID > 0 { cond = cond.And(builder.Eq{"comment.review_id": opts.ReviewID}) @@ -1077,16 +1101,13 @@ func (opts *FindCommentsOptions) ToConds() builder.Cond { if len(opts.TreePath) > 0 { cond = cond.And(builder.Eq{"comment.tree_path": opts.TreePath}) } - if !opts.Invalidated.IsNone() { - cond = cond.And(builder.Eq{"comment.invalidated": opts.Invalidated.IsTrue()}) - } return cond } // FindComments returns all comments according options func FindComments(ctx context.Context, opts *FindCommentsOptions) ([]*Comment, error) { comments := make([]*Comment, 0, 10) - sess := db.GetEngine(ctx).Where(opts.ToConds()) + sess := db.GetEngine(ctx).Where(opts.toConds()) if opts.RepoID > 0 { sess.Join("INNER", "issue", "issue.id = comment.issue_id") } @@ -1105,19 +1126,13 @@ func FindComments(ctx context.Context, opts *FindCommentsOptions) ([]*Comment, e // CountComments count all comments according options by ignoring pagination func CountComments(opts *FindCommentsOptions) (int64, error) { - sess := db.GetEngine(db.DefaultContext).Where(opts.ToConds()) + sess := db.GetEngine(db.DefaultContext).Where(opts.toConds()) if opts.RepoID > 0 { sess.Join("INNER", "issue", "issue.id = comment.issue_id") } return sess.Count(&Comment{}) } -// UpdateCommentInvalidate updates comment invalidated column -func UpdateCommentInvalidate(ctx context.Context, c *Comment) error { - _, err := db.GetEngine(ctx).ID(c.ID).Cols("invalidated").Update(c) - return err -} - // UpdateComment updates information of comment. func UpdateComment(c *Comment, doer *user_model.User) error { ctx, committer, err := db.TxContext() @@ -1176,6 +1191,120 @@ func DeleteComment(ctx context.Context, comment *Comment) error { return DeleteReaction(ctx, &ReactionOptions{CommentID: comment.ID}) } +// CodeComments represents comments on code by using this structure: FILENAME -> LINE (+ == proposed; - == previous) -> COMMENTS +type CodeComments map[string]map[int64][]*Comment + +// FetchCodeComments will return a 2d-map: ["Path"]["Line"] = Comments at line +func FetchCodeComments(ctx context.Context, issue *Issue, currentUser *user_model.User) (CodeComments, error) { + return fetchCodeCommentsByReview(ctx, issue, currentUser, nil) +} + +func fetchCodeCommentsByReview(ctx context.Context, issue *Issue, currentUser *user_model.User, review *Review) (CodeComments, error) { + pathToLineToComment := make(CodeComments) + if review == nil { + review = &Review{ID: 0} + } + opts := FindCommentsOptions{ + Type: CommentTypeCode, + IssueID: issue.ID, + ReviewID: review.ID, + } + + comments, err := findCodeComments(ctx, opts, issue, currentUser, review) + 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) + } + return pathToLineToComment, nil +} + +func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issue, currentUser *user_model.User, review *Review) ([]*Comment, error) { + var comments []*Comment + if review == nil { + review = &Review{ID: 0} + } + conds := opts.toConds() + if review.ID == 0 { + conds = conds.And(builder.Eq{"invalidated": false}) + } + e := db.GetEngine(ctx) + if err := e.Where(conds). + Asc("comment.created_unix"). + Asc("comment.id"). + Find(&comments); err != nil { + return nil, err + } + + if err := issue.LoadRepo(ctx); err != nil { + return nil, err + } + + if err := CommentList(comments).loadPosters(ctx); err != nil { + return nil, err + } + + // Find all reviews by ReviewID + reviews := make(map[int64]*Review) + ids := make([]int64, 0, len(comments)) + for _, comment := range comments { + if comment.ReviewID != 0 { + ids = append(ids, comment.ReviewID) + } + } + if err := e.In("id", ids).Find(&reviews); err != nil { + return nil, err + } + + n := 0 + for _, comment := range comments { + if re, ok := reviews[comment.ReviewID]; ok && re != nil { + // If the review is pending only the author can see the comments (except if the review is set) + if review.ID == 0 && re.Type == ReviewTypePending && + (currentUser == nil || currentUser.ID != re.ReviewerID) { + continue + } + comment.Review = re + } + comments[n] = comment + n++ + + if err := comment.LoadResolveDoer(); err != nil { + return nil, err + } + + if err := comment.LoadReactions(issue.Repo); err != nil { + return nil, err + } + + var err error + if comment.RenderedContent, err = markdown.RenderString(&markup.RenderContext{ + Ctx: ctx, + URLPrefix: issue.Repo.Link(), + Metas: issue.Repo.ComposeMetas(), + }, comment.Content); err != nil { + return nil, err + } + } + 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) ([]*Comment, error) { + opts := FindCommentsOptions{ + Type: CommentTypeCode, + IssueID: issue.ID, + TreePath: treePath, + Line: line, + } + return findCodeComments(ctx, opts, issue, currentUser, nil) +} + // UpdateCommentsMigrationsByType updates comments' migrations information via given git service type and original id and poster id func UpdateCommentsMigrationsByType(tp structs.GitServiceType, originalAuthorID string, posterID int64) error { _, err := db.GetEngine(db.DefaultContext).Table("comment"). diff --git a/models/issues/comment_code.go b/models/issues/comment_code.go deleted file mode 100644 index cd5b312189..0000000000 --- a/models/issues/comment_code.go +++ /dev/null @@ -1,129 +0,0 @@ -// Copyright 2022 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -package issues - -import ( - "context" - - "code.gitea.io/gitea/models/db" - user_model "code.gitea.io/gitea/models/user" - "code.gitea.io/gitea/modules/markup" - "code.gitea.io/gitea/modules/markup/markdown" - - "xorm.io/builder" -) - -// CodeComments represents comments on code by using this structure: FILENAME -> LINE (+ == proposed; - == previous) -> COMMENTS -type CodeComments map[string]map[int64][]*Comment - -// FetchCodeComments will return a 2d-map: ["Path"]["Line"] = Comments at line -func FetchCodeComments(ctx context.Context, issue *Issue, currentUser *user_model.User) (CodeComments, error) { - return fetchCodeCommentsByReview(ctx, issue, currentUser, nil) -} - -func fetchCodeCommentsByReview(ctx context.Context, issue *Issue, currentUser *user_model.User, review *Review) (CodeComments, error) { - pathToLineToComment := make(CodeComments) - if review == nil { - review = &Review{ID: 0} - } - opts := FindCommentsOptions{ - Type: CommentTypeCode, - IssueID: issue.ID, - ReviewID: review.ID, - } - - comments, err := findCodeComments(ctx, opts, issue, currentUser, review) - 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) - } - return pathToLineToComment, nil -} - -func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issue, currentUser *user_model.User, review *Review) ([]*Comment, error) { - var comments []*Comment - if review == nil { - review = &Review{ID: 0} - } - conds := opts.ToConds() - if review.ID == 0 { - conds = conds.And(builder.Eq{"invalidated": false}) - } - e := db.GetEngine(ctx) - if err := e.Where(conds). - Asc("comment.created_unix"). - Asc("comment.id"). - Find(&comments); err != nil { - return nil, err - } - - if err := issue.LoadRepo(ctx); err != nil { - return nil, err - } - - if err := CommentList(comments).loadPosters(ctx); err != nil { - return nil, err - } - - // Find all reviews by ReviewID - reviews := make(map[int64]*Review) - ids := make([]int64, 0, len(comments)) - for _, comment := range comments { - if comment.ReviewID != 0 { - ids = append(ids, comment.ReviewID) - } - } - if err := e.In("id", ids).Find(&reviews); err != nil { - return nil, err - } - - n := 0 - for _, comment := range comments { - if re, ok := reviews[comment.ReviewID]; ok && re != nil { - // If the review is pending only the author can see the comments (except if the review is set) - if review.ID == 0 && re.Type == ReviewTypePending && - (currentUser == nil || currentUser.ID != re.ReviewerID) { - continue - } - comment.Review = re - } - comments[n] = comment - n++ - - if err := comment.LoadResolveDoer(); err != nil { - return nil, err - } - - if err := comment.LoadReactions(issue.Repo); err != nil { - return nil, err - } - - var err error - if comment.RenderedContent, err = markdown.RenderString(&markup.RenderContext{ - Ctx: ctx, - URLPrefix: issue.Repo.Link(), - Metas: issue.Repo.ComposeMetas(), - }, comment.Content); err != nil { - return nil, err - } - } - 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) ([]*Comment, error) { - opts := FindCommentsOptions{ - Type: CommentTypeCode, - IssueID: issue.ID, - TreePath: treePath, - Line: line, - } - return findCodeComments(ctx, opts, issue, currentUser, nil) -} diff --git a/models/issues/pull_list.go b/models/issues/pull_list.go index 2bdcdacc4e..c69f18492b 100644 --- a/models/issues/pull_list.go +++ b/models/issues/pull_list.go @@ -13,6 +13,7 @@ import ( "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/base" + "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" "xorm.io/xorm" @@ -161,7 +162,7 @@ func (prs PullRequestList) loadAttributes(ctx context.Context) error { } // Load issues. - issueIDs := prs.GetIssueIDs() + issueIDs := prs.getIssueIDs() issues := make([]*Issue, 0, len(issueIDs)) if err := db.GetEngine(ctx). Where("id > 0"). @@ -180,8 +181,7 @@ func (prs PullRequestList) loadAttributes(ctx context.Context) error { return nil } -// GetIssueIDs returns all issue ids -func (prs PullRequestList) GetIssueIDs() []int64 { +func (prs PullRequestList) getIssueIDs() []int64 { issueIDs := make([]int64, 0, len(prs)) for i := range prs { issueIDs = append(issueIDs, prs[i].IssueID) @@ -193,3 +193,24 @@ func (prs PullRequestList) GetIssueIDs() []int64 { func (prs PullRequestList) LoadAttributes() error { return prs.loadAttributes(db.DefaultContext) } + +// InvalidateCodeComments will lookup the prs for code comments which got invalidated by change +func (prs PullRequestList) InvalidateCodeComments(ctx context.Context, doer *user_model.User, repo *git.Repository, branch string) error { + if len(prs) == 0 { + return nil + } + issueIDs := prs.getIssueIDs() + var codeComments []*Comment + if err := db.GetEngine(ctx). + Where("type = ? and invalidated = ?", CommentTypeCode, false). + In("issue_id", issueIDs). + Find(&codeComments); err != nil { + return fmt.Errorf("find code comments: %w", err) + } + for _, comment := range codeComments { + if err := comment.CheckInvalidation(repo, doer, branch); err != nil { + return err + } + } + return nil +} diff --git a/models/issues/review.go b/models/issues/review.go index 88fc0d94a8..40781befe4 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -978,7 +978,7 @@ func DeleteReview(r *Review) error { ReviewID: r.ID, } - if _, err := sess.Where(opts.ToConds()).Delete(new(Comment)); err != nil { + if _, err := sess.Where(opts.toConds()).Delete(new(Comment)); err != nil { return err } @@ -988,7 +988,7 @@ func DeleteReview(r *Review) error { ReviewID: r.ID, } - if _, err := sess.Where(opts.ToConds()).Delete(new(Comment)); err != nil { + if _, err := sess.Where(opts.toConds()).Delete(new(Comment)); err != nil { return err } @@ -1012,7 +1012,7 @@ func (r *Review) GetCodeCommentsCount() int { IssueID: r.IssueID, ReviewID: r.ID, } - conds := opts.ToConds() + conds := opts.toConds() if r.ID == 0 { conds = conds.And(builder.Eq{"invalidated": false}) } @@ -1032,7 +1032,7 @@ func (r *Review) HTMLURL() string { ReviewID: r.ID, } comment := new(Comment) - has, err := db.GetEngine(db.DefaultContext).Where(opts.ToConds()).Get(comment) + has, err := db.GetEngine(db.DefaultContext).Where(opts.toConds()).Get(comment) if err != nil || !has { return "" } diff --git a/services/pull/pull.go b/services/pull/pull.go index 3ec7ec92b7..fbfc31d4e7 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -240,7 +240,7 @@ func checkForInvalidation(ctx context.Context, requests issues_model.PullRequest } go func() { // FIXME: graceful: We need to tell the manager we're doing something... - err := InvalidateCodeComments(ctx, requests, doer, gitRepo, branch) + err := requests.InvalidateCodeComments(ctx, doer, gitRepo, branch) if err != nil { log.Error("PullRequestList.InvalidateCodeComments: %v", err) } diff --git a/services/pull/review.go b/services/pull/review.go index 5c86b7a9f3..16c9e108ee 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -23,53 +23,6 @@ import ( "code.gitea.io/gitea/modules/util" ) -var notEnoughLines = regexp.MustCompile(`fatal: file .* has only \d+ lines?`) - -// checkInvalidation checks if the line of code comment got changed by another commit. -// If the line got changed the comment is going to be invalidated. -func checkInvalidation(ctx context.Context, c *issues_model.Comment, doer *user_model.User, repo *git.Repository, branch string) error { - // FIXME differentiate between previous and proposed line - commit, err := repo.LineBlame(branch, repo.Path, c.TreePath, uint(c.UnsignedLine())) - if err != nil && (strings.Contains(err.Error(), "fatal: no such path") || notEnoughLines.MatchString(err.Error())) { - c.Invalidated = true - return issues_model.UpdateCommentInvalidate(ctx, c) - } - if err != nil { - return err - } - if c.CommitSHA != "" && c.CommitSHA != commit.ID.String() { - c.Invalidated = true - return issues_model.UpdateCommentInvalidate(ctx, c) - } - return nil -} - -// InvalidateCodeComments will lookup the prs for code comments which got invalidated by change -func InvalidateCodeComments(ctx context.Context, prs issues_model.PullRequestList, doer *user_model.User, repo *git.Repository, branch string) error { - if len(prs) == 0 { - return nil - } - issueIDs := prs.GetIssueIDs() - var codeComments []*issues_model.Comment - - if err := db.Find(ctx, &issues_model.FindCommentsOptions{ - ListOptions: db.ListOptions{ - ListAll: true, - }, - Type: issues_model.CommentTypeCode, - Invalidated: util.OptionalBoolFalse, - IssueIDs: issueIDs, - }, &codeComments); err != nil { - return fmt.Errorf("find code comments: %v", err) - } - for _, comment := range codeComments { - if err := checkInvalidation(ctx, comment, doer, repo, branch); err != nil { - return err - } - } - return nil -} - // CreateCodeComment creates a comment on the code line func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git.Repository, issue *issues_model.Issue, line int64, content, treePath string, isReview bool, replyReviewID int64, latestCommitID string) (*issues_model.Comment, error) { var ( @@ -161,6 +114,8 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. return comment, nil } +var notEnoughLines = regexp.MustCompile(`exit status 128 - fatal: file .* has only \d+ lines?`) + // createCodeComment creates a plain code comment at the specified line / path func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, issue *issues_model.Issue, content, treePath string, line, reviewID int64) (*issues_model.Comment, error) { var commitID, patch string