Skip to content

Commit 1fb7b01

Browse files
lunnylunny
committed
improvements
1 parent 105179a commit 1fb7b01

File tree

5 files changed

+58
-63
lines changed

5 files changed

+58
-63
lines changed

‎models/migrations/v1_25/v321.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,6 @@ func AddBeforeCommitIDForComment(x *xorm.Engine) error {
2323
}, new(comment)); err != nil {
2424
return err
2525
}
26-
_, err := x.Exec("UPDATE comment SET before_commit_id = (SELECT merge_base FROM pull_request WHERE pull_request.issue_id = comment.issue_id) WHERE before_commit_id IS NULL")
26+
_, err := x.Exec("UPDATE comment SET before_commit_id = (SELECT merge_base FROM pull_request WHERE pull_request.issue_id = comment.issue_id) WHERE `type`=21 AND before_commit_id IS NULL")
2727
return err
2828
}

‎services/issue/comments.go‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ func UpdateComment(ctx context.Context, c *issues_model.Comment, contentVersion
134134

135135
// DeleteComment deletes the comment
136136
func DeleteComment(ctx context.Context, doer *user_model.User, comment *issues_model.Comment) error {
137-
if comment.ReviewID > 0 {
137+
if comment.Type == issues_model.CommentTypeCode {
138138
if err := comment.LoadIssue(ctx); err != nil {
139139
return err
140140
}
@@ -152,18 +152,15 @@ func DeleteComment(ctx context.Context, doer *user_model.User, comment *issues_m
152152
return err
153153
}
154154

155-
if comment.ReviewID > 0 {
155+
if comment.Type == issues_model.CommentTypeCode {
156+
// We should not return error here, because the comment has been removed from database.
157+
// users have to delete this ref manually or we should have a synchronize between
158+
// database comment table and git refs.
156159
if err := git.RemoveRef(ctx, comment.Issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(comment.Issue.PullRequest.Index, comment.ID, "before")); err != nil {
157160
log.Error("Unable to remove ref in base repository for PR[%d] Error: %v", comment.Issue.PullRequest.ID, err)
158-
// We should not return error here, because the comment has been removed from database.
159-
// users have to delete this ref manually or we should have a synchronize between
160-
// database comment table and git refs.
161161
}
162162
if err := git.RemoveRef(ctx, comment.Issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(comment.Issue.PullRequest.Index, comment.ID, "after")); err != nil {
163163
log.Error("Unable to remove ref in base repository for PR[%d] Error: %v", comment.Issue.PullRequest.ID, err)
164-
// We should not return error here, because the comment has been removed from database.
165-
// users have to delete this ref manually or we should have a synchronize between
166-
// database comment table and git refs.
167164
}
168165
}
169166

‎services/issue/issue.go‎

Lines changed: 23 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,7 @@ func DeleteIssue(ctx context.Context, doer *user_model.User, gitRepo *git.Reposi
191191
}
192192

193193
// delete entries in database
194-
attachmentPaths, comments, err := deleteIssue(ctx, issue)
194+
attachmentPaths, err := deleteIssue(ctx, issue)
195195
if err != nil {
196196
return err
197197
}
@@ -211,7 +211,7 @@ func DeleteIssue(ctx context.Context, doer *user_model.User, gitRepo *git.Reposi
211211
}
212212
}
213213

214-
for _, comment := range comments {
214+
for _, comment := range issue.Comments {
215215
if comment.Type == issues_model.CommentTypeCode {
216216
if err := git.RemoveRef(ctx, issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID, "before")); err != nil {
217217
log.Error("Unable to remove ref %s in base repository for PR[%d] Error: %v", issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID, "before"), issue.PullRequest.ID, err)
@@ -277,48 +277,48 @@ func GetRefEndNamesAndURLs(issues []*issues_model.Issue, repoLink string) (map[i
277277
}
278278

279279
// deleteIssue deletes the issue
280-
func deleteIssue(ctx context.Context, issue *issues_model.Issue) ([]string, []*issues_model.Comment, error) {
281-
var attachmentPaths []string
282-
if err := db.WithTx(ctx, func(ctx context.Context) error {
280+
func deleteIssue(ctx context.Context, issue *issues_model.Issue) ([]string, error) {
281+
return db.WithTx2(ctx, func(ctx context.Context) ([]string, error) {
283282
// update the total issue numbers
284283
if err := repo_model.UpdateRepoIssueNumbers(ctx, issue.RepoID, issue.IsPull, false); err != nil {
285-
return err
284+
return nil, err
286285
}
287286
// if the issue is closed, update the closed issue numbers
288287
if issue.IsClosed {
289288
if err := repo_model.UpdateRepoIssueNumbers(ctx, issue.RepoID, issue.IsPull, true); err != nil {
290-
return err
289+
return nil, err
291290
}
292291
}
293292

294293
if err := issues_model.UpdateMilestoneCounters(ctx, issue.MilestoneID); err != nil {
295-
return fmt.Errorf("error updating counters for milestone id %d: %w",
294+
return nil, fmt.Errorf("error updating counters for milestone id %d: %w",
296295
issue.MilestoneID, err)
297296
}
298297

299298
if err := activities_model.DeleteIssueActions(ctx, issue.RepoID, issue.ID, issue.Index); err != nil {
300-
return err
299+
return nil, err
301300
}
302301

303302
// find attachments related to this issue and remove them
304303
if err := issue.LoadAttachments(ctx); err != nil {
305-
return err
304+
return nil, err
306305
}
307306

307+
var attachmentPaths []string
308308
for i := range issue.Attachments {
309309
attachmentPaths = append(attachmentPaths, issue.Attachments[i].RelativePath())
310310
}
311311

312312
// deference all review comments
313313
if err := issue.LoadRepo(ctx); err != nil {
314-
return err
314+
return nil, err
315315
}
316316
if err := issue.LoadPullRequest(ctx); err != nil {
317-
return err
317+
return nil, err
318318
}
319319

320320
if err := issue.LoadComments(ctx); err != nil {
321-
return err
321+
return nil, err
322322
}
323323

324324
// delete all database data still assigned to this issue
@@ -341,12 +341,12 @@ func deleteIssue(ctx context.Context, issue *issues_model.Issue) ([]string, []*i
341341
&issues_model.Comment{DependentIssueID: issue.ID},
342342
&issues_model.IssuePin{IssueID: issue.ID},
343343
); err != nil {
344-
return err
344+
return nil, err
345345
}
346346

347347
for _, comment := range issue.Comments {
348348
if err := issues_model.DeleteComment(ctx, comment); err != nil {
349-
return err
349+
return nil, err
350350
}
351351
}
352352

@@ -355,29 +355,25 @@ func deleteIssue(ctx context.Context, issue *issues_model.Issue) ([]string, []*i
355355
// Delete scheduled auto merges
356356
if _, err := db.GetEngine(ctx).Where("pull_id=?", issue.PullRequest.ID).
357357
Delete(&pull_model.AutoMerge{}); err != nil {
358-
return err
358+
return nil, err
359359
}
360360

361361
// Delete review states
362362
if _, err := db.GetEngine(ctx).Where("pull_id=?", issue.PullRequest.ID).
363363
Delete(&pull_model.ReviewState{}); err != nil {
364-
return err
364+
return nil, err
365365
}
366366

367367
if _, err := db.GetEngine(ctx).ID(issue.PullRequest.ID).Delete(&issues_model.PullRequest{}); err != nil {
368-
return err
368+
return nil, err
369369
}
370370
}
371371

372372
if _, err := db.GetEngine(ctx).ID(issue.ID).NoAutoCondition().Delete(issue); err != nil {
373-
return err
373+
return nil, err
374374
}
375-
return nil
376-
}); err != nil {
377-
return nil, nil, err
378-
}
379-
380-
return attachmentPaths, issue.Comments, nil
375+
return attachmentPaths, nil
376+
})
381377
}
382378

383379
// DeleteOrphanedIssues delete issues without a repo
@@ -425,12 +421,12 @@ func DeleteIssuesByRepoID(ctx context.Context, repoID int64) (attachmentPaths []
425421
}
426422

427423
for _, issue := range issues {
428-
issueAttachPaths, comments, err := deleteIssue(ctx, issue)
424+
issueAttachPaths, err := deleteIssue(ctx, issue)
429425
if err != nil {
430426
return nil, fmt.Errorf("deleteIssue [issue_id: %d]: %w", issue.ID, err)
431427
}
432428

433-
for _, comment := range comments {
429+
for _, comment := range issue.Comments {
434430
if comment.Type == issues_model.CommentTypeCode {
435431
if err := git.RemoveRef(ctx, issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID, "before")); err != nil {
436432
log.Error("Unable to remove ref %s in base repository for PR[%d] Error: %v", issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID, "before"), issue.PullRequest.ID, err)

‎services/issue/issue_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ func TestIssue_DeleteIssue(t *testing.T) {
4141

4242
issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: issueIDs[2]})
4343

44-
_, _, err = deleteIssue(db.DefaultContext, issue)
44+
_, err = deleteIssue(db.DefaultContext, issue)
4545
assert.NoError(t, err)
4646
issueIDs, err = issues_model.GetIssueIDsByRepoID(db.DefaultContext, 1)
4747
assert.NoError(t, err)
@@ -52,7 +52,7 @@ func TestIssue_DeleteIssue(t *testing.T) {
5252
assert.NoError(t, err)
5353
issue, err = issues_model.GetIssueByID(db.DefaultContext, 4)
5454
assert.NoError(t, err)
55-
_, _, err = deleteIssue(db.DefaultContext, issue)
55+
_, err = deleteIssue(db.DefaultContext, issue)
5656
assert.NoError(t, err)
5757
assert.Len(t, attachments, 2)
5858
for i := range attachments {
@@ -75,7 +75,7 @@ func TestIssue_DeleteIssue(t *testing.T) {
7575
assert.NoError(t, err)
7676
assert.False(t, left)
7777

78-
_, _, err = deleteIssue(db.DefaultContext, issue2)
78+
_, err = deleteIssue(db.DefaultContext, issue2)
7979
assert.NoError(t, err)
8080
left, err = issues_model.IssueNoDependenciesLeft(db.DefaultContext, issue1)
8181
assert.NoError(t, err)

‎services/pull/review.go‎

Lines changed: 26 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -518,7 +518,7 @@ func ReCalculateLineNumber(hunks []*git.HunkInfo, leftLine int64) int64 {
518518
}
519519

520520
// FetchCodeCommentsByLine fetches the code comments for a given commit, treePath and line number of a pull request.
521-
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) {
521+
func FetchCodeCommentsByLine(ctx context.Context, gitRepo *git.Repository, repo *repo_model.Repository, issueID int64, currentUser *user_model.User, beforeCommitID, afterCommitID, treePath string, line int64, showOutdatedComments bool) (issues_model.CommentList, error) {
522522
opts := issues_model.FindCommentsOptions{
523523
Type: issues_model.CommentTypeCode,
524524
IssueID: issueID,
@@ -533,52 +533,54 @@ func FetchCodeCommentsByLine(ctx context.Context, gitRepo *git.Repository, repo
533533
if len(comments) == 0 {
534534
return nil, nil
535535
}
536+
afterCommit, err := gitRepo.GetCommit(afterCommitID)
537+
if err != nil {
538+
return nil, fmt.Errorf("GetCommit[%s]: %w", afterCommitID, err)
539+
}
536540
n := 0
537541
hunksCache := make(map[string][]*git.HunkInfo)
538542
for _, comment := range comments {
539543
// Code comment should always have a commit SHA, if not, we need to set it based on the line number
540544
if comment.CommitSHA == "" {
541545
if comment.Line > 0 {
542-
comment.CommitSHA = endCommitID
546+
comment.CommitSHA = afterCommitID
543547
} else if comment.Line < 0 {
544-
comment.CommitSHA = startCommitID
548+
comment.CommitSHA = beforeCommitID
545549
} else {
546550
// If the comment has no line number, we cannot display it in the diff view
547551
continue
548552
}
549553
}
550554

551-
dstCommitID := startCommitID
555+
dstCommitID := beforeCommitID
556+
commentCommitID := comment.BeforeCommitID
552557
if comment.Line > 0 {
553-
dstCommitID = endCommitID
558+
dstCommitID = afterCommitID
559+
commentCommitID = comment.CommitSHA
554560
}
555561

556-
if comment.CommitSHA == dstCommitID {
557-
if comment.Line == line {
558-
comments[n] = comment
559-
n++
562+
if commentCommitID != dstCommitID {
563+
// If the comment is not for the current commit, we need to recalculate the line number
564+
hunks, ok := hunksCache[commentCommitID+".."+dstCommitID]
565+
if !ok {
566+
hunks, err = git.GetAffectedHunksForTwoCommitsSpecialFile(ctx, repo.RepoPath(), commentCommitID, dstCommitID, treePath)
567+
if err != nil {
568+
return nil, fmt.Errorf("GetAffectedHunksForTwoCommitsSpecialFile[%s, %s, %s]: %w", repo.FullName(), commentCommitID, dstCommitID, err)
569+
}
570+
hunksCache[commentCommitID+".."+dstCommitID] = hunks
560571
}
561-
continue
572+
comment.Line = ReCalculateLineNumber(hunks, comment.Line)
562573
}
563574

564-
// If the comment is not for the current commit, we need to recalculate the line number
565-
hunks, ok := hunksCache[comment.CommitSHA+".."+dstCommitID]
566-
if !ok {
567-
hunks, err = git.GetAffectedHunksForTwoCommitsSpecialFile(ctx, repo.RepoPath(), comment.CommitSHA, dstCommitID, treePath)
575+
if comment.Line == line {
576+
commentAfterCommit, err := gitRepo.GetCommit(comment.CommitSHA)
568577
if err != nil {
569-
return nil, fmt.Errorf("GetAffectedHunksForTwoCommitsSpecialFile[%s, %s, %s]: %w", repo.FullName(), comment.CommitSHA, dstCommitID, err)
578+
return nil, fmt.Errorf("GetCommit[%s]: %w", comment.CommitSHA, err)
570579
}
571-
hunksCache[comment.CommitSHA+".."+dstCommitID] = hunks
572-
}
573580

574-
comment.Line = ReCalculateLineNumber(hunks, comment.Line)
575-
if comment.Line != 0 {
576-
dstCommit, err := gitRepo.GetCommit(dstCommitID)
577-
if err != nil {
578-
return nil, fmt.Errorf("GetCommit[%s]: %w", dstCommitID, err)
579-
}
580581
// If the comment is not the first one or the comment created before the current commit
581-
if n > 0 || comment.CreatedUnix.AsTime().Before(dstCommit.Committer.When) {
582+
if n > 0 || comment.CommitSHA == afterCommitID ||
583+
commentAfterCommit.Committer.When.Before(afterCommit.Committer.When) {
582584
comments[n] = comment
583585
n++
584586
}

0 commit comments

Comments
 (0)