improvements

models/migrations/v1_25/v321.go

@@ -23,6 +23,6 @@ func AddBeforeCommitIDForComment(x *xorm.Engine) error {
 	}, new(comment)); err != nil {
 		return err
 	}
-	_, err := x.Exec("UPDATE comment SET before_commit_id = (SELECT merge_base FROM pull_request WHERE pull_request.issue_id = comment.issue_id) WHERE before_commit_id IS NULL")
+	_, err := x.Exec("UPDATE comment SET before_commit_id = (SELECT merge_base FROM pull_request WHERE pull_request.issue_id = comment.issue_id) WHERE `type`=21 AND before_commit_id IS NULL")
 	return err
 }

services/issue/comments.go

@@ -134,7 +134,7 @@ func UpdateComment(ctx context.Context, c *issues_model.Comment, contentVersion
 
 // DeleteComment deletes the comment
 func DeleteComment(ctx context.Context, doer *user_model.User, comment *issues_model.Comment) error {
-	if comment.ReviewID > 0 {
+	if comment.Type == issues_model.CommentTypeCode {
 		if err := comment.LoadIssue(ctx); err != nil {
 			return err
 		}
@@ -152,18 +152,15 @@ func DeleteComment(ctx context.Context, doer *user_model.User, comment *issues_m
 		return err
 	}
 
-	if comment.ReviewID > 0 {
+	if comment.Type == issues_model.CommentTypeCode {
+		// We should not return error here, because the comment has been removed from database.
+		// users have to delete this ref manually or we should have a synchronize between
+		// database comment table and git refs.
 		if err := git.RemoveRef(ctx, comment.Issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(comment.Issue.PullRequest.Index, comment.ID, "before")); err != nil {
 			log.Error("Unable to remove ref in base repository for PR[%d] Error: %v", comment.Issue.PullRequest.ID, err)
-			// We should not return error here, because the comment has been removed from database.
-			// users have to delete this ref manually or we should have a synchronize between
-			// database comment table and git refs.
 		}
 		if err := git.RemoveRef(ctx, comment.Issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(comment.Issue.PullRequest.Index, comment.ID, "after")); err != nil {
 			log.Error("Unable to remove ref in base repository for PR[%d] Error: %v", comment.Issue.PullRequest.ID, err)
-			// We should not return error here, because the comment has been removed from database.
-			// users have to delete this ref manually or we should have a synchronize between
-			// database comment table and git refs.
 		}
 	}
 

services/issue/issue.go

@@ -191,7 +191,7 @@ func DeleteIssue(ctx context.Context, doer *user_model.User, gitRepo *git.Reposi
 	}
 
 	// delete entries in database
-	attachmentPaths, comments, err := deleteIssue(ctx, issue)
+	attachmentPaths, err := deleteIssue(ctx, issue)
 	if err != nil {
 		return err
 	}
@@ -211,7 +211,7 @@ func DeleteIssue(ctx context.Context, doer *user_model.User, gitRepo *git.Reposi
 		}
 	}
 
-	for _, comment := range comments {
+	for _, comment := range issue.Comments {
 		if comment.Type == issues_model.CommentTypeCode {
 			if err := git.RemoveRef(ctx, issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID, "before")); err != nil {
 				log.Error("Unable to remove ref %s in base repository for PR[%d] Error: %v", issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID, "before"), issue.PullRequest.ID, err)
@@ -277,48 +277,48 @@ func GetRefEndNamesAndURLs(issues []*issues_model.Issue, repoLink string) (map[i
 }
 
 // deleteIssue deletes the issue
-func deleteIssue(ctx context.Context, issue *issues_model.Issue) ([]string, []*issues_model.Comment, error) {
+func deleteIssue(ctx context.Context, issue *issues_model.Issue) ([]string, error) {
-	var attachmentPaths []string
+	return db.WithTx2(ctx, func(ctx context.Context) ([]string, error) {
-	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
+			return nil, err
 		}
 		// if the issue is closed, update the closed issue numbers
 		if issue.IsClosed {
 			if err := repo_model.UpdateRepoIssueNumbers(ctx, issue.RepoID, issue.IsPull, true); err != nil {
-				return err
+				return nil, err
 			}
 		}
 
 		if err := issues_model.UpdateMilestoneCounters(ctx, issue.MilestoneID); err != nil {
-			return fmt.Errorf("error updating counters for milestone id %d: %w",
+			return nil, fmt.Errorf("error updating counters for milestone id %d: %w",
 				issue.MilestoneID, err)
 		}
 
 		if err := activities_model.DeleteIssueActions(ctx, issue.RepoID, issue.ID, issue.Index); err != nil {
-			return err
+			return nil, err
 		}
 
 		// find attachments related to this issue and remove them
 		if err := issue.LoadAttachments(ctx); err != nil {
-			return err
+			return nil, err
 		}
 
+		var attachmentPaths []string
 		for i := range issue.Attachments {
 			attachmentPaths = append(attachmentPaths, issue.Attachments[i].RelativePath())
 		}
 
 		// deference all review comments
 		if err := issue.LoadRepo(ctx); err != nil {
-			return err
+			return nil, err
 		}
 		if err := issue.LoadPullRequest(ctx); err != nil {
-			return err
+			return nil, err
 		}
 
 		if err := issue.LoadComments(ctx); err != nil {
-			return err
+			return nil, err
 		}
 
 		// delete all database data still assigned to this issue
@@ -341,12 +341,12 @@ func deleteIssue(ctx context.Context, issue *issues_model.Issue) ([]string, []*i
 			&issues_model.Comment{DependentIssueID: issue.ID},
 			&issues_model.IssuePin{IssueID: issue.ID},
 		); err != nil {
-			return err
+			return nil, err
 		}
 
 		for _, comment := range issue.Comments {
 			if err := issues_model.DeleteComment(ctx, comment); err != nil {
-				return err
+				return nil, err
 			}
 		}
 
@@ -355,29 +355,25 @@ func deleteIssue(ctx context.Context, issue *issues_model.Issue) ([]string, []*i
 			// Delete scheduled auto merges
 			if _, err := db.GetEngine(ctx).Where("pull_id=?", issue.PullRequest.ID).
 				Delete(&pull_model.AutoMerge{}); err != nil {
-				return err
+				return nil, err
 			}
 
 			// Delete review states
 			if _, err := db.GetEngine(ctx).Where("pull_id=?", issue.PullRequest.ID).
 				Delete(&pull_model.ReviewState{}); err != nil {
-				return err
+				return nil, err
 			}
 
 			if _, err := db.GetEngine(ctx).ID(issue.PullRequest.ID).Delete(&issues_model.PullRequest{}); err != nil {
-				return err
+				return nil, err
 			}
 		}
 
 		if _, err := db.GetEngine(ctx).ID(issue.ID).NoAutoCondition().Delete(issue); err != nil {
-			return err
+			return nil, err
 		}
-		return nil
+		return attachmentPaths, nil
-	}); err != nil {
+	})
-		return nil, nil, err
-	}
-
-	return attachmentPaths, issue.Comments, nil
 }
 
 // DeleteOrphanedIssues delete issues without a repo
@@ -425,12 +421,12 @@ func DeleteIssuesByRepoID(ctx context.Context, repoID int64) (attachmentPaths []
 		}
 
 		for _, issue := range issues {
-			issueAttachPaths, comments, err := deleteIssue(ctx, issue)
+			issueAttachPaths, err := deleteIssue(ctx, issue)
 			if err != nil {
 				return nil, fmt.Errorf("deleteIssue [issue_id: %d]: %w", issue.ID, err)
 			}
 
-			for _, comment := range comments {
+			for _, comment := range issue.Comments {
 				if comment.Type == issues_model.CommentTypeCode {
 					if err := git.RemoveRef(ctx, issue.Repo.RepoPath(), issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID, "before")); err != nil {
 						log.Error("Unable to remove ref %s in base repository for PR[%d] Error: %v", issues_model.GetCodeCommentRefName(issue.PullRequest.Index, comment.ID, "before"), issue.PullRequest.ID, err)

services/issue/issue_test.go

@@ -41,7 +41,7 @@ func TestIssue_DeleteIssue(t *testing.T) {
 
 	issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: issueIDs[2]})
 
-	_, _, err = deleteIssue(db.DefaultContext, issue)
+	_, err = deleteIssue(db.DefaultContext, issue)
 	assert.NoError(t, err)
 	issueIDs, err = issues_model.GetIssueIDsByRepoID(db.DefaultContext, 1)
 	assert.NoError(t, err)
@@ -52,7 +52,7 @@ func TestIssue_DeleteIssue(t *testing.T) {
 	assert.NoError(t, err)
 	issue, err = issues_model.GetIssueByID(db.DefaultContext, 4)
 	assert.NoError(t, err)
-	_, _, err = deleteIssue(db.DefaultContext, issue)
+	_, err = deleteIssue(db.DefaultContext, issue)
 	assert.NoError(t, err)
 	assert.Len(t, attachments, 2)
 	for i := range attachments {
@@ -75,7 +75,7 @@ func TestIssue_DeleteIssue(t *testing.T) {
 	assert.NoError(t, err)
 	assert.False(t, left)
 
-	_, _, err = deleteIssue(db.DefaultContext, issue2)
+	_, err = deleteIssue(db.DefaultContext, issue2)
 	assert.NoError(t, err)
 	left, err = issues_model.IssueNoDependenciesLeft(db.DefaultContext, issue1)
 	assert.NoError(t, err)

services/pull/review.go

@@ -518,7 +518,7 @@ func ReCalculateLineNumber(hunks []*git.HunkInfo, leftLine int64) int64 {
 }
 
 // FetchCodeCommentsByLine fetches the code comments for a given commit, treePath and line number of a pull request.
-func FetchCodeCommentsByLine(ctx context.Context, gitRepo *git.Repository, repo *repo_model.Repository, issueID int64, currentUser *user_model.User, startCommitID, endCommitID, treePath string, line int64, showOutdatedComments bool) (issues_model.CommentList, error) {
+func FetchCodeCommentsByLine(ctx context.Context, gitRepo *git.Repository, repo *repo_model.Repository, issueID int64, currentUser *user_model.User, beforeCommitID, afterCommitID, treePath string, line int64, showOutdatedComments bool) (issues_model.CommentList, error) {
 	opts := issues_model.FindCommentsOptions{
 		Type:     issues_model.CommentTypeCode,
 		IssueID:  issueID,
@@ -533,52 +533,54 @@ func FetchCodeCommentsByLine(ctx context.Context, gitRepo *git.Repository, repo
 	if len(comments) == 0 {
 		return nil, nil
 	}
+	afterCommit, err := gitRepo.GetCommit(afterCommitID)
+	if err != nil {
+		return nil, fmt.Errorf("GetCommit[%s]: %w", afterCommitID, err)
+	}
 	n := 0
 	hunksCache := make(map[string][]*git.HunkInfo)
 	for _, comment := range comments {
 		// Code comment should always have a commit SHA, if not, we need to set it based on the line number
 		if comment.CommitSHA == "" {
 			if comment.Line > 0 {
-				comment.CommitSHA = endCommitID
+				comment.CommitSHA = afterCommitID
 			} else if comment.Line < 0 {
-				comment.CommitSHA = startCommitID
+				comment.CommitSHA = beforeCommitID
 			} else {
 				// If the comment has no line number, we cannot display it in the diff view
 				continue
 			}
 		}
 
-		dstCommitID := startCommitID
+		dstCommitID := beforeCommitID
+		commentCommitID := comment.BeforeCommitID
 		if comment.Line > 0 {
-			dstCommitID = endCommitID
+			dstCommitID = afterCommitID
+			commentCommitID = comment.CommitSHA
 		}
 
-		if comment.CommitSHA == dstCommitID {
+		if commentCommitID != dstCommitID {
-			if comment.Line == line {
+			// If the comment is not for the current commit, we need to recalculate the line number
-				comments[n] = comment
+			hunks, ok := hunksCache[commentCommitID+".."+dstCommitID]
-				n++
+			if !ok {
+				hunks, err = git.GetAffectedHunksForTwoCommitsSpecialFile(ctx, repo.RepoPath(), commentCommitID, dstCommitID, treePath)
+				if err != nil {
+					return nil, fmt.Errorf("GetAffectedHunksForTwoCommitsSpecialFile[%s, %s, %s]: %w", repo.FullName(), commentCommitID, dstCommitID, err)
+				}
+				hunksCache[commentCommitID+".."+dstCommitID] = hunks
 			}
-			continue
+			comment.Line = ReCalculateLineNumber(hunks, comment.Line)
 		}
 
-		// If the comment is not for the current commit, we need to recalculate the line number
+		if comment.Line == line {
-		hunks, ok := hunksCache[comment.CommitSHA+".."+dstCommitID]
+			commentAfterCommit, err := gitRepo.GetCommit(comment.CommitSHA)
-		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)
+				return nil, fmt.Errorf("GetCommit[%s]: %w", comment.CommitSHA, err)
 			}
-			hunksCache[comment.CommitSHA+".."+dstCommitID] = hunks
-		}
 
-		comment.Line = ReCalculateLineNumber(hunks, comment.Line)
-		if comment.Line != 0 {
-			dstCommit, err := gitRepo.GetCommit(dstCommitID)
-			if err != nil {
-				return nil, fmt.Errorf("GetCommit[%s]: %w", dstCommitID, err)
-			}
 			// If the comment is not the first one or the comment created before the current commit
-			if n > 0 || comment.CreatedUnix.AsTime().Before(dstCommit.Committer.When) {
+			if n > 0 || comment.CommitSHA == afterCommitID ||
+				commentAfterCommit.Committer.When.Before(afterCommit.Committer.When) {
 				comments[n] = comment
 				n++
 			}