diff --git a/models/issues/comment.go b/models/issues/comment.go index 4fdb0c1808fb4..a3a3858a4d5d8 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 GetCodeCommentRefName(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) @@ -1007,6 +1011,7 @@ type FindCommentsOptions struct { RepoID int64 IssueID int64 ReviewID int64 + CommitSHA string Since int64 Before int64 Line int64 @@ -1052,6 +1057,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..b5849c6e90513 100644 --- a/models/issues/comment_test.go +++ b/models/issues/comment_test.go @@ -68,15 +68,20 @@ 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) + 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, 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/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/models/issues/pull.go b/models/issues/pull.go index 4c25b6f0c8494..77af38e0afa6b 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 "" @@ -413,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/models/issues/review.go b/models/issues/review.go index 71fdb7456f185..e388981fe63e3 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 } @@ -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/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/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 35d115be0e5da..34307f7768eee 100644 --- a/modules/git/diff.go +++ b/modules/git/diff.go @@ -107,12 +107,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 = 1 } 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 = 1 } } else { log.Debug("Parse line number failed: %v", diffHunk) @@ -342,3 +346,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..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" @@ -181,4 +182,28 @@ 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, 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) +} + +func Test_GetAffectedHunksForTwoCommitsSpecialFile(t *testing.T) { + 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) + // @@ -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/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.go b/modules/git/repo_commit.go index 4066a1ca7ba1f..5c74d077fde97 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,50 @@ 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 +} + +// 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 - 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 +403,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/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_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]) +} 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 25ee4c5198568..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") @@ -99,7 +69,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 +86,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/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/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/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..4050f75fc9b92 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,17 @@ 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) - } - } - 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 - } - } - baseCommit = parentCommit - } else { - // Keep an empty history or original commit - baseCommit = pull.MergeBase +func GetMergedBaseCommitID(ctx *context.Context, pull *issues_model.PullRequest) string { + if pull.MergeBase != "" { + return pull.MergeBase } - return baseCommit + 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 { @@ -271,7 +240,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) @@ -643,8 +618,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 +637,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 +647,70 @@ 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 + headCommitID, err := gitRepo.GetRefCommitID(pull.GetGitHeadRefName()) + if err != nil { + ctx.ServerError("GetRefCommitID", err) + return + } - if !(foundStartCommit && foundEndCommit) { - for _, commit := range prInfo.Commits { - if commit.ID.String() == specifiedStartCommit { - foundStartCommit = true - } - if commit.ID.String() == specifiedEndCommit { - foundEndCommit = true - } + isSingleCommit := beforeCommitID == "" && afterCommitID != "" + ctx.Data["IsShowingOnlySingleCommit"] = isSingleCommit + isShowAllCommits := (beforeCommitID == "" || beforeCommitID == prInfo.MergeBase) && (afterCommitID == "" || afterCommitID == headCommitID) + ctx.Data["IsShowingAllCommits"] = isShowAllCommits - if foundStartCommit && foundEndCommit { - break - } + 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 !(foundStartCommit && foundEndCommit) { - ctx.NotFound(nil) - return } } - if ctx.Written() { - return + if afterCommitID == "" || afterCommitID == headCommitID { + afterCommitID = headCommitID } - - headCommitID, err := gitRepo.GetRefCommitID(pull.GetGitHeadRefName()) - if err != nil { - ctx.ServerError("GetRefCommitID", err) + afterCommit = indexCommit(prInfo.Commits, afterCommitID) + if afterCommit == nil { + ctx.HTTPError(http.StatusBadRequest, "after commit not found in PR commits") return } - ctx.Data["IsShowingOnlySingleCommit"] = willShowSpecifiedCommit - - if willShowSpecifiedCommit || willShowSpecifiedCommitRange { - if len(specifiedEndCommit) > 0 { - endCommitID = specifiedEndCommit - } else { - endCommitID = headCommitID - } - if len(specifiedStartCommit) > 0 { - startCommitID = specifiedStartCommit - } else { - startCommitID = prInfo.MergeBase + if isSingleCommit { + beforeCommit, err = afterCommit.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() } 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["MergeBase"] = prInfo.MergeBase + 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 +718,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 +729,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 +737,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 +749,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 +785,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 +805,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 +812,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 +859,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 +888,17 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi } func ViewPullFilesForSingleCommit(ctx *context.Context) { - viewPullFiles(ctx, "", ctx.PathParam("sha"), true, true) + // 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) { - viewPullFiles(ctx, ctx.PathParam("shaFrom"), ctx.PathParam("shaTo"), true, false) -} - -func ViewPullFilesStartingFromCommit(ctx *context.Context) { - viewPullFiles(ctx, "", ctx.PathParam("sha"), true, false) + viewPullFiles(ctx, ctx.PathParam("shaFrom"), ctx.PathParam("shaTo")) } 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..fcb4b88776c45 100644 --- a/routers/web/repo/pull_review.go +++ b/routers/web/repo/pull_review.go @@ -7,15 +7,18 @@ 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" "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 +52,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 +76,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 +88,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 +109,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 +160,47 @@ 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 + } + + headCommitID, err := ctx.Repo.GitRepo.GetRefCommitID(comment.Issue.PullRequest.GetGitHeadRefName()) + if err != nil { + 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) + return + } + + if beforeCommitID == "" || beforeCommitID == comment.Issue.PullRequest.MergeBase { + beforeCommitID = comment.Issue.PullRequest.MergeBase + } 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 == "" || 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)) + 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.GitRepo, ctx.Repo.Repository, comment.IssueID, + ctx.Doer, beforeCommitID, afterCommitID, comment.TreePath, comment.Line, showOutdatedComments) if err != nil { ctx.ServerError("FetchCodeCommentsByLine", err) return @@ -195,16 +225,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 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/feed/notifier.go b/services/feed/notifier.go index 64aeccdfd227b..221bc2bdcbd9b 100644 --- a/services/feed/notifier.go +++ b/services/feed/notifier.go @@ -223,21 +223,19 @@ func (a *actionNotifier) PullRequestReview(ctx context.Context, pr *issues_model } actions := make([]*activities_model.Action, 0, 10) - for _, lines := range review.CodeComments { - for _, comments := range lines { - for _, comm := range comments { - actions = append(actions, &activities_model.Action{ - ActUserID: review.Reviewer.ID, - ActUser: review.Reviewer, - Content: fmt.Sprintf("%d|%s", review.Issue.Index, strings.Split(comm.Content, "\n")[0]), - OpType: activities_model.ActionCommentPull, - RepoID: review.Issue.RepoID, - Repo: review.Issue.Repo, - IsPrivate: review.Issue.Repo.IsPrivate, - Comment: comm, - CommentID: comm.ID, - }) - } + for _, comments := range review.CodeComments { + for _, comm := range comments { + actions = append(actions, &activities_model.Action{ + ActUserID: review.Reviewer.ID, + ActUser: review.Reviewer, + Content: fmt.Sprintf("%d|%s", review.Issue.Index, strings.Split(comm.Content, "\n")[0]), + OpType: activities_model.ActionCommentPull, + RepoID: review.Issue.RepoID, + Repo: review.Issue.Repo, + IsPrivate: review.Issue.Repo.IsPrivate, + Comment: comm, + CommentID: comm.ID, + }) } } diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go index cb267f891ccb7..ff937704c0be6 100644 --- a/services/forms/repo_form.go +++ b/services/forms/repo_form.go @@ -558,7 +558,8 @@ type CodeCommentForm struct { TreePath string `form:"path" binding:"Required"` SingleReview bool `form:"single_review"` Reply int64 `form:"reply"` - LatestCommitID string + BeforeCommitID string `form:"before_commit_id"` + AfterCommitID string `form:"after_commit_id"` Files []string } diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index 7c99e049d548a..092fc121588de 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -13,7 +13,6 @@ import ( "html/template" "io" "net/url" - "sort" "strings" "time" @@ -21,7 +20,6 @@ import ( git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" pull_model "code.gitea.io/gitea/models/pull" - user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/analyze" "code.gitea.io/gitea/modules/charset" "code.gitea.io/gitea/modules/git" @@ -454,32 +452,6 @@ type Diff struct { NumViewedFiles int // user-specific } -// LoadComments loads comments into each line -func (diff *Diff) LoadComments(ctx context.Context, issue *issues_model.Issue, currentUser *user_model.User, showOutdatedComments bool) error { - allComments, err := issues_model.FetchCodeComments(ctx, issue, currentUser, showOutdatedComments) - if err != nil { - return err - } - for _, file := range diff.Files { - if lineCommits, ok := allComments[file.Name]; ok { - for _, section := range file.Sections { - for _, line := range section.Lines { - if comments, ok := lineCommits[int64(line.LeftIdx*-1)]; ok { - line.Comments = append(line.Comments, comments...) - } - if comments, ok := lineCommits[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 -} - const cmdDiffHead = "diff --git " // ParsePatch builds a Diff object from a io.Reader and some parameters. diff --git a/services/gitdiff/gitdiff_test.go b/services/gitdiff/gitdiff_test.go index b84530043a132..5f0fd1a9049db 100644 --- a/services/gitdiff/gitdiff_test.go +++ b/services/gitdiff/gitdiff_test.go @@ -11,8 +11,6 @@ import ( "code.gitea.io/gitea/models/db" 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/json" "code.gitea.io/gitea/modules/setting" @@ -545,46 +543,6 @@ index 0000000..6bb8f39 } } -func setupDefaultDiff() *Diff { - return &Diff{ - Files: []*DiffFile{ - { - Name: "README.md", - Sections: []*DiffSection{ - { - Lines: []*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, diff.LoadComments(db.DefaultContext, issue, user, 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}) - diff := setupDefaultDiff() - assert.NoError(t, diff.LoadComments(db.DefaultContext, issue, user, true)) - assert.Len(t, diff.Files[0].Sections[0].Lines[0].Comments, 3) -} - func TestDiffLine_CanComment(t *testing.T) { assert.False(t, (&DiffLine{Type: DiffLineSection}).CanComment()) assert.False(t, (&DiffLine{Type: DiffLineAdd, Comments: []*issues_model.Comment{{Content: "bla"}}}).CanComment()) diff --git a/services/issue/comments.go b/services/issue/comments.go index 10c81198d57e2..33934f05d7732 100644 --- a/services/issue/comments.go +++ b/services/issue/comments.go @@ -13,8 +13,10 @@ 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/timeutil" git_service "code.gitea.io/gitea/services/git" notify_service "code.gitea.io/gitea/services/notify" @@ -132,13 +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 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 := db.WithTx(ctx, func(ctx context.Context) error { return issues_model.DeleteComment(ctx, comment) - }) - if err != nil { + }); 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 f03be3e18f6c7..c1b9a213e285d 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" @@ -190,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 } @@ -200,9 +201,25 @@ 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 } + if issue.PullRequest.HasMerged { + if err := git.RemoveRef(ctx, gitRepo.Path, issue.PullRequest.GetGitMergeRefName()); err != nil { + return 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. + } + } } notify_service.DeleteIssue(ctx, doer, issue) @@ -260,75 +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() +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 := db.GetEngine(ctx).ID(issue.ID).NoAutoCondition().Delete(issue); 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) + } - // 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 + if err := activities_model.DeleteIssueActions(ctx, issue.RepoID, issue.ID, issue.Index); 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) - } + // find attachments related to this issue and remove them + if err := issue.LoadAttachments(ctx); err != nil { + return err + } - if err := activities_model.DeleteIssueActions(ctx, issue.RepoID, issue.ID, issue.Index); err != nil { - return nil, err - } + 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 nil, err - } + // deference all review comments + if err := issue.LoadRepo(ctx); err != nil { + return err + } + if err := issue.LoadPullRequest(ctx); err != nil { + return err + } - var attachmentPaths []string - for i := range issue.Attachments { - attachmentPaths = append(attachmentPaths, issue.Attachments[i].RelativePath()) - } - - // 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}, - &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 + } - if err := committer.Commit(); 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 + } + + for _, comment := range issue.Comments { + if err := issues_model.DeleteComment(ctx, comment); 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 err + } + + // Delete review states + if _, err := db.GetEngine(ctx).Where("pull_id=?", issue.PullRequest.ID). + Delete(&pull_model.ReviewState{}); err != nil { + return 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.ID).NoAutoCondition().Delete(issue); err != nil { + return err + } + return nil + }); err != nil { + return nil, nil, err } - return attachmentPaths, nil + + return attachmentPaths, issue.Comments, nil } // DeleteOrphanedIssues delete issues without a repo @@ -376,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 bad0d65d1ed8f..920a8e5ace7f0 100644 --- a/services/issue/issue_test.go +++ b/services/issue/issue_test.go @@ -39,12 +39,9 @@ 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) + _, _, err = deleteIssue(db.DefaultContext, issue) assert.NoError(t, err) issueIDs, err = issues_model.GetIssueIDsByRepoID(db.DefaultContext, 1) assert.NoError(t, err) @@ -55,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 { @@ -78,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/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/mailer/incoming/incoming_handler.go b/services/mailer/incoming/incoming_handler.go index 38a234eac1fd2..7a07c946f3495 100644 --- a/services/mailer/incoming/incoming_handler.go +++ b/services/mailer/incoming/incoming_handler.go @@ -122,11 +122,12 @@ func (h *ReplyHandler) Handle(ctx context.Context, content *MailContent, doer *u nil, issue, comment.Line, + "", // no special commit ID, so we use the merge base of the pull request + "", // no special commit ID, so we use the current HEAD of the pull request content.Content, comment.TreePath, false, // not pending review but a single review comment.ReviewID, - "", attachmentIDs, ) if err != nil { diff --git a/services/mailer/mail_issue_common.go b/services/mailer/mail_issue_common.go index a34d8a68c97f3..e74bd310e7989 100644 --- a/services/mailer/mail_issue_common.go +++ b/services/mailer/mail_issue_common.go @@ -90,12 +90,7 @@ func composeIssueCommentMessages(ctx context.Context, comment *mailComment, lang fallback = prefix + fallbackIssueMailSubject(comment.Issue) if comment.Comment != nil && comment.Comment.Review != nil { - reviewComments = make([]*issues_model.Comment, 0, 10) - for _, lines := range comment.Comment.Review.CodeComments { - for _, comments := range lines { - reviewComments = append(reviewComments, comments...) - } - } + reviewComments = comment.Comment.Review.CodeComments.AllComments() } locale := translation.NewLocale(lang) diff --git a/services/pull/merge.go b/services/pull/merge.go index cd9aeb2ad1ed5..d8c8d19416e51 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.Issue.Repo.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/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 +} 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 ee18db38599e8..2fc9c917ac0d7 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -10,18 +10,22 @@ import ( "fmt" "io" "regexp" + "slices" + "sort" "strings" "code.gitea.io/gitea/models/db" issues_model "code.gitea.io/gitea/models/issues" repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" + "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" + "code.gitea.io/gitea/services/gitdiff" notify_service "code.gitea.io/gitea/services/notify" ) @@ -91,12 +95,49 @@ func InvalidateCodeComments(ctx context.Context, prs issues_model.PullRequestLis } // CreateCodeComment creates a comment on the code line -func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git.Repository, issue *issues_model.Issue, line int64, content, treePath string, pendingReview bool, replyReviewID int64, latestCommitID string, attachments []string) (*issues_model.Comment, error) { +func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git.Repository, + issue *issues_model.Issue, line int64, beforeCommitID, afterCommitID, content, treePath string, + pendingReview bool, replyReviewID int64, attachments []string, +) (*issues_model.Comment, error) { var ( existsReview bool err error ) + if gitRepo == nil { + var closer io.Closer + gitRepo, closer, err = gitrepo.RepositoryFromContextOrOpen(ctx, issue.Repo) + if err != nil { + return nil, fmt.Errorf("RepositoryFromContextOrOpen: %w", err) + } + 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) + } + 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) + } + + if beforeCommitID == "" || beforeCommitID == issue.PullRequest.MergeBase { + beforeCommitID = issue.PullRequest.MergeBase + } 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 == "" || 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) + } + // CreateCodeComment() is used for: // - Single comments // - Comments that are part of a review @@ -119,7 +160,10 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. comment, err := createCodeComment(ctx, doer, issue.Repo, + gitRepo, issue, + beforeCommitID, + afterCommitID, content, treePath, line, @@ -151,7 +195,7 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. Reviewer: doer, Issue: issue, Official: false, - CommitID: latestCommitID, + CommitID: afterCommitID, }); err != nil { return nil, err } @@ -160,7 +204,10 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. comment, err := createCodeComment(ctx, doer, issue.Repo, + gitRepo, issue, + beforeCommitID, + afterCommitID, content, treePath, line, @@ -173,7 +220,7 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. if !pendingReview && !existsReview { // Submit the review we've just created so the comment shows up in the issue view - if _, _, err = SubmitReview(ctx, doer, gitRepo, issue, issues_model.ReviewTypeComment, "", latestCommitID, nil); err != nil { + if _, _, err = SubmitReview(ctx, doer, gitRepo, issue, issues_model.ReviewTypeComment, "", afterCommitID, nil); err != nil { return nil, err } } @@ -183,9 +230,16 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. return comment, nil } +func patchCacheKey(issueID int64, beforeCommitID, afterCommitID, treePath string, line int64) string { + // The key is used to cache the patch for a specific line in a review comment. + // It is composed of the issue ID, commit IDs, tree path and line number. + return fmt.Sprintf("review-line-patch-%d-%s-%s-%s-%d", issueID, beforeCommitID, afterCommitID, treePath, line) +} + // 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, attachments []string) (*issues_model.Comment, error) { - var commitID, patch string +func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, gitRepo *git.Repository, + issue *issues_model.Issue, beforeCommitID, afterCommitID, content, treePath string, line, reviewID int64, attachments []string, +) (*issues_model.Comment, error) { if err := issue.LoadPullRequest(ctx); err != nil { return nil, fmt.Errorf("LoadPullRequest: %w", err) } @@ -193,96 +247,55 @@ func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_mo if err := pr.LoadBaseRepo(ctx); err != nil { return nil, fmt.Errorf("LoadBaseRepo: %w", err) } - gitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, pr.BaseRepo) - if err != nil { - return nil, fmt.Errorf("RepositoryFromContextOrOpen: %w", err) - } - defer closer.Close() - - invalidated := false - head := pr.GetGitHeadRefName() - if line > 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) + 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) + 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 { - log.Error("Error whilst generating patch: %v", err) return nil, err } - } - 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: commitID, - ReviewID: reviewID, - Patch: patch, - Invalidated: invalidated, - 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 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 + } + + return comment, nil }) } @@ -328,15 +341,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 +482,176 @@ 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, 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, + 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 { + // 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++ + } + 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, treePath) + if err != nil { + return nil, fmt.Errorf("GetAffectedHunksForTwoCommitsSpecialFile[%s, %s, %s]: %w", repo.FullName(), comment.CommitSHA, dstCommitID, 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) { + 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, + beforeCommit, afterCommit *git.Commit, showOutdatedComments bool, +) error { + if beforeCommit == nil || afterCommit == 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 { + 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 = afterCommit.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 { + 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..65abc0a277fab 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() + beforeCommit, err := gitRepo.GetCommit(issue.PullRequest.MergeBase) + assert.NoError(t, err) + 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, beforeCommit, afterCommit, 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/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 } diff --git a/templates/repo/diff/box.tmpl b/templates/repo/diff/box.tmpl index 22abf9a2193cc..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"}} @@ -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}} 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)() 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}`); } }