aboutsummaryrefslogtreecommitdiff
path: root/services
diff options
context:
space:
mode:
authorBrecht Van Lommel2023-03-09 19:14:22 +0100
committerGitHub2023-03-09 12:14:22 -0600
commit8bdc0acf97f2fce504bf43ee3843be3c638dfd2f (patch)
tree96793bcab1db39bbfa4675fa43fcfb3c43a22629 /services
parent689770c928ed46725e72ddaa2e988c52e44c2975 (diff)
Fix pull request update showing too many commits with multiple branches (#22856)
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 mechansim 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.
Diffstat (limited to 'services')
-rw-r--r--services/pull/comment.go90
-rw-r--r--services/repository/push.go6
2 files changed, 15 insertions, 81 deletions
diff --git a/services/pull/comment.go b/services/pull/comment.go
index 068aca6cd..933ad09a8 100644
--- a/services/pull/comment.go
+++ b/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,33 @@ func getCommitIDsFromRepo(ctx context.Context, repo *repo_model.Repository, oldC
return nil, false, err
}
- if err = oldCommit.LoadBranchName(); err != nil {
- return nil, false, err
- }
-
- if len(oldCommit.Branch) == 0 {
- 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)
+ isForcePush, err = newCommit.IsForcePush(oldCommitID)
if err != nil {
return nil, false, err
}
- commitIDs = make([]string, 0, len(commits))
- commitChecks := make(map[string]*commitBranchCheckItem)
+ if isForcePush {
+ commitIDs = make([]string, 2)
+ commitIDs[0] = oldCommitID
+ commitIDs[1] = newCommitID
- for _, commit := range commits {
- commitChecks[commit.ID.String()] = &commitBranchCheckItem{
- Commit: commit,
- Checked: false,
- }
+ return commitIDs, isForcePush, err
}
- if err = commitBranchCheck(gitRepo, newCommit, oldCommitID, baseBranch, commitChecks); err != nil {
- return
+ // Find commits between new and old commit exclusing base branch commits
+ commits, err := gitRepo.CommitsBetweenNotBase(newCommit, oldCommit, baseBranch)
+ if err != nil {
+ return nil, false, err
}
+ commitIDs = make([]string, 0, len(commits))
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
diff --git a/services/repository/push.go b/services/repository/push.go
index cdf030396..4b574e344 100644
--- a/services/repository/push.go
+++ b/services/repository/push.go
@@ -206,12 +206,12 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error {
return fmt.Errorf("newCommit.CommitsBeforeUntil: %w", err)
}
- isForce, err := repo_module.IsForcePush(ctx, opts)
+ isForcePush, err := newCommit.IsForcePush(opts.OldCommitID)
if err != nil {
- log.Error("isForcePush %s:%s failed: %v", repo.FullName(), branch, err)
+ log.Error("IsForcePush %s:%s failed: %v", repo.FullName(), branch, err)
}
- if isForce {
+ if isForcePush {
log.Trace("Push %s is a force push", opts.NewCommitID)
cache.Remove(repo.GetCommitsCountCacheKey(opts.RefName(), true))