Skip to content

Commit 968b2c5

Browse files
lunnylunny
committed
Fix bug
1 parent 65a01c2 commit 968b2c5

File tree

9 files changed

+112
-51
lines changed

9 files changed

+112
-51
lines changed

‎models/issues/comment_test.go

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -71,9 +71,14 @@ func TestFetchCodeComments(t *testing.T) {
7171
res, err := issues_model.FetchCodeComments(db.DefaultContext, issue.Repo, issue.ID, user, false)
7272
assert.NoError(t, err)
7373
assert.Contains(t, res, "README.md")
74-
assert.Contains(t, res["README.md"], int64(4))
75-
assert.Len(t, res["README.md"][4], 1)
76-
assert.Equal(t, int64(4), res["README.md"][0].ID)
74+
fourthLineComments := []*issues_model.Comment{}
75+
for _, comment := range res["README.md"] {
76+
if comment.Line == 4 {
77+
fourthLineComments = append(fourthLineComments, comment)
78+
}
79+
}
80+
assert.Len(t, fourthLineComments, 1)
81+
assert.Equal(t, int64(4), fourthLineComments[0].ID)
7782

7883
user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
7984
res, err = issues_model.FetchCodeComments(db.DefaultContext, issue.Repo, issue.ID, user2, false)

‎models/issues/pull.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -392,11 +392,16 @@ func (pr *PullRequest) getReviewedByLines(ctx context.Context, writer io.Writer)
392392
return committer.Commit()
393393
}
394394

395-
// GetGitHeadRefName returns git ref for hidden pull request branch
395+
// GetGitHeadRefName returns git head commit id ref for the pull request's branch
396396
func (pr *PullRequest) GetGitHeadRefName() string {
397397
return fmt.Sprintf("%s%d/head", git.PullPrefix, pr.Index)
398398
}
399399

400+
// GetGitMergeRefName returns git merged commit id ref for the pull request
401+
func (pr *PullRequest) GetGitMergeRefName() string {
402+
return fmt.Sprintf("%s%d/merge", git.PullPrefix, pr.Index)
403+
}
404+
400405
func (pr *PullRequest) GetGitHeadBranchRefName() string {
401406
return fmt.Sprintf("%s%s", git.BranchPrefix, pr.HeadBranch)
402407
}

‎routers/web/repo/pull_review.go

