Fix the wrong push commits in the pull request when force push (#36914)

Fix #36905 The changes focus on force-push PR timeline handling and commit range calculation: - Reworked pull-request push comment creation to use a new `gitrepo.GetCommitIDsBetweenReverse` helper, with special handling for force pushes (merge-base based range, tolerate missing/invalid old commits, and keep force-push timeline entries). - Added `Comment.GetPushActionContent` to parse push comment payloads and used it to delete only non-force-push push comments during force pushes. - Removed the old `Repository.CommitsBetweenNotBase` helper from `modules/git/repo_commit.go` in favor of the new commit ID range helper. - Added tests for `GetCommitIDsBetweenReverse` (normal range, `notRef` filtering, fallback branch usage) and expanded pull comment tests to cover force-push edge cases. <img width="989" height="563" alt="image" src="https://github.com/user-attachments/assets/a01e1bc2-fa8a-4028-8a35-d484e601ff3b" /> --------- Signed-off-by: Lunny Xiao <xiaolunwen@gmail.com> Signed-off-by: wxiaoguang <wxiaoguang@gmail.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
2026-04-04 16:27:57 -07:00
parent 3c17daf615
commit f59d1d3cef
9 changed files with 333 additions and 246 deletions
@@ -5,117 +5,166 @@ package pull

 import (
 	"context"
+	"slices"

 	"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/container"
+	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/gitrepo"
 	"code.gitea.io/gitea/modules/json"
 	"code.gitea.io/gitea/modules/log"
 )

-// getCommitIDsFromRepo get commit IDs from repo in between oldCommitID and newCommitID
-// Commit on baseBranch will skip
-func getCommitIDsFromRepo(ctx context.Context, repo *repo_model.Repository, oldCommitID, newCommitID, baseBranch string) (commitIDs []string, err error) {
-	gitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, repo)
+const maxPushCommitsInCommentCount = 1000
+
+func preparePushPullCommentPushActionContent(ctx context.Context, pr *issues_model.PullRequest, oldCommitID, newCommitID string, isForcePush bool) (data issues_model.PushActionContent, shouldCreate bool, err error) {
+	if isForcePush {
+		// if it's a force push, we need to get the whole pull request commits
+		// the force-push timeline comment should always be created, so all errors are ignored and logged only.
+		mergeBase, err := gitrepo.MergeBase(ctx, pr.BaseRepo, pr.BaseBranch, newCommitID)
+		if err != nil {
+			log.Debug("MergeBase %q..%q failed: %v", pr.BaseBranch, newCommitID, err)
+		} else {
+			data.CommitIDs, err = gitrepo.GetCommitIDsBetweenReverse(ctx, pr.BaseRepo, mergeBase, newCommitID, "", maxPushCommitsInCommentCount)
+			if err != nil {
+				log.Debug("GetCommitIDsBetweenReverse %q..%q failed: %v", mergeBase, newCommitID, err)
+			}
+		}
+		return data, true, nil
+	}
+
+	// for a normal push, it maybe an empty pull request, only non-empty pull request need to create push comment
+	data.CommitIDs, err = gitrepo.GetCommitIDsBetweenReverse(ctx, pr.BaseRepo, oldCommitID, newCommitID, pr.BaseBranch, maxPushCommitsInCommentCount)
+	return data, len(data.CommitIDs) > 0, err
+}
+
+func reconcileOldCommitCommentsForForcePush(ctx context.Context, oldCommitComments []*issues_model.Comment, newData *issues_model.PushActionContent) (needDeleteCommentIDs []int64) {
+	newPushCommitIDMaps := container.SetOf(newData.CommitIDs...)
+	for _, oldCommitComment := range oldCommitComments {
+		oldData, err := oldCommitComment.GetPushActionContent()
+		if err != nil {
+			continue
+		}
+		if oldData.IsForcePush {
+			// old comment is for force push, it should be kept
+			continue
+		}
+
+		// remove the old comment's commit IDs which are not in the new "force" push
+		oldData.CommitIDs = slices.DeleteFunc(oldData.CommitIDs, func(oldCommitID string) bool { return !newPushCommitIDMaps.Contains(oldCommitID) })
+		// if old comment doesn't contain any commit ID after the force push, then it can be deleted
+		if len(oldData.CommitIDs) == 0 {
+			needDeleteCommentIDs = append(needDeleteCommentIDs, oldCommitComment.ID)
+			continue
+		}
+		// remove new comment's commit IDs which are already in old comment
+		for _, oldCommitID := range oldData.CommitIDs {
+			newData.CommitIDs = slices.DeleteFunc(newData.CommitIDs, func(newCommitID string) bool { return newCommitID == oldCommitID })
+		}
+
+		// update the old comment's content (the commit IDs have been changed)
+		updatedOldContent, _ := json.Marshal(oldData)
+		_, err = db.GetEngine(ctx).ID(oldCommitComment.ID).Cols("content").NoAutoTime().Update(&issues_model.Comment{Content: string(updatedOldContent)})
+		if err != nil {
+			log.Error("Update Comment content failed: %v", err)
+		}
+	}
+	return needDeleteCommentIDs
+}
+
+func cleanUpOldCommitCommentsForNewForcePush(ctx context.Context, pr *issues_model.PullRequest, data *issues_model.PushActionContent) error {
+	// All old non-force-push commit comments will be deleted if they are not in the new commit list.
+	var oldCommitComments []*issues_model.Comment
+	err := db.GetEngine(ctx).Table("comment").
+		Where("issue_id = ?", pr.IssueID).And("type = ?", issues_model.CommentTypePullRequestPush).
+		Find(&oldCommitComments)
 	if err != nil {
-		return nil, err
-	}
-	defer closer.Close()
-
-	oldCommit, err := gitRepo.GetCommit(oldCommitID)
-	if err != nil {
-		return nil, err
+		return err
 	}

-	newCommit, err := gitRepo.GetCommit(newCommitID)
-	if err != nil {
-		return nil, err
+	needDeleteCommentIDs := reconcileOldCommitCommentsForForcePush(ctx, oldCommitComments, data)
+	if len(needDeleteCommentIDs) == 0 {
+		return nil
 	}
-
-	// Find commits between new and old commit excluding base branch commits
-	commits, err := gitRepo.CommitsBetweenNotBase(newCommit, oldCommit, baseBranch)
-	if err != nil {
-		return nil, err
-	}
-
-	commitIDs = make([]string, 0, len(commits))
-	for i := len(commits) - 1; i >= 0; i-- {
-		commitIDs = append(commitIDs, commits[i].ID.String())
-	}
-
-	return commitIDs, err
+	_, err = db.GetEngine(ctx).In("id", needDeleteCommentIDs).Delete(&issues_model.Comment{})
+	return err
 }

 // CreatePushPullComment create push code to pull base comment
-func CreatePushPullComment(ctx context.Context, pusher *user_model.User, pr *issues_model.PullRequest, oldCommitID, newCommitID string, isForcePush bool) (comment *issues_model.Comment, err error) {
-	if pr.HasMerged || oldCommitID == "" || newCommitID == "" {
-		return nil, nil //nolint:nilnil // return nil because no comment needs to be created
+func CreatePushPullComment(ctx context.Context, pusher *user_model.User, pr *issues_model.PullRequest, oldRef, newRef string, isForcePush bool) (comment *issues_model.Comment, created bool, err error) {
+	if pr.HasMerged || oldRef == "" || newRef == "" {
+		return nil, false, nil
 	}

-	opts := &issues_model.CreateCommentOptions{
-		Type:  issues_model.CommentTypePullRequestPush,
-		Doer:  pusher,
-		Repo:  pr.BaseRepo,
-		Issue: pr.Issue,
-	}
-
-	var data issues_model.PushActionContent
-	data.CommitIDs, err = getCommitIDsFromRepo(ctx, pr.BaseRepo, oldCommitID, newCommitID, pr.BaseBranch)
+	gitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, pr.BaseRepo)
 	if err != nil {
-		// For force-push events, a missing/unreachable old commit should not prevent
-		// deleting stale push comments or creating the force-push timeline entry.
-		if !isForcePush {
-			return nil, err
-		}
-		log.Error("getCommitIDsFromRepo: %v", err)
+		return nil, false, err
 	}
-	// It maybe an empty pull request. Only non-empty pull request need to create push comment
-	// for force push, we always need to delete the old push comment so don't return here.
-	if len(data.CommitIDs) == 0 && !isForcePush {
-		return nil, nil //nolint:nilnil // return nil because no comment needs to be created
+	defer closer.Close()
+
+	oldCommitID := oldRef
+	if !git.IsEmptyCommitID(oldRef) {
+		oldCommitID, err = gitRepo.GetRefCommitID(oldRef)
+		if err != nil {
+			return nil, false, err
+		}
+	}
+	newCommitID, err := gitRepo.GetRefCommitID(newRef)
+	if err != nil {
+		return nil, false, err
 	}

-	return db.WithTx2(ctx, func(ctx context.Context) (*issues_model.Comment, error) {
+	data, shouldCreate, err := preparePushPullCommentPushActionContent(ctx, pr, oldCommitID, newCommitID, isForcePush)
+	if !shouldCreate {
+		return nil, false, err
+	}
+
+	comment, err = db.WithTx2(ctx, func(ctx context.Context) (comment *issues_model.Comment, err error) {
 		if isForcePush {
-			// Push commits comment should not have history, cross references, reactions and other
-			// plain comment related records, so that we just need to delete the comment itself.
-			if _, err := db.GetEngine(ctx).Where("issue_id = ?", pr.IssueID).
-				And("type = ?", issues_model.CommentTypePullRequestPush).
-				NoAutoCondition().
-				Delete(new(issues_model.Comment)); err != nil {
-				return nil, err
+			err := cleanUpOldCommitCommentsForNewForcePush(ctx, pr, &data)
+			if err != nil {
+				log.Error("CleanUpOldCommitComments failed: %v", err)
 			}
 		}

 		if len(data.CommitIDs) > 0 {
-			dataJSON, err := json.Marshal(data)
-			if err != nil {
-				return nil, err
+			// if the push has commit IDs, add a "normal push" commit comment
+			dataJSON, _ := json.Marshal(data)
+			opts := &issues_model.CreateCommentOptions{
+				Type:    issues_model.CommentTypePullRequestPush,
+				Doer:    pusher,
+				Repo:    pr.BaseRepo,
+				Issue:   pr.Issue,
+				Content: string(dataJSON),
 			}
-			opts.Content = string(dataJSON)
 			comment, err = issues_model.CreateComment(ctx, opts)
 			if err != nil {
 				return nil, err
 			}
 		}

-		if isForcePush { // if it's a force push, we need to add a force push comment
-			data.CommitIDs = []string{oldCommitID, newCommitID}
-			data.IsForcePush = true
-			dataJSON, err := json.Marshal(data)
-			if err != nil {
-				return nil, err
+		if isForcePush {
+			// if it's a force push, we need to add a force push comment
+			forcePushDataJSON, _ := json.Marshal(&issues_model.PushActionContent{IsForcePush: true, CommitIDs: []string{oldCommitID, newCommitID}})
+			opts := &issues_model.CreateCommentOptions{
+				Type:    issues_model.CommentTypePullRequestPush,
+				Doer:    pusher,
+				Repo:    pr.BaseRepo,
+				Issue:   pr.Issue,
+				Content: string(forcePushDataJSON),
+
+				// It seems the field is unnecessary anymore because PushActionContent includes IsForcePush field.
+				// However, it can't be simply removed.
+				IsForcePush: true, // See the comment of "Comment.IsForcePush"
 			}
-			opts.Content = string(dataJSON)
-			opts.IsForcePush = true // FIXME: it seems the field is unnecessary any more because PushActionContent includes IsForcePush field
 			comment, err = issues_model.CreateComment(ctx, opts)
 			if err != nil {
 				return nil, err
 			}
 		}
-
-		return comment, err
+		return comment, nil
 	})
+	return comment, comment != nil, err
 }