forks/forgejo
1
0
Fork 0
Code Releases Activity

Fix pull request update showing too many commits with multiple branches (#22856)

Browse source 
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.
This commit is contained in:
Brecht Van Lommel 2023-03-09 19:14:22 +01:00 committed by GitHub
parent 689770c928
commit 8bdc0acf97
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
5 changed files with 53 additions and 103 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

13
modules/git/commit.go
View file

@ -218,6 +218,19 @@ func (c *Commit) HasPreviousCommit(commitHash SHA1) (bool, error) {
return false, err
}
// IsForcePush returns true if a push from oldCommitHash to this is a force push
func (c *Commit) IsForcePush(oldCommitID string) (bool, error) {
if oldCommitID == EmptySHA {
return false, nil
}
oldCommit, err := c.repo.GetCommit(oldCommitID)
if err != nil {
return false, err
}
hasPreviousCommit, err := c.HasPreviousCommit(oldCommit.ID)
return !hasPreviousCommit, err
}
// CommitsBeforeLimit returns num commits before current revision
func (c *Commit) CommitsBeforeLimit(num int) ([]*Commit, error) {
return c.repo.getCommitsBeforeLimit(c.ID, num)

21
modules/git/repo_commit.go
View file

@ -323,6 +323,27 @@ func (repo *Repository) CommitsBetweenLimit(last, before *Commit, limit, skip in
return repo.parsePrettyFormatLogToList(bytes.TrimSpace(stdout))
}
// CommitsBetweenNotBase 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) CommitsBetweenNotBase(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()).AddOptionValues("--not", baseBranch).RunStdBytes(&RunOpts{Dir: repo.Path})
} else {
stdout, _, err = NewCommand(repo.Ctx, "rev-list").AddDynamicArguments(before.ID.String()+".."+last.ID.String()).AddOptionValues("--not", 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()).AddOptionValues("--not", 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)

18
modules/repository/push.go
View file

@ -4,10 +4,8 @@
package repository
import (
"context"
"strings"
repo_model "code.gitea.io/gitea/models/repo"
"code.gitea.io/gitea/modules/git"
)
@ -96,19 +94,3 @@ func (opts *PushUpdateOptions) RefName() string {
func (opts *PushUpdateOptions) RepoFullName() string {
return opts.RepoUserName + "/" + opts.RepoName
}
// IsForcePush detect if a push is a force push
func IsForcePush(ctx context.Context, opts *PushUpdateOptions) (bool, error) {
if !opts.IsUpdateBranch() {
return false, nil
}
output, _, err := git.NewCommand(ctx, "rev-list", "--max-count=1").AddDynamicArguments(opts.OldCommitID, "^"+opts.NewCommitID).
RunStdString(&git.RunOpts{Dir: repo_model.RepoPath(opts.RepoUserName, opts.RepoName)})
if err != nil {
return false, err
} else if len(output) > 0 {
return true, nil
}
return false, nil
}

98
services/pull/comment.go
View file

@ -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
}
if isForcePush {
commitIDs = make([]string, 2)
commitIDs[0] = oldCommitID
commitIDs[1] = newCommitID
return commitIDs, isForcePush, err
}
// 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))
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

6
services/repository/push.go
View file

@ -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))