Lines changed: 39 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,13 @@ import (
77
"errors"
88
"fmt"
99
"net/http"
10+
"slices"
1011

1112
issues_model "code.gitea.io/gitea/models/issues"
1213
"code.gitea.io/gitea/models/organization"
1314
pull_model "code.gitea.io/gitea/models/pull"
1415
user_model "code.gitea.io/gitea/models/user"
16+
"code.gitea.io/gitea/modules/git"
1517
"code.gitea.io/gitea/modules/json"
1618
"code.gitea.io/gitea/modules/log"
1719
"code.gitea.io/gitea/modules/setting"
@@ -170,20 +172,50 @@ func renderConversation(ctx *context.Context, comment *issues_model.Comment, ori
170172
ctx.ServerError("comment.Issue.LoadPullRequest", err)
171173
return
172174
}
173-
if beforeCommitID == "" {
174-
beforeCommitID = comment.Issue.PullRequest.MergeBase
175+
176+
headCommitID, err := ctx.Repo.GitRepo.GetRefCommitID(comment.Issue.PullRequest.GetGitHeadRefName())
177+
if err != nil {
178+
ctx.ServerError("GetRefCommitID", err)
179+
return
175180
}
176-
if afterCommitID == "" {
177-
var err error
178-
afterCommitID, err = ctx.Repo.GitRepo.GetRefCommitID(comment.Issue.PullRequest.GetGitHeadRefName())
181+
182+
prCommitIDs, err := git.CommitIDsBetween(ctx, ctx.Repo.GitRepo.Path, comment.Issue.PullRequest.MergeBase, headCommitID)
183+
if err != nil {
184+
ctx.ServerError("CommitIDsBetween", err)
185+
return
186+
}
187+
188+
if beforeCommitID == "" || beforeCommitID == comment.Issue.PullRequest.MergeBase {
189+
beforeCommitID = comment.Issue.PullRequest.MergeBase
190+
} else {
191+
// beforeCommitID must be one of the pull request commits
192+
if !slices.Contains(prCommitIDs, beforeCommitID) {
193+
ctx.HTTPError(http.StatusBadRequest, fmt.Sprintf("beforeCommitID[%s] is not a valid pull request commit", beforeCommitID))
194+
return
195+
}
196+
197+
beforeCommit, err := ctx.Repo.GitRepo.GetCommit(beforeCommitID)
179198
if err != nil {
180-
ctx.ServerError("GetRefCommitID", err)
199+
ctx.ServerError("GetCommit", err)
181200
return
182201
}
202+
203+
beforeCommit, err = beforeCommit.Parent(0)
204+
if err != nil {
205+
ctx.ServerError("GetParent", err)
206+
return
207+
}
208+
beforeCommitID = beforeCommit.ID.String()
209+
}
210+
if afterCommitID == "" {
211+
afterCommitID = headCommitID
212+
} else if !slices.Contains(prCommitIDs, afterCommitID) { // afterCommitID must be one of the pull request commits
213+
ctx.HTTPError(http.StatusBadRequest, fmt.Sprintf("afterCommitID[%s] is not a valid pull request commit", afterCommitID))
214+
return
183215
}
184216

185217
showOutdatedComments := origin == "timeline" || ctx.Data["ShowOutdatedComments"].(bool)
186-
comments, err := pull_service.FetchCodeCommentsByLine(ctx, ctx.Repo.Repository, comment.IssueID,
218+
comments, err := pull_service.FetchCodeCommentsByLine(ctx, ctx.Repo.GitRepo, ctx.Repo.Repository, comment.IssueID,
187219
ctx.Doer, beforeCommitID, afterCommitID, comment.TreePath, comment.Line, showOutdatedComments)
188220
if err != nil {
189221
ctx.ServerError("FetchCodeCommentsByLine", err)

‎services/automergequeue/automergequeue.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,11 +39,11 @@ func StartPRCheckAndAutoMerge(ctx context.Context, pull *issues_model.PullReques
3939
return
4040
}
4141
defer gitRepo.Close()
42-
commitID, err := gitRepo.GetRefCommitID(pull.GetGitHeadRefName())
42+
headCommitID, err := gitRepo.GetRefCommitID(pull.GetGitHeadRefName())
4343
if err != nil {
4444
log.Error("GetRefCommitID: %v", err)
4545
return
4646
}
4747

48-
AddToQueue(pull, commitID)
48+
AddToQueue(pull, headCommitID)
4949
}

‎services/convert/pull_review.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,10 @@ func ToPullReview(ctx context.Context, r *issues_model.Review, doer *user_model.
2121
r.Reviewer = user_model.NewGhostUser()
2222
}
2323

24+
if err := r.Issue.LoadRepo(ctx); err != nil {
25+
return nil, err
26+
}
27+
2428
result := &api.PullReview{
2529
ID: r.ID,
2630
Reviewer: ToUser(ctx, r.Reviewer, doer),

‎services/issue/issue.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -204,6 +204,11 @@ func DeleteIssue(ctx context.Context, doer *user_model.User, gitRepo *git.Reposi
204204
if err := git.RemoveRef(ctx, gitRepo.Path, issue.PullRequest.GetGitHeadRefName()); err != nil {
205205
return err
206206
}
207+
if issue.PullRequest.HasMerged {
208+
if err := git.RemoveRef(ctx, gitRepo.Path, issue.PullRequest.GetGitMergeRefName()); err != nil {
209+
return err
210+
}
211+
}
207212
}
208213

209214
notify_service.DeleteIssue(ctx, doer, issue)

‎services/issue/pull.go

Lines changed: 1 addition & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -7,33 +7,15 @@ import (
77
"context"
88
"fmt"
99
"slices"
10-
"time"
1110

1211
issues_model "code.gitea.io/gitea/models/issues"
1312
org_model "code.gitea.io/gitea/models/organization"
1413
user_model "code.gitea.io/gitea/models/user"
15-
"code.gitea.io/gitea/modules/git"
1614
"code.gitea.io/gitea/modules/gitrepo"
1715
"code.gitea.io/gitea/modules/log"
1816
"code.gitea.io/gitea/modules/setting"
1917
)
2018

21-
func getMergeBase(repo *git.Repository, pr *issues_model.PullRequest, baseBranch, headBranch string) (string, error) {
22-
// Add a temporary remote
23-
tmpRemote := fmt.Sprintf("mergebase-%d-%d", pr.ID, time.Now().UnixNano())
24-
if err := repo.AddRemote(tmpRemote, repo.Path, false); err != nil {
25-
return "", fmt.Errorf("AddRemote: %w", err)
26-
}
27-
defer func() {
28-
if err := repo.RemoveRemote(tmpRemote); err != nil {
29-
log.Error("getMergeBase: RemoveRemote: %v", err)
30-
}
31-
}()
32-
33-
mergeBase, _, err := repo.GetMergeBase(tmpRemote, baseBranch, headBranch)
34-
return mergeBase, err
35-
}
36-
3719
type ReviewRequestNotifier struct {
3820
Comment *issues_model.Comment
3921
IsAdd bool
@@ -96,15 +78,9 @@ func PullRequestCodeOwnersReview(ctx context.Context, pr *issues_model.PullReque
9678
return nil, nil
9779
}
9880

99-
// get the mergebase
100-
mergeBase, err := getMergeBase(repo, pr, git.BranchPrefix+pr.BaseBranch, pr.GetGitHeadRefName())
101-
if err != nil {
102-
return nil, err
103-
}
104-
10581
// https://github.com/go-gitea/gitea/issues/29763, we need to get the files changed
10682
// between the merge base and the head commit but not the base branch and the head commit
107-
changedFiles, err := repo.GetFilesChangedBetween(mergeBase, pr.GetGitHeadRefName())
83+
changedFiles, err := repo.GetFilesChangedBetween(pr.MergeBase, pr.GetGitHeadRefName())
10884
if err != nil {
10985
return nil, err
11086
}

‎services/pull/merge.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -726,6 +726,12 @@ func SetMerged(ctx context.Context, pr *issues_model.PullRequest, mergedCommitID
726726
return false, issues_model.ErrIssueAlreadyChanged
727727
}
728728

729+
// update merge ref, this is necessary to ensure pr.MergedCommitID can be used to do diff operations even
730+
// if the repository rebased/force-pushed and the pull request's merge commit is no longer in the history
731+
if err := git.UpdateRef(ctx, pr.BaseRepo.RepoPath(), pr.GetGitMergeRefName(), pr.MergedCommitID); err != nil {
732+
return false, fmt.Errorf("UpdateRef: %w", err)
733+
}
734+
729735
if err := committer.Commit(); err != nil {
730736
return false, err
731737
}

‎services/pull/review.go

Lines changed: 41 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,12 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git.
113113
defer closer.Close()
114114
}
115115

116-
prCommitIDs, err := git.CommitIDsBetween(ctx, gitRepo.Path, beforeCommitID, afterCommitID)
116+
headCommitID, err := gitRepo.GetRefCommitID(issue.PullRequest.GetGitHeadRefName())
117+
if err != nil {
118+
return nil, fmt.Errorf("GetRefCommitID[%s]: %w", issue.PullRequest.GetGitHeadRefName(), err)
119+
}
120+
121+
prCommitIDs, err := git.CommitIDsBetween(ctx, gitRepo.Path, issue.PullRequest.MergeBase, headCommitID)
117122
if err != nil {
118123
return nil, fmt.Errorf("CommitIDsBetween[%s, %s]: %w", beforeCommitID, afterCommitID, err)
119124
}
@@ -138,10 +143,7 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git.
138143
beforeCommitID = beforeCommit.ID.String()
139144
}
140145
if afterCommitID == "" {
141-
afterCommitID, err = gitRepo.GetRefCommitID(issue.PullRequest.GetGitHeadRefName())
142-
if err != nil {
143-
return nil, fmt.Errorf("GetRefCommitID[%s]: %w", issue.PullRequest.GetGitHeadRefName(), err)
144-
}
146+
afterCommitID = headCommitID
145147
} else if !slices.Contains(prCommitIDs, afterCommitID) { // afterCommitID must be one of the pull request commits
146148
return nil, fmt.Errorf("afterCommitID[%s] is not a valid pull request commit", afterCommitID)
147149
}
@@ -520,7 +522,7 @@ func ReCalculateLineNumber(hunks []*git.HunkInfo, leftLine int64) int64 {
520522
}
521523

522524
// FetchCodeCommentsByLine fetches the code comments for a given commit, treePath and line number of a pull request.
523-
func FetchCodeCommentsByLine(ctx context.Context, repo *repo_model.Repository, issueID int64, currentUser *user_model.User, startCommitID, endCommitID, treePath string, line int64, showOutdatedComments bool) (issues_model.CommentList, error) {
525+
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) {
524526
opts := issues_model.FindCommentsOptions{
525527
Type: issues_model.CommentTypeCode,
526528
IssueID: issueID,
@@ -538,7 +540,24 @@ func FetchCodeCommentsByLine(ctx context.Context, repo *repo_model.Repository, i
538540
n := 0
539541
hunksCache := make(map[string][]*git.HunkInfo)
540542
for _, comment := range comments {
541-
if comment.CommitSHA == endCommitID {
543+
// Code comment should always have a commit SHA, if not, we need to set it based on the line number
544+
if comment.CommitSHA == "" {
545+
if comment.Line > 0 {
546+
comment.CommitSHA = endCommitID
547+
} else if comment.Line < 0 {
548+
comment.CommitSHA = startCommitID
549+
} else {
550+
// If the comment has no line number, we cannot display it in the diff view
551+
continue
552+
}
553+
}
554+
555+
dstCommitID := startCommitID
556+
if comment.Line > 0 {
557+
dstCommitID = endCommitID
558+
}
559+
560+
if comment.CommitSHA == dstCommitID {
542561
if comment.Line == line {
543562
comments[n] = comment
544563
n++
@@ -547,18 +566,27 @@ func FetchCodeCommentsByLine(ctx context.Context, repo *repo_model.Repository, i
547566
}
548567

549568
// If the comment is not for the current commit, we need to recalculate the line number
550-
hunks, ok := hunksCache[comment.CommitSHA]
569+
hunks, ok := hunksCache[comment.CommitSHA+".."+dstCommitID]
551570
if !ok {
552-
hunks, err = git.GetAffectedHunksForTwoCommitsSpecialFile(ctx, repo.RepoPath(), comment.CommitSHA, endCommitID, treePath)
571+
hunks, err = git.GetAffectedHunksForTwoCommitsSpecialFile(ctx, repo.RepoPath(), comment.CommitSHA, dstCommitID, treePath)
553572
if err != nil {
554-
return nil, fmt.Errorf("GetAffectedHunksForTwoCommitsSpecialFile[%s, %s, %s]: %w", repo.FullName(), comment.CommitSHA, endCommitID, err)
573+
return nil, fmt.Errorf("GetAffectedHunksForTwoCommitsSpecialFile[%s, %s, %s]: %w", repo.FullName(), comment.CommitSHA, dstCommitID, err)
555574
}
556-
hunksCache[comment.CommitSHA] = hunks
575+
hunksCache[comment.CommitSHA+".."+dstCommitID] = hunks
557576
}
558577

559578
comment.Line = ReCalculateLineNumber(hunks, comment.Line)
560-
comments[n] = comment
561-
n++
579+
if comment.Line != 0 {
580+
dstCommit, err := gitRepo.GetCommit(dstCommitID)
581+
if err != nil {
582+
return nil, fmt.Errorf("GetCommit[%s]: %w", dstCommitID, err)
583+
}
584+
// If the comment is not the first one or the comment created before the current commit
585+
if n > 0 || comment.CreatedUnix.AsTime().Before(dstCommit.Committer.When) {
586+
comments[n] = comment
587+
n++
588+
}
589+
}
562590
}
563591
return comments[:n], nil
564592
}

0 commit comments

Comments
 (0)