Fix pull request update showing too many commits with multiple branches

When the base repository contains multiple branches with the same commits as
the base branch, pull requests can show a long list of commits already in the
base branch as having been added.

What this is supposed to do is exclude commits already in the base branch. But
the mechanism to do so assumed a commit only exists in a single branch. Now use
`git rev-list A B --not branchName` instead of filtering commits afterwards.

The logic to detect if there was a force push also was wrong for multiple
branches. If the old commit existed in any branch in the base repository it
would assume there was no force push. Instead check if the old commit is an
ancestor of the new commit.

Author: brechtvl

modules/git/repo_commit.go

@@ -323,6 +323,28 @@ func (repo *Repository) CommitsBetweenLimit(last, before *Commit, limit, skip in
 	return repo.parsePrettyFormatLogToList(bytes.TrimSpace(stdout))
 }
 
+// CommitsBetween returns a list that contains commits between [before, last),
+// excluding commits in baseBranch.
+// If before is detached (removed by reset + push) it is not included.
+func (repo *Repository) CommitsBetweenSkipBase(last, before *Commit, baseBranch string) ([]*Commit, error) {
+	var stdout []byte
+	var err error
+	if before == nil {
+		stdout, _, err = NewCommand(repo.Ctx, "rev-list").AddDynamicArguments(last.ID.String()).AddArguments("--not").AddDynamicArguments(baseBranch).RunStdBytes(&RunOpts{Dir: repo.Path})
+	} else {
+		stdout, _, err = NewCommand(repo.Ctx, "rev-list").AddDynamicArguments(before.ID.String() + ".." + last.ID.String()).AddArguments("--not").AddDynamicArguments(baseBranch).RunStdBytes(&RunOpts{Dir: repo.Path})
+		if err != nil && strings.Contains(err.Error(), "no merge base") {
+			// future versions of git >= 2.28 are likely to return an error if before and last have become unrelated.
+			// previously it would return the results of git rev-list before last so let's try that...
+			stdout, _, err = NewCommand(repo.Ctx, "rev-list").AddDynamicArguments(before.ID.String(), last.ID.String()).AddArguments("--not").AddDynamicArguments(baseBranch).RunStdBytes(&RunOpts{Dir: repo.Path})
+		}
+	}
+	if err != nil {
+		return nil, err
+	}
+	return repo.parsePrettyFormatLogToList(bytes.TrimSpace(stdout))
+}
+
 // CommitsBetweenIDs return commits between twoe commits
 func (repo *Repository) CommitsBetweenIDs(last, before string) ([]*Commit, error) {
 	lastCommit, err := repo.GetCommit(last)

services/pull/comment.go

@@ -14,58 +14,6 @@ import (
 	issue_service "code.gitea.io/gitea/services/issue"
 )
 
-type commitBranchCheckItem struct {
-	Commit  *git.Commit
-	Checked bool
-}
-
-func commitBranchCheck(gitRepo *git.Repository, startCommit *git.Commit, endCommitID, baseBranch string, commitList map[string]*commitBranchCheckItem) error {
-	if startCommit.ID.String() == endCommitID {
-		return nil
-	}
-
-	checkStack := make([]string, 0, 10)
-	checkStack = append(checkStack, startCommit.ID.String())
-
-	for len(checkStack) > 0 {
-		commitID := checkStack[0]
-		checkStack = checkStack[1:]
-
-		item, ok := commitList[commitID]
-		if !ok {
-			continue
-		}
-
-		if item.Commit.ID.String() == endCommitID {
-			continue
-		}
-
-		if err := item.Commit.LoadBranchName(); err != nil {
-			return err
-		}
-
-		if item.Commit.Branch == baseBranch {
-			continue
-		}
-
-		if item.Checked {
-			continue
-		}
-
-		item.Checked = true
-
-		parentNum := item.Commit.ParentCount()
-		for i := 0; i < parentNum; i++ {
-			parentCommit, err := item.Commit.Parent(i)
-			if err != nil {
-				return err
-			}
-			checkStack = append(checkStack, parentCommit.ID.String())
-		}
-	}
-	return nil
-}
-
 // getCommitIDsFromRepo get commit IDs from repo in between oldCommitID and newCommitID
 // isForcePush will be true if oldCommit isn't on the branch
 // Commit on baseBranch will skip
@@ -82,47 +30,34 @@ func getCommitIDsFromRepo(ctx context.Context, repo *repo_model.Repository, oldC
 		return nil, false, err
 	}
 
-	if err = oldCommit.LoadBranchName(); err != nil {
+	newCommit, err := gitRepo.GetCommit(newCommitID)
+	if err != nil {
+		return nil, false, err
+	}
+
+	// If old commit is not an ancestor of new commits, it's a force push
+	isAncestor, err := newCommit.HasPreviousCommit(oldCommit.ID)
+	if err != nil {
 		return nil, false, err
 	}
 
-	if len(oldCommit.Branch) == 0 {
+	if !isAncestor {
 		commitIDs = make([]string, 2)
 		commitIDs[0] = oldCommitID
 		commitIDs[1] = newCommitID
 
 		return commitIDs, true, err
 	}
 
-	newCommit, err := gitRepo.GetCommit(newCommitID)
-	if err != nil {
-		return nil, false, err
-	}
-
-	commits, err := newCommit.CommitsBeforeUntil(oldCommitID)
+	// Find commits between new and old commit exclusing base branch commits
+	commits, err := gitRepo.CommitsBetweenSkipBase(newCommit, oldCommit, baseBranch)
 	if err != nil {
 		return nil, false, err
 	}
 
 	commitIDs = make([]string, 0, len(commits))
-	commitChecks := make(map[string]*commitBranchCheckItem)
-
-	for _, commit := range commits {
-		commitChecks[commit.ID.String()] = &commitBranchCheckItem{
-			Commit:  commit,
-			Checked: false,
-		}
-	}
-
-	if err = commitBranchCheck(gitRepo, newCommit, oldCommitID, baseBranch, commitChecks); err != nil {
-		return
-	}
-
 	for i := len(commits) - 1; i >= 0; i-- {
-		commitID := commits[i].ID.String()
-		if item, ok := commitChecks[commitID]; ok && item.Checked {
-			commitIDs = append(commitIDs, commitID)
-		}
+		commitIDs = append(commitIDs, commits[i].ID.String())
 	}
 
 	return commitIDs, isForcePush, err