Fix bug

models/issues/comment_test.go

@@ -71,9 +71,14 @@ func TestFetchCodeComments(t *testing.T) {
 	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))
+	fourthLineComments := []*issues_model.Comment{}
-	assert.Len(t, res["README.md"][4], 1)
+	for _, comment := range res["README.md"] {
-	assert.Equal(t, int64(4), res["README.md"][0].ID)
+		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.Repo, issue.ID, user2, false)

models/issues/pull.go

@@ -392,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)
 }

routers/web/repo/pull_review.go

@@ -7,11 +7,13 @@ 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"
@@ -170,20 +172,50 @@ func renderConversation(ctx *context.Context, comment *issues_model.Comment, ori
 		ctx.ServerError("comment.Issue.LoadPullRequest", err)
 		return
 	}
-	if beforeCommitID == "" {
+
-		beforeCommitID = comment.Issue.PullRequest.MergeBase
+	headCommitID, err := ctx.Repo.GitRepo.GetRefCommitID(comment.Issue.PullRequest.GetGitHeadRefName())
+	if err != nil {
+		ctx.ServerError("GetRefCommitID", err)
+		return
 	}
-	if afterCommitID == "" {
+
-		var err error
+	prCommitIDs, err := git.CommitIDsBetween(ctx, ctx.Repo.GitRepo.Path, comment.Issue.PullRequest.MergeBase, headCommitID)
-		afterCommitID, err = ctx.Repo.GitRepo.GetRefCommitID(comment.Issue.PullRequest.GetGitHeadRefName())
+	if err != nil {
-		if err != nil {
+		ctx.ServerError("CommitIDsBetween", err)
-			ctx.ServerError("GetRefCommitID", err)
+		return
+	}
+
+	if beforeCommitID == "" || beforeCommitID == comment.Issue.PullRequest.MergeBase {
+		beforeCommitID = comment.Issue.PullRequest.MergeBase
+	} else {
+		// beforeCommitID must be one of the pull request commits
+		if !slices.Contains(prCommitIDs, beforeCommitID) {
+			ctx.HTTPError(http.StatusBadRequest, fmt.Sprintf("beforeCommitID[%s] is not a valid pull request commit", beforeCommitID))
 			return
 		}
+
+		beforeCommit, err := ctx.Repo.GitRepo.GetCommit(beforeCommitID)
+		if err != nil {
+			ctx.ServerError("GetCommit", err)
+			return
+		}
+
+		beforeCommit, err = beforeCommit.Parent(0)
+		if err != nil {
+			ctx.ServerError("GetParent", err)
+			return
+		}
+		beforeCommitID = beforeCommit.ID.String()
+	}
+	if afterCommitID == "" {
+		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 := pull_service.FetchCodeCommentsByLine(ctx, ctx.Repo.Repository, comment.IssueID,
+	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)

services/automergequeue/automergequeue.go

@@ -39,11 +39,11 @@ func StartPRCheckAndAutoMerge(ctx context.Context, pull *issues_model.PullReques
 		return
 	}
 	defer gitRepo.Close()
-	commitID, err := gitRepo.GetRefCommitID(pull.GetGitHeadRefName())
+	headCommitID, err := gitRepo.GetRefCommitID(pull.GetGitHeadRefName())
 	if err != nil {
 		log.Error("GetRefCommitID: %v", err)
 		return
 	}
 
-	AddToQueue(pull, commitID)
+	AddToQueue(pull, headCommitID)
 }

services/convert/pull_review.go

@@ -21,6 +21,10 @@ func ToPullReview(ctx context.Context, r *issues_model.Review, doer *user_model.
 		r.Reviewer = user_model.NewGhostUser()
 	}
 
+	if err := r.Issue.LoadRepo(ctx); err != nil {
+		return nil, err
+	}
+
 	result := &api.PullReview{
 		ID:                r.ID,
 		Reviewer:          ToUser(ctx, r.Reviewer, doer),

services/issue/issue.go

@@ -204,6 +204,11 @@ func DeleteIssue(ctx context.Context, doer *user_model.User, gitRepo *git.Reposi
 		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
+			}
+		}
 	}
 
 	notify_service.DeleteIssue(ctx, doer, issue)

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
 	}

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.BaseRepo.RepoPath(), pr.GetGitMergeRefName(), pr.MergedCommitID); err != nil {
+		return false, fmt.Errorf("UpdateRef: %w", err)
+	}
+
 	if err := committer.Commit(); err != nil {
 		return false, err
 	}

services/pull/review.go

@@ -113,7 +113,12 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git.
 		defer closer.Close()
 	}
 
-	prCommitIDs, err := git.CommitIDsBetween(ctx, gitRepo.Path, beforeCommitID, afterCommitID)
+	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)
 	}
@@ -138,10 +143,7 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git.
 		beforeCommitID = beforeCommit.ID.String()
 	}
 	if afterCommitID == "" {
-		afterCommitID, err = gitRepo.GetRefCommitID(issue.PullRequest.GetGitHeadRefName())
+		afterCommitID = headCommitID
-		if err != nil {
-			return nil, fmt.Errorf("GetRefCommitID[%s]: %w", issue.PullRequest.GetGitHeadRefName(), err)
-		}
 	} 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)
 	}
@@ -520,7 +522,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, 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, startCommitID, endCommitID, treePath string, line int64, showOutdatedComments bool) (issues_model.CommentList, error) {
 	opts := issues_model.FindCommentsOptions{
 		Type:     issues_model.CommentTypeCode,
 		IssueID:  issueID,
@@ -538,7 +540,24 @@ func FetchCodeCommentsByLine(ctx context.Context, repo *repo_model.Repository, i
 	n := 0
 	hunksCache := make(map[string][]*git.HunkInfo)
 	for _, comment := range comments {
-		if comment.CommitSHA == endCommitID {
+		// 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++
@@ -547,18 +566,27 @@ func FetchCodeCommentsByLine(ctx context.Context, repo *repo_model.Repository, i
 		}
 
 		// If the comment is not for the current commit, we need to recalculate the line number
-		hunks, ok := hunksCache[comment.CommitSHA]
+		hunks, ok := hunksCache[comment.CommitSHA+".."+dstCommitID]
 		if !ok {
-			hunks, err = git.GetAffectedHunksForTwoCommitsSpecialFile(ctx, repo.RepoPath(), comment.CommitSHA, endCommitID, treePath)
+			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, endCommitID, err)
+				return nil, fmt.Errorf("GetAffectedHunksForTwoCommitsSpecialFile[%s, %s, %s]: %w", repo.FullName(), comment.CommitSHA, dstCommitID, err)
 			}
-			hunksCache[comment.CommitSHA] = hunks
+			hunksCache[comment.CommitSHA+".."+dstCommitID] = hunks
 		}
 
 		comment.Line = ReCalculateLineNumber(hunks, comment.Line)
-		comments[n] = comment
+		if comment.Line != 0 {
-		n++
+			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
 }